feat(editor): Song Fit ▸ Re-sync from this bar on — the drift rescue front door - #267
Conversation
|
Warning Review limit reached
Next review available in: 27 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)
📝 WalkthroughWalkthroughSong Fit adds a “Re-sync from this bar on…” option. It computes an anchor from the playhead, enables Tempo Map mode, selects the anchor, clears multi-selection, and triggers tempo suggestions. Tests cover choice and anchor behavior, and the changelog documents the feature. ChangesSong Fit re-sync
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as Song Fit editor
participant SongFit as _songFitResync
participant TempoMap as Tempo Map
Editor->>SongFit: select resync
SongFit->>SongFit: compute anchor from beats and cursor time
SongFit->>TempoMap: enable mode and set tempo selection
SongFit->>TempoMap: trigger tempoSuggestFit
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 (1)
tests/song_fit.test.mjs (1)
45-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the side-effectful resync workflow.
These tests cover the pure helpers, but would not catch regressions in Tempo Map entry,
S.tempoSel,tempoSelMulti.clear(), ortempoSuggestFitdispatch. Add a focused test with mocked editor state and registry commands.🤖 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/song_fit.test.mjs` around lines 45 - 68, Add a focused regression test alongside the existing resync-anchor tests that exercises the side-effectful re-sync workflow with mocked editor state and registry commands. Verify Tempo Map entry, updating S.tempoSel, clearing tempoSelMulti, and dispatching tempoSuggestFit; keep the existing pure-helper tests unchanged.
🤖 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 `@tests/song_fit.test.mjs`:
- Around line 45-68: Add a focused regression test alongside the existing
resync-anchor tests that exercises the side-effectful re-sync workflow with
mocked editor state and registry commands. Verify Tempo Map entry, updating
S.tempoSel, clearing tempoSelMulti, and dispatching tempoSuggestFit; keep the
existing pure-helper tests unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3e0eeba-4462-4e76-b913-e63ee0ce9fec
📒 Files selected for processing (3)
CHANGELOG.mdsrc/song-fit.jstests/song_fit.test.mjs
…mits-nothing Addresses CodeRabbit's nitpick on #267 (the pure-helper tests would not catch a regression in Tempo Map entry, S.tempoSel, tempoSelMulti.clear(), or the tempoSuggestFit dispatch) — but via this module's OWN convention for chrome (the section-3 source guards) rather than the mocked-state test it proposed. _songFitResync is a 6-line private dispatcher; standing up window + S + a fake registry to drive it would be a heavier and more brittle test than the thing under test, and every sibling verb here (editorSyncTempo, editorNudgeOffset, the session revalidation) is already pinned exactly this way. The guard catches the four regressions CodeRabbit named, plus two it did not: - ORDER: entering Tempo Map CLEARS tempoSel, so the anchor must be taken AFTER entry and the fit must read it after it is set. Hoisting the anchor above the toggle would silently fit from a cleared selection — the source comment warns about this, nothing enforced it. - COMMITS NOTHING: re-sync is proposal-only (it shows ghosts; tempo.js owns the undoable TempoMapCmd on accept). Asserting the body never touches history.exec / TempoMapCmd / S.beats pins the undo-safety property that makes this the safe front door to the highest-blast-radius op in the editor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Partially accepted — the gap is real, the proposed remedy isn't the one this module uses.
It also pins two properties you didn't flag, which are the ones that actually matter here:
Separately, |
…mits-nothing Addresses CodeRabbit's nitpick on #267 (the pure-helper tests would not catch a regression in Tempo Map entry, S.tempoSel, tempoSelMulti.clear(), or the tempoSuggestFit dispatch) — but via this module's OWN convention for chrome (the section-3 source guards) rather than the mocked-state test it proposed. _songFitResync is a 6-line private dispatcher; standing up window + S + a fake registry to drive it would be a heavier and more brittle test than the thing under test, and every sibling verb here (editorSyncTempo, editorNudgeOffset, the session revalidation) is already pinned exactly this way. The guard catches the four regressions CodeRabbit named, plus two it did not: - ORDER: entering Tempo Map CLEARS tempoSel, so the anchor must be taken AFTER entry and the fit must read it after it is set. Hoisting the anchor above the toggle would silently fit from a cleared selection — the source comment warns about this, nothing enforced it. - COMMITS NOTHING: re-sync is proposal-only (it shows ghosts; tempo.js owns the undoable TempoMapCmd on accept). Asserting the body never touches history.exec / TempoMapCmd / S.beats pins the undo-safety property that makes this the safe front door to the highest-blast-radius op in the editor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e2ed7d6 to
f974314
Compare
Fourth Song Fit choice: anchors the assisted barline fit on the last downbeat at/before the playhead (_songFitResyncAnchorPure), enters Tempo Map, and runs the fit immediately so the ghost corrections show without another keypress. Pure chrome — mode entry and the fit both dispatch through registry commands (editorRunShortcutCommand). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Two properties of "Re-sync from this bar on" carried the whole feature and
neither was pinned.
1. THE BOUNDARY. "From this bar ON" is a promise about the bars BEFORE it: the
drift rescue is reached for precisely when the chart is already right up to
bar N, so eating any of that authored grid is the one unforgivable failure,
and an off-by-one at the boundary bar is the bug this class of feature ships.
The invariant does hold — structurally, since _suggestFitPure marches only
downbeats >= fromIdx and _suggestApplyPure re-spaces only spans with a moved
edge — but structure regresses silently.
Composed over the same engine pures the accept path runs (_suggestFitPure →
_suggestApplyPure) on a grid whose recording drifts 2% from bar 5. The
fixture puts the recording a uniform 40ms behind the authored grid so the
early bars sit where the USER authored them, not where a naive onset fit
would drag them — that lag is what gives the test teeth: without it the
pre-anchor region is already grid-true and the assertion passes VACUOUSLY
even with a one-bar-early anchor. Mutation-checked both ways. Pins both
sides: beats[0..anchor] byte-identical, and the re-fit begins at the very
next beat (no off-by-one dead bar).
2. THE DISPATCH (CodeRabbit's nitpick, via this module's own convention). It
asked for a mocked-state test; _songFitResync is a 6-line private dispatcher
and this suite's section 3 already pins every sibling verb with source
guards, so the guard follows that pattern. It covers the four things
CodeRabbit named (registry mode entry, the anchor, S.tempoSel, the
tempoSuggestFit dispatch) plus two it missed:
- ORDER: entering Tempo Map CLEARS tempoSel, so the anchor must be taken
AFTER entry and the fit must read it after it is set. Hoisting the anchor
above the toggle would silently fit from a cleared selection. The source
comment warned about this; nothing enforced it.
- COMMITS NOTHING: re-sync is proposal-only (it shows ghosts; tempo.js owns
the undoable TempoMapCmd on accept). Asserting the body never touches
history.exec / TempoMapCmd / S.beats pins the undo-safety property that
makes this a safe front door to the highest-blast-radius op in the editor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f974314 to
7a5525b
Compare
2 participants
What
The workshop item from the tester drift thread, built to Christian's call ("show fixes immediately"): the classic workflow trap is a constant tempo set from a tab while the band really plays a hair slower — the chart is right up to bar N and increasingly wrong after, and the cure (anchor the assisted fit at the drift point) was undiscoverable from Song Fit's three song-scope verbs.
_songFitResyncAnchorPure— before bar 1 anchors on bar 1; no downbeats refuses), enters Tempo Map, drops any barline multi-selection (a live multi outranks the anchor in the fit), and runs the assisted fit immediately — the suggested corrections appear as ghost markers with the standard click-to-accept-through flow. Nothing commits until accepted; Esc dismisses.editorRunShortcutCommand— the same registry commands the keyboard uses. No new engine, no new imports.Tests
tests/song_fit.test.mjs: the choices pin updated to four; new anchor cases (inside a bar, exactly on a downbeat, past the end, before bar 1, interiors-only refusal). 9/9; full suite green; lint 0 errors;routes.pyuntouched.Live-verified
Real project, playhead parked at 150s in its drift zone: Song Fit → Re-sync → Tempo Map opens anchored on the bar at 148.23s with "Suggested 72 barlines ahead of the anchor — click a ghost handle to accept through it" showing immediately. Zero page errors.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit