fix(ui): an undo in the same tick as a rebase steps back to a rebased snapshot
rebase updated the state only, so the mirror the callbacks read still held the removed row and an undo right after it wrote that row back for a moment.
This commit is contained in:
@@ -26,4 +26,15 @@ describe("useUndoable", () => {
|
||||
act(() => result.current.undo());
|
||||
expect(result.current.value).toEqual([2]);
|
||||
});
|
||||
|
||||
it("undoes to a rebased snapshot when the undo comes in the same tick as the rebase", () => {
|
||||
const { result } = renderHook(() => useUndoable<number[]>([1, 2]));
|
||||
act(() => result.current.set([2, 1]));
|
||||
act(() => {
|
||||
result.current.rebase((xs) => xs.filter((x) => x !== 2)); // a row removed
|
||||
result.current.undo();
|
||||
});
|
||||
expect(result.current.value).toEqual([1]);
|
||||
expect(result.current.canRedo).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -11,7 +11,8 @@ import { useLatest } from "./useLatest";
|
||||
//
|
||||
// Not playlist-specific: any component with an undoable value (sort order, a draft, a set
|
||||
// of filters…) can use it. The history is state (what render reads) mirrored in a ref (what the
|
||||
// stable callbacks read, so two calls in one tick chain on each other, never on a stale snapshot).
|
||||
// stable callbacks read, so two calls in one tick chain on each other, never on a stale snapshot —
|
||||
// save after a `reset`, which is state only and reaches the mirror at its commit; see `reset`).
|
||||
// Render never reads the ref: a compiled component would otherwise memoize a stale value (E32).
|
||||
export interface Undoable<T> {
|
||||
value: T;
|
||||
@@ -82,7 +83,8 @@ export function useUndoable<T>(
|
||||
}, [commit, onApplyRef]);
|
||||
|
||||
// State only: safe to call during render. `rebase` builds on it at once; `set`/`undo`/`redo` read
|
||||
// the mirror, so they see it from the next commit on (no caller acts in the tick of a reset).
|
||||
// the mirror, so they see it from the next commit on (no caller acts in the tick of a reset — a
|
||||
// set, undo or redo in that tick would commit the history from before it).
|
||||
const reset = useCallback((value: T) => {
|
||||
setH({ past: [], present: value, future: [] });
|
||||
}, []);
|
||||
@@ -92,11 +94,20 @@ export function useUndoable<T>(
|
||||
// removing a playlist item is not itself undoable, yet dropping it from each stored order keeps
|
||||
// the user's reorder history valid and usable instead of throwing it away (which is what a
|
||||
// `reset` does). No `onApply`: the caller is already persisting the change by its own route.
|
||||
// An updater, not the mirror: a `reset` earlier in the same tick is state only, so the mirror
|
||||
// The STATE through an updater: a `reset` earlier in the same tick is state only, so the mirror
|
||||
// still held the history before it, and rebasing that wrote the old value back over the reset (a
|
||||
// cached playlist remounted under StrictMode re-seeds with reset-then-rebase and painted empty).
|
||||
// The mirror is rebased too, so an `undo` later in the same tick steps back to a rebased snapshot,
|
||||
// not to one that still holds the removed row (E32 review round 2). Called from handlers and
|
||||
// effects only, never during render.
|
||||
const rebase = useCallback((map: (value: T) => T) => {
|
||||
setH((s) => ({ past: s.past.map(map), present: map(s.present), future: s.future.map(map) }));
|
||||
const over = (s: History<T>) => ({
|
||||
past: s.past.map(map),
|
||||
present: map(s.present),
|
||||
future: s.future.map(map),
|
||||
});
|
||||
ref.current = over(ref.current);
|
||||
setH(over);
|
||||
}, []);
|
||||
|
||||
return {
|
||||
|
||||
Reference in New Issue
Block a user