React: Improve error handling of component manifest generation#32855
Conversation
📝 WalkthroughWalkthroughAdds NodePath-backed tracking to CSF parsing (meta and story declaration paths), tightens Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Parser as CsfFile
participant Generator as ManifestGenerator
participant Snippet as getCodeSnippet
participant Enricher as EnrichCsf
Generator->>Parser: parse file (BabelFile.path: NodePath)
Parser-->>Generator: return CSF with _metaStatementPath & _storyDeclarationPath
Generator->>Snippet: getCodeSnippet(csf, storyName, component?)
Snippet->>Snippet: lookup declaration via csf._storyDeclarationPath[storyName]
Snippet->>Snippet: normalize initializer (.bind, TS wrappers), merge meta/story args
alt unresolved / invalid
Snippet-->>Generator: throw code-frame error
else
Snippet-->>Generator: return AST node (VariableDeclaration | FunctionDeclaration)
end
Generator->>Enricher: provide AST node for formatting
Enricher->>Enricher: recast.print(node).code -> format(...)
alt format/print fails
Enricher-->>Generator: record per-example error
else
Enricher-->>Generator: attach formatted snippet to docs.source.code
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 74728f2
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
code/renderers/react/src/enrichCsf.ts (1)
21-27: Consider logging snippet generation failures for observability.The try-catch silently swallows errors during code snippet generation. While avoiding user-facing errors for non-critical features is reasonable, consider logging failures (using Storybook's node-logger) to help maintainers identify and fix issues in snippet generation. This is especially valuable during development and debugging.
+import { logger } from 'storybook/internal/node-logger'; + export const enrichCsf: PresetPropertyFn<'experimental_enrichCsf'> = async (input, options) => { const features = await options.presets.apply('features'); if (!features.experimentalCodeExamples) { return; } return async (csf: CsfFile, csfSource: CsfFile) => { const promises = Object.entries(csf._storyPaths).map(async ([key, storyExport]) => { if (!csfSource._meta?.component) { return; } const { format } = await getPrettier(); let node; try { node = getCodeSnippet(storyExport, csfSource._metaNode, csfSource._meta?.component); } catch (e) { - // don't bother the user if we can't generate a snippet + logger.debug(`Failed to generate snippet for story "${key}":`, e); return; }As per coding guidelines.
code/renderers/react/src/componentManifest/generator.test.ts (1)
218-218: Verify: Should successful cases showerror: undefined?The snapshots now include
error: undefinedfor successful manifest generation. Consider whether theerrorfield should be omitted entirely when there's no error (using optional field semantics) rather than explicitly setting it toundefined. This would make the successful case output cleaner and reduce payload size.Current behavior:
{ error: undefined, name: "Button", ... }Alternative:
{ name: "Button", ... } // error field omitted when undefinedAlso applies to: 322-322
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (5)
30-33: Avoid eager code-frame construction in invariant for performanceWrap the
buildCodeFrameError(...).messagein a lazy function, as elsewhere, to avoid doing work on the happy path.- invariant( - declarations.length === 1, - storyExportPath.buildCodeFrameError('Expected one story declaration').message - ); + invariant( + declarations.length === 1, + () => storyExportPath.buildCodeFrameError('Expected one story declaration').message + );
84-89: Unwrap TS non‑null expressions during normalizationHandle
TSNonNullExpression(expr!) like other TS wrappers so downstream checks see the inner expression.- normalizedInit = normalizedInit.isTSSatisfiesExpression() - ? normalizedInit.get('expression') - : normalizedInit.isTSAsExpression() - ? normalizedInit.get('expression') - : normalizedInit; + normalizedInit = normalizedInit.isTSSatisfiesExpression() + ? normalizedInit.get('expression') + : normalizedInit.isTSAsExpression() + ? normalizedInit.get('expression') + : normalizedInit.isTSNonNullExpression?.() + ? normalizedInit.get('expression') + : normalizedInit;
253-255: Add code‑frame context when component name is missingSurface a code‑framed message to improve debugging; you have
storyExportPathin scope.- invariant(componentName, 'Could not generate snippet without component name.'); + invariant( + componentName, + () => + storyExportPath.buildCodeFrameError( + 'Could not generate snippet without component name. Did you set meta.component?' + ).message + );
326-342: Correct handling of namespaced JSX attributes (e.g., xlink:href)
toAttrbuildst.jsxIdentifier(key)for all keys. For namespaced attrs containing:, this should be aJSXNamespacedName; otherwise the AST can be semantically off. Keep your existing validation; just construct the right node.-const toAttr = (key: string, value: t.Node): t.JSXAttribute | null => { - if (t.isBooleanLiteral(value)) { - // Keep falsy boolean attributes by rendering an explicit expression container - return value.value - ? t.jsxAttribute(t.jsxIdentifier(key), null) - : t.jsxAttribute(t.jsxIdentifier(key), t.jsxExpressionContainer(value)); - } - - if (t.isStringLiteral(value)) { - return t.jsxAttribute(t.jsxIdentifier(key), t.stringLiteral(value.value)); - } - - if (t.isExpression(value)) { - return t.jsxAttribute(t.jsxIdentifier(key), t.jsxExpressionContainer(value)); - } - return null; // non-expression nodes are not valid as attribute values -}; +const toAttr = (key: string, value: t.Node): t.JSXAttribute | null => { + const makeName = (): t.JSXIdentifier | t.JSXNamespacedName | null => { + if (key.includes(':')) { + const [ns, local] = key.split(':'); + if (!ns || !local) return null; + return t.jsxNamespacedName(t.jsxIdentifier(ns), t.jsxIdentifier(local)); + } + return t.jsxIdentifier(key); + }; + const name = makeName(); + if (!name) return null; + + if (t.isBooleanLiteral(value)) { + // Keep falsy boolean attributes by rendering an explicit expression container + return value.value + ? t.jsxAttribute(name, null) + : t.jsxAttribute(name, t.jsxExpressionContainer(value)); + } + if (t.isStringLiteral(value)) { + return t.jsxAttribute(name, t.stringLiteral(value.value)); + } + if (t.isExpression(value)) { + return t.jsxAttribute(name, t.jsxExpressionContainer(value)); + } + return null; // non-expression nodes are not valid as attribute values +};
496-522: Minor duplication of injection logic
makeInjectedPiecesduplicates attr filtering logic already computed earlier. Consider extracting a small helper to compute injected attrs and invalid spread once to keep behavior consistent.code/renderers/react/src/componentManifest/generator.ts (5)
33-34: Usemapinstead offlatMapwith async callback
flatMapprovides no benefit here (callbacks return non-array Promises) and is misleading.- const components = await Promise.all( - singleEntryPerComponent.flatMap(async (entry): Promise<ReactComponentManifest> => { + const components = await Promise.all( + singleEntryPerComponent.map(async (entry): Promise<ReactComponentManifest> => {
60-66: Rename loop variable to avoid shadowingpathimportThe
[storyName, path]binding shadows the top‑levelpathimport and hurts readability.- .map(([storyName, path]) => { + .map(([storyName, storyExportPath]) => { try { return { name: storyName, - snippet: recast.print(getCodeSnippet(path, csf._metaNode ?? null, name)).code, + snippet: recast.print(getCodeSnippet(storyExportPath, csf._metaNode ?? null, name)).code, };
78-86: Likely unreachable!entry.componentPathbranchYou filter
entry.componentPathtruthy earlier (Line 26), so this guard should never trigger. If intentional for future resilience, keep it; otherwise remove to reduce noise.
92-101: Preserve original I/O error in messageIncluding the cause eases debugging.
- error: { - message: `Could not read the component file located at ${entry.componentPath}`, - }, + error: { + message: `Could not read the component file located at ${entry.componentPath}: ${e.message}`, + },
110-111: Enrich docgen error with file contextAdd the component path (and/or name) to make the error actionable.
- const error = !docgen ? { message: 'Docgen could not find a component' } : undefined; + const error = !docgen + ? { message: `Docgen could not find a component in ${entry.componentPath} for "${name}"` } + : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/core/src/csf-tools/CsfFile.ts(4 hunks)code/core/src/types/modules/core-common.ts(1 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx(1 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.ts(5 hunks)code/renderers/react/src/componentManifest/generator.test.ts(6 hunks)code/renderers/react/src/componentManifest/generator.ts(3 hunks)code/renderers/react/src/componentManifest/utils.ts(1 hunks)code/renderers/react/src/enrichCsf.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/enrichCsf.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/utils.tscode/renderers/react/src/componentManifest/generator.test.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/generateCodeSnippet.tscode/core/src/csf-tools/CsfFile.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/enrichCsf.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/utils.tscode/renderers/react/src/componentManifest/generator.test.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/generateCodeSnippet.tscode/core/src/csf-tools/CsfFile.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/enrichCsf.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/utils.tscode/renderers/react/src/componentManifest/generator.test.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/generateCodeSnippet.tscode/core/src/csf-tools/CsfFile.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/enrichCsf.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/utils.tscode/renderers/react/src/componentManifest/generator.test.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/componentManifest/generateCodeSnippet.tscode/core/src/csf-tools/CsfFile.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/generator.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/generator.test.ts
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsxcode/renderers/react/src/componentManifest/generator.test.ts
🧠 Learnings (6)
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to type and access mocked functions
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with vi.mocked()
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Applied to files:
code/renderers/react/src/componentManifest/generator.test.ts
🧬 Code graph analysis (4)
code/renderers/react/src/enrichCsf.ts (1)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(18-275)
code/renderers/react/src/componentManifest/generator.test.ts (1)
code/renderers/react/src/componentManifest/generator.ts (1)
componentManifestGenerator(20-141)
code/renderers/react/src/componentManifest/generator.ts (6)
code/core/src/csf-tools/CsfFile.ts (1)
loadCsf(1058-1062)code/core/src/types/modules/core-common.ts (1)
ComponentManifest(348-358)code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(18-275)code/renderers/react/src/componentManifest/utils.ts (1)
invariant(14-19)code/core/src/csf-tools/enrichCsf.ts (1)
extractDescription(158-178)code/renderers/react/src/componentManifest/jsdocTags.ts (1)
extractJSDocInfo(5-21)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
code/renderers/react/src/componentManifest/utils.ts (1)
invariant(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (10)
code/core/src/csf-tools/CsfFile.ts (3)
36-36: LGTM: Type safety improvement.Changing from
anytoNodePath<t.Program>provides better type safety and aligns with Babel's actual API.
304-304: LGTM: NodePath tracking for enhanced error reporting.The addition of
_metaStatementPathcomplements the existing_metaStatementfield and provides access to Babel's NodePath API for generating better error messages with code frames. The implementation correctly maintains consistency by setting both fields together in all code paths.Also applies to: 490-497, 504-504
1052-1055: LGTM: Appropriate configuration for error handling context.Adding
highlightCode: falseto the BabelFileClass options is reasonable for server-side tooling where automatic syntax highlighting may not be needed or where custom error formatting is preferred.code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (1)
80-94: LGTM! Improved error handling test.The test now properly validates that invalid story exports throw descriptive errors with code frames, making debugging easier for developers.
code/renderers/react/src/enrichCsf.ts (1)
32-34: LGTM! Cleaner code with extracted node variable.The refactoring improves readability by separating snippet generation from formatting.
code/renderers/react/src/componentManifest/utils.ts (1)
14-19: LGTM! Well-designed invariant helper.The invariant function properly supports lazy message evaluation for performance and uses TypeScript's assertion signature for type narrowing. This is a clean, reusable utility for runtime checks.
code/renderers/react/src/componentManifest/generator.test.ts (3)
12-12: Simplified mock implementation.The change from a spy-based mock to a direct implementation is cleaner and more explicit about the mock behavior (returning null result type).
418-482: LGTM! Well-structured test helpers.The
getManifestForStoryandwithCSF3helper functions significantly improve test readability and maintainability by reducing duplication.
484-615: LGTM! Comprehensive error scenario coverage.The three new test cases effectively cover important edge cases:
- Missing component field in meta
- Component exported from another file (re-export)
- Unknown/invalid story expressions
Each test validates that appropriate error messages with code frames are generated, which will greatly improve the developer experience when debugging manifest generation issues.
code/core/src/types/modules/core-common.ts (1)
348-358: Design is intentional and correctly typed; no changes required.The optional
namefield is intentionally paired with theerrorfield as an error-reporting mechanism. The code ingenerator.tsshows thatnameis only absent whencomponentNamecannot be extracted (meta.component not specified), and in that case, theerrorfield explains why. All other error scenarios includename. This is a valid pattern where the manifest can report processing failures while still providing apathreference for debugging. The type definition correctly reflects this design.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (2)
24-41: Solid error handling with room for minor optimization.The normalization logic correctly handles both
FunctionDeclarationandVariableDeclaratorpaths, and the use of code-frame errors significantly improves debugging.Consider simplifying the invariant pattern at lines 30–35 to avoid constructing an Error object just to extract its message:
- invariant( - init.isExpression(), - () => - storyDeclaration.buildCodeFrameError('Expected story initializer to be an expression') - .message - ); + if (!init.isExpression()) { + throw storyDeclaration.buildCodeFrameError('Expected story initializer to be an expression'); + }This pattern can be applied to similar invariant calls throughout the file (e.g., lines 66–74).
64-76: Consider more specific error messages for call expression validation.The fallback logic for call expressions is correct, but both invariants (lines 66–69 and 71–74) use the same generic message "Could not evaluate story expression."
More specific messages would help users identify whether the issue is argument count or argument type:
invariant( args.length === 1, - () => storyPath.buildCodeFrameError('Could not evaluate story expression').message + () => storyPath.buildCodeFrameError('Story factory must have exactly one argument').message ); const storyArgument = args[0]; invariant( storyArgument.isExpression(), - () => storyPath.buildCodeFrameError('Could not evaluate story expression').message + () => storyPath.buildCodeFrameError('Story factory argument must be an expression').message );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx(4 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.ts
🧬 Code graph analysis (1)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
code/renderers/react/src/componentManifest/utils.ts (1)
invariant(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (11)
.gitignore (1)
68-69: Note: File scope mismatch with PR objectives.The provided file (
.gitignore) does not align with the PR objectives, which focus on improving React error handling for component manifest generation, AST tracking, and schema extensions. Only a subset of files appears to be under review.The
.gitignoreentry addition itself is straightforward and low-risk. However, please clarify:
- What is
.junie? (artifact, build output, tool-specific file?)- Are other files changed in this PR pending review?
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (10)
18-23: LGTM! Signature refactoring aligns with CSF1/CSF2 flexibility.The broadened signature correctly accepts both variable declarators and function declarations, and the optional
componentNameallows for stories that may not need explicit component wrapping. The mixed return type properly reflects the different output formats.
79-83: LGTM! TypeScript type assertion unwrapping is handled correctly.The nested ternary pattern cleanly unwraps both
satisfiesandasexpressions.
86-100: LGTM! Function detection logic is comprehensive.The code correctly identifies arrow functions, function expressions, and function declarations, and appropriately throws a code-frame error for unsupported story shapes.
102-118: LGTM! CSF2 render function extraction is well-structured.The code correctly prioritizes an explicit
render()function when present in CSF2 object stories.
120-138: LGTM! Args merging follows CSF semantics.The code correctly merges
meta.argsandstory.argswith proper override behavior, and appropriately separates valid JSX attribute names from invalid ones.
140-156: LGTM! Arrow function JSX transformation is correct.The code properly handles arrow functions that directly return JSX elements or fragments, applying spread transformation and arg inlining as needed.
158-211: LGTM! Block body transformation comprehensively handles function variants.The code correctly extracts block bodies from function declarations, arrow functions, and function expressions, transforming only return statements that return JSX. The preservation of function form (declaration vs. expression) in the output is appropriate.
214-236: LGTM! Fallback JSX generation is sound.The invariant at line 217 ensures
componentNameis present before constructing the JSX element, preventing potential null/undefined errors. The fallback correctly builds a simple arrow function returning the component with merged args.
542-559: LGTM! Children injection logic is correct.The code correctly recurses into children and only injects
merged.childrenwhen appropriate (args spread present, no existing children, and children available in merged args). This prevents unintentionally overwriting existing children.
586-590: LGTM! Signature broadening improves reusability.Changing the parameter from a specific export declaration type to a generic
NodePath<t.Node>makes the function more flexible while maintaining correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/renderers/react/src/componentManifest/generator.ts (2)
58-77: Strong per-story error handling; remove unnecessary filter.The per-story error handling with try/catch is excellent and aligns with the PR objective. Each story failure is captured individually without breaking the entire manifest generation.
However, the
.filter(Boolean)on line 77 is unnecessary since themapcallback always returns an object (either withsnippetorerror), never a falsy value.Apply this diff to remove the unnecessary filter:
}) - .filter(Boolean); +;
89-103: Robust file reading error handling.The try/catch around file reading is excellent. The non-null assertion on line 92 is safe due to the early return at line 79, but consider extracting
entry.componentPathto a variable after the check for improved clarity.Optional: Extract
componentPathafter the check for improved type safety and clarity:if (!entry.componentPath) { const message = `No component file found for the "${name}" component.`; return { ...base, name, examples, error: { message }, }; } + const componentPath = entry.componentPath; let componentFile; try { - componentFile = await readFile(path.join(process.cwd(), entry.componentPath!), 'utf-8'); + componentFile = await readFile(path.join(process.cwd(), componentPath), 'utf-8'); } catch (e) { invariant(e instanceof Error); return { ...base, name, examples, error: { - message: `Could not read the component file located at ${entry.componentPath}`, + message: `Could not read the component file located at ${componentPath}`, }, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/generator.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generator.ts
🧬 Code graph analysis (1)
code/renderers/react/src/componentManifest/generator.ts (5)
code/core/src/csf-tools/CsfFile.ts (1)
loadCsf(1060-1064)code/core/src/types/modules/core-common.ts (1)
ComponentManifest(348-358)code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(18-237)code/renderers/react/src/componentManifest/utils.ts (1)
invariant(14-19)code/core/src/csf-tools/enrichCsf.ts (1)
extractDescription(158-178)
🔇 Additional comments (4)
code/renderers/react/src/componentManifest/generator.ts (4)
47-56: Excellent error handling with code context.This change perfectly aligns with the PR objective to surface errors with line numbers and surrounding context instead of silently skipping components. Using
buildCodeFrameErrorprovides valuable debugging information.
79-87: Good error surfacing for missing component paths.This properly handles the case where
componentPathis missing, surfacing the error in the manifest instead of silently skipping. This works in tandem with the relaxed filter condition on line 26.
111-111: Good docgen failure surfacing.Computing and attaching the error when docgen fails to find a component ensures failures are visible in the manifest instead of being silently ignored.
119-129: Consistent manifest structure with proper error propagation.The final manifest assembly properly spreads the base object and includes the computed error field. The populated
examplesarray correctly overwrites the empty array frombase, ensuring all story snippets and their errors are included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/renderers/react/src/componentManifest/generator.ts (1)
42-60: Remove unnecessary.filter(Boolean).The
.map()callback always returns an object (either withsnippetorerror), so the.filter(Boolean)on line 60 is redundant. All mapped values are truthy.Apply this diff:
}) - .filter(Boolean); +;Otherwise, the per-story error handling looks excellent and aligns well with the PR objectives—errors are captured with their messages rather than being hidden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/generator.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generator.ts
🧬 Code graph analysis (1)
code/renderers/react/src/componentManifest/generator.ts (6)
code/core/src/csf-tools/CsfFile.ts (1)
loadCsf(1060-1064)code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(18-237)code/renderers/react/src/componentManifest/utils.ts (1)
invariant(14-19)code/core/src/types/modules/core-common.ts (1)
ComponentManifest(348-358)code/core/src/csf-tools/enrichCsf.ts (1)
extractDescription(158-178)code/renderers/react/src/componentManifest/jsdocTags.ts (1)
extractJSDocInfo(5-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (6)
code/renderers/react/src/componentManifest/generator.ts (6)
37-40: LGTM!The extraction of
id,importPath, andnameis clear and sets up the base data for manifest generation.
62-67: LGTM!The base object pattern avoids duplication across the multiple return paths and ensures consistent manifest structure. The
satisfiesclause provides good type safety.
69-78: LGTM!The missing componentName handling is excellent:
- Clear, actionable error message
- Uses
buildCodeFrameErrorto provide line numbers and context- Falls back gracefully when
_metaStatementPathis unavailable- Aligns perfectly with the PR objective to surface errors with context
80-104: LGTM!The error handling for missing or unreadable component files is solid:
- Returns partial manifests with
nameandexampleswhen available- Provides clear error messages about what went wrong
- The non-null assertion on line 93 is safe because we're inside the else branch
- Preserving partial data helps users understand what succeeded before the failure
This aligns well with the PR objective to report errors clearly while providing maximum context.
106-130: LGTM!The docgen handling and final manifest assembly are well-structured:
- Sets an error when docgen fails but continues processing to provide as much data as possible
- Includes all available metadata (description, summary, tags, examples)
- The error field is conditionally included only when needed
- Spreading the base object maintains consistency across all return paths
This approach maximizes the information available to users even when parts of the manifest generation fail, which aligns perfectly with the PR objectives.
26-26: The subtype filter is necessary and correct—it is not redundant.The type definition shows that
StoryIndexEntryentries can havesubtype: 'story' | 'test'. Test entries sharetype: 'story'with regular stories but havesubtype: 'test'. The second filter correctly excludes these test entries from the component manifest, which should only include regular stories, not tests. The filter is defensive and intentional, not unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/renderers/react/src/componentManifest/generator.ts (1)
41-57: Don’t assert inside catch; coerce and continue (prevents aborting generation).invariant(e instanceof Error) can throw and terminate manifest generation. Coerce unknown throws to string and include file/story context in the error message.
- .map((storyName) => { + .map((storyName) => { try { return { name: storyName, snippet: recast.print(getCodeSnippet(csf, storyName, name)).code, }; } catch (e) { - invariant(e instanceof Error); + const message = e instanceof Error ? e.message : String(e); return { name: storyName, - error: { - message: e.message, - }, + error: { message: `Snippet generation failed for ${entry.importPath}#${storyName}: ${message}` }, }; } })
🧹 Nitpick comments (5)
code/renderers/react/src/componentManifest/generator.ts (5)
34-34: Use map, not flatMap, with async callbacks.flatMap ignores the “flattening” benefit when the callback returns a Promise; it reads as a mistake.
- singleEntryPerComponent.flatMap(async (entry): Promise<ReactComponentManifest> => { + singleEntryPerComponent.map(async (entry): Promise<ReactComponentManifest> => {
68-76: Avoid redundant fields; base already includes name/examples.Keeps objects smaller and prevents accidental divergence.
if (!entry.componentPath) { const message = `No component file found for the "${name}" component.`; return { ...base, - name, - examples, error: { message }, }; }
113-122: Same redundancy in final return; name/examples are already in base.Remove duplicates; rely on base.
return { ...base, - name, description: manifestDescription?.trim(), summary: tags.summary?.[0], import: tags.import?.[0], reactDocgen: docgen, jsDocTags: tags, - examples, error, };
14-14: Drop unused invariant import after fixes.Prevents dead code warnings.
-import { groupBy, invariant } from './utils'; +import { groupBy } from './utils';
60-66: Useentry.componentPathwith fallback toimportPathfor thepathfield.The ComponentManifest.path should reference the component file, not the story file. In the test setup,
componentPath: './src/stories/Button.tsx'andimportPath: './src/stories/Button.stories.ts'are distinct. Currently, path is always set to importPath (story file), which doesn't align with the interface name semantically.The suggested change
path: entry.componentPath ?? importPathcorrectly prioritizes the component file when available while maintaining consistency in the error path (which spreads...base). The test snapshot will need updating to reflect the new path value:'./src/stories/Button.tsx'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/renderers/react/src/componentManifest/generator.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generator.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generator.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generator.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generator.ts
🧬 Code graph analysis (1)
code/renderers/react/src/componentManifest/generator.ts (6)
code/renderers/react/src/componentManifest/utils.ts (2)
groupBy(2-12)invariant(15-23)code/core/src/csf-tools/CsfFile.ts (1)
loadCsf(1060-1064)code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(6-211)code/core/src/types/modules/core-common.ts (1)
ComponentManifest(348-358)code/core/src/csf-tools/enrichCsf.ts (1)
extractDescription(158-178)code/renderers/react/src/componentManifest/jsdocTags.ts (1)
extractJSDocInfo(5-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/renderers/react/src/componentManifest/generator.ts (2)
24-29: Double filtering by type/subtype — confirm intent.If type === 'story' already guarantees story entries, the subtype filter may be redundant; if not, keep both. Just flagging for clarity.
37-37: Remove the suggested type guard - no issue exists.The code already ensures
csf._meta?.componentis a string. InCsfFile.tsline 395, the component property is explicitly assigned viarecast.print(p.value, {}).code, which converts the AST node to a string. The typeStaticMeta.componentis correctly defined asstring | undefined(line 263), and the fallback handles the undefined case.Likely an incorrect or invalid review comment.
| try { | ||
| componentFile = await readFile(path.join(process.cwd(), entry.componentPath!), 'utf-8'); | ||
| } catch (e) { | ||
| invariant(e instanceof Error); | ||
| return { | ||
| ...base, | ||
| name, | ||
| examples, | ||
| error: { | ||
| message: `Could not read the component file located at ${entry.componentPath}`, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Same issue: remove invariant in catch; preserve original cause.
This can also abort the run. Include the fs error message for debugging.
- } catch (e) {
- invariant(e instanceof Error);
- return {
- ...base,
- name,
- examples,
- error: {
- message: `Could not read the component file located at ${entry.componentPath}`,
- },
- };
- }
+ } catch (e) {
+ const message = e instanceof Error ? e.message : String(e);
+ return {
+ ...base,
+ error: {
+ message: `Could not read the component file at ${entry.componentPath}: ${message}`,
+ },
+ };
+ }🤖 Prompt for AI Agents
In code/renderers/react/src/componentManifest/generator.ts around lines 80 to
91, the catch block currently calls invariant(e instanceof Error) which can
abort the run and discards the original fs error; change the catch to not use
invariant, instead capture the thrown value safely (e.g. derive a message via e
instanceof Error ? e.message : String(e)) and include that message (and
optionally stack) in the returned error object so the original filesystem error
is preserved for debugging while still returning the manifest error.
| const error = !docgen | ||
| ? { | ||
| message: `Could not parse props information for the located at ${entry.componentPath}`, | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
Fix typo and improve clarity of docgen error.
Minor grammar; also clarify the target file.
- ? {
- message: `Could not parse props information for the located at ${entry.componentPath}`,
- }
+ ? {
+ message: `Could not parse props information for the component file located at ${entry.componentPath}`,
+ }
: undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const error = !docgen | |
| ? { | |
| message: `Could not parse props information for the located at ${entry.componentPath}`, | |
| } | |
| : undefined; | |
| const error = !docgen | |
| ? { | |
| message: `Could not parse props information for the component file located at ${entry.componentPath}`, | |
| } | |
| : undefined; |
🤖 Prompt for AI Agents
In code/renderers/react/src/componentManifest/generator.ts around lines 100 to
104, the error message contains a typo ("the located at") and is unclear about
the target; update the message to correct grammar and explicitly reference the
component/file path (e.g. "Could not parse props information for the component
located at ${entry.componentPath}." or "for the file located at
${entry.filePath}."), leaving the rest of the logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (1)
13-15: Use public CsfFile getter instead of private_meta.Replace
csf._meta?.componentwithcsf.meta?.componentto avoid coupling to internals.- .map((name) => getCodeSnippet(csf, name, csf._meta?.component ?? 'ComponentTitle')) + .map((name) => getCodeSnippet(csf, name, csf.meta?.component ?? 'ComponentTitle'))code/renderers/react/src/componentManifest/generateCodeSnippet.ts (2)
14-17: Avoid throwing a string; always throw an Error/SyntaxError.Ensure consistent error objects when story is missing.
- if (!storyDeclaration) { - const message = 'Expected story to be a function or variable declaration'; - throw csf._storyPaths[storyName]?.buildCodeFrameError(message) ?? message; - } + if (!storyDeclaration) { + const message = 'Expected story to be a function or variable declaration'; + const err = csf._storyPaths[storyName]?.buildCodeFrameError(message); + throw err ?? new SyntaxError(message); + }
208-213: ValidatecomponentNamefor a safe JSX identifier.Guard against invalid identifiers (e.g., names with dots/spaces) to avoid generating invalid JSX. Fallback to a safe default.
- const name = t.jsxIdentifier(componentName); + const safeName = t.isValidIdentifier(componentName) ? componentName : 'Component'; + const name = t.jsxIdentifier(safeName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx(7 hunks)code/renderers/react/src/componentManifest/generateCodeSnippet.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.tscode/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to **/*.{ts,tsx,js,jsx,mjs} : Export functions from modules when they need to be unit-tested
Applied to files:
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx
🧬 Code graph analysis (2)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (2)
code/core/src/csf-tools/CsfFile.ts (2)
CsfFile(281-1042)meta(964-966)code/renderers/react/src/componentManifest/utils.ts (1)
invariant(15-23)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (1)
code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
getCodeSnippet(6-225)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (7)
code/renderers/react/src/componentManifest/generateCodeSnippet.test.tsx (6)
79-87: LGTM: error is surfaced with codeframe.Good assertion on the thrown SyntaxError with location context.
100-107: LGTM: CSF4 zero‑arg factory support.Covers
meta.story()and expected fallback rendering.
219-239: LGTM: meta‑level render precedence.Validates merging and inlining from
meta.render+ story args.
284-288: LGTM: block‑body transform snapshot.Confirms JSX transform inside function block preserves structure.
543-548: LGTM: multiple spreads scenario.Ordering and de‑duplication look correct.
550-569: LGTM: top‑level export functions.Keeps
functionform and correctly inlines args.code/renderers/react/src/componentManifest/generateCodeSnippet.ts (1)
116-120: Review comment is incorrect—BabelFile already has the correctpathproperty.The local
BabelFileinterface incode/core/src/csf-tools/CsfFile.ts(lines 31–39) includespath: NodePath<t.Program>, which matches the expected type forpathForNode's first parameter. The call site at line 116 (pathForNode(csf._file.path, metaObj)) is correctly typed. No refactoring or type widening is required.Likely an incorrect or invalid review comment.
| invariant( | ||
| init.isExpression(), | ||
| () => | ||
| storyDeclaration.buildCodeFrameError('Expected story initializer to be an expression') | ||
| .message | ||
| ); | ||
| storyPath = init; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Preserve SyntaxError + codeframe by throwing the Babel error directly (not via invariant).
Using invariant(...messageFromBuildCodeFrameError...) downgrades to Error and can drop SyntaxError typing. Throw the codeframe error object directly to align with the PR goal of better error surfacing.
- invariant(
- init.isExpression(),
- () =>
- storyDeclaration.buildCodeFrameError('Expected story initializer to be an expression')
- .message
- );
+ if (!init.isExpression()) {
+ throw storyDeclaration.buildCodeFrameError('Expected story initializer to be an expression');
+ }
...
- invariant(
- args.length === 1,
- () => storyPath.buildCodeFrameError('Could not evaluate story expression').message
- );
+ if (args.length !== 1) {
+ throw storyPath.buildCodeFrameError('Could not evaluate story expression');
+ }
const storyArg = args[0];
- invariant(
- storyArg.isExpression(),
- () => storyPath.buildCodeFrameError('Could not evaluate story expression').message
- );
+ if (!storyArg.isExpression()) {
+ throw storyPath.buildCodeFrameError('Could not evaluate story expression');
+ }Also applies to: 65-75
🤖 Prompt for AI Agents
In code/renderers/react/src/componentManifest/generateCodeSnippet.ts around
lines 25-31 (and similarly 65-75), replace the invariant(...) pattern that
passes storyDeclaration.buildCodeFrameError(...).message with directly throwing
the Babel/codeframe Error object returned by
storyDeclaration.buildCodeFrameError(...). Specifically, when init is not an
expression (and the other similar check), call const err =
storyDeclaration.buildCodeFrameError('...'); throw err; so the original
SyntaxError/codeframe type and stack are preserved instead of converting to a
plain Error via invariant.
* add GET handler to /mcp path * merge changelogs * update manifest type to match upcoming manifest changes in storybookjs/storybook#32855 * add changesets
kasperpeulen
merged commit b390162
into
10.1-with-canary-release
2 participants
Closes #32848
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-32855-sha-350cba2a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-32855-sha-350cba2a sandboxor in an existing project withnpx storybook@0.0.0-pr-32855-sha-350cba2a upgrade.More information
0.0.0-pr-32855-sha-350cba2akasper/error-manifest350cba2a1761660872)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=32855Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores