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

[ekumanov]

Current Behavior

Returning to a tab that had been idle long enough for the websocket to die shows a discussion listed twice. Refreshing the page clears it.

This is the report #4993 was filed against, so rc.8's changelog line reads as if it is fixed. It is not — I can still reproduce it on 2.x with #4993 in place.

Why #4993 doesn't close it

#4993 changes deleteDiscussion to match 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". That premise doesn't hold: Store.data is keyed [type][id], pushObject returns the existing model when one is present, and the only eviction is Store.remove() from Model.delete(). So pushPayload hands back the same instance for a discussion the store already knows, and by-id and by-reference matching are equivalent there.

More directly: nothing on the failing path calls deleteDiscussion at all.

(The change is harmless, and its splice(index) → splice(index, 1) half is a real fix. It just isn't this bug.)

What actually happens

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 whole point, 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 into extraDiscussions, because a new post is what moved them to the top in the first place. getPages() prepends extraDiscussions as a synthetic page, so the discussion renders from both halves at once.

addDiscussion has no caller in core or any bundled extension — only realtime adds discussions this way — so only a forum with realtime enabled can reach it.

Steps to Reproduce

With flarum/realtime enabled and flarum-realtime.release-discussion-updates-interval at its default of 10 (auto-release on):

1. Open the discussion index and leave it open.
2. Have someone post in an existing discussion that is on the first page. After ~10s the "N new discussions" updates auto-release and that discussion moves to the top of the list — one copy, correct.
3. Leave the tab in the background long enough for the websocket to be dropped (WebKit suspends hidden pages; on iOS this happens every time).
4. Return to the tab. The reconnect catch-up in extend/Application.ts runs and calls discussions.revalidate().
5. The discussion from step 2 is now listed twice.

A unit test reproducing it without the browser, written against the 2.x tree at v2.0.0-rc.8:

// pages hold [b, a]; realtime releases `a` to the top
state.addDiscussion(a);                 // -> ['1', '2'], correct

// the catch-up revalidates and the server returns `a` first
state.nextPage = [a, b];
await state.revalidate();

// -> ['1', '1', '2']

The same test against the pre-#4889 catch-up shape (refresh() instead of revalidate()) passes, which is the counterfactual: the duplicate arrives with the switch to revalidate().

Expected Behavior

The discussion is listed once. The background catch-up is invisible by design, so it should not leave the list in a state a manual refresh would not produce.

Screenshots

n/a — reproduced and demonstrated with a unit test instead (see the PR).

Environment

• Flarum version: 2.0.0-rc.8 (reproduced against the 2.x tree; introduced in 2.0.0-rc.6 by [2.x] fix: stop the reconnect catch-up from emptying the discussion list #4889)
• Website URL: https://pianoclack.com
• Webserver: nginx
• Hosting environment: vps
• Device type: desktop and mobile
• Operating system: Ubuntu 24.04 (server); reproduced from Safari/Chrome
• Browser: Safari, Chrome
• Database: MySQL 8.4
• PHP version: 8.4.24

Possible Solution

Reconcile after the revalidation lands: drop from extraDiscussions only the ids the new pages actually contain.

Not a blanket clear — revalidate() swallows a failure and resolves, leaving pages untouched, so clearing unconditionally would take realtime's additions off a list that was never reloaded.

PR follows, with a regression test.

While in there: getAllItems() counts extraDiscussions twice — it concatenates them onto super.getAllItems(), which flattens the getPages() this class already overrode to prepend them. Harmless where it is read today (isEmpty()), wrong for anything that counts.