CI Workflow Modernization#463
Conversation
…updating language configuration
📝 WalkthroughWalkthroughReworks GitHub Actions workflows: simplifies CodeQL config to target javascript-typescript and disables autobuild; splits the Node CI into separate lint (type-check & lint) and test jobs with an expanded matrix; updates runner images and action versions, and upgrades Node in the release workflow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/nodejs.yml (1)
38-38:⚠️ Potential issue | 🟡 MinorNode.js 18.x in the test matrix is past its EOL date.
Node.js 18 reached end-of-life on April 30, 2025. As of February 2026, continuing to test against it provides no security coverage guarantee and may produce misleading green results. Consider replacing it with Node.js 23.x (current) or removing it entirely now that Node.js 24.x covers the top of the matrix.
🔧 Remove EOL Node 18.x from the matrix
- node-version: [18.x, 20.x, 22.x, 24.x] + node-version: [20.x, 22.x, 24.x]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml at line 38, Update the GitHub Actions Node.js test matrix by removing the EOL 18.x entry under the node-version key (or replace it with a supported version such as 23.x); locate the node-version: [18.x, 20.x, 22.x, 24.x] array in the workflow and update it to a supported set (e.g., [20.x, 22.x, 23.x, 24.x] or [20.x, 22.x, 24.x]) so CI no longer runs tests against Node 18.
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
44-47: Addcache: npmto the test job'ssetup-nodestep.With 12 matrix combinations (4 Node versions × 3 OS), every run performs 12 independent cold
npm ciinvocations. Addingcache: npmhere — consistent withrelease.yml— cuts redundant network traffic and shaves significant time off each CI run.⚡ Proposed change
- name: Use Node.js ${{ matrix.node-version }} uses: actions/setup-node@v6 with: node-version: ${{ matrix.node-version }} + cache: npm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml around lines 44 - 47, Add the npm cache configuration to the test job's setup-node step: in the actions/setup-node@v6 step (the "Use Node.js ${{ matrix.node-version }}" invocation) add the key "cache: npm" under the "with:" block so the action uses the built-in npm caching like release.yml; keep the existing node-version entry and ensure spacing/indentation matches the YAML structure.
🤖 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/workflows/nodejs.yml:
- Around line 9-28: The lint job's Node setup omits npm caching and risks
hitting the 15-minute single-CPU runner timeout; update the lint job's
actions/setup-node@v6 step (the "Use Node.js LTS" step) to include cache: npm in
its with block so setup-node will cache based on the lockfile (matching
release.yml) and avoid repeated cold npm installs.
---
Outside diff comments:
In @.github/workflows/nodejs.yml:
- Line 38: Update the GitHub Actions Node.js test matrix by removing the EOL
18.x entry under the node-version key (or replace it with a supported version
such as 23.x); locate the node-version: [18.x, 20.x, 22.x, 24.x] array in the
workflow and update it to a supported set (e.g., [20.x, 22.x, 23.x, 24.x] or
[20.x, 22.x, 24.x]) so CI no longer runs tests against Node 18.
---
Nitpick comments:
In @.github/workflows/nodejs.yml:
- Around line 44-47: Add the npm cache configuration to the test job's
setup-node step: in the actions/setup-node@v6 step (the "Use Node.js ${{
matrix.node-version }}" invocation) add the key "cache: npm" under the "with:"
block so the action uses the built-in npm caching like release.yml; keep the
existing node-version entry and ensure spacing/indentation matches the YAML
structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/nodejs.yml (1)
39-39: Consider dropping18.xfrom the matrix — Node 18 has been EOL since April 2025.Node.js 18 reached End of Life on April 30, 2025. Keeping it in the matrix burns 3 extra CI jobs per run (one per OS) while providing no meaningful signal, since the runtime no longer receives security fixes or patches.
♻️ Proposed matrix update
- node-version: [18.x, 20.x, 22.x, 24.x] + node-version: [20.x, 22.x, 24.x]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nodejs.yml at line 39, Remove Node 18 from the GitHub Actions matrix since it reached EOL; update the node-version matrix entry (the node-version key in .github/workflows/nodejs.yml) to exclude "18.x" so it reads only the supported versions (e.g., [20.x, 22.x, 24.x]) and run the workflow to confirm CI job count drops accordingly.
🤖 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/workflows/nodejs.yml:
- Around line 59-60: The workflow step "Unit Tests" currently runs only "npx
mocha --timeout 8000", which drops the type check and lint steps previously run
by the "npm test" script (the original "npm run test:types && xo && mocha
--timeout 8000"); restore CI completeness by either (A) replacing that single
run with separate steps that run the type checker ("npm run test:types" / "tsc
--project test/tsconfig.json"), the linter ("xo"), and then the unit tests ("npx
mocha --timeout 8000"), or (B) if the removals were intentional, add a brief
documented comment in the workflow noting that type checking and linting were
intentionally removed; update the "Unit Tests" job name or step to reference
"npx mocha --timeout 8000" only when those other checks are moved to their own
steps.
---
Duplicate comments:
In @.github/workflows/nodejs.yml:
- Around line 9-29: No changes required — the GitHub Actions job is correct:
keep the uses entries as actions/checkout@v6 and actions/setup-node@v6 (they
resolve to v6.0.1 and v6.2.0 respectively) and retain the cache: npm setting; no
modifications to the lint job, the Install Dependencies, Type Check, or Lint
steps (including the npm ci, npm run test:types, and npx xo commands) are
needed.
---
Nitpick comments:
In @.github/workflows/nodejs.yml:
- Line 39: Remove Node 18 from the GitHub Actions matrix since it reached EOL;
update the node-version matrix entry (the node-version key in
.github/workflows/nodejs.yml) to exclude "18.x" so it reads only the supported
versions (e.g., [20.x, 22.x, 24.x]) and run the workflow to confirm CI job count
drops accordingly.
|
Coderabbit suggests removing node 18, and while I fundamentally agree, I think that's outside the scope of this PR. In my opinion, this PR as it stands now can be merged 🙂 |
2 participants
Housekeeping pass over all
.github/workflows/files. No behavioral changes to the release or auto-merge logic.nodejs.yml
lint(Type Check + XO) andtest(matrix) — lint failures bail out early without burning the full 12-job matrixlintjob usesubuntu-slim(1 vCPU, container-based, cheaper for fast jobs)npm installstep afternpm cinpm testwith individual steps (npx mocha) so step names are visible in the UI@v6permissions: contents: readat workflow level (principle of least privilege)codeql-analysis.yml
strategy.matrix(single language, no matrix needed)Autobuildstep (not needed for JS/TS)javascript-typescript, addedbuild-mode: nonepermissionsblock (security-events: write,contents: read, etc.)release.yml
actions/checkoutandactions/setup-nodefrom@v4→@v620→24(Node 20 EOL May 2026)npm install -g npm@latest— Node 24 ships with npm 10, which supports OIDC/Trusted Publishing nativelydependabot-automerge.yml
ubuntu-latest→ubuntu-slimSummary by CodeRabbit