Skip to content

fix: terminal font dont always load#2155

Open
deadlyjack wants to merge 2 commits into
mainfrom
ajit/fix-terminal-font
Open

fix: terminal font dont always load#2155
deadlyjack wants to merge 2 commits into
mainfrom
ajit/fix-terminal-font

Conversation

@deadlyjack
Copy link
Copy Markdown
Member

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 5, 2026

Greptile Summary

This PR addresses terminal fonts not reliably loading by introducing a two-phase approach: injectFontFace (new, synchronous) registers the @font-face CSS and kicks off a non-blocking browser font load immediately, while loadFont (now augmented with await document.fonts.load(...)) ensures the font is fully parsed before xterm renders. An early call in main.js pre-registers the default terminal font at app startup.

  • fonts.injectFontFace: New synchronous helper that injects @font-face CSS and fires a background document.fonts.load(). Called before loadFont in the terminal init path and on font-setting changes.
  • terminal.js init path: loadTerminalFont() is now awaited via a _fontReady promise; after resolution it re-applies fontFamily and refreshes xterm. A safety requestAnimationFrame in mount() also re-applies the font on first paint.
  • Regression risk for remote fonts: injectFontFace injects the raw remote-URL CSS; loadFont then skips re-injection (font face already present), so the locally-cached URI version is never used — breaking offline loading for non-bundled fonts.

Confidence Score: 3/5

The fix improves font loading for bundled fonts but introduces a regression for remote fonts and a crash path if the terminal is disposed quickly.

Two concrete defects are present on the changed path: the requestAnimationFrame callbacks in mount() access this.terminal without a null guard (the _fontReady.then() handler added in the same PR already protects itself), and injectFontFace injecting raw remote-URL CSS before loadFont prevents loadFont from ever substituting the locally-cached URI, silently breaking offline support for non-bundled fonts like JetBrains Mono or Cascadia Code.

src/lib/fonts.js (remote-URL injection ordering) and src/components/terminal/terminal.js (missing null guard in mount's rAF callbacks).

Important Files Changed

Filename Overview
src/lib/fonts.js Adds injectFontFace (non-blocking CSS injection + fire-and-forget font load) and makes loadFont await document.fonts.load. The new function has a logic flaw with remote fonts: injecting raw CSS before loadFont runs causes loadFont to skip re-injection with the locally cached URI.
src/components/terminal/terminal.js Font loading made async in init() with a _fontReady promise; mount() adds a safety rAF re-apply. The rAF/setTimeout callbacks lack the if (this.terminal) null guard present in _fontReady.then(), risking a crash on early dispose.
src/main.js Adds an early fonts.injectFontFace call during app startup to pre-register the default terminal font. Only affects a bundled asset font so the remote-URL issue does not apply here.
src/settings/terminalSettings.js Adds fonts.injectFontFace(value) before fonts.loadFont(value) when a font setting changes. Same remote-font regression as in terminal.js loadTerminalFont().

Sequence Diagram

sequenceDiagram
    participant App as main.js
    participant Term as TerminalComponent
    participant Fonts as fonts.js
    participant Browser as Browser FontFaceSet

    App->>Fonts: injectFontFace(MesloLGS NF Regular)
    Fonts->>Browser: "inject @font-face CSS (bundled URL)"
    Fonts-->>Browser: document.fonts.load() fire and forget

    App->>Term: new TerminalComponent()
    Term->>Term: init()
    Term->>Fonts: injectFontFace(fontFamily)
    Fonts->>Browser: "inject @font-face CSS (raw remote URL)"
    Fonts-->>Browser: document.fonts.load() fire and forget
    Term->>Fonts: loadFont(fontFamily) await
    Fonts->>Fonts: downloadFont and replace remote URL with local URI
    Note over Fonts: Skip injection - font-family already in style!
    Fonts->>Browser: await document.fonts.load()
    Browser-->>Fonts: resolved
    Fonts-->>Term: returns

    Term->>Term: _fontReady.then() apply fontFamily refresh guarded

    Term->>Term: mount()
    Term->>Browser: requestAnimationFrame
    Browser-->>Term: frame fires apply fontFamily refresh no null check
Loading

Reviews (1): Last reviewed commit: "fix: terminal font dont always load" | Re-trigger Greptile

Comment thread src/components/terminal/terminal.js
Comment thread src/lib/fonts.js
Comment on lines +257 to +272
function injectFontFace(name) {
const $style = ensureStyleElement(FONT_FACE_STYLE_ID);
const css = get(name);

if (!css) return;

// Inject CSS if not already present (skip remote URL downloads - loadFont handles those)
if (!$style.textContent.includes(`font-family: '${name}'`)) {
$style.textContent = `${$style.textContent}\n${css}`;
}

// Kick off browser font loading without blocking
if (document.fonts?.load) {
document.fonts.load(`12px '${name}'`).catch(() => {});
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Remote fonts bypass local caching when injectFontFace is called before loadFont

injectFontFace injects the raw @font-face CSS (containing the original https://acode.app/… URL) into the style element. When loadFont is subsequently called in loadTerminalFont() or terminalSettings.js, it downloads the font file to local storage and replaces the remote URL with an internal URI (content://…) — but then the "already present" guard ($style.textContent.includes(...)) is true, so the locally-URI'd CSS is never injected. The browser is left with the remote URL and the locally cached copy is never served.

For locally bundled fonts (like MesloLGS NF Regular) this is harmless, but for any remote font (e.g., JetBrains Mono Regular, Cascadia Code) this breaks offline/cached font loading — a regression from the pre-PR behaviour where only loadFont ran and always replaced the URL before injecting.

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant