diff --git a/extensions/realtime/js/src/forum/extend/Application.ts b/extensions/realtime/js/src/forum/extend/Application.ts index 5ba81e3889..cb469b9f30 100644 --- a/extensions/realtime/js/src/forum/extend/Application.ts +++ b/extensions/realtime/js/src/forum/extend/Application.ts @@ -133,42 +133,93 @@ export default function () { } }; - app.websocket = new Pusher(wsKey, pusherOptions); - setupChannels(app.websocket); - - // iOS browsers (all WebKit) silently drop WebSocket connections when - // the tab is backgrounded or the device sleeps, without firing `close` - // — pusher-js's built-in recovery never triggers, so realtime updates - // go missing until the page is reloaded. iOS also bfcaches pages on - // app-switch, which restores via `pageshow` (persisted=true) and does + // WebKit silently drops WebSocket connections when the tab is + // backgrounded or the device sleeps, often without firing `close` — + // pusher-js's built-in recovery never triggers (or triggers only after + // its foreground-only activity/pong timers finally elapse), so realtime + // updates go missing until the page is reloaded. iOS also bfcaches pages + // on app-switch, which restores via `pageshow` (persisted=true) and does // NOT fire `visibilitychange` on return. We therefore hook both events. // - // The visibilitychange path is gated on `isIOS()`: desktop browsers - // (and Android) maintain the WebSocket fine across tab backgrounding - // and don't need a forced reconnect, which would otherwise cause an - // unnecessary discussion-list refetch on every tab-switch return. + // The visibilitychange path reconnects when on iOS (where backgrounding + // kills the socket every time — see #4590/#4654) OR when the connection + // is demonstrably unhealthy: pusher-js already reports a non-connected + // state, or no protocol frame (events, pusher:pong, ...) has arrived + // within the activity window — the zombie case where `state` still + // claims 'connected'. Desktop Safari suspends hidden pages just like + // iOS, so it routinely lands in that zombie case (#4717). Healthy + // desktop tabs keep receiving pongs every ≤30s even while hidden, so a + // plain tab switch still triggers no spurious refetch (#4662). // // `forceReconnect` constructs a fresh Pusher instance rather than // calling `connect()` on the existing one. pusher-js 7.6's default // strategy enforces a `lives: 2` budget on its WebSocket transport; - // every iOS-initiated 1006 close decrements the budget and after the - // second backgrounding the strategy reports unsupported, so every - // subsequent `connect()` transitions straight to `'failed'`. A fresh - // Pusher comes with a fresh strategy tree and a full `livesLeft`, - // making the recovery survive arbitrarily many backgrounding cycles. - // - // After reconnecting, Pusher has no server-side buffering for events - // that fired while the socket was dead — we refresh the visible - // discussions list once the new connection reports `'connected'` so the - // UI catches up on missed activity. Refresh is gated on the - // `'connected'` event (not fired immediately after `connect()`) because - // an immediate Mithril redraw races with pusher-js's channel - // resubscription and can leave the client receiving no further push - // events. + // every silent 1006 close decrements the budget and after the second + // one the strategy reports unsupported, so every subsequent `connect()` + // transitions straight to `'failed'`. A fresh Pusher comes with a fresh + // strategy tree and a full `livesLeft`, making the recovery survive + // arbitrarily many backgrounding cycles. // - // See flarum/framework#4588 and #4597. + // See flarum/framework#4588, #4597 and #4717. const RECONNECT_HIDDEN_THRESHOLD_MS = 5_000; + // A healthy connection sees protocol traffic at least every + // `activity_timeout` (30s, advertised by the server) plus the pong wait. + const STALE_CONNECTION_THRESHOLD_MS = 65_000; + let hiddenSince: number | null = null; + let lastFrameAt = Date.now(); + let everConnected = false; + + // Refresh the data that realtime events would have kept current. Pusher + // has no server-side buffering: anything that fired while the socket was + // down is lost, so on reconnect the UI has to catch up by refetching. + const catchUp = (): void => { + (app as any).discussions?.refresh?.(); + + const discussion = app.current.get('discussion'); + const stream = app.current.get('stream'); + + if (discussion && stream) { + // Capture BEFORE the refetch grows postIds: missed events never + // reached the store, so right now viewingEnd() still reflects the + // last synced state. Afterwards its 1-post drift tolerance could no + // longer tell "was at the end, several posts arrived while + // disconnected" apart from "viewing an interior window" (#4717). + const wasViewingEnd: boolean = !!stream.viewingEnd(); + + app.store.find('discussions', discussion.id() as string).then(() => { + if (wasViewingEnd) { + stream.syncEnd().then((): void => m.redraw()); + } else { + m.redraw(); + } + }); + } + }; + + // Track liveness and reconnects. Applied to the initial Pusher instance + // and to every fresh instance `forceReconnect` builds. + const watchConnection = (websocket: Pusher): void => { + websocket.bind_global((): void => { + lastFrameAt = Date.now(); + }); + + websocket.connection.bind('state_change', (states: { previous: string; current: string }) => { + if (states.current !== 'connected') return; + + const isReconnect = everConnected; + everConnected = true; + lastFrameAt = Date.now(); + + // Catch up on every effective reconnect — pusher-js's own recovery + // as well as our forced ones. Gated on the transition into + // `'connected'` (not run right after `connect()`) because an + // immediate Mithril redraw races with pusher-js's channel + // resubscription and can leave the client receiving no further push + // events (see #4590). + if (isReconnect) catchUp(); + }); + }; const forceReconnect = (): void => { const previous = app.websocket; @@ -177,16 +228,15 @@ export default function () { const fresh = new Pusher(wsKey, pusherOptions); app.websocket = fresh; - const onReconnected = (): void => { - fresh.connection.unbind('connected', onReconnected); - (app as any).discussions?.refresh?.(); - }; - fresh.connection.bind('connected', onReconnected); - + watchConnection(fresh); setupChannels(fresh); RealtimeState.notifyChannelsReconnected(); }; + app.websocket = new Pusher(wsKey, pusherOptions); + watchConnection(app.websocket); + setupChannels(app.websocket); + // Application.mount() runs once per page load, so these listeners are // installed once and live for the lifetime of the page — no teardown needed. document.addEventListener('visibilitychange', () => { @@ -197,7 +247,11 @@ export default function () { if (hiddenSince === null) return; const wasHiddenFor = Date.now() - hiddenSince; hiddenSince = null; - if (wasHiddenFor > RECONNECT_HIDDEN_THRESHOLD_MS && isIOS()) { + if (wasHiddenFor <= RECONNECT_HIDDEN_THRESHOLD_MS) return; + + const unhealthy = app.websocket?.connection.state !== 'connected' || Date.now() - lastFrameAt > STALE_CONNECTION_THRESHOLD_MS; + + if (isIOS() || unhealthy) { forceReconnect(); } }); diff --git a/extensions/realtime/js/src/forum/extend/Discussion/NewActivity.ts b/extensions/realtime/js/src/forum/extend/Discussion/NewActivity.ts index ca6be5bae7..a9a2c5b54f 100644 --- a/extensions/realtime/js/src/forum/extend/Discussion/NewActivity.ts +++ b/extensions/realtime/js/src/forum/extend/Discussion/NewActivity.ts @@ -10,13 +10,24 @@ const CORE_REVISED_EVENT = 'revisedEvent'; export default function (): void { extend(DiscussionPage.prototype, 'oninit', function (this: any) { this.websocketEventPosted = (data: unknown): void => { + // Capture BEFORE the payload (and the refetch below) updates postIds: + // once new posts are in the store, viewingEnd()'s 1-post drift + // tolerance can no longer tell "was at the end, several posts arrived + // at once" apart from "viewing an interior window" — so a single + // missed event would permanently disable live-append (#4717). + const wasViewingEnd: boolean = !!this.stream?.viewingEnd(); + const discussion = app.store.pushPayload(data as Parameters[0]) as any; if (discussion.id() === this.discussion?.id() && this.stream) { const oldCount: number = this.discussion.commentCount(); app.store.find('discussions', this.discussion.id()).then(() => { - this.stream.update().then((): void => m.redraw()); + if (wasViewingEnd) { + this.stream.syncEnd().then((): void => m.redraw()); + } else { + m.redraw(); + } if (!document.hasFocus()) { app.setTitleCount(Math.max(0, this.discussion.commentCount() - oldCount)); @@ -30,11 +41,19 @@ export default function (): void { }; this.websocketEventStreamUpdate = (data: unknown): void => { + // Same pre-payload capture as websocketEventPosted (#4717) — event + // posts (sticky/lock/rename) move postIds too. + const wasViewingEnd: boolean = !!this.stream?.viewingEnd(); + const discussion = app.store.pushPayload(data as Parameters[0]) as any; if (discussion.id() === this.discussion?.id() && this.stream) { app.store.find('discussions', this.discussion.id()).then(() => { - this.stream.update().then((): void => m.redraw()); + if (wasViewingEnd) { + this.stream.syncEnd().then((): void => m.redraw()); + } else { + m.redraw(); + } }); } }; diff --git a/framework/core/js/src/forum/states/PostStreamState.ts b/framework/core/js/src/forum/states/PostStreamState.ts index ece5ffc966..e99b0a6998 100644 --- a/framework/core/js/src/forum/states/PostStreamState.ts +++ b/framework/core/js/src/forum/states/PostStreamState.ts @@ -71,10 +71,28 @@ export default class PostStreamState { /** * Update the stream so that it loads and includes the latest posts in the * discussion, if the end is being viewed. + * + * Note: viewingEnd() tolerates a drift of at most one post, so this is only + * suitable when the store has gained a single new post since the window was + * last synced (the live-event case). When several posts land in the store + * at once — e.g. they were missed while a realtime connection was down — + * capture viewingEnd() BEFORE pushing the new payloads and call syncEnd() + * directly instead. */ update() { if (!this.viewingEnd()) return Promise.resolve(); + return this.syncEnd(); + } + + /** + * Load through to the latest post in the discussion, expanding the visible + * window to the end of the stream. Unlike update(), this places no bound on + * how far the window has fallen behind — the caller is responsible for + * ensuring the end was being viewed before the posts being caught up on + * entered the store. + */ + syncEnd(): Promise { this.visibleEnd = this.count(); return this.loadRange(this.visibleStart, this.visibleEnd); diff --git a/framework/core/js/tests/unit/forum/states/PostStreamState.test.ts b/framework/core/js/tests/unit/forum/states/PostStreamState.test.ts index 9fc9042758..ffb413964e 100644 --- a/framework/core/js/tests/unit/forum/states/PostStreamState.test.ts +++ b/framework/core/js/tests/unit/forum/states/PostStreamState.test.ts @@ -147,4 +147,51 @@ describe('PostStreamState', () => { expect(nearPage(discussion)).toEqual([]); }); }); + + // Regression tests for #4717 — catching up after a missed-event gap. + // + // update() guards on viewingEnd(), which tolerates a drift of at most one + // post. That covers the live-event case (exactly one new post since the + // last sync), but once ≥2 posts have entered the store — e.g. one was + // missed while a realtime connection was down and the next live event + // refetched the discussion — every update() call returns early and + // live-append is permanently dead. syncEnd() is the unbounded variant that + // realtime catch-up paths call after capturing end-ness up front. + describe('update() / syncEnd() after a missed-event gap (#4717)', () => { + test('update() appends a single new post when viewing the end (live-event path)', async () => { + const { discussion, postIds } = seedStore(5, [0, 1, 2, 3, 4]); + // Window synced through post 4 of 5: one post arrived since. + const state = new PostStreamState(discussion, postsFor(postIds.slice(0, 4))); + + await state.update(); + + expect((state as any).visibleEnd).toBe(5); + expect(state.posts()).toHaveLength(5); + expect(state.posts().every((p) => p !== null)).toBe(true); + }); + + test('update() refuses once the window is two or more posts behind (locks the guard semantics)', async () => { + const { discussion, postIds } = seedStore(6, [0, 1, 2, 3, 4, 5]); + // Window synced through post 4, but TWO posts entered the store since + // (one missed while disconnected + one live): bookkeeping alone cannot + // tell this apart from viewing an interior window, so update() must + // not append — callers with better knowledge use syncEnd(). + const state = new PostStreamState(discussion, postsFor(postIds.slice(0, 4))); + + await state.update(); + + expect((state as any).visibleEnd).toBe(4); + }); + + test('syncEnd() loads through to the latest post no matter how far behind the window is', async () => { + const { discussion, postIds } = seedStore(8, [0, 1, 2, 3, 4, 5, 6, 7]); + const state = new PostStreamState(discussion, postsFor(postIds.slice(0, 4))); + + await state.syncEnd(); + + expect((state as any).visibleEnd).toBe(8); + expect(state.posts()).toHaveLength(8); + expect(state.posts().every((p) => p !== null)).toBe(true); + }); + }); });