From 27b62175edf1ee9b13a85a048a351836f674344b Mon Sep 17 00:00:00 2001 From: YellowSnnowmann <167776381+YellowSnnowmann@users.noreply.github.com> Date: Thu, 9 Jul 2026 07:22:30 +0530 Subject: [PATCH] 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 --- app/src-tauri/src/telegram_scanner/mod.rs | 5 +- .../flows/WorkflowCopilotPanel.test.tsx | 137 +++++++++++++++++- .../components/flows/WorkflowCopilotPanel.tsx | 43 +++++- app/src/hooks/useWorkflowBuilderChat.test.ts | 63 +++++++- app/src/hooks/useWorkflowBuilderChat.ts | 58 ++++++-- app/src/pages/FlowCanvasPage.tsx | 27 ++++ .../pages/__tests__/FlowCanvasPage.test.tsx | 41 +++++- 7 files changed, 348 insertions(+), 26 deletions(-) diff --git a/app/src-tauri/src/telegram_scanner/mod.rs b/app/src-tauri/src/telegram_scanner/mod.rs index e55e21be0..92a1ad9e3 100644 --- a/app/src-tauri/src/telegram_scanner/mod.rs +++ b/app/src-tauri/src/telegram_scanner/mod.rs @@ -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, diff --git a/app/src/components/flows/WorkflowCopilotPanel.test.tsx b/app/src/components/flows/WorkflowCopilotPanel.test.tsx index 6c9322c83..9a744770b 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.test.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.test.tsx @@ -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( + + ); + 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( + + ); + 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( + + ); + 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( + + ); + 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( + + ); + 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( + + ); + 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( 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 diff --git a/app/src/hooks/useWorkflowBuilderChat.test.ts b/app/src/hooks/useWorkflowBuilderChat.test.ts index e0be52332..4b8e0c5c0 100644 --- a/app/src/hooks/useWorkflowBuilderChat.test.ts +++ b/app/src/hooks/useWorkflowBuilderChat.test.ts @@ -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 => ( 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 = { diff --git a/app/src/hooks/useWorkflowBuilderChat.ts b/app/src/hooks/useWorkflowBuilderChat.ts index 1a290b8ea..4e0c4e86b 100644 --- a/app/src/hooks/useWorkflowBuilderChat.ts +++ b/app/src/hooks/useWorkflowBuilderChat.ts @@ -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; /** 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 => { 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] ); diff --git a/app/src/pages/FlowCanvasPage.tsx b/app/src/pages/FlowCanvasPage.tsx index a95d25651..0752378ef 100644 --- a/app/src/pages/FlowCanvasPage.tsx +++ b/app/src/pages/FlowCanvasPage.tsx @@ -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` 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) }; + 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} /> ); } diff --git a/app/src/pages/__tests__/FlowCanvasPage.test.tsx b/app/src/pages/__tests__/FlowCanvasPage.test.tsx index 3b46338dc..bb157b286 100644 --- a/app/src/pages/__tests__/FlowCanvasPage.test.tsx +++ b/app/src/pages/__tests__/FlowCanvasPage.test.tsx @@ -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( + + + } /> + + + ); + + 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(