Skip to content

Conversation

@jamesbhobbs
Copy link
Contributor

@jamesbhobbs jamesbhobbs commented Oct 20, 2025

Summary

Standardizes Node.js version management across the repository by using .nvmrc instead of hardcoded environment variables, and updates documentation to reflect npm install usage for local development.

Link to Devin run: https://app.devin.ai/sessions/3b7b4414be8845d1bde6d8fedb3c734d
Requested by: James Hobbs (james@deepnote.com) / @jamesbhobbs

Changes

1. Node Version Management (.nvmrc adoption)

  • Removed: NODE_VERSION environment variable from all GitHub workflow files
  • Updated: All actions/setup-node steps to use node-version-file: '.nvmrc'
  • Affected files: ci.yml, package.yml, deps.yml, copilot-setup-steps.yml

2. Local Development Commands

  • Changed: .husky/post-checkout from npm cinpm install
  • Updated: CONTRIBUTING.md to reference npm install for local development (4 locations)
  • Updated: Devcontainer example comments to use npm install
  • Unchanged: All workflow files continue to use npm ci for reproducible CI builds

Review Checklist

⚠️ Critical:

  • Verify .nvmrc exists at repo root and contains 22.15.1
  • Confirm package-lock.json is in sync (run npm install locally and check for modifications)
  • Test that CI workflows successfully use node-version-file parameter

Important:

  • Verify all GitHub workflows still use npm ci (no changes to install commands in workflows)
  • Confirm post-checkout hook behavior is acceptable (runs npm install on every branch switch)

Risks

  • CI dependency on .nvmrc: All workflows will fail if .nvmrc is missing or malformed
  • Post-checkout side effects: Every branch switch triggers npm install, which may update package-lock.json if versions have drifted
  • Untested parameter: The node-version-file parameter can't be validated until CI actually runs

Summary by CodeRabbit

  • Chores

    • Standardized Node.js version resolution across CI/CD and dev environments to use the project's .nvmrc file.
    • Updated automation and container setup to align with the .nvmrc-driven Node selection.
  • Documentation

    • Updated contributing docs and devcontainer comments to recommend npm install instead of npm ci.
    • Updated repository hooks and setup guidance to reflect the npm install recommendation.

…t-checkout hook

- Update all workflow files (ci.yml, package.yml, deps.yml, copilot-setup-steps.yml) to use node-version-file: '.nvmrc' instead of hardcoded NODE_VERSION
- Remove NODE_VERSION environment variable from workflow files
- Change post-checkout hook from npm ci to npm install for local development
- All CI workflows continue to use npm ci as expected
@devin-ai-integration
Copy link

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2025

📝 Walkthrough

Walkthrough

Switches GitHub Actions Node.js setup from using a top-level NODE_VERSION env to reading .nvmrc via node-version-file: '.nvmrc' in all actions/setup-node steps. Removes the NODE_VERSION env declarations. Changes several developer-facing docs and hooks from npm ci to npm install (comments, CONTRIBUTING.md, and .husky/post-checkout).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant GH as GitHub Actions
    participant SetupNode as actions/setup-node
    participant Repo as Repository

    note over GH,Repo #D6EAF8: Previous flow (env variable)
    GH->>SetupNode: node-version: ${{ env.NODE_VERSION }}
    SetupNode->>GH: Install Node (version from env)
    GH->>Repo: run job steps

    par New flow (reads file)
        note over GH,Repo #FDEBD0: Updated flow (node-version-file)
        GH->>Repo: read .nvmrc
        GH->>SetupNode: node-version-file: '.nvmrc'
        SetupNode->>GH: Install Node (version from .nvmrc)
        GH->>Repo: run job steps
    end
Loading

Possibly related PRs

Suggested reviewers

  • Artmann
  • saltenasl
  • andyjakubowski

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly describes the two primary changes in the changeset: replacing hardcoded NODE_VERSION with .nvmrc file across CI/CD workflows (ci.yml, package.yml, deps.yml, copilot-setup-steps.yml), and switching the post-checkout hook from npm ci to npm install. The title is specific about what is being changed and where, avoiding vague terms while remaining concise. A developer scanning commit history would immediately understand that this PR centralizes Node version management to .nvmrc and adjusts local development tooling.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d597dd0 and 2419d9f.

📒 Files selected for processing (1)
  • CONTRIBUTING.md (4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T13:01:49.096Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Pre-commit: run `npm run format-fix`, `npm run lint`, and `npm run test:unittests` before committing

Applied to files:

  • CONTRIBUTING.md
🔇 Additional comments (1)
CONTRIBUTING.md (1)

61-61: Documentation updates align with local development workflow changes.

All changed lines consistently replace npm ci with npm install across developer-facing sections (GitHub Packages setup, main dependencies, typical workflow, local build). This reflects the PR's intent: npm install for local development flexibility, npm ci remains in CI workflows (as noted in PR objectives). The existing caution at line 364 appropriately documents when package-lock.json may be updated.

Also applies to: 81-81, 280-280, 357-357


Comment @coderabbitai help to get the list of available commands and usage tips.

@jamesbhobbs jamesbhobbs marked this pull request as ready for review October 20, 2025 17:36
@jamesbhobbs jamesbhobbs changed the title Use .nvmrc for Node version in CI/CD workflows and npm install in post-checkout hook chore: Use .nvmrc for Node version in CI/CD workflows and npm install in post-checkout hook Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71%. Comparing base (9e00675) to head (2419d9f).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #92   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        523     523           
  Lines      39001   39001           
  Branches    4905    4905           
=====================================
  Hits       27834   27834           
  Misses      9542    9542           
  Partials    1625    1625           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 20, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5b1ad6 and f56b0ec.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml (7 hunks)
  • .github/workflows/copilot-setup-steps.yml (1 hunks)
  • .github/workflows/deps.yml (2 hunks)
  • .github/workflows/package.yml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build & Test
  • GitHub Check: Build & Package Extension
  • GitHub Check: copilot-setup-steps
  • GitHub Check: copilot-setup-steps
🔇 Additional comments (3)
.github/workflows/deps.yml (1)

26-32: Both jobs correctly reference .nvmrc for Node version.

Consistent with changes across other workflows.

Also applies to: 59-63

.github/workflows/package.yml (1)

27-33: Setup-node correctly uses .nvmrc for Node version.

Consistent with other workflows.

.github/workflows/ci.yml (1)

28-34: Workflow updates verified. .nvmrc exists with pinned version 22.15.1.

All seven job updates consistently reference the .nvmrc file correctly. The version pin is explicit and valid for actions/setup-node@v5.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 20, 2025
Changes:
- Updated CD workflow (package.yml) to use npm install
- Updated deps workflow (deps.yml) to use npm install
- Updated copilot-setup-steps workflow to use npm install
- Updated GitHub actions (npm-ci, build-vsix) to use npm install
- Updated Azure pipeline files (stable, pre-release) to use npm install
- Updated devcontainer files to use npm install in comments
- CI workflow files (ci.yml) continue to use npm ci as expected

This ensures npm ci is only used in CI environments where reproducible builds are critical, while npm install is used everywhere else for flexibility.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

♻️ Duplicate comments (1)
.github/workflows/copilot-setup-steps.yml (1)

41-46: Step name still references removed environment variable.

The step name references ${{env.NODE_VERSION}} which no longer exists (removed in this PR). Update the step name to reflect the actual behavior.

-      - name: Use Node ${{env.NODE_VERSION}}
+      - name: Setup Node.js
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f56b0ec and e40fffb.

📒 Files selected for processing (9)
  • .devcontainer/dev-with-python/devcontainer.json (1 hunks)
  • .devcontainer/dev/devcontainer.json (1 hunks)
  • .github/actions/build-vsix/action.yml (1 hunks)
  • .github/actions/npm-ci/action.yml (1 hunks)
  • .github/workflows/copilot-setup-steps.yml (2 hunks)
  • .github/workflows/deps.yml (2 hunks)
  • .github/workflows/package.yml (1 hunks)
  • build/azure-pipeline.pre-release.yml (1 hunks)
  • build/azure-pipeline.stable.yml (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T15:02:51.813Z
Learnt from: Artmann
PR: deepnote/vscode-deepnote#22
File: CONTRIBUTING.md:45-62
Timestamp: 2025-10-07T15:02:51.813Z
Learning: The vscode-deepnote project uses npm for package management, not pnpm. All dependency installation and script execution should use npm commands (e.g., npm ci, npm run).

Applied to files:

  • .github/workflows/package.yml
  • .github/workflows/deps.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: copilot-setup-steps
  • GitHub Check: Build & Package Extension
  • GitHub Check: Build & Test
  • GitHub Check: copilot-setup-steps
🔇 Additional comments (4)
.devcontainer/dev-with-python/devcontainer.json (1)

36-36: LGTM.

Using npm install in devcontainer postCreateCommand is appropriate for local development flexibility.

.devcontainer/dev/devcontainer.json (1)

22-22: LGTM.

For development environments, npm install is the right choice.

.github/actions/build-vsix/action.yml (1)

12-13: File not located; cannot verify code state.

The file .github/actions/build-vsix/action.yml could not be found in the repository. Without access to the actual file, I cannot confirm whether the npm ci → npm install swap occurred or validate the comment text.

npm ci is designed for CI/CD with deterministic builds (requires lockfile, errors on mismatch, never mutates lockfile), while npm install rewrites the lockfile and may install new versions—so the review's technical concern is valid if the swap happened. But I need you to confirm the file exists and show the current state.

Please verify: Does .github/actions/build-vsix/action.yml exist in this repository? If so, paste its content or confirm the file path.

build/azure-pipeline.stable.yml (1)

70-71: Review comment is based on incomplete context; change is intentional.

The commit e40fffb9 explicitly states "Update npm ci to npm install everywhere except CI workflow files." This was a deliberate team decision, not an oversight. The team distinguished between CI workflows (which retain npm ci) and release pipelines (which use npm install with --foreground-scripts). The --foreground-scripts flag indicates a specific build requirement.

Since the change reflects informed architectural choice, no action needed.

devin-ai-integration bot and others added 3 commits October 20, 2025 19:12
CI workflow files should use npm ci for reproducible builds:
- package.yml (CD workflow)
- deps.yml (dependency checking)
- copilot-setup-steps.yml

Non-workflow files continue to use npm install:
- GitHub actions (npm-ci, build-vsix)
- Azure pipelines (stable, pre-release)
- Post-checkout git hook
- Devcontainer files
All build and deployment pipelines should use npm ci for reproducible builds:
- .github/actions/npm-ci/action.yml
- .github/actions/build-vsix/action.yml
- build/azure-pipeline.stable.yml
- build/azure-pipeline.pre-release.yml

Only the post-checkout git hook and devcontainer files now use npm install.
@jamesbhobbs jamesbhobbs requested a review from saltenasl October 21, 2025 10:46
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 21, 2025
@jamesbhobbs jamesbhobbs enabled auto-merge (squash) October 21, 2025 10:48
Changed all references from npm ci to npm install in documentation:
- Setup instructions
- Typical workflow example
- Local build instructions

This aligns with the change to use npm install for local development
while CI workflows continue to use npm ci for reproducible builds.
@jamesbhobbs jamesbhobbs disabled auto-merge October 21, 2025 11:00
@jamesbhobbs jamesbhobbs merged commit 614334b into main Oct 21, 2025
12 checks passed
@jamesbhobbs jamesbhobbs deleted the devin/1760981665-use-nvmrc-in-workflows branch October 21, 2025 11:00
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.

3 participants