diff --git a/apps/desktop/src/app/chat/composer/index.tsx b/apps/desktop/src/app/chat/composer/index.tsx index 2a9580af3..d41d25d1a 100644 --- a/apps/desktop/src/app/chat/composer/index.tsx +++ b/apps/desktop/src/app/chat/composer/index.tsx @@ -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) => { @@ -602,7 +609,15 @@ export function ChatBar({ } } - const handleEditorKeyUp = () => { + const handleEditorKeyUp = (event: KeyboardEvent) => { + // 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) } diff --git a/apps/desktop/src/app/chat/composer/text-utils.test.ts b/apps/desktop/src/app/chat/composer/text-utils.test.ts new file mode 100644 index 000000000..21b433bf2 --- /dev/null +++ b/apps/desktop/src/app/chat/composer/text-utils.test.ts @@ -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() + }) +}) diff --git a/apps/desktop/src/app/chat/composer/text-utils.ts b/apps/desktop/src/app/chat/composer/text-utils.ts index 5725883d8..db0dab977 100644 --- a/apps/desktop/src/app/chat/composer/text-utils.ts +++ b/apps/desktop/src/app/chat/composer/text-utils.ts @@ -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 = 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() diff --git a/apps/desktop/src/components/assistant-ui/thread.tsx b/apps/desktop/src/components/assistant-ui/thread.tsx index dbeecd526..0eede6204 100644 --- a/apps/desktop/src/components/assistant-ui/thread.tsx +++ b/apps/desktop/src/components/assistant-ui/thread.tsx @@ -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 = ({ 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 = ({ cwd, gateway, sessionId } } } + const handleKeyUp = (event: KeyboardEvent) => { + // 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 ( @@ -1325,7 +1344,7 @@ const UserEditComposer: FC = ({ cwd, gateway, sessionId } onFocus={() => markActiveComposer('edit')} onInput={handleInput} onKeyDown={handleKeyDown} - onKeyUp={() => window.setTimeout(refreshTrigger, 0)} + onKeyUp={handleKeyUp} onMouseUp={refreshTrigger} onPaste={handlePaste} ref={editorRef}