Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

pmusolino · 2025-01-28T15:54:57Z

Description

We observed that while opening product details, it's easy to see the following warning, generated by multiple reads on storage (Core Data). Also, this issue sometimes generate a lag opening the Order Detail screen.

This warning is occurring because we're making a lot of different fetch requests when loading the details, causing the hang issue.

I solved the issue by removing two fetches from OrderDetailsResultsControllers:

updating the feeLines property to return the relationship from order.fees instead, of using a Results Controller to fetch the order fees.
I added the shippingLabels property to Order which was connected in Storage with a relationship, but not present inside the Networking model.
- Update Networking.generated.swift, Models+Copiable.generated.swift, and Order.swift to include shippingLabels property in Order struct.
- Update OrderDetailsResultsControllers.swift by removing shippingLabelResultsController and using order.shippingLabels instead.
- Update various files to incorporate shippingLabels property in Order creation and manipulation.
- Add Sendable conformance to ShippingLabel, ShippingLabelAddress, ShippingLabelRefund, ShippingLabelRefundStatus, and ShippingLabelStatus structs and enums for concurrency safety.
- Update tests in OrderDetailsDataSourceTests.swift to handle shippingLabels within Order instances.

Those were the only relationships with Order. In my opinion, we could potentially improve performance further by handling some fetches externally (e.g., fetching all order statuses). However, the current implementation is structured such that each view/model doesn't have visibility beyond the order itself. While this approach is possible, we need to discuss how deep we want to go with such changes. The current solution, however, does address the I/O problem effectively.

Steps to reproduce

Open multiple times different Order Details. You can notice a purple warning mentioning Performing 1/0 on the main thread by reading or writing to a database can cause slow launches. in ResultsController.

Testing information

You should not be able anymore to replicate the warning after these changes.

Make sure you are able to see “Custom amounts” section adding a fee, and that editing the fee update the fee title/amount.
Test that Shipping Labels in Order Details works like before if the plugin is installed: fetching/creation/update.
- Ensure the "Create Shipping Label" button is visible when the order is eligible for shipping label creation and has no existing labels.
- Ensure the "Create Shipping Label" button is not visible when the order is eligible but already has shipping labels.
- Ensure the "More" button is visible in the product section when the order is eligible and has no refunded shipping labels.
- Ensure the "More" button is visible in the product section when the order is eligible and has refunded shipping labels.
- Ensure the "More" button is not visible in the product section when the order is not eligible for shipping label creation.
- Ensure that refunded shipping labels are correctly identified and handled, and that the UI reflects this state appropriately.
- Confirm that the order is correctly marked as eligible for shipping label creation based on predefined criteria (e.g., order status, payment status).
- Ensure that any changes to the shipping labels (e.g., addition, refund) are immediately reflected in the UI without requiring a manual refresh.

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.

…oved `feeLinesResultsController` and related logic

…ge, but not present inside the Networking model. Doing this to reduce number of `ResultsController` from `OrderDetailsResultsControllers`: - Update `Networking.generated.swift`, `Models+Copiable.generated.swift`, and `Order.swift` to include `shippingLabels` property in `Order` struct - Update `OrderDetailsResultsControllers.swift` by removing `shippingLabelResultsController` and using `order.shippingLabels` instead - Update various files to incorporate `shippingLabels` property in Order creation and manipulation - Add `Sendable` conformance to `ShippingLabel`, `ShippingLabelAddress`, `ShippingLabelRefund`, `ShippingLabelRefundStatus`, and `ShippingLabelStatus` structs and enums for concurrency safety - Update tests in `OrderDetailsDataSourceTests.swift` to handle `shippingLabels` within `Order` instances

…ch-fetching-in-order-details

wpmobilebot · 2025-01-28T16:08:40Z

📲 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	pr14999-bbfb18c
	Version	22.0
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`bbfb18c`
	App Center Build	WooCommerce - Prototype Builds #13442

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

selanthiraiyan · 2025-01-29T05:45:01Z

Thanks for working on this, @pmusolino!

Not directly related to this PR: While attempting to test this PR I noticed that the "Add Extension To Store" from the following screen doesn't work. I also noticed that the code is not linked to any action. Code link

Video recording:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-29.at.11.13.48.mp4

selanthiraiyan · 2025-01-29T05:55:43Z

I noticed one more issue that is probably not related to this PR.

I have installed WooCommerce Shipping on my JN store, but I still see the "Get WooCommerce Shipping" banner in the Order Details screen.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-29.at.11.22.44.mp4

I have tried restarting the app (stop and run again from Xcode), but the banner doesn't go away.

selanthiraiyan

The following issue happens in trunk as well and I don't think this is related to this PR. IMO, this looks related to this CUW post - peaMlT-Yo-p2

After creating a shipping label on my JN store I started facing issues with loading orders.

I tried emptying the orders creating Woo Smooth Generator from my JN store and kept only the order that I manually created from the mobile app. But still the mobile app fails to load that order and shows the following decoding error in logs.

⛔️ Error synchronizing orders: typeMismatch(Swift.Int64, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "data", intValue: nil), _JSONKey(stringValue: "Index 4", intValue: 4), CodingKeys(stringValue: "line_items", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "taxes", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "id", intValue: nil)], debugDescription: "Expected to decode Int64 but found a string instead.", underlyingError: nil))

My JN site has the following plugins installed.

Please let me know if you need an invitation to my JN site.

rachelmcr · 2025-01-29T10:56:42Z

Not directly related to this PR: While attempting to test this PR I noticed that the "Add Extension To Store" from the following screen doesn't work. I also noticed that the code is not linked to any action.

FWIW the "Get WooCommerce Shipping" banner with this install prompt is behind the shippingLabelsOnboardingM1 feature flag. I believe that was an older Kiwi project that never shipped; to avoid confusion it's probably worth removing the code or entirely disabling that feature flag.

selanthiraiyan · 2025-01-30T06:42:03Z

@pmusolino While trying to Refund a shipping label, I found that the "Request refund" button doesn't do anything.

selanthiraiyan

@pmusolino 👋

Quoting the following from the "Testing Information" in the PR description

Ensure that any changes to the shipping labels (e.g., addition, refund) are immediately reflected in the UI without requiring a manual refresh.

I noticed that the order detail page doesn't reflect the newly purchased shipping label unless I pull down to refresh. Even after I pull to refresh, I see the "Create Shipping Label" button, and it goes away after another pull down to refresh.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-30.at.12.13.22.mp4

pmusolino · 2025-01-30T12:01:11Z

I noticed one more issue that is probably not related to this PR.
I have installed WooCommerce Shipping on my JN store, but I still see the "Get WooCommerce Shipping" banner in the Order Details screen.

I filled an issue here about removing the feature flag.

pmusolino · 2025-01-30T12:03:07Z

@pmusolino While trying to Refund a shipping label, I found that the "Request refund" button doesn't do anything.

This doesn't work in trunk either because I think it's still a WIP under a feature flag. cc @rachelmcr

pmusolino · 2025-01-30T12:26:05Z

I noticed that the order detail page doesn't reflect the newly purchased shipping label unless I pull down to refresh. Even after I pull to refresh, I see the "Create Shipping Label" button, and it goes away after another pull down to refresh.

I tested it by creating the shipping label from the web, and it works; it's visible in the app when opening the order. I also tried creating the shipping label from the app. In this specific case, the refresh does not work as you mentioned, but it also doesn't refresh in the trunk branch. I've asked in C02KUCFCSFP/p1738239651479579 if this is part of a new flow, since I remember the UI for creating the shipping label was different a while back. In any case, it's not something related to this PR. 👍

selanthiraiyan · 2025-01-31T05:33:27Z

Networking/Networking/Model/Order.swift

@@ -239,6 +245,9 @@ public struct Order: Decodable, Sendable, GeneratedCopiable, GeneratedFakeable {
            return OrderAttributionInfo(metaData: allOrderMetaData)
        }()

+        // Shipping labels
+        let shippingLabels = try container.decodeIfPresent([ShippingLabel].self, forKey: .shippingLabels) ?? []


👋 @pmusolino Do we receive this shipping_labels key in orders response? Based on my testing in a Woo store with shipping labels, I don't see it being sent in wc/v3/orders.

If we do receive the shipping_labels, should we update the mock JSON and add unit tests to ensure that shipping_labels are mapped correctly?

Good question, @selanthiraiyan. In ShippingLabelStore, the method upsertShippingLabels connects the fetched shipping labels to the specific order, so the order doesn't return specific shipping labels, making it unnecessary to decode the Shipping Labels in the Order model. However, declaring the property (without decoding) in the networking model seems necessary to access shippingLabels, unless you have another solution in mind.

Yes, we could access the shipping labels from storage, but that's what we're trying to avoid. Do you think I can leave the shippingLabels property in Order.swift but remove the decoding? This way, by default, it would be an empty array.

Thanks, @pmusolino! I don't have a solution in mind. But I think we should remove the decoding, as it will be misleading.

I'm tagging @itsmeichigo to check whether she has any other solutions.

Thanks for the ping Sharma!

I think if the key for shipping labels is non-existent in the order response, keeping the property an empty array should be the way to go. There's no need to waste more resources to look for such a key to decode. Please also leave a comment in the code to explain why the value is empty by default, and when it's going to be updated.

However, I'm concerned about this part:

In ShippingLabelStore, the method upsertShippingLabels connects the fetched shipping labels to the specific order

In the order details screen, after syncing shipping labels, we'd need to ensure that the order in the view model is updated with the fetched label(s), just so they can be displayed on the UI.

Thank you both! I updated the decoding here 85c0115 the comment should be clear enough.

In the order details screen, after syncing shipping labels, we'd need to ensure that the order in the view model is updated with the fetched label(s), just so they can be displayed on the UI.

Yes, this already happens from my testing. If you create or update a shipping label, or open an order detail with an existing shipping label that needs to be fetched remotely, you will see the order detail UI correctly updated.

wpmobilebot · 2025-01-31T12:41:42Z

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

…ch-fetching-in-order-details-take-2

…ch-fetching-in-order-details

pmusolino · 2025-03-06T12:16:24Z

@selanthiraiyan when I replied in that discussion with Huong, I wasn't aware of the problem mentioned here by Rachel.

Why?
In my testing, I was always pressing "Update" for updating the order after creating a shipping label, while I discovered this is not needed for generating and attaching a shipping label on the web (In fact, I believe it is a web issue because the Update button is enabled even if there are no changes).
Pressing "Update", changes the behavior of the refresh of the shipping labels, and the reason is explained in this Rachel's comment:

When we sync the order, if the order already exists in storage, we use the last modified date to check if it has been updated remotely. (This date doesn't change if shipping labels are added to the order, since those are stored separately in the database and not considered part of the order

So, when you create a shipping label from the web without saving the order, the order's modification date does not update. As a result, everything does not refresh correctly because the shipping label and the order are treated as two separate entities. However, if you refresh the order, the shipping labels will be attached correctly.

When we get the shipping labels from the syncShippingLabels() method, now we need to explicitly attach them to the order using:

if let updatedOrder = self?.order.copy(shippingLabels: shippingLabels) {
    self?.update(order: updatedOrder)
}

Previously, this was unnecessary because we were retrieving data from storage.
Without this, the UI would continue to use the existing order object in memory, which is not updated.

Let me know if I have cleared your doubts, Sharma. 🙌

wpmobilebot · 2025-03-07T04:39:23Z

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

selanthiraiyan · 2025-03-07T08:24:41Z

Thanks for the detailed answer, @pmusolino!

Would it work to sync the shipping labels first before loading the order from the storage?

Send the remote request to fetch shipping labels.
Update the order in storage with the shipping label.
Send a request to retrieve the order.
If the order has not been modified recently, we will still end up loading the latest order with shipping labels from storage.

In the above approach,

We also avoid manually updating the Order in the view layer.
I assume we can also avoid showing the "Create Shipping Labels" momentarily.

wpmobilebot · 2025-03-21T11:56:50Z

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

pmusolino · 2025-03-23T17:58:10Z

@selanthiraiyan Not sure this is the ideal solution because the core of Order Detail is the sync of the order in OrderDetailsViewModel. From there, there is a list of nested operations, including shipping labels. So, the initial info of the order is displayed, and other sections are loaded later. If we first load Shipping Labels, we create a series of dependencies in reverse because we should first check the plugins to see if the shipping labels are supported, then fetch the shipping labels if they are supported, and then fetch the order to retrieve it for the first time or update it. This would also complicate the logic, seemingly with no advantages, and, in fact, it might worsen the experience at the UI level (eg. now there is a bar displaying that the order is still loading, but the main info is already displayed).

selanthiraiyan · 2025-03-24T01:23:11Z

Thanks for the explanation, @pmusolino! I understand. Manually updating the order with the synched shipping labels seems like the way to go. Do you plan to add those changes to this PR?

pmusolino · 2025-03-24T11:42:52Z

@selanthiraiyan if you think the work I did here feat/batch-fetching-in-order-details...feat/batch-fetching-in-order-details-take-2 works, I can merge the changes in this PR 🙌

selanthiraiyan · 2025-03-24T13:46:30Z

I can merge the changes in this PR 🙌

@pmusolino Yes, please go ahead with this. Thanks!

…batch-fetching-in-order-details

pmusolino · 2025-03-26T21:34:52Z

@selanthiraiyan done ✅ waiting for the final approval

selanthiraiyan

Thanks for your patience with this work, Paolo! ❤️

Code

I left a few minor comments and one question about the code. We are getting pretty close to merging this PR. 👏

Testing

I validated that a label purchased from web is shown in the order details page. This test case scenario. ✅
The order details page doesn't reflect a newly purchased shipping label unless I pull down to refresh. Mentioned in this previous comment I tested that this bug exists in trunk as well. ✅

👋 @itsmeichigo It would be great if you could you take a look at this PR as a confidence check? Thanks! 🙇

selanthiraiyan · 2025-03-27T01:06:05Z

WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift

+                    group.leave()
+                }
+                // Check Woo Shipping support first, to ensure correct flows are enabled for shipping labels.
+                self?.dataSource.isEligibleForWooShipping = ((await self?.isWooShippingSupported()) != nil)


We could unwrap self using a guard statement to avoid using self as an optional here and a few other places below.

Done in bbfb18c.

WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift

selanthiraiyan · 2025-03-27T01:22:26Z

WooCommerce/Classes/ViewModels/Order Details/OrderDetailsViewModel.swift

+                self?.dataSource.isEligibleForShippingLabelCreation = isEligible ?? false
+
+                // Sync shipping labels and update order with the result if available
+                if let shippingLabels = await self?.syncShippingLabels() {


Should we call syncShippingLabels even if checkShippingLabelCreationEligibility returned false above? If yes, can we start this request asynchronously without waiting for the response from checkShippingLabelCreationEligibility?

We still need to sync shipping labels just in case the creation eligibility is only updated recently. I moved these calls to a task group to ensure they don't block each other in bbfb18c.

itsmeichigo · 2025-03-27T09:49:11Z

Thanks for the ping @selanthiraiyan! I don't have any additional comment aside from your latest ones.

Regarding this point:

The order details page doesn't reflect a newly purchased shipping label unless I pull down to refresh. #14999 (review) I tested that this bug exists in trunk as well.

I did some debugging, and this seems to be an issue with viewWillAppear not being triggered after dismissing the shipping label creation flow. This was caused by the shipping label creation flow being presented with overFullScreen mode.

I fixed this by switching to fullScreen mode for both the shipping label creation flow and order edit flow in 32dbb12. This ensures that the order is synced again after dismissing these flows. The created shipping labels should now be displayed after dismissing the creation flow.

itsmeichigo · 2025-03-28T03:58:15Z

@selanthiraiyan I added the final changes following your latest comments, please take another look when you can.

selanthiraiyan

Thanks for handling this, Huong! The changes LGTM.

Let's merge this PR. 🚀

pmusolino · 2025-03-28T10:51:43Z

Thank you, Huong, for taking care of the remaining changes! 🙏

pmusolino added 4 commits January 27, 2025 17:47

update: feeLines property to return order.fees instead, while rem…

19def60

…oved `feeLinesResultsController` and related logic

Merge commit 'f12dbd8aeff195fe6da618d23550f9d8d6d24b7f' into feat/bat…

ec15b19

…ch-fetching-in-order-details

update: relese-notes

0311bd9

pmusolino added the feature: order details Related to order details. label Jan 28, 2025

pmusolino added this to the 21.6 milestone Jan 28, 2025

pmusolino requested review from selanthiraiyan and hichamboushaba January 28, 2025 15:54

update: add PR link to release-notes

0ef427b

selanthiraiyan self-assigned this Jan 29, 2025

selanthiraiyan reviewed Jan 29, 2025

View reviewed changes

pmusolino requested review from toupper and rachelmcr January 29, 2025 09:13

selanthiraiyan removed their assignment Jan 29, 2025

selanthiraiyan reviewed Jan 30, 2025

View reviewed changes

selanthiraiyan self-assigned this Jan 30, 2025

pmusolino mentioned this pull request Jan 30, 2025

Remove shippingLabelsOnboardingM1 feature flag #15019

Open

Merge branch 'trunk' into feat/batch-fetching-in-order-details

22d0a31

selanthiraiyan reviewed Jan 31, 2025

View reviewed changes

wpmobilebot modified the milestones: 21.6, 21.7 Jan 31, 2025

pmusolino added 2 commits March 6, 2025 10:34

Merge commit 'd231feff7094c39c56123f4fd2a065bcf459ca6f' into feat/bat…

f84f26e

…ch-fetching-in-order-details-take-2

Merge commit 'd231feff7094c39c56123f4fd2a065bcf459ca6f' into feat/bat…

bc264a9

…ch-fetching-in-order-details

wpmobilebot modified the milestones: 21.9, 22.0 Mar 7, 2025

wpmobilebot modified the milestones: 22.0, 22.1 Mar 21, 2025

pmusolino added 3 commits March 26, 2025 22:30

Merge branch 'feat/batch-fetching-in-order-details-take-2' into feat/…

85530e1

…batch-fetching-in-order-details

Merge branch 'trunk' into feat/batch-fetching-in-order-details

58fdc9b

update: release-notes

e6278c3

pmusolino requested a review from selanthiraiyan March 26, 2025 21:34

selanthiraiyan reviewed Mar 27, 2025

View reviewed changes

Fix issue triggering order syncing after dismissing modals

32dbb12

itsmeichigo added 2 commits March 28, 2025 10:52

Fix unit test build failures

5bc56ac

Improve syncing of shipping label details

bbfb18c

itsmeichigo requested a review from selanthiraiyan March 28, 2025 03:58

selanthiraiyan approved these changes Mar 28, 2025

View reviewed changes

itsmeichigo merged commit 02c2794 into trunk Mar 28, 2025
13 checks passed

itsmeichigo deleted the feat/batch-fetching-in-order-details branch March 28, 2025 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

pmusolino commented Jan 28, 2025

wpmobilebot commented Jan 28, 2025 •

edited

Loading

selanthiraiyan commented Jan 29, 2025

selanthiraiyan commented Jan 29, 2025 •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

rachelmcr commented Jan 29, 2025

selanthiraiyan commented Jan 30, 2025

selanthiraiyan left a comment •

edited

Loading

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

selanthiraiyan Jan 31, 2025

pmusolino Feb 3, 2025

selanthiraiyan Feb 4, 2025

itsmeichigo Feb 4, 2025 •

edited

Loading

pmusolino Feb 5, 2025

wpmobilebot commented Jan 31, 2025

pmusolino commented Mar 6, 2025

wpmobilebot commented Mar 7, 2025

selanthiraiyan commented Mar 7, 2025

wpmobilebot commented Mar 21, 2025

pmusolino commented Mar 23, 2025

selanthiraiyan commented Mar 24, 2025

pmusolino commented Mar 24, 2025

selanthiraiyan commented Mar 24, 2025

pmusolino commented Mar 26, 2025

selanthiraiyan left a comment

selanthiraiyan Mar 27, 2025

itsmeichigo Mar 28, 2025

selanthiraiyan Mar 27, 2025

itsmeichigo Mar 28, 2025

itsmeichigo commented Mar 27, 2025

itsmeichigo commented Mar 28, 2025

selanthiraiyan left a comment

pmusolino commented Mar 28, 2025

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Improve data fetching in Order Details, to avoid I/O performance on the main thread #14999

Conversation

pmusolino commented Jan 28, 2025

Description

Steps to reproduce

Testing information

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

wpmobilebot commented Jan 28, 2025 • edited Loading

selanthiraiyan commented Jan 29, 2025

selanthiraiyan commented Jan 29, 2025 • edited Loading

selanthiraiyan left a comment • edited Loading

Choose a reason for hiding this comment

rachelmcr commented Jan 29, 2025

selanthiraiyan commented Jan 30, 2025

selanthiraiyan left a comment • edited Loading

Choose a reason for hiding this comment

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

pmusolino commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsmeichigo Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Jan 31, 2025

pmusolino commented Mar 6, 2025

wpmobilebot commented Mar 7, 2025

selanthiraiyan commented Mar 7, 2025

wpmobilebot commented Mar 21, 2025

pmusolino commented Mar 23, 2025

selanthiraiyan commented Mar 24, 2025

pmusolino commented Mar 24, 2025

selanthiraiyan commented Mar 24, 2025

pmusolino commented Mar 26, 2025

selanthiraiyan left a comment

Choose a reason for hiding this comment

Code

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsmeichigo commented Mar 27, 2025

itsmeichigo commented Mar 28, 2025

selanthiraiyan left a comment

Choose a reason for hiding this comment

pmusolino commented Mar 28, 2025

wpmobilebot commented Jan 28, 2025 •

edited

Loading

selanthiraiyan commented Jan 29, 2025 •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

selanthiraiyan left a comment •

edited

Loading

itsmeichigo Feb 4, 2025 •

edited

Loading