fix: pin NodeJS version explicitly & use consistently in CI#2818
Conversation
|
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:
WalkthroughThe Node setup action now derives Node and pnpm versions from the repository root package.json (engines.node and packageManager), exposes them as outputs, and uses those outputs for ChangesDynamic Version Detection from package.json
TypeScript Dependency Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/node/action.yaml (1)
85-87:⚠️ Potential issue | 🟠 MajorUpdate the pnpm cache key to use the resolved pnpm version.
Lines 85–87 hardcode
pnpm-8-store, but the action now installs pnpm dynamically frompackage.json(currentlypnpm@9.12.3). This mismatch means pnpm major upgrades will restore/save under the old cache namespace instead of creating a clean cache boundary when the pnpm version changes.Suggested fix
- key: ${{ runner.os }}-pnpm-8-store-${{ hashFiles('**/pnpm-lock.yaml') }} + key: ${{ runner.os }}-pnpm-${{ steps.runtime-versions.outputs.pnpm-version }}-store-${{ hashFiles('**/pnpm-lock.yaml') }} restore-keys: | - ${{ runner.os }}-pnpm-8-store- + ${{ runner.os }}-pnpm-${{ steps.runtime-versions.outputs.pnpm-version }}-store-The
steps.runtime-versions.outputs.pnpm-versionoutput is already defined in the action (line 63) and used elsewhere (line 72).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/node/action.yaml around lines 85 - 87, The cache key currently hardcodes "pnpm-8-store" so cache scope doesn't change when pnpm is updated; update the cache key to incorporate the resolved pnpm version by using the existing output steps.runtime-versions.outputs.pnpm-version (the same output used elsewhere) instead of the literal "pnpm-8" token, and likewise update the restore-keys prefix to match so caches separate by pnpm version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/actions/node/action.yaml:
- Around line 85-87: The cache key currently hardcodes "pnpm-8-store" so cache
scope doesn't change when pnpm is updated; update the cache key to incorporate
the resolved pnpm version by using the existing output
steps.runtime-versions.outputs.pnpm-version (the same output used elsewhere)
instead of the literal "pnpm-8" token, and likewise update the restore-keys
prefix to match so caches separate by pnpm version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 672f112e-157a-4de2-952b-5418950c55c5
📒 Files selected for processing (3)
.github/actions/node/action.yaml.github/workflows/release-preview.yaml.github/workflows/release.yaml
💤 Files with no reviewable changes (2)
- .github/workflows/release.yaml
- .github/workflows/release-preview.yaml
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 `@package.json`:
- Line 115: Remove the stray root-level "test" property from package.json:
delete the top-level "test": 1 entry so the package.json contains only valid
configuration fields (do not replace or alter existing scripts or other
properties); locate the literal "test" key at the package root and remove that
key/value pair.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
from
dbe5ec9 to
de59277
Compare
Router image scan passed✅ No security vulnerabilities found in image: |
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
2 times, most recently
from
011880f to
77e7bf7
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 @.github/actions/node/action.yaml:
- Around line 19-35: Change the GitHub Actions step to use the correct shell
template "node {0}", import the node modules and remove top-level await by
wrapping the script in an async IIFE: add "const path = require('path'); const
fs = require('fs').promises;" near the top, keep using
process.env.nodeVersionOverride and pkgPath/pk g variables as-is, and wrap the
code that uses await (the file read and any subsequent await calls that
reference nodeVersion/node parsing) in "(async () => { ... })().catch(err => {
console.error(err); process.exit(1); });" so the script runs in CommonJS without
top-level await.
🪄 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: 67905679-b46a-4e1f-bc54-c01014bba1b5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.github/actions/node/action.yaml.github/workflows/release-preview.yaml.github/workflows/release.yamlpnpm-workspace.yaml
💤 Files with no reviewable changes (2)
- .github/workflows/release.yaml
- .github/workflows/release-preview.yaml
✅ Files skipped from review due to trivial changes (1)
- pnpm-workspace.yaml
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
from
77e7bf7 to
b510185
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2818 +/- ##
===========================================
- Coverage 65.92% 47.39% -18.54%
===========================================
Files 254 1104 +850
Lines 26527 149552 +123025
Branches 0 10246 +10246
===========================================
+ Hits 17489 70875 +53386
- Misses 7639 76861 +69222
- Partials 1399 1816 +417 🚀 New features to boost your workflow:
|
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
from
6d280df to
c54ae9c
Compare
comatory
changed the title
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
2 times, most recently
from
21da3f8 to
edf4e59
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
2 times, most recently
from
dca9162 to
3dfe50f
Compare
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
2 times, most recently
from
a85887c to
6d001ad
Compare
comatory
force-pushed
the
ondrej/eng-9086-use-lts-only-in-cli
branch
2 times, most recently
from
a6057e2 to
16be22b
Compare
3 participants
Caution
Our previous semver for NodeJS specified anything
22.11.0and above, but theactual version
22.11.00would fail to compile. That's why I opted for explicitversion
This PR modifies CI action for setting NodeJS +
pnpmversions to usepackage.jsonas the source of truth, instead of hard-coding different values in CI scripts. On top of this, I removed
working-directoryinput which was unused and does not completely make sense in monorepo setup I think.The consequence of this change is for local development, you must have explicit 22.22.2 installed.
This change was tested by manually modifying
pnpm-lock.yaml(appending a comment) which triggered all CI pipelines related to TS packages, see here. That commit was removed to not pollute the file as it was only intended for testing.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.