Feat/162 service restart notify - #2125
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesService restart notifications
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant BaseProvider
participant UnderlyingProvider
participant IncentiveBackend
BaseProvider->>UnderlyingProvider: serviceRestart(params)
BaseProvider->>IncentiveBackend: POST restart metadata when image, tag, or dockerCmd exists
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Action performedReview finished.
|
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: high
Summary:
This PR introduces a notification mechanism for service restarts. However, it contains a critical ReferenceError where dockerCmd is accessed without being declared in the local scope, which will fatally crash the serviceRestart method. Furthermore, there is an unused parameter in the new method and risky usage of process.env that may crash browser-based environments consuming this library.
Comments:
• [ERROR][bug] Critical: dockerCmd is not defined in the scope of serviceRestart. This will throw a ReferenceError and break the function. Assuming dockerCmd is a property of the optional params object, you should access it via params?.dockerCmd.
- if (dockerCmd !== undefined) {
- this.notifyIncentiveBackendServiceRestarted(nodeUri, serviceId, dockerCmd).catch(() => {})
+ if (params?.dockerCmd !== undefined) {
+ this.notifyIncentiveBackendServiceRestarted(nodeUri, serviceId, params.dockerCmd)Note: The trailing .catch(() => {}) can also be safely removed because notifyIncentiveBackendServiceRestarted wraps its logic in a try/catch and never rejects.
• [WARNING][style] The parameter nodeUri is declared but never used inside the notifyIncentiveBackendServiceRestarted method. You can safely remove it to keep the method signature clean.
- private async notifyIncentiveBackendServiceRestarted(
- nodeUri: OceanNode,
- serviceId: string,
- dockerCmd: string[]
- ): Promise<void> {
+ private async notifyIncentiveBackendServiceRestarted(
+ serviceId: string,
+ dockerCmd: string[]
+ ): Promise<void> {(Be sure to update the call site at line 899 to match the removed argument)..
• [WARNING][bug] ocean.js is a library often consumed in both Node.js and browser environments. Accessing process.env directly can throw a fatal ReferenceError in browsers where the global process object is undefined (unless specifically polyfilled by the consumer's bundler). It is much safer to guard this check.
- const incentiveBackendUrl = process.env.INCENTIVE_BACKEND_URL
+ const incentiveBackendUrl = typeof process !== 'undefined' ? process.env.INCENTIVE_BACKEND_URL : undefined![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/providers/BaseProvider.ts`:
- Around line 897-900: Update serviceRestart to extract dockerCmd from its
params before the notification block, then use that optional restart parameter
value in the existing undefined check and notifyIncentiveBackendServiceRestarted
call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38588806-6f93-477f-a2dd-4630eafc7c76
📒 Files selected for processing (1)
src/services/providers/BaseProvider.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR introduces a helpful feature to notify the incentive backend when a service is restarted with new parameters. The changes are largely well-implemented, correctly preventing URL injection and gracefully handling missing configuration. However, there are a few architectural and performance improvements to consider. Reading from process.env directly in an SDK can cause issues in browser environments, the fetch call is unawaited and missing the AbortSignal, and there is a redundant .catch() statement.
Comments:
• [WARNING][architecture] Directly accessing process.env inside an SDK (which may be used in browsers or non-Node environments) can cause reference errors or require specific build-time polyfills. It's generally a better practice to accept such configuration values through the class constructor (e.g., via a Config object injected into BaseProvider) rather than hardcoding environment variable reads.
• [WARNING][performance] The fetch request does not use the signal provided to the parent serviceRestart method. This means if the user aborts the restart operation, this fetch will keep running. Additionally, without a timeout, a hanging endpoint could cause resource leaks. Consider passing the AbortSignal down to this method and adding it to the fetch options.
- private async notifyIncentiveBackendServiceRestarted(
- serviceId: string,
- params: ServiceRestartParams
+ private async notifyIncentiveBackendServiceRestarted(
+ serviceId: string,
+ params: ServiceRestartParams,
+ signal?: AbortSignal
): Promise<void> {And in the fetch:
await fetch(`${baseUrl}/services/${encodeURIComponent(serviceId)}/restarted`, {
method: 'POST',
+ signal,
headers: { 'Content-Type': 'application/json' },• [INFO][style] The .catch(() => {}) here is redundant because the notifyIncentiveBackendServiceRestarted method already wraps its entire body in a try/catch and gracefully handles any errors without re-throwing. If this was added to satisfy a linter rule like no-floating-promises, consider using void this.notifyIncentiveBackendServiceRestarted(...) instead.
Additionally, because this call is 'fire-and-forget' (not awaited), if this code runs in a short-lived serverless environment (like AWS Lambda), the Node process might exit before the background fetch completes, causing the notification to be lost. If reliable delivery is required, you should await it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/providers/BaseProvider.ts (1)
482-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDestructure restart metadata in the helper.
Use parameter destructuring for
image,tag, anddockerCmdinstead of repeatedly accessingparams.*.Proposed refactor
private async notifyIncentiveBackendServiceRestarted( serviceId: string, - params: ServiceRestartParams + { image, tag, dockerCmd }: ServiceRestartParams ): Promise<void> { ... body: JSON.stringify({ - image: params.image, - tag: params.tag, - dockerCmd: params.dockerCmd + image, + tag, + dockerCmd })As per coding guidelines: “Use destructuring for imports and function parameters.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/providers/BaseProvider.ts` around lines 482 - 499, Update notifyIncentiveBackendServiceRestarted to destructure image, tag, and dockerCmd from the params parameter, then use those local values in the request body instead of accessing params.*.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/providers/BaseProvider.ts`:
- Around line 492-500: Update the restart notification fetch in BaseProvider to
store its response and validate response.ok; throw a status-only error for
non-2xx responses so the surrounding catch handles failed notifications and
prevents stale incentive state.
---
Nitpick comments:
In `@src/services/providers/BaseProvider.ts`:
- Around line 482-499: Update notifyIncentiveBackendServiceRestarted to
destructure image, tag, and dockerCmd from the params parameter, then use those
local values in the request body instead of accessing params.*.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b409f8e-bbbc-406f-9699-eec53dc7ae91
📒 Files selected for processing (1)
src/services/providers/BaseProvider.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
The PR adds a notification mechanism to the incentive backend when a service is restarted. The logic is generally sound and correctly executes as a fire-and-forget background task. However, the direct use of process.env in a library that supports browser environments poses a significant risk of runtime crashes. A fallback check for the process object is needed, or ideally, the configuration should be injected.
Comments:
• [ERROR][bug] Accessing process.env directly can cause ReferenceError: process is not defined in browser environments. If ocean.js is intended to be run in browsers without a bundler that polyfills process, you must verify that process exists before accessing its properties. Alternatively, consider passing the INCENTIVE_BACKEND_URL through the provider's configuration or constructor rather than relying on environment variables inside a core library class.
- const incentiveBackendUrl = process.env.INCENTIVE_BACKEND_URL
+ const incentiveBackendUrl = typeof process !== 'undefined' && process.env ? process.env.INCENTIVE_BACKEND_URL : undefined;• [INFO][security] Good use of encodeURIComponent on serviceId here. This prevents potential path injection vulnerabilities in the outgoing fetch URL.
• [INFO][style] The notifyIncentiveBackendServiceRestarted method has an internal try-catch block that catches and logs any errors without re-throwing them, which means its returned Promise will never reject. Because of this, the appended .catch(() => {}) here is redundant and can be safely removed.
- this.notifyIncentiveBackendServiceRestarted(serviceId, params).catch(() => {})
+ this.notifyIncentiveBackendServiceRestarted(serviceId, params)
3 participants
Fixes # .
Changes proposed in this PR:
Summary by CodeRabbit