diff --git a/app/src/components/flows/WorkflowCopilotPanel.test.tsx b/app/src/components/flows/WorkflowCopilotPanel.test.tsx index 0fe3fdea7..d46ae467e 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.test.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.test.tsx @@ -301,8 +301,8 @@ describe('WorkflowCopilotPanel', () => { expect(screen.getByTestId('workflow-copilot-removed')).toBeInTheDocument(); }); - it('Accept applies to the draft and clears the proposal (never persists)', () => { - const onAccept = vi.fn(); + it('Accept calls onAccept (host applies + saves) and clears the proposal once it resolves', async () => { + const onAccept = vi.fn().mockResolvedValue(undefined); hookState.proposal = proposalWith(['a', 'c']); render( { ); fireEvent.click(screen.getByTestId('workflow-copilot-accept')); expect(onAccept).toHaveBeenCalledWith(hookState.proposal); - expect(hookState.clearProposal).toHaveBeenCalledTimes(1); + await waitFor(() => expect(hookState.clearProposal).toHaveBeenCalledTimes(1)); + }); + + it('shows the saving label and disables Accept while the host save is in flight', async () => { + // Deferred promise so the test controls exactly when the host's save + // (`onAccept`) resolves, to observe the in-between "saving" state. + let resolveSave!: () => void; + const savePromise = new Promise(resolve => { + resolveSave = resolve; + }); + const onAccept = vi.fn().mockReturnValue(savePromise); + hookState.proposal = proposalWith(['a', 'c']); + render( + + ); + + fireEvent.click(screen.getByTestId('workflow-copilot-accept')); + await waitFor(() => + expect(screen.getByTestId('workflow-copilot-accept')).toHaveTextContent( + 'flows.copilot.saving' + ) + ); + expect(screen.getByTestId('workflow-copilot-accept')).toBeDisabled(); + expect(hookState.clearProposal).not.toHaveBeenCalled(); + + resolveSave(); + await waitFor(() => expect(hookState.clearProposal).toHaveBeenCalledTimes(1)); + }); + + it('leaves the proposal visible for retry when the host save rejects', async () => { + const onAccept = vi.fn().mockRejectedValue(new Error('save failed')); + hookState.proposal = proposalWith(['a', 'c']); + render( + + ); + + fireEvent.click(screen.getByTestId('workflow-copilot-accept')); + await waitFor(() => expect(onAccept).toHaveBeenCalledTimes(1)); + // The button re-enables once the rejected save settles, and the proposal + // was never cleared — the card stays up so the user can retry. + await waitFor(() => expect(screen.getByTestId('workflow-copilot-accept')).not.toBeDisabled()); + expect(hookState.clearProposal).not.toHaveBeenCalled(); + }); + + it('disables Reject while an Accept save is in flight, so it cannot race the persisted save', async () => { + // Regression for the CodeRabbit finding: Reject must not stay clickable + // while `onAccept`'s save is still pending, otherwise the user's cancel + // can be silently overridden by the earlier Accept's save landing after. + let resolveSave!: () => void; + const savePromise = new Promise(resolve => { + resolveSave = resolve; + }); + const onAccept = vi.fn().mockReturnValue(savePromise); + const onReject = vi.fn(); + hookState.proposal = proposalWith(['a', 'c']); + render( + + ); + + fireEvent.click(screen.getByTestId('workflow-copilot-accept')); + await waitFor(() => expect(screen.getByTestId('workflow-copilot-reject')).toBeDisabled()); + + // A click while disabled is a no-op in jsdom/RTL — Reject must not fire. + fireEvent.click(screen.getByTestId('workflow-copilot-reject')); + expect(onReject).not.toHaveBeenCalled(); + expect(hookState.clearProposal).not.toHaveBeenCalled(); + + resolveSave(); + await waitFor(() => expect(hookState.clearProposal).toHaveBeenCalledTimes(1)); + expect(screen.getByTestId('workflow-copilot-reject')).not.toBeDisabled(); }); it('Reject discards the proposal without applying it', () => { diff --git a/app/src/components/flows/WorkflowCopilotPanel.tsx b/app/src/components/flows/WorkflowCopilotPanel.tsx index d897fcedc..582f3ea7c 100644 --- a/app/src/components/flows/WorkflowCopilotPanel.tsx +++ b/app/src/components/flows/WorkflowCopilotPanel.tsx @@ -16,8 +16,13 @@ * `streamingAssistantByThread`, streamed here by Phase B). So the copilot reads * like a real chat rather than a one-shot form. * - * Invariant: the copilot only PROPOSES. Accept applies to the UNSAVED local - * draft (no `flows_update`); persistence stays behind the canvas's own Save. + * Invariant: the copilot only PROPOSES — the agent turn itself never + * persists. Accept applies the proposal to the local draft AND immediately + * saves it (review + save in one click) via the host's `onAccept`, which + * awaits the host's own persistence call; the panel shows a saving state + * meanwhile and, if the save fails, leaves the proposal visible for retry + * rather than silently discarding it. Reject remains local-only (revert the + * overlay, no persistence call). */ import createDebug from 'debug'; import { useCallback, useEffect, useRef, useState } from 'react'; @@ -65,8 +70,13 @@ interface Props { * reflects it. */ onProposal: (proposal: WorkflowProposal) => void; - /** Accept the pending proposal into the local draft (host commits it). */ - onAccept: (proposal: WorkflowProposal) => void; + /** + * Accept the pending proposal (host applies it to the local draft AND + * persists it — "accept" is now review + save in one step). May return a + * promise the panel awaits to show a saving state; a rejected promise + * leaves the proposal visible so the user can retry. + */ + onAccept: (proposal: WorkflowProposal) => void | Promise; /** Reject the pending proposal (host reverts the overlay). */ onReject: () => void; /** Close the panel. */ @@ -376,12 +386,35 @@ export default function WorkflowCopilotPanel({ const noopAttach = useCallback(async () => {}, []); const noop = useCallback(() => {}, []); - const accept = useCallback(() => { - if (!proposal) return; - onAccept(proposal); - clearProposal(); - lastSurfacedRef.current = null; - }, [proposal, onAccept, clearProposal]); + // Accept now review-and-saves: `onAccept` (the host's `handleAcceptProposal`) + // applies the proposal to the draft AND persists it. Track a local + // `acceptSaving` flag so the button can show a saving state and disable + // re-clicks while that's in flight. If the host's save throws, leave the + // proposal card visible (don't `clearProposal()`) so the user can retry — + // otherwise a failed autosave would silently vanish the only affordance to + // try again from the copilot itself (the header Save button is a fallback, + // but this keeps the copilot's own flow self-contained). + const [acceptSaving, setAcceptSaving] = useState(false); + const accept = useCallback(async () => { + // Self-guard against re-entrance: the JSX `disabled={acceptSaving}` on + // the Accept button prevents a normal double-click, but `acceptSaving` + // only flips after the FIRST call's `setAcceptSaving(true)` commits — a + // second invocation racing ahead of that render (e.g. programmatic + // re-fire) must not start a second concurrent save. + if (!proposal || acceptSaving) return; + setAcceptSaving(true); + log('accept: saving proposal via host onAccept'); + try { + await onAccept(proposal); + log('accept: save succeeded, clearing proposal'); + clearProposal(); + lastSurfacedRef.current = null; + } catch (err) { + log('accept: save failed, leaving proposal visible for retry err=%o', err); + } finally { + setAcceptSaving(false); + } + }, [proposal, acceptSaving, onAccept, clearProposal]); const reject = useCallback(() => { onReject(); @@ -533,14 +566,16 @@ export default function WorkflowCopilotPanel({ type="button" variant="primary" size="sm" + disabled={acceptSaving} data-testid="workflow-copilot-accept" - onClick={accept}> - {t('flows.copilot.accept')} + onClick={() => void accept()}> + {acceptSaving ? t('flows.copilot.saving') : t('flows.copilot.acceptAndSave')}