Add contact support #94
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
BaseStyle/BaseStyle/CustomUI/Buttons/DismissButton.swift (1)
20-24: Consider using a configuration object pattern for better maintainability.While the initializer is functionally correct, it has many parameters which could make it harder to maintain as more customization options are added. Consider refactoring using a configuration object pattern.
Here's a suggested approach:
public struct DismissButtonConfiguration { let iconName: String let iconSize: (CGFloat, weight: Font.Weight) let padding: (horizontal: CGFloat, vertical: CGFloat) let borderColor: Color let foregroundColor: Color let backgroundColor: Color? public static let `default` = DismissButtonConfiguration( iconName: "multiply", iconSize: (24, .regular), padding: (0, 0), borderColor: .clear, foregroundColor: secondaryText, backgroundColor: nil ) } public init( configuration: DismissButtonConfiguration = .default, onDismissAction: (() -> Void)? = nil )This approach would:
- Improve readability
- Make it easier to add new customization options
- Provide a clear default configuration
- Reduce parameter count in the initializer
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (5)
13-14: Consider moving constants to a configuration fileThe
VIDEO_SIZE_LIMIT_IN_BYTESandTITLE_CHARACTER_MIN_LIMITconstants should be moved to a configuration file or settings bundle for easier maintenance and potential runtime configuration.
19-33: Add documentation for public propertiesConsider adding documentation comments for published properties to explain their purpose and usage. This will improve code maintainability and help other developers understand the view model's state management.
Example:
/// The user's feedback description text @Published var description: String = "" /// The current state of the feedback view @Published private(set) var currentState: ViewState = .initial
57-60: Consider extracting device information collectionThe device information collection logic could be moved to a separate utility class for better separation of concerns and reusability.
Example:
struct DeviceInfoProvider { static func getCurrentDeviceInfo() -> (appVersion: String, deviceName: String, osVersion: String) { return ( DeviceInfo.appVersionName, UIDevice.current.name, UIDevice.current.systemVersion ) } }
168-181: Add upload progress trackingConsider implementing upload progress tracking to provide better user feedback during file uploads.
Example:
private func upload(attachment: Attachment) { // ... existing code ... if let data = attachment.videoData { let progress = Progress(totalUnitCount: Int64(data.count)) // Implement progress tracking progress.addObserver(self, forKeyPath: "fractionCompleted", options: .new, context: nil) } }
207-210: Enhance ViewState enumConsider adding more states to better represent the view's lifecycle, such as
uploading,submitting,error, etc.enum ViewState { case initial case loading case uploading(progress: Double) case submitting case error(String) case success }Splito/UI/Home/Account/Feedback/FeedbackView.swift (3)
58-61: Consider extracting keyboard dismiss mode setup to a view modifierThe keyboard dismiss mode setup could be extracted to a reusable view modifier for better code organization.
Create a custom view modifier:
struct KeyboardDismissModeModifier: ViewModifier { func body(content: Content) -> some View { content.onAppear { UIScrollView.appearance().keyboardDismissMode = .interactive } } } extension View { func interactiveKeyboardDismissMode() -> some View { modifier(KeyboardDismissModeModifier()) } }Then use it like:
- .onAppear { - focusField = .title - UIScrollView.appearance().keyboardDismissMode = .interactive - } + .onAppear { + focusField = .title + } + .interactiveKeyboardDismissMode()
124-130: Remove duplicate foregroundColor modifierThe text view has duplicate foregroundColor modifiers.
Apply this diff:
Text(shouldShowValidationMessage ? (isValidTitle ? " " : "Minimum 3 characters are required") : " ") .foregroundColor(errorColor) .font(.body1(12)) - .foregroundColor(errorColor) .minimumScaleFactor(0.5) .lineLimit(1)
319-321: Consider adding configuration for supported media typesThe picker configuration could be more specific about supported media types and file size limits.
Apply this diff:
var configuration = PHPickerConfiguration() configuration.selectionLimit = 10 + configuration.filter = .any(of: [ + .images, + .videos(matching: [ + UTType.quickTimeMovie.identifier, + UTType.mpeg4Movie.identifier + ]) + ]) + configuration.preferredAssetRepresentationMode = .current let picker = PHPickerViewController(configuration: configuration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
BaseStyle/BaseStyle/CustomUI/Buttons/DismissButton.swift(2 hunks)Splito/Localization/Localizable.xcstrings(4 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/Localization/Localizable.xcstrings
🔇 Additional comments (3)
BaseStyle/BaseStyle/CustomUI/Buttons/DismissButton.swift (1)
12-12: LGTM! Well-structured property declaration.The new
iconNameproperty follows Swift naming conventions and has appropriate access level modifiers.Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (1)
105-121:⚠️ Potential issueFix potential race condition in deleteAttachment
The current implementation has a potential race condition when removing items from multiple collections. The index used to remove from
uploadingAttachmentsmay be invalid after removing fromselectedAttachments.- self.uploadingAttachments.removeAll { $0.id == self.selectedAttachments[index].id } + self.uploadingAttachments.removeAll { $0.id == attachment.id }Likely invalid or redundant comment.
Splito/UI/Home/Account/Feedback/FeedbackView.swift (1)
71-74: Prevent direct reference to function arguments in the sheet initThe sheet presentation still directly references the function argument which could cause immediate execution.
Apply this diff to fix the issue:
.sheet(isPresented: $viewModel.showMediaPicker) { - MultipleMediaSelectionPickerView(isPresented: $viewModel.showMediaPicker, - onDismiss: viewModel.onMediaPickerSheetDismiss(attachments:)) + MultipleMediaSelectionPickerView(isPresented: $viewModel.showMediaPicker, + onDismiss: { attachments in + viewModel.onMediaPickerSheetDismiss(attachments: attachments) + }) }
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (4)
24-26: Clarify selection limit.
Theconfiguration.selectionLimit = 10is a good approach to limit the number of items that can be picked.Consider whether 10 items is an intentional product requirement or if it should be configurable for future reuse.
56-56: Asynchronous image loading suggestion.
UsingimageRequestOptions.isSynchronous = trueblocks the thread while loading images. Consider using asynchronous loading to prevent potential UI thread blocking.
62-65: Add error-handling or fallback image.
If image loading fails, you log the error and move on. Consider providing a fallback (e.g., placeholder image) or user-facing error message so users know something went wrong.
55-92: Consider concurrency for loading tasks.
You are using aDispatchGroupfor concurrency, but each itemProvider load calls the main queue dispatch afterwards. If the total items are large, consider if off-main-queue runs can improve performance.Splito/UI/Home/Account/AccountHomeViewModel.swift (2)
30-31: Consider adding error handling for routing failures.While the implementation is clean and focused, consider handling potential routing failures to maintain a robust user experience. This is especially important as this is a user-facing support feature.
func onContactSupportTap() { - router.push(.FeedbackView) + do { + try router.push(.FeedbackView) + } catch { + showToastFor(toast: ToastPrompt( + type: .error, + title: "Error", + message: "Unable to open feedback form. Please try again." + )) + LogE("AccountHomeViewModel: Failed to open feedback view - \(error)") + } }
Line range hint
13-92: Great architectural improvements!The shift from email-based contact to a dedicated feedback system has improved the code architecture by:
- Reducing external dependencies
- Simplifying the user flow
- Maintaining consistency with existing patterns
- Following MVVM architecture with clear separation of concerns
The implementation aligns well with the existing dependency injection, routing, and state management patterns in the codebase.
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
13-14: Extract constants to a configuration fileConsider moving these constants to a dedicated configuration file or enum for better maintainability. Also, the video size limit should use byte-size literals for better readability.
- private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB + private let VIDEO_SIZE_LIMIT_IN_BYTES = 5 * 1024 * 1024 // 5 MB
19-32: Group related published propertiesConsider organizing related properties using private nested types for better code organization and maintainability. This will help manage the growing number of state variables.
private struct ViewState { var description: String = "" var title: String = "" var currentState: State = .initial var showLoader: Bool = false // ... other view state properties } @Published private var state: ViewState
57-60: Extract device information collection to a utility classConsider moving device information collection logic to a dedicated utility class for better separation of concerns and reusability.
struct DeviceInfoProvider { static func getCurrentDeviceInfo() -> DeviceInfo { return DeviceInfo( appVersion: DeviceInfo.appVersionName, deviceName: UIDevice.current.name, deviceOsVersion: UIDevice.current.systemVersion ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj(4 hunks)BaseStyle/BaseStyle/CustomUI/MailComposeView.swift(0 hunks)Data/Data/Utils/DeviceInfo.swift(1 hunks)Splito.xcodeproj/project.pbxproj(6 hunks)Splito/Localization/Localizable.xcstrings(5 hunks)Splito/UI/Home/Account/AccountHomeView.swift(2 hunks)Splito/UI/Home/Account/AccountHomeViewModel.swift(2 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift(1 hunks)
💤 Files with no reviewable changes (1)
- BaseStyle/BaseStyle/CustomUI/MailComposeView.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Data/Data/Utils/DeviceInfo.swift
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
🔇 Additional comments (11)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj (1)
12-12: LGTM! Project configuration changes look good.The integration of
MediaPickerOptionsView.swiftinto the project is properly configured:
- File references are correctly added with consistent identifiers
- File is appropriately placed in the CustomUI group
- Build phases are properly updated
Also applies to: 74-74, 308-308, 523-523
Splito/UI/Home/Account/AccountHomeView.swift (2)
29-29: Method reference updated toonContactSupportTapRenaming from
onContactUsTaptoonContactSupportTapis clear and consistent with the new feature's purpose.
117-117: Label text changed to "Contact Support"Renaming “Contact Us” to “Contact Support” more accurately reflects the updated functionality. No concerns here.
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (1)
29-29: Ensure consistent theming.
You're manually settingpicker.view.tintColor = UIColor(infoColor). If your project uses a dynamic theming system, confirm that this approach won't conflict with global styling.Splito.xcodeproj/project.pbxproj (5)
20-22: Approved addition of Feedback files.
AddingFeedbackView.swift,FeedbackViewModel.swift, andMultiMediaSelectionPickerView.swiftto the build phases ensures they will be compiled and packaged.
168-170: File references are set correctly.
These lines accurately reference the new files in thePBXFileReferencesection, aligning them with the project structure.
349-358: Appropriate grouping for new Feedback files.
Creating a dedicatedFeedbackgroup in the Xcode project helps maintain clarity and organization.
666-666: Confirmed integration into Account folder.
Inclusion of theFeedbackgroup underAccountensures logical categorization in your folder hierarchy.
1092-1092: Correctly added to sources build phase.
The new files (MultiMediaSelectionPickerView.swift,FeedbackViewModel.swift,FeedbackView.swift) are properly listed under thePBXSourcesBuildPhase. They will be compiled with the rest of the Swift code.Also applies to: 1118-1118, 1123-1123
Splito/UI/Home/Account/AccountHomeViewModel.swift (2)
10-10: LGTM! UIKit import is appropriately placed.The UIKit import is necessary for URL handling and application operations used throughout the class.
Line range hint
1-92: Verify cleanup of email-related functionality.Let's ensure all email-related code has been properly removed from the codebase.
✅ Verification successful
Let me perform additional verification to check for any remaining email-related code, particularly focusing on the contact functionality changes.
Let me check the FeedbackView implementation to understand how the contact functionality is now handled.
Email functionality has been properly replaced with in-app feedback system.
The email-based contact functionality has been completely replaced with a new in-app feedback system that:
- Allows users to submit feedback with title and description
- Supports attachments (images and videos)
- Handles file uploads to storage
- Stores feedback in the database with device information
- Shows appropriate success/error messages
The cleanup is complete with no remaining email-related code in the contact flow. The only email-related code that exists is for user authentication purposes, which is separate from the contact functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to email-related functionality # Check for MFMailCompose references echo "Checking for MFMailCompose references..." rg -i "MFMailCompose|MailComposeView|canSendMail" # Check for email-related strings echo "Checking for email-related strings..." rg -i "Contact Us|showMailToast|onContactUsTap" # Check for any remaining email validation or handling echo "Checking for email handling..." rg -i "mailto:|MFMailComposeResult|mailComposeController"Length of output: 1339
Script:
#!/bin/bash # Search for contact-related functionality in the codebase # Check for contact/support related methods and strings echo "Checking for contact/support related code..." rg -i "contact.*support|support.*contact|feedback.*support" # Check for any email validation or patterns echo "Checking for email patterns..." rg -i "email.*validation|validate.*email|email.*pattern" # Look for any remaining mail-related imports echo "Checking for mail-related imports..." rg -i "import.*MessageUI" # Check for specific feedback view implementation echo "Checking FeedbackView implementation..." fd -e swift "FeedbackView"Length of output: 43065
Script:
#!/bin/bash # Check the implementation of FeedbackView and related files echo "Checking FeedbackView implementation..." cat Splito/UI/Home/Account/Feedback/FeedbackView.swift echo -e "\nChecking FeedbackViewModel implementation..." cat Splito/UI/Home/Account/Feedback/FeedbackViewModel.swiftLength of output: 20006
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
functions/src/contact_support_service/contact_support_service.ts (1)
28-32: Avoid hardcoding email addresses.
It’s better to fetch recipient and sender addresses from environment variables or a secure configuration to avoid future code changes in case the email addresses change. It also improves security and maintainability.functions/src/mail_service/mail_service.ts (1)
54-59: Surface error context for more robust debugging.
When an email fails to send, you log the error and re-throw a generic error message. You might want to propagate additional details in a controlled manner to help debugging (e.g., the result ofsesClientif available).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
firebase-debug.logis excluded by!**/*.log
📒 Files selected for processing (4)
functions/package.json(1 hunks)functions/src/contact_support_service/contact_support_service.ts(1 hunks)functions/src/index.ts(2 hunks)functions/src/mail_service/mail_service.ts(1 hunks)
🔇 Additional comments (2)
functions/src/mail_service/mail_service.ts (1)
7-12: Double-check AWS credentials handling.
You correctly fail fast if credentials are missing. However, ensure you also review how these credentials are stored in Firebase config (e.g. applying least privilege if possible, limiting the IAM role specifically to SES usage).functions/package.json (1)
19-19: Keep an eye on@aws-sdk/client-sesfor security updates.
Pinned dependencies help avoid breaking changes, but ensure you routinely check for new versions or security patches in the AWS SDK as vulnerabilities and improvements are discovered.
functions/src/contact_support_service/contact_support_service.ts
Outdated
Show resolved
Hide resolved
functions/src/contact_support_service/contact_support_service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (1)
74-91:⚠️ Potential issueImprove temporary file handling for videos.
The temporary URL provided by
loadFileRepresentationis only valid during the callback. The current implementation might lead to accessing invalid URLs.Consider this safer approach:
attachment.itemProvider.loadFileRepresentation(forTypeIdentifier: UTType.movie.identifier) { url, error in if let url = url, let fileName = attachment.itemProvider.suggestedName { do { let data = try Data(contentsOf: url) + // Create a permanent URL in the app's temporary directory + let permanentURL = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString) + .appendingPathExtension("mov") + try data.write(to: permanentURL) - let videoObject = Attachment(videoData: data, video: url, name: fileName) + let videoObject = Attachment(videoData: data, video: permanentURL, name: fileName) attachments.append(videoObject) } catch { LogE("MultiMediaSelectionPickerCoordinator: \(#function) Error loading data from URL: \(error)") } } // ... rest of the code }
🧹 Nitpick comments (7)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (3)
13-22: Add documentation for public interface.Consider adding documentation comments for the public struct and its initializer to improve API clarity. This is especially important for reusable components.
Add documentation like this:
+/// A SwiftUI view that presents a media picker supporting multiple image and video selections. public struct MultiMediaSelectionPickerView: UIViewControllerRepresentable { + /// Binding to control the presentation state of the picker. @Binding var isPresented: Bool + /// Closure called when media selection is complete, providing an array of selected attachments. let onDismiss: ([Attachment]) -> Void + /// Creates a new media picker view. + /// - Parameters: + /// - isPresented: Binding to control the picker's presentation state + /// - onDismiss: Closure called with selected attachments when picker is dismissed public init(isPresented: Binding<Bool>, onDismiss: @escaping ([Attachment]) -> Void) {
24-40: Consider making configuration more flexible.The selection limit is currently hardcoded to 5. Consider making this configurable through the initializer to improve reusability.
Here's how you could improve it:
public struct MultiMediaSelectionPickerView: UIViewControllerRepresentable { @Binding var isPresented: Bool let onDismiss: ([Attachment]) -> Void + let selectionLimit: Int - public init(isPresented: Binding<Bool>, onDismiss: @escaping ([Attachment]) -> Void) { + public init(isPresented: Binding<Bool>, selectionLimit: Int = 5, onDismiss: @escaping ([Attachment]) -> Void) { self._isPresented = isPresented + self.selectionLimit = selectionLimit self.onDismiss = onDismiss } public func makeUIViewController(context: Context) -> PHPickerViewController { var configuration = PHPickerConfiguration() - configuration.selectionLimit = 5 + configuration.selectionLimit = selectionLimit
67-67: Specify image resize parameters.The
resizeImageIfNeededWhilePreservingAspectRatio()method is called without specific size limits. Consider adding maximum dimensions to prevent memory issues with large images.-let imageObject = Attachment(image: selectedImage.resizeImageIfNeededWhilePreservingAspectRatio(), name: fileName) +let imageObject = Attachment(image: selectedImage.resizeImageIfNeededWhilePreservingAspectRatio(maxDimension: 2048), name: fileName)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (4)
13-15: Extract magic numbers into documented constantsConsider moving these magic numbers into a configuration enum or struct with proper documentation explaining the rationale behind these limits.
- private let TITLE_CHARACTER_MIN_LIMIT = 3 - private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB - private let IMAGE_SIZE_LIMIT_IN_BYTES = 3000000 // 3 MB + private enum Constraints { + /// Minimum number of characters required for feedback title + static let titleMinLength = 3 + + /// Maximum video size (5 MB) to ensure optimal storage and loading + static let maxVideoSize = 5 * 1024 * 1024 + + /// Maximum image size (3 MB) to ensure optimal storage and loading + static let maxImageSize = 3 * 1024 * 1024 + + /// Maximum number of attachments allowed per feedback + static let maxAttachments = 5 + }
57-60: Extract device info collection into a separate utilityConsider moving the device info collection logic into a dedicated utility class to improve modularity and reusability.
+ struct DeviceInfoProvider { + static func getCurrentDeviceInfo() -> (appVersion: String, deviceName: String, osVersion: String) { + return ( + DeviceInfo.appVersionName, + UIDevice.current.name, + UIDevice.current.systemVersion + ) + } + } - let feedback = Feedback(title: title, description: description, userId: userId, - attachmentUrls: attachmentsUrls.map { $0.url }, appVersion: DeviceInfo.appVersionName, - deviceName: UIDevice.current.name, deviceOsVersion: UIDevice.current.systemVersion) + let deviceInfo = DeviceInfoProvider.getCurrentDeviceInfo() + let feedback = Feedback( + title: title, + description: description, + userId: userId, + attachmentUrls: attachmentsUrls.map { $0.url }, + appVersion: deviceInfo.appVersion, + deviceName: deviceInfo.deviceName, + deviceOsVersion: deviceInfo.osVersion + )
194-198: Add exponential backoff for upload retriesConsider implementing an exponential backoff retry mechanism for failed uploads to handle temporary network issues.
+ private func uploadWithRetry(data: Data, attachment: Attachment, type: StorageManager.AttachmentType, retryCount: Int = 0) { + let maxRetries = 3 + let backoffDelay = pow(2.0, Double(retryCount)) + + Task { [weak self] in + do { + // ... existing upload code ... + } catch { + if retryCount < maxRetries { + try? await Task.sleep(nanoseconds: UInt64(backoffDelay * 1_000_000_000)) + await self?.uploadWithRetry(data: data, attachment: attachment, type: type, retryCount: retryCount + 1) + } else { + self?.failedAttachments.append(attachment) + self?.uploadingAttachments.removeAll { $0.id == attachment.id } + LogE("FeedbackViewModel: \(#function) Failed to upload attachment after \(maxRetries) retries: \(error)") + } + } + } + }
204-208: Localize error messagesConsider moving error messages to a localization file for better internationalization support.
+ private enum LocalizedError { + static let genericError = NSLocalizedString("Something went wrong", comment: "Generic error message") + static let attachmentRemovalError = NSLocalizedString("Failed to remove attachment", comment: "Attachment removal error") + } func handleError(message: String? = nil) { if let message { - showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: message)) + showToastFor(toast: ToastPrompt(type: .error, title: NSLocalizedString("Error", comment: "Error title"), message: message)) } else { showToastForError() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Splito/Localization/Localizable.xcstrings(7 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Splito/Localization/Localizable.xcstrings
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift
[Warning] 152-152: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (4)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (1)
1-12: LGTM! File structure and imports are well-organized.The file includes all necessary framework imports and follows proper Swift conventions.
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
70-73: Fix potential retain cycle in alert closureThe closure capturing
selfcould create a retain cycle.
82-87: Add validation for maximum attachmentsConsider adding a limit to the number of attachments that can be added.
147-158: 🛠️ Refactor suggestionUse synchronized access for attachment arrays
Multiple arrays are modified without synchronization, which could lead to race conditions in concurrent scenarios. Consider using a serial queue for array modifications.
+ private let attachmentQueue = DispatchQueue(label: "com.splito.attachment.queue") + private func removeAttachmentFromArrays(attachmentId: String, removeAllSelected: Bool = false) { - withAnimation { + attachmentQueue.async { [weak self] in + guard let self = self else { return } attachmentsUrls.removeAll { $0.id == attachmentId } selectedAttachments.removeAll { $0.id == attachmentId } uploadingAttachments.removeAll { $0.id == attachmentId } - + if removeAllSelected { failedAttachments.removeAll { $0.id == attachmentId } uploadedAttachmentIDs.remove(attachmentId) } - } + DispatchQueue.main.async { + withAnimation {} + } + } }Likely invalid or redundant comment.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 152-152: Lines should not have trailing whitespace
(trailing_whitespace)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift
Outdated
Show resolved
Hide resolved
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
13-15: Improve constant naming and valuesConsider using more descriptive names and byte notation for size limits:
- private let TITLE_CHARACTER_MIN_LIMIT = 3 - private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB - private let IMAGE_SIZE_LIMIT_IN_BYTES = 3000000 // 3 MB + private let MIN_TITLE_LENGTH = 3 + private let MAX_VIDEO_SIZE = 5 * 1024 * 1024 // 5 MB + private let MAX_IMAGE_SIZE = 3 * 1024 * 1024 // 3 MB
12-46: Add documentation for public interfaceConsider adding documentation comments for the public interface to improve code maintainability:
+/// ViewModel responsible for managing user feedback submission and attachment handling class FeedbackViewModel: BaseViewModel, ObservableObject { + /// Minimum number of characters required for the feedback title private let TITLE_CHARACTER_MIN_LIMIT = 3 + /// User's feedback description @Published var description: String = ""
56-59: Extract device info collection to a separate methodConsider extracting device info collection to improve readability and reusability:
+ private func getDeviceInfo() -> (appVersion: String, deviceName: String, osVersion: String) { + return ( + appVersion: DeviceInfo.appVersionName, + deviceName: UIDevice.current.name, + osVersion: UIDevice.current.systemVersion + ) + } let feedback = Feedback(title: title, description: description, userId: userId, - attachmentUrls: attachmentsUrls.map { $0.url }, appVersion: DeviceInfo.appVersionName, - deviceName: UIDevice.current.name, deviceOsVersion: UIDevice.current.systemVersion) + attachmentUrls: attachmentsUrls.map { $0.url }, + appVersion: deviceInfo.appVersion, + deviceName: deviceInfo.deviceName, + deviceOsVersion: deviceInfo.osVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
🔇 Additional comments (2)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (2)
201-209: LGTM! Error handling looks goodThe error handling implementation is clean and provides good flexibility with optional custom messages.
211-215: LGTM! Focus field enum is well-definedThe enum is simple and effectively serves its purpose for managing form field focus.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (3)
13-24: Add documentation for public interface.Consider adding documentation comments for the public struct and its properties to improve API clarity. This is especially important for public interfaces.
+/// A SwiftUI view that presents a media picker interface for selecting multiple images and videos. public struct MultiMediaSelectionPickerView: UIViewControllerRepresentable { + /// Binding to control the presentation state of the picker @Binding var isPresented: Bool + /// Maximum number of items that can be selected let attachmentLimit: Int + /// Closure called with selected attachments when picker is dismissed let onDismiss: ([Attachment]) -> Void
26-33: Enhance picker configuration options.The current configuration is basic. Consider adding support for:
- Configurable media types (photos only, videos only, or both)
- Quality preferences
- Customizable appearance beyond just tintColor
public func makeUIViewController(context: Context) -> PHPickerViewController { var configuration = PHPickerConfiguration() configuration.selectionLimit = attachmentLimit + configuration.filter = .any(of: [.images, .videos]) + configuration.preferredAssetRepresentationMode = .current let picker = PHPickerViewController(configuration: configuration) picker.delegate = context.coordinator picker.view.tintColor = UIColor(infoColor) return picker }
94-98: Consider adding progress indication for large media processing.When processing multiple or large media files, users should see a loading indicator.
dispatchGroup.notify(queue: .main) { + // Assuming you add a loading state binding + self.isLoading = false self.isPresented = false self.onDismiss(attachments) }Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
14-16: Consider moving size limits to configurationThe character and size limits are hardcoded. Consider moving these to a configuration file or environment variables for easier maintenance and potential runtime configuration.
- private let TITLE_CHARACTER_MIN_LIMIT = 3 - private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB - private let IMAGE_SIZE_LIMIT_IN_BYTES = 3000000 // 3 MB + private var TITLE_CHARACTER_MIN_LIMIT: Int { AppConfig.shared.feedbackTitleMinLength } + private var VIDEO_SIZE_LIMIT_IN_BYTES: Int { AppConfig.shared.maxVideoSizeInBytes } + private var IMAGE_SIZE_LIMIT_IN_BYTES: Int { AppConfig.shared.maxImageSizeInBytes }
24-27: Consider consolidating attachment trackingMultiple arrays are used to track different attachment states. Consider using a single array of attachments with an enum state property for better state management.
+ enum AttachmentState { + case uploading, failed, uploaded + } + + struct TrackedAttachment { + let attachment: Attachment + var state: AttachmentState + var url: String? + } + - @Published var failedAttachments: [Attachment] = [] - @Published var attachmentsUrls: [(id: String, url: String)] = [] - @Published var selectedAttachments: [Attachment] = [] - @Published var uploadingAttachments: [Attachment] = [] + @Published private(set) var attachments: [TrackedAttachment] = []
57-59: Extract device info collection to a utilityDevice information collection is mixed with feedback creation. Consider extracting this to a separate utility class for better separation of concerns and reusability.
+ private func getDeviceInfo() -> (appVersion: String, deviceName: String, osVersion: String) { + return ( + DeviceInfo.appVersionName, + UIDevice.current.name, + UIDevice.current.systemVersion + ) + } + let feedback = Feedback(title: title, description: description, userId: userId, - attachmentUrls: attachmentsUrls.map { $0.url }, appVersion: DeviceInfo.appVersionName, - deviceName: UIDevice.current.name, deviceOsVersion: UIDevice.current.systemVersion) + attachmentUrls: attachmentsUrls.map { $0.url }, + appVersion: deviceInfo.appVersion, + deviceName: deviceInfo.deviceName, + deviceOsVersion: deviceInfo.osVersion)functions/src/feedback/feedback_service.ts (1)
41-46: Avoid hard-coding recipient email addresses.
Relying on a hard-coded recipient array can cause maintenance issues and hamper flexibility in different environments. Instead, store recipient addresses in configuration or environment variables.- await this.mailService.sendEmail( - ["[email protected]"], - "[email protected]", - "Splito: Feedback Request Created", - body, - ); + const toAddresses = process.env.FEEDBACK_RECIPIENTS?.split(",") || ["[email protected]"]; + await this.mailService.sendEmail( + toAddresses, + "[email protected]", + "Splito: Feedback Request Created", + body, + );functions/src/mail/mail_service.ts (1)
23-47: Consider adding inline logging or error handling.
When you create and send the email command, consider adding logs or capturing exceptions locally in case AWS SES returns detailed error messages. This could help diagnose failures more effectively.async sendEmail(to: string[], from: string, subject: string, body: string): Promise<void> { const mail = this.createSendEmailCommand(to, from, subject, body); try { + console.info(`Sending email to: ${to.join(", ")} with subject '${subject}'`); await sesClient.send(mail); + console.info(`Email sent successfully to: ${to.join(", ")}`); } catch (error) { + console.error(`Failed to send email to: ${to.join(", ")}`, error); throw error; } }functions/src/index.ts (1)
30-46: Validate feedback data structure.
Before callingonFeedbackCreated, consider verifying that essential fields (such ascreated_atorattachment_urls) exist or meet certain criteria (e.g., maximum attachment count). This helps protect against partially formed or malformed documents in the "feedbacks" collection.const data = snapshot.data(); + if (!data || typeof data !== 'object') { + logger.error("Feedback data is missing or improperly structured"); + return; + } try { await feedbackService.onFeedbackCreated(data); } catch (error) { logger.error('Error handling feedback:', error); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
BaseStyle/BaseStyle/Resource/AppColors.swift(1 hunks)BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/SecondaryLightTextColor.colorset/Contents.json(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift(1 hunks)functions/src/feedback/feedback_service.ts(1 hunks)functions/src/index.ts(2 hunks)functions/src/mail/mail_service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/SecondaryLightTextColor.colorset/Contents.json
🚧 Files skipped from review as they are similar to previous changes (1)
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
🔇 Additional comments (8)
BaseStyle/BaseStyle/Resource/AppColors.swift (1)
36-36: Ensure color accessibility standards are addressed.Adding a new color constant is useful; however, please confirm it meets accessibility requirements, including contrast ratios in both light and dark modes.
Would you like me to run a color contrast check against typical foreground/background usage scenarios in your codebase?
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (3)
8-11: LGTM! All necessary imports are present.
58-59: Avoid synchronous image loading.Using synchronous loading can cause UI freezes, especially with large images.
67-75: Enhance error handling and user feedback.Currently, errors are only logged without user feedback.
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (1)
186-205: 🛠️ Refactor suggestionAdd retry mechanism with exponential backoff
The upload functionality should include a retry mechanism with exponential backoff for better reliability in poor network conditions.
+ private let MAX_RETRIES = 3 + private func uploadWithRetry(data: Data, attachment: Attachment, type: StorageManager.AttachmentType, retryCount: Int = 0) { + guard retryCount < MAX_RETRIES else { + failedAttachments.append(attachment) + uploadingAttachments.removeAll { $0.id == attachment.id } + LogE("FeedbackViewModel: Max retries exceeded for attachment: \(attachment.id)") + return + } + + let delay = TimeInterval(pow(2.0, Double(retryCount))) + + Task { [weak self] in + do { + try await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) + try await self?.uploadAttachment(data: data, attachment: attachment, type: type) + } catch { + self?.uploadWithRetry(data: data, attachment: attachment, type: type, retryCount: retryCount + 1) + } + } + }Likely invalid or redundant comment.
functions/src/feedback/feedback_service.ts (1)
5-6: Ensure consistent naming conventions and method responsibilities.
While the class name and method name are expressive, consider clarifying in doc comments whetherFeedbackServicemight handle more feedback-related operations in the future.functions/src/mail/mail_service.ts (1)
10-11: Ensure environment variables are configured securely.
The code throws an error if credentials are missing, which is good. Confirm that your deployment process securely setsaws.access_key_idandaws.secret_access_keyin the Firebase Functions config and that these configs won’t be accidentally exposed in logs or other outputs.functions/src/index.ts (1)
6-8: Confirm region setup consistency.
Here, the GCP region is set to"asia-south1", while AWS uses"ap-south-1". Confirm that the region mismatch is intentional. Using separate regions for GCP and AWS is valid but ensure you’re aware of any latency or compliance implications.
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift
Outdated
Show resolved
Hide resolved
cp-nirali-s
force-pushed
the
add-send-feedback-option
branch
from
e4536bf to
e064228
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (4)
26-33: Consider adding media type filters to the configuration.The current configuration only sets the selection limit. Consider adding media type filters to restrict selection to specific types:
var configuration = PHPickerConfiguration() configuration.selectionLimit = attachmentLimit +configuration.filter = PHPickerFilter.any(of: [.images, .videos]) +configuration.preferredAssetRepresentationMode = .current
35-37: Document the empty updateUIViewController method.Add a documentation comment explaining why no updates are needed:
public func updateUIViewController(_ uiViewController: PHPickerViewController, context: Context) { - // Nothing to update here + // This method is required by UIViewControllerRepresentable but not needed + // as PHPickerViewController is presented modally and doesn't need updates }
49-51: Consider caching and memory management for PHImageManager.The current implementation could benefit from image caching and memory management settings:
let imageManager = PHImageManager.default() let imageRequestOptions = PHImageRequestOptions() +imageRequestOptions.deliveryMode = .opportunistic +imageRequestOptions.isNetworkAccessAllowed = true +imageRequestOptions.version = .current
95-99: Consider weak self in dispatch group notification.To prevent potential retain cycles, use
[weak self]in the closure:-dispatchGroup.notify(queue: .main) { +dispatchGroup.notify(queue: .main) { [weak self] in - self.isPresented = false - self.onDismiss(attachments) + guard let self = self else { return } + self.isPresented = false + self.onDismiss(attachments) }Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
13-16: Follow Swift naming conventions for constantsConstants should be static and use camelCase naming convention.
- let MAX_ATTACHMENTS = 5 - private let TITLE_CHARACTER_MIN_LIMIT = 3 - private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB - private let IMAGE_SIZE_LIMIT_IN_BYTES = 3000000 // 3 MB + static let maxAttachments = 5 + private static let titleCharacterMinLimit = 3 + private static let videoSizeLimitInBytes = 5_000_000 // 5 MB + private static let imageSizeLimitInBytes = 3_000_000 // 3 MB
51-75: Add validation for empty descriptionThe
onSubmitBtnTapmethod validates the title but not the description. Consider adding validation for empty description to ensure meaningful feedback.func onSubmitBtnTap() { - guard let userId = preference.user?.id, isValidTitle else { + guard let userId = preference.user?.id, + isValidTitle, + !description.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { shouldShowValidationMessage = !isValidTitle + handleError(message: "Please provide a description for your feedback.") return }
199-206: Localize error messagesError messages are hardcoded strings. Consider using localized strings for better internationalization support.
func handleError(message: String? = nil) { if let message { - showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: message)) + showToastFor(toast: ToastPrompt( + type: .error, + title: NSLocalizedString("error.title", comment: "Error alert title"), + message: message)) } else { showLoader = false showToastForError() } }Data/Data/Utils/DeviceInfo.swift (3)
10-10: Add documentation for the DeviceInfo class.Consider adding documentation comments to describe the purpose and usage of this utility class. This will help other developers understand when and how to use these device information utilities.
+/// A utility class that provides access to device-specific information such as device name, OS version, and app version. public class DeviceInfo {
16-18: Add documentation for deviceOsVersion property.Add documentation to explain the property's purpose and return value format.
+/// Returns the operating system version (e.g., "16.2"). +/// - Returns: The current iOS version string. public static var deviceOsVersion: String {
20-22: Enhance version information and add documentation.Consider including the build number along with the version string for better debugging capabilities. Also, add documentation to explain the property's purpose.
+/// Returns the application's version string from Info.plist. +/// - Returns: The application's version (e.g., "1.0.0") or empty string if not found. public static var appVersionName: String { - return Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String ?? "" + let version = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String ?? "" + let build = Bundle.main.infoDictionary?["CFBundleVersion"] as? String ?? "" + return "\(version) (\(build))" }functions/src/index.ts (1)
23-25: Consider enhancing service initialization.While the service initialization order is correct, consider these improvements:
- Move the REGION constant to a configuration file for better maintainability
- Add error handling for service initialization
Example improvement:
+ import { config } from './config'; - const REGION = "asia-south1"; + const { REGION } = config; - const mailService = new MailService(); - const feedbackService = new FeedbackService(mailService); + let mailService: MailService; + let feedbackService: FeedbackService; + try { + mailService = new MailService(); + feedbackService = new FeedbackService(mailService); + logger.info('Services initialized successfully'); + } catch (error) { + logger.error('Failed to initialize services:', error); + throw error; + }Data/Data/Helper/Firebase/StorageManager.swift (2)
Line range hint
17-33: Consider renamingImageStoreTypeto better reflect its purposeThe enum now handles various attachment types, not just images. Consider renaming it to
StorageTypeorAttachmentStoreTypefor better clarity.-public enum ImageStoreType { +public enum StorageType {
Line range hint
78-87: Update logging messages and add URL validationThe logging messages still reference "image" instead of "attachment". Also, consider adding URL validation.
public func deleteAttachment(attachmentUrl: String) async throws { + guard URL(string: attachmentUrl)?.scheme == "https" else { + throw StorageError.invalidUrl + } do { let storageRef = storage.reference(forURL: attachmentUrl) try await storageRef.delete() - LogD("StorageManager: \(#function) Image deleted successfully.") + LogD("StorageManager: \(#function) Attachment deleted successfully.") } catch { - LogE("StorageManager: \(#function) Failed to delete image: \(error).") + LogE("StorageManager: \(#function) Failed to delete attachment: \(error).") throw error } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
BaseStyle/BaseStyle/Resource/AppColors.swift(1 hunks)BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/SecondaryLightTextColor.colorset/Contents.json(1 hunks)Data/Data/Helper/Firebase/StorageManager.swift(2 hunks)Data/Data/Utils/DeviceInfo.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift(1 hunks)functions/src/feedback/feedback_service.ts(1 hunks)functions/src/index.ts(2 hunks)functions/src/mail/mail_service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- BaseStyle/BaseStyle/Resource/AppColors.swift
- BaseStyle/BaseStyle/Resource/BaseAssets.xcassets/Colors/SecondaryLightTextColor.colorset/Contents.json
- functions/src/feedback/feedback_service.ts
- functions/src/mail/mail_service.ts
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
Splito/UI/Home/Account/Feedback/MultiMediaSelectionPickerView.swift (4)
1-12: LGTM! File structure and imports are well-organized.The file includes all necessary imports and follows proper Swift conventions.
13-24: LGTM! Well-structured view declaration and initialization.The public API is clean, properties are well-named, and the initialization follows SwiftUI best practices.
67-75: Previous comment about error handling is still applicable.The error handling needs improvement to provide better user feedback.
77-92: Previous comment about video handling and memory management is still applicable.The implementation needs video size limits and proper cleanup.
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (1)
210-212: LGTM!The
FocusedFieldenum is well-defined and serves its purpose.Data/Data/Utils/DeviceInfo.swift (2)
10-23: Add unit tests for DeviceInfo class.Since this class is used for collecting support information, it's crucial to ensure it works correctly across different scenarios. Consider adding unit tests to verify:
- Proper version string parsing
- Handling of missing Info.plist values
- Format of the combined version and build number
Would you like me to help create a test suite for this class?
12-14: Consider privacy implications of device name.The device name often contains personal information (e.g., "John's iPhone"). Consider if this is necessary for your use case or if a more privacy-preserving alternative would suffice.
Also, add documentation to explain the property's purpose and potential privacy implications:
+/// Returns the device name. Note: This may contain personal information set by the user. +/// - Returns: The current device name as set in system settings. public static var deviceName: String {✅ Verification successful
Device name usage is appropriate for feedback diagnostics
The device name is only collected during user-initiated feedback submission, along with other diagnostic information like app version and OS version. This is a standard practice for support purposes and is covered by the app's privacy policy. The implementation is appropriate as-is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for usages of deviceName echo "=== Searching for deviceName usages ===" rg "deviceName" -B 2 -A 2 echo -e "\n=== Searching for privacy-related documentation ===" rg -i "privacy|gdpr|personal.?data" -g "*.{md,swift}" echo -e "\n=== Looking for alternative device identifiers ===" rg "identifier|UUID|UDID|deviceId" -g "*.swift" -B 2 -A 2Length of output: 11307
functions/src/index.ts (2)
4-8: LGTM! Import statements are well-organized.The imports are properly structured and follow a consistent pattern, with clear separation between Firebase core imports and custom service imports.
30-32: Verify feedback document path structure.The feedback observer is correctly configured, but let's verify the document path structure in the codebase.
✅ Verification successful
Feedback document path structure is correctly implemented
The document path "feedbacks/{feedbackId}" is consistently used throughout the codebase, and there are no conflicting patterns or variations. The FeedbackService handles only the email notifications without direct Firestore operations, maintaining a clean separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the feedback collection to ensure consistent path usage rg -g '!node_modules' -g '*.{ts,js}' -i 'feedbacks/[^/]+' --context 2Length of output: 356
Script:
#!/bin/bash # Search for other variations of feedback collection references and related types echo "=== Searching for feedback collection references ===" rg -g '!node_modules' -g '*.{ts,js}' -i 'feedback/' --context 2 echo -e "\n=== Searching for feedbackId references ===" rg -g '!node_modules' -g '*.{ts,js}' 'feedbackId' --context 2 echo -e "\n=== Searching for Feedback type definitions ===" ast-grep --pattern 'interface Feedback { $$$ }'Length of output: 1214
Script:
#!/bin/bash # Check feedback service implementation echo "=== Checking feedback service implementation ===" cat functions/src/feedback/feedback_service.ts 2>/dev/null || echo "File not found" echo -e "\n=== Searching for feedback collection path constants ===" rg -g '!node_modules' -g '*.{ts,js}' -i 'const.*feedback.*collection' --context 2Length of output: 1765
Data/Data/Helper/Firebase/StorageManager.swift (1)
Line range hint
1-87: Verify existing image handling functionalityThe refactoring from image-specific to general attachment handling is significant. Please ensure all existing image upload/update flows continue to work correctly.
Run the following script to find all existing image upload/update usages that might be affected:
✅ Verification successful
Refactoring is already properly implemented ✅
All repositories are already using the new generalized attachment API with proper ImageStoreType cases:
- UserRepository:
.user- GroupRepository:
.group- TransactionRepository:
.payment- ExpenseRepository:
.expense🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all references to the old image methods rg -l "uploadImage|updateImage|deleteImage" --type swift # Find all usages of ImageStoreType to verify they're updated rg "ImageStoreType\." --type swift -A 2Length of output: 377
Script:
#!/bin/bash # Check actual usage in repositories echo "=== UserRepository.swift ===" rg "StorageManager" Data/Data/Repository/UserRepository.swift -B 2 -A 5 echo -e "\n=== GroupRepository.swift ===" rg "StorageManager" Data/Data/Repository/GroupRepository.swift -B 2 -A 5 echo -e "\n=== TransactionRepository.swift ===" rg "StorageManager" Data/Data/Repository/TransactionRepository.swift -B 2 -A 5 echo -e "\n=== ExpenseRepository.swift ===" rg "StorageManager" Data/Data/Repository/ExpenseRepository.swift -B 2 -A 5 echo -e "\n=== ImagePickerView.swift ===" rg "StorageManager" BaseStyle/BaseStyle/CustomUI/ImagePickerView.swift -B 2 -A 5Length of output: 2464
Script:
#!/bin/bash # Check for actual storage method calls in repositories echo "=== UserRepository Storage Calls ===" rg "storageManager\." Data/Data/Repository/UserRepository.swift -B 1 -A 3 echo -e "\n=== GroupRepository Storage Calls ===" rg "storageManager\." Data/Data/Repository/GroupRepository.swift -B 1 -A 3 echo -e "\n=== TransactionRepository Storage Calls ===" rg "storageManager\." Data/Data/Repository/TransactionRepository.swift -B 1 -A 3 echo -e "\n=== ExpenseRepository Storage Calls ===" rg "storageManager\." Data/Data/Repository/ExpenseRepository.swift -B 1 -A 3Length of output: 3149
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (3)
13-16: Move constants to a configuration namespaceConsider organizing the constants in a nested Configuration enum for better maintainability.
- let MAX_ATTACHMENTS = 5 - private let TITLE_CHARACTER_MIN_LIMIT = 3 - private let VIDEO_SIZE_LIMIT_IN_BYTES = 5000000 // 5 MB - private let IMAGE_SIZE_LIMIT_IN_BYTES = 3000000 // 3 MB + private enum Configuration { + static let maxAttachments = 5 + static let titleCharacterMinLimit = 3 + static let videoSizeLimitInBytes = 5_000_000 // 5 MB + static let imageSizeLimitInBytes = 3_000_000 // 3 MB + }
86-88: Use Configuration constant instead of direct MAX_ATTACHMENTS accessReplace the direct access to MAX_ATTACHMENTS with the Configuration constant.
- if attachmentsUrls.count >= MAX_ATTACHMENTS { - handleError(message: "Maximum \(MAX_ATTACHMENTS) attachments allowed.") + if attachmentsUrls.count >= Configuration.maxAttachments { + handleError(message: "Maximum \(Configuration.maxAttachments) attachments allowed.")
127-129: Document empty case handlingAdd a comment explaining why these cases are empty.
case .camera, .remove: + // These cases are handled by the view layer break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (4)
41-46:⚠️ Potential issueMake router reference weak to prevent retain cycles
The router is stored as a strong reference which could create a retain cycle.
- private let router: Router<AppRoute> + private weak var router: Router<AppRoute>? init(router: Router<AppRoute>) { self.router = router super.init() }Likely invalid or redundant comment.
67-69:⚠️ Potential issueFix potential retain cycle in alert closure
The closure capturing
selfcould create a retain cycle.self.alert = .init(message: "Thanks! your feedback has been recorded.", positiveBtnTitle: "Ok", - positiveBtnAction: { [weak self] in self?.router.pop() }) + positiveBtnAction: { [weak self] in + guard let self = self else { return } + self.router?.pop() + })Likely invalid or redundant comment.
200-207: 🛠️ Refactor suggestionImprove error handling with specific error types and localization
The current error handling is too generic. Consider implementing specific error types and localized messages.
+ enum FeedbackError: LocalizedError { + case attachmentSizeExceeded(type: String, limit: Int) + case uploadFailed(reason: String) + case deletionFailed(reason: String) + + var errorDescription: String? { + switch self { + case .attachmentSizeExceeded(let type, let limit): + return String(format: NSLocalizedString("error_attachment_size_exceeded_%@_%d", comment: ""), type, limit) + case .uploadFailed(let reason): + return String(format: NSLocalizedString("error_upload_failed_%@", comment: ""), reason) + case .deletionFailed(let reason): + return String(format: NSLocalizedString("error_deletion_failed_%@", comment: ""), reason) + } + } + } func handleError(message: String? = nil) { - if let message { - showToastFor(toast: ToastPrompt(type: .error, title: "Error", message: message)) + if let error = message as? FeedbackError { + showToastFor(toast: ToastPrompt(type: .error, + title: NSLocalizedString("error", comment: ""), + message: error.localizedDescription)) } else { showLoader = false showToastForError() } }Likely invalid or redundant comment.
171-197:⚠️ Potential issueAdd file type validation and task cancellation
The attachment upload needs file type validation and proper task cancellation support.
+ private let SUPPORTED_IMAGE_TYPES = ["image/jpeg", "image/png"] + private let SUPPORTED_VIDEO_TYPES = ["video/mp4", "video/quicktime"] + private var attachmentTasks: [String: Task<Void, Never>] = [:] private func uploadAttachment(data: Data, attachment: Attachment, maxSize: Int, type: StorageManager.AttachmentType) { + guard let mimeType = attachment.mimeType else { + handleError(message: "Unsupported file format") + return + } + + switch type { + case .image: + guard SUPPORTED_IMAGE_TYPES.contains(mimeType) else { + handleError(message: "Unsupported image format. Please use JPEG or PNG.") + return + } + case .video: + guard SUPPORTED_VIDEO_TYPES.contains(mimeType) else { + handleError(message: "Unsupported video format. Please use MP4 or MOV.") + return + } + } if data.count <= maxSize { uploadingAttachments.append(attachment) - Task { [weak self] in + let task = Task { [weak self] in do { let attachmentId = attachment.id let attachmentUrl = try await self?.feedbackRepository.uploadAttachment(attachmentId: attachmentId, attachmentData: data, attachmentType: type) if let attachmentUrl { self?.attachmentsUrls.append((id: attachmentId, url: attachmentUrl)) self?.uploadingAttachments.removeAll { $0.id == attachmentId } } LogD("FeedbackViewModel: \(#function) Attachment uploaded successfully.") } catch { self?.failedAttachments.append(attachment) self?.uploadingAttachments.removeAll { $0.id == attachment.id } LogE("FeedbackViewModel: \(#function) Failed to upload attachment: \(error)") } } + attachmentTasks[attachment.id] = task } } + deinit { + attachmentTasks.values.forEach { $0.cancel() } + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj (1)
Tests needed for new UI components
The test target exists but lacks test coverage for the new MultiMediaSelectionPickerView and MediaPickerOptionsView components. The BaseStyleTests.swift file only contains template code with no actual test implementations.
- Add tests for
MultiMediaSelectionPickerView:
- Media selection state management
- Multiple selection behavior
- Selection limits and validation
- Add tests for
MediaPickerOptionsView:
- Options presentation
- Option selection handling
- View state updates
🔗 Analysis chain
Line range hint
10-527: Consider adding tests for the new components.While the project structure changes are correct, ensure that appropriate test coverage is added for the new MultiMediaSelectionPickerView and MediaPickerOptionsView components.
Run this script to check for test coverage:
Consider adding test cases for:
- Media selection functionality
- Multiple media handling
- Error cases and edge conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test coverage of the new components # Search for test files related to the new components echo "Searching for test files..." fd -e swift -e xctestplan "test.*MediaPicker|MediaPicker.*test" # Search for test cases within existing test files echo "Searching for test cases in existing files..." rg -l "test.*MediaPicker|func test.*media|func test.*picker" --type swiftLength of output: 307
Script:
#!/bin/bash # First, find all test-related files to understand the testing structure echo "=== Searching for all test files ===" fd -e swift ".*Tests?\.swift$" echo -e "\n=== Searching for test target configuration ===" fd ".*\.xctest" echo -e "\n=== Checking for test directories ===" fd -t d ".*[Tt]est.*" echo -e "\n=== Searching for test-related strings in project file ===" rg "Test" "BaseStyle/BaseStyle.xcodeproj/project.pbxproj"Length of output: 3827
Script:
#!/bin/bash # Check content of BaseStyleTests.swift echo "=== Content of BaseStyleTests.swift ===" cat "BaseStyle/BaseStyleTests/BaseStyleTests.swift" # Look for any existing view-related tests echo -e "\n=== Searching for view-related tests ===" rg "class.*Test|func test.*View" "BaseStyle/BaseStyleTests/BaseStyleTests.swift"Length of output: 1590
🧹 Nitpick comments (9)
BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift (8)
13-14: Consider exposing comment documentation for clarity.Documenting how
isPresentedis used to control picker visibility can help future maintainers quickly understand the binding’s purpose.
24-31: InitializePHPickerConfigurationwith desired filter(s).If you intend to allow only images and videos, consider specifying
configuration.filter = .any(of: [.images, .videos])to reduce undesired asset types.var configuration = PHPickerConfiguration() +configuration.filter = .any(of: [.images, .videos]) configuration.selectionLimit = attachmentLimit let picker = PHPickerViewController(configuration: configuration)
37-41: Leverage Swift concurrency for clarity.Using
DispatchGroupworks fine, but adopting Swift’s structured concurrency (e.g.,async/await) might improve readability and reduce potential race conditions.
50-53: InitializePHImageRequestOptionsearlier or pass as parameter.Currently,
imageRequestOptionsis modified indidFinishPicking(line 56). Consider making it a constant property or passing it in to avoid potential side effects if extended in the future.
64-73: Handle large image memory usage gracefully.If users select very large images, resizing and memory usage can become a bottleneck. You might consider adding asynchronous scaling or progressive decoding for more robust memory management.
75-90: Ensure robust video file error handling.Currently, an error is printed but not returned to the user. For better UX, consider prompting the user if a video fails to load.
81-81: Check for file removal errors.
try? FileManager.default.removeItem(at: url)silently ignores potential errors. Consider logging or handling these more explicitly, especially if failure to remove temp files leads to disk usage growth.
93-96: Prompt user after finishing selection.Currently, the code dismisses the picker once all attachments load. If you'd like a user confirmation step (e.g., “Are you sure?”), that can happen before closing. This ensures the user is fully aware of the attachments being added.
Splito.xcodeproj/project.pbxproj (1)
663-663: Check references to 'Feedback' under 'Account' folder.Ensure that referencing the new group under “Account” is an intentional design choice and doesn't conflict with future expansions (e.g., Feedback might also be used in other modules).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj(4 hunks)BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift(1 hunks)Data/Data/Model/Feedback.swift(1 hunks)Splito.xcodeproj/project.pbxproj(5 hunks)functions/src/feedback/feedback_service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Data/Data/Model/Feedback.swift
- functions/src/feedback/feedback_service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
BaseStyle/BaseStyle.xcodeproj/project.pbxproj (2)
310-311: LGTM! New UI components are properly organized.The new media picker components are correctly placed in the CustomUI group, maintaining the project's organizational structure.
10-10: Verify the impact of file changes on existing functionality.The changes look good from a project structure perspective. However, since MailComposeView.swift has been removed (as mentioned in the summary) and replaced with new media picker components, we should ensure:
- All previous email composition functionality has been properly migrated
- Any existing features that depended on MailComposeView have been updated
Run this script to check for any remaining references to the removed functionality:
Also applies to: 13-13, 73-73, 76-76, 521-521, 527-527
✅ Verification successful
Clean removal confirmed - no remaining dependencies found
The verification shows that MailComposeView and related email composition functionality have been completely removed from the project. All found mail-related references are for different features (authentication, user management) and not related to the removed email composition functionality. The addition of MultiMediaSelectionPickerView appears to be a clean replacement with no conflicting dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to MailComposeView that might need updating # Search for references to the removed MailComposeView echo "Searching for references to MailComposeView..." rg -l "MailComposeView" # Search for any email-related functionality that might need migration echo "Searching for email-related functionality..." rg -l "MFMailComposeViewController|canSendMail|mailComposeDelegate"Length of output: 295
Script:
#!/bin/bash # Case-insensitive search for mail-related terms echo "Searching for mail-related terms (case-insensitive)..." rg -il "mail|compose|mailcompose|mailcomposeview" # Check specifically in the project file for the removed MailComposeView echo "Checking project file for MailComposeView references..." rg "MailComposeView" BaseStyle/BaseStyle.xcodeproj/project.pbxproj # Search for any import statements related to mail functionality echo "Searching for mail-related imports..." ast-grep --pattern 'import $_Mail$_'Length of output: 1512
BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift (4)
11-12: Great use ofUIViewControllerRepresentable.By conforming to
UIViewControllerRepresentable, this struct seamlessly bridges SwiftUI and UIKit'sPHPickerViewController, ensuring minimal boilerplate. This approach is clean and follows Apple's recommended patterns.
18-22: Validate theattachmentLimit.When
attachmentLimitis set to a very large number or zero, confirm that the UI and data processing can handle edge cases without crashing or leading to memory issues.Would you like a script to probe how
attachmentLimitis used throughout the code?
29-29: Handle potential fallback for tint color.
UIColor(infoColor)suggests a custom color, but consider a fallback color ifinfoColoris missing or doesn't exist in certain contexts.
102-116: Good design of theAttachmentstruct.Providing distinct fields for
image,videoData, andvideoplus an identifying name is concise and flexible. Nice job making it public and reusable.Splito.xcodeproj/project.pbxproj (5)
20-21: New feedback-related files are properly added to the build.The references to “FeedbackView.swift" and “FeedbackViewModel.swift" in sources ensure these files are compiled. No issues spotted here.
167-168: File references for feedback files appear consistent.These references match the newly added Swift files. Confirm that the filenames match exactly to avoid broken references.
347-355: Great addition of a 'Feedback' group for project organization.Creating a dedicated group keeps related files together, enhancing maintainability.
1114-1114: FeedbackViewModel added to the build phase.Ensure all necessary frameworks or dependencies (e.g., for uploading attachments) are linked here or in separate build phases as needed.
1119-1119: FeedbackView included in the build phase.Everything looks consistent. If you have any usage within SwiftUI previews, confirm that Previews can reference it as well.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift (4)
11-22: Ensure consistent access modifiers and documentation.Since
MultiMediaSelectionPickerViewis marked public, consider documenting it for future reference and maintaining consistent access control for its properties, ensuring that only the necessary members are exposed.
29-29: Customize tintColor with caution.While setting a custom tint color can help with brand consistency, ensure that other UI elements remain accessible, especially for users with visual impairments. Consider system dynamic colors or high-contrast palette.
73-88: Check video loading behavior for large files.Loading video data into memory might lead to memory usage issues. Consider streaming or partial transfer if large video files are expected.
99-114: Add MIME-type checks for attachments.While the selected attachments are validated by iOS Photos, additional checks for MIME-types or file extensions can guard against unexpected or corrupted files.
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift (7)
32-34: Remove trailing whitespace to comply with SwiftLint.The static analysis flagged trailing whitespace. Consider removing them for cleaner diffs and code style compliance.
- +🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 34-34: Lines should not have trailing whitespace
(trailing_whitespace)
41-47: Consider a weak reference for router.Storing
routeras a strong reference can lead to retain cycles depending on the navigation flow. Evaluate changing this toweak var router: Router<AppRoute>?.- private let router: Router<AppRoute> + private weak var router: Router<AppRoute>?
80-85: Add concurrency safeguards when appending attachments.Multiple attachments can be processed concurrently by the system or user repetition. Consider using a synchronization mechanism when modifying
selectedAttachments.
100-114: Cancel in-progress upload before removing attachment.If an attachment is being uploaded, consider canceling that upload task first to avoid wasted resources.
150-161: Avoid potential data races on arrays.Multiple async tasks may modify
attachmentsUrls,selectedAttachments, anduploadingAttachments. Accessing these arrays withinwithAnimationimplies the main thread is used, but consider a single, well-defined queue or other concurrency-safe approaches if these lists grow large or are mutated in background threads.
163-171: Implement fallback for unsupported file types.Currently, the code only checks for image or video data. For better error handling, add a fallback path or error if an attachment is of unknown type.
173-199: Include progress or queued-state to inform users.Uploads can take time, especially for large files. Consider providing progress status or queued-state to improve user experience.
Splito/UI/Home/Account/Feedback/FeedbackView.swift (2)
131-142: Rename “titleText” to “descriptionText” for clarity
WithinFeedbackDescriptionView, the propertytitleTextis used to store the description. Renaming it would prevent confusion in the future.-struct FeedbackDescriptionView: View { - @Binding var titleText: String +struct FeedbackDescriptionView: View { + @Binding var descriptionText: String var focusField: FocusState<FeedbackViewModel.FocusedField?>.Binding var body: some View { - TextEditor(text: $titleText) + TextEditor(text: $descriptionText) ...
180-188: Consider validating or resizing oversized attachments
Large images or lengthy videos may cause performance and memory issues. Consider pre-checking file size or dimensions, and prompt users or compress the media before upload.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift(1 hunks)BaseStyle/BaseStyle/Extension/UIImage+Extension.swift(1 hunks)Data/Data/Model/Feedback.swift(1 hunks)Data/Data/Store/FeedbackStore.swift(1 hunks)Splito/Localization/Localizable.xcstrings(8 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)Splito/UI/Home/Groups/Create Group/CreateGroupView.swift(1 hunks)functions/src/feedback/feedback_service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Splito/UI/Home/Groups/Create Group/CreateGroupView.swift
- Data/Data/Store/FeedbackStore.swift
- Data/Data/Model/Feedback.swift
- BaseStyle/BaseStyle/Extension/UIImage+Extension.swift
- Splito/Localization/Localizable.xcstrings
- functions/src/feedback/feedback_service.ts
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift
[Warning] 32-32: Lines should not have trailing whitespace
(trailing_whitespace)
[Warning] 34-34: Lines should not have trailing whitespace
(trailing_whitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
BaseStyle/BaseStyle/CustomUI/MultiMediaSelectionPickerView.swift (2)
37-39: Double-check the coordinator’s ownership.When using
UIViewControllerRepresentable, verify that theMultiMediaSelectionPickerCoordinatoris not inadvertently retained. SwiftUI usually handles this correctly, but watch for memory leaks if additional strong references exist.
62-72: 🛠️ Refactor suggestionPotential large image loading overhead.
Images are loaded synchronously (
imageRequestOptions.isSynchronous = true). This may block the main thread if images are large. Consider using asynchronous loading or downscaling for performance.- imageRequestOptions.isSynchronous = true + imageRequestOptions.isSynchronous = falseLikely invalid or redundant comment.
Splito/UI/Home/Account/Feedback/FeedbackView.swift (2)
25-27: Title validation logic looks great
The validation and error messaging for the title field is well-structured and user-friendly.
67-73: Wrap the dismissal in a closure to avoid accidental immediate invocation
This appears to be the same issue raised in a past review comment, ensuring the function is invoked only after the user finishes picking.-sheet(isPresented: $viewModel.showMediaPicker) { - MultiMediaSelectionPickerView( - isPresented: $viewModel.showMediaPicker, - attachmentLimit: abs(viewModel.attachmentsUrls.count - viewModel.MAX_ATTACHMENTS), - onDismiss: viewModel.onMediaPickerSheetDismiss(attachments:) - ) +sheet(isPresented: $viewModel.showMediaPicker) { + MultiMediaSelectionPickerView( + isPresented: $viewModel.showMediaPicker, + attachmentLimit: abs(viewModel.attachmentsUrls.count - viewModel.MAX_ATTACHMENTS), + onDismiss: { pickedAttachments in + viewModel.onMediaPickerSheetDismiss(attachments: pickedAttachments) + } + ) }
|
Let's show success toast on feedback submission. |
| .padding([.horizontal, .bottom], 16) | ||
| } | ||
| } | ||
| .frame(maxWidth: isIpad ? 600 : nil, alignment: .center) |
There was a problem hiding this comment.
How this view will appear in iPad?
| /* eslint-disable */ | ||
|
|
||
| import {SendEmailCommand, SESClient} from "@aws-sdk/client-ses"; | ||
| import { config } from 'firebase-functions'; |
There was a problem hiding this comment.
This config is deprecated or something, let's have a look and add needed changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
functions/src/index.ts (3)
4-9: Consider organizing imports by typeGroup related imports together for better maintainability:
- External dependencies (firebase-admin, firebase-functions)
- Internal services (users, notifications, feedback, mail)
import * as admin from 'firebase-admin'; import * as logger from 'firebase-functions/logger'; +import { onDocumentCreated } from "firebase-functions/v2/firestore"; import { onGroupWrite } from './users/users_service'; import { onActivityCreate } from './notifications/notifications_service'; import { FeedbackService } from "./feedback/feedback_service"; -import { onDocumentCreated } from "firebase-functions/v2/firestore"; import { MailService } from './mail/mail_service';
24-26: Consider lazy initialization of servicesGlobal service instantiation can increase cold start times. Consider moving service initialization inside the function handler.
-const mailService = new MailService(); -const REGION = "asia-south1"; -const feedbackService = new FeedbackService(mailService); +const REGION = process.env.REGION || "asia-south1"; + +function getServices() { + const mailService = new MailService(); + return { + feedbackService: new FeedbackService(mailService) + }; +}
28-30: Consider migrating all functions to Firebase v2The codebase mixes v1 (
exports.onGroupWrite) and v2 (export const feedbackCreateObserver) function exports. Consider migrating all functions to v2 for consistency and to leverage newer features.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Data/Data.xcodeproj/project.pbxproj(9 hunks)Data/Data/Repository/FeedbackRepository.swift(1 hunks)Splito/Localization/Localizable.xcstrings(7 hunks)Splito/UI/Home/Account/Feedback/FeedbackView.swift(1 hunks)Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift(1 hunks)functions/src/feedback/feedback_service.ts(1 hunks)functions/src/index.ts(2 hunks)functions/src/mail/mail_service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- functions/src/feedback/feedback_service.ts
- Splito/Localization/Localizable.xcstrings
- functions/src/mail/mail_service.ts
- Data/Data/Repository/FeedbackRepository.swift
- Splito/UI/Home/Account/Feedback/FeedbackView.swift
- Splito/UI/Home/Account/Feedback/FeedbackViewModel.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
Data/Data.xcodeproj/project.pbxproj (2)
14-17: Well-organized file structure following the established architecture pattern.The new feedback-related files are properly organized following the existing Model-Store-Repository pattern, with clear separation of concerns:
- Model:
Feedback.swift- Store:
FeedbackStore.swiftfor data persistence- Repository:
FeedbackRepository.swiftfor business logic- Utils:
DeviceInfo.swiftfor supporting functionalityAlso applies to: 78-81, 191-191, 205-205, 215-215, 262-262
191-192: Consistent architectural approach.The feedback implementation correctly follows the established architectural patterns in the project:
- Clear separation of concerns with Model, Store, and Repository layers
- Proper grouping in the project structure
- Consistent file naming conventions
Also applies to: 205-206, 215-216, 262-263
| @@ -533,17 +545,20 @@ | |||
| 21CF56512D1E804000B47A6D /* DeepLinkManager.swift in Sources */, | |||
| D865F8AE2BD7CB0B0084BD36 /* Array+Extension.swift in Sources */, | |||
| 21559CB02CBD2B400039F127 /* ActivityLogRepository.swift in Sources */, | |||
| 2163D3B22D265CF9004B4F20 /* DeviceInfo.swift in Sources */, | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for feedback functionality confirmed
The codebase contains implementation files but lacks corresponding test coverage in Data/DataTests/ for:
Data/Data/Model/Feedback.swift: Model validationData/Data/Store/FeedbackStore.swift: Store CRUD operationsData/Data/Repository/FeedbackRepository.swift: Repository business logic
🔗 Analysis chain
Missing test coverage for new feedback functionality.
While the files are properly added to the build phases, there are no corresponding test files added to the test target. Consider adding unit tests for:
- Feedback model validation
- FeedbackStore CRUD operations
- FeedbackRepository business logic
Run this script to check for existing test files:
Also applies to: 553-553, 561-561, 573-573
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to feedback functionality
fd -e swift -p ".*Test.*Feedback.*" "DataTests"
Length of output: 144
Script:
#!/bin/bash
# Find test directories and Swift test files
echo "=== Searching for test directories ==="
fd -t d -p ".*[tT]est.*"
echo -e "\n=== Searching for Swift test files ==="
fd -e swift -p ".*[tT]est.*"
echo -e "\n=== Searching for feedback-related code ==="
rg -l -i "feedback" -g "*.swift"
Length of output: 1151
| export const feedbackCreateObserver = onDocumentCreated( | ||
| { region: REGION, document: "feedbacks/{feedbackId}" }, | ||
| async (event) => { | ||
| const snapshot = event.data; | ||
| if (!snapshot) { | ||
| logger.error("No data associated with the event"); | ||
| return; | ||
| } | ||
| const data = snapshot.data(); | ||
|
|
||
| try { | ||
| await feedbackService.onFeedbackCreated(data); | ||
| } catch (error) { | ||
| logger.error('Error handling feedback:', error); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and error handling
The feedback observer function could benefit from:
- Type definition for feedback data
- More specific error handling
- Validation of required fields
+interface FeedbackData {
+ // Add required fields based on your feedback structure
+ userId: string;
+ message: string;
+ // ... other fields
+}
export const feedbackCreateObserver = onDocumentCreated(
{ region: REGION, document: "feedbacks/{feedbackId}" },
async (event) => {
const snapshot = event.data;
if (!snapshot) {
logger.error("No data associated with the event");
return;
}
- const data = snapshot.data();
+ const data = snapshot.data() as FeedbackData;
+
+ // Validate required fields
+ if (!data.userId || !data.message) {
+ logger.error('Missing required fields in feedback data');
+ return;
+ }
try {
await feedbackService.onFeedbackCreated(data);
} catch (error) {
- logger.error('Error handling feedback:', error);
+ if (error instanceof Error) {
+ logger.error('Error handling feedback:', error.message, error.stack);
+ } else {
+ logger.error('Unknown error handling feedback:', error);
+ }
+ throw error; // Re-throw to mark function as failed
}
}
);Committable suggestion skipped: line range outside the PR's diff.
| export const feedbackCreateObserver = onDocumentCreated( | ||
| { region: REGION, document: "feedbacks/{feedbackId}" }, | ||
| async (event) => { |
There was a problem hiding this comment.
💡 Codebase verification
Missing Firestore security rules for feedbacks collection
The 'feedbacks' collection lacks explicit security rules in firestore.rules, which could lead to unauthorized access. Please add specific rules:
- Add a rule block for
/feedbacks/{feedbackId} - Define clear read/write permissions
- Consider using the existing
isAuthorized()function
Example structure:
match /feedbacks/{feedbackId} {
allow create: if isAuthorized();
allow read: if isAuthorized();
}🔗 Analysis chain
Verify Firestore security rules for feedback collection
Ensure that proper security rules are in place to prevent unauthorized feedback submissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Firestore rules exist for the feedback collection
rg -A 10 "match\s*/feedbacks/\{.*\}\s*{" firestore.rules
Length of output: 180
Script:
#!/bin/bash
# Check if firestore.rules exists
fd "firestore.rules$"
# Search for feedback-related rules with a simpler pattern
rg -i "feedback" firestore.rules || echo "No matches found"
# Broader search for collection rules
rg "match /\w+" firestore.rules || echo "No matches found"
Length of output: 450
Script:
#!/bin/bash
# Check root-level security rules
rg -A 5 "match /databases/\{database\}/documents" firestore.rules
Length of output: 188
cp-nirali-s
force-pushed
the
add-send-feedback-option
branch
4 times, most recently
from
d4044df to
7159ca3
Compare
Simulator.Screen.Recording.-.iPhone.16.-.2025-01-06.at.19.21.26.1.mp4
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores
These release notes provide a concise overview of the significant changes in the application, focusing on user-facing improvements and new functionalities.