Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Current dependency pins lack the new model and UI support, and the API schema version and server test coverage need updating.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds clipboard support for Tidal device-login codes through the shared setup-flow contract.
Changes:
- Adds optional
copy_textpropagation to external setup steps. - Supplies the Tidal device code as copyable text.
- Requires companion model/frontend releases and dependency updates.
File summaries
| File | Description |
|---|---|
music_assistant/providers/tidal/setup_flow.py |
Exposes the Tidal user code for copying. |
music_assistant/models/setup_flow.py |
Propagates copyable text into setup-flow steps. |
Review details
Suppressed comments (2)
music_assistant/models/setup_flow.py:564
- [CRITICAL] This unconditionally passes a keyword that the pinned
music-assistant-models==1.1.208does not define (models#395is still open), so constructing any setup-flow step raisesTypeErroreven whencopy_textisNone; merge/release the model change and bump the server pin before this code lands.
copy_text=copy_text,
music_assistant/providers/tidal/setup_flow.py:52
- [PROBLEM] The existing Tidal setup-flow test checks the URL and
translation_paramsbut never verifies this new field, so a wiring regression would pass; addassert step.copy_text == "ABCDE"neartests/controllers/config/test_setup_flows.py:1908.
copy_text=str(device["userCode"]),
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| step_id: str = "auth", | ||
| expires_in: float | None = None, | ||
| translation_params: list[str] | None = None, | ||
| copy_text: str | None = None, |
| url=_verification_url(device), | ||
| step_id="device_login", | ||
| expires_in=float(device["expiresIn"]), | ||
| copy_text=str(device["userCode"]), |
|
The PR title needs to clearly state what the PR fixes or enables in a user facing style |
|
@afeldman1 The title or description of this pull request needs a fix:
Release notes are generated from the title, and the template carries what reviewers need, so please edit them before this is reviewed. |
…ant#6216) # What does this implement/fix? Release notes could list changes that never shipped in that release. When a commit had no merge line, the generator looked for `#<number>` anywhere in the commit message, so a pull request merely *mentioned* in a cherry-pick note or a `fixes #...` trailer was treated as shipped. That is how music-assistant#5557 ended up in the generated 2.10.2 stable notes (removed by hand before release): the backport of music-assistant#6171 carries a note saying the conflicting test file belongs to music-assistant#5557, "an enhancement that is not on stable". Only music-assistant#5557 slipped through because it merged to `dev` after the previous stable tag, which is the one case the merge-date cutoff cannot catch. - Read the pull request number from the commit title only, and share that one helper between the include and exclude paths (the exclude path already did this) - Log a warning when a commit title names no pull request, so a future change in how commits land is visible in the workflow log - Cover the case in `tests/test_generate_release_notes.py` Replaying the real 2.10.1..2.10.2 range yields the same 41 pull requests and drops all four stray body references (music-assistant#5557, music-assistant#4108, music-assistant#6268, music-assistant#6290). **Related issue (if applicable):** - related issue <link to issue> ## Types of changes <!-- Tick exactly one box. CI (.github/workflows/pr-labels.yaml) derives the label from the ticked box and applies it automatically; the release-notes generator uses that same label to slot this change into the next release notes. --> - [ ] Bugfix (non-breaking change which fixes an issue) — `bugfix` - [ ] New feature (non-breaking change which adds functionality) — `new-feature` - [ ] Enhancement to an existing feature — `enhancement` - [ ] New music/player/metadata/plugin provider — `new-provider` - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) — `breaking-change` - [ ] Refactor (no behaviour change) — `refactor` - [ ] Documentation only — `documentation` - [ ] Maintenance / chore — `maintenance` - [x] CI / workflow change — `ci` - [ ] Dependencies bump — `dependencies` ## Checklist - [x] The code change is tested and works locally. - [x] `pre-commit run --all-files` passes. - [x] `pytest` passes, and tests have been added/updated under `tests/` where applicable. - [ ] For changes to shared models, the companion PR in `music-assistant/models` is linked. - [ ] For changes affecting the UI, the companion PR in `music-assistant/frontend` is linked. - [x] I have read and complied with the project's [AI Policy](https://github.com/music-assistant/.github/blob/main/AI_POLICY.md) for any AI-assisted contributions. - [ ] I have [raised a PR against the documentation repository](https://github.com/music-assistant/music-assistant.io/blob/main/CONTRIBUTING.md) targeting the main or beta branch as appropriate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The pinned model lacks copy_text, causing setup flows to fail, and contract/version integration remains incomplete.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| reason=reason, | ||
| translation_owner=self._translation_owner, | ||
| translation_params=translation_params, | ||
| copy_text=copy_text, |
| url=_verification_url(device), | ||
| step_id="device_login", | ||
| expires_in=float(device["expiresIn"]), | ||
| copy_text=str(device["userCode"]), |
There was a problem hiding this comment.
🟡 Changes recommended
Existing unresolved reviews identify missing model/frontend dependency updates, an API schema bump, and test coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The unresolved model/frontend dependency pins, schema-version bump, and test coverage issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The frontend only renders copyable text for external steps, making the Spotify form enhancement unreachable.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| step_id="developer", | ||
| last_step=True, | ||
| translation_params=[HOSTED_CALLBACK_URL], | ||
| copy_text=HOSTED_CALLBACK_URL, |
There was a problem hiding this comment.
🟡 Changes recommended
The pinned models package cannot accept the new field, and the companion frontend does not render it for FORM steps.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
music_assistant/models/setup_flow.py:567
- [CRITICAL]
pyproject.tomlis still exact-pinned tomusic-assistant-models==1.1.209, whoseSetupFlowStephas nocopy_textargument; because_build_stepnow always passes this keyword, every setup flow will fail withTypeErrorbefore publishing its first step. Bump the models pin to 1.1.210 (the release containing models#395) in this PR.
copy_text=copy_text,
music_assistant/models/setup_flow.py:206
- [PROBLEM] This adds a serialized
SetupFlowStepfield without bumpingAPI_SCHEMA_VERSION(currently 70). Bump the schema version and gate frontend#2710 on it so API clients can feature-detect support for the new shared-model field.
copy_text=copy_text,
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| last_step: bool | None = None, | ||
| expires_in: float | None = None, | ||
| translation_params: list[str] | None = None, | ||
| copy_text: str | None = None, |
| url=url, | ||
| expires_in=expires_in, | ||
| translation_params=translation_params, | ||
| copy_text=copy_text, |
|
Marking this PR as draft so we can keep track of which PRs needs our attention. Please mark as 'Ready for review' when you want us to have another look 🙏 . |
What does this implement/fix?
Add easy to use copy button for Tidal, Spotify, and Audible setup
Related issue
music-assistant/models#395
music-assistant/frontend#2710
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.