diff --git a/src/web/public/app.js b/src/web/public/app.js index 27ad196f..4315e308 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -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. diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 0302807f..8944288f 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -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'; diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 59791f60..0e234b80 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -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; diff --git a/test/terminal-font-settle.test.ts b/test/terminal-font-settle.test.ts new file mode 100644 index 00000000..b92960fc --- /dev/null +++ b/test/terminal-font-settle.test.ts @@ -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 { + const dir = resolve(import.meta.dirname, '../src/web/public'); + const FakeCodemanApp = function () {} as unknown as { prototype: Record }; + 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; + 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 } = {}) { + 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); + }); +});