feat(editor): multi-track MIDI playback — the chart plays as a band, mixed by the Tracks strips - #280
Merged
Merged
Conversation
…mixed by the Tracks strips DAW move: 'All tracks' (Mixer header toggle + registry command + Transport menu row, persisted pref) voices EVERY part's charted notes through its own GM instrument simultaneously; drum-grid hits clap along. The Mixer's Tracks strips become a real mixer over the band. - audio.js: @pure:midi-playback block (_bandPartsPure mirrors the mixer strip keys exactly — 'arr:<idx>'/'drums' — so strips and engine can never disagree; part-scoped dedupe keys so two parts on the same millisecond both sound); per-part GainNodes into the guide bus, ramped ~20 ms via host.partMixChanged (the stem-mixer architecture); the band scheduler branch (per-part GM program by kind, ensure/ready per preset, clap fallback per bucket, unit-scale voices — the gain node owns the strip level); _guideSourceTimes unions all parts in band mode (a bass outro past the lead's last note bounds the song again); seek/wrap clears the band dedupe (typeof-guarded for the sliced compose_transport suite); teardown drops the gains. - keys.js: _rollPitchCtxFor(arr) — the per-arrangement form of the one shared pitch converter, so the band can never disagree with the roll. - mixer-panel.js: _mixerPartStripState(key) (whole-map solo rule per key), every strip write pokes host.partMixChanged, the header toggle renders its pressed state from the pref. - host.js: partStripState / partMixChanged / stripUiChanged / playAllTracksEnabled hooks, inert defaults. OFF = today's behavior, bit-identical (the single-guide path is the untouched else-branch). tests/midi_playback.test.mjs (5 cases, fails on main): roster/strip-key mirror, part-scoped dedupe, cross-part DAW rule, pref round-trip + panel notify, all-parts duration bound. Live-verified on :8001 (AC/DC, 3 tracks): toggle + status, playback schedules voices (instrumented node counts), soloing Rhythm silences Lead across parts, zero errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a persisted “Play all tracks” mode with mixer-aware per-track MIDI playback, controls in the Mixer and Transport menu, command wiring, expanded scheduling bounds, and tests for playback routing, persistence, live toggling, and timing. ChangesPlay all tracks band mode
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MixerPanel
participant editorTogglePlayAllTracks
participant GuideScheduler
participant MixerStripState
MixerPanel->>editorTogglePlayAllTracks: Toggle band mode
editorTogglePlayAllTracks->>GuideScheduler: Refresh playback preference
GuideScheduler->>MixerStripState: Read per-part mute, solo, and volume
GuideScheduler->>GuideScheduler: Schedule pitched and drum events
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🧹 Nitpick comments (1)
tests/midi_playback.test.mjs (1)
44-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated, indentation-fragile source-slicing logic.
The
indexOf('function ' + name)…indexOf('\n}', at) + 2extraction algorithm inslice()(lines 46-50) is re-implemented verbatim inline at lines 116-117 to build the injectable_guideSourceTimesfunction. Both rely on the target function's closing brace sitting at column 0 while every nested block's closing brace is indented — true today, but a reformat ofaudio.js(e.g. a differently-indented block, or a trailing comment after}) breaks this silently for whichever copy wasn't checked, and since both call sites run outsidet()'s try/catch, a break surfaces as a raw crash rather than a clean "FAIL" line.Consider extracting a single
sliceBody(name)that returns the raw string, then have both the plainslice()helper and the parameterizedFunctionconstruction build on top of it.♻️ Suggested consolidation
function slice(name) { + return sliceBody(name); +} +function sliceBody(name) { const at = src.indexOf(`function ${name}`); - const body = src.slice(at, src.indexOf('\n}', at) + 2); + return src.slice(at, src.indexOf('\n}', at) + 2); +} +function slice(name) { + const body = sliceBody(name); return new Function(`return (${body.replace(`function ${name}`, 'function')})`)(); }const fn = new Function('S', 'notes', '_guideSanitizeTimesPure', 'editorPlayAllTracksEnabled', - `return (${src.slice(src.indexOf('function _guideSourceTimes'), src.indexOf('\n}', src.indexOf('function _guideSourceTimes')) + 2).replace('function _guideSourceTimes', 'function')})()`); + `return (${sliceBody('_guideSourceTimes').replace('function _guideSourceTimes', 'function')})()`);Also applies to: 116-117
🤖 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 `@tests/midi_playback.test.mjs` around lines 44 - 52, Consolidate the duplicated source-extraction logic by adding a shared sliceBody(name) helper that returns the raw function body from src. Update slice(name) and the parameterized _guideSourceTimes Function construction to use this helper, preserving their existing behavior while ensuring both call sites use the same extraction implementation.
🤖 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/audio.js`:
- Around line 1294-1304: Update the band-mode timing and eligibility logic
around the arrangement-length checks and the all-track time collection to
include drum data, allowing drum-only charts and drum hits beyond arrangement
notes to determine the transport duration. Preserve existing arrangement
behavior while removing the S.arrangements.length-only gates in the affected
paths.
- Around line 1386-1389: Update the missing-node creation path in the part-gain
initialization logic around _partGains and the later mixer-state application so
each newly created GainNode receives the current mixer gain/mute/solo state
before any notes are scheduled. Ensure the initial value is applied immediately
after creating the node, while preserving the existing bus.guideGain connection
and subsequent scheduling behavior.
- Around line 1368-1375: Update editorTogglePlayAllTracks to reset queued guide
scheduling when the playback mode changes: clear or invalidate pending scheduled
guide voices and reset _guideScheduledUntil before applying the new mode.
Preserve the existing preference persistence, status update, UI notification,
and return behavior.
---
Nitpick comments:
In `@tests/midi_playback.test.mjs`:
- Around line 44-52: Consolidate the duplicated source-extraction logic by
adding a shared sliceBody(name) helper that returns the raw function body from
src. Update slice(name) and the parameterized _guideSourceTimes Function
construction to use this helper, preserving their existing behavior while
ensuring both call sites use the same extraction implementation.
🪄 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: eb191917-e18e-483f-abcd-4e27b6d448a9
📒 Files selected for processing (11)
CHANGELOG.mdscreen.htmlsrc/audio.jssrc/host.jssrc/input.jssrc/keys.jssrc/main.jssrc/menu-bar.jssrc/mixer-panel.jssrc/shortcuts.jstests/midi_playback.test.mjs
This was referenced Jul 15, 2026
…nit (review #280 items 8-10) Item 8: _guideSourceTimes unions drum-grid hits in band mode (before the no-arrangements early-out) and _guideTick gates on the real band roster — a drum-only chart plays and a late drum hit bounds the song. Item 9: a live Play All Tracks toggle cancels the old mode's queued voices and rewinds the schedule watermark to the current transport time so the next tick refills in the new mode, both directions. Item 10: _ensurePartGain seats a fresh gain node at the strip's current mute/solo/volume before the first voice connects — no unity leak. Regression suite drives the real _guideTimerSync/_guideTick over a recording fake AudioContext; all six cases fail on the pre-fix code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…ecar Band mode (multi-track MIDI playback) clapped only the 'drums' sidecar key. A drum-ENCODED arrangement — the created/imported/legacy "Drums" part that lives in S.arrangements (key 'arr:i') — has no pitch, so _bandPartPitchedEvents returns [] by design. With no clap fallback on the arrangement path, that whole part voiced neither GM nor clap and went SILENT in band mode, even though single-part mode sounds it. This contradicts the feature's promise that EVERY track plays. Root cause: the clap path was gated on part.key === 'drums' only. Unify it so percussion — the drum grid OR any /^drums/ arrangement — claps its rhythm through its own part gain (mute/solo/volume still apply). Regression test in tests/band_review_fixes.test.mjs (fails pre-fix: drum-arrangement claps absent; also asserts the mute path seats the clap's gain at 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/band_review_fixes.test.mjs (1)
301-306: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the clap fallback, not only event timing.
This assertion passes for any voice scheduled at
0.01and0.05; it would not catch routing the drum arrangement through a pitched GM voice instead of the required clap fallback. Capture and assert the voice/instrument type as well.🤖 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 `@tests/band_review_fixes.test.mjs` around lines 301 - 306, Update the assertion around tickOnce and the collected ctx.oscs entries to verify both charted start times and the expected clap fallback voice/instrument type. Match each relevant event by its startAt value and assert its voice/instrument identity, so pitched GM routing at those times cannot satisfy the test.
🤖 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.
Nitpick comments:
In `@tests/band_review_fixes.test.mjs`:
- Around line 301-306: Update the assertion around tickOnce and the collected
ctx.oscs entries to verify both charted start times and the expected clap
fallback voice/instrument type. Match each relevant event by its startAt value
and assert its voice/instrument identity, so pitched GM routing at those times
cannot satisfy the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d42ebdb9-51af-4f4c-9797-8b003be47f69
📒 Files selected for processing (2)
src/audio.jstests/band_review_fixes.test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/audio.js
# Conflicts: # CHANGELOG.md
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
2 participants
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.
What
Multi-track MIDI playback — the chart plays as a band, and the Mixer mixes it. A new All tracks toggle (Mixer panel header, Transport menu, command palette; persisted editor pref) makes every track voice its GM instrument simultaneously — lead and rhythm as guitars, bass as bass, the drum grid clapping along — instead of today's single current-track guide voice. The Tracks strips stop being clap-gates and become a real mixer over the band: per-track volume / mute / solo ramp live per-part gain nodes (~20 ms, never a pop, never a restart), with the standard DAW rule (mute wins; any solo isolates).
OFF = today's behavior, bit-identical — the single-guide path is the untouched else-branch.
Design
_bandPartsPure) uses exactly the mixer panel's strip keys (arr:<idx>/drums), and per-part audibility comes from the panel's own whole-map solo pure through a host hook._rollPitchCtxFor(arr)is the per-arrangement form of_rollPitchCtx, so the band's pitches can never disagree with the roll/strip (capo-aware fretted, keys packing).ensureGmPresetper program) and falls back to one clap per bucket until ready — the existing guide contract, per part.compose_transportsuite)._guideSourceTimesunions all parts, so a bass outro past the lead's final chord no longer cuts the song short.Relationship to #275 (stem mixer)
Complementary, not conflicting: #275 mixes the recording (audio stems), this mixes the chart (MIDI voices). Both hang per-source gain nodes off the mixer panel with live ramps — same idiom, different axis. Expect an adjacent keep-both in
host.js/mixer-panel.js/audio.jswhichever merges second.Testing
tests/midi_playback.test.mjs(5 cases, fails on main): roster/strip-key mirroring (empty drum tab is not a part), part-scoped dedupe, the cross-part DAW rule (someone else's solo silences you; mute beats own solo), pref round-trip + panel notification, and the all-parts duration bound vs active-only. Full suites green: 159 files / 0 fail, lint at the 3-warning baseline. Live-verified on the testbed (AC/DC, Lead/Rhythm/Bass): toggle + honest status, voices schedule during playback (instrumented node counts), soloing Rhythm silences Lead through the real strips, zero page errors.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Tests