-
Notifications
You must be signed in to change notification settings - Fork 121
[Local catalog] Add configurable search debounce strategies #16377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Local catalog] Add configurable search debounce strategies #16377
Conversation
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]>
|
|
Update mocks to conform to PointOfSaleSearchingItemsControllerProtocol which now requires currentDebounceStrategy property.
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]>
1efd43d to
9656089
Compare
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 I think you're best off starting by testing here, then reviewing down the stack... but it's hard to say for sure. Let me know whether you find this breakdown more helpful, or if you'd prefer the whole story in one. I spent most of today just wrangling the stack, and getting the tiny details right around loading indicators and other searches working correctly... Note that there's a pre-existing behaviour where there's a loading indicator for popular products if you open search too soon after opening POS – this is just how popular products works, we try to load it on POS opening, but don't want to delay using POS, so if we need to, we show the loading indicators |
|
👋 @joshheald is this one ready for review as stand-alone PR? |
Yeah, absolutely – I don't want to merge any the stack until this one's reviewed, because of all the periphery nonsense further down. This is the leaf PR though, so go for it. |
iamgabrielma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
| /// - Parameters: | ||
| /// - duration: The debounce duration in nanoseconds for subsequent keystrokes | ||
| /// - loadingDelayThreshold: Optional threshold in nanoseconds before showing loading indicators. If nil, shows loading immediately. | ||
| case smart(duration: UInt64, loadingDelayThreshold: UInt64? = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all cases seem to use the same default, perhaps we can pass it here so we don't need to add it on each instance:
| case smart(duration: UInt64, loadingDelayThreshold: UInt64? = nil) | |
| case smart(duration: UInt64 = 500 * NSEC_PER_MSEC, loadingDelayThreshold: UInt64? = nil) |
Modules/Tests/YosemiteTests/PointOfSale/SearchDebounceStrategyTests.swift
Outdated
Show resolved
Hide resolved
| // finish i.e. it is still ongoing, we debounce the next keystrokes by 300ms. In either case, | ||
| // the ongoing search is redundant now there's a new search term, so we cancel it. | ||
| let shouldDebounceNextSearchRequest = !didFinishSearch | ||
| // Cancel any ongoing search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not specially opinionated about it, but would you say extracting this onChange logic into a switch first for its selection, and then separate each into private functions for its implementation, would make it more readable?
.onChange {
await performSearchWithStrategy(term: newValue, strategy: debounceStrategy)
...
private func performSearchWithStrategy(term: String, strategy: SearchDebounceStrategy) async {
switch strategy {
case .smart(let duration, let loadingDelayThreshold):
await executeSmartSearch(term: term, duration: duration, threshold: loadingDelayThreshold)
case .simple(let duration, let loadingDelayThreshold):
await executeSimpleSearch(term: term, duration: duration, threshold: loadingDelayThreshold)
case .immediate:
await executeImmediateSearch(term: term)
}
}
...
Co-authored-by: Gabriel Maldonado <[email protected]>
Generated by 🚫 Danger |
851119c
into
woomob-1112-woo-poslocal-catalog-analytics

Part of: WOOMOB-1112
Description
This PR fixes the flickering when we do a local search, while retaining the debounce approach used in existing places
Implements configurable debouncing strategies for search to optimize the user experience for both local and remote searches. Different search types have different performance characteristics and benefit from different debounce behaviors.
Changes include:
SearchDebounceStrategyenum with three strategies:.smart: Skip debounce on first keystroke after search completes, then debounce subsequent keystrokes (optimized for slow network searches).simple: Always debounce every keystroke with optional loading delay threshold (optimized for fast local searches to prevent flicker).immediate: No debouncing, execute search on every keystrokedebounceStrategyproperty to fetch strategy protocolsThis ensures responsive UI for fast local searches while preventing loading indicator flicker, and optimized behavior for slower network searches.
This PR is stacked on #16376 and is the fifth and final PR in the series implementing local catalog search.
Test Steps
N.B. – the remote feature flag now requires WCiOS 23.8 – change it in the project file when you want to use a local catalog store.

Debouncing
Repeat this slower with the software keyboard and observe that there's no flicker (but there will be intermediate results)
Test with the feature flag disabled – observe that loading indicators are still shown and work as expected
Check Coupons and Order History as well.
General search testing
Remaining issues
Note that there are two issues still to tackle in future PRs
Screenshots
Search.-.01.mp4
RELEASE-NOTES.txtif necessary.