feat(diagrams): let each diagram name its own roles, and lock the door - #12648
Conversation
|
This pull request is part of a Mergify stack:
|
Merge Protections🔴 3 of 7 protections blocking · waiting on 👀 reviews and ⛓️ dependency
🔴 ⛓️ Depends-On RequirementsWaiting for
This rule is failing.Requirement based on the presence of
🔴 👀 Review RequirementsWaiting for
This rule is failing.
🔴 🔎 ReviewsWaiting for
This rule is failing.
Show 4 satisfied protections🟢 🤖 Continuous Integration
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
There was a problem hiding this comment.
🟡 Changes recommended
The new diagram-token scanner’s documented/declared defaults are inconsistent (and currently miss src/util/diagramSvg.ts), weakening the intended CI enforcement until corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes the diagram theming migration by removing role inference from legacy colors and making each Graphviz diagram explicitly declare semantic roles via DOT class attributes, with a new CI check to prevent reintroducing hardcoded colors.
Changes:
- Refactors Graphviz rendering to inject layout “kinds” (
queue/flow/arch) and rely on authoredclassroles rather than mapping colors. - Updates diagram CSS to avoid cluster-label overflow and to give
archdiagrams full-column width. - Rewrites multiple docs diagrams to use semantic roles and adds a deterministic
check:diagram-tokensscanner + CI job + tests.
File summaries
| File | Description |
|---|---|
| src/util/diagramSvg.ts | Removes transitional paint-based role inference; keeps only structural SVG cleanup and plain caption detection. |
| src/styles/index.css | Adjusts Graphviz label styling and adds full-width rule for .dg.arch. |
| src/content/docs/merge-queue/two-step.mdx | Converts diagram from hardcoded styling to semantic class roles and flow kind. |
| src/content/docs/merge-queue/stacks.mdx | Replaces color-based diagrams with queue kind + role classes. |
| src/content/docs/merge-queue/scopes.mdx | Updates scope-selection diagram to role classes and batch container styling. |
| src/content/docs/merge-queue/queue-modes.mdx | Converts multiple queue-mode diagrams to queue/flow kinds and role classes. |
| src/content/docs/merge-queue/performance.mdx | Updates performance diagram to role classes and external CI node styling. |
| src/content/docs/merge-queue/lifecycle.mdx | Rewrites lifecycle diagram with semantic roles and improved edge labeling. |
| src/content/docs/merge-queue/direct-merge.mdx | Updates direct-merge decision diagram to semantic roles. |
| src/content/docs/merge-queue/batches.mdx | Converts batching diagrams to queue kind and role classes. |
| src/content/docs/integrations/gha.mdx | Updates GitHub Actions scopes diagram to role classes and muted dashed edge. |
| src/content/docs/integrations/buildkite.mdx | Updates Buildkite scopes diagram to role classes and muted dashed edge. |
| src/content/docs/enterprise/architecture.mdx | Rewrites enterprise architecture diagram to semantic roles and arch kind. |
| src/content/docs/ci-insights/flaky-test-detection.mdx | Rebuilds flaky test visualization using role classes (no embedded palette). |
| scripts/check-diagram-tokens.test.mjs | Adds unit tests and a repo-level assertion that docs/components contain no diagram colors. |
| scripts/check-diagram-tokens.mjs | Introduces a scanner for color attributes/hex literals inside Graphviz fences and specific components. |
| plugins/remark-graphviz.ts | Removes legacy color-to-role migration table; adds kind-based layout injection guarded against prototype keys. |
| plugins/remark-graphviz.test.ts | Updates tests to assert class passthrough, no role invention, and kind layout differences. |
| package.json | Adds check:diagram-tokens script entry. |
| .github/workflows/ci.yaml | Adds a dedicated diagram-tokens CI job to enforce token-based diagram styling. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The migration shim recolored the diagrams without touching them, which was the point of landing it first — but a table that infers intent from a hex can only be as good as the colors it reads, and those colors were four dialects that disagreed. Two datastores came out the same color because both were pastel greens; "tests passed" and "merged to main" fused because both were greens. Each fence now says what it means, and the inference goes away. The 19 fences drop from 713 lines of DOT to 286, because most of what they said was color. What is left is structure, plus a role per element and a layout kind per fence — `queue`, `flow` or `arch`, which set direction and spacing and nothing else. A few things the diagrams had wrong get fixed on the way through, since the rewrite makes them visible: batches.mdx had a pull request that was grey rather than queued only because it was declared implicitly by an edge and so missed the fill every other node got. The emoji come out of the lifecycle and two-step diagrams. They render differently on every platform, Graphviz cannot measure them, and they carried no meaning that the color does not now carry. `pnpm check:diagram-tokens` is what makes this permanent — the same shape as `check:internal-leaks`, a deterministic scan wired into CI. Nothing about a hardcoded color fails a build on its own: the diagram renders, it just renders wrong on half the site, which is exactly how 63 of them accumulated unnoticed. It reads inside Graphviz fences only, so a color in prose or a CSS example is untouched, and it reads the three hand-drawn diagram components end to end, since those have no fence to scan and are where two of the four dialects lived. A CI job that nothing waits on is decoration, so `diagram-tokens` joins the `CheckRuns` anchor in `.mergify.yml` — the list the merge protection, both queue rules and the review-request rule all share. Note this leaves `actionlint`, `config-examples` and `internal-leaks` outside that anchor, which they were before this change; whether they belong there is a separate question from this diff. `enterprise/architecture.mdx` keeps its click-to-zoom overlay. The diagram is 945pt now rather than 1736pt, which is what makes it legible at all, but at 80% of the prose column that still scales its labels to roughly 8px — so the overlay is still doing something. Change-Id: I6cfa48c833499804a082a59bf6199236172cdcca
8e3f6a0 to
f82599e
Compare
Revision history
|
|
Re-pushed to address the review: 8e3f6a0 → f82599e (compare).
The commit message picked up a paragraph on the merge-protection change, so the PR body moved with it.
|
The migration shim recolored the diagrams without touching them, which was the
point of landing it first — but a table that infers intent from a hex can only
be as good as the colors it reads, and those colors were four dialects that
disagreed. Two datastores came out the same color because both were pastel
greens; "tests passed" and "merged to main" fused because both were greens.
Each fence now says what it means, and the inference goes away.
The 19 fences drop from 713 lines of DOT to 286, because most of what they said
was color. What is left is structure, plus a role per element and a layout kind
per fence —
queue,floworarch, which set direction and spacing andnothing else. A few things the diagrams had wrong get fixed on the way through,
since the rewrite makes them visible: batches.mdx had a pull request that was
grey rather than queued only because it was declared implicitly by an edge and
so missed the fill every other node got.
The emoji come out of the lifecycle and two-step diagrams. They render
differently on every platform, Graphviz cannot measure them, and they carried no
meaning that the color does not now carry.
pnpm check:diagram-tokensis what makes this permanent — the same shape ascheck:internal-leaks, a deterministic scan wired into CI. Nothing about ahardcoded color fails a build on its own: the diagram renders, it just renders
wrong on half the site, which is exactly how 63 of them accumulated unnoticed.
It reads inside Graphviz fences only, so a color in prose or a CSS example is
untouched, and it reads the three hand-drawn diagram components end to end,
since those have no fence to scan and are where two of the four dialects lived.
A CI job that nothing waits on is decoration, so
diagram-tokensjoins theCheckRunsanchor in.mergify.yml— the list the merge protection, both queuerules and the review-request rule all share. Note this leaves
actionlint,config-examplesandinternal-leaksoutside that anchor, which they werebefore this change; whether they belong there is a separate question from this
diff.
enterprise/architecture.mdxkeeps its click-to-zoom overlay. The diagram is945pt now rather than 1736pt, which is what makes it legible at all, but at 80%
of the prose column that still scales its labels to roughly 8px — so the overlay
is still doing something.