flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) - #407
Conversation
Anchor restoration can scroll before Virtua measures the rows beneath the anchor. After #364 changed row grouping and heights, WebKit could deliver that scroll event while the estimated list was still short, so recordPosition saw a near-bottom offset and turned the restored reading position into bottom follow. Later measurements then kept the timeline at the end. A restored anchor now stays a reading position until reader input clears it. Signed-off-by: Logan Johnson <loganj@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Star Lord — automated source review via Wes’s account
No actionable production defects found in this revision. Non-blocking source review, not approval or merge authorization.
- Head:
d8c40d33bc2e257dfaf106e1c43560b6014d413b - Base:
61da2662d74aeea46ea8344fb4dc6e247baaab68
The guard in ChannelTimeline.tsx:208 preserves non-follow reading intent while a restored anchor is active, before that observation reaches both savedPosition and follow. This also preserves the restore effect’s existing cleanup/correction path. Reader gestures clear the anchor before recording a new position; explicit message navigation and local-send reveal clear it as well. The ordinary bottom-follow path remains unchanged when no restored anchor exists.
I traced restoration, late row measurements, row refresh/cleanup, reader input, local-send reveal, and the Channels/Sessions callers. Separately checked the existing loading/error/retry and focused-row retention paths: this change introduces no new focus transfer or retry state; native focus/scroll behavior remains unexercised by this review.
Optional test follow-up: the new case uses the existing hook-mocked harness. AGENTS.md and docs/contributing.md direct new component lifecycle coverage to real React; the nearby ChannelTimeline.restore.test.tsx already provides that boundary. Moving this scenario there would cover React cleanup/rerender ordering rather than the substitute hook scheduler, without migrating the entire older suite. The new assertions do target the reported accidental-bottom-follow state; this is not an additional demonstrated production defect.
Public-material check: reviewed the two-file diff, the single commit message and the public PR description; no attached images were present and no internal coordination URLs, workspace identifiers or secrets were identified in those surfaces. Legitimate public links and commit authorship attribution are not treated as leaks.
Validation limits: source only; no PR code, tests, builds or app were executed. Reviewed immutable Git blobs, not live-working-tree content; git diff --check passed for the pinned comparison. The existing browser reload/append journey and the author’s reported WebKit/Chromium runs are relevant, but I did not reproduce them. One hosted-check snapshot showed DCO/Semgrep/zizmor passing, JavaScript/Rust/browser jobs still running, and Windows validation skipped. This does not establish CI completion, packaged/native correctness, or elimination of the WebKit flake.
* origin/main: (58 commits) flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407) Replace fixed browser-test waits with conditions, gates and the clock (#373) feat(updates): show installed version in Software Updates settings (#430) fix(desktop): allow deep-link delivery to the main webview (#432) feat(shell): open your profile from the account menu avatar (#390) Polish top bar and animate contextual sidebar toggle (#360) fix(profiles): preserve nonlocal agent identity in profile fallback (#327) test(agents): pause the status poll around the failed-Stop checks (#431) fix(sidebar): paint channel rows with the scroller contents (#428) feat(channels): archive and delete channels from settings (#385) feat(updates): add in-app auto-updates with restart toast (#312) fix(ui): keep background loading from shifting populated views (#418) Improve member and agent identity previews (#412) Fix initial emoji autocomplete selection (#419) Add community membership settings (#348) Keep nested replies compact and place actions above message text (#367) Import an exact inventory identity from its selected source with retry (#288) ci: add gated macOS preview updater feed promotion (#414) Set up incomplete inventory identities through a working Use here dialog (#287) Show saved local and relay inventory while retaining existing import controls (#286) ...
* origin/main: (58 commits) flake fix: keep restored reading anchor out of bottom follow (WebKit scroll measurement) (#407) Replace fixed browser-test waits with conditions, gates and the clock (#373) feat(updates): show installed version in Software Updates settings (#430) fix(desktop): allow deep-link delivery to the main webview (#432) feat(shell): open your profile from the account menu avatar (#390) Polish top bar and animate contextual sidebar toggle (#360) fix(profiles): preserve nonlocal agent identity in profile fallback (#327) test(agents): pause the status poll around the failed-Stop checks (#431) fix(sidebar): paint channel rows with the scroller contents (#428) feat(channels): archive and delete channels from settings (#385) feat(updates): add in-app auto-updates with restart toast (#312) fix(ui): keep background loading from shifting populated views (#418) Improve member and agent identity previews (#412) Fix initial emoji autocomplete selection (#419) Add community membership settings (#348) Keep nested replies compact and place actions above message text (#367) Import an exact inventory identity from its selected source with retry (#288) ci: add gated macOS preview updater feed promotion (#414) Set up incomplete inventory identities through a working Use here dialog (#287) Show saved local and relay inventory while retaining existing import controls (#286) ... Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Flake fix for main. The WebKit
Browser measurementsjob failed in run 36583908128 (job 109458992105). Inscroll.spec.mjs:97, after a reload, the saved anchor ended up 650px from its saved Y and the timeline was scrolled to the bottom. The previous main run (36581079093) passed. The only commit in between is #364.Root cause
When
ChannelTimelinerestores a saved reading anchor, it scrolls withscrollToIndexbefore Virtua has measured the rows beneath that anchor. After #364 changed row grouping and heights, WebKit can deliver that restoration scroll event while the estimated list is still short:scrollTop=452withscrollHeight=1140.recordPositionread that asbottom: trueand setfollow.current = true. The restore effect's cleanup only keeps the restoration while!follow.current, so the restoration was dropped. Each later measurement then pinned the timeline to the end.Instrumented failing sequence (local WebKit):
Fix
While a restored anchor is active (
restoredAnchoris set and has not been cleared by reader input),recordPositionno longer marks the position as bottom follow. Reader gestures already clearrestoredAnchorbefore they record a position, so moving to the bottom by hand still resumes follow.This PR doesn't change any tests, timeouts or browser projects. It adds a unit regression test that fails without the fix.
Validation (local, macOS, Playwright 1.63 WebKit/Chromium)
ae687c7c:scroll.spec.mjs:97onwebkit-measurementsfailed with the sameReceived: 650in about 1 of 6 runs, plus 1 of 5 instrumented runs.d8c40d33):scroll.spec.mjs:97passed 12/12 isolated WebKit runs and 2/2 Chromium runs.chromium-measurementsandwebkit-measurements(CI config): 7 passed.initial-position,history-loading,image-scroll,new-messageandmessageson Chromium and WebKit: 56 passed.vitest run: 421 files, 5121 tests passed. The new test fails on the base.pnpm typecheckpassed, and biome is clean on the changed files.