Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Updates download page dropdowns (OS/platform/install method/package manager) to pass Reviewed by Cursor Bugbot for commit 208a19d. Bugbot is set up for automated code reviews on this repo. Configure here. |
👋 Codeowner Review RequestThe 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! 🙏 |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
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
defaultValuewithvalueinSelectPropsand remove internal state syncing fromSelect. - Update Storybook stories to keep
valuein sync viauseArgs. - Update Downloads “Release” dropdown components to pass
valueinstead ofdefaultValue, 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 defaultValue → value. |
| 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
handleChangeno longer updates any internal value. Because the trigger rendering (currentItem) is derived solely from thevalueprop, the Select won't visually update when used without a controlledvalue(or when callers still pass the now-ignoreddefaultValueprop). This is likely to break existing uncontrolled usage (and the current Select unit tests that passdefaultValue). Consider either (a) makingvaluerequired and documenting Select as controlled-only, or (b) restoring an uncontrolled path (e.g., keep internal state only whenvalueis undefined / support adefaultValueprop and avoid overridingSelectPrimitive.Valuecontent 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
defaultValueprop fromSelectPropsand replaces it withvalue, 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 keepingdefaultValueas a deprecated alias (or doing a major version bump + migration) to avoid silent runtime regressions (e.g.,defaultValuecurrently 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.
| 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> |
There was a problem hiding this comment.
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).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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.
| const Select = <T extends string>({ | ||
| values = [], | ||
| defaultValue, | ||
| value, |
There was a problem hiding this comment.
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).
Reviewed by Cursor Bugbot for commit a1bd464. Configure here.
araujogui
force-pushed
the
refactor/select-defaultvalue
branch
from
a1bd464 to
208a19d
Compare
|
I think this reduces complexity and is a better naming choice. Applying this prop naming to It would also be worth checking |
Description
Refactor selects with dual controlled state to just one
Validation
Try changing selects
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.