Issue/woomob 1075 woo poslocal catalog implement local search for products v2 step 2 #14900
Conversation
malinajirka
changed the base branch from
trunk
to
issue/woomob-1075-woo-poslocal-catalog-implement-local-search-for-products-v2-step-1
malinajirka
added
the
status: do not merge
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
…search-for-products-v2-step-1' into issue/woomob-1075-woo-poslocal-catalog-implement-local-search-for-products-v2-step-2
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the search functionality to use a unified Flow-based API instead of separate local and remote search methods. The main changes consolidate search logic into a single searchProducts() method that emits SearchProductsResult.Local and SearchProductsResult.Remote sequentially, improving code organization and reducing duplication.
- Replaced separate
searchLocalProducts()andsearchRemoteProducts()methods with a unifiedsearchProducts()Flow API - Updated ViewModel to use
collectLatestfor handling search results instead of managing separate coroutine jobs - Modified test mocks to return Flows with sequential emissions of Local and Remote results
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| WooPosItemsSearchViewModelTest.kt | Updated all test mocks to use the new Flow-based search API and adjusted timing with advanceUntilIdle() |
| WooPosProductsInDbDataSourceTest.kt | Added new constructor parameter for search data source dependency |
| WooPosProductsRemoteDataSourceTest.kt | Changed test class name and added new tests for Flow-based search behavior |
| WooPosItemsSearchViewModel.kt | Refactored search logic to use single Flow collection with collectLatest instead of separate jobs |
| WooPosProductsDataSourceInterface.kt | Added SearchProductsResult sealed class and new search-related methods to interface |
| WooPosProductsDataSource.kt | Implemented Flow-based search in both InDb and Remote data sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ain/kotlin/com/woocommerce/android/ui/woopos/home/items/search/WooPosItemsSearchViewModel.kt
Show resolved
Hide resolved
| if (query == currentQuery.get()) { | ||
| childToParentEventSender.sendToParent(ChildToParentEvent.SearchEvent.Finished) | ||
| if (_viewState.value is WooPosItemsSearchViewState.Loading) { | ||
| _viewState.value = WooPosItemsSearchViewState.Empty | ||
| } | ||
| } |
There was a problem hiding this comment.
The 'Finished' event and Empty state handling are placed after collectLatest completes, but collectLatest will be cancelled if a new emission arrives. This code may not execute consistently. Move this logic inside the Remote result handling (after line 156) to ensure it always runs when the Remote result is processed.
There was a problem hiding this comment.
I believe this is a false positive -> the block is after the collectLatest block so will be performed exactly once when the flow terminates. @samiuelson Could you double check my train of thoughts here?
There was a problem hiding this comment.
I agree. I also tested cashed+remote results multiple times and didn't find any issues.
| } | ||
| } | ||
| } | ||
| delay(SEARCH_DEBOUNCING_TIME) |
There was a problem hiding this comment.
📓 This is a behavioral change: Before, we'd immediately search in in-memory cache and use debounce only for remote requests. However, now we debounce all requests, since the VM doesn't know whether the request is expensive or not - moreover, even in-memory and SQL requests aren't free, so this saves battery.
…search-for-products-v2-step-1' into issue/woomob-1075-woo-poslocal-catalog-implement-local-search-for-products-v2-step-2
…search-for-products-v2-step-1' into issue/woomob-1075-woo-poslocal-catalog-implement-local-search-for-products-v2-step-2
malinajirka
removed
the
status: do not merge
malinajirka
deleted the
issue/woomob-1075-woo-poslocal-catalog-implement-local-search-for-products-v2-step-2
branch
Fixes WOOMOB-1075
Do not merge - target branch must be trunk.
Description
This PR completes the local catalog search implementation by integrating the search infrastructure (from part 1) with existing UI components and ViewModels.
Key Changes:
WooPosProductsDataSourceInterfaceWooPosProductsDataSourceWooPosItemsSearchViewModelto use unified search approachThis is part 2 of 2, building on the search infrastructure added in part 1. The feature is now fully functional end-to-end.
Test Steps
Test with site with local catalog enabled
Test the above on site with local catalog disabled. Also verify the search returns cached result first and then results from remote.
Images/gif
No user facing changes - just faster search with local catalog.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.