[isekovanic]

Ticket: https://linear.app/stream/issue/REACT-1476/fix-odd-edge-cases-for-channel-list-recovery

CLA

• I have signed the Stream CLA (required).
• Code changes are tested

Description of the changes, What, Why and How?

Fixes to the reconnect recovery of channel lists (ChannelPaginator.recoverLoadedItems), one per commit. The first four came up while testing the recovery on device, the last three in review. The last one is in BasePaginator and also covers pull-to-refresh.

Hold list pagination while a recovery runs

The recovery's first page is a refresh, which resets the list's offset to 0, and the later pages use a forced query shape that never commits an offset. A next page requested while the recovery ran (the list end reached) therefore asked for offset: 0, fetched page 1 again and placed it as a tail page. A recovery page landing could also clear that query's isLoading.

executeQuery now refuses tailward and headward queries while the recovery runs. A refresh still goes through. A refused next page is queried once after the recovery ends, because the list end does not ask again when the recovery changed nothing.

Keep the loaded channels past the range a recovery re-queried

The recovery re-queries at most 150 channels, but it removed every loaded channel no page returned, including all of those past 150. With the list scrolled past 150, the rows on screen were removed, the list jumped and remounted rows, and only one page loaded back.

A channel no page returned is now removed only when it sorts above the last channel the server returned. Channels below it were not re-queried and stay as they are. When a page comes back shorter than requested and shorter than a page, the pages reached the end of the list, so every loaded channel not returned is removed, as before. A channel past the re-queried range that was deleted while away stays until a live event or a later query covers it.

Wait for the list's in-flight query before recovering

The recovery's pages go through executeQuery, which refuses to run while the list is loading. With a next page in flight at reconnect, the first recovery page was refused and the whole recovery was skipped, so deleted, left, hidden and archived channels stayed in the list. The recovery now waits for that query to settle first. The ChannelManager test that asserted the skip now asserts the wait.

Continue the shown list with later recovery pages

A later recovery page is queried at a caller's shape, so it starts an interval of its own and merges only with intervals it overlaps. A page that shared no channel with the list, for example after several channels above it were deleted, became a separate interval, and the loop made it the shown one. The list then showed only that page. Recovery pages start where the previous one ended, so they are now placed as a continuation of the shown interval.

Let a new recovery replace a running one

Every reconnect starts a recovery of each list, and nothing stopped two from running at once. The recovery's pages are silent, so they never publish isLoading, and a second recovery did not wait for the first. The two loops interleaved their pages and shared _lastPageCids, and the first one to end opened pagination again while the other was still running.

A call while a recovery runs now returns right away, and the running pass stops once its page in flight lands and starts over from the first page. The pages it already fetched were watched on a connection that is gone, so they have to be queried again. The loop has no cap, so the pass that starts after the last reconnect always runs to the end.

Hand the list position back when a recovery stops early

The first recovery page resets offset to 0 and no recovery page commits one, so only the end of the recovery set it back. A failed page, or a pass replaced as above, returned before that and left offset at 0 with the whole list loaded. The next pages then fetched channels the list already had, one page at a time. The recovery now sets offset to the number of loaded channels whenever it returns early.

Restore the position after a failed refresh

A keepPreviousItems refresh (pull-to-refresh in the RN SDK, or the deferred refresh of a list loaded from the offline cache) resets the position before its request, because getNextQueryShape() reads it from state. When the request failed, no page was placed and nothing wrote the position back, so it stayed at the first page, with the same result as above. prepareQuery now passes the position it reset along with the query (positionBeforeReset), and placeQueryPage puts it back when there is no page to place. A refresh that succeeds is unchanged.

Testing

• Each fix has a unit test that fails without it.
• A randomized run of 300 reconnects with deletions, moves and new channels went from 12 collapsed lists to none, with no wrong channel or order within the re-queried range.
• On an Android emulator with a release build of the RN SampleApp against the real backend:
  • 90 test channels with deletes, leaves, hides and archives at the top, middle and bottom of the loaded range, plus one channel that got a message, come back exactly right in the paginator state and on screen;
  • a next page requested mid-recovery is held and loaded once the recovery ends;
  • with 210 channels loaded and the list scrolled near row 200, the recovery keeps all 210 channels and the rows on screen.
• The in-flight query case and the last three fixes are covered by the unit tests only.
• tsc, types:tests, eslint and prettier on the changed files, and yarn test (4833 tests) pass.

Changelog

• Fixed a next page requested during a reconnect recovery fetching the first page again.
• The reconnect recovery no longer removes loaded channels past the 150 it re-queries, so a list scrolled far down keeps its rows.
• The reconnect recovery now runs when the list was loading a page at reconnect, instead of being skipped.
• Fixed a reconnect recovery collapsing the list to one page when a later page shared no channel with it.
• A reconnect while a recovery runs now restarts it, instead of running two recoveries at once.
• Fixed the next pages after a failed reconnect recovery fetching channels the list already had.
• Fixed a failed keepPreviousItems refresh leaving the list position at the first page.

[MartinCupela]

The four fixes look right to me (read against the code, not run). One gap and two nits:

1. Two recoveries of the same list can overlap, and the first one's finally re-opens paging while the second runs.

• ConnectionRecoveryManager starts recoverChannelLists() detached on each reconnect edge (ConnectionRecoveryManager.ts:100, :132). Unlike recoverActiveChannels, it has no isRecovering guard.
• The recovery's pages are silent with keepPreviousItems, so they never publish isLoading: true (BasePaginator.ts:3087-3090). A second recoverLoadedItems() therefore doesn't wait in waitForQueryToSettle. It starts at once and runs its own offset loop next to the first.
• When the first finishes, its finally sets _isRecoveringLoadedItems = false while the second is still paging. executeQuery then lets tailward queries through again, which is the case the first commit fixes.

A quick double reconnect on a flaky network is enough to hit it. Making recoverLoadedItems single-flight would cover it: keep the running promise, and have a call that arrives meanwhile await it and then run once more, since the connection changed again. That would also keep the two loops from interleaving their pages.

2. Nit: ChannelManager.recover's doc comment is stale. It says recovery "is not silent: the isLoading transition guards against an overlapping query" (ChannelManager.ts:1003-1011). The pages are silent, and the guard is now _isRecoveringLoadedItems.

3. Nit: the end-of-list check assumes the server returns a full pageSize. reachedEnd is set when a page is shorter than min(this.pageSize, requested). If an app sets pageSize above the server's maximum limit for queryChannels, every full page from the server looks short. Recovery then removes all loaded channels it didn't get back, which is the behaviour the second commit replaces. That only happens with an unusual config, but comparing against the limit the server actually applied, or treating only an empty page as the end, would avoid it.

[isekovanic]

Heyo @MartinCupela , thanks for the review !

1. Really nice catch, fixed in 39e4481. However I decided to fix it by superseding the query rather than queueing. The reason for this is because if we wait for the current one to finish, we could potentially do multiple queryChannels calls for nothing.

2. Fixed

3. I’d keep this one as it is. The end rule is the same one BasePaginator uses for normal paging, so a page is the tail when requestedPageSize > items.length. With pageSize above the server max, the list already stops after its first page and never loads enough for the recovery to remove anything past the range (this is how we are handling this already). Treating only an empty page as the end breaks short lists. Say A, B, C are loaded, C gets deleted and a new N comes in. The server returns N, A, B, the loop ends without an empty page and C stays. And comparing against the limit the server applied would mean hardcoding the cap. Is this okay from your pov ?

[MartinCupela]

Thanks @isekovanic, all three look good to me.

1. Superseding is better than queueing: the earlier pass's pages were watched on a connection that's gone, so starting over is right. Restoring the position when a recovery stops early or a refresh fails (8aade93b, f7886ab6) covers the edges I'd have worried about.
2. 👍
3. Agreed, keep it. It's the same end rule normal paging uses, so a list with pageSize above the server max never loads past what recovery covers. Your A, B, C / N example shows why "only an empty page is the end" would be wrong.

One small nit, not blocking: a recovery requested while one runs now returns at once, before the restarted pass finishes. So ConnectionRecoveryManager.recover() ("Runs a full recovery now") can resolve while the channel lists are still recovering. Returning the running promise from recoverLoadedItems would let callers await the real end. On reconnect it doesn't matter, since that path runs detached.

[stream-ci-bot]

🎉 This PR is included in version 10.0.0-rc.27 🎉

The release is available on:

• GitHub release
• npm package (@rc dist-tag)

Your semantic-release bot 📦🚀

[stream-ci-bot]

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀