fix(desktop): keep slash/@ completion menu navigable and Esc-dismissable

The desktop composer's `onKeyUp` handler unconditionally re-ran
`refreshTrigger` on every keyup, including the Arrow/Enter/Tab/Escape keys
the open-trigger `onKeyDown` branch had already fully handled. Because
`refreshTrigger` re-detects the trigger and resets the active index to 0,
this produced two bugs in the `/` (and `@`) completion popover:

- ArrowDown/ArrowUp moved the highlight on keydown, then keyup snapped it
  straight back to the top — so the user could never cycle past the first
  couple of items.
- Escape closed the menu on keydown, then keyup re-detected the still-present
  `/` and immediately reopened it — so Esc appeared to do nothing.

Fix: skip the keyup-driven refresh for the navigation/control keys while a
trigger menu is open (they never edit text, so refreshing is pointless), and
only reset the highlight in `refreshTrigger` when the detected trigger query
actually changed. Applied to both the main composer (chat/composer/index.tsx)
and the message-edit composer (assistant-ui/thread.tsx), which shared the
same bug. New `shouldSkipTriggerRefreshOnKeyUp` helper is unit-tested.
This commit is contained in:
kshitijk4poor
2026-06-03 11:11:35 +05:30
parent 9272e4019a
commit 188e52db91
4 changed files with 109 additions and 7 deletions

View File

@ -64,7 +64,7 @@ import {
RICH_INPUT_SLOT
} from './rich-editor'
import { SkinSlashPopover } from './skin-slash-popover'
import { detectTrigger, extractClipboardImageBlobs, textBeforeCaret, type TriggerState } from './text-utils'
import { detectTrigger, extractClipboardImageBlobs, shouldSkipTriggerRefreshOnKeyUp, textBeforeCaret, type TriggerState } from './text-utils'
import { ComposerTriggerPopover } from './trigger-popover'
import type { ChatBarProps } from './types'
import { UrlDialog } from './url-dialog'
@ -442,7 +442,14 @@ export function ChatBar({
const detected = detectTrigger(before ?? composerPlainText(editor))
setTrigger(detected)
setTriggerActive(0)
// Only reset the highlight when the trigger actually changed (opened, or
// the query/kind differs). Re-detecting the *same* trigger — e.g. on a
// caret move (mouseup) or a stray refresh — must preserve the user's
// current selection instead of snapping back to the first item.
if (detected?.kind !== trigger?.kind || detected?.query !== trigger?.query) {
setTriggerActive(0)
}
}, [trigger])
const handleEditorInput = (event: FormEvent<HTMLDivElement>) => {
@ -602,7 +609,15 @@ export function ChatBar({
}
}
const handleEditorKeyUp = () => {
const handleEditorKeyUp = (event: KeyboardEvent<HTMLDivElement>) => {
// Arrow/Enter/Tab/Escape while the trigger menu is open are fully handled
// in keydown and never edit text. Refreshing the trigger here would reset
// the highlight to the top (breaking ArrowDown/ArrowUp) and re-open a menu
// that Escape just closed, so skip it.
if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) {
return
}
window.setTimeout(refreshTrigger, 0)
}

View File

@ -0,0 +1,49 @@
import { describe, expect, it } from 'vitest'
import { detectTrigger, shouldSkipTriggerRefreshOnKeyUp } from './text-utils'
describe('shouldSkipTriggerRefreshOnKeyUp', () => {
it('skips the trigger refresh for nav/control keys while a menu is open', () => {
// These keys are fully handled by the open-trigger keydown branch and
// never edit text. Refreshing on their keyup resets the highlight to the
// top (breaking ArrowDown/ArrowUp cycling) and re-opens a menu Escape just
// closed — the exact bugs this guard prevents.
for (const key of ['ArrowUp', 'ArrowDown', 'Enter', 'Tab', 'Escape']) {
expect(shouldSkipTriggerRefreshOnKeyUp(key, true)).toBe(true)
}
})
it('does not skip the refresh when no trigger menu is open', () => {
for (const key of ['ArrowUp', 'ArrowDown', 'Enter', 'Tab', 'Escape']) {
expect(shouldSkipTriggerRefreshOnKeyUp(key, false)).toBe(false)
}
})
it('never skips ordinary text-editing keys, so completions still refresh', () => {
for (const key of ['a', '/', '@', ' ', 'Backspace', 'ArrowLeft', 'ArrowRight']) {
expect(shouldSkipTriggerRefreshOnKeyUp(key, true)).toBe(false)
}
})
})
describe('detectTrigger', () => {
it('detects a bare slash trigger with an empty query', () => {
expect(detectTrigger('/')).toEqual({ kind: '/', query: '', tokenLength: 1 })
})
it('detects a slash command query', () => {
expect(detectTrigger('/skill')).toEqual({ kind: '/', query: 'skill', tokenLength: 6 })
})
it('detects a bare at-mention trigger with an empty query', () => {
expect(detectTrigger('@')).toEqual({ kind: '@', query: '', tokenLength: 1 })
})
it('detects an at-mention query', () => {
expect(detectTrigger('@file')).toEqual({ kind: '@', query: 'file', tokenLength: 5 })
})
it('returns null for plain text', () => {
expect(detectTrigger('hello there')).toBeNull()
})
})

View File

@ -8,6 +8,25 @@ export interface TriggerState {
const TRIGGER_RE = /(?:^|[\s])([@/])([^\s@/]*)$/
/**
* Keys that the open-trigger keydown handler fully consumes to drive the
* completion popover (move highlight, accept, dismiss). None of them mutate
* the editor text, so re-running `refreshTrigger` on their *keyup* is both
* useless and actively harmful: `refreshTrigger` re-detects the trigger and
* resets `triggerActive` to 0 (snapping the highlight back to the top after
* every ArrowDown/ArrowUp) and re-opens a trigger that Escape just closed.
*/
const TRIGGER_NAV_KEYS: ReadonlySet<string> = new Set(['ArrowUp', 'ArrowDown', 'Enter', 'Tab', 'Escape'])
/**
* True when a keyup event should NOT trigger a completion-popover refresh.
* Only applies while a trigger menu is open and the key is one the keydown
* handler already handled — guarding against the highlight-reset / reopen race.
*/
export function shouldSkipTriggerRefreshOnKeyUp(key: string, triggerOpen: boolean): boolean {
return triggerOpen && TRIGGER_NAV_KEYS.has(key)
}
export function extractClipboardImageBlobs(clipboard: DataTransfer): Blob[] {
const blobs: Blob[] = []
const seen = new Set<Blob>()

View File

@ -44,7 +44,7 @@ import {
renderComposerContents,
RICH_INPUT_SLOT
} from '@/app/chat/composer/rich-editor'
import { detectTrigger, textBeforeCaret, type TriggerState } from '@/app/chat/composer/text-utils'
import { detectTrigger, shouldSkipTriggerRefreshOnKeyUp, textBeforeCaret, type TriggerState } from '@/app/chat/composer/text-utils'
import { ComposerTriggerPopover } from '@/app/chat/composer/trigger-popover'
import { extractDroppedFiles, HERMES_PATHS_MIME } from '@/app/chat/hooks/use-composer-actions'
import { ClarifyTool } from '@/components/assistant-ui/clarify-tool'
@ -997,8 +997,15 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
}
setTrigger(detected)
setTriggerActive(0)
}, [])
// Only reset the highlight when the trigger actually changed (opened, or
// the query/kind differs). Re-detecting the *same* trigger — e.g. on a
// caret move (mouseup) or a stray refresh — must preserve the user's
// current selection instead of snapping back to the first item.
if (detected?.kind !== trigger?.kind || detected?.query !== trigger?.query) {
setTriggerActive(0)
}
}, [trigger])
const closeTrigger = useCallback(() => {
setTrigger(null)
@ -1275,6 +1282,18 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
}
}
const handleKeyUp = (event: KeyboardEvent<HTMLDivElement>) => {
// Arrow/Enter/Tab/Escape while the trigger menu is open are fully handled
// in keydown and never edit text. Refreshing the trigger here would reset
// the highlight to the top (breaking ArrowDown/ArrowUp) and re-open a menu
// that Escape just closed, so skip it.
if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) {
return
}
window.setTimeout(refreshTrigger, 0)
}
return (
<ComposerPrimitive.Root className="contents" data-slot="aui_edit-composer-root">
<StickyHumanMessageContainer>
@ -1325,7 +1344,7 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
onFocus={() => markActiveComposer('edit')}
onInput={handleInput}
onKeyDown={handleKeyDown}
onKeyUp={() => window.setTimeout(refreshTrigger, 0)}
onKeyUp={handleKeyUp}
onMouseUp={refreshTrigger}
onPaste={handlePaste}
ref={editorRef}