Skip to content

refactor: remove select dual states#8821

Open
araujogui wants to merge 2 commits intonodejs:mainfrom
araujogui:refactor/select-defaultvalue
Open

refactor: remove select dual states#8821
araujogui wants to merge 2 commits intonodejs:mainfrom
araujogui:refactor/select-defaultvalue

Conversation

@araujogui
Copy link
Copy Markdown
Member

@araujogui araujogui commented Apr 14, 2026

Description

Refactor selects with dual controlled state to just one

Validation

Try changing selects

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run pnpm format to ensure the code follows the style guide.
  • I have run pnpm test to check if all tests are passing.
  • I have run pnpm build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@araujogui araujogui requested a review from a team as a code owner April 14, 2026 15:45
Copilot AI review requested due to automatic review settings April 14, 2026 15:45
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nodejs-org Ready Ready Preview Apr 17, 2026 7:21pm

Request Review

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 14, 2026

PR Summary

Medium Risk
Touches a core UI primitive used across the site; regressions would show up as dropdowns not reflecting selections or not initializing correctly, especially around the noscript fallback and loading/undefined states.

Overview
Refactors the shared Select component to be fully controlled by replacing defaultValue with a value prop and removing its internal state/syncing logic.

Updates download-page dropdowns to pass value directly, adjusts the NoScriptSelect/StatelessSelect fallback API to keep using defaultValue, and aligns tests and Storybook stories (including a Storybook-controlled render) with the new controlled behavior.

Reviewed by Cursor Bugbot for commit 92c3f10. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions
Copy link
Copy Markdown
Contributor

👋 Codeowner Review Request

The following codeowners have been identified for the changed files:

Team reviewers: @nodejs/nodejs-website

Please review the changes when you have a chance. Thank you! 🙏

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.87%. Comparing base (5a6d7d2) to head (92c3f10).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/ui-components/src/Common/Select/index.tsx 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8821      +/-   ##
==========================================
- Coverage   73.90%   73.87%   -0.03%     
==========================================
  Files         105      105              
  Lines        8889     8883       -6     
  Branches      326      326              
==========================================
- Hits         6569     6562       -7     
- Misses       2319     2320       +1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the shared Select UI component to remove its internal “dual state” behavior and shift to an externally controlled value API, then updates downstream call sites (site download dropdowns + Storybook) to match.

Changes:

  • Replace defaultValue with value in SelectProps and remove internal state syncing from Select.
  • Update Storybook stories to keep value in sync via useArgs.
  • Update Downloads “Release” dropdown components to pass value instead of defaultValue, and adjust NoScript/Stateless select typings/wiring.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/ui-components/src/Common/Select/index.tsx Removes internal state and changes public API from defaultValuevalue.
packages/ui-components/src/Common/Select/index.stories.tsx Updates stories to use value and keeps Storybook args controlled via useArgs.
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx Adjusts props typing to reintroduce defaultValue semantics for StatelessSelect.
packages/ui-components/src/Common/Select/NoScriptSelect/index.tsx Threads defaultValue into JS Select as value and into noscript select as defaultValue.
apps/site/components/Downloads/Release/PlatformDropdown.tsx Migrates Select usage to value.
apps/site/components/Downloads/Release/PackageManagerDropdown.tsx Migrates Select usage to value.
apps/site/components/Downloads/Release/OperatingSystemDropdown.tsx Migrates Select usage to value.
apps/site/components/Downloads/Release/InstallationMethodDropdown.tsx Migrates Select usage to value.
Comments suppressed due to low confidence (2)

packages/ui-components/src/Common/Select/index.tsx:115

  • handleChange no longer updates any internal value. Because the trigger rendering (currentItem) is derived solely from the value prop, the Select won't visually update when used without a controlled value (or when callers still pass the now-ignored defaultValue prop). This is likely to break existing uncontrolled usage (and the current Select unit tests that pass defaultValue). Consider either (a) making value required and documenting Select as controlled-only, or (b) restoring an uncontrolled path (e.g., keep internal state only when value is undefined / support a defaultValue prop and avoid overriding SelectPrimitive.Value content in uncontrolled mode).
  const handleChange = (value: T) => {
    if (typeof onChange === 'function') {
      onChange(value);
    }
  };

packages/ui-components/src/Common/Select/index.tsx:35

  • This change removes the public defaultValue prop from SelectProps and replaces it with value, which is a breaking API change for any consumers using Select uncontrolled/with an initial default. If this package is consumed outside this repo, consider keeping defaultValue as a deprecated alias (or doing a major version bump + migration) to avoid silent runtime regressions (e.g., defaultValue currently becomes an ignored prop).
export type SelectProps<T extends string> = {
  values: Array<SelectGroup<T>> | Array<T> | Array<SelectValue<T>>;
  value?: T;
  placeholder?: string;
  label?: string;
  inline?: boolean;
  onChange?: (value: T) => void;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 8 to 22
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>
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.
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a1bd464. Configure here.

Comment thread packages/ui-components/src/Common/Select/index.tsx
@canerakdas
Copy link
Copy Markdown
Member

I think this reduces complexity and is a better naming choice. Applying this prop naming to StatelessSelect and NoScriptSelect as well would be great for consistency 🙌

It would also be worth checking doc-kit and learn to see whether these components/props are actually being used. I took a quick look and it doesn't seem like they are 👀

Copy link
Copy Markdown
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

@araujogui can you please verify all selects on all pages including with mobile and desktop viewport and with JavaScript enabled and disabled they still work and behave as expected?

@araujogui
Copy link
Copy Markdown
Member Author

I think this reduces complexity and is a better naming choice. Applying this prop naming to StatelessSelect and NoScriptSelect as well would be great for consistency 🙌

It would also be worth checking doc-kit and learn to see whether these components/props are actually being used. I took a quick look and it doesn't seem like they are 👀

I thought we could use value for controlled components and defaultValue for uncontrolled ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants