[2.x] fix: stop the reconnect catch-up from emptying the discussion list - #4889
Merged
Conversation
Returning to a backgrounded tab took the discussion list off screen, replaced it with a loading spinner, and left the reader waiting on a request they had not asked for. On iOS, where the reconnect is unconditional, this happened on every return after five seconds — most visibly on a forum pinned to the home screen, where opening the app is exactly this sequence. The cause is the shape of the catch-up, not the catch-up itself. Pusher does not buffer, so anything that fired while the socket was down is lost and the client genuinely has to re-ask on reconnect. It did so with `refresh()`, which sets the loading state and clears the pages before it sends anything. That is the right behaviour when the reader has changed what they are looking at, because the old results are then wrong; it is the wrong behaviour for a background reconciliation, where what is on screen is still valid and only needs updating. Scroll position and any pages loaded past the first went with it. `revalidate()` reloads the first page and swaps the results in when they arrive, leaving the list rendered throughout. Concurrent calls collapse onto the in-flight promise rather than racing to replace the pages, and a failure resolves rather than rejecting — there is no user-initiated action to report it against, and the results already on screen remain the best answer available. `refresh()` is unchanged, so the index refresh button and the reload after posting still clear and show their loading state as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Returning to a backgrounded tab makes the discussion list disappear, shows a loading spinner in its place, and leaves you waiting on a request you did not ask for. Most noticeable on a forum pinned to a phone home screen, where opening the app is exactly that sequence, but it happens on desktop too when coming back to a tab that has been idle a while.
Cause
The catch-up itself is necessary. Pusher does not buffer, so events that fire while the socket is down are lost and the client has to re-ask on reconnect. The problem is how it asks.
catchUp()in the realtime extension calledapp.discussions.refresh(), andrefresh()setsinitialLoadingand callsclear()before the request goes out:So the list is guaranteed to blank, with no overlap between losing the old results and receiving the new ones. Scroll position and any pages loaded past the first go too.
That is correct behaviour when the reader has changed what they are looking at — the old results are wrong and continuing to show them would be misleading. It is the wrong behaviour for a background reconciliation, where what is on screen is still valid and merely out of date.
Two things made it constant rather than occasional:
RECONNECT_HIDDEN_THRESHOLD_MSis 5s, so a brief glance away is enough to arm it.isIOS() || unhealthy. On iOS the health check is bypassed entirely — for a good reason, since iOS freezes sockets on backgrounding without reporting them closed — so the refetch ran even with a perfectly healthy connection and nothing missed.Change
PaginatedListStategainsrevalidate(): reload the first page, keep the list rendered, swap the results in when they land. Concurrent calls collapse onto the in-flight promise instead of racing to replacepages. A failure resolves rather than rejecting, since there is no user-initiated action to report it against and the results already on screen remain the best answer available.catchUp()uses it, falling back torefresh()if core does not have it.refresh()is untouched, so the index refresh button and the reload after posting a discussion still clear and show their loading state.The
isIOS()reconnect is deliberately left alone. Rebuilding the socket is cheap and defensible on a platform that freezes them silently; only the UI teardown was the problem.Verified
Driven in a real browser against a dev forum, backgrounding for 7s and returning:
Still reconciles: dropping an item from the loaded page and then triggering a reconnect restores it, silently and without reordering.
refresh()re-checked in the same harness and still clears to zero items as before.Unit tests cover the visible-during-request behaviour, replacement on arrival, first-page-only, concurrent collapsing, failure leaving the list intact, a failure not blocking the next attempt, and an empty response emptying the list rather than keeping stale rows.
Forums without the realtime extension are unaffected — nothing in core triggers an unrequested
refresh(); the only two call sites are the refresh button and post-submit, both user-initiated.Fixes the behaviour reported on mobile and desktop after #4588 / #4597 / #4717.