Skip to content

Conversation

@liranmauda
Copy link
Contributor

@liranmauda liranmauda commented Nov 9, 2025

Explain the Changes

  1. Bumping deps to avoid CVE (13/11/2025)

Summary by CodeRabbit

  • Chores
    • Bumped several runtime and developer dependencies (Node types, test frameworks, native build tools, and related packages) to newer patch/minor releases.
    • Public API surface unchanged — no additions, removals, or signature changes; no user-facing behavior changes.

@coderabbitai
Copy link

coderabbitai bot commented Nov 9, 2025

Walkthrough

Bumps multiple dependency and devDependency versions in package.json (patch/minor updates for dependencies, types, test/build tooling, native modules). No source code or public API/signature changes.

Changes

Cohort / File(s) Summary
Dependency & devDependency updates
package.json
Updated dependency versions: ping 0.4.4 → 1.0.0. Updated devDependencies: @types/node 22.18.6 → 24.10.1, @types/pg 8.15.5 → 8.15.6, jest 30.1.3 → 30.2.0, mocha 11.7.2 → 11.7.5, node-gyp 11.4.2 → 12.1.0, wtfnode 0.10.0 → 0.10.1. Public API unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify type compatibility and CI with @types/node → 24.x and run full test matrix.
  • Check native-build/tooling flows for node-gyp 12.x and test runners (jest, mocha) under updated versions.

Possibly related PRs

Suggested labels

size/M

Suggested reviewers

  • nimrod-becker
  • jackyalbo

Pre-merge checks and finishing touches

✅ 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 'Bumping deps to avoid CVE (13/11/2025)' accurately describes the main change of updating dependencies to address security vulnerabilities.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 343d614 and 05858a3.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json (2 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). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-package-lock-validation
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
package.json (4)

1-147: PR description lacks context and testing instructions.

The PR description is essentially empty—it contains no explanation of:

  • Why these dependency updates are necessary
  • Any bug fixes, security patches, or feature improvements they address
  • Testing instructions to validate the changes
  • Whether this is a routine maintenance bump or addresses a specific issue

This makes it difficult for reviewers to assess the risk and impact of upgrading to these versions. Please provide:

  1. A clear rationale for each major/minor version update (especially ping and node-gyp)
  2. Testing steps that confirm functionality after the updates
  3. Any known issues or compatibility notes for significant version jumps

134-134: Upgrade to @types/node 24.10.1 is safe and appropriate.

The .nvmrc explicitly sets Node.js 24.4.1 as the target, making @types/node 24.10.1 the correct version choice. Verification found no usage of APIs that were removed or broken in Node.js 23-24:

  • No deprecated fs constants (F_OK, R_OK, W_OK, X_OK)
  • No removed APIs like tls.createSecurePair or util.is*()
  • url.parse() calls will trigger runtime deprecation warnings only; type definitions remain intact

TypeScript compilation should succeed without type errors given the codebase's API surface.


116-116: The ping npm package upgrade from 0.4.4 to 1.0.0 poses no functional risk to the codebase.

The codebase does not import or use the ping npm package. All references to "ping" in the code are internal RPC protocol implementations (e.g., this.client.node.ping() calls custom RPC methods, not the npm package). Therefore, breaking changes in ping v1.0.0—including promise/callback alignment and Node.js engine requirements—have zero impact on the application's functionality.

This is a dead dependency that can be safely upgraded.

Likely an incorrect or invalid review comment.


141-141: Let me extract the node-gyp 12.1.0 engine requirements directly from the lock file:

Let me search for the specific Node version requirements for npm 11 and node-gyp 12:

Based on my verification, I've confirmed that:

  1. node-gyp v12.0.0 aligns to npm 11's node engine range and includes major dependency bumps (gyp-next v0.21.0, env-paths, which, make-fetch-happen, nopt, proc-log).

  2. The repository's package.json has no engines field (currently null), meaning no explicit Node version requirement is enforced.

  3. The build:native script directly invokes node-gyp configure && node-gyp build with multiple native modules (binding.gyp files present).

  4. No CI configuration files or .nvmrc was found to indicate tested Node versions.


Verify node-gyp 12.1.0 compatibility before merging.

The upgrade from node-gyp 11.4.2 to 12.1.0 includes breaking changes to the Node engine range requirement. node-gyp v12.0.0 aligned to npm 11's node engine range and bumped dependencies (gyp-next, env-paths, which, make-fetch-happen, nopt, proc-log) that may affect environments and CI.

Since the repository has no engines field constraint, ensure:

  • Native builds (npm run build:native) execute successfully on all supported Node versions
  • Dependencies on specific node-gyp behaviors are still met
  • CI/deployment environments meet the new npm 11 node engine requirements

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.

❤️ Share

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

@liranmauda liranmauda force-pushed the liran-bump-deps branch 2 times, most recently from 393e07b to 5962edf Compare November 11, 2025 14:42
@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 11, 2025
@liranmauda liranmauda force-pushed the liran-bump-deps branch 2 times, most recently from 3920b72 to 2bbb03f Compare November 12, 2025 08:42
Copy link

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
package.json (1)

80-145: PR description incomplete; testing status unclear.

The PR description is an empty template, and testing instructions are not documented. For a dependency bump PR affecting 14 packages, explicit verification of test coverage and CI results is essential.

Please provide:

  1. Summary of why these specific versions were selected
  2. Confirmation that npm test passes completely
  3. Any breaking changes encountered and how they were addressed
  4. Links to CI test results (if available)
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3920b72 and 2bbb03f.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • package.json (4 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). (3)
  • GitHub Check: run-package-lock-validation
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
package.json (1)

134-134: Verify @types/node v24 compatibility; plan url.parse() refactoring.

The major version bump from v22.18.6 to v24.10.0 introduces runtime deprecation warnings: url.parse() is deprecated in Node.js 24, and the codebase uses this API extensively (14+ calls across multiple files including src/util/url_utils.js, src/util/signature_utils.js, src/util/http_utils.js, src/util/cloud_utils.js, and others). While not a breaking change yet, this will generate deprecation warnings in logs.

No breaking changes are expected from other Node.js 24 removals (e.g., tls.createSecurePair() is not used in the codebase).

Please confirm:

  1. TypeScript compilation passes without type errors
  2. CI tests completed successfully with @types/node v24.10.0
  3. Deprecation warnings from url.parse() are acceptable or plan refactoring to WHATWG URL API

Bumping deps to avoid CVE (13/11/2025)

Signed-off-by: liranmauda <liran.mauda@gmail.com>
@liranmauda liranmauda changed the title Test Bumping deps to avoid CVE (13/11/2025) Nov 13, 2025
@liranmauda liranmauda merged commit 477d3ae into noobaa:master Nov 13, 2025
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants