test(theme): regression coverage for the theme-transition timer teardown guard - #2071
Conversation
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:38 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 98 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
…the timer teardown guard (PR #2071)
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYndHWrYJzirxbBvt68Tmx
Uh oh!
There was an error while loading. Please reload this page.
Summary
Scope changed while this PR was open — it is now test-only.
This PR originally carried a fix for an unguarded 200 ms timer in
use-theme.tsthat threwReferenceError: document is not definedafter jsdom teardown, failing a whole Vitest run while every test still passed. While it was open, a concurrent session landed an equivalent fix onmainas049760a("fix(theme): clear and guard the theme-transition removal timer", root-caused on #2052). Sametypeof document === "undefined"early return, same tracked handle cleared before scheduling.So
src/components/clinical-dashboard/use-theme.tshere has been reverted tomain's version byte-for-byte — shipping a second, cosmetically different copy of the same fix would be churn and a conflict magnet.What
maindoes not have is any regression test for this behaviour. That is all this PR now adds:tests/theme-transition-timer.dom.test.tsx(82 lines, 3 cases): the transition class is set and cleared on schedule; the pending callback does not throw when the document disappears mid-transition; and rapid Light → Dark → Auto switching keeps one pending timer so an early callback cannot end a later transition.Verification
npm run verify:cheapExit 0.
Test Files 649 passed (649)/Tests 6997 passed | 4 skipped (7001), noErrorsline.The test was re-verified against
main's implementation, not the one it was written for. This matters — a regression test that only guards code that never shipped is worthless:main'suse-theme.tsunmodified:Test Files 1 passed (1)/Tests 3 passed (3).main's guard line deleted:× does not throw when the document disappears before the timer fires—"ReferenceError: document is not defined",Tests 1 failed | 2 passed (3).So it genuinely guards the code that shipped.
npm run verify:uiUI verification not run: no runtime behaviour changes — the only non-test file is now identical to
main. Container also has the known Chromium revision drift (#255, has 1194, Playwright pins 1234).npm run check:production-readinessNot applicable:
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false.Risk and rollout
main.Notes
#292("two assistants built the same queued conversion twice because neither workflow checks the open-PR list before starting"). Both sessions independently root-caused the same flake within ~30 minutes of each other; theirs merged first. Worth noting the ledger item is not hypothetical.main.