[2.x] fix: Stop the reconnect catch-up duplicating a discussion in the list - #5003
Open
ekumanov wants to merge 1 commit into
Open
[2.x] fix: Stop the reconnect catch-up duplicating a discussion in the list#5003ekumanov wants to merge 1 commit into
ekumanov wants to merge 1 commit into
Conversation
Returning to a tab that had been idle long enough for the websocket to die shows a discussion listed twice, until the reader refreshes. `refresh()` empties `extraDiscussions`, because it routes through `clear()`. `revalidate()` (flarum#4889) deliberately clears nothing before it asks the API — that is the point of it, the list stays on screen — so it rebuilds `pages` and leaves `extraDiscussions` alone. The first page it gets back contains exactly the discussions realtime had put there, since a new post is what moved them to the top, and the list then renders them from both halves at once. `addDiscussion` has no caller in core; only realtime adds discussions this way, so only a forum with realtime enabled can reach the duplicate at all. Nothing on the path calls `deleteDiscussion`, which is why matching there by id rather than by reference (flarum#4993) does not close it. - Reconcile after the revalidation lands, dropping from `extraDiscussions` only the ids the new pages actually contain. Not a blanket clear: a revalidation that failed resolves rather than rejecting and leaves `pages` untouched, and clearing then would take realtime's additions off a list that was never reloaded. - Stop `getAllItems()` counting `extraDiscussions` twice. It concatenated them onto `super.getAllItems()`, which flattens the `getPages()` this class had already overridden to prepend them. Harmless where it is read today (`isEmpty()`), wrong for anything that counts. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #5002.
Changes proposed in this pull request:
Returning to a tab that had been idle long enough for the websocket to die shows a discussion listed twice, until the reader refreshes.
refresh()emptiesextraDiscussions, because it routes throughclear()andDiscussionListState.clear()resets it.revalidate()(#4889) deliberately clears nothing before it asks the API — that is the point of it, the list stays on screen and keeps its scroll position — so it rebuildspagesand leavesextraDiscussionsalone. The first page it gets back contains exactly the discussions realtime had put there, because a new post is what moved them to the top in the first place, andgetPages()prependsextraDiscussionsas a synthetic page. The discussion then renders from both halves at once.revalidate()now reconciles once the results have landed, dropping fromextraDiscussionsonly the ids the new pages actually contain. Deliberately not a blanket clear:revalidate()swallows a failure and resolves rather than rejecting, leavingpagesuntouched, and clearing there would take realtime's additions off a list that was never reloaded.getAllItems()no longer countsextraDiscussionstwice. It concatenated them ontosuper.getAllItems(), which flattens thegetPages()this class had already overridden to prepend them. Harmless where it is read today (isEmpty()), wrong for anything that counts. Adjacent enough to the reconciliation that I'd rather not leave it in place underneath it, but happy to split it out if you'd prefer.A note on #4993: that PR was filed against this same report, so I want to be explicit rather than look like I'm re-fixing it. #4993 matches by
id()instead of by object reference on the reasoning that "the store returns a fresh instance whenever it no longer holds the one already on screen".Store.datais keyed[type][id],pushObjectreturns the existing model when there is one, and the only eviction isStore.remove()fromModel.delete()— sopushPayloadhands back the same instance and the two matches are equivalent. And nothing on the failing path callsdeleteDiscussionat all. Itssplice(index)→splice(index, 1)half is a real fix and this PR keeps it; the duplicate simply comes from somewhere else. I have a test in this PR that fails on2.xtoday with #4993 in place.A note on forums without realtime:
addDiscussionhas no caller in core or any bundled extension, soextraDiscussionsis only ever non-empty with realtime enabled, andrevalidate()only has a caller in realtime's reconnect catch-up. On a forum without realtime both changes are no-ops.Reviewers should focus on:
super.getPages()rather thanthis.getPages()is the right call — the override prependsextraDiscussions, sothis.getPages()would match every one of them against itself and the filter would never drop anything.getAllItems()change belongs here or in its own PR.Necessity
extraDiscussionsinrevalidate()outright is simpler but loses realtime additions when the revalidation fails; reconciling by id keeps the invisible-refetch contract intact.revalidate()both live in core; realtime only calls them.Confirmed
framework/core/js/tests/unit, including [2.x] fix: Stop a realtime update duplicating a discussion in the list #4993'sDiscussionListState.dedupsuite and [2.x] fix: stop the reconnect catch-up from emptying the discussion list #4889'sPaginatedListStaterevalidate suite.check-typingsandprettier --checkclean.DiscussionListState.revalidate.test.ts: the duplicate itself (fails pre-fix, passes post-fix), that a realtime addition the refetch did not return stays on the list, that a failed revalidation leaves both halves untouched, and thatrefresh()still emptiesextraDiscussionsas it always has.Required changes: