diff --git a/src/components/ui/sheet.tsx b/src/components/ui/sheet.tsx index 2e60c7f72b..06c165f4ac 100644 --- a/src/components/ui/sheet.tsx +++ b/src/components/ui/sheet.tsx @@ -4,6 +4,7 @@ import { useEffect, useCallback, useId, + useLayoutEffect, useRef, type CSSProperties, type PointerEvent as ReactPointerEvent, @@ -24,6 +25,8 @@ import { type SheetFocusController, } from "@/components/ui/sheet-focus"; +const useIsomorphicLayoutEffect = typeof window === "undefined" ? useEffect : useLayoutEffect; + export type SheetMobileSize = "content" | "viewport"; export type SheetMobileHeaderSafeArea = "none" | "padding" | "offset"; @@ -197,7 +200,11 @@ export function Sheet({ onClose(); }, [dismissOnBack, onClose, sheetId]); - useEffect(() => { + // Layout effect, not a passive one: the Close button, swipe and back handlers + // read these refs, and a click that lands after a commit but before passive + // effects flush would otherwise call the previous render's onClose (a stale + // closure — e.g. a sheet that had just saved would skip its own reset). + useIsomorphicLayoutEffect(() => { onCloseRef.current = onClose; requestCloseRef.current = requestClose; }, [onClose, requestClose]); diff --git a/tests/sheet.dom.test.tsx b/tests/sheet.dom.test.tsx index 197805101e..55d4a94e5b 100644 --- a/tests/sheet.dom.test.tsx +++ b/tests/sheet.dom.test.tsx @@ -1,4 +1,4 @@ -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; @@ -325,3 +325,54 @@ describe("Sheet stacked-overlay coordination", () => { await waitFor(() => expect(screen.getByRole("button", { name: "Updated return target" })).toHaveFocus()); }); }); + +describe("Sheet close handler freshness", () => { + it("calls the latest onClose when Close is clicked right after a commit, before passive effects flush", async () => { + // Reproduces the admin-renewed-sheet flake: an async save re-renders the + // sheet with a new onClose, and a click that lands between that commit and + // the passive-effect flush must not reach the previous render's handler. + const { createRoot } = await import("react-dom/client"); + const previousActEnvironment = (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT; + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = false; + const calls: number[] = []; + const bump: { current: (() => void) | null } = { current: null }; + function Harness() { + const [n, setN] = useState(0); + useEffect(() => { + bump.current = () => setN(1); + }, []); + return ( + calls.push(n)} title="Freshness sheet" dismissOnBack={false}> +

{n}

+
+ ); + } + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + try { + root.render(); + for (let i = 0; i < 50 && !document.querySelector('[data-testid="freshness-n"]'); i += 1) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + const label = document.querySelector('[data-testid="freshness-n"]')!; + await new Promise((resolve) => setTimeout(resolve, 50)); + const clickedAfterCommit = new Promise((resolve) => { + const observer = new MutationObserver(() => { + if (label.textContent !== "1") return; + observer.disconnect(); + (document.querySelector('button[aria-label="Close"]') as HTMLButtonElement).click(); + resolve(); + }); + observer.observe(label, { childList: true, characterData: true, subtree: true }); + }); + setTimeout(() => bump.current?.(), 0); + await clickedAfterCommit; + expect(calls).toEqual([1]); + } finally { + root.unmount(); + container.remove(); + (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = previousActEnvironment; + } + }); +});