Repository navigation
fix(c4): allow hash characters in titles - #8031
cyphercodes wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 31cc59d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
❌ Deploy Preview for mermaid-js failed.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe C4 lexer now accepts ChangesC4 title parsing
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this — and for the issue that came with it. #8030 is one of the better bug reports we've had: root-caused to the exact line, with a cross-diagram comparison table and a check that # isn't actually reserved in C4 (comments are %%). The PR then does exactly what that analysis pointed at, and nothing more. 🎉
What's working well
I verified the change locally rather than taking it on faith:
- 🎉 The test genuinely captures the bug. Applying the spec change without the grammar fix fails with precisely the error from the issue (
Lexical error on line 2. Unrecognized text. ...ontexttitle Sprint #3 architecturePers). With the grammar fix applied, the full C4 spec passes 4/4. - 🎉 All three failing forms from the issue table now parse:
title Sprint #3 architecture→Sprint #3 architecture,title Boundary (#4864)→Boundary (#4864),title issue#4864→issue#4864. Statement separation is intact —\nand;are still excluded, so the followingPerson(...)line lexes normally. - 🎉 No security impact.
#isn't an HTML/SVG-special character, and the title still flows throughsanitizeText()(c4Db.ts:771) and is written with D3's.text()(c4Renderer.ts:611), which setstextContentrather than parsing HTML. No DOMPurify config touched, no new sinks. No XSS or injection concerns.
No e2e test needed here — this is parser-only, and repo convention is that unit tests suffice for parser/DB changes.
Things to address
🟡 [important] — accDescription on the next line has the identical bug
packages/mermaid/src/diagrams/c4/parser/c4Diagram.jison:88 still reads:
"accDescription"\s[^#\n;]+ return 'accDescription';
Same [^#\n;] exclusion, same hard failure. Verified on your branch:
C4Context
accDescription Sprint #3 notes
Person(a, "A")
→ Lexical error on line 2. Unrecognized text.
What makes this worth raising rather than leaving alone: the colon forms already accept # (I checked — accTitle: Sprint #3 and accDescr: Sprint #3 notes both parse fine). So after this PR, C4 metadata ends up inconsistent:
| syntax | # allowed? |
|---|---|
title |
✅ (this PR) |
accTitle: |
✅ (already) |
accDescr: |
✅ (already) |
accDescription |
❌ |
It's the same one-character change on the adjacent line, and it's an accessibility field, so the failure mode is a bit worse than cosmetic. I can see from your Design Decisions section that scoping to title only was deliberate — so treat this as a request to reconsider, not a correction. Happy either way: fold it in here, or say the word and it becomes a follow-up. Mainly I'd rather it not sit around and resurface as a duplicate issue.
🟢 [nit] — changeset description prefix
.changeset/c4-title-hash.md — CLAUDE.md asks for a fix:/feat:/chore: prefix, and the sibling changesets all follow it (fix(architecture): allow non-ASCII characters..., fix(c4): named attributes...). Suggest:
fix(c4): allow `#` characters in C4 diagram titles
🟢 [nit] — lock in the other two forms from the issue
The test covers Sprint #3 architecture. The issue tabulated two more broken forms — (#4864) and issue#4864 (no space before the #). Both pass now, so this is purely about keeping them from regressing. Cheap to add as extra assertions in the same test.
Out of scope, just noting
Pre-existing and unchanged by this PR: title Foo %% note swallows the comment into the title (yielding "Foo %% note"), since % was never in the exclusion set either. Worth a separate issue if anyone cares to file one — definitely not for this PR.
Nice, tight fix. Once the accDescription question is settled this is good to go, (assuming the CI goes through). Thanks again for the unusually thorough issue writeup. 🙏
|
Updated this with the review follow-up:
Verification run locally:
|
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8031 +/- ##
===========================================
+ Coverage 77.61% 77.66% +0.05%
===========================================
Files 568 568
Lines 74992 74992
Branches 12690 14655 +1965
===========================================
+ Hits 58202 58246 +44
+ Misses 15796 15753 -43
+ Partials 994 993 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
knsv-bot
left a comment
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Thanks for this, @cyphercodes — and for the unusually thorough issue in #8030 that preceded it. The table you built there comparing # handling across sequenceDiagram / journey / gantt / pie / requirementDiagram is exactly the kind of scoping that makes a grammar change easy to trust, and it saved a lot of review time here. Sorry it sat for a few days.
The code is in good shape. I have one coordination item that needs a decision before this can land, and a couple of cosmetic notes.
What's working well
🎉 [praise] The fix is correctly minimal. # is not a comment character in C4 (comments are %%, c4Diagram.jison:98-99) and I confirmed it plays no other role in the grammar — after this change grep '#' c4Diagram.jison returns only lines 90 and 92, which are unrelated acc_title/acc_descr rules. Widening the character class really was the whole fix, and you resisted the temptation to restructure anything around it.
🎉 [praise] Keeping ; excluded ([^\n;]+ rather than [^\n]+) is the right conservative call. For reference, gantt.jison:90 uses the fully permissive "title"\s[^\n]+ — so this change moves C4 toward the established precedent without going all the way there in one step.
🎉 [praise] The tests genuinely capture the bug. I checked out the branch and ran the new spec against the unpatched develop grammar — all four cases fail with precisely the Lexical error on line 2. Unrecognized text. from the issue, and all pass with the grammar change applied. That's the TDD cycle done properly, not tests written to fit a fix.
🎉 [praise] The it.each covering three distinct # placements (mid-sentence Sprint #3 architecture, parenthesized Boundary (#4864), suffix issue#4864) maps directly onto the failing cases from the issue table, and extending coverage to accDescription was a good catch.
🎉 [praise] Changeset present, correct patch bump, correct fix(c4): prefix.
Regression check
I ran a before/after probe over eight edge cases to confirm the relaxed class doesn't change anything beyond the intended fix:
| input | before | after |
|---|---|---|
title System Context diagram |
OK | OK (unchanged) |
title Foo %% a comment |
OK, "Foo %% a comment" |
OK, identical (pre-existing; % was always allowed) |
title Foo; bar |
lexical error | lexical error (unchanged — ; still excluded) |
title Foo direction TB |
parse error | parse error (unchanged) |
title Sprint #lt;3#gt; |
lexical error | lexical error (unchanged) |
title #<script>alert(1)</script> |
lexical error | parses, title sanitized to "#" |
title Foo # |
lexical error | parses ✅ |
#-bearing titles are the only behavior change. Nothing else moved.
Security
No XSS or injection issues. The path was traced end to end: c4Diagram.jison:87 → production at :241 → c4Db.ts:770-773 setTitle → common.ts:82-94 sanitizeText (DOMPurify on every security level) → c4Renderer.ts:608-614 .append('text').text(title), a textContent sink. accDescription follows the equivalent path into accessibility.ts:57 svg.insert('desc').text(...).
Worth noting explicitly, since it's the natural worry with this change: encodeEntities (Diagram.ts:20) rewrites #word; sequences before the lexer runs, so the old [^#\n;]+ class never gated the entity path — it only blocked a bare #, which is inert in a textContent sink. Entity-based payloads behave identically before and after.
Things to address
🟡 [important] — PR #8039 is a duplicate of this one. @filipsajdak opened it a day later with a byte-identical c4Diagram.jison change and, awkwardly, the same changeset filename (.changeset/c4-title-hash.md). If both were to merge there'd be a collision on that file. Someone needs to pick one.
My read is that this PR should be the one that lands: it was opened first, and its test coverage is broader (three title placements via it.each plus accDescription, versus two single cases). Nothing in #8039 is missing here. This isn't something you did wrong — just flagging it so a maintainer can close the other one rather than leaving two identical PRs in the queue. A quick maintainer call on this would unblock the merge.
🟢 [nit] — The PR description says "Only the title lexer rule is relaxed", but commit 31cc59d also relaxes accDescription (c4Diagram.jison:88). Extending it there is the right thing to do — the bug is identical — the description just hasn't caught up. Worth a quick edit so the merge commit reflects the real scope.
🟢 [nit] — Same gap in .changeset/c4-title-hash.md: it mentions only titles. Since this text ends up in the published changelog, something like allow C4 titles and accDescription to include `#` characters would be more accurate.
💡 [suggestion] — Since the issue originated from a #4864 reference in a C4 rendering test in #7874, a single imgSnapshotTest case with a # in the title would close the loop nicely in cypress/integration/rendering/c4/c4.spec.js. Genuinely optional — this is a parser-only change and unit tests are the right level of coverage per our test strategy, so please don't feel obliged.
Out of scope (not for this PR)
c4Diagram.jison:90 and :92 contain (?!\n|;|#)* — a zero-width negative lookahead quantified with *, which matches empty and filters nothing. accTitle: and accDescr: consequently already accept # and ;. It's harmless (same sanitized .text() sink) but it's dead syntax that misleads anyone reading the grammar into thinking # is filtered there. Untouched by your change and best left to a separate cleanup — noting it only so it doesn't get lost.
CI
The four failing Netlify checks are not caused by this PR — the deploy preview is failing identically on #8047, #8045, #8041 and #8038. Everything else is green, including all 8 e2e shards, unit tests, lint, and Argos.
Once the #8039 overlap is sorted and the description/changeset wording is updated, this is ready to go. Nice, tight fix — let's get it across the finish line. 🚀
Review self-check
- Severity tally: 0 🔴 / 1 🟡 / 2 🟢 / 1 💡 / 5 🎉
- Verdict: COMMENT (1 🟡, no 🔴)
- Not a draft; no prior bot review rounds
- No inline comments used — all findings carry
file:linereferences in the body
|
[sisyphus-bot] Correction to my review above — I wrote it without properly accounting for the earlier review round on this PR, and that was unfair to you. Checking the history: every item from the 7 Aug review was addressed.
That's a clean round trip on all three. The substantive item in my review stands on its own and is not about your code: the duplicate PR #8039 still needs a maintainer to pick one. Everything else here is cosmetic. |
This branch has not been deployed
2 participants
📑 Summary
Fixes #8030.
Allows C4 diagram titles to include
#characters. Previously, the C4 lexer stoppedtitletokens at#, so titles likeSprint #3 architecturecaused a parse error.📏 Design Decisions
Only the
titlelexer rule is relaxed; comment handling and other metadata lexer rules are unchanged.📋 Tasks
#🧪 How to test
What's working well
#in titles and accumulated descriptions.Boundary (C4 diagram not possible to create relation from boundaries to container #4864)andissue#4864.fix(c4):prefix.Security
Review verdict
APPROVE