feat(editor): assisted tempo mapping — suggest a barline fit from onsets - #215
Merged
Conversation
The first Assisted Mapping slice from docs/TEMPO-MAPPING-DESIGN.md: in Tempo Map mode, G proposes corrected times for every downbeat ahead of the anchor (selected barline, else bar 1). Each bar is predicted from the grid's own per-bar spacing (meter changes and pickups carry for free) scaled by a drift-tracking stretch EMA, snapped to the strongest onset within a ±12% window, and rendered as a dashed ghost pole whose alpha is its confidence, with a hollow handle offset BELOW the real pole handles so the two grab bands never collide. Proposal-only, per the design's non-negotiables: nothing commits silently — clicking a ghost handle accepts THROUGH that barline as one TempoMapCmd (equal beat count; notes ride the reproject; interiors re-space by original fraction exactly like a pole drag), then the suggestions ahead regenerate from the newly confirmed anchor — the seed → suggest → correct loop. Locked barlines are pinned at their own times, full confidence, and re-anchor the march. Where onsets stop corroborating (silence / phase break / tempo change) the run stops, drops its trailing guesses, and the HUD asks for the next human anchor. Esc dismisses. Proposals are keyed on editGen, so any edit invalidates them before a stale click can land. Cycle discipline: src/tempo-suggest.js imports only state/geometry/ui; the onset list is passed in by input.js (which already imports audio) and remembered for regeneration; command execution stays in tempo.js. tests/tempo_suggest.test.mjs (13): drift tracking against a grid-vs- recording tempo mismatch, silence stop + trailing-guess drop, lock pinning, accept-through apply (fixed far edge, equal count), editGen staleness, regeneration, dismissal, ghost hit-test. Suite 108/108, lint 0 errors. Verified live on the :8000 testbed: G proposed 93 bars, a ghost click snapped the barline to the proposed onset time and regenerated 92 ahead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
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 (4)
📝 WalkthroughWalkthroughAdds assisted Tempo Map fitting that proposes onset-aligned barlines, displays them as ghost handles, and applies accepted proposals through undoable tempo re-fitting with lock, dismissal, and stale-edit handling. ChangesAssisted tempo mapping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditorInput
participant tempoSuggest
participant TempoMapView
participant TempoMapCmd
EditorInput->>tempoSuggest: Compute proposals from analyzed onsets
tempoSuggest-->>TempoMapView: Return ghost proposals and confidence
TempoMapView->>tempoSuggest: Hit-test clicked ghost handle
tempoSuggest-->>TempoMapView: Return proposal index
TempoMapView->>TempoMapCmd: Apply accepted tempo re-fit
TempoMapView->>tempoSuggest: Regenerate from accepted barline
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/tempo-suggest.js`:
- Around line 182-234: The direct S.beats replacement paths in loadCDLC() and
editorApplyCreateResult() must also invalidate cached tempo suggestions. Call
_suggestDismiss() or otherwise bump editGen immediately when swapping S.beats,
alongside clearing S.tempoMapMode, so re-entering tempo mode cannot reuse
proposals from the previous grid.
🪄 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: 6484f951-1121-4a6e-a198-eebf56a9224c
📒 Files selected for processing (7)
CHANGELOG.mdsrc/input.jssrc/menu-bar.jssrc/shortcuts.jssrc/tempo-suggest.jssrc/tempo.jstests/tempo_suggest.test.mjs
Comment on lines
+182
to
+234
| // ── Module state (proposal-only; keyed on editGen for staleness) ───── | ||
| let _sug = null; // { anchorIdx, proposals, stopReason, onsets, gen } | ||
|
|
||
| export function _suggestActive() { | ||
| return !!(_sug && _sug.gen === editGen && S.tempoMapMode && _sug.proposals.length); | ||
| } | ||
| export function _suggestProposals() { | ||
| return _suggestActive() ? _sug.proposals : []; | ||
| } | ||
| export function _suggestStopReason() { | ||
| return _sug ? _sug.stopReason : 'end'; | ||
| } | ||
| export function _suggestAvgConf() { | ||
| if (!_suggestActive()) return 0; | ||
| let s = 0; | ||
| for (const p of _sug.proposals) s += p.conf; | ||
| return s / _sug.proposals.length; | ||
| } | ||
|
|
||
| export function _suggestDismiss() { | ||
| _sug = null; | ||
| } | ||
|
|
||
| // Compute (or forward-regenerate) proposals from `anchorIdx`, using — and | ||
| // remembering — the caller-provided onset list. Returns the proposal count. | ||
| export function _suggestCompute(anchorIdx, onsets) { | ||
| const list = onsets || (_sug && _sug.onsets) || null; | ||
| if (!list || !list.length) { _sug = null; return 0; } | ||
| const { proposals, stopReason } = _suggestFitPure(S.beats, list, anchorIdx); | ||
| _sug = { anchorIdx, proposals, stopReason, onsets: list, gen: editGen }; | ||
| return proposals.length; | ||
| } | ||
|
|
||
| // After an accept bumped editGen, re-key and regenerate forward from the | ||
| // newly authoritative downbeat with the remembered onsets. | ||
| export function _suggestRegenerateFrom(anchorIdx) { | ||
| if (!_sug) return 0; | ||
| return _suggestCompute(anchorIdx, _sug.onsets); | ||
| } | ||
|
|
||
| // Ghost-handle hit test: x within `half` px of a proposal's ghost pole. | ||
| // The caller gates y to the ghost-handle band, so suggestions never steal | ||
| // the real poles' grab zone. Returns the proposal's beat index, or -1. | ||
| export function _suggestHitAt(x, half) { | ||
| if (!_suggestActive()) return -1; | ||
| const h = half || 7; | ||
| let best = -1, bestD = h + 1; | ||
| for (const p of _sug.proposals) { | ||
| const d = Math.abs(timeToX(p.time) - x); | ||
| if (d < bestD) { bestD = d; best = p.i; } | ||
| } | ||
| return best; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate editGen, its mutator, and confirm tempo-map commands bump it.
echo "---- editGen references ----"
rg -n 'editGen' -g '*.js' src
echo "---- bumpEditGen definition ----"
rg -n 'function bumpEditGen|bumpEditGen\s*=|export.*bumpEditGen' -g '*.js' src
echo "---- TempoMapCmd / TempoGridCmd definitions ----"
rg -n 'class TempoMapCmd|class TempoGridCmd' -A 25 srcRepository: got-feedBack/feedBack-plugin-editor
Length of output: 6147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- state.js around editGen ----"
sed -n '100,140p' src/state.js
echo "---- bumpEditGen call sites ----"
rg -n 'bumpEditGen\s*\(' src
echo "---- edit history / afterEdit ----"
rg -n 'afterEdit|_afterEdit|editGen' -A 4 -B 4 src/main.js src/state.js src/tempo.js
echo "---- tests importing bumpEditGen ----"
rg -n 'bumpEditGen' tests src -g '*test*' -g '*.mjs' -g '*.js'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 4396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- history.js around _afterEdit ----"
sed -n '1,160p' src/history.js
echo "---- tempo.js command execution points ----"
rg -n 'new TempoMapCmd|new TempoGridCmd|exec\(\)\s*{' src/tempo.js -A 4 -B 4
echo "---- direct S.beats mutations ----"
rg -n 'S\.beats\s*=' srcRepository: got-feedBack/feedBack-plugin-editor
Length of output: 6559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- places that assign to S.beats ----"
rg -n 'S\.beats\s*=' src
echo "---- places that create tempo commands ----"
rg -n 'new TempoMapCmd|new TempoGridCmd' src
echo "---- Tempo command classes and any history dispatch nearby ----"
sed -n '1080,1175p' src/tempo.js
echo
sed -n '1910,1965p' src/tempo.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 10014
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- file-ops.js around beats assignment ----"
sed -n '1,120p' src/file-ops.js
echo "---- create.js around beats assignment ----"
sed -n '2120,2175p' src/create.js
echo "---- tempo-suggest cleanup hooks ----"
rg -n '_suggestDismiss|tempoMapMode|suggest' src/main.js src/file-ops.js src/create.js src/tempo.js src/history.jsRepository: got-feedBack/feedBack-plugin-editor
Length of output: 13827
Invalidate tempo suggestions when beats are replaced outside history. EditHistory._afterEdit() covers command-based edits, but loadCDLC() and editorApplyCreateResult() assign S.beats directly and only clear S.tempoMapMode. That leaves cached proposals around, so re-entering tempo mode can surface suggestions for the previous grid. Dismiss suggestions or bump editGen when swapping S.beats.
🤖 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-suggest.js` around lines 182 - 234, The direct S.beats replacement
paths in loadCDLC() and editorApplyCreateResult() must also invalidate cached
tempo suggestions. Call _suggestDismiss() or otherwise bump editGen immediately
when swapping S.beats, alongside clearing S.tempoMapMode, so re-entering tempo
mode cannot reuse proposals from the previous grid.
This was referenced Jul 12, 2026
…gest-fit # Conflicts: # CHANGELOG.md # src/input.js
This was referenced Jul 12, 2026
byrongamatos
added a commit
that referenced
this pull request
Jul 12, 2026
* feat(editor): undo to last checkpoint (Ctrl+Alt+Z)
Charrette arch 7. Coarse rewind points so a whole tempo-mapping session can be
undone in one keystroke instead of tapping Ctrl+Z through every barline move.
- EditHistory.checkpoint(label) stamps the top-of-undo command (no-op on an
empty stack); the stamp rides the command object, so it survives redo.
- EditHistory.undoToCheckpoint() undoes through (and including) the nearest
stamped command and returns { undone, label, foundCheckpoint }. Two graceful
degradations: no checkpoint in the stack falls back to a single plain undo
(never a silent whole-session rewind — a checkpoint can be shifted off by
MAX_UNDO or dropped by reset()); a no-progress guard stops the instant the
stack stops shrinking, so a refused doUndo (ensureArr / roll-lock) can't spin.
- Surface: Ctrl+Alt+Z (added ahead of the plain Ctrl+Z handler, which doesn't
exclude Alt) + an Edit-menu row; the status line names what it unwound to.
- Checkpoints stamped at three milestones: Tempo Map entry, #215's suggest-fit
accept, and the barline lock toggle (lock toggles aren't history events, so
the stamp records the moment on the current top-of-undo).
tests/undo_checkpoints.test.mjs (real EditHistory over the real S): rewind-
through, single-undo fallback, stamp-survives-redo, no-progress guard, empty
stack — all fail on main. Verified live: Ctrl+Alt+Z routes to the checkpoint
undo (no collision with Ctrl+Z) and a real accent→vibrato→enter-TempoMap→hammer
flow reports "Undid 2 steps back to checkpoint: Tempo Map session." npm test
115 green, lint 0 errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
* fix(editor): checkpoint rewind lands ON the checkpointed state, not one edit past it
undoToCheckpoint() rolled back the stamped command too, so a Tempo-Map-entry
or barline-lock checkpoint (stamped on the last edit BEFORE the milestone)
silently undid one unrelated pre-session edit while the status line claimed a
clean return to the checkpoint.
- checkpoint() now means: the state as of this call. undoToCheckpoint() undoes
everything above the stamp and stops with the stamped command still applied.
- Pressing while already at a checkpoint walks to the PREVIOUS one, so
repeated Ctrl+Alt+Z steps back boundary by boundary instead of going inert.
- Suggest-fit stamps BEFORE exec'ing the accept, preserving its intended
rewind-the-accept-too behaviour under the corrected semantics.
- Status line: a refused first undo no longer gets stomped by 'Nothing to
undo.', and a partial (refused mid-way) rewind reports the real step count.
Regression tests fail on the previous behaviour (verified by revert).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
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>
ChrisBeWithYou
pushed a commit
that referenced
this pull request
Jul 12, 2026
Charrette UX P5 / arch 1 (first half). Select and delete multiple Tempo Map barlines at once, without touching the single-focus model (S.tempoSel — which inspector / tap / lock / modulate / suggest all key on). - New S.tempoSelMulti: Set<downbeatIdx> (the S.drumSel pattern). Shift+click a pole extends the contiguous downbeat range; a drag on empty grid arms a tempo-marquee (the drum editor's deferred-3px `moved` idiom) that box-selects downbeats in its swept X range (plain replaces, Shift unions); Ctrl+A in mode selects every downbeat. The set is index-based, so it is CLEARED — never remapped — in TempoGridCmd exec/rollback and on Tempo Map exit. - Render: a light amber wash across the selected range, and selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass. - Bulk delete = ONE TempoGridCmd: _tempoDeleteBarlinesPure demotes every selected INTERIOR downbeat (never the first/last — the existing guard, generalized to a set) + one renumber. Reachable via Del and right-click "Delete N barlines". - Escape clears the selection, layered UNDER #215's suggest-dismiss (ghosts own Escape while showing). tests/tempo_multiselect.test.mjs (6): marquee hit math, the delete transform (demote+renumber, first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection. Verified live: Ctrl+A → "105 barlines selected" (amber render), Escape clears, a marquee box-selects, Ctrl+A+Del → "Deleted 103 barlines" (first/last kept), no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only on merged #215); keep-both seams with #220 (_tempoMapDraw) and #218/#225 on the shared tempo.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
byrongamatos
pushed a commit
that referenced
this pull request
Jul 12, 2026
…5a) (#226) Charrette UX P5 / arch 1 (first half). Select and delete multiple Tempo Map barlines at once, without touching the single-focus model (S.tempoSel — which inspector / tap / lock / modulate / suggest all key on). - New S.tempoSelMulti: Set<downbeatIdx> (the S.drumSel pattern). Shift+click a pole extends the contiguous downbeat range; a drag on empty grid arms a tempo-marquee (the drum editor's deferred-3px `moved` idiom) that box-selects downbeats in its swept X range (plain replaces, Shift unions); Ctrl+A in mode selects every downbeat. The set is index-based, so it is CLEARED — never remapped — in TempoGridCmd exec/rollback and on Tempo Map exit. - Render: a light amber wash across the selected range, and selected poles read amber (the focus keeps its unique halo). Rides the existing draw pass. - Bulk delete = ONE TempoGridCmd: _tempoDeleteBarlinesPure demotes every selected INTERIOR downbeat (never the first/last — the existing guard, generalized to a set) + one renumber. Reachable via Del and right-click "Delete N barlines". - Escape clears the selection, layered UNDER #215's suggest-dismiss (ghosts own Escape while showing). tests/tempo_multiselect.test.mjs (6): marquee hit math, the delete transform (demote+renumber, first/last guard), the bulk-delete round-trip through the command, the exec/rollback set-clearing contract, and range selection. Verified live: Ctrl+A → "105 barlines selected" (amber render), Escape clears, a marquee box-selects, Ctrl+A+Del → "Deleted 103 barlines" (first/last kept), no errors. npm test 115 green, lint 0 errors. Independent of the in-review queue (deps only on merged #215); keep-both seams with #220 (_tempoMapDraw) and #218/#225 on the shared 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>
This was referenced Jul 12, 2026
Merged
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.
What
The first Assisted Mapping slice from
docs/TEMPO-MAPPING-DESIGN.md(the accepted timing-model design, #211): the seed → suggest → correct loop over existing downbeats.In Tempo Map mode, G (also
Tempo/Grid ▸ Suggest barline fit from anchor) proposes corrected times for every downbeat ahead of the anchor — the selected barline, or bar 1:TempoMapCmd(equal beat count — notes ride the reproject; interior beats re-space by original fraction, exactly like a pole drag with a fixed far edge), then the suggestions ahead regenerate from the newly confirmed anchor.editGen, so any edit invalidates them before a stale click can land. Mode exit and song load also clear them.Design conformance (the non-negotiables)
Architecture
New
src/tempo-suggest.jsimports onlystate/geometry/ui(fully node-testable). The onset list is passed in byinput.js— which already importsaudio.js— and remembered for regeneration, sotempo.jsnever gains an audio import and no cycle forms. Command execution stays intempo.js(ghost-click accept) per the one-undo-owner convention. The pinned_tempoMapHudTextPurestrings are untouched — suggestion state uses its own HUD pure.Testing
tests/tempo_suggest.test.mjs(13, real-import ESM): drift tracking against a 120-grid/126-recording mismatch (all bars within 90 ms of truth), silence stop + trailing-guess drop, lock pinning, accept-through apply semantics (moved anchor, fixed far edge, equal count), editGen staleness, forward regeneration, dismissal lifecycle, ghost hit-test.Gproposed 93 barlines on a real feedpak, a ghost-handle click snapped that barline from 4.294 s to the proposed 4.248 s and regenerated 92 suggestions ahead; zero page errors.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit