Skip to content

Conversation

samiuelson
Copy link
Contributor

@samiuelson samiuelson commented Oct 2, 2025

WOOMOB-1412

Description

The goal of PR is to improve the reliability of the full sync of POS local catalog.
The full sync is scheduled to be performed every 24h, at night. However, in case the device was turned off at night, the sync may not succeed. To handle such case, this PR adds:

  1. Keeping track of the last full sync timestamp
  2. Checking if the last full sync is overdue (7 days)
  3. Scheduling one-time, instant full sync in case the full sync was never done before, or data is missing (WooPosProductEntity is empty)

⚠️ Do not merge until the base branch is trunk.

Steps to reproduce

The full sync should be performed in a blocking way before allowing access to the POS for the first time (or in case product data is missing)

Testing information

  • Wipe the PosProductsEntity table and then verify that the full sync occurs when entering POS.

The tests that have been performed

Above.

Images/gif

Screen_recording_20251007_181723.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@samiuelson samiuelson changed the title Woomob 1412 woo poslocal catalog full sync overdue handling [Woo POS][Local Catalog] Full sync overdue handling Oct 2, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 2, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit33cfa10
Direct Downloadwoocommerce-wear-prototype-build-pr14678-33cfa10.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 2, 2025

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

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit33cfa10
Direct Downloadwoocommerce-prototype-build-pr14678-33cfa10.apk

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2025

Codecov Report

❌ Patch coverage is 28.87701% with 133 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.26%. Comparing base (8956252) to head (3e747f4).

Files with missing lines Patch % Lines
...woopos/localcatalog/WooPosFullSyncStatusChecker.kt 4.44% 43 Missing ⚠️
...ocalcatalog/WooPosPerformInstantCatalogFullSync.kt 49.27% 33 Missing and 2 partials ⚠️
...erce/android/ui/woopos/home/WooPosHomeViewModel.kt 37.50% 19 Missing and 1 partial ⚠️
...os/util/datastore/WooPosSyncTimestampRepository.kt 0.00% 13 Missing ⚠️
...os/localcatalog/WooPosLocalCatalogSyncScheduler.kt 0.00% 8 Missing ⚠️
...commerce/android/ui/woopos/home/WooPosHomeState.kt 33.33% 6 Missing ⚠️
.../store/pos/localcatalog/WooPosLocalCatalogStore.kt 0.00% 4 Missing ⚠️
...oopos/util/datastore/WooPosSyncTimestampManager.kt 0.00% 3 Missing ⚠️
...mmerce/android/ui/woopos/home/WooPosHomeUIEvent.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #14678      +/-   ##
============================================
- Coverage     38.27%   38.26%   -0.02%     
- Complexity    10010    10018       +8     
============================================
  Files          2118     2120       +2     
  Lines        118488   118668     +180     
  Branches      15824    15849      +25     
============================================
+ Hits          45357    45410      +53     
- Misses        68917    69041     +124     
- Partials       4214     4217       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…n-pos-splash' into woomob-1412-woo-poslocal-catalog-full-sync-overdue-handling
@samiuelson samiuelson added the type: task An internally driven task. label Oct 2, 2025
@samiuelson samiuelson added this to the 23.4 milestone Oct 2, 2025
@samiuelson samiuelson requested a review from kidinov October 2, 2025 16:42
@samiuelson samiuelson marked this pull request as ready for review October 2, 2025 16:43
@samiuelson samiuelson requested review from malinajirka and removed request for kidinov October 3, 2025 07:11
@samiuelson samiuelson added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 3, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 3, 2025

1 Error
🚫 This PR is tagged with status: do not merge label(s).
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

@malinajirka malinajirka modified the milestones: 23.4, 23.5 Oct 3, 2025
Base automatically changed from woomob-1405-woo-poslocal-catalog-run-incremental-sync-on-pos-splash to trunk October 3, 2025 11:05
@samiuelson samiuelson marked this pull request as draft October 3, 2025 11:33
@samiuelson samiuelson removed the request for review from malinajirka October 3, 2025 11:33
@samiuelson samiuelson requested a review from malinajirka October 7, 2025 16:27
@samiuelson samiuelson marked this pull request as ready for review October 7, 2025 16:27
@malinajirka malinajirka self-assigned this Oct 8, 2025
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @samiuelson!

I'm not entirely sure what is in scope of this PR and what is out of scope. I'll list issues I encountered, feel free to ignore those that are out of scope.

  1. When we are doing an initial full sync, the blocking screen should be full screen (part of splash screen):
POS (51)

instead of the current:
Screenshot_20251008_082416

  1. I noticed that currently during the full sync, the blocking layer is semi-transparent, so I can see products already loaded below the loading state:
Screenshot_20251008_081437
  1. I can see loading skeletons during an incremental sync - the incremental sync shouldn't be visible to the user as it goes against the speed we are trying to achieve.
Screen.Recording.2025-10-08.at.8.16.52.mov
  1. Not sure if it's an issue, but I can see the loading skeletons for 0.5seconds even when I turn off the feature flag even though we load the items during the splashscreen.

  2. The POS is not accessible when there is no internet connection even though I have data in the database + I can see the data behind the error:

Screenshot_20251008_081814

Let me know what you think.

}

private fun isFullSyncOverdue(lastSyncTimestamp: Long): Boolean {
val currentTime = System.currentTimeMillis()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 This code doesn't seem testable, we might want to inject time provider.

when (workInfo?.state) {
WorkInfo.State.SUCCEEDED -> {
val completedTimestamp = syncTimestampManager.getFullSyncLastCompletedTimestamp()
if (completedTimestamp != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ How can this scenario happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@malinajirka 👋 Most common case I can think of - completedTimestamp can be null here, for example, in case the site was switched during full sync. The completedTimestamp would be null for the newly selected site, even though the worker would finish full sync with success for the previous site. WooPosSyncTimestampRepository handles values specific to the currently selected site.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think - maybe we should schedule sync workers with names unique to the currently selected site? This way we could make sure at POS splash screen, that the active worker is syncing data of the currently selected site. Wdyt, @malinajirka ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samiuelson I'm up for it, but it might not be trivial I think - the worker should likely be scheduled in app startup and the user might not be logged in yet. I'd be a bit worried we might end up in a situation where we have a bunch of workers waking up the device too often.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. In that case, relying on getFullSyncLastCompletedTimestamp to verify if the completed sync was related to the currently selected store should be good enough, IMO.

private suspend fun FlowCollector<WooPosFullSyncState>.monitorOneTimeWorkerProgress() {
emit(WooPosFullSyncState.InProgress)

val finalWorkInfo = syncScheduler.observeOneTimeWorkInfo()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ It seems the worker as well as this code runs in a coroutine (not main thread) => are we guaranteed that between

val isOneTimeRunning = syncScheduler.observeOneTimeWorkStatus().first()

and

val finalWorkInfo = syncScheduler.observeOneTimeWorkInfo().filter { workInfo -> workInfo?.state?.isFinished == true } .first()

the worker doesn't finish? I'm not sure, but I have a gut feeling this code might lead to a deadlock.


I think we could fix it by completing this observer also when workInfo is null (aka if no work is in progress anymore). Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I tried improving it in 6190d97

@samiuelson samiuelson marked this pull request as draft October 15, 2025 13:33
samiuelson and others added 7 commits October 15, 2025 15:45
…overdue-handling

Resolved conflicts:
- WooPosLocalCatalogSyncWorker.kt: Combined timestamp storage with incremental sync
- WooPosLocalCatalogSyncScheduler.kt: Merged all required coroutine imports

Fixed compilation errors:
- WooPosItemsScreen.kt: Updated to use WooPosErrorScreenButtonState instead of private Button component

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@samiuelson samiuelson marked this pull request as ready for review October 16, 2025 12:56
@samiuelson
Copy link
Contributor Author

  1. When we are doing an initial full sync, the blocking screen should be full screen (part of splash screen):
  2. I noticed that currently during the full sync, the blocking layer is semi-transparent, so I can see products already loaded below the loading state:

That was fixed 👍

  1. I can see loading skeletons during an incremental sync - the incremental sync shouldn't be visible to the user as it goes against the speed we are trying to achieve.
  2. Not sure if it's an issue, but I can see the loading skeletons for 0.5seconds even when I turn off the feature flag even though we load the items during the splashscreen.

Let me improve 3. and 4. in a separate PR, as this one grew too big—sorry for that, BTW! I created task for this: WOOMOB-1525

  1. The POS is not accessible when there is no internet connection even though I have data in the database + I can see the data behind the error:

Good catch. This was fixed 👍

Thank you for the review, @malinajirka! I addressed your comments. Also, sorry for the long PR I failed to keep it within a limit.

@samiuelson samiuelson requested a review from Copilot October 16, 2025 13:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@wpmobilebot wpmobilebot modified the milestones: 23.5, 23.6 Oct 17, 2025
@wpmobilebot
Copy link
Collaborator

Version 23.5 has now entered code-freeze, so the milestone of this PR has been updated to 23.6.

@samiuelson samiuelson marked this pull request as draft October 17, 2025 15:12
@samiuelson samiuelson marked this pull request as ready for review October 17, 2025 15:14
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @samiuelson for iterating on the PR! I've reviewed the code and it all looks good to me. However, I've encountered a couple issues when testing the PR. I'd consider merging it as is and fixing the bugs in a separate PR, wdyt?

Infinite splash screen

  1. Downgrade your site to WooCommerce 10.2 or older
  2. Clean apps data
  3. Login
  4. Enter POS
  5. Notice catalog is syncing -> notice /variations requests fail since the endpoint isn't available in v10.2 => the logs mention Worker result RETRY which isn't properly handled (it goes again into ENQUEUED state on Retry).
Screenshot 2025-10-18 at 19 13 12

POS Opened with empty catalog

  1. Clean apps data
  2. Login
  3. Turn off wifi/data
  4. Open POS
  5. Notice expected error
  6. Tap on retry
  7. Turn on wifi/data
  8. Tap on retry
  9. Notice you are allowed to enter POS before the initial sync completes.
My.Movie.mp4

startTime: Long
) = FlowCollector<WooPosLocalCatalogInitialFullSyncState> { syncState ->
when (syncState) {
is WooPosLocalCatalogInitialFullSyncState.Ready -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 np: I'd consider calling this NotRequired instead of Ready. At first I thought Ready means that the sync is about to start.

abstract suspend fun getProduct(localSiteId: LocalId, remoteId: RemoteId): WooPosProductEntity?

@Query("SELECT COUNT(*) FROM PosProductEntity WHERE localSiteId = :localSiteId")
abstract suspend fun getProductCount(localSiteId: LocalId): Int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📓 I think this is fine for the current use case, but we need to be careful, since getProductCount returns count of all products including those which are not supported by the POS (hidden in the POS).

wooPosLogWrapper.e("Full sync check failed: No site selected")
return WooPosFullSyncRequirement.Error("No site selected")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 I'm wondering if we need to check isPeriodicSyncEnabledForSite here as well. Otherwise, this class might require sync while the worker always finishes with failure on line WooPosLocalCatalogSyncWorker::54. Wdyt?

P.S. Feel free to create a separate ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: do not merge Dependent on another PR, ready for review but not ready for merge. type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants