feat(editor): make the ruler authoritative for tempo mapping - #211
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR documents an authoritative musical ruler, changes tempo-map insertion to append barlines, preserves tempo selection through undo, marks unmapped ruler tails, and updates tempo-map terminology from “sync point” to “barline” across controls, guidance, and tests. ChangesTempo mapping and barline editing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ChrisBeWithYou
force-pushed
the
feat/editor-tempo-ruler-truth
branch
from
July 11, 2026 19:29
b236206 to
a961c89
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tempo.js (2)
286-296: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the user-facing “barline” rename.
The inspector still exposes “sync point” in
deleteTitle/hintand the final-measure delete message; the context-menu lock action and lock confirmation also retain that wording. This leaves users with mixed terminology in the same workflow.Also applies to: 318-323, 701-705, 1428-1440
🤖 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 `@src/tempo.js` around lines 286 - 296, Complete the terminology update in the inspector state around deleteTitle and hint, replacing user-facing “sync point” wording with “barline” consistently. Also update the final-measure delete message and the context-menu lock action and lock confirmation near the referenced code so this workflow no longer exposes “sync point” to users, without changing behavior.
829-835: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass selection through the in-range topology command.
When an interior beat is promoted,
S.tempoSelchanges after aTempoGridCmdwith no selection snapshots. Undo and redo therefore do not restore the pre-edit and post-edit selections, unlike the append path, and subsequent BPM/delete actions can target the wrong barline.Proposed fix
- S.history.exec(new TempoGridCmd(oldBeats, newBeats, 'insert')); - S.tempoSel = bestS; + S.history.exec(new TempoGridCmd( + oldBeats, newBeats, 'insert', S.tempoSel, bestS));🤖 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 `@src/tempo.js` around lines 829 - 835, Update the in-range beat-promotion flow around _tempoRenumberMeasures and TempoGridCmd to capture and pass the pre-edit and post-edit tempo selections in the command, matching the append path. Ensure undo restores the previous S.tempoSel and redo restores bestS, while retaining the existing beat updates and selection assignment.
🤖 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 `@README.md`:
- Around line 25-28: Update the Time description’s “sync points” wording in
README.md to use the public UI terminology “barlines” or “Mark barline,” while
preserving the surrounding timing-domain explanation.
In `@tests/tempo_sync_inspector.test.mjs`:
- Line 26: Update the strings returned by _tempoSyncInspectorStatePure in
src/tempo.js from “sync point” to “barline” for the no-selection delete
title/hint and the first/final non-deletable title. Extend tests in
tempo_sync_inspector.test.mjs to cover each updated state and assert the renamed
copy.
---
Outside diff comments:
In `@src/tempo.js`:
- Around line 286-296: Complete the terminology update in the inspector state
around deleteTitle and hint, replacing user-facing “sync point” wording with
“barline” consistently. Also update the final-measure delete message and the
context-menu lock action and lock confirmation near the referenced code so this
workflow no longer exposes “sync point” to users, without changing behavior.
- Around line 829-835: Update the in-range beat-promotion flow around
_tempoRenumberMeasures and TempoGridCmd to capture and pass the pre-edit and
post-edit tempo selections in the command, matching the append path. Ensure undo
restores the previous S.tempoSel and redo restores bestS, while retaining the
existing beat updates and selection assignment.
🪄 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: f01160b8-21dc-4ba0-8804-9832b5d52d58
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mddocs/TEMPO-MAPPING-DESIGN.mdsrc/input.jssrc/menu-bar.jssrc/ruler.jssrc/shortcuts.jssrc/tempo.jstests/menu_model.test.mjstests/ruler.test.mjstests/tempo_barline_append.test.mjstests/tempo_map_guidance.test.mjstests/tempo_sync_inspector.test.mjs
Comment on lines
+25
to
+28
| The authoritative musical ruler fits bars and beats to fixed source audio; | ||
| authored musical content aligns through that ruler. See | ||
| [`docs/TEMPO-MAPPING-DESIGN.md`](docs/TEMPO-MAPPING-DESIGN.md) for the four | ||
| timing domains, marker model, assisted mapping, and audition-speed rules. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use barline terminology in the public README.
The existing Time description still refers to “sync points” on Line 22. Update it to “barlines” or “Mark barline” so the public documentation matches the new UI contract.
🤖 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 `@README.md` around lines 25 - 28, Update the Time description’s “sync points”
wording in README.md to use the public UI terminology “barlines” or “Mark
barline,” while preserving the surrounding timing-domain explanation.
| t('asks for a selected sync point before enabling edits', () => { | ||
| const state = _tempoSyncInspectorStatePure(measures, -1); | ||
| assert.strictEqual(state.label, 'No sync point selected'); | ||
| assert.strictEqual(state.label, 'No barline selected'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the remaining inspector copy to “barline.”
src/tempo.js still returns “sync point” in _tempoSyncInspectorStatePure for the no-selection delete title/hint and the first/final non-deletable title. Those states will contradict this renamed label; update the source strings and add coverage for them here.
🤖 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_sync_inspector.test.mjs` at line 26, Update the strings returned
by _tempoSyncInspectorStatePure in src/tempo.js from “sync point” to “barline”
for the no-selection delete title/hint and the first/final non-deletable title.
Extend tests in tempo_sync_inspector.test.mjs to cover each updated state and
assert the renamed copy.
This was referenced Jul 12, 2026
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 12, 2026
Charrette arch 5a / rhythm G5 / trans P6-prep (design slice 2a). Sparse labeled chips on the ruler show where the tempo and meter change through the song. ZERO storage — markers are a PURE function of S.beats (the executable truth, never a second source): _tempoMarkersPure derives tempo-change markers where a measure's BPM leaves the current run beyond 0.01 (the same constant _tempoHasMultipleMeasureBpmsPure uses) and meter markers where the numerator / den changes, with a bar-1 baseline of each and no spurious marker on a trailing partial bar. Memoized on editGen (+ the S.beats ref). Painted in the ruler band via timeToX only (the D-T1 invariant); tempo chips (cyan) and meter chips (violet) stack when they share a barline. Marker fields are never smuggled into S.beats entries. Filed feedpak-spec#51 for the AUTHORED markers the grid can't express (tempo ramps, meter groupings, fermata holds, lock/provenance) — slice 2b, range-provenance and community maps gate on it; not built here. tests/tempo_markers.test.mjs (7): baseline-only on a constant map, tempo-change detection, within-tolerance run behaviour, mid-song meter change + trailing- partial suppression, denominator change, degenerate inputs, and purity (no mutation of the input beats). Verified live: the ruler shows per-bar BPM chips tracking AC/DC's imported tempo drift + a 4/4 meter chip, no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only merged ruler #209/#211). Keep-both seam with #220 on tempo.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
…10) (#227) Charrette arch 5a / rhythm G5 / trans P6-prep (design slice 2a). Sparse labeled chips on the ruler show where the tempo and meter change through the song. ZERO storage — markers are a PURE function of S.beats (the executable truth, never a second source): _tempoMarkersPure derives tempo-change markers where a measure's BPM leaves the current run beyond 0.01 (the same constant _tempoHasMultipleMeasureBpmsPure uses) and meter markers where the numerator / den changes, with a bar-1 baseline of each and no spurious marker on a trailing partial bar. Memoized on editGen (+ the S.beats ref). Painted in the ruler band via timeToX only (the D-T1 invariant); tempo chips (cyan) and meter chips (violet) stack when they share a barline. Marker fields are never smuggled into S.beats entries. Filed feedpak-spec#51 for the AUTHORED markers the grid can't express (tempo ramps, meter groupings, fermata holds, lock/provenance) — slice 2b, range-provenance and community maps gate on it; not built here. tests/tempo_markers.test.mjs (7): baseline-only on a constant map, tempo-change detection, within-tolerance run behaviour, mid-song meter change + trailing- partial suppression, denominator change, degenerate inputs, and purity (no mutation of the input beats). Verified live: the ruler shows per-bar BPM chips tracking AC/DC's imported tempo drift + a 4/4 meter chip, no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only merged ruler #209/#211). Keep-both seam with #220 on tempo.js. Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q Co-authored-by: ChrisBeWithYou <chris@rifflarr.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
This was referenced Jul 12, 2026
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.
What changed
Why
The prior insert action could only promote an existing interior beat. At the end of a truncated map it failed with nothing to insert, even though users reasonably expected to mark the next measure. This aligns the editor with a beat-mapping workflow: fit the musical ruler to fixed source audio and keep topology edits nondestructive in seconds.
Validation
Summary by CodeRabbit
New Features
Documentation