feat(editor): beat-lock — sync points immune to tempo re-fits (Phase A5) - #138
Merged
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 (3)
📝 WalkthroughWalkthroughAdds Beat-lock for tempo-map sync points: locked anchors survive tempo re-fits, persist per file in editor preferences, update rendering and commands, and are covered by a dedicated test suite. ChangesBeat-lock implementation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TempoContextUI
participant Editor as _editorToggleSyncLock
participant Storage as _saveBeatLocks/_restoreBeatLocks
participant Respace as _respaceWithLocksPure
participant Renderer as Tempo Map renderer
User->>TempoContextUI: click togglelock
TempoContextUI->>Editor: _editorToggleSyncLock()
Editor->>Storage: save locked sync-point positions
Storage-->>Editor: persisted editor preference
User->>Editor: edit BPM or modulate tempo
Editor->>Respace: _respaceWithLocksPure(oldBeats, newBeats)
Respace-->>Editor: lock-adjusted beat grid
Editor->>Editor: reproject note and section timing
Editor->>Renderer: redraw locked poles with distinct colors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)screen.jsast-grep timed out on this file Comment |
byrongamatos
force-pushed
the
feat/editor-beat-lock
branch
from
July 9, 2026 06:28
d4426e6 to
60676d4
Compare
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Around line 25-26: The changelog entry misstates the persistence scope by
saying the lock is keyed by song; update the wording in the editor-pref release
note to match the actual contract used in tests and implementation, which
stores/restores it per filename and never writes it to the pack. Keep the note
aligned with the beat_lock behavior referenced by tests/beat_lock.test.js so the
release wording matches what the code actually does.
In `@screen.js`:
- Around line 10096-10126: The locked-grid handling in the beat rescale flow
should also reproject section start times, not just note times, so sections stay
aligned with the warped beat map around locked anchors. Update the same timing
path in screen.js where `locked` is computed from `_respaceWithLocksPure` to
apply the `beatOf(oldBeats, ...)` then `timeOf(respaced, ...)` mapping to
section markers (the `start_time` values), and keep the linear scale path only
for the unlocked case.
🪄 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: 3ed32dd9-75a2-4a73-ab3a-f6335f11f5a8
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/beat_lock.test.js
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/beat_lock.test.js (1)
172-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRegex-extracted eval of the section-scaling loop is fragile but self-guarding.
Extracting
// Scale section times ... for (const s of S.sections) {...}fromscreen.jsvia regex and running it throughnew Function(...)(Line 177-179) ties this test to the exact comment text and loop shape inscreen.js. Any refactor there (renamed comment, restructured loop) breaks extraction — mitigated somewhat byassert.ok(secLoop, ...)at Line 176, which fails loudly instead of silently passing a stale/empty block.Longer-term, exporting the section-scaling logic as a small named pure function in
screen.js(similar to_respaceWithLocksPure) would let this test import it directly instead of scraping source text, improving robustness against comment/formatting drift.🤖 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/beat_lock.test.js` around lines 172 - 194, The test is fragile because it scrapes the section-scaling loop from screen.js via a regex and executes it with new Function, so any comment or loop-shape refactor can break it. Move the section-time scaling logic out of the inline “Scale section times” block in screen.js into a small named pure function (similar to _respaceWithLocksPure), then have editorApplySync/section-scaling call that helper and update this test to import and invoke it directly instead of extracting source text.
🤖 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/beat_lock.test.js`:
- Around line 172-194: The test is fragile because it scrapes the
section-scaling loop from screen.js via a regex and executes it with new
Function, so any comment or loop-shape refactor can break it. Move the
section-time scaling logic out of the inline “Scale section times” block in
screen.js into a small named pure function (similar to _respaceWithLocksPure),
then have editorApplySync/section-scaling call that helper and update this test
to import and invoke it directly instead of extracting source text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31c6d214-caf2-4dee-8f08-45d5462bf194
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/beat_lock.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
…e-fits (Phase A5)
Lock a Tempo-Map sync point (press S, or right-click the pole → Lock) and its
time is held through GLOBAL tempo re-fits — detect / sync-to-audio, metric
modulation, and measure-BPM re-space. `_respaceWithLocksPure` holds every locked
anchor at its old time and affine-interpolates the runs around it: the song ends
still carry the tempo change (a global BPM edit still lengthens/shortens the
song) and no beat is ever pushed before the start. Locked poles render emerald
on the beat bar. This is the guard (charrette §1.8/D-T5) that makes the
beat-primary model safe for placing notes by ear before the grid is trusted — a
nailed passage can't be walked off by a later global tempo op.
- Locks are EDITOR-PREF, keyed by filename, restored on load, NEVER written to
the pack (D15): persist the locked seconds, re-attach by time-match on load.
- Reuses the pre-wired (previously 'planned') tempoToggleSyncLock command (key
S) + a right-click "Lock/unlock sync point" affordance. The `locked` flag
rides through every tempo command via the existing {...b} clones, so it
survives undo/redo for free.
Requires A2 (beat-primary) — stacked on feat/editor-beat-primary; rebase onto
main when A2 merges. Tests: tests/beat_lock.test.js (11, would-fail-on-main).
Full JS suite 72/0. No routes.py / no wire change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013JgxKh99UAeQqmhzSc73tv
…tes (review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ding (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/editor-beat-lock
branch
from
July 9, 2026 07:45
4221a17 to
98315cb
Compare
This was referenced Jul 9, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase A5 — beat-lock
Final Phase A slice (§1.8 / D-T5). Stacked on A2 (#135) — base is
feat/editor-beat-primary; retargets tomainwhen A2 merges. Sibling to A4 (#137).The problem it guards
Under pure beat-primary, notes placed by ear on transients before the grid is trusted move with the grid on a later big re-fit (the transcription "Scenario B"). Beat-lock is the promised guard: a hand-verified sync point can be pinned so a global tempo op can't walk it off.
What it does
Lock a Tempo-Map sync point — press
S, or right-click the pole → Lock sync point — and its time is held through the three global re-fit ops:editorApplySync)_editorModulateTempoAtSelection)editorSetBPM/_tempoSetMeasureBpm)Each routes its candidate grid through
_respaceWithLocksPure(oldBeats, newBeats), which treats the two song ends (carrying the re-fit) plus every locked anchor (pinned to its old time) as fixed points, and affine-remaps each run between them — so:Direct pole drag is unaffected (dragging is deliberate hand-placement); the lock only guards indirect/global re-fits.
Distinct render
Locked poles render emerald on the beat bar (the selection halo still shows through, so lock ≠ selection). The drag handle stays — a locked point can still be dragged directly.
Persistence (editor-pref, never the pack — D15)
Locks are stored in
localStoragekeyed by filename (editorBeatLocks:<file>) as the locked seconds (which survive save/reload — the pack rebuilds the grid to the same times), and re-attached to the matching beats by time on load. Nothing is written to the pack / wire. In-session thelockedflag rides on the beat objects, so it survives every tempo command + undo/redo for free via the existing{...b}clones.Command wiring
Reuses the pre-existing
tempoToggleSyncLockregistry slot (wasstatus: 'planned', keyS, dispatch stubbed) — flipped toready, handler implemented, plus the right-click affordance.Tests
tests/beat_lock.test.js— 11, all would-fail-on-main:_respaceWithLocksPure— no-locks identity; the §1.8 acceptance (locked anchor holds while neighbours move); ends still carry the re-fit; big-stretch + early lock never goes negative; two-lock interior interpolation; length-mismatch no-op; thelockedflag rides onto the output._beatLockParsePuredefensive (junk / non-array / bad values drop out);_applyBeatLocksPurere-attaches by nearest-within-tolerance and clears the rest.Verification
node --check screen.jscleanroutes.pychange (no pytest); no wire/spec changeOrdering
Requires A2 (#135). With this, Phase A of the workspace-shell time-model foundation is complete (A1 #133 merged · A2 #135 · A3 #136 · A4 #137 · A5 this). Phase B (shell chrome) waits on the feature-PR backlog draining. Design source:
SONG-EDITOR-WORKSPACE-SHELL-CHARRETTE.md§1.8 / D-T5.🤖 Generated with Claude Code
Summary by CodeRabbit