UI: Fix crashes in Select when passed falsy non-string options #33164
Conversation
📝 WalkthroughWalkthroughSelect and its helpers were modified to support non-string option values (number, boolean, null, undefined) via an internal representation (Symbol for undefined). Select public prop types and callbacks now use a Value union; stories and several callers were updated to exercise and new-typed behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Select as Select Component
participant Helpers as helpers.tsx
participant Consumer as onSelect/onChange callbacks
User->>Select: interact (open/select/deselect/reset)
Select->>Helpers: externalToValue(externalValue)
Note right of Helpers `#eef2ff`: maps undefined -> UNDEFINED_VALUE (Symbol)
Helpers-->>Select: internalValue (InternalValue)
Select->>Select: store/render InternalOption / ResetOption, update active/focus
Select->>Helpers: valueToExternal(internalValue)
Helpers-->>Select: externalValue
Select->>Consumer: call onSelect/onDeselect/onChange with externalValue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
code/core/src/components/components/Select/helpers.tsx (1)
28-37:isLiteralValueincludes symbol check butValuetype excludes symbols.The function checks
typeof value === 'symbol'(line 35), but theValuetype on line 8 isstring | number | null | boolean | undefinedand doesn't includesymbol. This makes the symbol check unreachable when the input is typed asValue | Value[] | undefined.If the symbol check is intentional for internal use (e.g., checking if an
InternalValuecontainingUNDEFINED_VALUEis a literal), consider either:
- Updating the function signature to accept
InternalValueas well, or- Removing the symbol check if it's truly unreachable.
export function isLiteralValue(value: Value | Value[] | undefined): value is Value { return ( value === undefined || value === null || typeof value === 'string' || typeof value === 'number' || - typeof value === 'boolean' || - typeof value === 'symbol' + typeof value === 'boolean' ); }code/core/src/components/components/Select/Select.tsx (1)
272-285: Consider typingresetOptionexplicitly asResetOption | undefined.The
resetOptionmemo returns an object literal withtype: 'reset', but TypeScript may infer a wider type. Adding an explicit type annotation would ensure consistency with theResetOptioninterface:- const resetOption = useMemo( + const resetOption = useMemo<ResetOption | undefined>( () => onReset ? { - type: 'reset', + type: 'reset' as const, value: undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/components/components/Select/Select.stories.tsx(1 hunks)code/core/src/components/components/Select/Select.tsx(12 hunks)code/core/src/components/components/Select/helpers.tsx(1 hunks)code/core/src/toolbar/components/ToolbarMenuSelect.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/Select/Select.stories.tsxcode/core/src/components/components/Select/helpers.tsxcode/core/src/components/components/Select/Select.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/Select/Select.stories.tsxcode/core/src/components/components/Select/helpers.tsxcode/core/src/components/components/Select/Select.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/Select/Select.stories.tsxcode/core/src/components/components/Select/helpers.tsxcode/core/src/components/components/Select/Select.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/Select/Select.stories.tsxcode/core/src/components/components/Select/helpers.tsxcode/core/src/components/components/Select/Select.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Select/Select.stories.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/Select/Select.stories.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Select/Select.stories.tsx (4)
code/core/src/csf-tools/CsfFile.ts (1)
meta(964-966)code/core/src/node-logger/logger/logger.ts (1)
step(197-202)code/core/src/test/testing-library.ts (1)
userEvent(123-126)code/renderers/react/template/stories/js-argtypes.stories.jsx (1)
args(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (10)
code/core/src/toolbar/components/ToolbarMenuSelect.tsx (2)
59-60: LGTM! Clean type guard consolidation.The single type guard filter with
item is ToolbarItemis cleaner than the previous two-step filtering approach and provides better TypeScript type narrowing for the subsequentmapoperation.
94-95: Potential unintended selection whencurrentValueis undefined.Wrapping
currentValuein an array creates[undefined]whenglobals[id]is undefined. Per the new Select behavior (as demonstrated inDefaultOptionUndefinedInArrayWorksstory),[undefined]will select an option withvalue: undefinedif one exists, whereas previously a bareundefinedwas filtered out.If toolbar items can have
undefinedas a valid option value, this change might unintentionally pre-select that option when no global is set. Consider whether this is the intended behavior or if you need to filter out undefined:- defaultOptions={[currentValue]} + defaultOptions={currentValue !== undefined ? [currentValue] : undefined}code/core/src/components/components/Select/helpers.tsx (2)
5-6: Good use ofSymbol.forfor the undefined sentinel.Using
Symbol.for('undefined')ensures the same symbol is returned across different parts of the codebase, which is important for value comparisons. This cleanly separates the "undefined as a valid option value" case from the "reset/no selection" case.
39-68: LGTM! Clean conversion utilities.The bidirectional conversion functions (
optionToInternal,valueToExternal,externalToValue) correctly handle the mapping between externalundefinedand internalUNDEFINED_VALUEsentinel. TheoptionOrResetToInternalfunction appropriately preserves reset options while converting regular options.code/core/src/components/components/Select/Select.stories.tsx (3)
1059-1067: Comprehensive test data covering all falsy and non-string values.The
nonStringOptionsarray covers all the edge cases that previously caused crashes:0,false,null,undefined, along with truthy non-string values. This is excellent test coverage for the bug fix.
1234-1258: Good documentation of undefined handling semantics through stories.These two stories clearly document the asymmetry:
defaultOptions: undefinedmeans "no default" whiledefaultOptions: [undefined]means "select the option with value undefined". This is valuable for maintainers understanding the API behavior.
1288-1333: Critical test for Reset vs Undefined option distinction.This story validates that the
UNDEFINED_VALUEsymbol correctly separates the reset functionality from an actualundefinedoption value. The test flow (select → select undefined → reset → select undefined again) thoroughly exercises this edge case.code/core/src/components/components/Select/Select.tsx (3)
91-106: LGTM! Clean handling of defaultOptions with proper internal conversion.The function correctly:
- Returns empty array for bare
undefined(no default selection)- Uses
isLiteralValueto distinguish single value from array- Maps matched options through
optionToInternalfor internal state
232-269: Solid implementation of selection logic with type discrimination.The
option.type === 'reset'check cleanly handles reset options separately from regular options, andvalueToExternalcorrectly converts internal values back to externalValuetypes for callbacks.
533-586: Good dual-mapping approach for internal/external option handling.Mapping to both
option(internal) andexternalOptionallows proper internal state management while still passing the original option tosetActiveOptionviaonFocus. This preserves the expected behavior while supporting the new value types.
|
View your CI Pipeline Execution ↗ for commit 9aeb817
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/pseudo-states/src/manager/PseudoStateTool.tsx (1)
37-41: Multi-select reduction correctly builds the pseudo-state mapUsing
curr as stringinsidereducematches the fact that option values are the string keys fromPSEUDO_STATES, so the resulting{ [state]: true }map is type-safe and behaviorally unchanged.If you ever care about tiny perf wins here, you could mutate the accumulator instead of recreating it on each iteration:
const next = selected.reduce<Record<string, boolean>>((acc, curr) => { acc[curr as string] = true; return acc; }, {});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/addons/pseudo-states/src/manager/PseudoStateTool.tsx(1 hunks)code/core/src/backgrounds/components/Tool.tsx(1 hunks)code/core/src/manager/components/sidebar/RefIndicator.tsx(1 hunks)code/core/src/toolbar/components/ToolbarMenuSelect.tsx(2 hunks)code/core/src/viewport/components/Tool.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/manager/components/sidebar/RefIndicator.tsxcode/core/src/viewport/components/Tool.tsxcode/core/src/backgrounds/components/Tool.tsxcode/addons/pseudo-states/src/manager/PseudoStateTool.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/manager/components/sidebar/RefIndicator.tsxcode/core/src/viewport/components/Tool.tsxcode/core/src/backgrounds/components/Tool.tsxcode/addons/pseudo-states/src/manager/PseudoStateTool.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/manager/components/sidebar/RefIndicator.tsxcode/core/src/viewport/components/Tool.tsxcode/core/src/backgrounds/components/Tool.tsxcode/addons/pseudo-states/src/manager/PseudoStateTool.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/manager/components/sidebar/RefIndicator.tsxcode/core/src/viewport/components/Tool.tsxcode/core/src/backgrounds/components/Tool.tsxcode/addons/pseudo-states/src/manager/PseudoStateTool.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsx
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/viewport/components/Tool.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/viewport/components/Tool.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/manager/components/sidebar/RefIndicator.tsx (1)
200-205: String cast inonSelectmatches how options are constructed
optionsonly ever use string ids asvalue, so treatingitemas a string to indexref.versionsis consistent and safe, and the clarifying comment helps future readers.code/core/src/backgrounds/components/Tool.tsx (1)
96-108: Select handler correctly narrows the selected value to a background nameGiven that
optionsuse the background map keys (strings) asvalue, castingselectedtostringbefore storing it in globals is consistent with the existing data model and keeps behavior unchanged under the new Select typing.code/core/src/viewport/components/Tool.tsx (1)
112-126: ViewportonSelectcast is consistent with viewport option keysSince viewport options use the map keys (strings) as their
value, castingselectedtostringbefore updating{ value, isRotated: false }is a safe way to align with the updated Select typings without changing behavior.code/core/src/toolbar/components/ToolbarMenuSelect.tsx (1)
57-63: Toolbar option narrowing, title fallback, anddefaultOptionsshape all look correct
- The
.filter((item): item is ToolbarItem => item.type === 'item')guard cleanly narrows to selectable items before mapping.- The
itemTitlefallback to'Untitled'makes the menu more resilient when a config omits bothtitleandvalue.- Passing
defaultOptions={[currentValue]}aligns with aValue | Value[]Select API and preserves whatever value type the toolbar uses for this global.No issues from a correctness or UX standpoint.
Also applies to: 95-103
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/core/src/components/components/Select/Select.tsx (1)
81-83: Nullish coalescing afterString()is ineffective (regression or not fixed).
String(value)always returns a string (including"undefined"forundefined), so??never triggers. For the reset option wherevalueisundefined, this produces"select-...-opt-undefined"instead of the intended"select-...-opt-sb-reset".A past review comment flagged this same issue and claimed it was addressed, but the current code still contains the bug.
Apply this diff to fix:
- return `${parentId}-opt-${String(value) ?? 'sb-reset'}`; + return `${parentId}-opt-${value === undefined ? 'sb-reset' : String(value)}`;
🧹 Nitpick comments (1)
code/core/src/components/components/Select/Select.tsx (1)
17-26: Consider adding a comparison helper to the helpers module.The comparison between external options (from the
optionsarray) and internal options (activeOption,selectedOptions) is repeated in multiple places and has subtle behavior forResetOption. Consider adding a helper function tohelpers.tsx:export function compareOptionValues( externalOption: Option | ResetOption, internalOption: InternalOption | ResetOption ): boolean { if ('type' in externalOption && externalOption.type === 'reset') { return 'type' in internalOption && internalOption.type === 'reset'; } return externalToValue(externalOption.value) === internalOption.value; }This would make the comparison logic explicit, reduce duplication, and prevent similar bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/components/components/Select/Select.tsx(12 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/Select/Select.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/Select/Select.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/Select/Select.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/Select/Select.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Select/Select.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/Select/Select.tsx (1)
code/core/src/components/components/Select/helpers.tsx (8)
Value(8-8)InternalOption(19-22)ResetOption(23-26)isLiteralValue(28-37)optionToInternal(39-45)valueToExternal(56-61)optionOrResetToInternal(47-54)externalToValue(63-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/core/src/components/components/Select/Select.tsx (4)
91-106: LGTM!The function correctly handles both single values and arrays for
defaultOptions, properly converts them to the internal representation, and appropriately handlesundefinedas a valid value.
227-269: LGTM!The state management and selection handling correctly:
- Uses internal representation for state
- Converts to external values when invoking callbacks
- Properly discriminates reset options using the type field
- Handles both single-select and multi-select modes appropriately
370-414: LGTM!The keyboard handler correctly determines the initial focus position. The comparison at line 381 works properly because it compares internal representations on both sides (after
externalToValueconversion), and the reset option is never inselectedOptions.
533-586: Rendering logic is sound, but depends on fixingvalueToId.The rendering correctly:
- Maps options to internal representation for consistent comparison
- Uses the proper pattern for the
keyprop (line 546) that was fixed from a past review- Compares internal values when determining selection state (line 540-541)
- Passes the external option to
onFocusfor proper re-conversionHowever, the
idgeneration at line 557 depends onvalueToId(lines 81-83), which has the bug flagged earlier. Once that's fixed, this rendering logic will work correctly.
| const currentIndex = options.findIndex( | ||
| (option) => externalToValue(option.value) === activeOption.value | ||
| ); |
There was a problem hiding this comment.
Comparison fails for active reset option, breaking keyboard navigation.
The comparison externalToValue(option.value) === activeOption.value doesn't work when activeOption is the reset option. Here's why:
ResetOptionkeepsvalue: undefined(not converted to a symbol)InternalOptionconvertsundefinedtoUNDEFINED_VALUEsymbol- When comparing:
externalToValue(undefined)returnsUNDEFINED_VALUEsymbol, butactiveOption.valueisundefined - Result:
UNDEFINED_VALUE === undefinedisfalse, sofindIndexreturns-1 - This causes incorrect keyboard navigation behavior when the reset option is active
Apply this diff to fix the comparison:
- const currentIndex = options.findIndex(
- (option) => externalToValue(option.value) === activeOption.value
- );
+ const currentIndex = options.findIndex((option) => {
+ if ('type' in option && option.type === 'reset') {
+ return 'type' in activeOption && activeOption.type === 'reset';
+ }
+ return externalToValue(option.value) === activeOption.value;
+ });🤖 Prompt for AI Agents
In code/core/src/components/components/Select/Select.tsx around lines 328 to
330, the findIndex comparison compares externalToValue(option.value) to
activeOption.value which fails when activeOption is the reset option (undefined
vs UNDEFINED_VALUE). Replace the comparison so both sides are normalized via
externalToValue: use externalToValue(option.value) ===
externalToValue(activeOption.value) so the reset option matches correctly and
keyboard navigation works.
| const currentIndex = options.findIndex( | ||
| (option) => externalToValue(option.value) === activeOption.value | ||
| ); |
There was a problem hiding this comment.
Same comparison bug as moveActiveOptionDown.
This has the identical issue flagged in moveActiveOptionDown (lines 328-330) where the comparison fails when activeOption is the reset option.
Apply the same fix:
- const currentIndex = options.findIndex(
- (option) => externalToValue(option.value) === activeOption.value
- );
+ const currentIndex = options.findIndex((option) => {
+ if ('type' in option && option.type === 'reset') {
+ return 'type' in activeOption && activeOption.type === 'reset';
+ }
+ return externalToValue(option.value) === activeOption.value;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentIndex = options.findIndex( | |
| (option) => externalToValue(option.value) === activeOption.value | |
| ); | |
| const currentIndex = options.findIndex((option) => { | |
| if ('type' in option && option.type === 'reset') { | |
| return 'type' in activeOption && activeOption.type === 'reset'; | |
| } | |
| return externalToValue(option.value) === activeOption.value; | |
| }); |
🤖 Prompt for AI Agents
In code/core/src/components/components/Select/Select.tsx around lines 352-354,
the findIndex comparison fails when activeOption is the reset option; update the
comparison to normalize both sides (apply externalToValue to activeOption.value
as well) and guard against activeOption being undefined/null so the equality
check uses externalToValue(option.value) ===
externalToValue(activeOption?.value).
What I did
Fixes an internal bug report, where a global type created with
trueandundefinedvalues did not work well with Select:undefinedvalue was filtered outtruevalue caused a crash upon selection because type checking indefaultOptionswas not defensive enoughThe fix is performed by allowing a wider range of values in Select
optionsand internally convertingundefinedtoSymbol.for('undefined')so it does not conflict with the 'Reset' option.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
@yannbf has a repo with a local addon using both
trueandundefined, to be tested against.Anyone else: please run the new stories that should cover all potential use cases within Select.
Documentation
ø
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.