[Local Catalog] Add incremental sync functionality to `POSCatalogSyncCoordinator` #16117

jaclync · 2025-09-11T05:28:07Z

⚠️ Based on #16115

Description

This PR adds incremental sync functionality to the POSCatalogSyncCoordinator by integrating with POSCatalogIncrementalSyncService which only syncs changes since the last sync.

The implementation adds a new performIncrementalSyncIfApplicable method that intelligently determines when incremental sync is appropriate based on:

Whether a full sync has been previously performed
The age of the last incremental sync (configurable via dependency injection)
A force sync option to bypass age checks when needed

Key Changes

New functionality: Added performIncrementalSyncIfApplicable(for:forceSync:) method to sync coordinator
- I combined the "should perform sync" logic inside performIncrementalSyncIfApplicable so that the call site just makes one call when it's appropriate to trigger an incremental sync. Feel free to share any thoughts on this!
Sync logic: Implements conditions to determine when incremental sync should run:
1. No ongoing incremental sync for the site
2. A full sync has been performed previously (last full sync date exists)
3. Sufficient time has passed since last incremental sync (respects maxIncrementalSyncAge) (I set it to 5 minutes tentatively)
Force sync support: forceSync parameter bypasses age checks for immediate sync when needed
Testing: Added test cases covering sync conditions and edge cases

Steps to reproduce

As incremental sync hasn't been integrated with POS yet, it can be tested by adding debug code below MainTabBarController.swift#L315 to trigger an incremental sync when tapping on the POS tab:

Task { @MainActor in
    do {
        try await posCatalogSyncCoordinator?.performIncrementalSyncIfApplicable(for: ServiceLocator.stores.sessionManager.defaultStoreID!, forceSync: false)
        print("Incremental sync succeeded")
    } catch {
        print("Incremental sync failed: \(error)")
    }
}

And then:

Launch the app, log in to a store eligible for POS if needed, and ensure a full sync has been performed successfully by checking the console
In the Products tab or wp-admin, update at least one variation of a variable product (e.g. price, inventory) and save
Tap on the POS tab --> it should trigger an incremental sync, and the console logs should indicate that 1 product at least 1 variation have been synced
Exit POS
Tap on the POS tab again --> the console logs should indicate that the sync is not needed with the last incremental sync date

Testing information

I tested the above steps in iPad A16 iOS 18.4 simulator.

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

…IfApplicable`.

wpmobilebot · 2025-09-11T05:37:36Z

📲 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	`pr16117-54ca153`
	Version	`23.2`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`54ca153`
	Installation URL	65vngotrej7t0

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

…nto feat/WOOMOB-1252-sync-coordinator-incremental-sync

…al-sync

joshheald · 2025-09-11T11:29:06Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift

+    /// Tracks ongoing full syncs by site ID to prevent duplicates
    private var ongoingSyncs: Set<Int64> = []
+    /// Tracks ongoing incremental syncs by site ID to prevent duplicates
+    private var ongoingIncrementalSyncs: Set<Int64> = []


I don't know whether we really want to be able to do a full sync and an incremental sync for the same site simultaneously.

Start a full sync that takes 1 minute to download

10s later, start an incremental sync which takes 30s to download

40s after starting, the incremental sync changes are written to the db

1m after starting, the full sync write starts – it deletes everything in the db for that site, and puts new data in.

Let's tackle in a separate ticket, as it's an edge case, but WDYT?

I don't know whether we really want to be able to do a full sync and an incremental sync for the same site simultaneously.

Yea good point, incremental sync should be skipped when a full sync is ongoing. Created an issue WOOMOB-1331.

joshheald · 2025-09-11T11:31:23Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift

+        if ongoingIncrementalSyncs.contains(siteID) {
+            DDLogInfo("⚠️ POSCatalogSyncCoordinator: Incremental sync already in progress for site \(siteID)")
+            throw POSCatalogSyncError.syncAlreadyInProgress(siteID: siteID)
+        }
+
+        guard let lastFullSyncDate = await lastFullSyncDate(for: siteID) else {
+            DDLogInfo("📋 POSCatalogSyncCoordinator: No full sync performed yet for site \(siteID), skipping incremental sync")
+            return
+        }
+
+        if !forceSync, let lastIncrementalSyncDate = await lastIncrementalSyncDate(for: siteID) {
+            let age = Date().timeIntervalSince(lastIncrementalSyncDate)
+
+            if age <= maxIncrementalSyncAge {
+                return DDLogInfo("📋 POSCatalogSyncCoordinator: Last incremental sync for site \(siteID) was \(Int(age))s ago, sync not needed")
+            }
+        }


Probably worth moving this to a shouldPerform function, even if it's only used privately in this class and not exposed.

I think we should probably use the same approach for both – I don't always enjoy the "if applicable/if needed" uncertainty, but then it is the job of this coordinator to manage when syncs happen, so it's not unreasonable.

What do you think are the use cases / benefits of a separate shouldPerform that is public? It'd be useful for the call site to handle the result, but I don't think we plan to show any UI for it. A private shouldPerform function could make the code more readable though.

I think giving both a private shouldPerform is the best approach for now.

We probably don't ever need to make them public, but some possible future reasons:

To log analytics when a background task runs about whether we synced, or why we didn't.

To show the status in settings "will sync" or make different buttons available.

I think giving both a private shouldPerform is the best approach for now.

Sounds good, we can make it public when the use case calls for it. I created an issue WOOMOB-1333 for making this change.

joshheald · 2025-09-11T11:33:18Z

Modules/Sources/Yosemite/Tools/POS/POSCatalogSyncCoordinator.swift

+    private func lastIncrementalSyncDate(for siteID: Int64) async -> Date? {
+        do {
+            return try await grdbManager.databaseConnection.read { db in
+                return try PersistedSite.filter(key: siteID).fetchOne(db)?.lastCatalogIncrementalSyncDate
+            }
+        } catch {
+            DDLogError("⛔️ POSCatalogSyncCoordinator: Error loading site \(siteID) for incremental sync date: \(error)")
+            return nil
+        }
+    }


I changed this, removing the loadSite/updateSite functions from the persistence service, and saving the dates as part of the catalog, not afterwards.

It's worked out to be much simpler IMO, but I wanted to bring it to your attention, so let me know if you think we should go back

RafaelKayumov and others added 9 commits September 10, 2025 15:05

Add CIABEligibilityChecker placeholder

de3d16f

Use temporary site name based logic to detect garden/ciab site

d8ff294

Add full sync date to PersistedSite

90d138e

Combine duplicated MockPOSCatalogPersistenceService

7bb4af6

Use full sync date from site entity

3d9d468

Stop storing full sync date in settings

d54dd2e

Lint fixes

a467350

Add incremental sync support to POSCatalogSyncCoordinator.

616957e

POSCatalogSyncCoordinator: add test cases for `performIncrementalSync…

db3456e

…IfApplicable`.

jaclync changed the base branch from trunk to woomob-1252-sync-coordinator-store-sync-date-on-site September 11, 2025 05:28

jaclync changed the title ~~POSCatalogSyncCoordinator: Add incremental sync functionality~~ [Local Catalog] Add incremental sync functionality to POSCatalogSyncCoordinator Sep 11, 2025

jaclync added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. feature: POS labels Sep 11, 2025

jaclync added this to the 23.3 milestone Sep 11, 2025

Add periphery ignore comment to performIncrementalSyncIfApplicable.

45e7608

jaclync requested a review from joshheald September 11, 2025 06:37

joshheald force-pushed the woomob-1252-sync-coordinator-store-sync-date-on-site branch from a467350 to 0ffe5d7 Compare September 11, 2025 09:20

joshheald self-assigned this Sep 11, 2025

joshheald added 3 commits September 11, 2025 11:41

Merge branch 'woomob-1252-sync-coordinator-store-sync-date-on-site' i…

d5c7feb

…nto feat/WOOMOB-1252-sync-coordinator-incremental-sync

Remove unused persistence service

3cb66b6

Ignore

c6aa59a

Base automatically changed from woomob-1252-sync-coordinator-store-sync-date-on-site to trunk September 11, 2025 11:09

Merge branch 'trunk' into feat/WOOMOB-1252-sync-coordinator-increment…

54ca153

…al-sync

joshheald approved these changes Sep 11, 2025

View reviewed changes

joshheald merged commit c69e785 into trunk Sep 11, 2025
14 checks passed

joshheald deleted the feat/WOOMOB-1252-sync-coordinator-incremental-sync branch September 11, 2025 11:34

jaclync mentioned this pull request Oct 1, 2025

[Local Catalog] Update full/incremental sync to have consistent interface #16187

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Local Catalog] Add incremental sync functionality to `POSCatalogSyncCoordinator` #16117

[Local Catalog] Add incremental sync functionality to `POSCatalogSyncCoordinator` #16117

Uh oh!

jaclync commented Sep 11, 2025 •

edited

Loading

Uh oh!

wpmobilebot commented Sep 11, 2025 •

edited

Loading

Uh oh!

joshheald Sep 11, 2025

Uh oh!

jaclync Sep 12, 2025

Uh oh!

joshheald Sep 11, 2025

Uh oh!

jaclync Sep 12, 2025

Uh oh!

joshheald Sep 12, 2025

Uh oh!

jaclync Sep 12, 2025

Uh oh!

joshheald Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Local Catalog] Add incremental sync functionality to POSCatalogSyncCoordinator #16117

[Local Catalog] Add incremental sync functionality to POSCatalogSyncCoordinator #16117

Uh oh!

Conversation

jaclync commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes

Steps to reproduce

Testing information

Uh oh!

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

Uh oh!

Uh oh!

joshheald Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

jaclync Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Local Catalog] Add incremental sync functionality to `POSCatalogSyncCoordinator` #16117

[Local Catalog] Add incremental sync functionality to `POSCatalogSyncCoordinator` #16117

jaclync commented Sep 11, 2025 •

edited

Loading

wpmobilebot commented Sep 11, 2025 •

edited

Loading