feat(ui): add SearchableSelect component with keyboard navigation#594
Merged
Conversation
Extract a reusable SearchableSelect from the chat AgentSelector. Supports search filtering, arrow key navigation, disabled options, customizable trigger/footer slots, and full ARIA combobox semantics. - Add SearchableSelect component, tests (32), and Storybook stories - Refactor AgentSelector to use SearchableSelect - Fix missing vitest client project setupFiles for jest-dom matchers
Greptile SummaryIntroduced a reusable Key Changes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| services/platform/app/components/ui/forms/searchable-select.tsx | Well-structured component with proper accessibility, keyboard navigation, and state management |
| services/platform/app/components/ui/forms/searchable-select.test.tsx | Comprehensive test coverage for rendering, interactions, keyboard navigation, and accessibility |
| services/platform/app/features/chat/components/agent-selector.tsx | Successfully refactored to use SearchableSelect, removed ~100 lines of duplicate code while maintaining functionality |
Last reviewed commit: 954d0f5
📝 WalkthroughWalkthroughThis PR introduces a new SearchableSelect component with full test coverage and Storybook documentation, then refactors the AgentSelector component to use it. The SearchableSelect provides a searchable, keyboard-navigable dropdown with filtering, accessibility features (ARIA attributes, keyboard navigation), optional descriptions, disabled option support, and customizable styling. The AgentSelector refactor removes custom dropdown logic and replaces it with the new component, simplifying the implementation by delegating search, rendering, and interaction handling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/forms/searchable-select.test.tsx`:
- Around line 18-36: The test helper renderSelect creates an onOpenChange mock
but never passes it into the SearchableSelect, so open-state callbacks can't be
asserted; update renderSelect to pass the onOpenChange prop into the rendered
<SearchableSelect> (or remove the mock if tests don't need it) so the
onOpenChange spy you return is actually wired to the component; look for the
renderSelect function and the SearchableSelect JSX and add the
onOpenChange={onOpenChange} prop.
In `@services/platform/app/components/ui/forms/searchable-select.tsx`:
- Around line 131-138: The initializeHighlight function can incorrectly default
to index 0 when the current value is not present in filteredOptions; modify
initializeHighlight to defensively handle that case by checking if idx >= 0 and
calling setHighlightedIndex(idx), otherwise avoid forcing index 0 — for example
setHighlightedIndex(-1) or leave the previous highlighted index if
filteredOptions is empty or the value is absent; update logic around
initializeHighlight (references: initializeHighlight, value, filteredOptions,
setHighlightedIndex) and add a short comment noting the component currently
clears search on close so this is a defensive guard.
- Around line 327-332: Remove the unreachable keyboard handler from the option
div in the SearchableSelect component: delete the onKeyDown={(e) => { if (e.key
=== 'Enter' || e.key === ' ') { e.preventDefault(); if (!option.disabled)
onSelect(option.value); } }} JSX prop on the option element (or its equivalent
handler) since the div has no tabIndex and keyboard focus is handled by the
search input; if keyboard support for options is desired instead, add a proper
tabIndex and focus management rather than leaving this dead handler.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
services/platform/app/components/ui/forms/searchable-select.stories.tsxservices/platform/app/components/ui/forms/searchable-select.test.tsxservices/platform/app/components/ui/forms/searchable-select.tsxservices/platform/app/features/chat/components/agent-selector.tsxservices/platform/vitest.config.mjs
Comment on lines
+18
to
+36
| function renderSelect( | ||
| overrides: Partial<React.ComponentProps<typeof SearchableSelect>> = {}, | ||
| ) { | ||
| const onValueChange = vi.fn(); | ||
| const onOpenChange = vi.fn(); | ||
| const result = render( | ||
| <SearchableSelect | ||
| value={null} | ||
| onValueChange={onValueChange} | ||
| options={options} | ||
| trigger={<button type="button">Open select</button>} | ||
| searchPlaceholder="Search..." | ||
| emptyText="No results" | ||
| aria-label="Test listbox" | ||
| {...overrides} | ||
| />, | ||
| ); | ||
| return { ...result, onValueChange, onOpenChange }; | ||
| } |
There was a problem hiding this comment.
onOpenChange mock is created but not passed to the component.
The onOpenChange spy is created and returned but never passed to SearchableSelect. If any tests need to verify open state change callbacks, they will fail silently.
🐛 Proposed fix
function renderSelect(
overrides: Partial<React.ComponentProps<typeof SearchableSelect>> = {},
) {
const onValueChange = vi.fn();
const onOpenChange = vi.fn();
const result = render(
<SearchableSelect
value={null}
onValueChange={onValueChange}
+ onOpenChange={onOpenChange}
options={options}
trigger={<button type="button">Open select</button>}
searchPlaceholder="Search..."
emptyText="No results"
aria-label="Test listbox"
{...overrides}
/>,
);
return { ...result, onValueChange, onOpenChange };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/searchable-select.test.tsx` around
lines 18 - 36, The test helper renderSelect creates an onOpenChange mock but
never passes it into the SearchableSelect, so open-state callbacks can't be
asserted; update renderSelect to pass the onOpenChange prop into the rendered
<SearchableSelect> (or remove the mock if tests don't need it) so the
onOpenChange spy you return is actually wired to the component; look for the
renderSelect function and the SearchableSelect JSX and add the
onOpenChange={onOpenChange} prop.
Comment on lines
+131
to
+138
| const initializeHighlight = useCallback(() => { | ||
| if (value) { | ||
| const idx = filteredOptions.findIndex((o) => o.value === value); | ||
| setHighlightedIndex(idx >= 0 ? idx : 0); | ||
| } else { | ||
| setHighlightedIndex(0); | ||
| } | ||
| }, [value, filteredOptions]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider edge case: initializeHighlight may not find value in filtered list.
When initializeHighlight runs, the filteredOptions list is unfiltered (search is empty on open), so this works correctly. However, if the component is opened with a pre-existing search value, the selected value might not be in filteredOptions, causing highlight to default to index 0 unexpectedly.
Since search is cleared on close (line 156), this is currently safe, but documenting this assumption or adding a defensive check would improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/searchable-select.tsx` around lines
131 - 138, The initializeHighlight function can incorrectly default to index 0
when the current value is not present in filteredOptions; modify
initializeHighlight to defensively handle that case by checking if idx >= 0 and
calling setHighlightedIndex(idx), otherwise avoid forcing index 0 — for example
setHighlightedIndex(-1) or leave the previous highlighted index if
filteredOptions is empty or the value is absent; update logic around
initializeHighlight (references: initializeHighlight, value, filteredOptions,
setHighlightedIndex) and add a short comment noting the component currently
clears search on close so this is a defensive guard.
Comment on lines
+327
to
+332
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| if (!option.disabled) onSelect(option.value); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The onKeyDown handler on the option div may be unreachable.
Since the option div doesn't have tabIndex, it cannot receive keyboard focus, so this onKeyDown handler will never be triggered. Keyboard navigation is handled by the search input, making this handler dead code.
♻️ Proposed fix: remove unreachable handler
onClick={() => !option.disabled && onSelect(option.value)}
- onKeyDown={(e) => {
- if (e.key === 'Enter' || e.key === ' ') {
- e.preventDefault();
- if (!option.disabled) onSelect(option.value);
- }
- }}
onMouseEnter={() => onMouseEnter(index)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/components/ui/forms/searchable-select.tsx` around lines
327 - 332, Remove the unreachable keyboard handler from the option div in the
SearchableSelect component: delete the onKeyDown={(e) => { if (e.key === 'Enter'
|| e.key === ' ') { e.preventDefault(); if (!option.disabled)
onSelect(option.value); } }} JSX prop on the option element (or its equivalent
handler) since the div has no tabIndex and keyboard focus is handled by the
search input; if keyboard support for options is desired instead, add a proper
tabIndex and focus management rather than leaving this dead handler.
yannickmonney
changed the title
- Remove unused onOpenChange mock from test helper - Guard initializeHighlight against empty filteredOptions - Replace dead onKeyDown on option div with lint suppression (keyboard is handled on the combobox input via aria-activedescendant)
This was referenced Mar 12, 2026
yannickmonney
added a commit
that referenced
this pull request
Apr 8, 2026
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests