CLI: Handle minimumReleaseAge conflicts across package managers#34769
Merged
Conversation
JReinhold
added
bug
ci:normal
maintenance
JReinhold
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Storybook CLI/create flow when pnpm’s minimumReleaseAge or Yarn Berry’s npmMinimalAgeGate would block installing a just-released Storybook version, by adding proactive prechecks and clearer, actionable error output.
Changes:
- Add a
precheckStorybookPackageInstall()hook to package managers and call it fromcreate-storybookpreflight andstorybook upgradebefore writing package.json changes. - Enhance pnpm/yarn2 proxies to detect minimum-age gating, offer interactive remediation (or auto-apply in non-interactive mode), and format specific install error codes into clearer messages.
- Add shared utility helpers (time parsing, “latest mature stable” selection, rerun guidance) plus unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| code/lib/create-storybook/src/initiate.ts | Thread error through the top-level failure handler (but currently leaves it unused). |
| code/lib/create-storybook/src/initiate.test.ts | Adjust mocks/imports (currently introduces unused vitest imports). |
| code/lib/create-storybook/src/commands/PreflightCheckCommand.ts | Call package-manager precheck before continuing create flow. |
| code/lib/create-storybook/src/commands/PreflightCheckCommand.test.ts | Assert precheck is invoked; stabilize isTTY for non-interactive behavior. |
| code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts | Re-throw HandledError install failures so they aren’t swallowed as generic failures. |
| code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts | Add coverage for rethrowing HandledError. |
| code/lib/cli-storybook/src/upgrade.ts | Invoke package-manager precheck before dependency rewrites during upgrade. |
| code/core/src/common/js-package-manager/JsPackageManager.ts | Add default no-op precheckStorybookPackageInstall() hook. |
| code/core/src/common/js-package-manager/Yarn2Proxy.ts | Implement Yarn Berry age-gate precheck and better YN0016 quarantined error formatting. |
| code/core/src/common/js-package-manager/Yarn2Proxy.test.ts | Add tests for age-gate behavior and quarantined-error formatting. |
| code/core/src/common/js-package-manager/PNPMProxy.ts | Implement pnpm minimumReleaseAge precheck and better ERR_PNPM_NO_MATURE_MATCHING_VERSION formatting. |
| code/core/src/common/js-package-manager/PNPMProxy.test.ts | Add tests for minimumReleaseAge behavior and formatted error output. |
| code/core/src/common/js-package-manager/util.ts | Introduce shared parsing/time/version-selection and rerun-guidance helpers. |
| code/core/src/common/js-package-manager/util.test.ts | Add unit tests for the new util helpers. |
Comments suppressed due to low confidence (1)
code/lib/create-storybook/src/initiate.ts:10
HandledErroris imported but never used in this file. Please remove the unused import (or use it for handled-error-specific output) to avoid unused-import lint warnings.
import {
HandledError,
type JsPackageManager,
PackageManagerName,
cache,
executeCommand,
} from 'storybook/internal/common';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
What about npm and Bun package manager handling? |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 60 | 60 | 0 |
| Self size | 20.50 MB | 20.52 MB | 🚨 +25 KB 🚨 |
| Dependency size | 32.84 MB | 32.84 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 194 | 194 | 0 |
| Self size | 907 KB | 908 KB | 🚨 +511 B 🚨 |
| Dependency size | 84.50 MB | 84.53 MB | 🚨 +26 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 82.99 MB | 83.02 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 61 | 61 | 0 |
| Self size | 1.08 MB | 1.08 MB | 🚨 +753 B 🚨 |
| Dependency size | 53.33 MB | 53.36 MB | 🚨 +25 KB 🚨 |
| Bundle Size Analyzer | node | node |
Contributor
valentinpalkovic
left a comment
valentinpalkovic
left a comment
There was a problem hiding this comment.
Automated CCG review (Codex + Gemini synthesis). Blocker / high / med inline comments below. See thread for rationale.
…okjs/storybook into jeppe/handle-minimum-release-age
…ndle-minimum-release-age
… add VTA to list of excludes
JReinhold
changed the title
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds minimum-release-age gating for Storybook installs across npm, pnpm, yarn (v1/v2) and bun. Introduces MinimumReleaseAgeHandledError, shared utilities for release-time checks, per-package-manager precheck/install handling, wiring into create/upgrade/preflight flows, and tests validating interactive/non-interactive behaviors and config updates. ChangesMinimum-Release-Age Gating for Storybook Installation
sequenceDiagram
participant User
participant CLI as PreflightCheck / Upgrade
participant PM as PackageManagerProxy
participant Registry as npm Registry
participant Config as Project Config (bun/pnpm/.yarnrc)
User->>CLI: run create/upgrade
CLI->>PM: precheckStorybookPackageInstall(storybookVersion, nonInteractive, installContext)
PM->>Config: read minimum-age and exclusions
alt gate configured and not excluded
PM->>Registry: fetch storybook time map
Registry-->>PM: version → publishedAt
PM->>PM: compute allowed compatible stable version
alt non-interactive
PM->>Config: update exclusions/preapproved list
Config-->>PM: write success
PM-->>CLI: return (continue)
else interactive
PM->>User: prompt (exclude now or rerun)
alt user: exclude
User-->>PM: exclude
PM->>Config: update exclusions
Config-->>PM: write success
PM-->>CLI: return (continue)
else user: rerun
User-->>PM: rerun
PM-->>CLI: throw MinimumReleaseAgeHandledError(with rerun guidance)
end
end
else gate unset or version allowed or already excluded
PM-->>CLI: return (no-op)
end
CLI->>PM: proceed with installDependencies()
PM->>PM: run install
alt install fails due to minimum-age enforcement
PM-->>CLI: throw MinimumReleaseAgeHandledError (includes cause)
else other failures
PM-->>CLI: original error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 6
♻️ Duplicate comments (7)
code/core/src/common/js-package-manager/Yarn2Proxy.ts (3)
604-622:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win[Duplicate] Missing
--jsonflag in config read, risking parse failures.Line 608 calls
yarn config get npmPreapprovedPackageswithout--json. Yarn prints config values in JavaScript-like format (single quotes) rather than valid JSON. WhileparsePreapprovedPackages(line 624) includes a regex fallback for this case, the primaryJSON.parsepath (line 626) will fail on real Yarn output when existing entries use single quotes, e.g.,['react', 'webpack'].Pass
--jsonto ensure valid JSON output, or rely solely on the regex fallback.🔧 Proposed fix
const result = await this.runInternalCommand( 'config', - ['get', 'npmPreapprovedPackages'], + ['get', 'npmPreapprovedPackages', '--json'], undefined, 'pipe' );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts` around lines 604 - 622, The getPreapprovedPackages method calls runInternalCommand('config', ['get', 'npmPreapprovedPackages'], ...) without the --json flag so Yarn can emit non-JSON (single-quoted) output that breaks JSON.parse in parsePreapprovedPackages; update the argument list passed to runInternalCommand in getPreapprovedPackages to include '--json' (e.g., ['get', 'npmPreapprovedPackages', '--json']) so the stdout is valid JSON before calling parsePreapprovedPackages, and keep the existing regex fallback intact for robustness.
293-293:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift[Duplicate] Precheck only inspects
storybookpackage time, missing scoped packages.Same drift risk as flagged for pnpm: the check uses
getPackageTimeMap('storybook')but the preapproval list covers@storybook/*and other packages. If scoped packages publish after the meta-package, they may still be inside the age gate when the precheck passes.Consider checking the youngest publish time across all managed Storybook packages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts` at line 293, The precheck only calls getPackageTimeMap('storybook') which misses scoped Storybook packages; update the logic in Yarn2Proxy (where timeMap is assigned) to fetch package times for all managed Storybook packages instead of just 'storybook' — e.g., iterate the full list of `@storybook/`* (from your preapproval/config list) and merge their time maps via getPackageTimeMap(packageName), then compute the most recent publish timestamp (youngest) across all those packages and use that value for the age gate check; update references to timeMap and any downstream comparisons to use this combined/youngest timestamp.
351-351: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win[Duplicate] Prompt label doesn't clarify project vs global scope.
The label "Update yarn config to preapprove Storybook packages" doesn't indicate that
yarn config setdefaults to the project-local.yarnrc.yml. Users may assume this is a global change.Suggested: "Update yarn config (for this project) to preapprove Storybook packages"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts` at line 351, The prompt label string in Yarn2Proxy.ts ("Update yarn config to preapprove Storybook packages") is ambiguous about scope; update the label property used in the UI/notification (the string assigned to label in the relevant object in Yarn2Proxy.ts) to explicitly indicate project-local scope, e.g. "Update yarn config (for this project) to preapprove Storybook packages", so users don't assume a global change.code/core/src/common/js-package-manager/PNPMProxy.ts (2)
311-311: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win[Duplicate] Prompt label doesn't clarify local vs global config scope.
The prompt reads "Update pnpm config to exclude Storybook packages" but line 568 passes
--location=project, so the mutation is project-local. Users may not realize this distinction.Suggested label: "Update pnpm config (for this project) to exclude Storybook packages"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/PNPMProxy.ts` at line 311, The prompt label "Update pnpm config to exclude Storybook packages" is ambiguous about scope; update the label string used where the prompt object is created in PNPMProxy.ts (the property named label in the prompt/options passed when building the command that later uses --location=project) to explicitly state project-local scope, e.g. "Update pnpm config (for this project) to exclude Storybook packages"; locate the label assignment in the prompt/options object in PNPMProxy.ts and replace the text accordingly so it matches the actual use of --location=project.
262-262:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift[Duplicate] Precheck only verifies
storybookpackage age, not scoped packages.The age gate check uses
getPackageReleaseTime('storybook', storybookVersion)but the exclusion list (line 559) covers@storybook/*and other packages. Because the Storybook meta-package can publish before/after the scoped packages in a release, this check may pass while@storybook/*packages remain inside the age window, causing post-precheck install failures.Consider checking the youngest publish time across all managed packages or always updating the exclusion list when a gate is detected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/PNPMProxy.ts` at line 262, The precheck currently only queries getPackageReleaseTime('storybook', storybookVersion) which can miss newer publishes in scoped packages like `@storybook/`*; update the check in PNPMProxy (where publishedAt is computed) to instead compute the most recent release time across all relevant managed/excluded packages (e.g., iterate the set of packages you manage or the exclusion list that contains `@storybook/`*), call getPackageReleaseTime for each package/version (use Promise.all), take the maximum/youngest timestamp, and use that value for the age gate comparison so scoped packages cannot be younger than the checked meta-package.code/lib/create-storybook/src/commands/PreflightCheckCommand.ts (1)
89-93:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift[Duplicate] CI and
--yessilently mutate package-manager security gates.When
nonInteractiveis true (CI, non-TTY, or--yes), the precheck automatically updatesminimumReleaseAgeExclude/npmPreapprovedPackageswithout explicit user consent. In CI environments, this:
- Weakens a supply-chain security control the user explicitly configured
- Leaves a dirty git state (modified
.npmrc/.yarnrc.yml)- Surprises operators expecting CI to fail closed rather than mutate config
Consider defaulting non-interactive mode to fail with rerun guidance, and require an explicit opt-in flag (e.g.,
--accept-package-manager-config-changes) to allow automatic config mutation.As per coding guidelines: past review comments identified this as a high-priority concern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/create-storybook/src/commands/PreflightCheckCommand.ts` around lines 89 - 93, The call to packageManager.precheckStorybookPackageInstall is currently passing nonInteractive: !!options.yes || !process.stdout.isTTY || !!isCI(), which allows CI/--yes paths to silently mutate package-manager security gates (minimumReleaseAgeExclude, npmPreapprovedPackages) and alter .npmrc/.yarnrc.yml; change the behavior so nonInteractive only means “no prompts” and does not permit automatic config mutation: add a new explicit boolean flag (suggest name --accept-package-manager-config-changes) and pass that into precheckStorybookPackageInstall (or into a new parameter like allowConfigMutation) so that when running in CI or with --yes the precheck will fail fast and emit clear rerun guidance unless the explicit opt-in flag is present; update call sites and option parsing to supply this explicit flag and ensure references to minimumReleaseAgeExclude and npmPreapprovedPackages are only modified when allowConfigMutation is true.code/lib/cli-storybook/src/upgrade.ts (1)
393-399:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift[Duplicate] Same CI/
--yesauto-mutation concern as in create flow.The
nonInteractivecomputation and precheck call mirror the behavior inPreflightCheckCommand.ts. In CI or with--yes, this automatically updates package-manager config (pnpm/yarn minimum-age exclusions) without explicit opt-in, which silently weakens security gates and creates dirty git state in CI environments.Consider requiring an explicit flag to allow automatic config mutation in non-interactive contexts.
As per coding guidelines: past review comments identified this as a high-priority concern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/cli-storybook/src/upgrade.ts` around lines 393 - 399, The current loop calling packageManager.precheckStorybookPackageInstall for each project uses a computed nonInteractive (based on options.yes, process.stdout.isTTY, isCI()) which allows automatic package-manager config mutation in CI/--yes scenarios; change this to require an explicit opt-in flag (e.g., options.allowAutoConfigMutation or cli flag --allow-config-mutation) before passing nonInteractive=true or invoking precheckStorybookPackageInstall when in a non-interactive environment. Concretely: update the loop around storybookProjects to check the new explicit flag and only set nonInteractive to true or call project.packageManager.precheckStorybookPackageInstall when that flag is present; otherwise call the precheck with nonInteractive=false or skip the call to avoid silent config mutations (referencing precheckStorybookPackageInstall, nonInteractive, options.yes, isCI()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/common/js-package-manager/BUNProxy.ts`:
- Around line 500-514: The getPackageTimeMap function can throw from
executeCommand or JSON.parse which causes precheck to hard-fail; update
getPackageTimeMap to catch any errors around the executeCommand call and the
JSON.parse/parsePackageTimeMap call and return null on failure so precheck falls
back to install-time handling. Specifically wrap the call to executeCommand and
the subsequent JSON.parse + parsePackageTimeMap invocation in a try/catch
(references: getPackageTimeMap, executeCommand, parsePackageTimeMap) and on any
thrown error swallow it (optionally log/debug) and return null instead of
letting the exception propagate.
- Around line 225-233: The current check in installDependencies incorrectly
classifies many Bun/npm errors because it throws MinimumReleaseAgeHandledError
whenever parsedError !== 'BUN error'; change the logic so you only wrap the
error as MinimumReleaseAgeHandledError when the parsedError explicitly indicates
the minimum-release-age condition (e.g., match a case-insensitive substring or
regex like /minimum[- ]?age|minimum release age/i), otherwise log the
parsedError (using logger.error) and rethrow the original error (or throw the
parsedError) so unrelated errors from parseErrorFromLogs(getErrorLogs(error))
are not reclassified; update the conditional inside the installDependencies
catch block accordingly (references: installDependencies, parseErrorFromLogs,
getErrorLogs, MinimumReleaseAgeHandledError).
In `@code/core/src/common/js-package-manager/NPMProxy.ts`:
- Around line 204-212: The current check in NPMProxy.installDependencies uses
parsedError !== 'NPM error', which wrongly treats parsed messages like 'NPM
error ERESOLVE ...' as non-NPM and wraps them in MinimumReleaseAgeHandledError;
change the condition to detect NPM errors by prefix (e.g.
parsedError.startsWith('NPM error')) so only non-NPM parsed errors are logged
and rethrown as MinimumReleaseAgeHandledError; update the conditional around
parseErrorFromLogs(getErrorLogs(error)) accordingly and keep references to
parseErrorFromLogs, getErrorLogs, installDependencies, and
MinimumReleaseAgeHandledError.
- Around line 408-435: Both getMinimumReleaseAge and getPackageTimeMap currently
let executeCommand (and JSON.parse in getPackageTimeMap) throw and abort
precheck; change both to fail open by wrapping the executeCommand calls in
try/catch and returning null on any error. Specifically, in getMinimumReleaseAge
(function name) catch errors from executeCommand and return null so
parsePositiveIntegerConfigValue is only called on successful stdout, and in
getPackageTimeMap (function name) catch errors from executeCommand and from
JSON.parse/parsePackageTimeMap and return null on any failure; keep using
parsePackageTimeMap for valid parsed data and preserve existing trimming/empty
checks. Ensure you reference executeCommand and parsePackageTimeMap when
locating the code to modify.
In `@code/lib/create-storybook/src/initiate.test.ts`:
- Around line 21-35: The tests use vi.mock calls without the required spy
option; update each vi.mock invocation (the three mocks for
'storybook/internal/telemetry', 'storybook/internal/core-server', and
'storybook/internal/node-logger') to pass the options object { spy: true } as
the third argument (or second when no factory is used) so mocks follow the
project's Vitest convention; ensure you keep the existing factory functions (the
arrow functions returning getServerPort/withTelemetry and the node-logger
object) and only add the { spy: true } option to those vi.mock calls.
In `@code/lib/create-storybook/src/initiate.ts`:
- Around line 233-235: The caller at the promise rejection uses
handleCommandFailure(options.logfile, error) but handleCommandFailure is defined
with a single parameter; update the function signature of handleCommandFailure
to accept both (logfile: string | undefined, error: unknown) and use both
parameters (logfile for logging and error for message/stack) or else change the
call to pass only the single value the function expects; modify either the
handleCommandFailure function definition or the caller in initiate.ts to make
the arity and parameter types consistent (refer to handleCommandFailure and the
.catch(...) caller).
---
Duplicate comments:
In `@code/core/src/common/js-package-manager/PNPMProxy.ts`:
- Line 311: The prompt label "Update pnpm config to exclude Storybook packages"
is ambiguous about scope; update the label string used where the prompt object
is created in PNPMProxy.ts (the property named label in the prompt/options
passed when building the command that later uses --location=project) to
explicitly state project-local scope, e.g. "Update pnpm config (for this
project) to exclude Storybook packages"; locate the label assignment in the
prompt/options object in PNPMProxy.ts and replace the text accordingly so it
matches the actual use of --location=project.
- Line 262: The precheck currently only queries
getPackageReleaseTime('storybook', storybookVersion) which can miss newer
publishes in scoped packages like `@storybook/`*; update the check in PNPMProxy
(where publishedAt is computed) to instead compute the most recent release time
across all relevant managed/excluded packages (e.g., iterate the set of packages
you manage or the exclusion list that contains `@storybook/`*), call
getPackageReleaseTime for each package/version (use Promise.all), take the
maximum/youngest timestamp, and use that value for the age gate comparison so
scoped packages cannot be younger than the checked meta-package.
In `@code/core/src/common/js-package-manager/Yarn2Proxy.ts`:
- Around line 604-622: The getPreapprovedPackages method calls
runInternalCommand('config', ['get', 'npmPreapprovedPackages'], ...) without the
--json flag so Yarn can emit non-JSON (single-quoted) output that breaks
JSON.parse in parsePreapprovedPackages; update the argument list passed to
runInternalCommand in getPreapprovedPackages to include '--json' (e.g., ['get',
'npmPreapprovedPackages', '--json']) so the stdout is valid JSON before calling
parsePreapprovedPackages, and keep the existing regex fallback intact for
robustness.
- Line 293: The precheck only calls getPackageTimeMap('storybook') which misses
scoped Storybook packages; update the logic in Yarn2Proxy (where timeMap is
assigned) to fetch package times for all managed Storybook packages instead of
just 'storybook' — e.g., iterate the full list of `@storybook/`* (from your
preapproval/config list) and merge their time maps via
getPackageTimeMap(packageName), then compute the most recent publish timestamp
(youngest) across all those packages and use that value for the age gate check;
update references to timeMap and any downstream comparisons to use this
combined/youngest timestamp.
- Line 351: The prompt label string in Yarn2Proxy.ts ("Update yarn config to
preapprove Storybook packages") is ambiguous about scope; update the label
property used in the UI/notification (the string assigned to label in the
relevant object in Yarn2Proxy.ts) to explicitly indicate project-local scope,
e.g. "Update yarn config (for this project) to preapprove Storybook packages",
so users don't assume a global change.
In `@code/lib/cli-storybook/src/upgrade.ts`:
- Around line 393-399: The current loop calling
packageManager.precheckStorybookPackageInstall for each project uses a computed
nonInteractive (based on options.yes, process.stdout.isTTY, isCI()) which allows
automatic package-manager config mutation in CI/--yes scenarios; change this to
require an explicit opt-in flag (e.g., options.allowAutoConfigMutation or cli
flag --allow-config-mutation) before passing nonInteractive=true or invoking
precheckStorybookPackageInstall when in a non-interactive environment.
Concretely: update the loop around storybookProjects to check the new explicit
flag and only set nonInteractive to true or call
project.packageManager.precheckStorybookPackageInstall when that flag is
present; otherwise call the precheck with nonInteractive=false or skip the call
to avoid silent config mutations (referencing precheckStorybookPackageInstall,
nonInteractive, options.yes, isCI()).
In `@code/lib/create-storybook/src/commands/PreflightCheckCommand.ts`:
- Around line 89-93: The call to packageManager.precheckStorybookPackageInstall
is currently passing nonInteractive: !!options.yes || !process.stdout.isTTY ||
!!isCI(), which allows CI/--yes paths to silently mutate package-manager
security gates (minimumReleaseAgeExclude, npmPreapprovedPackages) and alter
.npmrc/.yarnrc.yml; change the behavior so nonInteractive only means “no
prompts” and does not permit automatic config mutation: add a new explicit
boolean flag (suggest name --accept-package-manager-config-changes) and pass
that into precheckStorybookPackageInstall (or into a new parameter like
allowConfigMutation) so that when running in CI or with --yes the precheck will
fail fast and emit clear rerun guidance unless the explicit opt-in flag is
present; update call sites and option parsing to supply this explicit flag and
ensure references to minimumReleaseAgeExclude and npmPreapprovedPackages are
only modified when allowConfigMutation is true.
🪄 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: 56bb5196-b750-402e-8434-ee194e582542
📒 Files selected for processing (20)
code/core/src/common/js-package-manager/BUNProxy.test.tscode/core/src/common/js-package-manager/BUNProxy.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/core/src/common/js-package-manager/MinimumReleaseAgeHandledError.tscode/core/src/common/js-package-manager/NPMProxy.test.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/common/js-package-manager/PNPMProxy.test.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.test.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/common/js-package-manager/index.tscode/core/src/common/js-package-manager/util.test.tscode/core/src/common/js-package-manager/util.tscode/lib/cli-storybook/src/upgrade.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.test.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.test.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.tscode/lib/create-storybook/src/initiate.test.tscode/lib/create-storybook/src/initiate.ts
valentinpalkovic
approved these changes
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
reviewed
May 13, 2026
yannbf
approved these changes
May 13, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/common/js-package-manager/NPMProxy.ts (1)
388-415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrecheck helpers must fail open, not abort the flow.
Both
getMinimumReleaseAgeandgetPackageTimeMapcan throw on npm config/network failures or invalid JSON, aborting the entirecreate-storybook/upgradeflow instead of gracefully degrading to install-time handling.Wrap both methods' bodies in try/catch blocks that log the error and return
nullon any failure.🛡️ Recommended fix to add try/catch wrappers
private async getMinimumReleaseAge(): Promise<number | null> { + try { const result = await executeCommand({ command: 'npm', args: ['config', 'get', 'min-release-age', ...NPM_CONFIG_WORKSPACE_ARGS], cwd: this.cwd, stdio: 'pipe', }); return parsePositiveIntegerConfigValue( typeof result.stdout === 'string' ? result.stdout : undefined ); + } catch (error) { + logger.debug(`Failed to read npm min-release-age: ${String(error)}`); + return null; + } } private async getPackageTimeMap(packageName: string): Promise<Record<string, string> | null> { + try { const result = await executeCommand({ command: 'npm', args: ['info', packageName, 'time', '--json'], cwd: this.cwd, stdio: 'pipe', }); const normalizedValue = typeof result.stdout === 'string' ? result.stdout.trim() : ''; if (!normalizedValue) { return null; } return parsePackageTimeMap(JSON.parse(normalizedValue)); + } catch (error) { + logger.debug(`Failed to fetch npm time metadata for ${packageName}: ${String(error)}`); + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/NPMProxy.ts` around lines 388 - 415, Wrap the bodies of getMinimumReleaseAge and getPackageTimeMap in try/catch blocks so npm/network/JSON errors don't abort the flow; inside each catch, log the error (using the same logger/context available in the class) and return null to fail open, preserving the existing return contract and keeping the rest of the flow intact.
🧹 Nitpick comments (1)
code/core/src/common/js-package-manager/BUNProxy.test.ts (1)
14-26: 💤 Low valueAdd
spy: trueoption to thenode-loggermock for consistency with mocking guidelines.The mock for
storybook/internal/node-loggershould use{ spy: true }like thecommand.tsmock on line 28.Suggested fix
-vi.mock('storybook/internal/node-logger', () => ({ +vi.mock('storybook/internal/node-logger', { spy: true }, () => ({ prompt: { executeTaskWithSpinner: vi.fn(), getPreferredStdio: vi.fn(() => 'inherit'), select: vi.fn(), }, logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn(), }, }));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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/BUNProxy.test.ts` around lines 14 - 26, The vitest mock for 'storybook/internal/node-logger' is missing the spy option; update the vi.mock call that defines the mock for 'storybook/internal/node-logger' to include the `{ spy: true }` option (matching how `vi.mock()` is used for `command.ts`) so the mock is created as a spy while keeping the same mocked shape (prompt.executeTaskWithSpinner, getPreferredStdio, select, and logger methods).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/core/src/common/js-package-manager/NPMProxy.ts`:
- Around line 388-415: Wrap the bodies of getMinimumReleaseAge and
getPackageTimeMap in try/catch blocks so npm/network/JSON errors don't abort the
flow; inside each catch, log the error (using the same logger/context available
in the class) and return null to fail open, preserving the existing return
contract and keeping the rest of the flow intact.
---
Nitpick comments:
In `@code/core/src/common/js-package-manager/BUNProxy.test.ts`:
- Around line 14-26: The vitest mock for 'storybook/internal/node-logger' is
missing the spy option; update the vi.mock call that defines the mock for
'storybook/internal/node-logger' to include the `{ spy: true }` option (matching
how `vi.mock()` is used for `command.ts`) so the mock is created as a spy while
keeping the same mocked shape (prompt.executeTaskWithSpinner, getPreferredStdio,
select, and logger methods).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34a2b5a2-a300-41c2-9685-4c1e446f742e
📒 Files selected for processing (18)
code/core/src/common/js-package-manager/BUNProxy.test.tscode/core/src/common/js-package-manager/BUNProxy.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/core/src/common/js-package-manager/NPMProxy.test.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/common/js-package-manager/PNPMProxy.test.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/Yarn1Proxy.test.tscode/core/src/common/js-package-manager/Yarn1Proxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.test.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/server-errors.tscode/core/src/storybook-error.tscode/lib/cli-storybook/src/upgrade.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.test.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.test.tscode/lib/create-storybook/src/commands/PreflightCheckCommand.ts
💤 Files with no reviewable changes (2)
- code/core/src/common/js-package-manager/Yarn1Proxy.ts
- code/core/src/common/js-package-manager/Yarn1Proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts
- code/lib/cli-storybook/src/upgrade.ts
- code/lib/create-storybook/src/commands/PreflightCheckCommand.test.ts
- code/lib/create-storybook/src/commands/PreflightCheckCommand.ts
- code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts
- code/core/src/common/js-package-manager/PNPMProxy.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/common/js-package-manager/BUNProxy.ts (1)
477-491:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
getPackageTimeMapin try/catch to avoid aborting precheck on metadata fetch failures.Both
executeCommandandJSON.parsecan throw. An unhandled exception here aborts precheck unexpectedly instead of gracefully falling back to install-time handling.Suggested fix
private async getPackageTimeMap(packageName: string): Promise<Record<string, string> | null> { + try { const result = await executeCommand({ command: 'npm', cwd: this.cwd, args: ['info', packageName, 'time', '--json'], stdio: 'pipe', }); const normalizedValue = typeof result.stdout === 'string' ? result.stdout.trim() : ''; if (!normalizedValue) { return null; } return parsePackageTimeMap(JSON.parse(normalizedValue)); + } catch (error) { + logger.debug(`Failed to fetch npm time metadata for ${packageName}: ${String(error)}`); + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/common/js-package-manager/BUNProxy.ts` around lines 477 - 491, The getPackageTimeMap method can throw from executeCommand or JSON.parse which aborts precheck; wrap the body of getPackageTimeMap in a try/catch, call executeCommand and JSON.parse inside the try, pass the parsed value to parsePackageTimeMap as before, and on any error catch it, optionally log or debug the error, and return null so precheck continues instead of throwing; reference getPackageTimeMap, executeCommand, JSON.parse, and parsePackageTimeMap when locating where to add the try/catch and error-handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/core/src/common/js-package-manager/BUNProxy.ts`:
- Around line 477-491: The getPackageTimeMap method can throw from
executeCommand or JSON.parse which aborts precheck; wrap the body of
getPackageTimeMap in a try/catch, call executeCommand and JSON.parse inside the
try, pass the parsed value to parsePackageTimeMap as before, and on any error
catch it, optionally log or debug the error, and return null so precheck
continues instead of throwing; reference getPackageTimeMap, executeCommand,
JSON.parse, and parsePackageTimeMap when locating where to add the try/catch and
error-handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dddfddb7-7107-4eb0-971c-3b772a11ec85
📒 Files selected for processing (6)
code/core/src/common/js-package-manager/BUNProxy.test.tscode/core/src/common/js-package-manager/BUNProxy.tscode/core/src/common/js-package-manager/NPMProxy.test.tscode/core/src/common/js-package-manager/Yarn1Proxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/server-errors.ts
💤 Files with no reviewable changes (1)
- code/core/src/common/js-package-manager/Yarn1Proxy.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/server-errors.ts
- code/core/src/common/js-package-manager/NPMProxy.test.ts
- code/core/src/common/js-package-manager/BUNProxy.test.ts
- code/core/src/common/js-package-manager/Yarn2Proxy.ts
This was referenced May 13, 2026
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.
Closes #
What I did
Adds minimum-release-age handling for Storybook package installs across the package managers we currently support in this flow.
create-storybookandupgradecontinue.minimumReleaseAgenpmMinimalAgeGateminimumReleaseAgemin-release-ageCurrent Storybook allowlist/exclude set:
storybook@storybook/*eslint-plugin-storybook@chromatic-com/storybookNotes:
minimumReleaseAgeExcludesinside[install].Init prompt:
Selected first option
Selected second option
Error during final dependency installation
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
The old version of this section duplicated create vs upgrade flows per package manager. That was getting long while still missing important late-added behaviors like "already allowlisted should not warn again". The steps below are organized by behavior so a human or agent can execute them as a matrix instead of as four separate scripts.
Use the current canary from this PR:
0.0.0-pr-34769-sha-457f45d6Expected Storybook allowlist/exclude entries after the update path succeeds:
storybook@storybook/*eslint-plugin-storybook@chromatic-com/storybookShared behavior matrix
Recommended manual coverage
create-storybookwith that package manager forced and verify precheck behavior.upgradescenario for pnpm and Yarn Berry and verify the precheck happens before package rewrites, with the same update/rerun/cancel behavior.minimumReleaseAgeExcludesremains inside the[install]section whenbunfig.tomlcontains multiple TOML sections.Package-manager-specific setup commands
pnpm create / repeat-upgrade setup
Verify the config after choosing the update path:
Yarn Berry create / repeat-upgrade setup
Verify the config after choosing the update path:
Bun create / repeat-upgrade setup
Verify the config after choosing the update path:
npm create / repeat-upgrade setup
Verify rerun guidance or install-time handled errors; do not expect an automatic allowlist update option.
Agent-ready verification prompt
Prompt to paste into an agent
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-34769-sha-457f45d6. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34769-sha-457f45d6 sandboxor in an existing project withnpx storybook@0.0.0-pr-34769-sha-457f45d6 upgrade.More information
0.0.0-pr-34769-sha-457f45d6jeppe/handle-minimum-release-age457f45d61778577695)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=34769Summary by CodeRabbit
New Features
Bug Fixes
Tests