Repository navigation
React: Fix RDT tsconfig selection for Vite project references - #35743
Conversation
Keep explicit .ts imports, prefer node:path, and rename the local RDT owner-lookup helper so it is not confused with common findTsconfigPathForFile. Co-authored-by: Cursor <cursoragent@cursor.com>
Fixes CircleCI eslint import/extensions failures from the #34415 revive. Co-authored-by: Cursor <cursoragent@cursor.com>
Windows unit tests failed because TypeScript fileNames use forward slashes while Node path.join uses backslashes, so referenced-app ownership never matched and Vite-style roots stayed empty. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@kasperpeulen, can you tell me if this PR is more aligned with your vision? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds shared, file-aware TypeScript configuration discovery with project-reference support. React docgen integrations use the owning configuration for alias resolution and TypeScript parsing. Tests cover matching, parser behavior, manifest generation, aliases, and JSDoc preservation. ChangesTypeScript configuration resolution
Sequence Diagram(s)sequenceDiagram
participant Docgen
participant findTsconfigPathForFile
participant TsconfigParser
participant TypeScript
Docgen->>findTsconfigPathForFile: resolve configuration for component file
findTsconfigPathForFile->>TsconfigParser: follow project references
TsconfigParser-->>findTsconfigPathForFile: return owning tsconfig path
findTsconfigPathForFile-->>Docgen: return file-specific configuration
Docgen->>TypeScript: create or reuse parser for configuration and options
TypeScript-->>Docgen: return component metadata and prop information
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/core/src/common/utils/__tests__/tsconfig.test.ts`:
- Around line 1-3: Update the filesystem tests around createTempProject and the
affected assertions to use memfs instead of host filesystem helpers. Reset vol
in beforeEach, seed each virtual project before assertions, and redirect
filesystem calls with spies rather than mocking the async factory; use
path.resolve when constructing expected Windows-compatible paths.
- Line 42: Move the paths mock setup to module scope by declaring
vi.mock('../paths.ts', { spy: true }), replace direct paths.getProjectRoot spy
usage in the affected tests with vi.mocked(paths.getProjectRoot), and configure
its return value in beforeEach. Apply this consistently to all referenced mock
setup locations while preserving the existing test behavior.
In `@code/core/src/common/utils/tsconfig.ts`:
- Around line 169-175: Update readTsconfigConfig to call stripJsonComments with
trailing comma removal enabled via { trailingCommas: true }, allowing referenced
JSONC configs with trailing commas to parse successfully; add a fixture covering
this syntax and verify selection uses the referenced config.
In `@code/frameworks/react-vite/src/plugins/react-docgen.ts`:
- Around line 119-123: Guard the result of findTsconfigPathForFile before
invoking TsconfigPaths.loadConfig in createTsconfigMatchPath and the
corresponding sites in code/frameworks/react-vite/src/plugins/react-docgen.ts
(119-123), code/presets/react-webpack/src/loaders/react-docgen-loader.ts
(155-159), and code/renderers/react/src/componentManifest/reactDocgen.ts
(76-80); when no path is found, return the existing failed lookup result
directly so loadConfig is never called with undefined.
In `@code/renderers/react/src/componentManifest/reactDocgen.ts`:
- Around line 61-63: Update matchPath and getReactDocgenImporter so getTsConfig
receives the actual importer file path rather than the basedir directory; if
directory-based resolution is still required, keep it on a separate lookup path
and ensure file-aware resolution does not apply dirname() to a directory.
In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts`:
- Line 48: Move the inline mock behavior into beforeEach blocks: configure the
process.cwd spy in
code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts:48-48,
generator.react-docgen-typescript.test.ts:26-26, reactDocgen.test.ts:252-252,
and reactDocgen/extractReactTypescriptDocgenInfo.test.ts:492-492; in
utils.test.ts:252-277, initialize the common.findTsconfigPath behavior through
per-test state in beforeEach. Remove the corresponding mock implementations from
individual test cases while preserving each test’s configured return values.
- Around line 274-284: Replace host-filesystem fixture setup with memfs across
the four identified test sites:
code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts:274-284,
generator.react-docgen-typescript.test.ts:82-184, reactDocgen.test.ts:258-269,
and reactDocgen/extractReactTypescriptDocgenInfo.test.ts:506-517. Reset vol in
each beforeEach, seed fixtures through vol, redirect node:fs and
node:fs/promises calls with spies rather than full async factory mocks, and use
path.resolve for expected paths.
In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.ts`:
- Around line 188-196: Update the parserKey construction near optionsKey so
parser-options values, especially function-valued callbacks such as propFilter,
cannot collide when different ParserOptions objects are supplied. Preserve the
existing configPath component while incorporating callback identity or another
cache-key mechanism that distinguishes these options and prevents reusing the
wrong fileParser.
🪄 Autofix
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: bd40d7de-5e2e-43a5-9501-834accf3aac0
📒 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
Clear retained TS programs on parser invalidation, guard loadConfig when no tsconfig is found, and align test mocking with repo Vitest rules. Co-authored-by: Cursor <cursoragent@cursor.com>
makeFsImporter passes a basedir, and treating that as a file caused an extra dirname that searched from the parent and could pick the wrong tsconfig. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/common/utils/__tests__/tsconfig.test.ts (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure
paths.getProjectRootinbeforeEach.Line 117 sets mock behavior inside the test case. Declare the mocked return behavior in
beforeEach. Use a per-test variable when each test needs a different project root.As per coding guidelines: “Implement mock behaviors in
beforeEachblocks in Vitest tests.”🤖 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/core/src/common/utils/__tests__/tsconfig.test.ts` at line 117, Move the default paths.getProjectRoot mockReturnValue setup from the individual test into the test suite’s beforeEach block. If tests require different project roots, retain a per-test variable and configure the mock from that variable in beforeEach.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.
Nitpick comments:
In `@code/core/src/common/utils/__tests__/tsconfig.test.ts`:
- Line 117: Move the default paths.getProjectRoot mockReturnValue setup from the
individual test into the test suite’s beforeEach block. If tests require
different project roots, retain a per-test variable and configure the mock from
that variable in beforeEach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85540809-168c-420b-a2ae-5fa83d4fa8fc
📒 Files selected for processing (3)
code/core/src/common/utils/__tests__/tsconfig.test.tscode/core/src/common/utils/tsconfig.tscode/renderers/react/src/componentManifest/reactDocgen.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/reactDocgen.ts
Avoid reloading and rebuilding MatchPath on every Vite/webpack transform by caching matcher instances by resolved tsconfig path. Co-authored-by: Cursor <cursoragent@cursor.com>
Use ts.readConfigFile so comments and trailing commas match tsc, instead of a manual strip-json-comments + JSON.parse path. Co-authored-by: Cursor <cursoragent@cursor.com>
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Static typescript in storybook/internal/common OOMd CI Vitest. Keep lightweight JSONC parsing with trailingCommas and document why. Co-authored-by: Cursor <cursoragent@cursor.com>
Inherited include/exclude/files from extended configs were ignored, so selection could miss autogen-style globs or over-own via **/* defaults. Co-authored-by: Cursor <cursoragent@cursor.com>
…aware-tsconfig React: Fix RDT tsconfig selection for Vite project references (cherry picked from commit 94365e1)
Closes #34414
What I did
Revives Kasper’s broader fix from #34415 (supersedes stale #34415 / #34416 and the narrower #35586).
Modern Vite React TypeScript apps often have a root
tsconfig.jsonthat does not list source files. It only points at other configs:{ "files": [], "references": [ { "path": "./tsconfig.app.json" }, { "path": "./tsconfig.node.json" } ] }The real compiler options and
includelive intsconfig.app.json. Storybook’sreact-docgen-typescriptpaths that stopped at the root shell built an empty TypeScript program and returned no component docs.This PR restores file-aware tsconfig selection aligned with the Volar-inspired approach already used by
ComponentMetaManager:findTsconfigPath/findTsconfigPathForFileinstorybook/internal/commonfileNames+ project references) for the component-manifest RDT parser, with per-config parser cachingRelationship to earlier PRs
nextChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Validated locally with:
yarn test \ 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/utils.test.ts \ code/renderers/react/src/componentManifest/generator.react-docgen-typescript.test.ts \ code/renderers/react/src/componentManifest/reactDocgen/extractReactTypescriptDocgenInfo.test.tsResult: 6 files / 42 tests passed.
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
files: []+references).yarn task sandbox --template react-vite/default-ts --start-from autofrom this branch).typescript.reactDocgento'react-docgen-typescript'in.storybook/main.ts./manifests/components.html.react-docgen-typescript did not return any component docs for this file.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