Support background media slot controls - #204
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR adds primary and background media slots, playlist state, targeted Core API media commands, slot-aware connection reconciliation, playlist controls, feature gating, localized messages, and expanded tests. It also changes scroll restoration to run once per mount. ChangesSlot-aware media and playlist flow
Scroll restoration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant IndexPage
participant CoreAPI
participant StatusStore
User->>IndexPage: Select playlist control
IndexPage->>CoreAPI: Send media.control for selected slot
CoreAPI-->>IndexPage: Resolve or reject command
IndexPage->>StatusStore: Update local playlist playback state
StatusStore-->>IndexPage: Render slot-specific media controls
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/lib/models.ts (1)
345-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a literal union for
action.
actionaccepts any string. The only current caller sends"stop". A union documents the supported commands and catches typos at compile time.♻️ Optional refactor
+export type MediaControlAction = "stop"; + export interface MediaControlRequest { - action: string; + action: MediaControlAction; slot?: MediaSlot; args?: Record<string, string>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/models.ts` around lines 345 - 349, Update the MediaControlRequest interface’s action property to a string-literal union containing the currently supported "stop" command, preserving the optional slot and args fields.src/lib/store.ts (1)
230-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one exported factory for empty playback state.
The empty
PlayingResponseliteral now appears four times in this file.src/components/ConnectionProvider.tsxLine 95 defines the same object asemptyPlaying(). Export a single factory fromsrc/lib/models.tsor this file and reuse it in both places. This keeps the shape aligned ifPlayingResponsegains required fields.Also applies to: 470-479
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/store.ts` around lines 230 - 241, Centralize the empty PlayingResponse construction in one exported factory, such as emptyPlaying, instead of repeating the literal in the store and ConnectionProvider. Update the backgroundPlaying initialization and all other empty playback-state usages in the store, including the referenced later block, to call the factory, and reuse that same exported factory from ConnectionProvider.src/__tests__/unit/components/home/NowPlayingInfo.test.tsx (1)
138-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse accessible queries instead of DOM traversal for the control assertions.
Lines 142-148 use
controls.parentElementandcontrols.querySelectorAll("button"). Usewithin(controls).getAllByRole("button")and compare accessible names. This keeps the assertion aligned with the accessible-query order and removes the dependency on the current DOM nesting.♻️ Proposed refactor
const controls = screen.getByRole("group", { name: "scan.playlistControls", }); expect(controls).toBeInTheDocument(); - expect(controls.parentElement).toContainElement( - screen.getByRole("heading", { name: "scan.backgroundMediaHeading" }), - ); expect( - Array.from(controls.querySelectorAll("button")).map((button) => - button.getAttribute("aria-label"), - ), + within(controls) + .getAllByRole("button") + .map((button) => button.getAttribute("aria-label")), ).toEqual([ "scan.playlistPrevious", "scan.stopBackgroundMediaButton", "scan.playlistPause", "scan.playlistNext", ]); + expect( + screen.getByRole("heading", { name: "scan.backgroundMediaHeading" }), + ).toBeInTheDocument();Add
withinto thetest-utilsimport.As per coding guidelines: "Use accessible queries in this order:
getByRole,getByLabelText,getByText, thengetByTestId".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/unit/components/home/NowPlayingInfo.test.tsx` around lines 138 - 154, Update the control assertions in the NowPlayingInfo test to use accessible queries: import within from the existing test utilities, locate the heading independently with getByRole instead of controls.parentElement, and retrieve buttons through within(controls).getAllByRole("button"), comparing their accessible names in order rather than querying DOM elements and aria-label attributes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/unit/components/PageFrame.test.tsx`:
- Line 239: Update the test around the “Update 0” interaction to import and
initialize userEvent with userEvent.setup(), make the test asynchronous, and
replace fireEvent.click with await user.click. Preserve the existing findByRole
assertion for the post-click rerender.
In `@src/components/ConnectionProvider.tsx`:
- Around line 348-358: Update the MediaStarted notification handling in
ConnectionProvider so clearStagedToken() runs only when the resolved slot is
"primary", matching the MediaStopped branch. Continue canceling reconciliation
and refreshing media state for every recognized slot.
- Around line 104-112: Update getMediaSlot to treat null as an alias for the
primary slot, alongside undefined, empty string, and "primary"; preserve the
existing "background" mapping and null return for other unsupported values so
primary playback state and media.started notifications are handled correctly.
- Line 827: Update fetchMediaState to increment mediaStateRequestToken and
capture that request’s token before awaiting CoreAPI.media(); compare it with
the current token before calling applyMediaState, skipping stale responses. Keep
token creation inside fetchMediaState so retry attempts receive fresh tokens,
and preserve refreshMediaState’s notification-driven invalidation behavior.
In `@src/components/PageFrame.tsx`:
- Line 51: Update PageFrame’s scroll-restoration logic around hasRestoredScroll
so it tracks the router history-entry identity and resets the guard whenever
that identity changes, including same-route parameter navigations for the
edit-mapping and device-settings routes. Add a test covering navigation between
parameter values on the same route and verify scroll restoration runs for the
new history entry.
In `@src/lib/coreApi.ts`:
- Around line 1508-1523: Update mediaControl to inspect the value resolved by
this.call and reject when it indicates cancellation ({ cancelled: true}),
matching the existing guard in run(). Preserve the current success resolution
for non-cancelled results and retain logMediaApiFailure handling for rejected
API calls.
---
Nitpick comments:
In `@src/__tests__/unit/components/home/NowPlayingInfo.test.tsx`:
- Around line 138-154: Update the control assertions in the NowPlayingInfo test
to use accessible queries: import within from the existing test utilities,
locate the heading independently with getByRole instead of
controls.parentElement, and retrieve buttons through
within(controls).getAllByRole("button"), comparing their accessible names in
order rather than querying DOM elements and aria-label attributes.
In `@src/lib/models.ts`:
- Around line 345-349: Update the MediaControlRequest interface’s action
property to a string-literal union containing the currently supported "stop"
command, preserving the optional slot and args fields.
In `@src/lib/store.ts`:
- Around line 230-241: Centralize the empty PlayingResponse construction in one
exported factory, such as emptyPlaying, instead of repeating the literal in the
store and ConnectionProvider. Update the backgroundPlaying initialization and
all other empty playback-state usages in the store, including the referenced
later block, to call the factory, and reuse that same exported factory from
ConnectionProvider.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 028bd08d-1fc6-462d-aacc-318397656694
📒 Files selected for processing (19)
src/__tests__/integration/index-route.test.tsxsrc/__tests__/unit/components/ConnectionProvider.test.tsxsrc/__tests__/unit/components/PageFrame.test.tsxsrc/__tests__/unit/components/home/NowPlayingInfo.test.tsxsrc/__tests__/unit/coreApi.error-handling.test.tssrc/__tests__/unit/featureGates.test.tssrc/__tests__/unit/lib/coreApi.test.tssrc/__tests__/unit/lib/store.test.tssrc/__tests__/unit/routes/create.index.test.tsxsrc/components/ConnectionProvider.tsxsrc/components/PageFrame.tsxsrc/components/home/NowPlayingInfo.tsxsrc/components/home/StopConfirmModal.tsxsrc/lib/coreApi.tssrc/lib/featureGates.tssrc/lib/models.tssrc/lib/store.tssrc/routes/-pages/Index.tsxsrc/translations/en-US.json
1 participant
Summary
Follow-ups
Accurate native-audio pause behavior and per-slot playback status remain tracked in ZaparooProject/zaparoo-core#1172 and ZaparooProject/zaparoo-core#1173.
Ref #196
Summary by CodeRabbit
New Features
Bug Fixes