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 2 commits 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
2 commits
Select commit Hold shift + click to select a range
1480df2
refactor: remove select dual states
araujogui Apr 14, 2026
92c3f10
test: change property name
araujogui Apr 17, 2026
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
9 changes: 7 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 @@ -15,8 +15,13 @@ 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
4 changes: 2 additions & 2 deletions packages/ui-components/src/Common/Select/__tests__/index.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Select', () => {
const { getByLabelText } = render(
<Select
values={values}
defaultValue={defaultValue}
value={defaultValue}
placeholder={placeholder}
dropdownLabel={dropdownLabel}
label={label}
Expand All @@ -38,7 +38,7 @@ describe('Select', () => {
const { getByText } = render(
<Select
values={values}
defaultValue={defaultValue}
value={defaultValue}
placeholder={placeholder}
dropdownLabel={dropdownLabel}
label={label}
Expand Down
45 changes: 24 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 @@ type Meta = MetaObj<typeof Select>;
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 @@ export const DropdownLabel: Story = {
{
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 @@ export const DropdownLabel: Story = {
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 @@ export const InlineSelect: Story = {
],
},
],
defaultValue: 'macos',
value: 'macos',
inline: true,
},
};
Expand All @@ -118,4 +108,17 @@ export const WithNoScriptSelect: Story = {
),
};

export default { component: Select } as Meta;
export default {
component: Select,
render: args => {
// eslint-disable-next-line @eslint-react/rules-of-hooks
const [, setArgs] = useArgs();

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 Badge, { type BadgeKind } from '#ui/Common/Badge';
import Skeleton from '#ui/Common/Skeleton';
Expand Down Expand Up @@ -33,7 +33,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 @@ -54,7 +54,7 @@ export type SelectProps<T extends string> = {

const Select = <T extends string>({
values = [],
defaultValue,
value,
Comment thread
cursor[bot] marked this conversation as resolved.
placeholder,
label,
inline,
Expand All @@ -67,9 +67,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 @@ -121,10 +118,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