woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
321 stars 113 forks source link

[Woo POS] Stop `GhostableCardView` rendering, and network requests when there are no new items #14382

Closed iamgabrielma closed 1 week ago

iamgabrielma commented 2 weeks ago

Description

This PR makes the GhostableCardView to only rely on isLoading state in order to render, as well as marking the last page when no new unique items are fetched from the remote. This resolves two issues:

Caveat: I think there’s a compromise to be made with our current limitations on eligible products: if there’s a gap in the API response between eligible products we might trigger the “last page” too early (this is easily reproducible if we limit products per page to something like 5, and then there's 5+ non-eligible products in the request).

We can see this in the video below where the GhostableCardView doesn't render for a second if there's a gap, but then we immediately fetch next page, it has items, so loads the rest normally. This should not be a problem in release, as would imply that the merchant has a 100+ product gap that are not eligible between page fetches. This can be improved in the future as needed, but any advice or recommendation of how to handle it better is welcome.

Update: these have been resolved in the latest iteration

https://github.com/user-attachments/assets/4b6fb126-1037-49f9-98bd-bf0eef3ae62f

Steps to reproduce


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:

wpmobilebot commented 2 weeks ago

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

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14382-2113b1b
Version21.1
Bundle IDcom.automattic.alpha.woocommerce
Commit2113b1b9f4452042fc4942b0c8867faee5669b95
App Center BuildWooCommerce - Prototype Builds #11613

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

iamgabrielma commented 1 week ago

Thanks for the review and the pairing @joshheald , this is ready for review when you have the chance. No logic has been changed since yesterday except for this commit, which checks we're not throwing the out of range error if we just happen to have no eligible products at all.

https://github.com/user-attachments/assets/cf2b1579-02ce-47d3-bd62-ae1e5f3f4d82

When cleaning up the loadNextItems() method here https://github.com/woocommerce/woocommerce-ios/pull/14382/commits/8956c01a42484791edcd27bc1c6876c7c2fad267 on my testing I could see the behaviour was exactly the same, however some tests started to fail because expected .empty but received .loaded([]) instead, so I reverted it for now, this can be done separately once the design/arch changes are completed.

iamgabrielma commented 1 week ago

Thanks tidying up things and merging, appreciated!

I didn't write an extra test for the initialPageLoad cases here; perhaps that would be worthwhile to do next week?

Definitely. Logged here: https://github.com/woocommerce/woocommerce-ios/issues/14441

I found a "fun" edge case...If the first page of results doesn't have any products that pass the filter, we show the empty state... but that means we never load page 2 even if it might have something.

Yes, I think we could try triggering a couple of subsequent pages automatically to address this edge case, or perhaps offer a "retry" button that does this. It would be a better experience than getting stuck without knowing why. Logged: https://github.com/woocommerce/woocommerce-ios/issues/14442