Angular: Update Node CI executors to support Angular prerelease Node.js min version#34112
Conversation
|
View your CI Pipeline Execution ↗ for commit c19ad44
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit d0c693f
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated Node.js and Playwright versions across CI executors, docs, and package manifests; added a new exported Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Helpers as Playwright Helper
participant Workspace as Repo Workspace
participant TestRunner as Test/Serve Step
CI->>Workspace: checkout + install dependencies
CI->>Helpers: call playwright.install(working_directory)
Helpers->>Workspace: run "npx playwright@1.58.2 install chromium --with-deps" rgba(100,150,240,0.5)
Workspace->>CI: Playwright installed
CI->>TestRunner: run e2e/vitest/storybook steps
TestRunner->>Workspace: execute tests/serve using installed Playwright
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
valentinpalkovic
added
ci:daily
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
valentinpalkovic
changed the title
valentinpalkovic
requested review from
jonniebigodes and
kylegach
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/sandbox-templates.ts (1)
628-630: Extract the repeated Svelte plugin override into a shared constant.Lines 629, 642, and 687 all define identical
modifications.extraDependenciesliterals. Extracting this to a shared constant prevents accidental partial updates when the version needs to change.♻️ Proposed refactor
+const svelteVitePluginOverride: NonNullable<Template['modifications']> = { + extraDependencies: ['@sveltejs/vite-plugin-svelte@7.0.0'], +}; + export const baseTemplates = { 'cra/default-js': { @@ 'svelte-vite/default-js': { @@ - modifications: { - extraDependencies: ['@sveltejs/vite-plugin-svelte@7.0.0'], - }, + modifications: svelteVitePluginOverride, @@ 'svelte-vite/default-ts': { @@ - modifications: { - extraDependencies: ['@sveltejs/vite-plugin-svelte@7.0.0'], - }, + modifications: svelteVitePluginOverride, @@ 'svelte-kit/skeleton-ts': { @@ - modifications: { - extraDependencies: ['@sveltejs/vite-plugin-svelte@7.0.0'], - }, + modifications: svelteVitePluginOverride,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/cli-storybook/src/sandbox-templates.ts` around lines 628 - 630, Create a shared constant (e.g., SVELTE_VITE_PLUGIN = '@sveltejs/vite-plugin-svelte@7.0.0') and replace the three identical inline arrays used for modifications.extraDependencies with a reference to that constant (e.g., extraDependencies: [SVELTE_VITE_PLUGIN]). Update every occurrence of the literal in sandbox-templates.ts where modifications.extraDependencies is set so the version is maintained in one place and remove the duplicated string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-storybooks/yarn-pnp/package.json`:
- Line 14: The package.json has inconsistent Playwright versions: resolutions
pins "@playwright/test" and "playwright" to 1.58.2 but devDependencies still
list "playwright" as ^1.56.1; update the devDependency entry for "playwright"
(and any other Playwright entries) to match the resolved version 1.58.2 (or
update resolutions to match the declared devDependency) so the versions are
consistent; locate the "devDependencies" keys for "playwright" and
"@playwright/test" and the "resolutions" block and make them match (use 1.58.2)
to avoid future confusion.
---
Nitpick comments:
In `@code/lib/cli-storybook/src/sandbox-templates.ts`:
- Around line 628-630: Create a shared constant (e.g., SVELTE_VITE_PLUGIN =
'@sveltejs/vite-plugin-svelte@7.0.0') and replace the three identical inline
arrays used for modifications.extraDependencies with a reference to that
constant (e.g., extraDependencies: [SVELTE_VITE_PLUGIN]). Update every
occurrence of the literal in sandbox-templates.ts where
modifications.extraDependencies is set so the version is maintained in one place
and remove the duplicated string literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 880c97a5-9033-4091-a7ff-d640358dc0fb
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.github/copilot-instructions.mdcode/lib/cli-storybook/src/sandbox-templates.tscode/package.jsondocs/writing-tests/in-ci.mdxdocs/writing-tests/index.mdxpackage.jsonscripts/package.jsonscripts/utils/yarn.tstest-storybooks/portable-stories-kitchen-sink/react-vitest-3/package.jsontest-storybooks/portable-stories-kitchen-sink/react/package.jsontest-storybooks/portable-stories-kitchen-sink/svelte/package.jsontest-storybooks/portable-stories-kitchen-sink/vue3/package.jsontest-storybooks/yarn-pnp/package.json
✅ Files skipped from review due to trivial changes (1)
- docs/writing-tests/in-ci.mdx
| }, | ||
| "resolutions": { | ||
| "@playwright/test": "1.52.0", | ||
| "@playwright/test": "1.58.2", |
There was a problem hiding this comment.
Version mismatch between resolutions and devDependencies.
The resolutions pin @playwright/test and playwright to 1.58.2, but the devDependency for playwright at line 68 still specifies ^1.56.1. While resolutions will override, this inconsistency can cause confusion and issues if resolutions are later removed.
Proposed fix
- "playwright": "^1.56.1",
+ "playwright": "^1.58.2",Also applies to: 48-48, 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-storybooks/yarn-pnp/package.json` at line 14, The package.json has
inconsistent Playwright versions: resolutions pins "@playwright/test" and
"playwright" to 1.58.2 but devDependencies still list "playwright" as ^1.56.1;
update the devDependency entry for "playwright" (and any other Playwright
entries) to match the resolved version 1.58.2 (or update resolutions to match
the declared devDependency) so the versions are consistent; locate the
"devDependencies" keys for "playwright" and "@playwright/test" and the
"resolutions" block and make them match (use 1.58.2) to avoid future confusion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/utils/helpers.ts`:
- Around line 98-107: The Playwright helper hardcodes Chromium in
playwright.install (function playwright.install) which limits future browser
support; change the command string in playwright.install from 'yarn playwright
install chromium --with-deps' to the default browser set 'yarn playwright
install --with-deps' (or, if Chromium-only is intentional, document/rename the
helper and keep an explicit per-job browser list) so the helper installs the
standard browsers by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85fdfd29-08bb-4e37-b24a-d36ca7524160
📒 Files selected for processing (5)
scripts/ci/common-jobs.tsscripts/ci/sandboxes.tsscripts/ci/test-storybooks.tsscripts/ci/utils/executors.tsscripts/ci/utils/helpers.ts
| export const playwright = { | ||
| install: (working_directory = 'code') => { | ||
| return { | ||
| run: { | ||
| name: 'Install Playwright', | ||
| working_directory, | ||
| command: 'yarn playwright install chromium --with-deps', | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Playwright configs that mention Firefox/WebKit/Safari projects =="
rg -n -C2 --glob '**/playwright*.{ts,js,mjs,cjs}' --glob '!**/node_modules/**' \
"browserName:\s*['\"](firefox|webkit)['\"]|devices\[['\"](Desktop Firefox|Desktop Safari)['\"]|name:\s*['\"][^'\"]*(Firefox|WebKit|Safari)[^'\"]*['\"]"
echo
echo "== Package scripts that force Firefox/WebKit projects =="
rg -n -C2 --glob '**/package.json' --glob '!**/node_modules/**' \
"\"[^\"]*playwright[^\"]*\"\\s*:\\s*\"[^\"]*(--project(=| )[^\"]*(firefox|webkit)|--browser(=| )[^\"]*(firefox|webkit))[^\"]*\""Repository: storybookjs/storybook
Length of output: 5515
Consider using the default Playwright browser set instead of hardcoding Chromium.
Currently all Firefox/WebKit browser configurations are commented out in the codebase, so hardcoding Chromium poses no immediate regression risk. However, since this helper is now the browser provisioning step after sb_playwright switched to a generic Node image, hardcoding could become problematic if those browsers are re-enabled. Use the default browser set (yarn playwright install --with-deps without chromium) for better forward compatibility, or keep the browser list explicit per job if Chromium-only is intentional.
Suggested fix
export const playwright = {
install: (working_directory = 'code') => {
return {
run: {
name: 'Install Playwright',
working_directory,
- command: 'yarn playwright install chromium --with-deps',
+ command: 'yarn playwright install --with-deps',
},
};
},
};📝 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.
| export const playwright = { | |
| install: (working_directory = 'code') => { | |
| return { | |
| run: { | |
| name: 'Install Playwright', | |
| working_directory, | |
| command: 'yarn playwright install chromium --with-deps', | |
| }, | |
| }; | |
| }, | |
| export const playwright = { | |
| install: (working_directory = 'code') => { | |
| return { | |
| run: { | |
| name: 'Install Playwright', | |
| working_directory, | |
| command: 'yarn playwright install --with-deps', | |
| }, | |
| }; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/utils/helpers.ts` around lines 98 - 107, The Playwright helper
hardcodes Chromium in playwright.install (function playwright.install) which
limits future browser support; change the command string in playwright.install
from 'yarn playwright install chromium --with-deps' to the default browser set
'yarn playwright install --with-deps' (or, if Chromium-only is intentional,
document/rename the helper and keep an explicit per-job browser list) so the
helper installs the standard browsers by default.
3327d61 to
13bd66f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci/utils/helpers.ts`:
- Around line 98-108: The Playwright helper always appends the Linux-only flag
"--with-deps" which breaks Windows jobs; change the exported helper
playwright.install to accept an optional boolean (e.g., includeDeps = true) or a
depsFlag string and only add "--with-deps" to the generated command when
includeDeps is true, leaving the default behavior unchanged, and update the
Windows callsites that invoke playwright.install(...) to pass includeDeps: false
so their commands omit the deps flag; refer to the playwright.install function
to implement the conditional flag and update the two Windows callers that
currently reuse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 157b76b0-9916-45b2-a9c0-bf9949f2d865
📒 Files selected for processing (5)
scripts/ci/common-jobs.tsscripts/ci/sandboxes.tsscripts/ci/test-storybooks.tsscripts/ci/utils/executors.tsscripts/ci/utils/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/common-jobs.ts
| export const playwright = { | ||
| install: (working_directory = 'code') => { | ||
| return { | ||
| run: { | ||
| name: 'Install Playwright', | ||
| working_directory, | ||
| command: 'npx playwright@1.58.2 install chromium --with-deps', | ||
| }, | ||
| }; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does npx playwright install --with-deps officially support Windows environments, and what is the recommended install command for Windows CI runners?
💡 Result:
Yes—Playwright officially supports Windows (Chromium/WebKit/Firefox run on Windows locally and in CI) [1]. The --with-deps option is an official CLI flag (“Install browser system dependencies”), but on Windows CI runners Playwright’s CI docs state no additional OS-level configuration is required (that guidance is specifically called out for Windows/macOS vs Linux) [2][3].
Recommended command for Windows CI runners:
npm ci
npx playwright install
npx playwright test[2]
🏁 Script executed:
cat -n scripts/ci/sandboxes.ts | sed -n '395,415p'Repository: storybookjs/storybook
Length of output: 891
🏁 Script executed:
cat -n scripts/ci/sandboxes.ts | sed -n '440,455p'Repository: storybookjs/storybook
Length of output: 665
🏁 Script executed:
head -20 scripts/ci/utils/helpers.ts && echo "..." && tail -20 scripts/ci/utils/helpers.tsRepository: storybookjs/storybook
Length of output: 1043
🏁 Script executed:
cat -n scripts/ci/utils/helpers.ts | sed -n '95,115p'Repository: storybookjs/storybook
Length of output: 645
🏁 Script executed:
grep -n "export const playwright" scripts/ci/utils/helpers.ts -A 12Repository: storybookjs/storybook
Length of output: 393
🏁 Script executed:
grep -rn "playwright\.install" scripts/ci/ --include="*.ts"Repository: storybookjs/storybook
Length of output: 967
🏁 Script executed:
cat -n scripts/ci/sandboxes.ts | sed -n '370,410p'Repository: storybookjs/storybook
Length of output: 1593
🏁 Script executed:
cat -n scripts/ci/sandboxes.ts | sed -n '350,365p'Repository: storybookjs/storybook
Length of output: 505
🏁 Script executed:
cat -n scripts/ci/sandboxes.ts | sed -n '420,460p'Repository: storybookjs/storybook
Length of output: 1536
🏁 Script executed:
grep -n "defineWindowsSandbox" scripts/ci/sandboxes.tsRepository: storybookjs/storybook
Length of output: 406
Shared Playwright helper uses a Linux-only deps flag but is called by Windows jobs.
Line 104 hardcodes --with-deps in a helper now reused by Windows flows (lines 403 and 447 in scripts/ci/sandboxes.ts). Per Playwright's official CI documentation, Windows runners require no additional system dependencies installation. Make deps installation configurable and disable it for Windows callsites.
Proposed fix
--- a/scripts/ci/utils/helpers.ts
+++ b/scripts/ci/utils/helpers.ts
@@
export const playwright = {
- install: (working_directory = 'code') => {
+ install: (
+ working_directory = 'code',
+ options: { withDeps?: boolean } = { withDeps: true }
+ ) => {
+ const depsFlag = options.withDeps ? ' --with-deps' : '';
return {
run: {
name: 'Install Playwright',
working_directory,
- command: 'npx playwright@1.58.2 install chromium --with-deps',
+ command: `npx playwright@1.58.2 install chromium${depsFlag}`,
},
};
},
};--- a/scripts/ci/sandboxes.ts
+++ b/scripts/ci/sandboxes.ts
@@
- playwright.install(`${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.id}`),
+ playwright.install(`${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.id}`, { withDeps: false }),
@@
- playwright.install(`${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.id}`),
+ playwright.install(`${WINDOWS_ROOT_DIR}\\${SANDBOX_DIR}\\${sandbox.id}`, { withDeps: false }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci/utils/helpers.ts` around lines 98 - 108, The Playwright helper
always appends the Linux-only flag "--with-deps" which breaks Windows jobs;
change the exported helper playwright.install to accept an optional boolean
(e.g., includeDeps = true) or a depsFlag string and only add "--with-deps" to
the generated command when includeDeps is true, leaving the default behavior
unchanged, and update the Windows callsites that invoke playwright.install(...)
to pass includeDeps: false so their commands omit the deps flag; refer to the
playwright.install function to implement the conditional flag and update the two
Windows callers that currently reuse it.
13bd66f to
ebbc503
Compare
valentinpalkovic
force-pushed
the
sidnioulz/update-node-executors-angular
branch
from
ebbc503 to
080ef08
Compare
| }; | ||
| /** Additional CI steps in case this template has special needs during CI. */ | ||
| extraCiSteps?: { | ||
| useNode22?: boolean; |
There was a problem hiding this comment.
nit: For clarity, I would name this "reinstallMinNodeSupportedVersion", so we know when looking at the templates that this enforces an install of the minimum supported version regardless of what the image has previously done.
yannbf
added
the
patch:yes
…utors-angular Angular: Update Node CI executors to support Angular prerelease Node.js min version (cherry picked from commit a61002d)
github-actions
Bot
added
the
patch:done
3 participants
What I did
Follows up on #34079 to fix CI errors with Angular sandboxes such as:
We need to ensure 22.22 as a min ver for the Angular CLI to work nowadays.
Checklist for Contributors
Testing
NO AUTOMATIC TESTS!
Manual testing
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