fix(node-editor): auto-layout on load + viewport to top-left#1465
Conversation
Run auto-layout automatically when a workflow is loaded so nodes are tidily positioned instead of scattered at persisted positions. Position the viewport at (0,0) with zoom 1 so the laid-out nodes are visible in the top-left corner. - workflow-auto-layout.ts: change ORIGIN_X/Y from 40 to 0 - WorkflowNodeEditor.tsx: apply autoLayout on load, setViewport to top-left - WorkflowNodeEditor.test.tsx: update test to verify layout runs on load
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughWorkflowNodeEditor now automatically applies graph auto-layout during workflow load, repositioning nodes with zero-based origin coordinates. The editor captures React Flow's ChangesAuto-layout on workflow load
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx (1)
430-457: ⚡ Quick winStrengthen these tests to assert auto-layout behavior directly
The current checks can pass even if auto-layout-on-load/click behavior regresses. Prefer spying on
autoLayoutand asserting it is called on initial load and again when the button is clicked.Suggested test hardening
+import * as autoLayoutModule from "../workflow-auto-layout"; ... it("runs auto-layout on load (nodes are positioned at layout positions)", async () => { vi.mocked(fetchWorkflows).mockResolvedValue([v2Def()]); - const { container } = render( - <WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />, - ); - await screen.findByTestId("wf-node-start"); - // React Flow positions step nodes via a translate transform on their wrapper. - const wrapperFor = (id: string) => - container.querySelector<HTMLElement>(`.react-flow__node[data-id="${id}"]`); - // After load, the step node should have been auto-laid-out (positioned). - await waitFor(() => { - const transform = wrapperFor("step")?.style.transform ?? ""; - expect(transform).not.toBe(""); - }); + const spy = vi.spyOn(autoLayoutModule, "autoLayout"); + render(<WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />); + await screen.findByTestId("wf-node-start"); + await waitFor(() => expect(spy).toHaveBeenCalled()); + spy.mockRestore(); }); it("clicking auto-layout still works after initial load", async () => { vi.mocked(fetchWorkflows).mockResolvedValue([v2Def()]); + const spy = vi.spyOn(autoLayoutModule, "autoLayout"); render( <WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />, ); await screen.findByTestId("wf-node-start"); - // The auto-layout button should still be present and clickable. const btn = screen.getByTestId("wf-auto-layout"); - expect(btn).toBeInTheDocument(); - // Clicking it should not throw. + const initialCalls = spy.mock.calls.length; fireEvent.click(btn); + await waitFor(() => expect(spy.mock.calls.length).toBeGreaterThan(initialCalls)); + spy.mockRestore(); });🤖 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 `@packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx` around lines 430 - 457, The tests should spy on the auto-layout function and assert it runs on load and on button click: locate where WorkflowNodeEditor instantiates or imports autoLayout (the autoLayout function used by the editor/React Flow instance), mock/spy on that autoLayout function before rendering in the two tests (the "runs auto-layout on load..." and "clicking auto-layout still works..." tests), then assert the spy was called once after initial render and again when the auto-layout button (data-testid="wf-auto-layout") is clicked; keep existing fetchWorkflows mocking (vi.mocked(fetchWorkflows).mockResolvedValue([v2Def()])) and use the same render/screen utilities so only the addition of the spy and call assertions are required.
🤖 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.
Nitpick comments:
In `@packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx`:
- Around line 430-457: The tests should spy on the auto-layout function and
assert it runs on load and on button click: locate where WorkflowNodeEditor
instantiates or imports autoLayout (the autoLayout function used by the
editor/React Flow instance), mock/spy on that autoLayout function before
rendering in the two tests (the "runs auto-layout on load..." and "clicking
auto-layout still works..." tests), then assert the spy was called once after
initial render and again when the auto-layout button
(data-testid="wf-auto-layout") is clicked; keep existing fetchWorkflows mocking
(vi.mocked(fetchWorkflows).mockResolvedValue([v2Def()])) and use the same
render/screen utilities so only the addition of the spy and call assertions are
required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b972bae1-0790-41be-9e71-7eece3cf03df
📒 Files selected for processing (3)
packages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsxpackages/dashboard/app/components/workflow-auto-layout.ts
Greptile SummaryThis PR auto-applies layout on workflow load and resets the viewport to
Confidence Score: 4/5Safe to merge; the load effect and snapshot logic are coherent and the behaviour change is intentional. The core load-effect changes are sound: auto-layout runs before setNodes, the snapshot is correctly rebased to laidOutNodes, and setViewport is properly scoped with the right dependency. The two remaining concerns are visual (zero-pixel margin at the canvas edge) and test coverage (the button repositioning behaviour is no longer asserted). WorkflowNodeEditor.test.tsx — the button-click test lost its position-change assertion and should be strengthened. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Editor as WorkflowNodeEditor
participant RF as ReactFlow
participant Layout as autoLayout()
User->>Editor: Open workflow
Editor->>Editor: irToFlow(activeWorkflow)
Editor->>Layout: autoLayout(flow.nodes, flow.edges, columns)
Layout-->>Editor: layoutPositions (Map)
Editor->>Layout: applyAutoLayout(flow.nodes, layoutPositions)
Layout-->>Editor: laidOutNodes
Editor->>RF: setNodes(laidOutNodes)
Editor->>RF: setEdges(flow.edges)
Editor->>Editor: "loadedSnapshotRef = serializeGraph(..., laidOutNodes, ...)"
Editor->>RF: "setViewport({x:0, y:0, zoom:1})"
RF-->>User: Nodes visible at top-left
User->>Editor: Click Auto-layout button
Editor->>Layout: autoLayout(currentNodes, edges, columns)
Layout-->>Editor: newPositions
Editor->>RF: setNodes(applyAutoLayout(ns, newPositions))
RF-->>User: Nodes repositioned
Reviews (1): Last reviewed commit: "fix(node-editor): auto-layout on load + ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("clicking auto-layout still works after initial load", async () => { | ||
| vi.mocked(fetchWorkflows).mockResolvedValue([v2Def()]); | ||
| render( | ||
| <WorkflowNodeEditor isOpen onClose={() => {}} addToast={() => {}} />, | ||
| ); | ||
| await screen.findByTestId("wf-node-start"); | ||
| // The auto-layout button should still be present and clickable. | ||
| const btn = screen.getByTestId("wf-auto-layout"); | ||
| expect(btn).toBeInTheDocument(); | ||
| // Clicking it should not throw. | ||
| fireEvent.click(btn); | ||
| }); |
There was a problem hiding this comment.
The replacement test only asserts that the button exists and that clicking it doesn't throw — it doesn't verify that node positions actually change. The original test confirmed that style.transform changed after a click, giving real confidence that handleAutoLayout calls setNodes. If the click handler were silently disconnected or applyAutoLayout returned the same nodes, this test would still pass.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /** Left/top origin for the laid-out graph. */ | ||
| const ORIGIN_X = 40; | ||
| const ORIGIN_Y = 40; | ||
| const ORIGIN_X = 0; | ||
| const ORIGIN_Y = 0; |
There was a problem hiding this comment.
With ORIGIN_X/Y = 0 and setViewport({ x: 0, y: 0, zoom: 1 }), the first node's top-left corner renders at the exact top-left pixel of the canvas container, with no breathing room. The original 40 px offset was the visible margin between the canvas edge and the first card. Even a small value (e.g. 16 px) would prevent the node header from being clipped by the canvas border or the React Flow minimap chrome.
| /** Left/top origin for the laid-out graph. */ | |
| const ORIGIN_X = 40; | |
| const ORIGIN_Y = 40; | |
| const ORIGIN_X = 0; | |
| const ORIGIN_Y = 0; | |
| /** Left/top origin for the laid-out graph. */ | |
| const ORIGIN_X = 16; | |
| const ORIGIN_Y = 16; |
1 participant
Problem
The workflow node editor was loading workflows with nodes at their persisted positions, which were often scattered or off-screen. Users had to manually click "Auto-layout" and pan the viewport to see the nodes every time they opened a workflow.
Solution
autoLayout()is now called automatically and the computed tidy positions are applied before the first render.{x: 0, y: 0, zoom: 1}so the laid-out nodes are immediately visible in the top-left corner.(40, 40)to(0, 0)so nodes start at the true top-left instead of being offset.Changes
workflow-auto-layout.tsORIGIN_X/ORIGIN_Ychanged from40to0WorkflowNodeEditor.tsxautoLayout+applyAutoLayouton load; callsetViewport({x: 0, y: 0, zoom: 1})WorkflowNodeEditor.test.tsxVerification
WorkflowNodeEditortests passSummary by CodeRabbit