feat(desktop): re-land virtualized timeline to fix macOS beachball (#1250)

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@sprout-oss.stage.blox.sqprod.co>
Co-authored-by: npub1067mpdna4vy22u9eltmmhtdfw56kww6ve2aze025dtfm4pxg07nqztjfhm <7ebdb0b67dab08a570b9faf7bbada97535673b4ccaba2cbd546ad3ba84c87fa6@sprout-oss.stage.blox.sqprod.co>

Author: 3 people

desktop/src/features/channels/ui/ChannelPane.tsx

@@ -669,9 +669,6 @@ export const ChannelPane = React.memo(function ChannelPane({
             hasComposerOverlay={hasMainComposerOverlay}
             hasOlderMessages={hasOlderMessages}
             isFetchingOlder={isFetchingOlder}
-            layoutShiftKey={
-              useSplitAuxiliaryPane ? (openThreadHeadId ?? "closed") : "overlay"
-            }
             isFollowingThreadById={isFollowingThreadById}
             isMessageUnreadById={isMessageUnreadById}
             personaLookup={personaLookup}

desktop/src/features/messages/lib/scrollConvergence.test.mjs

@@ -0,0 +1,210 @@
+import assert from "node:assert/strict";
+import test from "node:test";
+
+import { CONVERGENCE_FRAME_CAP, convergenceStep } from "./scrollConvergence.ts";
+
+function input(overrides) {
+  return {
+    targetMessageId: "target",
+    indexByMessageId: new Map([["target", 100]]),
+    lastIssuedIndex: null,
+    librarySettled: false,
+    stalledOffTarget: false,
+    framesUsed: 0,
+    ...overrides,
+  };
+}
+
+// --- re-aim / staleness guard ------------------------------------------------
+
+test("convergenceStep: first frame aims at the resolved index, not yet done", () => {
+  const step = convergenceStep(input({ lastIssuedIndex: null }));
+  assert.equal(step.nextIndex, 100);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+test("convergenceStep: re-resolves a shifted index from the map each frame", () => {
+  // A prepend shifted the target from 100 to 105. The library is still chasing
+  // the old index (lastIssuedIndex 100); the reducer must aim at the NEW index
+  // so the adapter re-issues scrollToIndex(105). This is the staleness guard.
+  const step = convergenceStep(
+    input({
+      indexByMessageId: new Map([["target", 105]]),
+      lastIssuedIndex: 100,
+    }),
+  );
+  assert.equal(step.nextIndex, 105);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+test("convergenceStep: target removed mid-settle stops with converged=false", () => {
+  // Target deleted from the map while the loop was chasing it. Terminate so the
+  // adapter clears the highlight instead of chasing a vanished row.
+  const step = convergenceStep(
+    input({
+      indexByMessageId: new Map(), // target gone
+      lastIssuedIndex: 100,
+      framesUsed: 3,
+    }),
+  );
+  assert.equal(step.nextIndex, null);
+  assert.equal(step.done, true);
+  assert.equal(step.converged, false);
+});
+
+// --- settle ------------------------------------------------------------------
+
+test("convergenceStep: library settled while aiming at current index converges", () => {
+  const step = convergenceStep(
+    input({ lastIssuedIndex: 100, librarySettled: true }),
+  );
+  assert.equal(step.nextIndex, 100);
+  assert.equal(step.done, true);
+  assert.equal(step.converged, true);
+});
+
+test("convergenceStep: a settle reported WHILE re-aiming is ignored", () => {
+  // The index just moved (105) but the library reports settled — that settle is
+  // on the OLD index (100), so it must NOT count as convergence. The reducer
+  // keeps going and aims at the new index.
+  const step = convergenceStep(
+    input({
+      indexByMessageId: new Map([["target", 105]]),
+      lastIssuedIndex: 100,
+      librarySettled: true,
+    }),
+  );
+  assert.equal(step.nextIndex, 105);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+test("convergenceStep: aiming at current but not yet settled keeps waiting", () => {
+  // Library is chasing the right index but its offset hasn't stabilized. The
+  // reducer returns the same index (so the adapter re-issues NOTHING — issuing
+  // would reset the library's stableFrames and prevent settling) and waits.
+  const step = convergenceStep(
+    input({ lastIssuedIndex: 100, librarySettled: false }),
+  );
+  assert.equal(step.nextIndex, 100);
+  assert.equal(step.reissue, false);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+// --- off-target stall (liveness) ---------------------------------------------
+
+test("convergenceStep: stalled off-target while aiming at current re-issues same index", () => {
+  // The library's offset stopped moving but never reached the current index's
+  // target (its internal reconcile deadlocked after rows re-measured). The
+  // reducer signals a same-index re-issue to kick it — the loop continues.
+  const step = convergenceStep(
+    input({ lastIssuedIndex: 100, stalledOffTarget: true }),
+  );
+  assert.equal(step.nextIndex, 100);
+  assert.equal(step.reissue, true);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+test("convergenceStep: a stall reported WHILE re-aiming does not re-issue", () => {
+  // The index just moved (105) but the library reports a stall on the OLD index
+  // (100). The reducer re-aims at the new index normally; the stale stall must
+  // NOT trigger a same-index kick (there is no current-index stall to kick).
+  const step = convergenceStep(
+    input({
+      indexByMessageId: new Map([["target", 105]]),
+      lastIssuedIndex: 100,
+      stalledOffTarget: true,
+    }),
+  );
+  assert.equal(step.nextIndex, 105);
+  assert.equal(step.reissue, false);
+  assert.equal(step.done, false);
+  assert.equal(step.converged, false);
+});
+
+test("convergenceStep: a settle takes priority over a concurrent stall flag", () => {
+  // Defensive: the adapter computes settle and stall as mutually exclusive, but
+  // if both arrive, a genuine settle must win (converge) rather than spin on a
+  // pointless re-issue.
+  const step = convergenceStep(
+    input({
+      lastIssuedIndex: 100,
+      librarySettled: true,
+      stalledOffTarget: true,
+    }),
+  );
+  assert.equal(step.done, true);
+  assert.equal(step.converged, true);
+  assert.equal(step.reissue, false);
+});
+
+// --- frame cap ---------------------------------------------------------------
+
+test("convergenceStep: terminates at the frame cap without converging", () => {
+  // A row that never settles (librarySettled stays false) must still stop at the
+  // cap rather than spin forever.
+  const step = convergenceStep(
+    input({
+      lastIssuedIndex: 100,
+      librarySettled: false,
+      framesUsed: CONVERGENCE_FRAME_CAP - 1,
+    }),
+  );
+  assert.equal(step.done, true);
+  assert.equal(step.converged, false);
+  assert.equal(step.nextIndex, 100);
+});
+
+test("convergenceStep: frame cap bounds a perpetually shifting target", () => {
+  // Drive the loop the way the adapter would: the target index moves every
+  // frame, so the library never settles. The loop must terminate at the cap.
+  let lastIssuedIndex = null;
+  let framesUsed = 0;
+  let done = false;
+  let converged = true;
+
+  while (framesUsed < CONVERGENCE_FRAME_CAP + 5) {
+    const movingIndex = 100 + framesUsed; // shifts every frame
+    const step = convergenceStep(
+      input({
+        indexByMessageId: new Map([["target", movingIndex]]),
+        lastIssuedIndex,
+        librarySettled: false,
+        framesUsed,
+      }),
+    );
+    lastIssuedIndex = step.nextIndex;
+    framesUsed += 1;
+    if (step.done) {
+      done = step.done;
+      converged = step.converged;
+      break;
+    }
+  }
+
+  assert.equal(done, true);
+  assert.equal(converged, false);
+  assert.ok(framesUsed <= CONVERGENCE_FRAME_CAP);
+});
+
+test("convergenceStep: converges once a re-aimed index then settles", () => {
+  // Realistic flow: frame 0 aims (lastIssued null -> 100), frame 1 the library
+  // is chasing 100 and reports settled -> converged.
+  const aim = convergenceStep(input({ lastIssuedIndex: null }));
+  assert.equal(aim.nextIndex, 100);
+  assert.equal(aim.done, false);
+
+  const settle = convergenceStep(
+    input({
+      lastIssuedIndex: aim.nextIndex,
+      librarySettled: true,
+      framesUsed: 1,
+    }),
+  );
+  assert.equal(settle.done, true);
+  assert.equal(settle.converged, true);
+});

desktop/src/features/messages/lib/scrollConvergence.ts

@@ -0,0 +1,151 @@
+/**
+ * Pure staleness + termination decision for scrolling a virtualized timeline to
+ * a message that may be far off-screen.
+ *
+ * @tanstack/react-virtual already owns the OFFSET convergence: a single
+ * `scrollToIndex(index)` captures that index in `scrollState`, and its internal
+ * `reconcileScroll` rAF loop re-runs `getOffsetForIndex(index)` every frame —
+ * re-aiming as off-screen rows mount and `measureElement` corrects their
+ * heights — until the offset is stable (or a 5s safety valve fires). We do NOT
+ * recompute offsets; duplicating `getOffsetForIndex` against the library's own
+ * `measurementsCache`/`scrollMargin`/`scrollPadding` would only drift.
+ *
+ * What the library does NOT do: it chases the INDEX captured at call time, with
+ * no concept of a message id. If the data shifts mid-settle — a prepend or a
+ * delete above the target — the captured index now points at the wrong row and
+ * the library happily settles on it. This reducer owns exactly that gap: each
+ * frame it re-resolves the target's CURRENT index from the live map and decides
+ * whether the adapter must re-aim the library, let it settle, or stop.
+ *
+ * Two correctness properties this enforces and the `.mjs` suite gates:
+ *  - The target index is re-resolved by id every frame (never frozen), so a
+ *    concurrent prepend/delete that shifts the target re-aims the library at the
+ *    new index instead of stranding it on the old one.
+ *  - If the target id leaves the data mid-settle (deleted), the loop terminates
+ *    with `converged: false` rather than chasing a vanished row to the cap.
+ *
+ * Plus one liveness property: a large windowed-out jump can leave the library's
+ * own reconcile deadlocked off-target (offset stable but short of the target
+ * after rows re-measured under it). When that happens the reducer signals a
+ * same-index re-issue (`reissue: true`) to restart the library's reconcile —
+ * the single case where re-issuing an unchanged index is correct.
+ */
+
+/** Where a scroll target should land in the viewport. Mirrors the library's align. */
+export type ConvergenceAlign = "start" | "center" | "end";
+
+export type ConvergenceInput = {
+  /** Id of the message to settle on — re-resolved against the map each frame. */
+  targetMessageId: string;
+  /** Live message-id -> item-index map; re-read every frame (staleness guard). */
+  indexByMessageId: Map<string, number>;
+  /**
+   * Index the library is currently chasing (the last index the adapter issued
+   * via `scrollToIndex`), or `null` before the first issue. Lets the reducer
+   * tell a re-aim (index moved) from a steady settle (index unchanged).
+   */
+  lastIssuedIndex: number | null;
+  /**
+   * Whether the library's offset reached the current index's target this frame
+   * and stopped moving. Only meaningful once the library is chasing the CURRENT
+   * index; a settle reported while re-aiming is ignored.
+   */
+  librarySettled: boolean;
+  /**
+   * Whether the library's offset has stopped moving but is NOT at the current
+   * index's target — it stalled mid-reconcile (its internal re-aim deadlocked
+   * after rows re-measured under it). The reducer kicks it with a fresh re-issue
+   * at the same index. Mutually exclusive with `librarySettled`.
+   */
+  stalledOffTarget: boolean;
+  /** Frames already spent in the loop (the adapter increments per rAF). */
+  framesUsed: number;
+};
+
+export type ConvergenceDecision = {
+  /**
+   * Index the adapter should be aiming the library at, or `null` when the
+   * target is gone. The adapter only re-issues `scrollToIndex` when this differs
+   * from `lastIssuedIndex`, so a steady settle issues no redundant scroll (which
+   * would reset the library's `stableFrames` and prevent it from ever settling).
+   */
+  nextIndex: number | null;
+  /**
+   * True when the adapter must re-issue `scrollToIndex(nextIndex)` even though
+   * the index is unchanged — used to kick the library out of an off-target
+   * stall. A normal steady settle leaves this false so no redundant scroll
+   * resets the library's `stableFrames`.
+   */
+  reissue: boolean;
+  /** True once the loop must stop (settled, target gone, or frame cap hit). */
+  done: boolean;
+  /** True only when the loop stopped because the target row actually settled. */
+  converged: boolean;
+};
+
+/**
+ * Hard cap on frames so a perpetually re-measuring row, or a target whose index
+ * keeps shifting, can't spin the loop forever. The library has its own 5s valve;
+ * this is the adapter-side bound expressed in frames for deterministic testing.
+ */
+export const CONVERGENCE_FRAME_CAP = 32;
+
+/**
+ * One frame of the convergence loop. Pure: given the live map and the library's
+ * settle state, decides the index to aim at and whether to stop.
+ */
+export function convergenceStep(input: ConvergenceInput): ConvergenceDecision {
+  const currentIndex = input.indexByMessageId.get(input.targetMessageId);
+
+  // Target left the data mid-settle (deleted) — stop without converging so the
+  // adapter clears the highlight instead of chasing a vanished row.
+  if (currentIndex === undefined) {
+    return { nextIndex: null, reissue: false, done: true, converged: false };
+  }
+
+  const aimingAtCurrent = input.lastIssuedIndex === currentIndex;
+
+  // The library only settles meaningfully once it is chasing the CURRENT index.
+  // A settle reported while we are still re-aiming (index just moved) is stale.
+  if (aimingAtCurrent && input.librarySettled) {
+    return {
+      nextIndex: currentIndex,
+      reissue: false,
+      done: true,
+      converged: true,
+    };
+  }
+
+  // Frame cap: accept the best index we have rather than spin forever on a row
+  // whose height never settles or a target whose index keeps shifting.
+  if (input.framesUsed + 1 >= CONVERGENCE_FRAME_CAP) {
+    return {
+      nextIndex: currentIndex,
+      reissue: false,
+      done: true,
+      converged: false,
+    };
+  }
+
+  // Library stalled off-target: its offset stopped moving but never reached the
+  // current index (its internal reconcile deadlocked after rows re-measured).
+  // Re-issue the SAME index to restart its reconcile — the one case where a
+  // same-index re-issue is correct rather than a stableFrames-resetting bug.
+  if (aimingAtCurrent && input.stalledOffTarget) {
+    return {
+      nextIndex: currentIndex,
+      reissue: true,
+      done: false,
+      converged: false,
+    };
+  }
+
+  // Either the index moved (adapter will re-issue scrollToIndex) or the library
+  // is still settling on the current index (adapter issues nothing, just waits).
+  return {
+    nextIndex: currentIndex,
+    reissue: false,
+    done: false,
+    converged: false,
+  };
+}