React: Implement manifests/component.json for React #32751
Conversation
Co-authored-by: Jeppe Reinhold <[email protected]>
Co-authored-by: Jeppe Reinhold <[email protected]>
Co-authored-by: Jeppe Reinhold <[email protected]>
Co-authored-by: Jeppe Reinhold <[email protected]>
…r/code-snippets # Conflicts: # code/core/src/core-server/dev-server.ts
![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
♻️ Duplicate comments (1)
code/core/src/types/modules/core-common.ts (1)
343-351: RenamejsDocTags→jsdocTag?and align to generator output (arrays).
Generator/tests emitjsdocTag(singular) with array values; current type is plural and differently cased. This breakssatisfiesand narrows. Apply the rename and make it optional.Apply this diff:
export interface ComponentManifest { id: string; name: string; description?: string; import?: string; summary?: string; examples: { name: string; snippet: string }[]; - jsDocTags: Record<string, string[]>; + jsdocTag?: Record<string, string[]>; }Note: You have both top-level
import/summaryand tags that may include@import/@summary. Please confirm single source of truth to avoid duplication drift.Run to verify repo-wide usage:
#!/bin/bash set -euo pipefail # Show all variants and nearby context rg -nC2 -P '\bjs[Dd]ocTags\b|\bjsdocTags?\b|\bjsdocTag\b' --type ts --type tsx --type js --type jsx
🧹 Nitpick comments (3)
code/core/src/types/modules/core-common.ts (3)
353-356: Optionally lock manifest protocol version.
Consider narrowingvto a literal (e.g.,1) to make breaking changes explicit at the type level.Example:
-export interface ComponentsManifest { - v: number; +export interface ComponentsManifest { + v: 1; components: Record<string, ComponentManifest>; }
375-375: Clarify exposure: internal vs public config.
componentManifestGeneratoris onStorybookConfigRawonly. If meant to be internal (preset-provided), add/** @internal */to prevent external use; if user-configurable, mirror it inStorybookConfigasPresetValue<...>.
474-474: Feature flag addition looks good; document semantics.
Add a short JSDoc explaining what enablingexperimentalComponentsManifestdoes and default behavior in dev/build. Verify it gates all routes/outputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/types/modules/core-common.ts(4 hunks)code/renderers/react/src/componentManifest/generator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/renderers/react/src/componentManifest/generator.ts
🧰 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/core/src/types/modules/core-common.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/core/src/types/modules/core-common.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/core/src/types/modules/core-common.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/core/src/types/modules/core-common.ts
🧬 Code graph analysis (1)
code/core/src/types/modules/core-common.ts (1)
code/core/src/core-server/index.ts (1)
StoryIndexGenerator(11-11)
⏰ 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/core/src/types/modules/core-common.ts (2)
3-3: LGTM: type-only import avoids runtime deps.
import { type StoryIndexGenerator }is correct and tree-shakeable.
358-361: LGTM: generator type shape is clear and minimal.
Signature matches usage: takesStoryIndexGenerator, returnsPromise<ComponentsManifest>.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/lib/cli-storybook/src/automigrate/multi-project.test.ts (1)
10-27: Addspy: trueoption tovi.mock()call.The mock should include the
spy: trueoption to align with the project's testing guidelines.As per coding guidelines.
Apply this diff:
-vi.mock('storybook/internal/node-logger', async (importOriginal) => { +vi.mock('storybook/internal/node-logger', { spy: true }, async (importOriginal) => { return { ...(await importOriginal<typeof import('storybook/internal/node-logger')>()), prompt: { multiselect: vi.fn(), error: vi.fn(), }, logger: { log: vi.fn(), error: vi.fn(), debug: vi.fn(), SYMBOLS: { success: '✔', error: '✕', }, }, }; });code/core/src/csf-tools/CsfFile.ts (1)
699-705: Support ObjectMethod properties for renamed exports.
export { story as A }withconst story = { play({}) {}, render(args) {} }drops methods (p.valueis undefined). HandleObjectMethod.- if (t.isObjectExpression(storyNode)) { - (storyNode.properties as t.ObjectProperty[]).forEach((p) => { - if (t.isIdentifier(p.key)) { - annotations[p.key.name] = p.value; - } - }); - } + if (t.isObjectExpression(storyNode)) { + storyNode.properties.forEach((p) => { + if (t.isObjectMethod(p) && t.isIdentifier(p.key)) { + annotations[p.key.name] = p; + } else if (t.isObjectProperty(p) && t.isIdentifier(p.key)) { + annotations[p.key.name] = p.value; + } + }); + }code/addons/vitest/src/updateVitestFile.ts (1)
141-171: Setupdated = truewhen adding imports/vars, and harden dedupe (avoid specifiers[0] crash, handle all specifiers and module sources).Actual code confirms: added imports/vars don't flip
updated, so callers may skip writing changes. Usingspecifiers[0]crashes on side‑effect imports (import 'foo'). The first section (141-171) also lacks module source comparison while the second already has it. Multi-specifier variable declarations only check the first declarator.Apply consolidated patch:
Lines 141-171:
if (sourceNode.type === 'ImportDeclaration') { - // Insert imports that don't already exist (according to their local specifier name) + // Insert imports that don't already exist for the same module+locals. - if ( - !target.program.body.some( - (targetNode) => - targetNode.type === sourceNode.type && - targetNode.specifiers.some((s) => s.local.name === sourceNode.specifiers[0].local.name) - ) + const exists = target.program.body.some((targetNode) => { + if (targetNode.type !== 'ImportDeclaration') return false; + if (targetNode.source.value !== sourceNode.source.value) return false; + if (sourceNode.specifiers.length === 0) { + // Side-effect import: match only if target also has side-effect from same module. + return targetNode.specifiers.length === 0; + } + const sourceLocals = new Set(sourceNode.specifiers.map((s) => s.local.name)); + return targetNode.specifiers.some((s) => sourceLocals.has(s.local.name)); + }); + if (!exists) { const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration'); target.program.body.splice(lastImport + 1, 0, sourceNode); + updated = true; } } else if (sourceNode.type === 'VariableDeclaration') { - // Copy over variable declarations, making sure they're inserted after any imports + // Copy over variable declarations (after imports). Handle all declarators. + const sourceIds = sourceNode.declarations + .map((d) => (d.id.type === 'Identifier' ? d.id.name : undefined)) + .filter(Boolean); - if ( - !target.program.body.some( - (targetNode) => - targetNode.type === sourceNode.type && - targetNode.declarations.some( - (d) => - 'name' in d.id && - 'name' in sourceNode.declarations[0].id && - d.id.name === sourceNode.declarations[0].id.name - ) - ) + const exists = target.program.body.some( + (targetNode) => + targetNode.type === 'VariableDeclaration' && + targetNode.declarations.some( + (d) => d.id.type === 'Identifier' && sourceIds.includes(d.id.name) + ) ) { const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration'); target.program.body.splice(lastImport + 1, 0, sourceNode); + updated = true; } }Lines 306-335:
if (sourceNode.type === 'ImportDeclaration') { - // Insert imports that don't already exist + // Insert imports that don't already exist (handle all specifiers, including side-effects). + const exists = target.program.body.some((targetNode) => { + if (targetNode.type !== 'ImportDeclaration') return false; + if (targetNode.source.value !== sourceNode.source.value) return false; + if (sourceNode.specifiers.length === 0) { + return targetNode.specifiers.length === 0; + } + const sourceLocals = new Set(sourceNode.specifiers.map((s) => s.local.name)); + return targetNode.specifiers.some((s) => sourceLocals.has(s.local.name)); + }); - if ( - !target.program.body.some( - (targetNode) => - targetNode.type === sourceNode.type && - targetNode.source.value === sourceNode.source.value && - targetNode.specifiers.some((s) => s.local.name === sourceNode.specifiers[0].local.name) - ) + if (!exists) { const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration'); target.program.body.splice(lastImport + 1, 0, sourceNode); + updated = true; } } else if (sourceNode.type === 'VariableDeclaration') { - // Copy over variable declarations, making sure they're inserted after any imports + // Copy over variable declarations (after imports). Handle all declarators. + const sourceIds = sourceNode.declarations + .map((d) => (d.id.type === 'Identifier' ? d.id.name : undefined)) + .filter(Boolean); - if ( - !target.program.body.some( - (targetNode) => - targetNode.type === sourceNode.type && - targetNode.declarations.some( - (d) => - 'name' in d.id && - 'name' in sourceNode.declarations[0].id && - d.id.name === sourceNode.declarations[0].id.name - ) - ) + const exists = target.program.body.some( + (targetNode) => + targetNode.type === 'VariableDeclaration' && + targetNode.declarations.some( + (d) => d.id.type === 'Identifier' && sourceIds.includes(d.id.name) + ) ) { const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration'); target.program.body.splice(lastImport + 1, 0, sourceNode); + updated = true; } }Array.findLastIndex is supported (Node ≥ 18; your project requires ≥ 22.0.0).
🧹 Nitpick comments (34)
README.md (1)
1-243: Consider documenting new experimental component manifest features in the README.The PR introduces experimental component manifest generation for React (experimentalComponentsManifest flag, manifest generation flow, JSDoc extraction, prop type extraction). While the Jest addon link update is correct, the root README could benefit from mentioning or pointing to these new capabilities, even if marked as experimental. This would help users discover and adopt the feature.
You may want to verify whether component manifest features should be mentioned in the root README, or if they're intentionally documented only in the React renderer's package README or a separate changelog.
code/core/src/node-logger/prompts/prompt-provider-clack.ts (1)
56-59: Redundant object spread pattern; verify necessity.The pattern
{ ...options, required: options.required }is redundant since spreadingoptionsalready includes therequiredproperty. This differs from other methods in the class (text,confirm,select) which pass options directly to their respective clack functions.Unless the
clack.multiselectAPI has specific type constraints requiring this pattern, consider simplifying to:- const result = await clack.multiselect<T>({ - ...options, - required: options.required, - }); + const result = await clack.multiselect<T>(options);If this wrapping is intentional (e.g., for type compatibility with the clack library), please verify and consider adding a comment explaining why
multiselectrequires different handling than other prompt methods.code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
126-134: Improve type safety for mergeConfig argument handling.The cast
arg as CallExpressionon line 132 is unsafe becausemergeConfigarguments can be object literals, other expressions, or call expressions. WhileisSafeToExtendWorkspacestarts with anisCallExpressioncheck that will gracefully reject non-CallExpression arguments, the cast is misleading.Apply this diff to add explicit type checking before validation:
ExportDefaultDeclaration(path) { if (isDefineConfigExpression(path.node.declaration)) { isValidVitestConfig = isSafeToExtendWorkspace(path.node.declaration as CallExpression); } else if (isMergeConfigExpression(path.node.declaration)) { // the config could be anywhere in the mergeConfig call, so we need to check each argument const mergeCall = path.node.declaration as CallExpression; isValidVitestConfig = mergeCall.arguments.some((arg) => - isSafeToExtendWorkspace(arg as CallExpression) + isCallExpression(arg) && isSafeToExtendWorkspace(arg) ); } },code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
11-16: Consider updating mock pattern to follow coding guidelines.The mock is missing the
spy: trueoption and mock behaviors should be inbeforeEachblocks per coding guidelines. Since line 13 is the only addition and the rest is pre-existing, this refactor is optional but would align the test with project standards.As per coding guidelines.
Example refactor:
-vi.mock('storybook/internal/node-logger', () => ({ - logger: { - log: vi.fn(), - warn: vi.fn(), - }, -})); +vi.mock('storybook/internal/node-logger', { spy: true });Then add in the test suite or individual tests:
beforeEach(() => { vi.mocked(logger.log).mockClear(); vi.mocked(logger.warn).mockClear(); });code/core/src/components/components/syntaxhighlighter/clipboard.ts (1)
26-28: Consider aligning detection logic with actual usage.The detection checks only
globalWindow.navigator?.clipboard, butcopyUsingClipboardAPItriesglobalWindow.top?.navigator.clipboardfirst. While the try-catch handles edge cases, aligning the detection with the actual priority would be more consistent.Consider this adjustment if you want stricter alignment:
export function createCopyToClipboardFunction() { - return globalWindow.navigator?.clipboard ? copyUsingClipboardAPI : copyUsingWorkAround; + return (globalWindow.top?.navigator?.clipboard || globalWindow.navigator?.clipboard) + ? copyUsingClipboardAPI + : copyUsingWorkAround; }MIGRATION.md (3)
514-529: Clarify ESM guidance for projects without "type":"module"Many users will hit ESM loading issues unless you call out using .mjs or setting "type":"module". Suggest adding a sentence here.
#### The `.storybook/main.*` file and other presets must be valid ESM -Storybook will load the `.storybook/main.*` file and any custom preset files as ESM files. +Storybook will load the `.storybook/main.*` file and any custom preset files as ESM files. +If your project does not set `"type": "module"` in `package.json`, use a `.mjs` extension for JavaScript-based config/preset files (or set `"type": "module"`).
533-547: Extensionless imports note: add link to TS moduleResolution requirementGood callout. Since the section below requires moduleResolution that supports
types, add a cross‑reference to reduce confusion.-Additionally, **extensionless relative imports are no longer supported** in JavaScript-based configuration files (`.storybook/main.js`) and custom presets. All relative imports must now include explicit file extensions. +Additionally, **extensionless relative imports are no longer supported** in JavaScript-based configuration files (`.storybook/main.js`) and custom presets. All relative imports must now include explicit file extensions. For TypeScript projects, see the “Require tsconfig.json moduleResolution…” section below for compatible settings.
549-553: Consider mentioning .mts/.cts and when to prefer .mjs vs .jsNode’s TS ESM/CJS file extensions are commonly used. Adding them improves completeness.
**Recommended approach for all files:** -- Use `.js` for JavaScript files -- Use `.mjs` for ES modules -- Use `.ts` for TypeScript files +- Use `.js` for JavaScript files when `"type": "module"` is set +- Use `.mjs` for ES modules when not using `"type": "module"` +- Use `.ts` for TypeScript files (or `.mts`/`.cts` when you opt into Node’s ESM/CJS-aware TS extensions) - Always include the extension in relative importscode/lib/cli-storybook/src/add.ts (1)
181-187: Reloading main with skipCache is right; add guard + debug log to avoid silent failures
- Guard previewConfigPath instead of forcing non-null.
- Log at debug level in catch (don’t swallow everything).
Apply:
// TODO: remove try/catch once CSF factories is shipped, for now gracefully handle any error try { - const newMainConfig = await loadMainConfig({ configDir, skipCache: true }); - await syncStorybookAddons(newMainConfig, previewConfigPath!, configDir); + const newMainConfig = await loadMainConfig({ configDir, skipCache: true }); + if (previewConfigPath) { + await syncStorybookAddons(newMainConfig, previewConfigPath, configDir); + } } catch (e) { - // + // Intentionally non-fatal; surface for troubleshooting + if (logger?.debug) logger.debug(`Addon sync skipped due to error: ${e instanceof Error ? e.message : String(e)}`); }Also consider defaulting the logger param to Storybook’s node-logger to avoid console defaults, per coding guidelines.
code/core/src/shared/utils/module.ts (1)
75-77: Minor: be defensive when invalidating require cache
require.resolve(path)can throw; guard before delete to avoid masking the original import error flow.Apply:
- if (skipCache) { - delete require.cache[require.resolve(path)]; - } + if (skipCache) { + try { + const key = require.resolve(path); + if (require.cache[key]) delete require.cache[key]; + } catch {} + }code/core/src/common/utils/load-main-config.ts (2)
34-37: Broaden ESM error detection to cover Node variantsMatch Node’s
ERR_REQUIRE_ESMand similar messages to make the fallback more reliable across versions.Apply:
- if (e.message.includes('not defined in ES module scope')) { + const err = e as any; + if ( + (typeof err?.message === 'string' && err.message.includes('not defined in ES module scope')) || + err?.code === 'ERR_REQUIRE_ESM' + ) { logger.info( 'Loading main config failed as the file does not seem to be valid ESM. Trying a temporary fix, please ensure the main config is valid ESM.' );
53-56: Nit: avoid double dot in tmp filenameUsing
${name}.tmp.${ext}yields e.g.main.tmp..js. Prefer${name}.tmp${ext}.Apply:
- const modifiedMainPath = join(dir, `${name}.tmp.${ext}`); + const modifiedMainPath = join(dir, `${name}.tmp${ext}`);code/core/src/manager-api/modules/stories.ts (1)
392-404: Mirror the same safety injumpToStory.Optional-chain
refs[story.refId]to avoid rare NPEs when a ref is unset/racing.- const hash = story.refId ? refs[story.refId].filteredIndex : filteredIndex; + const hash = story.refId ? refs[story.refId]?.filteredIndex : filteredIndex;code/core/src/manager-api/tests/stories.test.ts (1)
1109-1147: Good coverage for filteredIndex-first-visible-child selection.Test clearly proves the new behavior. Minor robustness nit: since
store.setStateis async (returns a Promise), awaiting it removes any timing flake, even if the mock updates synchronously.- store.setState({ filteredIndex }); + await store.setState({ filteredIndex });code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (2)
223-264: Nice coverage for pattern stripping; add a block‑comment case and a regex‑literal case.Add two tests:
- Verify
/* ... __dirname ... */is ignored.- Verify
const r = /https?:\/\/.*/;doesn’t break line‑comment stripping (false negatives).
300-332: Add negative tests to prevent false positives in hasDirnameDefined.hasDirnameDefined currently matches inside comments/strings. Add tests like:
// const __dirname = ...const s = "var __dirname = x";
This will catch regressions once implementation is hardened.code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
278-281: Add tests for containsFilenameUsage.Parity with containsDirnameUsage improves confidence.
298-303: Banner detection is fine; consider trimming/normalizing whitespace if content may be formatted.Low priority.
CHANGELOG.prerelease.md (1)
9-12: Copy edits: fix minor typos.
- “into it's own repository” → “into its own repository”
- “Supress” → “Suppress”
- - Move: Addon jest into it's own repository - [#32646](...) + - Move: Addon jest into its own repository - [#32646](...) - - CLI: Supress npm notice update log messages - [#31334](...) + - CLI: Suppress npm notice update log messages - [#31334](...)code/core/src/csf-tools/CsfFile.test.ts (1)
1825-1855: Play/mount ObjectMethod tests LGTM; consider meta+mount method case.These validate ObjectMethod plays and mount detection well. Consider adding one more for
export default { play({ mount }) {} }to assert meta-level mount=true on stories.Also applies to: 1857-1884, 1950-1980
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
8-12: Use vi.mock with spy: true and hoist pattern per guidelines.Adopt type‑safe spying with original module and
spy: true. This keeps native behavior unless overridden and matches our test guidelines.-vi.mock('node:fs/promises', async (importOriginal) => ({ - ...(await importOriginal<typeof import('node:fs/promises')>()), - readFile: vi.fn(), - writeFile: vi.fn(), -})); +vi.mock('node:fs/promises', async () => { + const mod = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises'); + return { + ...mod, + readFile: vi.fn(), + writeFile: vi.fn(), + }; +}, { spy: true });As per coding guidelines
281-297: Read last write for resilience (multiple writes possible).updateMainConfig may write once and banner prepend writes again. Assert against the final call.
-const writtenContent = mockWriteFile.mock.calls[0][1]; +const writtenContent = mockWriteFile.mock.calls.at(-1)?.[1] as string; +expect(writtenContent).toBeDefined();Apply to all similar places. As per coding guidelines
Also applies to: 325-341, 383-397, 423-434
code/core/src/csf-tools/CsfFile.ts (1)
178-183: Extend hasMount to handle FunctionExpression.ObjectMethod support is great. Include
FunctionExpressionto coverplay: function({ mount }) {}.- if ( - t.isArrowFunctionExpression(play) || - t.isFunctionDeclaration(play) || - t.isObjectMethod(play) - ) { + if ( + t.isArrowFunctionExpression(play) || + t.isFunctionDeclaration(play) || + t.isFunctionExpression(play) || + t.isObjectMethod(play) + ) {code/addons/vitest/src/updateVitestFile.ts (5)
9-16: Template replacements only replace the first occurrence.
String.prototype.replacereplaces one match. Use a global replacement.- Object.entries(replacements).forEach(([key, value]) => (template = template.replace(key, value))); + Object.entries(replacements).forEach(([key, value]) => { + // robust for literals without regex escaping + template = template.split(key).join(value); + });
20-52: Object merge misses StringLiteral keys and SpreadElements; arrays may duplicate entries.
- Keys quoted in configs (
'test','plugins') won’t match because only Identifiers are compared.- Spread elements (
...foo) are skipped entirely.- Array concatenation can duplicate items (e.g., plugins).
Minimal key handling improvement:
const mergeProperties = ( source: t.ObjectExpression['properties'], target: t.ObjectExpression['properties'] ) => { for (const sourceProp of source) { - if (sourceProp.type === 'ObjectProperty') { - const targetProp = target.find( - (p) => - sourceProp.key.type === 'Identifier' && - p.type === 'ObjectProperty' && - p.key.type === 'Identifier' && - p.key.name === sourceProp.key.name - ); + if (sourceProp.type === 'ObjectProperty') { + const srcKey = + sourceProp.key.type === 'Identifier' + ? sourceProp.key.name + : sourceProp.key.type === 'StringLiteral' + ? sourceProp.key.value + : undefined; + const targetProp = target.find( + (p) => + p.type === 'ObjectProperty' && + (p.key.type === 'Identifier' ? p.key.name : p.key.type === 'StringLiteral' ? p.key.value : undefined) === + srcKey + ); if (targetProp && targetProp.type === 'ObjectProperty') { if ( sourceProp.value.type === 'ObjectExpression' && targetProp.value.type === 'ObjectExpression' ) { mergeProperties(sourceProp.value.properties, targetProp.value.properties); } else if ( sourceProp.value.type === 'ArrayExpression' && targetProp.value.type === 'ArrayExpression' ) { - targetProp.value.elements.push(...sourceProp.value.elements); + targetProp.value.elements.push(...sourceProp.value.elements); } else { targetProp.value = sourceProp.value; } } else { target.push(sourceProp); } } + // Preserve spread properties + if (sourceProp.type === 'SpreadElement') { + target.push(sourceProp); + } } };Optional: consider de-duplication for well-known arrays (e.g., by stringifying items or using a Set on identifiers).
82-108: Pre-checks: broaden function-notation guard and avoid over-restricting source shape.
- Guard both ArrowFunctionExpression and FunctionExpression for
defineConfig(() => ...).- Requiring the source default export to be a CallExpression is strict; if templates ever export a plain object, we’ll bail unnecessarily.
Suggested tweak for the guard:
- if ( - targetExportDefault.declaration.type === 'CallExpression' && - targetExportDefault.declaration.callee.type === 'Identifier' && - targetExportDefault.declaration.callee.name === 'defineConfig' && - targetExportDefault.declaration.arguments.length > 0 && - targetExportDefault.declaration.arguments[0].type === 'ArrowFunctionExpression' - ) { + if ( + targetExportDefault.declaration.type === 'CallExpression' && + targetExportDefault.declaration.callee.type === 'Identifier' && + targetExportDefault.declaration.callee.name === 'defineConfig' && + targetExportDefault.declaration.arguments.length > 0 && + (targetExportDefault.declaration.arguments[0].type === 'ArrowFunctionExpression' || + targetExportDefault.declaration.arguments[0].type === 'FunctionExpression') + ) { return false; }
109-139: Handle mergeConfig with plain ObjectExpression args (not just defineConfig).Many configs use
export default mergeConfig(base, { test: {...} }). Support ObjectExpression args too.} else if ( targetExportDefault.declaration.type === 'CallExpression' && targetExportDefault.declaration.callee.type === 'Identifier' && targetExportDefault.declaration.callee.name === 'mergeConfig' && targetExportDefault.declaration.arguments.length >= 2 ) { - const defineConfigNodes = targetExportDefault.declaration.arguments.filter( + const defineConfigNodes = targetExportDefault.declaration.arguments.filter( (arg): arg is t.CallExpression => arg?.type === 'CallExpression' && arg.callee.type === 'Identifier' && arg.callee.name === 'defineConfig' && arg.arguments[0]?.type === 'ObjectExpression' ); - canHandleConfig = defineConfigNodes.length > 0; + const objectArgs = targetExportDefault.declaration.arguments.filter( + (arg): arg is t.ObjectExpression => arg?.type === 'ObjectExpression' + ); + canHandleConfig = defineConfigNodes.length > 0 || objectArgs.length > 0; }
151-153: findLastIndex availability across Node/TS lib.If your TS config doesn’t include ES2023 libs or CI uses an older Node,
findLastIndexcan error. Consider:
- Ensure tsconfig includes
"lib": ["es2023"], or- Replace with a small helper:
const lastImport = (() => { for (let i = target.program.body.length - 1; i >= 0; i--) { if (target.program.body[i].type === 'ImportDeclaration') return i; } return -1; })();Also applies to: 168-170, 316-318, 333-335
code/addons/vitest/src/updateVitestFile.test.ts (6)
10-16: Vitest mocking policy: add spy: true, move behaviors to beforeEach, and use vi.mocked().Current mocks don’t use spy: true and set implementations at module scope. Align with the repo’s test-mocking rules for stability and type-safety. Also prefer not relying on hoisting order by placing mocks before SUT imports.
Apply this refactor (also adds typed access and resets between tests):
-import { describe, expect, it, vi } from 'vitest'; +import { describe, expect, it, vi, beforeEach } from 'vitest'; @@ -import { loadTemplate, updateConfigFile, updateWorkspaceFile } from './updateVitestFile'; +// Mocks first to avoid relying on hoisting order vi.mock('storybook/internal/node-logger', () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), }, -})); +}), { spy: true }); + +// Typed access to mocked exports +import * as NodeLogger from 'storybook/internal/node-logger'; +const mockedLogger = vi.mocked(NodeLogger.logger); + +beforeEach(() => { + vi.clearAllMocks(); + // Optionally configure logger behaviors here if needed, e.g. mockedLogger.info.mockImplementation(...) +}); + +import { loadTemplate, updateConfigFile, updateWorkspaceFile } from './updateVitestFile';Based on learnings
18-20: ESM-safe mock for resolvePackageDir + spy: true and vi.mocked().Avoid __dirname in ESM tests; compute dirname from import.meta.url. Also expose a typed handle to adjust behavior per test.
-vi.mock('../../../core/src/shared/utils/module', () => ({ - resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')), -})); +import { fileURLToPath } from 'node:url'; +const DIRNAME = join(fileURLToPath(new URL('.', import.meta.url)), '..'); + +vi.mock('../../../core/src/shared/utils/module', () => ({ + resolvePackageDir: vi.fn(), +}), { spy: true }); + +import * as ModuleUtils from '../../../core/src/shared/utils/module'; +const mockedModuleUtils = vi.mocked(ModuleUtils); + +beforeEach(() => { + mockedModuleUtils.resolvePackageDir.mockReturnValue(DIRNAME); +});Please confirm your Vitest version supports the third argument
{ spy: true }onvi.mock. If not, I can adjust the suggestion to the supported API. Based on learnings
46-57: Nice pattern: before/after + diff. Consider de-duplicating via a small helper.The repeated “generate→update→diff snapshot” boilerplate is solid but verbose. Extract a tiny helper to reduce noise and ease future updates.
Example helper:
async function expectConfigUpdate(tmpl: string, repl: Record<string,string>, targetSrc: string, expectUpdated: boolean, snapshot: string) { const source = babel.babelParse(await loadTemplate(tmpl, repl)); const target = babel.babelParse(targetSrc); const before = babel.generate(target).code; const updated = updateConfigFile(source, target); expect(updated).toBe(expectUpdated); const after = babel.generate(target).code; expect(after === before).toBe(!expectUpdated); expect(getDiff(before, after)).toMatchInlineSnapshot(snapshot); }Call this helper in tests to DRY them up.
Also applies to: 120-131, 227-238, 302-313, 376-387, 447-458, 524-535, 601-612, 675-686, 745-756, 798-809
62-93: Snapshot brittleness: consider trimming doc/comment lines from assertions.Long comments/URLs in injected blocks can cause churn. Consider normalizing
getDiffoutput (e.g., stripping comment-only lines) before snapshotting, or snapshot only key tokens (imports, keys, plugin name).I can provide a small
normalizeDiff()helper to filter comments and blank lines prior to snapshot.Also applies to: 241-274, 392-424, 463-494, 551-576, 629-647, 703-726, 764-779, 817-833
18-20: Optional: Assert call contract of resolvePackageDir.To guard against regressions, assert that the mocked
resolvePackageDirwas called with'@storybook/addon-vitest'.Example:
expect(mockedModuleUtils.resolvePackageDir).toHaveBeenCalledWith('@storybook/addon-vitest');
1-8: Static imports order: keep node built-ins together and add node:url near node:path.Minor readability nit; colocate
node:urlwithnode:pathat the top.-import { join } from 'node:path'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url';
| const require = createRequire(import.meta.url); | ||
| require.resolve(`${addon}/preview`, { paths: [configDir] }); |
There was a problem hiding this comment.
🧩 Analysis chain
Resolution strategy looks correct; verify paths option behavior.
The switch to createRequire-based resolution is well-implemented. The paths: [configDir] option ensures addons are resolved relative to the user's configuration directory, which is essential for the manifest generation workflow.
Run this script to verify the resolution works correctly with the paths option in various addon scenarios:
🏁 Script executed:
#!/bin/bash
# Description: Verify addon resolution logic handles core/non-core addons and missing previews correctly
# Test 1: Find usages of getAddonAnnotations to understand call sites
echo "=== Call sites of getAddonAnnotations ==="
rg -n -C3 'getAddonAnnotations\s*\(' --type=ts --type=js
# Test 2: Check if there are any tests for this function
echo -e "\n=== Existing tests for getAddonAnnotations ==="
rg -n 'getAddonAnnotations' --glob='**/*.test.*' --glob='**/*.spec.*'
# Test 3: Find examples of addon packages to understand resolution context
echo -e "\n=== Example addon imports to verify resolution ==="
rg -n -C2 '@storybook/addon-\w+/preview' --type=ts --type=js | head -30Length of output: 5323
---
🏁 Script executed:
```shell
#!/bin/bash
# View the complete implementation of getAddonAnnotations to understand the resolution strategy
echo "=== Full implementation of getAddonAnnotations ==="
cat code/core/src/common/utils/get-addon-annotations.ts
echo -e "\n=== Check imports for createRequire ==="
head -20 code/core/src/common/utils/get-addon-annotations.ts
Length of output: 286
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the correct file path for get-addon-annotations
echo "=== Locating get-addon-annotations file ==="
fd -t f 'get-addon-annotations'
echo -e "\n=== Check if file exists and show full content ==="
fd -t f 'get-addon-annotations' -x cat {}
Length of output: 2492
---
🏁 Script executed:
```shell
#!/bin/bash
# Check how sync-main-preview-addons.ts handles the getAddonAnnotations return value
echo "=== Full context of getAddonAnnotations call and usage ==="
sed -n '50,75p' code/core/src/common/utils/sync-main-preview-addons.ts
echo -e "\n=== Check test mocking to understand expected behavior ==="
sed -n '35,70p' code/core/src/common/utils/sync-main-preview-addons.test.ts
Length of output: 2456
Fix resolution logic to match core vs. non-core addon patterns.
The resolution on line 43 does not match the importPath logic. It always tries to resolve ${addon}/preview, but for core addons (where isCoreAddon is true), the importPath is set to just addon without /preview. This causes core addon resolution to fail and return null.
The resolution should conditionally resolve based on the same logic:
- Core addons: resolve
addon - Non-core addons: resolve
${addon}/preview
Update line 42-43 to:
const pathToResolve = data.isCoreAddon ? addon : `${addon}/preview`;
require.resolve(pathToResolve, { paths: [configDir] });🤖 Prompt for AI Agents
In code/core/src/common/utils/get-addon-annotations.ts around lines 42 to 43,
the require.resolve call always uses `${addon}/preview` which mismatches the
importPath logic for core addons; change the resolution to compute pathToResolve
= data.isCoreAddon ? addon : `${addon}/preview` and call
require.resolve(pathToResolve, { paths: [configDir] }) so core addons resolve to
addon and non-core to addon/preview.
| it('Object export with args render method', () => { | ||
| expect( | ||
| parse( | ||
| dedent` | ||
| export default { title: 'foo/bar' }; | ||
| export const A = { | ||
| render(args) {} | ||
| } | ||
| `, | ||
| true | ||
| ) | ||
| ).toMatchInlineSnapshot(` | ||
| meta: | ||
| title: foo/bar | ||
| stories: | ||
| - id: foo-bar--a | ||
| name: A | ||
| parameters: | ||
| __isArgsStory: true | ||
| __id: foo-bar--a | ||
| __stats: | ||
| factory: false | ||
| play: false | ||
| render: true | ||
| loaders: false | ||
| beforeEach: false | ||
| globals: false | ||
| tags: false | ||
| storyFn: false | ||
| mount: false | ||
| moduleMock: false | ||
| `); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Add no-args ObjectMethod render coverage and fix __isArgsStory for methods.
You added args-method coverage. Please also add a case for render() {} (no args). Current parser sets __isArgsStory only for value functions, not for ObjectMethod, so no-args methods will incorrectly report true. Add the test and fix in CsfFile.ts as suggested below.
I'll verify this review comment by examining the test file and the implementation. Let me search the codebase for relevant code.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the CsfFile.test.ts file around the specified lines and look for related test cases
fd -e test.ts CsfFile | head -5
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find the CsfFile.ts implementation file (not test)
fd -e ts -type f CsfFile | grep -v test | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for __isArgsStory references in test file to see all related tests
rg -n "__isArgsStory" code/core/src/csf-tools/CsfFile.test.ts -B5 -A5 | head -100Length of output: 2821
🏁 Script executed:
#!/bin/bash
# Search for the ObjectMethod render logic in CsfFile.ts implementation
rg -n "ObjectMethod\|__isArgsStory" code/core/src/csf-tools/CsfFile.ts -B3 -A3 | head -150Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are existing test cases for no-args render method
rg -n "render\(\s*\)" code/core/src/csf-tools/CsfFile.test.ts -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the broader test context around lines 1426-1458
sed -n '1400,1500p' code/core/src/csf-tools/CsfFile.test.tsLength of output: 2367
🏁 Script executed:
#!/bin/bash
# Find where args story detection logic is implemented
ast-grep --pattern 'function $$$isArgsStory$$$'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for isArgsStory or similar function/logic
rg -n "isArgsStory\|hasArgs\|render.*args" code/core/src/csf-tools/CsfFile.ts -B2 -A2 | head -100Length of output: 0
I need to locate the implementation file and understand the __isArgsStory logic more thoroughly.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search more broadly for CsfFile implementation (may be .js or elsewhere)
find code/core/src/csf-tools -name "CsfFile*" -type f
Length of output: 74
🏁 Script executed:
#!/bin/bash
# Search for where __isArgsStory is assigned or set in the codebase
rg -n "__isArgsStory\s*:" code/core/src/csf-tools/ -B3 -A1 | head -150Length of output: 8728
🏁 Script executed:
#!/bin/bash
# Check broader context around the test area - look for other render tests
sed -n '1350,1480p' code/core/src/csf-tools/CsfFile.test.tsLength of output: 3167
🏁 Script executed:
#!/bin/bash
# Search for how ObjectMethod is handled (look for ObjectMethod references)
rg -n "ObjectMethod\|render\s*\(" code/core/src/csf-tools/ -B2 -A2 | head -200Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the actual story generation/parsing logic
rg -n "stories\s*:" code/core/src/csf-tools/CsfFile.ts -B5 -A5 | head -100Length of output: 326
I need to find the implementation logic in CsfFile.ts that determines __isArgsStory and understand how it handles ObjectMethod vs arrow functions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for hasArg, getParam, or logic checking method parameters
rg -n "hasArg\|getParam\|Object.*Method\|render" code/core/src/csf-tools/CsfFile.ts -B2 -A2 | head -250
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at a larger section of CsfFile.ts to understand the structure
sed -n '1,100p' code/core/src/csf-tools/CsfFile.tsLength of output: 2671
🏁 Script executed:
#!/bin/bash
# Search for where story parameters are set including __isArgsStory
rg -n "parameters\|__isArgsStory" code/core/src/csf-tools/CsfFile.ts -B3 -A3 | head -200Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for function parameter checking logic
rg -n "params\|\.params\|parameters\|\.length" code/core/src/csf-tools/CsfFile.ts -B2 -A2 | head -250Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the actual StaticStory interface or where __isArgsStory gets assigned
ast-grep --pattern 'const $_ = { __isArgsStory: $_ }'Length of output: 107
Let me search more broadly for where __isArgsStory gets set and understand the CsfFile.ts structure better.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get line count and search for isArgsStory (without underscores)
wc -l code/core/src/csf-tools/CsfFile.ts
rg -n "isArgsStory" code/core/src/csf-tools/CsfFile.ts
Length of output: 377
🏁 Script executed:
#!/bin/bash
# Search for story building or metadata construction
rg -n "story\|Story" code/core/src/csf-tools/CsfFile.ts | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where the parse output is generated, especially looking at end of file
tail -300 code/core/src/csf-tools/CsfFile.tsLength of output: 10424
🏁 Script executed:
#!/bin/bash
# Search for ObjectMethod across all csf-tools files
rg -n "ObjectMethod" code/core/src/csf-tools/ -B3 -A3Length of output: 1666
🏁 Script executed:
#!/bin/bash
# Search for where render is checked or analyzed for parameters
rg -n "render\|ObjectProperty\|method" code/core/src/csf-tools/CsfFile.ts -B1 -A1 | head -150Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for where parameter checking happens in story analysis
rg -n "\.params\|\.parameters\|params\.length\|function.*params" code/core/src/csf-tools/ -B2 -A2 | head -200Length of output: 0
I found the key locations. Let me examine the isArgsStory function and the render handling logic more carefully.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the isArgsStory function definition
sed -n '118,160p' code/core/src/csf-tools/CsfFile.ts
Length of output: 1413
🏁 Script executed:
#!/bin/bash
# Get the render handling code around lines 615-650
sed -n '610,660p' code/core/src/csf-tools/CsfFile.tsLength of output: 2481
🏁 Script executed:
#!/bin/bash
# Check the broader context around ObjectMethod handling
sed -n '600,670p' code/core/src/csf-tools/CsfFile.tsLength of output: 3390
🏁 Script executed:
#!/bin/bash
# Look at the section with the default render handling
sed -n '605,635p' code/core/src/csf-tools/CsfFile.tsLength of output: 1711
I can now see the bug clearly. Let me verify the test coverage one more time to confirm the exact gap.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all render() tests to see exact coverage
rg -n "it\(.*render" code/core/src/csf-tools/CsfFile.test.ts | grep -i "object\|method"
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Look for "render()" without args in test file
grep -n "render()" code/core/src/csf-tools/CsfFile.test.tsLength of output: 0
Add ObjectMethod no-args render test case and fix __isArgsStory detection for ObjectMethod render functions.
The review comment is valid. The codebase has a coverage gap and a corresponding bug:
-
Missing test: There's no test for
render() {}(ObjectMethod with no args). Current tests cover:render: () => {}(arrow function, no args) ✓render: (args) => {}(arrow function, with args) ✓render(args) {}(ObjectMethod, with args) ✓- Missing:
render() {}(ObjectMethod, no args)
-
Bug in implementation: The
isArgsStory()function (line 118–148) only checksArrowFunctionExpressionandFunctionDeclaration, but notObjectMethod. When an ObjectMethod render is detected (line 618), the code stores the annotation but never callsisArgsStory(), leaving__isArgsStoryat its default value oftrue(line 613). This meansrender() {}will incorrectly report__isArgsStory: trueinstead offalse.
Add the test case and update isArgsStory() to handle ObjectMethod by checking its params.length > 0.
🤖 Prompt for AI Agents
In code/core/src/csf-tools/CsfFile.test.ts around lines 1426 to 1458 add a test
case for an ObjectMethod render with no args (render() {}) to assert
__isArgsStory: false; then update the implementation: in the isArgsStory()
function (around lines 118–148) include handling for AST nodes of type
ObjectMethod by returning node.params.length > 0, and ensure the code path that
detects ObjectMethod renders (around line 618) calls isArgsStory() so
__isArgsStory is set based on the ObjectMethod params rather than left as the
default at line 613.
| if (t.isIdentifier(p.key)) { | ||
| if (p.key.name === 'render') { | ||
| parameters.__isArgsStory = isArgsStory( | ||
| p.value as t.Expression, | ||
| parent, | ||
| self | ||
| ); | ||
| } else if (p.key.name === 'name' && t.isStringLiteral(p.value)) { | ||
| name = p.value.value; | ||
| } else if (p.key.name === 'storyName' && t.isStringLiteral(p.value)) { | ||
| logger.warn( | ||
| `Unexpected usage of "storyName" in "${exportName}". Please use "name" instead.` | ||
| ); | ||
| } else if (p.key.name === 'parameters' && t.isObjectExpression(p.value)) { | ||
| const idProperty = p.value.properties.find( | ||
| (property) => | ||
| t.isObjectProperty(property) && | ||
| t.isIdentifier(property.key) && | ||
| property.key.name === '__id' | ||
| ) as t.ObjectProperty | undefined; | ||
| if (idProperty) { | ||
| parameters.__id = (idProperty.value as t.StringLiteral).value; | ||
| const key = p.key.name; | ||
| if (t.isObjectMethod(p)) { | ||
| self._storyAnnotations[exportName][key] = p; | ||
| } else { | ||
| if (p.key.name === 'render') { | ||
| parameters.__isArgsStory = isArgsStory( | ||
| p.value as t.Expression, | ||
| parent, | ||
| self | ||
| ); | ||
| } else if (p.key.name === 'name' && t.isStringLiteral(p.value)) { | ||
| name = p.value.value; | ||
| } else if (p.key.name === 'storyName' && t.isStringLiteral(p.value)) { | ||
| logger.warn( | ||
| `Unexpected usage of "storyName" in "${exportName}". Please use "name" instead.` | ||
| ); | ||
| } else if (p.key.name === 'parameters' && t.isObjectExpression(p.value)) { | ||
| const idProperty = p.value.properties.find( | ||
| (property) => | ||
| t.isObjectProperty(property) && | ||
| t.isIdentifier(property.key) && | ||
| property.key.name === '__id' | ||
| ) as t.ObjectProperty | undefined; | ||
| if (idProperty) { | ||
| parameters.__id = (idProperty.value as t.StringLiteral).value; | ||
| } | ||
| } | ||
| self._storyAnnotations[exportName][p.key.name] = p.value; | ||
| } | ||
| self._storyAnnotations[exportName][p.key.name] = p.value; | ||
| } |
There was a problem hiding this comment.
Set __isArgsStory for ObjectMethod render.
For CSF3 object exports using method syntax, __isArgsStory isn’t derived. Fix to use method params length.
- if (t.isIdentifier(p.key)) {
- const key = p.key.name;
- if (t.isObjectMethod(p)) {
- self._storyAnnotations[exportName][key] = p;
- } else {
+ if (t.isIdentifier(p.key)) {
+ const key = p.key.name;
+ if (t.isObjectMethod(p)) {
+ if (key === 'render') {
+ parameters.__isArgsStory = p.params.length > 0;
+ }
+ self._storyAnnotations[exportName][key] = p;
+ } else {
if (p.key.name === 'render') {
parameters.__isArgsStory = isArgsStory(
p.value as t.Expression,
parent,
self
);Follow-up: add a test for render() {} to assert __isArgsStory: false. Based on learnings
📝 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.
| if (t.isIdentifier(p.key)) { | |
| if (p.key.name === 'render') { | |
| parameters.__isArgsStory = isArgsStory( | |
| p.value as t.Expression, | |
| parent, | |
| self | |
| ); | |
| } else if (p.key.name === 'name' && t.isStringLiteral(p.value)) { | |
| name = p.value.value; | |
| } else if (p.key.name === 'storyName' && t.isStringLiteral(p.value)) { | |
| logger.warn( | |
| `Unexpected usage of "storyName" in "${exportName}". Please use "name" instead.` | |
| ); | |
| } else if (p.key.name === 'parameters' && t.isObjectExpression(p.value)) { | |
| const idProperty = p.value.properties.find( | |
| (property) => | |
| t.isObjectProperty(property) && | |
| t.isIdentifier(property.key) && | |
| property.key.name === '__id' | |
| ) as t.ObjectProperty | undefined; | |
| if (idProperty) { | |
| parameters.__id = (idProperty.value as t.StringLiteral).value; | |
| const key = p.key.name; | |
| if (t.isObjectMethod(p)) { | |
| self._storyAnnotations[exportName][key] = p; | |
| } else { | |
| if (p.key.name === 'render') { | |
| parameters.__isArgsStory = isArgsStory( | |
| p.value as t.Expression, | |
| parent, | |
| self | |
| ); | |
| } else if (p.key.name === 'name' && t.isStringLiteral(p.value)) { | |
| name = p.value.value; | |
| } else if (p.key.name === 'storyName' && t.isStringLiteral(p.value)) { | |
| logger.warn( | |
| `Unexpected usage of "storyName" in "${exportName}". Please use "name" instead.` | |
| ); | |
| } else if (p.key.name === 'parameters' && t.isObjectExpression(p.value)) { | |
| const idProperty = p.value.properties.find( | |
| (property) => | |
| t.isObjectProperty(property) && | |
| t.isIdentifier(property.key) && | |
| property.key.name === '__id' | |
| ) as t.ObjectProperty | undefined; | |
| if (idProperty) { | |
| parameters.__id = (idProperty.value as t.StringLiteral).value; | |
| } | |
| } | |
| self._storyAnnotations[exportName][p.key.name] = p.value; | |
| } | |
| self._storyAnnotations[exportName][p.key.name] = p.value; | |
| } | |
| if (t.isIdentifier(p.key)) { | |
| const key = p.key.name; | |
| if (t.isObjectMethod(p)) { | |
| if (key === 'render') { | |
| parameters.__isArgsStory = p.params.length > 0; | |
| } | |
| self._storyAnnotations[exportName][key] = p; | |
| } else { | |
| if (p.key.name === 'render') { | |
| parameters.__isArgsStory = isArgsStory( | |
| p.value as t.Expression, | |
| parent, | |
| self | |
| ); | |
| } else if (p.key.name === 'name' && t.isStringLiteral(p.value)) { | |
| name = p.value.value; | |
| } else if (p.key.name === 'storyName' && t.isStringLiteral(p.value)) { | |
| logger.warn( | |
| `Unexpected usage of "storyName" in "${exportName}". Please use "name" instead.` | |
| ); | |
| } else if (p.key.name === 'parameters' && t.isObjectExpression(p.value)) { | |
| const idProperty = p.value.properties.find( | |
| (property) => | |
| t.isObjectProperty(property) && | |
| t.isIdentifier(property.key) && | |
| property.key.name === '__id' | |
| ) as t.ObjectProperty | undefined; | |
| if (idProperty) { | |
| parameters.__id = (idProperty.value as t.StringLiteral).value; | |
| } | |
| } | |
| self._storyAnnotations[exportName][p.key.name] = p.value; | |
| } | |
| } |
🤖 Prompt for AI Agents
In code/core/src/csf-tools/CsfFile.ts around lines 616 to 646, object-method
style story exports aren't setting parameters.__isArgsStory for a render defined
as an ObjectMethod; when p is an ObjectMethod and p.key.name === 'render'
compute isArgsStory using the method's parameter list (e.g. isArgs =
p.params.length > 0 or by calling isArgsStory-like logic with p.params) and
assign parameters.__isArgsStory accordingly, then continue to store the
annotation in self._storyAnnotations[exportName][key] as before; additionally
add a unit test that parses an object story with render() {} and asserts that
__isArgsStory is false.
| /** Check if the file content contains a pattern matching the given regex */ | ||
| export function containsPatternUsage(content: string, pattern: RegExp): boolean { | ||
| // Remove strings first, then comments | ||
| const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""'); | ||
| const withoutStrings = stripStrings(content); | ||
| const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, ''); | ||
| const cleanContent = withoutBlock | ||
| .split('\n') | ||
| .map((line) => line.split('//')[0]) | ||
| .join('\n'); | ||
|
|
||
| // Check for pattern usage in the cleaned content | ||
| return pattern.test(cleanContent); | ||
| } |
There was a problem hiding this comment.
Make containsPatternUsage resilient to global regexes and regex literals.
- Reset
lastIndexto avoid statefulness with/g. - Optional: strip regex literals before
//processing to avoid false removals on//inside a regex/URL.
export function containsPatternUsage(content: string, pattern: RegExp): boolean {
// Remove strings first, then comments
const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""');
- const withoutStrings = stripStrings(content);
- const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, '');
+ const withoutStrings = stripStrings(content);
+ // Strip regex literals to prevent '//' inside them from being treated as line comments
+ const withoutRegex = withoutStrings.replace(/\/(?![*\/])(?:\\.|[^\/\n])+\/[dgimsuy]*/g, '/*regex*/');
+ const withoutBlock = withoutRegex.replace(/\/\*[\s\S]*?\*\//g, '');
const cleanContent = withoutBlock
.split('\n')
.map((line) => line.split('//')[0])
.join('\n');
// Check for pattern usage in the cleaned content
- return pattern.test(cleanContent);
+ if ((pattern as any).global) (pattern as any).lastIndex = 0;
+ return pattern.test(cleanContent);
}📝 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.
| /** Check if the file content contains a pattern matching the given regex */ | |
| export function containsPatternUsage(content: string, pattern: RegExp): boolean { | |
| // Remove strings first, then comments | |
| const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""'); | |
| const withoutStrings = stripStrings(content); | |
| const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, ''); | |
| const cleanContent = withoutBlock | |
| .split('\n') | |
| .map((line) => line.split('//')[0]) | |
| .join('\n'); | |
| // Check for pattern usage in the cleaned content | |
| return pattern.test(cleanContent); | |
| } | |
| export function containsPatternUsage(content: string, pattern: RegExp): boolean { | |
| // Remove strings first, then comments | |
| const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""'); | |
| const withoutStrings = stripStrings(content); | |
| // Strip regex literals to prevent '//' inside them from being treated as line comments | |
| const withoutRegex = withoutStrings.replace(/\/(?![*\/])(?:\\.|[^\/\n])+\/[dgimsuy]*/g, '/*regex*/'); | |
| const withoutBlock = withoutRegex.replace(/\/\*[\s\S]*?\*\//g, ''); | |
| const cleanContent = withoutBlock | |
| .split('\n') | |
| .map((line) => line.split('//')[0]) | |
| .join('\n'); | |
| // Check for pattern usage in the cleaned content | |
| if ((pattern as any).global) (pattern as any).lastIndex = 0; | |
| return pattern.test(cleanContent); | |
| } |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts around lines
258 to 271, containsPatternUsage is vulnerable to stateful RegExp objects and
mis-parses regex literals when stripping comments; ensure you reset
pattern.lastIndex = 0 before calling pattern.test(cleanContent) to avoid /g
statefulness, and modify the cleaning logic to remove/replace regex literals
(e.g. /.../flags) before you split lines for // comments so that // inside
regexes or URLs aren't treated as line comments; keep the existing string and
block-comment stripping order, then strip regex literals, then do the line-level
// removal, and finally call pattern.test on the cleaned content.
| /** Check if __dirname is already defined in the file */ | ||
| export function hasDirnameDefined(content: string): boolean { | ||
| // Check if __dirname is already defined as a const/let/var | ||
| const dirnameDefinedRegex = /(?:const|let|var)\s+__dirname\s*=/; | ||
| return dirnameDefinedRegex.test(content); | ||
| } |
There was a problem hiding this comment.
Harden hasDirnameDefined to ignore strings/comments.
As written, it can match const __dirname = inside comments/strings, causing skipped fixes and potential runtime errors. Clean content before testing.
Apply minimal fix:
export function hasDirnameDefined(content: string): boolean {
- // Check if __dirname is already defined as a const/let/var
- const dirnameDefinedRegex = /(?:const|let|var)\s+__dirname\s*=/;
- return dirnameDefinedRegex.test(content);
+ // Strip strings and comments to avoid false positives
+ const stripStrings = (s: string) => s.replace(/(['"`])(?:\\.|(?!\1)[\s\S])*?\1/g, '""');
+ const withoutStrings = stripStrings(content);
+ const withoutBlock = withoutStrings.replace(/\/\*[\s\S]*?\*\//g, '');
+ const cleanContent = withoutBlock
+ .split('\n')
+ .map((line) => line.split('//')[0])
+ .join('\n');
+ const dirnameDefinedRegex = /(?:const|let|var)\s+__dirname\s*=/;
+ return dirnameDefinedRegex.test(cleanContent);
}🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts around lines
282 to 287, hasDirnameDefined currently runs a regex against raw file text and
can falsely match occurrences inside comments or string/template literals;
update the function to first strip/neutralize comments (//... and /*...*/) and
all string/template literal contents (single-quoted, double-quoted, and backtick
templates) from the input (e.g., replace them with blanks) and then run the
existing (or same) /\b(?:const|let|var)\s+__dirname\s*=/ regex against the
cleaned content so only real JS declarations are detected. Ensure the cleaning
step is robust enough for minimal cases (escape sequences) and that behavior for
true declarations remains unchanged.
| describe('promptForAutomigrations', () => { | ||
| it('should call multiselect with required: false', async () => { | ||
| const { prompt } = await import('storybook/internal/node-logger'); | ||
| const multiselectMock = vi.mocked(prompt.multiselect); | ||
| multiselectMock.mockResolvedValue(['fix1']); | ||
|
|
||
| const fix1 = createMockFix('fix1', { needsFix: true }); | ||
| const project1 = createMockProject('/project1/.storybook'); | ||
|
|
||
| const automigrations = [ | ||
| { | ||
| fix: fix1, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| await promptForAutomigrations(automigrations, { dryRun: false, yes: false }); | ||
|
|
||
| expect(multiselectMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: 'Select automigrations to run', | ||
| required: false, | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move mock implementations to beforeEach blocks.
Mock implementations like multiselectMock.mockResolvedValue(['fix1']) at line 129 should be placed in beforeEach blocks rather than inline within test cases. This pattern applies to similar implementations throughout the test suite (lines 129, 160).
As per coding guidelines.
Consider restructuring each test block to use a beforeEach hook:
describe('promptForAutomigrations', () => {
+ beforeEach(() => {
+ // Reset mocks before each test
+ vi.clearAllMocks();
+ });
+
it('should call multiselect with required: false', async () => {
const { prompt } = await import('storybook/internal/node-logger');
const multiselectMock = vi.mocked(prompt.multiselect);
+
+ // In beforeEach for this specific test context, or use nested describe blocks
multiselectMock.mockResolvedValue(['fix1']);Committable suggestion skipped: line range outside the PR's diff.
| ```ts filename="vitest.config.ts" renderer="react" language="ts" | ||
| import { defineConfig } from "vite"; | ||
| import { storybookNextJsPlugin } from '@storybook/nextjs-vite/vite-plugin' | ||
|
|
||
| export default defineConfig({ | ||
| // only necessary when not using @storybook/addon-vitest, otherwise the plugin is loaded automatically | ||
| plugins: [storybookNextJsPlugin()], | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the import path and usage pattern for the Storybook Next.js Vite plugin.
The snippet references @storybook/nextjs-vite/vite-plugin and calls storybookNextJsPlugin(). Please confirm these are correct and available in the package/version being documented.
🏁 Script executed:
#!/bin/bash
# Verify the import path exists in the React/Next.js preset exports
rg -A 5 -B 5 "storybookNextJsPlugin|nextjs-vite/vite-plugin" --type ts --type jsLength of output: 2208
Update import path to use the correct Storybook experimental package name.
The import should use @storybook/experimental-nextjs-vite/vite-plugin, not @storybook/nextjs-vite/vite-plugin.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: ts filename="vitest.config.ts" renderer="react" language="ts" import { defineConfig } from "vite"; import { storybookNextJsPlugin } from '@storybook/nextjs-vite/vite-plugin' export default defineConfig({ // only necessary when not using @storybook/addon-vitest, otherwise the plugin is loaded automatically plugins: [storybookNextJsPlugin()], });
(QB_NEW_DE)
🤖 Prompt for AI Agents
In docs/_snippets/vite-includeStorybookNextjsPlugin.md around lines 1 to 9, the
import path uses the wrong package name (@storybook/nextjs-vite/vite-plugin);
update the import to the correct experimental package path
@storybook/experimental-nextjs-vite/vite-plugin so the plugin resolves correctly
when users follow this snippet.
| ``` | ||
|
|
||
| A `main.ts` file that's CJS is no longer supported. | ||
| A `main.ts` file that's CJS is no longer supported. The same applies to any custom preset files. |
There was a problem hiding this comment.
“main.ts that’s CJS” phrasing is confusing
TypeScript files aren’t “CJS” per se; they may compile to CommonJS or use CJS syntax. Reword to avoid ambiguity and tell users how to fix it.
-A `main.ts` file that's CJS is no longer supported. The same applies to any custom preset files.
+CommonJS syntax (`require`, `module.exports`) in `main.ts` (or custom presets) is not supported. If your TS config compiles to CommonJS, switch to ESM syntax and set `compilerOptions.module` to `esnext` and `moduleResolution` to `bundler`/`node16`/`nodenext`.📝 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.
| A `main.ts` file that's CJS is no longer supported. The same applies to any custom preset files. | |
| CommonJS syntax (`require`, `module.exports`) in `main.ts` (or custom presets) is not supported. If your TS config compiles to CommonJS, switch to ESM syntax and set `compilerOptions.module` to `esnext` and `moduleResolution` to `bundler`/`node16`/`nodenext`. |
🤖 Prompt for AI Agents
MIGRATION.md line 531: reword the ambiguous phrase "main.ts that's CJS" to
explicitly say that TypeScript files that compile to CommonJS or that use
CommonJS module syntax (require/module.exports) are no longer supported;
instruct users to convert their main.ts and any custom preset files to ES
modules by using ES module syntax (import/export), configuring their TypeScript
compiler to emit ESM output (adjust tsconfig module setting) and/or setting
package.json to use "type":"module", or alternatively build them to ESM before
publishing.
kasperpeulen
force-pushed
the
kasper/props
branch
from
e2269ac to
91d3927
Compare
| return; | ||
| } | ||
|
|
||
| const examples = !componentName |
There was a problem hiding this comment.
the !componentName here seems redundant
|
Dangerously merging even though CI is failing.... |
closes #32705, closes #32706, closes #32670, closes #32671, closes #32672, closes #32703, closes #32669,
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Updates
Tests