Add multi currency support #98
Conversation
WalkthroughThis pull request introduces several modifications to the Splito application, primarily focusing on enhancing currency management. New files and structures have been added, including a Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
Data/Data/Model/Groups.swift (1)
Add currency code validation against supported currencies
The codebase includes a comprehensive
Currencies.jsonfile with supported currencies, but there's no validation to ensure only valid currency codes are used. Consider:
- Creating a
CurrencyCodetype/enum using the codes fromCurrencies.json- Adding validation to ensure only supported currency codes are accepted
- Using the type-safe currency code instead of optional String
🔗 Analysis chain
Line range hint
13-24: Validate currency codes against supported currenciesWhile adding multi-currency support, it's important to validate currency codes. Consider adding validation to ensure only supported currency codes from
Currencies.jsonare accepted.Let's verify the currency codes validation in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if currency validation exists rg -l "Currency.*valid|valid.*Currency" # Check the contents of Currencies.json fd -g "Currencies.json" --exec cat {}Length of output: 44191
🧹 Nitpick comments (5)
Data/Data/Model/Groups.swift (1)
Line range hint
1-165: Consider these architectural points for multi-currency supportWhile the basic structure for multi-currency support is being added, consider these important aspects:
- Currency conversion handling for mixed-currency expenses
- Decimal precision for monetary calculations
- Currency formatting for display
- Default currency inheritance (group → expense → split)
- Historical exchange rate handling for past transactions
Consider:
- Using a decimal type instead of Double for monetary values to avoid floating-point precision issues
- Adding currency conversion service integration points
- Implementing currency formatting utilities
- Documenting the currency handling strategy
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (2)
106-107: Document the currency code parameterSince this is a significant change adding multi-currency support, consider adding documentation about the currency code parameter and its implications.
Add a documentation comment:
+ // Initialize group with local currency code for expense tracking + // Currency code follows ISO 4217 standard (e.g., USD, EUR, GBP) let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, members: [userId], balances: [memberBalance], currencyCode: localCurrency)
104-107: Consider broader implications of multi-currency supportWhile the currency code implementation looks good for group creation, consider these architectural points:
- Ensure the UI layer can handle currency symbol display
- Plan for currency conversion functionality in transactions
- Consider adding currency formatting utilities for consistent display
- Think about handling historical exchange rates for past transactions
Would you like me to provide more detailed guidance on implementing any of these aspects?
Data/Data/Model/Currency/Currency.swift (1)
16-19: Consider caching the loaded currencies.The
getCurrencies()method reads from JSON file on every call. Consider caching the results in a static property to improve performance.public struct Currency: Codable { + private static var cachedCurrencies: [Currency]? private static func getCurrencies() -> [Currency] { + if let cached = cachedCurrencies { + return cached + } let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .baseBundle) ?? [] + cachedCurrencies = allCurrencies return allCurrencies }Data/Data/Model/Transaction.swift (1)
27-29: Improve parameter organization in initializer.The initializer parameters could be better organized by grouping related fields together.
- public init(payerId: String, receiverId: String, date: Timestamp, addedBy: String, amount: Double, - currencyCode: String? = "INR", updatedBy: String? = nil, note: String? = nil, reason: String? = nil, - imageUrl: String? = nil, updatedAt: Timestamp? = nil, isActive: Bool = true) { + public init( + // Required transaction details + amount: Double, + currency: Currency = Currency.defaultCurrency, + date: Timestamp, + + // User information + payerId: String, + receiverId: String, + addedBy: String, + updatedBy: String? = nil, + + // Optional details + note: String? = nil, + reason: String? = nil, + imageUrl: String? = nil, + + // Metadata + updatedAt: Timestamp? = nil, + isActive: Bool = true + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Data/Data.xcodeproj/project.pbxproj(5 hunks)Data/Data/Model/Currency/Currencies.json(1 hunks)Data/Data/Model/Currency/Currency.swift(1 hunks)Data/Data/Model/Groups.swift(3 hunks)Data/Data/Model/Transaction.swift(2 hunks)Splito.xcodeproj/project.pbxproj(4 hunks)Splito/Resource/Currencies.json(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- Splito/Resource/Currencies.json
- Data/Data/Model/Currency/Currencies.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (3)
104-104:⚠️ Potential issueAdd import statement for Currency module
The code uses
Currency.getCurrentLocalCurrency()but the Currency module is not imported.Add this import at the top of the file:
import FirebaseFirestore +import CurrencyLikely invalid or redundant comment.
104-107: Verify currency handling in group updatesThe
updateGroupmethod doesn't handle currency code updates. This might be intentional, but we should verify if currency code should be immutable after group creation.✅ Verification successful
Currency code immutability is intentional
The currency code is intentionally immutable after group creation. This is a common design pattern in expense-splitting applications to maintain consistency in balances and historical transactions. Changing currency mid-way would require complex balance recalculations and could lead to data inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if currency code is modified in any update operations rg -A 5 "currencyCode.*="Length of output: 4888
Script:
#!/bin/bash # Search for group update methods and any currency code modifications ast-grep --pattern 'func update$$$Group' # Also search for any direct modifications to group currency rg -B 3 -A 3 "group.*currencyCode.*="Length of output: 910
Script:
#!/bin/bash # Search for any group update methods with broader patterns rg -B 2 -A 4 "func.*[Uu]pdate.*[Gg]roup|func.*[Gg]roup.*[Uu]pdate" # Search for Firebase/database update operations on groups rg -B 2 -A 4 "updateData.*group|set.*group|modify.*group"Length of output: 40019
106-107: Consider validating the currency codeThe currency code is directly used in group creation without validation. Consider verifying that it exists in your
Currencies.jsonlist.- let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, - members: [userId], balances: [memberBalance], currencyCode: localCurrency) + guard let validatedCurrency = Currency.validate(currencyCode: localCurrency) else { + throw GroupError.invalidCurrency + } + let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, + members: [userId], balances: [memberBalance], currencyCode: validatedCurrency)✅ Verification successful
The currency code usage in this context is already validated
The
localCurrencyused here comes fromCurrency.getCurrentLocalCurrency(), which ensures a valid currency by checking against the predefined list in Currencies.json with a fallback to INR. However, for better code safety across the application, consider adding a validation method to the Currency struct:extension Currency { static func isValid(code: String) -> Bool { return getCurrencies().contains(where: { $0.code == code }) } }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Currency validation exists ast-grep --pattern 'func isValid(currencyCode: String)'Length of output: 57
Script:
#!/bin/bash # Find currency-related files fd -i currency # Look for Currency.json specifically fd -i currencies.json # Search for currency validation patterns rg -i "currency.*valid" -A 3 ast-grep --pattern 'class Currency { $$$ }' # Search for currency code handling rg -i "currencycode" -A 3Length of output: 4924
Script:
#!/bin/bash # Check Currency model implementation cat "Data/Data/Model/Currency/Currency.swift" # Check Currencies.json content structure head -n 10 "Data/Data/Model/Currency/Currencies.json" # Look for any validation methods with different naming rg -i "func.*currency" -A 3Length of output: 2454
Data/Data/Model/Currency/Currency.swift (1)
10-15: LGTM! Well-structured currency model.The
Currencystruct is well-designed with appropriate properties andCodableconformance.Data/Data.xcodeproj/project.pbxproj (1)
31-32: LGTM! Project configuration is correct.The new Currency files are properly integrated into the project structure with appropriate build phases.
Also applies to: 98-99, 200-208, 461-461, 557-557
Splito.xcodeproj/project.pbxproj (1)
68-68: LGTM! Currency configuration file is properly integrated.The
Currencies.jsonfile has been correctly added to the project with proper build phases and resource bundling configuration.Let's verify the currency file's presence and structure:
Also applies to: 230-230, 910-910
✅ Verification successful
Currency configuration is properly integrated and well-structured
The currency file contains a comprehensive list of world currencies with proper formatting and complete data (code, name, symbol, region) for each entry. The file is correctly referenced in the Xcode project for bundling with the app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the currency file exists and contains valid JSON data # Expected: File exists and contains currency data in valid JSON format # Check if file exists and show its location fd -t f "Currencies.json" # Validate JSON structure and count currencies if [ -f "Splito/Resource/Currencies.json" ]; then echo "Validating JSON structure..." jq -r '. | length' "Splito/Resource/Currencies.json" fiLength of output: 265
Script:
#!/bin/bash # Compare the two currency files and show their content echo "=== Comparing currency files ===" if cmp -s "Data/Data/Model/Currency/Currencies.json" "Splito/Resource/Currencies.json"; then echo "Files are identical" else echo "Files are different" fi echo -e "\n=== Content of Data/Data/Model/Currency/Currencies.json ===" cat "Data/Data/Model/Currency/Currencies.json" echo -e "\n=== Content of Splito/Resource/Currencies.json ===" cat "Splito/Resource/Currencies.json"Length of output: 44746
Data/Data/Model/Groups.swift
Outdated
| // public struct GroupMemberBalance: Codable { | ||
| // public let id: String /// Member Id | ||
| // public var balanceByCurrency: [String: GroupCurrencyBalance] /// Currency wise member balance | ||
| // } | ||
| // public struct GroupCurrencyBalance: Codable { | ||
| // public var balance: Double | ||
| // public var totalSummary: [GroupTotalSummary] | ||
| // } |
There was a problem hiding this comment.
Remove commented code and complete the multi-currency migration
The commented-out structures suggest an incomplete migration to multi-currency support. The new GroupCurrencyBalance structure looks promising, but:
- The old
GroupMemberBalanceis still active - The new structures are commented out
- No migration plan is visible
This could lead to confusion and maintenance issues.
Consider:
- Removing the commented code
- Completing the migration to the new structure
- Adding migration logic for existing data
-// public struct GroupMemberBalance: Codable {
-// public let id: String /// Member Id
-// public var balanceByCurrency: [String: GroupCurrencyBalance] /// Currency wise member balance
-// }
-// public struct GroupCurrencyBalance: Codable {
-// public var balance: Double
-// public var totalSummary: [GroupTotalSummary]
-// }
Data/Data/Model/Groups.swift
Outdated
| public init(group: Groups, userBalance: Double, memberOweAmount: [String: Double], | ||
| members: [AppUser], hasExpenses: Bool) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update GroupInformation to support multi-currency balances
The GroupInformation structure needs to be updated to handle multi-currency scenarios. Currently:
userBalanceis a simple Double without currency informationmemberOweAmountdictionary values don't specify currencies
Consider updating the structure to include currency information:
public struct GroupInformation {
public let group: Groups
- public let userBalance: Double
- public let memberOweAmount: [String: Double]
+ public let userBalances: [String: Double] // Currency code to balance mapping
+ public let memberOweAmount: [String: [String: Double]] // Member ID to currency-amount mapping
public let members: [AppUser]
public let hasExpenses: BoolCommittable suggestion skipped: line range outside the PR's diff.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
Currency conversion handling needed for expense updates
The review comment is valid. The Expense model has currency support (
currencyCodefield) but thehandleUpdateExpenseActionmethod doesn't handle currency conversion when updating expenses. This could lead to inconsistencies if expenses are updated with different currencies.
- Add currency conversion logic in
handleUpdateExpenseActionwhennewExpense.currencyCodediffers fromexpense.currencyCode- Consider adding validation to ensure the currency code is supported
- Update the balance calculations to account for currency differences during updates
🔗 Analysis chain
Line range hint
401-448: Consider handling currency conversion in expense updates.When updating expenses, there might be scenarios where the currency changes. Consider adding logic to handle currency conversion when the selected currency differs from the original expense currency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing currency conversion implementations rg -l "convert|exchange|rate" --type swiftLength of output: 938
Script:
#!/bin/bash # Check Expense model for currency fields and CalculateExpensesFunctions for conversion logic echo "=== Expense.swift ===" rg "currency" Data/Data/Model/Expense.swift -A 2 -B 2 echo -e "\n=== CalculateExpensesFunctions.swift ===" rg "currency|convert" Splito/UI/Home/Groups/CalculateExpensesFunctions.swift -A 5 -B 5Length of output: 1163
♻️ Duplicate comments (1)
Data/Data/Model/Currency/Currency.swift (1)
34-40: 🛠️ Refactor suggestionMake the default currency configurable.
The
getCurrentLocalCurrencymethod has a hardcoded fallback to INR.This is a duplicate of a previous review comment. Please refer to the earlier suggestion about making the default currency configurable.
🧹 Nitpick comments (11)
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
121-121: Standardize amount formatting and fallback values.There are inconsistencies in how amounts are formatted and handled:
- Different fallback values: "0" vs "nothing"
- Mixed usage of
formattedAmountandformattedCurrencyConsider standardizing the approach:
- Text(viewModel.expense?.formattedAmount ?? "0") + Text(viewModel.expense?.formattedAmount ?? "0.00") - Text("\(userName.localized) paid \(expense?.formattedAmount ?? "nothing")") + Text("\(userName.localized) paid \(expense?.formattedAmount ?? "0.00")")Also applies to: 182-182, 196-197
121-123: Enhance accessibility for amount displays.Consider adding accessibility labels to provide clear context about the currency:
Text(viewModel.expense?.formattedAmount ?? "0") .font(.Header3()) .foregroundStyle(primaryText) + .accessibilityLabel(Text("Amount: \(viewModel.expense?.formattedAmount ?? "0")"))Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
41-45: Add documentation for new currency-related properties.Consider adding documentation comments to explain the purpose and usage of the new currency properties, especially since they're part of the public interface.
+/// Boolean flag to control the visibility of the currency picker sheet. @Published var showCurrencyPicker = false +/// The currently selected currency for the expense. +/// This property is initialized with the local currency and can be updated based on group settings. @Published private(set) var selectedCurrency: Currency
104-104: Improve currency fallback handling.The current implementation uses a hardcoded "INR" fallback. Consider using the device's locale-based currency as fallback instead of a hardcoded value.
- selectedCurrency = Currency.getCurrencyOfCode(group.defaultCurrency ?? "INR") + selectedCurrency = Currency.getCurrencyOfCode(group.defaultCurrency ?? Currency.getCurrentLocalCurrency().code)
312-313: Enhance validation error handling with specific member information.The current validation only indicates that a member is missing but doesn't specify which one. This makes it harder for users to identify and resolve the issue.
- for payer in expense.paidBy where !group.members.contains(payer.key) { return false } - for memberId in expense.splitTo where !group.members.contains(memberId) { return false } + for payer in expense.paidBy where !group.members.contains(payer.key) { + LogE("Payer \(payer.key) is no longer in the group") + return false + } + for memberId in expense.splitTo where !group.members.contains(memberId) { + LogE("Split recipient \(memberId) is no longer in the group") + return false + }
387-387: Consider extracting group status update logic.The group's
hasExpensesflag update is embedded within the expense addition flow. Consider extracting this into a separate method for better code organization and reusability.+ private func updateGroupExpenseStatus(group: inout Groups) { + if !group.hasExpenses { + group.hasExpenses = true + } + } private func addExpense(group: Groups, expense: Expense) async -> Bool { // ... - if !group.hasExpenses { selectedGroup?.hasExpenses = true } + updateGroupExpenseStatus(group: &selectedGroup!) // ... }Data/Data/Extension/Bundle+Extension.swift (2)
10-10: Consider a more descriptive class nameThe class name
UIBundleFakeClasscould be more descriptive of its purpose, such asDataBundleAnchororDataBundleReference.-private class UIBundleFakeClass {} +private class DataBundleReference {}
12-16: Add documentation for the dataBundle propertyAdd documentation explaining the purpose of this bundle extension and when it should be used, especially since it's marked as public.
public extension Bundle { + /// Returns the bundle containing the Data module's resources. + /// Use this to access currency data and other Data module specific resources. static var dataBundle: Bundle { - return Bundle(for: UIBundleFakeClass.self) + return Bundle(for: DataBundleReference.self) } }Data/Data/Utils/JSONUtils.swift (1)
19-22: Add input validation and improve error handlingThe method should validate the fileName parameter and could benefit from more structured error handling.
public struct JSONUtils { + /// Error types for JSON file operations + public enum JSONError: Error { + case fileNotFound(String) + case decodingError(String, Error) + case invalidFileName + } + + /// Reads and decodes a JSON file into the specified type + /// - Parameters: + /// - fileName: Name of the JSON file without extension + /// - type: The type to decode the JSON into + /// - bundle: Optional bundle containing the JSON file (defaults to main bundle) + /// - Returns: Decoded object of type T or nil if operation fails + /// - Throws: JSONError public static func readJSONFromFile<T: Decodable>(fileName: String, type: T.Type, bundle: Bundle? = nil) -> T? { + guard !fileName.isEmpty else { + LogE("Invalid file name provided") + return nil + } + if let url = (bundle ?? Bundle.main).url(forResource: fileName, withExtension: "json") {Splito/UI/Home/Expense/CurrencyPickerView.swift (1)
23-28: Optimize currency filtering performanceThe current filtering implementation could be optimized for better performance with large currency lists.
private var filteredCurrencies: [Currency] { + // Cache the lowercased search term to avoid repeated conversions + let searchTerm = searchedCurrency.lowercased() currencies.filter { currency in - searchedCurrency.isEmpty ? true : currency.name.lowercased().contains(searchedCurrency.lowercased()) || - currency.code.lowercased().contains(searchedCurrency.lowercased()) + searchedCurrency.isEmpty ? true : + currency.name.lowercased().contains(searchTerm) || + currency.code.lowercased().contains(searchTerm) } }Splito/Localization/Localizable.xcstrings (1)
531-533: Consider improving the error message.The error message "No country found for "%@"!" could be more user-friendly.
Consider this alternative:
- "No country found for \"%@\"!" : { + "Unable to find currency for country code \"%@\". Please try a different country." : {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Data/Data.xcodeproj/project.pbxproj(8 hunks)Data/Data/Extension/Bundle+Extension.swift(1 hunks)Data/Data/Model/Currency/Currencies.json(1 hunks)Data/Data/Model/Currency/Currency.swift(1 hunks)Data/Data/Utils/JSONUtils.swift(1 hunks)Splito.xcodeproj/project.pbxproj(8 hunks)Splito/Localization/Localizable.xcstrings(3 hunks)Splito/UI/Home/Expense/AddAmountView.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(5 hunks)Splito/UI/Home/Expense/CurrencyPickerView.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(1 hunks)Splito/UI/Home/Expense/Note/AddNoteViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (1)
121-121: Verify currency formatting consistency across the view.The removal of the hardcoded "₹" symbol aligns with multi-currency support. However, ensure that:
- The
formattedAmountproperty includes the appropriate currency symbol- The format matches other amount displays in the view (e.g., in
ExpenseInfoView)✅ Verification successful
Currency formatting is properly handled through computed properties ✅
The removal of the hardcoded "₹" symbol is correct as currency formatting is consistently handled through
formattedAmountandformattedCurrencycomputed properties across the codebase. TheformattedAmountinExpenseDetailsViewis backed byformattedCurrencyin the view model, maintaining formatting consistency with other views.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other currency format usages in the view rg "formattedAmount|formattedCurrency" "Splito/UI/Home"Length of output: 6625
Splito/UI/Home/Expense/CurrencyPickerView.swift (1)
54-61: 🛠️ Refactor suggestionImprove list performance and accessibility
The currency list could benefit from performance optimizations and better accessibility support.
- List(currencies, id: \.self) { currency in + List(filteredCurrencies, id: \.self) { currency in CurrencyCellView(currency: currency) { selectedCurrency = currency isPresented = false } .listRowSeparator(.hidden) .listRowInsets(EdgeInsets()) + .accessibilityElement(children: .combine) + .accessibilityLabel("\(currency.name) - \(currency.code)") + .accessibilityAddTraits(.isButton) }Likely invalid or redundant comment.
Splito/UI/Home/Expense/Note/AddNoteViewModel.swift (1)
Line range hint
27-35: LGTM! Parameter reordering looks good.The initializer parameters have been reordered to group related parameters together, improving readability while maintaining functionality.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
117-121: LGTM! AddNoteView parameter order updated correctly.The AddNoteView instantiation has been updated to match the new parameter order from AddNoteViewModel.
Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift (2)
106-108: LGTM! AddNoteView parameter order updated correctly.The AddNoteView instantiation has been updated to match the new parameter order from AddNoteViewModel.
225-225: Verify currency formatting consistency.The default amount format has been changed from a currency-specific format ("₹ 0") to a generic "0". This might affect display consistency across the app.
Run this script to check currency formatting consistency:
Splito/UI/Home/Expense/AddExpenseView.swift (2)
143-144: LGTM! Amount view updated with currency support.The AmountRowView has been replaced with AddAmountView, properly integrating currency selection with amount input.
100-104: LGTM! Currency picker implementation looks good.The currency picker is properly integrated using a sheet presentation and binds to the view model's selected currency.
Let's verify the currency data source:
✅ Verification successful
Currency data source implementation verified and approved
The currency picker uses a well-structured data source with comprehensive currency information including symbols, regions, and proper fallback handling. The duplicate JSON files are intentionally structured for different purposes (data module vs UI previews).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify currency data implementation # Look for currency data source rg -l 'Currencies.json'Length of output: 92
Script:
#!/bin/bash # Find the Currencies.json file fd Currencies.json # Look for Currency model and getAllCurrencies implementation ast-grep --pattern 'struct Currency { $$$ }' rg -A 5 "getAllCurrencies"Length of output: 2382
Script:
#!/bin/bash # Compare the two JSON files diff "Data/Data/Model/Currency/Currencies.json" "Splito/Resource/Currencies.json" # Check JSON structure cat "Data/Data/Model/Currency/Currencies.json"Length of output: 62547
Splito/Localization/Localizable.xcstrings (1)
128-130: LGTM! Improved currency formatting.The change from " ₹ 0.00" to "0.00" removes the hardcoded currency symbol, making the format more flexible for multi-currency support.
Data/Data/Model/Currency/Currency.swift (2)
10-21: LGTM! Well-structured currency model.The
Currencystruct is well-designed with appropriate protocol conformance and property access levels.
23-26: 🛠️ Refactor suggestionAdd error handling for JSON loading failures.
The
getAllCurrenciesmethod silently returns an empty array on failure, which could mask issues.Consider this improved implementation:
- public static func getAllCurrencies() -> [Currency] { - let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) ?? [] - return allCurrencies + public static func getAllCurrencies() throws -> [Currency] { + guard let currencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) else { + throw CurrencyError.failedToLoadCurrencies + } + return currencies + } + + public enum CurrencyError: Error { + case failedToLoadCurrencies + case invalidCurrencyCode + }Likely invalid or redundant comment.
Data/Data.xcodeproj/project.pbxproj (1)
202-210: LGTM! Project configuration is correct.The new files are properly integrated into the project structure with appropriate file references and build phases.
Also applies to: 22-22, 32-33, 90-90, 100-101
Splito.xcodeproj/project.pbxproj (3)
38-39: New UI components added for multi-currency support.The project now includes two new Swift files for handling currency-related UI:
AddAmountView.swift: Likely handles amount input with currency supportCurrencyPickerView.swift: Provides currency selection functionality
547-548: Proper organization of new files in project structure.The new files are well-organized within the project structure:
- UI components (
AddAmountView.swift,CurrencyPickerView.swift) are correctly placed in the UI/Home/Expense group- Currency data (
Currencies.json) is appropriately placed in the Resource groupAlso applies to: 663-663
70-70: Currency data resource added.A new
Currencies.jsonfile has been added to the project resources. This file likely contains the currency metadata for supporting multiple currencies in the app.Let's verify the currency data file:
Also applies to: 234-234
✅ Verification successful
Currency data file verified successfully
The Currencies.json file contains a comprehensive and well-structured list of world currencies with proper ISO codes, names, symbols, and region information. The data is valid and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the structure and content of the Currencies.json file # Expect: A JSON file containing currency definitions # Check if the file exists and examine its contents cat Splito/Resource/Currencies.jsonLength of output: 22092
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: 2
♻️ Duplicate comments (3)
Data/Data/Model/Currency/Currency.swift (3)
23-26:⚠️ Potential issueAdd error handling for currency loading failures.
The
getAllCurrenciesmethod silently returns an empty array if JSON loading fails, which could mask configuration issues.Consider throwing an error when currencies can't be loaded:
- public static func getAllCurrencies() -> [Currency] { - let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) ?? [] - return allCurrencies + public static func getAllCurrencies() throws -> [Currency] { + guard let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) else { + throw CurrencyError.failedToLoadCurrencies + } + return allCurrencies + } + + public enum CurrencyError: Error { + case failedToLoadCurrencies + case invalidCurrencyCode + }
28-32:⚠️ Potential issueImprove error handling in getCurrencyFromCode.
The method silently falls back to INR without indicating the failure to find the requested currency code.
Consider throwing an error for invalid currency codes:
- public static func getCurrencyFromCode(_ code: String) -> Currency { - let allCurrencies = getAllCurrencies() - let currency = allCurrencies.first(where: { $0.code == code }) ?? Currency(code: "INR", name: "Indian Rupee", symbol: "₹", region: "IN") - return currency + public static func getCurrencyFromCode(_ code: String) throws -> Currency { + let allCurrencies = try getAllCurrencies() + guard let currency = allCurrencies.first(where: { $0.code == code }) else { + throw CurrencyError.invalidCurrencyCode + } + return currency + }
34-40: 🛠️ Refactor suggestionMake the default currency configurable.
The hardcoded fallback to INR makes the code less flexible for international deployment.
Consider adding a configurable default currency:
+ public static var defaultCurrency = Currency(code: "INR", name: "Indian Rupee", symbol: "₹", region: "IN") + public static func getCurrentLocalCurrency() throws -> Currency { - let allCurrencies = getAllCurrencies() + let allCurrencies = try getAllCurrencies() let currentLocal = Locale.current.region?.identifier return allCurrencies.first(where: { $0.region == currentLocal }) ?? - (allCurrencies.first ?? Currency(code: "INR", name: "Indian Rupee", symbol: "₹", region: "IN")) + (allCurrencies.first ?? defaultCurrency) }
🧹 Nitpick comments (3)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
115-119: Consider moving currency loading to the view model.Loading currencies directly in the view using
Currency.getAllCurrencies()could impact performance as it's called on every view render. Consider moving this to the view model and loading it once during initialization.- let currencies = Currency.getAllCurrencies() CurrencyPickerView(currencies: currencies, selectedCurrency: $viewModel.selectedCurrency, isPresented: $viewModel.showCurrencyPicker)Move the currency loading to the view model:
class GroupPaymentViewModel: ObservableObject { @Published private(set) var currencies: [Currency] init() { self.currencies = Currency.getAllCurrencies() // ... other initialization } }Then use
viewModel.currenciesin the view.Data/Data/Repository/UserRepository.swift (1)
38-46: Optimize batch user fetching performance.The current implementation fetches users sequentially, which could be slow for large batches. Consider using batch operations if supported by the backend.
Consider this optimization:
public func fetchUsersBy(userIds: [String]) async throws -> [AppUser] { + // TODO: If backend supports batch fetching, use that instead + async let userFetches = userIds.map { fetchUserBy(userID: $0) } + let users = try await userFetches.compactMap { try await $0 } - var users: [AppUser] = [] - for userId in userIds { - let user = try await fetchUserBy(userID: userId) - guard let user else { continue } - users.append(user) - } return users.uniqued() }Splito/UI/Home/Expense/AddExpenseView.swift (1)
143-144: Add loading and error states to AddAmountView.The view doesn't handle currency loading states or errors, which could lead to a poor user experience if currency data fails to load.
Consider adding loading and error states to the view:
AddAmountView(amount: $viewModel.expenseAmount, showCurrencyPicker: $viewModel.showCurrencyPicker, - selectedCurrencySymbol: viewModel.selectedCurrency.symbol, isAmountFocused: $isAmountFocused) + selectedCurrencySymbol: viewModel.selectedCurrency.symbol, + isLoading: viewModel.isLoadingCurrencies, + error: viewModel.currencyLoadError, + isAmountFocused: $isAmountFocused)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Model/Currency/Currency.swift(1 hunks)Data/Data/Repository/UserRepository.swift(1 hunks)Splito/UI/Home/Expense/AddAmountView.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(9 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(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Splito/UI/Home/Expense/AddAmountView.swift
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
🔇 Additional comments (2)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
64-65: LGTM! Clean implementation of the amount input view.The AddAmountView integration is well-structured with proper bindings and currency symbol handling.
122-125: LGTM! Parameter ordering is consistent.The AddNoteView initialization correctly follows the updated parameter order from AddNoteViewModel.
#Conflicts: # Splito/Localization/Localizable.xcstrings # Splito/UI/Home/Expense/AddExpenseViewModel.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
64-65: Fix parameter indentation and consider separating currency selection.The parameter indentation is inconsistent (12 spaces). Additionally, consider separating the currency selection into a dedicated component to better follow the Single Responsibility Principle.
- AddAmountView(amount: $viewModel.amount, showCurrencyPicker: $viewModel.showCurrencyPicker, - selectedCurrencySymbol: viewModel.selectedCurrency.symbol, isAmountFocused: $isAmountFocused) + AddAmountView( + amount: $viewModel.amount, + showCurrencyPicker: $viewModel.showCurrencyPicker, + selectedCurrencySymbol: viewModel.selectedCurrency.symbol, + isAmountFocused: $isAmountFocused + )
121-122: Maintain consistent parameter ordering and indentation.The parameter order has been changed without apparent reason, which could make future diffs harder to read. Also, fix the inconsistent indentation.
- AddNoteView(viewModel: AddNoteViewModel(group: viewModel.group, note: viewModel.paymentNote, - payment: viewModel.transaction, - paymentReason: viewModel.paymentReason, - handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:reason:))) + AddNoteView(viewModel: AddNoteViewModel( + group: viewModel.group, + payment: viewModel.transaction, + note: viewModel.paymentNote, + paymentReason: viewModel.paymentReason, + handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:reason:) + ))Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (1)
69-69: Consider group's default currency during initialization.Currently initializing with local currency, but the group might have a different default currency.
- self.selectedCurrency = Currency.getCurrentLocalCurrency() + // Defer currency initialization until group is fetched + self.selectedCurrency = Currency.getCurrencyFromCode("USD") // Use a stable default like USD
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Data/Data.xcodeproj/project.pbxproj(8 hunks)Splito.xcodeproj/project.pbxproj(9 hunks)Splito/Localization/Localizable.xcstrings(3 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(10 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(1 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(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift
- Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
- Data/Data.xcodeproj/project.pbxproj
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
Splito/UI/Home/Expense/AddExpenseView.swift (3)
101-105: Move currency loading to ViewModel and add error handling.The synchronous loading of currencies in the view could block the UI thread. Additionally, there's no error handling for currency loading failures.
144-145: LGTM! Clean implementation of the amount input view.The new AddAmountView properly integrates currency selection functionality with clear bindings and good separation of concerns.
Line range hint
558-559: LGTM! Good file organization.Currency-related views are appropriately placed in the Expense directory since they're primarily used in the expense creation flow.
Splito.xcodeproj/project.pbxproj (1)
39-40: LGTM! Project configuration properly updated.All new files are correctly added to the project with appropriate build phases and file references:
- AddAmountView.swift and CurrencyPickerView.swift added to Sources
- Currencies.json added to Resources
Also applies to: 71-71, 202-203, 236-236, 927-927, 1160-1160, 1169-1169
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (4)
33-37: LGTM! Currency picker and selection properties added.The new properties
showCurrencyPickerandselectedCurrencyare well-placed and properly scoped for managing currency selection in the view model.
123-129: LGTM! User data fetching consolidated.Good refactoring to combine user data fetching into a single method, improving code organization and reducing duplicate logic.
239-239: Ensure consistent currency handling across transactions.The implementation correctly sets the currency code for both new and updated transactions. However, consider validating currency conversion when updating transactions with different currencies.
Consider these scenarios:
- Currency conversion when updating transaction currency
- Group balance updates with mixed currencies
- Historical exchange rates for past transactions
Would you like me to propose a more robust currency handling implementation?
Also applies to: 242-244
95-96: Verify error handling for invalid currency codes.The currency initialization from group's defaultCurrency needs validation.
✅ Verification successful
Currency code handling is properly implemented with fallback mechanism
The
getCurrencyFromCodemethod already implements a fail-safe approach by returning a default INR currency when an invalid code is provided, which is appropriate for UI scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Currency.getCurrencyFromCode error handling rg -A 3 "getCurrencyFromCode"Length of output: 2602
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: 2
🔭 Outside diff range comments (1)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (1)
Currency conversion functionality is indeed missing
The codebase has proper currency tracking per group and transaction but lacks:
- Currency conversion implementation for mixed-currency calculations
- Validation for currency compatibility between transactions
- Cross-currency balance aggregation logic
This confirms the architectural concerns raised in the review comment about potential issues when dealing with transactions in multiple currencies.
🔗 Analysis chain
Line range hint
309-321: Consider enhancing currency handling for mixed-currency scenarios.The current implementation might need additional handling for:
- Validating currency compatibility between transactions in a group
- Converting amounts to a common currency when calculating group balances
- Displaying total balances with proper currency indicators
This could prevent potential issues when dealing with transactions in multiple currencies within the same group.
Let's verify if there are any existing currency conversion implementations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for currency conversion related code rg -i "convert.*currency|exchange.*rate"Length of output: 42
Script:
#!/bin/bash # Search for currency-related code with broader patterns echo "=== Searching for currency types and models ===" rg -i "class.*currency|struct.*currency|enum.*currency" echo -e "\n=== Searching for currency property declarations ===" ast-grep --pattern 'let currency' ast-grep --pattern 'var currency' echo -e "\n=== Searching for common currency codes ===" rg -i "USD|EUR|GBP" echo -e "\n=== Searching for amount calculations with potential currency context ===" rg -i "amount.*currency|balance.*currency"Length of output: 30494
🧹 Nitpick comments (4)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
120-123: Fix parameter indentation while maintaining logical grouping.The parameters are logically grouped, but the indentation is inconsistent with Swift style guidelines.
- AddNoteView(viewModel: AddNoteViewModel(group: viewModel.group, note: viewModel.paymentNote, - payment: viewModel.transaction, - paymentReason: viewModel.paymentReason, - handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:reason:))) + AddNoteView( + viewModel: AddNoteViewModel( + group: viewModel.group, + note: viewModel.paymentNote, + payment: viewModel.transaction, + paymentReason: viewModel.paymentReason, + handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:reason:) + ) + )Data/Data/Model/Currency/Currency.swift (2)
25-28: Consider adding debug logging.While the fail-safe implementation is appropriate, adding debug logging when currency loading fails could help with troubleshooting.
public static func getAllCurrencies() -> [Currency] { - let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) ?? [] + let allCurrencies = JSONUtils.readJSONFromFile(fileName: "Currencies", type: [Currency].self, bundle: .dataBundle) ?? { + #if DEBUG + print("Warning: Failed to load currencies, using empty list") + #endif + return [] + }() return allCurrencies }
30-34: Consider caching currencies for better performance.The method calls
getAllCurrencies()for each currency code lookup. Consider caching the currencies list to avoid repeated JSON parsing.public struct Currency: Decodable, Hashable { + private static var cachedCurrencies: [Currency]? + public static func getCurrencyFromCode(_ code: String?) -> Currency { - let allCurrencies = getAllCurrencies() + let allCurrencies = cachedCurrencies ?? { + let currencies = getAllCurrencies() + cachedCurrencies = currencies + return currencies + }() let currency = allCurrencies.first(where: { $0.code == code }) ?? defaultCurrency return currency }Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
250-251: Consider explicit case handling instead of default.While adding a default case improves safety, consider explicitly handling all
AVAuthorizationStatuscases to ensure future enum cases are properly addressed.- case .authorized: authorized() - default: return + case .authorized: + authorized() + @unknown default: + LogE("AddExpenseViewModel: Unknown camera authorization status") + return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Data/Data/Model/Currency/Currencies.json(1 hunks)Data/Data/Model/Currency/Currency.swift(1 hunks)Data/Data/Model/Expense.swift(2 hunks)Data/Data/Model/Groups.swift(4 hunks)Data/Data/Model/Transaction.swift(2 hunks)Data/Data/Repository/CommentRepository.swift(1 hunks)Splito.xcodeproj/project.pbxproj(5 hunks)Splito/Localization/Localizable.xcstrings(2 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(2 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(10 hunks)Splito/UI/Home/Expense/CurrencyPickerView.swift(1 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(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Data/Data/Model/Currency/Currencies.json
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Expense/AddExpenseView.swift
- Splito/UI/Home/Expense/CurrencyPickerView.swift
- Data/Data/Model/Transaction.swift
- Data/Data/Model/Groups.swift
- Splito.xcodeproj/project.pbxproj
🧰 Additional context used
📓 Learnings (2)
Data/Data/Model/Currency/Currency.swift (2)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Data/Data/Model/Currency/Currency.swift:0-0
Timestamp: 2025-01-16T13:25:11.563Z
Learning: In the Splito app, when an invalid or unsupported currency code is encountered, the system should silently fall back to INR (Indian Rupee) as the default currency, without throwing errors.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Data/Data/Model/Currency/Currency.swift:0-0
Timestamp: 2025-01-16T13:08:06.581Z
Learning: In the Splito app's currency handling, prefer fail-safe implementations with INR as the default currency over throwing errors, as demonstrated in the `Currency.getCurrentLocalCurrency()` method.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Expense/AddAmountView.swift:0-0
Timestamp: 2025-01-16T13:24:04.180Z
Learning: In the Splito app's AddAmountView, simple amount formatting is preferred over complex validation and formatting logic with NumberFormatter.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (15)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
64-65: Fix inconsistent parameter indentation.The parameters are indented with 15 spaces, which is inconsistent with Swift style guidelines. Align parameters with 4 spaces from the opening parenthesis.
- AddAmountView(amount: $viewModel.amount, showCurrencyPicker: $viewModel.showCurrencyPicker, - selectedCurrencySymbol: viewModel.selectedCurrency.symbol, isAmountFocused: $isAmountFocused) + AddAmountView( + amount: $viewModel.amount, + showCurrencyPicker: $viewModel.showCurrencyPicker, + selectedCurrencySymbol: viewModel.selectedCurrency.symbol, + isAmountFocused: $isAmountFocused + )
114-117: Optimize currency loading and fix indentation.
- The currency loading optimization suggestion from previous reviews hasn't been addressed. Consider moving currency loading to the ViewModel to improve performance.
- The parameter indentation is inconsistent (15 spaces).
.sheet(isPresented: $viewModel.showCurrencyPicker) { - CurrencyPickerView(selectedCurrency: $viewModel.selectedCurrency, - isPresented: $viewModel.showCurrencyPicker) + CurrencyPickerView( + selectedCurrency: $viewModel.selectedCurrency, + isPresented: $viewModel.showCurrencyPicker + ) }Consider implementing the previously suggested ViewModel optimization:
class GroupPaymentViewModel: ObservableObject { @Published private(set) var currencies: [Currency] = [] func loadCurrencies() async { currencies = await Currency.getAllCurrencies() } }Data/Data/Model/Currency/Currency.swift (1)
10-16: LGTM! Well-structured currency model.The Currency struct is well-designed with immutable properties and appropriate protocol conformance. The default currency implementation aligns with the established requirement to use INR as a fallback.
Data/Data/Model/Expense.swift (1)
18-18: LGTM! Consistent use of default currency.The changes appropriately integrate with the new currency system by using
Currency.defaultCurrency.codefor both the property default value and initializer parameter.Also applies to: 32-33
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift (5)
33-33: LGTM! Well-structured currency-related property declarations.The properties are correctly marked as @published and follow SwiftUI conventions for state management.
Also applies to: 37-37
69-69: LGTM! Proper currency initialization with system locale.The selectedCurrency is appropriately initialized with the user's local currency as a sensible default.
95-96: LGTM! Robust currency handling in data fetching.The implementation correctly prioritizes:
- Transaction's currency code (if available)
- Falls back to group's default currency
- Uses system locale as final fallback
Also applies to: 114-114
238-238: LGTM! Consistent currency handling in transactions.The implementation properly uses the selected currency for both new and existing transactions, avoiding any hardcoded values.
Also applies to: 241-243
122-128: LGTM! Well-structured user data fetching.The consolidated getPaymentUsersData() method improves code organization with proper error handling and state management.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (6)
42-42: LGTM! Well-structured currency-related properties.The new properties follow the established patterns and provide clear separation between UI state (
showCurrencyPicker) and data state (selectedCurrency).Also applies to: 46-46
81-81: LGTM! Good currency synchronization with group.The currency is correctly synchronized with the group's default currency after fetching group data.
122-122: LGTM! Robust currency handling with proper fallback.The implementation correctly prioritizes the expense's currency code while falling back to the group's default currency when needed.
377-380: LGTM! Clean integration of currency in expense creation.The currency code is properly integrated into the expense creation process while maintaining code readability.
316-317: LGTM! Clean code refactoring.The simplified validation logic improves readability while maintaining the same functionality.
60-60: Consider adding fallback handling for currency initialization.While using the local currency as default is user-friendly, consider adding a fallback mechanism in case
getCurrentLocalCurrency()fails.
| @@ -94,7 +94,7 @@ public class CommentRepository { | |||
| let payerName = (user.id == transaction.payerId && memberId == transaction.payerId) ? | |||
| (user.id == transaction.addedBy ? "You" : "you") : | |||
| (memberId == transaction.payerId) ? "you" : members.payer.nameWithLastInitial | |||
|
|
|||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify redundant receiver name conditions.
The current implementation has duplicate conditions that check memberId == transaction.receiverId and return the same value. Additionally, the capitalization of "you" should be consistent with the payerName logic above.
Apply this diff to simplify the logic and maintain consistency:
- let receiverName = (memberId == transaction.receiverId) ? "you" : (memberId == transaction.receiverId) ? "you" : members.receiver.nameWithLastInitial
+ let receiverName = (memberId == transaction.receiverId) ?
+ (user.id == transaction.addedBy ? "You" : "you") :
+ members.receiver.nameWithLastInitialCommittable suggestion skipped: line range outside the PR's diff.
| @@ -416,6 +413,7 @@ extension AddExpenseViewModel { | |||
| newExpense.updatedAt = Timestamp() | |||
| newExpense.updatedBy = userId | |||
| newExpense.note = expenseNote | |||
| newExpense.currencyCode = selectedCurrency.code | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling currency conversion during expense updates.
When updating the currency code, consider implementing currency conversion logic to maintain the expense's value across different currencies. This is particularly important when changing the currency of an existing expense.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
Splito/UI/Home/Groups/GroupListViewModel.swift (2)
304-305:⚠️ Potential issueCritical: Currency summation issue in search bar handling
Same issue as in
filteredGroupswhere balances in different currencies are being summed directly.
326-327:⚠️ Potential issueCritical: Currency summation issue in tab selection handling
Same issue as in
filteredGroupswhere balances in different currencies are being summed directly.
🧹 Nitpick comments (14)
Data/Data/Extension/Double+Extension.swift (1)
Line range hint
34-43: Consider refactoring to reduce code duplication.The
formattedCurrencyWithSignproperty appears redundant with similar formatting logic. Consider removing it or refactoring to use the newformattedCurrencyWithSign(_ sign: String)method.- var formattedCurrencyWithSign: String { - let formatter = NumberFormatter() - formatter.locale = Locale.current - - if let formattedAmount = formatter.string(from: NSNumber(value: self)) { - return formattedAmount - } else { - return String(format: "%.2f", self.rounded()) // Fallback to a basic decimal format - } - }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
24-33: Consider additional architectural improvements for robust multi-currency support.The current implementation handles basic multi-currency storage, but consider these architectural improvements:
- Exchange rate handling for currency conversions
- Currency formatting utilities for consistent display
- Default currency fallback mechanism
- Currency precision handling (different currencies have different decimal places)
Would you like me to provide detailed implementation suggestions for any of these improvements?
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift (1)
17-17: Consider adding type aliases for better readability.The nested dictionary structure is well-documented but could be more readable with type aliases.
+ typealias MemberBalance = [String: Double] + typealias CurrencyMemberBalances = [String: MemberBalance] - @Published private(set) var memberOwingAmount: [String: [String: Double]] = [:] + @Published private(set) var memberOwingAmount: CurrencyMemberBalances = [:]Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
204-204: Consider extracting balance display logic into a separate view.The balance display logic could be moved to a dedicated view component to improve maintainability and reusability.
struct BalanceView: View { let currency: String let amount: Double var body: some View { if amount == 0 { Text("settled up") .font(.caption1()) .foregroundStyle(disableText) } else { let isBorrowed = amount < 0 VStack(alignment: .trailing, spacing: 4) { Text(isBorrowed ? "owes" : "gets back") .font(.caption1()) if let currencySymbol = Currency.getCurrencyFromCode(currency)?.symbol { Text("\(currencySymbol) \(amount.formattedCurrency)") .font(.body1()) } } .foregroundStyle(isBorrowed ? errorColor : successColor) } } }Also applies to: 250-269
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (2)
281-286: Enhance error handling for unknown members.The current implementation falls back to "Unknown" for missing member data. Consider logging these cases and handling them more gracefully.
-let name = viewModel.getMemberDataBy(id: memberId)?.nameWithLastInitial ?? "Unknown" +guard let memberData = viewModel.getMemberDataBy(id: memberId) else { + Logger.error("Member data not found for ID: \(memberId)") + return +} +let name = memberData.nameWithLastInitial
304-305: Consider currency formatting localization.The current implementation might not handle currency symbol placement correctly for all locales. Different regions have different conventions for currency symbol placement (prefix vs suffix) and decimal separator.
Consider using
NumberFormatterwith proper locale settings:extension Double { var localizedCurrencyString: String { let formatter = NumberFormatter() formatter.numberStyle = .currency formatter.locale = Locale.current // or user's preferred locale return formatter.string(from: NSNumber(value: self)) ?? "" } }Also applies to: 308-312
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (1)
78-99: Improve code clarity and efficiency.The function could benefit from some improvements:
- Add documentation explaining the currency-based balance logic
- Simplify the filtering logic using Swift's functional methods
Consider this refactor:
+ /// Returns a dictionary of non-zero balances for a member, mapped by currency. + /// If all balances are zero, returns the original balance amounts. + /// - Parameter memberId: The ID of the member whose balances to retrieve + /// - Returns: Dictionary mapping currency codes to balance amounts func getMembersBalance(memberId: String) -> [String: Double] { guard let group else { LogE("GroupSettingViewModel: \(#function) group not found.") return [:] } guard let memberBalance = group.balances.first(where: { $0.id == memberId })?.balanceByCurrency else { LogE("GroupSettingViewModel: \(#function) Member's balance not found from balances.") return [:] } - var filteredBalances: [String: Double] = [:] - for (currency, balanceInfo) in memberBalance { - if balanceInfo.balance == 0 { continue } - filteredBalances[currency] = balanceInfo.balance - } + let balances = memberBalance.mapValues { $0.balance } + let nonZeroBalances = balances.filter { $0.value != 0 } - if filteredBalances.isEmpty { - return memberBalance.mapValues { $0.balance } + if nonZeroBalances.isEmpty { + return balances } - return filteredBalances + return nonZeroBalances }Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (1)
75-80: Consider handling currency conversion for overall balancesCurrently, the balances are managed per currency without conversion rates. If there's a need to display or compute overall balances across all currencies, consider integrating currency conversion using exchange rates.
This would involve fetching current exchange rates and converting all balances to a base currency for aggregate calculations, enhancing the user's understanding of their total balances.
Also applies to: 123-123
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (1)
93-94: Rounding may introduce cumulative errorsRounding the amounts to two decimal places after each settlement can introduce cumulative rounding errors, especially in loops. Instead, consider maintaining high-precision calculations throughout and only rounding when displaying amounts.
Apply this diff to adjust your rounding strategy:
- creditors[i].1 = round(credAmt * 100) / 100 - debtors[j].1 = round(debtAmt * 100) / 100 + // Avoid rounding here to maintain precisionOnly round the final amounts when presenting to the user.
Splito/UI/Home/Groups/GroupListWithDetailView.swift (1)
109-113: Simplify balance calculations and handle possible nil valuesWhen calculating
userBalanceand checking if the user has balances, ensure that you handle possible nil values and consider using a computed property for clarity.Consider refactoring as follows:
let defaultCurrency = group.group.defaultCurrencyCode let userBalance = group.userBalance[defaultCurrency] ?? group.userBalance.first?.value ?? 0 let totalBalance = group.userBalance.values.reduce(0, +) let isBorrowed = totalBalance < 0 if totalBalance == 0 { // ... } else { // ... }Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (1)
89-89: ComputetotalOwedonce and reuseYou are calculating
totalOwedmultiple times. Computing it once and reusing it can improve readability and performance.Ensure
totalOwedis calculated once:let totalOwed = memberBalance.totalOwedAmount.reduce(0) { $0 + $1.value }Data/Data/Model/Groups.swift (3)
25-25: Consider makingdefaultCurrencynon-optionalSince
defaultCurrencyhas a default value and the computed propertydefaultCurrencyCodealways provides a fallback, consider makingdefaultCurrencynon-optional to better represent its invariant.- private var defaultCurrency: String? = Currency.defaultCurrency.code + private var defaultCurrency: String = Currency.defaultCurrency.code public var defaultCurrencyCode: String { - defaultCurrency ?? Currency.defaultCurrency.code + defaultCurrency }Also applies to: 28-30
Line range hint
89-102: Add documentation for GroupCurrencyBalance propertiesConsider adding documentation comments to explain the purpose of each property:
public struct GroupCurrencyBalance: Codable { + /// The current balance in this currency public var balance: Double + /// Monthly summary of transactions in this currency public var totalSummary: [GroupTotalSummary]
159-165: Add documentation and consider type aliases for currency mappingsThe multi-currency changes look good, but could benefit from better documentation and type safety:
+ /// Type alias for currency code to amount mapping + public typealias CurrencyBalance = [String: Double] + /// Type alias for member ID to currency balance mapping + public typealias MemberCurrencyBalance = [String: CurrencyBalance] + public struct GroupInformation { public let group: Groups - public let userBalance: [String: Double] - public let memberOweAmount: [String: [String: Double]] + /// Balance per currency code for the current user + public let userBalance: CurrencyBalance + /// Amount owed per member ID and currency code + public let memberOweAmount: MemberCurrencyBalance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Model/Groups.swift(4 hunks)Splito/Localization/Localizable.xcstrings(4 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(10 hunks)Splito/UI/Home/Groups/CalculateExpensesFunctions.swift(8 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift(5 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift(3 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(4 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(2 hunks)Splito/UI/Home/Groups/GroupListViewModel.swift(4 hunks)Splito/UI/Home/Groups/GroupListWithDetailView.swift(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpView.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentViewModel.swift
🧰 Additional context used
📓 Learnings (1)
Data/Data/Extension/Double+Extension.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Data/Data/Extension/Double+Extension.swift:23-34
Timestamp: 2025-01-16T13:28:57.497Z
Learning: In the Splito app's Double extension, the `formattedCurrencyWithSign(_ sign: String)` method should not set NumberFormatter's numberStyle to .currency as it would use the current locale's currency format, which conflicts with the app's multi-currency support. Instead, it should accept and use the currency sign parameter explicitly.
🔇 Additional comments (19)
Data/Data/Extension/Double+Extension.swift (1)
24-25: Correctly implements currency formatting without .currency style.The implementation correctly avoids using
.currencynumberStyle, which aligns with the multi-currency support requirements by preventing locale-specific currency formatting.Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (2)
283-286: 🛠️ Refactor suggestionAdd currency validation and error handling.
The current implementation assumes all currency codes are valid. Consider adding validation and error handling:
- let balance = group.balances.first(where: { $0.id == userId })?.balanceByCurrency ?? [:] - for (currency, groupBalance) in balance { - overallOwingAmount[currency] = groupBalance.balance - } + let balance = group.balances.first(where: { $0.id == userId })?.balanceByCurrency ?? [:] + for (currency, groupBalance) in balance { + guard Currency.isValid(currency) else { + LogE("Invalid currency code: \(currency)") + continue + } + overallOwingAmount[currency] = groupBalance.balance + }Also, consider adding documentation to explain the currency handling:
+ /// Dictionary mapping currency codes to their respective balances. + /// The keys are ISO 4217 currency codes (e.g., "USD", "EUR"). @Published private(set) var overallOwingAmount: [String: Double] = [:]Likely invalid or redundant comment.
24-24: Verify the impact of type changes on dependent code.The property type changes from simple to dictionary types support multi-currency functionality. However, these changes might affect code that consumes these properties.
Run the following script to find potential usage that needs updating:
Also applies to: 33-33
✅ Verification successful
Type changes have been properly handled across the codebase
All usages of
overallOwingAmountandmemberOwingAmounthave been correctly updated to handle the new currency-based dictionary types, including balance calculations, display logic, and settlement processing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find potential usages of overallOwingAmount and memberOwingAmount that might need updates # Search for direct property access rg -A 3 "overallOwingAmount|memberOwingAmount" --type swift # Search for pattern matching or dictionary access ast-grep --pattern 'overallOwingAmount[$_]' --lang swift ast-grep --pattern 'memberOwingAmount[$_]' --lang swiftLength of output: 9359
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
Line range hint
131-135: Add validation for empty balance dictionary.While the multi-currency support looks good, consider adding validation to handle cases where the balance dictionary is empty to prevent potential UI issues.
Consider adding a check:
let balance = viewModel.getMembersBalance(memberId: member.id) +guard !balance.isEmpty else { + // Handle empty balance case, perhaps show a default currency with 0 amount + return +} GroupMemberCellView(member: member, balance: balance,Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift (2)
Line range hint
1-133: Consider architectural improvements for multi-currency support.The current implementation needs a more comprehensive approach to currency management:
Add currency-related state:
- Track selected currency
- Handle currency switching
- Support currency formatting
Consider UI/UX implications:
- Currency selection UI
- Currency display format
- Currency conversion handling
Consider these architectural improvements:
// Add currency-related state @Published private(set) var selectedCurrency: String @Published private(set) var availableCurrencies: [String] // Add currency formatting helper private func formatAmount(_ amount: Double, currency: String) -> String { // Implement currency-aware formatting } // Add currency switching support func switchCurrency(_ currency: String) { // Handle currency switching logic }Let's verify the currency-related implementations across the codebase:
69-70:⚠️ Potential issueRemove hard-coded currency and improve null safety.
The current implementation has several issues:
- Hard-coding "INR" as the currency defeats the purpose of multi-currency support
- No fallback handling if the "INR" currency data is missing
- Currency-specific logic should be abstracted
Consider this approach:
- let groupBalance = group.balances.first(where: { $0.id == userId })?.balanceByCurrency ?? [:] - summaries = groupBalance["INR"]?.totalSummary ?? [] + guard let defaultCurrency = preference.defaultCurrency else { + LogE("Default currency not set") + summaries = [] + return + } + let groupBalance = group.balances.first(where: { $0.id == userId })?.balanceByCurrency ?? [:] + summaries = groupBalance[defaultCurrency]?.totalSummary ?? []Let's verify if
preference.defaultCurrencyexists and check for other currency-related configurations:Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (1)
258-258: Consider currency-specific balance checks.The current implementation sums up amounts across different currencies, which might hide currency-specific imbalances. For example, if a user owes +100 USD and -100 EUR, the sum would be 0 but there are still unsettled amounts in each currency.
Consider checking if all currency-specific balances are zero instead:
-if viewModel.overallOwingAmount.values.reduce(0, +) == 0 { +if viewModel.overallOwingAmount.allSatisfy({ $0.value == 0 }) {Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift (2)
186-186: Same currency and precision concerns as in showRemoveMemberAlert.This check has the same issues as discussed in the previous comment about currency conversion and floating-point comparison.
78-99: Verify currency handling across the codebase.Let's verify that currency handling is consistent across the application.
Run this script to check currency-related implementations:
✅ Verification successful
Multi-currency implementation verified as correct
The code correctly handles multi-currency balances by:
- Maintaining separate balances per currency
- Preserving currency information throughout transactions
- Appropriately filtering zero balances while maintaining currency separation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for currency handling patterns across the codebase echo "Checking balance calculations:" rg -l "balance.*reduce|reduce.*balance" --type swift echo "\nChecking currency conversion implementations:" rg -l "convert.*currency|currency.*convert" --type swift echo "\nChecking for potential floating-point comparisons:" rg -l "== 0|!= 0" --type swiftLength of output: 1871
Script:
#!/bin/bash # Examine balance calculations and currency handling echo "Checking balance calculation implementations:" rg "func.*balance|balance.*func" -A 5 --type swift echo "\nChecking currency-related type definitions:" ast-grep --pattern 'struct $_ { $$$ currency $$$ }' echo "\nChecking for currency property access patterns:" rg "\.currency" -A 2 --type swiftLength of output: 15718
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift (4)
75-80: Good implementation of multi-currency member balancesThe use of
flatMapto handlebalanceByCurrencyeffectively creates combined balances for each member per currency. This approach correctly accommodates the multi-currency support.
88-106: Ensure consistent updates of member balances during settlementsThe logic for updating sender and receiver balances during settlements seems complex due to the nested dictionary structures. Verify that all currency balances are accurately updated and that negative balances are correctly handled.
Consider adding unit tests for this section to cover various scenarios, ensuring the correctness of balance updates across different currencies.
84-84:⚠️ Potential issueVerify that
calculateSettlementssupports multiple currenciesSince balances are now managed per currency, please ensure that the
calculateSettlementsfunction has been updated to handle settlements for each currency individually. This is crucial to maintain accurate financial calculations across different currencies.If necessary, update
calculateSettlementsto process settlements separately for each currency to prevent incorrect settlements between members holding different currencies.
184-185: Verify all usages ofMembersCombinedBalanceare updatedWith the
MembersCombinedBalancestruct now using dictionaries fortotalOwedAmountandbalances, ensure that all references throughout the codebase are updated to reflect these changes. This helps prevent runtime errors due to mismatched data types.Run the following script to find potential outdated usages:
This script searches for usages of
totalOwedAmountandbalancesnot followed by a subscript[, which may indicate accesses expecting non-dictionary types.✅ Verification successful
All usages of dictionary properties are correctly implemented
The verification confirms that all references to
totalOwedAmountandbalancesthroughout the codebase are properly using dictionary access patterns and operations. No outdated or incorrect usages were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find instances where 'totalOwedAmount' or 'balances' are accessed improperly. rg --glob '*.swift' '\.totalOwedAmount\s*[^[]' -A 2 rg --glob '*.swift' '\.balances\s*[^[]' -A 2Length of output: 304
Script:
#!/bin/bash # Search for property accesses and assignments rg --glob '*.swift' 'totalOwedAmount' -B 2 -A 2 rg --glob '*.swift' '\.balances[^a-zA-Z]' -B 2 -A 2 # Search for struct initialization ast-grep --pattern 'MembersCombinedBalance(totalOwedAmount: $_)' ast-grep --pattern 'MembersCombinedBalance(balances: $_)'Length of output: 16637
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (1)
64-70: Handle missing currency balances gracefullyIn the loop where you separate creditors and debtors, if
balance.balanceByCurrency[currency]?.balanceis nil, the loop skips processing that member. Ensure that all balances are considered, and handle cases where the currency balance might be missing.Consider verifying that all members have an entry for the current currency or handle the nil case appropriately.
Splito/UI/Home/Groups/GroupListWithDetailView.swift (1)
122-124: Handle currency code safely when retrieving currency symbolWhen accessing the currency symbol, ensure that the currency code is valid to prevent unexpected errors.
Add a default case or error handling in case
Currency.getCurrencyFromCode(currency)returns nil.let currencySymbol = Currency.getCurrencyFromCode(currency)?.symbol ?? "$"Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (1)
Line range hint
156-196: Iterate over balances carefully to avoid unexpected behaviorWhen iterating over nested dictionaries of balances, ensure that the sorting and access are correct, and handle cases where balances might be zero or nil.
Double-check the iteration logic to confirm that all balances are displayed correctly and that the amounts correspond to the right members and currencies.
Data/Data/Model/Groups.swift (2)
35-35: LGTM!Good use of
Currency.defaultCurrency.codeas the default value for consistency.
76-87: Well-structured multi-currency balance implementation!The implementation properly handles multiple currencies with clear property naming, documentation, and complete Codable support.
Splito/UI/Home/Groups/GroupListViewModel.swift (1)
245-254: LGTM! Well-structured currency balance retrievalThe implementation properly handles currency-specific balances and safely handles the case when a member is not found.
| func formattedCurrencyWithSign(_ sign: String) -> String { | ||
| let amount: String | ||
| let formatter = NumberFormatter() | ||
| formatter.locale = Locale.current | ||
|
|
||
| if let formattedAmount = formatter.string(from: NSNumber(value: self)) { | ||
| amount = formattedAmount | ||
| } else { | ||
| amount = String(format: "%.2f", self.rounded()) // Fallback to a basic decimal format | ||
| } | ||
| return sign + " " + amount | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider edge cases in currency formatting.
The implementation needs improvements to handle various currency formatting scenarios:
- Negative numbers aren't handled consistently with
formattedCurrency - Fixed space between sign and amount might not work for all currencies
- No support for RTL languages where the sign might need to be on the right
Consider this improved implementation:
func formattedCurrencyWithSign(_ sign: String) -> String {
let amount: String
let formatter = NumberFormatter()
formatter.locale = Locale.current
+ formatter.minimumFractionDigits = 2
+ formatter.maximumFractionDigits = 2
if let formattedAmount = formatter.string(from: NSNumber(value: self)) {
- amount = formattedAmount
+ amount = formattedAmount.hasPrefix("-") ?
+ String(formattedAmount.dropFirst()) : formattedAmount
} else {
amount = String(format: "%.2f", self.rounded())
}
- return sign + " " + amount
+
+ // Handle RTL languages
+ let isRTL = Locale.current.languageCode?.matches(
+ NSLocale.characterDirection: .rightToLeft) ?? false
+ return isRTL ? amount + " " + sign : sign + " " + amount
}Committable suggestion skipped: line range outside the PR's diff.
| func getMembersBalance(memberId: String) -> [String: Double] { | ||
| guard let group else { | ||
| LogE("GroupSettingViewModel: \(#function) group not found.") | ||
| return 0 | ||
| return [:] | ||
| } | ||
|
|
||
| if let index = group.balances.firstIndex(where: { $0.id == memberId }) { | ||
| return group.balances[index].balance | ||
| guard let memberBalance = group.balances.first(where: { $0.id == memberId })?.balanceByCurrency else { | ||
| LogE("GroupSettingViewModel: \(#function) Member's balance not found from balances.") | ||
| return [:] | ||
| } | ||
|
|
||
| return 0 | ||
| var filteredBalances: [String: Double] = [:] | ||
| for (currency, balanceInfo) in memberBalance { | ||
| if balanceInfo.balance == 0 { continue } | ||
| filteredBalances[currency] = balanceInfo.balance | ||
| } | ||
|
|
||
| if filteredBalances.isEmpty { // If no non-zero balances, fallback to original data | ||
| return memberBalance.mapValues { $0.balance } | ||
| } | ||
|
|
||
| return [:] |
There was a problem hiding this comment.
Fix the incorrect return statement that makes the method ineffective.
The method always returns an empty dictionary on line 100, making all the filtering logic ineffective.
func getMembersBalance(memberId: String) -> [String: Double] {
guard let group else {
LogE("GroupSettingViewModel: \(#function) group not found.")
return [:]
}
guard let memberBalance = group.balances.first(where: { $0.id == memberId })?.balanceByCurrency else {
LogE("GroupSettingViewModel: \(#function) Member's balance not found from balances.")
return [:]
}
var filteredBalances: [String: Double] = [:]
for (currency, balanceInfo) in memberBalance {
if balanceInfo.balance == 0 { continue }
filteredBalances[currency] = balanceInfo.balance
}
if filteredBalances.isEmpty { // If no non-zero balances, fallback to original data
return memberBalance.mapValues { $0.balance }
}
- return [:]
+ return filteredBalances
}Consider adding currency validation.
The method should validate currency codes to ensure they match supported currencies.
+ @Inject private var currencyValidator: CurrencyValidator
+
func getMembersBalance(memberId: String) -> [String: Double] {
// ... existing guards ...
var filteredBalances: [String: Double] = [:]
for (currency, balanceInfo) in memberBalance {
+ guard currencyValidator.isValid(currency) else {
+ LogE("Invalid currency code: \(currency)")
+ continue
+ }
if balanceInfo.balance == 0 { continue }
filteredBalances[currency] = balanceInfo.balance
}
// ... rest of the method ...
}Optimize the performance by avoiding unnecessary iteration.
The method iterates through balances to filter zero values but then potentially discards the result to return the original data.
func getMembersBalance(memberId: String) -> [String: Double] {
// ... existing guards ...
+ // Return early if all balances are zero
+ if memberBalance.allSatisfy({ $0.value.balance == 0 }) {
+ return [:]
+ }
+
var filteredBalances: [String: Double] = [:]
for (currency, balanceInfo) in memberBalance {
if balanceInfo.balance == 0 { continue }
filteredBalances[currency] = balanceInfo.balance
}
-
- if filteredBalances.isEmpty { // If no non-zero balances, fallback to original data
- return memberBalance.mapValues { $0.balance }
- }
return filteredBalances
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getMembersBalance(memberId: String) -> [String: Double] { | |
| guard let group else { | |
| LogE("GroupSettingViewModel: \(#function) group not found.") | |
| return 0 | |
| return [:] | |
| } | |
| if let index = group.balances.firstIndex(where: { $0.id == memberId }) { | |
| return group.balances[index].balance | |
| guard let memberBalance = group.balances.first(where: { $0.id == memberId })?.balanceByCurrency else { | |
| LogE("GroupSettingViewModel: \(#function) Member's balance not found from balances.") | |
| return [:] | |
| } | |
| return 0 | |
| var filteredBalances: [String: Double] = [:] | |
| for (currency, balanceInfo) in memberBalance { | |
| if balanceInfo.balance == 0 { continue } | |
| filteredBalances[currency] = balanceInfo.balance | |
| } | |
| if filteredBalances.isEmpty { // If no non-zero balances, fallback to original data | |
| return memberBalance.mapValues { $0.balance } | |
| } | |
| return [:] | |
| func getMembersBalance(memberId: String) -> [String: Double] { | |
| guard let group else { | |
| LogE("GroupSettingViewModel: \(#function) group not found.") | |
| return [:] | |
| } | |
| guard let memberBalance = group.balances.first(where: { $0.id == memberId })?.balanceByCurrency else { | |
| LogE("GroupSettingViewModel: \(#function) Member's balance not found from balances.") | |
| return [:] | |
| } | |
| // Return early if all balances are zero | |
| if memberBalance.allSatisfy({ $0.value.balance == 0 }) { | |
| return [:] | |
| } | |
| var filteredBalances: [String: Double] = [:] | |
| for (currency, balanceInfo) in memberBalance { | |
| if balanceInfo.balance == 0 { continue } | |
| filteredBalances[currency] = balanceInfo.balance | |
| } | |
| return filteredBalances | |
| } |
| let currencySymbol = Currency.getCurrencyFromCode(currency).symbol | ||
| Text("\(currencySymbol) \(amount.formattedCurrency)") |
There was a problem hiding this comment.
Add error handling for currency symbol retrieval.
The current implementation assumes the currency code will always be valid. Add error handling to gracefully handle invalid currency codes.
-let currencySymbol = Currency.getCurrencyFromCode(currency).symbol
+let currencySymbol = Currency.getCurrencyFromCode(currency)?.symbol ?? currency📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let currencySymbol = Currency.getCurrencyFromCode(currency).symbol | |
| Text("\(currencySymbol) \(amount.formattedCurrency)") | |
| let currencySymbol = Currency.getCurrencyFromCode(currency)?.symbol ?? currency | |
| Text("\(currencySymbol) \(amount.formattedCurrency)") |
| if let firstBalance = balance.first { | ||
| let currency = firstBalance.key | ||
| let amount = firstBalance.value | ||
| let isBorrowed = amount < 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle all balances instead of just the first one.
Currently, only the first balance entry is displayed, which could hide other currency balances from the user. Consider showing all balances or implementing a way to toggle between different currencies.
-if let firstBalance = balance.first {
+// Option 1: Show all balances in a list
+ForEach(balance.sorted(by: { $0.key < $1.key }), id: \.key) { currency, amount in
+ BalanceView(currency: currency, amount: amount)
+}
+
+// Option 2: Add currency selector if multiple currencies exist
+if balance.count > 1 {
+ // Add picker or dropdown to select currency
+}Committable suggestion skipped: line range outside the PR's diff.
| let groupBalance = group.balances.first(where: { $0.id == user.id })?.balanceByCurrency ?? [:] | ||
| return groupBalance["INR"]?.totalSummary.filter { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract currency logic and optimize year filtering.
Similar to the previous issue, this code segment hard-codes "INR" currency and has potential null safety concerns. Additionally, there's duplicated currency access logic.
Consider extracting the currency logic into a reusable method:
+ private func getTotalSummaryForCurrency(_ currency: String, userId: String) -> [GroupTotalSummary]? {
+ group.balances.first(where: { $0.id == userId })?.balanceByCurrency[currency]?.totalSummary
+ }
+
private func getTotalSummaryForCurrentYear() -> [GroupTotalSummary] {
guard let user = preference.user, let group else {
viewState = .initial
return []
}
let currentYear = Calendar.current.component(.year, from: Date())
- let groupBalance = group.balances.first(where: { $0.id == user.id })?.balanceByCurrency ?? [:]
- return groupBalance["INR"]?.totalSummary.filter {
+ guard let defaultCurrency = preference.defaultCurrency else {
+ LogE("Default currency not set")
+ return []
+ }
+ return getTotalSummaryForCurrency(defaultCurrency, userId: user.id)?.filter {
$0.year == currentYear
} ?? []
}This refactoring:
- Removes the hard-coded currency
- Extracts reusable currency logic
- Improves code organization
- Maintains null safety
Committable suggestion skipped: line range outside the PR's diff.
| memberBalance[index].balanceByCurrency[currency]?.balance += newSplitAmount | ||
|
|
||
| // Update the corresponding total summary if it exists for the expense date | ||
| if var totalSummary = getLatestSummaryFrom(totalSummary: memberBalance[index].totalSummary, date: expenseDate)?.summary, | ||
| let summaryIndex = getLatestSummaryIndex(totalSummary: memberBalance[index].totalSummary, date: expenseDate) { | ||
| let groupTotalSummary = memberBalance[index].balanceByCurrency[currency]?.totalSummary ?? [] | ||
| if var totalSummary = getLatestSummaryFrom(totalSummary: groupTotalSummary, date: expenseDate)?.summary, |
There was a problem hiding this comment.
Ensure balanceByCurrency entries are initialized before updating
When updating balances with memberBalance[index].balanceByCurrency[currency]?.balance += ..., if the currency key does not exist in balanceByCurrency, the optional chaining ? will cause the operation to be skipped, leaving the balance unmodified. This can lead to incorrect balance calculations.
Apply this diff to safely initialize balanceByCurrency[currency] before updating:
if memberBalance[index].balanceByCurrency[currency] == nil {
memberBalance[index].balanceByCurrency[currency] = GroupCurrencyBalance(balance: 0.0, totalSummary: [])
}
memberBalance[index].balanceByCurrency[currency]?.balance += newSplitAmountRepeat this pattern in other instances where balances are updated:
- When updating summaries or other properties within
balanceByCurrency[currency]?, ensure the entry exists or safely handle the nil case.
Also applies to: 261-265, 286-288, 298-300, 342-346, 367-369, 379-381
| creditors[i].1 = round(credAmt * 100) / 100 | ||
| debtors[j].1 = round(debtAmt * 100) / 100 | ||
|
|
||
| // Move the index forward if someone's balance is settled | ||
| if creditors[i].1 == 0 { i += 1 } | ||
| if debtors[j].1 == 0 { j += 1 } | ||
| // Move the index forward if someone's balance is settled | ||
| if creditors[i].1 == 0 { i += 1 } | ||
| if debtors[j].1 == 0 { j += 1 } |
There was a problem hiding this comment.
Avoid direct comparison of floating-point numbers to zero
Comparing floating-point numbers directly to zero can lead to precision issues due to the nature of floating-point arithmetic. Instead of checking if the amounts are exactly zero after rounding, consider using an epsilon value to determine if the amounts are effectively zero.
Apply this diff to improve the precision of your comparisons:
- if creditors[i].1 == 0 { i += 1 }
- if debtors[j].1 == 0 { j += 1 }
+ let epsilon = 1e-10
+ if abs(creditors[i].1) < epsilon { i += 1 }
+ if abs(debtors[j].1) < epsilon { j += 1 }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| creditors[i].1 = round(credAmt * 100) / 100 | |
| debtors[j].1 = round(debtAmt * 100) / 100 | |
| // Move the index forward if someone's balance is settled | |
| if creditors[i].1 == 0 { i += 1 } | |
| if debtors[j].1 == 0 { j += 1 } | |
| // Move the index forward if someone's balance is settled | |
| if creditors[i].1 == 0 { i += 1 } | |
| if debtors[j].1 == 0 { j += 1 } | |
| creditors[i].1 = round(credAmt * 100) / 100 | |
| debtors[j].1 = round(debtAmt * 100) / 100 | |
| // Move the index forward if someone's balance is settled | |
| let epsilon = 1e-10 | |
| if abs(creditors[i].1) < epsilon { i += 1 } | |
| if abs(debtors[j].1) < epsilon { i += 1 } |
| @@ -86,7 +86,7 @@ private struct GroupListCellView: View { | |||
| self.isLastGroup = isLastGroup | |||
| self.group = group | |||
| self.viewModel = viewModel | |||
| self._showInfo = State(initialValue: isFirstGroup && group.userBalance != 0) | |||
| self._showInfo = State(initialValue: isFirstGroup && group.userBalance.values.reduce(0, +) != 0) | |||
There was a problem hiding this comment.
Avoid using mutable state in init with @State properties
Using @State properties in init can lead to unexpected behavior because @State is meant to be initialized in the body of the struct. Directly setting _showInfo in the initializer is not recommended.
Refactor to initialize showInfo within the body:
-init(isFirstGroup: Bool, isLastGroup: Bool = false, group: GroupInformation, viewModel: GroupListViewModel) {
self.isFirstGroup = isFirstGroup
self.isLastGroup = isLastGroup
self.group = group
self.viewModel = viewModel
self._showInfo = State(initialValue: isFirstGroup && group.userBalance.values.reduce(0, +) != 0)
}
+var body: some View {
+ @State var showInfo: Bool = isFirstGroup && group.userBalance.values.reduce(0, +) != 0
+
+ // Rest of the body...
+}Committable suggestion skipped: line range outside the PR's diff.
| Group { | ||
| Text("\(owedMemberName.capitalized) \(owesText.localized) ") | ||
| + Text(amount.formattedCurrency) | ||
| .foregroundColor(hasDue ? errorColor : successColor) | ||
| + Text(" in \(currency.localized) to \(owesMemberName)") | ||
| } |
There was a problem hiding this comment.
Incorrect amount formatting in balance display
The amount displayed might not include proper currency formatting, and the currency code is appended directly in the text.
Apply this diff to format the amount with the currency symbol:
Group {
Text("\(owedMemberName.capitalized) \(owesText.localized) ")
- + Text(amount.formattedCurrency)
+ + Text("\(Currency.getCurrencyFromCode(currency).symbol)\(amount.formattedCurrency)")
.foregroundColor(hasDue ? errorColor : successColor)
- + Text(" in \(currency.localized) to \(owesMemberName)")
+ + Text(" to \(owesMemberName)")
}Committable suggestion skipped: line range outside the PR's diff.
| return searchedGroup.isEmpty | ||
| ? combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 } | ||
| : combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 && | ||
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | ||
| case .unsettled: | ||
| return searchedGroup.isEmpty ? combinedGroups.filter { $0.userBalance != 0 } : combinedGroups.filter { $0.userBalance != 0 && | ||
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | ||
| return searchedGroup.isEmpty | ||
| ? combinedGroups.filter { $0.userBalance.values.reduce(0, +) != 0 } | ||
| : combinedGroups.filter { $0.userBalance.values.reduce(0, +) != 0 && | ||
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } |
There was a problem hiding this comment.
Critical: Avoid direct summation of different currencies
The current implementation adds balances across different currencies without conversion, which could lead to incorrect results. For example, adding 100 USD and 100 EUR directly would give 200, ignoring their different values.
Consider:
- Converting all balances to a common currency before summation
- Or treating each currency separately in the filtering logic
Example approach:
- return searchedGroup.isEmpty
- ? combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 }
- : combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 &&
+ return searchedGroup.isEmpty
+ ? combinedGroups.filter { $0.userBalance.allSatisfy { $0.value == 0 } }
+ : combinedGroups.filter { $0.userBalance.allSatisfy { $0.value == 0 } &&📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return searchedGroup.isEmpty | |
| ? combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 } | |
| : combinedGroups.filter { $0.userBalance.values.reduce(0, +) == 0 && | |
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | |
| case .unsettled: | |
| return searchedGroup.isEmpty ? combinedGroups.filter { $0.userBalance != 0 } : combinedGroups.filter { $0.userBalance != 0 && | |
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | |
| return searchedGroup.isEmpty | |
| ? combinedGroups.filter { $0.userBalance.values.reduce(0, +) != 0 } | |
| : combinedGroups.filter { $0.userBalance.values.reduce(0, +) != 0 && | |
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | |
| return searchedGroup.isEmpty | |
| ? combinedGroups.filter { $0.userBalance.allSatisfy { $0.value == 0 } } | |
| : combinedGroups.filter { $0.userBalance.allSatisfy { $0.value == 0 } && | |
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } | |
| case .unsettled: | |
| return searchedGroup.isEmpty | |
| ? combinedGroups.filter { !$0.userBalance.allSatisfy { $0.value == 0 } } | |
| : combinedGroups.filter { !$0.userBalance.allSatisfy { $0.value == 0 } && | |
| $0.group.name.localizedCaseInsensitiveContains(searchedGroup) } |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Performance
These updates provide a more flexible and user-friendly approach to managing currencies within the Splito application.