[Woo POS][Local Catalog] Check catalog size in sync coordinator #16119

joshheald · 2025-09-11T13:13:24Z

Description

This PR adds a 1000 item limit for full and incremental syncs.

When we check whether we can perform a sync, we perform two minimal GET requests (products and variations, 1 item each, ids only.)
We sum those, and if they're over 1000 we don't return shouldPerform (for full syncs) and block incremental syncs.

Originally I used HEAD requests, which work for site credentials but for some reason don't make it through our networking stack with Jetpack.

Steps to reproduce

Launch the app and switch to a large catalog site
Observe that syncs don't occur, and the reason is logged.
Switch to a smaller site
Observe that syncs happen as normal

Screenshots

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

wpmobilebot · 2025-09-11T13:22:58Z

📲 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	`pr16119-cea0e8e`
	Version	`23.2`
	Bundle ID	`com.automattic.alpha.woocommerce`
	Commit	`cea0e8e`
	Installation URL	5srtg2g8s3ii8

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

joshheald · 2025-09-11T13:47:52Z

@jaclync Sorry this is so long, it's a bit ridiculous for checking two numbers. I could remove some tests, but perhaps it's better to keep them.

jaclync

❓ A high-level question: in the sync coordinator, how about fetching the eligibility state async, caching it throughout the app session (until logout or app relaunch), and exposing it so that the POS item data source can determine whether to display data from the local catalog or the pre-existing remote approach? This way, the local catalog feature availability is consistent, and we don't have to handle edge cases when the availability changes from any of the conditions in the middle of any catalog tasks. Merchants also have the same POS UX throughout the app session.

In the PR, the eligibility check is made for each full/incremental sync trigger. If the store was first eligible (for the catalog feature) with full sync completed, then store size grew to be ineligible in the next sync, adding in the variable that the merchant could be using POS at the moment, it felt like there could be quite many scenarios to handle. It also adds the number of network requests to check the catalog size + remote feature flag as a secondary consideration.

WDYT?

The rest of the PR changes (Networking and POSCatalogSizeChecker + tests) look good 👍

jaclync · 2025-09-12T02:06:56Z

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

+public protocol POSCatalogSizeCheckerProtocol {
+    /// Checks the size of the remote catalog for the specified site
+    /// - Parameter siteID: The site ID to check catalog size for
+    /// - Returns: The size information of the catalog
+    /// - Throws: Network or parsing errors
+    func checkCatalogSize(for siteID: Int64) async throws -> POSCatalogSize
+}
+
+/// Implementation of catalog size checker that uses the sync remote to get counts
+public struct POSCatalogSizeChecker: POSCatalogSizeCheckerProtocol {


nit: the catalog size checker seems like an internal tool within Yosemite, do you foresee this being used in the app layer?

Yes. For example, we'll want to check the size when we decide whether to use the existing approach or the local catalog, which needs to be decided in the app layer.

Given that there are other sync/async conditions for the local catalog feature as in p1757587795454369/1757587788.433699-slack-C070SJRA8DP, I thought we might have another high-level checker for all the conditions (and implement caching if we decide to). So that this class is for internal use in Yosemite. WDYT?

jaclync · 2025-09-12T02:19:34Z

Modules/Tests/YosemiteTests/Mocks/MockPOSCatalogSyncRemote.swift

+    private(set) var getProductVariationCountCallCount = 0
+    private(set) var lastVariationCountSiteID: Int64?
+    var getProductVariationCountResult: Result<Int, Error> = .success(0)
+    var variationCountDelay: UInt64 = 0


nit: are variationCountDelay and productCountDelay used in tests?

joshheald · 2025-09-12T05:50:46Z

Thanks for the review @jaclync – please could you mark it as approved though? I think any caching would need to happen outside this PR because it's already huge.

❓ A high-level question: in the sync coordinator, how about fetching the eligibility state async, caching it throughout the app session (until logout or app relaunch), and exposing it so that the POS item data source can determine whether to display data from the local catalog or the pre-existing remote approach? This way, the local catalog feature availability is consistent, and we don't have to handle edge cases when the availability changes from any of the conditions in the middle of any catalog tasks. Merchants also have the same POS UX throughout the app session.

In the PR, the eligibility check is made for each full/incremental sync trigger. If the store was first eligible (for the catalog feature) with full sync completed, then store size grew to be ineligible in the next sync, adding in the variable that the merchant could be using POS at the moment, it felt like there could be quite many scenarios to handle. It also adds the number of network requests to check the catalog size + remote feature flag as a secondary consideration.

WDYT?

I think we could cache the eligibility state, but I think an app session might be too long. Especially for a POS where the iPad may be set to never sleep (Povilas has experience of companies doing this from previous work.)

I agree there are some edge cases around the catalog size growing. I'm just not sure we that solve them all perfectly is a good use of time, for this temporary limit. It'd still be confusing with the cache, just at a different time.

Adding a cache usually adds different edge cases around cache invalidation and expiry. The onboarding state cache is an example of one which appears to work well. In onboarding checks, we ignore the cached value if it's not success... I believe this was to minimise the edge case handling.

If we add a cache, it should probably cover all the criteria for eligibility in the same place – at the moment, that will either require a refactor, or putting the cache in the app layer. It can still use this size checker in Yosemite though, and the more of that business logic we can move to Yosemite, the better, IMO.

jaclync

Thanks for the review @jaclync – please could you mark it as approved though? I think any caching would need to happen outside this PR because it's already huge.

Yup makes sense, just finished reviewing the coordinator changes.

I think we could cache the eligibility state, but I think an app session might be too long. Especially for a POS where the iPad may be set to never sleep (Povilas has experience of companies doing this from previous work.)

What do you think is a good duration for the same local catalog feature availability? I don't have a good answer myself, just thought that an app session is the simplest in terms of implementation, and the UX stays the same for merchants until they manually relaunch the app or log out.

I agree there are some edge cases around the catalog size growing. I'm just not sure we that solve them all perfectly is a good use of time, for this temporary limit. It'd still be confusing with the cache, just at a different time.

Adding a cache usually adds different edge cases around cache invalidation and expiry. The onboarding state cache is an example of one which appears to work well. In onboarding checks, we ignore the cached value if it's not success... I believe this was to minimise the edge case handling.

If we add a cache, it should probably cover all the criteria for eligibility in the same place – at the moment, that will either require a refactor, or putting the cache in the app layer. It can still use this size checker in Yosemite though, and the more of that business logic we can move to Yosemite, the better, IMO.

I was thinking a simple cache like the GeneralStoreSettings which is used for POS tab visibility caching. During site initialization (app launch in logged-in state / after logging in / after switching stores), it (the high-level eligibility checker) always checks eligibility and caches this value for background tasks. The feature availability remains the same value until the next check. I agree that this cached value should include all the checks.

Feel free to go with the current implementation, and I can work on the high-level eligibility checker (with or without caching depending on the decision) later in WOOMOB-1287.

jaclync · 2025-09-12T06:04:54Z

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

+    ///   - maxAge: Maximum age before a sync is considered stale
+    ///   - maxCatalogSize: Maximum allowed catalog size for syncing (default: 1000)
+    /// - Returns: True if a sync should be performed
+    func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval, maxCatalogSize: Int) async -> Bool


nit: the use case in MainTabBarController still calls the existing shouldPerformFullSync, what use cases do you think will call this new method with maxCatalogSize? If for unit testing, we could consider passing the max size in the coordinator initializer.

Similar question for the performIncrementalSyncIfApplicable method.

It was mostly for unit tests, but yes, those could come in from the co-ordinator. Another potential use case would be making it a setting to allow users to override our limits... but I'll remove these for now.

jaclync · 2025-09-12T06:06:36Z

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

    }

    public func shouldPerformFullSync(for siteID: Int64, maxAge: TimeInterval) async -> Bool {
+        return await shouldPerformFullSync(for: siteID, maxAge: maxAge, maxCatalogSize: 1000)


nit: we could keep the 1000 default as a constant. Same in performIncrementalSyncIfApplicable.

joshheald · 2025-09-12T08:23:46Z

What do you think is a good duration for the same local catalog feature availability? I don't have a good answer myself, just thought that an app session is the simplest in terms of implementation, and the UX stays the same for merchants until they manually relaunch the app or log out.

I don't think it needs to be particularly long. 24h maximum, potentially as low as 1hr, but I'd agree it could also reset for app launch or log out.

I was thinking a simple cache like the GeneralStoreSettings which is used for POS tab visibility caching. During site initialization (app launch in logged-in state / after logging in / after switching stores), it (the high-level eligibility checker) always checks eligibility and caches this value for background tasks. The feature availability remains the same value until the next check. I agree that this cached value should include all the checks.

Feel free to go with the current implementation, and I can work on the high-level eligibility checker (with or without caching depending on the decision) later in WOOMOB-1287.

I'll merge it with the current approach, let's iterate as we go. Thanks!

…inator

joshheald added 3 commits September 11, 2025 12:57

Check catalog size before sync

29370f0

Add tests for checking size before sync

9c2e389

Allow max catalog size to be passed in

323cbe4

joshheald added this to the 23.3 milestone Sep 11, 2025

joshheald 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

Use minimal get requests to get through jetpack

69ef11f

joshheald requested a review from jaclync September 11, 2025 13:47

jaclync reviewed Sep 12, 2025

View reviewed changes

jaclync approved these changes Sep 12, 2025

View reviewed changes

joshheald added 2 commits September 12, 2025 09:33

Don’t expose max catalog size

61a7497

Merge branch 'trunk' into woomob-1252-check-eligibility-in-sync-coord…

cea0e8e

…inator

joshheald enabled auto-merge September 12, 2025 08:39

joshheald merged commit 710519d into trunk Sep 12, 2025
14 checks passed

joshheald deleted the woomob-1252-check-eligibility-in-sync-coordinator branch September 12, 2025 08:59

[Woo POS][Local Catalog] Check catalog size in sync coordinator #16119

[Woo POS][Local Catalog] Check catalog size in sync coordinator #16119

Uh oh!

Conversation

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

Uh oh!

Description

Steps to reproduce

Screenshots

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 commented Sep 11, 2025

Uh oh!

jaclync left a comment

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!

jaclync Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

joshheald commented Sep 12, 2025

Uh oh!

jaclync left a comment

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 commented Sep 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshheald commented Sep 11, 2025 •

edited

Loading

wpmobilebot commented Sep 11, 2025 •

edited

Loading