Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions packages/components/src/ui/select.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export interface SelectProps<T extends React.Key = string>

// Default search input built on top of CommandInput. Supports cmdk props at runtime.
const DefaultSearchInput = forwardRef<HTMLInputElement, React.ComponentPropsWithoutRef<typeof CommandInput>>(
(props, _ref) => <CommandInput {...props} />,
(props, ref) => <CommandInput {...props} />,
);
DefaultSearchInput.displayName = 'SelectSearchInput';

Expand All @@ -84,17 +84,34 @@ export function Select<T extends React.Key = string>({
const triggerRef = useRef<HTMLButtonElement>(null);
const popoverRef = useRef<HTMLDivElement>(null);
const selectedItemRef = useRef<HTMLElement>(null);
const searchInputRef = useRef<HTMLInputElement>(null);
const [searchQuery, setSearchQuery] = useState('');
// No need for JavaScript width measurement - Radix provides --radix-popover-trigger-width CSS variable

// When opening, ensure the currently selected option is the active item for keyboard nav
// and focus the search input if searchable
useEffect(() => {
if (!popoverState.isOpen) return;
if (!popoverState.isOpen) {
// Clear search query when closing
setSearchQuery('');
return;
}
requestAnimationFrame(() => {
if (searchable) {
// Query for the input element since CommandInput uses asChild and ref forwarding is complex
const inputElement = popoverRef.current?.querySelector<HTMLInputElement>('input[type="text"]');
if (inputElement) {
inputElement.focus();
const selectionEnd = inputElement.value.length;
if (selectionEnd > 0) {
inputElement.setSelectionRange(selectionEnd, selectionEnd);
}
}
}
const selectedEl = selectedItemRef.current as HTMLElement | null;
if (selectedEl) selectedEl.scrollIntoView({ block: 'center' });
});
}, [popoverState.isOpen]);
}, [popoverState.isOpen, searchable]);

const selectedOption = options.find((o) => o.value === value);

Expand All @@ -116,6 +133,34 @@ export function Select<T extends React.Key = string>({
const ChevronIcon = components?.ChevronIcon || DefaultChevronIcon;
const SearchInput = components?.SearchInput || DefaultSearchInput;

// Handle keydown on trigger to open popover and start typing
const handleTriggerKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
// Allow normal keyboard navigation (Enter, Space, Arrow keys, etc.)
if (
e.key === 'Enter' ||
e.key === ' ' ||
e.key === 'ArrowDown' ||
e.key === 'ArrowUp' ||
e.key === 'Escape' ||
e.key === 'Tab'
) {
return;
}

// If it's a printable character and searchable is enabled, open the popover and start typing
if (searchable && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
e.preventDefault();
if (!popoverState.isOpen) {
popoverState.open();
}
// Set the initial search query
setSearchQuery(e.key);
}

// Call the original onKeyDown if provided
buttonProps.onKeyDown?.(e);
};
Comment on lines +136 to +162
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n packages/components/src/ui/select.tsx | sed -n '120,160p'

Repository: lambda-curry/forms

Length of output: 1683


🏁 Script executed:

cat -n packages/components/src/ui/select.tsx | head -130

Repository: lambda-curry/forms

Length of output: 5790


🏁 Script executed:

cat -n packages/components/src/ui/select.tsx | sed -n '156,200p'

Repository: lambda-curry/forms

Length of output: 2569


🏁 Script executed:

fd select -e test -e spec | head -10

Repository: lambda-curry/forms

Length of output: 44


🏁 Script executed:

rg "handleTriggerKeyDown" packages/components/src/ui/select.tsx -B 5 -A 15

Repository: lambda-curry/forms

Length of output: 1811


Preserve consumer onKeyDown for navigation keys.

The early return at line 139 prevents buttonProps.onKeyDown from firing for Enter/Space/Arrow/Escape/Tab, breaking the prop delegation contract. Consumers passing an onKeyDown handler expect it to be called for all keyboard events.

🛠️ Proposed fix
 const handleTriggerKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
   // Allow normal keyboard navigation (Enter, Space, Arrow keys, etc.)
   if (
     e.key === 'Enter' ||
     e.key === ' ' ||
     e.key === 'ArrowDown' ||
     e.key === 'ArrowUp' ||
     e.key === 'Escape' ||
     e.key === 'Tab'
   ) {
+    buttonProps.onKeyDown?.(e);
     return;
   }

   // If it's a printable character and searchable is enabled, open the popover and start typing
   if (searchable && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
     e.preventDefault();
     if (!popoverState.isOpen) {
       popoverState.open();
     }
     // Set the initial search query
     setSearchQuery(e.key);
   }

   // Call the original onKeyDown if provided
   buttonProps.onKeyDown?.(e);
 };
📝 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.

Suggested change
// Handle keydown on trigger to open popover and start typing
const handleTriggerKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
// Allow normal keyboard navigation (Enter, Space, Arrow keys, etc.)
if (
e.key === 'Enter' ||
e.key === ' ' ||
e.key === 'ArrowDown' ||
e.key === 'ArrowUp' ||
e.key === 'Escape' ||
e.key === 'Tab'
) {
return;
}
// If it's a printable character and searchable is enabled, open the popover and start typing
if (searchable && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
e.preventDefault();
if (!popoverState.isOpen) {
popoverState.open();
}
// Set the initial search query
setSearchQuery(e.key);
}
// Call the original onKeyDown if provided
buttonProps.onKeyDown?.(e);
};
// Handle keydown on trigger to open popover and start typing
const handleTriggerKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
// Allow normal keyboard navigation (Enter, Space, Arrow keys, etc.)
if (
e.key === 'Enter' ||
e.key === ' ' ||
e.key === 'ArrowDown' ||
e.key === 'ArrowUp' ||
e.key === 'Escape' ||
e.key === 'Tab'
) {
buttonProps.onKeyDown?.(e);
return;
}
// If it's a printable character and searchable is enabled, open the popover and start typing
if (searchable && e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey) {
e.preventDefault();
if (!popoverState.isOpen) {
popoverState.open();
}
// Set the initial search query
setSearchQuery(e.key);
}
// Call the original onKeyDown if provided
buttonProps.onKeyDown?.(e);
};
🤖 Prompt for AI Agents
In `@packages/components/src/ui/select.tsx` around lines 128 - 154, The early
return in handleTriggerKeyDown stops consumer buttonProps.onKeyDown from being
invoked for navigation keys; update handleTriggerKeyDown to always call
buttonProps.onKeyDown(e) for all events (including
Enter/Space/ArrowUp/ArrowDown/Escape/Tab) before returning, and keep the
existing searchable behavior (preventDefault, open popover via
popoverState.open(), and setSearchQuery) only for printable characters;
reference handleTriggerKeyDown, buttonProps.onKeyDown, popoverState.open,
setSearchQuery and searchable when making the change.


return (
<Popover open={popoverState.isOpen} onOpenChange={popoverState.setOpen}>
<PopoverTrigger asChild>
Expand All @@ -131,6 +176,7 @@ export function Select<T extends React.Key = string>({
aria-haspopup="listbox"
aria-expanded={popoverState.isOpen}
aria-controls={listboxId}
onKeyDown={handleTriggerKeyDown}
{...buttonProps}
>
{value != null && value !== '' ? (selectedOption?.label ?? String(value)) : placeholder}
Expand Down Expand Up @@ -159,6 +205,7 @@ export function Select<T extends React.Key = string>({
{searchable && (
<div className="px-1.5 pb-1.5 pt-1.5">
<SearchInput
ref={searchInputRef}
placeholder="Search..."
Comment on lines 207 to 209
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n packages/components/src/ui/select.tsx | head -250 | tail -100

Repository: lambda-curry/forms

Length of output: 5451


🏁 Script executed:

wc -l packages/components/src/ui/select.tsx

Repository: lambda-curry/forms

Length of output: 103


🏁 Script executed:

grep -n "DefaultSearchInput\|searchInputRef" packages/components/src/ui/select.tsx | head -30

Repository: lambda-curry/forms

Length of output: 505


🏁 Script executed:

sed -n '59,65p' packages/components/src/ui/select.tsx

Repository: lambda-curry/forms

Length of output: 343


🏁 Script executed:

sed -n '85,105p' packages/components/src/ui/select.tsx

Repository: lambda-curry/forms

Length of output: 942


Forward the ref through DefaultSearchInput so auto-focus works.

DefaultSearchInput ignores the ref passed to it, leaving searchInputRef.current null. This breaks the auto-focus functionality in the useEffect that attempts to focus the search input when the popover opens.

🛠️ Proposed fix
-const DefaultSearchInput = forwardRef<HTMLInputElement, React.ComponentPropsWithoutRef<typeof CommandInput>>(
-  (props, _ref) => <CommandInput {...props} />,
-);
+const DefaultSearchInput = forwardRef<HTMLInputElement, React.ComponentPropsWithoutRef<typeof CommandInput>>(
+  (props, ref) => <CommandInput ref={ref} {...props} />,
+);
🤖 Prompt for AI Agents
In `@packages/components/src/ui/select.tsx` around lines 199 - 201,
DefaultSearchInput currently does not forward the ref so SearchInput receives a
ref that never becomes searchInputRef.current, breaking auto-focus; update
DefaultSearchInput to accept and forward refs (use forwardRef) and pass the
incoming ref to the underlying input element, ensuring the SearchInput usage
with ref={searchInputRef} receives the real DOM node; locate DefaultSearchInput
and replace its function/constant with React.forwardRef((props, ref) => ...) and
attach ref to the actual input element, leaving SearchInput and the useEffect
that calls searchInputRef.current.focus() unchanged.

value={searchQuery}
onValueChange={(v: string) => {
Expand Down