From 0a1cf564c7f3a8018e59eeca648eb0232fe700b2 Mon Sep 17 00:00:00 2001 From: allenliang2022 Date: Tue, 30 Jun 2026 17:03:39 +0800 Subject: [PATCH] fix(chat): make keep-settled-worklog-open genuinely transient (#5260 gate-cert) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RED gate-cert (Codex-verified) found the keep-open was NOT the one-frame transient the PR claimed: _disarmKeepSettledWorklogOpen() only cleared the token with no follow-up render, so the just-settled worklog stayed force-open and could be cached into _sessionHtmlCache and persist across session restores. Two coordinated fixes: 1. Don't cache the forced-open DOM. Add _isKeepSettledWorklogOpenArmed() and gate the _sessionHtmlCache.set() population on a typeof-safe !_keepOpenArmed local, so the armed settle render is never cached. The typeof guard also keeps standalone renderMessages() node harnesses (which don't define the helper) working. 2. Collapse pass after disarm for BOTH pin states. The STREAM_DONE handler disarms, then runs _renderMessagesWithScrollSnapshot() UNCONDITIONALLY (a scroll-preserving re-render that collapses the worklog back to its copied live/user disclosure state), and only THEN scrollToBottom() for followers to re-settle at the tail. The second RED gate-cert caught that gating the collapse pass to the non-following path left a pinned follower forced-open, because scrollToBottom() only settles scroll position and does NOT re-render. Running the collapse render before the follower re-settle fixes both pin states. The same-frame JS restore absorbs the collapse height change instead of leaving it to native scroll-anchoring (which mobile suppresses mid-render), so there is no jump, and this unarmed render is the one that populates the cache. Net: keep-open is genuinely one settle render for pinned AND unpinned readers — neither persists nor caches — and a user-collapsed worklog is preserved. Verified e2e on an isolated debug instance for both pin states (worklog open-while-armed -> collapsed-after-pass; pinned follower stays pinned at tail). Regression tests lock the cache-skip and the collapse-before-scrollToBottom ordering; the scrollToBottom adjacency keeps the #1360 done-follow test green. --- static/messages.js | 15 ++-- static/ui.js | 37 +++++++--- ...issue4970_stream_done_shrink_regression.py | 71 +++++++++++++++++++ 3 files changed, 109 insertions(+), 14 deletions(-) diff --git a/static/messages.js b/static/messages.js index 3cc98752b..b8883686b 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5205,13 +5205,20 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // turn boundary so the following syncTopbar() refetches the authoritative // effort exactly once (not per-token — the storm short-circuit is intact). if(typeof _lastReasoningFetchKey!=='undefined') _lastReasoningFetchKey=null; - // Arm the one-shot keep-open token for JUST this settled turn so a - // pinned follower's worklog stays height-stable (no STREAM_DONE shrink - // jump); disarm right after the render so historical worklogs collapse - // compact as normal. Scoped to the just-settled stream id. + // Arm one-shot keep-open so the JUST-settled worklog stays open on the + // settle render (height-stable swap, no shrink jump). Disarm, then run a + // scroll-PRESERVING collapse pass for BOTH pin states so the worklog + // returns to its copied live/user disclosure state (a pinned follower's + // scrollToBottom() only settles scroll, it does NOT re-render, so without + // this pass the forced-open DOM would persist for them). This unarmed + // render also populates the cache with the correctly-collapsed DOM, and + // the same-frame JS restore absorbs the collapse so there is no jump. + // (#5260 gate-cert: keep-open must be transient + uncached for everyone.) if(typeof _armKeepSettledWorklogOpen==='function') _armKeepSettledWorklogOpen(_settledStreamId); syncTopbar();renderMessages({preserveScroll:true}); if(typeof _disarmKeepSettledWorklogOpen==='function') _disarmKeepSettledWorklogOpen(); + if(typeof _renderMessagesWithScrollSnapshot==='function') _renderMessagesWithScrollSnapshot(); + else renderMessages({preserveScroll:true}); if(shouldFollowOnDone&&typeof scrollToBottom==='function') scrollToBottom(); if(typeof noteWorkspaceMutationsFromToolCalls==='function') noteWorkspaceMutationsFromToolCalls(S.toolCalls); loadDir('.', { preservePreview: true }); diff --git a/static/ui.js b/static/ui.js index 6cae78a4a..ca6bdb565 100644 --- a/static/ui.js +++ b/static/ui.js @@ -10538,6 +10538,15 @@ function _armKeepSettledWorklogOpen(streamId){ function _disarmKeepSettledWorklogOpen(){ _keepSettledWorklogOpenForStreamId=null; } +// True while a just-settled worklog is being force-rendered open (between +// _armKeepSettledWorklogOpen and _disarmKeepSettledWorklogOpen). renderMessages() +// consults this so it does NOT write the forced-open DOM into _sessionHtmlCache: +// the keep-open is a transient settle-frame device, and caching it would persist +// the forced-open worklog across session switches / restores, silently overriding +// a user-collapsed worklog. (#5260 gate-cert: keep-open must not leak into cache.) +function _isKeepSettledWorklogOpenArmed(){ + return _keepSettledWorklogOpenForStreamId!==null; +} if(typeof window!=='undefined'){ window._armKeepSettledWorklogOpen=_armKeepSettledWorklogOpen; window._disarmKeepSettledWorklogOpen=_disarmKeepSettledWorklogOpen; @@ -10569,15 +10578,15 @@ function _renderSettledAnchorSceneForMessage(message, segment, rawIdx){ if(streamId&&!_readActivityDisclosureState(activityKey)){ _copyActivityDisclosureState(`live:${streamId}`, activityKey); } - // keepSettledWorklogOpen forces collapsed:false for the ONE settle render of the - // just-settled turn so the live->settled swap is height-stable (no STREAM_DONE - // shrink jump) for both pinned followers AND unpinned mid-turn readers. If an - // unpinned reader had manually collapsed the live worklog, this overrides them - // for that single frame — but the disclosure state is preserved above - // (_copyActivityDisclosureState) and keep-open disarms right after the render - // (messages.js, _disarmKeepSettledWorklogOpen), so the NEXT re-render honors - // their collapsed state again. Net observable effect: a one-frame flash-open - // before it re-collapses — the accepted trade for removing the jump. + // keepSettledWorklogOpen forces collapsed:false for the ONE height-stable settle + // render of the just-settled turn (no STREAM_DONE shrink jump) for both pinned + // followers AND unpinned mid-turn readers. The keep-open is made genuinely + // transient by the STREAM_DONE handler (messages.js): right after this render it + // disarms the token and runs a scroll-PRESERVING collapse pass, so a worklog the + // reader had manually collapsed returns to its copied disclosure state + // (_copyActivityDisclosureState above) without the jump. While the token is + // armed this forced-open DOM is also kept OUT of _sessionHtmlCache + // (_isKeepSettledWorklogOpenArmed), so it never persists across restores. const group=_anchorSceneWorklogGroup(blocks,{ live:false, collapsed:!keepSettledWorklogOpen, @@ -13454,7 +13463,15 @@ function renderMessages(options){ if(typeof _applyMediaPlaybackPreferences==='function') _applyMediaPlaybackPreferences(inner); // Populate session cache so switching back here skips a full rebuild. _sessionHtmlCacheSid=sid; - if(sid&&!INFLIGHT[sid]&&!hasTransientTranscriptUi){ + // Skip caching while the just-settled keep-open token is armed: that render + // force-opens the settled worklog for height-stability, and caching it would + // persist the forced-open DOM across session switches / restores, overriding a + // user-collapsed worklog. The follow-up collapse pass (after disarm) produces + // the correct cacheable DOM on its own render. (#5260 gate-cert.) The typeof + // guard keeps standalone renderMessages() test harnesses (which don't define + // the helper) working — absent helper == not armed == cache normally. + const _keepOpenArmed=(typeof _isKeepSettledWorklogOpenArmed==='function')&&_isKeepSettledWorklogOpenArmed(); + if(sid&&!INFLIGHT[sid]&&!hasTransientTranscriptUi&&!_keepOpenArmed){ const _html=inner.innerHTML; // Only cache sessions with <300KB rendered HTML; evict oldest beyond 8 sessions. if(_html.length<300_000){ diff --git a/tests/test_issue4970_stream_done_shrink_regression.py b/tests/test_issue4970_stream_done_shrink_regression.py index a8dfb6b56..f9d6e4cf3 100644 --- a/tests/test_issue4970_stream_done_shrink_regression.py +++ b/tests/test_issue4970_stream_done_shrink_regression.py @@ -124,3 +124,74 @@ def test_just_settled_turn_stays_open_for_both_pin_states_history_collapses(): assert out["B_history_unpinned"] is False, "historical settled worklog must stay collapsed (unpinned)" assert out["A_after_disarm"] is False, "exception must be one-shot, cleared after the render" assert out["no_token"] is False, "no armed stream id → never keep open" + + +def test_forced_open_dom_is_not_cached_while_token_armed(): + # Round 9 (#5260 gate-cert, RED): the keep-open settle render force-opens the + # worklog; that DOM must NOT be written into _sessionHtmlCache while the token + # is armed, or it persists the forced-open worklog across session switches / + # restores and silently overrides a user-collapsed worklog. + armed = _function_body(UI_JS, "_isKeepSettledWorklogOpenArmed") + assert "_keepSettledWorklogOpenForStreamId!==null" in armed, ( + "_isKeepSettledWorklogOpenArmed() must report whether the one-shot " + "keep-open token is currently armed." + ) + # The cache-write guard in renderMessages must consult the armed check (via a + # typeof-safe local) so the forced-open settle render is excluded from + # _sessionHtmlCache.set(). + assert "typeof _isKeepSettledWorklogOpenArmed==='function'" in UI_JS, ( + "the cache guard must call _isKeepSettledWorklogOpenArmed() through a " + "typeof check so standalone renderMessages() harnesses still work (#5260)." + ) + assert "!_keepOpenArmed" in UI_JS, ( + "the _sessionHtmlCache population guard must include !_keepOpenArmed so the " + "forced-open settle render DOM is not cached (#5260)." + ) + # Anchor it to the actual cache-write site: the armed check sits on the same + # condition as the INFLIGHT / transient-UI guards that gate the cache .set(). + cache_guard_idx = UI_JS.index("!_keepOpenArmed") + window = UI_JS[cache_guard_idx : cache_guard_idx + 400] + assert "_sessionHtmlCache.set(" in window, ( + "the !_keepOpenArmed guard must gate the " + "_sessionHtmlCache.set() call, not some unrelated branch." + ) + + +def test_stream_done_runs_scroll_preserving_collapse_pass_after_disarm(): + # Round 9 (#5260 gate-cert, RED x2): disarming the token alone leaves the + # forced-open worklog on screen. The first re-push collapse-rendered only the + # NON-following path and let a pinned follower fall through to scrollToBottom() + # — but scrollToBottom() does NOT re-render (ui.js), so the armed-open DOM + # persisted for pinned followers. Fix: run the scroll-PRESERVING collapse pass + # UNCONDITIONALLY right after disarm (covers both pin states), THEN scrollToBottom() + # only for followers to re-settle at the tail. This makes keep-open genuinely + # one-frame for everyone. + disarm_idx = MESSAGES_JS.index("_disarmKeepSettledWorklogOpen()") + after = MESSAGES_JS[disarm_idx : disarm_idx + 700] + # The collapse pass must run after disarm for BOTH pin states. + assert "_renderMessagesWithScrollSnapshot()" in after, ( + "after _disarmKeepSettledWorklogOpen() the STREAM_DONE handler must run a " + "scroll-preserving collapse pass (_renderMessagesWithScrollSnapshot) so the " + "forced-open worklog collapses back to the user/live state without the jump." + ) + # The follower re-settle (scrollToBottom) must come AFTER the collapse render — + # otherwise a pinned follower keeps the forced-open DOM (scrollToBottom does not + # re-render). This is the exact pinned-path bug the second RED gate-cert caught. + collapse_pos = after.index("_renderMessagesWithScrollSnapshot()") + follow_pos = after.index("shouldFollowOnDone") + assert collapse_pos < follow_pos, ( + "the collapse render must run BEFORE the shouldFollowOnDone scrollToBottom() " + "so BOTH pinned and unpinned readers get the worklog collapsed; scrollToBottom() " + "alone does not re-render and would leave a pinned follower forced-open." + ) + assert "scrollToBottom()" in after, ( + "a pinned/near-bottom follower must scrollToBottom() after the collapse " + "render to re-settle exactly at the tail." + ) + # And the wrapper must exist and be scroll-preserving (capture → render → + # restore same-frame), so the collapse height change is absorbed by the JS + # restore rather than left to native scroll-anchoring (suppressed on mobile). + wrapper = _function_body(UI_JS, "_renderMessagesWithScrollSnapshot") + assert "_captureMessageScrollSnapshot()" in wrapper + assert "_restoreMessageScrollSnapshotSameFrame" in wrapper +