fix(editor): re-land barline group drag (tempo PR 5b) — #230 never reached main - #258
Merged
Merged
Conversation
…230) * feat(editor): group drag for a barline multi-selection (tempo PR 5b) Grabbing any pole in a Tempo Map multi-selection now drags the whole group by one offset, instead of only the single focused barline. - _tempoGroupDragClampPure: the group's Δt is clamped by the tightest headroom of any selected pole against its nearest UNSELECTED (fixed) downbeat ± MIN_MEASURE, so the group moves rigidly and can never cross a fixed neighbour or reorder the grid. Song start (0) / passed `duration` bound a pole with no fixed neighbour on that side. - _tempoApplyGroupDragPure: shifts every selected downbeat by the clamped Δt, then re-spaces each measure's interior proportionally between its (possibly moved) downbeats — a span between two selected poles rigid-shifts for free, an edge span re-spaces against its fixed outside pole, and a leading pickup / trailing tail rigid-shifts only when its bounding downbeat moved. Pure (never mutates the input). - Locked poles are dropped from the moving group: they stay put and act as fixed anchors; a status notes how many stayed. Locks defend hand-verified times, so excluding them is least surprising. - Wiring mirrors the single-pole drag: mousedown starts a 'tempo-group' drag when a multi-selection (2+) is grabbed by one of its poles; _tempoMapOnDragMove rebuilds from the original grid each move (no compounding); the shared _tempoMapOnDragEnd finalizes it as one undoable TempoMapCmd (equal-count invariant) — notes ride the grid. The selection is index-preserving, so it survives the drag. tests/tempo_group_drag.test.mjs (12 cases): clamp math both directions, fixed-pole-between-two-selected binding, rigid interior + edge re-space, pickup/tail shift, locked-pole exclusion, all-locked no-op, purity, and an exec -> undo -> redo round-trip that also checks a note rides. Fails on main (the pures don't exist). 116 JS green, lint 0-err. Verified live on the testbed: Ctrl+A -> drag a pole -> "Moved 105 barlines together." Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q * Prevent locked group drag fallthrough --------- Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
📝 WalkthroughWalkthroughChangesTempo Map group dragging
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant TempoMap
participant TempoMapCmd
Editor->>TempoMap: drag selected unlocked barline
TempoMap->>TempoMap: clamp delta and rebuild beats
Editor->>TempoMap: release drag
TempoMap->>TempoMapCmd: record group-drag undo step
TempoMapCmd-->>Editor: updated beats and note times
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1009-1042: Update the group-drag condition around S.tempoSelMulti
and S.drag to require that the grabbed hit is movable, not merely selected. When
hit refers to a locked selected pole, refuse to start the group drag and
preserve the selection without falling through to single-pole dragging; retain
the existing behavior for movable anchors and all-locked selections.
🪄 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: 75dbb172-7351-4d08-b987-cd90b69f50db
📒 Files selected for processing (4)
CHANGELOG.mdsrc/mouse.jssrc/tempo.jstests/tempo_group_drag.test.mjs
Comment on lines
+1009
to
+1042
| // Group drag (PR 5b): grabbing a pole that belongs to a multi-selection | ||
| // of 2+ downbeats drags the whole selection rigidly. Locked poles are | ||
| // excluded — they defend re-fits, so excluding them is least surprising — | ||
| // and the status says how many stayed put. The selection is KEPT (a | ||
| // TempoMapCmd is index-preserving), so the group survives the drag. | ||
| if (S.tempoSelMulti && S.tempoSelMulti.size >= 2 && S.tempoSelMulti.has(hit)) { | ||
| const all = [...S.tempoSelMulti]; | ||
| const movable = all.filter(i => S.beats[i] && S.beats[i].measure > 0 && !S.beats[i].locked); | ||
| if (movable.length) { | ||
| if (hit !== S.tempoSel) _tapTempo = null; | ||
| S.tempoSel = hit; | ||
| S.drag = { | ||
| type: 'tempo-group', | ||
| selIdxs: movable, | ||
| startX: x, | ||
| startTime: xToTime(x), | ||
| origBeats: S.beats.map(b => ({ ...b })), | ||
| moved: false, | ||
| }; | ||
| const lockedOut = all.length - movable.length; | ||
| if (lockedOut > 0) { | ||
| setStatus(`Dragging ${movable.length} barline${movable.length === 1 ? '' : 's'} — ` | ||
| + `${lockedOut} locked ${lockedOut === 1 ? 'stays' : 'stay'} put.`); | ||
| } | ||
| host.draw(); | ||
| return; | ||
| } | ||
| // Every selected pole is locked: keep the group selected and do not | ||
| // fall through to a single-pole drag, which would move a protected | ||
| // anchor despite the group no-op rule. | ||
| setStatus('Selected barlines are locked — unlock one to move it.'); | ||
| host.draw(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Grabbing a locked pole inside a mixed-lock multi-selection starts a group drag anchored on a pole that never moves.
S.tempoSelMulti.has(hit) only checks selection membership, not whether hit itself is locked. If the user selects {4, 8} with 8 locked, and clicks directly on pole 8 (locked), this branch still fires: movable = [4], and S.drag is armed with startX/startTime taken from the locked pole's click position. As the drag proceeds, pole 8 (under the cursor) never moves — only pole 4 does — so the cursor visually detaches from the barline it grabbed. The existing "every selected pole is locked" guard (lines 1036-1041) only covers the all-locked case, not this partial case, and it isn't covered by the test suite either.
🔧 Proposed fix — refuse to start a group drag anchored on a locked pole
if (S.tempoSelMulti && S.tempoSelMulti.size >= 2 && S.tempoSelMulti.has(hit)) {
const all = [...S.tempoSelMulti];
const movable = all.filter(i => S.beats[i] && S.beats[i].measure > 0 && !S.beats[i].locked);
+ if (S.beats[hit] && S.beats[hit].locked) {
+ // Grabbing the locked pole itself would anchor the drag on a
+ // point that never moves, detaching the cursor from the pole
+ // it grabbed. Require an unlocked member instead.
+ setStatus('This barline is locked — grab an unlocked one to move the group.');
+ host.draw();
+ return;
+ }
if (movable.length) {Want me to add a regression test for this (grabbing the locked member of a mixed-lock selection) alongside the fix?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Group drag (PR 5b): grabbing a pole that belongs to a multi-selection | |
| // of 2+ downbeats drags the whole selection rigidly. Locked poles are | |
| // excluded — they defend re-fits, so excluding them is least surprising — | |
| // and the status says how many stayed put. The selection is KEPT (a | |
| // TempoMapCmd is index-preserving), so the group survives the drag. | |
| if (S.tempoSelMulti && S.tempoSelMulti.size >= 2 && S.tempoSelMulti.has(hit)) { | |
| const all = [...S.tempoSelMulti]; | |
| const movable = all.filter(i => S.beats[i] && S.beats[i].measure > 0 && !S.beats[i].locked); | |
| if (movable.length) { | |
| if (hit !== S.tempoSel) _tapTempo = null; | |
| S.tempoSel = hit; | |
| S.drag = { | |
| type: 'tempo-group', | |
| selIdxs: movable, | |
| startX: x, | |
| startTime: xToTime(x), | |
| origBeats: S.beats.map(b => ({ ...b })), | |
| moved: false, | |
| }; | |
| const lockedOut = all.length - movable.length; | |
| if (lockedOut > 0) { | |
| setStatus(`Dragging ${movable.length} barline${movable.length === 1 ? '' : 's'} — ` | |
| + `${lockedOut} locked ${lockedOut === 1 ? 'stays' : 'stay'} put.`); | |
| } | |
| host.draw(); | |
| return; | |
| } | |
| // Every selected pole is locked: keep the group selected and do not | |
| // fall through to a single-pole drag, which would move a protected | |
| // anchor despite the group no-op rule. | |
| setStatus('Selected barlines are locked — unlock one to move it.'); | |
| host.draw(); | |
| return; | |
| } | |
| // Group drag (PR 5b): grabbing a pole that belongs to a multi-selection | |
| // of 2+ downbeats drags the whole selection rigidly. Locked poles are | |
| // excluded — they defend re-fits, so excluding them is least surprising — | |
| // and the status says how many stayed put. The selection is KEPT (a | |
| // TempoMapCmd is index-preserving), so the group survives the drag. | |
| if (S.tempoSelMulti && S.tempoSelMulti.size >= 2 && S.tempoSelMulti.has(hit)) { | |
| const all = [...S.tempoSelMulti]; | |
| const movable = all.filter(i => S.beats[i] && S.beats[i].measure > 0 && !S.beats[i].locked); | |
| if (S.beats[hit] && S.beats[hit].locked) { | |
| // Grabbing the locked pole itself would anchor the drag on a | |
| // point that never moves, detaching the cursor from the pole | |
| // it grabbed. Require an unlocked member instead. | |
| setStatus('This barline is locked — grab an unlocked one to move the group.'); | |
| host.draw(); | |
| return; | |
| } | |
| if (movable.length) { | |
| if (hit !== S.tempoSel) _tapTempo = null; | |
| S.tempoSel = hit; | |
| S.drag = { | |
| type: 'tempo-group', | |
| selIdxs: movable, | |
| startX: x, | |
| startTime: xToTime(x), | |
| origBeats: S.beats.map(b => ({ ...b })), | |
| moved: false, | |
| }; | |
| const lockedOut = all.length - movable.length; | |
| if (lockedOut > 0) { | |
| setStatus(`Dragging ${movable.length} barline${movable.length === 1 ? '' : 's'} — ` | |
| `${lockedOut} locked ${lockedOut === 1 ? 'stays' : 'stay'} put.`); | |
| } | |
| host.draw(); | |
| return; | |
| } | |
| // Every selected pole is locked: keep the group selected and do not | |
| // fall through to a single-pole drag, which would move a protected | |
| // anchor despite the group no-op rule. | |
| setStatus('Selected barlines are locked — unlock one to move it.'); | |
| host.draw(); | |
| return; | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 1029-1030: React's useState should not be directly called
Context: setStatus(Dragging ${movable.length} barline${movable.length === 1 ? '' : 's'} —
+ ${lockedOut} locked ${lockedOut === 1 ? 'stays' : 'stay'} put.)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 1038-1038: React's useState should not be directly called
Context: setStatus('Selected barlines are locked — unlock one to move it.')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 1009 - 1042, Update the group-drag condition
around S.tempoSelMulti and S.drag to require that the grabbed hit is movable,
not merely selected. When hit refers to a locked selected pole, refuse to start
the group drag and preserve the selection without falling through to single-pole
dragging; retain the existing behavior for movable anchors and all-locked
selections.
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 happened
#230 shows MERGED but its code never reached main. Its base was still
feat/editor-tempo-multiselect(the #226 stack parent) when it was merged, so merge commit91b75e8landed on that dead branch — GitHub never retargeted the base to main after #226's squash. Result: every build since ships barline multi-select without group drag — grabbing a pole after Ctrl+A/marquee silently clears the selection and drags one pole. A tester (Kisscool) hit exactly this tonight ("I can't move them in bulk, only select them").I audited every other recently-merged PR (#226–#253): only #230 was lost — all other merge commits are ancestors of main.
What this PR is
A cherry-pick of the lost
91b75e8onto current main (a866705), with two keep-both integrations:_tempoMapOnDragEnd: group status message composed with feat(editor): light onset-snap on a dragged tempo-map barline (tempo PR 11) #235's onset-snap confirmation (group drags don't onset-snap, so the two are exclusive branches).TempoOffsetCmd(fix(editor): route Sync / BPM-rescale / Offset through undoable total-reproject commands #218) kept below the new group pures.Everything else applied clean: the two pures (
_tempoGroupDragClampPure/_tempoApplyGroupDragPure), thetempo-groupdrag wiring in mousedown/move/end,mouse.jsrouting, the 13-case test suite, CHANGELOG.Verification
tests/tempo_group_drag.test.mjs), lint 0 errors,routes.pyuntouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Undo/Redo
Status