fix(chat): defer mobile overflow-anchor suppression across the full height-churn window (residual scroll jump-back root fix) - #5392
Conversation
|
| Filename | Overview |
|---|---|
| static/ui.js | Replaces the single-rAF overflow-anchor restore with a deferred, re-armable suppression window that tracks CSS max-height animations; logic is sound, state machine is consistent, and all release paths converge through _liftMobileAnchorSuppression. |
| tests/test_issue4856_android_scroll_regression.py | Updates existing cleanup-invariant test to target the new shared _liftMobileAnchorSuppression helper; adds two source-string tests and two mutation-verified behavioral harness tests covering re-arm extension and hard-cap release on missed transitionend. |
Reviews (3): Last reviewed commit: "fix(chat): make re-arm reachable + hard ..." | Re-trigger Greptile
| if(_mobileAnchorSuppressReleaseTimer){ clearTimeout(_mobileAnchorSuppressReleaseTimer); _mobileAnchorSuppressReleaseTimer=null; } | ||
| if(_mobileAnchorSuppressRafId&&typeof cancelAnimationFrame==='function'){ cancelAnimationFrame(_mobileAnchorSuppressRafId); } | ||
| _mobileAnchorSuppressRafId=0; | ||
| const rafHop=(cb)=>{ if(typeof requestAnimationFrame==='function') return requestAnimationFrame(cb); return setTimeout(cb,16); }; |
There was a problem hiding this comment.
The
rafHop fallback returns a setTimeout ID (a number) and stores it in _mobileAnchorSuppressRafId, but the re-arm code unconditionally calls cancelAnimationFrame(_mobileAnchorSuppressRafId) to cancel it. cancelAnimationFrame ignores setTimeout IDs, so the pending 16ms timer is never actually cancelled in the fallback path — consecutive calls won't properly extend the suppression window there. Real mobile browsers always have requestAnimationFrame, so this path is never taken in production, but a parallel clearTimeout call would close the gap and avoid a future maintainability trap.
| const rafHop=(cb)=>{ if(typeof requestAnimationFrame==='function') return requestAnimationFrame(cb); return setTimeout(cb,16); }; | |
| let _rafHopIsTimeout=false; | |
| const rafHop=(cb)=>{ if(typeof requestAnimationFrame==='function') return requestAnimationFrame(cb); _rafHopIsTimeout=true; return setTimeout(cb,16); }; |
… to kill the residual scroll jump-back Follow-up to nesquena#5338 (shipped in v0.51.797). That PR routed the sync anchor-realign write and the async postProcess reflow through overflow-anchor suppression, but a residual mobile scroll jump-back remained on a real device, captured mid-stream. ## Root cause (real on-device data) A scrollTop flight-recorder on the running instance captured the jumps on the actual phone. Decisive signal: every captured jump (dTop -101/+350/+748/-400) had a call stack of the rAF sampler ONLY -- no JS render function on the stack. The jumps are not our JS writing scrollTop; they are the browser's native scroll-anchoring engine re-compensating scrollTop in the LAYOUT phase whenever above-viewport content changes height: virtual-scroll topPad recompute, worklog live->settled collapse, the STREAM_DONE multi-render sequence, and -- dominant during streaming -- CSS max-height collapse/expand animations on worklog rows (.activity-body max-height .34s, .tool-group-body .3s, .tool-card-detail .26s). Because that compensation runs in the browser's layout step it is independent of which frame our JS wrote scrollTop, so the previous single-rAF _fixMobileScrollJank released before the collapse/reflow landed and every per-write / single-frame suppression missed it. ## Fix _fixMobileScrollJank now (1) DEFERS its restore -- each call re-arms and cancels any pending release so a burst of renders (STREAM_DONE fires renderMessages several times back-to-back plus a deferred postProcess reflow) shares one suppression window with a 400ms settle floor for non-animated churn; and (2) TRACKS CSS animations -- it binds transitionrun/transitionend for max-height on #messages, so an animation start holds suppression (cancels the pending release) and an animation end schedules a short 90ms settle after the last one. Hard-capped at 1200ms so a looping transition can't pin overflow-anchor:none forever. Desktop rests at overflow-anchor:none so _browserOverflowAnchorActive() returns false and the whole guard is a no-op. ## Verification (isolated debug instance, real 500-message session, Playwright) - Reproduce: bare auto, above-viewport -400px height change -> scrollTop jumped -400. - Fixed: same change with the guard armed -> 0. - Real 340ms CSS max-height animation: overflow-anchor stays none through 160/250/340ms sample points (the old fixed window released at ~153ms) and restores after the animation ends. - No-regression: desktop computed none -> guard no-op, inline untouched, normal scrolling works. ## Tests Updates the nesquena#4856 source-string test that keyed on the old single-rAF cleanup body to match the deferred-release structure, and adds two behavioral source-invariant tests (deferred re-arm/cancel + settle floor; transitionrun/transitionend max-height extender + hard cap), both base-fails on the old body / head-passes on the new one. static/ui.js + the one test file only.
cb1e8cf to
737e496
Compare
🔬 Gate certification — RED ⛔ (2 SILENT defects that undercut the fix's own goal — dead re-arm + missed-transitionend leak)Certified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean; node -c OK |
| Codex (reproduce) | SHIP-WITH-FIXES — 2 SILENT; I confirmed the first by inspection |
| Full pytest suite | 1 failed / 11747 passed — the 1 failure (test_issue1567_nous_picker…falls_back_to_static_4) is a pre-existing env-dependent flake (fails isolated too; Nous Portal model-fetch env, unrelated to this ui.js-only change) |
| PR's own test | ✅ 20/20 (but they don't exercise the repeated-call re-arm or missed-transitionend paths — which is why they're green while the defects exist) |
Findings
⛔ SILENT (I CONFIRMED) — the re-arm/extend is dead code on repeated calls (static/ui.js:12324): _fixMobileScrollJank early-returns if(!_browserOverflowAnchorActive(el)) return;, and _browserOverflowAnchorActive reads the computed overflow-anchor (getComputedStyle(el).overflowAnchor==='auto'). After the FIRST call sets inline overflow-anchor:none, the computed value is none → the predicate returns false → every subsequent back-to-back render (the STREAM_DONE settle fires renderMessages several times + a deferred postProcess reflow) bails out before the clear/reschedule re-arm logic. So the "consecutive renders EXTEND the suppression window" behavior — the PR's core "full height-churn window" fix — never actually happens; suppression only lasts the first call's base window.
- Fix (Codex-exact): allow already-owned inline suppression through the guard —
const alreadySuppressed = el.style.overflowAnchor==='none'; if(!alreadySuppressed && !_browserOverflowAnchorActive(el)) return;— then the existing clear/reschedule runs. Add a runtime test that calls the helper twice and asserts a cancel/reschedule occurs.
⛔ SILENT — missed transitionend pins overflow-anchor:none (regresses #5338's mobile resting-auto contract) (static/ui.js:12290): onRun cancels the pending base release when a max-height transition starts, relying on transitionend/transitioncancel to reschedule. But the _MOBILE_ANCHOR_MAX_HOLD_MS=1200 is only a guard check inside onRun (whether to cancel), NOT an independent release timer. If transitionend/transitioncancel is missed (interrupted animation, element removed mid-transition), nothing restores → overflow-anchor:none stays inline indefinitely, regressing #5338's mobile-rests-at-auto contract.
- Fix: add a separate max-hold release timer, armed on every suppression window, NOT canceled by
onRun, restoring only ifel.style.overflowAnchor==='none'; clear it from every normal release path.
Recommendation to the next agent
RED — gate-fail/changes-requested (2 SILENT fixes): (1) let already-owned inline suppression through the guard so the re-arm/extend actually works; (2) add an independent max-hold release timer so a missed transitionend can't pin overflow-anchor:none. The approach + consistency with #5338 (_browserOverflowAnchorActive predicate, guarded restore, single bound listener) are good — these fixes make the intended behavior real and leak-safe. Once green, it's a VISIBLE mobile-scroll fix → Nathan screenshot/live-drive (follow-on to #5338, also parked for his sign-off). (The 1 suite failure is a pre-existing Nous-picker env flake, not this PR.) concept 4/5 (#4856 residual scroll-jump-back root fix). Author @allenliang2022 (T1). crit=3.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Rebased onto current master; re-arm-dead-code confirmed by inspecting _browserOverflowAnchorActive (reads computed → false after inline none), missed-transitionend leak is real (1200ms is a guard not a release timer). Cert valid for sha:737e4967.
…ate-cert defects) Addresses two SILENT defects the gate-cert caught: 1. Re-arm was dead code. _fixMobileScrollJank opened with if(!_browserOverflowAnchorActive(el)) return; and that predicate reads the COMPUTED overflow-anchor. The first call sets inline 'none', flipping computed to 'none', so the 2nd..Nth call of a STREAM_DONE burst returned before the re-arm logic — the 'consecutive renders extend the window' behavior never happened, collapsing to a single first-call window. Now: treat an inline 'none' WE set as still-armed (alreadySuppressed) so re-arm runs; desktop has computed 'none' with EMPTY inline so it stays a no-op. 2. The hard cap was only a guard clause inside onRun, not a release. A missed transitionend (interrupted animation / detached element) pinned overflow-anchor:none forever, breaking the nesquena#5338 resting-'auto' contract. Now an independent _mobileAnchorMaxHoldTimer, armed every call and cancelled ONLY by an actual lift (never by re-arm or onRun), force-lifts at the cap. All release paths route through a shared _liftMobileAnchorSuppression(). Tests: replaces the source-string assertions with a behavioral node harness that executes the REAL function against a mock #messages + fake timers and asserts the time-delta / failure-path behavior: - test_behavioral_rearm_extends_suppression_window: a 2nd call at t=200 must keep anchor 'none' at t=500 (dead-code re-arm releases ~432ms after call nesquena#1). - test_behavioral_hard_cap_releases_when_transitionend_missed: transitionrun with no transitionend must still restore 'auto' by the cap. Both mutation-verified: reverting either fix flips the matching test to FAIL. Also re-points test_raf_cleanup_checks_none to the extracted _liftMobileAnchorSuppression helper (avoids orphaning it). static/ui.js + the test file only.
|
Both gate-cert defects fixed in 1. Re-arm was dead code. You are exactly right: the guard const alreadySuppressed = el.style.overflowAnchor===none;
if(!alreadySuppressed && !_browserOverflowAnchorActive(el)) return;Desktop rests at computed 2. The hard cap was a guard clause, not a release. On the tests — the deeper problem was that my assertions checked absolute state on the happy path, which could not distinguish "re-arm works" from "re-arm is dead but the first call already set none." Replaced them with a behavioral node harness that executes the REAL function against a mock
Both are mutation-verified: reverting either fix flips the matching test to FAIL (I checked by reverting each hunk and re-running). Also re-pointed |
🔬 Gate certification — GREEN (engineering) ✅ · CONVERGED (bounce closed) · ⏸️ visible → Nathan screenshotCertified head: What I ran (rebased worktree
|
| Gate | Result |
|---|---|
| Rebase onto current master | ✅ git apply clean; node -c OK |
| Codex (reproduce) | SAFE TO SHIP — gated the rebased worktree, 0 findings |
| Full pytest suite | 1 failed / 11749 passed — the 1 failure (test_issue1567_nous_picker…falls_back_to_static_4) is the SAME pre-existing env flake as round 1 (fails isolated; Nous Portal model-fetch env, unrelated to this ui.js-only change) |
| PR's own test | ✅ 22/22 (grew 20→22: added re-arm + max-hold coverage) |
Findings — both prior SILENT closed (verified)
- Finding 1 (re-arm was dead code) → FIXED: the guard is now
const alreadySuppressed=el.style.overflowAnchor==='none'; if(!alreadySuppressed && !_browserOverflowAnchorActive(el)) return;(ui.js:12350-51). Reading the inline value (own-set) instead of only the computed predicate means a repeated call while already-suppressed passes the guard and reaches the clear/reschedule re-arm — so consecutive renders now genuinely EXTEND the window (the PR's core goal). Comment credits the gate feedback. - Finding 2 (missed-transitionend leak) → FIXED: a new independent
_mobileAnchorMaxHoldTimer(ui.js:12364) armed on every suppression window —setTimeout(() => _liftMobileAnchorSuppression(el), _MOBILE_ANCHOR_MAX_HOLD_MS=1200)that "NOTHING cancels except an actual lift", so a missedtransitionend/transitioncancelcan't pinoverflow-anchor:nonepast 1.2s. Release logic was consolidated into_liftMobileAnchorSuppressionwhich clears ALL timers (base + max-hold + rAF) — no double-fire, no orphaned timers. Guarded restore (only clears if still'none'). - Routes through the same
_browserOverflowAnchorActivepredicate as fix(chat): suppress browser overflow-anchor during JS scroll-anchor realign (mobile scroll jump-back) #5338 (desktopnone= no-op); single bound transition listener. Codex SAFE, suite green (bar the env flake).
Recommendation to the next agent
Engineering-GREEN — merge from branch gate-rebase/5392-mobile-anchor-defer (sha:9fd34989), NOT the PR's stale head 22b4b8da — AFTER Nathan's visible sign-off. Both SILENT defects closed via a clean centralized-release refactor (_liftMobileAnchorSuppression + independent hard-cap timer); Codex SAFE + 22/22 tests + suite green (the 1 failure is a pre-existing nous-picker env flake, not this PR). Because it's a visible mobile-scroll motion fix (residual jump-back root fix, follow-on to #5338), it's not autonomous: live-drive the jump-back scenario on a mobile viewport + screenshot/GIF for Nathan (pair with #5338's sign-off). concept 4/5 (#4856 root fix). Credit @allenliang2022 (T1, co-authored). crit=3.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Rebased onto current master; both SILENT closed (re-arm reachable via inline-value guard + independent max-hold release timer, verified by reading the centralized _liftMobileAnchorSuppression), Codex SAFE + 22/22 + suite green (bar the pre-existing nous env flake). Engineering green; visible → Nathan sign-off. Cert valid for sha:9fd34989.
|
Shipped in v0.51.829 📱 — thanks @allenliang2022! Merged via release #5440, with Nathan's sign-off (mobile motion fix — proof is the Codex repro + the 22-test regression file, since a still can't show absence-of-jump). Full gate (rebased onto current master v0.51.828):
This is a root-layer fix — it targets the browser's layout-phase scroll-anchoring compensation the mobile flight-recorder pinned, not our JS scroll writes. Credit preserved via |
… + independent hard cap) nesquena#4856 Clean rebase of allenliang2022's nesquena#5392 (rebase-first). Co-authored-by: allenliang2022 <allenliang2022@users.noreply.github.com>
…height-churn window (nesquena#4856) [allenliang2022]
Follow-up to #5338 (shipped in v0.51.797). That PR routed the sync anchor-realign write and the async postProcess reflow through overflow-anchor suppression, but a residual mobile scroll jump-back remained on a real device, captured mid-stream.
Root cause (confirmed with real on-device data)
I instrumented the running instance with a scrollTop flight-recorder and captured the jumps on the actual phone. The decisive signal: every captured jump (dTop -101 / +350 / +748 / -400) had a call stack of the rAF sampler ONLY — no JS render function on the stack. The jumps are not our JS writing scrollTop; they are the browser's native scroll-anchoring engine re-compensating scrollTop in the layout phase whenever above-viewport content changes height:
topPadspacer recomputerenderMessagesfires several times back-to-back).activity-body(.34s),.tool-group-body(.3s),.tool-card-detail(.26s) — the dominant driver during streamingBecause that compensation runs in the browser's layout step it is independent of which frame our JS wrote scrollTop in. A single-rAF
_fixMobileScrollJankreleased before the collapse/reflow landed, and even a fixed deferred window (~150ms) lifts mid-animation for the 260–340ms CSS transitions — so the remaining frames of each animation still jump.Fix
_fixMobileScrollJanknow does two things:clearTimeout+cancelAnimationFrame), so a burst of renders shares one window with a 400ms settle floor for non-animated churn (virtual topPad recompute, image-decode, katex measure).transitionrun/transitionstart+transitionend/transitioncancelformax-heighton#messages(bubbled from descendant worklog rows). An animation start holds suppression (cancels the pending release); an animation end schedules a short 90ms settle after the last one. Hard-capped at 1200ms so a looping transition can't pinoverflow-anchor:noneforever.Desktop rests at
overflow-anchor:none, so_browserOverflowAnchorActive()returns false and the whole guard is a no-op there.Verification (end-to-end, isolated debug instance, real 500-message session, Playwright)
auto, above-viewport -400px height changeauto+ armed guard, same -400px changenonethrough 160/250/340ms (old fixed window released at ~153ms), restores afternoneTests
none" invariant).test_fix_mobile_scroll_jank_defers_release_across_height_churn(deferred re-arm/cancel + settle floor) andtest_fix_mobile_scroll_jank_tracks_css_max_height_animations(transitionrun/transitionendmax-heightextender + hard cap) — both base-fail on the old single-rAF body / head-pass on the new one.static/ui.js+ the one test file only. @nesquena