Repository navigation
fix(c4): allow '#' in title and accDescription - #8039
filipsajdak wants to merge 2 commits into
Conversation
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 0e8cfbf 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 |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the primary change: allowing '#' in C4 title and accDescription values. |
| Description check | ✅ Passed | The description explains the problem, cause, implementation, tests, affected grammar rules, and linked issue. It does not use the repository template headings or include the task checklist, but it con… |
| Linked Issues check | ✅ Passed | The changes directly satisfy issue [#8030] by allowing '#' in C4 title values and adding parser coverage. The lexer preserves newline and semicolon termination, and the accDescription fix is consisten… |
| Out of Scope Changes check | ✅ Passed | All changes are directly related to the linked issue: the C4 lexer fix, parser tests, and a patch changeset. No unrelated code or scope expansion is present. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
Comment @coderabbitai help to get the list of available commands.
@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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8039 +/- ##
===========================================
- Coverage 81.15% 79.46% -1.70%
===========================================
Files 623 601 -22
Lines 86032 77232 -8800
Branches 19003 15707 -3296
===========================================
- Hits 69822 61370 -8452
- Misses 15203 15862 +659
+ Partials 1007 0 -1007
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 ↗︎ Awaiting the start of a new Argos build… |
b93f46b to
5e4446e
Compare
|
Force-pushed to remove the 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 Nothing else about the PR moved: same single commit, same diff, same tests. |
|
Gentle nudge - this one has been open since 08-07 with no review yet. It is a 1-line grammar fix plus tests: One note on the checks so they do not put you off: the Netlify reds are stale rather than real. This PR last built on 08-07, inside the outage window described in #8049, and Netlify passes on PRs opened since. No urgency - just flagging it, since it is small and self-contained enough to be easy to lose. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Synced with 171 C4 tests green on the merge. Gentle nudge while I am here, and no rush at all - open since 08-07, and I last flagged it on 08-20. It is a one-line grammar fix: |
|
Correcting the last line of my previous comment. I said the red Netlify checks were "the docs build on a base that predates the current toolchain". I do not have evidence for that, and I should have checked my own notes before writing it: those failures are the repo-wide intermittent one I filed as #8049, which since 2026-08-07 has hit roughly 78% of deploy previews across unrelated authors, this PR's 2026-08-11 build among them. The deploy preview is green on the synced head, in about two minutes - which matches the timing signature of a real build rather than the ~20-second death that marks the failing ones. But one green run cannot tell the two explanations apart, since #8049 has always had successes interleaved. So: the checks are green now, the sync was worth doing on its own merits, and I am not claiming it is what fixed them. Everything else in that comment stands - clean merge, diff unchanged, tests green. |
|
The That is a wall-clock ratio assertion. It landed on Run locally on this branch's head, five times in a row: 5 passes, no failures. So the implementation is linear as intended and the CI run was a loaded runner, which is the failure mode the test's own comment anticipates - "the bar is set at 8 so ordinary timing noise on a loaded machine cannot fail it". On a shared runner 8x headroom turns out not to be enough. Not my call what to do about it, and I do not want to speak for the author. If it would help I can open an issue with the numbers rather than leaving it to be rediscovered one PR at a time - say the word. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Synced with Correcting what I said about the flaky test on 09-04The red here was That was too strong. Today it failed on my own machine, on the first run after a
1 failure in 13 local runs, and the one failure came while the machine was still finishing an install. That is a better description than either "it passes locally" or "it is broken": the implementation is linear, and the assertion is sensitive to whatever else the machine is doing. The bar is Not my call what to do about it, and I do not want to speak for the author. Recorded here because I put a weaker claim on this thread three days ago and it deserves the correction. |
|
The
Two other people's branches hit the same 30-minute wall in the same three days - Everything else here is green: I have not pushed anything to force a re-run. It would probably go green - 30m05s against a 30m cap is a coin flip - but spending a full e2e run to reroll a dice throw seemed worse than saying plainly what the red is. Happy to re-trigger it if you would rather see it green before reviewing. |
|
Closing the loop on the flaky test, because the maintainers fixed it and did a better job of it than my framing did. I had characterised
That is the sharper diagnosis: not "this assertion is noisy" but "this assertion cannot detect the regression it guards". Worth saying plainly since I had it on this thread as a flake. Both fixes are on The current |
The lexer rules for `title` and `accDescription` excluded `#` from their character class, so a `#` anywhere in either value terminated the token and the remainder of the line failed to lex with "Lexical error ... Unrecognized text". C4's comment syntax is `%%` and the grammar has no `#` comment rule, so the exclusion served no purpose. Fixes mermaid-js#8030
d8b36b2 to
9dff9f1
Compare
|
@pbrolin47 this is ready for review: CI is green, Argos included, and it is mergeable. |
|
Gentle ping, no hurry: this is one commit that closes #8030, it touches the parser only (no renderer or snapshot change), and checks are green. |
Problem
A
#anywhere in a C4titlebreaks the whole diagram:accDescriptionhas the same defect.Cause
c4Diagram.jisonmatched both values with[^#\n;]+:The
#in the character class ends the token early, and the remainder of the line then fails to lex. C4's comment syntax is%%, with its own\%\%lexer rules, and the grammar has no#comment rule at all, so nothing depends on that exclusion.Verified through
mermaid.parse, so the same preprocessing as the render path:title Relationship to a boundary (#4864)title Relationship to a boundaryaccDescription covers issue #4864accTitle: covers #4864accTitle:/accDescr:use different lexer rules and were already unaffected. This also looks C4-specific:sequenceDiagramaccepts a#in its title despite carrying a similar-looking exclusion.Change
Drop
#from the two character classes.\nand;still terminate the value, so nothing else about the rules changes. Two parser tests cover a#in each.Found while adding the Cypress case @pbrolin47 asked for in #7874, where a
(#4864)in the test's owntitlewas what made that test fail.Fixes #8030
Summary
#characters in C4titleandaccDescriptionvalues.