From 9c013eaaf80a7adad9f46d21d5c8b60ad11cd9ad Mon Sep 17 00:00:00 2001 From: chelsealong Date: Mon, 24 Aug 2026 04:13:04 +0000 Subject: [PATCH] fix(dashboard): follow scroll on implicit active-session resume (#93518) pty_ws already fell back to the per-channel active-session file when a /chat WS connects with no ?resume= param, replaying the whole session into the PTY, but the frontend only pinned xterm's viewport to the bottom when resumeParam came from the URL (#59591). The implicit path had no way to learn a replay was happening, so the viewport stayed at the top of the scrollback. pty_ws now sends a one-off JSON control frame naming the session id it resolved from the active-session file, before any PTY bytes; PTY output itself always arrives as binary frames, so this is unambiguous on the wire. ChatPage tracks an `effectiveResume` value seeded from resumeParam and updated when this control frame arrives, and the existing follow-scroll/sanitizer/hydration logic keys off it instead of the URL param alone. Fixes #93518. --- hermes_cli/web_server.py | 6 +++ .../test_web_server_pty_reconnect.py | 39 +++++++++++++++ web/src/lib/pty-scroll.test.ts | 33 +++++++++++- web/src/lib/pty-scroll.ts | 28 +++++++++++ web/src/pages/ChatPage.tsx | 50 ++++++++++++++++--- 5 files changed, 147 insertions(+), 9 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index f2933fafe1..7444ebf2f0 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -17409,6 +17409,12 @@ async def pty_ws(ws: WebSocket) -> None: _forget_active_session_file(active_session_file) elif not resume: resume = _read_active_session_file(active_session_file) + if resume: + # The client only knows to pin the viewport to the bottom + # when it requested `?resume=`. Tell it a replay is coming + # anyway so the implicit active-session fallback gets the + # same follow-scroll treatment as an explicit resume (#93518). + await ws.send_json({"type": "resume", "id": resume}) resolve_kwargs = { "resume": resume, diff --git a/tests/hermes_cli/test_web_server_pty_reconnect.py b/tests/hermes_cli/test_web_server_pty_reconnect.py index 6a1185e7bb..60b6833c30 100644 --- a/tests/hermes_cli/test_web_server_pty_reconnect.py +++ b/tests/hermes_cli/test_web_server_pty_reconnect.py @@ -83,6 +83,45 @@ def test_fresh_param_ignores_channel_active_session_file(pty_client, monkeypatch assert not active_file.exists() +def test_active_session_fallback_sends_resume_control_message(pty_client, monkeypatch): + """Implicit resume (no `?resume=`) must tell the client which session. + + Regression for #93518: the dashboard's stick-to-bottom replay logic only + fires when the frontend can see a resume id. Without `?resume=` on the URL + it previously had no way to learn that `pty_ws` fell back to the + per-channel active-session file, so the viewport stayed pinned at the top + of the replayed scrollback. + """ + ws, client, token = pty_client + channel = "implicit-resume-chan" + active_file = ws._active_session_file_for_channel(ws.app, channel) + active_file.write_text(json.dumps({"session_id": "sess-old"}), encoding="utf-8") + + monkeypatch.setattr( + ws, "_resolve_chat_argv", lambda **kw: (["fake-hermes-tui"], None, None) + ) + + with client.websocket_connect(_url(token, channel=channel)) as conn: + assert conn.receive_json() == {"type": "resume", "id": "sess-old"} + assert conn.receive_bytes() == b"ready" + + +def test_explicit_resume_sends_no_control_message(pty_client, monkeypatch): + """An explicit `?resume=` already tells the client via the URL param.""" + ws, client, token = pty_client + channel = "explicit-resume-chan" + + monkeypatch.setattr( + ws, "_resolve_chat_argv", lambda **kw: (["fake-hermes-tui"], None, None) + ) + + with client.websocket_connect( + _url(token, channel=channel, resume="sess-explicit") + ) as conn: + # The first (and only) frame is PTY output, not a control message. + assert conn.receive_bytes() == b"ready" + + def test_child_eof_closes_socket_and_bridge(pty_client, monkeypatch): """Child EOF must close the WS server-side and reap the PTY. diff --git a/web/src/lib/pty-scroll.test.ts b/web/src/lib/pty-scroll.test.ts index 887efa3b70..d87ea5967a 100644 --- a/web/src/lib/pty-scroll.test.ts +++ b/web/src/lib/pty-scroll.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; -import { isViewportPinnedToBottom, shouldFollowPtyOutput } from "./pty-scroll"; +import { + isViewportPinnedToBottom, + parseResumeControlMessage, + shouldFollowPtyOutput, +} from "./pty-scroll"; describe("isViewportPinnedToBottom", () => { it("is pinned when the viewport sits on the bottom row", () => { @@ -47,3 +51,30 @@ describe("shouldFollowPtyOutput", () => { expect(shouldFollowPtyOutput("", true)).toBe(false); }); }); + +describe("parseResumeControlMessage", () => { + it("extracts the id from a resume control frame", () => { + // #93518: the implicit active-session fallback has no `?resume=` on the + // URL, so the server names the session it resolved in a control frame. + expect( + parseResumeControlMessage('{"type":"resume","id":"sess-123"}'), + ).toBe("sess-123"); + }); + + it("ignores plain ANSI banner text sent as a text frame", () => { + // pty_ws also sends "Chat unavailable: ..." error banners as text + // frames; those must keep rendering into the terminal, not get + // swallowed as a (mis-parsed) control message. + expect( + parseResumeControlMessage("\r\n\x1b[31mChat unavailable: x\x1b[0m\r\n"), + ).toBeNull(); + }); + + it("ignores JSON of the wrong shape", () => { + expect(parseResumeControlMessage('{"type":"other","id":"x"}')).toBeNull(); + expect(parseResumeControlMessage('{"type":"resume"}')).toBeNull(); + expect(parseResumeControlMessage('{"type":"resume","id":""}')).toBeNull(); + expect(parseResumeControlMessage("null")).toBeNull(); + expect(parseResumeControlMessage('"resume"')).toBeNull(); + }); +}); diff --git a/web/src/lib/pty-scroll.ts b/web/src/lib/pty-scroll.ts index 6c0b15ceff..9290ae3131 100644 --- a/web/src/lib/pty-scroll.ts +++ b/web/src/lib/pty-scroll.ts @@ -48,3 +48,31 @@ export function shouldFollowPtyOutput( ): boolean { return Boolean(resumeParam) && stickToBottom; } + +/** + * When `pty_ws` falls back to the per-channel active-session file (no + * `?resume=` on the URL), the server sends a one-off JSON text frame naming + * the session it resolved before any PTY replay bytes arrive, since the + * frontend has no other way to learn a replay is happening (#93518). PTY + * output itself always arrives as binary frames, so any text frame is a + * candidate; this returns the resume id on a match and `null` for anything + * else (including the plain ANSI error banners `pty_ws` still sends as text + * on failure, which must keep rendering into the terminal as before). + */ +export function parseResumeControlMessage(data: string): string | null { + try { + const parsed = JSON.parse(data); + if ( + parsed && + typeof parsed === "object" && + parsed.type === "resume" && + typeof parsed.id === "string" && + parsed.id + ) { + return parsed.id; + } + } catch { + /* not JSON — an ANSI banner or other plain-text frame */ + } + return null; +} diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 371c145556..244fa16807 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -68,6 +68,7 @@ import { } from "@/lib/pty-keyboard-shortcuts"; import { isViewportPinnedToBottom, + parseResumeControlMessage, shouldFollowPtyOutput, } from "@/lib/pty-scroll"; import { @@ -1064,6 +1065,11 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // ``return cleanup`` stays at the top level; handlers + disposables // are hoisted to ``let`` bindings the cleanup closes over. let unmounting = false; + // The implicit active-session fallback (no `?resume=` on the URL) only + // becomes known once the server's control frame arrives (see + // `ws.onmessage` below) — everything gated on "is this a resume replay" + // reads this instead of `resumeParam` directly (#93518). + let effectiveResume = resumeParam; let onDataDisposable: { dispose(): void } | null = null; let onResizeDisposable: { dispose(): void } | null = null; let onScrollDisposable: { dispose(): void } | null = null; @@ -1088,7 +1094,7 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { } }; const noteResumePtyChunk = (chunkText: string) => { - if (!resumeParam || unmounting) { + if (!effectiveResume || unmounting) { return; } if (shouldFinishResumeHydrationOnChunk(chunkText)) { @@ -1256,14 +1262,42 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // in-place redraws through untouched. See pty-resume-sanitizer.ts. const decoder = new TextDecoder(); const sanitizer = new PtyResumeSanitizer(); + const beginResumeReplay = () => { + stickToBottomRef.current = true; + if (!eraseSuppressionTimer) { + eraseSuppressionTimer = setTimeout(() => { + eraseSuppressionTimer = null; + sanitizer.endEraseSuppression(); + }, PTY_RESUME_SANITIZE_WINDOW_MS); + } + if (!resumeMaxTimer) { + setResumeHydrating(true); + resumeMaxTimer = setTimeout( + finishResumeHydration, + PTY_RESUME_LOADING_MAX_MS, + ); + } + }; if (resumeParam) { - eraseSuppressionTimer = setTimeout(() => { - eraseSuppressionTimer = null; - sanitizer.endEraseSuppression(); - }, PTY_RESUME_SANITIZE_WINDOW_MS); + beginResumeReplay(); } ws.onmessage = (ev) => { + if (typeof ev.data === "string") { + // The active-session fallback (no `?resume=` on the URL) tells us + // via a one-off JSON control frame that a replay is starting (#93518, + // see `pty_ws` in web_server.py). Real PTY output always arrives as + // binary frames, so any text frame is a candidate; anything that + // isn't this control shape (e.g. the ANSI "Chat unavailable" banners + // pty_ws sends as text on failure) falls through to the write path + // below unchanged. + const resumeId = parseResumeControlMessage(ev.data); + if (resumeId) { + effectiveResume = resumeId; + beginResumeReplay(); + return; + } + } const text = typeof ev.data === "string" ? ev.data @@ -1274,13 +1308,13 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // sanitizer can turn a nonempty erase-only / all-newline / partial-CSI // resume frame into "" (pty-resume-sanitizer.ts); keying off raw `text` // would hide the wait notice while the terminal is still blank. - const rendered = resumeParam ? sanitizer.next(text) : text; + const rendered = effectiveResume ? sanitizer.next(text) : text; // Resume replay lands over many write chunks; pin the viewport to the // bottom as each chunk COMMITS (xterm write callback) instead of // guessing with a fixed delay, and release the pin the moment the user // scrolls up to read the backlog (#59591). const followScroll = shouldFollowPtyOutput( - resumeParam, + effectiveResume, stickToBottomRef.current, ) ? () => termRef.current?.scrollToBottom() @@ -1293,7 +1327,7 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // Drain buffered sanitizer state. A buffered partial escape is dropped // (writing an unterminated CSI would wedge xterm's parser); a buffered // newline run is emitted collapsed. - if (resumeParam) { + if (effectiveResume) { clearEraseSuppressionTimer(); try { term.write(sanitizer.flush());