Skip to content

feat: Escape should close windows#1271

Merged
arnestrickmann merged 5 commits intogeneralaction:mainfrom
jschwxrz:emdash/feat-esc-closes-windows-5o9
Mar 4, 2026
Merged

feat: Escape should close windows#1271
arnestrickmann merged 5 commits intogeneralaction:mainfrom
jschwxrz:emdash/feat-esc-closes-windows-5o9

Conversation

@jschwxrz
Copy link
Collaborator

@jschwxrz jschwxrz commented Mar 4, 2026

summary:

  • browser, diffviewer, editor and kanban could only be closed via their toggle shortcut or close button
  • users expect escape to close them same as command palette and settings

changes:

  • extended the existing CLOSE_MODAL escape system with a priority chain: command palette > settings > browser > diffviewer > editor > kanban
  • broadened hasClosableView gate in useKeyboardShortcuts to include all six dismissable views
  • added BrowserAwareShortcuts bridge component in workspace to access BrowserProvider context (same pattern as existing RightSidebarBridge)
  • deduplicated DiffViewer close logic into a shared useCallback

@vercel
Copy link

vercel bot commented Mar 4, 2026

@jschwxrz is attempting to deploy a commit to the General Action Team on Vercel.

A member of the Team first needs to authorize it.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR extends the existing CLOSE_MODAL / Escape keyboard shortcut system to cover four additional dismissable views — browser, diff viewer, editor, and kanban — using a priority chain (command palette > settings > browser > diffviewer > editor > kanban). The implementation is clean and follows established patterns (e.g., BrowserAwareShortcuts mirrors the existing RightSidebarBridge).

Key changes:

  • onCloseModal in AppKeyboardShortcuts now resolves via a .find() priority array instead of a nested ternary — more readable and easily extensible.
  • hasClosableView in useKeyboardShortcuts correctly gates the modal-priority Escape shortcut on any of the six dismissable views being open.
  • handleCloseDiffViewer was properly extracted into a useCallback and reused on the <DiffViewer> onClose prop.

Two issues found: (1) <CodeEditor>'s onClose still uses an inline function () => setShowEditorMode(false) rather than the newly defined handleCloseEditor — an inconsistency with the DiffViewer deduplication; (2) showEditor (= showEditorMode) is passed as isEditorOpen even when the <CodeEditor> is not rendered (requires activeTask && selectedProject), meaning hasClosableView can return true and Escape can be silently consumed with no visible effect.

Confidence Score: 4/5

  • Safe to merge after addressing the isEditorOpen ghost-state edge case; the rest of the implementation is solid.
  • The core logic is correct and the pattern is well-established. The main concern is the isEditorOpen flag being true when no editor is actually rendered, which can silently consume Escape keypresses in an invisible state. The missed handleCloseEditor deduplication on CodeEditor is a minor consistency nit. Neither issue causes a crash or data loss, but the ghost-state issue is a real behavioral edge case worth fixing.
  • src/renderer/views/Workspace.tsx — the showEditor prop passed to BrowserAwareShortcuts and the CodeEditor onClose prop.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([User presses Escape]) --> B{Target is editable?\ninput / textarea / contenteditable}
    B -- Yes --> Z([Keypress ignored by shortcut system])
    B -- No --> C{hasClosableView?\ncommandPalette OR settings OR\nbrowser OR diffViewer OR editor OR kanban}
    C -- No --> Z
    C -- Yes --> D[Resolve onCloseModal\nvia priority chain]
    D --> E{showCommandPalette?}
    E -- Yes --> F([handleCloseCommandPalette])
    E -- No --> G{showSettings?}
    G -- Yes --> H([handleCloseSettings])
    G -- No --> I{showBrowser?}
    I -- Yes --> J([handleCloseBrowser])
    I -- No --> K{showDiffViewer?}
    K -- Yes --> L([handleCloseDiffViewer])
    K -- No --> M{showEditor?}
    M -- Yes --> N([handleCloseEditor])
    M -- No --> O{showKanban?}
    O -- Yes --> P([handleCloseKanban])
    O -- No --> Z
Loading

Last reviewed commit: 238e274

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 4, 2026

Additional Comments (1)

src/renderer/views/Workspace.tsx
The <CodeEditor> component's onClose prop uses an inline function, which duplicates the logic already defined in the handleCloseEditor callback. For consistency with how <DiffViewer> is handled (which uses the deduplicated handleCloseDiffViewer on line 400), this should use the callback instead:

                  onClose={handleCloseEditor}

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@jschwxrz
Copy link
Collaborator Author

jschwxrz commented Mar 4, 2026

fixed greptile issues, should be ready to merge

@arnestrickmann
Copy link
Contributor

Thanks @jschwxrz,
great fix.

@arnestrickmann arnestrickmann merged commit 70e2f9d into generalaction:main Mar 4, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants