feat(editor): drum companion strip — VSTi-style kit view + sampler pad view, cursor input, GM MIDI monitor - #199
Merged
Merged
Conversation
… MIDI monitor The drum editor's counterpart to the fretboard companion strip (product owner's ask, 2026-07-10): a docked row of kit pads, one per drum piece in physical-kit family groups, doing three jobs at once: - VISUAL CUE: selected hits light their pads — the lane grid shows rows, the pads show the KIT. - INPUT: click a pad -> add a hit of that piece at the snapped cursor, through the same AddDrumHitCmd the lane grid uses (undo-able, sorted, dirty-flagged — pinned by a real-EditHistory round-trip test). - MIDI MAPPER: arm Listen and e-kit note-ons flash their pads live, mapped through GENERAL MIDI percussion to start (the import default). GM notes with no chart piece (claps, tambourine) flash nothing rather than the wrong pad; every pad's tooltip documents its GM notes, so the strip doubles as the mapping reference. Per-kit custom maps follow. The monitor NEVER grows a second device path: src/midi-record.js gains a small tap API (_midiMonitorTap/Untap + _midiMonitorEnsure) that routes every raw packet through the existing _recMidiOnData BEFORE the recording gate, riding the same refcounted domain session the Record modal manages. The tap loop is typeof-guarded so the sliced-source midi_domain suite's env stays clean (the standing recipe). On the legacy private Web-MIDI backend the tap sees data whenever the Record modal has a device connected — best-effort by design. Mode wiring crosses the host seam (host.refreshDrumPadStrip): drum.js flips drumEditMode but the strip already imports drum.js's command surface, so a direct import would close a cycle. Pads are DOM buttons (a11y + CSS flash for free) — a sidecar with zero per-frame cost. Shown in drum edit mode; the Pads toggle persists as an editor pref (never the pack). tests/drum_pad_strip.test.mjs (5): GM canonical assignments land only on real chart pieces, the pad model covers the kit exactly once in family groups, note-on parsing rejects offs/CC/short packets (channel-agnostic — GM drums arrive on channel 10), selection lighting, and the add-command round-trip. Suite 91/91, ESLint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a docked drum-pad strip with persisted Kit/Pads views, GM percussion mapping, clickable hit entry, selection lighting, MIDI listen flashing, editor lifecycle wiring, styling, and automated tests. ChangesDrum-pad companion strip
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DrumEditor
participant DrumPadStrip
participant AddDrumHitCmd
participant MidiMonitor
DrumEditor->>DrumPadStrip: Click pad or arm Listen
DrumPadStrip->>AddDrumHitCmd: Add snapped drum hit
DrumPadStrip->>MidiMonitor: Ensure monitor tap
MidiMonitor-->>DrumPadStrip: Deliver raw NOTE-ON packet
DrumPadStrip-->>DrumEditor: Flash mapped pad
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…er note) Rework the flat pad row into a KIT VIEW in the same companion slot as the fretboard strip: three rows arranged the way the kit faces the player — cymbal arc on top, hi-hats left of the tom arc in the middle, feet + snare + a wide kick at the bottom. Same three roles (selection cue, click-to-add at the cursor, GM MIDI Listen flash); KIT_VIEW_ROWS is pure layout data, and a new test pins that it covers DRUM_PIECE_ORDER exactly once. Pads grew to performance-size targets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Contributor
Author
|
Reworked per Christian's note: the flat pad row is now a performance-style kit view in the same companion slot as the fretboard strip (#198) — cymbal arc / hats + tom arc / feet + snare + wide kick. Same three roles (selection cue, click-to-add, GM Listen flash); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/drum_pad_strip.test.mjs (1)
81-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the redo result, not only its length.
After redo, the test verifies
hits.length === 2but not the reinserted piece, time, or velocity. A regression that redoes the wrong hit would pass.Proposed assertion
S.history.doRedo(); assert.strictEqual(S.drumTab.hits.length, 2); + assert.deepStrictEqual(S.drumTab.hits[0], hit);🤖 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/drum_pad_strip.test.mjs` around lines 81 - 97, Strengthen the redo verification in the “pad-click add: AddDrumHitCmd round-trip” test by asserting that the reinserted hit matches the original hit object, including its time, pitch, and velocity, rather than checking only hits.length. Use deep equality against hit at the expected sorted position after S.history.doRedo().src/drum-pad-strip.js (1)
208-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the
_midiMonitorEnsure()await.
_midiMonitorEnsureisn't fully wrapped upstream (e.g._recMidiDomain().listSources()can throw), andarmListen()is invoked fire-and-forget from the click handler (Line 233). A rejection here becomes an unhandled promise rejection, and the tap stays registered whilearia-pressed/monitorArmednever settle. Wrapping keeps state consistent on failure.♻️ Proposed guard
_midiMonitorTap(onMidiData); - const ok = await _midiMonitorEnsure(); + let ok = false; + try { ok = await _midiMonitorEnsure(); } + catch (e) { console.warn('[Editor] MIDI monitor ensure failed:', e); } monitorArmed = true; // the tap also works whenever the record modal has a session🤖 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/drum-pad-strip.js` around lines 208 - 224, Guard the await of _midiMonitorEnsure() inside armListen() with try/catch, handling failures by untapping onMidiData, resetting monitorArmed to false, restoring the button’s aria-pressed state, and reporting an appropriate status so the fire-and-forget click handler cannot produce an unhandled rejection or leave partial state.
🤖 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`:
- Line 1813: Update the drum-pad teardown associated with initDrumPadStrip() to
remove the registered onMidiData callback from the shared _midiTaps set when the
editor is destroyed or replaced; ensure the cleanup is safe when listening is
not armed and prevents stale MIDI taps from firing after reinjection.
In `@src/midi-record.js`:
- Around line 251-270: The _midiMonitorEnsure function must not initialize or
persist the record MIDI session from the drum-pad Listen path. Remove its
fallback device selection and _recMidiEnsureOpen call; instead, only re-arm the
listener on an already-established _recMidiHandle, returning whether that handle
is available, while leaving session creation and teardown to the record-session
lifecycle.
---
Nitpick comments:
In `@src/drum-pad-strip.js`:
- Around line 208-224: Guard the await of _midiMonitorEnsure() inside
armListen() with try/catch, handling failures by untapping onMidiData, resetting
monitorArmed to false, restoring the button’s aria-pressed state, and reporting
an appropriate status so the fire-and-forget click handler cannot produce an
unhandled rejection or leave partial state.
In `@tests/drum_pad_strip.test.mjs`:
- Around line 81-97: Strengthen the redo verification in the “pad-click add:
AddDrumHitCmd round-trip” test by asserting that the reinserted hit matches the
original hit object, including its time, pitch, and velocity, rather than
checking only hits.length. Use deep equality against hit at the expected sorted
position after S.history.doRedo().
🪄 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: 5f076380-350e-4a9d-93b8-c5573499d884
📒 Files selected for processing (9)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/drum-pad-strip.jssrc/drum.jssrc/host.jssrc/main.jssrc/midi-record.jstests/drum_pad_strip.test.mjs
…ler pad view Second rework per the product owner: 'kit view' means a drawn drum kit like a drum VSTi's UI, plus a SECOND, separate sample-pad view. - KIT: an inline-SVG kit graphic — kick, snare with a rim zone, rack + floor toms, the open/closed hat pair with its pedal, the cymbal wash on stands, the ride's bell as its own zone, cowbell on the kick mount (the VSTi hit-zone idiom). Every piece is its own SVG element (data-piece + <title> tooltip), so hit-testing, lit/flash CSS state and tooltips come free and nothing repaints per frame. KIT_GRAPHIC is pure shape data; a test pins it to DRUM_PIECE_ORDER (once each) and that zones (bell, hat pair) reference real instruments. - PADS: the sampler-style grid (three banks of six), same wiring. - One switcher (Kit | Pads buttons, editor-pref 'editorDrumPadView'; corrupt prefs collapse to kit). Both views light from the selection, click-to-add at the cursor, and flash from the GM MIDI monitor — everything routes through [data-piece], one code path. - Fix: the strip's click delegate guarded on HTMLElement, which silently dropped SVG clicks (SVG elements are SVGElement) — now Element. Suite 94 checks green (8 in this module), ESLint 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
Contributor
Author
|
Second rework per Christian: kit view now means a drawn kit — an inline-SVG drum-kit graphic with VSTi-style hit zones (snare head/rim, ride bow/bell, open/closed hat pair + pedal, cowbell) — plus a second, separate sampler-style pad view (three banks of six). One switcher (Kit ⇄ Pads, persisted editor pref); both views share the |
ChrisBeWithYou
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/drum-pad-strip.js (1)
190-218: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake SVG kit zones keyboard-operable.
The Kit view’s interactive SVG shapes are not focusable, and the delegate only handles
click. Keyboard-only users cannot add hits from the default Kit view.Proposed fix
function buildKitSvg(meta) { const el = []; @@ for (const s of KIT_GRAPHIC) { const cls = `editor-kit-piece is-${s.kind}`; const title = `<title>${gmTitle(meta, s.piece)}</title>`; + const attrs = `class="${cls}" data-piece="${s.piece}" tabindex="0" role="button" aria-label="${gmTitle(meta, s.piece)}"`; if (s.kind === 'drum' || s.kind === 'rim' || s.kind === 'kick') { - el.push(`<circle class="${cls}" data-piece="${s.piece}" cx="${s.cx}" cy="${s.cy}" r="${s.r}">${title}</circle>`); + el.push(`<circle ${attrs} cx="${s.cx}" cy="${s.cy}" r="${s.r}">${title}</circle>`); } else if (s.kind === 'cym' || s.kind === 'pedal') { - el.push(`<ellipse class="${cls}" data-piece="${s.piece}" cx="${s.cx}" cy="${s.cy}" rx="${s.rx}" ry="${s.ry}">${title}</ellipse>`); + el.push(`<ellipse ${attrs} cx="${s.cx}" cy="${s.cy}" rx="${s.rx}" ry="${s.ry}">${title}</ellipse>`); } else if (s.kind === 'bell') { - el.push(`<rect class="${cls}" data-piece="${s.piece}" x="${s.cx - s.w / 2}" y="${s.cy - s.h / 2}" width="${s.w}" height="${s.h}" rx="2">${title}</rect>`); + el.push(`<rect ${attrs} x="${s.cx - s.w / 2}" y="${s.cy - s.h / 2}" width="${s.w}" height="${s.h}" rx="2">${title}</rect>`); } @@ strip.addEventListener('click', (e) => { @@ }); + strip.addEventListener('keydown', (e) => { + if (e.key !== 'Enter' && e.key !== ' ') return; + const target = e.target instanceof Element ? e.target : null; + const pad = target?.closest('.editor-kit-piece[data-piece]'); + if (!pad) return; + e.preventDefault(); + addHitAtCursor(pad.dataset.piece); + });+#plugin-editor .editor-kit-piece:focus-visible { + stroke: `#f8fafc`; + stroke-width: 3; + outline: none; +}Also applies to: 345-354
🤖 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/drum-pad-strip.js` around lines 190 - 218, Make each interactive SVG shape generated by buildKitSvg keyboard-focusable by adding tabindex="0" and an appropriate role, while leaving decorative hardware and labels non-interactive. Update the Kit view’s delegated event handling to respond to Enter and Space keydown events by triggering the same hit-selection logic as click, and prevent default scrolling for Space. Ensure the equivalent handler near the alternate referenced section is updated as well.
🤖 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 `@src/drum-pad-strip.js`:
- Around line 190-218: Make each interactive SVG shape generated by buildKitSvg
keyboard-focusable by adding tabindex="0" and an appropriate role, while leaving
decorative hardware and labels non-interactive. Update the Kit view’s delegated
event handling to respond to Enter and Space keydown events by triggering the
same hit-selection logic as click, and prevent default scrolling for Space.
Ensure the equivalent handler near the alternate referenced section is updated
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b9f597c-d845-47ea-afa0-99ebd175f43f
📒 Files selected for processing (5)
CHANGELOG.mdassets/v3-theme.cssscreen.htmlsrc/drum-pad-strip.jstests/drum_pad_strip.test.mjs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.html
- _midiMonitorEnsure never auto-picks a device: it opens the SAVED record device only, so arming Listen can't silently overwrite the user's record-device preference — picking a device stays the Record MIDI modal's job (Listen says so when nothing is saved yet). - New _midiMonitorRelease(): Listen-off drops the monitor's shared session ref (refcounted at the domain; a no-op while a take is recording, so it can never yank the device from the record path). - initDrumPadStrip self-heals across re-boots: untap + disarm first, so a previous injection's armed Listen can't keep a stale tap firing behind a fresh DOM that shows Listen off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
The drum companion strip's MIDI monitor tap + domain device session were never released on screen teardown. Leaving the editor (or a re-injection) with Listen armed leaked the domain session and left onMidiData in the tap set. initDrumPadStrip's self-heal untapped the handler but never released the device, so the session ref leaked until an unrelated Record open/close. Add teardownDrumPadStrip(): untaps onMidiData, releases our device-session ref ONLY when we armed it (never yanks a session the Record modal opened), and clears pending flash timers. Wired into __editorScreenTeardown alongside the other injection-scoped cleanup. Idempotent and safe when never armed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
pushed a commit
that referenced
this pull request
Jul 12, 2026
The squash-merge resolutions that landed the sweep bar (#201) and the drum-pad strip (#199) each dropped their block's closing </div> at the canvas-wrap overlay anchor — the seam every chrome PR collides on. Since then #editor-canvas has been nested inside the HIDDEN drum-pad strip (itself inside the hidden sweep bar), so the canvas laid out at 0x0: chart and waveform invisible after every load and import, while the status bar reported the load and all 108 JS tests stayed green (nothing parses the markup). First tester-visible in the 20260711 nightly. Restores the two closers, byte-identical markup otherwise, and adds tests/screen_markup.test.mjs: a dependency-free tag-stack walk that fails if screen.html ever unbalances again, if #editor-canvas stops being a direct child of #editor-canvas-wrap, or if any canvas-wrap overlay becomes an ancestor instead of a sibling. Fails 3/3 on main. Verified end-to-end on the :8000 testbed via headless Chromium: before the fix the canvas had clientHeight 0 and the timeline was black; after, ruler + minimap + waveform + 1328 notes render. Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
The drum editor's counterpart to the fretboard companion strip (#198), per the product owner's notes: a docked row of kit pads — one per drum piece, in physical-kit family groups — doing three jobs:
AddDrumHitCmdthe lane grid uses (undo-able, sorted, dirty-flagged).No second device path
src/midi-record.jsgains a small tap API (_midiMonitorTap/_midiMonitorUntap+_midiMonitorEnsure): every raw packet routes through the existing_recMidiOnDatabefore the recording gate, riding the same refcounted domain session the Record modal manages. The tap loop is typeof-guarded so the sliced-sourcemidi_domainsuite's env stays clean. On the legacy private Web-MIDI backend the monitor sees data whenever the Record modal has a device connected — best-effort by design, documented in the module header.Wiring notes
host.refreshDrumPadStrip) —drum.jsflipsdrumEditModebut the strip importsdrum.js's command surface, so a direct import would close a cycle.Padstoggle persists as an editor pref, never in the pack.Tests
tests/drum_pad_strip.test.mjs(5, real-import): GM canonical assignments land only on real chart pieces, the pad model covers the kit exactly once in family groups, note-on parsing rejects offs/CC/short packets (channel-agnostic — GM drums arrive on channel 10), selection lighting, and the add-command round-trip through the real EditHistory. Full suite 91/91 · ESLint 0 errors · CHANGELOG updated.Screenshot (selection lighting + click flash) shared with Christian during review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit