diff --git a/src/hooks/useUndoBanners.test.tsx b/src/hooks/useUndoBanners.test.tsx index 10c1e1b2..7d4446d1 100644 --- a/src/hooks/useUndoBanners.test.tsx +++ b/src/hooks/useUndoBanners.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { act, renderHook } from "@testing-library/react"; +import { StrictMode } from "react"; import { useUndoBanners, DEFAULT_UNDO_DURATION_MS } from "./useUndoBanners"; beforeEach(() => { @@ -11,6 +12,94 @@ afterEach(() => { }); describe("useUndoBanners", () => { + it("flushes every pending action once in StrictMode and skips already committed actions", () => { + const pending = vi.fn(); + const dismissed = vi.fn(); + const expired = vi.fn(); + const { result, unmount } = renderHook(() => useUndoBanners(), { wrapper: StrictMode }); + act(() => { + result.current.registerUndo({ message: "Expires", onUndo: vi.fn(), onCommit: expired, durationMs: 100 }); + const id = result.current.registerUndo({ message: "Dismissed", onUndo: vi.fn(), onCommit: dismissed }); + result.current.dismiss(id); + result.current.registerUndo({ message: "Pending A", onUndo: vi.fn(), onCommit: pending }); + result.current.registerUndo({ message: "Pending B", onUndo: vi.fn(), onCommit: pending }); + result.current.registerUndo({ message: "Already persisted", onUndo: vi.fn() }); + vi.advanceTimersByTime(100); + }); + expect(pending).not.toHaveBeenCalled(); + unmount(); + expect(pending).toHaveBeenCalledTimes(2); + expect(dismissed).toHaveBeenCalledTimes(1); + expect(expired).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + }); + + it("does not schedule timers or render while empty, including empty bulk actions", () => { + const render = vi.fn(); + const { result } = renderHook(() => { + render(); + return useUndoBanners(); + }); + const initial = result.current.banners; + const renders = render.mock.calls.length; + expect(vi.getTimerCount()).toBe(0); + act(() => { + vi.advanceTimersByTime(1000); + result.current.undoAll(); + result.current.dismissAll(); + }); + expect(render).toHaveBeenCalledTimes(renders); + expect(result.current.banners).toBe(initial); + }); + + it("runs one ticker while pending and stops it after expiry", () => { + const { result } = renderHook(() => useUndoBanners()); + act(() => { + result.current.registerUndo({ message: "A", onUndo: vi.fn(), durationMs: 1000 }); + result.current.registerUndo({ message: "B", onUndo: vi.fn(), durationMs: 1000 }); + }); + expect(vi.getTimerCount()).toBe(1); + act(() => vi.advanceTimersByTime(300)); + expect(result.current.banners.map(banner => banner.remainingMs)).toEqual([700, 700]); + act(() => vi.advanceTimersByTime(700)); + expect(result.current.banners).toEqual([]); + expect(vi.getTimerCount()).toBe(0); + const empty = result.current.banners; + act(() => vi.advanceTimersByTime(1000)); + expect(result.current.banners).toBe(empty); + }); + + it("flushes pending commits once on unmount without committing undone actions", () => { + const onCommit = vi.fn(); + const undoneCommit = vi.fn(); + const onUndo = vi.fn(); + const { result, unmount } = renderHook(() => useUndoBanners()); + act(() => { + result.current.registerUndo({ message: "Pending", onUndo: vi.fn(), onCommit }); + const id = result.current.registerUndo({ message: "Undone", onUndo, onCommit: undoneCommit }); + result.current.undo(id); + }); + unmount(); + expect(onCommit).toHaveBeenCalledTimes(1); + expect(onUndo).toHaveBeenCalledTimes(1); + expect(undoneCommit).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + act(() => vi.advanceTimersByTime(10000)); + expect(onCommit).toHaveBeenCalledTimes(1); + }); + + it.each(["undo", "dismiss", "undoAll", "dismissAll"] as const)("stops and restarts the ticker after %s", (method) => { + const { result } = renderHook(() => useUndoBanners()); + let id = ""; + act(() => { id = result.current.registerUndo({ message: "A", onUndo: vi.fn() }); }); + act(() => result.current[method](id)); + expect(vi.getTimerCount()).toBe(0); + act(() => { result.current.registerUndo({ message: "B", onUndo: vi.fn() }); }); + expect(vi.getTimerCount()).toBe(1); + act(() => vi.advanceTimersByTime(100)); + expect(result.current.banners[0].remainingMs).toBe(4900); + }); + it("registers a banner with the default duration", () => { const { result } = renderHook(() => useUndoBanners()); diff --git a/src/hooks/useUndoBanners.ts b/src/hooks/useUndoBanners.ts index 2b4d6a83..5c936cb1 100644 --- a/src/hooks/useUndoBanners.ts +++ b/src/hooks/useUndoBanners.ts @@ -4,7 +4,8 @@ * State manager for the Undo banner pattern (Issue #162). It owns a stack of * pending reversible actions, each with a countdown to permanence. When a * countdown elapses (or the user dismisses the banner) the action is committed; - * if the user presses "Undo" the action is reversed instead. + * if the user presses "Undo" the action is reversed instead. Unmounting commits + * all still-pending actions because their undo controls are no longer available. * * The hook is the single source of truth for timing so the presentational * `` stays a pure render of the current stack. @@ -37,8 +38,8 @@ export interface RegisterUndoOptions { onUndo: () => void; /** * Make the action permanent. Called when the countdown elapses or the user - * dismisses the banner. Optional — omit when the action is already persisted - * and Undo is the only thing that changes state. + * dismisses the banner, or the hook unmounts. Optional — omit when the action + * is already persisted and Undo is the only thing that changes state. */ onCommit?: () => void; /** Label for the Undo button. Defaults to "Undo". */ @@ -85,6 +86,15 @@ export interface UseUndoBannersResult { export function useUndoBanners(): UseUndoBannersResult { const [banners, setBanners] = useState([]); const entriesRef = useRef>(new Map()); + const visibleRef = useRef(banners); + const intervalRef = useRef(null); + + const stopTicker = useCallback(() => { + if (intervalRef.current !== null) { + window.clearInterval(intervalRef.current); + intervalRef.current = null; + } + }, []); const sync = useCallback(() => { const now = performance.now(); @@ -112,14 +122,35 @@ export function useUndoBanners(): UseUndoBannersResult { entry.onCommit?.(); } - setBanners(visible); - }, []); + // Avoid even scheduling a state update for an unchanged snapshot. + const previous = visibleRef.current; + if (previous.length !== visible.length || visible.some((entry, index) => + entry.id !== previous[index].id || entry.remainingMs !== previous[index].remainingMs + )) { + visibleRef.current = visible; + setBanners(visible); + } - // A single shared ticker keeps every countdown in step. + if (entriesRef.current.size === 0) { + stopTicker(); + } else if (intervalRef.current === null) { + intervalRef.current = window.setInterval(sync, TICK_MS); + } + }, [stopTicker]); + + // No timer is needed until an action is registered. Detach pending entries + // before invoking callbacks so cleanup cannot commit an action twice. useEffect(() => { - const interval = window.setInterval(sync, TICK_MS); - return () => window.clearInterval(interval); - }, [sync]); + const entries = entriesRef.current; + return () => { + stopTicker(); + const pending = Array.from(entries.values()).reverse(); + entries.clear(); + for (const entry of pending) { + entry.onCommit?.(); + } + }; + }, [stopTicker]); const registerUndo = useCallback( (options: RegisterUndoOptions): string => {