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 cdf592b16b8e..ddefafa821a0 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx @@ -1,15 +1,28 @@ import type { ToolCallMessagePartProps } from '@assistant-ui/react' -import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' import type { ReactNode } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' import { onComposerInsertRequest } from '@/app/chat/composer/focus' import { I18nProvider } from '@/i18n' +import { clearClarifyRequest, setClarifyRequest } from '@/store/clarify' +import { $gateway } from '@/store/gateway' +import { $activeSessionId } from '@/store/session' import { ClarifyTool, 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. +vi.mock('@assistant-ui/react', () => ({ + useAuiState: () => true +})) + afterEach(() => { cleanup() + clearClarifyRequest() + $activeSessionId.set(null) + $gateway.set(null) + vi.clearAllMocks() }) function renderClarify(ui: ReactNode) { @@ -40,6 +53,40 @@ function settledClarifyProps( } } +function liveClarifyProps(choices = ['staging', 'production']): ToolCallMessagePartProps { + const args = { choices, question: 'Which deployment target?' } + + return { + addResult: vi.fn(), + args, + argsText: JSON.stringify(args), + isError: false, + respondToApproval: vi.fn(), + result: undefined, + resume: vi.fn(), + status: { type: 'running' }, + toolCallId: 'clarify-live', + toolName: 'clarify', + type: 'tool-call' + } +} + +function renderLiveClarify() { + const request = vi.fn().mockResolvedValue({ ok: true }) + + $activeSessionId.set('session-1') + $gateway.set({ request } as never) + setClarifyRequest({ + choices: ['staging', 'production'], + question: 'Which deployment target?', + requestId: 'request-1', + sessionId: 'session-1' + }) + renderClarify() + + return request +} + describe('readClarifyResult', () => { it('reads question + user_response from the tool JSON payload', () => { expect( @@ -182,3 +229,70 @@ describe('ClarifyTool settled view', () => { expect(document.querySelector('[data-clarify-late-choices]')).toBeNull() }) }) + +describe('ClarifyTool keyboard navigation', () => { + it('cycles through choices and Other with the arrow keys', () => { + renderLiveClarify() + + const staging = screen.getByRole('button', { name: /staging/ }) + const production = screen.getByRole('button', { name: /production/ }) + const other = screen.getByPlaceholderText(/Other/) + + expect(staging.getAttribute('data-highlighted')).toBe('true') + expect(staging.getAttribute('aria-current')).toBe('true') + expect(staging.getAttribute('aria-keyshortcuts')).toBe('A 1') + + fireEvent.keyDown(window, { key: 'ArrowDown' }) + expect(production.getAttribute('data-highlighted')).toBe('true') + + fireEvent.keyDown(window, { key: 'ArrowDown' }) + expect(other.closest('label')?.getAttribute('data-highlighted')).toBe('true') + expect(other.getAttribute('aria-current')).toBe('true') + expect(other.getAttribute('aria-keyshortcuts')).toBe('C 3') + + fireEvent.keyDown(window, { key: 'ArrowDown' }) + expect(staging.getAttribute('data-highlighted')).toBe('true') + + fireEvent.keyDown(window, { key: 'ArrowUp' }) + expect(other.closest('label')?.getAttribute('data-highlighted')).toBe('true') + }) + + it('selects by number and confirms the answer with Enter', async () => { + const request = renderLiveClarify() + + fireEvent.keyDown(window, { key: '2' }) + fireEvent.keyDown(window, { key: 'Enter' }) + + await waitFor(() => { + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: 'production', + request_id: 'request-1' + }) + }) + }) + + it('focuses Other when its number is pressed and leaves typing keys alone', () => { + renderLiveClarify() + + const other = screen.getByPlaceholderText(/Other/) + + fireEvent.keyDown(window, { key: '3' }) + expect(document.activeElement).toBe(other) + + fireEvent.change(other, { target: { value: 'canary' } }) + fireEvent.keyDown(window, { key: 'ArrowUp' }) + expect(document.activeElement).toBe(other) + expect((other as HTMLTextAreaElement).value).toBe('canary') + }) + + it('does not intercept keyboard events while an action button has focus', () => { + const request = renderLiveClarify() + const skip = screen.getByRole('button', { name: 'Skip' }) + + skip.focus() + + expect(fireEvent.keyDown(window, { key: 'Enter' })).toBe(true) + expect(fireEvent.keyDown(window, { key: 'ArrowDown' })).toBe(true) + expect(request).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx index 23cfe9ad52e1..f9353aadad18 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -138,16 +138,20 @@ function KeyBadge({ char, preview, selected }: { char: string; preview?: boolean * selects an answer) and the settled skip card (where a click drafts a * follow-up), so both stay visually identical. */ function ChoiceButton({ + active = false, char, choice, disabled, + keyShortcuts, onClick, selected = false, title }: { + active?: boolean char: string choice: string disabled?: boolean + keyShortcuts?: string onClick: () => void selected?: boolean title?: string @@ -156,20 +160,28 @@ function ChoiceButton({ // tooltip on a @@ -298,6 +310,9 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { const [draft, setDraft] = useState('') const [submitting, setSubmitting] = useState(false) const [selectedChoice, setSelectedChoice] = useState(null) + // The keyboard cursor. Indices 0..choices.length-1 are the options; the + // trailing index (=== choices.length) is the "Other" free-text row. + const [activeIndex, setActiveIndex] = useState(0) const [otherFocused, setOtherFocused] = useState(false) const textareaRef = useRef(null) @@ -346,12 +361,31 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { // confirms with Continue (or Enter from the field). const pendingAnswer = selectedChoice ?? (trimmedDraft || null) - const selectChoice = useCallback((choice: string) => { + const selectChoice = useCallback((choice: string, index: number) => { // Picking a choice and typing are mutually exclusive answers. setDraft('') setSelectedChoice(choice) + setActiveIndex(index) }, []) + // Keep the cursor in range when the choice set changes (never past "Other"). + useEffect(() => { + setActiveIndex(index => Math.min(index, choices.length)) + }, [choices.length]) + + const moveActive = useCallback( + (delta: number) => { + const itemCount = choices.length + 1 + + // Arrow navigation is a move, not a pick — clear any staged answer so the + // cursor and the selection can't disagree. + setDraft('') + setSelectedChoice(null) + setActiveIndex(index => (index + delta + itemCount) % itemCount) + }, + [choices.length] + ) + const submitAnswer = useCallback(() => { if (selectedChoice !== null) { void respond(selectedChoice) @@ -364,6 +398,27 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { } }, [respond, selectedChoice, trimmedDraft]) + const activateActive = useCallback(() => { + // A staged answer (picked choice or typed text) wins — confirm it. + if (pendingAnswer) { + submitAnswer() + + return + } + + // Otherwise act on the highlighted row: a choice responds immediately, and + // the trailing "Other" row focuses the free-text field. + const choice = choices[activeIndex] + + if (choice) { + void respond(choice) + + return + } + + textareaRef.current?.focus() + }, [activeIndex, choices, pendingAnswer, respond, submitAnswer]) + const handleTextareaKey = useCallback( (event: KeyboardEvent) => { if (event.nativeEvent.isComposing) { @@ -386,10 +441,11 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { [submitAnswer] ) - // Letter shortcuts: A/B/C… pick the matching option, the trailing letter jumps - // into "Other", and Enter confirms the current pick. Stands down whenever a - // field is focused (you're typing, not navigating) so it never eats keystrokes - // meant for the composer or the Other box. + // Arrow keys move a visual cursor, 1-9 and A/B/C… pick directly, and Enter + // confirms the current answer (or acts on the highlighted row). Stands down + // whenever a focusable control (a field, a choice button, the action bar) is + // focused, so it never eats keystrokes meant for the composer, the Other box, + // or a button the user tabbed to. useEffect(() => { if (!ready || !hasChoices || submitting) { return @@ -402,7 +458,32 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { const active = document.activeElement as HTMLElement | null - if (active && (active.tagName === 'INPUT' || active.tagName === 'TEXTAREA' || active.isContentEditable)) { + if ( + active && + (active.isContentEditable || active.matches('a[href], button, input, select, textarea, [role="button"]')) + ) { + return + } + + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + event.preventDefault() + moveActive(event.key === 'ArrowDown' ? 1 : -1) + + return + } + + if (/^[1-9]$/.test(event.key)) { + const index = Number(event.key) - 1 + + if (index < choices.length) { + event.preventDefault() + selectChoice(choices[index], index) + } else if (index === choices.length) { + event.preventDefault() + setActiveIndex(index) + textareaRef.current?.focus() + } + return } @@ -413,25 +494,26 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) { if (index < choices.length) { event.preventDefault() - selectChoice(choices[index]) + selectChoice(choices[index], index) } else if (index === choices.length) { event.preventDefault() + setActiveIndex(index) textareaRef.current?.focus() } return } - if (event.key === 'Enter' && pendingAnswer) { + if (event.key === 'Enter') { event.preventDefault() - submitAnswer() + activateActive() } } window.addEventListener('keydown', onKeyDown) return () => window.removeEventListener('keydown', onKeyDown) - }, [choices, hasChoices, pendingAnswer, ready, selectChoice, submitAnswer, submitting]) + }, [activateActive, choices, hasChoices, moveActive, ready, selectChoice, submitting]) if (loading) { return ( @@ -467,23 +549,39 @@ function ClarifyToolPending({ args }: ToolCallMessagePartProps) {
{choices.map((choice, index) => ( selectChoice(choice)} + keyShortcuts={`${letterFor(index)} ${index + 1}`} + onClick={() => selectChoice(choice, index)} selected={selectedChoice === choice} /> ))} -