diff --git a/packages/react-ui/src/components/ThemeProvider/AGENTS.md b/packages/react-ui/src/components/ThemeProvider/AGENTS.md index 071f26ae3..7612cb0d9 100644 --- a/packages/react-ui/src/components/ThemeProvider/AGENTS.md +++ b/packages/react-ui/src/components/ThemeProvider/AGENTS.md @@ -2,14 +2,14 @@ ## File Map -| File | Purpose | -| ------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `ThemeProvider.tsx` | React component, `ThemeContext`, `InternalContext` (nesting detection), `useTheme` hook. Injects `--openui-*` vars into `` via `useInsertionEffect`. | -| `types.ts` | TypeScript interfaces: `Theme`, `ColorTheme`, `LayoutTheme`, `TypographyTheme`, `EffectTheme`, `ChartColorPalette`. | -| `defaultTheme.ts` | Builds `defaultLightTheme` / `defaultDarkTheme` (frozen) from the swatch system. Contains `createColorTheme()` and all layout/typography/shadow defaults. | -| `swatches.ts` | 18 oklch color families x 14 shades. Exports `swatch()`, `withAlpha()`, `swatchToken()`, `swatchTokens`. | -| `utils.ts` | Exports `camelToKebab`, `themeToCssVars`, `createTheme` (dev-mode typo detection with Levenshtein distance suggestions). | -| `index.ts` | Barrel re-exports from all the above files. | +| File | Purpose | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `ThemeProvider.tsx` | React component, `ThemeContext`, `InternalContext` (nesting detection), `useTheme` hook. Injects `--openui-*` vars into `` via `useInsertionEffect`. | +| `types.ts` | TypeScript interfaces: `Theme`, `ColorTheme`, `LayoutTheme`, `TypographyTheme`, `EffectTheme`, `ChartColorPalette`. | +| `defaultTheme.ts` | Builds `defaultLightTheme` / `defaultDarkTheme` (frozen) from the swatch system. Contains `createColorTheme()` and all layout/typography/shadow defaults. | +| `swatches.ts` | 18 oklch color families x 14 shades. Exports `swatch()`, `withAlpha()`, `swatchToken()`, `swatchTokens`. | +| `utils.ts` | Exports `camelToKebab`, `themeToCssVars`, `createTheme` (dev-mode typo detection with Levenshtein distance suggestions), and `KNOWN_THEME_KEYS` (shared validator allow-list). | +| `index.ts` | Barrel re-exports from all the above files. | ## Architecture @@ -44,6 +44,8 @@ Only **2 files** need editing — the runtime walker (`themeToCssVars`) and the Run `pnpm build` (or `pnpm generate:css-utils`) to regenerate `cssUtils.scss` with the new token and its fallback. +**Exception — chart palette keys** (`string[]`, no defaults): the validator allow-list is `Object.keys(defaultLightTheme)` + `CHART_PALETTE_KEYS`. The single source is the `CHART_PALETTE_KEYS` `as const` array in `types.ts` — the `ChartColorPalette` type is derived from it, so a new palette is added in exactly one place and type/runtime drift is impossible by construction. Do **not** give palettes defaults in `defaultTheme.ts`: a default would take precedence over the per-chart `theme` prop fallback in `useChartPalette`. + ### Naming Convention ``` diff --git a/packages/react-ui/src/components/ThemeProvider/ThemeProvider.tsx b/packages/react-ui/src/components/ThemeProvider/ThemeProvider.tsx index b377550cf..cbb4dffb1 100644 --- a/packages/react-ui/src/components/ThemeProvider/ThemeProvider.tsx +++ b/packages/react-ui/src/components/ThemeProvider/ThemeProvider.tsx @@ -1,7 +1,7 @@ import React, { createContext, useContext, useId, useInsertionEffect, useMemo } from "react"; import { defaultDarkTheme, defaultLightTheme } from "./defaultTheme"; import { Theme, ThemeMode } from "./types"; -import { themeToCssVars } from "./utils"; +import { KNOWN_THEME_KEYS, themeToCssVars } from "./utils"; /** * Props for the {@link ThemeProvider} component. @@ -109,8 +109,6 @@ function cssSafeId(id: string): string { return id.replace(/[^a-zA-Z0-9-_]/g, ""); } -const _knownThemeKeys = new Set(Object.keys(defaultLightTheme)); - function validateThemeObject(themeObj: Theme, propName: string) { for (const [key, value] of Object.entries(themeObj)) { if (value !== undefined && typeof value !== "string" && !Array.isArray(value)) { @@ -119,7 +117,7 @@ function validateThemeObject(themeObj: Theme, propName: string) { `[OpenUI] ${propName} key "${key}" has a non-string value (${typeof value}). All theme values should be strings.`, ); } - if (!_knownThemeKeys.has(key)) { + if (!KNOWN_THEME_KEYS.has(key)) { warnOnce( `unknown-key:${propName}:${key}`, `[OpenUI] ${propName} contains unknown key "${key}". It will be ignored. Use createTheme() for typo detection with suggestions.`, diff --git a/packages/react-ui/src/components/ThemeProvider/__tests__/ThemeProvider.test.tsx b/packages/react-ui/src/components/ThemeProvider/__tests__/ThemeProvider.test.tsx new file mode 100644 index 000000000..4c6c0942a --- /dev/null +++ b/packages/react-ui/src/components/ThemeProvider/__tests__/ThemeProvider.test.tsx @@ -0,0 +1,139 @@ +import { renderToString } from "react-dom/server"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useChartPalette } from "../../Charts/utils/PalletUtils"; +import { defaultDarkTheme, defaultLightTheme } from "../defaultTheme"; +import { ThemeProvider, useTheme } from "../ThemeProvider"; +import type { ChartColorPalette, Theme } from "../types"; +import { CHART_PALETTE_KEYS } from "../types"; +import { createTheme, KNOWN_THEME_KEYS } from "../utils"; + +// Regression tests for the types-vs-runtime drift where every `*ChartPalette` +// key (valid per the Theme type) was rejected as "unknown key" by both +// validators because the allow-list only contained Object.keys(defaultLightTheme). +// See https://github.com/thesysdev/openui/issues/714 + +// `Required` forces this literal to cover every declared palette key, so a new +// key added to ChartColorPalette fails compilation here until it is tested. +const allChartPalettes: Required = { + defaultChartPalette: ["#101010", "#202020", "#303030"], + barChartPalette: ["#111111", "#222222", "#333333"], + lineChartPalette: ["#414141", "#525252", "#636363"], + areaChartPalette: ["#747474", "#858585", "#969696"], + pieChartPalette: ["#a7a7a7", "#b8b8b8", "#c9c9c9"], + radarChartPalette: ["#dadada", "#ebebeb", "#fcfcfc"], + radialChartPalette: ["#0d0d0d", "#1e1e1e", "#2f2f2f"], + horizontalBarChartPalette: ["#404040", "#515151", "#626262"], +}; + +let warnSpy: ReturnType; + +beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); +}); + +afterEach(() => { + warnSpy.mockRestore(); +}); + +describe("theme validator allow-list", () => { + it("covers every declared chart palette key", () => { + const missing = CHART_PALETTE_KEYS.filter((key) => !KNOWN_THEME_KEYS.has(key)); + expect(missing).toEqual([]); + }); + + it("covers every key of the default themes", () => { + const missingLight = Object.keys(defaultLightTheme).filter((k) => !KNOWN_THEME_KEYS.has(k)); + const missingDark = Object.keys(defaultDarkTheme).filter((k) => !KNOWN_THEME_KEYS.has(k)); + expect(missingLight).toEqual([]); + expect(missingDark).toEqual([]); + }); +}); + +describe("createTheme", () => { + it("does not warn for any declared chart palette key", () => { + createTheme({ ...allChartPalettes }); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("still warns with a suggestion for a genuine typo", () => { + createTheme({ defaultChartPalete: ["#ffffff"] } as unknown as Theme); + expect(warnSpy).toHaveBeenCalledWith( + '[OpenUI] Unknown theme key "defaultChartPalete". Did you mean "defaultChartPalette"?', + ); + }); +}); + +describe("ThemeProvider prop validation", () => { + it("does not warn for chart palette keys on lightTheme or darkTheme", () => { + renderToString( + + + , + ); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('still warns "unknown key" for keys not on the Theme type', () => { + renderToString( + + + , + ); + expect(warnSpy).toHaveBeenCalledWith( + '[OpenUI] lightTheme contains unknown key "bogusThemeKey". It will be ignored. Use createTheme() for typo detection with suggestions.', + ); + }); +}); + +describe("chart palette flow (theme -> useTheme -> useChartPalette)", () => { + it("delivers user palettes to charts and falls back to defaultChartPalette", () => { + const defaultPalette = ["#57c8d6", "#ff6a2b", "#aabbcc"]; + const barPalette = ["#111111", "#222222", "#333333"]; + + let themeFromContext: Theme | undefined; + let barColors: string[] = []; + let lineColors: string[] = []; + + const Probe = () => { + const { theme } = useTheme(); + themeFromContext = theme; + // Exactly what BarChart does. + barColors = useChartPalette({ + chartThemeName: "ocean", + themePaletteName: "barChartPalette", + dataLength: 2, + }); + // lineChartPalette is not set -> must fall back to defaultChartPalette. + lineColors = useChartPalette({ + chartThemeName: "ocean", + themePaletteName: "lineChartPalette", + dataLength: 2, + }); + return null; + }; + + renderToString( + + + , + ); + + expect(themeFromContext?.defaultChartPalette).toEqual(defaultPalette); + expect(themeFromContext?.barChartPalette).toEqual(barPalette); + expect(barColors.length).toBe(2); + expect(barColors.every((color) => barPalette.includes(color))).toBe(true); + expect(lineColors.length).toBe(2); + expect(lineColors.every((color) => defaultPalette.includes(color))).toBe(true); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/react-ui/src/components/ThemeProvider/types.ts b/packages/react-ui/src/components/ThemeProvider/types.ts index 18a6328e8..2fc8832b5 100644 --- a/packages/react-ui/src/components/ThemeProvider/types.ts +++ b/packages/react-ui/src/components/ThemeProvider/types.ts @@ -4,17 +4,25 @@ export type ThemeMode = "light" | "dark"; /** * Optional color arrays used by chart components. * Each palette overrides the default for its specific chart type. + * + * Single source of truth for palette keys: the `ChartColorPalette` type is + * derived from this array and the runtime theme-key validators consume it, + * so a new palette is added in exactly one place. */ -export interface ChartColorPalette { - defaultChartPalette?: string[]; - barChartPalette?: string[]; - lineChartPalette?: string[]; - areaChartPalette?: string[]; - pieChartPalette?: string[]; - radarChartPalette?: string[]; - radialChartPalette?: string[]; - horizontalBarChartPalette?: string[]; -} +export const CHART_PALETTE_KEYS = [ + "defaultChartPalette", + "barChartPalette", + "lineChartPalette", + "areaChartPalette", + "pieChartPalette", + "radarChartPalette", + "radialChartPalette", + "horizontalBarChartPalette", +] as const; + +export type ChartPaletteKey = (typeof CHART_PALETTE_KEYS)[number]; + +export type ChartColorPalette = { [K in ChartPaletteKey]?: string[] }; /** * Color-related design tokens: surfaces, text, interactive states, borders, diff --git a/packages/react-ui/src/components/ThemeProvider/utils.ts b/packages/react-ui/src/components/ThemeProvider/utils.ts index 4dd47ca83..c05e785b9 100644 --- a/packages/react-ui/src/components/ThemeProvider/utils.ts +++ b/packages/react-ui/src/components/ThemeProvider/utils.ts @@ -1,5 +1,5 @@ import { defaultLightTheme } from "./defaultTheme"; -import { Theme } from "./types"; +import { CHART_PALETTE_KEYS, Theme } from "./types"; /** * Convert a camel case string to a kebab case string. @@ -27,6 +27,19 @@ export function themeToCssVars(theme: Record, prefix = "openui" .join("\n "); } +/** + * Every theme key accepted at runtime: all keys present in the default themes + * plus the chart palette keys, which are declared only on the type (a default + * palette would override the per-chart `theme` prop fallback in + * useChartPalette). `CHART_PALETTE_KEYS` in types.ts is the single source the + * `ChartColorPalette` type itself derives from. Shared by `createTheme()` and + * the ThemeProvider prop validator so the two allow-lists cannot drift apart. + */ +export const KNOWN_THEME_KEYS: ReadonlySet = new Set([ + ...Object.keys(defaultLightTheme), + ...CHART_PALETTE_KEYS, +]); + function levenshteinDistance(a: string, b: string): number { const m = a.length; const n = b.length; @@ -67,14 +80,13 @@ const _warnedKeys = new Set(); */ export function createTheme(theme: Theme): Theme { if (typeof process !== "undefined" && process.env?.["NODE_ENV"] !== "production") { - const knownKeys = Object.keys(defaultLightTheme); for (const key of Object.keys(theme)) { - if (knownKeys.includes(key) || _warnedKeys.has(key)) continue; + if (KNOWN_THEME_KEYS.has(key) || _warnedKeys.has(key)) continue; _warnedKeys.add(key); let suggestion = ""; let bestDist = Infinity; - for (const known of knownKeys) { + for (const known of KNOWN_THEME_KEYS) { const dist = levenshteinDistance(key, known); if (dist < bestDist) { bestDist = dist; diff --git a/packages/react-ui/tsconfig.test.json b/packages/react-ui/tsconfig.test.json new file mode 100644 index 000000000..60b3001d5 --- /dev/null +++ b/packages/react-ui/tsconfig.test.json @@ -0,0 +1,9 @@ +{ + "$schema": "https://json.schemastore.org/tsconfig", + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true + }, + "include": ["src/**/*"], + "exclude": ["dist", "node_modules"] +}