Redesign screens and fixes #72
Conversation
#Conflicts: # Data/Data/Model/Expense.swift # Data/Data/Model/Transaction.swift # Splito/UI/Home/Expense/AddExpenseViewModel.swift # Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift # Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.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/GroupListViewModel.swift
#Conflicts: # Data/Data/Repository/GroupRepository.swift # Splito/UI/Home/Expense/AddExpenseViewModel.swift # Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.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/GroupHomeViewModelExtension.swift
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (56)
functions/src/index.ts (1)
8-18: Consider extracting Firebase initialization into a dedicated function.While the initialization logic is robust with proper error handling and logging, it could be more maintainable as a separate function.
Consider refactoring to:
+function initializeFirebaseApp() { + if (admin.apps.length === 0) { + try { + admin.initializeApp(); + logger.info('Firebase app initialized successfully'); + } catch (error) { + logger.error('Failed to initialize Firebase app:', error); + throw error; + } + } else { + logger.debug('Firebase app already initialized'); + } +} + +initializeFirebaseApp(); -if (admin.apps.length === 0) { - try { - admin.initializeApp(); - logger.info('Firebase app initialized successfully'); - } catch (error) { - logger.error('Failed to initialize Firebase app:', error); - throw error; - } -} else { - logger.debug('Firebase app already initialized'); -}Data/Data/Extension/Notification+Extension.swift (1)
25-26: LGTM! Consider adding documentation.The new notification constant follows the established naming pattern and integrates well with the activity log navigation feature.
Consider adding a documentation comment to describe the notification's purpose and expected userInfo parameters:
+ /// Notification posted when activity log navigation is requested + /// - Parameter userInfo: Dictionary containing "activityId" for the target activity static let showActivityLog = Notification.Name("showActivityLog")Splito/UI/Home/HomeRouteViewModel.swift (1)
37-40: Consider adding documentation for better maintainability.The implementation looks good, but adding documentation would improve maintainability.
+ /// Switches to the activity log tab and sets the selected activity + /// - Parameter activityId: The ID of the activity to display func switchToActivityLog(activityId: String) { activityLogId = activityId selectedTab = 1 }functions/lib/locales/en.json (1)
10-13: Consider platform-specific line breaks.The
\nnewline character might render differently across platforms. Consider using platform-specific line breaks or HTML/markdown formatting if supported by the notification system.Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift (2)
31-33: Consider adding completion handling for better state managementThe current implementation is fire-and-forget style. Consider adding a completion handler or async/await return to allow callers to react to the operation's completion or failure.
- func fetchGroupWithMembersData() { - Task { - await fetchGroupWithMembers() - } + func fetchGroupWithMembersData() async { + await fetchGroupWithMembers()
31-45: Consider implementing structured concurrency patternsThe current implementation could benefit from more robust concurrency handling:
- Task management for proper cancellation
- Structured concurrency using TaskGroup for parallel fetching if needed
- Actor-based state management to prevent race conditions
Example implementation:
private actor ViewState { private(set) var members: [AppUser] = [] private(set) var state: State = .loading func update(members: [AppUser], state: State) { self.members = members self.state = state } } // Usage private var taskHandle: Task<Void, Never>? func fetchGroupWithMembersData() { taskHandle?.cancel() taskHandle = Task { @MainActor in await fetchGroupWithMembers() } }Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift (1)
Line range hint
38-49: Consider enhancing error handling and state management.While the implementation is functional, there are several opportunities for improvement:
- The method makes two sequential API calls without intermediate state updates, which might leave users wondering about progress.
- Error handling doesn't distinguish between group fetch and member fetch failures.
- The success state is set to
.initialwhich doesn't clearly indicate success to the UI.Consider this enhanced implementation:
private func fetchGroupWithMembers() async { do { let group = try await groupRepository.fetchGroupBy(id: groupId) guard let group else { - currentViewState = .initial + currentViewState = .error(.groupNotFound) return } + + // Update state to show we're fetching members + await MainActor.run { currentViewState = .loadingMembers } + self.members = try await groupRepository.fetchMembersBy(memberIds: group.members) - currentViewState = .initial + currentViewState = .loaded } catch { - handleServiceError() + await MainActor.run { + if !networkMonitor.isConnected { + currentViewState = .noInternet + } else if (error as? GroupError) != nil { + currentViewState = .error(.groupFetchFailed) + } else { + currentViewState = .error(.membersFetchFailed) + } + } } }You'll need to update the ViewState enum accordingly:
enum ViewState { case initial case loading case loadingMembers case loaded case noInternet case error(FetchError) } enum FetchError { case groupNotFound case groupFetchFailed case membersFetchFailed }Data/Data/Model/AppUser.swift (1)
18-18: Consider FCM token storage strategyThe current implementation stores a single FCM token per user. Consider these architectural implications:
- Multi-device support: Users might log in from multiple devices, each needing its own FCM token
- Token refreshes: FCM tokens can change frequently and might need a timestamp
- Historical data: Some use cases might require keeping track of old tokens
Consider creating a separate
UserDevicemodel if these scenarios are relevant to your use case.Example structure:
public struct UserDevice: Codable { let userId: String let fcmToken: String let lastUpdated: Date let deviceInfo: DeviceInfo }Also applies to: 58-58
Data/Data/Store/ActivityLogStore.swift (1)
Line range hint
30-53: Consider enhancing error handling and thread safety.The implementation is good, with proper listener cleanup and error logging. However, consider these improvements:
- Use structured error types instead of returning nil for better error handling
- Ensure completion handler is called on the main thread for UI updates
- Add more specific error messages
Here's a suggested improvement:
- func fetchLatestActivityLogs(userId: String, completion: @escaping ([ActivityLog]?) -> Void) { + enum ActivityLogError: Error { + case fetchError(Error) + case snapshotMissing + case decodingError(Error) + } + + func fetchLatestActivityLogs(userId: String, completion: @escaping (Result<[ActivityLog], ActivityLogError>) -> Void) { listener?.remove() listener = activityReference(userId: userId).addSnapshotListener { [weak self] snapshot, error in if let error { - LogE("ActivityLogStore :: \(#function) Error fetching document: \(error.localizedDescription)") - completion(nil) + LogE("ActivityLogStore :: \(#function) Firestore fetch failed: \(error.localizedDescription)") + DispatchQueue.main.async { + completion(.failure(.fetchError(error))) + } return } guard let snapshot else { - LogE("ActivityLogStore :: \(#function) snapshot is nil for requested user.") - completion(nil) + LogE("ActivityLogStore :: \(#function) Firestore snapshot is nil for user: \(userId)") + DispatchQueue.main.async { + completion(.failure(.snapshotMissing)) + } return } let activityLogs: [ActivityLog] = snapshot.documents.compactMap { document in do { return try document.data(as: ActivityLog.self) } catch { - LogE("ActivityLogStore :: \(#function) Error decoding document data: \(error.localizedDescription)") + LogE("ActivityLogStore :: \(#function) Failed to decode ActivityLog from document \(document.documentID): \(error.localizedDescription)") return nil } } - completion(activityLogs) + DispatchQueue.main.async { + completion(.success(activityLogs)) + } } }Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift (1)
Line range hint
52-70: Consider improving currency formatting and internationalization.The implementation could benefit from centralizing currency formatting and internationalization:
- Extract the decimal format "%.2f" to a constant or configuration.
- Use a localized currency formatter instead of hardcoding "₹".
Consider creating a shared currency formatter:
// Add to a shared utilities file struct CurrencyFormatter { static let shared = NumberFormatter().apply { $0.numberStyle = .currency $0.currencySymbol = "₹" // Get from localization $0.minimumFractionDigits = 2 $0.maximumFractionDigits = 2 } }Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (2)
47-50: Consider handling task cancellation.The async task should be stored and cancelled when appropriate to prevent potential memory leaks, especially if the view model is deallocated while the task is still running.
class ChooseMultiplePayerViewModel: BaseViewModel, ObservableObject { + private var fetchTask: Task<Void, Never>? + func fetchGroupWithMembersData() { - Task { + fetchTask?.cancel() + fetchTask = Task { await self.fetchGroupWithMembers() } } + + deinit { + fetchTask?.cancel() + }
53-59: Implementation aligns with PR optimization goals.The sequential fetching with early exit pattern effectively reduces unnecessary API calls, aligning with the PR's objective to optimize API calls for members and groups.
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (1)
64-75: Consider adding documentation for the optimized member fetching.Since this is part of a broader optimization effort, it would be helpful to add a comment explaining the new member fetching strategy and its benefits.
private func fetchGroupMembers() async { + // Fetch members directly using group.members instead of a separate group lookup + // to optimize API calls and reduce loading times guard let group else {Splito/UI/Home/Expense/Detail Selection/SelectGroupView.swift (1)
120-125: Consider extracting padding values to constants.While the implementation correctly handles the last divider visibility, consider extracting the hardcoded padding values (20) to named constants for better maintainability.
+ private let dividerVerticalPadding: CGFloat = 20 + if !isLastGroup { Divider() .frame(height: 1) .background(dividerColor) - .padding(.vertical, 20) + .padding(.vertical, dividerVerticalPadding) }Data/Data/Repository/ExpenseRepository.swift (1)
Line range hint
1-116: Consider adding documentation for public methodsThe implementation is solid, but adding documentation comments for public methods would improve maintainability. Consider documenting:
- Parameter requirements
- Return value expectations
- Possible error conditions
- Usage examples
Example for the
deleteExpensemethod:+ /// Deletes an expense by marking it as inactive and records the action in activity logs + /// - Parameters: + /// - group: The group containing the expense + /// - expense: The expense to be deleted + /// - Throws: FirebaseError if the update fails public func deleteExpense(group: Groups, expense: Expense) async throws {Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
Line range hint
119-141: Consider enhancing error handling and validation.The current implementation could benefit from:
- More specific error handling with custom error types
- Validation for empty group name after trimming
- Separation of concerns between image processing and group updates
Consider applying these improvements:
private func updateGroup(group: Groups) async -> Bool { guard let userId = preference.user?.id else { return false } var newGroup = group let trimmedName = groupName.trimming(spaces: .leadingAndTrailing) + guard !trimmedName.isEmpty else { + showToastFor(toast: ToastPrompt(type: .error, title: "Invalid Input", message: "Group name cannot be empty")) + return false + } - newGroup.name = groupName.trimming(spaces: .leadingAndTrailing) + newGroup.name = trimmedName newGroup.updatedBy = userId newGroup.updatedAt = Timestamp() do { self.showLoader = true + // Consider moving image processing to a separate method + let updatedGroup = try await updateGroupWithImage(newGroup) - let updatedGroup = try await groupRepository.updateGroupWithImage(imageData: imageData, newImageUrl: profileImageUrl, - group: newGroup, oldGroupName: group.name) NotificationCenter.default.post(name: .updateGroup, object: updatedGroup) showLoader = false return true } catch { showLoader = false - showToastForError() + // Handle specific error cases + switch error { + case let storageError as StorageError: + showToastFor(toast: ToastPrompt(type: .error, title: "Storage Error", message: storageError.localizedDescription)) + default: + showToastForError() + } return false } } + private func updateGroupWithImage(_ group: Groups) async throws -> Groups { + let resizedImage = profileImage?.aspectFittedToHeight(200) + let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + return try await groupRepository.updateGroupWithImage( + imageData: imageData, + newImageUrl: profileImageUrl, + group: group, + oldGroupName: group.name + ) + }Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift (2)
31-31: Consider implementing data prefetching.To further optimize the loading experience, consider implementing data prefetching when the view is likely to be shown, rather than waiting for an error to occur.
Example approach:
// In the parent view or coordinator func prepareForChoosePayer() { viewModel.prefetchGroupWithMembersData() }
Line range hint
16-85: Enhance accessibility support.While the view implementation looks good, consider adding these accessibility improvements:
ScrollView { VSpacer(27) VStack(alignment: .leading, spacing: 0) { ForEach(users) { user in ChooseMemberCellView( member: user, isSelected: (viewModel.selectedPayers.count > 1) ? false : viewModel.selectedPayers.keys.contains(user.id) ) + .accessibilityLabel("\(user.fullName), tap to select as payer") + .accessibilityValue(viewModel.selectedPayers.keys.contains(user.id) ? "Selected" : "Not selected") + .accessibilityAddTraits(.isButton) .onTapGestureForced { viewModel.handlePayerSelection(user: user) } }Splito/UI/Home/Account/AccountHomeView.swift (1)
Line range hint
1-180: Consider enhancing accessibility supportWhile the implementation is clean and well-structured, consider adding accessibility labels and hints to improve the experience for users with disabilities:
Add accessibility modifiers to key interactive elements. For example:
AccountItemCellView(optionText: "Contact Us", onClick: onContactTap) + .accessibilityLabel("Contact Us button") + .accessibilityHint("Opens contact form to reach support")Similar accessibility enhancements would be valuable for:
- Profile image and user info section
- Rate app and share app buttons
- Sign out button
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
97-119: Consider extracting the corner radius to a constantThe corner radius value (12) should be extracted to a constant or design system value for better maintainability and consistency.
+ private let cornerRadius: CGFloat = 12 ... - RoundedRectangle(cornerRadius: 12) + RoundedRectangle(cornerRadius: cornerRadius)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift (1)
Line range hint
39-42: Consider localizing the explanatory textThe explanatory text about payment recording should be moved to the localization file for better maintainability and internationalization support.
- Text("This payment was noted using the \"record a payment\" feature, No money has been transferred.") + Text("payment_recording_explanation".localized)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
Line range hint
130-144: Consider enhancing error handling specificityThe error handling could be more specific to provide better feedback to users. Instead of a generic error toast, consider handling different error cases (network, permission, etc.) separately.
try await expenseRepository.deleteExpense(group: group, expense: deletedExpense) await updateGroupMemberBalance(expense: deletedExpense, updateType: .Delete) } catch { - showToastForError() + switch error { + case let NetworkError.connectionError(message): + showToastFor(toast: .init(type: .error, title: "Connection Error", message: message)) + case let RepositoryError.permissionDenied(message): + showToastFor(toast: .init(type: .error, title: "Permission Denied", message: message)) + default: + showToastForError() + } }
145-161: Optimize alert display and improve message clarityThe validation logic is well-implemented, but there are a few potential improvements:
- The 0.1-second delay for showing the alert seems unnecessary:
- DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { + DispatchQueue.main.async { self.showAlertFor(message: "This expense involves a person who has left the group, and thus it can no longer be deleted. If you wish to change this expense, you must first add that person back to your group.") }
- Consider making the error message more concise:
- message: "This expense involves a person who has left the group, and thus it can no longer be deleted. If you wish to change this expense, you must first add that person back to your group." + message: "Cannot delete expense: One or more members have left the group. Add them back to modify this expense."
Line range hint
1-262: Consider implementing a dedicated error handling strategyThe file would benefit from a centralized error handling approach. Consider:
- Creating an
ExpenseErrorenum to encapsulate different error types- Implementing a dedicated error handling protocol
- Using result builders for structured error presentation
This would make error handling more consistent and maintainable across the codebase.
Example implementation:
enum ExpenseError: Error { case memberValidation(String) case deletion(Error) case balanceUpdate(Error) var userMessage: String { switch self { case .memberValidation(let message): return message case .deletion: return "Failed to delete expense" case .balanceUpdate: return "Failed to update balances" } } } protocol ErrorHandling { func handle(_ error: ExpenseError) }Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (2)
214-214: Maintain consistency in success messagesConsider reviewing other success messages in the app to ensure consistent punctuation usage.
#!/bin/bash # Search for other success messages to check punctuation consistency rg "showToastFor.*success.*message:" --type swift
59-65: Consider implementing a loading state for member fetchingTo improve the user experience, consider implementing a loading state specifically for member fetching. This would allow showing a loading indicator while members are being fetched, rather than setting the entire view to the initial state.
Example approach:
enum MemberLoadingState { case loading case loaded case error } @Published private(set) var memberLoadingState: MemberLoadingState = .loadingSplito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (3)
128-128: Consider a more user-friendly error message.The current message could be more approachable. Consider:
- message: "The group associated with this payment has been deleted, so it cannot be restored.") + message: "This payment cannot be restored because the group has been deleted.")
213-213: Simplify the error message.The current message is a bit verbose and contains redundant information.
- message: "This payment could not be \(operationText). You do not have permission to \(action) this payment, Sorry!") + message: "You don't have permission to \(action) this payment.")
Line range hint
1-267: Well-structured view model with consistent error handling.The view model demonstrates good separation of concerns and robust validation. The consistent terminology changes and standardized validation patterns make the code more maintainable.
A few suggestions for future improvements:
- Consider extracting alert/error messages to a localization file for easier maintenance and internationalization.
- The validation methods could potentially be moved to a shared protocol if similar validation is needed in other view models.
Splito/UI/Home/Groups/GroupListView.swift (2)
Line range hint
1-350: Consider architectural improvements for better maintainabilityWhile reviewing the entire file context, here are some architectural suggestions to enhance the implementation:
- Consider extracting the tab bar, header, and action sheet into separate files to improve maintainability
- The
OptionListenum could be moved to a separate constants file- Consider using a dedicated configuration object for
SystemPlusButtonpropertiesExample structure:
// GroupListComponents/GroupListTabBar.swift struct GroupListTabBar: View { // Current implementation of GroupListTabBarView } // Constants/GroupListOptions.swift enum GroupListOption: CaseIterable { // Current implementation of OptionList } // Components/SystemButtons.swift struct SystemPlusButtonConfig { let imageName: String let size: Double let imageWeight: Font.Weight let color: Color static let `default` = SystemPlusButtonConfig( imageName: "plus", size: 16, imageWeight: .medium, color: .primaryText ) }
Line range hint
1-350: Enhance accessibility supportThe UI components could benefit from improved accessibility support:
- Add accessibility labels to buttons and interactive elements
- Provide accessibility hints for the tab bar items
- Ensure proper VoiceOver support for the amount displays
Example improvements:
Button { onSelect(tab) } label: { Text(tab.tabItem) // ... existing modifiers ... .accessibilityLabel("\(tab.tabItem) groups") .accessibilityHint("Shows \(tab.tabItem.lowercased()) groups") } Text("\(totalOweAmount.formattedCurrency)") // ... existing modifiers ... .accessibilityLabel("Amount \(isOwed ? "owed" : "to receive"): \(totalOweAmount.formattedCurrency)")Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (1)
175-189: Consider improving the alert timing and message lengthThe validation logic is solid, but there are two potential improvements:
- The 0.1-second delay for showing the alert seems arbitrary. Consider showing it immediately or explaining the need for the delay in a comment.
- The error message is quite long. Consider breaking it into shorter, more digestible sentences.
- 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.") - } + self.showAlertFor(message: "Cannot delete this payment. One or more members have left the group. Add them back to the group to modify this payment.")Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
305-310: Improve error handling for missing user ID caseWhile the optional binding for
userIdis good, the function silently returnsfalseif the user ID is missing. Consider providing feedback to the user in this scenario.- guard let selectedGroup, let userId = preference.user?.id else { return false } + guard let selectedGroup else { + showToastFor(toast: ToastPrompt(type: .warning, title: "Error", + message: "No group selected")) + return false + } + guard let userId = preference.user?.id else { + showToastFor(toast: ToastPrompt(type: .error, title: "Error", + message: "User session expired. Please log in again.")) + return false + }
Line range hint
313-353: Consider adding validation for negative expense amountsThe
handleAddExpenseActionmethod allows for any non-zero expense amount. Consider adding validation to prevent negative amounts, which could lead to unexpected behavior in balance calculations.if expenseName == "" || expenseAmount == 0 || selectedGroup == nil || selectedPayers == [:] { showToastFor(toast: ToastPrompt(type: .warning, title: "Warning", message: "Please fill all data to add expense.")) return false } + + if expenseAmount < 0 { + showToastFor(toast: ToastPrompt(type: .warning, title: "Warning", + message: "Expense amount cannot be negative.")) + return false + }Splito/Localization/Localizable.xcstrings (4)
Line range hint
185-289: Improve clarity of amount-related stringsThe string "Amount" is quite generic. Consider providing more context for better user understanding.
Consider using more descriptive text:
- "Amount" : { + "Payment Amount" : {
485-491: Enhance search result messagesThe search-related messages could be more helpful to users.
Consider making these messages more actionable:
- "No payments yet!" : { + "No payments found. Add a payment to get started!" : { - "No results were found that match your search criteria." : { + "No matching payments found. Try different search terms." : {
527-536: Verify payment success message punctuationThe success message ends with a period while other similar messages don't. Maintain consistency in punctuation.
Consider removing the period for consistency:
- "Payment made successfully." : { + "Payment made successfully" : {
792-797: Improve error message clarityThe error messages are verbose and contain redundant information.
Consider simplifying the messages:
- "This payment could not be deleted. You do not have permission to delete this payment, Sorry!" : { + "You don't have permission to delete this payment" : { - "This payment could not be edited. You do not have permission to edit this payment, Sorry!" : { + "You don't have permission to edit this payment" : {functions/src/users_service/users_service.ts (2)
35-38: Handle document creation and deletion events appropriatelyThe function exits when either
beforeDataorafterDatais null, logging a warning. However, during document creation (beforeDatais null) or deletion (afterDatais null), this behavior might not be sufficient. Consider handling these cases explicitly to ensure thattotal_owe_amountis accurately maintained for all users involved.Apply this diff to handle creation and deletion events:
// Handle edge cases where beforeData or afterData might be null if (!beforeData && afterData) { // Document was created // Handle initialization if necessary } else if (beforeData && !afterData) { // Document was deleted // Handle cleanup if necessary - } else if (!beforeData || !afterData) { + } else if (!beforeData && !afterData) { logger.warn('Both beforeData and afterData are null. Exiting function.'); return; }
4-6: Import only the necessary lodash function to reduce bundle sizeCurrently, the entire
lodashlibrary is imported, but only theisEqualfunction is used. To optimize the bundle size and improve performance, import only theisEqualfunction.Apply this diff to import only
isEqual:- import * as _ from 'lodash'; + import isEqual from 'lodash/isEqual';And update the usage accordingly:
- if (!_.isEqual(beforeData.balances, afterData.balances)) { + if (!isEqual(beforeData.balances, afterData.balances)) {Splito/AppDelegate.swift (2)
26-26: Fix method formatting for readabilityThe method
registerForPushNotificationshas code on the same line as its declaration, which affects readability. Please moveMessaging.messaging().delegate = selfto the next line.Apply this diff:
- private func registerForPushNotifications(application: UIApplication) { Messaging.messaging().delegate = self + private func registerForPushNotifications(application: UIApplication) { + Messaging.messaging().delegate = self
29-30: Review notification authorization options for user experienceConsider whether the use of
.provisionalauthorization is appropriate for your app's user experience. Provisional authorization delivers notifications quietly without user interruption, which may go unnoticed by users. Ensure this aligns with your notification strategy.Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (2)
51-54: DifferentiateviewStatebetween data not found and data loadedIn lines 52~ and 57~,
viewStateis set to.initialboth when the group is not found and after data is successfully loaded. This could lead to ambiguity in the UI state handling. Consider using distinctviewStatecases to represent 'no data' and 'data loaded' scenarios to improve clarity and enhance user experience.Also applies to: 57-57
71-73: Optimize mapping overfilteredBalancesto improve performanceYou're mapping over
filteredBalancestwice to creatememberBalancesandgroupMemberBalancesin lines 73~ and 78~. To enhance performance, consider combining these into a single mapping to reduce redundancy.You could refactor the code as follows:
let combinedData = filteredBalances.map { let memberCombinedBalance = MembersCombinedBalance(id: $0.id, totalOwedAmount: $0.balance) let groupMemberBalance = GroupMemberBalance(id: $0.id, balance: $0.balance, totalSummary: $0.totalSummary) return (memberCombinedBalance, groupMemberBalance) } // Separate the combined data as needed let memberBalances = combinedData.map { $0.0 } let groupMemberBalances = combinedData.map { $0.1 }Also applies to: 78-78
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (2)
107-107: Refactor repeated guard conditions into a single validation methodThe guard statements in
handleEditBtnAction,restoreExpense, anddeleteExpensemethods are repeating the same validation logic. Consider extracting this into a single method to improve maintainability.Apply this diff to simplify the guard statements:
-guard validateUserPermission(operationText: "edited", action: "edit"), validateGroupMembers(action: "edited") else { return } +guard validateAction(operationText: "edited", action: "edit") else { return } ... -guard var expense, let userId = preference.user?.id, validateUserPermission(operationText: "restored", action: "restored"), validateGroupMembers(action: "restored") else { return } +guard var expense, let userId = preference.user?.id, validateAction(operationText: "restored", action: "restore") else { return } ... -guard let group, let expense, validateUserPermission(operationText: "deleted", action: "delete"), validateGroupMembers(action: "deleted") else { return } +guard let group, let expense, validateAction(operationText: "deleted", action: "delete") else { return }Define the new method:
private func validateAction(operationText: String, action: String) -> Bool { return validateUserPermission(operationText: operationText, action: action) && validateGroupMembers(action: action) }Also applies to: 130-130, 161-161
188-190: Simplify dispatching to the main queueUsing
DispatchQueue.main.asyncAfterwith a 0.1-second delay may be unnecessary. Consider usingDispatchQueue.main.asyncto execute the closure immediately on the main queue.Apply this diff to simplify the code:
-DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { +DispatchQueue.main.async {Data/Data/Repository/GroupRepository.swift (2)
217-217: Implement cache invalidation forgroupMembersAppending members to
groupMemberswithout an invalidation strategy may lead to stale data if member information changes.Implement a cache invalidation policy to ensure
groupMembersremains up-to-date, such as refreshing cached data after a certain time or upon detecting changes.
71-78: SimplifygetActivityTypeconditional logicThe
getActivityTypemethod's conditional statements can be simplified for better readability.Consider restructuring the logic as follows:
if imageChanged && nameChanged { return .groupUpdated } else if imageChanged { return .groupImageUpdated } else if nameChanged { return .groupNameUpdated } else { return .none }functions/src/notifications_service/notifications_service.ts (4)
1-1: Specify ESLint Rules Instead of Disabling AllCurrently, all ESLint rules are disabled with
/* eslint-disable */. It's better practice to disable only specific rules that are causing issues to maintain code quality.Replace with specific rule(s):
-/* eslint-disable */ +/* eslint-disable no-unused-vars */Replace
no-unused-varswith the actual rule(s) you need to disable.
17-20: Validate Environment Variables for Locale and CurrencyThe
currencyFormatteruses environment variablesprocess.env.LOCALEandprocess.env.CURRENCYwith default values. To prevent runtime errors due to invalid or unsupported locale and currency codes, consider validating these environment variables or handling exceptions fromIntl.NumberFormat.
136-139: Log Unknown Activity Types in 'generateNotificationMessage'In the default case of the switch statement in
generateNotificationMessage, only a generic message is returned. For better monitoring and debugging, consider logging a warning when an unrecognizedactivityData.typeis encountered.Apply this change:
default: + logger.warn(`Unhandled activity type: ${activityData.type}`); return messages.new_activity;
153-203: Add Jitter to Exponential Backoff in 'sendNotification'When implementing exponential backoff in retries, adding random jitter helps to prevent synchronized retry attempts, reducing the risk of cascading failures.
Modify the delay calculation:
// Calculate exponential backoff delay with jitter - const delay = baseDelay * Math.pow(2, attempt); + const jitter = Math.random() * 1000; // Random jitter up to 1 second + const delay = baseDelay * Math.pow(2, attempt) + jitter;Splito/UI/Home/Expense/AddExpenseView.swift (3)
142-145: Consider makingfieldnon-optional to simplify control flowThe
fieldvariable inExpenseDetailRowis currently an optional. Making it non-optional can simplify the control flow and ensure that all cases are explicitly handled, reducing the risk of unexpected behavior whenfieldisnil.Apply this diff to make
fieldnon-optional:-var field: AddExpenseViewModel.AddExpenseField? +var field: AddExpenseViewModel.AddExpenseFieldEnsure that all instances of
ExpenseDetailRowprovide a value forfield.
100-103: RefactorExpenseDetailRowto accept only necessary bindings
ExpenseDetailRowaccepts bindings fornameanddateeven when they are not used in certain cases, which can lead to confusion and unnecessary data passing. Consider refactoring the component to accept only the bindings required for each specific case, enhancing readability and maintainability.Adjust the
ExpenseDetailRowto accept different parameters based on the context:-@Binding var name: String -@Binding var date: Date +var title: String +var value: StringModify the usages accordingly to pass only the necessary data.
Also applies to: 142-145
Line range hint
226-227: Simplify the calculation ofmaximumDateThe
maximumDateis currently calculated by adding zero years to the current date, which is redundant. You can simplify this by directly assigningDate()tomaximumDate.Apply this diff to simplify:
-private let maximumDate = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date() +private let maximumDate = Date()Splito/UI/Home/Groups/GroupListViewModel.swift (1)
64-64: Consider removing observers to prevent potential issuesWhile adding the observer for
.addExpensenotification is correct, it's good practice to remove observers when they are no longer needed. Consider removing the observer in adeinitmethod to prevent potential memory leaks or unexpected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Podfile.lockis excluded by!**/*.lockfirebase-debug.logis excluded by!**/*.log
📒 Files selected for processing (52)
.firebaserc(1 hunks)BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/AccentColor.colorset/Contents.json(0 hunks)Data/Data/Extension/Notification+Extension.swift(1 hunks)Data/Data/Model/AppUser.swift(3 hunks)Data/Data/Model/Groups.swift(0 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Data/Data/Store/ActivityLogStore.swift(1 hunks)Podfile(1 hunks)Splito.xcodeproj/project.pbxproj(3 hunks)Splito/AppDelegate.swift(1 hunks)Splito/Localization/Localizable.xcstrings(18 hunks)Splito/UI/Home/Account/AccountHomeView.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(7 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(0 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(4 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(2 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/SelectGroupView.swift(3 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(4 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidView.swift(2 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/GroupWhoIsPayingView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeView.swift(3 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(3 hunks)Splito/UI/Home/Groups/GroupListView.swift(1 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(4 hunks)Splito/UI/Home/HomeRouteView.swift(2 hunks)Splito/UI/Home/HomeRouteViewModel.swift(2 hunks)functions/lib/locales/en.json(1 hunks)functions/lint(1 hunks)functions/package.json(1 hunks)functions/src/index.ts(1 hunks)functions/src/locales/en.json(1 hunks)functions/src/notifications_service/notifications_service.ts(1 hunks)functions/src/users_service/users_service.ts(1 hunks)functions/tsconfig.json(1 hunks)lint(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (3)
- BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/AccentColor.colorset/Contents.json
- Data/Data/Model/Groups.swift
- Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
✅ Files skipped from review due to trivial changes (3)
- .firebaserc
- functions/lint
- functions/src/locales/en.json
🔇 Additional comments (70)
package.json (2)
3-3: Consider updating to the latest stable version
The current version specification is ^12.0.0 while version 12.6.0 is available according to the summary. Consider updating to the latest stable version to benefit from bug fixes and security updates.
Let's verify the latest stable version and check for any breaking changes:
1-5: Verify the necessity of firebase-admin dependency
The addition of Firebase Admin SDK seems unrelated to the PR's stated objectives of screen redesigns. This SDK provides powerful server-side capabilities and should only be included if absolutely necessary.
Let's verify if any of the changed files actually require this dependency:
functions/tsconfig.json (1)
6-7: LGTM! Good compiler options for enhanced module support.
These additions are beneficial:
resolveJsonModulewill enable direct JSON imports, supporting localization needsesModuleInteropensures smooth interoperability between different module systems, which is particularly useful for Firebase integration
lint (1)
1-2: Verify ESLint configuration and rules.
Since this is a TypeScript project, ensure that the ESLint configuration properly handles TypeScript-specific rules.
Also applies to: 12-13
✅ Verification successful
Let me try searching for ESLint configuration with hidden files and also check for TypeScript-related files to better understand the project setup.
Let's check the content of the ESLint configuration file and the tsconfig.json to verify the TypeScript setup.
ESLint is properly configured for TypeScript
The project has a comprehensive ESLint setup with TypeScript support:
- TypeScript-specific ESLint parser and plugin are configured
- Appropriate extensions including TypeScript-specific rules are enabled
- Required dependencies are properly installed in functions/package.json
- TSConfig is properly set up with strict type checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for ESLint configuration and TypeScript-related plugins
# Look for ESLint config files
echo "Checking for ESLint configuration files..."
fd -t f "^\.eslintrc\.(js|json|yaml|yml)$"
# Check package.json for ESLint dependencies
echo "Checking for TypeScript ESLint dependencies..."
if [ -f "package.json" ]; then
jq '.dependencies + .devDependencies | with_entries(select(.key | contains("eslint")))' package.json
fi
Length of output: 582
Script:
#!/bin/bash
# Check for ESLint configuration including hidden files and TypeScript setup
echo "Checking for ESLint configuration files (including hidden)..."
fd -H -t f "^\.eslintrc\.(js|json|yaml|yml)$"
echo -e "\nChecking for TypeScript configuration..."
fd -H -t f "^tsconfig\.json$"
echo -e "\nChecking package.json content..."
if [ -f "package.json" ]; then
echo "Dependencies and scripts related to TypeScript and ESLint:"
jq '{
dependencies: (.dependencies // {}),
devDependencies: (.devDependencies // {}),
scripts: (.scripts // {})
}' package.json
fi
Length of output: 850
Script:
#!/bin/bash
echo "Checking ESLint configuration content..."
cat functions/.eslintrc.js
echo -e "\nChecking TypeScript configuration..."
cat functions/tsconfig.json
echo -e "\nChecking functions/package.json for TypeScript and ESLint dependencies..."
if [ -f "functions/package.json" ]; then
jq '{
dependencies: (.dependencies // {}),
devDependencies: (.devDependencies // {}) | with_entries(select(.key | contains("typescript") or contains("eslint")))
}' functions/package.json
fi
Length of output: 2050
functions/src/index.ts (2)
5-6: LGTM! Good separation of concerns.
The modularization of functionality into dedicated services (users_service and notifications_service) improves code organization and maintainability.
21-22: Verify Cloud Function implementations in respective services.
Let's ensure the exported functions are properly implemented as Firebase Cloud Functions in their respective services.
✅ Verification successful
Cloud Functions are properly implemented and configured
Both functions are correctly implemented as Firebase Cloud Functions:
onGroupWriteis properly implemented inusers_service/users_service.tsusingonDocumentWrittentrigger forgroups/{groupId}pathonActivityCreateis properly implemented innotifications_service/notifications_service.tsusingonDocumentCreatedtrigger forusers/{userId}/activity/{activityId}path
Both functions include proper error handling, logging, and type safety measures. The exports in index.ts are correctly mapping to their implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Cloud Function implementations in service files
# Check implementation of onGroupWrite in users_service
echo "Checking onGroupWrite implementation:"
rg -A 10 "export.*onGroupWrite.*=.*functions\.firestore" ./functions/src/users_service/
# Check implementation of onActivityCreate in notifications_service
echo "Checking onActivityCreate implementation:"
rg -A 10 "export.*onActivityCreate.*=.*functions\." ./functions/src/notifications_service/
# Verify function triggers
echo "Checking Firestore triggers:"
rg "functions\.firestore\.document|functions\.database\.ref" ./functions/src/
Length of output: 507
Script:
#!/bin/bash
# Let's try a broader search to locate these functions and their implementations
# Search for onGroupWrite function
echo "Searching for onGroupWrite function:"
rg -l "onGroupWrite" ./functions/src/
# If found, show the implementation
echo -e "\nImplementation of onGroupWrite:"
rg -A 10 "onGroupWrite" ./functions/src/
# Search for onActivityCreate function
echo -e "\nSearching for onActivityCreate function:"
rg -l "onActivityCreate" ./functions/src/
# If found, show the implementation
echo -e "\nImplementation of onActivityCreate:"
rg -A 10 "onActivityCreate" ./functions/src/
# Look for any Firebase function definitions
echo -e "\nSearching for Firebase function definitions:"
rg "functions\." ./functions/src/
Length of output: 5751
functions/package.json (2)
18-18: LGTM on firebase-admin update
The minor version bump from ^12.1.0 to ^12.6.0 is backwards compatible and aligns with the new functionality mentioned in the AI summary.
19-19: Verify compatibility with firebase-functions v6
The major version bump from v5 to v6 may introduce breaking changes. Let's verify the compatibility and migration requirements.
Consider the following recommendations:
- Review the migration guide for v6
- Test the functions in a staging environment
- Update the deployment scripts if needed
✅ Verification successful
Firebase Functions v6 compatibility verified
The codebase is already using v6-compatible patterns:
- Uses modern syntax with
onDocumentWrittenandonDocumentCreatedtriggers - No usage of deprecated features like
functions.https.onRequest - Properly structured TypeScript exports and event handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes and migration guide for firebase-functions v6
# Check the changelog for breaking changes
curl -s "https://raw.githubusercontent.com/firebase/firebase-functions/master/CHANGELOG.md" | grep -A 10 "6.0.0"
# Verify if any deprecated features are used in the codebase
rg -l "functions\.https\.onRequest|functions\.handler|functions\.region|functions\.runWith"
# Check if new functions use v6 compatible syntax
rg -A 5 "export const \w+ = functions\."
Length of output: 247
Script:
#!/bin/bash
# Let's check the actual function implementations and usage patterns
# Find all files that import firebase-functions
rg -l "require\('firebase-functions'\)|import.*from.*'firebase-functions'"
# Look for function definitions and their patterns
rg -B 2 -A 5 "exports\.|export const" functions/
# Check package.json for any related dependencies or scripts
cat functions/package.json
Length of output: 2628
Splito/UI/Home/HomeRouteViewModel.swift (1)
19-19: LGTM! Property declaration looks good.
The @Published property wrapper is correctly used for SwiftUI binding, and the optional String type is appropriate for this use case.
Podfile (1)
31-31: Clarify scope: Push notification implementation not mentioned in PR objectives
The addition of Firebase/Messaging suggests implementation of push notifications, but this isn't mentioned in the PR objectives which focus on screen redesigns and optimizations. Please clarify if this is an intentional addition or if it should be part of a separate PR focused on push notification implementation.
functions/lib/locales/en.json (2)
2-9: LGTM! Group-related messages are well-structured.
The messages use consistent formatting and proper quotation marks around group names for better readability.
18-24: LGTM! General messages are clear and concise.
The messages provide good fallback values and proper placeholder usage.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift (1)
31-34: LGTM! Clean async wrapper implementation.
The method follows Swift concurrency best practices and has a clear, descriptive name that matches its functionality.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidView.swift (2)
18-18: LGTM! Improved error handling with optimized data fetching.
The change to use fetchGroupWithMembersData aligns with the PR's goal of optimizing API calls and provides a more consistent approach to data fetching during error recovery.
44-44: LGTM! Enhanced consistency in data fetching.
The update to use fetchGroupWithMembersData in onAppear ensures a unified approach to data fetching across different view states, improving code maintainability.
Splito/UI/Home/HomeRouteView.swift (2)
37-37: LGTM! Clean implementation of view model sharing.
The addition of environmentObject follows SwiftUI best practices for dependency injection, enabling proper state management between the home route and activity log views.
56-60: LGTM! Well-structured notification handling.
The notification handler is implemented correctly with proper type checking and error handling. This implementation supports the new feature allowing users to navigate to specific activities from notifications.
A few points worth noting:
- The userInfo dictionary access is safely unwrapped
- The type casting is explicit and safe
- The implementation aligns with the PR objective of improving activity log navigation
Let's verify the notification name constant and handler implementation:
✅ Verification successful
Notification handling implementation verified successfully
The implementation is correctly structured and all components are properly connected:
Notification.Name.showActivityLogis properly defined in the extensions- The notification is posted from AppDelegate with correct userInfo dictionary
switchToActivityLogmethod in HomeRouteViewModel handles the navigation by updating the activityLogId and tab selection- The notification observer in HomeRouteView safely unwraps and processes the payload
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the notification name constant and related implementations
# Check for the notification name constant definition
rg -l "static let showActivityLog"
# Verify all places where this notification is posted
rg "post.*showActivityLog"
# Check the switchToActivityLog method implementation
ast-grep --pattern 'func switchToActivityLog(activityId: String)'
Length of output: 331
Script:
#!/bin/bash
# Let's verify the notification extension and view model implementation
# Check the notification extension implementation
cat Data/Data/Extension/Notification+Extension.swift
# Find and check the view model implementation containing switchToActivityLog
rg -l "class.*HomeViewModel"
rg "func switchToActivityLog" -A 5
Length of output: 1476
Data/Data/Model/AppUser.swift (3)
18-18: LGTM: FCM token property properly defined
The new deviceFcmToken property is correctly implemented as an optional String with appropriate access level.
58-58: LGTM: Coding key properly defined
The new coding key for deviceFcmToken follows the established pattern and uses the correct snake_case convention for JSON serialization.
37-38: Verify the impact of imageUrl parameter rename
The initializer has been updated correctly with the new FCM token parameter. The rename from profileImageUrl to imageUrl improves consistency with the property name, but we should verify all call sites.
Also applies to: 44-45
Data/Data/Store/ActivityLogStore.swift (2)
17-18: LGTM! Good practice for listener management.
The addition of the listener property and its cleanup in deinit is a good practice that prevents memory leaks by ensuring proper cleanup of Firestore listeners.
Also applies to: 26-28
30-32: Verify the frequency of fetchLatestActivityLogs calls.
Since the PR aims to optimize API calls, let's verify that this method isn't being called unnecessarily in the view models.
✅ Verification successful
Let me analyze the usage pattern further by checking the initialization and lifecycle methods that might trigger fetchLatestActivityLogs.
Let me check one more aspect to understand the initialization and lifecycle methods.
Based on the complete context, I can now provide the final response:
The current implementation of fetchLatestActivityLogs is well-optimized
The implementation shows a proper lifecycle management:
fetchLatestActivityLogsis called only once during initialization- Real-time updates are handled through a single listener that's properly cleaned up
- Additional calls are triggered only through user actions:
- Pull-to-refresh via
.refreshable - Loading more items via pagination (
.onAppearon progress view)
- Pull-to-refresh via
The PR's optimization goal is already achieved through proper listener cleanup and controlled refresh mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to fetchLatestActivityLogs to verify usage patterns
rg -A 5 "fetchLatestActivityLogs"
# Search for potential view model implementations that might cause repeated calls
ast-grep --pattern 'class $_ {
$$$
fetchLatestActivityLogs($$$)
$$$
}'
Length of output: 3067
Script:
#!/bin/bash
# Check the full implementation of ActivityLogViewModel to understand when fetchLatestActivityLogs is called
rg -A 20 "class ActivityLogViewModel" Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
# Check for potential observers or bindings that might trigger multiple calls
rg -B 5 -A 10 "onAppear|\.onChange|viewDidAppear|viewWillAppear" Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
Length of output: 1022
Script:
#!/bin/bash
# Search for the initialization and any potential lifecycle methods in ActivityLogViewModel
rg -B 2 -A 10 "init\(|fetchActivityLogsInitialData|viewDidLoad|onAppear" Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
# Check if there are any observers or bindings in the ActivityLog view
rg -B 2 -A 10 "ActivityLogView|\.onAppear" "Splito/UI/Home/ActivityLog/"
Length of output: 16254
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (3)
32-32: LGTM! Method name change improves clarity.
The renamed method better describes its functionality of fetching both group and member data.
35-39: LGTM! Proper async task handling.
The implementation correctly handles asynchronous operations using Swift's async/await pattern.
Line range hint 42-53: Consider caching group data to optimize the two-step fetch process.
While the new implementation ensures data consistency by first fetching the group and then its members, it introduces an additional API call. Consider implementing caching for group data to minimize latency, especially if the group data is accessed frequently.
Let's verify if group caching is already implemented:
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift (1)
31-31: LGTM! The error handling change aligns with API optimization goals.
The update to use fetchGroupWithMembersData is consistent with the PR's objective to optimize API calls for members and groups.
Let's verify the optimization impact:
✅ Verification successful
Optimization confirmed: fetchGroupWithMembersData is a more efficient implementation
The verification shows that:
fetchInitialMembersDatainExpenseSplitOptionsViewModelmakes separate API calls for membersfetchGroupWithMembersDatainChooseMultiplePayerViewModeloptimizes by:- First fetching the group data
- Then fetching all members in a single batch using the group's member IDs
- This pattern is consistently used across multiple views (
GroupWhoGettingPaidView,ChoosePayerView, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar optimizations in related files and verify API call reduction
# Search for other views that might need similar optimization
ast-grep --pattern 'fetchInitialMembersData'
# Look for the optimized method implementation
ast-grep --pattern 'func fetchGroupWithMembersData'
Length of output: 533
Script:
#!/bin/bash
# Let's check the implementation details of both methods and their usage patterns
# Check the implementation of fetchInitialMembersData
rg "fetchInitialMembersData" -A 10
# Look for any references to fetchGroupWithMembersData
rg "fetchGroupWithMembersData"
# Check the view model associated with ChooseMultiplePayerView
fd "ChooseMultiplePayerViewModel" --type f --exec cat {}
Length of output: 13069
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (1)
44-44: LGTM! Method rename improves clarity.
The renamed method fetchGroupWithMembersData better reflects its purpose of fetching both group and member data.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingView.swift (2)
19-19: LGTM! Error handling implementation looks good.
The error view appropriately handles both no internet and general error states, with the correct callback to refresh the data.
44-44: Verify the impact of the renamed data fetching method.
The onAppear implementation looks good, but let's verify that all callers of the old method name have been updated.
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (2)
64-67: LGTM! Good defensive programming practice.
The added guard statement improves null safety and maintains consistency with error handling patterns used elsewhere in the codebase.
75-75: Verify error handling for empty member lists.
While the change to use group.members aligns with the PR's optimization goals, please ensure proper handling of edge cases:
- Empty member lists
- Invalid member IDs
✅ Verification successful
Let me gather more information about the error handling in the repository implementation.
Let me check one more aspect of the implementation to see how the repository handles the member fetching.
Based on the implementation details found, I can now provide the final response:
Empty member lists are properly handled in the implementation
The fetchMembersBy implementation has appropriate handling for both empty and cached cases:
- Empty member lists will return an empty array (implicit in the implementation)
- Cached members are efficiently filtered and returned
- Missing members are fetched asynchronously with proper error propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for empty member list handling in the codebase
# Look for empty array checks on group.members
rg -A 2 "group\.members\.isEmpty|group\.members\.count\s*==\s*0"
# Check if GroupRepository's fetchMembersBy handles empty arrays
ast-grep --pattern 'func fetchMembersBy(memberIds: [String]) $$$'
Length of output: 343
Script:
#!/bin/bash
# Check GroupRepository implementation for error handling
rg -A 5 "func fetchMembersBy"
# Look for any throw statements or error handling related to members
rg -A 2 "throw.*Member"
# Check for any custom error types related to members
rg "enum.*Error.*case.*member"
Length of output: 693
Script:
#!/bin/bash
# Check the complete implementation of fetchMembersBy
rg -B 5 -A 15 "func fetchMembersBy"
# Look for any validation or error handling in GroupSettleUpViewModel
rg -B 3 -A 5 "GroupSettleUpViewModel.*init"
Length of output: 1803
Splito/UI/Home/Expense/Detail Selection/SelectGroupView.swift (3)
40-44: LGTM! Clean implementation of group cell initialization.
The code correctly handles the last group check using a safe ID comparison approach.
49-50: LGTM! Good spacing adjustment.
The added bottom spacing improves the visual layout of the group list.
101-101: LGTM! Well-defined property.
The constant property is appropriately declared and follows Swift naming conventions.
Data/Data/Repository/ExpenseRepository.swift (1)
23-28: Improved user identification handling looks good!
The addition of the guard statement for userId and explicit assignment to updatedBy enhances code safety and clarity. This change aligns well with Swift best practices by providing early returns and explicit null handling.
Let's verify the balance update functionality mentioned in the PR objectives:
✅ Verification successful
Based on the shell script results, I can now generate the final response as the code shows comprehensive evidence of balance update functionality:
Balance update functionality is properly implemented
The codebase shows a robust implementation of balance updates when expenses are deleted:
- The
updateExpensecall in the deletion flow properly triggers balance recalculation - Balance updates are handled through the
CalculateExpensesFunctions.swiftwhich contains comprehensive logic for:- Recalculating member balances
- Updating total summaries
- Handling expense deletion cases
- Changes are properly persisted through
groupRepository.updateGroup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for balance calculation implementations in related files
# Search for balance calculation methods
rg -A 5 "balance|owe.*amount" --type swift
# Look for group member update implementations
ast-grep --pattern 'func update.*Group.*Member'
Length of output: 44969
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
119-120: LGTM! Improved user identification handling.
The changes properly handle user identification through preferences and correctly track group updates.
Also applies to: 123-123
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift (1)
31-31: LGTM! Verify error recovery behavior.
The change to use fetchGroupWithMembersData aligns with the PR's optimization goals. However, let's ensure proper error recovery behavior.
✅ Verification successful
Based on the search results, I can now generate the final response as the error handling pattern is clear across the codebase.
Error handling implementation is consistent with app-wide patterns
The change to use fetchGroupWithMembersData follows the established error handling pattern across the app where similar views like GroupWhoIsPayingView and ChooseMultiplePayerView also use the same method. The error recovery behavior is consistent with the app's standard approach of:
- Showing
ErrorViewfor network issues - Transitioning to
LoaderViewduring data fetch - Displaying appropriate content views based on the response
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consistent error handling patterns across similar views
rg -A 5 "ErrorView.*fetch.*Data"
Length of output: 13373
Splito/UI/Home/Account/AccountHomeView.swift (1)
58-58: Verify toast positioning consistency across screens
The removal of bottomPadding: 32 parameter from .toastView might affect toast notification positioning. Please ensure this change:
- Maintains consistent toast positioning across all screens
- Doesn't cause layout issues on different device sizes
- Aligns with the overall UI redesign objectives
Let's verify the toast implementation across other screens:
✅ Verification successful
Toast implementation is consistent across screens
Based on the search results, the removal of bottomPadding parameter from .toastView in AccountHomeView aligns with the implementation across all other screens in the app. Every screen consistently uses the simple .toastView(toast: $viewModel.toast) modifier without any additional padding parameters, indicating this is the standard implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other toastView implementations to ensure consistency
rg -l "\.toastView" | while read -r file; do
echo "=== $file ==="
rg "\.toastView" "$file" -A 1 -B 1
done
Length of output: 11595
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
27-27: LGTM: Improved visual consistency
The spacing adjustments and styling changes enhance the overall layout while maintaining a clean structure.
Also applies to: 48-49, 51-51
53-57: LGTM: Clean component integration
The new AmountRowView is well-integrated with appropriate spacing and follows a logical flow in the view hierarchy.
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift (3)
64-64: LGTM: Title change aligns with PR objectives
The navigation title change from "Transaction detail" to "Payment detail" is consistent with the PR's goal to standardize terminology across the app.
130-131: LGTM: Enhanced visual hierarchy
The addition of background color and corner radius improves the visual separation of components.
158-158: Verify the impact of reduced ProfileCardView height
The height reduction from 97 to 87 pixels might affect the layout. Please verify that:
- Profile images and names remain properly centered
- No content gets cut off on smaller devices
- The layout remains consistent across different screen sizes
✅ Verification successful
Let me gather more information about the ProfileCardView to better understand its implementation and usage.
Let me search for the complete implementation of ProfileCardView to understand its layout structure.
Let me try a different approach to get the ProfileCardView implementation.
Based on the implementation details found, I can now provide the final response:
Height change in ProfileCardView is safe to proceed
The height reduction from 97 to 87 pixels is safe because:
- The ProfileCardView has a simple layout with just two elements (MemberProfileImageView and a single-line Text) in a VStack with 8px spacing
- The implementation is consistent across both usage locations (GroupTransactionDetailView and GroupPaymentView)
- The width is dynamically calculated based on the device size, ensuring proper scaling
- The Text view has a lineLimit(1), preventing any content overflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of ProfileCardView to ensure consistent height
rg -l "ProfileCardView" | while read -r file; do
echo "=== $file ==="
rg "height:|frame\(height:" "$file"
done
Length of output: 953
Script:
#!/bin/bash
# Search for ProfileCardView implementation and its usages
echo "=== ProfileCardView Implementation ==="
ast-grep --pattern 'struct ProfileCardView'
echo -e "\n=== ProfileCardView Usage Context ==="
rg "ProfileCardView" -B 2 -A 2
Length of output: 3718
Script:
#!/bin/bash
# Get the complete implementation of ProfileCardView
echo "=== ProfileCardView Complete Implementation ==="
rg -U "struct ProfileCardView.*?\n}" -A 50 "Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift"
Length of output: 258
Script:
#!/bin/bash
# Get the ProfileCardView implementation with more context
echo "=== ProfileCardView Implementation ==="
rg -A 50 "^struct ProfileCardView" "Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift"
# Also check for any view modifiers related to ProfileCardView
echo -e "\n=== ProfileCardView View Modifiers ==="
rg "ProfileCardView.*\.(frame|padding|background)" "Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift"
Length of output: 1970
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
235-235: LGTM: Success message update aligns with terminology changes
The message update correctly reflects the PR's objective to rename "Transaction" to "Payment" throughout the application.
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (2)
59-63: LGTM: Improved error handling and API optimization
The added guard statement prevents unnecessary API calls when the group is nil, which aligns with the PR's objective of optimizing API calls.
65-65: Verify the impact of the member fetching changes
The change to fetch members by IDs instead of group ID could affect other screens that rely on group member data.
✅ Verification successful
Member fetching changes are consistent across the codebase
The verification shows that all group member fetching operations across the codebase consistently use the fetchMembersBy(memberIds:) method. This includes:
- Group list view
- Group settings
- Settle up flows
- Balance calculations
- Expense split options
- Payer selection screens
The implementation in GroupRepository suggests an optimization where it filters out existing members before making API calls, indicating proper caching considerations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of member fetching methods to ensure consistency
rg -A 3 "fetchMembersBy|fetchGroupMembers" --type swift
# Look for potential group member caching implementations
ast-grep --pattern 'class GroupRepository {
$$$
private var groupMembers: $_
$$$
}'
Length of output: 7511
Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailViewModel.swift (3)
110-110: LGTM! Improved validation order.
The validation sequence ensures proper permission checks before proceeding with group member validation.
116-117: LGTM! Consistent terminology.
The alert messages now correctly use "payment" terminology, aligning with the PR objectives.
162-163: LGTM! Clear and consistent messaging.
The alert and success messages maintain consistent terminology and provide clear feedback to users.
Also applies to: 182-182
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift (3)
251-257: LGTM! Clear and well-formatted empty state message
The updated text maintains consistent terminology while providing clear guidance to users. The typography settings (tracking and line spacing) ensure good readability.
Line range hint 113-122: Well-implemented scroll-to-top functionality
The implementation includes visibility management and smooth animation, aligning with the PR objective to add easy scrolling to the top of the screen. The ScrollToTopButton visibility is properly managed based on scroll position.
43-43: Consider renaming related components for consistency
While the text has been updated from "Transactions" to "Payments", the file name and struct names (e.g., GroupTransactionListView, TransactionListWithDetailView, TransactionItemView) still use "Transaction". Consider renaming these for consistency.
Splito/UI/Home/Groups/Group/GroupHomeView.swift (2)
35-41: LGTM: Text changes align with terminology standardization
The replacement of "transactions" with "payments" in the EmptyStateView subtitles maintains consistency with the PR's objective of standardizing terminology throughout the application.
69-69: Verify toast implementation in view model
The toast view binding looks correct, but let's ensure proper initialization and handling in the view model.
✅ Verification successful
Toast implementation is properly handled
The toast view binding is correctly implemented. The GroupHomeViewModel inherits from BaseViewModel which provides the toast property and proper methods for showing toasts. The view model includes error handling that utilizes the toast functionality through showToastForError() method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for toast-related implementations in the view model
rg -l "toast" --type swift | grep "GroupHomeViewModel"
Length of output: 141
Script:
#!/bin/bash
# Search for GroupHomeViewModel file and its content
fd "GroupHomeViewModel.swift" --type f --exec cat {}
Length of output: 8858
Script:
#!/bin/bash
# Search for BaseViewModel to check toast implementation
fd "BaseViewModel.swift" --type f --exec cat {}
Length of output: 1671
Splito/UI/Home/Groups/GroupListView.swift (1)
109-109: Verify toast message content and timing
The activation of .toastView modifier is good for providing user feedback. However, ensure that the toast messages in viewModel.toast are properly set for all relevant user actions, especially for:
- Group creation/joining success
- Expense addition confirmation
- Error scenarios
Consider implementing a centralized toast management system using an environment object to handle toasts consistently across the app, especially given the app-wide redesign effort.
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (3)
149-150: LGTM: Alert message terminology update
The alert message changes align with the PR objective to rename "Transaction" to "Payment" for improved clarity and consistency.
281-281: LGTM: Success message terminology update
The success message change aligns with the PR objective to rename "Transaction" to "Payment" for improved clarity and consistency.
162-165: LGTM: Improved validation for transaction deletion
Good addition of validation checks to ensure both payer and receiver are still group members before proceeding with deletion.
Let's verify error handling for the guard statement:
✅ Verification successful
Based on the search results, I can see a consistent pattern of error handling across the codebase for guard statements involving group validation. The code under review follows the same pattern where it silently returns on validation failure, which is consistent with other similar scenarios in the codebase, particularly in view models.
LGTM: Error handling is consistent with codebase patterns
The guard statement's error handling (silent return) aligns with the established patterns across the codebase, particularly in view models where group validation fails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in similar validation contexts
rg -A 3 "guard\s+let\s+group.*else\s*\{" --type swift
Length of output: 15667
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
420-420: Verify date field validation implementation
The date case has been added to AddExpenseField enum, but there doesn't appear to be any corresponding validation logic in the view model. This might be related to the date picker height fix mentioned in the PR objectives.
✅ Verification successful
Let me gather more information about the AddExpenseField enum and its usage.
Let me search for more context about the AddExpenseField enum and any field validation.
Let me check how the date field is used in the AddExpenseView to understand if validation is needed.
Based on the gathered information, I can now provide the final response:
The date case addition is correctly implemented without need for validation
The date case in AddExpenseField enum is appropriately used for focus management in the UI, specifically in ExpenseDetailRow with the date picker. Unlike other fields like expense name or amount, the date field doesn't require explicit validation since:
- It uses SwiftUI's
DatePickerwith a predefined date range (...maximumDate) - The date is always bound to a valid
Datevalue through@Binding - The field is used for focus state management rather than validation purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new date case in validation logic
rg -l "AddExpenseField.date"
# Search for date picker related view code
rg "DatePicker|datePicker" --type swift
Length of output: 1280
Script:
#!/bin/bash
# Search for AddExpenseField enum definition and usage
ast-grep --pattern 'enum AddExpenseField'
# Search for validation logic in AddExpenseViewModel
rg -A 5 "func validate|func isValid" Splito/UI/Home/Expense/AddExpenseViewModel.swift
Length of output: 435
Script:
#!/bin/bash
# Search for the complete AddExpenseField enum definition
rg -B 5 -A 10 "enum AddExpenseField" Splito/UI/Home/Expense/AddExpenseViewModel.swift
# Search for any field validation or error handling related to fields
rg "field|Field|error|Error" Splito/UI/Home/Expense/AddExpenseViewModel.swift
Length of output: 948
Script:
#!/bin/bash
# Get the content of AddExpenseView to see how date is handled
rg -A 5 "DatePickerRow|struct DatePickerRow|date:" Splito/UI/Home/Expense/AddExpenseView.swift
# Check if there are any validation calls or bindings in AddExpenseView
rg "viewModel\.validate|\.isValid|@StateObject.*AddExpenseViewModel" Splito/UI/Home/Expense/AddExpenseView.swift
Length of output: 2691
Splito.xcodeproj/project.pbxproj (1)
1284-1284: LGTM! The switch to automatic code signing is a good improvement.
The changes to code signing configuration are well-structured:
- Standardizing to "Apple Development" signing identity
- Enabling automatic code signing
- Setting consistent development team
- Removing manual provisioning profile specification
This simplifies the development workflow and reduces potential code signing issues.
Also applies to: 1368-1368, 1382-1383, 1386-1386
Splito/AppDelegate.swift (2)
89-90:
Correct syntax for optional binding in if let statement
The optional binding syntax if let error is incorrect. It should be if let error = error.
Apply this diff to fix the syntax error:
- if let error {
+ if let error = error {Likely invalid or redundant comment.
31-33:
Correct syntax for optional binding in if let statement
The optional binding syntax if let error is incorrect. It should be if let error = error.
Apply this diff to fix the syntax error:
- if let error {
+ if let error = error {Likely invalid or redundant comment.
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (1)
179-195: Validation logic for group members is correct
The validateGroupMembers function correctly identifies missing group members and handles the error appropriately.
Splito/UI/Home/Expense/AddExpenseView.swift (2)
183-185: Verify the icon used in ScrollToTopButton
The ScrollToTopButton is using the "chevron.down" icon, which might be counterintuitive for a button intended to scroll to the top. Typically, an upward-pointing chevron ("chevron.up") is used to indicate scrolling upwards.
Consider changing the icon to "chevron.up":
-ScrollToTopButton(icon: "chevron.down", iconColor: disableText, bgColor: surfaceColor,
+ScrollToTopButton(icon: "chevron.up", iconColor: disableText, bgColor: surfaceColor,
size: (12, 12), padding: 0, onClick: onTap)194-196:
Check the usage of .onTapGestureForced modifier
The modifier .onTapGestureForced is not a standard SwiftUI method. If this is a custom extension, ensure it's properly defined and its usage is intentional. Otherwise, consider replacing it with the standard .onTapGesture.
Apply this diff if appropriate:
-.onTapGestureForced {
- onTap?()
-}
+.onTapGesture {
+ onTap?()
+}Verify if the custom .onTapGestureForced is necessary or if the standard modifier suffices.
Splito/UI/Home/ActivityLog/ActivityLogView.swift (2)
104-104: Verify Custom Font Modifier .subTitle1()
The font modifier .subTitle1() used in line 104 should be confirmed to exist within your custom font extensions.
If not already defined, ensure that .subTitle1() is implemented to avoid runtime errors.
47-47: Confirm Injection of HomeRouteViewModel Environment Object
The addition of @EnvironmentObject var homeRouteViewModel in ActivityLogListView requires that HomeRouteViewModel is provided in the ancestor view's environment.
Ensure that when ActivityLogListView is instantiated, HomeRouteViewModel is injected properly. For example:
ActivityLogListView(viewModel: viewModel)
.environmentObject(homeRouteViewModel)Splito/UI/Home/Groups/GroupListViewModel.swift (3)
72-72: Evaluate the impact of asynchronous call ordering
Calling fetchLatestUser() after await fetchGroups() may introduce delays in updating the user's total owe amount. Ensure that this change does not negatively affect the user experience, especially if the owe amount needs to be displayed promptly.
137-137: Ensure error propagation is handled appropriately
In fetchGroupInformation, the call to try await groupRepository.fetchMembersBy(memberIds: group.members) may throw an error. Confirm that any errors not caught here are handled at a higher level to prevent unhandled exceptions.
354-376: Improved error handling in processGroup method
Adding the do-catch block enhances error management during member fetching in processGroup. This ensures that any exceptions are caught and handled gracefully, improving the application's robustness.
...ome/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidViewModel.swift
Show resolved
Hide resolved
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift
Show resolved
Hide resolved
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
Splito/AppDelegate.swift (2)
26-41: LGTM: Well-structured push notification registrationThe implementation follows best practices with proper error handling and thread management. Consider adding a success log when authorization is granted for better debugging.
guard granted else { return } +LogI("Push notification authorization granted") DispatchQueue.main.async {
88-102: Consider potential race conditions in retry mechanismWhile the retry logic is well implemented, multiple retries could potentially overlap if a new token update is triggered before the previous retries complete. Consider adding a mechanism to cancel pending retries when a new token update is initiated.
class AppDelegate: NSObject, UIApplicationDelegate, MessagingDelegate, UNUserNotificationCenterDelegate { + private var tokenUpdateWorkItem: DispatchWorkItem? + func updateDeviceFcmToken(userId: String, fcmToken: String, retryCount: Int = 3) { + tokenUpdateWorkItem?.cancel() Firestore.firestore().collection("users").document(userId).setData([ "device_fcm_token": fcmToken ], merge: true) { error in if let error { LogE("Error updating device FCM token: \(error)") if retryCount > 0 { + let workItem = DispatchWorkItem { [weak self] in + self?.updateDeviceFcmToken(userId: userId, fcmToken: fcmToken, retryCount: retryCount - 1) + } + self.tokenUpdateWorkItem = workItem DispatchQueue.main.asyncAfter(deadline: .now() + 2.0) { [weak self] in - self?.updateDeviceFcmToken(userId: userId, fcmToken: fcmToken, retryCount: retryCount - 1) + workItem.perform() } } } else {Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (3)
38-39: Fix indentation in notification observer setup.The notification observer setup has inconsistent indentation. Swift style guidelines recommend aligning parameters vertically.
- NotificationCenter.default.addObserver(self, selector: #selector(getUpdatedExpense(notification:)), - name: .updateExpense, object: nil) + NotificationCenter.default.addObserver(self, + selector: #selector(getUpdatedExpense(notification:)), + name: .updateExpense, + object: nil)
185-201: Consider adjusting alert timing and extracting delay constant.The implementation is solid, using efficient Set operations to validate group membership. However, the alert delay of 0.1 seconds seems arbitrary and might be too short for proper UI feedback.
private func validateGroupMembers(action: String) -> Bool { guard let group, let expense else { LogE("ExpenseDetailsViewModel: Missing required group or expense.") return false } let missingMemberIds = Set(expense.splitTo + Array(expense.paidBy.keys)).subtracting(group.members) if !missingMemberIds.isEmpty { - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { + // Standard UI feedback delay for better user experience + let standardAlertDelay: TimeInterval = 0.3 + DispatchQueue.main.asyncAfter(deadline: .now() + standardAlertDelay) { 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.") } return false } return true }
194-196: Consider centralizing error messages in a dedicated constants file.To improve maintainability and localization support, consider moving error messages to a dedicated constants file or localization system.
This would help:
- Maintain consistency across error messages
- Simplify localization
- Make message updates easier to manage
- Reduce string duplication
Example structure:
enum ExpenseError { static let memberLeftGroup = { (action: String) in "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." } // ... other error messages }Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (2)
290-290: LGTM with a suggestion for improvementThe padding provides consistent spacing, but consider extracting common padding values (16) into a constant or design system value for better maintainability.
private enum Layout { static let standardPadding: CGFloat = 16 }
Line range hint
1-400: Implementation aligns well with PR objectivesThe code successfully implements several PR objectives:
- Pull-to-refresh functionality in the activity log
- Scroll-to-top feature
- Pagination for performance optimization
The implementation is clean, well-structured, and follows SwiftUI best practices.
Consider implementing view state management using a dedicated enum to handle loading, error, and empty states more systematically:
enum GroupExpenseListState { case loading case empty case error(Error) case loaded([ExpenseWithUser]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Data/Data/Model/AppUser.swift(3 hunks)Splito.xcodeproj/project.pbxproj(3 hunks)Splito/AppDelegate.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(7 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Data/Data/Model/AppUser.swift
- Splito.xcodeproj/project.pbxproj
🔇 Additional comments (15)
Splito/AppDelegate.swift (4)
13-17: LGTM: Required imports and protocol conformance added correctly
The new imports and protocol conformance are properly implemented for Firebase Cloud Messaging and UserNotifications handling.
47-59: LGTM: Proper notification handling implementation
Good implementation with proper error logging and main thread handling for UI updates.
66-71: Previous review comment about completionHandler is still valid
The completion handler should be called in all cases, not just when Firebase can handle the notification.
77-86: Previous review comment about optional binding syntax is still valid
The optional binding syntax issue in the guard statement needs to be fixed.
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (3)
108-109: LGTM: Robust validation chain for edit action.
The validation sequence properly checks both user permissions and group member status before allowing edits.
132-134: LGTM: Comprehensive validation and update logic for restore action.
The implementation properly validates permissions and group membership before restoring an expense, followed by correct expense update with proper type annotation.
Also applies to: 143-144
166-167: LGTM: Consistent validation implementation for delete action.
The delete action follows the same robust validation pattern used throughout the class.
Splito/UI/Home/ActivityLog/ActivityLogView.swift (5)
Line range hint 34-44: LGTM: Toast, alert, and toolbar implementation
The implementation follows SwiftUI best practices with proper styling and error handling.
90-94: LGTM: Clean scroll implementation
The scrollToActivityLog function is well-implemented with proper animation handling.
105-107: LGTM: Consistent styling
The font and tracking adjustments follow the design system guidelines.
215-215: LGTM: Proper text alignment
The leading alignment is appropriate for description text layout.
52-85: Verify scroll performance with large datasets
The ScrollViewReader implementation looks good, but consider the following performance aspects:
- Ensure smooth scrolling with large datasets
- Test scroll-to behavior with various screen sizes
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification of the scroll performance concerns. The implementation shows several important patterns:
- The ActivityLogView uses LazyVStack for efficient loading
- Pagination is implemented via loadMoreActivityLogs
- The ScrollViewReader is used appropriately with proper ID management
- The scrolling behavior is triggered in specific scenarios (onAppear and onChange)
ScrollView implementation follows performance best practices
The implementation demonstrates good performance practices:
- Uses LazyVStack for memory efficiency with large datasets
- Implements pagination to handle data in chunks
- Employs ScrollViewReader with proper ID-based scrolling
- Handles scroll animations within proper lifecycle events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential memory management or performance related code
rg -g '*.swift' -A 3 'ScrollViewReader|\.scrollTo|\.onAppear'
Length of output: 18789
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (3)
93-93: LGTM: Improved SwiftUI modifier placement
Moving the padding modifier outside the onAppear closure follows SwiftUI best practices for view modifiers.
111-111: LGTM: Consistent modifier placement
Moving the padding modifier outside the closure maintains consistency with SwiftUI's view modifier best practices.
362-362: LGTM: Improved text styling consistency
Moving the font modifier outside the Text combination ensures consistent styling across the entire text group.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
Splito/AppDelegate.swift (3)
26-41: Consider enhancing error handling feedback.The implementation is solid with proper delegate setup, permission handling, and thread management. However, consider providing more detailed error feedback for analytics or debugging purposes.
private func registerForPushNotifications(application: UIApplication) { Messaging.messaging().delegate = self UNUserNotificationCenter.current().delegate = self let options: UNAuthorizationOptions = [.alert, .badge, .sound, .provisional] UNUserNotificationCenter.current().requestAuthorization(options: options) { (granted, error) in if let error { - LogE("Failed to request notification authorization: \(error)") + LogE("Failed to request notification authorization: \(error.localizedDescription), domain: \(error.domain), code: \(error._code)") return } - guard granted else { return } + guard granted else { + LogI("User denied notification permissions") + return + } DispatchQueue.main.async { application.registerForRemoteNotifications() } } }
47-59: Consider enhancing notification payload handling.While the implementation is functional, consider these improvements for robustness:
- Type-safe payload handling
- Additional validation for activity ID format
- Structured logging for notification events
func userNotificationCenter(_ center: UNUserNotificationCenter, didReceive response: UNNotificationResponse, withCompletionHandler completionHandler: @escaping () -> Void) { let userInfo = response.notification.request.content.userInfo + LogI("Received notification: \(userInfo)") - if let activityId = userInfo["activityId"] as? String { + if let activityId = userInfo["activityId"] as? String, !activityId.isEmpty { DispatchQueue.main.async { [weak self] in NotificationCenter.default.post(name: .showActivityLog, object: self, userInfo: ["activityId": activityId]) } } else { - LogE("Activity id not found in notification data.") + LogE("Invalid or missing activity ID in notification data: \(userInfo)") } completionHandler() }Also applies to: 66-69
86-100: Consider enhancing FCM token update reliability.The retry mechanism is good, but consider these improvements:
- Add exponential backoff between retries
- Validate token format before updating
- Consider persisting failed updates for retry on next app launch
func updateDeviceFcmToken(userId: String, fcmToken: String, retryCount: Int = 3) { + guard !fcmToken.isEmpty else { + LogE("Invalid FCM token") + return + } + + let backoffDelay = pow(2.0, Double(3 - retryCount)) // Exponential backoff + Firestore.firestore().collection("users").document(userId).setData([ "device_fcm_token": fcmToken ], merge: true) { error in if let error { - LogE("Error updating device FCM token: \(error)") + LogE("Error updating device FCM token: \(error.localizedDescription)") if retryCount > 0 { - DispatchQueue.main.asyncAfter(deadline: .now() + 2.0) { [weak self] in + DispatchQueue.main.asyncAfter(deadline: .now() + backoffDelay) { [weak self] in self?.updateDeviceFcmToken(userId: userId, fcmToken: fcmToken, retryCount: retryCount - 1) } + } else { + // Consider persisting failed update for retry on next launch + LogE("Failed to update FCM token after all retries") } } else { LogI("Device fcm token successfully updated in Firestore") } } }Splito/UI/Home/Groups/Create Group/CreateGroupView.swift (1)
144-146: LGTM! Good UX improvement for the Remove button.The addition of
.destructiverole properly indicates this as a destructive action, following iOS Human Interface Guidelines.Consider these additional UX improvements:
Button("Remove", role: .destructive) { + // Add confirmation dialog for destructive action handleActionSelection(.remove) -} +} +.accessibilityLabel("Remove group image")Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
145-161: Consider enhancing the validation error message and timing.While the validation logic is solid, there are two potential improvements:
- The 0.1 second delay in showing the alert seems arbitrary. Consider showing it immediately or explaining the delay in a comment.
- The error message could be more specific by including the names of members who have left.
Here's a suggested improvement:
private func validateGroupMembers(expense: Expense) -> Bool { guard let group else { LogE("GroupHomeViewModel: Missing required group.") return false } let missingMemberIds = Set(expense.splitTo + Array(expense.paidBy.keys)).subtracting(group.members) if !missingMemberIds.isEmpty { - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { + let missingMembers = missingMemberIds.compactMap { getMemberDataBy(id: $0)?.name }.joined(separator: ", ") + DispatchQueue.main.async { self.showAlertFor(message: "This expense involves a person who has left the group, and thus it can no longer be deleted. If you wish to change this expense, you must first add that person back to your group.") } return false } return true }Splito/UI/Home/Groups/GroupListViewModel.swift (1)
334-350: Consider extracting the group update logic for better readability.While the implementation is correct, the nested if statements could be simplified for better readability.
Consider this refactoring:
@objc private func handleAddExpense(notification: Notification) { guard let expenseInfo = notification.userInfo, let notificationGroupId = expenseInfo["groupId"] as? String else { return } Task { - if let existingIndex = combinedGroups.firstIndex(where: { $0.group.id == notificationGroupId }) { - if let updatedGroup = await fetchGroup(groupId: notificationGroupId) { - do { - let groupInformation = try await fetchGroupInformation(group: updatedGroup) - combinedGroups[existingIndex] = groupInformation - } catch { - showToastForError() - } - } - } + guard let existingIndex = combinedGroups.firstIndex(where: { $0.group.id == notificationGroupId }), + let updatedGroup = await fetchGroup(groupId: notificationGroupId) else { return } + + do { + let groupInformation = try await fetchGroupInformation(group: updatedGroup) + combinedGroups[existingIndex] = groupInformation + } catch { + showToastForError() + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Podfile.lockis excluded by!**/*.lockfirebase-debug.logis excluded by!**/*.log
📒 Files selected for processing (8)
BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/AccentColor.colorset/Contents.json(1 hunks)Splito/AppDelegate.swift(1 hunks)Splito/Resource/Assets.xcassets/AccentColor.colorset/Contents.json(0 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupView.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(5 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(4 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(4 hunks)functions/src/users_service/users_service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- Splito/Resource/Assets.xcassets/AccentColor.colorset/Contents.json
✅ Files skipped from review due to trivial changes (1)
- BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/AccentColor.colorset/Contents.json
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/Groups/Group/GroupExpenseListView.swift
- functions/src/users_service/users_service.ts
🔇 Additional comments (8)
Splito/AppDelegate.swift (2)
13-17: LGTM! Necessary imports and protocol conformance added.
The added imports and protocol conformances are required for implementing push notification handling with Firebase Cloud Messaging.
22-22: LGTM! Proper initialization sequence.
The push notification registration is correctly placed after Firebase configuration.
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
95-95: LGTM! Alert message update aligns with terminology changes.
The alert message now uses "payments" instead of "transactions", which is consistent with the PR objective to standardize terminology.
130-130: LGTM! Good addition of member validation before expense deletion.
The validation check ensures data integrity by preventing deletion of expenses involving former group members.
235-235: LGTM! Success message update aligns with terminology changes.
The success message now uses "Payment" instead of "Transaction", which is consistent with the PR objective to standardize terminology.
Splito/UI/Home/Groups/GroupListViewModel.swift (3)
64-64: LGTM! Proper initialization and data fetching sequence.
The notification observer for expense handling is correctly set up, and the sequence of operations in fetchGroupsInitialData() ensures groups are loaded before fetching the latest user data.
Also applies to: 72-72
Line range hint 137-144: LGTM! Optimized member fetching implementation.
The change to fetch members by IDs instead of group ID aligns with the PR's optimization goals. The implementation includes proper error handling and correctly constructs the GroupInformation object.
357-379: LGTM! Robust error handling and state management.
The implementation properly handles member fetching errors and maintains correct state for both new and existing groups. The separation of logic between new and existing groups is clear and well-structured.
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/Payment/GroupPaymentViewModel.swift (2)
113-114: Duplicate validation logic detected.The amount validation is duplicated in both
showSaveFailedErrorandhandleSaveAction. Consider extracting this validation into a separate method to maintain DRY principles.+ private func isValidAmount() -> Bool { + return amount > 0 + } func showSaveFailedError() { - guard amount > 0 else { return } + guard isValidAmount() else { return } ... } func handleSaveAction() async -> Bool { - guard amount > 0 else { + guard isValidAmount() else { showAlertFor(title: "Whoops!", message: "Please enter an amount greater than zero.") return false } ... }
Line range hint
192-211: Consider enhancing error handling with specific error typesThe error handling in
addTransactioncould be more specific about the types of errors that can occur, allowing for more targeted error messages to the user.private func addTransaction(transaction: Transactions) async -> Bool { guard let group, let payer, let receiver else { LogE("GroupPaymentViewModel: \(#function) Missing required group or member information.") return false } do { showLoader = true self.transaction = try await transactionRepository.addTransaction(/*...*/) // ... return true - } catch { + } catch let error as NetworkError { + showLoader = false + switch error { + case .connectionError: + showToastFor(toast: ToastPrompt(type: .error, title: "Network Error", message: "Please check your connection")) + case .serverError: + showToastFor(toast: ToastPrompt(type: .error, title: "Server Error", message: "Please try again later")) + default: + showToastForError() + } + return false + } catch { showLoader = false showToastForError() return false } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(1 hunks)
…on&multipayer screen
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
Data/Data/Extension/Double+Extension.swift (1)
30-32: Consider a more descriptive property name.While the implementation is correct, consider renaming
formattedCurrencyWithSignto something more specific likeformattedCurrencyPreservingSignorformattedCurrencyWithNegativeto better indicate its exact behavior.- var formattedCurrencyWithSign: String { + var formattedCurrencyPreservingSign: String { return formattedCurrency(removeMinusSign: false) }Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift (1)
Line range hint
89-146: Consider enhancing accessibility for BottomInfoCardView.The bottom card contains important split information but could benefit from improved accessibility:
- Add accessibility labels for the split amounts
- Include accessibility hints explaining the "All" button functionality
Example implementation:
Text(title.localized) .font(.Header4()) .foregroundStyle(inversePrimaryText) + .accessibilityLabel("Split amount: \(title)") if value != nil { Text(value!.localized) .font(.body1()) .foregroundStyle(inverseDisableText) + .accessibilityLabel("Remaining amount: \(value!)") }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
171-185: Consider tracking failed member fetches.While the task group implementation is good for parallel fetching, it silently drops failed fetches. Consider tracking these failures to:
- Inform the user about partially loaded data
- Enable retry mechanisms for failed fetches
Example implementation:
await withTaskGroup(of: ExpenseWithUser?.self) { taskGroup in + var failedFetches: [String] = [] for expense in expenses.uniqued() { taskGroup.addTask { [weak self] in guard let self else { return nil } if let user = await self.fetchMemberData(for: expense.paidBy.keys.first ?? "") { return ExpenseWithUser(expense: expense, user: user) } + failedFetches.append(expense.paidBy.keys.first ?? "") return nil } } + if !failedFetches.isEmpty { + await MainActor.run { + self.showToast("Some member data could not be loaded") + } + } }Data/Data/Repository/GroupRepository.swift (2)
191-199: Add error handling for network failuresThe method should handle potential network failures gracefully. Consider implementing a retry mechanism or proper error propagation for network-related issues.
public func fetchMemberBy(userId: String) async throws -> AppUser? { if let existingMember = groupMembers.first(where: { $0.id == userId }) { return existingMember } else { + do { let member = try await userRepository.fetchUserBy(userID: userId) if let member { self.groupMembers.append(member) } return member + } catch { + logger.error("Failed to fetch member: \(error)") + throw RepositoryError.networkError(error) + } } }
22-22: Consider implementing cache management strategyThe current implementation of
groupMemberscache lacks:
- Cache invalidation mechanism
- Size limits
- Staleness handling
This could lead to:
- Memory issues from unbounded growth
- Stale data being served
- Inconsistencies across sessions
Consider implementing:
- LRU cache with size limits
- TTL-based invalidation
- Cache clearing on logout/session end
- Periodic refresh mechanism for frequently accessed members
Would you like me to provide a detailed implementation for any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift(2 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
🔇 Additional comments (8)
Data/Data/Extension/Double+Extension.swift (2)
26-28: LGTM! Good backward compatibility.
The property maintains API compatibility while delegating to the new implementation.
11-24: LGTM! Consider testing edge cases.
The implementation is clean and handles currency formatting well, with proper locale support and fallback mechanisms.
Let's verify the handling of edge cases:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the currency formatting implementation:
Currency formatting implementation is robust and well-used
The codebase shows consistent and appropriate usage of the formattedCurrency method across various scenarios:
- Handles both positive and negative amounts correctly
- Used in expense calculations, balances, and transaction displays
- Properly integrated with locale settings
- Provides fallback formatting when needed
- Successfully manages edge cases through NumberFormatter's built-in handling
The implementation is proven through extensive real-world usage in:
- Balance calculations and displays
- Transaction summaries
- Expense splits and totals
- Group financial overviews
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases covering currency formatting edge cases
# Look for test files related to Double+Extension
fd -e swift -e xctestplan ".*Test.*" | rg -l "Double.*Extension|Currency.*Format"
# Look for actual usage with extreme values or special cases
rg "formattedCurrency|NumberFormatter" -A 3 -B 3
Length of output: 23162
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift (2)
125-125: LGTM! Clean refactoring of currency formatting.
The change simplifies the currency display logic by delegating formatting responsibility to a dedicated formatter method, improving code maintainability.
125-125: Verify the currency formatter implementation.
Let's ensure the formattedCurrencyWithSign extension method is properly implemented.
✅ Verification successful
Let me gather more information about the currency formatting implementation since we only see that formattedCurrencyWithSign calls formattedCurrency(removeMinusSign:).
Currency formatter implementation is correctly implemented
The formattedCurrencyWithSign property is properly implemented as an extension on Double. It uses the standard NumberFormatter with .currency style and respects the user's locale settings. The implementation:
- Uses the system's
NumberFormatterfor locale-aware currency formatting - Preserves negative signs when needed via
formattedCurrencyWithSign - Falls back gracefully if formatting fails
- Follows Swift best practices for number formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the formattedCurrencyWithSign implementation
rg -t swift "formattedCurrencyWithSign" -A 5
# Look for any other currency formatting methods to ensure consistency
ast-grep --pattern 'extension Double {
$$$
var formattedCurrency$_: $_ {
$$$
}
$$$
}'
Length of output: 2752
Script:
#!/bin/bash
# Search for the formattedCurrency implementation
rg -t swift "func formattedCurrency" -A 10
# Also look for any NumberFormatter setup related to currency
rg -t swift "NumberFormatter.*currency" -A 5
Length of output: 1033
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift (2)
79-79: LGTM! The sign display improves clarity for remaining amounts.
The change to use formattedCurrencyWithSign is appropriate here as it helps users clearly see whether they've over or under-allocated the expense amount.
Let's verify consistent currency formatting across related views:
✅ Verification successful
Currency formatting is consistently applied across the codebase
The codebase shows a clear pattern where:
formattedCurrencyWithSignis used when displaying remaining/difference amounts that can be negative (in split options and multiple payer views)formattedCurrencyis used for displaying absolute amounts (expense totals, balances, etc.)
The current change aligns with this pattern, making it a valid implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other currency formatting usage in expense-related views
rg "formattedCurrency|formattedCurrencyWithSign" --type swift
Length of output: 5399
76-81: Consider adding validation for amount overflow.
When calculating the remaining amount (viewModel.expenseAmount - viewModel.totalFixedAmount), consider adding validation to handle potential edge cases like:
- Negative expense amounts
- Total fixed amount exceeding expense amount
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
64-64: LGTM! Improved variable naming.
The rename from groupUserData to groupMembers better reflects the variable's purpose and content.
Data/Data/Repository/GroupRepository.swift (1)
22-22: Thread safety concerns with groupMembers cache
The newly added groupMembers cache has the same thread safety concerns as previously identified. Please refer to the existing review comment about ensuring thread-safe access to shared mutable state.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (3)
103-104: Consider batch fetching members for better performanceInstead of fetching members one by one in a loop, consider implementing batch fetching to reduce the number of network calls.
- for member in group.members where member != self.preference.user?.id { - if let memberData = await fetchMemberData(for: member) { - addMemberIfNotExist(memberData) - } - } + let memberIds = group.members.filter { $0 != self.preference.user?.id } + if let members = try? await groupRepository.fetchMembersBy(userIds: memberIds) { + members.forEach { addMemberIfNotExist($0) } + }
171-185: Consider aggregating errors from task group operationsWhile the task group implementation effectively handles concurrent fetching, it silently drops failed operations. Consider collecting and aggregating errors to provide better feedback.
await withTaskGroup(of: ExpenseWithUser?.self) { taskGroup in + var errors: [Error] = [] for expense in expenses.uniqued() { taskGroup.addTask { [weak self] in guard let self else { return nil } if let user = await self.fetchMemberData(for: expense.paidBy.keys.first ?? "") { return ExpenseWithUser(expense: expense, user: user) + } else if let error = self.lastError { + errors.append(error) } return nil } } + if !errors.isEmpty { + self.handleAggregatedErrors(errors) + }
209-214: Remove trailing whitespaceThere is trailing whitespace on line 214 that should be removed.
- +🧰 Tools
🪛 SwiftLint
[Warning] 214-214: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)
🧰 Additional context used
🪛 SwiftLint
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
[Warning] 214-214: Lines should not have trailing whitespace
(trailing_whitespace)
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/Settle up/Payment/GroupPaymentView.swift (2)
27-54: Add accessibility label for the transaction iconThe layout changes look good, but the transaction icon needs accessibility support for better VoiceOver experience.
Image(.transactionIcon) .resizable() .scaledToFit() - .frame(width: 42, height: 42) + .frame(width: 42, height: 42) + .accessibilityLabel("Transaction direction indicator")
102-124: Extract styling constants for consistencyThe implementation looks good, but consider extracting the styling values into constants for better maintainability.
private struct PaymentDateRow: View { + private enum Constants { + static let cornerRadius: CGFloat = 12 + static let strokeWidth: CGFloat = 1 + static let padding: CGFloat = 16 + static let spacing: CGFloat = 8 + } // ... existing code ... var body: some View { - VStack(alignment: .leading, spacing: 8) { + VStack(alignment: .leading, spacing: Constants.spacing) { // ... existing code ... DatePickerRow(date: $date) - .padding(16) + .padding(Constants.padding) .overlay { - RoundedRectangle(cornerRadius: 12) - .stroke(outlineColor, lineWidth: 1) + RoundedRectangle(cornerRadius: Constants.cornerRadius) + .stroke(outlineColor, lineWidth: Constants.strokeWidth) } } } }Splito/UI/Home/Expense/AddExpenseView.swift (3)
98-134: Consider adjusting the horizontal padding value.The
.padding(.horizontal, 1)on line 132 seems unusual. Consider using a more standard padding value (e.g., 0 or 16) or removing it if not necessary.
158-174: Consider enhancing form field navigation.The submit action on the description field always navigates to the amount field. Consider implementing a more flexible field navigation system that can handle different field sequences based on the form's state or configuration.
98-134: Consider implementing comprehensive error handling.While the implementation is solid, consider adding error handling for edge cases such as:
- Invalid amount formats
- Network connectivity issues during group/member fetching
- Date validation errors
This would enhance the robustness of the expense form and improve user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
firebase-debug.logis excluded by!**/*.log
📒 Files selected for processing (5)
Splito/UI/Home/Expense/AddExpenseView.swift(4 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift(5 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/Transaction Detail/GroupTransactionDetailView.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
🔇 Additional comments (4)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
126-177: 🛠️ Refactor suggestion
Previous internationalization concerns still apply
The previous review comment about internationalization and maintainability is still relevant. Additionally:
- Consider using NumberFormatter for proper decimal handling
- Add input validation for maximum amount
struct AmountRowView: View {
+ private let numberFormatter: NumberFormatter = {
+ let formatter = NumberFormatter()
+ formatter.numberStyle = .currency
+ formatter.locale = Locale.current
+ return formatter
+ }()
// ... existing code ...
private func formatAmount(newValue: String) {
- let numericInput = newValue.replacingOccurrences(of: "₹", with: "").trimmingCharacters(in: .whitespaces)
+ let numericInput = newValue.replacingOccurrences(of: numberFormatter.currencySymbol ?? "₹", with: "").trimmingCharacters(in: .whitespaces)
if let value = Double(numericInput) {
+ // Add validation for maximum amount
+ guard value <= 999999999 else {
+ amount = 0
+ amountString = ""
+ return
+ }
amount = value
} else {
amount = 0
}
- amountString = numericInput.isEmpty ? "" : "₹ " + numericInput
+ amountString = numericInput.isEmpty ? "" : "\(numberFormatter.currencySymbol ?? "₹") " + numericInput
}Splito/UI/Home/Expense/AddExpenseView.swift (3)
28-30: LGTM! Clean implementation of the new expense form structure.
The consolidation of expense input fields into ExpenseInfoView improves code organization and maintainability.
195-197: Verify the necessity of forced tap gesture.
The .onTapGestureForced might bypass standard gesture handling. Ensure this is necessary and consider using standard .onTapGesture if possible.
250-250: LGTM! Good fix for the date picker height issue.
Adding .id(tempDate) effectively resolves the height change issue mentioned in the PR objectives by forcing a view refresh when the date changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift (1)
40-47: Consider enhancing error handling and state managementWhile the implementation is functional, consider these improvements:
- Add handling for empty member arrays
- Handle partial member fetch failures
- Add more granular view states for better error feedback
private func fetchGroupWithMembers() async { do { let group = try await groupRepository.fetchGroupBy(id: groupId) guard let group else { currentViewState = .initial return } + guard !group.members.isEmpty else { + currentViewState = .noMembers + return + } self.members = try await groupRepository.fetchMembersBy(memberIds: group.members) + guard !members.isEmpty else { + currentViewState = .membersFetchFailed + return + } currentViewState = .initial } catch { + if let repoError = error as? RepositoryError { + currentViewState = .fetchError(repoError) + } else { handleServiceError() + } } }Don't forget to add the new states to the ViewState enum:
enum ViewState { case initial case loading case noMembers case membersFetchFailed case fetchError(RepositoryError) case noInternet case somethingWentWrong }Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (1)
Line range hint
42-53: Consider enhancing error handling and state managementWhile the implementation correctly fetches group and member data, consider these improvements:
- Add intermediate loading states to reflect the two-step fetch process
- Consider parallel fetching if the group data isn't needed for member validation
- Add specific error handling for different failure cases (e.g., group not found vs network error)
Example enhancement:
private func fetchGroupWithMembers() async { do { + currentViewState = .loadingGroup let group = try await groupRepository.fetchGroupBy(id: groupId) guard let group else { - currentViewState = .noMember + currentViewState = .groupNotFound return } + currentViewState = .loadingMembers let users = try await groupRepository.fetchMembersBy(memberIds: group.members) currentViewState = users.isEmpty ? .noMember : .hasMembers(users) } catch { - handleServiceError() + switch error { + case let GroupError.notFound(message): + currentViewState = .groupNotFound(message) + case let NetworkError.noConnection: + currentViewState = .noInternet + default: + currentViewState = .somethingWentWrong + } } }Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (1)
44-44: Consider a more descriptive method name.The method name
fetchInitialViewDatais quite generic. Consider a more specific name likefetchGroupAndMemberDatato better reflect its purpose and maintain code clarity.Also applies to: 47-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift(2 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidView.swift(2 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/GroupWhoIsPayingView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift
- Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Getting Paid/GroupWhoGettingPaidView.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/GroupWhoIsPayingView.swift
🔇 Additional comments (6)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Who Is paying/GroupWhoIsPayingViewModel.swift (2)
31-34: LGTM! Method rename improves clarity
The rename from fetchInitialMembersData to fetchInitialViewData better reflects the broader scope of data being fetched, and the implementation follows Swift concurrency best practices.
38-39: LGTM! Method name accurately reflects the implementation
The rename to fetchGroupWithMembers better describes the sequential process of fetching both group and member data.
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (2)
32-32: LGTM! Method name better reflects its responsibility
The rename from fetchInitialMembersData() to fetchInitialViewData() better represents that we're fetching both group and member data.
35-39: LGTM! Clean async implementation
The method follows Swift's async/await best practices.
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (2)
53-63: Previous error handling feedback still applies.
While the sequential fetching logic is optimized, the previous review comment about enhancing error handling and validation remains relevant.
53-63: Good optimization of data fetching.
The implementation efficiently fetches group data first and only proceeds to fetch member data if the group exists, which aligns well with the PR's objective of reducing API calls.
Summary by CodeRabbit
Release Notes
New Features
onGroupWriteCloud Function for automatic updates to user balances when group balances change.Improvements
Bug Fixes
Chores