Skip to content

Conversation

@yannbf
Copy link
Member

@yannbf yannbf commented Oct 17, 2025

Relates to #32291

What I did

This PR improves two things:

  1. The detection of valid vitest config files during Storybook init
  2. The modification of vitest config files in the vitest addon postinstall

It now supports a [commonly used](https://github.com/search?q=%22export%20default%20mergeConfig(%22%20path%3Avitest.config.*&type=code) pattern like so:

import { defineConfig, mergeConfig } from 'vitest/config';
import viteBaseConfig from './vite.base.config';
import path from 'path';

export default mergeConfig(
  viteBaseConfig,
  defineConfig({
    test: {
       // ...
    },
  })
);

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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

Release Notes

  • New Features

    • Better support for Vitest configs that use merge-style composition, including multi-argument cases.
    • Improved merging behavior to consolidate test/project/workspace entries into a consistent structure.
  • Tests

    • Expanded test coverage across many Vitest config shapes and edge cases to ensure safer config updates.
    • Added scenarios validating non-modification for unsupported or unchanged configurations.

@yannbf yannbf self-assigned this Oct 17, 2025
@yannbf yannbf added bug ci:merged Run the CI jobs that normally run when merged. addon: vitest labels Oct 17, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

📝 Walkthrough

Walkthrough

Adds detection and handling for Vitest configs that use mergeConfig (including multi-argument calls). updateVitestFile now recognizes ObjectExpression, defineConfig, and mergeConfig shapes, extracts/relocates test properties into workspace/projects arrays, updates imports, and sets updated when transformations occur. Tests and config-file checks were extended accordingly.

Changes

Cohort / File(s) Summary
Core merge & reshape logic
code/addons/vitest/src/updateVitestFile.ts
Detects supported config shapes (ObjectExpression, defineConfig(...), mergeConfig(...)). Finds a defineConfig with a test property when present, moves test into a workspace/projects template (prepends a test project with extends: true), removes original test, merges template into target, preserves fallbacks, and marks updates.
Unit tests for core behavior
code/addons/vitest/src/updateVitestFile.test.ts
Expands test matrix: multi-defineConfig and mergeConfig scenarios, projects/workspace variations, Vitest 3.2+ patterns, no-op cases, and function-notation unsupported cases. Reworks mocks (consistent resolvePackageDir) and uses before/after + diff snapshots (getDiff) to assert changes.
Config compatibility detection
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
Adds isMergeConfigExpression helper and extends ExportDefaultDeclaration handling to treat mergeConfig calls as valid when any argument is safe to extend; supports nested mergeConfig/defineConfig shapes and requires at least one argument for merge checks.
Compatibility test fixtures
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
Adds mock vitest configs demonstrating mergeConfig usage (single and multi-argument) and tests that these are recognized as compatible by the vitestConfigFiles checker.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant updateVitestFile
    participant AST
    participant mergeHandler
    participant templateMerger

    Caller->>updateVitestFile: request update (source, target)
    updateVitestFile->>AST: parse ExportDefaultDeclaration

    alt Export is ObjectExpression or defineConfig(Object)
        AST-->>updateVitestFile: object config
        updateVitestFile->>templateMerger: merge template into target
    else Export is mergeConfig(...)
        AST-->>updateVitestFile: mergeConfig node
        updateVitestFile->>mergeHandler: inspect mergeConfig arguments
        mergeHandler->>mergeHandler: find defineConfig arg with `test` prop
        alt matching defineConfig found
            mergeHandler->>mergeHandler: extract `test`, build project obj (extends:true)
            mergeHandler->>mergeHandler: locate/prepend to workspace/projects array in template
            mergeHandler->>mergeHandler: remove original `test` from defineConfig
            mergeHandler-->>updateVitestFile: transformed template
            updateVitestFile->>templateMerger: merge transformed template into target
        else no matching arg
            mergeHandler-->>updateVitestFile: no transform
            updateVitestFile->>templateMerger: apply fallback merge
        end
    end

    templateMerger-->>Caller: updated file (or no-op)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Rationale: Non-trivial AST transformations with multiple config shapes, several branching paths, and expanded tests. Core logic concentrated in one file but affects detection logic elsewhere; requires careful review of AST handling, edge cases, and test coverage.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch yann/support-mergeconfig-vitest-setup

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Oct 17, 2025

View your CI Pipeline Execution ↗ for commit b6a3c4a

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 53s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-17 15:43:20 UTC

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)

124-131: Also accept test.projects and only inspect defineConfig args in mergeConfig.
Right now safety check only validates test.workspace and tries every arg regardless of shape. Recommend:

  • Extend safety to accept projects arrays.
  • Filter mergeConfig args to defineConfig calls before checking.

Apply this diff to the safety check:

-const isSafeToExtendWorkspace = (path: CallExpression) =>
-  isObjectExpression(path.arguments[0]) &&
-  path.arguments[0]?.properties.every(
-    (p) =>
-      p.key.name !== 'test' ||
-      (isObjectExpression(p.value) &&
-        p.value.properties.every(
-          ({ key, value }) => key.name !== 'workspace' || isArrayExpression(value)
-        ))
-  );
+const isSafeToExtendWorkspace = (path: CallExpression) =>
+  isObjectExpression(path.arguments[0]) &&
+  path.arguments[0]?.properties.every(
+    (p) =>
+      p.key.name !== 'test' ||
+      (isObjectExpression(p.value) &&
+        p.value.properties.every(({ key, value }) =>
+          (key.name !== 'workspace' || isArrayExpression(value)) &&
+          (key.name !== 'projects' || isArrayExpression(value))
+        ))
+  );

And constrain the merge check:

-            isValidVitestConfig = mergeCall.arguments.some((arg) =>
-              isSafeToExtendWorkspace(arg as CallExpression)
-            );
+            isValidVitestConfig = mergeCall.arguments.some(
+              (arg) => isDefineConfigExpression(arg as any) && isSafeToExtendWorkspace(arg as CallExpression)
+            );
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (1)

197-204: Nice: validates defineConfig in non-first position.
Ensures argument-order robustness. Consider adding a case with test.projects to guard Vitest 3.2+.

Optional test additions:

@@
 const fileMocks = {
@@
+  'testProjects.ts': `
+    import { defineConfig } from 'vitest/config'
+    export default defineConfig({
+      test: { projects: ['packages/*'] }
+    })
+  `,
@@
   describe('Check Vitest config files', () => {
@@
+    it('should allow test.projects config option (Vitest 3.2+)', async () => {
+      vi.mocked(find.any).mockImplementation(coerce('config', 'testProjects.ts'));
+      const result = await vitestConfigFiles.condition(mockContext, state);
+      expect(result).toEqual({ type: 'compatible' });
+    });

Also consider using spy mocks for consistency:

-vi.mock('empathic/find', () => ({ any: vi.fn(), }));
+vi.mock('empathic/find', () => ({ any: vi.fn() }), { spy: true });

Based on learnings

code/addons/vitest/src/updateVitestFile.ts (1)

131-236: Avoid early function exit when no defineConfig is found in mergeConfig.
return false ends the whole update, potentially skipping other source nodes; prefer continuing the loop.

Apply:

-          if (!defineConfigNode) {
-            return false;
-          }
+          if (!defineConfigNode) {
+            // No defineConfig inside mergeConfig; skip this branch and keep scanning.
+            continue;
+          }

Optional: consider deduping array merges (e.g., projects/workspace entries, plugin list) to keep updates idempotent.

code/addons/vitest/src/updateVitestFile.test.ts (1)

17-19: Mock setup is fine; optionally enable spy mode.
Keeps behavior explicit; adding { spy: true } aligns with our test guidelines.

Apply:

-vi.mock('../../../core/src/shared/utils/module', () => ({
-  resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')),
-}));
+vi.mock('../../../core/src/shared/utils/module', () => ({
+  resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')),
+}), { spy: true });

Based on learnings

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28ab482 and 2adce21.

📒 Files selected for processing (4)
  • code/addons/vitest/src/updateVitestFile.test.ts (2 hunks)
  • code/addons/vitest/src/updateVitestFile.ts (1 hunks)
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (2 hunks)
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (2 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 (use yarn lint:js:cmd <file>)

Files:

  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.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 using vi.mock() (e.g., filesystem, loggers)

Files:

  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.287Z
Learning: Applies to code/vitest.workspace.ts : Keep and use the Vitest workspace configuration at code/vitest.workspace.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.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/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
📚 Learning: 2025-09-17T08:11:04.287Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursorrules:0-0
Timestamp: 2025-09-17T08:11:04.287Z
Learning: Applies to code/vitest.workspace.ts : Keep and use the Vitest workspace configuration at code/vitest.workspace.ts

Applied to files:

  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.test.ts
🧬 Code graph analysis (2)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
  • vitestConfigFiles (93-144)
code/addons/vitest/src/updateVitestFile.test.ts (1)
code/addons/vitest/src/updateVitestFile.ts (2)
  • loadTemplate (9-16)
  • updateConfigFile (79-241)
⏰ 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). (3)
  • GitHub Check: Danger JS
  • GitHub Check: merged
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)

58-60: Good addition: mergeConfig call detection is clear.
Helper keeps logic readable and localized.

code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (2)

16-42: Fixtures cover realistic mergeConfig shapes.
Both two-arg and multi-arg examples read well.


189-196: Test for mergeConfig compatibility looks good.
Covers the common “base vite + defineConfig” path.

code/addons/vitest/src/updateVitestFile.test.ts (1)

426-621: Great coverage for multi-arg mergeConfig scenarios.
Snapshots validate both with/without existing test and projects pattern.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (8)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (2)

62-64: Loosen type on isSafeToExtendWorkspace to avoid unsafe casts

You’re casting arbitrary mergeConfig args to CallExpression before passing to isSafeToExtendWorkspace. Make the helper accept Declaration and self‑guard, then drop the casts.

-const isSafeToExtendWorkspace = (path: CallExpression) =>
-  isCallExpression(path) &&
+const isSafeToExtendWorkspace = (path: Declaration) =>
+  isCallExpression(path) &&
   path.arguments.length > 0 &&
   isObjectExpression(path.arguments[0]) &&
   ...
 
- isValidVitestConfig = mergeCall.arguments.some((arg) =>
-   isSafeToExtendWorkspace(arg as CallExpression)
- );
+ isValidVitestConfig = mergeCall.arguments.some((arg) => isSafeToExtendWorkspace(arg));

Also applies to: 131-133


126-134: Support mergeConfig(..., { test: {...} }) (plain object arg) as valid

Many Vitest configs pass a plain ObjectExpression to mergeConfig. Consider treating an ObjectExpression arg that satisfies the same safety predicate as valid.

- isValidVitestConfig = mergeCall.arguments.some((arg) =>
-   isSafeToExtendWorkspace(arg as CallExpression)
- );
+ isValidVitestConfig = mergeCall.arguments.some((arg) => {
+   if (isSafeToExtendWorkspace(arg)) return true; // defineConfig(...)
+   return isObjectExpression(arg) && arg.properties?.some((p) => p.key?.name === 'test');
+ });
code/addons/vitest/src/updateVitestFile.test.ts (2)

10-16: Add spy: true to mocks (test guideline)

Our test guidelines require vi.mock(..., { spy: true }). Please add the option to both mocks.

-vi.mock('storybook/internal/node-logger', () => ({
+vi.mock('storybook/internal/node-logger', () => ({
   logger: {
     info: vi.fn(),
     warn: vi.fn(),
     error: vi.fn(),
   },
-}));
+}), { spy: true });
 
-vi.mock('../../../core/src/shared/utils/module', () => ({
-  resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')),
-}));
+vi.mock('../../../core/src/shared/utils/module', () => ({
+  resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')),
+}), { spy: true });

Based on learnings

Also applies to: 18-20


498-577: Add a test for mergeConfig with a plain object argument

We handle defineConfig args; add a case for mergeConfig(viteConfig, { test: {...} }) to lock behavior.

+  it('supports mergeConfig with a plain object containing test property', async () => {
+    const source = babel.babelParse(
+      await loadTemplate('vitest.config.template.ts', {
+        CONFIG_DIR: '.storybook',
+        BROWSER_CONFIG: "{ provider: 'playwright' }",
+        SETUP_FILE: '../.storybook/vitest.setup.ts',
+      })
+    );
+    const target = babel.babelParse(`
+      import { mergeConfig } from 'vite'
+      import viteConfig from './vite.config'
+      export default mergeConfig(viteConfig, {
+        test: { environment: 'jsdom' }
+      })
+    `);
+    const before = babel.generate(target).code;
+    const updated = updateConfigFile(source, target);
+    expect(updated).toBe(true);
+    const after = babel.generate(target).code;
+    expect(after).not.toBe(before);
+    expect(getDiff(before, after)).toMatchInlineSnapshot();
+  });

I can generate the expected snapshot once the implementation lands.

Also applies to: 580-649, 651-727

code/addons/vitest/src/updateVitestFile.ts (4)

109-135: Handle ObjectExpression args in mergeConfig detection

mergeConfig often takes a plain object. Expand canHandleConfig to accept that shape 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 objectArgExists = targetExportDefault.declaration.arguments.some(
+    (arg) => arg?.type === 'ObjectExpression'
+  );
+  canHandleConfig = defineConfigNodes.length > 0 || objectArgExists;
 }

140-171: Mark updated when adding imports or variables; align import de-dupe with workspace updater

  • Return value promises “true if the target was modified”. Adding imports/vars modifies the file; set updated = true.
  • Import de-dupe should also compare source module like updateWorkspaceFile does.
- if (
-   !target.program.body.some(
-     (targetNode) =>
-       targetNode.type === sourceNode.type &&
-       targetNode.specifiers.some((s) => s.local.name === sourceNode.specifiers[0].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)
+   )
+ ) {
   const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration');
   target.program.body.splice(lastImport + 1, 0, sourceNode);
+  updated = true;
 }
 ...
       ) {
         const lastImport = target.program.body.findLastIndex((n) => n.type === 'ImportDeclaration');
         target.program.body.splice(lastImport + 1, 0, sourceNode);
+        updated = true;
       }

191-296: Support mergeConfig with plain object args; avoid early return path

When default export is mergeConfig and one arg is an ObjectExpression, merge template props directly into that object. Also avoid returning false deep in this branch.

-        } else if (
+        } else if (
           exportDefault.declaration.type === 'CallExpression' &&
           exportDefault.declaration.callee.type === 'Identifier' &&
           exportDefault.declaration.callee.name === 'mergeConfig' &&
           exportDefault.declaration.arguments.length >= 2
         ) {
           const defineConfigNodes = exportDefault.declaration.arguments.filter(
             (arg): arg is t.CallExpression =>
               arg?.type === 'CallExpression' &&
               arg.callee.type === 'Identifier' &&
               arg.callee.name === 'defineConfig' &&
               arg.arguments[0]?.type === 'ObjectExpression'
           );
 
           const defineConfigNodeWithTest = defineConfigNodes.find(
             (node) =>
               node.arguments[0].type === 'ObjectExpression' &&
               node.arguments[0].properties.some(
                 (p) =>
                   p.type === 'ObjectProperty' &&
                   p.key.type === 'Identifier' &&
                   p.key.name === 'test'
               )
           );
 
           // Give precedence for the defineConfig expression which contains a test config property
           // As with mergeConfig you never know where the test could be e.g. mergeConfig(viteConfig, defineConfig({}), defineConfig({ test: {...} }))
-          const defineConfigNode = defineConfigNodeWithTest || defineConfigNodes[0];
-
-          if (!defineConfigNode) {
-            return false;
-          }
+          const defineConfigNode = defineConfigNodeWithTest || defineConfigNodes[0];
 
-          const defineConfigProps = defineConfigNode.arguments[0] as t.ObjectExpression;
+          if (defineConfigNode) {
+            const defineConfigProps = defineConfigNode.arguments[0] as t.ObjectExpression;
             ...
-          updated = true;
+            updated = true;
+          } else {
+            // Fallback: look for a plain object argument
+            const objectArg = exportDefault.declaration.arguments.find(
+              (arg): arg is t.ObjectExpression => arg?.type === 'ObjectExpression'
+            );
+            if (objectArg) {
+              mergeProperties(properties, objectArg.properties);
+              updated = true;
+            }
+          }
         }

18-52: mergeProperties: consider stable uniqueing of array elements (optional)

Shallow push can duplicate entries (e.g., duplicate plugins). If feasible, unique by Identifier.name or literal value.

I can sketch a small helper that de-dupes ArrayExpression elements by a stable key if you’d like.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2adce21 and b6a3c4a.

📒 Files selected for processing (3)
  • code/addons/vitest/src/updateVitestFile.test.ts (11 hunks)
  • code/addons/vitest/src/updateVitestFile.ts (2 hunks)
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (2 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 (use yarn lint:js:cmd <file>)

Files:

  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.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/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.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/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size

Files:

  • code/addons/vitest/src/updateVitestFile.ts
  • code/addons/vitest/src/updateVitestFile.test.ts
  • code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.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/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.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 using vi.mock() (e.g., filesystem, loggers)

Files:

  • code/addons/vitest/src/updateVitestFile.test.ts
🧠 Learnings (4)
📚 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/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.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/addons/vitest/src/updateVitestFile.test.ts
🧬 Code graph analysis (1)
code/addons/vitest/src/updateVitestFile.test.ts (1)
code/addons/vitest/src/updateVitestFile.ts (3)
  • updateConfigFile (79-301)
  • loadTemplate (9-16)
  • updateWorkspaceFile (303-364)
⏰ 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). (3)
  • GitHub Check: merged
  • GitHub Check: Danger JS
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)

58-60: Good addition: mergeConfig detection

Detecting mergeConfig at export default is correct and keeps traversal simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addon: vitest bug ci:merged Run the CI jobs that normally run when merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants