Build: Make the git root the yarn workspace root #33079
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR restructures the Storybook monorepo by consolidating the Yarn workspace root from the code/ directory to the git repository root. This simplifies the workspace structure by having a single workspace configuration instead of multiple nested ones.
Key changes:
- Consolidated workspace definitions and dependency resolutions to the root
package.json - Removed redundant
.yarnrc.ymlfiles fromcode/andscripts/subdirectories - Updated build utilities to reference the new workspace root location
Reviewed Changes
Copilot reviewed 8 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added workspace configuration and resolutions, making it the new workspace root |
| code/package.json | Removed workspace definitions, resolutions, and packageManager as it's no longer the workspace root |
| code/core/package.json | Removed resolutions that are now consolidated at the root level |
| scripts/package.json | Removed resolutions that are now consolidated at the root level |
| scripts/utils/tools.ts | Updated getWorkspace() to read workspace config from ROOT_DIRECTORY instead of CODE_DIRECTORY |
| scripts/utils/yarn.ts | Removed yarn set version berry command as yarn version is now managed at root level |
| code/.yarnrc.yml | Deleted - no longer needed with single workspace root |
| scripts/.yarnrc.yml | Deleted - no longer needed with single workspace root |
| .gitignore | Updated yarn-related patterns to reflect new structure with root-level .yarn directory |
| node_modules/.yarn-state.yml | Deleted - part of cleanup |
| node_modules/.package-lock.json | Deleted - part of cleanup |
| .yarn/patches/*.patch | Added patch files at root level with updated paths from nested structure |
|
View your CI Pipeline Execution ↗ for commit 74699b7
☁️ Nx Cloud last updated this comment at |
kasperpeulen
force-pushed
the
kasper/ai-friendly-setup
branch
from
276f911 to
1c1ffd8
Compare
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdd a root Nx workspace and consolidate workspace tooling: unify Yarn config to root, rename workspace utilities, introduce dynamic sandbox port allocation and caching, update many package.json/project.json manifests and CI cache keys, and refactor scripts to use ROOT_DIRECTORY and sandbox orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI/User
participant NX as Nx Runner
participant Scripts as scripts/*
participant Sandbox as Sandbox process
participant Registry as Local Verdaccio
User->>NX: run workspace task (build/dev/e2e)
NX->>Scripts: invoke task (passes key, template, selectedTask)
Scripts->>Scripts: port = getPort({key, selectedTask})
Scripts->>Scripts: detectFreePort(port)
alt port free
Scripts->>Sandbox: start server on port
Sandbox->>Scripts: wait-on ready
Scripts->>Registry: optionally publish via Verdaccio
Scripts->>NX: report success
else port occupied
Scripts->>Scripts: killPort(port)
Scripts->>Scripts: retry or fail
Scripts-->>User: error or retry result
end
sequenceDiagram
participant Runner as scripts/run-registry.ts
participant Kill as killPort
participant Verdaccio as Verdaccio
participant Proxy as proxy
Runner->>Kill: clear ports (6001/6002)
Runner->>Verdaccio: start Verdaccio (6001)
Runner->>Proxy: start proxy (6002)
Verdaccio-->>Runner: ready
Proxy-->>Runner: ready
Runner->>Runner: return Servers handle
Note right of Runner: on signal/error -> Servers.close() -> kill ports and exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Focus areas for review:
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (21)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/Tabs/TabsView.tsx (1)
8-8: LGTM! Type cast resolves styled-components compatibility.The
CSSObjectimport and cast when spreadingbarInnerStylefix type incompatibility betweenReact.CSSProperties(line 91) and the styled-components system. This mirrors the change inStatelessTabsView.tsxand maintains consistent typing across both Tab components.Optionally, you could update the
barInnerStyleprop type definition on line 91 fromReact.CSSPropertiestoCSSObjectto eliminate the need for the cast, though the current approach maintains compatibility with callers passing standard React styles.Also applies to: 153-153
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
.yarn/patches/@testing-library-user-event-npm-14.6.1-5da7e1d4e2.patchis excluded by!**/.yarn/**.yarn/patches/@types-babel__traverse-npm-7.20.6-fac4243243.patchis excluded by!**/.yarn/**.yarn/patches/@vitest-expect-npm-3.2.4-97c526d5cc.patchis excluded by!**/.yarn/**.yarn/patches/react-aria-components-npm-1.12.2-6c5dcdafab.patchis excluded by!**/.yarn/**code/yarn.lockis excluded by!**/yarn.lock,!**/*.locknode_modules/.package-lock.jsonis excluded by!**/node_modules/**node_modules/.yarn-state.ymlis excluded by!**/node_modules/**scripts/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.gitignore(2 hunks)code/.yarnrc.yml(0 hunks)code/core/package.json(0 hunks)code/core/src/components/components/Tabs/StatelessTabsView.tsx(2 hunks)code/core/src/components/components/Tabs/TabsView.tsx(2 hunks)code/package.json(1 hunks)nx.json(3 hunks)package.json(2 hunks)scripts/.yarnrc.yml(0 hunks)scripts/build/utils/generate-types.ts(2 hunks)scripts/package.json(0 hunks)scripts/utils/tools.ts(2 hunks)scripts/utils/yarn.ts(0 hunks)
💤 Files with no reviewable changes (5)
- code/core/package.json
- code/.yarnrc.yml
- scripts/package.json
- scripts/.yarnrc.yml
- scripts/utils/yarn.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
.gitignorecode/package.jsonpackage.json
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
.gitignorecode/package.json
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Tabs/StatelessTabsView.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/components/components/Tabs/StatelessTabsView.tsx
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/package.jsonpackage.json
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/package.json
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
package.json
🧬 Code graph analysis (1)
scripts/utils/tools.ts (1)
scripts/utils/constants.ts (1)
ROOT_DIRECTORY(6-6)
🔇 Additional comments (11)
code/core/src/components/components/Tabs/StatelessTabsView.tsx (1)
5-5: LGTM! Type cast improves compatibility with styled-components.The addition of the
CSSObjectimport and the type cast when spreadingbarInnerStyleresolves type incompatibility betweenReact.CSSPropertiesand the styled-components system. This change is consistent with the identical modification inTabsView.tsxand has no runtime impact.Also applies to: 77-77
scripts/build/utils/generate-types.ts (2)
68-68: Clarify the 10x timeout increase from 2 to 20 minutes.The timeout has been dramatically increased from 120,000ms (2 minutes) to 1,200,000ms (20 minutes). This is a substantial change that could mask performance issues or indicate a real need for extended processing time.
Please clarify:
- Is this timeout increase necessary due to larger codebases or more complex type generation?
- Have you observed type generation processes timing out at 2 minutes?
- Could this hide underlying performance problems that should be addressed?
Consider whether a more moderate timeout (e.g., 5 minutes) would be sufficient, or if performance optimizations should be investigated instead.
32-32: This review comment is incorrect and should be ignored.The file
scripts/build/utils/generate-types.tshas not been modified in this PR. Git diff shows no changes to this file, and the path using three directory levels (join(import.meta.dirname, '..', '..', '..', 'node_modules', '.bin', 'jiti')) already existed in the previous commit. The review comment's claim that the path "now goes up three levels instead of two" is inaccurate—there is no change to this line as part of the current PR.Likely an incorrect or invalid review comment.
scripts/utils/tools.ts (1)
23-23: LGTM: Consistent path constant rename aligns with new workspace structure.The renaming from
CODE_DIRECTORYtoROOT_DIRECTORYis consistent across all usages (import, workspace reading, globbing, and path construction). This correctly reflects the workspace root moving fromcode/to the git repository root.Also applies to: 134-146
.gitignore (1)
22-28: LGTM: Gitignore updates align with centralized Yarn workspace.The changes correctly update Yarn-related ignore patterns for a workspace root at the git repository level:
- Changed from recursive
/**/.yarnto top-level.yarnwith selective negations for necessary Yarn directories- Added
node_modules/.yarn-state.ymlwhich is a Yarn 4 state file- Added
.junieentryThese changes are appropriate for the new workspace structure.
Also applies to: 67-69
code/package.json (2)
1-252: LGTM: Removal of workspace configuration is correct.The removal of
workspaces,resolutions, andpackageManagerfields fromcode/package.jsonis appropriate since this package is no longer the workspace root. These configurations have been moved to the rootpackage.json.
171-171: No issues found — react-popper-tooltip placement is correct.The package is properly placed in
dependenciesat the monorepo root. It's a runtime dependency used incode/core/src/components/components/tooltip/WithTooltip.tsx(lines 10–11) and exported as part of the public API via./internal/components. Since it's not bundled into the distribution (unlike bundled packages likefast-printfandopenwhich remain indevDependencies), it should be available as an external dependency to consumers.package.json (3)
2-17: LGTM: Workspace configuration correctly moved to repository root.The package has been appropriately renamed to
@storybook/super-rootand configured as the new workspace root with:
- Private flag set (correct for workspace roots)
- Workspace packages listing
code,code/*subdirectories, andscriptsThis aligns with the PR objective of making the git root the Yarn workspace root.
43-67: Resolutions and nx dependencies moved to workspace root.The
resolutionsblock and nx-relateddevDependencieshave been appropriately moved fromcode/package.jsonto the root workspace configuration. This centralizes dependency resolution and build tooling at the workspace level.
4-4: I need to check if these CommonJS files are isolated in subdirectories with their own package.json files, as that would affect whether the root-level"type": "module"change impacts them.Now I need to check if the subdirectories with CommonJS files have their own package.json files, which would shield them from the root-level "type": "module" change.
Now I need to check if these test directories have their own package.json files that would override the root setting:
Based on my verification, I can now provide the rewritten review comment. The original concern is partially valid:
.babelrc.jsandbabel.config.jsbehave like the.mjsequivalents when your package.json file contains the"type": "module"option, and configuration files being treated as ES modules whenpackage.jsoncontains"type": "module"requires renaming them to use the.cjsfile extension. However, the script output reveals that most CommonJS files are intest-storybooks/subdirectories, which may have their ownpackage.jsonfiles that override the root setting. Additionally, subdirectories are considered ESM only "until either another package.json or node_modules directory is encountered".
Verify root-level configuration files for ES module compatibility after adding
"type": "module".Setting
"type": "module"at the root level affects.jsfiles in the root scope, particularly configuration files. Files likejest.config.js,babel.config.js, and.babelrc.jswill be interpreted as ES modules, which breaks CommonJS patterns likemodule.exportsandrequire().The script found CommonJS files mostly within
test-storybooks/subdirectories; these may be shielded by their ownpackage.jsonfiles if present. However, ensure that any root-level.jsconfiguration files either:
- Use
.cjsextension (e.g.,jest.config.cjs,babel.config.cjs)- Use
.jsonformat (e.g.,babel.config.json)- Use
.mjsextension if ES module syntax is appropriateVerify the repository structure for root-level configuration files that need migration before this change is applied.
nx.json (1)
30-33: LGTM: Nx configuration correctly updated for new workspace structure.All path adjustments consistently add the
code/prefix or adjust working directories to{workspaceRoot}/code, correctly reflecting that the workspace root has moved up one level. Key changes:
- Build outputs:
code/bench/esbuild-metafiles/{projectName}- Test working directory and inputs: prefixed with
code/sb:serveworking directory:{workspaceRoot}/code- Shared globals: updated to
code/tsconfig.jsonand addedscripts/**/*monitoringThe removal of the explicit inputs entry from the check configuration (line 54) appears to be a cleanup, relying on the default and ^production inputs.
Also applies to: 39-46, 54-54, 96-96, 105-105
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.circleci/config.yml (1)
204-210: Confirm bareyarninvocation is intentional and idempotent.Lines 206, 255, and 283 use bare
yarncommands (which implicitly runyarn install). Verify that:
- This is intentional and matches your project's Yarn configuration (e.g., immutable mode, frozen lockfile flags).
- The command is idempotent so that re-runs don't fail if dependencies are already installed.
If Yarn is configured with immutable mode (common for CI), consider explicitly adding
--immutableor--frozen-lockfileflags to lines 206, 255, and 283 for clarity and to prevent accidental lock file modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.circleci/config.yml(5 hunks)
🔇 Additional comments (4)
.circleci/config.yml (4)
200-210: Verify yarn.lock is at repository root for cache key resolution.Cache keys using
{{ checksum "yarn.lock" }}for Yarn 2.x assume the file path is relative to the executor's working directory (set to/tmp/storybookon line 54). The cache key change from separatecode/yarn.lockandscripts/yarn.lockto a singleyarn.lockrequires this file to exist at the repository root. Confirm that the monorepo restructuring includes a root-levelyarn.lockand that prior per-directory Yarn configurations have been removed.
224-237: Verify workspace persistence paths align with root-level yarn consolidation.The
persist_to_workspacestep (lines 224–237) includesscripts/node_modulesalongsidecode/node_modules. If the PR consolidates to a single root-level yarn workspace, verify that:
scripts/node_modulesshould still be a separate persisted path, or- All workspace packages (including scripts) share the root
node_modulesand only the root should be persisted.If moving to a unified root workspace, you may need to remove
scripts/node_modulesfrom line 228 and ensure downstream jobs reference the correct node_modules location.
543-555: LGTM: Cache strategy properly supports yarn workspaces.The pretty-docs job demonstrates correct handling of yarn workspaces: yarn workspaces use a single yarn.lock at the root with separate node_modules directories per workspace package, requiring each workspace's node_modules to be included in the cache. The pattern of restoring from a root
yarn.lockchecksum, then runningyarn installin a workspace subdirectory, is appropriate for this setup.
1-1150: Cache consolidation verified and aligned with monorepo structure.The monorepo structure fully supports the cache consolidation changes:
Root yarn.lock is the canonical dependency source — The
workspacesfield in rootpackage.jsondeclares all workspace packages includingscripts, ensuring all dependencies (from code, scripts, and other workspaces) are captured in the single rootyarn.lock.All cache keys correctly reference root yarn.lock — The build, check, and pretty-docs jobs all use
{{ checksum "yarn.lock" }}which resolves to the repository root. This is consistent and correct.scripts workspace is properly integrated — The pretty-docs job's
cd scripts && yarn installstep is safe because scripts is declared as a workspace, so its dependencies are already in the rootyarn.lockand will be restored when the cache is populated.The changes align the CircleCI configuration with the monorepo's workspace structure. The implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/utils/workspace.ts (1)
9-18: Fix workspace path resolution - critical double-prefix bug.The change to
getWorkspaces()cwd introduces a breaking bug. Whenyarn workspaces listruns fromROOT_DIRECTORY, it returns paths like"code/addons/a11y"(relative to root). However, consumers combine these withCODE_DIRECTORY, creating double-prefixed paths like"code/code/addons/a11y".The
workspacePath()function must normalize returned locations to be relative toCODE_DIRECTORY:export async function workspacePath(type: string, packageName: string) { const workspaces = await getWorkspacesMemo(); const workspace = workspaces.find((w) => w.name === packageName); if (!workspace) { throw new Error(`Unknown ${type} '${packageName}', not in yarn workspace!`); } // Strip 'code/' prefix since locations are now relative to ROOT_DIRECTORY return workspace.location.replace(/^code\//, ''); }This breaks calls at scripts/tasks/sandbox-parts.ts lines 611, 647, 680, 686, and 719 where
workspacePath()results are joined withCODE_DIRECTORY.
🧹 Nitpick comments (1)
scripts/run-registry.ts (1)
145-145: Simplify by reusing therootconstant.Line 145 recomputes
resolvePath(join(__dirname, '..')), which is identical to therootconstant defined on line 26.Apply this diff:
- const loggedLocation = location.replace(resolvePath(join(__dirname, '..')), '.'); + const loggedLocation = location.replace(root, '.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
scripts/run-registry.ts(1 hunks)scripts/utils/workspace.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
🧬 Code graph analysis (1)
scripts/utils/workspace.ts (1)
scripts/utils/constants.ts (1)
ROOT_DIRECTORY(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
🔇 Additional comments (3)
scripts/run-registry.ts (2)
112-116: <!-- [scratchpad_start]
[task verify_workspace_restructuring_final]
Analyze the workspace structure and review consistency.[observations]
From root package.json:
- Workspaces still defined as: ["code", "code/core", "code/addons/*", ...]
- Root package.json is "@storybook/super-root", private: true
- This is the workspace definition
From code/package.json:
- "@storybook/root", version: "10.1.0-alpha.11"
- This is where the version comes from
From script context:
- Line 187: version = currentVersion() reads from code/package.json
- Line 201: version is displayed in log: "found X packages at version Y"
- Line 146: resolvedLocation = resolvePath('..', location)
- location comes from packages returned by getWorkspaces(false)
- packages would be paths like "code", "code/core", "code/addons/...", etc.
[analysis]
CRITICAL INSIGHT - The review comment is INCORRECT:
- The workspaces array in root package.json STILL references "code" directory
- code/ is NOT being removed - it's still a workspace package directory
- Line 113 reading from code/package.json is CORRECT because code/package.json is the actual Storybook version
- Line 146's change from resolvePath('../code', location) to resolvePath('..', location) is CORRECT because:
- Old: join(__dirname, '..', 'code') + location would cause double 'code' if location contains 'code'
- New: join(__dirname, '..') + location is correct since location from getWorkspaces includes 'code'
- Both lines are consistent and correct with current workspace structure
[conclusion]
The PR did NOT move packages OUT of code/ directory. The workspace root remains at code/ for Storybook packages. The change on line 146 is a bug fix to path resolution, not a root change. Line 113 is correct as-is.[tag]
[done]
The review comment is incorrect. The workspace structure has not changed to move packages out of the
code/directory.Analysis:
- The root
package.jsonworkspaces still define packages as["code", "code/core", "code/addons/*", ...], confirmingcode/remains the workspace package directory.code/package.jsoncontains the actual Storybook version (10.1.0-alpha.11), whichcurrentVersion()correctly reads from at line 113.- The change on line 146 from
resolvePath('../code', location)toresolvePath('..', location)is a bug fix: sincelocationfromgetWorkspaces()already includes'code'as a prefix, the old code would have created double'code'in the path. The new code is correct.- Both line 113 and line 146 are consistent with the current workspace structure and work correctly together.
Likely an incorrect or invalid review comment.
146-146: Path resolution change is correct and verified.The workspace location values returned by
getWorkspaces()still include the"code/"prefix (e.g.,"code/addons/a11y","code/builders/builder-vite"). The change fromresolvePath('../code', location)toresolvePath('..', location)properly resolves these locations to the correct package directories:
- With
resolvePath('..', location)where location is"code/addons/a11y", the resolved path correctly points to/home/jailuser/git/code/addons/a11yThe previous path would have resulted in a double
code/directory in the path, which would be incorrect. This change aligns with the PR objective without requiring any additional modifications.scripts/utils/workspace.ts (1)
5-5: LGTM: Import updated to ROOT_DIRECTORY.The import change aligns with the PR objective to make the git root the workspace root.
This commit sets up Nx Cloud for your Nx workspace, enabling distributed caching and the Nx Cloud GitHub integration for fast CI and improved developer experience. You can access your Nx Cloud workspace by going to https://cloud.nx.app/orgs/65e9fd00a2d2dea5ebe4e94c/workspaces/691dbcdcb2822ca4fdc727c4 **Note:** This commit attempts to maintain formatting of the nx.json file, however you may need to correct formatting by running an nx format command and committing the changes.
feat(nx-cloud): set up nx workspace
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/nx.yml(1 hunks)nx.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
🪛 Gitleaks (8.29.0)
nx.json
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (7)
nx.json (3)
41-41: Verify workspace path correctness after consolidation.The PR objective mentions making the git root the yarn workspace root. Lines 41 and 57 reference
{workspaceRoot}/code/...paths—confirm whether the/codesubdirectory is still the correct location post-consolidation, or if paths should be adjusted to the root level.Also applies to: 57-57
20-52: Nx targetDefaults and cache configuration look appropriate.The build and check targets are well-configured with proper inputs, outputs, dependencies, and caching rules aligned with the monorepo structure.
54-68: namedInputs configuration is well-structured.The exclusion rules for production builds appropriately avoid unnecessary cache invalidations from test, spec, stories, and config file changes.
.github/workflows/nx.yml (4)
25-26: Inconsistent state: Distributed CI is uncommented despite comment guidance.Line 25 states "Uncomment this line to enable task distribution," but line 26 is already uncommented. Clarify the intent: should distributed CI be enabled by default, or should this line remain commented out pending explicit enablement?
29-34: Node setup and dependency installation are well-configured.The caching strategy using
cache: 'yarn'will automatically use the repo rootyarn.lock, aligning with the PR's objective to standardize CI cache keys. The--immutableflag ensures reproducible installs.
35-39: Nx command configuration looks correct.The
nx-set-shasstep andnx affectedcommand are properly configured to detect and build only changed packages with production optimization flags.
7-9: Permissions are appropriately minimal.The workflow only requests
readpermissions for actions and contents, following least-privilege security practices.
kasperpeulen
requested review from
jonniebigodes and
kylegach
as code owners
# Conflicts: # code/addons/vitest/src/postinstall.ts # code/nx.json # code/package.json # code/yarn.lock # scripts/npm-auth.ts # scripts/yarn.lock
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.circleci/config.yml(4 hunks)code/addons/a11y/package.json(0 hunks)code/addons/docs/package.json(0 hunks)code/addons/links/package.json(0 hunks)code/addons/onboarding/package.json(0 hunks)code/addons/pseudo-states/package.json(0 hunks)code/addons/themes/package.json(0 hunks)code/addons/vitest/package.json(0 hunks)code/addons/vitest/src/postinstall.ts(1 hunks)code/builders/builder-vite/package.json(0 hunks)code/builders/builder-webpack5/package.json(0 hunks)code/core/package.json(0 hunks)code/frameworks/angular/package.json(0 hunks)code/frameworks/ember/package.json(0 hunks)code/frameworks/html-vite/package.json(0 hunks)code/frameworks/nextjs-vite/package.json(0 hunks)code/frameworks/nextjs/package.json(0 hunks)code/frameworks/preact-vite/package.json(0 hunks)code/frameworks/react-native-web-vite/package.json(0 hunks)code/frameworks/react-vite/package.json(0 hunks)code/frameworks/react-webpack5/package.json(0 hunks)code/frameworks/server-webpack5/package.json(0 hunks)code/frameworks/svelte-vite/package.json(0 hunks)code/frameworks/sveltekit/package.json(0 hunks)code/frameworks/vue3-vite/package.json(0 hunks)code/frameworks/web-components-vite/package.json(0 hunks)
💤 Files with no reviewable changes (24)
- code/frameworks/vue3-vite/package.json
- code/frameworks/react-webpack5/package.json
- code/addons/onboarding/package.json
- code/frameworks/angular/package.json
- code/builders/builder-vite/package.json
- code/addons/docs/package.json
- code/builders/builder-webpack5/package.json
- code/core/package.json
- code/addons/links/package.json
- code/addons/pseudo-states/package.json
- code/frameworks/sveltekit/package.json
- code/frameworks/nextjs-vite/package.json
- code/frameworks/svelte-vite/package.json
- code/addons/a11y/package.json
- code/frameworks/preact-vite/package.json
- code/frameworks/server-webpack5/package.json
- code/frameworks/ember/package.json
- code/frameworks/web-components-vite/package.json
- code/addons/vitest/package.json
- code/addons/themes/package.json
- code/frameworks/nextjs/package.json
- code/frameworks/react-vite/package.json
- code/frameworks/html-vite/package.json
- code/frameworks/react-native-web-vite/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .circleci/config.yml
🧰 Additional context used
🪛 Biome (2.1.2)
code/addons/vitest/src/postinstall.ts
[error] 122-122: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 122-122: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 122-122: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 122-122: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 122-122: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 123-123: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 126-126: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 126-126: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 126-126: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 126-126: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 126-127: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 129-129: Shouldn't redeclare 'versionedDependencies'. Consider to delete it or rename it.
'versionedDependencies' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
| const versionedDependencies = uniqueDependencies.map((p) => { | ||
| if (p.includes('vitest')) { | ||
| return vitestVersionSpecifier ? `${p}@${vitestVersionSpecifier.split('::')[0]}` : p; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical logic error: variable redeclaration and undefined reference.
Line 129 redeclares versionedDependencies, which was already defined at line 100 from addonVitestService.collectDependencies(). The code also attempts to map over uniqueDependencies, which is not defined anywhere in the visible scope. This would cause a ReferenceError at runtime.
The intended logic appears to be adding version specifiers to vitest-related dependencies. However, the implementation has several issues:
- Should reassign rather than redeclare:
versionedDependencies = versionedDependencies.map(...) - If coverage-v8 needs to be conditionally added (per the AI summary), it should be pushed to
versionedDependenciesbefore the mapping - The coverage addition logic from lines 122-127 should be integrated properly
Proposed fix (after removing lines 122-127):
+ if (!v8Version && !istanbulVersion) {
+ versionedDependencies.push('@vitest/coverage-v8');
+ }
+ }
+
- const versionedDependencies = uniqueDependencies.map((p) => {
+ const versionedDependenciesWithVersions = versionedDependencies.map((p) => {
if (p.includes('vitest')) {
return vitestVersionSpecifier ? `${p}@${vitestVersionSpecifier.split('::')[0]}` : p;
}
+ return p;Note: Line 132 appears incomplete - it should have a return p; statement to handle non-vitest packages. Please verify the complete mapping logic and update line 136 to use versionedDependenciesWithVersions instead of versionedDependencies.
Run the following script to check if there are other references to these variables:
🏁 Script executed:
#!/bin/bash
# Search for all references to uniqueDependencies and versionedDependencies
rg -n -C3 'uniqueDependencies|versionedDependencies' --type tsLength of output: 3212
🏁 Script executed:
#!/bin/bash
# Get more context around lines 95-145 to see full variable declarations and usage
cat code/addons/vitest/src/postinstall.ts | sed -n '95,145p' | cat -nLength of output: 2450
Critical runtime errors: undefined variable, redeclaration, incomplete mapping, and misplaced logic.
The code at lines 129-141 contains multiple fatal errors:
-
Undefined variable: Line 126 references
uniqueDependencies, which is never declared. This should beversionedDependencies(the variable assigned at line 100). -
Redeclaration: Line 129 redeclares
versionedDependencies, shadowing the original from line 100, and attempts to map the undefineduniqueDependencies. -
Incomplete mapping: Lines 130-132 are missing a
returnstatement for non-vitest packages, causing the map to returnundefinedfor those entries. -
Structural error: Lines 134-141 (the installation logic) are incorrectly nested inside the map callback. They should execute after the mapping completes.
Proposed fix:
- uniqueDependencies.push(`@vitest/coverage-v8`);
+ versionedDependencies.push(`@vitest/coverage-v8`);
}
- const versionedDependencies = uniqueDependencies.map((p) => {
+ versionedDependencies = versionedDependencies.map((p) => {
if (p.includes('vitest')) {
return vitestVersionSpecifier ? `${p}@${vitestVersionSpecifier.split('::')[0]}` : p;
}
+ return p;
-
+ });
+
if (versionedDependencies.length > 0) {
logger.step('Adding dependencies to your package.json');
logger.log(' ' + versionedDependencies.join(', '));Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 129-129: Shouldn't redeclare 'versionedDependencies'. Consider to delete it or rename it.
'versionedDependencies' is defined here:
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
In code/addons/vitest/src/postinstall.ts around lines 129-132 (and referencing
the earlier declaration at ~line 100 and the install logic at ~lines 134-141):
the map is redeclaring and shadowing versionedDependencies and is iterating an
undefined uniqueDependencies, missing a return for non-vitest packages, and
contains installation logic inside the map callback; change the map to operate
on the existing versionedDependencies (do not redeclare the variable), iterate
the correct array (the one declared at line ~100), ensure the callback returns p
for non-vitest entries (i.e., return p when not matching 'vitest'), and move the
installation logic currently nested inside the map callback so it runs after the
mapping completes.
# Conflicts: # code/package.json
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/tasks/dev.ts (1)
19-60: Tighten dry-run behavior and add visibility when reusing an existing portThe new
runimplementation looks consistent with the port-awarereadycheck and correctly waits for both preview and manager before recording benchmarks. Two follow-ups you might consider:
- In
dryRunmode we still callwaitOn(...)and will likely hit the 10s timeout because no server is actually started. Short‑circuiting whendryRunis true (e.g., returncontrollerimmediately after computingdevCommand) would make dry runs faster and less surprising.- When
detectFreePort(port) !== portand we decide not to start Storybook, we silently no-op. Emitting a debug/info log in that branch would make it clearer in CI/dev logs that we’re intentionally reusing an existing listener rather than silently skipping startup.If you’d like, I can sketch a small diff for both tweaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.circleci/config.yml(4 hunks)scripts/tasks/dev.ts(1 hunks)scripts/tasks/e2e-tests-build.ts(3 hunks)scripts/tasks/serve.ts(2 hunks)scripts/tasks/test-runner-build.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/tasks/e2e-tests-build.ts
- .circleci/config.yml
- scripts/tasks/test-runner-build.ts
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/tasks/dev.ts (1)
scripts/ecosystem-ci/before-test.js (1)
sandboxDir(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (5)
scripts/tasks/dev.ts (1)
16-18: Port-aware readiness logic is correct and verifiedThe
ready({ port })implementation correctly checks whether the requested port is already in use by comparing the detected port against the requested port. This matches thedetect-port1.6.1 contract, wheredetect(port)resolves to the same port when free and a different port when already in use.scripts/tasks/serve.ts (4)
4-6: LGTM: Imports support the dynamic port workflow.The new imports for
getPortandROOT_DIRECTORYare necessary for the per-sandbox port allocation and workspace root changes.
17-20: LGTM: Ready check correctly uses dynamic port.The
readyfunction correctly determines if the service is already running by checking if the dynamically computed port is occupied. The logic is sound:detectFreePort(port) !== portreturnstruewhen the port is in use (service ready).
9-11: The PORT constant is actively used and should not be removed.The review comment incorrectly assumes
PORTis dead code because it's not used withinserve.tsitself. However, verification showsPORTis exported and actively imported in three other files:
scripts/tasks/test-runner-build.tsscripts/tasks/e2e-tests-build.tsscripts/tasks/bench.tsThe fact that
serve.tsinternally usesgetPort()instead does not make the exportedPORTconstant unused.Likely an incorrect or invalid review comment.
21-39: ****The
builtSandboxDiris always constructed as an absolute path viajoin(path.isAbsolute(sandboxDir) ? sandboxDir : join(ROOT_DIRECTORY, sandboxDir), dir ?? templateKey.replace('/', '-')). Since the first argument tojoin()is always absolute (eithersandboxDiritself if absolute, orjoin(ROOT_DIRECTORY, sandboxDir)if relative), the resulting path is always absolute. Absolute paths resolve consistently regardless of the working directory set inexec(), so there is no path resolution concern inserve.tsor elsewhere.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/tasks/sandbox-parts.ts (3)
413-413: Remove debug console.log statement.This debug logging was previously flagged for removal before merging.
555-555: Remove debug console.log statement.This debug logging was previously flagged for removal before merging.
262-262: Path remains incorrect—esbuild-loader will not be found at runtime.This issue was previously flagged: the path assumes
esbuild-loaderis installed at rootnode_modules, but it's declared incode/package.jsonand will be atcode/node_modules/esbuild-loader. The path../../node_modules/esbuild-loaderfromscripts/tasks/will fail withMODULE_NOT_FOUND.
🧹 Nitpick comments (1)
scripts/utils/main-js.ts (1)
3-4: Consider using Node.js built-in path utilities.The ESLint disable suggests
slashis flagged by project dependency rules. Modern Node.js providespath.posix.normalize()or similar utilities that could potentially replace this external dependency, eliminating the need for the disable comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tasks/sandbox-parts.ts(11 hunks)scripts/utils/main-js.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-24T13:04:58.631Z
Learnt from: cylewaitforit
Repo: storybookjs/storybook PR: 31965
File: code/lib/eslint-plugin/src/rules/only-csf3.ts:31-33
Timestamp: 2025-09-24T13:04:58.631Z
Learning: The Storybook ESLint plugin supports ESLint v8.57+ where context.sourceCode is already available as a property since it was introduced in v8.40.0, so no fallback to context.getSourceCode() is needed in rules.
Applied to files:
scripts/utils/main-js.ts
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
scripts/tasks/sandbox-parts.ts
🧬 Code graph analysis (1)
scripts/tasks/sandbox-parts.ts (1)
scripts/utils/filterExistsInRootDir.ts (1)
filterExistsInRootDir(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (4)
scripts/tasks/sandbox-parts.ts (4)
5-5: Import updates align with workspace restructuring.The import changes correctly support the monorepo restructuring:
- Added
pathdefault import for path.sep usage in path manipulation- Updated
SupportedLanguageto import from src instead of dist- Added
ROOT_DIRECTORYconstant for new root-based path resolution- Switched to
filterExistsInRootDirutility for the new structureAlso applies to: 24-24, 29-29, 31-31
556-556: Improved comma handling logic.The updated logic correctly handles both cases where a comma is not needed:
- After an opening brace
{(newly created object)- After an existing comma
,This prevents syntax errors when inserting the
tagsproperty into the storybookTest configuration.
650-650: ROOT_DIRECTORY migrations are consistent with workspace restructuring.All references have been correctly updated from
CODE_DIRECTORYtoROOT_DIRECTORY:
- Renderer and framework template path resolutions now use root-relative paths
- Story existence filtering switched to
filterExistsInRootDir- Angular compodoc script references the scripts directory at root level
These changes align with the PR objective of making the git root the workspace root.
Also applies to: 666-666, 689-689, 701-701, 777-777, 1043-1043
405-408: Path stripping logic is correct and requires no changes.Verification confirms the implementation properly handles the workspace restructuring:
- All
packageDirinputs originate fromworkspacePath(), which returns paths with the "code/" prefix per the workspace configuration ("code/core", "code/addons/*", etc.)- The filter condition correctly removes only the first "code" segment:
it === 'code' && i === 0- Path reconstruction via
join(CODE_DIRECTORY, packageDir, ...)whereCODE_DIRECTORY = ROOT_DIRECTORY/codeyields correct filesystem paths- No edge cases exist since all workspace paths consistently follow the "code/*" pattern
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/tasks/e2e-tests-build.ts (1)
36-36: Consider making the timeout configurable for slower environments.The 20-second timeout is reasonable for most cases, but might be insufficient for slow CI runners or complex sandboxes with many dependencies. Consider extracting this to a constant or environment variable to allow adjustment without code changes.
Example:
+const WAIT_TIMEOUT = process.env.E2E_WAIT_TIMEOUT + ? parseInt(process.env.E2E_WAIT_TIMEOUT, 10) + : 20000; + - await waitOn({ resources: [`http://localhost:${port}`], interval: 16, timeout: 20000 }); + await waitOn({ resources: [`http://localhost:${port}`], interval: 16, timeout: WAIT_TIMEOUT });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tasks/e2e-tests-build.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/tasks/e2e-tests-build.ts (1)
scripts/sandbox/utils/getPort.ts (1)
getPort(4-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (2)
scripts/tasks/e2e-tests-build.ts (2)
1-54: Good improvements to E2E test reliability with dynamic port management.The addition of
waitOnto ensure the server is ready before running tests is a solid improvement that should reduce flaky test failures. The dynamic port computation viagetPortaligns with the broader port-management patterns mentioned in the PR objectives and integrates well with the monorepo restructuring.Subject to verification of the port computation logic, these changes enhance the robustness of the E2E testing infrastructure.
20-21: No issues found—port computation logic is correct.Verification shows that
selectedTaskwill always be'e2e-tests'when this task runs (set at task.ts:414 to the current taskKey). The ternary correctly evaluates togetPort({ key, selectedTask: 'serve' }), which aligns with the task'sdependsOn: ['serve']. The else branch is unreachable in normal execution and serves as defensive fallback. The code is safe as written.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
scripts/run-registry.ts (3)
63-67: Fix minor log message formattingThe redirect log is missing a space after the emoji, which reads a bit cramped in logs.
- logger.log(`🌿redirecting 6001 request to verdaccio on 6002`); + logger.log(`🌿 redirecting 6001 request to verdaccio on 6002`);
163-170: Use ROOT_DIRECTORY when resolving package locations for publish (cwd‑dependent bug)
resolvedLocation = resolvePath('../code', location)depends onprocess.cwd(). If this script is run from the repo root (likely after moving the Yarn root there),path.resolve('../code', location)will resolve relative to/and can point outside the repo (e.g.,/code/...) instead of<repo>/code/....Since
locationfromgetCodeWorkspaces(false)is now relative to thecodedirectory, it’s safer to resolve from the known root:- const loggedLocation = location.replace(resolvePath(join(__dirname, '..')), '.'); - const resolvedLocation = resolvePath('../code', location); + const resolvedLocation = resolvePath(ROOT_DIRECTORY, 'code', location); + const loggedLocation = resolvedLocation.replace(ROOT_DIRECTORY, '.');This makes resolution independent of cwd and consistent with other uses of
ROOT_DIRECTORY.#!/bin/bash # Sanity-check how path.resolve behaves from different cwd values node - << 'EOF' const path = require('node:path'); for (const cwd of ['/repo', '/repo/scripts']) { process.chdir(cwd); console.log(`\nCWD = ${cwd}`); console.log('resolve("../code", "addons/a11y") =', path.resolve('../code', 'addons/a11y')); } EOF
254-259: Signal handlers are functional; optional centralization of shutdown logicThe signal handlers correctly invoke
servers?.close()and thenprocess.exit(0), reusing theServers.closeabstraction. If you want to avoid duplicate shutdown paths and make behavior uniform with the error handler, you could factor a sharedshutdown()helper that closes servers and exits with the desired code, then call it from both the signal handlers andrun().catch.Not strictly necessary, but it would centralize lifecycle behavior.
🧹 Nitpick comments (2)
scripts/utils/workspace.ts (1)
11-37:getCodeWorkspacesbehavior matches callers, minor docstring nitThe new
getCodeWorkspacesimplementation (runningyarn workspaces listfromROOT_DIRECTORY, excluding@storybook/scripts/@storybook/code, and stripping the leadingcode/segment) correctly produces locations that are relative to thecodedirectory, which matches usages inscripts/release/version.tsand other scripts that prefix withcode/. The memoized wrapper is also updated appropriately.Minor: the docblock could be clarified to explicitly state that
locationis “relative to the/codedirectory” to avoid confusion for future consumers.scripts/run-registry.ts (1)
40-48:Serversabstraction and startup race look correct; consider tightening typingThe
Serverstype and thestartVerdaccioimplementation (resolving once both proxy and Verdaccio are listening, plus a 10s timeout) look reasonable, andservers.close()properly closes both servers and verifies their ports are free.For clarity and stronger typing, you could:
- Type the first promise explicitly as
new Promise<Servers>((resolve) => { … }), and- Return
await Promise.race<Servers>([…])(dropping theas Promise<Servers>cast),to avoid any ambiguity around the inferred return type of
startVerdaccio.Also applies to: 76-95, 96-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.yarnrc.yml(1 hunks)scripts/build-package.ts(1 hunks)scripts/check-package.ts(1 hunks)scripts/release/version.ts(2 hunks)scripts/run-registry.ts(7 hunks)scripts/tasks/e2e-tests-build.ts(3 hunks)scripts/tasks/sandbox-parts.ts(4 hunks)scripts/utils/workspace.ts(1 hunks)scripts/utils/yarn.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/utils/yarn.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
.yarnrc.ymlscripts/release/version.ts
🧬 Code graph analysis (5)
scripts/build-package.ts (1)
scripts/utils/workspace.ts (1)
getCodeWorkspaces(15-35)
scripts/tasks/e2e-tests-build.ts (1)
scripts/sandbox/utils/getPort.ts (1)
getPort(4-9)
scripts/run-registry.ts (2)
scripts/npm-auth.ts (1)
npmAuth(21-52)scripts/utils/workspace.ts (1)
getCodeWorkspaces(15-35)
scripts/release/version.ts (2)
scripts/utils/workspace.ts (1)
getCodeWorkspaces(15-35)code/lib/create-storybook/src/services/VersionService.ts (1)
getCurrentVersion(10-12)
scripts/check-package.ts (1)
scripts/utils/workspace.ts (1)
getCodeWorkspaces(15-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (12)
scripts/tasks/sandbox-parts.ts (3)
540-554: LGTM—comma insertion logic is correct.The logic for determining whether a comma is needed when inserting the
tagsproperty into thestorybookTest()configuration correctly handles the cases wherebeforeends with{(empty object, first property) or,(already has trailing comma).
1035-1035: ROOT_DIRECTORY usage aligns with workspace restructuring.The use of
ROOT_DIRECTORYto construct the path to the scripts directory is consistent with this PR's objective of making the git root the yarn workspace root. The change correctly points to the scripts directory at the repository root.
31-31: The import statement is correct. The verification confirms that the filefilterExistsInRootDir.tsexports a function namedfilterExistsInCodeDir(line 17:export const filterExistsInCodeDir = async (...)). The import at line 31 correctly imports this function by its actual export name, and the usage at line 769 correctly calls it. The function export name was not changed—only the filename was updated. There is no runtime error or named export mismatch.Likely an incorrect or invalid review comment.
scripts/tasks/e2e-tests-build.ts (4)
2-4: LGTM! Appropriate imports for dynamic port management.The addition of
waitOnandgetPortimports aligns well with the new dynamic port allocation and server readiness checks introduced in this PR.
20-20: LGTM! Function signature appropriately extended.The addition of
key,sandboxDir, andselectedTaskparameters provides the necessary context for dynamic port computation and sandbox management.
36-36: Verify the 200-second timeout is appropriate.The
waitOntimeout of 200 seconds (3.3 minutes) is quite long. While this ensures tolerance for slow environments, it could mask underlying server startup issues and delay feedback in CI/CD pipelines. Please confirm this timeout is intentional and appropriate for your infrastructure.
41-42: No issues found—STORYBOOK_TYPEis correct.
STORYBOOK_TYPE: this.typecorrectly sets'build'because the task depends on'serve', which serves production builds. The conditional port selection on line 21 determines which server port to connect to based onselectedTask, not the build type. Tests correctly useSTORYBOOK_TYPEto detect production vs. dev environments, and the code handles both scenarios appropriately..yarnrc.yml (1)
1-35: Centralized Yarn 4 root config looks consistent
compressionLevel,enableGlobalCache,tsEnableAutoTypes,npmRegistryServer,supportedArchitectures, andunsafeHttpWhitelistare coherent for a Yarn v4 monorepo root and match the repo’s Yarn‑4 usage. Based on learnings, this looks aligned with the intended single-root setup.scripts/release/version.ts (1)
14-14: Versioning script correctly switched togetCodeWorkspacesSwapping to
getCodeWorkspaces()here is consistent with the new workspace helper, andbumpAllPackageJsons’sjoin(CODE_DIR_PATH, pkg.location, 'package.json')matches the updated convention wherelocationis relative to thecodedirectory. This should avoid the previouscode/code/...path duplication.No further changes needed from this file’s side.
Also applies to: 246-246
scripts/build-package.ts (3)
24-24: LGTM!The import change from
getWorkspacestogetCodeWorkspacescorrectly aligns with the workspace restructuring changes inscripts/utils/workspace.ts.
27-27: LGTM!The function call correctly uses
getCodeWorkspaces(), maintaining the same filtering logic while adapting to the workspace restructuring.
168-176: The hardcoded 'code' paths are correct and intentionally designed.Verification confirms that
getCodeWorkspaces()explicitly strips the 'code' prefix from workspace locations (with a documented backwards compatibility comment), and the hardcoded paths in lines 168 and 176 correctly add it back. This coupling is intentional and working properly with the new workspace structure at the git root. The paths resolve correctly to actual package.json files and working directories.
| import { getCodeWorkspaces } from './utils/workspace'; | ||
|
|
||
| async function run() { | ||
| const packages = await getWorkspaces(); | ||
| const packages = await getCodeWorkspaces(); | ||
| const packageTasks = packages |
There was a problem hiding this comment.
🧩 Analysis chain
Align package.json path resolution with new getCodeWorkspaces locations
Now that getCodeWorkspaces() returns location relative to the code directory (e.g. addons/a11y), the readFile path:
const content = await readFile(resolve('../code', v.location, 'package.json'), 'utf-8');is still dependent on process.cwd(). If the script is run from the repo root instead of the scripts directory, this can resolve outside the repo (/code/...) rather than <repo>/code/....
You’re already using an __dirname‑based path for cwd; it’s safer to mirror that here:
- const content = await readFile(resolve('../code', v.location, 'package.json'), 'utf-8');
+ const content = await readFile(
+ resolve(__dirname, '..', 'code', v.location, 'package.json'),
+ 'utf-8'
+ );This keeps the behavior consistent and independent of where the command is invoked from.
Also applies to: 115-119
🏁 Script executed:
#!/bin/bash
# Demonstrate cwd-dependent resolution
node - << 'EOF'
const path = require('node:path');
for (const cwd of ['/repo', '/repo/scripts']) {
process.chdir(cwd);
console.log(`\nCWD = ${cwd}`);
console.log('resolve("../code", "addons/a11y", "package.json") =',
path.resolve('../code', 'addons/a11y', 'package.json'));
}
EOFLength of output: 856
🏁 Script executed:
#!/bin/bash
# Inspect the check-package.ts file to verify the code and lines mentioned
cat scripts/check-package.tsLength of output: 4092
🏁 Script executed:
#!/bin/bash
# Find and inspect the getCodeWorkspaces function definition
find . -name "*.ts" -o -name "*.js" | xargs grep -l "getCodeWorkspaces" | head -5Length of output: 137
🏁 Script executed:
#!/bin/bash
# Check the workspace utilities
cat scripts/utils/workspace.ts 2>/dev/null || cat scripts/utils/workspace.js 2>/dev/null || echo "File not found in standard locations"Length of output: 1495
Keep the package.json path resolution consistent with the cwd-based approach
The review comment is accurate. The readFile call at line 113 uses a cwd-dependent relative path, while the execa cwd at line 115 already uses __dirname for independence. Since getCodeWorkspaces() returns location with the code/ prefix stripped (e.g., addons/a11y), the inconsistency means the script behaves differently depending on the working directory from which it's invoked.
Apply the suggested fix to line 113:
- const content = await readFile(resolve('../code', v.location, 'package.json'), 'utf-8');
+ const content = await readFile(
+ resolve(__dirname, '..', 'code', v.location, 'package.json'),
+ 'utf-8'
+ );This aligns the path resolution with the cwd logic and makes the script independent of the invocation directory.
🤖 Prompt for AI Agents
In scripts/check-package.ts around lines 14 to 18, the readFile call constructs
package.json with a relative ../code path which is cwd-dependent while later
commands set cwd using resolve(__dirname, '..', 'code', v.location); change the
package.json path construction to use the same resolve(__dirname, '..', 'code',
v.location, 'package.json') pattern (or derive it from the same cwd variable) so
both file reads and execa invocations use identical,
invocation-directory-independent paths.
| if (opts.publish) { | ||
| // when running e2e locally, clear cache to avoid EPUBLISHCONFLICT errors | ||
| const verdaccioCache = resolvePath(__dirname, '..', '.verdaccio-cache'); | ||
| const verdaccioCache = join(ROOT_DIRECTORY, '.verdaccio-cache'); | ||
| if (await pathExists(verdaccioCache)) { | ||
| logger.log(`🗑 cleaning up cache`); | ||
| await rm(verdaccioCache, { force: true, recursive: true }); | ||
| } | ||
| } | ||
|
|
||
| logger.log(`🎬 starting verdaccio (this takes ±5 seconds, so be patient)`); | ||
|
|
||
| const [verdaccioServer, packages, version] = await Promise.all([ | ||
| startVerdaccio(), | ||
| getWorkspaces(false), | ||
| currentVersion(), | ||
| ]); | ||
|
|
||
| logger.log(`🌿 verdaccio running on ${verdaccioUrl}`); | ||
|
|
||
| logger.log(`👤 add temp user to verdaccio`); | ||
| // Use npmAuth helper to authenticate to the local Verdaccio registry | ||
| // This will create a .npmrc file in the root directory | ||
| await npmAuth({ | ||
| username: 'foo', | ||
| password: 's3cret', | ||
| email: '[email protected]', | ||
| registry: 'http://localhost:6002', | ||
| outputDir: root, | ||
| }); | ||
|
|
||
| logger.log( | ||
| `📦 found ${packages.length} storybook packages at version ${picocolors.blue(version)}` | ||
| ); | ||
|
|
||
| if (opts.publish) { | ||
| await publish(packages, 'http://localhost:6002'); | ||
| } | ||
|
|
||
| await rm(join(root, '.npmrc'), { force: true }); | ||
| // Use npmAuth helper to authenticate to the local Verdaccio registry | ||
| // This will create a .npmrc file in the root directory | ||
| logger.log(`👤 add temp user to verdaccio`); | ||
| await npmAuth({ | ||
| username: 'foo', | ||
| password: 's3cret', | ||
| email: '[email protected]', | ||
| registry: 'http://localhost:6002', | ||
| outputDir: root, | ||
| }); | ||
| try { | ||
| const [packages, version] = await Promise.all([getCodeWorkspaces(false), currentVersion()]); | ||
| logger.log( | ||
| `📦 found ${packages.length} storybook packages at version ${picocolors.blue(version)}` | ||
| ); | ||
| await publish(packages, 'http://localhost:6002'); | ||
| } finally { | ||
| await rm(join(root, '.npmrc'), { force: true }); | ||
| } | ||
|
|
||
| if (!opts.open) { | ||
| verdaccioServer.close(); | ||
| process.exit(0); | ||
| if (!opts.open) { | ||
| await servers?.close(); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure failures cause a non‑zero exit code and always clean up Verdaccio
Two related concerns in the new run path:
- In the success path with
opts.publish && !opts.open, you callawait servers?.close(); process.exit(0);, which is fine. - In the error path (
run().catch(async (e) => { … })), you log the error and close servers, but do not exit with a non‑zero status, so CI or callers may treat failures as success.
To make failures visible and keep cleanup consistent, consider:
run().catch(async (e) => {
logger.error(e);
await servers?.close();
-});
+ process.exit(1);
+});This ensures any error in run() results in a failing exit code after attempting a graceful shutdown.
Also applies to: 249-252
🏁 Script executed:
#!/bin/bash
# Example: simulate a failing run() to see current exit code
node - << 'EOF'
(async () => {
try {
// Simulate run() throwing
throw new Error('simulated failure');
} catch (e) {
console.error('logged error:', e.message);
// Note: no process.exit(1) here would yield exit code 0
}
})();
EOF
echo "Actual exit code: $?"Length of output: 51
I need to inspect the actual error handler in the codebase to verify the current state.
🏁 Script executed:
#!/bin/bash
# Inspect the run().catch() handler in scripts/run-registry.ts
rg -n "run\(\)\.catch" scripts/run-registry.ts -A 5 -B 2Length of output: 198
🏁 Script executed:
#!/bin/bash
# Also search for the error handler pattern more broadly
rg -n "\.catch\(" scripts/run-registry.ts -A 8 -B 2Length of output: 790
🏁 Script executed:
#!/bin/bash
# Check lines 249-252 specifically
sed -n '245,255p' scripts/run-registry.tsLength of output: 208
🏁 Script executed:
#!/bin/bash
# Get broader context around the run() function call
sed -n '240,260p' scripts/run-registry.tsLength of output: 347
Add process.exit(1) to the error handler for proper failure signaling
The error handler at lines 249-252 logs errors and closes servers but exits with code 0, causing CI and callers to treat failures as success. Add process.exit(1) after cleanup:
run().catch(async (e) => {
logger.error(e);
await servers?.close();
+ process.exit(1);
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/run-registry.ts around lines 215 to 245, the error handler currently
logs the error and closes servers but exits with code 0 which signals success;
modify the catch/finally/error block to call process.exit(1) after awaiting
servers?.close() (or ensuring servers are closed) so the process exits with a
non-zero status on failure; ensure any async cleanup completes before calling
process.exit(1).
| }, | ||
| async run({ codeDir, junitFilename, key, sandboxDir }, { dryRun, debug }) { | ||
| async run({ codeDir, junitFilename, key, sandboxDir, selectedTask }, { dryRun, debug }) { | ||
| const port = getPort({ key, selectedTask: selectedTask === 'e2e-tests' ? 'serve' : 'dev' }); |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the conditional task mapping logic.
The conditional selectedTask === 'e2e-tests' ? 'serve' : 'dev' is unclear. Since this is the e2eTestsBuild task, when would selectedTask be anything other than 'e2e-tests'? Please clarify the execution scenarios where this conditional matters.
Additionally, verify whether the static port: PORT property on line 15 is still used elsewhere, as it appears to be superseded by the dynamic port calculation.
Run this script to check for references to the static port property:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of e2eTestsBuild.port to verify if static port is still needed
# Search for references to e2eTestsBuild.port
rg -nP --type=ts 'e2eTestsBuild\.port\b' -C3Length of output: 0
I need to run additional verification scripts to understand the context better.
🏁 Script executed:
#!/bin/bash
# Search for broader port references and how e2eTestsBuild is used
# 1. Find all references to port in the e2e-tests-build.ts file
echo "=== Port references in e2e-tests-build.ts ==="
rg -n 'port' scripts/tasks/e2e-tests-build.ts
# 2. Find where e2eTestsBuild is instantiated or used
echo -e "\n=== References to e2eTestsBuild ==="
rg -nP --type=ts '\be2eTestsBuild\b' -C2
# 3. Search for selectedTask usage to understand when it differs from 'e2e-tests'
echo -e "\n=== selectedTask usage ==="
rg -nP --type=ts 'selectedTask' -C2Length of output: 5767
🏁 Script executed:
#!/bin/bash
# Verify the exact port values and how they relate to each other
# 1. Check what PORT values are imported from serve and dev
echo "=== PORT values from serve and dev ==="
rg -n 'export.*PORT' scripts/tasks/serve.ts scripts/tasks/dev.ts
# 2. Check how e2eTestsDev is structured
echo -e "\n=== e2eTestsDev full definition ==="
cat scripts/tasks/e2e-tests-dev.ts
# 3. Verify how port is accessed at the TemplateDetails level
echo -e "\n=== Port usage in TemplateDetails context ==="
rg -nP --type=ts 'template\.port|details\.port|TemplateDetails.*port' -C2Length of output: 545
Remove or update the static port property—it doesn't match the dynamic port calculation.
The conditional selectedTask === 'e2e-tests' ? 'serve' : 'dev' is intentional: e2eTestsBuild's run function is reused by both the 'e2e-tests' task (which depends on serve) and e2eTestsDev (which depends on dev). When the selected task is 'e2e-tests-dev', the conditional maps to 'dev' for the port calculation.
However, the static port: PORT property on line 15 is misleading. The actual port used by the run function comes from getPort(), which calculates the port as 3000 + templateIndex * 100 + taskIndex—not from the hardcoded PORT environment variable. This static property should either be removed or updated to reflect the actual dynamic calculation. Consider removing it if it's not consumed elsewhere.
🤖 Prompt for AI Agents
In scripts/tasks/e2e-tests-build.ts around lines 15 and 21, the file defines a
static port property (line 15) that is misleading because the run function uses
getPort(...) (line 21) to compute a dynamic port based on templateIndex and
taskIndex; remove the static port: PORT property if it is not used elsewhere, or
replace it with a getter/function that returns the same dynamic calculation as
getPort(...) so the exported port value always matches the actual port logic
used by run. Ensure any consumer of the static property is updated to call the
new getter or to rely on getPort(...) so there is no mismatch.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tasks/sandbox-parts.ts (1)
259-275: Escapeesbuild-loaderpath before embedding into generated webpack code.The code at line 262–273 directly interpolates
require.resolve()output into a template string without escaping. On Windows,require.resolve('../../node_modules/esbuild-loader')returns a path with backslashes (e.g.,C:\path\to\esbuild-loader), which when embedded asloader: '${esbuildLoaderPath}'produces invalid JavaScript string literals with unintended escape sequences. This causesbabelParse()at line 300 to fail on Windows builds.Wrap the resolved path with
JSON.stringify()to ensure proper escaping:const require = createRequire(import.meta.url); const esbuildLoaderPath = require.resolve('../../node_modules/esbuild-loader'); + const esbuildLoaderPathLiteral = JSON.stringify(esbuildLoaderPath); const webpackFinalCode = ` (config) => ({ ...config, module: { ...config.module, rules: [ // Ensure esbuild-loader applies to all files in ./template-stories { test: [/\\/template-stories\\//], exclude: [/\\.mdx$/], - loader: '${esbuildLoaderPath}', + loader: ${esbuildLoaderPathLiteral},
🧹 Nitpick comments (2)
scripts/tasks/sandbox-parts.ts (2)
24-31: Align helper name with new root-based utility (and verify export).You’re importing
filterExistsInCodeDirfrom../utils/filterExistsInRootDir. Even if that module re‑exports the symbol under this name, the mismatch between the import name and the file’s purpose is confusing and could be a compile error if the export was renamed tofilterExistsInRootDir.Consider either:
- Renaming the import (and call sites) to
filterExistsInRootDir, or- Re‑exporting under both names in
filterExistsInRootDir.tsand importing the root‑dir name here.This keeps naming consistent with the new root‑based workspace layout and avoids surprises if the helper’s export name changes.
1033-1038: Quote--cwdpath indocs:jsonscript to handle spaces.The new script:
'docs:json': `DIR=$PWD; yarn --cwd ${join(ROOT_DIRECTORY, 'scripts')} jiti combine-compodoc $DIR`,will break if
ROOT_DIRECTORYcontains spaces (e.g.,.../My Projects/storybook), since the shell will split the--cwdargument.Safer version:
- 'docs:json': `DIR=$PWD; yarn --cwd ${join(ROOT_DIRECTORY, 'scripts')} jiti combine-compodoc $DIR`, + 'docs:json': `DIR=$PWD; yarn --cwd "${join(ROOT_DIRECTORY, 'scripts')}" jiti combine-compodoc $DIR`,This keeps the
--cwdargument robust regardless of where the repo is checked out.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/release/__tests__/version.test.ts(1 hunks)scripts/tasks/sandbox-parts.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
🔇 Additional comments (2)
scripts/release/__tests__/version.test.ts (1)
21-28: Workspace util mock rename matches new APIRenaming the mocked function to
getCodeWorkspaceskeeps the test aligned with the updated workspace utility without changing behavior. Looks good as-is.scripts/tasks/sandbox-parts.ts (1)
541-551: Improved comma handling when injectingtagsintostorybookTestconfig.The new
needsCommalogic correctly treats both an empty object ({}) and a trailing comma before}as cases where no extra comma should be inserted, reducing the chance of syntactically invalid configs when augmentingstorybookTest({...}).Looks good as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/tasks/test-runner-build.ts (2)
8-13: ClarifytestRunnerBuild.portvs dynamicgetPortusage
testRunnerBuildstill exposesport: PORT(fromserve.ts), but the actual runtime port is now computed viagetPort({ key, selectedTask: ... })insiderun. If any caller or tooling still relies ontestRunnerBuild.portas the authoritative port, it will diverge from whatrunuses.Consider either:
- Dropping the
{ port: number }extension from this task, or- Clearly documenting that
portis a legacy/static hint and not authoritative, or- Refactoring consumers to derive the port via
getPort(withkey/task) instead of readingtestRunnerBuild.port.This will avoid subtle mismatches between metadata and actual behavior.
Also applies to: 16-18
16-17: Ensurekey/selectedTaskare always provided and consider hardeninggetPortusageThe dynamic port resolution via:
const port = getPort({ key, selectedTask: selectedTask === 'test-runner' ? 'serve' : 'dev' });is consistent with the
serve/devtasks, and thewaitOncall before running tests is a good safety net. The main fragility is that this assumesrunis always invoked with a validkeyandselectedTask:
- If either is
undefined,getPortwill compute a port using an invalid template index, which may be surprising and hard to debug.- The conditional mapping from
'test-runner'to'serve'is also implicit knowledge; if new task names are introduced, this may need updating.If the
Tasktype allows it, you might:
- Strengthen the type for
run’s first argument here (e.g., a dedicated interface that requireskeyandselectedTask), and/or- Add a small runtime assertion that
keyandselectedTaskare defined before callinggetPort, failing fast with a clear error.This will make misconfigurations much easier to diagnose.
Also applies to: 21-21, 28-28
scripts/tasks/serve.ts (1)
4-6: Dynamic serve ports look good; double‑checkkeycontract andbuiltSandboxDirrelative toROOT_DIRECTORYThe switch to
getPort({ key, selectedTask: 'serve' })in bothreadyandrun, plus thedetectFreePortguard andwaitOnontcp:127.0.0.1:${port}, makes the serve task behave consistently with the new per-sandbox port scheme.Two things to verify:
keyavailability
ready({ key })andrun({ builtSandboxDir, key }, ...)now both depend onkeybeing present and valid forgetPort. If any existing caller was invokingservewithout akey, this will now fail or produce an unexpected port. It’s worth confirming that all orchestrators (Nx tasks, scripts, etc.) are updated to always passkey.CWD vs path semantics
http-serveris now run withcwd: ROOT_DIRECTORYand the directory argument set tobuiltSandboxDir. This is fine as long asbuiltSandboxDiris either:
- Absolute, or
- Relative to
ROOT_DIRECTORY.If
builtSandboxDirused to be resolved relative to some other directory, you may want to ensure all call sites now construct it withROOT_DIRECTORYin mind, or normalize it withpath.resolve(ROOT_DIRECTORY, builtSandboxDir).Nothing here looks blocking, but a quick check of those assumptions would prevent subtle CI issues.
Also applies to: 17-19, 21-29, 35-36
scripts/tasks/dev.ts (1)
16-18: Port‑driven dev flow is consistent; confirm callers always provide the expectedportThe refactor to make
devfully port-driven (ready({ port }),run({ sandboxDir, port }, ...), and using thatportfordetectFreePort,waitOn, and bench metrics) lines up with the new dynamic port orchestration.A couple of points to validate:
Caller contract: All existing invocation sites need to provide a concrete
portnow (most likely viagetPort). If any older code was relying ondevinferring its own port (e.g., via a constant), it will now misbehave. Strengthening the type of the first argument to requireport(or adding a short runtime assertion) would help catch misuses early.Readiness semantics:
ready({ port })returnstruewhendetectFreePort(port) !== port, i.e., something is already listening on that port. This matches the usual “service is up” meaning, andrunwill then no-op when the port is busy. Just confirm that your task runner never callsrunwhenreadyis alreadytruefor this task, so you don’t end up returning a controller that isn’t tied to any process.Functionally this looks solid; these are mostly about making the contract explicit and future‑proof.
Also applies to: 19-23, 35-50, 52-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/tasks/dev.ts(1 hunks)scripts/tasks/serve.ts(2 hunks)scripts/tasks/test-runner-build.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
scripts/tasks/test-runner-build.ts (2)
scripts/sandbox/utils/getPort.ts (1)
getPort(4-9)scripts/ecosystem-ci/before-test.js (1)
sandboxDir(34-34)
scripts/tasks/serve.ts (1)
scripts/sandbox/utils/getPort.ts (1)
getPort(4-9)
scripts/tasks/dev.ts (1)
scripts/ecosystem-ci/before-test.js (1)
sandboxDir(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: nx
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Build & Infrastructure
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.