feat(editor): flatten dialog names both directions (conform vs rebuild) - #225
Merged
Merged
Conversation
…-reproject commands Sync tempo, the BPM box's constant-tempo rescale, and the audio Offset nudge each mutated the timeline directly with no undo, walking only the current arrangement's plain notes plus the global beats/sections. Every other arrangement — and all chords/anchors/handshapes/phrases, plus the drum tab on the partial paths — was left behind: silent multi-part corruption that couldn't be undone. All three now route through one command (TempoMapCmd / TempoOffsetCmd) whose beat-primary lift->reproject is total (every timed object, every part) and fully undoable. Sync and rescale pivot the scale at the first downbeat (or the focused barline) rather than t=0, so a pickup/lead-in song no longer skews. The applied audio offset moved from the DOM input's dataset.applied onto command-owned S.appliedOffset so undo restores it; the Sync dialog's duplicate offset field was removed in favor of the undoable toolbar Offset. New tests/tempo_op_commands.test.mjs deep-diffs a two-arrangement song with chords + drums to prove every part moves and undo restores the exact seconds, plus source guards that the wrappers route through the commands. Removes the obsolete apply_offset.test.js (sliced the deleted _shiftArrangementTimes) and updates beat_lock.test.mjs's section case off the deleted inline loop. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Charrette UX P2 / arch 4 (tempo-mapping PR 3). Typing a BPM for a song with a variable tempo map replaced the bare window.confirm with a small in-app dialog (new _editorPromptChoice in ui.js, same modal idiom as _editorPromptText) that NAMES the two genuinely-different edits: - Conform notes to the new tempo — the previously-missing path. Flattens in the TempoMapCmd direction: beats are truth, seconds reproject onto the flat grid, so every part (all arrangements, chords, drums, anchors, handshapes) rides to the new constant tempo. No hand-scaled seconds (an invariant violation on locked/warped grids — the command's lift→reproject is the safe path). - Rebuild the grid only — today's flatten (TempoGridCmd): seconds hold, beats re-lift, for when the notes already sit on the recording. Both use the same flat grid from _tempoFlattenToBpmPure (anchored at bar 1 = PR 1's pivot) and are undoable; the status lines name the result + "Undo restores the map." editorSetBPM's flatten branch becomes async to await the choice. tests/flatten_choice.test.mjs: conform moves notes in ALL parts (2-arr song) and undo restores; rebuild keeps every note's exact seconds while the grid flattens; _tempoFlattenToBpmPure keeps the beat count + bar-1 anchor. tests/prompt_choice.test.mjs: the dialog resolves the chosen key, Cancel→null, and a second dialog settles the first. npm test 116 green, lint 0 errors. Stacked on #218 (needs its pivot helper) — base this PR on fix/editor-tempo-op-commands; after #218 squash-merges, rebase onto main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughVariable-tempo BPM flattening now opens an asynchronous choice dialog. Users can conform notes to a constant tempo or rebuild only the beat grid, with session validation, distinct history commands, undo coverage, and prompt interaction tests. ChangesVariable Tempo Flattening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant ChoiceModal
participant History
participant TempoCommand
Editor->>ChoiceModal: Request flattening choice
ChoiceModal-->>Editor: Return conform or rebuild-grid selection
Editor->>History: Execute selected tempo command
History->>TempoCommand: Apply flattening edit
TempoCommand-->>Editor: Return updated tempo and note state
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 |
…/redo Undo restored S.appliedOffset but left the visible toolbar input stale, and editorNudgeOffset computes the NEXT offset from el.value — so one +10ms click after a Ctrl-Z re-applied the undone nudge on top (input 0.010 → passes 0.020, delta = 0.020 − 0 = 0.020). exec/rollback now both sync the input to S.appliedOffset (typeof-document guarded for the node suites). Regression test fails pre-fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…— every phrase was stranded by tempo ops
Phrases anchor on start_time (input.js authoring, routes.py save); _eachTimed
visited ph.time, so lift skipped every phrase and reproject left them on the
old timeline through sync / BPM-rescale / offset — the exact stale-scale bug
class this PR set out to close, hidden because the fixtures (here AND in the
pre-A2 golden's frozen legacy reference) seeded the same wrong field, making
the phrase assertions vacuous. Now visits ('start_time', 'span') so end_time
(present on server-loaded phrases) rides too; fixtures use the real shape and
the tempo_op_commands phrase assertions fail pre-fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…log await The flatten choice dialog awaits across real time. The overlay traps pointer and keyboard, but an already-in-flight async import can land while the dialog is open, swapping the session and beat grid — the user's choice would then flatten the NEW song unprompted. Re-check S.sessionId and the variable-map precondition after the await, before either command executes. (window.confirm was synchronous, so this gap is new with the dialog.) Also correct the _editorPromptChoice doc: the backdrop re-focuses the dialog (like the editor's other modals); it does not dismiss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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>
…hoice # Conflicts: # CHANGELOG.md # src/main.js # src/tempo.js # tests/tempo_op_commands.test.mjs
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.
Tempo-mapping PR 3 (charrette UX P2 / arch 4). Typing a BPM for a song with a variable tempo map replaced the bare
window.confirmwith a small in-app dialog that names the two genuinely-different edits:TempoMapCmddirection: beats are truth, seconds reproject onto the flat grid, so every part (all arrangements, chords, drums, anchors, handshapes) rides to the new constant tempo. No hand-scaled seconds — the charrette flags that shortcut as an invariant violation on locked/warped grids; the command's lift→reproject is the safe path.TempoGridCmd): seconds hold, beats re-lift — for when the notes already sit on the recording.Both use the same flat grid from
_tempoFlattenToBpmPure(anchored at bar 1 = PR 1's pivot) and are undoable; status lines name the result + "Undo restores the map."New
_editorPromptChoiceinsrc/ui.jsreuses the_editorPromptText/_installModalKeyboardmodal idiom.editorSetBPM's flatten branch becomesasyncto await the choice.Tests
tests/flatten_choice.test.mjs: conform moves notes in ALL parts of a 2-arrangement song and undo restores exact seconds; rebuild keeps every note's exact seconds while the grid flattens;_tempoFlattenToBpmPurekeeps the beat count + bar-1 anchor.tests/prompt_choice.test.mjs: the dialog resolves the chosen key, Cancel → null (no accidental edit), and opening a second dialog settles the first (no hung await).npm test116 green,npm run lint0 errors. (The testbed is down, so the dialog was verified via the DOM-stub test rather than driven live.)⚠ Stacked on #218
This PR depends on #218's pivot helper + is based on
fix/editor-tempo-op-commands, so the diff here is PR 3-only. Merge #218 first (it's ahead in the queue); after #218 squash-merges, this needs a quickgit rebase --onto main fix/editor-tempo-op-commands feat/editor-flatten-choiceto drop the now-merged commits, and GitHub will retarget the base tomain.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes