From 8eefbc5287f73414df7feba6c78dc8ca73783b1d Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Wed, 21 Oct 2026 14:26:35 +0900 Subject: fix(review-followup-fea): clarify Stop with queued input --- src/app/App.test.ts | 72 ++++++++++++++++++++++ src/app/store-types.ts | 7 ++- src/app/store.svelte.ts | 4 +- src/app/surface-protocol.svelte.ts | 20 +++++- src/app/surface-protocol.test.ts | 65 +++++++++++++++++++ src/app/views/ComposerPane.svelte | 8 +++ src/features/chat/logic/composer.test.ts | 20 ++++++ src/features/chat/logic/composer.ts | 11 ++++ src/features/chat/ui.test.ts | 51 +++++++++++++++ src/features/chat/ui/Composer.svelte | 18 ++++-- src/features/surface-host/index.ts | 1 + .../surface-host/logic/message-queue.test.ts | 35 +++++++++++ src/features/surface-host/logic/message-queue.ts | 10 +++ 13 files changed, 312 insertions(+), 10 deletions(-) create mode 100644 src/app/surface-protocol.test.ts create mode 100644 src/features/chat/logic/composer.test.ts create mode 100644 src/features/chat/logic/composer.ts diff --git a/src/app/App.test.ts b/src/app/App.test.ts index 14dac3a..62798d6 100644 --- a/src/app/App.test.ts +++ b/src/app/App.test.ts @@ -481,6 +481,78 @@ describe("App component interaction tests", () => { store.dispose(); }); + it("uses only the focused conversation's queue snapshot for the interrupt label and POSTs stop", async () => { + const ws = fakeSocket(); + const fetchImpl = vi.fn(fakeFetchImpl()); + const store = createAppStore({ + socketFactory: () => ws, + fetchImpl, + localStorage: createFakeStorage(), + chatFlushScheduler: null, + }); + ws.resolveOpen(); + render(App, { props: { store, onNavigate: vi.fn() } }); + const user = userEvent.setup(); + await user.type(screen.getByRole("textbox", { name: "Message input" }), "start"); + await user.click(screen.getByRole("button", { name: "Send" })); + const firstId = activeConversationId(store); + const startTurn = (conversationId: string) => + ws.feedServerMessage({ + type: "chat.delta", + event: { type: "turn-start", conversationId, turnId: `turn-${conversationId}` }, + }); + const pushQueue = (conversationId: string, payload: unknown) => + ws.feedSurfaceMessage({ + type: "surface", + conversationId, + spec: { + id: "message-queue", + region: "inline", + title: "Message queue", + fields: [{ kind: "custom", rendererId: "message-queue", payload }], + }, + }); + startTurn(firstId); + await screen.findByRole("button", { name: "Stop generation" }); + ws.feedSurfaceMessage({ + type: "catalog", + catalog: [ + { id: "message-queue", region: "inline", title: "Message queue", scope: "conversation" }, + ], + }); + const pending = { messages: [{ id: "m1", text: "next", queuedAt: 1 }] }; + pushQueue(firstId, pending); + const interrupt = await screen.findByRole("button", { name: "Interrupt & send queued" }); + ws.sent.length = 0; + fetchImpl.mockClear(); + await user.click(interrupt); + expect(fetchImpl.mock.calls.filter(([, init]) => init?.method === "POST")).toEqual([ + [expect.stringContaining(`/conversations/${firstId}/stop`), { method: "POST" }], + ]); + expect(sentMessages(ws)).toEqual([]); // No new send, queue, or cancel operation. + + store.newDraft(); + await user.type(screen.getByRole("textbox", { name: "Message input" }), "second"); + await user.click(screen.getByRole("button", { name: "Send" })); + const secondId = activeConversationId(store); + expect(secondId).not.toBe(firstId); + startTurn(secondId); + await screen.findByRole("button", { name: "Stop generation" }); + pushQueue(firstId, pending); // Late old-scope update must not relabel the new conversation. + await vi.waitFor(() => + expect(screen.getByRole("button", { name: "Stop generation" })).toBeInTheDocument(), + ); + pushQueue(secondId, pending); + await screen.findByRole("button", { name: "Interrupt & send queued" }); + pushQueue(secondId, { messages: [] }); + await screen.findByRole("button", { name: "Stop generation" }); + pushQueue(secondId, { messages: "malformed" }); + await vi.waitFor(() => + expect(screen.getByRole("button", { name: "Stop generation" })).toBeInTheDocument(), + ); + store.dispose(); + }); + it("incoming chat.delta renders text in the chat transcript", async () => { const ws = fakeSocket(); const store = createAppStore({ diff --git a/src/app/store-types.ts b/src/app/store-types.ts index 465b21d..774d041 100644 --- a/src/app/store-types.ts +++ b/src/app/store-types.ts @@ -153,8 +153,11 @@ export interface AppStore { /** The localStorage instance the store uses for persistence (tabs, chatLimit). * Exposed so the shell can persist sidebar layout via the same adapter. */ readonly storage: Storage | undefined; - /** The current spec for one surface by id (discovery-by-id), or null if absent. */ - surface(surfaceId: string): SurfaceSpec | null; + /** + * The current spec by id, or null if absent. With conversationId, require a + * snapshot received for that conversation (not a retained prior-scope spec). + */ + surface(surfaceId: string, conversationId?: string): SurfaceSpec | null; /** * Send a user message (start a turn). Forwards any staged `images` * (`ImageInput[]` — base64 data URLs / https URLs) on the `chat.send` op; diff --git a/src/app/store.svelte.ts b/src/app/store.svelte.ts index 5dcdc7d..fa12036 100644 --- a/src/app/store.svelte.ts +++ b/src/app/store.svelte.ts @@ -511,8 +511,8 @@ export function createAppStore(opts?: CreateAppStoreOptions): AppStore { return workspaceConversationId(); }, - surface(surfaceId: string): SurfaceSpec | null { - return getSurfaceSpec(surfaces.protocol, surfaceId); + surface(surfaceId: string, conversationId?: string): SurfaceSpec | null { + return surfaces.surfaceSpecOp(surfaceId, conversationId); }, send(text: string, images?: readonly ImageInput[]): void { diff --git a/src/app/surface-protocol.svelte.ts b/src/app/surface-protocol.svelte.ts index 4ba684d..400f612 100644 --- a/src/app/surface-protocol.svelte.ts +++ b/src/app/surface-protocol.svelte.ts @@ -16,8 +16,18 @@ export function createSurfaceProtocol(deps: { send: SurfaceSocketHandle["send"]; }) { let protocol = $state(protocolInitialState()); + // Rendering may retain the prior spec during re-scope. Consumers making + // conversation-specific decisions can opt into a snapshot with a known scope. + let receivedScopes = $state>(new Map()); function handleServerMessage(msg: SurfaceServerMessage): void { - protocol = applyServerMessage(protocol, msg); + const next = applyServerMessage(protocol, msg); + if (next !== protocol && (msg.type === "surface" || msg.type === "update")) { + const snapshot = msg.type === "surface" ? msg : msg.update; + const scopes = new Map(receivedScopes); + scopes.set(snapshot.spec.id, snapshot.conversationId); + receivedScopes = scopes; + } + protocol = next; // Surfaces are auto-expanded: whenever the catalog changes, subscribe to // every entry (and drop subscriptions for entries that vanished). if (msg.type === "catalog") { @@ -55,6 +65,7 @@ export function createSurfaceProtocol(deps: { } } } + receivedScopes = new Map([...receivedScopes].filter(([id]) => protocol.subscriptions.has(id))); } /** @@ -99,12 +110,17 @@ export function createSurfaceProtocol(deps: { void conversationId; const result = protocolUnsubscribe(protocol, surfaceId); protocol = result.state; + const scopes = new Map(receivedScopes); + scopes.delete(surfaceId); + receivedScopes = scopes; for (const msg of result.outgoing) { deps.send(msg); } } - function surfaceSpecOp(surfaceId: string): SurfaceSpec | null { + function surfaceSpecOp(surfaceId: string, conversationId?: string): SurfaceSpec | null { + if (conversationId !== undefined && receivedScopes.get(surfaceId) !== conversationId) + return null; return getSurfaceSpec(protocol, surfaceId); } diff --git a/src/app/surface-protocol.test.ts b/src/app/surface-protocol.test.ts new file mode 100644 index 0000000..5ac4f70 --- /dev/null +++ b/src/app/surface-protocol.test.ts @@ -0,0 +1,65 @@ +import type { SurfaceSpec } from "@dispatch/ui-contract"; +import { describe, expect, it, vi } from "vitest"; +import { createSurfaceProtocol } from "./surface-protocol.svelte"; + +const spec: SurfaceSpec = { id: "s1", region: "inline", title: "Snapshot", fields: [] }; + +function fixture() { + let focusedId = "c1"; + const surfaces = createSurfaceProtocol({ + focusedConversationId: () => focusedId, + send: vi.fn(), + }); + surfaces.handleServerMessage({ + type: "catalog", + catalog: [{ id: "s1", region: "inline", title: "Snapshot", scope: "conversation" }], + }); + return { + surfaces, + focus(id: string) { + focusedId = id; + surfaces.syncSubscriptions(); + }, + }; +} + +describe("conversation-scoped surface lookup", () => { + it("keeps retained rendering but withholds another conversation's snapshot from decisions", () => { + const { surfaces, focus } = fixture(); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + surfaces.handleServerMessage({ type: "surface", conversationId: "c1", spec }); + expect(surfaces.surfaceSpecOp("s1", "c1")).toEqual(spec); + focus("c2"); + expect(surfaces.surfaceSpecOp("s1")).toEqual(spec); + expect(surfaces.surfaceSpecOp("s1", "c2")).toBeNull(); + surfaces.handleServerMessage({ type: "surface", conversationId: "c1", spec }); + expect(surfaces.surfaceSpecOp("s1", "c2")).toBeNull(); + const updated = { ...spec, title: "Current" }; + surfaces.handleServerMessage({ + type: "update", + update: { surfaceId: "s1", conversationId: "c2", spec: updated }, + }); + expect(surfaces.surfaceSpecOp("s1", "c2")).toEqual(updated); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + }); + + it("does not treat an unscoped snapshot as known conversation-specific data", () => { + const { surfaces } = fixture(); + surfaces.handleServerMessage({ type: "surface", spec }); + expect(surfaces.surfaceSpecOp("s1")).toEqual(spec); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + }); + + it("drops snapshot scope on explicit unsubscribe and catalog removal", () => { + const { surfaces } = fixture(); + surfaces.handleServerMessage({ type: "surface", conversationId: "c1", spec }); + surfaces.unsubscribeSurfaceOp("s1"); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + surfaces.subscribeSurfaceOp("s1"); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + surfaces.handleServerMessage({ type: "surface", conversationId: "c1", spec }); + expect(surfaces.surfaceSpecOp("s1", "c1")).toEqual(spec); + surfaces.handleServerMessage({ type: "catalog", catalog: [] }); + expect(surfaces.surfaceSpecOp("s1", "c1")).toBeNull(); + }); +}); diff --git a/src/app/views/ComposerPane.svelte b/src/app/views/ComposerPane.svelte index ac95598..00dbc9f 100644 --- a/src/app/views/ComposerPane.svelte +++ b/src/app/views/ComposerPane.svelte @@ -6,11 +6,18 @@ * is a persistent column fixture, exactly like today. */ import { Composer, type ComposerStatus } from "../../features/chat"; + import { MESSAGE_QUEUE_RENDERER_ID, messageQueueCount } from "../../features/surface-host"; import type { AppViewPorts } from "../view-ports"; let { ports }: { ports: AppViewPorts } = $props(); const store = $derived(ports.store); + const queuedMessageCount = $derived( + store.activeConversationId === null + ? undefined + : messageQueueCount(store.surface(MESSAGE_QUEUE_RENDERER_ID, store.activeConversationId)), + ); + // Priority: error > queued > running > idle. `queued` (the turn is in flight // but waiting for a concurrency slot — CR-13) wins over `running` so the // corner shows a ring, not dots, during the wait. `turn-start` fires before @@ -31,5 +38,6 @@ onStop={() => store.stopGeneration()} contextSize={store.activeChat.currentContextSize} contextWindow={store.modelInfo[store.activeModel]?.contextWindow} + {queuedMessageCount} status={composerStatus} /> diff --git a/src/features/chat/logic/composer.test.ts b/src/features/chat/logic/composer.test.ts new file mode 100644 index 0000000..6ef5eab --- /dev/null +++ b/src/features/chat/logic/composer.test.ts @@ -0,0 +1,20 @@ +import { describe, expect, it } from "vitest"; +import { stopButtonLabels } from "./composer"; + +describe("stopButtonLabels", () => { + it.each([undefined, 0])("preserves plain Stop with queued count %s", (count) => { + expect(stopButtonLabels(count)).toEqual({ + text: "Stop", + ariaLabel: "Stop generation", + title: undefined, + }); + }); + + it.each([1, 2])("describes interrupt-and-send with queued count %s", (count) => { + expect(stopButtonLabels(count)).toEqual({ + text: "Interrupt & send queued", + ariaLabel: "Interrupt & send queued", + title: "Stop the current response and start the queued message now", + }); + }); +}); diff --git a/src/features/chat/logic/composer.ts b/src/features/chat/logic/composer.ts new file mode 100644 index 0000000..1ea8173 --- /dev/null +++ b/src/features/chat/logic/composer.ts @@ -0,0 +1,11 @@ +/** Stop still uses the same action; the backend carries queued input into a new turn. */ +export function stopButtonLabels(queuedMessageCount: number | undefined) { + if (queuedMessageCount !== undefined && queuedMessageCount >= 1) { + return { + text: "Interrupt & send queued", + ariaLabel: "Interrupt & send queued", + title: "Stop the current response and start the queued message now", + }; + } + return { text: "Stop", ariaLabel: "Stop generation", title: undefined }; +} diff --git a/src/features/chat/ui.test.ts b/src/features/chat/ui.test.ts index 2367e64..480d593 100644 --- a/src/features/chat/ui.test.ts +++ b/src/features/chat/ui.test.ts @@ -849,6 +849,57 @@ describe("ChatView", () => { }); describe("Composer", () => { + it.each([ + "running", + "queued", + ] as const)("relabels Stop only with queued input while %s and preserves its action", async (status) => { + const onStop = vi.fn(); + const onSend = vi.fn(); + const onQueue = vi.fn(); + const user = userEvent.setup(); + const { rerender } = render(Composer, { props: { onSend, onQueue, onStop, status } }); + const expectPlainStop = () => { + const button = screen.getByRole("button", { name: "Stop generation" }); + expect(button).toHaveTextContent(/^Stop$/); + expect(button).not.toHaveAttribute("title"); + }; + expectPlainStop(); // No queue snapshot available. + await rerender({ queuedMessageCount: 0 }); + expectPlainStop(); + await user.click(screen.getByRole("button", { name: "Stop generation" })); + expect(onStop).toHaveBeenCalledTimes(1); + + for (const queuedMessageCount of [1, 2]) { + await rerender({ queuedMessageCount }); + const button = screen.getByRole("button", { name: "Interrupt & send queued" }); + expect(button).toHaveTextContent(/^Interrupt & send queued$/); + expect(button).toHaveAttribute( + "title", + "Stop the current response and start the queued message now", + ); + } + await user.click(screen.getByRole("button", { name: "Interrupt & send queued" })); + expect(onStop).toHaveBeenCalledTimes(2); + expect(onSend).not.toHaveBeenCalled(); + expect(onQueue).not.toHaveBeenCalled(); + + const input = screen.getByRole("textbox", { name: "Message input" }); + await user.type(input, "next"); + expect(screen.getByRole("button", { name: "Queue" })).toBeInTheDocument(); + expect(screen.queryByRole("button", { name: "Interrupt & send queued" })).toBeNull(); + await user.click(screen.getByRole("button", { name: "Queue" })); + expect(onQueue).toHaveBeenCalledExactlyOnceWith("next", undefined); + expect(screen.getByRole("button", { name: "Interrupt & send queued" })).toBeInTheDocument(); + await rerender({ queuedMessageCount: 0 }); + expectPlainStop(); // Queue drained or cancelled. + await rerender({ queuedMessageCount: 1, status: "idle" }); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + await rerender({ status: "error" }); + expect(screen.getByRole("button", { name: "Send" })).toBeDisabled(); + await rerender({ status, queuedMessageCount: undefined }); + expectPlainStop(); + }); + it("calls onSend with the typed text and clears", async () => { const onSend = vi.fn(); const user = userEvent.setup(); diff --git a/src/features/chat/ui/Composer.svelte b/src/features/chat/ui/Composer.svelte index 3cc115b..a43ed53 100644 --- a/src/features/chat/ui/Composer.svelte +++ b/src/features/chat/ui/Composer.svelte @@ -1,6 +1,7 @@