fix(desktop): fix auto-updater unable to quit the app#3253
Merged
Conversation
Two regressions introduced in #3157 (Host Service): 1. Host-service cleanup in before-quit had no error handling. If getHostServiceManager(), releaseAll(), or disposeTray() threw, app.exit(0) was never reached. Combined with the macOS window close handler (also added in #3157) that calls event.preventDefault() + window.hide(), this blocked the quit entirely. 2. installUpdate() used prepareQuit("release") to participate in the quit protocol, but the updater doesn't need to — it can exit directly after quitAndInstall(). Fixes: - Wrap before-quit cleanup in try/catch so app.exit(0) always runs - Replace prepareQuit("release") with exitImmediately() in the auto-updater, reverting to the pre-#3157 approach where the updater bypasses the quit protocol entirely - Update stale plan doc to reflect actual shipped state
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks macOS quit/tray lifecycle: removes quit-mode API in favor of a single exported Changes
Sequence Diagram(s)sequenceDiagram
participant Tray as Tray Menu
participant Main as Main Process
participant Host as HostServiceManager
participant OS as macOS / Squirrel
Tray->>Main: user clicks "Quit Superset" -> quitApp()
Main->>Main: set skipQuitConfirmation, call app.quit()
Main->>Main: before-quit handler runs
Main->>Host: releaseAll() -- wrapped in try/catch -->
Main->>Main: disposeTray() -- wrapped in try/catch -->
Main->>OS: app.exit(0)
sequenceDiagram
participant Updater as AutoUpdater
participant Main as Main Process
participant OS as macOS / Squirrel
Updater->>Main: user selects "Install Update"
Main->>Updater: autoUpdater.quitAndInstall(false, true)
Updater->>Main: (may not quit on macOS immediately)
Main->>Main: exitImmediately()
Main->>OS: process exits to allow installer to run
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
5-5: Circular import betweenindex.tsandauto-updater.ts.This import creates a circular dependency:
index.ts→auto-updater.ts→index.ts. It's safe at runtime becauseexitImmediatelyis only accessed wheninstallUpdate()is called (after both modules are initialized), but circular dependencies can make the codebase harder to reason about.Consider extracting
exitImmediately,requestQuit, andprepareQuitinto a dedicated module (e.g.,lib/quit.ts) that both files can import from.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/auto-updater.ts` at line 5, There is a circular import caused by importing exitImmediately from index.ts into auto-updater.ts; extract the shared quit-related functions into a new module (e.g., lib/quit.ts) that exports exitImmediately, requestQuit, and prepareQuit, then update both index.ts and auto-updater.ts to import those symbols from lib/quit.ts (leave installUpdate in auto-updater.ts untouched but call the relocated exitImmediately from the new module). Ensure the new quit module has the same API and update any imports/usages in index.ts and auto-updater.ts to avoid the circular dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/auto-updater.ts`:
- Line 5: There is a circular import caused by importing exitImmediately from
index.ts into auto-updater.ts; extract the shared quit-related functions into a
new module (e.g., lib/quit.ts) that exports exitImmediately, requestQuit, and
prepareQuit, then update both index.ts and auto-updater.ts to import those
symbols from lib/quit.ts (leave installUpdate in auto-updater.ts untouched but
call the relocated exitImmediately from the new module). Ensure the new quit
module has the same API and update any imports/usages in index.ts and
auto-updater.ts to avoid the circular dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebac53c2-e418-48aa-aa40-b0934a398d36
📒 Files selected for processing (3)
apps/desktop/plans/20260405-quit-tray-lifecycle.mdapps/desktop/src/main/index.tsapps/desktop/src/main/lib/auto-updater.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance. |
The QuitMode system ("release" | "stop") was introduced in #3157 and
added unnecessary complexity to the quit lifecycle. It coupled the
auto-updater, tray, and before-quit handler through a shared mutable
state (pendingQuitMode) that was consumed on each before-quit call.
Revert to the simpler pre-#3157 approach:
- Replace QuitMode/requestQuit/prepareQuit with a simple
skipQuitConfirmation flag and quitApp() helper
- Always releaseAll() on quit (services survive as detached processes)
- Tray menu simplified to single "Quit Superset" item
- before-quit cleanup wrapped in try/catch so app.exit(0) always runs
- Auto-updater uses exitImmediately() — bypasses quit protocol entirely
Workspace names with special characters (e.g. apostrophes) break PlistBuddy commands. Use an allowlist (alphanumeric, spaces, hyphens) instead of stripping individual characters.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 10, 2026
…m upstream superset-sh#3253) Cherry-picked from upstream 146c86d (only the patch-dev-protocol.ts sanitize hunk). The rest of superset-sh#3253 is a quit lifecycle rewrite that conflicts with fork's requestQuit(mode) system and is intentionally NOT being taken. Workspace names with special characters (e.g. apostrophes) break PlistBuddy commands. Use an allowlist (alphanumeric, spaces, hyphens) instead of stripping individual characters.
Merged
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
before-quithost-service cleanup in try/catch soapp.exit(0)always runsprepareQuit("release")withexitImmediately()in the auto-updater — bypasses the quit protocol entirely (pre-Host service #3157 approach)Root Cause
Two regressions from #3157 (Host Service):
Unguarded cleanup before
app.exit(0)—getHostServiceManager(),releaseAll(), anddisposeTray()were added beforeapp.exit(0)without try/catch. If any threw,app.exit(0)was never reached. Combined with the macOS window close handler (also added in Host service #3157) that callsevent.preventDefault()+window.hide(), the quit was blocked entirely. Pre-Host service #3157, the window close handler didn't prevent close, so cleanup failures didn't matter.Updater coupled to quit protocol —
installUpdate()usedprepareQuit("release")to set a one-shot quit mode, but the updater doesn't need to participate in the quit lifecycle. Pre-Host service #3157 it used a simplesetSkipQuitConfirmation()flag. Now it just callsexitImmediately()afterquitAndInstall(), bypassingbefore-quitentirely.#3205 fixed part of the problem (removed the background-to-tray block) but didn't address these two issues.
Test plan
Summary by cubic
Fixes the desktop auto-updater failing to quit by simplifying and hardening app shutdown. Quits now always succeed (Cmd+Q and tray), services are released, and updates install reliably.
before-quitcleanup in try/catch soapp.exit(0)always runs; always callreleaseAll()and log cleanup errors.quitAndInstall()thenexitImmediately()(dropprepareQuit("release")).QuitModeand pending state; addquitApp()to skip confirmation; simplify tray to a single "Quit Superset"; update the macOS quit/tray plan doc.PlistBuddyerrors in dev builds.Written for commit 324fc60. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Chores