From a5e558c68542d08e036e284ddcff4fe62f38d70d Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 14:13:10 -0700 Subject: [PATCH 1/3] Refresh slash popup in place instead of close+reopen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openSlashCommands closed and reopened the palette overlay on every filter keystroke, and closeSlashPopup routes through closeInsetOverlay which fires notifyOverlayClosed — releasing the host long enough for a queued permission/operator gate to drain onto it mid-typing. Mirror the @-mention popup fix (PR #515): when the slash popup is already open, refresh its items via setOverlayItems instead, leaving priorOverlay stacking untouched. --- src/tui/shell.ts | 33 +++++++++ src/tui/slash-popup-gate.test.ts | 115 +++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 src/tui/slash-popup-gate.test.ts diff --git a/src/tui/shell.ts b/src/tui/shell.ts index cbe8e1451..b3d6cf078 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -5199,6 +5199,39 @@ export function openSlashCommands(shell: AppShell): boolean { closeSlashPopup(shell) return false } + + // 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. + if (isSlashPopupOpen(shell) && shell.overlayKind === "palette") { + 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, + paletteLabels(matches), + matches.map((c) => c.id), + ) + return true + } + closeSlashPopup(shell) openPalette(shell, { catalog: matches, title: "commands · /" }) slashPopups.add(shell) diff --git a/src/tui/slash-popup-gate.test.ts b/src/tui/slash-popup-gate.test.ts new file mode 100644 index 000000000..fefb63f18 --- /dev/null +++ b/src/tui/slash-popup-gate.test.ts @@ -0,0 +1,115 @@ +/** + * 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, + 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) + 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() + + // 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() + + // 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() + + // A true dismiss still drains the queue as before. + press("Escape") + await Bun.sleep(60) + expect(shell.overlayKind).toBe("permissions") + expect(resolved).toBeUndefined() + } finally { + dispose() + } + }) + }) +}) From 7e224e920071d9dedeecdc06682d2758bd0ccd65 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 14:25:26 -0700 Subject: [PATCH 2/3] Keep slash popup owned on zero-match filter and relayout in place --- src/tui/prompt-slash-exit.test.ts | 29 +++++--- src/tui/shell.ts | 120 ++++++++++++++++++++---------- src/tui/slash-popup-gate.test.ts | 38 ++++++++++ 3 files changed, 136 insertions(+), 51 deletions(-) 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 b3d6cf078..fc1c99b2a 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,10 +5217,6 @@ export function openSlashCommands(shell: AppShell): boolean { const matches = resolvePaletteCatalog(shell).filter((cmd) => cmd.id.toLowerCase().startsWith(q), ) - if (matches.length === 0) { - closeSlashPopup(shell) - return false - } // Every keystroke lands here while the popup is already open. Closing and // reopening released the overlay host between the two calls (closeSlashPopup @@ -5208,36 +5226,56 @@ export function openSlashCommands(shell: AppShell): boolean { // 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") { - 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, - paletteLabels(matches), - matches.map((c) => c.id), - ) + 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 diff --git a/src/tui/slash-popup-gate.test.ts b/src/tui/slash-popup-gate.test.ts index fefb63f18..4c9cac67c 100644 --- a/src/tui/slash-popup-gate.test.ts +++ b/src/tui/slash-popup-gate.test.ts @@ -14,6 +14,7 @@ import { wireGates } from "./gate-wire" import { createAppShell, isSlashPopupOpen, + onOverlayClosed, type AppShell, } from "./shell" @@ -62,6 +63,18 @@ describe("/ popup keeps a queued gate queued across a filter refresh", () => { 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) @@ -83,6 +96,7 @@ describe("/ popup keeps a queued gate queued across a filter refresh", () => { // 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") @@ -94,6 +108,7 @@ describe("/ popup keeps a queued gate queued across a filter refresh", () => { "mcp", ]) expect(resolved).toBeUndefined() + expect(closedCount).toBe(0) // Filtering keeps working after the refresh. press("o") @@ -101,13 +116,36 @@ describe("/ popup keeps a queued gate queued across a filter refresh", () => { 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() } }) From c806d05e774ed50ed122f9dfe8d21fd876930120 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 22 Aug 2026 14:31:42 -0700 Subject: [PATCH 3/3] Preserve typed text when Enter hits zero slash matches --- src/tui/shell.ts | 6 ++++- src/tui/slash-popup-gate.test.ts | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/tui/shell.ts b/src/tui/shell.ts index fc1c99b2a..5d736789d 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -5313,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 index 4c9cac67c..263919db0 100644 --- a/src/tui/slash-popup-gate.test.ts +++ b/src/tui/slash-popup-gate.test.ts @@ -150,4 +150,49 @@ describe("/ popup keeps a queued gate queued across a filter refresh", () => { } }) }) + + 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() + } + }) + }) })