feat(player-bar): redesign right cluster (Spotify-style) + overflow defaults#18
Conversation
…ault Both features were previously hidden behind opt-in visibility toggles that, when ON, surfaced them as primary buttons. This violated the overflow-first doctrine in CLAUDE.md and left typical users unable to discover the features. Now both live in the "..." menu by default and the Settings toggle re-purposes to "pin in bar" for frequent users.
…volume Promotes the two view toggles to primary slots — they're frequent enough that hiding them behind "..." cost a click on every use. The overflow menu now only hosts sleep timer + A-B loop and auto-hides its trigger when both are pinned.
Speed is used too rarely to deserve a permanent slot — most users never touch 1×. The "..." trigger now shows a compact emerald badge when speed differs from 1× so the live indicator stays visible without opening the menu. Sleep-timer countdown still wins the badge slot when armed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR reorganizes player bar control placement by moving playback speed from a dedicated inline component into the overflow menu, refactoring that menu to host speed, A-B loop, and sleep timer controls with conditional visibility based on per-feature pin toggles, and updating all documentation and 16 locales to reflect the new "pin to bar" semantics. ChangesPlayer Bar Overflow Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/components/player/MoreActionsMenu.tsx`:
- Around line 112-113: The button and panel currently use menu semantics
(aria-haspopup="menu" and role="menu"), which is incorrect for a panel with
sliders and form fields; change these to dialog/popover semantics by setting
aria-haspopup="dialog" (or removing aria-haspopup) and replace role="menu" with
role="dialog" (and add aria-modal={true} if the panel is modal) for the
corresponding elements in MoreActionsMenu.tsx (update both the trigger element
using aria-haspopup/aria-expanded and the panel element that currently has
role="menu"); ensure focus management matches dialog semantics (focus trap or
return focus on close) after making the role change.
- Around line 275-283: The formatRemaining function currently uses Math.ceil
when converting totalSec to minutes/hours which overstates remaining time (e.g.,
1h1m becomes 2h); change the conversions for hours and minutes to use Math.floor
instead of Math.ceil while keeping totalSec = Math.max(0, Math.ceil(ms/1000))
for second-level rounding. Update the logic in formatRemaining (the branches
that compute h and m) to compute h = Math.floor(totalSec / 3600) and m =
Math.floor(totalSec / 60) so the badge rounds down once it reaches minutes or
hours.
- Around line 249-257: The custom sleep input currently only uses a placeholder
so it has no stable accessible name; update the input in MoreActionsMenu (the
form using handleCustomSleep, state customMinutes and setter setCustomMinutes)
to include a programmatic label by either adding a <label> with htmlFor matching
an id on the input (e.g., id="custom-sleep-minutes") or by adding an
aria-label/aria-labelledby attribute with a descriptive string (e.g., "Custom
sleep minutes"), and ensure the input keeps min/max/value/onChange behavior and
the form submission via handleCustomSleep continues to work.
- Around line 115-120: The trigger is being tinted green even when speed
controls are hidden because isOffSpeed is true whenever playbackSpeed !== 1;
update the condition so the emerald tint only applies when speed UI is visible:
either change how isOffSpeed is computed (e.g., include showSpeed &&
playbackSpeed !== 1) or add showSpeed to the className ternary check so the
emerald/hover styles only apply when showSpeed is true; adjust references to
isOffSpeed, playbackSpeed, and showSpeed in MoreActionsMenu (around the
className/conditional that uses isOpen, sleepArmed, showSleepInMenu, isOffSpeed)
accordingly.
In `@src/i18n/locales/de.json`:
- Around line 1128-1133: The German subtitle strings for the entries containing
"title": "Sleep-Timer in der Leiste anheften" and "title": "A-B-Schleife in der
Leiste anheften" use an escaped ASCII double-quote (\") producing „…"; update
both "subtitle" values to use proper German matching quotes „…“ (replace the
trailing \" with the Unicode closing quote so each subtitle reads e.g. 'Wenn
aus, bleibt der Timer über das Menü „…“ erreichbar'). Ensure you change the
"subtitle" fields for the Sleep-Timer and A-B-Schleife entries (the keys shown
as the surrounding objects) only.
In `@src/i18n/locales/nl.json`:
- Around line 1129-1133: The Dutch subtitles for the keys "subtitle" under the
related entries (the one ending with menu „…\" and the "showAbLoop" subtitle)
use an escaped straight double-quote (\"), causing mismatched quotes; update
both subtitle strings to use a matching closing quote style (for example change
the trailing `\"` to `“` or the matching German-style closing `“` so the menu
reads `„…“`) so both lines use the same paired quote characters (refer to the
"subtitle" entries for the loop and showAbLoop objects).
In `@src/i18n/locales/tr.json`:
- Around line 1129-1133: The two Turkish subtitle strings use mixed quote
characters (`„…\"`) and should use a single consistent quote style; update the
subtitle value that currently reads "Kapalıyken zamanlayıcıya „…\" menüsünden
erişilebilir" and the "showAbLoop" object's subtitle to use a consistent
ellipsis menu reference (e.g., Kapalıyken zamanlayıcıya "…" menüsünden
erişilebilir and Kapalıyken A-B döngüsüne "…" menüsünden erişilebilir),
replacing `„…\"` with `"…"`.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2336a931-3784-48fa-8d07-08c40467ea02
📒 Files selected for processing (23)
CLAUDE.mddocs/features/playback.mddocs/features/ui.mdsrc/components/player/MoreActionsMenu.tsxsrc/components/player/PlayerBar.tsxsrc/components/player/SpeedControl.tsxsrc/i18n/locales/ar.jsonsrc/i18n/locales/de.jsonsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/hi.jsonsrc/i18n/locales/id.jsonsrc/i18n/locales/it.jsonsrc/i18n/locales/ja.jsonsrc/i18n/locales/kr.jsonsrc/i18n/locales/nl.jsonsrc/i18n/locales/pt-BR.jsonsrc/i18n/locales/pt.jsonsrc/i18n/locales/ru.jsonsrc/i18n/locales/tr.jsonsrc/i18n/locales/zh-CN.jsonsrc/i18n/locales/zh-TW.json
💤 Files with no reviewable changes (1)
- src/components/player/SpeedControl.tsx
- Switch role="menu" → role="dialog" (panel hosts sliders + form inputs, not menuitems) and matching aria-haspopup - Round h/m countdown branches with floor instead of ceil so 1h 1m doesn't display as "2h" - Add aria-label to the custom-minutes input (sleepTimer.customAriaLabel added to all 17 locales) - Gate isOffSpeed behind showSpeed so the trigger doesn't tint green in Spotify mode when speed is hidden - Fix mismatched German/Dutch/Turkish quote characters in the show* subtitles (proper „…" pair for de/nl, "…" for tr)
…ckout/critical) The previous one-workflow design checked out a release-please PR branch inside a workflow_run context with contents: write and ran cargo check on it. CodeQL flags that as critical (alert #18, actions/untrusted-checkout/critical) because a privileged workflow_run job must never execute code from a non-default ref — the gh pr list author + branch-prefix gate is a soft filter, not a security boundary, since CodeQL treats any non-default ref as untrusted-by-default. Split into the canonical two-workflow pattern: - release-please-lockfile-build.yml (NEW, pull_request, contents: read): checks out the PR branch, runs cargo check, uploads the refreshed Cargo.lock + PR metadata as a 1-day artifact. Runs unprivileged so executing release-please's Cargo.toml / Cargo.lock through cargo can't reach secrets or push. - release-please-bump-lockfile.yml (REWRITTEN, workflow_run, contents: write): no actions/checkout of the PR branch, no cargo invocation. Downloads the artifact, validates the Cargo.lock header + size envelope, re-fetches the PR via the API to confirm it's still open, authored by github-actions[bot], on the same head SHA the artifact was built against, then pushes the lockfile via the Git Data API (createBlob -> createTree -> createCommit -> updateRef). Head-SHA check ensures we never push a Cargo.lock generated against a stale Cargo.toml — if the bot pushed a follow-up while the build was running, we skip and let the next build-workflow artifact carry the fresh lock.
Summary
Reshapes the right side of the player bar to follow the Spotify layout and the overflow-first doctrine from CLAUDE.md.
1.25×emerald badge when speed ≠ 1× so the live indicator stays visible without opening the menu. Sleep-timer countdown still wins the badge slot when armed.SpeedControl.tsxremoved (functionality folded intoMoreActionsMenu).Test plan
1.25×badge appears on "⋯".ui.show_sleep_timer = truereads as "pinned" → no behaviour regression.bun run typecheck && bun run lintpass.Summary by CodeRabbit