From cd9431700272c621c7a55e6f0a1342f937ab165d Mon Sep 17 00:00:00 2001 From: ankit-thesys Date: Sat, 4 Jul 2026 06:34:50 +0530 Subject: [PATCH] fix(react-ui): stop false "unknown key" warnings for chart palette theme keys The Theme type declares eight *ChartPalette keys, but both theme validators derived their allow-list from Object.keys(defaultLightTheme), which defines none of them - so the typed chart-theming API warned "unknown key ... It will be ignored" on every dev boot (twice per key), even though the palettes flow through the merge and charts apply them. Both validators now share one allow-list (default theme keys + a CHART_PALETTE_KEYS constant guarded by `satisfies Record`), so type/runtime drift is a compile error. Regression tests cover the allow-list, warning behavior, and palette flow. Fixes #714 Co-authored-by: Cursor --- .../src/components/ThemeProvider/AGENTS.md | 18 ++- .../ThemeProvider/ThemeProvider.tsx | 6 +- .../__tests__/ThemeProvider.test.tsx | 138 ++++++++++++++++++ .../src/components/ThemeProvider/utils.ts | 37 ++++- packages/react-ui/tsconfig.test.json | 9 ++ 5 files changed, 192 insertions(+), 16 deletions(-) create mode 100644 packages/react-ui/src/components/ThemeProvider/__tests__/ThemeProvider.test.tsx create mode 100644 packages/react-ui/tsconfig.test.json diff --git a/packages/react-ui/src/components/ThemeProvider/AGENTS.md b/packages/react-ui/src/components/ThemeProvider/AGENTS.md index 071f26ae3..f05866ed2 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` / `CHART_PALETTE_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` (`utils.ts`). A new key on `ChartColorPalette` must also be added to `CHART_PALETTE_KEY_MAP` in `utils.ts` — the `satisfies Record` guard turns any drift into a compile error. 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..422151e2f --- /dev/null +++ b/packages/react-ui/src/components/ThemeProvider/__tests__/ThemeProvider.test.tsx @@ -0,0 +1,138 @@ +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, 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/utils.ts b/packages/react-ui/src/components/ThemeProvider/utils.ts index 4dd47ca83..94f7af7a1 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 { ChartColorPalette, Theme } from "./types"; /** * Convert a camel case string to a kebab case string. @@ -27,6 +27,36 @@ export function themeToCssVars(theme: Record, prefix = "openui" .join("\n "); } +// Chart palettes are declared on the Theme type but intentionally have no +// entries in the default themes (a default palette would override the +// per-chart `theme` prop fallback in useChartPalette). The validators' +// allow-list is derived from Object.keys(defaultLightTheme), so these keys +// must be added explicitly. The `satisfies` map makes any drift between +// ChartColorPalette and this list a compile error. +const CHART_PALETTE_KEY_MAP = { + defaultChartPalette: true, + barChartPalette: true, + lineChartPalette: true, + areaChartPalette: true, + pieChartPalette: true, + radarChartPalette: true, + radialChartPalette: true, + horizontalBarChartPalette: true, +} as const satisfies Record; + +export const CHART_PALETTE_KEYS = Object.keys(CHART_PALETTE_KEY_MAP) as (keyof ChartColorPalette)[]; + +/** + * Every theme key accepted at runtime: all keys present in the default themes + * plus the chart palette keys that exist only on the type. 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 +97,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"] +}