Merge pull request #72956 from NousResearch/bb/slash-freetext-enter

fix(desktop): don't let Enter swap a free-text slash argument for a completion

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<TriggerAcceptInput> = {}) =>
+    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 <prose>` 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

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

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, [])

apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts

@@ -53,6 +53,11 @@ export function useComposerTrigger({
 }: UseComposerTriggerOptions) {
   const [trigger, setTrigger] = useState<TriggerState | null>(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<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
@@ -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
@@ -149,7 +159,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(() => {
@@ -360,12 +376,14 @@ export function useComposerTrigger({
     ascendTriggerPath,
     closeTrigger,
     commitTypedSlashDirective,
+    moveTriggerActive,
     refreshTrigger,
     replaceTriggerWithChip,
     setTriggerActive,
     slashFreeTextArgStage,
     trigger,
     triggerActive,
+    triggerActiveExplicit,
     triggerItems,
     triggerKeyConsumedRef,
     triggerLoading

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