fix(terminal): force the re-measure, bound the wait, and test both

Review of the previous commit found that waiting for the font does not, on its
own, do anything.

`FitAddon.proposeDimensions()` measures nothing — it divides the container by a
CACHED cell size, and xterm refreshes that cache only from `open()`, from a
resize that actually changed the grid, and on a device-pixel-ratio change.
Nothing in it listens for font loading. So a fit that runs after the font
arrives can still divide by the fallback cell, propose the grid it already has,
and short-circuit before anything re-measures. The wait now ends by calling
`_charSizeService.measure()` itself, which is the step that makes the following
fit see the real font. Private API, as FitAddon's own dependency on `_core` is,
and guarded because a terminal can be disposed mid-wait.

The wait was also unbounded, and it sat behind the buffer-load gate. Neither
`FontFaceSet.load()` nor `FontFaceSet.ready` has a deadline, so a font request
that never settled left the tab spinning with live output queued behind it —
permanently, and on every session, since they share one promise. The comment
claimed the opposite ("a font that never loads must not block the terminal, so
this always resolves"), which was true of the per-face loads and false of
`ready`. It is now raced against TERMINAL_FONT_WAIT_MS, and the await moved
ahead of `_beginBufferLoad` so a slow font cannot hold output back at all —
which also removes the stale-select interaction with `_restoringFlushedState`,
since that flag is not yet set when the wait runs.

The awaited set no longer includes faces that cannot move the measured cell.
The bundled symbols font is ~1.2MB of private-use-area glyphs and xterm
measures `W`, so awaiting it put a megabyte in front of the first frame for
nothing; the generic families match no FontFace at all.

A runtime font change had the same race the boot-time one did:
applyTerminalFontFamily wrote the new family and fit on the next line, against
a family the browser might not have loaded. It now re-arms the wait and fits
again when it settles.

The claim that this could not be tested was wrong: the repo's vm harness
reaches both halves. The new suite pins the family filter, the forced
re-measure, the deadline, a rejecting load, a browser with no font API, and a
terminal disposed mid-wait — plus the four ordering properties in
selectSession, including that iOS Safari's synchronous focus still precedes the
first await. Each assertion was checked by reverting its fix.

Also corrects the docstring's reason for calling `document.fonts.load` (the
stylesheet is render-blocking and long parsed by then; the real reason is that
the WebGL renderer rasterises through a canvas atlas, and canvas text never
triggers a CSS font fetch), restores the JSDoc block the previous commit
displaced from getTerminalDimensions, and fixes a comment that described the
first fit as already having run when the mobile-Safari branch defers it.
This commit is contained in:
Michael Grundberg
2026-09-09 16:57:42 +02:00
parent 0e82443222
commit 77fcd65b4a
4 changed files with 300 additions and 32 deletions
+17 -13
View File
@@ -5906,6 +5906,23 @@ class CodemanApp {
}
}
// Hold for the terminal font before measuring anything. A cell measured
// against a fallback font gives the wrong column and row count, and the
// correction would land after the replay, leaving the CLI drawing against a
// frame the terminal no longer shows. Resolves immediately once the font is
// in, so this costs a tab switch nothing after the first load, and it is
// bounded, so a font that never arrives cannot strand the session.
// ⚠️ BEFORE `_beginBufferLoad` on purpose: inside it, every live SSE event
// for this session queues instead of painting, so a slow font would hold
// output back rather than merely mis-measuring the grid.
if (this._terminalFontReady) {
await this._terminalFontReady;
if (this._isStaleSelect(selectGen)) {
this._clearTerminalLoadState(sessionId, selectGen);
return;
}
}
// Load terminal buffer for this session
// Show cached content instantly while fetching fresh data in background.
// Use tail mode for faster initial load (128KB is enough for recent visible content).
@@ -5926,19 +5943,6 @@ class CodemanApp {
let bufferWasEmpty = false;
let cacheResetAndParseMs = 0;
try {
// Hold for the terminal font before measuring anything. A cell measured
// against a fallback font gives the wrong column and row count, and the
// correction lands after the replay, leaving the CLI drawing against a
// frame the terminal no longer shows. Resolves immediately once the font
// is in, so this costs a tab switch nothing after the first load.
if (this._terminalFontReady) {
await this._terminalFontReady;
if (this._isStaleSelect(selectGen)) {
this._clearTerminalLoadState(sessionId, selectGen);
return;
}
}
// Fit terminal to container BEFORE writing any buffer data.
// If the browser was resized while viewing another session, the terminal
// canvas may be at stale dimensions — content would render at wrong width.
+16
View File
@@ -659,6 +659,22 @@ function sortSessionsByActivity(rows) {
// prompt icons (powerline segments, folder/git glyphs from p10k, starship,
// oh-my-posh) render even though the text fonts carry no private-use-area
// symbols — while all readable text keeps coming from the text fonts.
/**
* How long a terminal fit will wait for the terminal font, in ms.
*
* `FontFaceSet.ready` has no deadline of its own and the wait sits in front of
* the buffer replay, so a font request that never settles would leave the
* session unpainted. Past this we measure whatever is painted.
*/
const TERMINAL_FONT_WAIT_MS = 2000;
/**
* Families in the stack that cannot move the measured cell, so nothing waits on
* them: the generics match no `FontFace`, and the bundled symbols face carries
* private-use-area glyphs only (xterm measures `W`) while weighing ~1.2MB.
*/
const TERMINAL_FONT_UNMEASURED = new Set(['monospace', 'serif', 'sans-serif', 'system-ui', 'symbols nerd font mono']);
const TERMINAL_FONT_DEFAULT_STACK =
'"Fira Code", "Cascadia Code", "JetBrains Mono", "SF Mono", Monaco, "Symbols Nerd Font Mono", monospace';
+57 -19
View File
@@ -514,9 +514,10 @@ Object.assign(CodemanApp.prototype, {
} else {
this.fitAddon.fit();
}
// That first fit measures whatever font the browser has painted with so far,
// which is not necessarily the terminal font. Start the wait now so the
// buffer load can hold for it.
// Whenever that first fit runs — on this line, or a frame or two later on
// the mobile-Safari branch above — it measures whatever font the browser has
// painted with so far, which is not necessarily the terminal font. Start the
// wait now so the buffer load can hold for it.
this._terminalFontReady = this._awaitTerminalFont();
// Register link provider for clickable file paths in Bash tool output
@@ -4789,6 +4790,15 @@ Object.assign(CodemanApp.prototype, {
const resolved = window.CodemanTerminalFont.resolve(custom);
if (!this.terminal || this.terminal.options.fontFamily === resolved) return;
this.terminal.options.fontFamily = resolved;
// Changing the family at runtime is the same race as the boot-time one: the
// option write makes xterm re-measure immediately, against a family the
// browser may not have loaded. Re-arm the wait for the new stack and fit
// again once it settles, so the setting takes effect at the right size
// without needing a tab switch. The fit below still runs, so the terminal
// is never left unfitted if the wait is slow.
this._terminalFontReady = this._awaitTerminalFont().then(() => {
if (this.terminal?.options?.fontFamily === resolved) this.fitAddon?.fit();
});
this.fitAddon?.fit();
this._localEchoOverlay?.refreshFont();
this._predictiveEcho?.refreshFont();
@@ -4806,12 +4816,7 @@ Object.assign(CodemanApp.prototype, {
},
/**
* Get terminal dimensions with minimum enforcement.
* Prevents extremely narrow terminals that cause vertical text wrapping.
* @returns {{cols: number, rows: number}|null}
*/
/**
* Resolve once the terminal's own font is loaded and measurable.
* Wait for the terminal's own font, then make xterm re-measure against it.
*
* A character cell measured against a fallback font has a different width and
* height from one measured against the terminal font, so a fit taken too early
@@ -4819,12 +4824,24 @@ Object.assign(CodemanApp.prototype, {
* the buffer has been replayed, and the CLI redraws a frame that no longer
* matches what the terminal is showing.
*
* `document.fonts.ready` is not enough on its own: it can resolve before the
* stylesheet declaring @font-face has even been parsed. `document.fonts.load`
* for each family in the stack is what actually requests the faces, so this
* asks for those first and only then waits for the document to go quiet.
* Every step is best-effort — a font that never loads must not block the
* terminal, so this always resolves.
* ⚠️ Waiting is not sufficient on its own, which is what the re-measure at the
* end is for. `FitAddon.proposeDimensions()` divides the container by a CACHED
* cell size, and xterm refreshes that cache only from `open()`, from a resize
* that actually changed the grid, and on a device-pixel-ratio change — nothing
* in it listens for font loading. So a fit that runs after the font arrives can
* still divide by the fallback cell, propose the grid it already has, and
* short-circuit before anything re-measures.
*
* `document.fonts.load` for each family is what actually REQUESTS the faces:
* the WebGL renderer rasterises glyphs through a canvas texture atlas, and
* canvas text never triggers a CSS font fetch, so `document.fonts.ready` can
* resolve with a face never having been asked for at all.
*
* Every step is best-effort and the whole thing is bounded, because a font
* request that never settles must not hold up the terminal: `FontFaceSet.ready`
* has no deadline of its own, and the caller awaits this in front of the buffer
* replay. Past the deadline we fit against whatever is painted, which is the
* old behaviour rather than a new failure.
*/
async _awaitTerminalFont() {
try {
@@ -4832,15 +4849,36 @@ Object.assign(CodemanApp.prototype, {
const size = this.terminal?.options?.fontSize || 14;
const families = String(this.terminal?.options?.fontFamily || '')
.split(',')
.map((family) => family.trim())
.filter(Boolean);
await Promise.all(families.map((family) => document.fonts.load(`${size}px ${family}`).catch(() => {})));
await document.fonts.ready;
.map((family) => family.trim().replace(/^["']|["']$/g, ''))
.filter(Boolean)
// Only the faces that can supply the measured glyph are worth waiting on.
// The bundled symbols font is ~1.2MB and carries private-use-area glyphs
// only — xterm measures `W`, which it does not contain — so awaiting it
// puts a megabyte between the user and their first frame for nothing.
// Generic families match no FontFace at all.
.filter((family) => !TERMINAL_FONT_UNMEASURED.has(family.toLowerCase()));
const loaded = Promise.all(
families.map((family) => document.fonts.load(`${size}px "${family}"`).catch(() => {}))
).then(() => document.fonts.ready);
await Promise.race([loaded, new Promise((resolve) => setTimeout(resolve, TERMINAL_FONT_WAIT_MS))]);
} catch {
/* font loading is unavailable or failed — fit against whatever is painted */
}
// Force the cache refresh xterm will not do for us. Without this the wait
// buys nothing on the common path (see the warning above). Private API, as
// FitAddon itself is; guarded because a terminal can be disposed mid-wait.
try {
this.terminal?._core?._charSizeService?.measure();
} catch {
/* renderer not ready or internals moved — the next real resize re-measures */
}
},
/**
* Get terminal dimensions with minimum enforcement.
* Prevents extremely narrow terminals that cause vertical text wrapping.
* @returns {{cols: number, rows: number}|null}
*/
getTerminalDimensions() {
const MIN_COLS = 40;
const MIN_ROWS = 10;
+210
View File
@@ -0,0 +1,210 @@
/**
* @fileoverview A terminal is measured only once its own font can be measured.
*
* The first fit runs while the browser is still painting with a fallback font,
* whose character cell is a different size from the terminal font's. The grid
* that fit produces is therefore wrong, the pane is sized to it, and the
* correction arrives after the session's buffer has been replayed — so the CLI
* repaints for a shape that does not match what is on screen.
*
* Two properties carry the fix and both are pinned here:
*
* - The wait REQUESTS each measurable face and then forces xterm to re-measure.
* Waiting alone buys nothing: `FitAddon.proposeDimensions()` divides by a
* cached cell size that xterm refreshes only from `open()`, from a resize
* that changed the grid, and on a device-pixel-ratio change. Nothing in it
* listens for font loading, so a fit after the font arrives can still divide
* by the fallback cell and short-circuit.
* - The wait is BOUNDED. `FontFaceSet.ready` has no deadline, and `selectSession`
* awaits this before painting, so an unbounded wait would strand the session
* instead of merely mis-measuring it.
*
* Loaded via `vm` with a stubbed context (no jsdom — jsdom is broken on this
* box; see connection-indicator.test.ts), the same way terminal-buffer-flush
* extracts the real mixin methods from terminal-ui.js.
*/
import { readFileSync } from 'node:fs';
import { performance } from 'node:perf_hooks';
import { resolve } from 'node:path';
import vm from 'node:vm';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
/** The mixin runs inside the vm context, so its `document` must live there. */
let currentDocument: unknown;
function loadTerminalMixin(): Record<string, unknown> {
const dir = resolve(import.meta.dirname, '../src/web/public');
const FakeCodemanApp = function () {} as unknown as { prototype: Record<string, unknown> };
const context = vm.createContext({
console,
performance,
setTimeout,
clearTimeout,
setInterval: vi.fn(),
clearInterval: vi.fn(),
requestAnimationFrame: vi.fn(),
CodemanApp: FakeCodemanApp,
window: { addEventListener: vi.fn(), removeEventListener: vi.fn() },
get document() {
return currentDocument;
},
});
// constants.js supplies TERMINAL_FONT_WAIT_MS and TERMINAL_FONT_UNMEASURED.
const constants = readFileSync(resolve(dir, 'constants.js'), 'utf8');
const source = readFileSync(resolve(dir, 'terminal-ui.js'), 'utf8');
vm.runInContext(`${constants}\n${source}`, context);
return FakeCodemanApp.prototype;
}
const mixin = loadTerminalMixin();
type FontApp = {
_awaitTerminalFont: () => Promise<void>;
terminal: unknown;
};
function makeApp(fontFamily: string, opts: { measure?: () => void } = {}) {
const measure = vi.fn(opts.measure);
const app = {
_awaitTerminalFont: mixin._awaitTerminalFont,
terminal: {
options: { fontFamily, fontSize: 14 },
_core: { _charSizeService: { measure } },
},
} as unknown as FontApp & { terminal: { _core: { _charSizeService: { measure: typeof measure } } } };
return { app, measure };
}
/** A FontFaceSet stub recording what was asked for. */
function fontsStub(overrides: { load?: unknown; ready?: Promise<unknown> } = {}) {
const requested: string[] = [];
return {
requested,
fonts: {
load: overrides.load ?? ((spec: string) => (requested.push(spec), Promise.resolve([]))),
ready: overrides.ready ?? Promise.resolve(),
status: 'loaded',
},
};
}
beforeEach(() => {
vi.useRealTimers();
});
afterEach(() => {
currentDocument = undefined;
vi.useRealTimers();
});
describe('terminal font settle', () => {
it('requests every measurable family in the stack, unquoted', async () => {
const stub = fontsStub();
currentDocument = stub;
const { app } = makeApp('"Fira Code", "JetBrains Mono", monospace');
await app._awaitTerminalFont();
expect(stub.requested).toEqual(['14px "Fira Code"', '14px "JetBrains Mono"']);
});
it('does not wait on faces that cannot move the measured cell', async () => {
// The symbols face is ~1.2MB of private-use-area glyphs and xterm measures
// `W`, so awaiting it puts a megabyte in front of the first frame for
// nothing. The generics match no FontFace at all.
const stub = fontsStub();
currentDocument = stub;
const { app } = makeApp('"JetBrains Mono", "Symbols Nerd Font Mono", monospace, serif, system-ui');
await app._awaitTerminalFont();
expect(stub.requested).toEqual(['14px "JetBrains Mono"']);
});
it('forces xterm to re-measure, because loading a font does not', async () => {
// The property the whole change rests on. Without this the fit that follows
// still divides the container by the fallback cell.
currentDocument = fontsStub();
const { app, measure } = makeApp('"JetBrains Mono"');
await app._awaitTerminalFont();
expect(measure).toHaveBeenCalledTimes(1);
});
it('gives up on a font that never arrives, and still re-measures', async () => {
// FontFaceSet.ready has no deadline of its own, and selectSession awaits
// this before painting: unbounded here means a session that never renders.
currentDocument = fontsStub({ ready: new Promise(() => {}) });
const { app, measure } = makeApp('"JetBrains Mono"');
const started = Date.now();
await app._awaitTerminalFont();
expect(measure).toHaveBeenCalledTimes(1);
// Bounded by TERMINAL_FONT_WAIT_MS (2s), not left pending.
expect(Date.now() - started).toBeLessThan(4000);
}, 10_000);
it('survives a rejecting load and a browser with no font API', async () => {
currentDocument = fontsStub({ load: () => Promise.reject(new Error('network')) });
const { app: rejecting, measure: m1 } = makeApp('"JetBrains Mono"');
await expect(rejecting._awaitTerminalFont()).resolves.toBeUndefined();
expect(m1).toHaveBeenCalledTimes(1);
currentDocument = {};
const { app: noApi, measure: m2 } = makeApp('"JetBrains Mono"');
await expect(noApi._awaitTerminalFont()).resolves.toBeUndefined();
// No font API means nothing to wait for and nothing to re-measure against.
expect(m2).not.toHaveBeenCalled();
});
it('does not throw when the terminal was disposed mid-wait', async () => {
currentDocument = fontsStub();
const app = { _awaitTerminalFont: mixin._awaitTerminalFont, terminal: null } as unknown as FontApp;
await expect(app._awaitTerminalFont()).resolves.toBeUndefined();
});
});
describe('selectSession font gate', () => {
const appSource = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8');
const selectStart = appSource.indexOf('async selectSession(sessionId, options = {})');
const body = appSource.slice(
selectStart,
appSource.indexOf('\n // Shared cleanup for all session data', selectStart)
);
it('waits for the font before the first fit', () => {
const wait = body.indexOf('await this._terminalFontReady');
const fit = body.indexOf('if (this.fitAddon) this.fitAddon.fit();');
expect(wait).toBeGreaterThan(-1);
expect(fit).toBeGreaterThan(-1);
expect(wait).toBeLessThan(fit);
});
it('waits BEFORE opening the buffer-load gate', () => {
// Inside the gate every live SSE event queues instead of painting, so a slow
// font would hold output back rather than only mis-measuring the grid.
const wait = body.indexOf('await this._terminalFontReady');
const gate = body.indexOf('this._beginBufferLoad(selectGen)');
expect(gate).toBeGreaterThan(-1);
expect(wait).toBeLessThan(gate);
});
it('keeps the synchronous focus ahead of the wait (iOS Safari)', () => {
// iOS honours programmatic focus only inside the user-gesture call stack,
// which the first await ends.
const focus = body.indexOf('if (shouldFocusTerminal && this.terminal) this.terminal.focus();');
const wait = body.indexOf('await this._terminalFontReady');
expect(focus).toBeGreaterThan(-1);
expect(focus).toBeLessThan(wait);
});
it('re-checks for a newer selection after the wait', () => {
const wait = body.indexOf('await this._terminalFontReady');
const guard = body.indexOf('this._isStaleSelect(selectGen)', wait);
expect(guard).toBeGreaterThan(-1);
expect(guard - wait).toBeLessThan(200);
});
});