fix(webui): refuse stale-anchor scroll restore during streaming (#5637) - #5666
5 commits merged into
Conversation
|
| Filename | Overview |
|---|---|
| static/ui.js | Adds scrollHeightAtCapture to anchor object, two stale-anchor guards (realign + fallback), and _isIOSWebKit/_isTouchLikeMessageViewport predicates — logic is sound, desktop/iOS exclusions are correctly gated, all code paths are tested. |
| tests/test_issue5637_stale_anchor_guard.py | 16 node-harness tests covering refuse/allow cases for both guards, desktop and iOS exclusions, the matchMedia-clobbered inline override, backward-compat with legacy snapshots, and active-intent pass-through for both guard paths; mutation annotations are present. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_renderMessagesWithScrollSnapshot] -->|capture| B[_captureMessageScrollSnapshot\nrecords scrollHeight, anchor\nwith scrollHeightAtCapture]
B --> C[renderMessages DOM update\ncontent may grow above viewport]
C --> D[_restoreMessageScrollSnapshotSameFrame]
D --> E{pinned follower?}
E -->|yes| F[tail-relative restore]
E -->|no| G{anchor restore\n_restoreMessageViewportAnchor}
G --> H{NEW GUARD:\ntouchHold AND\ngrewSinceCapture AND\n!activeIntent AND\ndelta > 8px?}
H -->|yes| I[return false - refuse realign]
H -->|no| J[scrollTop realign]
G -->|no anchor / refused| K{fallback: absolute\nsnapshot.top write}
K --> L{NEW GUARD:\nfbTouchHold AND\n!pinned AND\ngrewSinceSnap AND\n!fbActiveIntent AND\ndelta > 8px?}
L -->|yes| M[return early - set userUnpinned=true]
L -->|no| N[el.scrollTop = target]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[_renderMessagesWithScrollSnapshot] -->|capture| B[_captureMessageScrollSnapshot\nrecords scrollHeight, anchor\nwith scrollHeightAtCapture]
B --> C[renderMessages DOM update\ncontent may grow above viewport]
C --> D[_restoreMessageScrollSnapshotSameFrame]
D --> E{pinned follower?}
E -->|yes| F[tail-relative restore]
E -->|no| G{anchor restore\n_restoreMessageViewportAnchor}
G --> H{NEW GUARD:\ntouchHold AND\ngrewSinceCapture AND\n!activeIntent AND\ndelta > 8px?}
H -->|yes| I[return false - refuse realign]
H -->|no| J[scrollTop realign]
G -->|no anchor / refused| K{fallback: absolute\nsnapshot.top write}
K --> L{NEW GUARD:\nfbTouchHold AND\n!pinned AND\ngrewSinceSnap AND\n!fbActiveIntent AND\ndelta > 8px?}
L -->|yes| M[return early - set userUnpinned=true]
L -->|no| N[el.scrollTop = target]
Reviews (5): Last reviewed commit: "fix(webui): exclude iOS WebKit from stal..." | Re-trigger Greptile
|
Thanks for the review — addressed all three points in the follow-up commit:
|
🔬 Gate certification — RED ⛔ (the mobile stale-anchor guard runs on ALL platforms → breaks DESKTOP scroll-restore, which has no native overflow-anchor to fall back to — 2 CORE)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean |
| Codex | SHIP-WITH-FIXES — 2 CORE (cross-platform, harness-verified) |
| Fable-UX | SHIP-UX (mobile surface — correct for what it reviewed; the break is desktop) |
| Full pytest suite | ✅ 12193 passed, 0 failed (+ 233 scroll/anchor/pin tests) |
Findings
⛔ CORE (Codex, static/ui.js:1051) — desktop unpinned streaming anchor-restore stops preserving the reader's position: the new growth/no-intent refusal runs for EVERY production anchor because scrollHeightAtCapture is now always set (captured at ui.js:960), and it assumes native overflow-anchor:auto will hold the viewport. But desktop .messages uses overflow-anchor:none → a legitimate realign after above-viewport height growth is refused and NO scroll write happens → desktop reader gets yanked/mispositioned. Fix: gate the refusal on _browserOverflowAnchorActive(container) — only refuse when the browser anchoring layer can actually hold the viewport; otherwise keep the existing _restoreMessageViewportAnchor scrollTop realign.
⛔ CORE (Codex, static/ui.js:13233) — desktop same-frame fallback restore also skipped with no native anchoring: the fallback guard has the same "let browser overflow-anchor hold" assumption → on desktop, anchor-failed restores after content growth leave the viewport at the wrong absolute position AND force _messageUserUnpinned=true. Fix: apply the stale-snapshot refusal only when _browserOverflowAnchorActive(el) is true; otherwise preserve the previous absolute fallback write.
Why the tests + Fable missed it: the #4295 anchor-restore harness uses a legacy anchor WITHOUT scrollHeightAtCapture, while production captures it — so the guard never fires in the test. Fable reviewed the mobile scroll-feel (correct: SHIP-UX there); the regression is desktop-only, where there's no native overflow-anchor to compensate the refused restore.
Recommendation to the next agent / author
RED — gate-fail/changes-requested (2 CORE): gate both stale-anchor refusals on _browserOverflowAnchorActive(container/el) so they fire ONLY where native overflow-anchor:auto can actually hold the viewport (mobile); on desktop (overflow-anchor:none) keep the existing semantic-realign / absolute-fallback scroll write. The mobile fix is good and addresses the real secondary #5637 JS cause Fable flagged on #5639 — it just can't assume browser-native anchoring universally. Add a desktop-path regression using the production snapshot shape (with scrollHeightAtCapture) so this is caught. concept 4/5 (correct mobile fix, cross-platform gap). Author @allenliang2022 (new, T1). crit=3. (Gate value: Fable SHIP-UX'd the mobile surface; Codex caught the desktop break by building a harness with the production anchor shape — the two legs covered different platforms, exactly the point of running both. This is the 3rd #5637-family fix (#5635/#5638/#5666) — all need the same land-order reconciliation.)
_Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Stale-anchor-during-streaming refusal is correct for mobile (overflow-anchor:auto) but runs on ALL platforms since scrollHeightAtCapture is always set → desktop (overflow-anchor:none) refuses a legitimate restore with nothing to compensate = 2 CORE (ui.js:1051 realign, ui.js:13233 fallback). Fix: gate both on browserOverflowAnchorActive. Fable SHIP-UX (mobile), Codex caught desktop via harness; suite green (harness uses legacy anchor w/o scrollHeightAtCapture). 3rd #5637 fix — land-order w/ #5635/#5638. Cert valid for sha:34f9d55e.
…uena#5637) A residual mobile scroll jump-back remained after nesquena#5638: while the reader is up in history during a live stream, the anchor captured for a same-frame restore goes stale as the streaming chunk grows content ABOVE the viewport. The anchor's captured topOffset (and the absolute snapshot.top) no longer map to the same content, so realigning to them yanks a still reader backward by a few hundred px per tick. The existing snapshot.userUnpinned===true fallback skip does not cover it: the scrollHeight-collapse scroll event re-pins the state machine (flips userUnpinned back to false) mid-stream, so both the semantic realign (_restoreMessageViewportAnchor) and the absolute snapshot.top fallback still fire. Fix: two guards, both keyed on content-growth-since-capture + absence of recent real input intent, NOT on a scrollTop diff — on an overflow-anchor:auto container the browser itself writes scrollTop to compensate above-viewport growth, so a still reader's scrollTop is not stationary; _recentMessage*ScrollIntent instead reflects genuine touch/wheel/key input, which the browser's anchor layer never sets. - _captureMessageViewportAnchor records scrollHeightAtCapture (+scrollTopAtCapture). - _restoreMessageViewportAnchor refuses the realign when content grew since capture, there is no recent intent, and the delta would move scrollTop >8px. - the absolute snapshot.top fallback in _restoreMessageScrollSnapshotSameFrame mirrors the same guard. An actively scrolling reader (recent intent) keeps the legitimate restore; a fresh anchor (no growth) and legacy snapshots without the captured geometry are unaffected. Adds tests/test_issue5637_stale_anchor_guard.py (7 node-harness cases; two are mutation-checked to fail if either guard is removed).
…ck intent test (nesquena#5637) Greptile review follow-ups (no behavior change): - Remove the captured-but-never-read scrollTopAtCapture field from _captureMessageViewportAnchor; only scrollHeightAtCapture is consulted. - Rename _grewAbove -> _grewSinceCapture in the realign guard (the check is overall scrollHeight growth since capture, not specifically above-viewport), matching the fallback guard's _grewSinceSnap. - Add a mutation-checked fallback active-intent test: content grew but the reader has recent real input intent, so the absolute snapshot.top restore is kept.
…ena#5637) The two streaming stale-anchor guards added for nesquena#5637 refuse a scroll restore and rely on the browser's native overflow-anchor layer to hold the viewport. That layer is only active where .messages computes to overflow-anchor:auto (touch viewports). On hover+fine-pointer desktops .messages is overflow-anchor:none, so refusing the restore leaves nothing to hold the reader -> the desktop reader is yanked after above-viewport growth (the same jump the guards fix on mobile), and the fallback also latches _messageUserUnpinned=true. Gate both refusals on _isTouchLikeMessageViewport(container), a matchMedia('(pointer:coarse)') predicate (falling back to the computed overflow-anchor probe) so they fire only where native anchoring can actually hold the viewport. Desktop keeps its semantic scrollTop realign and absolute snapshot.top fallback. matchMedia is used rather than the computed-anchor probe alone because the realign temporarily writes inline overflowAnchor:none for its own scroll write, which a computed probe would misread mid-realign. Add two desktop regression tests (touch_like=False) covering the exact stale-anchor case on a no-native-anchor viewport; both are mutation-checked (dropping the touch gate makes them fail).
34f9d55 to
cab238c
Compare
|
Thanks for the gate cert — you're right, and I've fixed both CORE findings. What was wrongBoth stale-anchor refusals (realign at The fixBoth refusals are now gated on I used a Desktop regression tests (mutation-checked)Added two desktop-path tests that use the production anchor/snapshot shape (with
Both are mutation-checked: dropping the Rebased onto current master; the three prior review points (dead |
Re-review of
|
…best-effort (nesquena#5637) Address the two non-blocking observations from re-review of the touch-gate fix: 1. Add a direct unit test for _isTouchLikeMessageViewport itself — the guard-wiring tests stub the predicate via a boolean, which validates that the _touchHold term gates the refusal but not the predicate's own mid-realign stability (the claim that motivated choosing matchMedia over the computed overflow-anchor probe). The new _predicate_harness exercises the real predicate + _browserOverflowAnchorActive with mocked matchMedia/getComputedStyle: - test_predicate_stays_true_on_touch_when_inline_anchor_clobbered_to_none: on a pointer:coarse device whose inline overflowAnchor was clobbered to 'none' by a prior realign tick, the predicate must still report touch=true. Mutation-checked: reverting the predicate to the bare computed probe makes exactly this test fail. - test_predicate_false_on_desktop_fine_pointer / _falls_back_to_computed_probe_without_matchmedia cover the desktop and no-matchMedia paths. 2. Note in the predicate comment that the no-matchMedia fallback to the computed probe is best-effort (matchMedia('(pointer:coarse)') is universally supported in every targeted browser, so the primary path is what runs).
|
Thanks for the re-review — I've addressed both non-blocking observations in 1. Predicate mid-realign stability now has direct coverage. You're right that the
2. Fallback best-effort note. Added a comment on the no- Full file is 13 tests now, all green; the desktop-regression and predicate-stability cases are both mutation-checked. |
🔬 Gate certification — RED ⛔ (round-1 desktop regression fixed, but the touch predicate conflates Android with iOS — on iOS overflow-anchor is INERT so refusing the restore leaves nothing to hold the reader — 1 CORE, iOS)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean |
| Codex (both platforms) | SAFE TO SHIP — 0 findings (verified desktop + Android; did not flag the iOS overflow-anchor-inert nuance) |
| Fable-UX (cross-device) | SHIP-WITH-UX-FIXES — 1 real iOS gap + a merge-blocking real-device recording requirement |
| Full pytest suite | ✅ 12209 passed, 0 failed (+ 238 scroll/anchor/pin, incl. new test_realign_allows_on_desktop_no_native_anchor) |
Findings
✅ Round-1 desktop regression FIXED: both stale-anchor refusals now require _isTouchLikeMessageViewport (matchMedia('(pointer:coarse)')), which is false on hover+fine-pointer desktop → desktop keeps the explicit semantic realign + absolute restore (byte-identical to pre-round-1). Codex + Fable both confirm; the mid-realign predicate-stability test proves the transient inline overflow-anchor:none (round-1's classification trap) can't clobber the matchMedia probe. Android Chrome (pointer:coarse + working overflow-anchor): refusal + native anchor holds — matches the issue's real-device telemetry. Sound.
⛔ CORE (Fable, iOS gap) — the refusal fires on iOS where overflow-anchor is inert: pointer:coarse is true on iOS/iPadOS, so the guard refuses the restore there — but the refusal's own premise ("let the browser overflow-anchor hold") is FALSE on iOS WebKit per the repo's own static/style.css:5838 comment. On iOS the semantic realign IS the compensation for above-viewport height changes; refusing it leaves nothing holding a still, scrolled-up reader (narrow: needs above-viewport growth — history remeasure / image load above viewport — while scrolled up; the >8px deadband + no-intent requirement limit it further). The node harness structurally CANNOT test this ("native anchor holds" is an assumed postcondition it mocks, never observed). Fix: don't treat "touch" as one platform — the refusal is only safe where overflow-anchor actually holds (Android), NOT iOS WebKit; either detect iOS/WebKit and keep the semantic realign there, or verify on a real iOS device that refusing is acceptable. AND: this needs a real iOS-Safari recording before merge (structurally untestable in the suite).
Recommendation to the next agent / author
RED — gate-fail/changes-requested (1 CORE, iOS): the touch predicate conflates Android (overflow-anchor works → refusal safe) with iOS WebKit (overflow-anchor inert per style.css:5838 → refusing leaves the reader unheld). Split the platform check so the stale-anchor refusal fires only where native overflow-anchor actually holds the viewport (Android), and keep the semantic realign on iOS; OR prove on a real iOS device that the deadband+no-intent guard makes refusal acceptable there. The desktop fix is correct and the Android path is sound — this is the third platform (iOS) needing the same "does native anchoring actually hold HERE" gate. concept 4/5 (converging; the "mobile is not one platform" nuance is the last mile). Author @allenliang2022 (new, T1). crit=3. (Gate value: Codex SAFE'd desktop+Android; Fable caught the iOS gap by knowing the repo's own CSS comment that overflow-anchor is inert on iOS WebKit — exactly the kind of platform-specific browser-behavior knowledge the advisor leg brings. This is also 3rd #5637-family fix — land-order with #5635/#5638.)
_Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Round-1 desktop regression fixed (both guards require isTouchLikeMessageViewport; desktop byte-identical, Codex SAFE + desktop regression test). BUT Fable: matchMedia('(pointer:coarse)') is true on iOS too, and overflow-anchor is INERT on iOS WebKit (repo's own style.css:5838) → refusing the restore on iOS leaves nothing to hold a scrolled-up reader (CORE, narrow). Fix: split Android (anchor works, refuse) vs iOS (anchor inert, keep realign); real-iOS recording needed (node harness can't test the assumed "native anchor holds"). Suite green 12209 + 238 scroll. Cert valid for sha:5a5f5448.
…nchor is inert there (nesquena#5637) Round-2 gate cert (iOS CORE): the touch predicate _isTouchLikeMessageViewport used matchMedia('(pointer:coarse)'), which is true on BOTH Android and iOS. But overflow-anchor is inert on iOS WebKit (the repo's own static/style.css mobile content-visibility block documents this — it deliberately does not set overflow-anchor:none because it is a no-op on iOS and re-opens the nesquena#4856/nesquena#5338 jump on Android). So on iOS the stale-anchor refusal fired but its premise (let the native overflow-anchor layer hold the viewport) is false → a scrolled-up iOS reader was left unheld after above-viewport growth, the same class as the round-1 desktop regression, one platform over. Split the platform check: add _isIOSWebKit() (classic iPhone/iPod/iPad UA, plus iPadOS 13+ which masquerades as MacIntel but has maxTouchPoints>1 unlike a real Mac) and exclude it from _isTouchLikeMessageViewport. The refusal now fires ONLY on Android (pointer:coarse AND overflow-anchor actually works); desktop and iOS both keep the explicit semantic realign / absolute snapshot.top restore. Tests: 3 new predicate cases (iPhone, iPadOS-as-Mac, Android control), all mutation-checked — removing the _isIOSWebKit exclusion fails exactly the iOS cases while Android/desktop stay green; broadening _isIOSWebKit to any touch device fails the Android control. The Node harness forces the navigator mock via Object.defineProperty because Node 18+ ships a built-in read-only navigator that a plain assignment silently ignores. Note: iOS Safari cannot be exercised by the Node harness (the 'native anchor holds' postcondition is mocked, never observed), so this needs a real iOS-Safari recording before merge per the gate cert.
|
Thanks — fixed the iOS gap in The fixSplit the platform check instead of treating "touch" as one platform. Added
Tests3 new predicate cases, all mutation-checked:
(The Node harness forces the On the real-iOS recordingYou're right that the suite can't observe the "native anchor holds" postcondition — it's mocked, never exercised. The predicate logic is now unit-covered, but I can't provide the real iOS-Safari recording myself: I only have an Android device to reproduce on. So this last leg is genuinely unverified on-device for iOS from my side — if you or anyone with an iOS device can confirm a scrolled-up reader stays put on iOS during above-viewport growth, that would close it. The fix is conservative (iOS now behaves exactly like desktop, which is the known-good pre-round-1 path), so the risk is bounded to "iOS keeps the prior behavior" rather than a new iOS-specific code path. |
🔬 Gate certification — GREEN ✅ · CONVERGED (round 3 — complete platform matrix: desktop + Android + iOS all correct) · ⏸️ visible scroll → Nathan sign-offCertified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean |
| Codex (complete platform matrix) | SAFE TO SHIP — 0 findings |
| Fable-UX | SHIP-UX — all 3 platforms hold the reader during streaming |
| Full pytest suite | ✅ 12212 passed, 0 failed (+ 241 scroll/anchor/pin, incl. new iOS-exclusion tests) |
Findings — complete platform matrix, all 3 rounds closed
✅ The refusal now fires ONLY where native overflow-anchor actually holds the viewport:
- Desktop (fine pointer):
_isTouchLikeMessageViewportfalse → refusal inert → semantic realign kept (round-1 fix). - Android (coarse pointer + working overflow-anchor): refusal fires → native anchor holds (the Mobile scroll jump-back: off-screen .msg-row content-visibility (#5541) collapses scrollHeight → browser clamps scrollTop #5637 fix works).
- iOS/iPadOS WebKit (coarse pointer BUT overflow-anchor INERT): now EXCLUDED via the new
_isIOSWebKit()(_isTouchLikeMessageViewportreturns false → treated like desktop → semantic realign kept) — round-2 iOS gap closed, no unheld reader.
Codex confirmed _isIOSWebKit detection is robust (correctly identifies iOS Safari/iPadOS/iOS-webview, no false-positive on desktop Safari or Android) and no scroll-pin regression (#3250/#4295/#4856/#1731/#3470 all green). Fable confirmed all three platforms hold the reader during streaming. Full suite green.
Recommendation to the next agent / Nathan
GREEN — merge from branch gate-rebase/5666-stale-anchor-ios-excluded (sha:57612d05), NOT the PR's stale head c1bfdbfe — after a quick screen-recording sign-off (desktop + Android + iOS scroll-up-during-stream hold). A properly-converged cross-platform scroll-stability fix that took 3 rounds to get right on all platforms (the browser-native overflow-anchor availability differs by platform). Codex SAFE + Fable SHIP-UX + full suite green. Land-order: 3rd #5637-family fix (with #5635/#5638) — reconcile land order (they touch overlapping scroll-restore paths). concept 4/5. Author @allenliang2022 (new, T1). crit=3.
_Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Complete platform matrix: refusal fires only where overflow-anchor holds — desktop (inert, realign kept, r1), Android (works, refuse+anchor holds, #5637), iOS WebKit (INERT, excluded via isIOSWebKit, realign kept, r2). Codex SAFE (robust iOS detection, no pin regression) + Fable-UX SHIP-UX + full suite green (0 failed) + 241 scroll tests. 3-round cross-platform convergence. Land-order w/ #5635/#5638. Visible → Nathan. Cert valid for sha:57612d05.
c93672a
|
Shipped in v0.51.904. Thanks @allenliang2022 — on Android, a scrolled-up reader no longer drifts backward each streaming tick; the stale viewport-anchor restore is refused during streaming so native scroll-anchoring holds, carefully scoped to Android only (iOS overflow-anchor is inert; desktop uses overflow-anchor:none). Verified coexisting with #5685+#5681, full gate green, live-served. |
Summary
A residual mobile scroll jump-back remained after #5638. While the reader is scrolled up
in history during a live stream, the viewport is nudged backward by a few hundred px on
each streaming tick.
Root cause
The same-frame scroll restore (
_restoreMessageScrollSnapshotSameFrame) captures ananchor before a live DOM update, then restores to it afterward. During streaming, the
incoming chunk grows content above the viewport, so the captured
topOffset(and theabsolute
snapshot.top) no longer map to the same content — realigning to them yanks astill reader backward.
The existing
snapshot.userUnpinned === truefallback skip does not cover this: thescrollHeight-collapse scroll event re-pins the state machine (flipsuserUnpinnedbackto
false) mid-stream, so both scroll-writing exits still fire:_restoreMessageViewportAnchor,scrollTop += delta), andsnapshot.topfallback.Fix
Two guards, both keyed on content-growth-since-capture + absence of recent real input
intent — deliberately not a
scrollTopdiff. On anoverflow-anchor: autocontainer (the mobile resting state) the browser itself writes
scrollTopto compensateabove-viewport growth, so a genuinely still reader's
scrollTopis not stationary; ascrollTop-diff "did the user move?" test is defeated by that compensation._recentMessageScrollIntent/_recentMessageTouchScrollIntentinstead reflect genuinetouchmove/wheel/keydowninput, which the browser's anchor layer never sets._captureMessageViewportAnchornow recordsscrollHeightAtCapture(+scrollTopAtCapture)._restoreMessageViewportAnchorrefuses the realign when content grew since capture, thereis no recent input intent, and the delta would move
scrollTopmore than a few px.snapshot.topfallback in_restoreMessageScrollSnapshotSameFramemirrorsthe same guard.
An actively scrolling reader (recent intent, e.g. a load-older prepend they triggered)
keeps the legitimate restore; a fresh anchor (no growth) and legacy snapshots without the
captured geometry are unaffected.
Tests
tests/test_issue5637_stale_anchor_guard.py— 7 node-harness cases covering both guards:refuses the stale realign / fallback, allows a fresh anchor, allows an actively-scrolling
reader, and stays backward-compatible with snapshots lacking the captured geometry. Two
cases are mutation-checked (removing either guard makes them fail).
Builds on #5638 (content-visibility scoping + the unpinned-hold), which addressed the
earlier layers of the same report.