mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
Issue #464, "text gets muffled sometimes, in both TUI default and fullscreen". The screenshot is not a dropped frame or a frozen renderer — it is arithmetic. Claude Code's TUI wraps its frame at the width the PTY reported and erases the previous frame by walking the cursor up the rows it believes that frame took. A browser terminal of a different width makes each logical line occupy more physical rows than Ink counted, so `eraseLines(n)` clears too few and the new frame paints over rows nothing erased: doubled lines, and short tool summaries sitting inside longer prose rows with the prose's tail still visible. Reproduced against this repo's own xterm before changing anything — a 120-column PTY against a 62-column terminal renders every wrapped line twice. `test/ terminal-pty-geometry.test.ts` pins that, and pins the clean render at matching widths beside it, so the assertion cannot be satisfied by code that fixes nothing. Four ways the two drifted apart, none of them observable from either end: 1. `fitAddon.fit()` resizes xterm to `proposeDimensions()` RAW while every server-facing path reported those floored at 40x10. Measured in Chrome at 430px: font size 44 proposed 13 columns, the server was told 40, and xterm stayed at 13. Three call sites each did their own fit-then-floor, and two re-read the proposal after the fit — `_shrinkPaddingToFit()` runs exactly there, so the container had moved. 2. `throttledResize` (keyboard up) and `sendResize` (session detached into its own window) reflowed locally and withheld only the SIGWINCH. That is the one combination that cannot be right: a reflow nothing is rendering for buys nothing and costs correctness. Both now withhold everything, and the keyboard's settle timer still sends the one resize that stops the PTY going stale. 3. `setFontSize`/`setFontFamily`/`setFontWeight` move the cell size — a geometry change — and told the server nothing at all, so raising the font on a phone left the CLI wrapping at the old column count. 4. `Session.resize` DECLINES a small-viewport request while a desktop connection holds an active sizing claim, and said nothing, because resize was write-only. `syncTerminalGeometry()` is now the one function that may change the terminal's size: it fits, floors and applies as a single step, so the numbers xterm holds are the numbers the server is told. A test sweeps every module for a bare `fit()` on the main terminal, and finds exactly one — the owner's own. For (4) the client cannot win, so it is told the truth instead: both transports answer a resize with `session.ptyCols`/`ptyRows` (`{"t":"zc"}` on the socket, the body of the resize POST) and `_onPtyGeometryReport` adopts them. A terminal that keeps a shape the PTY refused does not render "too narrow", it renders garbled. Adopting can leave the pane wider than the screen and the container is `overflow: hidden`, so `.pty-oversized` grants horizontal reach for exactly as long as the mismatch lasts: correct-and-reachable beats correct-and-clipped beats garbled. That rule sets both overflow axes and its own `touch-action` because mobile.css loads later and sets `.terminal-container { overflow: visible; touch-action: none }` — a bare `overflow-x` would leave overflow-y computing to `auto` and hand the browser a vertical scroll container the terminal's touch handler knows nothing about. Verified in Chrome at 430px against a live server, with a desktop client holding the claim: the phone adopts 198x43, gets `overflow-x: auto` / `overflow-y: hidden` / `touch-action: pan-x`, 758px of reach to the right, and keeps its own vertical scrolling. The pre-fix build was measured in the same harness for the control. Two things this deliberately does not do. It does not change who owns the pane size — the desktop still wins, and `_startMobileResizeRetry` still takes it back once that goes idle. And `throttledResize` still holds the PTY's shape for the whole keyboard animation rather than sending a SIGWINCH per step; that decision predates this and was not re-tested here. Also in this commit, Ark0N's third-pass review items on #431: - The response viewer's byte-buffer fallback and `_onSessionClearTerminal` both used the no-param `/terminal` form, capped only by `terminalBufferMaxBytes` (32MB) — the largest body the frontend asks for anywhere. One carried no deadline at all and the other got the 15s tail budget. Both now take the full-history budget. - A `?full=1` capture that outruns its deadline falls back to the bounded tail. The pane is blanked before that fetch, so an abort used to leave a black rectangle, discard the queued live output and never reach `_connectWs`. A failed load now still opens the socket, says one dim line where the content would have been, and clears the tab's spinner — which nothing did, so a failed select left `aria-busy="true"` set forever. - `_wsOutputGapSession` is cleared at the repaint that settles it, not in a `finally` that also ran on the catch. A reconcile that threw, or hit the new deadline — the flaky link the marker exists for — dropped the gap with nothing to retry it. `ws.onopen` no longer clears it up front either. - The replay-clear invariant is pinned in the gate, which is the drift this PR exists to fix: `_resetTerminalForReplay` must be a queued write and nothing else, and no module may blank the terminal with a `clear()+reset()` pair. - `DIAG_ENTRY_MAX_CHARS` replaces the hardcoded 300, bound through a local first: `CodemanDiag?.x` still throws a ReferenceError when the identifier was never declared, and that is the one function in the app that must not throw. - panels-ui's two kill-all clears route through the same helper, and the xterm-version guard's comment says "resolved lockfile version" rather than "dependency RANGE", which is what it has pinned since the last round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
291 lines
14 KiB
TypeScript
291 lines
14 KiB
TypeScript
// Port: none (pure helpers — no browser, no server).
|
||
//
|
||
// Three small decision functions behind the mobile terminal resilience work,
|
||
// pinned here because the code that consumes them lives in app.js /
|
||
// terminal-ui.js, which the CI gate cannot execute. Keeping the decision pure
|
||
// and the DOM half thin is what makes any of this testable without a browser.
|
||
//
|
||
// The renderer-liveness case is the one worth reading. iOS DISCARDS scheduled
|
||
// requestAnimationFrame callbacks when a PWA backgrounds — never delivered, not
|
||
// deferred — and xterm's RenderDebouncer only clears its `_animationFrame`
|
||
// handle from inside that callback. One drop leaves the handle permanently set,
|
||
// so every later refresh() returns immediately and the terminal freezes while
|
||
// its buffer keeps updating correctly. Codeman has exactly one xterm instance
|
||
// per page load, so a single backgrounding can wedge it until a reload.
|
||
import { readFileSync } from 'node:fs';
|
||
import { resolve } from 'node:path';
|
||
import vm from 'node:vm';
|
||
import { createServer, type ServerResponse } from 'node:http';
|
||
import type { AddressInfo } from 'node:net';
|
||
import { describe, expect, it } from 'vitest';
|
||
|
||
function loadConstants() {
|
||
const context = vm.createContext({ window: {}, globalThis: {} });
|
||
const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/constants.js'), 'utf8');
|
||
vm.runInContext(source, context, { filename: 'constants.js' });
|
||
const w = context.window as {
|
||
CodemanRenderLiveness: {
|
||
shouldKickRenderer: (s: {
|
||
wroteAt: number;
|
||
renderedAt: number;
|
||
now: number;
|
||
visible: boolean;
|
||
thresholdMs?: number;
|
||
}) => boolean;
|
||
RENDER_STALL_MS: number;
|
||
RENDER_LIVENESS_POLL_MS: number;
|
||
};
|
||
CodemanFetchDeadline: {
|
||
terminalFetchDeadlineMs: (s: { full?: boolean; inflight?: number }) => number;
|
||
FETCH_DEADLINE_TAIL_MS: number;
|
||
FETCH_DEADLINE_FULL_MS: number;
|
||
FETCH_DEADLINE_MAX_MS: number;
|
||
};
|
||
CodemanDiag: {
|
||
sanitizeDiagEntry: (msg: unknown) => string;
|
||
DIAG_ENTRY_MAX_CHARS: number;
|
||
};
|
||
};
|
||
return w;
|
||
}
|
||
|
||
describe('shouldKickRenderer', () => {
|
||
const { CodemanRenderLiveness } = loadConstants();
|
||
const { shouldKickRenderer, RENDER_STALL_MS } = CodemanRenderLiveness;
|
||
|
||
// The signature of the real failure: bytes were written, the element is
|
||
// visible, and no frame has been produced since.
|
||
it('kicks when a visible terminal owes a frame past the threshold', () => {
|
||
expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS, visible: true })).toBe(
|
||
true
|
||
);
|
||
});
|
||
|
||
it('does not kick before the threshold elapses', () => {
|
||
expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS - 1, visible: true })).toBe(
|
||
false
|
||
);
|
||
});
|
||
|
||
// A render at or after the last write means the pipeline is alive. This is
|
||
// the common case on every healthy terminal and must never kick.
|
||
it('does not kick when a render landed after the last write', () => {
|
||
expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1000, now: 99_999, visible: true })).toBe(false);
|
||
expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1200, now: 99_999, visible: true })).toBe(false);
|
||
});
|
||
|
||
// A hidden terminal legitimately stops rendering — xterm pauses it. Kicking
|
||
// there would fire on every backgrounded tab, forever.
|
||
it('never kicks a hidden terminal', () => {
|
||
expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 99_999, visible: false })).toBe(false);
|
||
});
|
||
|
||
// A quiet terminal is the normal state, not a stalled one. Gating on "no
|
||
// render recently" instead of "owes a frame" would kick every idle session.
|
||
it('never kicks a terminal that has never been written to', () => {
|
||
expect(shouldKickRenderer({ wroteAt: 0, renderedAt: 0, now: 99_999, visible: true })).toBe(false);
|
||
});
|
||
|
||
it('tolerates missing and malformed input rather than throwing', () => {
|
||
expect(shouldKickRenderer(undefined as never)).toBe(false);
|
||
expect(shouldKickRenderer({} as never)).toBe(false);
|
||
expect(shouldKickRenderer({ wroteAt: NaN, renderedAt: NaN, now: NaN, visible: true } as never)).toBe(false);
|
||
});
|
||
|
||
it('polls coarsely enough not to wake an idle phone every second', () => {
|
||
expect(CodemanRenderLiveness.RENDER_LIVENESS_POLL_MS).toBeGreaterThanOrEqual(1000);
|
||
});
|
||
});
|
||
|
||
describe('terminalFetchDeadlineMs', () => {
|
||
const { CodemanFetchDeadline } = loadConstants();
|
||
const { terminalFetchDeadlineMs, FETCH_DEADLINE_TAIL_MS, FETCH_DEADLINE_FULL_MS, FETCH_DEADLINE_MAX_MS } =
|
||
CodemanFetchDeadline;
|
||
|
||
// A full scrollback capture can be megabytes where a tail is one frame, so a
|
||
// single fixed timeout is wrong in both directions on a mobile link.
|
||
it('gives a full capture more budget than a tail', () => {
|
||
expect(terminalFetchDeadlineMs({ full: true })).toBeGreaterThan(terminalFetchDeadlineMs({ full: false }));
|
||
expect(terminalFetchDeadlineMs({ full: false })).toBe(FETCH_DEADLINE_TAIL_MS);
|
||
expect(terminalFetchDeadlineMs({ full: true })).toBe(FETCH_DEADLINE_FULL_MS);
|
||
});
|
||
|
||
// Eight tabs resuming must not all expire together because each assumed it
|
||
// had the link to itself.
|
||
it('scales with captures already in flight', () => {
|
||
const alone = terminalFetchDeadlineMs({ full: false, inflight: 0 });
|
||
const queued = terminalFetchDeadlineMs({ full: false, inflight: 3 });
|
||
expect(queued).toBeGreaterThan(alone);
|
||
});
|
||
|
||
it('is bounded — a stuck link still fails eventually', () => {
|
||
expect(terminalFetchDeadlineMs({ full: true, inflight: 1000 })).toBe(FETCH_DEADLINE_MAX_MS);
|
||
});
|
||
|
||
it('treats absent and nonsense input as a lone tail fetch', () => {
|
||
expect(terminalFetchDeadlineMs({})).toBe(FETCH_DEADLINE_TAIL_MS);
|
||
expect(terminalFetchDeadlineMs({ inflight: -5 } as never)).toBe(FETCH_DEADLINE_TAIL_MS);
|
||
expect(terminalFetchDeadlineMs({ inflight: NaN } as never)).toBe(FETCH_DEADLINE_TAIL_MS);
|
||
});
|
||
});
|
||
|
||
describe('sanitizeDiagEntry', () => {
|
||
const { CodemanDiag } = loadConstants();
|
||
const { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS } = CodemanDiag;
|
||
|
||
// The crash trail is joined with '\n' into one localStorage value and
|
||
// beaconed, and at least one call site interpolates a WebSocket close
|
||
// `reason`, which the server controls. A newline there forges entries.
|
||
it('collapses every newline form so an entry cannot forge another', () => {
|
||
expect(sanitizeDiagEntry('WS CLOSE reason=a\nFAKE ENTRY')).toBe('WS CLOSE reason=a FAKE ENTRY');
|
||
expect(sanitizeDiagEntry('a\r\nb')).toBe('a b');
|
||
expect(sanitizeDiagEntry('a
b
c')).toBe('a b c');
|
||
});
|
||
|
||
it('bounds length so one entry cannot exhaust the storage quota', () => {
|
||
const out = sanitizeDiagEntry('x'.repeat(DIAG_ENTRY_MAX_CHARS * 3));
|
||
expect(out).toHaveLength(DIAG_ENTRY_MAX_CHARS);
|
||
});
|
||
|
||
it('never throws on the values a diagnostic call site can actually pass', () => {
|
||
expect(sanitizeDiagEntry(null)).toBe('');
|
||
expect(sanitizeDiagEntry(undefined)).toBe('');
|
||
expect(sanitizeDiagEntry(42)).toBe('42');
|
||
expect(sanitizeDiagEntry({ toString: () => 'obj' })).toBe('obj');
|
||
});
|
||
});
|
||
|
||
// ── The deadline must cover the BODY, not just the handshake ────────────────
|
||
//
|
||
// `await fetch()` settles on response HEADERS. Clearing the abort timer there
|
||
// leaves the body — the multi-megabyte `?full=1` capture the deadline exists
|
||
// for — completely unbounded; it only ever covered a server that accepts a
|
||
// connection and never replies at all.
|
||
//
|
||
// Measured on the pre-fix shape against a server that sends headers immediately
|
||
// and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, the timer
|
||
// was cleared there, and the body completed at 4026ms unaborted.
|
||
//
|
||
// This exercises the real property with a real socket rather than asserting on
|
||
// source text, because the bug was a lifetime mistake that reads correctly.
|
||
describe('terminal capture deadline covers the response body', () => {
|
||
// Mirrors _fetchTerminalCapture's lifetime: one timer spanning headers AND
|
||
// body, cleared only once the body has been read.
|
||
async function captureUnderDeadline(url: string, deadlineMs: number) {
|
||
const controller = new AbortController();
|
||
const timer = setTimeout(() => controller.abort(), deadlineMs);
|
||
try {
|
||
const res = await fetch(url, { signal: controller.signal });
|
||
const headersAt = performance.now();
|
||
const json = await res.json();
|
||
return { json, headers: res.headers, headersAt };
|
||
} finally {
|
||
clearTimeout(timer);
|
||
}
|
||
}
|
||
|
||
async function serve(handler: (res: ServerResponse) => void) {
|
||
const srv = createServer((_req, res) => handler(res));
|
||
await new Promise<void>((r) => srv.listen(0, '127.0.0.1', r));
|
||
const { port } = srv.address() as AddressInfo;
|
||
return { url: `http://127.0.0.1:${port}/`, close: () => srv.close() };
|
||
}
|
||
|
||
it('aborts a stalled body instead of waiting on it forever', async () => {
|
||
let finish: NodeJS.Timeout | undefined;
|
||
const { url, close } = await serve((res) => {
|
||
res.writeHead(200, { 'Content-Type': 'application/json' });
|
||
res.write(' '); // headers out immediately, body never completes in time
|
||
finish = setTimeout(() => res.end('{"data":{}}'), 5000);
|
||
});
|
||
try {
|
||
await expect(captureUnderDeadline(url, 300)).rejects.toThrow(/abort/i);
|
||
} finally {
|
||
if (finish) clearTimeout(finish);
|
||
close();
|
||
}
|
||
});
|
||
|
||
// The two tests above exercise the PATTERN against a real socket, using a
|
||
// local mirror — so on their own they would still pass if the real helper
|
||
// regressed to clearing its timer at headers. This pins the real one.
|
||
it('_fetchTerminalCapture reads the body before releasing its deadline', () => {
|
||
const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
|
||
const start = app.indexOf('async _fetchTerminalCapture(');
|
||
expect(start, 'helper not found — renamed?').toBeGreaterThan(-1);
|
||
const body = app.slice(start, app.indexOf('\n }', start));
|
||
const jsonAt = body.indexOf('await res.json()');
|
||
const finallyAt = body.indexOf('} finally {');
|
||
expect(jsonAt, 'the body must be read inside the helper, not by callers').toBeGreaterThan(-1);
|
||
expect(finallyAt).toBeGreaterThan(-1);
|
||
expect(
|
||
jsonAt,
|
||
'await res.json() must run BEFORE the finally that clears the abort timer — ' +
|
||
'fetch() settles on headers, so a timer cleared there leaves the body unbounded'
|
||
).toBeLessThan(finallyAt);
|
||
// And the returned shape the five call sites destructure.
|
||
expect(body).toContain('return { json, headers: res.headers, headersAt };');
|
||
});
|
||
|
||
// Nothing in the gate pinned the invariant this PR exists to establish, which
|
||
// is the same drift it is fixing: a clear()+reset() pair reads as obviously
|
||
// equivalent to the queued RIS and is exactly what someone tidies back in.
|
||
it('_resetTerminalForReplay is a queued write and nothing else', () => {
|
||
const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
|
||
const start = app.indexOf('_resetTerminalForReplay() {');
|
||
expect(start, 'helper not found — renamed?').toBeGreaterThan(-1);
|
||
const body = app.slice(start, app.indexOf('\n }', start));
|
||
// RIS, queued through write() so it lands after any bytes already parsing.
|
||
expect(body).toContain("this.terminal.write('\\x1bc')");
|
||
expect(
|
||
body,
|
||
'reset()/clear() are SYNCHRONOUS and skip the write queue, so bytes queued ' +
|
||
'before them are parsed after and fuse into the snapshot written next'
|
||
).not.toMatch(/\.(reset|clear)\(\)/);
|
||
});
|
||
|
||
it('every replay path clears through that helper, never by hand', () => {
|
||
const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
|
||
// The three paths that blank the terminal before rewriting it from a capture.
|
||
for (const site of ['_onSessionNeedsRefresh(event = {}) {', 'async _onSessionClearTerminal(data) {']) {
|
||
const start = app.indexOf(site);
|
||
expect(start, `${site} not found — renamed?`).toBeGreaterThan(-1);
|
||
const body = app.slice(start, start + 4000);
|
||
expect(body, `${site} must clear via _resetTerminalForReplay`).toContain('this._resetTerminalForReplay()');
|
||
expect(body, `${site} hand-rolled a clear again`).not.toContain('this.terminal.clear()');
|
||
}
|
||
// And the PAIR appears nowhere in the frontend. A lone `clear()` before
|
||
// `showWelcome()` is fine — nothing is written after it, so there is nothing
|
||
// for stray bytes to fuse into. `clear()` immediately followed by `reset()`
|
||
// is the signature of someone blanking the terminal to rewrite it, which is
|
||
// precisely the case that has to be queued instead.
|
||
const pair = /\.clear\(\);\s*\n\s*this\.terminal\.reset\(\)/;
|
||
for (const rel of [
|
||
'src/web/public/app.js',
|
||
'src/web/public/panels-ui.js',
|
||
'src/web/public/terminal-ui.js',
|
||
'src/web/public/session-ui.js',
|
||
]) {
|
||
const src = readFileSync(resolve(import.meta.dirname, '..', rel), 'utf8');
|
||
expect(src, `${rel} blanks the terminal with clear()+reset() — use _resetTerminalForReplay()`).not.toMatch(pair);
|
||
}
|
||
});
|
||
|
||
it('returns the parsed envelope and headers on a healthy response', async () => {
|
||
const { url, close } = await serve((res) => {
|
||
res.writeHead(200, { 'Content-Type': 'application/json', 'server-timing': 'db;dur=12' });
|
||
res.end('{"data":{"terminalBuffer":"hello"}}');
|
||
});
|
||
try {
|
||
const out = await captureUnderDeadline(url, 5000);
|
||
// Callers read `capture.json?.data`, `capture.headers.get(...)` and
|
||
// `capture.headersAt` — all three must survive.
|
||
expect((out.json as { data: { terminalBuffer: string } }).data.terminalBuffer).toBe('hello');
|
||
expect(out.headers.get('server-timing')).toBe('db;dur=12');
|
||
expect(typeof out.headersAt).toBe('number');
|
||
} finally {
|
||
close();
|
||
}
|
||
});
|
||
});
|