diff --git a/frontend/src/lib/useUndoable.dom.test.ts b/frontend/src/lib/useUndoable.dom.test.ts index 65fca73b..88003698 100644 --- a/frontend/src/lib/useUndoable.dom.test.ts +++ b/frontend/src/lib/useUndoable.dom.test.ts @@ -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([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); + }); }); diff --git a/frontend/src/lib/useUndoable.ts b/frontend/src/lib/useUndoable.ts index d0766abd..56b89b38 100644 --- a/frontend/src/lib/useUndoable.ts +++ b/frontend/src/lib/useUndoable.ts @@ -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 { value: T; @@ -82,7 +83,8 @@ export function useUndoable( }, [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( // 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) => ({ + past: s.past.map(map), + present: map(s.present), + future: s.future.map(map), + }); + ref.current = over(ref.current); + setH(over); }, []); return {