React: Fix manifest RDT for referenced tsconfig projects#34415
Open
kasperpeulen wants to merge 8 commits into
Open
React: Fix manifest RDT for referenced tsconfig projects#34415kasperpeulen wants to merge 8 commits into
kasperpeulen wants to merge 8 commits into
Conversation
|
View your CI Pipeline Execution ↗ for commit 9e796a4
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 019bf38 ☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.51 MB | 20.48 MB | 🎉 -27 KB 🎉 |
| Dependency size | 16.55 MB | 16.55 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 782 KB | 0 B |
| Dependency size | 67.73 MB | 67.71 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 66.26 MB | 66.23 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🎉 -192 B 🎉 |
| Dependency size | 37.06 MB | 37.03 MB | 🎉 -27 KB 🎉 |
| Bundle Size Analyzer | node | node |
Contributor
📝 WalkthroughWalkthroughThis PR introduces per-file TypeScript configuration (tsconfig) discovery and applies it throughout the React docgen and component manifest generation pipeline. A new Changes
Sequence DiagramsequenceDiagram
participant Plugin as Vite/Webpack Plugin
participant Discover as Tsconfig Discovery
participant Docgen as React Docgen
participant TypeScript as TypeScript Compiler
participant FS as File System
Plugin->>Plugin: Process module/file
Plugin->>Discover: findTsconfigPathForFile(dirname(filePath), filePath)
Discover->>FS: Read root tsconfig.json
FS-->>Discover: Config with references
Discover->>FS: Read referenced tsconfig.app.json
FS-->>Discover: App config with include patterns
Discover->>Discover: Match file against include/exclude
Discover-->>Plugin: Selected tsconfig path
Plugin->>Docgen: Parse with matchPath/getTsConfig
Docgen->>TypeScript: Load config via selected tsconfig
TypeScript->>TypeScript: Build compiler options (paths, baseUrl)
TypeScript-->>Docgen: Compiler instance
Docgen->>Docgen: Resolve aliases, parse JSDoc
Docgen-->>Plugin: Component metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
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: 4
🧹 Nitpick comments (2)
code/renderers/react/src/componentManifest/reactDocgen.test.ts (1)
252-252: Keep theprocess.cwd()spy in shared setup instead of the test body.This suite already has shared lifecycle hooks, so moving this stub into a helper or
beforeEachkeeps the mock pattern consistent and avoids inline test-local behavior.As per coding guidelines, 'Implement mock behaviors in
beforeEachblocks in Vitest tests' and 'Avoid inline mock implementations within test cases in Vitest tests'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/reactDocgen.test.ts` at line 252, The test is stubbing process.cwd inline via vi.spyOn(process, 'cwd').mockReturnValue(dir);—move this mock into the suite-level setup (e.g., the existing beforeEach/shared helper) so all tests use the same mock pattern: create the vi.spyOn(process, 'cwd') and .mockReturnValue(dir) in beforeEach and restore it in afterEach (or use vi.restoreAllMocks), updating reactDocgen.test.ts to remove the inline spy from the individual test body and reference the shared mocked cwd instead.code/renderers/react/src/componentManifest/utils.test.ts (1)
193-228: Move thecommon.findTsconfigPathstub into shared setup.These cases keep redefining the spy inline. A small
beforeEach/helper default would keep the suite aligned with the repo’s Vitest mocking rules and make mock state easier to reset between cases.As per coding guidelines, 'Implement mock behaviors in
beforeEachblocks in Vitest tests' and 'Avoid inline mock implementations within test cases in Vitest tests'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/utils.test.ts` around lines 193 - 228, The tests repeatedly stub common.findTsconfigPath inline; move that stub into a shared beforeEach so mock behavior is centralized and reset between cases: in the test suite setup add a beforeEach that calls vi.mocked(common.findTsconfigPath).mockReturnValue(undefined) (or a sensible default) and ensure test-specific cases override it via mockReturnValue when needed; keep findTsconfigPath and logger.warn assertions the same and rely on Vitest reset/restore semantics to isolate tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/common/utils/tsconfig.ts`:
- Around line 26-34: findTsconfigPath currently searches by filename across
ancestor chain (using TSCONFIG_CANDIDATES) which lets a top-level tsconfig.json
win over a nearer tsconfig.app.json; change the algorithm in findTsconfigPath to
search by directory depth first: starting at the provided cwd and walking up to
getProjectRoot(), for each directory check for any of the TSCONFIG_CANDIDATES
files (e.g., with fs.existsSync(path.join(dir, candidate)) or a find call
limited to that directory) and return the first match found; if no candidate is
found in any ancestor directory, fall back to the original filename-oriented
search or return undefined—modify findTsconfigPath and rely on getProjectRoot
and TSCONFIG_CANDIDATES to locate the correct config.
- Around line 179-189: normalizeTsconfigPattern currently keeps a leading "./"
from normalizePath which makes returned glob patterns like "./src/**/*" fail to
match relative paths; update normalizeTsconfigPattern to remove any leading "./"
(e.g., replace a leading "./" or multiple "./" with an empty string) immediately
after calling normalizePath so subsequent checks and returned patterns (in
function normalizeTsconfigPattern using normalizePath and picomatch.scan)
produce patterns like "src/**/*" and correctly match relative() paths.
In `@code/renderers/react/src/componentManifest/reactDocgen.ts`:
- Around line 61-63: The importer callback currently calls matchPath(filename,
basedir) passing a directory as the second argument which causes
getTsConfig(importerFilePath) to treat a directory as a file and lose file
context; update the importer callback to pass the actual file path (e.g.,
filename or the full file path) as the importerFilePath so matchPath and
getTsConfig receive a file-aware path and can correctly resolve the owning
tsconfig (fix the call site where the react-docgen importer invokes matchPath).
In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 193-195: The code uses findTsconfigPathForFile/findTsconfigPath
(e.g., the configPath and configKey logic in reactDocgenTypescript.ts) which
seeds lookup from process.cwd() and can pick the wrong tsconfig; replace that
local tsconfig-selection with the shared selector from storybook/internal/common
(the same ownership-ranking selector used elsewhere) so react-docgen-typescript
uses the same tsconfig resolution as Vite/webpack/manifest; remove or stop using
local helpers findTsconfigPathForFile/findTsconfigPathIncludingFile and update
all affected blocks (including the similar logic around lines 348–396) to call
the shared selector and derive configKey from its result.
---
Nitpick comments:
In `@code/renderers/react/src/componentManifest/reactDocgen.test.ts`:
- Line 252: The test is stubbing process.cwd inline via vi.spyOn(process,
'cwd').mockReturnValue(dir);—move this mock into the suite-level setup (e.g.,
the existing beforeEach/shared helper) so all tests use the same mock pattern:
create the vi.spyOn(process, 'cwd') and .mockReturnValue(dir) in beforeEach and
restore it in afterEach (or use vi.restoreAllMocks), updating
reactDocgen.test.ts to remove the inline spy from the individual test body and
reference the shared mocked cwd instead.
In `@code/renderers/react/src/componentManifest/utils.test.ts`:
- Around line 193-228: The tests repeatedly stub common.findTsconfigPath inline;
move that stub into a shared beforeEach so mock behavior is centralized and
reset between cases: in the test suite setup add a beforeEach that calls
vi.mocked(common.findTsconfigPath).mockReturnValue(undefined) (or a sensible
default) and ensure test-specific cases override it via mockReturnValue when
needed; keep findTsconfigPath and logger.warn assertions the same and rely on
Vitest reset/restore semantics to isolate tests.
🪄 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: c82d93da-28ca-4327-bead-8a6964bb8af2
📒 Files selected for processing (15)
code/core/src/common/index.tscode/core/src/common/utils/__tests__/tsconfig.test.tscode/core/src/common/utils/tsconfig.tscode/frameworks/react-vite/src/plugins/react-docgen.tscode/presets/react-webpack/src/loaders/react-docgen-loader.tscode/renderers/react/src/componentManifest/generator.react-docgen-typescript.test.tscode/renderers/react/src/componentManifest/reactDocgen.test.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/renderers/react/src/componentManifest/reactDocgen/extractReactTypescriptDocgenInfo.test.tscode/renderers/react/src/componentManifest/reactDocgen/extractReactTypescriptDocgenInfo.tscode/renderers/react/src/componentManifest/reactDocgen/utils.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.test.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.tscode/renderers/react/src/componentManifest/utils.test.tscode/renderers/react/src/componentManifest/utils.ts
huang-julien
reviewed
Apr 1, 2026
| } | ||
|
|
||
| function createTsconfigMatchPath(filePath: string) { | ||
| const tsconfig = TsconfigPaths.loadConfig(findTsconfigPathForFile(dirname(filePath), filePath)); |
Contributor
There was a problem hiding this comment.
Can we cache this ? I thinkTsconfigPaths.loadConfig and findTsconfigPathForFile would run for every single file ? 🤔
Contributor
There was a problem hiding this comment.
Or maybe have ssome sort of filePaths[] -> tsconfig Map
| import { readFileSync } from 'node:fs'; | ||
| import { basename, dirname, relative, resolve } from 'node:path'; | ||
|
|
||
| import * as find from 'empathic/find'; |
Contributor
There was a problem hiding this comment.
We can directly import { up } as it's treeshakable
| * first config filename we discover. It extends the simpler fallback-chain fix from #34353 so | ||
| * docgen-style callers can handle project references too. | ||
| */ | ||
| export const findTsconfigPathForFile = (cwd: string, filePath: string): string | undefined => { |
Contributor
There was a problem hiding this comment.
I'm a bit confused, there's also a findTsconfigPathForFile at reactDocgenTypescript.ts. Implementations seems different
shilman
added
the
patch:yes
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.
Closes #34414
What I did
Fixed the React component manifest
react-docgen-typescriptpath to follow referenced tsconfig files and choose the config that actually includes the component file. This fixes Vite-style projects whose roottsconfig.jsononly containsfiles: []and references totsconfig.app.json, which previously caused manifest generation to report that no component docs were found.Added dedicated real-filesystem parser coverage for
react-docgen-typescriptplus an end-to-end manifest regression that reproduces the Vitetsconfig.json+tsconfig.app.jsonlayout from the bug report.Extended the same file-aware tsconfig selection strategy to the React docgen alias-resolution paths used by the manifest, the React Vite plugin, the React webpack loader, and the
react-docgen-typescriptargtypes extraction path. This builds on the fallback-chain work from #34353, but goes further by following project references and selecting the config that actually owns the file.Implementation note: the file-aware selection strategy is intentionally aligned with the Volar-inspired project selection already used by
react-component-meta'sComponentMetaManager.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Validated with:
yarn exec vitest run code/core/src/common/utils/__tests__/tsconfig.test.ts code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts code/renderers/react/src/componentManifest/reactDocgen.test.ts code/renderers/react/src/componentManifest/generator.react-docgen-typescript.test.ts code/renderers/react/src/componentManifest/reactDocgen/extractReactTypescriptDocgenInfo.test.tsManual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
../storybook-sandboxes/react-vite-default-tssandbox withyarn storybook. This sandbox already uses the Vite-style referenced-tsconfig layout (tsconfig.jsonwithfiles: []plus references totsconfig.app.jsonandtsconfig.node.json) and hastypescript.reactDocgen = 'react-docgen-typescript'enabled in.storybook/main.ts.http://localhost:6006/manifests/components.htmland confirmed the manifest debugger loaded successfully, reportedUsing react-docgen-typescript, and listedexample-button/Buttonwith5 prop typesinstead of showingreact-docgen-typescript did not return any component docs for this file.http://localhost:6006/?path=/docs/example-button--docsand confirmed the Example/Button docs entry rendered successfully under the same referenced-tsconfig layout.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
Improvements