fix(editor): drum-transcription delete is undoable; deleted drums stay deleted after Save

CHANGELOG.md

@@ -43,6 +43,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   *analyze* follows the click.
 
 ### Fixed
+- **Deleting the drum transcription is undoable — and no longer wipes the
+  whole undo stack.** The Tracks column's drum delete used to blank the drum
+  tab in place and reset undo history entirely, so one confirm click could
+  strand an hour of edits with nothing to undo. It is now a single history
+  command: Ctrl+Z brings the drums back (same tab object, mixer strip,
+  stem pairing, and tree placement included), and every earlier edit keeps
+  its undo.
+- **A deleted drum transcription stays deleted after Save.** The save body
+  only shipped `drum_tab` when it was non-null, so a delete never reached
+  the backend's explicit-removal path — the pack kept its `drum_tab.json`
+  and the drums resurrected on the next load. A dirty null now ships as the
+  removal wire.
 - **Saving a from-scratch song now works — Save builds it.** A create-mode
   session (New… → GP/MIDI import or blank draft) used to dead-end on Save with
   "Only sloppak-format sessions can be saved" (or the baffling "drum_tab can

routes.py

@@ -4507,8 +4507,8 @@ async def save_song(data: dict):
         #             untouched. This is what the editor frontend sends
         #             unless the user imported/edited a drum tab this session.
         #   - None  → explicit removal — unlinks drum_tab.json and clears the
-        #             manifest key. Supported by the API for completeness;
-        #             the current editor UI has no remove-drums control.
+        #             manifest key. The Tracks column's drum-transcription
+        #             delete ships this (a dirty null in _buildSaveBody).
         drum_tab_payload = data.get("drum_tab", _DRUM_TAB_ABSENT)
         if drum_tab_payload is not _DRUM_TAB_ABSENT and not isinstance(
             drum_tab_payload, (dict, type(None))

src/file-ops.js

@@ -566,7 +566,8 @@ export function _activeArrangementExceedsArchiveLimit() {
 // arrangements, then return the request body for the chosen endpoint.
 // `forceFullSnapshot` is true for save_as_sloppak so the new sloppak
 // gets every arrangement (not just S.currentArr).
-function _buildSaveBody(forceFullSnapshot) {
+// (Exported for tests — the drum_tab wire semantics are pinned there.)
+export function _buildSaveBody(forceFullSnapshot) {
     if (_recState === 'recording') window.editorStopRecordMidi();
 
     // Persist suggested marks BEFORE reconstructChords mints fresh note objects
@@ -700,11 +701,15 @@ function _buildSaveBody(forceFullSnapshot) {
     // Drum-tab payload — separate from arrangements (see sloppak-spec §5.3).
     // S.drumTab is null while the sloppak has none; after +Drums it holds the
     // parsed JSON dict. Only ship `drum_tab` when the user actually
-    // imported / edited it this session (`S.drumTabDirty`) — a tab merely
-    // loaded from disk is left out so the backend's no-op path preserves
-    // the manifest entry unchanged instead of re-serialising the whole
-    // hit list on every unrelated save.
-    if (S.drumTabDirty && S.drumTab !== undefined && S.drumTab !== null) {
+    // imported / edited / DELETED it this session (`S.drumTabDirty`) — a tab
+    // merely loaded from disk is left out so the backend's no-op path
+    // preserves the manifest entry unchanged instead of re-serialising the
+    // whole hit list on every unrelated save. A dirty null MUST ship: it is
+    // the explicit-removal wire (the Tracks column's drum delete) — the
+    // backend only unlinks drum_tab.json on a literal null, so omitting the
+    // field here hit the absent→preserve path and the deleted drums
+    // resurrected on the next load.
+    if (S.drumTabDirty && S.drumTab !== undefined) {
         body.drum_tab = S.drumTab;
     }
     // Beat-primary: strip the runtime beat cache so the wire stays seconds-only.

src/track-session.js

@@ -797,6 +797,69 @@ function selectTrack(trackId, openEditor = false) {
     return true;
 }
 
+// Deleting the drum transcription, as ONE undoable command. This used to
+// blank S.drumTab in place and RESET the whole undo stack — losing not just
+// the delete but every prior edit's undo (the shortcut for "commands in the
+// stack hold references into the tab"). As a command, stack ORDER gives the
+// same guarantee for free: no older drum edit can be undone until this
+// rollback has put the very same tab object back.
+//
+// The capture set is everything the delete touches:
+//   - the tab REFERENCE (identity matters — older drum commands hold
+//     references into its hits; restoring the same object keeps them valid);
+//   - the drumTabDirty flag (a tab loaded from disk and deleted must return
+//     to clean on undo, so an unrelated later save doesn't re-serialize it);
+//   - the drums mixer strip (session mix state, dropped by the delete);
+//   - the pairing map (replaced immutably by _trackLinksRetargetPure, so the
+//     captured reference IS the restore);
+//   - the tree (normalize drops the drum row on exec; committing the
+//     captured tree back restores its folder placement and display rename).
+export class DeleteDrumTabCmd {
+    constructor(rowName) {
+        // Song-level data (like tempo-grid commands): the read-only-roll lock
+        // must not block deleting/undeleting drums while a fretted part is
+        // shown in the piano roll.
+        this.songScope = true;
+        this._name = rowName;
+        this._tab = S.drumTab;
+        this._dirty = !!S.drumTabDirty;
+        this._hadMixStrip = !!(S.partMix && ('drums' in S.partMix));
+        this._mixStrip = S.partMix ? S.partMix.drums : undefined;
+        this._links = S.stemLinks;
+        this._tree = S.trackSession;
+        this._selectedTrackId = S.selectedTrackId;
+    }
+    exec() {
+        S.drumTab = null;
+        // Dirty is what ships the explicit `drum_tab: null` removal on the
+        // next save (see _buildSaveBody) — without it the backend's
+        // absent→preserve path would resurrect drum_tab.json on reload.
+        S.drumTabDirty = true;
+        if (S.partMix) delete S.partMix.drums;
+        S.stemLinks = _trackLinksRetargetPure(S.stemLinks, DRUM_TARGET_ID);
+        if (S.selectedTrackId === transcriptionTrackId(DRUM_TARGET_ID)) {
+            S.selectedTrackId = '';
+        }
+        commit(S.trackSession, `Deleted drum transcription “${this._name}”.`);
+        host.partMixChanged();
+    }
+    rollback() {
+        S.drumTab = this._tab;
+        S.drumTabDirty = this._dirty;
+        if (this._hadMixStrip) {
+            if (!S.partMix) S.partMix = {};
+            S.partMix.drums = this._mixStrip;
+        }
+        S.stemLinks = this._links;
+        if (this._selectedTrackId === transcriptionTrackId(DRUM_TARGET_ID)
+                && !S.selectedTrackId) {
+            S.selectedTrackId = this._selectedTrackId;
+        }
+        commit(this._tree, `Restored drum transcription “${this._name}”.`);
+        host.partMixChanged();
+    }
+}
+
 async function deleteTrack(trackId) {
     const row = _rowsLive().rows.find(item => item.id === trackId);
     if (!row) return false;
@@ -813,12 +876,8 @@ async function deleteTrack(trackId) {
         host.audioSourcesChanged();
     } else if (row.targetId === DRUM_TARGET_ID) {
         if (!confirm(`Delete drum transcription “${row.name}”?`)) return false;
-        S.drumTab = null;
-        S.drumTabDirty = true;
-        delete S.partMix.drums;
-        S.stemLinks = _trackLinksRetargetPure(S.stemLinks, DRUM_TARGET_ID);
-        if (S.history) S.history.reset();
-        commit(S.trackSession, `Deleted drum transcription “${row.name}”.`);
+        const cmd = new DeleteDrumTabCmd(row.name);
+        if (S.history) S.history.exec(cmd); else cmd.exec();
     } else {
         const targets = _trackSessionTargetsPure(S.arrangements, S.drumTab);
         const target = targets.find(item => item.id === row.targetId);

tests/drum_delete_undo.test.mjs

@@ -0,0 +1,149 @@
+/*
+ * Drum-transcription delete is undoable (src/track-session.js
+ * DeleteDrumTabCmd) and persists as an explicit removal
+ * (src/file-ops.js _buildSaveBody ships a dirty null).
+ *
+ * Pinned here:
+ *   - deleting the drum transcription is ONE EditHistory command — it no
+ *     longer resets the whole undo stack (which lost not just the delete
+ *     but every prior edit's undo);
+ *   - rollback restores the SAME tab object (older drum commands hold
+ *     references into its hits), the dirty flag, the drums mixer strip,
+ *     the pairing entry, and the captured tree;
+ *   - exec → rollback is a deep round-trip; redo re-deletes;
+ *   - a dirty NULL drum_tab ships on the save wire (the backend only
+ *     unlinks drum_tab.json on a literal null — omitting the field hit the
+ *     absent→preserve path and deleted drums resurrected on reload);
+ *   - a clean (undeleted, untouched) tab still ships nothing.
+ *
+ * Run: node tests/drum_delete_undo.test.mjs
+ */
+import assert from 'node:assert';
+
+globalThis.localStorage = globalThis.localStorage || {
+    getItem: () => null, setItem: () => {}, removeItem: () => {},
+};
+globalThis.document = globalThis.document || { getElementById: () => null };
+
+const { DeleteDrumTabCmd } = await import('../src/track-session.js');
+const { EditHistory } = await import('../src/history.js');
+const { _buildSaveBody } = await import('../src/file-ops.js');
+const { S } = await import('../src/state.js');
+
+let pass = 0, fail = 0;
+function t(name, fn) {
+    try { fn(); pass++; console.log('  ok   ' + name); }
+    catch (e) { fail++; console.error('  FAIL ' + name + ': ' + e.message); }
+}
+
+function seed() {
+    const tab = { name: 'Drums', version: 1, hits: [{ t: 0.5, lane: 'kick' }, { t: 1.0, lane: 'snare' }] };
+    Object.assign(S, {
+        sessionId: 'sess-1', createMode: false, format: 'sloppak', sloppakForm: 'zip',
+        filename: 'song.feedpak', title: 'T', artist: 'A',
+        arrangements: [{
+            name: 'Lead', tuning: [0, 0, 0, 0, 0, 0], capo: 0,
+            notes: [], chords: [], chord_templates: [],
+        }],
+        currentArr: 0, beats: [], sections: [],
+        drumTab: tab, drumTabDirty: false,
+        partMix: { drums: { audible: false, vol: 0.5 }, 'arr:0': { audible: true, vol: 1 } },
+        stemLinks: { drums: 'Drums_stem', lead: 'Guitar_L' },
+        trackSession: null, trackHeights: {}, stems: [],
+        sessionDirty: false,
+        history: new EditHistory(),
+        sel: new Set(),
+    });
+    return tab;
+}
+
+t('delete execs as one history command — the stack survives', () => {
+    seed();
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    assert.strictEqual(S.drumTab, null, 'tab cleared');
+    assert.strictEqual(S.drumTabDirty, true, 'dirty → the removal ships on the next save');
+    assert.strictEqual('drums' in S.partMix, false, 'mixer strip dropped');
+    assert.strictEqual(S.stemLinks.drums, undefined, 'pairing retargeted away');
+    assert.strictEqual(S.stemLinks.lead, 'Guitar_L', 'other pairings untouched');
+    assert.strictEqual(S.history.undo.length, 1, 'the delete IS on the undo stack');
+});
+
+t('undo restores the very same tab object, flags, strip, and pairing', () => {
+    const tab = seed();
+    const linksBefore = S.stemLinks;
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    S.history.doUndo();
+    assert.strictEqual(S.drumTab, tab,
+        'IDENTITY restore — older drum commands hold references into these hits');
+    assert.strictEqual(S.drumTabDirty, false,
+        'a disk-clean tab returns clean, so an unrelated save does not re-serialize it');
+    assert.deepStrictEqual(S.partMix.drums, { audible: false, vol: 0.5 }, 'mixer strip back');
+    assert.strictEqual(S.stemLinks, linksBefore, 'pairing map reference restored');
+    assert.strictEqual(S.history.redo.length, 1, 'redo holds the delete');
+});
+
+t('redo re-deletes; a second undo restores again (round-trip stability)', () => {
+    const tab = seed();
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    S.history.doUndo();
+    S.history.doRedo();
+    assert.strictEqual(S.drumTab, null);
+    assert.strictEqual(S.drumTabDirty, true);
+    assert.strictEqual('drums' in S.partMix, false);
+    S.history.doUndo();
+    assert.strictEqual(S.drumTab, tab);
+    assert.strictEqual(S.drumTabDirty, false);
+});
+
+t('redo clears a drum selection made after undo', () => {
+    seed();
+    S.selectedTrackId = 'transcription:drums';
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    assert.strictEqual(S.selectedTrackId, '', 'initial delete cannot leave a dangling selection');
+    S.history.doUndo();
+    assert.strictEqual(S.selectedTrackId, 'transcription:drums',
+        'undo restores the selection captured with the deleted row');
+    // This interaction happens outside history, after the row exists again.
+    // Redo calls DeleteDrumTabCmd.exec directly, not deleteTrack's click handler.
+    S.selectedTrackId = 'transcription:drums';
+    S.history.doRedo();
+    assert.strictEqual(S.selectedTrackId, '',
+        'redo owns selection cleanup instead of leaving a missing row selected');
+});
+
+t('a dirty tab deleted and undone returns dirty (its edits still need saving)', () => {
+    seed();
+    S.drumTabDirty = true;   // the user edited hits earlier this session
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    S.history.doUndo();
+    assert.strictEqual(S.drumTabDirty, true);
+});
+
+t('prior commands stay undoable after the delete — the old reset lost them', () => {
+    seed();
+    let state = 'applied';
+    // A stand-in for any earlier edit: exec/rollback just flip a flag.
+    S.history.exec({ songScope: true, exec() { state = 'applied'; }, rollback() { state = 'rolled-back'; } });
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    assert.strictEqual(S.history.undo.length, 2, 'both commands on the stack');
+    S.history.doUndo();          // un-delete drums
+    S.history.doUndo();          // then the earlier edit rolls back fine
+    assert.strictEqual(state, 'rolled-back');
+});
+
+t('the save wire ships an explicit drum_tab null after a delete', () => {
+    seed();
+    S.history.exec(new DeleteDrumTabCmd('Drums'));
+    const body = _buildSaveBody(false);
+    assert.ok('drum_tab' in body, 'field present — absent means preserve-on-disk');
+    assert.strictEqual(body.drum_tab, null, 'literal null is the removal wire');
+});
+
+t('a clean, untouched tab still ships nothing (preserve path)', () => {
+    seed();
+    const body = _buildSaveBody(false);
+    assert.strictEqual('drum_tab' in body, false);
+});
+
+console.log(`\n${pass} passed, ${fail} failed`);
+process.exit(fail ? 1 : 0);