Skip to content

ci: read node version from .nvmrc in github workflows (#4919) - #4921

Open
NoopDog wants to merge 3 commits into
mainfrom
noopdog/4919-nvmrc-workflows
Open

ci: read node version from .nvmrc in github workflows (#4919)#4921
NoopDog wants to merge 3 commits into
mainfrom
noopdog/4919-nvmrc-workflows

Conversation

@NoopDog

@NoopDog NoopDog commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Closes #4919

What changed

  • run-checks.yml, run-playwright-tests-anvil-catalog.yml, run-playwright-tests-anvil-cmg.yml: actions/setup-node now uses node-version-file: ".nvmrc" instead of a hard-coded node-version: "22.12.0", and enables npm caching (cache: "npm"). The two workflows that install files/ dependencies key the cache on both lockfiles; anvil-cmg keys on the root lockfile only, since it never installs files/.
  • The four deploy scripts (cgl-anvil-catalog-prod-deploy.sh, cc-anvil-catalog-dev-deploy.sh, cc-data-browser.lungmap.dev.clevercanary.com-deploy.sh, cc-explore.anvilproject.dev.clevercanary.com-deploy.sh): n 22.12.0n $(cat .nvmrc), so deploys build on the same Node CI tests.
  • README.md: prerequisite updated from 22.12.0 to 22.13.0 with a pointer to .nvmrc.

Why

CI had drifted to Node 22.12.0 while the repo's pin (package.json engines, .gitlab/Dockerfile, and .nvmrc from #4918) is 22.13.0 — and the deploy scripts had drifted the same way, so production artifacts were built on a Node version CI never tested. Reading .nvmrc everywhere makes it the single source of truth: a future Node bump touches .nvmrc, engines, and the GitLab Dockerfile, and CI plus deploys follow automatically. Matches the convention in clevercanary/hca-atlas-tracker. Completes the remaining workflows item from #4876.

Assumptions I made

  • The deploy scripts always run from the repo root (they already reference ./out and npm ci relative to it), so $(cat .nvmrc) resolves.
  • Uniform npm caching is wanted in all three workflows even though it slightly grows the anvil-cmg job's setup step on cache miss; on hit it saves the full registry download.
  • Playwright browser caching (a larger recurring CI cost) was deliberately left out to keep this change scoped; it's tracked as ci: cache Playwright browser downloads in the Playwright workflows #4920.
  • The triplicated setup-node block was left as-is: the repo has no composite-action convention, and three copies of six lines didn't justify introducing one.

How to verify

Definition of done from #4919, mapped to steps:

  • All three workflows read the version from .nvmrc — open the "Run checks" run for this PR, expand the setup-node step, and confirm it logs Resolved .nvmrc as 22.13.0 and installs Node 22.13.0 (not 22.12.0). Repeat for either Playwright workflow run.
  • No workflow hard-codes a Node versiongrep -rn "node-version:" .github/workflows/ returns nothing; only node-version-file remains.
  • npm cache is active — in the same setup-node step log, confirm the npm cache is saved/restored (first run saves, re-run of the job restores).
  • Deploy scripts follow the pingrep -n "^n " *.sh shows n $(cat .nvmrc) in all four deploy scripts; running one locally (e.g. the anvil-catalog dev deploy up to the npm ci line) switches to Node 22.13.0 with no EBADENGINE warning.
  • Upgrade to Next.js 16 + new findable-ui major (Phase 2 from findable-ui#952) #4876 follow-through — with this merged, the "workflows on 22.13.0" item on Upgrade to Next.js 16 + new findable-ui major (Phase 2 from findable-ui#952) #4876 is satisfied.

🤖 Generated with Claude Code

NoopDog and others added 2 commits August 11, 2026 01:13
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns CI workflows and deploy scripts with the repository’s pinned Node version by using .nvmrc as the single source of truth, reducing drift between tested and deployed artifacts.

Changes:

  • Updated GitHub Actions workflows to use actions/setup-node with node-version-file: ".nvmrc" and enabled npm caching.
  • Updated deploy scripts to switch Node via .nvmrc instead of a hard-coded version.
  • Updated README prerequisite Node version to match .nvmrc.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
README.md Updates documented Node prerequisite to reference .nvmrc.
cgl-anvil-catalog-prod-deploy.sh Uses .nvmrc for Node version during prod deploy build.
cc-anvil-catalog-dev-deploy.sh Uses .nvmrc for Node version during dev deploy build.
cc-data-browser.lungmap.dev.clevercanary.com-deploy.sh Uses .nvmrc for Node version during lungmap dev deploy build.
cc-explore.anvilproject.dev.clevercanary.com-deploy.sh Uses .nvmrc for Node version during explore dev deploy build.
.github/workflows/run-checks.yml Reads Node from .nvmrc and enables npm dependency caching.
.github/workflows/run-playwright-tests-anvil-catalog.yml Reads Node from .nvmrc and enables npm dependency caching (including files/).
.github/workflows/run-playwright-tests-anvil-cmg.yml Reads Node from .nvmrc and enables npm dependency caching.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cgl-anvil-catalog-prod-deploy.sh Outdated
Comment thread cc-anvil-catalog-dev-deploy.sh Outdated
Comment thread cc-explore.anvilproject.dev.clevercanary.com-deploy.sh Outdated
Comment thread cc-data-browser.lungmap.dev.clevercanary.com-deploy.sh Outdated
)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@frano-m

frano-m commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Review

First off — really nice work on this one. The PR does exactly what #4919 asked for and does it cleanly: .nvmrc becomes the single source of truth, CI and deploys stop drifting independently, and the npm caching is a thoughtful bonus. The PR description is exemplary — the explicit "Assumptions I made" section and the verification checklist mapped to the definition of done make this genuinely easy to review. Matching the established convention from hca-atlas-tracker rather than inventing a new one is also appreciated.

A code review pass (with adversarial verification) surfaced three findings worth considering before merge:

1. n "$(cat .nvmrc)" depends on the caller's working directory — all four deploy scripts

The "scripts always run from the repo root" assumption is the one place this bites. If a deploy script is ever invoked from outside the repo root (e.g. ~/repos/data-browser/cc-anvil-catalog-dev-deploy.sh from $HOME, or a cron/CI wrapper with a different cwd), cat .nvmrc fails — and because a $(...) failure isn't caught by set -e, the substitution silently yields an empty string. We verified with n v9 that n "" resolves to the latest Node release (26.x today) and exits 0, so the deploy would proceed on an untested major version rather than failing fast.

Suggested fix: n auto is a drop-in replacement — it reads .nvmrc itself (also engines, .node-version) and errors out properly when no version file is found.

2. README.md:9 re-hardcodes 22.13.0 next to the .nvmrc pointer

The README now says 22.13.0 and points at .nvmrc — which preserves exactly the second-copy drift this PR was filed to eliminate. On the next Node bump, .nvmrc/engines get updated and the README quietly goes stale; a developer following it installs the old version and hits an EBADENGINE mismatch on npm ci. Suggest dropping the literal version and saying "the version pinned in .nvmrc" (optionally with nvm install/n auto as the install command).

3. (Minor) The triplicated setup-node block

Acknowledged this was a deliberate call in the assumptions, and it's a defensible one — flagging only that the copies have already diverged (anvil-cmg omits files/package-lock.json from the cache key), so a future change to Node setup or caching needs three coordinated edits. If it grows any further, a composite action (.github/actions/setup-node-npm with the lockfile list as an input) would hold it once. Fine to defer.


Findings 1 and 2 are small, mechanical fixes; happy to push them to this branch if wanted.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: read Node version from .nvmrc in GitHub workflows (node-version-file)

3 participants