chore: adopt Node.js 24 development baseline and Node >=22.13.0 (#317) - #326
chore: adopt Node.js 24 development baseline and Node >=22.13.0 (#317)#326thedavidweng wants to merge 4 commits into
Conversation
|
@cursoragent is attempting to deploy a commit to the open-slide Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe project now targets Node.js 24 for development and CI while published packages require Node.js ChangesNode.js and pnpm toolchain
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
20-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence in checkout.
By default,
actions/checkoutpersists the GitHub token in the local git configuration. Since these CI jobs execute untrusted code (e.g., duringpnpm install,test, andbuild), this exposes the token to potential exfiltration. Consider settingpersist-credentials: falseacross all CI checkout steps to improve the workflow's security posture.
.github/workflows/ci.yml#L20-L21: addwith: persist-credentials: falseunder this step..github/workflows/ci.yml#L68-L69: addwith: persist-credentials: falseunder this step..github/workflows/ci.yml#L113-L114: addwith: persist-credentials: falseunder this step..github/workflows/ci.yml#L158-L159: addwith: persist-credentials: falseunder this step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 20 - 21, Disable Git credential persistence for every actions/checkout@v4 step by adding persist-credentials: false under with at .github/workflows/ci.yml lines 20-21, 68-69, 113-114, and 158-159.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/template/package.json`:
- Around line 6-8: Remove the Node.js engines constraint from the template
package.json, reverting the change while leaving the rest of the template
unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 20-21: Disable Git credential persistence for every
actions/checkout@v4 step by adding persist-credentials: false under with at
.github/workflows/ci.yml lines 20-21, 68-69, 113-114, and 158-159.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b023a742-10bd-4aea-8a28-0f050ac30b69
📒 Files selected for processing (13)
.changeset/node-engines-22-13.md.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/ci.yml.github/workflows/release.yml.node-version.nvmrcCONTRIBUTING.mdREADME.mdpackage.jsonpackages/cli/package.jsonpackages/cli/template/package.jsonpackages/core/package.jsonpnpm-workspace.yaml
| "engines": { | ||
| "node": ">=22.13.0" | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert changes to this template file.
As per path instructions, packages/cli/template/**/{package.json,open-slide.config.ts}: Do not touch package.json, open-slide.config.ts, or other slides. Please remove this engines constraint to comply with the guidelines.
🐛 Proposed fix to revert changes
- "engines": {
- "node": ">=22.13.0"
- },📝 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.
| "engines": { | |
| "node": ">=22.13.0" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/template/package.json` around lines 6 - 8, Remove the Node.js
engines constraint from the template package.json, reverting the change while
leaving the rest of the template unchanged.
Source: Path instructions
Raise published package engines from node >=18 to >=22.13.0, pin pnpm 11.13.1 via Corepack, add .nvmrc/.node-version, and matrix CI across Node 22.13.x and 24. Configure pnpm 11 allowBuilds for required install scripts. Co-authored-by: Davy <thedavidweng@users.noreply.github.com>
Node 22.13 ships an outdated Corepack that fails pnpm signature verification
("Cannot find matching keyid"), leaving STORE_PATH empty and breaking cache.
Also run pull_request CI for stacked branches, not only PRs targeting main.
Co-authored-by: Davy <thedavidweng@users.noreply.github.com>
45e9c6f to
816b3fa
Compare
|
Rebased onto current
Local validation on Node 24.18.0 with pnpm 11.13.1: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-18: Add a top-level permissions block to the workflow with
contents read access, before the jobs or strategy definitions. Review all jobs
for required token scopes and add only demonstrably necessary permissions; leave
unspecified scopes disabled.
In @.github/workflows/release.yml:
- Around line 29-33: Update the “Enable Corepack” step to avoid the mutable
corepack@latest dependency by using the Node 22.13.0-bundled Corepack or pinning
the global installation to corepack@0.30.0; preserve the subsequent Corepack
enable and prepare actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4f8012e-89f2-4424-9c9d-ab9465517f4e
📒 Files selected for processing (12)
.changeset/node-engines-22-13.md.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/ci.yml.github/workflows/release.yml.node-version.nvmrcCONTRIBUTING.mdREADME.mdpackage.jsonpackages/cli/package.jsonpackages/core/package.jsonpnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- .nvmrc
- pnpm-workspace.yaml
- packages/core/package.json
- .changeset/node-engines-22-13.md
- package.json
- .github/ISSUE_TEMPLATE/bug_report.yml
- README.md
- CONTRIBUTING.md
- packages/cli/package.json
|
Addressed the latest CodeRabbit review (post-rebase): Fixed:
The |
2 participants
Issue reference
Closes / implements #317 (roadmap #318).
Base commit:
61839b78969edc1173657b7f53d53f6bff939f39Exact scope
Runtime / package-manager / CI / release baseline only:
engines.node→>=22.13.0.nvmrc,.node-version, release workflow)packageManager→ pnpm@11.13.1 via CorepackallowBuildsfor required install scripts (esbuild,sharp,msw,protobufjs,core-js,rolldown)Out of scope (unchanged): React, Vite, lint tooling, TypeScript, Vitest, tsdown, Turbo, application dependencies.
Versions before and after
packages/coreengines.node>=18>=22.13.0packages/cliengines.node>=18>=22.13.0engines.node>=22.13.0engines.node>=22.13.0packageManagerpnpm@10.17.0pnpm@11.13.1.nvmrc/.node-version)2222.13.x,24pnpm/action-setup@v4corepack enable) + pnpm store cacheWhy the old state was a problem
node >=18while Node 18 and Node 20 are end-of-life — an untruthful support contract..nvmrc/.node-version), so local and release environments drifted.node >=22.13, matching the intended support floor.pnpm build.This is framed as unsupported/EOL runtime claims and compatibility baseline, not as a confirmed vulnerability in Node 18/pnpm 10 themselves.
Why these versions and tools were chosen
>=22.13.0: truthful published minimum; matches pnpm 11’sengines.node(>=22.13) and remains a maintained LTS line.packageManager.packageManagerfield locally and in CI without a separate pnpm installer action.allowBuilds: required by pnpm 11’s default ignored-build-scripts policy soesbuild/sharp/ etc. can run their install scripts. This is explicit allowlisting, not--force/ overrides / aliases.Breaking changes reviewed
@open-slide/coreand@open-slide/cli: Node 18 and Node 20 are no longer supported for install/runtime of published packages.>=22.13.0.corepack enable && pnpm install).Source changes required
package.json,packages/core/package.json,packages/cli/package.json,packages/cli/template/package.json.nvmrc,.node-versionpnpm-workspace.yaml(allowBuilds).github/workflows/ci.yml,.github/workflows/release.ymlCONTRIBUTING.md,README.md, bug report template placeholder.changeset/node-engines-22-13.md(major for core + cli)Tests added or changed
None. Existing suite re-run on both Node lines (305 tests passed).
Benchmark results
This PR does not claim performance improvements. Install/size observations on the same machine (Node 24, pnpm 11.13.1 after):
node_modulessize (du -sh)@open-slide/core@open-slide/clipnpm installreported lockfile up to date)No percentage runtime improvement is claimed.
Security audit results
pnpm audit/pnpm audit --prodcurrently fail on this registry with HTTP 410 (npm legacy audit endpoint retired). Used OSV Scanner againstpnpm-lock.yamlbefore and after.next@16.2.4(apps/web)vite@5.4.21vitest@2.1.9@babel/core@7.29.0esbuild0.21.5 / 0.28.0dompurify,protobufjs,postcss,valibot,diff,js-yaml,@opentelemetry/core)Total OSV findings: 37 before → 37 after (lockfile resolutions intentionally unchanged).
This PR does not claim to resolve those advisories; it only changes the Node/pnpm contract.
Package and bundle-size changes
See table above. Published tarball sizes unchanged aside from the CLI template
enginesfield (+0.1 kB unpacked).Manual validation results
Same machine; Corepack-enabled pnpm 11.13.1.
Node 24.18.0
pnpm install --frozen-lockfile✅pnpm format:check✅pnpm lint✅ (1 pre-existing Biome warning inrequest-guard.ts)pnpm typecheck✅pnpm test✅ 305 testspnpm build✅ (core, cli, demo, web)Node 22.13.0
pnpm install --frozen-lockfile✅pnpm format:check✅pnpm lint✅pnpm typecheck✅pnpm test✅ 305 testspnpm --filter @open-slide/core build✅pnpm --filter @open-slide/cli build✅pnpm --filter demo build✅Known limitations
pnpm auditis currently unusable against registry.npmjs.org (410). OSV Scanner used instead.allowBuildsentries; listed packages are only those that already needed install scripts under pnpm 10’s ignored-build warning.Prohibited workarounds — confirmation
Not used:
--force--legacy-peer-depspatch-package/ vendored forksSummary by CodeRabbit
Compatibility
Documentation
Chores