diff --git a/packages/app/e2e/browser/appearance-font-family.spec.ts b/packages/app/e2e/browser/appearance-font-family.spec.ts new file mode 100644 index 0000000000..6ec8600017 --- /dev/null +++ b/packages/app/e2e/browser/appearance-font-family.spec.ts @@ -0,0 +1,48 @@ +import { expect, test } from "../support/fixtures"; +import type { Page } from "@playwright/test"; +import { openSettingsSection } from "../support/helpers/settings"; + +async function openAppearance(page: Page) { + await page.goto("/settings"); + await openSettingsSection(page, "appearance"); +} + +async function setFontFamily(page: Page, name: string, family: string) { + const input = page.getByRole("textbox", { name }); + await input.fill(family); + await input.press("Tab"); +} + +for (const name of ["Code font family", "Interface font family"]) { + test(`keeps settings usable after an unfinished quote in ${name}`, async ({ page }, testInfo) => { + await openAppearance(page); + await setFontFamily(page, name, "'Fira Code', monospace"); + await expect(page.getByRole("textbox", { name })).toHaveValue("'Fira Code', monospace"); + + await setFontFamily(page, name, "'foo"); + await expect(page.getByRole("textbox", { name })).toHaveValue("'Fira Code', monospace"); + await expect(page.getByRole("button", { name: "Appearance", exact: true })).toBeInViewport(); + await expect(page.getByText("Theme", { exact: true }).first()).toHaveCSS("font-size", "12px"); + + await page.screenshot({ path: testInfo.outputPath("rejected-font.png") }); + await page.reload(); + await expect(page.getByRole("textbox", { name })).toHaveValue("'Fira Code', monospace"); + await expect(page.getByRole("button", { name: "Appearance", exact: true })).toBeInViewport(); + await setFontFamily(page, name, ""); + await expect(page.getByRole("textbox", { name })).toHaveValue(""); + }); +} + +test("opens settings with defaults when a malformed font was already saved", async ({ page }) => { + await page.addInitScript(() => { + localStorage.setItem( + "@paseo:app-settings", + JSON.stringify({ monoFontFamily: "'foo", uiFontFamily: '"foo' }), + ); + }); + await openAppearance(page); + await expect(page.getByRole("textbox", { name: "Code font family" })).toHaveValue(""); + await expect(page.getByRole("textbox", { name: "Interface font family" })).toHaveValue(""); + await expect(page.getByRole("button", { name: "Appearance", exact: true })).toBeInViewport(); + await expect(page.getByText("Theme", { exact: true }).first()).toHaveCSS("font-size", "12px"); +}); diff --git a/packages/app/src/hooks/use-settings/storage.test.ts b/packages/app/src/hooks/use-settings/storage.test.ts index d396f26d9a..6d9c8d4c7c 100644 --- a/packages/app/src/hooks/use-settings/storage.test.ts +++ b/packages/app/src/hooks/use-settings/storage.test.ts @@ -15,6 +15,7 @@ import { parseClampedFontSize, parseTerminalScrollbackLines, resolveContentMaxWidth, + sanitizeFontFamily, saveAppSettings, type SettingsDeps, } from "./storage"; @@ -1094,3 +1095,34 @@ describe("content max width", () => { expect((await load("wide")).contentMaxWidth).toBeNull(); }); }); + +describe("font family CSS safety", () => { + it.each(["'foo", '"foo', "Menlo, 'foo", "foo\\", "foo/*", "foo(", "foo["])( + "rejects unfinished CSS syntax: %s", + (value) => expect(sanitizeFontFamily(value)).toBeNull(), + ); + + it.each([ + "Menlo", + "'Fira Code', monospace", + '"Fira Code", monospace', + "'Font\\'s name'", + "Font\\ Name", + "'Font (Mono)'", + "'Font [Mono]'", + ])("preserves complete font families: %s", (value) => + expect(sanitizeFontFamily(value)).toBe(value), + ); + + it.each(["uiFontFamily", "monoFontFamily"])( + "recovers a persisted unmatched quote in %s", + async (field) => { + const deps = makeDeps({ + storage: createInMemoryKeyValueStorage({ + [APP_SETTINGS_KEY]: JSON.stringify({ [field]: "'foo" }), + }), + }); + expect(await loadAppSettingsFromStorage(deps)).toMatchObject({ [field]: "" }); + }, + ); +}); diff --git a/packages/app/src/hooks/use-settings/storage.ts b/packages/app/src/hooks/use-settings/storage.ts index 6c44546668..54ee6b10bc 100644 --- a/packages/app/src/hooks/use-settings/storage.ts +++ b/packages/app/src/hooks/use-settings/storage.ts @@ -522,7 +522,24 @@ export function sanitizeFontFamily(value: unknown): string | null { if ([...trimmed].some((char) => char.charCodeAt(0) <= 0x1f)) { return null; // control chars would corrupt the font-family string } - return trimmed; // quotes/commas are legit in stacks + // Unistyles serializes theme values into one shared stylesheet. An unfinished + // string, escape, comment, or block consumes the declarations that follow. + let quote: string | null = null; + let escaped = false; + for (const char of trimmed) { + if (escaped) { + escaped = false; + } else if (char === "\\") { + escaped = true; + } else if (quote !== null) { + if (char === quote) quote = null; + } else if (char === "'" || char === '"') { + quote = char; + } else if (/[()[\]/*]/.test(char)) { + return null; + } + } + return quote === null && !escaped ? trimmed : null; } async function loadLegacyDesktopSettingsFromStorage(storage: KeyValueStorage): Promise<{ diff --git a/packages/app/src/screens/settings/appearance/appearance-section.tsx b/packages/app/src/screens/settings/appearance/appearance-section.tsx index 6e98819323..2dfea09fd0 100644 --- a/packages/app/src/screens/settings/appearance/appearance-section.tsx +++ b/packages/app/src/screens/settings/appearance/appearance-section.tsx @@ -241,7 +241,7 @@ interface FontFamilyRowProps { draft: string; withBorder: boolean; onChangeDraft: (value: string) => void; - onCommit: (value: string) => void; + onCommit: (value: string) => string; } function FontFamilyRow({ @@ -255,13 +255,16 @@ function FontFamilyRow({ onChangeDraft, onCommit, }: FontFamilyRowProps) { + const inputRef = useRef(null); const handleCommit = useCallback(() => { - onCommit(draft); + const committed = onCommit(draft); + inputRef.current?.replaceText(committed); }, [draft, onCommit]); // Resync from the committed value when it changes elsewhere. useEffect(() => { onChangeDraft(value); + inputRef.current?.replaceText(value); // Only resync on external value changes, not on local keystrokes. // eslint-disable-next-line react-hooks/exhaustive-deps }, [value]); @@ -273,6 +276,7 @@ function FontFamilyRow({ {hint}