From 251b668f4d83c113cc4b3175741ebb16126a4a05 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 20:50:32 -0500 Subject: [PATCH 1/2] fix(desktop): let the composer answer past a clarify card Typing a real message instead of picking an option left the agent parked: the clarify blocks inside its tool batch waiting on clarify.respond, so a follow-up routed through steer/queue sat undelivered until the 5-minute clarify timeout. skipClarifyRequest answers the parked question with the same empty answer the card's own Skip button sends, so the tool returns and the turn carries on with the user's words. --- .../lib/keybinds/composer-focus-keys.test.ts | 55 ++++++++++++------- .../src/lib/keybinds/composer-focus-keys.ts | 55 +++++++++++++++++-- apps/desktop/src/store/clarify.test.ts | 48 +++++++++++++++- apps/desktop/src/store/clarify.ts | 40 ++++++++++++++ 4 files changed, 170 insertions(+), 28 deletions(-) diff --git a/apps/desktop/src/lib/keybinds/composer-focus-keys.test.ts b/apps/desktop/src/lib/keybinds/composer-focus-keys.test.ts index 7594a12b06d1..a75f649c0010 100644 --- a/apps/desktop/src/lib/keybinds/composer-focus-keys.test.ts +++ b/apps/desktop/src/lib/keybinds/composer-focus-keys.test.ts @@ -80,22 +80,11 @@ describe('composerFocusBlockedBySurface', () => { expect(composerFocusBlockedBySurface()).toBe(true) }) - it('blocks while a live clarify choices card owns its letter keys', () => { + it('ignores a clarify card — it yields only its own keys, per-key', () => { const card = document.createElement('div') - card.setAttribute('data-clarify-choices', '') + card.setAttribute('data-clarify-choices', '2') document.body.append(card) - expect(composerFocusBlockedBySurface()).toBe(true) - }) - - it('ignores a clarify card waiting in a kept-alive background tab', () => { - const tab = document.createElement('div') - tab.setAttribute('data-pane-hidden', '') - const card = document.createElement('div') - card.setAttribute('data-clarify-choices', '') - tab.append(card) - document.body.append(tab) - expect(composerFocusBlockedBySurface()).toBe(false) }) @@ -174,15 +163,41 @@ describe('composerFocusKeysAllowed', () => { expect(composerFocusKeysAllowed(keydown({ key: 'a', code: 'KeyA', target: document.body }), 'type')).toBe(false) }) - it('yields letter + Enter keys to a live clarify choices card', () => { + it('yields only the keys a live clarify card actually binds', () => { const card = document.createElement('div') - card.setAttribute('data-clarify-choices', '') + // Two choices → rows A/B plus the "Other" row C; 1/2/3 are the digit twins. + card.setAttribute('data-clarify-choices', '2') document.body.append(card) - // The clarify card's own A/B/C… + Enter shortcuts must win over type-to-focus. - expect(composerFocusKeysAllowed(keydown({ key: 'a', code: 'KeyA', target: document.body }), 'type')).toBe(false) - expect(composerFocusKeysAllowed(keydown({ key: 'Enter', code: 'Enter', target: document.body }), 'enter')).toBe( - false - ) + const allowed = (key: string, combo: string) => + composerFocusKeysAllowed(keydown({ key, target: document.body }), combo) + + // The card's own shortcuts win over type-to-focus. + expect(allowed('a', 'type')).toBe(false) + expect(allowed('B', 'type')).toBe(false) + expect(allowed('c', 'type')).toBe(false) + expect(allowed('1', 'type')).toBe(false) + expect(allowed('3', 'type')).toBe(false) + expect(allowed('Enter', 'enter')).toBe(false) + + // Everything past the last row is not the card's — typing a real message + // instead of picking an option must still reach the composer. + expect(allowed('d', 'type')).toBe(true) + expect(allowed('z', 'type')).toBe(true) + expect(allowed('4', 'type')).toBe(true) + expect(allowed('?', 'type')).toBe(true) + expect(allowed(' ', 'type')).toBe(true) + }) + + it('leaves every key alone for a clarify card in a background tab', () => { + const tab = document.createElement('div') + tab.setAttribute('data-pane-hidden', '') + const card = document.createElement('div') + card.setAttribute('data-clarify-choices', '2') + tab.append(card) + document.body.append(tab) + + expect(composerFocusKeysAllowed(keydown({ key: 'a', target: document.body }), 'type')).toBe(true) + expect(composerFocusKeysAllowed(keydown({ key: 'Enter', target: document.body }), 'enter')).toBe(true) }) }) diff --git a/apps/desktop/src/lib/keybinds/composer-focus-keys.ts b/apps/desktop/src/lib/keybinds/composer-focus-keys.ts index 29da02cda58f..4660ed0ff51b 100644 --- a/apps/desktop/src/lib/keybinds/composer-focus-keys.ts +++ b/apps/desktop/src/lib/keybinds/composer-focus-keys.ts @@ -56,19 +56,61 @@ export function isActivateOnEnterTarget(target: EventTarget | null): boolean { } /** - * Dialogs, menus, terminal, full pages, session switcher, any open overlay - * (settings / command-center / star map / …), and a live clarify choices card — + * True when a live clarify card binds THIS key, so type-to-focus must yield it. + * + * The card owns Enter plus the shortcuts it actually renders — `1..N+1` and + * `A..` for its N choices and the trailing "Other" row. It does NOT own the + * rest of the alphabet: typing a real message instead of picking an option is a + * legitimate answer ("none of these"), and blanket-blocking every printable + * left the user unable to start that message at all — the first letter vanished + * and the composer never focused. Out-of-range keys fall through to the + * composer, which skips the question on send. + * + * The choice count rides in the attribute's value, so this stays a DOM read + * with no store coupling. + */ +export function clarifyCardOwnsKey(event: KeyboardEvent): boolean { + const card = queryVisible(BLOCKING_IN_SURFACE) + + if (!card) { + return false + } + + if (event.key === 'Enter') { + return true + } + + // "Other" is the row past the last choice, hence the +1. + const rows = Number(card.getAttribute('data-clarify-choices')) + 1 + + if (!Number.isFinite(rows)) { + return false + } + + const key = event.key.toLowerCase() + + if (key.length !== 1) { + return false + } + + const index = /^[1-9]$/.test(key) ? Number(key) - 1 : key >= 'a' && key <= 'z' ? key.charCodeAt(0) - 97 : -1 + + return index >= 0 && index < rows +} + +/** + * Dialogs, menus, terminal, full pages, session switcher, and any open overlay — * they keep their keys, so type-to-focus / soft `/` / Enter stand down rather * than stealing keystrokes those surfaces own (or leaking them into the composer - * mounted behind an overlay). + * mounted behind an overlay). A live clarify card is handled per-key by + * `clarifyCardOwnsKey`, not here — it only owns its own shortcuts. */ export function composerFocusBlockedBySurface(): boolean { return ( switcherActive() || $workspaceIsPage.get() || isFocusWithin('[data-terminal]') || - Boolean(document.querySelector(BLOCKING_OVERLAY)) || - Boolean(queryVisible(BLOCKING_IN_SURFACE)) + Boolean(document.querySelector(BLOCKING_OVERLAY)) ) } @@ -95,7 +137,8 @@ export function composerFocusKeysAllowed(event: KeyboardEvent, combo: string): b event.defaultPrevented || event.isComposing || isEditableTarget(event.target) || - composerFocusBlockedBySurface() + composerFocusBlockedBySurface() || + clarifyCardOwnsKey(event) ) { return false } diff --git a/apps/desktop/src/store/clarify.test.ts b/apps/desktop/src/store/clarify.test.ts index ed995a7a52a6..aec413b5b369 100644 --- a/apps/desktop/src/store/clarify.test.ts +++ b/apps/desktop/src/store/clarify.test.ts @@ -1,13 +1,16 @@ -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { $clarifyRequest, $clarifyRequests, type ClarifyRequest, clearClarifyRequest, + hasClarifyRequest, normalizeChoices, - setClarifyRequest + setClarifyRequest, + skipClarifyRequest } from './clarify' +import { $gateway } from './gateway' import { $activeSessionId } from './session' function clarify(sessionId: string | null, requestId: string): ClarifyRequest { @@ -81,6 +84,47 @@ describe('clarify store', () => { }) }) +describe('skipClarifyRequest', () => { + const request = vi.fn(async () => ({ ok: true })) + + beforeEach(() => { + $clarifyRequests.set({}) + request.mockClear() + $gateway.set({ request } as unknown as ReturnType) + }) + + afterEach(() => { + $clarifyRequests.set({}) + $gateway.set(null) + }) + + it('answers the session\u2019s clarify with an empty answer and drops it', async () => { + setClarifyRequest(clarify('session-a', 'req-a')) + setClarifyRequest(clarify('session-b', 'req-b')) + + await expect(skipClarifyRequest('session-a')).resolves.toBe(true) + + expect(request).toHaveBeenCalledWith('clarify.respond', { request_id: 'req-a', answer: '' }) + expect(hasClarifyRequest('session-a')).toBe(false) + // A background session's question is untouched — only the one being typed + // over is skipped. + expect(hasClarifyRequest('session-b')).toBe(true) + }) + + it('is a no-op when the session has no clarify parked', async () => { + await expect(skipClarifyRequest('session-a')).resolves.toBe(false) + expect(request).not.toHaveBeenCalled() + }) + + it('still reports the skip when the respond RPC fails', async () => { + setClarifyRequest(clarify('session-a', 'req-a')) + request.mockRejectedValueOnce(new Error('socket closed')) + + await expect(skipClarifyRequest('session-a')).resolves.toBe(true) + expect(hasClarifyRequest('session-a')).toBe(false) + }) +}) + describe('normalizeChoices', () => { it('returns empty array for null/undefined', () => { expect(normalizeChoices(null)).toEqual([]) diff --git a/apps/desktop/src/store/clarify.ts b/apps/desktop/src/store/clarify.ts index f90dcd13174b..6cdc41eb44ae 100644 --- a/apps/desktop/src/store/clarify.ts +++ b/apps/desktop/src/store/clarify.ts @@ -1,5 +1,6 @@ import { atom, computed } from 'nanostores' +import { $gateway } from './gateway' import { $activeSessionId } from './session' export interface ClarifyRequest { @@ -102,3 +103,42 @@ export function clearClarifyRequest(requestId?: string, sessionId?: string | nul $clarifyRequests.set(next) } } + +/** Whether `sessionId` has a clarify parked on it right now (imperative read — + * the composer checks this on Enter, not on every render). */ +export const hasClarifyRequest = (sessionId: string | null | undefined): boolean => + Boolean($clarifyRequests.get()[keyFor(sessionId)]) + +/** + * Answer `sessionId`'s pending clarify with an empty answer (a skip) and drop it + * locally, resolving to whether there was one to skip. + * + * The composer uses this when the user types a real message instead of picking + * an option: a clarify blocks the agent inside its tool batch, so leaving it + * unanswered would park the follow-up until the server-side clarify timeout + * (default 5 min) — the message looks sent and nothing happens. Skipping lets + * the tool return and the turn carry on with the user's actual words. + * + * An empty answer is the same thing the card's own Skip button sends, and + * `clarify.respond` is `allow_expired`, so racing the timeout is harmless. + */ +export async function skipClarifyRequest(sessionId: string | null | undefined): Promise { + const request = $clarifyRequests.get()[keyFor(sessionId)] + + if (!request) { + return false + } + + // Clear first: the answer is already decided, and an in-flight RPC must not + // leave a live card the user can answer a second time. + clearClarifyRequest(request.requestId, request.sessionId) + + try { + await $gateway.get()?.request('clarify.respond', { request_id: request.requestId, answer: '' }) + } catch { + // The tool times out on its own; a failed skip must never swallow the + // message the user is actually sending. + } + + return true +} From c0dd6e1f3fb6fdabdeb13a037cc4a2d063e2e367 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 20:50:32 -0500 Subject: [PATCH 2/2] fix(desktop): stop a clarify card swallowing keys it doesn't bind The card marked itself as owning every printable key, so the first letter of a typed-out answer vanished and the composer never focused. It now publishes its row count and yields only Enter plus the 1..N+1 / A.. rows it actually renders; everything else reaches the composer, which skips the question on send. --- .../hooks/use-composer-submit.test.tsx | 79 +++++++++++++++++++ .../composer/hooks/use-composer-submit.ts | 16 ++++ .../assistant-ui/clarify-tool.test.tsx | 13 +-- .../components/assistant-ui/clarify-tool.tsx | 16 ++-- 4 files changed, 114 insertions(+), 10 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx index b3bcaaa463e3..dd83d04f0c0f 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.test.tsx @@ -1,7 +1,9 @@ import { act, cleanup, renderHook, waitFor } from '@testing-library/react' import { afterEach, describe, expect, it, vi } from 'vitest' +import { $clarifyRequests } from '@/store/clarify' import type { ComposerAttachment } from '@/store/composer' +import { $gateway } from '@/store/gateway' import { useComposerSubmit } from './use-composer-submit' @@ -184,3 +186,80 @@ describe('useComposerSubmit busy-turn routing', () => { ) }) }) + +describe('useComposerSubmit with a clarify parked on the session', () => { + const gatewayRequest = vi.fn(async () => ({ ok: true })) + + const parkClarify = (sessionId: string) => { + $clarifyRequests.set({ + [sessionId]: { requestId: `req-${sessionId}`, question: 'which one?', choices: ['a', 'b'], sessionId } + }) + $gateway.set({ request: gatewayRequest } as unknown as ReturnType) + } + + afterEach(() => { + cleanup() + gatewayRequest.mockClear() + $clarifyRequests.set({}) + $gateway.set(null) + vi.restoreAllMocks() + }) + + it('skips the question and still sends the typed message on an idle session', async () => { + parkClarify('runtime-session') + const { hook, onSubmit } = renderSubmitHook({ text: 'actually do this instead' }) + + act(() => { + hook.result.current.submitDraft() + }) + + await waitFor(() => + expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', { + request_id: 'req-runtime-session', + answer: '' + }) + ) + await waitFor(() => + expect(onSubmit).toHaveBeenCalledWith('actually do this instead', expect.objectContaining({ attachments: [] })) + ) + expect($clarifyRequests.get()['runtime-session']).toBeUndefined() + }) + + it('skips the question before steering a busy turn', async () => { + parkClarify('runtime-session') + const { hook, onSteer } = renderSubmitHook({ busy: true, text: 'change course' }) + + act(() => { + hook.result.current.submitDraft() + }) + + await waitFor(() => expect(onSteer).toHaveBeenCalledWith('change course')) + expect(gatewayRequest).toHaveBeenCalledWith('clarify.respond', { request_id: 'req-runtime-session', answer: '' }) + }) + + it('leaves the question alone for an empty Enter (Stop, not an answer)', () => { + parkClarify('runtime-session') + const { hook, onCancel } = renderSubmitHook({ busy: true }) + + act(() => { + hook.result.current.submitDraft() + }) + + expect(gatewayRequest).not.toHaveBeenCalled() + expect($clarifyRequests.get()['runtime-session']).toBeDefined() + expect(onCancel).toHaveBeenCalledTimes(1) + }) + + it("leaves another session's question alone", async () => { + parkClarify('other-session') + const { hook, onSubmit } = renderSubmitHook({ text: 'unrelated message' }) + + act(() => { + hook.result.current.submitDraft() + }) + + await waitFor(() => expect(onSubmit).toHaveBeenCalled()) + expect(gatewayRequest).not.toHaveBeenCalled() + expect($clarifyRequests.get()['other-session']).toBeDefined() + }) +}) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts index 315157a96e55..fff0b8153fcd 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts @@ -2,6 +2,7 @@ import { type RefObject, useEffect, useRef } from 'react' import { SLASH_COMMAND_RE } from '@/lib/chat-runtime' import { triggerHaptic } from '@/lib/haptics' +import { hasClarifyRequest, skipClarifyRequest } from '@/store/clarify' import { clearSessionDraft, type ComposerAttachment } from '@/store/composer' import { resetBrowseState } from '@/store/composer-input-history' import { enqueueQueuedPrompt, type QueuedPromptEntry } from '@/store/composer-queue' @@ -141,6 +142,21 @@ export function useComposerSubmit({ const text = draftRef.current const payloadPresent = text.trim().length > 0 || attachments.length > 0 + // A clarify card parked on this session owns the turn: the agent is blocked + // inside its tool batch waiting on `clarify.respond`, so a follow-up routed + // through steer/queue sits undelivered until the clarify's own timeout + // (default 5 min) — the message looks sent and nothing happens. Typing a + // real message instead of picking an option IS the answer "none of these": + // skip the question so the tool returns, then route the words normally. + // + // Fire-and-forget, not awaited: the skip clears the card synchronously and + // both RPCs ride the same socket in call order, so the gateway resolves the + // clarify before it sees the follow-up. Awaiting first would leave the draft + // live for a tick — long enough for a second Enter to send it twice. + if (payloadPresent && !queueEdit && hasClarifyRequest(sessionId)) { + void skipClarifyRequest(sessionId) + } + if (queueEdit) { exitQueuedEdit('save') } else if (busy) { 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 7e0daf7ef34a..ecb4c6c2faaa 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx @@ -298,13 +298,16 @@ describe('ClarifyTool keyboard navigation', () => { }) describe('ClarifyTool pending marker', () => { - it('marks a live choices card so type-to-focus yields its shortcut keys', () => { + it('marks a live choices card with its row count so type-to-focus yields exactly its keys', () => { renderLiveClarify() - // The marker is what `composerFocusBlockedBySurface` keys off of, so the - // global type-to-focus listener stands down and A/B/C… + 1-9 + Enter reach - // the card instead of the composer. - expect(document.querySelector('[data-clarify-choices]')).toBeTruthy() + // `clarifyCardOwnsKey` reads the count off this marker to yield only the + // shortcuts the card renders (A..N + "Other", 1-9, Enter) and let every + // other printable through to the composer. + const card = document.querySelector('[data-clarify-choices]') + + expect(card).toBeTruthy() + expect(Number(card?.getAttribute('data-clarify-choices'))).toBeGreaterThan(0) }) it('does not mark a free-text (no-choice) pending card', () => { diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx index 6f1442e2cfed..1c143eb0262f 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -489,6 +489,10 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { const key = event.key.toLowerCase() + // Only the letters this card actually renders a row for. Anything past + // the last row belongs to the composer — the user is typing a message + // instead of picking an option, and swallowing the keystroke here would + // make the first letter of it vanish. if (key.length === 1 && key >= 'a' && key <= 'z') { const index = key.charCodeAt(0) - 97 @@ -538,11 +542,13 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { } return ( - // `data-clarify-choices` marks the panel as owning printable/Enter keys - // while its A/B/C… shortcuts are live, so the global type-to-focus listener - // (`composerFocusBlockedBySurface`) stands down and the letters reach this - // card instead of being redirected into the composer. - + // `data-clarify-choices` marks the panel as owning its OWN shortcut keys + // (Enter, and 1..N+1 / A.. for the N choices plus "Other") while they're + // live, so the global type-to-focus listener (`clarifyCardOwnsKey`) yields + // exactly those and lets every other printable through to the composer — + // typing a real message instead of picking an option stays possible. The + // value is the choice count so the check needs no store access. +
{question}