Repository navigation
feat(frontend): per-action loading and connection gating on music controls - #1866
Conversation
📝 WalkthroughWalkthroughMusic commands now carry typed action keys, ChangesMusic control state and interaction flow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MusicPage
participant MusicControls
participant useMusicPlayer
participant useMusicCommands
MusicPage->>MusicControls: render connection and pending state
MusicControls->>useMusicPlayer: invoke action callback
useMusicPlayer->>useMusicCommands: send command with actionKey
useMusicPlayer-->>MusicPage: update pendingAction and isLoading
MusicPage-->>MusicControls: disable controls and show busy indicator
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
d11ddba to
ad9b1f1
Compare
There was a problem hiding this comment.
0 issues found across 10 files (changes from recent commits).
Auto-approved: Frontend-only UX improvement adding per-action loading and connection gating to music controls; bounded, clearly beneficial, with no operational or security tradeoffs.
Re-trigger cubic
Kimi review (
|
|
Addressed in 465a532. Guild changes now use commit-scoped tokens, in-flight commands keep their own pending state, and rollback refresh failures are shown to the user. I also added coverage for disabled controls, busy icons, both command completion orders, and the Previous action path. The full frontend suite passes with 1,029 tests. The branch is up to date with main. |
There was a problem hiding this comment.
Correcting myself first: I posted an earlier version of this review claiming a hung request would lock the UI forever. That was wrong and I've removed it. packages/frontend/src/services/api.ts:86 sets timeout: 10000 on the axios instance, so a stalled request rejects after 10s, the finally runs, and pendingAction clears. I should have checked before writing it. Sorry for the noise.
What stands after a closer read:
The test coverage here is genuinely good. Asserting exactly one .animate-spin inside the toolbar is the sort of thing that actually catches a regression, and the disconnected/pending matrix is thorough. Guarding against stale responses landing after a guild switch is a real bug fix, not busywork.
My one substantive concern is the primitive that guard is built on.
useMemo isn't an identity guarantee
const guildToken = useMemo(() => Symbol('guild'), [guildId])React's docs are explicit that useMemo is a performance optimisation and not a semantic guarantee: React reserves the right to discard a cached value and recompute it even when the dependencies haven't changed. Here the Symbol's identity isn't an optimisation, it's load-bearing for correctness.
If it ever does recompute without guildId changing:
- the
useLayoutEffectkeyed onguildTokenre-runs, callingactiveCommandsRef.current.clear(),setState(EMPTY_STATE)andsetPendingAction(null), so the visible queue empties mid-session for no reason; - every in-flight command captured the old token, so
isCurrentGuild()goes false and those commands drop their results including thefinallythat clearspendingAction.
To be straight about the confidence: I'm not aware of stable React discarding a memo in this situation today, so this is a "relying on a documented non-guarantee" problem rather than a bug I can reproduce right now. But the failure mode if it ever bites is silent and would look like "the UI randomly emptied and then froze", which is horrible to debug, and the fix is cheap. The captured guildId in the closure already tells you what you need:
const guildRef = useRef(guildId)
useLayoutEffect(() => { guildRef.current = guildId /* ...reset... */ }, [guildId])
const isCurrentGuild = () => guildRef.current === guildIdThat drops the Symbol, drops guildToken from the useCallback deps, and stops sendCommand's identity churning through useMusicCommands into every memoised child.
"per-action" in the title, global lockout in the behaviour
const canPlay = (hasTrack || isPaused) && isConnected && !pendingAction
const canTrack = hasTrack && isConnected && !pendingAction
const canAux = isConnected && !pendingActionOnly the spinner is per-action; the disabling is all-or-nothing. A slow volume or seek blocks skip, previous, stop, shuffle and repeat, and those don't actually conflict with each other. Serialising every mutation is a defensible choice and I'm not going to insist otherwise, but I want it to be a decision rather than a side effect. If it is intended, the prop name pendingAction undersells it.
Two nits:
clearError: () => setError(null)is returned withoutuseCallback, so it's a new identity every render and defeats memoisation in any consumer that takes it as a prop or dep. Stands out because the rest of this hook is careful about exactly that.activeCommands.slice().reverse().find(c => c.actionKey)means with two commands in flight the most recently registered wins the spinner, so finishing command A can move the spinner onto command B's control even though B started first. Cosmetic, and it mostly goes away if the point above changes.
Merge order: this rewrites the same useMusicPlayer.ts region as #1867 and touches Music.tsx, QueueList.tsx and the locale files, overlapping #1867 and #1864. I'll sequence the three.
On the red checks: not yours. Security was an unpassable repo-wide gate, kimi-review fails on every PR because an API key isn't set (#1877), and the npm ci errors come from a stale lockfile on main. Fixed in #1876; rebase once it lands.
|
Heads-up: the CI blockers I mentioned are fixed on What that clears:
Please rebase onto The review feedback above is separate and still stands. |
9a32603 to
33aa7da
Compare
|
Addressed in 4132c62 (squashed, rebased onto main).
|
33aa7da to
4132c62
Compare
LucasSantana-Dev
left a comment
LucasSantana-Dev
left a comment
There was a problem hiding this comment.
Review: changes-required
The cross-guild race guard is unsound for the navigate-away-and-back case, the PR's own new regression test for that exact case appears to fail against the implementation, and frontend CI never ran on this branch to catch it.
P1 — stale rollback can overwrite a fresh visit: useMusicPlayer.ts (the isCurrentGuild guard in sendCommand) compares guild IDs by string equality, so it cannot distinguish a re-visit to the same guild. Trace: on guild A, a command's optimistic update rejects and the rollback api.music.getState() is slow; user switches A → B → A; the layout effect resets state and guildRef.current is 'A' again; when the stale rollback resolves, isCurrentGuild() is true, so setState(staleData) and setError('volume: command failed') run, overwriting the fresh state and showing a phantom error. This is exactly what the new test does not let an old guild rollback update a new visit to that guild asserts against (expect(state.volume).toBe(35), expect(error).toBeNull()), so that test appears to fail as written. Fix: gate on activeCommandsRef.current.has(commandId) (the map is cleared per guild change in the useLayoutEffect, so membership means "issued during the current visit"), or use a per-visit epoch counter.
P1 — CI never executed the frontend tests: the check rollup on 4132c622 shows only label/cubic/CodeRabbit/GitGuardian/Socket; the CI/CD Pipeline frontend job (which runs npm run test --workspace=packages/frontend) is absent, likely pending maintainer approval for an external contributor. ~300 lines of new/changed tests are unverified, and per the finding above the branch is likely red. A maintainer should approve the workflow run before merge.
P2 — tested path is not the shipped path: PlaybackControls.tsx is referenced only by itself and its test (repo-wide grep); the controls users interact with are the inline duplicates in Music.tsx's NowPlayingHero. The page-level gating (hero spinners, controlsEnabled wiring) got no behavioral tests. Fix: render PlaybackControls from the page, or add page-level assertions for disconnected/busy states.
P3 (nits): hardcoded English dismiss label and new Error('Player is not connected') on a page otherwise fully localized through t(); music.progressMayBeOutdated* locale keys added but referenced nowhere; pendingAction?: string | null is an untyped string across the hook/component boundary — a union of the action keys would catch mismatches.
What's good: the rollback path now awaits the queue refresh and surfaces Queue refresh failed: ... instead of fire-and-forget; the FIFO pendingAction recompute from the insertion-ordered Map is correct for both completion orders (and both are tested); gating is thorough at the interaction layer (spacebar, drag/drop, remove, clear all respect disabled, with aria-busy/aria-disabled).
…trols Show a spinner on the control in flight and disable controls while disconnected or busy. Guard in-flight commands with a guildId ref so guild switches drop stale results without relying on useMemo identity. Stable clearError, FIFO spinner selection, intentional global lockout while any command is pending.
4132c62 to
c0445b4
Compare
|
Addressed the changes-requested review. P1 — visit-scoped guard: in-flight results now gate on Also:
Rebased onto main. 62 frontend music tests green locally. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/frontend/src/pages/Music.tsx (1)
34-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate disabled-check logic instead of reusing
controlsEnabled.
handlePlayPauseandhandleRepeatCyclere-derive!player.isConnected || player.isLoadinginline, while every other handler in this component (onPrevious,onSkip,onShuffle,onVolumeChange) uses the already-computedcontrolsEnabled. If the definition ofcontrolsEnabledchanges later, these two call sites are easy to miss.♻️ Proposed fix
const handlePlayPause = useCallback(() => { - if (!player.isConnected || player.isLoading) return + if (!controlsEnabled) return if (player.state.isPlaying) player.pause() else player.resume() - }, [player]) + }, [player, controlsEnabled]) const handleRepeatCycle = useCallback(() => { - if (!player.isConnected || player.isLoading) return + if (!controlsEnabled) return const modes: Array<'off' | 'track' | 'queue' | 'autoplay'> = [ 'off', 'track', 'queue', 'autoplay', ] const idx = modes.indexOf(player.state.repeatMode) player.setRepeatMode(modes[(idx + 1) % modes.length]) - }, [player]) + }, [player, controlsEnabled])🤖 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 `@packages/frontend/src/pages/Music.tsx` around lines 34 - 50, Update handlePlayPause and handleRepeatCycle to reuse the existing controlsEnabled value for their early-return checks instead of duplicating player.isConnected and player.isLoading logic, while preserving the current disabled behavior and callback dependencies.packages/frontend/src/components/Music/PlaybackControls.tsx (2)
68-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueToolbar
aria-disabledignorespendingAction.
aria-disabled={!isConnected}only reflects connection state, even thoughpendingActionalso globally locks every control (canPlay/canTrack/canAuxall require!pendingAction). Individual buttons already carrydisabled, so impact is limited, but the toolbar-level state could be made consistent.♻️ Suggested tweak
- aria-disabled={!isConnected} + aria-disabled={!isConnected || Boolean(pendingAction)}🤖 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 `@packages/frontend/src/components/Music/PlaybackControls.tsx` around lines 68 - 73, Update the playback controls toolbar’s aria-disabled value to reflect both disconnected and pending-action states, matching the locking conditions used by canPlay, canTrack, and canAux while preserving the existing individual button disabled behavior.
31-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
PlaybackControlscomponent inNowPlayingHero.
Music.tsxrenders its own control row insideNowPlayingHeroand does not importPlaybackControls, so the new component supportsisConnected/pendingActiononly in definition. PointNowPlayingHeroat the shared component instead of maintaining duplicate control markup and icon/busy logic.🤖 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 `@packages/frontend/src/components/Music/PlaybackControls.tsx` around lines 31 - 49, Update NowPlayingHero in Music.tsx to render the shared PlaybackControls component instead of its duplicated control row, icon markup, and busy-state logic. Import PlaybackControls and pass the existing playback state, isConnected, pendingAction, and action callbacks through its defined props; remove the redundant local controls implementation.packages/frontend/src/hooks/useMusicCommands.test.ts (1)
105-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIncomplete actionKey coverage for play/skip/shuffle/move/import.
Assertions verify the new
sendCommandactionKey argument for pause/resume/stop/volume/repeat/seek/remove/clear, but not forplay,skip,shuffle,moveTrack, orimportPlaylist. Since this file is the dedicated test surface for the new action-key contract, consider adding the missingsendCommandassertions (with actionKey) for completeness.🤖 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 `@packages/frontend/src/hooks/useMusicCommands.test.ts` around lines 105 - 165, The useMusicCommands test assertions cover actionKey values for several commands but omit play, skip, shuffle, moveTrack, and importPlaylist. Extend the relevant test cases around sendCommand to assert each command is called with its expected payload and actionKey, matching the existing assertion style.packages/frontend/src/components/Music/QueueList.tsx (1)
222-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGrip handle keeps
cursor-grabstyling even when disabled.The item container now gets
opacity-60anddraggable={!disabled}, but the adjacentGripVerticalhandle still always showscursor-grabon hover, implying draggability even though dragging is disabled.🤖 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 `@packages/frontend/src/components/Music/QueueList.tsx` around lines 222 - 229, Update the GripVertical handle styling in the queue item component to conditionally remove its cursor-grab hover behavior when disabled, matching the container’s disabled state and draggable={!disabled} behavior; preserve the grab cursor for enabled items.
🤖 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 `@packages/frontend/src/pages/Music.tsx`:
- Around line 380-386: Update the disconnected, not-busy message in the
controls-disabled branch to use the existing music.playerNotConnected
translation key instead of music.notConnectedToVoiceChannel, preserving the
commandInProgress key for busy states. Add or extend a test for the disconnected
and not-busy case if the relevant test setup is available.
---
Nitpick comments:
In `@packages/frontend/src/components/Music/PlaybackControls.tsx`:
- Around line 68-73: Update the playback controls toolbar’s aria-disabled value
to reflect both disconnected and pending-action states, matching the locking
conditions used by canPlay, canTrack, and canAux while preserving the existing
individual button disabled behavior.
- Around line 31-49: Update NowPlayingHero in Music.tsx to render the shared
PlaybackControls component instead of its duplicated control row, icon markup,
and busy-state logic. Import PlaybackControls and pass the existing playback
state, isConnected, pendingAction, and action callbacks through its defined
props; remove the redundant local controls implementation.
In `@packages/frontend/src/components/Music/QueueList.tsx`:
- Around line 222-229: Update the GripVertical handle styling in the queue item
component to conditionally remove its cursor-grab hover behavior when disabled,
matching the container’s disabled state and draggable={!disabled} behavior;
preserve the grab cursor for enabled items.
In `@packages/frontend/src/hooks/useMusicCommands.test.ts`:
- Around line 105-165: The useMusicCommands test assertions cover actionKey
values for several commands but omit play, skip, shuffle, moveTrack, and
importPlaylist. Extend the relevant test cases around sendCommand to assert each
command is called with its expected payload and actionKey, matching the existing
assertion style.
In `@packages/frontend/src/pages/Music.tsx`:
- Around line 34-50: Update handlePlayPause and handleRepeatCycle to reuse the
existing controlsEnabled value for their early-return checks instead of
duplicating player.isConnected and player.isLoading logic, while preserving the
current disabled behavior and callback dependencies.
🪄 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: c7e35f4e-e121-4ff8-a082-1d0a40528c12
📒 Files selected for processing (13)
packages/frontend/src/components/Music/ImportPlaylist.tsxpackages/frontend/src/components/Music/PlaybackControls.test.tsxpackages/frontend/src/components/Music/PlaybackControls.tsxpackages/frontend/src/components/Music/QueueList.tsxpackages/frontend/src/components/Music/SearchBar.tsxpackages/frontend/src/hooks/useMusicCommands.test.tspackages/frontend/src/hooks/useMusicCommands.tspackages/frontend/src/hooks/useMusicPlayer.test.tspackages/frontend/src/hooks/useMusicPlayer.tspackages/frontend/src/locales/en.jsonpackages/frontend/src/locales/pt-BR.jsonpackages/frontend/src/pages/Music.test.tsxpackages/frontend/src/pages/Music.tsx
## Summary Two structural failures block every fork PR's required checks (seen on #1863, #1864, #1865, #1866, #1867, #1674 after their CI was approved): - **SonarCloud Scan (required) hard-fails on forks**: fork PRs get no secrets, so `SONAR_TOKEN` is never present and the token-policy step exits 1. Now the sonar job is skipped for fork PRs (a skipped required check counts as passing). Same pattern deploy-staging already uses. - **danger 403s on forks**: `review-tools.yml` ran on `pull_request`, where the fork token is forced read-only and the comment POST fails with 403. Switched to `pull_request_target`; the reusable workflow checks out and executes base-repo code only (documented in the file header, same safety rule as the other target workflows). ## Test plan - [x] actionlint clean on both files - [ ] Next push to an external contributor PR: SonarCloud Scan shows skipped, danger posts its comment After merge I will update the seven open contributor branches to main so they pick this up. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Unblocks fork PRs by fixing CI gates for SonarCloud and `danger`. Fork PRs now pass required checks without secrets and get review comments. - Bug Fixes - Skip SonarCloud Scan on fork PRs to avoid failing when `SONAR_TOKEN` is unavailable (skipped required check counts as passing). - Run review tools on `pull_request_target` so `danger` can comment on forks; workflow executes base-repo code only. <sup>Written for commit 316f567. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1898?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
|
All findings addressed and verified in the follow-up commits. Thanks for the quick turnaround.
|
A note from the maintainer side: sorry this PR waited as long as it did for a proper review, and sorry for the rounds of branch updates and re-running checks today. The churn was on our side, not yours. Also thank you for the quick turnaround on the review feedback: every finding was addressed correctly, and I have dismissed the earlier change request. Your PRs exposed real gaps in how this repo handled external contributions: CI runs sat in a silent approval queue, some gates could never pass on fork PRs (SonarCloud, danger), and the team had no notification when external PRs arrived. Those are all fixed as of today:
Your branch is up to date and the full suite is green. Thanks for the patience and for the contribution. External contributors are very welcome here. |
🤖 I have created a release *beep* *boop* --- <details><summary>2.38.0</summary> ## [2.38.0](v2.37.3...v2.38.0) (2026-07-27) ### Features * **bot:** add /ticket-setup for support category and agent role ([#1863](#1863)) ([3f4af39](3f4af39)) * **frontend:** per-action loading and connection gating on music controls ([#1866](#1866)) ([2dda60f](2dda60f)) * **frontend:** show stale progress when music SSE lags ([#1867](#1867)) ([4952e73](4952e73)) * **music:** surface recommendationReason in nowplaying and queue ([#1864](#1864)) ([960fd62](960fd62)) * **ops:** blue/green zero-downtime deploys — Phase 1 web tier ([#1786](#1786)) ([f5f7597](f5f7597)) ### Bug Fixes * **docker:** make compose stack boot from a fresh .env ([#1674](#1674)) ([babe0ef](babe0ef)) * **docker:** treat an empty db password as missing in compose guards ([#1881](#1881)) ([718c0ad](718c0ad)) * **frontend:** make landing page usable at mobile widths ([#1865](#1865)) ([6190350](6190350)) * **frontend:** stop hero grid columns overflowing on narrow viewports ([#1874](#1874)) ([ce5cea0](ce5cea0)) * **invite:** add /invite where cloudflare pages reads it ([#1895](#1895)) ([0528f66](0528f66)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Description
Dashboard music controls stayed clickable when the SSE stream was down, and failures only set a generic bottom-of-page error with no idea which action failed.
Fix
useMusicPlayer/useMusicCommands: each command passes anactionKey;pendingActiontracks the in-flight action and errors are prefixed with that key.PlaybackControls: optionalisConnected+pendingActionso the shared component matches the same rules (tests updated).Verification
isConnected={false}.Checklist
Destructive / irreversible interaction (Tier A)
Not applicable. Frontend control UX only.
Feature-removal sweep
Not applicable.
Fixes #1771
Summary by cubic
Add per-action loading and connection gating to music controls. Controls disable when disconnected or while a command is in flight, show which control is busy, and prevent stale rollbacks and cross-guild races. Also fixes the hero track selection. Fixes #1771.
New Features
aria-busy, and global disable viaisConnected/pendingAction; sharedBusyIcon; tests cover disconnected, pending, and single-spinner states.SearchBar,ImportPlaylist, andQueueList(drag/remove/clear) when not connected or busy; localized “working”/“player not connected”; dismissible error viaclearError.useMusicCommandspassesactionKey;useMusicPlayertrackspendingActionwith FIFO spinner selection, enforces global lockout during any command, resets loading/pending/error on guild switch, and exposes stableclearError.Bug Fixes
pendingAction; surface rollback queue refresh failures in the action-scoped error.state.currentTrackor falls back tostate.tracks[0]for older payloads.Written for commit 5b4f8af. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes