Skip to content

Add error handling for dagre layout and fallback to simple layout#55

Open
cxzl25 wants to merge 1 commit intodataflint:mainfrom
cxzl25:fix_ui_layout
Open

Add error handling for dagre layout and fallback to simple layout#55
cxzl25 wants to merge 1 commit intodataflint:mainfrom
cxzl25:fix_ui_layout

Conversation

@cxzl25
Copy link
Copy Markdown
Contributor

@cxzl25 cxzl25 commented Mar 19, 2026

Failed to layout SQL flow: Cannot set properties of undefined (setting 'order')

https://github.com/dagrejs/dagre/issues/234
image

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves resilience of the SQL flow graph layout by handling Dagre compound-layout failures and falling back to a simpler layout, preventing the UI from hard-failing on known Dagre compound graph edge cases.

Changes:

  • Wrap compound dagre.layout(g) in try/catch inside the staged (compound) layout path.
  • On failure, log a warning and fall back to the existing “simple” (non-compound) Dagre layout.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dagre.layout(g);
} catch (e) {
console.warn("Compound dagre layout failed, falling back to simple layout", e);
return getLayoutedElements(flowNodes, flowEdges, cacheKey + "-fallback");
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback layout is cached under cacheKey + "-fallback", but getLayoutedElementsWithStages (and SqlElementsToLayout) look up layoutCache using the original cacheKey. If dagre compound layout keeps failing for a given graph, this will re-run the failing layout and emit console.warn on every call (performance + log spam) instead of reusing the fallback.

Consider caching the fallback result under the original cacheKey (e.g., compute fallback once, layoutCache.set(cacheKey, fallbackResult), then return it), and/or avoid the -fallback suffix so subsequent calls immediately reuse the fallback without retrying the broken compound layout.

Suggested change
return getLayoutedElements(flowNodes, flowEdges, cacheKey + "-fallback");
return getLayoutedElements(flowNodes, flowEdges, cacheKey);

Copilot uses AI. Check for mistakes.
@minskya
Copy link
Copy Markdown
Contributor

minskya commented Mar 24, 2026

@cxzl25 where and how this change was required? I'd like to understand better the scenario. Thanks

@cxzl25
Copy link
Copy Markdown
Contributor Author

cxzl25 commented Mar 25, 2026

where and how this change was required? I'd like to understand better the scenario.

When I open some complex SQL, it reports this error. This SQL has 5,000 lines and 350,000 characters.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants