diff --git a/src/tui/prompt-slash-exit.test.ts b/src/tui/prompt-slash-exit.test.ts index 8b0921ba0..5538a4f65 100644 --- a/src/tui/prompt-slash-exit.test.ts +++ b/src/tui/prompt-slash-exit.test.ts @@ -160,29 +160,38 @@ describe("slash command popup", () => { }) }) - test("an unmatched name prefix closes the popup and keeps the typed text", async () => { + test("an unmatched name prefix refreshes in place instead of closing", async () => { await withShell(async ({ shell, press, render, frame }) => { press("/") press("z") await render() - expect(isSlashPopupOpen(shell)).toBe(false) - expect(shell.overlayList).toBeNull() + // The popup was already open (from "/") when the filter zeroed out — + // closing here would release the host, which is exactly the gap a + // queued gate can drain into mid-filter. It stays owned and shows the + // same "(no matches)" row the general palette uses. + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.overlayList).not.toBeNull() expect(shell.prompt.value).toBe("/z") - expect(shell.overlayItems).not.toContain("(no matches)") - expect(frame()).not.toContain("(no matches)") + expect(shell.overlayItems).toEqual(["(no matches)"]) + expect(frame()).toContain("(no matches)") + + // A backspace that restores a match refreshes back in place. + press("Backspace") + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.paletteCommands.map((c) => c.id)).toEqual(CATALOG.map((c) => c.id)) }) }) - test("description prose does not keep the slash list open", async () => { + test("description prose keeps the slash list open with no matches", async () => { await withShell(async ({ shell, press, render, frame }) => { press("/") press("p") await render() - expect(isSlashPopupOpen(shell)).toBe(false) - expect(shell.overlayList).toBeNull() + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.overlayList).not.toBeNull() expect(shell.prompt.value).toBe("/p") - expect(shell.overlayItems).not.toContain("(no matches)") - expect(frame()).not.toContain("(no matches)") + expect(shell.overlayItems).toEqual(["(no matches)"]) + expect(frame()).toContain("(no matches)") }) }) }) diff --git a/src/tui/shell.ts b/src/tui/shell.ts index cbe8e1451..5d736789d 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -1309,6 +1309,32 @@ function overlayRowsPerItem(kind: PrimaryOverlayKind | null): number { return isDecisionOverlay(kind) ? DECISION_CHOICE_ROWS : 1 } +/** + * Recompute the overlay host's row budget from the current item count and + * relayout into it. Callers that refresh an already-open overlay's items in + * place (rather than reopening) must call this themselves — a filter that + * narrows a list and then widens it again would otherwise stay pinned at + * whatever size it first opened at. + */ +function relayoutOverlayHost(shell: AppShell, itemCount: number): void { + const perItem = overlayRowsPerItem(shell.overlayKind) + const hostRows = overlayHostRows( + shell, + shell.overlayBodyLines.length, + itemCount * perItem, + ) + const minHostRows = overlayMinHostRows( + shell, + shell.overlayBodyLines.length, + itemCount > 0, + ) + relayout(shell, { + overlayMode: "inset", + overlayBodyRows: hostRows, + overlayMinBodyRows: minHostRows, + }) +} + /** Columns a body/choice row may paint into, inside border and leading space. */ function overlayRowWidth(shell: AppShell): number { return Math.max(8, Math.max(20, shell.layout.contentWidth) - 4) @@ -3690,20 +3716,9 @@ export function openListOverlay( // against OVERLAY_MAX_FRACTION and the transcript floor, and applyLayout // shrinks the viewport to whatever survived — so a longer list scrolls // instead of growing, and a short one leaves no dead rows below it. - const perItem = overlayRowsPerItem(shell.overlayKind) // An empty list charges no rows: a chooser with nothing to choose must not // reserve a blank band the operator can neither read nor act on. const listItems = labels.length - const hostRows = overlayHostRows( - shell, - shell.overlayBodyLines.length, - listItems * perItem, - ) - const minHostRows = overlayMinHostRows( - shell, - shell.overlayBodyLines.length, - listItems > 0, - ) shell.overlayList = createListViewport({ count: labels.length, @@ -3720,11 +3735,7 @@ export function openListOverlay( target: focusTarget, scrollOwner: isPalette ? "palette" : "overlay", }) - relayout(shell, { - overlayMode: "inset", - overlayBodyRows: hostRows, - overlayMinBodyRows: minHostRows, - }) + relayoutOverlayHost(shell, listItems) applyFocus(shell) paintOverlayList(shell) } @@ -4301,13 +4312,24 @@ export function setOverlayItems( items: readonly string[], itemIds?: readonly string[], itemValues?: readonly (string | undefined)[], + opts?: { readonly resetActive?: boolean }, ): void { if (!shell.overlayList) return shell.overlayItems = items const bag = internals.get(shell) if (bag && itemIds) bag.overlayItemIds = [...itemIds] if (bag && itemValues) bag.overlayItemValues = [...itemValues] - shell.overlayList = setListCount(shell.overlayList, items.length) + // Most callers (mention/model-picker filtering) keep the operator's current + // selection as the list narrows. The `/` popup instead resets to the top + // row on every keystroke, matching pre-refresh behavior where each filter + // reopened the overlay fresh. + shell.overlayList = opts?.resetActive + ? createListViewport({ + count: items.length, + height: shell.overlayList.height, + activeIndex: 0, + }) + : setListCount(shell.overlayList, items.length) paintOverlayList(shell) } @@ -5195,16 +5217,65 @@ export function openSlashCommands(shell: AppShell): boolean { const matches = resolvePaletteCatalog(shell).filter((cmd) => cmd.id.toLowerCase().startsWith(q), ) + + // Every keystroke lands here while the popup is already open. Closing and + // reopening released the overlay host between the two calls (closeSlashPopup + // routes through closeInsetOverlay, which fires notifyOverlayClosed) — long + // enough for a queued permission/operator gate to drain onto it. Refreshing + // the open palette in place never releases the host, so a queued gate has + // nothing to drain into. priorOverlay stacking is untouched here (it is only + // ever written by openListOverlay's stack-on-open path), so a palette + // stacked over a prior overlay keeps that snapshot across the refresh. + // + // A typo that zeroes the matches must not fall through to closeSlashPopup + // while the popup is already open — that closes through the same + // notifyOverlayClosed path and drains a queued gate mid-filter. Instead + // this refreshes in place to a "(no matches)" row, same as the general + // palette does, and holds the host until a real dismiss (deleting the `/`, + // Esc, accept) or a backspace that restores matches. + if (isSlashPopupOpen(shell) && shell.overlayKind === "palette") { + refreshSlashPopupInPlace(shell, matches) + return true + } + if (matches.length === 0) { closeSlashPopup(shell) return false } + closeSlashPopup(shell) openPalette(shell, { catalog: matches, title: "commands · /" }) slashPopups.add(shell) return true } +/** Refresh the already-open `/` popup's rows in place for the given matches. */ +function refreshSlashPopupInPlace( + shell: AppShell, + matches: readonly PaletteCommand[], +): void { + const labels = matches.length > 0 ? paletteLabels(matches) : ["(no matches)"] + shell.paletteCommands = matches + const bag = internals.get(shell) + if (bag) { + bag.paletteFilter = { + query: bag.paletteFilter?.query ?? "", + title: "commands · /", + catalog: matches, + typeToFilter: false, + } + bag.overlayDescribe = (id) => { + const cmd = matches.find((c) => c.id === id) + const what = cmd?.description?.trim() + return what ? { what } : null + } + } + setOverlayItems(shell, labels, matches.map((c) => c.id), undefined, { + resetActive: true, + }) + relayoutOverlayHost(shell, labels.length) +} + function setPromptText(shell: AppShell, value: string): void { shell.prompt.value = value shell.prompt.cursorOffset = value.length @@ -5242,8 +5313,12 @@ export function handleSlashPopupKey(shell: AppShell, key: KeyEvent): boolean { !key.option ) { closeSlashPopup(shell) + // Zero matches: nothing to dispatch. Preserve the typed text instead of + // wiping it — pre-refresh this state was unreachable (popup closed on + // zero matches, so Enter fell through to normal prompt handling). + if (!active) return true setPromptText(shell, "") - if (active) dispatchPaletteSelection(shell, active) + dispatchPaletteSelection(shell, active) return true } diff --git a/src/tui/slash-popup-gate.test.ts b/src/tui/slash-popup-gate.test.ts new file mode 100644 index 000000000..263919db0 --- /dev/null +++ b/src/tui/slash-popup-gate.test.ts @@ -0,0 +1,198 @@ +/** + * CL-6699: a queued permission/operator gate must not open onto the host in + * the middle of a `/` command filter session. The old close-then-reopen + * refresh (closeSlashPopup -> closeInsetOverlay -> notifyOverlayClosed) + * released the host between the two calls, and a gate queued behind the + * popup drained into that gap. + */ +import { EventEmitter } from "node:events" +import { describe, expect, test } from "bun:test" + +import { withTestRenderer } from "./harness" +import type { PaletteCommand } from "./command-catalog" +import { wireGates } from "./gate-wire" +import { + createAppShell, + isSlashPopupOpen, + onOverlayClosed, + type AppShell, +} from "./shell" + +const CATALOG: readonly PaletteCommand[] = [ + { + id: "model", + label: "/model", + description: "Open model picker", + keywords: ["model", "Open model picker", "slash", "command"], + }, + { id: "mcp", label: "/mcp" }, + { id: "compact", label: "/compact" }, +] + +type Ctx = { + readonly shell: AppShell + readonly press: (key: string) => void + readonly render: () => Promise +} + +function withShell(fn: (ctx: Ctx) => Promise): Promise { + return withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: true, + run: "idle", + paletteCatalog: CATALOG, + }) + try { + await fn({ + shell, + press: (key) => h.pressKey(key as Parameters[0]), + render: h.renderOnce, + }) + } finally { + shell.dispose() + } + }, + { width: 80, height: 24 }, + ) +} + +describe("/ popup keeps a queued gate queued across a filter refresh", () => { + test("filter keystroke while a gate is queued", async () => { + await withShell(async ({ shell, press }) => { + const emitter = new EventEmitter() + const dispose = wireGates(emitter, shell) + // The host closing (onOverlayClosed) is what the queued gate waits + // on to drain — see gate-wire.ts's onOverlayClosed/pending. Under the + // old close-then-reopen refresh this fires on every filter keystroke + // (closeSlashPopup -> closeInsetOverlay -> notifyOverlayClosed) even + // though the palette immediately re-stacks on top and every assertion + // on shell.overlayKind alone sees only "palette" again by the time it + // runs. Counting this call directly is what actually distinguishes + // the in-place refresh from the old close+reopen one. + let closedCount = 0 + const disposeClosedSpy = onOverlayClosed(shell, () => { + closedCount++ + }) + try { + press("/") + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.overlayKind).toBe("palette") + + let resolved: unknown + emitter.emit("permission.gate", { + request: { + tool: "run_shell", + action: "Run shell command", + subject: "bun test", + scopes: [], + }, + resolve: (outcome: unknown) => { + resolved = outcome + }, + }) + + // Queued, not opened — the slash popup still owns the host. + expect(shell.overlayKind).toBe("palette") + expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) + + // Refreshing the filter must not release the host to the queued gate. + press("m") + expect(shell.prompt.value).toBe("/m") + expect(shell.overlayKind).toBe("palette") + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.paletteCommands.map((c) => c.id)).toEqual([ + "model", + "mcp", + ]) + expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) + + // Filtering keeps working after the refresh. + press("o") + expect(shell.prompt.value).toBe("/mo") + expect(shell.paletteCommands.map((c) => c.id)).toEqual(["model"]) + expect(isSlashPopupOpen(shell)).toBe(true) + expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) + + // A keystroke that drops matches to zero must not dismiss the popup + // either — it stays owned with a "(no matches)" row, and the gate + // stays queued behind it. + press("z") + expect(shell.prompt.value).toBe("/moz") + expect(isSlashPopupOpen(shell)).toBe(true) + expect(shell.overlayKind).toBe("palette") + expect(shell.paletteCommands).toEqual([]) + expect(shell.overlayItems).toEqual(["(no matches)"]) + expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) + + // A backspace that restores matches refreshes back in place too. + press("Backspace") + expect(shell.prompt.value).toBe("/mo") + expect(shell.paletteCommands.map((c) => c.id)).toEqual(["model"]) + expect(isSlashPopupOpen(shell)).toBe(true) + expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) + + // A true dismiss still drains the queue as before. + press("Escape") + await Bun.sleep(60) + expect(shell.overlayKind).toBe("permissions") + expect(resolved).toBeUndefined() + expect(closedCount).toBe(1) + } finally { + disposeClosedSpy() + dispose() + } + }) + }) + + test("Enter on zero matches closes the popup, keeps the typed text, and drains a queued gate", async () => { + await withShell(async ({ shell, press }) => { + const emitter = new EventEmitter() + const dispose = wireGates(emitter, shell) + const disposeClosedSpy = onOverlayClosed(shell, () => {}) + try { + press("/") + press("m") + press("o") + press("z") + expect(shell.prompt.value).toBe("/moz") + expect(shell.paletteCommands).toEqual([]) + expect(isSlashPopupOpen(shell)).toBe(true) + + let resolved: unknown + emitter.emit("permission.gate", { + request: { + tool: "run_shell", + action: "Run shell command", + subject: "bun test", + scopes: [], + }, + resolve: (outcome: unknown) => { + resolved = outcome + }, + }) + expect(shell.overlayKind).toBe("palette") + expect(resolved).toBeUndefined() + + // Enter with no active command must not wipe the typed text. + press("Enter") + expect(isSlashPopupOpen(shell)).toBe(false) + expect(shell.prompt.value).toBe("/moz") + + // Popup close is a genuine dismiss: the queued gate drains onto it. + await Bun.sleep(60) + expect(shell.overlayKind).toBe("permissions") + expect(resolved).toBeUndefined() + } finally { + disposeClosedSpy() + dispose() + } + }) + }) +})