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 9e524be181..42f062852a 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.test.tsx @@ -1,9 +1,12 @@ import type { ToolCallMessagePartProps } from '@assistant-ui/react' import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { atom } from 'nanostores' import type { ReactNode } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' import { onComposerInsertRequest } from '@/app/chat/composer/focus' +import { type SessionView, SessionViewProvider } from '@/app/chat/session-view' +import { hiddenPaneProps } from '@/components/pane-shell/pane-visibility' import { I18nProvider } from '@/i18n' import { clearClarifyRequest, setClarifyRequest } from '@/store/clarify' import { $gateway } from '@/store/gateway' @@ -842,3 +845,90 @@ describe('ClarifyTool owner routing', () => { expect(ambient).not.toHaveBeenCalled() }) }) + +describe('ClarifyTool visible-card scoping', () => { + const BACKGROUND_SESSION = 'session-background' + const FOREGROUND_SESSION = 'session-foreground' + const BACKGROUND_REQUEST = 'request-background' + const FOREGROUND_REQUEST = 'request-foreground' + const QUESTION = 'Which deployment target?' + + /** Minimal per-session view — the pending card only reads `$runtimeId`. */ + function tileView(sessionId: string): SessionView { + return { ...({} as SessionView), $runtimeId: atom(sessionId), kind: 'tile' } + } + + function pendingCardProps(toolCallId: string): ToolCallMessagePartProps { + const args = { choices: ['staging', 'production'], question: QUESTION } + + return { ...liveClarifyProps(), args, argsText: JSON.stringify(args), toolCallId } + } + + function parkClarify(requestId: string, sessionId: string) { + setClarifyRequest({ + choices: ['staging', 'production'], + multiSelect: false, + question: QUESTION, + requestId, + sessionId + }) + } + + /** A card inside an inactive tab layer — mounted and live, just not on screen. */ + function backgroundCard() { + return ( +
+ + + +
+ ) + } + + it('answers the visible card, not a background one that mounted first', async () => { + const request = vi.fn().mockResolvedValue({ ok: true }) + + $gateway.set({ request } as never) + parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) + parkClarify(FOREGROUND_REQUEST, FOREGROUND_SESSION) + + // The background card is rendered FIRST, so its window listener registers + // first. Registration order used to decide the winner, which meant the card + // the user was looking at lost to one parked in an inactive tab. + renderClarify( + <> + {backgroundCard()} + + + + + ) + + fireEvent.keyDown(window, { key: 'Enter' }) + + await waitFor(() => { + expect(request).toHaveBeenCalledTimes(1) + }) + + // Exactly one answer, carrying the FOREGROUND request id — the background + // session's turn must not be resumed by a keystroke aimed at this one. + expect(request).toHaveBeenCalledWith('clarify.respond', { + answer: 'staging', + request_id: FOREGROUND_REQUEST + }) + }) + + it('leaves the key alone when the only pending card is hidden', () => { + const request = vi.fn().mockResolvedValue({ ok: true }) + + $gateway.set({ request } as never) + parkClarify(BACKGROUND_REQUEST, BACKGROUND_SESSION) + + renderClarify(backgroundCard()) + + // Untouched (no preventDefault) ⇒ the keystroke stays available to the + // composer, matching what `clarifyCardOwnsKey` reports with no visible card. + expect(fireEvent.keyDown(window, { key: 'Enter' })).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 0495576f8c..a9836d3ff6 100644 --- a/apps/desktop/src/components/assistant-ui/clarify-tool.tsx +++ b/apps/desktop/src/components/assistant-ui/clarify-tool.tsx @@ -24,6 +24,7 @@ import { Tip } from '@/components/ui/tooltip' import { useI18n } from '@/i18n' import { triggerHaptic } from '@/lib/haptics' import { CircleLetterA, Loader2, MessageQuestion } from '@/lib/icons' +import { visibleClarifyCard } from '@/lib/keybinds/composer-focus-keys' import { cn } from '@/lib/utils' import { bareChoice, @@ -444,6 +445,9 @@ function ClarifyToolSinglePending({ const [activeIndex, setActiveIndex] = useState(0) const [otherFocused, setOtherFocused] = useState(false) const textareaRef = useRef(null) + // Identity for the visible-card check below — this is the element carrying + // `data-clarify-choices`, i.e. the one `visibleClarifyCard()` resolves. + const formRef = useRef(null) // Race: tool.start fires a tick before clarify.request, so request_id // arrives slightly after the tool block mounts. If the question text is @@ -611,7 +615,8 @@ function ClarifyToolSinglePending({ // 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. + // or a button the user tabbed to — and whenever this card is not the visible + // one, since the binding is window-wide but the answer is session-specific. useEffect(() => { if (!ready || !hasChoices || submitting) { return @@ -622,6 +627,17 @@ function ClarifyToolSinglePending({ return } + // Not the visible card ⇒ not our keystroke. Inactive tabs stay MOUNTED, + // so every parked clarify keeps a live `window` listener; without this the + // card that acts is whichever mounted first, and answering the question in + // front of you silently answers a background session's question instead — + // resuming an agent turn the user never saw. Same resolver the composer's + // `clarifyCardOwnsKey` yields to, so the two cannot disagree about which + // card is live. + if (visibleClarifyCard() !== formRef.current) { + return + } + const active = document.activeElement as HTMLElement | null if ( @@ -717,6 +733,7 @@ function ClarifyToolSinglePending({ className="my-1.5 grid gap-4" data-clarify-choices={hasChoices ? choices.length : undefined} onSubmit={handleSubmit} + ref={formRef} >
diff --git a/apps/desktop/src/lib/keybinds/composer-focus-keys.ts b/apps/desktop/src/lib/keybinds/composer-focus-keys.ts index 4660ed0ff5..799fc56af9 100644 --- a/apps/desktop/src/lib/keybinds/composer-focus-keys.ts +++ b/apps/desktop/src/lib/keybinds/composer-focus-keys.ts @@ -48,6 +48,17 @@ const BLOCKING_OVERLAY = // must not take the foreground composer's letter keys. const BLOCKING_IN_SURFACE = '[data-clarify-choices]' +/** + * THE live clarify card — the single card whose shortcuts are armed right now. + * + * Exported because the card's own `keydown` handler has to resolve the same + * element this module does. Both bind the same keys on `window`, so if they + * disagree about which card is live they act for different sessions: this + * resolver yields to the VISIBLE card, while a handler keyed on anything else + * (mount order, say) answers one the user cannot see. + */ +export const visibleClarifyCard = (): HTMLElement | null => queryVisible(BLOCKING_IN_SURFACE) + /** True when the focused control would normally handle Enter itself. */ export function isActivateOnEnterTarget(target: EventTarget | null): boolean { const el = target as HTMLElement | null @@ -70,7 +81,7 @@ export function isActivateOnEnterTarget(target: EventTarget | null): boolean { * with no store coupling. */ export function clarifyCardOwnsKey(event: KeyboardEvent): boolean { - const card = queryVisible(BLOCKING_IN_SURFACE) + const card = visibleClarifyCard() if (!card) { return false