Controls: Fix displaying as object instead of select for optional union types #33200
Conversation
…n types Fixes storybookjs#12641 When TypeScript properties are optional (prop?: Type), TypeScript adds | undefined to the union type. The enum detection logic was failing because undefined is not a literal type, causing the union to not be recognized as an enum. This resulted in controls displaying as "object" type instead of "select" dropdown for optional properties with union types, regardless of the number of items in the union. The fix filters out undefined elements before checking if all remaining elements are literals, allowing optional unions to be correctly recognized as enums and display as select controls.
📝 WalkthroughWalkthroughAdded literal/undefined type guards and refined union handling in the TypeScript argTypes converter to exclude Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (1)
44-58: Wrap the case block in braces to scope the variable declaration.The static analysis tool correctly identifies that
nonUndefinedElementsshould be block-scoped within theunioncase. While the current code works due to the return statements, it's a best practice to wrap case blocks containing variable declarations in braces.Apply this diff to fix the scoping issue:
- case 'union': + case 'union': { let result; // Filter out undefined from optional props when checking for enum const nonUndefinedElements = type.elements?.filter((element) => element.name !== 'undefined'); if (nonUndefinedElements?.length > 0 && nonUndefinedElements.every((element) => element.name === 'literal')) { result = { ...base, name: 'enum', // @ts-expect-error fix types value: nonUndefinedElements.map((v) => parseLiteral(v.value)), }; } else { result = { ...base, name, value: type.elements?.map(convert) }; } return result; + }
🧹 Nitpick comments (1)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (1)
52-53: Consider fixing the TypeScript error instead of suppressing it.The
@ts-expect-errorcomment suggests thatparseLiteralmight expect a different type than whatv.valueprovides. Consider adding proper type assertions or fixing the type definitions rather than suppressing the error.If the type is truly incorrect, you could add a type assertion:
// @ts-expect-error fix types - value: nonUndefinedElements.map((v) => parseLiteral(v.value)), + value: nonUndefinedElements.map((v) => parseLiteral(v.value as string)),Or investigate whether the type definitions in
types.tsorparseLiteralneed to be updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/docs-tools/argTypes/convert/typescript/convert.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
🧠 Learnings (1)
📓 Common learnings
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.
🪛 Biome (2.1.2)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
[error] 47-47: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (1)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (1)
46-53: Add regression tests for TypeScript union type handling in optional properties.This fix for optional properties with union types (#12641) should include unit tests to prevent regression. Test coverage should include:
- Optional properties with literal unions (e.g.,
prop?: 'a' | 'b' | 'c')- Mixed unions with
undefined(e.g.,'a' | 'b' | undefined)- Edge case: union containing only
undefinedCheck existing test coverage in
code/core/src/docs-tools/argTypes/convert/typescript/for these scenarios and add tests if absent.
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the contribution @tanujbhaud!
Did you test whether it's possible to clear the select once an option has been selected?
We have another Select implementation in storybook/internal/components which has a reset option. @ghengeveld I'm curious to see what you think. Should we switch Form.Select to the newer Select with a reset option?
Oh my goodness please. This is a significant missing feature from controls, currently requireing awkward mappings to allow for either genuinely optional properties, or just the ability to test that defaults are corretly applied. (Same would be nice for tri-state bools and radios, but I guess we can achieve that with a resettable select) |
Given the size of our user base, someone will unavoidably report that it's now impossible to clear that control without resetting all controls :) The requirement to allow clearing was already highlighted by the author of the bug report. |
|
View your CI Pipeline Execution ↗ for commit d837343 ☁️ Nx Cloud last updated this comment at |
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
CI reports several type errors, and you stripped the part of the PR template requesting a manual testing procedure.
Please provide both automatic tests and a manual testing procedure we can replicate to validate the PR.
Thanks,
| let result; | ||
| if (type.elements?.every((element) => element.name === 'literal')) { | ||
| // Filter out undefined from optional props when checking for enum | ||
| const nonUndefinedElements = type.elements?.filter((element) => element.name !== 'undefined'); |
There was a problem hiding this comment.
The fact that this throws TS errors either means that TSType is wrong, or that the check isn't done correctly, or both.
My preference would go towards fixing TSType to actually match all the inputs that can be received at runtime (so we can discover other type bugs elsewhere).
Can you also please check if typeof element.raw is consistently undefined when element.name is 'undefined'? If so, then inspecting the type of raw (like done on line 29) could be safer.
| if (type.elements?.every((element) => element.name === 'literal')) { | ||
| // Filter out undefined from optional props when checking for enum | ||
| const nonUndefinedElements = type.elements?.filter((element) => element.name !== 'undefined'); | ||
| if (nonUndefinedElements?.length > 0 && nonUndefinedElements.every((element) => element.name === 'literal')) { |
There was a problem hiding this comment.
Type errors here if nonUndefinedElements is undefined.
| // @ts-expect-error fix types | ||
| value: type.elements?.map((v) => parseLiteral(v.value)), | ||
| value: nonUndefinedElements.map((v) => parseLiteral(v.value)), |
There was a problem hiding this comment.
This might be fixable if the code above used type guard functions. The fact that you found a type bug in a code path with @ts-expect-error hints that we really want to remove @ts-expect-error and actually get types right this time (without any casting, just with type guards).
- Add type guard functions (isLiteral, isUndefined) for better type narrowing - Add 'undefined' to TSScalarType union in types.ts - Make elements non-optional in TSCombinationType (unions always have elements) - Wrap case block in braces to fix Biome linting error - Use proper type assertion instead of @ts-expect-error
tanujbhaud
force-pushed
the
fix/controls-optional-union-enum
branch
from
0e34418 to
d3b4545
Compare
|
Hi @Sidnioulz, thanks for the review! I've addressed the feedback:
Let me know if anything else is needed! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (2)
7-9: Consider using proper TypeScript type guard syntax for automatic narrowing.The comment says "Type guards for narrowing" but these are boolean predicates, not TypeScript type guards. Using the
iskeyword would enable automatic type narrowing and eliminate the need for the explicit cast on line 58.-// Type guards for narrowing TSType discriminant unions -const isLiteral = (type: TSType): boolean => type.name === 'literal'; -const isUndefined = (type: TSType): boolean => type.name === 'undefined'; +// Type guards for narrowing TSType discriminant unions +const isLiteral = (type: TSType): type is Extract<TSType, { name: 'literal' }> => + type.name === 'literal'; +const isUndefined = (type: TSType): type is Extract<TSType, { name: 'undefined' }> => + type.name === 'undefined';With proper type guards, the cast on line 58 could be removed since TypeScript would infer the narrowed type after the
every(isLiteral)check when iterating withmap.
56-60: The type assertion is safe but could be eliminated with proper type guards.The cast to
Extract<typeof element, { name: 'literal' }>is logically safe becauseallLiteralsguarantees allnonUndefinedElementshavename === 'literal'. However, TypeScript can't infer this without proper type guard syntax.If you adopt the type guard refactor suggested above, this block simplifies to:
- value: nonUndefinedElements.map((element) => { - // We know element is a literal type because of the allLiterals check - const literalElement = element as Extract<typeof element, { name: 'literal' }>; - return parseLiteral(literalElement.value); - }), + value: nonUndefinedElements.map((element) => parseLiteral(element.raw!)),Note: This assumes
rawcontains the literal value. Ifvalueis a separate property on literal types not currently in the type definition, you'd need to updateTSScalarTypeto include it whenname === 'literal'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts(2 hunks)code/core/src/docs-tools/argTypes/convert/typescript/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.tscode/core/src/docs-tools/argTypes/convert/typescript/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.tscode/core/src/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/docs-tools/argTypes/convert/typescript/convert.tscode/core/src/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.tscode/core/src/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/docs-tools/argTypes/convert/typescript/convert.tscode/core/src/docs-tools/argTypes/convert/typescript/types.ts
🧠 Learnings (2)
📓 Common learnings
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.
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.
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode
Applied to files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
🧬 Code graph analysis (1)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (1)
code/core/src/docs-tools/argTypes/convert/typescript/types.ts (1)
TSType(46-46)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (4)
code/core/src/docs-tools/argTypes/convert/typescript/types.ts (2)
12-12: LGTM! Makingelementsrequired is the right fix.This addresses the past review feedback about type errors when
nonUndefinedElementscould be undefined. Nowtype.elementsis guaranteed to exist, enabling safe filtering and mapping inconvert.ts.
36-36: LGTM! Adding'undefined'to scalar names enables proper type narrowing.This allows the
isUndefinedtype guard to correctly identify undefined elements in unions, addressing the past feedback about fixingTSTypeto match runtime inputs rather than using@ts-expect-error.code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (2)
48-64: Core logic correctly handles optional union types.The approach is sound:
- Filter out
undefinedelements first- Check if all remaining elements are literals
- Build enum from non-undefined elements only, or fall back to original behavior
This correctly fixes the issue where optional props (
prop?: 'a' | 'b') were displayed as "object" controls instead of "select".
58-59: No action needed. Thevalueproperty is properly defined on Storybook's literal type definitions (as part ofSBEnumTypeandSBUnionTypein the type system). The type cast correctly narrows the type and accesses a property that exists in the type definitions. The type definitions are complete and not incomplete as originally suggested.
- Separate TSLiteralType from TSScalarType to include 'value' property - Use proper TypeScript type guard syntax (type is X) for narrowing - Filter with type guard to get properly typed literal elements - Removes need for explicit type casting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/docs-tools/argTypes/convert/typescript/types.ts (1)
35-42: Consider broadeningTSLiteralType.valuefor non‑string literal supportIf these types ever need to represent numeric/boolean/null literals (not just string keys),
value: stringloses some information and allows mismatches. You might future‑proof this asstring | number | boolean | null(orunknown) and narrow where needed; if the intent is to normalize everything to strings for controls, a brief comment here stating that invariant would help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/docs-tools/argTypes/convert/typescript/convert.ts(2 hunks)code/core/src/docs-tools/argTypes/convert/typescript/types.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/docs-tools/argTypes/convert/typescript/convert.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
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/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
🧠 Learnings (2)
📓 Common learnings
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-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode
Applied to files:
code/core/src/docs-tools/argTypes/convert/typescript/types.ts
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (2)
code/core/src/docs-tools/argTypes/convert/typescript/types.ts (2)
10-13: RequiredelementsonTSCombinationTypealigns with usage; just confirm no callers omit itMaking
elementsmandatory better reflects how unions/intersections are actually used and removes the need for defensive checks under strict mode. Please double‑check any code constructingTSCombinationTypemanually (tests, fixtures, converters) to ensure they always populateelementsso the declared type stays truthful.
49-51: UpdatedTSTypeunion – ensure all discriminated usages now handleTSLiteralTypeexplicitlyAdding
TSLiteralTypetoTSType(and removing'literal'fromTSScalarType) is a clean separation, but anywhere that switches ontype.nameor assumes'literal'is a scalar should be reviewed to handle the newliteralbranch viaTSLiteralType. In particular, verify any exhaustiveswitch/if‑else chains overTSTypestill compile exhaustively and don’t fall through tonever/default cases unexpectedly.
Thanks @tanujbhaud! Looks good to me, let's now see how CI behaves. Some tests are known to be flaky so you might see us update your branch and re-run CI a couple of times. |
Sidnioulz
changed the title
|
@tanujbhaud by the way, this issue was funded on IssueHunt, so feel free to register your PR on the IssueHunt issue. I've no idea how this stuff actually works but let's try and ensure that reward goes to you if possible :) |
|
@Sidnioulz Thanks for the heads up! I'll check out IssueHunt |
valentinpalkovic
changed the title
|
@valentinpalkovic @Sidnioulz Hey, just wanted to follow up, I submitted the PR on IssueHunt. Does it need an approval on your end there? |
|
@shilman seems you are the project owner on IssueHunt. Are you able to act on the issue there? |
5 participants
What
Partially fixes #12641
Why
When TypeScript properties are optional (
prop?: Type), TypeScript adds| undefinedto the union type. The enum detection logic in the TypeScript converter was failing becauseundefinedis not a literal type, causing the union to not be recognized as an enum.This resulted in controls displaying as "object" type instead of "select" dropdown for optional properties with union types, regardless of the number of items in the union.
How
The fix filters out
undefinedelements before checking if all remaining elements are literals. This allows optional unions to be correctly recognized as enums and display as select controls.Changes
code/core/src/docs-tools/argTypes/convert/typescript/convert.tsundefinedfrom union elements when checking for enum typeExample
Before this fix:
After this fix:
How to test manually
interface Props {
variant?: 'primary' | 'secondary' | 'tertiary';
size: 'small' | 'medium' | 'large';
manyOptions?: 'k1' | 'k2' | 'k3' | 'k4' | 'k5';
}
- variant (optional) as radio buttons — not "object"
- size (required) as radio buttons
- manyOptions (optional) as a select dropdown — not "object"
Checklist
Summary by CodeRabbit
Bug Fixes
Changes
✏️ Tip: You can customize this high-level summary in your review settings.
IssueHunt Summary
Referenced issues
This pull request has been submitted to: