Add expense attachment support #73
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, primarily focusing on improving date handling, image management, and user interface components. Key updates include new computed properties and methods in date extensions, refactoring of currency formatting in double extensions, and the addition of image handling capabilities in expense-related models and repositories. Furthermore, new SwiftUI views for displaying zoomable images are introduced, along with improved error handling and localization updates. Overall, these changes aim to streamline functionality and enhance user experience. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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: 13
🧹 Outside diff range and nitpick comments (20)
Data/Data/Extension/Double+Extension.swift (1)
Line range hint
1-32: Consider adding currency formatting tests.Given that this extension is critical for expense management and the new attachment feature, it's important to ensure currency formatting remains consistent across different locales and edge cases.
Would you like me to help create comprehensive unit tests covering:
- Different locales and currency formats
- Positive and negative values
- Zero and large numbers
- Edge cases with attachments
Data/Data/Helper/Firebase/StorageManager.swift (1)
Line range hint
31-45: Consider adding image validation and optimization.The current implementation could benefit from additional safeguards:
- Image size validation and limits
- Support for different image formats beyond JPG
- Image optimization before upload
- Cleanup mechanism for orphaned images
Example improvements for the
uploadImagemethod:public func uploadImage(for storeType: ImageStoreType, id: String, imageData: Data) async throws -> String? { + guard imageData.count <= 5 * 1024 * 1024 else { // 5MB limit + throw StorageError.imageTooLarge + } + + // Detect image type and set appropriate content type + let imageType = try detectImageType(from: imageData) + let metadata = StorageMetadata() + metadata.contentType = imageType.mimeType + + // Optimize image if needed + let optimizedData = try await optimizeImageIfNeeded(imageData, maxSize: 1024) let storageRef = storage.reference(withPath: "/\(storeType.pathName)/\(id)") - let metadata = StorageMetadata() - metadata.contentType = "image/jpg"Data/Data/Model/Expense.swift (2)
20-20: Add documentation for the new imageUrl propertyConsider adding a documentation comment explaining:
- Expected image URL format
- Any size/type limitations
- Whether it supports local or remote URLs
Example:
/// URL to the expense's attached image. /// Supports remote URLs to cloud storage locations. /// - Note: Images should follow the app's size and format guidelines public var imageUrl: String?
Missing test coverage for Expense Codable implementation
The codebase lacks test coverage for the Expense model's Codable implementation. No tests were found that verify:
- JSON encoding/decoding of Expense model
- Handling of optional imageUrl property
- Custom CodingKeys implementation
Action items:
- Add unit tests in
Data/DataTests/DataTests.swiftto verify Codable conformance- Include test cases for both presence and absence of imageUrl
- Verify correct mapping of snake_case keys to camelCase properties
🔗 Analysis chain
Line range hint
20-54: Verify Codable implementation testingThe changes affect the Codable implementation. Please ensure test coverage for:
- Encoding/decoding with imageUrl present
- Encoding/decoding with imageUrl absent
Let me search for test files and Codable implementation tests specifically.
Let me check the content of the test files we found to look for any Expense model tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relevant test files fd -e swift -e test expenseLength of output: 749
Script:
#!/bin/bash # Search for test files and test implementations fd -e swift test # Search for Expense model test cases rg -l "class.*Test.*Expense" # Look for Codable related test implementations rg -A 5 "func test.*Expense.*[Cc]odable|func test.*[Cc]odable.*Expense"Length of output: 309
Script:
#!/bin/bash # Check content of test files for any Expense related tests cat Data/DataTests/DataTests.swift cat SplitoTests/SplitoTests.swift # Search for any Codable conformance tests with different pattern rg -A 5 "XCTestCase.*Expense|test.*Expense" Data/DataTests/DataTests.swift SplitoTests/SplitoTests.swiftLength of output: 2609
Data/Data/Extension/Date+Extension.swift (1)
19-32: Consider moving date formatting to the Presentation layerThe Data layer currently contains UI-specific date formatting logic. This might violate separation of concerns, as formatting for display typically belongs in the Presentation layer.
Consider:
- Moving these formatting properties to a UI/Presentation layer extension
- Keeping only data-essential date operations in the Data layer
- Creating a dedicated DateFormatter service in the Presentation layer
This would improve maintainability and better align with architectural boundaries.
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (1)
Line range hint
65-72: Consider enhancing error handling for attachment-related scenariosThe current error handling in
fetchMoreActivityLogsis generic. With the addition of expense attachments, consider providing more specific error feedback for attachment-related issues (e.g., attachment size limits, format issues).Consider updating the error handling like this:
} catch { - viewState = .initial - showToastForError() + viewState = .initial + if let attachmentError = error as? AttachmentError { + showToastFor(attachmentError) + } else { + showToastForError() + } }Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
Line range hint
168-182: Add amount validation and handle edge cases.The current implementation could benefit from additional validation:
- Maximum amount validation to prevent overflow
- Handling of negative numbers
- Decimal place limitations
Consider adding these validations to the
formatAmountfunction:private func formatAmount(newValue: String) { let numericInput = newValue.replacingOccurrences(of: "₹", with: "").trimmingCharacters(in: .whitespaces) if let value = Double(numericInput) { + // Validate maximum amount + guard value <= 999999999 else { return } + // Ensure positive amount + guard value >= 0 else { return } + // Limit decimal places to 2 + let formatted = String(format: "%.2f", value) + guard let finalValue = Double(formatted) else { return } - amount = value + amount = finalValue } else { amount = 0 } amountString = numericInput.isEmpty ? "" : "₹ " + numericInput }
Line range hint
144-182: Consider localization for currency formatting.The currency symbol "₹" is hardcoded. Consider using NumberFormatter for proper locale-aware currency formatting.
Here's a suggested approach:
struct AmountRowView: View { + private let currencyFormatter: NumberFormatter = { + let formatter = NumberFormatter() + formatter.numberStyle = .currency + formatter.locale = Locale(identifier: "en_IN") // Or use system locale + return formatter + }() // ... existing code ... private func formatAmount(newValue: String) { - let numericInput = newValue.replacingOccurrences(of: "₹", with: "").trimmingCharacters(in: .whitespaces) + let numericInput = newValue.replacingOccurrences(of: currencyFormatter.currencySymbol ?? "₹", with: "").trimmingCharacters(in: .whitespaces) if let value = Double(numericInput) { amount = value } else { amount = 0 } - amountString = numericInput.isEmpty ? "" : "₹ " + numericInput + amountString = numericInput.isEmpty ? "" : "\(currencyFormatter.currencySymbol ?? "₹") " + numericInput } }Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
29-33: Consider enhancing image handling for better UX.The image display implementation looks good, but could benefit from additional user experience improvements.
Consider adding:
- Loading state while the image is being fetched
- Error handling for failed image loads
- Tap gesture to view the image in full screen
if let imageUrl = viewModel.expense?.imageUrl { ExpenseImageView(imageUrl: imageUrl) .aspectRatio(16/9, contentMode: .fit) .cornerRadius(12) + .onTapGesture { + // TODO: Show full screen image view + } }
29-33: Consider implementing image caching and size optimization.To improve performance and reduce memory usage:
- Implement image caching to prevent unnecessary network requests
- Consider implementing image size optimization, especially for large images
- Add image compression before upload (in the related upload functionality)
This will help:
- Reduce network bandwidth usage
- Improve loading times for frequently viewed images
- Optimize memory usage when displaying multiple expenses with images
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (3)
Line range hint
13-15: Consider making EXPENSES_LIMIT configurable.The hardcoded
EXPENSES_LIMIT = 10could be made more flexible by moving it to a configuration file or making it adjustable based on device capabilities and network conditions.- private let EXPENSES_LIMIT = 10 + private var expensesLimit: Int { + // Return different limits based on device/network conditions + return preference.isLowPowerMode ? 5 : 10 + }
Line range hint
232-239: Optimize expense filtering performance.The filtering logic in
updateGroupExpensesperforms multiple operations that could be optimized:
- Multiple calls to
uniqued()- Case-insensitive string comparison in a loop
- String to Double conversion attempt for each expense
Consider this optimization:
private func updateGroupExpenses() { - let filteredExpenses = expensesWithUser.uniqued().filter { expense in - searchedExpense.isEmpty || - expense.expense.name.lowercased().contains(searchedExpense.lowercased()) || - expense.expense.amount == Double(searchedExpense) - } - self.groupExpenses = Dictionary(grouping: filteredExpenses.uniqued().sorted { $0.expense.date.dateValue() > $1.expense.date.dateValue() }) { expense in - return expense.expense.date.dateValue().monthWithYear - } + let searchTerm = searchedExpense.lowercased() + let searchAmount = Double(searchedExpense) + + let uniqueExpenses = expensesWithUser.uniqued() + let filteredExpenses = searchedExpense.isEmpty ? uniqueExpenses : uniqueExpenses.filter { expense in + expense.expense.name.lowercased().contains(searchTerm) || + (searchAmount != nil && expense.expense.amount == searchAmount) + } + + self.groupExpenses = Dictionary(grouping: filteredExpenses.sorted { $0.expense.date.dateValue() > $1.expense.date.dateValue() }) { expense in + expense.expense.date.dateValue().monthWithYear + }
Line range hint
64-71: Structure notification handling more robustly.The notification handling could be more maintainable by:
- Using a dedicated enum for notification names
- Centralizing notification registration
- Adding proper cleanup in deinit
Consider creating a dedicated notification handler:
private extension GroupHomeViewModel { enum GroupNotification: String { case addExpense = "addExpense" case updateExpense = "updateExpense" case deleteExpense = "deleteExpense" case addTransaction = "addTransaction" case updateTransaction = "updateTransaction" case deleteTransaction = "deleteTransaction" case updateGroup = "updateGroup" var name: Notification.Name { return Notification.Name(self.rawValue) } } func registerNotifications() { GroupNotification.allCases.forEach { notification in NotificationCenter.default.addObserver( self, selector: #selector(handleNotification(_:)), name: notification.name, object: nil ) } } @objc private func handleNotification(_ notification: Notification) { guard let notificationType = GroupNotification(rawValue: notification.name.rawValue) else { return } switch notificationType { case .addExpense: handleAddExpense(notification: notification) case .updateExpense: handleUpdateExpense(notification: notification) // ... handle other cases } } }Data/Data/Repository/GroupRepository.swift (2)
Line range hint
213-234: Enhance bulk member fetching implementationThe current implementation has similar thread safety concerns as
fetchMemberBy:
- Async barrier writes should be awaited
- Consider using a Set for more efficient duplicate checking
- Add structured error handling for the async operations
Here's a suggested implementation:
let missingMemberIds = memberIds.filter { memberId in let cachedMember = self.groupMembersQueue.sync { self.groupMembers.first { $0.id == memberId } } return cachedMember == nil } if missingMemberIds.isEmpty { return groupMembersQueue.sync { self.groupMembers.filter { memberIds.contains($0.id) } } } try await withThrowingTaskGroup(of: AppUser?.self) { groupTask in for memberId in missingMemberIds { groupTask.addTask { try await self.fetchMemberBy(userId: memberId) } } for try await member in groupTask { if let member { members.append(member) - groupMembersQueue.async(flags: .barrier) { - if !self.groupMembers.contains(where: { $0.id == member.id }) { - self.groupMembers.append(member) - } - } + // Use async/await with barrier to ensure write completion + try await groupMembersQueue.async(flags: .barrier) { + // Use a more efficient way to check duplicates + let memberIds = Set(self.groupMembers.map { $0.id }) + if !memberIds.contains(member.id) { + self.groupMembers.append(member) + } + } } } }
Line range hint
11-23: Consider adding documentation for concurrent access patternsThe class has a good architecture with proper dependency injection and separation of concerns. However, it would benefit from documentation explaining:
- The thread safety approach for
groupMembers- When and why barrier operations are used
- The caching strategy for group members
This will help maintainers understand the concurrent access patterns.
Data/Data/Repository/ExpenseRepository.swift (2)
68-73: UseEquatableconformance for expense comparisonThe
hasExpenseChangedmethod manually compares each property of theExpenseobject. This can be error-prone and hard to maintain as theExpensestructure evolves.Consider making
Expenseconform to theEquatableprotocol and using a simple comparison:private func hasExpenseChanged(_ expense: Expense, oldExpense: Expense) -> Bool { return oldExpense != expense }This approach simplifies the code and reduces the risk of missing property comparisons.
Line range hint
75-80: Handle potential nilgroup.idbefore updating expenseIn the
updateExpensemethod, you guard against a nilgroupIdbut don't provide feedback or throw an error if it's nil. This could lead to silent failures.Consider throwing an error or handling the nil case explicitly to ensure that the calling code is aware of the failure.
public func updateExpense(group: Groups, expense: Expense, oldExpense: Expense, type: ActivityType) async throws { guard let groupId = group.id else { + throw NSError(domain: "ExpenseRepository", code: 0, userInfo: [NSLocalizedDescriptionKey: "Group ID is nil"]) + } try await store.updateExpense(groupId: groupId, expense: expense) try await addActivityLogForExpense(group: group, expense: expense, oldExpense: oldExpense, type: type) }🧰 Tools
🪛 SwiftLint
[Warning] 41-41: Function should have 5 parameters or less: it currently has 6
(function_parameter_count)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (3)
25-25: Confirm the use ofUIImagewith SwiftUIYou're using
UIImageforexpenseImagein a SwiftUI context. Consider using SwiftUI'sImagetype or ensure thatUIImageis compatible with your SwiftUI views to maintain consistency and take full advantage of SwiftUI features.
363-364: Refactor image processing to avoid code duplicationThe image resizing and compression logic is duplicated in both
addExpenseandupdateExpensemethods. Extract this code into a helper method to improve maintainability and reduce repetition.Extract the image processing into a helper method:
private func prepareImageData() -> Data? { guard let resizedImage = expenseImage?.aspectFittedToHeight(200) else { return nil } return resizedImage.jpegData(compressionQuality: 0.2) }Update your methods to use the helper:
// In addExpense(group:expense:) - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let imageData = prepareImageData() // In updateExpense(group:expense:oldExpense:) - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let imageData = prepareImageData()Also applies to: 411-412
363-364: Consider adjusting the image compression qualityThe JPEG compression quality is set to
0.2, which may result in low image quality. Consider increasing the compression quality to0.5or0.7to achieve a better balance between image quality and file size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/date.icon (1).svgis excluded by!**/*.svgSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/date.icon.svgis excluded by!**/*.svgSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/camera.icon (1).pngis excluded by!**/*.pngSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/camera.icon (2).pngis excluded by!**/*.png
📒 Files selected for processing (15)
Data/Data/Extension/Date+Extension.swift(1 hunks)Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Helper/Firebase/StorageManager.swift(1 hunks)Data/Data/Model/Expense.swift(2 hunks)Data/Data/Repository/ExpenseRepository.swift(2 hunks)Data/Data/Repository/GroupRepository.swift(4 hunks)Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/Contents.json(1 hunks)Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/Contents.json(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(7 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(7 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(2 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/Contents.json
- Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/Contents.json
🧰 Additional context used
🪛 SwiftLint
Data/Data/Repository/ExpenseRepository.swift
[Warning] 41-41: Function should have 5 parameters or less: it currently has 6
(function_parameter_count)
🔇 Additional comments (13)
Data/Data/Helper/Firebase/StorageManager.swift (2)
15-15: LGTM! The new enum case follows existing conventions.
The addition of the expense case aligns well with the PR objective and maintains consistency with existing cases.
23-24: Verify storage path compatibility.
While the path naming follows the existing convention, ensure that "expense_images" is compatible with Firebase Storage path requirements and organizational policies.
✅ Verification successful
Let me gather more information about the storage path patterns and related code.
Based on the gathered information, I can now provide the final response:
Storage path follows consistent naming pattern and is safe to use
The expense_images path follows the established pattern used throughout the StorageManager, consistent with other cases:
user_imagesgroup_imagesexpense_images
The path uses simple alphanumeric characters with underscores, which is compatible with Firebase Storage path requirements. The consistent naming pattern across all cases indicates an established convention within the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar path patterns are used consistently across the codebase
# and if there are any existing guidelines for storage paths
# Search for storage path definitions
rg -g '*.swift' 'pathName.*images"'
# Look for any storage path related documentation
fd -g 'README*' -X grep -l 'storage' || fd -g '*.md' -X grep -l 'storage'
Length of output: 124
Script:
#!/bin/bash
# Search for the entire enum and switch case implementation to understand the context
ast-grep --pattern 'enum StorageFolder {
$$$
}'
# Search for the complete pathName implementation
ast-grep --pattern 'var pathName: String {
$$$
}'
# Check the content of the storage documentation
cat docs/index.md
Length of output: 13918
Data/Data/Model/Expense.swift (2)
27-35: LGTM! Well-structured initializer update
The initializer changes are well-implemented:
- Maintains backward compatibility with optional parameter
- Logical parameter ordering
- Consistent with existing style
50-50: LGTM! Consistent coding key convention
The new coding key follows the established snake_case convention for JSON serialization.
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (2)
100-100: Verify the monthWithYear property implementation
The code now uses a new monthWithYear computed property from the Date extension. While this change simplifies the date grouping logic, we should ensure the property is properly implemented.
Let's verify the implementation of this property:
✅ Verification successful
The monthWithYear property is correctly implemented and used consistently
The monthWithYear property is properly implemented in Date+Extension.swift using a standard DateFormatter with "MMMM yyyy" format (e.g., "November 2024"). The property is used consistently across the codebase for date grouping in activity logs, expenses, and transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the monthWithYear property definition
ast-grep --pattern 'extension Date {
$$$
var monthWithYear: $_ {
$$$
}
$$$
}'
# Backup: Search using ripgrep if the property is defined differently
rg -p "monthWithYear" --type swift
Length of output: 656
Script:
#!/bin/bash
# Let's check the implementation of monthWithYear in Date+Extension.swift
rg -A 5 "var monthWithYear" Data/Data/Extension/Date+Extension.swift
Length of output: 248
Line range hint 82-97: Consider adding support for attachment-related activities
Since this PR adds expense attachment support, the handleActivityItemTap method might need to handle new activity types related to expense attachments (e.g., attachment added, removed, or updated).
Let's check if there are any new activity types we need to handle:
Consider adding cases for attachment-related activities if they're defined in the ActivityType enum.
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (1)
105-106: LGTM! Clean styling implementation.
Good optimization by simplifying the view hierarchy and applying styling directly instead of using an overlay.
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
239-239: Verify date formatting consistency across expense grouping.
The change to use monthWithYear property aligns with the standardized date formatting approach. However, let's verify that all expense date grouping operations use this consistent format.
✅ Verification successful
Let me check the implementation of monthWithYear to ensure it's properly standardized.
Date formatting is consistently implemented across expense grouping
The verification shows that:
monthWithYearis properly implemented as a Date extension with a standardized format "MMMM yyyy"- It's consistently used across all relevant views:
- GroupHomeViewModel (expense grouping)
- GroupTransactionListViewModel (transactions)
- ActivityLogViewModel (activity logs)
- Only one legacy date formatting remains in ActivityLogView which uses a different context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct date formatting in expense grouping logic
# across the codebase to ensure consistency.
# Search for date formatting patterns in Swift files
rg -t swift 'dateFormatter.*month|Date.*format.*month'
# Search for monthWithYear usage to verify consistent adoption
rg -t swift 'monthWithYear'
Length of output: 812
Script:
#!/bin/bash
# Check the implementation of monthWithYear
rg -A 5 -B 2 'var monthWithYear: String'
Length of output: 570
Data/Data/Repository/GroupRepository.swift (1)
23-23: Well-implemented concurrent queue for thread safety!
Good practice using a concurrent queue with a descriptive label for managing access to shared state.
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (2)
211-211: LGTM: Date formatting refactor looks clean.
The switch to using the monthWithYear computed property from Date extension makes the code more maintainable and consistent.
Also applies to: 218-218
Line range hint 1-324: Implementation for expense attachment support appears to be missing.
While the PR's objective is to add expense attachment support, this view model doesn't contain any changes related to handling attachments or images for expenses. Consider adding:
- Properties to track attachment state
- Methods to handle attachment selection/preview
- Logic to upload/download attachments
- UI state updates for attachment loading
Let's verify if attachment-related code exists in other files:
Consider implementing the following components for a robust attachment system:
- Attachment handling methods in the view model
- Image upload/download service integration
- Progress tracking for uploads
- Error handling for failed uploads/downloads
- Cache management for downloaded images
Would you like me to help design these components?
Data/Data/Repository/ExpenseRepository.swift (1)
58-59: Verify that image changes trigger expense updates correctly
The hasExpenseChanged method checks whether the expense has changed before updating. Ensure that changes to the image (addition, deletion, or modification) are properly detected so that the expense is updated in the store when image changes occur.
Confirm that hasExpenseChanged accounts for image-related changes and that the expense is updated accordingly.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
11-12: Added imports are appropriate
The imports of AVFoundation and SwiftUI are necessary for the new functionalities related to camera access and SwiftUI views.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
Data/Data/Store/ExpenseStore.swift (1)
24-26: Consider providing a higher-level API for better usabilityThe current implementation exposes Firebase-specific types (
DocumentReference) in the public API, which could make the code harder to test and maintain. Consider providing a higher-level API that abstracts these details.+func addExpense(groupId: String, expense: Expense) async throws { + let documentRef = expenseReference(groupId: groupId).document() + try await addExpense(documentRef: documentRef, expense: expense) + return documentRef.documentID +} func addExpense(documentRef: DocumentReference, expense: Expense) async throws { try documentRef.setData(from: expense) }Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (1)
53-55: LGTM! Consider adding user feedback for edge cases.The nil check for settings URL is a good safety improvement. However, in the rare case where the settings URL is invalid, the user receives no feedback about why they can't proceed.
Consider adding an else clause to handle the edge case:
if let settingsURL = URL(string: UIApplication.openSettingsURLString) { UIApplication.shared.open(settingsURL) +} else { + self?.showToastFor(toast: ToastPrompt( + type: .error, + title: "Error", + message: "Unable to open Settings. Please enable camera access manually." + )) }Data/Data/Repository/ExpenseRepository.swift (1)
45-64: Consider enhancing error handling for image operationsWhile the implementation handles different image update scenarios well, consider adding more specific error handling for image operations.
public func updateExpenseWithImage(imageData: Data?, newImageUrl: String?, group: Groups, expense: (new: Expense, old: Expense), type: ActivityType) async throws -> Expense { var updatedExpense = expense.new + + enum ImageUpdateError: Error { + case imageUploadFailed + case imageDeletionFailed + } if let imageData { let uploadedImageUrl = try await uploadImage(imageData: imageData, expense: updatedExpense) + guard !uploadedImageUrl.isEmpty else { + throw ImageUpdateError.imageUploadFailed + } updatedExpense.imageUrl = uploadedImageUrl } else if let currentUrl = updatedExpense.imageUrl, newImageUrl == nil { try await storageManager.deleteImage(imageUrl: currentUrl) updatedExpense.imageUrl = nil } else if let newImageUrl = newImageUrl { + guard !newImageUrl.isEmpty else { + throw ImageUpdateError.imageUploadFailed + } updatedExpense.imageUrl = newImageUrl }Splito/UI/Home/Expense/AddExpenseView.swift (2)
118-119: Consider standardizing the layout spacingThe spacing between ExpenseDetailRow components varies. Consider using consistent spacing values for better visual hierarchy:
- Between group selection and description
- Between amount and payment details
VStack(spacing: 16) { - ExpenseDetailRow(name: $viewModel.expenseName, focusedField: focusedField, subtitle: "With you and:", + VStack(spacing: 24) { // Increased spacing for major sections + ExpenseDetailRow(name: $viewModel.expenseName, focusedField: focusedField, subtitle: "With you and:", inputValue: viewModel.selectedGroup?.name ?? "Select group", showButton: true, onTap: viewModel.handleGroupBtnAction)Also applies to: 122-123, 132-136
237-238: Consider more flexible date range handlingThe maximum date calculation is hardcoded to the current year. Consider:
- Making the date range configurable through parameters
- Using a computed property for dynamic updates
- private let maximumDate = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date() + private var maximumDate: Date { + Calendar.current.startOfDay(for: Date()) + }Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
365-366: Consider increasing the image compression quality for better image clarityCompressing the image with a
compressionQualityof 0.2 may result in noticeably low image quality, affecting the user experience. To achieve a better balance between image size and quality, consider using a higher compression quality, such as 0.6 or 0.7.Also applies to: 413-414
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Data/Data/Repository/ExpenseRepository.swift(2 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(7 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(7 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Repository/GroupRepository.swift
🔇 Additional comments (9)
Data/Data/Store/ExpenseStore.swift (1)
17-22: Verify the necessity of internal access for expenseReference
The method's access level has been changed from private to internal. While this might be needed for image handling coordination, exposing Firestore collection references could lead to inconsistent data access patterns.
✅ Verification successful
Internal access for expenseReference is appropriate
The internal access level is justified as it follows a clean architecture pattern where:
ExpenseRepositoryacts as a public-facing repository layerExpenseStoreserves as an internal implementation detail- The reference is only used within the data layer for proper encapsulation
- Usage is controlled and consistent through dependency injection (via
AppAssembly)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this reference is used outside this file
rg -p "expenseReference" --type swift --glob '!**/ExpenseStore.swift'
Length of output: 202
Script:
#!/bin/bash
# Let's check the ExpenseRepository usage to understand the context better
rg -p "class ExpenseRepository" -A 10 --type swift
# Also check if there are any tests or other legitimate uses
rg -p "ExpenseRepository" --type swift
Length of output: 1641
Data/Data/Repository/ExpenseRepository.swift (5)
14-14: LGTM: StorageManager dependency properly injected
The StorageManager dependency is correctly injected following the existing pattern and will be used for handling image uploads and deletions.
17-31: LGTM: Improved expense creation flow with proper image handling
The implementation correctly:
- Generates the document ID first
- Handles image upload before storing the expense
- Updates the expense with the image URL if provided
66-69: Previous comment about nil expense.id handling still applies
The current implementation returns an empty string when expense.id is nil, which could lead to silent failures.
71-77: LGTM: Comprehensive change detection implementation
The method thoroughly compares all relevant fields, including the new imageUrl field, in a clear and maintainable way.
14-14: Verify consistent image handling across the codebase
Let's verify that image-related operations are handled consistently across the codebase.
Also applies to: 17-31, 45-64, 66-69
✅ Verification successful
Image handling is consistently implemented across the codebase
Based on the verification results:
- All image operations are centralized through the
StorageManagerclass - Image handling is consistently used in repositories (
ExpenseRepository,GroupRepository,UserRepository) - The
StorageManagerprovides a unified interface for image upload/delete operations - Image-related operations are properly categorized using
ImageStoreTypeenum - All UI components consistently use the same image handling patterns through repository layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent image handling patterns
# Look for image-related operations
echo "Checking for image upload/delete operations:"
rg -l "uploadImage|deleteImage|imageUrl" --type swift
# Check for potential image handling in other repositories
echo "\nChecking for other image handling implementations:"
rg -l "class.*Repository.*{" --type swift | xargs rg "image|photo|attachment" -l
# Look for StorageManager usage
echo "\nChecking StorageManager usage patterns:"
rg "StorageManager" --type swift -A 3
Length of output: 12610
Splito/UI/Home/Expense/AddExpenseView.swift (2)
76-83: LGTM: Well-structured image picker implementation
The image picker implementation follows iOS best practices with proper sheet presentation and confirmation dialog handling.
Line range hint 163-191: Verify the necessity of forced tap gesture
The use of onTapGestureForced might interfere with the natural touch handling of buttons and text fields. Consider using regular onTapGesture unless there's a specific reason for forcing the gesture.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
227-231: Ensure UI updates occur on the main thread and handle permission denial appropriately
In the checkCameraPermission method:
- Thread Safety: The closure in
AVCaptureDevice.requestAccessmay run on a background thread. Wrap theauthorized()call insideDispatchQueue.main.asyncto ensure UI updates occur on the main thread. - Handle Permission Denial: If permission is not granted (
granted == false), consider informing the user that camera access is required for this feature.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift (3)
89-91: Good URL safety, but consider these improvementsThe URL handling is properly implemented with optional binding. However, consider these enhancements:
- Move the alert dismissal before opening settings
- Add completion handler to handle URL opening failures
- Use
[weak self]explicitly for better memory management- if let settingsURL = URL(string: UIApplication.openSettingsURLString) { - UIApplication.shared.open(settingsURL) - } - self?.showAlert = false + self?.showAlert = false + if let settingsURL = URL(string: UIApplication.openSettingsURLString) { + UIApplication.shared.open(settingsURL, options: [:]) { success in + if !success { + self?.showAlertFor(message: "Could not open Settings") + } + } + }
Line range hint
1-400: Consider splitting responsibilities for better maintainabilityThe
UserProfileViewModelhandles multiple concerns which could be separated into dedicated services:
- Extract authentication logic into an
AuthenticationService- Move image handling into an
ImageService- Create a dedicated
ProfileManagerfor profile data operationsThis separation would:
- Improve testability
- Make the code more maintainable
- Allow reuse of these services across the app
Example structure:
// AuthenticationService.swift protocol AuthenticationServiceProtocol { func reAuthenticate(for loginType: LoginType) async throws -> AuthCredential } // ImageService.swift protocol ImageServiceProtocol { func checkCameraPermission() async throws func handleImageSelection(source: ImageSource) async throws -> UIImage } // ProfileManager.swift protocol ProfileManagerProtocol { func updateProfile(user: User, image: UIImage?) async throws func deleteProfile() async throws } // Simplified ViewModel class UserProfileViewModel: BaseViewModel, ObservableObject { private let authService: AuthenticationServiceProtocol private let imageService: ImageServiceProtocol private let profileManager: ProfileManagerProtocol // Reduced complexity with dependency injection }
Line range hint
1-400: Improve error handling consistency and structureThe current error handling could be enhanced for better maintainability and consistency:
- Use structured error types instead of string comparisons
- Centralize error messages
- Migrate completion handlers to async/await
Example implementation:
enum UserProfileError: LocalizedError { case reauthenticationRequired case invalidCredentials case permissionDenied case networkError(Error) var errorDescription: String? { switch self { case .reauthenticationRequired: return "Recent authentication is required" case .invalidCredentials: return "Invalid credentials provided" case .permissionDenied: return "Camera access is required" case .networkError(let error): return "Network error: \(error.localizedDescription)" } } } // Example usage: func deleteUser() async throws { guard let user = preference.user else { throw UserProfileError.invalidCredentials } do { try await userRepository.deleteUser(id: user.id) preference.clearPreferenceSession() goToOnboardScreen() } catch { if error.localizedDescription.contains(REQUIRE_AGAIN_LOGIN_TEXT) { throw UserProfileError.reauthenticationRequired } throw UserProfileError.networkError(error) } }Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
Line range hint
364-421: Improve image processing and validationThe image handling implementation could be enhanced:
- Magic numbers should be constants
- Large images could cause memory issues
- Missing image size validation
Apply these improvements:
+private enum ImageConstants { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 + static let maxImageSizeInBytes = 5 * 1024 * 1024 // 5MB +} +private func processImage(_ image: UIImage?) -> Data? { + guard let image = image else { return nil } + let resizedImage = image.aspectFittedToHeight(ImageConstants.maxHeight) + guard let imageData = resizedImage?.jpegData(compressionQuality: ImageConstants.compressionQuality), + imageData.count <= ImageConstants.maxImageSizeInBytes else { + showAlertFor(alert: .init( + title: "Image Too Large", + message: "Please select an image smaller than 5MB", + positiveBtnTitle: "OK" + )) + return nil + } + return imageData +} private func addExpense(group: Groups, expense: Expense) async -> Bool { guard let groupId = group.id else { return false } - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let imageData = processImage(expenseImage) do { showLoader = trueApply similar changes to
updateExpensemethod.Splito/UI/Home/Expense/AddExpenseView.swift (4)
76-80: Consider adding error handling for image picker failuresThe image picker implementation should handle potential failures when accessing the camera or photo library.
.sheet(isPresented: $viewModel.showImagePicker) { ImagePickerView(cropOption: .square, sourceType: !viewModel.sourceTypeIsCamera ? .photoLibrary : .camera, - image: $viewModel.expenseImage, isPresented: $viewModel.showImagePicker) + image: $viewModel.expenseImage, + isPresented: $viewModel.showImagePicker, + onError: { error in + viewModel.toast = .init(style: .error, message: error.localizedDescription) + }) }
132-136: Consider using enum for input types to improve type safetyThe ExpenseDetailRow is used for multiple purposes with different input types. Using an enum would make the intent clearer and prevent potential misuse.
+ enum ExpenseDetailRowType { + case text(Binding<String>) + case button(String, () -> Void) + } struct ExpenseDetailRow: View { - @Binding var name: String + let type: ExpenseDetailRowType // ... rest of the implementation }
250-257: Add accessibility labels for better VoiceOver supportThe date display should have appropriate accessibility labels to improve the experience for VoiceOver users.
if !isForAddExpense { Text(date.longDate) .font(.subTitle2()) .foregroundStyle(primaryText) .frame(maxWidth: .infinity, alignment: .leading) + .accessibilityLabel("Selected date: \(date.longDate)") } else { DateDisplayView(date: $date) }
326-343: Add haptic feedback for better user interactionAdding haptic feedback when tapping the image picker button would improve the user experience.
Button(action: { + UIImpactFeedbackGenerator(style: .light).impactOccurred() handleImageBtnTap() }) { // ... rest of the implementation }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(7 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(7 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Expense/AddExpenseView.swift
[Warning] 495-495: File should contain 400 lines or less: currently contains 495
(file_length)
🔇 Additional comments (2)
Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
Line range hint 11-35: LGTM: Well-structured property declarations
The new properties for image handling are well-organized with appropriate access levels and follow SwiftUI conventions.
436-439: LGTM: Improved change detection logic
The change detection has been improved by removing the updatedBy field and correctly including the imageUrl comparison.
cp-amisha-i
force-pushed
the
main
branch
2 times, most recently
from
c0a8cc4 to
ce8c6f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (3)
30-36: Consider adding loading and error states for image loading.While the image view implementation is solid, consider enhancing the user experience by:
- Adding a loading placeholder while the image loads
- Handling potential image loading failures
Example implementation:
if let imageUrl = viewModel.expense?.imageUrl { ExpenseImageView(showZoomableImageView: $showZoomableImageView, imageUrl: imageUrl) + .overlay { + AsyncImageLoadingView(url: imageUrl) + } .aspectRatio(16/9, contentMode: .fit) .cornerRadius(12) }
59-71: Simplify toolbar conditional logic.The nested conditionals could be simplified for better readability.
Consider refactoring to:
-if viewModel.viewState != .loading { - if (viewModel.expense?.isActive ?? false) && (viewModel.group?.isActive ?? false) { - ToolbarItem(placement: .topBarTrailing) { - ToolbarButtonView(imageIcon: .binIcon, onClick: viewModel.handleDeleteButtonAction) - } - ToolbarItem(placement: .topBarTrailing) { - ToolbarButtonView(imageIcon: .editPencilIcon, onClick: viewModel.handleEditBtnAction) - } - } else { - ToolbarItem(placement: .topBarTrailing) { - RestoreButton(onClick: viewModel.handleRestoreButtonAction) - } - } -} +if viewModel.viewState == .loading { return } + +let isActiveState = (viewModel.expense?.isActive ?? false) && (viewModel.group?.isActive ?? false) +ToolbarItem(placement: .topBarTrailing) { + if isActiveState { + HStack { + ToolbarButtonView(imageIcon: .binIcon, onClick: viewModel.handleDeleteButtonAction) + ToolbarButtonView(imageIcon: .editPencilIcon, onClick: viewModel.handleEditBtnAction) + } + } else { + RestoreButton(onClick: viewModel.handleRestoreButtonAction) + } +}
74-77: Enhance image zoom transition animation.The zoomable image view implementation could benefit from proper animation configuration.
Consider creating a shared namespace for smooth transitions:
struct ExpenseDetailsView: View { + @Namespace private var imageNamespace // ... existing code ... .navigationDestination(isPresented: $showZoomableImageView) { if let imageUrl = viewModel.expense?.imageUrl { - ZoomableImageView(imageUrl: imageUrl, animationNamespace: Namespace()) + ZoomableImageView(imageUrl: imageUrl, animationNamespace: imageNamespace) + .transition(.opacity.combined(with: .scale)) } }Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (3)
72-78: LGTM! Consider enhancing error handling feedback.The async Task implementation for save action is well structured. However, consider providing more specific error feedback to users instead of a generic save failed error.
Task { let isSucceed = await viewModel.handleSaveAction() if isSucceed { dismiss() } else { - viewModel.showSaveFailedError() + viewModel.showSaveFailedError(with: viewModel.errorMessage ?? "Failed to save payment") } }
185-185: Consider making date constraints configurable.The maximum date is hardcoded to the current year. Consider making this configurable through an initializer parameter to improve reusability.
- private let maximumDate = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date() + private let maximumDate: Date + + init(date: Binding<Date>, isForAddExpense: Bool = false, + maximumDate: Date = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date()) { + self._date = date + self.isForAddExpense = isForAddExpense + self._tempDate = State(initialValue: date.wrappedValue) + self.maximumDate = maximumDate + }
250-250: Add localization for "Today" string.Consider localizing the "Today" string for better internationalization support.
- Text(date.isToday() ? "Today" : date.shortDate) + Text(date.isToday() ? "Today".localized : date.shortDate)Splito/UI/Home/Expense/AddExpenseView.swift (2)
109-141: Add accessibility identifiers for UI testingThe ExpenseInfoView contains important user input fields but lacks accessibility identifiers which are crucial for UI testing.
Consider adding accessibility identifiers to key elements:
VStack(spacing: 16) { ExpenseDetailRow(name: $viewModel.expenseName, focusedField: focusedField, subtitle: "With you and:", inputValue: viewModel.selectedGroup?.name ?? "Select group", - showButton: true, onTap: viewModel.handleGroupBtnAction) + showButton: true, onTap: viewModel.handleGroupBtnAction) + .accessibilityIdentifier("group-selection-row") ExpenseDetailRow(name: $viewModel.expenseName, focusedField: focusedField, - subtitle: "Description", field: .expenseName) + subtitle: "Description", field: .expenseName) + .accessibilityIdentifier("expense-description-row")
Line range hint
1-416: Consider splitting the file into smaller componentsThe file currently contains 416 lines, which exceeds the recommended maximum of 400 lines. Consider moving some components to separate files:
- Move
ZoomableImageViewto a separate file- Move
ExpenseImageViewandExpenseImagePickerViewto a new file focused on image handlingThis will improve maintainability and make the code easier to test.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 417-417: File should contain 400 lines or less: currently contains 417
(file_length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(4 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(3 hunks)
🔇 Additional comments (3)
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
15-15: LGTM: State variable properly declared.
The state variable follows SwiftUI best practices and is appropriately scoped.
113-114: LGTM: Consistent styling applied.
The background and corner radius styling matches the design system and improves visual hierarchy.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (1)
155-177: LGTM! Clean implementation.
The PaymentDateRow implementation provides a good separation of concerns and consistent styling.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (2)
87-91: Consider adding input validation.While the async/await refactoring is good, consider adding validation for the group name before proceeding with create/update operations.
func handleDoneAction() async -> Bool { + guard !groupName.trimming(spaces: .leadingAndTrailing).isEmpty else { + showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: "Group name cannot be empty")) + return false + } if let group { await updateGroup(group: group) } else { await createGroup() } }
Line range hint
99-143: Consider optimizing image handling.While the image resizing and compression are good, consider:
- Moving image processing to a background queue to prevent UI blocking
- Implementing proper cleanup of image data after upload
private func createGroup() async -> Bool { guard let userId = preference.user?.id else { return false } let memberBalance = GroupMemberBalance(id: userId, balance: 0, totalSummary: []) let group = Groups(name: groupName.trimming(spaces: .leadingAndTrailing), createdBy: userId, updatedBy: userId, imageUrl: nil, members: [userId], balances: [memberBalance], createdAt: Timestamp(), updatedAt: Timestamp()) - let resizedImage = profileImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let imageData = await Task.detached { + let resizedImage = self.profileImage?.aspectFittedToHeight(200) + return resizedImage?.jpegData(compressionQuality: 0.2) + }.value do { showLoader = true let group = try await groupRepository.createGroup(group: group, imageData: imageData) + // Clean up the image data + self.profileImage = nil NotificationCenter.default.post(name: .addGroup, object: group) showLoader = false return true } catch {Splito/UI/Home/Expense/AddExpenseView.swift (2)
109-141: Consider extracting spacing constants and improving component separationThe ExpenseInfoView could benefit from:
- Extracting hardcoded spacing values into constants
- Breaking down into smaller, more focused components
Consider creating a separate spacing constants file:
enum Spacing { static let defaultPadding: CGFloat = 16 static let rowSpacing: CGFloat = 16 static let horizontalInset: CGFloat = 1 }And refactoring the view to use these constants:
-VStack(spacing: 16) { +VStack(spacing: Spacing.rowSpacing) { // ... content ... } -.padding(.horizontal, 1) +.padding(.horizontal, Spacing.horizontalInset)
303-416: Document gesture handling and extract magic numbersThe ZoomableImageView implementation is robust but would benefit from:
- Documentation explaining the gesture handling logic
- Extraction of magic numbers into named constants
Consider these improvements:
+/// Constants for zoom and pan limits +private enum ZoomLimits { + static let minScale: CGFloat = 1.0 + static let maxScale: CGFloat = 4.0 + static let defaultDoubleTapScale: CGFloat = 3.0 +} struct ZoomableImageView: View { // ... existing properties ... private func fixOffsetAndScale(geometry: GeometryProxy) { - let newScale: CGFloat = .minimum(.maximum(scale, 1), 4) + let newScale: CGFloat = .minimum(.maximum(scale, ZoomLimits.minScale), ZoomLimits.maxScale) // ... rest of the implementation ... } private func resetZoom() { - scale = lastScale > 1 ? 1 : 3 + scale = lastScale > ZoomLimits.minScale ? ZoomLimits.minScale : ZoomLimits.defaultDoubleTapScale // ... rest of the implementation ... } }Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
201-248: Enhance user experience for camera permission handling.While the implementation is generally good, consider these improvements for better user experience:
- The camera denied case could guide users better by explaining how to enable camera access in settings
- Consider handling the case when settings URL fails to open
case .restricted, .denied: - showAlertFor(alert: .init(title: "Important!", message: "Camera access is required to take picture for expenses", + showAlertFor(alert: .init(title: "Camera Access Required", + message: "To take expense photos, please enable camera access in Settings > Privacy > Camera > Splito", positiveBtnTitle: "Allow", positiveBtnAction: { [weak self] in if let settingsURL = URL(string: UIApplication.openSettingsURLString) { - UIApplication.shared.open(settingsURL) + UIApplication.shared.open(settingsURL) { success in + if !success { + self?.showToastFor(toast: ToastPrompt(type: .error, + title: "Error", + message: "Could not open Settings")) + } + } } self?.showAlert = false }))
313-322: Optimize member validation logic.The validation logic can be made more efficient by using set operations instead of iterating multiple times.
private func validateMembersInGroup(group: Groups, expense: Expense) -> Bool { - for payer in expense.paidBy where !group.members.contains(payer.key) { - return false - } - for memberId in expense.splitTo where !group.members.contains(memberId) { - return false - } - - return true + let memberSet = Set(group.members) + let payerSet = Set(expense.paidBy.keys) + let splitToSet = Set(expense.splitTo) + + return memberSet.isSuperset(of: payerSet) && memberSet.isSuperset(of: splitToSet) }
367-369: Extract image processing constants.Consider extracting magic numbers for image dimensions and compression quality into named constants for better maintainability.
+private enum ImageProcessing { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 +} -let resizedImage = expenseImage?.aspectFittedToHeight(200) -let imageData = resizedImage?.jpegData(compressionQuality: 0.2) +let resizedImage = expenseImage?.aspectFittedToHeight(ImageProcessing.maxHeight) +let imageData = resizedImage?.jpegData(compressionQuality: ImageProcessing.compressionQuality)Also applies to: 415-417
436-441: Consider using Equatable for expense comparison.The current comparison logic could be made more maintainable by implementing
Equatableon theExpensestruct and comparing relevant properties there.+extension Expense: Equatable { + static func == (lhs: Expense, rhs: Expense) -> Bool { + return lhs.name == rhs.name && + lhs.amount == rhs.amount && + lhs.date.dateValue() == rhs.date.dateValue() && + lhs.paidBy == rhs.paidBy && + lhs.imageUrl == rhs.imageUrl && + lhs.splitTo == rhs.splitTo && + lhs.splitType == rhs.splitType && + lhs.splitData == rhs.splitData && + lhs.isActive == rhs.isActive + } +} private func hasExpenseChanged(_ expense: Expense, oldExpense: Expense) -> Bool { - return oldExpense.name != expense.name || oldExpense.amount != expense.amount || - oldExpense.date.dateValue() != expense.date.dateValue() || oldExpense.paidBy != expense.paidBy || - oldExpense.imageUrl != expense.imageUrl || oldExpense.splitTo != expense.splitTo || - oldExpense.splitType != expense.splitType || oldExpense.splitData != expense.splitData || - oldExpense.isActive != expense.isActive + return expense != oldExpense }Splito/Localization/Localizable.xcstrings (5)
783-791: Consider consolidating similar error messages.There are multiple similar error messages for permissions and left group scenarios. Consider using a templated approach with placeholders for better maintainability.
- "This expense could not be deleted. You do not have permission to delete this expense, Sorry!" : { - "extractionState" : "manual" - }, - "This expense could not be edited. You do not have permission to edit this expense, Sorry!" : { - "extractionState" : "manual" - }, + "action_not_allowed_%@_%@" : { + "extractionState" : "manual", + "comment": "First parameter is the entity (expense/payment), second is the action (delete/edit)" + },
804-812: Consolidate left group error messages.Similar error messages for payments involving users who left the group can be consolidated into a single templated string.
- "This payment involves a person who has left the group, and thus it can no longer be deleted. If you wish to change this payment, you must first add that person back to your group." : { - "extractionState" : "manual" - }, - "This payment involves a person who has left the group, and thus it can no longer be edited. If you wish to change this payment, you must first add that person back to your group." : { - "extractionState" : "manual" - }, - "This payment involves a person who has left the group, and thus it can no longer be restored. If you wish to change this payment, you must first add that person back to your group." : { - "extractionState" : "manual" - }, + "payment_action_blocked_member_left_%@" : { + "extractionState" : "manual", + "comment": "Parameter is the action (delete/edit/restore)" + },
912-913: Consider adding comments for translators.The restoration-related strings might benefit from translator comments explaining the context, especially for languages with different grammar structures.
"You can restore it to recover all activities, expenses and payments." : { - "extractionState" : "manual" + "extractionState" : "manual", + "comment": "Shown when a group is deleted and can be restored" }, "This will restore all activities, expenses and payments for this group." : { - "extractionState" : "manual" + "extractionState" : "manual", + "comment": "Confirmation message shown before restoring a group" },Also applies to: 816-817
738-738: Improve clarity of empty state message.The message "Start spending or receiving money to see your payments here." could be more engaging and informative.
- "Start spending or receiving money to see your payments here." : { + "No payments yet! Record your first expense or payment to get started." : { + "extractionState" : "manual", + "comment": "Empty state message for payments list" },
142-146: Add comments for activity-related strings.The "Activities" and "Activity" strings might need context for proper translation in languages with different plural forms.
"Activities" : { - "extractionState" : "manual" + "extractionState" : "manual", + "comment": "Title for the activities list view (plural)" }, "Activity" : { + "extractionState" : "manual", + "comment": "Title for a single activity item" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Splito/Localization/Localizable.xcstrings(29 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(8 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift
🔇 Additional comments (6)
Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift (3)
16-18: LGTM! Clean dependency injection setup.
The dependencies are well-organized and appropriate for the view model's responsibilities.
45-48: Improved camera permission handling with safe practices.
Good improvements:
- Using
DispatchQueue.main.asyncfor UI updates - Safe optional binding for settings URL handling
Also applies to: 53-57
95-97: LGTM! Good error handling separation.
Clean implementation of error feedback mechanism.
Splito/UI/Home/Expense/AddExpenseView.swift (1)
92-98: 🛠️ Refactor suggestion
Enhance error handling in save action
The current implementation could benefit from more robust error handling to provide specific feedback to users.
Consider this improved implementation:
Task {
- let isActionSucceed = await viewModel.handleSaveAction()
- if isActionSucceed {
- dismiss()
- } else {
- viewModel.showSaveFailedError()
- }
+ do {
+ let isActionSucceed = try await viewModel.handleSaveAction()
+ if isActionSucceed {
+ dismiss()
+ }
+ } catch {
+ viewModel.showSaveFailedError(with: error)
+ }
}Likely invalid or redundant comment.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
Line range hint 11-35: LGTM! Well-structured property declarations.
The new properties for image handling are well-organized with appropriate access levels and follow SwiftUI conventions.
Splito/Localization/Localizable.xcstrings (1)
236-240: LGTM: Camera access permission strings are well-defined.
The strings for camera access permissions are clear and specific to their use cases (expenses and profile pictures).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Splito/UI/Home/Expense/AddExpenseView.swift (3)
162-176: Add accessibility support for text inputThe text input implementation should include accessibility labels and hints for better VoiceOver support.
Consider these accessibility enhancements:
TextField("Enter a description", text: $name) .font(.subTitle2()) .foregroundStyle(primaryText) + .accessibilityLabel("Expense description") + .accessibilityHint("Enter a description for your expense") .keyboardType(.default)
204-233: Add haptic feedback for better user interactionConsider adding haptic feedback when the user interacts with the date picker or image selection buttons.
Add a feedback generator:
private struct BottomCardView: View { + private let feedback = UIImpactFeedbackGenerator(style: .light) // ... existing properties ... var body: some View { HStack(spacing: 16) { Spacer() DatePickerView(date: $date, isForAddExpense: true) .onTapGesture { + feedback.impactOccurred() } ExpenseImagePickerView(image: expenseImage, imageUrl: expenseImageUrl) .onTapGesture { + feedback.impactOccurred() handleImageBtnTap() } } } }
288-296: Optimize image caching strategyThe current Kingfisher implementation could benefit from additional caching options for better performance.
Consider these optimizations:
KFImage(url) .placeholder { _ in ImageLoaderView() } .setProcessor(DownsamplingImageProcessor(size: UIScreen.main.bounds.size)) .cacheMemoryOnly() + .memoryCacheExpiration(.days(7)) + .fadeTransition(.linear(0.3)) + .onFailure { error in + print("Image loading failed: \(error.localizedDescription)") + } .resizable() .aspectRatio(contentMode: .fill)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)
🔇 Additional comments (2)
Splito/UI/Home/Expense/AddExpenseView.swift (2)
109-141: LGTM! Well-structured view implementation
The ExpenseInfoView implementation is clean, consistent, and follows SwiftUI best practices with proper spacing and layout.
92-98: 🛠️ Refactor suggestion
Enhance error handling in save action
The current implementation could benefit from more specific error handling and user feedback.
Consider this improved implementation:
Task {
- let isActionSucceed = await viewModel.handleSaveAction()
- if isActionSucceed {
- dismiss()
- } else {
- viewModel.showSaveFailedError()
- }
+ do {
+ let isActionSucceed = try await viewModel.handleSaveAction()
+ if isActionSucceed {
+ dismiss()
+ }
+ } catch let error as ExpenseError {
+ viewModel.showSaveFailedError(with: error.localizedDescription)
+ } catch {
+ viewModel.showSaveFailedError(with: "Failed to save expense. Please try again.")
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
Splito/UI/Home/Expense/AddExpenseView.swift (3)
78-82: Add error handling for image picker failuresWhile the image picker implementation is good, consider adding error handling for cases where the image picker fails or user denies camera/photo library permissions.
.sheet(isPresented: $viewModel.showImagePicker) { ImagePickerView(cropOption: .square, sourceType: !viewModel.sourceTypeIsCamera ? .photoLibrary : .camera, - image: $viewModel.expenseImage, isPresented: $viewModel.showImagePicker) + image: $viewModel.expenseImage, + isPresented: $viewModel.showImagePicker, + onError: { error in + viewModel.handleImagePickerError(error) + }) }
303-332: Add loading state handling for image displayConsider adding a loading state indicator while the image is being prepared for display, especially for remote images.
struct ImageDisplayView: View { + @State private var isLoading = false var image: UIImage? var imageUrl: String? @Namespace var animationNamespace var body: some View { GeometryReader { geometry in ZStack { + if isLoading { + ProgressView() + .progressViewStyle(.circular) + } if #available(iOS 18.0, *) { if let image { ZoomableImageView(image: image, geometry: geometry) .matchedGeometryEffect(id: "image", in: animationNamespace) .navigationTransition(.zoom(sourceID: "zoom", in: animationNamespace)) } else if let imageUrl { ZoomableImageView(imageUrl: imageUrl, geometry: geometry) + .onAppear { isLoading = true } + .onSuccess { _ in isLoading = false } + .onFailure { _ in isLoading = false } .matchedGeometryEffect(id: "image", in: animationNamespace) .navigationTransition(.zoom(sourceID: "zoom", in: animationNamespace)) }
286-293: Optimize memory management for image loadingWhile Kingfisher's
cacheMemoryOnly()is used, consider adding more aggressive memory management:
- Add memory warning handlers to clear the image cache
- Consider implementing a maximum cache size
- Add image size limits for uploads
Example implementation:
// Add to your app delegate or scene delegate NotificationCenter.default.addObserver( forName: UIApplication.didReceiveMemoryWarningNotification, object: nil, queue: .main ) { _ in ImageCache.default.clearMemoryCache() } // Configure cache size ImageCache.default.memoryStorage.config.totalCostLimit = 300 * 1024 * 1024 // 300MBBaseStyle/BaseStyle/Views/ZoomableImageView.swift (4)
51-55: Avoid passingGeometryProxyto the initializerPassing
GeometryProxyas a parameter is unconventional in SwiftUI and may lead to issues. Consider usingGeometryReaderwithin thebodyto access geometry information.Refactored code:
public init(image: UIImage? = nil, imageUrl: String? = nil) { self.image = image self.imageUrl = imageUrl - self.geometry = geometry } public var body: some View { + GeometryReader { geometry in ZStack { // Use geometry.size where needed } + } }
59-95: Reduce code duplication in image view configurationThere is significant code duplication when configuring the image view for local and remote images. To improve maintainability, consider extracting common modifiers and gestures into a shared extension or applying them after the conditional blocks.
Example refactor:
public var body: some View { GeometryReader { geometry in let imageView: AnyView if let image = image { imageView = AnyView( Image(uiImage: image) .resizable() .scaledToFit() ) loadedImage = image } else if let imageUrl = imageUrl, let url = URL(string: imageUrl) { imageView = AnyView( KFImage(url) .placeholder { _ in ImageLoaderView() } .setProcessor(DownsamplingImageProcessor(size: UIScreen.main.bounds.size)) .cacheMemoryOnly() .onSuccess { result in loadedImage = result.image } .resizable() .scaledToFit() ) } else { imageView = AnyView(EmptyView()) } imageView .position(x: geometry.size.width / 2, y: geometry.size.height / 2) .scaleEffect(scale, anchor: scaleAnchor) .offset(offset) .animation(.spring(), value: offset) .animation(.spring(), value: scale) .gesture(dragGesture) .gesture(magnificationGesture) .simultaneousGesture( TapGesture(count: 2).onEnded { _ in resetZoom() } ) } }
124-124: Remove trailing whitespaceLine 124 contains trailing whitespace as indicated by the static analysis tool. Please remove the unnecessary whitespace to comply with code style guidelines.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 124-124: Lines should not have trailing whitespace
(trailing_whitespace)
23-23: InitializeloadedImageappropriatelyThe
@StatepropertyloadedImageis initialized with an emptyUIImage(), which may not be necessary and could lead to unintended behavior. Consider makingloadedImageoptional (UIImage?) and initializing it when needed.Apply this change:
- @State var loadedImage: UIImage = UIImage() + @State var loadedImage: UIImage?Ensure to handle cases where
loadedImagemight benilin your code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj(5 hunks)BaseStyle/BaseStyle/Views/ZoomableImageView.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift
[Warning] 124-124: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (4)
Splito/UI/Home/Expense/AddExpenseView.swift (2)
109-141: LGTM! Well-structured view composition
The ExpenseInfoView provides a clean separation of concerns and consistent UI structure using ExpenseDetailRow components.
11-11: LGTM! Consider specifying Kingfisher version
The integration of Kingfisher for image handling is appropriate. However, it's recommended to specify the version in the package dependencies to ensure consistency.
Also applies to: 38-41
BaseStyle/BaseStyle.xcodeproj/project.pbxproj (2)
12-12: LGTM: ZoomableImageView integration looks good.
The ZoomableImageView.swift file is properly integrated into the project structure and build phases, which aligns with the PR objective of adding expense attachment support.
Let's verify the implementation details:
Also applies to: 75-75, 509-509
✅ Verification successful
Implementation of ZoomableImageView is well-designed and complete
The implementation includes all necessary features for expense attachment viewing:
- Proper zoom functionality with pinch-to-zoom gesture (1x to 4x scale)
- Pan/drag gesture for moving zoomed images
- Double-tap to toggle between normal and zoomed states
- Handles both local UIImage and remote URLs via Kingfisher
- Maintains image bounds within screen limits
- Smooth animations and proper gesture handling
- Image caching and downsampling for performance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of ZoomableImageView
# Expected: SwiftUI view with zoom/pan gestures and image loading capabilities
# Search for the implementation
ast-grep --pattern 'struct ZoomableImageView: View {
$$$
}'
# Check for gesture recognizers
rg -A 5 'gesture|zoom|scale|drag' ZoomableImageView.swift
# Check for image loading
rg -A 5 'Image|UIImage|URL' ZoomableImageView.swift
Length of output: 305
Script:
#!/bin/bash
# Let's first locate the ZoomableImageView.swift file
fd ZoomableImageView.swift
# Then check its content if found
fd ZoomableImageView.swift -x cat {}
Length of output: 5738
15-15: Clarify the purpose of RestoreButton.
While the file is properly integrated, its purpose seems unrelated to expense attachment functionality. Please clarify how this component contributes to the PR's objective.
Let's check its implementation and usage:
Also applies to: 78-78, 533-533
✅ Verification successful
RestoreButton is a reusable component for restoring deleted items
The RestoreButton component is a generic UI button used for restoring deleted items (both expenses and payments) in the application. Its implementation shows it's a simple SwiftUI button that displays "Restore" text and triggers a provided callback. In ExpenseDetailsView, it's used to implement the restore functionality for deleted expenses, which is relevant to the expense management functionality.
- Used in ExpenseDetailsView with
handleRestoreButtonActionthat prompts a confirmation alert before restoring an expense - Also reused in transaction-related views for restoring payments
- Implementation is clean and follows SwiftUI best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Investigate RestoreButton's purpose and usage
# Expected: UI component potentially used in expense attachment flow
# Search for the implementation
ast-grep --pattern 'struct RestoreButton: View {
$$$
}'
# Check for usage in expense-related files
rg -l 'RestoreButton' | rg -i 'expense'
Length of output: 228
Script:
#!/bin/bash
# Let's check the implementation and usage context of RestoreButton in expense-related files
# Check RestoreButton implementation
rg -A 10 "RestoreButton" ".*\.swift$"
# Specifically check its usage in expense detail files
rg -A 5 "RestoreButton" "ExpenseDetailsView.swift" "ExpenseDetailsViewModel.swift"
Length of output: 302
Script:
#!/bin/bash
# Let's try a different approach to find and analyze RestoreButton
# Find all Swift files containing RestoreButton
fd -e swift -x rg -A 10 "RestoreButton" {}
# Find the specific expense detail files
fd -e swift -x echo {} \; | grep -i "expense.*detail"
Length of output: 2623
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (4)
11-24: Add documentation for public API.As this is a public struct, please add documentation comments explaining the purpose, parameters, and usage of the view.
Add documentation like this:
+/// A SwiftUI view that displays an image with zoom and pan capabilities. +/// +/// Example usage: +/// ```swift +/// GeometryReader { geometry in +/// ZoomableImageView(image: myImage, geometry: geometry) +/// } +/// ``` public struct ZoomableImageView: View {
23-23: Consider makingloadedImageoptional.Initialize
loadedImageas an optional to better represent its state before an image is loaded.-@State var loadedImage: UIImage = UIImage() +@State var loadedImage: UIImage?
60-99: Reduce code duplication in view modifiers.The gesture, animation, and positioning modifiers are duplicated between local and remote image views. Consider extracting them into a helper function.
+private func applyImageModifiers<T: View>(_ view: T) -> some View { + view + .position(x: geometry.size.width / 2, y: geometry.size.height / 2) + .scaleEffect(scale, anchor: scaleAnchor) + .offset(offset) + .animation(.spring(), value: offset) + .animation(.spring(), value: scale) + .gesture(dragGesture) + .gesture(magnificationGesture) + .simultaneousGesture(TapGesture(count: 2).onEnded({ _ in + resetZoom() + })) +} public var body: some View { ZStack { if let image = image { - Image(uiImage: image) + applyImageModifiers(Image(uiImage: image) .resizable() - .scaledToFit() - .position(x: geometry.size.width / 2, y: geometry.size.height / 2) - .scaleEffect(scale, anchor: scaleAnchor) - .offset(offset) - .animation(.spring(), value: offset) - .animation(.spring(), value: scale) - .gesture(dragGesture) - .gesture(magnificationGesture) - .simultaneousGesture(TapGesture(count: 2).onEnded({ _ in - resetZoom() - }))) + .scaledToFit())
127-127: Remove trailing whitespace.Remove the trailing whitespace at the end of this line.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
189-189: Simplify maximumDate calculationThe current calculation of maximumDate can be simplified. Adding 0 years is unnecessary.
- private let maximumDate = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date() + private let maximumDate = Date()
211-215: Replace onTapGestureForced with standard gestureUsing forced tap gesture might interfere with accessibility features. Consider using the standard onTapGesture instead.
- .onTapGestureForced { + .onTapGesture { tempDate = date showDatePicker = true UIApplication.shared.endEditing() }Splito/UI/Home/Expense/AddExpenseView.swift (1)
204-233: Consider adding accessibility labels for better VoiceOver supportThe bottom card implementation looks good, but could benefit from improved accessibility.
Add accessibility identifiers and labels:
HStack(spacing: 16) { Spacer() - DatePickerView(date: $date, isForAddExpense: true) + DatePickerView(date: $date, isForAddExpense: true) + .accessibilityLabel("Expense date picker") - ExpenseImagePickerView(image: expenseImage, imageUrl: expenseImageUrl, handleImageBtnTap: handleExpenseImageTap) + ExpenseImagePickerView(image: expenseImage, imageUrl: expenseImageUrl, handleImageBtnTap: handleExpenseImageTap) + .accessibilityLabel("Expense image picker") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(6 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Repository/GroupRepository.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (7)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (1)
111-143: 🛠️ Refactor suggestion
Add null safety for loadedImage access.
If loadedImage is made optional as suggested earlier, ensure proper null safety in fixOffsetAndScale.
private func fixOffsetAndScale(geometry: GeometryProxy) {
+ guard let image = loadedImage else { return }
let newScale: CGFloat = .minimum(.maximum(scale, 1), 4)
let screenSize = geometry.size
let originalScale = loadedImage.size.width / loadedImage.size.height >= screenSize.width / screenSize.height ?
geometry.size.width / loadedImage.size.width :
geometry.size.height / loadedImage.size.heightLikely invalid or redundant comment.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (3)
158-158: Previous currency formatting improvements are still applicable
The previous review comment about enhancing currency formatting robustness is still valid. The current implementation needs improvements in decimal validation, maximum amount validation, and currency symbol handling.
248-268: LGTM! Clean implementation of date display
The DateDisplayView implementation is well-structured with good accessibility considerations and clear visual hierarchy.
216-245: Verify sheet presentation lifecycle
The sheet presentation might cause memory leaks if not properly managed. Let's verify the implementation across the codebase.
✅ Verification successful
Sheet presentation implementation is safe and follows SwiftUI lifecycle patterns
The verification of sheet presentations across the codebase shows:
- All sheet presentations use
isPresentedbinding, which properly manages the presentation lifecycle - The date picker sheet in
GroupPaymentViewfollows the same pattern as other sheets in the codebase - Memory management is handled automatically by SwiftUI's view lifecycle:
- Sheets are dismissed through binding state changes
- Parent views properly clean up resources in
onDisappearanddeinitwhere needed - No manual retain cycle management is needed for simple sheet presentations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential memory leaks in sheet presentations
# Look for patterns where sheet modifiers are used without proper cleanup
# Search for sheet modifier usage
rg "\.sheet\(" -A 5
# Search for cleanup patterns
rg "onDisappear|deinit" -A 3
Length of output: 12617
Splito/UI/Home/Expense/AddExpenseView.swift (3)
11-11: LGTM! Good choice using Kingfisher for image handling
Kingfisher is an excellent choice for efficient image loading and caching.
303-346: 🛠️ Refactor suggestion
Consider implementing image cleanup on dismiss
The image display view handles layout and transitions well, but should manage memory explicitly.
Add cleanup logic when the view is dismissed:
struct ImageDisplayView: View {
@Environment(\.dismiss) var dismiss
var image: UIImage?
var imageUrl: String?
+ @State private var loadedImage: UIImage?
@Namespace var animationNamespace
var body: some View {
GeometryReader { geometry in
// ... existing implementation ...
}
.navigationBarBackButtonHidden()
+ .onDisappear {
+ // Clear loaded image when view disappears
+ loadedImage = nil
+ }
}
}Likely invalid or redundant comment.
78-82: 🛠️ Refactor suggestion
Verify image size limits before upload
The image picker implementation should verify image sizes before upload to prevent performance issues.
Consider adding size validation:
ImagePickerView(cropOption: .square,
sourceType: !viewModel.sourceTypeIsCamera ? .photoLibrary : .camera,
- image: $viewModel.expenseImage,
+ image: Binding(
+ get: { viewModel.expenseImage },
+ set: { newImage in
+ if let imageData = newImage?.jpegData(compressionQuality: 0.8),
+ imageData.count <= 5 * 1024 * 1024 { // 5MB limit
+ viewModel.expenseImage = newImage
+ } else {
+ // Show error for oversized image
+ viewModel.showAlert = true
+ }
+ }
+ ),
isPresented: $viewModel.showImagePicker)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (2)
60-99: Consider extracting common view modifiers to reduce duplication.The view modifiers for both local and remote images are identical. Consider extracting them into a helper function or view extension.
Here's a suggested refactoring:
public var body: some View { ZStack { - if let image = image { - Image(uiImage: image) - .resizable() - .scaledToFit() - .position(x: geometry.size.width / 2, y: geometry.size.height / 2) - .scaleEffect(scale, anchor: scaleAnchor) - .offset(offset) - .animation(.spring(), value: offset) - .animation(.spring(), value: scale) - .gesture(dragGesture) - .gesture(magnificationGesture) - .simultaneousGesture(TapGesture(count: 2).onEnded({ _ in - resetZoom() - })) - } else if let imageUrl = imageUrl, let url = URL(string: imageUrl) { - KFImage(url) - .placeholder { _ in - ImageLoaderView() - } - .setProcessor(DownsamplingImageProcessor(size: UIScreen.main.bounds.size)) - .cacheMemoryOnly() - .onSuccess { result in - loadedImage = result.image - } - .resizable() - .scaledToFit() - .position(x: geometry.size.width / 2, y: geometry.size.height / 2) - .scaleEffect(scale, anchor: scaleAnchor) - .offset(offset) - .animation(.spring(), value: offset) - .animation(.spring(), value: scale) - .gesture(dragGesture) - .gesture(magnificationGesture) - .simultaneousGesture(TapGesture(count: 2).onEnded({ _ in - resetZoom() - })) + Group { + if let image = image { + Image(uiImage: image) + .resizable() + .scaledToFit() + } else if let imageUrl = imageUrl, let url = URL(string: imageUrl) { + KFImage(url) + .placeholder { _ in + ImageLoaderView() + } + .setProcessor(DownsamplingImageProcessor(size: UIScreen.main.bounds.size)) + .cacheMemoryOnly() + .onSuccess { result in + loadedImage = result.image + } + .resizable() + .scaledToFit() + } + } + .position(x: geometry.size.width / 2, y: geometry.size.height / 2) + .scaleEffect(scale, anchor: scaleAnchor) + .offset(offset) + .animation(.spring(), value: offset) + .animation(.spring(), value: scale) + .gesture(dragGesture) + .gesture(magnificationGesture) + .simultaneousGesture(TapGesture(count: 2).onEnded({ _ in + resetZoom() + })) } }
127-127: Remove trailing whitespace.There is trailing whitespace on this line.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
191-192: Optimize date picker state managementThe current implementation:
- Uses separate tempDate state which might cause unnecessary redraws
- The DatePicker is recreated on each tempDate change due to the id modifier
Consider this optimization:
- @State private var tempDate: Date - @State private var showDatePicker = false + @State private var showDatePicker = false { + didSet { + if showDatePicker { + tempDate = date + } + } + } + private var tempDate: Date = Date() // In the DatePicker view - DatePicker("", selection: $tempDate, in: ...maximumDate, displayedComponents: .date) - .id(tempDate) + DatePicker("", selection: $tempDate, in: ...maximumDate, displayedComponents: .date)Also applies to: 227-232
254-262: Add accessibility supportThe date display needs accessibility improvements:
- "Today" text should be localized
- Calendar icon needs accessibility label
- Text(date.isToday() ? "Today" : date.shortDate) + Text(date.isToday() ? "Today".localized : date.shortDate) Image(.calendarIcon) .resizable() .scaledToFit() .frame(width: 24, height: 24) + .accessibility(label: Text("Calendar".localized))Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsTabView.swift (1)
Line range hint
233-251: Consider refactoring for improved maintainability.The initialization and value calculation logic could be simplified for better maintainability.
Consider these improvements:
- Group related parameters using a configuration struct:
struct MemberCellConfiguration { let suffixText: String let formatString: String let inputFieldWidth: Double // ... other configuration parameters }
- Extract the calculated value logic into a computed property:
private var calculatedAmount: Double { guard let total = totalValue, total != 0 else { return 0 } return (expenseAmount * Double(value)) / total }This would make the code more maintainable and easier to test.
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift (2)
60-60: Consider caching the sorted months.The
sorted(by: sortMonthYearStrings)operation is being performed twice - once forlastMonthand previously forfirstMonth. This could be optimized by storing the sorted result.- let firstMonth = viewModel.filteredTransactions.keys.sorted(by: sortMonthYearStrings).first - let lastMonth = viewModel.filteredTransactions.keys.sorted(by: sortMonthYearStrings).last + let sortedMonths = viewModel.filteredTransactions.keys.sorted(by: sortMonthYearStrings) + let firstMonth = sortedMonths.first + let lastMonth = sortedMonths.last
67-67: Extract the magic number into a constant.The padding value of 50 should be defined as a named constant to improve code maintainability and make the purpose of this specific padding value clear.
+ private let lastItemBottomPadding: CGFloat = 50 + var body: some View { GeometryReader { geometry in ScrollViewReader { scrollProxy inThen update the padding line:
- .padding(.bottom, (month == lastMonth && viewModel.filteredTransactions[month]?.last?.transaction.id == transaction.transaction.id) ? 50 : 0) + .padding(.bottom, (month == lastMonth && viewModel.filteredTransactions[month]?.last?.transaction.id == transaction.transaction.id) ? lastItemBottomPadding : 0)Splito/UI/Home/Expense/AddExpenseView.swift (4)
78-82: Consider enhancing image picker error handlingWhile the image picker implementation is functional, consider adding error handling for cases where the user denies camera/photo library access.
.sheet(isPresented: $viewModel.showImagePicker) { ImagePickerView(cropOption: .square, - sourceType: !viewModel.sourceTypeIsCamera ? .photoLibrary : .camera, + sourceType: viewModel.sourceTypeIsCamera ? .camera : .photoLibrary, image: $viewModel.expenseImage, isPresented: $viewModel.showImagePicker) + .onDisappear { + if viewModel.expenseImage == nil { + // Handle image selection failure or cancellation + viewModel.handleImageSelectionError() + } + } }
204-233: Add accessibility support for image selectionThe BottomCardView implementation looks good, but consider adding accessibility labels and hints for the image selection functionality.
ExpenseImagePickerView(image: expenseImage, imageUrl: expenseImageUrl, handleImageBtnTap: handleExpenseImageTap) + .accessibilityLabel("Expense image") + .accessibilityHint("Double tap to add or change expense image")
235-268: Enhance user experience with haptic feedbackConsider adding haptic feedback when the user taps the camera icon for a better tactile experience.
.onTouchGesture { + UIImpactFeedbackGenerator(style: .light).impactOccurred() handleImageBtnTap() }
303-344: Add loading state for image displayConsider showing a loading indicator while the image is being prepared for display, especially for remote images.
if let imageUrl { ZoomableImageView(imageUrl: imageUrl, geometry: geometry) + .overlay { + if viewModel.isLoadingImage { + ProgressView() + .progressViewStyle(.circular) + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift(1 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(6 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsTabView.swift(4 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Repository/GroupRepository.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift
[Warning] 127-127: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (9)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (1)
26-49: Previous review comment is still applicable.
The gesture handlers should store their final values before calling fixOffsetAndScale.
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (3)
158-158: Enhance currency formatting implementation
The previous review comment about improving currency formatting robustness is still valid. The current implementation needs:
- Decimal point validation
- Maximum amount validation
- Better currency symbol handling
Please refer to the previous review comment for the detailed implementation suggestion.
173-173: Add missing isForAddExpense parameter
The DatePickerView is instantiated without the isForAddExpense parameter. While it has a default value, it should be explicitly set to false for payment flows to maintain consistency.
189-189: Review date range constraints
The maximum date constraint only considers the current year. Consider:
- Adding a minimum date constraint
- Making the date range configurable based on business requirements
Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsTabView.swift (2)
220-221: LGTM! Consistent implementation of user identification.
The SplitoPreference integration matches the implementation in ExpenseMemberCellView, maintaining consistency across the codebase.
Also applies to: 256-256
Line range hint 103-116: LGTM! Verify preference injection setup.
The personalization of member display using SplitoPreference is well implemented. The optional chaining in the comparison member.id == preference.user?.id safely handles the case when no user is set.
Let's verify the dependency injection setup:
✅ Verification successful
SplitoPreference injection is properly configured
The verification shows that SplitoPreference is correctly registered in the DI container (AppAssembly.swift) with container scope and is consistently injected across the app, including in ExpenseSplitOptionsTabView.swift. The implementation follows the established dependency injection pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if SplitoPreference is properly registered in the DI container
rg -A 5 "SplitoPreference" --type swift
Length of output: 24086
Splito/UI/Home/Expense/AddExpenseView.swift (3)
117-118: LGTM! Clean and consistent implementation
The ExpenseDetailRow usage follows a consistent pattern with proper parameter ordering and clear naming.
Also applies to: 121-122, 131-135
270-301: Previous review comment about error handling is still applicable
286-294: Verify memory usage patterns for large images
While Kingfisher's downsampling processor is used, it's important to verify memory usage patterns with large images.
✅ Verification successful
Let me gather more information about the image handling implementation to ensure a comprehensive verification.
Memory management for images follows consistent patterns
The codebase demonstrates a consistent and well-implemented approach to image memory management:
- All image loading uses KFImage with DownsamplingImageProcessor to reduce memory footprint
- Memory-only caching is consistently applied across image views
- Custom UIImage extension includes size management utilities
- ZoomableImageView and other image views follow the same memory optimization patterns
The implementation in AddExpenseView.swift aligns with the project's image handling best practices and doesn't require additional memory optimizations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other image processing implementations to ensure consistent memory management
rg -g '*.swift' -B 2 -A 2 'DownsamplingImageProcessor|UIImage.*scale|imageWith|CGImage'
Length of output: 1067
Script:
#!/bin/bash
# Search for KFImage usage patterns and caching configurations
rg -g '*.swift' -B 3 -A 3 'KFImage.*|ImageCache.*|\.cache'
# Look for any custom image processing or memory management utilities
rg -g '*.swift' -B 2 -A 2 'class.*Image|struct.*Image|extension.*Image'
Length of output: 11344
cp-amisha-i
force-pushed
the
add-expense-attachment-support
branch
from
7c38436 to
ab9f9b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (28)
Data/Data/Extension/Double+Extension.swift (1)
11-32: Consider implementing a more flexible currency formatting strategy.The current implementation is tightly coupled to the device's locale. As the application grows or expands to different regions, you might need to:
- Support displaying amounts in multiple currencies
- Maintain consistent formatting regardless of device locale
- Handle different currency conversion and display requirements
Consider creating a dedicated
CurrencyFormatterservice that:
- Allows specifying the desired currency and locale
- Handles currency conversion if needed
- Provides consistent formatting across the app
- Can be easily mocked for testing
Example structure:
protocol CurrencyFormatterProtocol { func format(_ amount: Double, currency: Currency, removeSign: Bool) -> String } class CurrencyFormatter: CurrencyFormatterProtocol { private let formatter: NumberFormatter init(defaultCurrency: Currency = .current) { formatter = NumberFormatter() formatter.numberStyle = .currency // Additional configuration... } func format(_ amount: Double, currency: Currency = .current, removeSign: Bool = false) -> String { // Implementation... } }Data/Data/Store/GroupStore.swift (3)
21-23: Add documentation for the new methodConsider adding documentation explaining the purpose and usage of this method, especially since it's part of a two-step group creation process.
+/// Returns a new document reference for creating a group in Firestore. +/// - Returns: A DocumentReference that can be used with createGroup(document:group:) func getNewGroupDocument() async throws -> DocumentReference {
25-27: Add validation and documentation for group creationConsider adding validation to ensure the document reference belongs to the correct collection and adding documentation for the method.
+/// Creates a new group in Firestore at the specified document reference. +/// - Parameters: +/// - document: The document reference where the group should be created +/// - group: The group data to be stored +/// - Throws: FirestoreError if the operation fails func createGroup(document: DocumentReference, group: Groups) async throws { + guard document.parent.collectionPath == GroupStore.COLLECTION_NAME else { + throw ServiceError.invalidOperation + } try document.setData(from: group) }
31-33: Remove outdated comment and consider adding member ID validationThe comment about Task wrapping is no longer relevant as the method is already async. Also, consider adding validation for the member ID format.
- // Wrap the updateData in a Task for async handling + guard !memberId.isEmpty else { + throw ServiceError.invalidInput + } try await groupReference.document(groupId).updateData([ "members": FieldValue.arrayUnion([memberId]) ])Data/Data/Store/ExpenseStore.swift (2)
28-29: Consider using a transaction for data consistencyWhen setting expense data that includes an image URL, consider using a transaction to ensure atomicity. If the image upload succeeds but data setting fails, you might want to roll back both operations.
func addExpense(document: DocumentReference, expense: Expense) async throws { - try document.setData(from: expense) + try await database.runTransaction { transaction in + try transaction.setData(from: expense, forDocument: document) + } }
24-29: Document the expense creation workflowWith the introduction of image attachments, the expense creation process now involves multiple steps (document creation, image upload, and data setting). Consider adding documentation to clarify:
- The expected order of operations
- Error handling expectations
- Rollback behavior for partial failures
Add documentation like this:
+/// Creates a new document reference for an expense. +/// This should be called before uploading any attachments to ensure proper cleanup in case of failures. func getNewExpenseDocument(groupId: String) async throws -> DocumentReference { +/// Sets the expense data to the specified document. +/// - Important: Ensure any image uploads are completed and the imageUrl is set in the expense object +/// before calling this method. func addExpense(document: DocumentReference, expense: Expense) async throws {Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (3)
15-15: LGTM! Consider adding accessibility support.The image view implementation is well-structured with proper conditional rendering and consistent styling.
Consider adding accessibility label to the image view:
ExpenseImageView(showImageDisplayView: $showImageDisplayView, imageUrl: imageUrl) .aspectRatio(16/9, contentMode: .fit) .cornerRadius(12) + .accessibilityLabel("Expense receipt image")Also applies to: 30-34
74-78: Consider moving Namespace to a stored property.Creating a new Namespace() instance in the view body could impact performance as it may be recreated on each render.
Consider moving it to a stored property:
struct ExpenseDetailsView: View { @StateObject var viewModel: ExpenseDetailsViewModel @State private var showImageDisplayView = false + @Namespace private var imageNamespace var body: some View { // ... .navigationDestination(isPresented: $showImageDisplayView) { if let imageUrl = viewModel.expense?.imageUrl { - ExpenseImageZoomView(imageUrl: imageUrl, animationNamespace: Namespace()) + ExpenseImageZoomView(imageUrl: imageUrl, animationNamespace: imageNamespace) } }
Add accessibility labels to image-related views for better VoiceOver support
The image-related views lack proper accessibility support. Here are the specific locations that need accessibility modifiers:
ExpenseImageViewandExpenseImageZoomViewneed accessibility labels describing the expense image and zoom functionalityGroupProfileImageViewshould include the group name in its accessibility labelMemberProfileImageViewshould include the member's name in its accessibility label- Image-based buttons (like camera icon) need accessibility labels describing their actions
Example implementation for
ExpenseImageView:.accessibilityLabel("Expense receipt image") .accessibilityAddTraits(.isButton) .accessibilityHint("Double tap to view full size")🔗 Analysis chain
Line range hint
1-190: Verify accessibility support for image-related features.Let's ensure all image-related views have proper accessibility support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for accessibility modifiers in image-related views rg -i "accessibility|voiceover" --type swiftLength of output: 46
Script:
#!/bin/bash # Let's search for image-related view components to check their implementation ast-grep --pattern 'struct $name: View { $$$ Image($$$) $$$ }' # Also search for specific image view components rg "ExpenseImageView|ExpenseImageZoomView|GroupProfileImageView|MemberProfileImageView" -A 10Length of output: 30183
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (2)
13-44: LGTM! Consider adding error handling for image loading.The implementation looks good with proper handling of both local and remote images. The use of Kingfisher's downsizing processor is a good optimization.
Consider adding error handling for failed image loads:
KFImage(url) .placeholder { _ in ImageLoaderView() } + .onFailure { error in + // Handle image loading failure + print("Error loading image: \(error.localizedDescription)") + }
137-173: Reduce code duplication in view body.The image view configuration is duplicated between local and remote image cases. Consider extracting the common configuration into a separate view modifier.
+private extension View { + func applyZoomableConfig( + geometry: GeometryProxy, + scale: CGFloat, + scaleAnchor: UnitPoint, + offset: CGSize, + dragGesture: some Gesture, + magnificationGesture: some Gesture, + onDoubleTap: @escaping () -> Void + ) -> some View { + self + .scaledToFit() + .position(x: geometry.size.width / 2, y: geometry.size.height / 2) + .scaleEffect(scale, anchor: scaleAnchor) + .offset(offset) + .animation(.spring(), value: offset) + .animation(.spring(), value: scale) + .gesture(dragGesture) + .gesture(magnificationGesture) + .simultaneousGesture( + TapGesture(count: 2).onEnded { _ in + onDoubleTap() + } + ) + } +}Then use it in both cases:
if let image { Image(uiImage: image) .resizable() .applyZoomableConfig( geometry: geometry, scale: scale, scaleAnchor: scaleAnchor, offset: offset, dragGesture: dragGesture, magnificationGesture: magnificationGesture, onDoubleTap: resetZoom ) } else if let imageUrl, let url = URL(string: imageUrl) { KFImage(url) // ... Kingfisher configuration ... .resizable() .applyZoomableConfig( geometry: geometry, scale: scale, scaleAnchor: scaleAnchor, offset: offset, dragGesture: dragGesture, magnificationGesture: magnificationGesture, onDoubleTap: resetZoom ) }Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (3)
Line range hint
13-14: Consider making EXPENSES_LIMIT configurable.The hardcoded EXPENSES_LIMIT could be moved to a configuration file or made injectable for better flexibility and testing.
236-240: Consider debouncing the search filter.The
updateGroupExpensesmethod is called on every search text change and performs filtering on the entire expense list. Consider implementing debouncing to improve performance for larger datasets.Example implementation:
private var searchDebounceTimer: Timer? @Published var searchedExpense: String = "" { didSet { searchDebounceTimer?.invalidate() searchDebounceTimer = Timer.scheduledTimer(withTimeInterval: 0.3, repeats: false) { [weak self] _ in self?.updateGroupExpenses() } } }
Line range hint
156-161: Standardize error handling approach.The error handling in
fetchMoreExpensesusesshowToastForError()while other methods usehandleServiceError(). Consider standardizing the error handling approach across all methods for consistency.Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (3)
189-189: Improve date range validationThe maximumDate calculation could be more precise. Consider:
- Using Date.now instead of Date() for better performance
- Adding minimum date validation to prevent selecting dates too far in the past
- private let maximumDate = Calendar.current.date(byAdding: .year, value: 0, to: Date()) ?? Date() + private let maximumDate = Date.now + private let minimumDate = Calendar.current.date(byAdding: .year, value: -1, to: Date.now) ?? Date.now
191-192: Optimize state managementThe tempDate state variable might cause unnecessary redraws. Consider:
- Moving tempDate initialization to sheet presentation
- Using @State private var showDatePicker = false with initial value
- @State private var tempDate: Date - @State private var showDatePicker = false + @State private var showDatePicker: Bool = false + @State private var tempDate: Date?
248-268: Enhance DateDisplayView reusabilityConsider these improvements:
- Extract padding and size values to constants
- Make the view more reusable by accepting customization parameters
private struct DateDisplayView: View { + private enum Constants { + static let horizontalPadding: CGFloat = 8 + static let verticalPadding: CGFloat = 4 + static let iconSize: CGFloat = 24 + static let spacing: CGFloat = 8 + } @Binding var date: Date + var iconColor: Color = primaryText + var textColor: Color = primaryText var body: some View { - HStack(spacing: 8) { + HStack(spacing: Constants.spacing) { Text(date.isToday() ? "Today" : date.shortDate) .font(.subTitle2()) - .foregroundStyle(primaryText) + .foregroundStyle(textColor) Image(.calendarIcon) .resizable() .scaledToFit() - .frame(width: 24, height: 24) + .frame(width: Constants.iconSize, height: Constants.iconSize) + .foregroundColor(iconColor) } - .padding(.horizontal, 8) - .padding(.vertical, 4) + .padding(.horizontal, Constants.horizontalPadding) + .padding(.vertical, Constants.verticalPadding) .background(container2Color) .cornerRadius(8) } }Splito/UI/Home/Expense/AddExpenseView.swift (3)
77-81: Add error handling for image picker failures.While the image picker implementation is good, consider adding error handling for potential failures during image selection.
.sheet(isPresented: $viewModel.showImagePicker) { ImagePickerView(cropOption: .square, sourceType: !viewModel.sourceTypeIsCamera ? .photoLibrary : .camera, - image: $viewModel.expenseImage, isPresented: $viewModel.showImagePicker) + image: $viewModel.expenseImage, isPresented: $viewModel.showImagePicker) { error in + if let error { + viewModel.toast = .init(style: .error, message: error.localizedDescription) + } + } }
116-134: Enhance accessibility for expense detail rows.Consider adding accessibility labels and hints to improve the experience for users with assistive technologies.
ExpenseDetailRow(name: $viewModel.expenseName, focusedField: focusedField, subtitle: "With you and:", - inputValue: viewModel.selectedGroup?.name ?? "Select group", + inputValue: viewModel.selectedGroup?.name ?? "Select group", + accessibilityLabel: "Group selection", + accessibilityHint: "Double tap to select a group", showButton: true, onTap: viewModel.handleGroupBtnAction)
203-232: Add localization support for the confirmation dialog.The confirmation dialog strings should be localized for international users.
- .confirmationDialog("", isPresented: $showImagePickerOptions, titleVisibility: .hidden) { + .confirmationDialog(String(localized: "image_picker_options_title"), + isPresented: $showImagePickerOptions, + titleVisibility: .hidden) {Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (2)
216-220: Consider caching the grouped transactions for better performance.The current implementation recomputes the dictionary and formats dates for each transaction every time the property is accessed. For large transaction lists, this could impact performance.
Consider caching the grouped transactions and only recomputing when the underlying data changes:
- var groupedTransactions: [String: [TransactionWithUser]] { - Dictionary(grouping: transactionsWithUser.uniqued()) { transaction in - transaction.transaction.date.dateValue().monthWithYear - } - } + private var cachedGroupedTransactions: [String: [TransactionWithUser]] = [:] + + private func updateGroupedTransactions() { + cachedGroupedTransactions = Dictionary(grouping: transactionsWithUser.uniqued()) { transaction in + transaction.transaction.date.dateValue().monthWithYear + } + }Then update the cache in relevant places like
combinedTransactionsWithUserand transaction update handlers.
211-220: Consider separating date formatting changes into a separate PR.While the date formatting refactoring is beneficial, it appears unrelated to the PR's main objective of adding expense attachment support. Separating these changes into a dedicated PR would:
- Make the changes easier to review
- Reduce the risk of introducing unrelated issues
- Follow the single responsibility principle at the PR level
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
367-368: Make image processing parameters configurableThe image resizing height (200) and JPEG compression quality (0.2) are hardcoded. These values should be configurable constants to allow easy adjustment based on requirements.
Consider extracting these values to constants:
+private enum ImageConfig { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 +} - let resizedImage = expenseImage?.aspectFittedToHeight(200) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let resizedImage = expenseImage?.aspectFittedToHeight(ImageConfig.maxHeight) + let imageData = resizedImage?.jpegData(compressionQuality: ImageConfig.compressionQuality)Also applies to: 415-416
Splito/UI/Home/Expense/ZoomableImageView.swift (5)
146-147: Consolidate animation modifiers to avoid conflictsApplying multiple
.animation()modifiers to the same view hierarchy can cause unexpected behavior. It's better to combine them into a single animation or apply the animation to a parent view.Refactor the code to use a single animation:
-.animation(.spring(), value: offset) -.animation(.spring(), value: scale) +.animation(.spring(), value: [offset, scale])Alternatively, wrap the modifiers within a Group and apply the animation once.
178-184: Improve zoom reset logic for consistent behaviorThe
resetZoomfunction toggles the scale between1and3based onlastScale. This may lead to an inconsistent user experience.Consider using a boolean state variable to track the zoomed state:
@State private var isZoomed: Bool = false private func resetZoom() { isZoomed.toggle() scale = isZoomed ? 3 : 1 offset = .zero scaleAnchor = .center lastScale = scale lastOffset = .zero }
153-174: Handle image loading state to avoid blank viewsWhen loading images from a URL, there may be a delay before the image appears. Currently, there's no explicit handling of the loading state in the zoomable view.
Consider displaying a placeholder or loading indicator until the image is fully loaded to improve user experience.
36-38: Avoid hardcoding screen size for downsamplingUsing
UIScreen.main.bounds.sizefor downsampling may not account for different device orientations or sizes, especially in multitasking environments like iPad.Consider using the size of the view or
geometry.sizeinstead:.setProcessor(DownsamplingImageProcessor(size: geometry.size))
157-158: Review caching strategy for image loadingUsing
.cacheMemoryOnly()restricts caching to memory, which could lead to increased memory usage and images being reloaded when the app restarts.Evaluate whether disk caching might be beneficial for your use case by removing
.cacheMemoryOnly()or adjusting the caching configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/date.icon (1).svgis excluded by!**/*.svgSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/date.icon.svgis excluded by!**/*.svgSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/camera.icon (1).pngis excluded by!**/*.pngSplito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/camera.icon (2).pngis excluded by!**/*.png
📒 Files selected for processing (34)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj(4 hunks)BaseStyle/BaseStyle/Views/ZoomableImageView.swift(1 hunks)Data/Data/Extension/Date+Extension.swift(1 hunks)Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Helper/Firebase/StorageManager.swift(1 hunks)Data/Data/Model/ActivityLog.swift(1 hunks)Data/Data/Model/Expense.swift(2 hunks)Data/Data/Repository/ActivityLogRepository.swift(1 hunks)Data/Data/Repository/ExpenseRepository.swift(2 hunks)Data/Data/Repository/GroupRepository.swift(3 hunks)Data/Data/Store/ActivityLogStore.swift(1 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(2 hunks)Splito.xcodeproj/project.pbxproj(4 hunks)Splito/Localization/Localizable.xcstrings(1 hunks)Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/Contents.json(1 hunks)Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/Contents.json(1 hunks)Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogRouteView.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(1 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(2 hunks)Splito/UI/Home/ActivityLog/ActivityLogsFunctions.swift(0 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(7 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(4 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsTabView.swift(4 hunks)Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift(1 hunks)Splito/UI/Home/Expense/ZoomableImageView.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift(1 hunks)Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift(1 hunks)Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Splito/UI/Home/ActivityLog/ActivityLogsFunctions.swift
✅ Files skipped from review due to trivial changes (6)
- Data/Data/Model/ActivityLog.swift
- Data/Data/Repository/ActivityLogRepository.swift
- Data/Data/Store/ActivityLogStore.swift
- Splito/UI/Home/ActivityLog/ActivityLogRouteView.swift
- Splito/UI/Home/ActivityLog/ActivityLogView.swift
- Splito/UI/Home/Groups/Group/GroupHomeViewModelExtension.swift
🚧 Files skipped from review as they are similar to previous changes (14)
- BaseStyle/BaseStyle.xcodeproj/project.pbxproj
- Data/Data/Extension/Date+Extension.swift
- Data/Data/Helper/Firebase/StorageManager.swift
- Data/Data/Model/Expense.swift
- Data/Data/Repository/GroupRepository.swift
- Splito/Localization/Localizable.xcstrings
- Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CalendarIcon.imageset/Contents.json
- Splito/Resource/Assets.xcassets/Images/Group/Add Expense/CameraIcon.imageset/Contents.json
- Splito/UI/Home/Account/User Profile/UserProfileViewModel.swift
- Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsTabView.swift
- Splito/UI/Home/Expense/Expense Split Option/ExpenseSplitOptionsView.swift
- Splito/UI/Home/Groups/Create Group/CreateGroupViewModel.swift
- Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListView.swift
🔇 Additional comments (18)
Data/Data/Store/GroupStore.swift (2)
17-19: LGTM! Good improvement in reference handling
The change to a computed property ensures fresh Firestore references, which is a recommended practice. The naming is also more precise now.
Line range hint 46-50: LGTM! Well-structured query implementation
The query implementation follows Firestore best practices with proper pagination and explicit source specification. The conditions and ordering are well-structured.
Also applies to: 60-61
Data/Data/Repository/ExpenseRepository.swift (3)
14-14: LGTM: StorageManager dependency properly injected
The StorageManager dependency is correctly injected and well-placed with other repository dependencies.
66-69: Previous review comment about nil expense.id handling still applies
71-77: LGTM: Comprehensive change detection implementation
The method thoroughly compares all relevant expense properties and properly handles date comparison using dateValue().
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
36-36: LGTM! Consistent spacing.
The vertical spacing maintains the view hierarchy's visual rhythm.
113-114: LGTM! Consistent styling.
The header styling uses semantic colors and matches the app's design system.
BaseStyle/BaseStyle/Views/ZoomableImageView.swift (1)
185-216: LGTM! The offset and scale fixing logic is well implemented.
The fixOffsetAndScale function properly handles:
- Scale limits (1x to 4x)
- Image bounds calculation based on aspect ratio
- Proper offset adjustment based on image dimensions
Splito/UI/Home/Groups/Group/GroupHomeViewModel.swift (1)
239-239: Verify the monthWithYear Date extension property.
The change to use monthWithYear property improves code readability and maintainability by moving date formatting logic to a Date extension. However, we should ensure this extension property is properly implemented.
✅ Verification successful
The monthWithYear Date extension is properly implemented and used consistently
The monthWithYear property in the Date extension is well-implemented, using a standard DateFormatter to return dates in "MMMM yyyy" format (e.g., "December 2024"). The property is used consistently across the codebase in group expenses, activity logs, and transactions for month-based grouping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the monthWithYear Date extension implementation
# Expected: Find the monthWithYear property definition in Date extension
# Search for the monthWithYear property in Date extensions
ast-grep --pattern 'extension Date {
$$$
var monthWithYear: $_ {
$$$
}
$$$
}'
# Backup: Search using ripgrep
rg -p "monthWithYear" --type swift
Length of output: 656
Script:
#!/bin/bash
# Get the implementation of monthWithYear property
cat "Data/Data/Extension/Date+Extension.swift"
Length of output: 4302
Splito/UI/Home/Groups/Group/Group Options/Settle up/Payment/GroupPaymentView.swift (2)
158-158: Previous currency formatting concerns remain unaddressed
The currency formatting implementation still needs enhancement as mentioned in the previous review. Consider implementing the suggested improvements for better robustness.
173-173: Previous DatePickerView parameter issue remains unaddressed
The DatePickerView is still instantiated without the isForAddExpense parameter. As mentioned in the previous review, it should be explicitly set to false for payment flows.
Splito/UI/Home/Expense/AddExpenseView.swift (1)
32-40: LGTM! Well-structured view composition.
The integration of AddExpenseFooterView with proper bindings and handlers is clean and follows SwiftUI best practices.
Splito/UI/Home/Groups/Group/Group Options/Transactions/GroupTransactionListViewModel.swift (1)
211-213: LGTM! Good refactoring of date formatting logic.
Moving the date formatting to a Date extension property improves code maintainability and reduces duplication.
Splito/UI/Home/Expense/AddExpenseViewModel.swift (2)
22-35: LGTM! Well-structured property declarations
The new properties for image handling are well-organized with appropriate access levels and SwiftUI bindings.
421-422: Add error handling for image updates
The image update operation could fail independently of the expense update. Consider handling image-specific errors separately.
Run this script to check for error handling patterns in the codebase:
Consider separating the image update logic and adding specific error handling:
- let updatedExpense = try await expenseRepository.updateExpenseWithImage(imageData: imageData, newImageUrl: expenseImageUrl,
- group: group, expense: (expense, oldExpense), type: .expenseUpdated)
+ let updatedExpense: Expense
+ do {
+ updatedExpense = try await expenseRepository.updateExpenseWithImage(
+ imageData: imageData,
+ newImageUrl: expenseImageUrl,
+ group: group,
+ expense: (expense, oldExpense),
+ type: .expenseUpdated
+ )
+ } catch let error as ImageUpdateError {
+ showToastFor(toast: ToastPrompt(
+ type: .warning,
+ title: "Image Update Failed",
+ message: "Expense updated but image couldn't be processed: \(error.localizedDescription)"
+ ))
+ return false
+ }Splito.xcodeproj/project.pbxproj (2)
118-118: LGTM! ZoomableImageView.swift is properly integrated.
The new file is correctly added to all required sections of the project configuration.
Also applies to: 277-277, 494-494, 1119-1119
1119-1119: Verify Firebase Storage framework integration.
While Firebase core frameworks are present, ensure Firebase Storage framework is properly linked for image upload functionality.
✅ Verification successful
Firebase Storage framework is properly integrated
Firebase Storage framework is correctly linked in the project:
- Declared as a dependency in
Podfile - Installed version 11.0.0 as shown in
Podfile.lock - Properly imported in
StorageManager.swiftwhich handles storage operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Firebase Storage framework is properly linked
# Search for Firebase Storage framework in project configuration
rg -i "FirebaseStorage"
Length of output: 309
Splito/UI/Home/Expense/ZoomableImageView.swift (1)
60-64: Verify compatibility with iOS 18 features
You're using APIs introduced in iOS 18, such as navigationTransition and checking availability with #available(iOS 18.0, *). Ensure that your project's minimum deployment target includes iOS 18, or provide appropriate fallbacks for earlier iOS versions to maintain broader compatibility.
Consider the impact on users who may not have devices that support iOS 18.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Data/Data/Extension/Date+Extension.swift (1)
58-62: Add documentation to clarify the year checkThe method name might be slightly misleading as it also verifies the year.
Consider adding documentation:
+ /// Returns true if the date is in the current month of the current year + /// - Returns: Boolean indicating if the date falls within the current month and year func isInCurrentMonth() -> Bool { let calendar = Calendar.current let currentDate = Date() return calendar.isDate(self, equalTo: currentDate, toGranularity: .month) && calendar.isDate(self, equalTo: currentDate, toGranularity: .year) }Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (2)
151-159: Consider optimizing the sorting implementation.While the current implementation works, it could be improved in terms of performance and robustness:
- The current implementation requires multiple array operations (filter, sort, concatenate)
- Error handling for invalid date strings could be more robust
Consider this alternative implementation:
func sortKeysByDayAndMonth() -> [String] { - let dayKeys = filteredLogs.keys.filter { ActivityLogViewModel.dateFormatter.date(from: $0) != nil } - let monthYearKeys = filteredLogs.keys.filter { ActivityLogViewModel.dateFormatter.date(from: $0) == nil } - - let sortedDayKeys = dayKeys.sorted(by: sortDayMonthYearStrings) - let sortedMonthYearKeys = monthYearKeys.sorted(by: sortMonthYearStrings) - - return (sortedDayKeys + sortedMonthYearKeys) // Concatenate for final sorted order + return Array(filteredLogs.keys).sorted { key1, key2 in + // Try parsing as full date first + if let date1 = ActivityLogViewModel.dateFormatter.date(from: key1), + let date2 = ActivityLogViewModel.dateFormatter.date(from: key2) { + return date1 > date2 + } + // If either is a month-year format, prioritize day-wise entries + if ActivityLogViewModel.dateFormatter.date(from: key1) != nil { + return true + } + if ActivityLogViewModel.dateFormatter.date(from: key2) != nil { + return false + } + // Both are month-year format + return key1 > key2 + } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 157-157: Lines should not have trailing whitespace
(trailing_whitespace)
154-154: Remove trailing whitespace.There are trailing whitespace characters on lines 154 and 157.
- +Also applies to: 157-157
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Data/Data/Extension/Date+Extension.swift(2 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(2 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/ActivityLog/ActivityLogView.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 157-157: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (3)
Data/Data/Extension/Date+Extension.swift (1)
19-24: Previous optimization feedback still applies
The current implementation creates a new DateFormatter instance on every access, which is inefficient.
Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift (2)
Line range hint 1-160: Verify the relevance of date formatting changes.
The changes in this file focus on date handling and activity log sorting, which appear to be unrelated to the PR's main objective of adding expense attachment support. Could you clarify if these changes are necessary for the expense attachment feature or if they should be in a separate PR?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 154-154: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 157-157: Lines should not have trailing whitespace
(trailing_whitespace)
100-104: Verify date extension methods and add documentation.
The new date formatting logic introduces a dependency on isInCurrentMonth() and monthWithYear extension methods. Please ensure:
- These extension methods are properly implemented in
Date+Extension.swift - Add documentation explaining the different date formats used for current vs. previous months
✅ Verification successful
Date extension methods are properly implemented
The required date extension methods are correctly implemented in Data/Data/Extension/Date+Extension.swift:
isInCurrentMonth()uses Calendar API to compare dates at month and year granularitymonthWithYearproperty uses DateFormatter to format dates as "MMMM yyyy"
The date formatting logic in ActivityLogViewModel is consistent with other parts of the codebase, as the same extensions are used in GroupHomeViewModel and GroupTransactionListViewModel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of required Date extension methods
# Search for isInCurrentMonth() implementation
ast-grep --pattern 'extension Date {
$$$
func isInCurrentMonth() -> Bool {
$$$
}
$$$
}'
# Search for monthWithYear property implementation
ast-grep --pattern 'extension Date {
$$$
var monthWithYear: String {
$$$
}
$$$
}'
Length of output: 197
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find these date-related implementations
# Search for Date extensions and their implementations
# Search for isInCurrentMonth
rg "isInCurrentMonth" -A 5
# Search for monthWithYear
rg "monthWithYear" -A 5
# Search for all Date extensions to ensure we don't miss any implementation
rg "extension Date"
Length of output: 4289
cp-amisha-i
force-pushed
the
add-expense-attachment-support
branch
from
a7d7735 to
2dfac69
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Data/Data/Extension/Date+Extension.swift (2)
58-63: Add documentation and optimize current date accessThe implementation is correct but could be improved with documentation and by avoiding multiple Date() instantiations.
Consider this improvement:
+ /// Returns whether the date falls within the current month and year + /// - Returns: true if the date is in the current month and year, false otherwise func isCurrentMonth() -> Bool { let calendar = Calendar.current let currentDate = Date() - return calendar.isDate(self, equalTo: currentDate, toGranularity: .month) && - calendar.isDate(self, equalTo: currentDate, toGranularity: .year) + return calendar.isDate(self, equalTo: currentDate, toGranularity: .month) }Note: The
.monthgranularity in Calendar automatically includes the year check, so we can simplify the condition.
Line range hint
1-64: Consider implementing a centralized date formatting strategyThe file contains multiple date formatting methods, each creating its own DateFormatter instance. This approach is both inefficient and inconsistent.
Consider:
- Creating a dedicated DateFormatting service or manager class
- Implementing a shared formatter cache
- Standardizing date format patterns across the application
- Adding proper documentation for all formatting methods
Example architecture:
// DateFormattingService.swift public final class DateFormattingService { static let shared = DateFormattingService() private let cache: [String: DateFormatter] = [:] func formatter(for format: DateFormat) -> DateFormatter { if let cached = cache[format.rawValue] { return cached } let formatter = DateFormatter() formatter.dateFormat = format.rawValue cache[format.rawValue] = formatter return formatter } enum DateFormat: String { case shortDate = "dd MMM" case monthYear = "MMMM yyyy" case longDate = "dd MMMM yyyy" // Add other formats } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Data/Data/Extension/Date+Extension.swift(2 hunks)Splito/UI/Home/ActivityLog/ActivityLogView.swift(2 hunks)Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/UI/Home/ActivityLog/ActivityLogView.swift
- Splito/UI/Home/ActivityLog/ActivityLogViewModel.swift
🔇 Additional comments (1)
Data/Data/Extension/Date+Extension.swift (1)
19-31: Optimize DateFormatter instantiation
The previous review comments about optimizing DateFormatter usage and adding documentation are still valid. Please refer to the existing comments for the detailed implementation using shared formatters.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
Data/Data/Store/GroupStore.swift (1)
21-23: Consider adding documentation for error scenariosThe separation of document creation is a good practice. Consider adding documentation to clarify potential error scenarios that could be thrown by Firestore.
+ /// Returns a new document reference for a group + /// - Throws: FirestoreError if the database operation fails func getNewGroupDocument() async throws -> DocumentReference { return groupReference.document() }Data/Data/Store/ExpenseStore.swift (1)
24-29: Consider architectural improvements for image supportGiven that this PR adds image attachment support:
- Consider using transactions when both expense data and image URL need to be updated
- Add documentation explaining the relationship between
ExpenseStoreand image storage- Consider adding methods to handle image URL cleanup when expenses are deleted
Example transaction usage:
func addExpense(document: DocumentReference, expense: Expense) async throws { try await database.runTransaction { transaction, _ in transaction.setData(from: expense, forDocument: document) // Additional image-related operations if needed } }Data/Data/Store/TransactionStore.swift (1)
36-36: Document merge behaviorConsider adding a comment explaining why merge: true is used and its implications for data consistency.
+ // Using merge: true to preserve existing fields not present in the transaction object + // This ensures backward compatibility and prevents accidental field deletion try transactionReference(groupId: groupId).document(transactionId).setData(from: transaction, merge: true)Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
367-368: Consider making image processing parameters configurableThe image height (200) and compression quality (0.2) are hardcoded. These values should be configurable to allow for future adjustments based on requirements or user preferences.
Consider moving these values to a configuration:
+ private enum ImageConfig { + static let maxHeight: CGFloat = 200 + static let compressionQuality: CGFloat = 0.2 + static let maxFileSizeBytes = 5 * 1024 * 1024 // 5MB + } let resizedImage = expenseImage?.aspectFittedToHeight(ImageConfig.maxHeight) - let imageData = resizedImage?.jpegData(compressionQuality: 0.2) + let imageData = resizedImage?.jpegData(compressionQuality: ImageConfig.compressionQuality) + + guard let imageData = imageData, imageData.count <= ImageConfig.maxFileSizeBytes else { + showToastFor(toast: ToastPrompt( + type: .error, + title: "Error", + message: "Image size too large. Please choose a smaller image." + )) + return false + }BaseStyle/BaseStyle/Views/ZoomableImageView.swift (2)
138-179: Refactor to eliminate code duplication inZoomableImageViewThe code for displaying the image using a local
UIImageand a remote URL (imageUrl) is nearly identical. Refactoring this section can reduce duplication, improve maintainability, and enhance readability.Consider abstracting the common code into a helper function or creating a custom view modifier. Here's an example of how you might refactor:
private func buildImageView<Content: View>(_ content: Content) -> some View { content .resizable() .scaledToFit() .position(x: geometry.size.width / 2, y: geometry.size.height / 2) .scaleEffect(scale, anchor: scaleAnchor) .offset(offset) .animation(.spring(), value: offset) .animation(.spring(), value: scale) .gesture(dragGesture) .gesture(magnificationGesture) .simultaneousGesture(TapGesture(count: 2).onEnded { resetZoom() }) } var body: some View { ZStack { if let image { buildImageView(Image(uiImage: image)) } else if let imageUrl, let url = URL(string: imageUrl) { buildImageView( KFImage(url) .placeholder { _ in ImageLoaderView() } .setProcessor(DownsamplingImageProcessor(size: UIScreen.main.bounds.size)) .cacheMemoryOnly() .onSuccess { result in DispatchQueue.main.async { loadedImage = result.image } } ) } } }
192-192: Remove trailing whitespaceLine 192 contains trailing whitespace, which can lead to unnecessary diffs and may affect code styling tools. Please remove the extra whitespace to maintain code cleanliness.
Apply this diff to fix the issue:
- guard loadedImage.size.width > 0, loadedImage.size.height > 0 else { return } + guard loadedImage.size.width > 0, loadedImage.size.height > 0 else { return }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 192-192: Lines should not have trailing whitespace
(trailing_whitespace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift(1 hunks)Data/Data/Extension/Double+Extension.swift(1 hunks)Data/Data/Store/ExpenseStore.swift(1 hunks)Data/Data/Store/GroupStore.swift(2 hunks)Data/Data/Store/TransactionStore.swift(1 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Data/Data/Extension/Double+Extension.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
BaseStyle/BaseStyle/Views/ZoomableImageView.swift
[Warning] 192-192: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (11)
Data/Data/Store/GroupStore.swift (5)
17-19: LGTM! Improved property naming
The rename from groupsCollection to groupReference better reflects its purpose as a Firestore collection reference. The computed property ensures a fresh reference on each access.
25-26: Verify merge behavior for group creation
The setData call doesn't specify a merge parameter, which means it will overwrite any existing data at this document reference. Verify if this is intentional, especially if the document reference might be reused.
Consider adding merge: true if partial updates should be supported:
- try document.setData(from: group)
+ try document.setData(from: group, merge: true)31-33: LGTM! Thread-safe implementation
The use of FieldValue.arrayUnion ensures atomic updates when adding members, preventing race conditions.
38-38: Previous review comment implemented correctly
Line range hint 46-60: Verify composite index existence
The query uses multiple conditions (is_active, members) along with ordering (created_at). Ensure that the required composite index exists in Firebase to support this query efficiently.
You should have a composite index on:
- Collection:
groups - Fields:
is_active(Ascending),members(Ascending),created_at(Descending)
Add the following to your firestore.indexes.json:
{
"indexes": [
{
"collectionGroup": "groups",
"queryScope": "COLLECTION",
"fields": [
{ "fieldPath": "is_active", "order": "ASCENDING" },
{ "fieldPath": "members", "order": "ASCENDING" },
{ "fieldPath": "created_at", "order": "DESCENDING" }
]
}
]
}Data/Data/Store/ExpenseStore.swift (2)
24-26: Previous validation concerns still apply
The previous review comment about adding parameter validation for groupId remains valid. Invalid group IDs could lead to documents being created in incorrect locations.
34-34: Verify merge behavior change implications
The change to merge: true allows partial updates. While this supports updating individual fields like the new imageUrl, it could lead to unintended field preservation. Please verify that this behavior is intended and won't cause data inconsistencies.
✅ Verification successful
Let me gather more specific information about the Expense model and its usage.
Based on the gathered information, I can now provide a final response:
Merge behavior change is consistent with codebase patterns
The change to merge: true in ExpenseStore.updateExpense aligns with the established patterns in the codebase:
- All other stores (TransactionStore, GroupStore, UserStore) use
merge: truefor their update operations - The Expense model has an optional
imageUrlfield, making partial updates appropriate - Other fields in the Expense model are designed to handle partial updates safely (all required fields have default values or are optional)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other expense update operations that might be affected by this change
rg -l 'setData.*merge:' --type swift
# Look for Expense model usage to understand all fields that could be partially updated
ast-grep --pattern 'struct Expense {
$$$
}'
Length of output: 225
Script:
#!/bin/bash
# Look for Expense struct definition with context
rg -A 20 "struct Expense" --type swift
# Check how setData with merge is used in other stores for comparison
rg "setData.*merge:" --type swift -A 2
Length of output: 32262
Data/Data/Store/TransactionStore.swift (1)
36-36: Verify merge behavior implications
The change from merge: false to merge: true is appropriate for maintaining backward compatibility and preventing accidental field deletion. However, we should verify that this doesn't cause any unintended side effects.
✅ Verification successful
Change to merge: true is safe and consistent with codebase patterns
The change from merge: false to merge: true in TransactionStore is verified to be safe and appropriate because:
- It follows the established pattern in the codebase - all other stores (GroupStore, UserStore, ExpenseStore) consistently use
merge: truefor their setData operations - The codebase shows proper transaction update handling with:
- Explicit tracking of updates via
updatedByfield - Proper balance recalculation after updates
- Notification system for propagating changes
- Explicit tracking of updates via
- No explicit field deletion operations (FieldValue.delete()) were found that could be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any explicit field deletions in the codebase that might be affected by merge: true
# Look for any FieldValue.delete() usage which might be affected
rg -A 3 "FieldValue\.delete"
# Look for comments or code mentioning field deletion
rg -i "(?:remove|delete).*field"
Length of output: 65
Script:
#!/bin/bash
# Let's check for any Transaction model/struct definition and its usage
# to understand the data structure and potential field management
# Find Transaction model/struct definition
ast-grep --pattern 'struct Transaction {
$$$
}'
# Also check for any documentation or comments about transaction updates
rg -i "transaction.*update" -A 3
Length of output: 17387
Script:
#!/bin/bash
# Let's examine the Transaction model and store implementation more closely
# Look for the Transaction model fields
ast-grep --pattern 'struct Transactions: Codable {
$$$
}'
# Check the TransactionStore implementation for any other merge-related operations
rg -A 3 "setData.*merge"
Length of output: 1459
Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
206-222: Consider adding error handling for image selection
The handleActionSelection method should handle potential errors that might occur during image selection, such as:
- Image size/format validation
- Device storage constraints
- Image processing failures
Let's check if there's any error handling for image selection in the codebase:
Consider adding validation and error handling:
func handleActionSelection(_ action: ActionsOfSheet) {
switch action {
case .camera:
self.checkCameraPermission {
+ guard UIImagePickerController.isSourceTypeAvailable(.camera) else {
+ self.showToastFor(toast: ToastPrompt(
+ type: .error,
+ title: "Error",
+ message: "Camera is not available on this device"
+ ))
+ return
+ }
self.sourceTypeIsCamera = true
self.showImagePicker = true
}
case .gallery:
+ guard UIImagePickerController.isSourceTypeAvailable(.photoLibrary) else {
+ self.showToastFor(toast: ToastPrompt(
+ type: .error,
+ title: "Error",
+ message: "Photo library is not available"
+ ))
+ return
+ }
sourceTypeIsCamera = false
showImagePicker = true
case .remove:
withAnimation { [weak self] in
self?.expenseImage = nil
self?.expenseImageUrl = nil
}
}
}BaseStyle/BaseStyle/Views/ZoomableImageView.swift (2)
40-40:
Verify the usage of onTapGestureForced
The method onTapGestureForced is not a standard SwiftUI method. Please verify that it exists and is properly imported or defined elsewhere in the codebase. If it's a custom extension, ensure it's accessible in this context. Otherwise, consider using the standard onTapGesture modifier.
Run the following script to check if onTapGestureForced is defined:
✅ Verification successful
Let me check the implementation details of onTapGestureForced to ensure it's properly defined.
onTapGestureForced is correctly defined and safe to use
The method is properly defined as a View extension in BaseStyle/BaseStyle/Extension/View+Extension.swift. It's a wrapper around SwiftUI's standard onTapGesture that adds contentShape(Rectangle()) to ensure the tap area covers the entire view bounds, which is a common pattern to improve tap detection. The usage in ZoomableImageView is correct and follows best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the definition of 'onTapGestureForced'
# Search for the definition of 'onTapGestureForced' in the codebase
rg 'extension.*onTapGestureForced' -A 5
# Search for any usage of 'onTapGestureForced' to identify its module or file
rg 'onTapGestureForced' --glob '!.build/*'
Length of output: 1909
Script:
#!/bin/bash
# Get the implementation of onTapGestureForced
rg -A 10 'func onTapGestureForced.*perform action' 'BaseStyle/BaseStyle/Extension/View+Extension.swift'
Length of output: 467
76-76:
Import BaseStyle to access disableText color
The disableText color is not defined in the current scope. To access the color definitions, you need to import the BaseStyle module.
Apply this diff to fix the issue:
+import BaseStyle
import SwiftUI
import KingfisherLikely invalid or redundant comment.
cp-amisha-i
force-pushed
the
add-expense-attachment-support
branch
from
14e5a87 to
3222964
Compare
Summary by CodeRabbit
Release Notes
New Features
shortDateandmonthWithYearin the Date extension.ExpenseImageViewandExpenseImageZoomViewfor improved image display and zooming capabilities.AddExpenseViewfor selecting and displaying images.Improvements
Bug Fixes
Localization
Documentation
Simulator.Screen.Recording.-.iPhone.16.-.2024-11-25.at.10.43.46.1.mp4