Conversation
|
Warning Rate limit exceeded@kotAPI has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughRefactors Select and its fragments/primitives to use React.forwardRef, adds typed props, attaches static subcomponents to the Select export, and introduces displayName metadata. Updates core Select primitives to forward refs and tighten prop types. Adds tests validating ref forwarding, hidden native select in forms, and snapshots. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant App as App
participant Sel as Select.Root/Trigger
participant Prim as SelectPrimitive (Root/Trigger/Content)
participant Flo as Floater
U->>Sel: Click Trigger
Sel->>Prim: Forward props + ref
Prim->>Flo: useMergeRefs([internal, forwarded])
Prim->>Prim: Toggle isOpen
Prim->>Prim: Render Content (when open)
Prim->>U: Display options
rect rgba(200,240,255,0.3)
note over App,Prim: New: refs are forwarded from App through Select to primitives
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (2)
47-81: Guard DOM queries and fix effect deps.
- Access
refs.floating.currentwith optional chaining.- Avoid
[refs.floating.current]in deps; it’s unstable. Depend on the ref object orhasContext.- React.useEffect(() => { - if (!refs.floating.current) return; + React.useEffect(() => { + if (!context?.refs?.floating?.current) return; - const floatingElement = refs.floating.current; + const floatingElement = context.refs.floating.current!; @@ - context.setActiveIndex(null); - elementsRef.current = visible.map(item => item.element); - labelsRef.current = visible.map(item => item.label); - }, [search, refs.floating.current]); + context.setActiveIndex(null); + context.elementsRef.current = visible.map(item => item.element); + context.labelsRef.current = visible.map(item => item.label); + }, [search, context?.refs?.floating]);
39-46: Call hooks unconditionally; guard side effects instead of early-returning.This effect can run unconditionally and no-op when context is missing.
- React.useEffect(() => { - setHasSearch(true); - // Reset navigation state - if (context) { - context.setActiveIndex(0); - } - }, [setHasSearch]); + React.useEffect(() => { + if (!context) return; + context.setHasSearch(true); + context.setActiveIndex(0); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [context]);src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (3)
19-27: Remove early return before hooks; fix wrong ref type.The early return violates the Rules of Hooks, and itemRef should be HTMLDivElement (root is Primitive.div). Also, don’t call hooks inside JSX.
- if (!context) { - console.error('SelectPrimitiveItem must be used within a SelectPrimitive'); - return null; - } + // Assuming provider is always present. If not, make context default safe rather than early-returning here. - const itemRef = React.useRef<HTMLButtonElement>(null); - const { ref: listItemRef, index } = Floater.useListItem({ label: value }); + const itemRef = React.useRef<HTMLDivElement>(null); + const { ref: listItemRef, index } = Floater.useListItem({ label: value }); + const mergedRef = Floater.useMergeRefs([ref, listItemRef, itemRef]);
41-67: Respect disabled state, fix strict equality, and avoid calling hooks in JSX.Disabled items should be non-interactive and unfocusable. Also prefer strict equality and use the precomputed mergedRef.
- <Primitive.div - ref={Floater.useMergeRefs([ref, listItemRef, itemRef])} + <Primitive.div + ref={mergedRef} id={itemId} role="option" className={className} data-value={value} - data-active={!hasSearch ? isActive : virtualItemRef.current?.id == itemId } - aria-selected={isSelected} - tabIndex={isActive ? 0 : -1} + data-active={!hasSearch ? isActive : virtualItemRef.current?.id === itemId} + aria-selected={isSelected} + aria-disabled={disabled || undefined} + data-disabled={disabled ? '' : undefined} + tabIndex={disabled ? -1 : (isActive ? 0 : -1)} {...getItemProps({ - onClick: () => handleSelect(index), + onClick: () => { if (!disabled) handleSelect(index); }, onKeyDown: (event: React.KeyboardEvent) => { - if (event.key === 'Enter') { + if (disabled) return; + if (event.key === 'Enter') { event.preventDefault(); handleSelect(index); } - if (event.key === ' ' && !isTypingRef.current) { + if (event.key === ' ' && !isTypingRef.current) { event.preventDefault(); handleSelect(index); } } })} {...props} >
1-75: Hoist Floater.useMergeRefs calls out of JSX attributes
Move allFloater.useMergeRefs([...])invocations to top-level constants before thereturnin the following components:
• SelectPrimitiveTrigger.tsx (l.20)
• SelectPrimitiveSearch.tsx (l.26, l.92)
• SelectPrimitiveItem.tsx (l.42)
• SelectPrimitiveRoot.tsx (l.142)
• SelectPrimitiveContent.tsx (l.22)
• MenuPrimitiveTrigger.tsx (l.25)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
25-31: Submit the option value (not label) via hidden select; keep label separate.Currently, the form submits the label and onValueChange receives the label. Track value separately and update both on selection.
- const [selectedLabel, setSelectedLabel] = useControllableState( - value, - defaultValue, - onValueChange - ); + // Controlled "value" for form/onValueChange; label for display only. + const [selectedValue, setSelectedValue] = useControllableState( + value, + defaultValue, + onValueChange + ); + const [selectedLabel, setSelectedLabel] = React.useState<string>(''); @@ - const handleSelect = React.useCallback((index: number | null) => { - setSelectedIndex(index); - setIsOpen(false); - if (index !== null) { - const label = labelsRef.current[index]; - if (label) { - setSelectedLabel(label); - } - } - }, []); + const handleSelect = React.useCallback((index: number | null) => { + setSelectedIndex(index); + setIsOpen(false); + if (index !== null) { + const label = labelsRef.current[index]; + const nextValue = valuesRef.current[index]; + if (label != null) setSelectedLabel(label); + if (nextValue != null) setSelectedValue(nextValue); + } + }, [labelsRef, valuesRef, setSelectedValue]); @@ - <select + <select name={name} - value={selectedLabel} + value={selectedValue} hidden aria-hidden="true" tabIndex={-1} > - <option value={selectedLabel}>{selectedLabel}</option> + <option value={selectedValue}>{selectedLabel || selectedValue}</option> </select>Also add selectedValue to context if needed by other fragments:
const values = { @@ - selectedLabel, + selectedLabel, + selectedValue,Also applies to: 65-74, 148-156
🧹 Nitpick comments (20)
src/components/ui/Select/fragments/SelectGroup.tsx (1)
5-7: Optional: avoid redundant children typing.
childrenis already included inReact.ComponentPropsWithoutRef<typeof SelectPrimitive.Group>. You can drop the explicitchildrento reduce duplication.-export type SelectGroupProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Group> & { - children: React.ReactNode -}; +export type SelectGroupProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Group>;src/core/primitives/Select/fragments/SelectPrimitiveGroup.tsx (1)
3-6: Simplify props type; avoid duplicating built-in div props.
React.ComponentPropsWithoutRef<'div'>already includeschildrenandclassName. Duplicating them adds noise and risks divergence.-export type SelectPrimitiveGroupProps = React.ComponentPropsWithoutRef<'div'> & { - children: React.ReactNode, - className?: string, -} +export type SelectPrimitiveGroupProps = React.ComponentPropsWithoutRef<'div'>;src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (1)
93-97: Type-safe onChange; remove ts-ignore.Use a typed
ChangeEventparam and drop@ts-ignore.- // @ts-ignore - onChange={(e) => setSearch(e.target.value)} + onChange={(e: React.ChangeEvent<HTMLInputElement>) => setSearch(e.target.value)}src/components/ui/Select/tests/Select.test.tsx (3)
26-41: Always restore console spies via try/finally; also assert the hidden select’s name.Prevents cross-test leakage and strengthens the accessibility/form assertion.
- test('renders hidden native select for accessibility inside forms', () => { - const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - const { container } = render( + test('renders hidden native select for accessibility inside forms', () => { + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + try { + const { container } = render( <form> <Select.Root name="test"> <Select.Trigger>Trigger</Select.Trigger> <Select.Content> <Select.Item value="one">One</Select.Item> </Select.Content> </Select.Root> </form> - ); - const hiddenSelect = container.querySelector('select[aria-hidden="true"]'); - expect(hiddenSelect).toBeInTheDocument(); - errorSpy.mockRestore(); + ); + const hiddenSelect = container.querySelector('select[aria-hidden="true"]') as HTMLSelectElement | null; + expect(hiddenSelect).toBeInTheDocument(); + expect(hiddenSelect).toHaveAttribute('name', 'test'); + } finally { + errorSpy.mockRestore(); + } });
43-55: Use try/finally when mocking console to avoid side effects on failure.- test('renders without warnings', () => { - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - render( - <Select.Root> - <Select.Trigger>Trigger</Select.Trigger> - </Select.Root> - ); - expect(warnSpy).not.toHaveBeenCalled(); - expect(errorSpy).not.toHaveBeenCalled(); - warnSpy.mockRestore(); - errorSpy.mockRestore(); - }); + test('renders without warnings', () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + try { + render( + <Select.Root> + <Select.Trigger>Trigger</Select.Trigger> + </Select.Root> + ); + expect(warnSpy).not.toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + errorSpy.mockRestore(); + } + });
57-64: Snapshot is brittle; prefer explicit assertions on roles/attributes.Consider asserting Trigger role/aria rather than snapshot.
src/components/ui/Select/fragments/SelectSearch.tsx (1)
11-18: Minor: self-close empty element.- <SelectPrimitive.Search - className={`${rootClass}-search`} - ref={ref} - {...props} - > - - </SelectPrimitive.Search> + <SelectPrimitive.Search className={mergedClassName} ref={ref} {...props} />src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
6-9:childrenis already in button props; remove redundancy.-export type SelectPrimitiveTriggerProps = React.ComponentPropsWithoutRef<'button'> & { - children: React.ReactNode; - className?: string; -}; +export type SelectPrimitiveTriggerProps = React.ComponentPropsWithoutRef<'button'> & { + className?: string; +};src/components/ui/Select/fragments/SelectItem.tsx (2)
10-29: Avoid className overwrite; wire up customRootClass.props.className currently overrides
${rootClass}-itemdue to spread order, and customRootClass is unused. Merge class names and honor customRootClass.-const SelectItem = React.forwardRef<React.ElementRef<typeof SelectPrimitive.Item>, SelectItemProps>( - ({ customRootClass, children, value, disabled, ...props }, ref) => { +const SelectItem = React.forwardRef<React.ElementRef<typeof SelectPrimitive.Item>, SelectItemProps>( + ({ customRootClass, className, children, value, disabled, ...props }, ref) => { const { rootClass } = useContext(SelectRootContext); + const baseClass = customRootClass ?? rootClass; return ( <SelectPrimitive.Item - className={`${rootClass}-item`} + className={`${baseClass}-item${className ? ` ${className}` : ''}`} value={value} disabled={disabled} data-disabled={disabled ? '' : undefined} role="option" - aria-disabled={disabled ? 'true' : undefined} + aria-disabled={disabled || undefined} ref={ref} {...props} > - <span className={`${rootClass}-text`}>{children}</span> + <span className={`${baseClass}-text`}>{children}</span> </SelectPrimitive.Item> ); } );
15-26: Consider removing redundant ARIA/role from UI layer.If SelectPrimitive.Item already sets role/aria, duplicating them here is unnecessary and risks divergence.
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
31-33: Type vs comment mismatch for itemId.value is typed as required (string), so the “fallback to index” comment is misleading. Either make value optional or drop the fallback verbiage.
src/components/ui/Select/fragments/SelectTrigger.tsx (1)
19-21: Minor: aria-disabled is redundant on a disabled button.You can drop aria-disabled when disabled is present.
src/components/ui/Select/Select.tsx (2)
23-26: Gate the console.warn to dev and avoid unused ref.Prevents production noise and satisfies linters.
-const Select = React.forwardRef<HTMLDivElement, React.ComponentPropsWithoutRef<'div'>>((_, ref) => { - console.warn('Direct usage of Select is not supported. Please use Select.Root, Select.Content, etc. instead.'); +const Select = React.forwardRef<HTMLDivElement, React.ComponentPropsWithoutRef<'div'>>((_, _ref) => { + if (process.env.NODE_ENV !== 'production') { + // Direct usage is not supported. Use Select.Root, Select.Content, etc. + console.warn('Direct usage of Select is not supported. Please use Select.Root, Select.Content, etc. instead.'); + } return null; }) as SelectComponent;
29-36: Optional: attach statics via Object.assign for a single expression.Keeps the value immutable and avoids post-assignment mutation.
// Alternative pattern: const Select = Object.assign( React.forwardRef<HTMLDivElement, React.ComponentPropsWithoutRef<'div'>>((_, _ref) => { if (process.env.NODE_ENV !== 'production') console.warn('Direct usage of Select is not supported. Please use Select.Root, Select.Content, etc. instead.'); return null; }), { Root: SelectRoot, Content: SelectContent, Item: SelectItem, Trigger: SelectTrigger, Portal: SelectPortal, Group: SelectGroup, Indicator: SelectIndicator, Search: SelectSearch } ) as SelectComponent;src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (2)
31-31: Avoid any in refs.Use a concrete DOM type.
- const selectedItemRef = React.useRef<any>(null); + const selectedItemRef = React.useRef<HTMLElement | null>(null);
105-115: Effect dependencies should not use ref.current.selectedItemRef.current won’t trigger re-runs; recompute when selection/open state changes.
- }, [selectedItemRef.current, shift]); + }, [shift, selectedIndex, isOpen]);src/components/ui/Select/fragments/SelectRoot.tsx (4)
16-31: Stabilize context value to avoid redundant re-rendersProvider gets a new object each render; memoize to prevent unnecessary downstream updates.
- return ( - <SelectRootContext.Provider value={{ rootClass }}> + const ctxValue = React.useMemo(() => ({ rootClass }), [rootClass]); + return ( + <SelectRootContext.Provider value={ctxValue}>
8-10: Export props type for consumersThis component is part of a UI library; exporting the prop type helps downstream typing and docs.
-type SelectRootProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Root> & { +export type SelectRootProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Root> & { customRootClass?: string; };
20-22: Dev-only guard: avoid passing both value and defaultValueHelps catch controlled/uncontrolled misuse without affecting production.
- ({ customRootClass, className, children, defaultValue, value, onValueChange, shift, ...props }, ref) => { + ({ customRootClass, className, children, defaultValue, value, onValueChange, shift, ...props }, ref) => { const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); + if (process.env.NODE_ENV !== 'production' && value !== undefined && defaultValue !== undefined) { + // eslint-disable-next-line no-console + console.warn('SelectRoot: avoid passing both value and defaultValue at the same time.'); + }
6-6: Minor naming consistency
COMPONENT_NAME= "Select" whiledisplayName= "SelectRoot". If CSS naming relies on "Select", keeping both is fine; otherwise consider aligning them.Also applies to: 35-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
src/components/ui/Select/tests/__snapshots__/Select.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
src/components/ui/Select/Select.tsx(2 hunks)src/components/ui/Select/fragments/SelectContent.tsx(1 hunks)src/components/ui/Select/fragments/SelectGroup.tsx(1 hunks)src/components/ui/Select/fragments/SelectIndicator.tsx(1 hunks)src/components/ui/Select/fragments/SelectItem.tsx(1 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(1 hunks)src/components/ui/Select/fragments/SelectSearch.tsx(1 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(1 hunks)src/components/ui/Select/tests/Select.test.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveGroup.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(4 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(4 hunks)src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-12T08:34:33.079Z
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`.
Applied to files:
src/components/ui/Select/fragments/SelectTrigger.tsx
📚 Learning: 2024-12-12T08:22:59.375Z
Learnt from: decipher-cs
PR: rad-ui/ui#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/Select/fragments/SelectTrigger.tsx
🧬 Code graph analysis (10)
src/components/ui/Select/fragments/SelectSearch.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectRoot.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectContent.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(32-32)
src/components/ui/Select/fragments/SelectIndicator.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectGroup.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectItem.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(32-32)
src/components/ui/Select/fragments/SelectTrigger.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(32-32)
🪛 Biome (2.1.2)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx
[error] 26-26: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 92-92: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx
[error] 22-22: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx
[error] 26-26: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 42-42: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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 (15)
src/core/primitives/Select/fragments/SelectPrimitiveGroup.tsx (1)
8-14: LGTM on forwardRef conversion.Ref forwarding and prop passthrough look correct.
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (1)
94-95: valuesRef.current assignments not found in root or search—unable to confirm it stores element IDs. Ensure aria-activedescendant uses an actual element id (e.g.,elementsRef.current[activeIndex]?.id) rather than option values.src/components/ui/Select/tests/Select.test.tsx (1)
38-40: jest-dom matchers are configured
Import of@testing-library/jest-domis present insrc/setupTests.ts, sotoBeInTheDocumentis available.src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
20-21: No changes needed for useMergeRefs
Floater correctly re-exportsuseMergeRefsfrom@floating-ui/react, so the existingref={Floater.useMergeRefs([...])}usage is valid.src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (5)
31-39: Selected ref assignment effect runs after early return path.This effect must remain unconditional (it is now) but was previously guarded by an early return above. Removing the early return resolves the hook-order issue.
8-13: Prop surface looks good.ForwardRef typing to Primitive.div and explicit props are clear.
15-16: Name alignment and displayName are correct.displayName addition aids debugging.
24-30: Active/selected computation is straightforward.No issues spotted here.
50-64: Keyboard handling is solid.Enter/Space behaviors align with listbox expectations.
src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1)
16-35: Conditional rendering approach looks good.Mounting only when open keeps the tree light while preserving focus management via Floater.
src/components/ui/Select/fragments/SelectTrigger.tsx (2)
12-23: ForwardRef conversion: solid.Ref correctly forwards to the underlying Trigger.
19-21: Revisit data-placeholder semantics.Setting data-placeholder based only on the prop (not selection state) can mis-style a non-empty value. Consider deriving this from context/selection instead.
src/components/ui/Select/Select.tsx (1)
12-21: API surface definition looks good.Typed static subcomponents on the Select export are clear and consistent.
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
42-43: Verify dynamic updates ofisFormChild
EnsureuseIsInsideForm(rootRef.current)tracks the transition fromnullto the mounted element and updatesisFormChildaccordingly. If it does not, either refactor the hook to accept the ref object (rootRef) or defer the call until after mount. Applies to lines 42–43 and 146–158.src/components/ui/Select/fragments/SelectRoot.tsx (1)
12-13: Ref forwarding + typing look correctGeneric params and element ref inference are spot on.
|
closing as stale |
1 participant
Summary
Testing
npm testhttps://chatgpt.com/codex/tasks/task_e_68b9baa8268083319e5073c99aa10cba
Summary by CodeRabbit
Refactor
Tests