Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Alexa player provider with several significant changes:
Changes:
- Changed default API URL port from 3000 to 5000
- Added new API request infrastructure with session management and basic auth support
- Refactored playback control methods (stop, play, pause) to use Alexa intents instead of direct API calls
- Implemented intent caching mechanism to improve performance
- Enhanced metadata handling in play_media method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| async def api_request( | ||
| provider: Any, |
There was a problem hiding this comment.
The provider parameter is typed as Any, which provides no type safety. Consider using a more specific type such as AlexaProvider or a Protocol that defines the required interface (config.get_value and mass.http_session). This would improve type safety and IDE support.
There was a problem hiding this comment.
Please use appropriate type here
There was a problem hiding this comment.
Should be the appropriate type now
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| url = f"{api_url.rstrip('/')}/{endpoint.lstrip('/')}" | ||
|
|
||
| mass_session = None | ||
| try: |
There was a problem hiding this comment.
I think you can just rely on mass.http_session to be there. No need for all this fallback logic.
There was a problem hiding this comment.
Should be removed now
| auth = BasicAuth(str(username), str(password)) | ||
|
|
||
| stream_url = await self.provider.mass.streams.resolve_stream_url(self.player_id, media) | ||
| # Prefer the player's current_media (may contain enriched metadata), |
There was a problem hiding this comment.
Why prefer the current metadata? If MA is playing track A and you request play_media with Track B, the metadata of track B will be in media. I think you should just use media.xxx or "" here ?
There was a problem hiding this comment.
I thought that would be the case but when I was testing, media didn't contain relevant metadata. Album and artist were null and title was set to "Music Assistant" and image_url was the Music Assistant logo.
If there is some other field that contains the relevant metadata, I can update the assignments, or if there is a bug in the provider somewhere I could address that.
There was a problem hiding this comment.
This is due to flow mode playing as 1 big continuous stream, so play_media is only called once in those cases. If you want rich metadata in flow mode, you need an endpoint on the player side that allows you to just update the metadata. You can implement _on_player_media_updated for this. Have a look at the Airplay or Chromecast provider as an example.
MarvinSchenkel
left a comment
There was a problem hiding this comment.
Could you please update the PR description with the goal? I miss the context to properly review the functional changes of the player commands. What is this PR improving?
Also gave some inline feedback. Please have a look, address the comments and let us know when you want us to have another look by selecting 'Ready for review' again
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state = getattr(self, "state", None) | ||
| current = getattr(state, "current_media", None) or media |
There was a problem hiding this comment.
The metadata extraction logic is overly defensive. Since state is always a property on Player instances (inherited from the Player base class), using getattr(self, "state", None) is unnecessary. The code should directly access self.state.current_media as done in other providers (e.g., airplay/player.py:621, roku_media_assistant/player.py:312, sendspin/player.py:550, squeezelite/player.py:354).
| state = getattr(self, "state", None) | |
| current = getattr(state, "current_media", None) or media | |
| current = self.state.current_media or media |
| except Exception as exc: # pylint: disable=broad-except | ||
| _LOGGER.error("Failed API request to %s: %s", url, exc) | ||
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") | ||
|
|
||
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| return await _request_with_session(session, method, url, json_data, timeout, auth) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| _LOGGER.error("Failed API request to %s: %s", url, exc) | ||
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") |
There was a problem hiding this comment.
The broad exception handler catches ActionUnavailable exceptions raised by _request_with_session and converts them to a less informative generic message. This loses the detailed error information (status code and response text) from the original exception. Consider catching ActionUnavailable separately and re-raising it to preserve the detailed error message, similar to the pattern used in _load_intents at line 547.
| except Exception as exc: # pylint: disable=broad-except | |
| _LOGGER.error("Failed API request to %s: %s", url, exc) | |
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") | |
| async with aiohttp.ClientSession() as session: | |
| try: | |
| return await _request_with_session(session, method, url, json_data, timeout, auth) | |
| except Exception as exc: # pylint: disable=broad-except | |
| _LOGGER.error("Failed API request to %s: %s", url, exc) | |
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") | |
| except ActionUnavailable: | |
| # Preserve detailed ActionUnavailable from _request_with_session | |
| raise | |
| except Exception as exc: # pylint: disable=broad-except | |
| _LOGGER.error("Failed API request to %s: %s", url, exc) | |
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") from exc | |
| async with aiohttp.ClientSession() as session: | |
| try: | |
| return await _request_with_session(session, method, url, json_data, timeout, auth) | |
| except ActionUnavailable: | |
| # Preserve detailed ActionUnavailable from _request_with_session | |
| raise | |
| except Exception as exc: # pylint: disable=broad-except | |
| _LOGGER.error("Failed API request to %s: %s", url, exc) | |
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") from exc |
| async with aiohttp.ClientSession() as session: | ||
| try: | ||
| return await _request_with_session(session, method, url, json_data, timeout, auth) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| _LOGGER.error("Failed API request to %s: %s", url, exc) | ||
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") |
There was a problem hiding this comment.
The broad exception handler catches ActionUnavailable exceptions raised by _request_with_session and converts them to a less informative generic message. This loses the detailed error information (status code and response text) from the original exception. Consider catching ActionUnavailable separately and re-raising it to preserve the detailed error message, similar to the pattern used in _load_intents at line 547.
| _LOGGER.error("Failed API request to %s: %s", url, exc) | ||
| raise ActionUnavailable("Failed to connect to the configured API endpoint.") | ||
|
|
||
| async with aiohttp.ClientSession() as session: |
There was a problem hiding this comment.
I think this is practically dead code. I cannot think of a reason that mass.http_session is None? Have you encountered this in your development.
| if mass_session: | ||
| try: | ||
| return await _request_with_session(mass_session, method, url, json_data, timeout, auth) | ||
| except Exception as exc: # pylint: disable=broad-except |
There was a problem hiding this comment.
Would be nice to narrow this down to specific Exceptions you are trying to catch here.
There was a problem hiding this comment.
I see more of those "catch all" patterns. I think that, in combination with my comments above is all that's left. Then we should be good to merge this :)
This PR should bring some improvements to the Alexa Player Provider and help bring it more in line with the current state of the Alexa skill.