Replacing roving from select #1168
Conversation
|
""" WalkthroughThe changes expand the Floater utility with additional hooks and components from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectTrigger
participant SelectRoot
participant Floater
participant SelectContent
participant SelectItem
User->>SelectTrigger: Clicks trigger
SelectTrigger->>SelectRoot: Opens dropdown
SelectRoot->>Floater: Initializes FloatingList, navigation, typeahead
Floater->>SelectContent: Provides element/label refs, navigation handlers
SelectContent->>SelectItem: Renders items with indices
User->>SelectItem: Navigates/selects via keyboard or click
SelectItem->>SelectRoot: Calls handleSelect(index)
SelectRoot->>Floater: Updates active/selected index, closes dropdown
SelectRoot->>SelectTrigger: Updates displayed label
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! ✨ 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: 5
🔭 Outside diff range comments (5)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (1)
8-8: Align Search with Index-Based Selection and Restore Keyboard SelectionThe
SelectPrimitiveSearchfragment is still usingactiveItemValue/setActiveItemValueeven though the context now exposesactiveIndex,setActiveIndex, andhandleSelect(index). In its current form keyboard selection (Enter) is effectively dead—typing and arrow‐key nav updates the value, but there’s noEnterhandler to commit.Please update:
- Destructure the index‐based API:
const { refs, selectedItemRef, handleSelect,- activeItemValue,
- setActiveItemValue,
- activeIndex,
- setActiveIndex,
setSelectedValue
} = useContext(SelectPrimitiveContext);- In `handleKeyDown`: - Use `setActiveIndex(currentIndex ± 1)` rather than computing new `data-value` - Re-add an `if (e.key === 'Enter') handleSelect(activeIndex)` branch so pressing Enter commits the highlighted option - Replace all remaining uses of `activeItemValue` (e.g. in `aria-activedescendant`) with the item ID at `activeIndex` (or derive it via `elementsRef.current[activeIndex].id`) - Verify that clicking an option still calls `handleSelect` via `getItemProps` Affected file: - src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (context destructuring at line 8, keydown logic around lines 54–90) </blockquote></details> <details> <summary>src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)</summary><blockquote> `3-29`: **Missing context properties mentioned in usage.** The context type is missing `selectedLabel` and `isTypingRef` properties that are used by other components according to the changes. Add these missing properties to maintain type safety. ```diff export type SelectPrimitiveContextType = { isOpen: boolean, setIsOpen: React.Dispatch<React.SetStateAction<boolean>>, selectedValue: string, setSelectedValue: React.Dispatch<React.SetStateAction<string>> handleSelect: (index: number | null) => void refs: { reference: React.RefObject<any>; floating: React.RefObject<any>; setReference: (node: any) => void; setFloating: (node: any) => void; }; floatingStyles: React.CSSProperties; floatingContext: any; getReferenceProps: () => any; getFloatingProps: () => any; getItemProps: (userProps?: any) => any; selectedItemRef: React.RefObject<any>; setOffsetPositionValue: React.Dispatch<React.SetStateAction<number | undefined>>; activeItemValue: string; setActiveItemValue: React.Dispatch<React.SetStateAction<string>>; elementsRef: React.MutableRefObject<(HTMLElement | null)[]>; labelsRef: React.MutableRefObject<(string | null)[]>; activeIndex: number; setActiveIndex: React.Dispatch<React.SetStateAction<number>>; selectedIndex: number; + selectedLabel: string | null; + isTypingRef: React.MutableRefObject<boolean>; }src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
9-13: Implement disabled support in SelectPrimitiveItemThe
disabledprop is currently unused. To align with other primitives and ensure accessibility:
- Expose disabled state via attributes (
aria-disabled,data-disabled).- Prevent focus on disabled items (
tabIndex = -1).- Guard selection logic in
onClickandonKeyDown.Proposed diff in
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx:@@ function SelectPrimitiveItem({ children, value, disabled, ...props }: SelectPrimitiveItemProps) { - return ( - - <Primitive.div - ref={ref} - role="option" - aria-selected={isActive && isSelected} - tabIndex={isActive ? 0 : -1} - {...getItemProps({ - onClick: () => handleSelect(index), - onKeyDown: (event: React.KeyboardEvent) => { - if (event.key === 'Enter') { - event.preventDefault(); - handleSelect(index); - } - - if (event.key === " " && !isTypingRef.current) { - event.preventDefault(); - handleSelect(index); - } - } - })} - > - {children} - </Primitive.div> + return ( + <Primitive.div + ref={ref} + role="option" + data-disabled={disabled ? '' : undefined} + aria-disabled={disabled ? 'true' : undefined} + aria-selected={isActive && isSelected} + tabIndex={disabled ? -1 : (isActive ? 0 : -1)} + {...getItemProps({ + onClick: () => { + if (!disabled) handleSelect(index); + }, + onKeyDown: (event: React.KeyboardEvent) => { + if (disabled) return; + if (event.key === 'Enter' || (event.key === ' ' && !isTypingRef.current)) { + event.preventDefault(); + handleSelect(index); + } + }, + })} + > + {children} + </Primitive.div> + );This ensures disabled items are neither focusable nor selectable and correctly expose their disabled state.
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (2)
5-5: Remove unused import.The
RovingFocusGroupimport is no longer used after the refactoring.-import RovingFocusGroup from '~/core/utils/RovingFocusGroup';
9-9: Fix typo in import path.-import { useIsInsideForm } from '~/core/hooks/useIsInsideFrom'; +import { useIsInsideForm } from '~/core/hooks/useIsInsideForm';
🧹 Nitpick comments (2)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (1)
44-44: Remove empty line.There's an empty line where the Enter key handling was removed.
-src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
37-40: Fix inconsistent indentation.if (event.key === " " && !isTypingRef.current) { - event.preventDefault(); - handleSelect(index); - } + event.preventDefault(); + handleSelect(index); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/core/primitives/Floater/index.tsx(1 hunks)src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(3 hunks)src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(2 hunks)src/core/primitives/Select/stories/Select.stories.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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/Select/stories/Select.stories.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/Select/fragments/SelectPrimitiveSearch.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`.
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.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: 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`.
src/core/primitives/Select/fragments/SelectPrimitiveContent.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.
src/core/primitives/Select/contexts/SelectPrimitiveContext.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/Select/fragments/SelectPrimitiveItem.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/Select/fragments/SelectPrimitiveRoot.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: 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.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`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/core/primitives/Floater/index.tsx (1)
1-1: LGTM! Clean expansion of floating-ui utilities.The addition of these new hooks and components from floating-ui provides the necessary primitives for the Select component refactoring. The centralized approach through the Floater utility is well-structured.
Also applies to: 8-12
src/core/primitives/Select/stories/Select.stories.tsx (1)
137-137: Verify the value change is intentional.The value changed from
"g2option3"to"option3", breaking the naming pattern in the second group. While this might align with the index-based refactoring, it creates inconsistency with other items in the same group (g2option1,g2option2).Please confirm if this change is intentional for testing purposes or if it should maintain the
"g2option3"pattern for consistency.src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
9-9: LGTM! Correctly updated to use selectedLabel.The change from
selectedValuetoselectedLabelis appropriate for the trigger component, which should display the human-readable label rather than the internal value. This aligns well with the index-based selection refactoring.Also applies to: 18-18
src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1)
13-13: LGTM! Clean refactoring to use floating-ui utilities.The replacement of
RovingFocusGroup.GroupwithFloater.FloatingListis well-executed. The component now properly delegates list management to floating-ui while maintaining its core functionality. The context updates and removal of keyboard event handling align with the centralized approach.Also applies to: 19-19, 30-30
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
30-34: selectedValue state is used via context – no changes neededThe
selectedValueandsetSelectedValuefromuseControllableStateare passed intoSelectPrimitiveContextinSelectPrimitiveRoot.tsxand consumed by downstream fragments (e.g.,SelectPrimitiveSearch.tsxusessetSelectedValue). The state is therefore in use and should not be removed.Likely an incorrect or invalid review comment.
| { | ||
| isFormChild && ( | ||
| <select | ||
| name={name} | ||
| value={selectedLabel} | ||
| hidden | ||
| aria-hidden="true" | ||
| tabIndex={-1} | ||
| > | ||
| <option value={selectedLabel}>{selectedLabel}</option> | ||
| </select> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Form integration may be broken.
The hidden select element is using selectedLabel for both the value and the option text. This could cause issues if:
- The label differs from the actual value that should be submitted
- The initial selectedLabel is null (no default handling)
Consider maintaining the value-label mapping to ensure proper form submission.
Would you like me to help implement a proper value-label mapping system to fix the form integration?
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx around lines 158
to 170, the hidden select element incorrectly uses selectedLabel for both the
option value and display text, which can break form integration if the label
differs from the actual value or if selectedLabel is null. To fix this, update
the hidden select to use the actual selected value for the option's value
attribute and the corresponding label for the option's display text. Ensure you
maintain a mapping between values and labels so the form submits the correct
value while displaying the appropriate label.
| useEffect(() => { | ||
| if (!selectedIndex) return; | ||
| const selectedItemRef = labelsRef.current[selectedIndex]; | ||
| setSelectedItemRef(selectedItemRef); | ||
| },[selectedIndex]) |
There was a problem hiding this comment.
Fix useEffect implementation and dependencies.
The effect has multiple issues:
- Missing dependency
labelsRef - Confusing variable naming - using
selectedItemReffor both the label and the state setter - Should be storing the actual element reference, not the label
useEffect(() => {
if (!selectedIndex) return;
- const selectedItemRef = labelsRef.current[selectedIndex];
- setSelectedItemRef(selectedItemRef);
- },[selectedIndex])
+ // Find the corresponding element for the selected index
+ const selectedElement = elementsRef.current[selectedIndex];
+ setSelectedItemRef(selectedElement);
+ }, [selectedIndex, elementsRef]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx around lines 109
to 113, fix the useEffect by adding labelsRef as a dependency, rename the
variable to clearly distinguish between the label element and the state setter,
and ensure you store the actual DOM element reference from
labelsRef.current[selectedIndex] in the state instead of the label itself.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
155-167: Form integration may be broken.The hidden select element uses
selectedLabelfor both the value and the option text. This could cause issues if the label differs from the actual value that should be submitted, or ifselectedLabelis null.Consider maintaining the value-label mapping to ensure proper form submission:
<select name={name} - value={selectedLabel} + value={selectedIndex !== null ? valuesRef.current[selectedIndex] || '' : ''} hidden aria-hidden="true" tabIndex={-1} > - <option value={selectedLabel}>{selectedLabel}</option> + <option value={selectedIndex !== null ? valuesRef.current[selectedIndex] || '' : ''}> + {selectedLabel || ''} + </option> </select>
🧹 Nitpick comments (4)
src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx (2)
32-42: Consider combining related useEffect hooks.The useEffect hooks for initialization and ref updates have similar patterns and could potentially be combined for better performance.
- // Set hasSearch to true when search component mounts - React.useEffect(() => { - setHasSearch(true); - return () => setHasSearch(false); - }, [setHasSearch]); - - // Initialize refs when component mounts - React.useEffect(() => { - if (refs.floating.current) { - updateRefs(); - } - }, [refs.floating.current, updateRefs]); + // Initialize search state and refs when component mounts + React.useEffect(() => { + setHasSearch(true); + if (refs.floating.current) { + updateRefs(); + } + return () => setHasSearch(false); + }, [setHasSearch, refs.floating.current, updateRefs]);
114-114: Simplify aria-activedescendant logic.The current logic is complex and could be simplified for better readability.
- aria-activedescendant={virtualItemRef.current?.id || (activeIndex !== null && valuesRef.current[activeIndex] ? valuesRef.current[activeIndex] : undefined)} + aria-activedescendant={virtualItemRef.current?.id || (activeIndex !== null ? `select-item-${activeIndex}` : undefined)}src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
57-57: Optimize data attribute comparison.The current comparison uses loose equality which could be inefficient. Consider using strict equality or a more direct comparison.
- data-active={virtualItemRef.current?.id == itemId } + data-active={virtualItemRef.current?.id === itemId}src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
124-147: Optimize updateRefs callback dependencies.The
updateRefscallback has many dependencies that could cause unnecessary re-renders. Consider memoizing or reducing dependencies.updateRefs: React.useCallback(() => { if (!refs.floating.current) return; const floatingElement = refs.floating.current; const visibleItems = Array.from(floatingElement.querySelectorAll('[role="option"]')) as HTMLElement[]; // Update elementsRef with visible elements elementsRef.current = visibleItems; // Update labelsRef with visible element labels labelsRef.current = visibleItems.map(item => item.textContent?.trim() || null ); // Update valuesRef with visible element values (from data-value attribute or id) valuesRef.current = visibleItems.map(item => item.getAttribute('data-value') || item.id || null ); // Update virtualItemRef if there's an active item if (activeIndex !== null && visibleItems[activeIndex]) { virtualItemRef.current = visibleItems[activeIndex]; } - }, [refs.floating, elementsRef, labelsRef, activeIndex, virtualItemRef]) + }, [refs.floating, activeIndex])The refs (
elementsRef,labelsRef,virtualItemRef) are mutable and don't need to be dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx(2 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Select/stories/Select.stories.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/primitives/Select/stories/Select.stories.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 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/Select/fragments/SelectPrimitiveSearch.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/Select/fragments/SelectPrimitiveItem.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`.
src/core/primitives/Select/contexts/SelectPrimitiveContext.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/Select/fragments/SelectPrimitiveRoot.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: 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.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`.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
3-30: Well-structured context type definition.The context type properly reflects the shift from value-based to index-based selection system. The new refs and state management support the enhanced functionality with virtual navigation and search.
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
75-82: Verify virtual navigation configuration.The virtual navigation is enabled only when search is present. Ensure this behavior is correct and doesn't cause issues when search is removed.
The virtual navigation setup looks correct, but verify that:
- Virtual navigation properly handles the case when search is removed
- The
virtualItemRefis properly cleared when switching between virtual and non-virtual modes- The
hasSearchstate is properly managed across component lifecycle
| console.log('Enter key pressed, selecting item at index:', activeIndex); | ||
| handleSelect(activeIndex); |
There was a problem hiding this comment.
Remove debug console.log statements.
Debug logging should not be present in production code.
- console.log('Enter key pressed, selecting item at index:', activeIndex);- console.log('Space key pressed, selecting item at index:', activeIndex);Also applies to: 134-135
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx at lines
128-129 and 134-135, remove the debug console.log statements to clean up the
production code. Simply delete these console.log calls to prevent debug output
from appearing in the production environment.
| // @ts-ignore | ||
| type="search" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TypeScript ignore comments with proper types.
Using @ts-ignore suppresses type checking and should be avoided. Define proper types for the input element.
- // @ts-ignore
type="search"- // @ts-ignore
onChange={(e) => setSearch((e.target as HTMLInputElement).value)}The type="search" is a valid HTML input type and shouldn't need ignoring. For the onChange handler, the type is already properly cast.
Also applies to: 20-21
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx around lines
15-16 and 20-21, remove the @ts-ignore comments before the type="search"
attribute and any other similar ignores. Instead, ensure the input element is
properly typed by using correct TypeScript types for the input props and event
handlers, as type="search" is valid and the onChange handler is already
correctly typed. This will eliminate unnecessary suppression of type checking.
| console.log('Virtual focus debug:', { | ||
| activeIndex, | ||
| virtualItemRef: virtualItemRef.current?.id, | ||
| ariaActivedescendant: virtualItemRef.current?.id || (activeIndex !== null ? `select-item-${activeIndex}` : undefined) | ||
| }); | ||
| }, [activeIndex, virtualItemRef.current]); |
There was a problem hiding this comment.
Remove debug console.log statements.
Debug logging should not be present in production code.
- // Debug virtual focus
- React.useEffect(() => {
- console.log('Virtual focus debug:', {
- activeIndex,
- virtualItemRef: virtualItemRef.current?.id,
- ariaActivedescendant: virtualItemRef.current?.id || (activeIndex !== null ? `select-item-${activeIndex}` : undefined)
- });
- }, [activeIndex, virtualItemRef.current]);📝 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.
| console.log('Virtual focus debug:', { | |
| activeIndex, | |
| virtualItemRef: virtualItemRef.current?.id, | |
| ariaActivedescendant: virtualItemRef.current?.id || (activeIndex !== null ? `select-item-${activeIndex}` : undefined) | |
| }); | |
| }, [activeIndex, virtualItemRef.current]); |
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveSearch.tsx between lines
46 and 51, remove the console.log statement used for debugging virtual focus
details to clean up the production code and avoid unnecessary logging.
| className={className} | ||
| data-value={value} | ||
| data-active={virtualItemRef.current?.id == itemId } | ||
| aria-selected={isActive && isSelected} |
There was a problem hiding this comment.
Fix aria-selected logic.
The aria-selected attribute should indicate whether the item is selected, not whether it's both active and selected. An item can be selected without being active.
- aria-selected={isActive && isSelected}
+ aria-selected={isSelected}📝 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.
| aria-selected={isActive && isSelected} | |
| aria-selected={isSelected} |
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx at line 58, the
aria-selected attribute is incorrectly set to true only when the item is both
active and selected. Update the logic to set aria-selected to true whenever the
item is selected, regardless of whether it is active, to correctly reflect the
selection state.
| // Find the current element by ID | ||
| const element = document.getElementById(itemId); | ||
| if (element && virtualItemRef.current !== element) { | ||
| (virtualItemRef as React.MutableRefObject<HTMLElement | null>).current = element; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unsafe type assertion.
The type assertion could be unsafe. Use proper typing instead.
- (virtualItemRef as React.MutableRefObject<HTMLElement | null>).current = element;
+ if (virtualItemRef.current !== element) {
+ virtualItemRef.current = element;
+ }However, this assumes virtualItemRef is properly typed in the context. Verify the context type definition allows mutation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx at line 44,
remove the unsafe type assertion on virtualItemRef. Instead, ensure
virtualItemRef is correctly typed as a React.MutableRefObject<HTMLElement |
null> in its declaration or context so that you can assign to its current
property without type assertions. Verify and update the type definition of
virtualItemRef to allow mutation safely.
| role | ||
| role, | ||
| listNav, | ||
| // typeahead |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Uncommented typeahead hook not being used.
The typeahead hook is instantiated but commented out in the interactions. This suggests incomplete implementation.
Is the typeahead functionality intentionally disabled, or should it be included in the interactions?
🏁 Script executed:
#!/bin/bash
# Description: Check if typeahead functionality is implemented elsewhere or if this is intentional
# Search for typeahead usage in Select components
rg -A 3 -B 3 "typeahead" --type tsx src/core/primitives/Select/
# Look for any related TODO or FIXME comments
rg -i "todo|fixme|hack" src/core/primitives/Select/Length of output: 145
🏁 Script executed:
#!/bin/bash
# Search for typeahead references without file type filtering
rg -A3 -B3 "typeahead" src/core/primitives/Select/
# Search for any hook named useTypeahead across the repo
rg -A3 -B3 "useTypeahead" -g "*.ts*" .
# Show lines 90–110 in SelectPrimitiveRoot.tsx to view the commented instantiation
sed -n '90,110p' src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx || trueLength of output: 3282
Enable or remove the unused typeahead interaction
The useTypeahead hook is initialized but never passed into useInteractions, so type-to-search won’t work. You should either include it in the interactions array or remove the hook entirely if it’s not needed.
• File: src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx
• Location: around the useInteractions call (lines ~97–101)
Suggested diff:
// Merge all the interactions into prop getters
const { getReferenceProps, getFloatingProps, getItemProps } = Floater.useInteractions([
click,
dismiss,
role,
listNav,
- // typeahead
+ typeahead,
]);Also ensure you have tests covering keyboard typeahead behavior.
📝 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.
| // typeahead | |
| // Merge all the interactions into prop getters | |
| const { getReferenceProps, getFloatingProps, getItemProps } = Floater.useInteractions([ | |
| click, | |
| dismiss, | |
| role, | |
| listNav, | |
| typeahead, | |
| ]); |
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx around line 100,
the useTypeahead hook is initialized but not included in the useInteractions
call, so type-to-search functionality is not enabled. Fix this by adding the
typeahead interaction to the array passed into useInteractions if typeahead is
needed; otherwise, remove the useTypeahead hook entirely. Also, verify that
keyboard typeahead behavior is covered by tests.
Under progress
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores