fix(flows): canvas re-sync after save + Fix with agent on failed runs (B21/B22) (#4823)

This commit is contained in:
Cyrus Gray
2026-07-13 20:12:03 +05:30
committed by GitHub
parent 407fd8edd7
commit f517ff858e
6 changed files with 447 additions and 17 deletions
@@ -0,0 +1,182 @@
/**
* FlowRunsSidebar — the flow canvas's projected run-history sidebar. Asserts
* the run list renders, a run row opens the {@link FlowRunInspectorDrawer},
* and (issue B22) the drawer's "Fix with agent" action navigates to this same
* flow's canvas seeded with a `copilotRepair` state and closes the sidebar's
* own drawer — this sidebar is only ever mounted while the user is ALREADY on
* the failing run's own `/flows/:id` canvas (`FlowCanvasPage` projects it into
* the shell sidebar), so re-navigating to the SAME route with a fresh repair
* seed is the fix (see `FlowCanvasPage.tsx`'s `locationKey`-based copilot
* panel remount, which reacts to exactly this navigation).
*/
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
import { Provider } from 'react-redux';
import { MemoryRouter, Route, Routes, useLocation } from 'react-router-dom';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { FlowRun } from '../../services/api/flowsApi';
import { store } from '../../store';
import FlowRunsSidebar from './FlowRunsSidebar';
const listFlowRuns = vi.hoisted(() => vi.fn());
vi.mock('../../services/api/flowsApi', () => ({ listFlowRuns }));
// Capture the props handed to the drawer so "Fix with agent" can be invoked
// directly without standing up the drawer's own run-polling machinery
// (mirrors `FlowApprovalCard.test.tsx`'s stub pattern).
const inspectorDrawerProps = vi.hoisted(() => ({
current: null as Record<string, unknown> | null,
}));
vi.mock('./FlowRunInspectorDrawer', () => ({
FLOW_RUN_STATUS_ACCENT: {
running: '',
completed: '',
completed_with_warnings: '',
pending_approval: '',
failed: '',
cancelled: '',
},
FLOW_RUN_STATUS_DOT: {
running: '',
completed: '',
completed_with_warnings: '',
pending_approval: '',
failed: '',
cancelled: '',
},
FLOW_RUN_STATUS_KEY: {
running: 'flowRuns.status.running',
completed: 'flowRuns.status.completed',
completed_with_warnings: 'flowRuns.status.completed_with_warnings',
pending_approval: 'flowRuns.status.pending_approval',
failed: 'flowRuns.status.failed',
cancelled: 'flowRuns.status.cancelled',
},
FlowRunInspectorDrawer: (props: Record<string, unknown>) => {
inspectorDrawerProps.current = props;
return props.runId ? (
<div data-testid="flow-run-inspector-drawer-stub">{props.runId as string}</div>
) : null;
},
}));
function makeRun(overrides: Partial<FlowRun> = {}): FlowRun {
return {
id: 'run-1',
flow_id: 'flow-1',
thread_id: 'run-1',
status: 'failed',
started_at: '2026-07-13T18:23:00Z',
finished_at: '2026-07-13T18:23:05Z',
steps: [],
pending_approvals: [],
error: 'GMAIL_SEND_EMAIL: empty body',
...overrides,
};
}
/** Renders whatever `location.state` a navigation landed with, for assertions. */
function LocationStateProbe() {
const location = useLocation();
return <div data-testid="location-state-probe">{JSON.stringify(location.state)}</div>;
}
function renderSidebar(flowId = 'flow-1') {
return render(
<Provider store={store}>
<MemoryRouter initialEntries={[`/flows/${flowId}`]}>
<Routes>
<Route path="/flows/:id" element={<FlowRunsSidebar flowId={flowId} />} />
</Routes>
</MemoryRouter>
</Provider>
);
}
describe('FlowRunsSidebar', () => {
beforeEach(() => {
vi.clearAllMocks();
inspectorDrawerProps.current = null;
});
it('lists runs and opens the inspector drawer for the clicked run', async () => {
listFlowRuns.mockResolvedValue([makeRun()]);
renderSidebar();
const row = await screen.findByTestId('flow-runs-sidebar-run-run-1');
expect(screen.queryByTestId('flow-run-inspector-drawer-stub')).not.toBeInTheDocument();
fireEvent.click(row);
expect(screen.getByTestId('flow-run-inspector-drawer-stub')).toHaveTextContent('run-1');
});
it('passes onFixWithAgent through to the run inspector drawer', async () => {
listFlowRuns.mockResolvedValue([makeRun()]);
renderSidebar();
fireEvent.click(await screen.findByTestId('flow-runs-sidebar-run-run-1'));
expect(inspectorDrawerProps.current?.onFixWithAgent).toBeInstanceOf(Function);
});
it('"Fix with agent" closes the drawer and navigates to the same flow seeded with the repair context (B22)', async () => {
listFlowRuns.mockResolvedValue([makeRun()]);
render(
<Provider store={store}>
<MemoryRouter initialEntries={['/flows/flow-1']}>
<Routes>
<Route
path="/flows/:id"
element={
<>
<FlowRunsSidebar flowId="flow-1" />
<LocationStateProbe />
</>
}
/>
</Routes>
</MemoryRouter>
</Provider>
);
fireEvent.click(await screen.findByTestId('flow-runs-sidebar-run-run-1'));
expect(screen.getByTestId('flow-run-inspector-drawer-stub')).toBeInTheDocument();
act(() => {
(
inspectorDrawerProps.current?.onFixWithAgent as (request: {
flowId: string;
runId: string;
error?: string | null;
failingNodeIds?: string[];
}) => void
)({
flowId: 'flow-1',
runId: 'run-1',
error: 'GMAIL_SEND_EMAIL: empty body',
failingNodeIds: ['send_summary'],
});
});
// The sidebar's own run-inspector drawer closes (repair takes over).
await waitFor(() =>
expect(screen.queryByTestId('flow-run-inspector-drawer-stub')).not.toBeInTheDocument()
);
const probe = screen.getByTestId('location-state-probe');
expect(JSON.parse(probe.textContent ?? 'null')).toEqual({
copilotRepair: {
runId: 'run-1',
error: 'GMAIL_SEND_EMAIL: empty body',
failingNodeIds: ['send_summary'],
},
});
});
it('shows the empty state when there are no runs', async () => {
listFlowRuns.mockResolvedValue([]);
renderSidebar();
expect(await screen.findByTestId('flow-runs-sidebar-empty')).toBeInTheDocument();
});
});
+34 -1
View File
@@ -11,6 +11,7 @@
*/
import createDebug from 'debug';
import { useCallback, useEffect, useState } from 'react';
import { useNavigate } from 'react-router-dom';
import { useT } from '../../lib/i18n/I18nContext';
import { type FlowRun, listFlowRuns } from '../../services/api/flowsApi';
@@ -19,6 +20,7 @@ import {
FLOW_RUN_STATUS_ACCENT,
FLOW_RUN_STATUS_DOT,
FLOW_RUN_STATUS_KEY,
type FlowRepairRequest,
FlowRunInspectorDrawer,
} from './FlowRunInspectorDrawer';
@@ -44,11 +46,38 @@ export interface FlowRunsSidebarProps {
export default function FlowRunsSidebar({ flowId }: FlowRunsSidebarProps) {
const { t } = useT();
const navigate = useNavigate();
const [runs, setRuns] = useState<FlowRun[]>([]);
const [loading, setLoading] = useState(true);
const [error, setError] = useState<string | null>(null);
const [selectedRunId, setSelectedRunId] = useState<string | null>(null);
// "Fix with agent" (issue B22) — this sidebar is only ever mounted while
// already on the failed run's own `/flows/:id` canvas (`FlowCanvasPage`
// projects it into the shell sidebar), so re-navigating to the SAME route
// with a fresh `copilotRepair` state is enough to open the canvas copilot
// preloaded with the failure — same mechanism `FlowsPage`'s run-history
// drawer uses to reach this page from elsewhere. `replace: true` avoids
// stacking a new history entry per click on top of the page the user is
// already viewing.
const handleFixWithAgent = useCallback(
(request: FlowRepairRequest) => {
log('fix with agent: flow=%s run=%s', request.flowId, request.runId);
setSelectedRunId(null);
navigate(`/flows/${request.flowId}`, {
replace: true,
state: {
copilotRepair: {
runId: request.runId,
error: request.error,
failingNodeIds: request.failingNodeIds,
},
},
});
},
[navigate]
);
const load = useCallback(async () => {
log('loading runs for flow=%s', flowId);
setLoading(true);
@@ -145,7 +174,11 @@ export default function FlowRunsSidebar({ flowId }: FlowRunsSidebarProps) {
</ul>
</div>
<FlowRunInspectorDrawer runId={selectedRunId} onClose={() => setSelectedRunId(null)} />
<FlowRunInspectorDrawer
runId={selectedRunId}
onClose={() => setSelectedRunId(null)}
onFixWithAgent={handleFixWithAgent}
/>
</div>
);
}
@@ -1,15 +1,20 @@
/**
* Approve/Dismiss/View-run contract for the flow-pending-approval
* notification card (issues B3a + B3b). Asserts that Approve reads
* `{ flow_id, thread_id, node_ids }` from the notification's action payload,
* calls `flowsApi.resumeFlow` with those args, clears the notification on
* success, surfaces a localized error on failure (including when `node_ids`
* contains non-string entries — an invalid payload), that Dismiss clears the
* notification WITHOUT calling any RPC (there is no `flows_deny` endpoint
* yet), and that "View run" opens the {@link FlowRunInspectorDrawer}.
* Approve/Dismiss/View-run/Fix-with-agent contract for the
* flow-pending-approval notification card (issues B3a + B3b + B22). Asserts
* that Approve reads `{ flow_id, thread_id, node_ids }` from the notification's
* action payload, calls `flowsApi.resumeFlow` with those args, clears the
* notification on success, surfaces a localized error on failure (including
* when `node_ids` contains non-string entries — an invalid payload), that
* Dismiss clears the notification WITHOUT calling any RPC (there is no
* `flows_deny` endpoint yet), that "View run" opens the
* {@link FlowRunInspectorDrawer}, and that the drawer's "Fix with agent"
* action navigates to the flow's canvas seeded with a `copilotRepair` state
* (issue B22 — this card can render anywhere in the app, so it always
* navigates rather than assuming the canvas is already open).
*/
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
import { Provider } from 'react-redux';
import { MemoryRouter, Route, Routes, useLocation } from 'react-router-dom';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import { store } from '../../store';
@@ -19,9 +24,20 @@ import FlowApprovalCard from './FlowApprovalCard';
const resumeFlow = vi.hoisted(() => vi.fn());
vi.mock('../../services/api/flowsApi', () => ({ resumeFlow }));
// Capture the props `FlowApprovalCard` hands the drawer (mirrors
// `FlowCanvasPage.test.tsx`'s copilot-panel stub pattern) so tests can invoke
// `onFixWithAgent` directly without needing the drawer's own run-polling
// machinery.
const inspectorDrawerProps = vi.hoisted(() => ({
current: null as Record<string, unknown> | null,
}));
vi.mock('../flows/FlowRunInspectorDrawer', () => ({
FlowRunInspectorDrawer: ({ runId }: { runId: string | null; onClose: () => void }) =>
runId ? <div data-testid="flow-run-inspector-drawer-stub">{runId}</div> : null,
FlowRunInspectorDrawer: (props: Record<string, unknown>) => {
inspectorDrawerProps.current = props;
return props.runId ? (
<div data-testid="flow-run-inspector-drawer-stub">{props.runId as string}</div>
) : null;
},
}));
function makeItem(overrides: Partial<NotificationItem> = {}): NotificationItem {
@@ -43,10 +59,21 @@ function makeItem(overrides: Partial<NotificationItem> = {}): NotificationItem {
};
}
/** Renders whatever `location.state` a navigation landed with, for assertions. */
function LocationStateProbe() {
const location = useLocation();
return <div data-testid="location-state-probe">{JSON.stringify(location.state)}</div>;
}
function renderCard(item: NotificationItem) {
return render(
<Provider store={store}>
<FlowApprovalCard notification={item} />
<MemoryRouter initialEntries={['/home']}>
<Routes>
<Route path="/home" element={<FlowApprovalCard notification={item} />} />
<Route path="/flows/:id" element={<LocationStateProbe />} />
</Routes>
</MemoryRouter>
</Provider>
);
}
@@ -55,6 +82,7 @@ describe('FlowApprovalCard', () => {
beforeEach(() => {
vi.clearAllMocks();
store.dispatch({ type: 'notifications/clearAll' });
inspectorDrawerProps.current = null;
});
it('renders both Approve and Dismiss buttons', () => {
@@ -227,4 +255,46 @@ describe('FlowApprovalCard', () => {
expect(drawer).toBeInTheDocument();
expect(drawer).toHaveTextContent('thread-1');
});
it('passes onFixWithAgent through to the run inspector drawer', () => {
renderCard(makeItem());
fireEvent.click(screen.getByTestId('flow-approval-view-run'));
expect(inspectorDrawerProps.current?.onFixWithAgent).toBeInstanceOf(Function);
});
it('"Fix with agent" navigates to the flow canvas seeded with the repair context (B22)', async () => {
renderCard(makeItem());
fireEvent.click(screen.getByTestId('flow-approval-view-run'));
expect(screen.getByTestId('flow-run-inspector-drawer-stub')).toBeInTheDocument();
act(() => {
(
inspectorDrawerProps.current?.onFixWithAgent as (request: {
flowId: string;
runId: string;
error?: string | null;
failingNodeIds?: string[];
}) => void
)({
flowId: 'flow-1',
runId: 'thread-1',
error: 'GMAIL_SEND_EMAIL: empty body',
failingNodeIds: ['send_summary'],
});
});
// Navigated away — the drawer stub (and the whole card) unmounts.
await waitFor(() =>
expect(screen.queryByTestId('flow-run-inspector-drawer-stub')).not.toBeInTheDocument()
);
const probe = await screen.findByTestId('location-state-probe');
expect(JSON.parse(probe.textContent ?? 'null')).toEqual({
copilotRepair: {
runId: 'thread-1',
error: 'GMAIL_SEND_EMAIL: empty body',
failingNodeIds: ['send_summary'],
},
});
});
});
@@ -28,6 +28,7 @@
*/
import debug from 'debug';
import { useState } from 'react';
import { useNavigate } from 'react-router-dom';
import { useT } from '../../lib/i18n/I18nContext';
import { resumeFlow } from '../../services/api/flowsApi';
@@ -37,7 +38,7 @@ import {
markRead,
type NotificationItem,
} from '../../store/notificationSlice';
import { FlowRunInspectorDrawer } from '../flows/FlowRunInspectorDrawer';
import { type FlowRepairRequest, FlowRunInspectorDrawer } from '../flows/FlowRunInspectorDrawer';
import Button from '../ui/Button';
const log = debug('notifications:flow-approval-card');
@@ -71,10 +72,31 @@ interface Props {
const FlowApprovalCard = ({ notification: n }: Props) => {
const { t } = useT();
const dispatch = useAppDispatch();
const navigate = useNavigate();
const [pending, setPending] = useState<'approve' | null>(null);
const [error, setError] = useState<string | null>(null);
const [inspecting, setInspecting] = useState(false);
// "Fix with agent" (issue B22) — a run opened from this notification can
// fail with the gate never actually reached (or after it), so the same
// repair action `FlowsPage`'s run-history drawer offers must be reachable
// from here too. This card can render from anywhere in the app (it's a
// `CoreNotification` surface), so — unlike `FlowRunsSidebar`, which is
// always already on the flow's own canvas — always navigate there first.
const handleFixWithAgent = (request: FlowRepairRequest) => {
log('fix with agent: notification=%s flow=%s run=%s', n.id, request.flowId, request.runId);
setInspecting(false);
navigate(`/flows/${request.flowId}`, {
state: {
copilotRepair: {
runId: request.runId,
error: request.error,
failingNodeIds: request.failingNodeIds,
},
},
});
};
const payload = n.actions?.[0]?.payload;
const parsed = isFlowApprovalPayload(payload) ? payload : null;
@@ -204,6 +226,7 @@ const FlowApprovalCard = ({ notification: n }: Props) => {
<FlowRunInspectorDrawer
runId={inspecting ? parsed.thread_id : null}
onClose={() => setInspecting(false)}
onFixWithAgent={handleFixWithAgent}
/>
)}
</div>
+53 -3
View File
@@ -138,6 +138,7 @@ function FlowEditor({
initialCopilotSeed = null,
initialBuildSeed = null,
onBuildSeedConsumed,
locationKey,
}: {
editorFlow: EditorFlow;
initialCopilotSeed?: CopilotRepairSeed | null;
@@ -145,6 +146,16 @@ function FlowEditor({
initialBuildSeed?: CopilotBuildSeed | null;
/** Clear the route's build seed once the copilot has dispatched it (#4597). */
onBuildSeedConsumed?: () => void;
/**
* The route's `location.key` (issue B22) — react-router mints a fresh one on
* every navigation, including a same-path repeat navigation (e.g. the "Fix
* with agent" action from {@link FlowRunsSidebar}, which stays on this same
* `/flows/:id` route and so does NOT remount `FlowEditor`). Folded into the
* copilot panel's `key` below so a repair seed arriving without a page-level
* remount still forces a fresh panel mount — required for the panel's
* once-per-mount auto-fire guard (`repairSentRef`) to actually fire again.
*/
locationKey: string;
}) {
const { t } = useT();
const navigate = useNavigate();
@@ -206,6 +217,19 @@ function FlowEditor({
const [copilotOpen, setCopilotOpen] = useState(
initialCopilotSeed !== null || initialBuildSeed !== null
);
// Issue B22: a repair seed can also arrive WITHOUT a `FlowEditor` remount —
// "Fix with agent" clicked from `FlowRunsSidebar` stays on this same
// `/flows/:id` route (only `location.state`/`location.key` change), so the
// `useState` initializer above (mount-only) won't re-open the panel for it.
// Re-assert open whenever a new repair seed prop arrives — done during
// render (React's "adjusting state when a prop changes" pattern) rather
// than a `useEffect`, so it lands in the same render pass instead of an
// extra one.
const [seenCopilotSeed, setSeenCopilotSeed] = useState(initialCopilotSeed);
if (initialCopilotSeed !== seenCopilotSeed) {
setSeenCopilotSeed(initialCopilotSeed);
if (initialCopilotSeed) setCopilotOpen(true);
}
// Per-workflow copilot thread: seeded from the session cache so opening/closing
// the panel (or switching flows and back) resumes the same conversation
// instead of starting a fresh `workflow_builder` thread each time.
@@ -323,6 +347,14 @@ function FlowEditor({
// the new flow's canonical `/flows/:id` canvas so further saves update it.
// Rejections propagate so the canvas surfaces the failure inline (and leaves
// the draft dirty).
//
// Issue B21: `flows_update` re-validates/normalizes the graph server-side
// (schema migration, id defaults, port normalization, etc.) before
// persisting, so the canonical saved shape can differ from what the client
// sent. Re-sync the canvas draft from the RESPONSE (not just the just-sent
// `next`) and bump `canvasVersion` so the editable canvas re-seeds from the
// canonical persisted graph immediately — matching what a navigate-away-
// and-back remount would show, without requiring one.
const handleSave = useCallback(
async (next: WorkflowGraph) => {
if (isDraft) {
@@ -338,9 +370,17 @@ function FlowEditor({
return;
}
log('save: flow id=%s nodes=%d edges=%d', flowId, next.nodes.length, next.edges.length);
await updateFlow(flowId, { graph: next });
persistedGraphRef.current = next;
log('save: flow id=%s persisted', flowId);
const updated = await updateFlow(flowId, { graph: next });
const persisted = updated.graph as WorkflowGraph;
persistedGraphRef.current = persisted;
setDraftGraph(persisted);
setCanvasVersion(v => v + 1);
log(
'save: flow id=%s persisted — canvas re-synced from response nodes=%d edges=%d',
flowId,
persisted.nodes.length,
persisted.edges.length
);
},
[isDraft, flowId, name, requireApproval, navigate]
);
@@ -534,6 +574,14 @@ function FlowEditor({
{copilotOpen && (
<WorkflowCopilotPanel
// Stable ('copilot') across manual open/close and build-seed
// navigations (unaffected — those always land on a fresh
// `FlowEditor` mount already, see `locationKey`'s doc comment).
// Repair seeds fold in `locationKey` so a same-route "Fix with
// agent" click (no `FlowEditor` remount) still forces a fresh
// panel mount, resetting the once-per-mount `repairSentRef` guard
// so the repair turn actually (re)fires (issue B22).
key={initialCopilotSeed ? `copilot-repair-${locationKey}` : 'copilot'}
graph={preview?.base ?? draftGraph}
flowId={flowId}
onProposal={handleProposal}
@@ -647,6 +695,7 @@ export default function FlowCanvasPage() {
initialCopilotSeed={copilotSeed}
initialBuildSeed={buildSeed}
onBuildSeedConsumed={clearBuildSeed}
locationKey={location.key}
/>
);
}
@@ -735,6 +784,7 @@ export function FlowCanvasDraftPage() {
graph: draft.graph,
requireApproval: draft.requireApproval,
}}
locationKey={location.key}
/>
<ToastContainer notifications={toasts} onRemove={removeToast} />
</>
@@ -194,6 +194,78 @@ describe('FlowCanvasPage', () => {
]);
});
// Issue B21: `flows_update` re-validates/normalizes the graph server-side
// before persisting, so the canonical response can legitimately differ from
// what the client sent (schema migration, id defaults, etc.). Previously the
// canvas re-baselined against its OWN pre-save nodes/edges and ignored the
// response entirely — the canonical shape only ever appeared after a
// navigate-away-and-back remount refetched it via `flows_get`. Assert the
// canvas now reflects the SAVE RESPONSE's graph immediately, with no
// navigation and no remount of `FlowCanvasPage`.
it('re-syncs the canvas from the flows_update response on save, without a remount (B21)', async () => {
getFlow.mockResolvedValue(makeFlow());
// The server "normalizes" the saved graph: it accepts the client's
// trigger+agent nodes but also injects a third node the client never
// added (standing in for a server-side migration/default-fill), and
// renames the trigger. A stale canvas would keep showing only the two
// client-added nodes named "Start"/"New agent".
updateFlow.mockResolvedValue(
makeFlow({
graph: {
schema_version: 1,
id: 'test-id',
name: 'Daily digest',
nodes: [
{
id: 't',
kind: 'trigger',
name: 'Start (normalized)',
config: {},
ports: [],
position: { x: 0, y: 0 },
},
{
id: 'new-agent-0',
kind: 'agent',
name: 'New agent',
config: {},
ports: [],
position: { x: 80, y: 80 },
},
{
id: 'server-added',
kind: 'transform',
name: 'Server-added node',
config: {},
ports: [],
position: { x: 160, y: 160 },
},
],
edges: [],
},
})
);
renderEditor();
await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument());
fireEvent.click(screen.getByTestId('flow-palette-item-agent'));
expect(screen.getAllByTestId('flow-node')).toHaveLength(2);
fireEvent.click(screen.getByTestId('flow-editor-save'));
await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1));
// The canvas now shows the RESPONSE's three nodes (including the one the
// client never added and the renamed trigger) — no navigation, no
// `flows_get` refetch, no remount required.
await waitFor(() => expect(screen.getAllByTestId('flow-node')).toHaveLength(3));
expect(screen.getByText('Start (normalized)')).toBeInTheDocument();
expect(screen.getByText('Server-added node')).toBeInTheDocument();
// Still the same page/component — proving this wasn't a navigate-away
// remount refetch in disguise.
expect(getFlow).toHaveBeenCalledTimes(1);
expect(screen.getByTestId('flow-canvas-page')).toBeInTheDocument();
});
it('does not prompt when navigating Back with no unsaved changes', async () => {
getFlow.mockResolvedValue(makeFlow());
renderEditor();