test(mobile): serve the xterm vendor bundles the browser suite needs - #241
Merged
Conversation
The mobile suite drives a real browser against a WebServer started from TypeScript source, so fastify-static serves join(__dirname, 'public') = src/web/public — not dist/web/public, where `npm run build` puts the vendor bundles. Every /vendor/xterm* request 404s, so `Terminal` is never defined, initTerminal() never runs, and any test touching app.terminal dies with "Cannot read properties of null". Measured in one worktree, toggling only the vendor files: before: 404s=5 Terminal=undefined app.terminal=null 8 failed | 26 passed after: 404s=0 Terminal=function app.terminal=live 6 failed | 28 passed The 6 remaining failures are genuine pre-existing bugs (stale layout and accessory-bar expectations, a CJK timeout) and are left alone here. This went unnoticed because config/vitest.ci.config.ts excludes test/mobile/**, so CI never ran the suite. `npm run test:mobile` now runs it, with a pretest hook that builds the bundles. The asset list was derived from the actual 404s rather than from build.mjs — which is how xterm-addon-unicode11 and xterm-zerolag-input got included; reading the build file alone would have missed both. Outputs go to the gitignored src/web/public/vendor/, so they stay build artifacts. The script is idempotent (skips outputs newer than their source) and does not touch the normal build. Full CI suite unchanged: 4368 passed.
The zerolag bundle exports only `XtermZerolagInput`, but app.js constructs `new LocalEchoOverlay(terminal)` directly. scripts/build.mjs appends global aliases after esbuild (build.mjs:53-66); the first version of this script omitted that step. Without them initTerminal() throws `LocalEchoOverlay is not defined` at the line that builds the overlay — and because that is midway through the function, EVERY later step silently never runs, including the mobile touch handlers on #terminalContainer. The page still had a terminal, so the failure looked like a tap-routing bug rather than a boot error. Verified: boot errors none, and all four terminalContainer touch listeners (touchstart/touchmove/touchend/touchcancel) now register.
Lint111
commented
Aug 9, 2026
ContributorAuthor
Pushed a follow-up commit after finding the fix was incomplete. The zerolag bundle exports only Without them Updated numbers, measured in one worktree toggling only the vendor files:
The 5 remaining failures are genuine pre-existing bugs (stale layout and accessory-bar expectations, a CJK timeout), untouched here. |
Uh oh!
There was an error while loading. Please reload this page.
Ark0N pushed a commit
that referenced
this pull request
Aug 9, 2026
…pin esbuild Follow-ups to #241 (thanks @Lint111), from an independent review of that PR. The script is a real fix for a real gap; these are the four defects the review found, each reproduced before and after. 1. A wrong-but-fresh output was never repaired. The zerolag bundle is finished by a SECOND step (the alias append), so anything landing between esbuild and the append is permanent: the file looks complete, carries a current mtime, and the mtime-only cache reports "up to date" forever while the suite dies on `LocalEchoOverlay is not defined`. Reproduced by replaying #241's own two commits: running the first and then pulling the second kept the broken bundle. Fixed twice over, because the two halves address different cases. Builds now go to a temp file and `renameSync` into place, so this script can never publish a half-written output (that also covers an interrupted esbuild or copy, and two concurrent runs). And `isFresh` verifies the bundle actually contains its alias tail, which is what repairs a file an EARLIER version already poisoned; a rename alone cannot fix what is already on disk. 2. Freshness compared against the entry file only, but esbuild bundles its four siblings too, so editing overlay-renderer.ts left the suite testing a stale overlay while reporting "up to date". Editing those siblings is exactly the single-source workflow CLAUDE.md mandates. It now stats every `.ts` in the package source dir. A full rebuild is ~2s, so the cache was not buying much. 3. `execFileSync('npx', ...)` passed no cwd, unlike scripts/build.mjs, so a run from another directory missed the repo's pinned esbuild and would fetch an unpinned one from the registry. Both calls now pass `cwd: ROOT`. 4. Every invocation in test/mobile/README.md was a bare `npx vitest`, which skips the `pretest:mobile` hook npm only fires for `npm run test:mobile`, so the documented commands all bypassed the fix. Rewritten, with a note on why. Also: an esbuild failure printed a raw stack; it now names the asset and its input, matching the missing-input message. And the header comment no longer implies the vendor dir is always empty: scripts/postinstall.js already writes these same seven outputs, so what this script adds is freshness and independence from install time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ark0N pushed a commit
that referenced
this pull request
Aug 9, 2026
…d temps Second review round on the #241 follow-ups. Three defects in my own previous commit, each reproduced before and after. 1. The temp path was shared between runs (`${dest}.tmp`), so two concurrent runs fought over it: 4 of 4 concurrent pairs had one run die. Worse than a crash, a sibling's cleanup landing between the esbuild and the alias append makes `appendFileSync` CREATE the file, so the rename publishes a bundle-less file containing only the alias tail, which still satisfies the content check and would be blessed by the cache forever. The name now carries the owning pid. 8 concurrent pairs afterwards: no failures, no strays, aliases intact. 2. The content check only covered the bundle, so a truncated xterm.min.js with a fresh mtime stayed truncated. This script can no longer produce one, but postinstall.js writes the same directory in place, so a Ctrl+C during `npm install` does, and a 200-byte xterm.min.js means `Terminal` is undefined and every mobile test dies on a null. A copy must now match its source byte for byte, and a derived output must clear a floor far below the real ratios (measured 0.97-1.00 minified, 0.51 for the bundle) while a truncation misses by orders of magnitude. Verified: 200-byte and 50-byte poisonings both repaired. 3. The try block ended before the append and rename, so a rename failure leaked its temp behind a raw stack. It now covers both and reports which asset failed. Per-pid names mean a killed run's temp is never reclaimed by a later rebuild, so startup sweeps temps whose owning process is gone, and only those: `kill(pid, 0)` throwing ESRCH. Deleting a live run's temp would recreate the collision fix 1 removes. Verified both directions, plus SIGKILL mid-build leaving no litter. The sweep swallows its own errors, because reclaiming litter must never fail the run: a directory named like a dead temp otherwise crashed the whole prepare step. Security-reviewed: no shell (execFileSync with an array, `shell` unset), every argument from the static asset table plus a numeric pid, all writes confined to the vendor dir under strace, `process.kill` only ever with signal 0 (and pid 0 skipped, since to kill(2) it means this process group), no new dependencies, no network, no eval, nothing published. The emitted browser bundle is byte-identical to the one scripts/build.mjs ships, tail included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
The mobile suite (
test/mobile/**) drives a real browser against aWebServerstarted from TypeScript source, so fastify-static serves
join(__dirname, 'public')=src/web/public— notdist/web/public, wherenpm run buildputs the vendor bundles.Every
/vendor/xterm*request 404s.Terminalis therefore never defined,initTerminal()never runs, and any test touchingapp.terminaldies withTypeError: Cannot read properties of null (reading 'reset' / 'write' / 'focus').This has been invisible because
config/vitest.ci.config.ts:22excludestest/mobile/**, so CI never runs the suite.Evidence
Measured in a single worktree, toggling only the presence of the vendor
files — nothing else changed:
typeof Terminalapp.terminalundefinednullfunctionThe missing assets were
xterm.min.js,xterm-addon-fit.min.js,xterm-addon-serialize.min.js,xterm-addon-unicode11.min.jsandxterm-zerolag-input.js.The fix
scripts/prepare-test-vendor.mjsbuilds those bundles intosrc/web/public/vendor/,mirroring the vendor steps in
scripts/build.mjs(same inputs, same output names,so the page markup needs no test-only branch). Wired as a
pretest:mobilehook, sonpm run test:mobilejust works.src/web/public/vendor/, so they stay buildartifacts and are never committed.
0 built, 7 up to dateon a second run).
One note on how the asset list was derived: I took it from the actual 404s, not
from reading
build.mjs. My first attempt read the build file and missedxterm-addon-unicode11andxterm-zerolag-input, which left the page just as broken —the terminal still never initialised.
Scope
Infrastructure only: 2 files, +107. No production code, no behaviour change.
The 6 remaining failures in
keyboard.test.tsare genuine pre-existing bugs — stalelayout and accessory-bar expectations and a CJK timeout — and are deliberately left
alone here rather than folded into an infrastructure change.
Since this is tooling rather than a behaviour fix, it has no "test that fails on master"
in the usual sense; the verifiable claim is the 404-count and test-count deltas above,
reproducible with
npm run test:mobilebefore and after.This unblocks the
document.activeElement === terminal.textareaassertion you asked foron #186 — that test cannot run at all until the terminal initialises.
🤖 Generated with Claude Code