Conversation
commit: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Greptile OverviewGreptile SummaryThis PR extends the toast close API to support dismissing all active toasts by calling Two issues to address before merge:
Confidence Score: 4/5
Important Files Changed
|
|
I'm okay with this change, but it will conflict a lot with #3464, not sure whether to merge it before or after @flaviendelangle |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
|
Given that this PR is significantly smaller than the other one, I think it may make sense for me to wait 😅 |
| closeToast = (toastId?: string) => { | ||
| const closeAll = toastId === undefined; | ||
| const { limit, toasts } = this.state; | ||
|
|
There was a problem hiding this comment.
onClose fired too often
closeToast now invokes onClose for all toasts before checking whether each toast is already ending (transitionStatus === 'ending') (store.ts:233-240 vs mapping logic at 247-249). This means calling close() (close-all) can trigger onClose for toasts that were already in the process of closing (e.g. auto-dismiss just started), which differs from the previous behavior where onClose was only called for the explicitly targeted toast ID and only when that toast existed. Consider aligning onClose invocation with the same predicate used to mark a toast as ending (skip ones already ending) to avoid duplicate onClose calls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react/src/toast/store.ts
Line: 229:232
Comment:
**onClose fired too often**
`closeToast` now invokes `onClose` for *all* toasts before checking whether each toast is already ending (`transitionStatus === 'ending'`) (store.ts:233-240 vs mapping logic at 247-249). This means calling `close()` (close-all) can trigger `onClose` for toasts that were already in the process of closing (e.g. auto-dismiss just started), which differs from the previous behavior where `onClose` was only called for the explicitly targeted toast ID and only when that toast existed. Consider aligning `onClose` invocation with the same predicate used to mark a toast as ending (skip ones already ending) to avoid duplicate `onClose` calls.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
It is true. This can also happen with single-close as well.
Since this is an existing issue, I will create a separate PR to address it.
Additional Comments (1)
The Prompt To Fix With AIThis is a comment left during a code review.
Path: docs/src/app/(docs)/react/components/toast/page.mdx
Line: 282:285
Comment:
**Docs type mismatch**
The `useToastManager` return-value table still documents `close` as `(toastId: string) => void` (page.mdx:283-286), but the implementation now allows `close()` with no argument to close all toasts. This makes the docs internally inconsistent and will mislead TypeScript users reading the docs; the table should reflect the new optional parameter (e.g. `close: (toastId?: string) => void` or similar).
How can I resolve this? If you propose a fix, please make it concise. |
|
Quick ping on this PR 🙂 |
|
Uh-oh.. Prettier rule has changed. I will re-format and push the change soon! |
Codex ReviewOverviewThis patch adds support for dismissing every toast by calling FindingsNo blocking issues found in this patch. 1. 🟡 [Non-blocking] Close-all exposes an existing duplicate
|
|
@atomiks As always, thanks for jumping in and taking care of this so quickly. Much appreciated! |
This PR adds a way to close all toasts at once.
The
closeAPI now accepts an optionalid:close(id)closes a specific toastclose()closes all active toastsAlternatively, I considered introducing a separate
closeAllAPI, but decided to keep the change minimal for the following reasons:Adding
closeAllcould require changes toToastManagerEventused bysubscribe. I was not sure whether it is a public API, and making changes to theToastManagerEventmight be considered a breaking change (though that can be decoupled).Other libraries such as Sonner and Chakra expose the same behavior through a single
dismissAPI, where calling it without an argument dismisses all toasts.I have followed (at least) the PR section of the contributing guide.