Skip to content

Fix: Enhance keyboard navigation and focus for BasicSelect component #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fehranbit merged 1 commit into from
Dec 12, 2025
Merged

Fix: Enhance keyboard navigation and focus for BasicSelect component #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"lint": "npx eslint",
"format": "npx prettier --write .",
"prisma:migrate": "npx prisma migrate dev",
"test": "jest"
"test": "npx jest"
},
"dependencies": {
"@eslint/js": "^9.37.0",
Expand Down
114 changes: 114 additions & 0 deletions src/__tests__/BasicSelect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { render, screen, fireEvent } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { BasicSelect } from '../components/common/BasicSelect';

describe('BasicSelect', () => {
const mockItems = [
{ label: 'Option 1', value: '1' },
{ label: 'Option 2', value: '2' },
{ label: 'Option 3', value: '3' },
];

const mockOnChange = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
});

it('renders with placeholder and opens on click', async () => {
render(
<BasicSelect
placeholder="Select an option"
items={mockItems}
onChange={mockOnChange}
/>
);

expect(screen.getByText('Select an option')).toBeInTheDocument();

await userEvent.click(screen.getByRole('combobox'));
expect(screen.getByText('Option 1')).toBeInTheDocument();
expect(screen.getByText('Option 2')).toBeInTheDocument();
expect(screen.getByText('Option 3')).toBeInTheDocument();
});

it('allows selecting an option with click', async () => {
render(
<BasicSelect
placeholder="Select an option"
items={mockItems}
onChange={mockOnChange}
/>
);

await userEvent.click(screen.getByRole('combobox'));
await userEvent.click(screen.getByText('Option 2'));

expect(mockOnChange).toHaveBeenCalledWith('2');
expect(screen.queryByText('Option 1')).not.toBeInTheDocument(); // Menu should close
expect(screen.getByText('Option 2')).toBeInTheDocument(); // Selected value
});

it('navigates options with arrow keys and selects with Enter', async () => {
render(
<BasicSelect
placeholder="Select an option"
items={mockItems}
onChange={mockOnChange}
/>
);

const combobox = screen.getByRole('combobox');
await userEvent.click(combobox); // Open the select

// Navigate down to Option 2
fireEvent.keyDown(combobox, { key: 'ArrowDown' }); // Focus Option 1
fireEvent.keyDown(combobox, { key: 'ArrowDown' }); // Focus Option 2

// Select Option 2 with Enter
fireEvent.keyDown(combobox, { key: 'Enter' });

expect(mockOnChange).toHaveBeenCalledWith('2');
expect(screen.queryByText('Option 1')).not.toBeInTheDocument(); // Menu should close
expect(screen.getByText('Option 2')).toBeInTheDocument(); // Selected value
});

it('closes the select with Escape key', async () => {
render(
<BasicSelect
placeholder="Select an option"
items={mockItems}
onChange={mockOnChange}
/>
);

const combobox = screen.getByRole('combobox');
await userEvent.click(combobox); // Open the select

expect(screen.getByText('Option 1')).toBeInTheDocument(); // Menu is open

fireEvent.keyDown(combobox, { key: 'Escape' });

expect(screen.queryByText('Option 1')).not.toBeInTheDocument(); // Menu should close
});

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();
});
Comment on lines +95 to +113
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

});
89 changes: 45 additions & 44 deletions src/components/common/BasicSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,52 +42,53 @@ export function BasicSelect({
)}
{helperText && <FormDescription>{helperText}</FormDescription>}
<div className='relative'>
<Select
disabled={disabled}
onValueChange={onChange}
value={value?.toString() ?? ''}
onOpenChange={(isOpen) => {
if (!isOpen && !hasValue) {
onChange(null);
}
}}
>
<SelectTrigger
className={cn(
'bg-transparent hover:bg-transparent focus:ring-0 border-neutral-700/60 w-full dark:bg-neutral-900 dark:text-white dark:border-neutral-700',
allowEmpty && hasValue && 'pr-12',
className
)}
>
<SelectValue placeholder={placeholder} />
</SelectTrigger>
<SelectContent className='dark:bg-neutral-900 dark:border-neutral-700'>
<SelectGroup>
{items.map(({ value, label, disabled }) => (
<SelectItem
key={value}
value={value.toString()}
disabled={disabled}
className='dark:text-white dark:focus:bg-neutral-800 dark:focus:text-white'
>
{label}
</SelectItem>
))}
</SelectGroup>
</SelectContent>
</Select>
{allowEmpty && hasValue && (
<button
type='button'
onClick={(e) => {
e.stopPropagation();
onChange(null);
<div className="relative flex items-center">
<Select
disabled={disabled}
onValueChange={onChange}
value={value?.toString() ?? ''}
onOpenChange={(isOpen) => {
if (!isOpen && !hasValue) {
onChange(null);
}
}}
Comment on lines +50 to 54
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

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>
)}
<SelectTrigger
className={cn(
'bg-transparent hover:bg-transparent focus:ring-0 border-neutral-700/60 w-full dark:bg-neutral-900 dark:text-white dark:border-neutral-700',
allowEmpty && hasValue && 'pr-12',
className
)}
>
<SelectValue placeholder={placeholder} />
</SelectTrigger>
<SelectContent className='dark:bg-neutral-900 dark:border-neutral-700'>
<SelectGroup>
{items.map(({ value, label, disabled }) => (
<SelectItem
key={value}
value={value.toString()}
disabled={disabled}
className='dark:text-white dark:focus:bg-neutral-800 dark:focus:text-white'
>
{label}
</SelectItem>
))}
</SelectGroup>
</SelectContent>
</Select>
{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>
)}
Comment on lines +80 to +90
Copy link

@coderabbitai coderabbitai bot Dec 12, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

</div>
</div>
</div>
);
Expand Down
Loading