mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(terminal): replay a pane capture at the geometry it was taken at
A visible-frame capture repaints each row at an absolute position, counting up to the pane's height. A terminal shorter than that clamps every address past its own height onto its last line. The overflow rows then overwrite one another, and the rows underneath are lost. Replaying a real 50-row capture into a 30-row terminal rendered 28 lines of a 45-line command and drew the frame twice. Nothing in the response said what height the frame was built for, so the client could not detect this. A capture now reports the geometry it was really taken at through `capturedGeometry` on `PaneCaptureOptions`, and the terminal response carries it as `captureCols` and `captureRows`. When the captured pane is taller than the terminal, or the size that produced the capture did not survive the load, `selectSession` replays once at the size that stuck. `resizeRetry` caps that at one attempt, so two competing fits cannot trade replays forever. The retry re-arms the full-history flag only when the pass that ran had consumed it. A tab switch takes the bounded tail, so its retry takes the tail too: clearing the flag unconditionally would upgrade that switch into a fresh scrollback capture the user never asked for, which the route's own comments put at tens of megabytes. What this repairs is a capture that won a race against the resize meant to precede it. It does not repair a capture whose pane was too tall because `Session.resize` declined the resize outright, which it does for a small viewport while a desktop viewport's size claim is live. The retry re-sends the same declined resize and captures the same pane, and `resizeRetry` then stops it. Repairing that means changing who owns the pane size, which is a policy question this does not touch. The reported geometry still helps there, because the client can see the mismatch at all rather than being blind to it. Follows #395, #396 and #397, which fixed the other ways the replayed frame and the terminal could disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3cdb4bf42e
commit
3edf9aae2f
@@ -0,0 +1,202 @@
|
||||
/**
|
||||
* @fileoverview A capture drawn for a taller pane makes the client replay once.
|
||||
*
|
||||
* A visible-frame capture repaints each row at an absolute position, counting
|
||||
* up to the PANE's height. A terminal shorter than that clamps every address
|
||||
* past its own height onto its last line, so the overflow rows overwrite one
|
||||
* another and the rows underneath are lost. The client cannot see that from
|
||||
* the escape sequence, so the terminal response reports the geometry the
|
||||
* capture was taken at (`captureCols`/`captureRows`) and `selectSession`
|
||||
* replays once at the size that stuck.
|
||||
*
|
||||
* These drive the REAL client in chromium and stub only the terminal endpoint,
|
||||
* because the mismatch itself needs two viewports to stage against live tmux.
|
||||
* Without the fix the first assertion below sees one fetch instead of two.
|
||||
*
|
||||
* Port: 3252 (capture geometry retry)
|
||||
*
|
||||
* Run: npx vitest run --config config/vitest.browser.config.ts test/capture-geometry-retry.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 = 3252;
|
||||
const BASE_URL = `http://localhost:${PORT}`;
|
||||
|
||||
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);
|
||||
|
||||
/** A visible-frame capture: one absolutely-addressed paint per row. */
|
||||
function paneSnapshot(rows: number): string {
|
||||
const parts: string[] = [];
|
||||
for (let row = 1; row <= rows; row++) parts.push(`\x1b[${row};1Hprobe-row-${row}`);
|
||||
parts.push(`\x1b[${rows};6H`);
|
||||
return parts.join('');
|
||||
}
|
||||
|
||||
/**
|
||||
* Serve every terminal fetch from a stub reporting `captureRows`, counting the
|
||||
* fetches. The real route needs live tmux to produce a mismatched frame.
|
||||
*/
|
||||
async function stubTerminal(page: Page, captureRows: number, counter: { n: number; urls: string[] }) {
|
||||
await page.route('**/api/sessions/*/terminal*', async (route) => {
|
||||
counter.n += 1;
|
||||
counter.urls.push(route.request().url());
|
||||
await route.fulfill({
|
||||
status: 200,
|
||||
contentType: 'application/json',
|
||||
body: JSON.stringify({
|
||||
success: true,
|
||||
data: {
|
||||
terminalBuffer: paneSnapshot(captureRows),
|
||||
status: 'idle',
|
||||
fullSize: 1024,
|
||||
retainedBytes: 1024,
|
||||
truncated: false,
|
||||
truncationReason: null,
|
||||
source: 'mux-visible',
|
||||
captureCols: 200,
|
||||
captureRows,
|
||||
},
|
||||
}),
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
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 is loaded from /vendor, so the terminal appears a beat after the app.
|
||||
// Without it `app.terminal.rows` reads 0 and every height comparison below
|
||||
// would pass vacuously.
|
||||
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-geometry-test' }),
|
||||
});
|
||||
const body = await res.json();
|
||||
return body.data?.session?.id ?? body.data?.id ?? body.id;
|
||||
});
|
||||
}
|
||||
|
||||
/** The terminal is sized by the first select, so this only reads after one. */
|
||||
async function terminalRows(page: Page): Promise<number> {
|
||||
return page.evaluate(() => (window as unknown as { app: { terminal?: { rows: number } } }).app.terminal?.rows ?? 0);
|
||||
}
|
||||
|
||||
async function select(page: Page, sessionId: string, options: object = {}): Promise<void> {
|
||||
await page.evaluate(
|
||||
async ({ sid, opts }) => {
|
||||
const app = (window as unknown as { app: { selectSession: (id: string, o?: object) => Promise<void> } }).app;
|
||||
await app.selectSession(sid, opts);
|
||||
},
|
||||
{ sid: sessionId, opts: options }
|
||||
);
|
||||
await page.waitForTimeout(1500);
|
||||
}
|
||||
|
||||
async function closeSession(page: Page, sessionId: string): Promise<void> {
|
||||
await page.evaluate(
|
||||
(sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined),
|
||||
sessionId
|
||||
);
|
||||
}
|
||||
|
||||
describe('a capture taller than the terminal', () => {
|
||||
let context: BrowserContext;
|
||||
let page: Page;
|
||||
|
||||
afterAll(async () => {
|
||||
await context?.close();
|
||||
});
|
||||
|
||||
it('replays once when the captured pane is taller, and stops at one retry', async () => {
|
||||
context = await browser.newContext({ viewport: { width: 1280, height: 800 } });
|
||||
page = await context.newPage();
|
||||
const sessionId = await openSession(page);
|
||||
expect(sessionId).toBeTruthy();
|
||||
|
||||
// 200 rows is taller than any terminal this viewport can produce, so the
|
||||
// trigger is the captured height alone and not a size that moved.
|
||||
const fetches = { n: 0, urls: [] as string[] };
|
||||
await stubTerminal(page, 200, fetches);
|
||||
await select(page, sessionId);
|
||||
|
||||
// The terminal is sized by that select, so the premise is checkable now.
|
||||
expect(await terminalRows(page)).toBeLessThan(200);
|
||||
// One original load plus exactly one retry. `resizeRetry` caps it there:
|
||||
// the retry's own response reports the same mismatch, so an uncapped
|
||||
// implementation would loop.
|
||||
expect(fetches.n).toBe(2);
|
||||
|
||||
await closeSession(page, sessionId);
|
||||
await context.close();
|
||||
}, 60_000);
|
||||
|
||||
it('retries at the same scope the first pass used, not a wider one', async () => {
|
||||
// The retry re-arms the full-history flag only when the pass that ran had
|
||||
// consumed it. A tab switch takes the bounded tail, so its retry must take
|
||||
// the tail too; clearing the flag unconditionally would upgrade it into a
|
||||
// fresh multi-megabyte scrollback capture the user never asked for.
|
||||
context = await browser.newContext({ viewport: { width: 1280, height: 800 } });
|
||||
page = await context.newPage();
|
||||
const sessionId = await openSession(page);
|
||||
|
||||
const fetches = { n: 0, urls: [] as string[] };
|
||||
await stubTerminal(page, 200, fetches);
|
||||
|
||||
// First select: a fresh session, so this one legitimately pulls full history
|
||||
// and its retry may do the same.
|
||||
await select(page, sessionId);
|
||||
const afterFirst = fetches.n;
|
||||
expect(afterFirst).toBe(2);
|
||||
|
||||
// Re-select the SAME session. `selectSession` early-returns on an already
|
||||
// active session unless forceReload is set, and forceReload is the shape a
|
||||
// tab switch back to this session takes: `_fullHistoryLoaded` still holds
|
||||
// it, so neither this pass nor its retry should ask for full history again.
|
||||
await select(page, sessionId, { forceReload: true });
|
||||
const tabSwitchUrls = fetches.urls.slice(afterFirst);
|
||||
expect(tabSwitchUrls.length).toBe(2);
|
||||
expect(tabSwitchUrls.filter((u) => u.includes('full=1'))).toHaveLength(0);
|
||||
|
||||
await closeSession(page, sessionId);
|
||||
await context.close();
|
||||
}, 60_000);
|
||||
|
||||
it('does not replay when the captured pane fits the terminal', async () => {
|
||||
context = await browser.newContext({ viewport: { width: 1280, height: 800 } });
|
||||
page = await context.newPage();
|
||||
const sessionId = await openSession(page);
|
||||
|
||||
// Five rows is shorter than any terminal this viewport can produce, so the
|
||||
// frame fits, nothing is clamped, and nothing needs repeating. A retry here
|
||||
// would double the work of every tab switch.
|
||||
const fetches = { n: 0, urls: [] as string[] };
|
||||
await stubTerminal(page, 5, fetches);
|
||||
await select(page, sessionId);
|
||||
|
||||
expect(await terminalRows(page)).toBeGreaterThan(5);
|
||||
expect(fetches.n).toBe(1);
|
||||
|
||||
await closeSession(page, sessionId);
|
||||
await context.close();
|
||||
}, 60_000);
|
||||
});
|
||||
@@ -373,8 +373,19 @@ export class MockSession extends EventEmitter {
|
||||
/** Stub for sendInput */
|
||||
sendInput = vi.fn();
|
||||
|
||||
/**
|
||||
* The geometry the pane is drawing at, which the terminal route reports on
|
||||
* every response so a client can tell whether the frame fits its own
|
||||
* terminal. The stubbed `resize` records it the way the real one does.
|
||||
*/
|
||||
ptyCols = 120;
|
||||
ptyRows = 40;
|
||||
|
||||
/** Stub for resize */
|
||||
resize = vi.fn();
|
||||
resize = vi.fn((cols: number, rows: number) => {
|
||||
this.ptyCols = cols;
|
||||
this.ptyRows = rows;
|
||||
});
|
||||
|
||||
/** Stubs for the desktop sizing claims used by resize arbitration */
|
||||
claimDesktopSizing = vi.fn();
|
||||
|
||||
@@ -775,7 +775,60 @@ describe('session-routes', () => {
|
||||
body.data.terminalBuffer.indexOf('visible tmux pane only')
|
||||
);
|
||||
// No ?full=1 → visible-frame capture (no fullHistory opts).
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
});
|
||||
|
||||
// ── The geometry a capture was taken at ──
|
||||
//
|
||||
// A visible-frame capture repaints each row at an absolute position
|
||||
// (`\x1b[<row>;1H`). A terminal with fewer rows than the pane clamps every
|
||||
// address past its own height onto its last line, so the overflow rows
|
||||
// overwrite each other and the rows they land on are lost. The client can
|
||||
// only notice that if the response says what height the frame was built
|
||||
// for, which is what captureRows/captureCols carry.
|
||||
|
||||
it('reports the geometry the capture was really taken at', async () => {
|
||||
harness.ctx._session.terminalBuffer = '';
|
||||
(harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn(
|
||||
(_name: string, opts?: { capturedGeometry?: { cols: number; rows: number } }) => {
|
||||
// Stand in for TmuxManager, which fills this from the pane itself.
|
||||
if (opts) opts.capturedGeometry = { cols: 100, rows: 50 };
|
||||
return 'visible frame';
|
||||
}
|
||||
);
|
||||
|
||||
const res = await harness.app.inject({
|
||||
method: 'GET',
|
||||
url: `/api/sessions/${harness.ctx._sessionId}/terminal`,
|
||||
});
|
||||
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.data.source).toBe('mux-visible');
|
||||
expect(body.data.captureCols).toBe(100);
|
||||
expect(body.data.captureRows).toBe(50);
|
||||
});
|
||||
|
||||
it('falls back to the session geometry when the capture reports none', async () => {
|
||||
// The cursor query can fail, and a byte-history response never captures
|
||||
// at all. The session's own PTY size is the best answer available, and a
|
||||
// missing field would read as "no mismatch" and suppress the client's
|
||||
// repair.
|
||||
harness.ctx._session.terminalBuffer = 'byte history only';
|
||||
(harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn(() => null);
|
||||
harness.ctx._session.resize(111, 44, { force: true });
|
||||
|
||||
const res = await harness.app.inject({
|
||||
method: 'GET',
|
||||
url: `/api/sessions/${harness.ctx._sessionId}/terminal`,
|
||||
});
|
||||
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.data.source).toBe('history');
|
||||
expect(body.data.captureCols).toBe(111);
|
||||
expect(body.data.captureRows).toBe(44);
|
||||
});
|
||||
|
||||
// ── COD-47: full tmux scrollback replay on full page reload ──
|
||||
@@ -985,7 +1038,10 @@ describe('session-routes', () => {
|
||||
expect(res.statusCode).toBe(200);
|
||||
const body = JSON.parse(res.body);
|
||||
// Tail/tab-switch must NOT request fullHistory (undefined opts).
|
||||
expect(captureSpy).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(captureSpy).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
expect(body.data.terminalBuffer).toContain('visible frame only');
|
||||
expect(body.data.terminalBuffer).not.toContain('FULL_HISTORY_SHOULD_NOT_APPEAR');
|
||||
expect(body.data.source).toBe('mux-visible');
|
||||
@@ -1045,7 +1101,10 @@ describe('session-routes', () => {
|
||||
expect(body.data.terminalBuffer.indexOf('hello world')).toBeLessThan(
|
||||
body.data.terminalBuffer.indexOf('visible tmux pane only')
|
||||
);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
});
|
||||
|
||||
it('preserves one-time OAuth authorization URLs in Codex TUI replay history', async () => {
|
||||
@@ -1119,7 +1178,10 @@ describe('session-routes', () => {
|
||||
expect(body.data.terminalBuffer.indexOf('hello world')).toBeLessThan(
|
||||
body.data.terminalBuffer.indexOf('visible tmux pane only')
|
||||
);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
});
|
||||
|
||||
it('uses live mux pane capture only when the accumulated buffer is empty', async () => {
|
||||
@@ -1138,7 +1200,10 @@ describe('session-routes', () => {
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.data.terminalBuffer).toContain('visible restored tmux pane');
|
||||
expect(body.data.terminalBuffer).toContain('› current prompt');
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
});
|
||||
|
||||
it('returns error for unknown session', async () => {
|
||||
@@ -1166,7 +1231,10 @@ describe('session-routes', () => {
|
||||
expect(buf).toContain('\x1b[H\x1b[2J');
|
||||
expect(buf).toContain('LIVE-PANE-FRAME');
|
||||
expect(buf.indexOf('history-bytes')).toBeLessThan(buf.indexOf('LIVE-PANE-FRAME'));
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined);
|
||||
expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(
|
||||
harness.ctx._session.muxName,
|
||||
expect.not.objectContaining({ fullHistory: true })
|
||||
);
|
||||
});
|
||||
|
||||
it('falls back to the byte history when no live pane buffer is available', async () => {
|
||||
|
||||
@@ -11,7 +11,7 @@
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { resolve } from 'node:path';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { formatCursorRestore, hasVisibleContent } from '../src/tmux-manager.js';
|
||||
import { formatCursorRestore, formatPaneSnapshot, hasVisibleContent } from '../src/tmux-manager.js';
|
||||
|
||||
describe('tmux full-history pane capture (COD-47)', () => {
|
||||
const source = readFileSync(resolve(import.meta.dirname, '../src/tmux-manager.ts'), 'utf8');
|
||||
@@ -121,3 +121,53 @@ describe('hasVisibleContent', () => {
|
||||
expect(hasVisibleContent('\x1b[m \x1b[0m\n\x1b[m x \x1b[0m')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the geometry a capture reports back', () => {
|
||||
const source = readFileSync(resolve(import.meta.dirname, '../src/tmux-manager.ts'), 'utf8');
|
||||
const methodStart = source.indexOf('capturePaneBuffer(muxName: string');
|
||||
const methodEnd = source.indexOf('captureActivePaneBuffer(muxName: string', methodStart);
|
||||
const methodBody = source.slice(methodStart, methodEnd);
|
||||
|
||||
it('writes the pane size onto the caller options before either replay path returns', () => {
|
||||
// IS_TEST_MODE no-ops execSync, so assert from source (same approach as the
|
||||
// capture-flag tests above). The write must precede the fullHistory branch:
|
||||
// both paths return from inside it, and a caller that got no geometry
|
||||
// cannot tell a mismatched frame from a matching one.
|
||||
const write = methodBody.indexOf('opts.capturedGeometry = { cols: geometry.cols, rows: geometry.rows }');
|
||||
// Anchor on the REPLAY branch, not the earlier `if (fullHistory)` that only
|
||||
// sizes the exec buffer.
|
||||
const replayBranch = methodBody.indexOf('if (!geometry) return normalizeScrollbackEol(');
|
||||
const visibleReturn = methodBody.indexOf('if (geometry) return formatPaneSnapshot(');
|
||||
expect(write).toBeGreaterThan(-1);
|
||||
expect(replayBranch).toBeGreaterThan(-1);
|
||||
expect(visibleReturn).toBeGreaterThan(-1);
|
||||
expect(write).toBeLessThan(replayBranch);
|
||||
expect(write).toBeLessThan(visibleReturn);
|
||||
});
|
||||
|
||||
it('reports nothing when the cursor query gave no geometry', () => {
|
||||
// `queryPaneCursor` returns null on a failed or nonsensical query, and the
|
||||
// snapshot repaint is skipped in that case. Reporting a size anyway would
|
||||
// describe a frame that was never positioned.
|
||||
expect(methodBody).toContain('if (opts && geometry)');
|
||||
});
|
||||
});
|
||||
|
||||
describe('why a capture has to report its height', () => {
|
||||
it('a snapshot addresses rows the receiving terminal may not have', () => {
|
||||
// formatPaneSnapshot positions every row absolutely. A terminal shorter
|
||||
// than the pane clamps each address past its own height onto its last
|
||||
// line, so the overflow rows overwrite one another and the rows underneath
|
||||
// are lost. Nothing in the escape sequence tells the client this happened —
|
||||
// hence captureRows on the response.
|
||||
const lines = Array.from({ length: 50 }, (_, i) => `row-${i + 1}`);
|
||||
// cursorX 5 keeps the trailing cursor-restore move (`\x1b[50;6H`) out of the
|
||||
// `;1H` row-paint match below, so the count is row paints alone.
|
||||
const snapshot = formatPaneSnapshot(lines, { cols: 100, rows: 50, cursorX: 5, cursorY: 49 });
|
||||
const addressed = [...snapshot.matchAll(/\x1b\[(\d+);1H/g)].map((m) => Number(m[1]));
|
||||
|
||||
expect(Math.max(...addressed)).toBe(50);
|
||||
// A 30-row terminal cannot honour 20 of those addresses.
|
||||
expect(addressed.filter((row) => row > 30)).toHaveLength(20);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user