feat(editor): light onset-snap on a dragged tempo-map barline (tempo PR 11) - #235
Conversation
…PR 11)
The manual complement to Suggest-fit (G): in Tempo Map mode with Snap = Onset,
dragging a barline gently pulls to the nearest detected audio attack, so
downbeats land on real hits instead of by eye.
- The snap is LIGHT — a few pixels' worth of grab (so the pull feels the same at
any zoom), capped at 50ms so a zoomed-out view can't reach a distant attack and
it never fights a deliberate drag. It never crosses a neighbouring barline (the
snapped time is re-clamped into the drag's existing bounds).
- **Locked barlines never snap** (they defend their time); with Snap = Grid the
drag stays a plain continuous move. A drag-end status confirms when a barline
lands on an attack.
- Wiring is one gated branch in `_tempoMapOnDragMove` over two new pures
(`_tempoOnsetSnapTolPure`, `_tempoOnsetSnapPure`) that reuse the existing
`_nearestOnsetTimePure` / `_ensureOnsets` onset infrastructure — no new state,
no per-frame allocation, no backend calls.
`tests/tempo_onset_snap.test.mjs` (9: pixel window + seconds cap + NaN-safety,
snap-within-tol, bound re-clamp, no-onset/no-window no-ops, and drag-handler
source guards for the Snap=Onset + non-locked gate). 126 JS suites green, lint
0-err (3 pre-existing ratchet warnings). routes.py untouched.
Verified live on AC/DC — Back In Black: with Snap = Onset, dragging a barline
snapped it to a detected attack ("Barline snapped to a detected attack at
6.96s"); Snap = Grid produced no snap; no errors from the change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
Warning Review limit reached
Next review available in: 19 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 (2)
📝 WalkthroughWalkthroughTempo Map barline dragging now snaps to nearby detected audio onsets in Onset mode, while respecting locked barlines and drag bounds. Drag completion reports successful snaps, and tests cover utility behavior and integration guards. ChangesTempo Map onset snapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant TempoMapDrag
participant OnsetCache
participant StatusFeedback
Editor->>TempoMapDrag: drag barline
TempoMapDrag->>OnsetCache: read live onset cache
OnsetCache-->>TempoMapDrag: detected onsets
TempoMapDrag->>TempoMapDrag: snap eligible drag
TempoMapDrag->>StatusFeedback: show snap status on completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🤖 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 2088-2092: Update _tempoOnsetSnapPure to return null when no onset
is within tolerance, or the clamped numeric snap time when one is found,
eliminating per-call result-object allocation. Adjust _tempoMapOnDragMove to
interpret that return value and derive dg.snappedT and snapped state while
preserving existing drag behavior.
- Around line 2078-2082: Update _tempoOnsetSnapTolPure to validate secPerPx and
pxWindow individually before multiplication, treating non-finite or non-positive
values as zero so negative pairs cannot produce a positive window and Infinity
multiplied by zero cannot yield NaN. Preserve the existing non-negative maxSec
cap and return the bounded tolerance.
In `@tests/tempo_onset_snap.test.mjs`:
- Around line 65-80: Replace the regex-only tests for _tempoMapOnDragMove and
_tempoMapOnDragEnd with behavioral coverage using runtime integration or pure
gating/status helpers, verifying onset-mode gating, locked-pole bypass, and
conditional snap status. Also update body() to assert that the end marker is
found before slicing, preventing a missing marker from producing slice(s, -1).
🪄 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: 7d8628bc-5d2b-45d9-af81-ebb9b7b8a946
📒 Files selected for processing (3)
CHANGELOG.mdsrc/tempo.jstests/tempo_onset_snap.test.mjs
| export function _tempoOnsetSnapTolPure(secPerPx, pxWindow, maxSec) { | ||
| const px = Math.max(0, (Number(secPerPx) || 0) * (Number(pxWindow) || 0)); | ||
| const cap = Math.max(0, Number(maxSec) || 0); | ||
| return Math.min(px, cap); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate tolerance operands before multiplying.
Clamping only the product means two negative inputs can produce a positive snap window, e.g. (-1, -6), and Infinity * 0 produces NaN. Validate each operand individually so non-positive inputs reliably disable snapping.
Proposed fix
export function _tempoOnsetSnapTolPure(secPerPx, pxWindow, maxSec) {
- const px = Math.max(0, (Number(secPerPx) || 0) * (Number(pxWindow) || 0));
- const cap = Math.max(0, Number(maxSec) || 0);
- return Math.min(px, cap);
+ const sec = Number(secPerPx);
+ const pxWindowN = Number(pxWindow);
+ const cap = Number(maxSec);
+ if (!(sec > 0) || !(pxWindowN > 0) || !(cap > 0)) return 0;
+ return Math.min(sec * pxWindowN, cap);
}📝 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.
| export function _tempoOnsetSnapTolPure(secPerPx, pxWindow, maxSec) { | |
| const px = Math.max(0, (Number(secPerPx) || 0) * (Number(pxWindow) || 0)); | |
| const cap = Math.max(0, Number(maxSec) || 0); | |
| return Math.min(px, cap); | |
| } | |
| export function _tempoOnsetSnapTolPure(secPerPx, pxWindow, maxSec) { | |
| const sec = Number(secPerPx); | |
| const pxWindowN = Number(pxWindow); | |
| const cap = Number(maxSec); | |
| if (!(sec > 0) || !(pxWindowN > 0) || !(cap > 0)) return 0; | |
| return Math.min(sec * pxWindowN, cap); | |
| } |
🤖 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 2078 - 2082, Update _tempoOnsetSnapTolPure to
validate secPerPx and pxWindow individually before multiplication, treating
non-finite or non-positive values as zero so negative pairs cannot produce a
positive window and Infinity multiplied by zero cannot yield NaN. Preserve the
existing non-negative maxSec cap and return the bounded tolerance.
| export function _tempoOnsetSnapPure(rawT, onsets, tol, loBound, hiBound) { | ||
| const near = _nearestOnsetTimePure(onsets, rawT, tol); | ||
| if (near === null) return { t: rawT, snapped: false }; | ||
| return { t: Math.max(loBound, Math.min(hiBound, near)), snapped: true }; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid allocating a result object on every drag frame.
_tempoMapOnDragMove invokes _tempoOnsetSnapPure for every onset-mode move, and the helper allocates { t, snapped } even when no snap occurs. This conflicts with the PR objective of avoiding per-frame allocation and can add garbage-collection churn during dragging. Return null for no snap and the clamped numeric time for a snap, then derive dg.snappedT in the caller.
Proposed fix
export function _tempoOnsetSnapPure(rawT, onsets, tol, loBound, hiBound) {
const near = _nearestOnsetTimePure(onsets, rawT, tol);
- if (near === null) return { t: rawT, snapped: false };
- return { t: Math.max(loBound, Math.min(hiBound, near)), snapped: true };
+ if (near === null) return null;
+ return Math.max(loBound, Math.min(hiBound, near));
}
- const res = _tempoOnsetSnapPure(rawT, _ensureOnsets(), tol, loBound, hiBound);
- newT = res.t;
- if (res.snapped) dg.snappedT = newT;
+ const snappedT = _tempoOnsetSnapPure(rawT, _ensureOnsets(), tol, loBound, hiBound);
+ if (snappedT !== null) {
+ newT = snappedT;
+ dg.snappedT = snappedT;
+ }Also applies to: 2110-2122
🤖 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 2088 - 2092, Update _tempoOnsetSnapPure to return
null when no onset is within tolerance, or the clamped numeric snap time when
one is found, eliminating per-call result-object allocation. Adjust
_tempoMapOnDragMove to interpret that return value and derive dg.snappedT and
snapped state while preserving existing drag behavior.
| const src = fs.readFileSync(new URL('../src/tempo.js', import.meta.url), 'utf8'); | ||
| function body(header, end) { | ||
| const s = src.indexOf(header); | ||
| assert.ok(s >= 0, `"${header}" must exist`); | ||
| return src.slice(s, end ? src.indexOf(end, s) : s + 1400); | ||
| } | ||
| t('the drag move gates onset-snap on Snap = Onset and a non-locked pole', () => { | ||
| const b = body('export function _tempoMapOnDragMove', 'export function _tempoMapOnDragEnd'); | ||
| assert.match(b, /S\.snapMode === 'onset'/, 'only snaps in Onset mode'); | ||
| assert.match(b, /!\(orig\[d\] && orig\[d\]\.locked\)/, 'locked poles never snap'); | ||
| assert.match(b, /_tempoOnsetSnapPure\(rawT, _ensureOnsets\(\)/, 'uses the live onset cache'); | ||
| }); | ||
| t('the drag end reports a snap', () => { | ||
| const b = body('export function _tempoMapOnDragEnd', 'export function _makeTimeRemap'); | ||
| assert.match(b, /dg\.snappedT != null/, 'status only when a snap actually landed'); | ||
| assert.match(b, /snapped to a detected attack/, 'names the snap for the user'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the handler checks behavioral, not only textual.
Lines 71-80 only verify that source snippets contain certain regexes; they do not prove snapping is actually gated, locked poles bypass it, or drag-end status is conditional. A regression such as an unconditional status message could still pass. Also, body() silently uses slice(s, -1) when the end marker is missing.
Prefer a runtime integration test or pure gating/status helpers; at minimum, assert the end marker exists before slicing.
Suggested hardening
function body(header, end) {
const s = src.indexOf(header);
assert.ok(s >= 0, `"${header}" must exist`);
- return src.slice(s, end ? src.indexOf(end, s) : s + 1400);
+ const e = end ? src.indexOf(end, s) : s + 1400;
+ assert.ok(e > s, `"${end}" must exist after "${header}"`);
+ return src.slice(s, e);
}📝 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.
| const src = fs.readFileSync(new URL('../src/tempo.js', import.meta.url), 'utf8'); | |
| function body(header, end) { | |
| const s = src.indexOf(header); | |
| assert.ok(s >= 0, `"${header}" must exist`); | |
| return src.slice(s, end ? src.indexOf(end, s) : s + 1400); | |
| } | |
| t('the drag move gates onset-snap on Snap = Onset and a non-locked pole', () => { | |
| const b = body('export function _tempoMapOnDragMove', 'export function _tempoMapOnDragEnd'); | |
| assert.match(b, /S\.snapMode === 'onset'/, 'only snaps in Onset mode'); | |
| assert.match(b, /!\(orig\[d\] && orig\[d\]\.locked\)/, 'locked poles never snap'); | |
| assert.match(b, /_tempoOnsetSnapPure\(rawT, _ensureOnsets\(\)/, 'uses the live onset cache'); | |
| }); | |
| t('the drag end reports a snap', () => { | |
| const b = body('export function _tempoMapOnDragEnd', 'export function _makeTimeRemap'); | |
| assert.match(b, /dg\.snappedT != null/, 'status only when a snap actually landed'); | |
| assert.match(b, /snapped to a detected attack/, 'names the snap for the user'); | |
| const src = fs.readFileSync(new URL('../src/tempo.js', import.meta.url), 'utf8'); | |
| function body(header, end) { | |
| const s = src.indexOf(header); | |
| assert.ok(s >= 0, `"${header}" must exist`); | |
| const e = end ? src.indexOf(end, s) : s + 1400; | |
| assert.ok(e > s, `"${end}" must exist after "${header}"`); | |
| return src.slice(s, e); | |
| } | |
| t('the drag move gates onset-snap on Snap = Onset and a non-locked pole', () => { | |
| const b = body('export function _tempoMapOnDragMove', 'export function _tempoMapOnDragEnd'); | |
| assert.match(b, /S\.snapMode === 'onset'/, 'only snaps in Onset mode'); | |
| assert.match(b, /!\(orig\[d\] && orig\[d\]\.locked\)/, 'locked poles never snap'); | |
| assert.match(b, /_tempoOnsetSnapPure\(rawT, _ensureOnsets\(\)/, 'uses the live onset cache'); | |
| }); | |
| t('the drag end reports a snap', () => { | |
| const b = body('export function _tempoMapOnDragEnd', 'export function _makeTimeRemap'); | |
| assert.match(b, /dg\.snappedT != null/, 'status only when a snap actually landed'); | |
| assert.match(b, /snapped to a detected attack/, 'names the snap for the user'); |
🤖 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_onset_snap.test.mjs` around lines 65 - 80, Replace the regex-only
tests for _tempoMapOnDragMove and _tempoMapOnDragEnd with behavioral coverage
using runtime integration or pure gating/status helpers, verifying onset-mode
gating, locked-pole bypass, and conditional snap status. Also update body() to
assert that the end marker is found before slicing, preventing a missing marker
from producing slice(s, -1).
# Conflicts: # CHANGELOG.md
_tempoMapOnDragMove snapped a dragged barline to the nearest onset using _ensureOnsets() (BUFFER time) against rawT (CHART time). When the recording is shifted (S.audioShift != 0) the two diverge, so the barline snapped to the un-shifted attack. Use _ensureOnsetsShifted() — the chart-time onsets Suggest-fit already uses. Pre-existing since #235; low impact (only with a shifted audio), undoable, but a wrong snap target. Also drops the now-unused _ensureOnsets import. tempo_onset_snap.test.mjs's source-text assertion updated to _ensureOnsetsShifted. 146 JS green, lint 0-err. Fixes #254. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
…255) _tempoMapOnDragMove snapped a dragged barline to the nearest onset using _ensureOnsets() (BUFFER time) against rawT (CHART time). When the recording is shifted (S.audioShift != 0) the two diverge, so the barline snapped to the un-shifted attack. Use _ensureOnsetsShifted() — the chart-time onsets Suggest-fit already uses. Pre-existing since #235; low impact (only with a shifted audio), undoable, but a wrong snap target. Also drops the now-unused _ensureOnsets import. tempo_onset_snap.test.mjs's source-text assertion updated to _ensureOnsetsShifted. 146 JS green, lint 0-err. Fixes #254. Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
The manual complement to Suggest-fit (G): in Tempo Map mode with Snap = Onset, dragging a barline gently pulls to the nearest detected audio attack — so downbeats land on real hits instead of by eye. (Christian's request off the tempo charrette.)
Behaviour
_nearestOnsetTimePure/_ensureOnsetsonset infrastructure (the same detection the note-placement onset-snap and Suggest already use).Implementation
One gated branch in
_tempoMapOnDragMoveover two new pures:_tempoOnsetSnapTolPure(pixel window → seconds, capped) and_tempoOnsetSnapPure(snap within tol, re-clamp into bounds, else pass through). Gated onS.snapMode === 'onset'and a non-locked pole.Tests / gates
tests/tempo_onset_snap.test.mjs(9): pixel-window + seconds-cap + NaN-safety, snap-within-tol, bound re-clamp, no-onset / no-window no-ops, and drag-handler source guards for the Snap=Onset + non-locked gate. 126 JS suites green, lint 0 errors (3 pre-existing ratchet warnings).routes.pyuntouched.Verified live
On AC/DC — Back In Black: with Snap = Onset, dragging a barline snapped it to a detected attack ("Barline snapped to a detected attack at 6.96s"); with Snap = Grid the same drag produced no snap. No errors from the change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests