React: Fix subcomponent prop extraction without JSX usage#35107
Conversation
RCM Path 2 now only applies meta.component props when the ref matches the meta component. Subcomponents resolve through Path 3 so args tables show the correct props for each subcomponent entry. Co-authored-by: Cursor <cursoragent@cursor.com>
Bring back the Step 1 import-binding comments and resolvePropsFromStoryFile JSDoc that were dropped when extracting findImportSymbolInStoryFile. Keep the new Path 2/3 comments alongside the original Path 2 note. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds story-file import resolution and a module-export props-resolution fallback so props can be extracted for declared subcomponents referenced only in CSF meta.subcomponents; tightens meta.component validation and adds CSF-parsing tests covering default, namespace, and named-import scenarios. ChangesDeclared Subcomponents Props Extraction
sequenceDiagram
participant TestRunner
participant StoryFile
participant ComponentMetaExtractor
participant ComponentModule
participant ComponentMetaProject
TestRunner->>StoryFile: read story + loadCsf
StoryFile->>ComponentMetaExtractor: extractDeclaredSubcomponents / findImportSymbolInStoryFile
ComponentMetaExtractor->>ComponentModule: resolvePropsFromComponentExport (if no JSX)
ComponentModule-->>ComponentMetaExtractor: ResolvedComponentTarget (props)
ComponentMetaExtractor->>ComponentMetaProject: return resolved targets
ComponentMetaProject->>TestRunner: extractPropsFromStories results/assertions
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts (1)
1-1: ⚡ Quick winUse
memfs(or the in-memory fixture content) instead ofnode:fshere.This regression test now depends on a real filesystem read just to hand the story source to
loadCsf(). Incode/**/*.test.ts, fs-touching unit tests are supposed to stay onmemfs, so it would be better to source this content from the virtual fixture setup rather thanreadFileSync(). As per coding guidelines, "For unit tests that touchnode:fs/node:fs/promises, usememfsinstead of real temp directories or wholesalenode:fsmocks".Also applies to: 418-419
🤖 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/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts` at line 1, The test imports the real filesystem (import * as fs from 'node:fs') and calls readFileSync to feed story source into loadCsf(); replace that real fs usage with the memfs-backed fixture content instead: remove the node:fs import, use the in-memory fixture or memfs API to read the virtual story file, and pass that string into loadCsf() (the same place where readFileSync is currently used) so the test uses memfs rather than touching the real filesystem.Source: Coding guidelines
🤖 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/renderers/react/src/componentManifest/componentMeta/componentMetaExtractor.ts`:
- Around line 540-551: The current namespace-import branch uses the local
namespace alias (componentRef.namespace) to look up a symbol in exports which
fails for `import * as UI` because the alias isn't an export; instead, iterate
the module exports to find an exported symbol whose type has a property named
componentRef.member (use
checker.getTypeOfSymbol(exportedSymbol).getProperty(componentRef.member)), then
set memberSymbol/componentType/exportSymbol based on that found export's
property (instead of using exports.find by namespace name); keep existing null
checks and return undefined if no such export+member is found.
- Around line 337-345: The current symbol-equality branch (using
resolveAliasedSymbol with refSymbol and metaSymbol) incorrectly matches member
refs like Button.Aligner because findImportSymbolInStoryFile returns the base
Button symbol; change the logic in componentMetaExtractor so you only take the
plain symbol-equality path when componentRef.member is unset
(componentRef.member is falsy). If componentRef.member exists, instead compare
the full member expression against metaComponentInitializer (e.g., check
metaComponentInitializer is a PropertyAccessExpression or Identifier and compare
the member/identifier names) so Button.Aligner does not match plain Button; keep
references to resolveAliasedSymbol, refSymbol, metaSymbol, componentRef.member,
and metaComponentInitializer to locate the code.
---
Nitpick comments:
In
`@code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts`:
- Line 1: The test imports the real filesystem (import * as fs from 'node:fs')
and calls readFileSync to feed story source into loadCsf(); replace that real fs
usage with the memfs-backed fixture content instead: remove the node:fs import,
use the in-memory fixture or memfs API to read the virtual story file, and pass
that string into loadCsf() (the same place where readFileSync is currently used)
so the test uses memfs rather than touching the real filesystem.
🪄 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: 70d8a346-b7fc-40c9-8340-a382ed585fd9
📒 Files selected for processing (3)
code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.tscode/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.tscode/renderers/react/src/componentManifest/componentMeta/componentMetaExtractor.ts
Add regression tests for declared compound subcomponents (Button.Aligner) and namespace-import subcomponents without JSX. metaComponentMatchesRef no longer treats member refs as matching a plain meta.component identifier, and Path 3 resolves namespace imports by module export name rather than the local namespace alias. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts (1)
1-1: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse
memfsinstead ofnode:fsfor file operations in unit tests.The new test cases import and use
node:fsdirectly (line 1) withfs.readFileSync(lines 426, 485). As per coding guidelines, unit tests in thecode/**/*.test.ts{,x}pattern should usememfsinstead of real temp directories or directnode:fsusage. This improves test speed and avoids real file I/O.While this follows the existing pattern in the file (line 559), new test cases should align with the guideline. Consider refactoring
withProjectand these tests to usememfs.Also applies to: 426-426, 485-485
🤖 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/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts` at line 1, The tests import node:fs and call fs.readFileSync in new cases; replace usage with memfs so unit tests avoid real disk I/O: remove the node:fs import and instead mount an in-memory filesystem from memfs (e.g., create an in-memory vol) and update the test helpers (notably withProject) to accept or create that memfs volume; update the two failing tests that call fs.readFileSync (and any other fs ops) to use the memfs-provided fs instance so reads/writes use the in-memory volume and follow the existing pattern used elsewhere in this file.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.ts`:
- Line 1: The tests import node:fs and call fs.readFileSync in new cases;
replace usage with memfs so unit tests avoid real disk I/O: remove the node:fs
import and instead mount an in-memory filesystem from memfs (e.g., create an
in-memory vol) and update the test helpers (notably withProject) to accept or
create that memfs volume; update the two failing tests that call fs.readFileSync
(and any other fs ops) to use the memfs-provided fs instance so reads/writes use
the in-memory volume and follow the existing pattern used elsewhere in this
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b93aed4c-d774-47ae-afe4-1ae1bf6ce600
📒 Files selected for processing (2)
code/renderers/react/src/componentManifest/componentMeta/ComponentMetaProject.test.tscode/renderers/react/src/componentManifest/componentMeta/componentMetaExtractor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/componentMeta/componentMetaExtractor.ts
Mirror withProject files into memfs and read story sources from the in-memory volume instead of node:fs in ComponentMetaProject tests. Adds loadDeclaredSubcomponentComponents helper shared by subcomponent cases. Co-authored-by: Cursor <cursoragent@cursor.com>
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 18 | 18 | 0 |
| Self size | 1.28 MB | 1.26 MB | 🎉 -21 KB 🎉 |
| Dependency size | 9.27 MB | 9.27 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 20.74 MB | 20.72 MB | 🎉 -21 KB 🎉 |
| Dependency size | 36.11 MB | 36.11 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 203 | 0 |
| Self size | 908 KB | 908 KB | 🚨 +69 B 🚨 |
| Dependency size | 88.99 MB | 88.96 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -72 B 🎉 |
| Dependency size | 87.47 MB | 87.45 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 56.85 MB | 56.83 MB | 🎉 -21 KB 🎉 |
| Bundle Size Analyzer | node | node |
Closes #
What I did
Fixes React Component Meta (RCM) resolving the wrong props for subcomponents when they are not used in JSX. Path 2 (
resolveFromMetaComponent) now only applies when the ref matchesmeta.component; subcomponents resolve through Path 3 (resolvePropsFromComponentExport).Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
No manual testing required beyond the new
ComponentMetaProjectregression test for subcomponents without JSX.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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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>Made with Cursor
Summary by CodeRabbit
New Features
Improvements
Tests
Chores