Skip to content

feat(editor): beat-primary note model (Phase A2) - #135

Merged
byrongamatos merged 2 commits into
mainfrom
feat/editor-beat-primary
Jul 9, 2026
Merged

byrongamatos merged 2 commits into
mainfrom
feat/editor-beat-primary

Conversation

@ChrisBeWithYou

@ChrisBeWithYou ChrisBeWithYou commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Phase A2 — beat-primary note model

Second step of the workspace-shell time-model foundation (after A1's beatOf/timeOf converter, #133). Promotes a note's musical beat to runtime truth; seconds become a derived cache.

What changes

  • note.beat (float) is the truth; note.time (seconds) is a cache regenerated by timeOf(beat) whenever the grid changes.
  • Load-lift: on load, every timed object (notes, chords + chord notes, anchors/anchors_user, hand-shapes, phrases, sections, drum hits) gets a beat lifted from its seconds via _liftAllBeats — beat is the truth from load onward.
  • Total reproject: a tempo flex reprojects every part from its beat (_reprojectAll), replacing the old scoped remap. TempoGridCmd (re-indexing edits) re-lifts beats from the unchanged seconds.
  • Wire unchanged: the save body strips the beat cache (_stripBeatsFromSaveBody) so .sloppak/archive stay seconds-only — no feedpak-spec change.

Why (the corruption class this deletes)

A snapped note keeps its exact subdivision, an off-grid note its fractional beat, and both follow the beat on a flex — so a tempo edit can no longer silently corrupt note timing. This retires the entire tempoRideScope machinery: the drum/all/per-part "which notes ride the grid" picker, its Ctrl+T toggle, _applyTempoRemap, _captureScopedTimes/_restoreScopedTimes, and the ride-resolver pures. Under total reprojection there is no ride choice left to get wrong. Net screen.js is −402/+150 — it removes more than it adds.

Trust-critical testing

This is the trust-critical migration, so it's over-tested. Golden gate: a test embeds the pre-A2 _applyTempoRemap and proves lift+reproject reproduces it to 3 dp on a multi-part drifting-grid fixture — the gate that had to pass before any legacy remap/ride-snapshot code was deleted.

tests/beat_primary.test.js — 9 groups: golden gate · load→save identity on an unedited grid · flex keeps every beat while seconds reproject · snapped-keeps-subdivision vs off-grid-keeps-fraction · reproject is total (old ride-corruption impossible) · <2-beat grid degrades to seconds-primary · |time − timeOf(beat)| < 1 ms invariant · exact undo (TempoMapCmd exec→rollback restores times exactly) · save body never leaks beat/beatEnd to the wire. All new fns (_liftAllBeats/_reprojectAll/_eachTimed/_stripBeatsFromSaveBody) are absent on main, so the suite fails on main.

Also updated: eof_shortcuts.test.js (Ctrl+T now falls through to t → toggleTap since the ride toggle is gone), loop_undo_mode.test.js / midi_tempo_import.test.js (harness wiring to the new helpers), and removed tempo_ride_parts.test.js (tested the deleted ride machinery).

Verification

  • node --check screen.js clean
  • Full JS suite: 71 pass / 0 fail
  • No routes.py change (no pytest needed); wire format unchanged

Ordering

Requires A1 (#133, merged). Phase A is parallel-safe with the feature backlog. Phase B (the shell chrome) intentionally waits for the open feature PRs to merge — not part of this PR.

Design source: SONG-EDITOR-WORKSPACE-SHELL-CHARRETTE.md §1.3/§1.4/§1.10.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Tempo map now uses a beat-first (“beat-primary”) timing model, reprojecting all timed objects from their beats for consistent results across grid edits and tempo flexing.
    • Tempo map undo/rollback restores note times exactly (including off-grid placements), avoiding quantization drift.
    • Saved data remains seconds-based; timing beat caches are stripped automatically.
  • Bug Fixes

    • Improved reliability and correctness of tempo map undo/redo across grid edits.
  • Documentation

    • Updated the changelog to reflect the beat-primary model and exact tempo-flex undo behavior.
  • Tests

    • Added/updated coverage for beat-primary timing invariants and tempo map behaviors.

…d (Phase A2)

Promote a float note.beat to runtime truth; note.time (seconds) becomes a
cache regenerated by timeOf(beat) on every grid change. On load every timed
object (notes, chords + chord notes, anchors, hand-shapes, phrases, sections,
drum hits) lifts a beat from its seconds (_liftAllBeats); a tempo flex
reprojects every part from its beat (_reprojectAll); the save body strips the
beat cache so the wire stays seconds-only (_stripBeatsFromSaveBody) — no spec
change.

Snapped notes keep their exact subdivision, off-grid notes their fractional
beat; both follow the beat on a flex, so a tempo edit can no longer silently
corrupt note timing. Retires the whole tempoRideScope machinery (the
drum/all/per-part "which notes ride the grid" picker, its Ctrl+T toggle, and
the _applyTempoRemap/_captureScopedTimes remap walk): under total reprojection
there is no ride choice left to get wrong.

Golden-gated: a test proves lift+reproject reproduces the pre-A2 all-parts
_applyTempoRemap to 3 dp on a multi-part drifting-grid fixture before any
legacy code was deleted. Full JS suite green (71/0).

Tests: tests/beat_primary.test.js (9 groups incl. golden gate, exact undo,
load/save identity, 1 ms invariant, no beat leak to wire).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013JgxKh99UAeQqmhzSc73tv
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 039431ee-7419-4faf-8f38-2f6e800998b5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR replaces ride-scope-based tempo retiming with a beat-primary model, updates load/save and undo paths to lift and reproject beats, removes ride-scope UI and shortcut wiring, and adds tests plus changelog entries for the new behavior.

Changes

Beat-primary refactor

Layer / File(s) Summary
Beat-primary core model and TempoMapCmd rewrite
screen.js
Adds unified timed traversal, beat lifting/reprojection helpers, save-body beat stripping, and rewrites TempoMapCmd exec/rollback around beat caches and exact undo restoration.
Removal of tempoRideScope state, UI, and shortcuts
screen.js
Removes tempoRideScope and tempoRideCustom state, the ride-scope command and shortcut paths, the toggle helper, EOF handling, localStorage restore, checklist rebase, the ride-scope picker overlay, and the refresh call.
Load, reset, edit, and save wiring
screen.js
Calls beat lifting during load and reset flows, lifts beats again during tempo-grid edit handling and rollback, and strips beat caches from save serialization.
Changelog updates
CHANGELOG.md
Documents the beat-primary timing model and the exact tempo-flex undo behavior in the changelog.
New beat_primary test suite
tests/beat_primary.test.js
Adds a sandboxed test harness, a legacy remap reference, song and tempo fixtures, and assertions covering lift/reproject equivalence, identity, invariants, exact undo, save stripping, and beat bookkeeping.
Existing test updates for ride-scope removal and _liftAllBeats dependency
tests/eof_shortcuts.test.js, tests/loop_undo_mode.test.js, tests/midi_tempo_import.test.js, tests/tempo_ride_parts.test.js
Updates shortcut expectations for the removed ride-scope toggle, reshapes TempoMapCmd stubs for loop undo mode, adds _liftAllBeats to TempoGridCmd extraction, and removes the obsolete ride-parts test file.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Loader
  participant ScreenJS
  participant TempoMapCmd

  Loader->>ScreenJS: load song seconds
  ScreenJS->>ScreenJS: _liftAllBeats(S.beats)
  TempoMapCmd->>ScreenJS: exec() tempo grid edit
  ScreenJS->>ScreenJS: _reprojectAll(beats -> seconds)
  TempoMapCmd->>ScreenJS: rollback()
  ScreenJS->>ScreenJS: restore exact seconds snapshot
  ScreenJS->>ScreenJS: _stripBeatsFromSaveBody(body) on save
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.83% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: introducing the beat-primary editor time model in Phase A2.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/editor-beat-primary

Comment @coderabbitai help to get the list of available commands.

TempoMapCmd.rollback reprojected note seconds through _r3 on undo, silently
quantizing sub-millisecond placement (a note at 1.23456 came back 1.235), so
edit->undo->save diverged from the original save. exec now snapshots the exact
pre-edit seconds of every timed object and rollback restores them verbatim;
beat indexing is untouched (a flex preserves it). Redo re-snapshots on each
exec. Adds a sub-ms regression test and a _reprojectAll invariant guard comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@byrongamatos

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants