fix(quit): target main window for dirty-editor check on quit#840
Conversation
Use getMainWindow() instead of BrowserWindow.getAllWindows()[0] so the app:query-dirty-editors round-trip isn't sent to the tray panel or settings window, and skip the check when the main window is hidden to avoid the 5s timeout fallback during tray-initiated quit.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e43958a66
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!win || win.isDestroyed?.() || !win.isVisible?.()) { | ||
| commitQuit(); |
There was a problem hiding this comment.
Do not skip the dirty-editor IPC check solely because mainWindow is hidden: in the close-to-tray flow, users can still have unsaved editor tabs, and this branch now calls commitQuit() immediately, so tray "Quit" exits without any dirty-state confirmation. That creates a direct data-loss path for hidden-to-tray sessions; this should still run a dirty check (or surface an explicit confirmation) instead of unconditional quit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Electron quit-guard logic to avoid the 5-second fallback timeout when quitting from the system tray by ensuring the dirty-editor query targets the app’s main window (instead of an arbitrary first window that might not have the listener).
Changes:
- Switch the dirty-editor check target from
BrowserWindow.getAllWindows()[0]togetWindowManager().getMainWindow(). - Add additional early-exit conditions intended to skip the renderer round-trip when the main window is absent/hidden.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const win = getWindowManager().getMainWindow(); | ||
| // No main window, or it's hidden (tray-panel "Quit" path) — there's no | ||
| // visible UI to surface a "save first" toast on, so skip the round-trip | ||
| // and quit directly. The renderer's dirty-editor check exists to warn the | ||
| // user; if they can't see the warning, it's just dead 5-second wait. | ||
| if (!win || win.isDestroyed?.() || !win.isVisible?.()) { |
There was a problem hiding this comment.
before-quit only checks win.isDestroyed() before calling win.webContents.send(...). In this codebase there are multiple safeSend patterns that also guard win.webContents.isDestroyed() (e.g. windowManager), because a window can exist while its render frame/webContents is disposed (HMR reload, renderer crash). Consider checking win.webContents + !win.webContents.isDestroyed() (and/or wrapping the send in try/catch) and falling back to commitQuit() if the renderer can’t receive the query, to avoid throwing and preventing quit.
| // No main window, or it's hidden (tray-panel "Quit" path) — there's no | ||
| // visible UI to surface a "save first" toast on, so skip the round-trip | ||
| // and quit directly. The renderer's dirty-editor check exists to warn the | ||
| // user; if they can't see the warning, it's just dead 5-second wait. | ||
| if (!win || win.isDestroyed?.() || !win.isVisible?.()) { |
There was a problem hiding this comment.
The new !win.isVisible?.() fast-path will also be true when the main window is minimized/hidden for reasons other than the tray-panel quit flow. That bypasses the dirty-editor round-trip and can allow the app to quit with unsaved editor tabs (data loss). Since the main issue was selecting the wrong window, it’s safer to always query the main window when it exists (even if hidden), or alternatively restore/show the window (or present a native dialog) when hasDirty is true rather than skipping the check based on visibility.
| // No main window, or it's hidden (tray-panel "Quit" path) — there's no | |
| // visible UI to surface a "save first" toast on, so skip the round-trip | |
| // and quit directly. The renderer's dirty-editor check exists to warn the | |
| // user; if they can't see the warning, it's just dead 5-second wait. | |
| if (!win || win.isDestroyed?.() || !win.isVisible?.()) { | |
| // Only skip the round-trip when there is no usable main window. Hidden or | |
| // minimized main windows must still answer app:query-dirty-editors so we | |
| // don't bypass unsaved-editor protection and quit with dirty tabs. | |
| if (!win || win.isDestroyed?.()) { |
…ness A minimized main window has a taskbar/Dock entry the user can click to restore, so the dirty-editor toast is still useful even though the window isn't currently in the foreground. On some platforms isVisible() can return false for a minimized window (see the comment at globalShortcutBridge.cjs:478), so the original `!isVisible()` short-circuit would silently lose dirty-editor protection in that case. Treat a window as "reachable by the user" when either isVisible() or isMinimized() is true. Truly hidden windows (close-to-tray, app.hide() on macOS) still skip the round-trip and quit instantly, which is the behaviour this PR set out to introduce. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Harden the dirty-editor quit guard Follow-up to #840. Three concrete failure modes that round-2 review turned up: 1. `webContents.send` is unguarded. If the renderer is destroyed between the reachability check and the send (e.g. a dying GPU process), the throw escapes the `before-quit` handler with `quitGuardChannelBusy = true` already set and no timeout scheduled yet — the app becomes un-quittable until restart. Wrap the send, and tear the listener/timer down on failure. 2. The timeout vs. response race silently commits a quit on `hasDirty=true`. Once `setTimeout` has already enqueued its callback for the next tick, `clearTimeout` is a no-op and the timeout callback runs even after the response arrived — which unconditionally calls `commitQuit()`, overriding the user's "save first" intent. Funnel both paths through a `settle()` helper that only acts the first time it's called. 3. The reply listener accepted any sender. A rogue or future-buggy `webContents` could decide the quit by sending the channel name first. Validate `evt.sender === wc` and ignore non-matches; switch from `.once` to `.on` + explicit `removeListener` so a rogue early reply doesn't consume the listener slot. Also wrap the renderer-side handler in try/catch so an unexpected throw inside `editorTabStore.getTabs()` reports `hasDirty=false` immediately instead of stranding the main process for 5 s on a silent timeout. Verify `webContents.isCrashed()` before sending so a known-dead renderer skips the round-trip and quits instantly instead of waiting on the timeout fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Tighten dirty-editor quit-guard validation Codex round-2-2 review suggested two small follow-ons: 1. Sender check should reject missing/falsy `evt.sender` outright. In real Electron IPC the sender is always populated; a falsy sender is anomalous and treating it as legit defeats the rogue-reply defence we just added. 2. Wrap `bridge.reportDirtyEditorsResult` in try/catch on the renderer side. If the IPC bridge is in a bad state and the call throws, the rest of the listener body is fine but the React useEffect callback would propagate the error — and an uncaught error in the listener would silently disable the quit guard for the rest of the session. Both are pure tightening; no behaviour change on the happy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
解决在系统托盘退出应用等待5秒的问题