[Woo POS] Workaround for refreshable task being canceled by redraw

[jaclync]

Closes: #14853
Closes: #14860

Description

There is a known issue initially from Previous discussion in #14728 (comment), where the refreshable async task (triggered by pull-to-refresh, abbreviated PTR) gets canceled when the view is redrawn from POS aggregate model's state change. For example, when we set the initial loading state before the remote request. The initial state was removed to prevent redraws to fix the PTR error in the loaded state, but this results in issue #14860 where the Retry action has no loading UI.

Before we can refactor POS aggregate model to provide more granular updates for the SwiftUI views (created an issue in the backlog #14869), we want to explore workarounds for the linked issues at the top. Moving refreshable task to a detached Task was one option, but the changes looked quite complicated and hard to test. Alternatively, from this SO answer > Option 1, we can use task associated with a unique ID set in refreshable to preserve the async task. The UI/UX tradeoff is that the refresh control does not stay visible until the async task is complete. I haven't found a way to programmatically display the refresh control in SwiftUI, we might have to implement a custom refreshable view if we want to ensure the refresh control staying visible when awaiting the async task for these workarounds.

Set initial loading state in loadItems:

• WooCommerce/Classes/POS/Controllers/PointOfSaleItemsController.swift: Updated loadRootItems and loadChildItems methods to set the loading state while preserving existing items. [1] [2]

View updates to use task instead of refreshable for the async loadItems:

• WooCommerce/Classes/POS/Presentation/Item Selector/ChildItemList.swift: Added a workaround for refresh control issues by introducing a refreshControlTaskID state and ensuring only one refresh task is allowed at a time. [1] [2]
• WooCommerce/Classes/POS/Presentation/ItemListView.swift: Similar to ChildItemList, added a refreshControlTaskID state to handle refresh control issues and ensure only one refresh task is allowed at a time. [1] [2]

New test cases:

• WooCommerce/WooCommerceTests/POS/Controllers/PointOfSaleItemsControllerTests.swift: Added new test cases to verify that the loading state is correctly set for root and child items while preserving existing items.

Steps to reproduce

Issue 1: PTR while the next page is being loaded

🗒️ If the page size for products is still 100, it's much easier to reproduce this issue by reducing the page size from 100 to 15 or 25 for products here.

• Switch to a store eligible for POS, with more than one page of products and variations in POS
• Go to Menu > Point of Sale
• Scroll toward the bottom of the list to trigger the next page load
• Quickly scroll to the top and pull to refresh --> no fullscreen error should appear
• Repeat the above steps for variations of a variable product

Issue 2: Retry UX

• Switch to a store eligible for POS, with at least one variable product
• Go to Menu > Point of Sale --> items should be loaded
• Simulate a network error by going offline, or using a proxy tool to abort the request
• Pull to refresh --> error UI should be shown
• Simulate a success response for the products request
• Tap Retry --> the loading UI should be shown, and then the loaded state
• Repeat the above steps above for variation item list

Testing information

Screenshots

Simulator.Screen.Recording.-.iPad.Pro.11-inch.M4.-.2025-01-15.at.14.44.14.mp4

---

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

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

• The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
• Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
• Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

[wpmobilebot]

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

	App Name	WooCommerce iOS
	Build Number	pr14870-1db7a63
	Version	21.4
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	1db7a63
	App Center Build	WooCommerce - Prototype Builds #12549

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

[staskus]

It's a tricky issue that goes right in the center of SwiftUI state management.

Would refactoring of the aggregate model make a difference in this case? The view observes itemsViewState which changes before and after making the request, which results in a view reload. Or does ability to provide more granular updates would result in only part of ItemListView updating while leaving the refresh control not affected?

I wonder if we should restructure PointOfSaleItemsController so it would disptach item loading tasks synchronously and wouldn't allow making all these requests all at once. Especially given every single task can update itemsViewState.

Either way, I recognize it's tricky. I gave one suggestion but you may have already considered it.

[staskus]

Would such an approach work? Or does it end up having similar problems as a detached task?

Suggested change

	// Only allows one refresh task at a time now that the refresh control is released on redraw.
	guard refreshControlTaskID == nil else {
	return
	}
	refreshControlTaskID = .init()
	}
	.task(id: refreshControlTaskID) {
	guard refreshControlTaskID != nil else {
	return
	}
	await posModel.loadItems(base: .root)
	refreshControlTaskID = nil
	await Task {
	await posModel.loadItems(base: .root)
	}.value

[joshheald]

This does work, at least from a quick test. Perhaps it's a better workaround for now since it leaves the UI as we'd expect it?

There is some suggestion on Stack Overflow that it could cause hard to diagnose issues, because the refresh won't get cancelled when the view's lifetime ends. I tried to break it, and couldn't, and I think the risk of that with a refresh control is quite low.

[jaclync]

Thanks for suggesting this @staskus! I tested it and it worked on my end, it's a better workaround because the refresh control is shown while awaiting the async task and the changes are simpler. The lack of cancellation when the view lifetime ends is a downside, but given the impact this tradeoff seems small.

[jaclync]

Updated in 0e52b0f.

[joshheald]

Would refactoring of the aggregate model make a difference in this case? The view observes itemsViewState which changes before and after making the request, which results in a view reload. Or does ability to provide more granular updates would result in only part of ItemListView updating while leaving the refresh control not affected?

This is my expectation – the refresh control is on ItemListView's navigation stack, which contains views that depend on the posModel.itemsViewState.itemsStack.root state. The refresh control is above the dependant views in the heirarchy, so as I understand it, the refresh control shouldn't be redrawn when we change that state to loading.

If the container state changed (used up the heirarchy in the PointOfSaleDashboardView, and part of the same struct), then the refreshable would be redrawn, and cancel, but that's what we'd want to happen because that would indicate a different change in state, such that the root list shouldn't show any more.

I'm going to try adopting observation to see if it helps.

In the meantime, I think @jaclync, if you're happy with @staskus' suggested workaround, we should go with that one for now.

[joshheald]

Do we definitely want to delete the sublists here, by setting itemStates to the empty dictionary? I don't think we should do that until the PTR completes, if possible.

[jaclync]

Great catch! I updated it with a test case with some mock provider refactoring to provide any items in 1db7a63.

[jaclync]

It's a tricky issue that goes right in the center of SwiftUI state management.

Would refactoring of the aggregate model make a difference in this case? The view observes itemsViewState which changes before and after making the request, which results in a view reload. Or does ability to provide more granular updates would result in only part of ItemListView updating while leaving the refresh control not affected?

I wonder if we should restructure PointOfSaleItemsController so it would disptach item loading tasks synchronously and wouldn't allow making all these requests all at once. Especially given every single task can update itemsViewState.

Either way, I recognize it's tricky. I gave one suggestion but you may have already considered it.

@joshheald described it well why granular updates should not trigger redraws at the refresh control level in #14870 (comment). Right now, PointOfSaleDashboardView's posModel is changed every time the aggregate model's state changes, which probably triggers a redraw of all subviews that depend on posModel including ItemListView. I tried updating ItemListView to remove the direct dependency on the aggregate model and only take in a state enum & reload closure, but it still didn't work because Self changed.

[jaclync]

I've updated the PR with Povilas' suggested workaround if anyone would like to test again! If not I'll merge it in a few hours.

[staskus]

I retested again, this time not on simulator but on iPad Air M2 18.2 dvice and couldn't break it. 👍

[joshheald]

This is my expectation – the refresh control is on ItemListView's navigation stack, which contains views that depend on the posModel.itemsViewState.itemsStack.root state. The refresh control is above the dependant views in the heirarchy, so as I understand it, the refresh control shouldn't be redrawn when we change that state to loading.

If the container state changed (used up the heirarchy in the PointOfSaleDashboardView, and part of the same struct), then the refreshable would be redrawn, and cancel, but that's what we'd want to happen because that would indicate a different change in state, such that the root list shouldn't show any more.

I'm going to try adopting observation to see if it helps.

I hacked Observation in here. I had to comment a bit of stuff out to do it but most of the POS works fine; enough for testing this, at least.

I added the code back to set the loading state for new calls to loadRootItems, and made some changes to the view hierarchy to make sure that the refreshable isn't redrawn when the root state reloads – see the last commit note for full details.

In short; Observation does seem to fix this problem. I'm going to see whether it also makes animation easier, then look at doing a proper implementation. I'll try to write this up more clearly tomorrow, run out of time today.

[jaclync]

I hacked Observation in here. I had to comment a bit of stuff out to do it but most of the POS works fine; enough for testing this, at least.

Great news! Let's remove the workaround in the refreshable after implementing the Observation framework. It's interesting how we can just use properties now from a look at the branch. It simplifies some of the Combine logic.

Merging this PR now in case the Observation framework changes might take longer as the scope of changes is bigger.