Skip to content

Conversation

@jamesbhobbs
Copy link
Contributor

@jamesbhobbs jamesbhobbs commented Oct 21, 2025

Summary

Updates Node.js version to 22.21.0 in both .nvmrc files:

  • Root .nvmrc: 22.15.1 → 22.21.0 (minor version bump)
  • src/test/vscode-notebook-perf/.nvmrc: v20.17.0 → 22.21.0 (major version jump)

⚠️ Important: The performance test folder was previously on Node 20.x and is now being upgraded to Node 22.x - please verify this is intentional.

Context

Part of a Node version standardization across Deepnote repositories.

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

Review Checklist

  • Critical: Confirm src/test/vscode-notebook-perf should be on Node 22 (was intentionally on Node 20)
  • Verify all CI checks pass, especially native module builds (zeromq, etc.)
  • Test local development: nvm use && npm install && npm test
  • Check performance benchmarks still run correctly and produce consistent results
  • Verify no dependency compatibility warnings with Node 22.21.0

Summary by CodeRabbit

Chores

  • Updated Node.js version specifications in project configuration files to ensure consistent development environments.

@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 21, 2025

📝 Walkthrough

Walkthrough

Node.js version specifications in two .nvmrc files are updated. The root .nvmrc changes from 22.15.1 to 22.21.0, and the test folder .nvmrc changes from v20.17.0 to 22.21.0, standardizing both to the same version.

Possibly related PRs

Suggested reviewers

  • Artmann

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 "chore: update Node.js version to 22.21.0" directly and accurately summarizes the main change in the PR. Both modified .nvmrc files are updated to Node.js 22.21.0, and the title clearly conveys this primary objective. The title is specific about what is being updated and to what version, avoiding vague or generic phrasing, while following standard commit message conventions.
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 553a99f and ff5df51.

📒 Files selected for processing (2)
  • .nvmrc (1 hunks)
  • src/test/vscode-notebook-perf/.nvmrc (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T10:19:33.783Z
Learning: Applies to {.nvmrc,.node-version,package.json} : Use Node.js 22 for this project (e.g., nvm use 22)
📚 Learning: 2025-09-29T10:19:33.783Z
Learnt from: CR
PR: deepnote/deepnote-internal#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T10:19:33.783Z
Learning: Applies to {.nvmrc,.node-version,package.json} : Use Node.js 22 for this project (e.g., nvm use 22)

Applied to files:

  • .nvmrc
  • src/test/vscode-notebook-perf/.nvmrc
🔇 Additional comments (2)
src/test/vscode-notebook-perf/.nvmrc (1)

1-1: Verify the Node 20→22 jump is intentional and re-baseline performance.

This moves the perf test suite from Node 20 to 22—a major version upgrade that affects performance baselines and native module builds. Confirm this aligns with your testing strategy and that performance benchmarks have been re-run.

.nvmrc (1)

1-1: LGTM. Standardizing to 22.21.0 aligns with project conventions. Ensure nvm use && npm install succeeds locally and CI passes.


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

@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

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

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #105   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        527     527           
  Lines      39474   39474           
  Branches    4933    4933           
=====================================
  Hits       28277   28277           
  Misses      9563    9563           
  Partials    1634    1634           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jamesbhobbs jamesbhobbs marked this pull request as ready for review October 21, 2025 18:01
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

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

I think we should get rid of telemetry tests before we reimplement them to work for deepnote notebooks. They currently are meant so send telemetry data reg. how fast jupyter block outputs for text, html, image render. There's also no ci check that runs them.

@saltenasl
Copy link
Member

Admin merging

@saltenasl saltenasl merged commit a50443c into main Oct 22, 2025
12 checks passed
@saltenasl saltenasl deleted the devin/1761062736-update-node-22.21.0 branch October 22, 2025 07:48
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