Add fixes #82
Add fixes #82
Conversation
WalkthroughThis pull request introduces several enhancements across the Splito application, focusing on user management, expense tracking, and UI improvements. The changes span multiple files, including user repositories, view models, and localization strings. Key modifications include making user login types mutable, adding new methods for user retrieval by email, improving focus management in input fields, and refining text input behaviors. Additionally, structural changes in the project organization accommodate new functionalities related to expense management. The updates aim to enhance user experience, provide more flexible user authentication, and streamline expense management functionalities. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
Splito/UI/Login/EmailLogin/EmailLoginView.swift (1)
144-144: LGTM! Consistent with email field changes.The capitalization setting is appropriate for password fields.
Consider using
SecureFieldinstead of manually managing password masking with TextField. This would provide better security and reduce complexity. Example:ZStack(alignment: .trailing) { if isSecured { SecureField("Enter your password", text: $password) } else { TextField("Enter your password", text: $password) } // ... rest of the view remains same }Splito/UI/Home/Expense/AddExpenseView.swift (1)
50-52: Consider conditional focus for new vs. existing expenses.While automatically focusing the expense name field improves UX for new expenses, it might be disruptive when editing existing ones. Consider conditionally setting the focus only for new expenses.
.onAppear { - focusedField = .expenseName + if viewModel.expenseId == nil { + focusedField = .expenseName + } }Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift (2)
37-41: LGTM! Clean implementation of user ID validation.The guard statement effectively prevents potential nil crashes and follows Swift best practices for optional handling.
Consider adding analytics tracking for the missing user ID case to monitor its occurrence in production.
37-40: Consider adding a distinct error state for missing user ID.Currently, both missing user ID and empty groups list result in
.noGroupsstate. Consider introducing a separate state like.missingUserDatato help users better understand and troubleshoot the issue.Example implementation:
enum ViewState: Equatable { case loading case hasGroups(groups: [Groups]) case noGroups + case missingUserData case noInternet case somethingWentWrong public var key: String { switch self { case .loading: return "loading" case .hasGroups: return "hasGroups" case .noGroups: return "noGroups" + case .missingUserData: + return "missingUserData" case .noInternet: return "noInternet" case .somethingWentWrong: return "somethingWentWrong" } } } private func fetchGroups() async { guard let userId = preference.user?.id else { - currentViewState = .noGroups + currentViewState = .missingUserData return } // ... rest of the implementation }Also applies to: 43-43
Splito/UI/Home/Expense/Note/AddNoteView.swift (1)
Line range hint
1-124: Consider adding input validation and character limitsWhile the current implementation is solid, consider these improvements for better user experience and data integrity:
- Add character limit validation for both reason and note fields
- Add input sanitization to prevent empty or whitespace-only submissions
Example implementation:
private struct NoteInputFieldView: View { @Binding var text: String + let maxLength: Int // ... existing properties ... var body: some View { VStack(alignment: .leading, spacing: 8) { // ... existing view code ... TextField(placeholder.localized, text: $text, axis: axis) // ... existing modifiers ... + .onChange(of: text) { newValue in + if newValue.count > maxLength { + text = String(newValue.prefix(maxLength)) + } + } + if !text.isEmpty { + Text("\(text.count)/\(maxLength)") + .font(.caption) + .foregroundStyle(disableText) + } } } }Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift (1)
149-152: Consider adding unit tests for user ID handlingWith the addition of safe user ID unwrapping, it's important to ensure the initialization and state management flows are properly tested.
Would you like me to help create unit tests covering:
- Initialization with nil user ID
- Member selection with invalid user ID
- State transitions with user ID changes
Splito/UI/Home/Groups/GroupListViewModel.swift (1)
382-383: LGTM! Consider adding error feedback.The guard statement for user ID is a good addition that prevents potential nil usage. However, consider providing user feedback when the guard condition fails.
-guard let userId = preference.user?.id else { return } +guard let userId = preference.user?.id else { + LogE("GroupListViewModel: \(#function) Failed to process group: User ID not found.") + showToastFor(toast: .init(type: .error, title: "Error", message: "Unable to process group information.")) + return +}Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
323-328: Consider refactoring validation logic for better maintainability.The validation logic is complex and could benefit from being broken down into smaller, focused methods. Also, similar to the previous comment, the nil handling could be improved.
Consider refactoring like this:
-if let userId = preference.user?.id, selectedPayers == [:] || selectedPayers[userId] == 0 || - (selectedPayers.count == 1 && selectedPayers[userId] != expenseAmount) { - selectedPayers = [userId: expenseAmount] -} +private func validateAndUpdatePayers(userId: String) { + let currentAmount = selectedPayers[userId] ?? 0 + if selectedPayers.isEmpty || currentAmount == 0 || + (selectedPayers.count == 1 && currentAmount != expenseAmount) { + selectedPayers = [userId: expenseAmount] + } +} + +if let userId = preference.user?.id { + validateAndUpdatePayers(userId) +} -if expenseName == "" || expenseAmount == 0 || selectedGroup == nil || selectedPayers == [:] || selectedPayers.values.reduce(0, +) != expenseAmount { +private func validateExpenseData() -> Bool { + guard !expenseName.isEmpty, + expenseAmount > 0, + selectedGroup != nil, + !selectedPayers.isEmpty, + selectedPayers.values.reduce(0, +) == expenseAmount + else { return false } + return true +} + +if !validateExpenseData() {
Line range hint
1-478: Consider implementing a validation service for expense-related operations.Given the recurring patterns of user and expense validation throughout the view model, consider extracting these validations into a dedicated validation service. This would:
- Centralize validation logic
- Make the code more testable
- Reduce duplication
- Make it easier to maintain consistent validation rules
This could be implemented as:
protocol ExpenseValidationService { func validatePayers(userId: String, selectedPayers: [String: Double], expenseAmount: Double) -> Bool func validateExpenseData(expense: ExpenseData) -> Bool }Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
89-94: Consider using a more targeted approach for dismissing focus.While the current implementation works, using
onTapGestureon the entire view might interfere with other interactive elements. Consider using a dedicated overlay or background view for handling tap-to-dismiss behavior.- .onTapGesture { - isAmountFocused = false - } + overlay(alignment: .center) { + if isAmountFocused { + Color.black.opacity(0.001) // Nearly transparent + .onTapGesture { + isAmountFocused = false + } + } + }Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
46-68: Usedeferto ensureisLoginInProgressis reset properlyTo ensure
isLoginInProgressis set tofalsein all exit paths, including early returns and exceptions, consider using adeferstatement. This will help maintain consistent UI state and prevent potential loading indicator issues.Apply this diff to implement the change:
func onLoginClick() { guard validateEmailAndPassword() else { return } Task { + isLoginInProgress = true + defer { isLoginInProgress = false } - isLoginInProgress = true do { if let user = try await userRepository.fetchUserBy(email: email) { if user.loginType != .Email { - isLoginInProgress = false showAlertFor(title: "Email Already in Use", message: "The email address is already associated with an existing account. Please use a different email or log in to your existing account.") return } FirebaseAuth.Auth.auth().signIn(withEmail: email, password: password) { [weak self] result, error in - self?.isLoginInProgress = false self?.handleAuthResponse(result: result, error: error, isLogin: true) } } else { - isLoginInProgress = false showAlertFor(title: "Account Not Found", message: "No account found with the provided email address. Please sign up.") LogE("EmailLoginViewModel: \(#function) No user found with this email.") } } catch { - isLoginInProgress = false showAlertFor(title: "Error", message: "An error occurred during login. Please try again later.") LogE("EmailLoginViewModel: \(#function) Error fetching user: \(error).") } } }Data/Data/Repository/UserRepository.swift (1)
36-38: Consider adding documentation for the new method.The new email-based user lookup method is a good addition. Consider adding documentation to describe the method's behavior, especially for edge cases like non-existent users or invalid email formats.
+ /// Fetches a user by their email address + /// - Parameter email: The email address to search for + /// - Returns: The matching AppUser if found, nil otherwise + /// - Throws: Any errors from the underlying store public func fetchUserBy(email: String) async throws -> AppUser? { return try await store.fetchUserBy(email: email) }Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
214-216: Consider using string extension for empty checks.The empty string validation is good, but consider creating a reusable string extension for better maintainability.
+ extension String { + var isNotEmpty: Bool { + !self.isEmpty + } + } private var subInfo: String { - if let emailId = member.emailId, !emailId.isEmpty { + if let emailId = member.emailId, emailId.isNotEmpty { return emailId - } else if let phoneNumber = member.phoneNumber, !phoneNumber.isEmpty { + } else if let phoneNumber = member.phoneNumber, phoneNumber.isNotEmpty { return phoneNumber } else { return "No email address" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Data/Data/Model/AppUser.swift(1 hunks)Data/Data/Repository/UserRepository.swift(2 hunks)Data/Data/Store/UserStore.swift(1 hunks)Splito/Localization/Localizable.xcstrings(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(4 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(5 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/SelectGroupViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Expense/Note/AddNoteView.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(5 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift(1 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(1 hunks)Splito/UI/Login/EmailLogin/EmailLoginView.swift(2 hunks)Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift(1 hunks)Splito/UI/Login/LoginViewModel.swift(1 hunks)
🔇 Additional comments (20)
Splito/UI/Home/Groups/Create Group/CreateGroupView.swift (1)
166-166: LGTM! The sentence capitalization improves user experience.
Adding .textInputAutocapitalization(.sentences) is appropriate for group names as they are typically proper nouns. This change aligns with iOS text input best practices.
Splito/UI/Login/EmailLogin/EmailLoginView.swift (1)
98-98: LGTM! Good update to use the newer SwiftUI API.
The change from autocapitalization(.none) to textInputAutocapitalization(.never) follows the latest SwiftUI best practices.
Splito/Localization/Localizable.xcstrings (2)
628-630: LGTM! Improved string clarity.
The new message "Please select a group before choosing the split option." is more user-friendly and provides clearer guidance on the required sequence of actions.
628-631: 🛠️ Refactor suggestion
Consider removing the old string to maintain consistency.
Both the old string "Please select a group to get payer list." and the new string are present in the file. This could lead to inconsistent messaging if both strings are still being used in different parts of the app.
Let's verify the usage of both strings in the codebase:
If the old string is no longer used, consider removing it to maintain consistency and avoid confusion during future translations.
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerView.swift (1)
47-47: LGTM! The change aligns with best practices for data handling.
The modification to pass only the user ID instead of the entire user object is a good improvement as it:
- Reduces coupling between the view and view model
- Follows the principle of passing minimal required data
- Aligns with the broader changes in the codebase for consistent user ID handling
Let's verify the corresponding view model changes:
✅ Verification successful
✅ Method signature change is correctly implemented and consistent
The verification confirms that:
- The view model (
ChoosePayerViewModel.swift) has the matching method signaturefunc handlePayerSelection(userId: String) - The implementation correctly uses the userId parameter to update the selectedPayers state
- No instances of the old signature (
handlePayerSelection(user: AppUser)) were found - The change is properly integrated with the parent view (
AddExpenseView) and its view model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the view model's method signature matches this change
# Search for handlePayerSelection method in ChoosePayerViewModel
ast-grep --pattern 'func handlePayerSelection(userId: String)'
# Also check for any remaining instances of the old signature
ast-grep --pattern 'func handlePayerSelection(user: AppUser)'
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search to find the view model file and its implementation
# Find the view model file
fd "ChoosePayerViewModel" --type f
# Search for any handlePayerSelection method with ripgrep for more flexible matching
rg "handlePayerSelection" -A 3
Length of output: 1663
Splito/UI/Home/Expense/AddExpenseView.swift (1)
178-178: LGTM! Good UX improvement.
Adding sentence-case autocapitalization for expense descriptions aligns with iOS text input conventions and improves the user experience.
Splito/UI/Home/Expense/Detail Selection/SelectGroupViewModel.swift (1)
37-43: Verify consistent user ID handling across view models.
The AI summary mentions similar changes in AddExpenseViewModel and ExpenseSplitOptionsViewModel. Let's verify the consistency of this pattern.
✅ Verification successful
User ID validation pattern is consistently implemented across view models
The verification shows that all view models consistently use the guard let userId = preference.user?.id pattern for user ID validation. The pattern is properly implemented across various features:
- Group management (Create, List, Settings)
- Expense handling (Add, Detail, Split options)
- Activity logging
- Transaction management
- Payment processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other view models follow the same user ID validation pattern
# Expected: Similar guard statements for user ID validation
# Search for user ID validation patterns in view models
rg -t swift "guard\s+let\s+userId\s*=\s*preference\.user\?.id" --glob '*ViewModel.swift'
# Search for direct access to preference.user?.id to find potential missed cases
rg -t swift "preference\.user\?.id" --glob '*ViewModel.swift'
Length of output: 8692
Splito/UI/Home/Expense/Note/AddNoteView.swift (1)
98-98: LGTM: Improved text input behavior
The switch to .textInputAutocapitalization(.sentences) provides a better user experience by enabling natural sentence capitalization, which is more appropriate than completely disabled autocorrection.
Let's verify this change is consistent across other text input views:
✅ Verification successful
Text input behavior is consistently implemented across views
The verification confirms that .textInputAutocapitalization(.sentences) is uniformly applied across relevant text input views:
- CreateGroupView.swift
- AddExpenseView.swift
- AddNoteView.swift
This consistent implementation ensures a cohesive user experience throughout the app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent text input behavior across views
# Expect: Similar textInputAutocapitalization usage in other view files
rg "textInputAutocapitalization\(.sentences\)" --type swift
Length of output: 387
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift (1)
149-152: Verify consistent user ID handling across split types
The safe user ID unwrapping aligns with the PR's focus on improving user ID handling. Let's verify this pattern is consistently applied across related components.
✅ Verification successful
User ID handling is consistently implemented across the codebase
The codebase demonstrates a consistent pattern of safely unwrapping user IDs using optional chaining and guard statements. The pattern in ExpenseSplitOptionsViewModel aligns with the broader codebase's approach where:
- Critical operations properly guard against nil user IDs
- UI components handle user identification safely for display purposes
- Repository layer provides fallback empty strings where appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential unsafe user ID access patterns in related view models
ast-grep --pattern 'preference.user?.id'
rg -g '*.swift' 'preference\.user.*id'
Length of output: 17899
Splito/UI/Home/Groups/GroupListViewModel.swift (1)
382-383: Standardize user ID validation across the view model.
For consistency, consider implementing similar error handling patterns across all methods that retrieve the user ID. This includes fetchGroups(), fetchMoreGroups(), fetchLatestUser(), and fetchCurrentUser().
Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
278-278: LGTM! Improved error message clarity.
The updated error message is more descriptive and user-friendly, providing clearer guidance to users.
Line range hint 75-85: LGTM! Improved user validation and simplified payer selection.
The changes improve the code by adding early user validation and simplifying the payer selection logic. The removal of fetchDefaultUser in favor of direct assignment reduces complexity.
Let's verify that the removed fetchDefaultUser method isn't referenced elsewhere:
✅ Verification successful
The removal of fetchDefaultUser is safe and improves the code
The comprehensive search confirms that fetchDefaultUser is not referenced anywhere in the codebase. The simplified approach using direct user ID validation and payer selection is better aligned with the existing user fetching patterns in the codebase, which primarily use methods like fetchUserData, fetchMemberData, and fetchUserBy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to fetchDefaultUser
rg "fetchDefaultUser"
Length of output: 21
Script:
#!/bin/bash
# Let's try a more comprehensive search for any references to fetchDefaultUser
# First, let's check if there are any git changes that removed this method
git log -p | rg -A 5 -B 5 "fetchDefaultUser"
# Also, let's check for any similar method names in case it was renamed
rg -i "fetch.*user"
# Let's also check the specific view model implementation for any remnants
ast-grep --pattern 'func fetchDefaultUser() { $$$ }'
Length of output: 10560
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (3)
17-17: LGTM! Good focus state management practice.
Moving the focus state to the parent view allows for better coordination of focus across child views.
64-64: LGTM! Proper binding propagation.
The focus state binding is correctly passed down to the AmountRowView component.
131-131: LGTM! Consider additional currency formatting edge cases.
The focus binding implementation looks good. However, the amount formatting logic could be enhanced to handle:
- Multiple decimal points
- Negative numbers
- Currency symbol placement for different locales
Let's check if there are any currency-related issues reported:
Also applies to: 149-149
Data/Data/Store/UserStore.swift (1)
48-59: Function fetchUserBy(email:) implemented correctly
The fetchUserBy(email:) method is correctly implemented, allowing retrieval of a user by email. Error handling is appropriately managed for cases where the user is found or not found.
Splito/UI/Home/Expense/Detail Selection/Payer/ChoosePayerViewModel.swift (1)
59-60: Ensure all calls to handlePayerSelection are updated
The method handlePayerSelection has changed its parameter from user: AppUser to userId: String. Please verify that all calls to this method have been updated to match the new signature to prevent runtime errors.
Run the following script to verify method usage:
✅ Verification successful
No issues found with handlePayerSelection usage
The codebase search reveals that there are two distinct handlePayerSelection methods with different signatures:
-
In
ChoosePayerViewModel.swift:handlePayerSelection(userId: String)- Called correctly in
ChoosePayerView.swiftwithuser.id
- Called correctly in
-
In
AddExpenseViewModel.swift:handlePayerSelection(payers: [String: Double])- Called correctly in
AddExpenseView.swiftwith dictionary parameter
- Called correctly in
These are separate methods in different view models, and all calls match their respective method signatures correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `handlePayerSelection` and check if they pass `userId` as a String.
# Search for method calls to `handlePayerSelection` and display two lines of context
rg -A 2 'handlePayerSelection\('
Length of output: 1308
Data/Data/Repository/UserRepository.swift (1)
19-23: LGTM! Verify login type synchronization across the app.
The logic for handling user state and login type changes is sound. The condition properly handles both inactive users and login type changes.
Let's verify the login type handling across the codebase:
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax issues with Swift. Let's try a direct search to understand the login type handling across the codebase.
The search revealed several usages of loginType, but we should verify the enum definition and any switch cases to ensure the synchronization logic is complete.
Login type synchronization verification completed - Implementation is correct
The codebase shows consistent handling of login types:
- LoginType is properly defined as an enum with three cases: Apple, Google, and Email
- Login type updates are handled correctly in UserRepository with proper state synchronization
- Login type checks are implemented in:
- EmailLoginViewModel (prevents email login for accounts with different login types)
- UserProfileViewModel (handles login type-specific flows)
- LoginViewModel (sets appropriate login type during authentication)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential login type handling inconsistencies
# Look for places where loginType is accessed or modified
ast-grep --pattern 'loginType = $_'
Length of output: 37
Script:
#!/bin/bash
# Let's try a more comprehensive search for loginType usage
# First, let's find direct references to loginType property
rg "loginType" -A 2 -B 2
# Also search for specific patterns in Swift files
rg "\.loginType" --type swift -A 2 -B 2
Length of output: 9134
Script:
#!/bin/bash
# Let's search for the LoginType enum definition to understand all possible values
rg "enum.*LoginType" -A 5 -B 1
# Also search for any login type validation or checks we might have missed
rg "case.*LoginType" -A 2 -B 2
Length of output: 366
Splito/UI/Login/LoginViewModel.swift (2)
35-39: LGTM! Proper loading state management.
Good addition of loading state management with proper cleanup on early returns.
58-61: LGTM! Proper error handling with loading state cleanup.
Good handling of error cases with appropriate loading state cleanup when user data or ID token is missing.
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsViewModel.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
59-59: Remove trailing whitespaceThere is trailing whitespace on line 59.
- +🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 59-59: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Data/Data/Model/AppUser.swift(2 hunks)Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Model/AppUser.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift
[Warning] 59-59: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (3)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (3)
56-58: Notify user when account doesn't exist
When no user is found, the error is only logged but not communicated to the user.
Consider showing an alert to inform the user that no account exists with this email.
64-67: Improve error handling for user fetch failures
The catch block only logs the error without informing the user.
Consider displaying an alert to inform the user about the login failure.
77-77: Validate user profile completeness
Creating a user with empty first and last names might cause issues in the UI or other parts of the app that expect these fields to be populated.
Consider either:
- Making these fields optional in the
AppUsermodel, or - Adding a profile completion step after signup
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (2)
60-63: 🛠️ Refactor suggestionConvert Firebase Auth to async/await for consistent error handling
The Firebase Auth call uses completion handlers while the rest of the method uses async/await, creating inconsistent error handling patterns.
- FirebaseAuth.Auth.auth().signIn(withEmail: email, password: password) { [weak self] result, error in - self?.isLoginInProgress = false - self?.handleAuthResponse(result: result, error: error, isLogin: true) - } + do { + let result = try await FirebaseAuth.Auth.auth().signIn(withEmail: email, password: password) + isLoginInProgress = false + handleAuthResponse(result: result, error: nil, isLogin: true) + } catch let error { + isLoginInProgress = false + handleAuthResponse(result: nil, error: error, isLogin: true) + }
50-58:⚠️ Potential issueImprove user feedback for login failures
When no user is found with the provided email, only a log message is generated. The user should be informed about the issue.
} else { LogE("EmailLoginViewModel: \(#function) No user found with this email.") + isLoginInProgress = false + showAlertFor(title: "Account Not Found", + message: "No account found with this email. Please sign up first.") + return }
🧹 Nitpick comments (2)
Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsViewModel.swift (1)
149-152: LGTM! Consider adding error logging.The guard statement for user ID improves null safety, and the selection logic correctly ensures the current user remains selected when deselecting all members.
Consider adding error logging when the user ID is nil:
-guard let userId = preference.user?.id else { return } +guard let userId = preference.user?.id else { + LogE("ExpenseSplitOptionsViewModel: \(#function) User ID is nil") + return +}Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
59-59: Remove trailing whitespaceLine 59 contains trailing whitespace.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 59-59: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Splito.xcodeproj/project.pbxproj(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(5 hunks)Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift
[Warning] 59-59: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (2)
Splito.xcodeproj/project.pbxproj (1)
527-527: LGTM! Good structural organization.
The addition of the "Expense Split Option" group improves project organization by grouping related files together.
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
78-78: Verify LoginType enum case usage
Ensure that .Email is a valid case in the LoginType enum.
✅ Verification successful
.Email case is correctly used in the code
The LoginType enum is defined in Data/Data/Model/AppUser.swift and includes the .Email case, confirming its valid usage in EmailLoginViewModel.swift.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LoginType enum definition
rg -A 5 "enum LoginType"
Length of output: 296
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
46-68: 🛠️ Refactor suggestionInconsistent async/await usage in login flow
The login flow mixes async/await with completion handlers, which creates inconsistent error handling patterns. The Firebase Auth call should be converted to use async/await for consistency.
- FirebaseAuth.Auth.auth().signIn(withEmail: email, password: password) { [weak self] result, error in - self?.isLoginInProgress = false - self?.handleAuthResponse(result: result, error: error, isLogin: true) - } + do { + let result = try await FirebaseAuth.Auth.auth().signIn(withEmail: email, password: password) + isLoginInProgress = false + handleAuthResponse(result: result, error: nil, isLogin: true) + } catch let error { + isLoginInProgress = false + handleAuthResponse(result: nil, error: error, isLogin: true) + }
🧹 Nitpick comments (3)
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (1)
100-100: Approve grammar fix but suggest localization improvementThe grammatical improvement for handling "You are" vs "[Name] is" is good. However, this string should be localized to support proper translations across different languages where the verb conjugation rules might differ.
Consider extracting this to a localized string format:
- Text(" \(name == "You" ? "are" : "is") settled up") + Text(String(format: NSLocalizedString(" %@ settled up", comment: "Shows if a member is settled up. Use appropriate verb form based on the placeholder being 'are' or 'is'"), name == "You" ? "are" : "is"))Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (2)
78-78: Fix indentationRemove extra spaces in the indentation to maintain consistency with the codebase style.
- emailId: email, loginType: .Email) + emailId: email, loginType: .Email)
46-68: Consider implementing rate limiting for login attemptsTo enhance security and prevent brute force attacks, consider implementing client-side rate limiting for login attempts. This would complement Firebase's server-side protection.
Example approach:
- Track failed login attempts with timestamps
- Implement exponential backoff
- Lock login functionality temporarily after X failed attempts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Data/Data/Store/UserStore.swift(1 hunks)Splito/Localization/Localizable.xcstrings(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift(1 hunks)Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Data/Data/Store/UserStore.swift
- Splito/Localization/Localizable.xcstrings
🔇 Additional comments (1)
Splito/UI/Login/EmailLogin/EmailLoginViewModel.swift (1)
74-74: LGTM! Improved error logging
The error message now provides better context by specifying whether the error occurred during login or signup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (1)
88-94: LGTM! Consider adding loading state handlingThe simplified loading indicator implementation is cleaner and more maintainable. Moving it outside the ForEach loop improves code readability and ensures consistent visibility.
Consider handling the loading state in the view model to prevent potential duplicate loading requests:
if viewModel.hasMoreExpenses { ProgressView() .frame(maxWidth: .infinity, alignment: .center) - .onAppear(perform: viewModel.loadMoreExpenses) + .onAppear { + guard !viewModel.isLoadingMoreExpenses else { return } + viewModel.loadMoreExpenses() + } .padding(.vertical, 8) }This would require adding an
@Published var isLoadingMoreExpenses: Bool = falseto your view model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(1 hunks)
🔇 Additional comments (1)
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (1)
88-94: Verify infinite scroll edge cases
While the loading indicator implementation looks good, please verify these edge cases:
- Rapid scrolling behavior
- Network error handling during load more
- Empty response handling when no more items exist
✅ Verification successful
Based on the search results, I can see comprehensive error handling in the codebase for the infinite scroll implementation:
-
In
GroupHomeViewModel.swift, theloadMoreExpenses()andfetchMoreExpenses()methods handle:- Network errors with proper error logging and user feedback via
showToastForError() - Empty response handling by updating
hasMoreExpensesbased on response size - Proper state management with
lastDocumenttracking
- Network errors with proper error logging and user feedback via
-
The view implementation in
GroupExpenseListView.swiftshows:- Conditional rendering of loading indicator only when
hasMoreExpensesis true - Proper empty state handling with
ExpenseNotFoundView - Clean separation of loading state from content
- Conditional rendering of loading indicator only when
-
The implementation includes:
- Debounced loading through SwiftUI's
onAppear - Proper pagination with
EXPENSES_LIMIT - Deduplication of expenses with
uniqued()
- Debounced loading through SwiftUI's
The infinite scroll implementation has robust error handling and edge case management
- Network errors are properly caught and displayed to users
- Empty states are handled gracefully
- Duplicate prevention is in place
- Pagination is implemented correctly with proper state tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in loadMoreExpenses implementation
rg -A 5 "loadMoreExpenses|hasMoreExpenses" --type swift
# Look for network error handling patterns
rg -A 5 "catch|error|failure" --type swift
Length of output: 81935
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (1)
Line range hint
281-305: Fix potential memory leak in completion handler.While the error handling is comprehensive, there's a potential memory leak in the completion handler. The
selfreference should be marked asweakconsistently throughout the closure to prevent retain cycles.-GIDSignIn.sharedInstance.signIn(withPresenting: controller) { result, error in +GIDSignIn.sharedInstance.signIn(withPresenting: controller) { [weak self] result, error in guard error == nil else { - self.isDeleteInProgress = false - LogE("UserProfileViewModel: \(#function) Google Login Error: \(String(describing: error)).") + self?.isDeleteInProgress = false + LogE("UserProfileViewModel: \(#function) Google Login Error: \(String(describing: error)).") return } guard let user = result?.user, let idToken = user.idToken?.tokenString else { - self.isDeleteInProgress = false + self?.isDeleteInProgress = false return } let credential = GoogleAuthProvider.credential(withIDToken: idToken, accessToken: user.accessToken.tokenString) completion(credential) }
🧹 Nitpick comments (3)
Splito/UI/Login/SignInWithAppleDelegate.swift (2)
14-19: LGTM! Consider adding documentation.The error handling implementation looks good. The closure property and initializer are properly defined.
Consider adding documentation comments to describe when the
onErrorclosure is called:+/// Closure called when any error occurs during the Apple Sign-In process private let onError: (() -> Void) +/// Initializes the delegate with success and error handling closures +/// - Parameters: +/// - signInSucceeded: Called when sign-in is successful with user details +/// - onError: Called when any error occurs during the sign-in process init(signInSucceeded: @escaping (String, String, String, String) -> Void, onError: @escaping () -> Void)
27-29: LGTM! Consider passing error details to the callback.The error handling is properly implemented, but consider providing more context to the error handler.
Consider passing error details to help with user feedback:
-private let onError: (() -> Void) +private let onError: ((SignInError) -> Void) -onError() +onError(.identityTokenMissing) +enum SignInError { + case identityTokenMissing + case tokenSerializationFailed + case authorizationFailed(Error) +}Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (1)
268-272: LGTM! Consider adding error logging.The Apple authentication credential creation and error handling look correct. However, consider adding error logging in the error callback for better debugging capabilities.
} onError: { self.isDeleteInProgress = false + LogE("UserProfileViewModel: \(#function) Apple Login Error occurred during re-authentication.") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(3 hunks)Splito/UI/Login/LoginViewModel.swift(2 hunks)Splito/UI/Login/SignInWithAppleDelegate.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Login/LoginViewModel.swift
🔇 Additional comments (3)
Splito/UI/Login/SignInWithAppleDelegate.swift (2)
33-35: LGTM! Error handling is properly implemented.
The error handling for token serialization failure is well implemented with appropriate logging.
47-49: LGTM! Verify error handling in the view model.
The error callback implementation is correct. Since this is a delegate callback, ensure that the view model properly handles and displays these errors to the user.
Let's verify the error handling implementation in the view model:
✅ Verification successful
Let me generate one more verification script to check how the SignInWithAppleDelegate is connected to the view model.
Let me fix the regex syntax and try one more time.
Error handling in LoginViewModel is properly implemented
The view model correctly handles errors from the Apple sign-in process:
- Shows loading states appropriately with
showAppleLoading - Displays user-friendly error messages through alerts
- Logs errors for debugging
- Properly initializes and connects the
SignInWithAppleDelegates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how errors are handled in the view model and UI
# Look for error handling in view models
rg -t swift "onError.*SignInWithAppleDelegate" --context 3
# Look for error alert presentations
rg -t swift "alert|error.*message" -g "*LoginViewModel.swift" --context 3
Length of output: 3232
Script:
#!/bin/bash
# Check how SignInWithAppleDelegate is initialized and connected
rg -t swift "SignInWithAppleDelegate.*init" --context 3
# Check for onError closure setup
rg -t swift "onError.*=.*{" -g "*LoginViewModel.swift" --context 3
Length of output: 216
Script:
#!/bin/bash
# Check how SignInWithAppleDelegate is initialized and connected
rg -t swift "SignInWithAppleDelegate.*init" --context 3
# Check for onError closure setup with fixed regex
rg -t swift "onError\s*=\s*\{" -g "*LoginViewModel.swift" --context 3
# Also check for SignInWithAppleDelegate usage in LoginViewModel
rg -t swift "SignInWithAppleDelegate" -g "*LoginViewModel.swift" --context 3
Length of output: 1470
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (1)
Line range hint 268-305: Consider adding timeout handling for authentication operations.
The re-authentication flow is well-implemented, but it lacks timeout handling. If the authentication providers are unreachable, the user might be stuck in a loading state indefinitely. Consider implementing a timeout mechanism to provide a better user experience.
Let's verify if there are any existing timeout implementations in the codebase:
Suggested approach:
- Add a timeout configuration
- Implement a timeout handler that resets the loading state
- Show an appropriate error message to the user
Example implementation:
private func handleAuthenticationTimeout() {
DispatchQueue.main.asyncAfter(deadline: .now() + 30) { [weak self] in
guard let self = self, self.isDeleteInProgress else { return }
self.isDeleteInProgress = false
self.showAlertFor(message: "Authentication timed out. Please try again.")
}
}✅ Verification successful
Based on the search results, I can now provide a final response:
Consider implementing a timeout mechanism using the existing loading state management
The codebase already has a robust loading state management system with isDeleteInProgress flag and proper error handling. However, the authentication operations could still benefit from a timeout mechanism. You can leverage the existing infrastructure by:
- Using the
isDeleteInProgressflag which is already properly set and cleared throughout the authentication flow - Adding a timeout using
DispatchQueue.main.asyncAfterin the authentication methods - Utilizing the existing error handling and alert system that's already in place
Example implementation to add in handleGoogleLogin, handleAppleLogin, and handleEmailLogin:
private func startAuthenticationTimeout() {
DispatchQueue.main.asyncAfter(deadline: .now() + 30) { [weak self] in
guard let self = self, self.isDeleteInProgress else { return }
self.isDeleteInProgress = false
self.showAlertFor(message: "Authentication timed out. Please try again.")
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timeout implementations in authentication flows
rg -i "timeout|timeoutInterval" --type swift
Length of output: 46
Script:
#!/bin/bash
# Let's search for any existing error handling or alert mechanisms in the authentication flow
rg -i "error.*auth|auth.*error|alert.*auth|auth.*alert" --type swift -A 2 -B 2
Length of output: 8563
Script:
#!/bin/bash
# Search for any existing loading state handling or cancellation mechanisms
rg -i "isdeleteInProgress|loading.*state|cancel.*auth" --type swift -A 2 -B 2
Length of output: 23230
Script:
#!/bin/bash
# Look for any existing async operation handling patterns
ast-grep --pattern 'DispatchQueue.main.asyncAfter(deadline: $_, body: $_)'
Length of output: 76
Summary by CodeRabbit
New Features
AddExpenseViewandGroupPaymentViewwith improved focus management for input fields.Bug Fixes
Documentation
Style