feat(ui): add workflow builder page with routing and persistence#1113
feat(ui): add workflow builder page with routing and persistence#1113geoffjay wants to merge 2 commits intoissue-1099from
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
geoffjay
left a comment
There was a problem hiding this comment.
Review: feat(ui): add workflow builder page with routing and persistence
Changes requested — one blocking issue. Cannot self-request-changes; marking needs-rework via label.
Stack note
This branch (issue-1101) is stacked on PR #1112 (issue-1099: "feat(ui): build node palette sidebar with drag-and-drop onto workflow canvas"), which also carries review-agent and has not yet been reviewed. Ideally #1112 is reviewed first. PR #1114 depends on this PR and has already been approved — it will need no changes once the blocking issue here is fixed.
Blocking: edit mode calls createWorkflow instead of updateWorkflow
handleSave unconditionally calls orchestratorClient.createWorkflow in both create and edit modes. When a user navigates to /workflows/:id/edit via the "Edit in builder" link and saves, they silently create a new duplicate workflow instead of updating the existing one.
orchestratorClient.updateWorkflow(id, request) exists and has the right signature — it just isn't wired up.
Fix: branch on isEditing in handleSave:
const requests = graphToWorkflows(nodes, edges);
const saved = await Promise.all(
requests.map((req) => {
const named = workflowName.trim() ? { ...req, name: workflowName.trim() } : req;
if (isEditing && editWorkflowId) {
return orchestratorClient.updateWorkflow(editWorkflowId, named);
}
return orchestratorClient.createWorkflow(named);
}),
);The post-save navigation logic (navigate(/workflows/${saved[0].id})) already works correctly for the update path since updateWorkflow returns the updated Workflow.
A test for this path is also missing — add one to WorkflowBuilder.test.tsx:
it("calls updateWorkflow (not createWorkflow) in edit mode", async () => {
mockUpdateWorkflow.mockResolvedValue({ ...baseWorkflow, name: "Updated" });
const user = userEvent.setup();
renderEdit("wf-1");
await waitFor(() =>
expect(screen.getByTestId("builder-name-input")).toHaveValue("My Workflow"),
);
await user.click(screen.getByTestId("builder-save-btn"));
await waitFor(() => expect(mockUpdateWorkflow).toHaveBeenCalledWith("wf-1", expect.any(Object)));
expect(mockCreateWorkflow).not.toHaveBeenCalled();
});Non-blocking suggestions
1. allAgents missing from load useEffect deps
workflowsToGraph([wf], allAgents, ...) uses allAgents but the dep array is [editWorkflowId] with an eslint-disable masking the omission. On first render allAgents is typically still loading ([]), so agent nodes get built with incomplete data. The real-world fix is to defer graph construction until agents are available:
useEffect(() => {
if (!editWorkflowId || allAgents.length === 0) return;
// ... existing load logic
}, [editWorkflowId, allAgents]);This re-runs once agents load and avoids the race. Remove the eslint-disable comment.
2. rfInstanceRef is declared but never populated
rfInstanceRef is a useRef(null) that's never assigned — onInit isn't passed to WorkflowCanvas, so the ref stays null for the lifetime of the page. Drop coordinates silently fall back to raw screen coordinates (screenPos), which is correct only when the canvas is at zoom=1 and no pan. After any pan or zoom, dropped nodes will land in wrong positions.
Wire it up once WorkflowCanvas exposes the onInit prop (added in PR #1114):
onInit={(instance) => { rfInstanceRef.current = instance; }}Also replace the manual inline type with useRef<ReactFlowInstance | null>(null) — ReactFlowInstance is already exported by @xyflow/react.
3. Two tests assert the wrong thing
-
"shows status bar with validation error on empty save attempt" — asserts
builder-canvas-areais present (always true), not that the status bar actually shows a validation message. Considerexpect(screen.queryByTestId("builder-status-bar")).not.toBeInTheDocument()(empty graph passes validation, nothing to show) or a test with a lone unconnected node that does trigger an error. -
"calls createWorkflow with the workflow name when nodes exist" — the comment admits it "Just verify the component didn't crash". Either wire up a proper assertion on
mockCreateWorkflowor remove the test.
What was verified (no issues)
- Route ordering in
App.tsx: React Router v6 scores literal segments higher than params, so/workflows/buildercorrectly wins over/workflows/:id. ✓ - Dirty tracking via
handleNodesChange/handleEdgesChangewrappers. ✓ - Duplicate-edge guard in
handleConnect. ✓ trigger → agent-only connection type check. ✓- Cancel navigation: create →
/workflows, edit →/workflows/:id. ✓ beforeunloadguard correctly cleaned up in effect return. ✓- "Edit in builder" link and "New in builder" button styling matches existing header buttons. ✓
Fix applied locally — push neededAll blocking + non-blocking issues addressed (commit 1 ahead of origin on branch issue-1101):
Action needed @geoff: ShawnSunClio lacks push access — please push branch issue-1101. |
Adds WorkflowBuilder page at /workflows/builder (create) and /workflows/:id/edit (edit), wires up routing in App.tsx, adds 'New in builder' button to WorkflowList, and 'Edit in builder' link to WorkflowDetail.
Closes #1101