fix(editor): route Sync / BPM-rescale / Offset through undoable total-reproject commands

[ChrisBeWithYou]

The P0

Three whole-song tempo operations — Sync tempo, the BPM box's constant-tempo rescale, and the audio Offset nudge — mutated the timeline directly, with no undo, and moved only the current arrangement's plain notes() plus the global beat grid and sections. Everything else was left behind:

• every other arrangement (a bass/keys part next to the guitar)
• all chords, anchors, handshapes, phrases
• the drum tab (on the Sync and rescale paths)

So a multi-part song silently drifted out of phase and the damage couldn't be undone. This is the charrette's opening P0 (TEMPO-MAPPING-CHARRETTE.md).

The fix

All three now route through one command each — TempoMapCmd ('sync' / 'rescale') and the new TempoOffsetCmd ('offset') — whose beat-primary lift→reproject is total: _eachTimed walks every timed object in every part, so a grid change is structurally incapable of stranding a note. Fully undoable, exact-seconds rollback.

• Pivot at the first downbeat (or the focused barline) instead of t=0: t' = t0 + (t − t0)·scale. A song with a pickup / lead-in no longer skews under a bare time *= factor. New pure _tempoPivotTimePure.
• S.appliedOffset replaces the Offset input's hidden dataset.applied, so undo restores the applied shift (the next nudge's delta was computing off a stale base). _effectiveAudioOffset() reads it; TempoOffsetCmd owns it undoably; keeps the drum-hit ≥0 clamp.
• Removed the Sync dialog's duplicate offset field — the toolbar Offset is the single, undoable path (design doc demotes it).
• TempoMapCmd now carries songScope = true (matching TempoGridCmd) so these toolbar-fired ops aren't refused by the read-only-roll lock.

Tests

• tests/tempo_op_commands.test.mjs (new): deep-diffs a two-arrangement song with chords + drums, proving every part moves on a sync stretch and undo restores the exact pre-edit seconds; TempoOffsetCmd rigid-shift + past-grid extrapolation pin + drum ≥0 clamp + exec→undo→redo; _tempoPivotTimePure cases; and source guards that each wrapper routes through the commands (fail-on-main wiring proof).
• Removed obsolete apply_offset.test.js (sliced the deleted _shiftArrangementTimes); updated beat_lock.test.mjs's section case off the deleted inline loop.
• npm test 114 files green, npm run lint 0 errors (3 pre-existing warnings = main's ratchet). routes.py untouched.

First PR of the tempo-mapping track (TEMPO-MAPPING-PR-PROMPTS.md, PR 1). PRs 3/6/9 depend on this pivot + offset command.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Fixed whole-song tempo edits affecting multi-part songs, ensuring all timed elements move together and undo/redo restores timing correctly.
  • Tempo syncing/BPM changes now rescale around the first downbeat (or the focused barline) with consistent beat re-spacing.
  • Audio offset nudges are now fully undoable, including correct state restoration and no invalid negative drum-hit timings.
• Improvements
  • Updated the Sync Tempo dialog by removing the duplicate time offset field and adding a manual audio BPM override.
• Tests
  • Added regression coverage for tempo and offset operations across multi-arrangement content.

[coderabbitai]

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b513aa2f-8c1d-4782-8f55-5ba51b678191

📥 Commits

Reviewing files that changed from the base of the PR and between d194eb0 and 046f09a.

📒 Files selected for processing (8)

• CHANGELOG.md
• screen.html
• src/file-ops.js
• src/main.js
• src/state.js
• src/tempo.js
• tests/tempo_markers.test.mjs
• tests/tempo_op_commands.test.mjs

🚧 Files skipped from review as they are similar to previous changes (5)

• src/file-ops.js
• screen.html
• src/state.js
• CHANGELOG.md
• tests/tempo_op_commands.test.mjs

---

📝 Walkthrough

Walkthrough

Tempo sync, BPM rescaling, and audio offset edits now use undoable tempo commands with pivot-based beat reprojection. Applied offset state moved into editor state, the duplicate Sync offset field was replaced by a manual BPM override, and multi-part undo/redo coverage was added.

Changes

Tempo operation routing

Layer / File(s)	Summary
Tempo command primitives src/state.js, src/tempo.js	Adds pivot selection and TempoOffsetCmd, makes tempo commands song-scoped, tracks applied offset in state, and corrects phrase anchor handling.
Sync and BPM rescaling src/sync-tempo.js, src/main.js	Routes sync and constant-tempo BPM changes through pivoted TempoMapCmd execution with lock-aware respacing.
Audio offset state and UI src/main.js, src/file-ops.js, screen.html, src/sync-tempo.js	Routes offset changes through TempoOffsetCmd, resets command-owned offset state on load, and replaces the Sync offset input with a manual BPM override.
Cross-part validation and release notes tests/tempo_op_commands.test.mjs, tests/beat_lock.test.mjs, tests/beat_primary.test.mjs, tests/tempo_markers.test.mjs, CHANGELOG.md	Adds multi-part timing, undo/redo, routing, beat-lock, phrase-remapping, and meter-marker coverage, and documents the changes.

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

Sequence Diagram(s)

sequenceDiagram
  participant SyncDialog
  participant Editor
  participant History
  participant TempoMapCmd
  participant SongParts
  SyncDialog->>Editor: submit tempo factor
  Editor->>TempoMapCmd: create pivoted respaced grid
  Editor->>History: execute command
  History->>SongParts: reproject notes, phrases, drums, and sections
  History->>Editor: redraw and update status

Loading

Possibly related PRs

• got-feedBack/feedBack-plugin-editor#110: Both changes modify TempoMapCmd behavior for song-wide tempo operations.
• got-feedBack/feedBack-plugin-editor#138: Both changes address lock-aware tempo respacing and sync-point preservation.
• got-feedBack/feedBack-plugin-editor#185: Both changes modify Sync Tempo reprojection and editorApplySync control flow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 accurately summarizes the core change: routing Sync, BPM rescale, and Offset through undoable reproject commands.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/editor-tempo-op-commands

---

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

[coderabbitai]

Actionable comments posted: 1

🤖 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/main.js`:
- Around line 1413-1451: The editorApplyOffset function currently bypasses undo
history when S.beats.length is less than 2. Remove that special-case branch,
including its direct S.appliedOffset update and status handling, so every
meaningful offset change proceeds through TempoOffsetCmd and S.history.exec;
retain the existing no-op delta check and normal UI updates.

🪄 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: 25ca6c4b-f77a-4f9c-9586-ec952a770279

📥 Commits

Reviewing files that changed from the base of the PR and between 2a764a1 and dc3a51e.

📒 Files selected for processing (10)

• CHANGELOG.md
• screen.html
• src/file-ops.js
• src/main.js
• src/state.js
• src/sync-tempo.js
• src/tempo.js
• tests/apply_offset.test.js
• tests/beat_lock.test.mjs
• tests/tempo_op_commands.test.mjs

💤 Files with no reviewable changes (2)

• tests/apply_offset.test.js
• screen.html

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@tests/tempo_markers.test.mjs`:
- Around line 37-44: Update the beat fixture in the within-tolerance test around
_tempoMarkersPure so measure 2 has a subsequent downbeat, ensuring its BPM is
calculated from the constructed span rather than using the final-run fallback.
Preserve the 60.005 BPM duration and assert that only the baseline tempo marker
is emitted.

🪄 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: b513aa2f-8c1d-4782-8f55-5ba51b678191

📥 Commits

Reviewing files that changed from the base of the PR and between d194eb0 and 046f09a.

📒 Files selected for processing (8)

• CHANGELOG.md
• screen.html
• src/file-ops.js
• src/main.js
• src/state.js
• src/tempo.js
• tests/tempo_markers.test.mjs
• tests/tempo_op_commands.test.mjs

🚧 Files skipped from review as they are similar to previous changes (5)

• src/file-ops.js
• screen.html
• src/state.js
• CHANGELOG.md
• tests/tempo_op_commands.test.mjs

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🤖 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 `@tests/tempo_markers.test.mjs`:
- Around line 37-44: Update the beat fixture in the within-tolerance test around
_tempoMarkersPure so measure 2 has a subsequent downbeat, ensuring its BPM is
calculated from the constructed span rather than using the final-run fallback.
Preserve the 60.005 BPM duration and assert that only the baseline tempo marker
is emitted.

🪄 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: b513aa2f-8c1d-4782-8f55-5ba51b678191

📥 Commits

Reviewing files that changed from the base of the PR and between d194eb0 and 046f09a.

📒 Files selected for processing (8)

• CHANGELOG.md
• screen.html
• src/file-ops.js
• src/main.js
• src/state.js
• src/tempo.js
• tests/tempo_markers.test.mjs
• tests/tempo_op_commands.test.mjs

🚧 Files skipped from review as they are similar to previous changes (5)

• src/file-ops.js
• screen.html
• src/state.js
• CHANGELOG.md
• tests/tempo_op_commands.test.mjs

🛑 Comments failed to post (1)

tests/tempo_markers.test.mjs (1)

37-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test doesn't actually exercise the tolerance comparison it claims to.

db here is [0, 4] — measure 2 (index 4) is the last downbeat, so _tempoMarkersPure takes the nextI === null branch and sets bpm = runBpm directly, never computing the ratio from the carefully constructed span. The test would pass identically even if the > tol comparison were broken, since that branch is never reached for this beat set.

🧪 Proposed fix: close measure 2 so its BPM is actually computed

     const span = (4 * 60) / 60.005;   // m2 duration for 60.005 BPM
-    const beats = g([0, 1], [1, -1], [2, -1], [3, -1], [4, 2], [4 + span / 4, -1], [4 + span / 2, -1], [4 + 3 * span / 4, -1]);
+    const beats = g([0, 1], [1, -1], [2, -1], [3, -1], [4, 2], [4 + span / 4, -1], [4 + span / 2, -1], [4 + 3 * span / 4, -1], [4 + span, 3]);
     const mk = _tempoMarkersPure(beats, 0.01);
     assert.deepStrictEqual(mk.filter(m => m.kind === 'tempo').length, 1, 'only the baseline tempo');

📝 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.

t('a within-tolerance drift does NOT emit a new tempo marker (run behaviour)', () => {
    // m2 spans 4.001s → ~59.985 BPM, inside 0.01 of 60? No — but test the run:
    // make m2 exactly 60.005 BPM (within 0.01 tol → no marker).
    const span = (4 * 60) / 60.005;   // m2 duration for 60.005 BPM
    const beats = g([0, 1], [1, -1], [2, -1], [3, -1], [4, 2], [4 + span / 4, -1], [4 + span / 2, -1], [4 + 3 * span / 4, -1], [4 + span, 3]);
    const mk = _tempoMarkersPure(beats, 0.01);
    assert.deepStrictEqual(mk.filter(m => m.kind === 'tempo').length, 1, 'only the baseline tempo');
});

🤖 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_markers.test.mjs` around lines 37 - 44, Update the beat fixture
in the within-tolerance test around _tempoMarkersPure so measure 2 has a
subsequent downbeat, ensuring its BPM is calculated from the constructed span
rather than using the final-run fallback. Preserve the 60.005 BPM duration and
assert that only the baseline tempo marker is emitted.