Skip to content

feat(c4): render elements in the c4model.com outline style - #7991

Open
filipsajdak wants to merge 1 commit into
mermaid-js:developfrom
filipsajdak:feature/c4-outline-theme
Open

feat(c4): render elements in the c4model.com outline style#7991
filipsajdak wants to merge 1 commit into
mermaid-js:developfrom
filipsajdak:feature/c4-outline-theme

Conversation

@filipsajdak

@filipsajdak filipsajdak commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Description

The C4 shape-migration PR #7842 originally introduced the c4model.com outline look (a light fill with the element's identity colour as the border and text). During review, @aloisklink pointed out that bundling that visual redesign into the shapes-migration PR made a breaking change to the existing C4 colour config and muddied the visual-regression review. So #7842 keeps the existing colours, and this PR carries the outline redesign on its own.

An element now sits on the theme's surface colour with its identity colour as the border and the label text. The identity colour is the element's <type>_bg_color palette entry, shifted until it reads against that surface - so a dark theme gets a light identity on a dark body rather than a dark one on a dark one. UpdateElementStyle's $bgColor, $borderColor and $fontColor are still emitted inline on the node and continue to take precedence.

8 files, +324/-42, one commit. Three of those files are tests and one is the changeset; the production change is 3 files, +59/-42.

The open question this PR used to carry is now answered

Earlier revisions hard-coded the fill to #ffffff and asked whether the colours should come from theme variables instead, deferring to #7868. Review said not to wait, which was right - but following it up showed that moving only the fill would have shipped a bug: the old ensureReadable clamped the identity colour darker, which is correct on a white fill and wrong the moment the fill follows the theme, because theme-dark's surface is dark. Darkening the identity colour into a dark background is the same unreadability the redesign is meant to remove, pointing the other way.

It also could not be fixed where it was: elementCssStyles only received the c4 config, so it had no way to know what surface it was drawing on.

So the palette-derived colours are emitted by the stylesheet, which is the one place with both getConfig().c4 and the theme variables, using the same CSSOM construction as the existing per-type font rules. readableOn() picks the shift direction by measuring contrast against the surface rather than assuming one, so light themes darken and dark themes lighten. Dark themes are supported here rather than deferred, and there is no hard-coded white left.

The fill uses options.background. mainBkg - the flowchart/state/er convention for a node fill - is #ECECFF in the default theme and #cde498 in forest, which would make an "outline" element lavender or olive; background is the only public theme variable that is near-white in every light theme and genuinely dark in theme-dark. Happy to switch if you would rather C4 match the flowchart convention exactly.

Two behaviour changes worth calling out

  • <type>_border_color no longer affects rendering. The border is the identity colour derived from <type>_bg_color. The 40 <type>_border_color config keys still exist and still validate, but setting one has no effect. There is no way to detect "the user customised this" at runtime, since config always carries a default, so this is documented in the changeset rather than handled in code. Whether those keys should be deprecated in the schema, or honoured as an explicit stroke override that wins over the derived colour, is a maintainer call - happy to add the override if you want the escape hatch.
  • Element bodies follow the theme instead of a solid palette fill, so existing C4 diagrams change appearance: a light fill with a coloured outline in light themes, a dark fill in dark themes, where they were previously a solid colour with white text.

Tests

  • c4Colors.spec.ts on readableOn: passthrough when a colour already reads, darkening on a light surface, lightening on a dark one, every palette colour reaching 4.5:1 on #ffffff/#f4f4f4/#333333, an unparseable value returned untouched, and the invariant that the result is never less readable than the input.
  • styles.spec.ts on the generated stylesheet: the body takes the surface colour, two palette entries produce two different identity colours, the identity colour is lighter on a dark surface than on a light one, and it is set on the group so the label inherits it.
  • C4.9, a default vs dark imgSnapshotTest. Worth noting no C4 Cypress spec set a theme before this, so Argos could not have shown a dark-theme problem at all.

argos will show the intended legacy-colours -> outline change plus the new dark-theme case.

Part of the C4 modernization tracked in #7849; the shape migration it builds on (#7842) has merged.

Summary

  • Render C4 elements in the c4model.com outline style.
  • Use theme-surface fills with readable, identity-colored borders and label text.
  • Preserve explicit element color overrides.
  • Add readableOn utilities for WCAG AA contrast.
  • Add C4 style tests and light/dark theme snapshot coverage.
  • Add a minor mermaid changeset.
  • Preserve intentional stereotype label routing from PR #7842.
  • Defer theme-variable color sourcing because it overlaps with PR #7868 and tracker #7849.

Validation

  • 126/126 C4 unit tests passed.
  • Mermaid package TypeScript check passed.

@netlify

netlify Bot commented Jul 23, 2026

Copy link
Copy Markdown

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit de8e745
🔍 Latest deploy log https://app.netlify.com/projects/mermaid-js/deploys/6a7cdef0b0ac3f00087c551d
😎 Deploy Preview https://deploy-preview-7991--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@changeset-bot

changeset-bot Bot commented Jul 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: de8e745

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mermaid Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c5e65c84-7b72-4cb2-9413-6154140f1d41

📥 Commits

Reviewing files that changed from the base of the PR and between 0d42c7a and 2db6952.

📒 Files selected for processing (1)
  • packages/mermaid/src/diagrams/c4/styles.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/mermaid/src/diagrams/c4/styles.spec.ts

📝 Walkthrough

Walkthrough

C4 outline elements now use theme surfaces for fills. Stylesheets generate contrast-adjusted identity colors for borders and labels. Explicit element color overrides remain authoritative. C4 rendering and snapshot coverage use the updated styling model.

Changes

C4 outline styling

Layer / File(s) Summary
Contrast-aware C4 colors
packages/mermaid/src/diagrams/c4/c4Colors.ts, packages/mermaid/src/diagrams/c4/c4Colors.spec.ts
Adds readableOn, which adjusts valid colors toward WCAG AA contrast and preserves invalid values. Tests cover light and dark surfaces, palette colors, and adjustment direction.
Theme-based stylesheet colors
packages/mermaid/src/diagrams/c4/styles.js, packages/mermaid/src/diagrams/c4/styles.spec.ts
Generates readable identity colors for C4 shape borders and inherited labels. Shape fills use theme surfaces and shared shape defaults.
Shape rendering integration
packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts, packages/mermaid/src/diagrams/c4/c4Renderer.ts, cypress/integration/rendering/c4/c4.spec.js, .changeset/c4-outline-theme.md
Removes palette styling from inline node construction, updates renderer calls, and adds default and dark theme snapshot coverage. The Changeset records the new outline behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Possibly related PRs

Suggested labels: Graph: C4

Suggested reviewers: aloisklink

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the primary change: rendering C4 elements in the c4model.com outline style.
Description check ✅ Passed The description explains the design, behavior changes, implementation decisions, tests, and changeset, but does not follow the template headings or include an issue reference.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pkg-pr-new

pkg-pr-new Bot commented Jul 23, 2026

Copy link
Copy Markdown

Open in StackBlitz

@mermaid-js/examples

npm i https://pkg.pr.new/@mermaid-js/examples@7991

mermaid

npm i https://pkg.pr.new/mermaid@7991

@mermaid-js/layout-elk

npm i https://pkg.pr.new/@mermaid-js/layout-elk@7991

@mermaid-js/layout-tidy-tree

npm i https://pkg.pr.new/@mermaid-js/layout-tidy-tree@7991

@mermaid-js/mermaid-zenuml

npm i https://pkg.pr.new/@mermaid-js/mermaid-zenuml@7991

@mermaid-js/parser

npm i https://pkg.pr.new/@mermaid-js/parser@7991

@mermaid-js/tiny

npm i https://pkg.pr.new/@mermaid-js/tiny@7991

commit: de8e745

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.38554% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.56%. Comparing base (19563d8) to head (de8e745).
⚠️ Report is 37 commits behind head on develop.

Files with missing lines Patch % Lines
packages/mermaid/src/diagrams/c4/styles.js 94.73% 2 Missing ⚠️
packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts 88.88% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #7991      +/-   ##
===========================================
- Coverage    77.61%   77.56%   -0.05%     
===========================================
  Files          567      573       +6     
  Lines        74906    75329     +423     
  Branches     14619    14703      +84     
===========================================
+ Hits         58141    58432     +291     
- Misses       15768    15893     +125     
- Partials       997     1004       +7     
Flag Coverage Δ
e2e 70.69% <91.48%> (+0.09%) ⬆️
unit 74.81% <92.77%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/mermaid/src/diagrams/c4/c4Colors.ts 100.00% <100.00%> (ø)
packages/mermaid/src/diagrams/c4/c4Renderer.ts 82.07% <100.00%> (ø)
packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts 91.12% <88.88%> (-2.72%) ⬇️
packages/mermaid/src/diagrams/c4/styles.js 97.75% <94.73%> (-2.25%) ⬇️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/mermaid/src/diagrams/c4/styles.js`:
- Around line 20-46: Update elementFontStyles to avoid constructing
CSSStyleSheet directly, using a document-created style element’s sheet instead
so it works in older browsers and jsdom. Insert and serialize the rules through
that sheet as currently done, then remove the temporary style element after
generating the returned CSS text while preserving the existing font-style
behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 452dc016-9bc7-4eea-965c-7f70b290a182

📥 Commits

Reviewing files that changed from the base of the PR and between ee0610f and caef72e.

📒 Files selected for processing (23)
  • .changeset/c4-outline-theme.md
  • .changeset/c4-unified-shapes.md
  • .changeset/person-shape.md
  • .cspell/mermaid-terms.txt
  • cypress/integration/rendering/c4/c4-characterization.spec.js
  • cypress/integration/rendering/c4/c4.spec.js
  • cypress/integration/rendering/newShapes.spec.ts
  • docs/config/setup/mermaid/interfaces/LayoutData.md
  • docs/syntax/c4.md
  • docs/syntax/flowchart.md
  • packages/mermaid/scripts/docs.spec.ts
  • packages/mermaid/src/diagrams/c4/c4Renderer.ts
  • packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts
  • packages/mermaid/src/diagrams/c4/styles.js
  • packages/mermaid/src/diagrams/c4/svgDraw.ts
  • packages/mermaid/src/docs/syntax/c4.md
  • packages/mermaid/src/rendering-util/rendering-elements/shapes.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/c4LabelHelper.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/cylinder.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/person.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/tiltedCylinder.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
  • packages/mermaid/src/rendering-util/types.ts

Comment thread packages/mermaid/src/diagrams/c4/styles.js
@argos-ci

argos-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 4 changed Aug 12, 2026, 9:18 PM

@filipsajdak
filipsajdak force-pushed the feature/c4-outline-theme branch from caef72e to 17f0e9f Compare July 30, 2026 12:59
@filipsajdak

Copy link
Copy Markdown
Contributor Author

Re-pointed onto the new #7842 base. #7842 was approved and its history tidied to 9 commits (38 before), and this branch was sitting on an older point of it - missing its last 5 commits - as well as CONFLICTING against develop. Both are now resolved.

The outline change is still the single commit it was; it 3-way merged onto the new base with no conflicts, and the delta is byte-identical to before (3 files, +35/-14):

.changeset/c4-outline-theme.md                     |  5 +++
packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts | 40 +++++++++++++++-------
packages/mermaid/src/diagrams/c4/styles.js         |  4 +--

Diffing the old delta against the new one produces no output, so rebasing changed nothing about what this PR proposes.

Local checks on the new tip: c4 unit suite 126/126, tsc clean for the mermaid package. Still stacked on #7842, so the diff here reduces to just the outline change once that merges.

For context, since this PR has not been reviewed yet: it reintroduces the c4model.com outline look (white fill, identity colour as border and text) that was deliberately reverted out of #7842 to keep the legacy element colour configuration working. The open question in the description still stands - whether these colours should come from theme variables instead, which overlaps #7868.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts (1)

18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reserve stereotype-based label routing for C4.

stereotype is on the shared BaseNode type, while only C4 adapters populate it; any diagram that adds it will bypass the standard label path (HTML labels, markdown, centerLabel, icon/img background). Add an explicit routing flag, or call c4LabelHelper directly from the C4 label paths.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts` around
lines 18 - 21, Restrict the stereotype-based routing in the visible
label-selection logic to C4 diagrams instead of checking the shared
BaseNode.stereotype field alone. Add or reuse an explicit C4 routing indicator,
or move the c4LabelHelper call into the C4-specific label paths, while
preserving the standard label handling for non-C4 nodes.
packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts (1)

154-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Hardcoded fill:#ffffff`` will read poorly on dark themes.

The outline fill is a literal white while everything else derives from config/theme. As already noted in the PR description, sourcing this (and the darkened identity colour) from theme variables would keep dark themes legible; worth coordinating with the theme-variable work rather than shipping the literal.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts` around lines 154 - 161,
The elementCssStyles function hardcodes a white fill, which is not theme-aware.
Replace the literal fill value with the appropriate configured/theme-derived
variable, and coordinate the identity color passed to ensureReadable so both
fill and darkened identity styling remain legible across light and dark themes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cypress/integration/rendering/c4/c4-characterization.spec.js`:
- Around line 265-268: Update the shape assertions in the C4 rendering test to
target the individual nodes by their labels instead of using aggregate .node
counts. Assert that the cylinder node contains its expected path structure and
the folder node contains the expected rectangle structure, preserving the
existing override behavior.

---

Nitpick comments:
In `@packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts`:
- Around line 154-161: The elementCssStyles function hardcodes a white fill,
which is not theme-aware. Replace the literal fill value with the appropriate
configured/theme-derived variable, and coordinate the identity color passed to
ensureReadable so both fill and darkened identity styling remain legible across
light and dark themes.

In `@packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts`:
- Around line 18-21: Restrict the stereotype-based routing in the visible
label-selection logic to C4 diagrams instead of checking the shared
BaseNode.stereotype field alone. Add or reuse an explicit C4 routing indicator,
or move the c4LabelHelper call into the C4-specific label paths, while
preserving the standard label handling for non-C4 nodes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 87037f4b-36c3-4ef7-98a3-9d4bb61e8ae0

📥 Commits

Reviewing files that changed from the base of the PR and between caef72e and 17f0e9f.

📒 Files selected for processing (24)
  • .changeset/c4-outline-theme.md
  • .changeset/c4-unified-shapes.md
  • .changeset/person-shape.md
  • .cspell/mermaid-terms.txt
  • cypress/integration/rendering/c4/c4-characterization.spec.js
  • cypress/integration/rendering/c4/c4.spec.js
  • cypress/integration/rendering/newShapes.spec.ts
  • docs/config/setup/mermaid/interfaces/LayoutData.md
  • docs/syntax/c4.md
  • docs/syntax/flowchart.md
  • packages/mermaid/scripts/docs.spec.ts
  • packages/mermaid/src/diagrams/c4/c4Renderer.ts
  • packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts
  • packages/mermaid/src/diagrams/c4/c4Types.ts
  • packages/mermaid/src/diagrams/c4/styles.js
  • packages/mermaid/src/diagrams/c4/svgDraw.ts
  • packages/mermaid/src/docs/syntax/c4.md
  • packages/mermaid/src/rendering-util/rendering-elements/shapes.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/c4LabelHelper.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/cylinder.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/person.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/tiltedCylinder.ts
  • packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
  • packages/mermaid/src/rendering-util/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
  • .changeset/person-shape.md
  • .changeset/c4-outline-theme.md
  • .cspell/mermaid-terms.txt
  • docs/config/setup/mermaid/interfaces/LayoutData.md
  • docs/syntax/c4.md
  • packages/mermaid/src/docs/syntax/c4.md
  • packages/mermaid/scripts/docs.spec.ts
  • packages/mermaid/src/rendering-util/types.ts

Comment thread cypress/integration/rendering/c4/c4-characterization.spec.js
@filipsajdak

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai - both nitpicks addressed here rather than in code, with reasons.

util.ts: reserve stereotype-based label routing for C4. Not changing this. The dispatch is deliberate and was @aloisklink's own instruction during the #7842 review: stereotype was added to BaseNode and labelHelper dispatches on it at the top, precisely so shapes keep a single entry point instead of each C4 caller reaching for a different helper. It is also no longer this PR's code - #7842 merged earlier today, so that routing now lives in develop.

Worth noting the premise is only half right: any node carrying a stereotype should take the stacked-label path, because that is what the field means - it is documented on BaseNode as "Stereotype line rendered between label and description in multi-section labels". Nothing outside the C4 adapters sets it today, so no existing diagram changes behaviour. If a future diagram wants a stereotype line and HTML labels/markdown/centerLabel, that is a real gap - but the right time to add a routing flag is when a second consumer exists, not before.

c4ShapeAdapter.ts: hardcoded fill:#ffffff will read poorly on dark themes. Agreed, and this is the open question already stated in the PR description rather than an oversight. The literal white is a placeholder: the outline look needs both the fill and the identity colour to come from theme variables, which is the same ground #7868 covers, so picking a source unilaterally here would pre-empt that and likely be undone. Deliberately left for a maintainer to steer - it is the one design decision this PR is asking about.

I have made that consequence explicit in the PR description so the trade-off is visible rather than implied.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@filipsajdak Thanks for the detailed clarification. I agree with both resolutions:

  • The stereotype dispatch is the intended shared rendering contract from #7842, and adding a speculative C4-specific routing flag before another consumer needs it would be premature.
  • The hardcoded white fill is a known, documented design trade-off pending the theme-variable direction in #7868, rather than an accidental omission in this PR.

I’m withdrawing both findings.

🐇💭


✏️ Learnings added
Learnt from: filipsajdak
URL: https://github.com/mermaid-js/mermaid/pull/7991

Timestamp: 2026-07-30T17:03:27.806Z
Learning: In Mermaid’s unified node renderer, `BaseNode.stereotype` is a semantic contract for a stereotype line between a node label and description; `labelHelper` intentionally routes any node with `stereotype` to the stacked C4-style label helper. This is shared behavior introduced by PR `#7842`, not C4-only routing, and no non-C4 adapter currently sets `stereotype`.

Learnt from: filipsajdak
URL: https://github.com/mermaid-js/mermaid/pull/7991

Timestamp: 2026-07-30T17:03:27.806Z
Learning: For the C4 outline-style migration, the literal white fill and identity-color border/text in `packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts` are an explicit temporary design trade-off. Theme-variable sourcing is intentionally deferred for maintainer direction because it overlaps the theme-variable work in PR `#7868`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@filipsajdak

Copy link
Copy Markdown
Contributor Author

Rebased onto current develop and dropped the trailing merge commit: 2 commits -> 1.

The tree is byte-identical to the previous head - the commit was rebuilt from that exact tree rather than replayed, and git diff <old head> <new head> was empty before pushing. 126 c4 unit tests pass, tsc clean.

Still 3 files, +35/-14, and the open question in the description stands: whether the outline fill and identity colour should be sourced from theme variables (overlapping #7868) before this merges.

@filipsajdak

Copy link
Copy Markdown
Contributor Author

No human review yet on this one either. Standalone since #7842 merged: 3 files, +35/-14.

There is a genuine open question here that needs a maintainer opinion rather than more work from me: whether the light fill and identity colour should come from theme variables (which overlaps #7868) or stay literal for now. As it stands the fill is a literal white, which reads poorly on dark themes. If you would rather this waited for the theme work, it is cheap to park - it is deliberately the most droppable PR in the set.

The argos failure is the intended redesign awaiting approval, not a broken test.

Full stack status: #7849 (comment)

@filipsajdak
filipsajdak force-pushed the feature/c4-outline-theme branch from c05a376 to 0a56256 Compare August 11, 2026 14:16
@filipsajdak

Copy link
Copy Markdown
Contributor Author

Force-pushed to remove the Co-Authored-By: Claude / Claude-Session: trailers from the commit message. Message-only rewrite - the tree is byte-identical, verified with git diff <old-tip> <new-tip> returning empty, so the fix itself is unchanged and any review you have already done still applies.

This was raised in review on #7859, citing the project's own rule against tagging commits as AI-authored. Since this repo merges rather than squashes, the message would land on develop verbatim, so it had to be corrected before merge rather than after.

Nothing else about the PR moved: same single commit, same diff, same tests.

@knsv-bot knsv-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sisyphus-bot]

Thanks for splitting this out, @filipsajdak — separating the visual redesign from the shape migration was exactly the right call, and it paid off: the diff is 3 files and the Argos comparison is precisely the legacy-colours → outline change, nothing else. That is a genuinely well-constructed PR.

Flagging this as "changes requested" mostly because you asked us a direct question that should be answered before this merges — your own words — plus one behaviour change I don't think was intentional. The direction is right; none of this is rework.

What's working well

🎉 The split itself. Following through on @aloisklink's review by rebasing the redesign onto merged #7842 rather than arguing for the bundle is the kind of thing that makes review cheap for everyone downstream.

🎉 The characterization suite from #7842 is doing its job here. c4-characterization.spec.js covers every element variant, so this PR gets a meaningful visual diff for free. That's the payoff for having written it.

🎉 ensureReadable guards the NaN path (c4ShapeAdapter.ts:140-142). I verified against the real palette — #08427B, #686868, #1168BD all pass through untouched (L ≤ 0.42 already), and only the lighter entries move: #999999#6b6b6b, #85BBF0#176cbf. Sensible, minimal behaviour on the defaults.

🎉 The PR description states the cost of deferring instead of quietly leaving it. More PRs should do this.

Things to address

🟡 [important] <type>_border_color is now silently ignored

c4ShapeAdapter.ts:154-171 — the stroke used to come from the <type>_border_color config entry. It now comes from <type>_bg_color (darkened), and _border_color is never read at all.

Those keys are still public config: 40 of them in config.schema.yaml:757-877, each with its own default distinct from the bg colour (person_border_color: #073B6F, system_border_color: #3C7FC0, external_person_border_color: #8A8A8A). Anyone who set one today gets it dropped with no warning and no error.

I don't think you can detect "user customised this" at runtime — config always carries the default — so honouring it conditionally isn't really available. That makes it a documentation and changeset problem rather than a code one:

  • Add a line to .changeset/c4-outline-theme.md saying <type>_border_color no longer affects rendering, so it lands in the release notes.
  • Worth a maintainer opinion on whether the keys should be marked deprecated in the schema, or kept as a way to override the derived stroke.

Happy to be told this is fine as-is — but it should be a decision on the record rather than a side effect.

🟡 [important] The hardcoded white fill — answering your open question

c4ShapeAdapter.ts:156 (const styles: string[] = ['fill:#ffffff']). You're right that this leaves white boxes on dark themes, and I don't think you need to wait for #7868 to fix it.

personBkg and personBorder already exist as theme variables in every theme, dark included (theme-default.js:120-121, theme-dark.js:88-89), and c4/styles.js already consumes options.personBkg for the .person rule. mainBkg is likewise what userNodeOverrides falls back to for node fills across the codebase.

So the shape that avoids the dark-theme regression without blocking on the theme-variable work:

  • Move the base fill out of the inline per-element styles and into styles.js as a .c4-shape rule driven by a theme variable (options.personBkg / mainBkg).
  • Keep emitting an inline fill: only for an explicit $bgColor override.

That ordering matters and works in your favour: an inline style attribute beats a stylesheet rule, so dropping the unconditional inline white is what lets the theme value apply while $bgColor still wins. Dark themes then get a dark fill for free, and this stops being coupled to #7868 at all.

If you'd rather sequence it after #7868, that's a defensible call — but then I'd want the changeset to say dark themes are knowingly not supported yet.

🟡 [important] Test coverage for the changed code

The "126/126 C4 unit tests passed" figure is all parser tests (parser/c4*.spec.ts) — there is no c4ShapeAdapter.spec.ts, so elementCssStyles and ensureReadable have no direct coverage, and this PR could not have failed a unit test.

Two concrete gaps:

  1. ensureReadable is a pure string → string function — ideal unit test. A handful of cases (already-dark passthrough, the #999999 clamp, the NaN passthrough) would pin the behaviour cheaply.
  2. No theme coverage in the C4 Cypress specs at all — I grepped both c4.spec.js and c4-characterization.spec.js and neither sets a theme. This is the one that matters: Argos will show a clean, correct-looking default-theme diff while the dark-theme regression above stays completely invisible. A single dark-theme imgSnapshotTest would make the tradeoff visible instead of theoretical.

Security

Ran a dedicated XSS/injection pass over the diff. No new injection surface — the old code already interpolated $bgColor/$borderColor/$fontColor into the same three properties, and these values reach the DOM via .attr('style', …) (drawRect.ts:57, person.ts:53, util.ts:35), never a <style> element or innerHTML. useHtmlLabels: false keeps labels on the SVG <text> path. Routing the palette colour through hsl().formatHex() actually narrows it to #rrggbb. DOMPurify is untouched.

One pre-existing item, explicitly out of scope for this PR, noted so it isn't lost: c4Db.ts:504-527 (updateElStyle) stores $bgColor/$borderColor/$fontColor verbatim without sanitizeText, and styles2Map splits on the first : only — so $fontColor="red;font-family:x" smuggles an extra declaration into the style attribute. Bounded to CSS-in-an-attribute (no script execution), but worth a separate issue.

Nits

🟢 c4ShapeAdapter.ts:176 — the buildC4Node JSDoc still says the <type>_bg_color/<type>_border_color palette "drives the fill and border". After this change _bg_color drives border and text, and _border_color drives nothing. You updated the elementCssStyles doc comment right above it; this one got left behind.

🟢 ensureReadable (c4ShapeAdapter.ts:139-146) promises a bit more than it delivers — it clamps HSL lightness, which isn't perceptual luminance. #ffff00#d6d600, roughly 1.5:1 against white. The defaults are all blues and greys so nothing is affected today, but a custom yellow/cyan palette still lands unreadable. Either a luminance-based check (khroma exposes luminance/contrast) or a slightly humbler name would match the behaviour.

🟢 The double fill: (:156 then :163) is correct — I traced it through compileStylesstyles2Map, which is a Map, so last-write-wins, and the rough path reads the same map via stylesMap.get('fill'). But it depends on non-obvious semantics in shared rendering-util/ code. A short comment on :156 noting the override relies on last-wins would protect it.

💡 The codebase's colour-manipulation convention is khroma (themes/*.js, kanban/styles.ts); there's no other hsl import from d3 in src/. d3 is already a dependency so this isn't wrong, just off the beaten path — and khroma would give you the luminance check in the same move.


To be clear about the ask: items 2 and 3 are really one decision (theme variable + a dark-theme snapshot to prove it), and item 1 is a sentence in the changeset. Happy to look again quickly once you've picked a direction on the fill — and if you'd rather defer the theme work to after #7868, say so and I think this can go in with the changeset updated to match. Thanks again for the clean slice.

@knsv

knsv commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Hej Filip!

Your contributions have really stood out, and we appreciate the care and momentum you’ve brought to the project. If you’d be interested, we’d be happy to talk about ways for you to be involved more closely.

@filipsajdak

Copy link
Copy Markdown
Contributor Author

Hi Knut!

At the moment I am focusing on bringing full support for C4 diagrams that I need to build my architecture documentation (arc42). I am open to discuss any ideas that you have in mind.

@filipsajdak
filipsajdak force-pushed the feature/c4-outline-theme branch 2 times, most recently from 0d42c7a to 2db6952 Compare August 12, 2026 17:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/mermaid/src/diagrams/c4/c4Colors.ts`:
- Around line 30-40: The contrast adjustment logic must retry from the original
color using the opposite shift direction when the initial 12-step pass remains
below CONTRAST_TARGET. Update the relevant readable-color function around the
shift/readable loop, preserving the bounded behavior, and add a regression
assertion that readableOn('`#08427B`', '`#888888`') achieves at least 4.5:1
contrast.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 18accfd5-6c2a-4469-9941-b7afea9364d7

📥 Commits

Reviewing files that changed from the base of the PR and between c05a376 and 0d42c7a.

📒 Files selected for processing (8)
  • .changeset/c4-outline-theme.md
  • cypress/integration/rendering/c4/c4.spec.js
  • packages/mermaid/src/diagrams/c4/c4Colors.spec.ts
  • packages/mermaid/src/diagrams/c4/c4Colors.ts
  • packages/mermaid/src/diagrams/c4/c4Renderer.ts
  • packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts
  • packages/mermaid/src/diagrams/c4/styles.js
  • packages/mermaid/src/diagrams/c4/styles.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/mermaid/src/diagrams/c4/c4ShapeAdapter.ts

Comment thread packages/mermaid/src/diagrams/c4/c4Colors.ts Outdated
@filipsajdak

Copy link
Copy Markdown
Contributor Author

Thank you - and thank you for answering the open question instead of leaving it to me. You were right that this did not need to wait for #7868, and following the theme-variable pointer turned up something I had not seen: the change as you described it would have shipped a bug, so it grew a little beyond the three items.

🟡 The white fill - taken, and the reason it needed more than moving the fill

Your reasoning about the mechanism holds exactly. styles2String emits node styles as an inline style attribute with !important (handDrawnShapeStyles.ts:87), so a stylesheet rule for the base fill is overridden by an inline $bgColor just as you said, and the inline unconditional white was the only thing in the way.

What that surfaced: ensureReadable clamped lightness to <= 0.42 - always darker. That is correct on a white fill and wrong the moment the fill follows the theme, because theme-dark's surface is dark and the clamp would push the identity colour into its own background. Dark text on a dark body is precisely the regression the change is meant to remove, so moving the fill alone would have traded a white-on-dark problem for a dark-on-dark one.

The clamp also could not be fixed where it was: elementCssStyles only received the c4 config, so it had no way to know what surface it was drawing on.

So the palette-derived colours now come from the stylesheet, which is the one place with both getConfig().c4 and the theme variables - the same place, and the same CSSOM construction, as the existing per-type font rules. The adapter emits only the UpdateElementStyle overrides, which stay inline and keep precedence.

readableOn(color, surface) picks the direction from isDark(surface) and shifts until it clears 4.5:1, so light themes darken and dark themes lighten.

One caveat on tooling, in case it saves you time later: khroma's contrast is not the WCAG ratio - it drops the + 0.05 terms and clamps to 10 (dist/methods/contrast.js), so it saturates for nearly every realistic pair and cannot tell white/#333 from #f4f4f4/#333. The ratio is computed from luminance instead. Otherwise khroma is used as you suggested, which also retires the d3 hsl import - thanks for that nudge, it was the only hsl from d3 in src/.

On the fill variable: I used options.background rather than mainBkg. mainBkg is #ECECFF in the default theme and #cde498 in forest, so an "outline" element would come out lavender or olive; background is the only public variable that is near-white in every light theme (white, white, #ffffff, #f4f4f4) and genuinely dark in theme-dark (#333). There is precedent for reading it this way in vennRenderer.ts:120 and for the surface role in state/styles.js:112. Happy to switch to mainBkg if you would rather C4 match the flowchart convention exactly - it is a one-line change and I do not feel strongly, beyond wanting the c4model.com look to stay white-ish.

While in there: the .person { fill: ${options.personBkg} } rule was dead CSS. Since #7842 nothing applies a bare person class - elements carry c4-shape c4-person - so personBkg was wired up and unreachable. Removed rather than retargeted, since the type rules now cover it.

🟡 <type>_border_color silently ignored - documented

Correct, and your read that it cannot be detected at runtime matches what I found: the config always carries a default, so "did the user set this" is not answerable. Treated as documentation, per your suggestion - the changeset now says the keys no longer affect rendering, alongside a note that element bodies follow the theme.

Leaving the schema question to you deliberately: the 40 keys can be marked deprecated, or kept as an explicit stroke override that wins over the derived identity colour. The second is a small change if you want the escape hatch, and I have no strong preference - it is your API.

🟡 Test coverage - added, and you were right about what the old figure meant

The "126/126" was indeed all parser tests; nothing exercised elementCssStyles or ensureReadable, and the PR could not have failed a unit test. Now:

  • c4Colors.spec.ts on readableOn: passthrough when already readable, darkening on a light surface, lightening on a dark one (the guard for the bug above), every palette colour reaching 4.5:1 on #ffffff/#f4f4f4/#333333, and an unparseable value returned untouched so it reaches CSS and is dropped there rather than becoming NaN.
  • styles.spec.ts on the generated stylesheet: the body takes the surface colour, two different palette entries produce two different identity colours, the identity colour is lighter on a dark surface than on a light one, and it is set on the group so the label inherits it.
  • C4.9, a default vs dark imgSnapshotTest. You were right that this was the one that mattered - with no theme anywhere in the C4 specs, Argos would have shown a clean light-theme diff while the dark-theme problem stayed invisible.

Deliberately in its own c4Colors.spec.ts/styles.spec.ts rather than c4ShapeAdapter.spec.ts, because #7883 already adds a file at that path and I would rather not hand you a conflict.

🟢 Nits

Stale buildC4Node JSDoc: fixed - and the config parameter it described is gone, since the palette read moved out and nothing else used it. The double fill: and its last-write-wins subtlety are gone with the unconditional white, so that comment is moot. The two duplicated selector lists in styles.js are now one SHAPE_PARTS const.

On ensureReadable overpromising: agreed, and it is retired rather than renamed - readableOn measures an actual contrast ratio instead of clamping lightness, so #ffff00 is now handled by measurement rather than by hoping.

Also

Local checks: 135 c4 unit tests pass (including the 9 new ones), tsc, eslint and Prettier clean. argos will show the redesign plus the new dark-theme case.

An element sits on the theme's surface colour with its identity colour as the
border and the label text, as on c4model.com, rather than a solid palette fill
with white text. The identity colour is the element's `<type>_bg_color` palette
entry shifted until it reads against that surface, so a dark theme gets a light
identity on a dark body instead of a dark one on a dark one.

The palette-derived colours are emitted by the stylesheet, keyed on the
`c4-<type>` class the shape adapter sets, because that is where the theme
variables are available. `UpdateElementStyle`'s `$bgColor`, `$borderColor` and
`$fontColor` are still emitted inline on the node and so continue to take
precedence over all of it.

`<type>_border_color` no longer affects rendering: the border is the identity
colour derived from `<type>_bg_color`.
@filipsajdak

Copy link
Copy Markdown
Contributor Author

Filed the element-colour validation item as #8057, as promised.

Worth flagging one thing, because I nearly filed it wrong. I had written down the mechanism as "styles2Map splits on the first : only, so $fontColor="red;font-family:x" smuggles a second declaration". Checking it before filing, that is not what happens - styles2Map (handDrawnShapeStyles.ts:44-51) does const [key, value] = style.split(':') with no limit and reads only the first two elements, so everything after the second colon is dropped. The default look yields color:red;font-family !important: one declaration that silently lost its !important, plus a colonless fragment the browser discards. Nothing attacker-chosen lands.

What is actually true, and what the issue says instead:

  • In every look, an injected ; corrupts the declaration rather than extending it - a robustness bug, not an injection.
  • A complete declaration of the author's choosing lands only under look: handDrawn, where the raw cssStyles array goes straight to .attr('style', ...) (drawRect.ts:49, person.ts:52-53, cylinder.ts:105-106) and bypasses styles2Map altogether. look is settable from frontmatter, so that path is reachable.
  • Severity low, and stated as such: inline style attribute on SVG shapes only, never innerHTML or a <style> element, C4 labels are SVG text (useHtmlLabels: false in both the adapter and c4LabelHelper), no script execution. I explicitly did not claim a confirmed external fetch, since background properties do not paint on SVG shapes and I have not tested it in a browser.

The suggested fix is the asColor shape from #7883, ideally as one helper shared by the element and relationship paths rather than a third copy - #7883 and #8042 each carry one already.

That leaves the strengthened characterization assertions as the only thing still owed from this review. Both blockers have merged, so I will do them with the $shape/$sprite rewire as described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants