fix(desktop): stop Esc reopening the slash/@ menu; harden keyup guard

Follow-up to #37937. That fix guarded the composer's keyup with
`shouldSkipTriggerRefreshOnKeyUp(key, trigger !== null)`. The `trigger !== null`
check is timing-fragile for Escape: Escape's *keydown* sets `trigger = null`
and closes the menu, but in a real browser the *keyup* fires after a re-render,
so the handler closure sees `trigger === null`, the guard returns false,
`refreshTrigger` runs, re-detects the still-present `/` in the input, and
instantly reopens the menu. (jsdom batches state synchronously so a unit test
could not observe this -- only the running app does.)

Replace the value-based guard with a `triggerKeyConsumedRef` set synchronously
in keydown whenever the open popover consumes a nav/control key
(Arrow/Enter/Tab/Escape). keyup consults and clears that ref, so it is immune
to the keydown->re-render->keyup timing. Applied to both the main composer
(chat/composer/index.tsx) and the message-edit composer
(assistant-ui/thread.tsx).

Removes the now-unused `shouldSkipTriggerRefreshOnKeyUp` helper and its unit
test. The real-DOM regression test now fires keydown+keyup pairs through the
ref-based handlers and asserts Esc closes and stays closed.

Verified by running a production renderer build (Vite v8) under Electron
against a local backend: ArrowDown/ArrowUp cycle the full list and Esc
dismisses the menu without reopening.

apps/desktop/src/app/chat/composer/index.tsx

@@ -65,7 +65,7 @@ import {
   RICH_INPUT_SLOT
 } from './rich-editor'
 import { SkinSlashPopover } from './skin-slash-popover'
-import { detectTrigger, extractClipboardImageBlobs, shouldSkipTriggerRefreshOnKeyUp, textBeforeCaret, type TriggerState } from './text-utils'
+import { detectTrigger, extractClipboardImageBlobs, textBeforeCaret, type TriggerState } from './text-utils'
 import { ComposerTriggerPopover } from './trigger-popover'
 import type { ChatBarProps } from './types'
 import { UrlDialog } from './url-dialog'
@@ -421,6 +421,14 @@ export function ChatBar({
   const [trigger, setTrigger] = useState<TriggerState | null>(null)
   const [triggerActive, setTriggerActive] = useState(0)
   const [triggerItems, setTriggerItems] = useState<readonly Unstable_TriggerItem[]>([])
+  // Set synchronously in keydown when the open trigger popover consumes a
+  // navigation/control key (Arrow/Enter/Tab/Escape). The subsequent keyup must
+  // NOT run refreshTrigger for that keypress: it never edits text, and for
+  // Escape the keydown has already set trigger=null, so a keyup refresh would
+  // re-detect the still-present `/` and instantly reopen the menu. A ref is
+  // used instead of reading `trigger` in keyup because by keyup time React has
+  // re-rendered and the handler closure sees the post-keydown state.
+  const triggerKeyConsumedRef = useRef(false)
 
   const refreshTrigger = useCallback(() => {
     const editor = editorRef.current
@@ -572,6 +580,7 @@ export function ChatBar({
     if (trigger && triggerItems.length > 0) {
       if (event.key === 'ArrowDown') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx + 1) % triggerItems.length)
 
         return
@@ -579,6 +588,7 @@ export function ChatBar({
 
       if (event.key === 'ArrowUp') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx - 1 + triggerItems.length) % triggerItems.length)
 
         return
@@ -586,6 +596,7 @@ export function ChatBar({
 
       if (event.key === 'Enter' || event.key === 'Tab') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         const item = triggerItems[triggerActive]
 
         if (item) {
@@ -597,6 +608,7 @@ export function ChatBar({
 
       if (event.key === 'Escape') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         closeTrigger()
 
         return
@@ -616,12 +628,16 @@ export function ChatBar({
     }
   }
 
-  const handleEditorKeyUp = (event: KeyboardEvent<HTMLDivElement>) => {
+  const handleEditorKeyUp = () => {
-    // Arrow/Enter/Tab/Escape while the trigger menu is open are fully handled
+    // If this keyup belongs to a key the open trigger popover already consumed
-    // in keydown and never edit text. Refreshing the trigger here would reset
+    // in keydown (Arrow/Enter/Tab/Escape), skip the refresh. Those keys never
-    // the highlight to the top (breaking ArrowDown/ArrowUp) and re-open a menu
+    // edit text, and for Escape the keydown already closed the menu — a refresh
-    // that Escape just closed, so skip it.
+    // here would re-detect the still-present `/` and instantly reopen it. We
-    if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) {
+    // read a ref set during keydown rather than `trigger`, because by keyup
+    // time React has re-rendered and `trigger` may already be null.
+    if (triggerKeyConsumedRef.current) {
+      triggerKeyConsumedRef.current = false
+
       return
     }
 

apps/desktop/src/app/chat/composer/slash-nav-dom-repro.test.tsx

@@ -4,18 +4,20 @@ import { useCallback, useEffect, useRef, useState } from 'react'
 import { describe, expect, it, vi } from 'vitest'
 
 import { useLiveCompletionAdapter } from './hooks/use-live-completion-adapter'
-import { detectTrigger, shouldSkipTriggerRefreshOnKeyUp, type TriggerState } from './text-utils'
+import { detectTrigger, type TriggerState } from './text-utils'
 
 // Faithful mirror of index.tsx's trigger wiring, driven through REAL DOM
-// keydown+keyup events on a contentEditable. This exercises the parts the
+// keydown+keyup events on a contentEditable. Exercises the parts a direct
-// older direct-call repro missed: the keyup -> refreshTrigger path, the
+// reducer-call repro misses: the keyup -> refreshTrigger path, the
-// `refreshTrigger` reset guard, and the per-press keydown+keyup ordering.
+// keydown-set "consumed" ref that guards it, and per-press keydown+keyup
+// ordering (critical for Escape, whose keydown nulls `trigger` before keyup).
 function Harness({
   onState
 }: {
   onState: (s: { active: number; items: readonly Unstable_TriggerItem[]; open: boolean }) => void
 }) {
   const editorRef = useRef<HTMLDivElement>(null)
+  const triggerKeyConsumedRef = useRef(false)
   const [trigger, setTrigger] = useState<TriggerState | null>(null)
   const [triggerActive, setTriggerActive] = useState(0)
   const [triggerItems, setTriggerItems] = useState<readonly Unstable_TriggerItem[]>([])
@@ -34,17 +36,22 @@ function Harness({
 
   const refreshTrigger = useCallback(() => {
     const editor = editorRef.current
-    if (!editor) return
+
+    if (!editor) {return}
     const raw = editor.textContent ?? ''
+
     if (!raw.includes('@') && !raw.includes('/')) {
       if (trigger) {
         setTrigger(null)
         setTriggerActive(0)
       }
+
       return
     }
+
     const detected = detectTrigger(raw)
     setTrigger(detected)
+
     if (detected?.kind !== trigger?.kind || detected?.query !== trigger?.query) {
       setTriggerActive(0)
     }
@@ -53,8 +60,10 @@ function Harness({
   useEffect(() => {
     if (!trigger || !triggerAdapter?.search) {
       setTriggerItems([])
+
       return
     }
+
     setTriggerItems(triggerAdapter.search(trigger.query))
   }, [trigger, triggerAdapter])
 
@@ -64,33 +73,51 @@ function Harness({
 
   onState({ active: triggerActive, items: triggerItems, open: trigger !== null })
 
-  // Exact copies of index.tsx handlers.
+  const closeTrigger = () => {
+    setTrigger(null)
+    setTriggerItems([])
+    setTriggerActive(0)
+  }
+
+  // Exact copies of index.tsx handlers, including the keydown-set "consumed"
+  // ref that the keyup consults.
   const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => {
     if (trigger && triggerItems.length > 0) {
       if (event.key === 'ArrowDown') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx + 1) % triggerItems.length)
+
         return
       }
+
       if (event.key === 'ArrowUp') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx - 1 + triggerItems.length) % triggerItems.length)
+
         return
       }
+
       if (event.key === 'Escape') {
         event.preventDefault()
-        setTrigger(null)
+        triggerKeyConsumedRef.current = true
-        setTriggerItems([])
+        closeTrigger()
-        setTriggerActive(0)
+
         return
       }
     }
   }
 
-  const handleKeyUp = (event: React.KeyboardEvent<HTMLDivElement>) => {
+  const handleKeyUp = () => {
-    if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) return
+    if (triggerKeyConsumedRef.current) {
-    // index.tsx uses setTimeout(refreshTrigger, 0); call synchronously here so
+      triggerKeyConsumedRef.current = false
-    // the test deterministically observes the keyup-driven refresh effect.
+
+      return
+    }
+
+    // index.tsx defers via setTimeout(refreshTrigger, 0); call synchronously
+    // here so the test deterministically observes the keyup-driven refresh.
     refreshTrigger()
   }
 
@@ -114,13 +141,13 @@ async function flush() {
 }
 
 describe('slash menu navigation — real DOM keydown+keyup', () => {
-  it('cycles through ALL items and Esc closes, even with keyup refresh firing', async () => {
+  it('cycles through ALL items and Esc closes (and stays closed)', async () => {
     vi.useRealTimers()
     let latest = { active: 0, items: [] as readonly Unstable_TriggerItem[], open: false }
     const { getByTestId } = render(<Harness onState={s => (latest = s)} />)
     const editor = getByTestId('editor')
 
-    // Simulate typing '/' : set text + fire input (mirrors the composer).
+    // Simulate typing '/'.
     await act(async () => {
       editor.textContent = '/'
       fireEvent.input(editor)
@@ -130,8 +157,9 @@ describe('slash menu navigation — real DOM keydown+keyup', () => {
     expect(latest.open).toBe(true)
     expect(latest.items.length).toBe(5)
 
-    // Press ArrowDown 6 times with the REAL keydown+keyup pair each time.
+    // ArrowDown 6x with REAL keydown+keyup pairs. Bug = stuck [0,1,0,1,...].
     const seen: number[] = [latest.active]
+
     for (let i = 0; i < 6; i++) {
       await act(async () => {
         fireEvent.keyDown(editor, { key: 'ArrowDown' })
@@ -141,15 +169,15 @@ describe('slash menu navigation — real DOM keydown+keyup', () => {
       seen.push(latest.active)
     }
 
-    // Bug = stuck oscillating [0,1,0,1,0,1,0]. Fixed = [0,1,2,3,4,0,1].
     expect(seen).toEqual([0, 1, 2, 3, 4, 0, 1])
 
-    // Escape must close and stay closed (keyup must not reopen it).
+    // Escape: keydown closes; keyup must NOT reopen (the '/' is still in text).
     await act(async () => {
       fireEvent.keyDown(editor, { key: 'Escape' })
       fireEvent.keyUp(editor, { key: 'Escape' })
       await Promise.resolve()
     })
+    await flush()
     expect(latest.open).toBe(false)
   })
 })

apps/desktop/src/app/chat/composer/text-utils.test.ts

@@ -1,30 +1,6 @@
 import { describe, expect, it } from 'vitest'
 
-import { detectTrigger, shouldSkipTriggerRefreshOnKeyUp } from './text-utils'
+import { detectTrigger } 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', () => {

apps/desktop/src/app/chat/composer/text-utils.ts

@@ -8,25 +8,6 @@ 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, shouldSkipTriggerRefreshOnKeyUp, textBeforeCaret, type TriggerState } from '@/app/chat/composer/text-utils'
+import { detectTrigger, 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'
@@ -873,6 +873,10 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
   const [trigger, setTrigger] = useState<TriggerState | null>(null)
   const [triggerActive, setTriggerActive] = useState(0)
   const [triggerItems, setTriggerItems] = useState<readonly Unstable_TriggerItem[]>([])
+  // See index.tsx: set in keydown when the open popover consumes a nav/control
+  // key so the matching keyup skips refreshTrigger (timing-immune vs reading
+  // `trigger`, which keyup sees as already-null after Escape).
+  const triggerKeyConsumedRef = useRef(false)
   const [triggerPlacement, setTriggerPlacement] = useState<'bottom' | 'top'>('top')
   const [focusRequestId, setFocusRequestId] = useState(0)
   const [submitting, setSubmitting] = useState(false)
@@ -1238,6 +1242,7 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
     if (trigger && triggerItems.length > 0) {
       if (event.key === 'ArrowDown') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx + 1) % triggerItems.length)
 
         return
@@ -1245,6 +1250,7 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
 
       if (event.key === 'ArrowUp') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         setTriggerActive(idx => (idx - 1 + triggerItems.length) % triggerItems.length)
 
         return
@@ -1252,6 +1258,7 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
 
       if (event.key === 'Enter' || event.key === 'Tab') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         const item = triggerItems[triggerActive]
 
         if (item) {
@@ -1263,6 +1270,7 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
 
       if (event.key === 'Escape') {
         event.preventDefault()
+        triggerKeyConsumedRef.current = true
         closeTrigger()
 
         return
@@ -1282,12 +1290,16 @@ const UserEditComposer: FC<UserEditComposerProps> = ({ cwd, gateway, sessionId }
     }
   }
 
-  const handleKeyUp = (event: KeyboardEvent<HTMLDivElement>) => {
+  const handleKeyUp = () => {
-    // Arrow/Enter/Tab/Escape while the trigger menu is open are fully handled
+    // If this keyup belongs to a key the open trigger popover already consumed
-    // in keydown and never edit text. Refreshing the trigger here would reset
+    // in keydown (Arrow/Enter/Tab/Escape), skip the refresh. Those keys never
-    // the highlight to the top (breaking ArrowDown/ArrowUp) and re-open a menu
+    // edit text, and for Escape the keydown already closed the menu — a refresh
-    // that Escape just closed, so skip it.
+    // here would re-detect the still-present `/` and instantly reopen it. We
-    if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) {
+    // read a ref set during keydown rather than `trigger`, because by keyup
+    // time React has re-rendered and `trigger` may already be null.
+    if (triggerKeyConsumedRef.current) {
+      triggerKeyConsumedRef.current = false
+
       return
     }