feat(editor): snap-to-onset — bridge musical time to audio-attack time (D1) - #144
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (4)
📝 WalkthroughWalkthroughAdds a "Snap to audio onset" editor preference that toggles snapping between tempo-grid subdivisions and nearest detected audio transients, wired through a new UI button, command entry, persisted localStorage preference, updated ChangesSnap to Audio Onset Feature
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 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 (3)
screen.js (1)
7058-7092: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency: unguarded
_ensureOnsets()call vs. guarded usage insnapTime.Line 7076 calls
_ensureOnsets()directly, whilesnapTime(line 1184) guards the same call withtypeof _ensureOnsets === 'function'. If the guard insnapTimeis defensively necessary, the same defensiveness should apply here for consistency; if it isn't necessary, the guard insnapTimecould be dropped instead.Optional consistency fix
if (S.snapMode === 'onset') { - const onsets = _ensureOnsets(); + const onsets = (typeof _ensureOnsets === 'function') ? _ensureOnsets() : null; setStatus(onsets && onsets.length🤖 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` around lines 7058 - 7092, The snap-mode toggle in _editorToggleSnapMode() calls _ensureOnsets() directly, while snapTime() uses a typeof guard for the same helper. Make the two paths consistent by either guarding the _ensureOnsets() call here as well or removing the unnecessary guard in snapTime(), and keep the fallback status messaging in sync with the chosen approach.tests/onset_snap.test.js (2)
55-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo test for
_ensureOnsetsbeing unavailable.The mock always supplies
_ensureOnsetsas a function returningonsets; the realtypeof _ensureOnsets === 'function'guard branch (falling back tonull) insnapTimeis never exercised.🤖 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/onset_snap.test.js` around lines 55 - 65, Add a test in onset_snap.test.js that exercises the snapTime path where _ensureOnsets is not available, since the current setup only mocks _ensureOnsets as a function. Update the snapTime test cases around the existing _nearestOnsetTimePure/onsets setup to pass an undefined or non-function _ensureOnsets and assert the fallback null behavior is used. Keep the existing snapTime helper invocation, but vary the _ensureOnsets dependency so the typeof _ensureOnsets === 'function' guard is covered.
96-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIncorrect math in comment.
Math.round(2.5)returns3in JavaScript (rounds ties toward +∞), not2as the comment states —gridPointis actually1.5, not1.0. The assertion still passes either way since1.234 !== gridPointregardless, but the inline reasoning is misleading for future maintainers.📝 Fix the comment
const onset = _nearestOnsetTimePure([{ t: 1.234 }], 1.25, 0.07); - const gridPoint = Math.round(1.25 * 2) / 2; // 1.5? no: round(2.5)=2 -> 1.0... nearest 0.5 + const gridPoint = Math.round(1.25 * 2) / 2; // round(2.5)=3 -> 1.5 (nearest 0.5)🤖 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/onset_snap.test.js` around lines 96 - 103, The inline note in the _nearestOnsetTimePure test has incorrect rounding math, so update the comment to match JavaScript’s Math.round behavior and the actual gridPoint value. Keep the test logic unchanged, but revise the explanatory text near _nearestOnsetTimePure so it accurately states that Math.round(2.5) yields 3 and the nearest 0.5-second grid point is 1.5.
🤖 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`:
- Around line 7058-7092: The snap-mode toggle in _editorToggleSnapMode() calls
_ensureOnsets() directly, while snapTime() uses a typeof guard for the same
helper. Make the two paths consistent by either guarding the _ensureOnsets()
call here as well or removing the unnecessary guard in snapTime(), and keep the
fallback status messaging in sync with the chosen approach.
In `@tests/onset_snap.test.js`:
- Around line 55-65: Add a test in onset_snap.test.js that exercises the
snapTime path where _ensureOnsets is not available, since the current setup only
mocks _ensureOnsets as a function. Update the snapTime test cases around the
existing _nearestOnsetTimePure/onsets setup to pass an undefined or non-function
_ensureOnsets and assert the fallback null behavior is used. Keep the existing
snapTime helper invocation, but vary the _ensureOnsets dependency so the typeof
_ensureOnsets === 'function' guard is covered.
- Around line 96-103: The inline note in the _nearestOnsetTimePure test has
incorrect rounding math, so update the comment to match JavaScript’s Math.round
behavior and the actual gridPoint value. Keep the test logic unchanged, but
revise the explanatory text near _nearestOnsetTimePure so it accurately states
that Math.round(2.5) yields 3 and the nearest 0.5-second grid point is 1.5.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 817a2abe-480f-4e40-b241-b4cbfb6ff3d4
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/onset_snap.test.js
…e (D1) Add a second snap target alongside the subdivision grid: a Grid / Onset toggle (and the `toggleSnapMode` command) switches note placement between the tempo-map subdivisions and the nearest detected transient in the recording. Grid time and the actual attack routinely differ by tens of milliseconds, so this is the bridge between musical time and audio-attack time for by-ear transcription. - `snapTime()` gains an onset branch: when snap is on and `S.snapMode` is 'onset', prefer the nearest onset within ~70 ms (`ONSET_SNAP_TOL`), else fall back to grid snap. Because every add/drag routes through `snapTime`, the mode applies to all note, drum, anchor and section placement at once. - `@pure:onset-snap` → `_nearestOnsetTimePure(onsets, t, tol)`: binary- searched nearest transient, or null so the caller falls back to grid. - Reuses the existing onset detector that already draws the display-only onset strip — no warp, no new DSP. - Grid/Onset toolbar button + registry command; `S.snapMode` is an editor pref persisted to localStorage, never written to the pack. Tests: tests/onset_snap.test.js (the pure nearest-onset helper + the grid-vs-onset snapTime routing; both fail on main). Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
7644a3a to
031430f
Compare
2 participants
What
Adds a second snap target alongside the subdivision grid. A Grid / Onset toggle by the Snap controls (and the
toggleSnapModecommand) switches note placement between the tempo-map subdivisions and the nearest detected transient in the recording.Grid time and the actual audio attack routinely differ by tens of milliseconds, so this is the bridge between musical time and audio-attack time for by-ear transcription (Phase D1 of the workspace-shell design). No warp — just snap placement to the onset time.
How
snapTime()onset branch — when snap is on andS.snapMode === 'onset', prefer the nearest onset within a ~70 ms window (ONSET_SNAP_TOL); otherwise fall back to grid snap. Since every add/drag routes throughsnapTime, the mode applies to all note, drum, anchor and section placement at once — one chokepoint, no per-call sites.@pure:onset-snap→_nearestOnsetTimePure(onsets, t, tol)— binary-searched nearest transient over the sorted onsets, ornullso the caller falls back to grid. Guards non-finitet, empty onsets, andtol ≤ 0._ensureOnsets) — no new DSP, no warp.toggleSnapModeregistry command (no keybind —Shift+G/Alt+Gare already claimed by the planned grid-display / custom-snap commands).S.snapModeis an editor pref persisted tolocalStorage, never written to the pack.Degrade behaviour
Onset mode falls back to grid snap when no attack is near, when no onsets are computed (no recording), and the master snap-off checkbox gates onset mode too — so placement stays sensible in every state.
Tests
tests/onset_snap.test.js— 13 cases:@pure:onset-snapnearest-onset helper (within/outside tolerance, nearest-of-many, before-first/after-last, degrade-to-null, adversarial inputs);snapTimerouting (grid vs onset, near-transient wins, no-onset/none-computed/snap-off → grid), via the extracted function with injected deps.Both seams fail on main (the
@pureblock and thesnapModebranch don't exist there).Verification
node --check screen.jscleanroutes.pychange → no pytestorigin/main(b367c23)Notes
Onset-snap operates in the seconds domain (snaps to the onset time), so it has zero dependency on the Phase A2
note.beatmodel (#135) and rebases clean regardless of Phase-A merge order. It's placement, not the time model.Summary by CodeRabbit
New Features
Tests