[Local catalog] Wire local search strategy to factory and UI #16375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Integrates the local search strategy with the existing fetch strategy factory pattern and wires it up to the POS UI. The factory automatically chooses between local and remote search based on GRDB manager availability. Changes: - Update PointOfSaleItemFetchStrategyFactory: - Add optional grdbManager parameter to initializer - Add localSearchStrategy() factory method - Update searchStrategy() to prefer local when GRDB manager available - Falls back to remote search when local unavailable - Update POSTabCoordinator: - Pass ServiceLocator.grdbManager to factory initialization - Enables local search when local catalog is available - Update analytics infrastructure: - Add trackSearchLocalResultsFetchComplete() to protocol - Update mock analytics tracker for testing Behavior: - When GRDB manager is available → uses local catalog search - When GRDB manager is nil → falls back to existing remote search - No changes to UI layer (transparent integration) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add isLocalCatalogEnabled parameter to factory init - Replace implicit GRDB presence check with explicit flag check - Remove side-effecty localSearchStrategy helper method - Add stub implementation for trackSearchLocalResultsFetchComplete - Pass isLocalCatalogEligible flag when creating factories in POSTabCoordinator - Convert lazy factory properties to factory methods with eligibility parameter - Makes intent clear and improves testability The factory now explicitly checks the isLocalCatalogEnabled flag rather than relying on the presence of GRDBManager to determine which search strategy to use. This makes the behavior more predictable and testable. The POSTabCoordinator now dynamically creates factories with the current eligibility state each time POS is presented, ensuring the correct search strategy is used based on real-time eligibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Rename analytics event from pointOfSaleSearchLocalResultsFetched to pointOfSaleSearchResultsFetched - Change stat name from search_local_results_fetched to search_results_fetched - Add analytics tracking to local search strategy fetchProducts method - Track milliseconds since request sent and total results count - Only track on first page to avoid duplicate analytics The new event name reflects that this will be the only search method in the future when local catalog is fully rolled out. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Introduces a new enum to define different debouncing behaviors for search operations: - `.smart(duration)`: Skip debounce on first keystroke after search completes, then debounce subsequent keystrokes. Optimized for slow network searches. - `.simple(duration, loadingDelayThreshold?)`: Always debounce every keystroke. Optionally delays showing loading indicators until threshold is exceeded. Optimized for fast local searches. - `.immediate`: No debouncing for non-search operations. This allows different search types (local vs remote) to use appropriate debouncing strategies tailored to their performance characteristics. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds a `debounceStrategy` property to both PointOfSalePurchasableItemFetchStrategy and PointOfSaleCouponFetchStrategy protocols with default implementations returning `.immediate`. This allows each fetch strategy implementation to declare its optimal debouncing behavior: - Remote search strategies can use `.smart()` for network latency - Local search strategies can use `.simple()` with loading delay thresholds - Default strategies use `.immediate` for no debouncing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Implements the debouncing strategy pattern in the search UI layer: - Adds `debounceStrategy` property to POSSearchable protocol - Updates POSSearchField's onChange handler to execute different debouncing logic based on strategy: - `.smart`: Skip debounce on first keystroke, debounce subsequent - `.simple`: Always debounce, with optional delayed loading indicators - `.immediate`: No debouncing - Adds `currentDebounceStrategy` to item and coupon controllers to expose fetch strategy's debouncing behavior - Implements protocol conformance in POSProductSearchable, POSOrderListView, and preview helpers The `.simple` strategy with loading delay threshold prevents flicker on fast local searches by only showing loading indicators if the search exceeds the threshold duration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Implements `.simple(duration: 150ms, loadingDelayThreshold: 300ms)` for local GRDB product searches. This strategy: - Always debounces every keystroke by 150ms to prevent excessive queries - Delays showing loading indicators until 300ms threshold is exceeded - Prevents loading flicker for fast local searches (< 300ms) - Only shows loading indicators for slower searches (> 300ms) The combination of debouncing with delayed loading provides a responsive feel while avoiding visual flickering on fast local database queries. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds comprehensive test coverage for the debouncing strategy pattern: - Tests for SearchDebounceStrategy enum equality - Tests verifying each fetch strategy returns the correct debouncing behavior: - Local product search: `.simple` with loading delay threshold - Remote product search: `.smart` for network latency - Coupon search: `.smart` for network latency - Default strategies: `.immediate` for no debouncing Tests ensure the debouncing strategies are correctly configured across all search types for optimal UX. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 task
Collaborator
|
|
Only access ServiceLocator.grdbManager when isLocalCatalogEnabled is true to avoid fatal error when pointOfSaleLocalCatalog feature flag is disabled.
Update mocks to conform to PointOfSaleSearchingItemsControllerProtocol which now requires currentDebounceStrategy property.
Accidentally changed version number in commit 3e82715. Reverting to correct version.
Adds optional loading delay threshold parameter to smart debounce strategy. This enables delayed loading indicators for fast local searches to prevent flicker, while maintaining immediate loading indicators for remote searches. Changes: - Add optional loadingDelayThreshold parameter to SearchDebounceStrategy.smart case - Update POSSearchView to handle smart strategy with optional threshold: - Check for non-empty search term before showing loading (prevents loading on initial view) - Reset didFinishSearch to true when search view appears (ensures first search shows loading) - First keystroke without threshold: Show loading immediately (remote searches) - First keystroke with threshold: Delay loading until threshold or completion (local searches) - Subsequent keystrokes: Debounce request, loading already showing - Remote searches use .smart without threshold for immediate responsive feedback - Local searches use .simple with threshold to prevent flicker on fast queries - Update test expectations to match strategy configurations Fixes: - No loading indicators shown when opening search view with popular products - Loading indicators show immediately on first search keystroke for remote searches - Local searches avoid flicker by only showing loading if query takes longer than threshold
The issue was that the debounce strategy was determined by the current fetch strategy, but the fetch strategy didn't change from "popular products" to "search" until performSearch() was called. This meant the first keystroke used the `.immediate` strategy from popular products, which set `didFinishSearch=false` but didn't call `clearSearchResults()` to show loading. Solution: Added `searchDebounceStrategy` property to POSSearchable protocol that returns the debounce strategy that will be used for searches, regardless of the current fetch mode. The onChange handler now captures this strategy synchronously before creating the Task, ensuring we use the search strategy (.smart) from the very first keystroke. Changes: - Added `searchDebounceStrategy` to POSSearchable protocol - Updated POSSearchView to use searchDebounceStrategy for non-empty searches - Implemented searchDebounceStrategy in controllers by creating temporary search strategy to query its debounce settings - Updated all mocks and preview helpers to conform to new protocol 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The POSSearchable protocol had two confusingly named properties: - debounceStrategy: The currently active strategy - searchDebounceStrategy: The strategy that will be used for searches Renamed debounceStrategy → currentDebounceStrategy to make the distinction clear: currentDebounceStrategy reflects what's active right now, while searchDebounceStrategy reflects what will be used when searching. This improves code readability without changing any behavior.
iamgabrielma
approved these changes
Nov 21, 2025
| "$(inherited)", | ||
| "@executable_path/Frameworks", | ||
| ); | ||
| MARKETING_VERSION = 23.7; |
Contributor
There was a problem hiding this comment.
Should we revert this change? Should be overwritten by code freeze through build_code_code_freeze(version_short: new_version), but I'm not positive will also override the lines newly added.
Contributor
Author
There was a problem hiding this comment.
Ugh I thought I caught this! I'll make sure it's dealt with in the merge, and if not, I'll fix it before it hits trunk. Thanks for pointing it out.
This reverts commit 300cd55.
… into woomob-1112-woo-poslocal-catalog-factory-integration
…nto woomob-1112-woo-poslocal-catalog-analytics
…-1112-configurable-search-debounce-strategies
… into woomob-1112-woo-poslocal-catalog-factory-integration
…nto woomob-1112-woo-poslocal-catalog-analytics
…-1112-configurable-search-debounce-strategies
Co-authored-by: Gabriel Maldonado <[email protected]>
joshheald
merged commit 9d31a95
into
woomob-1112-woo-poslocal-catalog-local-search-strategy
3 of 13 checks passed
joshheald
deleted the
woomob-1112-woo-poslocal-catalog-factory-integration
branch
Collaborator
Generated by 🚫 Danger |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of: WOOMOB-1112
Description
Integrates the local search strategy into the factory pattern and wires it up to the POS UI.
This ensures the correct search strategy (local or remote) is used based on the runtime state of local catalog eligibility.
This PR is stacked on #16374 and is the third in a series of 5 PRs implementing local catalog search.
Test Steps
You can try out search from here, but this whole stack is still probably best tested using #16377, the last one in the stack.
Note that with this initial implementation, there's some flickering in the searches, but that's resolved in the last PR
Note that #16377 resolves all the periphery issues – I'll merge them from that down, so the final merge to trunk will pass periphery. It seems silly to put a load of ignore statements in for the sake of this.
RELEASE-NOTES.txtif necessary.