mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
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>
101 lines
5.1 KiB
TypeScript
101 lines
5.1 KiB
TypeScript
// Port: none (static source contract — no browser, no server).
|
|
//
|
|
// The service worker's precache list used to be maintained by hand with the
|
|
// PRE-hash filenames, while scripts/build.mjs renamed those same files to
|
|
// content-hashed names and rewrote only index.html. So in production every
|
|
// precache entry pointed at a file that no longer existed, and
|
|
// `cache.add(url).catch(() => {})` in the install handler swallowed all of it.
|
|
// Measured against a running instance: 15 of 23 entries 404'd.
|
|
//
|
|
// Nothing caught it because nothing could: the two lists lived in different
|
|
// files, in different languages, with no shared symbol. The fix is to derive
|
|
// the list from the build's own manifest — and this test pins the contract that
|
|
// makes that derivation possible, because the failure mode is silent in both
|
|
// directions. A renamed anchor in sw.js means the build throws (loud, fine). A
|
|
// build that stops rewriting means the worker precaches nothing while still
|
|
// looking correct (silent, not fine).
|
|
import { readFileSync } from 'node:fs';
|
|
import { resolve } from 'node:path';
|
|
import { describe, expect, it } from 'vitest';
|
|
|
|
const root = resolve(import.meta.dirname, '..');
|
|
const sw = readFileSync(resolve(root, 'src/web/public/sw.js'), 'utf8');
|
|
const build = readFileSync(resolve(root, 'scripts/build.mjs'), 'utf8');
|
|
|
|
// The exact declarations scripts/build.mjs rewrites. They must appear EXACTLY
|
|
// once: the build asserts the same thing and throws otherwise, so a second
|
|
// occurrence (in a comment, say) fails the build rather than shipping stale.
|
|
const BUILD_ID_ANCHOR = "const BUILD_ID = 'dev';";
|
|
const HASHED_ASSETS_ANCHOR = 'const HASHED_ASSETS = [];';
|
|
|
|
describe('service worker precache contract', () => {
|
|
it('sw.js carries exactly one of each anchor the build rewrites', () => {
|
|
expect(sw.split(BUILD_ID_ANCHOR).length - 1).toBe(1);
|
|
expect(sw.split(HASHED_ASSETS_ANCHOR).length - 1).toBe(1);
|
|
});
|
|
|
|
it('build.mjs rewrites those exact anchors', () => {
|
|
expect(build).toContain(BUILD_ID_ANCHOR);
|
|
expect(build).toContain(HASHED_ASSETS_ANCHOR);
|
|
});
|
|
|
|
// The cache key must vary per build, or `activate`'s cleanup — which deletes
|
|
// every cache whose key is not the current one — never deletes anything, and
|
|
// hashed assets from every past release accumulate until the origin hits its
|
|
// storage quota. That is what the old constant 'codeman-v1' did.
|
|
it('derives the cache name from the build id rather than a constant', () => {
|
|
expect(sw).toContain('const CACHE_NAME = `codeman-${BUILD_ID}`;');
|
|
expect(sw).not.toMatch(/const CACHE_NAME = ['"]codeman-v\d+['"]/);
|
|
});
|
|
|
|
// The whole point of the rewrite: the shell is derived, not hand-listed.
|
|
it('builds the app shell from the hashed manifest', () => {
|
|
expect(sw).toContain("...HASHED_ASSETS.map((p) => '/' + p)");
|
|
});
|
|
|
|
// The regression itself: any pre-hash filename hand-listed in APP_SHELL will
|
|
// 404 in production, because the build renames it.
|
|
//
|
|
// The HASHABLE list is PARSED out of scripts/build.mjs rather than copied
|
|
// here. A hand-kept duplicate would be the same drift this whole PR exists to
|
|
// fix — it would go stale the first time someone adds an asset to the build,
|
|
// and then silently stop covering it.
|
|
it('never hand-lists a filename the build content-hashes', () => {
|
|
const block = build.slice(
|
|
build.indexOf('const HASHABLE = ['),
|
|
build.indexOf('];', build.indexOf('const HASHABLE = ['))
|
|
);
|
|
const hashedByBuild = [...block.matchAll(/'([^']+)'/g)].map((m) => m[1]);
|
|
// Guard the parse itself: an empty list would make this test vacuously pass.
|
|
expect(hashedByBuild.length, 'failed to parse HASHABLE out of scripts/build.mjs').toBeGreaterThan(10);
|
|
expect(hashedByBuild).toContain('app.js');
|
|
|
|
const shell = sw.slice(sw.indexOf('const APP_SHELL'), sw.indexOf('].map(B);'));
|
|
for (const name of hashedByBuild) {
|
|
expect(shell, `APP_SHELL must not hand-list ${name} — the build renames it`).not.toContain(`'/${name}'`);
|
|
}
|
|
});
|
|
|
|
// Dev serves sw.js unrewritten, so the literals must be valid on their own:
|
|
// an empty precache plus the unhashed modules cached on first use.
|
|
it('is valid unrewritten, for dev', () => {
|
|
expect(() => new Function(sw.replace(/self\./g, 'globalThis.'))).not.toThrow();
|
|
});
|
|
|
|
// Without ignoreSearch the whole precache is unreachable, which is subtle
|
|
// enough to be re-broken by anyone tidying this handler.
|
|
//
|
|
// `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=<mtime>` to
|
|
// EVERY same-origin `.js`/`.css` reference — content-hashed names included.
|
|
// Observed on a running instance: `src="app.556be563.js?v=1789423735875"`.
|
|
// `caches.match` is query-sensitive by default, so a precache keyed on
|
|
// `/app.556be563.js` can never serve that request, and the install would be
|
|
// downloading ~1.3MB per deploy that nothing can ever read back.
|
|
it('falls back to the cache ignoring the cache-busting query string', () => {
|
|
expect(sw).toContain('caches.match(request, { ignoreSearch: true })');
|
|
expect(sw, 'a bare caches.match(request) cannot match the ?v=<mtime> URLs cacheBustAssets emits').not.toMatch(
|
|
/caches\.match\(request\)\s*\)/
|
|
);
|
|
});
|
|
});
|