Repository navigation
refactor(illustrations): rewrite sync-illustrations as source → engine → sinks - #900
Merged
Merged
Conversation
Collaborator
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
adrienzheng-cb
previously approved these changes
Oct 2, 2026
cb-ekuersch
force-pushed
the
sync-illustrations-rewrite
branch
from
October 2, 2026 15:27
d8307ad to
5b17514
Compare
…ne -> sinks Replace the illustration sync script with a smaller design modelled on sync-icons: an IllustrationSource (Figma or in-memory), a pure runSync engine that reconciles against the manifest and plans per-sink changes, and Sinks (destinations) that build their files from pure artifact helpers. Outputs are byte-identical to the previous implementation; behaviour is verified against the real Figma library and a mock library exercising add/remove/rename/recolor. - Fix the legacy rename bug (files kept the old name while versionMap pointed at the new one), the gradient url(#) crash on --sync-all, the #abc -> #abcabc hex expansion and nondeterministic duplicate handling. - Manifest keyed by node id with a fixed field order; colors stored as the palette. - Sinks self-heal: missing files are detected via has() and restored on the next incremental run. - Move story generation to packages/web (web:generate-illustration-stories) and add the repo-level `yarn sync-illustrations` workflow that chains sync, story generation, branch, commit and push. - Drop prettier at sync time in favour of a small printer verified byte-for-byte against the committed generated files.
cb-ekuersch
force-pushed
the
sync-illustrations-rewrite
branch
3 times, most recently
from
October 2, 2026 15:49
bb2f530 to
e78b756
Compare
Theming only recognizes 6-digit hex, so an alpha hex, currentColor, rgba() or var() in a color attribute would ship with its light color in every variant without anything downstream noticing. optimizeSvg now rejects any color that is not #RRGGBB, none or url(#id) after svgo has converted names and rgb(); the engine wraps the error with the component and its Figma deep link and the run stops before any sink, manifest or version plan is touched. Validated against the production file: a --sync-all of all 1597 components produces zero rejections and identical output.
cb-ekuersch
force-pushed
the
sync-illustrations-rewrite
branch
from
October 2, 2026 15:49
e78b756 to
78266a0
Compare
…it in the sync docs
The palette and the operations on it (single-pass recolor, dark derivation, manifest serialization) were a bare array plus free functions in artifacts/paletteColors. They now live together in ColorPalette, which also builds the light-color lookup once. The CSS-variable rewrite is an implementation detail of IllustrationsPackageSink, expressed through recolor.
adrienzheng-cb
self-requested a review
October 2, 2026 17:55
adrienzheng-cb
previously approved these changes
Oct 2, 2026
…release branch into the shell The docsite stories depend only on each type's set of names, so they are a WebStoriesSink: has() is always true and apply rewrites every type's file from the full set. One sync run now updates every destination derived from Figma. A test renders the stories from the committed manifest and asserts they equal the committed files byte for byte; the only change to them is the header line. This reverts the earlier move of story generation to a web nx target chained by a repo-level script. The git workflow (fresh illustrations/YYYY-MM-DD branch, push on changes, abandon otherwise) lives in the shell as ReleaseBranch; --no-git and scratch runs skip it.
adrienzheng-cb
approved these changes
Oct 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed? Why?
packages/illustrations/scripts/sync-illustrationsis rewritten from scratch with a smaller design: source → engine → sinks.source/):IllustrationSourceanswers three questions — what is published, give me these SVGs, what is the palette.FigmaSourceis production;InMemorySourcelets the whole pipeline run inside jest.sync.ts):runSyncreconciles what the source reports withmanifest.json, plans each sink's changes (remove stale, write changed + missing, full set for indexes) and applies them. It knows nothing about Figma, file formats, theming, git or the CLI.sinks/): a sink is a destination, not a file format.IllustrationsPackageSinkownssrc/__generated__end to end (svg, png, cjs, esm, data, types) and derives its dark/CSS-variable variants itself from the light SVG + palette.WebStoriesSinkowns web's illustration stories. A second destination (another repo, a native asset catalog) is a new subclass plus one line inconfig.sinks; a ~20-lineMirrorSinkin the e2e test shows the shape.artifacts/): pure content helpers (PNG rasterization, module wrappers, data-file renderers, a prettier-style printer). They return content; sinks decide paths.ColorPalette(colorPalette.ts): a value object for the 15illustration/*variables read from the colors Figma file. The light SVG is the canonical artifact; the palette'srecolor(lightSvg, fn)is the single-pass substitution every other variant is derived with (toDarkSvgfor dark values; the package sink passesvar(--<prefix>-<name>)for its themeable files, so the CSS-variable naming stays a detail of that one sink).toRecord()is what the manifest stores.index.ts,git.ts): the nx target — config, CLI args, manifest, version plan, summary, and the release branch. The only module with side effects on the repository.Behavioural fixes that fell out of the rewrite (all covered by tests):
renameFnpattern never matched real paths, so renamed illustrations kept their old filenames whileversionMappointed at the new name → CDN 404. Reproduced on the mock library (mockTwoFactor → mockPasskey), fixed.--sync-allcrashed onurl(#gradient)fills;#abcwas expanded to#abcabc; duplicate component names were resolved nondeterministically; a palette fetch failure silently produced light-only output. All fixed (palette failure is now fatal).has()detects missing files and the next incremental run restores them, so adding a sink or wiping a directory needs no--sync-all.manifest.jsonis regenerated: keyed by node id with a fixed field order (a rename diffs as one modified entry instead of a remove + add),colorsnow stores the palette rather than the legacyColorStyleobjects. All 1,596 existing entries match HEAD field for field;lastUpdatedis preserved.Design rationale, data flow and testing strategy:
packages/illustrations/scripts/sync-illustrations/README.md. Operations:packages/illustrations/DOCS.md.How equivalence with
masterwas guaranteedThe whole point of the rewrite is that nothing downstream notices. Five layers, from cheapest to most faithful:
GET /components,GET /imagesandGET /variablesresponses for the production file were recorded into__tests__/__fixtures__/, alongside the files the old script produced from them (expected-spotIcon-2fa-1-{light,dark}.svg,expected-*-themeable.{cjs,esm}.js, hashes from the committed manifest). Unit tests assert the new code reproduces those bytes and hashes exactly, including the manifest hash formula.data/*.ts+types/*.tsfiles (names, versionMap, svgJsMap, svgEsmMap, name unions) and prettier-canonical ondescriptionMap.ts(whose ordering the old script left nondeterministic).masterscript and the rewrite were run with--sync-allagainst the production Figma file (read-only) into scratch directories and diffed: 12,813 generated files byte-identical,descriptionMap.tsdiffers only in ordering (semantically identical), and all 1,597 manifest entries match field for field exceptwidth/heighton 207 items where the rewrite records the viewBox size instead of the Figma frame size (unused by consumers; drops agetFileNodesround-trip).qtdIR0QTyK0NZcZoAeJmS8) mirroring the real organisation was created and both scripts run through two passes: a first publish of 5 components (70 files + version plan identical), then add / delete / rename / recolor / description change (rewrite correct on every case; this is where the legacy rename bug surfaced — the old output is the one with the defect). The final sink/artifact restructure was re-run against the same pass and compared to the earlier rewrite output: identical files and version plan, idempotent second run, wiped directory restored.feat: Publish illustrations 2026-10-01) landed on master while this branch was open, produced by the old script. Starting from the pre-feat: Publish illustrations 2026-10-01 #899 tree (master~1manifest +__generated__) the rewrite was run incrementally against the real file, exactly as a release would be: it downloaded the same 1 component, addedusdj, bumped nothing else, and its__generated__is byte-identical to what feat: Publish illustrations 2026-10-01 #899 committed (12,813 files incl. PNGs; onlydescriptionMap.tsordering differs), with identical manifest items, palette and version plan.On top of that,
sync.test.tsdrives the full pipeline in memory through: first publish, no-op, description-only change, artwork change (v0 → v1), rename, case-only rename (rejected), node re-created under the same name, duplicate name, deletion,--sync-all, sink added mid-sequence, wiped sink directory, idempotence. 88 tests, zerojest.mock.A note on
--sync-allagainst the committed tree: a full regeneration does differ from master in two expected ways, with either script. Many committed dark variants still carry an older dark palette (e.g. primary#588AF5vs today's#578BFA) because the incremental sync never rewrites unchanged illustrations, and four light SVGs pick up float jitter in Figma's export (23.802→23.803) without anupdated_atchange. Neither is reachable from an incremental run, which is why layer 5 is the one that matters for releases.Gradient support (the
svgoConfig.tschange in #899)#899 also patched the legacy
svgoConfig.tsso that gradient illustrations stop crashing the sync:url(#…)fills are skipped instead of throwing, andblack/whiteare mapped to hex. The rewrite'soptimizeSvgwas analysed against that change rather than ported from it:url(#…)fills:normalizeHexColorpasses through every non-#value (none,url(…),currentColor). Same result as the patch.convertColors(names2hex: true, part ofpreset-default) runs before the custom normalization plugin in both configs and already rewriteswhite→#fff→#FFFFFF. The explicitblack/whitetable in the patch is therefore never reached in practice; the rewrite does not need it. Any other CSS color name is handled the same way, where the legacy table would throw.stop-colorvalues in<linearGradient>are themed exactly like fills (#FFFFFF→var(--illustration-white)), and stops outside the palette are left alone.This is pinned by a new fixture: the raw Figma export of
usdj(the first gradient illustration — 6fill="white", 3stop-color="white",url(#paint0_linear_…)references,fill-opacity,mix-blend-mode) must optimize to the exact light SVG #899 committed and theme to the exact dark SVG. The replay in layer 5 confirmed the same for its PNGs and module files.Strict color check (new behaviour, fails the run)
Theming only recognizes 6-digit hex. Anything else in a color attribute — alpha hex (
#0052FF80),currentColor,rgba(),var()— would be published with its light color in every variant, and nothing downstream would notice (the hash, diff and version plan look normal; the PR diff is a one-line SVG; fixtures pin known exports). The legacy script threw on some of these and silently let alpha hex through.The rewrite now rejects any color attribute that is not
#RRGGBB,noneorurl(#id)after svgo'sconvertColors(so CSS names andrgb()are fine). It fails before any sink, manifest or version plan is touched, and the message names the component, the attribute/value, the fix and the node in Figma:Figma never exports these forms, so the check costs nothing on a healthy file. Validated against the current state of the production file: a
--sync-alldownloads and optimizes all 1,597 components with zero rejections, and its output is identical to the run before the check was added. Covered by unit tests for each rejected/accepted form and an e2e test asserting the exact message and that nothing was written.Sinks run in parallel
After the source's three calls (
/components,/variables,/images+ S3 downloads) nothing touches the network: optimization, hashing, diffing, palette swaps, PNG rasterization and the sinks' writes are CPU and local disk. The engine therefore plans every sink and applies them withPromise.all.Web stories are a sink; the command stays
yarn nx run illustrations:sync-illustrationsThe docsite stories (
packages/web/src/illustrations/__stories__/<Type>.stories.tsx) depend only on each type's set of names, so they are nowWebStoriesSink:has()is always true (nothing per illustration to restore, so it never triggers a download) andapplyrewrites every type's file from the full set, ignoringremove/write/palette. One sync run now updates every destination derived from Figma, with the same guarantee that nothing is written on failure. The legacygenerateStories.tsscript,illustrations:generate-stories/releasetargets and thechalk/prettierdevDeps are gone; the template is emitted already in prettier's style.Byte-for-byte: a test (
WebStoriesSink.test.ts) renders the five stories files from the committedmanifest.jsonand asserts they equal the committed files inpackages/web, so stories cannot drift from the manifest again. The only change to the committed stories is the header line (illustrations:generate-stories→illustrations:sync-illustrations).The git workflow (clean tree →
illustrations/YYYY-MM-DDfrom the origin default branch → sync → commitfeat: Publish illustrations YYYY-MM-DD→ push; abandon the branch on failure or no changes) lives in the shell asReleaseBranch(git.ts); the engine never touches git.--no-gitruns in place on the current branch, and scratch runs (SYNC_ILLUSTRATIONS_SCRATCH_DIR) never use git. An earlier revision of this PR had moved the stories to a web nx target chained by a repo-levelyarn sync-illustrationsscript; that is reverted,packages/webis untouched apart from the header line.Not yet exercised end to end because it pushes; master is currently in sync with Figma (see layer 5 above), so the first real run should be a no-op.
UI changes
None.
Testing
How has it been tested?
Testing instructions
Real-API scratch run against the mock library (needs
FIGMA_ACCESS_TOKEN; never touches the repo):cd packages/illustrations SYNC_ILLUSTRATIONS_SCRATCH_DIR=/tmp/sync SYNC_ILLUSTRATIONS_FIGMA_FILE_ID=qtdIR0QTyK0NZcZoAeJmS8 \ npx tsx scripts/sync-illustrations/index.ts --sync-allIllustrations/Icons Checklist
Change management
type=routine
risk=low
impact=sev5
automerge=false
Generated with Toshi