diff --git a/ui/desktop/src/components/BaseChat.tsx b/ui/desktop/src/components/BaseChat.tsx index d3abed351..4b332f8c2 100644 --- a/ui/desktop/src/components/BaseChat.tsx +++ b/ui/desktop/src/components/BaseChat.tsx @@ -80,6 +80,7 @@ import ArtifactViewer from './artifacts/ArtifactViewer'; import { useArtifactPanel } from './artifacts/useArtifactPanel'; import InAppTerminalDock from './InAppTerminalDock'; import { ChatTurnError, hasVisibleTurnErrorMessage } from './conversation/ChatTurnError'; +import { ChatTurnStopped } from './conversation/ChatTurnStopped'; import type { ArtifactRenderError } from './artifacts/ArtifactViewer'; import type { ArtifactSource } from './artifacts/artifactTypes'; import type { LiveBrowserShare } from './artifacts/WebPagePreview'; @@ -1387,6 +1388,7 @@ function BaseChatContent({ steer, sessionLoadError, turnError, + stopConfirmed, setWorkflowUserParams, tokenState, turnStartedAt, @@ -2462,6 +2464,10 @@ function BaseChatContent({ {turnError && !hasVisibleTurnErrorMessage(turnError, messages) && ( )} + {/* F5: a CONFIRMED Stop's outcome, in the slot a + failed Stop's notice takes. Transient — the + store decides when it shows and when it goes. */} + {stopConfirmed && } {/* No tail spacer. A `block h-8` used to sit here, and diff --git a/ui/desktop/src/components/conversation/ChatTurnStopped.test.tsx b/ui/desktop/src/components/conversation/ChatTurnStopped.test.tsx new file mode 100644 index 000000000..c90f8487b --- /dev/null +++ b/ui/desktop/src/components/conversation/ChatTurnStopped.test.tsx @@ -0,0 +1,43 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { render, screen } from '@testing-library/react'; +import { describe, expect, it } from 'vitest'; +import { ChatTurnStopped } from './ChatTurnStopped'; + +/** vitest runs with `ui/desktop` as its root — the idiom `BaseChat.privacy.test.tsx` uses. */ +const read = (...p: string[]) => readFileSync(path.join(process.cwd(), ...p), 'utf8'); + +/** + * F5 (QA of 7c96d796, 2026-09-10): a Stop that worked stated no outcome. The + * store decides WHEN the line shows (`chatStreamStore.test.ts`, "a Stop the + * daemon confirms"); this file pins what it says and where it goes. + */ +describe('ChatTurnStopped', () => { + it('states the outcome in words, in a polite live region', () => { + render(); + expect(screen.getByRole('status')).toHaveTextContent('Stopped.'); + }); + + // The success half of M2's notice is quiet: nothing about this ending is an + // error, and nothing is left for the user to do about it. + it('is neither an alert nor the error card', () => { + render(); + expect(screen.queryByRole('alert')).toBeNull(); + expect(screen.queryByTestId('chat-turn-error')).toBeNull(); + }); + + /** + * BaseChat cannot be mounted in jsdom (see `BaseChat.privacy.test.tsx`), so + * its half is asserted at the source: the line is fed by the store's + * `stopConfirmed`, and it sits in the slot a failed Stop's notice takes — + * the transcript's tail, after the pending tool calls, beside `ChatTurnError`. + */ + it('is rendered by BaseChat from the store, in the failed Stop notice’s slot', () => { + const source = read('src', 'components', 'BaseChat.tsx'); + + expect(source).toMatch(/const \{[^}]*\bstopConfirmed,[^}]*\} = useChatStream\(/); + const tail = //.exec(source); + expect(tail, 'BaseChat no longer ends its transcript with PendingToolCallList').not.toBeNull(); + expect(tail![0]).toMatch(/\}/); + }); +}); diff --git a/ui/desktop/src/components/conversation/ChatTurnStopped.tsx b/ui/desktop/src/components/conversation/ChatTurnStopped.tsx new file mode 100644 index 000000000..53d054735 --- /dev/null +++ b/ui/desktop/src/components/conversation/ChatTurnStopped.tsx @@ -0,0 +1,40 @@ +import Stop from '../ui/Stop'; + +/** + * F5 — what a Stop the daemon CONFIRMED did, said once and quietly. + * + * The success half of M2's notice (`ChatTurnError`'s "Stop not confirmed"). A + * failed Stop gets a card because the user has something to do about it: the + * turn may still be running. A confirmed one gets a line because nothing is left + * to do. The turn is over, Send is back, and the only thing missing was the + * words. So it carries no surface, no status hue and no action — + * `text-supporting` in `--text-muted`, the settings vocabulary's status line + * (rule 6) — in the slot the failed Stop's card would take. + * + * It borrows the trailing activity line's geometry on purpose. That line + * (`TurnActivityIndicator`) is what the user was watching when they pressed + * Stop, and this one lands where it was, at its height, with the Stop button's + * own glyph where the working pulse had been: the working line's last state + * rather than a new element. + * + * WHEN it shows is the store's decision, not this component's + * (`ChatStreamSnapshot.stopConfirmed`): only for a cancel the daemon answered + * `cancelled: true`, never persisted, and retracted after + * `STOP_CONFIRMED_NOTICE_MS` or by the next turn. Reduced motion is handled by + * the global reset in `styles/main.css`. + */ +export function ChatTurnStopped() { + return ( +
+
+ + Stopped. +
+
+ ); +} diff --git a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.privacy.test.tsx b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.privacy.test.tsx index eb5f44bf6..dabcbbdd5 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.privacy.test.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.privacy.test.tsx @@ -112,6 +112,79 @@ describe('SwitchModelModal — pre-flight, not post-refusal', () => { expect(row).toHaveTextContent(/private chat/i); }); + /** + * F3 (QA of 7c96d796, 2026-09-10). A disabled ROW is not a disabled SELECTION. + * The auto-select fills the field with the provider's first model without + * asking `isOptionDisabled`, so opening this on a public provider in a private + * chat put a barred model in the field — and "Select model" stayed live, + * because validity started `true` and was first computed inside the click. + * The click was refused, so the gate held; the pre-flight did not. + * + * ⚠ Fails against the code before the fix: the confirm is enabled and the + * reason is nowhere on screen until something is clicked. + */ + it('disables the confirm, with the reason beside it, before any click on a barred selection', async () => { + render( + + ); + + // The auto-selected model: every row of this provider is barred here. + await screen.findByText('Claude Opus 4.8'); + + const confirm = screen.getByRole('button', { name: 'Select model' }); + expect(confirm).toBeDisabled(); + // The reason is on screen with the menu closed, and it is the confirm's + // own description rather than a sentence that merely happens to be nearby. + const reason = screen.getByText(/private chat, so only private models/i); + expect(reason.id).not.toBe(''); + expect(confirm).toHaveAttribute('aria-describedby', reason.id); + + fireEvent.click(confirm); + expect(mocks.changeModel).not.toHaveBeenCalled(); + }); + + // "Every selection change", not "whatever was there at mount": moving the + // same dialog onto a private provider has to bring the confirm back. + it('re-validates when the selection moves off the barred provider', async () => { + render( + + ); + + await screen.findByText('Claude Opus 4.8'); + const confirm = screen.getByRole('button', { name: 'Select model' }); + expect(confirm).toBeDisabled(); + + fireEvent.change(screen.getAllByRole('combobox')[0], { target: { value: 'Versa' } }); + fireEvent.click(await screen.findByRole('option', { name: 'Versa' })); + + await waitFor(() => expect(confirm).toBeEnabled()); + expect(confirm).not.toHaveAttribute('aria-describedby'); + expect(screen.queryByText(/private chat/i)).toBeNull(); + }); + + // The control for the two above: a private model in the same private chat + // leaves the confirm live and says nothing, so the fix cannot have been + // "disable the confirm in every private chat". + it('leaves the confirm live for a private model in a private chat', async () => { + render( + + ); + + await screen.findByText('Claude Opus 4.8'); + const confirm = screen.getByRole('button', { name: 'Select model' }); + await waitFor(() => expect(confirm).toBeEnabled()); + expect(screen.queryByText(/private chat/i)).toBeNull(); + + fireEvent.click(confirm); + await waitFor(() => expect(mocks.changeModel).toHaveBeenCalledTimes(1)); + }); + // Without this the assertion above passes for a modal that disables EVERY // row, which would be a worse bug than the one it is meant to catch. it('leaves the same row selectable in a public chat', async () => { diff --git a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx index 5fead65d5..db0a47719 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, useCallback, useMemo, useRef } from 'react'; +import { useEffect, useState, useCallback, useId, useMemo, useRef } from 'react'; import { Brain, ExternalLink } from '../../../icons/app-icons'; import { @@ -120,7 +120,12 @@ const modelOptionSearchText = (option: ModelOption) => [option.value, option.label, option.detail].filter(Boolean).join(' ').toLowerCase(); /** - * §14.2's pre-flight reason, rendered ON the row rather than after the attempt. + * §14.2's pre-flight reason, rendered before the attempt rather than after it: + * ON each barred row of the menu, and beside the field — with the confirm + * disabled — whenever the selection itself is barred. The second half is not + * redundant. A disabled row does not stop a barred model reaching the field + * (see `validation`), and until F3 the confirm only found out about one when it + * was clicked. * * States the rule and stops there, deliberately, because for THIS chat there is * no way forward to name. A row's classification only ever rises: @@ -172,8 +177,9 @@ type SwitchModelModalProps = { /** * The tier of the chat being switched (issue #56, §14.2) — "pre-flight, not * post-refusal". A public model in a private chat is rendered disabled with - * the reason inline, instead of being offered, accepted, and then refused by - * Gate A with a 409. + * the reason inline — its row in the menu, and the confirm whenever it is the + * selection — instead of being offered, accepted, and then refused by Gate A + * with a 409. * * `undefined` judges nothing: the settings grid opens this modal with no * session at all (`sessionId={null}`), and a modal that greyed out every @@ -213,11 +219,6 @@ export const SwitchModelModal = ({ initialModel || (carryOverCurrentModel ? currentModel : '') ); const [isCustomModel, setIsCustomModel] = useState(false); - const [validationErrors, setValidationErrors] = useState({ - provider: '', - model: '', - }); - const [isValid, setIsValid] = useState(true); const [attemptedSubmit, setAttemptedSubmit] = useState(false); const [usePredefinedModels] = useState(shouldShowPredefinedModels()); const [selectedPredefinedModel, setSelectedPredefinedModel] = useState(null); @@ -312,55 +313,71 @@ export const SwitchModelModal = ({ [privacyTier, publicProviderNames] ); - // Validate form data - const validateForm = useCallback(() => { - const errors = { - provider: '', - model: '', - }; - let formIsValid = true; + /** + * The form's verdict, DERIVED from the selection on every render — never + * computed inside a click (F3, QA of 7c96d796, 2026-09-10). + * + * ⚠ **A disabled row is not a disabled selection.** `isOptionDisabled` keeps a + * barred model from being PICKED, but the field can hold one the menu never + * offered: the auto-select effect below takes the provider's first model + * without asking it (`findFirstAvailableModel`), `initialModel` and the + * carried-over current model arrive from the caller, and the custom-model + * field and the predefined list bypass the option list outright. Choosing + * Claude Code in a private chat filled the field with a model whose every row + * was disabled while "Select model" stayed live — validity started `true` and + * was first computed inside the click. The click was refused, so the gate + * held; the pre-flight did not. + * + * So the rule is asked of whatever the selection IS. `blocked` is shown beside + * the field and disables the confirm before anything is clicked; + * `attemptedSubmit` only decides whether the "nothing chosen yet" messages + * show, because those are prompts, not refusals. `handleSubmit` reads the same + * verdict, as the fallback for a submit that arrives some other way. + */ + const validation = useMemo(() => { + const errors = { provider: '', model: '' }; + let blocked: string | null = null; if (usePredefinedModels) { if (!selectedPredefinedModel) { errors.model = 'Select a model'; - formIsValid = false; } else { // This branch swaps both selects for a flat radio list and reaches the // same `changeModel`, so it bypasses the option list's pre-flight // exactly the way the custom-model field below does. Guarding only that // one would leave the identical hole open on the sibling path. - const blocked = blockedReasonFor(selectedPredefinedModel.provider); - if (blocked) { - errors.model = blocked; - formIsValid = false; - } + blocked = blockedReasonFor(selectedPredefinedModel.provider); } } else { if (!provider) { errors.provider = 'Select a provider'; - formIsValid = false; } if (!model) { errors.model = 'Select or type a model name'; - formIsValid = false; - } - - // The custom-model field bypasses the option list entirely, so the same - // rule has to be asked again here or "Enter a model not listed…" would be - // the one way around the pre-flight. - const blocked = blockedReasonFor(provider); - if (blocked && model) { - errors.model = blocked; - formIsValid = false; + } else { + // The custom-model field bypasses the option list entirely, so the same + // rule has to be asked again here or "Enter a model not listed…" would be + // the one way around the pre-flight. + blocked = blockedReasonFor(provider); } } + if (blocked) errors.model = blocked; - setValidationErrors(errors); - setIsValid(formIsValid); - return formIsValid; + return { errors, blocked, isValid: !errors.provider && !errors.model }; }, [model, provider, usePredefinedModels, selectedPredefinedModel, blockedReasonFor]); + // A refusal shows at once, because it is WHY the confirm is disabled; a + // prompt to choose something waits for an attempt. One node, rendered under + // whichever field is on screen, so the confirm can name it as its reason. + const modelMessageId = useId(); + const modelMessage = validation.blocked ?? (attemptedSubmit ? validation.errors.model : ''); + const modelMessageNode = modelMessage ? ( +
+ {modelMessage} +
+ ) : null; + const handleClose = () => { onClose(); }; @@ -397,8 +414,10 @@ export const SwitchModelModal = ({ } setAttemptedSubmit(true); setSubmitError(null); - const isFormValid = validateForm(); - if (!isFormValid) return; + // The confirm is already disabled whenever this is false; this is the + // post-click half of the same verdict, for a submit that reaches here some + // other way (a keyboard submit, a caller that renders its own confirm). + if (!validation.isValid) return; setSwitching(true); try { @@ -443,13 +462,6 @@ export const SwitchModelModal = ({ } }; - // Re-validate when inputs change and after attempted submission - useEffect(() => { - if (attemptedSubmit) { - validateForm(); - } - }, [attemptedSubmit, validateForm]); - useEffect(() => { // Load predefined models if enabled if (usePredefinedModels) { @@ -725,7 +737,7 @@ export const SwitchModelModal = ({ key={model.id || model.name} // The predefined branch swaps both selects for this flat // list, so it bypasses `isDisabled` on them entirely — the - // same hole `validateForm` documents for the tier + // same hole `validation` documents for the tier // pre-flight, and it has to be closed here for the same // reason. onClick={hostManaged ? undefined : () => setSelectedPredefinedModel(model)} @@ -784,9 +796,7 @@ export const SwitchModelModal = ({ })} - {attemptedSubmit && validationErrors.model && ( -
{validationErrors.model}
- )} + {modelMessageNode} ) : ( /* Manual Provider/Model Selection */ @@ -823,8 +833,8 @@ export const SwitchModelModal = ({ isClearable isDisabled={hostManaged} /> - {attemptedSubmit && validationErrors.provider && ( -
{validationErrors.provider}
+ {attemptedSubmit && validation.errors.provider && ( +
{validation.errors.provider}
)} {/* Issue #56, DR-26. Whose agreements cover the models under this @@ -872,11 +882,7 @@ export const SwitchModelModal = ({ isDisabled={loadingModels || hostManaged} /> - {attemptedSubmit && validationErrors.model && ( -
- {validationErrors.model} -
- )} + {modelMessageNode} ) : (
@@ -896,11 +902,7 @@ export const SwitchModelModal = ({ value={model} disabled={hostManaged} /> - {attemptedSubmit && validationErrors.model && ( -
- {validationErrors.model} -
- )} + {modelMessageNode}
)} @@ -936,12 +938,15 @@ export const SwitchModelModal = ({ diff --git a/ui/desktop/src/hooks/chatStreamStore.test.ts b/ui/desktop/src/hooks/chatStreamStore.test.ts index ee26ab45f..d65e94162 100644 --- a/ui/desktop/src/hooks/chatStreamStore.test.ts +++ b/ui/desktop/src/hooks/chatStreamStore.test.ts @@ -1,6 +1,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ChatState } from '../types/chatState'; -import { ChatStreamRegistry, NOTIFY_FALLBACK_MS, isRunningState } from './chatStreamStore'; +import { + ChatStreamRegistry, + NOTIFY_FALLBACK_MS, + STOP_CONFIRMED_NOTICE_MS, + isRunningState, +} from './chatStreamStore'; import type { Message, MessageEvent, Session, TokenState } from '../api'; import { cancelTurn, editMessage, getSession, interrupt, reply, resumeAgent } from '../api'; import { abandonContinuationLease, recoverContinuationGroup } from '../utils/continuationLease'; @@ -1919,6 +1924,20 @@ describe('ChatStreamRegistry', () => { }); }); +/** + * The daemon's synthesized "this turn produced no ending" frame. Shared by the + * M2 and F5 batteries below, which both need a Stop's wedged-writer ending. + */ +function endedWithoutTerminal(): MessageEvent { + return { + type: 'Error', + error: 'The stream for this turn ended without a result. Please retry.', + code: 'stream_ended_without_terminal', + scope: 'internal', + retryable: true, + } as MessageEvent; +} + /** * M2 — a Stop the daemon never confirms. * @@ -1937,17 +1956,6 @@ describe('ChatStreamRegistry', () => { * || {}`) and the console warning printed nothing a person could act on. */ describe('ChatStreamRegistry — a Stop the daemon never confirms (M2)', () => { - /** The daemon's synthesized "this turn produced no ending" frame. */ - function endedWithoutTerminal(): MessageEvent { - return { - type: 'Error', - error: 'The stream for this turn ended without a result. Please retry.', - code: 'stream_ended_without_terminal', - scope: 'internal', - retryable: true, - } as MessageEvent; - } - /** * Drive a Stop whose cancel is still on the wire when the daemon's terminal * frame lands, then settle the cancel however the caller asks. @@ -2150,6 +2158,228 @@ describe('ChatStreamRegistry — a Stop the daemon never confirms (M2)', () => { }); }); +/** + * F5 (QA of 7c96d796, 2026-09-10) — a Stop the daemon CONFIRMS. + * + * M2 gave the failed Stop its notice; the successful one still said nothing. + * Measured in the running app: Send came back 150 ms after the press, and the + * transcript held the user's message with no reply and no word about why. + * + * The notice is keyed on the daemon's own answer, not on the renderer's hope. + * `cancelled: true` means the cancel found the turn running and tripped it; + * `cancelled: false` is the idempotent 200 for a turn that had already ended, + * which is exactly the turn that must never be described as stopped. + */ +describe('ChatStreamRegistry — a Stop the daemon confirms (F5)', () => { + /** A turn that stays open until the test ends it. */ + async function startLongTurn(sessionId: string) { + const registry = new ChatStreamRegistry(); + const controlled = createControlledStream(); + vi.mocked(resumeAgent).mockResolvedValue({ data: { session: session(sessionId) } } as never); + vi.mocked(reply).mockResolvedValue({ stream: controlled.stream } as never); + + const controller = registry.getController(sessionId); + const submit = controller.handleSubmit('a long turn'); + await vi.waitFor(() => expect(reply).toHaveBeenCalledTimes(1)); + return { controller, controlled, submit }; + } + + /** Stop, and let the daemon's terminal frame land before its cancel answer. */ + async function stopWithFrameFirst(sessionId: string, frame: MessageEvent, answer: unknown) { + const { controller, controlled, submit } = await startLongTurn(sessionId); + const cancellation = deferred(); + vi.mocked(cancelTurn).mockReturnValueOnce(cancellation.promise as never); + + const stopped = controller.stopStreaming(); + await flush(); + controlled.push(frame); + controlled.close(); + await submit; + + cancellation.resolve(answer); + return { controller, stopped }; + } + + it('says the turn was stopped once the daemon confirms it', async () => { + const { controller, controlled, submit } = await startLongTurn('stop-confirmed'); + vi.mocked(cancelTurn).mockResolvedValueOnce({ + data: { cancelled: true, settled: true }, + } as never); + + await expect(controller.stopStreaming()).resolves.toBe(true); + + expect(controller.getSnapshot().chatState).toBe(ChatState.Idle); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + // A quiet line, not a card: nothing about this ending is an error. + expect(controller.getSnapshot().turnError).toBeUndefined(); + + controlled.close(); + await submit; + }); + + // The healthy daemon's other ordering: its real `Finish { reason: cancelled }` + // beats the cancel response, so the Stop gate defers the Idle transition to it. + it('says so when the cancelled turn’s own ending arrives before the confirmation', async () => { + const { controller, stopped } = await stopWithFrameFirst( + 'stop-confirmed-frame-first', + { type: 'Finish', reason: 'cancelled', token_state: tokenState } as MessageEvent, + { data: { cancelled: true, settled: true } } + ); + + await expect(stopped).resolves.toBe(true); + expect(controller.getSnapshot().chatState).toBe(ChatState.Idle); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + expect(controller.getSnapshot().turnError).toBeUndefined(); + }); + + /** + * A wedged writer ends the turn with the daemon's synthesized frame, which + * M2 re-codes to a "Turn stopped" CARD while the cancel is still out. Once the + * cancel confirms, that card and this line would say one thing twice, in an + * error's voice and a status's; the confirmation is the stronger evidence. + */ + it('replaces the interim “Turn stopped” card once the cancel confirms', async () => { + const { controller, stopped } = await stopWithFrameFirst( + 'stop-confirmed-wedged-writer', + endedWithoutTerminal(), + { data: { cancelled: true, settled: true } } + ); + + await expect(stopped).resolves.toBe(true); + expect(controller.getSnapshot().turnError).toBeUndefined(); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + }); + + it('is transient — it retracts itself after its display window', async () => { + const { controller, controlled, submit } = await startLongTurn('stop-confirmed-transient'); + await expect(controller.stopStreaming()).resolves.toBe(true); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + + vi.advanceTimersByTime(STOP_CONFIRMED_NOTICE_MS - 1); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + vi.advanceTimersByTime(1); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + + controlled.close(); + await submit; + }); + + it('is retracted the moment the next turn starts', async () => { + const { controller, controlled, submit } = await startLongTurn('stop-confirmed-next-turn'); + await expect(controller.stopStreaming()).resolves.toBe(true); + expect(controller.getSnapshot().stopConfirmed).toBeDefined(); + controlled.close(); + await submit; + + const next = createControlledStream(); + vi.mocked(reply).mockResolvedValue({ stream: next.stream } as never); + const nextSubmit = controller.handleSubmit('carry on'); + await vi.waitFor(() => expect(reply).toHaveBeenCalledTimes(2)); + + // While the new turn runs — not merely once it has ended. + expect(isRunningState(controller.getSnapshot().chatState)).toBe(true); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + + next.push({ type: 'Finish', reason: 'done', token_state: tokenState } as MessageEvent); + next.close(); + await nextSubmit; + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + }); + + // ---- The endings that must NOT claim a stop. These pass before the fix too; + // they are what keeps the fix from being "announce every Idle". ---- + + it('does not appear for a turn that ended on its own', async () => { + const { controller, controlled, submit } = await startLongTurn('ended-on-its-own'); + + controlled.push({ type: 'Finish', reason: 'done', token_state: tokenState } as MessageEvent); + controlled.close(); + await submit; + + expect(controller.getSnapshot().chatState).toBe(ChatState.Idle); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + }); + + // The race a Stop can lose: the turn finished by itself a moment before the + // cancel reached it, and the daemon answers that nothing was running. + it('does not appear when the turn had already finished before the cancel reached it', async () => { + const { controller, stopped } = await stopWithFrameFirst( + 'finished-before-the-cancel', + { type: 'Finish', reason: 'done', token_state: tokenState } as MessageEvent, + { data: { cancelled: false, settled: true } } + ); + + await expect(stopped).resolves.toBe(true); + expect(controller.getSnapshot().chatState).toBe(ChatState.Idle); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + }); + + it('does not appear beside the notice for a Stop that was never confirmed', async () => { + // Installed first: the failed cancel logs from a microtask that runs before + // the helper below hands control back. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const { controller, stopped } = await stopWithFrameFirst( + 'stop-unconfirmed', + endedWithoutTerminal(), + // A bare 504: the daemon's settlement timeout. + { error: {}, response: { status: 504 } } + ); + await expect(stopped).resolves.toBe(false); + await flush(); + + expect(controller.getSnapshot().turnError?.code).toBe('stop_not_confirmed'); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + } finally { + warn.mockRestore(); + } + }); + + // Stop-and-Send is followed at once by the user's replacement turn, and that + // turn is the outcome; a "Stopped." line would flash for the length of a submit. + it('does not appear for a Stop-and-Send', async () => { + const { controller, controlled, submit } = await startLongTurn('stop-and-send'); + + await expect(controller.stopStreaming(true)).resolves.toBe(true); + expect(controller.getSnapshot().pendingContinuation?.ownership).toBe('owned'); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + + controlled.close(); + await submit; + }); + + // The case above's harder sibling: an ORDINARY Stop the user upgrades while + // it is still on the wire. That Stop's own cancel comes back `cancelled: + // true` — and the replacement turn is still the outcome. + it('does not appear for an ordinary Stop upgraded to Stop-and-Send mid-flight', async () => { + const { controller, controlled, submit } = await startLongTurn('stop-upgraded-mid-flight'); + const ordinaryCancellation = deferred(); + const continuationAdmission = deferred(); + vi.mocked(cancelTurn) + .mockReturnValueOnce(ordinaryCancellation.promise as never) + .mockReturnValueOnce(continuationAdmission.promise as never); + + const ordinaryStop = controller.stopStreaming(false); + await vi.waitFor(() => expect(cancelTurn).toHaveBeenCalledTimes(1)); + const stopAndSend = controller.stopStreaming(true); + + ordinaryCancellation.resolve({ data: { cancelled: true, settled: true } }); + await expect(ordinaryStop).resolves.toBe(true); + // Between the two requests: exactly where a line keyed on the ordinary + // Stop alone would flash. + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + + continuationAdmission.resolve({ + data: { cancelled: false, settled: true, continuation_lease: 'lease-upgraded' }, + }); + await expect(stopAndSend).resolves.toBe(true); + expect(controller.getSnapshot().stopConfirmed).toBeUndefined(); + + controlled.close(); + await submit; + }); +}); + /** * Progressive conversation loading. * diff --git a/ui/desktop/src/hooks/chatStreamStore.tsx b/ui/desktop/src/hooks/chatStreamStore.tsx index 20c084a8e..fd6eec56b 100644 --- a/ui/desktop/src/hooks/chatStreamStore.tsx +++ b/ui/desktop/src/hooks/chatStreamStore.tsx @@ -470,6 +470,23 @@ export interface ChatStreamSnapshot { pendingSteer?: PendingSteer; /** A durable Stop-and-Send gap discovered from the daemon on resume. */ pendingContinuation?: PendingContinuationView; + /** + * F5 — this chat's last Stop provably ended a running turn: the daemon + * answered its exact-generation cancel `cancelled: true, settled: true`. + * BaseChat renders it as a quiet "Stopped." line in the slot a failed Stop's + * notice takes (`ChatTurnStopped`), because until this a Stop that WORKED + * stated no outcome at all. + * + * Transient and never persisted: retracted `STOP_CONFIRMED_NOTICE_MS` later, + * and at once by anything that starts or joins a turn in this chat. + * + * ⚠ Absent for every other ending, which is why it is keyed on the daemon's + * `cancelled` rather than on the press: a turn that finished on its own, a + * Stop that raced the turn to its end (`cancelled: false`, the daemon's + * idempotent answer), a Stop the daemon never confirmed (M2's card speaks + * instead), and a Stop-and-Send, whose replacement turn is the outcome. + */ + stopConfirmed?: StopConfirmedView; /** * Whether this session's agent — model provider + extensions — has finished * loading on the backend. The transcript paints before this flips (see @@ -522,6 +539,12 @@ export interface PinnedModelView { model: string; } +/** F5 — a Stop the daemon confirmed. See `ChatStreamSnapshot.stopConfirmed`. */ +export interface StopConfirmedView { + /** The exact generation the confirmed cancel named. */ + turnId: string; +} + /** A tool call announced before its arguments finished streaming (§6.1b). */ export interface PendingToolCallView { id: string; @@ -545,6 +568,13 @@ export const TURN_STOPPED_BY_USER = 'turn_stopped_by_user'; /** A Stop whose cancel never came back confirmed. */ export const STOP_NOT_CONFIRMED = 'stop_not_confirmed'; +/** + * F5 — how long a confirmed Stop's "Stopped." line stays in the transcript. + * design.md §4.3's toast duration, so the app's transient confirmations last + * one length of time rather than two. + */ +export const STOP_CONFIRMED_NOTICE_MS = 5000; + /** * The in-chat notice for a Stop the daemon never confirmed (M2). * @@ -795,6 +825,15 @@ class ChatStreamController { * notice written over the top of it would be both wrong and destructive. */ private lastStopFailure: string | null = null; + /** + * F5 — whether the last exact-generation cancel that SETTLED found the turn + * running and tripped it (`cancelled: true`), rather than finding it already + * over (`cancelled: false`, the daemon's idempotent answer to a Stop that + * raced the turn's own ending). Only the first is a stop the user caused, so + * only it earns `stopConfirmed`. Reset with `lastStopFailure` at the top of + * every cancel request. + */ + private lastStopCancelled = false; /** * The turn this controller is currently rendering — the id it POSTed, or the * id it attached to. Held so a re-attach can re-POST the SAME turn (rather @@ -1900,6 +1939,7 @@ class ChatStreamController { session: undefined, sessionLoadError: undefined, turnError: undefined, + stopConfirmed: undefined, chatState: ChatState.LoadingConversation, })); @@ -3123,6 +3163,7 @@ class ChatStreamController { chatState: ChatState.Streaming, turnStartedAt: prev.turnStartedAt ?? Date.now(), turnError: undefined, + stopConfirmed: undefined, })); await this.streamFromResponse( stream as AsyncIterable, @@ -3229,7 +3270,8 @@ class ChatStreamController { if ( prev.chatState === chatState && prev.turnStartedAt === turnStartedAt && - prev.turnError === undefined + prev.turnError === undefined && + prev.stopConfirmed === undefined ) { return prev; } @@ -3238,6 +3280,7 @@ class ChatStreamController { chatState, turnStartedAt, turnError: undefined, + stopConfirmed: undefined, }; }); } @@ -3324,6 +3367,8 @@ class ChatStreamController { notifications: [], pendingToolCalls: [], turnError: undefined, + // F5 — "Stopped." spoke about the previous turn; this one supersedes it. + stopConfirmed: undefined, turnStartedAt: Date.now(), lastMessageAt: undefined, pendingSteer: undefined, @@ -3775,6 +3820,7 @@ class ChatStreamController { continuationPending: boolean ): Promise => { this.lastStopFailure = null; + this.lastStopCancelled = false; try { const body = { session_id: this.sessionId, @@ -3800,6 +3846,7 @@ class ChatStreamController { } const data = result?.data; if (data?.settled === true) { + this.lastStopCancelled = data.cancelled === true; if (!continuationPending) return true; const lease = data.continuation_lease; if (!lease) { @@ -3940,6 +3987,16 @@ class ChatStreamController { return false; } + // F5 — say that it worked, but only when the daemon says the Stop is what + // ended the turn. Sampled BEFORE `stopContinuationPending` is cleared below: + // an ordinary Stop the user upgraded to Stop-and-Send while it was on the + // wire is followed at once by their replacement turn, and that turn is the + // outcome — not a line that flashes for the length of one submit. + const confirmedStop: StopConfirmedView | undefined = + this.lastStopCancelled && !requestContinuationPending && !this.stopContinuationPending + ? { turnId: stoppedTurnId } + : undefined; + this.activeStreamId += 1; this.abortController?.abort(); this.endReplayHold(); @@ -3957,12 +4014,36 @@ class ChatStreamController { lastMessageAt: undefined, pendingSteer: undefined, // A retry that succeeded retracts the notice the failed attempt raised. - turnError: prev.turnError?.code === STOP_NOT_CONFIRMED ? undefined : prev.turnError, + // A confirmed stop also retracts M2's interim "Turn stopped" card — a + // wedged writer's synthesized ending raises it while the cancel is still + // out — because it says what `stopConfirmed` says, in an error's voice. + turnError: + prev.turnError?.code === STOP_NOT_CONFIRMED || + (confirmedStop && prev.turnError?.code === TURN_STOPPED_BY_USER) + ? undefined + : prev.turnError, + ...(confirmedStop ? { stopConfirmed: confirmedStop } : {}), })); + if (confirmedStop) this.retractStopConfirmedLater(confirmedStop); this.flushNotify(); return true; }; + /** + * F5 — the confirmed-stop line is transient. Identity-checked, so a timer + * armed for one notice can never retract a later one, and a notice a new + * turn already retracted is left alone. Untracked on purpose: a stale timer + * is a no-op, and the registry never drops a controller outside + * `resetForTests`. + */ + private retractStopConfirmedLater(notice: StopConfirmedView): void { + setTimeout(() => { + this.updateSnapshot((prev) => + prev.stopConfirmed === notice ? { ...prev, stopConfirmed: undefined } : prev + ); + }, STOP_CONFIRMED_NOTICE_MS); + } + /** * M2 — finish what the Stop gate deferred, and say out loud that the stop did * not take. diff --git a/ui/desktop/src/hooks/useChatStream.ts b/ui/desktop/src/hooks/useChatStream.ts index c72649e57..1f6a0afcb 100644 --- a/ui/desktop/src/hooks/useChatStream.ts +++ b/ui/desktop/src/hooks/useChatStream.ts @@ -7,6 +7,7 @@ import { type PendingContinuationView, type PendingToolCallView, type PinnedModelView, + type StopConfirmedView, } from './chatStreamStore'; import type { ContinuationRecoveryAction } from '../utils/continuationLease'; import type { ChatTurnErrorData } from '../types/turnError'; @@ -60,6 +61,8 @@ interface UseChatStreamReturn { /** BR-61: a soft interrupt issued but not yet echoed back by the agent. */ pendingSteer?: PendingSteer; pendingContinuation?: PendingContinuationView; + /** F5: the daemon confirmed that this chat's last Stop ended a running turn. Transient. */ + stopConfirmed?: StopConfirmedView; /** * Whether the session's model + extensions have finished loading. The * transcript is up well before this — anything reading AGENT state must gate @@ -138,6 +141,7 @@ export function useChatStream({ lastMessageAt: snapshot.lastMessageAt, pendingSteer: snapshot.pendingSteer, pendingContinuation: snapshot.pendingContinuation, + stopConfirmed: snapshot.stopConfirmed, agentReady: snapshot.agentReady, notifications: notificationsMap, pendingToolCalls: snapshot.pendingToolCalls,