Conversation
WalkthroughAdds portal container support and disabled-item plumbing across Select UI and primitives, updates trigger to use Primitive.button with asChild and disabled forwarding, exposes disabledIndices in context/root, adds data-state/ARIA attributes, and introduces an end-to-end Select test suite. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant Tr as SelectTrigger
participant R as SelectRoot
participant L as Listbox
participant It as Item
participant DOM as Portal/DOM
U->>Tr: Click / Enter
alt Trigger disabled
Tr-->>U: No action
else Enabled
Tr->>R: toggleOpen()
R->>L: render list (data-state: open)
end
U->>L: ArrowDown / Typeahead
L->>R: request next index
R->>R: skip indices in disabledIndices
R-->>L: activeIndex
U->>It: Enter/Click
alt Item disabled
It-->>U: ignored
else Selectable
It->>R: handleSelect(index)
R->>Tr: close() and focus()
Tr->>DOM: focus reference
end
sequenceDiagram
autonumber
participant App as App
participant SP as SelectPortal / PrimitivePortal
participant DOM as Document
App->>SP: render(container?)
alt container provided
SP->>DOM: mount into provided container
else no container
SP->>DOM: mount into #rad-ui-theme-container or document.body
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
24-25: Disabled indices in context: verify provider + consider id-based tracking
- Ensure SelectPrimitiveRoot supplies both
disabledIndicesandsetDisabledIndices; otherwise consumers destructuring these will crash.- Index-based tracking can desync on reorder/unmount. Prefer
Set<string>of itemIds and derive indices when needed.src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1)
28-28:data-state="closed"is unreachable due to conditional mountSince the content only renders when
isOpen,data-statewill never be"closed". If exit animations are desired, keep the node mounted (presence component) and toggledata-state; otherwise, remove the closed branch.- data-state={isOpen ? 'open' : 'closed'} + data-state="open"src/components/ui/Select/fragments/SelectPortal.tsx (1)
7-7: Container prop plumbing—LGTM. Minor typing simplificationYou can avoid duplicating prop types by aliasing the primitive’s props.
-export type SelectPortalProps = { - children: React.ReactNode; - container?: HTMLElement | null; -}; - -type SelectPortalPrimitiveProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Portal> & SelectPortalProps; - -const SelectPortal = React.forwardRef<SelectPortalElement, SelectPortalPrimitiveProps>(({ children, container, ...props }, forwardedRef) => { +export type SelectPortalProps = React.ComponentPropsWithoutRef<typeof SelectPrimitive.Portal>; + +const SelectPortal = React.forwardRef<SelectPortalElement, SelectPortalProps>(({ children, ...props }, forwardedRef) => { return ( - <SelectPrimitive.Portal ref={forwardedRef} container={container} {...props}> + <SelectPrimitive.Portal ref={forwardedRef} {...props}> {children} </SelectPrimitive.Portal> ); });Also applies to: 13-16
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (4)
66-70: Prevent focus on mouse down for disabled itemsStops focus before click when an item is disabled.
{...getItemProps({ - onClick: () => !disabled && handleSelect(index), + onMouseDown: (e: React.MouseEvent) => { if (disabled) e.preventDefault(); }, + onClick: () => !disabled && handleSelect(index), onKeyDown: (event: React.KeyboardEvent) => { if (disabled) return;
27-27: Drop unuseddisabledIndicesfrom destructuringIt’s never read here.
- const { handleSelect, isTypingRef, getItemProps, activeIndex, selectedIndex, virtualItemRef, selectedItemRef, hasSearch, disabledIndices, setDisabledIndices, valuesRef } = context; + const { handleSelect, isTypingRef, getItemProps, activeIndex, selectedIndex, virtualItemRef, selectedItemRef, hasSearch, setDisabledIndices, valuesRef } = context;
61-61: Use strict equalityAvoid loose equality when comparing ids.
- data-active={!hasSearch ? isActive : virtualItemRef.current?.id == itemId } + data-active={!hasSearch ? isActive : virtualItemRef.current?.id === itemId }
28-28: Ref type mismatch
Primitive.divrenders a div; align the ref type.- const itemRef = React.useRef<HTMLButtonElement>(null); + const itemRef = React.useRef<HTMLDivElement>(null);src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (2)
12-18: Broaden container type, ensure it’s connected, and memoize root lookupAvoid querying the DOM every render and handle detached containers.
Apply:
-import React, { useContext, useEffect, useState } from 'react'; +import React, { useContext, useEffect, useMemo, useState } from 'react'; @@ - { children: React.ReactNode; container?: HTMLElement | null } & React.ComponentPropsWithoutRef<typeof Floater.Portal> + { children: React.ReactNode; container?: Element | null } & React.ComponentPropsWithoutRef<typeof Floater.Portal> @@ - const rootElement = (container || document.querySelector('#rad-ui-theme-container') || document.body) as HTMLElement | null; + const rootElement = useMemo(() => { + if (container && (container as Element).isConnected) return container as Element; + return (document.querySelector('#rad-ui-theme-container') || document.body) as Element | null; + }, [container]);
20-26: Optional: drop rootElementFound stateYou can render-gate on rootElement directly and remove the effect/state to simplify.
- const [rootElementFound, setRootElementFound] = useState(false); - useEffect(() => { - if (rootElement) { - setRootElementFound(true); - } - }, [rootElement]); - if (!isOpen || !rootElementFound) return null; + if (!isOpen || !rootElement) return null;src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
151-171: Hidden native form control: prefer input[type="hidden"]A hidden works but is heavier and can be focusable via accessibility tools. An input[type="hidden"] is simpler and unambiguous. - { - isFormChild && ( - <select - name={name} - value={selectedLabel} - hidden - aria-hidden="true" - tabIndex={-1} - onChange={() => {}} - > - <option value={selectedLabel}>{selectedLabel}</option> - </select> - ) - } + {isFormChild && ( + <input type="hidden" name={name} value={selectedLabel ?? ''} /> + )} src/components/ui/Select/tests/Select.full.test.tsx (4) 24-29: Prefer role-based queries over text Querying the trigger by role reflects semantics and reduces fragility if text changes. - const trigger = screen.getByText('choose'); + const trigger = screen.getByRole('combobox', { name: /choose/i }); 122-143: Clean up appended portal container Prevent leaking detached nodes in JSDOM. await userEvent.click(screen.getByText('One')); expect(document.activeElement).toBe(trigger); + container.remove(); 145-168: Consider jest-axe for simpler a11y tests Using jest-axe provides idiomatic assertions and integration. -import axe from 'axe-core'; +import { axe, toHaveNoViolations } from 'jest-axe'; +expect.extend(toHaveNoViolations); @@ - const results = await axe.run(container, { rules: { 'color-contrast': { enabled: false } } }); - expect(results.violations).toHaveLength(0); + const results = await axe(container, { rules: { 'color-contrast': { enabled: false } } }); + expect(results).toHaveNoViolations(); 42-75: Add a keyboard-nav test for disabled items You cover click on disabled; add a keyboard scenario to ensure navigation skips disabled items. + test('keyboard navigation skips disabled items', async () => { + render( + <Select.Root> + <Select.Trigger>open</Select.Trigger> + <Select.Portal> + <Select.Content> + <Select.Group> + <Select.Item value="a">A</Select.Item> + <Select.Item value="b" disabled>B</Select.Item> + <Select.Item value="c">C</Select.Item> + </Select.Group> + </Select.Content> + </Select.Portal> + </Select.Root> + ); + const trigger = screen.getByText('open'); + await userEvent.click(trigger); + await userEvent.keyboard('{ArrowDown}{ArrowDown}'); // should skip B and land on C + const options = screen.getAllByRole('option'); + expect(options[2]).toHaveAttribute('data-active', 'true'); + }); src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (3) 7-12: Tighten the props type (drop index signature; avoid duplication). The [key: string]: any weakens type safety and disabled/className are already on Primitive.button props. Recommend trimming. Apply: export type SelectPrimitiveTriggerProps = { children: React.ReactNode; - className?: string; - disabled?: boolean; - [key: string]: any; + className?: string; // optional: keep if you prefer explicitness + disabled?: boolean; // optional: or rely solely on Primitive.button’s prop }; 35-35: Use nullish coalescing to avoid treating empty string as “no label”. s'' would currently fall back to children. Apply: - {selectedLabel || children} + {selectedLabel ?? children} 26-28: A11y polish: add aria-haspopup and guard disabled prop aria-expanded={isOpen} + aria-haspopup="listbox" data-state={isOpen ? 'open' : 'closed'} aria-disabled={disabled ? true : undefined} - disabled={disabled ? true : undefined} + disabled={asChild ? undefined : (disabled ? true : undefined)} No aria-controls hook found in this primitive—skip wiring until an ID mechanism is introduced. 📜 Review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 Commits Reviewing files that changed from the base of the PR and between 142c0cb and 90b9c3c. 📒 Files selected for processing (9) src/components/ui/Select/fragments/SelectPortal.tsx (1 hunks) src/components/ui/Select/fragments/SelectTrigger.tsx (1 hunks) src/components/ui/Select/tests/Select.full.test.tsx (1 hunks) src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1 hunks) src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1 hunks) src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (3 hunks) src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1 hunks) src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (5 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: #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-11-24T06:43:42.194Z Learnt from: kotAPI 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/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx 🧬 Code graph analysis (2) src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1) src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1) SelectPrimitiveContext (34-34) src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1) src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1) SelectPrimitiveContext (34-34) ⏰ 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 (12) src/components/ui/Select/fragments/SelectTrigger.tsx (1) 21-21: Forwarding disabled to the primitive—LGTM Passing the native disabled is correct; keeping aria-disabled helps when asChild renders a non-button. src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1) 60-60: data-label addition—LGTM Handy for testing/automation without affecting a11y. src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1) 8-9: API addition for container prop looks good Clear extension of the public surface with a backward-compatible defaulting path. src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (5) 39-39: State for disabledIndices: good placement Localizing disabledIndices in root and exposing via context is appropriate. 63-69: Typeahead should skip disabled: LGTM Early-return guard is correct and prevents activating disabled options. 153-153: data-state on root: good for styling and tests This addition improves debuggability and theming. 8-9: Import path is correct The hook is exported from src/core/hooks/useIsInsideFrom/index.ts, so importing from '~/core/hooks/useIsInsideFrom' matches the directory name and won’t break the build. Likely an incorrect or invalid review comment. 84-92: Verify external Floater.useListNavigation API supports disabledIndices & virtualItemRef No local implementation found—ensure the upstream signature (e.g. its TypeScript definitions or documentation) accepts both disabledIndices and virtualItemRef, otherwise those options will be ignored. src/components/ui/Select/tests/Select.full.test.tsx (2) 36-39: Verify case sensitivity in displayed value The expectation uses 'banana' (lowercase) while the item label is 'Banana'. Ensure this matches component behavior (value vs. label). If labels should be preserved, assert 'Banana'. 194-210: Nice coverage of disabled trigger Good negative test ensuring no popup when disabled. src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (2) 5-5: Primitive.button import looks good. Matches the rest of the primitives pattern. 15-17: Correct forwardRef typing. Right target type and prop intersection for Primitive.button/asChild.
There was a problem hiding this comment.
Actionable comments posted: 0
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/Select/fragments/SelectPrimitiveRoot.tsx (1)
8-8: Fix import path typo: hook name mismatch will fail buildPath imports
useIsInsideFrombut symbol isuseIsInsideForm.Apply:
-import { useIsInsideForm } from '~/core/hooks/useIsInsideFrom'; +import { useIsInsideForm } from '~/core/hooks/useIsInsideForm';src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
22-25: Avoid early return before hooks (Biome error); throw insteadReturning before later hooks violates hook order. Throwing stops render without conditional hooks.
- if (!context) { - console.error('SelectPrimitiveItem must be used within a SelectPrimitive'); - return null; - } + if (!context) { + throw new Error('SelectPrimitiveItem must be used within a SelectPrimitive'); + }
♻️ Duplicate comments (3)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (2)
38-70: Cleanup on index change/unmount is solidRemoves stale indices and value mappings; aligns with earlier feedback.
82-84: Disabled state reflected correctly
aria-disabled/data-disabledapplied and selection is gated.src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
21-28: Compose Floater’s onClick with consumer onClick; currently _refOnClick is unusedDropping the ref handler can break internal interactions. Pass onClick into getReferenceProps or compose with the returned onClick.
- const { onClick: _refOnClick, ...refProps } = getReferenceProps(); - - const handleClick = composeEventHandlers(onClick, () => { - if (!disabled) { - setIsOpen(prev => !prev); - } - }); + const refProps = getReferenceProps({ + onClick: (e: React.MouseEvent) => { + onClick?.(e); + if (e.defaultPrevented) return; + if (!disabled) setIsOpen(prev => !prev); + }, + }); @@ - onClick={handleClick} {...refProps} {...props}Also applies to: 40-42
🧹 Nitpick comments (5)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (3)
117-128: Tighten effect null-check and dependencies
if (!selectedItemRef)is always false; dependency onselectedItemRef.currentwon’t re-run. Recompute when selection changes and elements exist.- useLayoutEffect(() => { - if (!shift) return; - if (!selectedItemRef) return; - if (refs.floating.current && selectedItemRef.current) { - const rectA = refs.floating.current.getBoundingClientRect(); - const rectB = selectedItemRef.current.getBoundingClientRect(); - - const relativeTop = rectA.top - rectB.bottom; - setOffsetPositionValue(relativeTop); - } - }, [selectedItemRef.current, shift]); + useLayoutEffect(() => { + if (!shift) return; + const floatingEl = refs.floating.current; + const selectedEl = selectedItemRef.current; + if (!floatingEl || !selectedEl) return; + const rectA = floatingEl.getBoundingClientRect(); + const rectB = selectedEl.getBoundingClientRect(); + setOffsetPositionValue(rectA.top - rectB.bottom); + }, [shift, selectedIndex, isOpen, refs.floating]);
35-35: Ref typing nitPrefer
React.useRef<HTMLElement | null>(null)overany.- const selectedItemRef = React.useRef<any>(null); + const selectedItemRef = React.useRef<HTMLElement | null>(null);
129-153: Memoize context value to reduce child re-renders
valuesobject is recreated each render; memoize by stable deps.- const values = { + const values = React.useMemo(() => ({ ... - }; + }), [ + isOpen, setIsOpen, handleSelect, floatingContext, refs, + getFloatingProps, getReferenceProps, floatingStyles, getItemProps, + activeIndex, selectedIndex, disabledIndices, setDisabledIndices, + selectedLabel, hasSearch + ]);src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (2)
28-28: Ref type mismatch with rendered elementRoot is a div; use
HTMLDivElement(orHTMLElement) to avoid type drift.- const itemRef = React.useRef<HTMLButtonElement>(null); + const itemRef = React.useRef<HTMLDivElement>(null);
85-101: Deduplicate tabIndex source; keep oneYou set tabIndex via getItemProps and again on the element; drop the trailing prop to avoid confusion/override.
{...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}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(4 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(4 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-24T06:43:42.194Z
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.
Applied to files:
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx
🧬 Code graph analysis (1)
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(34-34)
🪛 Biome (2.1.2)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx
[error] 28-28: 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] 29-29: 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] 30-30: 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 (6)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (5)
62-69: Typeahead correctly skips disabled items—niceEarly-return on null/disabled is correct.
71-85: handleSelect fix looks goodGuards disabled, focuses trigger, and dependencies include refs/indices.
93-95: Good: pass disabledIndices into list navigationPrevents navigation landing on disabled items.
144-146: Context surface extension LGTMExposing
disabledIndices/setDisabledIndicesmakes downstream logic straightforward.
156-156: data-state attribute addition LGTMUseful for styling and tests.
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
30-37: State/ARIA/disabled wiring LGTM
data-state,aria-expanded,aria-disabled, anddisabledare correct;typehandling withasChildis right.Also applies to: 34-37
kotAPI
deleted the
kotapi/add-keyboard-navigation-to-select-component
branch
1 participant
Summary
Testing
npx jest src/components/ui/Select/tests/Select.full.test.tsx src/components/ui/Select/tests/Select.test.tsx src/core/primitives/Select/tests/SelectPrimitive.test.tsxSummary by CodeRabbit
New Features
Accessibility
Bug Fixes
Tests