From 49f1b9e4b44f3affb33ac2d8810896d89fbbdf9f Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 3 Jun 2026 13:15:08 +0530 Subject: [PATCH] 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 | 30 +++++++-- .../composer/slash-nav-dom-repro.test.tsx | 64 +++++++++++++------ .../src/app/chat/composer/text-utils.test.ts | 26 +------- .../src/app/chat/composer/text-utils.ts | 19 ------ .../src/components/assistant-ui/thread.tsx | 26 ++++++-- 5 files changed, 89 insertions(+), 76 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/index.tsx b/apps/desktop/src/app/chat/composer/index.tsx index 37360f39f..aae4827ed 100644 --- a/apps/desktop/src/app/chat/composer/index.tsx +++ b/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(null) const [triggerActive, setTriggerActive] = useState(0) const [triggerItems, setTriggerItems] = useState([]) + // 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) => { - // 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)) { + const handleEditorKeyUp = () => { + // If this keyup belongs to a key the open trigger popover already consumed + // in keydown (Arrow/Enter/Tab/Escape), skip the refresh. Those keys never + // edit text, and for Escape the keydown already closed the menu — a refresh + // here would re-detect the still-present `/` and instantly reopen it. We + // 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 } diff --git a/apps/desktop/src/app/chat/composer/slash-nav-dom-repro.test.tsx b/apps/desktop/src/app/chat/composer/slash-nav-dom-repro.test.tsx index 0eac9d731..8835d4275 100644 --- a/apps/desktop/src/app/chat/composer/slash-nav-dom-repro.test.tsx +++ b/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 -// older direct-call repro missed: the keyup -> refreshTrigger path, the -// `refreshTrigger` reset guard, and the per-press keydown+keyup ordering. +// keydown+keyup events on a contentEditable. Exercises the parts a direct +// reducer-call repro misses: the keyup -> refreshTrigger path, the +// 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(null) + const triggerKeyConsumedRef = useRef(false) const [trigger, setTrigger] = useState(null) const [triggerActive, setTriggerActive] = useState(0) const [triggerItems, setTriggerItems] = useState([]) @@ -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) => { 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) - setTriggerItems([]) - setTriggerActive(0) + triggerKeyConsumedRef.current = true + closeTrigger() + return } } } - const handleKeyUp = (event: React.KeyboardEvent) => { - if (shouldSkipTriggerRefreshOnKeyUp(event.key, trigger !== null)) return - // index.tsx uses setTimeout(refreshTrigger, 0); call synchronously here so - // the test deterministically observes the keyup-driven refresh effect. + const handleKeyUp = () => { + if (triggerKeyConsumedRef.current) { + triggerKeyConsumedRef.current = false + + 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( (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) }) }) diff --git a/apps/desktop/src/app/chat/composer/text-utils.test.ts b/apps/desktop/src/app/chat/composer/text-utils.test.ts index 21b433bf2..998dc8b33 100644 --- a/apps/desktop/src/app/chat/composer/text-utils.test.ts +++ b/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' - -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) - } - }) -}) +import { detectTrigger } from './text-utils' describe('detectTrigger', () => { it('detects a bare slash trigger with an empty query', () => { diff --git a/apps/desktop/src/app/chat/composer/text-utils.ts b/apps/desktop/src/app/chat/composer/text-utils.ts index db0dab977..5725883d8 100644 --- a/apps/desktop/src/app/chat/composer/text-utils.ts +++ b/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 = 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 0eede6204..1576ab9b5 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, 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 = ({ cwd, gateway, sessionId } const [trigger, setTrigger] = useState(null) const [triggerActive, setTriggerActive] = useState(0) const [triggerItems, setTriggerItems] = useState([]) + // 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 = ({ 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 = ({ 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 = ({ 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 = ({ cwd, gateway, sessionId } if (event.key === 'Escape') { event.preventDefault() + triggerKeyConsumedRef.current = true closeTrigger() return @@ -1282,12 +1290,16 @@ 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)) { + const handleKeyUp = () => { + // If this keyup belongs to a key the open trigger popover already consumed + // in keydown (Arrow/Enter/Tab/Escape), skip the refresh. Those keys never + // edit text, and for Escape the keydown already closed the menu — a refresh + // here would re-detect the still-present `/` and instantly reopen it. We + // 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 }