fix(tui): don't make Enter swallow trailing-space-only slash completions (#48425)
* fix(tui): don't make Enter swallow trailing-space-only slash completions
Submitting a slash command in the TUI took three Enter presses: one to
complete the name (/ex → /exit), a second that only appended the trailing
space the gateway adds to keep the classic-CLI prompt_toolkit dropdown open
(/exit → "/exit "), and a third to actually submit.
The composer's submit handler accepted the highlighted completion whenever
applying it changed the input at all, so the whitespace-only delta ate an
extra keypress. Treat a completion whose only change is trailing whitespace
on an already-complete token as "already complete" and fall through to
submit. Partial-name and argument completions (a real token change) still
accept on Enter as before.
The replace/accept logic is extracted into pure helpers (applyCompletion,
completionToApplyOnSubmit) in domain/slash.ts.
* test(tui): cover Enter/completion trailing-space behavior and isolate poller queue
- completionApply.test.ts asserts completionToApplyOnSubmit accepts real
token completions (partial command name, argument) but returns null for a
trailing-space-only delta on an already-complete command, so Enter submits
instead of needing extra presses.
- test_notification_poller_delivers_completion / _skips_consumed previously
shared the process-global process_registry.completion_queue. Their events
carry no session_key, so a leaked/concurrent poller could dequeue and
dispatch them to a fixture agent without run_conversation, flaking CI
("AttributeError: '_FakeAgent' object has no attribute 'run_conversation'").
Isolate the queue per test (fresh queue.Queue via monkeypatch), matching the
sibling poller tests that already do this.
This commit is contained in:
parent
25c590ccd0
commit
58ad6942d9
@ -6890,6 +6890,8 @@ def test_config_show_displays_nested_max_turns(monkeypatch):
|
||||
|
||||
def test_notification_poller_delivers_completion(monkeypatch):
|
||||
"""Poller picks up completion events and triggers agent turns."""
|
||||
import queue as _queue_mod
|
||||
|
||||
from tools.process_registry import process_registry
|
||||
|
||||
turns = []
|
||||
@ -6916,16 +6918,23 @@ def test_notification_poller_delivers_completion(monkeypatch):
|
||||
monkeypatch.setattr(server, "make_stream_renderer", lambda cols: None)
|
||||
monkeypatch.setattr(server, "render_message", lambda raw, cols: None)
|
||||
|
||||
# Clear queue
|
||||
while not process_registry.completion_queue.empty():
|
||||
process_registry.completion_queue.get_nowait()
|
||||
# Isolate the completion queue for the duration of this test. The poller
|
||||
# reads process_registry.completion_queue by attribute at runtime; the
|
||||
# event below carries no session_key, so any *other* poller (a leaked
|
||||
# daemon thread from another test, or a concurrent one in the same xdist
|
||||
# worker) is allowed to dequeue and dispatch it to its own session — whose
|
||||
# agent may be a fixture double without run_conversation. A fresh Queue
|
||||
# here fully isolates this test; monkeypatch restores the original on
|
||||
# teardown. (Same pattern as test_notification_poller_requeues_when_busy.)
|
||||
isolated_queue: _queue_mod.Queue = _queue_mod.Queue()
|
||||
monkeypatch.setattr(process_registry, "completion_queue", isolated_queue)
|
||||
process_registry._completion_consumed.discard("proc_poller_test")
|
||||
|
||||
stop = threading.Event()
|
||||
|
||||
# Put event on queue, then immediately signal stop so the poller
|
||||
# runs exactly one iteration.
|
||||
process_registry.completion_queue.put({
|
||||
isolated_queue.put({
|
||||
"type": "completion",
|
||||
"session_id": "proc_poller_test",
|
||||
"command": "echo hello",
|
||||
@ -6953,6 +6962,8 @@ def test_notification_poller_delivers_completion(monkeypatch):
|
||||
|
||||
def test_notification_poller_skips_consumed(monkeypatch):
|
||||
"""Already-consumed completions are not dispatched by the poller."""
|
||||
import queue as _queue_mod
|
||||
|
||||
from tools.process_registry import process_registry
|
||||
|
||||
turns = []
|
||||
@ -6975,11 +6986,15 @@ def test_notification_poller_skips_consumed(monkeypatch):
|
||||
monkeypatch.setattr(server, "make_stream_renderer", lambda cols: None)
|
||||
monkeypatch.setattr(server, "render_message", lambda raw, cols: None)
|
||||
|
||||
while not process_registry.completion_queue.empty():
|
||||
process_registry.completion_queue.get_nowait()
|
||||
# Isolate the completion queue so a concurrent/leaked poller in the same
|
||||
# xdist worker can't dequeue this session_key-less event before our poller
|
||||
# does. monkeypatch restores the shared singleton on teardown. (Same
|
||||
# pattern as test_notification_poller_requeues_when_busy.)
|
||||
isolated_queue: _queue_mod.Queue = _queue_mod.Queue()
|
||||
monkeypatch.setattr(process_registry, "completion_queue", isolated_queue)
|
||||
|
||||
process_registry._completion_consumed.add("proc_already_done")
|
||||
process_registry.completion_queue.put({
|
||||
isolated_queue.put({
|
||||
"type": "completion",
|
||||
"session_id": "proc_already_done",
|
||||
"command": "echo x",
|
||||
|
||||
51
ui-tui/src/__tests__/completionApply.test.ts
Normal file
51
ui-tui/src/__tests__/completionApply.test.ts
Normal file
@ -0,0 +1,51 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { applyCompletion, completionToApplyOnSubmit } from '../domain/slash.js'
|
||||
|
||||
describe('applyCompletion', () => {
|
||||
it('replaces from compReplace and drops the leading slash from the row', () => {
|
||||
// The gateway's slash completer returns bare command names with
|
||||
// replace_from = 1 (after the leading "/").
|
||||
expect(applyCompletion('/ex', 'exit', 1)).toBe('/exit')
|
||||
})
|
||||
|
||||
it('keeps the leading slash when the row carries one and input does not', () => {
|
||||
expect(applyCompletion('ex', '/exit', 0)).toBe('/exit')
|
||||
})
|
||||
|
||||
it('replaces an argument token after a space (subcommand completion)', () => {
|
||||
expect(applyCompletion('/cron ad', 'add', 6)).toBe('/cron add')
|
||||
})
|
||||
})
|
||||
|
||||
describe('completionToApplyOnSubmit', () => {
|
||||
it('accepts a completion that finishes a partial command name', () => {
|
||||
// "/ex" -> "/exit": a real token change, so Enter accepts it.
|
||||
expect(completionToApplyOnSubmit('/ex', 'exit', 1)).toBe('/exit')
|
||||
})
|
||||
|
||||
it('does NOT swallow Enter when the completion only adds a trailing space', () => {
|
||||
// This is the bug: once "/exit" is fully typed, the gateway returns the
|
||||
// command with a trailing space ("exit ") so the classic-CLI dropdown
|
||||
// stays open. In the TUI that must NOT eat the Enter — the command is
|
||||
// already complete, so Enter should submit.
|
||||
expect(completionToApplyOnSubmit('/exit', 'exit ', 1)).toBeNull()
|
||||
})
|
||||
|
||||
it('does not swallow Enter when applying the row is a no-op', () => {
|
||||
expect(completionToApplyOnSubmit('/exit', 'exit', 1)).toBeNull()
|
||||
})
|
||||
|
||||
it('still accepts a real argument completion (no trailing-space false positive)', () => {
|
||||
expect(completionToApplyOnSubmit('/cron ad', 'add', 6)).toBe('/cron add')
|
||||
})
|
||||
|
||||
it('submits (no accept) once an argument is fully typed and only a space is added', () => {
|
||||
expect(completionToApplyOnSubmit('/cron add', 'add ', 6)).toBeNull()
|
||||
})
|
||||
|
||||
it('returns null when there is no row text', () => {
|
||||
expect(completionToApplyOnSubmit('/exit', undefined, 1)).toBeNull()
|
||||
expect(completionToApplyOnSubmit('/exit', '', 1)).toBeNull()
|
||||
})
|
||||
})
|
||||
@ -2,7 +2,7 @@ import { type MutableRefObject, useCallback, useEffect, useRef } from 'react'
|
||||
|
||||
import { TYPING_IDLE_MS } from '../config/timing.js'
|
||||
import { attachedImageNotice } from '../domain/messages.js'
|
||||
import { looksLikeSlashCommand } from '../domain/slash.js'
|
||||
import { completionToApplyOnSubmit, looksLikeSlashCommand } from '../domain/slash.js'
|
||||
import type { GatewayClient } from '../gatewayClient.js'
|
||||
import type {
|
||||
InputDetectDropResponse,
|
||||
@ -354,14 +354,10 @@ export function useSubmission(opts: UseSubmissionOptions) {
|
||||
(value: string) => {
|
||||
if (composerState.completions.length) {
|
||||
const row = composerState.completions[composerState.compIdx]
|
||||
const next = completionToApplyOnSubmit(value, row?.text, composerState.compReplace)
|
||||
|
||||
if (row?.text) {
|
||||
const text = value.startsWith('/') && row.text.startsWith('/') ? row.text.slice(1) : row.text
|
||||
const next = value.slice(0, composerState.compReplace) + text
|
||||
|
||||
if (next !== value) {
|
||||
return composerActions.setInput(next)
|
||||
}
|
||||
if (next !== null) {
|
||||
return composerActions.setInput(next)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -8,3 +8,43 @@ export const parseSlashCommand = (cmd: string) => {
|
||||
|
||||
return { arg: rest.join(' '), cmd, name: name.toLowerCase() }
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply a completion row to the current input, mirroring the editor's
|
||||
* replace semantics: replace from `compReplace` with the row text, dropping
|
||||
* the leading slash when both the input and the row carry one (the gateway's
|
||||
* slash completer returns bare command names whose replace span begins after
|
||||
* the leading `/`).
|
||||
*/
|
||||
export const applyCompletion = (value: string, rowText: string, compReplace: number): string => {
|
||||
const text = value.startsWith('/') && rowText.startsWith('/') ? rowText.slice(1) : rowText
|
||||
|
||||
return value.slice(0, compReplace) + text
|
||||
}
|
||||
|
||||
/**
|
||||
* Decide what Enter does when a completion is highlighted: returns the value
|
||||
* to set (accept the completion) or `null` to fall through to submit.
|
||||
*
|
||||
* Enter accepts a completion only when it changes the command/argument token.
|
||||
* A completion that merely appends trailing whitespace to an already-complete
|
||||
* command (e.g. `/exit` → `/exit `, the trailing space the gateway adds so the
|
||||
* classic CLI's prompt_toolkit dropdown stays open) must NOT swallow the Enter
|
||||
* — otherwise every slash command needs an extra keypress: type → Enter
|
||||
* completes the name → Enter adds the space → Enter finally submits. Treating a
|
||||
* whitespace-only delta as "already complete" collapses that back to the
|
||||
* expected one/two presses.
|
||||
*/
|
||||
export const completionToApplyOnSubmit = (
|
||||
value: string,
|
||||
rowText: string | undefined,
|
||||
compReplace: number
|
||||
): string | null => {
|
||||
if (!rowText) {
|
||||
return null
|
||||
}
|
||||
|
||||
const next = applyCompletion(value, rowText, compReplace)
|
||||
|
||||
return next !== value && next.trimEnd() !== value.trimEnd() ? next : null
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user