fix(radio): emit form-friendly onChange on RadioGroup for RHF#5821
fix(radio): emit form-friendly onChange on RadioGroup for RHF#5821ariansj01 wants to merge 4 commits into
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. |
WalkthroughThis PR addresses compatibility and documentation issues across three components: DateRangePicker now correctly handles both start and end focus changes with improved troubleshooting documentation; RadioGroup adds onChange handler support for React Hook Form integration; Toast provider adds optional disableAnimation parameter to closeToast and closeAll methods. Changes
Sequence Diagram(s)sequenceDiagram
participant RHF as React Hook Form
participant RG as RadioGroup
participant Consumer as Consumer Handler
RHF->>RG: Register field (expects onChange)
Note over RG: User selects radio option
RG->>RG: handleValueChange invoked
RG->>Consumer: Call onValueChange(value)
RG->>RHF: Emit synthetic onChange event<br/>(target.name, target.value)
RHF->>RHF: Update form state
sequenceDiagram
participant App as Application
participant TP as Toast Provider
participant Toast as Toast Component
App->>TP: closeToast(key, disableAnimation?)
alt disableAnimation = true or globalDisableAnimation set
TP->>Toast: Close immediately
else disableAnimation = false
TP->>TP: Wait 300ms timeout
TP->>Toast: Close after timeout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR introduces focused feature additions across distinct components with moderate logic complexity. RadioGroup's onChange synthesis and Toast's animation control branching require careful validation against their respective use cases (React Hook Form compatibility and animation semantics). Documentation changes are straightforward but comprehensive verification of examples is needed. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4
📜 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 (4)
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/radio/src/use-radio-group.ts(3 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)
🪛 LanguageTool
apps/docs/content/docs/components/date-range-picker.mdx
[grammar] ~369-~369: There might be a mistake here.
Context: ... Common Issues #### onFocusChange only updates start date When using onFocusChange i...
(QB_NEW_EN)
🔇 Additional comments (2)
packages/components/radio/src/use-radio-group.ts (1)
101-118: Implementation correctly supports React Hook Form integration.The
handleValueChangefunction properly:
- Invokes
onValueChangefirst for controlled usage- Creates a synthetic form-like event for
onChangecompatibility with libraries like RHF- Maintains backward compatibility by only creating the synthetic event when
onChangeis presentThis aligns with the PR objective of fixing RadioGroup integration with React Hook Form.
Minor consideration: The
namefield (line 110) may beundefinedif not provided to RadioGroup. While most form libraries handle this gracefully, consider whether to provide a fallback or document thatnameis required when usingonChangewith form libraries.packages/components/toast/src/toast-provider.tsx (1)
77-94: Animation control logic is implemented correctly.The updated
closeToastandcloseAllfunctions properly:
- Accept optional
disableAnimationparameters for explicit control- Fall back to the global setting when not specified
- Immediately close toasts when animations are disabled
- Maintain backward compatibility with optional parameters
However, this implementation depends on the global state approach. Once the architectural concern with
globalDisableAnimationis addressed, verify that the fallback mechanism still works correctly with the new approach.Also applies to: 104-113
| 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: onFocusChange incorrectly overwrites date values with strings.
The val parameter in onFocusChange represents the focus state ("start" or "end"), not a date value. Lines 101 and 103 incorrectly set start: val and end: val, which overwrites the date objects with strings.
Apply this fix to preserve the actual date values:
-onFocusChange: (val) => {
- if (val === "start") {
- setValue({...value, start: val});
- } else if (val === "end") {
- setValue({...value, end: val});
- }
-},
+onFocusChange: (focusedValue) => {
+ // onFocusChange receives the new focused date value, not a string
+ // Update the corresponding date in the range
+ setValue((prev) => ({
+ ...prev,
+ // The focusedValue is the actual date being focused
+ ...(value.start.compare(focusedValue) === 0 ? {} : { start: focusedValue }),
+ ...(value.end.compare(focusedValue) === 0 ? {} : { end: focusedValue }),
+ }));
+},Or, if the intent is only to track which field has focus (not update dates), remove the handler entirely or use a different callback.
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, the onFocusChange handler is incorrectly assigning the focus string to
the start/end date fields (overwriting date objects). Fix by preserving the
existing date values and storing the focus state instead (e.g., update a
dedicated focus property or state) or remove the handler if only focus tracking
is needed; ensure setValue spreads the existing value and only updates a
non-date focus key rather than assigning val to start/end.
| ```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.
Critical: Documentation example has the same bug as the code.
The "correct" example still contains the critical error identified in presets.raw.jsx. Lines 379 and 381 set start: val and end: val, where val is the focus state string ("start" or "end"), not a date value. This overwrites date objects with strings.
The documentation needs to be corrected to show the proper usage. Based on the actual onFocusChange API, this callback receives the focused date value (not a string identifier). The example should be:
-// ✅ Correct - handles both start and end focus
-onFocusChange: (val) => {
- if (val === "start") {
- setValue({...value, start: val});
- } else if (val === "end") {
- setValue({...value, end: val});
- }
-},
+// ✅ Correct - onFocusChange receives the focused date value
+onFocusChange: (focusedValue) => {
+ // focusedValue is the date that now has focus
+ // Typically you only need this for advanced use cases
+ // For basic range selection, this handler is often not needed
+},Alternatively, if the Troubleshooting section is addressing a different issue, please clarify what onFocusChange is supposed to do in this context.
🤖 Prompt for AI Agents
In apps/docs/content/docs/components/date-range-picker.mdx around lines 372 to
384, the "correct" example mistakenly assigns start/end to val (a focus
identifier) and thus overwrites date objects with strings; update the snippet so
the focus identifier and the focused date value are treated separately — rename
the date param (e.g., focusedDate) and use the focus flag to decide which field
to update, calling setValue({...value, start: focusedDate}) when focus is start
and setValue({...value, end: focusedDate}) when focus is end, ensuring you do
not assign the focus string to the date fields.
| 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.
🧩 Analysis chain
Verify the onFocusChange API behavior and update documentation accordingly.
The updated description suggests handling "both start and end date focus changes" but may be based on incorrect assumptions about the onFocusChange API.
Please verify the actual signature and behavior of onFocusChange in the DateRangePicker component:
Once verified, update the description to accurately reflect the API's actual behavior.
🏁 Script executed:
#!/bin/bash
# Search for onFocusChange implementation in DateRangePicker
ast-grep --pattern $'onFocusChange: ($$$) => {
$$$
}'
# Find the type definition for onFocusChange
rg -nP --type=ts -C5 'onFocusChange.*:.*\(' Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Find DateRangePicker component
fd -e ts -e tsx 'date-range-picker' --type f | grep -E '(src|component)' | head -20
# Search for onFocusChange in date-picker package
rg -n 'onFocusChange' packages/components/date-picker/src/ -A2 -B2 --type ts --type tsxLength of output: 296
🏁 Script executed:
#!/bin/bash
# Inspect DateRangePicker component
cat git/packages/components/date-picker/src/date-range-picker.tsx | head -150
# Look for Calendar props interface
rg -n 'calendarProps' git/packages/components/date-picker/src/ -A5 -B2Length of output: 174
🏁 Script executed:
#!/bin/bash
# Search for onFocusChange definition
rg -n 'onFocusChange' git/packages/components/date-picker/src/ -B3 -A3Length of output: 80
🏁 Script executed:
#!/bin/bash
# Inspect DateRangePicker component with correct path
cat packages/components/date-picker/src/date-range-picker.tsx | head -200
# Search for onFocusChange in date-picker component files
rg -n 'onFocusChange' packages/components/date-picker/src/ -B3 -A3Length of output: 4003
🏁 Script executed:
#!/bin/bash
# Search use-date-range-picker for onFocusChange
cat packages/components/date-picker/src/use-date-range-picker.ts | grep -n 'onFocusChange' -B5 -A5
# Check RangeCalendar component
fd -e tsx -e ts 'range-calendar' packages/components/
# Search all onFocusChange references in date-picker
rg 'onFocusChange' packages/components/date-picker/Length of output: 2406
🏁 Script executed:
#!/bin/bash
# Inspect RangeCalendar component
cat packages/components/calendar/src/range-calendar.tsx | head -200
# Search use-range-calendar for onFocusChange type definition
rg -n 'onFocusChange' packages/components/calendar/src/use-range-calendar.ts -B3 -A3Length of output: 1089
🏁 Script executed:
#!/bin/bash
# Get full use-range-calendar.ts file to find interface definition
cat packages/components/calendar/src/use-range-calendar.ts | head -300
# Also search for UseRangeCalendarProps interface
rg -n 'interface UseRangeCalendarProps|export.*UseRangeCalendarProps' packages/components/calendar/src/use-range-calendar.ts -A20Length of output: 4479
🌐 Web query:
React Aria RangeCalendar onFocusChange callback signature
💡 Result:
The RangeCalendar onFocusChange signature is:
(date: CalendarDate) => void. [1][2]
Sources: React Aria RangeCalendar docs. [1][2]
Update onFocusChange description to reflect accurate API behavior.
The documented description is incorrect. React Aria's RangeCalendar onFocusChange receives a single CalendarDate parameter, not separate start/end parameters. The current description suggesting developers must "handle both start and end date focus changes" is misleading and inconsistent with the actual API.
The callback simply tracks which date is focused in the calendar—the developer controls how to apply that focused date to their state (e.g., assign to start date if empty, otherwise to end date).
Replace the description with:
Handler that is called when the focused date in the calendar changes. Receives the newly focused date as a parameter.
🤖 Prompt for AI Agents
In apps/docs/content/docs/components/date-range-picker.mdx around line 729, the
onFocusChange description incorrectly states it handles start/end focus changes;
update the string to accurately describe the API by replacing the current
sentence with: "Handler that is called when the focused date in the calendar
changes. Receives the newly focused date as a parameter." Ensure the new text
replaces the existing description exactly and preserves surrounding punctuation
and formatting.
| const loadFeatures = () => import("framer-motion").then((res) => res.domMax); | ||
|
|
||
| let globalToastQueue: ToastQueue<ToastProps> | null = null; | ||
| let globalDisableAnimation: boolean = false; |
There was a problem hiding this comment.
Reconsider using global mutable state for disableAnimation.
Storing disableAnimation in a module-level mutable variable has several issues:
- Multiple
ToastProviderinstances would overwrite each other's settings - Not compatible with React's concurrent rendering model
- Could cause race conditions in SSR or test environments
- Violates React best practices
Consider these alternatives:
- Store the setting in the ToastQueue - Pass
disableAnimationwhen adding toasts, or store it in the queue instance - Accept disableAnimation in closeToast - Require callers to explicitly pass the animation preference (already partially done with the optional parameter)
- Use a WeakMap - Map toast keys to their animation settings if persistence is needed
Example of option 1:
// Extend ToastQueue to store disableAnimation
const toastQueue = new ToastQueue({
maxVisibleToasts: Infinity,
disableAnimation: disableAnimation,
});
// Access it in closeToast
export const closeToast = (key: string, disableAnimation?: boolean) => {
const shouldDisableAnimation = disableAnimation ?? toastQueue.disableAnimation;
// ...
};Also applies to: 47-48
🤖 Prompt for AI Agents
packages/components/toast/src/toast-provider.tsx lines ~14 and ~47-48: remove
the module-level mutable globalDisableAnimation and instead make
disableAnimation an instance property on the ToastQueue (or otherwise stored
per-provider); update the ToastQueue constructor/type to accept and persist a
disableAnimation boolean, set that value when each ToastProvider creates its
queue from the provider prop, and change closeToast to compute
shouldDisableAnimation = disableAnimationParam ?? toastQueue.disableAnimation so
callers can still override. Also remove/replace any reads/writes to the old
global variable, update types and creation sites, and adjust tests to construct
queues with the desired disableAnimation rather than relying on global state.
wingkwong
left a comment
wingkwong
left a comment
There was a problem hiding this comment.
- please add changeset
- please add tests
- please don't include those unrelated files in this PR.
@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: |
|
Closing - inactivity |
2 participants
Description
{ target: { name, value } }alongsideonValueChange(value).register("type")onRadioGroupand toggle between options;watch()should reflect correct values and never be null.@heroui/radioCloses #4918
Summary by CodeRabbit
Bug Fixes
New Features
Documentation