Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Follower-only playback capabilities are incorrectly advertised even though commands remain leader-bound.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes sync-group volume controls when a volume-less player becomes leader.
Changes:
- Aggregates supported features from all active group members.
- Preserves group-derived volume capabilities in serialized player state.
- Adds regression coverage for mixed-capability sync groups.
File summaries
| File | Description |
|---|---|
music_assistant/providers/sync_group/player.py |
Aggregates member capabilities. |
music_assistant/models/player.py |
Preserves group volume features. |
tests/providers/sync_group/test_sync_group.py |
Tests mixed leader/member capabilities. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not currently exercise the core group-feature finalization fix.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Hi David. Just marking this one as draft due to open copilot conversation so we can keep focussed on the ones that need our attention. Please mark as ready for review when you want another look. |
|
SIGH I knew I should audit my open PRs. Thank you, I'd missed that this was waiting on me. |
a0c3597 to
a426dec
Compare
Derive volume and mute support from available group members, including when a volume-less display is the sync leader. Preserve these member-derived capabilities during final PlayerState normalization and add mixed-group regression coverage.
Uniting every member feature into the group advertised enqueue, gapless playback and multi-device DSP from followers, although those commands are only ever forwarded to the sync leader. Only volume and mute are fanned out to every member, so only those are now taken from the members while a leader exists; a dormant group still derives everything from its configured members. The regression test now stores "none" as the group's volume and mute control, so it fails without the group-specific finalization in the player model instead of passing through an unconfigured mock.
Group volume and mute are fanned out to the members of the sync leader's live session, so a configured member that never joined must not add a volume control the group cannot drive. Use the same member list the commands use, which still falls back to the configured members while the group is dormant. Also correct the README on the power feature and the leader/member split, and tighten the new tests to assert the positive cases too.
Group volume and mute are always fanned out to the members and use each member's own control, so the group's own volume/mute control selection (and the min/max volume limits that hang off it) never had an effect on routing and, now that a group's features come from its members, no longer affects what is advertised either. Only the opt-in power control remains for groups. Values already stored for a group are left in place; nothing consults them anymore.
0a71ed0 to
57b530c
Compare
|
Okay, findings addressed, as well as one that I discovered while reviewing the new code - the group-level volume/mute config was wholly vestigial after this. It mostly was before (setting the volume control to 'Native' on a group was never actually doing anything, for example) but now it is completely. So now only the fake Power is at the group-level config. |
What does this implement/fix?
If you have a mixed sync group including Players and Screens, the front-end could end up in a state where the volume value was shown but the control was disabled, if one of the Screens became the group leader.
Types of changes
bugfixnew-featureenhancementnew-providerbreaking-changerefactordocumentationmaintenancecidependenciesChecklist
pre-commit run --all-filespasses.pytestpasses, and tests have been added/updated undertests/where applicable.music-assistant/modelsis linked.music-assistant/frontendis linked.