test(fe): regression guards for metric-render bugs + fix theme crash (#1294)

src/api/transforms.regression.test.ts

@@ -0,0 +1,144 @@
+/**
+ * Regression guards for the metric-rendering bug classes that recurred most
+ * (silent wrong/blank numbers). These pin behavior that is currently CORRECT so
+ * a future refactor can't reintroduce the regressions:
+ *
+ *  - not-ingested (NULL) must render as a null value, never "0" / "0%" — a
+ *    fabricated zero misrepresents a contributor (Refs #1337).
+ *  - a real zero must stay distinct from not-ingested.
+ *  - no delta is fabricated when the previous period is absent.
+ *  - delta direction respects `higher_is_better` (a sign flip is a wrong-signal
+ *    correctness bug).
+ *
+ * Mirrors the fixture builders in transforms.test.ts.
+ */
+import { describe, expect, it } from "vitest";
+
+import type { CatalogMetric, CatalogResponse } from "./catalog-client";
+import type { RawIcAggregateRow } from "./raw-types";
+import { transformIcKpis } from "./transforms";
+
+const TENANT = "t-test";
+
+function icKpiRow(
+  bareKey: string,
+  overrides: Partial<CatalogMetric> = {},
+): CatalogMetric {
+  return {
+    id: `id-kpi-${bareKey}`,
+    metric_key: `ic_kpis.${bareKey}`,
+    label: `KPI ${bareKey}`,
+    sublabel: "src",
+    description: "desc",
+    higher_is_better: true,
+    is_member_scale: false,
+    source_tags: [],
+    schema_status: "ok",
+    format: "integer",
+    thresholds: {
+      good: 0,
+      warn: 0,
+      resolved_from: "product-default",
+      bounded_by_lock: false,
+    },
+    ...overrides,
+  };
+}
+
+function catalogWith(metrics: CatalogMetric[]): CatalogResponse {
+  return {
+    tenant_id: TENANT,
+    generated_at: "2026-06-01T00:00:00Z",
+    metrics,
+    links: [],
+  };
+}
+
+function rawIc(overrides: Partial<RawIcAggregateRow> = {}): RawIcAggregateRow {
+  return {
+    person_id: "p",
+    loc: null,
+    ai_loc_share_pct: 0,
+    prs_merged: null,
+    pr_cycle_time_h: null,
+    focus_time_pct: 0,
+    tasks_closed: 0,
+    bugs_fixed: 0,
+    build_success_pct: null,
+    ai_sessions: 0,
+    ...overrides,
+  };
+}
+
+function pick(out: ReturnType<typeof transformIcKpis>, key: string) {
+  return out.find((k) => k.metric_key === key);
+}
+
+describe("transformIcKpis — not-ingested vs zero (Refs #1337)", () => {
+  it("renders a NULL (not-ingested) metric as a null value, never '0'", () => {
+    const out = transformIcKpis(
+      rawIc({ prs_merged: null }),
+      null,
+      "week",
+      catalogWith([icKpiRow("prs_merged")]),
+    );
+    const r = pick(out, "prs_merged");
+    expect(r?.raw_value).toBeNull();
+    // The bug: a not-ingested metric showing "0" fakes a real measurement.
+    expect(r?.value).toBeNull();
+  });
+
+  it("renders a real zero as '0', distinct from not-ingested null", () => {
+    const out = transformIcKpis(
+      rawIc({ tasks_closed: 0 }),
+      null,
+      "week",
+      catalogWith([icKpiRow("tasks_closed")]),
+    );
+    const r = pick(out, "tasks_closed");
+    expect(r?.raw_value).toBe(0);
+    expect(r?.value).toBe("0");
+  });
+
+  it("renders a NULL percent metric as null, never '0%' (null-ratio bug)", () => {
+    const out = transformIcKpis(
+      rawIc({ build_success_pct: null }),
+      null,
+      "week",
+      catalogWith([icKpiRow("build_success_pct", { format: "percent" })]),
+    );
+    expect(pick(out, "build_success_pct")?.value).toBeNull();
+  });
+});
+
+describe("transformIcKpis — delta direction & honesty", () => {
+  it("emits no delta when the previous period is missing (no fabricated trend)", () => {
+    const out = transformIcKpis(
+      rawIc({ tasks_closed: 5 }),
+      null,
+      "week",
+      catalogWith([icKpiRow("tasks_closed")]),
+    );
+    const r = pick(out, "tasks_closed");
+    expect(r?.delta).toBe("");
+    expect(r?.delta_type).toBe("neutral");
+  });
+
+  it("colors an increase 'good' when higher_is_better, 'bad' when not", () => {
+    const goodOut = transformIcKpis(
+      rawIc({ tasks_closed: 9 }),
+      rawIc({ tasks_closed: 4 }),
+      "week",
+      catalogWith([icKpiRow("tasks_closed", { higher_is_better: true })]),
+    );
+    expect(pick(goodOut, "tasks_closed")?.delta_type).toBe("good");
+
+    const badOut = transformIcKpis(
+      rawIc({ tasks_closed: 9 }),
+      rawIc({ tasks_closed: 4 }),
+      "week",
+      catalogWith([icKpiRow("tasks_closed", { higher_is_better: false })]),
+    );
+    expect(pick(badOut, "tasks_closed")?.delta_type).toBe("bad");
+  });
+});

src/components/theme-provider.test.tsx

@@ -0,0 +1,93 @@
+/**
+ * ThemeProvider robustness against untrusted `localStorage` (Refs #1294).
+ *
+ * The persisted theme is user-controllable storage. A stale/edited value — or
+ * one with whitespace like "light dark" — used to flow straight into
+ * `classList.add(theme)` (cast `as Theme`) and throw `InvalidCharacterError`,
+ * crashing the whole app at the React error boundary. These tests pin that an
+ * invalid value falls back to the default and never crashes the render.
+ *
+ * `defaultTheme="light"` is used throughout so the effect takes the explicit
+ * light/dark branch and never touches `matchMedia` (also stubbed for safety).
+ */
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import { fireEvent, render, screen } from "@testing-library/react";
+
+import { ThemeProvider, useTheme } from "./theme-provider";
+
+function ThemeProbe() {
+  const { theme, setTheme } = useTheme();
+  return (
+    <div>
+      <span data-testid="theme">{theme}</span>
+      <button onClick={() => setTheme("dark")}>set-dark</button>
+    </div>
+  );
+}
+
+beforeEach(() => {
+  localStorage.clear();
+  document.documentElement.className = "";
+  // jsdom has no matchMedia; the "system" branch calls it.
+  vi.stubGlobal("matchMedia", (query: string) => ({
+    matches: false,
+    media: query,
+    onchange: null,
+    addEventListener: () => {},
+    removeEventListener: () => {},
+    addListener: () => {},
+    removeListener: () => {},
+    dispatchEvent: () => false,
+  }));
+});
+
+describe("ThemeProvider — untrusted localStorage (Refs #1294)", () => {
+  it("falls back to the default for an unknown stored theme (no crash)", () => {
+    localStorage.setItem("theme", "not-a-theme");
+    expect(() =>
+      render(
+        <ThemeProvider defaultTheme="light">
+          <ThemeProbe />
+        </ThemeProvider>,
+      ),
+    ).not.toThrow();
+    expect(screen.getByTestId("theme").textContent).toBe("light");
+  });
+
+  it("does not crash on a whitespace value that would break classList.add", () => {
+    // "light dark" → classList.add("light dark") throws InvalidCharacterError
+    // in the effect → error boundary, pre-fix. Post-fix it falls back.
+    localStorage.setItem("theme", "light dark");
+    expect(() =>
+      render(
+        <ThemeProvider defaultTheme="light">
+          <ThemeProbe />
+        </ThemeProvider>,
+      ),
+    ).not.toThrow();
+    expect(screen.getByTestId("theme").textContent).toBe("light");
+    expect(document.documentElement.classList.contains("light")).toBe(true);
+  });
+
+  it("honors a valid stored theme", () => {
+    localStorage.setItem("theme", "dark");
+    render(
+      <ThemeProvider defaultTheme="light">
+        <ThemeProbe />
+      </ThemeProvider>,
+    );
+    expect(screen.getByTestId("theme").textContent).toBe("dark");
+    expect(document.documentElement.classList.contains("dark")).toBe(true);
+  });
+
+  it("persists a theme change through setTheme", () => {
+    render(
+      <ThemeProvider defaultTheme="light">
+        <ThemeProbe />
+      </ThemeProvider>,
+    );
+    fireEvent.click(screen.getByText("set-dark"));
+    expect(localStorage.getItem("theme")).toBe("dark");
+    expect(screen.getByTestId("theme").textContent).toBe("dark");
+  });
+});

src/components/theme-provider.tsx

@@ -20,13 +20,34 @@ const initialState: ThemeProviderState = {
 
 const ThemeProviderContext = createContext<ThemeProviderState>(initialState);
 
+const THEMES: readonly Theme[] = ["dark", "light", "system"];
+
+/**
+ * Read the persisted theme, tolerating a hostile / stale `localStorage`.
+ *
+ * The value is user-controllable storage, so it cannot be trusted as a `Theme`:
+ * a stale key, a manual edit, or a value with whitespace (e.g. `"light dark"`)
+ * would otherwise flow into `classList.add(theme)` and throw
+ * `InvalidCharacterError`, crashing the whole app at the React error boundary
+ * (Refs #1294). Validate against the known set and fall back to the default.
+ * `localStorage` access itself can throw (Safari private mode) — guard that too.
+ */
+function readStoredTheme(storageKey: string, fallback: Theme): Theme {
+  try {
+    const stored = localStorage.getItem(storageKey);
+    return THEMES.includes(stored as Theme) ? (stored as Theme) : fallback;
+  } catch {
+    return fallback;
+  }
+}
+
 export function ThemeProvider({
   children,
   defaultTheme = "system",
   storageKey = "theme",
 }: ThemeProviderProps) {
-  const [theme, setTheme] = useState<Theme>(
-    () => (localStorage.getItem(storageKey) as Theme) || defaultTheme,
+  const [theme, setTheme] = useState<Theme>(() =>
+    readStoredTheme(storageKey, defaultTheme),
   );
 
   useEffect(() => {