Implement export group report support #99
Conversation
WalkthroughThis pull request introduces modifications across several repositories and view models, focusing on simplifying method signatures and enhancing data retrieval functionalities. Key changes include the removal of 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: 4
🧹 Nitpick comments (10)
Data/Data/Repository/CommentRepository.swift (2)
Line range hint
1-150: Consider adding export-related activity logging.Since this PR implements export report support, consider extending the
ActivityLogTypeenum and the activity logging functionality to track when users export or share data. This would help maintain an audit trail of data exports.Would you like me to provide an example implementation for export-related activity logging?
Line range hint
29-43: Consider enhancing error handling for export operations.The current implementation throws only the first error from parallel operations. For export functionality, consider:
- Aggregating all errors instead of throwing just the first one
- Allowing partial success when some operations fail
- Providing detailed error reporting to users
Example approach:
struct BatchResult { let successes: [String] // Successfully processed member IDs let failures: [(memberId: String, error: Error)] } private func addActivityLogsInParallel(...) async throws -> BatchResult { var successes: [String] = [] var failures: [(String, Error)] = [] await withTaskGroup(of: (String, Error?).self) { taskGroup in // ... existing task creation ... for await (memberId, error) in taskGroup { if let error { failures.append((memberId, error)) } else { successes.append(memberId) } } } return BatchResult(successes: successes, failures: failures) }Splito/UI/Home/Groups/Group/GroupHomeView.swift (3)
139-139: Document the completion handler's purpose.The Bool completion handler's purpose isn't immediately clear. Consider adding a documentation comment explaining when true/false should be returned and how it affects the share sheet display.
+ /// Handles the export action for the selected time range + /// - Parameters: + /// - option: The selected export time range + /// - completion: Callback indicating if the export was successful (true) or failed (false) let onExportTap: (ExportOptions, @escaping (Bool) -> Void) -> Void
159-168: Enhance accessibility for the export options dialog.While the implementation is clean, consider adding a descriptive title for screen readers instead of using an empty string.
- .confirmationDialog("", isPresented: $showExportOptions, titleVisibility: .hidden) { + .confirmationDialog("Select Export Time Range", isPresented: $showExportOptions, titleVisibility: .hidden) {
179-196: Improve enum documentation and localization.The enum implementation could benefit from the following improvements:
- Add documentation explaining the purpose and usage
- Remove unused Int raw value type
- Use a more structured localization approach
-enum ExportOptions: Int, CaseIterable { +/// Represents available time ranges for exporting group data +enum ExportOptions: CaseIterable { case month, threeMonths, sixMonths, year, all - var option: String { + var localizedKey: String { switch self { case .month: - return "Month" + return "export_option_month" case .threeMonths: - return "Three months" + return "export_option_three_months" case .sixMonths: - return "Six months" + return "export_option_six_months" case .year: - return "Year" + return "export_option_year" case .all: - return "All" + return "export_option_all" } } }Data/Data/Store/ExpenseStore.swift (1)
63-84: Consider extracting common query logic to reduce duplication.The implementation is nearly identical to TransactionStore's fetchTransactions method. Consider creating a generic query builder to reduce code duplication and ensure consistent behavior.
Example approach:
protocol DateRangeQueryable { associatedtype T func reference(groupId: String) -> CollectionReference func buildDateRangeQuery(reference: CollectionReference, startDate: Date?, endDate: Date) -> Query } extension DateRangeQueryable { func fetchWithDateRange(groupId: String, startDate: Date?, endDate: Date) async throws -> [T] { guard endDate > (startDate ?? endDate) else { throw ServiceError.invalidDateRange } let query = buildDateRangeQuery( reference: reference(groupId: groupId), startDate: startDate, endDate: endDate ) return try await query.getDocuments(source: .server).documents.map { try $0.data(as: T.self) } } }Data/Data/Repository/ExpenseRepository.swift (1)
167-169: Enhance error handling context.Consider wrapping store errors with additional context to help with debugging and error reporting.
public func fetchExpenses(groupId: String, startDate: Date?, endDate: Date) async throws -> [Expense] { - try await store.fetchExpenses(groupId: groupId, startDate: startDate, endDate: endDate) + do { + return try await store.fetchExpenses(groupId: groupId, startDate: startDate, endDate: endDate) + } catch { + LogE("ExpenseRepository: Failed to fetch expenses: \(error)") + throw ServiceError.fetchFailed(context: "Failed to fetch expenses", error: error) + } }Data/Data/Repository/TransactionRepository.swift (1)
158-160: Enhance error handling context.Similar to ExpenseRepository, consider wrapping store errors with additional context.
public func fetchTransactions(groupId: String, startDate: Date?, endDate: Date) async throws -> [Transactions] { - try await store.fetchTransactions(groupId: groupId, startDate: startDate, endDate: endDate) + do { + return try await store.fetchTransactions(groupId: groupId, startDate: startDate, endDate: endDate) + } catch { + LogE("TransactionRepository: Failed to fetch transactions: \(error)") + throw ServiceError.fetchFailed(context: "Failed to fetch transactions", error: error) + } }Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
85-85: Remove debug print statement.The debug print statement should be removed from production code.
- print("xxx \(expenses.count) \(transactions.count)")Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
35-38: Document new @published properties.Add documentation comments to explain the purpose and usage of the new properties:
- showShareSheet
- showExportOptions
+ /// Controls the visibility of the share sheet UI. @Published var showShareSheet = false @Published var showSettleUpSheet = false @Published var showBalancesSheet = false + /// Controls the visibility of the export options menu. @Published var showExportOptions = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Data/Data/Repository/CommentRepository.swift(1 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Data/Data/Repository/TransactionRepository.swift(1 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeView.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🔇 Additional comments (5)
Splito/UI/Home/Groups/Group/GroupHomeView.swift (3)
130-132: LGTM! Well-structured binding declarations.The new bindings follow SwiftUI conventions and are appropriately named for their purpose.
152-154: LGTM! Consistent UI implementation.The export button follows the existing UI pattern and integrates well with other options.
169-175:⚠️ Potential issueFix empty activity items in ShareSheetView.
The ShareSheetView is initialized with an empty string as activity items. This appears to be a placeholder and needs to be replaced with actual export data.
Please ensure that the actual export data is passed to the ShareSheetView. Consider adding a loading state while the export data is being prepared.
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift (1)
22-30: Consider adding error handling and loading state management for export operations.The export functionality implementation could benefit from:
- Error handling for failed export operations
- Loading state management during export processing
- Handling of concurrent export requests
Could you verify how export failures and concurrent export operations are handled in the ViewModel? This will help ensure a robust user experience.
✅ Verification successful
Error handling is implemented, but loading state management could be improved
The current implementation includes error handling with user feedback through toast messages. However, consider these improvements:
- Add a loading indicator during the export operation
- Prevent multiple concurrent export operations by tracking the export state
- Consider using a more detailed completion handler to distinguish between different failure cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in export-related code rg -A 5 "handleExportTap|showExportOptions"Length of output: 4620
Script:
#!/bin/bash # Search for the complete implementation of handleExportTap ast-grep --pattern 'func handleExportTap($_) { $$$ }'Length of output: 57
Script:
#!/bin/bash # Search for the complete implementation with more context rg -A 20 "func handleExportTap"Length of output: 2215
Script:
#!/bin/bash # Search with more context lines to see complete implementation rg -A 40 "func handleExportTap"Length of output: 4479
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
35-38: Verify UI state cleanup.Ensure that
showShareSheetandshowExportOptionsare properly reset when:
- The view disappears
- Export operation completes
- User cancels the operation
✅ Verification successful
State management is properly implemented ✅
The UI states are correctly managed:
showExportOptionsis reset before transitioning to share sheetshowShareSheetis properly cleaned up in the share completion handler- Sheet dismissal automatically resets the presentation state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for state cleanup in view lifecycle methods ast-grep --pattern 'func onDisappear() { $$$ showShareSheet = false showExportOptions = false $$$ }'Length of output: 110
Script:
#!/bin/bash # Search for sheet presentations and their bindings rg "sheet\(isPresented:|sheet\(\$" -A 3 -B 1 # Search for any direct state modifications rg "showShareSheet = |showExportOptions = " -A 2 -B 2Length of output: 13366
cp-nirali-s
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
81-99:⚠️ Potential issueFix completion handler and improve error handling.
Several issues need to be addressed:
- Completion handler is called twice (lines 89 and 93).
- Debug print statement should be removed.
- Missing memory management considerations.
- Generic error toast message.
Apply this diff to fix the issues:
- Task { + Task { [weak self] in + guard let self else { return } do { let expenses = try await expenseRepository.fetchExpenses(groupId: groupId, startDate: startDate, endDate: today) let transactions = try await transactionRepository.fetchTransactions(groupId: groupId, startDate: startDate, endDate: today) - print("xxx \(expenses.count) \(transactions.count)") + guard !expenses.isEmpty || !transactions.isEmpty else { + self.showToastFor(toast: ToastPrompt(type: .info, title: "No Data", message: "No data available for the selected period")) + completion(false) + return + } LogD("GroupHomeViewModel: \(#function) Expenses/payments fetched successfully.") if let reportURL = generateCSVReport(expenses: expenses, payments: transactions) { self.generatedReportURL = reportURL completion(true) } else { + self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", message: "Failed to generate report")) completion(false) } - completion(true) } catch { LogE("GroupHomeViewModel: \(#function) Failed to fetch Expenses/payments: \(error).") - showToastForError() + self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", message: "Failed to fetch data: \(error.localizedDescription)")) completion(false) } }
🧹 Nitpick comments (8)
Data/Data/Extension/Date+Extension.swift (1)
33-38: Consider optimizing DateFormatter usage.Creating a new DateFormatter instance for each property access is expensive. Consider using static formatters for better performance.
Here's an optimized implementation:
+ private static let numericFormatter: DateFormatter = { + let formatter = DateFormatter() + formatter.dateFormat = "dd-MM-yyyy" + return formatter + }() // 13-12-2001 var numericDate: String { - let dateFormatter = DateFormatter() - dateFormatter.dateFormat = "dd-MM-yyyy" - return dateFormatter.string(from: self) + return Self.numericFormatter.string(from: self) }Similar optimization can be applied to other date formatting properties (
longDate,shortDate,monthWithYear) for consistent performance improvements across the extension.Splito/UI/Home/Groups/Group/GroupHomeView.swift (3)
130-140: LGTM! Consider adding documentation for the export functionality.The new bindings and properties are well-structured for managing the export and share functionality. Consider adding documentation comments to describe the purpose of
generatedReportURLand the expected behavior ofonExportTapclosure's completion handler.Add documentation comments:
+ /// URL of the generated export report file let generatedReportURL: URL? + /// Handles the export action for the selected option + /// - Parameters: + /// - option: The selected export time range + /// - completion: Callback indicating if the export succeeded let onExportTap: (ExportOptions, @escaping (Bool) -> Void) -> Void
172-180: Consider cleaning up the generated file after sharing.While the share sheet implementation is correct, consider adding cleanup logic to remove the temporary export file after sharing is complete to prevent accumulation of temporary files.
.sheet(isPresented: $showShareSheet) { if let fileURL = generatedReportURL { ShareSheetView(activityItems: [fileURL]) { isCompleted in if isCompleted { showShareSheet = false + try? FileManager.default.removeItem(at: fileURL) } } } }
184-200: Enhance the ExportOptions enum implementation.Consider these improvements to the enum:
- Remove unused
Intraw value type- Add documentation for each case
- Use a more structured approach for localization
-enum ExportOptions: Int, CaseIterable { +/// Options for exporting group data based on time range +enum ExportOptions: CaseIterable { case month, threeMonths, sixMonths, year, all var option: String { switch self { case .month: - return "Month" + return String(localized: "export.option.month") case .threeMonths: - return "Three months" + return String(localized: "export.option.three_months") case .sixMonths: - return "Six months" + return String(localized: "export.option.six_months") case .year: - return "Year" + return String(localized: "export.option.year") case .all: - return "All" + return String(localized: "export.option.all") } } }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (3)
22-23: Add access control modifier to the group property.The
groupproperty should explicitly declare its access level for better encapsulation. Consider making itprivate(set)if it only needs to be modified within the view model.- @Published var group: Groups? + @Published private(set) var group: Groups?
23-23: Document the lifecycle of generatedReportURL.Add documentation comments to explain when this URL is generated, how long it's valid, and when it's cleared. This helps prevent potential memory leaks or invalid URL access.
+ /// The URL of the generated export report. + /// This URL is set when a report is generated and should be cleared after sharing. @Published var generatedReportURL: URL?
25-25: Remove trailing whitespace.Remove trailing whitespace on lines 25 and 36 to maintain consistent code formatting.
Also applies to: 36-36
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 25-25: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
134-134: Remove hardcoded currency code "INR".The currency code "INR" is hardcoded in multiple places. This should be replaced with the actual currency code from the group or payment settings.
- var paymentRow = "\(date), \(payer) paid \(receiver), Payment, \(payment.amount), INR" + var paymentRow = "\(date), \(payer) paid \(receiver), Payment, \(payment.amount), \(payment.currencyCode ?? group.defaultCurrency ?? "INR")" - csvContent += "\(date), Total balance, , , INR" + csvContent += "\(date), Total balance, , , \(group.defaultCurrency ?? "INR")"Also applies to: 148-148
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Data/Data/Extension/Date+Extension.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeView.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.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
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
[Warning] 104-104: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 109-109: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 113-113: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 117-117: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 123-123: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 135-135: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 141-141: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 145-145: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 149-149: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 158-158: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 162-162: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
[Warning] 25-25: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 36-36: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (4)
Data/Data/Extension/Date+Extension.swift (1)
33-38: Implementation looks good and follows existing patterns.The new
numericDateproperty is well-documented with an example and follows the consistent implementation pattern used throughout the extension.Splito/UI/Home/Groups/Group/GroupHomeView.swift (1)
153-167: LGTM! Clean implementation of the export flow.The export button and confirmation dialog implementation follows SwiftUI best practices with proper state management and user flow.
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (2)
20-20: Keep TransactionRepository private to maintain encapsulation.The
transactionRepositoryproperty should remain private unless there's a compelling reason to expose it. If external access is needed, consider adding specific public methods instead.- @Inject var transactionRepository: TransactionRepository + @Inject private var transactionRepository: TransactionRepository
22-42: Verify the export report implementation.While the UI state properties for export functionality are in place, the actual export implementation seems to be missing. Please ensure that:
- The export logic is implemented in a separate extension/file
- The
generatedReportURLis properly managed- Error handling for export failures is implemented
✅ Verification successful
Export functionality implementation verified successfully.
The implementation includes:
- Complete export logic with time-range filtering
- Proper CSV generation with expenses and settlements
- Error handling and completion callbacks
- Temporary file management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for export-related implementations rg -l "func.*export|func.*generate.*report" --type swiftLength of output: 120
Script:
#!/bin/bash # Get the content of the extension file cat "Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift"Length of output: 14638
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 25-25: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 36-36: Lines should not have trailing whitespace
(trailing_whitespace)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Data/Data/Store/TransactionStore.swift (1)
59-82:⚠️ Potential issueAdd validation, limits and error handling for better reliability.
The implementation needs several improvements:
- Missing validation that endDate is after startDate
- No limit on query size could impact performance
- Duplicate query construction between paths
- Inconsistent ordering between paths (only else branch orders by date)
- Missing error handling for document conversion
Consider this improved implementation:
func fetchTransactions(groupId: String, startDate: Date?, endDate: Date) async throws -> [Transactions] { + guard endDate > (startDate ?? endDate) else { + throw ServiceError.invalidDateRange + } + + let baseQuery = transactionReference(groupId: groupId) + .whereField("is_active", isEqualTo: true) + .order(by: "date", descending: true) + .limit(to: 1000) + if let startDate { let startTimestamp = Timestamp(date: startDate) let endTimestamp = Timestamp(date: endDate) - - let query = transactionReference(groupId: groupId) - .whereField("is_active", isEqualTo: true) - .whereField("date", isGreaterThanOrEqualTo: startTimestamp) - .whereField("date", isLessThanOrEqualTo: endTimestamp) - - let snapshot = try await query.getDocuments(source: .server).documents - - return try snapshot.map { document in - try document.data(as: Transactions.self) - } + + let query = baseQuery + .whereField("date", isGreaterThanOrEqualTo: startTimestamp) + .whereField("date", isLessThanOrEqualTo: endTimestamp) + return try await fetchAndMapDocuments(query) } else { - let query = transactionReference(groupId: groupId) - .whereField("is_active", isEqualTo: true) - .order(by: "date", descending: true) - return try await query.getDocuments(source: .server).documents.map { - try $0.data(as: Transactions.self) - } + let query = baseQuery + .whereField("date", isLessThanOrEqualTo: Timestamp(date: endDate)) + return try await fetchAndMapDocuments(query) } } + +private func fetchAndMapDocuments(_ query: Query) async throws -> [Transactions] { + let snapshot = try await query.getDocuments(source: .server) + return try snapshot.documents.compactMap { document in + do { + return try document.data(as: Transactions.self) + } catch { + LogE("TransactionStore: Error decoding transaction from document \(document.documentID): \(error)") + return nil + } + } +}
🧹 Nitpick comments (4)
Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift (1)
Line range hint
206-213: Consider validating comment content before submission.While the implementation is generally good, consider adding basic validation to ensure the comment is not empty after trimming.
Task { [weak self] in guard let self else { return } do { self.showLoader = true + let trimmedComment = self.comment.trimming(spaces: .leadingAndTrailing) + guard !trimmedComment.isEmpty else { + self.showLoader = false + return + } let comment = Comment(parentId: transactionId, - comment: self.comment.trimming(spaces: .leadingAndTrailing), + comment: trimmedComment, commentedBy: userId)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
81-105: Add progress indication during export.Long-running operations like data fetching and CSV generation should provide visual feedback to users.
Consider adding a loading state:
func handleExportTap(exportOption: ExportOptions, completion: @escaping (Bool) -> Void) { + isExporting = true // Add this property to your ViewModel Task { [weak self] in // ... existing code ... } catch { // ... error handling ... } + self?.isExporting = false } }
165-173: Improve error handling for file operations.The error handling for file operations could be more informative.
do { let fileName = "Splito group report \(date.shortDate).csv" let filePath = FileManager.default.temporaryDirectory.appendingPathComponent(fileName) try csvContent.write(to: filePath, atomically: true, encoding: .utf8) return filePath } catch { - LogE("GroupHomeViewModel: \(#function) Failed to write CSV file: \(error)") + LogE("GroupHomeViewModel: \(#function) Failed to write CSV file to \(filePath.path): \(error)") + self.showToastFor(toast: ToastPrompt( + type: .error, + title: "Export Failed", + message: "Failed to save the report. Please try again." + )) return nil }
63-174: Consider architectural improvements for scalability and security.A few architectural considerations:
- Implement cleanup of temporary files
- Add pagination or streaming for large datasets
- Consider memory usage for large groups
For large groups or extensive history:
- Consider implementing pagination in the data fetch
- Stream the CSV generation instead of building it in memory
- Add a cleanup mechanism for temporary files
Would you like me to provide implementation examples for any of these improvements?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Data/Data/Repository/CommentRepository.swift(4 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
- Data/Data/Repository/CommentRepository.swift
🧰 Additional context used
📓 Learnings (1)
Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift (1)
Learnt from: cp-nirali-s
PR: canopas/splito#97
File: Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift:180-210
Timestamp: 2025-01-15T07:27:18.714Z
Learning: Comments in the Splito app do not require length validation. The product decision is to keep comments unrestricted in length.
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Splito/UI/Home/Groups/Group/Group Options/Settlements/Transaction Detail/GroupTransactionDetailViewModel.swift (1)
Line range hint
201-205: LGTM! Well-structured guard clause.The guard clause effectively validates all required data before proceeding with comment addition. The error logging is informative and helpful for debugging.
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
100-103: 🛠️ Refactor suggestionImprove error handling with specific error messages.
The generic
showToastForError()doesn't provide enough context about what went wrong during the export process.- self.showToastForError() + self.showToastFor(toast: ToastPrompt( + type: .error, + title: "Export Failed", + message: "Failed to fetch data: \(error.localizedDescription)" + ))Likely invalid or redundant comment.
87-91:⚠️ Potential issueFix the data validation logic.
The condition
!expenses.isEmpty && !transactions.isEmptyrequires both arrays to be non-empty, which is too restrictive. Users might want to export even if they only have expenses or only have transactions.- guard !expenses.isEmpty && !transactions.isEmpty else { + guard !expenses.isEmpty || !transactions.isEmpty else {Likely invalid or redundant comment.
107-174: 🛠️ Refactor suggestionImprove CSV generation robustness and memory efficiency.
Several improvements needed for production readiness:
- CSV fields need proper escaping
- Currency code is hardcoded
- String concatenation can be memory-intensive
- Missing validation for group members
Consider implementing these improvements:
func generateCSVReport(expenses: [Expense], payments: [Transactions]) -> URL? { - guard let group else { return nil } + guard let group, !group.members.isEmpty else { return nil } + + // Helper function to escape CSV fields + func escapeCSV(_ field: String) -> String { + if field.contains(",") || field.contains("\"") || field.contains("\n") { + return "\"\(field.replacingOccurrences(of: "\"", with: "\"\""))\"" + } + return field + } - var csvContent = "" + // Use StringBuilder pattern for better memory efficiency + var csvBuilder = StringBuilder() // Add Expenses Section - csvContent += "Expenses\n" + csvBuilder.append("Expenses\n") let memberNames = group.members.map { getMemberDataBy(id: $0)?.nameWithLastInitial ?? "Unknown" } - let header = "Date, Description, Category, Cost, Currency, \(memberNames.joined(separator: ", "))\n\n" + let headerFields = ["Date", "Description", "Category", "Cost", "Currency"] + .map(escapeCSV) + csvBuilder.append(headerFields.joined(separator: ",")) + csvBuilder.append(",") + csvBuilder.append(memberNames.map(escapeCSV).joined(separator: ",")) + csvBuilder.append("\n\n") for expense in expenses { let date = expense.date.dateValue().numericDate - var expenseRow = "\(date), \(expense.name), \(expense.category ?? "General"), \(expense.amount), \(expense.currencyCode ?? "INR")" + let expenseFields = [ + date, + escapeCSV(expense.name), + escapeCSV(expense.category ?? "General"), + String(format: "%.2f", expense.amount), + escapeCSV(expense.currencyCode ?? group.defaultCurrency ?? "INR") + ] + csvBuilder.append(expenseFields.joined(separator: ","))Also, consider using a unique filename to prevent conflicts:
- let fileName = "Splito group report \(date.shortDate).csv" + let dateFormatter = DateFormatter() + dateFormatter.dateFormat = "yyyyMMdd_HHmmss" + let timestamp = dateFormatter.string(from: Date()) + let fileName = "Splito_Group_Report_\(timestamp).csv"Likely invalid or redundant comment.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 163-163: Lines should not have trailing whitespace
(trailing_whitespace)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
175-176: Use unique filenames to prevent conflicts.The current filename format could lead to conflicts if multiple exports happen on the same day.
- let fileName = "Splito group report \(date.shortDate).csv" + let dateFormatter = DateFormatter() + dateFormatter.dateFormat = "yyyyMMdd_HHmmss" + let timestamp = dateFormatter.string(from: date) + let fileName = "Splito group report \(timestamp).csv"
174-182: Add file size check and cleanup.The method should check the file size before writing and clean up temporary files.
do { + // Check estimated content size + let estimatedSize = csvContent.utf8.count + guard estimatedSize < 50 * 1024 * 1024 else { // 50MB limit + LogE("GroupHomeViewModel: \(#function) CSV content too large: \(estimatedSize) bytes") + return nil + } + let fileName = "Splito group report \(date.shortDate).csv" let filePath = FileManager.default.temporaryDirectory.appendingPathComponent(fileName) try csvContent.write(to: filePath, atomically: true, encoding: .utf8) + + // Schedule cleanup after a reasonable time (e.g., 1 hour) + DispatchQueue.global().asyncAfter(deadline: .now() + 3600) { + try? FileManager.default.removeItem(at: filePath) + } + return filePath
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🔇 Additional comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
109-138: 🛠️ Refactor suggestionImprove CSV generation robustness and memory efficiency.
The current implementation has several issues:
- No escaping for special characters in CSV fields (commas, quotes, newlines).
- Inefficient string concatenation.
- Hardcoded currency code.
+ // Helper function to escape CSV fields + func escapeCSV(_ field: String) -> String { + if field.contains(",") || field.contains("\"") || field.contains("\n") { + return "\"\(field.replacingOccurrences(of: "\"", with: "\"\""))\"" + } + return field + } + var csvContent = "" + var csvBuilder = StringBuilder() // Consider using a StringBuilder class for better memory efficiency // Add Expenses Section - csvContent += "Expenses\n" + csvBuilder.append("Expenses\n") for expense in expenses { let date = expense.date.dateValue().numericDate - var expenseRow = "\(date), \(expense.name), \(expense.category ?? "General"), \(expense.amount), \(expense.currencyCode ?? "INR")" + let fields = [ + date, + escapeCSV(expense.name), + escapeCSV(expense.category ?? "General"), + String(expense.amount), + expense.currencyCode ?? group.currencyCode ?? "INR" // Use group's currency code as fallback + ] + var expenseRow = fields.joined(separator: ",")Likely invalid or redundant comment.
98-102: 🛠️ Refactor suggestionImprove error handling with specific error messages.
The generic
showToastForError()doesn't provide context about what went wrong during the export process.} catch { LogE("GroupHomeViewModel: \(#function) Failed to fetch Expenses/payments: \(error).") - self.showToastForError() + self.showToastFor(toast: ToastPrompt( + type: .error, + title: "Export Failed", + message: "Failed to fetch data: \(error.localizedDescription)" + )) completion(false) }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
131-132: 🛠️ Refactor suggestionEnsure proper CSV field escaping to handle special characters.
Currently, the CSV fields are constructed without escaping, which may lead to issues if any field contains commas, quotes, or newline characters. Implement proper CSV escaping for all fields to ensure the CSV file is valid and data is correctly parsed.
You can add a helper function to escape CSV fields:
// Helper function to escape CSV fields func escapeCSV(_ field: String) -> String { if field.contains(",") || field.contains("\"") || field.contains("\n") { return "\"\(field.replacingOccurrences(of: "\"", with: "\"\""))\"" } return field }Then modify the header construction:
let headerFields = ["Date", "Description", "Category", "Cost", "Currency"] + memberNames let header = headerFields.map { escapeCSV($0) }.joined(separator: ",") + "\n\n"
153-159: 🛠️ Refactor suggestionEscape CSV fields in expense rows to prevent format issues.
When constructing the CSV rows for expenses, fields containing commas or special characters may break the CSV format. Ensure all fields are properly escaped.
Update the expense row construction:
let expenseFields = [ date, escapeCSV(expense.name), escapeCSV(expense.category ?? "General"), String(expense.amount), expense.currencyCode ?? "INR" ] // Add owe amounts for each group member let oweAmounts = allMemberIDs.map { String(expense.getCalculatedSplitAmountOf(member: $0)) } // Combine all fields let row = (expenseFields + oweAmounts).joined(separator: ",")
🧹 Nitpick comments (5)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
83-88: Provide user feedback when report generation fails.If the CSV report generation fails and
reportURLisnil, the user is not notified about the failure. Consider displaying an error toast to inform the user that the report could not be generated.Apply this change to notify the user:
if let reportURL = await self.generateCSVReport(expenses: expenses, payments: transactions) { groupReportUrl = reportURL showShareReportSheet = true } else { showShareReportSheet = false self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", message: "Unable to generate the report. Please try again later.")) }
77-77: Remove trailing whitespace from empty lines.Static analysis tools have detected trailing whitespace on lines 77 and 82. Removing these will adhere to coding standards and improve code cleanliness.
Also applies to: 82-82
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 77-77: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/GroupHomeView.swift (3)
168-175: EnsureshowShareReportSheetis dismissed after sharing is completed or canceled.Currently,
showShareReportSheetis set tofalseonly when sharing is completed. To handle cases where the user cancels sharing, consider settingshowShareReportSheettofalseregardless of the outcome.Modify the completion handler:
ShareSheetView(activityItems: [reportUrl]) { _ in showShareReportSheet = false }
130-141: Consider refactoring to reduce the number of parameters inGroupOptionsListView.The
GroupOptionsListViewhas multiple@Bindingvariables and closures, which can make the view harder to maintain. Consider creating a view model or struct to encapsulate related properties and actions to simplify the interface.For example, you could create a struct:
struct ExportOptionsConfig { @Binding var showExportOptions: Bool @Binding var showShareReportSheet: Bool let groupReportUrl: URL? let onExportTap: () -> Void let handleExportOptionSelection: (_ option: ExportOptions) -> Void }Then update
GroupOptionsListView:struct GroupOptionsListView: View { let exportOptionsConfig: ExportOptionsConfig // Other properties... }
179-195: Ensure localization support for export option titles.The
ExportOptionsenum returns hard-coded English strings. To support localization, ensure these strings are localized using the appropriate localization methods.Update the
optionproperty:var option: String { switch self { case .month: return NSLocalizedString("Month", comment: "") case .threeMonths: return NSLocalizedString("Three months", comment: "") case .sixMonths: return NSLocalizedString("Six months", comment: "") case .year: return NSLocalizedString("Year", comment: "") case .all: return NSLocalizedString("All", comment: "") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Splito/UI/Home/Groups/Group/GroupExpenseListView.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeView.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/Groups/Group/GroupExpenseListView.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
[Warning] 77-77: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 82-82: Lines should not have trailing whitespace
(trailing_whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
71-72: Good use of[weak self]to prevent retain cycles.Using
[weak self]in theTaskand safely unwrappingselfprevents potential retain cycles and ensures memory is managed properly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Data/Data/Store/TransactionStore.swift (1)
59-82: Enhance error handling and logging.The implementation would benefit from:
- Error handling for empty results
- Logging for important operations
- Consistent use of source parameter
Consider adding:
func fetchTransactions(groupId: String, startDate: Date?, endDate: Date) async throws -> [Transactions] { + LogI("Fetching transactions for group: \(groupId), startDate: \(String(describing: startDate)), endDate: \(endDate)") // ... existing code ... let snapshot = try await query.getDocuments(source: .server).documents + guard !snapshot.isEmpty else { + LogI("No transactions found for the specified date range") + return [] + }Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
96-112: Add validation for calendar operations.The implementation is correct, but calendar operations could fail in edge cases.
private func getStartDate(exportOption: ExportOptions) -> Date? { let today = Date() let calendar = Calendar.current + + // Ensure calendar operations are valid + guard calendar.isDate(today, inSameDayAs: today) else { + LogE("GroupHomeViewModel: Invalid calendar configuration") + return today + } switch exportOption { case .month: return calendar.date(byAdding: .month, value: -1, to: today) ?? today case .threeMonths: return calendar.date(byAdding: .month, value: -3, to: today) ?? today case .sixMonths: return calendar.date(byAdding: .month, value: -6, to: today) ?? today case .year: return calendar.date(byAdding: .year, value: -1, to: today) ?? today case .all: return nil } }
63-194: Consider architectural improvements for better maintainability.
- Extract CSV generation logic into a separate service class to improve separation of concerns.
- Consider using a CSV library to handle escaping and formatting.
- Add support for different currency formats and localization.
- Consider implementing progress tracking for large exports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeView.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Data/Data/Store/ExpenseStore.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
- Splito/UI/Home/Groups/Group/GroupHomeView.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Data/Data/Store/TransactionStore.swift (3)
60-74: Add query limits and optimize query construction.The implementation needs improvements in query handling:
- Missing query size limits could impact performance with large datasets
- Duplicate query conditions between branches
This was previously flagged in an earlier review. The suggestions remain valid:
- Add reasonable query limits
- Consolidate common query conditions
- Consider error handling for empty results
75-81:⚠️ Potential issueFix inconsistent sorting between code paths.
The non-date filtering branch uses ascending order while the date-filtered branch has no explicit ordering. This inconsistency could lead to unexpected behavior.
Apply this fix:
- .order(by: "date", descending: false) + .order(by: "date", descending: true) // Match the ordering in date-filtered branchLikely invalid or redundant comment.
59-60: 🛠️ Refactor suggestionAdd input validation and documentation.
The method lacks important validations and documentation:
- Missing validation that endDate is after startDate
- No documentation explaining the behavior with nil startDate
- Consider adding pagination support for consistency with
fetchTransactionsByAdd validation and documentation:
+/// Fetches transactions for a group within the specified date range. +/// - Parameters: +/// - groupId: The ID of the group +/// - startDate: Optional start date. If nil, fetches transactions up to endDate +/// - endDate: The end date for the range +/// - Returns: Array of transactions within the date range +/// - Throws: ServiceError.invalidDateRange if endDate is before startDate func fetchTransactions(groupId: String, startDate: Date?, endDate: Date) async throws -> [Transactions] { + if let startDate, endDate < startDate { + throw ServiceError.invalidDateRange + }Likely invalid or redundant comment.
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
63-65: LGTM!The implementation is clean and straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Splito/Localization/Localizable.xcstrings (1)
550-552: Consider making the "No Data" message more specific.The generic "No Data" message could be more descriptive to provide better context to users about what data is missing.
Consider using a more specific message like:
- "No Data" : { + "No Group Report Data" : { "extractionState" : "manual" },Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (1)
92-95: Improve error handling with specific messages.The generic error handling could be more informative for users.
- LogE("GroupHomeViewModel: \(#function) Failed to fetch Expenses/transactions: \(error).") - self.showToastForError() + LogE("GroupHomeViewModel: \(#function) Failed to fetch Expenses/transactions: \(error).") + self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", + message: "Failed to fetch data: \(error.localizedDescription)"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Splito/Localization/Localizable.xcstrings(2 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(3 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
[Warning] 70-70: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 78-78: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 83-83: Lines should not have trailing whitespace
(trailing_whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
Splito/Localization/Localizable.xcstrings (1)
403-405: LGTM! Clear and actionable messages.Both messages are well-written:
- The error message clearly indicates the failure and provides an action.
- The empty state message explains why no data is shown and suggests what to do next.
Also applies to: 553-554
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (6)
63-65: LGTM!The handler follows Swift naming conventions and correctly toggles the export options visibility.
71-72: LGTM! Task cancellation is properly implemented.The implementation correctly cancels any existing export task before starting a new one, preventing concurrent exports.
99-115: LGTM! Date calculation is well-implemented.The method correctly handles all export options and uses the Calendar API appropriately.
79-82: 🛠️ Refactor suggestionConsider relaxing the empty data check condition.
The current condition requires both expenses AND transactions to be empty to show the "No Data" message. This means if there are expenses but no transactions (or vice versa), the export will proceed but might generate an incomplete report.
- if expenses.isEmpty && transactions.isEmpty { + if expenses.isEmpty && transactions.isEmpty { + self.showToastFor(toast: ToastPrompt(type: .info, title: "No Data", + message: "No data found for \(option.displayName). Try a different time period.")) + return + }Likely invalid or redundant comment.
133-134:⚠️ Potential issueAdd CSV field escaping.
CSV fields containing commas, quotes, or newlines need proper escaping to maintain data integrity.
+ func escapeCSV(_ field: String) -> String { + if field.contains(",") || field.contains("\"") || field.contains("\n") { + return "\"\(field.replacingOccurrences(of: "\"", with: "\"\""))\"" + } + return field + } + var csvContent = "" - let header = "Date, Description, Category, Cost, Currency, \(memberNames.joined(separator: ", "))\n\n" + let headers = ["Date", "Description", "Category", "Cost", "Currency"] + .map(escapeCSV) + .joined(separator: ",") + let header = headers + "," + memberNames.map(escapeCSV).joined(separator: ",") + "\n\n"Likely invalid or redundant comment.
141-148: 🛠️ Refactor suggestionImprove file operations with better error handling and unique filenames.
The current implementation uses a static filename and doesn't provide detailed error feedback.
do { - let filePath = FileManager.default.temporaryDirectory.appendingPathComponent(GROUP_REPORT_FILE_NAME) + let dateFormatter = DateFormatter() + dateFormatter.dateFormat = "yyyyMMdd_HHmmss" + let timestamp = dateFormatter.string(from: Date()) + let fileName = "Group_Report_\(timestamp).csv" + let filePath = FileManager.default.temporaryDirectory.appendingPathComponent(fileName) try csvContent.write(to: filePath, atomically: true, encoding: .utf8) return filePath } catch { LogE("GroupHomeViewModel: \(#function) Failed to write CSV file: \(error)") + self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", + message: "Failed to save report: \(error.localizedDescription)")) return nil }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (3)
79-82: Improve empty data check message.The message could be more specific about what data is missing (expenses, transactions, or both).
- self.showToastFor(toast: ToastPrompt(type: .info, title: "No Data", message: "No data found for selected time period. Try a different time period.")) + let expensesMsg = expenses.isEmpty ? "expenses" : "" + let transactionsMsg = transactions.isEmpty ? "transactions" : "" + let dataTypes = [expensesMsg, transactionsMsg].filter { !$0.isEmpty }.joined(separator: " or ") + self.showToastFor(toast: ToastPrompt(type: .info, title: "No Data", + message: "No \(dataTypes) found for selected time period. Try a different time period."))
93-95: Use specific error message instead of generic error handler.Replace the generic error handler with a specific error message for better user feedback.
- self.showToastForError() + self.showToastFor(toast: ToastPrompt(type: .error, title: "Export Failed", + message: "Failed to fetch data: \(error.localizedDescription)"))
99-115: Add error handling for calendar date calculations.The date calculations could fail if the calendar operations return nil. Consider adding a fallback.
private func getStartDate(exportOption: ExportOptions) -> Date? { let today = Date() let calendar = Calendar.current + let fallbackDate = today switch exportOption { case .month: - return calendar.date(byAdding: .month, value: -1, to: today) ?? today + return calendar.date(byAdding: .month, value: -1, to: today) ?? fallbackDate case .threeMonths: - return calendar.date(byAdding: .month, value: -3, to: today) ?? today + return calendar.date(byAdding: .month, value: -3, to: today) ?? fallbackDate case .sixMonths: - return calendar.date(byAdding: .month, value: -6, to: today) ?? today + return calendar.date(byAdding: .month, value: -6, to: today) ?? fallbackDate case .year: - return calendar.date(byAdding: .year, value: -1, to: today) ?? today + return calendar.date(byAdding: .year, value: -1, to: today) ?? fallbackDate case .all: return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
🧰 Additional context used
📓 Learnings (1)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift (2)
155-162: LGTM! CSV escaping implementation is robust.The escaping logic correctly handles all common CSV special characters (commas, quotes, newlines).
164-211: LGTM! CSV section generators are well-implemented.The section generators:
- Use proper CSV escaping
- Handle all required data fields
- Format numbers correctly
- Use "INR" as currency code (as per app requirements)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Data/Data/Store/TransactionStore.swift (1)
71-73: Improve error handling in document transformation.The current implementation using
mapcould silently skip failed document transformations. Consider usingcompactMapwith proper error handling.- return try await query.getDocuments(source: .server).documents.map { document in - try document.data(as: Transactions.self) - } + let snapshot = try await query.getDocuments(source: .server) + return try snapshot.documents.compactMap { document in + do { + return try document.data(as: Transactions.self) + } catch { + LogE("Failed to parse transaction document: \(error)") + return nil + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Store/ExpenseStore.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Data/Data/Store/TransactionStore.swift (2)
63-70: 🛠️ Refactor suggestionOptimize query construction and ensure consistency.
The current implementation has several issues:
- Missing query limit could impact performance with large datasets
- Inconsistent ordering between code paths
- Missing endDate filter when startDate is nil
if let startDate { - query - .whereField("date", isGreaterThanOrEqualTo: Timestamp(date: startDate)) - .whereField("date", isLessThanOrEqualTo: Timestamp(date: endDate)) + query + .whereField("date", isGreaterThanOrEqualTo: Timestamp(date: startDate)) + .whereField("date", isLessThanOrEqualTo: Timestamp(date: endDate)) + .order(by: "date", descending: true) + .limit(to: 1000) } else { query - .order(by: "date", descending: false) + .whereField("date", isLessThanOrEqualTo: Timestamp(date: endDate)) + .order(by: "date", descending: true) + .limit(to: 1000) }Likely invalid or redundant comment.
59-62: 🛠️ Refactor suggestionAdd parameter validation and documentation.
The method lacks input validation and documentation. Consider:
- Adding parameter validation
- Including documentation explaining the parameters and return value
- Handling invalid date ranges
+/// Fetches transactions for a specific group within a date range. +/// - Parameters: +/// - groupId: The ID of the group to fetch transactions for +/// - startDate: Optional start date for filtering transactions. If nil, fetches up to endDate +/// - endDate: End date for filtering transactions +/// - Returns: Array of transactions matching the criteria +/// - Throws: ServiceError.invalidDateRange if date range is invalid func fetchTransactions(groupId: String, startDate: Date?, endDate: Date) async throws -> [Transactions] { + if let startDate, startDate > endDate { + throw ServiceError.invalidDateRange + } + let query = transactionReference(groupId: groupId) .whereField("is_active", isEqualTo: true)Likely invalid or redundant comment.
ScreenRecording_01-17-2025.15-53-36_1.1.1.mp4
ScreenRecording_01-17-2025.16-00-23_1.1.1.mp4
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes