Repository navigation
fix(Toast): a toast dismissed with no row on screen leaves the stack - #8050
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsToast (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 494 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 26 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works: a toast dismissed while hidden past maxVisible, or before its first render, now leaves the stack, the next toast surfaces, and the empty Notifications region is gone. Two changes are needed before approval:
- A hidden toast dismissed twice before the viewport re-renders now fires
onHidetwice, because the no-row branch removes the id fromexitingIdsRefwhiletoastsRefstill lists the toast; also dropping it fromtoastsRef(or keeping the guard until the list commits) fixes it, and a regression test for that case would pin it. - The new commit-time
useLayoutEffectdoes not name an external system asarchitecture:react-component-runtimeINV3 requires; every exit path already deletes the pending copy, so it can be removed, or its comment can name what it synchronizes.
[Automated review]
| // Waiting for its row to finish exiting would keep it forever: when it | ||
| // surfaced, its row would mount already collapsed, with no transition | ||
| // left to end, and hold a visible slot as an empty row. | ||
| exitingIdsRef.current.delete(id); |
There was a problem hiding this comment.
Releasing the guard here while toastsRef.current still lists the toast (it is only reassigned on the next render) lets a second dismiss() in the same batch pass the check at line 462 and call onHide again. With maxVisible={1} and two dismissA() calls in one act(), onHide runs twice on this branch and once on main. Dropping the entry from toastsRef.current here, or keeping the id in exitingIdsRef until the list commits, fixes it; a regression test for the double call would pin it.
| // Once committed, a toast is found in the list; forget its pending copy. | ||
| useLayoutEffect(() => { | ||
| const pending = pendingEntriesRef.current; | ||
| for (const entry of toasts) { | ||
| pending.delete(entry.id); | ||
| } | ||
| }, [toasts]); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes; this one reconciles a private map with React's own commit. It is also not load-bearing: handleExited, the no-row branch and the uniqueID loop already delete a toast's pending copy when it leaves the list, and the Toast suite and both new tests pass without it, so it can be removed (or kept with a comment naming what it synchronizes).
A dismissed toast left the list only when its row's exit transition ended. A toast with no mounted row (hidden past maxVisible, or dismissed before it first rendered) has no transition: when it came back into view its row mounted already collapsed and never left, holding a visible slot and the Notifications landmark open with nothing in it. Such a toast now leaves the list at once, and a dismiss() before the first render still finds the toast and reports onHide once, even when it is dismissed twice before the viewport renders again.
13228e6 to
46755a5
Compare
|
Both findings are fixed at 46755a5.
|
There was a problem hiding this comment.
Recommending approval: a toast dismissed while it is hidden past maxVisible, or before its first render, now leaves the stack, the slot it held is freed, and the empty Notifications landmark is gone, with onHide firing once on each path. Both findings from the earlier review are fixed in this revision. One optional cleanup: the comment on pendingEntriesRef still says it holds toasts dispatched since the last commit, but with the layout effect removed it holds every toast until that toast leaves the list.
[Automated review]
| // Toasts dispatched since the last commit, by id: a dismiss() called | ||
| // before the toast first renders still finds it (and its onHide). |
There was a problem hiding this comment.
Optional, non-blocking: with the commit-time effect removed, nothing prunes this map at commit. An entry now leaves only through the uniqueID loop in addToast, the no-row branch in removeToast, or handleExited, so the map holds every live toast, committed or not. Something like // Every toast dispatched and not yet removed, by id: a dismiss() called before the toast first renders still finds it (and its onHide). would match the code.
meta#8050, now on main, tracks toasts dispatched before they render (pendingEntriesRef) so a dismissal still finds them, and lets a toast with no mounted row leave the list at once. This pull request moves toastsRef ahead of the render and keeps the toasts a repeat replaced in the batch. Both are kept: a dismissal finds a toast in toastsRef, then among the replaced ones, then among the pending ones. Two paths had to leave every list for the union to report onHide once (red first): a leaving toast that a new one pushes out of view leaves the pending map too, and a toast with no mounted row leaves the replaced ones too, so a toast a repeat replaced before it rendered is reported once when one handler dismisses it twice.
User impact
A toast dismissed while it has no row on screen never leaves the stack, when an app calls the
dismiss()fromuseToast():maxVisible, orIt stays as an invisible row that holds a visible slot; after every real toast is gone, the "Notifications" landmark stays and F6 focuses an empty viewport.
Problem and solution fit
dismiss()returns early (already leaving).dismiss()did not find the toast, soonHidenever fired.Expected behavior and authority
architecture:layer-runtime(current): ToastViewport owns the entries, timers and focus handoff.ToastViewport region ARIA).ToastOptions.onHide: "Callback fired when the toast is removed."Smallest restoration
onHidefires once.Evidence
Mobile Safari, iOS 26.2 simulator, "Programmatic dismiss" story: a persistent toast, then five shown and dismissed (labels drawn by the test page). Left,
main: the five stay as invisible rows (the label counts them) and push the toast out. Right: it stays.main(maxVisible2, toasts A, B, C;dismiss()hidden A; close C): A returns as an empty row beside B; after B closes, the row and landmark remain.toast()thendismiss()in one handler leaves the same row, with noonHide.onHidefires once in both cases, even for twodismiss()calls in one batch.mainand pass here.Related: touches adjacent lines with #8047 and #8051; whichever lands second rebases.
Scope
Testing
pnpm exec vitest run packages/core/src/Toast/: 3 files, 80 tests passed. The three new tests fail onmain; the double-dismiss one also fails on the previous head.pnpm -F @astryxdesign/core typecheck: pass.architecture:component-style-authoringINV6): Toast's import, bundled with React and StyleX external and minified, is 51,488 bytes gzip onmainand +73 bytes here; no CSS change.check:changesets,check:sync,check:knowledge, …).