feat: implement M1 workspace shell and M2 rehearsal pack UI#147
Merged
Conversation
- Added ChordRecognizer using librosa's chromagrams to extract chords from stems. - Added PitchTracker using librosa's pYIN/YIN to determine pitch ranges. - Updated RoleExtractor to use real stems and these new DSP components to inject actual chords and ranges into roles. - Reached 100% test coverage with extensive mocking of librosa functions to simulate error conditions. - Resolved type hinting and formatting issues.
📝 WalkthroughSummary by CodeRabbit새로운 기능
테스트
기타
WalkthroughPR은 작업 폴링 기반 분석 관리를 workspace/pack 모델로 대체합니다. 새로운 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Desktop App
participant JobRunner as job_runner.ts
participant Tauri
participant Backend
User->>App: Enqueue Song (local/YouTube)
App->>JobRunner: enqueueSong(request)
JobRunner->>Tauri: invoke "enqueue_song"
Tauri->>Backend: Process job
App->>JobRunner: subscribeToWorkspaceUpdates(callback)
Backend->>Tauri: workspace-updated event
Tauri->>JobRunner: Event listener
JobRunner->>App: Call callback (new workspace)
App->>App: Update UI with workspace state
User->>App: Request workspace state
App->>JobRunner: getWorkspaceState()
JobRunner->>Tauri: invoke "get_workspace_state"
Tauri->>Backend: Fetch state
Backend-->>Tauri: RehearsalWorkspace
Tauri-->>JobRunner: Parsed workspace
JobRunner-->>App: RehearsalWorkspace | null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/App.test.tsx`:
- Around line 290-291: The test uses an explicit any for the finder callback
which must be replaced with the proper song type or left untyped to allow
inference; update the mockWorkspaceStore.songs.find callback (currently written
as (s: any) => s.id === "non-existent") to either use the actual
Song/WorkspaceSong type (e.g., (s: Song) => s.id === "non-existent") after
importing that type, or simply remove the : any and write (s) => s.id ===
"non-existent" so TypeScript infers the correct type; apply the same change you
made at Line 275 to keep tests type-safe.
- Line 203: Replace the unsafe cast "song: { id: 'song2' } as any" with a
minimal valid RehearsalSong object or call the test helper
createDemoRehearsalSong() so the test uses a real shape; update the test in
App.test.tsx to construct a RehearsalSong with required fields (or use
createDemoRehearsalSong('song2')/similar helper) and pass that into the
component instead of casting to any.
- Around line 8-9: Replace the explicit any types: change mockWorkspaceStore to
the concrete RehearsalWorkspace type (e.g., RehearsalWorkspace | null) and type
workspaceSubscribers as an array of callback signatures that accept a
RehearsalWorkspace (e.g., Array<(ws: RehearsalWorkspace) => void> or the actual
subscriber function type used elsewhere). Update references in tests that
push/invoke subscribers to match the new callback signature so the ESLint rule
`@typescript-eslint/no-explicit-any` is satisfied and type checks pass for
mockWorkspaceStore and workspaceSubscribers.
- Around line 275-276: Replace the loose any cast on the find callback with the
concrete SongRehearsalPack type to satisfy linting: change the callback
signature from (s: any) => ... to (s: SongRehearsalPack) => ... when searching
mockWorkspaceStore.songs for id "non-existent" (the badPack variable), and add
the appropriate import or type reference for SongRehearsalPack if it's not in
scope.
In `@apps/desktop/src/App.tsx`:
- Line 23: The import line brings in LoadingState and ErrorState which are not
used; remove the unused imports to fix the pipeline failure—update the import
that currently references EmptyState, LoadingState, ErrorState in App (symbol:
EmptyState, LoadingState, ErrorState) so it only imports the symbols actually
used (e.g., keep EmptyState or remove the whole import if none are used).
- Around line 189-198: The button uses aria-disabled but not the actual disabled
attribute, causing mismatched behavior; update the button element in App.tsx to
set disabled={!workspace} in addition to aria-disabled, and guard the click
handler (handleSaveProject) so it no-ops when disabled (or ensure the onClick
isn’t called when disabled) to keep keyboard/assistive tech and visual behavior
consistent; target the JSX button with onClick={handleSaveProject} and
aria-disabled so both attributes and the handler logic are aligned.
- Around line 56-71: subscribeToWorkspaceUpdates의 Promise와 콜백에서 발생하는 예외를 처리하고
cleanup이 항상 동작하도록 unlisten을 안전한 no-op으로 초기화하십시오: useEffect 내에서 let unlisten: ()
=> void = () => {}; 호출한 subscribeToWorkspaceUpdates(...)의 then과 catch를 사용해 프로미스
거부를 로깅/처리하고, 콜백 인자(ws)를 처리할 때 parseRehearsalWorkspace가 예외를 던질 수 있으니 해당 콜백 내부에서
try/catch로 감싸 setWorkspace 호출을 안전하게 수행하도록 하며, getWorkspaceState().then(...) 역시
.catch로 오류를 처리하고 컴포넌트 언마운트 시 항상 unlisten()을 호출하도록 유지하세요 (참조:
subscribeToWorkspaceUpdates, parseRehearsalWorkspace, unlisten,
getWorkspaceState, setWorkspace, useEffect).
- Around line 6-7: The file imports the types AnalysisJobRequest and
ProjectBootstrapSummary but never uses them; remove these two unused imports
from the import statement (or restore usage if they were intended to be used) to
clear the lint/compile failure—specifically edit the import that currently lists
AnalysisJobRequest and ProjectBootstrapSummary and delete those two symbols.
- Around line 77-82: The call to enqueueSong (which returns a Promise) is
currently fire-and-forget and lacks error handling; wrap the enqueueSong
invocation (the call that uses selection.bootstrap.projectId,
selection.bootstrap.source.fileName and defaultRequest.roleFocus) in async error
handling (await inside an async function or use .catch) and on failure surface
feedback to the user (e.g., trigger the app's notification/toast or log an error
via your logger/IPC channel) so IPC call failures are not silent. Ensure the
error handler includes context (projectId/sourceLabel/roleFocus) when reporting.
In `@apps/desktop/src/features/workspace/Workspace.tsx`:
- Line 91: The UI is rendering the role ID via activeRole instead of the
human-readable role.name; update the rendering logic in Workspace.tsx (where
<strong>Stem Player: {activeRole}</strong> appears) to resolve the role ID to
its display name by either having RoleSwitcher expose the selected role's name
or by looking up the role object in allRoles (find role where role.id ===
activeRole) and use role.name, falling back to the ID only if no match is found.
- Around line 89-96: The three emoji-only buttons in the Workspace component
(the Play button, "Loop Section" button, and "Mute Others (Solo)" button
rendered when activeRole is truthy) lack accessible names; add distinct
aria-label attributes to each button (e.g., aria-label="Play stem for
{activeRole}", aria-label="Loop section", aria-label="Mute other stems (solo)")
so screen readers can convey their purpose, keeping the labels descriptive and
optionally including the activeRole where relevant.
In `@apps/desktop/src/lib/job_runner.ts`:
- Around line 30-35: The module-level mockWorkspace (type RehearsalWorkspace)
leads to shared mutable state across tests; replace it with a factory function
(e.g., createMockWorkspace) that returns a fresh RehearsalWorkspace object for
each test or reset mockWorkspace at test setup. Update any usages that reference
mockWorkspace in job_runner.ts to call the factory (createMockWorkspace()) or to
clone the object before mutation, ensuring tests get isolated instances.
- Around line 132-137: The event handler registered via
listen("workspace-updated", ...) can throw when calling parseRehearsalWorkspace
even after isRehearsalWorkspace passed; wrap the parseRehearsalWorkspace and
callback invocation in a try-catch to prevent unhandled exceptions: inside the
listener for "workspace-updated" (where isRehearsalWorkspace,
parseRehearsalWorkspace, and callback are used) catch any error, log it
(including the event.payload and error) and return/ignore the bad event so the
listener stays alive.
- Around line 62-76: The simulated processing setTimeouts in the enqueue
simulation update packs even after a pack is cancelled because their timer
handles aren't tracked; modify the code that schedules the two timeouts (the
outer and inner setTimeout that update pack.packState/engineState and call
triggerMockUpdate) to store their timer IDs keyed by packId (e.g., a Map<string,
number> or Map<string, number[]>), and update cancelSong to clear any pending
timers for that packId using clearTimeout and remove the entry from that map;
additionally, before applying state changes in the timeout callbacks (where you
locate the pack via mockWorkspace.songs.find), check that the pack still exists
to avoid updating removed packs.
- Around line 22-28: getInvoke currently returns the imported invoke fallback
which is always truthy, so it never signals non-Tauri environments; update
getInvoke to detect a real Tauri injection instead of relying on
window.__TAURI_INVOKE__ — check for window.TAURI or window.TAURI_INTERNALS (or
window.__TAURI_INVOKE__ only if you intentionally support that legacy key) and
only return actual invoke when one of those globals exists; otherwise return
null so the caller's browserFallback path can run (refer to the getInvoke
function and the imported invoke symbol when implementing the check).
In `@docs/plans/2026-03-28-ml-engine-integration.md`:
- Line 20: The heading "### Track 3: Harmonic & Pitch Pipelines (`#107`)
(COMPLETED)" violates markdownlint MD022 because there is no blank line after
the title; fix it by adding a single empty line immediately after that heading
line in the document so the title is followed by a blank line as required.
In `@services/analysis-engine/src/bandscope_analysis/chords/chord_recognizer.py`:
- Around line 99-106: The chromagram frames are not normalized before
dot-product with the L2-normalized templates, so high-energy frames bias
similarity; normalize each chromagram frame (columns of the chromagram matrix
used in chord_recognizer.py) to unit L2 norm prior to computing similarity (used
in the np.dot(self.templates, chromagram) step) and handle zero-energy frames
(e.g., leave as zeros or skip) so similarity and the subsequent best_matches =
np.argmax(similarity, axis=0) reflect spectral shape rather than frame energy.
In `@services/analysis-engine/src/bandscope_analysis/roles/extractor.py`:
- Around line 65-71: PitchTracker.track() never returns None, so the current
guard "if p_res:" wrongly treats failed detections as truthy and overwrites
defaults; update the logic in the vocal block (where p_res is assigned from
PitchTracker.track) to only overwrite vocal_range when p_res contains valid
pitch data (e.g., check p_res.get("lowest_note") and p_res.get("highest_note")
are non-empty/None or use an explicit success flag from TrackedPitchRange), and
apply the same change to the bass block that sets bass_range so defaults are
preserved when detection fails.
In `@services/analysis-engine/tests/test_chord_recognizer.py`:
- Line 24: Remove the debug print statement left in the test: delete the
print("RESULT:", result) call in
services/analysis-engine/tests/test_chord_recognizer.py (the line that prints
the local variable result) so tests do not produce extraneous CI log output;
ensure no other debug prints remain in the same test function.
In `@services/analysis-engine/tests/test_roles_ml.py`:
- Around line 30-45: The mocks for PitchTracker.track and
ChordRecognizer.recognize don't match the real TypedDict shapes: update
side_effect_track to include the "confidence" key in the returned dicts for
vocals_stem and bass_stem (matching PitchTracker.track), and update
side_effect_recognize to return chord dicts using "start_time" and "end_time"
instead of "start"/"end" for bass_stem and other_stem (matching
ChordRecognizer.recognize); modify the mock functions named side_effect_track
and side_effect_recognize used in the test to return these corrected keys so the
mocked shapes align with the real implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e55decef-b962-4c33-8080-4823cfd6c8b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignoreapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/features/workspace/Workspace.tsxapps/desktop/src/lib/job_runner.tsapps/desktop/vite.config.tsdocs/plans/2026-03-28-ml-engine-integration.mdpackages/shared-types/src/index.tspackages/shared-types/test/index.test.tsservices/analysis-engine/src/bandscope_analysis/chords/chord_recognizer.pyservices/analysis-engine/src/bandscope_analysis/ranges/pitch_tracker.pyservices/analysis-engine/src/bandscope_analysis/roles/extractor.pyservices/analysis-engine/tests/test_chord_recognizer.pyservices/analysis-engine/tests/test_pitch_tracker.pyservices/analysis-engine/tests/test_roles_ml.py
Comment on lines
+8
to
+9
| let mockWorkspaceStore: any = null; | ||
| let workspaceSubscribers: any[] = []; |
There was a problem hiding this comment.
파이프라인 실패: any 타입을 구체적인 타입으로 교체 필요
ESLint @typescript-eslint/no-explicit-any 규칙으로 인해 빌드가 실패합니다. RehearsalWorkspace와 콜백 배열에 적절한 타입을 사용해주세요.
🔧 타입 수정 제안
+import type { RehearsalWorkspace } from "@bandscope/shared-types";
+
const mockLoadProject = vi.fn();
const mockSaveProject = vi.fn();
-let mockWorkspaceStore: any = null;
-let workspaceSubscribers: any[] = [];
+let mockWorkspaceStore: RehearsalWorkspace | null = null;
+let workspaceSubscribers: Array<(ws: RehearsalWorkspace) => void> = [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mockWorkspaceStore: any = null; | |
| let workspaceSubscribers: any[] = []; | |
| import type { RehearsalWorkspace } from "@bandscope/shared-types"; | |
| const mockLoadProject = vi.fn(); | |
| const mockSaveProject = vi.fn(); | |
| let mockWorkspaceStore: RehearsalWorkspace | null = null; | |
| let workspaceSubscribers: Array<(ws: RehearsalWorkspace) => void> = []; |
🧰 Tools
🪛 GitHub Actions: release
[error] 8-8: ESLint (@typescript-eslint/no-explicit-any): Unexpected any. Specify a different type.
🪛 GitHub Check: ci / build-and-test
[failure] 9-9:
Unexpected any. Specify a different type
[failure] 8-8:
Unexpected any. Specify a different type
🪛 GitHub Check: release-preflight
[failure] 9-9:
Unexpected any. Specify a different type
[failure] 8-8:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 8 - 9, Replace the explicit any
types: change mockWorkspaceStore to the concrete RehearsalWorkspace type (e.g.,
RehearsalWorkspace | null) and type workspaceSubscribers as an array of callback
signatures that accept a RehearsalWorkspace (e.g., Array<(ws:
RehearsalWorkspace) => void> or the actual subscriber function type used
elsewhere). Update references in tests that push/invoke subscribers to match the
new callback signature so the ESLint rule `@typescript-eslint/no-explicit-any` is
satisfied and type checks pass for mockWorkspaceStore and workspaceSubscribers.
| id: "pack-ready2", | ||
| packState: "ready", | ||
| sourceLabel: "Ready Song", | ||
| song: { id: "song2" } as any |
There was a problem hiding this comment.
파이프라인 실패: any 타입 캐스팅 제거 필요
song: { id: "song2" } as any 대신 최소한의 유효한 RehearsalSong 구조를 사용하거나, 테스트 헬퍼 함수 createDemoRehearsalSong()을 활용해주세요.
🔧 수정 제안
+import { createDemoRehearsalSong } from "@bandscope/shared-types";
+
song: {
id: "pack-ready2",
packState: "ready",
sourceLabel: "Ready Song",
- song: { id: "song2" } as any
+ song: createDemoRehearsalSong()
}🧰 Tools
🪛 GitHub Check: ci / build-and-test
[failure] 203-203:
Unexpected any. Specify a different type
🪛 GitHub Check: release-preflight
[failure] 203-203:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` at line 203, Replace the unsafe cast "song: {
id: 'song2' } as any" with a minimal valid RehearsalSong object or call the test
helper createDemoRehearsalSong() so the test uses a real shape; update the test
in App.test.tsx to construct a RehearsalSong with required fields (or use
createDemoRehearsalSong('song2')/similar helper) and pass that into the
component instead of casting to any.
Comment on lines
+275
to
+276
| const badPack = mockWorkspaceStore.songs.find((s: any) => s.id === "non-existent"); | ||
| expect(badPack).toBeUndefined(); |
There was a problem hiding this comment.
파이프라인 실패: 타입 캐스팅을 SongRehearsalPack으로 변경
콜백 파라미터의 any 타입이 린트 오류를 발생시킵니다.
🔧 수정 제안
+import type { SongRehearsalPack } from "@bandscope/shared-types";
+
- const badPack = mockWorkspaceStore.songs.find((s: any) => s.id === "non-existent");
+ const badPack = mockWorkspaceStore?.songs.find((s: SongRehearsalPack) => s.id === "non-existent");🧰 Tools
🪛 GitHub Check: ci / build-and-test
[failure] 275-275:
Unexpected any. Specify a different type
🪛 GitHub Check: release-preflight
[failure] 275-275:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 275 - 276, Replace the loose any
cast on the find callback with the concrete SongRehearsalPack type to satisfy
linting: change the callback signature from (s: any) => ... to (s:
SongRehearsalPack) => ... when searching mockWorkspaceStore.songs for id
"non-existent" (the badPack variable), and add the appropriate import or type
reference for SongRehearsalPack if it's not in scope.
Comment on lines
+290
to
+291
| const badPack = mockWorkspaceStore.songs.find((s: any) => s.id === "non-existent"); | ||
| expect(badPack).toBeUndefined(); |
There was a problem hiding this comment.
파이프라인 실패: 동일하게 any 타입 수정 필요
Line 275와 동일한 문제입니다.
🔧 수정 제안
- const badPack = mockWorkspaceStore.songs.find((s: any) => s.id === "non-existent");
+ const badPack = mockWorkspaceStore?.songs.find((s: SongRehearsalPack) => s.id === "non-existent");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const badPack = mockWorkspaceStore.songs.find((s: any) => s.id === "non-existent"); | |
| expect(badPack).toBeUndefined(); | |
| const badPack = mockWorkspaceStore?.songs.find((s: SongRehearsalPack) => s.id === "non-existent"); | |
| expect(badPack).toBeUndefined(); |
🧰 Tools
🪛 GitHub Check: ci / build-and-test
[failure] 290-290:
Unexpected any. Specify a different type
🪛 GitHub Check: release-preflight
[failure] 290-290:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.test.tsx` around lines 290 - 291, The test uses an
explicit any for the finder callback which must be replaced with the proper song
type or left untyped to allow inference; update the
mockWorkspaceStore.songs.find callback (currently written as (s: any) => s.id
=== "non-existent") to either use the actual Song/WorkspaceSong type (e.g., (s:
Song) => s.id === "non-existent") after importing that type, or simply remove
the : any and write (s) => s.id === "non-existent" so TypeScript infers the
correct type; apply the same change you made at Line 275 to keep tests
type-safe.
Comment on lines
6
to
+7
| type AnalysisJobRequest, | ||
| type ProjectBootstrapSummary, | ||
| type RehearsalSong | ||
| type ProjectBootstrapSummary |
There was a problem hiding this comment.
파이프라인 실패: 사용되지 않는 import 제거 필요
AnalysisJobRequest와 ProjectBootstrapSummary가 import되었지만 사용되지 않습니다.
🔧 수정 제안
import {
SUPPORTED_AUDIO_FORMATS,
type RehearsalWorkspace,
- type SongRehearsalPack,
- type AnalysisJobRequest,
- type ProjectBootstrapSummary
+ type SongRehearsalPack
} from "@bandscope/shared-types";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type AnalysisJobRequest, | |
| type ProjectBootstrapSummary, | |
| type RehearsalSong | |
| type ProjectBootstrapSummary | |
| import { | |
| SUPPORTED_AUDIO_FORMATS, | |
| type RehearsalWorkspace, | |
| type SongRehearsalPack | |
| } from "@bandscope/shared-types"; |
🧰 Tools
🪛 GitHub Check: ci / build-and-test
[failure] 7-7:
'ProjectBootstrapSummary' is defined but never used
[failure] 6-6:
'AnalysisJobRequest' is defined but never used
🪛 GitHub Check: release-preflight
[failure] 7-7:
'ProjectBootstrapSummary' is defined but never used
[failure] 6-6:
'AnalysisJobRequest' is defined but never used
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.tsx` around lines 6 - 7, The file imports the types
AnalysisJobRequest and ProjectBootstrapSummary but never uses them; remove these
two unused imports from the import statement (or restore usage if they were
intended to be used) to clear the lint/compile failure—specifically edit the
import that currently lists AnalysisJobRequest and ProjectBootstrapSummary and
delete those two symbols.
| - **Output**: 4 or 6 discrete stems (vocals, bass, drums, other). | ||
|
|
||
| ### Track 3: Harmonic & Pitch Pipelines (#107) | ||
| ### Track 3: Harmonic & Pitch Pipelines (#107) (COMPLETED) |
There was a problem hiding this comment.
마크다운 포맷팅: 제목 아래에 빈 줄이 필요합니다.
정적 분석 도구(markdownlint)에서 MD022 규칙 위반을 감지했습니다. 제목 아래에 빈 줄을 추가해주세요.
📝 수정 제안
### Track 3: Harmonic & Pitch Pipelines (`#107`) (COMPLETED)
+
- **Goal**: Replace hardcoded `C#m7` strings with DSP-derived chord and pitch arrays.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Track 3: Harmonic & Pitch Pipelines (#107) (COMPLETED) | |
| ### Track 3: Harmonic & Pitch Pipelines (`#107`) (COMPLETED) | |
| - **Goal**: Replace hardcoded `C#m7` strings with DSP-derived chord and pitch arrays. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/2026-03-28-ml-engine-integration.md` at line 20, The heading "###
Track 3: Harmonic & Pitch Pipelines (`#107`) (COMPLETED)" violates markdownlint
MD022 because there is no blank line after the title; fix it by adding a single
empty line immediately after that heading line in the document so the title is
followed by a blank line as required.
Comment on lines
+99
to
+106
| # Compare chromagram frames to templates using dot product | ||
| # chromagram shape: (12, n_frames) | ||
| # templates shape: (24, 12) | ||
| # similarity shape: (24, n_frames) | ||
| similarity = np.dot(self.templates, chromagram) | ||
|
|
||
| # Find the best matching chord template for each frame | ||
| best_matches = np.argmax(similarity, axis=0) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
크로마그램 정규화 고려 필요.
템플릿은 L2 정규화되어 있지만(Line 42-43), 크로마그램 프레임은 정규화되지 않은 상태로 dot product에 사용됩니다. 이로 인해 에너지가 높은 프레임이 더 높은 유사도 점수를 받을 수 있습니다.
♻️ 크로마그램 프레임 정규화 제안
+ # Normalize chromagram frames for consistent similarity comparison
+ chroma_norms = np.linalg.norm(chromagram, axis=0, keepdims=True)
+ chroma_norms = np.where(chroma_norms > 0, chroma_norms, 1.0)
+ chromagram_normalized = chromagram / chroma_norms
+
# Compare chromagram frames to templates using dot product
# chromagram shape: (12, n_frames)
# templates shape: (24, 12)
# similarity shape: (24, n_frames)
- similarity = np.dot(self.templates, chromagram)
+ similarity = np.dot(self.templates, chromagram_normalized)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Compare chromagram frames to templates using dot product | |
| # chromagram shape: (12, n_frames) | |
| # templates shape: (24, 12) | |
| # similarity shape: (24, n_frames) | |
| similarity = np.dot(self.templates, chromagram) | |
| # Find the best matching chord template for each frame | |
| best_matches = np.argmax(similarity, axis=0) | |
| # Normalize chromagram frames for consistent similarity comparison | |
| chroma_norms = np.linalg.norm(chromagram, axis=0, keepdims=True) | |
| chroma_norms = np.where(chroma_norms > 0, chroma_norms, 1.0) | |
| chromagram_normalized = chromagram / chroma_norms | |
| # Compare chromagram frames to templates using dot product | |
| # chromagram shape: (12, n_frames) | |
| # templates shape: (24, 12) | |
| # similarity shape: (24, n_frames) | |
| similarity = np.dot(self.templates, chromagram_normalized) | |
| # Find the best matching chord template for each frame | |
| best_matches = np.argmax(similarity, axis=0) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/src/bandscope_analysis/chords/chord_recognizer.py`
around lines 99 - 106, The chromagram frames are not normalized before
dot-product with the L2-normalized templates, so high-energy frames bias
similarity; normalize each chromagram frame (columns of the chromagram matrix
used in chord_recognizer.py) to unit L2 norm prior to computing similarity (used
in the np.dot(self.templates, chromagram) step) and handle zero-energy frames
(e.g., leave as zeros or skip) so similarity and the subsequent best_matches =
np.argmax(similarity, axis=0) reflect spectral shape rather than frame energy.
Comment on lines
+65
to
+71
| if "vocals" in stems: | ||
| p_res = pitch_tracker.track(stems["vocals"], sr=sr) | ||
| if p_res: | ||
| vocal_range = { | ||
| "lowestNote": p_res["lowest_note"] or "", | ||
| "highestNote": p_res["highest_note"] or "", | ||
| } |
There was a problem hiding this comment.
p_res 조건 검사가 의도한 대로 동작하지 않을 수 있습니다.
PitchTracker.track()은 항상 TrackedPitchRange 딕셔너리를 반환하며 None을 반환하지 않습니다. 따라서 if p_res:는 피치 감지 실패 시에도 항상 True입니다.
현재 로직에서 피치 감지가 실패하면 {"lowestNote": "", "highestNote": ""}가 설정되어 기본값("G#3"/"C#5")을 덮어씁니다.
🐛 의도에 따른 수정 제안
피치 감지 실패 시 기본값을 유지하려면:
if "vocals" in stems:
p_res = pitch_tracker.track(stems["vocals"], sr=sr)
- if p_res:
+ if p_res and p_res.get("lowest_note") and p_res.get("highest_note"):
vocal_range = {
- "lowestNote": p_res["lowest_note"] or "",
- "highestNote": p_res["highest_note"] or "",
+ "lowestNote": p_res["lowest_note"],
+ "highestNote": p_res["highest_note"],
}bass 범위에도 동일하게 적용해주세요 (Lines 73-79).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "vocals" in stems: | |
| p_res = pitch_tracker.track(stems["vocals"], sr=sr) | |
| if p_res: | |
| vocal_range = { | |
| "lowestNote": p_res["lowest_note"] or "", | |
| "highestNote": p_res["highest_note"] or "", | |
| } | |
| if "vocals" in stems: | |
| p_res = pitch_tracker.track(stems["vocals"], sr=sr) | |
| if p_res and p_res.get("lowest_note") and p_res.get("highest_note"): | |
| vocal_range = { | |
| "lowestNote": p_res["lowest_note"], | |
| "highestNote": p_res["highest_note"], | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/src/bandscope_analysis/roles/extractor.py` around
lines 65 - 71, PitchTracker.track() never returns None, so the current guard "if
p_res:" wrongly treats failed detections as truthy and overwrites defaults;
update the logic in the vocal block (where p_res is assigned from
PitchTracker.track) to only overwrite vocal_range when p_res contains valid
pitch data (e.g., check p_res.get("lowest_note") and p_res.get("highest_note")
are non-empty/None or use an explicit success flag from TrackedPitchRange), and
apply the same change to the bass block that sets bass_range so defaults are
preserved when detection fails.
| np.random.seed(42) | ||
| y = np.random.randn(22050 * 2) * 0.1 | ||
| result = recognizer.recognize(y, sr=22050) | ||
| print("RESULT:", result) |
There was a problem hiding this comment.
디버그 출력문을 제거해주세요.
테스트 코드에 디버그용 print 문이 남아 있습니다. CI 로그가 불필요하게 길어질 수 있으므로 제거를 권장합니다.
🧹 수정 제안
result = recognizer.recognize(y, sr=22050)
- print("RESULT:", result)
# Could be N (No chord) or empty📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| print("RESULT:", result) | |
| result = recognizer.recognize(y, sr=22050) | |
| # Could be N (No chord) or empty |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/tests/test_chord_recognizer.py` at line 24, Remove
the debug print statement left in the test: delete the print("RESULT:", result)
call in services/analysis-engine/tests/test_chord_recognizer.py (the line that
prints the local variable result) so tests do not produce extraneous CI log
output; ensure no other debug prints remain in the same test function.
Comment on lines
+30
to
+45
| def side_effect_track(y, sr): | ||
| if y is vocals_stem: | ||
| return {"lowest_note": "A3", "highest_note": "A4"} | ||
| elif y is bass_stem: | ||
| return {"lowest_note": "E1", "highest_note": "E2"} | ||
| return None | ||
|
|
||
| mock_track.side_effect = side_effect_track | ||
|
|
||
| # Bass and other recognize results | ||
| def side_effect_recognize(y, sr): | ||
| if y is bass_stem: | ||
| return [{"chord": "Emaj", "start": 0.0, "end": 1.0}] | ||
| elif y is other_stem: | ||
| return [{"chord": "Amaj", "start": 0.0, "end": 1.0}] | ||
| return None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mock 반환값이 실제 TypedDict 구조와 일치하지 않습니다.
현재 테스트는 RoleExtractor가 해당 필드에 접근하지 않기 때문에 통과하지만, mock 데이터가 실제 구현과 일치하지 않습니다:
- PitchTracker.track() (Line 32, 34):
confidence키가 누락됨 - ChordRecognizer.recognize() (Line 42, 44):
start/end대신start_time/end_time이어야 함
향후 코드 변경 시 혼란을 방지하기 위해 수정을 권장합니다.
♻️ Mock 반환값 수정 제안
def side_effect_track(y, sr):
if y is vocals_stem:
- return {"lowest_note": "A3", "highest_note": "A4"}
+ return {"lowest_note": "A3", "highest_note": "A4", "confidence": "high"}
elif y is bass_stem:
- return {"lowest_note": "E1", "highest_note": "E2"}
+ return {"lowest_note": "E1", "highest_note": "E2", "confidence": "high"}
return None
mock_track.side_effect = side_effect_track
# Bass and other recognize results
def side_effect_recognize(y, sr):
if y is bass_stem:
- return [{"chord": "Emaj", "start": 0.0, "end": 1.0}]
+ return [{"chord": "Emaj", "start_time": 0.0, "end_time": 1.0}]
elif y is other_stem:
- return [{"chord": "Amaj", "start": 0.0, "end": 1.0}]
+ return [{"chord": "Amaj", "start_time": 0.0, "end_time": 1.0}]
return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/tests/test_roles_ml.py` around lines 30 - 45, The
mocks for PitchTracker.track and ChordRecognizer.recognize don't match the real
TypedDict shapes: update side_effect_track to include the "confidence" key in
the returned dicts for vocals_stem and bass_stem (matching PitchTracker.track),
and update side_effect_recognize to return chord dicts using "start_time" and
"end_time" instead of "start"/"end" for bass_stem and other_stem (matching
ChordRecognizer.recognize); modify the mock functions named side_effect_track
and side_effect_recognize used in the test to return these corrected keys so the
mocked shapes align with the real implementations.
This was referenced Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RehearsalWorkspace,SongRehearsalPack, andPackStateto@bandscope/shared-typeswith 100% test coverage.job_runner.tsbridging Tauri IPC for background task queuing and multi-song status reporting.App.tsxinto a multi-song rehearsal workspace withqueued,analyzing,ready, andfailedstatus support.Test Plan
App.test.tsxintegration tests running error-handling, local loads, and fallback components safely.vitest run --coveragelocally to verify logic regressions.