From 620f30e5d2cfcf67e800913cae1dd63f78ce978b Mon Sep 17 00:00:00 2001 From: Kevin Blackburn-Matzen Date: Sat, 18 Jul 2026 16:12:30 -0700 Subject: [PATCH] Record node toggles in undo history Fixes #54. toggleNode mutated the tree without pushing a history entry, so `tree` and `history[historyIndex]` diverged as soon as a node was disabled. A subsequent undo then restored a snapshot predating both the toggle and the edit before it -- discarding one more edit than the user asked to undo. The new test demonstrates this directly: toggle after a parameter change, undo, and pre-fix the parameter edit is gone too. Disabling a node changes the rendered geometry and the exported mesh, so it is a document mutation, not view state. Compare toggleExpanded and selectNode, which are genuinely view-only and correctly stay out of history. Also clears a dangling selectedNodeId on undo/redo. Both restore the tree wholesale and can land on one where the selected node no longer exists; consumers mostly do findNode(...) and bail on null, so it degraded quietly rather than throwing. The tests fail 4/5 against the pre-fix code. The one that passes asserts a selection surviving an undo, which the old code also did. This is the focused fix, not the commit() refactor the issue proposes as the better long-term shape -- see the PR for why. Co-Authored-By: Claude Opus 4.8 --- src/store/modelerStore.test.ts | 57 ++++++++++++++++++++++++++++++++++ src/store/modelerStore.ts | 27 +++++++++++++--- 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/store/modelerStore.test.ts b/src/store/modelerStore.test.ts index 5bb6bff..975840c 100644 --- a/src/store/modelerStore.test.ts +++ b/src/store/modelerStore.test.ts @@ -130,6 +130,29 @@ describe('Modeler editing scenarios', () => { expect(getState().tree!.params.radius).toBe(20); // default }); + it('clears a selection that the undone tree no longer contains', () => { + getState().addPrimitive('box'); + const boxId = getState().tree!.id; + getState().selectNode(boxId); + expect(getState().selectedNodeId).toBe(boxId); + + // Undo back past the box's creation — the selected node is gone. + getState().undo(); + expect(getState().tree).toBeNull(); + expect(getState().selectedNodeId).toBeNull(); + }); + + it('keeps a selection that survives the undo', () => { + getState().addPrimitive('box'); + const boxId = getState().tree!.id; + getState().updateNodeParams(boxId, { radius: 3 }); + getState().selectNode(boxId); + + getState().undo(); + expect(getState().tree!.id).toBe(boxId); + expect(getState().selectedNodeId).toBe(boxId); + }); + it('undoes wrap operation', () => { getState().addPrimitive('box'); const boxId = getState().tree!.id; @@ -236,6 +259,40 @@ describe('Modeler editing scenarios', () => { expect(getState().tree!.enabled).toBe(true); }); + it('records the toggle in history so undo reverts only the toggle', () => { + getState().addPrimitive('box'); + const id = getState().tree!.id; + getState().updateNodeParams(id, { radius: 7 }); + + getState().toggleNode(id); + expect(getState().tree!.enabled).toBe(false); + + // Undo should restore the enabled box with radius 7 — not discard the + // parameter edit along with the toggle. + getState().undo(); + expect(getState().tree!.enabled).toBe(true); + expect(getState().tree!.params.radius).toBe(7); + }); + + it('keeps tree in sync with history[historyIndex] after a toggle', () => { + getState().addPrimitive('box'); + getState().toggleNode(getState().tree!.id); + + const { tree, history, historyIndex } = getState(); + expect(history[historyIndex]).toEqual(tree); + }); + + it('redoes a toggle', () => { + getState().addPrimitive('box'); + const id = getState().tree!.id; + getState().toggleNode(id); + getState().undo(); + expect(getState().tree!.enabled).toBe(true); + + getState().redo(); + expect(getState().tree!.enabled).toBe(false); + }); + it('disabled nodes pass tree validation', () => { getState().addPrimitive('box'); const boxId = getState().tree!.id; diff --git a/src/store/modelerStore.ts b/src/store/modelerStore.ts index ae43848..d40c04a 100644 --- a/src/store/modelerStore.ts +++ b/src/store/modelerStore.ts @@ -76,6 +76,16 @@ function cloneTree(node: SDFNodeUI): SDFNodeUI { return JSON.parse(JSON.stringify(node)); } +/** + * The selection to keep after the tree is replaced wholesale. Undo/redo can + * restore a tree in which the selected node no longer exists; leaving the id + * dangling makes every `findNode` consumer silently no-op. + */ +function surviving(tree: SDFNodeUI | null, selectedNodeId: string | null): string | null { + if (!tree || !selectedNodeId) return null; + return findNode(tree, selectedNodeId) ? selectedNodeId : null; +} + function findNode(tree: SDFNodeUI, id: string): SDFNodeUI | null { if (tree.id === id) return tree; for (const child of tree.children) { @@ -258,10 +268,17 @@ export const useModelerStore = create()((set, get) => ({ }, toggleNode: (id) => { - const { tree } = get(); + const state = get(); + const { tree } = state; if (!tree) return; const newTree = updateInTree(tree, id, (node) => ({ ...node, enabled: !node.enabled })); - set({ tree: newTree }); + // Disabling a node changes the rendered geometry and the exported mesh, so + // this is a document mutation and belongs in history like any other. + // Without the push, `tree` and `history[historyIndex]` diverge and a + // subsequent undo discards one more edit than the user asked for. + const newHistory = state.history.slice(0, state.historyIndex + 1); + newHistory.push(cloneTree(newTree)); + set({ tree: newTree, history: newHistory, historyIndex: newHistory.length - 1 }); }, toggleExpanded: (id) => { @@ -683,7 +700,8 @@ export const useModelerStore = create()((set, get) => ({ const { historyIndex, history } = get(); if (historyIndex > 0) { const newIndex = historyIndex - 1; - set({ tree: history[newIndex] ? cloneTree(history[newIndex]!) : null, historyIndex: newIndex }); + const restored = history[newIndex] ? cloneTree(history[newIndex]!) : null; + set({ tree: restored, historyIndex: newIndex, selectedNodeId: surviving(restored, get().selectedNodeId) }); } }, @@ -691,7 +709,8 @@ export const useModelerStore = create()((set, get) => ({ const { historyIndex, history } = get(); if (historyIndex < history.length - 1) { const newIndex = historyIndex + 1; - set({ tree: history[newIndex] ? cloneTree(history[newIndex]!) : null, historyIndex: newIndex }); + const restored = history[newIndex] ? cloneTree(history[newIndex]!) : null; + set({ tree: restored, historyIndex: newIndex, selectedNodeId: surviving(restored, get().selectedNodeId) }); } },