[Woo POS][Local Catalog] Add incremental sync triggers #16264

staskus · 2025-10-21T10:12:01Z

WOOMOB-1098
Done based on the project test plan: pdfdoF-7RD-p2

Description

Add incremental sync triggers:

Pull to refresh
Payment completion
POS opened
App opened

Solution

Pull to Refresh

For PTR, I needed to update PointOfSaleObservableItemsController to support refreshItems method. Within it, I added coordinator.performIncrementalSync call. However, the error handling was more trickier part. I had to introduce refreshState to track when the refresh happens or fails.

ChildItemList was also updating every time anything was changing in observable, therefore, the .refreshable would get canceled immediately after refreshState changed. I had to wrap it in another task to make sure it doesn't get canceled automatically.

refreshState needs to be included in multiple checks to handle error and empty list cases, to make sure the refresh is triggered at the right times.

Payment Completion

For payment completion, I'm using Observation framework observation tracking to check for cash and card payment success to trigger incremental sync

POS Opened

The most straightforward place was to trigger sync within aggregate model loading.

App Opened

It was already implemented, but I updated performSmartSync to not do incremental sync if it has happened within an hour.

Steps to reproduce

Launch POS

Confirm in logs that incremental sync is done

PTR on Parent and Child view

Do pull to refresh on parent and child views
Check logs, confirm incremental sync happens
Make a change on wp-admin
Do PTR, confirm changes appear (note incremental sync for variations has issues WOOMOB-1546)

PTR + Errors + Recovery

Block connection before PTR
PTR, confirm inline error appears
Re-enable connection
PTR / Tap Retry
Confirm the incremental sync is done

Payment completion

Complete cash payment, confirm in logs that incremental sync is done
Complete card payment, confirm in logs that incremental sync is done

App open + incremental sync skipped if 1h not passed

Do pull to refresh
Go to background
Come back to foreground
Check logs, confirm the incremental sync is skipped

Testing information

Tested on iPad Air 26 simulator, new and established sites.

Screenshots

Incremental sync on: Open POS, PTR on parent and child, payment completion, skipped on app launch

incremental.sync.2.mov

Empty State

Simulator.Screen.Recording.-.iPad.Air.11-inch.M3.-.2025-10-21.at.12.37.46.mov

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Catalog size limit check can fail due to network issues and get unreported up the chain. Throw an error when that happens.

Set refreshError when syncing fails. Most often it will be an inline error, showed together with the content. Also, if error is set, refresh items when loadItems is called, which happens when retry is tapped on the error.

… redrawn

…ental-sync-triggers-pull-to

wpmobilebot · 2025-10-21T10:23:20Z

📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS Prototype
	Build Number	`pr16264-b9d209a`
	Version	`23.5`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`b9d209a`
	Installation URL	63vq1e7m016to

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

joshheald

However, the error handling was more trickier part. I had to introduce refreshState to track when the refresh happens or fails.

Would this be needed if we didn't have to wrap the refresh task in a Task block? Since it's not required in the existing implementation, I assume not?

It seems like it would be a lot simpler if we didn't have that extra state...

Potentially it can be removed. The important thing is not to change the view while there's a refresh happening. Perhaps that's unavoidable while we're observing the underlying database... but I don't think it should be changing except when the refresh task completes... and maybe that would be fine?

I've not had time to dig deep, can sync on it tomorrow if you like. We did face similar issues with the original implementation and managed to fix them all so that we didn't have to swallow the errors with that Task block.

There's a few questions and suggestions in line too.

It does work as expected and test well, so I'm cautiously approving it, but it'd be really good to explore further whether we can remove refreshState before merging. Let me know what you think...

joshheald · 2025-10-22T15:07:19Z

Modules/Sources/PointOfSale/Presentation/Item Selector/ChildItemList.swift

+                await Task {
+                    await itemsController.refreshItems(base: node)
+                }.value


I thought that this is only needed because we set the state to loading on a refresh, which isn't really needed – the refreshing spinner that iOS puts up for us is the indicator that something's happening. However, removing the task shows that some explicitlyCancelled errors are coming through in that case.

I remember writing some code to swallow these – they're not real errors, it's an AlamoFire thing when a previous request gets cancelled, I think. Perhaps we need the same in the incremental sync service somewhere? With that, and removing the call to always set a loading state, I think we could get rid of the task.

I noticed it happening even before I had a loading state, and only had an error state. It was enough to set the error to nil before the incremental sync to cancel it.

In short, if I made any view updates while the refreshItems was still ongoing, the SwiftUI would kill the refreshable task, which caused an explicitlyCancelled error to happen. Even if we swallow the error not to be displayed, the refresh task remains canceled.

I think the problem may be with the view structure, since it doesn't happen on the ItemListView. However, I wasn't able to find a structure that wouldn't produce this cancellation...

Can you reproduce it with the existing item controller? If not, perhaps that rules out the view structure?

In short, if I made any view updates while the refreshItems was still ongoing, the SwiftUI would kill the refreshable task, which caused an explicitlyCancelled error to happen. Even if we swallow the error not to be displayed, the refresh task remains canceled.

Yes, that's unavoidable unfortunately. I'm confusing two memories, we had explicitly cancelled errors from elsewhere for something else. The key is that we can't change the view while we're refreshing.

Can you reproduce it with the existing item controller? If not, perhaps that rules out the view structure?

I'll try!

joshheald · 2025-10-22T15:54:14Z

Modules/Sources/PointOfSale/Controllers/PointOfSaleObservableItemsController.swift

+        // Loading state - preserve existing items (both for data loading and refresh)
+        if dataSource.isLoadingProducts || refreshState == .loading {


Suggested change

// Loading state - preserve existing items (both for data loading and refresh)

if dataSource.isLoadingProducts || refreshState == .loading {

// Loading state - preserve existing items (only use for data loading, refresh has a PTR spinner)

if dataSource.isLoadingProducts && refreshState != .loading {

We shouldn't show the loading cell if we're refreshing – the PTR indicator is sufficient, and changing the state breaks the refresh task anyway.

I implemented it to support the error "retry" case, but we could live without it. It's a shame there's no way to manually show a PTR loading animation. I remember trying to replicate it without success.

joshheald · 2025-10-22T15:59:28Z

Modules/Sources/PointOfSale/Controllers/PointOfSaleObservableItemsController.swift

+        // Loading state - preserve existing items (both for data loading and refresh)
+        if dataSource.isLoadingVariations || refreshState == .loading {


Suggested change

// Loading state - preserve existing items (both for data loading and refresh)

if dataSource.isLoadingVariations || refreshState == .loading {

// Loading state - preserve existing items (only use for data loading, refresh has a PTR spinner)

if dataSource.isLoadingProducts && refreshState != .loading {

joshheald · 2025-10-22T16:00:12Z

Modules/Sources/PointOfSale/Controllers/PointOfSaleObservableItemsController.swift


    // Track current parent for variation state mapping
    private var currentParentItem: POSItem?
+    private var refreshState: RefreshState = .idle


What do you think about this being shared between root and variations? It feels like an opportunity to leak state between views by accident...

Well, I made it shared since the incremental sync itself is shared. There's no separate sync for specific parent views. How do you think I should structure it?

Ah yes, I see. Makes sense.

The one niggle I have is that this can be out of sync with the view state, because Apple's code's in control of which PTR indicator is shown. If you PTR on a variation screen, then go back to a parent screen and scroll down to load more items, we'd be loading and refreshing at the same time. Which is kind of OK... but if we use the refresh state to decide to show a loading indicator, we could finish one load without finishing the incremental sync, and still show a loading indicator for the refresh.

If we remove the loading cell for refresh tasks it's less of an issue... and we deliberately don't show that for PTR in the existing implementation, as the spinner is enough.

…ental-sync-triggers-pull-to

staskus · 2025-10-23T09:20:38Z

@joshheald, thanks for testing!

Would this be needed if we didn't have to wrap the refresh task in a Task block? Since it's not required in the existing implementation, I assume not?
It seems like it would be a lot simpler if we didn't have that extra state...
Potentially it can be removed. The important thing is not to change the view while there's a refresh happening. Perhaps that's unavoidable while we're observing the underlying database... but I don't think it should be changing except when the refresh task completes... and maybe that would be fine?

True, we could avoid that if we wouldn't be able to set error to nil until the refresh is complete. So when the user taps "Retry" on an error, there would be no indication of progress. I could try changing the state separately from the view side, that would be a bit of a hack.

The best thing would be to figure out why SwiftUI redraws the ChildList and kills the refreshable task when the error changes but it doesn't do that for the ItemListView.

It does work as expected and test well, so I'm cautiously approving it, but it'd be really good to explore further whether we can remove refreshState before merging. Let me know what you think...

I'm not sure I can remove refreshState entirely, we still need to have some state to display errors.

joshheald · 2025-10-23T09:27:04Z

True, we could avoid that if we wouldn't be able to set error to nil until the refresh is complete. So when the user taps "Retry" on an error, there would be no indication of progress.

Could we do that and indicate progress for the retry using a different indicator? Perhaps a spinner on the retry button? I think the UX makes sense – you PTR, it fails, we show an error. You then tap a button to retry, it's not a PTR any more, so an indicator on the button is perhaps clearer.

staskus · 2025-10-23T10:20:49Z

Could we do that and indicate progress for the retry using a different indicator? Perhaps a spinner on the retry button? I think the UX makes sense – you PTR, it fails, we show an error. You then tap a button to retry, it's not a PTR any more, so an indicator on the button is perhaps clearer.

@joshheald thanks, I'll try that. I think we have a button loading state developed.

staskus · 2025-10-23T10:44:27Z

@joshheald I also remembered one more case for the loading state. When we have an empty view and we want to show a loading view when the refresh is tapped. However, we could only affect this particular state.

joshheald · 2025-10-23T13:50:45Z

@joshheald I also remembered one more case for the loading state. When we have an empty view and we want to show a loading view when the refresh is tapped. However, we could only affect this particular state.

Yeah. I think the key is: don't change the state when doing a Pull to Refresh, let Apple handle it.

Additionally, remove the business logic duplications from the controller

They can happen when PTR is started in one view and we move to another. No need to show an error.

dangermattic · 2025-10-23T16:03:52Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

This hack was required to support child view changes while refresh is happening. However, we avoid changing the state now while refresh is happening so it's no longer necessary.

staskus · 2025-10-24T06:37:31Z

Removed the Task {} wrapped in the refreshable task
Added a retry animation on an inline error
Made error comparison more robust
Only showing loading from error/empty if there are no items
Ignoring cancellation errors as in other controllers

Retry.animation.mov

staskus · 2025-10-24T07:58:20Z

@joshheald

It does work as expected and test well, so I'm cautiously approving it, but it'd be really good to explore further whether we can remove refreshState before merging. Let me know what you think...

I wasn't able to remove refreshState, so I'm not forcing a merge for now, especially if there are some unexplored ways 🤔

joshheald · 2025-10-24T08:13:53Z

OK, I'll take it over and try the other ways early next week. I'll probably end up merging as is, but at least we'll be sure

staskus · 2025-10-24T09:24:04Z

Thanks! @joshheald. I wanted to get it over the line but better be safe with this core functionality.

staskus added 12 commits October 20, 2025 14:51

Skip incremental sync if one hour hasn't passed in performSmartSync

052a5e2

Perform incremental POS catalog sync on pull to refresh

a2d89cb

Throw errors from missing paths in POSCatalogSyncCoordinator

2e2560a

Catalog size limit check can fail due to network issues and get unreported up the chain. Throw an error when that happens.

Remove unnecessary tests from GRDBObservableDataSourceTests

2bb944d

Add error handling to PointOfSaleObservableItemsController

3cfabc2

Set refreshError when syncing fails. Most often it will be an inline error, showed together with the content. Also, if error is set, refresh items when loadItems is called, which happens when retry is tapped on the error.

Add a helper for continuously tracking observations

5d88b5a

Perform incremental sync after a successful payment on POS

557db95

Verify that incremental sync is triggered on successful payment

8d6c664

Perform incremental sync when POS launches

2e810f4

Ensure ChildItemList refresh task is not canceled even if the view is…

81af5ec

… redrawn

Add a RefreshState to handle loading and item list view cases

a49da86

Make withObservationTracking internal

97b2109

staskus added this to the 23.6 milestone Oct 21, 2025

staskus added type: task An internally driven task. feature: POS labels Oct 21, 2025

Merge branch 'trunk' into woomob-1098-woo-poslocal-catalog-add-increm…

964662a

…ental-sync-triggers-pull-to

staskus requested a review from joshheald October 21, 2025 10:36

joshheald approved these changes Oct 22, 2025

View reviewed changes

Merge branch 'trunk' into woomob-1098-woo-poslocal-catalog-add-increm…

d7585bb

…ental-sync-triggers-pull-to

staskus added 4 commits October 23, 2025 17:20

Add a loading state for an outlined button style

381fb22

Set loading state to POSListInlineErrorView

bb7d5f6

Only refresh items list if there are no products

ce0cf16

Additionally, remove the business logic duplications from the controller

Ignore requestCancelled failures

b182b62

They can happen when PTR is started in one view and we move to another. No need to show an error.

staskus added 4 commits October 24, 2025 09:10

Remove unused code

a31e679

Remove unnecessary "Observation+WithObservationTrackingOf" file

83944b5

Compare two POSCatalogSyncError in PointOfSaleObservableItemsController

c17e0bb

Remove a task wrapper around refreshable to avoid cancelation

b9d209a

This hack was required to support child view changes while refresh is happening. However, we avoid changing the state now while refresh is happening so it's no longer necessary.

joshheald self-assigned this Oct 24, 2025

		// Loading state - preserve existing items (both for data loading and refresh)
		if dataSource.isLoadingProducts \|\| refreshState == .loading {

		// Loading state - preserve existing items (both for data loading and refresh)
		if dataSource.isLoadingVariations \|\| refreshState == .loading {

[Woo POS][Local Catalog] Add incremental sync triggers #16264

Are you sure you want to change the base?

[Woo POS][Local Catalog] Add incremental sync triggers #16264

Uh oh!

Conversation

staskus commented Oct 21, 2025

Description

Solution

Pull to Refresh

Payment Completion

POS Opened

App Opened

Steps to reproduce

Launch POS

PTR on Parent and Child view

PTR + Errors + Recovery

Payment completion

App open + incremental sync skipped if 1h not passed

Testing information

Screenshots

Incremental sync on: Open POS, PTR on parent and child, payment completion, skipped on app launch

Empty State

Uh oh!

wpmobilebot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshheald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus commented Oct 23, 2025

Uh oh!

joshheald commented Oct 23, 2025

Uh oh!

staskus commented Oct 23, 2025

Uh oh!

staskus commented Oct 23, 2025

Uh oh!

joshheald commented Oct 23, 2025

Uh oh!

dangermattic commented Oct 23, 2025

Uh oh!

staskus commented Oct 24, 2025

Uh oh!

staskus commented Oct 24, 2025

Uh oh!

joshheald commented Oct 24, 2025

Uh oh!

staskus commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wpmobilebot commented Oct 21, 2025 •

edited

Loading