fix(webui): keep stale transcript mounted while hidden-tab return reloads new messages (#5177) - #5189
allenliang2022 wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| static/sessions.js | Core behavior change: defers synchronous transcript clear on visibility/focus recovery reloads; correctly gated on sameSessionForceReload so cross-session switches are unaffected. Both _ensureMessagesLoaded call sites forwarded the force flag; error paths and _pendingCarryForwardSnapshot carry-forward both work correctly with stale messages in place. |
| tests/test_issue5177_hidden_tab_blank_gap.py | New source-lock test with 7 cases covering the key structural invariants: guard computation, single-site clear, force escape-hatch, recovery-reason map boundaries. Good use of brace-counting parser to tolerate future function growth. |
| tests/test_webui_external_refresh_frontend.py | Updated two anchor strings to match the new loadSession call signature (added keepStaleUntilLoaded parameter) and _ensureMessagesLoaded prefix. No behavioral assertions changed. |
| tests/test_tars_scroll_reset_regressions.py | Single assertion updated to include the new keepStaleUntilLoaded parameter in the loadSession call anchor. No behavioral change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Tab as Browser
participant RASIEU as refreshActiveSession
participant LS as loadSession
participant EML as ensureMessagesLoaded
participant DOM as msgInner DOM
Tab->>RASIEU: visibilitychange visible
RASIEU->>RASIEU: "remoteCount != localCount"
RASIEU->>LS: "force=true keepStaleUntilLoaded=true"
LS->>LS: "_keepStaleUntilLoaded = true AND sameSessionForceReload"
Note over LS,DOM: S.messages NOT cleared, stale transcript stays mounted
LS->>LS: Phase 1 fetch metadata await
Note over DOM: Stale transcript visible during round-trip
LS->>EML: "_ensureMessagesLoaded sid force=true"
EML->>EML: opts.force bypasses early-return
EML->>EML: fetch new messages from server
EML->>EML: "S.messages = newMsgs atomic swap"
EML-->>LS: return
LS->>DOM: renderMessages single frame old replaced by new
%%{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"}}}%%
sequenceDiagram
participant Tab as Browser
participant RASIEU as refreshActiveSession
participant LS as loadSession
participant EML as ensureMessagesLoaded
participant DOM as msgInner DOM
Tab->>RASIEU: visibilitychange visible
RASIEU->>RASIEU: "remoteCount != localCount"
RASIEU->>LS: "force=true keepStaleUntilLoaded=true"
LS->>LS: "_keepStaleUntilLoaded = true AND sameSessionForceReload"
Note over LS,DOM: S.messages NOT cleared, stale transcript stays mounted
LS->>LS: Phase 1 fetch metadata await
Note over DOM: Stale transcript visible during round-trip
LS->>EML: "_ensureMessagesLoaded sid force=true"
EML->>EML: opts.force bypasses early-return
EML->>EML: fetch new messages from server
EML->>EML: "S.messages = newMsgs atomic swap"
EML-->>LS: return
LS->>DOM: renderMessages single frame old replaced by new
Reviews (4): Last reviewed commit: "fix(webui): keep stale transcript mounte..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
0a6d58c to
cdcfebc
Compare
|
Thanks @greptile-apps — P2 addressed in
The corresponding source-lock test ( Also (incidentally — surfaced when the broader test sweep ran on the new signature) updated five existing source-lock tests that anchored on the literal call signatures:
|
|
The The dual-shape test acceptance ( No further concerns on this point — P2 is resolved cleanly. |
🔬 Gate certification — GREEN ✅Certified head: What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Codex (reproduce, on clean rebase) | SAFE TO SHIP (no findings) |
| Opus (full review) | APPROVE — 1 documented non-blocking error-path caveat |
Full pytest suite (-p no:xdist, rebased head 797d141f) |
11091 passed, 0 failed (to completion) |
| PR's own tests (clean checkout) | 7 passed (test_issue5177_hidden_tab_blank_gap.py) |
| 8 edited existing-test files (145 tests) | 145 passed (window-bumps + signature loosen are benign) |
node --check static/sessions.js |
syntax OK |
⚠️ Stale-base note (resolved)
Codex's first pass returned SHIP-ONLY-WITH-FIXES with 3 findings (session-media PDF/HTML auth, macOS launchd git, shared todo renderer) — all stale-base artifacts from other shipped PRs (incl. #5157's media-auth, which I certified GREEN earlier this session). Codex confirmed: "the blockers above are unrelated production reverts in the working-tree diff… test_issue5177 passed 7/7." I rebased onto current master (1ae168aa) locally — clean (rc=0), post-rebase diff is exactly the #5189 files, mergeable. Re-running Codex on the clean diff flipped it to SAFE TO SHIP with zero findings.
Findings (the fix is solid)
- ✅ Cross-session-switch invariant preserved (the key safety property).
_keepStaleUntilLoaded = !!opts.keepStaleUntilLoaded && sameSessionForceReload(sessions.js:1206), andsameSessionForceReload = forceReload && currentSid===sid(1145). A real session switch (currentSid !== sid) can never enable keep-stale → the synchronous clear at :1240 still fires, so a prior session's transcript is never left on screen during navigation (the original bug that clear protects). Verified by me and independently by Codex. - ✅ Atomic swap is real, not a no-op.
_ensureMessagesLoaded(sid, {force:_keepStaleUntilLoaded})(:1510, :1608) bypasses the "messages already populated" early-return only on the keep-stale path; the non-force early-return (:2375) is preserved for everyone else (no perf regression). The new transcript replaces the old DOM in one render frame (old → new), eliminating the old → empty → new flash. - ✅ Reason gating is strict. Only
refreshActiveSessionIfExternallyUpdated's{visible:true, focus:true}recovery reasons request keep-stale (:4563); poll / idle-reconcile / external-import paths keep original behavior, with a commented rationale. - ✅ Scroll preserved —
renderMessages({preserveScroll:true})on same-session reload (:1550, :1665), no scroll-jump on the swap. ⚠️ Non-blocking (Opus, documented in PR body): on a failed fetch while keep-stale is set, the old DOM is replaced by the "Failed to load messages" placeholder — no worse than pre-PR (which blanked to "Loading…"); the guard only defers the successful swap. A nice future follow-up would keep stale DOM on transient fetch failure too, but it's not a regression and not a blocker.
Recommendation to the next agent
Ready to merge — cert fresh for sha:cdcfebc2a1a5. Clean fix-class reliability improvement (existing-flow smoothness, ranks high). Codex SAFE on rebase, Opus APPROVE, 11091/0 suite + 7/7 PR tests + 145/145 touched-test files, cross-session invariant verified. Branch rebases cleanly onto current master. No human-review label; the visible behavior (no blank flash on hidden-tab return) is well-covered by the new tests and the swap-in-place logic is sound. Cert valid only at sha:cdcfebc2a1a5.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close. Cert valid only at sha:cdcfebc2a1a5; a new push invalidates it → re-gate.
nesquena-hermes
added
the
gate-pass
…oads new messages (nesquena#5177) After a hidden interval that added persisted messages (post-turn bg-review writes, sibling-tab writes, the just-finished main turn flushing), switching the tab back caused refreshActiveSessionIfExternallyUpdated('visible'|'focus') to hit remoteCount !== localCount and call loadSession(sid, {force:true}), which synchronously did S.messages = [] before awaiting the metadata + messages fetches. The visible result was the entire conversation transcript blanking for the round-trip and reappearing — '对话突然消失，重刷才回来'. sessions.js itself warns about this 'disappear and reappear' tradeoff at the top of the remoteCount/remoteLast guard, but only short-circuits the metadata-only branch (nesquena#5061). nesquena#5122's 4-probe staged reconnect covers a different path (SSE error with ready_state=2 on a visible tab) and does not apply when the SSE error arrives while visibility_state='hidden' — that bottoms out through _deferStreamErrorIfPageHidden before the reconnect block ever runs. Fix: - refreshActiveSessionIfExternallyUpdated maps the visibility/focus recovery reasons ({visible, focus}) to a new keepStaleUntilLoaded option, forwarded to loadSession. The post-stream idle reconcile and the poll/external paths keep their existing behaviour. - loadSession ANDs opts.keepStaleUntilLoaded with sameSessionForceReload (so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a navigation is the original bug the clear was written for) and, on that path, skips the synchronous S.messages/S.toolCalls/_messagesTruncated/_oldestIdx clear. The new transcript is SWAPPED into S.messages by _ensureMessagesLoaded(sid, {force:true}), so the user sees old DOM directly replaced by new DOM in a single render frame. - _ensureMessagesLoaded grows an opts.force escape hatch so its 'messages already populated' early-return cannot skip the swap when stale messages are still in place. Verified live on a source 8701 build: - before: minHtmlObserved 65874, minKidsObserved 11 — 372 samples at 16ms, DOM html length NEVER dipped below the pre-call value during the reload. - loadSession was called with force:true keepStale:true reason:'visible', outcome 'reloaded', sMsgs 21 → 1037, after kids 11 → 134 (single-frame swap). Tests: - tests/test_issue5177_hidden_tab_blank_gap.py (new, 7 cases): source-locks the keep-stale guard shape, the recovery-reason map, the _ensureMessagesLoaded opts.force escape hatch, and that the synchronous clear is wrapped in if(!_keepStaleUntilLoaded). - Updated the two existing source-lock tests whose anchor strings included the loadSession call signature (tests/test_webui_external_refresh_frontend.py + tests/test_tars_scroll_reset_regressions.py) to match the new loadSession(... keepStaleUntilLoaded:_keepStaleUntilLoaded) form. No behavioural assertions changed. Local: node --check static/sessions.js OK. 122 passed across test_issue5177_hidden_tab_blank_gap.py + the full adjacent set (external-refresh / journal-frontend / scroll-reset / reconnect-chronology / inflight-stream-reuse / sse-error-multi-probe / issue4295 / issue4856).
cdcfebc to
f9a8aea
Compare
…turn (#5189, @allenliang2022) Release v0.51.744 — no transcript blank-flash on hidden-tab return (#5189, @allenliang2022)
|
Shipped in v0.51.744 (via #5219). Thanks @allenliang2022 — returning to a hidden tab after new messages arrived no longer blanks+flashes the transcript (kept stale-mounted until the reload completes). Fresh-gated: Codex SAFE TO SHIP (no wrong-session transcript — keep-stale gated on same-session force-reload; no double-mount; visible/focus-only), full suite 11018 passed. Maintainer-approved. Closes #5177. |
…E gap on a visible tab A visible tab can permanently miss a server-initiated turn (self-wake / cron / restart hook) when its per-session SSE is momentarily down at the emit instant. server_turn_started is a fire-and-forget broadcast with no replay buffer; if the turn also finishes before the tab reconnects, the on-subscribe self-heal finds no live run in ACTIVE_RUNS and replays nothing, so the transcript stays stale until a hard refresh. Extend the self-heal: the (re)subscribing tab reports its last-known message_count (?known_count); when there is no live run to replay, the handler compares the persisted count and, if the server is ahead, emits a lightweight session-updated frame. The frontend syncs incrementally via the nesquena#5189 keepStaleUntilLoaded swap-in-place loadSession path (no clear+refetch, so the nesquena#5177/nesquena#5189 blank-gap jump is not reintroduced). Idle-only and pane-scoped; emits only when the server is strictly ahead.
…E gap on a visible tab A visible tab can permanently miss a server-initiated turn (self-wake / cron / restart hook) when its per-session SSE is momentarily down at the emit instant. server_turn_started is a fire-and-forget broadcast with no replay buffer; if the turn also finishes before the tab reconnects, the on-subscribe self-heal finds no live run in ACTIVE_RUNS and replays nothing, so the transcript stays stale until a hard refresh. Extend the self-heal: the (re)subscribing tab reports its last-known message_count (?known_count); when there is no live run to replay, the handler compares the persisted count and, if the server is ahead, emits a lightweight session-updated frame. The frontend syncs incrementally via the nesquena#5189 keepStaleUntilLoaded swap-in-place loadSession path (no clear+refetch, so the nesquena#5177/nesquena#5189 blank-gap jump is not reintroduced). Idle-only and pane-scoped; emits only when the server is strictly ahead.
…E gap on a visible tab A visible tab can permanently miss a server-initiated turn (self-wake / cron / restart hook) when its per-session SSE is momentarily down at the emit instant. server_turn_started is a fire-and-forget broadcast with no replay buffer; if the turn also finishes before the tab reconnects, the on-subscribe self-heal finds no live run in ACTIVE_RUNS and replays nothing, so the transcript stays stale until a hard refresh. Extend the self-heal: the (re)subscribing tab reports its last-known message_count (?known_count); when there is no live run to replay, the handler compares the persisted count and, if the server is ahead, emits a lightweight session-updated frame. The frontend syncs incrementally via the nesquena#5189 keepStaleUntilLoaded swap-in-place loadSession path (no clear+refetch, so the nesquena#5177/nesquena#5189 blank-gap jump is not reintroduced). Idle-only and pane-scoped; emits only when the server is strictly ahead.
…E gap on a visible tab A visible tab can permanently miss a server-initiated turn (self-wake / cron / restart hook) when its per-session SSE is momentarily down at the emit instant. server_turn_started is a fire-and-forget broadcast with no replay buffer; if the turn also finishes before the tab reconnects, the on-subscribe self-heal finds no live run in ACTIVE_RUNS and replays nothing, so the transcript stays stale until a hard refresh. Extend the self-heal: the (re)subscribing tab reports its last-known message_count (?known_count); when there is no live run to replay, the handler compares the persisted count and, if the server is ahead, emits a lightweight session-updated frame. The frontend syncs incrementally via the nesquena#5189 keepStaleUntilLoaded swap-in-place loadSession path (no clear+refetch, so the nesquena#5177/nesquena#5189 blank-gap jump is not reintroduced). Idle-only and pane-scoped; emits only when the server is strictly ahead.
…E gap on a visible tab A visible tab can permanently miss a server-initiated turn (self-wake / cron / restart hook) when its per-session SSE is momentarily down at the emit instant. server_turn_started is a fire-and-forget broadcast with no replay buffer; if the turn also finishes before the tab reconnects, the on-subscribe self-heal finds no live run in ACTIVE_RUNS and replays nothing, so the transcript stays stale until a hard refresh. Extend the self-heal: the (re)subscribing tab reports its last-known message_count (?known_count); when there is no live run to replay, the handler compares the persisted count and, if the server is ahead, emits a lightweight session-updated frame. The frontend syncs incrementally via the nesquena#5189 keepStaleUntilLoaded swap-in-place loadSession path (no clear+refetch, so the nesquena#5177/nesquena#5189 blank-gap jump is not reintroduced). Idle-only and pane-scoped; emits only when the server is strictly ahead.
2 participants
Fixes #5177.
Problem
After a hidden interval that added real persisted messages (post-turn
bg-reviewthread writesskill_manage/memory, a sibling tab adds, the just-finished main turn flushes), switching the tab back caused the entire transcript to visibly blank for the round-trip and then reappear on the source WebUI. The user-reported field repro is in #5177 (<session>,a local LAN client, a source clone with #5061 and #5122 verified loaded).Why neither #5061 nor #5122 caught it
remoteCount === localCount && remoteLast > localLast. Hidden-window-added messages haveremoteCount > localCount→ falls through to the destructiveloadSession(force)branch.visibility_state='hidden',_deferStreamErrorIfPageHiddenreturns before the_reconnectAttemptedblock; the 4-probe never runs.static/sessions.jsitself warns about exactly this disappear/reappear pattern in the inline comment at the top of theremoteCount/remoteLastguard — but only short-circuits the metadata-only branch. The user-visible blank on the count-changed branch is the gap #5177 documents.Fix
Keep the stale transcript mounted while the recovery reload runs, then swap the new transcript into place atomically. Three small, additive changes:
refreshActiveSessionIfExternallyUpdatedmaps the visibility/focus recovery reasons ({visible, focus}) to a newkeepStaleUntilLoadedoption, forwarded toloadSession. The post-stream idle reconcile and the poll / external-imported-session paths keep their existing behaviour (the design comment captures the rationale).loadSessionANDsopts.keepStaleUntilLoadedwithsameSessionForceReload(so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a real navigation is the original bug the clear was written for) and, on that path, skips the synchronousS.messages/S.toolCalls/_messagesTruncated/_oldestIdxclear. The new transcript is SWAPPED intoS.messagesby_ensureMessagesLoaded(sid, {force:true})so the user sees old DOM directly replaced by new DOM in a single render frame._ensureMessagesLoadedgrows anopts.forceescape hatch so its"messages already populated"early-return cannot skip the swap when stale messages are still in place.Verification
Source-lock tests
tests/test_issue5177_hidden_tab_blank_gap.py(7 cases) — locks the keep-stale guard shape, the recovery-reason map, the_ensureMessagesLoadedopts.forceescape hatch, and that the synchronous clear is wrapped inif (!_keepStaleUntilLoaded). Also asserts'poll'/'idle-reconcile'are not in the recovery-reason map (so any accidental widening trips the lock).tests/test_webui_external_refresh_frontend.pyandtests/test_tars_scroll_reset_regressions.py— their anchor strings contained the literalloadSessioncall signature, updated to match the newkeepStaleUntilLoaded:_keepStaleUntilLoadedform. No behavioural assertions changed.Live verification on a source build of nesquena/hermes-webui at HEAD
Reproduced and verified end-to-end with Playwright on the affected session that originated #5177:
#msgInnerHTML length andchildElementCountacross the entire reload.outcome: 'reloaded'— theremoteCount !== localCountbranch did fire.loadSessionwas called with{force:true, keepStaleUntilLoaded:true, externalRefreshReason:'visible'}— the flag flows through correctly.minHtmlObserved: 65874,minKidsObserved: 11across 372 samples — the DOM never dipped below the pre-call values.S.messagesswap-in-place: small → full transcript, child count: 11 → 134.The old path (
keepStaleUntilLoaded: false) does still clear and refetch as before; that path is exercised by existing tests and visibly destroyed the page execution context when I tried to repro it for comparison (further evidence the new path is not a no-op).Adjacent locks
node --check static/sessions.jsOK. 122 passed across:tests/test_issue5177_hidden_tab_blank_gap.pytests/test_webui_external_refresh_frontend.pytests/test_issue3916_external_refresh_poll.pytests/test_run_journal_frontend_static.pytests/test_tars_scroll_reset_regressions.pytests/test_issue4811_reconnect_chronology.pytests/test_inflight_stream_reuse.pytests/test_sse_error_multi_probe_reconnect.pytests/test_issue4295_scroll_pin_reentry.pytests/test_issue4856_android_scroll_regression.pyScope notes
static/sessions.js) plus the new source-lock test, plus two sibling source-lock tests updated for the call-site signature change.sameSessionForceReloadso any navigation path that needs to wipe a prior session's transcript still does so synchronously.loadSession, the existing error handlers (Failed to load session,Failed to load conversation messages) still surface a toast and replace#msgInnerwith the error placeholder — the keep-stale guard only defers the successful swap.visible+focus).pageshow/ mobile-bfcache restore aren't currently wired throughrefreshActiveSessionIfExternallyUpdated, so this PR doesn't change them; if you'd like that path covered too I'm happy to extend in a follow-up.Tracked-from: #5177