Angular: Fix literal union types not inferring 'select' control (#12641)#34887
Open
WillyRelwitten wants to merge 2 commits into
Open
Angular: Fix literal union types not inferring 'select' control (#12641)#34887WillyRelwitten wants to merge 2 commits into
WillyRelwitten wants to merge 2 commits into
Conversation
…"select" for unions/keyof (storybookjs#12641) - compodoc extractEnumValues now handles single-quoted ('S' | 'M' | 'L'), double-quoted, spaced, and mixed literal unions emitted by compodoc for Angular inputs/signals. - Fixes regression where such props fell back to "object" control instead of proper select. - Also hardens enum lookup and adds null-safety. Refs: storybookjs#12641, storybookjs#33779 Opire bounty: 10
Contributor
Contributor
📝 WalkthroughWalkthroughThis PR improves the robustness of enum value extraction from Compodoc type definitions. The ChangesEnum Value Extraction Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/frameworks/angular/src/client/compodoc.ts`:
- Around line 137-151: The mapping currently strips surrounding quotes then
JSON.parse's the result, causing quoted tokens like "true" or "1" to be coerced
to boolean/number/null; change the logic in the .map((segment: string) => { ...
}) block so that if the original segment was quoted (detect before stripping
into s), you return the raw string value (with quotes removed) instead of
attempting JSON.parse, and only attempt JSON.parse for unquoted tokens (using
jsonCandidate) as a fallback; update the code paths around the variables s and
jsonCandidate and the try/catch accordingly so quoted string literals remain
strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f08c99ec-8590-4861-b2c7-c7c3e83469b3
📒 Files selected for processing (1)
code/frameworks/angular/src/client/compodoc.ts
Comment on lines
+137
to
+151
| .map((segment: string) => { | ||
| let s = segment; | ||
| // Strip matching surrounding single or double quotes (compodoc may emit 'foo' | 'bar' or "foo" | "bar") | ||
| if ( | ||
| (s.startsWith("'") && s.endsWith("'")) || | ||
| (s.startsWith('"') && s.endsWith('"')) | ||
| ) { | ||
| s = s.slice(1, -1); | ||
| } | ||
| // Try to parse as JSON (after normalizing quotes for safety); fallback to the raw literal content | ||
| try { | ||
| // Normalize single quotes to double for valid JSON, handle simple cases | ||
| const jsonCandidate = s.replace(/^'|'$/g, '"').replace(/'/g, '"'); | ||
| return JSON.parse(jsonCandidate); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
Quoted string literals are being coerced to booleans/numbers/null
On Line 139-Line 151, stripping quotes before parsing makes "true" | "1" | "null" become true | 1 | null instead of string values. That changes control option values and can break string-typed inputs.
Proposed fix
- .map((segment: string) => {
- let s = segment;
- // Strip matching surrounding single or double quotes (compodoc may emit 'foo' | 'bar' or "foo" | "bar")
- if (
- (s.startsWith("'") && s.endsWith("'")) ||
- (s.startsWith('"') && s.endsWith('"'))
- ) {
- s = s.slice(1, -1);
- }
- // Try to parse as JSON (after normalizing quotes for safety); fallback to the raw literal content
- try {
- // Normalize single quotes to double for valid JSON, handle simple cases
- const jsonCandidate = s.replace(/^'|'$/g, '"').replace(/'/g, '"');
- return JSON.parse(jsonCandidate);
- } catch {
- // Return as string literal (covers most identifier cases)
- return s;
- }
- });
+ .map((segment: string) => {
+ const s = segment.trim();
+ const isSingleQuoted = s.startsWith("'") && s.endsWith("'");
+ const isDoubleQuoted = s.startsWith('"') && s.endsWith('"');
+
+ // Keep quoted literals as strings
+ if (isSingleQuoted || isDoubleQuoted) {
+ return s.slice(1, -1);
+ }
+
+ // Parse only unquoted JSON primitives (e.g. 1, true, null)
+ try {
+ return JSON.parse(s);
+ } catch {
+ return s;
+ }
+ });📝 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.
Suggested change
| .map((segment: string) => { | |
| let s = segment; | |
| // Strip matching surrounding single or double quotes (compodoc may emit 'foo' | 'bar' or "foo" | "bar") | |
| if ( | |
| (s.startsWith("'") && s.endsWith("'")) || | |
| (s.startsWith('"') && s.endsWith('"')) | |
| ) { | |
| s = s.slice(1, -1); | |
| } | |
| // Try to parse as JSON (after normalizing quotes for safety); fallback to the raw literal content | |
| try { | |
| // Normalize single quotes to double for valid JSON, handle simple cases | |
| const jsonCandidate = s.replace(/^'|'$/g, '"').replace(/'/g, '"'); | |
| return JSON.parse(jsonCandidate); | |
| } catch { | |
| .map((segment: string) => { | |
| const s = segment.trim(); | |
| const isSingleQuoted = s.startsWith("'") && s.endsWith("'"); | |
| const isDoubleQuoted = s.startsWith('"') && s.endsWith('"'); | |
| // Keep quoted literals as strings | |
| if (isSingleQuoted || isDoubleQuoted) { | |
| return s.slice(1, -1); | |
| } | |
| // Parse only unquoted JSON primitives (e.g. 1, true, null) | |
| try { | |
| return JSON.parse(s); | |
| } catch { | |
| return s; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/frameworks/angular/src/client/compodoc.ts` around lines 137 - 151, The
mapping currently strips surrounding quotes then JSON.parse's the result,
causing quoted tokens like "true" or "1" to be coerced to boolean/number/null;
change the logic in the .map((segment: string) => { ... }) block so that if the
original segment was quoted (detect before stripping into s), you return the raw
string value (with quotes removed) instead of attempting JSON.parse, and only
attempt JSON.parse for unquoted tokens (using jsonCandidate) as a fallback;
update the code paths around the variables s and jsonCandidate and the try/catch
accordingly so quoted string literals remain strings.
WillyRelwitten
changed the title
Contributor
|
Hi @WillyRelwitten, Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the fix works, optimally with before-and-after comparisons. Thank you for your understanding! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports / completes the fix for Storybook controls of type "select" not being inferred for TypeScript literal union types (including optional
keyof typeoflarge objects and Angularinput<...>()/@Input()unions).The original bug was reported in #12641 (2020). React path was improved in #33200; this change hardens the still-used Angular/compodoc text-parsing path so
extractEnumValuescorrectly recognizes single-quoted and double-quoted literal unions and returns a properSBEnumType, allowinginferControlsto emit a realselect(orradio) control with options.This resolves the long-standing "Problem with storybook-controls of type 'select'" and the v10 regression reported in #33779.
Related Issues
Closes #12641
Closes #33779
Bounty: $110 on Opire.dev
Reference PRs: #33200 (React hardening), #33289 (reset UX)
Changes
code/frameworks/angular/src/client/compodoc.ts: RobustextractEnumValuesparser (trim, quote-stripping for'and", safe JSON.parse + fallback).Manual testing
input<...>()signals and@Input()decorators using literal union types ('S' | 'M' | 'L')inferControlsnow correctly returnsselectcontrol with proper options instead of falling back toobject