From 77a631e960b55ef1c563ad85f6d4fc5551e1c98c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:07:06 +0000 Subject: [PATCH 1/3] fix(theme): guard the theme-transition timer against document teardown `applyResolvedTheme` scheduled an unguarded 200ms timer that removed the `theme-transitioning` class. When a jsdom test file switched theme and finished inside that window, the callback ran after the environment was torn down and threw `ReferenceError: document is not defined` as an unhandled error. Vitest fails the whole run on an unhandled error even when every test passes, so the Unit coverage job reported: Test Files 648 passed (648) Tests 6970 passed (6970) Errors 1 error originating in tests/sidebar-production.dom.test.tsx, whose "switches Light, Dark, and Auto" case drives exactly that transition. Coverage instrumentation slows the run enough to widen the window, which is why it surfaced there rather than in the plain unit job. Two changes: - The callback returns early when `document` is gone, so a pending transition can never outlive the environment. - The timer handle is tracked and cleared before scheduling a new one. Rapid Light -> Dark -> Auto switching previously stacked one timer per change, letting an early callback strip the class while a later transition was still running. tests/theme-transition-timer.dom.test.tsx covers both. Verified red against the unfixed file: it reproduces `ReferenceError: document is not defined`, and the rapid-switch case fails too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx --- .../clinical-dashboard/use-theme.ts | 22 ++++- tests/theme-transition-timer.dom.test.tsx | 82 +++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 tests/theme-transition-timer.dom.test.tsx diff --git a/src/components/clinical-dashboard/use-theme.ts b/src/components/clinical-dashboard/use-theme.ts index b5d5562d8e..e1e87140a6 100644 --- a/src/components/clinical-dashboard/use-theme.ts +++ b/src/components/clinical-dashboard/use-theme.ts @@ -81,6 +81,23 @@ function syncThemeColorMetadata(theme: ResolvedTheme) { } } +// The pending transition timer must not outlive the document. A jsdom test file +// that switches theme can finish inside this 200ms window, and the unguarded +// callback then threw `ReferenceError: document is not defined` — an unhandled +// error that fails the entire Vitest run even when every test passed (seen on +// `tests/sidebar-production.dom.test.tsx` in the Unit coverage job, where the +// coverage instrumentation widened the window). Tracking one handle also stops +// rapid Light -> Dark -> Auto switching from stacking a timer per change, where +// an early callback could clear the class while a later transition was still +// running. +let themeTransitionTimer: number | null = null; + +function endThemeTransition() { + themeTransitionTimer = null; + if (typeof document === "undefined") return; + document.documentElement.classList.remove("theme-transitioning"); +} + function applyResolvedTheme(theme: ResolvedTheme) { const isCurrentlyDark = document.documentElement.classList.contains("dark"); const willBeDark = theme === "dark"; @@ -89,9 +106,8 @@ function applyResolvedTheme(theme: ResolvedTheme) { document.documentElement.classList.add("theme-transitioning"); document.documentElement.classList.toggle("dark", willBeDark); syncThemeColorMetadata(theme); - window.setTimeout(() => { - document.documentElement.classList.remove("theme-transitioning"); - }, 200); + if (themeTransitionTimer !== null) window.clearTimeout(themeTransitionTimer); + themeTransitionTimer = window.setTimeout(endThemeTransition, 200); } else { syncThemeColorMetadata(theme); } 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); + }); +}); From 36a5a2fcd3f963fadbcd06879ecd6903054d5283 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 17 Aug 2026 18:09:51 +0000 Subject: [PATCH 2/3] docs(ledger): record theme-transition timer fix review Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx --- ...4d7117683335cd7476a61ea84f765230826f4cd6d186cf0b95a.record.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/branch-review-records/534ba018669ad4d7117683335cd7476a61ea84f765230826f4cd6d186cf0b95a.record.md 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 | From 4d7d01ea144f30034f2e4f1fd9f692ef48ddcd00 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 01:42:38 +0000 Subject: [PATCH 3/3] refactor(theme): drop redundant timer fix, keep only the regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While this PR was open, a concurrent session landed an equivalent fix on main as 049760a ("fix(theme): clear and guard the theme-transition removal timer"), root-caused on PR #2052. Its implementation is functionally identical to the one here: same `typeof document === "undefined"` early return, same tracked handle cleared before scheduling. Keeping a second, cosmetically different version of the same fix would be pure churn and a conflict magnet, so use-theme.ts is reverted to main's version byte-for-byte. What main does NOT have is any regression test for this behaviour, so that is all this PR now carries. The test was re-verified against main's implementation rather than the one it was written for: it passes 3/3, and removing main's guard turns it red with the original `ReferenceError: document is not defined`. So it genuinely guards the code that shipped. This duplication is the failure mode tracked as outstanding issue #292 — two assistants building the same thing because neither checked the open PR list first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx --- .../clinical-dashboard/use-theme.ts | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/components/clinical-dashboard/use-theme.ts b/src/components/clinical-dashboard/use-theme.ts index 782f5d874b..d82bca7d07 100644 --- a/src/components/clinical-dashboard/use-theme.ts +++ b/src/components/clinical-dashboard/use-theme.ts @@ -81,23 +81,12 @@ function syncThemeColorMetadata(theme: ResolvedTheme) { } } -// The pending transition timer must not outlive the document. A jsdom test file -// that switches theme can finish inside this 200ms window, and the unguarded -// callback then threw `ReferenceError: document is not defined` — an unhandled -// error that fails the entire Vitest run even when every test passed (seen on -// `tests/sidebar-production.dom.test.tsx` in the Unit coverage job, where the -// coverage instrumentation widened the window). Tracking one handle also stops -// rapid Light -> Dark -> Auto switching from stacking a timer per change, where -// an early callback could clear the class while a later transition was still -// running. +// The transition class comes off on a short timer. Track the pending timer so a +// rapid second toggle replaces it instead of stacking removals, and bail out if +// it fires after the owning environment is gone — a leaked firing after DOM test +// teardown ("document is not defined") intermittently failed Unit coverage. let themeTransitionTimer: ReturnType | null = null; -function endThemeTransition() { - themeTransitionTimer = null; - if (typeof document === "undefined") return; - document.documentElement.classList.remove("theme-transitioning"); -} - function applyResolvedTheme(theme: ResolvedTheme) { const isCurrentlyDark = document.documentElement.classList.contains("dark"); const willBeDark = theme === "dark"; @@ -107,7 +96,11 @@ function applyResolvedTheme(theme: ResolvedTheme) { document.documentElement.classList.toggle("dark", willBeDark); syncThemeColorMetadata(theme); if (themeTransitionTimer !== null) clearTimeout(themeTransitionTimer); - themeTransitionTimer = setTimeout(endThemeTransition, 200); + themeTransitionTimer = setTimeout(() => { + themeTransitionTimer = null; + if (typeof document === "undefined") return; + document.documentElement.classList.remove("theme-transitioning"); + }, 200); } else { syncThemeColorMetadata(theme); }