Improve flow builder auto-layout and lazy-load the ELK engine - #4153
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesELK Auto-layout graph construction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReactFlow
participant applyAutoLayout
participant ELK
ReactFlow->>applyAutoLayout: provide nodes and edges
applyAutoLayout->>ELK: submit fixed-side ports and prioritized edges
ELK-->>applyAutoLayout: return layout coordinates
applyAutoLayout-->>ReactFlow: return positioned nodes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
739c99c to
ff3d628
Compare
DonOmalVindula
changed the title
ff3d628 to
bbcb7ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/apps/console/src/features/flows/utils/__tests__/applyAutoLayout.test.ts (1)
215-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't verify handle-to-side correspondence.
Sorting
sidesbefore comparing to['NORTH', 'SOUTH']only checks that both sides exist somewhere, not thatfailure→SOUTHandexec_INCOMPLETE→NORTHspecifically. A swapped mapping would still pass.🧪 Suggested stronger assertion
const byId = new Map(lastGraph?.children.map((child) => [child.id, child])); const ports = byId.get('exec')?.ports ?? []; - const sides = ports.map((port) => port.layoutOptions['elk.port.side']).sort(); - - expect(sides).toEqual(['NORTH', 'SOUTH']); + + expect(ports.find((port) => port.id === 'exec__failure')?.layoutOptions['elk.port.side']).toBe('SOUTH'); + expect(ports.find((port) => port.id === 'exec__exec_INCOMPLETE')?.layoutOptions['elk.port.side']).toBe('NORTH');🤖 Prompt for 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. In `@frontend/apps/console/src/features/flows/utils/__tests__/applyAutoLayout.test.ts` around lines 215 - 233, Update the test around applyAutoLayout to assert each port’s side by its handle identifier, rather than sorting the collected sides. Verify that the failure handle maps to SOUTH and the exec_INCOMPLETE handle maps to NORTH, so swapped port assignments fail the test.
🤖 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 `@frontend/apps/console/src/features/flows/utils/applyAutoLayout.ts`:
- Around line 55-75: The sourcePortSide helper currently ignores the layout
direction, causing DOWN/UP layouts to assign incorrect port sides. Update
sourcePortSide and its callers to derive EAST/WEST/NORTH/SOUTH from the active
direction while preserving the failure, incomplete, previous, and default handle
distinctions; alternatively remove or narrow the direction option if only RIGHT
is supported.
---
Nitpick comments:
In
`@frontend/apps/console/src/features/flows/utils/__tests__/applyAutoLayout.test.ts`:
- Around line 215-233: Update the test around applyAutoLayout to assert each
port’s side by its handle identifier, rather than sorting the collected sides.
Verify that the failure handle maps to SOUTH and the exec_INCOMPLETE handle maps
to NORTH, so swapped port assignments fail the test.
🪄 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: 57f7059f-f7f7-44af-9dca-8cb820338255
📒 Files selected for processing (2)
frontend/apps/console/src/features/flows/utils/__tests__/applyAutoLayout.test.tsfrontend/apps/console/src/features/flows/utils/applyAutoLayout.ts
bbcb7ef to
03b68f9
Compare
Model canvas handles as fixed-side ELK ports, straighten the success path, and drop the per-type post-processing pass that tore layouts apart. Load the ~1.5MB ELK engine on demand instead of bundling it into the builder chunk, clearing the cached import on failure so layout can retry. Refs thunder-id#3871
03b68f9 to
c343923
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3 participants
Purpose
Auto-layout in the flow builder sometimes produces incoherent results: Call nodes stranded far off the main line, long edge detours from view links, and inconsistent vertical alignment (see the issue's parent, flow builder UX). Refs #3871.
Two root causes:
'EXECUTION'while canvas executor nodes have type'TASK_EXECUTION', so that branch never ran, andCALL/RULEnodes were not handled at all. The result mixed two coordinate systems: some node types were moved after the fact while others kept positions from ELK's original (now torn apart) layout. This is exactly the "floating Call node" symptom.Approach
Give ELK the canvas semantics and trust its output, instead of correcting it afterwards:
*_NEXT→ east,failure→ south,*_INCOMPLETE→ north,*_PREVIOUS→ west; targets are west ports). ELK then places failure branches below their source and routes view-link branches sensibly.elk.layered.priority.straightness, which the NETWORK_SIMPLEX placement honors, so the main flow reads as one left-to-right row with branches hanging off it.Verified against real ELK (not just the test mock) with a graph mirroring the reported layout: the executor chain lands on a single aligned row, START aligns with the view it feeds, and both CALL nodes stack cleanly below the credentials column.
The test suite is rewritten to assert the semantics handed to ELK (ports, layer constraints, straightness priorities, dedup) plus uniform position mapping, rather than the removed post-pass behavior.
Note for reviewers: layout quality is visual; a manual pass on the starter templates and a flow with link widgets (self sign up / recovery) is recommended.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit