Record node toggles in undo history#58
Open
kmatzen wants to merge 1 commit into
Open
Conversation
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 <noreply@anthropic.com>
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
1 participant
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.
Fixes #54.
toggleNodemutated the tree without pushing a history entry, sotreeandhistory[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 it directly. Toggle a node 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
toggleExpandedandselectNode, which are genuinely view-only and correctly stay out of history.Also fixed
undoandredorestored onlytreeandhistoryIndex, neverselectedNodeId. Both replace the tree wholesale and can land on one where the selected node no longer exists. Consumers mostly dofindNode(...)and bail on null, so it degraded quietly rather than throwing — butremoveNodewas the only action that cleared a stale selection. Asurviving()helper now drops the id when the restored tree doesn't contain it.Verification
tsc --noEmitThe 23 pre-existing store tests still pass, which is the check that mattered most here —
toggleNodenow writes history, and nothing else in the suite depended on it not doing so. The new test that passes pre-fix asserts a selection surviving an undo, which the old code also did.Scope note
The issue proposes extracting a
commit(newTree)helper and routing all ~13 mutation sites through it, which would remove this bug class rather than this instance. I did not do that here: it is a much larger diff touching every mutating action, and I would rather it be reviewed deliberately than land as a side effect of a bug fix. The issue stays open-worthy on that point — happy to follow up with the refactor as its own PR if you want it.🤖 Generated with Claude Code