Skip to content

feat(workspace): start analysis from the first-run card - #974

Draft
seonghobae wants to merge 9 commits into
developfrom
feat/first-run-analyze-selected-song
Draft

feat(workspace): start analysis from the first-run card#974
seonghobae wants to merge 9 commits into
developfrom
feat/first-run-analyze-selected-song

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Product outcome

A musician who has already chosen a local song no longer stares at the empty-file prompt. The workspace names Analyze this song as the next rehearsal action and asks only which part they are playing tonight.

Parent buyer gap: #964 (first-run journey). This slice does not add a mocked demo analysis, licensed demo audio, or a parallel MIR product. #828 still owns #770.

choose a local song
→ pick tonight's part (whole band / lead vocal / bass / keys)
→ Analyze this song

Exact current identity

What landed

  • After local or YouTube source admission, EmptyState is replaced by a first-run card.
  • Closed role choices map onto existing analysis roleFocus IDs (bass-guitar, keys-right, lead-vocal). Unknown IDs are not admitted.
  • Header Start analysis keeps the previous whole-band default until a part is chosen, so current jobs stay compatible.
  • Selected file names are reduced to a basename before any first-run attribute is emitted. Local paths never render.
  • Customer-facing copy is in en and ko and enables the next action instead of describing the gap.

Verification

Desktop vitest --coverage on this head: 205 passed, App.tsx 100% statements/branches/functions/lines. No lockfile, dependency, model, or review-credential changes.

Security Notes

  • Untrusted input: user-selected local file names and first-run role clicks.
  • Trust boundary: UI role choice → closed allowlist → existing AnalysisJobRequest.roleFocus. Basename sanitization before any first-run attribute.
  • Safe failure: analysis still starts only after an explicit click. No mocked demo success path is exposed as a buyer demo.
  • Privacy: audio stays local; paths are not rendered.

Dependency / merge gate

  • Canonical npm HIGH owner remains fix(security): establish canonical npm, PDF.js, Nanoid, and Undici baseline #783 (pdfjs-dist / nanoid / undici). This branch must not copy or suppress that baseline.
  • Keep unmerged until this unchanged exact head has every applicable repository/central CI, security, SAST, SBOM, coverage, and review gate terminal-success; zero valid unresolved findings; a qualifying independent non-author last-push APPROVE; and ordinary branch-protection acceptance.
  • Queued, pending, skipped, cancelled, predecessor-head, author-only, model-only, or administrative-bypass evidence is non-passing.
  • Do not self-approve.

Open in Devin Review

Summary by CodeRabbit

  • 새 기능

    • 곡을 선택하면 빈 화면 대신 첫 실행 안내가 표시됩니다.
    • 전체 밴드, 리드 보컬, 베이스, 키보드 중 담당 파트를 선택해 분석을 시작할 수 있습니다.
    • 다른 파일로 쉽게 변경할 수 있으며, 분석 진행 중 관련 버튼이 비활성화됩니다.
    • 선택한 파일의 이름만 안전하게 표시됩니다.
  • 다국어 지원

    • 영어와 한국어 안내 문구 및 역할 선택 UI를 추가했습니다.
  • 테스트

    • 파일 선택, 역할 지정, 분석 시작 및 파일 변경 흐름을 검증했습니다.

After a local or YouTube source is admitted, replace the empty-file
prompt with a first-run card that names Analyze this song as the next
action and records tonight's part before the job starts.

This is a #964 first-run buyer slice. It does not add a mocked demo
analysis path and does not invent a parallel MIR product.

Security Notes:
- Untrusted input: selected file names are reduced to a basename before
  any first-run attribute is emitted; role IDs are a closed allowlist.
- Trust boundary: UI role choice maps onto existing analysis roleFocus
  values. Local paths never render.
- Safe failure: analysis still starts only after an explicit click.
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Limit 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.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d13dd9df-81d7-4b8b-bac4-5c4cf88ccdf2

📥 Commits

Reviewing files that changed from the base of the PR and between 822a445 and fd33104.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • apps/desktop/src/features/workspace/WorkspaceStates.test.tsx
  • apps/desktop/src/features/workspace/WorkspaceStates.tsx
  • apps/desktop/src/features/workspace/firstRunRoles.ts
📝 Walkthrough

Walkthrough

오디오 선택 후 빈 상태 대신 첫 실행 분석 카드가 표시됩니다. 사용자는 전체 밴드 또는 특정 파트를 선택할 수 있습니다. 선택한 역할은 분석 요청의 roleFocus에 전달됩니다. 영어와 한국어 문구 및 관련 테스트가 추가되었습니다.

Changes

첫 실행 분석

Layer / File(s) Summary
첫 실행 역할 계약
apps/desktop/src/features/workspace/firstRunRoles.ts, apps/desktop/src/features/workspace/firstRunRoles.test.ts
첫 실행 역할 옵션과 FirstRunRoleId를 추가했습니다. 역할을 roleFocus로 변환하고 파일명을 안전한 기본 이름으로 정규화합니다.
첫 실행 역할 선택 UI
apps/desktop/src/features/workspace/WorkspaceStates.tsx, apps/desktop/src/features/workspace/WorkspaceStates.test.tsx, apps/desktop/src/locales/en/common.json, apps/desktop/src/locales/ko/common.json
FirstRunState가 선택 파일명, 역할 라디오 그룹, 분석 버튼, 다른 파일 선택 버튼을 표시합니다. 분석 중에는 두 버튼을 비활성화합니다. 영어와 한국어 문구를 추가했습니다.
워크스페이스 및 분석 연결
apps/desktop/src/App.tsx, apps/desktop/src/App.test.tsx, AGENTS.md, ARCHITECTURE.md, CHANGELOG.md, CLAUDE.md
로컬 오디오 또는 YouTube 소스 선택 후 FirstRunState를 렌더링합니다. 선택된 역할을 분석 요청에 전달하고 새 파일 선택 시 전체 밴드로 초기화합니다. 관련 동작 지침과 테스트를 추가했습니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 822a4

The new first-run flow guides musicians from source selection into analysis, but the current change still has a lint-blocking declaration issue and does not show the selected song name on the card; merge should wait for those fixes, with the YouTube changelog wording updated as follow-up.

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant FirstRunState
  participant AnalysisJob
  App->>FirstRunState: 선택한 파일과 기본 역할 표시
  FirstRunState->>App: 사용자가 역할 선택
  App->>AnalysisJob: 선택한 roleFocus로 분석 시작
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PR 제목은 첫 실행 카드에서 분석을 시작하는 주요 변경 사항을 정확하고 간결하게 설명합니다.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 6 files. (6 skipped: 6 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/first-run-analyze-selected-song

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 4 potential issues.

Open in Devin Review

Comment thread apps/desktop/src/App.tsx
Comment on lines +530 to +532
onChooseDifferentFile={() => {
void handleChooseLocalAudio();
}}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Cancelling 'Choose a different file' discards the selected song

Opening the file picker from the card's "Choose a different file" button and then cancelling runs handleChooseLocalAudio, whose failure branch calls setSelectedBootstrap(null) and shows a "choose a WAV/MP3..." error. The already-selected song is dropped and the user falls back to the empty prompt, since cancellation is indistinguishable from an unsupported file.

Prompt for agents
The first-run card's "Choose a different file" button calls handleChooseLocalAudio (apps/desktop/src/App.tsx). When the user cancels the native file dialog, the Rust command select_local_audio_source returns Err("Choose a WAV, MP3, FLAC, or M4A file to start analysis.") (apps/desktop/src-tauri/src/main.rs:644), and selectLocalAudioSource in apps/desktop/src/lib/analysis.ts collapses any non-safe error into that same unsupported message, so a user cancellation cannot be distinguished from a genuinely unsupported file. As a result handleChooseLocalAudio runs its failure branch: it clears the current selection (setSelectedBootstrap(null)) and shows the unsupported-file error, dropping the user from the first-run card back to the empty prompt even though they only cancelled. Consider surfacing a distinct cancellation signal (e.g. a dedicated error code/message from the Rust command and a check like the existing isUserCancellation helper) so that cancelling the picker leaves the previously selected source and first-run card intact instead of wiping it and showing a false error.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revalidated on current live heads. This finding is still valid on #974 exact fd33104ca88030b5218e1c56a0cb683e2b50d7db, so I am leaving this thread unresolved. The cancellation protocol is already owned by canonical #976 current exact d07dc57347ce8187bc70b1d1f01b1ac5ec195887: its handleChooseLocalAudio explicitly checks isUserCancellation(...) and returns without clearing the admitted bootstrap or surfacing an unsupported-format error. #974 should inherit/restack that owner rather than duplicate a second native-cancellation authority. Acceptance for this thread is #974 on a head that contains that canonical cancellation behavior plus the first-run replacement-picker regression; predecessor/#976-only evidence is not sufficient to resolve it.

Comment thread apps/desktop/src/features/workspace/firstRunRoles.ts Outdated
Comment thread apps/desktop/src/features/workspace/WorkspaceStates.tsx
Comment thread apps/desktop/src/features/workspace/WorkspaceStates.tsx
Comment thread apps/desktop/src/App.tsx
onChooseDifferentFile={() => {
void handleChooseLocalAudio();
}}
analysisDisabled={analysisInFlight || isStarting || isImporting}
@opencode-agent opencode-agent Bot added area: ui-ux Frontend, interaction, design, or user experience priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: feature New or expanded product capability labels Aug 22, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Collaborator Author

Fresh ownership routing for the unresolved picker-cancellation finding on exact head fd33104ca88030b5218e1c56a0cb683e2b50d7db:

  • The first-run branch currently cannot distinguish native picker cancellation from an invalid/unsupported selection, so cancelling Choose a different file can clear the admitted source.
  • The causal picker/bridge boundary is already owned by canonical analysis-recovery PR feat(workspace): name the next action after analysis fails #976, current head d07dc57347ce8187bc70b1d1f01b1ac5ec195887: select_local_audio_source emits the explicit User cancelled signal, selectLocalAudioSource() preserves only that allowlisted signal while redacting arbitrary native errors, and handleChooseLocalAudio() treats it as a no-op. feat(workspace): name the next action after analysis fails #976 also has a focused analysis.selection.test.ts regression proving the signal survives the bridge.

Do not duplicate that authority in this leaf. Keep this first-run lane unready until the #976 cancellation boundary is integrated/restacked here, then add/retain the buyer-flow regression selected song -> Choose a different file -> cancel -> same first-run card/source, no unsupported-format alert and re-run exact-head CI. The other current static finding in this PR (redundant analysisDisabled terms) remains BandScope-local and should be repaired independently.

@seonghobae
seonghobae marked this pull request as draft August 23, 2026 11:12

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Continue only the existing canonical branch feat/first-run-analyze-selected-song at exact head fd33104ca88030b5218e1c56a0cb683e2b50d7db; do not create another branch/PR and preserve Draft state.

Repair only the independent current static finding in apps/desktop/src/App.tsx: inside renderWorkspaceState, the selectedBootstrap branch is reachable only after the earlier if (analysisInFlight || isStarting) return <LoadingState />, so analysisDisabled={analysisInFlight || isStarting || isImporting} contains two provably impossible terms. Make the semantics-preserving change to analysisDisabled={isImporting} and run the repository-pinned focused desktop test(s), typecheck, and full quickcheck/exact-head CI. Do not alter picker-cancellation semantics here: that causal boundary is owned by #976 and must be integrated/restacked separately as noted in the preceding ownership comment. Do not weaken #783 dependency-security gates or touch lockfiles.

Copy link
Copy Markdown
Collaborator Author

@opencode-agent Continue only the existing feat/first-run-analyze-selected-song owner branch from exact live head fd33104ca88030b5218e1c56a0cb683e2b50d7db over protected develop@acdbea6344fe1231c39535b575f4de35e4c607c9. Re-read current blobs/reviews before writing; the PR body’s 822a445... identity is stale and must not be treated as current evidence. One exact-current-head CodeQL finding remains locally actionable: in renderWorkspaceState, analysisInFlight || isStarting are impossible after the prior loading return, so change FirstRunState.analysisDisabled from analysisInFlight || isStarting || isImporting to the behavior-equivalent isImporting, with no wider first-run behavior change. Run focused first-run/App tests, lint/typecheck, and canonical quickcheck; commit only to this same branch and refetch the resulting exact head. Do not duplicate the picker-cancellation protocol here: current unresolved cancellation finding belongs to canonical #976, whose current branch already owns the distinct native-cancellation signal and preservation regression. Do not touch dependency/lock/security baseline work (#783-owned), reviewer credentials, or gates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui-ux Frontend, interaction, design, or user experience priority: medium Normal-priority or P2 work status: needs-review Open pull request requiring current-head review or checks type: feature New or expanded product capability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant