diff --git a/docs/branch-review-records/534ba018669ad4d7117683335cd7476a61ea84f765230826f4cd6d186cf0b95a.record.md b/docs/branch-review-records/534ba018669ad4d7117683335cd7476a61ea84f765230826f4cd6d186cf0b95a.record.md new file mode 100644 index 0000000000..97834ba153 --- /dev/null +++ b/docs/branch-review-records/534ba018669ad4d7117683335cd7476a61ea84f765230826f4cd6d186cf0b95a.record.md @@ -0,0 +1 @@ +| 2026-08-17 | claude/fix-theme-transition-timer-race | 77a631e960b55ef1c563ad85f6d4fc5551e1c98c | theme-transition timer race in use-theme.ts causing Vitest unhandled-error failures | Fixed: guarded the 200ms theme-transitioning callback against a torn-down document and tracked/cleared the timer handle so rapid switches cannot end a later transition early | verify:pr-local exit 0 (649 files/6968 tests, no Errors line); red-green proved — new spec reproduces ReferenceError: document is not defined against the unfixed file (2 failed), passes 3/3 with the fix | diff --git a/tests/theme-transition-timer.dom.test.tsx b/tests/theme-transition-timer.dom.test.tsx new file mode 100644 index 0000000000..1271f6a65b --- /dev/null +++ b/tests/theme-transition-timer.dom.test.tsx @@ -0,0 +1,82 @@ +/** @vitest-environment jsdom */ + +import { act, renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { useTheme } from "@/components/clinical-dashboard/use-theme"; +import { THEME_STORAGE_KEY } from "@/lib/theme"; + +/** + * Guards the 200ms `theme-transitioning` timer in use-theme.ts. + * + * The timer used to run unguarded. A jsdom test file that switched theme could + * finish inside the window, and the callback then threw + * `ReferenceError: document is not defined` as an *unhandled* error — which + * fails an entire Vitest run while still reporting every test as passed. That is + * exactly how the Unit coverage job went red on PR #2046 with + * `Test Files 648 passed / Tests 6970 passed / Errors 1 error`, originating in + * `tests/sidebar-production.dom.test.tsx`. + */ +describe("theme transition timer", () => { + beforeEach(() => { + vi.useFakeTimers(); + try { + window.localStorage.removeItem(THEME_STORAGE_KEY); + } catch { + // Storage blocked in this environment; the hook falls back to memory. + } + document.documentElement.classList.remove("dark", "theme-transitioning"); + }); + + afterEach(() => { + vi.useRealTimers(); + document.documentElement.classList.remove("dark", "theme-transitioning"); + }); + + it("marks the transition and clears it when the timer fires", () => { + const { result } = renderHook(() => useTheme()); + + act(() => result.current.setPreference("dark")); + expect(document.documentElement.classList.contains("dark")).toBe(true); + expect(document.documentElement.classList.contains("theme-transitioning")).toBe(true); + + act(() => void vi.advanceTimersByTime(200)); + expect(document.documentElement.classList.contains("theme-transitioning")).toBe(false); + }); + + it("does not throw when the document disappears before the timer fires", () => { + const { result } = renderHook(() => useTheme()); + act(() => result.current.setPreference("dark")); + expect(document.documentElement.classList.contains("theme-transitioning")).toBe(true); + + // Reproduce environment teardown mid-transition: the pending callback runs + // with no `document` in scope, which is what threw before the guard. + const realDocument = globalThis.document; + Reflect.deleteProperty(globalThis, "document"); + try { + expect(() => vi.advanceTimersByTime(200)).not.toThrow(); + } finally { + Object.defineProperty(globalThis, "document", { + value: realDocument, + configurable: true, + writable: true, + }); + } + }); + + it("keeps one pending timer across rapid switches so an early one cannot end a later transition", () => { + const { result } = renderHook(() => useTheme()); + + act(() => result.current.setPreference("dark")); + act(() => void vi.advanceTimersByTime(150)); + act(() => result.current.setPreference("light")); + + // Without the shared handle the first timer fires here and strips the class + // while the second transition still has 140ms to run. + act(() => void vi.advanceTimersByTime(60)); + expect(document.documentElement.classList.contains("theme-transitioning")).toBe(true); + + act(() => void vi.advanceTimersByTime(140)); + expect(document.documentElement.classList.contains("theme-transitioning")).toBe(false); + }); +});