fix(editor): drum-transcription delete is undoable; deleted drums stay deleted after Save - #306
Merged
Merged
Conversation
…eted tab persists as an explicit removal
Two halves of the same tester report ("I deleted my drum MIDI and
cannot undo — that work is lost"):
1. The Tracks column''s drum delete blanked S.drumTab in place and
RESET the entire undo stack — losing not just the delete but every
prior edit''s undo. The reset was the shortcut for "commands in the
stack hold references into the tab"; stack ORDER gives the same
guarantee for free once the delete is itself a command: no older
drum edit can be undone until the rollback has put the very same
tab object back. New DeleteDrumTabCmd (songScope) captures the tab
reference, the drumTabDirty flag, the drums mixer strip, the
pairing map, and the tree — undo restores folder placement and
display rename along with the chart.
2. _buildSaveBody only shipped drum_tab when it was non-null, so a
delete never reached the backend''s explicit-removal wire (a literal
null unlinks drum_tab.json): the pack kept its drum tab on disk and
the "deleted" drums resurrected on the next load. A dirty null now
ships; a clean untouched tab still ships nothing (preserve path).
Also unstales the routes.py comment claiming the UI has no
remove-drums control.
Tests: tests/drum_delete_undo.test.mjs — exec/rollback/redo round-trip
through the real EditHistory (identity restore pinned), prior commands
stay undoable (fails on main: the reset emptied the stack), and the
dirty-null wire vs the clean preserve path (the null leg fails on
main).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: ChrisBeWithYou <chris@rifflarr.local>
|
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)
📝 WalkthroughWalkthroughDrum transcription deletion now runs through an undoable command, preserving prior history and restoring related state on undo. Save payload generation includes dirty ChangesDrum transcription deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TracksColumn
participant EditHistory
participant DeleteDrumTabCmd
participant SharedState
TracksColumn->>EditHistory: Execute DeleteDrumTabCmd
EditHistory->>DeleteDrumTabCmd: exec()
DeleteDrumTabCmd->>SharedState: Clear drum tab and related state
EditHistory->>DeleteDrumTabCmd: rollback()
DeleteDrumTabCmd->>SharedState: Restore captured 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/drum_delete_undo.test.mjs (1)
39-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise track-tree restoration with a non-default fixture.
trackSession: nullcannot verify the command’s folder-placement/rename guarantee. Seed a customized drum row and assert the same tree is restored after both initial undo and redo→undo.Also applies to: 71-96
🤖 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/drum_delete_undo.test.mjs` around lines 39 - 52, Update the seed fixture in seed() to initialize a non-default customized drum track row instead of trackSession: null, including the folder placement and renamed-track properties relevant to restoration. Extend both the initial undo and redo→undo assertions to verify that this customized track tree is restored exactly, including its folder placement and name.
🤖 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.
Nitpick comments:
In `@tests/drum_delete_undo.test.mjs`:
- Around line 39-52: Update the seed fixture in seed() to initialize a
non-default customized drum track row instead of trackSession: null, including
the folder placement and renamed-track properties relevant to restoration.
Extend both the initial undo and redo→undo assertions to verify that this
customized track tree is restored exactly, including its folder placement and
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79059086-62b9-40d1-9781-4451525a5aa5
📒 Files selected for processing (5)
CHANGELOG.mdroutes.pysrc/file-ops.jssrc/track-session.jstests/drum_delete_undo.test.mjs
# Conflicts: # CHANGELOG.md
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.
The bug (tester report: "I deleted my drum MIDI and can''t undo — that work is lost")
Two halves, both in the Tracks column''s drum-transcription delete:
S.drumTabin place and calledS.history.reset()— so one confirm click lost not just the delete but the undo for every prior edit in the session. The reset was the shortcut for "commands in the stack hold references into the tab"._buildSaveBodyonly shippeddrum_tabwhen non-null, so a delete never reached the backend''s explicit-removal wire (a literalnullunlinksdrum_tab.json). Save after deleting → the pack keeps its drum tab → the "deleted" drums resurrect on the next load.The fix
DeleteDrumTabCmd(songScope): the delete is one EditHistory command. Stack order now provides the guarantee the reset was buying — no older drum edit can be undone until the rollback has put the very same tab object back (identity restore, so older commands'' references into its hits stay valid). Captures + restores: the tab, thedrumTabDirtyflag (disk-clean returns clean), the drums mixer strip, the pairing map, and the tree (folder placement + display rename survive undo).nullships on the save wire as the explicit removal; a clean untouched tab still ships nothing (preserve path unchanged). Also unstales theroutes.pycomment claiming the UI has no remove-drums control.Tests
tests/drum_delete_undo.test.mjs— exec/rollback/redo round-trip through the realEditHistorywith identity-restore pinned; prior commands stay undoable (fails on main — the reset emptied the stack); the dirty-null wire (fails on main — field was omitted) vs the clean preserve path. Full suite: 205 JS + 315 pytest green, lint 0 errors.Note for review: arrangement-track deletion (the
editorRemoveArrangementpath) still resets history — that one splices backend session state and renumbers arrangement indices under every stacked command, so it needs its own design pass rather than a rider here.🤖 Generated with Claude Code
Summary by CodeRabbit