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} 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 +}