Skip to content

fix(chat): keep a still reader at the bottom on phones without anchoring - #9391

Merged
bolichen97 merged 1 commit into
mainfrom
fix/mobile-chat-scroll-entry-jump
Sep 8, 2026
Merged

fix(chat): keep a still reader at the bottom on phones without anchoring#9391
bolichen97 merged 1 commit into
mainfrom
fix/mobile-chat-scroll-entry-jump

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

On a phone, opening a session lands the transcript mid-conversation instead of at the latest message, and scrolling to the bottom by hand leaves the view unsettled. Reported from an iPhone against builds that already carry the recent scroll work (0.6.0-insider.6 includes #7916, #8574, #8001).

Reproduced in Chromium by disabling CSS scroll anchoring (overflow-anchor: none), which is how iOS Safari behaves — it has no scroll anchoring at all. Isolated gateway + fake ACP backend, 390×844 touch viewport, a 40-turn session, /chat?sid= entry:

entry (iOS-like, 3 runs) distance above the end after 6s
main 1007 px, 1119 px, 0 px
this PR 0 px, 0 px, 0 px

A 60-turn session (one older page unloaded, so the earlier-messages bar mounts) landed 46 px short on every drawer entry on main; 0 with this PR. With native anchoring left on, every scenario lands at 0 on both — which is why none of this was visible on a desktop browser.

Why it matters

Every phone user opening an existing session reads the wrong part of the conversation and has to find the end by hand; a reader who then scrolls to the end does not get followed when the next turn streams. Long sessions with code blocks are the worst case, because the post-entry reprice that opens the gap scales with how much of the transcript above the viewport is still settling.

What changed (motivation → approach → change)

Symptom → cause. Per-frame tracing with scrollTo/scrollTop hooked and the scroller's children measured each frame:

  1. entry pin lands at the bottom (forcepin 0→18345);
  2. a row above the fold shrinks 112 px (code-block stand-in → highlighted block), the engine clamps scrollTop, still at the bottom;
  3. the top spacer reprices −1008 px as rows above the window measure, clamp again;
  4. both grow back (+112, +1008) — no scroll event, scrollTop stays: the reader is now 1120 px above the end;
  5. the ResizeObserver pin evaluates: nothing is running, distance > 0 → the idle rule releases follow. Nothing brings the reader back.

On Chromium, native scroll anchoring moves scrollTop through step 4 and the reader never leaves the bottom. On WebKit the whole reprice is paid as displacement.

Approach. The idle rule (evaluateAutoPin, runActive) cannot tell from distance who opened the gap, and it errs toward release. The signal that can tell is input plus position: a reader who left the bottom has touched the scroller (wheel / touch / pointer / scrolling key) since we last placed them, or is no longer resting where our last write put them. When neither holds, the gap is content settling under a still reader and they are carried back, live turn or not. Both halves are load-bearing — position alone reads our own write as consent for a reader who wheeled up and stopped on it; input alone would drag back a programmatic reveal (search hit, pinned-prompt jump, find-in-page) whose scroll event has not dispatched yet, since none of those touch the scroller's input listeners.

Changes (FollowController.ts, useVirtualChat.ts):

  • evaluateAutoPin takes readerMovedSinceWrite (default true = legacy). With it false and scrollTop within epsilon of lastWriteTop, a gap is closed regardless of runActive; every other branch is unchanged. This deliberately reverses a pin made in perf(chat): reachable archived history and stable phone scrolling #7916 (436fe5115), whose idle branch declined an exception for "the reader is resting on our own last write" on the grounds that a content-opened gap was indistinguishable from a reader-opened one. It is distinguishable once input is on record: the absence of any hardware input since our last placement is the evidence that decision said was missing. The released-with-input case that motivated the original pin stays covered by its own tests (idlePinGuard, prePaintPinComposer, FollowController).
  • The hook stamps lastPinAt at each bottom positioning (pin write, layout-clamp re-baseline, at-bottom observation) and feeds hardInputAt > lastPinAt to both call sites (post-paint RO pin, pre-paint height-sync pin).
  • Leading chrome. Content above the rows inside the scroller (the earlier-messages bar) is observed by nothing — not the row observer, not the scroller's box. A per-commit layout effect measures the leading offset while following and re-evaluates the pin through the same predicate when it changes (gated by the same gesture-settle window as the pre-paint pin).
  • Coverage watchdog misfire. The watchdog fired forcePin every 500 ms for any reader parked at the bottom, because the tail spacer and bottom sentinel below the last row read as uncovered list (33 px > 8 px slack). That same-value write re-armed follow the idle rule had released (measured: WRITE forcepin X->X every 500 ms in every inspector trace on main). On WebKit a programmatic scrollTo also ends an in-flight momentum or rubber-band animation, so this is the likely mechanism behind the bottom jitter — stated as inference: the emulation cannot reproduce WebKit scroll physics and this host cannot run Playwright's WebKit (Amazon Linux 2023, missing libgtk-4 and friends), see Manual verification. A side the window has reached the list's end on is now covered by construction.
  • Removing that misfire exposed a latent defect it had been papering over: releasing follow left lastWriteTop pointing at the old bottom, so a hand scroll landing on that exact pixel (it is still the maximum scrollTop) read as our own scroll and the re-engagement branch was skipped — follow never re-armed after a manual return. The self-scroll reference is now dropped with the release.

Tests

Each mutation-verified (the mutation named is caught by the named test and by nothing weaker):

  • FollowController.test.tsreaderMovedSinceWrite: idle + still → carried; idle + moved → released (unchanged); no input but scrollTop left our write → not ours (reveal race); resting on a clamp-rebaselined write; never overrides a released stick or an owning restore; omitted = legacy. (drop the branch / drop either condition → red)
  • useVirtualChat.idleCarryBack.test.tsx — clamp then regrow with no input → carried back; wheel-at-bottom then clamp → still carried (clamp re-establishes the bottom; drop the clamp stamp → red); programmatic reveal with its scroll event pending → left alone; input in the outgoing session does not disown the incoming entry pin; scrolling back onto the exact pixel of our last pin re-arms follow (drop releaseFollowBaseline → red).
  • useVirtualChat.leadingChrome.test.tsx — a 46 px bar mounting above the rows carries a still idle reader to the new bottom; a reader who wheeled is left alone (disable the effect → red).
  • useVirtualChat.coverageWatchdog.test.tsx — a reader at the bottom with page chrome below the rows gets no write per tick (drop the end < count guard → red).
  • Updated to the new contract, keeping the release case as a sibling with input: useVirtualChat.prePaintPinComposer.test.tsx, useVirtualChat.idlePinGuard.test.tsx.

Gates: tsc -b, eslint src, jscpd clean; all 100 virtualizer/chat test files (1082 tests) pass locally.

Manual verification

Real-browser rig (Playwright, iPhone profile, real touch sequences via CDP, isolated gateway with the fake ACP backend), both with overflow-anchor: none (iOS-like) and with native anchoring:

  • cold /chat?sid= entry, drawer switch short→long, drawer re-entry after a scroll, full reload, entry mid-stream: 0 px from the end in every phase, both modes (main: 46–1119 px in the iOS-like mode).
  • touch scroll up then back to the bottom: follow re-armed (stick=true) — on main this only held because the watchdog force-pinned every 500 ms.
  • inspector trace shows the entry forcepin writes only; the per-500 ms WRITE forcepin X->X stream is gone.

Not verified on a physical iPhone, and not under Playwright's WebKit engine either: it does not run on this host (Amazon Linux 2023 — playwright install webkit reports libgtk-4, libvulkan, libicu 74, libflite… missing, and installing system libraries is out of scope). The anchoring-off emulation is the same discriminator #7916 used to find the drift class and covers the entry/landing claims exactly (they are pure scroll geometry, no physics). The momentum/rubber-band claim about the watchdog is inference from the mechanism, not a measurement — the same rig scenarios under real WebKit or a physical iPhone would be the confirming step.

Related Issues

Follow-up to #7916 (which fixed the drift while reading history on a phone); this covers entering and returning to the bottom.

Pattern harvest

Rule candidate: review-prompt
Pattern: a scroll-position decision that reads "distance from bottom" as evidence of a user move is only correct where native scroll anchoring absorbs content growth; on WebKit it must be paired with an input signal.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no spec covers the follow controller
  • No secrets, credentials, or internal references in the diff

Entering an idle session on an iPhone opened the transcript a viewport
or more above its end, and scrolling back to the bottom by hand did
not re-engage follow. Both trace to decisions that were only correct
because Chromium's native scroll anchoring quietly covered for them;
WebKit has no scroll anchoring at all.

After the entry pin the rows keep settling: a code-block stand-in swaps
for the highlighted block, the top spacer reprices as rows above the
window measure, the earlier-messages bar mounts. Content above the fold
shrinks (the engine clamps scrollTop, the reader stays at the bottom)
and then grows back with no scroll event. The automatic pin's idle rule
read that gap as the reader having scrolled up and RELEASED follow, so
nothing brought them back. Reproduced in Chromium with overflow-anchor
disabled: 1007-1119px above the end on two of three entries; 0 with
this change.

The discriminator is input plus position, not distance: a reader who
left the bottom has touched the scroller since we last placed them, or
is no longer resting where our last write put them. When neither holds,
the gap is content settling under a still reader and they are carried
back, live turn or not. Either signal alone would be wrong: position
alone reads our own write as consent for a reader who wheeled up and
stopped on it, input alone drags back a programmatic reveal (a search
hit, a pinned prompt) whose scroll event has not dispatched yet.

Chrome above the rows -- the earlier-messages bar -- is observed by
nothing, so its mount now re-evaluates the bottom pin through the same
predicate (46px short at every drawer entry before).

The viewport-coverage watchdog fired forcePin every 500ms for any
reader parked at the bottom, because the tail spacer and sentinel below
the last row read as uncovered list. That same-value write re-armed
follow the idle rule had released and, on WebKit, cut every momentum
tail short. A side the window has reached the list's end on is covered
by construction. Removing the misfire exposed that releasing follow
left the self-scroll reference pointing at the old bottom, so a hand
scroll landing on that exact pixel read as our own write and follow
never re-armed; the reference is now dropped with the release.
@bolichen97
bolichen97 requested a review from a team September 8, 2026 05:35
@bolichen97
bolichen97 requested a review from a team as a code owner September 8, 2026 05:35
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of b481fdc5687e1be2a17e24a91f1087278275142f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

This diff is purely behavioral scroll-follow logic in FollowController.ts/useVirtualChat.ts plus tests — no JSX, no strings, no new or changed user-visible control, label, or state. Reconciliation table is empty: nothing to blind-read, nothing to screenshot. The behavior change (a still reader on a phone stays at the latest message; readers who scrolled are never yanked) is guarded by both input and position signals, preserving locus of control, and the PR documents real-device-profile verification with measurements. No lens-13 case: no persistent element changes form or place — the fix adjusts scroll position, which is the async-settling class, not an element swap.

UX-Verdict: PASS

Pure scroll-behavior fix with no visible surface: phone users now land on the latest message, and readers who scrolled away are never dragged back.

[UX-REVIEWED] b481fdc

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of b481fdc5687e1be2a17e24a91f1087278275142f — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound two-signal design (input + position) that fixes the real cause; but the WebKit fix was never run on WebKit, only Chromium emulating one WebKit trait.

Watch

  • The whole PR targets iOS Safari, yet verification is "Reproduced in Chromium by disabling CSS scroll anchoring" and "Not verified on a physical iPhone." Anchoring-off Chromium is a fair discriminator for the reprice drift (per the perf(chat): reachable archived history and stable phone scrolling #7916 precedent), but the watchdog change additionally claims to fix WebKit momentum/rubber-band tails being "cut short" — physics the emulation cannot reproduce, and rubber-band overscroll on WebKit moves scrollTop past max, geometry none of the tests model. The Playwright rig already exists; running it on Playwright's WebKit engine is cheap and would exercise the actual target.
    Clears when: the same rig scenarios pass under Playwright WebKit (or a physical-iPhone check), including the touch-scroll-up/return and momentum cases.

Suggestions

  • The old idle-release comment was a deliberate pin against "reading our own write as consent"; this PR reverses it with a principled reason (input absence is the missing evidence) and keeps the released-with-input sibling test — worth one line in the code history/PR pointing at the original decision's commit so the next reviewer sees the reversal was engaged, not overlooked.

[DESIGN-REVIEWED] b481fdc

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of b481fdc5687e1be2a17e24a91f1087278275142f — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: PASS

Whole fix was validated with overflow-anchor: none emulation only — confirm once on a physical iPhone before trusting the 0 px numbers.

What this change ships

Inventory (7 items)

Intent: on a phone, an opened session should land at the latest message and stay there while content settles — a FIX (reproduction with measurements in the description; new tests encode it).

  1. Opening a session on a phone lands at the latest message and stays there through settling — justified
  2. The earlier-messages bar mounting no longer leaves a phone reader 46 px short of the end — justified
  3. A reader parked at the bottom no longer receives a scroll write every 500 ms (momentum/rubber-band tails no longer cut short) — justified
  4. Scrolling back to the bottom by hand re-arms follow again — justified
  5. evaluateAutoPin gains optional readerMovedSinceWrite (2 real consumers: useVirtualChat.ts:2187, :3124; both call sites fed, no unfixed sibling) — justified
  6. New per-commit leading-chrome measurement re-runs the existing pin predicate — justified
  7. The prior pin "deliberately WITHOUT an exception for resting on our own last write" is inverted — justified

The rewritten pin's own words rested on the two cases being "indistinguishable" from distance alone; this PR introduces the input-plus-position signal that distinguishes them and keeps the release case as a sibling test with input, so the prior decision is answered, not erased. The defect's provenance is a written reproduction plus tests that fail on base; no item is undeclared, duplicated, or symptom-level (the WebKit no-anchoring root is unreachable, and the change compensates at the decision point that misread it).

[FIRST-PRINCIPLES-REVIEWED] b481fdc

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging b481fdc5687e1be2a17e24a91f1087278275142f. 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- website/electron/ipc-registrar.js:327 -- Validated pathname can be retargeted to a sensitive file
const real = fs.realpathSync(filePath); … const err = await openPathHardened(shell, real);
safe .env probe -> agent replaces it with a symlink to the credential .env -> user selects Open in editor -> credentials open locally
Anchor: residual/security
Fix: Validate sensitivity and launch a private snapshot copied from an O_NOFOLLOW descriptor.
[BLOCK-MERGE] b481fdc
[GPT-REVIEWED] b481fdc

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 is a FENCED finding — my only verdicts are UPHOLD-FENCED or FLAG, and FLAG requires the same complete evidence record (every condition at file:line, recovery path, rarity argument) proving the reaching conditions are so extreme a human would accept the residual risk.

Examining website/electron/ipc-registrar.js:271-337: dashboard:open-file resolves real = fs.realpathSync(filePath) (line 312), tests extname(real) against OPENABLE_EXTS (line 313) which deliberately includes .env (line 306), confirms isFile (line 316), and opens real (line 327). The check and open both use the resolved path, so there is no post-check retarget window — but there is also no sensitivity gate on the resolved target, and .env is an allowed extension. An agent-authored foo.env symlink pointing at a real credential .env passes all three checks and gets launched in the user's local editor.

Conditions for the FLAG record: (a) agent writes a symlink to a known credential path — within ordinary agent capability in a workspace, not an input the system's writer cannot produce; (b) user invokes open on the agent-authored path and confirms — plausible social-engineering flow; (c) assertLocalDashboard passes (line 272). None of these mutually contradict, none are legacy-only. Recovery path: none — credential display is irreversible. I cannot construct the rarity argument FLAG demands, and the harm class is credential exposure. When torn on a fenced finding, UPHOLD-FENCED.

[ADJUDICATION] b481fdc total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] b481fdc

[ADJUDICATION-FENCED] b481fdc fenced=1 flagged=0
UPHOLD-FENCED F1 website/electron/ipc-registrar.js:327 -- Every condition (agent-written credential symlink, user open, local dashboard) is producible in ordinary operation and the exposure is irreversible, so no rarity argument clears the credential-class harm.
[GPT-ADJUDICATED-FENCED] b481fdc

False positive or not applicable? A repository writer can comment:
/ai-review override gpt b481fdc5687e1be2a17e24a91f1087278275142f: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed b481fdc5687e1be2a17e24a91f1087278275142f — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] b481fdc

Verdict parsed from the review's SHA-scoped output markers for commit b481fdc5687e1be2a17e24a91f1087278275142f.

False positive or not applicable? A repository writer can comment:
/ai-review override fable b481fdc5687e1be2a17e24a91f1087278275142f: <one-sentence reason>

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 8, 2026
@bolichen97
bolichen97 merged commit 4180bce into main Sep 8, 2026
64 checks passed
@bolichen97
bolichen97 deleted the fix/mobile-chat-scroll-entry-jump branch September 8, 2026 06:40
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 8, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Watch: verify under Playwright WebKit / physical iPhone — not clearable on this host, disclosed in the PR body. playwright install webkit fails host validation on Amazon Linux 2023 (missing libgtk-4.so.1, libvulkan.so.1, libicu*.so.74, libflite*, libgstcodecparsers), and installing system libraries is outside this change's scope. The entry/landing claims are pure scroll geometry and are exercised exactly by the anchoring-off discriminator (the same one that located the drift class before); the momentum/rubber-band statement about the watchdog was over-stated and the PR body now names it as inference from the mechanism, not a measurement. Confirming step recorded in Manual verification for whoever has a WebKit-capable host or a phone.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Suggestion: point at the original idle-release decision so the reversal reads as engaged. Applied in the PR body ("What changed", first bullet): the idle branch's refusal of a resting-on-our-write exception came from perf(chat): reachable archived history and stable phone scrolling #7916 (436fe5115), the reason it gave (indistinguishable causes) is answered by the input signal, and the released-with-input case it protected keeps its own tests. Kept out of the code comment on purpose: the repo's comment rule excludes PR numbers and SHAs, so the code states the current rule and the PR carries the provenance.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Post-merge note: the red checks on b481fdc56 dated 06:43–06:51Z are artifacts, not findings against this change. A description edit at 06:43Z (adding the provenance of the reversed idle-release decision and softening the WebKit momentum claim to inference) fired the edited event after the PR had already merged at 06:40:21Z; the re-runs could not resolve the head commit once the branch was deleted (fatal: Not a valid commit name b481fdc56… in Automated Rule Check / PR Hygiene / Semgrep / Inclusive Language / Screenshot Evidence), and the GPT lane's blocking item names website/electron/ipc-registrar.js:327, a file this PR does not touch (8 files, all under website/src/hooks/virtualizer and website/src/test). PR Readiness for this revision stayed success. The ipc-registrar.js observation (an agent-authored .env symlink passing the open-file allowlist) concerns code already on main and is worth its own issue if not already tracked.

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.

2 participants