Adds firebase configuration #39
Conversation
WalkthroughThe changes update various project files. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as sendFirebase Route
participant F as Firebase Push Module
participant FA as Firebase Admin SDK
C->>R: POST /send-firebase with payload
R->>F: Call sendPushNotification()
F->>FA: Send notification message
FA-->>F: Return response or error
F-->>R: Return result
R-->>C: JSON response with status
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
routes/admin.js (1)
7-7:🧩 Analysis chain
Verify the removal of reminders functionality.
The removal of the reminders route and function might impact other parts of the codebase that depend on this functionality.
Run the following script to check for any remaining references to the removed functionality:
Also applies to: 25-31
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to list_reminders # Test: Search for references to list_reminders. Expect: No references except in comments. rg -A 5 'list_reminders' --type jsLength of output: 1519
Impact Detected: Reminders Functionality Still in Use
- The
list_remindersfunction is still implemented and exported inlib/database.js.- It’s actively referenced in
lib/reminders.js(e.g., withawait db.list_reminders(new Date())).- The commented-out code in
routes/admin.jsonly affects the admin endpoint and does not remove or disable the underlying reminders functionality.Please confirm if the intent was to disable reminders entirely. If so, additional cleanup in
lib/database.jsand in any modules (likelib/reminders.js) invoking this function is required.
🧹 Nitpick comments (4)
routes/health.js (1)
32-56: Refactor to use await consistently.The function is marked as async but still uses promise chaining. Consider refactoring to use await consistently for better readability and error handling.
Here's a suggested refactor:
async function healthCheck(_req, res, _next) { - return db.healthCheck().then(dbResult => { + try { + const dbResult = await db.healthCheck(); if (!dbResult) { console.error("Missing DB result"); return res.send(DB_UNREACHABLE); } - canReadFile(`certs/${process.env.APPLE_CERT_NAME}`).then((success) => { - if (success) { - return res.send(OK); - } else { - console.error("Failed to read cert"); - return res.send(CERT_UNREADABLE); - } - }).catch((error) => { + try { + const success = await canReadFile(`certs/${process.env.APPLE_CERT_NAME}`); + if (success) { + return res.send(OK); + } console.error("Failed to read cert"); - debug(error); return res.send(CERT_UNREADABLE); - }); - }).catch(err => { + } catch (error) { + console.error("Failed to read cert"); + debug(error); + return res.send(CERT_UNREADABLE); + } + } catch (err) { console.error("DB connection error"); debug(err); return res.status(500).send(DB_UNREACHABLE); - }); + } }lib/database.js (2)
30-46: Refactor to use await consistently.The function is marked as async but still uses promise chaining. Consider refactoring to use await and try/catch for better readability and error handling.
Here's a suggested refactor:
async function register_token(pub_key, token, platform, sandbox) { const q = ` INSERT INTO push_tokens (pub_key, token, platform, sandbox, updated_at) VALUES($1, $2, $3, $4, CURRENT_TIMESTAMP) ON CONFLICT ON CONSTRAINT index_pub_key_token DO UPDATE SET platform = $3, sandbox = $4, updated_at = CURRENT_TIMESTAMP `; debug(`Setting token for ${pub_key}`); - return pool.query(q, [pub_key.toLowerCase(), token, platform, sandbox]).then(res => { + try { + const res = await pool.query(q, [pub_key.toLowerCase(), token, platform, sandbox]); const success = res.rowCount > 0; if (!success) { throw "pub_key not added/updated."; } return true; - }); + } catch (error) { + throw error; + } }
52-143: Apply consistent async/await pattern across all functions.Multiple functions are marked as async but still use promise chaining. Consider applying the same refactoring pattern consistently across all functions for better readability and error handling.
Here's the pattern to follow for each function:
async function functionName(...args) { try { const res = await pool.query(q, [...args]); // Handle success case return result; } catch (error) { // Handle error case throw error; } }lib/push_notifications_firebase.js (1)
25-69: Enhance payload validation and error handling.While the function handles basic validation, consider these improvements:
- Validate payload structure more thoroughly
- Add type checking for critical fields
- Structure debug logs for better traceability
const sendPushNotification = async (deviceToken, payload) => { + // Validate payload structure + if (!payload || typeof payload !== 'object') { + throw new Error('Invalid payload format'); + } + const { title, pushType = "alert", sound = "default", body, expiry, topic, } = payload; + + // Validate required fields + if (!title || !body) { + throw new Error('Title and body are required'); + } // ... rest of the function try { const response = await admin.messaging().send(message); - debug(`Notification sent successfully: ${response}`); + debug({ + event: 'notification_sent', + response, + deviceToken: deviceToken || 'N/A', + topic: topic || 'N/A' + }); return response; } catch (error) { - debug(`Error sending notification: ${error.message}`); + debug({ + event: 'notification_error', + error: error.message, + deviceToken: deviceToken || 'N/A', + topic: topic || 'N/A' + }); throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)app.js(1 hunks)lib/database.js(6 hunks)lib/push_notifications_firebase.js(1 hunks)lib/reminders.js(0 hunks)package.json(1 hunks)routes/admin.js(1 hunks)routes/cancel_reminders.js(0 hunks)routes/health.js(1 hunks)routes/index.js(1 hunks)routes/register.js(0 hunks)routes/send.js(4 hunks)
💤 Files with no reviewable changes (3)
- routes/register.js
- routes/cancel_reminders.js
- lib/reminders.js
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- routes/index.js
- app.js
🔇 Additional comments (4)
routes/admin.js (1)
19-23: LGTM!Good practice to use
_prefix for unused parameters.lib/database.js (1)
16-23: LGTM!Good use of async/await pattern.
lib/push_notifications_firebase.js (1)
5-16: Validate environment variables before initialization.The Firebase configuration relies on multiple environment variables but doesn't validate their presence. Missing variables could cause runtime errors.
Add validation before initialization:
const firebaseConfig = { + // Validate required environment variables + if (!process.env.FIREBASE_PROJECT_ID || !process.env.FIREBASE_PRIVATE_KEY) { + throw new Error('Missing required Firebase configuration environment variables'); + } type: "service_account", auth_uri: "https://accounts.google.com/o/oauth2/auth",package.json (1)
13-13: Verify Firebase Admin SDK version compatibility and security.The Firebase Admin SDK version should be verified for compatibility with the Node.js version in use and checked for any security advisories.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for compatible Node.js versions and security advisories npm view [email protected] engines.node npm audit [email protected]Length of output: 433
Verify Firebase Admin SDK Dependency Compatibility and Security
- Node.js Compatibility: The output confirms that firebase-admin v13.0.2 requires Node.js version >=18. Please ensure that the Node.js version used in your project meets this requirement.
- Security Audit: The
npm auditwas not executed successfully due to the absence of a lockfile. To perform a proper security audit of the dependency, generate a lockfile by runningnpm i --package-lock-onlyand then re-runnpm audit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
migrations/0003-app-id-data.sql (1)
1-3: Consider indexing new columns if querying by user_id or app_id.If the application frequently searches or filters by these columns, adding indexes may significantly boost query performance, especially for large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.prettierignore(1 hunks).prettierrc(1 hunks)app.js(2 hunks)lib/database.js(2 hunks)migrations/0003-app-id-data.sql(1 hunks)package.json(1 hunks)routes/send.js(6 hunks)routes/sendFirebase.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .prettierignore
- .prettierrc
🚧 Files skipped from review as they are similar to previous changes (3)
- app.js
- routes/send.js
- lib/database.js
🔇 Additional comments (4)
routes/sendFirebase.js (3)
19-20: EnsureAPP_API_KEYis defined or imported.The code references
APP_API_KEY, but it is not declared in this file. Confirm that it’s either set as an environment variable or imported from a config file.Please verify or add the definition. For example, if using environment variables:
+ const { APP_API_KEY } = process.env; ... const msg = `${APP_API_KEY}${from_pub_key}${to_pub_key}`;
60-62: VerifyEXPIRES_AFTER_HOURSis defined.
EXPIRES_AFTER_HOURSis not declared in this file. Ensure its origin is properly set (e.g., environment variable or config file).
82-82: ConfirmREMINDER_PUSH_NOTIFICATIONS_ENABLEDusage.
REMINDER_PUSH_NOTIFICATIONS_ENABLEDis referenced but not declared. Verify it's properly imported or defined, otherwise reminders won't function as intended.package.json (1)
13-13: Dependencies look good; verify no version conflicts.Including
"firebase-admin"and"prettier"is aligned with the new features and formatting guidelines. Verify that these versions do not conflict with existing dependencies or require additional configuration.Also applies to: 20-21
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
routes/sendFirebase.js (2)
23-23: Use debug module instead of console.log.Replace console.log statements with the imported debug module for consistent logging.
- console.log('msg', msg); + debug('msg %s', msg); - console.log('Valid with new check'); + debug('Valid with new check');Also applies to: 27-27
90-90: Simplify success response.The double negation
!!successis unnecessary sincesuccessis already a boolean.- return res.json({ success: !!success }); + return res.json({ success });lib/database.js (2)
56-56: Improve error message in register_token.The current error message doesn't provide enough context about why the operation failed.
- throw 'pub_key not added/updated.'; + throw `Failed to register token for pub_key ${pub_key}. No rows were affected.`;
67-77: Use consistent async/await pattern.The functions are marked as async but still use .then(). Consider using consistent async/await pattern throughout for better readability.
Example for delete_token:
async function delete_token(pub_key, token) { const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`; debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`); - return pool.query(q, [pub_key.toLowerCase(), token]).then((res) => { - const success = res.rowCount > 0; - if (!success) { - throw `pub_key for token ${token.substring(0, 4)}... not deleted.`; - } - return true; - }); + const res = await pool.query(q, [pub_key.toLowerCase(), token]); + const success = res.rowCount > 0; + if (!success) { + throw `pub_key for token ${token.substring(0, 4)}... not deleted.`; + } + return true; }Also applies to: 84-98, 105-114, 121-129, 136-144, 146-151, 153-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/database.js(6 hunks)lib/push_notifications_firebase.js(1 hunks)lib/reminders.js(7 hunks)routes/register.js(3 hunks)routes/send.js(7 hunks)routes/sendFirebase.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/reminders.js
- routes/register.js
- routes/send.js
- lib/push_notifications_firebase.js
🔇 Additional comments (3)
routes/sendFirebase.js (2)
15-16: Include a leading slash in the route paths.Express route parameters typically require a leading slash. Without it, the router might not map the request URL as expected.
- firebaseRouter.use('/', check_signature); - firebaseRouter.post('/', sendFirebase); + firebaseRouter.use('/:to_pub_key', check_signature); + firebaseRouter.post('/:to_pub_key', sendFirebase);
43-43: Fix the check for empty token array.The current condition negates
tokenRowswhile expecting it to be an array. This won't correctly detect empty arrays.- if (!tokenRows && Array.isArray(tokenRows) && tokenRows.length === 0) { + if (!tokenRows || !Array.isArray(tokenRows) || tokenRows.length === 0) {lib/database.js (1)
22-24: Review the OR conditions in the SQL query.Using OR conditions for all fields might return unexpected results. Consider if AND conditions would be more appropriate for your use case.
const q = `SELECT token, sandbox from push_tokens - WHERE pub_key = $1 OR app_id = $2 OR user_id = $3`; + WHERE pub_key = $1 AND (app_id = $2 OR user_id = $3)`;
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
routes/sendFirebase.js (4)
45-47:⚠️ Potential issueFix the check for empty token array.
The current condition negates
tokenRowswhile expecting it to be an array, which won't correctly detect empty arrays.Apply this diff to fix the logic:
- if (!tokenRows && Array.isArray(tokenRows) && tokenRows.length === 0) { + if (!tokenRows || !Array.isArray(tokenRows) || tokenRows.length === 0) {
48-51:⚠️ Potential issueReturn error response on token retrieval failure.
The error is logged but no response is sent, which could leave the request hanging.
Apply this diff to fix error handling:
} catch (error) { console.error(`Failed to get device tokens for pub_key ${to_pub_key}`); console.error(error); + return res.status(500).json({ + success: false, + error: 'Failed to retrieve device tokens' + }); }
60-74:⚠️ Potential issueFix success flag handling in notification loop.
The success flag is set but never reset between iterations. If any notification succeeds, the overall result will be success, even if subsequent notifications fail.
Apply this diff to fix the logic:
try { + success = true; for (const { token, sandbox, pub_key } of tokenRows) { pubKey = pub_key; const service = sandbox ? sandbox_push_notifications : firebase_push_notifications; debug(`The send service is (sandbox=${sandbox})`); const sendResult = await service(token.trim(), payload); debug(`The sendResult of the notification service is ${sendResult}`); - success = true; + if (!sendResult) { + success = false; + break; + } }
85-91:⚠️ Potential issueSanitize error response.
Sending raw error objects in responses could expose sensitive information.
Apply this diff to sanitize the error:
if (!success) { console.error(error); return res.status(500).json({ success: false, - error + error: 'Failed to send notification' }); }
🧹 Nitpick comments (1)
lib/database.js (1)
67-158: Use consistent async/await pattern across all functions.The database operations mix promise chains with async/await. Consider refactoring all functions to use async/await consistently for better readability and error handling.
Example refactor for
delete_token:async function delete_token(pub_key, token) { const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`; debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`); - return pool.query(q, [pub_key.toLowerCase(), token]).then((res) => { - const success = res.rowCount > 0; - if (!success) { - throw `pub_key for token ${token.substring(0, 4)}... not deleted.`; - } - return true; - }); + const res = await pool.query(q, [pub_key.toLowerCase(), token]); + if (res.rowCount === 0) { + throw new Error(`Failed to delete token ${token.substring(0, 4)}...`); + } + return true; }Apply similar refactoring to other functions using promise chains.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/database.js(6 hunks)lib/push_notifications_firebase.js(1 hunks)routes/sendFirebase.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/push_notifications_firebase.js
🔇 Additional comments (1)
routes/sendFirebase.js (1)
1-16: LGTM! Route configuration is correct.The router is properly configured with root path ('/') since it's mounted at '/send-firebase' in app.js.
lib/database.js
Outdated
| return pool.query(q, [pub_key.toLowerCase()]).then(res => { | ||
| return res.rows; | ||
| }); | ||
| WHERE pub_key = $1 OR app_id = $2 OR user_id = $3`; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Revise the WHERE clause logic.
Using OR conditions for all identifiers could return unintended results. Consider using a more specific combination of identifiers based on your business logic.
🏁 Script executed:
#!/bin/bash
# Check if there's an index for this query
rg -A 5 'CREATE.*INDEX.*push_tokens'Length of output: 218
Revise the WHERE Clause Logic in Token Retrieval
-
The SQL in
lib/database.jsuses:WHERE pub_key = $1 OR app_id = $2 OR user_id = $3Using OR across these identifiers may return rows that match on just one of these fields, possibly leading to unintended results.
-
The migration file (
migrations/0001-initial.sql) shows an index forpub_key(and one forid) but no indexes forapp_idoruser_id. This discrepancy suggests that the query logic might not be aligned with the optimized database design or the intended business logic.
Please review and adjust the query to ensure it retrieves tokens as expected—potentially by revising the condition to better reflect the proper combination of identifiers (or by adding the necessary indexes if multiple identifiers are indeed required).
There was a problem hiding this comment.
Intended behaviour, one app_id or user_id might be registered on multiple devices
There was a problem hiding this comment.
@PanchoBubble Understood, thank you for the clarification about the token retrieval logic.
✏️ Learnings added
Learnt from: PanchoBubble
PR: tari-labs/ios_aurora_push#39
File: lib/database.js:24-24
Timestamp: 2025-02-12T13:00:34.669Z
Learning: In the push_tokens table, the WHERE clause `pub_key = $1 OR app_id = $2 OR user_id = $3` is intentionally designed to allow multiple devices to be registered with the same app_id or user_id, returning all matching rows based on any of these identifiers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
migrations/0003-app-id-data.sql (1)
1-3: Consider adding constraints and documentation.The column additions look good, but consider these improvements:
- Add column comments to document their purpose
- Consider adding foreign key constraints if these IDs reference other tables
- Consider adding NOT NULL constraints with DEFAULT values if these columns should never be null
Example improvement:
ALTER TABLE push_tokens -ADD COLUMN IF NOT EXISTS user_id VARCHAR(255), -ADD COLUMN IF NOT EXISTS app_id VARCHAR(255); +ADD COLUMN IF NOT EXISTS user_id VARCHAR(255) NOT NULL DEFAULT '', +ADD COLUMN IF NOT EXISTS app_id VARCHAR(255) NOT NULL DEFAULT '', +COMMENT ON COLUMN push_tokens.user_id IS 'Unique identifier for the user', +COMMENT ON COLUMN push_tokens.app_id IS 'Identifier for the Firebase application';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/database.js(6 hunks)migrations/0003-app-id-data.sql(1 hunks)routes/sendFirebase.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- routes/sendFirebase.js
- lib/database.js
🔇 Additional comments (1)
migrations/0003-app-id-data.sql (1)
5-6: LGTM! Verify query patterns for optimal indexing.The indexes are well-named and safely created. However, depending on your query patterns, you might benefit from a composite index if you frequently query using both columns together.
Let's check the query patterns in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for SQL queries using user_id and app_id columns rg -i "where.*user_id.*app_id|where.*app_id.*user_id" --type sql rg -i "select.*where.*user_id|select.*where.*app_id" --type jsLength of output: 131
Query Patterns Verification & Composite Index Consideration
The automated searches we ran did not yield any queries where both the
user_idandapp_idcolumns are used together. This suggests that, based on our current evidence, the existing single-column indexes appear appropriate. However, because dynamic queries or non-obvious composite conditions might exist, please manually verify any query-generation patterns in the application code to ensure no composite queries are overlooked.To further assist, you can run this additional script to search for files that contain both keywords:
🏁 Script executed:
[run_scripts] #!/bin/bash # List all files that contain both "user_id" and "app_id" rg -l "user_id" | xargs -r grep -li "app_id"Length of output: 212
Let's further investigate whether any composite query conditions exist in the application code (especially in
lib/database.js) that might benefit from a composite index. In addition to the earlier search, please run the following script to extract any context lines containing composite conditions (i.e. usage of bothuser_idandapp_idin the sameWHEREclause):If this search produces no output, it would confirm that the current single-column indexes are appropriate. Otherwise, if composite conditions are found, you might consider adding a composite index.
Please run this additional check and review the output.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for composite query conditions in lib/database.js..." rg -C5 -i "where.*user_id.*and.*app_id" lib/database.js rg -C5 -i "where.*app_id.*and.*user_id" lib/database.jsLength of output: 372
Query Patterns Verified – Single-Column Indexes Are Sufficient
The investigation into the codebase (including in
lib/database.js) did not reveal any composite query conditions involving bothuser_idandapp_id. Hence, the existing single-column indexes withIF NOT EXISTSare appropriate. Please manually verify if there are any dynamic query generators that might combine these columns, though current evidence suggests they are sufficient.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/database.js (1)
115-124: Simplify async functions by removing unnecessary promise chains.Several functions are marked as async but still use promise chains unnecessarily.
Apply these diffs to improve the implementations:
async function cancel_all_reminders(pub_key) { const q = ` DELETE FROM reminder_notifications WHERE pub_key = $1 `; debug(`Removing reminders for ${pub_key}`); - return pool.query(q, [pub_key.toLowerCase()]).then((_) => { - return true; - }); + await pool.query(q, [pub_key.toLowerCase()]); + return true; } async function list_reminders(send_at_before) { const q = ` SELECT id, pub_key, reminder_type, send_at FROM reminder_notifications WHERE send_at < $1`; - return pool.query(q, [send_at_before]).then((res) => { - return res.rows; - }); + const res = await pool.query(q, [send_at_before]); + return res.rows; } async function delete_reminder(id) { const q = ` DELETE FROM reminder_notifications WHERE id = $1 `; - return pool.query(q, [id]).then((_) => { - return true; - }); + await pool.query(q, [id]); + return true; } async function admin_list() { const q = `SELECT platform, updated_at from push_tokens`; - return pool.query(q, []).then((res) => { - return res.rows; - }); + const res = await pool.query(q, []); + return res.rows; } async function healthCheck() { const q = `SELECT 1`; - return pool.query(q, []).then((res) => { - return res.rowCount > 0; - }); + const res = await pool.query(q, []); + return res.rowCount > 0; }Also applies to: 131-139, 146-154, 156-161, 163-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/database.js(6 hunks)routes/send.js(6 hunks)routes/sendFirebase.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- routes/send.js
- routes/sendFirebase.js
🧰 Additional context used
🧠 Learnings (1)
lib/database.js (1)
Learnt from: PanchoBubble
PR: tari-labs/ios_aurora_push#39
File: lib/database.js:24-24
Timestamp: 2025-02-12T13:00:34.820Z
Learning: In the push_tokens table, the WHERE clause `pub_key = $1 OR app_id = $2 OR user_id = $3` is intentionally designed to allow multiple devices to be registered with the same app_id or user_id, returning all matching rows based on any of these identifiers.
🔇 Additional comments (4)
lib/database.js (4)
4-5: Review SSL configuration security implications.Setting
rejectUnauthorized: falsedisables SSL certificate validation, which could expose the application to man-in-the-middle attacks. In production, proper SSL certificates should be configured.Consider implementing proper certificate validation for production environments.
16-23: LGTM! Improved null handling and async pattern.The function has been properly converted to async/await and includes null safety for the pubKey parameter.
32-40: LGTM! Firebase token retrieval implementation.The function correctly implements the intended behavior of retrieving tokens based on either appId or userId, as confirmed by previous discussions.
53-71: LGTM! Improved error handling and parameter management.The function has been updated with:
- Proper async/await pattern
- Descriptive error message
- COALESCE for handling nullable app_id and user_id fields
| async function schedule_reminder(pub_key, reminder_type, send_at) { | ||
| const q = ` | ||
| INSERT INTO reminder_notifications (pub_key, reminder_type, send_at) | ||
| VALUES($1, $2, $3) | ||
| `; | ||
| debug(`Scheduling ${reminder_type} notification for ${pub_key}`); | ||
| return pool.query(q, [pub_key.toLowerCase(), reminder_type, send_at]).then(res => { | ||
| return pool.query(q, [pub_key?.toLowerCase(), reminder_type, send_at]).then((res) => { | ||
| const success = res.rowCount > 0; | ||
| if (!success) { | ||
| throw "reminder_not_scheduled"; | ||
| throw 'reminder_not_scheduled'; | ||
| } | ||
|
|
||
| return true | ||
| return true; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve async pattern consistency in schedule_reminder.
The function uses promise chains despite being marked as async.
Apply this diff to improve the implementation:
async function schedule_reminder(pub_key, reminder_type, send_at) {
const q = `
INSERT INTO reminder_notifications (pub_key, reminder_type, send_at)
VALUES($1, $2, $3)
`;
debug(`Scheduling ${reminder_type} notification for ${pub_key}`);
- return pool.query(q, [pub_key?.toLowerCase(), reminder_type, send_at]).then((res) => {
- const success = res.rowCount > 0;
- if (!success) {
- throw 'reminder_not_scheduled';
- }
-
- return true;
- });
+ const res = await pool.query(q, [pub_key?.toLowerCase(), reminder_type, send_at]);
+ if (res.rowCount === 0) {
+ throw new Error('Failed to schedule reminder');
+ }
+ return true;
}📝 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.
| async function schedule_reminder(pub_key, reminder_type, send_at) { | |
| const q = ` | |
| INSERT INTO reminder_notifications (pub_key, reminder_type, send_at) | |
| VALUES($1, $2, $3) | |
| `; | |
| debug(`Scheduling ${reminder_type} notification for ${pub_key}`); | |
| return pool.query(q, [pub_key.toLowerCase(), reminder_type, send_at]).then(res => { | |
| return pool.query(q, [pub_key?.toLowerCase(), reminder_type, send_at]).then((res) => { | |
| const success = res.rowCount > 0; | |
| if (!success) { | |
| throw "reminder_not_scheduled"; | |
| throw 'reminder_not_scheduled'; | |
| } | |
| return true | |
| return true; | |
| }); | |
| } | |
| async function schedule_reminder(pub_key, reminder_type, send_at) { | |
| const q = ` | |
| INSERT INTO reminder_notifications (pub_key, reminder_type, send_at) | |
| VALUES($1, $2, $3) | |
| `; | |
| debug(`Scheduling ${reminder_type} notification for ${pub_key}`); | |
| const res = await pool.query(q, [pub_key?.toLowerCase(), reminder_type, send_at]); | |
| if (res.rowCount === 0) { | |
| throw new Error('Failed to schedule reminder'); | |
| } | |
| return true; | |
| } |
| async function delete_token(pub_key, token) { | ||
| const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`; | ||
| debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`); | ||
| return pool.query(q, [pub_key.toLowerCase(), token]).then(res => { | ||
| return pool.query(q, [pub_key.toLowerCase(), token]).then((res) => { | ||
| const success = res.rowCount > 0; | ||
| if (!success) { | ||
| throw `pub_key for token ${token.substring(0, 4)}... not deleted.`; | ||
| } | ||
| return true | ||
| return true; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve async pattern consistency and error handling.
The function is marked as async but still uses promise chains. Also, the error message includes potentially sensitive token information.
Apply this diff to improve the implementation:
async function delete_token(pub_key, token) {
const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`;
debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`);
- return pool.query(q, [pub_key.toLowerCase(), token]).then((res) => {
- const success = res.rowCount > 0;
- if (!success) {
- throw `pub_key for token ${token.substring(0, 4)}... not deleted.`;
- }
- return true;
- });
+ const res = await pool.query(q, [pub_key.toLowerCase(), token]);
+ if (res.rowCount === 0) {
+ throw new Error('Failed to delete push token');
+ }
+ return true;
}📝 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.
| async function delete_token(pub_key, token) { | |
| const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`; | |
| debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`); | |
| return pool.query(q, [pub_key.toLowerCase(), token]).then(res => { | |
| return pool.query(q, [pub_key.toLowerCase(), token]).then((res) => { | |
| const success = res.rowCount > 0; | |
| if (!success) { | |
| throw `pub_key for token ${token.substring(0, 4)}... not deleted.`; | |
| } | |
| return true | |
| return true; | |
| }); | |
| } | |
| async function delete_token(pub_key, token) { | |
| const q = `DELETE FROM push_tokens WHERE pub_key = $1 AND token = $2`; | |
| debug(`Removing token ${token.substring(0, 4)}... for ${pub_key}`); | |
| const res = await pool.query(q, [pub_key.toLowerCase(), token]); | |
| if (res.rowCount === 0) { | |
| throw new Error('Failed to delete push token'); | |
| } | |
| return true; | |
| } |
https://www.loom.com/share/00f815219f1b446aa25fa3be9a57d960
Summary by CodeRabbit
.prettierignorefile to specify ignored paths for formatting..prettierrcconfiguration file for code formatting options.pnpm-lock.yamlto.gitignore.push_tokenstable in the database schema.