From e0d8a2eb89f6f5744dcfbf3a703a50ed5f3ec85f Mon Sep 17 00:00:00 2001 From: ethernet Date: Tue, 18 Aug 2026 16:12:58 -0400 Subject: [PATCH] feat(desktop): multi-question clarify card with per-question locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clarify card renders every batch question at once. Answers stage locally per question; the footer button locks the staged answer with a clarify.respond keyed by question_id. Locked answers stay editable — a new pick un-locks the row and a re-lock overwrites server-side. When exactly one question is unanswered the button relabels to Confirm and continue, and that final lock completes the batch. Skip cancels the whole batch (no question_id). Reconnect replay seeds the locked map so a reattached window restores its earlier state. The settled card lists every question with its answer; blank answers render as Skipped. Single-question cards are untouched. --- .../hooks/use-message-stream/gateway-event.ts | 61 ++- .../assistant-ui/clarify-tool.test.tsx | 183 ++++++- .../components/assistant-ui/clarify-tool.tsx | 448 +++++++++++++++++- apps/desktop/src/i18n/ar.ts | 5 +- apps/desktop/src/i18n/en.ts | 3 + apps/desktop/src/i18n/ja.ts | 3 + apps/desktop/src/i18n/types.ts | 3 + apps/desktop/src/i18n/zh-hant.ts | 3 + apps/desktop/src/i18n/zh.ts | 3 + apps/desktop/src/lib/chat-messages.ts | 4 + apps/desktop/src/store/clarify.test.ts | 48 ++ apps/desktop/src/store/clarify.ts | 54 +++ 12 files changed, 807 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts index 8ce770e3b2..a4d00bb0a7 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts @@ -25,7 +25,7 @@ import { invalidateSlashCompletions } from '@/lib/slash-completion-cache' import { type AgentNoticePayload, clearAgentNotice, nativeNoticeInput, showAgentNotice } from '@/store/agent-notices' import { reconcileApprovalModeForProfile } from '@/store/approval-mode' import { billingCtaLabel, clearBillingBlock, runBillingRecovery, setBillingBlock } from '@/store/billing-block' -import { clearClarifyRequest, normalizeChoices, setClarifyRequest, warnDroppedChoices } from '@/store/clarify' +import { clearClarifyRequest, normalizeChoices, normalizeQuestions, setClarifyRequest, warnDroppedChoices } from '@/store/clarify' import { setSessionCompacting } from '@/store/compaction' import { refreshBackgroundProcesses } from '@/store/composer-status' import { $gateway, activeGatewayConnectionId } from '@/store/gateway' @@ -1165,8 +1165,65 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) { const rawChoices = payload?.choices const choices = normalizeChoices(rawChoices) const multiSelect = payload?.multi_select === true + // Batch (multi-question) clarify: `questions` replaces question/choices + // on the wire. `answers` rides along only on reconnect replay, carrying + // the per-question locks the server already accepted. + const questions = normalizeQuestions(payload?.questions) + const lockedAnswers = + typeof payload?.answers === 'object' && payload?.answers !== null + ? Object.fromEntries( + Object.entries(payload.answers as Record).filter( + (entry): entry is [string, string] => typeof entry[1] === 'string' + ) + ) + : undefined - if (requestId && question) { + if (requestId && questions.length > 0) { + setClarifyRequest({ + choices: null, + lockedAnswers, + multiSelect: false, + question: '', + questions, + requestId, + sessionId: sessionId ?? null + }) + + if (sessionId) { + // Same hydration-race guard as the single-question path below: the + // form mounts from the tool row, so upsert a stable one keyed by + // the request id in case tool.start was missed. + upsertToolCall( + sessionId, + { + args: { + questions: questions.map(q => ({ + choices: q.choices ?? undefined, + multi_select: q.multiSelect || undefined, + question: q.question + })) + }, + name: 'clarify', + tool_id: requestId + }, + 'running', + event.type, + occurredAt + ) + updateSessionState(sessionId, state => ({ ...state, needsInput: true })) + + if (sessionId === activeSessionIdRef.current) { + requestScrollToBottom() + } + } + + dispatchNativeNotification({ + body: questions.map(q => q.question).join(' · '), + kind: 'input', + sessionId, + title: translateNow('notifications.native.inputTitle') + }) + } else if (requestId && question) { if (rawChoices != null && choices.length === 0) { warnDroppedChoices('gateway', question, rawChoices) } diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx index a0d616633c..e78158a2eb 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx @@ -9,7 +9,7 @@ import { clearClarifyRequest, setClarifyRequest } from '@/store/clarify' import { $gateway } from '@/store/gateway' import { $activeSessionId } from '@/store/session' -import { ClarifyTool, readClarifyResult } from './clarify-tool' +import { ClarifyTool, readClarifyBatchResult, readClarifyResult } from './clarify-tool' // The live pending card only renders while its message is running. Force that so // keyboard-navigation tests can exercise ClarifyToolPending directly. @@ -448,3 +448,184 @@ describe('ClarifyTool pending marker', () => { expect(document.querySelector('[data-clarify-choices]')).toBeNull() }) }) + +// ─── Batch (multi-question) clarify ───────────────────────────────────────── + +function batchArgs(): { questions: { question: string; choices?: string[] }[] } { + return { + questions: [ + { choices: ['red', 'blue'], question: 'Color?' }, + { question: 'Name?' } + ] + } +} + +function liveBatchProps(): ToolCallMessagePartProps { + const args = batchArgs() + + return { + addResult: vi.fn(), + args, + argsText: JSON.stringify(args), + isError: false, + respondToApproval: vi.fn(), + result: undefined, + resume: vi.fn(), + status: { type: 'running' }, + toolCallId: 'clarify-batch', + toolName: 'clarify', + type: 'tool-call' + } +} + +function renderLiveBatch(lockedAnswers?: Record) { + const request = vi.fn().mockResolvedValue({ ok: true, remaining: [] }) + + $activeSessionId.set('session-1') + $gateway.set({ request } as never) + setClarifyRequest({ + choices: null, + lockedAnswers, + multiSelect: false, + question: '', + questions: [ + { choices: ['red', 'blue'], multiSelect: false, qid: 'q0', question: 'Color?' }, + { choices: null, multiSelect: false, qid: 'q1', question: 'Name?' } + ], + requestId: 'request-batch', + sessionId: 'session-1' + }) + renderClarify() + + return request +} + +describe('readClarifyBatchResult', () => { + it('parses responses with string and list answers plus timed_out', () => { + const parsed = readClarifyBatchResult( + JSON.stringify({ + responses: [ + { question: 'Color?', user_response: 'red' }, + { question: 'Tools?', user_response: ['a', 'b'] }, + { question: 'Name?', user_response: '' } + ], + timed_out: true + }) + ) + + expect(parsed.timedOut).toBe(true) + expect(parsed.responses).toHaveLength(3) + expect(parsed.responses[1]?.answer).toEqual(['a', 'b']) + expect(parsed.responses[2]?.answer).toBe('') + }) + + it('returns empty responses for single-question payloads', () => { + expect(readClarifyBatchResult({ question: 'Q?', user_response: 'a' }).responses).toEqual([]) + }) +}) + +describe('ClarifyTool batch card', () => { + it('renders every question at once', () => { + renderLiveBatch() + + expect(screen.getByText('Color?')).toBeTruthy() + expect(screen.getByText('Name?')).toBeTruthy() + expect(screen.getByText('0 of 2 answered')).toBeTruthy() + }) + + it('locks a picked choice via Continue, keyed by qid', async () => { + const request = renderLiveBatch() + + fireEvent.click(screen.getByRole('button', { name: /red/ })) + fireEvent.submit(document.querySelector('form') as HTMLFormElement) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: 'red', + question_id: 'q0', + request_id: 'request-batch' + }) + }) + + await waitFor(() => { + expect(screen.getByText('1 of 2 answered')).toBeTruthy() + }) + }) + + it('answers in any order: free-text question first', async () => { + const request = renderLiveBatch() + + const nameBox = screen.getByPlaceholderText('Type your answer…') + fireEvent.change(nameBox, { target: { value: 'packet' } }) + fireEvent.submit(document.querySelector('form') as HTMLFormElement) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: 'packet', + question_id: 'q1', + request_id: 'request-batch' + }) + }) + }) + + it('relabels the button to Confirm and continue when one question remains', async () => { + renderLiveBatch({ q1: 'already locked' }) + + expect(screen.getByText('1 of 2 answered')).toBeTruthy() + expect(screen.getByRole('button', { name: /Confirm and continue/ })).toBeTruthy() + }) + + it('re-staging a locked answer un-locks it and a re-lock overwrites', async () => { + const request = renderLiveBatch({ q0: 'red' }) + + // q0 arrived locked from replay. Picking blue un-locks it locally… + fireEvent.click(screen.getByRole('button', { name: /blue/ })) + expect(screen.getByText('0 of 2 answered')).toBeTruthy() + + // …and Continue re-locks with the new answer. + fireEvent.submit(document.querySelector('form') as HTMLFormElement) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: 'blue', + question_id: 'q0', + request_id: 'request-batch' + }) + }) + }) + + it('Skip cancels the whole batch without a question_id', async () => { + const request = renderLiveBatch() + + fireEvent.click(screen.getByRole('button', { name: 'Skip' })) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: '', + request_id: 'request-batch' + }) + }) + }) + + it('renders the settled batch with all questions and answers', () => { + renderClarify( + + ) + + expect(screen.getByText('Color?')).toBeTruthy() + expect(screen.getByText('red')).toBeTruthy() + expect(screen.getByText('Name?')).toBeTruthy() + expect(screen.getByText('Skipped')).toBeTruthy() + }) +}) diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx index 27dd58fa53..b3584ccb4d 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -27,6 +27,8 @@ import { CircleLetterA, Loader2, MessageQuestion } from '@/lib/icons' import { cn } from '@/lib/utils' import { bareChoice, + type ClarifyQuestion, + type ClarifyRequest, clearClarifyRequest, normalizeChoices, RECOMMENDED_LABEL, @@ -43,6 +45,7 @@ interface ClarifyArgs { question?: string choices?: string[] | null multiSelect?: boolean + questions?: { question: string; choices?: string[] | null; multiSelect?: boolean }[] } interface ClarifyResult { @@ -72,13 +75,74 @@ function readClarifyArgs(args: unknown): ClarifyArgs { warnDroppedChoices('tool_args', question, rawChoices) } + // Batch form: tool args carry the model's questions array. Entries are + // normalized leniently here (qid comes from the gateway request, not args). + let questions: ClarifyArgs['questions'] + + if (Array.isArray(row.questions)) { + const parsed = row.questions + .map(entry => { + const item = parseMaybeObject(entry) + const text = stringField(item, 'question') + + if (!text) { + return null + } + + const itemChoices = normalizeChoices(item.choices) + + return { + choices: itemChoices.length > 0 ? itemChoices : null, + multiSelect: item.multi_select === true && itemChoices.length > 0, + question: text + } + }) + .filter((entry): entry is NonNullable => entry !== null) + + if (parsed.length > 0) { + questions = parsed + } + } + return { question, choices: choices.length > 0 ? choices : null, - multiSelect: row.multi_select === true + multiSelect: row.multi_select === true, + questions } } +interface ClarifyBatchResponse { + id?: string + question?: string + answer?: string | string[] +} + +/** Parse batch clarify tool JSON (`responses` array + optional timed_out). */ +export function readClarifyBatchResult(result: unknown): { + responses: ClarifyBatchResponse[] + timedOut: boolean +} { + const row = parseMaybeObject(result) + + if (!Array.isArray(row.responses)) { + return { responses: [], timedOut: false } + } + + const responses = row.responses.map((entry): ClarifyBatchResponse => { + const item = parseMaybeObject(entry) + const answer = item.user_response + + return { + answer: Array.isArray(answer) ? answer.map(String) : typeof answer === 'string' ? answer : undefined, + id: stringField(item, 'id'), + question: stringField(item, 'question') + } + }) + + return { responses, timedOut: row.timed_out === true } +} + /** Parse clarify tool JSON (`question` + `user_response`). */ export function readClarifyResult(result: unknown): ClarifyResult { const row = parseMaybeObject(result) @@ -236,7 +300,17 @@ function ClarifyToolLive(props: ToolCallMessagePartProps) { return } -function ClarifyToolSettled({ args, result }: ToolCallMessagePartProps) { +function ClarifyToolSettled(props: ToolCallMessagePartProps) { + const batch = readClarifyBatchResult(props.result) + + if (batch.responses.length > 0) { + return + } + + return +} + +function ClarifyToolSingleSettled({ args, result }: ToolCallMessagePartProps) { const { t } = useI18n() const copy = t.assistant.clarify const fromArgs = useMemo(() => readClarifyArgs(args), [args]) @@ -303,19 +377,36 @@ function ClarifyToolSettled({ args, result }: ToolCallMessagePartProps) { ) } -function ClarifyToolPending({ args }: ToolCallMessagePartProps) { - const { t } = useI18n() - const copy = t.assistant.clarify +function ClarifyToolPending(props: ToolCallMessagePartProps) { // The tool row is in whichever session's transcript rendered it — read THAT // session's clarify (primary or tile), not the globally-active one. const sessionId = useStore(useSessionView().$runtimeId) const $request = useMemo(() => sessionClarifyRequest(sessionId), [sessionId]) const request = useStore($request) + const fromArgs = useMemo(() => readClarifyArgs(props.args), [props.args]) + + // Batch: the gateway request carries qid-keyed questions. Args alone can't + // drive the form (no qids to respond with), so batch waits for the request. + if (request?.questions?.length || fromArgs.questions) { + return + } + + return +} + +function ClarifyToolSinglePending({ + fromArgs, + request +}: { + fromArgs: ClarifyArgs + request: ClarifyRequest | null +}) { + const { t } = useI18n() + const copy = t.assistant.clarify const gateway = useStore($gateway) - const fromArgs = useMemo(() => readClarifyArgs(args), [args]) const matchingRequest = useMemo(() => { - if (!request) { + if (!request || request.questions?.length) { return null } @@ -699,3 +790,346 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { ) } + +// ─── Batch (multi-question) clarify ───────────────────────────────────────── + +/** Settled batch card: every question with its locked (or absent) answer. */ +function ClarifyToolBatchSettled({ responses }: { responses: { question?: string; answer?: string | string[] }[] }) { + const { t } = useI18n() + const copy = t.assistant.clarify + + return ( + + {responses.map((row, index) => { + const answer = Array.isArray(row.answer) ? row.answer.join(', ') : (row.answer ?? '') + const blank = !answer.trim() + + return ( +
+ {row.question ? ( + + + {row.question} + + + ) : null} + +

+ {blank ? copy.skipped : answer} +

+ +
+ ) + })} + + ) +} + +/** One question's interactive block inside the live batch card. */ +function BatchQuestionBlock({ + disabled, + locked, + onDraft, + onToggle, + question, + staged +}: { + disabled: boolean + locked: boolean + onDraft: (value: string) => void + onToggle: (choice: string) => void + question: ClarifyQuestion + staged: { choices: string[]; draft: string } +}) { + const { t } = useI18n() + const copy = t.assistant.clarify + const choices = question.choices ?? [] + + return ( +
+
+ + {question.question} + + {locked ? ( + + ✓ {copy.answeredBadge} + + ) : null} +
+ + {choices.length > 0 ? ( +
+ {choices.map((choice, index) => ( + onToggle(choice)} + selected={staged.choices.includes(choice)} + /> + ))} +