Refactor time checker and notification functions for robustness #131
Conversation
|
Warning Rate limit exceeded@hmshuv has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo cloud functions were refactored: Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Cron
participant Checker as isTime-checker
participant DB as Appwrite DB
participant Notifier as Message-Notification
participant FCM as Firebase Messaging
Scheduler->>Checker: Trigger function
Checker->>DB: List upcomingRoom documents
DB-->>Checker: upcomingRoom list
Checker->>DB: Update room.isTime when within window
Checker-->>Scheduler: Success response
Note over Notifier,DB: Notification flow (separate trigger)
Notifier->>DB: Query subscribers for room
DB-->>Notifier: Subscriber docs (with tokens)
Notifier->>Notifier: Aggregate & deduplicate tokens
Notifier->>FCM: sendEachForMulticast(multicastMessage)
FCM-->>Notifier: Responses per token
Notifier-->>Caller: Completion JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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: 4
🧹 Nitpick comments (2)
functions/upcomingRoom-isTime-checker/src/main.js (2)
37-37: Add safety check for the isTime field.The code assumes
document["isTime"]exists and is a boolean, but doesn't validate this. If the field is missing or has an unexpected type, the comparison could behave unexpectedly.🔎 Proposed fix with safety check
-if (timeLeftInMinutes <= 5 && timeLeftInMinutes >= -5 && document["isTime"] === false) { +if (timeLeftInMinutes <= 5 && timeLeftInMinutes >= -5 && document["isTime"] !== true) {This change treats missing or falsy
isTimevalues the same asfalse, making the code more defensive.
50-56: Consider removing commented notification logic.The commented notification code is legacy and not executed. If this logic is no longer needed (as suggested by the PR objectives to clean up the codebase), consider removing it entirely rather than keeping it as commented code. If it's kept for reference, add a comment explaining why.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
functions/upcomingRoom-Message-Notification/src/main.jsfunctions/upcomingRoom-isTime-checker/src/main.js
🧰 Additional context used
🧬 Code graph analysis (2)
functions/upcomingRoom-Message-Notification/src/main.js (1)
functions/upcomingRoom-isTime-checker/src/main.js (4)
admin(2-2)client(11-11)sdk(1-1)database(12-12)
functions/upcomingRoom-isTime-checker/src/main.js (1)
functions/upcomingRoom-Message-Notification/src/main.js (3)
client(12-12)sdk(1-1)database(13-13)
🔇 Additional comments (9)
functions/upcomingRoom-isTime-checker/src/main.js (5)
10-18: LGTM! Good addition of error callback.The function signature now includes the
errorcallback, which enables structured error handling. The Appwrite client setup follows best practices with environment variable configuration.
20-24: LGTM! Proper error handling structure.The addition of try-catch wrapping the main logic aligns with the PR objectives for improved robustness and error handling.
59-61: LGTM! Clean success response.The success response is clear and appropriate for the function's purpose.
63-69: LGTM! Solid error handling.The error handling properly uses the
errorcallback for logging and returns an appropriate 500 status with error details. This aligns well with the PR objectives for improved error handling.
37-37: Clarify the intentional scope of late room activation in the design.The condition
timeLeftInMinutes >= -5is documented in the code comment as intentional ("Check if time is within +/- 5 minutes"), allowing rooms to activate up to 5 minutes past their scheduled start time. Since this cron function runs every 5 minutes, rooms may not be processed until 5 minutes after their scheduled time, making this threshold catch late-arriving checks.However, the README documentation does not explicitly mention support for late activation (it only states "less than or equal to 5 minutes"). Confirm whether this behavior aligns with product requirements:
- Is activating rooms 5 minutes past their scheduled time the intended behavior?
- If so, update the README to document the -5 minute grace period.
functions/upcomingRoom-Message-Notification/src/main.js (4)
33-47: Good implementation of array safety checks.The safety checks using
Array.isArray()before spreading tokens correctly address the issue objectives. This prevents runtime errors whenregistrationTokensorcreator_fcm_tokensare undefined or not arrays.
49-50: Token deduplication is correctly implemented.Using
Setfor deduplication is efficient and idiomatic. This addresses the PR objective to avoid sending duplicate notifications.
22-82: Overall structure improvements look good.The refactoring successfully achieves the PR objectives:
- Try-catch error handling is properly implemented
- Excessive debug logs are removed in favor of concise, contextual logging
varis replaced withconst- Safety checks and deduplication are correctly added
76-82: Theres.json()method in Appwrite Functions correctly supports the two-argument signatureres.json(data, statusCode). The 500 status code at line 81 will be set correctly. This pattern is documented in the official Appwrite Functions API and is used consistently throughout this codebase.Likely an incorrect or invalid review comment.
| priority: "high" | ||
|
|
||
| try { | ||
| const { roomId, payload } = JSON.parse(req.body); |
There was a problem hiding this comment.
Handle cases where req.body may already be parsed or undefined.
In Appwrite Functions, req.body can be a pre-parsed object (depending on content-type headers) or undefined. Calling JSON.parse() on an object throws "Unexpected token o in JSON", which is confusing.
🔎 Proposed fix
- const { roomId, payload } = JSON.parse(req.body);
+ const body = typeof req.body === 'string' ? JSON.parse(req.body) : req.body;
+ const { roomId, payload } = body;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { roomId, payload } = JSON.parse(req.body); | |
| const body = typeof req.body === 'string' ? JSON.parse(req.body) : req.body; | |
| const { roomId, payload } = body; |
🤖 Prompt for AI Agents
In functions/upcomingRoom-Message-Notification/src/main.js around line 23, avoid
blindly calling JSON.parse on req.body; instead check if req.body is undefined,
a string, or already an object. If undefined, set a safe default (e.g., {} or
return a 400); if typeof req.body === "string" parse it inside a try/catch and
handle parse errors (log and return an error response); if typeof req.body ===
"object" use it directly; finally validate that roomId and payload exist before
proceeding.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
functions/upcomingRoom-Message-Notification/src/main.js (1)
54-65: Message structure correctly addresses past feedback.The multicast message structure now:
- Removes the invalid top-level
priorityfield- Correctly sets
android.priorityfor Android devices- Adds
apnsheaders withapns-priority: "10"for iOS high-priority deliveryThis properly addresses the previous review comment.
Optional enhancement: Validate payload structure
Consider validating that
payloadcontains expected notification fields (e.g.,title,body) before building the message. This would provide clearer error messages if the payload is malformed:if (!payload.title || !payload.body) { throw new Error('Payload must contain title and body fields'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
functions/upcomingRoom-Message-Notification/src/main.jsfunctions/upcomingRoom-isTime-checker/src/main.js
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/upcomingRoom-isTime-checker/src/main.js
🧰 Additional context used
🧬 Code graph analysis (1)
functions/upcomingRoom-Message-Notification/src/main.js (1)
functions/upcomingRoom-isTime-checker/src/main.js (3)
client(11-11)sdk(1-1)database(12-12)
🔇 Additional comments (5)
functions/upcomingRoom-Message-Notification/src/main.js (5)
1-8: LGTM: Firebase Admin initialization is correct.The module-level initialization with service account credentials follows best practices. If the service account file is missing or invalid, the function will fail fast at load time, which is appropriate for cloud function deployments.
34-38: Excellent safety checks for array operations.The code properly validates that
registrationTokensexists and is an array before spreading, preventing potential runtime errors. This successfully addresses the PR objective for adding array safety checks.
40-48: Consistent safety checks applied.The same defensive pattern is correctly applied when collecting creator tokens, ensuring
creator_fcm_tokensexists and is an array before spreading. This consistency improves maintainability.
50-51: Token deduplication implemented correctly.Using a
Setfor deduplication is clean and efficient. This successfully addresses the PR objective to prevent duplicate notifications.
82-86: No issues found. Theres.json(payload, statusCode)syntax used in the error handler is the correct Appwrite Functions API for returning JSON responses with status codes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
functions/upcomingRoom-Message-Notification/src/main.js (5)
11-11: Consider usingletinstead ofconstfor semantically mutable arrays.While
constis technically correct (prevents reassignment but allows mutation), usingletforsubscribersTokenswould more clearly signal that the array contents will change throughout the function.🔎 Optional refactor
- const subscribersTokens = []; + let subscribersTokens = [];
26-30: Consider validating the structure ofpayloadand type ofroomId.The current validation ensures
roomIdandpayloadare present, but doesn't verify their structure or types. For Firebase Cloud Messaging,payloadshould typically containtitleandbodyfields, androomIdshould be a non-empty string.🔎 Proposed validation enhancement
if (!roomId || !payload) { return res.json({ message: 'Missing required fields', error: 'roomId and payload are required' }, 400); } + + if (typeof roomId !== 'string' || roomId.trim() === '') { + return res.json({ + message: 'Invalid roomId', + error: 'roomId must be a non-empty string' + }, 400); + } + + if (!payload.title || !payload.body) { + return res.json({ + message: 'Invalid payload', + error: 'payload must contain title and body fields' + }, 400); + }
41-45: Good array safety check implementation!The
Array.isArray()check before spreading tokens correctly addresses the PR objective. However, consider validating that individual tokens are non-empty strings to prevent invalid tokens from reaching Firebase Cloud Messaging.🔎 Optional enhancement to filter invalid tokens
subscriberList.documents.forEach(subscriber => { if (subscriber["registrationTokens"] && Array.isArray(subscriber["registrationTokens"])) { - subscribersTokens.push(...subscriber["registrationTokens"]); + const validTokens = subscriber["registrationTokens"].filter( + token => typeof token === 'string' && token.trim() !== '' + ); + subscribersTokens.push(...validTokens); } });
53-55: Array safety check correctly implemented.The guard condition mirrors the subscriber token collection pattern. As with subscriber tokens, consider filtering invalid tokens here as well.
🔎 Optional enhancement
if (roomDocument["creator_fcm_tokens"] && Array.isArray(roomDocument["creator_fcm_tokens"])) { - subscribersTokens.push(...roomDocument["creator_fcm_tokens"]); + const validTokens = roomDocument["creator_fcm_tokens"].filter( + token => typeof token === 'string' && token.trim() !== '' + ); + subscribersTokens.push(...validTokens); }
60-82: Multicast notification implementation looks solid.The message structure with Android and APNs high-priority configuration is correct, and
sendEachForMulticastis the appropriate method for sending to multiple tokens. The past review comment regarding the invalid top-levelpriorityfield has been properly addressed.Consider logging which specific tokens failed for easier debugging in production environments.
🔎 Optional enhancement for detailed failure logging
const response = await getMessaging(app).sendEachForMulticast(message); if (response.failureCount > 0) { - log(`Failed to send ${response.failureCount} notifications`); + const failedTokens = response.responses + .map((r, idx) => r.success ? null : uniqueTokens[idx]) + .filter(t => t !== null); + log(`Failed to send ${response.failureCount} notifications to tokens: ${failedTokens.join(', ')}`); } else { log('Notifications were sent successfully'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/upcomingRoom-Message-Notification/src/main.js
🧰 Additional context used
🧬 Code graph analysis (1)
functions/upcomingRoom-Message-Notification/src/main.js (1)
functions/upcomingRoom-isTime-checker/src/main.js (3)
client(11-11)sdk(1-1)database(12-12)
🔇 Additional comments (2)
functions/upcomingRoom-Message-Notification/src/main.js (2)
57-58: Excellent token deduplication implementation!Using
Setto deduplicate tokens is clean, efficient, and directly addresses the PR objective to prevent duplicate notifications.
88-94: Robust error handling implementation!The try-catch block with structured error logging and a clear 500 response effectively addresses the PR objective to improve error handling and robustness.
1 participant
Description
This PR refactors
upcomingRoom-isTime-checkerandupcomingRoom-Message-Notificationto improve code quality, robustness, and readability.Changes Made
upcomingRoom-isTime-checkernew Date()parsinglog("here"), etc.)vartoconst/lettry-catchblock for better error handlingupcomingRoom-Message-NotificationRelated Issue
Fixes #130
Checklist
Note: Ready for review! Happy to address any feedback or make additional improvements.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.