Repository navigation
CLI: Fix silent hang in deferred addon configuration during upgrade - #35423
Merged
Merged
Conversation
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 (1)
📝 WalkthroughWalkthroughThe upgrade flow now tracks collateral failures across dependency updates, automigrations, installation, and deduplication, reports them through logs and telemetry, and throws a handled error afterward. The accessibility postinstall command explicitly configures its standard streams to avoid stdin-related hangs. ChangesUpgrade failure tracking
Postinstall command streams
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant upgrade
participant logUpgradeResults
participant telemetry
upgrade->>upgrade: collect collateralFailures
upgrade->>logUpgradeResults: pass collateralFailures
upgrade->>telemetry: send collateralFailedSteps
upgrade->>upgrade: throw HandledError when failures remain
✨ 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/addons/vitest/src/postinstall.ts (1)
160-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
useRemotePkgis alwaysfalsein this call — dead/misleading condition.This call sits inside
if (!options.skipInstall) { ... }, sooptions.skipInstallis guaranteed falsy here, makinguseRemotePkg: !!options.skipInstallalwaysfalse. It matches the runtime default anyway, but it reads as ifuseRemotePkgvaries withskipInstall, which it never can in this branch (theskipInstall === truebranch just logs a warning and skipsinstallPlaywrightentirely). Either drop the option here or clarify the intent.🧹 Proposed fix
await addonVitestService.installPlaywright({ yes: options.yes, - useRemotePkg: !!options.skipInstall, });🤖 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/addons/vitest/src/postinstall.ts` around lines 160 - 167, The installPlaywright call in postinstall logic has a dead conditional because it is inside the !options.skipInstall branch, so useRemotePkg: !!options.skipInstall is always false and misleading. Update the AddonVitestService.installPlaywright invocation to either omit useRemotePkg entirely or pass a value that reflects an actual runtime decision, and keep the intent clear in the surrounding options handling so the branch in postinstall.ts matches the behavior of skipInstall and skipDependencyManagement.code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
22-57: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnconditional retry on "No files matched" can recurse forever with
--yesor an explicit--glob.The catch block retries
runStoriesCodemod(options)with the exact sameoptionson every "No files matched" error. Whenglobis explicitly provided, oryes: true(common in CI),globStringresolves identically each time with no interactive prompt to change it — if it matches zero files, this never terminates.🐛 Suggested fix: bound the retry and only allow it when a new glob can actually be entered
async function runStoriesCodemod(options: { dryRun: boolean | undefined; packageManager: JsPackageManager; useSubPathImports: boolean; previewConfigPath: string; yes: boolean | undefined; glob: string | undefined; -}) { +}, retried = false) { const { dryRun, packageManager, yes, glob, ...codemodOptions } = options; try { const inSandbox = optionalEnvToBoolean(process.env.IN_STORYBOOK_SANDBOX) ?? false; let globString = glob ?? '**/*.{stories,story}.{js,jsx,ts,tsx,mjs,mjsx,mts,mtsx}'; if (!glob && inSandbox) { globString = '{stories,src}/**/{Button,Header,Page,button,header,page}.stories.*'; - } else if (!glob && !yes) { + } else if (!glob && !yes && !retried) { logger.log('Please enter the glob for your stories to migrate'); globString = await prompt.text({ message: 'glob', initialValue: globString, }); } logger.step('Applying codemod on your stories, this might take some time...'); await packageManager.runPackageCommand({ args: ['storybook', 'migrate', 'csf-2-to-3', `--glob="${globString}"`], }); await runCodemod(globString, (info) => storyToCsfFactory(info, codemodOptions), { dryRun, }); } catch (err: any) { - if (err.message === 'No files matched') { - await runStoriesCodemod(options); + if (err.message === 'No files matched' && !retried) { + await runStoriesCodemod(options, true); } else { throw err; } } }🤖 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/codemod/csf-factories.ts` around lines 22 - 57, The retry path in `runStoriesCodemod` can loop forever when `No files matched` happens with an explicit `glob` or `yes: true`, since it re-invokes the same options without any chance to change `globString`. Update the `catch` logic to only retry when a new glob can actually be prompted (for example, when `glob` was not provided and `yes` is false), and otherwise rethrow or fail fast. Use the `runStoriesCodemod`, `globString`, and `options` flow to locate and bound this fallback.
♻️ Duplicate comments (1)
.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/autopilot-state.json (1)
1-10: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winLocal developer path/username leaked into committed state file.
project_pathembeds an absolute local filesystem path including the developer's username (/Users/valentinpalkovic/...). Combined with being an ephemeral agent-tool session artifact, this shouldn't be tracked in version control.Same fix as flagged in
code/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.json: remove these.omc/state/**files from the PR and gitignore the directory.🤖 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 @.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/autopilot-state.json around lines 1 - 10, The committed autopilot state artifact leaks a local absolute path via the project_path field, so remove this .omc/state/sessions/* JSON file from the PR and ensure the entire .omc/state directory is ignored going forward. Use the session-state tracking files like autopilot-state.json as the target for cleanup so these ephemeral agent artifacts are not committed again.
🧹 Nitpick comments (13)
code/frameworks/nextjs/src/export-mocks/link/index.tsx (1)
7-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing props instead of
any.
React.forwardRef<HTMLAnchorElement, any>drops type safety forhref,onClick, etc. A minimal props interface (mirroring the subset of Next'sLinkPropsactually used) would catch misuse at the call site without much extra code.♻️ Example typed props
-const MockLink = React.forwardRef<HTMLAnchorElement, any>(function MockLink( +interface MockLinkProps extends React.AnchorHTMLAttributes<HTMLAnchorElement> { + href: string | { pathname?: string; query?: Record<string, string>; hash?: string }; + as?: unknown; + replace?: boolean; + scroll?: boolean; + shallow?: boolean; + prefetch?: boolean; + passHref?: boolean; + legacyBehavior?: boolean; + locale?: string; +} + +const MockLink = React.forwardRef<HTMLAnchorElement, MockLinkProps>(function MockLink(🤖 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/frameworks/nextjs/src/export-mocks/link/index.tsx` around lines 7 - 23, The MockLink component currently uses React.forwardRef<HTMLAnchorElement, any>, which removes type safety for its Link props. Replace the any in MockLink with a proper props interface for the subset of Next.js LinkProps that this mock actually uses, so href, onClick, children, and related fields are validated at call sites. Keep the change focused on MockLink and its forwardRef signature so the mock stays compatible while restoring useful TypeScript checks.code/lib/cli-storybook/src/automigrate/multi-project.test.ts (1)
36-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock without
spy: true.
./fixesis mocked via inline factory withoutspy: true, inconsistent with the guideline used elsewhere for package/file mocks.Based on 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/lib/cli-storybook/src/automigrate/multi-project.test.ts` around lines 36 - 46, The `vi.mock('./fixes', ...)` setup in `multi-project.test.ts` should follow the Vitest guideline by using `spy: true` for file/package mocks. Update the existing mock for `./fixes` so it keeps the same mocked `allFixes` behavior while enabling spying semantics, matching the pattern used in other tests and avoiding an inline factory-only mock.Source: Coding guidelines
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock without
spy: true, implementation not inbeforeEach.Unlike the other mocks in this file,
postinstallAddonis stubbed via an inline factory withoutspy: true, and its resolved value is set at mock-declaration time rather than insidebeforeEach.Based on coding guidelines: "Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests" and "Implement mock behaviors in `beforeEach` blocks in Vitest tests".♻️ Suggested alignment with other mocks in the file
-vi.mock('../../../cli-storybook/src/postinstallAddon', () => ({ - postinstallAddon: vi.fn().mockResolvedValue(undefined), -})); +vi.mock('../../../cli-storybook/src/postinstallAddon', { spy: true });And in
beforeEach:+ const { postinstallAddon } = await import('../../../cli-storybook/src/postinstallAddon'); + vi.mocked(postinstallAddon).mockResolvedValue(undefined);🤖 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/AddonConfigurationCommand.test.ts` around lines 17 - 19, The `postinstallAddon` mock in `AddonConfigurationCommand.test` is set up inconsistently with the other Vitest mocks: it uses an inline factory instead of `spy: true`, and its resolved behavior is declared at mock time rather than in `beforeEach`. Update the `vi.mock` for `postinstallAddon` to follow the same `spy: true` pattern used elsewhere in this test file, and move the `mockResolvedValue(undefined)` setup into `beforeEach` so the mock behavior is initialized per test alongside the other mocks.Source: Coding guidelines
.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json (1)
1-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInternal tooling state file committed to repo.
This appears to be an auto-generated session/mission state artifact from local agent tooling (
.omc/state/sessions/...), not application source. Several similar.omc/**state files are included in this changeset. Consider adding.omc/(or the specific state directories) to.gitignoreto avoid committing ephemeral tool state.🤖 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 @.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json around lines 1 - 217, The committed .omc session state is generated tooling data, not application code, so it should not be tracked. Remove this mission-state artifact from the changeset and add .omc/ (or the relevant state paths) to .gitignore so future runs of the session tooling do not reintroduce files like mission-state.json.code/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json (1)
1-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAgent tooling state file appears to be an unintended commit.
This is a per-session
.omcagent state artifact (worker/task counters, timeline of agent runs) rather than product source. The PR's full file list contains many similar files (.omc/state/*,code/.omc/state/*,.codex/environments/environment.toml,*/agent-replay-*.jsonl,project-memory.json, etc.), suggesting these are locally generated tooling artifacts that got swept into the commit. Consider adding these paths to.gitignoreso they don't churn the repo on every agent run.🤖 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/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json` around lines 1 - 133, The session state artifact under the .omc state tree should not be committed, so remove this generated mission-state file from the change set and exclude similar tooling artifacts from future commits. Update the repo’s ignore rules (for example around .omc/state and other agent-generated files like replay logs or project memory artifacts) so these files are not tracked, and keep the cleanup focused on the generated state data rather than product code.code/core/src/csf-tools/CsfFile.ts (1)
843-871: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeuristic
rootObject.name === 'preview'check misses aliased imports.This new safeguard only fires when the local variable is literally named
preview. If a user aliases the import (import { preview as sbPreview } from '../wrong/path'), the same erroneous CSF-factory usage would silently pass through without raisingBadMetaError, defeating the fix's intent.Consider checking the import specifier's original/imported name (e.g.,
specifier.imported.namefor named imports) rather than the local binding name, to make this robust against aliasing.🤖 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/csf-tools/CsfFile.ts` around lines 843 - 871, The CSF factory safeguard in CsfFile currently only checks the local binding name with rootObject.name === 'preview', so aliased imports can bypass BadMetaError. Update the import-validation logic around the rootObject/configParent handling to inspect the actual imported specifier name from the ImportDeclaration (for example, the named import’s imported name) instead of relying on the local alias. Keep the existing BadMetaError path in place, but make it trigger for any alias of the preview import coming from the wrong source.code/lib/cli-storybook/src/codemod/helpers/csf-factories-utils.ts (1)
134-147: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClearing
firstNode.commentsmay drop trailing comments too.Setting
firstNode.comments = []removes all comments on the node, not just the leading ones just transferred. IffirstNodealso carries trailing comments (recast associates both types viacomments), they'd be lost.♻️ Suggested fix: only remove the transferred comments
- (importDecl as t.Node & { comments?: t.Comment[] }).comments = firstNode.leadingComments; - // Clear comments from the original first node to avoid duplication - firstNode.leadingComments = []; - firstNode.comments = []; + const movedComments = firstNode.leadingComments; + (importDecl as t.Node & { comments?: t.Comment[] }).comments = movedComments; + // Clear only the moved comments from the original first node to avoid duplication + firstNode.leadingComments = []; + firstNode.comments = (firstNode.comments ?? []).filter((c) => !movedComments.includes(c));🤖 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/codemod/helpers/csf-factories-utils.ts` around lines 134 - 147, The addImportToTop helper is clearing too much comment metadata from the first program node. Update addImportToTop so it only removes the leading comments that were transferred to importDecl, and do not reset firstNode.comments wholesale; preserve any trailing or other comments attached to firstNode. Use the existing addImportToTop and firstNode.leadingComments/comments handling to keep the comment transfer narrowly scoped.code/addons/vitest/src/postinstall.test.ts (1)
5-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extending coverage for aliased/namespace imports.
Current tests cover default-named import usage and absence of the plugin, but
isConfigAlreadySetupalso handles aliased/namespace import specifiers and thestorybookTestidentifier fallback (inpostinstall.ts). Adding a case with an aliased import (e.g.import { storybookTest as st } from '...') would strengthen confidence in the detection logic.🤖 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/addons/vitest/src/postinstall.test.ts` around lines 5 - 49, Add a test case in postinstall.test.ts to cover the aliased/namespace import path handled by isConfigAlreadySetup in postinstall.ts. Specifically, verify detection still returns true when the addon plugin is imported under a different local name (for example, an aliased storybookTest import) and used in the Vitest config, so the identifier fallback logic is exercised alongside the existing default import and missing-plugin cases.code/addons/vitest/src/postinstall.ts (1)
434-481: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNamespace/member-expression plugin usage isn't detected.
If the config imports the plugin as a namespace (
import * as vitestPlugin from '@storybook/addon-vitest/vitest-plugin') and calls it asvitestPlugin.storybookTest(...), theCallExpressioncheck only matchesIdentifiercallees, so a namespace-imported, member-accessed call won't be recognized as "already configured," potentially causing an unnecessary/duplicate config rewrite. This is an edge case unlikely in generated templates but worth a defensive check given this function gates whether existing user configs get silently skipped or rewritten.🤖 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/addons/vitest/src/postinstall.ts` around lines 434 - 481, The config detection in isConfigAlreadySetup only recognizes direct Identifier calls, so namespace-imported plugin usage like member-accessed calls is missed. Update the CallExpression traversal in isConfigAlreadySetup to also detect member-expression callees for the Storybook test plugin, including namespace imports from the existing import scan, so both direct calls and cases like pluginNamespace.storybookTest(...) are treated as already configured..omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.json (1)
1-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLikely unintended commit of internal tool state.
This file (and the numerous sibling
.omc/state/*.json,.omc/sessions/*.jsonfiles in this PR) looks like autogenerated session/agent tracking state from a local dev-tooling/agent framework, not product source. Consider adding.omc/(andcode/.omc/) to.gitignorerather than committing these artifacts.🤖 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 @.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.json around lines 1 - 116, The issue is that autogenerated internal agent/session state under .omc is being committed as source. Update the repo hygiene so .omc/ (and any nested code/.omc/ path) is ignored, and remove the tracked state artifacts from the PR while keeping the actual source tree unchanged. Use the .omc/state/sessions/subagent-tracking-state.json file and its sibling .omc JSON artifacts as the affected symbols to locate and clean up these generated files..omc/plans/autopilot-impl.md (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded personal absolute path in committed plan doc.
/Users/valentinpalkovic/Projects/storybook/.agtx/worktrees/...is a machine-specific path (also exposes a contributor's local username) baked into a plan file that appears to be checked into the repo. It won't be reproducible for anyone else running this plan and unnecessarily surfaces a local directory layout/username.♻️ Suggested fix
- node /Users/valentinpalkovic/Projects/storybook/.agtx/worktrees/c1adc443-regression-bug--upgrade--CLI/code/lib/cli-storybook/dist/bin/index.js upgrade + node "$(git rev-parse --show-toplevel)/code/lib/cli-storybook/dist/bin/index.js" upgradeAlso applies to: 662-666
🤖 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 @.omc/plans/autopilot-impl.md around lines 78 - 82, The plan document contains a hardcoded machine-specific absolute path in the CLI repro step, which makes it non-portable and exposes a local username. Update the referenced command in the plan content to use a repo-relative or placeholder-based path instead of the personal `/Users/...` location, and make the same replacement in the duplicate repro block mentioned by the comment so the instructions stay reusable.code/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.test.ts (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock setup doesn't follow the repo's Vitest mocking conventions.
The
node:urlmock omitsspy: true, and its behavior is hardcoded in the factory instead of viabeforeEach. Thevi.stubGlobal('import', ...)call also has no matchingvi.unstubAllGlobals()cleanup, risking state bleed into other tests sharing the worker.♻️ Suggested cleanup
-vi.mock('node:url', () => ({ - fileURLToPath: vi.fn(() => '/fake/mocker-runtime.js'), -})); - -// Mock import.meta.resolve -vi.stubGlobal('import', { meta: { resolve: () => 'file:///fake/mocker-runtime.js' } }); +vi.mock('node:url', () => ({ fileURLToPath: vi.fn() }), { spy: true }); + +// Mock import.meta.resolve +vi.stubGlobal('import', { meta: { resolve: () => 'file:///fake/mocker-runtime.js' } }); + +beforeEach(() => { + vi.mocked(fileURLToPath).mockReturnValue('/fake/mocker-runtime.js'); +}); + +afterAll(() => { + vi.unstubAllGlobals(); +});Based on learnings,
Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests,Implement mock behaviors in beforeEach blocks in Vitest tests, andnever assign ambient globals directly (for example globalThis.*, global.fetch, or globalThis.window); use vi.stubGlobal and vi.unstubAllGlobals() instead.🤖 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/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.test.ts` around lines 15 - 20, Update the Vitest setup in plugin.test.ts to match repo conventions: make the node:url mock use vi.mock with spy: true, move the fileURLToPath behavior out of the mock factory into a beforeEach block, and keep the mock state reset there as well. Also add a matching vi.unstubAllGlobals() cleanup so the vi.stubGlobal('import', ...) override does not leak between tests. Use the existing mocked import.meta.resolve and fileURLToPath setup in this test as the touchpoints for the change.Source: Coding guidelines
code/core/src/common/utils/resolve-path-in-sb-cache.test.ts (1)
21-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove mock return values into
beforeEachinstead of inline per-test.Every test sets
vi.mocked(pkg.cache).mockReturnValue(...)inline rather than in abeforeEachblock. As per coding guidelines, "Implement mock behaviors inbeforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases 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/utils/resolve-path-in-sb-cache.test.ts` around lines 21 - 106, Move the `pkg.cache` mock setup out of each individual test in `resolvePathInStorybookCache` and into a shared `beforeEach` so the Vitest mock behavior is centralized. Initialize the default return value there, then override only when a test needs a different cache path or `undefined`, keeping the test cases focused on assertions and avoiding inline mock implementations. Use the existing `beforeEach`, `resolvePathInStorybookCache`, and `pkg.cache` symbols to update the test setup consistently.Source: Coding guidelines
🤖 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 @.mcp.json:
- Around line 3-8: The shared MCP config is using machine-specific absolute
paths in the agtx entry, which makes the repo non-portable. Update the
configuration to avoid hard-coding the local home directory and binary location
by using a relative path, environment-based resolution, or moving this setup to
a local-only config. Keep the fix centered on the agtx command and args fields
so the shared .mcp.json works across machines.
In @.omc/state/hud-stdin-cache.json:
- Line 1: Remove the committed AI tool state artifact because it contains local
developer paths, usernames, and session telemetry. Delete this file from the PR,
and also remove any related `.omc/state/**` and `.codex/**` generated state
files referenced by the diff. Update `.gitignore` to exclude these tool-managed
cache/session artifacts so `hud-stdin-cache.json`, `project-memory.json`, and
similar runtime files are never committed again.
In
@.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/session-started.json:
- Around line 1-6: The change accidentally includes local AI tool session state
in the repository, leaking machine-specific details like the cwd and PID. Remove
this session-started artifact and any similar `.omc/` or `.codex/` state files
from the changeset, and update the repository ignore rules so these generated
state directories are not committed again.
In
@.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/last-tool-error-state.json:
- Around line 3-4: The tracked session state currently stores machine-specific
error payloads, including an absolute local path and npm debug output, which
should not be committed. Update the last-tool-error-state JSON so it no longer
records environment-specific details from tool failures; keep only generic,
non-sensitive status information in the state fields and remove or redact the
preview/error contents in the session artifact.
In
`@code/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.json`:
- Around line 1-134: The committed subagent tracking state is local tooling data
and should not be versioned. Remove this session file and any other generated
`.omc/state/**` or `.codex/**` session artifacts included in the PR, then update
the repository ignore rules so `.omc/` is excluded from future commits. Use the
existing session-state structure here as the indicator of the unwanted files to
delete.
In `@code/addons/vitest/src/updateVitestFile.ts`:
- Around line 339-356: The ad hoc merge in updateVitestFile only pushes template
test-level properties when the target lacks the same key, so same-named config
like setupFiles, env, or browser gets dropped instead of merged. Update the
logic around templateTestProp and existingTestProp to reuse mergeProperties for
matching ObjectProperty entries, preserving the existing deep-merge behavior
already used elsewhere in this file. Keep the special-case exclusion for
projects, but ensure same-key properties are merged rather than discarded.
In `@code/builders/builder-vite/src/build.ts`:
- Around line 43-64: The storybook:enforce-output-dir plugin in build.ts is
returning a full config object from the config hook, which can duplicate
array-valued fields during Vite merging; change it to return only the
build.outDir override and keep the rest of the config untouched. Also review the
configEnvironment callback in the same plugin and gate the outDir override by
environment name if only one Vite 6 environment should be forced to
options.outputDir.
In `@code/core/src/common/utils/get-storybook-refs.test.ts`:
- Line 6: The tests in get-storybook-refs.test.ts are mocking fetch via
vi.spyOn(global, 'fetch'), which violates the ambient-global guideline. Update
the test setup to use vi.stubGlobal for fetch in each case, and replace
vi.restoreAllMocks() with vi.unstubAllGlobals() in afterEach so the stubbed
global is cleaned up properly. Keep the changes localized to the
getStorybookRefs test cases and their shared teardown.
In `@code/core/src/common/utils/resolve-path-in-sb-cache.test.ts`:
- Around line 92-106: The test mutates the mocked module state for
versions.storybook directly, and the manual reset at the end of the test can be
skipped if an assertion fails. Move the restore logic into a guaranteed cleanup
hook, such as afterEach, so the mocked Storybook version is always returned to
its original value. Keep the change focused on resolvePathInStorybookCache test
setup and the versions mock to prevent leaked state across tests.
- Around line 10-18: Add the Vitest `spy: true` option to both mock declarations
in `resolve-path-in-sb-cache.test.ts`. Update the `vi.mock('empathic/package',
...)` and `vi.mock('../versions', ...)` calls so they use the required spy-based
mocking pattern, keeping the same mocked exports (`cache` and the `storybook`
version) while ensuring the mocks remain consistent with the test guidelines.
In `@code/core/src/common/utils/resolve-path-in-sb-cache.ts`:
- Line 5: The relative import in resolve-path-in-sb-cache.ts should use an
explicit TypeScript file extension to match the codebase import/export
guidelines. Update the import of versions in resolve-path-in-sb-cache.ts to
reference the concrete source file extension, and keep the change limited to
that import so the resolvePathInSbCache utility continues to work unchanged.
In `@code/core/src/core-server/server-channel/telemetry-channel.ts`:
- Around line 44-55: The new share-event handlers in telemetry-channel should
match the error-handling pattern used by the sibling PREVIEW_INITIALIZED
listener. Wrap the async telemetry(...) calls for SHARE_POPOVER_OPENED,
SHARE_STORY_LINK, and SHARE_ISOLATE_MODE in try/catch so rejected telemetry
promises are swallowed consistently and do not become unhandled rejections. Use
the existing channel.on handlers in telemetry-channel.ts as the place to apply
the same safe pattern.
In `@code/core/src/manager-api/tests/refs.test.ts`:
- Around line 89-100: The mocked Response in respond() resolves ok from ok ??
!!response, but status still falls back from the raw ok value, causing
inconsistent defaults when only response is provided. Update respond() so the
status default is derived from the same resolved ok value used for ok, keeping
the mocked Response fields consistent.
In `@code/lib/cli-storybook/src/sandbox.ts`:
- Around line 83-94: The welcome/version notice block in sandbox.ts swallows all
failures from logger.logBox, so the user can miss outdated/prerelease warnings
with no signal. Update the try/catch around logger.logBox to handle the error
explicitly, following the pattern used elsewhere in this file (for example the
other try/catch blocks), and log or surface the exception with enough context to
know the welcome box failed.
In `@code/lib/cli-storybook/src/upgrade.ts`:
- Around line 387-389: The interruption telemetry is using an empty
automigration results object because `automigrationResults` is shadowed inside
`upgrade()` instead of updating the outer binding. In `upgrade()`, change the
`runAutomigrations(...)` handling so the outer `automigrationResults` declared
near `doctorResults` is reassigned from the returned value, matching the
`doctorResults` pattern, and ensure
`handleInterruption`/`sendMultiUpgradeTelemetry` sees the populated results
rather than a block-scoped `const` with the same name.
In `@code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts`:
- Around line 8-9: The relative imports in AddonConfigurationCommand should use
explicit TypeScript file extensions to match the project guideline. Update the
addonA11yPostinstall and addonVitestPostinstall imports to include the source
extension on each relative path, keeping the same symbols and import locations
while making the module specifiers explicit.
- Around line 65-70: The manual-instructions filter in AddonConfigurationCommand
is checking a non-existent `.result === 'failed'` value, so failed addons are
never selected. Update the `hasFailures` branch to filter addons using the same
failure shape that `configureAddons` stores (the caught error object or non-null
result), matching the existing truthy/null checks used elsewhere in
`AddonConfigurationCommand`, so `logManualAddonInstructions` receives the actual
failed addons.
---
Outside diff comments:
In `@code/addons/vitest/src/postinstall.ts`:
- Around line 160-167: The installPlaywright call in postinstall logic has a
dead conditional because it is inside the !options.skipInstall branch, so
useRemotePkg: !!options.skipInstall is always false and misleading. Update the
AddonVitestService.installPlaywright invocation to either omit useRemotePkg
entirely or pass a value that reflects an actual runtime decision, and keep the
intent clear in the surrounding options handling so the branch in postinstall.ts
matches the behavior of skipInstall and skipDependencyManagement.
In `@code/lib/cli-storybook/src/codemod/csf-factories.ts`:
- Around line 22-57: The retry path in `runStoriesCodemod` can loop forever when
`No files matched` happens with an explicit `glob` or `yes: true`, since it
re-invokes the same options without any chance to change `globString`. Update
the `catch` logic to only retry when a new glob can actually be prompted (for
example, when `glob` was not provided and `yes` is false), and otherwise rethrow
or fail fast. Use the `runStoriesCodemod`, `globString`, and `options` flow to
locate and bound this fallback.
---
Duplicate comments:
In
@.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/autopilot-state.json:
- Around line 1-10: The committed autopilot state artifact leaks a local
absolute path via the project_path field, so remove this .omc/state/sessions/*
JSON file from the PR and ensure the entire .omc/state directory is ignored
going forward. Use the session-state tracking files like autopilot-state.json as
the target for cleanup so these ephemeral agent artifacts are not committed
again.
---
Nitpick comments:
In @.omc/plans/autopilot-impl.md:
- Around line 78-82: The plan document contains a hardcoded machine-specific
absolute path in the CLI repro step, which makes it non-portable and exposes a
local username. Update the referenced command in the plan content to use a
repo-relative or placeholder-based path instead of the personal `/Users/...`
location, and make the same replacement in the duplicate repro block mentioned
by the comment so the instructions stay reusable.
In @.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json:
- Around line 1-217: The committed .omc session state is generated tooling data,
not application code, so it should not be tracked. Remove this mission-state
artifact from the changeset and add .omc/ (or the relevant state paths) to
.gitignore so future runs of the session tooling do not reintroduce files like
mission-state.json.
In
@.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.json:
- Around line 1-116: The issue is that autogenerated internal agent/session
state under .omc is being committed as source. Update the repo hygiene so .omc/
(and any nested code/.omc/ path) is ignored, and remove the tracked state
artifacts from the PR while keeping the actual source tree unchanged. Use the
.omc/state/sessions/subagent-tracking-state.json file and its sibling .omc JSON
artifacts as the affected symbols to locate and clean up these generated files.
In
`@code/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json`:
- Around line 1-133: The session state artifact under the .omc state tree should
not be committed, so remove this generated mission-state file from the change
set and exclude similar tooling artifacts from future commits. Update the repo’s
ignore rules (for example around .omc/state and other agent-generated files like
replay logs or project memory artifacts) so these files are not tracked, and
keep the cleanup focused on the generated state data rather than product code.
In `@code/addons/vitest/src/postinstall.test.ts`:
- Around line 5-49: Add a test case in postinstall.test.ts to cover the
aliased/namespace import path handled by isConfigAlreadySetup in postinstall.ts.
Specifically, verify detection still returns true when the addon plugin is
imported under a different local name (for example, an aliased storybookTest
import) and used in the Vitest config, so the identifier fallback logic is
exercised alongside the existing default import and missing-plugin cases.
In `@code/addons/vitest/src/postinstall.ts`:
- Around line 434-481: The config detection in isConfigAlreadySetup only
recognizes direct Identifier calls, so namespace-imported plugin usage like
member-accessed calls is missed. Update the CallExpression traversal in
isConfigAlreadySetup to also detect member-expression callees for the Storybook
test plugin, including namespace imports from the existing import scan, so both
direct calls and cases like pluginNamespace.storybookTest(...) are treated as
already configured.
In `@code/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.test.ts`:
- Around line 15-20: Update the Vitest setup in plugin.test.ts to match repo
conventions: make the node:url mock use vi.mock with spy: true, move the
fileURLToPath behavior out of the mock factory into a beforeEach block, and keep
the mock state reset there as well. Also add a matching vi.unstubAllGlobals()
cleanup so the vi.stubGlobal('import', ...) override does not leak between
tests. Use the existing mocked import.meta.resolve and fileURLToPath setup in
this test as the touchpoints for the change.
In `@code/core/src/common/utils/resolve-path-in-sb-cache.test.ts`:
- Around line 21-106: Move the `pkg.cache` mock setup out of each individual
test in `resolvePathInStorybookCache` and into a shared `beforeEach` so the
Vitest mock behavior is centralized. Initialize the default return value there,
then override only when a test needs a different cache path or `undefined`,
keeping the test cases focused on assertions and avoiding inline mock
implementations. Use the existing `beforeEach`, `resolvePathInStorybookCache`,
and `pkg.cache` symbols to update the test setup consistently.
In `@code/core/src/csf-tools/CsfFile.ts`:
- Around line 843-871: The CSF factory safeguard in CsfFile currently only
checks the local binding name with rootObject.name === 'preview', so aliased
imports can bypass BadMetaError. Update the import-validation logic around the
rootObject/configParent handling to inspect the actual imported specifier name
from the ImportDeclaration (for example, the named import’s imported name)
instead of relying on the local alias. Keep the existing BadMetaError path in
place, but make it trigger for any alias of the preview import coming from the
wrong source.
In `@code/frameworks/nextjs/src/export-mocks/link/index.tsx`:
- Around line 7-23: The MockLink component currently uses
React.forwardRef<HTMLAnchorElement, any>, which removes type safety for its Link
props. Replace the any in MockLink with a proper props interface for the subset
of Next.js LinkProps that this mock actually uses, so href, onClick, children,
and related fields are validated at call sites. Keep the change focused on
MockLink and its forwardRef signature so the mock stays compatible while
restoring useful TypeScript checks.
In `@code/lib/cli-storybook/src/automigrate/multi-project.test.ts`:
- Around line 36-46: The `vi.mock('./fixes', ...)` setup in
`multi-project.test.ts` should follow the Vitest guideline by using `spy: true`
for file/package mocks. Update the existing mock for `./fixes` so it keeps the
same mocked `allFixes` behavior while enabling spying semantics, matching the
pattern used in other tests and avoiding an inline factory-only mock.
In `@code/lib/cli-storybook/src/codemod/helpers/csf-factories-utils.ts`:
- Around line 134-147: The addImportToTop helper is clearing too much comment
metadata from the first program node. Update addImportToTop so it only removes
the leading comments that were transferred to importDecl, and do not reset
firstNode.comments wholesale; preserve any trailing or other comments attached
to firstNode. Use the existing addImportToTop and
firstNode.leadingComments/comments handling to keep the comment transfer
narrowly scoped.
In `@code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts`:
- Around line 17-19: The `postinstallAddon` mock in
`AddonConfigurationCommand.test` is set up inconsistently with the other Vitest
mocks: it uses an inline factory instead of `spy: true`, and its resolved
behavior is declared at mock time rather than in `beforeEach`. Update the
`vi.mock` for `postinstallAddon` to follow the same `spy: true` pattern used
elsewhere in this test file, and move the `mockResolvedValue(undefined)` setup
into `beforeEach` so the mock behavior is initialized per test alongside the
other mocks.
🪄 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: bb6043ad-fd37-4ac1-98ce-73b6775ee5fb
⛔ Files ignored due to path filters (2)
.yarn/patches/@testing-library-user-event-npm-14.6.1-5da7e1d4e2.patchis excluded by!**/.yarn/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (167)
.codex/environments/environment.toml.mcp.json.omc/plans/autopilot-impl.md.omc/project-memory.json.omc/sessions/2db435ae-164a-4004-8a44-5dcf7b420df2.json.omc/sessions/be386e90-f09c-45e5-a71d-b38dffc94077.json.omc/specs/deep-interview-upgrade-install-regression.md.omc/state/hud-stdin-cache.json.omc/state/sessions/2db435ae-164a-4004-8a44-5dcf7b420df2/pre-tool-advisory-throttle.json.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/autopilot-state.json.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/hud-state.json.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/pre-tool-advisory-throttle.json.omc/state/sessions/a4326817-cec8-484b-9e62-263d11f67225/session-started.json.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/hud-state.json.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/last-tool-error-state.json.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.json.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/pre-tool-advisory-throttle.json.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.jsonCHANGELOG.mdcode/.eslintrc.jscode/.omc/state/agent-replay-be386e90-f09c-45e5-a71d-b38dffc94077.jsonlcode/.omc/state/idle-notif-cooldown.jsoncode/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/last-tool-error-state.jsoncode/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/mission-state.jsoncode/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/pre-tool-advisory-throttle.jsoncode/.omc/state/sessions/be386e90-f09c-45e5-a71d-b38dffc94077/subagent-tracking-state.jsoncode/addons/a11y/package.jsoncode/addons/a11y/src/postinstall.tscode/addons/docs/package.jsoncode/addons/links/package.jsoncode/addons/onboarding/package.jsoncode/addons/pseudo-states/package.jsoncode/addons/themes/package.jsoncode/addons/vitest/package.jsoncode/addons/vitest/src/node/vitest-manager.tscode/addons/vitest/src/postinstall.test.tscode/addons/vitest/src/postinstall.tscode/addons/vitest/src/typings.d.tscode/addons/vitest/src/updateVitestFile.test.tscode/addons/vitest/src/updateVitestFile.tscode/addons/vitest/src/vitest-plugin/index.tscode/builders/builder-vite/package.jsoncode/builders/builder-vite/src/build.tscode/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.test.tscode/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.tscode/builders/builder-vite/src/plugins/vite-mock/plugin.tscode/builders/builder-webpack5/package.jsoncode/builders/builder-webpack5/src/plugins/webpack-mock-plugin.tscode/core/build-config.tscode/core/package.jsoncode/core/src/bin/core.tscode/core/src/cli/AddonVitestService.test.tscode/core/src/cli/AddonVitestService.tscode/core/src/common/js-package-manager/JsPackageManager.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/utils/get-storybook-refs.test.tscode/core/src/common/utils/get-storybook-refs.tscode/core/src/common/utils/resolve-path-in-sb-cache.test.tscode/core/src/common/utils/resolve-path-in-sb-cache.tscode/core/src/common/versions.tscode/core/src/components/components/Select/Select.stories.tsxcode/core/src/components/components/Select/Select.tsxcode/core/src/core-events/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/server-channel/telemetry-channel.test.tscode/core/src/core-server/server-channel/telemetry-channel.tscode/core/src/csf-tools/CsfFile.test.tscode/core/src/csf-tools/CsfFile.tscode/core/src/manager-api/modules/refs.tscode/core/src/manager-api/modules/url.tscode/core/src/manager-api/tests/refs.test.tscode/core/src/manager-api/tests/url.test.jscode/core/src/manager-api/version.tscode/core/src/manager/components/preview/Viewport.tsxcode/core/src/manager/components/preview/tools/share.stories.tsxcode/core/src/manager/components/preview/tools/share.tsxcode/core/src/manager/components/preview/tools/zoom.stories.tsxcode/core/src/manager/components/preview/tools/zoom.tsxcode/core/src/manager/globals/exports.tscode/core/src/mocking-utils/index.tscode/core/src/mocking-utils/mocker-runtime.jscode/core/src/mocking-utils/redirect.tscode/core/src/mocking-utils/runtime.tscode/core/src/node-logger/index.test.tscode/core/src/node-logger/index.tscode/core/src/server-errors.tscode/core/src/telemetry/detect-agent.test.tscode/core/src/telemetry/detect-agent.tscode/core/src/telemetry/storybook-metadata.tscode/core/src/telemetry/telemetry.tscode/core/src/telemetry/types.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/e2e-tests/preview-api.spec.tscode/frameworks/angular/build-schema.jsoncode/frameworks/angular/package.jsoncode/frameworks/angular/start-schema.jsoncode/frameworks/ember/package.jsoncode/frameworks/html-vite/package.jsoncode/frameworks/nextjs-vite/package.jsoncode/frameworks/nextjs/build-config.tscode/frameworks/nextjs/package.jsoncode/frameworks/nextjs/src/aliases/webpack.tscode/frameworks/nextjs/src/export-mocks/link/index.tsxcode/frameworks/nextjs/src/export-mocks/webpack.tscode/frameworks/preact-vite/package.jsoncode/frameworks/react-native-web-vite/package.jsoncode/frameworks/react-vite/package.jsoncode/frameworks/react-webpack5/package.jsoncode/frameworks/server-webpack5/package.jsoncode/frameworks/svelte-vite/package.jsoncode/frameworks/sveltekit/package.jsoncode/frameworks/vue3-vite/package.jsoncode/frameworks/web-components-vite/package.jsoncode/lib/cli-sb/package.jsoncode/lib/cli-storybook/package.jsoncode/lib/cli-storybook/src/automigrate/index.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/automigrate/types.tscode/lib/cli-storybook/src/bin/run.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.test.tscode/lib/cli-storybook/src/codemod/helpers/config-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/csf-factories-utils.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/sandbox.tscode/lib/cli-storybook/src/upgrade.test.tscode/lib/cli-storybook/src/upgrade.tscode/lib/codemod/package.jsoncode/lib/codemod/src/index.test.tscode/lib/codemod/src/index.tscode/lib/core-webpack/package.jsoncode/lib/create-storybook/package.jsoncode/lib/create-storybook/src/commands/AddonConfigurationCommand.test.tscode/lib/create-storybook/src/commands/AddonConfigurationCommand.tscode/lib/create-storybook/src/commands/ProjectDetectionCommand.tscode/lib/create-storybook/src/initiate.tscode/lib/create-storybook/src/scaffold-new-project.tscode/lib/create-storybook/src/services/VersionService.test.tscode/lib/create-storybook/src/services/VersionService.tscode/lib/csf-plugin/package.jsoncode/lib/eslint-plugin/package.jsoncode/lib/react-dom-shim/package.jsoncode/package.jsoncode/presets/create-react-app/package.jsoncode/presets/react-webpack/package.jsoncode/presets/server-webpack/package.jsoncode/renderers/html/package.jsoncode/renderers/preact/package.jsoncode/renderers/react/package.jsoncode/renderers/server/package.jsoncode/renderers/svelte/package.jsoncode/renderers/vue3/package.jsoncode/renderers/web-components/package.jsondocs/_snippets/csf-factories-automigrate-with-config-directory.mddocs/_snippets/storybook-preview-configure-globaltypes.mddocs/api/cli-options.mdxdocs/api/csf/csf-next.mdxdocs/get-started/frameworks/angular.mdxdocs/versions/latest.jsondocs/versions/next.jsondocs/writing-stories/typescript.mdxscripts/bench/bench-packages.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-bundle.tsscripts/package.json
💤 Files with no reviewable changes (2)
- code/lib/create-storybook/src/initiate.ts
- code/core/src/server-errors.ts
Sidnioulz
changed the title
upgrade CLI hangs as soon as an error occurs (e.g. install error)upgrade hangs as soon as an error occurs
Sidnioulz
added
bug
cli
ci:normal
valentinpalkovic
force-pushed
the
task/c1adc443-regression-bug--upgrade--CLI
branch
from
July 9, 2026 10:21
646ff80 to
5d81419
Compare
valentinpalkovic
changed the title
upgrade hangs as soon as an error occurs
Contributor
|
Failed to publish canary version of this pull request, triggered by @Sidnioulz. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/29016304020 |
valentinpalkovic
marked this pull request as draft
July 9, 2026 12:03
valentinpalkovic
changed the title
valentinpalkovic
force-pushed
the
task/c1adc443-regression-bug--upgrade--CLI
branch
from
July 9, 2026 12:45
42c279a to
638424a
Compare
valentinpalkovic
changed the title
Sidnioulz
approved these changes
Jul 9, 2026
After the dedupe step, the upgrade command configures addons that automigrations deferred (e.g. addon-vitest/addon-a11y from angular-to-angular-vite). The a11y postinstall spawned the nested "storybook automigrate addon-a11y-addon-test" command with open stdio pipes. When that nested run failed (e.g. an npm ERESOLVE peer conflict), the process tree blocked forever with zero output - the upgrade appeared to hang right after the dedupe prompt. It now uses stdio: 'ignore' like the addon-vitest postinstall already does, so a failure surfaces instead of hanging. The upgrade also logs a visible "Configuring addons: ..." step and warns when that phase fails, so it can no longer fail invisibly.
valentinpalkovic
force-pushed
the
task/c1adc443-regression-bug--upgrade--CLI
branch
from
July 9, 2026 14:19
bfb22b8 to
c557fc0
Compare
Contributor
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 22.16 MB | 22.20 MB | 🚨 +42 KB 🚨 |
| Dependency size | 36.65 MB | 36.65 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 826 KB | 827 KB | 🚨 +1 KB 🚨 |
| Dependency size | 92.36 MB | 92.41 MB | 🚨 +41 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 90.85 MB | 90.89 MB | 🚨 +40 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🚨 +878 B 🚨 |
| Dependency size | 58.81 MB | 58.85 MB | 🚨 +42 KB 🚨 |
| Bundle Size Analyzer | node | node |
Only stdin needs to be closed to prevent the npm exec layers from blocking on a never-ending pipe; stdout/stderr can stay piped so that a failed nested run still reports the underlying error (e.g. the npm ERESOLVE details) instead of just an exit code.
…ailures Previously the postinstall hook of a deferred addon was resolved relative to the CLI bundle (import.meta.resolve first, and a createRequire anchored to a bare directory, which resolves from the parent). When the CLI runs from a different tree than the project (npx, monorepo), resolution failed and the addon was silently skipped and left unconfigured. The hook is now resolved from the user's project first. Because the upgrade command installs the addon mid-run, Node's module resolution has already cached the earlier negative lookup, so a short-lived child process (clean resolution cache) is used as a fallback before giving up. The subsequent load goes through a direct file URL, which is not affected by the cached exports lookup. Resolution and load failures now warn with the underlying error and a manual setup hint instead of silently returning.
valentinpalkovic
marked this pull request as ready for review
July 9, 2026 21:09
Merged
2 tasks done
This was referenced Jul 10, 2026
This was referenced Jul 13, 2026
Merged
Open
6 of 9 tasks
This was referenced Jul 16, 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
3 participants
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
storybook upgradecould hang forever right after the dedupe prompt, without any output. Additionally, addon setup failures in that phase were invisible. Both problems live in the deferred addon configuration introduced in #34202 (e.g. addon-vitest/addon-a11y from theangular-to-angular-vitemigration).The issue:
storybook automigrate addon-a11y-addon-testcommand with open stdio pipes. Thenpm execlayers in between wait for an EOF on stdin that never comes, because the parent never writes to or closes the pipe. When the nested run fails (in the reproduction: an npm ERESOLVE peer conflict), the whole process tree blocks forever, and the pipes swallow all output.Solution:
stdio: ['ignore', 'pipe', 'pipe']. Stdin is/dev/null, so it returns EOF immediately and the deadlock is gone. Stdout/stderr stay captured, so a failure still reports the full npm error.npx storybook add <addon>hint instead of silently returning.Configuring addons: ...step and warns when that phase fails.Verified against a real reproduction (bitwarden/clients): before, the identical flow hung every time. With the canary
0.0.0-pr-35423-sha-43213e67, the hooks resolve and run, the vitest setup failure is reported with the full npm error, and the upgrade completes.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
@storybook/angular(e.g. bitwarden/clients) and install dependencies.npx storybook@0.0.0-pr-35423-sha-43213e67 upgrade, select theangular-to-angular-viteautomigration, and accept the addon-vitest/addon-a11y setup prompts.Configuring addons: ...step should appear, the postinstall hooks should run, and any failure should be printed as a warning with the underlying error, instead of hanging silently.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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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-35423-sha-43213e67. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-35423-sha-43213e67 sandboxor in an existing project withnpx storybook@0.0.0-pr-35423-sha-43213e67 upgrade.More information
0.0.0-pr-35423-sha-43213e67task/c1adc443-regression-bug--upgrade--CLI43213e671783627666)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=35423Summary by CodeRabbit