-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
Add elk layout support to mermaid
#36486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
layout: elk support to mermaidelk layout support to mermaid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Enables Mermaid diagrams rendered from Markdown to optionally use the elk layout engine (via Mermaid configuration in frontmatter or %%{ init }%% directives) without changing the default layout.
Changes:
- Adds
@mermaid-js/layout-elkas a frontend dependency. - Registers ELK layout loaders during Mermaid initialization for markup-rendered Mermaid blocks.
- Updates lockfile to include ELK-related transitive dependencies (e.g.,
elkjs).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web_src/js/markup/mermaid.ts | Dynamically loads and registers ELK layout loaders with Mermaid before rendering diagrams. |
| package.json | Adds @mermaid-js/layout-elk dependency. |
| pnpm-lock.yaml | Locks @mermaid-js/layout-elk and new transitive dependencies. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
web_src/js/markup/mermaid.ts:30
- The heavy dynamic imports (now including
@mermaid-js/layout-elk) happen before the early-exit checkif (!pre || pre.hasAttribute('data-render-done')) return;. This means pages that re-runinitMarkupCodeMermaidon already-processed markup will still pay the cost of loading/registering Mermaid/elk.
Consider moving the pre/data-render-done check (and possibly the max-length check) before performing the dynamic imports, so already-rendered blocks can bail out without extra network/work.
mermaid.initialize({
startOnLoad: false,
theme: isDarkTheme() ? 'dark' : 'neutral',
securityLevel: 'strict',
suppressErrorRendering: true,
});
const pre = el.closest('pre');
if (!pre || pre.hasAttribute('data-render-done')) return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The suggestion from Copilot to load the layout on-demand is good, but it would require to pre-parse the mermaid source for the frontmatter block with a yaml parser, and the init block as a variant of JSON. I think this will be too complicated to add. |
I've actually implemented lazy loading now with a crude regex detection that works for both yaml frontmatter and json init directive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
web_src/js/markup/mermaid.ts:74
- Inside the per-diagram loop, the
catchformermaid.parse(source)callsreturn;, which exitsinitMarkupCodeMermaidentirely. This means a single invalid Mermaid block will prevent all subsequent Mermaid blocks in the same.markupcontainer from rendering. Usecontinuehere (and keep the error display) so other diagrams still render.
await mermaid.parse(source);
} catch (err) {
displayError(pre, err);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
web_src/js/markup/mermaid.ts:74
- In the
mermaid.parseerror handler,return;exitsinitMarkupCodeMermaidentirely, which means if one diagram fails to parse, subsequent mermaid blocks in the same.markupcontainer will never be processed. Previously (withqueryElems(..., async (el) => ...)) this only stopped processing for the failing element.
Use continue here (or otherwise ensure errors are per-element) so the remaining diagrams still render.
try {
await mermaid.parse(source);
} catch (err) {
displayError(pre, err);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* origin/main: Fix incorrect vendored detections (go-gitea#36508) Bump alpine to 3.23, add platforms to `docker-dryrun` (go-gitea#36379) Unify repo names in system notices (go-gitea#36491) Allow scroll propagation outside code editor (go-gitea#36502) Refactor ActionsTaskID (go-gitea#36503) Update JS deps, remove `knip`, misc tweaks (go-gitea#36499) [skip ci] Updated translations via Crowdin Fix editorconfig not respected in PR Conversation view (go-gitea#36492) Add FOLDER_ICON_THEME configuration option (go-gitea#36496) Don't create self-references in merged PRs (go-gitea#36490) Use reserved .test TLD for unit tests (go-gitea#36498) Fix bug when list pull request commits (go-gitea#36485) Update some go dependencies (go-gitea#36489) chore: add comments for "api/healthz", clean up test env (go-gitea#36481)
| // yaml frontmatter | ||
| if (/^['"\s]*(layout|defaultRenderer)['"\s]*:['"\s]*elk/m.test(source)) return true; | ||
| // json init | ||
| if (/"(layout|defaultRenderer)"\s*:\s*"elk/.test(source)) return true; | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still seems very fragile and shouldn't be that complex to maintain. And it's not possible to support other options in the future.
The format should be pretty well-defined. So I think it should: extract the yaml between --- and json between %%{ ... }%%, then parse the yaml and json, then get the options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will add 40kB (13kB gzipped) to the main chunk for the yaml parser though, but might be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will add 40kB (13kB gzipped) to the main chunk for the yaml parser though, but might be ok.
Really? We already have it in "Support OpenAPI spec"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will add 40kB (13kB gzipped) to the main chunk for the yaml parser though, but might be ok.
Really? We already have it in "Support OpenAPI spec"
If you don't like to put it into "main" trunk, you can first parse "----", if there is no yaml, then no need to load the yaml parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swagger.js is its own entry point, does not affect index.js chunk.
It might be ok, a yaml parsers can be useful for other future topics.
Fixes: #34769
This allows the user to opt-in to using
elklayouts using either YAML frontmatter or%%{ initdirectives inside the markup code block. The default layout is not changed.When configuration for
elklayout is detect, it causes a new webpack dynamic chunk to be loaded:If no
elklayout configuration is detected, the layout is not loaded:Before:
After:
