React: Add component metadata extraction via Volar-style LanguageService#33914
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 3e60442
☁️ Nx Cloud last updated this comment at |
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
c41a017 to
7e9c4c7
Compare
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 650 KB | 650 KB | 🎉 -120 B 🎉 |
| Dependency size | 59.95 MB | 60.20 MB | 🚨 +248 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 0 B |
| Dependency size | 22.48 MB | 22.72 MB | 🚨 +248 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 121 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.54 MB | 23.79 MB | 🚨 +248 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 20.26 MB | 20.51 MB | 🚨 +248 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 24 KB | 24 KB | 🎉 -12 B 🎉 |
| Dependency size | 44.56 MB | 44.81 MB | 🚨 +248 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 58 | 0 |
| Self size | 1.19 MB | 1.44 MB | 🚨 +248 KB 🚨 |
| Dependency size | 13.21 MB | 13.21 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
Contributor
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/22440973305 |
kasperpeulen
changed the title
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
b11b677 to
fa6219b
Compare
Add reactComponentMeta — a probe-free extraction engine built on @volar/typescript that uses React's own type system to detect components and extract props from existing story files. Architecture mirrors Vue Component Meta: - ComponentMetaProject: one LanguageService per tsconfig - ComponentMetaManager: multi-project manager with file watching - Path 1: resolvePropsFromStoryFile() via getResolvedSignature() - Path 2: resolvePropsFromComponentType() fallback for args-only stories Tested against Flowbite, Reshaped, Mantine, Primer, and Park UI.
Phase 1 — Error handling hardening: - Wrap per-entry/per-export loops in extractPropsFromStories with try/catch - Replace silent catches in ComponentMetaManager with logger.debug Phase 2 — Correctness fixes: - Fix TypeFlags.Undefined check: use bitwise AND instead of strict equality - Fix JSDoc: jsxDepth 1 = outermost JSX element (not 0) Phase 3 — Generator cleanup: - Remove unnecessary async/Promise.all on synchronous map - Move componentMetaStartTime after await managerWarmup - Rename shadowed `results` to `extractionResults` Phase 4 — Manager robustness: - Normalize Windows paths in rootTsConfigs and onConfigChanged - Watch newly discovered projects when watching is active - Fix typo prepareClosestootCommandLine → prepareClosestRootCommandLine Phase 5 — Nice-to-haves: - extractPropsFromStory takes StoryExtractionEntry object instead of positional params - Add MAX_UNWRAP_DEPTH constant, unify depth limit (was inconsistent 5/10) - Cache getPropSourceFile in getBulkSourceExclusions - Add comment explaining watch option type widening - Fix test timeout for multi-tsconfig test
- ComponentMetaManager: clear searchedDirs on config delete so findMatchTSConfig re-scans directories for new tsconfigs - ComponentMetaProject: bump projectVersion on created events, move shouldCheckRootFiles outside deleted-only branch - componentMetaExtractor: add depth guard to isReactNodeLike recursion, fix union forceOptional to check after collecting all members instead of per-member during iteration
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
7136626 to
f85b02d
Compare
…test - getComponentImports: restore react-docgen / react-docgen-typescript conditional execution (was accidentally changed to always-run) - getComponentImports: move docgenTimings above getComponents JSDoc so the doc comment is attached to the function it documents - ComponentMetaManager: add explanatory comment on prepareClosestRoot - ComponentMetaManager: deduplicate narrower watchers when broader dir is added, preventing duplicate events and wasted file descriptors - ComponentMetaProject.test: replace flaky Date.now() timing assertion with toStrictEqual for cache hit verification
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
f85b02d to
866c9f2
Compare
The version was alpha.5 (from the branch) while versions.ts had alpha.12 (from rebase on next). This caused sandbox creation failures because the local registry published alpha.5 but sandboxes tried to install alpha.12.
- Remove extractDocs() and tryAddFile() in favor of extractPropsFromStories() + ensureFiles() - Replace broad getSemanticDiagnostics() warmup with targeted entry replay - Refactor watchers: single Map<dir, FSWatcher> that properly closes subsumed watchers - Eager manager initialization at module load (TS imports in parallel with startup) - Run all 3 docgen engines unconditionally for QA comparison - Add lowercase export guard and isReactComponentType check to prevent false positives - Pass watch flag through manifests preset options
Contributor
|
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:
📝 WalkthroughWalkthroughIntroduces TypeScript-aware component metadata extraction with Volar-inspired multi-project support. Adds ComponentMetaManager for managing multiple TypeScript projects, ComponentMetaProject for per-project extraction, and componentMetaExtractor for prop documentation serialization. Integrates extraction into manifest generation with timing instrumentation. Core manifests function now supports optional watch flag. Test fixtures use underscore-prefixed parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant ManifestAPI as Manifest Endpoint
participant Generator as Generator
participant Manager as ComponentMetaManager
participant Project as ComponentMetaProject
participant Extractor as componentMetaExtractor
participant TS as TypeScript LS
ManifestAPI->>Generator: manifests(presets, {watch: true})
Generator->>Manager: Warmup call / get projects
Manager->>Manager: findMatchTSConfig(file)
Manager->>Project: getOrCreateConfiguredProject(tsconfig)
Project->>TS: createLanguageService()
Generator->>Manager: getProjectForFile(componentPath)
Manager->>Project: return project
Generator->>Project: extractPropsFromStories(entries[])
Project->>Extractor: resolvePropsFromStoryFile(story)
Extractor->>TS: getResolvedSignature(JSX)
TS-->>Extractor: componentType
Extractor-->>Project: ComponentDoc[]
Project-->>Generator: Map<file, Map<export, ComponentDoc[]>>
Generator->>Generator: Assign reactComponentMeta to components
Generator-->>ManifestAPI: ReactComponentManifest[] with timings
sequenceDiagram
participant FileWatcher as File Watcher
participant Manager as ComponentMetaManager
participant Project1 as ConfiguredProject1
participant Project2 as ConfiguredProject2
participant Debounce as Debounce Handler
FileWatcher->>Manager: onFilesChanged(changes[])
Manager->>Manager: Deduplicate/classify events
Manager->>Project1: onFilesChanged(filtered)
Manager->>Project2: onFilesChanged(filtered)
Project1->>Project1: Increment projectVersion
Project2->>Project2: Increment projectVersion
FileWatcher->>Manager: onConfigChanged(tsconfig, 'created')
Manager->>Manager: Update rootTsConfigs
Manager->>Project1: (new project or reuse)
Manager->>Debounce: Schedule warmup extraction
Debounce-->>Manager: handleFileEvent debounced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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: 5
🧹 Nitpick comments (2)
code/renderers/react/src/componentManifest/generator.ts (1)
193-194: Consider adding thewatchflag to the type signature.The
watchflag is cast from options with a comment explaining it's injected by the dev server. For better type safety, consider extending the options type or adding a dedicated interface.♻️ Optional: Type the watch flag explicitly
+interface ManifestOptions { + manifestEntries: IndexEntry[]; + presets?: any; + /** Injected by dev server for watch mode. */ + watch?: boolean; +} + export const manifests: PresetPropertyFn< 'experimental_manifests', StorybookConfigRaw, - { manifestEntries: IndexEntry[] } + ManifestOptions > = async (existingManifests = {}, options) => { - // `watch` is injected by the dev server but not declared in the preset function signature. - const { manifestEntries, presets, watch } = options as typeof options & { watch?: boolean }; + const { manifestEntries, presets, watch } = options;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/generator.ts` around lines 193 - 194, Add an explicit `watch` property to the options type instead of casting it inline: define/extend the options interface used by the preset function (the same type that declares `manifestEntries` and `presets`) to include `watch?: boolean`, update the preset/generator function signature to use that typed options parameter, and then read `const { manifestEntries, presets, watch } = options` without the ad-hoc `as` cast; this preserves type safety for `watch` across functions such as the generator/preset where `options` is accepted.code/renderers/react/src/componentManifest/checker/ComponentMetaManager.test.ts (1)
52-59: Cleanup function only deletes files, not directories.The
cleanupfunction deletes files but leaves empty directories behind. While this doesn't affect test correctness, it accumulates empty directories in.test-fixturesover time.♻️ Optional: Add directory cleanup
function cleanup(dir: string) { if (!sys.directoryExists(dir)) { return; } for (const entry of sys.readDirectory(dir, undefined, undefined, ['**/*'])) { sys.deleteFile!(entry); } + // Note: ts.sys doesn't provide rmdir, so directories remain. + // Consider using node:fs directly for full cleanup if needed. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/checker/ComponentMetaManager.test.ts` around lines 52 - 59, The cleanup function (cleanup) currently removes files returned by sys.readDirectory but leaves empty directories; update it to also remove directories by listing all entries, sorting them so children come before parents (e.g., descending path length), then for each entry use sys.fileExists/check + sys.deleteFile(entry) for files and sys.directoryExists/check + sys.deleteDirectory(entry) (or the equivalent sys.deleteFolder/removeDirectory API your sys provides) for directories; keep the initial guard using sys.directoryExists(dir) and ensure you delete nested directories after their files so no empty folders remain under .test-fixtures.
🤖 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/types/modules/core-common.ts`:
- Around line 375-379: Update the public type for
ComponentsManifest.meta.timings to match what's emitted by generator.ts: add the
missing properties reactDocgen and reactDocgenTypescript (same numeric type as
the other timings) to the timings object so the shape used in
ComponentsManifest.meta reflects generator.ts output; locate the timings
declaration in core-common.ts and add reactDocgen: number;
reactDocgenTypescript: number; to the timings interface/object.
In `@code/renderers/react/src/componentManifest/checker/ComponentMetaManager.ts`:
- Around line 329-333: The deletion branch currently only removes configPath
from rootTsConfigs when a project exists (code references rootTsConfigs,
configProjects, configPath), leaving deleted configs stale if no project
instance was present; update the logic so that when type === 'deleted' you
always remove the configPath from this.rootTsConfigs (and optionally from
this.configProjects) before proceeding — ensure the check for
this.configProjects.has(configPath) only gates project-specific cleanup and not
the rootTsConfigs deletion.
- Around line 455-456: The code calls watch(dir, { recursive: true }, ...)
inside ComponentMetaManager and silently returns on platforms where recursive is
unsupported; update the watcher creation to detect
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM (or platform !== 'win32' && platform !==
'darwin'), and implement a fallback: either instantiate a cross-platform watcher
(e.g., use chokidar) or walk the directory tree and create per-directory
fs.watch watchers, and ensure the catch path for the watch(dir, { recursive:
true }) failure logs an INFO/WARN-level message (including the error) so users
know recursive watching is unavailable; keep the original watcher variable name
(watcher) and ensure cleanup logic still closes the fallback watcher(s).
In `@code/renderers/react/src/componentManifest/checker/ComponentMetaProject.ts`:
- Around line 298-310: When isPackageImport is true and
this.typescript.resolveModuleName(...) returns no resolved.resolvedModule, fall
back to loading the file by entry.componentPath instead of aborting; update the
logic inside the block handling isPackageImport (around resolveModuleName,
resolved.resolvedModule, and program.getSourceFile) so that if
resolved.resolvedModule is falsy you set componentSourceFile =
program.getSourceFile(entry.componentPath) (same behavior as the else branch),
ensuring entry.importId, entry.storyFilePath, this.commandLine.options and
this.typescript.sys are still used for the preferred resolution but
entry.componentPath is used as a fallback.
In `@code/renderers/react/src/componentManifest/componentMetaExtractor.ts`:
- Around line 1409-1412: Remove the unnecessary optional chaining on
typescript.isTypeAssertionExpression and replace the unsafe cast: call
typescript.isTypeAssertionExpression(node) directly and pass a properly typed
expression to unwrapToFunctionAST by replacing (node as any).expression with
(node as typescript.TypeAssertion).expression (or ts.TypeAssertion if the local
alias is ts), ensuring the TypeAssertion type is imported/used consistently in
componentMetaExtractor.ts.
---
Nitpick comments:
In
`@code/renderers/react/src/componentManifest/checker/ComponentMetaManager.test.ts`:
- Around line 52-59: The cleanup function (cleanup) currently removes files
returned by sys.readDirectory but leaves empty directories; update it to also
remove directories by listing all entries, sorting them so children come before
parents (e.g., descending path length), then for each entry use
sys.fileExists/check + sys.deleteFile(entry) for files and
sys.directoryExists/check + sys.deleteDirectory(entry) (or the equivalent
sys.deleteFolder/removeDirectory API your sys provides) for directories; keep
the initial guard using sys.directoryExists(dir) and ensure you delete nested
directories after their files so no empty folders remain under .test-fixtures.
In `@code/renderers/react/src/componentManifest/generator.ts`:
- Around line 193-194: Add an explicit `watch` property to the options type
instead of casting it inline: define/extend the options interface used by the
preset function (the same type that declares `manifestEntries` and `presets`) to
include `watch?: boolean`, update the preset/generator function signature to use
that typed options parameter, and then read `const { manifestEntries, presets,
watch } = options` without the ad-hoc `as` cast; this preserves type safety for
`watch` across functions such as the generator/preset where `options` is
accepted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
code/core/src/core-server/utils/manifests/manifests.tscode/core/src/types/modules/core-common.tscode/renderers/react/package.jsoncode/renderers/react/src/componentManifest/__testfixtures__/Arrow.tscode/renderers/react/src/componentManifest/__testfixtures__/Button.tscode/renderers/react/src/componentManifest/__testfixtures__/DefaultExport.tscode/renderers/react/src/componentManifest/__testfixtures__/DefaultValues.tscode/renderers/react/src/componentManifest/__testfixtures__/DisplayNameOverride.tscode/renderers/react/src/componentManifest/__testfixtures__/Documented.tscode/renderers/react/src/componentManifest/__testfixtures__/DtsComponent.tsxcode/renderers/react/src/componentManifest/__testfixtures__/ForwardRef.tsxcode/renderers/react/src/componentManifest/__testfixtures__/FunctionProps.tscode/renderers/react/src/componentManifest/__testfixtures__/Generic.tscode/renderers/react/src/componentManifest/__testfixtures__/ImportedProps.tscode/renderers/react/src/componentManifest/__testfixtures__/Intersection.tscode/renderers/react/src/componentManifest/__testfixtures__/MultipleExports.tscode/renderers/react/src/componentManifest/__testfixtures__/PickOmit.tscode/renderers/react/src/componentManifest/__testfixtures__/RenamedExport.tscode/renderers/react/src/componentManifest/__testfixtures__/UnionProps.tscode/renderers/react/src/componentManifest/__testfixtures__/barrel/Button.tscode/renderers/react/src/componentManifest/__testfixtures__/barrel/Input.tscode/renderers/react/src/componentManifest/checker/ComponentMetaManager.test.tscode/renderers/react/src/componentManifest/checker/ComponentMetaManager.tscode/renderers/react/src/componentManifest/checker/ComponentMetaProject.test.tscode/renderers/react/src/componentManifest/checker/ComponentMetaProject.tscode/renderers/react/src/componentManifest/checker/index.tscode/renderers/react/src/componentManifest/componentMetaExtractor.checker.test.tscode/renderers/react/src/componentManifest/componentMetaExtractor.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/getComponentImports.test.tscode/renderers/react/src/componentManifest/getComponentImports.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts
Trigger the first extraction in the background as soon as routes are registered, so the TypeScript LanguageService, program, and type checker are ready before the first HTTP request arrives.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/generator.ts (1)
355-356: TightenProjectEntrytyping to remove the manifest cast.
ProjectEntry.componentcan be typed asReactComponentManifest, which removes the runtime-opaque cast before assignment.Type cleanup
- type ProjectEntry = { component: ComponentManifest; ctx: ComponentMetaContext }; + type ProjectEntry = { component: ReactComponentManifest; ctx: ComponentMetaContext }; @@ - (component as ReactComponentManifest).reactComponentMeta = docs[0]; + component.reactComponentMeta = docs[0];As per coding guidelines, "**/*.{ts,tsx}`: TypeScript strict mode is enabled and must be followed".
Also applies to: 392-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/generator.ts` around lines 355 - 356, ProjectEntry currently types component as the broad ComponentManifest which forces a runtime cast later; change ProjectEntry to use the narrower ReactComponentManifest for component (i.e. type ProjectEntry = { component: ReactComponentManifest; ctx: ComponentMetaContext }) and update any places that populate byProject (the Map keyed by manager.getProjectForFile) to assign ReactComponentManifest values without casting—ensure all assignments that previously did a runtime cast to ComponentManifest are updated to produce/return a ReactComponentManifest instead so the cast is no longer needed.
🤖 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/renderers/react/src/componentManifest/generator.ts`:
- Around line 271-296: The early return when !hasDocgen in generator.ts is
skipping extraction of reactComponentMeta/componentMetaCtx so entries with
component.path never reach the manager path; modify the block around hasDocgen
(the conditional that builds the error and returns a manifest) to still attach
componentMetaCtx/reactComponentMeta (from component?.path or csf.meta.component)
to the returned object when available, or avoid returning early and instead set
an error field but continue to populate and return base.manifest with
componentMetaCtx so the manager extraction (reactComponentMeta/componentMetaCtx
logic used elsewhere) can run; reference symbols: hasDocgen,
component?.reactDocgen, component?.reactDocgenTypescript, componentMetaCtx,
reactComponentMeta, csf, base, entry.importPath, storyFile.
- Around line 229-237: Wrap the per-entry file read and CSF parse (the logic
that computes storyFilePath, absoluteImportPath, storyFile =
cachedReadFileSync(...), and csf = loadCsf(...).parse()) in a try/catch so a
single failure doesn't reject the whole manifests() run; on error log the entry
identifier (e.g., entry.title or entry.importPath) and the error, skip
processing that entry (do not add its data to the manifest output), and continue
the loop. Ensure you catch both IO and parse errors and keep the rest of the
manifest generation unchanged.
---
Nitpick comments:
In `@code/renderers/react/src/componentManifest/generator.ts`:
- Around line 355-356: ProjectEntry currently types component as the broad
ComponentManifest which forces a runtime cast later; change ProjectEntry to use
the narrower ReactComponentManifest for component (i.e. type ProjectEntry = {
component: ReactComponentManifest; ctx: ComponentMetaContext }) and update any
places that populate byProject (the Map keyed by manager.getProjectForFile) to
assign ReactComponentManifest values without casting—ensure all assignments that
previously did a runtime cast to ComponentManifest are updated to produce/return
a ReactComponentManifest instead so the cast is no longer needed.
30aec66 to
940d3c3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/renderers/react/src/componentManifest/componentMetaExtractor.ts (1)
1410-1414:⚠️ Potential issue | 🟡 MinorFix optional chaining and type safety violation (flagged in past review).
This was flagged in a previous review. The issues remain:
typescript.isTypeAssertionExpression?.(node)— optional chaining is unnecessary since the API exists in TypeScript 4.0+ and the project uses TypeScript 5.9.3(node as any).expression— violates TypeScript strict mode// Type assertion: <Type>expr - if (typescript.isTypeAssertionExpression?.(node)) { - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- TS API compat (isTypeAssertionExpression) - return unwrapToFunctionAST(typescript, (node as any).expression, varMap, depth + 1); + if (typescript.isTypeAssertionExpression(node)) { + return unwrapToFunctionAST(typescript, (node as ts.TypeAssertion).expression, varMap, depth + 1); }Note: You'll need to use
ts.TypeAssertion(or reference the correct type from thetypescriptparameter if it differs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/componentMetaExtractor.ts` around lines 1410 - 1414, Remove the unnecessary optional chaining and replace the any cast with a proper TypeScript AST type: change the check to call typescript.isTypeAssertionExpression(node) directly, and cast node to the correct TypeScript AST type (e.g., typescript.TypeAssertion or the appropriate union type provided by the typescript parameter) before accessing the .expression, then pass that expression into unwrapToFunctionAST(typescript, expression, varMap, depth + 1); ensure you reference the same function names unwrapToFunctionAST and isTypeAssertionExpression so the change is localized and type-safe.
🧹 Nitpick comments (4)
code/renderers/react/src/componentManifest/getComponentImports.ts (2)
194-194: Remove duplicate comment.The comment "// missing binding -> keep (will become null import)" appears twice on the same line.
if (!binding) { - return false; - } // missing binding -> keep (will become null import) // missing binding -> keep (will become null import) + return false; // missing binding -> keep (will become null import) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/getComponentImports.ts` at line 194, In getComponentImports (file getComponentImports.ts) remove the duplicated inline comment "// missing binding -> keep (will become null import)" so only a single instance of that comment remains on the line; ensure spacing/formatting is preserved and no other trailing text is altered.
59-67: Mutable singleton for timing could cause issues in concurrent/parallel scenarios.
docgenTimingsis a module-level mutable object. If manifest generation runs concurrently (e.g., parallel workers), timings could be corrupted. Consider whether this is a concern given the usage context.For now this is likely fine since manifest generation appears to be single-threaded, but worth documenting the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/getComponentImports.ts` around lines 59 - 67, The module exports a mutable singleton object docgenTimings and a mutator resetDocgenTimings which can be corrupted if manifest generation ever runs concurrently; either make the API return a fresh timing object per run or document the single-threaded assumption. Update getComponentImports (and callers) to accept and return a timings object instead of using the module-level docgenTimings, or add a clear comment above the docgenTimings and resetDocgenTimings definitions stating they are intentionally single-threaded and must not be used concurrently; reference docgenTimings and resetDocgenTimings in the change so reviewers can find the export to update or annotate.code/renderers/react/src/componentManifest/utils.ts (1)
95-96: Type safety concern withcachedResolveImport: any.While the eslint-disable comment explains the complex overloads, exposing
anyloses type safety at call sites. Consider using a more precise type or at minimum documenting the expected signature in a JSDoc comment so callers know what they're getting.-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- resolveImport overloads are complex -export const cachedResolveImport: any = cached(resolveImport, { name: 'resolveImport' }); +/** Cached wrapper for resolveImport. Accepts (specifier, options) and returns the resolved path. */ +// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- resolveImport overloads are complex +export const cachedResolveImport: typeof resolveImport = cached(resolveImport, { name: 'resolveImport' }) as typeof resolveImport;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/utils.ts` around lines 95 - 96, The declaration cachedResolveImport: any loses type safety; replace the any with a precise function type derived from resolveImport (e.g., export const cachedResolveImport: typeof resolveImport = cached(resolveImport, { name: 'resolveImport' }) or create an explicit overloaded type alias matching resolveImport's signature and use that instead), and if overload complexity prevents exact typing add a clear JSDoc above cachedResolveImport describing the expected parameter and return types and why a best-fit type was chosen; ensure the identifier cachedResolveImport and the original resolveImport are referenced in your change.code/renderers/react/src/componentManifest/componentMetaExtractor.ts (1)
1259-1261: Unsafe cast of FunctionDeclaration as Expression.Casting a
FunctionDeclarationtoExpressionis type-unsafe since they're different AST node categories. This works at runtime becausevarMapis only used to look up identifiers, but it's a type hole.if (typescript.isFunctionDeclaration(stmt) && stmt.name) { - varMap.set(stmt.name.text, stmt as unknown as ts.Expression); + // Store function declarations separately or use a union type + varMap.set(stmt.name.text, stmt as ts.Node as ts.Expression); }Consider using
Map<string, ts.Expression | ts.FunctionDeclaration>for the varMap type, or document why the cast is safe.
🤖 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/renderers/react/src/componentManifest/generator.test.ts`:
- Around line 20-21: The tests call manifests(..., { manifestEntries } as any)
which bypasses TypeScript strictness; add a typed test helper in
generator.test.ts (e.g., a function like typedManifestOptions<T extends
ManifestOptions>(opts: Partial<T>): T) that narrows the options shape, then
replace the four instances of casting to any (the calls to manifests with {
manifestEntries } as any) to use this helper (pass the same object through
typedManifestOptions) so the calls remain strongly typed against the manifests
function signature.
---
Duplicate comments:
In `@code/renderers/react/src/componentManifest/componentMetaExtractor.ts`:
- Around line 1410-1414: Remove the unnecessary optional chaining and replace
the any cast with a proper TypeScript AST type: change the check to call
typescript.isTypeAssertionExpression(node) directly, and cast node to the
correct TypeScript AST type (e.g., typescript.TypeAssertion or the appropriate
union type provided by the typescript parameter) before accessing the
.expression, then pass that expression into unwrapToFunctionAST(typescript,
expression, varMap, depth + 1); ensure you reference the same function names
unwrapToFunctionAST and isTypeAssertionExpression so the change is localized and
type-safe.
---
Nitpick comments:
In `@code/renderers/react/src/componentManifest/getComponentImports.ts`:
- Line 194: In getComponentImports (file getComponentImports.ts) remove the
duplicated inline comment "// missing binding -> keep (will become null import)"
so only a single instance of that comment remains on the line; ensure
spacing/formatting is preserved and no other trailing text is altered.
- Around line 59-67: The module exports a mutable singleton object docgenTimings
and a mutator resetDocgenTimings which can be corrupted if manifest generation
ever runs concurrently; either make the API return a fresh timing object per run
or document the single-threaded assumption. Update getComponentImports (and
callers) to accept and return a timings object instead of using the module-level
docgenTimings, or add a clear comment above the docgenTimings and
resetDocgenTimings definitions stating they are intentionally single-threaded
and must not be used concurrently; reference docgenTimings and
resetDocgenTimings in the change so reviewers can find the export to update or
annotate.
In `@code/renderers/react/src/componentManifest/utils.ts`:
- Around line 95-96: The declaration cachedResolveImport: any loses type safety;
replace the any with a precise function type derived from resolveImport (e.g.,
export const cachedResolveImport: typeof resolveImport = cached(resolveImport, {
name: 'resolveImport' }) or create an explicit overloaded type alias matching
resolveImport's signature and use that instead), and if overload complexity
prevents exact typing add a clear JSDoc above cachedResolveImport describing the
expected parameter and return types and why a best-fit type was chosen; ensure
the identifier cachedResolveImport and the original resolveImport are referenced
in your change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/renderers/react/src/componentManifest/__testfixtures__/.eslintrc.cjscode/renderers/react/src/componentManifest/componentMetaExtractor.tscode/renderers/react/src/componentManifest/generator.test.tscode/renderers/react/src/componentManifest/getComponentImports.test.tscode/renderers/react/src/componentManifest/getComponentImports.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.test.tscode/renderers/react/src/componentManifest/utils.ts
✅ Files skipped from review due to trivial changes (2)
- code/renderers/react/src/componentManifest/testfixtures/.eslintrc.cjs
- code/renderers/react/src/componentManifest/reactDocgen.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
940d3c3 to
5b8be2b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
code/renderers/react/src/componentManifest/generator.test.ts (1)
20-21:⚠️ Potential issue | 🟠 MajorRemove
as anyfrommanifestscalls and keep tests strictly typed.Line 21, Line 292, Line 533, and Line 645 currently bypass typing with
as any(plus ESLint suppression). This can hidemanifestsAPI drift.💡 Suggested refactor
+type ManifestsOptions = NonNullable<Parameters<typeof manifests>[1]>; +type ManifestEntries = ManifestsOptions['manifestEntries']; + +const runManifests = (manifestEntries: ManifestEntries) => + manifests(undefined, { manifestEntries }); + test('manifests generates correct id, name, description and examples ', async () => { const manifestEntries = Object.values(indexJson.entries).filter( (entry) => entry.tags?.includes(Tag.MANIFEST) ?? false ); - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - const result = await manifests(undefined, { manifestEntries } as any); + const result = await runManifests(manifestEntries);Apply the same replacement at the other three call sites.
#!/bin/bash # Verify there are no remaining explicit-any bypasses in this test file. rg -nP 'as\s+any|@typescript-eslint/no-explicit-any' code/renderers/react/src/componentManifest/generator.test.tsAs per coding guidelines,
**/*.{ts,tsx}: TypeScript strict mode is enabled and must be followed.Also applies to: 291-292, 532-533, 644-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/generator.test.ts` around lines 20 - 21, Replace the ad-hoc "as any" casts when calling manifests by creating a properly typed argument object using the actual parameter type: declare a const like `const ctx: Parameters<typeof manifests>[1] = { manifestEntries };` and then call `await manifests(undefined, ctx);` Do this for every call site that currently uses `as any` (the calls to manifests at the locations using `manifestEntries`), removing the ESLint suppression comment as well so the tests remain strictly typed.code/renderers/react/src/componentManifest/componentMetaExtractor.ts (1)
1411-1413:⚠️ Potential issue | 🟠 MajorRemove optional guard +
anycast in type-assertion unwrapping.Line 1411-Line 1413 still uses
isTypeAssertionExpression?.(...)and(node as any).expression. This weakens strict typing and is the same concern previously raised.#!/bin/bash # Verify current usage and TS version declarations (read-only) rg -n "isTypeAssertionExpression\\?\\.|as any" code/renderers/react/src/componentManifest/componentMetaExtractor.ts rg -n "\"typescript\"\\s*:" package.jsonAs per coding guidelines,
**/*.{ts,tsx}: TypeScript strict mode is enabled and must be followed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/componentMetaExtractor.ts` around lines 1411 - 1413, Replace the optional-chained guard and any-cast with a proper TypeScript API type-narrowing: call typescript.isTypeAssertionExpression(node) (no ?. ) and then access node.expression as the correct TS AST type (e.g. cast to typescript.TypeAssertion or declare const ta = node as typescript.TypeAssertion) instead of (node as any).expression; then pass ta.expression into unwrapToFunctionAST(typescript, ta.expression, varMap, depth + 1). Ensure you reference typescript.isTypeAssertionExpression and typescript.TypeAssertion (or the right AST interface) so strict mode is satisfied.
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/utils.ts (1)
36-37: Avoid exportinganyhere—keepcachedResolveImportstrongly typed.Line [37] and especially Line [96] weaken type guarantees. Exporting
cachedResolveImport: anydrops argument/return checks for every caller.♻️ Suggested typing-only refactor
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- generic function cache needs any -let memoStore: WeakMap<(...args: any[]) => any, Map<string, unknown>> = new WeakMap(); +let memoStore: WeakMap<(...args: unknown[]) => unknown, Map<string, unknown>> = new WeakMap(); -// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- resolveImport overloads are complex -export const cachedResolveImport: any = cached(resolveImport, { name: 'resolveImport' }); +export const cachedResolveImport = cached(resolveImport as typeof resolveImport, { + name: 'resolveImport', +}) as typeof resolveImport;As per coding guidelines
**/*.{ts,tsx}: TypeScript strict mode is enabled and must be followed.Also applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/renderers/react/src/componentManifest/utils.ts` around lines 36 - 37, The memoization store and exported cachedResolveImport are currently typed with any; make them generic and strongly typed: change memoStore to a generic WeakMap keyed by the actual resolver function type (e.g., WeakMap<(...args: A) => R, Map<string, R>> using generic parameter names A and R) and update cachedResolveImport to be a generic function signature (e.g., function cachedResolveImport<A extends unknown[], R>(fn: (...args: A) => Promise<R> | R): (...args: A) => Promise<R>) so callers retain argument/return type safety; ensure all internal uses (lookups, keying, and stored values) use these generics and remove any use of the any type and implicit any returns.
🤖 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/renderers/react/src/componentManifest/componentMetaExtractor.ts`:
- Around line 755-767: The body-level destructuring currently collects defaults
from any object binding pattern; restrict this so collectBindingDefaults is only
invoked for destructuring tied to props: check varDecl.initializer and only
proceed if it's an Identifier named "props" or a CallExpression that either
calls "resolveProps" (initializer.expression is Identifier "resolveProps") or
whose first argument is the Identifier "props"; otherwise skip calling
collectBindingDefaults. This change affects the loop that examines
stmt.declarationList.declarations in componentMetaExtractor.ts and ensures
collectBindingDefaults is only run for props-derived initializers.
- Around line 1037-1042: computeDisplayName (in componentMetaExtractor.ts)
breaks on Windows because it uses fileName.split('/') to parse paths; replace
that logic with Node's path utilities: import path from 'path' (or require) and
use path.basename(fileName) to get the file base (strip extension with the
existing regex), and when base === 'index' use
path.basename(path.dirname(fileName)) to get the parent directory name; update
the code that sets base (and remove the manual split/pop logic) so it works on
both POSIX and Windows; ensure the import is added if not present.
In `@code/renderers/react/src/componentManifest/getComponentImports.ts`:
- Around line 211-214: The code currently computes member as mem = c.slice(dot +
1) using the first dot position, producing dotted member strings like
"Item.Root"; update the logic in getComponentImports so the member token is only
the terminal segment (e.g., "Root") by using the last dot position or splitting
on '.' and taking the last element before using localToImport and import
emission (refer to variables c, dot, ns, mem and the localToImport lookup).
In `@code/renderers/react/src/componentManifest/reactDocgenTypescript.test.ts`:
- Around line 1211-1212: Replace the comment-only coverage for the RDT edge
cases with a compact, executable table-driven assertion in
reactDocgenTypescript.test.ts that iterates the fixtures named DtsComponent,
ForwardRef, RenamedExport, DisplayNameOverride, and Barrel and asserts the
parser/RDT call returns [] for each; locate the test block currently containing
that comment, create an array of those fixture names and loop (or use a
parameterized/test.each) to call the same function used elsewhere in this file
to parse each fixture and expect the result toEqual([]), keeping the assertion
minimal and consistent with surrounding test helpers.
---
Duplicate comments:
In `@code/renderers/react/src/componentManifest/componentMetaExtractor.ts`:
- Around line 1411-1413: Replace the optional-chained guard and any-cast with a
proper TypeScript API type-narrowing: call
typescript.isTypeAssertionExpression(node) (no ?. ) and then access
node.expression as the correct TS AST type (e.g. cast to
typescript.TypeAssertion or declare const ta = node as typescript.TypeAssertion)
instead of (node as any).expression; then pass ta.expression into
unwrapToFunctionAST(typescript, ta.expression, varMap, depth + 1). Ensure you
reference typescript.isTypeAssertionExpression and typescript.TypeAssertion (or
the right AST interface) so strict mode is satisfied.
In `@code/renderers/react/src/componentManifest/generator.test.ts`:
- Around line 20-21: Replace the ad-hoc "as any" casts when calling manifests by
creating a properly typed argument object using the actual parameter type:
declare a const like `const ctx: Parameters<typeof manifests>[1] = {
manifestEntries };` and then call `await manifests(undefined, ctx);` Do this for
every call site that currently uses `as any` (the calls to manifests at the
locations using `manifestEntries`), removing the ESLint suppression comment as
well so the tests remain strictly typed.
---
Nitpick comments:
In `@code/renderers/react/src/componentManifest/utils.ts`:
- Around line 36-37: The memoization store and exported cachedResolveImport are
currently typed with any; make them generic and strongly typed: change memoStore
to a generic WeakMap keyed by the actual resolver function type (e.g.,
WeakMap<(...args: A) => R, Map<string, R>> using generic parameter names A and
R) and update cachedResolveImport to be a generic function signature (e.g.,
function cachedResolveImport<A extends unknown[], R>(fn: (...args: A) =>
Promise<R> | R): (...args: A) => Promise<R>) so callers retain argument/return
type safety; ensure all internal uses (lookups, keying, and stored values) use
these generics and remove any use of the any type and implicit any returns.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/.eslintrc.jscode/renderers/react/src/componentManifest/componentMetaExtractor.tscode/renderers/react/src/componentManifest/generator.test.tscode/renderers/react/src/componentManifest/getComponentImports.test.tscode/renderers/react/src/componentManifest/getComponentImports.tscode/renderers/react/src/componentManifest/reactDocgen.tscode/renderers/react/src/componentManifest/reactDocgenTypescript.test.tscode/renderers/react/src/componentManifest/utils.ts
✅ Files skipped from review due to trivial changes (1)
- code/.eslintrc.js
🚧 Files skipped from review as they are similar to previous changes (2)
- code/renderers/react/src/componentManifest/reactDocgen.ts
- code/renderers/react/src/componentManifest/getComponentImports.test.ts
- Add no-unused-vars override for __testfixtures__ in root .eslintrc.js
- Extract runManifests() test helper to centralize partial-options cast
- Use WeakMap<object, ...> instead of WeakMap<(...args: any[]) => any, ...>
- Remove unnecessary `: any` from cachedResolveImport (no overloads)
- Type PropItem.defaultValue as { value: string } (always a string)
- Split optional-chained type guard for proper TS narrowing
- Remove unnecessary `as any` casts where types already match
- Use unknown[] instead of any[] in reactDocgenTypescript test
- Remove dead reactDocgenConfig variable
- Use parameterless catch in reactDocgen.ts
kasperpeulen
force-pushed
the
kasper/react-prop-extraction-lsp
branch
from
5b8be2b to
0528828
Compare
JReinhold
requested changes
Mar 16, 2026
Contributor
There was a problem hiding this comment.
great work! This is massive, so I haven't been able to dive into the finer details. I left some comments, the biggest one probably being about the lack of readibility on the tests.
I think it would be valuable for everyone to get a sense of the overall architecture and code flow here, do you think it makes sense to commit a mermaid diagram in, or would that go stale soon?
Copilot gave me this, which helped me out a lot while reviewing, but only you would know if it's correct:
flowchart TD
A["Storybook manifest generation: generator.ts manifests()"] --> B["Select eligible entries"]
B --> C["Parse each story file with CSF tools"]
C --> D["getComponents discovers candidate components and imports"]
D --> E{"Docgen engine"}
E -->|react-docgen| F["react-docgen path"]
E -->|react-docgen-typescript| G["react-docgen-typescript path"]
E -->|experimentalReactComponentMeta| H["react-component-meta path"]
H --> I["Create or reuse ComponentMetaManager"]
I --> J["batchExtract(entries)"]
J --> K["Group entries by TypeScript project"]
K --> L["getProjectForFile(storyPath)"]
L --> M{"Matching tsconfig found"}
M -->|yes| N["Configured ComponentMetaProject"]
M -->|no| O["Inferred ComponentMetaProject"]
N --> P["One TypeScript LanguageService per project"]
O --> P
P --> Q["Shared file snapshot cache across projects"]
J --> R["project.extractPropsFromStories(entries)"]
R --> S["Ensure story and component files are in project"]
S --> T["Ensure freshness with mtime and projectVersion"]
T --> U["Get TypeScript Program and TypeChecker"]
U --> V{"Extraction path"}
V -->|primary| W["resolvePropsFromStoryFile scans existing story JSX"]
V -->|fallback| X["Resolve from meta.component type for args-only stories"]
W --> Y["getResolvedSignature on JSX element"]
X --> Z["Inspect call signatures or construct signatures"]
Y --> AA["Resolved props TypeScript type"]
Z --> AA
AA --> AB["serializeComponentDoc"]
AB --> AC["Normalize selected symbol especially compound members"]
AC --> AD["Extract prop docs defaults and JSDoc tags"]
AD --> AE["Filter bulk props from node_modules and d.ts sources"]
AE --> AF["Attach reactComponentMeta to component ref"]
AF --> AG["Back in generator.ts step 3"]
F --> AG
G --> AG
AG --> AH["Build final component manifest"]
AH --> AI["Add descriptions stories and import statements"]
AI --> AJ["Emit manifest JSON and HTML debugger output"]
P -. watch mode only .-> AK["fs.watch with debounce"]
AK -. changed files .-> AL["onFilesChanged and onConfigChanged"]
AL -. invalidates .-> P
AL -. warmup .-> AM["Re-run targeted extraction in background"]
mermaid code
flowchart TD
A["Storybook manifest generation: generator.ts manifests()"] --> B["Select eligible entries"]
B --> C["Parse each story file with CSF tools"]
C --> D["getComponents discovers candidate components and imports"]
D --> E{"Docgen engine"}
E -->|react-docgen| F["react-docgen path"]
E -->|react-docgen-typescript| G["react-docgen-typescript path"]
E -->|experimentalReactComponentMeta| H["react-component-meta path"]
H --> I["Create or reuse ComponentMetaManager"]
I --> J["batchExtract(entries)"]
J --> K["Group entries by TypeScript project"]
K --> L["getProjectForFile(storyPath)"]
L --> M{"Matching tsconfig found"}
M -->|yes| N["Configured ComponentMetaProject"]
M -->|no| O["Inferred ComponentMetaProject"]
N --> P["One TypeScript LanguageService per project"]
O --> P
P --> Q["Shared file snapshot cache across projects"]
J --> R["project.extractPropsFromStories(entries)"]
R --> S["Ensure story and component files are in project"]
S --> T["Ensure freshness with mtime and projectVersion"]
T --> U["Get TypeScript Program and TypeChecker"]
U --> V{"Extraction path"}
V -->|primary| W["resolvePropsFromStoryFile scans existing story JSX"]
V -->|fallback| X["Resolve from meta.component type for args-only stories"]
W --> Y["getResolvedSignature on JSX element"]
X --> Z["Inspect call signatures or construct signatures"]
Y --> AA["Resolved props TypeScript type"]
Z --> AA
AA --> AB["serializeComponentDoc"]
AB --> AC["Normalize selected symbol especially compound members"]
AC --> AD["Extract prop docs defaults and JSDoc tags"]
AD --> AE["Filter bulk props from node_modules and d.ts sources"]
AE --> AF["Attach reactComponentMeta to component ref"]
AF --> AG["Back in generator.ts step 3"]
F --> AG
G --> AG
AG --> AH["Build final component manifest"]
AH --> AI["Add descriptions stories and import statements"]
AI --> AJ["Emit manifest JSON and HTML debugger output"]
P -. watch mode only .-> AK["fs.watch with debounce"]
AK -. changed files .-> AL["onFilesChanged and onConfigChanged"]
AL -. invalidates .-> P
AL -. warmup .-> AM["Re-run targeted extraction in background"]
have you given any thought of the portability to this? I could imagine once this stabilizes, we might want to publish it as a standaloe @storybook/react-component-meta package for anyone to use. Does the architecture here support such an eventual split of the system?
finally, have you given any thoughts to how this could have been broken down into smaller PRs? I understand why you've done it all this way, but of course it makes it really hard to review and understand later. I don't know if sub parts of the system could have been neatly split up into telescoping PRs or not. At least maybe the watching part could have been self-contained work?
…e toMatchObject - Extract duplicated memfs mock setup from generator.test.ts and getComponentImports.test.ts into shared memfs-test-setup.ts - Restore dropped "detects multiple component exports" test - Restore dropped "sets source fileName on declarations for >30 filter" test - Refactor assertions across 6 test files to use toMatchObject for cleaner, more declarative expectations
…tractDoc - extract() now delegates to extractFromStory() using loadCsf → getComponents instead of hand-building StoryRef entries - extractFromStory() reuses withProject() instead of duplicating setup/teardown - withProject() supports async callbacks - Replace all ts.sys usage with standard node:fs in test helpers - Remove extractDoc helper from ComponentMetaManager tests - All test callbacks are async, extract() calls are awaited
…r-file setup - Delete old checker/ directory (moved to componentMeta/ in prior commits) - Delete old monolithic componentMetaExtractor.checker.test.ts - Delete old componentMetaExtractor.ts (moved to componentMeta/) - Move memfs mocks from vitest.setup.ts to per-file setup via __mocks__/ - Update generator.ts import path from checker to componentMeta - Add invalidateCache/invalidateParser calls to test setup
… formatting - extract() returns StoryRef directly, no throw, no ext option - Use toMatchObject everywhere instead of toBeDefined/toBe chains - Delete unused OptionalNested fixture (covered inline in props test) - Remove dead defaultImportName export, make DEFAULT_TSCONFIG private - Fix inconsistent template string indentation across test files
…move expect.anything noise
…pect.anything in infra tests
…lt values, title matching Cover the primary JSX path (resolvePropsFromStoryFile) via extractFromStory with stories that have no meta.component — title is auto-derived from the story file name, matching how generator.ts works in production. New tests: type serialization (tuple, Record, mapped, conditional, indexed access, template literal, enum, boolean collapse, Readonly, arrays, satisfies), default values (@DefaultValue, enum members, negative numbers, null, shorthand defaultProps, ternary/nullish coalescing body destructuring, overloaded functions, priority), JSX path import patterns (named, default, { default as X }, namespace, compound), title-based matching, jsxDepth selection, JSDoc tags (@deprecated, @see, @import).
…sistent naming props.test.ts: group flat list into describe blocks (basic types, union types, utility types, generics/inheritance, forwardRef, bulk filtering, JSDoc, satisfies, without meta.component). Consistent "extracts" verb throughout. detection.test.ts: dissolve "additional patterns" — move satisfies into "wrapped components", overloads into "function components". Merge "multiple" and "mixed" exports into one group. Move enum-like const into "non-components". qa.test.ts: remove JSDoc tag, title-matching, and JSX path tests already covered in props.test.ts. Keep only real-world library patterns (Park UI, Primer, Mantine) and standard Storybook fixtures. Trim verbose assertions to focus on key fields.
Member
Author
|
Thanks for the thorough review, Jeppe! Responding to the higher-level points: Mermaid diagram — That diagram is accurate! Tests readability — I have completely rewritten the tests so that they are as easy to read as possible! Portability — Yes, the architecture supports this. The three main pieces (
The only Storybook-specific glue is in Smaller PRs — Fair point. The watching part ( |
Instead of creating a fresh TypeScript project per test (re-parsing @types/react each time at ~150ms), share one ComponentMetaProject and swap file contents between tests. Incremental rebuilds take ~5ms.
JReinhold
approved these changes
Mar 18, 2026
This reverts commit 9f1ee2b.
Surface a specific manifest error when a resolved component file yields no react-docgen-typescript docs, and cover the regression with a dedicated test.
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.
What I did
Adds a new prop extraction engine (
react-component-meta) that uses TypeScript's own LanguageService — the same API behind IDE autocomplete — to detect components and extract props. Instead of AST heuristics (react-docgen) or throwawayts.Programper file (react-docgen-typescript), it maintains a persistent LanguageService with incremental updates.The architecture mirrors Vue Component Meta — built on
@volar/typescriptand@volar/language-core.Why
Neither existing engine works reliably on real design systems:
polymorphicFactory()export * as X from+ HOCstyled(ark.div, recipe)HOCReact.FC<T.Props>(namespace type import)forwardRef() as PolymorphicForwardRefComponentObject.assign(Component, { subs })compoundcreateComponent()for Web ComponentsHow it works
Probe-free. Instead of generating virtual files, the engine reads existing story files directly:
Path 1 — JSX extraction (~95%):
resolvePropsFromStoryFile()— finds JSX elements in the story file that match the target component (CSF1 or CSF3 withrender), then callschecker.getResolvedSignature()to extract the full props type. TypeScript resolves props the same way as autocompletion — polymorphic components with generic call signatures get instantiated with their default type parameter, giving correct concrete props.Path 2 — Component type fallback:
resolvePropsFromComponentType()— for args-only CSF3 stories with no JSX, inspects the component's type directly viagetCallSignatures()[0].parameters[0]. Does NOT work for polymorphic/generic components (TS #61133).We also tried virtual probe files with synthetic JSX to force
getResolvedSignatureon the fallback path — but injecting files into the TS program invalidates caches and hurts performance. Scanning real story files is much faster.Both paths produce a
ts.Typewhich is then serialized intoComponentDocformat byserializeComponentDocs().Why Volar-style LanguageService
The naive approach (
ts.createProgram()per request) means full TypeScript program creation on every dev server cycle — expensive for large projects like Mantine (9 tsconfig projects). Instead, we use@volar/typescriptdirectly:ComponentMetaProject— oneLanguageServiceper tsconfig (mirrors Volar'sTypeScriptProjectHost+createTypeScriptCheckerLanguageService)ComponentMetaManager— manages multiple projects, routes component requests to the right tsconfig (mirrors Volar LS'stypescriptProject.ts)projectVersion++invalidation — on file change events, bump the version counter. The LS compares file mtimes and only recompiles what changedfs.watchwith debounce. Watchers are created lazily when projects are discovered@volar/language-core'sFileMap(monorepo support — Mantine's 9 tsconfigs share one snapshot cache)scheduleWarmup()replays the last extraction so the cache is hot before the next requestArchitecture (Volar alignment)
TypeScriptProjectHost+createLanguageServiceHostComponentMetaProjecttypescriptProject.tsComponentMetaManager@volar/language-corecreateLanguage()@volar/language-corecreateLanguage()@volar/typescriptcreateLanguageServiceHost()@volar/typescriptcreateLanguageServiceHost()isReactComponentType()getResolvedSignature()on JSX /getCallSignatures()fallbackFileMapwith mtime cacheFileMapwith mtime cacheprojectVersion++projectVersion++Other changes
getComponentImports.ts— tracks JSX nesting depth per component, addsmemberfield for member expressions (Accordion.Root→member: "Root"). Both react-docgen and react-docgen-typescript always run.generator.ts— prefers outermost JSX component (shallowest depth) instead of alphabetical order.memberAccessderived fromcomponent.memberinstead of regex scanning..d.tsfiles (catches generated type systems like Panda CSSstyled-system/).Results
Tested on 5 design systems with published canary
0.0.0-pr-33914-sha-ba1d6f53.Component Detection
RCM: 526/526 — 100% detection across all 5 projects.
Props Extracted
Mantine RDT (12,382) > RCM (10,292) — RDT includes inherited HTML/DOM props (100+
HTMLAttributesper component). RCM's source filter (>30 props from a single.d.ts) keeps only meaningful props.Prop Quality (RCM = ground truth)
Deep per-component validation: RCM is a strict superset of both RD and RDT on 4 out of 5 projects. On Mantine, the only gap is
.d.tsstyle props (by design — in a user's project, Mantine is innode_modulesand style props are correctly excluded). No engine ever found a prop that RCM incorrectly missed.Performance — Cold Start (build mode)
Cold start happens once. RCM is in the same ballpark as RDT — except Park UI where RDT takes 33 seconds (Panda CSS types are deep + throwaway Program per file).
Performance — Dev Mode (after file edit)
RD and RDT rebuild from scratch every time. RCM's LanguageService is persistent — 18–200ms after every edit across all projects.
File Invalidation (no story touch needed)
ListGroup.tsxButton.types.tsbutton.tsxButton/types.tsButton.tsxOnly the component source file was edited — no story file touch needed. File watcher detects changes → bumps
projectVersion++→ LanguageService incrementally recompiles only affected files.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
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.Manual testing
Tested on 5 design systems (Flowbite, Reshaped, Park UI, Primer, Mantine) with published canary. See results tables in the PR description above.
Summary by CodeRabbit
New Features
Improvements