Fix bugs #100
Fix bugs #100
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on improving the handling of currency and member balances. Key changes include refactoring the 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: 2
🧹 Nitpick comments (2)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (2)
Line range hint
4-7: Add documentation for complex functions.The file contains complex functions for expense and transaction calculations. Consider adding documentation that explains:
- The purpose and responsibilities of each function
- The expected input and output
- The business rules being implemented
- Any assumptions or edge cases being handled
Line range hint
447-450: Extract epsilon value as a named constant.The epsilon value
1e-10is used for floating-point comparison. Consider extracting it as a named constant at the file level to improve maintainability and document its purpose.Apply this diff:
+/// Threshold for considering floating-point values as effectively zero +private let BALANCE_EPSILON: Double = 1e-10 let epsilon = 1e-10 for i in 0..<memberBalance.count where abs(memberBalance[i].balanceByCurrency[currency]?.balance ?? 0) < epsilon { memberBalance[i].balanceByCurrency[currency]?.balance = 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift(4 hunks)
🧰 Additional context used
📓 Learnings (1)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/GroupExpenseListView.swift:258-258
Timestamp: 2025-01-21T11:37:22.459Z
Learning: In the Splito app, when checking if all balances are settled in a group, we use `values.reduce(0, +)` to sum up all currency amounts, as the business requirement is to check if the total balance nets to zero across all currencies.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (2)
173-176: LGTM! Improved variable naming.The renaming of
groupTotalSummarytogroupOldTotalSummaryenhances code clarity by explicitly indicating that this summary is for the old expense.
189-191: LGTM! Improved variable naming.The renaming of
groupTotalSummarytogroupNewTotalSummaryenhances code clarity by explicitly indicating that this summary is for the new expense.
| @@ -383,7 +381,7 @@ public func getUpdatedMemberBalanceFor(transaction: Transactions, group: Groups, | |||
|
|
|||
| // Handle receiver role switch: Update the old receiver's balance and summary | |||
| if let oldReceiverIndex = memberBalance.firstIndex(where: { $0.id == oldReceiverId }) { | |||
| memberBalance[oldReceiverIndex].balanceByCurrency[currency]?.balance += oldAmount | |||
| memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency]?.balance += oldAmount | |||
There was a problem hiding this comment.
Add currency initialization check before updating balance.
The balance update could crash if the currency entry doesn't exist in balanceByCurrency. Add a currency initialization check similar to the one in the Add case.
Apply this diff to fix the potential crash:
if let oldReceiverIndex = memberBalance.firstIndex(where: { $0.id == oldReceiverId }) {
+ if memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] == nil {
+ memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: [])
+ }
memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency]?.balance += oldAmount📝 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.
| memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency]?.balance += oldAmount | |
| if memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] == nil { | |
| memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: []) | |
| } | |
| memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency]?.balance += oldAmount |
| @@ -297,7 +295,7 @@ | |||
|
|
|||
| // Handle payer role switch: Update the old payer's balance and summary | |||
| if let oldPayerIndex = memberBalance.firstIndex(where: { $0.id == oldPayerId }) { | |||
| memberBalance[oldPayerIndex].balanceByCurrency[currency]?.balance -= oldAmount | |||
| memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency]?.balance -= oldAmount | |||
There was a problem hiding this comment.
Add currency initialization check before updating balance.
The balance update could crash if the currency entry doesn't exist in balanceByCurrency. Add a currency initialization check similar to the one in the Add case.
Apply this diff to fix the potential crash:
if let oldPayerIndex = memberBalance.firstIndex(where: { $0.id == oldPayerId }) {
+ if memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] == nil {
+ memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: [])
+ }
memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency]?.balance -= oldAmount📝 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.
| memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency]?.balance -= oldAmount | |
| if let oldPayerIndex = memberBalance.firstIndex(where: { $0.id == oldPayerId }) { | |
| if memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] == nil { | |
| memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: []) | |
| } | |
| memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency]?.balance -= oldAmount |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Data/Data/Model/Currency/Currency.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(1 hunks)
🧰 Additional context used
📓 Learnings (2)
Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift (1)
Learnt from: cp-nirali-s
PR: canopas/splito#99
File: Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift:151-165
Timestamp: 2025-01-17T11:04:06.719Z
Learning: The Splito app currently only supports INR (Indian Rupee) as the currency, which is why it's hardcoded in various places including the CSV report generation.
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (3)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift:128-128
Timestamp: 2025-01-21T11:47:01.631Z
Learning: In the Splito app, when checking if any balance exists across multiple currencies, both summing values with `reduce(0, +)` and using `allSatisfy` to check for non-zero values work correctly, even though summing across currencies is generally not recommended.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/GroupExpenseListView.swift:258-258
Timestamp: 2025-01-21T11:37:22.459Z
Learning: In the Splito app, when checking if all balances are settled in a group, we use `values.reduce(0, +)` to sum up all currency amounts, as the business requirement is to check if the total balance nets to zero across all currencies.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:307-308
Timestamp: 2025-01-21T11:31:00.998Z
Learning: When checking if all balances are settled in a multi-currency system, use `allSatisfy` to verify each currency balance individually instead of summing them with `reduce`, as direct addition of different currencies can lead to incorrect results.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift (1)
Line range hint
95-144: Well-structured balance display logic with proper multi-currency handling!The implementation correctly uses
allSatisfyfor checking zero balances across multiple currencies, which is the recommended approach based on past feedback. The code also nicely separates positive and negative amounts for clear presentation.Data/Data/Model/Currency/Currency.swift (1)
20-20:⚠️ Potential issueUse
Locale.current.regionCodeinstead ofLocale.current.region?.identifierThe
Localestruct provides aregionCodeproperty to obtain the current region code. UsingLocale.current.region?.identifiermay not retrieve the desired region and could lead to incorrect currency selection.Apply this diff to correct the issue:
- let currentLocal = Locale.current.region?.identifier + let currentLocal = Locale.current.regionCodeLikely invalid or redundant comment.
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift (1)
22-24: LGTM!The change to use
Currency.defaultCurrencyaligns with the updatedCurrencystruct. The initialization and data recalculation are appropriately handled.Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
104-104: LGTM!Updating to
Currency.defaultCurrency.codeensures consistency with the revisedCurrencyimplementation.Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift (2)
51-51: Verify currency handling implementation.Based on the retrieved learning that the app currently only supports INR currency, the switch to using
Currency.defaultCurrency.codemight introduce inconsistencies if the default currency is not INR. Please ensure that:
- The
defaultCurrencyproperty always returns INR- Or the app has been updated to support multiple currencies
Also applies to: 81-81
✅ Verification successful
Currency handling implementation is correctly implemented
The switch to
Currency.defaultCurrency.codeis safe because:
- INR is guaranteed as the final fallback through
defaultLocalCurrency- The implementation supports graceful degradation through multiple fallback levels
- The change makes the code more maintainable for future multi-currency support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check currency handling implementation # Search for currency-related code echo "Searching for Currency struct definition and usage..." ast-grep --pattern 'struct Currency { $$$ }' echo "Searching for currency validation or conversion logic..." rg -l "currencyCode|defaultCurrency|Currency\."Length of output: 2502
Script:
#!/bin/bash # Check Currency implementation and currency picker echo "=== Currency.swift implementation ===" cat "Data/Data/Model/Currency/Currency.swift" echo -e "\n=== CurrencyPickerView implementation ===" cat "Splito/UI/Home/Expense/CurrencyPickerView.swift" echo -e "\n=== Searching for currency validation patterns ===" rg "Currency\." -A 3 "Data/Data/Model/Currency/Currency.swift"Length of output: 6217
Line range hint
351-354: Verify balance calculation logic across transaction and expense contexts.The
getUpdatedMemberBalanceForfunction is used with different parameter types (transaction vs expense) in different contexts. Please ensure that:
- The balance calculation logic handles both transaction and expense scenarios correctly
- The function properly handles currency conversions if transactions and expenses can have different currencies
✅ Verification successful
Balance calculation logic is properly implemented for both contexts
The verification shows that:
- Both expense and transaction implementations correctly maintain currency-specific balances
- Each context uses appropriate calculation logic:
- Expenses use split calculations per member
- Transactions handle direct transfers between payer and receiver
- Currency handling is consistent with proper fallback to default currency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check balance calculation implementation # Search for balance calculation function echo "Searching for getUpdatedMemberBalanceFor function implementation..." ast-grep --pattern 'func getUpdatedMemberBalanceFor($$$) { $$$ }' # Search for its usage echo "Searching for balance calculation usage..." rg "getUpdatedMemberBalanceFor"Length of output: 1904
Script:
#!/bin/bash # Get the implementation details of both balance calculation functions echo "Expense balance calculation implementation:" rg -A 20 "func getUpdatedMemberBalanceFor\(expense:" "Splito/UI/Home/Groups/CalculateExpensesFunctions.swift" echo -e "\nTransaction balance calculation implementation:" rg -A 20 "func getUpdatedMemberBalanceFor\(transaction:" "Splito/UI/Home/Groups/CalculateExpensesFunctions.swift"Length of output: 2656
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
60-60: Verify currency handling during expense updates.The switch to using
Currency.defaultCurrencyneeds careful handling when:
- Updating existing expenses that might have different currencies
- Converting amounts between currencies during group member balance updates
- Handling the currency selection in the UI (line 89 shows currency can be selected from group's default currency)
Please ensure that:
- Currency conversions are properly implemented
- The UI properly reflects currency changes
- Group balances are correctly updated when expenses have different currencies
Also applies to: 89-89
Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Splito/UI/Home/Groups/GroupListWithDetailView.swift (1)
116-116: Consider using epsilon comparison for floating-point equality checks.When comparing floating-point values with zero, it's safer to use an epsilon comparison to handle potential precision issues.
- if group.userBalance.allSatisfy({ $0.value == 0 }) { + if group.userBalance.allSatisfy({ abs($0.value) < Double.ulpOfOne }) {Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
Line range hint
254-254: Consider using epsilon comparison for floating-point equality checks.When comparing the amount with zero, use an epsilon comparison to handle floating-point precision issues.
- if amount == 0 { + if abs(amount) < Double.ulpOfOne {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Data/Data/Model/AppUser.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift(1 hunks)Splito/UI/Home/Groups/GroupListWithDetailView.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Data/Data/Model/AppUser.swift
🧰 Additional context used
📓 Learnings (2)
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/Group Setting/GroupSettingViewModel.swift:171-171
Timestamp: 2025-01-21T11:16:21.094Z
Learning: In financial calculations involving floating-point arithmetic, use epsilon comparison (e.g., `abs(value) < epsilon`) instead of direct equality comparison (`== 0`) to handle precision issues. This is particularly important for balance checks in group management features.
Splito/UI/Home/Groups/GroupListWithDetailView.swift (4)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/GroupExpenseListView.swift:258-258
Timestamp: 2025-01-21T11:37:22.459Z
Learning: In the Splito app, when checking if all balances are settled in a group, we use `values.reduce(0, +)` to sum up all currency amounts, as the business requirement is to check if the total balance nets to zero across all currencies.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:52-60
Timestamp: 2025-01-21T11:29:04.483Z
Learning: In multi-currency applications, avoid directly summing balances of different currencies without conversion. Each currency balance should be handled separately to prevent incorrect calculations.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesViewModel.swift:128-128
Timestamp: 2025-01-21T11:47:01.631Z
Learning: In the Splito app, when checking if any balance exists across multiple currencies, both summing values with `reduce(0, +)` and using `allSatisfy` to check for non-zero values work correctly, even though summing across currencies is generally not recommended.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:307-308
Timestamp: 2025-01-21T11:31:00.998Z
Learning: When checking if all balances are settled in a multi-currency system, use `allSatisfy` to verify each currency balance individually instead of summing them with `reduce`, as direct addition of different currencies can lead to incorrect results.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Splito/UI/Home/Groups/GroupListWithDetailView.swift (1)
109-112: LGTM! Improved balance selection logic.The new implementation correctly prioritizes the default currency while ensuring only non-zero balances are considered. This aligns with best practices for handling multi-currency balances.
Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift (1)
250-250: LGTM! Consistent balance handling approach.The implementation aligns with the app's approach to handling multi-currency balances by prioritizing non-zero balances.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift (1)
163-163: LGTM! Consider documenting the boolean parameter.The change to use
formattedCurrency(_:_:)aligns with the app-wide standardization of currency formatting. However, the purpose of the boolean parametertruecould be clearer.Consider adding a named parameter or documenting the boolean's purpose:
-Text(amount.formattedCurrency(currencySymbol, true)) +Text(amount.formattedCurrency(currencySymbol, includeSign: true))Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift (1)
50-51: Add a comment explaining the currency formatting choice.To improve code maintainability, consider adding a comment explaining why currency signs are omitted here while the symbol is shown separately in member cells. This will help future maintainers understand the design decision.
let currency = viewModel.amountCurrency +// Currency symbol is shown separately in member cells to maintain consistent layout BottomInfoCardView(title: "\(viewModel.totalAmount.formattedCurrency(currency)) of \(viewModel.expenseAmount.formattedCurrency(currency))", value: "\((viewModel.expenseAmount - viewModel.totalAmount).formattedCurrency(currency)) left")Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsTabView.swift (1)
Line range hint
291-297: Add input validation to prevent invalid amounts.The
updateValuefunction should include validation to:
- Prevent negative values
- Handle invalid decimal inputs gracefully
- Enforce appropriate precision limits based on the split type
Consider updating the implementation:
private func updateValue(from newValue: String) { - if let doubleValue = Double(newValue) { + guard let doubleValue = Double(newValue), doubleValue >= 0 else { + value = 0 + onChange(value) + return + } + + // Round to appropriate precision based on split type + let roundedValue = (suffixText == "%" || suffixText == "shares") + ? round(doubleValue) + : round(doubleValue * 100) / 100 + value = doubleValue + if value != roundedValue { + textValue = String(format: formatString, roundedValue) + } onChange(value) - } else { - value = 0 - } - onChange(value) }Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift (1)
252-254: Consider accessibility implications of currency formatting.When displaying currency amounts, ensure that screen readers can properly announce both the amount and currency. SwiftUI's accessibility system should be configured to handle the new currency formatting.
Add an accessibility label that includes both the amount and currency:
Text(amount?.formattedCurrency(currency) ?? 0.0.formattedCurrency(currency)) .font(.Header2()) .foregroundStyle(primaryText) + .accessibilityLabel(Text("Amount \(amount?.formattedCurrency(currency) ?? "") \(currency ?? "")"))Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift (2)
82-82: Consider enhancing the alert message for better user guidance.While the currency formatting change aligns with the app-wide standardization, the alert message could be more helpful by:
- Showing the current total entered
- Providing guidance on how to fix the discrepancy
- message: "The payment values do not add up to the total cost of \(expenseAmount.formattedCurrency(amountCurrency)). You are \(amountDescription) by \(differenceAmount.formattedCurrency(amountCurrency)).") + message: """ + The entered total of \(totalAmount.formattedCurrency(amountCurrency)) does not match the expense amount of \(expenseAmount.formattedCurrency(amountCurrency)). + You are \(amountDescription) by \(differenceAmount.formattedCurrency(amountCurrency)). + Please adjust the individual payment amounts to match the total expense. + """)
Line range hint
76-85: Consider adding real-time validation to prevent amount mismatches.Instead of waiting for the user to tap "Done" to discover mismatches, consider adding real-time validation as amounts are entered. This would provide immediate feedback and a better user experience.
func updateAmount(for memberId: String, amount: Double) { membersAmount[memberId] = amount totalAmount = membersAmount.values.reduce(0, +) + validateTotalAmount() } +private func validateTotalAmount() { + if totalAmount > expenseAmount { + // Optionally show warning UI that total exceeds expense + } else if totalAmount < expenseAmount { + // Optionally show remaining amount to be allocated + } +}Splito/UI/Home/ActivityLog/ActivityLogView.swift (1)
187-192: Consider extracting string formatting logic.The currency and description formatting logic could be moved to a dedicated formatter type to improve maintainability and testability. This would also make it easier to maintain consistency across the app.
Example structure:
struct ActivityLogFormatter { static func formatAmount(_ amount: Double, currency: Currency, for type: ActivityType) -> String { guard amount != 0 else { return "You do not owe anything" } switch type { case .expenseAdded, .expenseUpdated, .expenseDeleted, .expenseRestored: let action = amount > 0 ? "get back" : "owe" return "You \(action) \(amount.formattedCurrency(currency))" case .transactionAdded, .transactionUpdated, .transactionDeleted, .transactionRestored: let action = amount > 0 ? "paid" : "received" return "You \(action) \(amount.formattedCurrency(currency))" default: return "" } } }Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (1)
214-216: LGTM! Consider adding visual distinction between paid and owed amounts.The simplified currency formatting improves readability. To further enhance the user experience, consider using different text colors or styles to visually distinguish between paid and owed amounts.
Example enhancement:
-Text("\(memberName.localized) paid \(paidAmount.formattedCurrency(currencyCode)) and \(owes.localized) \(splitAmount)") +Text("\(memberName.localized) paid ") + +Text(paidAmount.formattedCurrency(currencyCode)).foregroundColor(.green) + +Text(" and \(owes.localized) ") + +Text(splitAmount).foregroundColor(.red)Splito/UI/Home/Groups/Group/Group Options/Settlements/GroupTransactionListView.swift (1)
189-192: Enhance accessibility for transaction amounts.While the color coding (success/error) helps visually distinguish between paid and received amounts, we should enhance accessibility by:
- Adding appropriate accessibility labels that indicate whether an amount was paid or received
- Using semantic indicators (like "+" or "-") in addition to colors
Consider adding an accessibility modifier:
Text("\(transactionWithUser.transaction.amount.formattedCurrency(currencyCode))") .font(.subTitle2()) .foregroundStyle(transactionWithUser.payer?.id == preference.user?.id ? successColor : transactionWithUser.receiver?.id == preference.user?.id ? errorColor : primaryText) + .accessibilityLabel(createAccessibilityLabel(amount: transactionWithUser.transaction.amount, currencyCode: currencyCode, isPayer: transactionWithUser.payer?.id == preference.user?.id)) // Add this helper function private func createAccessibilityLabel(amount: Double, currencyCode: String, isPayer: Bool) -> String { let formattedAmount = amount.formattedCurrency(currencyCode) return isPayer ? "Paid \(formattedAmount)" : "Received \(formattedAmount)" }Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (1)
380-380: Consider showing signs for owed amounts.While the direction is indicated by text and color, showing signs for owed amounts can improve clarity, especially in complex multi-currency scenarios. This aligns with the learning about handling multi-currency balances separately.
Apply this diff:
-amount.formattedCurrency(currency) +amount.formattedCurrency(currency, true)Also applies to: 388-388
Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsView.swift (1)
80-81: Consider refactoring for better readability.While the currency formatting changes are good, the string interpolation with multiple currency values and arithmetic operations could be simplified for better readability.
Consider this refactor:
- BottomInfoCardView(title: "\(viewModel.totalFixedAmount.formattedCurrency(selectedCurrency)) of \(viewModel.expenseAmount.formattedCurrency(selectedCurrency))", - value: "\((viewModel.expenseAmount - viewModel.totalFixedAmount).formattedCurrency(selectedCurrency)) left") + let totalFixed = viewModel.totalFixedAmount.formattedCurrency(selectedCurrency) + let total = viewModel.expenseAmount.formattedCurrency(selectedCurrency) + let remaining = (viewModel.expenseAmount - viewModel.totalFixedAmount).formattedCurrency(selectedCurrency) + + BottomInfoCardView(title: "\(totalFixed) of \(total)", + value: "\(remaining) left")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Data/Data/Extension/Double+Extension.swift(2 hunks)Data/Data/Model/Expense.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(2 hunks)Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsTabView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsViewModel.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift(5 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/GroupTransactionListView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(3 hunks)Splito/UI/Home/Groups/GroupListWithDetailView.swift(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
- Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsViewModel.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Splito/UI/Home/Groups/Group/Group Setting/GroupSettingView.swift
- Splito/UI/Home/Groups/GroupListWithDetailView.swift
- Splito/UI/Home/Groups/Group/Group Options/Balances/GroupBalancesView.swift
- Splito/UI/Home/Expense/AddExpenseViewModel.swift
🧰 Additional context used
📓 Learnings (5)
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpView.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:52-60
Timestamp: 2025-01-21T11:29:04.483Z
Learning: In multi-currency applications, avoid directly summing balances of different currencies without conversion. Each currency balance should be handled separately to prevent incorrect calculations.
Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsTabView.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.
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift (2)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift:97-112
Timestamp: 2025-01-21T11:39:50.602Z
Learning: In the Splito iOS app, using Decimal(0) for currency calculations is not possible due to environment limitations, and Double should be used instead.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:52-60
Timestamp: 2025-01-21T11:29:04.483Z
Learning: In multi-currency applications, avoid directly summing balances of different currencies without conversion. Each currency balance should be handled separately to prevent incorrect calculations.
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.
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (2)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:52-60
Timestamp: 2025-01-21T11:29:04.483Z
Learning: In multi-currency applications, avoid directly summing balances of different currencies without conversion. Each currency balance should be handled separately to prevent incorrect calculations.
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/Group/GroupExpenseListView.swift:258-258
Timestamp: 2025-01-21T11:37:22.459Z
Learning: In the Splito app, when checking if all balances are settled in a group, we use `values.reduce(0, +)` to sum up all currency amounts, as the business requirement is to check if the total balance nets to zero across all currencies.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
Splito/UI/Home/Expense/Detail Selection/Payer/ChooseMultiplePayerView.swift (1)
50-51: Verify currency visibility in the UI.The change from
formattedCurrencyWithSigntoformattedCurrencyremoves the currency signs from the formatted amounts. While this change is consistent with other views, please ensure that the currency is clearly visible elsewhere in the UI to avoid confusion.Consider these aspects:
- Is the currency shown in the navigation bar or section header?
- Is the currency symbol visible in the individual member cells? (I notice it's passed as
currencySymboltoMemberCellView)- Does this format align with iOS currency formatting guidelines for your target regions?
Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsTabView.swift (1)
260-260: LGTM! Currency formatting change aligns with app-wide standardization.The change from
formattedCurrencyWithSigntoformattedCurrencyis consistent with the app-wide initiative to standardize currency formatting and follows the established preference for simpler amount formatting.Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailView.swift (1)
252-252: Verify the currency display format change.The change from
formattedCurrencyWithSigntoformattedCurrencyremoves the currency sign from the display. While this aligns with the standardization of currency formatting across the app, we should ensure this doesn't impact the user's ability to identify the currency of the transaction.Consider adding a currency indicator elsewhere in the UI (e.g., in a subtitle or header) to maintain clarity about the transaction's currency type, especially in a multi-currency environment.
Splito/UI/Home/ActivityLog/ActivityLogView.swift (2)
188-188: LGTM! Currency formatting change looks good.The change to use
formattedCurrencyinstead offormattedCurrencyWithSignaligns with the standardization of currency formatting across the app.
191-191: LGTM! Consistent currency formatting.The currency formatting change is consistent with the app-wide standardization and matches the formatting used in expense-related activities.
Data/Data/Model/Expense.swift (2)
Line range hint
1-200: Implementation demonstrates solid handling of currency calculations.The code shows excellent attention to detail in handling currency calculations:
- Proper decimal precision using
.rounded(to: 2)- Deterministic remainder distribution
- Clear separation of split calculation methods
79-79: Verify currency sign behavior in the new formatting method.The change from
formattedCurrencyWithSigntoformattedCurrencymight affect how currency signs are displayed. Please ensure this change:
- Maintains the expected currency sign behavior in the UI
- Aligns with the formatting requirements across all expense displays
Run this script to check for consistency in currency formatting across the codebase:
✅ Verification successful
Currency formatting change is safe and consistent
The new
formattedCurrencymethod maintains the same behavior as the previous implementation, with an added flexibility to show currency signs when needed through an optional parameter. The migration is complete with consistent usage across all UI components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of formattedCurrencyWithSign to ensure complete migration echo "Checking for any remaining usage of old formatting method..." rg "formattedCurrencyWithSign" # Search for all currency formatting methods to verify consistency echo "Checking all currency formatting methods for consistency..." rg "formattedCurrency\("Length of output: 6221
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (1)
214-214: Verify consistent currency formatting across the app.The change from
formattedCurrencyWithSigntoformattedCurrencyaligns with the standardization of currency formatting across the app. However, we should ensure this change maintains clarity in distinguishing between paid amounts and owed amounts in the UI.Let's verify the currency formatting consistency across the codebase:
Also applies to: 216-216
✅ Verification successful
Currency formatting is consistently implemented across the app
The codebase shows complete migration to
formattedCurrencywith consistent usage across all views and view models. The change aligns with the app-wide currency formatting standard.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of formattedCurrencyWithSign to ensure complete migration rg "formattedCurrencyWithSign" --type swift # Search for all currency formatting patterns to verify consistency rg "formattedCurrency\(" --type swift -A 1Length of output: 9425
Splito/UI/Home/Groups/Group/Group Options/Settlements/GroupTransactionListView.swift (1)
189-189: Verify consistent currency sign display across the app.The change from
formattedCurrencyWithSigntoformattedCurrencyalters how transaction amounts are displayed. While this change aligns with the broader effort to standardize currency formatting across the app, we should ensure that:
- The currency sign is still visible somewhere in the UI to avoid confusion
- This change maintains consistency with similar transaction displays in other views
Let's verify the currency formatting consistency across the app:
✅ Verification successful
Currency formatting is consistent across the app ✅
The change aligns with the app-wide currency formatting pattern. The
formattedCurrencymethod is consistently used across all transaction displays, and currency signs are appropriately handled through context or explicit parameter when needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of formattedCurrencyWithSign to ensure complete migration rg "formattedCurrencyWithSign" --type swift # Search for other currency formatting methods to verify consistency rg "formattedCurrency\(" --type swiftLength of output: 5987
Data/Data/Extension/Double+Extension.swift (1)
Line range hint
12-29: LGTM! The currency formatting implementation aligns with multi-currency requirements.The refactored implementation correctly:
- Avoids using NumberFormatter's currency style
- Makes sign display optional
- Handles currency symbols appropriately
Please add unit tests to verify the behavior with the new
showSignparameter. Would you like me to help generate the test cases?Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (3)
229-229: LGTM! Removed redundant sign display.The amount's direction is already clearly conveyed through the text ("you borrowed"/"you lent") and color (red/green), making the sign redundant.
352-352: LGTM! Correctly displays absolute spending amounts.Using
formattedCurrencywithout sign is appropriate here as:
- The amounts are already converted to absolute values
- The context ("Your monthly spending") makes it clear these are outgoing amounts
352-354: LGTM! Proper handling of multi-currency amounts.The implementation correctly maintains currency separation by:
- Keeping balances separate by currency
- Displaying each amount with its currency symbol
- Using " + " to join multiple currency amounts
Splito/UI/Home/Groups/Group/Group Options/Settle up/GroupSettleUpView.swift (2)
128-128: LGTM! Currency formatting change maintains clarity while improving consistency.The removal of the sign from currency formatting is well-compensated by:
- Explicit "you owe"/"owes you" text
- Color coding (red/green)
- Separate sign handling via
abs()
Line range hint
91-106: Verify multi-currency balance handling in sorted members list.Based on our learnings from PR #98, we should ensure that member balances in different currencies are handled appropriately. The current sorting is only based on member names, but when displaying balances of different currencies, additional consideration might be needed.
Let's verify the currency handling in the view model:
Splito/UI/Home/Expense/Detail Selection/Expense Split Option/ExpenseSplitOptionsView.swift (1)
74-74: LGTM! Verify consistent currency formatting across the app.The removal of currency sign makes the per-person split amount display cleaner, especially when combined with the "/person" suffix.
Let's verify that this currency formatting change is consistent across similar displays in the app:
✅ Verification successful
Currency formatting is consistent across the entire codebase
The verification confirms:
- All currency formatting uses the
formattedCurrencymethod consistently- The per-person amount display is unique to the split options view
- No instances of the old
formattedCurrencyWithSignremain🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of formattedCurrencyWithSign rg "formattedCurrencyWithSign" --type swiftLength of output: 43
Script:
#!/bin/bash # Search for currency formatting method calls rg "formatted[A-Za-z]*Currency" --type swift # Search for per-person amount displays rg "/person" --type swiftLength of output: 6176
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (2)
299-299:⚠️ Potential issueAdd currency initialization check before updating balance.
The balance update could crash if the currency entry doesn't exist in
balanceByCurrency. Add a currency initialization check similar to the one in the Add case.Apply this diff to fix the potential crash:
if let oldPayerIndex = memberBalance.firstIndex(where: { $0.id == oldPayerId }) { + if memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] == nil { + memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: []) + } memberBalance[oldPayerIndex].balanceByCurrency[oldCurrency]?.balance -= oldAmount
385-385:⚠️ Potential issueAdd currency initialization check before updating balance.
The balance update could crash if the currency entry doesn't exist in
balanceByCurrency. Add a currency initialization check similar to the one in the Add case.Apply this diff to fix the potential crash:
if let oldReceiverIndex = memberBalance.firstIndex(where: { $0.id == oldReceiverId }) { + if memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] == nil { + memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency] = GroupCurrencyBalance(balance: 0.0, totalSummary: []) + } memberBalance[oldReceiverIndex].balanceByCurrency[oldCurrency]?.balance += oldAmount
🧹 Nitpick comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
154-154: LGTM! Consider adding a clarifying comment.The filter operation to remove zero-value currencies from current month spending is a good improvement for UI clarity.
Consider adding a comment to explain why zero values are filtered out:
.mapValues { summaries in summaries.reduce(0) { $0 + $1.summary.totalShare } } - .filter({ $0.value != 0 }) + // Filter out currencies with no spending to keep the UI clean + .filter({ $0.value != 0 })Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (1)
Line range hint
1-500: Consider using decimal types for monetary calculations.The code currently uses floating-point arithmetic (Double) for monetary calculations. This can lead to precision issues in financial calculations. Consider using decimal types (e.g.,
Decimalin Swift) to ensure precise monetary calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift(3 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Splito/UI/Home/Groups/CalculateExpensesFunctions.swift (1)
169-192: LGTM! Improved variable naming and currency handling.The changes enhance code clarity by:
- Using more descriptive variable names (e.g.,
groupOldTotalSummary)- Properly handling old and new currencies
- Correctly calculating the split amounts and balance changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
BaseStyle/BaseStyle/CustomUI/Buttons/CapsuleButton.swift (1)
15-17: LGTM! Consider adding documentation.The changes improve the reusability of CapsuleButton by making the corner radius configurable. The implementation maintains backward compatibility and follows Swift best practices.
Consider adding documentation comments to describe the new
cornerRadiusparameter:public init(buttonName: String, isEnabled: Bool = true, paddingHr: CGFloat = 73, - paddingVr: CGFloat = 12, cornerRadius: Double = 12, onClick: (() -> Void)?) { + paddingVr: CGFloat = 12, cornerRadius: Double = 12, onClick: (() -> Void)?) { + /// - Parameter cornerRadius: The corner radius of the button. Defaults to 12.Also applies to: 20-21, 25-26, 44-44
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift (1)
110-123: Consider reducing currency code duplication.While the implementation is correct, there's repetition in passing the same
currencyCodeto eachGroupSummaryAmountView.Consider moving
currencyCodeto a view-level property or environment value:private struct GroupTotalSummaryView: View { @ObservedObject var viewModel: GroupTotalsViewModel + private var currencyCode: String { viewModel.selectedCurrency.code } var body: some View { VStack(spacing: 0) { if let summaryData = viewModel.summaryData { - let currencyCode = viewModel.selectedCurrency.code // Use self.currencyCode in the GroupSummaryAmountView initializations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
BaseStyle/BaseStyle/CustomUI/Buttons/CapsuleButton.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift(4 hunks)Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsViewModel.swift
🔇 Additional comments (2)
Splito/UI/Home/Groups/Group/Group Options/Totals/GroupTotalsView.swift (2)
34-37: Improved currency selection UI with better accessibility.The change from Text to CapsuleButton improves:
- User interaction affordance
- Touch target size
- Visual consistency with other buttons
153-155: Verify the currency formatting implementation.The code uses
formattedCurrency(_:Bool)for amount formatting. Please ensure this method properly handles:
- Zero amounts
- Negative amounts
- Different currency formats
- Large numbers
Run this script to verify the currency formatting implementation:
✅ Verification successful
Currency formatting implementation is properly implemented and used consistently
The
formattedCurrencymethod correctly handles:
- Zero amounts with appropriate styling
- Negative and positive amounts
- Multiple currency formats
- Large numbers with proper locale formatting
- Consistent decimal places
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for formattedCurrency implementation and its test cases rg -A 10 "formattedCurrency.*\(" --type swiftLength of output: 34736
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Data/Data/Extension/Double+Extension.swift (2)
24-24: Enhance the fallback formatting comment.The current comment could be more descriptive about when and why the fallback is used.
- amount = String(format: "%.2f", self.rounded()) // Fallback to a basic decimal format + amount = String(format: "%.2f", self.rounded()) // Fallback to basic decimal format when NumberFormatter fails
27-32: Optimize string formatting logic.The current implementation has some duplication in the return statements. Consider using string interpolation consistently and reducing the conditional branches.
- if showSign { - let sign = self < 0 ? "-" : "" - return currencySymbol.isEmpty ? "\(sign)\(amount)" : "\(sign)\(currencySymbol) \(amount)" - } else { - return currencySymbol.isEmpty ? amount : (currencySymbol + " " + amount) - } + let sign = (showSign && self < 0) ? "-" : "" + return currencySymbol.isEmpty ? "\(sign)\(amount)" : "\(sign)\(currencySymbol) \(amount)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Data/Data/Extension/Double+Extension.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(4 hunks)Splito/UI/Home/Groups/GroupListView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Groups/Group/GroupExpenseListView.swift
🧰 Additional context used
📓 Learnings (2)
Splito/UI/Home/Groups/GroupListView.swift (1)
Learnt from: cp-amisha-i
PR: canopas/splito#98
File: Splito/UI/Home/Groups/GroupListViewModel.swift:52-60
Timestamp: 2025-01-21T11:29:04.483Z
Learning: In multi-currency applications, avoid directly summing balances of different currencies without conversion. Each currency balance should be handled separately to prevent incorrect calculations.
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Data/Data/Extension/Double+Extension.swift (1)
12-18: LGTM! Improved currency formatting configuration.The changes align with our previous learning about avoiding .currency numberStyle for better multi-currency support. The explicit fraction digits configuration ensures consistent decimal places.
Splito/UI/Home/Groups/GroupListView.swift (3)
189-189: LGTM! Consistent currency formatting approach.Good use of the standardized
formattedCurrencymethod, maintaining consistency with the negative amount formatting above.
182-191: Great approach to multi-currency handling!The implementation correctly handles multiple currencies by:
- Keeping amounts separated by currency
- Formatting each currency individually
- Clearly presenting multiple currencies with " + " separator
This aligns well with best practices for multi-currency applications.
182-182: Verify negative amount handling in formattedCurrency.The change to use
formattedCurrencyimproves consistency in currency formatting. However, since this is handling negative amounts (owed money), please ensure theformattedCurrencyextension properly formats negative values.✅ Verification successful
formattedCurrency correctly handles negative amounts
The implementation properly formats negative values by preserving the sign while applying locale-aware currency formatting. This is verified by both the implementation and its consistent usage across balance displays in the app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for formattedCurrency implementation to verify negative amount handling ast-grep --pattern 'func formattedCurrency'Length of output: 45
Script:
#!/bin/bash # Broader search for formattedCurrency rg "formattedCurrency" -A 3 # Look for potential currency formatting utilities fd -e swift -e kt -e java "Currency|Money|Format" # Try finding extension declarations related to currency ast-grep --pattern 'extension $_ { $$$ formattedCurrency $$$ }'Length of output: 17442
Script:
#!/bin/bash # Get the complete implementation of formattedCurrency rg -A 15 "func formattedCurrency" "Data/Data/Extension/Double+Extension.swift"Length of output: 858
Summary by CodeRabbit
Bug Fixes
New Features
Refactor