feat(workspace): name the first entrance time on lock-in pairs - #888
feat(workspace): name the first entrance time on lock-in pairs#888seonghobae wants to merge 10 commits into
Conversation
Replace the stub rehearsal-priorities card with role-and-section pairs so a player can see what to lock in before the room starts. Prefer high priority, then medium, then focus sections, and never turn blank or none sentinels into instructions. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Repeated verse labels no longer consume the third rehearsal-priority slot, and empty copy now points at the section roadmap instead of a no-op role click. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Clicking a named role-and-section pair selects that role and focuses the matching section so players can jump from the priority card into the part they need to lock. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Fallback focus labels now jump to the first matching section so players can still act when analysis only names a section, not a role pair. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Omit focus labels that do not match a roadmap card so a missing bridge cannot clear verse focus. Scroll the named section into view when a lock-in pair or fallback label is opened. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Show when each lock-in part starts, mark the activated pair as current, keep aria-label tokens from rewriting each other, and skip smooth roadmap scrolling when the player prefers reduced motion. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
A lock-in click now marks the same section on the song-structure timeline so players can see the entrance on both the form strip and the roadmap. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
There was a problem hiding this comment.
Stale comment
Review (
05935e02vsdevelop@acdbea63)Draft lock-in landing. Prefer this over #874 (
43cb9438) and closed #880/#882. Do not merge those in parallel. Do not mix Stem Lab (#884) or NIM (#881) onto this slice.Players can see when to lock in, not only which part. High-priority pairs stay first, unmatched focus labels are omitted so the card cannot sell a no-op, and the song-structure tile for the activated section is marked
data-focused-section.CodeRabbit CLI agent auth timed out here. This is a CodeRabbit-style review of the fetched head, not a CodeRabbit service run.
Critical: 0. Warning: 0. Info: 1.
Entrance time is the section window start (inline). That matches "open this section now." It is not a stem-onset timestamp.
Do not merge while draft. Do not merge until exact-head CI is green and a qualifying independent non-author last-push approval exists. Do not self-approve.
Sent by Cursor Automation: Fix Issues
| sectionId: section.id, | ||
| roleName, | ||
| sectionLabel, | ||
| sectionStartSeconds: section.timeRange.start |
There was a problem hiding this comment.
Info: sectionStartSeconds is the section window start, not a measured role onset. That is the right lock-in cue when the pair means "this part in this section." Keep copy and tests saying section entrance so a later pickup/dropout lane cannot silently reuse this as note-onset evidence. Not merge-blocking.
There was a problem hiding this comment.
Review — lock-in first entrance times (05935e02)
This Draft is the lock-in-first landing over #874. The written slice holds: named role/section pairs, first entrance from section.timeRange.start, honest empty copy, unmatched bridge omitted, one-pass {token} interpolation, aria-current on the activated pair, reduced-motion roadmap scroll, and the same section marked on the song-structure timeline.
Local verification: Workspace.test.tsx, SectionRoadmap.test.tsx, Workspace.priority-focus-dedup.test.tsx, and i18n/index.test.ts — 46 passed at this head. GitHub Checks were still queued/in progress on 05935e02; that is not a code blocker.
Buyer next action: Keep this Draft as the lock-in vehicle. Do not merge #874 in parallel. After required checks on this exact SHA, mark Ready so CodeRabbit can run (it skipped because this is a Draft). Do not mix #884 Stem Lab or #890 real-audio accuracy onto this branch.
Residual (not a slice blocker): the song-structure timeline paints data-focused-section but does not scrollIntoView. On a narrow cockpit or a longer form, chorus can be highlighted off-screen. The roadmap is the one destination that actually moves. Role-level entrance times are also out of scope until RehearsalRole grows an entrance field.
Merge posture: COMMENT only. Author is seonghobae. This automation does not approve or merge. Independent non-author review plus current-head required checks still have to land.
Sent by Cursor Automation: Fix Issues
| <div | ||
| key={section.id} | ||
| data-testid={`song-structure-${section.id}`} | ||
| data-focused-section={section.id === focusedSectionId ? "true" : undefined} |
There was a problem hiding this comment.
This cell gets data-focused-section and a ring, but SongStructure never calls scrollIntoView. The wrapping region is overflow-x-auto with min-w-[720px], so a lock-in click to chorus can highlight a cell the player cannot see.
SectionRoadmap already scrolls with reduced-motion auto. Mirror that here if this card is meant to be the second visible destination. Until then, CHANGELOG’s “one visible destination” is true for the roadmap only. Not a merge blocker for the written plan.


Summary
Players can now see when to lock in, not only which part. The Rehearsal Priorities card names the first entrance time on each lock-in pair and focus label, marks the activated pair as current, keeps role names from rewriting later aria-label tokens, and skips smooth roadmap scrolling when the player prefers reduced motion.
Supersession / canonical owner evidence
This work has been consolidated into the older canonical owner #874 rather than leaving two same-scope PRs open.
Immediately before consolidation:
43cb9438d6338890a218f2d3b7c4ba74199adb00;05935e02f29366b2bd240d6fb4d1bae784cb992b;status=ahead,ahead_by=2,behind_by=0, with merge-base exactly43cb9438....The #874 branch was then fast-forwarded non-forced to
05935e02f29366b2bd240d6fb4d1bae784cb992b. Fresh PR refetch confirms #874 and #888 now reference that identical exact head. Therefore every #888 source/test/doc commit is preserved on #874, and this PR is closed as superseded without semantic loss.No #888 check, review, or approval transfers to #874 merely because the code identity is now equal; #874 must generate/revalidate its own required PR-context evidence and qualifying independent last-push approval.
Security boundary
Presentation-only. Already-validated
RehearsalSongrole names, section labels, focus strings, andsection.timeRange.startvalues remain React text/control data. No dependency, lockfile, file, URL, subprocess, IPC, WebView, model, database, persistence, or new authority boundary is introduced.Test points retained on #874
0:10/0:30;aria-current="true";{sectionLabel}cannot recursively rewrite a later aria-label token;Keep #874 Draft until its own exact-head repository/central CI, exact coverage, security/SBOM/supply-chain/release, unresolved-thread, and independent non-author last-push approval gates pass.