fix(site): flush deferred Radix focus-scope timer before jsdom teardown#26983
Draft
EhabY wants to merge 2 commits into
Draft
fix(site): flush deferred Radix focus-scope timer before jsdom teardown#26983EhabY wants to merge 2 commits into
EhabY wants to merge 2 commits into
Conversation
Radix's FocusScope defers its unmount event dispatch and focus restore with setTimeout(0). The cleanup() in the shared afterEach schedules that timer, and for the last test in a file it could still be pending when vitest tears down the jsdom environment. The callback then constructs a CustomEvent from Node's built-in constructor instead of jsdom's, and jsdom rejects the dispatch with "parameter 1 is not of type 'Event'" as an unhandled error, failing the run. Await one timer turn in afterAll, while the jsdom environment is still alive, to deterministically drain the 0ms timers scheduled by cleanup(). Timers with the same delay run in FIFO order, so this is not a race. Fixes coder/internal#1613
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
UserDropdownContent.test.tsxintermittently fails CI with an unhandled error during teardown:Radix's
FocusScopedefers its unmount event dispatch and focus restore withsetTimeout(0)(a react#17894 workaround). Thecleanup()call in the sharedafterEachunmounts the tree and schedules that timer. For the last test in a file there is no next test to absorb it, so the timer can still be pending when vitest tears down the per-file jsdom environment. It then fires with Node's built-inCustomEventinstead of jsdom's, and jsdom rejects the dispatch, failing the run. Any test file whose last test unmounts a focus-trapped Radix component (Dialog, Popover, DropdownMenu) can hit this race.Fix
Await one timer turn in the shared
afterAll, after all tests andafterEachhooks but before environment teardown. Same-delay timers run in FIFO order, so this deterministically drains every 0ms timer scheduled bycleanup()while the jsdom realm is still alive. Cost is ~1ms per test file. Avi.isFakeTimers()guard prevents the flush from hanging if a test file leaves fake timers installed.Closes coder/internal#1613
Investigation notes and validation
Failure chain
<DropdownMenu defaultOpen>, mounting a focus-trapped RadixFocusScope.afterEachvia RTLcleanup().FocusScope's effect cleanup schedulessetTimeout(0)which constructs aCustomEventand dispatches it on the (now detached) container.CustomEventresolves to Node's built-in (Node >= 19). Dispatching a Node-realm event on a jsdom element fails jsdom's brand check, producing the unhandledTypeError. Timing-dependent, hence flaky.The commit blamed in the issue (adding a third test to the file) only shifted timing; the race predates it.
Why
afterAlland notafterEachThe crash requires environment teardown, which happens exactly once per file, after all
afterEachandafterAllhooks. Flushing once per file is positioned exactly in the gap and avoids paying ~1ms per test. Vitest's defaultsequence.hooks: "stack"runs setup-fileafterAllhooks last, so test files' ownafterAllhooks complete before the flush.A microtask wait (
await Promise.resolve()) would not work:setTimeoutcallbacks run in the timer phase after all microtasks.Alternatives considered
afterEach: same mechanism, stricter isolation, but pays the cost per test and the cross-test leakage it prevents is today's status quo and not the observed flake.unmount()+ flush): whack-a-mole; the race applies to every Radix-based test file.@radix-ui/react-focus-scope: lowers test fidelity for focus behavior.Validation: A/B flake reproduction, main vs. this fix
Methodology: 20 copies of
UserDropdownContent.test.tsxper vitest invocation (20 jsdom env teardowns per run, i.e. 20 race opportunities), alternating main's and this branch'stest/setup/msw.tson every iteration to cancel out machine-load drift. Failure = vitest reports an unhandled error.tasksetpinned to 2 cores), 12 invocations each (240 teardowns)Every main failure has the byte-identical signature from the CI flake (
Event.js:22brand check viareact-focus-scope/dist/index.mjs:92:21 Timeout._onTimeout); under starvation several of the 20 files fail per invocation. The fix produced zero unhandled errors across all ~1440 teardowns, including the starved round, which is expected: the flush does not shrink the race window, it removes the pending timer entirely.Also: full unit project green (198 files, 3026 tests, 25.96s),
biome checkandtsc -p .clean. The pre-existingclose timed out after 1000msmessage is unrelated (rolldown worker threads;teardownTimeout: 1000is deliberate per the vite config comment).This PR was generated by Coder Agents on behalf of @EhabY, investigating coder/internal#1613.