openclaw/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

Merge pull request #37999 from kshitijk4poor/desktop-slash-nav-dom-regression-test

Browse Source 
fix(desktop): slash/@ menu keyboard nav — cycle all items + Esc dismiss
This commit is contained in:
kshitij
2026-06-03 00:51:54 -07:00
committed by GitHub
parent e114b31eda 49f1b9e4b4
commit 46ea0a184d
5 changed files with 226 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
apps/desktop/src/app/chat/composer/index.tsx
View File

@ -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>) => {
// 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
}

183
apps/desktop/src/app/chat/composer/slash-nav-dom-repro.test.tsx Normal file
View File

@ -0,0 +1,183 @@
import type { Unstable_TriggerAdapter, Unstable_TriggerItem } from '@assistant-ui/core'
import { act, fireEvent, render } from '@testing-library/react'
import { useCallback, useEffect, useRef, useState } from 'react'
import { describe, expect, it, vi } from 'vitest'
import { useLiveCompletionAdapter } from './hooks/use-live-completion-adapter'
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. 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<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[]>([])
const { adapter } = useLiveCompletionAdapter({
enabled: true,
debounceMs: 0,
fetcher: async (query: string) => ({
query,
items: Array.from({ length: 5 }, (_, i) => ({ text: `/cmd${i}`, display: `/cmd${i}`, meta: '' }))
}),
toItem: (entry, index) => ({ id: `${entry.text}|${index}`, type: 'slash', label: entry.text.slice(1) })
})
const triggerAdapter: Unstable_TriggerAdapter | null = trigger?.kind === '/' ? adapter : null
const refreshTrigger = useCallback(() => {
const editor = editorRef.current
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)
}
}, [trigger])
useEffect(() => {
if (!trigger || !triggerAdapter?.search) {
setTriggerItems([])
return
}
setTriggerItems(triggerAdapter.search(trigger.query))
}, [trigger, triggerAdapter])
useEffect(() => {
setTriggerActive(idx => Math.min(idx, Math.max(0, triggerItems.length - 1)))
}, [triggerItems.length])
onState({ active: triggerActive, items: triggerItems, open: trigger !== null })
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()
triggerKeyConsumedRef.current = true
closeTrigger()
return
}
}
}
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()
}
return (
<div
contentEditable
data-testid="editor"
onInput={() => refreshTrigger()}
onKeyDown={handleKeyDown}
onKeyUp={handleKeyUp}
ref={editorRef}
suppressContentEditableWarning
/>
)
}
async function flush() {
await act(async () => {
await new Promise(r => setTimeout(r, 20))
})
}
describe('slash menu navigation — real DOM keydown+keyup', () => {
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 '/'.
await act(async () => {
editor.textContent = '/'
fireEvent.input(editor)
})
await flush()
expect(latest.open).toBe(true)
expect(latest.items.length).toBe(5)
// 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' })
fireEvent.keyUp(editor, { key: 'ArrowDown' })
await Promise.resolve()
})
seen.push(latest.active)
}
expect(seen).toEqual([0, 1, 2, 3, 4, 0, 1])
// 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)
})
})

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

@ -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', () => {

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

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

26
apps/desktop/src/components/assistant-ui/thread.tsx
View File

@ -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>) => {
// 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
}