Addon Vitest: Fix incorrect file modifications during setup #32844
Conversation
Only add `/// <reference types="vitest/config" />` to vite.config files, not vitest.config files, because vitest.config files already have the vitest/config types available by default. Co-authored-by: yannbf <[email protected]>
When transforming vitest configs with coverage settings, the addon now correctly: - Extracts the coverage property from the existing test config - Keeps it at the top-level test object (where it's global) - Moves other test properties to workspace/projects array items - Adds test cases to verify the fix works for both workspace and projects modes Co-authored-by: yannbf <[email protected]>
yannbf
added
bug
ci:merged
📝 WalkthroughWalkthroughAdds conditional insertion of triple-slash Vitest type references (only to vite.config when absent), and refactors mergeConfig/defineConfig handling to extract test/coverage objects, relocating coverage to top-level when workspace/projects exist and injecting sanitized test configs into projects. Changes
Sequence Diagram(s)sequenceDiagram
participant Postinstall as postinstall.ts
participant Updater as updateVitestFile.ts
participant FS as File System
Note over Postinstall: On postinstall/update file
Postinstall->>FS: Read config file
alt file is vite.config AND no existing reference
Postinstall->>FS: Prepend triple-slash reference
else
Postinstall->>FS: Leave formatted content unchanged
end
Note over Updater: When merging template into existing config
Updater->>FS: Parse existing config AST
Updater->>Updater: Collect config objects from mergeConfig args\n(defineConfig object args + plain objects)
Updater->>Updater: Select target config object (prefer one with `test`)
alt target has `test`
Updater->>Updater: Extract `coverage` from `test` (if present)
Updater->>Updater: Remove `coverage` from project test configs
Updater->>Updater: Prepend extracted `coverage` to top-level `test`
Updater->>Updater: Move `test` into workspace/projects entries
else
Updater->>Updater: No relocation needed
end
Updater->>FS: Write updated config file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (7)code/**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
Files:
**/*.{js,jsx,json,html,ts,tsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.@(test|spec).{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-17T08:11:04.287ZApplied to files:
🧬 Code graph analysis (1)code/addons/vitest/src/updateVitestFile.test.ts (1)
⏰ 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)
🔇 Additional comments (7)
Comment |
|
View your CI Pipeline Execution ↗ for commit 0384713
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/addons/vitest/src/postinstall.ts (2)
273-279: Use Storybook logger instead of console.error.Replace console.error with logger.error to follow code logging guidelines for code/*.
- console.error('Failed to install @storybook/nextjs-vite. Please install it manually'); + logger.error('Failed to install @storybook/nextjs-vite. Please install it manually');- console.error('Failed to install Playwright. Please install it manually'); + logger.error('Failed to install Playwright. Please install it manually');Also applies to: 324-331
470-479: Optional: preserve shebang if present.If a config file starts with a shebang, prepend the triple‑slash after it to avoid breaking executability.
- await writeFile( - rootConfig, - shouldAddReference - ? '/// <reference types="vitest/config" />\n' + formattedContent - : formattedContent - ); + const withRef = '/// <reference types="vitest/config" />\n' + formattedContent; + const contentToWrite = shouldAddReference + ? formattedContent.startsWith('#!') + ? formattedContent.replace(/^(#!.*\n)/, `$1/// <reference types="vitest/config" />\n`) + : withRef + : formattedContent; + await writeFile(rootConfig, contentToWrite);code/addons/vitest/src/updateVitestFile.ts (1)
252-304: Good: coverage hoisted to top‑level test, projects/workspace sanitized.This addresses the incorrect relocation of coverage into workspace/projects.
Two improvements:
- Guard against duplicate coverage at the template’s top‑level test if it already exists.
- Consider applying the same hoisting for defineConfig/object exports for consistency across patterns.
Minimal duplication guard diff:
- if (coverageProp && templateTestProp.value.type === 'ObjectExpression') { - templateTestProp.value.properties.unshift(coverageProp); - } + if (coverageProp && templateTestProp.value.type === 'ObjectExpression') { + const hasCoverageAlready = templateTestProp.value.properties.some( + (p) => + p.type === 'ObjectProperty' && + p.key.type === 'Identifier' && + p.key.name === 'coverage' + ); + if (!hasCoverageAlready) { + // Prefer cloning to avoid cross-AST node reuse + const node = (t as any).cloneNode + ? (t as any).cloneNode(coverageProp, /* deep */ true) + : coverageProp; + templateTestProp.value.properties.unshift(node); + } + }If helpful, I can draft a small helper (hoistCoverageFromTest(testObj: t.ObjectExpression, into: t.ObjectExpression)) and reuse it for defineConfig/object branches.
code/addons/vitest/src/updateVitestFile.test.ts (1)
10-16: Align vi.mock usage with repo test guidelines.Use spy: true for mocks to keep type‑safe spying per guidelines.
-vi.mock('storybook/internal/node-logger', () => ({ +vi.mock('storybook/internal/node-logger', { spy: true }, () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), }, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/addons/vitest/src/postinstall.ts(1 hunks)code/addons/vitest/src/updateVitestFile.test.ts(1 hunks)code/addons/vitest/src/updateVitestFile.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/addons/vitest/src/postinstall.tscode/addons/vitest/src/updateVitestFile.tscode/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/addons/vitest/src/postinstall.tscode/addons/vitest/src/updateVitestFile.tscode/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/addons/vitest/src/postinstall.tscode/addons/vitest/src/updateVitestFile.tscode/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, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/vitest/src/postinstall.tscode/addons/vitest/src/updateVitestFile.tscode/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/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 usingvi.mock()(e.g., filesystem, loggers)
Files:
code/addons/vitest/src/updateVitestFile.test.ts
🧠 Learnings (2)
📓 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: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.test.ts
🧬 Code graph analysis (1)
code/addons/vitest/src/updateVitestFile.test.ts (1)
code/addons/vitest/src/updateVitestFile.ts (2)
loadTemplate(9-16)updateConfigFile(79-325)
⏰ 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: merged
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/addons/vitest/src/postinstall.ts (1)
471-479: Correctly scope triple‑slash directive to vite.config files.This guards against adding it to vitest.config.* and avoids duplicates. Matches #32806.
code/addons/vitest/src/updateVitestFile.test.ts (2)
775-867: Tests validate coverage hoist with workspace.Solid snapshot reflecting coverage remaining top‑level and workspace receiving sanitized test.
869-961: Tests validate coverage hoist with projects.Confirms correct behavior for Vitest 3.2+ projects array.
Closes #32804, Closes #32806
What I did
Only add
/// <reference types="vitest/config" />to vite.config files,not vitest.config files, because vitest.config files already have the
vitest/config types available by default.
Modified the updateConfigFile function in updateVitestFile.ts to:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-32844-sha-03847137. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-32844-sha-03847137yann/vitest-addon-setup-fixes038471371761568509)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=32844Summary by CodeRabbit
Bug Fixes
Tests