diff --git a/packages/app/e2e/session/session-composer-dock.spec.ts b/packages/app/e2e/session/session-composer-dock.spec.ts index 80159a22d..b8c63da68 100644 --- a/packages/app/e2e/session/session-composer-dock.spec.ts +++ b/packages/app/e2e/session/session-composer-dock.spec.ts @@ -738,6 +738,79 @@ test("blocked question flow supports escape dismiss", async ({ page, llm, projec ) }) +test("blocked question dock disables controls while response is pending", async ({ page, llm, project }) => { + await project.open() + await withDockSession( + project.sdk, + "e2e composer dock question pending disabled", + async (session) => { + await withDockSeed(project.sdk, session.id, async () => { + await project.gotoSession(session.id) + + let responseCalls = 0 + let releaseResponse: (() => void) | undefined + const responseReleased = new Promise((resolve) => { + releaseResponse = resolve + }) + const respondRoute = async (route: any) => { + responseCalls += 1 + await responseReleased + await route + .fulfill({ status: 200, contentType: "application/json", body: JSON.stringify(true) }) + .catch(() => undefined) + } + + try { + const questions = defaultQuestions + await llm.toolMatch(inputMatch({ questions }), "question", { questions }) + await seedSessionQuestion(project.sdk, { + sessionID: session.id, + questions, + }) + + const dock = page.locator(questionDockSelector) + await expectQuestionBlocked(page) + + await page.route("**/session/*/tool/respond", respondRoute) + + const customOption = dock.locator('[data-slot="question-option"][data-custom="true"]') + await customOption.click() + + const customInput = dock.locator('[data-slot="question-custom-input"]') + const firstOption = dock.locator('[data-slot="question-option"]').first() + const submit = dock.getByRole("button", { name: /submit/i }) + await expect(customInput).toBeVisible() + + await dock.evaluate((el) => { + el.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true })) + }) + await expect.poll(() => responseCalls, { timeout: 10_000 }).toBe(1) + + await expect(customInput).toBeDisabled() + await expect(firstOption).toBeDisabled() + await expect(submit).toBeDisabled() + + const duplicateResponse = page + .waitForRequest((request) => request.url().includes("/tool/respond"), { timeout: 1_000 }) + .then(() => true) + .catch(() => false) + await dock.evaluate((el) => { + el.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true })) + }) + await submit.evaluate((button: HTMLButtonElement) => button.click()) + await firstOption.evaluate((button: HTMLButtonElement) => button.click()) + expect(await duplicateResponse).toBe(false) + expect(responseCalls).toBe(1) + } finally { + releaseResponse?.() + await page.unroute("**/session/*/tool/respond", respondRoute) + } + }) + }, + { trackSession: project.trackSession }, + ) +}) + test("blocked permission flow supports allow once", async ({ page, project }) => { await project.open() await withDockSession( diff --git a/packages/app/src/i18n/en.ts b/packages/app/src/i18n/en.ts index eca26d7d2..57a2ef66d 100644 --- a/packages/app/src/i18n/en.ts +++ b/packages/app/src/i18n/en.ts @@ -645,6 +645,7 @@ export const dict = { "session.question.error.staleSession": "Session restarted; resend your last message to try again.", "session.question.error.alreadyAnswered": "This question was already answered elsewhere.", "session.question.error.invalidPayload": "Your answer did not match the question; please try again.", + "session.question.error.unknown": "Could not submit your answer. Please try again.", "session.followupDock.summary.one": "{{count}} queued message", "session.followupDock.summary.other": "{{count}} queued messages", "session.followupDock.sendNow": "Send now", diff --git a/packages/app/src/i18n/zh.ts b/packages/app/src/i18n/zh.ts index 3ab60cdd0..9311afa21 100644 --- a/packages/app/src/i18n/zh.ts +++ b/packages/app/src/i18n/zh.ts @@ -1049,6 +1049,7 @@ export const dict = { "session.question.error.staleSession": "会话已重启，请重新发送上一条消息", "session.question.error.alreadyAnswered": "已在另一处回答这个问题", "session.question.error.invalidPayload": "回答不符合问题要求，请重试", + "session.question.error.unknown": "未能提交回答，请重试", "session.header.open.finder": "访达", "session.header.open.fileExplorer": "文件资源管理器", "session.header.open.fileManager": "文件管理器", diff --git a/packages/app/src/pages/session/composer/session-question-dock.test.ts b/packages/app/src/pages/session/composer/session-question-dock.test.ts index 5821bee68..3c20d3227 100644 --- a/packages/app/src/pages/session/composer/session-question-dock.test.ts +++ b/packages/app/src/pages/session/composer/session-question-dock.test.ts @@ -1,39 +1,165 @@ import { describe, expect, test } from "bun:test" -import { resolveSkipAction } from "./session-question-dock" +import { runBrowserCheck } from "@/testing/browser-subprocess" +import { + createQuestionResponseGuard, + isSameQuestionRequest, + normalizeToolRespondError, + resolveSkipAction, +} from "./session-question-dock" describe("resolveSkipAction", () => { test("navigates to next unsettled question when one exists after current", () => { - // 3 questions: Q0 settled, Q1 unsettled, Q2 (current) just skipped → now settled const isSettled = (i: number) => i !== 1 const result = resolveSkipAction(2, isSettled, 3) expect(result).toEqual({ type: "navigate", tab: 1 }) }) test("navigates to first unsettled overall when nothing after current", () => { - // 3 questions: Q0 unsettled, Q1 settled, Q2 (current) just skipped → settled const isSettled = (i: number) => i !== 0 const result = resolveSkipAction(2, isSettled, 3) expect(result).toEqual({ type: "navigate", tab: 0 }) }) test("submits when there is only one question and it was just skipped", () => { - // Single question: Q0 just skipped → settled const isSettled = () => true const result = resolveSkipAction(0, isSettled, 1) expect(result).toEqual({ type: "submit" }) }) test("submits when all questions are settled after skipping the last one", () => { - // 3 questions: all settled (Q0 and Q1 answered, Q2 just skipped) const isSettled = () => true const result = resolveSkipAction(2, isSettled, 3) expect(result).toEqual({ type: "submit" }) }) test("navigates to next unsettled before current when current is not the last", () => { - // 3 questions: Q0 settled, Q1 (current) just skipped → settled, Q2 unsettled const isSettled = (i: number) => i !== 2 const result = resolveSkipAction(1, isSettled, 3) expect(result).toEqual({ type: "navigate", tab: 2 }) }) }) + +describe("normalizeToolRespondError", () => { + test("normalizes plain already_resolved objects without exposing [object Object]", () => { + const result = normalizeToolRespondError({ error: "already_resolved" }) + + expect(result).toEqual({ type: "already_resolved", requestID: undefined }) + expect(JSON.stringify(result)).not.toContain("[object Object]") + }) + + test("keeps answer_count_mismatch details readable for 422 responses", () => { + const result = normalizeToolRespondError({ + response: { status: 422 }, + error: "answer_count_mismatch", + details: { expected: 2, received: 1 }, + }) + + expect(result).toEqual({ + type: "invalid_payload", + detail: 'answer_count_mismatch {"expected":2,"received":1}', + }) + expect(JSON.stringify(result)).not.toContain("[object Object]") + }) + + test("maps plain no_pending_tool_call body errors to stale session", () => { + expect(normalizeToolRespondError({ error: "no_pending_tool_call" })).toEqual({ type: "stale_session" }) + }) + + test("keeps plain answer_count_mismatch body details readable without a status", () => { + const result = normalizeToolRespondError({ + error: "answer_count_mismatch", + details: { expected: 2, received: 1 }, + }) + + expect(result).toEqual({ + type: "invalid_payload", + detail: 'answer_count_mismatch {"expected":2,"received":1}', + }) + expect(JSON.stringify(result)).not.toContain("[object Object]") + }) + + test("supports common error shapes without stringifying unknown objects", () => { + expect(normalizeToolRespondError(new Error("network failed"))).toEqual({ + type: "unknown", + detail: "network failed", + }) + expect(normalizeToolRespondError("offline")).toEqual({ type: "unknown", detail: "offline" }) + expect(normalizeToolRespondError({ status: 404 })).toEqual({ type: "stale_session" }) + expect(normalizeToolRespondError({ statusCode: 409, request: { id: "req_1" } })).toEqual({ + type: "already_resolved", + requestID: "req_1", + }) + expect(normalizeToolRespondError({ nested: true })).toEqual({ type: "unknown" }) + }) +}) + +describe("question response local completion guard", () => { + const request = { id: "req_1", sessionID: "ses_1", messageID: "msg_1", callID: "call_1" } + + test("does not treat already_resolved as completion without a same-request local submit", () => { + expect(isSameQuestionRequest(undefined, request, "req_1")).toBe(false) + expect(isSameQuestionRequest({ ...request, id: "req_other" }, request, "req_1")).toBe(false) + expect(isSameQuestionRequest({ ...request, callID: "call_other" }, request, "req_1")).toBe(false) + }) + + test("treats already_resolved as idempotent only for the same local request", () => { + expect(isSameQuestionRequest(request, request, "req_1")).toBe(true) + expect(isSameQuestionRequest(request, request)).toBe(true) + }) +}) + +describe("question response duplicate submission guard", () => { + test("blocks repeated response attempts while pending and while waiting for sync close", () => { + const guard = createQuestionResponseGuard("req_1") + + expect(guard.begin("req_1")).toBe(true) + expect(guard.begin("req_1")).toBe(false) + + guard.confirm("req_1") + + expect(guard.canInteract("req_1")).toBe(false) + expect(guard.begin("req_1")).toBe(false) + }) + + test("restores interaction after failed submit or a new request", () => { + const guard = createQuestionResponseGuard("req_1") + + expect(guard.begin("req_1")).toBe(true) + guard.fail("req_1") + expect(guard.begin("req_1")).toBe(true) + + guard.confirm("req_1") + + expect(guard.canInteract("req_1")).toBe(false) + expect(guard.canInteract("req_2")).toBe(true) + expect(guard.begin("req_2")).toBe(true) + }) + + test("updates reactive disabled state immediately after begin and confirm", () => { + runBrowserCheck(String.raw` + import { createMemo, createRoot } from "solid-js" + import { createQuestionResponseGuard } from "./src/pages/session/composer/session-question-dock.tsx" + + const assert = (condition, message) => { + if (!condition) throw new Error(message) + } + + createRoot((dispose) => { + const guard = createQuestionResponseGuard("req_1") + const disabled = createMemo(() => !guard.canInteract("req_1")) + + assert(disabled() === false, "initial request should be interactive") + assert(guard.begin("req_1") === true, "first begin should submit") + assert(disabled() === true, "begin should immediately disable reactive UI") + + guard.fail("req_1") + assert(disabled() === false, "failure should immediately restore interaction") + assert(guard.begin("req_1") === true, "retry should submit after failure") + guard.confirm("req_1") + assert(disabled() === true, "confirmed response should stay disabled while waiting for sync close") + + dispose() + }) + `) + }) +}) diff --git a/packages/app/src/pages/session/composer/session-question-dock.tsx b/packages/app/src/pages/session/composer/session-question-dock.tsx index 76b7afe9d..72216b58e 100644 --- a/packages/app/src/pages/session/composer/session-question-dock.tsx +++ b/packages/app/src/pages/session/composer/session-question-dock.tsx @@ -5,17 +5,139 @@ import { Button } from "@opencode-ai/ui/button" import { DockPrompt } from "@opencode-ai/ui/dock-prompt" import { Icon } from "@opencode-ai/ui/icon" import { showToast } from "@opencode-ai/ui/toast" +import { useLanguage } from "@/context/language" +import { useSDK } from "@/context/sdk" import type { DockQuestionRequest } from "@/pages/session/blockers/use-session-blockers" // One question's selected labels. Mirrors the per-row shape of the // `payload.answers: string[][]` body sent to POST /session/:id/tool/respond // (validated by questionDecoder in packages/opencode/src/tool/question.ts). type QuestionAnswer = readonly string[] -import { useLanguage } from "@/context/language" -import { useSDK } from "@/context/sdk" type DraftAnswer = QuestionAnswer | undefined +type QuestionRequestFingerprint = Pick + +type NormalizedToolRespondError = + | { type: "already_resolved"; requestID?: string } + | { type: "stale_session" } + | { type: "invalid_payload"; detail?: string } + | { type: "unknown"; detail?: string } + +type QuestionResponsePhase = "idle" | "submitting" | "closing" + +const invalidPayloadErrorCodes = new Set(["answer_count_mismatch"]) + +export function createQuestionResponseGuard(initialRequestID: string) { + const [state, setState] = createStore<{ requestID: string; phase: QuestionResponsePhase }>({ + requestID: initialRequestID, + phase: "idle", + }) + + const sync = (nextRequestID: string) => { + if (nextRequestID === state.requestID) return + setState({ requestID: nextRequestID, phase: "idle" }) + } + + return { + canInteract(nextRequestID: string) { + sync(nextRequestID) + return state.phase === "idle" + }, + begin(nextRequestID: string) { + sync(nextRequestID) + if (state.phase !== "idle") return false + setState("phase", "submitting") + return true + }, + confirm(nextRequestID: string) { + sync(nextRequestID) + if (state.phase === "submitting") setState("phase", "closing") + }, + fail(nextRequestID: string) { + sync(nextRequestID) + setState("phase", "idle") + }, + } +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null +} + +function stringField(value: unknown): string | undefined { + return typeof value === "string" && value.trim() ? value : undefined +} + +function statusFromToolRespondError(err: unknown): number | undefined { + if (!isRecord(err)) return undefined + const response = err.response + if (isRecord(response) && typeof response.status === "number") return response.status + if (typeof err.status === "number") return err.status + if (typeof err.statusCode === "number") return err.statusCode + return undefined +} + +function errorCodeFromToolRespondError(err: unknown): string | undefined { + if (!isRecord(err)) return undefined + const bodyError = err.error + if (typeof bodyError === "string") return bodyError + if (isRecord(bodyError)) return stringField(bodyError.error) + return undefined +} + +function detailsFromToolRespondError(err: unknown): string | undefined { + if (!isRecord(err)) return undefined + const details = err.details + if (typeof details === "string") return details + if (isRecord(details)) { + try { + return JSON.stringify(details) + } catch { + return undefined + } + } + return undefined +} + +function requestIDFromToolRespondError(err: unknown): string | undefined { + if (!isRecord(err)) return undefined + const request = err.request + if (isRecord(request)) return stringField(request.id) + return undefined +} + +export function normalizeToolRespondError(err: unknown): NormalizedToolRespondError { + const status = statusFromToolRespondError(err) + const code = errorCodeFromToolRespondError(err) + const details = detailsFromToolRespondError(err) + + if (code === "already_resolved") return { type: "already_resolved", requestID: requestIDFromToolRespondError(err) } + if (code === "no_pending_tool_call") return { type: "stale_session" } + if (status === 404) return { type: "stale_session" } + if (status === 409) return { type: "already_resolved", requestID: requestIDFromToolRespondError(err) } + if (status === 400 || status === 422 || invalidPayloadErrorCodes.has(code ?? "") || details !== undefined) { + const detail = [code, details].filter(Boolean).join(" ") + return { type: "invalid_payload", detail: detail || undefined } + } + if (err instanceof Error) return { type: "unknown", detail: err.message } + if (typeof err === "string") return { type: "unknown", detail: err } + if (code) return { type: "unknown", detail: code } + return { type: "unknown" } +} + +export function isSameQuestionRequest( + left: QuestionRequestFingerprint | undefined, + right: QuestionRequestFingerprint, + errorRequestID?: string, +) { + if (!left) return false + if (left.sessionID !== right.sessionID || left.messageID !== right.messageID || left.callID !== right.callID) + return false + if (errorRequestID !== undefined && errorRequestID !== right.id) return false + return left.id === right.id +} + const cache = new Map() function keepVisibleInQuestionOptions(el: HTMLElement) { @@ -124,6 +246,8 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu let customRef: HTMLButtonElement | undefined let optsRef: HTMLButtonElement[] = [] let replied = false + let locallySubmitted: QuestionRequestFingerprint | undefined + const responseGuard = createQuestionResponseGuard(props.request.id) let focusFrame: number | undefined const question = createMemo(() => questions()[store.tab]) @@ -207,45 +331,67 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu }) }) - const fail = (err: unknown) => { - // The route handler returns typed status codes. Surface a dedicated copy - // so the user understands whether to retry, reload, or accept that - // another client answered. - const status = (err as { response?: { status?: number } } | undefined)?.response?.status - if (status === 404) { + const currentRequest = (): QuestionRequestFingerprint => ({ + id: props.request.id, + sessionID: props.request.sessionID, + messageID: props.request.messageID, + callID: props.request.callID, + }) + + const complete = () => { + responseGuard.confirm(props.request.id) + replied = true + cache.delete(props.request.id) + props.onSubmit() + } + + const canInteract = () => responseGuard.canInteract(props.request.id) + + const fail = (err: unknown): "completed" | "failed" => { + const normalized = normalizeToolRespondError(err) + if (normalized.type === "already_resolved") { + if (isSameQuestionRequest(locallySubmitted, currentRequest(), normalized.requestID)) { + complete() + return "completed" + } showToast({ title: language.t("common.requestFailed"), - description: language.t("session.question.error.staleSession"), + description: language.t("session.question.error.alreadyAnswered"), }) - return + responseGuard.fail(props.request.id) + locallySubmitted = undefined + return "failed" } - if (status === 409) { + if (normalized.type === "stale_session") { showToast({ title: language.t("common.requestFailed"), - description: language.t("session.question.error.alreadyAnswered"), + description: language.t("session.question.error.staleSession"), }) - return + responseGuard.fail(props.request.id) + locallySubmitted = undefined + return "failed" } - if (status === 422 || status === 400) { - const body = (err as { error?: unknown } | undefined)?.error - const detail = - typeof body === "object" && body !== null && "error" in body - ? String((body as { error?: unknown }).error ?? "") - : err instanceof Error - ? err.message - : String(err) + if (normalized.type === "invalid_payload") { showToast({ title: language.t("common.requestFailed"), - description: detail || language.t("session.question.error.invalidPayload"), + description: normalized.detail || language.t("session.question.error.invalidPayload"), }) - return + responseGuard.fail(props.request.id) + locallySubmitted = undefined + return "failed" } - const message = err instanceof Error ? err.message : String(err) - showToast({ title: language.t("common.requestFailed"), description: message }) + showToast({ + title: language.t("common.requestFailed"), + description: normalized.detail || language.t("session.question.error.unknown"), + }) + responseGuard.fail(props.request.id) + locallySubmitted = undefined + return "failed" } const replyMutation = useMutation(() => ({ mutationFn: async (answers: QuestionAnswer[]): Promise => { + locallySubmitted = currentRequest() await sdk.client.session.toolRespond({ sessionID: props.request.sessionID, body: { @@ -256,18 +402,15 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu }, }) }, - onMutate: () => { - props.onSubmit() - }, onSuccess: () => { - replied = true - cache.delete(props.request.id) + complete() }, onError: fail, })) const rejectMutation = useMutation(() => ({ mutationFn: async (): Promise => { + locallySubmitted = currentRequest() await sdk.client.session.toolRespond({ sessionID: props.request.sessionID, body: { @@ -277,12 +420,8 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu }, }) }, - onMutate: () => { - props.onSubmit() - }, onSuccess: () => { - replied = true - cache.delete(props.request.id) + complete() }, onError: fail, })) @@ -290,12 +429,12 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu const sending = createMemo(() => replyMutation.isPending || rejectMutation.isPending) const reply = async (answers: QuestionAnswer[]) => { - if (sending()) return + if (sending() || !responseGuard.begin(props.request.id)) return await replyMutation.mutateAsync(answers) } const reject = async () => { - if (sending()) return + if (sending() || !responseGuard.begin(props.request.id)) return await rejectMutation.mutateAsync() } @@ -336,7 +475,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const customToggle = () => { - if (sending()) return + if (!canInteract()) return setStore("focus", options().length) if (!multi()) { @@ -366,7 +505,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const customOpen = () => { - if (sending()) return + if (!canInteract()) return setStore("focus", options().length) if (!on()) setStore("customOn", store.tab, true) setStore("editing", true) @@ -374,7 +513,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const move = (step: number) => { - if (store.editing || sending()) return + if (store.editing || !canInteract()) return focus(store.focus + step) } @@ -425,7 +564,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const selectOption = (optIndex: number) => { - if (sending()) return + if (!canInteract()) return if (optIndex === options().length) { if (!customAllowed()) return @@ -468,7 +607,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const next = () => { - if (sending()) return + if (!canInteract()) return if (store.editing) commitCustom() if (store.tab >= total() - 1) { @@ -483,7 +622,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const back = () => { - if (sending()) return + if (!canInteract()) return if (store.tab <= 0) return const tab = store.tab - 1 setStore("tab", tab) @@ -492,7 +631,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const skipCurrent = () => { - if (sending()) return + if (!canInteract()) return setStore("answers", store.tab, []) setStore("custom", store.tab, "") setStore("customOn", store.tab, false) @@ -509,7 +648,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } const jump = (tab: number) => { - if (sending()) return + if (!canInteract()) return setStore("tab", tab) setStore("editing", false) focus(pickFocus(tab)) @@ -536,7 +675,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu data-slot="question-progress-segment" data-active={i() === store.tab} data-answered={settled(i())} - disabled={sending()} + disabled={!canInteract()} onClick={() => jump(i())} aria-label={`${language.t("ui.tool.questions")} ${i() + 1}`} /> @@ -547,19 +686,18 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu } footer={ <> -
0}> -