Merge pull request #37937 from kshitijk4poor/fix/desktop-slash-menu-keyup-nav

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

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<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)
   }
 

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()
+  })
+})

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<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>()

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<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}