Build: Update verdaccio logic #33282
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the verdaccio (local npm registry) setup for testing by simplifying the configuration, upgrading to verdaccio v6, and implementing unique version publishing. The changes enable proper caching and eliminate authentication complexity for the local testing environment.
Key Changes
- Unique version publishing: Packages are now published with unique versions using the pattern
${version}-local.${RUN_ID}, enabling safe use of global cache - Simplified verdaccio configuration: Removed authentication and extensive package-specific proxy rules in favor of dynamic routing based on workspace packages
- Upgraded verdaccio: Updated from v5.31.1 (optional dependency) to v6.2.3 (regular dependency)
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/verdaccio.yaml | Simplified configuration removing auth requirements and package-specific proxy rules; added offline publishing support |
| scripts/run-registry.ts | Complete refactoring of publishing logic to use unique versioned packages, eliminated npm-auth dependency, improved workspace dependency resolution |
| scripts/utils/yarn.ts | Removed storybook version resolutions and enabled global cache (now safe with unique versions) |
| scripts/package.json | Updated verdaccio from optional v5.31.1 to regular v6.2.3 dependency, removed auth-memory plugin |
| code/core/src/common/js-package-manager/*.ts | Added cwd parameter to package info commands for proper registry resolution |
| code/core/src/common/js-package-manager/JsPackageManager.ts | Added logic to detect and handle -local. versioned packages |
| code/core/src/bin/dispatcher.ts | Changed to use storybook package version instead of hardcoded versions for npx fallback |
| code/**/package.json (45 files) | Standardized repository URL format to include git+ prefix for npm compliance |
| } | ||
|
|
||
| const publishPackage = async (ws: Package, workspaces: Package[]) => { | ||
| const info = workspaces.find((it) => it.name === ws.name); |
There was a problem hiding this comment.
The info variable could be undefined if workspaces.find() doesn't find a matching package. This would cause a runtime error when accessing info.publishVersion on line 157. Consider adding a null check or throwing a descriptive error if the package is not found.
scripts/run-registry.ts
Outdated
| res.writeHead(302, { Location: 'http://localhost:6002' + req.url }); | ||
| if ( | ||
| req.url === '/' || | ||
| packages.some((it) => decodeURIComponent(req.url)?.startsWith('/' + it.name)) || |
There was a problem hiding this comment.
The optional chaining ?. in decodeURIComponent(req.url)?.startsWith() could mask an error. If req.url is undefined or null, decodeURIComponent() will throw an error before reaching the optional chaining. Consider adding a null check before calling decodeURIComponent(), like: req.url && decodeURIComponent(req.url).startsWith('/' + it.name)
| // We don't want to use the cache or we might get older copies of our built packages | ||
| // (with identical versions), as yarn (correctly I guess) assumes the same version hasn't changed | ||
| // TODO publish unique versions instead | ||
| `yarn config set enableGlobalCache false`, | ||
| `yarn config set enableGlobalCache true`, |
There was a problem hiding this comment.
The comment states "We don't want to use the cache or we might get older copies of our built packages (with identical versions)" but the code now sets enableGlobalCache to true. This comment is now misleading and should be updated to reflect the new approach where unique versions are published (using -local.RUN_ID suffix) which makes caching safe.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughStandardizes many package manifest repository URLs to git+https, adds cwd to package-manager proxy execs, introduces local-version handling, uses a runtime package.json import in dispatcher fallback, and reworks workspace-aware local publish/registry tooling plus CI sandbox workflow waits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as run-registry.ts
participant Temp as Temp Dir (per-package)
participant Verd as Verdaccio (local registry)
participant Public as Public npm
Runner->>Runner: discover workspaces -> getPackages()
loop per workspace (concurrent, p-limit)
Runner->>Temp: copy workspace files -> temp dir
Runner->>Temp: resolveWorkspaceDeps() -> replace workspace: deps with concrete versions
Runner->>Temp: write patched package.json and .npmrc (registry -> Verd)
Runner->>Verd: npm publish (registry URL, local tag)
Verd-->>Runner: publish response
Runner->>Temp: cleanup temp dir
end
alt request for workspace-scoped package
Client->>Verd: GET `@storybook/...`
Verd-->>Client: serve published package
else request for public package
Client->>Verd: proxy -> Public
Public-->>Client: serve package
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/utils/yarn.ts (1)
128-140: Update comment forconfigureYarn2ForVerdaccioto match actual behaviorThe comment claiming "We don't want to use the cache" contradicts the code setting
enableGlobalCache true. This is inconsistent withinstallYarn2, which sets the same value with the opposite explanation: "Use the global cache so we aren't re-caching dependencies each time we run sandbox."In Yarn 4,
enableGlobalCache: trueenables the shared cache across projects for performance. This is safe here because versions are uniquified per run viaRUN_ID(timestamp inrun-registry.ts), so there's no risk of stale tarballs. Update the comment to clarify that caching is intentional and beneficial:// Use the global cache for performance; safe because versions are uniquified per run with RUN_ID `yarn config set enableGlobalCache true`,
🧹 Nitpick comments (5)
code/core/src/bin/dispatcher.ts (1)
77-83: Dynamic import fallback looks correct, but consider error handling.The fallback path now correctly uses the actual
storybookpackage version for the npx command. However, if the dynamic import ofstorybook/package.jsonfails (e.g., corrupted installation), this will throw an unhandled exception. Consider wrapping it in a try-catch with a fallback to theversionsmap:+ let version: string; + try { + const { default: packageJson } = await import(`storybook/package.json`, { + with: { type: 'json' }, + }); + version = packageJson.version; + } catch { + version = versions[targetCli.pkg]; + } const child = executeCommand({ command: 'npx', - args: ['--yes', `${targetCli.pkg}@${packageJson.version}`, ...targetCli.args], + args: ['--yes', `${targetCli.pkg}@${version}`, ...targetCli.args], stdio: 'inherit', });scripts/verdaccio.yaml (1)
19-22: Consider using a less verbose log level for CI.Debug-level logging may produce excessive output during CI runs. Consider using
warnorinfolevel to reduce noise, or parameterize the level via environment variable.scripts/run-registry.ts (3)
150-151: Redundant lookup -infois alwayswsitself.The
infolookup on line 151 searches for the same package that was passed in. Sincewsalready contains all the required fields, this lookup is unnecessary:const publishPackage = async (ws: Package, workspaces: Package[]) => { - const info = workspaces.find((it) => it.name === ws.name); const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-')); // ... const pkg = JSON.parse(await readFile(join(tmpDir, 'package.json'), 'utf8')); - pkg.version = info.publishVersion; + pkg.version = ws.publishVersion;
241-253: Variable shadowing may cause confusion.The
packagesvariable on line 242 shadows the outerpackagesfrom line 228. Consider renaming topackagesWithVersionorpublishablePackagesfor clarity:if (opts.publish) { - const packages = await Promise.all( + const packagesWithVersion = await Promise.all( (await getCodeWorkspaces(false)).map(async (ws) => { // ... }) ); const limit = pLimit(maxConcurrentTasks); - await Promise.all(packages.map((p) => limit(() => publishPackage(p, packages)))); + await Promise.all(packagesWithVersion.map((p) => limit(() => publishPackage(p, packagesWithVersion)))); }
243-243: RedundantgetCodeWorkspacescall.
getCodeWorkspaces(false)is already called on line 230. Reuse the existingpackagesarray instead of fetching workspaces again:if (opts.publish) { - const packages = await Promise.all( - (await getCodeWorkspaces(false)).map(async (ws) => { + const packagesWithVersion = await Promise.all( + packages.map(async (ws) => { const path = join(CODE_DIRECTORY, ws.location);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
code/addons/a11y/package.json(1 hunks)code/addons/docs/package.json(1 hunks)code/addons/links/package.json(1 hunks)code/addons/onboarding/package.json(1 hunks)code/addons/pseudo-states/package.json(1 hunks)code/addons/themes/package.json(1 hunks)code/addons/vitest/package.json(1 hunks)code/builders/builder-vite/package.json(1 hunks)code/builders/builder-webpack5/package.json(1 hunks)code/core/package.json(1 hunks)code/core/src/bin/dispatcher.ts(1 hunks)code/core/src/common/js-package-manager/JsPackageManager.ts(2 hunks)code/core/src/common/js-package-manager/NPMProxy.ts(1 hunks)code/core/src/common/js-package-manager/PNPMProxy.ts(1 hunks)code/core/src/common/js-package-manager/Yarn1Proxy.ts(1 hunks)code/core/src/common/js-package-manager/Yarn2Proxy.ts(1 hunks)code/frameworks/angular/package.json(1 hunks)code/frameworks/ember/package.json(1 hunks)code/frameworks/html-vite/package.json(1 hunks)code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs/package.json(1 hunks)code/frameworks/preact-vite/package.json(1 hunks)code/frameworks/react-native-web-vite/package.json(1 hunks)code/frameworks/react-vite/package.json(1 hunks)code/frameworks/react-webpack5/package.json(1 hunks)code/frameworks/server-webpack5/package.json(1 hunks)code/frameworks/svelte-vite/package.json(1 hunks)code/frameworks/sveltekit/package.json(1 hunks)code/frameworks/vue3-vite/package.json(1 hunks)code/frameworks/web-components-vite/package.json(1 hunks)code/lib/cli-sb/package.json(1 hunks)code/lib/cli-storybook/package.json(1 hunks)code/lib/codemod/package.json(1 hunks)code/lib/core-webpack/package.json(1 hunks)code/lib/create-storybook/package.json(1 hunks)code/lib/csf-plugin/package.json(1 hunks)code/lib/eslint-plugin/package.json(1 hunks)code/lib/react-dom-shim/package.json(1 hunks)code/presets/create-react-app/package.json(1 hunks)code/presets/react-webpack/package.json(1 hunks)code/presets/server-webpack/package.json(1 hunks)code/renderers/html/package.json(1 hunks)code/renderers/preact/package.json(1 hunks)code/renderers/react/package.json(1 hunks)code/renderers/server/package.json(1 hunks)code/renderers/svelte/package.json(1 hunks)code/renderers/vue3/package.json(1 hunks)code/renderers/web-components/package.json(1 hunks)scripts/package.json(1 hunks)scripts/run-registry.ts(8 hunks)scripts/utils/yarn.ts(1 hunks)scripts/verdaccio.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/lib/codemod/package.jsoncode/core/package.jsoncode/addons/onboarding/package.jsoncode/lib/react-dom-shim/package.jsonscripts/package.jsoncode/frameworks/react-webpack5/package.jsoncode/frameworks/react-native-web-vite/package.jsonscripts/utils/yarn.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/frameworks/react-vite/package.jsoncode/lib/core-webpack/package.jsoncode/frameworks/vue3-vite/package.jsoncode/frameworks/html-vite/package.jsoncode/frameworks/ember/package.jsoncode/addons/a11y/package.jsoncode/builders/builder-vite/package.jsoncode/addons/links/package.jsoncode/addons/docs/package.jsoncode/frameworks/preact-vite/package.jsoncode/builders/builder-webpack5/package.jsoncode/core/src/common/js-package-manager/Yarn2Proxy.tscode/lib/cli-sb/package.jsoncode/renderers/web-components/package.jsoncode/lib/create-storybook/package.jsoncode/renderers/svelte/package.jsoncode/frameworks/nextjs-vite/package.jsoncode/renderers/react/package.jsoncode/renderers/html/package.jsoncode/frameworks/web-components-vite/package.jsoncode/core/src/common/js-package-manager/Yarn1Proxy.tscode/addons/vitest/package.jsoncode/core/src/common/js-package-manager/NPMProxy.tscode/lib/csf-plugin/package.jsoncode/core/src/bin/dispatcher.tscode/addons/pseudo-states/package.jsoncode/core/src/common/js-package-manager/JsPackageManager.tscode/renderers/preact/package.jsoncode/presets/server-webpack/package.jsoncode/lib/cli-storybook/package.jsoncode/presets/create-react-app/package.jsoncode/presets/react-webpack/package.jsoncode/renderers/vue3/package.jsoncode/frameworks/nextjs/package.jsoncode/frameworks/sveltekit/package.jsoncode/renderers/server/package.jsoncode/lib/eslint-plugin/package.jsoncode/addons/themes/package.jsoncode/frameworks/angular/package.jsoncode/frameworks/server-webpack5/package.jsoncode/frameworks/svelte-vite/package.jsonscripts/run-registry.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/lib/codemod/package.jsoncode/core/package.jsoncode/addons/onboarding/package.jsoncode/lib/react-dom-shim/package.jsoncode/frameworks/react-webpack5/package.jsoncode/frameworks/react-native-web-vite/package.jsoncode/core/src/common/js-package-manager/PNPMProxy.tscode/frameworks/react-vite/package.jsoncode/lib/core-webpack/package.jsoncode/frameworks/vue3-vite/package.jsoncode/frameworks/html-vite/package.jsoncode/frameworks/ember/package.jsoncode/addons/a11y/package.jsoncode/builders/builder-vite/package.jsoncode/addons/links/package.jsoncode/addons/docs/package.jsoncode/frameworks/preact-vite/package.jsoncode/builders/builder-webpack5/package.jsoncode/core/src/common/js-package-manager/Yarn2Proxy.tscode/lib/cli-sb/package.jsoncode/renderers/web-components/package.jsoncode/lib/create-storybook/package.jsoncode/renderers/svelte/package.jsoncode/frameworks/nextjs-vite/package.jsoncode/renderers/react/package.jsoncode/renderers/html/package.jsoncode/frameworks/web-components-vite/package.jsoncode/core/src/common/js-package-manager/Yarn1Proxy.tscode/addons/vitest/package.jsoncode/core/src/common/js-package-manager/NPMProxy.tscode/lib/csf-plugin/package.jsoncode/core/src/bin/dispatcher.tscode/addons/pseudo-states/package.jsoncode/core/src/common/js-package-manager/JsPackageManager.tscode/renderers/preact/package.jsoncode/presets/server-webpack/package.jsoncode/lib/cli-storybook/package.jsoncode/presets/create-react-app/package.jsoncode/presets/react-webpack/package.jsoncode/renderers/vue3/package.jsoncode/frameworks/nextjs/package.jsoncode/frameworks/sveltekit/package.jsoncode/renderers/server/package.jsoncode/lib/eslint-plugin/package.jsoncode/addons/themes/package.jsoncode/frameworks/angular/package.jsoncode/frameworks/server-webpack5/package.jsoncode/frameworks/svelte-vite/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/utils/yarn.tscode/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/common/js-package-manager/Yarn1Proxy.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/bin/dispatcher.tscode/core/src/common/js-package-manager/JsPackageManager.tsscripts/run-registry.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/common/js-package-manager/Yarn1Proxy.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/bin/dispatcher.tscode/core/src/common/js-package-manager/JsPackageManager.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/core/src/common/js-package-manager/PNPMProxy.tscode/core/src/common/js-package-manager/Yarn2Proxy.tscode/core/src/common/js-package-manager/Yarn1Proxy.tscode/core/src/common/js-package-manager/NPMProxy.tscode/core/src/bin/dispatcher.tscode/core/src/common/js-package-manager/JsPackageManager.ts
🧠 Learnings (8)
📓 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.
📚 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:
code/lib/codemod/package.jsoncode/core/package.jsoncode/addons/onboarding/package.jsoncode/lib/react-dom-shim/package.jsonscripts/package.jsoncode/frameworks/react-webpack5/package.jsoncode/frameworks/react-native-web-vite/package.jsonscripts/utils/yarn.tscode/frameworks/react-vite/package.jsoncode/lib/core-webpack/package.jsoncode/frameworks/vue3-vite/package.jsoncode/frameworks/html-vite/package.jsoncode/frameworks/ember/package.jsoncode/addons/a11y/package.jsoncode/builders/builder-vite/package.jsoncode/addons/links/package.jsoncode/addons/docs/package.jsoncode/frameworks/preact-vite/package.jsoncode/builders/builder-webpack5/package.jsoncode/lib/cli-sb/package.jsoncode/renderers/web-components/package.jsoncode/lib/create-storybook/package.jsoncode/renderers/svelte/package.jsoncode/frameworks/nextjs-vite/package.jsoncode/renderers/react/package.jsoncode/renderers/html/package.jsoncode/frameworks/web-components-vite/package.jsoncode/lib/csf-plugin/package.jsoncode/core/src/bin/dispatcher.tscode/renderers/preact/package.jsoncode/presets/server-webpack/package.jsoncode/lib/cli-storybook/package.jsoncode/presets/create-react-app/package.jsoncode/presets/react-webpack/package.jsoncode/renderers/vue3/package.jsoncode/frameworks/nextjs/package.jsoncode/frameworks/sveltekit/package.jsoncode/renderers/server/package.jsoncode/lib/eslint-plugin/package.jsoncode/addons/themes/package.jsoncode/frameworks/angular/package.jsoncode/frameworks/server-webpack5/package.jsoncode/frameworks/svelte-vite/package.json
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
scripts/package.jsonscripts/utils/yarn.tscode/core/src/common/js-package-manager/Yarn1Proxy.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:
code/frameworks/react-webpack5/package.jsoncode/frameworks/react-vite/package.jsoncode/addons/a11y/package.jsoncode/core/src/bin/dispatcher.tscode/presets/react-webpack/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:
scripts/utils/yarn.ts
📚 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/renderers/react/package.jsoncode/core/src/bin/dispatcher.tscode/lib/cli-storybook/package.jsoncode/presets/react-webpack/package.jsoncode/frameworks/nextjs/package.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:
code/core/src/bin/dispatcher.ts
📚 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:
code/lib/eslint-plugin/package.json
🧬 Code graph analysis (1)
code/core/src/common/js-package-manager/JsPackageManager.ts (1)
code/core/src/manager-api/tests/versions.test.js (1)
latest(51-54)
🔇 Additional comments (53)
code/core/src/common/js-package-manager/PNPMProxy.ts (1)
212-242: Explicitcwdforpnpm infomatches other PNPMProxy callsAdding
cwd: this.cwdto theexecuteCommandinvocation keeps version resolution scoped to the proxy’s working directory, consistent withrunInstalland other methods. This looks correct and should avoid surprises in nested workspaces.code/renderers/react/package.json (1)
12-16: Repository URL metadata update looks goodSwitching
repository.urlto thegit+https://form is valid npm metadata and aligns with the rest of the repo. No issues here.code/core/src/common/js-package-manager/Yarn1Proxy.ts (1)
166-193:yarn infonow respects proxy working directoryAdding
cwd: this.cwdto theexecuteCommandforyarn infoaligns version resolution with other Yarn1Proxy commands and should improve correctness in nested or non-root workspaces. Looks good.code/addons/onboarding/package.json (1)
22-26: Consistentrepository.urlformatThe switch to
git+https://github.com/storybookjs/storybook.gitmatches the standardized repository URL used elsewhere. No concerns.code/frameworks/nextjs/package.json (1)
17-21: Next.js framework package metadata update is safeUpdating
repository.urlto thegit+https://form is purely informational and consistent with other Storybook packages. No issues.code/lib/cli-storybook/package.json (1)
17-21: CLI package repository URL change is correctThe
repository.urlchange togit+https://github.com/storybookjs/storybook.gitaligns with the rest of the monorepo and is valid for npm metadata.code/lib/csf-plugin/package.json (1)
12-16: CSF plugin repository URL normalizationThe
repository.urlupdate to thegit+https://scheme is consistent with other packages and has no runtime impact. Looks good.code/frameworks/preact-vite/package.json (1)
17-21: Preact Vite framework metadata matches repo conventionUsing
git+https://github.com/storybookjs/storybook.gitforrepository.urlis correct and consistent with other Storybook packages.code/presets/react-webpack/package.json (1)
14-14: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/lib/create-storybook/package.json (1)
17-17: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/renderers/vue3/package.json (1)
18-18: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/lib/cli-sb/package.json (1)
19-19: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/lib/core-webpack/package.json (1)
14-14: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/core/src/common/js-package-manager/Yarn2Proxy.ts (1)
265-269: LGTM! Improved command execution consistency.Adding the explicit working directory context ensures the
yarn npm infocommand executes in the correct location, consistent with other methods in this class (e.g.,runInternalCommand,runInstall,findInstallations) and the broader proxy pattern updates.code/frameworks/react-vite/package.json (1)
19-19: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/addons/themes/package.json (1)
27-27: LGTM! Repository URL standardization.The update to the git+https URL format is a valid metadata change with no functional impact.
code/frameworks/vue3-vite/package.json (1)
19-19: LGTM! Repository URL format updated.The change from
https://togit+https://is a valid npm repository URL format that makes the Git protocol more explicit. This metadata update has no functional impact.code/addons/a11y/package.json (1)
28-28: LGTM! Repository URL format updated.Consistent metadata update to use the
git+https://prefix for clarity.code/frameworks/react-native-web-vite/package.json (1)
21-21: LGTM! Repository URL format updated.code/addons/vitest/package.json (1)
25-25: LGTM! Repository URL format updated.code/addons/links/package.json (1)
20-20: LGTM! Repository URL format updated.code/core/src/common/js-package-manager/NPMProxy.ts (1)
219-219: LGTM! Explicit working directory improves correctness.Adding
cwd: this.cwdensures thenpm infocommand executes in the correct working directory context, which is important for proper package resolution in monorepo scenarios. This aligns the NPMProxy behavior with other package manager proxy implementations.code/lib/react-dom-shim/package.json (1)
14-14: LGTM! Repository URL format updated.code/frameworks/web-components-vite/package.json (1)
21-21: LGTM! Repository URL format updated.code/frameworks/svelte-vite/package.json (1)
17-20: Repository URL scheme change looks goodSwitching
repository.urltogit+https://github.com/storybookjs/storybook.gitis consistent with the rest of the repo and has no behavioral impact for consumers.code/renderers/web-components/package.json (1)
18-21: Consistent repository URL updateThe
repository.urlswitch togit+https://github.com/storybookjs/storybook.gitmatches the repo-wide convention and is fine from a tooling/metadata perspective.code/addons/pseudo-states/package.json (1)
24-28: Addon metadata alignment is correctUpdating
repository.urlto thegit+httpsvariant keeps this addon's metadata consistent with other Storybook packages.code/addons/docs/package.json (1)
29-33: Repository URL normalization for docs addonThe change to
git+https://github.com/storybookjs/storybook.gitis consistent with other packages and should work fine with npm/Yarn metadata consumers.code/frameworks/ember/package.json (1)
16-20: Ember framework package metadata update is fineThe
repository.urlchange togit+https://github.com/storybookjs/storybook.gitmatches the rest of the monorepo and is a safe, metadata‑only tweak.code/lib/codemod/package.json (1)
12-16: Codemod package repository URL change looks correctSwitching
repository.urlto thegit+httpsform brings this library in line with other Storybook packages without affecting how it runs.code/renderers/server/package.json (1)
15-19: Server renderer repository URL update is consistentThe move to
git+https://github.com/storybookjs/storybook.gitforrepository.urlis consistent with other renderer/framework packages and is safe.code/frameworks/html-vite/package.json (1)
19-19: Metadata update approved.The repository URL format change from
https://togit+https://is valid and aligns with standard npm package registry conventions for Git-based repository references. This format is commonly used by package managers to properly handle Git cloning and authentication during publish workflows.code/renderers/preact/package.json (1)
18-18: Metadata update approved.Repository URL scheme update is consistent across the codebase and follows standard npm conventions for Git URL references.
code/frameworks/sveltekit/package.json (1)
21-21: Metadata update approved.Repository URL scheme update is consistent and follows standard conventions. SvelteKit peer dependencies and exports remain unchanged.
code/renderers/svelte/package.json (1)
17-17: Metadata update approved.Repository URL scheme change is consistent with other files in the PR. Svelte renderer tooling and peer dependencies remain stable.
code/core/package.json (1)
28-28: Metadata update approved.Repository URL scheme update for core package is consistent with the PR-wide changes. All internal exports, dependencies, and peer dependencies remain unchanged.
code/builders/builder-vite/package.json (1)
20-20: Metadata update approved.Repository URL scheme update for Vite builder is consistent. Builder exports and Vite peer dependency ranges remain unchanged.
code/builders/builder-webpack5/package.json (1)
20-20: Metadata update approved.Repository URL scheme update for Webpack5 builder is consistent. Builder exports, loaders, webpack dependencies, and configurations remain unchanged.
code/renderers/html/package.json (1)
18-18: Metadata update approved.Repository URL scheme update for HTML renderer is consistent with other package.json changes. Renderer exports and dependencies remain stable.
code/presets/create-react-app/package.json (1)
14-14: Consistent with repository URL standardization across the PR.The change from
https://togit+https://is a valid npm protocol prefix that explicitly instructs package managers to clone via git+https. This aligns with the broader metadata standardization across ~40 packages in this PR.code/lib/eslint-plugin/package.json (1)
17-17: Repository URL improved with git+https protocol and .git suffix.This change both upgrades the protocol scheme and adds the missing
.gitsuffix, which aligns with npm package.json best practices. The original URL lacked the.gitsuffix; this correction standardizes the format across the project.code/frameworks/angular/package.json (1)
18-18: Repository URL standardized to git+https protocol.Aligns with the PR-wide standardization to use explicit
git+https://protocol prefix for package registry metadata.code/frameworks/server-webpack5/package.json (1)
17-17: Repository URL standardized to git+https protocol.Part of the PR-wide standardization to use explicit
git+https://protocol prefix.code/frameworks/react-webpack5/package.json (1)
19-19: Repository URL standardized to git+https protocol.Part of the PR-wide standardization to use explicit
git+https://protocol prefix.code/presets/server-webpack/package.json (1)
14-14: Repository URL standardized to git+https protocol.Part of the PR-wide standardization to use explicit
git+https://protocol prefix.code/frameworks/nextjs-vite/package.json (1)
20-21: Directory field points to wrong package directory.The
directoryfield on line 21 must be corrected to"code/frameworks/nextjs-vite". Currently it points to"code/frameworks/nextjs", but nextjs-vite is a separate package with its own source directory. Both@storybook/nextjsand@storybook/nextjs-vitemaintain distinct codebases, each with their ownsrcdirectory.Change line 21 from:
"directory": "code/frameworks/nextjs"to:
"directory": "code/frameworks/nextjs-vite"⛔ Skipped due to learnings
Learnt from: CR Repo: storybookjs/storybook PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-28T14:50:24.889Z Learning: Follow existing patterns and conventions in the Storybook codebasescripts/package.json (1)
178-178: Verdaccio integration is properly implemented and version 6.2.3 is stable.The
run-registry.tsfile correctly imports and uses verdaccio's core functions (parseConfigFile,runServer), manages the server lifecycle (initialization, listening on port 6002, graceful shutdown), handles caching, and integrates with a proxy on port 6001. The removal of@verdaccio/typesandverdaccio-auth-memoryfrom optionalDependencies has no remaining code references. Version 6.2.3 has no known security vulnerabilities and is appropriate for this use case. The promotion from optional to required dependency is justified by the extensive integration.scripts/verdaccio.yaml (2)
5-6: Auth removal is appropriate for local e2e testing.The empty auth block with the explanatory comment clearly indicates this is for testing purposes only. This simplification aligns well with the new publishing workflow.
24-26: Offline publishing support is well-suited for isolated e2e tests.The
allow_offline: truesetting ensures Verdaccio doesn't require npm registry connectivity, which is appropriate for the self-contained publishing workflow inrun-registry.ts.scripts/run-registry.ts (3)
19-23: Constants are well-defined for the publishing workflow.Using
Date.now()forRUN_IDensures unique publish versions across runs, preventing conflicts. The port constants centralize configuration.
71-85: Proxy routing logic is well-designed.The conditional routing correctly sends publish requests (PUT) and workspace package requests to Verdaccio while allowing other packages to resolve from the public npm registry. Using
decodeURIComponentproperly handles scoped package URLs.
182-207: Workspace dependency resolution logic is correct.The function properly handles all
workspace:protocol variants (^,~, and exact). The implementation correctly preserves version range prefixes while substituting the actual version.code/core/src/common/js-package-manager/JsPackageManager.ts (1)
383-385: LGTM - Safe local version detection.The optional chaining (
?.) correctly handles null/undefinedlatestInRange, and the early return for local versions is logically placed before the stable release comparison.
|
|
||
| return latest.includes('-local.') ? 'local' : `^${versionToUse}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the JsPackageManager.ts file
fd "JsPackageManager.ts" -t fRepository: storybookjs/storybook
Length of output: 124
🏁 Script executed:
# Get the file to understand context around lines 410-449
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '400,460p'Repository: storybookjs/storybook
Length of output: 2645
🏁 Script executed:
# Search for latestVersion method to understand its return type
rg -n "latestVersion" code/core/src/common/js-package-manager/JsPackageManager.ts -B 2 -A 5Repository: storybookjs/storybook
Length of output: 3511
🏁 Script executed:
# Search for 'local' dist-tag usage patterns in the codebase
rg -n "dist-tag.*local|--tag.*local|'local'" --type=ts --type=js code/ -B 2 -A 2Repository: storybookjs/storybook
Length of output: 5887
🏁 Script executed:
# Search for any Verdaccio configuration or publishing patterns
rg -n "verdaccio|dist-tag" --type=ts --type=js --type=json code/ -B 1 -A 1Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check for 'local' string usage in version context
rg -n "\-local\." --type=ts --type=js code/core/src/common/js-package-manager/ -B 3 -A 3Repository: storybookjs/storybook
Length of output: 1257
🏁 Script executed:
# Check line 383 context more carefully
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '370,390p'Repository: storybookjs/storybook
Length of output: 978
🏁 Script executed:
# Check if latestInRange could be null at line 383
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '360,395p'Repository: storybookjs/storybook
Length of output: 1555
🏁 Script executed:
# Check TypeScript configuration for strict mode
cat tsconfig.json | head -50Repository: storybookjs/storybook
Length of output: 110
🏁 Script executed:
# Check the exact control flow - does TypeScript narrow after throw?
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '427,450p'Repository: storybookjs/storybook
Length of output: 876
🏁 Script executed:
# Verify TypeScript version and strict mode config
fd "tsconfig" -t f | head -5Repository: storybookjs/storybook
Length of output: 235
🏁 Script executed:
# Check main tsconfig location
find . -maxdepth 2 -name "tsconfig*.json" -type fRepository: storybookjs/storybook
Length of output: 109
🏁 Script executed:
# Check TypeScript strict mode configuration
cat code/tsconfig.json | grep -A 20 "compilerOptions"Repository: storybookjs/storybook
Length of output: 693
Add JSDoc documentation for the local version behavior.
The method properly guards against null values at lines 430–432 with an explicit throw that narrows the latest variable to a non-null string type before line 448, consistent with how latest is already used at line 444 without defensive checks.
However, the special handling of local versions (checking for -local. and returning the string 'local' as a dist-tag) is undocumented. This behavior is used elsewhere in the codebase (line 384: ${packageName}@local) but the getVersion method's JSDoc doesn't explain it.
Add to the JSDoc comment for this method (around line 410):
/**
* Return the latest version of the input package available on npmjs registry. If constraint are
* provided it return the latest version matching the constraints.
*
* For `@storybook/*` packages the latest version is retrieved from `cli/src/versions.json` file
* directly
+ *
+ * When the latest version contains a local pre-release tag (e.g., '8.0.0-local.1'), this method
+ * returns the string 'local' to indicate the package should be installed using the 'local' dist-tag.
*
* @param packageName The name of the package
* @param constraint A valid semver constraint, example: '1.x || >=2.5.0 || 5.0.0 - 7.2.3'
+ * @returns A semver version string prefixed with '^', or the string 'local' for local development versions
*/Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/common/js-package-manager/JsPackageManager.ts around line 410,
update the method JSDoc for getVersion to document the special "local" dist-tag
behavior: note that the method already narrows `latest` to a non-null string
earlier, and that if the resolved latest string contains "-local." the method
returns the literal 'local' (used elsewhere as `${packageName}@local`),
otherwise it returns a caret-prefixed version `^${versionToUse}`; add a short
example and mention that callers should expect the 'local' dist-tag when
testing/local builds include "-local." in the version.
|
View your CI Pipeline Execution ↗ for commit 2e32364
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/generate-sandboxes.yml (1)
103-110: Add Verdaccio port wait and align with generate-next workflow.The
generate-mainjob has two inconsistencies withgenerate-next:
- It splits the registry setup into separate
--publishand--opencommands, whilegenerate-nextuses a combined command with--publish --open --no-local &(line 51).- It only waits for the proxy registry (port 6001) but not for Verdaccio (port 6002), which could cause race conditions if downstream steps access Verdaccio before it's ready.
Consider aligning with the
generate-nextworkflow by applying this diff:- name: Publish to local registry - run: yarn local-registry --publish - - - name: Run local registry - run: yarn local-registry --open & + run: yarn local-registry --publish --open & - name: Wait for registry run: yarn wait-on tcp:127.0.0.1:6001 + - name: Wait for Verdaccio + run: yarn wait-on tcp:127.0.0.1:6002 + - name: Generate id: generate run: yarn generate-sandboxes --local-registryNote: The
generate-nextjob uses--no-localflag while this suggestion omits it. Please verify whethergenerate-mainshould also use--no-localbased on your versioning requirements.
♻️ Duplicate comments (4)
scripts/run-registry.ts (4)
166-166: UseVERDACCIO_PORTconstant instead of hardcoded port.Line 166 hardcodes port
6002while line 174 correctly uses theVERDACCIO_PORTconstant. This is inconsistent and makes the port configuration less maintainable.Apply this diff:
- await writeFile(join(tmpDir, '.npmrc'), `//localhost:6002/:_authToken=fake`); + await writeFile(join(tmpDir, '.npmrc'), `//localhost:${VERDACCIO_PORT}/:_authToken=fake`);
78-82: Fix optional chaining to prevent decoding error.The optional chaining on line 80 doesn't prevent
decodeURIComponentfrom throwing ifreq.urlisnullorundefined, since the function is called before the optional chaining operator is evaluated.Apply this diff to check
req.urlbefore decoding:if ( req.url === '/' || - packages.some((it) => decodeURIComponent(req.url)?.startsWith('/' + it.name)) || + (req.url && packages.some((it) => decodeURIComponent(req.url).startsWith('/' + it.name))) || req.method === 'PUT' ) {
247-259: Remove redundant call and avoid variable shadowing.This code shadows the outer
packagesvariable (line 235) and redundantly callsgetCodeWorkspaces(false)which was already fetched on line 235.Apply this diff to reuse the existing
packagesarray and rename the enriched result:if (opts.publish) { - const packages = await Promise.all( - (await getCodeWorkspaces(false)).map(async (ws) => { + const packagesToPublish = await Promise.all( + packages.map(async (ws) => { const path = join(CODE_DIRECTORY, ws.location); const pkg = JSON.parse(await readFile(join(path, 'package.json'), 'utf8')); const version = pkg.version; return { ...ws, path, version, publishVersion: opts.local ? `${version}-${PUBLISH_TAG}.${RUN_ID}` : version, }; }) ); const limit = pLimit(maxConcurrentTasks); - await Promise.all(packages.map((p) => limit(() => publishPackage(p, packages)))); + await Promise.all(packagesToPublish.map((p) => limit(() => publishPackage(p, packagesToPublish)))); }
156-157: Add null check to prevent runtime error.The
findoperation can returnundefinedif no matching package is found, causing a runtime error when accessinginfo.publishVersionon line 157.Apply this diff to add a null check:
const publishPackage = async (ws: Package, workspaces: Package[]) => { const info = workspaces.find((it) => it.name === ws.name); + if (!info) { + throw new Error(`Package ${ws.name} not found in workspaces`); + } const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-'));
📜 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 (3)
.github/workflows/generate-sandboxes.yml(5 hunks)scripts/run-registry.ts(8 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
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/run-registry.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/run-registry.ts
🧠 Learnings (3)
📓 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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
.github/workflows/generate-sandboxes.yml
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Node.js version 22.21.1 as specified in .nvmrc
Applied to files:
.github/workflows/generate-sandboxes.yml
🧬 Code graph analysis (1)
scripts/run-registry.ts (3)
scripts/utils/port.ts (1)
killPort(7-20)code/core/src/manager-api/version.ts (1)
version(1-1)scripts/utils/constants.ts (1)
CODE_DIRECTORY(9-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). (1)
- GitHub Check: normal
🔇 Additional comments (1)
scripts/run-registry.ts (1)
169-180: ThenodeOptions.cwdproperty is correct for tinyexec v0.3.2. The tinyexec API explicitly supports passing Node.jschild_processspawn options (includingcwdandstdio) under thenodeOptionsfield, which are forwarded directly to Node's spawn. Both{ cwd: ... }and{ nodeOptions: { cwd: ... } }are valid approaches.Likely an incorrect or invalid review comment.
| - name: Wait for registry | ||
| run: yarn wait-on tcp:127.0.0.1:6001 | ||
|
|
||
| - name: Wait for registry | ||
| run: yarn wait-on tcp:127.0.0.1:6002 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use distinct names for registry wait steps.
Both wait steps are named "Wait for registry", which makes them indistinguishable in the GitHub Actions UI. Since they wait on different ports (6001 for the proxy and 6002 for Verdaccio), they should have distinct names.
Apply this diff:
- - name: Wait for registry
+ - name: Wait for proxy registry
run: yarn wait-on tcp:127.0.0.1:6001
- - name: Wait for registry
+ - name: Wait for Verdaccio
run: yarn wait-on tcp:127.0.0.1:6002📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Wait for registry | |
| run: yarn wait-on tcp:127.0.0.1:6001 | |
| - name: Wait for registry | |
| run: yarn wait-on tcp:127.0.0.1:6002 | |
| - name: Wait for proxy registry | |
| run: yarn wait-on tcp:127.0.0.1:6001 | |
| - name: Wait for Verdaccio | |
| run: yarn wait-on tcp:127.0.0.1:6002 |
🤖 Prompt for AI Agents
In .github/workflows/generate-sandboxes.yml around lines 53 to 57, both steps
are labeled "Wait for registry" which makes them indistinguishable in the
Actions UI; change the step names to unique, descriptive labels (e.g., "Wait for
proxy registry (6001)" for the tcp:127.0.0.1:6001 step and "Wait for Verdaccio
registry (6002)" for the tcp:127.0.0.1:6002 step) so each step is clearly
identifiable while keeping the run commands unchanged.
# Conflicts: # scripts/package.json # yarn.lock
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-registry.ts (2)
147-153: Avoid re-finding the current package and fix potentialundefinedaccess.
publishPackagere-finds the same workspace and then unconditionally readsinfo.publishVersion:const info = workspaces.find((it) => it.name === ws.name); pkg.version = info.publishVersion;Even though
wscomes from the sameworkspacesarray, this:
- Introduces a possible runtime error if
infois everundefined(e.g., if the inputs drift), and- Will trigger a TypeScript strict-mode error (
Object is possibly 'undefined').You already have
publishVersiononws, so the lookup is unnecessary.Suggested simplification:
-const publishPackage = async (ws: Package, workspaces: Package[]) => { - const info = workspaces.find((it) => it.name === ws.name); +const publishPackage = async (ws: Package, workspaces: Package[]) => { const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-')); @@ - const pkg = JSON.parse(await readFile(join(tmpDir, 'package.json'), 'utf8')); - pkg.version = info.publishVersion; - resolveWorkspaceDeps(pkg, workspaces); + const pkg = JSON.parse(await readFile(join(tmpDir, 'package.json'), 'utf8')); + pkg.version = ws.publishVersion; + resolveWorkspaceDeps(pkg, workspaces);This removes the redundant search, fixes the
undefinedrisk, and is friendlier to TS strict mode.Also applies to: 155-165, 168-183
55-56: HardendecodeURIComponent(req.url)usage in the proxy.The proxy condition still directly calls
decodeURIComponent(req.url):packages.some((it) => decodeURIComponent(req.url)?.startsWith('/' + it.name))Issues:
req.urlcan beundefinedor contain malformed escape sequences, in which casedecodeURIComponentwill throw and can take down the request handler.- The optional chaining only applies after
decodeURIComponentand doesn’t prevent that exception.A safer pattern is to guard
req.urland the decode in a try/catch, or skip the decode ifreq.urlis falsy. For example:- if ( - req.url === '/' || - packages.some((it) => decodeURIComponent(req.url)?.startsWith('/' + it.name)) || - req.method === 'PUT' - ) { + let shouldProxyToVerdaccio = req.method === 'PUT' || req.url === '/'; + if (!shouldProxyToVerdaccio && req.url) { + try { + const decoded = decodeURIComponent(req.url); + shouldProxyToVerdaccio = packages.some((it) => + decoded.startsWith('/' + it.name) + ); + } catch { + // On malformed URLs, fall back to the global registry. + } + } + + if (shouldProxyToVerdaccio) { res.writeHead(302, { Location: `http://localhost:${VERDACCIO_PORT}` + req.url });This keeps the heuristic but avoids crashes on bad URLs.
Also applies to: 76-84
🧹 Nitpick comments (2)
scripts/run-registry.ts (1)
186-212: Workspace-aware version resolution and concurrent publishing look solid.
resolveWorkspaceDepscorrectly rewritesworkspace:specs to concretepublishVersions (pinning or prefixing with^/~where appropriate) before publishing.getPackagesenriches workspace entries with absolutepath, currentversion, and apublishVersionthat optionally appends a-local.<RUN_ID>suffix when--localis enabled.- Using
p-limit(maxConcurrentTasks)to gatepublishPackagecalls provides a good balance between throughput and avoiding overloading Verdaccio.These pieces fit together cleanly for the new per-workspace publish flow.
If you ever need to tune performance further, you could consider making the concurrency limit configurable via a CLI flag, but it’s not necessary for this PR.
Also applies to: 214-227, 248-264
.github/workflows/generate-sandboxes.yml (1)
67-75:--no-localis redundant but valid; it does not cause an error.Commander.js automatically generates the
--no-localnegation for boolean options, so passing--no-localis valid and works correctly. However, since--localdefaults tofalse(line 29-31 ofscripts/run-registry.ts), using--no-localis unnecessary and adds no value.Suggested simplification:
- - name: Publish to local registry - run: yarn local-registry --publish --open --no-local & + - name: Publish to local registry + run: yarn local-registry --publish --open &
📜 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 (46)
.github/workflows/generate-sandboxes.yml(5 hunks)code/addons/a11y/package.json(1 hunks)code/addons/docs/package.json(1 hunks)code/addons/links/package.json(1 hunks)code/addons/onboarding/package.json(1 hunks)code/addons/pseudo-states/package.json(1 hunks)code/addons/themes/package.json(1 hunks)code/addons/vitest/package.json(1 hunks)code/builders/builder-vite/package.json(1 hunks)code/builders/builder-webpack5/package.json(1 hunks)code/core/package.json(1 hunks)code/frameworks/angular/package.json(1 hunks)code/frameworks/ember/package.json(1 hunks)code/frameworks/html-vite/package.json(1 hunks)code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs/package.json(1 hunks)code/frameworks/preact-vite/package.json(1 hunks)code/frameworks/react-native-web-vite/package.json(1 hunks)code/frameworks/react-vite/package.json(1 hunks)code/frameworks/react-webpack5/package.json(1 hunks)code/frameworks/server-webpack5/package.json(1 hunks)code/frameworks/svelte-vite/package.json(1 hunks)code/frameworks/sveltekit/package.json(1 hunks)code/frameworks/vue3-vite/package.json(1 hunks)code/frameworks/web-components-vite/package.json(1 hunks)code/lib/cli-sb/package.json(1 hunks)code/lib/cli-storybook/package.json(1 hunks)code/lib/codemod/package.json(1 hunks)code/lib/core-webpack/package.json(1 hunks)code/lib/create-storybook/package.json(1 hunks)code/lib/csf-plugin/package.json(1 hunks)code/lib/eslint-plugin/package.json(1 hunks)code/lib/react-dom-shim/package.json(1 hunks)code/presets/create-react-app/package.json(1 hunks)code/presets/react-webpack/package.json(1 hunks)code/presets/server-webpack/package.json(1 hunks)code/renderers/html/package.json(1 hunks)code/renderers/preact/package.json(1 hunks)code/renderers/react/package.json(1 hunks)code/renderers/server/package.json(1 hunks)code/renderers/svelte/package.json(1 hunks)code/renderers/vue3/package.json(1 hunks)code/renderers/web-components/package.json(1 hunks)scripts/package.json(1 hunks)scripts/run-registry.ts(8 hunks)scripts/utils/yarn.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (34)
- code/renderers/preact/package.json
- code/addons/links/package.json
- code/frameworks/sveltekit/package.json
- code/frameworks/vue3-vite/package.json
- code/renderers/svelte/package.json
- code/frameworks/svelte-vite/package.json
- code/builders/builder-webpack5/package.json
- code/frameworks/react-native-web-vite/package.json
- code/frameworks/react-vite/package.json
- code/frameworks/nextjs-vite/package.json
- code/lib/eslint-plugin/package.json
- code/lib/csf-plugin/package.json
- code/frameworks/server-webpack5/package.json
- scripts/utils/yarn.ts
- code/renderers/html/package.json
- code/lib/core-webpack/package.json
- code/frameworks/html-vite/package.json
- code/frameworks/ember/package.json
- code/renderers/web-components/package.json
- code/lib/react-dom-shim/package.json
- code/frameworks/web-components-vite/package.json
- code/presets/create-react-app/package.json
- code/frameworks/preact-vite/package.json
- code/addons/onboarding/package.json
- code/renderers/react/package.json
- code/addons/docs/package.json
- code/frameworks/nextjs/package.json
- code/addons/a11y/package.json
- code/lib/codemod/package.json
- code/builders/builder-vite/package.json
- code/addons/themes/package.json
- code/core/package.json
- code/addons/pseudo-states/package.json
- code/lib/cli-storybook/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/presets/react-webpack/package.jsoncode/frameworks/angular/package.jsonscripts/package.jsoncode/renderers/server/package.jsonscripts/run-registry.tscode/lib/cli-sb/package.jsoncode/lib/create-storybook/package.jsoncode/frameworks/react-webpack5/package.jsoncode/addons/vitest/package.jsoncode/renderers/vue3/package.jsoncode/presets/server-webpack/package.json
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/presets/react-webpack/package.jsoncode/frameworks/angular/package.jsoncode/renderers/server/package.jsoncode/lib/cli-sb/package.jsoncode/lib/create-storybook/package.jsoncode/frameworks/react-webpack5/package.jsoncode/addons/vitest/package.jsoncode/renderers/vue3/package.jsoncode/presets/server-webpack/package.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/run-registry.ts
🧠 Learnings (7)
📓 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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
📚 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/presets/react-webpack/package.jsoncode/frameworks/react-webpack5/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/presets/react-webpack/package.json
📚 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:
code/presets/react-webpack/package.jsoncode/frameworks/angular/package.jsonscripts/package.jsoncode/renderers/server/package.jsoncode/lib/cli-sb/package.jsoncode/lib/create-storybook/package.jsoncode/frameworks/react-webpack5/package.jsoncode/renderers/vue3/package.jsoncode/presets/server-webpack/package.json
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
scripts/package.json.github/workflows/generate-sandboxes.yml
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/run-registry.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Node.js version 22.21.1 as specified in .nvmrc
Applied to files:
.github/workflows/generate-sandboxes.yml
🧬 Code graph analysis (1)
scripts/run-registry.ts (2)
scripts/utils/port.ts (1)
killPort(7-20)scripts/utils/constants.ts (1)
CODE_DIRECTORY(9-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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (11)
code/presets/react-webpack/package.json (1)
12-16: Repository URL standardization looks good.The
git+https://prefix is a valid npm URL scheme for explicit git protocol specification and aligns with npm registry standards. This change is consistent across all updated package.json files in this PR.code/lib/cli-sb/package.json (1)
17-21: Repository URL standardization is consistent.code/renderers/vue3/package.json (1)
16-20: Consistent repository URL standardization.code/addons/vitest/package.json (1)
23-27: Repository URL standardization is consistent.code/presets/server-webpack/package.json (1)
12-16: Repository URL standardization is consistent.code/renderers/server/package.json (1)
15-19: Repository URL standardization is consistent.code/lib/create-storybook/package.json (1)
15-19: Repository URL standardization is consistent.scripts/package.json (1)
62-184: No action required — verdaccio 6.2.3 is valid and secure.Verdaccio 6.2.3 was released on November 27, 2025, and contains dependency updates plus an IPv6 Docker fix. The version has no known security vulnerabilities (Snyk confirmed), uses a standard caret range, and is appropriate for the local publish/registry tooling context in this private devDependency package.
code/frameworks/angular/package.json (1)
16-19: Repository URL scheme update looks good.Using the
git+https://URL is valid and consistent with NPM metadata; no functional impact.code/frameworks/react-webpack5/package.json (1)
17-20: Consistent repository URL normalization.Switching to
git+https://here is consistent with other packages and is safe..github/workflows/generate-sandboxes.yml (1)
3-6: Cosmetic YAML and setup-node adjustments are fine.
- Using single quotes for the cron expression and env vars is a no-op behaviorally.
node-version-file: '.nvmrc'in both jobs correctly centralizes the Node version, aligning the workflow with the repo’s.nvmrcwithout changing runtime behavior.Also applies to: 16-18, 51-54, 120-123
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-registry.ts (2)
155-162: Add null check forinfoto prevent potential runtime error.The
workspaces.find()call on line 156 can returnundefinedif the package is not found, causing a runtime error when accessinginfo.publishVersionon line 162. While this shouldn't happen logically (sincewscomes from the same array), TypeScript's type system doesn't guarantee this, and defensive coding is warranted. This issue was flagged in previous reviews.Apply this diff to add a null check:
const publishPackage = async (ws: Package, workspaces: Package[]) => { const info = workspaces.find((it) => it.name === ws.name); + if (!info) { + throw new Error(`Package ${ws.name} not found in workspaces`); + } const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-'));
78-82: Fix potential runtime error withdecodeURIComponent.The optional chaining
?.on line 80 will not preventdecodeURIComponent(req.url)from throwing ifreq.urlisnullorundefined. The error occurs before the optional chaining is evaluated. This issue was previously flagged but remains unresolved.Apply this diff to add a null check:
if ( req.url === '/' || - packages.some((it) => decodeURIComponent(req.url)?.startsWith('/' + it.name)) || + (req.url && packages.some((it) => decodeURIComponent(req.url).startsWith('/' + it.name))) || req.method === 'PUT' ) {
🧹 Nitpick comments (2)
scripts/run-registry.ts (2)
199-210: Consider logging a warning for unresolved workspace dependencies.When a workspace dependency is not found in the packages list (line 203), the code silently continues without resolving it. This could leave
workspace:protocol dependencies unresolved in the published package, which would cause installation failures. Consider adding a warning or throwing an error to make this visible.Apply this diff to add a warning:
const info = packages.find((it) => it.name === depName); if (!info) { + console.warn(`Warning: workspace dependency "${depName}" not found in packages list for ${pkg.name}`); continue; }
232-232: Use theREGISTRY_PORTconstant instead of hardcoded port.For consistency with the rest of the code, use the
REGISTRY_PORTconstant instead of hardcoding6001.Apply this diff:
- const verdaccioUrl = `http://localhost:6001`; + const verdaccioUrl = `http://localhost:${REGISTRY_PORT}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/run-registry.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/run-registry.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/run-registry.ts
🧠 Learnings (2)
📓 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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/run-registry.ts
🧬 Code graph analysis (1)
scripts/run-registry.ts (3)
scripts/utils/port.ts (1)
killPort(7-20)scripts/utils/constants.ts (1)
CODE_DIRECTORY(9-9)scripts/utils/tools.ts (2)
picocolors(118-118)limit(120-120)
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (1)
scripts/run-registry.ts (1)
28-32: Verify the default value for the--localflag.The
--localoption defaults totrue, which means local versioning (e.g.,10.2.0-alpha.1-local.1764865413053) will always be applied unless explicitly disabled. This is unusual for a CLI flag—typically flags default tofalse. Please verify this is the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/run-registry.ts (1)
156-185: Add null check for workspace lookup.The
findon line 157 could theoretically returnundefinedif the workspace is not found, causing a runtime error when accessinginfo.publishVersionon line 163.Apply this diff to add defensive error handling:
const publishPackage = async (ws: Package, workspaces: Package[]) => { const info = workspaces.find((it) => it.name === ws.name); + if (!info) { + throw new Error(`Package ${ws.name} not found in workspaces array`); + } + const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-'));Alternatively, since
wsis already from theworkspacesarray (based on the call site at line 264), consider passingwsdirectly instead of looking it up again, which would simplify the logic.
🧹 Nitpick comments (1)
scripts/run-registry.ts (1)
76-91: Add defensive check forreq.urlbefore decoding.While
req.urlis typically defined in Node.js HTTP servers, a defensive check beforedecodeURIComponentwould prevent potential errors in edge cases.Apply this diff to add the safety check:
const proxy = http.createServer((req, res) => { - // if request contains "storybook" redirect to verdaccio - const normalized = decodeURIComponent(req.url); - - if ( - req.url === '/' || - packages.some(({ name }) => normalized === `/${name}` || normalized.includes(`/${name}/`)) - ) { + if (!req.url) { + res.writeHead(400); + res.end(); + return; + } + + const normalized = decodeURIComponent(req.url); + + if ( + req.url === '/' || + packages.some(({ name }) => normalized === `/${name}` || normalized.includes(`/${name}/`)) + ) { res.writeHead(302, { Location: `http://localhost:${VERDACCIO_PORT}` + req.url }); res.end(); } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nx.json(1 hunks)scripts/run-registry.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/run-registry.tsnx.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/run-registry.ts
🧠 Learnings (3)
📓 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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/run-registry.ts
📚 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:
nx.json
⏰ 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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (6)
nx.json (1)
46-46: LGTM! Disabling cache aligns with ephemeral local publishing.Disabling cache for the
publishtarget ensures that each publish operation uses fresh artifacts, which is appropriate for the local Verdaccio workflow with per-package ephemeral versioning introduced in this PR.scripts/run-registry.ts (5)
19-23: LGTM! Constants improve maintainability and enable ephemeral versioning.The constants centralize configuration and
RUN_IDprovides a unique timestamp for ephemeral version tagging across the publish operation.
27-32: LGTM! CLI options provide flexibility for local vs. standard publishing.The
--localflag defaulting totrueis appropriate for development workflows, enabling ephemeral versioning by default while allowing standard versioning when needed.
188-213: LGTM! Workspace dependency resolution is correctly implemented.The function properly handles all dependency types and correctly resolves
workspace:protocol dependencies to published versions, includingworkspace:^andworkspace:~formats.
215-228: LGTM! Package enrichment with ephemeral versioning is well-implemented.The function correctly enriches workspace metadata with
publishVersion, using theRUN_IDtimestamp to ensure unique local versions when the--localflag is enabled.
263-265: LGTM! Concurrent publishing with proper concurrency control.Using
p-limitto control concurrent package publishing is a good approach for balancing performance and resource usage.
| const child = executeCommand({ | ||
| command: 'npx', | ||
| args: ['--yes', `${targetCli.pkg}@${versions[targetCli.pkg]}`, ...targetCli.args], | ||
| args: ['--yes', `${targetCli.pkg}@${packageJson.version}`, ...targetCli.args], |
There was a problem hiding this comment.
are you sure this is a safe change?
it will change it from being the version of @storybook/cli or create-storybook to always be the version of storybook?
There was a problem hiding this comment.
I think the actual behavior will be the same, but I don't understand why the change is needed.
What is the intend here?
|
|
||
| if ( | ||
| req.url === '/' || | ||
| packages.some(({ name }) => normalized === `/${name}` || normalized.includes(`/${name}/`)) |
There was a problem hiding this comment.
I think this warrants a comment, I'm not sure what this string condition means.
There was a problem hiding this comment.
The previous code was optimized for performance, a quick and dirty "does the url contain storybook or sb is a single check; and thus very fast.
The edge cases within the monorepo where this unintentionally hits verdacccio and verdaccio needs to proxy it further are existent, but very rare.
This function is getting invokes hundreds of thousands of times during an install of a sandbox.
| } | ||
| }; | ||
|
|
||
| // Resolve workspace dependencies to the published version |
There was a problem hiding this comment.
the comment you deleted was very helpful. I understand that the flow is now different, but it essentially solves the same thing just in a different way, so I would expect an equally verbose comment to explain why.
There was a problem hiding this comment.
I agree, deleting this comment will result in wasted time in the future; as the behavior is not self-explanatory/logical. The comment prevents future us from removing the seemly unnecessary logic.
scripts/run-registry.ts
Outdated
| * If you want to access the verdaccio UI, you can do so by visiting http://localhost:6002 | ||
| */ | ||
| const proxy = http.createServer((req, res) => { | ||
| // if request contains "storybook" redirect to verdaccio |
There was a problem hiding this comment.
| // if request contains "storybook" redirect to verdaccio | |
| // if request is for any of the monorepo packages redirect to verdaccio |
| .option( | ||
| '--local', | ||
| 'publish unique local version such as 10.2.0-alpha.1-local.1764865413053', | ||
| true | ||
| ); |
There was a problem hiding this comment.
Why should this be configurable? When should it be true, or false?
| await killPort(REGISTRY_PORT); | ||
| await killPort(VERDACCIO_PORT); |
There was a problem hiding this comment.
| await killPort(REGISTRY_PORT); | |
| await killPort(VERDACCIO_PORT); | |
| await Promise.all([ | |
| killPort(REGISTRY_PORT), | |
| killPort(VERDACCIO_PORT) | |
| ]); |
| }; | ||
|
|
||
| proxy.listen(6001, () => { | ||
| proxy.listen(REGISTRY_PORT, () => { |
There was a problem hiding this comment.
PROXY_PORT seems a more appropriate name
| interface Package { | ||
| name: string; | ||
| location: string; | ||
| path: string; | ||
| version: string; | ||
| publishVersion: string; | ||
| } |
There was a problem hiding this comment.
There seems to be a mixed use of the words workspace and package
And also overlap between the Workspace type and this type.
Should this type be defined here?
|
|
||
| const pkg = JSON.parse(await readFile(join(tmpDir, 'package.json'), 'utf8')); | ||
| pkg.version = info.publishVersion; | ||
| resolveWorkspaceDeps(pkg, workspaces); |
There was a problem hiding this comment.
Can we please not do mutation unless critical for performance reasons?
Surely a pure function is more appropriate?
I find understanding, and debugging mutative functions very non-fun.
| if (latestInRange?.includes('-local.')) { | ||
| return `${packageName}@local`; | ||
| } |
There was a problem hiding this comment.
The latestInRange is a lookup of what's on the latest tag on the current registry.
It including -local would only ever be the case when running verdaccio, right?
So I'm guessing latest and local tags are actually equal on verdaccio.
So what is the point of converting it to local tag?
There was a problem hiding this comment.
We have tests for this code IIRC, we should ensure added behavior has tests in place.
|
|
||
| if ( | ||
| req.url === '/' || | ||
| packages.some(({ name }) => normalized === `/${name}` || normalized.includes(`/${name}/`)) |
There was a problem hiding this comment.
The previous code was optimized for performance, a quick and dirty "does the url contain storybook or sb is a single check; and thus very fast.
The edge cases within the monorepo where this unintentionally hits verdacccio and verdaccio needs to proxy it further are existent, but very rare.
This function is getting invokes hundreds of thousands of times during an install of a sandbox.
| verdaccioApp = app; | ||
|
|
||
| app.listen(6002, () => { | ||
| app.listen(VERDACCIO_PORT, () => { |
There was a problem hiding this comment.
| app.listen(VERDACCIO_PORT, () => { | |
| verdaccioApp.listen(VERDACCIO_PORT, () => { |
| for (const field of [ | ||
| 'dependencies', | ||
| 'devDependencies', | ||
| 'peerDependencies', | ||
| 'optionalDependencies', | ||
| ] as const) { | ||
| const deps = pkg[field]; | ||
| if (!deps) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const [depName, raw] of Object.entries(deps)) { | ||
| if (typeof raw !== 'string' || !raw.startsWith('workspace:')) { | ||
| continue; | ||
| } | ||
| const info = packages.find((it) => it.name === depName); | ||
| if (!info) { | ||
| continue; | ||
| } | ||
| const spec = raw.slice('workspace:'.length); | ||
| const version = info.publishVersion; | ||
| deps[depName] = spec === '^' || spec === '~' ? `${spec}${version}` : version; | ||
| } | ||
| } |
There was a problem hiding this comment.
At some point in this code, we're 3 iterator-levels deep.. I find it hard to keep mental track of what is going on.
I hope there's a more expressive way to write this code so it's more self-explanatory what it's doing.
Or maybe a big JsDoc-comment will help explain what the code is doing.
| logs: | ||
| - { type: stdout, format: pretty, level: warn } | ||
| publish: | ||
| # no npm connection needed |
There was a problem hiding this comment.
Either link to the docs, or no comment at all. I think allow_offline: true is pretty self-explanatory.
Docs are here:
https://verdaccio.org/docs/configuration/#offline-publish
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
scripts/utils/yarn.ts (1)
131-133: Update the outdated comment to reflect the new caching strategy.The comment states packages might have "identical versions" causing cache issues, but the new workflow publishes unique versions with
-local.RUN_IDsuffixes (e.g.,10.2.0-alpha.1-local.1764865413053), making global caching safe. This is a duplicate of a previous review comment.- // We don't want to use the cache or we might get older copies of our built packages - // (with identical versions), as yarn (correctly I guess) assumes the same version hasn't changed + // We can use the global cache because each local publish uses a unique version suffix (-local.RUN_ID), + // ensuring no version collisions between different runs `yarn config set enableGlobalCache true`,scripts/run-registry.ts (5)
28-32: Clarify the purpose of the--localflag with a comment or improved help text.The
--localflag defaults totruebut the description only shows an example version format. Consider clarifying when a user might want to set this tofalseand what behavior changes. This was flagged in a past review comment.
154-161: Add null check forinfoto prevent runtime error.If
workspaces.find()doesn't find a matching package,infowill beundefined, causing a runtime error when accessinginfo.publishVersionon line 161. This was flagged in a previous review.const publishPackage = async (ws: Package, workspaces: Package[]) => { const info = workspaces.find((it) => it.name === ws.name); + if (!info) { + throw new Error(`Package ${ws.name} not found in workspaces`); + } const tmpDir = await mkdtemp(join(tmpdir(), ws.name.replace('/', '-') + '-'));
186-211: Consider refactoring to avoid mutation ofpkg.
resolveWorkspaceDepsmutates thepkgobject directly. Per past review feedback, a pure function that returns a new object would be easier to understand and debug. However, since this is an internal utility and the mutation is localized, this is a recommended (not essential) refactor.
213-226: Consider enhancinggetCodeWorkspacesinstead of wrapping it.Per past review feedback, this logic could be integrated into
getCodeWorkspacesto avoid the separatePackagetype and reduce the mapping overhead. The current approach works but creates some type/terminology overlap between "workspace" and "package."
146-152: Consider unifying thePackageinterface with the workspace type.The
Packageinterface overlaps with the workspace type returned bygetCodeWorkspaces, addingpath,version, andpublishVersion. Per past feedback, consolidating these types or extending the workspace type would reduce confusion.
🧹 Nitpick comments (3)
scripts/utils/yarn.ts (1)
39-39: Add a brief comment explaining the'local'resolution strategy.This change sets all Storybook package resolutions to
'local'instead of spreading specific versions fromstorybookVersions. This aligns with the new Verdaccio local publishing workflow where packages are published with unique-local.RUN_IDsuffixes. A brief inline comment would help future maintainers understand why'local'is used here and how it relates to the Verdaccio registry setup.packageJson.resolutions = { ...packageJson.resolutions, + // Resolve all Storybook packages to 'local' to use packages published to the local Verdaccio registry ...Object.fromEntries(Object.keys(storybookVersions).map((key) => [key, 'local'])),scripts/verdaccio.yaml (1)
19-22: Consider using a less verbose log level for routine usage.Setting
level: debugwill produce verbose output. This is appropriate for troubleshooting but may generate excessive noise during normal sandbox creation. Consider usinginfoorwarnfor typical runs, or making the level configurable.scripts/run-registry.ts (1)
76-81: Add a comment explaining the routing heuristic.Per past review feedback, this routing logic deserves a brief comment explaining that it routes monorepo package requests to Verdaccio while forwarding others to npm. The
normalizedcheck against package names is the key optimization here.const normalized = decodeURIComponent(req.url); + // Route requests for monorepo packages to Verdaccio; forward all others to npm for performance if ( req.url === '/' || packages.some(({ name }) => normalized === `/${name}` || normalized.includes(`/${name}/`)) ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/run-registry.ts(8 hunks)scripts/utils/yarn.ts(3 hunks)scripts/verdaccio.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/utils/yarn.tsscripts/run-registry.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/utils/yarn.tsscripts/run-registry.ts
🧠 Learnings (9)
📓 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.
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
📚 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:
scripts/utils/yarn.ts
📚 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:
scripts/utils/yarn.ts
📚 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:
scripts/utils/yarn.ts
📚 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:
scripts/utils/yarn.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
scripts/utils/yarn.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/utils/yarn.ts
📚 Learning: 2025-11-28T14:50:24.889Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.889Z
Learning: Applies to code/**/*.{ts,tsx,js,jsx,mjs} : Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Applied to files:
scripts/utils/yarn.ts
📚 Learning: 2025-11-24T17:49:31.838Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-24T17:49:31.838Z
Learning: Applies to code/vitest.workspace.ts : Vitest configuration is centralized in `code/vitest.workspace.ts` for workspace setup
Applied to files:
scripts/run-registry.ts
🧬 Code graph analysis (1)
scripts/run-registry.ts (3)
scripts/utils/port.ts (1)
killPort(7-20)code/lib/create-storybook/src/dependency-collector.ts (1)
getPackages(142-147)scripts/utils/constants.ts (1)
CODE_DIRECTORY(9-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). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (3)
scripts/verdaccio.yaml (1)
24-26: LGTM - offline publishing is appropriate for this workflow.Enabling
allow_offline: trueis correct for the local publishing workflow where packages are published directly to Verdaccio without needing to proxy to npmjs.scripts/run-registry.ts (2)
19-23: LGTM - Constants are well-defined.The constants
PUBLISH_TAG,RUN_ID,REGISTRY_PORT, andVERDACCIO_PORTare clearly named and appropriately scoped at module level. UsingDate.now()forRUN_IDensures unique version suffixes across runs.
257-259: LGTM - Concurrent publishing with rate limiting.Using
pLimit(maxConcurrentTasks)to parallelize package publishing while respecting system resources is a good pattern. The implementation correctly awaits all publish operations before proceeding.
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
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.