fix(ui): close the top dialog instead of the tab on Cmd+W

apps/desktop/src/App.vue

@@ -175,6 +175,7 @@ import { savedSqlErrorMessage } from "@/lib/savedSql/savedSqlErrors";
 import { savedSqlDefaultTargetForWrite } from "@/lib/savedSql/savedSqlExecutionTarget";
 import { countActiveUpdateBlockingTasks } from "@/lib/app/appUpdateTaskGuard";
 import { initSavedSqlEditorPositions } from "@/lib/app/savedSqlEditorPosition";
+import { dismissOpenModalSurface } from "@/lib/ui/modalSurface";
 import { hasTreeNodeDatabaseContext } from "@/lib/sidebar/treeNodeContext";
 import { objectBrowserTablesToAiTreeNodes } from "@/lib/ai/objectBrowserToAiTargets";
 import { aiTargetFromTab, type AiConversationBinding } from "@/lib/ai/aiConversationBinding";
@@ -4203,6 +4204,10 @@ function refreshPluginWorkbenches(pluginId: string): void {
 }
 
 async function closeActiveSurface() {
+  // Cmd+W (and the native macOS "Close Tab" menu item) must not reach the surface behind
+  // an open modal dialog. While a dialog owns the window the keystroke dismisses that
+  // dialog instead, exactly like Esc, so the shortcut never goes dead on the user.
+  if (dismissOpenModalSurface()) return;
   if (showSettingsPage.value) {
     closeSettingsPage();
   } else if (showPluginCenter.value) {

apps/desktop/src/lib/ui/__tests__/modalSurface.spec.ts

@@ -0,0 +1,139 @@
+// @vitest-environment happy-dom
+
+import { createApp, defineComponent, h, nextTick, ref, type App, type Component } from "vue";
+import { afterEach, describe, expect, it } from "vitest";
+import { Dialog, DialogContent, DialogScrollContent } from "@/components/ui/dialog";
+import { Popover, PopoverContent } from "@/components/ui/popover";
+import { dismissOpenModalSurface, hasOpenModalSurface } from "@/lib/ui/modalSurface";
+
+const mountedApps: App[] = [];
+
+afterEach(() => {
+  for (const app of mountedApps.splice(0)) app.unmount();
+  document.body.innerHTML = "";
+});
+
+function mount(component: Component) {
+  const app = createApp(component);
+  mountedApps.push(app);
+  app.mount(document.createElement("div"));
+  return app;
+}
+
+describe("hasOpenModalSurface", () => {
+  it("detects the overlay of an open dialog", async () => {
+    const open = ref(true);
+    mount(defineComponent({ setup: () => () => h(Dialog, { open: open.value }, () => h(DialogContent, {}, () => h("p", "body"))) }));
+    await nextTick();
+
+    const overlay = document.querySelector('[data-slot="dialog-overlay"]');
+    expect(overlay?.getAttribute("data-state")).toBe("open");
+    expect(hasOpenModalSurface()).toBe(true);
+
+    open.value = false;
+    await nextTick();
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("ignores an overlay that is only finishing its exit animation", () => {
+    document.body.innerHTML = '<div data-slot="dialog-overlay" data-state="closed"></div>';
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("ignores non-blocking dialogs that render no overlay", async () => {
+    mount(
+      defineComponent({
+        setup: () => () => h(Dialog, { open: true, modal: false }, () => h(DialogScrollContent, { showOverlay: false }, () => h("p", "body"))),
+      }),
+    );
+    await nextTick();
+
+    const content = document.querySelector('[data-slot="dialog-content"]');
+    expect(content?.getAttribute("data-state")).toBe("open");
+    expect(document.querySelector('[data-slot="dialog-overlay"]')).toBeNull();
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("ignores open popovers that reuse role=dialog", async () => {
+    mount(defineComponent({ setup: () => () => h(Popover, { open: true }, () => h(PopoverContent, {}, () => h("p", "body"))) }));
+    await nextTick();
+
+    const popover = document.querySelector('[data-slot="popover-content"]');
+    expect(popover?.getAttribute("role")).toBe("dialog");
+    expect(popover?.getAttribute("data-state")).toBe("open");
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("honours hand-rolled modal opt-in and peek opt-out", () => {
+    document.body.innerHTML = '<div role="dialog" aria-modal="true"></div>';
+    expect(hasOpenModalSurface()).toBe(true);
+
+    document.body.innerHTML = '<div role="dialog" aria-modal="false"></div>';
+    expect(hasOpenModalSurface()).toBe(false);
+
+    document.body.innerHTML = '<div role="alertdialog" aria-label="inline notice"></div>';
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("stays false without a queryable root", () => {
+    expect(hasOpenModalSurface(null)).toBe(false);
+    expect(hasOpenModalSurface({} as ParentNode)).toBe(false);
+  });
+});
+
+describe("dismissOpenModalSurface", () => {
+  it("lets the top-most layer take the keystroke first, exactly like Esc", async () => {
+    const dialogOpen = ref(true);
+    const popoverOpen = ref(true);
+    mount(
+      defineComponent({
+        setup: () => () =>
+          h(Dialog, { open: dialogOpen.value, "onUpdate:open": (value: boolean) => (dialogOpen.value = value) }, () =>
+            h(DialogContent, {}, () => [h(Popover, { open: popoverOpen.value, "onUpdate:open": (value: boolean) => (popoverOpen.value = value) }, () => h(PopoverContent, {}, () => h("p", "popover")))]),
+          ),
+      }),
+    );
+    await nextTick();
+
+    expect(dismissOpenModalSurface()).toBe(true);
+    await nextTick();
+
+    // The popover above the dialog absorbs the keystroke, so the dialog stays open --
+    // same as pressing Esc once. (The next press reaches the dialog once the popover's
+    // exit animation has unmounted its layer, which happy-dom cannot run.)
+    expect(popoverOpen.value).toBe(false);
+    expect(dialogOpen.value).toBe(true);
+    expect(hasOpenModalSurface()).toBe(true);
+  });
+
+  it("closes the dialog that owns the window", async () => {
+    const open = ref(true);
+    mount(
+      defineComponent({
+        setup: () => () => h(Dialog, { open: open.value, "onUpdate:open": (value: boolean) => (open.value = value) }, () => h(DialogContent, {}, () => h("p", "body"))),
+      }),
+    );
+    await nextTick();
+
+    expect(dismissOpenModalSurface()).toBe(true);
+    await nextTick();
+
+    expect(open.value).toBe(false);
+    expect(hasOpenModalSurface()).toBe(false);
+  });
+
+  it("leaves the window alone when no modal is open", () => {
+    const dispatched: Event[] = [];
+    const target = { dispatchEvent: (event: Event) => dispatched.push(event) } as unknown as Window;
+
+    expect(dismissOpenModalSurface(target)).toBe(false);
+    expect(dispatched).toEqual([]);
+  });
+
+  it("stays inert without a dispatch target", () => {
+    document.body.innerHTML = '<div role="dialog" aria-modal="true"></div>';
+
+    expect(dismissOpenModalSurface(null)).toBe(false);
+    expect(hasOpenModalSurface()).toBe(true);
+  });
+});

apps/desktop/src/lib/ui/modalSurface.ts

@@ -0,0 +1,43 @@
+/**
+ * Whether a modal layer currently owns the window, so an app-level shortcut such as
+ * "close tab" (Cmd/Ctrl+W) is handed to that dialog instead of tearing down the surface
+ * behind it.
+ *
+ * Modal Reka dialogs render through `DialogContent.vue` / `DialogScrollContent.vue`,
+ * which mount `data-slot="dialog-overlay"` with the `data-state` Reka writes onto the
+ * layer. `data-state="open"` skips a layer that is only on screen for its exit
+ * animation.
+ *
+ * The overlay -- not the dialog body -- is the marker on purpose, because the overlay is
+ * what actually covers the app (Reka gives it `pointer-events: auto`). The one dialog
+ * that stays interactive renders no overlay at all: `DialogScrollContent` with
+ * `:show-overlay="false"`, used by the background table import. `role="dialog"` alone is
+ * not enough either: Reka popovers, date pickers and similar floating surfaces reuse that
+ * role and stay open while the user keeps working in the shell. Hand-rolled full-screen
+ * modals opt in with `aria-modal="true"`, while non-modal peek overlays pass
+ * `aria-modal="false"`.
+ */
+const OPEN_MODAL_SURFACE_SELECTOR = '[data-slot="dialog-overlay"][data-state="open"], [role="dialog"][aria-modal="true"]';
+
+/** True while a modal dialog layer is open above the current surface. */
+export function hasOpenModalSurface(root: ParentNode | null | undefined = globalThis.document ?? null): boolean {
+  if (!root || typeof root.querySelector !== "function") return false;
+  return root.querySelector(OPEN_MODAL_SURFACE_SELECTOR) !== null;
+}
+
+/**
+ * Hand "close this" to the modal dialog that owns the window, and report whether a modal
+ * took the shortcut. The keystroke is replayed as a cancelable Escape keydown on the
+ * window because that is the dismissal path every Reka layer already implements: only the
+ * top-most layer in the layer stack reacts, layer-specific guards
+ * (`@escape-key-down`, e.g. cancelling a pending inline edit first) still run, and focus
+ * returns where users expect it.
+ *
+ * So Cmd+W with a dialog open closes that dialog -- an open popover inside it first, the
+ * dialog itself on the next press -- instead of leaving the user with a dead shortcut.
+ */
+export function dismissOpenModalSurface(target: Pick<Window, "dispatchEvent"> | null | undefined = globalThis.window ?? null): boolean {
+  if (!target || typeof target.dispatchEvent !== "function" || !hasOpenModalSurface()) return false;
+  target.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true }));
+  return true;
+}

packages/app-tests/closeTabModalGuard.test.ts

@@ -0,0 +1,31 @@
+import { strict as assert } from "node:assert";
+import { readFileSync } from "node:fs";
+import { test } from "vitest";
+
+const app = readFileSync("apps/desktop/src/App.vue", "utf8");
+
+function functionBody(source: string, signature: string): string {
+  const start = source.indexOf(signature);
+  assert.notEqual(start, -1, `missing ${signature}`);
+  const end = source.indexOf("\n}", start);
+  assert.notEqual(end, -1, `unterminated ${signature}`);
+  return source.slice(start, end);
+}
+
+test("closing a tab stops at an open modal dialog", () => {
+  // Cmd+W and the native macOS "Close Tab" menu item both funnel into
+  // closeActiveSurface, so the modal handling has to live there.
+  const body = functionBody(app, "async function closeActiveSurface()");
+  assert.match(body, /if \(dismissOpenModalSurface\(\)\) return;/);
+  assert.match(app, /import \{ dismissOpenModalSurface \} from "@\/lib\/ui\/modalSurface";/);
+  assert.match(app, /if \(isCloseTabShortcut\(e, shortcuts\)\) \{\s*e\.preventDefault\(\);\s*await closeActiveSurface\(\);/);
+});
+
+test("the modal branch hands the keystroke to the dialog instead of dropping it", () => {
+  const body = functionBody(app, "async function closeActiveSurface()");
+  const guardIndex = body.indexOf("dismissOpenModalSurface()");
+  const settingsIndex = body.indexOf("showSettingsPage.value");
+  assert.notEqual(guardIndex, -1, "closeActiveSurface must dismiss an open modal");
+  assert.ok(guardIndex < settingsIndex, "the modal must be dismissed before the surface behind it closes");
+  assert.match(body, /if \(dismissOpenModalSurface\(\)\) return;/);
+});