Repository navigation
fix(Toast): a leaving toast pushed past the cap leaves the stack - #8051
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsToast (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to 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.
Thanks for the fix: a dismissed toast no longer holds its uniqueID, and both new tests fail on main and pass here, but one regression on the same path needs a change first. When the stack is already at maxVisible (always with maxVisible={1}), the repeated toast pushes the leaving one out of toasts.slice(-maxVisible), so it never finishes its exit and later comes back as an empty 0 px row that keeps the "Notifications" landmark and the F6 stop after every real toast has closed. Please keep leaving rows out of the visible-limit count (or drop a leaving entry once the window cuts it off), and add a maxVisible={1} test that dismisses a toast, repeats it at once, closes the repeat, and expects no row and no landmark.
[Automated review]
| return prev.map(t => (t === existing ? entry : t)); | ||
| } | ||
| } | ||
| return [...prev, entry]; |
There was a problem hiding this comment.
When the stack is already at maxVisible, this append puts the repeat after a leaving namesake, and toasts.slice(-maxVisible) (line 547) can cut the leaving row off. Its grid-template-rows transition then never ends, so the entry stays in toasts and exitingIdsRef and later remounts as a collapsed row that keeps the "Notifications" landmark and the F6 stop. Reproduced with maxVisible={1}: dismiss, repeat at once, close the repeat. Keeping leaving entries out of the visible-limit count, or dropping a leaving entry once the window cuts it off, avoids it.
091fd17 to
fab26ee
Compare
|
Fixed at fab26ee. When a new toast pushes a leaving one out of the visible window, the leaving toast now leaves the list at once (it has already reported |
There was a problem hiding this comment.
Thanks for the follow-up: a closed toast no longer holds its uniqueID while the stack has room, and the stranded row at the visible limit is gone, but the new cut path needs one more change. At maxVisible (for example maxVisible={2} with the closed toast oldest), the setToasts updater runs prev.find(holdsUniqueID) after the cut has removed the namesake from exitingIdsRef, so it overwrites the cut entry in place: the repeat takes the closed toast's slot (with 'overwrite' exactly as on main), and an overwrite can bring back a toast the app dismissed while it was hidden, firing onHide twice. Filtering cutIds out of prev before that search fixes both; please add a maxVisible={2} test that closes the oldest toast, shows its repeat, and expects it after the newer toast.
[Automated review]
| setToasts(prev => { | ||
| if (uniqueID) { | ||
| const existing = prev.find(t => t.options.uniqueID === uniqueID); | ||
| const existing = prev.find(holdsUniqueID); |
There was a problem hiding this comment.
This search runs after lines 446-448 removed every cut id from exitingIdsRef, so a namesake the window just cut counts as a live holder here and is overwritten in place: at maxVisible the repeat lands in the closed toast's slot (with 'overwrite' exactly as on main). This branch also never applies cutIds, so a toast the app dismissed while it was hidden stays in the list as a live toast and fires onHide again when closed. Dropping the cut entries first fixes both:
const kept = prev.filter(t => !cutIds.has(t.id));
const existing = uniqueID ? kept.find(holdsUniqueID) : undefined;
return existing ? kept.map(t => (t === existing ? entry : t)) : [...kept, entry];
fab26ee to
9da49d4
Compare
|
Fixed at 9da49d4. The updater now drops the cut entries before it looks for a |
There was a problem hiding this comment.
Thanks for the update: the visible-limit cut now runs before the holder search, both earlier cases have tests, and a status repeated during its exit is shown and announced. One regression remains on a nearby path: when an app shows a repeat and then calls its namesake's dismiss function in the same handler, toastsRef has already replaced the namesake, so removeToast skips its onHide, while the setToasts updater re-runs holdsUniqueID, finds no holder and appends, leaving the namesake to play its exit with no onHide call (on main it is replaced in place and onHide fires once). Letting the updater apply the choice the projection already made (overwritten, by identity) keeps the list and toastsRef in step and keeps the Toast suite green; please also add a test that shows a repeat and dismisses its namesake in one handler.
[Automated review]
| // uniqueID (it left exitingIdsRef above) and must not be overwritten. | ||
| const kept = prev.filter(t => !cutIds.has(t.id)); | ||
| // An ignored collision already returned above; overwrite in place. | ||
| const existing = uniqueID ? kept.find(holdsUniqueID) : undefined; |
There was a problem hiding this comment.
When the namesake's dismiss function runs after addToast in the same handler, this search runs after removeToast added the namesake to exitingIdsRef, so it finds no holder and appends. The namesake stays on screen and plays its exit, but removeToast looked it up in the already-advanced toastsRef (line 452), did not find it, and skipped its onHide (0 calls; main: 1). Applying the projection's choice keeps the list and toastsRef in step, and the Toast suite still passes with it:
return overwritten && kept.includes(overwritten)
? kept.map(t => (t === overwritten ? entry : t))
: [...kept, entry];
9da49d4 to
a29c123
Compare
|
Fixed at a29c123. The updater now applies the overwrite the projection already chose, by identity, so a dismissal in the same handler can no longer turn it into an append; and an entry a repeat replaced in the same batch is still found by that batch's dismissal, so the namesake reports |
There was a problem hiding this comment.
Thanks for the update: a toast that is leaving no longer holds its uniqueID, a repeat and its namesake’s dismissal in one handler now report onHide once with the repeat in place, and each earlier request has a test that fails without its fix, so this is ready for approval. Two optional follow-ups are noted inline: the Changeset’s “finishes its exit” sentence does not hold at maxVisible, and focus after that same-handler dismissal now goes to the oldest toast rather than a neighbour.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A toast that is leaving no longer holds its `uniqueID`. Before, closing a toast and showing one with the same `uniqueID` during its exit dropped the new toast when its `collisionBehavior` was `'ignore'`, and with `'overwrite'` pulled the closed toast back on screen as the new one. The leaving toast now finishes its exit and the new one is shown. |
There was a problem hiding this comment.
At maxVisible the leaving toast does not finish its exit: when the new toast pushes it out of the visible window, addToast drops it from the list at once (ToastViewport.tsx lines 447-460), as the PR description says. Optional: “The leaving toast now finishes its exit (or leaves at once when the stack is full) and the new one is shown.”
| exitingIdsRef.current.add(id); | ||
| const entry = toastsRef.current.find(t => t.id === id); | ||
| const entry = | ||
| toastsRef.current.find(t => t.id === id) ?? replacedRef.current.get(id); |
There was a problem hiding this comment.
This finds a namesake that a repeat replaced earlier in the same handler, so onHide fires once. The focus handoff below still looks the id up in the advanced toastsRef.current (lines 508-513), where its index is -1, so toastsRef.current[0], the oldest toast, receives focus rather than a neighbour: with Alpha, Bravo and Offline, and a Retry control inside Offline that shows a repeat and then dismisses Offline, focus lands on Alpha here and on Bravo on main. Optional: using the replacement’s index there would keep the neighbour; focus does stay on a remaining toast either way.
There was a problem hiding this comment.
Changes requested: removing a closing toast once a newer toast pushes it past maxVisible fixes the lingering empty row and Notifications landmark, and the branch keeps FR8's uniqueID behavior, but two edge cases contradict packages/core/src/Toast/Toast.spec.md and the pull request text no longer matches the code.
- With
maxVisible={0},firstVisibleatToastViewport.tsx:451cuts every closing toast whenever another arrives, so it vanishes mid-collapse (FR7, FR5); guarding onmaxVisible > 0with a test fixes it. - A toast replaced through
uniqueIDbefore it rendered now reportsonHidewhen the same handler dismisses it (the test atToastViewport.test.tsx:2262), while FR3 says a replaced toast reports none. - A second same-
uniqueIDignoretoast in one handler is now dropped unannounced, butToast.spec.mdstill lists that gap (FR8, ORD1, its Current-state bullet and the gap count), so please update it, add a test and mention it in the changeset. - The title and description still describe the reverted “no longer holds its uniqueID” change, and the record edit needs a
## Writing rubric resultssection.
[Automated review]
| const committed = toastsRef.current; | ||
| const overwritten = uniqueID ? committed.find(holdsUniqueID) : undefined; | ||
| if (overwritten) { | ||
| replacedRef.current.set(overwritten.id, overwritten); |
There was a problem hiding this comment.
This also records a namesake that has not rendered yet, so when one handler shows a toast, shows its uniqueID repeat and then calls the first toast's dismiss function, removeToast finds it here and fires onHide('manual'); main reports nothing, and the test at ToastViewport.test.tsx:2262 pins the new call. Toast.spec.md FR3 says "A toast replaced through uniqueID is not dismissed and reports no onHide." Recording only entries from the last rendered list keeps the after-render case main already reports.
There was a problem hiding this comment.
Fixed in a503605: only a toast from the last rendered list stays findable for the batch, so one replaced before it rendered reports no onHide (FR3), and the test now asserts that.
| const projected = overwritten | ||
| ? committed.map(t => (t === overwritten ? entry : t)) | ||
| : [...committed, entry]; | ||
| const firstVisible = projected.length - maxVisible; |
There was a problem hiding this comment.
With maxVisible={0} this is projected.length, so every leaving toast is cut whenever any toast arrives, although toasts.slice(-0) (line 616) renders them all. FR7 says maxVisible={0} shows every toast, and FR5 has a dismissed card collapse before it unmounts. Suggestion: const firstVisible = maxVisible > 0 ? projected.length - maxVisible : 0; plus a maxVisible={0} test.
There was a problem hiding this comment.
Fixed in a503605: with maxVisible={0} nothing is cut (firstVisible is 0), and a test keeps the leaving toast collapsing while a new one arrives.
| setExitingIds(prev => new Set([...prev].filter(id => !cutIds.has(id)))); | ||
| } | ||
| // Later toasts in the same batch see this one before the next render. | ||
| toastsRef.current = projected.filter(t => !cutIds.has(t.id)); |
There was a problem hiding this comment.
Advancing toastsRef here also makes a second same-uniqueID ignore toast in one handler drop unannounced (on main both are announced and the second shows). Toast.spec.md still lists that as a gap: FR8 (line 200), ORD1 (line 223), the Current-state bullet at line 145 and the count at line 53. Please update those, add a test and mention it in the Changeset.
There was a problem hiding this comment.
Done in a503605: the record drops that gap from FR8, ORD1, its Current-state bullet and the count, a test pins the same-handler pair, and the changeset says so.
vjeux
changed the title
There was a problem hiding this comment.
Changes requested: closing a toast while a new one pushes it past maxVisible no longer leaves an invisible row, and the earlier edge cases are fixed, but the record edit now claims more than the code does. A toast that one handler dismisses right after raising a new toast at the cap (for example an Undo action that shows “Restored” and then dismisses its own toast) still comes back as a zero-height row that keeps the Notifications landmark and the F6 stop, while Toast.spec.md now lists only the outlet case. Please either have removeToast drop a toast that the same handler has already pushed out of the visible window, with a test, or keep that ordering in the Current-state bullet.
[Automated review]
| above the caller a dismissal called before the fallback viewport | ||
| mounts does nothing, so the toast shows and stays unless it | ||
| auto-hides (FR3, FR5, FR7, FR10, FR11, FR12). | ||
| - **A toast can outlive its dismissal.** A toast still collapsing when its |
There was a problem hiding this comment.
This bullet now names only an outlet move, but a toast dismissed in the same handler right after a new toast pushes it past the cap still lingers on this head: at maxVisible={1}, an Undo action that calls toast(...) and then its own dismiss leaves a 0 px row, the Notifications landmark and the F6 stop, exactly as on main. Please keep that ordering here, or fix it in removeToast.
| const firstVisible = maxVisible > 0 ? projected.length - maxVisible : 0; | ||
| const cutIds = new Set( | ||
| projected | ||
| .filter((t, i) => i < firstVisible && exitingIdsRef.current.has(t.id)) |
There was a problem hiding this comment.
This cut only sees toasts that are already leaving when the new toast is added. If the same handler dismisses the oldest toast after toast(...), removeToast still finds its row mounted and marks it exiting (line 561), and the next render unmounts that row mid-collapse, so it lingers. Since toastsRef.current is already advanced there, removeToast could take the no-row path when the toast's index is below toastsRef.current.length - maxVisible (for maxVisible > 0); a test that raises a toast and then dismisses the oldest in one handler at maxVisible={1} would pin it.
There was a problem hiding this comment.
This looks ready to approve: a toast that one handler dismisses right after raising a newer toast at the maxVisible cap (an Undo that shows its confirmation first) now leaves at once, so no invisible row keeps the Notifications landmark or the F6 stop, and the narrowed Toast.spec.md bullet matches the code. The new test fails on the previous revision and on main, and a browser run of the same Undo flow shows focus moving to the confirmation instead of the page body. One optional follow-up: after a toast's own uniqueID repeat in the same handler, focus goes to the oldest toast rather than its neighbour (ToastViewport.tsx lines 535-541), although FR10 still holds.
[Automated review]
| const dismissedIndex = toastsRef.current.findIndex(t => t.id === id); | ||
| const next = | ||
| toastsRef.current[dismissedIndex + 1] ?? | ||
| toastsRef.current[dismissedIndex - 1]; | ||
| pendingFocusRef.current = next ? next.id : 'restore'; |
There was a problem hiding this comment.
Optional: when the dismissed toast was replaced by its own uniqueID repeat earlier in this handler, it is no longer in the advanced toastsRef, so dismissedIndex is -1 and this picks toastsRef.current[0], the oldest toast. With Alpha, Bravo and Offline, a Retry inside Offline moves focus to Alpha here and to Bravo on main. Using the replacement's index (or the replaced entry's position) would keep the neighbour; FR10 holds either way.
c40df1a to
8665798
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: dropping a closing toast once a newer toast pushes it past maxVisible removes the stranded row, landmark and F6 stop, and the new tests prove it, but the list that now runs ahead within a handler breaks the focus handoff in one same-handler order. When a control inside a toast raises that toast's own uniqueID repeat and then dismisses it, removeToast finds the replaced entry but looks its index up in the advanced list, gets -1 and moves focus to the oldest toast, or to the Notifications region container when the cap hides it (at maxVisible={2} with three toasts, where main focuses the neighbour), against FR10 in Toast.spec.md. Taking the neighbour around the replacement, plus a test that focuses a control inside the namesake, would fix it. Minor: a toast that is dismissed and pushed out in one handler now skips its collapse even when the toast that pushed it out is dismissed before it renders, so the description's "Unchanged: … the exit animation" should name that case.
[Automated review]
| } | ||
| const entry = | ||
| toastsRef.current.find(t => t.id === id) ?? | ||
| replacedRef.current.get(id) ?? |
There was a problem hiding this comment.
This finds a toast that a uniqueID repeat replaced earlier in the same handler, but the focus pick below still looks its index up in the advanced toastsRef (line 539), gets -1 and picks toastsRef.current[0], the oldest toast. With Alpha, Bravo and Offline at maxVisible={2}, a Retry inside Offline that raises its repeat and then dismisses Offline leaves focus on the Notifications region container (Alpha is hidden), where main focuses Bravo; FR10 asks for a control in a remaining toast. Taking the index of the replacement (the entry with the same uniqueID in toastsRef.current) keeps the neighbour, and a test that focuses a control inside the namesake would pin it.
There was a problem hiding this comment.
Fixed in 8948d4e: focus now goes to the nearest toast that shows, taken around the replacement (into the replacement when no neighbor shows), with tests at maxVisible={2} and with one visible place; the description names the pushed-out-and-dismissed case.
With `maxVisible` set, a toast that is closing when a new toast takes its
visible place never finished leaving: its row unmounted mid-exit, and it
came back later as an invisible, zero-height row that kept the
notifications landmark, took a visible place and let F6 focus its hidden
dismiss button. Such a toast now leaves the list at once, as does one that
a handler pushes out and then dismisses. Under `maxVisible={0}` nothing is
pushed out (FR7). A toast replaced through `uniqueID` before it rendered
reports no `onHide` (FR3), and a second `ignore` toast with the same
`uniqueID` raised in the same handler is dropped unannounced. A leaving
toast keeps its `uniqueID` (FR8). When a control inside a toast raises
toasts and dismisses its own, focus moves to the nearest toast that shows,
taken around a replacement, which itself takes focus when no neighbor shows
(FR10). Toast.spec.md drops the gaps these close.
8665798 to
8948d4e
Compare
There was a problem hiding this comment.
Looks ready to approve: a closing toast that a newer toast pushes past maxVisible now leaves at once in either order, so no invisible row keeps the Notifications landmark or the F6 stop, and focus after a same-handler uniqueID repeat reaches the neighbour again, with tests that fail on main for each restored case. One optional follow-up: when a single handler dismisses a shown toast at the cap and also raises and dismisses another toast before it renders, the shown toast now disappears without its collapse, so the description's "Unchanged: … the exit animation" could name that case.
[Automated review]
| setExitingIds(prev => new Set([...prev].filter(id => !cutIds.has(id)))); | ||
| } | ||
| // Later toasts in the same batch see this one before the next render. | ||
| toastsRef.current = projected.filter(t => !cutIds.has(t.id)); |
There was a problem hiding this comment.
Optional: this cut is decided when the toast is raised, so if the same handler dismisses a shown toast at the cap, raises this one and then dismisses it before it renders, the shown toast disappears without its collapse (on main it collapses). Nothing lingers and focus is unaffected; naming this case beside "Unchanged: … the exit animation" in the description would keep the claim exact.
|
/approve-spec 8948d4e |
github-actions
Bot
removed
the
needs:spec-owner-review
cixzhang
added
the
ci:run
User impact
With
maxVisibleset, a toast closing when a new one takes its visible place never finishes leaving: later it returns as an invisible, zero-height row that keeps the notifications landmark, holds a visible place, and draws F6 to its hidden dismiss button.Problem and solution fit
Expected behavior and authority
Toast.spec.mdlists it under Current-state impact: "A toast that a new toast pushes past the cap during its collapse … comes back as an invisible zero-height row" (FR5, FR7, FR10, FR11).uniqueID; FR7 shows every toast undermaxVisible={0}; FR3 has a replaced toast report noonHide; FR10 moves focus to a control in a remaining toast.Smallest restoration
maxVisible={0}nothing is pushed out.uniqueIDignoretoast there is dropped unannounced; a dismissal finds a rendered replaced toast (onHideonce, as onmain) but not an unrendered one (FR3); focus goes to the nearest toast that shows, around a replacement, or into it.ignoregap.Evidence
jsdom tests:
main): withmaxVisible={1}, close "Saved", show "Uploaded", then close it: a row and the landmark remain; after, none do.main: the undo flow, the same-handlerignorepair, focus into a lone replacement, and focus past a hidden toast. Fail on earlier heads: themaxVisible={0}collapse, an unrendered replaced toast'sonHide, FR8's leaving namesakes, and focus to a replaced toast's neighbor atmaxVisible={2}.Scope
Testing
Local gates at 8948d4e:
pnpm test:ui(16 workers): 16,466 pass (654 files), with no unhandled error; the node project (16 workers): 14,560 pass, its only reds twocache.test.mjscases (see fix(cli): a crashed run's release install is recognized by the Node it ran on, whatever its name #8520).pnpm build,pnpm typecheck,CI=true pnpm lint(0 errors) andpnpm check:knowledge: clean. A merge-tree withmain(925c8e5) is clean.architecture:component-style-authoringINV6, the CI's measurement): ToastViewport's import JS 66,323 → 66,681 B gzip againstmain(9ee7457); CSS unchanged (10,526 B); Toast unchanged.Writing rubric results
R1 narration: the retitled lingering-row bullet, FR8's gap text, ORD1 and the gap count state what the system does, in the present tense; none found.
R2 research: none in the record.
R3 evidence: no basis cell changed.
R4 rejected alternatives: none.
R5 change references: none.
R6 open questions: none; the record keeps "None".