diff --git a/canvas/e2e/chat-desktop.spec.ts b/canvas/e2e/chat-desktop.spec.ts index 05f02d392..02f4c7404 100644 --- a/canvas/e2e/chat-desktop.spec.ts +++ b/canvas/e2e/chat-desktop.spec.ts @@ -1,7 +1,7 @@ import { test, expect } from "@playwright/test"; import type { Page } from "@playwright/test"; import { startEchoRuntime } from "./fixtures/echo-runtime"; -import { seedWorkspace, startHeartbeat, cleanupWorkspace } from "./fixtures/chat-seed"; +import { seedWorkspace, startHeartbeat, cleanupWorkspace, runPsql } from "./fixtures/chat-seed"; /** Enter the Org-map view so the Canvas (React Flow graph) mounts. */ async function enterMapView(page: Page): Promise { @@ -67,6 +67,17 @@ test.describe("Desktop ChatTab", () => { expect(hasEmptyState || hasHistory).toBeTruthy(); }); + test("echo fixture workspace is configured for push delivery", async () => { + // Regression for #2786: external echo-runtime workspaces must be push-mode + // so the proxy dispatches synchronously to the echo URL. Poll-mode defaults + // short-circuit to {status:'queued'} and the inline echo never renders. + const out = runPsql( + `SELECT delivery_mode FROM workspaces WHERE id = '${workspaceId}';`, + 10_000, + ); + expect(out).toContain("push"); + }); + test("send text message and receive echo response", async ({ page }) => { const chat = page.locator("#panel-chat [data-testid='chat-panel']:visible"); const textarea = page.locator("#panel-chat textarea").first(); diff --git a/canvas/e2e/chat-separation.spec.ts b/canvas/e2e/chat-separation.spec.ts index 1235cf738..65b2b97ee 100644 --- a/canvas/e2e/chat-separation.spec.ts +++ b/canvas/e2e/chat-separation.spec.ts @@ -18,7 +18,7 @@ async function enterMapView(page: Page): Promise { await btn.click(); } -/** Open the seeded workspace's Chat side panel. */ +/** Open the seeded workspace's Chat side panel (scoped to the visible panel). */ async function openChatPanel(page: Page, workspaceName: string): Promise { await page.setViewportSize({ width: 1280, height: 800 }); await page.goto("/"); @@ -38,11 +38,13 @@ async function openChatPanel(page: Page, workspaceName: string): Promise { await page.waitForSelector("#panel-chat [data-testid='chat-panel']:visible", { timeout: 5_000, }); - await expect(page.locator("#panel-chat textarea").first()).toBeEnabled({ + await expect(page.locator("#panel-chat [data-testid='chat-panel']:visible textarea").first()).toBeEnabled({ timeout: 15_000, }); } +const panelLocator = (page: Page) => + page.locator("#panel-chat [data-testid='chat-panel']:visible"); /** Post a message to the workspace via the A2A proxy so activity rows exist. * `source` determines the auth shape, which in turn determines * activity_logs.source_id: @@ -128,42 +130,40 @@ test.describe("Chat Sub-Tabs", () => { }); test("chat tab shows My Chat and Agent Comms sub-tabs", async ({ page }) => { - const panel = page.locator("#panel-chat"); - await expect(panel.getByRole("button", { name: "My Chat" })).toBeVisible(); - await expect(panel.getByRole("button", { name: "Agent Comms" })).toBeVisible(); + const panel = panelLocator(page); + await expect(panel.getByRole("tab", { name: "My Chat" })).toBeVisible(); + await expect(panel.getByRole("tab", { name: "Agent Comms" })).toBeVisible(); }); test("My Chat is selected by default", async ({ page }) => { - const myChatBtn = page - .locator("#panel-chat") - .getByRole("button", { name: "My Chat" }); + const myChatBtn = panelLocator(page).getByRole("tab", { name: "My Chat" }); await expect(myChatBtn).toHaveAttribute("aria-selected", "true"); }); test("switching to Agent Comms shows different content", async ({ page }) => { - const panel = page.locator("#panel-chat"); - await panel.getByRole("button", { name: "Agent Comms" }).click(); + const panel = panelLocator(page); + await panel.getByRole("tab", { name: "Agent Comms" }).click(); // Agent Comms should be selected and My Chat's textarea should not be visible. await expect( - panel.getByRole("button", { name: "Agent Comms" }), + panel.getByRole("tab", { name: "Agent Comms" }), ).toHaveAttribute("aria-selected", "true"); await expect(panel.locator("textarea").first()).not.toBeVisible(); }); test("My Chat has input box, Agent Comms does not", async ({ page }) => { - const panel = page.locator("#panel-chat"); + const panel = panelLocator(page); // My Chat has the textarea. await expect(panel.locator("textarea").first()).toBeVisible(); // Switch to Agent Comms. - await panel.getByRole("button", { name: "Agent Comms" }).click(); + await panel.getByRole("tab", { name: "Agent Comms" }).click(); await expect(panel.locator("textarea").first()).not.toBeVisible(); }); test("switching back to My Chat preserves messages", async ({ page }) => { - const panel = page.locator("#panel-chat"); + const panel = panelLocator(page); // Send a message so there is content to preserve. const textarea = panel.locator("textarea").first(); @@ -174,8 +174,8 @@ test.describe("Chat Sub-Tabs", () => { ).toBeVisible({ timeout: 15_000 }); // Switch to Agent Comms and back. - await panel.getByRole("button", { name: "Agent Comms" }).click(); - await panel.getByRole("button", { name: "My Chat" }).click(); + await panel.getByRole("tab", { name: "Agent Comms" }).click(); + await panel.getByRole("tab", { name: "My Chat" }).click(); // Message should still be there. await expect(panel.getByText("Persistence check", { exact: true })).toBeVisible(); @@ -343,13 +343,13 @@ test.describe("Data Flow — Initial Prompt in Chat", () => { }); test("seeded chat history appears in My Chat", async ({ page }) => { - const panel = page.locator("#panel-chat"); + const panel = panelLocator(page); await expect(panel.getByText('Hello from seed with "quotes"')).toBeVisible({ timeout: 5_000 }); await expect(panel.getByText('Hello back from seed with "quotes"')).toBeVisible({ timeout: 5_000 }); }); test("My Chat empty state is not shown when history exists", async ({ page }) => { - const panel = page.locator("#panel-chat"); + const panel = panelLocator(page); await expect(panel.getByText("No messages yet")).not.toBeVisible(); }); }); @@ -384,9 +384,9 @@ test.describe("No JS Errors", () => { await openChatPanel(page, workspaceName); // Switch between tabs. - const panel = page.locator("#panel-chat"); - await panel.getByRole("button", { name: "Agent Comms" }).click(); - await panel.getByRole("button", { name: "My Chat" }).click(); + const panel = panelLocator(page); + await panel.getByRole("tab", { name: "Agent Comms" }).click(); + await panel.getByRole("tab", { name: "My Chat" }).click(); const critical = errors.filter( (e) => !e.includes("WebSocket") && !e.includes("favicon") && !e.includes("hydration"), diff --git a/canvas/e2e/fixtures/chat-seed.ts b/canvas/e2e/fixtures/chat-seed.ts index 3e966edcf..c8c0f3ffe 100644 --- a/canvas/e2e/fixtures/chat-seed.ts +++ b/canvas/e2e/fixtures/chat-seed.ts @@ -21,7 +21,7 @@ interface PgCredentials { db: string; } -function parseDbUrl(): PgCredentials | null { +export function parseDbUrl(): PgCredentials | null { const dbUrl = process.env.E2E_DATABASE_URL; if (!dbUrl) return null; const pgRegex = /postgres:\/\/([^:]+):([^@]+)@([^:]+):(\d+)\/([^?]+)/; @@ -31,13 +31,13 @@ function parseDbUrl(): PgCredentials | null { return { user, pass, host, port, db }; } -function runPsql(sql: string, timeoutMs = 30_000): void { +export function runPsql(sql: string, timeoutMs = 30_000): string { const creds = parseDbUrl(); if (!creds) { throw new Error("E2E_DATABASE_URL must be set for DB seeding"); } const { user, pass, host, port, db } = creds; - execFileSync( + const out = execFileSync( "psql", ["-h", host, "-p", port, "-U", user, "-d", db, "-c", sql], { @@ -46,6 +46,7 @@ function runPsql(sql: string, timeoutMs = 30_000): void { timeout: timeoutMs, }, ); + return out.toString(); } export interface SeededWorkspace { @@ -75,6 +76,7 @@ export async function seedWorkspace(echoURL: string): Promise { external: true, runtime: "external", url: echoURL, + delivery_mode: "push", }), }); if (!createRes.ok) { diff --git a/canvas/e2e/fixtures/echo-runtime.ts b/canvas/e2e/fixtures/echo-runtime.ts index 69be2eeda..55b5e4b3f 100644 --- a/canvas/e2e/fixtures/echo-runtime.ts +++ b/canvas/e2e/fixtures/echo-runtime.ts @@ -145,16 +145,21 @@ export async function startEchoRuntime(): Promise { ? "Echo: received your file(s)." : "Echo: hello"; - const response = { - jsonrpc: "2.0", - id: rpc.id ?? null, - result: { - parts: [{ kind: "text", text: replyText }], - }, - }; + // Allow the activity-log assertion in chat-desktop.spec.ts to observe + // the thinking state before the instant echo reply clears it. + const delayMs = text === "Trigger activity" ? 800 : 0; + setTimeout(() => { + const response = { + jsonrpc: "2.0", + id: rpc.id ?? null, + result: { + parts: [{ kind: "text", text: replyText }], + }, + }; - res.writeHead(200); - res.end(JSON.stringify(response)); + res.writeHead(200); + res.end(JSON.stringify(response)); + }, delayMs); } catch { res.writeHead(400); res.end(JSON.stringify({ error: "invalid json" })); diff --git a/canvas/src/components/tabs/chat/hooks/__tests__/useChatSend.concurrentReplies.test.tsx b/canvas/src/components/tabs/chat/hooks/__tests__/useChatSend.concurrentReplies.test.tsx index 35c8b3ff4..0033b1f0b 100644 --- a/canvas/src/components/tabs/chat/hooks/__tests__/useChatSend.concurrentReplies.test.tsx +++ b/canvas/src/components/tabs/chat/hooks/__tests__/useChatSend.concurrentReplies.test.tsx @@ -609,3 +609,46 @@ describe("useChatSend — legacy no-messageId fallback is exact-one-token conser expect(result.current.error).toBeNull(); }); }); + +describe("useChatSend — push-mode reply is not dropped by a racing WS completion (core#2786)", () => { + it("renders the push-mode echo reply even when onSendComplete finishes the token first", async () => { + const send = deferred(); + apiPostMock.mockImplementationOnce(() => send.promise); + + const onAgentMessage = vi.fn(); + const { result } = renderHook(() => + useChatSend("push-echo-ws-race", { + getHistoryMessages: () => [], + onAgentMessage, + }), + ); + + await act(async () => { + await result.current.sendMessage("echo me"); + await Promise.resolve(); + }); + expect(result.current.sending).toBe(true); + + const messageId = (apiPostMock.mock.calls[0][1] as { params: { message: { messageId: string } } }).params.message.messageId; + + // A WebSocket completion event (ACTIVITY_LOGGED status=ok with messageId, + // or AGENT_MESSAGE) can arrive before the HTTP 200 on a fast echo/reply. + // It finishes the token — the spinner must drop. + act(() => { + result.current.finishSendByMessageId?.(messageId); + }); + expect(result.current.sending).toBe(false); + + // The late HTTP push-mode reply still carries the actual content and MUST + // be rendered; otherwise the echo bubble never appears (core#2786). + await act(async () => { + send.resolve({ result: { parts: [{ kind: "text", text: "Echo: echo me" }] } }); + await Promise.resolve(); + }); + + expect(onAgentMessage).toHaveBeenCalledTimes(1); + expect((onAgentMessage.mock.calls[0][0] as { content: string }).content).toBe( + "Echo: echo me", + ); + }); +}); diff --git a/canvas/src/components/tabs/chat/hooks/useChatSend.ts b/canvas/src/components/tabs/chat/hooks/useChatSend.ts index e06c29151..d1cc851fd 100644 --- a/canvas/src/components/tabs/chat/hooks/useChatSend.ts +++ b/canvas/src/components/tabs/chat/hooks/useChatSend.ts @@ -366,33 +366,17 @@ export function useChatSend(workspaceId: string, options: UseChatSendOptions) { { timeoutMs: 30 * 60 * 1000 }, ) .then((resp) => { - // core#2725: only process the reply that belongs to this token. - // If the token is neither in-flight nor pending-WS, it has already - // been finished or superseded — drop it to avoid misrouted replies - // / duplicates. - if ( - !inFlightTokensRef.current.has(myToken) && - !pendingWSTokensRef.current.has(myToken) - ) return; - - // Task #227 — poll-mode (external/MCP workspace) queued-200 - // short-circuit. ws-server's `proxyA2ARequest` returns - // `{status:"queued", delivery_mode:"poll", ...}` immediately - // when the target has no URL (delivery_mode=poll), BEFORE the - // agent has produced any reply. There is no `result.parts` - // payload here — the actual reply will arrive separately via - // the AGENT_MESSAGE WebSocket event after the agent's next - // `wait_for_message` poll. - // - // Keep the spinner up by moving the token to the WS-pending set; - // releaseSendGuards will prune it when the AGENT_MESSAGE lands - // (handled by useChatSocket `onAgentMessage`/`onSendComplete`) - // or an explicit error fires (`onSendError` from an - // ACTIVITY_LOGGED status="error"). Don't synthesise an empty - // agent bubble. + // Poll-mode queued short-circuit (external/MCP workspace): the + // server returns `{status:"queued"}` immediately and the real reply + // arrives later via the AGENT_MESSAGE WebSocket event. if (resp?.status === "queued") { - // If WS already completed for this token via the legacy fallback - // path, finish immediately instead of re-pending forever. + if ( + !inFlightTokensRef.current.has(myToken) && + !pendingWSTokensRef.current.has(myToken) + ) { + // Already finished by a WS event (legacy fallback raced ahead). + return; + } if (wsCompletedTokensRef.current.has(myToken)) { finishSendToken(myToken); } else { @@ -401,6 +385,14 @@ export function useChatSend(workspaceId: string, options: UseChatSendOptions) { return; } + // Push-mode synchronous reply: process the agent message even if a + // WebSocket completion event (ACTIVITY_LOGGED or AGENT_MESSAGE) + // already finished this token. Without this, a fast echo/reply that + // triggers a WS completion before the HTTP 200 lands would have its + // token removed here and the reply bubble would never render + // (core#2786 / #2759). Token cleanup is idempotent; ChatTab's + // message dedup handles the rare case where both paths carry the + // same content. const replyText = extractReplyText(resp); const replyFiles = extractFilesFromTask( (resp?.result ?? {}) as Record,