From 1b24e0044a0aeb592e87d79c00a36ea6049a39a2 Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 24 Jun 2026 14:24:15 +0000 Subject: [PATCH] fix(coding-agent): restore focus to the live editor-slot owner when a fullscreen overlay closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When /settings (or the Extensions/Agents dashboard) is open and a tool approval prompt fires, ExtensionUiController.showHookSelector swaps the editor out of editorContainer for the HookSelectorComponent. On exit, the overlay's done() called overlayHandle.hide() + setFocus(editor), both pointing at the editor captured as preFocus when the overlay opened — now no longer mounted. The visible approval prompt then sat unreachable: Up/Down/Enter/Esc routed to the unmounted editor and only Ctrl+C escaped (issue #3349). SelectorController now exposes focusActiveEditorArea(), which restores focus to editorContainer.children[0] (the live slot owner) or falls back to the editor. Wired into showSettingsSelector, showExtensionsDashboard, and showAgentsDashboard close paths after overlay.hide(). Tests: unit test verifying focusActiveEditorArea picks the live slot owner; TUI overlay-focus regression pinning the post-fix contract plus a 'pre-fix snapshot' test pinning the broken pre-fix behavior so the restore-from-preFocus assumption can't silently change. Fixes #3349 --- packages/coding-agent/CHANGELOG.md | 1 + .../modes/controllers/selector-controller.ts | 20 +++- .../selector-controller-overlay-focus.test.ts | 85 ++++++++++++++++ packages/tui/test/overlay-focus.test.ts | 96 ++++++++++++++++++- 4 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/modes/controllers/selector-controller-overlay-focus.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 03ec72a40..8e58ca81f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed the TUI freezing when a tool approval prompt fires while `/settings` (or the Extensions/Agents dashboard) is open. The fullscreen overlay's close handler restored focus to the editor it had captured at open time, but `ExtensionUiController` had since swapped the editor out of the editor slot for the approval prompt — so on exit the visible prompt sat unreachable while keystrokes routed to the now-unmounted editor (no Enter/Up/Down/Esc response, only Ctrl+C escaped). `SelectorController` now restores focus to whatever currently owns the editor slot via a `focusActiveEditorArea()` helper, applied to settings, extensions dashboard, and agents dashboard close paths. ([#3349](https://github.com/can1357/oh-my-pi/issues/3349)) - Fixed all extension loading silently failing on the cross-compiled `omp-darwin-arm64` release binary (downloaded directly or via a Homebrew tap wrapper) because `__computeBunfsPackageRoot` mis-handled `import.meta.dir = "//root/omp-darwin-arm64"`. Bun 1.3.14 reports `/` for the compiled entry's `import.meta.dir`, but the pre-fix function joined `metaDir + "packages"` and produced `/root/omp-darwin-arm64/packages` — the binary basename was baked into every bunfs path, so the TypeBox/legacy-pi shims and every `@oh-my-pi/pi-*` package-root override failed `existsSync` validation and `resolveCanonicalPiSpecifier` fell through to a bunfs `Bun.resolveSync` that also could not find the module. The function now detects the bunfs-root + binary-basename shape (`path.basename(path.dirname(metaDir)) === "root"`) and strips the trailing binary segment by slicing the original `metaDir`; the production bunfs shim join path also preserves Bun's bunfs-native `//root` / `B:\~BUN\root` prefix that `path.join` would otherwise collapse. ([#3329](https://github.com/can1357/oh-my-pi/issues/3329)) - Fixed llama.cpp discovery to prefer per-model `/v1/models` `meta.n_ctx`/`meta.n_ctx_train` values, refresh selected models after lazy load, and bypass fresh-cache reuse so server restarts update context windows. ([#3310](https://github.com/can1357/oh-my-pi/issues/3310)) - Fixed `task.maxConcurrency: 0` serializing subagent spawns instead of running them unbounded. The settings UI labels `0` as "Unlimited", but the session-scoped spawn `Semaphore` clamped `max` via `Math.max(1, max)`, so the second subagent body in a batch always waited for the first to release the seat. The constructor now treats `max <= 0` (and any non-finite input) as unbounded via `Number.POSITIVE_INFINITY`, matching the eval `parallel()`/`pipeline()` worker-pool semantics ([#3305](https://github.com/can1357/oh-my-pi/issues/3305)). diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index 7ecf68e90..d462f4bf4 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -84,6 +84,22 @@ export class SelectorController { ), ); } + + /** + * Restore keyboard focus to whatever currently owns the editor slot. The + * slot can hold the editor itself or a hook selector/input/editor pushed + * in by `ExtensionUiController` — e.g. an approval prompt that fired while + * a fullscreen overlay was up. `overlayHandle.hide()` restores focus to + * the component focused when the overlay opened, which is stale in that + * case (the editor was swapped out): keys land on a hidden editor and the + * visible prompt receives nothing (issue #3349). Call this after the + * overlay hides to re-target focus at the visible slot owner. + */ + focusActiveEditorArea(): void { + const visible = this.ctx.editorContainer.children[0] ?? this.ctx.editor; + this.ctx.ui.setFocus(visible); + } + /** * Shows a selector component in place of the editor. * @param create Factory that receives a `done` callback and returns the component and focus target @@ -109,7 +125,7 @@ export class SelectorController { let overlayHandle: OverlayHandle | undefined; const done = () => { overlayHandle?.hide(); - this.ctx.ui.setFocus(this.ctx.editor); + this.focusActiveEditorArea(); this.ctx.ui.requestRender(); }; const selector = new SettingsSelectorComponent( @@ -224,6 +240,7 @@ export class SelectorController { }); dashboard.onClose = () => { overlay.hide(); + this.focusActiveEditorArea(); this.ctx.ui.requestRender(); }; dashboard.onRequestRender = () => { @@ -251,6 +268,7 @@ export class SelectorController { }); dashboard.onClose = () => { overlay.hide(); + this.focusActiveEditorArea(); this.ctx.ui.requestRender(); }; dashboard.onRequestRender = () => { diff --git a/packages/coding-agent/test/modes/controllers/selector-controller-overlay-focus.test.ts b/packages/coding-agent/test/modes/controllers/selector-controller-overlay-focus.test.ts new file mode 100644 index 000000000..539dfb3c7 --- /dev/null +++ b/packages/coding-agent/test/modes/controllers/selector-controller-overlay-focus.test.ts @@ -0,0 +1,85 @@ +import { beforeAll, describe, expect, it, vi } from "bun:test"; +import { SelectorController } from "@oh-my-pi/pi-coding-agent/modes/controllers/selector-controller"; +import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; + +beforeAll(async () => { + await initTheme(); +}); + +interface EditorSlot { + children: unknown[]; + clear: () => void; + addChild: (child: unknown) => void; +} + +function createEditorSlot(...initial: unknown[]): EditorSlot { + return { + children: [...initial], + clear() { + this.children = []; + }, + addChild(child: unknown) { + this.children.push(child); + }, + }; +} + +function createCtx(slot: EditorSlot, editor: unknown) { + const setFocus = vi.fn(); + const ctx = { + editor, + editorContainer: slot, + ui: { + setFocus, + requestRender: vi.fn(), + }, + } as unknown as InteractiveModeContext; + return { ctx, setFocus }; +} + +describe("SelectorController.focusActiveEditorArea", () => { + // Regression for issue #3349: closing a fullscreen overlay (settings, + // extensions dashboard, agents dashboard) while a hook selector / approval + // prompt occupies the editor slot must restore focus to that prompt — not + // to the editor that the prompt replaced. Pre-fix, the close handlers + // hardcoded `setFocus(this.ctx.editor)`, leaving keystrokes routed to a + // no-longer-mounted editor while the visible prompt sat unreachable. + + it("focuses the editor when the slot has only the editor in it", () => { + const editor = { id: "editor" }; + const slot = createEditorSlot(editor); + const { ctx, setFocus } = createCtx(slot, editor); + + new SelectorController(ctx).focusActiveEditorArea(); + + expect(setFocus).toHaveBeenCalledTimes(1); + expect(setFocus).toHaveBeenCalledWith(editor); + }); + + it("focuses the active hook-selector-style prompt when the slot holds it instead of the editor", () => { + const editor = { id: "editor" }; + const approvalPrompt = { id: "approval-prompt" }; + // Mirrors `ExtensionUiController.showHookSelector`: the hook surface + // clears the slot and replaces the editor with its prompt component. + const slot = createEditorSlot(approvalPrompt); + const { ctx, setFocus } = createCtx(slot, editor); + + new SelectorController(ctx).focusActiveEditorArea(); + + expect(setFocus).toHaveBeenCalledTimes(1); + expect(setFocus).toHaveBeenCalledWith(approvalPrompt); + expect(setFocus).not.toHaveBeenCalledWith(editor); + }); + + it("falls back to the editor when the slot is empty (defensive)", () => { + const editor = { id: "editor" }; + const slot = createEditorSlot(); + const { ctx, setFocus } = createCtx(slot, editor); + + new SelectorController(ctx).focusActiveEditorArea(); + + expect(setFocus).toHaveBeenCalledTimes(1); + expect(setFocus).toHaveBeenCalledWith(editor); + }); +}); diff --git a/packages/tui/test/overlay-focus.test.ts b/packages/tui/test/overlay-focus.test.ts index 4863782c5..ce741aca6 100644 --- a/packages/tui/test/overlay-focus.test.ts +++ b/packages/tui/test/overlay-focus.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { type Component, type Focusable, type OverlayFocusOwner, TUI } from "@oh-my-pi/pi-tui"; +import { type Component, Container, type Focusable, type OverlayFocusOwner, TUI } from "@oh-my-pi/pi-tui"; import type { Terminal, TerminalAppearance } from "@oh-my-pi/pi-tui/terminal"; class MinimalTerminal implements Terminal { @@ -152,4 +152,98 @@ describe("TUI overlay focus", () => { tui.stop(); } }); + + it("hands focus to the live editor-slot owner after a fullscreen overlay closes (issue #3349)", () => { + // Repro for issue #3349: opening /settings (a fullscreen overlay) + // while a tool approval prompt fires lands the prompt component in + // the editor slot. When the overlay closes, `overlayHandle.hide()` + // restores focus to the preFocus captured at open time — the + // (now-unmounted) editor. Pre-fix, the visible prompt received no + // keystrokes and the TUI looked frozen. The SelectorController fix + // follows `hide()` with `setFocus(editorContainer.children[0] ?? editor)`; + // this test pins that pattern at the TUI level. + const terminal = new MinimalTerminal(); + const tui = new TUI(terminal); + + const editor = new FocusRecorder("editor"); + const editorContainer = new Container(); + editorContainer.addChild(editor); + tui.addChild(editorContainer); + tui.setFocus(editor); + + try { + tui.start(); + + // /settings opens a fullscreen overlay. preFocus captured = editor. + const settingsOverlay = new FocusRecorder("settings"); + const handle = tui.showOverlay(settingsOverlay, { fullscreen: true }); + expect(tui.getFocused()).toBe(settingsOverlay); + + // While settings is open, a tool approval prompt swaps the editor + // slot to a hook-selector component. Focus snaps back to the + // settings overlay because it owns the top of the overlay stack. + const approvalPrompt = new FocusRecorder("approval"); + editorContainer.clear(); + editorContainer.addChild(approvalPrompt); + tui.setFocus(approvalPrompt); + expect(tui.getFocused()).toBe(settingsOverlay); + + // User Esc's out of settings. Replicate the post-fix close path: + // hide(), then setFocus on whatever owns the slot right now. + handle.hide(); + const slotOwner = editorContainer.children[0] ?? editor; + tui.setFocus(slotOwner); + + // The visible approval prompt now receives input. Pre-fix the + // hide()-only restore left focus on the stale editor. + terminal.sendInput("\x1b[B"); + terminal.sendInput("\r"); + expect(tui.getFocused()).toBe(approvalPrompt); + expect(approvalPrompt.inputs).toEqual(["\x1b[B", "\r"]); + expect(editor.inputs).toEqual([]); + } finally { + tui.stop(); + } + }); + + it("pre-fix snapshot: hide() alone restores focus to the stale editor, missing the live slot owner (issue #3349)", () => { + // Companion to the test above: pin the exact pre-fix behavior so a + // future refactor of `overlayHandle.hide()` cannot silently change the + // contract that the SelectorController fix compensates for. `hide()` + // restores focus to `preFocus` captured at open time — the editor — + // regardless of what currently occupies the editor slot. The + // SelectorController close handlers MUST call `focusActiveEditorArea()` + // after `hide()`; this test demonstrates why. + const terminal = new MinimalTerminal(); + const tui = new TUI(terminal); + + const editor = new FocusRecorder("editor"); + const editorContainer = new Container(); + editorContainer.addChild(editor); + tui.addChild(editorContainer); + tui.setFocus(editor); + + try { + tui.start(); + + const settingsOverlay = new FocusRecorder("settings"); + const handle = tui.showOverlay(settingsOverlay, { fullscreen: true }); + + const approvalPrompt = new FocusRecorder("approval"); + editorContainer.clear(); + editorContainer.addChild(approvalPrompt); + tui.setFocus(approvalPrompt); + + // Close the overlay WITHOUT the SelectorController's follow-up + // `focusActiveEditorArea()`. hide() restores focus to preFocus. + handle.hide(); + + terminal.sendInput("\x1b[B"); + expect(tui.getFocused()).toBe(editor); + expect(editor.inputs).toEqual(["\x1b[B"]); + expect(approvalPrompt.inputs).toEqual([]); + } finally { + tui.stop(); + } + }); });