Skip to content

refactor: remove select dual states #8821

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

Open
araujogui wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: remove select dual states #8821

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 apps/site/components/Downloads/Release/InstallationMethodDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const InstallationMethodDropdown: FC = () => {
return (
<Select<InstallationMethod | ''>
values={grouppedMethods}
defaultValue={release.installMethod}
value={release.installMethod}
loading={release.os === 'LOADING' || release.installMethod === ''}
ariaLabel={t('layouts.download.dropdown.platform')}
onChange={platform => platform && release.setInstallMethod(platform)}
Expand Down
2 changes: 1 addition & 1 deletion apps/site/components/Downloads/Release/OperatingSystemDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const OperatingSystemDropdown: FC<OperatingSystemDropdownProps> = () => {
return (
<Select<OperatingSystem>
values={parsedOperatingSystems}
defaultValue={release.os !== 'LOADING' ? release.os : undefined}
value={release.os !== 'LOADING' ? release.os : undefined}
loading={release.os === 'LOADING'}
placeholder={t('layouts.download.dropdown.unknown')}
ariaLabel={t('layouts.download.dropdown.os')}
Expand Down
2 changes: 1 addition & 1 deletion apps/site/components/Downloads/Release/PackageManagerDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const PackageManagerDropdown: FC = () => {
return (
<Select<PackageManager>
values={parsedPackageManagers}
defaultValue={release.packageManager}
value={release.packageManager}
loading={release.os === 'LOADING' || release.installMethod === ''}
ariaLabel={t('layouts.download.dropdown.packageManager')}
onChange={manager => release.setPackageManager(manager)}
Expand Down
2 changes: 1 addition & 1 deletion apps/site/components/Downloads/Release/PlatformDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const PlatformDropdown: FC = () => {
return (
<Select<Platform>
values={parsedPlatforms}
defaultValue={release.platform !== '' ? release.platform : undefined}
value={release.platform !== '' ? release.platform : undefined}
loading={release.os === 'LOADING' || release.platform === ''}
placeholder={t('layouts.download.dropdown.unknown')}
ariaLabel={t('layouts.download.dropdown.platform')}
Expand Down
5 changes: 3 additions & 2 deletions packages/ui-components/src/Common/Select/NoScriptSelect/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,18 @@ import type { StatelessSelectProps } from '#ui/Common/Select/StatelessSelect';

const WithNoScriptSelect = <T extends string>({
as,
defaultValue,
...props
}: StatelessSelectProps<T>) => {
const id = useId();
const selectId = `select-${id.replace(/[^a-zA-Z0-9]/g, '')}`;

return (
<>
<Select {...props} fallbackClass={selectId} />
<Select {...props} value={defaultValue} fallbackClass={selectId} />
<noscript>
<style>{`.${selectId} { display: none!important; }`}</style>
<StatelessSelect {...props} as={as} />
<StatelessSelect {...props} defaultValue={defaultValue} as={as} />
</noscript>
Comment on lines 8 to 22
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

WithNoScriptSelect accepts a prop named defaultValue, but it is now passed through to the interactive <Select> as a controlled value. If a consumer provides a static defaultValue without updating it on onChange, the selection will appear “stuck” (no internal updates). To avoid this API trap, consider renaming this prop to value (and/or adding a wrapper state for the JS-enabled Select while keeping defaultValue semantics for the noscript version).

Copilot uses AI. Check for mistakes.
</>
);
Expand Down
6 changes: 4 additions & 2 deletions packages/ui-components/src/Common/Select/StatelessSelect/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ type StatelessSelectConfig = {
as?: LinkLike | 'div';
};

export type StatelessSelectProps<T extends string> = SelectProps<T> &
StatelessSelectConfig;
export type StatelessSelectProps<T extends string> = Omit<SelectProps<T>, 'value'> &
StatelessSelectConfig & {
defaultValue?: T;
};

const StatelessSelect = <T extends string>({
values = [],
Expand Down
44 changes: 23 additions & 21 deletions packages/ui-components/src/Common/Select/index.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { useArgs } from 'storybook/preview-api';

import Select from '#ui/Common/Select';
import StatelessSelect from '#ui/Common/Select/StatelessSelect';
import * as OSIcons from '#ui/Icons/OperatingSystem';
Expand All @@ -10,22 +12,22 @@
export const Default: Story = {
args: {
values: ['v20.8.0', 'v19.9.0', 'v18.18.0', 'v17.9.1', 'v16.20.2'],
defaultValue: 'v16.20.2',
value: 'v16.20.2',
label: 'Node.js version',
},
};

export const WithoutLabel: Story = {
args: {
values: ['v20.8.0', 'v19.9.0', 'v18.18.0', 'v17.9.1', 'v16.20.2'],
defaultValue: 'v16.20.2',
value: 'v16.20.2',
},
};

export const WithScrollButtons: Story = {
args: {
values: Array.from({ length: 100 }, (_, i) => `Item ${i}`),
defaultValue: 'Item 50',
value: 'Item 50',
},
};

Expand All @@ -35,14 +37,8 @@
{
label: 'Getting Started',
items: [
{
value: 'section-1',
label: 'Getting Started',
},
{
value: 'section-2',
label: 'How to install Node.js',
},
{ value: 'section-1', label: 'Getting Started' },
{ value: 'section-2', label: 'How to install Node.js' },
{
value: 'section-3',
label: 'How much JavaScript do you need to know to use Node.js?',
Expand All @@ -51,18 +47,12 @@
value: 'section-4',
label: 'Differences between Node.js and the Browser',
},
{
value: 'section-5',
label: 'The V8 JavaScript Engine',
},
{ value: 'section-5', label: 'The V8 JavaScript Engine' },
{
value: 'section-6',
label: 'An introduction to the npm package manager',
},
{
value: 'section-7',
label: 'ECMAScript 2015 (ES6) and beyond',
},
{ value: 'section-7', label: 'ECMAScript 2015 (ES6) and beyond' },
{
value: 'section-8',
label: 'Node.js, the difference between development and production',
Expand Down Expand Up @@ -104,7 +94,7 @@
],
},
],
defaultValue: 'macos',
value: 'macos',
inline: true,
},
};
Expand All @@ -118,4 +108,16 @@
),
};

export default { component: Select } as Meta;
export default {
component: Select,
render: (args) => {
const [, setArgs] = useArgs();

Check failure on line 114 in packages/ui-components/src/Common/Select/index.stories.tsx

View workflow job for this annotation

GitHub Actions / Quality checks 

React Hook "useArgs" is called in function "render" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use"

const onChange = (value: string) => {
args.onChange?.(value);
setArgs({ value });
};

return <Select {...args} onChange={onChange} />;
},
} as Meta;
12 changes: 3 additions & 9 deletions packages/ui-components/src/Common/Select/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { ChevronDownIcon, ChevronUpIcon } from '@heroicons/react/24/outline';
import * as SelectPrimitive from '@radix-ui/react-select';
import classNames from 'classnames';
import { useEffect, useId, useMemo, useState } from 'react';
import { useId, useMemo } from 'react';

import Skeleton from '#ui/Common/Skeleton';

Expand All @@ -28,7 +28,7 @@ export type SelectGroup<T extends string> = {

export type SelectProps<T extends string> = {
values: Array<SelectGroup<T>> | Array<T> | Array<SelectValue<T>>;
defaultValue?: T;
value?: T;
placeholder?: string;
label?: string;
inline?: boolean;
Expand All @@ -49,7 +49,7 @@ export type SelectProps<T extends string> = {

const Select = <T extends string>({
values = [],
defaultValue,
value,
Copy link
Copy Markdown

@cursor cursor bot Apr 14, 2026

Choose a reason for hiding this comment

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

Tests pass obsolete defaultValue prop to refactored Select

Medium Severity

The Select component's prop was renamed from defaultValue to value, but the existing test file (__tests__/index.test.jsx) was not updated. Because the test file is .jsx (not TypeScript), passing the now-unrecognized defaultValue prop won't produce a type error — it's silently ignored. This means Select renders with value={undefined}, so currentItem is undefined, and the "renders the default value when provided" test either fails or passes for the wrong reason (matching text in dropdown items rather than the selected trigger value).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a1bd464. Configure here.

placeholder,
label,
inline,
Expand All @@ -62,9 +62,6 @@ const Select = <T extends string>({
fallbackClass = '',
}: SelectProps<T>): ReactNode => {
const id = useId();
const [value, setValue] = useState(defaultValue);

useEffect(() => setValue(defaultValue), [defaultValue]);

const mappedValues = useMemo(() => mapValues(values), [values]) as Array<
SelectGroup<T>
Expand Down Expand Up @@ -111,10 +108,7 @@ const Select = <T extends string>({
// eslint-disable-next-line @eslint-react/exhaustive-deps
}, [JSON.stringify(values)]);

// Both change the internal state and emit the change event
const handleChange = (value: T) => {
setValue(value);

if (typeof onChange === 'function') {
onChange(value);
}
Expand Down
Loading