fix(ui): drop nodeDepth partitioning so ELK derives lineage layers from edges#29224
Open
chirag-madlani wants to merge 9 commits into
Open
fix(ui): drop nodeDepth partitioning so ELK derives lineage layers from edges#29224chirag-madlani wants to merge 9 commits into
chirag-madlani wants to merge 9 commits into
Conversation
…om edges Pinning each node to its backend nodeDepth via elk.partitioning forced a multi-branch node into the column of its first depth, misaligning its other edges. Let ELK's layered algorithm derive layers from edge topology and use BRANDES_KOEPF node placement for a cleaner, less rigid horizontal flow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
Contributor
Contributor
🔴 Playwright Results — 1 failure(s), 11 flaky✅ 4018 passed · ❌ 1 failed · 🟡 11 flaky · ⏭️ 84 skipped
Genuine Failures (failed on all attempts)❌
|
BRANDES_KOEPF aligned a node to one child (top-biased), so a source with children Y and Z sat parallel to Y instead of vertically centered. Switch node placement to NETWORK_SIMPLEX, which assigns cross-axis coordinates by minimizing weighted edge deviation, keeping a source balanced between its children. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
NETWORK_SIMPLEX (and bare Brandes-Köpf) resolve a symmetric source's placement to a top-aligned corner because all alignments tie on edge length, so the source did not sit centered between its upstream/downstream branches. Setting elk.layered.nodePlacement.bk.fixedAlignment to BALANCED averages the four extreme alignments into a centroid, restoring vertical centering while keeping edge-aware, straight-segment placement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ulixius9
previously approved these changes
Jun 22, 2026
Rohit0301
previously approved these changes
Jun 22, 2026
The new BALANCED Brandes-Köpf placement repositions nodes (source nodes now sit vertically centered among their children), so the exported lineage PNG no longer matches the old reference. Updated the snapshot to the new layout; edges remain present, so the regression guard for Issue #29124 still holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ ApprovedRemoves ELK partitioning to resolve visual lineage graph misalignment and cyclic graph crashes, transitioning to topology-driven layering with the BRANDES_KOEPF placement strategy. Updated unit tests confirm the new layout configuration is correctly implemented. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Describe your changes:
Fixes #29225
I dropped the
nodeDepth-based ELK partitioning in the lineage graph and let ELK'slayeredalgorithm derive layers from edge topology (withBRANDES_KOEPFnode placement) because pinning each node to its single backendnodeDepthforced a multi-branch node into the column of its first depth while its other edges fanned out to different columns, producing a visually misaligned graph. The result is a cleaner, less rigid horizontal flow, and it also removes the partition-on-cyclic-graph crash source.Type of change:
High-level design:
N/A — small change (3 files): removed the per-node
elk.partitioning.partitionhint ingetELKLayoutedElements, and removedelk.partitioning.activatewhile switchingelk.layered.nodePlacement.strategyfromSIMPLEtoBRANDES_KOEPFinELKUtil. Focus/root centering is unaffected — it is handled separately bycenterNodePosition, not by partitioning.Tests:
Use cases covered
Unit tests
ELKUtil.test.tsto match the new layout options (19/19 pass).Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
ELKUtilunit suite passes.nodeDepthin layout and that focus centering is independent.UI screen recording / screenshots:
Issue:
Screen.Recording.2026-06-22.at.5.50.17.PM.mov
Fix:
Screen.Recording.2026-06-22.at.5.50.57.PM.mov
Checklist:
Greptile Summary
This PR removes ELK's
nodeDepth-based partitioning from the lineage graph layout and replaces it with topology-driven layer assignment using theBRANDES_KOEPFnode placement strategy withBALANCEDalignment. The fix addresses misalignment when a node is reachable at multiple depths and also eliminates a potential crash when partitioning is applied to cyclic graphs.ELKUtil.ts: SwapsSIMPLE+elk.partitioning.activateforBRANDES_KOEPF+elk.layered.nodePlacement.bk.fixedAlignment: BALANCED; well-commented with rationale.EntityLineageLayoutUtils.ts: Removes the per-nodeelk.partitioning.partitionhint derived fromnode.data.nodeDepth, sonodeDepthis now unused in layout computation (the backend data still flows in harmlessly).ELKUtil.test.ts: Unit tests updated in full (19/19) to match the new layout options; snapshot updated to reflect the changed graph positions.Confidence Score: 5/5
Safe to merge — a focused, well-reasoned swap of ELK layout options with no behavioral regressions expected.
The change is minimal and surgical: two ELK options removed, two added, one dead variable dropped. The new BRANDES_KOEPF + BALANCED configuration is a well-established ELK layered-algorithm setting, the fix is correctly motivated, inline comments explain the why, unit tests cover the exact options changed, and the Playwright snapshot is updated. No data paths, API contracts, or backend interactions are touched.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[getELKLayoutedElements] --> B[Map nodes to ElkNode] A --> C[Map edges to ElkExtendedEdge] B --> D[ELKLayout.layoutGraph] C --> D D --> E[ELK layered algorithm] E --> F{Node Placement Strategy} F -->|BEFORE removed| G[SIMPLE plus partitioning per nodeDepth] F -->|AFTER this PR| H[BRANDES_KOEPF plus BALANCED alignment] G --> I[Misaligned graph or cyclic crash] H --> J[Clean topology-driven layout] J --> K[updatedNodes with x/y positions]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[getELKLayoutedElements] --> B[Map nodes to ElkNode] A --> C[Map edges to ElkExtendedEdge] B --> D[ELKLayout.layoutGraph] C --> D D --> E[ELK layered algorithm] E --> F{Node Placement Strategy} F -->|BEFORE removed| G[SIMPLE plus partitioning per nodeDepth] F -->|AFTER this PR| H[BRANDES_KOEPF plus BALANCED alignment] G --> I[Misaligned graph or cyclic crash] H --> J[Clean topology-driven layout] J --> K[updatedNodes with x/y positions]Reviews (7): Last reviewed commit: "Merge branch 'main' into fix-lineage-lay..." | Re-trigger Greptile