Fix data update issue #80
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the formatting of monetary values in the UI, improving error handling, and refining data fetching logic. Key changes include the adoption of a streamlined currency formatting method in Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (64)
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: 1
🧹 Outside diff range and nitpick comments (2)
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift (1)
Line range hint
78-85: Consider standardizing numeric value formatting across all cases.While the fixed amount case uses
formattedCurrency, percentage and shares cases still useString(format:). Consider creating similar formatted properties for consistency:case .percentage: - BottomInfoCardView(title: "\(String(format: "%.0f", viewModel.totalPercentage))% of 100%", + BottomInfoCardView(title: "\(viewModel.totalPercentage.formattedPercentage)% of 100%", value: "\(String(format: "%.0f", 100 - viewModel.totalPercentage))% left") case .shares: - BottomInfoCardView(title: "\(String(format: "%.0f", viewModel.totalShares)) total shares") + BottomInfoCardView(title: "\(viewModel.totalShares.formattedShares) total shares")Splito/UI/Home/Groups/GroupListView.swift (1)
130-130: Consider optimizing the onAppear behaviorThe current implementation might trigger unnecessary user fetches on every view appearance. Consider:
- Adding a flag to prevent redundant fetches
- Showing loading state during fetch
- Displaying error feedback to users
Consider this approach:
- .onAppear(perform: viewModel.fetchCurrentUser) + .onAppear { + // Only fetch if user data is stale or not present + if viewModel.shouldRefreshUserData { + viewModel.fetchCurrentUser() + } + } + .overlay { + if case .loading = viewModel.currentViewState { + LoaderView() + } + }Add a computed property in the ViewModel:
var shouldRefreshUserData: Bool { // Add logic to determine if refresh is needed return preference.user == nil || Date().timeIntervalSince(lastUserFetch) > refreshInterval }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift(1 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift(1 hunks)Splito/UI/Home/Groups/GroupListView.swift(1 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(1 hunks)
🔇 Additional comments (2)
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift (1)
78-78: LGTM! Currency formatting improvement.
The use of formattedCurrency property improves clarity and consistency in monetary value display.
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift (1)
48-49: LGTM! The currency formatting change improves code maintainability.
The switch to using formattedCurrency and formattedCurrencyWithSign methods makes the code more maintainable and consistent.
Let's verify the currency formatting implementation and its usage across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Currency formatting implementation is consistent across the codebase
The verification shows:
formattedCurrencyandformattedCurrencyWithSignare consistently used throughout the app for currency display- Both formatters are centrally defined in
Double+Extension.swiftusingNumberFormatter - No inconsistent or legacy currency formatting patterns were found
The change aligns with the established pattern in the codebase and maintains consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check currency formatting consistency across the codebase
# Look for different currency formatting patterns to ensure consistency
# Check for various currency formatting patterns
echo "Checking currency formatting patterns..."
rg -t swift "formattedCurrency|String\(format:.+\d+\.\d+f.+\)"
# Look for other potential currency-related formatting
echo "Checking for other currency formatting methods..."
rg -t swift "NumberFormatter|currencyFormatter|formatCurrency"
Length of output: 5508
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (19)
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift (1)
38-38: LGTM! Consider adding thread synchronization.The direct assignment simplifies the code and maintains proper error handling. However, consider adding thread synchronization if this method might be called from multiple places in the future.
You could make the property access thread-safe by using a serial queue or actor:
- private var group: Groups? + private let queue = DispatchQueue(label: "com.splito.grouptotals") + private var _group: Groups? + private var group: Groups? { + get { queue.sync { _group } } + set { queue.sync { _group = newValue } } + }Or with modern concurrency, consider refactoring the class to be an actor:
actor GroupTotalsViewModel: BaseViewModel, ObservableObject { // ... rest of the implementation }Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (3)
Line range hint
55-59: LGTM! Consider enhancing error logging.The direct assignment and state management look good. Consider including more error details in the error log by using error.localizedDescription for better debugging.
- LogE("GroupTransactionDetailViewModel: \(#function) Failed to fetch group \(groupId): \(error).") + LogE("GroupTransactionDetailViewModel: \(#function) Failed to fetch group \(groupId): \(error.localizedDescription)")
78-81: LGTM! Consider adding debug logging for nil transaction case.The nil check and early return pattern are well implemented. Consider adding a debug log when transaction is nil for better traceability.
guard let transaction else { + LogD("GroupTransactionDetailViewModel: \(#function) Transaction is nil, returning early.") viewState = .initial return }
Line range hint
1-300: Consider adding synchronization for shared state modifications.The view model contains multiple async operations that modify shared state (
viewState,transaction, etc.). Consider implementing proper synchronization to prevent potential race conditions. You could:
- Use an actor to encapsulate the shared state
- Use a serial dispatch queue for state modifications
- Implement @mainactor for UI state updates
Example implementation using @mainactor:
@MainActor class GroupTransactionDetailViewModel: BaseViewModel, ObservableObject { // ... existing code ... }This ensures all state modifications happen on the main thread, preventing race conditions.
Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift (3)
44-44: LGTM! Consider making the success state more explicit.The simplified group fetching logic improves code clarity. However, we could make the success state more explicit by updating the view state before the assignment.
Consider this minor improvement:
- self.group = try await groupRepository.fetchGroupBy(id: groupId) - viewState = .initial + let fetchedGroup = try await groupRepository.fetchGroupBy(id: groupId) + viewState = .initial + self.group = fetchedGroupThis makes it clearer that we're only updating the view state after a successful fetch, and helps prevent any potential race conditions.
Line range hint
53-63: Add retry limit to prevent potential infinite recursion.The current implementation of
generateInviteCode()uses recursion for retries without a limit, which could lead to a stack overflow if many codes are already taken.Consider implementing a retry limit:
- private func generateInviteCode() async { + private func generateInviteCode(retryCount: Int = 0, maxRetries: Int = 5) async { do { viewState = .loading inviteCode = inviteCode.randomString(length: 6).uppercased() let isAvailable = try await codeRepository.checkForCodeAvailability(code: inviteCode) - if !isAvailable { - await generateInviteCode() + if !isAvailable && retryCount < maxRetries { + await generateInviteCode(retryCount: retryCount + 1, maxRetries: maxRetries) + } else if !isAvailable { + throw NSError(domain: "InviteMemberViewModel", code: -1, + userInfo: [NSLocalizedDescriptionKey: "Failed to generate unique code after \(maxRetries) attempts"]) } viewState = .initial LogD("InviteMemberViewModel: \(#function) Invite code generated successfully.")
Line range hint
74-85: Align error handling with the rest of the codebase.The error handling in
storeSharedCode()differs from other methods by directly managingshowLoaderinstead of usingviewState. This inconsistency could make the code harder to maintain.Consider aligning with the established pattern:
private func storeSharedCode(completion: (Bool) -> Void) async { let shareCode = SharedCode(code: inviteCode.encryptHexCode(), groupId: groupId, expireDate: Timestamp()) do { - showLoader = true + viewState = .loading try await codeRepository.addSharedCode(sharedCode: shareCode) - showLoader = false + viewState = .initial LogD("InviteMemberViewModel: \(#function) Shared code stored successfully.") completion(true) } catch { - showLoader = false + handleServiceError() LogE("InviteMemberViewModel: \(#function) Failed to store shared code: \(error).") - showToastForError() completion(false) } }Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
104-105: Consider explicit timestamp managementThe group creation relies on the default Timestamp() from the Groups initializer. For better traceability, consider explicitly setting both createdAt and updatedAt to ensure they're identical at creation time.
- let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, updatedBy: userId, - members: [userId], balances: [memberBalance], createdAt: Timestamp()) + let timestamp = Timestamp() + let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, updatedBy: userId, + members: [userId], balances: [memberBalance], createdAt: timestamp, updatedAt: timestamp)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
Line range hint
146-162: Consider adding unit tests for member validationThe validateGroupMembers function handles important edge cases for expense deletion. Consider adding unit tests to verify behavior with:
- Empty groups
- All members present
- Some members missing
- All members missing
Would you like me to help create unit tests for these scenarios?
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (1)
295-298: Consider enhancing error handling for transaction updates.While the optional binding is a good practice, consider adding specific error handling for cases where the transaction is nil. This would help in debugging and provide better feedback to users.
- if let transaction = self.transaction { - guard hasTransactionChanged(transaction, oldTransaction: oldTransaction) else { return true } - await updateGroupMemberBalance(updateType: .Update(oldTransaction: oldTransaction)) - } + guard let transaction = self.transaction else { + LogE("GroupPaymentViewModel: Transaction was nil after update") + showToastFor(toast: ToastPrompt(type: .error, + title: "Error", + message: "Failed to process transaction update")) + return false + } + + guard hasTransactionChanged(transaction, oldTransaction: oldTransaction) else { + return true + } + + await updateGroupMemberBalance(updateType: .Update(oldTransaction: oldTransaction))Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (4)
57-64: LGTM! Consider standardizing error state handling.The error handling and logging improvements look good. However, for consistency, consider setting
currentViewState = .somethingWentWrongin the error case instead of delegating tohandleServiceError(), as this would align with the direct state setting pattern used in the success case.- handleServiceError() + currentViewState = .somethingWentWrong + LogE("GroupTransactionListViewModel: \(#function) Failed to fetch group \(groupId): \(error).")
Line range hint
143-159: Enhance error handling and type safety in user data fetching.While the caching mechanism is a good optimization, there are a few areas for improvement:
- The error case sets
currentViewState = .initialwhich might not be the most appropriate state for an error condition- The function could benefit from more specific error types for better error handling downstream
Consider this improved implementation:
private func fetchUserData(for userId: String) async -> AppUser? { if let existingUser = groupMembers.first(where: { $0.id == userId }) { return existingUser } else { do { let user = try await groupRepository.fetchMemberBy(userId: userId) if let user { groupMembers.append(user) } LogD("GroupTransactionListViewModel: \(#function) Member fetched successfully.") return user } catch { - currentViewState = .initial + currentViewState = .somethingWentWrong LogE("GroupTransactionListViewModel: \(#function) Failed to fetch member \(userId): \(error).") - showToastForError() + showToastFor(toast: .init( + type: .error, + title: "Error", + message: "Failed to fetch member details. Please try again." + )) return nil } } }
Line range hint
171-186: Improve alert handling and async patterns in group validation.The current implementation has potential timing issues and could benefit from better async/await usage:
- The 0.1s delay for showing alerts is arbitrary and could cause race conditions
- The alert message could be more concise while maintaining clarity
Consider this improved implementation:
private func validateGroupMembers(transaction: Transactions) -> Bool { guard let group else { LogE("GroupTransactionListViewModel: \(#function) Missing required group.") return false } if !group.members.contains(transaction.payerId) || !group.members.contains(transaction.receiverId) { - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - self.showAlertFor(message: "This payment involves a person who has left the group, and thus it can no longer be deleted. If you wish to change this payment, you must first add that person back to your group.") - } + @MainActor func showValidationAlert() { + self.showAlertFor(message: "Cannot delete payment: One or more members have left the group. Add them back to modify this payment.") + } + Task { @MainActor in + showValidationAlert() + } return false } return true }
Line range hint
1-324: Consider implementing a consistent state management pattern.While the individual changes improve error handling, consider implementing a more consistent state management pattern across the view model:
- Create a dedicated error type enum for better error handling
- Implement a consistent pattern for state transitions
- Consider using a Result type for better error propagation
This would make the code more maintainable and easier to debug.
Example error type implementation:
enum TransactionError: Error { case groupNotFound case memberNotFound(userId: String) case networkError(Error) case validationError(String) var userMessage: String { switch self { case .groupNotFound: return "Group not found" case .memberNotFound(let userId): return "Member not found: \(userId)" case .networkError: return "Network error occurred" case .validationError(let message): return message } } }Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (1)
Line range hint
42-50: Consider improving the optional handling patternThe current implementation has a few areas that could be enhanced:
- Using optional shadowing in the guard statement (
group) can be confusing. Consider using a more descriptive name.- The success log message is placed after state updates, which might not accurately reflect the success state.
- The error handling could be more specific about the type of error.
Consider this alternative implementation:
- self.group = try await groupRepository.fetchGroupBy(id: groupId) - guard let group else { - viewState = .initial - return - } - calculateMemberPayableAmount(group: group) - viewState = .initial - LogD("GroupSettleUpViewModel: \(#function) Group fetched successfully.") + let fetchedGroup = try await groupRepository.fetchGroupBy(id: groupId) + guard let fetchedGroup else { + LogE("GroupSettleUpViewModel: \(#function) Group not found for id: \(groupId)") + viewState = .initial + return + } + LogD("GroupSettleUpViewModel: \(#function) Group fetched successfully.") + self.group = fetchedGroup + calculateMemberPayableAmount(group: fetchedGroup) + viewState = .initialThis refactoring:
- Uses a clearer name for the optional value
- Adds more specific logging for the nil case
- Moves the success log before state updates
- Maintains better testability by using an intermediate variable
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (2)
Line range hint
47-56: Consider enhancing error handling robustness.While the current implementation works, consider these improvements to prevent potential issues:
- Clear group data in error cases to prevent stale state
- Add more specific error handling based on error types
Here's a suggested improvement:
private func fetchGroupDetails() async { do { self.group = try await groupRepository.fetchGroupBy(id: groupId) self.checkForGroupAdmin() await fetchGroupMembers() currentViewState = .initial LogD("GroupSettingViewModel: \(#function) Group fetched successfully.") } catch { + self.group = nil // Clear stale data + // More specific error handling + switch error { + case let repositoryError as RepositoryError: + LogE("GroupSettingViewModel: \(#function) Repository error: \(repositoryError)") + default: + LogE("GroupSettingViewModel: \(#function) Failed to fetch group \(groupId): \(error).") + } handleServiceError() } }
Line range hint
236-254: Add balance validation before group deletion.Similar to member removal, consider validating that all balances are settled before allowing group deletion to prevent data inconsistency.
Consider adding a validation method:
private func deleteGroup() { guard let group else { return } + + // Check if any member has outstanding balance + guard group.balances.allSatisfy({ $0.balance == 0 }) else { + alert = .init( + title: "Cannot Delete Group", + message: "There are outstanding balances in the group. Please ensure all debts are settled before deleting the group.", + negativeBtnTitle: "Ok", + negativeBtnAction: { self.showAlert = false } + ) + showAlert = true + return + } Task { do { currentViewState = .loadingSplito/UI/Home/Expense/AddExpenseViewModel.swift (2)
Line range hint
433-452: Consider refactoring expense update logic for better maintainabilityThe manual field copying in
handleUpdateExpenseActioncould be prone to errors when new fields are added. Consider introducing a copy constructor or builder pattern.Example approach:
+ extension Expense { + func copyWith( + name: String? = nil, + amount: Double? = nil, + date: Timestamp? = nil, + updatedAt: Timestamp? = nil, + updatedBy: String? = nil, + note: String? = nil, + paidBy: [String: Double]? = nil, + splitType: SplitType? = nil, + splitTo: [String]? = nil, + splitData: [String: Double]? = nil + ) -> Expense { + var copy = self + copy.name = name ?? self.name + copy.amount = amount ?? self.amount + // ... copy remaining fields + return copy + } + } - var newExpense = expense - newExpense.name = expenseName.trimming(spaces: .leadingAndTrailing) - newExpense.amount = expenseAmount - // ... remaining field assignments + let newExpense = expense.copyWith( + name: expenseName.trimming(spaces: .leadingAndTrailing), + amount: expenseAmount, + date: Timestamp(date: expenseDate), + // ... remaining fields + )
Line range hint
486-490: Consider extracting image processing constantsThe hardcoded values for image processing could be made configurable for better maintainability and flexibility.
Example approach:
+ private enum ImageConfig { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 + } private func getImageData() -> Data? { - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let resizedImage = expenseImage?.aspectFittedToHeight(ImageConfig.maxHeight) + let imageData = resizedImage?.jpegData(compressionQuality: ImageConfig.compressionQuality) return imageData }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
Data/Data/Model/Groups.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(1 hunks)Splito/UI/Home/Groups/Add Member/InviteMemberViewModel.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Groups/GroupListViewModel.swift
🔇 Additional comments (8)
Data/Data/Model/Groups.swift (1)
26-26: Consider implications of default Timestamp() for updatedAt
While adding a default value simplifies initialization, it could potentially mask cases where updatedAt should be explicitly set. This might lead to incorrect update timestamps if not used carefully.
Let's verify the usage of this parameter across the codebase:
✅ Verification successful
Default Timestamp() for updatedAt is appropriately managed
Based on the codebase analysis:
- The
Groupsinitialization with defaultupdatedAtis only used inCreateGroupViewModel.swiftduring group creation - All subsequent updates explicitly set
updatedAtto current timestamp inGroupRepository.swift - This pattern is consistent with other models (
Transaction,Expense) that follow the same approach - The default value simplifies object creation while maintaining data consistency through explicit updates in repositories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Groups initialization to ensure updatedAt is explicitly set where needed
ast-grep --pattern 'Groups(name: $_, createdBy: $_, updatedBy: $_, $$$)'
# Search for places where updatedAt should be explicitly managed
rg -g '*.swift' 'updatedAt.*=.*Timestamp'
Length of output: 1792
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
140-140: LGTM! Error logging format is now consistent
The addition of a period at the end of the error message maintains consistency with other error logs in the codebase.
Data/Data/Repository/GroupRepository.swift (1)
117-124: LGTM! The changes improve data integrity and audit trail.
The updates to deleteGroup method enhance data tracking by:
- Validating the user before proceeding
- Setting proper metadata (updatedBy, updatedAt)
- Implementing soft delete by marking the group as inactive
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (2)
127-127: LGTM! Code simplification improves readability.
The direct assignment of fetched users to properties simplifies the code while maintaining the same functionality and error handling.
Also applies to: 139-139
295-298: Verify transaction update notifications.
The transaction update notification is posted before verifying if the transaction actually changed. This could lead to unnecessary UI updates.
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (1)
Line range hint 42-50: Verify error handling coverage
The current error handling might miss specific error cases that could provide better feedback to users.
Let's check if there are specific error types defined in the codebase that should be handled:
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (1)
48-48: LGTM! Code simplification aligns with codebase patterns.
The direct assignment to self.group improves code clarity by removing unnecessary intermediate variables.
Let's verify if this pattern is consistently applied across other view models:
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
392-392: LGTM! Important fix for balance calculations
Good change to use newExpense instead of the input expense parameter. This ensures that member balances are calculated using the most up-to-date expense data returned from the server, which may include additional fields or modifications made during the save process.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Data/Data/Model/Groups.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(3 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Groups/GroupListViewModel.swift
- Data/Data/Model/Groups.swift
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
🔇 Additional comments (1)
Data/Data/Repository/GroupRepository.swift (1)
119-122: LGTM! Proper audit trail implementation
The implementation correctly:
- Maintains soft deletion pattern by setting
isActiveto false - Updates audit fields (
updatedByandupdatedAt) for tracking changes
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift
Outdated
Show resolved
Hide resolved
...ps/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Data/Data/Store/TransactionStore.swift (1)
34-34: Document the rationale for disabling mergeSince this appears to be part of a broader change across store files, please add a comment explaining why merge is explicitly disabled. This will help future maintainers understand the reasoning behind this potentially destructive operation.
+ // Disable merge to ensure complete document overwrite and prevent partial update issues try transactionReference(groupId: groupId).document(transactionId).setData(from: transaction, merge: false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Data/Data/Store/UserStore.swift(1 hunks)
🔇 Additional comments (4)
Data/Data/Store/TransactionStore.swift (1)
34-34: Verify transaction object completeness before update
The change from merge: true to merge: false means the entire document will be overwritten instead of merged. This could lead to data loss if the transaction object doesn't contain all required fields.
Data/Data/Store/GroupStore.swift (1)
38-38: Verify complete model data before switching to non-merge updates
Changing from merge: true to merge: false means the entire document will be overwritten instead of merged. While this might fix the reported "0 spending" issue, it could lead to data loss if the Groups model doesn't include all fields that might exist in Firestore.
Consider implementing these safeguards:
- Add a migration strategy if the model structure has changed
- Implement optimistic locking to prevent concurrent update issues
- Add logging to track any data discrepancies during updates
✅ Verification successful
Let me gather more information about the usage of this model to ensure we understand the complete context.
Switching to non-merge updates is safe for this model
The Groups model is complete and includes all fields used in Firestore:
- Core fields: id, name, createdBy, updatedBy, imageUrl, members, balances, createdAt
- Status field: isActive (confirmed by model and queries)
- All fields are properly mapped through CodingKeys
The change from merge: true to merge: false is safe because:
- The model captures all fields used in Firestore operations
- No direct field updates found except for members array union, which has a dedicated method
- The isActive field is properly handled through the model, not through direct updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all properties of Groups model to ensure completeness
rg -A 10 "struct Groups"
# Search for any Firestore field access to find potentially missing fields
rg "whereField\(\".*?\""
Length of output: 1188
Script:
#!/bin/bash
# Check for any additional fields being set in GroupStore
ast-grep --pattern 'setData(from: $_'
# Look for any direct field updates that might indicate additional fields
rg "updateData\(\[" -A 2
# Check if there's any migration code or field mapping
rg "is_active|isActive" --type swift
Length of output: 4736
Data/Data/Store/UserStore.swift (1)
31-31: Ensure real-time listener handles non-merge updates correctly
The change to merge: false aligns with other stores but requires special attention due to real-time listener implementation. This could affect user preferences updates mentioned in the PR objectives.
Recommendations:
- Add error recovery mechanism in
fetchLatestUserByto handle potential data inconsistencies - Consider adding a validation step before updates to ensure all required fields are present
- Add telemetry to monitor user data update patterns
✅ Verification successful
Real-time listener implementation is robust for non-merge updates
The real-time listener in fetchLatestUserBy already has comprehensive error handling that covers data inconsistencies:
- Handles snapshot errors with proper logging
- Validates snapshot existence
- Handles decoding errors for user data
- Returns nil in all error cases, allowing graceful fallbacks
The change to merge: false is safe as the UserProfileViewModel ensures data completeness by:
- Creating a complete user copy before updates
- Properly trimming and formatting all fields
- Handling image updates separately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for user preference related fields
rg -A 5 "preference|settings|config"
# Check for any error handling around user updates
rg -A 10 "catch|error|throw" --type swift
Length of output: 65618
Script:
#!/bin/bash
# Search for real-time listener implementation and error handling in UserStore
ast-grep --pattern 'func fetchLatestUserBy($$$) {
$$$
}'
# Look for user model definition and fields
ast-grep --pattern 'struct User {
$$$
}'
# Search for any user update related error handling
rg -A 5 "updateUser|setData.*user" --type swift
Length of output: 7666
Data/Data/Store/ExpenseStore.swift (1)
34-34:
Verify expense calculations after switching to non-merge updates
This change likely addresses the reported issues with zero spending and incorrect totals. However, we need to ensure consistency between expenses and transactions.
Before deploying:
- Verify that all expense amounts are properly calculated and stored
- Ensure transaction records are updated consistently with expenses
- Consider adding a data validation step to verify no zero amounts are saved unless intentional
Consider implementing:
- Data integrity checks between expenses and transactions
- Automated tests for amount calculations
- Monitoring for zero-amount expenses to detect potential issues early
✅ Verification successful
Based on the search results, I can see that the codebase has robust handling of expense and transaction calculations, with proper balance updates and data consistency mechanisms. The key findings:
- The
getUpdatedMemberBalanceForfunction properly handles both expense and transaction updates, maintaining consistency between them - The non-merge update in ExpenseStore is safe because the balance calculations are properly handled through the
calculateExpensesSimplifiedfunction - The codebase has proper validation of amounts and balance updates across both expenses and transactions
Let me verify one final aspect about the expense-transaction relationship:
Let me try a different search pattern to verify the expense update handling:
Based on the comprehensive code analysis, I can now provide a final response:
Non-merge update in ExpenseStore is safe and appropriate
The change to use merge: false in ExpenseStore is correct and safe because:
-
The expense update flow has proper validation:
- Validates member existence in the group
- Validates expense amounts and splits
- Maintains data consistency through the repository layer
-
The balance calculations are handled separately through
getUpdatedMemberBalanceForwhich:- Properly updates group member balances
- Maintains monthly summaries
- Handles both expense and transaction updates consistently
-
The non-merge update ensures complete expense data is written, preventing partial updates that could lead to data inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for amount calculation logic
rg -A 10 "total|amount|spending|calculate" --type swift
# Check for transaction-expense relationships
rg -A 5 "transaction.*expense|expense.*transaction" --type swift
# Look for any existing data migration or update scripts
fd -e swift -e sh -e py -E "test" -E "mock" "migration|update|script"
Length of output: 125159
Script:
#!/bin/bash
# Check for transaction updates when expenses change
ast-grep --pattern 'func updateExpense($$$) {
$$$
transaction
$$$
}'
# Look for expense validation before saving
ast-grep --pattern 'func validateExpense($$$) {
$$$
amount
$$$
}'
Length of output: 150
Script:
#!/bin/bash
# Check expense update implementation
rg -A 10 "func.*updateExpense" --type swift
# Check expense validation
rg -A 10 "func.*validate.*expense" --type swift --ignore-case
# Look for expense-transaction relationship in the store
rg -A 10 "class ExpenseStore" --type swift
Length of output: 9032
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (2)
Line range hint
52-57: Enhance error handling and logging in calculateMemberPayableAmount.While the guard statement correctly handles nil cases, consider:
- Adding error logging when group or userId is nil for better debugging
- Setting a more appropriate viewState (e.g., .somethingWentWrong) when preconditions fail
func calculateMemberPayableAmount() { guard let group, let userId = preference.user?.id else { + LogE("GroupSettleUpViewModel: Failed to calculate member payable amount. Group: \(group == nil ? "nil" : "exists"), UserId: \(preference.user?.id ?? "nil")") - viewState = .initial + viewState = .somethingWentWrong return } memberOwingAmount = calculateExpensesSimplified(userId: userId, memberBalances: group.balances) }
Line range hint
1-124: Consider extracting balance calculation logic to a separate service.The view model currently handles multiple responsibilities. Consider extracting the balance calculation logic (
calculateExpensesSimplifiedand related methods) into a dedicated service class to improve maintainability and testability.This would:
- Reduce the view model's complexity
- Make the balance calculation logic reusable
- Facilitate unit testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Groups/GroupListViewModel.swift
🔇 Additional comments (1)
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (1)
42-43: LGTM! Code flow has been simplified.
The direct assignment of the fetched group and immediate calculation improves code clarity by removing unnecessary intermediate steps.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift (1)
196-196: LGTM with a minor suggestionThe conditional bottom padding provides better visual balance for the last item. Consider extracting the magic numbers (13 and 20) into named constants for better maintainability.
+ private enum Layout { + static let lastItemBottomPadding: CGFloat = 13 + static let defaultBottomPadding: CGFloat = 20 + } - .padding(.bottom, isLastCell ? 13 : 20) + .padding(.bottom, isLastCell ? Layout.lastItemBottomPadding : Layout.defaultBottomPadding)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift(3 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(3 hunks)
🔇 Additional comments (5)
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift (2)
130-130: LGTM: Padding adjustment for compact layout
The reduction in vertical padding from 8 to 5 helps achieve a more compact layout while maintaining readability.
155-155: LGTM: Improved spacing control
Setting VStack spacing to 0 and relying on explicit padding values provides better control over the layout. This change allows for more precise spacing management.
Let's verify the layout consistency across different devices:
✅ Verification successful
LGTM: Consistent spacing pattern confirmed
The codebase shows a consistent pattern where VStack spacing is set to 0 when explicit padding is used for fine-grained control, particularly in list views and transaction-related interfaces. This change aligns with the established pattern across the codebase, especially in similar list/transaction views like GroupExpenseListView and GroupTransactionListView.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other VStack implementations in the codebase to ensure consistent spacing patterns
rg "VStack.*spacing:" --type swift
Length of output: 17077
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (3)
128-128: LGTM: Minor UI spacing adjustment
The reduction in vertical padding from 8 to 5 helps achieve better visual density in the section headers.
176-176: Verify visual alignment of expense items
The spacing adjustments look good:
- Setting VStack spacing to 0 for better control
- Adding conditional bottom padding for the last item
- Adding divider for non-last items
These changes should improve the consistency of expense item spacing.
Could you please verify the visual appearance by testing with:
- Multiple expense items
- Single expense item
- Different screen sizes
Also applies to: 244-249
Line range hint 332-348: Consider handling edge cases in spending calculations
The current month spending display logic could benefit from additional validation:
- Ensure
currentMonthSpendingis properly initialized in the view model - Consider adding a loading state while data is being fetched
- Add visual feedback for truly zero spending vs. loading state
This would help prevent the issue mentioned in the PR description where the group was showing 0 spending.
Let's verify the spending calculation logic:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (3)
133-133: Maintain parameter naming consistencyThe method parameter is named
userIdbut the repository call usesmemberId. Consider updating the parameter name for consistency:-private func fetchUserData(for userId: String) async -> AppUser? { +private func fetchUserData(for memberId: String) async -> AppUser? { if let existingUser = groupMembers.first(where: { $0.id == userId }) { return existingUser } else { do { - let user = try await groupRepository.fetchMemberBy(memberId: userId) + let user = try await groupRepository.fetchMemberBy(memberId: memberId)
133-133: Add documentation for caching behaviorThe method implements a caching mechanism using
groupMembers, but this behavior isn't documented. Consider adding documentation to explain this optimization:+/// Fetches user data for a given member ID, utilizing a cache to minimize network calls. +/// - Parameter userId: The ID of the member to fetch +/// - Returns: The fetched AppUser object, or nil if the fetch fails +/// - Note: This method first checks the local cache (groupMembers) before making a network request private func fetchUserData(for userId: String) async -> AppUser? {
133-133: Consider centralizing error handling logicThe error handling pattern (logging, state update, toast) is repeated across multiple methods. Consider extracting this into a reusable method to improve maintainability:
+private func handleMemberFetchError(_ error: Error, for memberId: String) { + currentViewState = .initial + LogE("GroupTransactionListViewModel: \(#function) Failed to fetch member \(memberId): \(error).") + showToastForError() +} private func fetchUserData(for userId: String) async -> AppUser? { if let existingUser = groupMembers.first(where: { $0.id == userId }) { return existingUser } else { do { let user = try await groupRepository.fetchMemberBy(memberId: userId) if let user { groupMembers.append(user) } LogD("GroupTransactionListViewModel: \(#function) Member fetched successfully.") return user } catch { - currentViewState = .initial - LogE("GroupTransactionListViewModel: \(#function) Failed to fetch member \(userId): \(error).") - showToastForError() + handleMemberFetchError(error, for: userId) return nil } } }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
Line range hint
197-208: Consider enhancing error handling and separation of concerns.The error handling could be more specific and the method could be split for better separation of concerns:
- Consider creating specific error states for different failure scenarios (e.g., member not found vs. network error)
- Consider separating the member data fetching logic from state management
Here's a suggested refactor:
func fetchMemberData(for memberId: String) async -> AppUser? { do { let member = try await groupRepository.fetchMemberBy(memberId: memberId) if let member { addMemberIfNotExist(member) } LogD("GroupHomeViewModel: \(#function) Member fetched successfully.") return member } catch { - groupState = .noMember LogE("GroupHomeViewModel: \(#function) Failed to fetch member \(memberId): \(error).") - showToastForError() + handleMemberFetchError(error) return nil } } + +private func handleMemberFetchError(_ error: Error) { + switch error { + case is NetworkError: + groupState = .noInternet + case is MemberNotFoundError: + groupState = .noMember + default: + groupState = .somethingWentWrong + } + showToastForError() +}Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (3)
Line range hint
134-144: Enhance image handling robustnessThe current image handling could be improved for better reliability and user experience:
- Consider validating maximum image size before upload
- Make image compression quality configurable
- Add specific error handling for image processing failures
Example enhancement:
+ private let MAX_IMAGE_SIZE_MB = 5 + private let IMAGE_COMPRESSION_QUALITY: CGFloat = 0.2 + private func updateUserProfile() async { guard let user = preference.user else { return } var newUser = user newUser.firstName = firstName.trimming(spaces: .leadingAndTrailing).capitalized newUser.lastName = lastName.trimming(spaces: .leadingAndTrailing).capitalized newUser.emailId = email.trimming(spaces: .leadingAndTrailing) newUser.phoneNumber = phoneNumber - let resizedImage = profileImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + guard let imageData = prepareImageForUpload(profileImage) else { + showAlertFor(message: "Failed to process profile image") + return + } do { isSaveInProgress = trueAdd this helper method:
private func prepareImageForUpload(_ image: UIImage?) -> Data? { guard let image = image else { return nil } let resizedImage = image.aspectFittedToHeight(200) guard let imageData = resizedImage?.jpegData(compressionQuality: IMAGE_COMPRESSION_QUALITY) else { return nil } let imageSizeMB = Double(imageData.count) / 1_000_000 guard imageSizeMB <= MAX_IMAGE_SIZE_MB else { showAlertFor(message: "Image size must be less than \(MAX_IMAGE_SIZE_MB)MB") return nil } return imageData }
Line range hint
191-208: Improve error handling robustnessThe current error handling has several areas for improvement:
- String comparison for error detection is fragile
- Generic error messages reduce user understanding
- Limited error logging for debugging
Consider this enhanced approach:
} catch { isDeleteInProgress = false - LogE("UserProfileViewModel: \(#function) Failed to delete user: \(error).") - if error.localizedDescription.contains(REQUIRE_AGAIN_LOGIN_TEXT) { + let nsError = error as NSError + LogE("UserProfileViewModel: \(#function) Failed to delete user: \(error). Domain: \(nsError.domain), Code: \(nsError.code)") + + if let authError = error as? AuthErrorCode { + switch authError { + case .requiresRecentLogin: alert = .init(title: "", message: error.localizedDescription, positiveBtnTitle: "Reauthenticate", positiveBtnAction: { self.reAuthenticateUser() }, negativeBtnTitle: "Cancel", negativeBtnAction: { self.showAlert = false }) showAlert = true - } else { - showToastForError() + case .networkError: + showAlertFor(message: "Please check your internet connection and try again") + case .userNotFound: + showAlertFor(message: "User account not found") + default: + showToastForError() + } + } else { + showToastForError() } }
Line range hint
249-353: Consider extracting authentication logic to separate serviceThe current authentication implementation mixes concerns and duplicates error handling. Consider:
- Extract authentication logic to a dedicated service
- Unify error handling across auth providers
- Use async/await instead of completion handlers where possible
Example structure:
// AuthenticationService.swift protocol AuthenticationService { func reauthenticate(with provider: AuthProvider) async throws -> AuthCredential } enum AuthProvider { case apple case google case phone(number: String) } class DefaultAuthenticationService: AuthenticationService { private let appleAuthProvider: AppleAuthProvider private let googleAuthProvider: GoogleAuthProvider private let phoneAuthProvider: PhoneAuthProvider func reauthenticate(with provider: AuthProvider) async throws -> AuthCredential { switch provider { case .apple: return try await appleAuthProvider.authenticate() case .google: return try await googleAuthProvider.authenticate() case .phone(let number): return try await phoneAuthProvider.authenticate(phoneNumber: number) } } }This would simplify the view model and improve testability. Would you like me to help create this service?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Data/Data/Repository/GroupRepository.swift(4 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
- Data/Data/Repository/GroupRepository.swift
🔇 Additional comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (2)
Line range hint 1-208: Verify proper cleanup of NotificationCenter observers.
The class adds multiple observers in init but doesn't seem to remove them when the view model is deallocated, which could lead to memory leaks.
Let's check if there's a deinit method that handles cleanup:
Consider adding proper cleanup:
deinit {
NotificationCenter.default.removeObserver(self)
}197-197: LGTM! Parameter name change improves clarity.
The change from userId to memberId better reflects the parameter's purpose in the context of group member data fetching.
Let's verify consistent parameter naming across the codebase:
✅ Verification successful
Based on the comprehensive search results, I can now provide the final response:
Parameter naming is consistent across the codebase
The change from userId to memberId in GroupHomeViewModel.swift aligns with the repository layer implementation where fetchMemberBy(memberId:) is consistently used. The search results show:
- Repository layer uses
memberIdparameter inGroupRepository.fetchMemberBy(memberId:)method - All view models calling this method use the
memberIdparameter name - The only occurrences of
userIdare in different contexts (e.g., current user identification, balance calculations) and not in member fetching operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of userId in member-related function calls
# to ensure consistent parameter naming.
# Search for member-related function calls still using userId
rg -A 2 'fetchMemberBy\(userId:'
# Search for other potential inconsistencies in member-related functions
ast-grep --pattern 'func $$$Member$$$($$$userId$$$)'
Length of output: 89
Script:
#!/bin/bash
# Let's broaden our search to find all member-related function signatures and calls
# Search for any function definitions or calls related to member fetching
rg -i 'fetch.*member' -A 2
# Search for parameter names in member-related functions
ast-grep --pattern 'func $$$Member$$$($$$: $_)'
# Search for any remaining instances of userId in the context of members
rg -i 'userid.*member|member.*userid' -A 2
Length of output: 41575
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (2)
Line range hint
75-94: LGTM with a minor suggestion for readability.The changes correctly implement user-specific balance text by checking the current user's ID. The implementation is robust and handles all cases appropriately.
Consider extracting the text logic into a computed property for better readability:
+ var balanceText: String { + if memberBalance.totalOwedAmount == 0 { return "is settled up" } + return hasDue ? + (memberBalance.id == preference.user?.id ? "owe" : "owes") : + (memberBalance.id == preference.user?.id ? "get back" : "gets back") + }
Line range hint
148-171: Consider standardizing text capitalization.While the changes correctly implement user-specific text, there's inconsistent capitalization between different text segments:
owedMemberName.capitalizedis used but the rest of the text remains lowercase- This might lead to inconsistent text presentation like "John owes $20 to bob"
Consider standardizing the capitalization:
- Text("\(owedMemberName.capitalized) \(owesText.localized) ") + Text("\(owedMemberName) \(owesText.localized) ")Or if capitalization is needed:
- Text(" to \(owesMemberName)") + Text(" to \(owesMemberName.capitalized)")Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingView.swift (1)
66-72: Consider adding nil-coalescing for preference.user.While the logic is correct, consider handling the case where preference.user might be nil to make the code more robust.
private var memberName: String { - if let user = preference.user, user.id == member.id { + if let user = preference.user { + return user.id == member.id ? "You" : member.fullName + } else { return "You" - } else { - return member.fullName } }Splito/Localization/Localizable.xcstrings (1)
Line range hint
1-1219: Overall localization structure is well-maintained.The localization file follows iOS best practices:
- Consistent JSON structure
- Proper state tracking for translations
- Clear organization of string resources
Consider adding comments for complex string patterns (like those with multiple placeholders) to help other developers understand their usage context.
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (1)
133-134: Enhance error handling and logging in getMemberName method.While the personalization logic is good, consider these improvements:
func getMemberName(id: String, needFullName: Bool = false) -> String { - guard let userId = preference.user?.id, let member = getMemberDataBy(id: id) else { return "" } - return needFullName ? (id == userId ? "You" : member.fullName) : (id == userId ? "you" : member.nameWithLastInitial) + guard let userId = preference.user?.id else { + LogE("GroupBalancesViewModel: \(#function) Current user ID not found") + return "" + } + guard let member = getMemberDataBy(id: id) else { + LogE("GroupBalancesViewModel: \(#function) Member not found for ID: \(id)") + return "" + } + let isCurrentUser = id == userId + if needFullName { + return isCurrentUser ? "You" : member.fullName + } + return isCurrentUser ? "You" : member.nameWithLastInitial }Changes suggested:
- Split guard statements for better error tracking
- Added error logging when user/member not found
- Consistent capitalization of "You" for better UI consistency
- Improved readability with intermediate boolean variable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Splito/Localization/Localizable.xcstrings(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift(5 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingView.swift(3 hunks)
🔇 Additional comments (7)
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (1)
10-10: LGTM: Data import is required for SplitoPreference.
The addition of the Data import statement is necessary to support the newly injected SplitoPreference dependency.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingView.swift (3)
56-57: LGTM! Clean dependency injection implementation.
The SplitoPreference injection follows best practices and provides clean access to user preferences.
86-86: LGTM! Clean text display update.
The Text view correctly uses the new memberName property while maintaining localization and styling.
Line range hint 66-86: Verify consistent member name handling across the app.
Let's ensure that the "You" vs full name logic is consistently implemented across the app.
✅ Verification successful
Based on the search results, I can now provide a final response:
Member name handling is consistently implemented across the app
The codebase shows consistent handling of member/user name displays:
- In
ExpenseSplitOptionsTabView.swift:Text(member.id == preference.user?.id ? "You" : member.fullName) - In
GroupWhoIsPayingView.swift: Similar logic inmemberNamecomputed property - Other views consistently use either full name or localized text for member displays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar member name display patterns
rg -l "fullName" | xargs rg "Text\(.*fullName.*\)"
# Search for other potential user name displays that might need similar treatment
ast-grep --pattern 'Text($value) { $$$member(\.|\s)+(fullName|name)$$$ }'
Length of output: 1646
Script:
#!/bin/bash
# Let's try a broader search for member name displays
rg -A 2 "Text\(" --type swift
# Also search for potential member name computations
ast-grep --pattern 'var $name: String {
if let user = $$$
return $$$
}'
Length of output: 65762
Splito/Localization/Localizable.xcstrings (1)
49-57: LGTM! Well-structured localization entry.
The new string pattern uses positional arguments (%1$@, %2$@) which is the recommended approach for maintaining correct order of substitutions across different languages.
Let's verify the usage of this new string pattern:
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (2)
Line range hint 41-47: Verify transaction notification handling for data updates.
The PR aims to fix data update issues. While the notification observer is in place, we should verify that handleAddTransaction is properly updating all relevant data.
Let's check for other transaction-related notifications that might be missing:
Also, verify the completeness of data refresh:
✅ Verification successful
Transaction notification handling is properly implemented
The verification shows that transaction updates are being handled correctly:
- Both
GroupHomeViewModelandGroupBalancesViewModelobserve transaction notifications (add/update/delete) GroupBalancesViewModel.handleAddTransactionproperly refreshes data by callingfetchGroupWithMembers()- The implementation aligns with other view models, where
GroupHomeViewModelhas similar refresh logic - The notification chain is complete: from
GroupPaymentViewModelposting the notification to the observers handling it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other transaction-related notifications in the codebase
rg -A 2 "\.addTransaction|NSNotification\.Name\(" --type swift
Length of output: 2397
Script:
#!/bin/bash
# Check if there are other view models that might need similar refresh logic
ast-grep --pattern 'class $_ : BaseViewModel {
$$$
@objc $_ handleAddTransaction($$$) {
$$$
}
$$$
}'
Length of output: 114
Script:
#!/bin/bash
# Let's try a different pattern to find transaction handlers
ast-grep --pattern '@objc func handleAddTransaction($$$) {
$$$
}'
# Also check for other transaction-related handlers
ast-grep --pattern '@objc func handle$_Transaction($$$) {
$$$
}'
# Let's also check the implementation of GroupBalancesViewModel
rg -B 5 -A 10 "class GroupBalancesViewModel" --type swift
Length of output: 3280
Line range hint 54-82: Validate balance calculation logic for zero amount edge cases.
Given the PR's objective to fix issues with zero spending amounts, let's ensure the balance calculation logic handles all edge cases correctly:
✅ Verification successful
Based on the comprehensive analysis of the balance calculation logic in the codebase, I can now provide a final response:
Balance calculation logic appears correct and handles zero amounts properly.
The balance calculation implementation in calculateExpensesSimplified() is working as intended:
- It correctly filters balances for active group members
- The balance calculation includes proper rounding using
roundAmount() - Zero amount handling is implemented with epsilon comparison
- The balance updates are properly synchronized across expenses and transactions
- The system maintains both individual balances and monthly summaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar balance calculation patterns that might need the same fixes
ast-grep --pattern 'let filteredBalances = $_.filter { $$.contains($_) }'
# Look for test files covering balance calculations
fd -e swift -e xctestplan | rg -l "test.*Balance|Balance.*test"
Length of output: 141
Script:
#!/bin/bash
# Search for balance calculation patterns with more flexible AST matching
ast-grep --pattern 'let $_ = $_.balances'
# Search for balance-related test files with broader pattern
rg -l "balance|Balance" --type swift
# Look for balance calculation implementations
rg -l "calculate.*balance|Balance" --type swift
# Search for specific balance calculation logic
rg "balance.*filter|filter.*balance" --type swift -A 3
Length of output: 4333
Script:
#!/bin/bash
# Let's examine the balance calculation functions in detail
rg -A 10 "calculateExpensesSimplified|calculateBalances" Splito/UI/Home/Groups/CalculateExpensesFunctions.swift
# Check the Groups model to understand balance structure
cat Data/Data/Model/Groups.swift
# Look for balance-related tests
rg -A 5 "class.*Test.*Balance|func test.*balance" --type swift
Length of output: 5024
Script:
#!/bin/bash
# Let's check how the balance is updated and calculated in other parts
rg -A 5 "balance.*=|changeInBalance" --type swift
# Look for balance initialization
rg -A 3 "GroupMemberBalance\(.*\)" --type swift
# Check for balance updates in expense-related code
rg -A 5 "updateBalance|updateGroupBalance" --type swift
Length of output: 49384
cp-amisha-i
force-pushed
the
fix-some-issues
branch
from
6cc30c8 to
116ac9b
Compare
Simulator.Screen.Recording.-.iPhone.16.-.2024-12-04.at.17.03.50.mp4
Simulator.Screen.Recording.-.iPhone.16.-.2024-12-04.at.18.45.12.mp4
Simulator.Screen.Recording.-.iPhone.16.-.2024-12-05.at.13.18.40.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI Improvements
Refactor
These updates aim to enhance user experience and streamline interactions within the app.