fix: add Node.js setup step to CI test job (frontend build fails on Node 18)#153
fix: add Node.js setup step to CI test job (frontend build fails on Node 18)#153
Conversation
- Add actions/setup-node@v6 (Node 22, npm cache) to the test job before the frontend build and test steps to ensure a deterministic Node version - Add frontend/.nvmrc pinned to 22 for local developer consistency - Add engines field to frontend/package.json requiring Node >=20.19 || >=22.12 (matches Vite 8 requirements) Fixes #149 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 434438b92c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
frontend/package.json
Outdated
| "version": "0.1.0", | ||
| "type": "module", | ||
| "engines": { | ||
| "node": ">=20.19 || >=22.12" |
There was a problem hiding this comment.
Encode disjoint Node engine ranges correctly
"node": ">=20.19 || >=22.12" is logically equivalent to just >=20.19, so it does not enforce the intended split support window and will treat Node 21.x (and any 22.x) as valid. That weakens the guardrail this commit adds: developers can run with versions that frontend deps in this repo already declare as unsupported (^20.19.0 || ^22.12.0 || >=24.0.0 in frontend/package-lock.json) and only hit failures later during install/build. Use a non-overlapping range such as ^20.19.0 || >=22.12.0 to make the engine check actually catch unsupported runtimes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. engines.node in frontend/package.json is now ^20.19.0 || >=22.12.0, which properly excludes Node 21.x and 22.0–22.11 and mirrors Vite 8's actual requirement.
There was a problem hiding this comment.
Pull request overview
This PR makes the frontend Node.js version requirement explicit and ensures CI uses a compatible Node version so Vite 8 builds don’t fail on older runner defaults.
Changes:
- Add
actions/setup-node@v6(Node 22 + npm cache) to the CItestjob before frontend build/tests. - Add
frontend/.nvmrcto guide local Node version selection. - Add a Node
enginesconstraint tofrontend/package.jsonto document/enforce the required Node range.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
frontend/package.json |
Adds engines.node constraint for Node compatibility with Vite 8. |
frontend/.nvmrc |
Pins local Node major version for nvm/volta workflows. |
.github/workflows/ci.yml |
Ensures CI test job installs a compatible Node version and enables npm caching before building/testing the frontend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/package.json
Outdated
| "version": "0.1.0", | ||
| "type": "module", | ||
| "engines": { | ||
| "node": ">=20.19 || >=22.12" |
There was a problem hiding this comment.
The engines.node range >=20.19 || >=22.12 collapses to just >=20.19, so it will still allow Node 22.0–22.11 even though Vite’s deps require ^20.19.0 || >=22.12.0 (see frontend/package-lock.json for @vitejs/plugin-react). Update this range so it actually excludes unsupported Node 22.x versions below 22.12 and uses full semver (e.g., patch-level) to match the dependency requirement.
| "node": ">=20.19 || >=22.12" | |
| "node": "^20.19.0 || >=22.12.0" |
There was a problem hiding this comment.
Applied the suggestion — engines.node is now ^20.19.0 || >=22.12.0 in frontend/package.json.
frontend/.nvmrc
Outdated
| @@ -0,0 +1 @@ | |||
| 22 | |||
There was a problem hiding this comment.
.nvmrc set to 22 can pick an already-installed Node 22.x that is <22.12 (nvm selects a matching installed version), which will still fail the Vite 8 engine requirement. Consider pinning at least 22.12.0 (or another known-good version) to prevent local builds from using an incompatible 22.x.
| 22 | |
| 22.12.0 |
There was a problem hiding this comment.
Fixed. frontend/.nvmrc is now pinned to 22.12.0 so nvm use / fnm use won't select an incompatible early 22.x.
.github/workflows/ci.yml
Outdated
|
|
||
| - uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: "22" |
There was a problem hiding this comment.
To keep CI consistent with the new frontend/.nvmrc and the Vite 8 requirement (Node 22.12+), consider using node-version-file: frontend/.nvmrc (or pinning node-version to a minimum like 22.12.0) instead of the broad major "22". This avoids version drift and prevents CI from ever selecting an incompatible early 22.x release.
| node-version: "22" | |
| node-version-file: frontend/.nvmrc |
There was a problem hiding this comment.
Applied the suggestion — both setup-node steps in ci.yml (test and dep-check jobs) now use node-version-file: frontend/.nvmrc so CI reads the version from a single source of truth (22.12.0).
- frontend/package.json: engines.node changed from >=20.19 || >=22.12 to ^20.19.0 || >=22.12.0 to properly exclude Node 21.x and 22.0–22.11 - frontend/.nvmrc: pinned to 22.12.0 so nvm/fnm picks a Vite-compatible version - .github/workflows/ci.yml: both setup-node steps now use node-version-file: frontend/.nvmrc instead of hard-coded "22" to stay in sync Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
actions/setup-node@v6with Node 22 + npm cache to thetestCI job, before the frontend build and test steps — makes Node version deterministic and matches thedep-checkjobfrontend/.nvmrcpinned to22so local developers using nvm/volta get the right version automaticallyenginesfield tofrontend/package.json(>=20.19 || >=22.12) to document and enforce the Vite 8 requirementFixes #149
Test plan
testjob now sets up Node 22 beforenpm ci && npm run build— no more runner-default version dependencyfrontend/.nvmrcpresent —nvm use/voltapicks up Node 22 automaticallyfrontend/package.jsonenginesfield warns when wrong Node is used locallyruff check .passes locally (no Python changes)🤖 Generated with Claude Code