Fix: Toast disableAnimation Not Respecting Immediate Close #5804
Conversation
|
|
@ariansj01 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdates DateRangePicker example logic to handle both start/end focus changes, expands DateRangePicker docs with troubleshooting and clarified onFocusChange description, and modifies toast provider to support immediate close via a disableAnimation flag (including a global flag), updating closeToast and closeAll signatures and control flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant ToastProvider
participant ToastItem
User->>App: Trigger closeAll(disableAnimation?)
App->>ToastProvider: closeAll(disableAnimation?)
alt disableAnimation true OR globalDisableAnimation
loop For each visible toast
ToastProvider->>ToastItem: close(immediate)
ToastItem-->>ToastProvider: removed instantly
end
else Delayed close path
loop For each visible toast
ToastProvider->>ToastItem: close(start exit)
Note over ToastItem: 300ms delay/animation
ToastItem-->>ToastProvider: removed after delay
end
end
sequenceDiagram
autonumber
participant User
participant DateRangePicker
participant ExampleState as Example State
User->>DateRangePicker: Focus changes to "start" or "end"
DateRangePicker->>ExampleState: onFocusChange(val)
alt val == "start"
ExampleState->>ExampleState: setRange({ start: newStart, ... })
else val == "end"
ExampleState->>ExampleState: setRange({ end: newEnd, ... })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/docs/content/components/date-range-picker/presets.raw.jsx(1 hunks)apps/docs/content/docs/components/date-range-picker.mdx(2 hunks)packages/components/toast/src/toast-provider.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/components/toast/src/toast-provider.tsx (1)
packages/components/toast/src/index.ts (2)
closeToast(9-9)closeAll(9-9)
🔇 Additional comments (3)
packages/components/toast/src/toast-provider.tsx (3)
87-94: LGTM! Immediate closure logic is correct.The conditional immediate closure path properly bypasses the timeout when
disableAnimationis true, fixing the reported issue. The fallback toglobalDisableAnimationensures programmatic calls respect the provider's setting.
104-113: LGTM! closeAll correctly propagates disableAnimation.The
disableAnimationparameter is properly passed through to eachcloseToastcall, enabling batch immediate closure when requested.
14-14: Ensure single ToastProvider or refactor disableAnimation to React context.
GlobalglobalDisableAnimationis mutated on each render, assuming a singleton provider; multiple instances will override the flag and cause unpredictable animations.
| onFocusChange: (val) => { | ||
| if (val === "start") { | ||
| setValue({...value, start: val}); | ||
| } else if (val === "end") { | ||
| setValue({...value, end: val}); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Critical logic error: onFocusChange incorrectly assigns string to date value.
The code checks if val === "start" or val === "end", but then directly assigns val to the date fields. This will set value.start or value.end to the string "start" or "end" instead of a valid date value, breaking the DateRangePicker.
Based on React Aria's RangeCalendar API, onFocusChange receives a DateValue (the newly focused date), not a focus indicator string. The conditional checks for string equality are incorrect.
Apply this diff to fix the logic:
- onFocusChange: (val) => {
- if (val === "start") {
- setValue({...value, start: val});
- } else if (val === "end") {
- setValue({...value, end: val});
- }
- },
+ onFocusChange: (val) => {
+ setValue({...value, start: val});
+ },If you need to track which field (start or end) is focused, use a separate state variable or the calendar's built-in focus management rather than inspecting the date value.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/docs/content/components/date-range-picker/presets.raw.jsx around lines
99–105, onFocusChange currently treats its argument as a focus indicator string
and assigns "start"/"end" into the date fields, which is wrong; replace this
logic so onFocusChange treats val as a DateValue (or null) and does not assign
literal strings into value.start/value.end — either update a separate
focus-tracking state (e.g., focusedField = "start" | "end") and use that to
decide which date field to update elsewhere, or if the intent was only to track
focus, set a focusedDate/focusedField state instead of mutating the date range;
remove the string-equality checks and only assign DateValue objects to date
fields (or avoid assigning here entirely and update dates via the proper
onChange handler).
| #### onFocusChange only updates start date | ||
| When using `onFocusChange` in DateRangePicker, make sure to handle both start and end date focus changes properly: | ||
|
|
||
| ```jsx | ||
| // ❌ Wrong - only updates start date | ||
| onFocusChange: (val) => setValue({...value, start: val}), | ||
|
|
||
| // ✅ Correct - handles both start and end focus | ||
| onFocusChange: (val) => { | ||
| if (val === "start") { | ||
| setValue({...value, start: val}); | ||
| } else if (val === "end") { | ||
| setValue({...value, end: val}); | ||
| } | ||
| }, | ||
| ``` |
There was a problem hiding this comment.
Documentation example contains the same logic error as the implementation.
The "correct" example shows the same bug present in presets.raw.jsx: checking if val === "start" or val === "end", then assigning val directly to the date fields. This treats val as both a focus indicator (string) and a date value, which is incorrect.
Based on React Aria's RangeCalendar API, onFocusChange receives a DateValue (the newly focused date cell), not a string indicator. The example will cause value.start or value.end to be set to the literal strings "start" or "end".
Update the documentation to show the correct pattern:
-#### onFocusChange only updates start date
-When using `onFocusChange` in DateRangePicker, make sure to handle both start and end date focus changes properly:
-
-```jsx
-// ❌ Wrong - only updates start date
-onFocusChange: (val) => setValue({...value, start: val}),
-
-// ✅ Correct - handles both start and end focus
-onFocusChange: (val) => {
- if (val === "start") {
- setValue({...value, start: val});
- } else if (val === "end") {
- setValue({...value, end: val});
- }
-},
-```
+#### onFocusChange receives DateValue, not a focus indicator
+When using `onFocusChange` in RangeCalendar, the callback receives the newly focused `DateValue`:
+
+```jsx
+// ✅ Correct - updates the focused value
+onFocusChange: (val) => {
+ setValue({...value, start: val});
+},
+```
+
+If you need to track whether the start or end field is focused, use `calendarProps.focusedValue` to determine which field should be updated, or manage focus state separately.🤖 Prompt for AI Agents
In apps/docs/content/docs/components/date-range-picker.mdx around lines 369 to
384, the docs incorrectly treat the onFocusChange callback value as a focus
indicator string and assign it to start/end; instead update the text and example
to state that onFocusChange receives a DateValue (the newly focused date cell)
and show a correct pattern that sets the appropriate date field (e.g.,
setValue({...value, start: val}) for the focused start), and add guidance to use
calendarProps.focusedValue or separate focus state if you need to know whether
start or end is focused; replace the broken examples with this corrected
explanation and example.
| attribute: "onFocusChange", | ||
| type: "(isFocused: boolean) => void", | ||
| description: "Handler that is called when the element's focus status changes.", | ||
| description: "Handler that is called when the element's focus status changes. **Important**: When using this with DateRangePicker, make sure to handle both start and end date focus changes properly to avoid losing the end date selection.", |
There was a problem hiding this comment.
Inconsistent with the flawed troubleshooting example.
The warning about handling both start and end date focus changes aligns with the incorrect troubleshooting example above (lines 377-384), which contains the same logic error as the implementation.
Update this description to accurately reflect the correct onFocusChange behavior:
- description: "Handler that is called when the element's focus status changes. **Important**: When using this with DateRangePicker, make sure to handle both start and end date focus changes properly to avoid losing the end date selection.",
+ description: "Handler that is called when the element's focus status changes. The callback receives a `DateValue` representing the newly focused date in the calendar.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: "Handler that is called when the element's focus status changes. **Important**: When using this with DateRangePicker, make sure to handle both start and end date focus changes properly to avoid losing the end date selection.", | |
| description: "Handler that is called when the element's focus status changes. The callback receives a `DateValue` representing the newly focused date in the calendar.", |
🤖 Prompt for AI Agents
In apps/docs/content/docs/components/date-range-picker.mdx around line 729, the
description for onFocusChange repeats the flawed troubleshooting logic and
misstates behavior; update the text to clearly describe the actual onFocusChange
contract: it receives the new focused input value (e.g., 'startDate', 'endDate'
or null), callers should update their focusedInput state to that value (not
toggle or infer the other field), and ensure focus changes do not inadvertently
clear or overwrite the other date value — replace the warning with concise
guidance on handling the focusedInput value directly and preserving start/end
date values.
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
There was a problem hiding this comment.
- please don't include other changes like date range picker which is not related in this PR
- please add a changeset
- eslint fails
- your PR changes the existing behaviour. With animation (default setting), try create a toast and wait for timeout. You will see the animation is gone when closing. The expected result should be same as closing the toast manually.
2 participants
from the #5802
Description
When
disableAnimation={true}was set onToastProvider, toasts still had a 300ms delay before closing whencloseToast()orcloseAll()was called programmatically. This issue occurred because thecloseToastfunction always applied a 300ms timeout regardless of thedisableAnimationsetting.Root Cause
The
closeToastfunction intoast-provider.tsxhad a hardcoded 300ms timeout that was always applied:Solution
1. Added Global State Tracking
2. Store Settings in ToastProvider
3. Enhanced closeToast Function
Result
Now when
disableAnimation={true}is set:And using
closeToast():Testing
disableAnimation={true}anddisableAnimation={false}Files Changed
packages/components/toast/src/toast-provider.tsxTypeScript Changes
disableAnimationparameter tocloseToastandcloseAllfunctionsdisableAnimationsettingdisableAnimationsettingFixes the issue where toasts with
disableAnimation={true}still had a 300ms delay before closing when using programmatic close methods.90Summary by CodeRabbit
New Features
Bug Fixes
Documentation