Skip to content

Addon-Vitest: Handle additional vitest config export patterns in postinstall#34106

Merged
yannbf merged 11 commits into
nextfrom
copilot/fix-vitest-postinstall-error
Mar 17, 2026
Merged

Addon-Vitest: Handle additional vitest config export patterns in postinstall#34106
yannbf merged 11 commits into
nextfrom
copilot/fix-vitest-postinstall-error

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 11, 2026

  • Fix defineConfig(mergeConfig(...)) pattern (defineConfig wrapping mergeConfig)
  • Fix mergeConfig(...) as ViteUserConfig pattern (TSAsExpression wrapping)
  • Fix defineConfig(mergeConfig(...) satisfies ViteUserConfig) pattern (TSSatisfiesExpression)
  • Fix export default config where config = mergeConfig(...) (variable re-export)
  • Fix mergeConfig(viteConfig, vitestConfig) where vitestConfig is a variable
  • Fix defineProject({...}) pattern (alias for defineConfig)
  • Fix const test = {...}; mergeConfig(viteConfig, { test }) pattern (shorthand property)
  • Add unit tests for all new patterns (7 new tests, all 26 tests passing)
  • Code review feedback addressed (type safety + readability improvements)
  • ESLint/Prettier issues resolved
  • CodeQL security scan (no issues)

View original Slack conversation


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Summary by CodeRabbit

  • New Tests

    • Added extensive test suites covering many Vitest config shapes, workspace/project scenarios, and expression-resolution behaviors; consolidated one existing test to focus on template loading.
  • Bug Fixes

    • Improved detection and safe merging of complex Vitest configs (workspace vs projects, TS wrappers), reducing incorrect updates.
  • Refactor

    • Adopted a resolution-first approach to reliably locate and modify target test configurations.
  • Behavior

    • Template selection refined to avoid introducing conflicting workspace/projects keys.

Copilot AI and others added 2 commits March 11, 2026 16:32
Fixes AddonVitestPostinstallError for these patterns:
- defineConfig(mergeConfig(...)) – defineConfig wrapping mergeConfig
- defineConfig(mergeConfig(...) satisfies ViteUserConfig) – satisfies operator
- mergeConfig(...) as ViteUserConfig – TSAsExpression wrapper
- const config = mergeConfig(...); export default config – re-exported variable
- const vitestConfig = {...}; export default mergeConfig(viteConfig, vitestConfig) – constant arg
- defineProject({...}) – defineProject as alias for defineConfig
- const test = {...}; mergeConfig(viteConfig, { test }) – shorthand property

New helpers added to updateVitestFile.ts:
- unwrapTSExpression: peels off TSAsExpression/TSSatisfiesExpression/TSTypeAssertion
- resolveExpression: resolves through variable references + TS type wrappers
- isDefineConfigLike: recognizes defineConfig and defineProject
- getEffectiveMergeConfigCall: finds the mergeConfig call through any wrappers

26 unit tests passing (19 existing + 7 new).

Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
- Use proper union type for TS expression unwrapping (TSAsExpression | TSSatisfiesExpression | TSTypeAssertion)
- Extract resolveTestPropValue helper to replace nested ternary with a named function

Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix AddonVitestPostinstallError in vitest config files fix(vitest-addon): handle additional vitest config export patterns in postinstall Mar 11, 2026
@valentinpalkovic valentinpalkovic moved this to In Progress in Core Team Projects Mar 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 11, 2026

Fails
🚫 PR description is missing the mandatory "#### Manual testing" section. Please add it so that reviewers know how to manually test your changes.

Generated by 🚫 dangerJS against f894b4d

@valentinpalkovic valentinpalkovic changed the title fix(vitest-addon): handle additional vitest config export patterns in postinstall Addon-Vitest: handle additional vitest config export patterns in postinstall Mar 11, 2026
@valentinpalkovic valentinpalkovic changed the title Addon-Vitest: handle additional vitest config export patterns in postinstall Addon-Vitest: Handle additional vitest config export patterns in postinstall Mar 11, 2026
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Mar 11, 2026

View your CI Pipeline Execution ↗ for commit f894b4d

Command Status Duration Result
nx run-many -t compile -c production --parallel=1 ✅ Succeeded 6m 1s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-17 08:26:53 UTC

@valentinpalkovic
Copy link
Copy Markdown
Contributor

@copilot Fix eslint/prettier issues

Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot Fix eslint/prettier issues

Fixed in 20e48e6. Ran yarn lint:js:cmd --fix which auto-corrected 19 prettier formatting errors in updateVitestFile.ts.

@valentinpalkovic valentinpalkovic marked this pull request as ready for review March 11, 2026 20:12
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 11, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an AST expression resolver and refactors Vitest config merging to be wrapper-aware (defineConfig/defineProject, mergeConfig, TS wrappers, identifier resolution). Expands AST-driven tests and adjusts postinstall template selection based on detected workspace usage. One legacy test file was trimmed to only loadTemplate checks.

Changes

Cohort / File(s) Summary
Expression resolver
code/core/src/babel/expression-resolver.ts, code/core/src/babel/index.ts
New utilities unwrapTSExpression and resolveExpression added and re-exported; used to unwrap TS wrappers and resolve identifier initializers within ASTs.
Vitest updater implementation
code/addons/vitest/src/updateVitestFile.ts
Large refactor to a resolution-first pipeline: resolves wrapped/indirect config objects (defineConfig/defineProject, mergeConfig, TS wrappers, identifiers), merges/appends testprojects, hoists coverage, and rejects unsupported runtime-function notations. Review resolution and merge edge cases.
Postinstall template logic
code/addons/vitest/src/postinstall.ts
getTemplateName now considers parsed configContent via configUsesWorkspace to prefer a workspace-safe template when existing config uses test.workspace.
Tests — new/expanded
code/addons/vitest/src/updateVitestFile.config.test.ts, .../updateVitestFile.config.3.2.test.ts, .../updateVitestFile.config.4.test.ts, .../updateVitestFile.config.workspace.test.ts
Adds extensive AST-driven tests covering many config shapes (defineConfig/defineProject, mergeConfig, TS as/satisfies, workspace vs projects, plugin insertion) with diff-based assertions.
Tests — modified
code/addons/vitest/src/updateVitestFile.test.ts
Significantly trimmed: removed most updateConfigFile/updateWorkspaceFile tests and related imports; retains only loadTemplate-related path-normalization tests.
Expression resolver tests
code/core/src/babel/expression-resolver.test.ts
New tests validating unwrapTSExpression and resolveExpression behavior across TS wrappers and identifier chains.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Postinstall
    participant TemplateLoader
    participant Parser
    participant Resolver as ExpressionResolver
    participant Updater as updateConfigFile
    participant FS as FileSystem

    Postinstall->>TemplateLoader: request template (may pass configContent)
    TemplateLoader->>Parser: load & parse template AST
    Postinstall->>Parser: parse target config AST
    Updater->>Resolver: resolve wrapped identifiers/expressions in target AST
    Resolver-->>Updater: resolved config object node
    Updater->>Parser: merge/append projects, hoist coverage, inject plugin code
    Updater->>FS: write updated file if modified
    FS-->>Postinstall: return write result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

📝 Coding Plan
  • Generate coding plan for human review comments

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

Tip

CodeRabbit can generate a title for your PR based on the changes.

Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the reviews.auto_title_placeholder setting.

Copy link
Copy Markdown
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: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/addons/vitest/src/updateVitestFile.ts`:
- Around line 103-110: The type guard isProjectsArrayProp only matches a
`projects` property so objects shaped like `test: { workspace: [...] }` are
missed; update the guard (used when detecting array-valued project lists for
mergeConfig/viteConfig handling) to accept either an Identifier named "projects"
OR an Identifier named "workspace" with ArrayExpression value so the same
append/merge logic applies; locate and change the isProjectsArrayProp predicate
and any usages in the merge flow that rely on it (the mergeConfig/viteConfig
handling where test.workspace is supported) to ensure test.workspace arrays are
treated identically to test.projects arrays.

In `@code/core/src/babel/expression-resolver.ts`:
- Around line 36-39: The declarator lookup only checks for top-level
VariableDeclaration nodes so exported vars like `export const config = ...` are
missed; update the logic that builds `declarator` (in expression-resolver.ts,
where `declarator` and `varName` are used) to also consider
`ExportNamedDeclaration` nodes that have a `declaration` which is a
`VariableDeclaration` (or unwrap exports before flatMapping), i.e. replicate the
dual-check pattern used in findVarInitialization.ts/ConfigFile.ts so you search
both bare VariableDeclaration nodes and VariableDeclaration nodes wrapped inside
ExportNamedDeclaration when matching the identifier name.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f4592b5-9415-4cef-b789-c473f39ff16b

📥 Commits

Reviewing files that changed from the base of the PR and between 20e48e6 and 653e55f.

📒 Files selected for processing (3)
  • code/addons/vitest/src/updateVitestFile.ts
  • code/core/src/babel/expression-resolver.ts
  • code/core/src/babel/index.ts

Comment thread code/addons/vitest/src/updateVitestFile.ts Outdated
Comment thread code/core/src/babel/expression-resolver.ts Outdated
Copy link
Copy Markdown
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: 3

🧹 Nitpick comments (2)
code/addons/vitest/src/updateVitestFile.config.test.ts (1)

22-1774: It looks like the split left the original catch-all suite behind almost unchanged.

Most of these scenarios are now repeated in updateVitestFile.config.3.2.test.ts and updateVitestFile.config.workspace.test.ts, so each snapshot-only change has to be updated in multiple places and it is much less clear which suite is authoritative. I'd trim this file down to the genuinely unique cases, or remove it in favor of the split suites.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/updateVitestFile.config.test.ts` around lines 22 -
1774, The test file's describe('updateConfigFile') suite contains many
snapshot-only scenarios duplicated across updateVitestFile.config.3.2.test.ts
and updateVitestFile.config.workspace.test.ts; remove redundant cases from this
file by keeping only genuinely unique tests (identify each by the it(...) titles
and the top-level describe('updateConfigFile')), or delete this entire file and
rely on the split suites, and ensure the canonical scenarios (e.g., tests that
exercise updateConfigFile directly) remain in the other two files; after
trimming, run the test suite to confirm snapshots are only updated in the
authoritative files.
code/addons/vitest/src/updateVitestFile.config.4.test.ts (1)

461-538: Remove or repurpose this duplicated snapshot case.

This block is a verbatim duplicate of the earlier adds projects property to test config case near Line 217, including the same target input and inline snapshot. Keeping both only adds runtime and snapshot churn without expanding coverage.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/updateVitestFile.config.4.test.ts` around lines 461 -
538, The test case titled "adds projects property to test config" is duplicated;
remove the duplicate block (the second occurrence in
updateVitestFile.config.4.test.ts) or modify it to cover a different scenario
instead of repeating the exact target and inline snapshot. Locate the duplicated
it(...) block that uses babel.babelParse,
loadTemplate('vitest.config.4.template', ...), updateConfigFile(source, target)
and getDiff(before, after) and either delete that whole it(...) test or change
its inputs/expected snapshot to exercise a distinct behavior (e.g., different
CONFIG_DIR or BROWSER_CONFIG) to avoid runtime and snapshot churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/addons/vitest/src/postinstall.ts`:
- Around line 193-205: The getTemplateName function currently calls
configUsesWorkspace(configContent) on the raw file text which misses
exported/aliased test objects and can false-positive; change getTemplateName to
first resolve the exported config AST/value using the same export-resolution
logic used by updateConfigFile (reuse or call that resolver) and pass the
resolved config object or its serialized source to configUsesWorkspace (or a new
helper that inspects the resolved test property) so template selection correctly
detects whether the effective config uses workspace vs projects before returning
'vitest.config.template' vs 'vitest.config.3.2.template' (keep references to
getTemplateName, configUsesWorkspace, and updateConfigFile to locate the code).

In `@code/addons/vitest/src/updateVitestFile.config.4.test.ts`:
- Around line 10-20: Replace the inline mocks in the vi.mock(...) calls by
enabling the spy behavior and moving implementations into beforeEach: use
vi.mock('storybook/internal/node-logger', () => ({ logger: { info: () => {},
warn: () => {}, error: () => {} } }), { spy: true }) and
vi.mock('../../../core/src/shared/utils/module', () => ({ resolvePackageDir: ()
=> undefined }), { spy: true }) in the top-level mock declarations, then in
beforeEach set the real implementations via vi.mocked(...) to access and set the
mock implementations for logger.info/logger.warn/logger.error and
resolvePackageDir (e.g., vi.mocked(logger.info).mockImplementation(...),
vi.mocked(resolvePackageDir).mockImplementation(() => join(__dirname, '..')));
update the sibling tests (updateVitestFile.config.3.2.test.ts,
updateVitestFile.config.test.ts) the same way.

In `@code/addons/vitest/src/updateVitestFile.config.workspace.test.ts`:
- Around line 24-30: The workspace test is parsing the raw template rather than
the preprocessed form used in production; replicate postinstall.ts preprocessing
before calling babel.babelParse by loading the template via
loadTemplate('vitest.workspace.template', ...) then remove the ROOT_CONFIG
placeholder and strip any empty "extends" entry (EXTENDS_WORKSPACE === ''/empty)
the same way postinstall.ts does, and use that processed string as the input to
babel.babelParse (affecting the variable source and the assertions that call
updateWorkspaceFile()); make the same change for the second test block around
the other occurrence (lines 79-85) so snapshots match the real postinstall
input.

---

Nitpick comments:
In `@code/addons/vitest/src/updateVitestFile.config.4.test.ts`:
- Around line 461-538: The test case titled "adds projects property to test
config" is duplicated; remove the duplicate block (the second occurrence in
updateVitestFile.config.4.test.ts) or modify it to cover a different scenario
instead of repeating the exact target and inline snapshot. Locate the duplicated
it(...) block that uses babel.babelParse,
loadTemplate('vitest.config.4.template', ...), updateConfigFile(source, target)
and getDiff(before, after) and either delete that whole it(...) test or change
its inputs/expected snapshot to exercise a distinct behavior (e.g., different
CONFIG_DIR or BROWSER_CONFIG) to avoid runtime and snapshot churn.

In `@code/addons/vitest/src/updateVitestFile.config.test.ts`:
- Around line 22-1774: The test file's describe('updateConfigFile') suite
contains many snapshot-only scenarios duplicated across
updateVitestFile.config.3.2.test.ts and
updateVitestFile.config.workspace.test.ts; remove redundant cases from this file
by keeping only genuinely unique tests (identify each by the it(...) titles and
the top-level describe('updateConfigFile')), or delete this entire file and rely
on the split suites, and ensure the canonical scenarios (e.g., tests that
exercise updateConfigFile directly) remain in the other two files; after
trimming, run the test suite to confirm snapshots are only updated in the
authoritative files.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6597ddcc-100a-44f7-8ab7-73e3a8f3d29a

📥 Commits

Reviewing files that changed from the base of the PR and between 653e55f and d5acaa6.

📒 Files selected for processing (6)
  • code/addons/vitest/src/postinstall.ts
  • code/addons/vitest/src/updateVitestFile.config.3.2.test.ts
  • code/addons/vitest/src/updateVitestFile.config.4.test.ts
  • code/addons/vitest/src/updateVitestFile.config.test.ts
  • code/addons/vitest/src/updateVitestFile.config.workspace.test.ts
  • code/addons/vitest/src/updateVitestFile.test.ts

Comment on lines +193 to 205
const getTemplateName = (configContent?: string) => {
if (isVitest4OrNewer) {
return 'vitest.config.4.template';
} else if (isVitest3_2To4) {
// In Vitest 3.2, `workspace` was deprecated in favor of `projects` but still works.
// If the user's existing config already uses `workspace`, use the old template that
// also uses `workspace` so that the merge doesn't introduce both keys.
if (configContent && configUsesWorkspace(configContent)) {
return 'vitest.config.template';
}
return 'vitest.config.3.2.template';
}
return 'vitest.config.template';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Resolve the exported config before picking the workspace vs. projects template.

configUsesWorkspace() does a file-wide ObjectProperty scan and only recognizes inline test: { workspace: ... }. That misses shapes like const test = { workspace: [...] }; export default defineConfig({ test }), and it can also false-positive on unrelated test.workspace objects elsewhere in the file. In either case getTemplateName(configFile) can choose the wrong template and reintroduce the workspace/projects conflict this change is trying to avoid. Reusing the same export-resolution path that updateConfigFile() uses would make template selection match the config shapes this PR now supports.

Also applies to: 264-264, 451-484

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/postinstall.ts` around lines 193 - 205, The
getTemplateName function currently calls configUsesWorkspace(configContent) on
the raw file text which misses exported/aliased test objects and can
false-positive; change getTemplateName to first resolve the exported config
AST/value using the same export-resolution logic used by updateConfigFile (reuse
or call that resolver) and pass the resolved config object or its serialized
source to configUsesWorkspace (or a new helper that inspects the resolved test
property) so template selection correctly detects whether the effective config
uses workspace vs projects before returning 'vitest.config.template' vs
'vitest.config.3.2.template' (keep references to getTemplateName,
configUsesWorkspace, and updateConfigFile to locate the code).

Comment on lines +10 to +20
vi.mock('storybook/internal/node-logger', () => ({
logger: {
info: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
},
}));

vi.mock('../../../core/src/shared/utils/module', () => ({
resolvePackageDir: vi.fn().mockImplementation(() => join(__dirname, '..')),
}));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check if the file exists and examine lines 10-20
head -30 code/addons/vitest/src/updateVitestFile.config.4.test.ts | cat -n

Repository: storybookjs/storybook

Length of output: 1195


🏁 Script executed:

# Also look for sibling test files to check if the pattern is repeated
fd 'updateVitestFile.*\.test\.ts' code/addons/vitest/src/

Repository: storybookjs/storybook

Length of output: 348


🏁 Script executed:

# Examine the full structure of the file to understand the mock setup
wc -l code/addons/vitest/src/updateVitestFile.config.4.test.ts

Repository: storybookjs/storybook

Length of output: 126


🏁 Script executed:

# Check the mock setup in sibling test files
head -25 code/addons/vitest/src/updateVitestFile.config.3.2.test.ts | cat -n

Repository: storybookjs/storybook

Length of output: 1124


🏁 Script executed:

# Check another sibling file
head -25 code/addons/vitest/src/updateVitestFile.config.test.ts | cat -n

Repository: storybookjs/storybook

Length of output: 978


🏁 Script executed:

# Check if there's a beforeEach block in the main test file
grep -n "beforeEach" code/addons/vitest/src/updateVitestFile.config.4.test.ts | head -5

Repository: storybookjs/storybook

Length of output: 47


🏁 Script executed:

# Search for how mocks are set up in other test files to see the standard pattern
grep -r "vi\.mock.*spy.*true" code/addons/vitest/src/ --include="*.test.ts" | head -3

Repository: storybookjs/storybook

Length of output: 47


🏁 Script executed:

# Check if there are any beforeEach blocks with mock setup in other vitest files in the codebase
find code/addons/vitest/src -name "*.test.ts" -exec grep -l "beforeEach" {} \; | head -5

Repository: storybookjs/storybook

Length of output: 277


🏁 Script executed:

# Check one of these files to see how beforeEach and mocks are actually used
grep -A 10 "beforeEach" code/addons/vitest/src/node/boot-test-runner.test.ts | head -20

Repository: storybookjs/storybook

Length of output: 686


🏁 Script executed:

# Check if spy: true is used in the codebase
grep -r "spy.*true" code/addons/vitest/src/ --include="*.test.ts" | head -5

Repository: storybookjs/storybook

Length of output: 47


🏁 Script executed:

# Look at the spy-mocking.mdc file to understand the exact rules
cat .cursor/rules/spy-mocking.mdc 2>/dev/null || cat .cursorrules 2>/dev/null | head -50

Repository: storybookjs/storybook

Length of output: 1585


Use the repo-standard Vitest spy mock setup here.

Lines 10–20 use inline vi.fn() and mockImplementation() directly inside vi.mock() calls, and the same pattern is copied into sibling test files (updateVitestFile.config.3.2.test.ts, updateVitestFile.config.test.ts). Per the repository's spy-mocking rules, refactor to use:

  1. spy: true option in all vi.mock() calls
  2. Mock behavior implementations moved to beforeEach blocks
  3. vi.mocked() to type and access the mocked functions
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/updateVitestFile.config.4.test.ts` around lines 10 -
20, Replace the inline mocks in the vi.mock(...) calls by enabling the spy
behavior and moving implementations into beforeEach: use
vi.mock('storybook/internal/node-logger', () => ({ logger: { info: () => {},
warn: () => {}, error: () => {} } }), { spy: true }) and
vi.mock('../../../core/src/shared/utils/module', () => ({ resolvePackageDir: ()
=> undefined }), { spy: true }) in the top-level mock declarations, then in
beforeEach set the real implementations via vi.mocked(...) to access and set the
mock implementations for logger.info/logger.warn/logger.error and
resolvePackageDir (e.g., vi.mocked(logger.info).mockImplementation(...),
vi.mocked(resolvePackageDir).mockImplementation(() => join(__dirname, '..')));
update the sibling tests (updateVitestFile.config.3.2.test.ts,
updateVitestFile.config.test.ts) the same way.

Comment on lines +24 to +30
const source = babel.babelParse(
await loadTemplate('vitest.workspace.template', {
EXTENDS_WORKSPACE: '',
CONFIG_DIR: '.storybook',
BROWSER_CONFIG: "{ provider: 'playwright' }",
SETUP_FILE: '../.storybook/vitest.setup.ts',
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Make these workspace snapshots match the real postinstall input.

postinstall.ts strips the ROOT_CONFIG placeholder and the empty extends entry before it parses the workspace template, but both tests build source from the raw loadTemplate() result and snapshot that unprocessed shape. The assertions therefore cover a template variant that production never passes to updateWorkspaceFile(). Preprocess the template here the same way before diffing so the snapshots track the shipped flow.

Also applies to: 79-85

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/updateVitestFile.config.workspace.test.ts` around
lines 24 - 30, The workspace test is parsing the raw template rather than the
preprocessed form used in production; replicate postinstall.ts preprocessing
before calling babel.babelParse by loading the template via
loadTemplate('vitest.workspace.template', ...) then remove the ROOT_CONFIG
placeholder and strip any empty "extends" entry (EXTENDS_WORKSPACE === ''/empty)
the same way postinstall.ts does, and use that processed string as the input to
babel.babelParse (affecting the variable source and the assertions that call
updateWorkspaceFile()); make the same change for the second test block around
the other occurrence (lines 79-85) so snapshots match the real postinstall
input.

Copy link
Copy Markdown
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: 2

♻️ Duplicate comments (1)
code/addons/vitest/src/updateVitestFile.ts (1)

103-110: ⚠️ Potential issue | 🟠 Major

Handle existing test.workspace arrays in the mergeConfig path too.

This guard still only matches projects, so mergeConfig(viteConfig, { test: { workspace: [...] } }) falls through at Line 483 and gets rewrapped as a new project instead of appending to the existing array.

Suggested fix
-/** Type guard for a property that is a `projects` key with an ArrayExpression value. */
+/** Type guard for a property that is a `projects` or `workspace` key with an ArrayExpression value. */
 const isProjectsArrayProp = (
   p: t.ObjectMethod | t.ObjectProperty | t.SpreadElement
 ): p is t.ObjectProperty =>
   p.type === 'ObjectProperty' &&
   p.key.type === 'Identifier' &&
-  p.key.name === 'projects' &&
+  (p.key.name === 'projects' || p.key.name === 'workspace') &&
   p.value.type === 'ArrayExpression';

Also applies to: 482-485

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@code/addons/vitest/src/updateVitestFile.ts` around lines 103 - 110, The type
guard isProjectsArrayProp only matches a "projects" key so mergeConfig paths
like test.workspace get rewrapped; update the guard (isProjectsArrayProp) to
accept either 'projects' or 'workspace' (e.g., p.key.name === 'projects' ||
p.key.name === 'workspace'), and then apply the same change to the duplicate
check used in the mergeConfig path (the other occurrence around the merge logic)
so both places will detect and append to existing ArrayExpression values for
workspace as well as projects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@code/addons/vitest/src/updateVitestFile.config.3.2.test.ts`:
- Around line 10-20: The two inline mocks should be converted to spy-style
mocks: call vi.mock('storybook/internal/node-logger', { spy: true }) and
vi.mock('../../../core/src/shared/utils/module', { spy: true }) (keeping the
same module strings), remove inline vi.fn() and mockImplementation usage, and
move behavior setup into a beforeEach where you obtain type-safe mock handles
via vi.mocked(loggerModule) / vi.mocked(moduleUtils) and set
logger.info/warn/error = vi.fn() and
moduleUtils.resolvePackageDir.mockReturnValue(join(__dirname, '..')); ensure all
mock implementations live in beforeEach and use vi.mocked for access.

In `@code/addons/vitest/src/updateVitestFile.config.test.ts`:
- Around line 10-20: Refactor the two vi.mock calls so the factories are passive
and use the { spy: true } option, then move all mock behavior into a beforeEach:
call vi.mock('storybook/internal/node-logger', () => ({ logger: { info: vi.fn(),
warn: vi.fn(), error: vi.fn() }}), { spy: true }) and
vi.mock('../../../core/src/shared/utils/module', () => ({ resolvePackageDir:
vi.fn() }), { spy: true }); in beforeEach set the implementations via
vi.mocked(logger).info.mockImplementation(...)/warn/error and
vi.mocked(resolvePackageDir).mockImplementation(() => join(__dirname, '..')) so
tests use vi.mocked(...) for type-safe access to logger.info/warn/error and
resolvePackageDir instead of inline factories.

---

Duplicate comments:
In `@code/addons/vitest/src/updateVitestFile.ts`:
- Around line 103-110: The type guard isProjectsArrayProp only matches a
"projects" key so mergeConfig paths like test.workspace get rewrapped; update
the guard (isProjectsArrayProp) to accept either 'projects' or 'workspace'
(e.g., p.key.name === 'projects' || p.key.name === 'workspace'), and then apply
the same change to the duplicate check used in the mergeConfig path (the other
occurrence around the merge logic) so both places will detect and append to
existing ArrayExpression values for workspace as well as projects.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e90512a-f0ec-450f-95d9-078b04bd4888

📥 Commits

Reviewing files that changed from the base of the PR and between d5acaa6 and d8b7f7c.

📒 Files selected for processing (4)
  • code/addons/vitest/src/updateVitestFile.config.3.2.test.ts
  • code/addons/vitest/src/updateVitestFile.config.4.test.ts
  • code/addons/vitest/src/updateVitestFile.config.test.ts
  • code/addons/vitest/src/updateVitestFile.ts

Comment thread code/addons/vitest/src/updateVitestFile.config.3.2.test.ts
Comment thread code/addons/vitest/src/updateVitestFile.config.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants