feat(editor): magnetic drag snapping — guidelines attract, never lock - #264
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughDragging notes and sustain edges now uses zoom-aware magnetic snapping that releases beyond a screen-space radius. Alt-drag remains unsnapped, onset snapping is preserved, and dedicated tests cover grid, zoom, disabled, degenerate-grid, and group-delta cases. ChangesMagnetic drag snapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Note editor
participant Mouse as mouse drag handlers
participant Snap as magneticSnapTime
participant Group as _groupTimeDeltaPure
Editor->>Mouse: drag note or sustain edge
Mouse->>Snap: evaluate dragged time
Snap-->>Mouse: snapped or free pointer time
Mouse->>Group: compose group time delta
Group-->>Editor: apply note movement
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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/loop.js`:
- Around line 422-440: Update magneticSnapTime so snaps produced by snapTime()
to an onset use ONSET_SNAP_TOL as the pull-radius limit instead of the beat-grid
gap cap. Preserve the existing grid-gap-based radius for regular subdivision
snaps, and keep the existing snap-off and unavailable-grid behavior unchanged.
🪄 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: 6e44dad1-6db1-4e6a-900d-8bec4302bb99
📒 Files selected for processing (4)
CHANGELOG.mdsrc/loop.jssrc/mouse.jstests/magnetic_snap.test.mjs
15da069 to
f56896a
Compare
|
Onset finding is correct and was already fixed in f56896a (pushed before this review landed): an onset hit now passes through @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show f56896a --stat 2>/dev/null || echo "commit not found"
git log --all --oneline | grep -i f56896a || echo "not in history"Length of output: 2095 🏁 Script executed: #!/bin/bash
sed -n '1,80p' src/loop.js | grep -n "magneticSnapTime\|ONSET_SNAP_TOL\|MAGNET"
git show f56896a -- src/loop.jsLength of output: 2916
Kicking off a full review now. ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 30 minutes. |
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 `@node_modules`:
- Line 1: Remove the tracked node_modules symbolic link, add node_modules to the
repository’s ignore rules, and ensure the package manifest and lockfile are
tracked so dependencies can be installed reproducibly without committing local
dependency contents.
🪄 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: b3a28d4b-dbee-4762-a687-455cfe0dc5aa
📒 Files selected for processing (5)
CHANGELOG.mdnode_modulessrc/loop.jssrc/mouse.jstests/magnetic_snap.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mouse.js
- src/loop.js
- CHANGELOG.md
f56896a to
39f46b9
Compare
|
Correct catch on @coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -8
git show --stat 39f46b9 2>/dev/null || echo "commit not found"
git diff --stat main...HEAD 2>/dev/null || git diff --stat origin/main...HEAD 2>/dev/nullLength of output: 2560
Kicking off a full review now. ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 25 minutes. |
magneticSnapTime: a guideline attracts the dragged edge within a small screen-space radius (MAGNET_PX, zoom-aware) capped at 35% of the local guideline gap so a free band always survives between magnets; pulling past the snap point releases the edge to follow the pointer exactly. Wired into the note move drag (replacing the hard grid lock) and the sustain end-edge resize drag (previously unsnapped); Alt = fully free unchanged; clicks and the explicit resnap verb still snap fully. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
magneticSnapTime re-gated EVERY snapTime result on the grid magnet's radius — including the onset-snap hit. Onset mode is already a magnet (ONSET_SNAP_TOL = 70 ms is its radius) and onsets are sparse, so it can't degrade into the locked stepping the grid cap exists to prevent. But the cap is a fraction of the local guideline gap, and at a fine subdivision that gap is tiny: at 1/16 the magnet is ~11 ms, well under the 70 ms tolerance, so the attack the user aimed at got released and drags in onset mode silently stopped snapping to onsets — a regression against snapTime, which drags used before this branch. An onset hit now passes through untouched; the grid magnet still governs the grid fallback (and still releases mid-gap). Also pins the guards that had no test: zero guidelines and a single-beat degenerate grid return the input unchanged (no NaN out of the falloff). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
39f46b9 to
455d7cd
Compare
2 participants
What
The design call from this session: "It's magnetic, not locking." With snapping on, dragging was hard-locked to the grid (Alt = the only escape). Now, per the DAW piano-roll feel:
MAGNET_PX = 8), so it's zoom-aware like Logic's Smart snap: zoom in → the magnet spans less time → finer control for free.MAGNET_GAP_FRAC), so however dense the grid is (fine subdivision, low zoom), the middle of every gap stays free — without the cap, guidelines closer than the magnet's diameter would cover the whole timeline and dragging would degrade to locked stepping. Caught live: at default zoom with 1/4-beat snap, gaps are ~15px and an uncapped 8px magnet could never be escaped.Tests
tests/magnetic_snap.test.mjs(7, fail on main): stick-within / release-beyond, zoom awareness both directions, the gap-cap free band (mid-gap stays free even at 20 px/s), snap-off identity, the pinned constants, and the_groupTimeDeltaPurecomposition (a magnetic stick = zero group creep; a release = exact pointer delta). Full suite 147 green, lint 0 errors,routes.pyuntouched.Live-verified
Real pak, real mouse drags on a real note (canvas hit-tested): a 4px pull sticks to the guideline (note lands exactly on the downbeat), a 40px pull releases and lands at precisely the pointer position, off-grid (2.5333 vs expected 2.533). Zero page errors. The resize-edge path was exercised too (the first probe accidentally grabbed an edge — it resized without page errors).
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Bug Fixes