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

[iamgabrielma]

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:

• The GhostableCardView will no longer render indefinitely when we reach the end of the item list
• We stop doing network requests unless we reload the item list, in which case isLastPage resets.

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

Screen.Recording.2024-11-12.at.13.48.32.mov

Steps to reproduce

• With a POS eligible store, for simplicity modify POSConstants.productsPerPage to something like 5 or 10, and load POS
• Scrolling down should show the GhostableCardView while there are more items to fetch
• At the end of the list, the GhostableCardView should disappear
• Pull-to-refresh should reload the 1st page normally, scrolling down should show the GhostableCardView while there are more items to fetch, and stop at the end of the list.
• If we have a store with 1-2 products, the GhostableCardView will also stop as soon as the end of the page is triggered ( you can create a new jurassic ninja site with 1 simple product, or I can send access to a test site if needed 👍 )

---

• 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	pr14382-ec763b3
	Version	21.1
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	ec763b3
	App Center Build	WooCommerce - Prototype Builds #11589

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

[joshheald]

I don't think this is the right approach – it still results in unlimited network requests, and can get the isLastPage decision wrong.

We will keep our views and logic easier to understand and change if we avoid adding flags like isLastPage, or at least avoid using them in views – the itemListState can give everything we need here. It should be either loaded or loading, depending on whether we want to show the ghost cell or not.

If we don't want to show it, but still do a network request, we can just leave it as loaded while that request continues.

I've done some exploration of it and it would work better if we determine whether we're at the end of the list in the service layer, since there it can throw a pageOutOfRange error if we get no products back in the API response. That avoids the client-side filtering having an effect.

We may still need some private state in the aggregate model, but it doesn't need to be published or even available to the view. It should just inform what we publish in the itemListState property.

Give me a shout if you want to pair on it tomorrow, especially if any of this isn't clear. I've got my exploration on a branch but it's messy and I want to focus on the cart and other fixes today, rather than polishing it.

[joshheald]

This seems a bit off...

Why do we need to check what page it's loading if the state is a loading state?

The itemListState shouldn't be any kind of loading if there's nothing left to load; it should just go directly to loaded.

[iamgabrielma]

True that, the isLastPage property shouldn't influence here, and leave the View to just render when state is loading, shouldn't have to know about anything else.

[joshheald]

We should lobby for X-WP-Total and X-WP-TotalPages headers to be passed through the JP tunnel, instead of trying to figure this out for ourselves.

[iamgabrielma]

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.

Screen.Recording.2024-11-14.at.10.58.11.mov

When cleaning up the loadNextItems() method here 8956c01 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.