fix(core,cli,lint): close the figma brand-token loop — runtime CSS variables, --name, snippet lint#2053
fix(core,cli,lint): close the figma brand-token loop — runtime CSS variables, --name, snippet lint#2053vanceingalls wants to merge 3 commits into
Conversation
…riables, --name, snippet lint Brand-loop live test (SDS duplicate, plans/figma/brand-loop-test-plan.md) proved the recolor chain end-to-end and surfaced three gaps: - runtime now defines every declared composition variable as a CSS custom property (document root at init + scoped sub-comp hosts in the loader), so imported var(--slug, literal) fills resolve live — without this the frozen literal always won and variable-driven rebranding could not propagate. Slug kept byte-compatible with the figma importer (parity test). render --variables overrides win. - figma component --name: variant frames are often all named 'Platform=Desktop' and slug-collided across imports. - imported fragments carry data-hf-snippet and the project linter skips composition-root rules for them. - /figma skill documents the field-tested non-Enterprise tokens path (MCP get_variable_defs joined with REST boundVariables ids). Shared-helper extractions (injectScopedStyles, flattenedRoot module, parseHostVariableValues, rasterizeFallback, shapeCss) satisfy the dedup/complexity audit the runtime changes tripped. Validated live: brand-loop renders purple from the attribute alone (no manual :root); 118 figma + 662 runtime/compiler + 331 lint tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vanceingalls
force-pushed
the
vi/figma-loop-fixes
branch
from
6b746e7 to
def2765
Compare
|
Max-effort review found 15 distinct defects (13 CONFIRMED) — the documentElement-global injection design was wrong. Redesigned + pushed: Injection redesign (fixes the systemic cluster):
Other confirmed fixes:
Re-validated live: the brand-loop comp still renders purple under the scoped design; 118 figma + 756 runtime/compiler + 333 lint tests green. 🤖 Generated with Claude Code |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Review: LGTM with nits ✅
Solid PR that correctly closes the brand-token loop with well-factored shared plumbing. The SSOT extractions are the right call.
SSOT audit — clean
tokenSlug.ts—slugifyandcssVariableNameunified in one place. No duplication between bundler and runtime. ✓flattenedRoot.ts—FLATTENED_INNER_ROOT_STRIP_ATTRSandmarkFlattenedInnerRootcorrectly factored out. ✓parseHostVariableValues— shared fromgetVariables.ts, used by both runtime init and composition loader. ✓- CSS variable injection chain — runtime init → element-scoped properties, bundler → compile-time stylesheet rules, loader → per-instance host properties. Define-if-absent with override-always-wins correctly implemented in both paths. ✓
No SSOT violations found.
Verified
- Runtime variable injection:
injectCompositionCssVariablescorrectly walks[data-composition-variables]elements, reads declared defaults, and sets them as CSS custom properties on scoped elements. Override precedence (render overrides > host attributes > declared defaults) is correct. - Bundler emission:
emitRootCompositionVariableStylesemits:rootrules for documentElement defaults and[data-composition-id]rules for scoped compositions. Correct. --nameflag: Solves the SDS variant overwrite problem (multiple "Platform=Desktop" frames).- Snippet lint exemption:
isSnippetFragmentcorrectly detects sub-composition fragments undercompositions/and skips root-level composition rules. Regex correctly skips comments and doctypes. - Test coverage: Slug parity, scoped injection, override precedence, clear/apply lifecycle, snippet lint exemption all covered.
Nits (non-blocking)
-
readRenderOverrideswrapper (getVariables.ts:183-185) — Trivial re-export ofreadOverrideswith no added information. Making the private function public would be cleaner than the one-line wrapper. -
Double-read of
documentElement(getVariables.ts:30-34) —readDeclaredDefaults(document.documentElement)is called first, then thequerySelectorAllloop matches<html>again. Functionally idempotent but inconsistent withinjectCompositionCssVariableswhich uses aSetto deduplicate. -
Mid-file import (
getVariables.ts:68) —import { cssVariableName, detectSlugCollisions }sits afterreadDeclaredDefaultsrather than at the top with other imports. ESM hoists it fine, but breaks the top-of-file convention.
Ship it.
— Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
One blocker before this can be stamped: CodeQL is red on the current head.
blocker: packages/core/src/tokenSlug.ts:14 — the required CodeQL check reports a high-severity js/polynomial-redos alert on the slug trim regex (/^-+|-+$/g). Per the CodeQL recommendation for polynomial ReDoS, this should be rewritten to remove the ambiguous repeated-regex path rather than suppressing the alert. A small non-regex trim loop for leading/trailing - would preserve behavior and avoid the security finding.
I also spot-checked the main behavior Miga called out: the CSS-variable injection path is properly centralized through tokenSlug.ts, readDeclaredDefaults/parseHostVariableValues, injectCompositionCssVariables, and the compile-time bundler emission; the --name plumbing reaches nodeToHtml({ rootName }); and the snippet lint exemption is root-anchored. No additional blocking code issue from that pass.
I cannot approve while a required security check is failing.
Verdict: REQUEST CHANGES
Reasoning: The implementation shape looks sound, but the current head has a required CodeQL failure in changed code, so it needs the slug regex fix and green CI before stamp.
- Magi
…t eval time Live testing of the compile-time variable emission surfaced four gaps: - The producer render path never emitted the compile-time stylesheet (only the preview bundler did), so eval-time reads — GSAP .from immediateRender, top-level getComputedStyle — saw undefined vars in rendered output. The producer's inlineSubCompositions now calls the shared emitRootCompositionVariableStyles and passes the variable hooks. - --variables overrides weren't visible at eval time. They now thread from the orchestrator / distributed plan through compileStage into the emitted rules (window.__hfVariables still covers script reads). - Per-declarer rules anchored on data-composition-id, which two inlined instances of one sub-composition share — instance A's rule restyled instance B, and a rule directly on the declarer defeated the host's inherited data-variable-values. Rules now anchor on per-instance data-hf-var-scope markers and layer nearest-host values over declared defaults, mirroring the runtime loader. - Emission ignored authored CSS; a declared default now yields to a var already defined in an authored <style> block (define-if-absent, matching the runtime injection). Also: the figma importer emits background-color (longhand) for solid fills. GSAP backgroundColor tweens cannot read a var() through the background shorthand — its pending-substitution longhands serialize empty, so .from captured nothing and settled on transparent (pre-existing GSAP interaction, reproduced with no composition variables involved). Validated live: eval-time default + override, .from + override, two-instance host branding, authored :root precedence, SDS brand-loop pixel parity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Live end-to-end validation of the redesign surfaced four render-path defects, all fixed in e2c88ef:
Also found a pre-existing GSAP interaction (reproduces with zero composition variables): Validation matrix (all real CLI renders, pixel-probed): eval-time declared default ✅, eval-time |
…leanups - slugify: replace the anchored alternated trim regex (/^-+|-+$/g) with a character-scan trim — CodeQL js/polynomial-redos blocker. - readRenderOverrides: fold the readOverrides wrapper into the exported function (one name, no pass-through). - getVariables: deduplicate declarers with a Set, matching injectCompositionCssVariables. - Move the tokenSlug import to the top of the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All feedback addressed in d9368ec: @miguel-heygen blocker — @miga-heygen nits — all three taken:
|
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review at d9368ec: LGTM, blocker cleared.
Verified delta since my def27652 block:
packages/core/src/tokenSlug.ts:10rewrites the slug trim as a character scan; CodeQL is green now.packages/core/src/runtime/getVariables.ts:22has the import at the top, uses aSetfor declarer dedupe, and exportsreadRenderOverridesdirectly.packages/core/src/compiler/htmlBundler.ts:1025now exports the shared compile-time variable stylesheet emitter and anchors declarer rules on per-instancedata-hf-var-scope.- Producer render path threads
variablesthroughrenderOrchestrator.ts:1285→compileStage.ts:128→htmlCompiler.ts:1647, and distributed planning passesconfig.variablesatplan.ts:796.
I did not run local tests because this host is out of disk, but GitHub CI is green: CodeQL, Windows, and all 8 regression shards pass.
Verdict: APPROVE
Reasoning: The CodeQL blocker that caused my CHANGES_REQUESTED review is fixed, the new producer/render variable plumbing is covered by focused tests and CI, and I found no remaining blocker.
— Magi
4 participants
What
The brand-loop live test (SDS community duplicate; full log in the figma test-plan doc) ran the whole lifecycle — pull tokens → import frames → render → rebrand in Figma → re-sync — and proved the
var()chain end-to-end, surfacing three gaps. All fixed here:1. Runtime CSS custom properties (the missing half of the recolor loop)
Imported components reference brand tokens as
var(--slug, literal), but nothing ever defined those custom properties — the frozen literal fallback always won, so changing a composition variable could never recolor anything. The runtime now defines every declared composition variable as a CSS custom property:data-composition-variableson<html>or any composition element — both authoring conventions exist)render --variables '<json>'overrides win over declared defaultsThe slug is kept byte-compatible with the figma importer's (
slugifyparity test), since a drifted slug silently breaks the chain.Validated live: the brand-loop comp renders purple from the attribute alone — no hand-written
:rootblock — after flipping one variable default. Green-probe comp confirms injection in the real render engine.2.
figma component --nameSDS-style variant frames are all named
Platform=Desktop— three imports produced three silent slug-collision overwrites.--nameoverrides the component name.3.
data-hf-snippetlint exemptionImported fragments under
compositions/trippedroot_missing_composition_id/root_missing_dimensions— they're mountable fragments, not standalone compositions. The importer marks the fragment root and the project linter skips composition-root rules for marked files (asset-reference checks still apply).4. Skill: non-Enterprise tokens path
Documents the field-tested MCP-assisted flow:
get_variable_defs(name→value, not Enterprise-gated) joined with RESTboundVariablesids per node/property → binding rows + composition variables, everything downstream shipped machinery.Shared-helper extractions (
injectScopedStyles,flattenedRoot,parseHostVariableValues,rasterizeFallback,shapeCss) satisfy the dedup/complexity audit the runtime changes tripped — no behavior change.Testing
--name, snippet lint skip🤖 Generated with Claude Code