feat(editor): make a track's instrument first-class data (not a name guess) - #335
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughArrangement instrument identity is now stored as ChangesAuthored instrument type data
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Manifest
participant routes.py
participant Editor
participant instrument.js
participant EditorFeatures
Manifest->>routes.py: provide arrangement type
routes.py->>Editor: load arrangement with type
Editor->>instrument.js: resolve arrangement kind
instrument.js->>EditorFeatures: return canonical kind
EditorFeatures->>Editor: select layout, views, audio, and guards
Editor->>routes.py: save snapshot with type
routes.py->>Manifest: preserve arrangement type
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lanes.js`:
- Around line 104-106: Apply the typed bass detection consistently across all
final string-count resolution paths: in src/lanes.js lines 127-128 within
_stringCountFor, use _arrTypeKind(arr) with the existing name-regex fallback; in
routes.py lines 4001-4002 within _arrangement_string_count, check
arr.get("type") with the name-substring fallback; and in routes.py lines
4045-4046 within _is_extended_range, reuse the same typed is_bass logic. The
anchor at src/lanes.js lines 104-106 already has the correct behavior and
requires no direct change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d18bfa9c-2466-490f-b915-43a5ccccce78
📒 Files selected for processing (7)
CHANGELOG.mdroutes.pysrc/instrument.jssrc/keys.jssrc/lanes.jstests/instrument_type.test.mjstests/test_manifest_type_preserve.py
|
Follow-on commits on this branch turn the seam into a visible feature: a canonical |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parts-view.js (1)
172-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
arrKind(arr)to honor authored types for bass coloring.The parts view currently still relies on the legacy, name-only inference (
_partsArrKindPure(arr.name)) to color the bass tracks. This means a track with a non-bass name but an explicitly authoredbasstype will not receive the correct bass color, which contradicts the PR objective.Replace the name-based check with the canonical
arrKind(arr). Note: If_partsArrKindPureis no longer used elsewhere in this file, consider removing its definition as well.💡 Proposed fix
// Per-lane bass detection: isBassArr(arr) ignores its arg and tests the // armed part, which would paint every lane the armed part's colour. - ctx.fillStyle = _partsArrKindPure(arr.name) === 'Bass' ? 'rgba(255,170,90,0.8)' : 'rgba(150,220,150,0.8)'; + ctx.fillStyle = arrKind(arr) === 'bass' ? 'rgba(255,170,90,0.8)' : 'rgba(150,220,150,0.8)';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parts-view.js` around lines 172 - 174, Update the bass-color condition in the per-lane rendering logic to use the canonical arrKind(arr) result instead of _partsArrKindPure(arr.name), so explicitly authored bass types receive bass coloring regardless of track name. If _partsArrKindPure has no remaining references in parts-view.js after this change, remove its definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.js`:
- Around line 1072-1074: Update the stringsMode calculation near activeKind and
arrKind to return true only when activeKind is explicitly guitar or bass,
excluding vocals and all other instrument types. Preserve the existing
session-id check when toggling stringsBtn visibility.
---
Outside diff comments:
In `@src/parts-view.js`:
- Around line 172-174: Update the bass-color condition in the per-lane rendering
logic to use the canonical arrKind(arr) result instead of
_partsArrKindPure(arr.name), so explicitly authored bass types receive bass
coloring regardless of track name. If _partsArrKindPure has no remaining
references in parts-view.js after this change, remove its definition.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e78ce810-145c-4bb2-8db6-b6d0db139180
📒 Files selected for processing (10)
CHANGELOG.mdsrc/audio.jssrc/instrument.jssrc/key-view.jssrc/keys.jssrc/main.jssrc/parts-view.jssrc/tab-view-live.jssrc/track-session.jstests/instrument_type.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
The foundation for multiple drum charts (the drums-as-arrangements arc). The single drum tab has always lived ENTIRELY OUTSIDE S.arrangements[] as a lone off-array singleton edited through a global mode — the one instrument that wasn't an ordinary arrangement. This gives it a home IN the list as a derived `type:"drums"` arrangement (via the #335 instrument-type-as-data seam), the substrate multiple drum charts grow from. BYTE-IDENTICAL: no pack change, no UI change, drum-editor undo untouched. New leaf `src/drum-arrangement.js`: - `syncDrumArrangement(S)` materializes / updates / removes the drums arrangement so its `.drumTab` payload IS S.drumTab — the SAME object reference, so every existing S.drumTab reader/mutator and every drum undo command (which hold references into S.drumTab.hits) keep working unchanged. APPENDS (never inserts), so existing arr indices / arr:<idx> keys are stable. Called at every S.drumTab (re)assignment: load migration (file-ops), GP/MIDI import + empty-add (arrangement.js), delete + its undo-restore (DeleteDrumTabCmd). - `isDrumArrangement` keys on the authored `type` ALONE (normalized), NOT arrKind — arrKind would name-infer and wrongly catch a pitched part a user literally named "Drums", then hide/drop it. The materialized arrangement always carries `type:"drums"`, so a type-only test is exact and safe. - `pitchedArrangementCount` / `clampAwayFromDrums` / `pitchedIndexOf` keep the index math correct now that S.arrangements[] can hold a drums entry: the remove-last-arrangement guard counts pitched parts, S.currentArr never lands on the drums arrangement, and the /remove-arrangement backend index is mapped to its pitched-only position (the backend manifest has no drums arrangement). Byte-identical surfaces (the drums arrangement is bridged to the legacy paths; promoting them to arr:<idx> + drum-edit-via-selection is the follow-up): - Save (file-ops `_buildSaveBody`, create `editorBuild`): the drums arrangement is EXCLUDED from body.arrangements — drums still persist as the song-level `drum_tab`, so the built pack is byte-identical (and no drums entry reaches arrangements[], where an old core would fretted-grade it as garbage). - Tracks targets (`_trackSessionTargetsPure`), Parts view (`_partsListPure`), band roster (`_bandPartsPure`), pitched switcher (updateArrangementSelector): each skips the drums arrangement so it isn't listed twice — drums stay the legacy `'drums'` target/key. routes.py untouched. Tests (tests/drum_arrangement.test.mjs, +15): the sync state machine (materialize/update/remove/idempotent/append/same-ref/degrade), the remove→restore undo round-trip, byte-identical load→save, the "Drums"-NAMED-but- untyped safety case (survives save, keeps its arr target), no duplicate tracks/band rows, and the index helpers incl. the interspersed-drums backend index. JS 297/0, lint 0 err / 3 baseline, routes.py untouched (no pytest). Stacked on #335 (needs arrKind / _arrTypeKind / the `type` round-trip). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
eaf318e to
7aa3b73
Compare
2 participants
The first brick of the multitrack-feedpak program (
MULTITRACK-FEEDPAK-DESIGN.md— Phase 1 / Milestone A, the foundation drums-as-arrangements, two-drummers, and the instrument-identity badge all stand on). Independent of the region stack (#332–#334); lands offmain.The problem
The editor inferred a part's instrument (keys / bass / guitar) from its name, in ~a dozen places, with two subtly disagreeing rules — a runtime prefix test (
/^(keys|piano|…)/) vs. a save-side word-boundary test. That's why renaming a track can flip its lane layout, and why the rename dialog has to refuse some names. Meanwhile the pack format already records an authoredtypeper arrangement (feedpak-spec §5.2) — the backend persists it and never clobbers an authored value — but the frontend ignored it entirely.The change
Make instrument identity first-class data, with name inference kept only as the fallback for untyped/legacy packs:
src/instrument.js(new leaf):_typeKindcanonicalizes the manifesttypevocabulary ("piano"→keys, plus the plural setdrums/vocalsgrow into) to the editor's runtime kind, ornullwhen absent/blank/unrecognized. Imports nothing fromsrc/, sokeys.js/lanes.jsconsult it without a cycle.type, falling back to their exact prior name test when untyped:isKeysArr+viewFor(keys.js) and the 4-vs-6 bass baseline in_seedExtendedStringsFromTuning(lanes.js).routes.py): load carries each manifest entry'stypeonto the arrangement (mirroring the existingidcarry); the full-snapshot save carries an editor-providedtypeinto the rebuilt entry, so a set/changed type persists (the existing_merge_manifest_entrystill preserves an on-disk type when the editor sends none, and infer-once still stamps an untyped entry from its name).Behavior
Byte-identical for existing songs — their
typewas inferred from the same name the predicates read. The win: a part named against its instrument (a guitar called "Grand Piano") finally reads as what it is, and identity now survives a rename and can be set explicitly.Scope (deliberately tight)
The ~30 other name-inference sites and the rename-guard relaxation follow behind this same seam once every identity reader consults
type— not in this PR.Tests / gates
tests/instrument_type.test.mjs(7): the vocabulary map, type-over-name in both directions, and the byte-identical untyped fallback for keys + the bass baseline.tests/test_manifest_type_preserve.py(+2): a newly-typed arrangement persists its type; an editor-set type overrides a stale on-disk value while preserved keys survive.🤖 Generated with Claude Code
Summary by CodeRabbit