Controls: Allow primitive values of ReactNode argType #31931
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
alexey-kozlenkov
changed the title
|
View your CI Pipeline Execution ↗ for commit f863ac3
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@valentinpalkovic @ghengeveld 👋 hey! Not sure what are appropriate channels to do so, yet could you help me getting a review for this? |
|
@alexey-kozlenkov Thanks for doing this. I've assigned @ndelangen for the review and merge. I've taken a quick look and see no immediate issues. Perhaps a code comment would be helpful to explain why only primitive values are supported for ReactNode props, just to discourage others from wanting to extend it to cover other (unsafe) types in the future. |
|
@ghengeveld thank you! UPD: also added non-primitive test cases Let me know please if any other changes needed. |
|
@ndelangen hey! |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds support in arg mapping for ArgTypes with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant URLArgs as URL / Controls
participant Mapper as mapArgsToTypes
participant Renderer
User->>URLArgs: supply arg value (e.g., label=...)
URLArgs->>Mapper: mapArgsToTypes(arg, argType)
alt argType.name == "other" and argType.value == "ReactNode"
alt arg is primitive (string|number|boolean)
Mapper-->>Renderer: mapped primitive value
else non-primitive
Mapper-->>Renderer: INCOMPATIBLE (no mapping)
end
else other argType cases
Mapper-->>Renderer: existing mapping logic
end
Renderer-->>User: render with mapped args
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
🧹 Nitpick comments (2)
code/core/src/preview-api/modules/store/args.ts (1)
61-63: Nit: tighten the comment and reference the correct identifierUse “type.value” (not “argType.value”) and state the intent crisply. Also note we intentionally exclude symbol/bigint/objects.
Apply this diff (if you prefer to keep the variable outside the block above, adjust line numbers accordingly):
- // Only proceed if `argType.value` appeared to be a `ReactNode`. - // Only permit primitives as they are included in `ReactNode` type, hence easily applicable. + // Only handle when type.value is 'ReactNode'. + // Allow primitives only (string | number | boolean); exclude symbol/bigint/objects for safety.code/core/src/preview-api/modules/store/args.test.ts (1)
127-136: Add null/undefined coverage for ReactNode primitivesNull and undefined are accepted early (lines 18–20 in args.ts) and are valid ReactNode values; add assertions to lock this in.
it('passes only primitives for ReactNode type', () => { expect(mapArgsToTypes({ a: 'foo bar' }, { a: { type: reactNodeType } })).toStrictEqual({ a: 'foo bar', }); expect(mapArgsToTypes({ a: 1.2 }, { a: { type: reactNodeType } })).toStrictEqual({ a: 1.2 }); expect(mapArgsToTypes({ a: true }, { a: { type: reactNodeType } })).toStrictEqual({ a: true }); + expect(mapArgsToTypes({ a: null }, { a: { type: reactNodeType } })).toStrictEqual({ a: null }); + expect( + mapArgsToTypes({ a: undefined }, { a: { type: reactNodeType } }) + ).toStrictEqual({ a: undefined }); expect(mapArgsToTypes({ a: new Date() }, { a: { type: reactNodeType } })).toStrictEqual({}); expect(mapArgsToTypes({ a: { foo: 'bar' } }, { a: { type: reactNodeType } })).toStrictEqual({}); expect(mapArgsToTypes({ a: Symbol('foo') }, { a: { type: reactNodeType } })).toStrictEqual({}); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/preview-api/modules/store/args.test.ts(2 hunks)code/core/src/preview-api/modules/store/args.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/core/src/preview-api/modules/store/args.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/core/src/preview-api/modules/store/args.test.ts
🧬 Code graph analysis (2)
code/core/src/preview-api/modules/store/args.ts (1)
code/core/src/csf/story.ts (2)
StoryContextUpdate(223-229)StrictArgs(156-158)
code/core/src/preview-api/modules/store/args.test.ts (1)
code/core/src/preview-api/modules/store/args.ts (1)
mapArgsToTypes(73-81)
🪛 Biome (2.1.2)
code/core/src/preview-api/modules/store/args.ts
[error] 58-59: 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/preview-api/modules/store/args.test.ts (1)
21-21: LGTM: clear SBType for ReactNodeThe test constant is precise and readable.
ndelangen
changed the title
4 participants
Closes #31735
What I did
Modified mapping in
mapArgsToTypesutility to allow primitive values (string, number, boolean) to pass through forReactNodecontrol.I chose simple yet limited approach to validate for primitive type and return it as is. Another option could be to simply convert to
stringwhateverargholds, let me know if this suits better.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Unfortunately, I didn't figure out how to modify smth in sandbox (e.g.
code/frameworks/react-vite/template/cli/ts/Button.tsx) and see the change when I run storybook, so I rely on units.Documentation
MIGRATION.MD
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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Modified
mapArgsToTypesutility in Storybook core to handle primitive values for ReactNode control types, fixing URL parameter handling in CSF3 iframe mode.code/core/src/preview-api/modules/store/args.tsto allow primitive values (string, number, boolean) to pass through for ReactNode control typecode/core/src/preview-api/modules/store/args.test.tsfor primitive value handlingSummary by CodeRabbit