fix(capture): address review feedback on font extractor

ukimsanov · ukimsanov · commit 5e7a7a89561d · 2026-05-20T17:54:32.000-07:00
Five fixes from Copilot's inline review + Miguel's note on PR heygen-com#987: 1. inferWeightFromSubfamily — only matched concatenated forms ("extralight", "semibold"). Spaced ("Extra Light") and hyphenated ("Extra-Light") variants fell through to the 400 default, misreporting 200-weight fonts as 400. Now normalizes `[\s-]+` out of the subfamily before matching. 2. meta.tool — was hardcoded to "fontkit@2.0.4" but `packages/cli/package.json` allows ^2.0.4, so the manifest string would drift on every dep bump. Now records just "fontkit"; the version moves with the dep and can be discovered from package.json at debug-time if needed. 3. FontFileMetadata.rawFamily — docstring said "nameID 16 preferred, then nameID 1" but the code also derives from PostScript via deriveFamilyFromPostscript when both name-table fields are missing. Doc now reflects the actual three-step precedence. 4. FontFileMetadata.weight — docstring said "100-900" but the code emits 0 (when identified: false) and 950 (when canonicalizeFamily picks ExtraBlack/UltraBlack). Doc now documents both edge values explicitly. 5. sharp ^0.34.5 — bumped from ^0.34.0 on this PR but font extraction doesn't use sharp; the bump is needed by the contact sheet code in PR heygen-com#988. Reverted on heygen-com#987; will re-bump on heygen-com#988 where it's actually consumed. Also adds vitest coverage: - 34 tests in fontMetadataExtractor.test.ts - Covers inferWeightFromSubfamily for concatenated, spaced, and hyphenated forms (including composite styles like "Bold Italic" and case-insensitivity) - Covers canonicalizeFamily for unchanged families, stripped weight tokens, preserved width modifiers, and the 950 emit - Integration tests for extractFontMetadata (non-existent dir, empty dir) verifying the meta.tool / generatedAt shape Exported `inferWeightFromSubfamily` and `canonicalizeFamily` for testing. Pure functions, internal helpers, but exporting is the clean way to pin their behavior against regressions.
diff --git a/packages/cli/package.json b/packages/cli/package.json
@@ -38,7 +38,7 @@
     "postcss": "^8.5.8",
     "prettier": "^3.8.1",
     "puppeteer-core": "^24.39.1",
-    "sharp": "^0.34.5"
+    "sharp": "^0.34.0"
   },
   "devDependencies": {
     "@clack/prompts": "^1.1.0",
diff --git a/packages/cli/src/capture/fontMetadataExtractor.test.ts b/packages/cli/src/capture/fontMetadataExtractor.test.ts
@@ -0,0 +1,176 @@
+import { describe, expect, it } from "vitest";
+import { mkdtempSync, rmSync, existsSync, readFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import {
+  canonicalizeFamily,
+  extractFontMetadata,
+  inferWeightFromSubfamily,
+} from "./fontMetadataExtractor.js";
+
+describe("inferWeightFromSubfamily", () => {
+  // The concatenated forms were always handled. The spaced and hyphenated
+  // forms were the bug Copilot flagged on PR #987 — "Extra Light" used to
+  // fall through to the 400 default before the whitespace-normalization fix.
+  describe("concatenated forms (already handled)", () => {
+    it.each([
+      ["Thin", 100],
+      ["ExtraLight", 200],
+      ["UltraLight", 200],
+      ["Light", 300],
+      ["Regular", 400],
+      ["Medium", 500],
+      ["SemiBold", 600],
+      ["DemiBold", 600],
+      ["Bold", 700],
+      ["ExtraBold", 800],
+      ["UltraBold", 800],
+      ["Black", 900],
+      ["Heavy", 900],
+    ])("%s → %d", (subfamily, expected) => {
+      expect(inferWeightFromSubfamily(subfamily)).toBe(expected);
+    });
+  });
+
+  describe("spaced forms (the bug fix)", () => {
+    it.each([
+      ["Extra Light", 200],
+      ["Ultra Light", 200],
+      ["Semi Bold", 600],
+      ["Demi Bold", 600],
+      ["Extra Bold", 800],
+      ["Ultra Bold", 800],
+    ])("%s → %d", (subfamily, expected) => {
+      expect(inferWeightFromSubfamily(subfamily)).toBe(expected);
+    });
+  });
+
+  describe("hyphenated forms (the bug fix)", () => {
+    it.each([
+      ["Extra-Light", 200],
+      ["Semi-Bold", 600],
+      ["Extra-Bold", 800],
+    ])("%s → %d", (subfamily, expected) => {
+      expect(inferWeightFromSubfamily(subfamily)).toBe(expected);
+    });
+  });
+
+  describe("composite styles", () => {
+    it("Bold Italic still detects Bold", () => {
+      expect(inferWeightFromSubfamily("Bold Italic")).toBe(700);
+    });
+    it("Semi Bold Italic still detects SemiBold (priority over Bold)", () => {
+      expect(inferWeightFromSubfamily("Semi Bold Italic")).toBe(600);
+    });
+    it("ExtraBold Italic still detects ExtraBold (priority over Bold)", () => {
+      expect(inferWeightFromSubfamily("ExtraBold Italic")).toBe(800);
+    });
+  });
+
+  it("unknown subfamily falls back to 400 (Regular)", () => {
+    expect(inferWeightFromSubfamily("Headline")).toBe(400);
+    expect(inferWeightFromSubfamily("")).toBe(400);
+    expect(inferWeightFromSubfamily("Some Random Style")).toBe(400);
+  });
+
+  it("is case-insensitive", () => {
+    expect(inferWeightFromSubfamily("EXTRA LIGHT")).toBe(200);
+    expect(inferWeightFromSubfamily("extra light")).toBe(200);
+    expect(inferWeightFromSubfamily("ExTrA LiGhT")).toBe(200);
+  });
+});
+
+describe("canonicalizeFamily", () => {
+  it("returns family unchanged when no weight token is trailing", () => {
+    expect(canonicalizeFamily("Inter")).toEqual({
+      canonical: "Inter",
+      inferredWeight: null,
+    });
+    expect(canonicalizeFamily("Tiempos Headline")).toEqual({
+      canonical: "Tiempos Headline",
+      inferredWeight: null,
+    });
+    expect(canonicalizeFamily("Söhne Breit")).toEqual({
+      canonical: "Söhne Breit",
+      inferredWeight: null,
+    });
+  });
+
+  it("strips trailing weight tokens and surfaces the implied weight", () => {
+    expect(canonicalizeFamily("Inter Medium")).toEqual({
+      canonical: "Inter",
+      inferredWeight: 500,
+    });
+    expect(canonicalizeFamily("Inter Light")).toEqual({
+      canonical: "Inter",
+      inferredWeight: 300,
+    });
+    expect(canonicalizeFamily("Inter Bold")).toEqual({
+      canonical: "Inter",
+      inferredWeight: 700,
+    });
+    expect(canonicalizeFamily("Funnel Display Light")).toEqual({
+      canonical: "Funnel Display",
+      inferredWeight: 300,
+    });
+  });
+
+  it("preserves width modifiers before the weight token", () => {
+    expect(canonicalizeFamily("Inter Tight Medium")).toEqual({
+      canonical: "Inter Tight",
+      inferredWeight: 500,
+    });
+  });
+
+  it("emits 950 for ExtraBlack / UltraBlack (mirrors foundry intent)", () => {
+    expect(canonicalizeFamily("Inter ExtraBlack")).toEqual({
+      canonical: "Inter",
+      inferredWeight: 950,
+    });
+  });
+
+  it("returns empty input unchanged", () => {
+    expect(canonicalizeFamily("")).toEqual({
+      canonical: "",
+      inferredWeight: null,
+    });
+  });
+});
+
+describe("extractFontMetadata", () => {
+  // Light integration tests against the public surface — uses a real
+  // temp directory and verifies the manifest shape. Doesn't require
+  // fixture font binaries; the non-existent and empty-directory cases
+  // exercise the happy paths for the surrounding pipeline.
+
+  it("returns an empty manifest when the fonts directory doesn't exist", () => {
+    const tmp = mkdtempSync(join(tmpdir(), "hf-font-test-"));
+    try {
+      const outputPath = join(tmp, "manifest.json");
+      const manifest = extractFontMetadata(join(tmp, "does-not-exist"), outputPath);
+      expect(manifest.files).toEqual([]);
+      expect(manifest.families).toEqual([]);
+      expect(manifest.unidentified).toEqual([]);
+      expect(existsSync(outputPath)).toBe(true);
+      const written = JSON.parse(readFileSync(outputPath, "utf-8")) as typeof manifest;
+      expect(written.files).toEqual([]);
+      expect(written.meta.tool).toBe("fontkit");
+      expect(typeof written.meta.generatedAt).toBe("string");
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+
+  it("writes a manifest with the documented meta shape", () => {
+    const tmp = mkdtempSync(join(tmpdir(), "hf-font-test-"));
+    try {
+      const outputPath = join(tmp, "manifest.json");
+      const manifest = extractFontMetadata(tmp, outputPath);
+      expect(manifest.meta.tool).toBe("fontkit"); // no version hardcoded — moves with the dep
+      // generatedAt is an ISO string
+      expect(manifest.meta.generatedAt).toMatch(/^\d{4}-\d{2}-\d{2}T/);
+    } finally {
+      rmSync(tmp, { recursive: true, force: true });
+    }
+  });
+});
diff --git a/packages/cli/src/capture/fontMetadataExtractor.ts b/packages/cli/src/capture/fontMetadataExtractor.ts
@@ -36,13 +36,30 @@ export interface FontFileMetadata {
    * typographic family aggregate cleanly. See rawFamily for the unmodified value.
    */
   family: string;
-  /** Raw family name from the OpenType name table (nameID 16 preferred, then nameID 1). Empty if unidentifiable. */
+  /**
+   * Raw family name as extracted, before canonicalization. Source precedence:
+   *   1. OpenType `name` table (nameID 16 if present, else nameID 1)
+   *   2. Fallback: derived from the PostScript name (nameID 6) before the first
+   *      `-` (e.g. PostScript "Inter-Regular" → "Inter")
+   * Empty string when both the name table and PostScript name are absent
+   * (i.e. when `identified` is false).
+   */
   rawFamily: string;
   /** Subfamily / style name from nameID 17 or 2 (e.g. "Regular", "Bold Italic") */
   subfamily: string;
   /** PostScript name from nameID 6 (e.g. "Inter-Regular") */
   postscript: string;
-  /** OS/2 usWeightClass (100–900). Approximate for variable fonts — see variationAxes. */
+  /**
+   * Weight value. Typically the OS/2 `usWeightClass` (100–900) when present.
+   * Other values you may see:
+   *   - `0`: returned when the file is `identified: false` (no name-table data
+   *     to infer from); treat as unknown.
+   *   - `950`: emitted by the family-name canonicalization when a foundry
+   *     packaged "ExtraBlack" or "UltraBlack" as its own family. This is
+   *     outside the 100-900 standard range but mirrors the foundry intent.
+   * For variable fonts, this is the file's default axis position — see
+   * `variationAxes` for the available `wght` range.
+   */
   weight: number;
   /** "normal" or "italic" — derived from subfamily and OS/2 fsSelection */
   style: "normal" | "italic";
@@ -110,7 +127,9 @@ export function extractFontMetadata(fontsDir: string, outputPath: string): Fonts
     unidentified,
     meta: {
       generatedAt: new Date().toISOString(),
-      tool: "fontkit@2.0.4",
+      // Record just the tool name; the version moves with the dep and would
+      // otherwise drift from a hardcoded string on every fontkit bump.
+      tool: "fontkit",
     },
   };
 
@@ -206,10 +225,21 @@ function deriveFamilyFromPostscript(postscript: string): string {
   return (dashIdx > 0 ? postscript.slice(0, dashIdx) : postscript).trim();
 }
 
-/** Fallback when OS/2 table is missing — guess weight from "Bold", "Light", etc. */
+/**
+ * Fallback when OS/2 table is missing — guess weight from "Bold", "Light", etc.
+ *
+ * Normalizes spaces and hyphens out of the subfamily before matching so that
+ * fonts using spaced names ("Extra Light", "Semi Bold") or hyphenated names
+ * ("Extra-Light", "Semi-Bold") resolve to the same weight as the concatenated
+ * forms ("ExtraLight", "SemiBold"). Without this, a font subfamily of
+ * "Extra Light" would fall through every concat check and end at the 400
+ * default, misreporting a 200-weight font as 400.
+ *
+ * Exported for unit testing.
+ */
 // fallow-ignore-next-line complexity
-function inferWeightFromSubfamily(subfamily: string): number {
-  const s = subfamily.toLowerCase();
+export function inferWeightFromSubfamily(subfamily: string): number {
+  const s = subfamily.toLowerCase().replace(/[\s-]+/g, "");
   if (s.includes("thin")) return 100;
   if (s.includes("extralight") || s.includes("ultralight")) return 200;
   if (s.includes("light")) return 300;
@@ -273,8 +303,9 @@ const WEIGHT_TOKEN_RE = new RegExp(`\\s+(${Object.keys(WEIGHT_TOKEN_TO_VALUE).jo
  * italic flag is recovered separately from the OS/2 fsSelection bit, so no
  * information is lost.
  */
+// Exported for unit testing.
 // fallow-ignore-next-line complexity
-function canonicalizeFamily(family: string): {
+export function canonicalizeFamily(family: string): {
   canonical: string;
   inferredWeight: number | null;
 } {
