🗒️ for reviewer: this case should not be reached if the pagination headers are available. When the pagination headers are missing, this "page out of range" case indicates that there there are no more data to load and thus return empty items and hasNextPage: false.

Thanks. If we want to keep this approach (rather than throwing when the header isn't available/an int) then let's add this comment in the code :)

I think it would also be reasonable to make the headers required though. All tunnelled requests will have them, and I think it's a really old header. We could check whether it's possible to have a site-credential connection without getting these headers back – first step would be to check WC3.5 as I think that's the minimum.

🗒️ for reviewer: sut.loadInitialItems() was added in a few test cases before sut.loadNextItems(), because the latest page number tracking has been moved from the items controller to the pagination tracker AsyncPaginationTracker. Before this move, the page number is already 1 by default in the items controller without actually having fetched the first page. When calling loadNextItems, it increments the page number to 2 and the mock item provider appends the page 2 items to the page 1 items and returns all these items for the page 2 request:

This behavior doesn't seem expected, as the item provider just returns the items for a given page number without any state from previous pages. This PR removes this behavior, but please let me know if I misunderstood the expected behavior of item provider.

Yeah that seems better

🗒️ for reviewer: similar to another comment https://github.com/woocommerce/woocommerce-ios/pull/14728/files#r1891180720, the "page out of range" error was removed because receiving empty data in > 1 page number is how we tell we've reached the last page when pagination headers are unavailable. In this scenario, I think it makes sense to transition the state to loaded.

Yep, agreed. The "error" was a hack so that we could figure out when we'd gone too far and should stop requesting extra pages – it shouldn't be needed now that we have the total pages header.

Is it worth adding a ResponseHeader struct and keeping them in an array?

Right now it's a dictionary for O(1) lookup, did you mean to convert it to convert the key-value pairs to an array? Or having a struct with a dictionary field?

I meant something like

struct ResponseHeader {
    let name: String
    let value: String
}

public protocol Network {
    func responseDataAndHeaders(for request: URLRequestConvertible) async throws -> (Data, [ResponseHeader])
}

Not a strong requirement, just wondered whether you thought it would make the code clearer. I didn't really consider performance, but the array should be small.

Personally, I feel like [String: String] is pretty similar to the key-value pairs array both with the need to look up and optional return value, and with guaranteed performance. Maybe we can keep it simple with a dictionary for now, and update to a struct if we have more use cases of the headers.

Should this use compactMap?

Should we throw if it's not there, rather than make it optional? I don't know how well we'll be able to handle the list if we don't have the page count...

Hm good question, I was mostly afraid that some plugins or hosts (for site credentials login with direct site requests) might alter the headers and remove the pagination headers. In this case where the pagination headers are missing, I'm not sure if it's worth blocking the merchants from using POS because of the missing headers. WDYT?

I think it depends...

1. What complexity does any workaround add to the code? (If we have to keep the previous workaround in, that's a strong sign we shouldn't do it.)
2. What kind of experience will we give them if the headers aren't present? (If it's just going to keep trying to load the next page, that's not great either, especially if those calls are rapidly repeated which is a risk here.)
3. How likely is it that the headers aren't present?

The docs for v3 of the API say:

The total number of resources and pages are always included in the X-WP-Total and X-WP-TotalPages HTTP headers.
Which is repeated back to v1 of the API – WC 2.6 got that, and we don't support that version.

I think we can rely on them being there, but perhaps we could add some tracking now for any request that comes back without the headers, if you have concerns?

What complexity does any workaround add to the code? (If we have to keep the previous workaround in, that's a strong sign we shouldn't do it.)

Great points on the various considerations and tradeoffs. I think the current workaround is not very complicated even though not ideal - it's mostly in the remote loadProductsForPointOfSale returning true for hasMorePages by default, and PointOfSaleProductService returning false when the page items are empty (there's room for optimization there to return false for items count < page size). Having dealt with all kinds of alterations in the API response by third-party plugins, I don't know about the likelihood but there's a chance some plugin or hosting provider strips some response headers. Of course there could be none, we can add tracking to the response headers missing case and observe. If it's almost zero, let's move to throw an error when response headers are missing to simplify the code. I created an issue to track this #14737.

What kind of experience will we give them if the headers aren't present? (If it's just going to keep trying to load the next page, that's not great either, especially if those calls are rapidly repeated which is a risk here.)

There should be up to 1 extra API request per load session with the current fallback implementation (before the next reload), when we fix the infinite scroll issue. If we implement the optimization to return false for response items count < page size, then the probability of extra API request is lower (only if the last page has items size = page size).

I think you can remove the constant here and just use the default initialiser

Makes sense, updated in 27e2f91

How would we get here while it was syncing? Is it impossible?

If loadNextItems is called again while the previous call is still loading items remotely, it reaches this state.

With the suggestion to gate calls to loadNextItems, that shouldn't happen, but it's good to handle it in case the caller isn't doing that.

Good catch... though could/should this be inside the itemsController?

Yup better not clutter the aggregate model, moved in 2d34806

I'm looking a bit closer at this now that it's breaking our unit tests... could you explain again what the problem was here? Was the action cancelled when you let go of the control?

That shouldn't really happen – the refreshable modifier accepts an async function and should work with it... I'd expect it to cancel the task if the view goes off screen. Breaking the async chain by dispatching to a task seems like it causes issues – when you release the list the spinner disappears immediately becaus the reload function returns before the network response comes back

Hmmm I think I can see what's going on now. The error we're getting is explicitlyCancelled – it looks like we're doing that by causing the view to redraw within the reload function. We call itemListStateSubject.send(.loading(allItems)) when it's called, which redraws the view to show the loading state at the top... but doing that redraw means that SwiftUI cancels the refresh control.

If we remove that line, the refresh works as you'd expect, but without the loading cell at the top. To be honest, I think that's a better trade off for the time being.

The problem is that we don't have fine-grained state updates yet – every time we update the POSModel we redraw everything.

I think we would be permitted to change the list content without the refresh action getting cancelled, but because we don't have Observation yet our changes result in the list itself being redrawn, including the refresh control.

Yes the explicitlyCancelled cancelation that results in error state is the issue. Thanks for the better solution by not triggering redraw in 1fb8df0! From my testing with stores with 2 POS products in your PR #14727, I occasionally still saw this error but only briefly because the pre-existing issue #14735 where loadNextItems is also triggered when pulling to refresh a short list (this request isn't canceled and the items from this request update the items view state). Would be great to use Observation to fix this issue properly.

Did you consider/try passing the syncFunction in through init?

At the call site, the repeated trailing closures feel repetitive, at least for the current example. As long as the sync functions we call are async throws, we can set our state accordingly before and after the calls from ItemListController.

I considered passing the sync function through init, but the sync involves states in self (allItems at least) so it cannot be passed in the initializer. I agree it feels repetitive passing for each method. We could consider updating this function to be static functional without any states, but that requires some refactoring and I'd prefer leaving it for later.

Ah yeah, of course. OK, let's keep it like this for now, I don't think it'll be realistic to make it static, because then the consumer will be really limited about what it can do with the state it stores, and testing will be a pain.

I had trouble with defer working as I expected when I used it like this in the ItemsListController – are you sure it gets called if we throw an error? Should it be?

Yes, if this defer statement is commented out, loadNextItems_after_itemProvider_throws_error_then_the_same_page_is_requested_next fails from the page not being marked as synced. unmarkAsBeingSynced was originally called after the do/catch, but when an error is thrown it's never reached. Moving it to defer gets called in both cases.

From the Apple doc:

The statements within the defer statement are executed no matter how program control is transferred. This means that a defer statement can be used, for example, to perform manual resource management such as closing file descriptors, and to perform actions that need to happen even if an error is thrown.

Great, thanks for checking. Goodness knows what I was doing wrong before then... perhaps I accidentally had it in the wrong scope or something.

Yeah that seems better

Yep, agreed. The "error" was a hack so that we could figure out when we'd gone too far and should stop requesting extra pages – it shouldn't be needed now that we have the total pages header.

Thanks. If we want to keep this approach (rather than throwing when the header isn't available/an int) then let's add this comment in the code :)

I think it would also be reasonable to make the headers required though. All tunnelled requests will have them, and I think it's a really old header. We could check whether it's possible to have a site-credential connection without getting these headers back – first step would be to check WC3.5 as I think that's the minimum.

It's been there a while... but this function is feeling a bit strange now.

It's called fetchItems, which it does, but it doesn't return them, it returns a Bool.

If I didn't have the doc comment, I'd expect that to indicate success/failure of the fetch, not whether there are more pages. It's not a huge deal on this private function but it's niggling at me a bit.

We can leave it for now, this one will change more when the state model changes, but it'd be good to know your thoughts. I guess just returning the whole PagedItems actually would make more sense...

This is the function called from the pagination tracker to sync items given a page number, which just requires a boolean return value that indicates whether there are more pages. Its usage/meaning does feel unintuitive without the doc comment.

I think it looks awkward because the items and pagination state are stored separately, but both came from one network request. I tried making the sync function stateless as in #14728 (comment) by passing the sync function try await itemProvider.providePointOfSaleItems(pageNumber: $0).hasMorePages in init, but then the controller cannot get the items easily. I'll think more about how the items and pagination states can be managed more cleanly, feel free to share any ideas as well.

Other than changing the function to return PagedItems<POSProduct>, which includes both the items and whether there are pages, I don't have any ideas.

Let's keep it this way; a better one might become obvious after we use it for variations.

You'll also need to make hasNextPage a private(set) var as well.

Updated in a2e792a, this should fix the flickering issue when loading the next page from the infinite scroll.