[Local Catalog] Persist catalog downloads in the background #16342
Conversation
joshheald
changed the base branch from
trunk
to
feat/WOOMOB-1173-background-catalog-download-updated
joshheald
changed the title
|
|
…nto feat/WOOMOB-1173-parse-catalog-downloads-in-the-background
iamgabrielma
left a comment
iamgabrielma
left a comment
There was a problem hiding this comment.
Works well! Added some non-blocking comments/questions below
| throw POSCatalogSyncError.timeout | ||
| } | ||
|
|
||
| public func parseAndPersistBackgroundDownload(fileURL: URL, siteID: Int64) async throws -> POSCatalog { |
There was a problem hiding this comment.
Nit: We can move this signature to a different extension or to the class. I get a warning here: 'public' modifier conflicts with extension's default access of 'private'
| #expect(parsedFileURL?.path == "/tmp/test.json") | ||
|
|
||
| // Cleanup | ||
| BackgroundDownloadState.clear() |
There was a problem hiding this comment.
AFAU this clean up is not needed, as we already perform it at the end of handleBackgroundSessionEvent right?
Not necessarily a suggestion, just something to think about: I wonder if there could be test isolation issues when these run in parallel, or a test crashes/fails before cleanup which could affect subsequent tests since these share UserDefaults.standard
There was a problem hiding this comment.
Not from this PR, but consider making a helper function in this test suite for clean up the temp files and background state, I think it's easy to miss the difference between .removeItem(at: documentsURL), .removeItem(at: tempFile) or when these should be called per test basis.
| // Simulates async behavior | ||
| try await Task.sleep(nanoseconds: 1_000_000) // 1ms | ||
| // Notify test that download has started | ||
| onDownloadStarted?() |
| switch newState { | ||
| case .syncCompleted, .syncFailed, .initialSyncFailed: | ||
| // Sync finished - clear the refreshing state if it was set | ||
| if isRefreshingCatalog { |
There was a problem hiding this comment.
We only toggle the value of isRefreshingCatalog when is user-initiated through refreshCatalog(). If a non-user-initiated background sync completes this always be false, so UI would only update when user-initiated. Is intended for now?
There was a problem hiding this comment.
Yep, that's totally intended. It'd be strange to show a button spinner for something the user didn't do, IMO
| await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in | ||
| withObservationTracking { | ||
| // Access the observed property to register observation | ||
| _ = currentSyncState | ||
| } onChange: { | ||
| // When state changes, resume the continuation | ||
| continuation.resume() | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a bit tricky to wrap my head around, I have a question: When we setup the observation through withCheckedContinuation, and then becomes inactive before reading the new state (let newState = currentSyncState)... could there be state changes that are missed before we loop back to the beginning of the loop?
It was my understanding that when .onChange fires we must callwithObservationTracking again to re-register, so the gap between these calls is not observed. But this could be just a miss-understanding on my part, since the signature does not return any cancellable/subscription object but is just a closure.
There was a problem hiding this comment.
Yeah, it's a little odd. The while loop takes care of re-upping the observation tracking, but I've changed it to make that a little clearer.
Generated by 🚫 Danger |
joshheald
deleted the
feat/WOOMOB-1173-parse-catalog-downloads-in-the-background
branch
Merge after: #16337
Description
This PR moves the handling (parsing and persistence) of background full catalog sync downloads, to be completed in the background. This happens in the 30s that Apple give us after the background completes.
Note than none of this will be live for a while, because we need the Catalog endpoints in Core first.
Test Steps
Backgrounded testing is tricky, and you can't really do it with the debugger attached. I've found network breakpoints are useful.
pointOfSaleCatalogAPIfeature flagPOSLocalCatalogEligibilityService.Constants.defaultCatalogSizeLimitto 100000Tools > Network conditions > +then set the host tolargefuntesting.mystagingwebsite.comand use the preset profile. Note that this is only wanted for the file download, so no need to do it on public-api.wordpress.com. For these settings, 3g takes about 30s to download, and 2g takes about 2 minutes.POS > Settingsand tap refresh catalog/wc/v3/products/catalog, wait for thecompletedstatus response with a download URL (this takes a few minutes)Menu > Settings > Help and Support > Application logs > CurrentClearing catalog data for site 247136746in the background. It's possible to run out of time with this approach to handling downloads, but they get completed when you open the app again.Screenshots
RELEASE-NOTES.txtif necessary.