fix(elk): scope rounded edge curve to ELK layout only#7452
fix(elk): scope rounded edge curve to ELK layout only#7452
Conversation
… LR GitGraph The background rectangle for branch labels was misaligned with the text when branch names contained multi-line text (e.g. "Feature A\n(ongoing)") in LR layout. The old transform used `pos - bbox.height / 2` which caused the rect to shift further up as text height increased, while the text position remained centered at `pos - 1`. Fixed by using a constant y-offset (`pos - 11`) in the transform, derived from the constraint that the rect center must equal the text center regardless of bbox.height. Closes #7362
Fix the TypeScript types for `createText.ts`, allowing us to remove the `@ts-nocheck` statement. I've also made sure to get rid of all the `any` types in this file.
I'm not exactly sure why `decodeEntities()` is needed and what it does, but I added a brief comment to make it a bit more clear that it's **NOT** decoding HTML entities, but just decoding what `encodeEntities` is doing.
In the `createText()` function, we are calling `decodeEntities()` on the input if `htmlLabels: true`, but we weren't doing it `htmlLabels: false` was set. I can't seem to find anywhere this is actually impacting diagrams, since we're calling `decodeEntities()` already normally before calling `createText()`, hence why this is just a `refactor` commit. Original-commit: #7297 Co-authored-by: chandershekhar22 <chandersj.it.22@nitj.ac.in>
When creating labels using `htmlLabels: false`, e.g.
```mermaid
---
config:
htmlLabels: false
---
flowchart TD
A[2 < 4 && 12 > 14]
```
The SVG node label gets rendered as
`2 < 4 && 12 > 14`. This is fine for HTML text, where we
use `.innerHTML` to set the value. But for non-HTML Labels, we use
`.textContent`, so we need to pass the unescaped values.
Ideally we would stop calling DOMPurify on this label when
`.textContent` is used, since the content doesn't need to be sanitized,
but adding a quick `<`/`>`/`&`-> `<`/`>`/`&` also works.
I've adapted this commit from #6406.
Closes: #6406
Co-authored-by: khalil <5alil.landolsi@gmail.com>
TypeScript wasn't catching these earlier, since `bbox` was `any`, but now that it's been typed correctly, all the `node.padding` uses in expressions with `bbox` are throwing TypeScript errors.
According to MDN, this is only `null` if the `Node` is a `Document`. I'm not 100% sure why TypeScript is throwing an error on this, since my VS Code shows that `Element.textContent` will never be `null`, since `Document` can never be an `Element`, but we do have other non-null assertions for this scattered over the place. See: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
We know that the output of `createText` when `htmlLabels: true` is set is a `<foreignObject>` that contains a `<div>`, so this can fix our TypeScript errors.
The sequence diagram JISON parser had no catch-all rule in the exclusive ID lexer state. When input like `participant X asAlias:text` was parsed (missing space after "as"), none of the ID-state rules could match due to the colon terminating the character class match before end-of-line. This caused a lexer error that could hang browsers depending on error recovery behavior. Add a catch-all `<ID>[^\n]+` rule that produces an INVALID token when no other ID-state rule matches, ensuring the parser always makes progress and produces a clean parse error. Resolves #6399 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replacing `<` and `>` in requirement shapes is no longer necessary, now that `createText` does it automatically when `htmlLabels: false`.
…n-htmlLabels-false fix: prevent escaping `<` and `&` when `htmlLabels: false`
…label-alignment fix: align branch label background with text for multi-line labels in LR GitGraph
fix: pie: Don't sort, keep order. Keep color order constant.
…ng_space_after_as fix(sequence): prevent hang when "as" lacks trailing space
Reverts the global flowchart curve default from 'rounded' back to 'basis' and instead applies 'rounded' only in the ELK render when no explicit interpolation is configured. Non-ELK layouts are unaffected. Fixes #7213 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ee0ade3 The changes in this PR will be included in the next version bump. 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 |
github-actions
bot
added
the
Type: Bug / Error
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
The previous commit set 'rounded' on edgeData which is not passed to insertEdge. The actual edge object (from ELK output) already has curve:'basis' set by flowDb.getData(), so it must be overridden unconditionally right before the insertEdge call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ashishjain0512
left a comment
ashishjain0512
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
This is a substantial PR that bundles the ELK edge curve scoping with several other fixes (createText typing, pie chart ordering, sequence hang, gitGraph label alignment). Reviewing the full scope here.
Note: PR targets master rather than develop. This appears to be an intentional hotfix/release PR given it's authored by knsv-bot and approved by @knsv.
File Triage
| Tier | Count | Files |
|---|---|---|
| Tier 1 (full read) | 1 | createText.ts |
| Tier 2 (diff + context) | 8 | render.ts, pieRenderer.ts, sequenceDiagram.jison, gitGraphRenderer.ts, shapeUtil.ts, shapes/util.ts, test files |
| Tier 3 (diff only) | 5 | changesets, .gitignore, cypress specs |
| Skip | 12 | shape files (mechanical node.padding → (node.padding ?? 0) null-safety) |
What's working well
🎉 [praise] The createText.ts cleanup is excellent — removing @ts-nocheck and eslint-disable @typescript-eslint/no-explicit-any, then adding proper types throughout. This makes the shared rendering code significantly more maintainable. The D3Selection typing and non-null assertions (node()!) are well-placed.
🎉 [praise] The decodeHTMLEntities() function (createText.ts:189) is well-designed: limited to the three entities that sanitizeText encodes, clearly scoped to the .text() (textContent) path, and includes a helpful TODO comment about potentially skipping sanitization entirely for this path. Good defensive design.
🎉 [praise] The ELK fix is properly scoped (render.ts:1073) — setting edge.curve = 'rounded' only in the ELK render loop, with a clear comment explaining why. This avoids the regression from the previous global default change.
🎉 [praise] The pie chart fix (pieRenderer.ts:17,96-98) correctly preserves insertion order with .sort(null) and sets an explicit .domain([...sections.keys()]) to maintain color consistency when slices are filtered out. The unit tests (pieRenderer.spec.ts) verify both ordering and color stability — thoughtful coverage.
🎉 [praise] The sequence diagram hang fix (sequenceDiagram.jison:39) is a clean catch-all that prevents the lexer from looping infinitely when as is used without a trailing space. The unit test has a 5-second timeout guard — good.
Things to address
🟡 [important] The PR checklist shows unchecked items for Cypress e2e snapshots and CI. The createText.ts changes affect the shared rendering pipeline used by all diagram types — rendering-util/ modifications can regress any diagram. Per project conventions, full e2e verification (pnpm e2e) is needed before merge. The new Cypress tests for flowchart angle brackets and gitGraph multi-line labels are great, but the shape null-safety changes touch 12 shape files and should be verified visually too. Worth confirming CI passes cleanly.
🟢 [nit] gitGraphRenderer.ts:880 — pos - 11 replaces pos - bbox.height / 2. The magic number 11 is fragile and will break with different font sizes or scaling. A brief comment explaining the derivation (e.g., "empirically matches single-line label height") would help future maintainers.
🟢 [nit] requirementBox.ts:201 — The manual .replaceAll('>', '>') / .replaceAll('<', '<') is removed. This is correct since decodeHTMLEntities in createText.ts now handles this upstream, but it's worth confirming the requirementBox rendering path goes through createText for its labels (I believe it does via labelHelper → createText).
Security
No XSS or injection issues identified. The decodeHTMLEntities() function outputs exclusively to D3's .text() method (which uses textContent, inherently safe against XSS). The decodeEntities() addition on line 329 is similarly safe — it feeds only into the non-HTML label path. No decoded content can reach any .html() or innerHTML sink through any code path introduced by this PR. The existing addHtmlSpan path with .html() is unaffected — it continues to use sanitizeText() before insertion.
Self-check
- At least one 🎉 [praise] item exists (5)
- No duplicate comments
- Severity tally: 0 🔴 blocking / 1 🟡 important / 2 🟢 nit / 0 💡 suggestion / 5 🎉 praise
- Verdict: COMMENT (1 🟡 — e2e verification needed)
- Not a draft PR
- No inline comments used
- Tone check: constructive, appreciative, actionable
6 participants
Summary
Scopes the fix from #7425 (issue #7213) so that the
roundedcurve is only applied when the ELK layout algorithm is used, rather than changing the global default for all flowcharts.flowchart.curvedefault from'rounded'back to'basis'edgeData.curveto'rounded'when the user hasn't explicitly configured an interpolationResolves #7213
Test plan
🤖 Generated with Claude Code