Repository navigation
fix: getPrevious after array push restores the previous array length (undo after push) - #668
Open
huytdps13400 wants to merge 1 commit into
Open
huytdps13400 wants to merge 1 commit into
huytdps13400 wants to merge 1 commit into
Conversation
Reverting a change that appended an element to an array (like push) set the new index to undefined instead of removing it, so getPrevious() returned an array with a trailing undefined. undoRedo cloned that into a trailing null. Remove the appended element instead, and revert changes in reverse order so multiple pushes in one batch and child-then-parent changes revert correctly. Fixes LegendApp#397 Co-Authored-By: Claude Opus 5.5 <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 #397
Root cause
push()with a single item takes the fast path incollectionSetter, so the change is recorded at the new index withprevAtPath: undefined. WhenapplyChanges(..., applyPrevious = true)reverts that change,setAtPathwritesundefinedto the index and leaves it in the array. As a resultgetPrevious()returns[...prev, undefined].undoRedoclones that value through JSON and gets[...prev, null], which is why undoing a push leaves anullbehind.Change
In
src/helpers.ts:applyChange: when reverting a change whose previous value isundefinedand whose target is the last index of an array, the element is removed instead of set toundefined.applyChanges: whenapplyPreviousis set, changes are reverted in reverse order. Without this, two pushes in one batch still left oneundefined. Reversing also fixes a batch that changes a child and then its parent (b.c.set(1); b.set({ c: 5 })). Before this change,getPrevious()there returned{ b: { c: 1 } }instead of{ b: { c: 0 } }.getPrevious,revertChangesand the crud update diff are the only callers ofapplyChanges(..., true). The forward (non-previous) path is unchanged.Verification
New tests:
tests/history.test.ts: "Undo/Redo with array push" and "Undo/Redo with array push in a batch"tests/tests.test.ts: "Array push getPrevious has the previous length" and "getPrevious with child and parent changes in a batch"The new tests use
toStrictEqualon purpose. Jest'stoEqualtreats[a, undefined]as equal to[a], which is why the existing "Array objects push getPrevious" test missed this bug.[null]instead of[].npx jestsuite passes (1085 tests), along withtsc --noEmit,lint:checkandformat:check. Theperf.test.tstiming tests flake on my machine under load, and they flake the same way on a cleanmain.One edge case: if an array really ended in an
undefinedelement that was then set to a value, the reverted array is now one element shorter. Objects already behave this way, since a reverted key comes back asundefined.This fix was prepared with AI assistance (Claude Code) and verified locally with the tests above.
🤖 Generated with Claude Code