fix(ui): stop Cmd+W deleting a thread out from under a dialog (#2474)

src/renderer/main.test.ts

@@ -14,3 +14,48 @@ describe('main layout boot', () => {
     )
   })
 })
+
+// #2474: Cmd/Ctrl+W deletes the active thread, and with Settings — or any other
+// dialog — on screen the keystroke a user meant as "close this" destroyed a
+// conversation instead. main.ts binds its shortcuts inside a boot function with
+// no seam to call, so the wiring is pinned at the source level (as the layout
+// test above does); the gate itself is unit-tested in
+// `views/dialog-shell.test.ts`.
+describe('destructive shortcuts defer to an open dialog', () => {
+  const src = readFileSync(join(process.cwd(), 'src/renderer/main.ts'), 'utf8')
+
+  it('never calls the thread delete without asking whether a dialog is open', () => {
+    // Every call, not just the one: a second unguarded call site would
+    // reintroduce the bug somewhere the first assertion never looks.
+    const calls = [...src.matchAll(/^.*confirmDeleteThread\(\).*$/gm)]
+      .map((m) => m[0])
+      .filter((line) => !line.includes('function confirmDeleteThread'))
+    assert.ok(calls.length > 0, 'expected main.ts to invoke confirmDeleteThread')
+    const unguarded = calls.filter((line) => !line.includes('!isAnyDialogOpen()'))
+    assert.deepEqual(
+      unguarded,
+      [],
+      `these delete the active thread without checking for an open dialog:\n${unguarded.join('\n')}`,
+    )
+  })
+
+  it('still swallows the keystroke whether or not the delete runs', () => {
+    // preventDefault is what keeps Cmd+W from also reaching the File ▸ Close
+    // accelerator. Moving it inside the guard would hand the keystroke back to
+    // the menu exactly when a dialog is up.
+    const block = /if \(meta && e\.key === 'w'\) \{([\s\S]*?)\n {4}\}/.exec(src)?.[1]
+    assert.ok(block, 'could not find the Cmd/Ctrl+W handler')
+    const preventIndex = block.indexOf('e.preventDefault()')
+    const guardIndex = block.indexOf('isAnyDialogOpen()')
+    assert.ok(preventIndex >= 0, 'the Cmd/Ctrl+W handler must preventDefault')
+    assert.ok(guardIndex >= 0, 'the Cmd/Ctrl+W handler must consult isAnyDialogOpen')
+    assert.ok(preventIndex < guardIndex, 'preventDefault must run before the dialog guard')
+  })
+
+  it('does not open the find bar underneath a dialog either', () => {
+    // The same gate, on the shortcut that used to name four dialogs of seventeen.
+    const block = /if \(matchFindInChatShortcut\(e\)\) \{([\s\S]*?)\n {4}\}/.exec(src)?.[1]
+    assert.ok(block, 'could not find the find-in-chat handler')
+    assert.match(block, /if \(isAnyDialogOpen\(\)\) return/)
+  })
+})

src/renderer/main.ts

@@ -34,6 +34,7 @@ import { mountTerminalRailResizers } from './views/terminal-rail-resizer.ts'
 import { mountRoadmapPane } from './views/roadmap-pane.ts'
 import { mountBrowserPane } from './views/browser-pane.ts'
 import { mountVncPane } from './views/vnc-pane.ts'
+import { isAnyDialogOpen } from './views/dialog-shell.ts'
 import {
   mountSettingsDialog,
   openSettingsDialog,
@@ -653,15 +654,12 @@ function registerKeyboardShortcuts(): void {
       openCommandPalette()
     }
     // Cmd/Ctrl+F opens the in-conversation find bar (find-in-page for the chat).
-    // Skipped while a modal dialog owns the screen so it can't open behind it.
+    // Skipped while a dialog owns the screen so it can't open behind it. This
+    // named four dialogs of the seventeen the renderer has; asking the DOM
+    // covers the rest — an approval, an SSH passphrase, a confirm — each of
+    // which is a question the find bar should not open underneath.
     if (matchFindInChatShortcut(e)) {
-      if (
-        isFileSearchDialogOpen() ||
-        isCommandPaletteOpen() ||
-        isSettingsDialogOpen() ||
-        isKeyboardShortcutsDialogOpen()
-      )
-        return
+      if (isAnyDialogOpen()) return
       e.preventDefault()
       openConversationSearch()
     }
@@ -684,9 +682,16 @@ function registerKeyboardShortcuts(): void {
       if (uiScaleAction === 'reset') void resetUiScale(store, api)
       else void bumpUiScale(store, api, uiScaleAction === 'in' ? 1 : -1)
     }
+    // Cmd/Ctrl+W deletes the active thread. `preventDefault` stays
+    // unconditional — it is what keeps the keystroke from also reaching the
+    // File ▸ Close accelerator — but with a dialog on screen the delete does
+    // not run: a user closing Settings with Cmd+W meant "close this", not
+    // "destroy this conversation" (#2474). Doing nothing is the right answer
+    // rather than closing the dialog for them; Esc already does that, right
+    // below, and every dialog handles it.
     if (meta && e.key === 'w') {
       e.preventDefault()
-      void confirmDeleteThread()
+      if (!isAnyDialogOpen()) void confirmDeleteThread()
     }
     if (e.key === 'Escape') {
       if (isCommandPaletteOpen()) {

src/renderer/views/dialog-shell.test.ts

@@ -1,7 +1,7 @@
 import '../../../tests/setup-dom.ts'
 import { describe, it, beforeEach } from 'node:test'
 import assert from 'node:assert/strict'
-import { createOverlayDialog } from './dialog-shell.ts'
+import { createOverlayDialog, isAnyDialogOpen } from './dialog-shell.ts'
 
 describe('createOverlayDialog', () => {
   beforeEach(() => {
@@ -44,3 +44,53 @@ describe('createOverlayDialog', () => {
     assert.equal(closes, 1)
   })
 })
+
+describe('isAnyDialogOpen', () => {
+  beforeEach(() => {
+    document.body.innerHTML = ''
+  })
+
+  it('is false with no dialogs, and false when one exists but is closed', () => {
+    assert.equal(isAnyDialogOpen(), false)
+    createOverlayDialog({ id: 'closed-overlay' })
+    assert.equal(isAnyDialogOpen(), false)
+  })
+
+  it('is true while a modal dialog is open, and false again once it closes', () => {
+    const shell = createOverlayDialog({ id: 'modal-overlay' })
+    shell.open()
+    assert.equal(isAnyDialogOpen(), true)
+    shell.close()
+    assert.equal(isAnyDialogOpen(), false)
+  })
+
+  it('counts a non-modal dialog too', () => {
+    // The approval prompt shows inline over the chat with `show()`, not
+    // `showModal()`. It is still a question the user is answering, so a global
+    // shortcut must not act on the transcript behind it.
+    const dialog = document.createElement('dialog')
+    document.body.append(dialog)
+    dialog.show()
+    assert.equal(isAnyDialogOpen(), true)
+    dialog.close()
+    assert.equal(isAnyDialogOpen(), false)
+  })
+
+  it('stays true while any one of several dialogs is still open', () => {
+    const first = createOverlayDialog({ id: 'first-overlay' })
+    const second = createOverlayDialog({ id: 'second-overlay' })
+    first.open()
+    second.open()
+    first.close()
+    assert.equal(isAnyDialogOpen(), true, 'the second dialog is still on screen')
+    second.close()
+    assert.equal(isAnyDialogOpen(), false)
+  })
+
+  it('does not need to know the dialog exists', () => {
+    // The point of reading the DOM: a dialog built anywhere, by anything, is
+    // covered without being registered (#2474).
+    document.body.insertAdjacentHTML('beforeend', '<dialog open id="ad-hoc"></dialog>')
+    assert.equal(isAnyDialogOpen(), true)
+  })
+})

src/renderer/views/dialog-shell.ts

@@ -37,3 +37,25 @@ export function createOverlayDialog(opts: { id: string; className?: string }): O
     isOpen: (): boolean => dialog.open,
   }
 }
+
+/**
+ * Whether any `<dialog>` in the document is currently open.
+ *
+ * Asked by global keyboard shortcuts, which must defer to whatever the user is
+ * answering rather than act on the screen behind it. Cmd/Ctrl+W is the case
+ * that made this necessary: it deletes the active thread, and with Settings —
+ * or any other dialog — on screen the keystroke a user meant as "close this"
+ * destroyed a conversation instead (#2474).
+ *
+ * Read off the DOM rather than from a list of `isXOpen()` predicates. There are
+ * seventeen dialogs in the renderer and the one hand-maintained list of them
+ * named four, which is the failure this is shaped to avoid: a new dialog is
+ * covered the moment it exists, without anyone remembering to add it here.
+ *
+ * The `open` attribute is set by both `show()` and `showModal()`, so a
+ * non-modal prompt sitting over the chat counts too — it is still a question
+ * the user is in the middle of.
+ */
+export function isAnyDialogOpen(): boolean {
+  return document.querySelector('dialog[open]') !== null
+}

tests/e2e/dialog-shortcuts.e2e.ts

@@ -0,0 +1,87 @@
+import { $, $$, browser, expect } from '@wdio/globals'
+import { resetUserData, seedE2eViewport, writeSeedConfig } from './helpers/seed-config.ts'
+import { saveElementScreenshot } from './helpers/screenshot.ts'
+
+const PROJECT_ID = 'e2e-dialog-shortcuts'
+
+describe('dialog keyboard shortcuts', () => {
+  before(async function () {
+    this.timeout(90_000)
+    resetUserData()
+    const now = Date.now()
+    writeSeedConfig({
+      projects: [{ id: PROJECT_ID, path: process.cwd(), name: 'workspace' }],
+      activeProjectId: PROJECT_ID,
+      activeThreadId: 'thread-b',
+      [`threads:${PROJECT_ID}`]: [
+        {
+          id: 'thread-a',
+          title: 'Keep me',
+          status: 'idle',
+          messages: [
+            {
+              id: 'msg-a',
+              role: 'user',
+              content: 'Keep this conversation.',
+              toolCalls: [],
+              createdAt: now,
+            },
+          ],
+          usage: { inputTokens: 0, outputTokens: 0 },
+          createdAt: now,
+          updatedAt: now,
+        },
+        {
+          id: 'thread-b',
+          title: 'Active conversation',
+          status: 'idle',
+          messages: [
+            {
+              id: 'msg-b',
+              role: 'user',
+              content: 'Keep this active conversation too.',
+              toolCalls: [],
+              createdAt: now + 1,
+            },
+          ],
+          usage: { inputTokens: 0, outputTokens: 0 },
+          createdAt: now + 1,
+          updatedAt: now + 1,
+        },
+      ],
+    })
+    seedE2eViewport()
+    await browser.reloadSession()
+    await $('.prompt-input').waitForExist({ timeout: 60_000 })
+  })
+
+  after(() => {
+    resetUserData()
+  })
+
+  it('does not delete the active thread while Settings is open', async () => {
+    await browser.waitUntil(async () => (await $$('.chats-list .chat-row')).length === 2, {
+      timeout: 15_000,
+      timeoutMsg: 'expected two seeded chat rows',
+    })
+    await expect($('.chat-row.selected .chat-title')).toHaveText('Active conversation')
+
+    await $('[aria-label="Settings"]').click()
+    const settings = $('#settings-dialog')
+    await settings.waitForDisplayed({ timeout: 10_000 })
+
+    // Dispatch in-page: Electron may consume a physical Cmd/Ctrl+W as the native
+    // File ▸ Close accelerator before the renderer shortcut handler sees it.
+    await browser.execute(() => {
+      document.dispatchEvent(
+        new KeyboardEvent('keydown', { key: 'w', metaKey: true, ctrlKey: true, bubbles: true }),
+      )
+    })
+
+    await expect(settings).toBeDisplayed()
+    await expect($('#confirm-dialog')).not.toBeDisplayed()
+    await expect($('.chat-row.selected .chat-title')).toHaveText('Active conversation')
+    expect(await $$('.chats-list .chat-row')).toHaveLength(2)
+    await saveElementScreenshot('#settings-dialog', 'cmd-w-settings-dialog-safe.png')
+  })
+})