From 8f8298de50fb27f1f2728555c9c58a167683847e Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Thu, 21 May 2026 23:52:20 +0800 Subject: [PATCH 1/5] fix(app): handle question response races clearly --- .../composer/session-question-dock.test.ts | 70 +++++++- .../composer/session-question-dock.tsx | 163 ++++++++++++++---- 2 files changed, 191 insertions(+), 42 deletions(-) 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..90027d963 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,97 @@ +import { readFileSync } from "node:fs" import { describe, expect, test } from "bun:test" -import { resolveSkipAction } from "./session-question-dock" +import { 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("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("keeps mouse and keyboard submit paths behind the pending send guard", () => { + const source = readFileSync(new URL("./session-question-dock.tsx", import.meta.url), "utf8") + + expect(source).toContain("const reply = async (answers: QuestionAnswer[]) => {\n if (sending()) return") + expect(source).toContain('if (mod && event.key === "Enter")') + expect(source).toContain("if (sending()) return\n if (store.editing) commitCustom()") + }) +}) 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..569125661 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,100 @@ 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 } + +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) + + if (code === "already_resolved") return { type: "already_resolved", requestID: requestIDFromToolRespondError(err) } + if (status === 404) return { type: "stale_session" } + if (status === 409) return { type: "already_resolved", requestID: requestIDFromToolRespondError(err) } + if (status === 400 || status === 422) { + const detail = [code, detailsFromToolRespondError(err)].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 +207,7 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu let customRef: HTMLButtonElement | undefined let optsRef: HTMLButtonElement[] = [] let replied = false + let locallySubmitted: QuestionRequestFingerprint | undefined let focusFrame: number | undefined const question = createMemo(() => questions()[store.tab]) @@ -207,45 +291,60 @@ 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 = () => { + replied = true + cache.delete(props.request.id) + props.onSubmit() + } + + 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 + 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 + 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 + 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.invalidPayload"), + }) + 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 +355,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 +373,8 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu }, }) }, - onMutate: () => { - props.onSubmit() - }, onSuccess: () => { - replied = true - cache.delete(props.request.id) + complete() }, onError: fail, })) @@ -558,7 +650,6 @@ export const SessionQuestionDock: Component<{ request: DockQuestionRequest; onSu
0}> -