feat(ui): add workflow builder page with routing and persistence

[geoffjay]

Fixes all 4 blocking bugs from second review:

1. Bug 1 (rfInstanceRef): Add onInit prop to WorkflowCanvas and wire it in WorkflowBuilder so screenToFlowPosition works correctly after any pan/zoom
2. Bug 2 (allAgents reload): Add hasLoadedRef guard to the edit-mode load useEffect so the workflow is not reloaded (overwriting unsaved edits) on every 10s useAgents background poll
3. Bug 3 (dirty tracking): Filter non-user React Flow change events (dimensions, select, position without dragging) in handleNodesChange/handleEdgesChange so the "Unsaved changes" badge does not fire on page load
4. Bug 4 (empty-graph save): Guard against empty graphToWorkflows result and show actionable error instead of misleading "Saved" confirmation

Tests updated: empty-graph save tests now assert the error message; new test suites for dirty-tracking and edit-mode load guard added.

Closes #1101

[geoffjay]

This change is part of the following stack:

• feat(ui): install @xyflow/react and scaffold WorkflowCanvas base component #1107
  • feat(ui): extend TriggerConfig types to match all backend trigger variants #1108
    • feat(ui): create trigger source node components for workflow canvas #1109
      • feat(ui): create agent node and prompt template edge components #1110
        • feat(ui): implement graph-to-API serialization for workflow canvas save/load #1111
          • feat(ui): build node palette sidebar with drag-and-drop onto workflow canvas #1112
            • feat(ui): add workflow builder page with routing and persistence #1113 ◀
              • feat(ui): add read-only workflow visualization with auto-layout #1114

_{Change managed by git-spice.}

[geoffjay]

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-area is present (always true), not that the status bar actually shows a validation message. Consider expect(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 mockCreateWorkflow or remove the test.

What was verified (no issues)

• Route ordering in App.tsx: React Router v6 scores literal segments higher than params, so /workflows/builder correctly wins over /workflows/:id. ✓
• Dirty tracking via handleNodesChange / handleEdgesChange wrappers. ✓
• Duplicate-edge guard in handleConnect. ✓
• trigger → agent-only connection type check. ✓
• Cancel navigation: create → /workflows, edit → /workflows/:id. ✓
• beforeunload guard correctly cleaned up in effect return. ✓
• "Edit in builder" link and "New in builder" button styling matches existing header buttons. ✓

[ShawnSunClio]

Fix applied locally — push needed

All blocking + non-blocking issues addressed (commit 1 ahead of origin on branch issue-1101):

1. handleSave now branches on isEditing: calls orchestratorClient.updateWorkflow(editWorkflowId, ...) in edit mode instead of always calling createWorkflow. Prevents silent duplicate workflow creation on /workflows/:id/edit saves.
2. useEffect load dep array now includes allAgents (eslint-disable removed) — graph construction deferred until agents are available.
3. rfInstanceRef typed as ReactFlowInstance | null instead of inline structural type.
4. Tests: mockUpdateWorkflow added to orchestrator mock; edit-mode save test added asserting updateWorkflow is called and createWorkflow is not; two weak test assertions replaced with meaningful checks.

Action needed @geoff: ShawnSunClio lacks push access — please push branch issue-1101.

[geoffjay]

Review: feat(ui) — workflow builder page

Stack note: This PR sits on top of #1112 (node palette sidebar). That parent is open, so changes here depend on it landing first.

---

4 issues must be addressed before merge

🔴 Bug 1 — rfInstanceRef is never wired up; drop coordinates are wrong after any pan/zoom

WorkflowBuilder allocates rfInstanceRef but never passes onInit to WorkflowCanvas, so the ref is always null.

WorkflowCanvas already exposes the prop (WorkflowCanvas.tsx line 66):

onInit?: (instance: ReactFlowInstance) => void;

The handleDrop fallback just uses raw screen coordinates:

// WorkflowBuilder.tsx — handleDrop
const position = rfInstanceRef.current
    ? rfInstanceRef.current.screenToFlowPosition(screenPos)
    : screenPos;   // ← always taken; broken whenever viewport is panned or zoomed

Fix — pass onInit when rendering WorkflowCanvas:

<WorkflowCanvas
    ...
    onInit={(instance) => { rfInstanceRef.current = instance; }}
/>

---

🔴 Bug 2 — allAgents in useEffect deps causes the workflow to reload every 10 s, overwriting unsaved edits

// WorkflowBuilder.tsx ~line 111
useEffect(() => {
    if (!editWorkflowId || allAgents.length === 0) return;
    setLoading(true);
    orchestratorClient.getWorkflow(editWorkflowId).then(...)
}, [editWorkflowId, allAgents]);   // ← allAgents is a new array reference every 10 s refresh

useAgents auto-refreshes every 10 s (DEFAULT_REFRESH_INTERVAL = 10_000 in useAgents.ts line 127). Each background refresh calls setAllAgents(result.items) with a new array, triggering this effect again and overwriting whatever the user has edited on the canvas. This is a data-loss bug.

Fix — gate the load to run only once per edit session:

const hasLoadedRef = useRef(false);

useEffect(() => {
    if (!editWorkflowId || allAgents.length === 0 || hasLoadedRef.current) return;
    hasLoadedRef.current = true;
    setLoading(true);
    orchestratorClient.getWorkflow(editWorkflowId).then(...);
}, [editWorkflowId, allAgents]);

---

🔴 Bug 3 — markDirty fires on non-user changes; "Unsaved changes" appears immediately on load in edit mode

React Flow's onNodesChange fires for internal events that are not user edits:

• type: "dimensions" — initial measurement pass when nodes first render
• type: "select" — clicking/deselecting a node
• type: "position" with dragging: false — viewport adjustments from FitViewOnLoad

Because handleNodesChange calls markDirty() unconditionally, the "Unsaved changes" badge and the beforeunload guard activate the moment the edit route loads, before the user touches anything.

Fix — only treat meaningful change types as dirty:

const handleNodesChange = useCallback(
    (changes: Parameters<typeof onNodesChange>[0]) => {
        onNodesChange(changes);
        const isUserEdit = changes.some(
            (c) =>
                c.type === "add" ||
                c.type === "remove" ||
                (c.type === "position" && c.dragging),
        );
        if (isUserEdit) markDirty();
    },
    [onNodesChange, markDirty],
);

Apply the same pattern to handleEdgesChange (filter out type: "select").

---

🔴 Bug 4 — empty-graph save shows "Saved — redirecting…" and navigates away without persisting anything

validateGraph([], []) returns no errors. graphToWorkflows([], []) returns []. Promise.all([]) resolves immediately. setSaveSuccess(true) fires, the status bar shows "Saved — redirecting…", and the user is navigated to /workflows — even though zero API calls were made.

The test at line 237 documents this and acknowledges it: mockCreateWorkflow is never called. Showing "Saved" when nothing was persisted is misleading.

Fix — guard before Promise.all:

const requests = graphToWorkflows(nodes, edges);
if (requests.length === 0) {
    setSaveError("Add at least one trigger connected to an agent before saving.");
    setSaving(false);
    return;
}

Update the test to assert the error message instead of success.

---

Non-blocking (address in follow-up or same PR)

Bug 5 — multi-workflow edit mode uses the same ID for every request

When in edit mode, all requests call updateWorkflow(editWorkflowId, ...) regardless of how many edges are in the graph. A second trigger→agent edge overwrites the first on the same workflow ID instead of creating a new one. This is correct for the single-edge case (the primary scenario), but silently wrong for multiple edges. Consider documenting the single-edge assumption.

Bug 6 — viewport is hardcoded {x: 0, y: 0, zoom: 1} in saveLayout

Once bug 1 is fixed, capture the live viewport:

viewport: rfInstanceRef.current?.getViewport() ?? { x: 0, y: 0, zoom: 1 },

Suggestion — GitFork icon is semantically confusing

GitFork is used inside NodePalette for the actual git-fork trigger type. Reusing it on the "New in builder" / "Edit in builder" action buttons conflates two unrelated concepts. PenLine, LayoutTemplate, or Workflow would be more appropriate.

---

Tests

Good coverage of create/edit/cancel/error paths. Updates needed for bugs 3 and 4. The useAgents mock returns a stable array reference so it won't catch the refresh loop (bug 2) — consider adding a test that simulates the hook returning a new array reference on a second render.

[geoffjay]

Review inline annotations — see main review comment for full context.

[geoffjay]

Bug 1: rfInstanceRef is always null because onInit is never passed to WorkflowCanvas. The fallback screenPos does not account for viewport pan/zoom; dropped nodes land at the wrong position after any panning or zooming. Fix: add onInit={(instance) => { rfInstanceRef.current = instance; }} to the WorkflowCanvas render.

[geoffjay]

Review: feat(ui): add workflow builder page with routing and persistence

Stack position: This PR (issue-1101) is stacked on PR #1112 (issue-1099 — node palette sidebar). PR #1114 (issue-1102 — read-only visualization) is stacked on top of this one. The PR cannot merge until #1112 lands; once it does, this one is ready to go.

All four bugs described in the PR description are correctly fixed. The implementation is clean, well-commented, and the test suite covers the critical paths. No blocking issues — approved.

---

What's correct

Bug 1 (rfInstanceRef) — onInit is wired through WorkflowCanvasProps → Canvas → <ReactFlow onInit={…}> cleanly. Fallback to raw screen coords when the instance hasn't initialised yet is the right defensive default.

Bug 2 (allAgents reload) — The hasLoadedRef guard is placed correctly: it only flips to true after passing all three preconditions (editWorkflowId, allAgents.length > 0, !hasLoadedRef.current), so a race where agents arrive on the very first render is handled correctly and subsequent 10s poll cycles are harmless.

Bug 3 (dirty tracking) — Filtering to add | remove | position{dragging:true} for nodes and everything-except-select for edges is the right cut. dimensions and select events are the main React Flow housekeeping noises that were causing false positives on page load. The replace type (node data mutation) is also excluded — this is fine since no in-canvas node data editing UI exists yet; revisit if that changes.

Bug 4 (empty-graph save) — Guard fires before the API call, error message is actionable, and createWorkflow is never called. Tests verify the exact message text, which is the right level of assertion.

Route ordering — /workflows/builder placed before /workflows/:id is correct; React Router v6 scores static segments higher than dynamic ones regardless of declaration order, so there's no shadowing risk.

Direct imports in App.tsx — WorkflowBuilder imported from @/pages/workflows/WorkflowBuilder directly (not the barrel) is consistent with how AgentDetail, QuestionDetail, and WorkflowDetail are already handled. No barrel update needed.

---

Non-blocking suggestions

1. setSaving(false) called twice in the empty-graph guard (WorkflowBuilder.tsx ~line 352)

if (requests.length === 0) {
    setSaveError("Add at least one trigger…");
    setSaving(false);   // ← explicit call
    return;             // ← `finally` still runs → setSaving(false) a second time
}
// …
} finally {
    setSaving(false);   // ← second call
}

Both calls are idempotent so there's no bug, but the explicit setSaving(false) inside the guard is noise — the finally block already covers it. Consider removing it and letting finally do the work.

2. Viewport hardcoded to {x:0, y:0, zoom:1} on save (~line 374)

saveLayout(ids, {
    nodes: Object.fromEntries(nodes.map((n) => [n.id, n.position])),
    viewport: { x: 0, y: 0, zoom: 1 },   // ← always zero
});

rfInstanceRef.current?.getViewport() is available at save time and would give the actual viewport. Since fitView is enabled, this won't cause a visible bug on reload (the canvas re-fits), but viewport state is never actually persisted. Low-priority follow-up.

3. handleDrop and handleSave not wrapped in useCallback

handleNodesChange, handleEdgesChange, and handleConnect all use useCallback, but handleDrop and handleSave are plain function declarations. handleDrop closes over rfInstanceRef, allAgents, setNodes, and markDirty — it'll be recreated on every render. No correctness issue (it's on a div), but inconsistent with the rest of the file.

4. GitFork icon on "Edit in builder" / "New in builder"

GitFork reads as "fork this workflow" rather than "open editor". Edit2 is already imported in WorkflowDetail.tsx for the existing edit button; using it here (or a PenSquare) would be more semantically clear. Minor UX quibble.

5. Edit-mode load guard test doesn't exercise the re-render path

it("calls getWorkflow only once even when allAgents reference changes", async () => {
    renderEdit("wf-1");
    await waitFor(() => expect(mockGetWorkflow).toHaveBeenCalledTimes(1));
    expect(mockGetWorkflow).toHaveBeenCalledTimes(1);
});

Because the mock returns a stable mockAllAgents reference throughout, this only validates the normal single-render path — not the re-render-with-new-allAgents scenario the comment describes. To actually prove the guard works, the test should force a re-render with a fresh allAgents reference (e.g. via rerender). The guard is correct in the implementation; the test just doesn't catch a future regression if the guard is accidentally removed.

---

Tests

Coverage is solid: create mode, edit mode load/cancel/error, save flow (spinner, empty graph, update-vs-create), dirty tracking (initial render, post-load), and load guard. The ResizeObserver stub and React Flow mock are correct patterns for jsdom unit tests.

---

Summary: All four bugs are fixed correctly. No blocking issues. The suggestions above are cosmetic or low-priority follow-ups. Ready to merge once parent PR #1112 lands.