Conversation
…ignment and preliminary share functionality.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProject/scheme version bumps and test build setting removals; model JSON CodingKeys changed from snake_case to camelCase for price fields; multiple SwiftUI view refactors and view-model ownership changes to shared singletons; onboarding/user-creation made async; new notification pub/sub for new listings and optimistic local save syncing. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProductDetailsView
participant ProductDetailsVM
participant HomeViewModel
participant Network
participant NotificationService
User->>ProductDetailsView: Tap Save button
ProductDetailsView->>ProductDetailsVM: updateItemSaved() (async)
ProductDetailsVM->>Network: send save/unsave request
Network-->>ProductDetailsVM: response
alt success
ProductDetailsVM-->>ProductDetailsView: update isSaved
ProductDetailsView->>HomeViewModel: toggleLocalSaveStatus(post, isSaving) (async)
HomeViewModel->>Network: getSavedPosts(forceRefresh: true)
Network-->>HomeViewModel: saved posts
HomeViewModel-->>ProductDetailsView: confirm local state
ProductDetailsView->>NotificationService: check authorization / request if needed
alt authorized
ProductDetailsView->>NotificationService: schedule/display notification
NotificationService-->>User: show confirmation
else not authorized
NotificationService-->>ProductDetailsView: prompt result
end
else failure
ProductDetailsVM-->>ProductDetailsView: log/error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resell/Views/ProductDetails/ProductDetailsView.swift (1)
381-397:⚠️ Potential issue | 🟡 MinorThread-safety concern with
@AppStorageinside function.Declaring
@AppStorageinsiderequestNotificationAuthorization()(line 382) is unconventional. The property wrapper is typically declared at the view level. While this may work, the closure captures it by reference after the function returns, which could lead to unexpected behavior.Consider using the existing view-level
isNotificationAuthorizedproperty (line 399) instead:Proposed fix
func requestNotificationAuthorization() { - `@AppStorage`("isNotificationAuthorized") var isNotificationAuthorized = false UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .sound, .badge]) { (granted, error) in if let error = error { print("Error sending notification: \(error.localizedDescription)") return } if granted { - isNotificationAuthorized = true + DispatchQueue.main.async { + self.isNotificationAuthorized = true + } print("Notification permission granted.") } else { - isNotificationAuthorized = false + DispatchQueue.main.async { + self.isNotificationAuthorized = false + } print("Notification permission denied.") } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/ProductDetails/ProductDetailsView.swift` around lines 381 - 397, The local declaration of `@AppStorage` inside requestNotificationAuthorization() is unsafe; remove the in-function "@AppStorage(\"isNotificationAuthorized\") var isNotificationAuthorized" and instead update the view-level isNotificationAuthorized property already declared on the view (isNotificationAuthorized) from within the UNUserNotificationCenter completion handler; ensure you marshal UI/state changes to the main thread (e.g., wrap assignments to isNotificationAuthorized inside DispatchQueue.main.async) and keep the existing error handling and prints in the same closure.
🧹 Nitpick comments (3)
Resell/Views/ProductDetails/ProductDetailsView.swift (1)
401-440: Reduce code duplication in saveButton and remove debug print.The
saveButtonhas duplicated UI code between theifandelsebranches—only the action differs. Additionally, line 425 contains a debugprint("Test1")statement that should be removed.♻️ Proposed refactor
private var saveButton: some View { - if isNotificationAuthorized { - Button { - viewModel.isSaved.toggle() - viewModel.updateItemSaved() + Button { + viewModel.isSaved.toggle() + viewModel.updateItemSaved() + if isNotificationAuthorized { sendNotification() - } label: { - ZStack { - Circle() - .frame(width: 72, height: 72) - .foregroundStyle(Constants.Colors.white) - .opacity(viewModel.isSaved ? 1.0 : 0.9) - .shadow(radius: 2) - - Image(viewModel.isSaved ? "saved.fill" : "saved") - .resizable() - .frame(width: 21, height: 27) - } + } else { + requestNotificationAuthorization() } - } else { - Button { - viewModel.isSaved.toggle() - viewModel.updateItemSaved() - requestNotificationAuthorization() - print("Test1") - } label: { - ZStack { - Circle() - .frame(width: 72, height: 72) - .foregroundStyle(Constants.Colors.white) - .opacity(viewModel.isSaved ? 1.0 : 0.9) - .shadow(radius: 2) - - Image(viewModel.isSaved ? "saved.fill" : "saved") - .resizable() - .frame(width: 21, height: 27) - } + } label: { + ZStack { + Circle() + .frame(width: 72, height: 72) + .foregroundStyle(Constants.Colors.white) + .opacity(viewModel.isSaved ? 1.0 : 0.9) + .shadow(radius: 2) + + Image(viewModel.isSaved ? "saved.fill" : "saved") + .resizable() + .frame(width: 21, height: 27) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/ProductDetails/ProductDetailsView.swift` around lines 401 - 440, The saveButton currently duplicates the button label UI in both branches and contains a debug print; refactor by creating one Button whose label is the shared ZStack (reuse the Circle and Image UI from both branches) and make the action choose between calling sendNotification() or requestNotificationAuthorization() based on isNotificationAuthorized while still toggling viewModel.isSaved and calling viewModel.updateItemSaved(); remove the debug print("Test1") and ensure the unique symbols referenced are saveButton, viewModel.isSaved.toggle(), viewModel.updateItemSaved(), sendNotification(), and requestNotificationAuthorization().Resell/Models/Transaction.swift (1)
130-131: Redundant explicit CodingKey mappings.These CodingKey mappings are redundant since
originalPriceandalteredPriceproperty names already match their JSON key values. They could be simplified by adding them to line 129:private enum CodingKeys: String, CodingKey { - case id, title, images, description, condition, sold - case originalPrice = "originalPrice" - case alteredPrice = "alteredPrice" + case id, title, images, description, condition, sold, originalPrice, alteredPrice }This is a minor stylistic preference—the current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Models/Transaction.swift` around lines 130 - 131, The CodingKeys enum in Transaction redundantly maps originalPrice and alteredPrice to identical JSON keys; simplify by removing their explicit case lines and instead include originalPrice and alteredPrice in the existing CodingKeys declaration (or rely on synthesized keys) so Transaction.CodingKeys no longer contains redundant mappings for originalPrice and alteredPrice.Resell/Models/Post.swift (1)
104-104: Lines 28-29 contain redundant CodingKey mappings that can be simplified.In the
Poststruct, lines 28-29 explicitly maporiginalPriceandalteredPriceto JSON keys with the same names:case originalPrice = "originalPrice" case alteredPrice = "alteredPrice"Since the Swift property names already match the JSON key names, these explicit mappings are redundant. Simplify to:
case originalPrice, alteredPriceAdditionally, the
PostBodystruct (line 95) uses a snake_case property name (original_price) that maps to camelCase JSON ("originalPrice"). While this works, consider aligning property naming conventions with the JSON format for consistency across the models.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Models/Post.swift` at line 104, In the Post struct update the CodingKeys to remove redundant explicit mappings by collapsing `case originalPrice = "originalPrice"` and `case alteredPrice = "alteredPrice"` into a single line `case originalPrice, alteredPrice` within the `CodingKeys` enum; also review the `PostBody` struct and rename the snake_case property `original_price` to `originalPrice` (and adjust its CodingKeys or remove the mapping if names now match) so property naming aligns with the JSON camelCase convention and avoids unnecessary key mappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resell/Views/ProductDetails/ProductDetailsView.swift`:
- Around line 69-70: The MARK comment in ProductDetailsView.swift is using an
invalid format; update the line "// MARK - Need to get a real shareable url (put
in post or viewmodel)" to a valid SwiftLint pattern such as "// MARK: - Need to
get a real shareable url (put in post or viewmodel)" or replace it with a task
comment like "// TODO: Need to get a real shareable url (put in post or
viewmodel)"; ensure the nearby .share(url: "TEST", itemName:
viewModel.item?.title ?? "cool item") comment stays intact.
---
Outside diff comments:
In `@Resell/Views/ProductDetails/ProductDetailsView.swift`:
- Around line 381-397: The local declaration of `@AppStorage` inside
requestNotificationAuthorization() is unsafe; remove the in-function
"@AppStorage(\"isNotificationAuthorized\") var isNotificationAuthorized" and
instead update the view-level isNotificationAuthorized property already declared
on the view (isNotificationAuthorized) from within the UNUserNotificationCenter
completion handler; ensure you marshal UI/state changes to the main thread
(e.g., wrap assignments to isNotificationAuthorized inside
DispatchQueue.main.async) and keep the existing error handling and prints in the
same closure.
---
Nitpick comments:
In `@Resell/Models/Post.swift`:
- Line 104: In the Post struct update the CodingKeys to remove redundant
explicit mappings by collapsing `case originalPrice = "originalPrice"` and `case
alteredPrice = "alteredPrice"` into a single line `case originalPrice,
alteredPrice` within the `CodingKeys` enum; also review the `PostBody` struct
and rename the snake_case property `original_price` to `originalPrice` (and
adjust its CodingKeys or remove the mapping if names now match) so property
naming aligns with the JSON camelCase convention and avoids unnecessary key
mappings.
In `@Resell/Models/Transaction.swift`:
- Around line 130-131: The CodingKeys enum in Transaction redundantly maps
originalPrice and alteredPrice to identical JSON keys; simplify by removing
their explicit case lines and instead include originalPrice and alteredPrice in
the existing CodingKeys declaration (or rely on synthesized keys) so
Transaction.CodingKeys no longer contains redundant mappings for originalPrice
and alteredPrice.
In `@Resell/Views/ProductDetails/ProductDetailsView.swift`:
- Around line 401-440: The saveButton currently duplicates the button label UI
in both branches and contains a debug print; refactor by creating one Button
whose label is the shared ZStack (reuse the Circle and Image UI from both
branches) and make the action choose between calling sendNotification() or
requestNotificationAuthorization() based on isNotificationAuthorized while still
toggling viewModel.isSaved and calling viewModel.updateItemSaved(); remove the
debug print("Test1") and ensure the unique symbols referenced are saveButton,
viewModel.isSaved.toggle(), viewModel.updateItemSaved(), sendNotification(), and
requestNotificationAuthorization().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b35089-82ba-421a-8836-c80cf89b7bef
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (8)
Resell.xcodeproj/project.pbxprojResell.xcodeproj/xcshareddata/xcschemes/Resell.xcschemeResell/Models/Listing.swiftResell/Models/Post.swiftResell/Models/Transaction.swiftResell/Views/Components/DraggableSheetView.swiftResell/Views/Components/OptionsMenuView.swiftResell/Views/ProductDetails/ProductDetailsView.swift
| // MARK - Need to get a real shareable url (put in post or viewmodel) | ||
| // .share(url: "TEST", itemName: viewModel.item?.title ?? "cool item"), |
There was a problem hiding this comment.
Fix MARK comment format.
The MARK comment on line 69 uses an invalid format. Per SwiftLint, it should follow the pattern // MARK: ... or // MARK: - ....
Proposed fix
- // MARK - Need to get a real shareable url (put in post or viewmodel)
+ // MARK: - TODO: Need to get a real shareable url (put in post or viewmodel)Or simply use a regular // TODO: comment since this is a task reminder rather than a section marker.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // MARK - Need to get a real shareable url (put in post or viewmodel) | |
| // .share(url: "TEST", itemName: viewModel.item?.title ?? "cool item"), | |
| // MARK: - TODO: Need to get a real shareable url (put in post or viewmodel) | |
| // .share(url: "TEST", itemName: viewModel.item?.title ?? "cool item"), |
🧰 Tools
🪛 SwiftLint (0.63.2)
[Warning] 69-69: MARK comment should be in valid format. e.g. '// MARK: ...' or '// MARK: - ...'
(mark)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resell/Views/ProductDetails/ProductDetailsView.swift` around lines 69 - 70,
The MARK comment in ProductDetailsView.swift is using an invalid format; update
the line "// MARK - Need to get a real shareable url (put in post or viewmodel)"
to a valid SwiftLint pattern such as "// MARK: - Need to get a real shareable
url (put in post or viewmodel)" or replace it with a task comment like "// TODO:
Need to get a real shareable url (put in post or viewmodel)"; ensure the nearby
.share(url: "TEST", itemName: viewModel.item?.title ?? "cool item") comment
stays intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resell/Views/Home/ForYouView.swift`:
- Around line 56-57: The empty-state container in ForYouView currently uses a
fixed .frame(height: 110) which can clip at larger Dynamic Type sizes; remove
the fixed height and make the container flexible (for example replace the fixed
height with a .frame(minHeight: 110) or let the view size itself by removing the
frame and using padding/flexible layout), ensure the empty-state view (the view
with the modifier chain containing .frame(height: 110) and .padding(.horizontal,
24)) can grow for accessibility by using minHeight or intrinsic sizing so text
isn't clipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c34137b-6af2-4afa-9d8c-33d14016a1cc
📒 Files selected for processing (1)
Resell/Views/Home/ForYouView.swift
| .frame(height: 110) | ||
| .padding(.horizontal, 24) |
There was a problem hiding this comment.
Avoid fixed-height empty-state container for Dynamic Type.
frame(height: 110) can clip text at larger accessibility sizes. Prefer a flexible height so the empty state remains readable.
Suggested adjustment
- .frame(height: 110)
+ .frame(minHeight: 110)
+ .frame(maxWidth: .infinity, alignment: .leading)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .frame(height: 110) | |
| .padding(.horizontal, 24) | |
| .frame(minHeight: 110) | |
| .frame(maxWidth: .infinity, alignment: .leading) | |
| .padding(.horizontal, 24) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resell/Views/Home/ForYouView.swift` around lines 56 - 57, The empty-state
container in ForYouView currently uses a fixed .frame(height: 110) which can
clip at larger Dynamic Type sizes; remove the fixed height and make the
container flexible (for example replace the fixed height with a
.frame(minHeight: 110) or let the view size itself by removing the frame and
using padding/flexible layout), ensure the empty-state view (the view with the
modifier chain containing .frame(height: 110) and .padding(.horizontal, 24)) can
grow for accessibility by using minHeight or intrinsic sizing so text isn't
clipped.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Resell/Views/Onboarding/SetupProfileView.swift (1)
39-42: Remove commented-out code.The commented-out
viewModel.createNewUser()call is dead code since user creation has been moved toVenmoView. Remove it to keep the codebase clean and avoid confusion for future readers.♻️ Proposed cleanup
PurpleButton(isActive: viewModel.checkInputIsValid(), text: "Next", horizontalPadding: 80) { -// viewModel.createNewUser() router.push(.venmo) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/Onboarding/SetupProfileView.swift` around lines 39 - 42, Remove the dead commented-out call to viewModel.createNewUser() inside the PurpleButton action in SetupProfileView; the button currently calls router.push(.venmo) and user creation was moved to VenmoView, so delete the commented line to clean up the code and avoid confusion while leaving the PurpleButton(...){ router.push(.venmo) } logic intact.Resell/Views/Onboarding/VenmoView.swift (2)
85-92: Remove commented-out observer code.The
.onChange(of: viewModel.isLoading)observer is no longer needed since the navigation logic is now driven by thecreateNewUser()result. Remove this commented-out block to keep the code clean.♻️ Proposed cleanup
} -// .onChange(of: viewModel.isLoading) { newValue in -// if !newValue { -// withAnimation { -// router.popToRoot() -// userDidLogin = true -// } -// } -// } .endEditingOnTap()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/Onboarding/VenmoView.swift` around lines 85 - 92, Remove the dead/commented observer block that watches viewModel.isLoading; delete the commented .onChange(of: viewModel.isLoading) { ... } block (which contains router.popToRoot() and userDidLogin assignment) because navigation is now driven by the createNewUser() result—leave createNewUser(), router.popToRoot(), viewModel, and userDidLogin intact but remove the commented code to clean up the file.
34-64: Duplicated async flow logic between Continue and Skip handlers.Both button handlers contain identical Task/success handling logic. Consider extracting the common flow to reduce duplication and simplify future maintenance.
♻️ Proposed extraction
Add a helper method and simplify both handlers:
private func performUserCreation() { viewModel.isLoading = true Task { let success = await viewModel.createNewUser() viewModel.isLoading = false if success { withAnimation { router.popToRoot() userDidLogin = true } } } }Then the button handlers become:
PurpleButton(isLoading: viewModel.isLoading, isActive: !viewModel.venmoHandle.cleaned().isEmpty, text: "Continue") { - viewModel.isLoading = true - - Task { - let success = await viewModel.createNewUser() - viewModel.isLoading = false - - if success { - withAnimation { - router.popToRoot() - userDidLogin = true - } - } - } + performUserCreation() } Button(action: { viewModel.venmoHandle = "" - viewModel.isLoading = true - - Task { - let success = await viewModel.createNewUser() - viewModel.isLoading = false - - if success { - withAnimation { - router.popToRoot() - userDidLogin = true - } - } - } + performUserCreation() }, label: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/Onboarding/VenmoView.swift` around lines 34 - 64, Extract the duplicated async flow from both the PurpleButton and the Skip Button into a single helper on the VenmoView (e.g., performUserCreation) that sets viewModel.isLoading = true, runs the Task that awaits viewModel.createNewUser(), sets isLoading = false, and on success runs withAnimation { router.popToRoot(); userDidLogin = true }; then call this helper from the PurpleButton action and from the Button(action:) after resetting viewModel.venmoHandle so both handlers reuse the same logic.Resell/ViewModels/SetupProfileViewModel.swift (1)
38-41: UIImage reference comparison is fragile.Comparing
UIImageinstances with!=relies on UIKit's image caching behavior to return the same instance forUIImage(named:). This is an implementation detail and may not hold in all scenarios (e.g., memory pressure purging the cache).Consider tracking image selection state explicitly with a boolean flag, or comparing against
nilonly sinceupdateUserProfilealready handles the selection flow.♻️ Alternative approach using explicit state
+ `@Published` var hasSelectedCustomImage: Bool = false `@Published` var selectedImage: UIImage? = nilThen in
checkInputIsValid():func checkInputIsValid() -> Bool { - let hasImage = selectedImage != nil && selectedImage != UIImage(named: "emptyProfile") - - return !(username.cleaned().isEmpty || bio.cleaned().isEmpty) && didAgreeWithEULA && hasImage + return !(username.cleaned().isEmpty || bio.cleaned().isEmpty) && didAgreeWithEULA && hasSelectedCustomImage }And set
hasSelectedCustomImage = trueinupdateUserProfilewhen a valid image is loaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/ViewModels/SetupProfileViewModel.swift` around lines 38 - 41, The current checkInputIsValid() compares selectedImage to UIImage(named: "emptyProfile"), which is fragile; instead add an explicit Bool property (e.g., hasSelectedCustomImage) to track whether the user picked a custom image, update that flag to true inside updateUserProfile (or wherever a custom image is successfully loaded) and set it false for the default/no-image state, then change checkInputIsValid() to use hasSelectedCustomImage (and only compare selectedImage to nil if needed) rather than relying on UIImage(named:) identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resell/ViewModels/SetupProfileViewModel.swift`:
- Around line 83-85: The method currently returns false silently if
selectedImage?.resizedToMaxDimension(256).toBase64() is nil; update the flow in
the function that performs image encoding (reference the
selectedImage?.resizedToMaxDimension(256).toBase64() call) to detect a nil
result, call presentError(...) with a user-facing message about image
encoding/processing failure, and then return false—ensuring presentError() is
invoked before exiting so the user gets feedback when image encoding fails.
---
Nitpick comments:
In `@Resell/ViewModels/SetupProfileViewModel.swift`:
- Around line 38-41: The current checkInputIsValid() compares selectedImage to
UIImage(named: "emptyProfile"), which is fragile; instead add an explicit Bool
property (e.g., hasSelectedCustomImage) to track whether the user picked a
custom image, update that flag to true inside updateUserProfile (or wherever a
custom image is successfully loaded) and set it false for the default/no-image
state, then change checkInputIsValid() to use hasSelectedCustomImage (and only
compare selectedImage to nil if needed) rather than relying on UIImage(named:)
identity.
In `@Resell/Views/Onboarding/SetupProfileView.swift`:
- Around line 39-42: Remove the dead commented-out call to
viewModel.createNewUser() inside the PurpleButton action in SetupProfileView;
the button currently calls router.push(.venmo) and user creation was moved to
VenmoView, so delete the commented line to clean up the code and avoid confusion
while leaving the PurpleButton(...){ router.push(.venmo) } logic intact.
In `@Resell/Views/Onboarding/VenmoView.swift`:
- Around line 85-92: Remove the dead/commented observer block that watches
viewModel.isLoading; delete the commented .onChange(of: viewModel.isLoading) {
... } block (which contains router.popToRoot() and userDidLogin assignment)
because navigation is now driven by the createNewUser() result—leave
createNewUser(), router.popToRoot(), viewModel, and userDidLogin intact but
remove the commented code to clean up the file.
- Around line 34-64: Extract the duplicated async flow from both the
PurpleButton and the Skip Button into a single helper on the VenmoView (e.g.,
performUserCreation) that sets viewModel.isLoading = true, runs the Task that
awaits viewModel.createNewUser(), sets isLoading = false, and on success runs
withAnimation { router.popToRoot(); userDidLogin = true }; then call this helper
from the PurpleButton action and from the Button(action:) after resetting
viewModel.venmoHandle so both handlers reuse the same logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06963465-15f6-4564-a120-1437231a519b
📒 Files selected for processing (3)
Resell/ViewModels/SetupProfileViewModel.swiftResell/Views/Onboarding/SetupProfileView.swiftResell/Views/Onboarding/VenmoView.swift
… with db, and fixed homeViewModel object passing within the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resell/Views/Home/FilterView.swift (1)
248-254:⚠️ Potential issue | 🟡 MinorSheet dismisses before filters finish applying.
The
isPresented = falseon line 253 executes immediately after spawning theTask, not afterapplyFilterscompletes. This can cause the sheet to dismiss while the network request is still in flight, potentially leading to a jarring UX if the filter results appear later.Proposed fix
Button{ Task { try await filtersVM.applyFilters(homeViewModel: homeViewModel) + isPresented = false } - // MARK: This should wait for the above request to complete - isPresented = false } label: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/Home/FilterView.swift` around lines 248 - 254, The sheet is dismissed immediately because isPresented = false runs synchronously after spawning Task; move the dismissal into the Task after the await so it runs only when filtersVM.applyFilters completes. Concretely, inside the Button action wrap the call in Task { do { try await filtersVM.applyFilters(homeViewModel: homeViewModel); await MainActor.run { isPresented = false } } catch { await MainActor.run { /* handle error, optionally keep sheet open or show error */ } } } or simply place isPresented = false directly after the try await inside the Task so dismissal occurs only after applyFilters returns; reference filtersVM.applyFilters, Task and isPresented.
🧹 Nitpick comments (3)
Resell/ViewModels/HomeViewModel.swift (1)
146-153: Minor formatting: missing space before{.- func getSavedPosts(forceRefresh: Bool = false) async { - if !forceRefresh{ + func getSavedPosts(forceRefresh: Bool = false) async { + if !forceRefresh {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/ViewModels/HomeViewModel.swift` around lines 146 - 153, In getSavedPosts(forceRefresh: Bool = false) async the opening brace is missing a space; update the function declaration to include a space before the `{` (i.e., change "async {" to "async {") so formatting is consistent—verify near symbols lastSavedFetchTime, cacheValidityDuration, and savedItems to ensure no other stray spacing issues.Resell/Views/Home/SavedView.swift (1)
13-14: Remove commented-out code.The commented
@EnvironmentObjectline should be removed to keep the codebase clean.Proposed fix
`@EnvironmentObject` var router: Router -// `@EnvironmentObject` private var viewModel: HomeViewModel `@ObservedObject` private var viewModel = HomeViewModel.shared🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/Home/SavedView.swift` around lines 13 - 14, Remove the commented-out declaration "@EnvironmentObject private var viewModel: HomeViewModel" to keep the file clean; keep the active "@ObservedObject private var viewModel = HomeViewModel.shared" as-is, and ensure no leftover commented references to EnvironmentObject remain in the file.Resell/Views/ProductDetails/ProductDetailsView.swift (1)
403-451: Reduce code duplication in save button and add rollback on failure.The two branches of
saveButtonare nearly identical, differing only in the notification handling. Consider extracting the common logic. Additionally, ifupdateItemSaved()fails,viewModel.isSavedremains toggled but the server state is unchanged, causing UI desynchronization.Proposed refactor
private var saveButton: some View { - if isNotificationAuthorized { - Button { - viewModel.isSaved.toggle() - - Task { - await viewModel.updateItemSaved() - await homeViewModel.toggleLocalSaveStatus(for: post, isSaving: viewModel.isSaved) - } - - sendNotification() - } label: { - ZStack { - Circle() - .frame(width: 72, height: 72) - .foregroundStyle(Constants.Colors.white) - .opacity(viewModel.isSaved ? 1.0 : 0.9) - .shadow(radius: 2) - - Image(viewModel.isSaved ? "saved.fill" : "saved") - .resizable() - .frame(width: 21, height: 27) - } - } - } else { - Button { - viewModel.isSaved.toggle() - - Task { + Button { + let previousState = viewModel.isSaved + viewModel.isSaved.toggle() + + Task { + do { await viewModel.updateItemSaved() await homeViewModel.toggleLocalSaveStatus(for: post, isSaving: viewModel.isSaved) + } catch { + // Rollback on failure + viewModel.isSaved = previousState } - - requestNotificationAuthorization() - } label: { - ZStack { - Circle() - .frame(width: 72, height: 72) - .foregroundStyle(Constants.Colors.white) - .opacity(viewModel.isSaved ? 1.0 : 0.9) - .shadow(radius: 2) - - Image(viewModel.isSaved ? "saved.fill" : "saved") - .resizable() - .frame(width: 21, height: 27) - } } + + if isNotificationAuthorized { + sendNotification() + } else { + requestNotificationAuthorization() + } + } label: { + ZStack { + Circle() + .frame(width: 72, height: 72) + .foregroundStyle(Constants.Colors.white) + .opacity(viewModel.isSaved ? 1.0 : 0.9) + .shadow(radius: 2) + + Image(viewModel.isSaved ? "saved.fill" : "saved") + .resizable() + .frame(width: 21, height: 27) + } } }Note: The
updateItemSaved()method currently doesn't throw errors to the caller. Consider having it rethrow for proper error handling, or check success internally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/ProductDetails/ProductDetailsView.swift` around lines 403 - 451, The two branches of saveButton duplicate the toggle/update/send flow; extract the common save action into a single helper (e.g., a private func performSaveAction(notify: Bool) or closure) that toggles viewModel.isSaved, calls await viewModel.updateItemSaved() and await homeViewModel.toggleLocalSaveStatus(for: post, isSaving: viewModel.isSaved), then either sendNotification() or requestNotificationAuthorization() based on a notify flag; also add rollback logic: make updateItemSaved() surface failure (either rethrow or return Bool) so performSaveAction can detect failure and revert viewModel.isSaved and local state if the server update fails. Ensure UI button uses the single helper and only differs by the notify flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resell/ViewModels/HomeViewModel.swift`:
- Around line 168-188: The optimistic-update rollback is vulnerable to race
conditions in toggleLocalSaveStatus(for:isSaving:): capture of prevSavedItems
can overwrite newer changes if multiple calls run concurrently; fix by
serializing/guarding operations—e.g., add a monotonic operation counter or UUID
property (e.g., saveOperationID or currentSaveToken) on the ViewModel,
increment/generate it at the start of toggleLocalSaveStatus, capture the token
in the async call, and on error only restore prevSavedItems if the captured
token matches the current token (otherwise ignore rollback); alternatively (or
additionally) disable the save button or debounce it while an operation is in
flight so NetworkManager.shared.getSavedPosts results and lastSavedFetchTime are
only applied when the operation token is still current.
---
Outside diff comments:
In `@Resell/Views/Home/FilterView.swift`:
- Around line 248-254: The sheet is dismissed immediately because isPresented =
false runs synchronously after spawning Task; move the dismissal into the Task
after the await so it runs only when filtersVM.applyFilters completes.
Concretely, inside the Button action wrap the call in Task { do { try await
filtersVM.applyFilters(homeViewModel: homeViewModel); await MainActor.run {
isPresented = false } } catch { await MainActor.run { /* handle error,
optionally keep sheet open or show error */ } } } or simply place isPresented =
false directly after the try await inside the Task so dismissal occurs only
after applyFilters returns; reference filtersVM.applyFilters, Task and
isPresented.
---
Nitpick comments:
In `@Resell/ViewModels/HomeViewModel.swift`:
- Around line 146-153: In getSavedPosts(forceRefresh: Bool = false) async the
opening brace is missing a space; update the function declaration to include a
space before the `{` (i.e., change "async {" to "async {") so formatting is
consistent—verify near symbols lastSavedFetchTime, cacheValidityDuration, and
savedItems to ensure no other stray spacing issues.
In `@Resell/Views/Home/SavedView.swift`:
- Around line 13-14: Remove the commented-out declaration "@EnvironmentObject
private var viewModel: HomeViewModel" to keep the file clean; keep the active
"@ObservedObject private var viewModel = HomeViewModel.shared" as-is, and ensure
no leftover commented references to EnvironmentObject remain in the file.
In `@Resell/Views/ProductDetails/ProductDetailsView.swift`:
- Around line 403-451: The two branches of saveButton duplicate the
toggle/update/send flow; extract the common save action into a single helper
(e.g., a private func performSaveAction(notify: Bool) or closure) that toggles
viewModel.isSaved, calls await viewModel.updateItemSaved() and await
homeViewModel.toggleLocalSaveStatus(for: post, isSaving: viewModel.isSaved),
then either sendNotification() or requestNotificationAuthorization() based on a
notify flag; also add rollback logic: make updateItemSaved() surface failure
(either rethrow or return Bool) so performSaveAction can detect failure and
revert viewModel.isSaved and local state if the server update fails. Ensure UI
button uses the single helper and only differs by the notify flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5a62770-0aff-4543-8d14-76d673f6bd1b
📒 Files selected for processing (8)
Resell/ViewModels/HomeViewModel.swiftResell/ViewModels/ProductDetailsViewModel.swiftResell/Views/Home/DetailedFilterView.swiftResell/Views/Home/FilterView.swiftResell/Views/Home/ForYouView.swiftResell/Views/Home/HomeView.swiftResell/Views/Home/SavedView.swiftResell/Views/ProductDetails/ProductDetailsView.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Resell/Views/Home/ForYouView.swift
| func toggleLocalSaveStatus(for post: Post, isSaving: Bool) async { | ||
| let prevSavedItems = self.savedItems | ||
|
|
||
| if isSaving { | ||
| if !savedItems.contains(where: { $0.id == post.id }) { | ||
| savedItems.insert(post, at: 0) | ||
| } | ||
| } else { | ||
| savedItems.removeAll(where: { $0.id == post.id }) | ||
| } | ||
|
|
||
| do { | ||
| let postsResponse = try await NetworkManager.shared.getSavedPosts() | ||
| self.savedItems = Post.sortPostsByDate(postsResponse.posts) | ||
| self.lastSavedFetchTime = Date() | ||
| } catch { | ||
| print("Network failed, rolling back local state...") | ||
| self.savedItems = prevSavedItems | ||
| NetworkManager.shared.logger.error("Sync failed: \(error)") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any debounce or rate-limiting on save button interactions
rg -n -C3 'toggleLocalSaveStatus|isSaved.toggle' --type=swiftRepository: cuappdev/resell-ios
Length of output: 2565
🏁 Script executed:
# Check HomeViewModel class definition and `@MainActor` annotation
rg -n '@MainActor|class HomeViewModel' --type=swift Resell/ViewModels/HomeViewModel.swift -B2 -A2Repository: cuappdev/resell-ios
Length of output: 749
🏁 Script executed:
# Look for the full toggleLocalSaveStatus implementation and surrounding code
cat -n Resell/ViewModels/HomeViewModel.swift | sed -n '150,200p'Repository: cuappdev/resell-ios
Length of output: 1973
🏁 Script executed:
# Check if there's any button state management or tap prevention in ProductDetailsView
rg -n 'saveButton|\.disabled|isLoading|isSaving' --type=swift Resell/Views/ProductDetails/ProductDetailsView.swift -B2 -A2Repository: cuappdev/resell-ios
Length of output: 1374
Consider potential race condition in optimistic update rollback.
If toggleLocalSaveStatus is called multiple times in rapid succession (e.g., user double-taps the save button), each call captures prevSavedItems at the start. If the first call's network request fails and rolls back while a second call is in flight, the rollback could restore stale state, overwriting changes from the second call.
Although @MainActor serializes synchronous code, the await on line 180 yields control, allowing task interleaving. The save button has no .disabled() or loading state protection to prevent concurrent calls. Consider adding a debounce mechanism on the save button or using a serial task queue to ensure only one save operation proceeds at a time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resell/ViewModels/HomeViewModel.swift` around lines 168 - 188, The
optimistic-update rollback is vulnerable to race conditions in
toggleLocalSaveStatus(for:isSaving:): capture of prevSavedItems can overwrite
newer changes if multiple calls run concurrently; fix by serializing/guarding
operations—e.g., add a monotonic operation counter or UUID property (e.g.,
saveOperationID or currentSaveToken) on the ViewModel, increment/generate it at
the start of toggleLocalSaveStatus, capture the token in the async call, and on
error only restore prevSavedItems if the captured token matches the current
token (otherwise ignore rollback); alternatively (or additionally) disable the
save button or debounce it while an operation is in flight so
NetworkManager.shared.getSavedPosts results and lastSavedFetchTime are only
applied when the operation token is still current.
…e view model after new listing.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
Resell/ViewModels/SetupProfileViewModel.swift (1)
69-98:⚠️ Potential issue | 🟠 MajorStill returning silently when image encoding fails.
If
selectedImage?.resizedToMaxDimension(256).toBase64()returnsnil, the method falls through to Line 98 and returnsfalsewithout callingpresentError(...), so the onboarding flow just stops loading with no feedback.🐛 Suggested fix
- return false + presentError("Failed to process profile image. Please try again.") + return false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/ViewModels/SetupProfileViewModel.swift` around lines 69 - 98, The code silently returns false when selectedImage?.resizedToMaxDimension(256).toBase64() is nil; update the flow in the block around selectedImage/resizedToMaxDimension/toBase64 so that when imageBase64 is nil you call presentError(...) with a clear message (e.g., "Image processing failed. Please try a different image.") and then return false, rather than falling through silently—make the check explicit before creating ImageBody/uploadImage and ensure presentError is used for any early exit related to image encoding.
🧹 Nitpick comments (1)
Resell/Views/NewListing/NewListingImagesView.swift (1)
101-114: Remove the commented-out.actionSheetblock.Keeping the old implementation commented here adds noise and can confuse future edits now that
.confirmationDialogis the active path.Suggested cleanup
-// .actionSheet(isPresented: $viewModel.didShowActionSheet) { -// ActionSheet( -// title: Text("Select Image Source"), -// buttons: [ -// .default(Text("Photo Library")) { -// viewModel.didShowPhotosPicker = true -// }, -// .default(Text("Camera")) { -// viewModel.didShowCamera = true -// }, -// .cancel() -// ] -// ) -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resell/Views/NewListing/NewListingImagesView.swift` around lines 101 - 114, Remove the commented-out .actionSheet block to avoid dead code and confusion: delete the entire commented section that references .actionSheet and ActionSheet (the block toggling viewModel.didShowActionSheet, viewModel.didShowPhotosPicker, viewModel.didShowCamera), since the active UI uses .confirmationDialog instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resell/ViewModels/CurrentUserProfileManager.swift`:
- Around line 169-178: The notification observer in setupNotificationObservers
currently calls loadProfile(forceRefresh: true) unconditionally, allowing
overlapping fetches; change the observer closure to first check the view model's
isLoading flag (accessing it on the MainActor) and return early if a load is
already in progress, then only call loadProfile when not loading—this prevents
concurrent network requests and nondeterministic UI updates; reference
setupNotificationObservers, isLoading, and loadProfile (and consider
ProfileViewModel.loadCurrentUser callers) when implementing the guard.
In `@Resell/ViewModels/SetupProfileViewModel.swift`:
- Around line 99-104: The catch in SetupProfileViewModel.createNewUser only
shows an error for ErrorResponse.usernameAlreadyExists and otherwise just logs
then returns false; update the catch to map other error cases to user-facing
messages by inspecting the caught error (e.g., ErrorResponse cases,
URLError/network errors, or a generic fallback) and call presentError(...) with
a contextual message before returning false; retain
NetworkManager.shared.logger.error("Error in
SetupProfileViewModel.createNewUser: \(error)") for logging but ensure
presentError is invoked for non-username failures so users get feedback.
- Around line 57-60: createNewUser() currently only rejects a nil selectedImage
and can still submit the placeholder image that clear() assigns; update
createNewUser() to perform the same image validation as checkInputIsValid():
ensure selectedImage is non-nil and not equal to the placeholder image instance
assigned in clear() (or call checkInputIsValid() at the start), and call
presentError("Please select a profile picture.") if the selectedImage is the
placeholder so the default asset cannot be uploaded.
- Around line 68-75: The current flow uploads the image before verifying
authentication, which can leave orphaned images; update the method in
SetupProfileViewModel (the block using selectedImage, ImageBody and calling
NetworkManager.shared.uploadImage) to check GoogleAuthManager.shared.user first
with a guard let user = GoogleAuthManager.shared.user and return false/present
the auth error if nil, then proceed to resize/encode and call
NetworkManager.shared.uploadImage; ensure no NetworkManager.shared.uploadImage
call happens unless the auth guard passes.
In `@Resell/Views/NewListing/NewRequestView.swift`:
- Around line 43-46: Remove the immediate router.pop() call from the
PurpleButton action so you only call viewModel.createNewRequest() (and UI
animations) there; rely on the existing onChange(of: viewModel.isLoading)
observer to call router.pop() once the async request finishes. Specifically,
edit the PurpleButton closure (where viewModel.createNewRequest() and
router.pop() are invoked) to delete the synchronous router.pop() invocation and
keep the remainder, leaving the onChange handler to perform the single
navigation pop.
---
Duplicate comments:
In `@Resell/ViewModels/SetupProfileViewModel.swift`:
- Around line 69-98: The code silently returns false when
selectedImage?.resizedToMaxDimension(256).toBase64() is nil; update the flow in
the block around selectedImage/resizedToMaxDimension/toBase64 so that when
imageBase64 is nil you call presentError(...) with a clear message (e.g., "Image
processing failed. Please try a different image.") and then return false, rather
than falling through silently—make the check explicit before creating
ImageBody/uploadImage and ensure presentError is used for any early exit related
to image encoding.
---
Nitpick comments:
In `@Resell/Views/NewListing/NewListingImagesView.swift`:
- Around line 101-114: Remove the commented-out .actionSheet block to avoid dead
code and confusion: delete the entire commented section that references
.actionSheet and ActionSheet (the block toggling viewModel.didShowActionSheet,
viewModel.didShowPhotosPicker, viewModel.didShowCamera), since the active UI
uses .confirmationDialog instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41198267-f2ce-445b-a8fb-0f5e9d1b36f0
📒 Files selected for processing (7)
Resell/Models/User.swiftResell/Utils/Constants.swiftResell/ViewModels/CurrentUserProfileManager.swiftResell/ViewModels/NewListingViewModel.swiftResell/ViewModels/SetupProfileViewModel.swiftResell/Views/NewListing/NewListingImagesView.swiftResell/Views/NewListing/NewRequestView.swift
✅ Files skipped from review due to trivial changes (1)
- Resell/Utils/Constants.swift
| private func setupNotificationObservers() { | ||
| NotificationCenter.default.addObserver( | ||
| forName: Constants.Notifications.NewListingCreated, | ||
| object: nil, | ||
| queue: .main | ||
| ) { [weak self] _ in | ||
| Task { @MainActor in | ||
| print("New listing detected. Refreshing profile...") | ||
| self?.loadProfile(forceRefresh: true) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify potential overlapping load paths for CurrentUserProfileManager.
rg -n -C3 'func loadProfile\(|isLoading|Task \{|setupNotificationObservers|NewListingCreated' Resell/ViewModels/CurrentUserProfileManager.swift
rg -n -C2 'loadCurrentUser\(forceRefresh' Resell/ViewModels/ProfileViewModel.swiftRepository: cuappdev/resell-ios
Length of output: 2174
Guard notification-triggered refreshes to avoid overlapping profile fetches.
The observer fires without checking isLoading, allowing concurrent loadProfile calls when a refresh is already in progress (including from ProfileViewModel.loadCurrentUser). This can cause duplicate network requests and nondeterministic last-write UI state.
Suggested fix
private func setupNotificationObservers() {
NotificationCenter.default.addObserver(
forName: Constants.Notifications.NewListingCreated,
object: nil,
queue: .main
) { [weak self] _ in
Task { `@MainActor` in
- print("New listing detected. Refreshing profile...")
- self?.loadProfile(forceRefresh: true)
+ guard let self else { return }
+ guard !self.isLoading else { return }
+ self.loadProfile(forceRefresh: true)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func setupNotificationObservers() { | |
| NotificationCenter.default.addObserver( | |
| forName: Constants.Notifications.NewListingCreated, | |
| object: nil, | |
| queue: .main | |
| ) { [weak self] _ in | |
| Task { @MainActor in | |
| print("New listing detected. Refreshing profile...") | |
| self?.loadProfile(forceRefresh: true) | |
| } | |
| private func setupNotificationObservers() { | |
| NotificationCenter.default.addObserver( | |
| forName: Constants.Notifications.NewListingCreated, | |
| object: nil, | |
| queue: .main | |
| ) { [weak self] _ in | |
| Task { `@MainActor` in | |
| guard let self else { return } | |
| guard !self.isLoading else { return } | |
| self.loadProfile(forceRefresh: true) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resell/ViewModels/CurrentUserProfileManager.swift` around lines 169 - 178,
The notification observer in setupNotificationObservers currently calls
loadProfile(forceRefresh: true) unconditionally, allowing overlapping fetches;
change the observer closure to first check the view model's isLoading flag
(accessing it on the MainActor) and return early if a load is already in
progress, then only call loadProfile when not loading—this prevents concurrent
network requests and nondeterministic UI updates; reference
setupNotificationObservers, isLoading, and loadProfile (and consider
ProfileViewModel.loadCurrentUser callers) when implementing the guard.
Look at the commit messages for specific bug fixes.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores