From ca85eb5e617e7f6f38e5193d6d5696c362170fb8 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Mon, 20 Apr 2026 04:33:03 +0530 Subject: [PATCH 1/3] fix(web): restore terminal cell metrics after xterm v6 upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v5 -> v6 xterm.js upgrade regressed the in-browser terminal: each cell rendered visibly wider and shorter than the glyph inside it, making horizontal spacing too wide and vertical spacing too tight. Root causes: 1. `fontFamily` contained `var(--font-jetbrains-mono)`. xterm's char measurement ultimately hits canvas `ctx.font`, which cannot resolve CSS custom properties. The var token poisoned the font string so measurement fell back to a default font while DOM rows still rendered in JetBrains Mono — cell width vs glyph width drifted apart. 2. xterm v6 defaults `lineHeight` to 1.0. Combined with JetBrains Mono's tall x-height, rows visually collided. 3. `document.fonts.ready` can resolve before next/font's `font-display: swap` actually paints JetBrains Mono, so the initial `fit()` measures against the fallback font. xterm does not re-measure when the swap later lands. 4. The fit-target div had `p-1.5` padding, skewing FitAddon's cols/rows computation. Fixes applied to `DirectTerminal.tsx`: - Drop `var(...)` from `fontFamily`; use a plain font stack. - Set `lineHeight: 1.2` to restore vertical breathing room. - Add a `document.fonts` `loadingdone` listener that clears the texture atlas and re-fits when the webfont swap completes. Cleaned up in the effect teardown. - Remove `p-1.5` from the terminal ref div. Co-Authored-By: Claude Opus 4 --- .../web/src/components/DirectTerminal.tsx | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index d8ea199c5..1729c166f 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -235,11 +235,17 @@ export function DirectTerminal({ const activeTheme = isDark ? terminalThemes.dark : terminalThemes.light; // Initialize xterm.js Terminal + // NOTE: fontFamily must not contain `var(...)` — xterm's internal char-size + // measurement uses canvas ctx.font which cannot resolve CSS custom properties. + // Using a plain stack keeps measurement and rendering in the same font. const terminal = new Terminal({ cursorBlink: true, fontSize: fontSize, fontFamily: - 'var(--font-jetbrains-mono), "JetBrains Mono", "SF Mono", Menlo, Monaco, "Courier New", monospace', + '"JetBrains Mono", "SF Mono", Menlo, Monaco, "Courier New", monospace', + // xterm v6 default lineHeight (1.0) collides rows with JetBrains Mono's + // tall x-height. 1.2 restores visual breathing room between lines. + lineHeight: 1.2, theme: activeTheme, // Light mode needs an explicit contrast floor because agent UIs often emit // dim/faint ANSI sequences that become unreadable on a near-white background. @@ -316,6 +322,24 @@ export function DirectTerminal({ } }, 100); + // Re-measure when webfonts finish loading. next/font uses font-display:swap, + // so document.fonts.ready can resolve before JetBrains Mono actually swaps + // in. Without this, xterm's initial cell measurement stays pinned to the + // fallback font — producing wide-looking horizontal cells once the real + // font swaps. Listening to 'loadingdone' forces a re-fit when the swap + // actually completes. + const handleFontsLoadingDone = () => { + if (!mounted || !fitAddon.current || !terminalInstance.current) return; + try { + terminalInstance.current.clearTextureAtlas?.(); + fitAddon.current.fit(); + resizeTerminalMux(sessionId, terminalInstance.current.cols, terminalInstance.current.rows); + } catch { + // Ignore fit errors + } + }; + document.fonts.addEventListener("loadingdone", handleFontsLoadingDone); + // Grab viewport element for manual follow-output scroll const viewport = terminal.element?.querySelector(".xterm-viewport") ?? null; @@ -470,6 +494,7 @@ export function DirectTerminal({ selectionDisposable.dispose(); if (safetyTimer) clearTimeout(safetyTimer); window.removeEventListener("resize", handleResize); + document.fonts.removeEventListener("loadingdone", handleFontsLoadingDone); viewport?.removeEventListener("scroll", handleViewportScroll); inputDisposable?.dispose(); inputDisposable = null; @@ -871,7 +896,7 @@ export function DirectTerminal({ ) : null}
From 76d30e386b8f7ec2ac918c3f476c8523c5bbc726 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Mon, 20 Apr 2026 04:49:19 +0530 Subject: [PATCH 2/3] fix(web): address PR #1352 review comments - Resolve --font-jetbrains-mono via getComputedStyle at runtime so xterm honours the app's configured mono font token instead of a hard-coded "JetBrains Mono" fallback. next/font generates a unique family name (stored in the CSS custom property) that now gets fed to xterm alongside the fallback stack. - Re-resolve the font-family on `document.fonts` `loadingdone` so xterm picks up the generated name once it registers. - Change SessionDetail's DirectTerminal loading skeleton from a fixed h-[440px] to h-full, so the terminal area stays viewport-sized during the lazy-load window instead of locking to 440px. Co-Authored-By: Claude Opus 4 --- .../web/src/components/DirectTerminal.tsx | 37 ++++++++++++++++--- packages/web/src/components/SessionDetail.tsx | 4 +- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index 1729c166f..c233687c4 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -21,6 +21,30 @@ const FONT_SIZE_MIN = 9; const FONT_SIZE_MAX = 18; const FONT_SIZE_DEFAULT = 13; +// Fallback mono stack used when the CSS custom property isn't resolvable yet. +const MONO_FONT_FALLBACK = + '"JetBrains Mono", "SF Mono", Menlo, Monaco, "Courier New", monospace'; + +/** + * Resolve the app's configured mono font token to a concrete font-family string. + * xterm's internal char-size measurement ultimately hits canvas ctx.font, which + * cannot evaluate `var(...)`. Reading the CSS custom property with + * getComputedStyle gives us the generated next/font family name (e.g. + * `__JetBrains_Mono_abc123`), which we can safely feed into xterm while still + * honouring the app's font configuration. + */ +function resolveMonoFontFamily(): string { + if (typeof window === "undefined") return MONO_FONT_FALLBACK; + try { + const resolved = getComputedStyle(document.documentElement) + .getPropertyValue("--font-jetbrains-mono") + .trim(); + return resolved ? `${resolved}, ${MONO_FONT_FALLBACK}` : MONO_FONT_FALLBACK; + } catch { + return MONO_FONT_FALLBACK; + } +} + function getStoredFontSize(): number { if (typeof window === "undefined") return FONT_SIZE_DEFAULT; try { @@ -235,14 +259,14 @@ export function DirectTerminal({ const activeTheme = isDark ? terminalThemes.dark : terminalThemes.light; // Initialize xterm.js Terminal - // NOTE: fontFamily must not contain `var(...)` — xterm's internal char-size - // measurement uses canvas ctx.font which cannot resolve CSS custom properties. - // Using a plain stack keeps measurement and rendering in the same font. + // NOTE: xterm's internal char-size measurement uses canvas ctx.font which + // cannot resolve `var(...)`. resolveMonoFontFamily() reads the CSS custom + // property at runtime so we still honour the app's configured font token + // (next/font generated name) while handing xterm a concrete string. const terminal = new Terminal({ cursorBlink: true, fontSize: fontSize, - fontFamily: - '"JetBrains Mono", "SF Mono", Menlo, Monaco, "Courier New", monospace', + fontFamily: resolveMonoFontFamily(), // xterm v6 default lineHeight (1.0) collides rows with JetBrains Mono's // tall x-height. 1.2 restores visual breathing room between lines. lineHeight: 1.2, @@ -331,6 +355,9 @@ export function DirectTerminal({ const handleFontsLoadingDone = () => { if (!mounted || !fitAddon.current || !terminalInstance.current) return; try { + // Re-resolve the CSS var in case next/font registered its family + // name after initial construction, then force a re-measure. + terminalInstance.current.options.fontFamily = resolveMonoFontFamily(); terminalInstance.current.clearTextureAtlas?.(); fitAddon.current.fit(); resizeTerminalMux(sessionId, terminalInstance.current.cols, terminalInstance.current.rows); diff --git a/packages/web/src/components/SessionDetail.tsx b/packages/web/src/components/SessionDetail.tsx index 1e3b57ed2..5bdbc255a 100644 --- a/packages/web/src/components/SessionDetail.tsx +++ b/packages/web/src/components/SessionDetail.tsx @@ -24,8 +24,10 @@ const DirectTerminal = dynamic( () => import("./DirectTerminal").then((m) => ({ default: m.DirectTerminal })), { ssr: false, + // h-full (not a fixed 440px) so the skeleton matches the eventual terminal's + // flex-1 sizing and the layout stays viewport-driven during lazy load. loading: () => ( -
+
), }, ); From 3be86e7dd18eaa3ed30b7f84506322310f059369 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Mon, 20 Apr 2026 04:57:28 +0530 Subject: [PATCH 3/3] fix(web): address pr-review findings on #1352 Follow-up to the first round of review comments: - Guard `document.fonts` add/removeEventListener with feature detection. Without the guard, environments where FontFaceSet doesn't expose EventTarget (jsdom test mocks, older browsers) threw a TypeError during xterm init and the terminal silently failed to attach. - Update DirectTerminal.render.test.tsx mock so `document.fonts` exposes addEventListener/removeEventListener stubs, restoring happy-path coverage of the init code path. - Export `resolveMonoFontFamily` and add unit tests covering: CSS var present (prepended to fallback), CSS var absent (fallback only), and the invariant that no `var(...)` token ever leaks into the output. - Clarify in the docblock why we read `--font-jetbrains-mono` and deliberately avoid `--font-mono` (the latter re-wraps in `var(...)` and would re-introduce the bug). Co-Authored-By: Claude Opus 4 --- .../web/src/components/DirectTerminal.tsx | 28 ++++++++++++-- .../__tests__/DirectTerminal.render.test.tsx | 9 ++++- .../__tests__/DirectTerminal.test.ts | 38 ++++++++++++++++++- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/web/src/components/DirectTerminal.tsx b/packages/web/src/components/DirectTerminal.tsx index c233687c4..d02e008f4 100644 --- a/packages/web/src/components/DirectTerminal.tsx +++ b/packages/web/src/components/DirectTerminal.tsx @@ -27,13 +27,22 @@ const MONO_FONT_FALLBACK = /** * Resolve the app's configured mono font token to a concrete font-family string. + * * xterm's internal char-size measurement ultimately hits canvas ctx.font, which - * cannot evaluate `var(...)`. Reading the CSS custom property with + * cannot evaluate `var(...)`. Reading `--font-jetbrains-mono` with * getComputedStyle gives us the generated next/font family name (e.g. * `__JetBrains_Mono_abc123`), which we can safely feed into xterm while still * honouring the app's font configuration. + * + * NOTE: we deliberately read `--font-jetbrains-mono` and NOT `--font-mono`. + * `--font-mono` in globals.css is itself a composed stack that contains + * `var(--font-jetbrains-mono)` — if we forwarded that to xterm, the raw + * `var(...)` token would end up back in canvas ctx.font and reintroduce the + * original measurement bug this helper exists to fix. + * + * Exported for unit testing. */ -function resolveMonoFontFamily(): string { +export function resolveMonoFontFamily(): string { if (typeof window === "undefined") return MONO_FONT_FALLBACK; try { const resolved = getComputedStyle(document.documentElement) @@ -365,7 +374,16 @@ export function DirectTerminal({ // Ignore fit errors } }; - document.fonts.addEventListener("loadingdone", handleFontsLoadingDone); + // Feature-detect `FontFaceSet.addEventListener` — it's missing in + // jsdom's `document.fonts` mock and some older runtimes. Without the + // guard, init throws a TypeError and the terminal never attaches. + const fontsFace = + typeof document !== "undefined" ? document.fonts : undefined; + const fontsListenerAttached = + !!fontsFace && typeof fontsFace.addEventListener === "function"; + if (fontsListenerAttached) { + fontsFace!.addEventListener("loadingdone", handleFontsLoadingDone); + } // Grab viewport element for manual follow-output scroll const viewport = terminal.element?.querySelector(".xterm-viewport") ?? null; @@ -521,7 +539,9 @@ export function DirectTerminal({ selectionDisposable.dispose(); if (safetyTimer) clearTimeout(safetyTimer); window.removeEventListener("resize", handleResize); - document.fonts.removeEventListener("loadingdone", handleFontsLoadingDone); + if (fontsListenerAttached && fontsFace) { + fontsFace.removeEventListener("loadingdone", handleFontsLoadingDone); + } viewport?.removeEventListener("scroll", handleViewportScroll); inputDisposable?.dispose(); inputDisposable = null; diff --git a/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx b/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx index 075015a87..dd41e2606 100644 --- a/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx +++ b/packages/web/src/components/__tests__/DirectTerminal.render.test.tsx @@ -108,7 +108,14 @@ describe("DirectTerminal render", () => { MockWebSocket.instances = []; Object.defineProperty(document, "fonts", { configurable: true, - value: { ready: Promise.resolve() }, + value: { + ready: Promise.resolve(), + // FontFaceSet is an EventTarget in real browsers; the component + // listens for 'loadingdone' to re-fit after webfont swap. Stub the + // methods so init doesn't throw. + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + }, }); vi.stubGlobal("WebSocket", MockWebSocket); vi.stubGlobal( diff --git a/packages/web/src/components/__tests__/DirectTerminal.test.ts b/packages/web/src/components/__tests__/DirectTerminal.test.ts index cf7b9b483..637c6296a 100644 --- a/packages/web/src/components/__tests__/DirectTerminal.test.ts +++ b/packages/web/src/components/__tests__/DirectTerminal.test.ts @@ -1,5 +1,5 @@ -import { describe, it, expect } from "vitest"; -import { buildTerminalThemes } from "@/components/DirectTerminal"; +import { afterEach, beforeEach, describe, it, expect } from "vitest"; +import { buildTerminalThemes, resolveMonoFontFamily } from "@/components/DirectTerminal"; const HEX_RE = /^#[0-9a-fA-F]{6}$/; const ANSI_KEYS = [ @@ -86,3 +86,37 @@ describe("buildTerminalThemes", () => { expect(dark.selectionBackground).not.toBe(light.selectionBackground); }); }); + +describe("resolveMonoFontFamily", () => { + const FALLBACK = + '"JetBrains Mono", "SF Mono", Menlo, Monaco, "Courier New", monospace'; + + beforeEach(() => { + document.documentElement.style.removeProperty("--font-jetbrains-mono"); + }); + + afterEach(() => { + document.documentElement.style.removeProperty("--font-jetbrains-mono"); + }); + + it("prepends the resolved CSS variable to the fallback stack", () => { + document.documentElement.style.setProperty( + "--font-jetbrains-mono", + "__JetBrains_Mono_abc123", + ); + const result = resolveMonoFontFamily(); + expect(result).toBe(`__JetBrains_Mono_abc123, ${FALLBACK}`); + }); + + it("returns the fallback stack when the CSS variable is not set", () => { + expect(resolveMonoFontFamily()).toBe(FALLBACK); + }); + + it("never emits `var(...)` (would reintroduce the canvas-parse bug)", () => { + document.documentElement.style.setProperty( + "--font-jetbrains-mono", + "__JetBrains_Mono_abc123", + ); + expect(resolveMonoFontFamily()).not.toMatch(/var\(/); + }); +});