mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 14:39:42 +02:00
Rounak DattaandClaude Opus 5
abd39318e6
Review fixes. Two of these are defects in the previous commit.
1. The fetch deadline only covered time-to-headers. `await fetch()` settles on
response headers, so clearing the abort timer in a finally around it left the
body — the multi-megabyte `?full=1` capture the deadline exists for —
completely unbounded; it only ever bounded a server that accepts a connection
and never replies. Measured against a server that sends headers immediately
and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, timer
cleared there, body completed at 4026ms unaborted. Now the body is read
inside `_fetchTerminalCapture`, which returns {json, headers, headersAt} —
headers because two callers read server-timing, headersAt because those same
callers measure header-vs-body time and can no longer observe that moment.
`_terminalCaptureInflight` is scoped the same way, so a body still streaming
counts toward a capture starting beside it. Same test now aborts at 1005ms.
2. The precache could never be hit, and the previous commit made that expensive
rather than free. `renderIndexHtml` runs `cacheBustAssets`, which appends
`?v=<mtime>` to every same-origin .js/.css reference INCLUDING content-hashed
names — confirmed against a running instance:
`vendor/xterm-zerolag-input.6fee72f2.js?v=1789402869101`. `caches.match` is
query-sensitive, so entries keyed on the bare hashed path were unreachable;
deriving the list from the manifest turned cheap 404s into ~1.3MB downloaded
at every install that nothing could read back, once per deploy now that
CACHE_NAME rotates. The fallback match takes `{ ignoreSearch: true }`, which
also lets runtime-cached entries survive an mtime change.
3. `_wsOutputGapSession` was only cleared in ws.onopen, so paths that already
repaint the buffer left it set and the socket replayed everything a second
time. `selectSession` loads the buffer and only THEN calls `_connectWs`, so
neither the _isLoadingBuffer nor the _terminalRefreshOwner guard applied.
`_markTerminalBufferReconciled()` is now called from _onSessionNeedsRefresh's
finally, from selectSession after its load, and from _cleanupSessionData.
The scope claim was also wrong and is corrected in the comment: when the
network drops, SSE drops with it and handleInit's keepTerminal branch already
reconciles. The genuinely uncovered case is the WS dying while SSE stays up,
where _onSSETerminal discards SSE terminal frames until _wsReady flips in
onclose — up to the ping+pong window of output nothing writes.
4. CLAUDE.md said "all of them measured rather than reasoned", which the PR's
own "not verified" section contradicted. Split explicitly: the replay race is
measured, the watchdog mechanism is verified against xterm 6.0.0 under jsdom
(field path resolves, a forced stale handle makes refreshRows a no-op, the
kick schedules a fresh frame), and the iOS rAF-discard premise is reasoned
and still wants a device. Adds the two missing entries — the WebSocket
reconcile and the sw.js/build.mjs "keep these in sync or the build throws"
contract.
Also: test/xterm-private-api.test.ts pins the RESOLVED lockfile version instead
of the declared `^6.0.0` range, which was the wrong assertion in both directions
— a real upgrade to 6.4.0 can rename a private field while resolving inside the
range, and an innocuous range edit failed while changing nothing installed. And
test/sw-precache-manifest.test.ts now parses HASHABLE out of scripts/build.mjs
rather than hand-copying it, which was the same drift this PR exists to fix; the
parse is guarded against silently matching nothing.
The deadline fix has a behavioural test against a real socket plus a source
guard asserting `await res.json()` precedes the finally — verified to fail when
the helper is reverted to the old shape, so it is not vacuous.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
76 lines
3.8 KiB
TypeScript
76 lines
3.8 KiB
TypeScript
// Port: none (dependency-range guard — no browser, no server).
|
|
//
|
|
// terminal-ui.js's `_kickRenderer` reaches into xterm internals to unwedge a
|
|
// frozen RenderDebouncer:
|
|
//
|
|
// terminal._core._renderService._renderDebouncer._animationFrame
|
|
// terminal._core._renderService.refreshRows(start, end)
|
|
//
|
|
// There is no public API for any of it — xterm exposes no way to ask "are you
|
|
// still producing frames" or "drop your stale animation handle" — and the bug
|
|
// it heals (iOS discarding a scheduled rAF, leaving that handle permanently set
|
|
// so every later refresh() early-returns) is otherwise unrecoverable without a
|
|
// page reload.
|
|
//
|
|
// That path CANNOT be asserted in this suite. `_renderService` is constructed
|
|
// by `Terminal.open()`, which needs a real DOM, and the CI gate runs in node —
|
|
// a headless Terminal reports `_renderService: undefined`, so a test here would
|
|
// pass whether or not the field still exists, which is worse than no test.
|
|
//
|
|
// So this guards the next best thing: the dependency range those field names
|
|
// were verified against. A major bump fails here, loudly, and sends someone to
|
|
// re-verify `_kickRenderer` by hand in a browser. The failure mode being
|
|
// defended against is silent — every access in `_kickRenderer` is
|
|
// optional-chained, so a renamed field degrades it to a permanent no-op with no
|
|
// error, no log, and a terminal that simply freezes again.
|
|
import { readFileSync } from 'node:fs';
|
|
import { resolve } from 'node:path';
|
|
import { describe, expect, it } from 'vitest';
|
|
|
|
const root = resolve(import.meta.dirname, '..');
|
|
const lock = JSON.parse(readFileSync(resolve(root, 'package-lock.json'), 'utf8')) as {
|
|
packages: Record<string, { version?: string }>;
|
|
};
|
|
const terminalUi = readFileSync(resolve(root, 'src/web/public/terminal-ui.js'), 'utf8');
|
|
|
|
// The exact version `_kickRenderer`'s field path was verified against.
|
|
//
|
|
// Read from the LOCKFILE, not package.json. The declared range is `^6.0.0`, so
|
|
// asserting on that string is the wrong test in both directions: a real upgrade
|
|
// to 6.4.0 — which can absolutely rename a private field — resolves inside the
|
|
// range and slips through, while an innocuous range edit that changes nothing
|
|
// about the installed code fails. The lockfile is what actually ships.
|
|
const VERIFIED_XTERM_VERSION = '6.0.0';
|
|
|
|
describe('xterm private-API dependency guard', () => {
|
|
it('pins the resolved xterm version _kickRenderer was verified against', () => {
|
|
expect(
|
|
lock.packages['node_modules/@xterm/xterm']?.version,
|
|
'xterm moved off the verified version — re-verify _kickRenderer in a real browser ' +
|
|
'(terminal-ui.js: _core._renderService._renderDebouncer._animationFrame), then update ' +
|
|
'VERIFIED_XTERM_VERSION here. The accessor is optional-chained, so a renamed field ' +
|
|
'degrades to a silent no-op and the freeze it heals comes back unnoticed.'
|
|
).toBe(VERIFIED_XTERM_VERSION);
|
|
});
|
|
|
|
// If someone deletes the watchdog, this guard is pointless noise — keep the
|
|
// two tied together so the range check cannot outlive what it protects.
|
|
it('is guarding a watchdog that still exists', () => {
|
|
expect(terminalUi).toContain('_kickRenderer()');
|
|
expect(terminalUi).toContain('_renderDebouncer');
|
|
expect(terminalUi).toContain('_animationFrame');
|
|
});
|
|
|
|
// Every private read must stay optional-chained. This is the property that
|
|
// makes reaching into internals acceptable at all: upstream can rename
|
|
// anything and the worst case is that healing stops, never that the terminal
|
|
// throws on a timer every two seconds.
|
|
it('reads every private field defensively', () => {
|
|
expect(terminalUi).toContain('this.terminal?._core?._renderService');
|
|
const body = terminalUi.slice(terminalUi.indexOf('_kickRenderer() {'));
|
|
const fn = body.slice(0, body.indexOf('\n },'));
|
|
expect(fn).toContain('try {');
|
|
expect(fn).toContain('catch');
|
|
});
|
|
});
|