fix(desktop): answer the visible clarify card, not the first-mounted one
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.
This commit is contained in:
@@ -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<null | string>(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 (
|
||||
<div {...hiddenPaneProps(true)}>
|
||||
<SessionViewProvider value={tileView(BACKGROUND_SESSION)}>
|
||||
<ClarifyTool {...pendingCardProps('clarify-background')} />
|
||||
</SessionViewProvider>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
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()}
|
||||
<SessionViewProvider value={tileView(FOREGROUND_SESSION)}>
|
||||
<ClarifyTool {...pendingCardProps('clarify-foreground')} />
|
||||
</SessionViewProvider>
|
||||
</>
|
||||
)
|
||||
|
||||
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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<HTMLTextAreaElement | null>(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<HTMLFormElement | null>(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}
|
||||
>
|
||||
<ClarifyShell className="grid gap-2">
|
||||
<div className="flex items-start gap-2">
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user