Repository navigation
Common Render pipeline 2: DAGRE - phase 1 - #7807
Conversation
🦋 Changeset detectedLatest commit: a0449a4 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 |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
…agre' into codex/common-render-pipeline-3-dagre-phases
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
knsv-bot
left a comment
There was a problem hiding this comment.
[sisyphus-bot]
Reviewing as a draft, so this is direction/early-red-flags rather than line-polish — posting as a comment, no blocking asks. This is phase 1 of the common-render-pipeline migration: removing the deprecated dagre-wrapper/ and routing block diagrams through the shared rendering-elements. Net +378 / −4801 — a satisfying amount of red.
What's working well
🎉 The removal is clean and complete. The whole dagre-wrapper/ tree (22 files) is gone with zero dangling imports — I grepped the tree on this branch and there are no remaining import … from '…/dagre-wrapper/…' statements (the surviving 'dagre-wrapper' hits are the defaultRenderer config string enum, which is correct to keep). renderHelpers.ts is cleanly re-pointed to rendering-util/rendering-elements/{edges,nodes}.js, and CI is fully green (unit-test, e2e 5/5, build-docs, lint).
🎉 classRenderer-v2.ts deletion is correctly identified as dead code. Nothing references it on either develop or this branch — the class detectors already route to classRenderer-v3-unified. Good call removing it rather than carrying it forward.
🎉 The blockArrowHelper.ts → shapes/blockArrow.ts rename improves typing on the way over — node: any becomes a proper BlockArrowNode, return types are annotated, and getArrowPoints is correctly de-exported (no external users). Nice to see a move that also tightens the types instead of just relocating them.
🎉 Schema change is honest housekeeping. The only config.schema.yaml edit removes stale comments that pointed at now-deleted classRenderer-v2.ts line numbers — no field change, so config.type.ts correctly didn't need regenerating, and types:verify-config stays green.
🎉 Tech debt is acknowledged, not hidden — e.g. the @ts-expect-error on the ellipse case links to #5976 rather than silently papering over it.
Heads up for when you mark this ready
💡 The PR bundles a shared-shape behavior change into a "preserve render behavior" removal. Beyond the mechanical move, six shared flowchart shapes (trapezoid, leanLeft, leanRight, subroutine, invertedTrapezoid, rectLeftInvArrow) all get the same sizing change — from node.height ?? bbox.height to Math.max(bbox.height + pad, node.height ?? 0) (and the analogous width). renderHelpers.ts also changes the block default shape '' → 'rect' and adds cssCompiledStyles. That consistent Math.max min-size pattern looks intentional (likely needed so block diagrams respect explicitly-set node dimensions now that they route through the unified shapes) — and it's almost certainly the source of the 36 changed Argos snapshots.
Two things would help land this safely:
- Call the visual change out explicitly in the PR description — the summary currently says "preserving the current DAGRE core/render behavior," but these are shared flowchart shapes, so the diff isn't behavior-neutral.
- If the shape-sizing change is separable from the dagre-wrapper removal, splitting it into its own PR keeps phase 1 mechanically pure and makes the 36 visual diffs reviewable on their own merits (and any regression bisectable). If it's not separable (i.e. block-via-unified-shapes requires it), then a quick maintainer eyeball confirming those 36 diffs are intended min-size improvements — not clipping/overflow regressions on existing flowcharts — is the thing to do before merge.
Security
🎉 XSS/injection scan: No XSS or injection issues identified. This is a deletion-heavy refactor that preserves the prior sanitization path — the moved block_arrow render fn's .attr('style', node.style ?? null) is byte-identical to the old dagre-wrapper version (the ?? null is a minor safety improvement), labels still flow through the shared labelHelper/sanitizeText pipeline (untouched), and the deleted classRenderer-v2.ts lost no live sanitization (dead code). No new DOM/SVG sinks in any added line.
Verification I ran
Checked out the PR, confirmed no dangling dagre-wrapper file imports and that classRenderer-v2.ts is gone, verified renderHelpers.ts imports rewired + still in use, confirmed the Math.max sizing change is consistent across all six shapes, confirmed getArrowPoints has no external importers, and confirmed the schema edit is comment-only. CI: unit-test / e2e (5/5) / build / lint all green; Argos 36 changed, approved.
Severity tally: 🔴 0 / 🟡 0 / 🟢 0 / 💡 1 / 🎉 5
Really clean phase 1 — the only thing I'd want before it goes non-draft is clarity on those 36 visual diffs. Nice work paying down dagre-wrapper.
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…-render-pipeline-3-dagre-phases
…/common-render-pipeline-4-dagre-layoutdata
…odex/common-render-pipeline-5-dagre-shared-paint
… codex/common-render-pipeline-6-dagre-shared-nodes
… codex/common-render-pipeline-6-dagre-shared-nodes
… codex/common-render-pipeline-6-dagre-shared-nodes
…agre-shared-nodes' into codex/common-render-pipeline-6-dagre-shared-nodes
…/common-render-pipeline-4-dagre-layoutdata
…odex/common-render-pipeline-5-dagre-shared-paint
… codex/common-render-pipeline-6-dagre-shared-nodes
… codex/common-render-pipeline-6-dagre-shared-nodes
Address review feedback on the LayoutData normalization: - Document that applyDagreLayoutResult only normalizes the top-level graph: recursive cluster children and intra-cluster edges live in the nested cluster graph and are owned by the recursive paint path, and unmerged self-loop segments replace the original self-edge. - Pin the recursive-cluster behavior with a unit test so the next pipeline phase does not build on assumptions the data does not satisfy. - Note on DAGRE_NODE_LAYOUT_PROPERTIES that the allowlist must stay a fixed literal list (graph keys are user-controlled) and why the intersect function references are copied. - Rename the now-used _data4Layout parameter in runDagreLayoutCore.
…odex/common-render-pipeline-5-dagre-shared-paint
… codex/common-render-pipeline-6-dagre-shared-nodes
…cleanups - Add changeset documenting the class.defaultRenderer switch to the unified (dagre-wrapper) renderer - Drop the shared EMPTY_CLUSTER_DB singleton in favor of a fresh Map per paint call to avoid a mutable module-level default - Document why class.padding is intentionally left undefined in defaultConfig - Add hand-drawn self-referential class diagram e2e snapshot test
…-6-dagre-shared-nodes Common Render pipeline 6: DAGRE shared node measurement
…-5-dagre-shared-paint Common Render pipeline 5: DAGRE shared paint
…-4-dagre-layoutdata Common Render pipeline 4: DAGRE LayoutData normalization
…agre' into pr-7808-fix # Conflicts: # docs/config/setup/defaultConfig/variables/configKeys.md # docs/config/setup/mermaid/functions/createCommonLayoutRenderer.md # docs/config/setup/mermaid/interfaces/CommonLayoutPaintContext.md # docs/config/setup/mermaid/interfaces/CommonLayoutPaintOptions.md # docs/config/setup/mermaid/interfaces/CommonLayoutRenderContext.md # docs/config/setup/mermaid/interfaces/CommonLayoutRendererDefinition.md
…-3-dagre-phases Common Render pipeline 3: DAGRE phase 3
Summary
Validation
Stacked on #7803 / codex/elk-prep-extraction.