Repository navigation
feat(core): map design tokens to DESIGN.md fields - #490
wudilyy999 wants to merge 10 commits into
Conversation
The v4 classifier matched only `font-family`, so `--font-sans`, `--font-mono` and the other `--font-*` theme variables were dropped. `--font-weight-*` and companions such as `--font-sans--font-feature-settings` are not families and stay out. Refs OpenCoworkAI#484
A `:root` value and a `[data-theme="dark"]` value with the same name looked identical. DesignToken gets an optional `block`, set by the CSS variable extractor to the selector of a `[data-theme]` block and left unset for `:root`, so existing tokens and callers are unchanged. Refs OpenCoworkAI#484
cssColorToHex() handles hex, rgb(), hsl() and oklch(). A color with an alpha below 1 or in another format is reported, not approximated. oklch() goes OKLab -> linear sRGB -> gamma; a channel outside sRGB is clamped to [0, 1] and the result says so. The oklch test vectors were computed on a separate route (CSS Color 4 reference matrices through XYZ) and include the Tailwind v4 palette steps whose hex values Tailwind documents, in-gamut, out-of-gamut, achromatic and values a fraction of a step outside sRGB. Refs OpenCoworkAI#484
serializeDesignMd() writes the frontmatter fields validateDesignMd accepts in the canonical order, then the body sections in the canonical section order (the ordering formatDesignMdForPrompt already uses). Hex colors stay quoted strings and long values are not folded. It sits next to the parser because yaml is a dependency of packages/shared, not of packages/core. Refs OpenCoworkAI#484
mapTokensToDesignFields() turns scanned tokens into the fields validateDesignMd accepts: colors (sRGB hex), typography (font families), rounded and spacing. Font sizes, line heights and shadows have no frontmatter field and come back as Typography and Elevation & Depth sections. A var() is replaced with the scanned token it names, with a cycle guard and the CSS fallback. Nothing is logged. The result carries where every value came from and a structured report: clamped colors with their original value, values DESIGN.md cannot hold (alpha, other color formats, other units), unresolved var(), values left out because they sit in another theme block, and names already taken by an earlier value. Refs OpenCoworkAI#484
There was a problem hiding this comment.
Findings
-
[Minor]
DIMENSIONandUNITLESSreject valid CSS dimensions, so real tokens are dropped —packages/core/src/brand/designFields.ts:44-45. Both patterns require at least one digit before the decimal point and match units case-sensitively, so--radius: .5rem,--space-4: .5,--spacing: -.5remand--radius: 1REMare classified asunsupported/dimension. That outcome is reported rather than silently changed, so it is not a data-loss bug, but.5is a valid CSS<number>and CSS units are ASCII case-insensitive, so these are valid — and common in hand-written token files — so they will never import.packages/core/src/brand/color.ts:20already accepts a leading-dot mantissa for its own numbers, so the two modules disagree on what a number is.
Suggested fix: accept a leading-dot mantissa and match units case-insensitively in both regexes, then add matchingit.eachcases todesignFields.test.ts:const DIMENSION = /^-?(?:\d+(?:\.\d+)?|\.\d+)(?:px|em|rem)$/i; const UNITLESS = /^-?(?:\d+(?:\.\d+)?|\.\d+)$/;
-
[Nit]
serializeDesignMd()inpackages/shared/src/design-md.tscasts every key ofALLOWED_TOP_LEVELtokeyof DesignMdFrontmatter, so the two lists can drift silently: a name added toALLOWED_TOP_LEVELbut not to the interface compiles, readsundefined, andyamlthen omits it. IfALLOWED_TOP_LEVELis the canonical field set, derive the interface from it or add a type-level subset check so the cast cannot hide a mismatch.
Questions
- None; the scope matches the PR body (step 2 of 3,
Refs #484) and the diff stays inside it.
Summary
- Review mode: initial
- No blockers. The change is additive and well insulated: nothing calls
mapTokensToDesignFields/serializeDesignMdyet,blockis optional and unset on every existing token and caller, and the--font-*classifier change is covered by a new test without disturbing the existing extractor fixtures. - Constraints: no new dependencies (
yamlwas already apackages/shareddependency), noconsole.*, no provider SDK imports. Returning areportinstead of mutating values quietly matches the project rule against silent user-visible fallbacks. - Linked issue:
Refs #484is the correct link for a partial slice; the PR does not claim to close the issue, so there is nothing to validate against acceptance criteria. - Residual observations: the
:rootunder@media (prefers-color-scheme: dark)and.darkclass gaps are pre-existing extractor limits and are disclosed in the PR body; the optionalblockfield onDesignTokenV1keepsschemaVersion: 1and is backward compatible, so no migration is required. - Testing gap: only the dimension-format cases noted above are uncovered.
Testing
- Not run (automation). Vitest coverage in the diff is otherwise thorough:
color.test.tsvectors cross-checked against an independent conversion route,designFields.test.tscovering each report path plus an end-to-end Tailwind v4 stylesheet re-validated withvalidateDesignMd,design-md.test.tsfor serializer round-trip/ordering, and extractor regression tests.
Open-CoDesign Bot
CodeQL flagged the function pattern in cssColorToHex: the lazy body between two `\s*` can match whitespace in several ways, so `rgb(` followed by many tabs backtracks quadratically. The numeric pattern `\d+\.?\d*` has the same ambiguity on a long run of digits. The input is CSS from a folder the user picked, which can be someone else's repository. The body is now matched greedily and split afterwards, and the number pattern is `\d+(?:\.\d*)?`. Tests run 100,000-character inputs for each case.
A value that names the same token twice at every level takes 2^levels steps to resolve in full, so a few lines of CSS could stall the scan. A chain is now followed at most 32 levels deep and a resolved value may reach 4,096 characters; past either limit the token is reported as unresolved with the reason `too-complex`.
There was a problem hiding this comment.
Findings
- [Nit]
UnresolvedReasonis exported fromdesignFields.tsbut left out of both barrels, while its siblingUnsupportedReasonis re-exported —packages/core/src/brand/index.ts:3-10andpackages/core/src/index.ts:39-46. A caller that wants to switch onreport.unresolved[].reasoncannot name the union from the public entry point, and the asymmetry looks accidental.
Suggested fix: addUnresolvedReasonnext toUnsupportedReasonin bothexport type { ... }blocks.
Residual observations
- Carried over from the previous review and still open:
DIMENSION/UNITLESSatpackages/core/src/brand/designFields.ts:59-60still require a digit before the decimal point and match units case-sensitively, so--radius: .5rem,--space-4: .5and--radius: 1REMland inreport.unsupportedasdimension. This push did not touch it, so it is not re-filed as a new finding, but the previous fix still applies:/^-?(?:\d+(?:\.\d+)?|\.\d+)(?:px|em|rem)$/iplus the matching unitless form, withit.eachcases indesignFields.test.ts. - The previous Nit on
serializeDesignMdinpackages/shared/src/design-md.ts(casting everyALLOWED_TOP_LEVELkey tokeyof DesignMdFrontmatter) is likewise unchanged. - The new bounds in this push read correctly:
resolveVarsis depth-capped viachain.length > MAX_VAR_DEPTHand expansion-capped viaout.length > MAX_RESOLVED_LENGTH, the greedyFUNCTION_REhas no nested quantifiers, and the reworkedNUMBERalternation stays linear on the long-input cases. Thechainis copied per branch, so a repeated (non-cyclic) reference is not misreported as a cycle — consistent with the newMAX_VAR_DEPTH + 8and doubling-shadowtests. No new issue found in the added lines.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The delta since
86ccc5bis the ReDoS-hardening commit plus its tests; it changes no in-range behavior and regresses no previously reviewed path. The earlier Minor (dimension regex) and earlier Nit (serializeDesignMdkey cast) are simply untouched, not worsened. - Constraints: no new dependencies (
yamlwas already apackages/shareddep), noconsole.*, no provider SDK imports; the new constants are internal and the mapper stays pure, so thereport-instead-of-logging approach still matches the no-silent-fallback rule.Refs #484remains the correct partial link. - Nothing blocking; the PR is mergeable as step 2 with the residual items above.
Testing
- Not run (automation). Suggested:
pnpm --filter @open-codesign/core testandpnpm --filter @open-codesign/shared testlocally; the added tests look proportional to the new bounds.
Open-CoDesign Bot
`.5rem`, `-.5rem`, `.5` and `1REM` are valid CSS but were reported as unsupported dimensions, so hand-written token files never imported. The mapper now accepts them and writes the form validateDesignMd takes (a digit before the point, a lowercase unit): `0.5rem`, `-0.5rem`, 0.5, `1rem`. Widening the pattern alone would have written `.5rem`, which validateDesignMd rejects. The same length spelled two ways counts as one value. Refs OpenCoworkAI#484
…zer from one list serializeDesignMd cast every allowed field name to keyof DesignMdFrontmatter, so a name added to one list and not the other compiled and was dropped by yaml. The fields are now one `as const` list: the allowed set is built from it and the serializer indexes the frontmatter with it, so a name the type lacks is a compile error. A test writes a document with every field set. Refs OpenCoworkAI#484
A caller that switches on report.unresolved[].reason could not name the union from the public entry point. Refs OpenCoworkAI#484
|
Thanks. All three are fixed: One note on the dimension finding. The suggested patterns alone would make the mapper write The field names are now one |
There was a problem hiding this comment.
Findings
- No new Blocker, Major, Minor, or Nit findings in this push. The delta since
f90c579is exactly the three items the previous review left open, and all three are now addressed:- The
UnresolvedReasonbarrel gap is fixed in both entry points (packages/core/src/brand/index.ts:9,packages/core/src/index.ts:45), so the union is now nameable from the public API alongsideUnsupportedReason. DIMENSION/UNITLESSatpackages/core/src/brand/designFields.ts:59-60now accept a leading.and lowercase the unit, withnormalizeDimensioncanonicalizing beforeclaimdedupes;.5rem,-.5rem,1REM,.25EM,8PXand bare.5all map or number-convert instead of landing inreport.unsupported, and the two-spelling case (treats the same length spelled two ways as one value) no longer produces a spuriousshadowedentry.- The
serializeDesignMdkey cast is gone:FRONTMATTER_FIELDSis now anas consttuple andinput.frontmatter[key]indexes it without akeyofassert (packages/shared/src/design-md.ts:26-38,:434-437), whileALLOWED_TOP_LEVELkeeps the same 8 members, so validation behavior is unchanged.
- The
Residual observations (not filed)
resolveVarsmatches the literalvar(token (packages/core/src/brand/designFields.ts:126), so uppercaseVAR(--x)will not resolve; it surfaces asunresolved/unsupportedrather than a silent wrong value, and the same applies to+-signed lengths whichDIMENSION/UNITLESSdo not accept. Both are rare enough in real stylesheets that I did not raise them as findings.- The
text-*--line-heightcompanion regex/^text-(.+)--line-height$/is greedy, but it only derives a display key and any odd capture stays inside the generated section, so there is no data-loss path.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- All previously reported items (the
UnresolvedReasonexport Nit, the dimension-regex Minor, theserializeDesignMdkey-cast Nit) are resolved in this push, and the newnormalizeDimensionpath is covered by focusedit.eachcases plus avalidateDesignMdround-trip (packages/core/src/brand/designFields.test.ts:140-210). No behavior outside the newly accepted CSS spellings moved: the earlier extractor, serializer, field-name andvar()tests keep their expectations. - Constraints still hold: no new dependencies (
yamlwas already apackages/shareddependency and is reused viastringifyYaml), noconsole.*inpackages/core/packages/shared, no direct provider SDK imports, and the mapper stays pure and returnsreport/originsinstead of logging, so the no-silent-fallback rule is respected.blockis added as an optional zod field onDesignTokenV1(packages/shared/src/design-token.ts:19-23) with a default, so no persisted shape breaks.Refs #484is still the correct partial link, and the changeset covers core + shared atpatch. - Nothing blocking; the PR is mergeable as step 2 of #484.
Testing
- Not run (automation). Suggested:
pnpm --filter @open-codesign/core testandpnpm --filter @open-codesign/shared test; the new dimension-normalization and frontmatter-completeness tests are proportional to the change.
Open-CoDesign Bot
Summary
Second step of #484 (design tokens from a folder into
DESIGN.md), after #486. It turns the tokens the scan finds intoDESIGN.mdfields. Nothing calls it yet; the IPC, the preview with the per-field diff, and the write are step 3.This is larger than the 400 lines I estimated in #484: about 510 lines outside tests and 630 in tests. The five commits stand alone, and I can open them as separate PRs if you prefer.
--font-*fix (tailwindExtractor.ts, one rule). The v4 classifier matched onlyfont-family, so--font-sans,--font-monoand the like were dropped.--font-weight-*and companions such as--font-sans--font-feature-settingsstay out. None of the 45 existing extractor tests changes: the fixtures use--font-family-*and--font-size-*, which the old rule already classified.DesignToken.block, optional). The CSS variable extractor sets it to the selector of a[data-theme]block and leaves it unset for:root, so:roottokens,@themetokens, and every existing caller and test are unchanged. A[data-theme="dark"]value no longer looks like the default.cssColorToHex, matched in linear time: CodeQL flagged the first version's function pattern, fixed inf269488, and its number pattern had the same ambiguity). Hex,rgb(),hsl()andoklch()become lowercase sRGB hex.oklch()goes OKLab → linear sRGB → gamma; a channel outside sRGB is clamped to [0, 1] and the result says so. An alpha below 1, or any other format (color-mix(),lab(), named colors,transparent), is reported, not approximated.serializeDesignMdinpackages/shared, next to the parser becauseyamlis a dependency there). It writes the fields in the canonical order, then the body sections in the orderformatDesignMdForPromptalready uses;#fffstays a quoted string and long values are not folded.mapTokensToDesignFieldsin core, no new dependency). Colors →colors, font families →typography(fontFamily), radii →rounded, spacing →spacing. Font sizes, line heights and shadows have no frontmatter field, so they come back asTypographyandElevation & Depthsections. A length is accepted in any CSS spelling and written in the formvalidateDesignMdtakes (.5rem→0.5rem,1REM→1rem,.5→ 0.5). Names lose the prefix each extractor adds (colors.brand.500,color-brand-500andcolor.brand.500all becomebrand-500);--spacingand--radiusalone becomebase.What the result reports
Nothing is logged (core and shared have no
console.*, and the mapper has no logger). The result carriesorigins(the file and token behind every value, keyed likecolors.brand-500) and areport:clamped: path, the original value, and the hex it became;unsupported: alpha, another color format, a unit DESIGN.md does not take (50%,calc(),clamp()), or an untyped token;unresolved: avar()that names no scanned token, names itself, or is nested past the limits (32 levels, 4,096 characters), reasontoo-complex;otherThemes: values in a[data-theme]block, left out; the default block wins;shadowed: a name already taken by an earlier value; the first is kept. The same value again is ignored.var()A
var(--name, fallback)is replaced with the scanned token it names, recursively, with a cycle guard. The fallback is used only when no scanned token has that name. It resolves only against tokens the extractors return, not every custom property in the folder, sohsl(var(--hue) 50% 40%)is unresolved when--hueis a bare number the extractor does not classify. The limits exist because the folder can be someone else's repository: a value that names the same token twice at every level takes 2^levels steps to resolve in full.Known limits
:rootinside@media (prefers-color-scheme: dark)is read as a default block (the extractor ignores at-rules). The mapper keeps the first value and lists the other undershadowed, so nothing changes silently, but the label is "name already taken", not "dark theme"..darkclass blocks are not read at all. Both predate this PR; fixing them means the extractor tracking at-rules, which I would do separately.:rootthemes are listed, not imported, as agreed in [Feature]: Import a design system from a local codebase into DESIGN.md #484. There is no theme picker.Type of change
Linked issue
Refs #484
Checklist
pnpm lint && pnpm typecheck && pnpm testpasses locallypnpm changeset) if user-visiblePrinciples
blockis optional and absent on every token that has no theme variant; the only output that moves is that--font-*tokens now appear.yamland its section order.Checks
pnpm -r typecheck,pnpm lint,CI=1 pnpm test: all passed; core 709 tests, shared 334, desktop 181 files / 2,640 tests.color.test.ts(90): fixedoklch()vectors for in-gamut, out-of-gamut and achromatic input. The expected values were computed on a separate route (the CSS Color 4 reference matrices through XYZ), not the matrices used incolor.ts, and the Tailwind v4 palette steps match the hex values Tailwind documents (red-500#fb2c36, gray-500#6a7282, blue-50#eff6ff). Also hue units, percent forms,none, alpha in each notation, values a fraction of a step outside sRGB (reported only when a channel changes by a full step), and 100,000-character inputs that must be rejected, or converted, within 250 ms.designFields.test.ts(49): every type and name style, clamping and its report, each unsupported reason,var()chains, fallbacks, cycles and unresolved names, theme variants, repeated names, the nesting limits (a 40-level chain and a 20-level chain that doubles at each level), the CSS spellings of a length (each one also checked throughvalidateDesignMd), and a Tailwind v4 stylesheet (@theme inline,:root,[data-theme="dark"]) run through the real extractors, mapped, serialized and checked withvalidateDesignMd.design-md.test.ts(+7): the serialized document validates and parses back to the same fields, a document with every field set, canonical field and section order, hex stays a string, long values are not folded.