feat(undo/redo): introduce clearUndoRedo in favor of store.clearStack#253
Conversation
|
@rainerhahnekamp Here is the PR for the |
|
Thanks @GregOnNet, marked as next review |
| }, | ||
| })), | ||
| withMethods((store) => ({ | ||
| /** @deprecated Use {@link clearUndoRedo} instead. */ |
There was a problem hiding this comment.
We should add here a note as well
/** * @deprecated Use {@link clearUndoRedo} instead. * Note: This method now performs a soft reset (keeps current state as baseline). * For hard reset behavior, useclearUndoRedo(store, { lastRecord: null }). */
There was a problem hiding this comment.
Pull request overview
This PR introduces a new clearUndoRedo function as the recommended way to clear undo/redo stacks, deprecating the existing store.clearStack() method. The enhancement adds support for setting a lastRecord option when clearing, allowing more granular control over the undo/redo state.
Key changes:
- New
clearUndoRedostandalone function with optionallastRecordparameter for better control - Refactored internal variable naming from
previoustolastRecordfor clarity - Deprecated
clearStack()method while maintaining backward compatibility
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| libs/ngrx-toolkit/src/lib/undo-redo/with-undo-redo.ts | Refactored to rename previous to lastRecord, added internal __clearUndoRedo__ method with options support, deprecated clearStack() |
| libs/ngrx-toolkit/src/lib/undo-redo/clear-undo-redo.ts | New file implementing the standalone clearUndoRedo function with type-safe options |
| libs/ngrx-toolkit/src/lib/undo-redo/clear-undo-redo.spec.ts | Test suite for the new clearUndoRedo function |
| libs/ngrx-toolkit/src/lib/undo-redo/with-undo-redo.spec.ts | Updated tests to use clearUndoRedo, added test coverage for lastRecord option |
| libs/ngrx-toolkit/src/index.ts | Updated exports to expose clearUndoRedo from the undo-redo module |
| docs/docs/with-undo-redo.md | Updated documentation to demonstrate the new clearUndoRedo usage pattern |
Comments suppressed due to low confidence (3)
libs/ngrx-toolkit/src/lib/undo-redo/with-undo-redo.ts:201
- The condition checks if redoStack.length exceeds maxStackSize, but then removes from undoStack instead. This appears to be a bug - it should check undoStack.length and should call shift() (not unshift()) to remove the oldest item.
libs/ngrx-toolkit/src/lib/undo-redo/with-undo-redo.ts:206 - Typo in comment: "propogate" should be "propagate"
libs/ngrx-toolkit/src/lib/undo-redo/with-undo-redo.ts:146 - When opts is not provided, lastRecord is not reset to null, which may cause unexpected behavior. Consider adding an else clause to set lastRecord = null when opts is undefined.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clearUndoRedo(this.store); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
We should add here a note about "Soft Reset (Default)" and "Hard Reset" and that we changed the behavior from Hard to soft.
|
@GregOnNet Thank you for the great PR! Just in the area of documentation I found two improvements - would be great if you could tackle them. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@GregOnNet thanks a lot. We will release v21 with this feature and backport it to v20 as well. |
…#253) [backport v19,v20] deprecates `clearStack` in favor of a new standalone function `clearUndoRedo`, which does a soft reset (not setting the state to null) by default. The hard reset can be set via options --------- Co-authored-by: Rainer Hahnekamp <rainer.hahnekamp@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…#253) [backport v19,v20] deprecates `clearStack` in favor of a new standalone function `clearUndoRedo`, which does a soft reset (not setting the state to null) by default. The hard reset can be set via options --------- Co-authored-by: Rainer Hahnekamp <rainer.hahnekamp@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…#253) [backport v19,v20] deprecates `clearStack` in favor of a new standalone function `clearUndoRedo`, which does a soft reset (not setting the state to null) by default. The hard reset can be set via options --------- Co-authored-by: Rainer Hahnekamp <rainer.hahnekamp@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
#210