refactor(editor): move the drum mouse handlers into src/drum.js (R2, step 20)

[byrongamatos]

Cleanup, not a new lift. src/main.js 13,347 → 12,908.

What was stranded, and why

Step 15 moved the drum model, geometry, draw pass and undo commands into src/drum.js — and left 444 lines of mouse handlers and toolbar buttons behind. Not by design.

They sit physically under the Tempo Map section banner and have none of their own, so the coupling scan never saw them as a candidate and the drum lift never picked them up. Step 19 tripped over the same fact from the other side: the tempo banner’s window ran 440 lines past the end of the tempo code, straight through these. Cutting at TempoMapCmd rather than at the banner is what kept #170 honest — and left this behind to do properly.

One genuine cycle, avoided

They reach eight main.js symbols; seven were already host hooks. _refreshPartsViewButton joins them.

The eighth is the interesting one. _refreshTempoMapButton lives in src/tempo.js — and tempo.js already imports drum.js. A direct import would close a cycle. It crosses through host instead, which is precisely what host.js is for.

tempo.js ──imports──▶ drum.js
   ▲                     │
   └──── host.js ◀───────┘   (refreshTempoMapButton)

Verified: 22 modules, zero cycles.

Why a headless harness, again

verify_drum.py now clicks a hit into a lane. AddDrumHitCmd rewrites the toolbar’s ⟳ Drums (N) label through host.updateArrangementSelector, so a single number proves the whole chain: the click reached _drumEditorOnMouseDown, the command executed through the real EditHistory, and the hook fired. Undo puts it back.

check	wired	updateArrangementSelector unwired
clicking a lane adds a hit	PASS	FAIL
undo removes it again	PASS	PASS

Comment out that one hook and all 89 unit tests still pass.

Verification

node --test 89/89 · pytest 248/248 · npm run lint 0 errors (6 warnings) · Codex clean — it normalized the moved block byte-for-byte against the old main.js and confirmed the graph is acyclic · all 14 headless harnesses pass.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added comprehensive drum editor interactions: click-to-add hits, selection (including marquee), moving hits, deletion, and articulation toggling.
  • Added velocity editing with Alt-drag plus keyboard nudging/set for the current selection.
  • Updated drum/tempo editing mode coordination and toolbar controls, including row-density toggles and refreshed related button states.

• Documentation

  • Updated the changelog with the latest drum editor improvements.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e3fb906-3755-451c-82e7-fc235b52f714

📥 Commits

Reviewing files that changed from the base of the PR and between d5656a3 and 3651494.

📒 Files selected for processing (1)

• src/drum.js

🚧 Files skipped from review as they are similar to previous changes (1)

• src/drum.js

---

📝 Walkthrough

Walkthrough

The drum editor’s canvas handlers, velocity and selection commands, and toolbar controls are consolidated in src/drum.js. main.js imports the new exports and registers host refresh hooks, while host.js provides their defaults.

Changes

Drum editor

Layer / File(s)	Summary
Canvas hit editing and drag lifecycle src/drum.js	Canvas input supports hit insertion, selection, snapped movement, and density-aware piece remapping through drum commands.
Velocity and selection actions src/drum.js	Velocity editing, marquee selection, deletion, and articulation toggling are implemented through command dispatch.
Drum editor toolbar state src/drum.js	Drum-edit and row-density buttons update editor state, labels, visibility, selection, and active drags.
Main-module and host integration src/host.js, src/main.js, CHANGELOG.md	New drum exports and host refresh callbacks are wired, handshape drag finalization uses generic clearing, and the migration is documented.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Canvas
  participant DrumEditor
  participant DrumCommands
  participant Host
  Canvas->>DrumEditor: mouse and drag events
  DrumEditor->>DrumCommands: add, move, velocity, or selection command
  DrumCommands->>DrumEditor: committed editor state
  Host->>DrumEditor: refresh toolbar callbacks

Loading

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#89: Introduced the drum command classes used by the new editor interactions.
• got-feedBack/feedBack-plugin-editor#111: Added related drum velocity authoring flows.
• got-feedBack/feedBack-plugin-editor#120: Introduced the density lane-table semantics used for piece remapping and row toggling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main refactor: moving drum mouse handlers into src/drum.js.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/r2-step20-drum-handlers

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Refactors the editor code by relocating the drum editor canvas mouse/drag handlers and related toolbar button logic out of src/main.js into src/drum.js, reducing main.js size and keeping module dependencies acyclic via host callbacks.

Changes:

• Moved drum editor mouse/drag/velocity/selection handlers and drum toolbar button logic into src/drum.js and exported the needed entry points.
• Extended host with refreshTempoMapButton and refreshPartsViewButton hooks to avoid an import cycle (tempo.js already imports drum.js).
• Updated src/main.js to import the new drum editor handlers and wire the additional host hooks; documented the refactor in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/main.js	Removes inlined drum mouse/toolbar logic, imports new drum editor entry points, and wires new host hooks.
src/host.js	Adds refreshTempoMapButton and refreshPartsViewButton to the shared host callback surface.
src/drum.js	Adds the moved drum editor interaction handlers and toolbar button logic, switching dependencies to host.* where needed.
CHANGELOG.md	Notes the drum interaction/toolbar move and the cycle avoidance via host.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

🧹 Nitpick comments (1)

src/drum.js (1)

1106-1144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: S.drumSel = new Set() instead of .clear() for reset.

Elsewhere the selection is reset with .clear() (lines 1054, 1059). Reassigning the object here (line 1119) is inconsistent, though likely harmless since no other reference to the old Set appears to be cached across a mode toggle.

🤖 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 1106 - 1144, In _ensureDrumEditButton, reset the
drum selection with S.drumSel.clear() instead of replacing it with a new Set,
matching the existing selection-reset pattern and preserving references to the
selection set.

🤖 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 1106-1144: In _ensureDrumEditButton, reset the drum selection with
S.drumSel.clear() instead of replacing it with a new Set, matching the existing
selection-reset pattern and preserving references to the selection set.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: facaea42-4213-41c3-9841-34acc5c4e2ca

📥 Commits

Reviewing files that changed from the base of the PR and between 1528dd6 and d5656a3.

📒 Files selected for processing (4)

• CHANGELOG.md
• src/drum.js
• src/host.js
• src/main.js

[byrongamatos]

Right on both counts, and it is pre-existing wording that this PR moved verbatim — which is how it surfaced.

Both clamps are load-bearing, for different reasons, which is why the comment naming only one read as a mistake:

• the inner Math.max(0, rawT) stops the padding region left of t=0 from handing snapTime a negative time;
• the outer one catches snapTime rounding backward past zero when the first beat is offset from 0.

Comment now says both. t is const.