From 27b0d1722bf190ec0984fcec1fc13014a07c0dad Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 21 Jul 2026 22:21:35 +0000 Subject: [PATCH] fix(tui): locked plan review overlay after a choice commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fullscreen Plan Review overlay stayed mounted, focused, and fully interactive after a choice was picked. showPlanReview's finish() resolves the choice promise but deliberately defers the hide until #approvePlan (guarded for #5688/#5319/#5689), so during slow async approval work — e.g. context compaction — arrow keys still moved the cursor and repeat Enter/Esc were silently swallowed by the settle guard, with the fullscreen buffer masking all progress underneath. Users on Ghostty and macOS Terminal read this as "/plan is frozen". The overlay now flips to a committed state the moment a choice fires: handleInput is a no-op, and the render shows a " — submitting…" indicator plus an "applying your selection" footer. The deferred-hide timing is untouched, so the existing stale-buffer and focus guards stay intact. Fixes #5926 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../modes/components/plan-review-overlay.ts | 24 +++++++++-- .../components/plan-review-overlay.test.ts | 43 ++++++++++++++++--- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ad20be261..9148f7078 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the fullscreen Plan Review overlay staying interactive with no feedback after a choice was picked: while the approval ran slow async work (e.g. context compaction) the overlay remained mounted, arrow keys still moved the cursor, and repeat Enter/Esc were silently swallowed, so users on Ghostty and macOS Terminal thought `/plan` was frozen. The overlay now locks input and shows a "submitting…" indicator the moment a choice commits ([#5926](https://github.com/can1357/oh-my-pi/issues/5926)). + ## [17.0.3] - 2026-07-17 ### Changed diff --git a/packages/coding-agent/src/modes/components/plan-review-overlay.ts b/packages/coding-agent/src/modes/components/plan-review-overlay.ts index 05e1a9a09..75e37a2a5 100644 --- a/packages/coding-agent/src/modes/components/plan-review-overlay.ts +++ b/packages/coding-agent/src/modes/components/plan-review-overlay.ts @@ -152,6 +152,14 @@ export class PlanReviewOverlay implements Component { * motion mouse reports and cleared when the pointer leaves the option rows. */ #hoveredOption: number | undefined; + // Once a choice fires, the promise-based caller resolves but keeps this + // overlay mounted while it runs slow async approval work (e.g. context + // compaction). Lock input and switch to a "submitting" indicator so the + // overlay stops looking interactive and repeat Enter/Esc are not silently + // swallowed with zero feedback (#5926). + #committed = false; + /** Label of the committed choice, shown while the async approval settles. */ + #committedLabel: string | undefined; #annotating = false; #input: Input; @@ -283,11 +291,14 @@ export class PlanReviewOverlay implements Component { #confirmSelection(): void { const index = this.#selectedIndex; if (index >= 0 && index < this.#options.length && !this.#disabled.has(index)) { + this.#committed = true; + this.#committedLabel = this.#options[index]!; this.callbacks.onPick(this.#options[index]!); } } handleInput(keyData: string): void { + if (this.#committed) return; if (keyData.startsWith("\x1b[<") && this.#handleMouse(keyData)) return; if (this.#annotating) { if (this.callbacks.onAnnotationExternalEditor && matchesAppExternalEditor(keyData)) { @@ -300,6 +311,7 @@ export class PlanReviewOverlay implements Component { return; } if (matchesSelectCancel(keyData)) { + this.#committed = true; this.callbacks.onCancel(); return; } @@ -838,10 +850,14 @@ export class PlanReviewOverlay implements Component { const innerWidth = Math.max(1, width - 4); const bodyContentWidth = sidebarShown ? splitBodyWidth(width, sidebarWidth) : innerWidth; - const sliderLines = this.#renderSliderLines(); - const optionLines = this.#renderOptionLines(); + const committed = this.#committed; + const sliderLines = committed ? [] : this.#renderSliderLines(); + const submittingLabel = this.#committedLabel ? `${this.#committedLabel} — submitting…` : "Submitting…"; + const optionLines = committed ? [theme.bold(theme.fg("accent", submittingLabel))] : this.#renderOptionLines(); const promptLines = this.#promptTitle ? [theme.bold(theme.fg("accent", this.#promptTitle))] : []; - const footerLines = this.#renderFooterLines(innerWidth); + const footerLines = committed + ? [theme.fg("dim", "Applying your selection — this can take a moment while context is prepared.")] + : this.#renderFooterLines(innerWidth); // Chrome rows: top border, two dividers, bottom border, plus the // prompt/slider/option/footer rows between them. @@ -884,7 +900,7 @@ export class PlanReviewOverlay implements Component { for (const line of promptLines) out.push(row(line, width)); for (const line of sliderLines) out.push(row(line, width)); for (let i = 0; i < optionLines.length; i++) { - this.#optionClickRows.set(out.length, i); + if (!committed) this.#optionClickRows.set(out.length, i); out.push(row(optionLines[i]!, width)); } out.push(divider(width)); diff --git a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts index 34a80a9bf..0463ed09a 100644 --- a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts +++ b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts @@ -75,7 +75,7 @@ describe("PlanReviewOverlay", () => { expect(onPick).toHaveBeenCalledWith("Approve and execute"); }); - it("moves the option cursor with up/down and confirms the new target", () => { + it("moves the option cursor with down and confirms the new target", () => { const onPick = vi.fn(); const overlay = new PlanReviewOverlay( "plan", @@ -85,13 +85,44 @@ describe("PlanReviewOverlay", () => { overlay.handleInput(DOWN); overlay.handleInput(ENTER); expect(onPick).toHaveBeenCalledWith("Approve and compact context"); + }); - onPick.mockClear(); + it("confirms the up-moved target from a lower start", () => { + const onPick = vi.fn(); + const overlay = new PlanReviewOverlay( + "plan", + { promptTitle: "next", options: APPROVAL_OPTIONS, initialIndex: 1 }, + { onPick, onCancel: vi.fn() }, + ); overlay.handleInput(UP); overlay.handleInput(ENTER); expect(onPick).toHaveBeenCalledWith("Approve and execute"); }); + it("locks input and shows a submitting indicator after a pick, ignoring repeat keys", () => { + const onPick = vi.fn(); + const onCancel = vi.fn(); + const overlay = new PlanReviewOverlay( + "plan", + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick, onCancel }, + ); + overlay.handleInput(ENTER); + expect(onPick).toHaveBeenCalledTimes(1); + expect(onPick).toHaveBeenCalledWith("Approve and execute"); + + const committed = stripVTControlCharacters(overlay.render(80).join("\n")); + expect(committed).toContain("Approve and execute — submitting…"); + + // The async approval window keeps the overlay mounted; further keys must + // not fire a second callback or move the cursor (#5926). + overlay.handleInput(DOWN); + overlay.handleInput(ENTER); + overlay.handleInput("\x1b"); + expect(onPick).toHaveBeenCalledTimes(1); + expect(onCancel).not.toHaveBeenCalled(); + }); + it("skips disabled options and never confirms them", () => { const onPick = vi.fn(); // Disable index 2 ("Approve and keep context"). @@ -654,14 +685,14 @@ describe("PlanReviewOverlay", () => { expect(hoverRow(overlay, "Approve and keep context")).toBe(true); expect(optionLineRaw(overlay, "Approve and keep context")).toContain(selectedBg); + // Pointer onto the top border (a non-option row) drops the highlight. + overlay.handleInput("\x1b[<35;6;1M"); + expect(optionLineRaw(overlay, "Approve and keep context")).not.toContain(selectedBg); + // Hover is visual only: the keyboard cursor stays on index 0, so Enter still // confirms the first option rather than the hovered one. overlay.handleInput(ENTER); expect(onPick).toHaveBeenCalledWith("Approve and execute"); - - // Pointer onto the top border (a non-option row) drops the highlight. - overlay.handleInput("\x1b[<35;6;1M"); - expect(optionLineRaw(overlay, "Approve and keep context")).not.toContain(selectedBg); }); it("never hovers a disabled option", () => {