Skip to content

fix(desktop): keep history prepend anchored on the visible row#1113

Closed
tlongwell-block wants to merge 3 commits into
mainfrom
max/channel-scroll-jump-fix
Closed

fix(desktop): keep history prepend anchored on the visible row#1113
tlongwell-block wants to merge 3 commits into
mainfrom
max/channel-scroll-jump-fix

Conversation

@tlongwell-block

@tlongwell-block tlongwell-block commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

The bug

Scrolling up in a channel to load older history yanks the viewport to the
oldest just-loaded message instead of keeping the row the user was reading
pinned to the same screen position. Recurring regression; previous "fixes"
relied on math against container.scrollTop, which is stale on macOS
WKWebView mid-wheel (root cause documented in
RESEARCH/BUZZ_SCROLL_PREPEND_2026-06-14.md and Element/Matrix
docs/scrolling.md).

The fix (two parts, in useLoadOlderOnScroll.ts + MessageTimeline.tsx)

  1. Anchor on a real visible DOM node, not on a scrollTop value.
    When the top sentinel intersects, findVisibleMessageAnchor walks
    [data-message-id] rows and picks the one whose top-offset is closest
    to min(12px, containerHeight / 2) — the row the user is most likely
    reading. We snapshot { id, topOffset, fallbackScrollHeight } in a
    ref while we wait for the prepended rows to commit.

  2. Restore via relative scrollBy(delta), no scrollTop read.
    In a layout-effect keyed on the actual rendered first-id + count
    changing (i.e., the prepend has committed), we read the anchor row's
    new top-offset and call restoreScrollBy(nextTopOffset - savedTopOffset).
    This is the difference between "passes headless Chromium" and
    "actually fixed on macOS" — no stale read, no read-then-write.

MessageTimeline.tsx switches the consumer from restoreScrollPosition
(absolute write) to restoreScrollBy (relative), and changes the
fetch-older spinner from an in-flow block to an absolute overlay so the
spinner's own mount doesn't shove the topmost message down and confuse
the anchor math.

useTimelineScrollManager.ts adds the restoreScrollBy primitive
(container.scrollBy(0, delta)) alongside the existing scroll API.

The regression gate

Mock-bridge invariant test in messaging.spec.ts ("older-history
prepend keeps the visible row pinned"):

  • Seeds a history-jump mock channel with 240 messages — enough to
    force at least one older-fetch round-trip past the 200-msg initial
    load.
  • New historyDelayMs config knob delays only the older-history REQ
    with until (not initial loads), giving a deterministic snapshot
    window after the sentinel fires but before the prepend commits.
  • Uses real mouse.wheel events, snapshots the anchor row by
    [data-message-id] rect against the timeline container, awaits
    prepend commit via first-id-changed + count-grew, then asserts
    ≤ 4px drift after layout-effect restore.
  • Accounts for scrollTop drift between snapshots so it doesn't
    false-pass when scrollBy is wired correctly but the user kept
    wheeling.

A nightly-gated scroll-prepend-stability.spec.ts is included for
manual real-relay smoke (excluded from both smoke and integration
testMatch in playwright.config.ts). Run manually with
pnpm -C desktop exec playwright test scroll-prepend-stability.spec.ts
against a relay started with BUZZ_RECONCILE_CHANNELS=1.

Verified

  • Positive test green against the fixed build (~3s, single worker).
  • Negative test (restore neutered to void delta) fails as expected —
    confirms the gate catches the regression.
  • pnpm typecheck + pnpm check (biome) clean.
  • Build clean.

What's NOT done in this PR

  • Manual macOS wheel-scroll repro. Headless Chromium ≠ WKWebView.
    A green CI run here is necessary, not sufficient. @tlongwell-block
    pulling this branch on macOS and confirming the jump is gone is the
    load-bearing pass before this is "shipped."
  • Empty-older-page guard. If fetchOlder resolves with zero new rows,
    hasCommittedPrepend never goes true, the layout effect early-returns
    leaving pendingRestoreRef set, and the observer never re-arms — older
    loading silently wedges. Happy path is covered; this is a follow-up
    commit on the same PR (resume observing if the fetch resolves with no
    commit). Landed as da1606c2 — converts the fetch handler to
    .then(onFulfilled, onRejected) and uses a single-rAF settle fence with
    an identity check on the captured pending to re-arm only on a true
    no-commit, never racing a real commit's restore.

npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta and others added 3 commits June 18, 2026 11:04
Co-authored-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: npub1mprnacetjua2xx3p5eddmhxyk6wv929ymm5py8kd2xfxurxahspqqlgyta <d8473ee32b973aa31a21a65adddcc4b69cc2a8a4dee8121ecd51926e0cddbc02@sprout-oss.stage.blox.sqprod.co>
…anchor invariant

Adds the regression gate for the channel scroll-jump fix in the previous
commit. Two pieces:

1. **Mock-bridge fixture in `e2eBridge.ts`**: a `history-jump` channel
   seeded with 240 sequential messages, enough to force at least one
   older-fetch round-trip past the initial 200-message load. A new
   `historyDelayMs` config knob (wired through `helpers/bridge.ts`)
   delays only the older-history `REQ`/`until` path — initial loads
   stay snappy — so the test has a deterministic window to snapshot the
   visible anchor row after the sentinel fires but before the prepended
   rows commit.

2. **`messaging.spec.ts` invariant test** ("older-history prepend keeps
   the visible row pinned"): real `mouse.wheel` events scroll near the
   top sentinel, the anchor row is snapshotted by `[data-message-id]`
   rect against the timeline container, the prepend is awaited via
   first-id-changed + count-grew, and drift is asserted ≤ 4px after
   the layout-effect restore settles. Accounts for `scrollTop` drift
   between snapshots so it doesn't false-pass when `scrollBy` is wired
   correctly but the user kept wheeling.

Also adds `scroll-prepend-stability.spec.ts` as a nightly-gated /
manual real-relay smoke artifact. Excluded from both the `smoke` and
`integration` `testMatch` arrays in `playwright.config.ts` so it never
runs in the default suite. Run manually with
`pnpm -C desktop exec playwright test scroll-prepend-stability.spec.ts`
against a relay started with `BUZZ_RECONCILE_CHANNELS=1`. The
top-of-file docblock spells out why the mock-bridge case is the
canonical gate (deterministic, no seed dependency) and why a green
Chromium run is necessary but not sufficient (WKWebView-specific
stale-`scrollTop` hazard requires manual macOS pass).

Verification: positive test green against the fixed build (~3s).
Negative test (restore neutered) fails as expected — confirms the gate
catches the regression. Both tsc and biome clean.

Co-authored-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@sprout-oss.stage.blox.sqprod.co>
If `fetchOlder` resolves successfully without a committed prepend (zero
new rows, all-duplicates page, or an early-returned fetch that no-ops),
the layout effect never fires — its deps (`renderedFirstMessageId`,
`renderedMessageCount`) don't change — and `pendingRestoreRef` stays
set. The IntersectionObserver's `observe()` guards on
`pendingRestoreRef.current` and silently refuses to re-arm, wedging
older-history loading until the user navigates away.

Most empty-page cases are already covered indirectly: `useFetchOlderMessages`
flips `hasOlderMessages` to false when the relay returns fewer than the
batch size or no new oldest, which triggers the outer effect's cleanup
path. The wedge described above is the narrow case where the fetch
resolves with no prepend AND `hasOlderMessages` stays true — e.g., a
transient empty page from the relay during pagination.

Convert the fetch handler from `.catch()` to a unified
`.then(onFulfilled, onRejected)`. On fulfilled, wait one animation
frame to give React time to commit any real prepend (the layout effect
runs synchronously inside that commit and clears `pendingRestoreRef`).
If our captured pending is still the live one after the frame, treat it
as a no-commit fetch: clear pending and call `resumeObservingRef.current`
to re-arm. Identity check (`pendingRestoreRef.current !== restorePending`)
prevents a stale rAF from clobbering a fresh pending created by a
subsequent intersect after a real commit re-armed the observer mid-frame.

Reject path is unchanged in behavior.

Follow-up to #1113 per Eva's PR review.

Co-authored-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@sprout-oss.stage.blox.sqprod.co>
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.

1 participant