fix(Toast): clear auto-close timer on unmount (jsdom teardown crash)#1091
fix(Toast): clear auto-close timer on unmount (jsdom teardown crash)#1091Firefds wants to merge 3 commits into
Conversation
Radix Toast v1.2.2 never clears its internal `closeTimerRef` on unmount, which leaks the auto-close `setTimeout` past the host's lifetime. In jsdom-based test runs the leftover timer fires after vitest tears down the test file's DOM, and `handleClose` -> `document.activeElement` throws `ReferenceError: document is not defined`, aborting the whole suite. (Upstream fix: radix-ui/primitives#3794, still unmerged.) Side-step it inside click-ui itself: pass `duration={Infinity}` to the underlying Radix `Toast.Root` (which short-circuits its `startTimer` so no real timeout is scheduled) and own the auto-close timer in our wrapper with a `useEffect` cleanup. Radix's public `onPause`/`onResume` callbacks are wired through so viewport-level pause-on-blur/focus behavior is preserved. Co-authored-by: Shauli Bracha <shauli.bracha@clickhouse.com>
|
|
|
Firefds
requested review from
DreaminDani,
Copilot,
serdec and
vineethasok
and removed request for
serdec and
vineethasok
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a downstream workaround in Click UI’s Toast wrapper to prevent a Radix Toast auto-close timer from leaking past unmount, which can crash jsdom-based test teardown. It does so by disabling Radix’s internal duration timer and managing the auto-close lifecycle within Click UI, plus adds a small test suite to guard against regressions.
Changes:
- Implement internal auto-close timer management in
Toastand clear it on unmount; wire RadixonPause/onResumeto pause/resume the internal timer. - Force
duration={Infinity}on the underlying Radix toast to avoid Radix’s internal timer scheduling. - Add
Toast.test.tsxwith auto-close, unmount cleanup regression, andInfinityduration behavior tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/components/Toast/Toast.tsx | Moves auto-close timing responsibility into Click UI and disables Radix’s internal duration timer. |
| src/components/Toast/Toast.test.tsx | Adds unit tests validating auto-close timing and ensuring no timers remain after unmount. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const Toast = ({ | ||
| type, | ||
| toastType = 'foreground', | ||
| title, | ||
| description, | ||
| actions = [], | ||
| duration, | ||
| duration = DEFAULT_TOAST_DURATION, | ||
| onClose, |
| const handleResume = useCallback(() => { | ||
| startCloseTimer(closeTimerRemainingTimeRef.current); | ||
| }, [startCloseTimer]); |
| it('clears the auto-close timer when the toast unmounts (regression for radix-ui/primitives#3703)', () => { | ||
| const onClose = vi.fn(); | ||
| const { unmount } = renderCUI( | ||
| <Toast | ||
| title="hello" | ||
| duration={5000} | ||
| onClose={onClose} | ||
| /> | ||
| ); | ||
|
|
||
| act(() => { | ||
| vi.advanceTimersByTime(1000); | ||
| }); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
|
|
||
| unmount(); | ||
|
|
||
| // No pending timers must remain after unmount. Radix v1.2.2 leaks its | ||
| // internal `closeTimerRef` (the auto-close `setTimeout`) here because it | ||
| // never `clearTimeout`s it on unmount. The leftover timer later fires | ||
| // after vitest tears down jsdom and calls `handleClose`, which accesses | ||
| // `document.activeElement` -- producing | ||
| // `ReferenceError: document is not defined` and aborting the whole test | ||
| // file. We side-step this by passing `duration={Infinity}` to Radix and | ||
| // owning the timer in click-ui itself with a cleanup effect. | ||
| expect(vi.getTimerCount()).toBe(0); | ||
|
|
||
| // And nothing left behind should be able to invoke our `onClose` later. | ||
| act(() => { | ||
| vi.advanceTimersByTime(60_000); | ||
| }); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| }); |
The previous commit put `onClose` in the dep array of `startCloseTimer`
(and therefore of the auto-close `useEffect`). `ToastProvider` produces
a new `onClose={onClose(id)}` closure on every render, so any provider
update (e.g. another toast appearing) would re-run the effect, reset
`closeTimerRemainingTimeRef` to the full duration, and discard pause
progress -- effectively letting a frequently-rerendering parent keep a
toast alive indefinitely.
Mirror Radix's own `useCallbackRef` pattern: keep a ref to the latest
`onClose` (updated in an effect), have `startCloseTimer` invoke
`onCloseRef.current` instead of capturing `onClose` directly, and drop
`onClose` from its deps. The effect now only re-runs when `duration`
actually changes.
Added a regression test that re-renders the toast with a fresh `onClose`
identity mid-countdown and asserts the original timer still fires at the
original deadline (test fails before this commit, passes after).
Co-authored-by: Shauli Bracha <shauli.bracha@clickhouse.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 177d109. Configure here.
| duration={duration} | ||
| duration={Infinity} | ||
| onPause={handlePause} | ||
| onResume={handleResume} |
There was a problem hiding this comment.
Props override pause handlers
Low Severity
ToastRoot sets onPause and onResume for the internal auto-close timer, then spreads {...props} afterward. ToastProps inherits Radix onPause/onResume, so values from createToast or {...toast} replace those handlers. The click-ui timer keeps running on hover, blur, and viewport pause, breaking documented Radix pause behavior for those toasts.
Reviewed by Cursor Bugbot for commit 177d109. Configure here.
…eadline, baseline-relative leak test)
Addresses three issues raised in PR review:
1) Provider-level duration (Copilot, Toast.tsx:132)
Previously, an unset `ToastProps.duration` let Radix's
`ToastProvider duration` (the default for every toast) take effect.
After moving the timer into our wrapper, defaulting `duration` to
`5000` at the `Toast` component boundary silently overrode any
consumer-supplied `<ToastProvider duration={...}>` / `config.toast`
value. Add an internal `ToastDurationContext` populated by
`ToastProvider` (still defaulting to 5000), and resolve in `Toast`
as `toast.duration ?? providerDuration ?? DEFAULT_TOAST_DURATION` --
restoring the original prop > provider > default resolution order.
2) Resume-after-deadline close (Copilot, Toast.tsx:211)
If `handlePause` ran after the auto-close deadline had already
passed (event-loop delay / timer throttling beat the close timer to
the punch), remaining time would clamp to 0 and `handleResume ->
startCloseTimer(0)` short-circuited, leaving the toast open
indefinitely. Track `wasPausedRef` so resume can distinguish
"never paused" from "paused with deadline already elapsed" and
call `onClose(false)` immediately in the latter case.
3) Baseline-relative leak detection (Copilot, Toast.test.tsx:67)
Replaced the brittle `getTimerCount() === 0` assertion with a
semantic check: stub `Document.prototype.activeElement` and
advance time past the leaked deadline. If Radix's auto-close
handler ever fires post-unmount it dereferences
`document.activeElement`, which now flips a boolean we assert
against. Insensitive to unrelated Radix/styled-components internal
timer counts.
Added two new regression tests covering #1 and #2; both fail on `main`
without the corresponding fix and pass with it.
Co-authored-by: Shauli Bracha <shauli.bracha@clickhouse.com>
Storybook Preview Deployed✅ Preview URL: https://click-84yry3toy-clickhouse.vercel.app Built from commit: |
DreaminDani
left a comment
DreaminDani
left a comment
There was a problem hiding this comment.
Feels like a lot of tests for one small change but otherwise LGTM
Why?
Radix
@radix-ui/react-toast@1.2.2(and the latest published1.2.17, verified by diffingToastImpl) never clears its internalcloseTimerRefwhen it unmounts. The leftoversetTimeoutfires after the host has gone away — in jsdom-based test runs, vitest tears down the file's DOM before the timer fires, then Radix'shandleClosedoesnode?.contains(document.activeElement)and crashes withReferenceError: document is not defined, aborting the entire test suite for that file.Upstream fix is open but unmerged: radix-ui/primitives#3794 (tracking issue: radix-ui/primitives#3703). When that ships and we bump our
@radix-ui/react-toastdep, this wrapper-side workaround can be reverted.How?
Take ownership of the auto-close timer inside click-ui's
Toastwrapper instead of relying on Radix's internal one:duration={Infinity}to the underlyingRadixUIToast.Root. Radix'sstartTimershort-circuits onInfinity(if (!duration2 || duration2 === Infinity) return;), so no realsetTimeoutis ever scheduled inside Radix — no leak possible.closeTimerRefourselves withuseRef+setTimeout, and clear it from theuseEffectcleanup, so the timer is guaranteed to be cancelled on unmount.onPause/onResume(public Radix props) to pause/resume our own timer. This preserves the existing viewport-level pause-on-blur / pause-on-hover behaviour for production consumers.durationastoast.duration ?? providerDuration ?? DEFAULT_TOAST_DURATIONvia a new internalToastDurationContextpopulated byToastProvider. This preserves the previous prop > provider > default precedence so<ToastProvider duration={...}>/<ClickUIProvider config={{ toast: { duration } }}>still works.onClosebehind a stable callback ref (the sameuseCallbackRefpattern Radix uses internally), so the auto-closeuseEffectdoes not re-run when the parent passes a newonCloseidentity. This matters becauseToastProviderproduces a newonClose={onClose(id)}closure on every render — without the ref, every provider update (e.g. another toast appearing) would restart the countdown from the full duration and discard pause progress, letting a frequently-rerendering parent keep a toast alive indefinitely.wasPausedRefso thathandleResumecan distinguish "never paused" from "paused with deadline already elapsed". In the latter case (event-loop delay / timer throttling beat the close timer to the punch),handleResumenow closes immediately instead of callingstartCloseTimer(0)and short-circuiting forever.Added
src/components/Toast/Toast.test.tsxwith six cases, four of them targeted regression tests:does not leak the auto-close timer past unmount (regression for radix-ui/primitives#3703)— stubsDocument.prototype.activeElementand advances time past the leaked deadline. If Radix's auto-close handler ever fires post-unmount it dereferencesdocument.activeElement, tripping the stub. Asserts the stub was never tripped, which is insensitive to unrelated Radix / styled-components internal timer counts.does not reset the auto-close timer when the parent re-renders with a new onClose identity— re-renders with a freshonClosemid-countdown and asserts the original deadline still fires.falls back to the provider-level duration when a toast does not set its own— creates a toast viauseToast().createToastunder a<ClickUIProvider config={{ toast: { duration: 2000 } }}>and asserts the toast disappears just past 2000 ms (not the 5000 ms hard default).closes immediately on resume when the auto-close deadline elapsed during a pause— advances the wall clock pastdurationwithout draining the fake-timer queue, then dispatches Radix'stoast.viewportPause/toast.viewportResumeevents on the viewport<ol>. AssertsonClose(false)fires immediately on resume.Verified: each new test fails on the corresponding pre-fix code path (parent-re-render, provider-duration, resume-after-deadline) and passes with the fix applied.
Verification
yarn test— 480 passed (59 files), including the new 6-testToastsuite.yarn typecheck— clean.yarn lint— clean (no new warnings/errors; same 13 pre-existing warnings in unrelated files).yarn build— clean;Toastchunk built successfully.@radix-ui/react-toast@1.2.17(2026-06-15) still has the samecloseTimerRef-without-unmount-cleanup code path as1.2.2; only whitespace differs inToastImpl.Tickets?
Contribution checklist?
buildcommand runs locallySecurity checklist?
dangerouslySetInnerHTMLPreview?
n/a — internal timer / lifecycle change with no visible UI/UX delta.
Slack Thread