Skip to content

ALFMOB-83: Wishlist Functionality(Continued)#13

Open
artyomromanov wants to merge 1 commit intomainfrom
dev/ALFMOB-83_add_to_wishlist
Open

ALFMOB-83: Wishlist Functionality(Continued)#13
artyomromanov wants to merge 1 commit intomainfrom
dev/ALFMOB-83_add_to_wishlist

Conversation

@artyomromanov
Copy link
Copy Markdown
Contributor

@artyomromanov artyomromanov commented Jan 19, 2026

What is here

  • Add Database for wishlist IDs,
  • Flows to get and set them for PLP
  • Grid and Large Item views
  • UseCase to fetch the Wishlist products in Wishlist Screen itself(now simplified)

Demo

Screen_recording_20260119_134311.webm

TODO

  • Unit Tests

@artyomromanov artyomromanov force-pushed the dev/ALFMOB-83_add_to_wishlist branch from 7deb9fa to 61b1b47 Compare January 19, 2026 13:53
@artyomromanov artyomromanov force-pushed the dev/ALFMOB-83_add_to_wishlist branch from 61b1b47 to 91ca4a5 Compare January 21, 2026 10:41
@amccall-mindera
Copy link
Copy Markdown

Code Review — ALFMOB-83: Wishlist Functionality (Continued)

🔴 High — Silent error swallowing in GetWishlistUseCase

GetWishlistUseCase.kt:28-36 — When productRepository.getProduct(id) fails, the error is silently swallowed. The product becomes null and gets filtered out by filterNotNull(). If the network is down, the entire wishlist appears empty with no error indication.

async(Dispatchers.IO) {
    var product: Product? = null
    productRepository.getProduct(id)
        .onSuccess { product = it }
    product  // null if getProduct failed — silently dropped
}

Suggestion: Propagate errors to the UI, or at minimum log warnings and show partial-load indicators.

🔴 High — Error handling removed from WishlistViewModel

WishlistViewModel.kt:36-43 — The previous implementation had WishlistUiState.Error handling which was completely removed. Now results are always treated as success. If loading fails, the UI will either stay in Loading forever or show an incomplete wishlist.

Suggestion: Add error handling back. At minimum, wrap collectLatest in a try/catch and update state to an error variant.

🟠 Medium — Unused imports in AddToWishlistUseCase

AddToWishlistUseCase.kt:4-11 — Multiple unused imports left over from the old implementation (ProductRepository, Product, onSuccess, awaitAll, coroutineScope, Flow, flatMapLatest, flowOf). Will cause lint warnings.

🟡 Medium — Missing unit tests (noted as TODO)

The PR introduces GetWishlistIdsUseCase, rewrites GetWishlistUseCase (async parallel fetching), adds WishlistDao, and rewrites WishlistRepositoryImpl — all without tests. The existing GetWishlistUseCaseTest expects the old interface and will likely fail. The error handling gaps above make test coverage especially important.

Recommendation: The error handling issues (silent failure, removed error state) should be addressed before merge. Unit tests are also needed given the complexity of the async product-fetching logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants