From 487887a310490b279e94c2bba87551ab27043724 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Sun, 15 Nov 2026 03:11:31 +0900 Subject: [PATCH] fix(sheet): reject theme tokens of different kinds sharing a variable Token names were checked for clashes only within one kind, so a color `card` and a shadow `card` both declared `--card` and one silently replaced the other in `:root`. Deserializing the theme now checks colors, `length` and `shadow` together and reports the two tokens and the variable. The atom identity fixture that gave a length and a shadow the same name now names the shadow `lift`. Co-Authored-By: Claude Opus 5.5 --- ...changepack_log_theme_variable_clashes.json | 8 ++ .../app/(detail)/docs/devup/colors/page.mdx | 2 +- .../src/atom_identity_tests/first_value.rs | 4 +- libs/sheet/src/theme.rs | 92 ++++++++++++++++++- 4 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 .changepacks/changepack_log_theme_variable_clashes.json diff --git a/.changepacks/changepack_log_theme_variable_clashes.json b/.changepacks/changepack_log_theme_variable_clashes.json new file mode 100644 index 000000000..50dddbfd3 --- /dev/null +++ b/.changepacks/changepack_log_theme_variable_clashes.json @@ -0,0 +1,8 @@ +{ + "changes": { + "bindings/devup-ui-wasm/package.json": "Patch", + "bindings/devup-ui-binding/package.json": "Patch" + }, + "note": "Theme tokens of different kinds can no longer silently share a CSS variable: a color `card` and a shadow `card` both declared `--card`, so one replaced the other in `:root`. New build error: `theme: color token 'card' and shadow token 'card' both become the CSS variable --card: rename one`, raised when the theme is registered for any two of colors, `length` and `shadow` (names compared after dots become dashes, so `gray.100` and `gray-100` clash too)", + "date": "2026-11-15T00:00:00.000Z" +} diff --git a/apps/landing/src/app/(detail)/docs/devup/colors/page.mdx b/apps/landing/src/app/(detail)/docs/devup/colors/page.mdx index f62a0709a..69b421f58 100644 --- a/apps/landing/src/app/(detail)/docs/devup/colors/page.mdx +++ b/apps/landing/src/app/(detail)/docs/devup/colors/page.mdx @@ -116,7 +116,7 @@ Each variant also sets the page's native color scheme — form controls, scrollb } ``` -Token names start with a letter, a digit or `_` and continue with letters, digits, `-`, `_` and `.` (`primary`, `text-primary`, `gray.100`); a dot becomes a dash in the CSS variable, so `gray.100` is `var(--gray-100)`. The build reports a name outside these characters, and two tokens of a variant that become the same variable (`a-b` and `a.b`), instead of emitting CSS the browser would drop. +Token names start with a letter, a digit or `_` and continue with letters, digits, `-`, `_` and `.` (`primary`, `text-primary`, `gray.100`); a dot becomes a dash in the CSS variable, so `gray.100` is `var(--gray-100)`. The build reports a name outside these characters, and two tokens of a variant that become the same variable (`a-b` and `a.b`), instead of emitting CSS the browser would drop. Colors, `length` and `shadow` tokens share one namespace of CSS variables, so a color `card` and a shadow `card` (both `--card`) are a build error too: rename one. ## Semantic Color Naming diff --git a/libs/sheet/src/atom_identity_tests/first_value.rs b/libs/sheet/src/atom_identity_tests/first_value.rs index fb0fb592e..981206cfc 100644 --- a/libs/sheet/src/atom_identity_tests/first_value.rs +++ b/libs/sheet/src/atom_identity_tests/first_value.rs @@ -3,7 +3,7 @@ use super::*; fn token_theme(first: &str) -> Theme { serde_json::from_value(serde_json::json!({ "length": {"default": {"space": [first, "2px"], "zero": ["0px", "4px"], "nullable": [null, "3px", "4px"]}}, - "shadows": {"default": {"card": ["0 1px 2px black", "0 2px 4px black"], "space": ["0 3px 4px black", "0 4px 8px black"]}} + "shadows": {"default": {"card": ["0 1px 2px black", "0 2px 4px black"], "lift": ["0 3px 4px black", "0 4px 8px black"]}} })) .unwrap_or_else(|error| panic!("{error}")) } @@ -28,7 +28,7 @@ fn first_value_refs_resolve_without_aliasing_responsive_variables() { // Given let source = r#"import {Box} from '@devup-ui/react'; export const A=<> - + ;"#; for atom in [false, true] { let mut sheet = token_sheet(atom); diff --git a/libs/sheet/src/theme.rs b/libs/sheet/src/theme.rs index 6f50afebb..c558b4bfa 100644 --- a/libs/sheet/src/theme.rs +++ b/libs/sheet/src/theme.rs @@ -624,8 +624,10 @@ impl ColorScheme { } } +/// The theme of `devup.json`. Deserializing it checks the tokens of every +/// kind together, as the derived field-by-field reading cannot #[derive(Serialize, Deserialize, Debug)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", remote = "Self")] pub struct Theme { #[serde(default, deserialize_with = "deserialize_color_themes")] pub colors: BTreeMap, @@ -651,6 +653,58 @@ pub struct Theme { pub tailwind_css: String, } +impl Serialize for Theme { + fn serialize(&self, serializer: S) -> Result { + Theme::serialize(self, serializer) + } +} + +impl<'de> Deserialize<'de> for Theme { + fn deserialize>(deserializer: D) -> Result { + let theme = Theme::deserialize(deserializer)?; + check_token_kinds(&theme).map_err(serde::de::Error::custom)?; + Ok(theme) + } +} + +/// Check that no two tokens of different kinds become one CSS variable, as a +/// color `card` and a shadow `card` would both set `--card`. Tokens of one +/// kind are checked as they are read; the same name in two variants of one +/// kind is the same variable on purpose. +fn check_token_kinds(theme: &Theme) -> Result<(), String> { + let colors = theme.colors.values().flat_map(|variant| { + variant + .entries + .values() + .map(|entry| ("color", entry.interface_key.as_str(), entry.css_key.clone())) + }); + let mut seen: BTreeMap = BTreeMap::new(); + for (kind, name, css_key) in colors + .chain(token_variables("length", &theme.length)) + .chain(token_variables("shadow", &theme.shadows)) + { + let (other_kind, other) = *seen.entry(css_key.clone()).or_insert((kind, name)); + if other_kind != kind { + return Err(format!( + "theme: {other_kind} token '{other}' and {kind} token '{name}' both become the CSS variable --{css_key}: rename one" + )); + } + } + Ok(()) +} + +/// The kind, name and CSS variable of each token of every variant of `themes` +fn token_variables<'t>( + kind: &'static str, + themes: &'t BTreeMap>, +) -> impl Iterator + 't { + themes.values().flat_map(move |variant| { + variant + .keys() + .map(move |name| (kind, name.as_str(), css_key_from(name).into_owned())) + }) +} + /// Deserialize the color variants, naming the variant a token error is in fn deserialize_color_themes<'de, D>( deserializer: D, @@ -2734,6 +2788,42 @@ mod tests { } } + // A color `card` and a shadow `card` both set `--card`, so one would + // silently replace the other + #[rstest::rstest] + #[case( + r##"{"colors":{"default":{"card":"#111"}},"shadow":{"default":{"card":"0 1px #000"}}}"##, + "theme: color token 'card' and shadow token 'card' both become the CSS variable --card: rename one" + )] + #[case( + r##"{"colors":{"default":{"a":"#111"},"dark":{"gray":{"100":"#222"}}},"length":{"default":{"gray-100":"4px"}}}"##, + "theme: color token 'gray.100' and length token 'gray-100' both become the CSS variable --gray-100: rename one" + )] + #[case( + r#"{"length":{"default":{"gap":"4px"}},"shadow":{"sm":{"x":"0 1px #000"},"lg":{"gap.y":"0 1px #000","gap":"0 2px #000"}}}"#, + "theme: length token 'gap' and shadow token 'gap' both become the CSS variable --gap: rename one" + )] + fn test_tokens_of_different_kinds_cannot_share_a_variable( + #[case] json: &str, + #[case] error: &str, + ) { + assert!( + theme_error(json).starts_with(error), + "{}", + theme_error(json) + ); + } + + #[test] + fn test_one_token_in_variants_of_its_kind_shares_a_variable() { + let theme: Theme = serde_json::from_str( + r##"{"colors":{"default":{"card":"#111"},"dark":{"card":"#eee"}},"length":{"default":{"gap":"4px"},"wide":{"gap":"8px"}}}"##, + ) + .unwrap(); + let json = serde_json::to_string(&theme).unwrap(); + assert!(json.contains("\"length\""), "{json}"); + } + #[test] fn test_dotted_length_and_shadow_names_become_dashed_variables() { assert_eq!(