Implement push notifications for each activity #67
Conversation
WalkthroughThe changes in this pull request include modifications across several files to enhance the application's functionality, particularly in relation to Firebase messaging and push notifications. Key updates consist of adding a Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 8
🧹 Outside diff range and nitpick comments (12)
firebase.json (1)
22-35: LGTM! Consider using variables for common configurations.The addition of the "expense-notifications-functions" configuration looks good and aligns with the PR objective of implementing push notifications for each activity. The structure is correct and consistent with the existing configuration.
To reduce duplication and improve maintainability, consider using variables for common configurations like ignore patterns and predeploy commands. This could be achieved by moving Firebase configuration to a separate file (e.g.,
firebase.config.js) and using a build step to generate the finalfirebase.json.Example
firebase.config.js:const commonConfig = { ignore: [ "node_modules", ".git", "firebase-debug.log", "firebase-debug.*.log", "*.local" ], predeploy: [ "npm --prefix \"$RESOURCE_DIR\" run lint", "npm --prefix \"$RESOURCE_DIR\" run build" ] }; module.exports = { firestore: { rules: "firestore.rules", indexes: "firestore.indexes.json" }, functions: [ { source: "functions", codebase: "default", ...commonConfig }, { source: "expense-notifications-functions", codebase: "expense-notifications-functions", ...commonConfig } ] };You could then use a script to generate
firebase.jsonfrom this configuration file.expense-notifications-functions/package.json (2)
1-12: LGTM! Consider adding a test script.The package name and scripts are well-defined for a Firebase Functions project. The scripts cover essential development tasks, including linting, building, serving, and deploying.
Consider adding a
"test"script to run unit tests for your functions. This can be beneficial for maintaining code quality and catching issues early. For example:"test": "jest"Don't forget to install Jest as a devDependency if you decide to add this.
21-29: LGTM! Comprehensive set of development dependencies. Consider adding a testing framework.The devDependencies include all necessary tools for TypeScript development and linting, which is excellent for maintaining code quality. The versions used are relatively recent, ensuring access to the latest features and bug fixes.
Consider adding a testing framework to your devDependencies. Jest is a popular choice that works well with TypeScript. You can add it with:
npm install --save-dev jest @types/jest ts-jestThen, add a Jest configuration file (jest.config.js) to properly handle TypeScript:
module.exports = { preset: 'ts-jest', testEnvironment: 'node', };This will enable you to write and run unit tests for your Firebase Functions, further improving code quality and reliability.
functions/src/index.ts (5)
Line range hint
21-33: LGTM: Function declaration and initial data retrieval are well-implemented.The function is correctly declared as an async Cloud Function triggered on document writes. The retrieval of before and after states with null checks is a good practice for handling edge cases. The use of TypeScript for type assertions enhances code reliability.
Consider using optional chaining for a more concise null check:
if (!beforeData?.balances || !afterData?.balances) { logger.warn('Either beforeData or afterData is missing balances. Exiting function.'); return; }
Line range hint
35-46: Improve balance comparison and data fetching efficiency.While the overall logic is correct, there are two areas for improvement:
The balance comparison
beforeData.balances != afterData.balancesmight not work as expected for complex objects. Consider using a deep comparison or comparing stringified versions.Fetching user documents in a loop could be inefficient for large groups. Consider using a batch get operation.
Here's a suggested improvement:
import * as _ from 'lodash'; // ... if (!_.isEqual(beforeData.balances, afterData.balances)) { const olderBalances = beforeData.balances || []; const updatedBalances = afterData.balances || []; const userIds = updatedBalances.map(balance => balance.id); const userDocs = await db.collection('users').where(admin.firestore.FieldPath.documentId(), 'in', userIds).get(); for (const updatedBalance of updatedBalances) { const userId = updatedBalance.id; const userDoc = userDocs.docs.find(doc => doc.id === userId); // ... rest of the logic } }This change uses Lodash for deep comparison and fetches all user documents in a single query.
Line range hint
47-77: LGTM: Balance update logic is correct with good error handling.The logic for calculating the difference between old and new balances and updating the user's
total_owe_amountfield is implemented correctly. The use ofawaitfor asynchronous operations is appropriate.Consider adding more detailed error logging:
try { await userDocRef.update({ total_owe_amount: newTotalOweAmount, }); logger.info(`Successfully updated user ${userId}'s totalOweAmount.`); } catch (updateError) { logger.error(`Failed to update totalOweAmount for user ${userId}:`, updateError); }This will provide more context if an update operation fails.
Line range hint
78-83: LGTM: Overall error handling is well-implemented.The use of a try-catch block for the entire function is a good practice for catching unexpected errors. Logging errors is crucial for debugging and monitoring.
Consider adding more context to the error log:
} catch (error) { logger.error('Error updating user owe amounts:', error); logger.error('Error occurred for group:', event.params.groupId); }This additional context can help in identifying which group caused the error.
Line range hint
1-83: Overall: Well-implemented Cloud Function with room for minor improvements.The
onGroupWritefunction effectively updates user balances based on group changes. Key strengths include:
- Good use of TypeScript for type safety.
- Proper error handling and logging.
- Correct implementation of Firebase Cloud Functions.
Main areas for improvement:
- Optimize data fetching for better performance with large groups.
- Enhance balance comparison for more reliable object equality checks.
- Add more detailed error logging for easier debugging.
These improvements will further enhance the function's efficiency and maintainability.
Consider implementing a batching mechanism for updates if this function is expected to handle large groups frequently. This would help in managing Firestore write quotas and improving overall performance.
expense-notifications-functions/src/index.ts (4)
32-34: Enhance Notification Message ClarityTo improve user experience, consider revising the notification message for clarity. Removing unnecessary characters like the hyphen and ensuring the message is easily understandable can enhance readability.
Apply this diff to adjust the message:
- `- You owe ${...}` + `You owe ${...}`
50-50: Remove Unnecessary Inline CommentThe inline comment
// Adjusted to use deviceFcmTokenmay be redundant or outdated. Consider removing it to keep the code clean and maintainable.Apply this diff to remove the comment:
- if (userDoc.exists && userDoc.data()?.deviceFcmToken) { // Adjusted to use deviceFcmToken + if (userDoc.exists && userDoc.data()?.deviceFcmToken) {
64-64: Handle Missing FCM Token More GracefullyWhen an FCM token is not found for a user, consider implementing a strategy to prompt the user to register their device or update their settings, rather than just logging a warning.
40-40: Include Error Details in LogsWhen logging errors, include additional context to aid in debugging. For example, specify which expense ID or user ID was involved.
Apply this diff to enhance the error log:
- logger.error('Error in onExpenseCreated function:', error); + logger.error(`Error in onExpenseCreated function for expense ID ${event.params.expenseId}:`, error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
- .firebaserc (1 hunks)
- Data/Data/Model/AppUser.swift (3 hunks)
- Podfile (1 hunks)
- Splito.xcodeproj/project.pbxproj (1 hunks)
- Splito/AppDelegate.swift (1 hunks)
- expense-notifications-functions/.eslintrc.js (1 hunks)
- expense-notifications-functions/.gitignore (1 hunks)
- expense-notifications-functions/package.json (1 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
- expense-notifications-functions/tsconfig.dev.json (1 hunks)
- expense-notifications-functions/tsconfig.json (1 hunks)
- firebase.json (1 hunks)
- functions/src/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- .firebaserc
- expense-notifications-functions/.eslintrc.js
- expense-notifications-functions/.gitignore
- expense-notifications-functions/tsconfig.dev.json
- expense-notifications-functions/tsconfig.json
🧰 Additional context used
🔇 Additional comments (14)
expense-notifications-functions/package.json (4)
16-16: LGTM! Main entry point is correctly set for a TypeScript project.The main entry point is appropriately set to "lib/index.js", which is the standard output location for compiled TypeScript files. This configuration aligns well with best practices for TypeScript projects.
30-30: LGTM! Private field is correctly set.The private field is appropriately set to true, which is correct for a Firebase Functions project. This prevents accidental publication to the npm registry.
17-20: LGTM! Core Firebase dependencies are included. Consider checking for updates.The essential Firebase dependencies (firebase-admin and firebase-functions) are correctly included. The versions used are relatively recent, which is good for security and feature availability.
To ensure you're using the latest stable versions and to check for any breaking changes, please run the following script:
#!/bin/bash # Description: Check for updates in Firebase dependencies and any breaking changes echo "Checking for updates in Firebase dependencies..." npm outdated firebase-admin firebase-functions echo "Checking for breaking changes in recent versions..." npm view firebase-admin changelog | grep -i "breaking change" -A 5 npm view firebase-functions changelog | grep -i "breaking change" -A 5Review the output and consider updating if there are newer versions available, taking note of any breaking changes that might affect your code.
13-15: Verify Node.js version compatibility with Firebase Functions.The use of Node.js 18 is a good choice as it's an LTS version. However, it's crucial to ensure this version is fully supported by the Firebase Functions runtime.
Please run the following script to check the compatibility:
If the results show any incompatibilities or warnings, consider adjusting the Node.js version accordingly.
Data/Data/Model/AppUser.swift (4)
18-18: LGTM: New propertydeviceFcmTokenadded correctly.The new
deviceFcmTokenproperty is well-implemented. It's correctly declared as an optionalStringand follows the proper naming conventions.
38-38: LGTM: Initializer updated correctly fordeviceFcmToken.The initializer has been properly updated to include the new
deviceFcmTokenparameter. The default value ofnilmaintains backward compatibility, and the property is correctly set in the initializer body.Also applies to: 45-45
58-58: LGTM:CodingKeysenum updated correctly fordeviceFcmToken.The
CodingKeysenum has been properly updated with a new case fordeviceFcmToken. The JSON key "device_fcm_token" follows the expected snake_case convention for API communication.
18-18: Verify usage of updatedAppUserstruct in the codebase.The changes to
AppUserlook good. However, it's important to ensure that these changes are reflected whereverAppUserinstances are created or used throughout the codebase.Run the following script to check for potential places that might need updates:
Also applies to: 38-38, 45-45, 58-58
functions/src/index.ts (1)
Line range hint
1-19: LGTM: Imports and initialization are correct.The necessary Firebase Admin SDK and Cloud Functions modules are imported correctly. The Firebase app is initialized, and a Firestore instance is obtained. The use of TypeScript interfaces for
BalanceandUserDataenhances type safety.Splito/AppDelegate.swift (4)
17-17: AppDelegate conforms to necessary protocolsConforming to
MessagingDelegateandUNUserNotificationCenterDelegateallowsAppDelegateto handle messaging and notification events appropriately.
22-23: Messaging delegate assignment and push registration look goodSetting
Messaging.messaging().delegate = selfand callingregisterForPushNotificationscorrectly sets up messaging and notification handling.
27-36: Push notifications registration implemented correctlyThe
registerForPushNotificationsfunction correctly requests notification permissions and registers for remote notifications upon authorization.
40-42:⚠️ Potential issueEnsure APNSTokenType matches the environment
In
didRegisterForRemoteNotificationsWithDeviceToken, you're setting the APNS token type to.sandbox. Please verify that this matches your build environment. Use.sandboxfor development and.prodfor production to ensure proper delivery of push notifications.Run the following script to check if the build configuration is correctly set:
Splito.xcodeproj/project.pbxproj (1)
1353-1357:⚠️ Potential issuePotential Issue: Release Configuration Signed with Development Certificate
Lines 1353 to 1357 show that the
CODE_SIGN_IDENTITYhas been changed to"Apple Development",CODE_SIGN_STYLEhas been set toAutomatic, andDEVELOPMENT_TEAMis nowS985H2T7J8in the Release build configuration.This change might cause the Release build to be signed with a development certificate instead of a distribution certificate, which can lead to issues when distributing the app through the App Store. Typically, the Release configuration should use
"Apple Distribution"as theCODE_SIGN_IDENTITYandManualforCODE_SIGN_STYLEto ensure proper signing for distribution.Please verify if this change is intentional.
Would you like assistance in adjusting the code signing settings to align with the App Store distribution requirements?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
expense-notifications-functions/src/index.ts (2)
13-53: LGTM with a minor suggestion for clarity.The overall structure of the
onExpenseCreatedfunction looks good. It correctly handles the expense creation event and notifies the relevant users.Consider using
Array.prototype.filter()to make the exclusion of the expense creator more explicit:- for (const userId of splitToUsers) { - if (userId !== addedBy) { + for (const userId of splitToUsers.filter(id => id !== addedBy)) {This change would make the exclusion of the expense creator more obvious at first glance.
106-129: LGTM with a suggestion for cleaner code.The
sendNotificationfunction correctly handles sending notifications using FCM.Consider using optional chaining for a more concise check of the FCM token:
- if (userDoc.exists && userDoc.data()?.deviceFcmToken) { - const fcmToken = userDoc.data()?.deviceFcmToken; + if (userDoc.exists && userDoc.data()?.deviceFcmToken) { + const fcmToken = userDoc.data()?.deviceFcmToken;This change makes the code slightly cleaner while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 64-64: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (2)
expense-notifications-functions/src/index.ts (2)
3-10: LGTM: Imports and initialization look good.The imports and Firebase initialization are appropriate for the functionality of this Cloud Function.
1-129: Overall implementation looks good.The Cloud Function and its supporting functions are well-structured and cover the necessary functionality for expense notifications. The code handles different split types, calculates owed amounts correctly, and sends notifications appropriately.
Great job on implementing this feature. With the suggested minor improvements, this code will be even more robust and maintainable.
🧰 Tools
🪛 Biome
[error] 64-64: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
expense-notifications-functions/src/index.ts (3)
13-53: LGTM with a suggestion for improved loggingThe
onExpenseCreatedfunction is well-structured and handles the expense creation notification process effectively. It correctly extracts data, calculates owed amounts, and sends notifications to relevant users.Consider using a more structured logging approach for better traceability:
- logger.info(`Expense created notification sent successfully. Expense Data: ${JSON.stringify(expenseData)}`); + logger.info('Expense created notification sent successfully', { expenseId: event.params.expenseId, expenseData });This change will make it easier to parse and analyze logs in the future.
56-143: LGTM with a suggestion for improved code reusabilityThe
onExpenseUpdatedfunction effectively handles various scenarios when an expense is updated, including notifying users who remain in the split list, those removed, and new payers. The error handling is also well-implemented.Consider extracting the notification message creation logic into a separate function to improve code reusability:
function createNotificationMessage(expenseName: string, expenseAmount: number, owedAmount: number): string { let message = ''; if (owedAmount < 0) { message = `- You owe ₹${Math.abs(owedAmount).toFixed(2)}`; } else if (owedAmount > 0) { message = `- You get back ₹${owedAmount.toFixed(2)}`; } else { message = `- You owe ₹0.00`; } return `${expenseName} (₹${expenseAmount.toFixed(2)})\n${message}`; }You can then use this function in both
onExpenseCreatedandonExpenseUpdatedto create consistent notification messages.
198-222: LGTM with a suggestion for enhanced notification payloadThe
sendNotificationfunction effectively handles sending notifications using FCM, including retrieving the user's FCM token and proper error handling.Consider including a data payload in the notification for more flexibility:
const payload = { notification: { title, body, }, + data: { + expenseId: expenseData.id, + type: 'expense_update', + }, token: fcmToken, };This addition allows the client app to handle the notification more dynamically, potentially deep-linking to the specific expense or performing custom actions based on the notification type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Splito/AppDelegate.swift (1 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 154-155: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (3)
expense-notifications-functions/src/index.ts (2)
1-222: Overall, well-implemented expense notification system with room for minor improvementsThe implementation of Firebase Cloud Functions for expense notifications is robust and covers the necessary scenarios effectively. The code demonstrates good error handling, logical structuring, and use of Firebase services.
Key strengths:
- Comprehensive handling of expense creation and updates
- Proper use of Firebase Admin SDK and Cloud Functions
- Detailed calculation of owed amounts for different split types
Suggested improvements:
- Re-enable ESLint and address specific linting issues
- Enhance logging for better traceability
- Improve code reusability by extracting common logic
- Use
Object.hasOwn()for safer object property checking- Include a data payload in notifications for more flexible client-side handling
These improvements will further enhance the code quality, maintainability, and functionality of the expense notification system.
🧰 Tools
🪛 Biome
[error] 154-155: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
1-1:⚠️ Potential issueReconsider disabling ESLint for the entire file
Disabling ESLint for the entire file may mask important code quality issues. It's better to enable ESLint and address specific linting errors as they arise, or disable only the specific rules that are causing problems.
Consider removing the ESLint disable comment and addressing any specific linting issues:
- /* eslint-disable */If there are specific rules causing issues, you can disable them individually instead.
Likely invalid or redundant comment.
Splito/AppDelegate.swift (1)
42-42: Verify necessity of setting APNs token for Firebase AuthenticationThe call to
FirebaseProvider.auth.setAPNSToken(deviceToken, type: .sandbox)is typically used when implementing phone number authentication with silent push notifications via Firebase Authentication. If your application does not use Firebase Authentication for phone number verification, this line may not be necessary.Please verify if this call is required for your app's functionality. If it's not needed, you can safely remove it to simplify your code.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Splito/AppDelegate.swift (1 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 154-155: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (6)
Splito/AppDelegate.swift (6)
13-15: LGTM: Necessary imports addedThe new import statements for FirebaseMessaging, UserNotifications, and FirebaseFirestore are appropriate for the added functionality.
17-17: LGTM: Appropriate protocol conformances addedThe AppDelegate class now correctly conforms to MessagingDelegate and UNUserNotificationCenterDelegate, which are necessary for handling push notifications and Firebase messaging.
22-23: LGTM: Proper setup for push notifications and messagingThe changes to set up the messaging delegate and register for push notifications are correct and necessary for the new functionality.
27-36: LGTM: Well-implemented push notification registrationThe
registerForPushNotificationsmethod correctly sets up the notification center delegate, requests authorization, and registers for remote notifications. The use of the main queue for UI-related operations is appropriate.
44-66: LGTM: Proper handling of FCM token and Firestore updatesThe new methods for handling FCM token registration and Firestore updates are well-implemented. The use of
setData(merge: true)inupdateFCMTokenInFirestoreaddresses the concern raised in a previous review comment, ensuring that the document is created if it doesn't exist and thedeviceFcmTokenis updated appropriately.The error handling and logging are also implemented correctly.
Line range hint
1-66: Overall: Excellent implementation of push notifications and Firebase messagingThe changes in this file successfully implement push notifications and Firebase Cloud Messaging integration. The code is well-structured, follows best practices, and addresses previous review comments. Key improvements include:
- Proper setup of delegates and protocols
- Correct implementation of push notification registration
- Appropriate handling of FCM token updates in Firestore
- Good error handling and logging practices
These changes significantly enhance the app's capability to handle push notifications and integrate with Firebase services.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 218-218: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (2)
expense-notifications-functions/src/index.ts (2)
1-1:⚠️ Potential issueRemove the ESLint disable directive
Disabling ESLint for the entire file may mask important code quality issues. It's better to enable ESLint and address specific linting errors as they arise, or disable only the specific rules that are causing problems.
Apply this diff to remove the ESLint disable directive:
- /* eslint-disable */If there are specific ESLint rules causing issues, consider disabling them individually or addressing the underlying code quality concerns.
Likely invalid or redundant comment.
34-42: 🛠️ Refactor suggestionUse
Intl.NumberFormatfor currency formattingInstead of using
toFixed(2), consider usingIntl.NumberFormatfor proper currency formatting according to the user's locale. This provides better internationalization support and handles currency-specific formatting rules.Apply this diff to update the currency formatting:
- message = `- You owe ₹${Math.abs(owedAmount).toFixed(2)}`; + message = `- You owe ${new Intl.NumberFormat('en-IN', { style: 'currency', currency: 'INR' }).format(Math.abs(owedAmount))}`; - message = `- You get back ₹${owedAmount.toFixed(2)}`; + message = `- You get back ${new Intl.NumberFormat('en-IN', { style: 'currency', currency: 'INR' }).format(owedAmount)}`; - message = `- You owe ₹0.00`; + message = `- You owe ${new Intl.NumberFormat('en-IN', { style: 'currency', currency: 'INR' }).format(0)}`; - const body = `${expenseData.name} (₹${expenseData.amount.toFixed(2)})\n${message}`; + const body = `${expenseData.name} (${new Intl.NumberFormat('en-IN', { style: 'currency', currency: 'INR' }).format(expenseData.amount)})\n${message}`;Consider creating a helper function for currency formatting to avoid repetition throughout the code.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
expense-notifications-functions/src/index.ts (3)
1-1: Consider using specific ESLint disable commentsInstead of disabling ESLint for the entire file, it's better to use specific disable comments for particular rules or lines where needed. This ensures that other potential issues are still caught by ESLint.
Replace the global disable with specific disable comments where necessary. For example:
// eslint-disable-next-line @typescript-eslint/no-explicit-any const someAny: any = {};
25-26: Handle potential undefined split_to arrayThe current code assumes that
expenseData.split_tois always an array. If it's undefined, the||operator will assign an empty array, but it's better to be explicit about this.Consider using a more explicit check:
const splitToUsers = Array.isArray(expenseData.split_to) ? expenseData.split_to : [];This ensures that
splitToUsersis always an array, even ifexpenseData.split_tois undefined or not an array.
129-129: Handle potential undefined paid_by objectThe current code assumes that
expenseData.paid_byis always an object. If it's undefined, the||operator will assign an empty object, but it's better to be explicit about this.Consider using a more explicit check:
const paidByUsers = typeof expenseData.paid_by === 'object' && expenseData.paid_by !== null ? expenseData.paid_by : {};This ensures that
paidByUsersis always an object, even ifexpenseData.paid_byis undefined or not an object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (1)
expense-notifications-functions/src/index.ts (1)
1-253: Overall, well-structured implementation with room for minor improvementsThe implementation of Cloud Functions for handling expense notifications is comprehensive and well-structured. It covers the creation, update, and deletion of expenses, with appropriate notifications sent to relevant users.
Some areas for improvement include:
- More specific ESLint disabling
- Making the notification title configurable
- Explicit handling of potentially undefined values
- Refactoring to reduce repetition in the
onExpenseUpdatedfunction- Using
Object.hasOwn()instead ofhasOwnProperty- Including a data payload in notifications
Addressing these points will enhance the code's robustness, maintainability, and flexibility.
🧰 Tools
🪛 Biome
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
Data/Data/Model/Transaction.swift (1)
21-25: Consider default value forupdatedByin initializerThe addition of the
updatedByparameter to the initializer is correct and necessary. However, consider if it's always appropriate to require anupdatedByvalue when creating a new transaction.You might want to consider one of these options:
- Provide a default value for
updatedByin the initializer, such as setting it to the same value asaddedBy.- Create an additional convenience initializer that doesn't require
updatedBy, setting it toaddedByby default.This could make creating new transactions more convenient while still allowing explicit updates to be tracked.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (1)
161-166: LGTM: Transaction update processThe changes to the transaction update process look good. Setting the
updatedByfield, updating the transaction in the repository, and then updating the group member balance is a logical sequence of operations.One minor suggestion for improved clarity:
Consider adding a comment explaining why we're creating a new transaction object. For example:
// Create a new transaction object to avoid modifying the original var newTransaction = transaction newTransaction.updatedBy = userIdThis would make the intent clearer for future developers working on this code.
expense-notifications-functions/src/index.ts (8)
14-44: LGTM: onExpenseCreated function is well-implementedThe
onExpenseCreatedfunction correctly handles the creation of new expenses and sends notifications to relevant users. The logic for calculating owed amounts and generating messages is sound.However, consider adding a data payload to the notification for richer client-side handling:
const payload = { notification: { title, body }, data: { expenseId: event.params.expenseId, groupId: event.params.groupId, amount: expenseData.amount.toString(), type: 'expense_created' }, token: fcmToken };This would allow the client app to handle the notification more flexibly.
118-159: LGTM: onExpenseDeleted function is well-implementedThe
onExpenseDeletedfunction correctly handles the deletion of expenses and sends appropriate notifications to affected users. The logic for notifying users in the split list and those who paid but are not in the split list is sound.Consider adding a data payload to the notification similar to the suggestion for
onExpenseCreated:const payload = { notification: { title, body }, data: { expenseId: event.params.expenseId, groupId: event.params.groupId, amount: expenseData.amount.toString(), type: 'expense_deleted' }, token: fcmToken };This would allow for more flexible handling on the client side.
256-288: LGTM with suggestions: onTransactionCreated functionThe
onTransactionCreatedfunction correctly handles the creation of new transactions and sends notifications to relevant users. However, there are a few improvements that can be made:
- Consider using optional chaining for safer property access:
const payerName = await getUserDisplayName(transactionData?.payer_id); const receiverName = await getUserDisplayName(transactionData?.receiver_id);
- The condition for determining the message could be simplified:
const isThirdPartyAdded = transactionData.added_by && transactionData.added_by !== transactionData.receiver_id && transactionData.added_by !== transactionData.payer_id; const receiverMessage = `${payerName} paid you ${formatCurrency(transactionData.amount)}`; const payerMessage = isThirdPartyAdded ? `You paid ${receiverName} ${formatCurrency(transactionData.amount)}` : undefined;
- Consider adding a data payload to the notification for richer client-side handling, similar to the expense notifications.
These changes will make the function more robust and easier to maintain.
336-362: LGTM: onTransactionDeleted function is well-implementedThe
onTransactionDeletedfunction correctly handles the deletion of transactions and sends appropriate notifications to affected users. The logic for notifying both the payer and receiver is sound.Consider these minor improvements:
- Use optional chaining for safer property access:
const payerName = await getUserDisplayName(deletedTransactionData?.payer_id); const receiverName = await getUserDisplayName(deletedTransactionData?.receiver_id);
- Add a data payload to the notification for richer client-side handling:
const payload = { notification: { title, body }, data: { transactionId: event.params.transactionId, groupId: event.params.groupId, amount: deletedTransactionData.amount.toString(), type: 'transaction_deleted' }, token: fcmToken };These changes will make the function more robust and provide more flexibility for client-side handling.
386-413: LGTM: onGroupDeleted function is well-implementedThe
onGroupDeletedfunction correctly handles the deletion of groups and sends notifications to all group members. The logic is sound and error handling is in place.Consider these minor improvements:
- Use optional chaining for safer property access:
const members = deletedGroupData?.members || [];
- Add a data payload to the notification for richer client-side handling:
const payload = { notification: { title, body }, data: { groupId: event.params.groupId, groupName: deletedGroupData.name, type: 'group_deleted' }, token: fcmToken };These changes will make the function more robust and provide more flexibility for client-side handling.
416-446: LGTM with suggestions: onMemberRemoved functionThe
onMemberRemovedfunction correctly handles the removal of members from a group and sends notifications to the removed members. The logic is sound, but there are a few improvements that can be made:
- Use optional chaining and nullish coalescing for safer property access:
const oldMembers: string[] = oldGroupData?.members ?? []; const newMembers: string[] = newGroupData?.members ?? [];
- Consider notifying remaining group members about the removal:
for (const memberId of newMembers) { if (memberId !== newGroupData.createdBy) { const message = `${removedMembers.join(', ')} ${removedMembers.length > 1 ? 'were' : 'was'} removed from the group "${newGroupData.name}".`; await sendNotification(memberId, notificationTitle, message); } }
- Add a data payload to the notification for richer client-side handling:
const payload = { notification: { title, body }, data: { groupId: event.params.groupId, groupName: newGroupData.name, type: 'member_removed' }, token: fcmToken };These changes will make the function more robust, informative, and provide more flexibility for client-side handling.
162-176: LGTM: Helper functions are well-implementedThe
generateNotificationMessageandformatCurrencyfunctions are correctly implemented and provide good utility for the main functions. The use ofIntl.NumberFormatfor currency formatting is a good practice for internationalization.Consider this minor improvement for
generateNotificationMessage:function generateNotificationMessage(owedAmount: number): string { const formattedAmount = formatCurrency(Math.abs(owedAmount)); return owedAmount < 0 ? `- You owe ${formattedAmount}` : owedAmount > 0 ? `- You get back ${formattedAmount}` : `- You owe ${formatCurrency(0)}`; }This change makes the function more concise and reduces repetition.
229-383: LGTM with suggestions: sendNotification and getUserDisplayName functionsBoth
sendNotificationandgetUserDisplayNamefunctions are well-implemented. However, there are a few improvements that can be made:
- In
sendNotification, use optional chaining for safer property access:const fcmToken = userData?.deviceFcmToken;
- In
getUserDisplayName, consider using nullish coalescing operator for a more concise return statement:return userData?.first_name ?? 'Unknown';
- For
sendNotification, consider adding retry logic for failed notifications:const maxRetries = 3; let retries = 0; while (retries < maxRetries) { try { await admin.messaging().send(payload); logger.info(`Notification sent to user: ${userId}`); break; } catch (error) { retries++; if (retries === maxRetries) { logger.error(`Failed to send notification to user: ${userId} after ${maxRetries} attempts`, error); } else { logger.warn(`Retrying to send notification to user: ${userId}. Attempt ${retries}`); } } }These changes will make the functions more robust and resilient to potential errors.
🧰 Tools
🪛 Biome
[error] 375-375: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Data/Data/Model/Transaction.swift (2 hunks)
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (2 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 375-375: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
Data/Data/Model/Transaction.swift (2)
17-17: LGTM: Addition ofupdatedBypropertyThe new
updatedByproperty is a good addition to theTransactionsstruct. It follows the existing coding style and visibility conventions, and will be useful for tracking transaction updates.
35-35: LGTM: Addition ofupdatedBytoCodingKeysThe addition of the
updatedBycase to theCodingKeysenum is correct and consistent with the existing code. It properly maps to the snake_case "updated_by" for JSON serialization, maintaining the established convention.Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (3)
136-136: LGTM: Tracking transaction updatesThe addition of the
updatedByparameter in theTransactionsobject creation is a good improvement. It allows tracking of who last updated the transaction, which can be useful for auditing and user accountability.
157-158: LGTM: User verification before updateGood addition of a user ID check before proceeding with the transaction update. This ensures that we have a valid user ID to set as the
updatedByfield, maintaining data integrity.
Line range hint
136-166: Summary: Improved transaction trackingThe changes in this file enhance the transaction tracking capabilities of the application. By adding the
updatedByfield to transactions and ensuring it's set during creation and updates, we now have a clear audit trail of who last modified each transaction. This improvement aligns well with the PR objective of implementing push notifications for each activity, as it provides the necessary data to potentially notify relevant users about transaction updates.The implementation is solid, with appropriate checks and a logical flow of operations. These changes should improve the overall robustness and accountability of the transaction management system.
expense-notifications-functions/src/index.ts (2)
1-11: LGTM: Imports and initialization look goodThe imports and initialization of Firebase Admin SDK are correctly implemented. The use of
getFirestore()andonDocumentCreated,onDocumentUpdated, andonDocumentDeletedfrom Firebase Functions v2 is appropriate for this use case.
1-446: Overall, the implementation is solid with room for minor improvementsThe Cloud Functions in this file are well-structured and cover the necessary functionality for handling expense, transaction, and group-related notifications. The code demonstrates good practices in error handling and logging.
Key strengths:
- Comprehensive coverage of various scenarios (create, update, delete) for expenses, transactions, and groups.
- Good use of helper functions to modularize the code.
- Proper error handling and logging throughout the functions.
Suggestions for improvement:
- Enhance type safety by using more type assertions and optional chaining.
- Refactor some of the longer functions (e.g.,
onExpenseUpdated,onTransactionUpdated) for better readability and maintainability.- Add data payloads to notifications for richer client-side handling.
- Implement retry logic for failed notifications in the
sendNotificationfunction.- Replace
hasOwnPropertywithObject.hasOwn()as suggested by the static analysis tool.Implementing these suggestions will further improve the robustness and maintainability of the code. Great work overall!
🧰 Tools
🪛 Biome
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 375-375: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| exports.onExpenseUpdated = onDocumentUpdated( | ||
| { document: 'groups/{groupId}/expenses/{expenseId}' }, | ||
| async (event) => { | ||
| try { | ||
| const oldExpenseData = event.data?.before?.data(); // Data before the update | ||
| const newExpenseData = event.data?.after?.data(); // Data after the update | ||
|
|
||
| if (!oldExpenseData || !newExpenseData) { | ||
| logger.warn('No data found for the updated expense.'); | ||
| return; | ||
| } | ||
|
|
||
| const splitToUsers = newExpenseData.split_to || []; | ||
| const addedBy = newExpenseData.added_by; | ||
|
|
||
| // Notify users who remain in the split list if their owed amount has changed | ||
| for (const userId of splitToUsers) { | ||
| if (userId !== addedBy) { | ||
| const oldOwedAmount = calculateOwedAmount(oldExpenseData, userId); | ||
| const newOwedAmount = calculateOwedAmount(newExpenseData, userId); | ||
|
|
||
| // Notify only if the user's owed or payback amount has changed | ||
| if (oldOwedAmount !== newOwedAmount) { | ||
| const message = generateNotificationMessage(newOwedAmount); | ||
| const body = `Expense updated: ${newExpenseData.name} (${formatCurrency(newExpenseData.amount)})\n${message}`; | ||
|
|
||
| await sendNotification(userId, notificationTitle, body); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Notify users who were removed from the expense split list | ||
| const oldSplitUsers = oldExpenseData.split_to || []; | ||
| for (const userId of oldSplitUsers) { | ||
| if (!splitToUsers.includes(userId) && userId !== addedBy) { | ||
| const body = `Expense updated: ${oldExpenseData.name} (${formatCurrency(oldExpenseData.amount)})\n- You do not owe anything`; | ||
|
|
||
| await sendNotification(userId, notificationTitle, body); | ||
| } | ||
| } | ||
|
|
||
| // Notify users added as payers but not in the split list | ||
| const newPayers = newExpenseData.paid_by || {}; | ||
| for (const userId of Object.keys(newPayers)) { | ||
| if (!splitToUsers.includes(userId) && userId !== addedBy) { | ||
| const owedAmount = calculateOwedAmount(newExpenseData, userId); | ||
| const message = generateNotificationMessage(owedAmount); | ||
| const body = `Expense updated: ${newExpenseData.name} (${formatCurrency(newExpenseData.amount)})\n${message}`; | ||
|
|
||
| await sendNotification(userId, notificationTitle, body); | ||
| } | ||
| } | ||
|
|
||
| // Notify payers removed from the expense | ||
| const oldPayers = oldExpenseData.paid_by || {}; | ||
| for (const userId of Object.keys(oldPayers)) { | ||
| if (!Object.keys(newPayers).includes(userId) && !splitToUsers.includes(userId) && userId !== addedBy) { | ||
| const body = `Expense updated: ${oldExpenseData.name} (${formatCurrency(oldExpenseData.amount)})\n- You do not owe anything`; | ||
|
|
||
| await sendNotification(userId, notificationTitle, body); | ||
| } | ||
| } | ||
|
|
||
| logger.info(`Expense updated notification sent successfully. Expense Data: ${JSON.stringify(newExpenseData)}`); | ||
| } catch (error) { | ||
| logger.error('Error in onExpenseUpdated function:', error); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor suggestion: Simplify onExpenseUpdated function
The onExpenseUpdated function correctly handles various scenarios when an expense is updated. However, the function is quite long and could benefit from some refactoring to improve readability and maintainability.
Consider creating helper functions for different notification scenarios:
async function notifyUserOfExpenseChange(userId: string, oldExpenseData: any, newExpenseData: any) {
const oldOwedAmount = calculateOwedAmount(oldExpenseData, userId);
const newOwedAmount = calculateOwedAmount(newExpenseData, userId);
if (oldOwedAmount !== newOwedAmount) {
const message = generateNotificationMessage(newOwedAmount);
const body = `Expense updated: ${newExpenseData.name} (${formatCurrency(newExpenseData.amount)})\n${message}`;
await sendNotification(userId, notificationTitle, body);
}
}
async function notifyUserRemovedFromExpense(userId: string, expenseData: any) {
const body = `Expense updated: ${expenseData.name} (${formatCurrency(expenseData.amount)})\n- You do not owe anything`;
await sendNotification(userId, notificationTitle, body);
}Then use these helper functions in the main onExpenseUpdated function to simplify the logic:
for (const userId of splitToUsers) {
if (userId !== addedBy) {
await notifyUserOfExpenseChange(userId, oldExpenseData, newExpenseData);
}
}
for (const userId of oldSplitUsers) {
if (!splitToUsers.includes(userId) && userId !== addedBy) {
await notifyUserRemovedFromExpense(userId, oldExpenseData);
}
}This refactoring will make the function more concise and easier to maintain.
| exports.onTransactionUpdated = onDocumentUpdated( | ||
| { document: 'groups/{groupId}/transactions/{transactionId}' }, | ||
| async (event) => { | ||
| try { | ||
| const oldTransactionData = event.data?.before.data(); // Data before the update | ||
| const newTransactionData = event.data?.after.data(); // Data after the update | ||
|
|
||
| if (!oldTransactionData || !newTransactionData) { | ||
| logger.warn('No data found for the updated transaction.'); | ||
| return; | ||
| } | ||
|
|
||
| // Only notify if the amount has changed | ||
| if (oldTransactionData !== newTransactionData) { | ||
| const payerName = await getUserDisplayName(newTransactionData.payer_id); | ||
| const receiverName = await getUserDisplayName(newTransactionData.receiver_id); | ||
|
|
||
| let receiverMessage; | ||
| let payerMessage; | ||
| if (newTransactionData.updated_by && newTransactionData.updated_by !== newTransactionData.receiver_id && newTransactionData.updated_by !== newTransactionData.payer_id) { | ||
| receiverMessage = `Payment updated: ${payerName} paid you ${formatCurrency(newTransactionData.amount)}`; // Notify the receiver that the payer has made a payment | ||
| payerMessage = `Payment updated: you paid ${receiverName} ${formatCurrency(newTransactionData.amount)}`; // Notify the payer that the someone has made a payment | ||
| } else if (newTransactionData.updated_by && newTransactionData.updated_by == newTransactionData.receiver_id) { | ||
| payerMessage = `You paid ${receiverName} ${formatCurrency(newTransactionData.amount)}`; // Notify the payer that the someone has made a payment | ||
| } else if (newTransactionData.updated_by && newTransactionData.updated_by == newTransactionData.payer_id) { | ||
| receiverMessage = `${payerName} paid you ${formatCurrency(newTransactionData.amount)}`; // Notify the receiver that the payer has made a payment | ||
| } | ||
|
|
||
| // Send notifications | ||
| if (receiverMessage) { | ||
| await sendNotification(newTransactionData.receiver_id, notificationTitle, receiverMessage); | ||
| } | ||
| if (payerMessage) { | ||
| await sendNotification(newTransactionData.payer_id, notificationTitle, payerMessage); | ||
| } | ||
|
|
||
| logger.info(`Transaction updated notification sent successfully. Updated Data: ${JSON.stringify(newTransactionData)}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Error in onTransactionUpdated function:', error); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor suggestion: Simplify onTransactionUpdated function
The onTransactionUpdated function correctly handles updates to transactions and sends notifications to relevant users. However, the function can be simplified and made more robust:
- The condition
if (oldTransactionData !== newTransactionData)is always true for object comparisons. Instead, check for specific changes:
if (oldTransactionData.amount !== newTransactionData.amount) {
// ... notification logic
}- The message generation logic can be simplified using a helper function:
function generateTransactionMessage(payerName: string, receiverName: string, amount: number, isUpdate: boolean): { receiverMessage: string, payerMessage: string } {
const action = isUpdate ? 'Payment updated:' : '';
return {
receiverMessage: `${action} ${payerName} paid you ${formatCurrency(amount)}`,
payerMessage: `${action} You paid ${receiverName} ${formatCurrency(amount)}`
};
}
// Usage
const { receiverMessage, payerMessage } = generateTransactionMessage(payerName, receiverName, newTransactionData.amount, true);- Consider adding a data payload to the notification for richer client-side handling, similar to the expense notifications.
These changes will make the function more concise, easier to maintain, and less prone to errors.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (1)
246-252: Approve changes with a minor suggestion.The modifications to the
deleteGroupmethod are well-implemented and improve the overall robustness of the code. The addition of the guard statement foruserIdenhances safety, while creating a mutable copy of the group before updating it is a good practice.Consider adding a comment explaining why we're creating a mutable copy of the group, as it might not be immediately obvious to other developers.
Here's a suggested modification:
guard let group, let userId = preference.user?.id else { return } do { currentViewState = .loading // Create a mutable copy to update the 'updatedBy' field without modifying the original group var deletedGroup = group deletedGroup.updatedBy = userId try await groupRepository.deleteGroup(group: deletedGroup) NotificationCenter.default.post(name: .deleteGroup, object: deletedGroup) currentViewState = .initial goBackToGroupList() } catch { currentViewState = .initial showToastForError() }Splito/UI/Home/Groups/GroupListViewModel.swift (1)
293-298: LGTM! Consider adding a comment for clarity.The changes improve traceability by associating the deletion action with a specific user. Creating a mutable copy of the group ensures that the original object remains unchanged, which is a good practice.
Consider adding a brief comment explaining why we're creating a mutable copy of the group:
// Create a mutable copy of the group to update the 'updatedBy' field without modifying the original object var deletedGroup = group deletedGroup.updatedBy = userIdexpense-notifications-functions/src/index.ts (4)
1-1: Consider using more targeted ESLint disablesInstead of disabling ESLint for the entire file, it's better to use more targeted disables for specific rules or line ranges where necessary. This approach helps maintain code quality while allowing exceptions where needed.
Replace the global ESLint disable with specific rule disables as needed throughout the file. For example:
// eslint-disable-next-line @typescript-eslint/no-explicit-any function exampleFunction(param: any) { // Function implementation }
14-44: LGTM! Consider adding a data payload to the notificationThe
onExpenseCreatedfunction is well-implemented, with proper error handling and use of helper functions. To enhance the notification's functionality, consider adding a data payload.Add a data payload to the notification to provide additional context:
const body = `${expenseData.name} (${formatCurrency(expenseData.amount)})\n${message}`; -await sendNotification(userId, notificationTitle, body); +await sendNotification(userId, notificationTitle, body, { + expenseId: event.params.expenseId, + groupId: event.params.groupId, + type: 'expense_created' +});Update the
sendNotificationfunction signature accordingly.
47-115: LGTM! Consider refactoring for improved maintainabilityThe
onExpenseUpdatedfunction is well-implemented and handles various scenarios comprehensively. To improve maintainability, consider refactoring the repeated notification logic.Create a helper function for sending notifications to reduce code duplication:
async function notifyUser(userId: string, expenseData: any, message: string) { const body = `Expense updated: ${expenseData.name} (${formatCurrency(expenseData.amount)})\n${message}`; await sendNotification(userId, notificationTitle, body, { expenseId: event.params.expenseId, groupId: event.params.groupId, type: 'expense_updated' }); }Use this helper function throughout the
onExpenseUpdatedfunction to simplify the code.
370-444: LGTM! Consider adding consistent data payloads to group notificationsThe group-related functions (
onGroupUpdatedandonGroupDeleted) are well-implemented with proper error handling and logging.For consistency with other notifications, consider adding a data payload to the group notifications:
-await sendNotification(memberId, notificationTitle, notificationMessage); +await sendNotification(memberId, notificationTitle, notificationMessage, { + groupId: event.params.groupId, + type: 'group_updated' +}); // Similarly for group deletion -await sendNotification(memberId, notificationTitle, message); +await sendNotification(memberId, notificationTitle, message, { + groupId: event.params.groupId, + type: 'group_deleted' +});Update the
sendNotificationfunction to handle this additional data parameter throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- Data/Data/Model/Groups.swift (2 hunks)
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (2 hunks)
- Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (1 hunks)
- Splito/UI/Home/Groups/GroupListViewModel.swift (1 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 360-360: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
Data/Data/Model/Groups.swift (4)
16-16: LGTM: Addition ofupdatedBypropertyThe new
updatedByproperty is a good addition to track the user who last updated the group. It's correctly defined as a non-optionalStringand marked aspublicfor external access.
24-28: LGTM: Updated initializer withupdatedByparameterThe initializer has been correctly updated to include the new
updatedByparameter. The placement and initialization of the new parameter are appropriate, and the changes maintain backward compatibility with existing code.
41-41: LGTM: AddedupdatedBytoCodingKeysenumThe
CodingKeysenum has been correctly updated to include theupdatedByproperty. The mapping to "updated_by" follows the established naming convention and ensures proper JSON serialization.
Line range hint
16-41: Consider data migration and code updates for the newupdatedByfieldThe addition of the
updatedByfield improves group update tracking. However, please consider the following:
- Existing code that creates or modifies
Groupsobjects may need to be updated to provide theupdatedByvalue.- A data migration strategy might be necessary for existing stored
Groupsdata to include the newupdatedByfield.To help identify potential areas that need updates, you can run the following script:
Would you like assistance in creating a data migration plan or identifying areas of the codebase that need updates?
✅ Verification successful
Data Migration and Code Updates Verified
All instances of
Groupsobject creation include theupdatedByfield as intended. No additional changes are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Group object creations or modifications rg --type swift 'Groups\(' -A 5Length of output: 7859
expense-notifications-functions/src/index.ts (1)
1-471: Overall assessment: Well-implemented with room for improvementsThe Cloud Functions implementation for handling expenses, transactions, and group activities is comprehensive and well-structured. The code demonstrates good error handling and logging practices. However, there are several areas where the code could be improved:
- More targeted ESLint disables
- Consistent use of data payloads in notifications
- Refactoring of repeated logic, especially in transaction-related functions
- Improved type safety in some functions
- Updating
hasOwnPropertyusage toObject.hasOwn()Addressing these points will enhance the maintainability, consistency, and robustness of the codebase.
🧰 Tools
🪛 Biome
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 360-360: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
119-119: SettingupdatedByto the current userAssigning
userIdtoupdatedByensures accurate tracking of the user who updated the group. This is important for maintaining proper update records.
| receiverMessage = `${payerName} paid you ${formatCurrency(transactionData.amount)}`; // Notify the receiver that the payer has made a payment | ||
| } | ||
|
|
||
| await sendNotification(transactionData.receiver_id, notificationTitle, receiverMessage); | ||
| if (payerMessage) { | ||
| await sendNotification(transactionData.payer_id, notificationTitle, payerMessage); | ||
| } | ||
|
|
||
| logger.info(`Transaction created notification sent successfully. Transaction Data: ${JSON.stringify(transactionData)}`); | ||
| } catch (error) { | ||
| logger.error('Error in onTransactionCreated function:', error); | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| // Cloud Function to handle update transaction and notify users | ||
| exports.onTransactionUpdated = onDocumentUpdated( | ||
| { document: 'groups/{groupId}/transactions/{transactionId}' }, | ||
| async (event) => { | ||
| try { | ||
| const oldTransactionData = event.data?.before.data(); // Data before the update | ||
| const newTransactionData = event.data?.after.data(); // Data after the update | ||
|
|
||
| if (!oldTransactionData || !newTransactionData) { | ||
| logger.warn('No data found for the updated transaction.'); | ||
| return; | ||
| } | ||
|
|
||
| // Only notify if the transaction has changed | ||
| if (oldTransactionData !== newTransactionData) { | ||
| const payerName = await getUserDisplayName(newTransactionData.payer_id); | ||
| const receiverName = await getUserDisplayName(newTransactionData.receiver_id); | ||
|
|
||
| let receiverMessage; | ||
| let payerMessage; | ||
|
|
||
| if (newTransactionData.updated_by && newTransactionData.updated_by !== newTransactionData.receiver_id && newTransactionData.updated_by !== newTransactionData.payer_id) { | ||
| payerMessage = `Payment updated: you paid ${receiverName} ${formatCurrency(newTransactionData.amount)}`; // Notify the payer that the someone has updated a payment | ||
| receiverMessage = `Payment updated: ${payerName} paid you ${formatCurrency(newTransactionData.amount)}`; // Notify the receiver that the someone has updated a payment | ||
| } else if (newTransactionData.updated_by && newTransactionData.updated_by == newTransactionData.receiver_id) { | ||
| payerMessage = `Payment updated: You paid ${receiverName} ${formatCurrency(newTransactionData.amount)}`; // Notify the payer that the receiver has updated a payment | ||
| } else if (newTransactionData.updated_by && newTransactionData.updated_by == newTransactionData.payer_id) { | ||
| receiverMessage = `Payment updated: ${payerName} paid you ${formatCurrency(newTransactionData.amount)}`; // Notify the receiver that the payer has updated a payment | ||
| } | ||
|
|
||
| if (receiverMessage) { | ||
| await sendNotification(newTransactionData.receiver_id, notificationTitle, receiverMessage); | ||
| } | ||
| if (payerMessage) { | ||
| await sendNotification(newTransactionData.payer_id, notificationTitle, payerMessage); | ||
| } | ||
|
|
||
| logger.info(`Transaction updated notification sent successfully. Updated Data: ${JSON.stringify(newTransactionData)}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Error in onTransactionUpdated function:', error); | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| // Cloud Function to handle delete transaction and notify users | ||
| exports.onTransactionDeleted = onDocumentDeleted( | ||
| { document: 'groups/{groupId}/transactions/{transactionId}' }, | ||
| async (event) => { | ||
| try { | ||
| const deletedTransactionData = event.data?.data(); | ||
|
|
||
| if (!deletedTransactionData) { | ||
| logger.warn('No data found for the deleted transaction.'); | ||
| return; | ||
| } | ||
|
|
||
| const payerName = await getUserDisplayName(deletedTransactionData.payer_id); | ||
| const receiverName = await getUserDisplayName(deletedTransactionData.receiver_id); | ||
|
|
||
| let receiverMessage; | ||
| let payerMessage; | ||
|
|
||
| if (deletedTransactionData.updated_by && deletedTransactionData.updated_by !== deletedTransactionData.receiver_id && deletedTransactionData.updated_by !== deletedTransactionData.payer_id) { | ||
| payerMessage = `Payment deleted: you paid ${receiverName} ${formatCurrency(deletedTransactionData.amount)}`; // Notify the payer that the someone has deleted a payment | ||
| receiverMessage = `Payment deleted: ${payerName} paid you ${formatCurrency(deletedTransactionData.amount)}`; // Notify the receiver that the someone has deleted a payment | ||
| } else if (deletedTransactionData.updated_by && deletedTransactionData.updated_by == deletedTransactionData.receiver_id) { | ||
| payerMessage = `Payment deleted: You paid ${receiverName} ${formatCurrency(deletedTransactionData.amount)}`; // Notify the payer that the receiver has deleted a payment | ||
| } else if (deletedTransactionData.updated_by && deletedTransactionData.updated_by == deletedTransactionData.payer_id) { | ||
| receiverMessage = `Payment deleted: ${payerName} paid you ${formatCurrency(deletedTransactionData.amount)}`; // Notify the receiver that the payer has deleted a payment | ||
| } | ||
|
|
||
| if (receiverMessage) { | ||
| await sendNotification(deletedTransactionData.receiver_id, notificationTitle, receiverMessage); | ||
| } | ||
| if (payerMessage) { | ||
| await sendNotification(deletedTransactionData.payer_id, notificationTitle, payerMessage); | ||
| } | ||
|
|
||
| logger.info(`Transaction deleted notification sent successfully. Deleted Data: ${JSON.stringify(deletedTransactionData)}`); | ||
| } catch (error) { | ||
| logger.error('Error in onTransactionDeleted function:', error); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor transaction-related functions for better maintainability
The transaction-related functions (onTransactionCreated, onTransactionUpdated, and onTransactionDeleted) are comprehensive but contain repeated logic for message generation.
Consider creating a helper function to generate transaction messages:
function generateTransactionMessage(action: 'created' | 'updated' | 'deleted', payerName: string, receiverName: string, amount: number, updatedBy: string | null): { receiverMessage: string, payerMessage: string | null } {
const actionText = action === 'created' ? '' : `Payment ${action}:`;
const baseMessage = `${actionText} ${payerName} paid you ${formatCurrency(amount)}`;
if (updatedBy && updatedBy !== receiverName && updatedBy !== payerName) {
return {
receiverMessage: baseMessage,
payerMessage: `${actionText} You paid ${receiverName} ${formatCurrency(amount)}`
};
} else if (updatedBy === receiverName) {
return {
receiverMessage: baseMessage,
payerMessage: `${actionText} You paid ${receiverName} ${formatCurrency(amount)}`
};
} else {
return {
receiverMessage: baseMessage,
payerMessage: null
};
}
}Use this helper function in all transaction-related functions to reduce code duplication and improve maintainability.
| async function sendNotification(userId: string, title: string, body: string) { | ||
| try { | ||
| const userDoc = await db.collection('users').doc(userId).get(); | ||
| const userData = userDoc.data(); | ||
|
|
||
| if (userDoc.exists && userData?.deviceFcmToken) { | ||
| const fcmToken = userData?.deviceFcmToken; | ||
|
|
||
| const payload = { | ||
| notification: { | ||
| title, | ||
| body, | ||
| }, | ||
| token: fcmToken, | ||
| }; | ||
|
|
||
| await admin.messaging().send(payload); | ||
| logger.info(`Notification sent to user: ${userId}`); | ||
| } else { | ||
| logger.warn(`No FCM token found for user: ${userId}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Error sending notification:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance sendNotification function with data payload and improved error handling
The sendNotification function is well-implemented, but it can be improved to handle data payloads and provide more specific error handling.
Consider the following improvements:
- Add a data payload parameter:
-async function sendNotification(userId: string, title: string, body: string) {
+async function sendNotification(userId: string, title: string, body: string, data?: Record<string, string>) {
// ...
const payload = {
notification: {
title,
body,
},
+ data,
token: fcmToken,
};
// ...
}- Improve error handling:
try {
// ... existing code ...
} catch (error) {
if (error instanceof admin.messaging.FirebaseMessagingError) {
logger.error(`FCM Error for user ${userId}: ${error.code} - ${error.message}`);
} else {
logger.error(`Unexpected error sending notification to user ${userId}:`, error);
}
}These changes will make the function more flexible and provide better error diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (18)
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (3)
125-126: Approved, with a suggestion for improvementSetting
updatedBybefore updating the expense is good for tracking the last modifier. However, updating the expense immediately before deletion could potentially lead to a race condition if multiple users are interacting with the same expense simultaneously.Consider using a transaction or a single atomic operation to update and delete the expense, if your backend supports it. This would ensure data consistency and prevent potential race conditions.
129-129: Approved, with a suggestion for error handlingPosting the notification after the expense is updated and deleted ensures that the most current information is communicated. This is a good improvement.
However, consider wrapping this notification in a conditional statement to ensure it's only posted if the deletion is successful. This would prevent misleading notifications in case the deletion operation fails.
Example:
if await expenseRepository.deleteExpense(groupId: groupId, expenseId: expenseId) { NotificationCenter.default.post(name: .deleteExpense, object: expense) // ... success handling } else { // ... error handling }
120-129: Overall improvements in expense deletion processThe changes in this file enhance the robustness and accuracy of the expense deletion process. The addition of guard clauses, updating of the expense before deletion, and posting of notifications with up-to-date information all contribute to a more reliable system.
These modifications align well with the PR objective of implementing push notifications for each activity, as they ensure that the data being communicated is accurate and up-to-date.
To further improve the code:
- Consider using a transaction for updating and deleting the expense to prevent race conditions.
- Implement conditional notification posting based on the success of the deletion operation.
These suggestions would further enhance the reliability and consistency of the expense management system.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (2)
133-133: Remove trailing whitespaceThere is unnecessary trailing whitespace on this line. While it doesn't affect functionality, removing it improves code cleanliness and adheres to SwiftLint recommendations.
Apply this change:
- +🧰 Tools
🪛 SwiftLint
[Warning] 133-133: Lines should not have trailing whitespace
(trailing_whitespace)
137-137: LGTM: Consistent transaction creationThe addition of
updatedBy: userIdin theTransactionsinitializer ensures that new transactions are created with theupdatedByproperty set. This change is consistent with the update scenario and improves transaction tracking.For better code consistency, consider removing the
addedByparameter if it's no longer used in theTransactionsstruct.If
addedByis indeed unused, apply this change:- let transaction = Transactions(payerId: payerId, receiverId: receiverId, addedBy: userId, - updatedBy: userId, amount: amount, date: .init(date: paymentDate)) + let transaction = Transactions(payerId: payerId, receiverId: receiverId, + updatedBy: userId, amount: amount, date: .init(date: paymentDate))Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
285-291: LGTM! Consider adding a guard statement for user ID.The introduction of
userIdand its usage in bothhandleUpdateExpenseActionandhandleAddExpenseActionimproves consistency in tracking user actions. This is a good change.For added robustness, consider using a guard statement to handle the case where
userIdis nil:guard let userId = preference.user?.id else { showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: "User ID not found. Please log in again.")) completion(false) return }This would provide a clear error message and early return if the user ID is not available for any reason.
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
Line range hint
1-458: Summary of changes in GroupHomeViewModelThe main changes in this file revolve around the expense deletion process. While the modifications improve safety with the addition of a guard clause and maintain asynchronous execution, there are opportunities for further optimization:
- The
showExpenseDeleteAlertmethod now callsdeleteExpensesynchronously, which could potentially block the UI thread.- The
deleteExpensemethod has been changed from async to synchronous, but its content is still wrapped in a Task.- The expense deletion process now includes an additional step of updating the expense before deletion, which may be unnecessary and could lead to race conditions.
Consider simplifying the deletion process and ensuring that it doesn't negatively impact UI responsiveness. Additionally, improve error handling and logging throughout the class to enhance debugging and user feedback.
expense-notifications-functions/src/index.ts (9)
1-1: Consider using specific ESLint disable directivesInstead of disabling ESLint for the entire file, it's better to use specific disable directives for individual rules or line ranges where necessary. This ensures that other potential issues are still caught by ESLint.
14-44: LGTM: Well-structured Cloud Function for expense creationThe
onExpenseCreatedfunction is well-implemented, with proper error handling and use of helper functions. However, there's one area for improvement:Consider obfuscating sensitive information when logging expense data:
- logger.info(`Expense created notification sent successfully. Expense Data: ${JSON.stringify(expenseData)}`); + logger.info(`Expense created notification sent successfully. Expense ID: ${event.params.expenseId}`);This change prevents potentially sensitive financial information from being logged while still providing useful debugging information.
47-115: Well-implemented function with room for improvementThe
onExpenseUpdatedfunction is comprehensive and handles various scenarios well. However, there are a few areas for improvement:
- Consider obfuscating sensitive information when logging:
- logger.info(`Expense updated notification sent successfully. Expense Data: ${JSON.stringify(newExpenseData)}`); + logger.info(`Expense updated notification sent successfully. Expense ID: ${event.params.expenseId}`);
- The function is quite long and complex. Consider refactoring it into smaller, more focused helper functions to improve readability and maintainability. For example:
async function notifyUpdatedUsers(oldData: any, newData: any, updatedBy: string) { // Logic for notifying users who remain in the split list } async function notifyRemovedUsers(oldData: any, newData: any, updatedBy: string) { // Logic for notifying users removed from the split list } async function notifyNewPayers(oldData: any, newData: any, updatedBy: string) { // Logic for notifying new payers } async function notifyRemovedPayers(oldData: any, newData: any, updatedBy: string) { // Logic for notifying removed payers } exports.onExpenseUpdated = onDocumentUpdated( { document: 'groups/{groupId}/expenses/{expenseId}' }, async (event) => { try { const oldExpenseData = event.data?.before?.data(); const newExpenseData = event.data?.after?.data(); if (!oldExpenseData || !newExpenseData) { logger.warn('No data found for the updated expense.'); return; } const updatedBy = newExpenseData.updated_by; await notifyUpdatedUsers(oldExpenseData, newExpenseData, updatedBy); await notifyRemovedUsers(oldExpenseData, newExpenseData, updatedBy); await notifyNewPayers(oldExpenseData, newExpenseData, updatedBy); await notifyRemovedPayers(oldExpenseData, newExpenseData, updatedBy); logger.info(`Expense updated notification sent successfully. Expense ID: ${event.params.expenseId}`); } catch (error) { logger.error('Error in onExpenseUpdated function:', error); } } );This refactoring would make the main function more readable and easier to maintain.
118-159: LGTM: Well-implemented expense deletion handlerThe
onExpenseDeletedfunction is well-structured and handles the deletion scenario comprehensively. It correctly notifies relevant users and includes proper error handling. However, there's one minor improvement to consider:Consider obfuscating sensitive information when logging:
- logger.info(`Expense deletion notification sent successfully. Expense Data: ${JSON.stringify(expenseData)}`); + logger.info(`Expense deletion notification sent successfully. Expense ID: ${event.params.expenseId}`);This change prevents potentially sensitive financial information from being logged while still providing useful debugging information.
162-226: Well-implemented helper functions with minor improvementsThe helper functions are well-structured and follow the single responsibility principle. However, there are a few areas for improvement:
- In the
calculateOwedAmountfunction, replacehasOwnPropertywithObject.hasOwn()for better practice:- if (expenseData.paid_by.hasOwnProperty(memberId)) { + if (Object.hasOwn(expenseData.paid_by, memberId)) {
- Consider adding more specific type annotations to improve type safety:
function calculateOwedAmount(expenseData: { paid_by: Record<string, number>; split_to: string[]; }, memberId: string): number { // ... function body ... } function getTotalSplitAmountOf(expenseData: { split_to: string[]; split_type: 'equally' | 'fixedAmount' | 'percentage' | 'shares'; split_data: Record<string, number>; amount: number; }, member: string): number { // ... function body ... }These changes will improve type safety and follow best practices for object property checking.
🧰 Tools
🪛 Biome
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
228-347: Well-implemented transaction functions with room for improvementThe transaction-related functions are comprehensive, but there are several areas for improvement:
- Consider creating a helper function for generating transaction messages to reduce code duplication:
function generateTransactionMessage(action: 'created' | 'updated' | 'deleted', payerName: string, receiverName: string, amount: number, updatedBy: string | null): { receiverMessage: string, payerMessage: string | null } { const actionText = action === 'created' ? '' : `Payment ${action}:`; const baseMessage = `${actionText} ${payerName} paid you ${formatCurrency(amount)}`; if (updatedBy && updatedBy !== receiverName && updatedBy !== payerName) { return { receiverMessage: baseMessage, payerMessage: `${actionText} You paid ${receiverName} ${formatCurrency(amount)}` }; } else if (updatedBy === receiverName) { return { receiverMessage: baseMessage, payerMessage: `${actionText} You paid ${receiverName} ${formatCurrency(amount)}` }; } else { return { receiverMessage: baseMessage, payerMessage: null }; } }
- In the
onTransactionUpdatedfunction, the conditionif (oldTransactionData !== newTransactionData)will always be true for object comparisons. Instead, check for specific changes:if (oldTransactionData.amount !== newTransactionData.amount || oldTransactionData.payer_id !== newTransactionData.payer_id || oldTransactionData.receiver_id !== newTransactionData.receiver_id) { // ... notification logic ... }
- Consider adding a data payload to the notification for richer client-side handling:
const payload = { notification: { title: notificationTitle, body: message }, data: { type: 'transaction', action: 'created', // or 'updated', 'deleted' transactionId: event.params.transactionId, amount: transactionData.amount.toString() }, token: fcmToken };These changes will improve code maintainability, fix the object comparison issue, and provide more context in the notifications.
349-368: Well-implemented function with suggestions for improvementThe
getUserDisplayNamefunction is well-structured with proper error handling. However, there are a few areas for improvement:
- Use the
loggerinstead ofconsole.errorfor consistency:- console.error('Invalid userId:', userId); + logger.error('Invalid userId:', userId); - console.error('Error retrieving user name:', error); + logger.error('Error retrieving user name:', error);
- Consider implementing caching to reduce database reads:
const userNameCache = new Map<string, string>(); async function getUserDisplayName(userId: string) { if (!userId) { logger.error('Invalid userId:', userId); return 'Unknown'; } if (userNameCache.has(userId)) { return userNameCache.get(userId)!; } try { const userDoc = await db.collection('users').doc(userId).get(); if (userDoc.exists) { const userData = userDoc.data(); const name = userData && userData.first_name ? userData.first_name : 'Unknown'; userNameCache.set(userId, name); return name; } else { userNameCache.set(userId, 'Unknown'); return 'Unknown'; } } catch (error) { logger.error('Error retrieving user name:', error); return 'Unknown'; } }This caching mechanism will significantly reduce the number of database reads for repeated user name lookups.
🧰 Tools
🪛 Biome
[error] 360-360: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
371-415: Well-implemented group update handler with room for improvementThe
onGroupUpdatedfunction is comprehensive and handles different update scenarios well. However, it could benefit from some refactoring to improve readability and maintainability:Consider breaking down the function into smaller, more focused helper functions:
async function notifyNameChange(oldName: string, newName: string, updatedBy: string, members: string[]) { const updaterName = await getUserDisplayName(updatedBy); const notificationMessage = `${updaterName} changed the group name to "${newName}".`; for (const memberId of members) { if (updatedBy !== memberId) { await sendNotification(memberId, notificationTitle, notificationMessage); } } } async function notifyMemberRemovals(oldMembers: string[], newMembers: string[], groupName: string, removedBy: string) { const removedMembers = oldMembers.filter(memberId => !newMembers.includes(memberId)); if (removedMembers.length > 0) { const removerName = await getUserDisplayName(removedBy); for (const memberId of removedMembers) { const message = `${removerName} removed you from the group "${groupName}".`; await sendNotification(memberId, notificationTitle, message); } } } exports.onGroupUpdated = onDocumentUpdated( { document: 'groups/{groupId}' }, async (event) => { try { const oldGroupData = event.data?.before.data(); const newGroupData = event.data?.after.data(); if (!oldGroupData || !newGroupData) { logger.warn('No data found for the updated group.'); return; } if (oldGroupData.name !== newGroupData.name) { await notifyNameChange(oldGroupData.name, newGroupData.name, newGroupData.updated_by, newGroupData.members); } await notifyMemberRemovals(oldGroupData.members, newGroupData.members, newGroupData.name, newGroupData.created_by); logger.info(`Group update notifications sent successfully for group ${event.params.groupId}.`); } catch (error) { logger.error('Error in onGroupUpdated function:', error); } } );This refactoring improves readability and makes the main function easier to understand at a glance.
447-471: Well-implemented notification sender with room for improvementThe
sendNotificationfunction is well-structured and correctly handles the process of sending notifications via FCM. However, there are a few areas for improvement:
- Enhance error handling to provide more specific error messages:
try { // ... existing code ... } catch (error) { if (error instanceof admin.messaging.FirebaseMessagingError) { logger.error(`FCM Error for user ${userId}: ${error.code} - ${error.message}`); } else { logger.error(`Unexpected error sending notification to user ${userId}:`, error); } }
- Add an optional data payload parameter to support richer notifications:
async function sendNotification(userId: string, title: string, body: string, data?: Record<string, string>) { // ... existing code ... const payload = { notification: { title, body, }, data, token: fcmToken, }; // ... rest of the function ... }These changes will provide more detailed error information and allow for more flexible notifications with additional data when needed.
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (2)
12-12: Remove trailing whitespaceLine 12 has trailing whitespace. Please remove it to adhere to coding standards.
🧰 Tools
🪛 SwiftLint
[Warning] 12-12: Lines should not have trailing whitespace
(trailing_whitespace)
130-130: Remove trailing whitespaceLine 130 has trailing whitespace. Please remove it.
🧰 Tools
🪛 SwiftLint
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- Data/Data/Model/Expense.swift (2 hunks)
- Splito/UI/Home/Expense/AddExpenseViewModel.swift (1 hunks)
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (1 hunks)
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (2 hunks)
- Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (2 hunks)
- Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (2 hunks)
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1 hunks)
- expense-notifications-functions/src/index.ts (1 hunks)
🧰 Additional context used
🪛 SwiftLint
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
[Warning] 133-133: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift
[Warning] 12-12: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 130-130: Lines should not have trailing whitespace
(trailing_whitespace)
🪛 Biome
expense-notifications-functions/src/index.ts
[error] 184-184: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 360-360: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
Data/Data/Model/Expense.swift (4)
19-19: LGTM: New propertyupdatedByadded correctly.The
updatedByproperty is a good addition to track who last updated the expense. It's correctly declared as public and uses an appropriate type (String) for storing user identifiers.
24-25: LGTM: Initializer updated correctly to includeupdatedBy.The initializer has been properly updated to include the new
updatedByparameter. The changes are consistent with the existing code style and maintain the default parameters. TheupdatedByproperty is correctly initialized in the initializer body.Also applies to: 31-31
44-44: LGTM: CodingKeys enum updated correctly.The
updatedBycase has been properly added to theCodingKeysenum, mapping to "updated_by" for JSON encoding/decoding. This maintains consistency with the existing naming convention for JSON keys.
19-19: Consider the broader impact of addingupdatedBy.While the implementation of the
updatedByproperty is correct, it's important to consider its impact on the rest of the application:
- Ensure that all places where
Expenseobjects are created or updated are modified to provide theupdatedByvalue.- Update any Firebase functions or API endpoints that create or modify expenses to include this new field.
- Consider adding logic to automatically set
updatedByto the current user when an expense is modified.- Update any UI components that display expense details to show the last updater if relevant.
To verify the usage of the new
updatedByproperty, you can run the following script:This will help identify areas of the codebase that might need updates to accommodate the new
updatedByproperty.Also applies to: 24-25, 31-31, 44-44
✅ Verification successful
updatedByProperty Properly ImplementedAll instances of
Expenseinitialization and updates correctly include theupdatedByproperty. No further changes are required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Expense initializations and updates rg --type swift 'Expense\(' -A 10 rg --type swift '\.update' -A 5Length of output: 64062
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (1)
120-121: LGTM: Improved robustness with guard statementThe addition of this guard statement enhances the method's safety by ensuring that both
expenseanduserIdare available before proceeding with the deletion process. This is a good practice to prevent potential runtime errors.Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (3)
132-132: LGTM: Improved transaction trackingThe addition of
newTransaction.updatedBy = userIdenhances the transaction tracking by recording the user who made the update. This change aligns with best practices for audit trails in financial transactions.
158-159: LGTM: Improved data validationThe addition of the guard statement
guard let userId = preference.user?.id else { return }at the beginning of theupdateTransactionmethod is a good practice. It ensures that the user ID is available before proceeding with the transaction update, preventing potential issues later in the method.The empty line after the guard statement improves code readability by clearly separating the initial validation from the main logic of the method.
Line range hint
1-201: Overall assessment: Improved transaction tracking and data validationThe changes in this file consistently enhance transaction tracking by adding the
updatedByproperty to both new and updated transactions. This improvement allows for better audit trails and user accountability in financial transactions.The addition of early data validation in the
updateTransactionmethod is a good practice that prevents potential issues later in the execution.These changes align well with the PR objectives of implementing push notifications for each activity, as they provide the necessary data for tracking who performed each action.
Minor suggestions for improvement:
- Remove trailing whitespace as noted in a previous comment.
- Consider removing the
addedByparameter in theTransactionsinitializer if it's no longer used.Overall, the changes are well-implemented and improve the functionality of the
GroupPaymentViewModel.🧰 Tools
🪛 SwiftLint
[Warning] 133-133: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
298-298: LGTM! Improved user action tracking.The addition of
updatedBy: userIdwhen creating theExpenseobject is a good improvement. It ensures that the user who created the expense is also recorded as the last user to update it, providing more accurate tracking of user actions.
305-310: LGTM! Consider updating method call sites.The addition of
userIdas a parameter and settingupdatedByto this value improves the tracking of expense updates. This change is consistent with the modifications in other parts of the file and enhances the accuracy of user action tracking.To ensure consistency, we should verify that all call sites for this method have been updated to include the
userIdparameter. Run the following script to check for any potential missed updates:If this script returns any results, those locations may need to be updated to include the
userIdparameter.✅ Verification successful
Verified! All call sites for
handleUpdateExpenseActionhave been updated to include theuserIdparameter, ensuring consistent and accurate tracking of expense updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for call sites of handleUpdateExpenseAction that might need updating # Search for method calls that might be missing the userId parameter rg --type swift 'handleUpdateExpenseAction\s*\(\s*groupId:.*expense:' -g '!AddExpenseViewModel.swift'Length of output: 103
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
295-295: Verify the synchronous nature ofdeleteExpenseThe
deleteExpensemethod is now called directly in thepositiveBtnActionclosure without being wrapped in aTask. This change assumes thatdeleteExpenseis now a synchronous method. Please ensure that this change doesn't block the UI thread, especially ifdeleteExpenseperforms any time-consuming operations.To confirm the impact of this change, please run the following script:
✅ Verification successful
Verified: The
deleteExpensemethod still utilizes asynchronous operations internally, ensuring that the UI thread remains unblocked.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any async calls within the deleteExpense method # Test: Search for async calls within deleteExpense rg --type swift -A 20 'func deleteExpense.*\{' Splito/UI/Home/Groups/Group/GroupHomeViewModel.swiftLength of output: 1021
expense-notifications-functions/src/index.ts (2)
3-11: LGTM: Proper setup and initializationThe imports, Firebase initialization, and constant definition are well-structured and follow best practices.
418-444: LGTM: Well-implemented group deletion handlerThe
onGroupDeletedfunction is well-structured and correctly handles the group deletion scenario. It properly notifies all group members except the one who deleted the group, uses the helper functiongetUserDisplayName, and implements error handling correctly. The function follows a consistent pattern with other notification functions in the codebase.Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (1)
16-16: Injection ofpreferenceproperty enables user preference accessThe addition of
@Inject private var preference: SplitoPreferenceallows theGroupTransactionListViewModelto access user preferences. This is necessary for retrieving the current user's ID when performing actions like deleting a transaction.
| private func deleteExpense(expense: Expense) { | ||
| guard let userId = preference.user?.id else { return } | ||
|
|
||
| Task { | ||
| do { | ||
| var deletedExpense = expense | ||
| deletedExpense.updatedBy = userId | ||
| try await expenseRepository.updateExpense(groupId: groupId, expense: deletedExpense) | ||
| try await expenseRepository.deleteExpense(groupId: groupId, expenseId: expense.id ?? "") | ||
| await updateGroupMemberBalance(expense: deletedExpense, updateType: .Delete) | ||
| } catch { | ||
| showToastForError() | ||
| } |
There was a problem hiding this comment.
Review changes in deleteExpense method
The deleteExpense method has undergone significant changes:
- The method is no longer marked as
async, but its content is wrapped in aTask. This maintains asynchronous execution while allowing synchronous calling. - A guard statement has been added to ensure the user ID is available, improving safety.
- The expense is now updated before deletion, which changes the order of operations.
While these changes improve safety and maintain a record of who deleted the expense, there are some considerations:
- Error handling: The catch block only shows a toast. Consider adding more robust error handling or logging.
- Potential race condition: Updating the expense before deleting it could lead to a race condition if another user tries to modify the expense simultaneously.
- Unnecessary copy: Creating a copy of the expense (
deletedExpense) seems unnecessary as it's immediately deleted.
Consider the following improvements:
private func deleteExpense(expense: Expense) {
guard let userId = preference.user?.id else { return }
Task {
do {
- var deletedExpense = expense
- deletedExpense.updatedBy = userId
- try await expenseRepository.updateExpense(groupId: groupId, expense: deletedExpense)
try await expenseRepository.deleteExpense(groupId: groupId, expenseId: expense.id ?? "")
- await updateGroupMemberBalance(expense: deletedExpense, updateType: .Delete)
+ await updateGroupMemberBalance(expense: expense, updateType: .Delete)
} catch {
+ print("Error deleting expense: \(error)")
showToastForError()
}
}
}This simplifies the process by removing the unnecessary update step and improves error logging.
📝 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.
| private func deleteExpense(expense: Expense) { | |
| guard let userId = preference.user?.id else { return } | |
| Task { | |
| do { | |
| var deletedExpense = expense | |
| deletedExpense.updatedBy = userId | |
| try await expenseRepository.updateExpense(groupId: groupId, expense: deletedExpense) | |
| try await expenseRepository.deleteExpense(groupId: groupId, expenseId: expense.id ?? "") | |
| await updateGroupMemberBalance(expense: deletedExpense, updateType: .Delete) | |
| } catch { | |
| showToastForError() | |
| } | |
| private func deleteExpense(expense: Expense) { | |
| guard let userId = preference.user?.id else { return } | |
| Task { | |
| do { | |
| try await expenseRepository.deleteExpense(groupId: groupId, expenseId: expense.id ?? "") | |
| await updateGroupMemberBalance(expense: expense, updateType: .Delete) | |
| } catch { | |
| print("Error deleting expense: \(error)") | |
| showToastForError() | |
| } | |
| } | |
| } |
| @@ -125,8 +126,12 @@ class GroupTransactionDetailViewModel: BaseViewModel, ObservableObject { | |||
| } | |||
|
|
|||
| private func deleteTransaction() async { | |||
| guard var transaction, let userId = preference.user?.id else { return } | |||
There was a problem hiding this comment.
Avoid shadowing the instance variable transaction
In the guard statement on line 129, guard var transaction, let userId = preference.user?.id else { return }, you're shadowing the instance variable transaction with a local variable of the same name. This can lead to confusion and potential bugs.
Consider renaming the local variable to avoid shadowing. For example:
guard var currentTransaction = transaction, let userId = preference.user?.id else { return }And then use currentTransaction within the function.
| guard let transactionId = transaction.id, let userId = preference.user?.id else { return } | ||
|
|
||
| do { | ||
| var deletedTransaction = transaction | ||
| deletedTransaction.updatedBy = userId | ||
| try await transactionRepository.updateTransaction(groupId: groupId, transaction: deletedTransaction) | ||
| try await transactionRepository.deleteTransaction(groupId: groupId, transactionId: transactionId) | ||
| await updateGroupMemberBalance(transaction: transaction, updateType: .Delete) | ||
| await updateGroupMemberBalance(transaction: deletedTransaction, updateType: .Delete) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the transaction deletion process
In the deleteTransaction method, you're updating the transaction's updatedBy field before deletion:
var deletedTransaction = transaction
deletedTransaction.updatedBy = userId
try await transactionRepository.updateTransaction(groupId: groupId, transaction: deletedTransaction)
try await transactionRepository.deleteTransaction(groupId: groupId, transactionId: transactionId)Updating a transaction immediately before deletion may introduce unnecessary overhead. To optimize:
-
Option 1: Modify the
deleteTransactionmethod inTransactionRepositoryto accept anupdatedByparameter. This way, you can pass theuserIddirectly, eliminating the need for a separate update call.try await transactionRepository.deleteTransaction(groupId: groupId, transactionId: transactionId, updatedBy: userId)
-
Option 2: If the
updatedByfield is essential for record-keeping before deletion, ensure that the update and delete operations are atomic to prevent potential race conditions.
🧰 Tools
🪛 SwiftLint
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
| @@ -158,10 +159,14 @@ | |||
| } | |||
|
|
|||
| private func deleteTransaction(transaction: Transactions) async { | |||
| guard let transactionId = transaction.id else { return } | |||
| guard let transactionId = transaction.id, let userId = preference.user?.id else { return } | |||
|
|
|||
There was a problem hiding this comment.
Remove trailing whitespace
Line 163 contains trailing whitespace, as indicated by SwiftLint. Please remove any unnecessary whitespace to maintain code cleanliness.
🧰 Tools
🪛 SwiftLint
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
Summary by CodeRabbit
New Features
AppUserstruct to include a device FCM token for improved messaging capabilities.Bug Fixes
Documentation
Chores