refactor(api-service, worker, ws, dashboard): Novu node 22 upgrade#10181
refactor(api-service, worker, ws, dashboard): Novu node 22 upgrade#10181
Conversation
- Update .nvmrc to 22.22.1 - Update root package.json engines to >=22 <23 - Update all Dockerfiles (api, worker, ws, webhook, inbound-mail, dashboard, cursor, devcontainer) to node:22-alpine3.20 - Update all GitHub Actions workflows and composite actions to use node 22.22.1 - Update @types/node from ^20.x to ^22.0.0 across all packages - Update dev-environment-setup.sh to install v22.22.1 - Update esbuild target from node20 to node22 - Update documentation (AGENTS.md, CLAUDE.md, CONTRIBUTING.md, bug report template) - Regenerate pnpm-lock.yaml Co-authored-by: Dima Grossman <dima@grossman.io>
|
Cursor Agent can help with this pull request. Just |
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
Resolved Alpine version conflicts in all Dockerfiles by combining Node 22 upgrade (ours) with Alpine 3.21 bump (next): node:22-alpine3.21 Co-authored-by: Dima Grossman <dima@grossman.io>
scopsy
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request upgrades Node.js from 20 to 22 across the repository. It updates Docker base images, devcontainer ARGs, GitHub Actions and workflow node-version values to 22.22.1, bumps 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/dev-environment-setup.sh (1)
268-288:⚠️ Potential issue | 🟠 MajorUpgrade existing Node 20 installs instead of only checking for a missing binary.
install_node()now targetsv22.22.1, but it still skips installation whenever anynodebinary is present. A developer who already has Node 20 installed will get the “already installed” path and stay on an unsupported runtime even though the repo now requires>=22 <23. Please compare the current version before deciding to skip.Suggested fix
install_node () { NODE_JS_VERSION="v22.22.1" + REQUIRED_NODE_MAJOR="22" SKIP="$(check_nvm)" if [[ -z "$SKIP" ]]; then TEST_CMD=$(execute_command_without_error_print "node --version") - if [[ -z "$TEST_CMD" ]] || [[ "$TEST_CMD" == "zsh: command not found: node" ]]; then + if [[ -z "$TEST_CMD" ]] || [[ "$TEST_CMD" == "zsh: command not found: node" ]] || [[ "$TEST_CMD" != v${REQUIRED_NODE_MAJOR}.* ]]; then installing_dependency "Node.js $NODE_JS_VERSION" nvm install $NODE_JS_VERSION + nvm alias default $NODE_JS_VERSION TEST_NODE_CMD=$(execute_command_without_error_print "node --version") if [[ -z "$TEST_NODE_CMD" ]] || [[ "$TEST_NODE_CMD" == "zsh: command not found: node" ]]; then error_message "Node.js" else success_message "Node.js $NODE_JS_VERSION" fi else already_installed_message "Node.js $NODE_JS_VERSION" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-environment-setup.sh` around lines 268 - 288, install_node currently treats any present node binary as "already installed" and skips upgrades; change it to read the existing version via execute_command_without_error_print "node --version" (handling "command not found" as before), parse the semantic version, and compare against NODE_JS_VERSION (v22.22.1) so that if the installed major/minor is outside the required range (>=22 <23) the function proceeds to install/upgrade via nvm install and verify with TEST_NODE_CMD; keep check_nvm logic but ensure after nvm install you also set/use the installed version (alias/default) and call success_message or error_message based on the post-install version check.
🧹 Nitpick comments (1)
apps/ws/Dockerfile (1)
1-1: Consider pinning the Docker Node patch version here too.
.nvmrc/CI are being standardized on22.22.1, butnode:22-alpine3.21will float to whatever 22.x patch is current later on. That reintroduces environment skew for the exact upgrade this PR is trying to lock down. If patch-level reproducibility matters, pin the image to the same Node patch or a digest.Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ws/Dockerfile` at line 1, The FROM line in the Dockerfile currently uses a floating Node patch tag ("FROM node:22-alpine3.21"); change it to a pinned Node patch (e.g., "node:22.22.1-alpine3.21") or use the image digest to ensure patch-level reproducibility, and apply the same pinning to any other Dockerfiles with the same pattern (the other "FROM node:22-alpine3.21" occurrence noted in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/Dockerfile`:
- Line 1: The Dockerfile uses floating Node tags ("FROM node:22-alpine3.21")
which can drift; update each FROM occurrence that references
"node:22-alpine3.21" (both the top-level FROM and the later FROM at the other
stage) to the pinned patch "node:22.22.1-alpine3.21" so the container build
matches .nvmrc/migration versions and ensures reproducible builds.
In `@apps/dashboard/dockerfile`:
- Line 1: Update the Dockerfile image tags to match the exact Node patch from
.nvmrc: replace the floating "node:22-alpine3.21" occurrences (e.g., the FROM
line and the second occurrence around Line 28) with the pinned patch
"node:22.22.1-alpine3.21" so the dashboard container matches the project's
.nvmrc Node version.
In `@apps/inbound-mail/Dockerfile`:
- Line 1: The Dockerfile base image is floating (FROM node:22-alpine3.21);
update that FROM line to pin the Node patch to 22.22.1 (use
node:22.22.1-alpine3.21) so it matches .nvmrc/CI, and then re-run the repository
check (the ripgrep command provided in the review) to ensure no other floating
Node 22 references remain; change the literal "node:22-alpine3.21" to
"node:22.22.1-alpine3.21" in the Dockerfile.
---
Outside diff comments:
In `@scripts/dev-environment-setup.sh`:
- Around line 268-288: install_node currently treats any present node binary as
"already installed" and skips upgrades; change it to read the existing version
via execute_command_without_error_print "node --version" (handling "command not
found" as before), parse the semantic version, and compare against
NODE_JS_VERSION (v22.22.1) so that if the installed major/minor is outside the
required range (>=22 <23) the function proceeds to install/upgrade via nvm
install and verify with TEST_NODE_CMD; keep check_nvm logic but ensure after nvm
install you also set/use the installed version (alias/default) and call
success_message or error_message based on the post-install version check.
---
Nitpick comments:
In `@apps/ws/Dockerfile`:
- Line 1: The FROM line in the Dockerfile currently uses a floating Node patch
tag ("FROM node:22-alpine3.21"); change it to a pinned Node patch (e.g.,
"node:22.22.1-alpine3.21") or use the image digest to ensure patch-level
reproducibility, and apply the same pinning to any other Dockerfiles with the
same pattern (the other "FROM node:22-alpine3.21" occurrence noted in the
review).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 93bc2dd1-de16-4d7d-a2d1-c21cc1050cd9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
.cursor/Dockerfile.devcontainer/Dockerfile.github/ISSUE_TEMPLATE/bug_report.yml.github/actions/setup-project-minimal/action.yml.github/actions/setup-project/action.yml.github/workflows/community-label.yml.github/workflows/contributor-checks.yml.github/workflows/deploy.yml.github/workflows/issue-label.yml.github/workflows/prepare-enterprise-self-hosted-release.yml.github/workflows/prepare-self-hosted-release.yml.github/workflows/preview-packages.yml.github/workflows/release-packages.yml.github/workflows/reusable-dashboard-e2e.yml.nvmrcAGENTS.mdCLAUDE.mdCONTRIBUTING.mdapps/api/Dockerfileapps/api/package.jsonapps/dashboard/dockerfileapps/inbound-mail/Dockerfileapps/inbound-mail/package.jsonapps/webhook/Dockerfileapps/webhook/package.jsonapps/worker/Dockerfileapps/worker/package.jsonapps/ws/Dockerfileapps/ws/package.jsonenterprise/packages/ai/package.jsonenterprise/packages/api/package.jsonenterprise/packages/auth/package.jsonenterprise/packages/billing/package.jsonenterprise/packages/shared-services/package.jsonenterprise/packages/translation/package.jsonlibs/automation/package.jsonlibs/dal/package.jsonlibs/internal-sdk/examples/package.jsonlibs/testing/package.jsonpackage.jsonpackages/framework/package.jsonpackages/js/package.jsonpackages/nextjs/package.jsonpackages/novu/src/commands/email/bundler/schema-extractor.tspackages/react-native/package.jsonpackages/react/package.jsonpackages/stateless/package.jsonplayground/nestjs/package.jsonplayground/nextjs/package.jsonscripts/dev-environment-setup.sh
- Pin Docker base images to node:22.22.1-alpine3.21 for reproducible builds - Fix install_node() to check existing version and upgrade from Node 20 Co-authored-by: Dima Grossman <dima@grossman.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/dev-environment-setup.sh (1)
1-1: Consider: Shebang declares POSIXshbut script uses Bash/Zsh features.The script uses
#!/bin/shbut relies on[[ ]]syntax (lines 274, 276, and many others) which is not POSIX-compliant. While this works on macOS where/bin/shoften supports these features, consider updating the shebang to#!/bin/bashor#!/bin/zshfor correctness.This is a pre-existing issue, not introduced by this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-environment-setup.sh` at line 1, The script's shebang (#!/bin/sh) is incorrect because the script uses Bash/Zsh-only test syntax like [[ ... ]] (seen around the [[ ]] usages at lines marked 274 and 276); update the shebang to a Bash-compatible interpreter (for portability prefer using /usr/bin/env bash) so the [[ ... ]] tests and other Bash features run correctly, or alternatively convert all [[ ... ]] usages to POSIX-compliant [ ... ] tests if you must keep /bin/sh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/dev-environment-setup.sh`:
- Line 1: The script's shebang (#!/bin/sh) is incorrect because the script uses
Bash/Zsh-only test syntax like [[ ... ]] (seen around the [[ ]] usages at lines
marked 274 and 276); update the shebang to a Bash-compatible interpreter (for
portability prefer using /usr/bin/env bash) so the [[ ... ]] tests and other
Bash features run correctly, or alternatively convert all [[ ... ]] usages to
POSIX-compliant [ ... ] tests if you must keep /bin/sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c06b0852-b9b6-4cfd-a7ce-2e21997be4d5
📒 Files selected for processing (7)
apps/api/Dockerfileapps/dashboard/dockerfileapps/inbound-mail/Dockerfileapps/webhook/Dockerfileapps/worker/Dockerfileapps/ws/Dockerfilescripts/dev-environment-setup.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webhook/Dockerfile
- apps/inbound-mail/Dockerfile
- apps/ws/Dockerfile
- apps/dashboard/dockerfile
node:22.22.1-alpine3.21 doesn't exist on Docker Hub. The available Alpine versions for Node 22.22.1 are 3.22 and 3.23. Updated all Dockerfiles to use node:22.22.1-alpine3.22. Co-authored-by: Dima Grossman <dima@grossman.io>
2 participants
What changed? Why was the change needed?
This PR upgrades the Novu repository to use Node.js 22.22.1 (LTS) from Node.js 20. This update is needed to leverage the latest performance improvements, security enhancements, and language features.
The changes are comprehensive and include:
.nvmrc,package.jsonengine constraints, and@types/nodedependencies across the monorepo.All major services (API, worker, dashboard) have been verified to build successfully with Node.js 22.
Screenshots
Slack Thread