fix(flows): clear prompt-bar build seed after dispatch so reopening the copilot can't re-save the flow (#4628)

Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
This commit is contained in:
YellowSnnowmann
2026-07-08 18:52:30 -07:00
committed by GitHub
co-authored by Steven Enamakel
parent b9c88abe90
commit 27b62175ed
7 changed files with 348 additions and 26 deletions
+4 -1
View File
@@ -694,7 +694,10 @@ mod tests {
assert_eq!(done.load(Ordering::SeqCst), 20, "every item must run");
let peak = max_seen.load(Ordering::SeqCst);
assert!(peak >= 2, "work should actually overlap (peak {peak})");
assert!(peak <= 3, "concurrency must stay bounded to 3 (peak {peak})");
assert!(
peak <= 3,
"concurrency must stay bounded to 3 (peak {peak})"
);
assert_eq!(
inflight.load(Ordering::SeqCst),
0,
@@ -1,4 +1,4 @@
import { act, fireEvent, render, screen } from '@testing-library/react';
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { WorkflowGraph, WorkflowNode } from '../../lib/flows/types';
@@ -55,7 +55,7 @@ describe('WorkflowCopilotPanel', () => {
hookState.toolTimeline = [];
hookState.liveResponse = '';
hookState.error = null;
hookState.send = vi.fn().mockResolvedValue({ proposed: false });
hookState.send = vi.fn().mockResolvedValue({ outcome: 'dispatched', proposed: false });
hookState.clearProposal = vi.fn();
});
@@ -358,13 +358,140 @@ describe('WorkflowCopilotPanel', () => {
expect(hookState.send).toHaveBeenCalledTimes(1);
});
it('reports the build seed as consumed once the turn actually dispatched', async () => {
const onBuildSeedConsumed = vi.fn();
render(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={{ description: 'digest my Slack every morning' }}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).toHaveBeenCalledTimes(1);
// Fires exactly once, after the dispatched build turn resolves, so the
// route seed can be stripped.
await waitFor(() => expect(onBuildSeedConsumed).toHaveBeenCalledTimes(1));
});
it('does not consume the seed when send no-ops (socket not connected) (#4597)', async () => {
// `send` resolves `outcome: 'skipped'` when the socket isn't connected —
// the turn never dispatched, so the seed must be preserved (not cleared) so
// the build can still fire once the socket connects.
hookState.send = vi.fn().mockResolvedValue({ outcome: 'skipped', proposed: false });
const onBuildSeedConsumed = vi.fn();
render(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={{ description: 'digest my Slack every morning' }}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).toHaveBeenCalledTimes(1);
// Flush the actual send() promise so the effect's `.then` runs, then assert
// the no-op path never consumed the seed.
await hookState.send.mock.results[0]?.value;
await waitFor(() => expect(onBuildSeedConsumed).not.toHaveBeenCalled());
});
it('does not consume or resend the seed when send fails (#4597)', async () => {
// A dispatch error resolves 'failed' (not 'skipped'): the seed is NOT
// consumed, and — crucially — the guard stays set so the effect does not
// auto-resend the turn (which would duplicate the user message and hammer
// the backend). The error surfaces separately for the user to retry.
hookState.send = vi.fn().mockResolvedValue({ outcome: 'failed', proposed: false });
const onBuildSeedConsumed = vi.fn();
const { rerender } = render(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={{ description: 'digest my Slack every morning' }}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).toHaveBeenCalledTimes(1);
await hookState.send.mock.results[0]?.value;
expect(onBuildSeedConsumed).not.toHaveBeenCalled();
// A re-render with a fresh `send` identity (as happens on any state change)
// must NOT resend — the guard remains set for a failed dispatch.
hookState.send = vi.fn().mockResolvedValue({ outcome: 'failed', proposed: false });
rerender(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={{ description: 'digest my Slack every morning' }}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).not.toHaveBeenCalled();
expect(onBuildSeedConsumed).not.toHaveBeenCalled();
});
it('does not re-fire the build turn when remounted after the seed is cleared (#4597)', async () => {
const onBuildSeedConsumed = vi.fn();
// First mount with the seed present (as the prompt-bar route lands).
const { unmount } = render(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={{ description: 'digest my Slack every morning' }}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).toHaveBeenCalledTimes(1);
await waitFor(() => expect(onBuildSeedConsumed).toHaveBeenCalledTimes(1));
// The host clears the route seed (buildSeed -> null) in response. Closing
// and reopening the copilot fully remounts the panel — the per-mount
// `buildSentRef` resets to false — but with no seed there is nothing to
// re-fire, so the build turn must NOT be sent a second time.
unmount();
render(
<WorkflowCopilotPanel
graph={baseGraph}
flowId="flow-1"
onProposal={vi.fn()}
onAccept={vi.fn()}
onReject={vi.fn()}
onClose={vi.fn()}
buildSeed={null}
onBuildSeedConsumed={onBuildSeedConsumed}
/>
);
expect(hookState.send).toHaveBeenCalledTimes(1);
expect(onBuildSeedConsumed).toHaveBeenCalledTimes(1);
});
it('carries the build seed description forward when the auto-sent build turn asks a clarifying question instead of proposing', async () => {
hookState.send = vi
.fn()
// The auto-sent build turn asks a question rather than proposing.
.mockResolvedValueOnce({ proposed: false })
// The auto-sent build turn dispatches but asks a question rather than
// proposing.
.mockResolvedValueOnce({ outcome: 'dispatched', proposed: false })
// The user's free-text answer then resolves it.
.mockResolvedValueOnce({ proposed: true });
.mockResolvedValueOnce({ outcome: 'dispatched', proposed: true });
render(
<WorkflowCopilotPanel
@@ -80,6 +80,14 @@ interface Props {
* opens already building it against the just-created blank flow.
*/
buildSeed?: { description: string } | null;
/**
* Fires once the build seed has been dispatched, so the host can clear the
* ephemeral route seed (`location.state.copilotBuild`). The in-mount
* `buildSentRef` guard only protects the current mount; closing and reopening
* the panel remounts it and resets that ref, so without clearing the route
* seed a remount would re-fire the same `build` turn (issue #4597).
*/
onBuildSeedConsumed?: () => void;
/**
* The workflow's persisted copilot thread id (from the per-flow cache), so
* reopening the panel resumes the same conversation instead of starting fresh.
@@ -98,6 +106,7 @@ export default function WorkflowCopilotPanel({
onClose,
repairSeed = null,
buildSeed = null,
onBuildSeedConsumed,
seedThreadId = null,
onThreadIdChange,
}: Props) {
@@ -195,17 +204,41 @@ export default function WorkflowCopilotPanel({
const buildSentRef = useRef(false);
useEffect(() => {
if (!buildSeed || buildSentRef.current) return;
// Optimistically guard re-entry while the async dispatch is in flight.
buildSentRef.current = true;
send({
displayText: buildSeed.description,
request: flowId
? { mode: 'build', instruction: buildSeed.description, graph, flowId }
: { mode: 'revise', instruction: buildSeed.description, graph, flowId },
}).then(({ proposed }) => {
// Not proposed => the seed turn asked a clarifying question instead of
// building. Carry the original description forward so the user's
// free-text answer (via `submit` below) doesn't strand the agent with
// no idea what it was asked to build.
}).then(({ outcome, proposed }) => {
if (outcome === 'dispatched') {
// Clear the ephemeral route seed only once the turn actually
// dispatched, so closing and reopening the panel (which remounts it
// and resets `buildSentRef`) can't re-fire the same build turn
// (issue #4597).
onBuildSeedConsumed?.();
} else if (outcome === 'skipped') {
// Retryable no-op (socket not connected yet, or a turn already in
// flight): keep the seed and release the guard so the effect retries
// once `send` changes identity on reconnect — otherwise the prompt is
// lost and the blank flow never auto-builds.
buildSentRef.current = false;
}
// `failed`: the dispatch was attempted but errored (surfaced via
// `error`). Leave the guard set so THIS mount doesn't auto-resend and
// duplicate the turn — the user retries from the input instead. Note the
// route seed is deliberately NOT consumed on failure, so a later close +
// reopen remounts the panel with a fresh `buildSentRef` and WILL re-fire
// the build: a reopen is thus an intentional retry, not a manual-only one.
// That's safe/desired — a failed turn persisted nothing (`mode:'build'`
// never saves; issue #4596), so there's no partial state to clean up.
//
// Regardless of outcome, record whether the turn proposed: when it didn't
// (a clarifying question, or a no-op/failed turn that never built), carry
// the original description forward so the user's free-text answer (via
// `submit` below) doesn't strand the agent with no idea what it was asked
// to build. A later turn that proposes clears it.
updatePendingAsk(proposed, buildSeed.description);
});
// `graph`/`flowId` are read once for the seed turn — later edits must not
+60 -3
View File
@@ -4,14 +4,16 @@ import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { BuilderTurnResult } from '../services/api/flowsApi';
import type { WorkflowProposal } from '../store/chatRuntimeSlice';
import type { ThreadMessage } from '../types/thread';
import { useWorkflowBuilderChat } from './useWorkflowBuilderChat';
import { useWorkflowBuilderChat, type WorkflowBuilderSendResult } from './useWorkflowBuilderChat';
// The hook now runs the builder server-side via `openhuman.flows_build`.
const buildWorkflow = vi.hoisted(() => vi.fn());
vi.mock('../services/api/flowsApi', () => ({ buildWorkflow }));
// Socket is always "connected" for these tests.
vi.mock('../store/socketSelectors', () => ({ selectSocketStatus: () => 'connected' }));
// Socket status is configurable per test (reset to 'connected' in beforeEach)
// so the no-op/`skipped` path (socket not connected) can be exercised.
const socketStatus = vi.hoisted(() => ({ current: 'connected' as string }));
vi.mock('../store/socketSelectors', () => ({ selectSocketStatus: () => socketStatus.current }));
const dispatch = vi.hoisted(() => vi.fn());
const selectorState = vi.hoisted(() => ({
@@ -69,6 +71,7 @@ const okResult = (over: Partial<BuilderTurnResult> = {}): BuilderTurnResult => (
describe('useWorkflowBuilderChat', () => {
beforeEach(() => {
buildWorkflow.mockReset().mockResolvedValue(okResult());
socketStatus.current = 'connected';
selectorState.proposals = {};
selectorState.messagesByThreadId = {};
selectorState.toolTimelineByThread = {};
@@ -280,6 +283,60 @@ describe('useWorkflowBuilderChat', () => {
await waitFor(() => expect(result.current.error).toBe('run failed'));
});
// The `send()` outcome contract (`dispatched` | `skipped` | `failed`) is the
// heart of the build-seed fix (PR #4628): a caller retries a seeded auto-send
// only on `skipped`, never on `failed` (which would resend and duplicate the
// turn). Assert it here at its SOURCE — the panel tests mock this hook, so
// without these a `failed` misreported as `skipped` would pass every test.
describe('send() outcome contract', () => {
it("returns 'dispatched' when the turn actually runs", async () => {
const { result } = renderHook(() => useWorkflowBuilderChat());
let outcome: WorkflowBuilderSendResult | undefined;
await act(async () => {
outcome = await result.current.send({
displayText: 'hi',
request: { mode: 'create', instruction: 'x' },
});
});
expect(outcome?.outcome).toBe('dispatched');
expect(buildWorkflow).toHaveBeenCalledTimes(1);
});
it("returns 'failed' (not 'skipped') when the dispatch throws", async () => {
// A thrown dispatch is a real error, distinct from the retryable
// `skipped` no-op — misreporting it as `skipped` would re-arm the seeded
// build effect and loop duplicate turns (see WorkflowCopilotPanel's
// `buildSentRef`). The error is also surfaced via `error`.
buildWorkflow.mockRejectedValueOnce(new Error('rpc boom'));
const { result } = renderHook(() => useWorkflowBuilderChat());
let outcome: WorkflowBuilderSendResult | undefined;
await act(async () => {
outcome = await result.current.send({
displayText: 'hi',
request: { mode: 'create', instruction: 'x' },
});
});
expect(outcome?.outcome).toBe('failed');
expect(result.current.error).toBe('rpc boom');
});
it("returns 'skipped' without dispatching when the socket is not connected", async () => {
socketStatus.current = 'connecting';
const { result } = renderHook(() => useWorkflowBuilderChat());
let outcome: WorkflowBuilderSendResult | undefined;
await act(async () => {
outcome = await result.current.send({
displayText: 'hi',
request: { mode: 'create', instruction: 'x' },
});
});
expect(outcome?.outcome).toBe('skipped');
// A retryable no-op: nothing was sent and no thread was created.
expect(buildWorkflow).not.toHaveBeenCalled();
await waitFor(() => expect(result.current.error).toBe('offline'));
});
});
describe('displayMessages', () => {
it('excludes isInterim agent messages but keeps user + terminal agent messages (incl. a clarifying question)', () => {
selectorState.messagesByThreadId = {
+47 -11
View File
@@ -59,6 +59,35 @@ export interface WorkflowBuilderSendParams {
request: BuilderTurnRequest;
}
/**
* Outcome of a {@link UseWorkflowBuilderChat.send} call:
* - `dispatched` — the turn actually ran (thread created + `flows_build` sent).
* - `skipped` — a retryable no-op: the socket wasn't connected, or a turn was
* already in flight. Nothing was sent; a caller may retry later.
* - `failed` — the dispatch was attempted but threw (thread create / RPC
* error). The error is surfaced via `error`; a caller must NOT auto-retry, or
* it would resend and duplicate the turn.
*/
export type WorkflowBuilderSendOutcome = 'dispatched' | 'skipped' | 'failed';
/**
* Result of a {@link UseWorkflowBuilderChat.send} call. Carries two orthogonal
* signals a caller may need:
* - `outcome` — whether/why the turn dispatched (see {@link
* WorkflowBuilderSendOutcome}). Drives a seeded auto-send's retry decision: it
* retries only on `skipped`, never on `failed` (which would duplicate the
* turn) or `dispatched`.
* - `proposed` — `true` iff this turn's `flows_build` call returned a proposal;
* `false` for a clarifying question, an error, or a call that never ran.
* Callers looping a conversation (the copilot's free-text follow-ups) use this
* to know whether the turn's instruction is still "unresolved" and must be
* carried into the next turn — see `WorkflowCopilotPanel`'s `pendingAskRef`.
*/
export interface WorkflowBuilderSendResult {
outcome: WorkflowBuilderSendOutcome;
proposed: boolean;
}
export interface UseWorkflowBuilderChat {
/** The dedicated thread id, or `null` before the first send creates it. */
threadId: string | null;
@@ -101,14 +130,15 @@ export interface UseWorkflowBuilderChat {
error: string | null;
/**
* Send a builder turn, creating the dedicated thread on first use. Resolves
* with `proposed: true` iff this turn's `flows_build` call returned a
* proposal — `false` for a clarifying question, an error, or a call that
* never ran (already sending / offline). Callers that loop a conversation
* (the copilot's free-text follow-ups) use this to know whether the turn's
* instruction is still "unresolved" and must be carried into the next turn
* — see `WorkflowCopilotPanel`'s `pendingAskRef`.
* to a {@link WorkflowBuilderSendResult}: `outcome` lets callers tell a
* retryable no-op (`skipped`) apart from a real dispatch (`dispatched`) or an
* error (`failed`) — a seeded auto-send retries only on `skipped`, never on
* `failed`, so a dispatch error can't loop into duplicate turns — while
* `proposed` reports whether this turn's `flows_build` call returned a
* proposal (vs. a clarifying question / error / no-op), so a looping caller
* knows whether the instruction is still unresolved.
*/
send: (params: WorkflowBuilderSendParams) => Promise<{ proposed: boolean }>;
send: (params: WorkflowBuilderSendParams) => Promise<WorkflowBuilderSendResult>;
/** Clear the current proposal (e.g. after Accept/Reject) without persisting. */
clearProposal: () => void;
}
@@ -239,15 +269,18 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
const sending = localSending;
const send = useCallback(
async ({ displayText, request }: WorkflowBuilderSendParams) => {
async ({
displayText,
request,
}: WorkflowBuilderSendParams): Promise<WorkflowBuilderSendResult> => {
if (localSending) {
log('send: ignored — a turn is already dispatching');
return { proposed: false };
return { outcome: 'skipped', proposed: false };
}
if (socketStatus !== 'connected') {
log('send: blocked — socket not connected (%s)', socketStatus);
setError('offline');
return { proposed: false };
return { outcome: 'skipped', proposed: false };
}
setLocalSending(true);
setError(null);
@@ -333,6 +366,7 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
dispatch(addMessageLocal({ threadId: targetThreadId, message: assistantMessage }));
}
}
return { outcome: 'dispatched', proposed };
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
log('send: failed err=%o', err);
@@ -351,10 +385,12 @@ export function useWorkflowBuilderChat(seedThreadId?: string | null): UseWorkflo
);
setThreadId(null);
}
// A dispatch error must NOT auto-retry (it would resend and duplicate
// the turn) — `failed` is distinct from the retryable `skipped`.
return { outcome: 'failed', proposed };
} finally {
setLocalSending(false);
}
return { proposed };
},
[dispatch, localSending, socketStatus, threadId]
);
+27
View File
@@ -137,11 +137,14 @@ function FlowEditor({
editorFlow,
initialCopilotSeed = null,
initialBuildSeed = null,
onBuildSeedConsumed,
}: {
editorFlow: EditorFlow;
initialCopilotSeed?: CopilotRepairSeed | null;
/** Prompt-bar instant-create seed: open the copilot already building this. */
initialBuildSeed?: CopilotBuildSeed | null;
/** Clear the route's build seed once the copilot has dispatched it (#4597). */
onBuildSeedConsumed?: () => void;
}) {
const { t } = useT();
const navigate = useNavigate();
@@ -539,6 +542,7 @@ function FlowEditor({
onClose={() => setCopilotOpen(false)}
repairSeed={copilotRepairSeed}
buildSeed={initialBuildSeed}
onBuildSeedConsumed={onBuildSeedConsumed}
seedThreadId={copilotThreadId}
onThreadIdChange={handleCopilotThreadId}
/>
@@ -561,6 +565,28 @@ export default function FlowCanvasPage() {
// seed so the copilot opens already building the described workflow.
const buildSeed = useMemo(() => asCopilotBuildSeed(location.state), [location.state]);
// Strip the ephemeral build seed from `location.state` once the copilot has
// dispatched it. The panel's own `buildSentRef` guard is per-mount, so
// closing + reopening the copilot remounts it and would otherwise re-fire the
// same `build` turn against the still-present route seed (issue #4597).
// Preserve any other state fields (e.g. a repair seed) — only drop
// `copilotBuild`.
const clearBuildSeed = useCallback(() => {
// Named `routeState` (not `state`) to avoid shadowing the component-level
// `state` from `useState<LoadState>` that drives this file's render switch.
const routeState = location.state;
if (!routeState || typeof routeState !== 'object' || !('copilotBuild' in routeState)) return;
const next = { ...(routeState as Record<string, unknown>) };
delete next.copilotBuild;
log('build seed consumed — clearing route state: id=%s', id);
// Navigate with an object (not a bare pathname) so the current search and
// hash are preserved — a string target would drop them.
navigate(
{ pathname: location.pathname, search: location.search, hash: location.hash },
{ replace: true, state: next }
);
}, [id, location.state, location.pathname, location.search, location.hash, navigate]);
useEffect(() => {
// Guards a stale response from clobbering newer state: this effect
// re-runs on every `:id` change without the component remounting (same
@@ -620,6 +646,7 @@ export default function FlowCanvasPage() {
}}
initialCopilotSeed={copilotSeed}
initialBuildSeed={buildSeed}
onBuildSeedConsumed={clearBuildSeed}
/>
);
}
@@ -5,7 +5,7 @@
* and the Phase 3d host wiring: Save persists via `flows_update`, and the
* unsaved-changes guard intercepts the Back button while dirty.
*/
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react';
import { createMemoryRouter, MemoryRouter, Route, RouterProvider, Routes } from 'react-router-dom';
import { beforeEach, describe, expect, it, vi } from 'vitest';
@@ -328,6 +328,45 @@ describe('FlowCanvasPage copilot build seed (prompt-bar instant create)', () =>
expect(copilotPanelProps.current?.flowId).toBe('test-id');
});
it('clears only the build seed on consume, preserving sibling route state (#4597)', async () => {
render(
<MemoryRouter
initialEntries={[
{
pathname: '/flows/test-id',
state: {
copilotBuild: { description: 'digest it' },
// A sibling seed (a repair context) must survive the strip: the
// host clones state and deletes ONLY `copilotBuild` — a regression
// that nuked the whole state object would drop this too.
copilotRepair: { runId: 'run-1' },
},
},
]}>
<Routes>
<Route path="/flows/:id" element={<FlowCanvasPage />} />
</Routes>
</MemoryRouter>
);
await waitFor(() =>
expect(copilotPanelProps.current?.buildSeed).toEqual({ description: 'digest it' })
);
// The sibling repair seed is present before consumption.
expect(copilotPanelProps.current?.repairSeed).toMatchObject({ runId: 'run-1' });
// The panel dispatched the build turn and reports it consumed; the host must
// strip `copilotBuild` from `location.state` so a later remount (close +
// reopen) has no seed left to re-fire.
act(() => {
(copilotPanelProps.current?.onBuildSeedConsumed as () => void)();
});
await waitFor(() => expect(copilotPanelProps.current?.buildSeed).toBeNull());
// ...but the sibling repair seed must NOT be nuked along with it.
expect(copilotPanelProps.current?.repairSeed).toMatchObject({ runId: 'run-1' });
});
it('keeps the copilot closed without a seed', async () => {
render(
<MemoryRouter initialEntries={['/flows/test-id']}>