fix(stream): preserve scroll position after live-to-final settlement (#6385)

[webtecnica]

Summary\n\nLive-to-final settlement reorders visible content and displaces the reader's scroll position.\n\n## Root Cause\n\nSTREAM_DONE handler does a two-render sequence: first expanded worklog, then collapsed. _renderMessagesWithScrollSnapshot captured the scroll anchor from the expanded DOM, then rendered collapsed — anchor lookup fails, viewport jumps.\n\n## Change\n\nModified _renderMessagesWithScrollSnapshot() to accept an optional prescrollSnapshot parameter. When provided, uses pre-captured state instead of re-capturing from the (potentially expanded) current DOM.\n\n## Verification\n\nESLint runtime guard clean.

[greptile-apps]

Greptile Summary

This PR preserves the reader’s scroll position when a live response settles. The main changes are:

• Captures the scroll state before the temporary expanded-worklog render.
• Reuses that state while rendering the final collapsed content.
• Adds checks for snapshot timing, forwarding, and fallback capture behavior.

Confidence Score: 5/5

This looks safe to merge.

• The updated settlement path captures the scroll state before the intermediate render.
• Existing helper callers retain fresh snapshot capture when no snapshot is supplied.
• No blocking issue was found in the changed code.

Important Files Changed

Filename	Overview
static/messages.js	Captures the live scroll state before the temporary worklog expansion and passes it to the final render.
static/ui.js	Allows the scroll-preserving render helper to reuse a supplied snapshot while keeping existing fallback behavior.
tests/test_issue4970_stream_done_shrink_regression.py	Adds checks for snapshot ordering, forwarding, restoration, and calls without a supplied snapshot.

_{Reviews (2): Last reviewed commit: "fix tests for #6385 scroll snapshot: upd..." | Re-trigger Greptile}

[nesquena-hermes]

Summary

Reading both changed files in full at 25d39da against origin/master, the timing change is coherent: static/messages.js captures while the live DOM still exists, before the one-shot keep-open render, and static/ui.js lets the collapse pass restore that earlier anchor. I do not see a product-path contradiction in the two-file change. The branch is not ready as submitted, however, because it deterministically breaks the existing #4970 regression test and adds no replacement coverage for the new pre-captured-snapshot contract.

Code reference

The new done path at static/messages.js:5944-5953 changes the exact collapse call to:

const _doneLiveScrollSnapshot=typeof _captureMessageScrollSnapshot==='function'
  ? _captureMessageScrollSnapshot()
  : null;
if(typeof _armKeepSettledWorklogOpen==='function') _armKeepSettledWorklogOpen(_settledStreamId);

It later calls _renderMessagesWithScrollSnapshot with an argument. The unchanged assertion at tests/test_issue4970_stream_done_shrink_regression.py:177-185 still requires the literal no-argument call and then indexes that exact string:

assert "_renderMessagesWithScrollSnapshot()" in after
collapse_pos = after.index("_renderMessagesWithScrollSnapshot()")
follow_pos = after.index("shouldFollowOnDone")

That literal no longer exists in the STREAM_DONE slice, so this test fails before it can check ordering. The helper change at static/ui.js:15088-15097 is also currently uncovered: tests/test_issue3479_ios_stream_scroll_jump.py:99-110 verifies capture, render, and restore exist, but does not prove that a supplied snapshot is used instead of a fresh capture.

Diagnosis / recommendation

Update the #4970 test to look for the argument-bearing call and retain the important ordering assertion: disarm, collapse render with the live snapshot, then shouldFollowOnDone. Better, avoid pinning the whole call spelling and assert the pre-capture variable is created before arm and threaded into the collapse pass after disarm.

Add a small behavioral harness for _renderMessagesWithScrollSnapshot that supplies a sentinel snapshot, stubs _captureMessageScrollSnapshot with a counter, and proves the sentinel reaches _restoreMessageScrollSnapshotSameFrame without a second capture. Also prove the no-option callers at static/ui.js:9565 and static/ui.js:14416-14424 still capture normally.

One cleanup would make the helper contract less leaky: remove _prescrollSnapshot from the object passed to renderMessages rather than forwarding an internal transport option into the general renderer. That is not the primary blocker, but it keeps the options surface explicit.

Verification step

Please run the focused #4970 and #3479 scroll suites plus the browser reproduction from #6385 after updating the tests. I did not execute PR-authored code because scheduled review treats PR worktrees as untrusted; the failing assertion is established directly from the unchanged test and changed call site.

[webtecnica]

@nesquena-hermes ✅ Fixed:

1. fix(chat): suppress post-render scroll artifacts #4970 test updated to look for argument-bearing call _renderMessagesWithScrollSnapshot({_prescrollSnapshot:_doneLiveScrollSnapshot}) with ordering preserved
2. New behavioral harness proving sentinel snapshot bypasses capture, and no-option callers still capture normally

15/15 tests passing. Ready for re-review!

[nesquena-hermes]

🔬 Gate certification — GREEN ✅

Certified head: sha:f35081683af2 · PR: #6390 · @webtecnica, fix(stream): preserve scroll position after live-to-final settlement (#6385)
Verdict: The engineering gate is green on the exact contributor head rebased onto frozen current master 8586572da54e; merge remains blocked only on Nathan's crown-jewel visual sign-off, including real iOS Safari motion proof.

What I ran (isolated exact-head worktree /tmp/wt-gate-6390)

Gate	Result
Threat / identity	CLEAN (score 0); exact contributor delta and rebased artifact have the same stable patch-id; live head rechecked unchanged before publication
Codex (reproduce)	SAFE TO SHIP — no regression risk; independently drove desktop + 390×844 settlement with 0px anchor drift
Senior correctness review	APPROVE — pinned, unpinned, missing-anchor, virtualization, double-render, cache, and mobile-native-anchor paths reviewed; no blocker
Fable UX	SHIP-UX — no new chrome; behavior matches ChatGPT/Claude's preserve-reader-position contract; 20-cell desktop/mobile regression probe clean
Full serial pytest	Completed to the end: 13,506 passed, 82 skipped, 1 xfailed, 2 xpassed, 34 subtests. The 15 failures + 2 collection errors are byte-for-byte the existing frozen-master approval/keyless-onboarding isolation baseline (13,505 passed there); this PR adds one pass and no new failure
Focused / test bite	15/15 targeted scroll tests passed. Replacing the production JS with frozen-master behavior makes both new contract tests fail, proving the coverage discriminates the fix
Static / private gates	git diff --check, Node syntax, runtime ESLint, scope-undefined, browser smoke, and threat scan clean
Real browser / live drive	Real Chromium lifecycle on desktop 1280×800 and mobile 390×844: normal settlement passed for both unpinned and pinned readers; unpinned semantic-anchor drift was 0px, pinned readers remained at bottom. Terminal-error settlement also passed unpinned desktop and pinned mobile, including reload parity and no product console errors

Findings

• ✅ static/messages.js:5962-5969 now captures the reader's geometry from the live DOM before arming the intermediate keep-open render, then threads that exact snapshot into the collapse restore.
• ✅ static/ui.js:15309-15318 preserves the supplied snapshot by identity and retains fresh-capture behavior for every no-option caller.
• ✅ The prior coverage blocker is closed: sentinel identity, zero recapture, fallback callers, and capture-before-arm ordering are executable assertions and go red on master behavior.
• ⚠️ The new test docstring's caller line numbers drifted by one during rebase; cosmetic only.
• ⚠️ This PR fixes the reader-displacement half of Live-to-final settlement reorders visible content and displaces the reader viewport #6385. The separate semantic segment-order/identity acceptance criterion is not implemented here, so Live-to-final settlement reorders visible content and displaces the reader viewport #6385 should remain open or that remainder should be tracked separately.
• ⚠️ This is motion on the crown-jewel chat surface. Sparse stills would not prove it. Before merge, Nathan should review realistic tall-worklog video for desktop/mobile pinned + unpinned states and a real iOS Safari pass; Chromium emulation cannot certify WebKit's inert overflow-anchor exception.

Recommendation to the next agent

Technically green — cert fresh for sha:f35081683af2. Add to Tier 1, but merge only after Nathan's crown-jewel visual sign-off and real iOS Safari motion check. Preserve @webtecnica attribution, keep #6385's remaining semantic-order criterion open, and re-gate if feature code changes.

---

Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy — that's the release agent's call. This cert is valid only while the head stays at sha:f35081683af2.

[nesquena-hermes]

Shipped in exp-v0.52.143 (experimental channel) — thanks @franksong2702. Your scroll-anchor fix is live: the transcript now stays put when a streaming reply settles while you're reading history. Full gate before ship — Codex (SAFE), full 3-shard suite (0 failures), Fable UI/UX (SHIP-UX, confirmed the bottom-pinned auto-follow path is preserved), plus a maintainer live-drive that measured the anchor held (scrollTop 4000→4068, no jump).