fix: stop the reconnect catch-up from emptying the discussion list

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.

Author: imorland

extensions/realtime/js/src/forum/extend/Application.ts

@@ -173,8 +173,22 @@ export default function () {
     // 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.
+    //
+    // Nobody asked for this refetch, so it must not be visible as one.
+    // `refresh()` empties the list and shows its loading state before the
+    // request goes out, which meant returning to a backgrounded tab — every
+    // time, on iOS, where the reconnect is unconditional — made the discussions
+    // vanish behind a spinner and lose their scroll position. `revalidate()`
+    // leaves the list on screen and swaps the results in when they land.
     const catchUp = (): void => {
-      (app as any).discussions?.refresh?.();
+      const discussions = (app as any).discussions;
+
+      if (discussions?.revalidate) {
+        discussions.revalidate();
+      } else {
+        // Older core without `revalidate()`. Still better than nothing.
+        discussions?.refresh?.();
+      }
 
       const discussion = app.current.get('discussion');
       const stream = app.current.get('stream');

framework/core/js/src/common/states/PaginatedListState.ts

@@ -56,6 +56,13 @@ export default abstract class PaginatedListState<T extends Model, P extends Pagi
   protected loadingNext: boolean = false;
   protected loadingPage: boolean = false;
 
+  /**
+   * The in-flight background refresh, if any. Deliberately not part of the
+   * `isLoading()` family: a revalidation is invisible by design, so exposing it
+   * there would put the loading state back on screen.
+   */
+  protected revalidating: Promise<void> | null = null;
+
   protected constructor(params: P = {} as P, page: number = 1, pageSize: number | null = null) {
     this.params = params;
 
@@ -227,6 +234,44 @@ export default abstract class PaginatedListState<T extends Model, P extends Pagi
     return this.goto(page);
   }
 
+  /**
+   * Reload the first page without taking the current results off screen.
+   *
+   * `refresh()` clears the list and shows the loading state before it asks the
+   * API for anything, which is what you want when the reader has changed what
+   * they are looking at: the old results are wrong, and showing them would be a
+   * lie. It is the wrong shape for a background catch-up — a reconnected
+   * websocket, a regained network — where what is on screen is still valid and
+   * merely out of date. There, clearing first means the reader watches content
+   * they already had disappear behind a spinner, and loses their scroll
+   * position, to service a request they never made.
+   *
+   * This keeps the list rendered and swaps the results in when they arrive. A
+   * failure resolves rather than rejecting: a background refresh that could not
+   * complete should leave the reader exactly as they were, not blank the page.
+   */
+  public revalidate(): Promise<void> {
+    // A second call while one is in flight would race to replace `pages`, and
+    // the loser would win by arriving last.
+    if (this.revalidating) return this.revalidating;
+
+    this.revalidating = this.loadPage(1)
+      .then((results) => {
+        this.pages = [];
+        this.parseResults(1, results);
+      })
+      .catch(() => {
+        // Deliberately swallowed. There is no user-initiated action to report
+        // a failure for, and the results already on screen remain the best
+        // answer available.
+      })
+      .finally(() => {
+        this.revalidating = null;
+      });
+
+    return this.revalidating;
+  }
+
   public goto(page: number): Promise<void> {
     this.location = { page };
 

framework/core/js/tests/unit/common/states/PaginatedListState.test.ts

@@ -27,6 +27,61 @@ class TestState extends PaginatedListState<Model, TestParams> {
   }
 }
 
+/**
+ * A state whose page loads resolve when we say so, so that the list can be
+ * inspected while a request is still outstanding.
+ */
+class DeferredState extends PaginatedListState<Model, TestParams> {
+  get type() {
+    return 'test';
+  }
+
+  requestParams() {
+    return {};
+  }
+
+  public loadPageCalls: number[] = [];
+  private resolvers: ((results: any) => void)[] = [];
+  private rejecters: ((error: any) => void)[] = [];
+
+  protected loadPage(page = 1): Promise<any> {
+    this.loadPageCalls.push(page);
+
+    return new Promise((resolve, reject) => {
+      this.resolvers.push(resolve);
+      this.rejecters.push(reject);
+    });
+  }
+
+  /** Fail the oldest outstanding load. */
+  public reject(): Promise<void> {
+    this.resolvers.shift();
+    this.rejecters.shift()!(new Error('network'));
+
+    return Promise.resolve().then(() => undefined);
+  }
+
+  /** Settle the oldest outstanding load with `count` items. */
+  public settle(count: number, label = 'x'): Promise<void> {
+    const resolve = this.resolvers.shift()!;
+    const items = Array.from({ length: count }, (_, i) => ({ id: `${label}${i}` }));
+
+    resolve(Object.assign(items, { payload: { links: {}, meta: {} } }));
+
+    // Let the .then() chain inside goto()/revalidate() run.
+    return Promise.resolve().then(() => undefined);
+  }
+
+  public seed(count: number, label = 'seed'): void {
+    const items = Array.from({ length: count }, (_, i) => ({ id: `${label}${i}` }));
+    this.pages = [{ number: 1, items: items as any, hasNext: false, hasPrev: false }];
+  }
+
+  public itemIds(): string[] {
+    return this.getPages().flatMap((page) => (page.items as any[]).map((item) => item.id));
+  }
+}
+
 describe('PaginatedListState', () => {
   describe('paramsChanged', () => {
     test('does not reload when called again with semantically identical primitive params', async () => {
@@ -82,4 +137,111 @@ describe('PaginatedListState', () => {
       expect(state.refreshCount).toBe(2);
     });
   });
+
+  /**
+   * `refresh()` empties the list before it asks for anything, which is right
+   * when the user has changed what they are looking at — the old results are
+   * wrong and should go. It is the wrong shape for a background catch-up, where
+   * the results on screen are still valid and only need reconciling: the list
+   * disappears, a spinner takes its place, and the reader waits for a request
+   * they never asked for.
+   */
+  describe('revalidate', () => {
+    test('keeps the current items on screen while the request is outstanding', async () => {
+      const state = new DeferredState();
+      state.seed(20);
+
+      const done = state.revalidate();
+
+      expect(state.itemIds()).toHaveLength(20);
+      expect(state.isInitialLoading()).toBe(false);
+      expect(state.isLoading()).toBe(false);
+
+      await state.settle(20, 'fresh');
+      await done;
+    });
+
+    test('replaces the items once the response arrives', async () => {
+      const state = new DeferredState();
+      state.seed(2);
+      expect(state.itemIds()).toEqual(['seed0', 'seed1']);
+
+      const done = state.revalidate();
+      await state.settle(3, 'fresh');
+      await done;
+
+      expect(state.itemIds()).toEqual(['fresh0', 'fresh1', 'fresh2']);
+    });
+
+    test('requests the first page', async () => {
+      const state = new DeferredState();
+      state.seed(20);
+
+      const done = state.revalidate();
+      await state.settle(20);
+      await done;
+
+      expect(state.loadPageCalls).toEqual([1]);
+    });
+
+    test('does not stack concurrent revalidations', async () => {
+      const state = new DeferredState();
+      state.seed(20);
+
+      const first = state.revalidate();
+      const second = state.revalidate();
+
+      expect(state.loadPageCalls).toEqual([1]);
+
+      await state.settle(20);
+      await Promise.all([first, second]);
+
+      // Once settled, a later call is free to run again.
+      state.revalidate();
+      expect(state.loadPageCalls).toEqual([1, 1]);
+    });
+
+    test('leaves the list intact when the request fails', async () => {
+      const state = new DeferredState();
+      state.seed(2);
+
+      const done = state.revalidate();
+      await state.reject();
+
+      // A failed background refresh must not be able to blank the page the
+      // reader is already looking at.
+      await expect(done).resolves.toBeUndefined();
+      expect(state.itemIds()).toEqual(['seed0', 'seed1']);
+      expect(state.isInitialLoading()).toBe(false);
+    });
+
+    test('a failed revalidation does not block the next one', async () => {
+      const state = new DeferredState();
+      state.seed(2);
+
+      const first = state.revalidate();
+      await state.reject();
+      await first;
+
+      const second = state.revalidate();
+      expect(state.loadPageCalls).toEqual([1, 1]);
+
+      await state.settle(1, 'fresh');
+      await second;
+
+      expect(state.itemIds()).toEqual(['fresh0']);
+    });
+
+    test('an empty result empties the list rather than keeping stale rows', async () => {
+      const state = new DeferredState();
+      state.seed(2);
+
+      const done = state.revalidate();
+      await state.settle(0);
+      await done;
+
+      expect(state.itemIds()).toEqual([]);
+      expect(state.isEmpty()).toBe(true);
+    });
+  });
 });