🤖 refactor: the terminal router keeps its sessions across a reconnect

src/browser/components/TerminalView/TerminalView.test.tsx

@@ -12,7 +12,7 @@ import { restoreModulesAfterSuite } from "../../../../tests/ui/moduleMocks";
 
 interface TerminalSubscribeCallbacks {
   onOutput: (data: string) => void;
-  onScreenState: (state: string) => void;
+  onScreenState: (state: string, options?: { replace: boolean }) => void;
   onExit: (code: number) => void;
 }
 
@@ -55,8 +55,14 @@ class MockTerminal {
   cols = 80;
   rows = 24;
   options: MockTerminalOptions;
+  calls: string[] = [];
   clear = mock(() => undefined);
-  write = mock((_data: string) => undefined);
+  reset = mock(() => {
+    this.calls.push("reset");
+  });
+  write = mock((data: string) => {
+    this.calls.push(`write:${data}`);
+  });
   resize = mock((cols: number, rows: number) => {
     this.cols = cols;
     this.rows = rows;
@@ -177,6 +183,24 @@ describe("TerminalView", () => {
     mock.restore();
   });
 
+  // After a reconnect the router sends the full screen again. Written on top of the old screen,
+  // the scrollback would repeat, so a replacing screen starts from an empty terminal.
+  test("a screen that replaces the old one is written after a full VT reset", async () => {
+    renderTerminal(mock((_exitCode: number) => undefined));
+    await waitFor(() => expect(mockRouter.subscribe).toHaveBeenCalledTimes(1));
+    const terminal = terminalInstances[0];
+    expect(terminal?.calls).toEqual(["write:initial screen"]);
+
+    act(() => subscribeCallbacks[0]?.onScreenState("screen after reconnect", { replace: true }));
+    // One write: a full reset (RIS) in the VT, then the screen. Never Terminal.reset(), which
+    // frees ghostty's WASM terminal while other parts of ghostty-web still use it.
+    expect(terminal?.calls).toEqual(["write:initial screen", "write:\x1bcscreen after reconnect"]);
+    expect(terminal?.reset).toHaveBeenCalledTimes(0);
+    // The view keeps its one subscription: no new subscribe, no clear.
+    expect(mockRouter.subscribe).toHaveBeenCalledTimes(1);
+    expect(terminal?.clear).toHaveBeenCalledTimes(1);
+  });
+
   test("keeps the router subscription stable when onExit changes", async () => {
     const firstOnExit = mock((_exitCode: number) => undefined);
     const secondOnExit = mock((_exitCode: number) => undefined);

src/browser/components/TerminalView/TerminalView.tsx

@@ -101,6 +101,34 @@ async function preloadTerminalWebfonts(
   }
 }
 
+/** RIS (full reset): clears the screen, the scrollback and the modes inside the VT itself. */
+const FULL_RESET = "\x1bc";
+
+/**
+ * Writes a full screen from the server (may be empty for new sessions).
+ *
+ * `replace` is set after a reconnect, when the router reattaches and sends the full screen again
+ * (TerminalSessionRouter). clear() keeps the scrollback, so writing on top would repeat it: the
+ * screen is written after a full reset instead, in one write, so no empty frame shows.
+ *
+ * Not Terminal.reset(): in ghostty-web it frees the WASM terminal and makes a new one, but the
+ * selection manager and input handler keep the freed one, and the next write after a second
+ * reconnect failed with "memory access out of bounds" and garbled the screen.
+ */
+export function writeScreenState(
+  term: Pick<Terminal, "write">,
+  state: string,
+  replace: boolean
+): void {
+  if (!replace && !state) return;
+  try {
+    term.write(replace ? FULL_RESET + state : state);
+  } catch (err) {
+    // xterm WASM can throw "memory access out of bounds" intermittently
+    console.warn("[TerminalView] Error writing screenState:", err);
+  }
+}
+
 interface TerminalViewProps {
   workspaceId: string;
   /** Session ID to connect to (required - must be created before mounting) */
@@ -365,16 +393,8 @@ export function TerminalView({
           console.warn("[TerminalView] Error writing output:", err);
         }
       },
-      onScreenState: (state) => {
-        // Write screen state (may be empty for new sessions)
-        if (state) {
-          try {
-            term.write(state);
-          } catch (err) {
-            // xterm WASM can throw "memory access out of bounds" intermittently
-            console.warn("[TerminalView] Error writing screenState:", err);
-          }
-        }
+      onScreenState: (state, options) => {
+        writeScreenState(term, state, options?.replace === true);
         // Mark loading complete - we now have valid content to show
         setIsLoading(false);
       },

src/browser/components/TerminalView/writeScreenState.test.ts

@@ -0,0 +1,55 @@
+import { describe, expect, test } from "bun:test";
+import { dirname, join } from "node:path";
+import { Ghostty } from "ghostty-web";
+import { writeScreenState } from "./TerminalView";
+
+// From the file: the test DOM's URL rejects the long data: URL that Ghostty.load() uses by default.
+const WASM_PATH = join(dirname(Bun.resolveSync("ghostty-web", import.meta.dir)), "ghostty-vt.wasm");
+
+/** Every non-empty line of a real ghostty VT, scrollback first. */
+function visibleLines(term: ReturnType<Ghostty["createTerminal"]>, rows: number): string[] {
+  const text = (cells: Array<{ codepoint: number }> | null) =>
+    (cells ?? [])
+      .map((cell) => String.fromCodePoint(cell.codepoint || 32))
+      .join("")
+      .trimEnd();
+  const lines: string[] = [];
+  for (let i = 0; i < term.getScrollbackLength(); i++) lines.push(text(term.getScrollbackLine(i)));
+  for (let row = 0; row < rows; row++) lines.push(text(term.getLine(row)));
+  return lines.filter((line) => line.length > 0);
+}
+
+// Runs on the real VT (WASM), so a ghostty-web change to how RIS treats the scrollback fails here.
+describe("writeScreenState on the ghostty VT", () => {
+  test("a replacing screen leaves no line of the old screen or scrollback behind", async () => {
+    const ghostty = await Ghostty.load(WASM_PATH);
+    const term = ghostty.createTerminal(80, 24);
+    try {
+      let before = "";
+      for (let i = 1; i <= 60; i++) before += `line-${i}\r\n`;
+      term.write(before);
+
+      // The server's screen state after a reconnect: its own copy of the same scrollback.
+      writeScreenState({ write: (data) => term.write(data) }, before + "after-reconnect\r\n", true);
+
+      const lines = visibleLines(term, 24);
+      expect(lines.filter((line) => line === "line-1")).toHaveLength(1);
+      expect(lines.filter((line) => line === "line-60")).toHaveLength(1);
+      expect(lines.at(-1)).toBe("after-reconnect");
+    } finally {
+      term.free();
+    }
+  });
+
+  test("a first screen is written as is", async () => {
+    const ghostty = await Ghostty.load(WASM_PATH);
+    const term = ghostty.createTerminal(80, 24);
+    try {
+      term.write("kept\r\n");
+      writeScreenState({ write: (data) => term.write(data) }, "first\r\n", false);
+      expect(visibleLines(term, 24)).toEqual(["kept", "first"]);
+    } finally {
+      term.free();
+    }
+  });
+});

src/browser/contexts/subscriptionInventory.test.ts

@@ -16,8 +16,8 @@ type RestartRule =
   | "effect-deps"
   /** A store fed through setClient(): it drops the client while offline and resubscribes. */
   | "store-live-feed"
-  /** The terminal router: today the provider builds a new router per client. */
-  | "router-per-client"
+  /** The terminal router: setClient() restarts its streams on each new client. */
+  | "router-setClient"
   /** A long-lived holder with its own capped backoff. */
   | "holder-backoff"
   /** One operation's progress stream (a clone): it is not restarted, the operation fails. */
@@ -56,7 +56,7 @@ const RESTART_RULES: Record<string, RestartRule> = {
   "src/browser/stores/ProvidersConfigStore.ts": "store-live-feed",
   "src/browser/stores/ReviewStateStore.ts": "store-live-feed",
   "src/browser/stores/WorkspaceStore.ts": "store-live-feed",
-  "src/browser/terminal/TerminalSessionRouter.ts": "router-per-client",
+  "src/browser/terminal/TerminalSessionRouter.ts": "router-setClient",
 };
 
 /**
@@ -72,10 +72,11 @@ const RULE_SHAPES: Record<RestartRule, { describe: string; matches: (text: strin
     describe: "a setClient(client) method that the app feeds with the live client",
     matches: (text) => /\bsetClient\s*\(\s*\w+\s*:/.test(text),
   },
-  "router-per-client": {
-    describe: "a router bound to one client (the provider builds one per client)",
-    matches: (text) => /constructor\(api: APIClient\b/.test(text),
+  "router-setClient": {
+    describe: "a setClient(api) method that restarts the streams",
+    matches: (text) => /\bsetClient\s*\(\s*api\s*:/.test(text),
   },
+
   "holder-backoff": {
     describe: "a retry with a delay",
     matches: (text) => text.includes("setTimeout(") && /backoff|retry/i.test(text),

src/browser/terminal/TerminalRouterContext.test.tsx

@@ -0,0 +1,111 @@
+import "../../../tests/ui/dom";
+
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import { cleanup, render, waitFor } from "@testing-library/react";
+import { useEffect } from "react";
+import { installDom } from "../../../tests/ui/dom";
+import { createConnectionHarness, GAP_MODES } from "@/browser/contexts/connection.testHarness";
+import { TerminalRouterProvider, useTerminalRouter } from "./TerminalRouterContext";
+import type { TerminalSessionRouter } from "./TerminalSessionRouter";
+
+/** One entry per connect: the attach calls that connection's server saw. */
+let attachesPerConnection: number[] = [];
+/** One entry per connect: the sizes that connection's server got. */
+let resizesPerConnection: Array<Array<{ cols: number; rows: number }>> = [];
+
+function serverOverrides() {
+  const index = attachesPerConnection.length;
+  attachesPerConnection.push(0);
+  resizesPerConnection.push([]);
+  return {
+    terminal: {
+      attach: () => {
+        attachesPerConnection[index] += 1;
+        return Promise.resolve(
+          (async function* () {
+            yield { type: "screenState" as const, data: `screen-${index}` };
+            await new Promise(() => undefined); // stays open until aborted
+          })()
+        );
+      },
+      // An exit stream that never yields: the process keeps running.
+      onExit: () =>
+        Promise.resolve(
+          (async function* () {
+            await new Promise(() => undefined);
+            yield 0;
+          })()
+        ),
+      resize: (input: { cols: number; rows: number }) => {
+        resizesPerConnection[index]?.push({ cols: input.cols, rows: input.rows });
+        return Promise.resolve();
+      },
+    },
+  };
+}
+
+let routers: Array<TerminalSessionRouter | null> = [];
+let subscribes = 0;
+let screens: Array<{ state: string; replace: boolean }> = [];
+
+/** Subscribes once per router, as TerminalView does. */
+function TerminalProbe() {
+  const router = useTerminalRouter();
+  routers.push(router);
+  useEffect(() => {
+    if (router == null) return;
+    subscribes += 1;
+    return router.subscribe("t1", {
+      onOutput: () => undefined,
+      onScreenState: (state, options) =>
+        screens.push({ state, replace: options?.replace === true }),
+      onExit: () => undefined,
+    });
+  }, [router]);
+  return null;
+}
+
+describe.each([...GAP_MODES])("TerminalRouterProvider across a reconnect (%s)", (mode) => {
+  let cleanupDom: (() => void) | null = null;
+  beforeEach(() => {
+    cleanupDom = installDom();
+    attachesPerConnection = [];
+    resizesPerConnection = [];
+    routers = [];
+    subscribes = 0;
+    screens = [];
+  });
+  afterEach(() => {
+    cleanup();
+    cleanupDom?.();
+    cleanupDom = null;
+  });
+
+  test("the terminal keeps its router and subscription, and reattaches on the new client", async () => {
+    const harness = createConnectionHarness(mode, serverOverrides);
+    render(
+      <TerminalRouterProvider>
+        <TerminalProbe />
+      </TerminalRouterProvider>,
+      { wrapper: harness.Wrapper }
+    );
+    await waitFor(() => expect(screens).toEqual([{ state: "screen-0", replace: false }]));
+    const router = routers.at(-1) ?? null;
+    expect(router).not.toBeNull();
+
+    await harness.drop();
+    // A resize in the gap goes to no client, and to the PTY once the next client attaches.
+    await router?.resize("t1", 100, 30);
+    expect(resizesPerConnection[0]).toEqual([]);
+    harness.reconnect();
+    await waitFor(() => expect(resizesPerConnection[1]).toEqual([{ cols: 100, rows: 30 }]));
+    await waitFor(() => expect(attachesPerConnection).toEqual([1, 1]));
+    await waitFor(() => expect(screens.at(-1)).toEqual({ state: "screen-1", replace: true }));
+
+    // A new router (or a null one in the gap) made the view subscribe again and clear itself.
+    const distinct: unknown[] = [...new Set(routers.filter((r) => r != null))];
+    expect(distinct).toEqual([router]);
+    expect(routers.slice(routers.indexOf(router))).not.toContain(null);
+    expect(subscribes).toBe(1);
+  });
+});

src/browser/terminal/TerminalRouterContext.tsx

@@ -6,7 +6,7 @@
  */
 
 import { createContext, useContext, useEffect, useState } from "react";
-import { useAPI } from "@/browser/contexts/API";
+import { useLiveAPI } from "@/browser/contexts/API";
 import { TerminalSessionRouter } from "./TerminalSessionRouter";
 
 const TerminalRouterContext = createContext<TerminalSessionRouter | null>(null);
@@ -20,46 +20,40 @@ interface TerminalRouterProviderProps {
 /**
  * Provides TerminalSessionRouter to the component tree.
  *
- * Creates a single router instance that lives for the lifetime of the provider.
- * The router is recreated if the API client changes (e.g., reconnection).
+ * One router lives as long as the provider. Each new API client (a reconnect) goes to the same
+ * router through setClient(), so open terminals keep their sessions and screens: a router built
+ * per client made every TerminalView subscribe again, which cleared the terminal.
  *
  * Always renders children so the app UI stays visible during reconnection.
- * The router may be null when API is unavailable; consumers must handle this.
  */
 export function TerminalRouterProvider(props: TerminalRouterProviderProps) {
-  const { api } = useAPI();
+  // Only the live client: a reconnecting state can keep the old one, and calls on it fail.
+  const api = useLiveAPI();
   const [router, setRouter] = useState<TerminalSessionRouter | null>(null);
+  const popout = props.popout === true;
 
+  // Create/cleanup after commit to avoid render-time disposal in concurrent mode.
   useEffect(() => {
-    if (!api) {
-      setRouter(null);
-      return;
-    }
-
-    // Create/cleanup after commit to avoid render-time disposal in concurrent mode.
-    const nextRouter = new TerminalSessionRouter(api, { popout: props.popout === true });
+    const nextRouter = new TerminalSessionRouter(null, { popout });
     setRouter(nextRouter);
     return () => {
       nextRouter.dispose();
     };
-  }, [api, props.popout]);
+  }, [popout]);
 
-  const routerForContext = api && router?.getApi() === api ? router : null;
+  useEffect(() => {
+    router?.setClient(api);
+  }, [router, api]);
 
-  // Always render children - the router may be null during reconnection.
-  // Consumers (useTerminalRouter) must handle the null case gracefully.
   return (
-    <TerminalRouterContext.Provider value={routerForContext}>
-      {props.children}
-    </TerminalRouterContext.Provider>
+    <TerminalRouterContext.Provider value={router}>{props.children}</TerminalRouterContext.Provider>
   );
 }
 
 /**
  * Hook to access the TerminalSessionRouter.
  *
- * Returns null when the API is disconnected (e.g., during reconnection).
- * Callers should handle the null case gracefully.
+ * Returns null only before the provider's first commit. Callers should handle the null case.
  *
  * @throws If used outside of TerminalRouterProvider
  */