feat(editor): per-beat rubato drag in Tempo Map mode - #97
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds per-beat rubato dragging in Tempo Map mode, letting a non-downbeat grid tick be dragged independently while keeping surrounding downbeats fixed and re-spacing nearby sub-beats proportionally. It also wires the new drag state through existing handlers, updates the HUD text, and adds tests plus a changelog entry. ChangesTempo Map Rubato Drag
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MouseDownHandler as _tempoMapOnMouseDown
participant TempoBeatOnDragMove as _tempoBeatOnDragMove
participant DragEnd as _tempoMapOnDragEnd
User->>MouseDownHandler: press on sub-beat tick
MouseDownHandler->>MouseDownHandler: _tempoSubBeatAtX() hit-test
MouseDownHandler->>MouseDownHandler: start tempo-beat drag
User->>TempoBeatOnDragMove: drag mouse
TempoBeatOnDragMove->>TempoBeatOnDragMove: _tempoBeatDragBoundsPure() clamp
TempoBeatOnDragMove->>TempoBeatOnDragMove: rebuild S.beats with proportional re-spacing
User->>DragEnd: release mouse
DragEnd->>DragEnd: finalize tempo-beat drag via TempoMapCmd
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
tests/tempo_beat_drag.test.js (1)
78-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a bounds test for the leading/pickup-beat case.
Test coverage exercises the "no next downbeat" branch (trailing beat) but not the symmetric "no previous downbeat" branch (
pdb < 0 && d > 0) — the pickup-measure case the PR explicitly calls out. Worth a companion case for full branch coverage of_tempoBeatDragBoundsPure.Proposed additional test
t('bounds: leading sub-beat with no previous downbeat clamps to its neighbor', () => { // Two pickup sub-beats before the first downbeat. const beats = [ { time: 0.2, measure: -1 }, { time: 0.5, measure: -1 }, { time: 1.0, measure: 1 }, { time: 3.0, measure: 2 }, ]; const b = _tempoBeatDragBoundsPure(beats, 1, 0.005, 4); assert.ok(Math.abs(b.lo - (0.2 + 0.005)) < 1e-9, 'lo from immediate neighbor'); });🤖 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_beat_drag.test.js` around lines 78 - 89, Add a companion bounds test for the pickup/leading sub-beat path in _tempoBeatDragBoundsPure, since current coverage only exercises the trailing “no next downbeat” branch. Create a case where the selected beat has no previous downbeat (pdb < 0 && d > 0) and assert the lower bound clamps to the immediate neighbor/expected pickup behavior. Place it alongside the existing bounds tests in tempo_beat_drag.test.js so both symmetric branches are covered.screen.js (1)
12030-12030: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo hover affordance for the newly advertised beat-tick drag.
The HUD now tells users they can drag beat ticks for rubato, but the tempo-map hover branch only hit-tests poles (
_tempoSyncAtX), so the cursor never switches toew-resizeover a draggable sub-beat. Consider extending the hover logic to also probe_tempoSubBeatAtXso the affordance matches the advertised interaction.🤖 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 `@screen.js` at line 12030, The tempo-map HUD now advertises beat-tick dragging, but the hover logic in the tempo-map branch still only checks `_tempoSyncAtX`, so the cursor never shows the resize affordance over sub-beat ticks. Update the hover handling in `screen.js` to also probe `_tempoSubBeatAtX` alongside `_tempoSyncAtX`, and switch the cursor to `ew-resize` when either hit-test finds a draggable tempo element.
🤖 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 `@screen.js`:
- Line 12030: The tempo-map HUD now advertises beat-tick dragging, but the hover
logic in the tempo-map branch still only checks `_tempoSyncAtX`, so the cursor
never shows the resize affordance over sub-beat ticks. Update the hover handling
in `screen.js` to also probe `_tempoSubBeatAtX` alongside `_tempoSyncAtX`, and
switch the cursor to `ew-resize` when either hit-test finds a draggable tempo
element.
In `@tests/tempo_beat_drag.test.js`:
- Around line 78-89: Add a companion bounds test for the pickup/leading sub-beat
path in _tempoBeatDragBoundsPure, since current coverage only exercises the
trailing “no next downbeat” branch. Create a case where the selected beat has no
previous downbeat (pdb < 0 && d > 0) and assert the lower bound clamps to the
immediate neighbor/expected pickup behavior. Place it alongside the existing
bounds tests in tempo_beat_drag.test.js so both symmetric branches are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b96cf2b9-9344-4e65-b648-89377a64a71f
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/tempo_beat_drag.test.js
Sync-point poles retime whole measures — too coarse for a recording that accelerates or ritards INSIDE a bar (rubato classical takes, live drummers). Grabbing an individual beat tick (any non-downbeat grid line) now drags just that beat: - The measure's bounding downbeats stay fixed; neighbouring sub-beats re-space proportionally on both sides (the accel/rit shape) via the existing _tempoApplyDrag, which was already general over any beat index — this PR adds the hit-test, clamping, and drag routing. - New pure _tempoBeatDragBoundsPure clamps the drag inside the measure with per-gap 5 ms minimums, so beats can squeeze but never collapse or reorder; ends without a bounding downbeat clamp against their immediate neighbor / song duration. - Finalizes through the same revert-then-exec path as a pole drag: one undoable TempoMapCmd, equal-count invariant, notes ride per the tempo-ride scope. Poles win the hit-test when both are in reach, so existing pole muscle memory is unchanged. HUD hint updated. Tests: tests/tempo_beat_drag.test.js (7 cases: bounds math incl. unbounded ends, fixed downbeats, proportional both-side re-space, monotonic order at clamp extremes, adjacent-measure isolation) — the drag engine is extracted from source by name, waveform_render style. node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
_tempoBeatOnDragMove set dg.moved = true before the `if (!bounds) return` guard, so a drag rejected by _tempoBeatDragBoundsPure (measure too tight to fit all beats at 5ms gaps) still left moved===true. On mouseup, _tempoMapOnDragEnd then snapshotted the untouched grid and pushed an inert TempoMapCmd(origBeats, identical newBeats) — an empty undo entry that dirtied the tab with no visible change. Move dg.moved = true to after the bounds guard so a bounds-rejected wiggle never marks the drag as moved: no inert command, no spurious dirty. Add a bounds.lo slam-and-assert-monotonic test (symmetry with the existing hi-slam case). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3574aba to
ed710e1
Compare
2 participants
Summary
Sync-point poles retime whole measures — too coarse for recordings that accelerate or ritard inside a bar (rubato classical takes, live un-clicked drummers). This makes individual beat ticks draggable in Tempo Map mode:
_tempoApplyDrag) was already general over any beat index; this PR adds the sub-beat hit-test, safe clamping, and drag routing._tempoBeatDragBoundsPureclamps the drag inside the measure with per-gap 5 ms minimums (beats can squeeze, never collapse or reorder); pickup/trailing beats without a bounding downbeat clamp against their immediate neighbor or the song end.TempoMapCmd, equal-count invariant, notes riding per the tempo-ride scope. Poles win the hit-test when both are within reach, so existing pole-drag muscle memory is untouched. The Tempo Map HUD hint now mentions beat ticks.This completes the rubato toolset ordering from the workspace design (D19): tap-tempo (#93) → per-beat drag (this) → scrub-on-drag (queued after the audio phase) → beat-lock (open design question on flag storage).
Verification
node --check screen.jscleantests/tempo_beat_drag.test.js— 7 cases: bounds math (interior + unbounded ends + degenerates), fixed downbeats, proportional both-side re-space, monotonic order under a clamp-edge slam, adjacent-measure isolation. The drag engine is extracted from source by name (thewaveform_renderharness pattern), so the tests run the real code.🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit