feat(editor): make drum edits undoable + harden undo history - #89
Merged
Conversation
Drum edits (click-add, drag-move, Delete, G/F/K articulation toggles) previously bypassed EditHistory entirely: a mis-drag was unrecoverable and Ctrl+Z in drum-edit mode silently undid the last NOTE edit in the hidden guitar/keys arrangement. Adds four ref-based drum command classes (AddDrumHitCmd / DeleteDrumHitsCmd / MoveDrumHitsCmd / ToggleDrumArticulationCmd) that preserve selection across resorts and mark the drum tab dirty on exec AND rollback. Hardens the shared history for multi-arrangement editing: - exec() tags each command with the arrangement it ran against; undo/redo switch back to that arrangement (or refuse if it was removed) instead of rolling index-based commands into whichever arrangement is active. - Undo stack capped at 500 (oldest dropped first). - remove-arrangement resets history (the splice renumbers arrangements). Tests: tests/drum_undo.test.js (9 cases); node --check clean; all 26 JS test files pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes undo/redo arrangement-aware in EditHistory (tagging commands with a target arrangement, capping the stack at 500, clearing history on arrangement removal) and converts drum-grid edits (add, delete, move, toggle) into undoable command classes wired into the drum editor UI, with accompanying tests and changelog updates. ChangesUndo History and Drum Commands
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DrumEditor
participant DrumCmd as DrumHitCmd
participant SHistory as S.history
participant DrumTab as S.drumTab
User->>DrumEditor: perform edit (add/move/delete/toggle)
DrumEditor->>DrumCmd: construct command with hit references
DrumEditor->>SHistory: exec(DrumCmd)
SHistory->>SHistory: tag command with _arrIdx
SHistory->>DrumCmd: exec()
DrumCmd->>DrumTab: update hits, resort, remap S.drumSel
DrumCmd-->>SHistory: set S.drumTabDirty
User->>SHistory: undo()
SHistory->>SHistory: _historyEnsureArr(cmd)
SHistory->>DrumCmd: rollback()
DrumCmd->>DrumTab: restore prior state, resort, remap selection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 `@screen.js`:
- Around line 13197-13220: The ToggleDrumArticulationCmd undo path is
re-toggling instead of restoring the original hit state, so pre-existing values
are not preserved. Update ToggleDrumArticulationCmd to capture each hit’s
original articulation value/absence at construction or exec time, then have
rollback restore that saved state instead of calling _toggle; keep exec as the
toggle operation and make rollback reconstruct the exact prior k/g field values
for every hit.
🪄 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: e81a7368-2e6b-4d08-8407-848e85fa3576
📒 Files selected for processing (3)
CHANGELOG.mdscreen.jstests/drum_undo.test.js
Comment on lines
+13197
to
+13220
| class ToggleDrumArticulationCmd { | ||
| // g/f/k toggles are involutive per hit, so rollback = exec and mixed | ||
| // initial states round-trip exactly. The choke cymbal-only gate is applied | ||
| // at CONSTRUCTION (callers pass only the refs the toggle really touches), | ||
| // keeping exec/rollback perfectly symmetric. | ||
| constructor(hitRefs, field) { | ||
| this.hits = [...hitRefs]; | ||
| this.field = field; | ||
| this.songScope = true; | ||
| } | ||
| _toggle() { | ||
| for (const h of this.hits) { | ||
| if (this.field === 'k') { | ||
| if (h.k) delete h.k; else h.k = 0.08; | ||
| } else if (h[this.field]) { | ||
| delete h[this.field]; | ||
| } else { | ||
| h[this.field] = true; | ||
| } | ||
| } | ||
| S.drumTabDirty = true; | ||
| } | ||
| exec() { this._toggle(); } | ||
| rollback() { this._toggle(); } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve original articulation values on undo.
rollback() re-toggles instead of restoring the captured pre-edit value, so existing values are not round-tripped exactly. For example, an imported k: 0.05 hit is undone as k: 0.08, and g: false is undone as missing.
Suggested fix
class ToggleDrumArticulationCmd {
- // g/f/k toggles are involutive per hit, so rollback = exec and mixed
- // initial states round-trip exactly. The choke cymbal-only gate is applied
+ // Capture original values so undo round-trips imported/noncanonical drum tabs
+ // exactly. The choke cymbal-only gate is applied
// at CONSTRUCTION (callers pass only the refs the toggle really touches),
- // keeping exec/rollback perfectly symmetric.
+ // keeping exec/rollback scoped to the same hits.
constructor(hitRefs, field) {
this.hits = [...hitRefs];
this.field = field;
+ this.before = this.hits.map(h => ({
+ has: Object.prototype.hasOwnProperty.call(h, field),
+ value: h[field],
+ }));
this.songScope = true;
}
@@
}
exec() { this._toggle(); }
- rollback() { this._toggle(); }
+ rollback() {
+ for (let i = 0; i < this.hits.length; i++) {
+ if (this.before[i].has) this.hits[i][this.field] = this.before[i].value;
+ else delete this.hits[i][this.field];
+ }
+ S.drumTabDirty = true;
+ }
}📝 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.
Suggested change
| class ToggleDrumArticulationCmd { | |
| // g/f/k toggles are involutive per hit, so rollback = exec and mixed | |
| // initial states round-trip exactly. The choke cymbal-only gate is applied | |
| // at CONSTRUCTION (callers pass only the refs the toggle really touches), | |
| // keeping exec/rollback perfectly symmetric. | |
| constructor(hitRefs, field) { | |
| this.hits = [...hitRefs]; | |
| this.field = field; | |
| this.songScope = true; | |
| } | |
| _toggle() { | |
| for (const h of this.hits) { | |
| if (this.field === 'k') { | |
| if (h.k) delete h.k; else h.k = 0.08; | |
| } else if (h[this.field]) { | |
| delete h[this.field]; | |
| } else { | |
| h[this.field] = true; | |
| } | |
| } | |
| S.drumTabDirty = true; | |
| } | |
| exec() { this._toggle(); } | |
| rollback() { this._toggle(); } | |
| class ToggleDrumArticulationCmd { | |
| // Capture original values so undo round-trips imported/noncanonical drum tabs | |
| // exactly. The choke cymbal-only gate is applied | |
| // at CONSTRUCTION (callers pass only the refs the toggle really touches), | |
| // keeping exec/rollback scoped to the same hits. | |
| constructor(hitRefs, field) { | |
| this.hits = [...hitRefs]; | |
| this.field = field; | |
| this.before = this.hits.map(h => ({ | |
| has: Object.prototype.hasOwnProperty.call(h, field), | |
| value: h[field], | |
| })); | |
| this.songScope = true; | |
| } | |
| _toggle() { | |
| for (const h of this.hits) { | |
| if (this.field === 'k') { | |
| if (h.k) delete h.k; else h.k = 0.08; | |
| } else if (h[this.field]) { | |
| delete h[this.field]; | |
| } else { | |
| h[this.field] = true; | |
| } | |
| } | |
| S.drumTabDirty = true; | |
| } | |
| exec() { this._toggle(); } | |
| rollback() { | |
| for (let i = 0; i < this.hits.length; i++) { | |
| if (this.before[i].has) this.hits[i][this.field] = this.before[i].value; | |
| else delete this.hits[i][this.field]; | |
| } | |
| S.drumTabDirty = true; | |
| } | |
| } |
🤖 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 `@screen.js` around lines 13197 - 13220, The ToggleDrumArticulationCmd undo
path is re-toggling instead of restoring the original hit state, so pre-existing
values are not preserved. Update ToggleDrumArticulationCmd to capture each hit’s
original articulation value/absence at construction or exec time, then have
rollback restore that saved state instead of calling _toggle; keep exec as the
toggle operation and make rollback reconstruct the exact prior k/g field values
for every hit.
_historyEnsureArr routed undo/redo through editorSelectArrangement, whose flattenChords() -> _flattenArrChords() re-sorts arr.notes. Index-based note commands (MoveNoteCmd/DeleteNotesCmd/ResizeSustainCmd) rely on notes() staying in exec-time order at rollback, so a drag-past-neighbour + arrangement switch + undo rolled back the WRONG index and silently corrupted a different note — reintroducing the exact bug this PR set out to fix. A MANUAL switch also re-sorts the revisited arrangement, so merely making the undo-driven switch non-flattening still leaves a hole (edit arr0, switch away, edit arr1, switch back to arr0 -> re-sort invalidates arr0's pending index commands). The provably-correct fix is to reset the history on every USER arrangement switch, so no index-based command ever survives an arr.notes re-sort and cross-arrangement undo can never occur. The undo-driven switch opts out via _undoDrivenArrSwitch so it never discards the stack it is replaying. This mirrors the existing save/build reset() (another renumbering event). Also cap the undo stack in doRedo (a redo pushes back onto it), mirroring exec()/doUndo, so a redo-heavy session can't grow past MAX_UNDO. Adds tests/cross_arr_undo.test.js, which eval's the REAL routing (_historyEnsureArr + editorSelectArrangement + MoveNoteCmd, with a flatten stub that mimics the real re-sort) — it fails on the pre-fix code and passes here. The existing drum_undo.test.js only eval's the @pure blocks and so stubbed _historyEnsureArr out entirely, leaving this path uncovered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 7, 2026
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.
Summary
Drum edits (click-add, drag-move, Delete, and the G/F/K ghost/flam/choke toggles) previously bypassed
EditHistoryentirely — a mis-drag in the drum grid was unrecoverable, and Ctrl+Z while drum-edit mode was active silently undid the last note edit in the hidden guitar/keys arrangement instead. This PR routes all four drum mutations through the shared undo history and hardens that history for multi-arrangement editing.Drum undo
@pure:drum-cmdsblock:AddDrumHitCmd,DeleteDrumHitsCmd,MoveDrumHitsCmd,ToggleDrumArticulationCmd.filter(), so captured indices go stale but refs survive both. Selection (S.drumSelis index-based) is rebuilt from refs after every reorder.MoveNoteCmdpath uses, so redo replays the exact drag result; a snapped no-op drag no longer pushes a command (or spuriously dirties the tab).exec()/rollback()marksS.drumTabDirtyso a save issued after an undo persists the reverted state.History hardening
exec()tags each command with the arrangement it ran against (_arrIdx); undo/redo now switch back to that arrangement first — updating the arrangement selector — instead of rolling index-based commands back into whichever arrangement happens to be active (silent corruption of the wrong arrangement's notes). Song-level commands (drum tab) opt out viasongScope.MAX_UNDO = 500, oldest dropped first.remove-arrangementnow resets history: the splice renumbers every arrangement after the removed index, so older commands would target the wrong one (same rationale as the save-timereconstructChords()reset, Undo history not invalidated on save — index-based commands can corrupt after reconstructChords() #18).Verification
node --check screen.jscleantests/drum_undo.test.js— 9 cases (add/delete/move/articulation exec+undo+redo round-trips, selection-survives-resort, malformed-drum_tabguard, 500-cap eviction, arrangement tagging)tests/edit_history_reset.test.jsunchanged and green🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit