api updates and tests for radio primitives #1200
Conversation
WalkthroughThis update expands the Radio and RadioGroup primitives with new props, improved context typing, and enhanced keyboard navigation supporting directionality and flexible orientation. Stories are refactored for controlled usage and new scenarios, and the RovingFocusGroup utilities are updated to support both horizontal and vertical navigation, as well as right-to-left layouts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RadioGroupPrimitiveRoot
participant RadioGroupContext
participant RadioGroupPrimitiveItem
participant RovingFocusGroup
User->>RadioGroupPrimitiveRoot: Render with props (name, disabled, orientation, dir, etc.)
RadioGroupPrimitiveRoot->>RovingFocusGroup: Wrap children for focus management
RadioGroupPrimitiveRoot->>RadioGroupContext: Provide context (selectedValue, setSelectedValue, groupDisabled, name)
User->>RadioGroupPrimitiveItem: Interact (click, focus)
RadioGroupPrimitiveItem->>RadioGroupContext: Consume context, update selected value
RadioGroupPrimitiveItem->>RovingFocusGroup: Participate in focus navigation (respecting orientation and dir)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
39-40: Consider completing the RovingFocusGroup implementation.The RovingFocusGroup code is commented out, which suggests an incomplete implementation. This would provide keyboard navigation support for the radio group.
Would you like me to help implement the RovingFocusGroup integration or create an issue to track this enhancement?
Also applies to: 48-50
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (2)
6-15: Replace empty interface with type alias.The empty
RadioButtonPropsinterface should be replaced with a type alias as suggested by static analysis.Apply this fix:
-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +type RadioButtonProps = Record<string, never>;Or if no props are needed, simply use
{}in the function signature.
30-30: Consider demonstrating new RadioGroup features.The story examples should showcase the new props added to
RadioGroupPrimitive.Rootsuch asdisabled,required,name, etc.Consider adding stories that demonstrate these new features:
- <RadioGroupPrimitive.Root > + <RadioGroupPrimitive.Root name="language" required disabled={false} onChange={handleChange}>This would provide better documentation of the enhanced API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/primitives/Radio/RadioPrimitive.stories.tsx(2 hunks)src/core/primitives/Radio/index.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(1 hunks)src/core/primitives/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(2 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/Radio/index.tsx (2)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/context/RadioGroupContext.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/Radio/RadioPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (2)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (1)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
🧬 Code Graph Analysis (2)
src/core/primitives/RadioGroup/context/RadioGroupContext.tsx (1)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(3-7)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(3-7)
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 11-14: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
src/core/primitives/Radio/index.tsx (2)
10-12: Good addition of new props with proper typing.The new optional props (
disabled,asChild,required) are correctly typed and will enhance the component's functionality.
34-34: Props correctly passed to Primitive.input.The new props are properly spread to the underlying input element, maintaining the component's API consistency.
src/core/primitives/Radio/RadioPrimitive.stories.tsx (3)
13-16: Good addition of form wrapper for better context.Wrapping the RadioPrimitive in a
<form>element provides a more realistic usage context and helps test form-related behavior.
28-32: Comprehensive story args demonstrating new props.The story args effectively showcase the new
name,value,checked, andrequiredprops, providing good documentation and testing coverage.
35-45: Good addition of Disabled story for edge case coverage.The new Disabled story provides important test coverage for the disabled state functionality.
src/core/primitives/RadioGroup/context/RadioGroupContext.tsx (2)
3-9: Excellent addition of strongly typed context interface.The
RadioGroupContextValueinterface provides clear type definitions for all context properties, improving type safety and developer experience.
11-11: Proper context typing with null safety.The context is correctly typed as
RadioGroupContextValue | null, allowing for proper null checks in consuming components.src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx (2)
12-15: Good interface definition for RadioGroupPrimitive structure.The
RadioGroupPrimitiveTypeinterface clearly defines the expected shape of the RadioGroupPrimitive object, improving type safety.
17-20: Clean refactor to object literal with type safety.The object literal approach is more concise and maintainable than separate property assignments, while the explicit typing ensures type safety.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (3)
8-8: Good addition of disabled prop to type definition.The optional
disabledprop enhances the component's flexibility while maintaining type safety.
12-16: Excellent runtime context validation.The context presence check with descriptive error message helps developers catch usage errors during development.
29-29: Correct disabled state logic combining group and item states.The disabled logic properly combines both the group's disabled state and the individual item's disabled prop using logical OR.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (3)
6-17: LGTM! Good accessibility and configuration enhancements.The new props significantly improve the component's accessibility and configurability. The additions align well with modern React patterns and ARIA best practices.
30-36: LGTM! Context value properly updated.The context value correctly includes the new
groupDisabledandnameproperties, maintaining the existing API while adding the new functionality.
19-19: Fix parameter destructuring syntax.The destructuring syntax has a syntax error with the
disabled:groupDisabledrenaming.Apply this fix:
-const RadioGroupPrimitiveRoot = ({ children, defaultChecked = '', onChange, asChild=false, disabled:groupDisabled = false, required=false, name='', orientation='vertical', loop=true, ...props }: RadioGroupPrimitiveRootProps) => { +const RadioGroupPrimitiveRoot = ({ children, defaultChecked = '', onChange, asChild = false, disabled: groupDisabled = false, required = false, name = '', orientation = 'vertical', loop = true, ...props }: RadioGroupPrimitiveRootProps) => {⛔ Skipped due to learnings
Learnt from: kotAPI PR: rad-ui/ui#576 File: src/core/primitives/Toggle/index.tsx:15-22 Timestamp: 2024-11-24T06:43:42.194Z Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
17-21: LGTM! Good TypeScript improvements.The explicit typing of the options array and consistent formatting enhance code readability and type safety.
23-27: LGTM! Proper typing and handler signature.The state typing and updated handler signature correctly align with the new context API that passes string values directly.
56-66: LGTM! Proper form integration example.The form template correctly demonstrates controlled usage with proper TypeScript typing and event handling.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
Outdated
Show resolved
Hide resolved
| return ( | ||
| // <RovingFocusGroup.Root orientation={orientation} loop={loop}> | ||
| <RadioGroupContext.Provider value={sendItems}> | ||
| {/* <RovingFocusGroup.Group > */} | ||
| <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled} asChild> | ||
|
|
||
| {children} | ||
|
|
||
|
|
||
| </Primitive.div> | ||
| {/* </RovingFocusGroup.Group> */} | ||
| </RadioGroupContext.Provider> | ||
| // </RovingFocusGroup.Root> | ||
| ) | ||
| ; |
There was a problem hiding this comment.
Fix ARIA attribute type and improve formatting.
The implementation has good ARIA support but needs a type fix and formatting improvements.
Apply this fix:
- return (
- // <RovingFocusGroup.Root orientation={orientation} loop={loop}>
- <RadioGroupContext.Provider value={sendItems}>
- {/* <RovingFocusGroup.Group > */}
- <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled} asChild>
-
- {children}
-
-
- </Primitive.div>
- {/* </RovingFocusGroup.Group> */}
- </RadioGroupContext.Provider>
- // </RovingFocusGroup.Root>
- )
- ;
+ return (
+ // <RovingFocusGroup.Root orientation={orientation} loop={loop}>
+ <RadioGroupContext.Provider value={sendItems}>
+ {/* <RovingFocusGroup.Group > */}
+ <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled ? 'true' : 'false'} asChild={asChild}>
+ {children}
+ </Primitive.div>
+ {/* </RovingFocusGroup.Group> */}
+ </RadioGroupContext.Provider>
+ // </RovingFocusGroup.Root>
+ );Key fixes:
aria-disabledshould be a string ('true'/'false'), not booleanasChildprop was missing from the Primitive.div- Improved formatting and indentation
- Removed unnecessary empty lines
📝 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.
| return ( | |
| // <RovingFocusGroup.Root orientation={orientation} loop={loop}> | |
| <RadioGroupContext.Provider value={sendItems}> | |
| {/* <RovingFocusGroup.Group > */} | |
| <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled} asChild> | |
| {children} | |
| </Primitive.div> | |
| {/* </RovingFocusGroup.Group> */} | |
| </RadioGroupContext.Provider> | |
| // </RovingFocusGroup.Root> | |
| ) | |
| ; | |
| return ( | |
| // <RovingFocusGroup.Root orientation={orientation} loop={loop}> | |
| <RadioGroupContext.Provider value={sendItems}> | |
| {/* <RovingFocusGroup.Group > */} | |
| <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled ? 'true' : 'false'} asChild={asChild}> | |
| {children} | |
| </Primitive.div> | |
| {/* </RovingFocusGroup.Group> */} | |
| </RadioGroupContext.Provider> | |
| // </RovingFocusGroup.Root> | |
| ); |
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx between
lines 38 and 52, fix the aria-disabled attribute by converting its boolean value
to a string 'true' or 'false'. Ensure the asChild prop is present on
Primitive.div. Also, clean up the formatting by removing unnecessary empty lines
and adjusting indentation for better readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
15-38: Connect state to RadioGroup and display selected value.The story defines state and a change handler but doesn't connect them to the RadioGroup component or display the selected value. This makes the story less useful for demonstrating the component's behavior.
- <RadioGroupPrimitive.Root orientation='vertical'> + <RadioGroupPrimitive.Root orientation='vertical' value={language} onChange={handleChange}> {options.map((option) => ( <RadioGroupPrimitive.Item key={option.id} value={option.value}> {option.label} </RadioGroupPrimitive.Item> ))} </RadioGroupPrimitive.Root> + <p>Selected: {language}</p>
♻️ Duplicate comments (3)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (2)
38-52: Clean up code formatting and structure.The return statement has inconsistent formatting with unnecessary empty lines and poor indentation. This affects code readability and maintainability.
- return ( - <RovingFocusGroup.Root orientation={orientation} loop={loop} > - <RadioGroupContext.Provider value={sendItems}> - - <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled} asChild> - <RovingFocusGroup.Group > - {children} - </RovingFocusGroup.Group> - - </Primitive.div> - - </RadioGroupContext.Provider> - </RovingFocusGroup.Root> - ) - ; + return ( + <RovingFocusGroup.Root orientation={orientation} loop={loop}> + <RadioGroupContext.Provider value={sendItems}> + <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled ? 'true' : 'false'} asChild={asChild}> + <RovingFocusGroup.Group> + {children} + </RovingFocusGroup.Group> + </Primitive.div> + </RadioGroupContext.Provider> + </RovingFocusGroup.Root> + );
42-42: Fix ARIA attribute type for accessibility compliance.The
aria-disabledattribute should be a string value ('true'/'false') rather than a boolean to comply with ARIA specifications.- aria-disabled={groupDisabled} + aria-disabled={groupDisabled ? 'true' : 'false'}src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (1)
31-31: Use context-provided name instead of hardcoded value.The
nameprop is hardcoded as'radio'despite the context providing anamevalue. This could cause issues with form submission when multiple radio groups exist on the same page.- <RadioPrimitive name='radio' value={value} checked={checkedItem === value} onChange={handleOnChange} onFocus={handleOnChange} disabled={groupDisabled || disabled} aria-checked={checkedItem === value} required /> + <RadioPrimitive name={name} value={value} checked={checkedItem === value} onChange={handleOnChange} onFocus={handleOnChange} disabled={groupDisabled || disabled} aria-checked={checkedItem === value} required />
🧹 Nitpick comments (2)
src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (1)
5-5: Update JSDoc comment to reflect the new 'both' orientation option.The JSDoc comment should be updated to include the new 'both' value that was added to the orientation type.
- * @property {('horizontal'|'vertical')} orientation - The navigation orientation for arrow keys + * @property {('horizontal'|'vertical'|'both')} orientation - The navigation orientation for arrow keyssrc/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
11-13: Replace empty interface with type alias.An empty interface is equivalent to
{}and should be a type alias instead, following TypeScript best practices.-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +type RadioButtonProps = { + // Add any props you want to pass to RadioButton here +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/core/primitives/Radio/RadioPrimitive.stories.tsx(2 hunks)src/core/primitives/Radio/index.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(2 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/primitives/RadioGroup/RadioGroupPrimitive.tsx
- src/core/primitives/Radio/RadioPrimitive.stories.tsx
- src/core/primitives/Radio/index.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (1)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (4)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 10-13: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (1)
9-9: LGTM! The 'both' orientation addition enhances navigation flexibility.The addition of 'both' as a valid orientation value allows roving focus groups to handle both horizontal and vertical navigation simultaneously, which is valuable for complex UI patterns.
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (2)
17-17: LGTM! Orientation type extension aligns with context changes.The addition of "both" to the orientation prop type is consistent with the changes made to the context type and enables flexible navigation behavior.
56-56: LGTM! Proper ARIA attribute handling for 'both' orientation.The conditional handling of
aria-orientationis correct - when orientation is 'both', omitting the attribute is appropriate since there's no single orientation to specify. This follows ARIA best practices.src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (4)
185-185: LGTM! Proper keyboard navigation support for 'both' orientation.The extension of the condition to include 'both' orientation correctly enables vertical navigation (ArrowUp) when the roving focus group supports bidirectional navigation.
192-192: LGTM! Proper keyboard navigation support for 'both' orientation.The extension of the condition to include 'both' orientation correctly enables horizontal navigation (ArrowLeft) when the roving focus group supports bidirectional navigation.
199-199: LGTM! Proper keyboard navigation support for 'both' orientation.The extension of the condition to include 'both' orientation correctly enables vertical navigation (ArrowDown) when the roving focus group supports bidirectional navigation.
206-206: LGTM! Proper keyboard navigation support for 'both' orientation.The extension of the condition to include 'both' orientation correctly enables horizontal navigation (ArrowRight) when the roving focus group supports bidirectional navigation.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (2)
6-17: LGTM! Comprehensive prop additions enhance RadioGroup functionality.The addition of
asChild,disabled,required,name,orientation, andloopprops significantly enhances the RadioGroup's flexibility and accessibility features. The type definitions are well-structured.
39-40: LGTM! Excellent integration with RovingFocusGroup for keyboard navigation.The wrapping with
RovingFocusGroup.Rootand the neworientationandloopprops provide proper keyboard navigation support for radio groups, which is essential for accessibility.src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx (3)
13-16: LGTM! Proper context validation with helpful error message.The runtime check for context existence with a descriptive error message is excellent for developer experience and prevents silent failures when the component is used incorrectly.
30-33: LGTM! Good integration with RovingFocusGroup for keyboard navigation.The wrapping with
RovingFocusGroup.Itemand therole='radio'attribute properly integrates the radio item with the roving focus behavior, enhancing keyboard accessibility.
31-31: LGTM! Comprehensive RadioPrimitive prop usage enhances functionality.The addition of
onFocus,disabled,aria-checked, andrequiredprops to the RadioPrimitive component significantly improves the accessibility and user experience of the radio group.src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (2)
5-9: Well-defined interface for radio options.The
Optioninterface provides clear typing for radio button options with appropriate string fields.
48-81: Good typing improvements in InFormTemplate.The component now has proper TypeScript typing for options, state, and event handlers. The form submission logic is correctly implemented.
Note: The existing issue with
defaultCheckedusage on line 70 has already been addressed in previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
70-70: Fix controlled component behavior with defaultChecked.The current usage of
defaultChecked={language}with a dynamic state value is problematic. ThedefaultCheckedprop only sets the initial selection and doesn't update when thelanguagestate changes.Choose one of the following solutions:
Option 1 (Recommended): Use uncontrolled mode with static initial value
-<RadioGroupPrimitive.Root defaultChecked={language} onChange={handleChange} > +<RadioGroupPrimitive.Root defaultChecked="html" onChange={handleChange} >Option 2: If controlled mode is needed, extend RadioGroupPrimitiveRoot to support a
valueprop-<RadioGroupPrimitive.Root defaultChecked={language} onChange={handleChange} > +<RadioGroupPrimitive.Root value={language} onChange={handleChange} >Note: Option 2 requires updating the RadioGroupPrimitiveRoot component to accept a
valueprop for controlled behavior.
🧹 Nitpick comments (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
11-13: Replace empty interface with type alias.The empty
RadioButtonPropsinterface should be replaced with a type alias as suggested by the static analysis tool.-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +type RadioButtonProps = Record<string, never>;Alternatively, if you expect to add props later, consider using:
-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +interface RadioButtonProps { + // TODO: Add props as needed +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(2 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(2 hunks)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(2 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(2 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
- src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
- src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 10-13: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (6)
1-1: LGTM: Proper TypeScript imports added.The addition of TypeScript-specific imports for React hooks and event types enhances type safety throughout the component.
5-9: LGTM: Well-defined interface for options.The
Optioninterface provides clear type definitions for the radio button options, improving code maintainability and IDE support.
15-26: LGTM: Improved TypeScript typing in RadioButton component.The explicit typing for the component props, options array, state, and event handlers enhances type safety and developer experience.
29-29: LGTM: Enhanced RadioGroupPrimitive with orientation and direction props.The addition of explicit
orientation='horizontal'anddir='rtl'props aligns with the API improvements mentioned in the PR objectives and provides better accessibility support.
43-43: LGTM: Proper TypeScript typing for render function.The explicit typing of the render function args parameter improves type safety in the Storybook configuration.
49-66: LGTM: Enhanced TypeScript typing in InFormTemplate.The consistent application of TypeScript typing for the options array, state, and event handlers follows the same pattern as the RadioButton component.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (3)
24-32: Improve radio input selection logicThe
getRadioByTexthelper might be fragile if the DOM structure changes. Consider using more robust selection methods.function getRadioByText(label: string): HTMLInputElement { - // Find the radio input by traversing from the text node - const textNode = screen.getByText(label); - const parent = textNode.closest('div'); - if (!parent) throw new Error('No parent div found for label: ' + label); - const radio = parent.querySelector('input[type="radio"]'); - if (!radio) throw new Error('No radio input found for label: ' + label); - return radio as HTMLInputElement; + // Find radio by aria-labelledby or use more robust selection + const radio = screen.getByRole('radio', { name: label }); + return radio as HTMLInputElement; }
111-120: Enhance keyboard navigation testThe current test for the
loopprop is too basic and doesn't actually verify looping behavior. Consider adding more comprehensive keyboard navigation tests.it('supports loop prop (keyboard navigation)', () => { renderGroup({ loop: true }); - // Focus the first radio and press ArrowRight to loop const radios = options.map(({ label }) => getRadioByText(label)); - if (radios[0]) (radios[0] as HTMLInputElement).focus(); - fireEvent.keyDown(radios[0], { key: 'ArrowRight' }); - // This test is basic, as focus management is handled by RovingFocusGroup - // and would require more integration tests for full coverage - expect(radios[1]).toBeDefined(); + + // Focus first radio and navigate to second + radios[0].focus(); + fireEvent.keyDown(radios[0], { key: 'ArrowDown' }); + expect(document.activeElement).toBe(radios[1]); + + // Navigate to third + fireEvent.keyDown(radios[1], { key: 'ArrowDown' }); + expect(document.activeElement).toBe(radios[2]); + + // Test looping: from last to first + fireEvent.keyDown(radios[2], { key: 'ArrowDown' }); + expect(document.activeElement).toBe(radios[0]); });
100-109: Add verification for the thrown error typeThe test verifies that an error is thrown but doesn't check the error message or type, which could help ensure the right error is being thrown.
it('throws if Item is used outside Root', () => { // Suppress error output for this test const spy = jest.spyOn(console, 'error').mockImplementation(() => {}); expect(() => render( <RadioGroupPrimitive.Item value="test">Test</RadioGroupPrimitive.Item> ) - ).toThrow(); + ).toThrow(/RadioGroupPrimitive\.Item must be used within RadioGroupPrimitive\.Root/); spy.mockRestore(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/primitives/Radio/index.tsx(2 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(2 hunks)src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/primitives/Radio/index.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (2)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
🪛 GitHub Actions: Build RAD UI
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx
[error] 51-62: Test failure due to unhandled TypeError: Cannot set properties of undefined (setting 'checked') in RadioGroupPrimitive tests. This causes multiple test failures including 'calls onChange with correct value' and 'only allows one item to be checked at a time'.
[error] 44-44: Test 'checks the defaultChecked value' failed: Expected radio input to be checked but it was not.
[error] 85-85: Test 'passes required and name props to input' failed: Expected input to have attribute name='test-group' but received name='radio'.
[error] 151-151: Test 'works correctly inside a form and submits checked value' failed: Expected submitted value 'js' but received null.
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx
Outdated
Show resolved
Hide resolved
| it('works correctly inside a form and submits checked value', () => { | ||
| let submittedValue: FormDataEntryValue | null = null; | ||
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
| const formData = new FormData(e.target as HTMLFormElement); | ||
| submittedValue = formData.get('fav'); | ||
| }; | ||
| render( | ||
| <form onSubmit={handleSubmit}> | ||
| <RadioGroupPrimitive.Root name="fav" defaultChecked="css"> | ||
| {options.map((option) => ( | ||
| <RadioGroupPrimitive.Item key={option.value} value={option.value}> | ||
| {option.label} | ||
| </RadioGroupPrimitive.Item> | ||
| ))} | ||
| </RadioGroupPrimitive.Root> | ||
| <button type="submit">Submit</button> | ||
| </form> | ||
| ); | ||
| // Change selection | ||
| const radio = getRadioByText('JavaScript'); | ||
| fireEvent.click(radio); | ||
| fireEvent.click(screen.getByText('Submit')); | ||
| expect(submittedValue).toBe('js'); | ||
| }); |
There was a problem hiding this comment.
Fix form submission test - use correct prop name
The test fails because it uses defaultChecked instead of defaultValue, and the name prop issue also affects form data collection.
it('works correctly inside a form and submits checked value', () => {
let submittedValue: FormDataEntryValue | null = null;
const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => {
e.preventDefault();
const formData = new FormData(e.target as HTMLFormElement);
submittedValue = formData.get('fav');
};
render(
<form onSubmit={handleSubmit}>
- <RadioGroupPrimitive.Root name="fav" defaultChecked="css">
+ <RadioGroupPrimitive.Root name="fav" defaultValue="css">
{options.map((option) => (
<RadioGroupPrimitive.Item key={option.value} value={option.value}>
{option.label}
</RadioGroupPrimitive.Item>
))}
</RadioGroupPrimitive.Root>
<button type="submit">Submit</button>
</form>
);
// Change selection
const radio = getRadioByText('JavaScript');
fireEvent.click(radio);
fireEvent.click(screen.getByText('Submit'));
expect(submittedValue).toBe('js');
});📝 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.
| it('works correctly inside a form and submits checked value', () => { | |
| let submittedValue: FormDataEntryValue | null = null; | |
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | |
| e.preventDefault(); | |
| const formData = new FormData(e.target as HTMLFormElement); | |
| submittedValue = formData.get('fav'); | |
| }; | |
| render( | |
| <form onSubmit={handleSubmit}> | |
| <RadioGroupPrimitive.Root name="fav" defaultChecked="css"> | |
| {options.map((option) => ( | |
| <RadioGroupPrimitive.Item key={option.value} value={option.value}> | |
| {option.label} | |
| </RadioGroupPrimitive.Item> | |
| ))} | |
| </RadioGroupPrimitive.Root> | |
| <button type="submit">Submit</button> | |
| </form> | |
| ); | |
| // Change selection | |
| const radio = getRadioByText('JavaScript'); | |
| fireEvent.click(radio); | |
| fireEvent.click(screen.getByText('Submit')); | |
| expect(submittedValue).toBe('js'); | |
| }); | |
| it('works correctly inside a form and submits checked value', () => { | |
| let submittedValue: FormDataEntryValue | null = null; | |
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | |
| e.preventDefault(); | |
| const formData = new FormData(e.target as HTMLFormElement); | |
| submittedValue = formData.get('fav'); | |
| }; | |
| render( | |
| <form onSubmit={handleSubmit}> | |
| <RadioGroupPrimitive.Root name="fav" defaultValue="css"> | |
| {options.map((option) => ( | |
| <RadioGroupPrimitive.Item key={option.value} value={option.value}> | |
| {option.label} | |
| </RadioGroupPrimitive.Item> | |
| ))} | |
| </RadioGroupPrimitive.Root> | |
| <button type="submit">Submit</button> | |
| </form> | |
| ); | |
| // Change selection | |
| const radio = getRadioByText('JavaScript'); | |
| fireEvent.click(radio); | |
| fireEvent.click(screen.getByText('Submit')); | |
| expect(submittedValue).toBe('js'); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Build RAD UI
[error] 151-151: Test 'works correctly inside a form and submits checked value' failed: Expected submitted value 'js' but received null.
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx lines 128 to
152, the test uses the incorrect prop name `defaultChecked` on
RadioGroupPrimitive.Root, which causes the form submission to fail capturing the
selected value. Replace `defaultChecked` with `defaultValue` to correctly set
the initial selected radio button. Also verify that the `name` prop is correctly
set on the RadioGroupPrimitive.Root to ensure form data is collected properly.
| it('passes required and name props to input', () => { | ||
| renderGroup({ required: true, name: 'test-group' }); | ||
| const radio = getRadioByText('HTML'); | ||
| expect(radio).toBeRequired(); | ||
| expect(radio).toHaveAttribute('name', 'test-group'); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix name prop forwarding issue
The test expects name='test-group' but receives name='radio', indicating the name prop isn't being properly forwarded to the radio inputs.
This suggests the RadioGroupPrimitive implementation isn't correctly passing the name prop to individual radio inputs. The implementation should ensure all radio items in a group share the same name attribute.
🏁 Script executed:
#!/bin/bash
# Check how the name prop is handled in RadioGroupPrimitive
echo "Searching for name prop usage in RadioGroup implementation..."
rg -A 5 -B 5 "name.*prop" src/core/primitives/RadioGroup/
echo "Looking for input element creation..."
ast-grep --pattern 'input[type="radio"]'Length of output: 2643
🏁 Script executed:
#!/bin/bash
# List all RadioGroup primitive fragment files
echo "Fragment files:"
fd -e tsx RadioGroupPrimitive* src/core/primitives/RadioGroup/fragments
# Search for context creation to see what values are provided
echo -e "\ncreateContext occurrences:"
rg -n "createContext" src/core/primitives/RadioGroup/fragments
# Search within provider usage for 'name' to see if it's included
echo -e "\nContext value includes 'name'?:"
rg -n "value\s*=\s*{[^}]*name" -n src/core/primitives/RadioGroup/fragments
# Find where input[type="radio"] is rendered
echo -e "\n<input type=\"radio\" occurrences:"
rg -n "<input[^>]*type=[\"']radio[\"']" -n src/core/primitives/RadioGroup/fragmentsLength of output: 837
🏁 Script executed:
#!/bin/bash
# Locate where the RadioGroup context is defined
echo "Searching for createContext in RadioGroup directory..."
rg -n createContext src/core/primitives/RadioGroup
# If a context file is found, display its contents
echo -e "\nContext definition file content (first 200 lines):"
rg -l createContext src/core/primitives/RadioGroup | xargs -I{} sed -n '1,200p' {}
# Show how the Provider is used in the Root component
echo -e "\nProvider usage in RadioGroupPrimitiveRoot.tsx:"
rg -n "Provider" -n src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
rg -n "value\s*=" -n src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
# Inspect the Input element in the Item component
echo -e "\n<input type=\"radio\" in RadioGroupPrimitiveItem.tsx:"
rg -n '<input[^>]*type=[\"'"'"']radio[\"'"'"']' -n src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
sed -n '1,200p' src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsxLength of output: 3216
Update RadioGroupPrimitiveItem to forward context.name
The name prop is currently hard-coded to 'radio' instead of using the name value from context. This causes tests like:
expect(radio).toHaveAttribute('name', 'test-group');to fail, since inputs all get name="radio".
Locations to fix:
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx, inside the
<RadioPrimitive>invocation.
Proposed change:
- <RadioPrimitive
- name='radio'
+ <RadioPrimitive
+ name={name}
value={value}
checked={checkedItem === value}
onChange={handleOnChange}
onFocus={handleOnChange}
disabled={groupDisabled || disabled}
aria-checked={checkedItem === value}
required
/>This ensures the name passed to each radio input comes from the RadioGroup’s name prop and satisfies the existing test.
🧰 Tools
🪛 GitHub Actions: Build RAD UI
[error] 85-85: Test 'passes required and name props to input' failed: Expected input to have attribute name='test-group' but received name='radio'.
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx around
the RadioPrimitive component, the name prop is currently hard-coded to 'radio'
instead of using the name from context. To fix this, update the RadioPrimitive
invocation to forward the name value from the RadioGroup context rather than a
fixed string. This will ensure the input elements receive the correct name prop,
matching the RadioGroup's name and making the test expecting 'test-group' pass.
| it('calls onChange with correct value', () => { | ||
| const handleChange = jest.fn(); | ||
| renderGroup({ onChange: handleChange }); | ||
| const radio = getRadioByText('JavaScript'); | ||
| fireEvent.click(radio); | ||
| expect(handleChange).toHaveBeenCalledWith('js'); | ||
| expect(radio).toBeChecked(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test may fail due to implementation issues
The pipeline failure indicates a TypeError when setting the 'checked' property. This suggests the underlying RadioGroupPrimitive implementation has bugs in how it manages radio state.
🏁 Script executed:
#!/bin/bash
# Check the RadioGroupPrimitive implementation for potential issues
echo "Searching for RadioGroupPrimitive implementation..."
fd -e tsx -e ts RadioGroupPrimitive | head -10
echo "Looking for how radio inputs are managed..."
rg -A 10 "checked.*=" src/core/primitives/RadioGroup/Length of output: 6245
🏁 Script executed:
#!/bin/bash
FILE=src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
echo "==== Top of RadioGroupPrimitiveItem.tsx ===="
head -n20 $FILE
echo
echo "==== handleOnChange snippet ===="
sed -n '1,200p' $FILE | sed -n '/const handleOnChange/,/};/p'
echo
echo "==== Searching for RadioPrimitive definition ===="
rg -R "function RadioPrimitive" -e tsx -e ts || true
rg -R "const RadioPrimitive" -e tsx -e ts || true
rg -R "export default RadioPrimitive" -e tsx -e ts || trueLength of output: 3050
🏁 Script executed:
#!/bin/bash
echo "Locating RadioPrimitive files under src/core/primitives/Radio"
fd -t f -e tsx -e ts src/core/primitives/Radio
echo
echo "Displaying Radio.tsx or index.tsx content if present"
for f in src/core/primitives/Radio.tsx src/core/primitives/Radio/index.tsx src/core/primitives/Radio/index.ts; do
if [ -f "$f" ]; then
echo "==== $f ===="
head -n200 "$f"
echo
fi
doneLength of output: 2678
🏁 Script executed:
#!/bin/bash
echo "Locating Primitive component"
fd -t f -e tsx -e ts primitives/Primitive --base-path src/core
echo
echo "Displaying Primitive.tsx content"
FILE=src/core/primitives/Primitive.tsx
if [ -f "$FILE" ]; then
sed -n '1,200p' "$FILE"
else
echo "Primitive.tsx not found"
fiLength of output: 611
Fix onChange forwarding in RadioPrimitive to satisfy RadioGroup handler
The TypeError in your tests stems from a mismatch between RadioPrimitive’s onChange payload and what RadioGroupPrimitiveItem expects. RadioPrimitive currently calls:
onChange({
value,
checked: isChecked
});so in the group item, handleOnChange(event) receives an object without event.target, causing event.target.checked = true to throw.
Please update:
- src/core/primitives/Radio/index.tsx – forward the native event to consumers instead of a custom object.
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx – you can then rely on
event.target.checkedor readevent.target.valuedirectly.
Suggested patch for RadioPrimitive:
--- a/src/core/primitives/Radio/index.tsx
+++ b/src/core/primitives/Radio/index.tsx
@@ const RadioPrimitive = ({ /*…*/ }: RadioPrimitiveProps) => {
- const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
- const isChecked = event.target.checked;
- console.log(isChecked);
- if (typeof onChange === 'function') {
- setIsChecked(isChecked);
- onChange({
- value,
- checked: isChecked
- });
- }
- };
+ const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+ const isChecked = event.target.checked;
+ setIsChecked(isChecked);
+ if (typeof onChange === 'function') {
+ onChange(event); // forward the original event
+ }
+ };With that change, in RadioGroupPrimitiveItem.tsx your existing handler can safely do:
const handleOnChange = (event: React.ChangeEvent<HTMLInputElement>) => {
// event.target is now defined
setCheckedItem(value);
onChange?.(value);
};This aligns both components and should resolve the TypeError in your test suite.
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx around lines
47 to 54, the test fails due to RadioPrimitive forwarding a custom object
instead of the native event, causing a TypeError when RadioGroupPrimitiveItem
tries to access event.target.checked. To fix this, update
src/core/primitives/Radio/index.tsx to forward the native change event to the
onChange handler instead of a custom object, and adjust
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx to handle
the native event by accessing event.target.checked and event.target.value
directly. This will align the event handling between components and resolve the
test error.
|
|
||
| RadioGroupPrimitive.Root = RadioGroupPrimitiveRoot; | ||
| RadioGroupPrimitive.Item = RadioGroupPrimitiveItem; | ||
| const RadioGroupPrimitive: RadioGroupPrimitiveType = { |
There was a problem hiding this comment.
the previous declaration is valid, we use this pattern/style everywhere else, so do other libs
we should make the ts lint work our way, not the other way around
There was a problem hiding this comment.
fixed, thanks for pointing out
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
15-38: Fix unused handleChange function and missing controlled behavior.The
handleChangefunction is defined but never used, and theRadioGroupPrimitive.Rootdoesn't receive thelanguagestate or change handler. This creates a disconnect between the component's state and the radio group's behavior.const RadioButton = (args: RadioButtonProps) => { const options: Option[] = [ { id: 'html', value: 'html', label: 'HTML' }, { id: 'css', value: 'css', label: 'CSS' }, { id: 'javascript', value: 'javascript', label: 'JavaScript' } ]; const [language, setLanguage] = useState<string>('css'); const handleChange = (item: string) => { setLanguage(item); }; return ( <SandboxEditor> - <RadioGroupPrimitive.Root orientation='horizontal' dir='rtl' name='test'> + <RadioGroupPrimitive.Root + orientation='horizontal' + dir='rtl' + name='test' + value={language} + onValueChange={handleChange} + > {options.map((option) => ( <RadioGroupPrimitive.Item key={option.id} value={option.value}> {option.label} </RadioGroupPrimitive.Item> ))} </RadioGroupPrimitive.Root> </SandboxEditor> ); };
🧹 Nitpick comments (2)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (2)
11-13: Replace empty interface with type alias.The empty interface
RadioButtonPropsshould be replaced with a type alias for better TypeScript practices.-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +type RadioButtonProps = Record<string, never>;Alternatively, if no props are needed, you can remove the interface entirely and use
{}directly in the function parameter.
92-95: Remove unnecessary blank lines.Multiple consecutive blank lines should be cleaned up for better code consistency.
- - - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/primitives/Radio/index.tsx(2 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(1 hunks)src/core/primitives/RadioGroup/context/RadioGroupContext.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/primitives/RadioGroup/context/RadioGroupContext.tsx
- src/core/primitives/Radio/index.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 10-13: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
48-90: Good implementation of controlled radio group usage.The
InFormcomponent correctly demonstrates controlled usage with proper state management, form submission handling, and value synchronization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (5)
10-10: Fix prop name: usedefaultValueinstead ofdefaultCheckedBased on the past review comments, the RadioGroupPrimitive uses
defaultValueto set the initially selected option, notdefaultChecked.
43-43: Fix prop name: usedefaultValueinstead ofdefaultCheckedConsistent with other tests, this should use
defaultValueinstead ofdefaultChecked.
56-56: Fix prop name: usedefaultValueinstead ofdefaultCheckedConsistent with other tests, this should use
defaultValueinstead ofdefaultChecked.
69-69: Fix prop name: usedefaultValueinstead ofdefaultCheckedConsistent with other tests, this should use
defaultValueinstead ofdefaultChecked.
74-74: Verify name prop forwarding implementationBased on past review comments, the test expects
name='my-radio-group'but the implementation might be forwardingname='radio'instead. Ensure theRadioGroupPrimitiveItemcomponent correctly forwards thenamefrom context.src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
35-48: Fix ARIA attributes and improve RovingFocusGroup integrationSeveral issues need to be addressed:
aria-disabledshould be a string ('true'/'false'), not boolean- Missing
asChildprop onPrimitive.divRovingFocusGroupcomponents aren't receiving the orientation, loop, and dir props- Poor formatting
Apply this fix:
- return ( - <RovingFocusGroup.Root> - <RadioGroupContext.Provider value={sendItems}> - <RovingFocusGroup.Group> - <Primitive.div {...props} aria-required={required} role='radiogroup' aria-disabled={groupDisabled}> - - {children} - - </Primitive.div> - </RovingFocusGroup.Group> - </RadioGroupContext.Provider> - </RovingFocusGroup.Root> - ) - ; + return ( + <RovingFocusGroup.Root orientation={orientation} loop={loop} dir={dir}> + <RadioGroupContext.Provider value={sendItems}> + <RovingFocusGroup.Group> + <Primitive.div + {...props} + aria-required={required} + role='radiogroup' + aria-disabled={groupDisabled ? 'true' : 'false'} + asChild={props.asChild} + > + {children} + </Primitive.div> + </RovingFocusGroup.Group> + </RadioGroupContext.Provider> + </RovingFocusGroup.Root> + );
🧹 Nitpick comments (3)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (2)
11-13: Fix empty interface issueAs indicated by static analysis, an empty interface is equivalent to
{}. Use a type alias instead.Apply this fix:
-interface RadioButtonProps { - // Add any props you want to pass to RadioButton here -} +type RadioButtonProps = {}
22-26: Remove unused state and handlerThe
languagestate andhandleChangefunction are defined but never used in theRadioButtoncomponent. Clean up by removing unused code.- const [language, setLanguage] = useState<string>('css'); - - const handleChange = (item: string) => { - setLanguage(item); - };src/core/primitives/Radio/index.tsx (1)
21-21: Consider making tabIndex configurableThe hardcoded
tabIndex={-1}might not be appropriate for all use cases. Consider making it configurable or removing it to allow natural tab order.- tabIndex={-1} + tabIndex={props.tabIndex ?? -1}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/ui/TabNav/fragments/TabNavLink.tsx(0 hunks)src/core/primitives/Primitive/index.tsx(1 hunks)src/core/primitives/Radio/RadioPrimitive.stories.tsx(2 hunks)src/core/primitives/Radio/index.tsx(1 hunks)src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx(1 hunks)src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx(1 hunks)src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/TabNav/fragments/TabNavLink.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/primitives/Radio/RadioPrimitive.stories.tsx
- src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveItem.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
src/core/primitives/RadioGroup/tests/RadioGroupPrimtive.test.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/Primitive/index.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:18.599Z
Learning: When the keys and values for the `as` prop are identical, it's preferable to use a union type directly for the `as` prop and eliminate unnecessary mappings like `tagMap`.
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (3)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (4)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
Learnt from: krau5
PR: rad-ui/ui#609
File: src/components/ui/Text/Text.tsx:19-22
Timestamp: 2024-12-06T14:30:05.834Z
Learning: When the keys and values for the `as` prop are the same, we should define the type directly using string literals (e.g., `'div' | 'span' | 'p' | 'label'`) and eliminate extra mappings like `tagMap` to simplify the code.
src/core/primitives/Radio/index.tsx (3)
Learnt from: kotAPI
PR: rad-ui/ui#576
File: src/core/primitives/Toggle/index.tsx:15-22
Timestamp: 2024-11-24T06:43:42.194Z
Learning: In the `TogglePrimitive` component (`src/core/primitives/Toggle/index.tsx`), when the component becomes controlled, it's acceptable to not sync the internal `isPressed` state with the external `pressed` prop.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
🧬 Code Graph Analysis (1)
src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (2)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)src/components/ui/RadioGroup/context/RadioGroupContext.tsx (1)
RadioGroupContext(3-7)
🪛 Biome (1.9.4)
src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx
[error] 10-13: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/core/primitives/Primitive/index.tsx (1)
8-10: Excellent type safety improvement!The union type approach correctly differentiates between input elements and other HTML elements, providing better type specificity. This change supports the updated
RadioPrimitivecomponent that now passes explicit input-related props toPrimitive.input.src/core/primitives/RadioGroup/fragments/RadioGroupPrimitiveRoot.tsx (1)
21-26: Excellent controlled/uncontrolled state implementation!The use of
useControllableStatecorrectly implements the controlled/uncontrolled pattern following the established learning about initializing fromdefaultValue, updating internal state when controlled, and callingonValueChangecallback.src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx (1)
73-77: Excellent controlled component example!The
InFormstory correctly demonstrates controlled usage withvalueandonValueChangeprops, following the established pattern for controlled components.src/core/primitives/Radio/index.tsx (1)
4-13: Good type definition improvements!The updated
RadioPrimitivePropstype correctly includes all necessary props for a radio input element. The addition ofid,required,disabled, andasChildprops enhances the component's flexibility.
| <RadioGroupPrimitive.Root | ||
| orientation='horizontal' dir='rtl' | ||
| name='test' | ||
| // defaultValue='css' | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defaultValue for uncontrolled mode
The RadioButton story should demonstrate uncontrolled usage with a defaultValue prop, as the commented line suggests.
<RadioGroupPrimitive.Root
orientation='horizontal' dir='rtl'
name='test'
- // defaultValue='css'
+ defaultValue='css'
>📝 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.
| <RadioGroupPrimitive.Root | |
| orientation='horizontal' dir='rtl' | |
| name='test' | |
| // defaultValue='css' | |
| > | |
| <RadioGroupPrimitive.Root | |
| orientation='horizontal' dir='rtl' | |
| name='test' | |
| defaultValue='css' | |
| > |
🤖 Prompt for AI Agents
In src/core/primitives/RadioGroup/RadioGroupPrimitive.stories.tsx around lines
29 to 33, the RadioGroupPrimitive.Root component is missing the defaultValue
prop needed for uncontrolled mode. Uncomment the defaultValue='css' line to
properly demonstrate uncontrolled usage in the story.
| function RadioPrimitive({ name, value, id, checked, required, onChange, disabled, asChild, ...props }: RadioPrimitiveProps) { | ||
| return ( | ||
| <Primitive.input id={value} type='radio' value={value} name={name} onChange={handleOnChange} {...props} {...dataAttributes}/> | ||
| <Primitive.input | ||
| type="radio" | ||
| checked={checked} | ||
| name={name} | ||
| tabIndex={-1} | ||
| value={value} | ||
| onChange={onChange} | ||
| id={value} | ||
| aria-disabled={disabled} | ||
| disabled={disabled} | ||
| required={required} | ||
| aria-required={required} | ||
| asChild={asChild} | ||
| {...props} | ||
| /> | ||
| ); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix id prop usage and verify onChange signature
Two issues need attention:
- The
idprop from props is not being used - line 24 setsid={value}instead ofid={id} - The
onChangesignature changed from accepting an event to no arguments - ensure this is intentional and compatible with parent components
Apply this fix:
<Primitive.input
type="radio"
checked={checked}
name={name}
tabIndex={-1}
value={value}
onChange={onChange}
- id={value}
+ id={id}
aria-disabled={disabled}
disabled={disabled}
required={required}
aria-required={required}
asChild={asChild}
{...props}
/>📝 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.
| function RadioPrimitive({ name, value, id, checked, required, onChange, disabled, asChild, ...props }: RadioPrimitiveProps) { | |
| return ( | |
| <Primitive.input id={value} type='radio' value={value} name={name} onChange={handleOnChange} {...props} {...dataAttributes}/> | |
| <Primitive.input | |
| type="radio" | |
| checked={checked} | |
| name={name} | |
| tabIndex={-1} | |
| value={value} | |
| onChange={onChange} | |
| id={value} | |
| aria-disabled={disabled} | |
| disabled={disabled} | |
| required={required} | |
| aria-required={required} | |
| asChild={asChild} | |
| {...props} | |
| /> | |
| ); | |
| }; | |
| } | |
| function RadioPrimitive({ name, value, id, checked, required, onChange, disabled, asChild, ...props }: RadioPrimitiveProps) { | |
| return ( | |
| <Primitive.input | |
| type="radio" | |
| checked={checked} | |
| name={name} | |
| tabIndex={-1} | |
| value={value} | |
| onChange={onChange} | |
| id={id} | |
| aria-disabled={disabled} | |
| disabled={disabled} | |
| required={required} | |
| aria-required={required} | |
| asChild={asChild} | |
| {...props} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/core/primitives/Radio/index.tsx between lines 15 and 33, the id prop is
incorrectly set to the value prop; update the id attribute to use the id prop
instead of value. Also, verify that the onChange handler's signature matches the
expected usage by parent components—if onChange no longer receives an event
argument, confirm this change is intentional and adjust the handler accordingly
to maintain compatibility.
kotAPI
added
the
automerge
closes #1192 #1193
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation
Tests