feat(workspace): loop tonight's first section on the map - #903
feat(workspace): loop tonight's first section on the map#903seonghobae wants to merge 17 commits into
Conversation
Replace the dead Loop section coming-soon control with a rehearsal action that arms the first role or focus window and jumps to that Section Roadmap card. Timeline chips start the same loop. Isolation playback stays uninvented.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Exact-current-head @opencode-agent review Read-only review of this unchanged SHA. Do not self-approve, update the branch, or merge. Inherited #783 npm HIGH findings stay #783-owned. |
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
Merge the canonical #903 loop lane into the count-in work so the live Workspace owns and passes loopedSectionId into SectionRoadmap. Preserve the loop controls/locales and add an integration regression for a non-first loop target while scoping the Workspace timeline assertion to its region.
Keep the canonical #903 loop guidance while recording the Section Roadmap count-in, update the architecture date, and make the changelog reflect first-or-looped runtime behavior.
Preserve #903's existing test layout, scope malformed-time assertions to the timeline region, and add only the non-first-loop integration regression needed to prove Workspace passes the selected map loop into the count-in.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head3f193bf01b7f059454ba13f192865bccbf091aac. -
Head SHA:
3f193bf01b7f059454ba13f192865bccbf091aac -
Workflow run: 32045676667
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (10 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (10 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: component-contract.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: component-contract.md"]
R2 --> V2["docs review"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (12 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (12 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: component-contract.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: component-contract.md"]
R2 --> V2["docs review"]
|
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headc03d84d8f5797684136b7b4b4ca1fed90efbc7bd. -
Head SHA:
c03d84d8f5797684136b7b4b4ca1fed90efbc7bd -
Workflow run: 32090847879
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (12 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (12 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: component-contract.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: component-contract.md"]
R2 --> V2["docs review"]
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head7d143339be4a26d34ef5a636ebcad6f139722d5c. -
Head SHA:
7d143339be4a26d34ef5a636ebcad6f139722d5c -
Workflow run: 32106883076
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (12 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (12 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs: component-contract.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: component-contract.md"]
R2 --> V2["docs review"]
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
2 similar comments
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
1 similar comment
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
opencode-agent
Bot
added
area: ui-ux
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
2 similar comments
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
|
Already queued @cwl-noema-review and @opencode-agent on this exact request for PR #903 at head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| node.scrollIntoView({ | ||
| behavior: prefersReducedMotion ? "auto" : "smooth", | ||
| block: "nearest", | ||
| inline: "center" | ||
| }); | ||
| node.focus(); |
There was a problem hiding this comment.
🟡 focus() cancels the smooth roadmap scroll
After scrollIntoView starts a smooth scroll to the loop card, node.focus() runs without preventScroll, so the browser immediately re-scrolls the element into view. The instant jump overrides the smooth animation the code just requested.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (activeRole) { | ||
| const forRoleIndex = song.sections.findIndex((section) => | ||
| section.roles.some((role) => role.id === activeRole) | ||
| ); | ||
| if (forRoleIndex !== -1) { | ||
| return forRoleIndex; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Role strip loops role's first appearance, not tonight's first section
firstLoopSectionIndex returns the first section containing the active role, so a role that first enters at the chorus arms and labels the chorus rather than tonight's opening section. Tests only cover a role present in section 0, so late-entering roles are unverified.
Was this helpful? React with 👍 or 👎 to provide feedback.
| {song.sections.map((section, sectionIndex) => ( | ||
| <Card | ||
| key={section.id} | ||
| className={`w-80 flex-none shrink-0 snap-start overflow-hidden shadow-[0_18px_60px_rgba(0,0,0,0.22)] transition duration-300 hover:-translate-y-1 hover:shadow-[0_24px_80px_rgba(0,0,0,0.32)] ${ | ||
| section.confidence.level === "low" ? "border-rose-300/30 bg-rose-950/30" : "border-white/10 bg-slate-950/80" | ||
| key={`${section.id}-${sectionIndex}`} | ||
| id={`workspace-section-card-${sectionIndex}`} | ||
| tabIndex={-1} |
There was a problem hiding this comment.
📝 Info: Index-based card ids resolve the duplicate-id focus fix
Card ids and focus routing now key on renderer position rather than analysis section.id. Both SongStructure chips and SectionRoadmap cards iterate song.sections in the same order, so duplicate or whitespace-bearing analysis ids no longer misroute focus or collide.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Re-review request for exact head |
# Conflicts: # AGENTS.md # CHANGELOG.md
|
Merged current develop into this branch: resolved shared-file conflicts by including both this feature's loop-first-section callout and the newly landed #957 playable-range work (AGENTS.md/CHANGELOG unioned). No behavior dropped. |
| const armSectionLoop = (sectionIndex: number): void => { | ||
| if (!Number.isSafeInteger(sectionIndex) || sectionIndex < 0 || sectionIndex >= song.sections.length) { | ||
| return; | ||
| } | ||
| setLoopedSectionIndex(sectionIndex); | ||
| focusWorkspaceSection(sectionIndex); | ||
| }; |
There was a problem hiding this comment.
🟡 Memoization on the timeline is defeated
armSectionLoop is a fresh closure on every Workspace render and is passed as onLoopSection to the memo-wrapped SongStructure. The changing prop reference means memo never skips, so the timeline re-renders on every unrelated state change (role switch, practice-progress edit). Wrap armSectionLoop in useCallback.
Was this helpful? React with 👍 or 👎 to provide feedback.
| </Button> | ||
| <Button | ||
| type="button" | ||
| aria-disabled={true} | ||
| aria-label="Loop section coming soon" | ||
| title="Loop section coming soon" | ||
| onClick={preventUnavailableAction} | ||
| disabled={loopSectionIndex === undefined || !loopSection} | ||
| aria-label={ | ||
| loopSection | ||
| ? loopCopy(t("workspaceLoopSectionAria"), loopSection) | ||
| : t("workspaceLoopUnavailable") | ||
| } | ||
| title={ | ||
| loopSection | ||
| ? loopCopy(t("workspaceLoopSectionAria"), loopSection) | ||
| : t("workspaceLoopUnavailable") | ||
| } | ||
| onClick={() => { | ||
| if (loopSectionIndex !== undefined && loopSection) { | ||
| armSectionLoop(loopSectionIndex); | ||
| } | ||
| }} | ||
| variant="outline" | ||
| className="min-h-11 cursor-not-allowed border-white/10 bg-white/5 text-slate-400 opacity-70" | ||
| className="min-h-11 border-cyan-300/30 bg-cyan-300/10 font-semibold text-cyan-50 hover:bg-cyan-300/20 hover:text-white disabled:cursor-not-allowed disabled:border-white/10 disabled:bg-white/5 disabled:text-slate-400" | ||
| > | ||
| Loop section | ||
| {loopSection | ||
| ? loopCopy(t("workspaceLoopSectionAction"), loopSection) | ||
| : t("workspaceLoopUnavailable")} | ||
| </Button> | ||
| <Button | ||
| type="button" | ||
| aria-disabled={true} | ||
| aria-label="Solo / mute others coming soon" | ||
| title="Solo / mute others coming soon" | ||
| aria-label={t("workspaceSoloUnavailable")} | ||
| title={t("workspaceSoloUnavailable")} |
There was a problem hiding this comment.
📝 Info: Play stem and Solo stay disabled by design
Play stem and Solo / mute others remain non-functional but their labels now point at the map loop (workspacePlayStemUnavailable, workspaceSoloUnavailable) rather than "coming soon". This satisfies the repo rule against "coming soon" dead ends while honoring the rule not to invent Stem Lab isolation here. Not flagged as a violation.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Single-writer succession review against fresh live heads ( #971 already preserves renderer-owned Section Roadmap occurrence IDs/highlighting and the role-strip-to-real-player transport boundary, but #903 still has unique buyer/UI contracts that are not yet present in #971: clickable Song Structure timeline selection, focus+scroll of the matching roadmap card, reduced-motion The PR should be Draft while this succession is incomplete. The attempted Draft transition hit a GitHub connector GraphQL schema failure ( |
| /** Documented. */ | ||
| export function Workspace({ song, sourceBootstrap = null, onSongUpdate }: WorkspaceProps) { | ||
| const [activeRole, setActiveRole] = useState<string | null>(null); | ||
| const [loopedSectionIndex, setLoopedSectionIndex] = useState<number | null>(null); |
There was a problem hiding this comment.
🟡 Loaded projects inherit previous loops
After loading another project, loopedSectionIndex still selects the old numeric position. The new song falsely shows and announces that section as looped.
Prompt for agents
Reset or reconcile the loop selection when Workspace receives a different song. App.handleLoadProject replaces jobResult while Workspace remains the same mounted component, so loopedSectionIndex survives. Preserve the loop across ordinary edits to the same song if desired, but clear it when the song identity changes and ensure section deletion or reordering cannot silently transfer the selection to another section.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Succession role
Draft preservation lane for the older Song Structure / roadmap interaction implementation. Canonical Active Player ownership is #971 (
feat/rehearsal-player-first-section-loop); this branch must not restore an independent loop/transport state or fork the source-to-audible stem authority now stacked under #971.Current identity
develop@314ddeae7b775a4957594b599358c8255617eb2e.09bedd835475015379716292e63e6be376fceec9.c27f3781ddcbcc013dce07a26c0baf6080e4b2ac→ feat(player): admit and bind playable stem artifacts #1160332240dbba957602f217dc6e4e6a82a59d4d39b2.1d2e0867d473b46869b845c9d07369822e667a5aon protecteddevelop.69efb4879af72f01315bd5ca9b17a7ae60745476.feat/rehearsal-player-first-section-loop; GitHub's stored base snapshot is historical whenever the live base branch moves and must not be treated as current authority.Semantic succession
Current #971 source reconstructs the still-valid #903 buyer behavior inside one playback authority/state machine: Song Structure occurrence actions, duplicate-analysis-ID-safe renderer occurrence identity, Section Roadmap selection/focus/scroll, timeline→player→roadmap synchronization, repeated activation, reduced-motion scrolling, role-filter reconciliation and EN/KO loop copy.
#1159/#1160 extend the same lineage with real PCM16 stem publication, strict native admission/binding, opaque stem serving, native availability discovery, stale/hostile-safe immutable renderer session handling, non-reused discovery/switch identity, exact-issued discovery completion, transport-continuity capture, same-project opaque source/target validation, finite/nonnegative/ordered loop timing, decoded-duration coverage, exact active-session/plan metadata admission, exact successful/failed switch-receipt retirement, paused-count-in rejection and the mounted canonical five-source selector.
#1160 now connects same-project selection to actual
<audio>source mutation, retires stale old-resourceplay()promises, revokes failed selected stems back to Full mix, localizes the selector in EN/KO, and distinguishes loading, verified Full-mix-only empty, retryable discovery error and normal multi-source states. Current TRACEABILITY/head is332240db…. Durable selected-source persistence/reload remains #970/#962 work; JA/ZH/VI/ES/DE/FR, translation ledger, wider interaction/a11y evidence and rights-cleared desktop audible acceptance remain downstream. None of those child changes are evidence that #903 itself can be closed.Do not transfer #903 checks/reviews/approvals to #971 or its descendants. Source-level succession alone is not enough to close this preservation lane.
Why this remains open
#971's current exact head still lacks terminal repository/central evidence across all required lanes, and #1159/#1160 are Draft stacked work. #1160 current head
332240dbba957602f217dc6e4e6a82a59d4d39b2must still obtain current exact-head hosted gates and qualifying review. #970 is also Draft and its selected-source durability/migration scope remains unfinished. Child source progress cannot substitute for canonical #971 executable evidence.Close #903 only after #971 (or a verified successor) has terminal executable evidence for every transferred behavior and a final semantic sweep proves #903 has no unique test, fixture, contract or evidence left. Until then, conflict and stale ancestry are repair/preservation findings rather than a reason for destructive cleanup.
Canonical design docs identify Figma file
zthWmqfNKUgJBECvv002Qk; historical Song Structure reference node19-457does not substitute for shipped Tauri interaction evidence.