WIP: feat: add mermaid-bundle flag for SVG rendering with bundled source#769
WIP: feat: add mermaid-bundle flag for SVG rendering with bundled source#769mrueg wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in “bundle Mermaid source into SVG” rendering path, wiring it through config and CLI so Mermaid fenced blocks can render as SVG attachments with embedded source instead of PNG.
Changes:
- Introduces
--mermaid-bundle(env/toml supported) and propagates it through CLI/config plumbing. - Adds
ProcessMermaidWithBundle()to render Mermaid diagrams as SVG usingmermaid.go’sWithBundle(). - Updates fenced code block rendering to choose SVG vs PNG Mermaid processing based on config.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| util/flags.go | Adds --mermaid-bundle flag with env/toml sources. |
| util/cli.go | Wires CLI flag into mark.Config. |
| types/types.go | Adds MermaidBundle to types.MarkConfig. |
| renderer/fencedcodeblock.go | Switches Mermaid fenced rendering between SVG(bundle) and PNG(scale). |
| mermaid/mermaid.go | Implements ProcessMermaidWithBundle() using Render(..., WithBundle()). |
| mark.go | Propagates MermaidBundle into types.MarkConfig construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return attachment.Attachment{ | ||
| ID: "", | ||
| Name: title, | ||
| Filename: fileName, | ||
| FileBytes: []byte(svgContent), | ||
| Checksum: checkSum, | ||
| Replace: title, | ||
| }, nil |
There was a problem hiding this comment.
ProcessMermaidWithBundle returns an attachment without Width/Height populated. Downstream rendering (calculateAlign/calculateLayout and the ac:image template) relies on these dimensions for alignment/layout decisions and original-size metadata, so SVG mermaid images may be laid out incorrectly (e.g., wide diagrams not forced to center/wide/full-width). Consider extracting width/height from the rendered SVG (e.g., from width/height attributes or viewBox) and setting Attachment.Width/Height accordingly (or otherwise providing dimensions for SVG output).
| // ProcessMermaidWithBundle renders a Mermaid diagram as an SVG file with the | ||
| // original diagram source embedded in the SVG <desc> element (via | ||
| // mermaid.go's WithBundle option). The resulting attachment is an SVG rather | ||
| // than a PNG, so it is resolution-independent and the source can be recovered | ||
| // from the attachment. The mermaid-scale flag is not applicable to SVG output. | ||
| func ProcessMermaidWithBundle(title string, mermaidDiagram []byte) (attachment.Attachment, error) { | ||
| ctx, cancel := context.WithTimeout(context.TODO(), renderTimeout) | ||
| defer cancel() | ||
|
|
||
| log.Debug().Msgf("Setting up Mermaid renderer (bundle/SVG): %q", title) | ||
| renderer, err := mermaid.NewRenderEngine(ctx, nil) | ||
| if err != nil { | ||
| return attachment.Attachment{}, err | ||
| } | ||
| defer renderer.Cancel() | ||
|
|
||
| log.Debug().Msgf("Rendering (bundle/SVG): %q", title) | ||
| svgContent, err := renderer.Render(string(mermaidDiagram), mermaid.WithBundle()) | ||
| if err != nil { |
There was a problem hiding this comment.
New SVG/bundling rendering path (ProcessMermaidWithBundle) is not covered by tests. Since mermaid/mermaid_test.go already validates the PNG path, please add a similar test for the bundle/SVG path (at least asserting .svg filename, non-empty SVG output, and that the SVG contains a with the original diagram source) to prevent regressions.
| &cli.BoolFlag{ | ||
| Name: "mermaid-bundle", | ||
| Value: false, | ||
| Usage: "Render mermaid diagrams as SVG with the diagram source bundled in the <desc> element instead of PNG.", | ||
| Sources: cli.NewValueSourceChain(cli.EnvVar("MARK_MERMAID_BUNDLE"), altsrctoml.TOML("mermaid-bundle", altsrc.NewStringPtrSourcer(&filename))), | ||
| }, |
There was a problem hiding this comment.
The flag help text doesn’t mention that --mermaid-scale is ignored when --mermaid-bundle is enabled (as noted in the PR description). Consider adding that note to the Usage string so CLI/config users don’t expect scaling to apply to SVG output.
mrueg
force-pushed
the
feature/mermaid-bundle
branch
2 times, most recently
from
1cd660e to
e24a374
Compare
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
e24a374 to
ecc2382
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| checkSum, err := attachment.GetChecksum(bytes.NewReader(mermaidDiagram)) | ||
| log.Debug().Msgf("Checksum: %q -> %s", title, checkSum) | ||
| if err != nil { | ||
| return attachment.Attachment{}, err | ||
| } | ||
| if title == "" { |
There was a problem hiding this comment.
In processMermaidSVG(), the attachment checksum is computed from mermaidDiagram only. That means toggling bundling on/off (same title + same diagram) will produce different SVG bytes but the same checksum, so attachment.ResolveAttachments() will treat the remote attachment as unchanged and skip uploading the new SVG.
Include the bundle flag (and any other rendering-affecting options) in the checksum input (similar to how ProcessMermaidLocally appends the scale) so changes in bundling correctly trigger an update.
| } | ||
|
|
||
| // ProcessMermaidSVG renders a Mermaid diagram as a plain SVG file. | ||
| // The mermaid-scale flag is not applicable to SVG output. | ||
| func ProcessMermaidSVG(title string, mermaidDiagram []byte) (attachment.Attachment, error) { | ||
| return processMermaidSVG(title, mermaidDiagram, false) | ||
| } | ||
|
|
||
| // ProcessMermaidWithBundle renders a Mermaid diagram as an SVG file with the | ||
| // original diagram source embedded in the SVG <desc> element (via | ||
| // mermaid.go's WithBundle option). The resulting attachment is an SVG rather | ||
| // than a PNG, so it is resolution-independent and the source can be recovered | ||
| // from the attachment. The mermaid-scale flag is not applicable to SVG output. | ||
| func ProcessMermaidWithBundle(title string, mermaidDiagram []byte) (attachment.Attachment, error) { |
There was a problem hiding this comment.
There are existing tests for ProcessMermaidLocally, but the newly added SVG paths (ProcessMermaidSVG / ProcessMermaidWithBundle) are untested. Adding tests that assert (1) output filename extension, (2) checksum behavior (e.g., bundle vs non-bundle should not collide if checksum incorporates the option), and (3) that WithBundle actually embeds the source (e.g., contains the diagram) would help prevent regressions.
| if cmd.IsSet("mermaid-scale") && config.MermaidOutput == "svg" { | ||
| return fmt.Errorf("--mermaid-scale is not applicable when --mermaid-output=svg") | ||
| } | ||
| if cmd.IsSet("mermaid-bundle") && config.MermaidOutput == "png" { | ||
| return fmt.Errorf("--mermaid-bundle is not applicable when --mermaid-output=png") | ||
| } |
There was a problem hiding this comment.
--mermaid-output accepts an arbitrary string, but the code only branches on "svg" vs "else" and there is no validation for unsupported values. A typo like --mermaid-output=svgg will silently fall back to PNG rendering.
Consider validating MermaidOutput (e.g., only "png" or "svg") and returning a clear error for invalid values. This validation likely fits best in util.CheckFlags (Before hook) alongside the other global flag validations, rather than only here.
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
ecc2382 to
4740694
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stripUnit := func(s string) string { | ||
| // Remove trailing non-digit characters (e.g. "px", "pt", "em") | ||
| i := len(s) | ||
| for i > 0 && (s[i-1] < '0' || s[i-1] > '9') { | ||
| i-- | ||
| } | ||
| return s[:i] | ||
| } | ||
|
|
||
| w := stripUnit(attrs.Width) | ||
| h := stripUnit(attrs.Height) | ||
|
|
||
| // Fall back to viewBox ("minX minY width height") | ||
| if (w == "" || h == "") && attrs.ViewBox != "" { | ||
| parts := strings.Fields(attrs.ViewBox) | ||
| if len(parts) == 4 { | ||
| if w == "" { | ||
| w = stripUnit(parts[2]) | ||
| } | ||
| if h == "" { | ||
| h = stripUnit(parts[3]) |
There was a problem hiding this comment.
stripUnit treats relative units like 100% as pixel values by stripping % and returning "100", which can lead to incorrect layout/alignment decisions (e.g., large diagrams rendered with width="100%" will be interpreted as 100px and won’t trigger the >=760px center/wide logic). Consider treating % (and possibly other non-absolute units) as “unknown” so the code falls back to viewBox, or explicitly prefer viewBox when width/height aren’t absolute numeric pixel values.
| stripUnit := func(s string) string { | |
| // Remove trailing non-digit characters (e.g. "px", "pt", "em") | |
| i := len(s) | |
| for i > 0 && (s[i-1] < '0' || s[i-1] > '9') { | |
| i-- | |
| } | |
| return s[:i] | |
| } | |
| w := stripUnit(attrs.Width) | |
| h := stripUnit(attrs.Height) | |
| // Fall back to viewBox ("minX minY width height") | |
| if (w == "" || h == "") && attrs.ViewBox != "" { | |
| parts := strings.Fields(attrs.ViewBox) | |
| if len(parts) == 4 { | |
| if w == "" { | |
| w = stripUnit(parts[2]) | |
| } | |
| if h == "" { | |
| h = stripUnit(parts[3]) | |
| parseAbsoluteLength := func(s string) string { | |
| s = strings.TrimSpace(s) | |
| if s == "" { | |
| return "" | |
| } | |
| if strings.HasSuffix(s, "px") { | |
| s = strings.TrimSpace(strings.TrimSuffix(s, "px")) | |
| } else if s[len(s)-1] < '0' || s[len(s)-1] > '9' { | |
| // Treat relative or other non-absolute units (for example "%", "em", | |
| // "rem", "vw", "vh") as unknown so callers can fall back to viewBox. | |
| return "" | |
| } | |
| if _, err := strconv.ParseFloat(s, 64); err != nil { | |
| return "" | |
| } | |
| return s | |
| } | |
| w := parseAbsoluteLength(attrs.Width) | |
| h := parseAbsoluteLength(attrs.Height) | |
| // Fall back to viewBox ("minX minY width height") | |
| if (w == "" || h == "") && attrs.ViewBox != "" { | |
| parts := strings.Fields(attrs.ViewBox) | |
| if len(parts) == 4 { | |
| if w == "" { | |
| w = parseAbsoluteLength(parts[2]) | |
| } | |
| if h == "" { | |
| h = parseAbsoluteLength(parts[3]) |
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
4740694 to
404a913
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return attachment.Attachment{}, err | ||
| } | ||
|
|
||
| checkSum, err := attachment.GetChecksum(bytes.NewReader(append(mermaidDiagram, boolByte(bundle)))) |
There was a problem hiding this comment.
append(mermaidDiagram, ...) can mutate the caller-provided mermaidDiagram slice when it has spare capacity, which is a surprising side effect and can affect subsequent uses of the same byte slice. Build the checksum input without modifying mermaidDiagram (e.g., copy into a new slice or write into a bytes.Buffer).
| checkSum, err := attachment.GetChecksum(bytes.NewReader(append(mermaidDiagram, boolByte(bundle)))) | |
| checksumInput := make([]byte, 0, len(mermaidDiagram)+1) | |
| checksumInput = append(checksumInput, mermaidDiagram...) | |
| checksumInput = append(checksumInput, boolByte(bundle)) | |
| checkSum, err := attachment.GetChecksum(bytes.NewReader(checksumInput)) |
| // It reads the width/height attributes of the root <svg> element, stripping | ||
| // any CSS unit suffix (px, pt, em, …). If either attribute is absent or | ||
| // non-numeric, it falls back to the viewBox (third and fourth fields). |
There was a problem hiding this comment.
The comment says this strips “any CSS unit suffix (px, pt, em, …)”, but parseAbsoluteLength only strips a trailing px and treats other unit suffixes (including absolute ones like pt, cm, in) as unknown. Either broaden the parser to handle the documented units or update the comment to match the actual behavior.
| // It reads the width/height attributes of the root <svg> element, stripping | |
| // any CSS unit suffix (px, pt, em, …). If either attribute is absent or | |
| // non-numeric, it falls back to the viewBox (third and fourth fields). | |
| // It reads the width/height attributes of the root <svg> element, accepting | |
| // unitless values and stripping a trailing "px". If either attribute is | |
| // absent, uses another unit suffix, or is otherwise non-numeric, it falls | |
| // back to the viewBox (third and fourth fields). |
| break | ||
| } | ||
| if se, ok := tok.(xml.StartElement); ok && se.Name.Local == "svg" { | ||
| _ = dec.DecodeElement(&attrs, &se) |
There was a problem hiding this comment.
dec.DecodeElement(&attrs, &se) parses through the entire <svg> subtree even though only the root attributes are needed. For large SVGs this is unnecessary overhead; prefer extracting width, height, and viewBox directly from se.Attr and then stop decoding.
| _ = dec.DecodeElement(&attrs, &se) | |
| for _, attr := range se.Attr { | |
| switch attr.Name.Local { | |
| case "width": | |
| attrs.Width = attr.Value | |
| case "height": | |
| attrs.Height = attr.Value | |
| case "viewBox": | |
| attrs.ViewBox = attr.Value | |
| } | |
| } |
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
404a913 to
e057e79
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &cli.StringFlag{ | ||
| Name: "mermaid-output", | ||
| Value: "png", | ||
| Usage: "Output format for mermaid diagrams: 'png' (default, respects --mermaid-scale) or 'svg' (resolution-independent, --mermaid-scale is not applicable).", | ||
| Sources: cli.NewValueSourceChain(cli.EnvVar("MARK_MERMAID_OUTPUT"), altsrctoml.TOML("mermaid-output", altsrc.NewStringPtrSourcer(&filename))), |
There was a problem hiding this comment.
The PR description/metadata focuses on adding --mermaid-bundle to enable SVG rendering, but this change also introduces a new --mermaid-output flag and makes bundling conditional on --mermaid-output=svg. Please update the PR description to reflect the new flag and the actual enablement/validation behavior so reviewers and users don’t get conflicting guidance.
| if _, err := strconv.ParseFloat(s, 64); err != nil { | ||
| return "" | ||
| } | ||
|
|
||
| return s |
There was a problem hiding this comment.
extractSVGDimensions currently accepts floats (via strconv.ParseFloat) and then returns the original string. Downstream image layout logic (renderer/image.go’s calculateAlign/calculateLayout) uses strconv.Atoi, so values like "760.5" will be treated as non-numeric and skip the wide/full-width alignment thresholds, and may also produce non-integer ac:original-width/height values. Consider normalizing parsed values to integer pixel strings (e.g., round/truncate consistently) or returning empty strings unless the value is an integer.
| if _, err := strconv.ParseFloat(s, 64); err != nil { | |
| return "" | |
| } | |
| return s | |
| v, err := strconv.ParseFloat(s, 64) | |
| if err != nil || math.IsNaN(v) || math.IsInf(v, 0) { | |
| return "" | |
| } | |
| return strconv.Itoa(int(math.Round(v))) |
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
e057e79 to
fb582f7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fall back to viewBox ("minX minY width height") | ||
| if (w == "" || h == "") && attrs.ViewBox != "" { | ||
| parts := strings.Fields(attrs.ViewBox) | ||
| if len(parts) == 4 { | ||
| if w == "" { | ||
| w = parseAbsoluteLength(parts[2]) | ||
| } | ||
| if h == "" { | ||
| h = parseAbsoluteLength(parts[3]) | ||
| } | ||
| } |
There was a problem hiding this comment.
extractSVGDimensions() falls back to parsing the root viewBox using strings.Fields(), which only handles whitespace-separated values. The SVG spec allows comma-separated viewBox values (e.g. "0,0,640,480" or mixed comma/space), which would cause the fallback to fail and return empty width/height. Consider normalizing viewBox by replacing commas with spaces (or using a tokenizer that supports both separators) before splitting/parsing so dimensions are reliably derived.
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
fb582f7 to
84eb175
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, "svgtest", got.Replace) | ||
| assert.NotEmpty(t, got.FileBytes) | ||
| assert.True(t, strings.HasPrefix(strings.TrimSpace(string(got.FileBytes)), "<svg"), "output should be SVG") | ||
| // Dimensions should be extracted from the SVG (returned as integer pixel strings) |
There was a problem hiding this comment.
The test assumes the rendered SVG always starts with "<svg" after TrimSpace. Valid SVG output may begin with an XML declaration (e.g. "") or comments/doctype, which would make this test fail even though the output is a valid SVG. Consider parsing as XML and asserting the root element is , or relax the assertion to check that an element exists.
| svgStr := string(got.FileBytes) | ||
| assert.True(t, strings.HasPrefix(strings.TrimSpace(svgStr), "<svg"), "output should be SVG") | ||
| // WithBundle embeds the diagram source in a <desc> element | ||
| assert.Contains(t, svgStr, "<desc>", "bundled SVG should contain a <desc> element") | ||
| assert.Contains(t, svgStr, "graph TD;", "bundled SVG <desc> should contain original diagram source") | ||
| // Dimensions should be extracted from the SVG (returned as integer pixel strings) |
There was a problem hiding this comment.
This assertion is brittle: it requires the bundled output to contain the literal substring "" with no attributes/namespace. If the SVG generator emits <desc ...> or uses a namespace/prefix, this will fail while still correctly bundling. Prefer parsing the SVG and asserting that a element exists (regardless of attributes) and that its character data contains the source diagram (with appropriate unescaping).
Introduce two new flags for controlling mermaid rendering:
--mermaid-output png (default) PNG via RenderAsScaledPng;
respects --mermaid-scale
--mermaid-output svg SVG via Render(); --mermaid-scale is not
applicable and will error if set
--mermaid-bundle Embed mermaid source in the SVG <desc>
element (WithBundle). Only valid with
--mermaid-output=svg; errors if used with
--mermaid-output=png
Validation in CLI:
- --mermaid-output must be 'png' or 'svg'; any other value errors
- --mermaid-scale + --mermaid-output=svg → error
- --mermaid-bundle + --mermaid-output=png → error
Checksum correctness:
- processMermaidSVG appends a bundle-flag byte to the checksum input
so toggling --mermaid-bundle with the same diagram correctly
triggers a re-upload (same approach as PNG appending scale bytes)
SVG dimension extraction:
- extractSVGDimensions uses parseAbsoluteLength which accepts bare
numbers and px values, but treats relative units (%, em, rem, vw,
vh, …) as unknown and falls back to viewBox — prevents 100% being
misinterpreted as 100px for large-diagram alignment logic
- Dimensions stored as float strings (SVG may use e.g. "85.4375")
Changes:
- mermaid/mermaid.go: ProcessMermaidSVG (plain SVG), refactored
ProcessMermaidWithBundle sharing processMermaidSVG helper,
extractSVGDimensions with parseAbsoluteLength, boolByte helpers
- mermaid/mermaid_test.go: TestProcessMermaidSVG,
TestProcessMermaidWithBundle (use ParseFloat for dimensions),
TestExtractSVGDimensions (7 cases incl. %, em fallback),
TestSVGBundleChecksumsDiffer
- types/types.go: MermaidOutput string and MermaidBundle bool
- mark.go: same fields in Config; propagated to both cfg sites
- util/flags.go: --mermaid-output (string) and --mermaid-bundle (bool)
with Usage strings noting applicability constraints
- util/cli.go: wire both flags; validate output value and incompatible
combinations
- renderer/fencedcodeblock.go: branch on MermaidOutput + MermaidBundle
- README.md: document SVG output options and update flags table
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mrueg
force-pushed
the
feature/mermaid-bundle
branch
from
84eb175 to
fad2e03
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add support for SVG rendering of Mermaid diagrams, with an optional bundled source variant.
New flags
--mermaid-output(env:MARK_MERMAID_OUTPUT, toml:mermaid-output): selects the output format for Mermaid diagrams. Accepted values:png(default, respects--mermaid-scale) orsvg(resolution-independent;--mermaid-scaleis not applicable and will error if set together).--mermaid-bundle(env:MARK_MERMAID_BUNDLE, toml:mermaid-bundle): when set, embeds the original Mermaid diagram source in the SVG<desc>element using mermaid.go'sWithBundle()option. Only valid with--mermaid-output=svg; errors if used with--mermaid-output=png.Validation
--mermaid-outputmust bepngorsvg; any other value returns a clear error.--mermaid-scale+--mermaid-output=svg→ error (scale is PNG-only).--mermaid-bundle+--mermaid-output=png→ error (bundle is SVG-only).Implementation details
ProcessMermaidSVG()(plain SVG) andProcessMermaidWithBundle()(SVG + embedded source) tomermaid/mermaid.go, sharing a commonprocessMermaidSVGhelper.width/heightattributes (accepting bare numbers andpxsuffix) with aviewBoxfallback for relative units (e.g.%,em). Values are normalised to integer pixel strings (viamath.Round) so they are compatible withstrconv.Atoiin the downstream layout/alignment logic.--mermaid-bundlecorrectly triggers a Confluence re-upload.MermaidOutput stringandMermaidBundle boolfields totypes.MarkConfigandmark.Config, wired throughutil/flags.go,util/cli.go, and both config construction sites inmark.go.renderer/fencedcodeblock.goto dispatch to the correct render function based on the flags.In the spirit of #719