Repository navigation
[toast] Fix timer and limit edge cases - #4933
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,380.94 ms +167.95 ms(+13.8%) | Renders: 50 (+0) | Paint: 2,081.84 ms +252.13 ms(+13.8%)
7 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
left a comment
There was a problem hiding this comment.
Critical Issues
None — the PR is mergeable as is.
Important Issues
-
clearTimerreset bypassed when timer fires for a removed toast —packages/react/src/toast/store.ts:444-447,:411-419,:312-318
scheduleTimerandresumeTimersinlinethis.timers.delete(id); callback()instead of routing throughclearTimer.closeToastthen early-returns at:314if the toast was concurrently removed viaremoveToast— so theareTimersPaused = falsereset never happens, re-introducing the bug this PR fixes. Narrow path (requires externalremoveToastof a toast with a running timer), but easy to close: extractprivate onTimerFired(id) { this.timers.delete(id); if (this.timers.size === 0) this.areTimersPaused = false; }and call it from both inline callbacks. -
store.handleDocumentPointerDownandToastViewport.handleKeyDown(Shift+Tab) still callresumeTimers()unconditionally —store.ts:425-437,ToastViewport.tsx:152-162
The PR addedresumeTimersIfWindowFocusedtohandleMouseLeave/handleBlurbut didn't apply the same gate elsewhere. Likely intentional (these paths imply explicit user activity), but worth confirming; if intentional, a one-line comment explains the asymmetry. -
Touchmove test doesn't verify scroll re-enables after pointer end —
packages/react/src/toast/root/ToastRoot.test.tsx:584-596
The new test firespointerCancelbut doesn't re-dispatch atouchmoveafterward to assertdefaultPrevented === false. Add one more dispatch afterpointerCancelto lock in the contract. -
Brittle
addEventListenerSpyreversed-find pattern —ToastViewport.test.tsx~:492-494,:539-541
Selecting the listener by last-registered ordering is fragile. Prefer dispatching a realwindow.dispatchEvent(blurEvent)or filter by the capture flag (the viewport'sblurlistener is the onlytrue-capture one in these tests).
Suggestions
-
applyLimitedrot risk —store.ts:69-83
Comment asserts "Toasts are ordered newest-first" but the function trusts iteration order. Tighten to "Caller guarantees newest-first ordering (seeaddToast)." -
Docs use "inert" colloquially —
docs/.../page.mdx:151,types.md,ToastProvider.tsx:75
Implementation does set the HTMLinertattribute (ToastRoot.tsx:521), so consider naming it: "remain mounted with theinertattribute". -
Missing test coverage:
- Limit change while toasts are mid-
ending(current "limit" test uses onlystartingtoasts). - Timer-elapsed callback path for
clearTimer(covers Finding 1). - Focus guard with
limit > 0and all visible toasts ending (onlylimit=0covered). syncProviderPropsearly-return reference stability.
- Limit change while toasts are mid-
-
Limited flag not cleared on ending toasts —
store.ts:69-83
applyLimitedskips ending toasts entirely, whereas the oldaddToastresetlimited: falseon every toast. Almost certainly benign (toast is on its way out), but no test pins this transition down.
Strengths
applyLimitedcleanly deduplicates three previously-divergent code paths and is reference-stable to avoid extra re-renders.syncProviderPropscorrectly usesuseIsoLayoutEffectper AGENTS.md with the right dep array and an early-return for unchanged values.- The
hasActiveToastscheck (replacingtoasts.length === 1) genuinely fixes a corner case where ending-only toasts lefthovering/focusedstucktrue. - The 5
timer pausingunit tests instore.test.tsenumerate failure modes in DAMP style — each test clearly documents the bug it pins down. - Public docs accurately reflect the new behavior (
data-limited↔inertattribute is correctly wired up). - F6 focus-guard skip correctly falls back to
restoreFocusToPrevElementwhen no toast can receive focus. - Tests follow project conventions (vitest + jest-dom only,
it.skipIfused correctly, no chai/sinon).
Recommended Action
- (Optional but cheap) Centralize the
areTimersPausedreset into a single helper called from bothscheduleTimer/resumeTimersinline callbacks (Finding 1). - Add the missing post-cancel
touchmoveassertion (Finding 3). - Decide intent on
handleDocumentPointerDown/handleKeyDownresume paths (Finding 2) and either gate them or add a comment. - Consider the docs/comment polish (Findings 5, 6).
- Test gaps (Finding 7) are nice-to-haves — current coverage is already strong for the fixes shipped.
7295cac to
067b981
Compare
Toast now handles several limit, timer, and focus edge cases that could leave hidden or inactive toasts in the wrong state.
Changes
limitchanges.