CLI: Fix Next.js Vite automigration corrupting configs already using @storybook/nextjs-vite#34249
Conversation
…using @storybook/nextjs-vite The regex /@storybook\/nextjs/g matches as a substring inside @storybook/nextjs-vite, rewriting it to @storybook/nextjs-vite-vite. Projects that had both @storybook/nextjs and @storybook/nextjs-vite installed simultaneously (valid in SB9) would hit this because their main.ts already referenced @storybook/nextjs-vite. Fix: add a negative lookahead so only bare @storybook/nextjs is replaced.
05daa6f to
f5567b4
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTightened the Next.js→Next.js-Vite migration regex to avoid touching already-migrated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.ts`:
- Line 250: Move the inline mock setup
vi.mocked(mockPackageManager.getDependencyVersion).mockReturnValue('7.0.0') into
a beforeEach so all tests that require Vite installed share the same setup;
create a nested describe block (e.g., "when vite is installed") around those
tests and add a beforeEach that calls
vi.mocked(mockPackageManager.getDependencyVersion).mockReturnValue('7.0.0'), and
remove the inline mock from individual tests to keep mocking consistent and DRY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28520e9a-3086-4bc3-a854-4f5f5f11ec05
📒 Files selected for processing (2)
code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tscode/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.ts
| }; | ||
| `); | ||
|
|
||
| vi.mocked(mockPackageManager.getDependencyVersion).mockReturnValue('7.0.0'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tsRepository: storybookjs/storybook
Length of output: 143
🏁 Script executed:
sed -n '230,270p' code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tsRepository: storybookjs/storybook
Length of output: 1427
🏁 Script executed:
head -100 code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tsRepository: storybookjs/storybook
Length of output: 3064
🏁 Script executed:
sed -n '100,180p' code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tsRepository: storybookjs/storybook
Length of output: 2559
🏁 Script executed:
sed -n '180,250p' code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.tsRepository: storybookjs/storybook
Length of output: 2652
Move mock setup into a beforeEach block for consistency.
Line 250 defines mock behavior inline inside the test. This violates the test mocking convention that mock behaviors should be set up in beforeEach blocks. Consider creating a nested describe + beforeEach block for tests that require vite to be installed, since multiple tests in this suite need getDependencyVersion to return a non-null version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/cli-storybook/src/automigrate/fixes/nextjs-to-nextjs-vite.test.ts`
at line 250, Move the inline mock setup
vi.mocked(mockPackageManager.getDependencyVersion).mockReturnValue('7.0.0') into
a beforeEach so all tests that require Vite installed share the same setup;
create a nested describe block (e.g., "when vite is installed") around those
tests and add a beforeEach that calls
vi.mocked(mockPackageManager.getDependencyVersion).mockReturnValue('7.0.0'), and
remove the inline mock from individual tests to keep mocking consistent and DRY.
|
Regarding the CodeRabbit suggestion to move the inline mock into a |
@storybook/nextjs-vite
What does this PR do?
Fixes a bug in the
nextjs-to-nextjs-viteautomigration where the regex/@storybook\/nextjs/gmatches as a substring inside@storybook/nextjs-vite, rewriting it to@storybook/nextjs-vite-vite.How to reproduce
Projects that had both
@storybook/nextjsand@storybook/nextjs-viteinstalled simultaneously in SB9 (valid — some projects installed both transitionally) would already have@storybook/nextjs-viteas the framework name inmain.ts. Runningstorybook@10 upgradeon such a project corrupts the config:Fix
Add a negative lookahead to the regex so only bare
@storybook/nextjs(not already@storybook/nextjs-vite) is replaced:A regression test is included that verifies a config already using
@storybook/nextjs-viteis left untouched by the migration.Checklist
nextbranchSummary by CodeRabbit
Bug Fixes
Tests