diff --git a/src/api/transforms.regression.test.ts b/src/api/transforms.regression.test.ts new file mode 100644 index 0000000..0ce75d9 --- /dev/null +++ b/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 { + 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 { + 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, 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"); + }); +}); diff --git a/src/components/theme-provider.test.tsx b/src/components/theme-provider.test.tsx new file mode 100644 index 0000000..161230f --- /dev/null +++ b/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 ( +
+ {theme} + +
+ ); +} + +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( + + + , + ), + ).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( + + + , + ), + ).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( + + + , + ); + expect(screen.getByTestId("theme").textContent).toBe("dark"); + expect(document.documentElement.classList.contains("dark")).toBe(true); + }); + + it("persists a theme change through setTheme", () => { + render( + + + , + ); + fireEvent.click(screen.getByText("set-dark")); + expect(localStorage.getItem("theme")).toBe("dark"); + expect(screen.getByTestId("theme").textContent).toBe("dark"); + }); +}); diff --git a/src/components/theme-provider.tsx b/src/components/theme-provider.tsx index c6fe0dc..80abfea 100644 --- a/src/components/theme-provider.tsx +++ b/src/components/theme-provider.tsx @@ -20,13 +20,34 @@ const initialState: ThemeProviderState = { const ThemeProviderContext = createContext(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( - () => (localStorage.getItem(storageKey) as Theme) || defaultTheme, + const [theme, setTheme] = useState(() => + readStoredTheme(storageKey, defaultTheme), ); useEffect(() => {