Conversation
🦋 Changeset detectedLatest commit: f2f78d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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). (3)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (2)
35-36: Update ID prefix to match Combobox naming.The fallback ID uses
select-item-${index}, which is a leftover from the Select component refactoring. It should be updated tocombobox-item-${index}for consistency.🔎 Proposed fix
// Use the value prop for the ID, fallback to index if value is not provided - const itemId = value || `select-item-${index}`; + const itemId = value || `combobox-item-${index}`;
84-101: Remove duplicatetabIndexprop.The
tabIndexis set both insidegetItemProps(line 85) and directly on the element (line 101). The second declaration will override the first, making the one ingetItemPropsineffective and potentially causing unexpected behavior.🔎 Proposed fix
{...getItemProps({ tabIndex: disabled ? -1 : isActive ? 0 : -1, onClick: () => !disabled && handleSelect(index), onKeyDown: (event: React.KeyboardEvent) => { if (disabled) return; if (event.key === 'Enter') { event.preventDefault(); handleSelect(index); } if (event.key === ' ' && !isTypingRef.current) { event.preventDefault(); handleSelect(index); } } })} {...props} - tabIndex={disabled ? -1 : isActive ? 0 : -1} >
🧹 Nitpick comments (7)
src/components/ui/Combobox/fragments/ComboboxSearch.tsx (1)
8-19: Simplify by making the component self-closing.The
ComboboxPrimitive.Searchelement has empty children (lines 15-17). Since no children are rendered, make it self-closing for cleaner code.🔎 Proposed refactor
const ComboboxSearch = React.forwardRef<ComboboxSearchElement, ComboboxSearchProps>((props, forwardedRef) => { const { rootClass } = useContext(ComboboxRootContext); return ( <ComboboxPrimitive.Search className={`${rootClass}-search`} ref={forwardedRef} {...props} - > - - </ComboboxPrimitive.Search> + /> ); });src/components/ui/Combobox/fragments/ComboboxIndicator.tsx (1)
8-17: Consider making the indicator icon customizable.The SVG checkmark is hardcoded. For greater flexibility, consider accepting an optional
iconprop to allow users to customize the indicator.🔎 Proposed enhancement
type ComboboxIndicatorElement = React.ElementRef<'div'>; -type ComboboxIndicatorProps = React.ComponentPropsWithoutRef<'div'>; +type ComboboxIndicatorProps = React.ComponentPropsWithoutRef<'div'> & { + icon?: React.ReactNode; +}; -const ComboboxIndicator = React.forwardRef<ComboboxIndicatorElement, ComboboxIndicatorProps>((props, forwardedRef) => { +const ComboboxIndicator = React.forwardRef<ComboboxIndicatorElement, ComboboxIndicatorProps>(({ icon, ...props }, forwardedRef) => { const { rootClass } = useContext(ComboboxRootContext); return ( <div className={`${rootClass}-item-indicator`} ref={forwardedRef} {...props}> - <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M13.3332 4L5.99984 11.3333L2.6665 8" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> - </svg> + {icon || ( + <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> + <path d="M13.3332 4L5.99984 11.3333L2.6665 8" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> + </svg> + )} </div> ); });src/components/ui/Combobox/fragments/ComboboxItem.tsx (1)
7-9: Remove unusedcustomRootClassprop.The
customRootClassprop is defined in the type signature and destructured in the component, but it's never used in the implementation. Consider removing it to keep the API clean.🔎 Proposed refactor
type ComboboxItemProps = React.ComponentPropsWithoutRef<typeof ComboboxPrimitive.Item> & { - customRootClass?: string; }; -const ComboboxItem = React.forwardRef<ComboboxItemElement, ComboboxItemProps>(({ customRootClass, children, value, disabled, ...props }, forwardedRef) => { +const ComboboxItem = React.forwardRef<ComboboxItemElement, ComboboxItemProps>(({ children, value, disabled, ...props }, forwardedRef) => {Also applies to: 11-11
src/components/ui/Combobox/fragments/ComboboxTrigger.tsx (1)
8-10: Remove unusedcustomRootClassprop.The
customRootClassprop is defined in the type signature and destructured in the component, but it's never used in the implementation. Consider removing it to keep the API clean.🔎 Proposed refactor
type ComboboxTriggerProps = React.ComponentPropsWithoutRef<typeof ComboboxPrimitive.Trigger> & { - customRootClass?: string; placeholder?: boolean; }; -const ComboboxTrigger = React.forwardRef<ComboboxTriggerElement, ComboboxTriggerProps>(({ customRootClass, children, disabled, placeholder, ...props }, forwardedRef) => { +const ComboboxTrigger = React.forwardRef<ComboboxTriggerElement, ComboboxTriggerProps>(({ children, disabled, placeholder, ...props }, forwardedRef) => {Also applies to: 13-13
src/components/ui/Combobox/fragments/ComboboxContent.tsx (1)
7-9: Remove unusedcustomRootClassprop.The
customRootClassprop is defined in the type signature and destructured in the component, but it's never used in the implementation. Consider removing it to keep the API clean.🔎 Proposed refactor
type ComboboxContentProps = React.ComponentPropsWithoutRef<typeof ComboboxPrimitive.Content> & { - customRootClass?: string; }; -const ComboboxContent = React.forwardRef<ComboboxContentElement, ComboboxContentProps>(({ customRootClass, children, position = 'popper', ...props }, forwardedRef) => { +const ComboboxContent = React.forwardRef<ComboboxContentElement, ComboboxContentProps>(({ children, position = 'popper', ...props }, forwardedRef) => {Also applies to: 11-11
src/core/primitives/Combobox/stories/Combobox.stories.tsx (2)
45-45: Fix typo in trigger text."helo" should be "hello".
🔎 Proposed fix
<ComboboxPrimitive.Trigger> - helo + hello </ComboboxPrimitive.Trigger>
154-169: Clarify commented Portal usage.The
ComboboxPrimitive.Portalis commented out in theSearchComboboxstory. If this is intentional for demonstration purposes (e.g., showing in-place rendering vs. portal rendering), consider adding a comment explaining why. Otherwise, ensure consistency with other stories.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
src/components/ui/Combobox/Combobox.tsx(1 hunks)src/components/ui/Combobox/contexts/ComboboxRootContext.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxContent.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxGroup.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxIndicator.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxItem.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxPortal.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxRoot.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxSearch.tsx(1 hunks)src/components/ui/Combobox/fragments/ComboboxTrigger.tsx(1 hunks)src/components/ui/Combobox/stories/Combobox.stories.tsx(1 hunks)src/components/ui/Combobox/tests/Combobox.full.test.tsx(1 hunks)src/components/ui/Combobox/tests/Combobox.test.tsx(2 hunks)src/components/ui/Select/Select.tsx(0 hunks)src/components/ui/Select/fragments/SelectContent.tsx(1 hunks)src/components/ui/Select/fragments/SelectGroup.tsx(1 hunks)src/components/ui/Select/fragments/SelectItem.tsx(1 hunks)src/components/ui/Select/fragments/SelectPortal.tsx(1 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(1 hunks)src/components/ui/Select/fragments/SelectSearch.tsx(0 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(1 hunks)src/components/ui/Select/stories/Select.stories.tsx(0 hunks)src/core/primitives/Combobox/ComboboxPrimitive.tsx(1 hunks)src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx(4 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx(2 hunks)src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx(2 hunks)src/core/primitives/Combobox/stories/Combobox.stories.tsx(1 hunks)src/core/primitives/Combobox/tests/Combobox.test.tsx(1 hunks)src/core/primitives/Select/Select.tsx(0 hunks)src/core/primitives/Select/stories/Select.stories.tsx(0 hunks)styles/themes/components/combobox.scss(1 hunks)styles/themes/components/select.scss(0 hunks)styles/themes/default.scss(1 hunks)
💤 Files with no reviewable changes (6)
- styles/themes/components/select.scss
- src/components/ui/Select/stories/Select.stories.tsx
- src/components/ui/Select/fragments/SelectSearch.tsx
- src/components/ui/Select/Select.tsx
- src/core/primitives/Select/Select.tsx
- src/core/primitives/Select/stories/Select.stories.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-12T08:22:59.375Z
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
Applied to files:
src/components/ui/Combobox/fragments/ComboboxRoot.tsxsrc/components/ui/Combobox/fragments/ComboboxTrigger.tsxsrc/components/ui/Combobox/Combobox.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 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`.
Applied to files:
styles/themes/components/combobox.scsssrc/core/primitives/Combobox/tests/Combobox.test.tsxsrc/components/ui/Combobox/fragments/ComboboxTrigger.tsxsrc/components/ui/Combobox/stories/Combobox.stories.tsxsrc/components/ui/Combobox/tests/Combobox.test.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsxsrc/components/ui/Combobox/tests/Combobox.full.test.tsxsrc/core/primitives/Combobox/stories/Combobox.stories.tsxsrc/components/ui/Select/fragments/SelectTrigger.tsx
📚 Learning: 2024-11-24T06:43:42.194Z
Learnt from: kotAPI
Repo: rad-ui/ui PR: 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.
Applied to files:
src/components/ui/Select/fragments/SelectPortal.tsxsrc/core/primitives/Combobox/tests/Combobox.test.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsxsrc/components/ui/Select/fragments/SelectTrigger.tsx
📚 Learning: 2025-07-18T16:43:26.175Z
Learnt from: GoldGroove06
Repo: rad-ui/ui PR: 1215
File: src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx:25-27
Timestamp: 2025-07-18T16:43:26.175Z
Learning: In the CheckboxGroupPrimitiveTrigger component (`src/core/primitives/CheckboxGroup/fragments/CheckboxGroupPrimitiveTrigger.tsx`), the component uses two separate useEffect hooks with different purposes: the first useEffect (lines 25-27) with empty dependency array `[]` is meant to set the initial state only once on mount by syncing with the group's checked values, while the second useEffect (lines 28-34) handles ongoing state updates by propagating local `isChecked` changes back to the group's `checkedValues`. This pattern prevents infinite loops while maintaining proper synchronization.
Applied to files:
src/core/primitives/Combobox/tests/Combobox.test.tsxsrc/components/ui/Combobox/fragments/ComboboxTrigger.tsxsrc/components/ui/Combobox/fragments/ComboboxGroup.tsxsrc/components/ui/Select/fragments/SelectGroup.tsxsrc/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsxsrc/components/ui/Select/fragments/SelectTrigger.tsx
🧬 Code graph analysis (16)
src/components/ui/Combobox/fragments/ComboboxIndicator.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/components/ui/Combobox/fragments/ComboboxRoot.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/components/ui/Combobox/fragments/ComboboxItem.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
src/components/ui/Combobox/fragments/ComboboxContent.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/components/ui/Combobox/fragments/ComboboxTrigger.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/components/ui/Combobox/stories/Combobox.stories.tsx (1)
src/components/ui/Select/stories/Select.stories.tsx (1)
Basic(11-33)
src/components/ui/Combobox/fragments/ComboboxGroup.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
src/components/ui/Combobox/tests/Combobox.full.test.tsx (1)
test-utils/index.ts (1)
axe(56-62)
src/components/ui/Combobox/fragments/ComboboxSearch.tsx (1)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
ComboboxRootContext(7-9)
src/core/primitives/Combobox/stories/Combobox.stories.tsx (2)
src/components/ui/Select/stories/Select.stories.tsx (2)
ControlledExample(142-166)GroupExample(168-191)docs/components/layout/Documentation/helpers/ComponentHero/ComponentHero.js (1)
data(32-32)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveItem.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
src/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
ComboboxPrimitiveContext(34-34)
🪛 GitHub Actions: Coverage
src/components/ui/Combobox/Combobox.tsx
[error] 38-38: TS2339: Property 'Search' does not exist on type 'ComboboxComponent'.
🪛 GitHub Actions: Tests
src/components/ui/Combobox/Combobox.tsx
[error] 38-38: TS2339: Property 'Search' does not exist on type 'ComboboxComponent'.
🪛 GitHub Check: lint
src/components/ui/Combobox/tests/Combobox.full.test.tsx
[warning] 147-147:
A form label must be associated with a control
🔇 Additional comments (20)
src/components/ui/Combobox/contexts/ComboboxRootContext.tsx (1)
1-9: LGTM!The context implementation is clean and follows standard React patterns. The empty string default for
rootClassis a safe choice.src/core/primitives/Combobox/tests/Combobox.test.tsx (1)
1-42: LGTM!The test suite comprehensively covers the key behaviors:
- Ref forwarding to the underlying button element
- Hidden select rendering for form integration with proper ARIA attributes
- Clean render without console warnings
Test structure follows best practices with proper mock cleanup.
src/components/ui/Combobox/tests/Combobox.test.tsx (1)
1-42: LGTM!The test migration from SelectPrimitive to Combobox API is clean and complete. All test cases maintain the same coverage:
- Ref forwarding validation
- Hidden select form integration
- Warning-free rendering
src/core/primitives/Combobox/fragments/ComboboxPrimitiveSearch.tsx (1)
7-113: LGTM with minor type safety concern.The component renaming from
SelectPrimitiveSearchtoComboboxPrimitiveSearchand context migration are correctly implemented. However, there are multiple@ts-ignorecomments (lines 19, 24, 88, 95) that suppress TypeScript errors for thetypeattribute and event handlers.Consider replacing
@ts-ignorewith proper type assertions or fixing the underlying type definitions. For example:// Instead of @ts-ignore <Primitive.input type="search" onChange={(e: React.ChangeEvent<HTMLInputElement>) => setSearch(e.target.value)} {...props} />If
Primitive.inputdoesn't support these props correctly, you may need to update the Primitive type definitions.src/components/ui/Combobox/fragments/ComboboxRoot.tsx (1)
13-34: LGTM!The
ComboboxRootcomponent correctly wrapsComboboxPrimitive.Rootwith context provider setup, forwards all necessary props, and applies the root class styling pattern consistently.styles/themes/components/combobox.scss (1)
1-213: LGTM!Comprehensive SCSS styling for the Combobox component with:
- Proper accessibility support (focus states, ARIA attributes, keyboard navigation indicators)
- Consistent theming using CSS custom properties
- Well-organized state management (hover, focus, disabled, highlighted, selected)
- Responsive and flexible sizing with CSS variables
The styling aligns well with the new Combobox primitive architecture.
src/core/primitives/Combobox/fragments/ComboboxPrimitiveGroup.tsx (1)
3-21: LGTM!Clean renaming from
SelectPrimitiveGrouptoComboboxPrimitiveGroupwith consistent updates to type names, component declaration, and display name. The implementation remains unchanged, which is appropriate for this migration.src/core/primitives/Combobox/fragments/ComboboxPrimitivePortal.tsx (1)
1-31: LGTM! Clean refactoring from Select to Combobox.The portal component has been properly refactored with consistent naming updates. The conditional rendering logic based on
isOpenand root element detection is correct.src/components/ui/Combobox/stories/Combobox.stories.tsx (1)
20-33: Verify Portal wrapper is not required.The
Selectcomponent pattern (fromSelect.stories.tsx) wrapsSelect.ContentinsideSelect.Portal. This story rendersCombobox.Contentdirectly underCombobox.Rootwithout aCombobox.Portalwrapper. Please verify whether this is an intentional API difference or if the Portal wrapper should be added for consistency.🔎 Expected pattern based on Select component
<Combobox.Root> <Combobox.Trigger> Select an option </Combobox.Trigger> + <Combobox.Portal> <Combobox.Content> <Combobox.Search /> <Combobox.Group> <Combobox.Item value="g1option 1">g1Option 1</Combobox.Item> <Combobox.Item value="g1option 2">g1Option 2</Combobox.Item> <Combobox.Item value="g1option 3">g1Option 3</Combobox.Item> </Combobox.Group> <Combobox.Group> <Combobox.Item value="g2option 1">g2Option 1</Combobox.Item> <Combobox.Item value="g2option 2">g2Option 2</Combobox.Item> <Combobox.Item value="g2option 3">g2Option 3</Combobox.Item> </Combobox.Group> </Combobox.Content> + </Combobox.Portal> </Combobox.Root>src/components/ui/Combobox/fragments/ComboboxPortal.tsx (1)
1-24: LGTM! Clean portal wrapper implementation.The component correctly wraps
ComboboxPrimitive.Portal, forwards refs and props appropriately, and maintains proper TypeScript typing.src/core/primitives/Combobox/contexts/ComboboxPrimitiveContext.tsx (1)
1-34: LGTM! Context refactoring is correct.The context has been properly renamed from
SelectPrimitiveContexttoComboboxPrimitiveContextwith all type definitions updated consistently.src/core/primitives/Combobox/fragments/ComboboxPrimitiveContent.tsx (1)
3-46: LGTM! Clean refactor from Select to Combobox.The renaming from
SelectPrimitiveContenttoComboboxPrimitiveContentis consistent throughout, and the context migration toComboboxPrimitiveContextis correct.src/core/primitives/Combobox/fragments/ComboboxPrimitiveTrigger.tsx (1)
3-51: LGTM! Consistent refactor from Select to Combobox.The component has been properly renamed to
ComboboxPrimitiveTriggerwith correct context usage (ComboboxPrimitiveContext). The implementation remains clean and functional.src/components/ui/Combobox/Combobox.tsx (1)
12-15: LGTM! Good developer experience with direct usage warning.The base component warns users appropriately when they attempt to use
Comboboxdirectly instead of via the composition API (e.g.,Combobox.Root).src/components/ui/Combobox/fragments/ComboboxGroup.tsx (1)
1-28: LGTM! Clean wrapper implementation.The component correctly consumes
ComboboxRootContextto apply contextual styling and forwards all props toComboboxPrimitive.Group. The implementation follows the established pattern across other Combobox fragments.src/core/primitives/Combobox/fragments/ComboboxPrimitiveRoot.tsx (1)
4-181: LGTM! Solid refactor with complex state management.The component has been properly renamed to
ComboboxPrimitiveRootwith correct context wiring toComboboxPrimitiveContext. The implementation handles floating UI positioning, keyboard navigation, typeahead, and form integration correctly.src/components/ui/Combobox/tests/Combobox.full.test.tsx (1)
1-210: Excellent comprehensive test coverage!The test suite thoroughly validates keyboard navigation, form submission, controlled/uncontrolled values, portal rendering, accessibility (including axe-core validation), asChild usage, and disabled state handling. This ensures the Combobox component behaves correctly across all scenarios.
Note: The static analysis warning about the label on line 147 is a false positive—the label is properly associated with the combobox via
aria-labelledby="label"on line 149.src/core/primitives/Combobox/stories/Combobox.stories.tsx (1)
1-175: Excellent story coverage!The stories comprehensively demonstrate basic usage, controlled state, form integration, grouped items, and search functionality. This provides clear examples for consumers of the Combobox primitive.
src/core/primitives/Combobox/ComboboxPrimitive.tsx (2)
10-13: LGTM! Good developer guidance with direct usage warning.The base component appropriately warns developers against direct usage and guides them toward the composition API (e.g.,
Combobox.Root,Combobox.Content).
17-25: LGTM! Complete interface definition.The
ComboboxPrimitiveComponentinterface correctly includes all static subcomponents, includingSearch. This avoids the TypeScript issue present in the non-primitive Combobox component.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/Select/fragments/SelectItem.tsx (1)
3-3: API compatibility verified — all props fully supported.The
ComboboxPrimitive.ItemAPI accepts all props being passed:value,disabled,className,data-disabled,role,aria-disabled, and ref. The migration is correctly implemented.Note: The component redundantly sets
role="option"andaria-disabledinternally in ComboboxPrimitiveItem (lines 76, 82), which are also explicitly passed from SelectItem. Consider removing these from SelectItem to avoid duplication, allowing ComboboxPrimitiveItem to manage them exclusively.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/ui/Combobox/stories/Combobox.stories.tsx(1 hunks)src/components/ui/Combobox/tests/Combobox.full.test.tsx(1 hunks)src/components/ui/Select/fragments/SelectContent.tsx(1 hunks)src/components/ui/Select/fragments/SelectGroup.tsx(1 hunks)src/components/ui/Select/fragments/SelectItem.tsx(2 hunks)src/components/ui/Select/fragments/SelectPortal.tsx(1 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(3 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(3 hunks)styles/themes/default.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/ui/Select/fragments/SelectTrigger.tsx
- src/components/ui/Select/fragments/SelectGroup.tsx
- styles/themes/default.scss
- src/components/ui/Combobox/stories/Combobox.stories.tsx
- src/components/ui/Select/fragments/SelectContent.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-12T08:34:33.079Z
Learnt from: decipher-cs
Repo: rad-ui/ui PR: 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`.
Applied to files:
src/components/ui/Select/fragments/SelectItem.tsxsrc/components/ui/Combobox/tests/Combobox.full.test.tsx
🧬 Code graph analysis (2)
src/components/ui/Select/fragments/SelectItem.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Combobox/tests/Combobox.full.test.tsx (1)
test-utils/index.ts (1)
axe(56-62)
🪛 GitHub Check: lint
src/components/ui/Combobox/tests/Combobox.full.test.tsx
[warning] 147-147:
A form label must be associated with a control
⏰ 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). (3)
- GitHub Check: Jest (1/2)
- GitHub Check: Jest (2/2)
- GitHub Check: coverage
🔇 Additional comments (9)
src/components/ui/Select/fragments/SelectPortal.tsx (1)
3-3: LGTM! Previous import alias issue resolved.The misleading import alias flagged in the previous review has been addressed. The import now correctly uses
ComboboxPrimitive(matching the actual module), and all type references and JSX usages have been updated consistently. The component properly wrapsComboboxPrimitive.Portalwhile maintaining the Select public API.src/components/ui/Combobox/tests/Combobox.full.test.tsx (7)
8-39: LGTM! Comprehensive keyboard navigation test.The test thoroughly validates keyboard interactions, state transitions, and accessibility attributes. The typo from the previous review has been corrected.
41-74: LGTM! Form integration test is thorough.The test validates both form submission behavior and correctly ensures disabled options cannot be selected, which is critical for accessibility and data integrity.
76-119: LGTM! Well-structured test for controlled and uncontrolled patterns.The test validates both React state management patterns effectively, ensuring the Combobox works correctly in both scenarios.
121-142: LGTM! Portal and focus management test is solid.The test validates both custom portal container rendering and proper focus return behavior, which are essential for accessibility.
144-167: LGTM! Accessibility test is comprehensive.The test validates both axe-core violations and specific ARIA attributes. The label on line 147 is correctly used for
aria-labelledbyreference, making the static analysis warning a false positive.
169-191: LGTM! asChild behavior is validated correctly.The test ensures that the
asChildprop properly delegates rendering to the child element while preserving Combobox functionality. The type assertion on line 173 is an acceptable workaround for polymorphic ref typing.
193-209: LGTM! Disabled state test is appropriate.The test validates that a disabled trigger correctly prevents the listbox from opening, which is essential for proper disabled state handling.
src/components/ui/Select/fragments/SelectRoot.tsx (1)
2-2: Migration to ComboboxPrimitive implemented correctly.The import properly references
ComboboxPrimitive, and all type references and JSX usage have been updated consistently. TheComboboxPrimitive.RootAPI supports all props being passed, including theshiftprop (boolean) andonValueChangecallback signature ((value: string) => void).
| import React from 'react'; | ||
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import axe from 'axe-core'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the custom axe wrapper from test-utils.
The import uses axe-core directly instead of the custom axe wrapper from test-utils. Based on the relevant code snippets, the project has a standardized axe helper that applies ACCESSIBILITY_TEST_TAGS configuration for consistent testing.
🔎 Proposed fix
-import axe from 'axe-core';
+import { axe } from '@/test-utils';Then update line 163 to use the wrapper appropriately:
- const results = await axe.run(container, { rules: { 'color-contrast': { enabled: false } } });
+ const results = await axe(container);Note: If you need to disable the color-contrast rule, you may need to extend the custom wrapper or verify if the wrapper already handles this configuration.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ui/Combobox/tests/Combobox.full.test.tsx around line 4, the
test imports axe directly from 'axe-core' but the project provides a custom axe
wrapper in test-utils that applies ACCESSIBILITY_TEST_TAGS and other
standardized configuration; replace the direct import with the custom wrapper
import from the project's test-utils and update the usage at line 163 to call
the wrapper's axe function (or pass options via that wrapper) so the
ACCESSIBILITY_TEST_TAGS configuration is applied consistently — if
color-contrast needs to be disabled, extend or configure the custom wrapper to
disable that rule before using it in the test.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/eighty-books-post.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/eighty-books-post.md
[grammar] ~5-~5: Ensure spelling is correct
Context: ...nor --- renamed the select to combobox primtive and separate themed comps for select an...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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). (2)
- GitHub Check: Build
- GitHub Check: coverage
🔇 Additional comments (1)
.changeset/eighty-books-post.md (1)
2-2: Verify version bump aligns with breaking changes.The "minor" version bump typically indicates backward-compatible additions. However, the PR summary indicates removing Select.Search from the API and renaming primitives, which could be breaking changes. Please verify that the version bump (minor vs. major) accurately reflects the nature and scope of these changes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.