[Local Catalog] Show items from GRDB in POS #16235
Conversation
Adds a new `.initial` case to `ItemListState` to represent "not yet loaded" state, distinct from "actively loading". Benefits: - More explicit state machine: initial → loading → loaded/empty/error - Foundation for delayed loading indicators (300ms threshold) - Better UX for instant GRDB loads (no flashing skeleton) Updated existing `PointOfSaleItemsController` to: - Start with `.initial` instead of `.loading([])` - Transition to `.loading([])` on first load - Preserve existing behavior for refreshes
Creates a new controller that wraps `POSObservableDataSourceProtocol` for GRDB-based item loading. Key features: - Conforms to `PointOfSaleItemsControllerProtocol` (same interface as existing controller) - Computed `itemsViewState` from observable data source properties - Automatic UI updates via observation (no manual state management) - Supports products and variations with `.initial` state - Tracks current parent for variation state mapping Implementation notes: - No pagination tracking needed (data source handles it) - No search support (out of scope for this PR)
Comprehensive test coverage for the new observable controller: - Initial state (loading container, initial root) - Product loading (loaded, empty, error states) - Variation loading (loaded, empty states) - Pagination (hasMoreItems flags) - Independence of products and variations - Parent switching behavior - Error handling - Loading state preservation Fix tests to use proper POSItem construction Use helper methods to create POSSimpleProduct and POSVariation structs instead of non-existent .fake() method, matching the pattern used in MockPOSItemProvider. Changes: - Added makeSimpleProduct() helper - Added makeVariation() helper - Updated all 12 tests to use proper construction
When grdbManager, catalogSyncCoordinator, and the pointOfSaleLocalCatalogi1 feature flag are all present, use the new PointOfSaleObservableItemsController backed by GRDBObservableDataSource instead of the standard PointOfSaleItemsController. This enables reactive database-driven item lists for stores with local catalog sync enabled.
joshheald
added
type: task
|
@staskus Sorry it took longer than I thought it would, but here's the second PR as mentioned this morning. I'm AFK next week – happy for you to merge them then if you prefer not to review this afternoon. As long as there's nothing major needed as changes! |
| import enum Yosemite.POSItem | ||
|
|
||
| enum ItemListState { | ||
| case initial |
There was a problem hiding this comment.
I added this state for more accurate modelling of what's happening.
My intention is to delay showing the loading indicators when we open variations with GRDB – it causes a slight flicker at the moment. This is some groundwork for that.
|
|
|
@staskus I also tested that it observes changes: this branch adds an incremental sync button to the ... menu. I used that by editing products on the web and then syncing. Trashing a product didn't remove it, which is probably down to how we request them or filter the products from the db... but I'll fix it in a separate PR. https://linear.app/a8c/issue/WOOMOB-1493/woo-poslocal-catalog-ensure-that-trashed-products-are-updated-to-not Other changes and additions worked as expected. |
| func loadItems(base: ItemListBaseItem) async { | ||
| switch base { | ||
| case .root: | ||
| hasLoadedProducts = true |
There was a problem hiding this comment.
Should we delay it until the items are loaded? Same with other similar flags.
| func refreshItems(base: ItemListBaseItem) async { | ||
| switch base { | ||
| case .root: | ||
| dataSource.refresh() |
There was a problem hiding this comment.
So, this is not implemented. Is the idea to trigger an incremental sync when refresh is called?
| } | ||
|
|
||
| // Error state | ||
| if let error = dataSource.error, items.isEmpty { |
There was a problem hiding this comment.
What kind of errors can we expect here? Since this comes from database, only something super-unexpected I suppose?
There was a problem hiding this comment.
Thank you, @joshheald, for work! It took some time to review and understand. I'm not yet that familiar with GRDB, plus there's complexity in clean state management and observability. We don't want the views to stuck in some "no man's land" state.
I think it's mostly good. The comments are in both PRs. I tested with smaller and larger stores. I think there are a couple of things that need to be addressed. I don't see much value in pushing this to trunk without addressing and understanding them properly. I will see if I find time to come back to this after finishing the synchronization work. Otherwise, we can discuss next week and fix things or move to other tasks and merge this work.
3 participants
Merge after: #16231
Description
This PR starts using the observable GRDB data source added in #16231 to show items from GRDB in the item list.
Note that there's nothing to enforce that GRDB actually has items in it at the moment... so you can definitely break this, but those guards are covered by other tickets and this is feature flagged.
Steps to reproduce
Check we're not breaking production...
pointOfSaleLocalCatalogi1feature flag to falseTest the GRDB item controller
Example sync log messages:
Optional: test with a larger site
POSCatalogSyncCoordinator.Constants.defaultSizeLimitForPOSCatalogto 100000Note that with larger catalogs, you start to hit scrolling performance issues. These are outside the scope of current work, but we may want to address them before too long. I don't think it's down to the mapping happening on the main thread, and it may even be that we're hitting some SwiftUI limitations.
Testing information
Note that Search and Barcode Scanning are still using the network-based controller, so have delays.
Screenshots
GRDB.items.mp4
RELEASE-NOTES.txtif necessary.