fix: prevent ESC from closing settings when a nested modal is open#1512
Conversation
The global keyboard shortcut handler (useKeyboardShortcuts) runs in the capture phase on window and processes CLOSE_MODAL (Escape) before any nested dialog can handle it. When settings are open and a nested modal (e.g. agent execution settings) is also open, pressing Escape would close the entire settings page instead of just the modal. Fix: - useKeyboardShortcuts: skip modal-priority shortcuts when a nested dialog ([role=dialog/alertdialog]) is present in the DOM - CustomCommandModal: use capture phase + stopImmediatePropagation to prevent the SettingsPage ESC handler from also firing - SettingsPage: check defaultPrevented before closing as a safety net
|
@janburzinski is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes a bug where pressing Escape while a nested modal (e.g. the agent execution settings dialog) was open inside the settings page would close both the nested modal and the entire settings page simultaneously. The fix uses a three-layer defence:
The three layers are complementary and together correctly handle the primary use case. The main open concern (raised in a prior thread) is that the document-wide selector in Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/renderer/hooks/useKeyboardShortcuts.ts | Adds a document.querySelector('[role="dialog"], [role="alertdialog"]') guard before executing modal-priority shortcuts (Escape). Effectively prevents onCloseModal from firing when any dialog is present in the DOM anywhere on the page, including elements that are in the process of animating out via AnimatePresence — this scope is wider than the settings nesting context and was flagged in a prior review thread. |
| src/renderer/components/SettingsPage.tsx | Adds !e.defaultPrevented guard to the Escape keydown handler. Because nested modals now run in capture phase and call event.preventDefault() before this bubble-phase handler fires, the guard correctly prevents settings from closing when a nested modal has already consumed the event. |
| src/renderer/components/CustomCommandModal.tsx | Moves the keydown listener from bubble phase to capture phase (adds true flag). Escape now calls event.preventDefault() in capture phase, which both stops SettingsPage's bubble-phase handler from firing and satisfies the defaultPrevented check. Change is correct and intentional. |
| src/renderer/components/FeedbackModal.tsx | Same capture-phase migration as CustomCommandModal. FeedbackModal is not imported by SettingsPage, so if the two views can be open simultaneously the change is necessary; if they are mutually exclusive it is harmless. The change is safe either way. |
Sequence Diagram
sequenceDiagram
participant User
participant useKeyboardShortcuts as useKeyboardShortcuts<br/>(window, capture, init-time)
participant NestedModal as NestedModal<br/>(CustomCommandModal / FeedbackModal)<br/>(window, capture, on-open)
participant SettingsPage as SettingsPage<br/>(window, bubble)
Note over User,SettingsPage: Scenario A — Escape with nested modal open (fixed case)
User->>useKeyboardShortcuts: keydown Escape (capture phase)
useKeyboardShortcuts->>useKeyboardShortcuts: document.querySelector('[role="dialog"]') → found
useKeyboardShortcuts-->>User: continue (skip onCloseModal, no preventDefault)
User->>NestedModal: keydown Escape (capture phase)
NestedModal->>NestedModal: event.preventDefault() + onClose()
NestedModal-->>User: modal closed
User->>SettingsPage: keydown Escape (bubble phase)
SettingsPage->>SettingsPage: e.defaultPrevented === true → skip onClose()
Note over SettingsPage: ✅ Settings stays open
Note over User,SettingsPage: Scenario B — Escape with NO nested modal open
User->>useKeyboardShortcuts: keydown Escape (capture phase)
useKeyboardShortcuts->>useKeyboardShortcuts: document.querySelector('[role="dialog"]') → null
useKeyboardShortcuts->>useKeyboardShortcuts: hasClosableView === true (settings open)
useKeyboardShortcuts->>useKeyboardShortcuts: event.preventDefault() + onCloseModal()
Note over useKeyboardShortcuts: Settings closed via onCloseModal
User->>SettingsPage: keydown Escape (bubble phase)
SettingsPage->>SettingsPage: e.defaultPrevented === true → skip onClose()
Note over SettingsPage: ✅ No double-close
Last reviewed commit: 5b21585
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (event.key === 'Escape') { | ||
| event.preventDefault(); | ||
| event.stopImmediatePropagation(); | ||
| onClose(); |
There was a problem hiding this comment.
stopImmediatePropagation() does not block useKeyboardShortcuts
The comment on line 51–52 states that stopImmediatePropagation() prevents the SettingsPage ESC handler from firing, but this reasoning is incorrect. useKeyboardShortcuts is registered on window in the capture phase at app initialization — long before CustomCommandModal opens and adds its own capture-phase listener. Because stopImmediatePropagation() only blocks listeners registered after the current one, it cannot stop useKeyboardShortcuts, which runs first.
The actual protection for useKeyboardShortcuts comes from the document.querySelector('[role="dialog"]') check added in useKeyboardShortcuts.ts. And the protection for SettingsPage.tsx comes from its new !e.defaultPrevented guard (which does work, because event.preventDefault() is called here before the bubble phase).
stopImmediatePropagation() as used here is misleading and unnecessary for the stated purpose. If left in, the comment should be corrected to avoid future confusion. If removed, the fix still works via the other two mechanisms.
| const nestedDialog = document.querySelector('[role="dialog"], [role="alertdialog"]'); | ||
| if (nestedDialog) continue; |
There was a problem hiding this comment.
document.querySelector('[role="dialog"], [role="alertdialog"]') searches the entire document, not just the settings context. This means that any role="dialog" element present anywhere in the UI — including during an AnimatePresence exit animation (where the element remains in the DOM while fading out) — will silently swallow the Escape key for ALL priority: 'modal' shortcuts.
Concretely, if the user has a non-settings closable view open (e.g. browser preview, diff viewer, editor, kanban) and some unrelated role="dialog" element happens to be in the DOM, pressing Escape will not trigger onCloseModal through this path. The user loses keyboard control.
A safer approach would be to scope the check: for example, only skip if the dialog is a descendant of the settings container, or use a dedicated state flag rather than querying the DOM:
// Instead of a document-wide DOM query, thread an explicit flag:
if (shortcut.priority === 'modal') {
if (handlers.isNestedModalOpen) continue;
// ...
}This makes the intent explicit, avoids timing issues with animations, and prevents interference with unrelated views.
|
@greptile do your thing again hehe |
2 participants
Summary
right now if you are in the settings and open another modal and then press escape the expected behavior would be that only the newly opened modal would be closed and not the entire settings. this was not the case. if you were in the settings and then opened another modal and then pressed escape it would close the modal and the entire settings with it.
Snapshot
Screen_Recording_2026-03-17_at_20.22.56.mov
Type of change
Mandatory Tasks
Checklist
pnpm run format)pnpm run lint)