Order details: Fix crash when reloading orders

[itsmeichigo]

Closes WOOMOB-604

Description

Previously, there was an attempt to fix the crash when reloading the order details screen. This mitigated some of the cases but the same crash still happens sometimes after refunding a cached order and reloading the same order.

The crash is due to the use of computed variables in OrderDetailsDataSource. For the case with refunding, the app would crash in the following scenario:

• User opens the order details screen or pulls to refresh the screen.
• OrderDetailsDataSource determines the sections in reloadSections.
• Refunds are loaded and synced in to the local storage asynchronously.
• The table view reloads data and configures the order items section, but the aggregateOrderItems computed property returns an empty array due to the refund.
  => The app crashes when configuring order items with index out of bounds error.

Screen.Recording.2025-06-17.at.12.35.16.mov

The solution is to freeze all access to the results controllers upon reloadSections to avoid discrepancies when reloading data.

Testing steps

1. Create an order and fulfill it.
2. Open the order on the app to ensure that it's cached into local storage.
3. Refund the same order on the web or on another device.
4. Back to the app, pull to refresh the order details screen.
5. Confirm that the app doesn't crash. The order details should display the correct details.

Since the crash happens depending on the timing of the refund syncing, so ensure to do a few tries to confirm.

Testing information

Tested and confirmed the fix on simulator iPhone 16 iOS 18.4

Screenshots

Screen.Recording.2025-06-17.at.14.57.19.mov

---

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

[wpmobilebot]

📲 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	30577
	Version	PR #15764
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	1737b8e
	Installation URL	36dbp7fdcohp8

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

[RafaelKayumov]

Looks good. I have a few questions.
The change extends the “state” nature of the OrderDetailsDataSource.
I wonder if it’s necessary to also make sure the state refreshed (reloadSections() called) after modifying the order of the OrderDetailsDataSource instance.

For example in:

func update(order: Order) {
    self.order = order
    resultsControllers.update(order: order)
}

and in

func configureResultsControllers(onReload: @escaping () -> Void) {
    resultsControllers.configureResultsControllers(onReload: onReload)
}

I didn't dive to deep. But it feels like those methods should be coupled with the reloadSections()

Also the lookUpProductVariation(...) relies on readonly value taken from resultsControllers.productVariations which is a fetched property and can potentially vary as a result of a race condition.

[jaclync]

(Non-blocking)

Just checking the code changes. Is the crash reproducible before this PR? Are the repro steps different from the one fixed previously? If so, I wonder why it was fixed back then but not anymore.

Using resultsController's properties which are struct type feels promising, but I was wondering if we could try not saving them as variables that can be mutated any time.

[jaclync]

❓ How is this products different from the products below in L1195? Shall we use the same products in the beginning of the function?

[itsmeichigo]

Good point - addressed in dc7f0dc.

[jaclync]

How about also freezing order (and its items among other properties used in the sections) in case the order changes in the middle?

[jaclync]

This probably requires some refactoring, but I was wondering if a crash could still happen when there are two reloadSections at the same time, and because they're async, the second call can overwrite the variables and the first call can use the updated values later on? If we want to freeze the values, it might be better passing them to functions called in reloadSections that depend on them instead of saving them as variables so that these values aren't mutable for the whole reload function.

[itsmeichigo]

I'm struggling to understand your suggestions here, so it'd be nice if you could clarify your suggestions further.

From my understanding, it'd be simpler if the reloadSections method were synchronous - that way, every reload would be subsequent and more predictable. Another refactoring idea is to udpate createPaymentSection to be synchronous and adjust the payment section when the receipt row is available. That could be simpler - WDYT?

[itsmeichigo]

If we want to freeze the values, it might be better passing them to functions called in reloadSections that depend on them instead of saving them as variables so that these values aren't mutable for the whole reload function.

The variables are also accessed when configuring cells, so it cannot be a simple as passing them through to determine sections.

[itsmeichigo]

@RafaelKayumov Regarding your suggestion:

But it feels like those methods should be coupled with the reloadSections()

I think the purpose is to decouple the logic, as the calls are triggered from the higher in the hierarchy: OrderDetailsViewController > OrderDetailsViewModel > OrderDetailsDatasource > OrderDetailsResultsControllers.

The view controllers triggers table view reload when there's a change to the order object:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift

Lines 204 to 212 in c8788ef

	func configureEntityListener() {
	entityListener.onUpsert = { [weak self] order in
	guard let self = self else {
	return
	}
	self.viewModel.update(order: order)
	self.reloadTableViewSectionsAndData()
	}
	}

The view controller also triggers table view reload upon results controller updates:

woocommerce-ios/WooCommerce/Classes/ViewRelated/Orders/Order Details/OrderDetailsViewController.swift

Lines 219 to 221 in c8788ef

	viewModel.configureResultsControllers { [weak self] in
	self?.reloadTableViewSectionsAndData()
	}

Also the lookUpProductVariation(...) relies on readonly value taken from resultsControllers.productVariations which is a fetched property and can potentially vary as a result of a race condition.

Good catch! Moved product variations to a property in dc7f0dc.

[itsmeichigo]

Thanks for taking a look @jaclync.

Just checking the code changes. Is the crash reproducible before this PR? Are the repro steps different from #15633 previously? If so, I wonder why it was fixed back then but not anymore.

The issue back then was 100% reproducible. The issue this PR is trying to address is more of a timing issue. The app crashes only when the refund update comes in the middle of a reload.

I responded to your other questions in the corresponding threads.