Merge pull request #396 from irisitymichaelgrundberg/fix/terminal-font-settle-before-fit

fix(terminal): fit the terminal only once the terminal font is measurable

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).

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';
 

src/web/public/terminal-ui.js

@@ -514,6 +514,11 @@ Object.assign(CodemanApp.prototype, {
     } else {
       this.fitAddon.fit();
     }
+    // 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
     this.registerFilePathLinkProvider();
@@ -4785,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();
@@ -4801,6 +4815,65 @@ Object.assign(CodemanApp.prototype, {
     }
   },
 
+  /**
+   * 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
+   * produces the wrong column and row count. The correction then arrives after
+   * the buffer has been replayed, and the CLI redraws a frame that no longer
+   * matches what the terminal is showing.
+   *
+   * ⚠️ 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 {
+      if (typeof document === 'undefined' || !document.fonts?.load) return;
+      const size = this.terminal?.options?.fontSize || 14;
+      const families = String(this.terminal?.options?.fontFamily || '')
+        .split(',')
+        .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.

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<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);
+  });
+});