Add logs #75
Add logs #75
Conversation
WalkthroughThe pull request introduces numerous enhancements across various files, primarily focusing on improving logging and error handling within methods related to data fetching and activity logging. Key changes include the addition of logging statements to capture successful operations and errors, thereby enhancing visibility into the application's behavior during runtime. Significant modifications include the introduction of a new case for expense-related image storage and updates to existing methods for better clarity and functionality. Overall, existing logic remains intact while improving the robustness of error reporting. Changes
Possibly related PRs
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: 11
🧹 Outside diff range and nitpick comments (83)
Data/Data/Store/UserStore.swift (2)
57-57: Maintain consistent log message formattingThe new log message format differs from the existing pattern. Consider updating it to match the established format.
-LogD("UserStore: Latest user fetched successfully.") +LogD("UserStore :: \(#function) Latest user fetched successfully.")
Line range hint
28-45: Consider adding logs to other critical operationsFor consistency with the logging improvements, consider adding similar success/error logging to other critical operations in the class:
addUserupdateUserfetchUserBydeactivateUserAfterDeleteExample implementation for one method:
func updateUser(user: AppUser) async throws -> AppUser? { try usersCollection.document(user.id).setData(from: user, merge: true) + LogD("UserStore :: \(#function) User updated successfully: \(user.id)") return user }Also applies to: 67-69
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift (1)
47-47: Enhance log message with additional context.While the log placement is good, consider including the
groupIdand member count in the message for better debugging context.- LogD("GroupWhoGettingPaidViewModel: Group with members fetched successfully.") + LogD("GroupWhoGettingPaidViewModel: Group[\(groupId)] fetched successfully with \(members.count) members.")Data/Data/Helper/Firebase/StorageManager.swift (1)
Line range hint
39-43: Consider adding success logging to updateImage method.For consistency with other methods, consider adding a success log after the image update completes successfully.
public func updateImage(for type: ImageStoreType, id: String, url: String, imageData: Data) async throws -> String? { try await deleteImage(imageUrl: url) // Upload the new image asynchronously - return try await uploadImage(for: type, id: id, imageData: imageData) + let newUrl = try await uploadImage(for: type, id: id, imageData: imageData) + LogD("StorageManager: Image updated successfully.") + return newUrl }Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift (1)
40-42: Consider enhancing log messages with additional context.While the current logs are functional, they could be more informative for debugging purposes.
Consider this enhancement:
- LogD("SelectGroupViewModel: Group fetched successfully.") + LogD("SelectGroupViewModel: Groups fetched successfully. Count: \(groups.count)") - LogE("SelectGroupViewModel: Failed to fetch groups: \(error)") + LogE("SelectGroupViewModel: Failed to fetch groups for user \(preference.user?.id ?? "unknown"): \(error)")Also consider adding a debug log at the start of the fetch operation:
LogD("SelectGroupViewModel: Starting group fetch for user \(preference.user?.id ?? "unknown")")Data/Data/Store/ActivityLogStore.swift (2)
54-54: LGTM! Consider adding more context to the log message.The debug log placement is correct, but it could be more informative.
Consider enhancing the log message with additional context:
- LogD("ActivityLogStore: Latest activity logs fetched successfully.") + LogD("ActivityLogStore: Latest activity logs fetched successfully for user \(userId). Count: \(activityLogs.count)")
Line range hint
63-68: Consider adding similar debug logs to other methods.For consistency with the logging improvements, consider adding debug logs to
addActivityLogandfetchActivitiesBymethods to track successful operations.Add these logs:
func addActivityLog(for userId: String, activity: ActivityLog) async throws { let documentRef = activityReference(userId: userId).document() var newActivity = activity newActivity.id = documentRef.documentID try documentRef.setData(from: newActivity) + LogD("ActivityLogStore: Activity log added successfully. ID: \(newActivity.id)") } func fetchActivitiesBy(userId: String, limit: Int, lastDocument: DocumentSnapshot?) async throws -> (data: [ActivityLog], lastDocument: DocumentSnapshot?) { // ... existing code ... let activityLogs = try snapshot.documents.compactMap { document in try document.data(as: ActivityLog.self) } + LogD("ActivityLogStore: Fetched \(activityLogs.count) activity logs by user \(userId)") return (activityLogs, snapshot.documents.last) }Also applies to: 70-83
Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift (2)
43-49: LGTM! Consider adding more context to success log.The error handling and logging implementation looks good. The error case properly calls completion(false) and logs the specific error.
Consider enhancing the success log message with the code used:
- LogD("JoinMemberViewModel: Member joined successfully.") + LogD("JoinMemberViewModel: Member joined successfully with code: \(code.code)")
77-81: LGTM! Consider adding group context to logs.The logging implementation looks good with appropriate error handling.
Consider enhancing both log messages with the group ID for better traceability:
- LogD("JoinMemberViewModel: Member added to group successfully.") + LogD("JoinMemberViewModel: Member \(userId) added to group \(code.groupId) successfully.") - LogE("JoinMemberViewModel: Failed to add member to group: \(error)") + LogE("JoinMemberViewModel: Failed to add member \(userId) to group \(code.groupId): \(error)")Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (2)
51-51: Consider enhancing the success log with additional context.While the log message follows good practices by including the class name, consider adding the
groupIdfor better traceability.- LogD("ChoosePayerViewModel: Group with members fetched successfully.") + LogD("ChoosePayerViewModel: Group with members fetched successfully for groupId: \(groupId)")
53-55: Consider structuring the error log for better parsing.The error logging implementation is good, but consider structuring the message in a key-value format for easier log parsing and including the
groupId.- LogE("ChoosePayerViewModel: Failed to fetch group with members: \(error)") + LogE("ChoosePayerViewModel: operation=fetchGroup, status=failed, groupId=\(groupId), error=\(error)")Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift (4)
47-47: LGTM! Consider enhancing error logging.The logging placement and messages are appropriate. However, consider including the groupId in the error log for better debugging context.
- LogE("InviteMemberViewModel: Failed to fetch group: \(error)") + LogE("InviteMemberViewModel: Failed to fetch group \(groupId): \(error)")Also applies to: 49-49
63-63: LGTM! Consider including the invite code in success log.The logging placement is good, but the success message could be more informative.
- LogD("InviteMemberViewModel: Invite code generated successfully.") + LogD("InviteMemberViewModel: Invite code generated successfully: \(inviteCode)")Also applies to: 65-65
83-83: LGTM! Consider including the shared code details in logs.The logging and error handling flow is correct. Consider enhancing the logs with more context.
- LogD("InviteMemberViewModel: Shared code stored successfully.") + LogD("InviteMemberViewModel: Shared code stored successfully for group \(groupId): \(inviteCode)") - LogE("InviteMemberViewModel: Failed to store shared code: \(error)") + LogE("InviteMemberViewModel: Failed to store shared code for group \(groupId): \(error)")Also applies to: 87-87, 89-89
Line range hint
47-89: Consider extracting logging messages to a dedicated constants file.The logging implementation is consistent across methods, which is great. To maintain this consistency and make future updates easier, consider moving the log messages to a dedicated constants file or enum.
Example structure:
enum LogMessages { enum InviteMember { static let groupFetchSuccess = "InviteMemberViewModel: Group fetched successfully." static func groupFetchError(_ groupId: String, _ error: Error) -> String { "InviteMemberViewModel: Failed to fetch group \(groupId): \(error)" } // ... other messages } }Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (2)
62-62: Consider enhancing the success log with group ID.While the log message is clear, including the group ID would make it more helpful for debugging.
- LogD("ChooseMultiplePayerViewModel: Group with members fetched successfully.") + LogD("ChooseMultiplePayerViewModel: Group \(groupId) with members fetched successfully.")
64-64: LGTM! Consider including group ID in error log.The error logging is well-placed and includes the error details. For consistency with the success log and better debugging, consider including the group ID.
- LogE("ChooseMultiplePayerViewModel: Failed to fetch group with members: \(error)") + LogE("ChooseMultiplePayerViewModel: Failed to fetch group \(groupId) with members: \(error)")Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift (1)
42-44: Enhance logging with additional context and entry point logging.While the added logs are good, they could be more informative for debugging purposes.
Consider applying these improvements:
private func fetchGroup() async { + LogD("GroupTotalsViewModel: Fetching group with id: \(groupId)") do { let latestGroup = try await groupRepository.fetchGroupBy(id: groupId) group = latestGroup filterDataForSelectedTab() viewState = .initial - LogD("GroupTotalsViewModel: Group fetched successfully.") + LogD("GroupTotalsViewModel: Group \(groupId) fetched successfully") } catch { - LogE("GroupTotalsViewModel: Failed to fetch group: \(error)") + LogE("GroupTotalsViewModel: Failed to fetch group \(groupId). Error: \(error.localizedDescription), Details: \(String(describing: error))") handleServiceError() }Changes:
- Added entry point logging to track when the fetch operation starts
- Included groupId in both success and error logs for better traceability
- Enhanced error logging with both user-friendly message and detailed error information
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (2)
50-50: LGTM! Consider standardizing the error message format.The logging additions in
fetchGroupDetails()are well-placed and use appropriate log levels. The success log confirms completion of all operations, and the error log captures the actual error for debugging.Consider standardizing the error message format across the codebase:
- LogE("GroupSettleUpViewModel: Failed to fetch group: \(error)") + LogE("GroupSettleUpViewModel: Failed to fetch group - \(error)")Also applies to: 52-52
76-76: Consider moving the success log before array modification.The logging additions in
fetchGroupMembers()are good, but the success log placement could be improved.Consider this modification for more accurate logging:
self.members = try await groupRepository.fetchMembersBy(memberIds: group.members) + LogD("GroupSettleUpViewModel: Group members fetched successfully.") self.members.removeAll(where: { $0.id == userId }) - LogD("GroupSettleUpViewModel: Group members fetched successfully.")This ensures the success log accurately reflects the fetch operation, separate from any subsequent array modifications.
Also applies to: 78-78
Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift (2)
Line range hint
1-147: Consider adding logs at other critical pointsTo improve observability, consider adding logging statements at these additional points:
In
verifyOTP():
- Before Firebase authentication attempt
- After successful/failed authentication
In
resendOtp():
- After successful OTP resend
In
onVerificationSuccess():
- Log the navigation outcome
Example implementation:
func verifyOTP() { guard !otp.isEmpty else { return } + LogD("VerifyOtpViewModel: Starting OTP verification") let credential = FirebaseProvider.phoneAuthProvider.credential(withVerificationID: verificationId, verificationCode: otp) showLoader = true FirebaseProvider.auth.signIn(with: credential) {[weak self] (result, _) in self?.showLoader = false if let result { + LogD("VerifyOtpViewModel: Firebase authentication successful") guard let self else { return } self.resendTimer?.invalidate() let user = AppUser(id: result.user.uid, firstName: nil, lastName: nil, emailId: nil, phoneNumber: result.user.phoneNumber, loginType: .Phone) Task { await self.storeUser(user: user) } } else { + LogE("VerifyOtpViewModel: Firebase authentication failed") self?.onLoginError() } } } func resendOtp() { showLoader = true FirebaseProvider.phoneAuthProvider.verifyPhoneNumber((dialCode + phoneNumber), uiDelegate: nil) { [weak self] (verificationID, error) in guard let self else { return } self.showLoader = false if error != nil { // ... existing error logging ... } else { self.verificationId = verificationID ?? "" self.runTimer() + LogD("VerifyOtpViewModel: OTP resent successfully") } } } private func onVerificationSuccess() { if onLoginSuccess == nil { + LogD("VerifyOtpViewModel: Verification successful, navigating to root") router?.popToRoot() } else { + LogD("VerifyOtpViewModel: Verification successful, executing completion handler") onLoginSuccess?(otp) } }
Line range hint
42-68: Consider improving error handling consistency in verifyOTP methodThe
resendOtpmethod has detailed error handling for specific Firebase error codes, whileverifyOTPuses a generic error handler. Consider applying similar error handling patterns across both methods for consistency.Example implementation:
func verifyOTP() { guard !otp.isEmpty else { return } let credential = FirebaseProvider.phoneAuthProvider.credential(withVerificationID: verificationId, verificationCode: otp) showLoader = true - FirebaseProvider.auth.signIn(with: credential) {[weak self] (result, _) in + FirebaseProvider.auth.signIn(with: credential) {[weak self] (result, error) in self?.showLoader = false if let result { guard let self else { return } self.resendTimer?.invalidate() let user = AppUser(id: result.user.uid, firstName: nil, lastName: nil, emailId: nil, phoneNumber: result.user.phoneNumber, loginType: .Phone) Task { await self.storeUser(user: user) } } else { - self?.onLoginError() + if let error = error as NSError? { + if error.code == FirebaseAuth.AuthErrorCode.invalidVerificationCode.rawValue { + self?.showAlertFor(title: "Invalid OTP", message: "Please enter a valid OTP code.") + } else if error.code == FirebaseAuth.AuthErrorCode.sessionExpired.rawValue { + self?.showAlertFor(message: "OTP session has expired. Please request a new OTP.") + } else { + LogE("VerifyOtpViewModel: Firebase verification failed with error: \(error)") + self?.showAlertFor(title: "Verification Failed", + message: "Unable to verify OTP. Please try again.") + } + } } } }Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (2)
113-121: Enhance logging with additional contextGood use of different log levels and clear prefixing. Consider enhancing the logs with:
- Success case: Include the created group's ID for better traceability
- Error case: Add structured context like group name for easier debugging
-LogD("CreateGroupViewModel: Group created successfully.") +LogD("CreateGroupViewModel: Group created successfully. GroupId: \(group.id)") -LogE("CreateGroupViewModel: Failed to create group: \(error)") +LogE("CreateGroupViewModel: Failed to create group. Name: \(groupName), Error: \(error)")
141-149: Enhance update logging with operation detailsConsider enriching the logs with operation-specific details:
- Success case: Include group ID and what was updated (name/image)
- Error case: Add structured context about the attempted changes
-LogD("CreateGroupViewModel: Group updated successfully.") +LogD("CreateGroupViewModel: Group updated successfully. GroupId: \(updatedGroup.id), NameChanged: \(group.name != newGroup.name), ImageChanged: \(imageData != nil)") -LogE("CreateGroupViewModel: Failed to update group: \(error)") +LogE("CreateGroupViewModel: Failed to update group. GroupId: \(group.id), AttemptedName: \(newGroup.name), HasImageUpdate: \(imageData != nil), Error: \(error)")Splito/UI/Login/LoginViewModel.swift (2)
117-117: Consider enhancing the success log message with non-sensitive user information.While the log placement is good, consider including the user ID to make the log more useful for debugging and auditing purposes.
-LogD("LoginViewModel: User stored successfully.") +LogD("LoginViewModel: User stored successfully for userID: \(user.id)")
Line range hint
1-144: Consider adding logging to other error scenarios.While the recent logging additions are good, consider adding similar error logging in other error handling blocks, particularly in:
- Google sign-in error handling
- Firebase authentication error handling in
performFirebaseLogin- Apple sign-in error scenarios
This would provide consistent logging coverage across all authentication paths.
Data/Data/Repository/TransactionRepository.swift (2)
94-94: LGTM! Consider adding transaction details to the log message.The error logging addition is well-placed and follows good practices by including the component name, context, and full error details. However, including the transaction ID (if available) in the log message would make debugging easier by providing complete context.
Consider enhancing the log message:
- LogE("TransactionRepository: Failed to add activity log for \(memberId): \(error)") + LogE("TransactionRepository: Failed to add activity log for member \(memberId), transaction \(activity.activityId): \(error)")
Line range hint
1-115: Consider adding consistent error logging across all repository operations.While the PR adds good error logging for activity logs, other critical operations like
addTransaction,updateTransaction, andfetchTransactionswould benefit from similar error logging for consistency and improved debugging capabilities.For example, consider wrapping Firebase operations with error logging:
public func addTransaction(group: Groups, transaction: Transactions, payer: AppUser, receiver: AppUser) async throws -> Transactions { do { let newTransaction = try await store.addTransaction(groupId: group.id ?? "", transaction: transaction) // ... rest of the code return newTransaction } catch { LogE("TransactionRepository: Failed to add transaction for group \(group.id ?? ""): \(error)") throw error } }Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (2)
58-60: Enhance logging with additional contextThe logging implementation is good, using appropriate log levels (Debug for success, Error for failures). Consider enhancing the logs with additional context:
- LogD("GroupBalancesViewModel: Group with members fetched successfully.") + LogD("GroupBalancesViewModel: Group[\(groupId)] fetched successfully with \(groupMemberData.count) members") - LogE("GroupBalancesViewModel: Failed to fetch group with members: \(error)") + LogE("GroupBalancesViewModel: Failed to fetch group[\(groupId)] with members. State: \(viewState). Error: \(error)")This would provide:
- Group ID for better traceability
- Member count in success case
- View state context in error case
Line range hint
1-1: Consider adding logs at additional critical pointsTo maintain consistent logging coverage across the ViewModel, consider adding logs at these critical points:
- In
calculateExpensesSimplified():private func calculateExpensesSimplified() { guard let group else { viewState = .initial LogE("GroupBalancesViewModel :: \(#function) group not found.") return } + LogD("GroupBalancesViewModel: Starting balance calculations for group[\(groupId)]") // ... calculation logic ... + LogD("GroupBalancesViewModel: Completed balance calculations. Found \(settlements.count) settlements") }
- In
handleAddTransaction():@objc private func handleAddTransaction(notification: Notification) { + LogD("GroupBalancesViewModel: New transaction added, refreshing balances for group[\(groupId)]") showToastFor(toast: .init(type: .success, title: "Success", message: "Payment made successfully.")) Task { await fetchGroupWithMembers() } }
- In
handleServiceError():private func handleServiceError() { if !networkMonitor.isConnected { viewState = .noInternet + LogE("GroupBalancesViewModel: Network connectivity issue detected") } else { viewState = .somethingWentWrong + LogE("GroupBalancesViewModel: Service error occurred") } }Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (3)
67-67: Consider including the count of fetched logs in the success message.The debug log is well-placed, but including the count of fetched logs would provide more useful context for debugging.
- LogD("ActivityLogViewModel: Activity logs fetched successfully.") + LogD("ActivityLogViewModel: Activity logs fetched successfully. Count: \(result.data.count)")
69-69: Consider including the userId in the error log for better traceability.While the error logging is good, including the userId would help in debugging user-specific issues.
- LogE("ActivityLogViewModel: Failed to fetch activity logs: \(error)") + LogE("ActivityLogViewModel: Failed to fetch activity logs for userId: \(userId). Error: \(error)")
93-93: Consider enhancing logs with additional context.Similar to the previous suggestions, include:
- Count of newly fetched logs in the success message
- UserId in the error message
- LogD("ActivityLogViewModel: Activity logs fetched successfully.") + LogD("ActivityLogViewModel: More activity logs fetched successfully. Count: \(result.data.count)") - LogE("ActivityLogViewModel: Failed to fetch more activity logs: \(error)") + LogE("ActivityLogViewModel: Failed to fetch more activity logs for userId: \(userId). Error: \(error)")Also applies to: 96-96
Data/Data/Repository/ExpenseRepository.swift (2)
143-143: LGTM! Good addition of error logging.The error logging statement provides valuable debugging information by capturing both the member ID and the specific error. This aligns well with the PR's objective of improving logging.
Consider structuring the log message as a static string constant for consistency and reusability:
- LogE("ExpenseRepository: Failed to add activity log for \(memberId): \(error)") + let errorMessage = "ExpenseRepository: Failed to add activity log for \(memberId): \(error)" + LogE(errorMessage)
143-143: Consider adding consistent error logging across all error-prone operations.While the added error logging for activity log failures is good, similar error logging could be beneficial in other critical operations:
- Image upload/deletion operations
- Firestore operations (add/update/fetch expenses)
- Group operations
This would provide comprehensive error tracking across the entire expense management flow.
Would you like me to suggest specific logging statements for these operations?
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift (1)
97-103: Enhance log messages with member context.Good implementation of logging with proper error handling. However, the log messages could be more specific to aid in debugging.
Consider this improvement:
- LogD("ExpenseSplitOptionsViewModel: Member fetched successfully.") + LogD("ExpenseSplitOptionsViewModel: Member \(memberId) fetched successfully.") - LogE("ExpenseSplitOptionsViewModel: Failed to fetch member: \(error)") + LogE("ExpenseSplitOptionsViewModel: Failed to fetch member \(memberId): \(error)")This change will make it easier to trace which specific member's data fetch succeeded or failed during debugging.
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
Line range hint
113-177: Consider introducing a logging protocol for better testabilityWhile the current logging implementation works well, consider introducing a logging protocol that can be injected into the view model. This would make it easier to:
- Mock logs in unit tests
- Verify logging behavior
- Switch logging implementations if needed
Example approach:
protocol Logging { func debug(_ message: String) func error(_ message: String, error: Error?) } class GroupHomeViewModel { private let logger: Logging init(logger: Logging) { self.logger = logger } }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (5)
104-104: LGTM! Consider enhancing logs with structured data.The logging placement and error handling are appropriate. To improve debugging capabilities, consider adding structured data to the logs.
-LogD("GroupHomeViewModel: Group fetched successfully.") +LogD("GroupHomeViewModel: Group fetched successfully.", metadata: ["groupId": groupId]) -LogE("GroupHomeViewModel: Failed to fetch group: \(error)") +LogE("GroupHomeViewModel: Failed to fetch group: \(error)", metadata: ["groupId": groupId])Also applies to: 106-106
125-125: LGTM! Consider adding operation context to logs.The logging placement is good, but the logs could be more informative by including operation details.
-LogD("GroupHomeViewModel: Expenses fetched successfully.") +LogD("GroupHomeViewModel: Expenses fetched successfully.", metadata: [ + "groupId": groupId, + "count": result.expenses.count, + "hasMore": hasMoreExpenses +]) -LogE("GroupHomeViewModel: Failed to fetch expenses: \(error)") +LogE("GroupHomeViewModel: Failed to fetch expenses: \(error)", metadata: [ + "groupId": groupId, + "limit": EXPENSES_LIMIT +])Also applies to: 127-127
161-161: LGTM! Maintain consistency with fetchExpenses logs.The logging and error handling look good. Consider maintaining consistency with the fetchExpenses() method's logging format.
-LogD("GroupHomeViewModel: Expenses fetched successfully.") +LogD("GroupHomeViewModel: More expenses fetched successfully.", metadata: [ + "groupId": groupId, + "count": result.expenses.count, + "hasMore": hasMoreExpenses +]) -LogE("GroupHomeViewModel: Failed to fetch more expenses: \(error)") +LogE("GroupHomeViewModel: Failed to fetch more expenses: \(error)", metadata: [ + "groupId": groupId, + "limit": EXPENSES_LIMIT +])Also applies to: 163-163
201-201: LGTM! Add member context to logs.The logging and error handling are appropriate. Consider adding member context to improve debugging capabilities.
-LogD("GroupHomeViewModel: Member fetched successfully.") +LogD("GroupHomeViewModel: Member fetched successfully.", metadata: [ + "memberId": memberId, + "groupId": groupId +]) -LogE("GroupHomeViewModel: Failed to fetch member data: \(error)") +LogE("GroupHomeViewModel: Failed to fetch member data: \(error)", metadata: [ + "memberId": memberId, + "groupId": groupId +])Also applies to: 205-205
Line range hint
104-205: Consider implementing a consistent logging strategy.While the logging additions are valuable, implementing a structured logging strategy across all methods would improve debugging and monitoring capabilities. Consider:
- Using consistent metadata fields across similar operations
- Adding log levels appropriately (DEBUG for success, ERROR for failures)
- Including relevant context (IDs, counts, operation status)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (4)
96-98: Consider consistent terminology in log messages.For better clarity and consistency, consider using "Transaction" instead of "Payment" in the log messages to match the method name:
-LogD("GroupPaymentViewModel: Payment fetched successfully.") +LogD("GroupPaymentViewModel: Transaction fetched successfully.") -LogE("GroupPaymentViewModel: Failed to fetch payment: \(error)") +LogE("GroupPaymentViewModel: Failed to fetch transaction: \(error)")
185-189: Consider consistent terminology in log messages.For better clarity and consistency, consider using "Transaction" instead of "Payment" in the log messages:
-LogD("GroupPaymentViewModel: Payment added successfully.") +LogD("GroupPaymentViewModel: Transaction added successfully.") -LogE("GroupPaymentViewModel: Failed to add payment: \(error)") +LogE("GroupPaymentViewModel: Failed to add transaction: \(error)")
217-221: Consider consistent terminology in log messages.For better clarity and consistency, consider using "Transaction" instead of "Payment" in the log messages:
-LogD("GroupPaymentViewModel: Payment updated successfully.") +LogD("GroupPaymentViewModel: Transaction updated successfully.") -LogE("GroupPaymentViewModel: Failed to update payment: \(error)") +LogE("GroupPaymentViewModel: Failed to update transaction: \(error)")
Line range hint
82-248: Consider implementing structured logging for better observability.While the current logging implementation is good, consider these enhancements for better observability:
- Create an enum for log categories/components to ensure consistency:
enum LogComponent: String { case transaction = "Transaction" case group = "Group" case user = "User" }
- Use a structured logging helper that includes additional context:
func logOperation(_ component: LogComponent, operation: String, success: Bool, error: Error? = nil) { let message = "\(Self.self): \(component.rawValue) \(operation)" if success { LogD(message) } else { LogE("\(message): \(error?.localizedDescription ?? "")") } }This would make logs more consistent and easier to filter/analyze.
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (7)
57-57: Enhance success log with group ID for better traceability.Consider including the group ID in the success log for easier debugging and correlation.
-LogD("ExpenseDetailsViewModel: Group fetched successfully.") +LogD("ExpenseDetailsViewModel: Group \(groupId) fetched successfully.")
70-70: Include identifiers in success log for better traceability.Consider including both group and expense IDs in the success log.
-LogD("ExpenseDetailsViewModel: Expense fetched successfully.") +LogD("ExpenseDetailsViewModel: Expense \(expenseId) for group \(groupId) fetched successfully.")
98-100: Move success log after validating member data.The current placement could log success even if
memberis nil. Consider moving the log after validating the member.let member = try await userRepository.fetchUserBy(userID: userId) -LogD("ExpenseDetailsViewModel: Member fetched successfully.") -return member +if let member = member { + LogD("ExpenseDetailsViewModel: Member \(userId) fetched successfully.") + return member +} +LogD("ExpenseDetailsViewModel: No member found for ID \(userId).") +return nil
155-155: Include expense ID in restoration logs for better traceability.Consider including the expense ID in both success and error logs.
-LogD("ExpenseDetailsViewModel: Expense restored successfully.") +LogD("ExpenseDetailsViewModel: Expense \(expense.id) restored successfully.") -LogE("ExpenseDetailsViewModel: Failed to restore expense: \(error)") +LogE("ExpenseDetailsViewModel: Failed to restore expense \(expense.id): \(error)")Also applies to: 158-158
186-186: Include expense ID in deletion logs for better traceability.Consider including the expense ID in both success and error logs.
-LogD("ExpenseDetailsViewModel: Expense deleted successfully.") +LogD("ExpenseDetailsViewModel: Expense \(expense.id) deleted successfully.") -LogE("ExpenseDetailsViewModel: Failed to delete expense: \(error)") +LogE("ExpenseDetailsViewModel: Failed to delete expense \(expense.id): \(error)")Also applies to: 190-190
235-235: Enhance balance update logs with operation details.Consider including the update type and group ID for better context.
-LogD("ExpenseDetailsViewModel: Group member balance updated successfully.") +LogD("ExpenseDetailsViewModel: Group \(group.id) member balance updated successfully for \(updateType) operation.") -LogE("ExpenseDetailsViewModel: Failed to update group member balance: \(error)") +LogE("ExpenseDetailsViewModel: Failed to update group \(group.id) member balance for \(updateType) operation: \(error)")Also applies to: 238-238
Line range hint
197-201: Add logging for validation failures.Consider adding error logs when validation fails in
validateGroupMembers. This would help track cases where operations fail due to missing members.if !missingMemberIds.isEmpty { + LogE("ExpenseDetailsViewModel: Cannot perform operation. Members \(missingMemberIds) are no longer in group \(group.id).") DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { self.showAlertFor(message: "This expense involves a person who has left the group, and thus it can no longer be \(action). If you wish to change this expense, you must first add that person back to your group.") }Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (4)
53-55: Enhance logging with contextual informationThe logging statements should include the group ID for better traceability.
- LogD("GroupSettingViewModel: Group fetched successfully.") + LogD("GroupSettingViewModel: Group \(groupId) fetched successfully.") - LogE("GroupSettingViewModel: Failed to fetch group: \(error)") + LogE("GroupSettingViewModel: Failed to fetch group \(groupId): \(error)")
69-71: Add member count to success logInclude the number of members fetched in the success log for better visibility.
- LogD("GroupSettingViewModel: Group members fetched successfully.") + LogD("GroupSettingViewModel: \(members.count) members fetched successfully for group \(group.id).") - LogE("GroupSettingViewModel: Failed to fetch group members: \(error)") + LogE("GroupSettingViewModel: Failed to fetch members for group \(group.id): \(error)")
221-224: Include member details in removal logsAdd member identification information to the logs for better tracking of member removals.
- LogD("GroupSettingViewModel: Group member removed successfully.") + LogD("GroupSettingViewModel: Member \(member.id) removed successfully from group \(group.id).") - LogE("GroupSettingViewModel: Failed to remove member from group: \(error)") + LogE("GroupSettingViewModel: Failed to remove member \(member.id) from group \(group.id): \(error)")
251-254: Add group context to deletion logsInclude group identification in the logs for better tracking of group deletions.
- LogD("GroupSettingViewModel: Group deleted successfully.") + LogD("GroupSettingViewModel: Group \(group.id) deleted successfully.") - LogE("GroupSettingViewModel: Failed to delete group: \(error)") + LogE("GroupSettingViewModel: Failed to delete group \(group.id): \(error)")Data/Data/Repository/GroupRepository.swift (2)
178-178: Consider enhancing log structure for better parsing.Consider structuring the log message to make it more machine-parseable, which could help with log aggregation and analysis:
- LogE("GroupRepository: Failed to add activity log for \(memberId): \(error)") + LogE("GroupRepository: Failed to add activity log | memberId=\(memberId) | error=\(error)")
Line range hint
1-266: Consider adding logging to other critical operations.While the error logging for activity logs is good, consider adding similar logging statements to other critical operations for better observability:
- Group Operations:
- Group creation success/failure
- Group updates
- Group deletion
- Member Management:
- Member addition/removal
- Image Operations:
- Image upload success/failure
- Image deletion
This would provide a more complete audit trail and help with debugging.
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (5)
67-67: Consider consistent terminology in log messages.While the logging implementation is good, consider using consistent terminology: the method name uses "transaction" but the log messages use "payment". Consider using "transaction" throughout for consistency.
- LogD("GroupTransactionDetailViewModel: Payment fetched successfully.") + LogD("GroupTransactionDetailViewModel: Transaction fetched successfully.") - LogE("GroupTransactionDetailViewModel: Failed to fetch payment: \(error)") + LogE("GroupTransactionDetailViewModel: Failed to fetch transaction: \(error)")Also applies to: 69-69
100-100: Consider consistent terminology in log messages.While the logging implementation is good, consider using consistent terminology: the method name uses "UserData" but the log messages use "Member". Consider using "User" throughout for consistency.
- LogD("GroupTransactionDetailViewModel: Member fetched successfully.") + LogD("GroupTransactionDetailViewModel: User fetched successfully.") - LogE("GroupTransactionDetailViewModel: Failed to fetch member: \(error)") + LogE("GroupTransactionDetailViewModel: Failed to fetch user: \(error)")Also applies to: 104-104
158-158: Consider consistent terminology in log messages.While the logging implementation is good, consider using consistent terminology: the method name uses "Transaction" but the log messages use "Payment". Consider using "transaction" throughout for consistency.
- LogD("GroupTransactionDetailViewModel: Payment restored successfully.") + LogD("GroupTransactionDetailViewModel: Transaction restored successfully.") - LogE("GroupTransactionDetailViewModel: Failed to restore payment: \(error)") + LogE("GroupTransactionDetailViewModel: Failed to restore transaction: \(error)")Also applies to: 162-162
193-193: Consider consistent terminology in log messages.While the logging implementation is good, consider using consistent terminology: the method name uses "Transaction" but the log messages use "Payment". Consider using "transaction" throughout for consistency.
- LogD("GroupTransactionDetailViewModel: Payment deleted successfully.") + LogD("GroupTransactionDetailViewModel: Transaction deleted successfully.") - LogE("GroupTransactionDetailViewModel: Failed to delete payment: \(error)") + LogE("GroupTransactionDetailViewModel: Failed to delete transaction: \(error)")Also applies to: 197-197
Line range hint
1-267: Overall logging implementation is solid with room for terminology consistency.The logging implementation across the file follows good practices:
- Consistent logging pattern in all methods
- Appropriate use of log levels (LogD for success, LogE for errors)
- Meaningful context in log messages
- Proper error details in error logs
However, there's inconsistent terminology usage between "transaction" and "payment" throughout the log messages. Consider standardizing on "transaction" to match the class and method names.
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (4)
64-64: Consider enhancing error logging with group IDThe logging implementation looks good, but consider including the group ID in the error log for better traceability.
- LogE("GroupTransactionListViewModel: Failed to fetch group: \(error)") + LogE("GroupTransactionListViewModel: Failed to fetch group \(groupId): \(error)")Also applies to: 66-67
83-83: Consider adding transaction count to success logThe logging implementation is good, but including the number of transactions fetched would provide valuable context.
- LogD("GroupTransactionListViewModel: Payments fetched successfully.") + LogD("GroupTransactionListViewModel: \(result.transactions.count) payments fetched successfully.")Also applies to: 85-86
105-105: Differentiate pagination logs from initial fetchThe current success log doesn't distinguish between initial fetch and pagination. Consider clarifying this.
- LogD("GroupTransactionListViewModel: Payments fetched successfully.") + LogD("GroupTransactionListViewModel: Additional \(result.transactions.count) payments fetched successfully.")Also applies to: 107-109
Line range hint
64-204: Consider implementing a logging level strategyWhile the logging implementation is good, consider defining a clear strategy for log levels:
- LogD: Use for detailed debugging information
- LogI: Use for important state transitions
- LogW: Use for recoverable issues
- LogE: Use for unrecoverable errors
This would help in filtering logs based on severity during production issues.
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (3)
143-143: LGTM! Consider enhancing the error log message.The logging implementation follows good practices with appropriate log levels (debug for success, error for failures). The message format is consistent and clear.
Consider structuring the error log message to separate the static text from the error details:
-LogE("UserProfileViewModel: Failed to update user: \(error)") +LogE("UserProfileViewModel: Failed to update user - Error: \(error.localizedDescription)")Also applies to: 151-151
172-172: Consider using error level logging for guard clause failure.Since this represents a potential error condition where a required user object is missing, consider using
LogEinstead ofLogDto maintain consistency with error handling patterns.-LogD("UserProfileViewModel: User does not exist.") +LogE("UserProfileViewModel: User does not exist.")
Line range hint
143-186: Consider implementing structured logging for better observability.While the current logging implementation is good, consider enhancing it further by implementing structured logging. This would make logs more machine-readable and easier to parse in logging systems.
Example structured format:
LogD("operation: update_user, status: success, component: UserProfileViewModel") LogE("operation: update_user, status: error, component: UserProfileViewModel, error: \(error.localizedDescription)")This would enable better:
- Log aggregation and filtering
- Metrics extraction
- Error tracking and analysis
Splito/UI/Home/Groups/GroupListViewModel.swift (5)
92-94: Enhance success log message with contextWhile the logging levels are appropriate, the success message could be more informative by including the count of groups fetched.
- LogD("GroupListViewModel: Groups fetched successfully.") + LogD("GroupListViewModel: Successfully fetched \(sortedGroups.count) groups.")
119-122: Enhance success log message with pagination detailsThe success message could be more informative by including the count of additional groups fetched and pagination status.
- LogD("GroupListViewModel: Groups fetched successfully.") + LogD("GroupListViewModel: Successfully fetched \(sortedGroups.count) more groups. Has more: \(hasMoreGroups)")
175-180: Include group ID in log messages for better traceabilityAdd the group ID to both success and error logs to make debugging easier.
- LogD("GroupListViewModel: Group fetched successfully.") + LogD("GroupListViewModel: Successfully fetched group with ID: \(groupId)") - LogE("GroupListViewModel: Failed to fetch group: \(error)") + LogE("GroupListViewModel: Failed to fetch group with ID \(groupId): \(error)")
291-293: Include group details in deletion logsAdd the group ID and name to the logs for better context when tracking group deletions.
- LogD("GroupListViewModel: Group deleted successfully.") + LogD("GroupListViewModel: Successfully deleted group '\(group.name)' (ID: \(group.id ?? "unknown"))") - LogE("GroupListViewModel: Failed to delete group: \(error)") + LogE("GroupListViewModel: Failed to delete group '\(group.name)' (ID: \(group.id ?? "unknown"): \(error)")
388-390: Make log messages more specific to the operationThe current log messages focus only on member fetching, but the method processes the entire group. Update the messages to reflect the complete operation.
- LogD("GroupListViewModel: Members fetched successfully.") + LogD("GroupListViewModel: Successfully processed group '\(group.name)' (ID: \(group.id ?? "unknown")) - \(isNewGroup ? "added new" : "updated existing")") - LogE("GroupListViewModel: Failed to fetch members: \(error)") + LogE("GroupListViewModel: Failed to process group '\(group.name)' (ID: \(group.id ?? "unknown")): \(error)")Splito/UI/Home/Expense/AddExpenseViewModel.swift (7)
86-90: Enhance success log with group ID for better traceabilityWhile the logging additions are good, including the group ID in the success log would make it easier to trace specific group operations.
- LogD("AddExpenseViewModel: Group fetched successfully.") + LogD("AddExpenseViewModel: Group \(groupId) fetched successfully.")
105-109: Include user ID in success log for better traceabilityConsider adding the user ID to the success log for improved debugging capabilities.
- LogD("AddExpenseViewModel: Default user fetched successfully.") + LogD("AddExpenseViewModel: Default user \(id) fetched successfully.")
121-125: Add expense and group IDs to success logInclude both expense and group IDs in the success log for better correlation of events.
- LogD("AddExpenseViewModel: Expense details with members fetched successfully.") + LogD("AddExpenseViewModel: Expense details for expense \(expenseId) in group \(groupId) fetched successfully.")
167-173: Add user ID to success logInclude the user ID in the success log for better traceability.
- LogD("AddExpenseViewModel: Member fetched successfully.") + LogD("AddExpenseViewModel: Member \(userId) fetched successfully.")
392-398: Enhance success log with expense detailsInclude basic expense details in the success log for better debugging context.
- LogD("AddExpenseViewModel: Expense added successfully.") + LogD("AddExpenseViewModel: Expense '\(expense.name)' added successfully to group \(groupId) with amount \(expense.amount).")
439-439: Remove trailing whitespaceRemove the trailing whitespace on this line.
- +🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 439-439: Lines should not have trailing whitespace
(trailing_whitespace)
441-446: Add expense details to success logInclude expense details in the success log for better debugging context.
- LogD("AddExpenseViewModel: Expense updated successfully.") + LogD("AddExpenseViewModel: Expense '\(expense.name)' updated successfully in group \(group.id ?? "") with new amount \(expense.amount).")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
Data/Data/Helper/Firebase/StorageManager.swift(1 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(1 hunks)Data/Data/Repository/TransactionRepository.swift(1 hunks)Data/Data/Store/ActivityLogStore.swift(1 hunks)Data/Data/Store/UserStore.swift(1 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(3 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(8 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(6 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift(3 hunks)Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift(2 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(7 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(3 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(6 hunks)Splito/UI/Login/LoginViewModel.swift(1 hunks)Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift(1 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Expense/AddExpenseViewModel.swift
[Warning] 439-439: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (22)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift (1)
47-47: Verify logging implementation consistency.
Let's verify the logging implementation and usage patterns across the codebase.
Also applies to: 49-49
✅ Verification successful
Logging implementation is consistent and well-structured
The logging implementation follows a consistent pattern across the codebase:
- Uses three well-defined logging functions:
LogD(debug),LogE(error), andLogI(info) defined inData/Data/Helper/Logger/DDLogProvider.swift - Follows a consistent format:
"{ClassName}: {descriptive message}" - Error logs include error details and stack traces
- Debug logs track successful operations
- Info logs are used sparingly for important state changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for logging function usage patterns
echo "Checking logging function usage patterns..."
rg "Log[DEI]?\(" -A 2
# Look for the logging function definitions
echo "Looking for logging function definitions..."
ast-grep --pattern 'func Log$_($$$) {
$$$
}'
Length of output: 72811
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift (1)
47-47: LGTM! Well-structured logging implementation.
The added logging statements follow good practices:
- Success log uses debug level (LogD) and error log uses error level (LogE)
- Messages include context (class name) and relevant details
- Error logging includes the actual error object for debugging
- Placement is appropriate - success log after state update, error log before handling
Also applies to: 49-49
Data/Data/Helper/Firebase/StorageManager.swift (1)
62-62: LGTM! The new log statement follows the established patterns.
The added success log maintains consistency with existing logging patterns and improves debugging visibility.
Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift (1)
40-40: LGTM! Success logging added appropriately.
The debug log is correctly placed after successful group fetching.
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (1)
51-53: Verify logging configuration and guidelines.
The logging implementation looks good, but let's ensure consistency across the app.
✅ Verification successful
Based on the output, I can now generate the final response as we have comprehensive information about the logging implementation across the codebase.
Logging implementation is consistent and well-structured
The codebase demonstrates a consistent logging pattern with:
- Centralized logging implementation in
Data/Data/Helper/Logger/DDLogProvider.swift - Consistent log levels (LogD, LogE, LogI, LogW) used across all files
- Structured log messages with clear prefixes indicating the source component
- Proper error handling with detailed error messages
The current implementation in ChoosePayerViewModel.swift follows these established patterns correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for logging configuration and usage patterns
# Look for logging configuration files
echo "Searching for logging configuration..."
fd -e swift -e json -e plist --full-path ".*[Ll]og.*[Cc]onfig.*"
# Check for consistent log level usage
echo -e "\nAnalyzing log level usage..."
rg "Log[DIWE]" --type swift
# Look for logging guidelines in documentation
echo -e "\nSearching for logging guidelines..."
fd -e md -e txt --full-path ".*[Ll]og.*[Gg]uideline.*"
Length of output: 33794
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (1)
62-65: Verify logging pattern consistency across the codebase.
Let's verify that similar logging patterns are used consistently across related files mentioned in the summary.
✅ Verification successful
Logging patterns are consistent across the codebase
The verification shows consistent logging patterns throughout the codebase:
- Success scenarios use
LogDwith "{ClassName}: {action} successfully" format - Error scenarios use
LogEwith "{ClassName}: Failed to {action}: (error)" format - The logging pattern in
ChooseMultiplePayerViewModel.swiftfollows the same convention as other files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check logging patterns in related files
# Expected: Similar logging patterns in repository and viewmodel files
# Search for logging patterns in Swift files
rg -t swift "LogD\(.*successfully"
rg -t swift "LogE\(.*failed"
# Search specifically for group-related logging
rg -t swift "group.*fetch.*successfully"
rg -t swift "Failed to fetch group"
Length of output: 14922
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift (1)
42-44: Verify logging consistency across repositories.
Let's ensure the logging pattern is consistent with other repositories mentioned in the summary.
✅ Verification successful
Logging pattern is consistent across the codebase
The logging pattern in GroupTotalsViewModel follows the same consistent pattern used throughout the codebase:
- Uses
LogDfor successful operations - Uses
LogEfor error cases - Includes class name prefix in messages
- Includes error details in error logs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check logging patterns in related repository files
# Expected: Similar logging patterns in addActivityLog methods
# Search for logging patterns in repository files
echo "Checking logging patterns in repositories:"
rg -A 2 "LogD|LogE" --type swift | grep -v "GroupTotalsViewModel"
Length of output: 66928
Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift (1)
131-135: Well-implemented logging statements!
The added logging statements effectively capture both success and error scenarios with appropriate log levels and context. The error logging includes the actual error details which will be valuable for debugging.
Splito/UI/Login/LoginViewModel.swift (1)
119-122: LGTM! Good error handling implementation.
The changes demonstrate good practices:
- Appropriate error level logging
- Comprehensive error details in log message
- User-friendly error handling with alerts
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
113-115: LGTM! Well-structured logging in restoreGroup()
The logging additions follow good practices with appropriate log levels and error details.
Line range hint 151-151: LGTM! Clear validation error logging
The error logging is well-placed and appropriately indicates the validation failure reason.
175-177: LGTM! Well-structured logging in updateGroupMemberBalance()
The logging additions follow good practices with appropriate log levels and error details.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (4)
82-84: LGTM! Well-structured logging implementation.
The logging statements are appropriately placed and include relevant context (class name) and error details.
109-111: LGTM! Consistent logging implementation.
The logging statements maintain the established pattern and provide clear context.
122-124: LGTM! Consistent logging implementation.
The logging statements maintain the established pattern and provide clear context.
245-248: LGTM! Well-structured logging implementation.
The logging statements are appropriately placed and include relevant context and error details.
Data/Data/Repository/GroupRepository.swift (1)
178-178: LGTM! Good error logging implementation.
The error logging statement effectively captures both the member ID and error details, which will be valuable for debugging activity log creation failures.
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (2)
53-53: LGTM! Logging is well implemented.
The logging implementation in fetchGroup() follows good practices:
- Includes class name for context
- Uses appropriate log levels (debug for success, error for failures)
- Error log includes the actual error details
Also applies to: 55-55
240-240: LGTM! Logging is well implemented.
The logging implementation in updateGroupMemberBalance() follows good practices:
- Includes class name for context
- Uses appropriate log levels
- Error log includes the actual error details
- Terminology is consistent with the method name
Also applies to: 243-243
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (1)
183-183: LGTM! Logging implementation is appropriate.
The logging implementation follows good practices:
- Appropriate log levels for success and failure cases
- Consistent message format
- Includes error details for debugging
Also applies to: 186-186
Splito/UI/Home/Groups/GroupListViewModel.swift (1)
Line range hint 92-390: Overall logging implementation looks good!
The implementation demonstrates good logging practices with:
- Appropriate use of debug and error log levels
- Consistent error handling with detailed error messages
- Strategic placement of logs at key operations
The suggested enhancements focus on adding more context to log messages for better debugging and traceability.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
Line range hint 86-472: Overall logging implementation looks good
The logging implementation follows good practices:
- Consistent logging of both success and error cases
- Error logs include the actual error object for debugging
- Clear class prefix in all log messages
- Appropriate log levels (Debug for success, Error for failures)
The suggestions above aim to enhance the logs by adding more context to success messages, making it easier to trace operations through the logs.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 439-439: Lines should not have trailing whitespace
(trailing_whitespace)
...ome/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (24)
Data/Data/Store/ShareCodeStore.swift (1)
20-20: LGTM! Consistent logging pattern with good context.The logging changes follow a consistent pattern by:
- Including method context using
#function- Using appropriate log levels (LogE for errors, LogD for debug)
- Providing clear and descriptive messages
Consider adding the shared code or document ID to the log messages where available for better traceability:
- LogE("ShareCodeStore: \(#function) Failed to add shared code: \(error).") + LogE("ShareCodeStore: \(#function) Failed to add shared code \(sharedCode.code): \(error).")- LogD("ShareCodeStore: \(#function) The document is not available.") + LogD("ShareCodeStore: \(#function) No document found for code: \(code).")- LogE("ShareCodeStore: \(#function) Deleting data failed with error: \(error).") + LogE("ShareCodeStore: \(#function) Failed to delete document \(documentId): \(error).")Also applies to: 32-32, 44-44
Data/Data/Store/SplitoPreference.swift (3)
49-49: Enhance error logging structure for better debuggingWhile the error logging includes the function context and error details, consider adding more structured information such as the user identifier (if available) and operation context.
- LogE("AppPreferences \(#function) json decode error: \(error).") + LogE("AppPreferences \(#function) json decode error for user data: \(error). User: \(String(describing: user?.id))")Also applies to: 57-57
Line range hint
41-60: Consider using Keychain for sensitive user data storageUserDefaults is not the most secure storage mechanism for sensitive user data. Consider using Keychain Services API for storing user information, as it provides encrypted storage and better security guarantees.
I can help provide an implementation using Keychain Services if you'd like.
Line range hint
22-60: Ensure consistent data persistence behaviorThere's inconsistent usage of
synchronize()across different properties. While it's used forisOnboardShown, it's missing for other properties. Note that whilesynchronize()is not strictly necessary in modern iOS versions, consistency in implementation is important.Additionally, consider handling potential race conditions in property observers.
public var user: AppUser? { get { do { let data = userDefaults.data(forKey: Key.user.rawValue) if let data { let user = try JSONDecoder().decode(AppUser.self, from: data) return user } } catch let error { LogE("AppPreferences \(#function) json decode error: \(error).") } return nil } set { do { let data = try JSONEncoder().encode(newValue) userDefaults.set(data, forKey: Key.user.rawValue) + userDefaults.synchronize() } catch let error { LogE("AppPreferences \(#function) json encode error: \(error).") } } }Data/Data/Extension/Query+Extension.swift (3)
34-34: Improve log message clarityThe current message could be more descriptive about the specific condition being logged:
- LogE("SnapshotPublisher: The document is not available.") + LogE("[\(#function)] No documents found in snapshot")
45-45: Enhance decode error loggingThe decoding error log could be more structured and informative:
- LogE("SnapshotPublisher: Decode error: \(error).") + LogE("[\(#function)] Failed to decode documents: \(error)")
Line range hint
41-42: Consider adding success loggingFor consistency with the error logging and better observability, consider adding a success log when documents are successfully decoded:
let decodedDocuments = try documents.compactMap { document in try document.data(as: T.self) } + LogI("[\(#function)] Successfully decoded \(decodedDocuments.count) documents") publisher.send(decodedDocuments)Data/Data/Store/GroupStore.swift (4)
21-23: Well-structured separation of concernsSeparating document reference creation from data setting is a good architectural decision that:
- Enables atomic operations
- Allows for pre-knowing document IDs
- Facilitates better transaction management
25-27: Add error logging and validationWhile the implementation is correct, consider adding:
- Error logging for creation failures
- Validation for the document parameter
func createGroup(document: DocumentReference, group: Groups) async throws { + guard !document.path.isEmpty else { + LogE("GroupStore: Invalid document reference") + throw ServiceError.invalidInput + } try document.setData(from: group) + LogI("GroupStore: Successfully created group with ID: \(document.documentID)") }
31-33: Add logging for better observabilityConsider adding logging to track member additions:
try await groupReference.document(groupId).updateData([ "members": FieldValue.arrayUnion([memberId]) ]) +LogI("GroupStore: Successfully added member \(memberId) to group \(groupId)")
60-60: Consider flexible source strategyUsing
source: .serveralways fetches from the network. Consider implementing a strategy that can utilize cache when appropriate:-return try await groupReference.document(id).getDocument(as: Groups.self, source: .server) +return try await groupReference.document(id).getDocument(as: Groups.self, source: preference.isOfflineMode ? .cache : .default)Data/Data/Store/ExpenseStore.swift (3)
24-26: LGTM! Good architectural improvementSeparating document creation from data insertion is a good practice as it:
- Enables atomic operations when needed
- Allows for better transaction control
- Makes it possible to handle the document reference before committing data
- Facilitates scenarios like image upload where you might need the document ID first
28-30: Add logging statements for better observabilityConsider adding logging statements to track successful operations and potential errors, consistent with the logging improvements in other methods.
func addExpense(document: DocumentReference, expense: Expense) async throws { + LogI("ExpenseStore: Adding expense with ID: \(document.documentID)") try document.setData(from: expense) + LogI("ExpenseStore: Successfully added expense with ID: \(document.documentID)") }
36-38: Add success logging for consistencyWhile error logging is in place, consider adding success logging to maintain consistency with other operations and improve debugging capabilities.
if let expenseId = expense.id { + LogI("ExpenseStore: Updating expense with ID: \(expenseId)") try expenseReference(groupId: groupId).document(expenseId).setData(from: expense, merge: false) + LogI("ExpenseStore: Successfully updated expense with ID: \(expenseId)") } else { LogE("ExpenseStore: \(#function) Expense not found.") throw ServiceError.dataNotFound }BaseStyle/BaseStyle/CustomUI/MailComposeView.swift (1)
38-38: Enhance error logging with additional contextWhile directly logging the error object is good, consider enhancing the error logging to include both the raw error and its localized description for better debugging context.
- print("MailComposeView: Failed to load attachment data: \(error).") + print("MailComposeView: Failed to load attachment data. Error: \(error), Description: \(error.localizedDescription)")Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift (1)
91-91: Enhance error logging with additional contextWhile logging the full error object is good, we can make the logs more searchable and parseable.
Consider this enhancement:
- LogE("Firebase: Phone login fail with error: \(error).") + LogE("Firebase: [\(#function)] Phone login failed - Error: \(error) - Number: \(currentCountry.dialCode + phoneNumber)")This change:
- Adds function context using
#function- Includes the phone number for debugging
- Uses consistent grammar ("failed" instead of "fail")
- Structures the message for easier log parsing
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (2)
104-108: Document date formatting logic.The conditional date formatting logic improves readability by using different formats for current month vs. older logs. Consider adding a comment explaining this design choice for better maintainability.
filteredLogs = Dictionary(grouping: sortedActivities) { log in + // Use day-wise format for current month logs and month-year format for older logs + // to improve readability and reduce visual clutter if log.recordedOn.dateValue().isCurrentMonth() { return ActivityLogViewModel.dateFormatter.string(from: log.recordedOn.dateValue()) // day-wise format } else { return log.recordedOn.dateValue().monthWithYear // month-year format }
155-163: Enhance readability and efficiency of sorting implementation.The sorting logic is sound but could be improved for better maintainability and performance.
Consider these improvements:
func sortKeysByDayAndMonth() -> [String] { - let dayKeys = filteredLogs.keys.filter { ActivityLogViewModel.dateFormatter.date(from: $0) != nil } - let monthYearKeys = filteredLogs.keys.filter { ActivityLogViewModel.dateFormatter.date(from: $0) == nil } - - let sortedDayKeys = dayKeys.sorted(by: sortDayMonthYearStrings) - let sortedMonthYearKeys = monthYearKeys.sorted(by: sortMonthYearStrings) - - return (sortedDayKeys + sortedMonthYearKeys) // Concatenate for final sorted order + // Partition keys into current and past periods for efficient sorting + let (currentPeriodKeys, pastPeriodKeys) = filteredLogs.keys.partition { + ActivityLogViewModel.dateFormatter.date(from: $0) != nil + } + + return currentPeriodKeys.sorted(by: sortDayMonthYearStrings) + + pastPeriodKeys.sorted(by: sortMonthYearStrings) }Data/Data/Repository/ExpenseRepository.swift (1)
17-33: Consider adding image validationWhile the image handling logic is well-structured, consider adding validation for:
- Image data size limits
- Acceptable image formats
- Basic image integrity checks
This would prevent potential issues with large or corrupt images.
Example validation:
public func addExpense(group: Groups, expense: Expense, imageData: Data?) async throws -> Expense { + if let imageData { + guard imageData.count <= 5_000_000 else { // 5MB limit + throw ExpenseError.invalidImageSize + } + // Add format validation here + } // Rest of the method...Splito/UI/Home/Expense/AddExpenseViewModel.swift (5)
Line range hint
86-171: Enhance log messages with additional context.While the logging implementation is good, consider adding more context to success messages for better debugging:
-LogD("AddExpenseViewModel: \(#function) Group fetched successfully.") +LogD("AddExpenseViewModel: \(#function) Group \(groupId) fetched successfully.") -LogD("AddExpenseViewModel: \(#function) Default user fetched successfully.") +LogD("AddExpenseViewModel: \(#function) Default user \(id) fetched successfully.") -LogD("AddExpenseViewModel: \(#function) Member fetched successfully.") +LogD("AddExpenseViewModel: \(#function) Member \(userId) fetched successfully.")
376-379: Extract image processing constants for better maintainability.Consider moving magic numbers into named constants at the class level.
+ private enum ImageConstants { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 + } - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let resizedImage = expenseImage?.aspectFittedToHeight(ImageConstants.maxHeight) + let imageData = resizedImage?.jpegData(compressionQuality: ImageConstants.compressionQuality)
433-438: Improve code formatting for better readability.Break down the long method call into multiple lines.
- let updatedExpense = try await expenseRepository.updateExpenseWithImage(imageData: imageData, newImageUrl: expenseImageUrl, group: group, expense: (expense, oldExpense), type: .expenseUpdated) + let updatedExpense = try await expenseRepository.updateExpenseWithImage( + imageData: imageData, + newImageUrl: expenseImageUrl, + group: group, + expense: (expense, oldExpense), + type: .expenseUpdated + )
452-457: Improve change detection readability.Consider breaking down the comparison into smaller, documented parts.
private func hasExpenseChanged(_ expense: Expense, oldExpense: Expense) -> Bool { - return oldExpense.name != expense.name || oldExpense.amount != expense.amount || - oldExpense.date.dateValue() != expense.date.dateValue() || oldExpense.paidBy != expense.paidBy || - oldExpense.imageUrl != expense.imageUrl || oldExpense.splitTo != expense.splitTo || - oldExpense.splitType != expense.splitType || oldExpense.splitData != expense.splitData || - oldExpense.isActive != expense.isActive + // Check basic properties + let basicPropertiesChanged = oldExpense.name != expense.name || + oldExpense.amount != expense.amount || + oldExpense.date.dateValue() != expense.date.dateValue() + + // Check payment details + let paymentDetailsChanged = oldExpense.paidBy != expense.paidBy || + oldExpense.imageUrl != expense.imageUrl + + // Check split details + let splitDetailsChanged = oldExpense.splitTo != expense.splitTo || + oldExpense.splitType != expense.splitType || + oldExpense.splitData != expense.splitData + + return basicPropertiesChanged || + paymentDetailsChanged || + splitDetailsChanged || + oldExpense.isActive != expense.isActive }
460-474: Enhance error logging with group context.Add group identifier to error logs for better debugging context.
- LogE("AddExpenseViewModel: \(#function) Failed to update member balance for expense \(expenseId): \(error).") + LogE("AddExpenseViewModel: \(#function) Failed to update member balance for expense \(expenseId) in group \(group.id ?? "unknown"): \(error).")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (44)
BaseStyle/BaseStyle/CustomUI/MailComposeView.swift(1 hunks)Data/Data/Extension/Query+Extension.swift(2 hunks)Data/Data/Helper/Firebase/StorageManager.swift(3 hunks)Data/Data/Helper/Logger/DDLogProvider.swift(2 hunks)Data/Data/Repository/ExpenseRepository.swift(3 hunks)Data/Data/Repository/GroupRepository.swift(4 hunks)Data/Data/Repository/TransactionRepository.swift(1 hunks)Data/Data/Repository/UserRepository.swift(1 hunks)Data/Data/Store/ActivityLogStore.swift(3 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(2 hunks)Data/Data/Store/ShareCodeStore.swift(3 hunks)Data/Data/Store/SplitoPreference.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Data/Data/Store/UserStore.swift(2 hunks)Data/Data/Utils/JSONUtils.swift(1 hunks)Splito/AppDelegate.swift(3 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(7 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(4 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(13 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(6 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift(3 hunks)Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift(2 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(7 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(7 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(5 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(4 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(6 hunks)Splito/UI/Login/LoginViewModel.swift(3 hunks)Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift(1 hunks)Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift(1 hunks)Splito/UI/Login/SignInWithAppleDelegate.swift(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- Data/Data/Helper/Logger/DDLogProvider.swift
- Data/Data/Repository/UserRepository.swift
- Data/Data/Store/TransactionStore.swift
- Data/Data/Utils/JSONUtils.swift
- Splito/AppDelegate.swift
- Splito/UI/Login/SignInWithAppleDelegate.swift
🚧 Files skipped from review as they are similar to previous changes (28)
- Data/Data/Helper/Firebase/StorageManager.swift
- Data/Data/Repository/GroupRepository.swift
- Data/Data/Repository/TransactionRepository.swift
- Data/Data/Store/ActivityLogStore.swift
- Data/Data/Store/UserStore.swift
- Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift
- Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift
- Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift
- Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
- Splito/UI/Home/Groups/GroupListViewModel.swift
- Splito/UI/Login/LoginViewModel.swift
- Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift
🔇 Additional comments (11)
Data/Data/Store/ShareCodeStore.swift (2)
19-22: LGTM! Robust error handling pattern.
The error handling implementation follows best practices:
- Errors are properly logged before propagation
- Original error context is preserved
- Consistent try-catch pattern across methods
Also applies to: 42-46
Line range hint 27-37: LGTM! Well-structured Firestore operations.
The Firestore implementation follows recommended patterns:
- Proper use of async/await
- Clean query construction
- Type-safe document decoding
Data/Data/Store/GroupStore.swift (2)
17-19: LGTM! Improved property naming
The rename from groupsCollection to groupReference better reflects its purpose as a Firestore collection reference. The computed property implementation ensures a fresh reference each time.
Line range hint 46-56: LGTM! Well-structured query implementation
The query implementation follows Firebase best practices with proper:
- Pagination
- Filtering
- Ordering
Data/Data/Store/ExpenseStore.swift (1)
24-30: Verify implementation in dependent code
The architectural change of splitting expense creation into two steps (getNewExpenseDocument and addExpense) is good, but requires careful implementation in dependent code.
Please ensure that:
- All callers properly handle errors from both steps
- The operations are performed atomically where necessary
- The transaction patterns are consistent across the codebase
✅ Verification successful
Let me gather more context about the error handling and transaction patterns.
Let me search for the complete implementation of the expense creation flow to verify error handling and transaction patterns.
Based on the code analysis, I can now generate the final response:
Implementation verified - error handling and atomicity are properly maintained
The split expense creation pattern is well-implemented with proper error handling and atomic operations:
- The
ExpenseRepository.addExpensemethod handles both steps (getNewExpenseDocumentandaddExpense) in a single transaction-like flow - Error propagation is maintained through async/throw pattern
- The implementation includes proper cleanup with activity logging
- The document reference is properly managed between the two operations
- Image upload is handled atomically within the same flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new expense creation pattern
rg -A 5 "getNewExpenseDocument|addExpense.*DocumentReference" --type swift
Length of output: 1358
Script:
#!/bin/bash
# Search for the complete implementation of addExpense in ExpenseRepository
rg -B 5 -A 15 "addExpense.*\{" --type swift
# Search for any transaction related code
rg -A 5 "transaction|atomic" --type swift
Length of output: 107737
Script:
#!/bin/bash
# Search for the complete implementation of addExpense in ExpenseRepository
rg -B 5 -A 20 "addExpense.*group.*expense.*imageData.*async" --type swift
# Search for any error handling patterns in the codebase
rg -A 5 "catch.*error" --type swift
Length of output: 3312
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (2)
67-67: LGTM! Well-structured logging implementation.
The added logging statements provide clear visibility into both success and failure scenarios, with appropriate log levels (Debug for success, Error for failures) and detailed context.
Also applies to: 69-69
93-93:
Standardize error handling approach.
While the logging implementation is good, there's an inconsistency in error handling between fetch methods:
fetchActivityLogsuseshandleServiceError()fetchMoreActivityLogsusesshowToastForError()
Apply this change to standardize the error handling:
} catch {
viewState = .initial
LogE("ActivityLogViewModel: \(#function) Failed to fetch more activity logs: \(error).")
- showToastForError()
+ handleServiceError()
}Also applies to: 96-97
Data/Data/Repository/ExpenseRepository.swift (3)
14-14: LGTM: Storage manager injection
The StorageManager dependency is properly injected to support the new image handling capabilities.
143-143: Previous comment about parallel error handling remains valid
While the error logging has been improved, the concern about handling multiple simultaneous failures in addActivityLogsInParallel is still applicable.
45-64: Consider potential race conditions in image updates
The current implementation might be susceptible to race conditions when multiple updates occur simultaneously. Consider implementing optimistic locking or version control for the image updates.
Let's check if there are any existing concurrent update protections:
Consider adding a version field to track image updates:
public func updateExpenseWithImage(imageData: Data?, newImageUrl: String?, group: Groups, expense: (new: Expense, old: Expense), type: ActivityType) async throws -> Expense {
var updatedExpense = expense.new
+ // Add version check here
+ guard updatedExpense.version == expense.old.version else {
+ throw ExpenseError.concurrentModification
+ }
// Rest of the method...Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
Line range hint 11-35: LGTM: Well-structured property declarations for image handling.
The new properties and imports are appropriately organized with proper access modifiers and clear naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
Data/Data/Store/GroupStore.swift (1)
38-41: Enhance logging for better observability.While error logging is present, consider adding success logging and improving error details for better debugging.
if let groupId = group.id { try groupReference.document(groupId).setData(from: group, merge: true) + LogI("GroupStore: Successfully updated group \(groupId)") } else { - LogE("GroupStore: \(#function) Group not found.") + LogE("GroupStore: Failed to update group - missing group ID") throw ServiceError.dataNotFound }Splito/UI/Home/Account/AccountHomeViewModel.swift (2)
103-103: Good logging enhancement, consider additional improvementsThe addition of
#functionimproves log traceability. Consider these enhancements:
- Add success logging for complete audit trail
- Structure the error log for better parsing
- Provide more specific error message to users based on
signOutErrorHere's a suggested improvement:
- LogE("AccountHomeViewModel: \(#function) Error signing out: \(signOutError)") + // Log error with structured format + LogE("AccountHomeViewModel.\(#function) failed: { error: \(signOutError.localizedDescription), code: \(signOutError.code) }") + // Show specific error to user + showToastFor(toast: ToastPrompt(type: .error, title: "Sign Out Failed", message: signOutError.localizedDescription)) // Add before the catch block for success logging + LogI("AccountHomeViewModel.\(#function) succeeded: User signed out successfully")
Line range hint
1-106: Consider architectural improvements for better maintainabilityWhile the current implementation is solid, consider these architectural improvements:
- Extract URL handling logic (privacy policy, acknowledgements, rate app) into a dedicated URLService
- Standardize error handling across all methods to follow the same pattern as
performLogoutAction- Consider using a Result type for operations that can fail
Example structure:
protocol URLService { func openURL(_ urlString: String) -> Result<Void, URLError> } // Then inject this service and use it in methods like: func handlePrivacyOptionTap() { switch urlService.openURL(Constants.privacyPolicyURL) { case .success: LogI("\(#function) succeeded: Opened privacy policy") case .failure(let error): LogE("\(#function) failed: \(error)") showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: error.localizedDescription)) } }Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
86-89: Include groupId in success log for consistencyThe error log includes the groupId but the success log doesn't. Consider including it for better traceability.
- LogD("AddExpenseViewModel: \(#function) Group fetched successfully.") + LogD("AddExpenseViewModel: \(#function) Group \(groupId) fetched successfully.")
121-124: Include expenseId in error log for better traceabilityConsider including the expenseId in the error log to make debugging easier.
- LogE("AddExpenseViewModel: \(#function) Failed to fetch expense details with members: \(error).") + LogE("AddExpenseViewModel: \(#function) Failed to fetch expense details with members for expense \(expenseId): \(error).")
Line range hint
423-445: Consider restructuring validation and improving logs
- The expenseId validation could be moved to the start of the method for early return.
- The success log could include more context about what was updated.
private func updateExpense(group: Groups, expense: Expense, oldExpense: Expense) async -> Bool { + guard let expenseId else { return false } + guard validateMembersInGroup(group: group, expense: expense) else { return false } - guard validateMembersInGroup(group: group, expense: expense), let expenseId else { - return false - } // ... rest of the method ... - LogD("AddExpenseViewModel: \(#function) Expense updated successfully.") + LogD("AddExpenseViewModel: \(#function) Expense \(expenseId) updated successfully with amount: \(expense.amount).")
460-464: Add error logging for validation failureConsider adding a log statement when the validation fails to help with debugging.
guard var group = selectedGroup, let expenseId = expense.id else { + LogE("AddExpenseViewModel: \(#function) Failed to update member balance: Invalid group or expense ID.") showLoader = false return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (37)
BaseStyle/BaseStyle/CustomUI/MailComposeView.swift(2 hunks)Data/Data/Extension/Query+Extension.swift(2 hunks)Data/Data/Helper/Firebase/StorageManager.swift(2 hunks)Data/Data/Helper/Logger/DDLogProvider.swift(2 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(1 hunks)Data/Data/Repository/TransactionRepository.swift(1 hunks)Data/Data/Repository/UserRepository.swift(1 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(1 hunks)Data/Data/Store/ShareCodeStore.swift(3 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Data/Data/Store/UserStore.swift(1 hunks)Data/Data/Utils/JSONUtils.swift(1 hunks)Splito/AppDelegate.swift(4 hunks)Splito/UI/Home/Account/AccountHomeViewModel.swift(1 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(10 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(9 hunks)Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(6 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift(3 hunks)Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift(2 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(9 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(8 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(5 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(6 hunks)Splito/UI/Login/LoginViewModel.swift(4 hunks)Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift(1 hunks)Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift(2 hunks)Splito/UI/Login/SignInWithAppleDelegate.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (34)
- BaseStyle/BaseStyle/CustomUI/MailComposeView.swift
- Data/Data/Extension/Query+Extension.swift
- Data/Data/Helper/Firebase/StorageManager.swift
- Data/Data/Helper/Logger/DDLogProvider.swift
- Data/Data/Repository/ExpenseRepository.swift
- Data/Data/Repository/GroupRepository.swift
- Data/Data/Repository/TransactionRepository.swift
- Data/Data/Repository/UserRepository.swift
- Data/Data/Store/ExpenseStore.swift
- Data/Data/Store/ShareCodeStore.swift
- Data/Data/Store/TransactionStore.swift
- Data/Data/Store/UserStore.swift
- Data/Data/Utils/JSONUtils.swift
- Splito/AppDelegate.swift
- Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift
- Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift
- Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift
- Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
- Splito/UI/Home/Groups/GroupListViewModel.swift
- Splito/UI/Login/LoginViewModel.swift
- Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift
- Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift
- Splito/UI/Login/SignInWithAppleDelegate.swift
🔇 Additional comments (5)
Data/Data/Store/GroupStore.swift (1)
38-38: LGTM! Safer merge strategy implemented.
The change to merge: true is a good improvement that prevents accidental field deletion during updates.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
105-108: LGTM!
The logging statements provide appropriate context for both success and error cases.
154-158: LGTM!
The logging statements provide appropriate context for both success and error cases.
167-171: LGTM!
The logging statements provide appropriate context for both success and error cases.
469-472: LGTM!
The logging statements provide appropriate context for both success and error cases.
cp-amisha-i
force-pushed
the
add-logs-in-the-app
branch
from
257d19b to
4d83ca4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
121-124: Enhance logging with expense ID for better traceabilityConsider including the expense ID in the logs to make debugging easier.
- LogD("AddExpenseViewModel: \(#function) Expense details with members fetched successfully.") + LogD("AddExpenseViewModel: \(#function) Expense details with members fetched successfully for expense \(expenseId).") - LogE("AddExpenseViewModel: \(#function) Failed to fetch expense details with members: \(error).") + LogE("AddExpenseViewModel: \(#function) Failed to fetch expense details with members for expense \(expenseId): \(error).")
392-396: Enhance logging with additional contextConsider including the group ID and expense amount in the logs for better traceability.
- LogD("AddExpenseViewModel: \(#function) Expense added successfully.") + LogD("AddExpenseViewModel: \(#function) Expense added successfully for group \(groupId) with amount \(expense.amount).") - LogE("AddExpenseViewModel: \(#function) Failed to add expense: \(error).") + LogE("AddExpenseViewModel: \(#function) Failed to add expense for group \(groupId): \(error).")
Line range hint
423-445: Consider moving validation check earlierThe validation check for
expenseIdcould be moved to the start of the function to fail fast and avoid unnecessary processing.private func updateExpense(group: Groups, expense: Expense, oldExpense: Expense) async -> Bool { + guard let expenseId else { return false } + guard validateMembersInGroup(group: group, expense: expense) else { return false } - guard validateMembersInGroup(group: group, expense: expense), let expenseId else { - return false - } let resizedImage = expenseImage?.aspectFittedToHeight(200) let imageData = resizedImage?.jpegData(compressionQuality: 0.2)
460-464: Separate validation checks for better error handlingConsider separating the validation checks and adding specific error messages for each failure case.
- guard var group = selectedGroup, let expenseId = expense.id else { + guard var group = selectedGroup else { + LogE("AddExpenseViewModel: \(#function) Selected group is nil") showLoader = false return } + guard let expenseId = expense.id else { + LogE("AddExpenseViewModel: \(#function) Expense ID is nil") + showLoader = false + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (45)
BaseStyle/BaseStyle/CustomUI/MailComposeView.swift(2 hunks)Data/Data/Extension/Query+Extension.swift(2 hunks)Data/Data/Helper/Firebase/StorageManager.swift(2 hunks)Data/Data/Helper/Logger/DDLogProvider.swift(2 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(1 hunks)Data/Data/Repository/TransactionRepository.swift(1 hunks)Data/Data/Repository/UserRepository.swift(1 hunks)Data/Data/Store/ActivityLogStore.swift(2 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(1 hunks)Data/Data/Store/ShareCodeStore.swift(3 hunks)Data/Data/Store/SplitoPreference.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Data/Data/Store/UserStore.swift(1 hunks)Data/Data/Utils/JSONUtils.swift(1 hunks)Splito/AppDelegate.swift(4 hunks)Splito/UI/Home/Account/AccountHomeViewModel.swift(1 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(10 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(9 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(6 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift(3 hunks)Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift(2 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(9 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(8 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(5 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(6 hunks)Splito/UI/Login/LoginViewModel.swift(4 hunks)Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift(1 hunks)Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift(2 hunks)Splito/UI/Login/SignInWithAppleDelegate.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (44)
- BaseStyle/BaseStyle/CustomUI/MailComposeView.swift
- Data/Data/Extension/Query+Extension.swift
- Data/Data/Helper/Firebase/StorageManager.swift
- Data/Data/Helper/Logger/DDLogProvider.swift
- Data/Data/Repository/ExpenseRepository.swift
- Data/Data/Repository/GroupRepository.swift
- Data/Data/Repository/TransactionRepository.swift
- Data/Data/Repository/UserRepository.swift
- Data/Data/Store/ActivityLogStore.swift
- Data/Data/Store/ExpenseStore.swift
- Data/Data/Store/GroupStore.swift
- Data/Data/Store/ShareCodeStore.swift
- Data/Data/Store/SplitoPreference.swift
- Data/Data/Store/TransactionStore.swift
- Data/Data/Store/UserStore.swift
- Data/Data/Utils/JSONUtils.swift
- Splito/AppDelegate.swift
- Splito/UI/Home/Account/AccountHomeViewModel.swift
- Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift
- Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift
- Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift
- Splito/UI/Home/Groups/Add Member/JoinMemberViewModel.swift
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift
- Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
- Splito/UI/Home/Groups/GroupListViewModel.swift
- Splito/UI/Login/LoginViewModel.swift
- Splito/UI/Login/PhoneLogin/PhoneLoginViewModel.swift
- Splito/UI/Login/PhoneLogin/VerifyOtp/VerifyOtpViewModel.swift
- Splito/UI/Login/SignInWithAppleDelegate.swift
🔇 Additional comments (3)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (3)
105-108: LGTM! Appropriate logging implementation.
The logging implementation provides good traceability for both success and error cases.
Line range hint 154-171: LGTM! Consistent logging implementation.
The logging implementation is consistent across both methods and provides good traceability with relevant IDs.
469-472: LGTM! Well-implemented logging.
The logging implementation provides good traceability with relevant context.
Summary by CodeRabbit
New Features
expenseadded for storing expense-related images, with updated logging for image upload and deletion.Bug Fixes
addActivityLogmethod across multiple repositories for better traceability.Documentation