Fix: Enhance keyboard navigation and focus for BasicSelect component #127
Conversation
WalkthroughThis PR updates the test runner configuration to use npx for Jest execution, adds a comprehensive test suite for the BasicSelect component covering rendering, selection, keyboard navigation, and clear functionality, and refactors the BasicSelect component's internal structure with a wrapper div and repositioned clear button. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
🧹 Nitpick comments (2)
package.json (1)
12-12:"test": "npx jest"is likely redundant inside an npm scriptWithin
package.jsonscripts,jesttypically resolves to the localnode_modules/.bin/jestwithoutnpx. Unless you’re working around a specific environment/path issue, consider reverting to"test": "jest"to reduce indirection.src/__tests__/BasicSelect.test.tsx (1)
52-74: PreferuserEvent.keyboard(...)overfireEvent.keyDown(...)for key interactionsThis usually better matches how focus/keyboard events are dispatched and can reduce flakiness with Radix components.
Also applies to: 76-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)src/__tests__/BasicSelect.test.tsx(1 hunks)src/components/common/BasicSelect.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/__tests__/BasicSelect.test.tsx (2)
src/components/common/ErrorBoundary.tsx (1)
render(32-61)src/components/common/BasicSelect.tsx (1)
BasicSelect(20-95)
src/components/common/BasicSelect.tsx (2)
src/components/ui/select.tsx (6)
Select(150-150)SelectTrigger(158-158)SelectValue(159-159)SelectContent(151-151)SelectGroup(152-152)SelectItem(153-153)src/lib/utils.ts (1)
cn(4-6)
| it('should clear selection when allowEmpty is true and clear button is clicked', async () => { | ||
| render( | ||
| <BasicSelect | ||
| placeholder="Select an option" | ||
| items={mockItems} | ||
| onChange={mockOnChange} | ||
| value="1" | ||
| allowEmpty={true} | ||
| /> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Option 1')).toBeInTheDocument(); | ||
|
|
||
| const clearButton = screen.getByRole('button', { name: /clear/i }); | ||
| await userEvent.click(clearButton); | ||
|
|
||
| expect(mockOnChange).toHaveBeenCalledWith(null); | ||
| expect(screen.getByText('Select an option')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Clear-button test will fail unless the button has an accessible name
getByRole('button', { name: /clear/i }) won’t match the current clear button (icon-only, no aria-label). Please add an aria-label (or visible text) in src/components/common/BasicSelect.tsx, and then assert against that label.
🤖 Prompt for AI Agents
In src/__tests__/BasicSelect.test.tsx around lines 95 to 113 and
src/components/common/BasicSelect.tsx, the test queries the clear button by
accessible name but the component's icon-only clear button lacks an aria-label;
update BasicSelect.tsx to add an appropriate aria-label (e.g., "Clear selection"
or "Clear") on the clear button so it is discoverable by getByRole, and ensure
the test uses the same label (or a case-insensitive regex /clear/i) when
querying and asserting behavior.
| onOpenChange={(isOpen) => { | ||
| if (!isOpen && !hasValue) { | ||
| onChange(null); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Potential “select then immediately clear” bug via onOpenChange + stale hasValue
When an option is selected, the dropdown typically closes; if onOpenChange(false) fires before the parent updates value, hasValue can still be false and onChange(null) will run, wiping the selection. Consider removing this close-handler normalization, or guarding it with additional state/ref logic so it can’t run right after a non-empty selection.
🤖 Prompt for AI Agents
In src/components/common/BasicSelect.tsx around lines 50 to 54, the onOpenChange
handler uses hasValue to clear the selection when the dropdown closes which can
run with stale hasValue and wipe a just-selected value; remove this close-time
normalization or prevent it running immediately after a user selection by
tracking selection-in-flight with a ref/state. Specifically: either delete the
onOpenChange block that calls onChange(null), or add a ref like
selectionInFlightRef set when an option is chosen (and cleared after the parent
value updates or on next render), and short-circuit the onOpenChange handler to
skip clearing if selectionInFlightRef is true; ensure the fallback still clears
only when there truly is no value and no recent selection event.
| {allowEmpty && hasValue && ( | ||
| <button | ||
| type='button' | ||
| onClick={() => { | ||
| onChange(null); | ||
| }} | ||
| className='absolute right-2 top-1/2 -translate-y-1/2 hover:bg-neutral-100 dark:hover:bg-neutral-800 p-1.5 rounded-sm' | ||
| > | ||
| <X className='h-3.5 w-3.5 text-neutral-500 hover:text-neutral-900 dark:text-neutral-400 dark:hover:text-neutral-50' /> | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Clear button needs an accessible name (and should match the test)
Add aria-label (e.g. "Clear selection") so screen readers can announce it and so getByRole(..., { name: /clear/i }) can pass.
{allowEmpty && hasValue && (
<button
type='button'
+ aria-label='Clear selection'
onClick={() => {
onChange(null);
}}
className='absolute right-2 top-1/2 -translate-y-1/2 hover:bg-neutral-100 dark:hover:bg-neutral-800 p-1.5 rounded-sm'
>
- <X className='h-3.5 w-3.5 text-neutral-500 hover:text-neutral-900 dark:text-neutral-400 dark:hover:text-neutral-50' />
+ <X
+ aria-hidden='true'
+ className='h-3.5 w-3.5 text-neutral-500 hover:text-neutral-900 dark:text-neutral-400 dark:hover:text-neutral-50'
+ />
</button>
)}🤖 Prompt for AI Agents
In src/components/common/BasicSelect.tsx around lines 80 to 90, the
clear-selection button is missing an accessible name which breaks screen-reader
accessibility and the existing test that queries by role/name; add an aria-label
attribute (e.g. "Clear selection" or similar matching the test's regex) to the
button element so getByRole(..., { name: /clear/i }) and assistive technologies
can identify it, keeping all other behavior and classes the same.
Overview: This PR addresses issues with keyboard navigation and focus management within the
BasicSelectcomponent.Changes:
src/components/common/BasicSelect.tsx.Summary by CodeRabbit
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.