From a3662bb32ccaa3d8d0e833d6fdfab6b4fe211624 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Sat, 1 Aug 2026 17:04:38 -0700 Subject: [PATCH] fix(desktop): answer the visible clarify card, not the first-mounted one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pending clarify card binds Enter / 1-9 / A-Z / arrows on `window`, and inactive tabs stay MOUNTED, so every parked clarify keeps a live listener. Nothing in the handler asked whether the card was on screen: the first listener registered won, called preventDefault(), and the rest bailed on defaultPrevented. Registration order is mount order, not visibility. With two chats waiting on a question, answering the one in front of you sent `clarify.respond` for a background session's request instead — silently answering a question the user never saw and resuming that turn. The invariant is already stated one file over, in composer-focus-keys.ts: "a clarify card waiting in a background thread must not take the foreground composer's letter keys". `clarifyCardOwnsKey` honours it via queryVisible(); the card's own listener did not. That asymmetry is the bug — the key-ownership resolver and the key handler disagreed about which card is live. Export that lookup as `visibleClarifyCard()` and have both sides use it, so they cannot drift apart again. The card bails unless it IS the visible card, which also leaves the keystroke unprevented for the composer when the only pending card is hidden. Scoped by visible-card identity rather than `usePaneVisible()`: split zones each render their own active pane, so two cards can be visible at once and a per-pane visible flag would not disambiguate them. --- .../assistant-ui/clarify-tool.test.tsx | 90 +++++++++++++++++++ .../components/assistant-ui/clarify-tool.tsx | 19 +++- .../src/lib/keybinds/composer-focus-keys.ts | 13 ++- 3 files changed, 120 insertions(+), 2 deletions(-) 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