mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): remove all extension overlays on /reload and session switch (#11853)
## Problem
`InteractiveMode.resetExtensionUI()` runs on `/reload` and before a
session is invalidated (session switch, new session, fork). For overlays
it called `this.ui.hideOverlay()` once, which pops only the topmost
entry of the TUI overlay stack.
Trace: an extension opens two overlays with `ctx.ui.custom({ overlay:
true })`, then the user runs `/reload`. Reset removes the upper overlay;
the lower one stays on screen, gets focus back and keeps receiving
input, although the extension instance that created it is gone. A hidden
overlay (`handle.setHidden(true)`) also stays in the stack and can be
shown again by stale code.
Blindly clearing the whole stack is not correct either: pi opens its own
overlays (the header logo easter egg in `components/easter-egg-3d.ts`).
With the old code, a reset while the easter egg was topmost removed the
easter egg and left the extension overlay.
## Fix
`showExtensionCustom` records each overlay it opens in
`extensionOverlays` (handle -> removal function). The same function
serves `done()` and `resetExtensionUI()`: it hides the overlay, clears
its blocked program status (#11851) and disposes its component. Deleting
the map entry first makes a second call a no-op, so a late `done()`
after reset does not dispose twice. Reset calls it for every recorded
overlay, newest first, including hidden ones. Overlays not opened by
extensions are left alone. This replaces the separate
`blockingOverlaySources` set and reset loop added in #11851.
Pending `ctx.ui.custom()` promises of removed overlays stay unsettled,
matching how reset already treats inline dialogs.
## Testing
- New regression test in `test/interactive-mode-status.test.ts`: a
pi-internal overlay plus three extension overlays (one hidden), then
reset. Checks that the extension overlays are gone and disposed, the
internal overlay remains and receives input. Fails without the fix.
- `test/interactive-mode-status.test.ts`,
`test/interactive-mode-extension-dialogs.test.ts`,
`test/interactive-mode-paste.test.ts`,
`test/suite/regressions/5943-session-start-notify.test.ts` pass.
- `npm run check` clean.
This commit is contained in:
parent
7f47a2ed09
commit
80d4d3c869
4 changed files with 113 additions and 30 deletions
|
|
@ -36,6 +36,7 @@
|
|||
- Fixed the read tool loading whole files into memory, which ran out of memory on large files even with `offset`/`limit`; it now streams the file and keeps only the shown lines. Custom `ReadOperations` can stream too by providing the new optional `createReadStream` ([#9654](https://github.com/earendil-works/pi/issues/9654))
|
||||
- Fixed OAuth logins such as `openai` and `anthropic` failing with `invalid_grant` after pi was stopped while it refreshed an expired token, for example an RPC client that stops pi right after startup while the background catalog refresh runs. The token endpoint had already replaced the refresh token, but pi exited before saving the new one. On exit, pi now waits for a token refresh in progress to be saved ([#11692](https://github.com/earendil-works/pi/issues/11692))
|
||||
- Fixed Ctrl+P not switching to the only model in scope (or the only available model) when the active model is a different one; it showed "Only one model in scope" instead. Cycling forward from a model outside the scope now also selects the first scoped model instead of skipping it ([#11806](https://github.com/earendil-works/pi/issues/11806))
|
||||
- Fixed `/reload` and session switches leaving extension overlays on screen when an extension had more than one `ctx.ui.custom({ overlay: true })` open: only the topmost overlay was removed, and the others kept focus and input although the extension that opened them was gone. All extension overlays, including hidden ones, are now removed and disposed; pi's own overlays stay open
|
||||
|
||||
|
||||
## [1.5.0] - 2026-11-02
|
||||
|
|
|
|||
|
|
@ -578,8 +578,11 @@ export class InteractiveMode {
|
|||
private extensionDialogs: Array<{ component: ExtensionDialogComponent; blocked: BlockedStatus | undefined }> = [];
|
||||
/** Numbers the program status sources of blocking overlays, so closing one keeps the others. */
|
||||
private nextBlockingOverlayId = 0;
|
||||
/** Program status sources of blocking overlays that have not called `done()` yet. */
|
||||
private readonly blockingOverlaySources = new Set<string>();
|
||||
/**
|
||||
* Open extension overlays (custom with overlay: true) and the function that removes each, in opening
|
||||
* order. Reset removes only these, not pi's own overlays.
|
||||
*/
|
||||
private readonly extensionOverlays = new Map<OverlayHandle, () => void>();
|
||||
private extensionTerminalInputSubscriptions = new Set<{
|
||||
handler: (data: string) => { consume?: boolean; data?: string } | undefined;
|
||||
unsubscribe: () => void;
|
||||
|
|
@ -2587,10 +2590,9 @@ export class InteractiveMode {
|
|||
this.extensionDialogs = [];
|
||||
this.showTopExtensionDialog();
|
||||
}
|
||||
this.ui.hideOverlay();
|
||||
// Reload and session switches drop overlays without calling done(), so clear their blocked status here.
|
||||
for (const source of this.blockingOverlaySources) this.programStatus.setBlocked(source, undefined);
|
||||
this.blockingOverlaySources.clear();
|
||||
// Reload and session switches replace every extension without calling done(): remove their overlays,
|
||||
// newest first, the same way done() does, but leave the promises pending as for inline dialogs.
|
||||
for (const removeOverlay of [...this.extensionOverlays.values()].reverse()) removeOverlay();
|
||||
this.clearExtensionTerminalInputListeners();
|
||||
this.setExtensionFooter(undefined);
|
||||
this.setExtensionHeader(undefined);
|
||||
|
|
@ -3090,18 +3092,7 @@ export class InteractiveMode {
|
|||
this.ui.requestRender();
|
||||
};
|
||||
|
||||
const close = (result: T) => {
|
||||
if (closed) return;
|
||||
closed = true;
|
||||
// Remove this overlay, not the topmost one: another overlay may have opened above it.
|
||||
if (isOverlay) overlayHandle?.hide();
|
||||
else restoreEditor();
|
||||
if (overlayStatusSource) {
|
||||
this.blockingOverlaySources.delete(overlayStatusSource);
|
||||
this.programStatus.setBlocked(overlayStatusSource, undefined);
|
||||
}
|
||||
// Note: both branches above already call requestRender
|
||||
resolve(result);
|
||||
const disposeComponent = () => {
|
||||
try {
|
||||
component?.dispose?.();
|
||||
} catch {
|
||||
|
|
@ -3109,6 +3100,28 @@ export class InteractiveMode {
|
|||
}
|
||||
};
|
||||
|
||||
// Shared by done() and resetExtensionUI(). Removing the map entry first makes a second call a no-op.
|
||||
const removeOverlay = () => {
|
||||
if (!overlayHandle || !this.extensionOverlays.delete(overlayHandle)) return;
|
||||
// Remove this overlay, not the topmost one: another overlay may have opened above it.
|
||||
overlayHandle.hide();
|
||||
if (overlayStatusSource) this.programStatus.setBlocked(overlayStatusSource, undefined);
|
||||
disposeComponent();
|
||||
};
|
||||
|
||||
const close = (result: T) => {
|
||||
if (closed) return;
|
||||
closed = true;
|
||||
if (isOverlay) {
|
||||
removeOverlay();
|
||||
resolve(result);
|
||||
} else {
|
||||
restoreEditor();
|
||||
resolve(result);
|
||||
disposeComponent();
|
||||
}
|
||||
};
|
||||
|
||||
Promise.resolve(factory(this.ui, theme, this.keybindings, close))
|
||||
.then((c) => {
|
||||
if (closed) return;
|
||||
|
|
@ -3128,10 +3141,8 @@ export class InteractiveMode {
|
|||
return w ? { width: w } : undefined;
|
||||
};
|
||||
overlayHandle = this.ui.showOverlay(c, resolveOptions());
|
||||
if (overlayStatusSource) {
|
||||
this.blockingOverlaySources.add(overlayStatusSource);
|
||||
this.programStatus.setBlocked(overlayStatusSource, options?.blocked);
|
||||
}
|
||||
this.extensionOverlays.set(overlayHandle, removeOverlay);
|
||||
if (overlayStatusSource) this.programStatus.setBlocked(overlayStatusSource, options?.blocked);
|
||||
// Expose handle to caller for visibility control
|
||||
options?.onHandle?.(overlayHandle);
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -12,11 +12,10 @@ type DialogContext = {
|
|||
setFocus: (component: Component) => void;
|
||||
requestRender: () => void;
|
||||
showOverlay: (component: Component) => { hide: () => void; setHidden: (hidden: boolean) => void };
|
||||
hideOverlay: () => void;
|
||||
};
|
||||
overlays: Component[];
|
||||
nextBlockingOverlayId: number;
|
||||
blockingOverlaySources: Set<string>;
|
||||
extensionOverlays: Map<unknown, () => void>;
|
||||
programStatus: { setBlocked: ReturnType<typeof vi.fn> };
|
||||
disposeActiveSelector: () => void;
|
||||
toggleToolOutputExpansion: () => void;
|
||||
|
|
@ -66,13 +65,10 @@ function createContext(): DialogContext & DialogMethods {
|
|||
setHidden: vi.fn(),
|
||||
};
|
||||
},
|
||||
hideOverlay: () => {
|
||||
context.overlays.pop();
|
||||
},
|
||||
},
|
||||
overlays: [],
|
||||
nextBlockingOverlayId: 0,
|
||||
blockingOverlaySources: new Set(),
|
||||
extensionOverlays: new Map(),
|
||||
programStatus: { setBlocked: vi.fn() },
|
||||
disposeActiveSelector: vi.fn(),
|
||||
toggleToolOutputExpansion: vi.fn(),
|
||||
|
|
@ -292,7 +288,8 @@ describe("InteractiveMode extension dialogs", () => {
|
|||
|
||||
context.resetExtensionUI();
|
||||
expect(context.programStatus.setBlocked).toHaveBeenLastCalledWith("extension-overlay:0", undefined);
|
||||
expect(context.blockingOverlaySources.size).toBe(0);
|
||||
expect(context.extensionOverlays.size).toBe(0);
|
||||
expect(context.overlays).toHaveLength(0);
|
||||
});
|
||||
|
||||
// Regression for #11431: replacing the editor while a dialog was open hid the dialog and left it unanswerable.
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ import { homedir } from "node:os";
|
|||
import * as path from "node:path";
|
||||
import { type AutocompleteProvider, CombinedAutocompleteProvider } from "@earendil-works/pi-tui";
|
||||
import { beforeAll, describe, expect, test, vi } from "vitest";
|
||||
import { type Component, Container, type Focusable, type TUI } from "../../tui/src/tui.ts";
|
||||
import { type Component, Container, type Focusable, type OverlayHandle, type TUI } from "../../tui/src/tui.ts";
|
||||
import { TuiMainScreen } from "../../tui/src/tui-main-screen.ts";
|
||||
import { VirtualTerminal } from "../../tui/test/virtual-terminal.ts";
|
||||
import type { AutocompleteProviderFactory } from "../src/core/extensions/types.ts";
|
||||
|
|
@ -237,6 +237,7 @@ describe("InteractiveMode.showExtensionCustom", () => {
|
|||
// Inline custom components go through the extension dialog stack.
|
||||
const extensionDialogState = () => ({
|
||||
extensionDialogs: [],
|
||||
extensionOverlays: new Map(),
|
||||
programStatus: { setBlocked: vi.fn() },
|
||||
pushExtensionDialog: (InteractiveMode as any).prototype.pushExtensionDialog,
|
||||
removeExtensionDialog: (InteractiveMode as any).prototype.removeExtensionDialog,
|
||||
|
|
@ -389,6 +390,79 @@ describe("InteractiveMode.showExtensionCustom", () => {
|
|||
}
|
||||
},
|
||||
);
|
||||
|
||||
// Regression: reset hid only the topmost overlay, so lower extension overlays stayed and kept input.
|
||||
test("resetExtensionUI removes every extension overlay but keeps pi's own overlays", async () => {
|
||||
const terminal = new VirtualTerminal(80, 24);
|
||||
const ui: TUI = new TuiMainScreen(terminal);
|
||||
const editorContainer = new Container();
|
||||
const editor = new TestFocusableComponent("EDITOR");
|
||||
const internal = new TestFocusableComponent("INTERNAL");
|
||||
const lower = Object.assign(new TestFocusableComponent("LOWER"), { dispose: vi.fn() });
|
||||
const hidden = Object.assign(new TestFocusableComponent("HIDDEN"), { dispose: vi.fn() });
|
||||
const upper = Object.assign(new TestFocusableComponent("UPPER"), { dispose: vi.fn() });
|
||||
const fakeThis = {
|
||||
editor,
|
||||
editorContainer,
|
||||
keybindings: {},
|
||||
ui,
|
||||
disposeActiveSelector: vi.fn(),
|
||||
...extensionDialogState(),
|
||||
// The rest of resetExtensionUI is not under test here.
|
||||
clearExtensionTerminalInputListeners: vi.fn(),
|
||||
setExtensionFooter: vi.fn(),
|
||||
setExtensionHeader: vi.fn(),
|
||||
clearExtensionWidgets: vi.fn(),
|
||||
footerDataProvider: { clearExtensionStatuses: vi.fn() },
|
||||
footer: { invalidate: vi.fn() },
|
||||
setCustomEditorComponent: vi.fn(),
|
||||
setupAutocompleteProvider: vi.fn(),
|
||||
defaultEditor: {},
|
||||
updateTerminalTitle: vi.fn(),
|
||||
setWorkingIndicator: vi.fn(),
|
||||
setHiddenThinkingLabel: vi.fn(),
|
||||
};
|
||||
const showOverlay = (component: Component, onHandle?: (handle: OverlayHandle) => void): Promise<unknown> =>
|
||||
(InteractiveMode as any).prototype.showExtensionCustom.call(fakeThis, () => component, {
|
||||
overlay: true,
|
||||
onHandle,
|
||||
}) as Promise<unknown>;
|
||||
|
||||
editorContainer.addChild(editor);
|
||||
ui.addChild(editorContainer);
|
||||
ui.setFocus(editor);
|
||||
ui.start();
|
||||
try {
|
||||
// A pi-internal overlay (for example the header easter egg) opened below the extension overlays.
|
||||
ui.showOverlay(internal);
|
||||
void showOverlay(lower);
|
||||
void showOverlay(hidden, (handle) => handle.setHidden(true));
|
||||
void showOverlay(upper);
|
||||
await flushTui(ui, terminal);
|
||||
expect(upper.focused).toBe(true);
|
||||
|
||||
(InteractiveMode as any).prototype.resetExtensionUI.call(fakeThis);
|
||||
await flushTui(ui, terminal);
|
||||
|
||||
const viewport = terminal.getViewport().join("\n");
|
||||
expect(viewport).toContain("INTERNAL");
|
||||
expect(viewport).not.toContain("LOWER");
|
||||
expect(viewport).not.toContain("UPPER");
|
||||
expect(lower.dispose).toHaveBeenCalledOnce();
|
||||
expect(hidden.dispose).toHaveBeenCalledOnce();
|
||||
expect(upper.dispose).toHaveBeenCalledOnce();
|
||||
terminal.sendInput("x");
|
||||
await flushTui(ui, terminal);
|
||||
expect(internal.inputs).toEqual(["x"]);
|
||||
expect(lower.inputs).toEqual([]);
|
||||
|
||||
ui.hideOverlay();
|
||||
expect(ui.hasOverlay()).toBe(false);
|
||||
expect(editor.focused).toBe(true);
|
||||
} finally {
|
||||
ui.stop();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("InteractiveMode.createExtensionUIContext addAutocompleteProvider", () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue