feat(workspace): name tonight's first part handoff on the map - #1007
feat(workspace): name tonight's first part handoff on the map#1007seonghobae wants to merge 40 commits into
Conversation
|
Warning Review limit reachedNext included review available in 46 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: Team Run ID: 📒 Files selected for processing (27)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The aside used a fixed id, so two mounted callouts would emit duplicate DOM ids. Follow the sibling FirstHarmonicFunctionCallout pattern and suffix the id with React useId(); nothing anchors to the bare id externally.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # AGENTS.md # CHANGELOG.md # CLAUDE.md # apps/desktop/src/locales/en/common.json # apps/desktop/src/locales/ko/common.json
|
Resolved merge conflicts against origin/develop (749511c): union-merged AGENTS.md/CHANGELOG.md bullets, sentence-merged CLAUDE.md, unioned en/ko locale tails (JSON valid, en/ko symmetric). Workspace.tsx auto-merged with FirstPartHandoffCallout intact (import + render verified). Merged legacy .Jules/palette.md into canonical .jules/palette.md, dropped alias. No conflict markers remain. Merge head: 679e518. |
|
@opencode-agent Please review exact current head |
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Case collision blocks native checkouts
A second palette.md differs only by directory capitalization from the existing palette. Case-insensitive filesystems cannot materialize both paths, blocking Windows and macOS checkouts.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@claude Please repair the current exact head |
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The PR adds a well-tested, defensive first part-handoff resolver and callout with navigation, i18n, reduced-motion, and workspace-scoping coverage. However, the Workspace integration still renders <FirstPartHandoffCallout song={song} /> without the activeRole prop, so the role-scope feature (the callout's guidance-only state when an unrelated role is selected) is never wired into the real app. The role-scope tests only exercise the callout directly, not the Workspace integration, so the bug is not caught by the suite. This is a confirmed functional gap and blocks approval.
Reviewed changed lines
apps/desktop/src/features/workspace/Workspace.tsx:362 (RIGHT): FirstPartHandoffCallout is mounted without the activeRole prop. Workspace owns the selected-role state (RoleSwitcher), but it is never forwarded, so the callout always falls back to activeRole=null and always shows the all-roles handoff. The role-scope behavior is therefore inert in production.apps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx:82 (RIGHT): The component accepts activeRole and passes it to resolveFirstPartHandoff, and the role-scope tests confirm the intended behavior when the prop is supplied. But because Workspace omits the prop, the default null path is the only path exercised in the real app.apps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx:77 (RIGHT): resolvePartHandoffRenderer scopes to origin.closest('aside')?.parentElement and falls back to a global single-grid lookup. The workspace-scope test pins the intended behavior; this is fragile to layout changes but not currently broken.apps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx:93 (RIGHT): namedSectionIndex derives from song.sections.indexOf(named.section), matching SongStructure's data-section-index stamping from the same unfiltered array, so navigation lands on the correct rendered section. Confirmed aligned.
Adversarial validation
apps/desktop/src/features/workspace/Workspace.tsx:362 (RIGHT)confirmed: Selecting a role in Workspace changes the callout to its guidance-only state when the role is unrelated to the handoff. — Source-traced: Workspace.tsx line 362 omits the prop; FirstPartHandoffCallout.tsx line 82 defaults activeRole to null. The open review thread at FirstPartHandoffCallout.tsx:82 documents the same omission. No Workspace-level test selects a role and asserts the callout switches.apps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx:77 (RIGHT)falsified: Open navigation scrolls the wrong renderer when multiple workspaces mount with duplicate timeline ids. — FirstPartHandoffCallout.workspace-scope.test.tsx mounts two workspaces, clicks the second action, and asserts only the second grid's target scrollIntoView is called. The local-scope query (origin.closest('aside')?.parentElement) resolves the owning grid and rejects ambiguous local matches. Probe falsified.apps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx:93 (RIGHT)falsified: A changed handoff (new song identity or timing) can display the previous handoff's armed guidance before the effect resets state. — The useEffect resets openedPartHandoff on songIdentity/sectionId/sectionIndex/roleIds/atSeconds changes, and theopenedguard compares all of those fields during render. Tests 'resets armed guidance when accessor-id songs change' and 'shows fresh guidance when the destination timing changes' pass. Probe falsified.- Residual risk: The role-scope feature is confirmed unwired in Workspace. Navigation, i18n, reduced-motion, and stale-state handling are covered by tests and appear correct; the remaining risk is the missing activeRole wiring plus the layout-coupling fragility of resolvePartHandoffRenderer.
Findings
-
[high] apps/desktop/src/features/workspace/Workspace.tsx:362 (RIGHT): FirstPartHandoffCallout is rendered without the activeRole prop, so the role-scope feature is never wired: selecting a role in Workspace never reaches the callout, and it always shows the all-roles handoff. The role-scope tests only exercise the callout directly. Pass the Workspace activeRole state into the callout and add an integration test that selects an unrelated role and verifies the guidance-only state.
-
Result: REQUEST_CHANGES
-
Head SHA:
504f2bae400e72dc284fb479319bb2763c134444 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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 reviewed the current-head product diff. Coverage is a separate gate.
Changed files
AGENTS.md— repository behaviorARCHITECTURE.md— repository behaviorCHANGELOG.md— repository behaviorCLAUDE.md— repository behaviorapps/desktop/src/features/workspace/FirstPartHandoffCallout.particle.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/FirstPartHandoffCallout.reduced-motion.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/FirstPartHandoffCallout.role-scope.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/FirstPartHandoffCallout.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/FirstPartHandoffCallout.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/FirstPartHandoffCallout.workspace-scope.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/Workspace.first-part-handoff-role-scope.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/Workspace.test.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/Workspace.tsx— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.inherited-metadata.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.role-scope.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.test-fixture.ts— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.transition.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/features/workspace/firstPartHandoff.ts— TypeScript or JavaScript runtimeapps/desktop/src/i18n/index.test.ts— TypeScript or JavaScript runtimeapps/desktop/src/i18n/index.ts— TypeScript or JavaScript runtimeapps/desktop/src/locales/en/common.json— repository behaviorapps/desktop/src/locales/ko/common.json— repository behaviordocs/design-system/component-contract.md— operator or user guidancedocs/doctoring/reduced-motion-first-part-handoff-navigation.md— operator or user guidanceservices/analysis-engine/src/bandscope_analysis/roles/extractor.py— Python module behaviorservices/analysis-engine/tests/test_roles.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: AGENTS.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: AGENTS.md"]
R1 --> V1["required checks"]
Evidence --> S2["Repository file: ARCHITECTURE.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: ARCHITECTURE.md"]
R2 --> V2["required checks"]
Evidence --> S3["Repository file: CHANGELOG.md"]
S3 --> I3["repository behavior"]
I3 --> R3["Review risk: Repository file: CHANGELOG.md"]
R3 --> V3["required checks"]
Evidence --> S4["Repository file: CLAUDE.md"]
S4 --> I4["repository behavior"]
I4 --> R4["Review risk: Repository file: CLAUDE.md"]
R4 --> V4["required checks"]
Evidence --> S5["TypeScript/JavaScript: FirstPartHandoffCallout.particle.test.tsx (17 files)"]
S5 --> I5["TypeScript or JavaScript runtime"]
I5 --> R5["Review risk: TypeScript/JavaScript: FirstPartHandoffCallout.particle.test.tsx (17 files)"]
R5 --> V5["package test plus coverage"]
Evidence --> S6["Repository file: common.json"]
S6 --> I6["repository behavior"]
I6 --> R6["Review risk: Repository file: common.json"]
R6 --> V6["required checks"]
Evidence --> S7["Repository file: common.json"]
S7 --> I7["repository behavior"]
I7 --> R7["Review risk: Repository file: common.json"]
R7 --> V7["required checks"]
Evidence --> S8["Docs: component-contract.md (2 files)"]
S8 --> I8["operator or user guidance"]
I8 --> R8["Review risk: Docs: component-contract.md (2 files)"]
R8 --> V8["docs review"]
Evidence --> S9["Python: extractor.py"]
S9 --> I9["Python module behavior"]
I9 --> R9["Review risk: Python: extractor.py"]
R9 --> V9["pytest plus coverage"]
Evidence --> S10["Test: test_roles.py"]
S10 --> I10["regression suite"]
I10 --> R10["Review risk: Test: test_roles.py"]
R10 --> V10["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
de4d39f6dae66089cd1d7268a7321d3917ede41c - Workflow run: 33776357240
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: AGENTS.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: AGENTS.md"]
R1 --> V1["required checks"]
Evidence --> S2["Repository file: ARCHITECTURE.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: ARCHITECTURE.md"]
R2 --> V2["required checks"]
Evidence --> S3["Repository file: CHANGELOG.md"]
S3 --> I3["repository behavior"]
I3 --> R3["Review risk: Repository file: CHANGELOG.md"]
R3 --> V3["required checks"]
Evidence --> S4["Repository file: CLAUDE.md"]
S4 --> I4["repository behavior"]
I4 --> R4["Review risk: Repository file: CLAUDE.md"]
R4 --> V4["required checks"]
Evidence --> S5["TypeScript/JavaScript: FirstPartHandoffCallout.particle.test.tsx (17 files)"]
S5 --> I5["TypeScript or JavaScript runtime"]
I5 --> R5["Review risk: TypeScript/JavaScript: FirstPartHandoffCallout.particle.test.tsx (17 files)"]
R5 --> V5["package test plus coverage"]
Evidence --> S6["Repository file: common.json"]
S6 --> I6["repository behavior"]
I6 --> R6["Review risk: Repository file: common.json"]
R6 --> V6["required checks"]
Evidence --> S7["Repository file: common.json"]
S7 --> I7["repository behavior"]
I7 --> R7["Review risk: Repository file: common.json"]
R7 --> V7["required checks"]
Evidence --> S8["Docs: component-contract.md (2 files)"]
S8 --> I8["operator or user guidance"]
I8 --> R8["Review risk: Docs: component-contract.md (2 files)"]
R8 --> V8["docs review"]
Evidence --> S9["Python: extractor.py"]
S9 --> I9["Python module behavior"]
I9 --> R9["Review risk: Python: extractor.py"]
R9 --> V9["pytest plus coverage"]
Evidence --> S10["Test: test_roles.py"]
S10 --> I10["regression suite"]
I10 --> R10["Review risk: Test: test_roles.py"]
R10 --> V10["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
seonghobae
added
enhancement
ChatGPT Codex Connector
1 participant
Product outcome
Name tonight's first corroborated source-to-destination part handoff on the mounted rehearsal map so the room can lock the pass before the next section. Open moves to the matching rendered destination section.
This map next-action is distinct from labeled
handoffform guidance (#937), Part Handoff Map visualization (#850), and metadata-handoff export. Do not mix with #811, #828, or #897.Exact current identity
develop@749511c3ad4000090048718f685c6bee6b3d2c25.feat/workspace-first-part-handoff.5261b1cbb15fd6587425c954c3480991394afc74.All predecessor-head check/review evidence is historical only.
Root-cause repairs present
The predecessor implementation treated
partGraph.handoff_toas an in-section relation and required the receiver to be another active role in that same section. That contradicts the analysis engine's transition topology: the source active role deactivates, the receiving role activates in the immediately following section, and the source graph carrieshandoff_toplus receiver-sidehandoff_fromcorroboration.The branch now:
handoff_toas outgoing authority;handoff_fromto corroborate the giver while the receiver is inactive in the source;WorkspaceselectedactiveRoleintoFirstPartHandoffCallout;handoff_to/handoff_from; fallback provenance now says handoffs are unavailable instead of claiming computed handoffs.A one-section same-state graph or heuristic fallback can no longer manufacture a buyer-visible transition.
TDD / executable evidence
firstPartHandoff.transition.test.tsreproduces the real deactivate/activate topology and source-side bidirectional corroboration requirement.firstPartHandoff.role-scope.test.tspreserves giver/receiver/unrelated selected-role semantics.FirstPartHandoffCallout.role-scope.test.tsxproves receiver selection retains the pass and unrelated selection remains guidance-only with no navigation button.Workspace.first-part-handoff-role-scope.test.tsxwas introduced before the mounted production wiring repair. It covers unrelated/giver/receiver role selection and now also covers a stale role from a previous song failing open to the new song's valid handoff.services/analysis-engine/tests/test_roles.pywas changed before the heuristic fallback production repair to reject invented Bass Guitar → Lead Vocal authority and require truthful fallback provenance.firstPartHandoff.test.ts, inherited-metadata tests and callout/navigation tests retain destination timing, ranking, hostile metadata, Korean copy, reduced motion and workspace scoping.Mounted integration RED-oriented test commit:
f5c364f715605b8c259cf4bd4b9673c455a5cfe3.Mounted production wiring:
234c31614249715e3b5aec5208cddad5bcd05523.Stale-role regression-first commit:
98b2faf6a8aa9e4b62217487619486d57a6e6dfc.Stale-role production repair:
cc20581f5c871c1a20562054ca2e7dbf845d0d4b.Heuristic-authority regression-first commit:
20937325286cfcff8e388b90c466e6e06ec86653.Heuristic fallback production repair:
5261b1cbb15fd6587425c954c3480991394afc74.No predecessor GREEN transfers across these head changes; fresh hosted checks on
5261b1c…are required.Canonicalization boundary with #1094
Fresh comparison showed #1094 still contained one unique scientifically meaningful contract: heuristic fallback removed a hard-coded Bass Guitar → Lead Vocal relation and disclosed that fallback handoffs are unavailable. That exact semantic contract is now represented in #1007 by executable analysis-engine regression coverage and the owning source repair, in addition to #1007's mounted selected-role coverage. #1094 must not contribute check/review evidence to this head. Close it as superseded only after the unchanged canonical head is revalidated and no further unique requirement remains.
Security / scientific-integrity notes
null.section.id.developremains dependency-security authority; this branch does not suppress inherited findings or weaken required gates.Documentation
AGENTS.md,ARCHITECTURE.md,docs/design-system/component-contract.md, and doctoring describe the source-to-destination transition boundary. Documentation/provenance must remain aligned with the now-explicit heuristic-handoff prohibition before merge.Merge gate
develop.