fix: render flows whose nodes have no position instead of crashing the canvas - #14258
Merged
Merged
Conversation
…e canvas Flows whose `data.nodes` lack a `position` crashed the canvas with a full-page error boundary. The auto-layout fallback that exists for exactly this case (`needsLayout` + `getLayoutedNodes`) was broken in two independent ways. 1. `getLayoutedNodes` passed `id: undefined` to ELK — for edges (`id: e.id`) and for ports derived from absent handles (`id: e.sourceHandle`). ELK rejected the graph with `JsonImportException: Id must be a string or an integer: 'null'`, so the layout failed for precisely the flows that needed it. Both id sources had to be fixed; repairing either alone still threw. Handle-less ports are now dropped (their edges fall back to the always-present node-id port) and edges without an id get a stable synthetic one. A failed layout now falls back to a deterministic grid rather than leaving nodes unpositioned. 2. `processFlows` iterates with `forEach(async …)`, which discards the returned promise, so it returns before the layout resolves and `useApplyFlowToCanvas` hands position-less nodes straight to React Flow — whose `getNodePositionWithOrigin` dereferences `node.position.x` unguarded. This crashed even when ELK succeeded. `processDataFromFlow` now seeds deterministic positions synchronously before awaiting ELK, so no node can reach React Flow without one. `needsLayout` also now rejects non-numeric and NaN coordinates, not just an absent `position`. A payload with this shape is served by `GET /api/v1/starter-projects/`, whose nodes have no `position` and whose edges have no `id`/`sourceHandle`/ `targetHandle`; POSTing one to `/api/v1/flows/` and opening it reproduced the crash. Position-less flows can also arrive from hand-written JSON, older exports, and third-party tooling.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughFrontend auto-layout now validates node positions, handles missing ELK edge and port identifiers, falls back to deterministic grid placement on layout failure, and synchronously seeds positions before asynchronous layout completes. ChangesAuto-layout resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FlowProcessor
participant PositionFallback
participant LayoutEngine
participant ReactFlow
FlowProcessor->>PositionFallback: seed positions for invalid nodes
PositionFallback-->>FlowProcessor: numeric grid positions
FlowProcessor->>ReactFlow: provide immediately positioned nodes
FlowProcessor->>LayoutEngine: compute asynchronous layout
LayoutEngine-->>FlowProcessor: return laid-out nodes or failure
Suggested labels: 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Test Coverage AdvisorNo source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉
|
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/utils/__tests__/layoutUtils.test.ts`:
- Around line 83-104: Extend the “getLayoutedNodes with handle-less edges” tests
with a focused error-case test that mocks or spies on elk.layout to reject, then
verifies getLayoutedNodes still returns all nodes with numeric grid positions.
Preserve the existing successful-layout assertions and use the module’s
established ELK dependency injection or mocking approach.
In `@src/frontend/src/utils/reactflowUtils.ts`:
- Around line 615-618: Update the coordinate validation predicate in
reactflowUtils.ts at lines 615-618 to use Number.isFinite for both
node.position.x and node.position.y, preserving the existing layout-required
behavior for invalid coordinates. In
src/frontend/src/utils/__tests__/layoutUtils.test.ts at lines 77-80, add an
assertion verifying that a node with x: Infinity requires layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a769df13-0176-4d06-b7a2-4a4f0003cc11
📒 Files selected for processing (3)
src/frontend/src/utils/__tests__/layoutUtils.test.tssrc/frontend/src/utils/layoutUtils.tssrc/frontend/src/utils/reactflowUtils.ts
Addresses review feedback on #14258. `needsLayout` used a typeof + NaN check, which let `Infinity` through: it is a number and is not NaN, so a node with an infinite coordinate skipped the synchronous seeding and could still reach React Flow as an unusable position. `Number.isFinite` does not coerce, so it subsumes the typeof check while also rejecting NaN and ±Infinity. Also adds the missing error-path coverage for the ELK fallback: the previous tests only exercised successful layouts, never the catch branch. The new test mocks elk.layout to reject and asserts getLayoutedNodes resolves to the deterministic grid rather than propagating. Both tests fail without their corresponding fix.
github-actions
Bot
added
bug
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-1.12.0 #14258 +/- ##
=================================================
Coverage 61.26% 61.27%
=================================================
Files 2347 2341 -6
Lines 238587 236643 -1944
Branches 35590 33294 -2296
=================================================
- Hits 146180 145005 -1175
+ Misses 90585 89875 -710
+ Partials 1822 1763 -59
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
1 participant
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.
Fixes #14257.
Problem
Any flow whose
data.nodeslack apositioncrashes the canvas with a full-page "Sorry, we found an unexpected error!" when opened at/flow/{id}.The frontend already intends to handle this —
needsLayout()+getLayoutedNodes()exist precisely to lay out position-less flows — but the fallback is broken in two independent ways, so it never recovers.1.
getLayoutedNodespassesid: undefinedto ELK. Edges are mapped withid: e.id, and ports are derived withid: e.sourceHandle/id: e.targetHandle. When a flow's edges have none of those, ELK rejects the graph withJsonImportException: Id must be a string or an integer: 'null'— so the layout fails for exactly the flows that need it. Bisected against a real payload, both id sources must be fixed; repairing either alone still throws:2. The layout is applied too late — the actual root cause of the crash.
processFlowsiterates withforEach(async (flow) => { … await processDataFromFlow(…) });forEachdiscards the returned promise, soprocessFlowsreturns before the layout resolves.useApplyFlowToCanvasthen callssetCurrentFlow()immediately →resetFlow()→ position-less nodes reach the store → React Flow'sadoptUserNodescallsgetNodePositionWithOrigin, which dereferencesnode.position.xunguarded. This crashes even when ELK succeeds, so fixing the ids alone is not sufficient.Changes
layoutUtils.ts— drop ports whose handle is absent (their edges fall back to the always-present node-id port) and give id-less edges a stable synthetic id, so ELK accepts the graph. Wrapelk.layoutso a failure returns a deterministic grid via the new exportedgetFallbackGridPositionsinstead of leaving nodes unpositioned.reactflowUtils.ts—processDataFromFlowseeds deterministic positions synchronously before awaiting ELK, so no node can reach React Flow without one regardless of the async timing.needsLayoutnow also rejects non-numeric andNaNcoordinates, not just an absentposition.Testing
New
src/frontend/src/utils/__tests__/layoutUtils.test.ts(10 cases) covering the fallback grid, the tightenedneedsLayout, ELK layout with handle-less edges, and the synchronous guarantee through the realprocessFlows.src/utilssuite green: 422 tests / 30 suites.tsc --noEmitclean;biome checkclean.Verified end to end against all five
GET /api/v1/starter-projects/payloads (4–9 nodes each): before the fix every node was unpositioned and ELK threw; after, each renders with a real layered layout.Note on scope
This stops the crash and produces a correct layout from
getLayoutedNodes. It does not changeprocessFlows' fire-and-forget signature, so on theuseApplyFlowToCanvaspath the canvas renders the synchronously-seeded grid rather than the ELK result —resetFlowcaptures the node array by reference before the async layout rebindsdata.nodes:Threading
awaitthroughprocessFlowstouches 8 call sites, so I kept it out of this PR to keep the crash fix small and reviewable. Happy to follow up with it if you'd like.Adjacent data-quality bugs (not fixed here)
Two more issues visible in the same
/api/v1/starter-projects/response, noted in #14257 — both look unintended and are backend-side:nameis alwaysnull(Graph.dump()only sets it whenself.flow_nameis truthy).endpoint_nameis the literal string"None"(str(endpoint_name)onNoneinlfx/graph/graph/base.py).Let me know if you'd like those in scope and I'll add them.
Summary by CodeRabbit
NaNnode positions.