From e3acdfb21da9a06f9b326f001563cf41641787c4 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 17:50:12 -0500 Subject: [PATCH 1/2] refactor(desktop): lift the completion-accept decision out of the keydown ladder Which keys accept the highlighted completion was an inline condition in the composer's keydown god-function, untestable without a DOM harness. Move it to a pure helper beside the other slash-query utilities. --- .../app/chat/composer/composer-utils.test.ts | 51 ++++++++++++++++++- .../src/app/chat/composer/composer-utils.ts | 44 ++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/app/chat/composer/composer-utils.test.ts b/apps/desktop/src/app/chat/composer/composer-utils.test.ts index 4df8463ba2d..062b2567738 100644 --- a/apps/desktop/src/app/chat/composer/composer-utils.test.ts +++ b/apps/desktop/src/app/chat/composer/composer-utils.test.ts @@ -2,12 +2,14 @@ import type { Unstable_TriggerItem } from '@assistant-ui/core' import { describe, expect, it } from 'vitest' import { + acceptsTriggerCompletion, isPendingDraftPersistCurrent, type PendingDraftPersist, pickPlaceholder, slashArgStage, slashChipKindForItem, - slashCommandToken + slashCommandToken, + type TriggerAcceptInput } from './composer-utils' const item = (group: string): Unstable_TriggerItem => @@ -39,6 +41,53 @@ describe('slashChipKindForItem', () => { }) }) +describe('acceptsTriggerCompletion', () => { + const press = (key: string, overrides: Partial = {}) => + acceptsTriggerCompletion({ + activeExplicit: false, + freeTextArgStage: false, + key, + kind: '/', + query: 'personality alic', + ...overrides + }) + + it('accepts on Enter / Tab / Space for a finite option list', () => { + expect(press('Enter')).toBe(true) + expect(press('Tab')).toBe(true) + expect(press(' ')).toBe(true) + }) + + it('ignores keys that are neither navigation nor acceptance', () => { + expect(press('a')).toBe(false) + expect(press('Escape')).toBe(false) + }) + + it('lets an `@` mention take a literal space', () => { + expect(press(' ', { kind: '@', query: 'src/comp' })).toBe(false) + expect(press('Enter', { kind: '@', query: 'src/comp' })).toBe(true) + }) + + it('types a space on a bare `/ ` instead of accepting', () => { + expect(press(' ', { query: '' })).toBe(false) + }) + + // The `/goal ` class: the popover may be live over free-form text, so + // the keys that mean something else in prose must keep meaning it. + it('sends the prose rather than the unchosen first row', () => { + expect(press('Enter', { freeTextArgStage: true, query: 'goal ship the redesign' })).toBe(false) + expect(press(' ', { freeTextArgStage: true, query: 'goal ship the' })).toBe(false) + }) + + it('accepts on Enter once the user has arrowed to a row deliberately', () => { + expect(press('Enter', { activeExplicit: true, freeTextArgStage: true, query: 'goal stat' })).toBe(true) + }) + + it('keeps Tab as the explicit accept even over free text', () => { + expect(press('Tab', { freeTextArgStage: true, query: 'goal stat' })).toBe(true) + }) +}) + describe('pickPlaceholder', () => { it('returns a member of the pool', () => { const pool = ['a', 'b', 'c'] as const diff --git a/apps/desktop/src/app/chat/composer/composer-utils.ts b/apps/desktop/src/app/chat/composer/composer-utils.ts index 547a210f06f..21e3c1ac4a1 100644 --- a/apps/desktop/src/app/chat/composer/composer-utils.ts +++ b/apps/desktop/src/app/chat/composer/composer-utils.ts @@ -4,6 +4,8 @@ import type { SlashChipKind } from '@/components/assistant-ui/directive-text' import type { ComposerAttachment } from '@/store/composer' import { setSessionPickerOpen } from '@/store/session' +import type { TriggerState } from './text-utils' + export const COMPOSER_STACK_BREAKPOINT_PX = 320 // Above the stack breakpoint but still cramped: the model pill sheds its label @@ -59,6 +61,48 @@ export const slashArgStage = (query: string) => query.includes(' ') /** The `/command` token of a slash query (`personality x` → `/personality`). */ export const slashCommandToken = (query: string) => `/${query.split(/\s+/, 1)[0]?.toLowerCase() ?? ''}` +export interface TriggerAcceptInput { + /** The user moved the highlight themselves (arrow keys) rather than + * inheriting the list's default first row. */ + activeExplicit: boolean + /** The trigger is a slash command whose argument is arbitrary prose. */ + freeTextArgStage: boolean + key: string + kind: TriggerState['kind'] + query: string +} + +/** + * Whether a keypress accepts the highlighted completion while the popover is + * open. Tab is always an accept — it has no other meaning in the composer. + * + * Enter and Space are conditional, because both mean something else while a + * free-text argument is being written (`/goal ship the redesign`). Space types + * a space, and Enter sends the message; letting either take the popover's + * pre-highlighted row would swap the prose the user is mid-sentence on for a + * subcommand they never chose. Enter still accepts once the user has arrowed + * to a row deliberately, so the highlight never lies about what Enter will do. + */ +export function acceptsTriggerCompletion({ + activeExplicit, + freeTextArgStage, + key, + kind, + query +}: TriggerAcceptInput): boolean { + if (key === 'Tab') { + return true + } + + if (key === 'Enter') { + return !freeTextArgStage || activeExplicit + } + + // Space is slash-only (an `@` mention takes a literal space) and gated to a + // non-empty query so a bare `/ ` still types a space. + return key === ' ' && kind === '/' && Boolean(query.trim()) && !freeTextArgStage +} + export interface QueueEditState { attachments: ComposerAttachment[] draft: string From 43571601aa71fbd30e839f567e540106a92d8a03 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 17:50:18 -0500 Subject: [PATCH 2/2] fix(desktop): don't let Enter swap a free-text slash argument for a completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/goal` keeps its completion popover open across arbitrary prose so its subcommands stay reachable. The popover highlights its first row on open, and Enter accepted that highlight unconditionally — so pressing Enter to send `/goal ship the redesign` would replace the sentence with a row the user never chose. Space was already guarded; Enter and Tab were not. Enter now accepts only after the user has arrowed to a row deliberately, so the highlight never lies about what Enter will do. Tab stays an unconditional accept, since it means nothing else in the composer. This is latent rather than reproducible today: `/goal` is absent from `SUBCOMMANDS` (its `args_hint` pipes are spaced, so the extraction regex misses them), so the backend returns no arg completions and the branch never runs. Giving `/goal` the subcommands it already advertises would resurrect the #71963 symptom in a worse form — losing the prose instead of chipping it. --- .../hooks/use-composer-trigger.test.ts | 24 ++++++++++++++ .../composer/hooks/use-composer-trigger.ts | 26 ++++++++++++--- apps/desktop/src/app/chat/composer/index.tsx | 32 ++++++++++++------- 3 files changed, 66 insertions(+), 16 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts index c7a4e2f138a..a4ab38b3b3a 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts @@ -148,6 +148,30 @@ describe('useComposerTrigger — free-text slash arguments', () => { expect(editor.querySelector('[data-slash-kind]')).toBeNull() }) + it('treats the default highlight as a suggestion until the user arrows to a row', () => { + const editor = mountEditor('/goal stat') + const { hook } = mountTrigger(editor, [item('/goal status', 'Options')]) + + act(() => hook.result.current.refreshTrigger()) + expect(hook.result.current.triggerActiveExplicit).toBe(false) + + act(() => hook.result.current.moveTriggerActive(1)) + expect(hook.result.current.triggerActiveExplicit).toBe(true) + }) + + it('drops a deliberate selection once the query moves on', () => { + const editor = mountEditor('/goal stat') + const { hook } = mountTrigger(editor, [item('/goal status', 'Options')]) + + act(() => hook.result.current.refreshTrigger()) + act(() => hook.result.current.moveTriggerActive(1)) + + renderComposerContents(editor, '/goal start the migration') + act(() => hook.result.current.refreshTrigger()) + + expect(hook.result.current.triggerActiveExplicit).toBe(false) + }) + it('still commits a fully typed finite option as one directive chip', () => { const editor = mountEditor('/personality creative') const { hook } = mountTrigger(editor, []) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts index 5f0be41c80c..65f09225f65 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts @@ -53,6 +53,11 @@ export function useComposerTrigger({ }: UseComposerTriggerOptions) { const [trigger, setTrigger] = useState(null) const [triggerActive, setTriggerActive] = useState(0) + // The list highlights its first row on open, which is a suggestion rather + // than a choice. This records that the user moved the highlight themselves, + // which is what lets Enter accept a completion in a free-text argument stage + // without stealing prose from everyone who never touched the arrows. + const [triggerActiveExplicit, setTriggerActiveExplicit] = useState(false) 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 @@ -63,6 +68,11 @@ export function useComposerTrigger({ // re-rendered and the handler closure sees the post-keydown state. const triggerKeyConsumedRef = useRef(false) + const resetTriggerActive = useCallback(() => { + setTriggerActive(0) + setTriggerActiveExplicit(false) + }, []) + const refreshTrigger = useCallback(() => { const editor = editorRef.current @@ -80,7 +90,7 @@ export function useComposerTrigger({ if (!rawText.includes('@') && !rawText.includes('/')) { if (trigger) { setTrigger(null) - setTriggerActive(0) + resetTriggerActive() } return @@ -109,9 +119,9 @@ export function useComposerTrigger({ // 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) + resetTriggerActive() } - }, [editorRef, trigger]) + }, [editorRef, resetTriggerActive, trigger]) const triggerAdapter: Unstable_TriggerAdapter | null = trigger?.kind === '@' ? at.adapter : trigger?.kind === '/' ? slash.adapter : null @@ -147,7 +157,13 @@ export function useComposerTrigger({ const closeTrigger = () => { setTrigger(null) setTriggerItems([]) - setTriggerActive(0) + resetTriggerActive() + } + + /** Step the highlight, marking it as the user's own deliberate pick. */ + const moveTriggerActive = (delta: number) => { + setTriggerActiveExplicit(true) + setTriggerActive(idx => (idx + delta + triggerItems.length) % triggerItems.length) } useEffect(() => { @@ -358,12 +374,14 @@ export function useComposerTrigger({ ascendTriggerPath, closeTrigger, commitTypedSlashDirective, + moveTriggerActive, refreshTrigger, replaceTriggerWithChip, setTriggerActive, slashFreeTextArgStage, trigger, triggerActive, + triggerActiveExplicit, triggerItems, triggerKeyConsumedRef, triggerLoading diff --git a/apps/desktop/src/app/chat/composer/index.tsx b/apps/desktop/src/app/chat/composer/index.tsx index 6ce8289fa0a..ac4f5dd3027 100644 --- a/apps/desktop/src/app/chat/composer/index.tsx +++ b/apps/desktop/src/app/chat/composer/index.tsx @@ -22,7 +22,12 @@ import { $autoSpeakReplies } from '@/store/voice-prefs' import { useTheme } from '@/themes' import { AttachmentList } from './attachments' -import { COMPOSER_FADE_BACKGROUND, type QueueEditState, slashArgStage } from './composer-utils' +import { + acceptsTriggerCompletion, + COMPOSER_FADE_BACKGROUND, + type QueueEditState, + slashArgStage +} from './composer-utils' import { ContextMenu } from './context-menu' import { COMPOSER_AREAS, runComposerMiddleware } from './contrib' import { ComposerControls } from './controls' @@ -302,12 +307,14 @@ export function ChatBar({ ascendTriggerPath, closeTrigger, commitTypedSlashDirective, + moveTriggerActive, refreshTrigger, replaceTriggerWithChip, setTriggerActive, slashFreeTextArgStage, trigger, triggerActive, + triggerActiveExplicit, triggerItems, triggerKeyConsumedRef, triggerLoading @@ -528,7 +535,7 @@ export function ChatBar({ if (event.key === 'ArrowDown') { event.preventDefault() triggerKeyConsumedRef.current = true - setTriggerActive(idx => (idx + 1) % triggerItems.length) + moveTriggerActive(1) return } @@ -536,20 +543,21 @@ export function ChatBar({ if (event.key === 'ArrowUp') { event.preventDefault() triggerKeyConsumedRef.current = true - setTriggerActive(idx => (idx - 1 + triggerItems.length) % triggerItems.length) + moveTriggerActive(-1) return } - // Enter / Tab / Space all accept the highlighted item: a no-arg command - // commits its directive chip, an arg-taking command expands to its - // options step, and an arg option commits the full `/cmd arg` chip. Space - // is slash-only (an `@` mention takes a literal space) and gated to a - // non-empty query so a bare `/ ` still types a space. - const acceptOnSpace = - event.key === ' ' && trigger.kind === '/' && Boolean(trigger.query.trim()) && !slashFreeTextArgStage - - const accept = event.key === 'Enter' || event.key === 'Tab' || acceptOnSpace + // Accepting the highlighted item: a no-arg command commits its directive + // chip, an arg-taking command expands to its options step, and an arg + // option commits the full `/cmd arg` chip. + const accept = acceptsTriggerCompletion({ + activeExplicit: triggerActiveExplicit, + freeTextArgStage: slashFreeTextArgStage, + key: event.key, + kind: trigger.kind, + query: trigger.query + }) if (accept) { event.preventDefault()