From 5acfbb3777d7fbac634f4ed83aefed56fb49cb8f Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 09:14:10 +0200 Subject: [PATCH] Let Escape close plugin previews before stopping agents (#7110) * fix(app): let Escape close an open popover before it interrupts the agent The agent.interrupt shortcut listens on the window in the capture phase, so it sees Escape before anything else. Registered overlays get first refusal through dispatchTopWebOverlayKeyDown, but plugins cannot register, and draw their floating panels with the Popover API instead. With such a panel open and the composer focused while an agent runs, Escape closed the panel and interrupted the agent in the same press. While a popover is open, Escape is left to it and no app shortcut runs. A second Escape, with nothing open, interrupts as before. Co-Authored-By: Claude Opus 5.5 Co-authored-by: RoundRedDot * test(app): cover Escape with a popover open through the shortcut path The first test only checked hasOpenDomPopover and would still pass with the guard removed from useKeyboardShortcuts. The check is now keyBelongsToDomPopover, called by the hook, and a browser test drives it the way the app does: a window capture listener for shortcuts, a popover closed by its own document Escape listener, focus in a textarea. The first Escape closes the popover with no interrupt; the second interrupts. Both new tests fail with the guard removed. Co-Authored-By: Claude Opus 5.5 Co-authored-by: RoundRedDot * test(app): exercise plugin Escape through the real composer Keep the contributor fix and replace the substitute shortcut-listener test with an installed-plugin browser regression. The first Escape must close the preview without cancelling the turn; the second must cancel it. Co-authored-by: luke Co-authored-by: RoundRedDot * test(app): check dismissed model pickers leave the viewport --------- Co-authored-by: luke Co-authored-by: Claude Opus 5.5 Co-authored-by: RoundRedDot Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- .../e2e/browser/bottom-sheet-reopen.spec.ts | 8 ++-- .../app/e2e/browser/plugin-popover.spec.ts | 47 +++++++++++++++++++ .../e2e/support/helpers/plugin-timeline.ts | 43 +++++++++++++++++ .../app/src/hooks/use-keyboard-shortcuts.ts | 6 ++- .../app/src/lib/overlay-root.browser.test.tsx | 38 ++++++++++++++- packages/app/src/lib/overlay-root.ts | 25 ++++++++++ 6 files changed, 162 insertions(+), 5 deletions(-) create mode 100644 packages/app/e2e/browser/plugin-popover.spec.ts diff --git a/packages/app/e2e/browser/bottom-sheet-reopen.spec.ts b/packages/app/e2e/browser/bottom-sheet-reopen.spec.ts index 89ee10af22..bc65ae65de 100644 --- a/packages/app/e2e/browser/bottom-sheet-reopen.spec.ts +++ b/packages/app/e2e/browser/bottom-sheet-reopen.spec.ts @@ -89,9 +89,11 @@ async function dismissStackedModelPickerWithBackdrop(page: Page) { .getByTestId("agent-controls-settings-list") .getByRole("button", { name: /Select model/ }) .click(); - await expect(page.getByTestId("agent-controls-model-browser-sheet")).toBeVisible(); + await expect(page.getByTestId("agent-controls-model-browser-sheet")).toBeInViewport(); await page.mouse.click(MOBILE_VIEWPORT.width / 2, 24); - await expect(page.getByTestId("agent-controls-model-browser-sheet")).not.toBeVisible(); + // In CI, Gorhom retains the dismissed sheet below the viewport. + // Dismissal is complete when it leaves the displayed surface. + await expect(page.getByTestId("agent-controls-model-browser-sheet")).not.toBeInViewport(); await expect(page.getByTestId("agent-controls-model-sheet")).toBeVisible(); await expect(page.getByTestId("agent-controls-settings-list")).toBeVisible(); } @@ -139,7 +141,7 @@ test.describe("mobile bottom sheet reopen", () => { await expect(sheet).toBeVisible(); await expect(page.getByTestId("agent-controls-settings-list")).toBeVisible(); await expect(page.getByTestId("agent-controls-model")).toContainText("Ten second stream"); - await expect(page.getByTestId("agent-controls-model-browser-sheet")).not.toBeVisible(); + await expect(page.getByTestId("agent-controls-model-browser-sheet")).not.toBeInViewport(); }); } finally { await session.cleanup(); diff --git a/packages/app/e2e/browser/plugin-popover.spec.ts b/packages/app/e2e/browser/plugin-popover.spec.ts new file mode 100644 index 0000000000..8ce3b35513 --- /dev/null +++ b/packages/app/e2e/browser/plugin-popover.spec.ts @@ -0,0 +1,47 @@ +import { expect, test, type Page } from "../support/fixtures"; +import { composerLocator, submitMessageWithButton } from "../support/helpers/composer"; +import { seedMockAgentWorkspace, openAgentRoute } from "../support/helpers/mock-agent"; +import { withPopoverTimelinePlugin } from "../support/helpers/plugin-timeline"; + +async function openPreviewWhileTyping(page: Page): Promise { + await composerLocator(page).focus(); + await page.getByRole("button", { name: "Hover plugin preview", exact: true }).hover(); + await expect(page.getByRole("dialog", { name: "Plugin preview" })).toBeVisible(); + await expect(composerLocator(page)).toBeFocused(); +} + +async function dismissPreview(page: Page): Promise { + await page.keyboard.press("Escape"); + await expect(page.getByRole("dialog", { name: "Plugin preview" })).not.toBeVisible(); +} + +test("Escape dismisses a plugin preview before interrupting the agent", async ({ page }) => { + test.setTimeout(120_000); + const agent = await seedMockAgentWorkspace({ + repoPrefix: "plugin-popover-", + title: "Plugin preview keyboard regression", + model: "e2e-fast-stream", + }); + try { + await withPopoverTimelinePlugin(async () => { + await openAgentRoute(page, agent); + await submitMessageWithButton(page, "Show the plugin preview."); + await agent.client.waitForFinish(agent.agentId, 30_000); + await expect(page.getByRole("button", { name: "Hover plugin preview" })).toBeVisible(); + await submitMessageWithButton(page, "withhold synthetic user message until interrupted"); + const stop = page.getByRole("button", { name: "Stop agent", exact: true }); + await expect(stop).toBeVisible(); + + await openPreviewWhileTyping(page); + await dismissPreview(page); + await expect(stop).toBeVisible(); + await expect(composerLocator(page)).toBeFocused(); + + await page.keyboard.press("Escape"); + await expect(stop).not.toBeVisible(); + await agent.client.waitForFinish(agent.agentId, 15_000); + }); + } finally { + await agent.cleanup(); + } +}); diff --git a/packages/app/e2e/support/helpers/plugin-timeline.ts b/packages/app/e2e/support/helpers/plugin-timeline.ts index 8ad7e0a632..8b0f680e2d 100644 --- a/packages/app/e2e/support/helpers/plugin-timeline.ts +++ b/packages/app/e2e/support/helpers/plugin-timeline.ts @@ -231,3 +231,46 @@ export async function attachPromptTimelineEvidence(page: Page, info: TestInfo): contentType: "image/png", }); } + +const POPOVER_CLIENT_SOURCE = `import React, { useEffect, useRef } from "react"; +import { z } from "zod"; + +function Preview() { + const panel = useRef(null); + useEffect(() => { + const close = (event) => { + if (event.key === "Escape" && panel.current.matches(":popover-open")) { + panel.current.hidePopover(); + } + }; + document.addEventListener("keydown", close, true); + return () => document.removeEventListener("keydown", close, true); + }, []); + return
+ +
Plugin hover preview
+
; +} + +export default function contribute(client) { + client.addTimelineTransformer({ + id: "preview", query: { itemType: "user_message" }, + transform({ item }) { + if (item.text !== "Show the plugin preview.") return; + return { items: [{ type: "plugin", kind: "preview", version: 1, data: {} }] }; + }, + }); + client.addTimelineRenderer({ + kind: "preview", version: 1, schema: z.object({}), Component: Preview, + }); + return () => {}; +}`; + +/** Installs a timeline preview that owns Escape through the DOM Popover API. */ +export async function withPopoverTimelinePlugin(run: () => Promise): Promise { + await withDirectoryPlugin({ + pluginId: "timeline-popover-e2e", + source: POPOVER_CLIENT_SOURCE, + run, + }); +} diff --git a/packages/app/src/hooks/use-keyboard-shortcuts.ts b/packages/app/src/hooks/use-keyboard-shortcuts.ts index d723b5be04..6f332b0a98 100644 --- a/packages/app/src/hooks/use-keyboard-shortcuts.ts +++ b/packages/app/src/hooks/use-keyboard-shortcuts.ts @@ -40,7 +40,7 @@ import { navigateToLastWorkspace, useActiveWorkspaceSelection, } from "@/stores/navigation-active-workspace-store"; -import { dispatchTopWebOverlayKeyDown } from "@/lib/overlay-root"; +import { dispatchTopWebOverlayKeyDown, keyBelongsToDomPopover } from "@/lib/overlay-root"; export function useKeyboardShortcuts({ enabled, @@ -321,6 +321,10 @@ export function useKeyboardShortcuts({ return; } + if (keyBelongsToDomPopover(event)) { + return; + } + // During IME composition, Enter confirms the candidate selection and must // not route through global shortcuts like message send. if (isImeComposingKeyboardEvent(event)) { diff --git a/packages/app/src/lib/overlay-root.browser.test.tsx b/packages/app/src/lib/overlay-root.browser.test.tsx index a21403dd8d..bd20d2aeeb 100644 --- a/packages/app/src/lib/overlay-root.browser.test.tsx +++ b/packages/app/src/lib/overlay-root.browser.test.tsx @@ -2,7 +2,11 @@ import React, { type RefCallback } from "react"; import { flushSync } from "react-dom"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { useWebOverlayRegistration } from "./overlay-root"; +import { + hasOpenDomPopover, + keyBelongsToDomPopover, + useWebOverlayRegistration, +} from "./overlay-root"; function nextFrame(): Promise { return new Promise((resolve) => requestAnimationFrame(() => resolve())); @@ -68,3 +72,35 @@ describe("useWebOverlayRegistration in the browser", () => { expect(openerFocusCalls).toBe(1); }); }); + +describe("hasOpenDomPopover", () => { + afterEach(() => { + document.body.replaceChildren(); + }); + + it("is true only while a popover is shown", () => { + const panel = document.createElement("div"); + panel.setAttribute("popover", "manual"); + document.body.append(panel); + expect(hasOpenDomPopover()).toBe(false); + panel.showPopover(); + expect(hasOpenDomPopover()).toBe(true); + panel.hidePopover(); + expect(hasOpenDomPopover()).toBe(false); + }); +}); + +describe("Escape with a plugin popover open", () => { + afterEach(() => { + document.body.replaceChildren(); + }); + + it("leaves other keys to the shortcuts while a popover is open", () => { + const panel = document.createElement("div"); + panel.setAttribute("popover", "manual"); + document.body.append(panel); + panel.showPopover(); + expect(keyBelongsToDomPopover({ key: "Enter" })).toBe(false); + expect(keyBelongsToDomPopover({ key: "Escape" })).toBe(true); + }); +}); diff --git a/packages/app/src/lib/overlay-root.ts b/packages/app/src/lib/overlay-root.ts index 9ffd81725f..d1ab0b9787 100644 --- a/packages/app/src/lib/overlay-root.ts +++ b/packages/app/src/lib/overlay-root.ts @@ -112,6 +112,31 @@ export function hasActiveWebOverlay(): boolean { return getTopWebOverlay() !== undefined; } +/** + * True while an element shown with the Popover API is open. Such a layer + * floats above the app like a registered overlay but lives outside the + * registry: plugins draw their panels this way, since they cannot register. + */ +export function hasOpenDomPopover(): boolean { + if (typeof document === "undefined") return false; + try { + return document.querySelector(":popover-open") !== null; + } catch { + // An engine without the Popover API rejects the selector, and has none open. + return false; + } +} + +/** + * Whether a key belongs to an open popover rather than to the app's shortcuts. + * An open popover (a plugin's panel) is the top layer and closes on its own + * Escape listener, which runs after the shortcut engine's capture listener: + * Escape must not also interrupt the running agent. + */ +export function keyBelongsToDomPopover(event: Pick): boolean { + return event.key === "Escape" && hasOpenDomPopover(); +} + function getFocusableElements(scope: HTMLElement): HTMLElement[] { const selector = [ "a[href]",