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.
| ); | ||
| }); | ||
|
|
||
| it('should not throw no error if the store is configured with withUndoRedo()', () => { |
There was a problem hiding this comment.
Double negative in test description: "should not throw no error" should be "should not throw an error"
| it('should not throw no error if the store is configured with withUndoRedo()', () => { | |
| it('should not throw an error if the store is configured with withUndoRedo()', () => { |
| 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. |
3 participants
#210