feat(desktop): add tray "Quit Superset Completely" for full shutdown#4212
Conversation
Adds a nuclear-quit tray entry that kills host-service(s) and the pty-daemon/terminal-host alongside the Electron app. The existing "Quit Superset" remains a soft quit so services survive for re-adoption. Reuses runDevQuitCleanup() via a forceFullCleanup flag in before-quit.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a "Quit Superset Completely" feature: a new exported ChangesComplete App Quit Flow
Sequence Diagram(s)sequenceDiagram
participant User
participant TrayMenu
participant Dialog
participant QuitAPI
participant ElectronApp
User->>TrayMenu: click "Quit Superset Completely"
TrayMenu->>Dialog: show confirmation
Dialog-->>TrayMenu: user confirms
TrayMenu->>QuitAPI: invoke quitAppCompletely()
QuitAPI->>ElectronApp: app.quit()
ElectronApp->>QuitAPI: before-quit handler checks forceFullCleanup
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds a "Quit Superset Completely" tray menu entry that performs a full shutdown — killing host-services and the pty-daemon — by reusing the existing
Confidence Score: 4/5Safe to merge; the new quit path correctly reuses the established cleanup sequence and cannot interfere with the soft-quit or auto-update flows. The logic change is small and well-contained — a boolean flag plus one extra branch in apps/desktop/src/main/lib/tray/index.ts — menu item layout and the unused tooltip.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/index.ts | Adds forceFullCleanup module flag and quitAppCompletely() export; gates runDevQuitCleanup() on `isDev |
| apps/desktop/src/main/lib/tray/index.ts | Adds "Quit Superset Completely" menu item wired to quitAppCompletely(). Missing separator between the two quit items makes accidental destructive click easy; toolTip is silently ignored by macOS tray context menus. |
Sequence Diagram
sequenceDiagram
participant User
participant Tray
participant Main as main/index.ts
participant Coordinator as HostServiceCoordinator
participant TermHost as TerminalHostClient
Note over User,TermHost: Soft quit path (existing)
User->>Tray: click "Quit Superset"
Tray->>Main: quitApp()
Main->>Main: setSkipQuitConfirmation()
Main->>Main: "app.quit() -> before-quit"
Main->>Coordinator: releaseAll() [services survive]
Main->>Main: app.exit(0)
Note over User,TermHost: Nuclear quit path (new)
User->>Tray: click "Quit Superset Completely"
Tray->>Main: quitAppCompletely()
Main->>Main: "forceFullCleanup = true"
Main->>Main: setSkipQuitConfirmation()
Main->>Main: "app.quit() -> before-quit"
Main->>Coordinator: stopAll() [services killed]
Main->>TermHost: "shutdownIfRunning({ killSessions: true })"
Main->>Main: disposeTerminalHostClient()
Main->>Main: app.exit(0)
Comments Outside Diff (1)
-
apps/desktop/src/main/lib/tray/index.ts, line 231-239 (link)No visual separator before the destructive quit action
"Quit Superset Completely" immediately follows "Quit Superset" with no separator, making it easy to accidentally click the destructive action (which kills all background services). A separator line gives the user a visual pause and distinguishes the soft quit from the nuclear one.
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/lib/tray/index.ts Line: 231-239 Comment: **No visual separator before the destructive quit action** "Quit Superset Completely" immediately follows "Quit Superset" with no separator, making it easy to accidentally click the destructive action (which kills all background services). A separator line gives the user a visual pause and distinguishes the soft quit from the nuclear one. How can I resolve this? If you propose a fix, please make it concise.
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!
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/main/lib/tray/index.ts:236-239
**`toolTip` not visible in macOS tray context menus**
The `toolTip` property on a `MenuItemConstructorOptions` object is not rendered by macOS when the menu is displayed as a tray context menu (or any `NSMenu`-backed context menu). macOS only shows item tooltips in the main menu bar, not in pop-up/context menus. The descriptive string `"Quit and stop all background services (host-service, daemon)"` will therefore never be visible to users, regardless of whether Electron silently accepts the property.
### Issue 2 of 2
apps/desktop/src/main/lib/tray/index.ts:231-239
**No visual separator before the destructive quit action**
"Quit Superset Completely" immediately follows "Quit Superset" with no separator, making it easy to accidentally click the destructive action (which kills all background services). A separator line gives the user a visual pause and distinguishes the soft quit from the nuclear one.
```suggestion
{
label: "Quit Superset",
click: () => quitApp(),
},
{ type: "separator" },
{
label: "Quit Superset Completely",
toolTip: "Quit and stop all background services (host-service, daemon)",
click: () => quitAppCompletely(),
},
```
Reviews (1): Last reviewed commit: "feat(desktop): add tray "Quit Superset C..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/index.ts">
<violation number="1" location="apps/desktop/src/main/index.ts:225">
P1: The new `forceFullCleanup` production path uses best-effort cleanup and still exits on cleanup failure, so "Quit Superset Completely" can leave host/terminal processes running.
(Based on your team's feedback about treating “Kill Sessions” as a hard outcome.) [FEEDBACK_USED]</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
| isQuitting = true; | ||
| try { | ||
| if (isDev) { | ||
| if (isDev || forceFullCleanup) { |
There was a problem hiding this comment.
P1: The new forceFullCleanup production path uses best-effort cleanup and still exits on cleanup failure, so "Quit Superset Completely" can leave host/terminal processes running.
(Based on your team's feedback about treating “Kill Sessions” as a hard outcome.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/index.ts, line 225:
<comment>The new `forceFullCleanup` production path uses best-effort cleanup and still exits on cleanup failure, so "Quit Superset Completely" can leave host/terminal processes running.
(Based on your team's feedback about treating “Kill Sessions” as a hard outcome.) </comment>
<file context>
@@ -214,7 +222,7 @@ app.on("before-quit", async (event) => {
isQuitting = true;
try {
- if (isDev) {
+ if (isDev || forceFullCleanup) {
await runDevQuitCleanup();
} else {
</file context>
Quit Superset Completely now shows a warning dialog noting that all terminal sessions will be killed; cancel by default. The soft path is renamed to "Close Superset" to make the distinction obvious — closing leaves background services running for re-adoption on next launch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/tray/index.ts`:
- Around line 87-101: confirmAndQuitCompletely currently awaits
dialog.showMessageBox and can throw, causing unhandled rejections when callers
intentionally discard its promise; wrap the await in a local try/catch inside
confirmAndQuitCompletely, catch any error, log it (using the same logger used
elsewhere in this module) and return void so errors are contained, and ensure
that when response === 0 you still call quitAppCompletely(); apply the same
pattern (local try/catch + logging) to the other menu-click handler(s) that call
dialog.showMessageBox (the similar call around the other Quit/Close flow).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2df9505-a252-4395-966d-51a62b825b3f
📒 Files selected for processing (1)
apps/desktop/src/main/lib/tray/index.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
runDevQuitCleanup()shutdown sequence (coordinator.stopAll()+terminalHostClient.shutdownIfRunning({ killSessions: true })) by gating it on a newforceFullCleanupflag inside thebefore-quithandler.Test plan
bun devinapps/desktop, sign in to an org, click tray → Quit Superset. App and services exit cleanly (no regression).pgrep -f host-serviceis empty and the pty-daemon socket is gone after quit.bun run lintandbun run typecheckexit 0.Summary by cubic
Add a macOS tray option “Quit Superset Completely” to fully shut down the app and all background services, with a confirmation prompt. The soft path is now “Close Superset” and keeps services running for re-adoption.
quitAppCompletely()+forceFullCleanup, reusingrunDevQuitCleanup()to stop host-service(s) and the pty-daemon/terminal-host.Written for commit 45f9d2b. Summary will update on new commits.
Summary by CodeRabbit