Skip to content

[2.x] fix: Stop the reconnect catch-up duplicating a discussion in the list - #5003

Open
ekumanov wants to merge 1 commit into
flarum:2.xfrom
ekumanov:fix/realtime-catchup-duplicate-discussion
Open

[2.x] fix: Stop the reconnect catch-up duplicating a discussion in the list#5003
ekumanov wants to merge 1 commit into
flarum:2.xfrom
ekumanov:fix/realtime-catchup-duplicate-discussion

Conversation

@ekumanov

Copy link
Copy Markdown
Contributor

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() empties extraDiscussions, because it routes through clear() and DiscussionListState.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 rebuilds pages and leaves extraDiscussions alone. 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, and getPages() prepends extraDiscussions as a synthetic page. The discussion then renders from both halves at once.

  • revalidate() now reconciles once the results have landed, dropping from extraDiscussions only the ids the new pages actually contain. Deliberately not a blanket clear: revalidate() swallows a failure and resolves rather than rejecting, leaving pages untouched, and clearing there would take realtime's additions off a list that was never reloaded.
  • getAllItems() no longer counts 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. 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.data is keyed [type][id], pushObject returns the existing model when there is one, and the only eviction is Store.remove() from Model.delete() — so pushPayload hands back the same instance and the two matches are equivalent. And nothing on the failing path calls deleteDiscussion at all. Its splice(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 on 2.x today with #4993 in place.

A note on forums without realtime: addDiscussion has no caller in core or any bundled extension, so extraDiscussions is only ever non-empty with realtime enabled, and revalidate() only has a caller in realtime's reconnect catch-up. On a forum without realtime both changes are no-ops.

Reviewers should focus on:

  • That reconciling against super.getPages() rather than this.getPages() is the right call — the override prepends extraDiscussions, so this.getPages() would match every one of them against itself and the filter would never drop anything.
  • That filtering by "id present in the new pages" is the right predicate, rather than clearing outright. The failure case is the reason: a swallowed network error leaves the list exactly as it was, and realtime's additions have to survive that.
  • Whether the getAllItems() change belongs here or in its own PR.

Necessity

  • Has the problem that is being solved here been clearly explained? — a discussion renders twice after a reconnect catch-up; reported in Reconnect catch-up duplicates a discussion in the list (not fixed by #4993) #5002.
  • If applicable, have various options for solving this problem been considered? — clearing extraDiscussions in revalidate() outright is simpler but loses realtime additions when the revalidation fails; reconciling by id keeps the invisible-refetch contract intact.
  • For core PRs, does this need to be in core, or could it be in an extension? — the list state and revalidate() both live in core; realtime only calls them.
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation. — reproduced on a full prod-mirror install (realtime enabled, auto-release at the default 10s) and then narrowed to a unit test.
  • Frontend changes: tests are green. — 230/230 in framework/core/js/tests/unit, including [2.x] fix: Stop a realtime update duplicating a discussion in the list #4993's DiscussionListState.dedup suite and [2.x] fix: stop the reconnect catch-up from emptying the discussion list #4889's PaginatedListState revalidate suite. check-typings and prettier --check clean.
  • Frontend changes: tests have been added. — 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 that refresh() still empties extraDiscussions as it always has.
  • Backend changes: tests are green — no backend changes.
  • Backend changes: tests have been added, or are not appropriate here — n/a.
  • Where applicable, changes are suitable for all supported database drivers (MySQL, MariaDB, PostgreSQL, SQLite). — no database involvement.
  • The description above is written by me and describes what this pull request actually does.

Required changes:

  • Related documentation PR: (Remove if irrelevant)

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>
@ekumanov
ekumanov requested a review from a team as a code owner August 27, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reconnect catch-up duplicates a discussion in the list (not fixed by #4993)

1 participant