Conversation
…ipt for token leaf
There was a problem hiding this comment.
Pull request overview
Adds a JSON Schema + AJV-based validator to enforce token leaf metadata requirements, and begins updating token docs (starting with space) to satisfy the new schema.
Changes:
- Added
schema/token.schema.jsondescribing the expected Cedar token file structure and requireddocsmetadata. - Added
style-dictionary/validate-schema.ts(AJV) and invoked validation from the Style Dictionary build script. - Expanded
tokens/global/space.jsondocs.descriptioncontent (including structured{ what, when, alternatives }).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tokens/global/space.json | Adds structured docs.description metadata across many space tokens. |
| style-dictionary/validate-schema.ts | New AJV-based schema validation utility for token JSON files. |
| style-dictionary/build.ts | Runs schema validation prior to Style Dictionary processing; adjusts import style to double-quote formatting. |
| schema/token.schema.json | New JSON Schema defining token group/leaf structure and required metadata. |
| package.json | Adds prebuild validator script and introduces ajv dependency. |
| pnpm-lock.yaml | Locks ajv@8.18.0 and related lockfile updates. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "build:tokens": "pnpm run clean && tsx ./style-dictionary/build.ts", | ||
| "clean": "rimraf dist/", | ||
| "prebuild": "tsx style-dictionary/validate-schema.ts", | ||
| "build": "pnpm run clean && pnpm run build:tokens && pnpm run site-tokens && pnpm run validate", | ||
| "build:grid": "tsx grid.ts", |
There was a problem hiding this comment.
pnpm run build will run prebuild (schema validation) and then build:tokens, whose build.ts also runs validateTokenSchema() at module load. This duplicates the validation work and doubles the output. Consider keeping validation in one place (either the npm lifecycle hook or the build script) to avoid redundant runs.
| import { register } from "@tokens-studio/sd-transforms"; | ||
| import { PLATFORMS, THEMES } from "./constants"; | ||
| import { getConfig } from "./configs"; | ||
| import { validateTokenSchema } from "./validate-schema.js"; |
There was a problem hiding this comment.
This import uses a .js extension even though the module is authored as TypeScript (style-dictionary/validate-schema.ts) and the rest of this folder’s relative imports are extensionless. This inconsistency can be brittle across runners/tooling (and makes grepping/refactors harder). Prefer using the same extensionless pattern as the other imports (or consistently using .js everywhere if that’s the intended ESM convention).
| import { validateTokenSchema } from "./validate-schema.js"; | |
| import { validateTokenSchema } from "./validate-schema"; |
| const targetFiles = files.filter( | ||
| (f) => !path.basename(f).startsWith("deprecated-"), | ||
| ); | ||
|
|
There was a problem hiding this comment.
The comment says _options files are excluded, but the filter only excludes basenames starting with deprecated-. If validateTokenSchema is ever called with a broader glob (e.g. tokens/**/*.json), files under tokens/_options/ will still be validated and will fail this schema by design. Either implement the _options exclusion (directory/path-based) or update the comment to match the actual behavior.
| const targetFiles = files.filter( | |
| (f) => !path.basename(f).startsWith("deprecated-"), | |
| ); | |
| const targetFiles = files.filter((f) => { | |
| const base = path.basename(f); | |
| if (base.startsWith("deprecated-")) return false; | |
| const segments = path.normalize(f).split(path.sep); | |
| if (segments.includes("_options")) return false; | |
| return true; | |
| }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 27 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "docs": { | ||
| "type": "background", | ||
| "category": "color", | ||
| "example": "slide-default", | ||
| "description": { | ||
| "what": "Default slide background color", | ||
| "when": "Use for the resting state of slide surfaces." | ||
| } |
There was a problem hiding this comment.
In the hover token, the docs.example/docs.description describe the default/resting state (e.g., example slide-default, “Default slide background color”). This looks swapped with the default token below, and will surface incorrect documentation/output metadata. Please align the hover/default docs with their respective token keys.
| "default": { | ||
| "$value": "{options.color.warm-grey-010}", | ||
| "$type": "color" | ||
| "$type": "color", | ||
| "docs": { | ||
| "type": "background", | ||
| "category": "color", | ||
| "example": "slide-hover", | ||
| "description": { | ||
| "what": "Hover background color for slide surfaces", | ||
| "when": "Use when a slide component is hovered", | ||
| "alternatives": ["{color.background.slide.default}"] | ||
| } |
There was a problem hiding this comment.
In the default token, the docs currently describe the hover state (example slide-hover, “Hover background color…”). This appears swapped with the hover token above, which will mislabel tokens in generated artifacts/docs. Please swap/adjust these docs so default documents the default state and hover documents the hover state.
| "type": "background", | ||
| "category": "color", | ||
| "example": "slide-default", |
There was a problem hiding this comment.
docs.category is set to "color" here (and also on the default token). Elsewhere in the token set, color tokens consistently use docs.category: "colors" (e.g., tokens/global/color.json). Using a different category will create a new top-level group in dist/docsite/json/* and can be a breaking change for consumers. Consider using "colors" for consistency.
There was a problem hiding this comment.
colors should be singular color
| "docs": { | ||
| "type": "chip", | ||
| "description": "Background color for active selected chips" | ||
| "type": "background", | ||
| "category": "color", | ||
| "example": "chip-default-selected-active", | ||
| "description": { | ||
| "what": "Active background for selected chips", | ||
| "when": "Use when a chip is selected and active" | ||
| } |
There was a problem hiding this comment.
This token’s docs switch to category: "color" and type: "background", while the rest of the chip tokens in this file use docs.category: "colors" and docs.type: "chip". Because the docsite output is grouped by docs.category, this will split default-selected-active into a different top-level group and make chip tokens inconsistent. Recommend keeping category/type consistent with the rest of tokens/global/chip.json and only adding the missing required fields (example/description structure).
There was a problem hiding this comment.
in the filtering story we define the naming convention to be singular so color vs colors etc.
| console.error("\n[Cedar] Schema validation failed:"); | ||
| console.error(errors.join("\n")); | ||
| process.exit(1); |
There was a problem hiding this comment.
validateTokenSchema is exported but calls process.exit(1) on failure. That makes it hard to reuse/test (callers can’t catch/handle validation errors) and can unexpectedly terminate other scripts that import it. Consider throwing an Error (or returning a structured result) and letting the top-level build script decide whether to exit.
| console.error("\n[Cedar] Schema validation failed:"); | |
| console.error(errors.join("\n")); | |
| process.exit(1); | |
| const errorMessage = ["\n[Cedar] Schema validation failed:", errors.join("\n")].join( | |
| "\n", | |
| ); | |
| console.error(errorMessage); | |
| throw new Error(errorMessage); |
| "docs": { | ||
| "category": "sizing", | ||
| "type": "icon", | ||
| "example": "sizing" | ||
| "type": "size", | ||
| "category": "icon", | ||
| "example": "size-sm", | ||
| "description": { | ||
| "what": "Small icon size", | ||
| "when": "Use for dense UI", | ||
| "alternatives": ["{icon.size}"] | ||
| } |
There was a problem hiding this comment.
This change updates icon tokens from docs.category: "sizing" / docs.type: "icon" to category: "icon" / type: "size". Since the published docsite JSON is grouped by docs.category, this renames the top-level group (as seen in the generated dist/docsite/json/* diffs) and can be a breaking change for consumers. If the intent is only schema compliance, consider preserving the existing category/type values and just adding the missing required fields (example/description).
mhewson
left a comment
There was a problem hiding this comment.
take a look at that last set of doc update questions but this looks good to me
| "type": "background", | ||
| "category": "color", | ||
| "example": "slide-default", |
There was a problem hiding this comment.
colors should be singular color
Created schema and validation script for token leaf.
Files missing required metadata properties ("docs" properties related fields):