Merge pull request #436 from irisitymichaelgrundberg/fix/replay-output-that-arrived-after-the-capture

fix(terminal): keep the output a pane capture could not contain

test/capture-load-window.browser.test.ts

@@ -0,0 +1,180 @@
+/**
+ * @fileoverview Output arriving after a pane capture survives the buffer load.
+ *
+ * `batchTerminalWrite` queues live terminal events while a buffer load runs,
+ * and `_finishBufferLoad` discards that queue by default. That is right when
+ * the loaded buffer is the server's accumulated byte history, which is current
+ * up to the response. A tmux pane capture is current only up to CAPTURE time,
+ * so anything arriving between the capture and the end of the chunked write is
+ * queued and then dropped, with nothing scheduling a re-fetch.
+ *
+ * The queue now stamps each entry with its arrival time, and a capture load
+ * replays the tail that arrived after the response headers. These drive the
+ * real client in chromium: the event is injected from inside the response's
+ * own `json()` call, which is the one place guaranteed to land after the
+ * headers and before the chunked write.
+ *
+ * Port: 3256 (capture load window)
+ *
+ * Run: npx vitest run --config config/vitest.browser.config.ts test/capture-load-window.browser.test.ts
+ */
+
+import { describe, it, expect, beforeAll, afterAll } from 'vitest';
+import { chromium, type Browser, type BrowserContext, type Page } from 'playwright';
+import { WebServer } from '../src/web/server.js';
+
+const PORT = 3256;
+const BASE_URL = `http://localhost:${PORT}`;
+const MARKER = 'ARRIVED-AFTER-THE-CAPTURE';
+
+let server: WebServer;
+let browser: Browser;
+
+beforeAll(async () => {
+  server = new WebServer(PORT, false, true); // testMode
+  await server.start();
+  browser = await chromium.launch({ headless: true });
+}, 60_000);
+
+afterAll(async () => {
+  await browser?.close();
+  await server?.stop();
+}, 30_000);
+
+/**
+ * Select the session with the terminal fetch stubbed, injecting one live event
+ * from inside `json()`. Returns how many terminal rows carry the marker, so a
+ * flush that replays too much fails as loudly as one that replays nothing.
+ */
+async function runLoad(page: Page, sessionId: string, source: string): Promise<number> {
+  return page.evaluate(
+    async ({ sid, src, marker }) => {
+      const app = (
+        window as unknown as {
+          app: {
+            selectSession: (id: string, o?: object) => Promise<void>;
+            _onSessionTerminal: (e: { id: string; data: string }) => void;
+            terminal: {
+              buffer: {
+                active: {
+                  length: number;
+                  getLine: (i: number) => { translateToString: (t: boolean) => string } | undefined;
+                };
+              };
+            };
+          };
+        }
+      ).app;
+
+      const realFetch = window.fetch.bind(window);
+      window.fetch = ((input: RequestInfo | URL, init?: RequestInit) => {
+        const url = String(typeof input === 'string' ? input : ((input as Request).url ?? input));
+        if (!url.includes('/terminal')) return realFetch(input as RequestInfo, init);
+        return Promise.resolve({
+          ok: true,
+          status: 200,
+          // `selectSession` timestamps the headers the moment this promise
+          // resolves, then calls json(). Injecting here puts the event after
+          // that timestamp and inside the load window, which is exactly the
+          // gap a pane capture cannot cover.
+          json: async () => {
+            app._onSessionTerminal({ id: sid, data: `\r\n${marker}\r\n` });
+            return {
+              success: true,
+              data: {
+                terminalBuffer: '\x1b[1;1Hcaptured frame line one\r\n',
+                status: 'idle',
+                fullSize: 512,
+                retainedBytes: 512,
+                truncated: false,
+                truncationReason: null,
+                source: src,
+                captureCols: 80,
+                captureRows: 24,
+              },
+            };
+          },
+        }) as unknown as Promise<Response>;
+      }) as typeof window.fetch;
+
+      try {
+        await app.selectSession(sid);
+        await new Promise((r) => setTimeout(r, 1200));
+        const buf = app.terminal.buffer.active;
+        let hits = 0;
+        for (let i = 0; i < buf.length; i++) {
+          if (buf.getLine(i)?.translateToString(true).includes(marker)) hits += 1;
+        }
+        return hits;
+      } finally {
+        window.fetch = realFetch;
+      }
+    },
+    { sid: sessionId, src: source, marker: MARKER }
+  );
+}
+
+async function openSession(page: Page): Promise<string> {
+  await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' });
+  await page.waitForFunction(() => document.body.classList.contains('app-loaded'), { timeout: 10_000 });
+  // xterm loads from /vendor, so the terminal appears a beat after the app.
+  // Without it every buffer assertion below would throw rather than compare.
+  await page.waitForFunction(() => (window as unknown as { app?: { terminal?: unknown } }).app?.terminal, null, {
+    timeout: 30_000,
+  });
+  return page.evaluate(async () => {
+    const res = await fetch('/api/sessions', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ workingDir: '/tmp', name: 'capture-load-window-test' }),
+    });
+    const body = await res.json();
+    return body.data?.session?.id ?? body.data?.id ?? body.id;
+  });
+}
+
+describe('output emitted during a capture load', () => {
+  let context: BrowserContext;
+  let page: Page;
+
+  afterAll(async () => {
+    await context?.close();
+  });
+
+  it('reaches the terminal exactly once when the buffer came from a pane capture', async () => {
+    context = await browser.newContext({ viewport: { width: 1280, height: 800 } });
+    page = await context.newPage();
+    const sessionId = await openSession(page);
+    expect(sessionId).toBeTruthy();
+
+    // Exactly once. The cutoff exists so the flush cannot also replay events the
+    // payload already carried, which would double the output rather than heal it.
+    expect(await runLoad(page, sessionId, 'mux-visible')).toBe(1);
+
+    await page.evaluate(
+      (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined),
+      sessionId
+    );
+    await context.close();
+  }, 60_000);
+
+  it('stays dropped when the buffer came from the accumulated byte history', async () => {
+    // The byte history already contains everything up to the response, so
+    // replaying the queue on top of it would duplicate the output — most
+    // visibly Ink's cursor-up redraws. The discard has to survive this fix.
+    context = await browser.newContext({ viewport: { width: 1280, height: 800 } });
+    page = await context.newPage();
+    const sessionId = await openSession(page);
+    // Without this, a failed create passes the zero-hit assertion below
+    // vacuously — nothing was loaded, so nothing was replayed.
+    expect(sessionId).toBeTruthy();
+
+    expect(await runLoad(page, sessionId, 'history')).toBe(0);
+
+    await page.evaluate(
+      (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined),
+      sessionId
+    );
+    await context.close();
+  }, 60_000);
+});

test/terminal-buffer-flush.test.ts

@@ -56,10 +56,10 @@ type BufferLoadApp = {
   _bufferLoadSeq: number;
   _bufferLoadOwner: string | null;
   _isLoadingBuffer: boolean;
-  _loadBufferQueue: string[] | null;
+  _loadBufferQueue: { at: number; data: string }[] | null;
   batchTerminalWrite: (data: string) => void;
   _beginBufferLoad: (owner?: string) => string;
-  _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean }) => boolean;
+  _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean; since?: number }) => boolean;
 };
 
 /**
@@ -84,10 +84,57 @@ function makeApp() {
   return { app, writes };
 }
 
-/** Simulate live SSE events arriving while a buffer load is in progress (the queue path). */
-function pushWhileLoading(app: BufferLoadApp, data: string) {
-  // Mirrors batchTerminalWrite's queue branch: if loading, push to the queue.
-  if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push(data);
+/**
+ * A stub carrying the REAL `batchTerminalWrite` on top of the real begin/finish
+ * methods, so a replay samples the sticky-scroll baseline exactly as it does in
+ * the browser. The terminal is a fake whose `buffer.active` the test moves by
+ * hand, which is what a caller's `scrollToLine` does to a real one.
+ */
+function makeScrollApp() {
+  const buffer = { viewportY: 0, baseY: 100 };
+  const app = {
+    buffer,
+    terminal: { buffer: { active: buffer } },
+    sessions: new Map(),
+    activeSessionId: null,
+    pendingWrites: [] as string[],
+    writeFrameScheduled: false,
+    _wasAtBottomBeforeWrite: false,
+    _bufferLoadSeq: 0,
+    _bufferLoadOwner: null as string | null,
+    _isLoadingBuffer: false,
+    _loadBufferQueue: null as { at: number; data: string }[] | null,
+    _scheduleTerminalWriteFlush: vi.fn(),
+    batchTerminalWrite: mixin.batchTerminalWrite as (data: string) => void,
+    isTerminalAtBottom: mixin.isTerminalAtBottom as () => boolean,
+    _syncStickyScrollBaseline: mixin._syncStickyScrollBaseline as () => void,
+    _beginBufferLoad: mixin._beginBufferLoad as BufferLoadApp['_beginBufferLoad'],
+    _finishBufferLoad: mixin._finishBufferLoad as BufferLoadApp['_finishBufferLoad'],
+  };
+  return app;
+}
+
+/**
+ * Slice one class method out of app.js, from its header to the next method's.
+ *
+ * Bounding the slice matters: the two methods checked below are not followed by
+ * a JSDoc block, so a scan for the next comment would run on into unrelated
+ * code and match its scroll calls instead of theirs.
+ */
+function methodBody(source: string, method: string): string {
+  const start = source.search(new RegExp(`^ {2}(?:async )?${method}\\(`, 'm'));
+  expect(start, `${method} not found in app.js`).toBeGreaterThan(-1);
+  const next = /^ {2}(?:async )?[A-Za-z_$][\w$]*\(/m.exec(source.slice(start + 1));
+  return next ? source.slice(start, start + 1 + next.index) : source.slice(start);
+}
+
+/**
+ * Simulate a live SSE event arriving while a buffer load is in progress.
+ * Mirrors batchTerminalWrite's queue branch, which stamps each entry with its
+ * arrival time so a flush can replay only the tail (see the `since` tests).
+ */
+function pushWhileLoading(app: BufferLoadApp, data: string, at = performance.now()) {
+  if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push({ at, data });
 }
 
 describe('buffer-load flush (COD-144)', () => {
@@ -153,11 +200,165 @@ describe('buffer-load flush (COD-144)', () => {
     // State untouched — still loading, queue intact, nothing replayed.
     expect(app._isLoadingBuffer).toBe(true);
     expect(app._bufferLoadOwner).toBe('real-owner');
-    expect(app._loadBufferQueue).toEqual(['queued']);
+    expect(app._loadBufferQueue).toEqual([{ at: expect.any(Number), data: 'queued' }]);
     expect(app.batchTerminalWrite).not.toHaveBeenCalled();
     expect(writes).toEqual([]);
   });
 
+  // ── The tmux-capture tail: `since` ──
+  //
+  // A pane capture is a point-in-time frame taken part-way through the fetch, so
+  // it holds what arrived BEFORE the capture and nothing after. selectSession
+  // passes the response's arrival time as `since`, which splits the queue at
+  // exactly that line: pre-capture events are already painted and must stay
+  // dropped, post-capture events exist nowhere else and must be replayed.
+
+  it('flushes only the entries at or after `since`', () => {
+    const { app, writes } = makeApp();
+    const owner = app._beginBufferLoad('load-since');
+    pushWhileLoading(app, 'already-in-the-capture', 100);
+    pushWhileLoading(app, 'arrived-at-the-headers', 200);
+    pushWhileLoading(app, 'arrived-after-the-headers', 300);
+
+    app._finishBufferLoad(owner, { flushQueued: true, since: 200 });
+
+    // The pre-capture event stays dropped; the boundary entry counts as after.
+    expect(writes).toEqual(['arrived-at-the-headers', 'arrived-after-the-headers']);
+  });
+
+  it('flushQueued without `since` still replays the whole queue', () => {
+    // The COD-144 path: a brand-new session's first prompt predates the
+    // response, so cutting the queue would drop the only content it has.
+    const { app, writes } = makeApp();
+    const owner = app._beginBufferLoad('load-no-since');
+    pushWhileLoading(app, 'prompt', 10);
+    pushWhileLoading(app, 'more', 20);
+
+    app._finishBufferLoad(owner, { flushQueued: true });
+
+    expect(writes).toEqual(['prompt', 'more']);
+  });
+
+  it('a `since` past every entry flushes nothing', () => {
+    const { app, writes } = makeApp();
+    const owner = app._beginBufferLoad('load-since-late');
+    pushWhileLoading(app, 'old', 10);
+
+    app._finishBufferLoad(owner, { flushQueued: true, since: 999 });
+
+    expect(writes).toEqual([]);
+    expect(app.batchTerminalWrite).not.toHaveBeenCalled();
+  });
+
+  // ── Re-entering one load ──
+  //
+  // `selectSession` opens the load before its fetch, and `chunkedTerminalWrite`
+  // opens it again under the SAME owner when it starts writing. A reset on that
+  // second call would silently throw away everything queued during the fetch,
+  // which on the capture path is output no buffer holds.
+
+  it('re-entering the same load keeps what the queue already holds', () => {
+    const { app, writes } = makeApp();
+    const owner = app._beginBufferLoad('load-reenter');
+    pushWhileLoading(app, 'arrived-during-the-fetch', 100);
+
+    // chunkedTerminalWrite re-opens the load it was handed.
+    app._beginBufferLoad(owner);
+    pushWhileLoading(app, 'arrived-during-the-write', 200);
+
+    app._finishBufferLoad(owner, { flushQueued: true, since: 50 });
+
+    expect(writes).toEqual(['arrived-during-the-fetch', 'arrived-during-the-write']);
+  });
+
+  it('a genuinely different load still starts with an empty queue', () => {
+    const { app, writes } = makeApp();
+    app._beginBufferLoad('load-first');
+    pushWhileLoading(app, 'belongs-to-the-abandoned-load', 100);
+
+    // A tab switch starts a new load under a new owner. Its events are not ours.
+    const second = app._beginBufferLoad('load-second');
+    pushWhileLoading(app, 'belongs-to-this-load', 200);
+
+    app._finishBufferLoad(second, { flushQueued: true, since: 0 });
+
+    expect(writes).toEqual(['belongs-to-this-load']);
+  });
+
+  // ── The sticky-scroll baseline across a replay ──
+  //
+  // `batchTerminalWrite` samples `_wasAtBottomBeforeWrite` before queueing, and
+  // `flushPendingWrites` scrolls to the bottom off that sample. The replay runs
+  // inside `chunkedTerminalWrite` before its promise resolves, with the terminal
+  // freshly reset and rewritten, so the sample is always true. A caller that
+  // then restores the reader's position would have that restore undone.
+
+  it('the replay latches the baseline true, and the viewport restore re-takes it', () => {
+    const app = makeScrollApp();
+    const owner = app._beginBufferLoad('load-scroll');
+    pushWhileLoading(app as unknown as BufferLoadApp, 'output-after-the-capture', 100);
+
+    // The load ends with the terminal reset and rewritten, so it reads as bottom.
+    app.buffer.viewportY = app.buffer.baseY;
+    app._finishBufferLoad(owner, { flushQueued: true, since: 0 });
+    expect(app._wasAtBottomBeforeWrite).toBe(true);
+
+    // The caller now puts the reader back where they were reading.
+    app.buffer.viewportY = 40;
+    app._syncStickyScrollBaseline();
+
+    // The next flush must leave them there.
+    expect(app._wasAtBottomBeforeWrite).toBe(false);
+  });
+
+  it('a restore that lands back at the bottom keeps sticky scroll armed', () => {
+    const app = makeScrollApp();
+    const owner = app._beginBufferLoad('load-scroll-bottom');
+    pushWhileLoading(app as unknown as BufferLoadApp, 'output-after-the-capture', 100);
+
+    app.buffer.viewportY = app.buffer.baseY;
+    app._finishBufferLoad(owner, { flushQueued: true, since: 0 });
+    app._syncStickyScrollBaseline();
+
+    // A reader who was already at the bottom still wants to be carried along.
+    expect(app._wasAtBottomBeforeWrite).toBe(true);
+  });
+
+  it('both callers that restore a scroll position re-take the baseline', () => {
+    // The wiring lives in app.js, outside this file's vm harness. Without it the
+    // two methods below restore the viewport and the next flush undoes it.
+    const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
+
+    for (const method of ['_onSessionNeedsRefresh', '_maybeRefetchFullHistory']) {
+      const body = methodBody(source, method);
+      const restoreAt = body.lastIndexOf('scrollToLine(');
+      const syncAt = body.indexOf('this._syncStickyScrollBaseline()');
+      expect(restoreAt, `${method} no longer restores a scroll position`).toBeGreaterThan(-1);
+      expect(syncAt, `${method} never re-takes the baseline`).toBeGreaterThan(-1);
+      expect(syncAt, `${method} re-takes the baseline before its restore`).toBeGreaterThan(restoreAt);
+    }
+  });
+
+  it('every path that fetches a terminal buffer and writes it asks the shared helper', () => {
+    // Drift guard. The first version of this fix covered one of the four paths,
+    // and a later pass found it still covering one of four. Nothing else in the
+    // gate stops a fifth path, or an inlined `{ flushQueued: true }`, from
+    // splitting the policy up again; the browser suite that would notice does
+    // not run in CI.
+    const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
+
+    for (const method of [
+      'selectSession',
+      '_onSessionNeedsRefresh',
+      '_onSessionClearTerminal',
+      '_maybeRefetchFullHistory',
+    ]) {
+      expect(methodBody(source, method), `${method} decides the flush policy itself`).toContain(
+        'this._bufferLoadFinishOpts('
+      );
+    }
+  });
+
   it('empty queue + flushQueued is a no-op (no throw, no writes)', () => {
     const { app, writes } = makeApp();
     const owner = app._beginBufferLoad('load-empty');

test/terminal-flush-budget.test.ts

@@ -355,6 +355,51 @@ describe('terminal flush budget', () => {
     expect(app._bufferLoadOwner).toBe(null);
   });
 
+  // ── Which payloads end their load by replaying the queue ──
+  //
+  // A pane capture is current only up to capture time, so the tail that arrived
+  // after the response exists nowhere else and has to be replayed. The server's
+  // accumulated byte history is current up to the response, so replaying on top
+  // of it would duplicate output. `_bufferLoadFinishOpts` is the one place that
+  // decides this, for all four paths that fetch a terminal buffer and write it.
+
+  it('replays the tail for a visible-pane capture', () => {
+    const { CodemanApp } = loadAppHarness();
+    const app = Object.create(CodemanApp.prototype) as any;
+
+    expect(app._bufferLoadFinishOpts({ source: 'mux-visible' }, 1234)).toEqual({
+      flushQueued: true,
+      since: 1234,
+    });
+  });
+
+  it('replays the tail for a full-history capture', () => {
+    const { CodemanApp } = loadAppHarness();
+    const app = Object.create(CodemanApp.prototype) as any;
+
+    expect(app._bufferLoadFinishOpts({ source: 'mux-full-history' }, 1234)).toEqual({
+      flushQueued: true,
+      since: 1234,
+    });
+  });
+
+  it('discards the queue for the accumulated byte history', () => {
+    const { CodemanApp } = loadAppHarness();
+    const app = Object.create(CodemanApp.prototype) as any;
+
+    expect(app._bufferLoadFinishOpts({ source: 'history' }, 1234).flushQueued).toBe(false);
+  });
+
+  it('discards the queue for a payload that names no source', () => {
+    // Fails toward the safe answer: a duplicated Ink redraw corrupts the screen,
+    // while a dropped tail is repaired by the CLI's next full repaint.
+    const { CodemanApp } = loadAppHarness();
+    const app = Object.create(CodemanApp.prototype) as any;
+
+    expect(app._bufferLoadFinishOpts({}, 1234).flushQueued).toBe(false);
+    expect(app._bufferLoadFinishOpts(undefined, 1234).flushQueued).toBe(false);
+  });
+
   it('does not snap back to bottom during Codex Working redraws right after the user scrolls up', () => {
     const { app } = loadTerminalUiHarness('codex');
     const scrollToBottom = vi.fn();