CLI: Improve yarn 1 vs Berry detection and log package manager during init#34427
Merged
Sidnioulz merged 5 commits intoApr 2, 2026
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit 06315a0
☁️ Nx Cloud last updated this comment at |
Sidnioulz
commented
Apr 1, 2026
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.47 MB | 20.51 MB | 🚨 +34 KB 🚨 |
| Dependency size | 16.55 MB | 16.56 MB | 🚨 +4 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 806 KB | 🚨 +24 KB 🚨 |
| Dependency size | 67.70 MB | 67.98 MB | 🚨 +279 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 66.22 MB | 66.26 MB | 🚨 +39 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.28 MB | 🚨 +240 KB 🚨 |
| Dependency size | 37.03 MB | 37.07 MB | 🚨 +38 KB 🚨 |
| Bundle Size Analyzer | node | node |
eslint-plugin-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 20 | 20 | 0 |
| Self size | 131 KB | 131 KB | 🎉 -6 B 🎉 |
| Dependency size | 3.41 MB | 3.45 MB | 🚨 +31 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Contributor
There was a problem hiding this comment.
Pull request overview
Improves Storybook CLI package-manager handling by making Yarn Classic vs Yarn Berry detection more reliable and surfacing the detected package manager during sb init.
Changes:
- Add human-friendly package manager logging during
PreflightCheckCommand.execute(). - Improve Yarn version detection by prioritizing
packageManagerinpackage.json, using.yarnrc.ymlas a Berry signal, and fixing the Yarn 1 regex. - Add unit tests covering the new logging and Yarn Berry detection heuristics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
code/lib/create-storybook/src/commands/PreflightCheckCommand.ts |
Logs the detected package manager with a friendly label during init. |
code/lib/create-storybook/src/commands/PreflightCheckCommand.test.ts |
Adds a unit test asserting the package manager is logged. |
code/core/src/common/js-package-manager/JsPackageManagerFactory.ts |
Enhances Yarn Classic vs Berry detection using packageManager and .yarnrc.yml, and fixes the Yarn 1 version regex. |
code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts |
Adds coverage for Berry detection via .yarnrc.yml, packageManager, and version outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds upward-walking package.json detection and .yarnrc.yml probing to Yarn detection, reorders resolution priority (package.json → .yarnrc.yml → yarn --version), adjusts fallback behavior when yarn fails, expands Yarn/Berry tests, and logs the resolved package manager label in PreflightCheckCommand. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant JsPM as "JsPackageManagerFactory"
participant FS as "filesystem (read/find)"
participant YarnCmd as "yarn --version"
Caller->>JsPM: getYarnVersion(cwd)
rect rgba(100,150,200,0.5)
Note over JsPM,FS: Step 1 — walk upward for package.json with packageManager
JsPM->>FS: findUp("package.json") up to project root
FS-->>JsPM: path or undefined
alt package.json found
JsPM->>FS: readFileSync(package.json)
FS-->>JsPM: contents / parse error
alt packageManager declares yarn@<major>
JsPM-->>Caller: return parsed major (1 or 2)
end
end
end
rect rgba(150,150,100,0.5)
Note over JsPM,FS: Step 2 — check for .yarnrc.yml (Yarn Berry config)
JsPM->>FS: findUp(".yarnrc.yml") up to project root
FS-->>JsPM: path or undefined
alt .yarnrc.yml found
JsPM-->>Caller: return Yarn2
end
end
rect rgba(150,100,150,0.5)
Note over JsPM,YarnCmd: Step 3 — run `yarn --version`
JsPM->>YarnCmd: execute --version
YarnCmd-->>JsPM: output ("1.x"/"2.x"/"3.x"/error)
alt output starts with "1."
JsPM->>FS: findUp(".yarnrc.yml")
FS-->>JsPM: path or undefined
alt .yarnrc.yml exists
JsPM-->>Caller: return Yarn2
else
JsPM-->>Caller: return Yarn1
end
else output >= 2
JsPM-->>Caller: return parsed major
else command failed
JsPM->>FS: findUp(".yarnrc.yml")
FS-->>JsPM: path or undefined
alt .yarnrc.yml exists
JsPM-->>Caller: return Yarn2
else
JsPM-->>Caller: return undefined
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts (1)
121-130: PreferbeforeEachhelpers for these new mock setups.These scenarios reconfigure
findMock.up,executeCommandSyncMock, andreadFileSyncMockinline inside individualit(...)blocks. Moving that setup into nesteddescribe/beforeEachhelpers would keep the new Berry cases isolated and make the suite less stateful.As per coding guidelines, "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".Also applies to: 223-232, 324-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts` around lines 121 - 130, The test in JsPackageManagerFactory.test.ts is setting mocks inline in individual it(...) blocks (e.g., findMock.up, executeCommandSyncMock, readFileSyncMock); refactor by creating nested describe blocks for the "Berry"/Yarn-Berry scenarios and move those mock implementations into beforeEach hooks so each case gets isolated setup, and use afterEach or beforeEach to reset or restore mocks (vi.resetAllMocks()/mockReset()) between tests; if you need the real implementation (vi.importActual) do that inside the beforeEach and assign the conditional mock behavior there so tests at lines ~121, ~223, ~324 are simplified and stateful inline mock code is removed.
🤖 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/core/src/common/js-package-manager/JsPackageManagerFactory.ts`:
- Around line 257-277: getYarnVersionFromPackageJson currently stops at the
first package.json found (using find.up) even if that file lacks a
packageManager field; change it to walk upward from the provided cwd (or project
root fallback from getProjectRoot()) to the repository root and check every
package.json until one declares packageManager. Concretely, in
getYarnVersionFromPackageJson: start at cwd (or getProjectRoot()), loop up
directories until reaching the project root, at each directory check for
package.json (e.g., using find.up or fs.existsSync/readFileSync), parse and
inspect content.packageManager, and return 1 or 2 when a matching "yarn@X." is
found; only return undefined after the loop completes. Keep the same error
handling (try/catch around JSON.parse) and continue up on missing or unparsable
package.json files.
---
Nitpick comments:
In `@code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts`:
- Around line 121-130: The test in JsPackageManagerFactory.test.ts is setting
mocks inline in individual it(...) blocks (e.g., findMock.up,
executeCommandSyncMock, readFileSyncMock); refactor by creating nested describe
blocks for the "Berry"/Yarn-Berry scenarios and move those mock implementations
into beforeEach hooks so each case gets isolated setup, and use afterEach or
beforeEach to reset or restore mocks (vi.resetAllMocks()/mockReset()) between
tests; if you need the real implementation (vi.importActual) do that inside the
beforeEach and assign the conditional mock behavior there so tests at lines
~121, ~223, ~324 are simplified and stateful inline mock code is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c94cb81b-054e-4cc5-a08c-83dbec981396
📒 Files selected for processing (4)
code/core/src/common/js-package-manager/JsPackageManagerFactory.test.tscode/core/src/common/js-package-manager/JsPackageManagerFactory.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.test.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.ts
Agent-Logs-Url: https://github.com/storybookjs/storybook/sessions/522e5ba1-5029-4ce2-a32b-ca4928d88f1d Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts (1)
25-33: Consider addingspy: truetoempathic/walkmock for consistency with other mocks.The new mocks for
../utils/pathsandnode:fscorrectly use{ spy: true }, butempathic/walkfollows the existing pattern ofempathic/findwithout it. While this is consistent within the file, it deviates from the coding guideline to usespy: truefor all package mocks.♻️ Optional: Add spy: true for consistency
-vi.mock('empathic/walk'); +vi.mock('empathic/walk', { spy: true }); const walkMock = vi.mocked(walk);Note: This would also apply to the existing
empathic/findmock at line 22 for full consistency.As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts` around lines 25 - 33, The mock for 'empathic/walk' is missing the { spy: true } option; update the vi.mock call for 'empathic/walk' to include { spy: true } so walkMock (vi.mocked(walk)) is a spy like the other mocks, and also add { spy: true } to the existing 'empathic/find' vi.mock to keep both package mocks consistent with the project's Vitest guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/common/js-package-manager/JsPackageManagerFactory.test.ts`:
- Around line 25-33: The mock for 'empathic/walk' is missing the { spy: true }
option; update the vi.mock call for 'empathic/walk' to include { spy: true } so
walkMock (vi.mocked(walk)) is a spy like the other mocks, and also add { spy:
true } to the existing 'empathic/find' vi.mock to keep both package mocks
consistent with the project's Vitest guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 860fd4a8-e9ad-470b-b944-4f2ee7d3a632
📒 Files selected for processing (2)
code/core/src/common/js-package-manager/JsPackageManagerFactory.test.tscode/core/src/common/js-package-manager/JsPackageManagerFactory.ts
Contributor
Author
|
@yannbf should be good to re-review now, I've addressed the bunny feedback |
yannbf
approved these changes
Apr 2, 2026
Member
yannbf
left a comment
yannbf
left a comment
There was a problem hiding this comment.
LGTM! As long as sandbox generation works well I'm good with it!
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/^1\./)packageManagerfield inpackage.json.yarnrc.ymlpresencesb initgetProjectRoot()once ingetYarnVersion()and pass result to helper functions to avoid repeated filesystem traversalsSummary by CodeRabbit
Bug Fixes
New Features
Tests