feat(editor): barline multi-select + marquee + bulk delete (tempo PR 5a) - #226
Merged
Conversation
📝 WalkthroughWalkthroughTempo Map now supports selecting multiple downbeat barlines with Shift-click, marquee selection, or Ctrl+A, displaying the selection in amber and deleting it as one undoable operation while preserving map bounds. ChangesTempo Map multiselect
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Input
participant TempoMap
participant TempoGridCmd
participant UndoRedo
User->>Input: Shift-click, marquee, Ctrl+A, or Delete
Input->>TempoMap: Update or delete tempoSelMulti
TempoMap->>TempoGridCmd: Create one grid command
TempoGridCmd->>UndoRedo: Record topology change
UndoRedo-->>TempoMap: Clear selection on undo or redo
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: 2
🧹 Nitpick comments (2)
tests/tempo_multiselect.test.mjs (2)
82-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert focus cleanup in the single-delete fallback.
This test should verify that
S.tempoSelis reset after deletion; otherwise stale focus could survive while all current assertions still pass.Suggested assertion
_tempoDeleteSelection(); assert.strictEqual(S.beats[8].measure, -1, 'the focused barline was demoted'); assert.strictEqual(S.history.undo.length, 1); + assert.strictEqual(S.tempoSel, -1, 'single focus cleared after delete');🤖 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/tempo_multiselect.test.mjs` around lines 82 - 88, Extend the test for _tempoDeleteSelection() in the with-nothing-multi-selected case to assert that S.tempoSel is reset after the focused beat is deleted. Keep the existing beat demotion and undo-history assertions unchanged.
103-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover reverse endpoint order for range selection.
Add the
(12, 4)case so the test protects the bidirectional Shift-click behavior.Suggested test extension
_tempoSelectDownbeatRange(4, 12); assert.deepStrictEqual([...S.tempoSelMulti].sort((a, b) => a - b), [4, 8, 12], 'downbeats 4,8,12 (not the sub-beats)'); + S.tempoSelMulti.clear(); + _tempoSelectDownbeatRange(12, 4); + assert.deepStrictEqual([...S.tempoSelMulti].sort((a, b) => a - b), [4, 8, 12], 'reverse range selects the same downbeats');🤖 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/tempo_multiselect.test.mjs` around lines 103 - 107, Extend the _tempoSelectDownbeatRange test to also invoke the range selection with reversed endpoints (12, 4), and assert that S.tempoSelMulti contains the same contiguous downbeats [4, 8, 12]. Preserve the existing forward-order assertion.
🤖 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/tempo.js`:
- Around line 787-789: Restrict the tempo-marquee initialization in the
empty-grid pointer handling branch to presses whose y-coordinate falls within
the grid’s vertical bounds. Preserve the existing waveform guard and
stationary-versus-drag behavior, while preventing blank areas such as the Tempo
HUD from creating or changing marquee selections.
- Around line 251-257: Update the barline styling in the rendering logic around
ctx.strokeStyle and ctx.fillStyle so poles included in S.tempoSelMulti receive
the amber selection treatment before the locked state is applied. Ensure
multi-selected locked poles visibly use the selection color and focus halo,
while preserving existing locked, hover, and unselected styling for other poles.
---
Nitpick comments:
In `@tests/tempo_multiselect.test.mjs`:
- Around line 82-88: Extend the test for _tempoDeleteSelection() in the
with-nothing-multi-selected case to assert that S.tempoSel is reset after the
focused beat is deleted. Keep the existing beat demotion and undo-history
assertions unchanged.
- Around line 103-107: Extend the _tempoSelectDownbeatRange test to also invoke
the range selection with reversed endpoints (12, 4), and assert that
S.tempoSelMulti contains the same contiguous downbeats [4, 8, 12]. Preserve the
existing forward-order assertion.
🪄 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: fae39895-5ec4-486b-8dca-75e824ef6f08
📒 Files selected for processing (6)
CHANGELOG.mdsrc/input.jssrc/mouse.jssrc/state.jssrc/tempo.jstests/tempo_multiselect.test.mjs
Charrette UX P5 / arch 1 (first half). Select and delete multiple Tempo Map barlines at once, without touching the single-focus model (S.tempoSel — which inspector / tap / lock / modulate / suggest all key on). - New S.tempoSelMulti: Set<downbeatIdx> (the S.drumSel pattern). Shift+click a pole extends the contiguous downbeat range; a drag on empty grid arms a tempo-marquee (the drum editor's deferred-3px `moved` idiom) that box-selects downbeats in its swept X range (plain replaces, Shift unions); Ctrl+A in mode selects every downbeat. The set is index-based, so it is CLEARED — never remapped — in TempoGridCmd exec/rollback and on Tempo Map exit. - Render: a light amber wash across the selected range, and selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass. - Bulk delete = ONE TempoGridCmd: _tempoDeleteBarlinesPure demotes every selected INTERIOR downbeat (never the first/last — the existing guard, generalized to a set) + one renumber. Reachable via Del and right-click "Delete N barlines". - Escape clears the selection, layered UNDER #215's suggest-dismiss (ghosts own Escape while showing). tests/tempo_multiselect.test.mjs (6): marquee hit math, the delete transform (demote+renumber, first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection. Verified live: Ctrl+A → "105 barlines selected" (amber render), Escape clears, a marquee box-selects, Ctrl+A+Del → "Deleted 103 barlines" (first/last kept), no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only on merged #215); keep-both seams with #220 (_tempoMapDraw) and #218/#225 on the shared tempo.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
force-pushed
the
feat/editor-tempo-multiselect
branch
from
July 12, 2026 20:06
393a2a0 to
f342417
Compare
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/tempo.js (1)
930-945: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the actual deletable count.
Ctrl+Aincludes the first and last downbeats, but_tempoDeleteSelection()filters them out. The menu can offer “Delete 5 barlines” while deleting only 3. Count valid interior selections for the label, and suppress/disable the bulk action when none are deletable.🤖 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/tempo.js` around lines 930 - 945, Update the context-menu logic around _tempoDeleteSelection and the nMulti count to derive the bulk-delete count from valid interior selections, excluding the first and last downbeats. Use that filtered count in the “Delete N barlines” label, and suppress or disable the delete-multi action when the count is zero while preserving the single-delete behavior.
🤖 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/tempo.js`:
- Around line 276-290: Update the tempoSelMulti rendering block to group
selected barlines into contiguous downbeat runs instead of using one global
minT/maxT range. Render a separate amber wash for each run, preserving the
existing timeToX bounds, fill styling, and canvas region so gaps between
disjoint selections remain unpainted.
---
Outside diff comments:
In `@src/tempo.js`:
- Around line 930-945: Update the context-menu logic around
_tempoDeleteSelection and the nMulti count to derive the bulk-delete count from
valid interior selections, excluding the first and last downbeats. Use that
filtered count in the “Delete N barlines” label, and suppress or disable the
delete-multi action when the count is zero while preserving the single-delete
behavior.
🪄 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: ba1e64eb-6a99-41ba-b480-77623317f9d4
📒 Files selected for processing (6)
CHANGELOG.mdsrc/input.jssrc/mouse.jssrc/state.jssrc/tempo.jstests/tempo_multiselect.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/state.js
- CHANGELOG.md
- src/mouse.js
Comment on lines
+276
to
+290
| // Multi-selected barlines (PR 5a): a light amber wash spanning the range | ||
| // between the outermost selected downbeats (the existing halo grammar). | ||
| if (S.tempoSelMulti && S.tempoSelMulti.size) { | ||
| let minT = Infinity, maxT = -Infinity; | ||
| for (const i of S.tempoSelMulti) { | ||
| const b = S.beats[i]; | ||
| if (b && b.measure > 0) { if (b.time < minT) minT = b.time; if (b.time > maxT) maxT = b.time; } | ||
| } | ||
| if (minT <= maxT) { | ||
| const xa = Math.max(LABEL_W, timeToX(minT)), xb = Math.min(w, timeToX(maxT)); | ||
| if (xb > xa) { | ||
| ctx.fillStyle = 'rgba(251,191,36,0.10)'; | ||
| ctx.fillRect(xa, (TIMELINE_TOP + WAVEFORM_H), xb - xa, gridBottom - (TIMELINE_TOP + WAVEFORM_H)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render disjoint selections as separate washes.
Shift-marquee unions can create disjoint selections, but this min/max fill paints the gaps as selected. Split the wash by contiguous downbeat runs so deletion feedback matches the actual selected barlines.
🤖 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/tempo.js` around lines 276 - 290, Update the tempoSelMulti rendering
block to group selected barlines into contiguous downbeat runs instead of using
one global minT/maxT range. Render a separate amber wash for each run,
preserving the existing timeToX bounds, fill styling, and canvas region so gaps
between disjoint selections remain unpainted.
This was referenced Jul 12, 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
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.
Tempo-mapping PR 5a (charrette UX P5 / arch 1, first half). Select and delete multiple Tempo Map barlines at once — without disturbing the single-focus model (
S.tempoSel, which inspector / tap / lock / modulate / suggest all key on).Selection
S.tempoSelMulti: Set<downbeatIdx>(theS.drumSelpattern), separate fromtempoSel.tempo-marquee(the drum editor's deferred-3pxmovedidiom) that box-selects downbeats in its swept X range — plain replaces, Shift unions.TempoGridCmdexec/rollback) and on Tempo Map exit.Render
A light amber wash across the selected range; selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass, no per-frame allocation.
Bulk delete
One
TempoGridCmd:_tempoDeleteBarlinesPuredemotes every selected interior downbeat (never the first/last — the existing guard generalized to a set) + one renumber. Via Del and right-click ▸ "Delete N barlines". Escape clears the selection, layered under #215's suggest-dismiss.Tests & verification
tests/tempo_multiselect.test.mjs(6): marquee hit math, the delete transform (demote+renumber + first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection.npm test115 green,npm run lint0 errors (3 pre-existing warnings = main's ratchet).Independent of the in-review queue (depends only on merged #215). Expect keep-both seams with #220 (
_tempoMapDraw) and #218/#225 on the sharedtempo.js. PR 5b (group drag) and PR 8 (range ops) stack on this.🤖 Generated with Claude Code
Summary by CodeRabbit