feat(editor): metronome guide + whole-song tempo fits (Accept Whole Fit) - #290
Merged
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 (15)
📝 WalkthroughWalkthroughAdds metronome guide locking, guide-audio onset analysis, whole-song tempo proposals with inferred tails, and an Accept Whole Fit command that commits the proposal as one undoable tempo-map edit. ChangesTempo guide and guide-audio analysis
Guide-routed suggestion generation
Whole-fit acceptance and command wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
Note: the ci workflow only triggers for PRs based on |
This was referenced Jul 15, 2026
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
…ack order
Three Tracks/mixer fixes surfaced by dogfooding:
1. The Master row and tempo guide could vanish ("No guide"): sources were
derived from S.audioUrl, which (a) still points at the previous song at
install time and (b) active-source switching reassigns to a focused
stem. Derive the master from the stable S.masterAudioUrl instead, pinned
at install.
2. "Master Mix" naming: the master track now defaults to the SONG name (its
recording), and the guide button follows the track's DISPLAY name so an
inline rename wins instead of reverting to a generic label.
3. Mixer strips now reorder to match a drag-reorder of the Tracks column
(and rename with it): the mixer reads host.mixerTrackOrder ->
trackSessionOrderedMixKeys() and reorders via _mixerOrderedPartsPure; the
tree commit refreshes the mixer.
Tests: _mixerOrderedPartsPure ordering (tree order, tail-stable, empty);
master-name default/override. 184 JS suites, lint 0 errors.
Fixes behaviour in the guide (#290), Tracks UI (#292), and mixer (#295)
features; carried on the stack tip. Verified live in the desktop build.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
force-pushed
the
feat/editor-metronome-guide
branch
from
July 16, 2026 03:54
cce7ca2 to
95b0219
Compare
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
Lock/unlock a whole barline selection as ONE undoable edit. The new
TempoLockCmd is a first-class exec/rollback command (replacing the old
toggle-plus-checkpoint): entries re-resolve by time (±0.02s) so undo
survives a grid rebuild, every apply re-persists the editor-pref lock
list, and the new `sessionNeutral` history flag keeps lock toggles out
of markSessionDirty — undoable, but never a false Save prompt.
The plan is uniform and uncapped: multi-selection wins over the single
focus, target state = lock-all unless every selected barline is already
locked, no size limit (Ctrl/Cmd+A then S locks the whole song). The
tempo toolbar gains a lock button whose face derives from the same plan
the verb executes ("Lock 12 barlines"), the right-click menu's lock item
is group-aware (right-click outside the group collapses to the clicked
barline), and Ctrl/Cmd-click toggles individual poles.
Selection ergonomics: the marker lane is split into a top HANDLE band
(direct edits — forgiving pole grabs, sub-beat rubato) and the lane body
below (marquee territory) — while the thin full-height barline keeps a
precise 2.5px drag target anywhere and a held selection modifier
restores the forgiving width. Marquee finalize now routes through a pure
selector that focuses the drag end. Hover mirrors the same band-aware
tolerance so the cursor never promises a grab the click would route to
the marquee.
Stacked on feat/editor-metronome-guide (#290).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
…ack order
Three Tracks/mixer fixes surfaced by dogfooding:
1. The Master row and tempo guide could vanish ("No guide"): sources were
derived from S.audioUrl, which (a) still points at the previous song at
install time and (b) active-source switching reassigns to a focused
stem. Derive the master from the stable S.masterAudioUrl instead, pinned
at install.
2. "Master Mix" naming: the master track now defaults to the SONG name (its
recording), and the guide button follows the track's DISPLAY name so an
inline rename wins instead of reverting to a generic label.
3. Mixer strips now reorder to match a drag-reorder of the Tracks column
(and rename with it): the mixer reads host.mixerTrackOrder ->
trackSessionOrderedMixKeys() and reorders via _mixerOrderedPartsPure; the
tree commit refreshes the mixer.
Tests: _mixerOrderedPartsPure ordering (tree order, tail-stable, empty);
master-name default/override. 184 JS suites, lint 0 errors.
Fixes behaviour in the guide (#290), Tracks UI (#292), and mixer (#295)
features; carried on the stack tip. Verified live in the desktop build.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
force-pushed
the
feat/editor-metronome-guide
branch
from
July 16, 2026 03:57
95b0219 to
31cc404
Compare
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
Lock/unlock a whole barline selection as ONE undoable edit. The new
TempoLockCmd is a first-class exec/rollback command (replacing the old
toggle-plus-checkpoint): entries re-resolve by time (±0.02s) so undo
survives a grid rebuild, every apply re-persists the editor-pref lock
list, and the new `sessionNeutral` history flag keeps lock toggles out
of markSessionDirty — undoable, but never a false Save prompt.
The plan is uniform and uncapped: multi-selection wins over the single
focus, target state = lock-all unless every selected barline is already
locked, no size limit (Ctrl/Cmd+A then S locks the whole song). The
tempo toolbar gains a lock button whose face derives from the same plan
the verb executes ("Lock 12 barlines"), the right-click menu's lock item
is group-aware (right-click outside the group collapses to the clicked
barline), and Ctrl/Cmd-click toggles individual poles.
Selection ergonomics: the marker lane is split into a top HANDLE band
(direct edits — forgiving pole grabs, sub-beat rubato) and the lane body
below (marquee territory) — while the thin full-height barline keeps a
precise 2.5px drag target anywhere and a held selection modifier
restores the forgiving width. Marquee finalize now routes through a pure
selector that focuses the drag end. Hover mirrors the same band-aware
tolerance so the cursor never promises a grab the click would route to
the marquee.
Stacked on feat/editor-metronome-guide (#290).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
…ack order
Three Tracks/mixer fixes surfaced by dogfooding:
1. The Master row and tempo guide could vanish ("No guide"): sources were
derived from S.audioUrl, which (a) still points at the previous song at
install time and (b) active-source switching reassigns to a focused
stem. Derive the master from the stable S.masterAudioUrl instead, pinned
at install.
2. "Master Mix" naming: the master track now defaults to the SONG name (its
recording), and the guide button follows the track's DISPLAY name so an
inline rename wins instead of reverting to a generic label.
3. Mixer strips now reorder to match a drag-reorder of the Tracks column
(and rename with it): the mixer reads host.mixerTrackOrder ->
trackSessionOrderedMixKeys() and reorders via _mixerOrderedPartsPure; the
tree commit refreshes the mixer.
Tests: _mixerOrderedPartsPure ordering (tree order, tail-stable, empty);
master-name default/override. 184 JS suites, lint 0 errors.
Fixes behaviour in the guide (#290), Tracks UI (#292), and mixer (#295)
features; carried on the stack tip. Verified live in the desktop build.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 16, 2026
…ack order
Three Tracks/mixer fixes surfaced by dogfooding:
1. The Master row and tempo guide could vanish ("No guide"): sources were
derived from S.audioUrl, which (a) still points at the previous song at
install time and (b) active-source switching reassigns to a focused
stem. Derive the master from the stable S.masterAudioUrl instead, pinned
at install.
2. "Master Mix" naming: the master track now defaults to the SONG name (its
recording), and the guide button follows the track's DISPLAY name so an
inline rename wins instead of reverting to a generic label.
3. Mixer strips now reorder to match a drag-reorder of the Tracks column
(and rename with it): the mixer reads host.mixerTrackOrder ->
trackSessionOrderedMixKeys() and reorders via _mixerOrderedPartsPure; the
tree commit refreshes the mixer.
Tests: _mixerOrderedPartsPure ordering (tree order, tail-stable, empty);
master-name default/override. 184 JS suites, lint 0 errors.
Fixes behaviour in the guide (#290), Tracks UI (#292), and mixer (#295)
features; carried on the stack tip. Verified live in the desktop build.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/audio.js`:
- Around line 350-365: Update ensureGuideOnsets to coalesce concurrent requests
for the same sourceId and url by caching and reusing a keyed in-flight promise,
rather than allowing each invocation to increment _guideGeneration
independently. Ensure all callers receive the shared analysis result and clear
the in-flight entry when processing completes or fails, while preserving
existing cache and supersession behavior for different guides.
In `@src/tempo-suggest.js`:
- Around line 166-180: In the inferred-tail logic around the lock handling
block, stop proposal generation when the authoritative lock time is not strictly
after lastTime; do not interpolate or emit that conflicting lock. Apply the same
chronology guard in the metronome walk around the lock-handling logic at
src/tempo-suggest.js lines 241-256, terminating the walk before emitting an
equal or decreasing locked downbeat time.
- Around line 259-278: Update the targetPulse advancement logic around
_suggestMedianPure and pulseIndex so oversized gaps between consecutive detected
pulses are compared with the recent median and split into the expected number of
beat intervals. Insert inferred pulse slots for missing transients, update
recentGaps with the inferred intervals, and advance time/confidence accordingly
so later barlines remain aligned; retain the existing fallback behavior when no
detected pulse is available.
In `@src/track-session.js`:
- Around line 397-410: The stale-source check must occur before normalization
preserves or replaces tempoGuideSourceId. Update installTrackSession() to detect
when the persisted tempoGuideSourceId is absent from the live sources, then
clear tempoGuideLocked and reset tempoGuideMode to 'audio' rather than allowing
the id to be replaced; add a load test covering a stale persisted stem id and
verifying the lock is cleared.
🪄 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: 8ddfc382-0773-4b16-8de5-53631e149331
📒 Files selected for processing (15)
CHANGELOG.mddocs/TEMPO-MAPPING-DESIGN.mddocs/USER-GUIDE.mdsrc/audio.jssrc/create.jssrc/file-ops.jssrc/input.jssrc/menu-bar.jssrc/shortcuts.jssrc/stem-tracks.jssrc/tempo-suggest.jssrc/tempo.jssrc/track-session.jstests/metronome_guide.test.mjstests/track_session.test.mjs
byrongamatos
added a commit
that referenced
this pull request
Jul 16, 2026
Three still-valid findings from the CodeRabbit pass: - audio.js: coalesce concurrent guide analyses. Two rapid G presses for the same (sourceId, url) each bumped _guideGeneration, so the older request superseded itself and returned null, briefly overwriting the newer success with an analysis error. Reuse a keyed in-flight promise instead; different guides / song switches still supersede via the token. - tempo-suggest.js: stop proposal generation when an authoritative lock conflicts with the proposed chronology. The completion tail no longer demotes a lock behind the last emitted time into an unlocked inferred proposal at a fabricated time, and the metronome walk no longer emits an equal/decreasing locked downbeat time when the pulse walk has run ahead — both terminate. - track-session.js: unlock a stale locked tempo guide at load. A persisted tempoGuideSourceId that no longer exists was silently repointed onto master while staying locked (reconcile then bailed because the id resolved); the load seam now clears the lock/mode, mirroring the stem-op reconcile. Skipped: the "account for missing pulses" finding (heavy heuristic lift) — inserting inferred pulse slots on oversized gaps can't distinguish a missed transient from a genuine tempo change, which the metronome engine is designed to follow; it would fight the module's stated contract and needs tuning against real audio. Tracked as a follow-up, not a minimal root-cause fix. Regression tests added to tests/metronome_guide.test.mjs (all fail pre-fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
A click/reference stem can be locked as the session's TEMPO GUIDE (the ♩ button in the Audio tracks manager; the role persists on the track-session tree). Assisted mapping (G) then analyzes THAT source — decoded on demand into an analysis-only cache, playback and the visible waveform untouched — with the click-track contract: consolidated transients are beat pulses walked by the chart's authored beats-per-measure, so tempo changes in the click are followed directly instead of rejected as drift. Pulse dropout continues on the recent median gap at low confidence, honestly flagged. Ordinary (non-guide) G fits now carry a low-confidence completion tail to the final authored barline (never auto-committed — proposals only), and a new Accept Whole Fit verb (tempo toolbar button + registry command) commits every proposal as ONE undoable TempoMapCmd. Accepting through the final downbeat rescales the open last measure's interior beats onto the accepted tempo (equal-length invariant preserved). Anchor semantics: the focused barline always anchors the fit — locked or not — and a stale multi-selection no longer resets analysis toward the beginning or caps the march (it is only an anchor fallback when nothing has focus). In the metronome engine a locked barline keeps its authored time without disturbing the pulse cursor, so one stale lock cannot phase-shift every later suggestion. Guide-routing discipline: G awaits the guide decode and revalidates its preconditions after the await (mode still on, guide unchanged); a new-song boundary resets the guide cache and orphans in-flight decodes. Design-doc and user-guide sections updated to match. Stacked on feat/editor-track-session (#289) — the guide fields ride the persistent track-session tree it introduced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Locking a stem as the metronome guide records its source id in the track session, but the tracks-manager rename/delete controls rewrite S.stems (via _adopt) without touching the guide role, so tempoGuideSourceId can dangle. A dangling LOCKED guide is not harmless: assisted mapping (G) can't find the source live, and the save-time normalize silently repoints the still-locked role onto the first surviving source (usually the master recording) — so a reopened song would analyze the wrong track as a click. Reconcile at the choke point every stem-list mutation flows through (_adopt): if the locked guide's source vanished, unlock back to the default instead of transferring the lock. Reorder keeps ids, so it is a no-op there. Regression test drives the delete/rename path and pins that a live guide is left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three still-valid findings from the CodeRabbit pass: - audio.js: coalesce concurrent guide analyses. Two rapid G presses for the same (sourceId, url) each bumped _guideGeneration, so the older request superseded itself and returned null, briefly overwriting the newer success with an analysis error. Reuse a keyed in-flight promise instead; different guides / song switches still supersede via the token. - tempo-suggest.js: stop proposal generation when an authoritative lock conflicts with the proposed chronology. The completion tail no longer demotes a lock behind the last emitted time into an unlocked inferred proposal at a fabricated time, and the metronome walk no longer emits an equal/decreasing locked downbeat time when the pulse walk has run ahead — both terminate. - track-session.js: unlock a stale locked tempo guide at load. A persisted tempoGuideSourceId that no longer exists was silently repointed onto master while staying locked (reconcile then bailed because the id resolved); the load seam now clears the lock/mode, mirroring the stem-op reconcile. Skipped: the "account for missing pulses" finding (heavy heuristic lift) — inserting inferred pulse slots on oversized gaps can't distinguish a missed transient from a genuine tempo change, which the metronome engine is designed to follow; it would fight the module's stated contract and needs tuning against real audio. Tracked as a follow-up, not a minimal root-cause fix. Regression tests added to tests/metronome_guide.test.mjs (all fail pre-fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/editor-metronome-guide
branch
from
July 16, 2026 18:29
59af34b to
1535883
Compare
byrongamatos
pushed a commit
that referenced
this pull request
Jul 16, 2026
Lock/unlock a whole barline selection as ONE undoable edit. The new
TempoLockCmd is a first-class exec/rollback command (replacing the old
toggle-plus-checkpoint): entries re-resolve by time (±0.02s) so undo
survives a grid rebuild, every apply re-persists the editor-pref lock
list, and the new `sessionNeutral` history flag keeps lock toggles out
of markSessionDirty — undoable, but never a false Save prompt.
The plan is uniform and uncapped: multi-selection wins over the single
focus, target state = lock-all unless every selected barline is already
locked, no size limit (Ctrl/Cmd+A then S locks the whole song). The
tempo toolbar gains a lock button whose face derives from the same plan
the verb executes ("Lock 12 barlines"), the right-click menu's lock item
is group-aware (right-click outside the group collapses to the clicked
barline), and Ctrl/Cmd-click toggles individual poles.
Selection ergonomics: the marker lane is split into a top HANDLE band
(direct edits — forgiving pole grabs, sub-beat rubato) and the lane body
below (marquee territory) — while the thin full-height barline keeps a
precise 2.5px drag target anywhere and a held selection modifier
restores the forgiving width. Marquee finalize now routes through a pure
selector that focuses the drag end. Hover mirrors the same band-aware
tolerance so the cursor never promises a grab the click would route to
the marquee.
Stacked on feat/editor-metronome-guide (#290).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
byrongamatos
pushed a commit
that referenced
this pull request
Jul 16, 2026
Lock/unlock a whole barline selection as ONE undoable edit. The new
TempoLockCmd is a first-class exec/rollback command (replacing the old
toggle-plus-checkpoint): entries re-resolve by time (±0.02s) so undo
survives a grid rebuild, every apply re-persists the editor-pref lock
list, and the new `sessionNeutral` history flag keeps lock toggles out
of markSessionDirty — undoable, but never a false Save prompt.
The plan is uniform and uncapped: multi-selection wins over the single
focus, target state = lock-all unless every selected barline is already
locked, no size limit (Ctrl/Cmd+A then S locks the whole song). The
tempo toolbar gains a lock button whose face derives from the same plan
the verb executes ("Lock 12 barlines"), the right-click menu's lock item
is group-aware (right-click outside the group collapses to the clicked
barline), and Ctrl/Cmd-click toggles individual poles.
Selection ergonomics: the marker lane is split into a top HANDLE band
(direct edits — forgiving pole grabs, sub-beat rubato) and the lane body
below (marquee territory) — while the thin full-height barline keeps a
precise 2.5px drag target anywhere and a held selection modifier
restores the forgiving width. Marquee finalize now routes through a pure
selector that focuses the drag end. Hover mirrors the same band-aware
tolerance so the cursor never promises a grab the click would route to
the marquee.
Stacked on feat/editor-metronome-guide (#290).
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Co-authored-by: ChrisBeWithYou <chris@rifflarr.local>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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 this is
PR 2 of the #285 re-cut (stacked on #289 — merge that first; this diff is against its branch and retargets to main after it lands). The Metronome Guide and whole-song fit features, rebuilt on the current suggest engine.
The features
tempoGuide*fields — this PR lights them up).S.audioBuffer/waveform/playback are never touched, so declaring a guide redirects tempo truth only. The async decode is generation-guarded and G revalidates its preconditions after the await; a new-song boundary resets the cache and orphans in-flight decodes._suggestMetronomeFitPure): consolidated transients are beat pulses walked by the chart's authored beats-per-measure — tempo changes in the click are followed directly instead of rejected as performance drift. Pulse dropout continues on the recent median gap atconf 0.18, flaggedinferred. A locked barline keeps its authored time without disturbing the pulse cursor — one stale lock can't phase-shift every later suggestion._suggestCompleteTailPure): after the conservative march stops, the proposal continues to the final authored barline atconf 0.08, visibly inferred, never auto-committed (stopDetail: 'inferred-<reason>'keeps the HUD honest).TempoMapCmdwith the standard checkpoint. Accepting through the final downbeat now rescales the open last measure's interiors onto the accepted tempo (equal-length invariant preserved — this fixes the last bar being stranded on the old grid).Design notes
docs/TEMPO-MAPPING-DESIGN.md's Assisted Mapping section is updated to the new contract (whole-song proposals, the metronome-guide exception, lock-vs-pulse-cursor rule);docs/USER-GUIDE.mddocuments the workflow.o.metronome), an exit wrap (o.complete), and one apply extension.Tests
tests/metronome_guide.test.mjs(12): the pulse walk + tempo-change following, dropout fallback, the lock/phase regression, completion tail (prefix untouched, honest stopDetail), tail-onto-lock interpolation, engine routing, the open-final-measure carry (proven red on the base branch), focused-anchor-vs-stale-selection (red on base), guide-analysis decision table, guide lock/unlock persistence round-trip, and Accept Whole Fit exec → one-undo → redo.Gates:
npm test181+1 suites green, pytest 291/2 skipped, lint 0 errors (3 pre-existing baseline warnings).Remaining from the #285 salvage (per
TRACK-SESSION-RECUT-PLAN.md)Marquee + bulk barline locks → Tracks lanes UI → engine/meters.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit
New Features
Bug Fixes