ApolloPagination's loadNext throwing `loadInProgress` incorrectly

[mapy1874]

Summary

I have a forward, offset-based list using the library. onAppear of the last item of the list, it will trigger loadNext to fetch more items. If I scroll the screen fast enough sometimes the loadNext will not fetch more items and throw the error since isFetching is true in paginationFetch. From a user perspective, this causes the pagination to stop.

I believe the defer { isFetching = false } in paginationFetch is not executed promptly after watcher.refetch finishes, as adding an additional line of isFetching = false here fix the issue I'm facing.

Version

0.1.0

Steps to reproduce the behavior

See the summary

Logs

No response

Anything else?

No response

[AnthonyMDev]

@Iron-Ham 👀

[Iron-Ham]

thanks for reporting! I'll take a look at this momentarily.

[mapy1874]

Here's a demo app to repro this if that helps: https://github.com/mapy1874/loadInProgressBugDemo

[Iron-Ham]

That's super helpful! Thank you!

[Iron-Ham]

@mapy1874 I think I see the issue.

If I'm understanding this correctly, you'd like to know whether or not the pager is currently fetching in order to show a loading cell/spinner at the tail of the list. In your function where you receive data from the pager:

https://github.com/mapy1874/loadInProgressBugDemo/blob/a226c76e2b7ea1b9b23db39ce6a158da2f3f7dd6/RocketReserver/LaunchListViewModel.swift#L57-L75

Since you're using the GraphQLQueryPager here instead of the AsyncGraphQLQueryPager, a better place to set self.isLoading = false would be in the completion callback of your loadNext: After all, that's when you're guaranteed to know that you're done loading. That doesn't help with your first load, though you could similarly set it at the end of the block above and have a similar result.

I have a few questions that can help in improving this API:

• Would exposing the loading state of the pager publicly have been helpful?
• Why use the GraphQLQueryPager instead of the Async variety when it seems you're in an async context?

[mapy1874]

@Iron-Ham Thanks for your quick response!

For the need, I guess it's less about the loading indicator. The bug was that no pagination would happen after we get the loadInProgress error given that we don't have any retry mechanism. It's about whether or not the pager is currently fetching in order to trigger loadNext after the loading is finished.

The assumption was that when we assign the new list and rendering the items on view, the library should have already done loading. Thus when the last item appears on the view, we are safe to trigger loadNext again.

For your questions:

Would exposing the loading state of the pager publicly have been helpful?

I do see people could get confused by this given it's natural to assume when GraphQLQueryPager publisher emits events with data/error, the loading is complete.

Why use the GraphQLQueryPager instead of the Async variety when it seems you're in an async context?

We started using ApolloPagination months ago, and there was no documentations back then and it seems natural for us to start with GraphQLQueryPager. Also, there are not that many materials on how to use AsyncGraphQLQueryPager in the official document. Thus, I assume people would default to use non-async one

[Iron-Ham]

Totally, that makes sense to me. Adding additional docs around AsyncGraphQLQueryPager may be a good move here – as I can see that it's not obvious that something like this is available.

The assumption was that when we assign the new list and rendering the items on view, the library should have already done loading. Thus when the last item appears on the view, we are safe to trigger loadNext again.

I think what's hard about this assumption is that it's not universally true. Imagine that you were fetching with returnCacheDataAndFetch instead of just fetching from the server. You could receive data twice: once from the cache, and then later from the server. When you receive data from the cache, you are still performing the fetch from the server. In that instance you're not done until:

• GraphQLQueryPager's loadNext's callback fires
• Or, if you're using AsyncGraphQLQueryPager, you continue on from your await.

I'd have to think about how best to improve this – whether that's documentation or API. 🤔

[mapy1874]

Thanks! May I get more context on why we choose to throw a PaginationError in when isFetching == true, instead of queueing the requests up/let it fetch even other threads are fetching?

The current way of throwing an error makes it challenging to work with the "onAppear of last item, try loadNext" scenario, given the cached item will appear andloadNext before the previous fetching finishes. I think one solution would be having the callsites of loadNext handle queueing so sequential execution is guaranteed. Maybe it's better to have the library handle the queueing under the hood?

Also let me know if you think there could be some other ways handling this scenario!

[Iron-Ham]

Before diving in – I think that this change should allow you to set your isLoading value in the callbacks of both fetch and loadNext -- instead of sink. By default, loadNext and loadPrevious fetches aren't performed with optimistic cache data, so that may be sufficient for your case?

---

Thanks! May I get more context on why we choose to throw a PaginationError in when isFetching == true, instead of queueing the requests up/let it fetch even other threads are fetching?

I intentionally didn't allow for parallel execution for a few reasons:

• It's too easy to accidentally trigger identical fetches. loadNext and loadPrevious rely on the pager's internal pagination state. That information isn't committed to the internal value-set until an operation completes. That means that two simultaneous loadNext operations would be fetching the same query, with the same arguments. Allowing for use of the cache-values opens up a can of worms with data consistency, as you could fetch data based on pagination-parameters that are no longer valid.
• I saw a potential use-case for allowing a loadPrevious and loadNext at the same time, but it didn't seem worth the difficulty of implementing it given the niche use-case of a bi-directional pager.

Queued fetches weren't allowed for similar reasons:

• Allowing for queued fetches mask logical errors. If I have an API that is triggering loadNext far more often than it should, all of these fetches would be queued, and the incorrect operation would be masked.
• The underlying AsyncGraphQLQueryPagerCoordinator, which performs all of the pagination, technically does queue all of its messages by virtue of being an actor. What it doesn't do is ensure sequential execution; it will swap to the next message if the currently active message suspends execution. As I understand it (and I'm happy to be told otherwise!) that behavior violates a principal of Swift Concurrency, since it opens up the possibility of permanently suspending execution.
• We would need to manage the queue manually: removing all queued values on reset isn't a big deal, but being able to determine when we should remove invalid queued operations is a bit harder. Let's imagine we have 10 queued loadNext operation, and that after the currently executing loadNext, we receive the final page of data. At that point, we'd have to look at all of the queued operations and manually remove loadNext. If there is a loadPrevious in the queue, that may still be a valid operation.
• fetch and refetch would jump the queue by virtue of calling reset prior to their execution.
• Though – perhaps we can maintain a queue with a limit of one, and that would alleviate issues. Definitely something to explore.

Effectively, what this translates into is an intentional design decision that it's up to the caller to ensure that they aren't calling loadNext and loadPrevious more than they should be.

In the context of GitHub, we do so by having an object which manages infinite-scroll behavior. At its core, it observes a list view's scroll position and triggers a load when we are within a certain distance of the end of the list (e.g., when we are within min(1.5*Vertical_Screen_Size, Some_Value) we trigger a fetch). We're primarily a UIKit app, so our conventions aren't identical to a SwiftUI app – but the gist of how we're using ApolloPagination for simple pagination tasks:

• When we reach our threshold for infinite scroll, we call loadNext/loadPrevious.
• Initial fetches are done via fetch, usually in viewDidLoad.
• Pull-to-refresh fetches are done via refetch – as this guarantees that we show the user new data, and that the pull-to-refresh spinner appears, spins, and disappears when the new data comes in.

---

The current way of throwing an error makes it challenging to work with the "onAppear of last item, try loadNext" scenario

I'll take some some time this week and think on improvements to SwiftUI with an onAppear/task scheme. task encapsulates onAppear, onDisappear, and onChange – with cancellations being automatically forwarded as necessary.

I really appreciate the feedback on this, it's super valuable!

[Iron-Ham]

@mapy1874 I took another look at your project – and realized that this could be rewritten in-place without relying on any changes in the pagination library:

https://github.com/mapy1874/loadInProgressBugDemo/compare/main...Iron-Ham:loadInProgressBugDemo:main

To summarize the changes made:

• (Unrelated) I updated the project target to iOS 17, purely for my convenience in using @Observable instead of ObservedObject.
• LaunchListView changes:
  • I moved the reset-button to a refreshable, so we can pull-to-refresh. Notably, this goes through a different code-path than the initial fetch (it uses refetch under the hood).
  • I updated the fetch logic for fetching the next page. See the change from onAppear to task. Note: I'm using task as it's a multi-purpose function. It will trigger both onAppear and onChange, as well as cancel onDisappear. You may or may not want the cancellation behavior onDisappear inherent to task -- and in the case you don't, you can choose to instead implement both onChange and onAppear.
  • I removed the error view at the top of the screen in favor of an error alert modal.
  • I removed the tail-load text indicator in favor of a ProgressView().
• LaunchListViewModel changes:
  • Set canLoadNext to be a calculated property.
  • Use a transforming initializer of GraphQLQueryPager, given that we are only seemingly interested in the Launch model of the results.
  • Swap to using subscribe(completion:) instead of receive(on:).sink(receiveValue:). Really just a convenience thing, this isn't a functional change.
  • Note that the subscribe call is just being used to set launches or error, and nothing else.
  • I renamed isLoading to showTailSpinner, since that's a more accurate representation of what's happening. If we wanted to show a spinner on first load, a tail spinner likely isn't the appropriate design choice.
  • I added a guard to the loadNextPage, which ensures that we aren't calling this when we shouldn't.
  • I added an error assertionFailure: We will halt execution in DEBUG if something is wrong. I didn't hit this in my testing.

My mental model of list view UIs

Everyone has a different mental model, and every application has unique constraints and requirements. With that said, my mental model for list view, generally speaking, is as follows: 1. We have an **optional** array of values that represent each entry in the list. 2. When the array is `nil`, it means we haven't fetched our first page of values (and we don't have any cached values) and should be showing the full screen loading page. 3. When the array is empty, it means we received values, but we should be showing an empty state. 4. Every list view should be refreshable; a pull to refresh should replace the full list of data with a brand new fetch of the first page. This should be a real, non-cache driven fetch, which means that the pull-to-refresh control will remain visible and spinning until data comes back. Note that this isn't happening in the code-sample above. When [this change](https://github.com/apollographql/apollo-ios-dev/pull/292) is merged & shipped, it should give you the capability to attach a callback to `refetch()` – which means you can make this an `async` function via a [Continuation](https://developer.apple.com/documentation/swift/checkedcontinuation). That will allow you to `await` the function, and have the refresh control remain spinning until it is done fetching data. 5. Pagination should ideally happen _prior_ to getting to the last item to minimize the chance that a user ever sees a tail loading spinner. 6. Network errors should be shown to the user – either via a full-screen error view (with retry button) or via a toast.

Let me know if that's helpful as a demonstration!

[mapy1874]

Thanks for sharing! This does help solve the bug/error for this specific scenario. Without library change, I guess in the real world the following scenario would still break if we have a large enough paged size and trigger pagination at certain position.

I made the following changes on top of your commit:

1. Instead of triggering pagination at the last item, trigger it at arr[-pageSize]
2. Make pageSize 15, so on an iPhone 15 pro, the item at arr[-15] would not appear on the screen if you scroll to the end of the list. This prevents users from retriggering task of the item when trying to scroll down multiple times. This means if the user doesn't scroll back up, we will only be able to trigger loadNext at the arr[-15] exactly once.
3. Add a navigation link to the list, so when we re-navigate to the list, the list will use fetch and read from cache first.

To reproduce the bug: (Use Network link conditioner to make it more reproducible)

1. navigate to the list
2. go back
3. navigate to the list and scroll fast down
4. pagination will stop

Though no more loadInProgress error will be thrown this time, the bug still happens due to the onAppear/onChange/onDisappear, trigger loadNext pattern. When the cached item appears and triggers loadNext, the initial fetch is still in progress so the guard will make loadNextPage return early. After fetch's load via the network completes, unless the user scrolls way back up to make the triggering item appear again, the pagination will stop.

Really appreciate the discussions! This really helps me understand the library more. Do you have any suggestions fore this case if we were not changing this onAppear/onChange/onDisappear, trigger loadNext pagination pattern?

[Iron-Ham]

I think the error being experienced at this point isn't to do with ApolloPagination specifically – but rather, SwiftUI's default behavior.

Stepping away from early invocation of loadNextPage for a moment – the issue you're describing seems to boil down to the behavior of the .task modifier, and could be triggered with any networking scheme – inclusive of the out-of-the-box URLSession's data(from:delegate:). The .task modifier will trigger its closure onAppear/onChange and will cancel its asynchronous operation onDisappear – so the moment you scroll off-screen, any active Task will be cancelled. Since ApolloPagination's GraphQLQueryPager relies on Tasks under the hood, it respects Task cancellations – and will attempt to respond to cancellations.

You could build in the behavior you'd prefer by:

• Avoiding the use of task, instead preferring to use onAppear and onChange.
• Building your own custom modifier.
• Something else.

As far as invocating loadNextPage early – there are a few ways of doing that. For UIKit-based apps, you could create your own InfiniteScrollController that communicates with a UICollectionView/UITableView, hooks into the scrollViewWillEndDragging(_:withVelocity:targetContentOffset) function to determine whether or not to fire off a load request. For a SwiftUI-based app, I would imagine you'd want some sort of ScrollViewReader based solution.

Ultimately, the question of what should trigger a load is somewhat philosophical. I'm of the opinion that it's a user-triggered event. I fall into the camp of thinking that a specific row of data appearing on-screen isn't a user-triggered event – but a side effect of the action the user took: scrolling. Given that, it makes sense to me that an infinite scroll load is triggered on scroll-based logic – especially given the default behaviors of the onAppear and task modifiers.

As an aside, I think I've got an idea for a small SwiftUI package centered around providing that kind of behavior to List. 🤔

---

As a quick note on why we don't allow fetches on cached data, since I don't think I explained that in the previous message – we don't allow fetches on cached data, since cached data may be stale. This is a common bug for folks that use a returnCacheDataAndFetch CachePolicy – and I've seen it across a fair number of apps.

[mapy1874]

Looks like it's better for us to adopt the scrolling scheme instead of relying on the library to adapt to this triggering loadMore onAppear approach.

Not sure what's a good way to point out that this loadMore onAppear approach is not desirable when using apollo-ios-pagination. I assume many SwiftUI users of the pagination library would face the same thing as us.

Also love the mini package idea! Do let me know if you get to it someday. Thanks for all the valuable inputs!

[github-actions[bot]]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

[Iron-Ham]

@mapy1874 As a quick aside – there should be a clean workaround for onAppear usage in the next release, when callbacks to fetch and refetch are added.

In the near future, your LaunchListViewModel could look something like this (note that I didn't implement this for refetch in this example – but should):

(changes are: addition of hasFirstPageLoaded variable, adding a callback within fetch())

import Apollo
import ApolloPagination
import RocketReserverAPI
import SwiftUI

private let pageSize = 10

extension LaunchListQuery.Data.Launches.Launch: Identifiable { }

@Observable final class LaunchListViewModel: ObservableObject {
    var showTailSpinner = false
    var canLoadNext: Bool { pager.canLoadNext }
    var launches: [LaunchListQuery.Data.Launches.Launch] = []
    var hasFirstPageLoaded: Bool = false
    var error: Error?
    var showError: Bool {
        get { error != nil }
        set { error = nil }
    }
    private var pager: GraphQLQueryPager<[LaunchListQuery.Data.Launches.Launch]>

    init() {
        let initialQuery = LaunchListQuery(pageSize: .some(pageSize), cursor: .none)
        self.pager = GraphQLQueryPager(
            client: Network.shared.apollo,
            initialQuery: initialQuery,
            extractPageInfo: { data in
                CursorBasedPagination.Forward(hasNext: data.launches.hasMore, endCursor: data.launches.cursor)
            },
            pageResolver: { page, direction in
                LaunchListQuery(pageSize: .some(pageSize), cursor: page.endCursor ?? .none)
            },
            transform: { data in
                data.launches.launches.compactMap { $0 }
            }
        )
        pager.subscribe { result in
            switch result {
            case .success((let launches, _)):
                self.launches = launches
            case .failure(let error):
                // These are network errors, and worth showing to the user.
                self.error = error
            }
        }

        fetch()
    }

    func refresh() {
        pager.refetch()
    }

    func fetch() {
        hasFirstPageLoaded = false
        pager.fetch() {
            hasFirstPageLoaded = true
        }
    }

    func loadNextPage() {
        guard canLoadNext, !showTailSpinner else { return }
        self.showTailSpinner = true
        pager.loadNext() { error in
            self.showTailSpinner = false
            // This is a usage error
            if let error {
                assertionFailure(error.localizedDescription)
            }
        }
    }
}

You could somewhat emulate this behavior today, by having the file look like this:

(Changes are: Addition of hasFirstPageLoaded variable, setting hasFirstPageLoaded to false in fetch(), setting it to true in subscribe if source is fetch)

import Apollo
import ApolloPagination
import RocketReserverAPI
import SwiftUI

private let pageSize = 10

extension LaunchListQuery.Data.Launches.Launch: Identifiable { }

@Observable final class LaunchListViewModel: ObservableObject {
    var showTailSpinner = false
    var canLoadNext: Bool { pager.canLoadNext }
    var launches: [LaunchListQuery.Data.Launches.Launch] = []
    var hasFirstPageLoaded: Bool = false
    var error: Error?
    var showError: Bool {
        get { error != nil }
        set { error = nil }
    }
    private var pager: GraphQLQueryPager<[LaunchListQuery.Data.Launches.Launch]>

    init() {
        let initialQuery = LaunchListQuery(pageSize: .some(pageSize), cursor: .none)
        self.pager = GraphQLQueryPager(
            client: Network.shared.apollo,
            initialQuery: initialQuery,
            extractPageInfo: { data in
                CursorBasedPagination.Forward(hasNext: data.launches.hasMore, endCursor: data.launches.cursor)
            },
            pageResolver: { page, direction in
                LaunchListQuery(pageSize: .some(pageSize), cursor: page.endCursor ?? .none)
            },
            transform: { data in
                data.launches.launches.compactMap { $0 }
            }
        )
        pager.subscribe { result in
            switch result {
            case .success((let launches, let source)):
                self.launches = launches
                if source == .fetch {
                    self.hasFirstPageLoaded = true
                }
            case .failure(let error):
                // These are network errors, and worth showing to the user.
                self.error = error
            }
        }

        fetch()
    }

    func refresh() {
        pager.refetch()
    }

    func fetch() {
        hasFirstPageLoaded = false
        pager.fetch()
    }

    func loadNextPage() {
        guard canLoadNext, !showTailSpinner else { return }
        self.showTailSpinner = true
        pager.loadNext() { error in
            self.showTailSpinner = false
            // This is a usage error
            if let error {
                assertionFailure(error.localizedDescription)
            }
        }
    }
}

This would rely on a task picking up on the onChange effect of a fetch() coming in (the hasFirstPageLoaded variable changing), and firing its event again. A bit of an abuse of SwiftUI, but functional for folks relying on task of the last row. This should also work even in the event of cached results – as you showcased in your code sample above.

[Iron-Ham]

@mapy1874 I found this – which seems to be in spirit with what i would have built out as a package: https://github.com/danielsaidi/ScrollKit/blob/main/Sources/ScrollKit/Helpers/ScrollViewOffsetTracker.swift#L11-L35

I haven't used it, so I can't speak to how well it works – but it seems like what you're looking for