Skip to content

feat(ui): re-analyze CTA for legacy Session Musician runs - #24

Closed
slittycode wants to merge 1 commit into
claude/thirsty-easley-faeaeafrom
claude/reanalyze-legacy-cta
Closed

feat(ui): re-analyze CTA for legacy Session Musician runs#24
slittycode wants to merge 1 commit into
claude/thirsty-easley-faeaeafrom
claude/reanalyze-legacy-cta

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Why

When a cached run shows up with transcriptionMethod: 'basic-pitch' (or any non-torchcrepe method), the Stem note draft block enters its legacy render state and shows the notice "Re-analyze for current stem-aware quality. The notes below are kept for archival." — but PR #22 left it at just words. This adds the actual button.

Behaviour

  • Block A renders a "Re-analyze with stem-aware pipeline" button only in the legacy render state AND only when App passes a callback (a source File is loaded and no analysis is currently in flight).
  • Clicking the button calls back up through <SessionMusicianPanel><AnalysisResults>App.tsx, which fires handleStartAnalysis({ pitchNoteRequested: true }). The File state (audioFile) is retained in App across analyses, so no re-upload is required — the existing bytes are re-POSTed to /api/analysis-runs with stem-aware mode on.
  • The STEM PITCH/NOTE TRANSLATION toggle is also updated to reflect the override so subsequent manual Run Analysis clicks behave consistently.

Investigation: does the backend support artifact reuse?

Read apps/backend/analysis_runtime.py. create_run takes content: bytes and stores the audio anew on every call — there's no existing API to reference a prior artifact ID. Because audioFile: File is retained in App state after analysis completes (apps/ui/src/App.tsx:225), the cleanest path is to re-POST the same File, no backend changes needed. The producer never sees an upload dialog.

Wiring

  • New optional onReanalyzeWithStemAware?: () => void prop on SessionMusicianPanel.tsx and NoteDraftBlock.tsx. AnalysisResults.tsx threads it through.
  • App constructs the handler as audioFile && !isAnalyzing ? () => handleStartAnalysis({ pitchNoteRequested: true }) : undefined, so the button is only rendered (and clickable) when a re-run is actually viable.
  • handleStartAnalysis gains a typed overrides param. The override is read in the call to analyzeAudio, and a matching setPitchNoteTranslationRequested keeps the toggle visually in sync.

Tests

  • Vitest (sessionMusicianPanel.test.ts): five new static-markup cases — legacy + callback → button renders, legacy + no callback → no button, stem-aware → no button, full-mix-fallback → no button, ran-with-no-result / requested-but-unavailable → no button. 30 panel tests pass total.
  • Playwright smoke (upload-phase1-midi.spec.ts): stubs a run that returns transcriptionMethod: 'basic-pitch'. After the page renders the legacy state, asserts the button is visible, clicks it, and confirms a second POST to /api/analysis-runs fires with pitch_note_mode=stem_notes. 47 smoke tests pass total (1 skipped — live Gemini).
  • No backend changes.

Base

Targets claude/thirsty-easley-faeaea (PR #22) since that's where the redesigned panel lives. Once #22 lands on main, GitHub can retarget this or I can rebase + force-push.

Plan

~/.claude/plans/re-evaluate-asa-s-session-musician-sharded-balloon.md — "Out-of-scope follow-ups" section.

🤖 Generated with Claude Code

When a cached run shows up with `transcriptionMethod: 'basic-pitch'` (or any
non-torchcrepe method), the Stem note draft block enters its `legacy` render
state and shows the existing "Re-analyze for current stem-aware quality"
notice. PR #22 left the notice as just words — this adds the actual button.

Behaviour
- Block A renders a "Re-analyze with stem-aware pipeline" button only in the
  `legacy` render state AND only when App passes a callback (i.e. when a
  source File is loaded and no analysis is currently in flight).
- Clicking the button calls back up through SessionMusicianPanel →
  AnalysisResults → App, which fires `handleStartAnalysis` with an explicit
  `pitchNoteRequested: true` override. The File state (`audioFile`) is
  retained in App across analyses, so no re-upload is required; the existing
  bytes are re-posted to `POST /api/analysis-runs` with stem-aware mode on.
- The UI toggle "STEM PITCH/NOTE TRANSLATION" is also updated to reflect the
  override so subsequent manual Run Analysis clicks behave consistently.

Investigation
- `analysis_runtime.create_run` stores the artifact bytes anew on every run —
  there's no artifact-reuse API today. But because `audioFile: File` is
  retained in App state after analysis completes, re-running just means
  re-POSTing the same File, which is what the producer expects. No backend
  changes needed.

Wiring
- New optional `onReanalyzeWithStemAware?: () => void` prop on
  `SessionMusicianPanel` and `NoteDraftBlock`. AnalysisResults threads it
  through. App constructs the handler as
  `audioFile && !isAnalyzing ? () => handleStartAnalysis({ pitchNoteRequested: true }) : undefined`,
  so the button is only rendered (and clickable) when a re-run is actually
  viable.
- `handleStartAnalysis` gains a typed `overrides` param. The override is
  read in the call to `analyzeAudio`, and a matching
  `setPitchNoteTranslationRequested` keeps the toggle visually in sync.

Tests
- Vitest (`sessionMusicianPanel.test.ts`): added five static-markup cases
  covering button visibility — legacy + callback → button renders, legacy +
  no callback → no button, stem-aware → no button, full-mix-fallback → no
  button, ran-with-no-result / requested-but-unavailable → no button.
- Playwright smoke (`upload-phase1-midi.spec.ts`): stubs a run that returns
  `transcriptionMethod: 'basic-pitch'`. After the page renders the legacy
  state, asserts the button is visible, clicks it, and confirms a second
  POST to `/api/analysis-runs` fires with `pitch_note_mode=stem_notes`.

Targets PR #22 (claude/thirsty-easley-faeaea) as the base; will retarget to
main once that lands. Plan in
~/.claude/plans/re-evaluate-asa-s-session-musician-sharded-balloon.md
("Out-of-scope follow-ups" section).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Verdict: APPROVE (submitted as COMMENT — GitHub blocks self-review)

Summary

Clean prop-threading addition that wires an onReanalyzeWithStemAware callback from App.tsx down through AnalysisResults → SessionMusicianPanel → NoteDraftBlock, rendering a re-run button only in the legacy render state. No backend changes. All 336 unit tests pass; the five new Vitest cases cover every relevant render-state combination, and the new Playwright smoke test confirms the second POST fires with pitch_note_mode=stem_notes.

Findings

Worth considering

  • App.tsx: setPitchNoteTranslationRequested is called inside handleStartAnalysis to keep the toggle visually in sync with the override, but no test actually asserts the toggle flips. The logic is trivial so it's not a blocking gap, but if the override path is ever refactored it'll be unguarded.

  • The smoke test stubs every POST's run with requestedStages.pitchNoteMode: 'stem_notes' regardless of what the first click sends. Fine for the purpose — only the second POST value is asserted — but postedPitchNoteModes[0] is never checked, so a regression in the first run's pitch_note_mode would slip through.

  • type StartAnalysisOverrides is defined inside the component body just before handleStartAnalysis. Zero runtime cost (types are erased), but convention is module scope. Not a finding per the style rules, just noting it.

Test results

336 passed / 0 failed across 35 test files.

Phase boundary check

Clean. No Phase 1 measurements read, re-derived, or mutated anywhere in this diff. Phase 2/3 files untouched. The re-analyze path re-POSTs the existing audioFile with pitchNoteRequested: true through the same analyzeAudio entry point as a normal run.


Generated by Claude Code

@slittycode

Copy link
Copy Markdown
Owner Author

Superseded by #29 (rebased onto main and re-opened as a new PR after PR #22 merged caused this one to auto-close).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant