refactor(editor): move the drum editor to src/drum.js (R2, step 15) - #166
Merged
Conversation
src/main.js 18,471 -> 17,757. Eighteenth module; the graph stays acyclic. 714 lines, the largest single lift of the split so far. Chosen by measurement, not feel: a free-identifier scan over every section banner in main.js ranked candidate clusters by how many main.js symbols they still reach for. The drum editor came out at three (draw, drawWaveform, updateArrangementSelector) for 714 lines — the best size-to-coupling ratio available. The Tempo Map editor is bigger but needs 23. drum.js carries the lane/density model, the limb-lint memo, drum geometry + hit-test, _drumEditorDraw, and the @pure:drum-cmds undo commands. main.js keeps the mouse/drag handlers that construct those commands, the toolbar buttons, the GM-percussion name table, and the MIDI-tempo import flow — which is about tempo, not drums, and was only sharing the section banner. Splitting there is what dropped TempoGridCmd off the dependency list. The three main.js symbols would close a cycle, so they arrive through setDrumHooks(), the same shape as setHistoryHooks() and setCanvas(). window.editorToggleDrumDensity is re-attached in main.js: a top-level `window.x =` throws when drum.js is imported under node, which its tests do. The typeof S / typeof editGen guards in _drumLimbConflicts existed only to keep the block eval-able in a sliced sandbox. Real imports make them dead. Tests: all four drum suites now import the real module. drum_density and drum_limb_lint were CJS and are now .mjs. drum_limb_lint's two source-locks on the memo key ("the body must mention editGen", "const key = ... gen") collapse into one behavioural assertion — bump editGen, the memo must recompute — which is strictly stronger: it fails when the key stops keying on editGen, and it cannot be satisfied by an incidental substring match. Verified beyond the unit tests, because they cannot see the hook wiring. verify_drum.py enters drum mode on a pack with a real drum_tab, and asserts Full density paints 18 lane rows, Compact paints 7, the toggle repaints, and drawWaveform's signature '#08081a' band appears inside the drum frame. Comment out setDrumHooks() and all 88 unit tests still pass while that harness fails on three of its seven checks. node --test 88/88, pytest 248/248, npm run lint 0 errors (9 warnings, was 10), Codex clean, all 11 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe drum editor's lane/density model, playability lint, rendering, hit-testing, and undo command logic are extracted from src/main.js into a new src/drum.js module. main.js now imports these via setDrumHooks to avoid an import cycle. Test suites for drum density, limb lint, undo, and velocity switch from eval-based extraction to direct ES module imports. CHANGELOG documents the refactor. ChangesDrum editor module extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MainJS as main.js
participant DrumJS as drum.js
participant State as S (shared state)
MainJS->>DrumJS: import commands, draw, hit-test, density helpers
MainJS->>DrumJS: setDrumHooks({draw, drawWaveform, updateArrangementSelector})
DrumJS->>State: read/update S.drumTab.hits, S.drumSel, S.drumTabDirty
DrumJS->>DrumJS: _drumEditorDraw renders lanes, hits, lint overlay
DrumJS->>MainJS: invoke updateArrangementSelector hook after edits
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)src/main.jsast-grep timed out on this file Comment |
There was a problem hiding this comment.
Pull request overview
This PR continues the src/main.js modularization by extracting the full “Drum editor” cluster into a dedicated module (src/drum.js) while keeping main.js as the wiring/orchestration layer (mouse/drag handlers, toolbar, and tempo/MIDI import flow). To avoid introducing import cycles, the extracted module receives a small set of callbacks via setDrumHooks() (mirroring the existing setHistoryHooks() / setCanvas() pattern).
Changes:
- Moved the drum editor implementation (lane/density model, limb-lint memoization, geometry + hit-testing, draw routine, and drum undo commands) from
src/main.jsinto newsrc/drum.js. - Added hook wiring in
src/main.js(setDrumHooks({ draw, drawWaveform, updateArrangementSelector })) and re-attachedwindow.editorToggleDrumDensityinmain.jsto keepsrc/drum.jsimportable under Node test runs. - Updated drum-related tests to import the real module instead of regex-slicing
main.js, and converted the remaining CJS drum tests to ESM.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/drum.js |
New module containing the extracted drum editor implementation and hook-based cycle breaks. |
src/main.js |
Imports drum exports, wires hooks via setDrumHooks(), and re-attaches the window handler. |
tests/drum_velocity.test.mjs |
Switches from source-slicing to direct imports; seeds real S and installs drum hooks. |
tests/drum_undo.test.mjs |
Same shift to real imports + setDrumHooks stubbing for DOM callback. |
tests/drum_limb_lint.test.mjs |
Drives real S.drag and editGen invalidation; source-lock reduced to draw-path assertion. |
tests/drum_density.test.mjs |
Imports density helpers/constants directly from src/drum.js (no slicing). |
CHANGELOG.md |
Documents the drum-editor extraction and the updated testing approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/drum_undo.test.mjs (1)
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared
makeEnvpattern.This
makeEnv()(seedState + setDrumHooks + trackHooks + command re-export) is nearly identical to the one intests/drum_velocity.test.mjs, and likelydrum_density/drum_limb_lintper the PR stack. Could live in a shared test helper to reduce duplication.🤖 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_undo.test.mjs` around lines 39 - 52, The makeEnv() setup is duplicated across multiple drum tests, so extract the shared seedState/setDrumHooks/trackHooks command wiring into a common test helper and reuse it here. Move the repeated environment construction for AddDrumHitCmd, DeleteDrumHitsCmd, MoveDrumHitsCmd, ToggleDrumArticulationCmd, _drumSortAndRemapSel, and EditHistory into a helper, then update this test to import and call that helper instead of defining its own makeEnv().src/drum.js (1)
154-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication: conflict-index flattening logic exists twice.
_drumConflictIndexSetPure(Lines 154-160) and the inline loop in_drumEditorDraw(Lines 352-354) both flattenconflicts[].indicesinto aSet. The draw path can't call_drumConflictIndexSetPuredirectly since it needs the memoized_lintConflictsresult rather than re-running the pure clusterer, but the flatten step itself could be factored into a small shared helper (e.g._flattenConflictIndices(conflicts)) used by both.♻️ Proposed refactor
+export function _flattenConflictIndices(conflicts) { + const set = new Set(); + for (const c of conflicts) for (const idx of c.indices) set.add(idx); + return set; +} + export function _drumConflictIndexSetPure(hits, epsilon) { - const set = new Set(); - for (const c of _drumLimbConflictsPure(hits, epsilon)) { - for (const idx of c.indices) set.add(idx); - } - return set; + return _flattenConflictIndices(_drumLimbConflictsPure(hits, epsilon)); }const _lintConflicts = _drumLimbConflicts(hits); - const _conflictIdx = new Set(); - for (const c of _lintConflicts) for (const idx of c.indices) _conflictIdx.add(idx); + const _conflictIdx = _flattenConflictIndices(_lintConflicts);Also applies to: 352-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.js` around lines 154 - 161, There is duplicated logic for flattening conflict indices into a Set in _drumConflictIndexSetPure and the inline loop inside _drumEditorDraw. Extract that shared flattening step into a small helper such as _flattenConflictIndices(conflicts), then use it both from _drumConflictIndexSetPure and the draw path so the memoized _lintConflicts result can be reused without rerunning the pure clusterer.
🤖 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.js`:
- Around line 154-161: There is duplicated logic for flattening conflict indices
into a Set in _drumConflictIndexSetPure and the inline loop inside
_drumEditorDraw. Extract that shared flattening step into a small helper such as
_flattenConflictIndices(conflicts), then use it both from
_drumConflictIndexSetPure and the draw path so the memoized _lintConflicts
result can be reused without rerunning the pure clusterer.
In `@tests/drum_undo.test.mjs`:
- Around line 39-52: The makeEnv() setup is duplicated across multiple drum
tests, so extract the shared seedState/setDrumHooks/trackHooks command wiring
into a common test helper and reuse it here. Move the repeated environment
construction for AddDrumHitCmd, DeleteDrumHitsCmd, MoveDrumHitsCmd,
ToggleDrumArticulationCmd, _drumSortAndRemapSel, and EditHistory into a helper,
then update this test to import and call that helper instead of defining its own
makeEnv().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c32b658-0da9-4f87-936c-e6ce0ce5259a
📒 Files selected for processing (7)
CHANGELOG.mdsrc/drum.jssrc/main.jstests/drum_density.test.mjstests/drum_limb_lint.test.mjstests/drum_undo.test.mjstests/drum_velocity.test.mjs
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
…s (R2, step 16) (#167) * refactor(editor): move the annotation lanes to src/annotation-lanes.js (R2, step 16) src/main.js 17,756 -> 16,078. Nineteenth module; the graph stays acyclic. 1,678 lines — the tone lane, the anchor lane and the handshape lane, which turned out to be a contiguous tail of the IIFE. They move together because they lean on each other: the handshape lane positions itself off _anchorLaneTopY, and both it and the anchor lane share _currentAnchorArr. Split apart, those would be cross-module imports for no gain. main.js keeps the canvas event routing (deciding which strip is under the cursor) and forwards to the on*LaneMouse* handlers. Four main.js symbols travel back — draw, hideContextMenu, snapTime, _editorPromptText — and would close a cycle, so they arrive through setLaneHooks(). snapTime stays behind because its onset-snap path reaches _ensureOnsets and the onset cache; _editorPromptText stays because it owns a modal and the shared _editorPromptCancel handle. Neither is a lane concern. TONE_LANE_H moved to geometry.js, where ANCHOR_LANE_H and HS_LANE_H already live. The tones modal's three window.* handlers became exported functions that main.js re-attaches: a top-level `window.x =` throws when the module is imported under node. Two internal call sites that reached back through window.editorHideTonesModal() now call the module-local function (Codex). Also fixes a regression this refactor's own predecessors introduced. `draw` is reassigned near the bottom of main.js to a wrapper that refreshes seven toolbar buttons before repainting. setHistoryHooks() and setDrumHooks() were handed the bare identifier, so they captured the ORIGINAL function at wiring time; every undo, redo and drum-density toggle has been skipping those refreshes since #165/#166. The canvas repaints either way, which is exactly why it went unnoticed — the only visible symptom is the drum-density button keeping its "Rows: Full" label after the grid has collapsed to Compact. All three hook sites now take `_drawLive = (...args) => draw(...args)`, resolving the live binding at call time as the in-IIFE call sites always did. Found by Codex; verify_drum.py now asserts the label, and fails on the pre-fix code. Tests: anchor_authoring (CJS -> .mjs) and handshape_authoring stop brace-matching declarations out of main.js and import them. Verified beyond the unit tests, which cannot see hook wiring: verify_lanes.py clicks the anchor lane and asserts an anchor is added, the canvas repaints (_hooks.draw), the anchor lands on the grid rather than under the cursor (_hooks.snapTime), and the tones modal opens through the re-attached window.* handler. Comment out setLaneHooks() and all 88 unit tests still pass while three of its eight checks fail. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 12 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): correct stale IIFE / brace-matching comments in the moved code Copilot, on #167. The three comments travelled verbatim with the code and now describe an ES module: TONE_LANE_H comes from geometry.js rather than being declared at the top of the IIFE; _editorConfirmToneDefinitions is an ordinary export, not a plain function relying on the global -> IIFE fallback; and _handshapeSpanFrets is imported, not brace-matched out of main.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
… step 17) (#168) * refactor(editor): collapse the per-module hooks into src/host.js (R2, step 17) history.js, drum.js and annotation-lanes.js each grew a setXHooks() for the same reason: they need a few main.js symbols that cannot be imported back without closing a cycle. By the fourth module the SAME four callbacks — draw, hideContextMenu, snapTime, editorPromptText — were being threaded through three separate hook objects, and the next extraction (the note command classes) needs nine. That is the moment to stop duplicating. One `host` object, wired once. A new module imports `host` and calls `host.draw()`; no new plumbing, no fourth setter. No behaviour change: every former _hooks.X() call site resolves to the same callback (verified by Codex, one-for-one, including the two renamed keys _editorPromptText -> editorPromptText and ensureArr). The draw thunk stays, and host.js's header now carries the warning where the next person will actually read it: `draw` is reassigned near the bottom of main.js to a button-refreshing wrapper, so passing the bare identifier captures the original function and the refreshes silently stop. That shipped in #165/#166. The header says: pass a thunk, and check `grep -n '^\s*<name> = '` before wiring anything. The inert defaults are type-honest rather than uniformly no-op — snapTime is the identity, editorPromptText resolves to null (a cancelled prompt) — so a module imported under node with no host wired degrades instead of crashing. That is exactly how the unit tests exercise them, which is also why the unit tests cannot see the wiring: comment out setHostHooks() and all 88 still pass, while verify_history.py, verify_drum.py and verify_lanes.py all fail. node --test 88/88, pytest 248/248, npm run lint 0 errors, Codex clean, all 12 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): make the host.js grep portable and drop stale setXHooks references Copilot, on #168. - host.js suggested `grep -n '^\s*<name> = '`. GNU grep accepts `\s` as an extension so it works here, but BSD/macOS grep treats it as a literal 's' and returns a silent zero match — the worst possible answer for a check whose whole job is to catch a reassignment. Switched to a POSIX class and said why. - drum_undo.test.mjs comment still named setDrumHooks. - Three CHANGELOG entries in the same Unreleased block still described the per-module setters that this PR removes, so the release notes contradicted the code they ship with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 9, 2026
… 19) (#170) * refactor(editor): move the Tempo Map editor to src/tempo.js (R2, step 19) src/main.js 15,042 -> 13,347. Twenty-second module; the graph stays acyclic. 1,720 lines: the measure model, _tempoMapDraw, the mouse handlers, the sync inspector, tap-tempo, beat-lock respacing, and the two undo commands (TempoGridCmd, TempoMapCmd). main.js is now 37% smaller than the 21,176 lines this refactor started from. The banner said 2,179 lines, but ~440 of those are the drum editor's mouse handlers and toolbar buttons, which have no banner of their own and were left behind by the drum.js lift. The real tempo region ends at TempoMapCmd. Cutting there is what kept the dependency surface honest. main.js keeps _finalizeActiveDrag. It dispatches whatever canvas drag is in flight — tempo, drum, handshape, pan — before a mode switch, so it belongs to none of them; it reaches back as host.finalizeActiveDrag(). Fifteen main.js symbols travel the other way: the transport (startPlayback / stopPlayback), the A/B loop strip (four callbacks), the toolbar readouts, and getMousePos. `_recState` is a REASSIGNED module scalar rather than a function, so it cannot cross as a value at all — it is wired as the predicate `host.isRecording: () => _recState === 'recording'`, a closure that reads the live binding. Same class of trap as `draw` in #165/#166, caught this time by looking for it. Tests: 15 suites stopped slicing @pure: blocks and command classes out of main.js. Ten were CJS and are now .mjs. A few now rely on host's inert defaults where they used to inject no-op stubs — equivalent, and Codex confirmed it. Verified beyond the unit tests, which cannot see host wiring: verify_tempo.py arms drum-edit mode, enters Tempo Map, and asserts the canvas repaints, that _tempoMapDraw paints its HUD line, and that the drum button relabels itself from "🎸 Back to Notes" back to "🥁 Edit Drums" — the only visible proof that host.refreshDrumEditButton fired when tempo mode kicked drum mode out. Comment out `draw` and `refreshDrumEditButton` and all 89 unit tests still pass while four of the harness's nine checks fail. node --test 89/89, pytest 248/248, npm run lint 0 errors (7 warnings, was 8), Codex clean, all 14 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(editor): drop the dead sync-inspector memo and correct stale headers Copilot, on #170. - _tempoSyncInspectorState was computed, assigned, and never compared: dead code, and a lint warning. Removed rather than 'completed' into an early return, because that early return would be WRONG. The DOM this function writes is not a pure function of the signature: the BPM field is deliberately left alone while it has focus, so skipping the writes on an unchanged signature would strand whatever the user typed and abandoned, with nothing else to restore it. The comment now says so. - tempo.js's header hard-coded 'Fourteen main.js symbols'. The number was already wrong (fifteen) and would rot again. Removed. - Five test headers still said the helpers they import live in src/main.js. Copilot named two; the other three had the same defect. The five suites that genuinely still slice main.js keep their references, which are accurate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 16, 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
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.
Eighteenth module.
src/main.js18,471 → 17,757 — 714 lines, the largest single lift of the split so far. The graph stays acyclic.Chosen by measurement
Rather than guessing at the next cluster, I ran a free-identifier scan over every section banner in
main.jsand ranked them by how manymain.jssymbols each still reaches for:The drum editor wins on size-to-coupling. (The 47 command classes remain the biggest single chunk, but they are interleaved with the feature code that constructs them and are still not a coherent lift.)
The split line
drum.jstakes the lane/density model, the limb-lint memo, drum geometry + hit-test,_drumEditorDraw, and the@pure:drum-cmdsundo commands.main.jskeeps what genuinely belongs to it: the mouse/drag handlers that construct those commands, the toolbar buttons, the GM-percussion name table, and the MIDI-tempo import flow — which is about tempo, not drums, and was only sharing the section banner. Splitting there is what droppedTempoGridCmdoff the dependency list and got the count to three.Those three (
draw,drawWaveform,updateArrangementSelector) would close a cycle, so they arrive viasetDrumHooks()— same shape assetHistoryHooks()andsetCanvas().window.editorToggleDrumDensityis re-attached inmain.js, because a top-levelwindow.x =throws whendrum.jsis imported under node, which its tests do.The
typeof S/typeof editGenguards in_drumLimbConflictsexisted only to keep the block eval-able in a sliced sandbox. Real imports make them dead.Tests
All four drum suites now import the real module instead of regex-slicing it;
drum_densityanddrum_limb_lintwere CJS and are now.mjs.drum_limb_linthad two source-locks on the memo key — “the body must mentioneditGen”, “const key = ... gen”. They collapse into one behavioural assertion: bumpeditGen, the memo must recompute. Strictly stronger — it fails when the key stops keying oneditGen, and it cannot be satisfied by an incidental substring match (editGencontainsgen).Why a headless harness, again
Unit tests import
drum.jsdirectly, so they are blind to the hook wiring.verify_drum.pyenters drum mode on a pack with a realdrum_tab.json:drawWaveform’s#08081aband appears in the drum framesetStatusComment out
setDrumHooks(...)and all 88 unit tests still pass while the harness fails three of seven. The status line still updates, so the buttons and messages look fine — only the canvas is stale.Verification
node --test88/88 ·pytest248/248 ·npm run lint0 errors (9 warnings, down from 10 —_drumConflictIndexSetPureis no longer an unused orphan) · Codex clean · all 11 headless harnesses pass.The
main.jsdiff is 729 deletions against 14 added lines: one import block, the hook call, and the re-attachedwindow.*handler.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests