Add notes support to expense #77
Conversation
WalkthroughThis pull request introduces enhancements to the expense management functionality within the application. Key changes include the addition of a Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
🔇 Additional comments (3)Splito/UI/Home/Expense/AddExpenseView.swift (3)
The note editor sheet implementation is well-structured and consistent with other sheet presentations in the file.
The addition of note-related parameters and their organization is clean and logical. Also applies to: 215-217
Enhance accessibility and maintainability of NoteButtonView Similar to previous feedback, the implementation needs improvements:
private struct NoteButtonView: View {
let handleNoteBtnTap: (() -> Void)
+ private let iconSize: CGFloat = 24
+ private let padding: CGFloat = 4
var body: some View {
Button {
- UIApplication.shared.endEditing()
handleNoteBtnTap()
} label: {
Image(.noteIcon)
.resizable()
.scaledToFit()
- .frame(width: 24, height: 24)
- .padding(4)
+ .frame(width: iconSize, height: iconSize)
+ .padding(padding)
.background(container2Color)
.cornerRadius(8)
+ .accessibilityLabel("Add note to expense")
}
}
}Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (15)
Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteView.swift (2)
28-31: Add accessibility identifier to the TextEditor overlay.The overlay rectangle should have an accessibility identifier to ensure proper VoiceOver support.
.overlay { RoundedRectangle(cornerRadius: 12) .stroke(outlineColor, lineWidth: 1) + .accessibility(identifier: "noteTextEditorBorder") }
33-35: Simplify frame modifiers.The double frame modifier can be combined into a single one for better readability.
-.frame(maxWidth: isIpad ? 600 : nil, alignment: .center) -.frame(maxWidth: .infinity, alignment: .center) +.frame(maxWidth: isIpad ? 600 : .infinity, alignment: .center)Data/Data/Model/Expense.swift (1)
20-20: Consider adding note size validationWith the addition of notes, consider implementing size validation to prevent performance issues with very large notes. This could affect:
- Memory usage when loading multiple expenses
- Network bandwidth during sync
- Database storage requirements
Consider adding validation in the appropriate service layer to enforce reasonable size limits.
Data/Data/Repository/ExpenseRepository.swift (1)
Line range hint
1-150: Well-structured integration of the notes feature.The note support has been cleanly integrated into the existing expense management flow:
- Notes are properly tracked in change detection
- Activity logging automatically captures note changes
- The separation of concerns is maintained between notes, images, and core expense data
This approach ensures that notes are treated as first-class citizens in the expense tracking system while maintaining the existing architectural patterns.
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
31-42: Consider dynamic height constraints for image attachmentsWhile the fixed height of 140 provides consistency, it might lead to image distortion or cropping for different aspect ratios. Consider using
aspectRatio(contentMode:)modifier for better image presentation.ExpenseImageView(showImageDisplayView: $showImageDisplayView, imageUrl: imageUrl) - .frame(height: 140, alignment: .leading) + .frame(maxHeight: 140, alignment: .leading) + .aspectRatio(contentMode: .fit) .frame(maxWidth: .infinity) .cornerRadius(12)
66-72: Consider adding explicit dismiss handlingThe fullScreenCover for note editing should handle explicit dismissal for better user experience.
.fullScreenCover(isPresented: $viewModel.showAddNoteEditor) { NavigationStack { ExpenseAddNoteView(viewModel: ExpenseAddNoteViewModel( groupId: viewModel.groupId, expenseId: viewModel.expenseId, expenseNote: viewModel.expenseNote - )) + )).toolbar { + ToolbarItem(placement: .cancellationAction) { + Button("Cancel") { + viewModel.showAddNoteEditor = false + } + } + } } }Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (2)
26-26: Consider grouping related sheet presentation states.The property is well-defined, but consider grouping it with
showEditExpenseSheetunder a descriptive comment for better organization.+ // MARK: - Sheet Presentation States @Published var showAddNoteEditor = false @Published var showEditExpenseSheet = false
23-26: Consider implementing state restoration for note editing.When the note editor is dismissed, the view model should ensure that
expenseNoteis restored to its original value if editing was cancelled. This prevents stale state in the view model.Consider adding:
- A backup of the original note value before editing
- State restoration logic when the editor is dismissed without saving
Splito/UI/Home/Expense/AddExpenseView.swift (1)
281-298: Enhance accessibility and platform independenceA few suggestions to improve this component:
- Add accessibility label for the note icon
- Consider moving UIKit-specific code to an extension or environment value
Consider these improvements:
struct ExpenseNoteView: View { let handleNoteTap: (() -> Void) var body: some View { Image(.noteIcon) .resizable() .scaledToFit() .frame(width: 24, height: 24) .padding(4) + .accessibilityLabel("Add note") .onTouchGesture { - UIApplication.shared.endEditing() + hideKeyboard() handleNoteTap() } .background(container2Color) .cornerRadius(8) } + + private func hideKeyboard() { + #if os(iOS) + UIApplication.shared.endEditing() + #endif + } }Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
217-224: Consider adding input validation for notesWhile the note handling functions are well-structured, consider adding:
- Maximum length validation to prevent excessive note sizes
- Trimming of whitespace (similar to how
expenseNameis handled)- Basic sanitization of input
func handleNoteSaveBtnTap(note: String) { showAddNoteEditor = false - self.expenseNote = note + let sanitizedNote = note.trimming(spaces: .leadingAndTrailing) + guard sanitizedNote.count <= 1000 else { + showToastFor(toast: ToastPrompt( + type: .warning, + title: "Note too long", + message: "Please keep your note under 1000 characters" + )) + return + } + self.expenseNote = sanitizedNote }Splito/Localization/Localizable.xcstrings (1)
224-225: Maintain consistent label formattingThe label strings "Attachment:" and "Note:" use colons, which might not be appropriate for all languages. Consider removing the colons and adding them in the UI layer if needed.
Apply this diff to make the labels consistent:
- "Attachment:" : { + "Attachment" : { - "Note:" : { + "Note" : {Also applies to: 512-513
Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteViewModel.swift (4)
48-62: RefactorshowLoaderhandling usingdeferinfetchGroup()The
showLoadervariable is set tofalsein multiple places withinfetchGroup(). To avoid code duplication and ensureshowLoaderis turned off regardless of how the method exits, consider using adeferstatement.Apply this diff to refactor the method:
private func fetchGroup() async -> Bool { + showLoader = true + defer { showLoader = false } - do { - showLoader = true self.group = try await groupRepository.fetchGroupBy(id: groupId) await fetchExpense() - showLoader = false LogD("ExpenseAddNoteViewModel: \(#function) Group fetched successfully.") return true } catch { - showLoader = false LogE("ExpenseAddNoteViewModel: \(#function) Failed to fetch group \(groupId): \(error).") showToastForError() return false } }
64-80: Usedeferto manageshowLoaderinfetchExpense()In
fetchExpense(),showLoader = falseis set in multiple paths. Utilizing adeferstatement ensuresshowLoaderis reset when the method exits, reducing redundancy.Apply this diff to refactor the method:
private func fetchExpense() async { + defer { showLoader = false } guard let expenseId else { - showLoader = false return } do { self.expense = try await expenseRepository.fetchExpenseBy(groupId: groupId, expenseId: expenseId) await updateExpense() - showLoader = false LogD("ExpenseAddNoteViewModel: \(#function) Expense fetched successfully.") } catch { - showLoader = false LogE("ExpenseAddNoteViewModel: \(#function) Failed to fetch expense \(expenseId): \(error).") showToastForError() } }
82-107: SimplifyshowLoadermanagement inupdateExpense()withdeferThe
showLoadervariable is being set tofalsein several places inupdateExpense(). To streamline the code, use adeferstatement to ensureshowLoaderis reset when the method exits.Apply this diff to refactor the method:
private func updateExpense() async { + defer { showLoader = false } guard let group, let expense, let expenseId = expense.id else { - showLoader = false return } var updatedExpense = expense updatedExpense.note = expenseNote.trimmingCharacters(in: .whitespacesAndNewlines) guard expense.note != updatedExpense.note else { - showLoader = false return } do { try await expenseRepository.updateExpense(group: group, expense: updatedExpense, oldExpense: expense, type: .expenseUpdated) NotificationCenter.default.post(name: .updateExpense, object: updatedExpense) - showLoader = false LogD("ExpenseAddNoteViewModel: \(#function) Expense updated successfully.") } catch { - showLoader = false LogE("ExpenseAddNoteViewModel: \(#function) Failed to update expense \(expenseId): \(error).") showToastForError() } }
39-46: Simplify thehandleSaveAction()methodThe
handleSaveAction()method can be streamlined by utilizing optional binding, which enhances readability and conciseness.Apply this diff to refactor the method:
func handleSaveAction() async -> Bool { - if handleSaveNoteTap != nil { - handleSaveNoteTap?(expenseNote) - return true - } else { - return await fetchGroup() - } + if let handleSaveNoteTap = handleSaveNoteTap { + handleSaveNoteTap(expenseNote) + return true + } + return await fetchGroup() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Splito/Resource/Assets.xcassets/Images/Group/Add Expense/NoteIcon.imageset/note-1_svgrepo.com (1).svgis excluded by!**/*.svgSplito/Resource/Assets.xcassets/Images/Group/Add Expense/NoteIcon.imageset/note-1_svgrepo.com.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
Data/Data/Model/Expense.swift(2 hunks)Data/Data/Repository/ExpenseRepository.swift(1 hunks)Splito.xcodeproj/project.pbxproj(6 hunks)Splito/Localization/Localizable.xcstrings(3 hunks)Splito/Resource/Assets.xcassets/Images/Group/Add Expense/NoteIcon.imageset/Contents.json(1 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(9 hunks)Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteView.swift(1 hunks)Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteViewModel.swift(1 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(4 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Splito/Resource/Assets.xcassets/Images/Group/Add Expense/NoteIcon.imageset/Contents.json
🔇 Additional comments (17)
Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteView.swift (2)
11-17: LGTM! Well-structured state management.
The view properly manages its state using appropriate property wrappers. The temporary note storage pattern is a good practice for handling user input before committing changes.
14-14: Verify ViewModel memory management.
Ensure that the ViewModel doesn't create retain cycles with its parent view or hold strong references to delegates/callbacks.
✅ Verification successful
Based on the code inspection results, I can now provide a final response:
No memory management concerns found in the ViewModel implementation
The implementation shows good memory management practices:
- The ViewModel uses value types for most properties (@published properties)
- The closure
handleSaveNoteTapis properly declared as optional and stored without capturing cycles - Dependencies are properly injected and don't create retain cycles
- BaseViewModel implementation shows standard publisher management with cancelable Set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential retain cycles in ViewModel
ast-grep --pattern 'class ExpenseAddNoteViewModel {
$$$
var $x: $t
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# Let's try a different approach to find the ViewModel implementation
rg -l "ExpenseAddNoteViewModel" --type swift
# Then let's look at its content
rg "class ExpenseAddNoteViewModel" -A 20 --type swift
Length of output: 2721
Script:
#!/bin/bash
# Let's check the BaseViewModel implementation for any potential retain cycle patterns
rg "class BaseViewModel" -A 10 --type swift
# Also check for any closure handling in the ViewModel
rg "func.*saveNote" -A 10 --type swift ExpenseAddNoteViewModel.swift
Length of output: 813
Data/Data/Model/Expense.swift (1)
52-52: Verify the intentional difference between property name and JSON key
The property name note (singular) maps to JSON key "notes" (plural). While this works, it creates a slight inconsistency in naming. Please verify if this mapping is intentional to match existing API contracts.
✅ Verification successful
Based on the codebase analysis, I can now provide a final response regarding the note vs notes naming difference:
The singular/plural difference is intentional and follows the codebase's data model design
The property name note in the Swift model correctly maps to the "notes" JSON key as it follows the established pattern in the codebase where:
- The Swift property
noterepresents a single note string in theExpensemodel - The Firebase/JSON field uses
"notes"as the storage key - The codebase consistently uses this mapping through the expense creation and editing flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of "notes" key in JSON responses or API documentation
rg -l "\"notes\"" --type json
rg -l "\"notes\":" --type json
Length of output: 60
Script:
#!/bin/bash
# Search for "notes" field usage in Swift files to understand the API contract
rg -i "notes" --type swift -C 2
# Search for any JSON mock data or test files that might contain "notes"
fd -e json -x cat {}
Length of output: 63500
Script:
#!/bin/bash
# Search for the Expense model structure and any related API/backend code
rg -A 10 -B 10 "struct Expense" --type swift
# Search for any Firebase or backend schema definitions that might show the API contract
fd -e rules -e schema -x cat {}
Length of output: 36647
Data/Data/Repository/ExpenseRepository.swift (2)
74-77: LGTM! The note comparison is properly integrated.
The hasExpenseChanged method has been correctly updated to include the note property comparison, maintaining consistency with the expense model changes.
Line range hint 1-150: Verify activity logging for note changes.
While the note field is properly integrated into change detection, we should verify that note changes are appropriately reflected in activity logs for proper user communication.
✅ Verification successful
Note changes are properly handled in the expense tracking system
The implementation is complete and properly handles note changes:
- Note changes are captured in
hasExpenseChanged()method through the comparisonoldExpense.note != expense.note - When note changes occur, they are automatically included in expense updates through the standard expense update flow
- Activity logging for note changes is handled through the generic
expenseUpdatedactivity type, which is appropriate since:- Notes are supplementary information that don't require special activity type
- Changes are tracked alongside other expense properties
- The activity log focuses on essential financial information (amount, split, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for note-related activity logging
rg -A 5 "note.*activity|activity.*note"
# Check if there are any specific activity types for note changes
ast-grep --pattern 'enum ActivityType {
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Let's first check the ActivityType enum definition
fd -e swift ActivityType
Length of output: 26
Script:
#!/bin/bash
# Let's try finding the enum definition using ripgrep
rg "enum ActivityType" -A 10
# Also search for any activity-related types or models
rg "class ActivityLog|struct ActivityLog" -A 10
Length of output: 9188
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift (2)
232-232: LGTM!
The padding adjustment improves the visual hierarchy of the expense information section.
Line range hint 25-72: Verify state handling across view updates
Please ensure that the view properly handles all possible state combinations:
- Loading state while fetching expense details
- Note editor state while showing image viewer
- Edit expense state while showing note editor
✅ Verification successful
State handling implementation appears safe and well-structured
The verification shows that the state management is properly implemented:
- Both
showAddNoteEditorandshowImageDisplayVieware initialized asfalseby default showAddNoteEditoris managed in the ViewModel whileshowImageDisplayViewis managed as local view state- The states are toggled independently and have clear reset logic
- The fullScreenCover modifiers in the view ensure proper modal presentation hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential state handling issues in view model
# Check for state management in view model
ast-grep --pattern 'class $_ {
$$$
@Published var showAddNoteEditor: Bool
$$$
@Published var showImageDisplayView: Bool
$$$
}'
# Check for concurrent state updates
rg -A 3 'showAddNoteEditor.*=.*true|showImageDisplayView.*=.*true'
Length of output: 1347
Script:
#!/bin/bash
# Let's check how these state variables are managed and if there are any safeguards
# Check view model implementation for state handling
ast-grep --pattern 'class ExpenseDetailsViewModel {
$$$
}'
# Check for any state reset logic
rg -A 3 'showAddNoteEditor.*=.*false|showImageDisplayView.*=.*false'
# Check for any state dependencies or mutual exclusivity handling
rg -A 5 'showAddNoteEditor|showImageDisplayView' 'Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift'
Length of output: 2338
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift (3)
23-25: LGTM! Properties are well-defined with appropriate access levels.
The new properties follow the established pattern in the codebase with proper access modifiers.
96-96: LGTM! Note assignment is properly placed.
The note assignment is correctly integrated into the expense processing flow.
114-114: Skip commenting on cosmetic change.
Splito/UI/Home/Expense/AddExpenseView.swift (2)
38-40: LGTM: Footer view parameters are well-structured
The parameter additions and ordering are clean and follow good SwiftUI practices.
77-85: Consider safer handling of optional values
While the optional binding is good, the view might not handle the case where groupId or expenseId is nil. Consider providing a fallback or error state.
Let's verify the handling of nil cases:
Consider this safer approach:
.sheet(isPresented: $viewModel.showAddNoteEditor) {
NavigationStack {
- if let groupId = viewModel.groupId, let expenseId = viewModel.expenseId {
- ExpenseAddNoteView(viewModel: ExpenseAddNoteViewModel(
- groupId: groupId, expenseId: expenseId, expenseNote: viewModel.expenseNote, handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:)
- ))
- }
+ Group {
+ if let groupId = viewModel.groupId, let expenseId = viewModel.expenseId {
+ ExpenseAddNoteView(viewModel: ExpenseAddNoteViewModel(
+ groupId: groupId, expenseId: expenseId, expenseNote: viewModel.expenseNote, handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:)
+ ))
+ } else {
+ ContentUnavailableView("Unable to add note", systemImage: "note.text.badge.exclamationmark")
+ }
+ }
}
}Splito/UI/Home/Expense/AddExpenseViewModel.swift (3)
22-22: LGTM: Property declarations for note support
The new properties are properly declared with appropriate types and access levels.
Also applies to: 35-35
55-56: Great improvement: Making identifiers immutable
Converting expenseId and groupId to let constants improves code safety by preventing accidental modifications after initialization.
377-378: Consider standardizing empty note handling
The code handles notes consistently across create and update operations, but consider standardizing empty note handling:
- Should empty notes be stored as empty strings or null?
- Should whitespace-only notes be treated as empty?
Let's check how empty notes are handled in the Expense model:
Also applies to: 419-419, 466-468
✅ Verification successful
Let's also search for note-related operations:
And check for any null handling patterns:
Based on the search results, I can now provide a final response:
Empty note handling is already standardized
The codebase shows a consistent approach to note handling:
- Notes are stored as non-optional
Stringin theExpensemodel - Notes are trimmed before saving using
trimming(spaces: .leadingAndTrailing) - Empty notes are handled as empty strings rather than null values
- Note comparison for changes is done with direct string equality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Expense model definition and note handling
ast-grep --pattern 'struct Expense {
$$$
var note: $_
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Search for Expense model definition with a more flexible pattern
rg -l "struct\s+Expense" --type swift
# After finding the files, let's examine their content
rg "struct\s+Expense" -A 20 --type swift
Length of output: 37133
Script:
#!/bin/bash
# Search for note property and its usage
rg "note\s*:" --type swift -A 2 -B 2
Length of output: 3525
Script:
#!/bin/bash
# Search for nil or optional handling related to notes
rg "note\s*[?!]|\?\?.*note|note.*\?\?" --type swift
Length of output: 454
Splito.xcodeproj/project.pbxproj (2)
158-159: LGTM: New files are properly organized in a dedicated group.
The new note-related files are well-organized in a dedicated "Notes" group, following good project structure practices.
Also applies to: 316-324
1077-1077: LGTM: Build phases and group hierarchy are correctly configured.
The new files are:
- Properly added to the compilation sources
- Correctly integrated into the "Detail Selection" group hierarchy
- Set up with appropriate file references
Also applies to: 1107-1107, 521-521
Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteView.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Expense/Detail Selection/Notes/ExpenseAddNoteViewModel.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (8)
Splito/UI/Home/Expense/Notes/ExpenseAddNoteViewModel.swift (3)
12-22: Consider making required properties non-optional.The
groupandexpenseproperties are marked as optional but are required for the core update functionality (as seen inupdateExpenseNote()). Consider making them non-optional to enforce this requirement at compile-time and reduce optional unwrapping.- private let group: Groups? - private let expense: Expense? + private let group: Groups + private let expense: Expense
23-29: Add validation for required parameters.If
groupandexpenseare required for core functionality, consider adding validation in the initializer.- init(group: Groups?, expense: Expense?, expenseNote: String, handleSaveNoteTap: ((String) -> Void)? = nil) { + init(group: Groups, expense: Expense, expenseNote: String, handleSaveNoteTap: ((String) -> Void)? = nil) { self.group = group self.expense = expense self.expenseNote = expenseNote self.handleSaveNoteTap = handleSaveNoteTap super.init() }
32-34: Use localized strings for error messages.The error message is hardcoded. Consider using a localized string for better internationalization support.
- self.showToastFor(toast: ToastPrompt(type: .error, title: "Oops", message: "Failed to save note.")) + self.showToastFor(toast: ToastPrompt(type: .error, title: L10n.error, message: L10n.failedToSaveNote))Splito/UI/Home/Expense/Notes/ExpenseAddNoteView.swift (3)
16-17: Consider initializing tempNote with viewModel.expenseNoteWhile the note is set in
onAppear, initializingtempNotewith an empty string could potentially lead to data loss ifonAppearfails to execute. Consider initializing it withviewModel.expenseNotedirectly.- @State private var tempNote: String = "" + @State private var tempNote: String + + init(viewModel: ExpenseAddNoteViewModel) { + _viewModel = StateObject(wrappedValue: viewModel) + _tempNote = State(initialValue: viewModel.expenseNote) + }
75-75: Use design system color instead of direct color referenceFor consistency with the design system, use a color from BaseStyle instead of direct color reference.
- .foregroundStyle(.blue) + .foregroundStyle(accentColor)
21-31: Add accessibility supportConsider enhancing the TextField with accessibility modifiers for better VoiceOver support.
TextField("", text: $tempNote, prompt: Text(""), axis: .vertical) + .accessibilityLabel(LocalizedStringKey("expense.note.accessibility.label")) + .accessibilityHint(LocalizedStringKey("expense.note.accessibility.hint"))Splito/UI/Home/Expense/AddExpenseView.swift (2)
77-83: Consider adding explicit dismiss handlingWhile the sheet presentation is well-implemented, consider adding explicit dismiss handling to ensure proper cleanup when the sheet is dismissed. This could be important if there are any temporary states that need to be reset.
.sheet(isPresented: $viewModel.showAddNoteEditor) { NavigationStack { ExpenseAddNoteView(viewModel: ExpenseAddNoteViewModel(group: viewModel.selectedGroup, expense: viewModel.expense, expenseNote: viewModel.expenseNote, - handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:))) + handleSaveNoteTap: viewModel.handleNoteSaveBtnTap(note:))) + .onDisappear { + // Reset any temporary states if needed + } } }
Line range hint
1-293: Add documentation for the new note functionalityConsider adding documentation comments to describe:
- The purpose and usage of the note feature
- The interaction flow between components
- Any important state management details
This will help maintain the codebase and assist other developers in understanding the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Splito.xcodeproj/project.pbxproj(6 hunks)Splito/Localization/Localizable.xcstrings(4 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(4 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(3 hunks)Splito/UI/Home/Expense/Notes/ExpenseAddNoteView.swift(1 hunks)Splito/UI/Home/Expense/Notes/ExpenseAddNoteViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
🔇 Additional comments (6)
Splito/UI/Home/Expense/Notes/ExpenseAddNoteViewModel.swift (1)
1-11: LGTM! File structure and imports are appropriate.
The imports are minimal and necessary for the functionality.
Splito/UI/Home/Expense/AddExpenseView.swift (2)
38-40: LGTM: Parameter addition is well-structured
The new handleNoteTap parameter is properly integrated into the existing parameter list, with good formatting for readability.
215-215: LGTM: Footer view integration is clean
The note functionality is well-integrated into the footer view, maintaining consistent spacing and layout with existing elements.
Also applies to: 230-231
Splito.xcodeproj/project.pbxproj (3)
158-159: LGTM: File references properly added
The new files follow the project's MVVM pattern naming convention and are correctly referenced with proper Swift file types.
316-324: LGTM: Group structure properly organized
The new "Notes" group is correctly placed under the Expense section and follows the project's organizational structure, keeping related files together.
Also applies to: 511-511
1077-1077: LGTM: Build phases correctly updated
The new files are properly added to the compilation sources, ensuring they will be included in the build process.
Also applies to: 1107-1107
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Splito/UI/Home/Expense/AddExpenseView.swift (1)
38-40: Improve parameter organization in AddExpenseFooterView initializationThe parameters should be organized more consistently for better readability. Consider grouping related parameters together and aligning them properly.
-AddExpenseFooterView(date: $viewModel.expenseDate, showImagePickerOptions: $viewModel.showImagePickerOptions, - expenseImage: viewModel.expenseImage, expenseImageUrl: viewModel.expenseImageUrl, - handleNoteBtnTap: viewModel.handleNoteBtnTap, handleExpenseImageTap: viewModel.handleExpenseImageTap, - handleActionSelection: viewModel.handleActionSelection(_:)) +AddExpenseFooterView( + // State bindings + date: $viewModel.expenseDate, + showImagePickerOptions: $viewModel.showImagePickerOptions, + // Image properties + expenseImage: viewModel.expenseImage, + expenseImageUrl: viewModel.expenseImageUrl, + // Action handlers + handleNoteBtnTap: viewModel.handleNoteBtnTap, + handleExpenseImageTap: viewModel.handleExpenseImageTap, + handleActionSelection: viewModel.handleActionSelection(_:) +)Splito/UI/Home/Expense/AddExpenseViewModel.swift (1)
378-378: Consider adding note content validationWhile the note integration is correct, consider adding validation for the note content:
- Maximum length limits
- Content sanitization
- Handling of special characters
This would prevent potential issues with storage or display of the notes.
private func handleAddExpenseAction(userId: String, group: Groups) async -> Bool { + let sanitizedNote = expenseNote.trimmingCharacters(in: .whitespacesAndNewlines) + guard sanitizedNote.count <= 1000 else { + showToastFor(toast: ToastPrompt(type: .warning, + title: "Note too long", + message: "Please keep the note under 1000 characters")) + return false + } let expense = Expense(name: expenseName.trimming(spaces: .leadingAndTrailing), amount: expenseAmount, date: Timestamp(date: expenseDate), paidBy: selectedPayers, addedBy: userId, updatedBy: userId, - note: expenseNote, + note: sanitizedNote, splitTo: (splitType == .equally) ? selectedMembers : splitData.map({ $0.key }), splitType: splitType, splitData: splitData)Also applies to: 418-418
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Data/Data/Model/Expense.swift(2 hunks)Splito/Localization/Localizable.xcstrings(5 hunks)Splito/UI/Home/Expense/AddExpenseView.swift(5 hunks)Splito/UI/Home/Expense/AddExpenseViewModel.swift(9 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift(3 hunks)Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift(3 hunks)Splito/UI/Home/Expense/Notes/ExpenseAddNoteView.swift(1 hunks)Splito/UI/Home/Expense/Notes/ExpenseAddNoteViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Data/Data/Model/Expense.swift
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsView.swift
- Splito/UI/Home/Expense/Expense Detail/ExpenseDetailsViewModel.swift
- Splito/UI/Home/Expense/Notes/ExpenseAddNoteView.swift
- Splito/UI/Home/Expense/Notes/ExpenseAddNoteViewModel.swift
🔇 Additional comments (7)
Splito/UI/Home/Expense/AddExpenseView.swift (3)
77-83: LGTM! Sheet presentation follows SwiftUI best practices
The note editor sheet presentation is well-implemented with proper view model dependency injection.
215-215: LGTM! Clean integration of note functionality
The note button is well-integrated into the footer view with proper parameter passing.
Also applies to: 231-231
276-294: 🛠️ Refactor suggestion
Enhance accessibility and maintainability of NoteButtonView
The implementation needs improvements in several areas:
- Move keyboard dismissal logic to the view model
- Add accessibility label for better VoiceOver support
- Extract hard-coded dimensions to constants
private struct NoteButtonView: View {
+ private let iconSize: CGFloat = 24
+ private let iconPadding: CGFloat = 4
let handleNoteBtnTap: (() -> Void)
var body: some View {
Button {
- UIApplication.shared.endEditing()
handleNoteBtnTap()
} label: {
Image(.noteIcon)
.resizable()
.scaledToFit()
- .frame(width: 24, height: 24)
- .padding(4)
+ .frame(width: iconSize, height: iconSize)
+ .padding(iconPadding)
.background(container2Color)
.cornerRadius(8)
+ .accessibilityLabel("Add note to expense")
}
}
}Splito/UI/Home/Expense/AddExpenseViewModel.swift (4)
22-22: LGTM: Property declarations for note support
The new properties are properly declared with appropriate default values and access levels.
Also applies to: 31-31
55-56: Good improvement: Enhanced immutability
Converting expenseId and groupId from var to let improves thread safety and enforces proper initialization semantics.
217-224: LGTM: Clean implementation of note handling methods
The methods are concise and follow the single responsibility principle.
465-467: LGTM: Proper integration of note change detection
The note comparison is correctly integrated into the change detection logic, maintaining consistency with the existing implementation.
cp-amisha-i
force-pushed
the
allow-to-add-notes-with-expense
branch
from
d187779 to
0995e7d
Compare
Simulator.Screen.Recording.-.iPhone.16.-.2024-11-27.at.18.35.29.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Localization
UI Enhancements