React: Fix undefined type of meta.input.play in CSF factories #33346
Conversation
kasperpeulen
changed the title
|
View your CI Pipeline Execution ↗ for commit 56aa06d
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 56aa06d
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull request overview
This PR fixes a TypeScript typing issue where meta.input.play was incorrectly typed as undefined in CSF (Component Story Format) factories for the React renderer. The fix ensures that all properties from ComponentAnnotations (including the play function) are properly available on meta.input.
Key Changes
- Updated the type definition for
ReactPreview.meta()return type to include allComponentAnnotationsproperties in theMetaInputtype parameter - Added a test case demonstrating that
meta.input.playcan now be safely accessed and called
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| code/renderers/react/src/preview.tsx | Changed the second type parameter of ReactMeta from a simple { args: ... } object to Omit<ComponentAnnotations<...>, 'args'> & { args: ... }, ensuring all ComponentAnnotations properties (including play) are properly typed |
| code/renderers/react/src/csf-factories.test.tsx | Added test case verifying that meta.input.play is accessible and can be invoked from within a story's play function, demonstrating the common pattern of composing play functions |
| it('meta.input also contains play', () => { | ||
| const meta = preview.meta({ | ||
| /** Title, component, etc... */ | ||
| play: async ({ canvas }) => { | ||
| /** Do some common interactions */ | ||
| }, | ||
| }); | ||
|
|
||
| const ExtendedInteractionsStory = meta.story({ | ||
| play: async ({ canvas, ...rest }) => { | ||
| await meta.input.play?.({ canvas, ...rest }); | ||
|
|
||
| /** Do some extra interactions */ | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test verifies that meta.input.play is properly typed but doesn't actually verify that the play function is callable or has the correct behavior at runtime. Consider adding a runtime assertion to verify that meta.input.play is actually defined and callable. For example, you could add expect(meta.input.play).toBeDefined(); or use a spy to verify the play function is called when invoked.
| const ExtendedInteractionsStory = meta.story({ | ||
| play: async ({ canvas, ...rest }) => { | ||
| await meta.input.play?.({ canvas, ...rest }); | ||
|
|
||
| /** Do some extra interactions */ | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Unused variable ExtendedInteractionsStory.
| const ExtendedInteractionsStory = meta.story({ | |
| play: async ({ canvas, ...rest }) => { | |
| await meta.input.play?.({ canvas, ...rest }); | |
| /** Do some extra interactions */ | |
| }, | |
| }); |
📝 WalkthroughWalkthroughThe PR adds a test verifying that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related issues
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: 0
🧹 Nitpick comments (1)
code/renderers/react/src/csf-factories.test.tsx (1)
387-403: Strengthen the test to assertmeta.input.playis callable (and optionally non-optional whenmeta.playis provided).
Right now,meta.input.play?.(...)proves the property exists on the type, but doesn’t strongly validate call signature (or presence whenmeta.playis set). Consider adding anexpectTypeOf(meta.input.play)assertion and/or usingmeta.input.play!in this case.Example:
it('meta.input also contains play', () => { const meta = preview.meta({ /** Title, component, etc... */ play: async ({ canvas }) => { /** Do some common interactions */ }, }); const ExtendedInteractionsStory = meta.story({ play: async ({ canvas, ...rest }) => { - await meta.input.play?.({ canvas, ...rest }); + expectTypeOf(meta.input.play).toMatchTypeOf<Function>(); + await meta.input.play!({ canvas, ...rest }); /** Do some extra interactions */ }, }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/renderers/react/src/csf-factories.test.tsx(1 hunks)code/renderers/react/src/preview.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsx
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/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsx
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/renderers/react/src/preview.tsxcode/renderers/react/src/csf-factories.test.tsx
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/renderers/react/src/csf-factories.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/renderers/react/src/csf-factories.test.tsx
code/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested
Mock external dependencies using vi.mock() for file system, loggers, and other external dependencies in tests
Aim for high test coverage of business logic (75%+ for statements/lines) using coverage reports
Files:
code/renderers/react/src/csf-factories.test.tsx
🧠 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.
🧬 Code graph analysis (1)
code/renderers/react/src/csf-factories.test.tsx (1)
code/core/src/csf-tools/CsfFile.ts (1)
meta(964-966)
⏰ 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). (6)
- GitHub Check: normal
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: nx
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/renderers/react/src/preview.tsx (1)
79-91: PassTArgsexplicitly as the second generic toComponentAnnotationsin the return type.The input parameter correctly uses
ComponentAnnotations<ReactTypes & T, TArgs>(line 69) with both generics, but the return type (lines 80–85) only passes one generic:ComponentAnnotations<ReactTypes & T & { args: Simplify<...> }>. This causes the second genericTArgsto default toArgs, breaking type inference and creating a mismatch between input and output parameter types. TheOmit<..., 'args'> & { args: ... }pattern correctly preserves fields likeplay, but it must also passTArgsexplicitly to maintain proper type constraints.Apply the suggested fix:
Omit< ComponentAnnotations< ReactTypes & T & { args: Simplify< TArgs & Simplify<RemoveIndexSignature<DecoratorsArgs<ReactTypes & T, Decorators>>> >; } + , + TArgs >, 'args' > & { args: Partial<TArgs> extends TMetaArgs ? {} : TMetaArgs; }
Closes #
What I did
Resolved the issue where
meta.input.playin CSF factories was undefined by fixing its type.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
meta.inputnow includes aplayhook, enabling developers to access shared play functions from story-level play hooks with merged arguments.Tests
meta.input.playfunctionality and composition of interactions.✏️ Tip: You can customize this high-level summary in your review settings.