Skip to content

fix(desktop): remove macOS background-to-tray quit interception#3205

Merged
Kitenite merged 5 commits into
mainfrom
review-v2-quit-tray-logic
Apr 6, 2026
Merged

fix(desktop): remove macOS background-to-tray quit interception#3205
Kitenite merged 5 commits into
mainfrom
review-v2-quit-tray-logic

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 6, 2026

Summary

  • Remove the macOS before-quit block that prevented quit and kept the tray alive when hasActiveInstances() was true. All quit paths now fully exit.

Why / Context

The macOS background-to-tray block caused two problems:

  1. Inconsistent quit behavior — Cmd+Q / Dock Quit only backgrounded to tray when host services were active. Without services, it fully quit. Users couldn't predict what would happen.
  2. Broken updaterinstallUpdate() called prepareQuit("release") then quitAndInstall(), but the background block intercepted the quit when services were active, leaving the app stuck in tray instead of installing the update.

Removing the block is the simplest fix. Background-to-tray is deferred until we can solve the dock icon problem (requires a separate tray-host process).

Testing

  • bunx tsc --noEmit — no new type errors
  • Manual: built packaged app, verified Cmd+Q / Dock Quit / tray quit all fully exit

Design Decisions

  • Why not background-to-tray: macOS shows the dock icon as long as the Electron process is alive. Backgrounding looks like the app is still running, which is confusing. Fixing this requires a separate lightweight tray-host process (documented in plans/20260405-quit-tray-lifecycle.md).
  • Why no lifecycle module: Premature abstraction. The existing QuitMode / requestQuit / prepareQuit API is sufficient. We can add intents later when background-to-tray is implemented.

Add architecture research, current-vs-target analysis, and short-term
migration plan for macOS tray-resident lifecycle behavior.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 6, 2026

📝 Walkthrough

Walkthrough

Simplified the app's quit lifecycle by removing a macOS-specific conditional that prevented shutdown and destroyed browser windows while keeping tray and services alive. Reordered the before-quit handler to acquire the host-service manager after confirmation checks. Added comprehensive documentation detailing the quit lifecycle, lifecycle intents, and future tray architecture considerations.

Changes

Cohort / File(s) Summary
Quit Handler Simplification
apps/desktop/src/main/index.ts
Removed macOS-specific early-return logic that blocked quitting when host-services were active. Reordered flow to call getHostServiceManager() after confirmation and development mode checks, ensuring service manager acquisition only when proceeding to stopAll() or releaseAll() operations.
Lifecycle Documentation
apps/desktop/plans/20260405-quit-tray-lifecycle.md
New documentation file codifying app exit behavior (no background-to-tray persistence), defining lifecycle intents (exit_release, exit_stop, restart), documenting updater integration changes, and outlining future dual-Electron or native Swift tray architectures.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 The quit path clears, no more delays,
No windows hiding in the tray's maze,
Services exit cleanly, swift and sure,
A simpler way—the lifecycle's cure!
Electrons freed, the tray lets go,
Clean exits now, a smoother flow. 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately and concisely summarizes the main change: removing the macOS background-to-tray quit interception block from the before-quit handler.
Description check ✅ Passed The PR description is well-structured and covers the essential sections (Summary, Why/Context, Testing, Design Decisions), but doesn't follow the provided template structure with its required headings.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch review-v2-quit-tray-logic

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 12 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/HOST_SERVICE_BOUNDARIES.md">

<violation number="1" location="apps/desktop/HOST_SERVICE_BOUNDARIES.md:235">
P2: Documentation references deprecated quit API names (`requestQuit("release"/"stop")`) instead of the current intent-based `requestExit` calls.</violation>

<violation number="2" location="apps/desktop/HOST_SERVICE_BOUNDARIES.md:242">
P2: The documented macOS quit semantics are outdated (`active services` + `"release"` mode) and conflict with the new intent-based lifecycle model.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/desktop/HOST_SERVICE_BOUNDARIES.md Outdated
Comment thread apps/desktop/HOST_SERVICE_BOUNDARIES.md Outdated
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 6, 2026

Greptile Summary

This PR introduces a clean lifecycle intent model (LifecycleIntent) to replace the overloaded QuitMode type, fixing two real bugs: (1) macOS implicit quit no longer incorrectly triggers the background-to-tray path for the updater, and (2) the hasActiveInstances() gate that made quit behavior unpredictable is removed. macOS now always backgrounds to tray on any implicit quit, while explicit requestExit(...) calls drive full termination. The notification HTTP server is promoted to a module-level singleton that survives window destruction, preventing port-conflict on window recreation.

Key changes:

  • New lifecycle.ts: LifecycleIntent type, requestExit / prepareIntent / consumeIntent / isFullExitIntent / markExiting / isExiting helpers — clean, well-documented module
  • before-quit handler: Simplified branching — macOS implicit quit always backgrounds; explicit intents flow to full exit; install_update returns early letting the updater own shutdown
  • auto-updater.ts: One-line fix — prepareIntent("install_update") instead of prepareQuit("release") — correctly bypasses the tray-background path
  • windows/main.ts: Window state saved on close, resources cleaned on closed (fires for both normal close and destroy()); notification server is a singleton
  • tray/index.ts: Labels updated for clarity; requestExit(...) used directly instead of indirect requestQuit
  • settings/index.ts: restartApp simplified to requestExit("restart")

Confidence Score: 4/5

Safe to merge — fixes two real bugs with no regressions; the only concerns are minor style issues and a well-documented edge case (tray init failure) that is mitigated by the dock

The intent model is correct, the updater fix is minimal and precise, and the destroy-instead-of-hide pattern is a genuine improvement. The three P2 comments are style/robustness suggestions that don't block merging. The PR is at most one targeted cleanup away from 5/5.

apps/desktop/src/main/index.ts (tray guard edge case), apps/desktop/src/main/lib/tray/index.ts (menu duplication), apps/desktop/src/main/windows/main.ts (redundant cleanup comment)

Important Files Changed

Filename Overview
apps/desktop/src/main/lib/lifecycle.ts New module cleanly encapsulates all lifecycle intent state; well-documented with clear separation between requestExit (triggers app.quit) and prepareIntent (for externally-triggered quits)
apps/desktop/src/main/index.ts before-quit handler correctly branches on macOS vs full-exit-intent; removes hasActiveInstances gate (intentional); install_update return path properly hands shutdown to updater
apps/desktop/src/main/windows/main.ts Destroy-on-close replaces hide-on-close; notification server promoted to singleton; close/closed split is correct but has minor redundant cleanup on reopen
apps/desktop/src/main/lib/auto-updater.ts One-line fix (prepareIntent('install_update') over prepareQuit('release')) correctly bypasses background-to-tray path during update install
apps/desktop/src/main/lib/tray/index.ts Labels updated for clarity; requestExit calls are correct; minor duplicate menu entry in both hasActive branches
apps/desktop/src/lib/trpc/routers/settings/index.ts restartApp simplified to single requestExit('restart') call; cleaner than manual app.relaunch() + exitImmediately()
apps/desktop/src/lib/electron-app/factories/app/setup.ts Comment-only update to reflect destroy-instead-of-hide semantics; logic unchanged

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User action] --> B{Intent set?}
    B -- No intent\n(Cmd+Q / red-X / Dock Quit) --> C{Platform?}
    C -- macOS --> D[event.preventDefault\ndestroy all windows\nkeep tray alive]
    C -- Win/Linux --> E{confirmOnQuit?}
    E -- Yes --> F[Show dialog]
    F -- Cancel --> G[Abort quit]
    F -- Confirm --> H[Full exit path]
    E -- No --> H
    B -- exit_release\nexit_stop\nrestart\ninstall_update --> H
    H --> I[markExiting]
    I --> J{intent?}
    J -- exit_stop --> K[manager.stopAll]
    J -- other --> L[manager.releaseAll]
    K --> M[disposeTray]
    L --> M
    M --> N{intent?}
    N -- install_update --> O[return\naupdater owns shutdown]
    N -- restart --> P[app.relaunch\napp.exit 0]
    N -- other --> Q[app.exit 0]
Loading

Reviews (1): Last reviewed commit: "docs(desktop): add tray lifecycle resear..." | Re-trigger Greptile

Comment on lines 240 to 256
...(hasActive
? [
{
label: "Quit (Keep Services Running)",
click: () => requestQuit("release"),
label: "Quit Superset",
click: () => requestExit("exit_release"),
},
{
label: "Quit & Stop Services",
click: () => requestQuit("stop"),
click: () => requestExit("exit_stop"),
},
]
: [
{
label: "Quit",
click: () => requestQuit("release"),
label: "Quit Superset",
click: () => requestExit("exit_release"),
},
]),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Duplicate tray menu entry

The "Quit Superset" item with identical label and handler appears in both branches of the hasActive conditional. This can be simplified by always including the item and conditionally appending "Quit & Stop Services":

Suggested change
...(hasActive
? [
{
label: "Quit (Keep Services Running)",
click: () => requestQuit("release"),
label: "Quit Superset",
click: () => requestExit("exit_release"),
},
{
label: "Quit & Stop Services",
click: () => requestQuit("stop"),
click: () => requestExit("exit_stop"),
},
]
: [
{
label: "Quit",
click: () => requestQuit("release"),
label: "Quit Superset",
click: () => requestExit("exit_release"),
},
]),
{ label: "Quit Superset", click: () => requestExit("exit_release") },
...(hasActive
? [{ label: "Quit & Stop Services", click: () => requestExit("exit_stop") }]
: []),

This reduces duplication and makes the intent clearer: "Quit Superset" is always available, and the stop-services variant is an optional extension when services are active.

Comment thread apps/desktop/src/main/index.ts Outdated
Comment on lines 176 to 182
if (PLATFORM.IS_MAC && !isFullExitIntent(intent)) {
event.preventDefault();
for (const win of BrowserWindow.getAllWindows()) {
win.destroy();
}
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 macOS always backgrounds — no tray availability guard

The new path removes the old hasActiveInstances() gate and now backgrounds to tray unconditionally on macOS implicit quit. This is correct by design per the PR description. However, initTray() can silently fail (e.g., icon file not found), leaving the app alive in the background with no tray icon and no visible affordance to reopen it.

The dock icon would still be present (since app.dock.hide() is never called), so the user can recover via the dock. But if this edge case is a concern, a guard like if (tray) could be added here to fall through to full exit when tray initialization failed — or at minimum an explicit log warning when backgrounding without a tray.

Comment thread apps/desktop/src/main/windows/main.ts Outdated
Comment on lines +168 to +175
// Dispose previous notification manager from a destroyed window
if (activeNotificationManager) {
activeNotificationManager.dispose();
activeNotificationManager = null;
}

// Clear stale event listeners from previous window lifecycle
notificationsEmitter.removeAllListeners();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Double removeAllListeners / dispose on reopen

notificationsEmitter.removeAllListeners() and the activeNotificationManager.dispose() block at the top of MainWindow() duplicate cleanup that the closed event handler already performed. In practice this is safe because destroy() fires closed synchronously before activate can deliver the reopen, so by the time MainWindow() runs again activeNotificationManager is already null and the emitter is already clear.

This is a minor defensive-coding redundancy rather than a bug, but a comment explaining the invariant ("closed event fires synchronously from destroy(), so these are guaranteed null/empty by here on macOS reopen") would help future readers understand why both sites exist.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/desktop/src/main/windows/main.ts (1)

321-337: ⚠️ Potential issue | 🟡 Minor

Flush the pending window state save before backgrounding to tray.

When the app destroys the window to background to tray (macOS), any resize, move, or zoom change still inside the 500ms debounce timer will be dropped. The pending saveTimeout will fire after the window is destroyed, triggering the early return at line 252. Call clearTimeout(saveTimeout); debouncedSave(); synchronously in the lifecycle code just before win.destroy() to ensure the last state is persisted.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/windows/main.ts` around lines 321 - 337, In the macOS
background-to-tray path, pending debounced window state saves can be dropped
after the window is destroyed; before calling win.destroy() ensure you
synchronously flush the pending save by calling clearTimeout(saveTimeout) and
then debouncedSave(); locate the lifecycle code that backgrounds to tray (the
code that calls win.destroy()) and insert these calls immediately before
win.destroy() so the final window bounds/zoom are persisted (references:
saveTimeout, debouncedSave, win.destroy()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/HOST_SERVICE_BOUNDARIES.md`:
- Around line 233-245: The document describes the retired quit model (mentions
requestQuit, "release"/"stop", and active-service gating) but the code now uses
requestExit with "exit_release"|"exit_stop" and null as the implicit
tray-background case; update the text to replace requestQuit("release" | "stop")
with requestExit("exit_release" | "exit_stop") and describe null as the implicit
background/tray case, remove/adjust references to the old `"release"` background
path and active-service gating, and ensure the final exit paths describe the
current behavior (calls to releaseAll(), stopAll(), disposeTray(), and app.exit)
and that the restart mention (coordinator.restart / restart) reflects the new
signature if it no longer accepts SpawnConfig.

In `@apps/desktop/HOST_SERVICE_LIFECYCLE.md`:
- Around line 14-28: The lifecycle doc still describes the old pre-intent quit
model and must be updated to reflect the new intent-based flow: remove
references to pendingQuitMode, requestQuit(), prepareQuit("release"),
exitImmediately(), hasActiveInstances() macOS branch and hidden-window reuse,
and instead document LifecycleIntent, the implicit macOS-to-tray behavior,
routing of updater installs via install_update, restart flow via
requestExit("restart"), and window recreation on reopen; update the high-level
component bullets that mention Electron main, tray and HostServiceManager to
reference the current functions/behaviors (e.g., LifecycleIntent handling in
lifecycle.ts, requestExit usage, install_update hook, and macOS
background-to-tray semantics) and update the other affected sections (lines
referenced) accordingly so the doc matches the actual lifecycle.ts and index.ts
logic.

In `@apps/desktop/src/main/index.ts`:
- Around line 174-181: The implicit-macOS-quit path uses
BrowserWindow.getAllWindows().destroy() to background to tray even if initTray()
hasn't run yet, which can leave the app without a tray to reopen—fix by gating
that behavior on the tray being initialized: add a check (e.g., a
global/app-scoped flag like trayInitialized or test for the actual Tray instance
set by initTray) alongside the existing PLATFORM.IS_MAC &&
!isFullExitIntent(intent) condition, and only perform event.preventDefault() +
destroy() + return when the tray is ready; otherwise call app.quit() (or allow
default quit) so the app exits cleanly until initTray() completes. Ensure
initTray() sets the flag/Tray instance used by this check.

In `@apps/desktop/src/main/windows/main.ts`:
- Around line 168-175: The current teardown unconditionally disposes the
process-wide NotificationManager and removes all listeners
(activeNotificationManager.dispose(), activeNotificationManager = null,
notificationsEmitter.removeAllListeners()) whenever a BrowserWindow is
destroyed; change this so the NotificationManager remains process-scoped and is
not disposed when the app is operating in tray-resident/background mode — only
dispose it on full app shutdown. Update the destruction logic to treat
currentWindow === null as an unfocused/no-window state (keep
activeNotificationManager and listeners active so notifications/sounds continue)
and only call dispose/removeAllListeners during explicit app quit; apply the
same change to the analogous branch around the other occurrence (lines
referenced in the review).

---

Outside diff comments:
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 321-337: In the macOS background-to-tray path, pending debounced
window state saves can be dropped after the window is destroyed; before calling
win.destroy() ensure you synchronously flush the pending save by calling
clearTimeout(saveTimeout) and then debouncedSave(); locate the lifecycle code
that backgrounds to tray (the code that calls win.destroy()) and insert these
calls immediately before win.destroy() so the final window bounds/zoom are
persisted (references: saveTimeout, debouncedSave, win.destroy()).
🪄 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: 3a3bbe2d-36c2-47d3-9258-a32215210964

📥 Commits

Reviewing files that changed from the base of the PR and between d670c4a and 432080e.

📒 Files selected for processing (12)
  • apps/desktop/HOST_SERVICE_BOUNDARIES.md
  • apps/desktop/HOST_SERVICE_LIFECYCLE.md
  • apps/desktop/plans/20260405-macos-tray-short-term-migration.md
  • apps/desktop/plans/20260405-quit-tray-current-vs-target.md
  • apps/desktop/plans/20260405-tray-architecture-research.md
  • apps/desktop/src/lib/electron-app/factories/app/setup.ts
  • apps/desktop/src/lib/trpc/routers/settings/index.ts
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/lib/auto-updater.ts
  • apps/desktop/src/main/lib/lifecycle.ts
  • apps/desktop/src/main/lib/tray/index.ts
  • apps/desktop/src/main/windows/main.ts

Comment thread apps/desktop/HOST_SERVICE_BOUNDARIES.md Outdated
Comment thread apps/desktop/HOST_SERVICE_LIFECYCLE.md Outdated
Comment thread apps/desktop/src/main/index.ts Outdated
Comment on lines 174 to 181
// macOS: implicit quit (no intent) backgrounds to tray.
// Destroy all windows but keep the tray and process alive.
if (PLATFORM.IS_MAC && !isFullExitIntent(intent)) {
event.preventDefault();
for (const win of BrowserWindow.getAllWindows()) {
win.destroy();
}
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '1,50p'

Repository: superset-sh/superset

Length of output: 2025


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '160,190p'

Repository: superset-sh/superset

Length of output: 1141


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '375,395p'

Repository: superset-sh/superset

Length of output: 751


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | wc -l

Repository: superset-sh/superset

Length of output: 67


🏁 Script executed:

cat -n apps/desktop/src/main/windows/main.ts | head -80

Repository: superset-sh/superset

Length of output: 3549


🏁 Script executed:

grep -n "on.*close\|\.close()" apps/desktop/src/main/windows/main.ts | head -20

Repository: superset-sh/superset

Length of output: 280


🏁 Script executed:

cat -n apps/desktop/src/main/windows/main.ts | sed -n '1,150p'

Repository: superset-sh/superset

Length of output: 6035


🏁 Script executed:

cat -n apps/desktop/src/main/windows/main.ts | sed -n '315,360p'

Repository: superset-sh/superset

Length of output: 1819


🏁 Script executed:

grep -A 10 'window.on("close"' apps/desktop/src/main/windows/main.ts

Repository: superset-sh/superset

Length of output: 400


🏁 Script executed:

grep -A 15 'window.on("closed"' apps/desktop/src/main/windows/main.ts

Repository: superset-sh/superset

Length of output: 447


🏁 Script executed:

cat -n apps/desktop/src/main/lib/tray.ts | head -100

Repository: superset-sh/superset

Length of output: 129


🏁 Script executed:

grep -n "initTray\|disposeTray" apps/desktop/src/main/lib/tray.ts | head -20

Repository: superset-sh/superset

Length of output: 130


🏁 Script executed:

grep -B 5 -A 10 "quit\|exit" apps/desktop/src/main/lib/tray.ts | head -40

Repository: superset-sh/superset

Length of output: 130


🏁 Script executed:

find apps/desktop -name "*tray*" -type f

Repository: superset-sh/superset

Length of output: 243


🏁 Script executed:

fd tray apps/desktop/src/main

Repository: superset-sh/superset

Length of output: 95


🏁 Script executed:

git ls-files apps/desktop | grep tray

Repository: superset-sh/superset

Length of output: 332


🏁 Script executed:

cat -n apps/desktop/src/main/lib/tray/index.ts

Repository: superset-sh/superset

Length of output: 9329


🏁 Script executed:

grep -n "initTray\|disposeTray\|app.quit\|markExiting" apps/desktop/src/main/lib/tray/index.ts

Repository: superset-sh/superset

Length of output: 172


🏁 Script executed:

cat -n apps/desktop/src/main/lib/lifecycle.ts

Repository: superset-sh/superset

Length of output: 2168


🏁 Script executed:

grep -n "requestExit\|markExiting\|isExiting" apps/desktop/src/main/lib/lifecycle.ts

Repository: superset-sh/superset

Length of output: 178


🏁 Script executed:

grep -rn "markExiting" apps/desktop/src/main

Repository: superset-sh/superset

Length of output: 242


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '200,250p'

Repository: superset-sh/superset

Length of output: 1524


🏁 Script executed:

grep -n "\.show()" apps/desktop/src/main/windows/main.ts

Repository: superset-sh/superset

Length of output: 129


🏁 Script executed:

grep -n "\.show()" apps/desktop/src/main/index.ts

Repository: superset-sh/superset

Length of output: 126


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '360,396p'

Repository: superset-sh/superset

Length of output: 1224


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '100,120p'

Repository: superset-sh/superset

Length of output: 756


🏁 Script executed:

grep -B 5 "makeAppSetup" apps/desktop/src/main/index.ts | head -20

Repository: superset-sh/superset

Length of output: 589


🏁 Script executed:

grep -n "saveWindowState\|move\|resize" apps/desktop/src/main/windows/main.ts | head -30

Repository: superset-sh/superset

Length of output: 542


🏁 Script executed:

cat -n apps/desktop/src/main/windows/main.ts | sed -n '240,320p'

Repository: superset-sh/superset

Length of output: 3108


🏁 Script executed:

find apps/desktop -path "*/lib/electron-app/factories/app/setup*" -type f

Repository: superset-sh/superset

Length of output: 120


🏁 Script executed:

git ls-files apps/desktop | grep -i "setup\|makeAppSetup"

Repository: superset-sh/superset

Length of output: 2042


🏁 Script executed:

cat -n apps/desktop/src/lib/electron-app/factories/app/setup.ts | head -150

Repository: superset-sh/superset

Length of output: 3238


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '350,365p'

Repository: superset-sh/superset

Length of output: 555


🏁 Script executed:

grep -n "app.whenReady\|app.on.*ready" apps/desktop/src/main/index.ts

Repository: superset-sh/superset

Length of output: 92


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '330,360p'

Repository: superset-sh/superset

Length of output: 1363


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '305,320p'

Repository: superset-sh/superset

Length of output: 632


🏁 Script executed:

cat -n apps/desktop/src/main/index.ts | sed -n '168,185p'

Repository: superset-sh/superset

Length of output: 745


Don't background to tray until the tray exists.

An early Cmd+Q during startup can trigger the implicit quit path at lines 174–181, destroying windows and backgrounding the app before initTray() completes at line 383. This leaves the user unable to quit or access the app—no tray menu exists yet. Either prevent backgrounding in this early window (e.g., by checking if tray is initialized) or call app.quit() unconditionally until the tray is ready. Note: state persistence via debounced saves (lines 244–273) means the destroy() bypass of the close handler is intentional and already covered.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/index.ts` around lines 174 - 181, The
implicit-macOS-quit path uses BrowserWindow.getAllWindows().destroy() to
background to tray even if initTray() hasn't run yet, which can leave the app
without a tray to reopen—fix by gating that behavior on the tray being
initialized: add a check (e.g., a global/app-scoped flag like trayInitialized or
test for the actual Tray instance set by initTray) alongside the existing
PLATFORM.IS_MAC && !isFullExitIntent(intent) condition, and only perform
event.preventDefault() + destroy() + return when the tray is ready; otherwise
call app.quit() (or allow default quit) so the app exits cleanly until
initTray() completes. Ensure initTray() sets the flag/Tray instance used by this
check.

Comment thread apps/desktop/src/main/windows/main.ts Outdated
Comment on lines +168 to +175
// Dispose previous notification manager from a destroyed window
if (activeNotificationManager) {
activeNotificationManager.dispose();
activeNotificationManager = null;
}

// Clear stale event listeners from previous window lifecycle
notificationsEmitter.removeAllListeners();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Tray-resident mode no longer has a notification consumer.

These branches tear down the only NotificationManager as soon as the last BrowserWindow is destroyed. The singleton HTTP server keeps notification ingress alive, but after an implicit macOS background there is nothing left to handle the emitted events, show the system notification, or play the sound until a window is recreated. Please keep notification handling process-scoped, with currentWindow === null treated as an unfocused/no-window state.

Also applies to: 343-348

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/windows/main.ts` around lines 168 - 175, The current
teardown unconditionally disposes the process-wide NotificationManager and
removes all listeners (activeNotificationManager.dispose(),
activeNotificationManager = null, notificationsEmitter.removeAllListeners())
whenever a BrowserWindow is destroyed; change this so the NotificationManager
remains process-scoped and is not disposed when the app is operating in
tray-resident/background mode — only dispose it on full app shutdown. Update the
destruction logic to treat currentWindow === null as an unfocused/no-window
state (keep activeNotificationManager and listeners active so
notifications/sounds continue) and only call dispose/removeAllListeners during
explicit app quit; apply the same change to the analogous branch around the
other occurrence (lines referenced in the review).

Cmd+Q / Dock Quit now fully quit like any other app. The lifecycle
intents remain for the updater fix (install_update) and restart, but
implicit quit no longer backgrounds to tray or leaves the dock icon.
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 3 files (changes from recent commits).

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:174">
P1: This change removes the macOS implicit-quit guard, so Cmd+Q/Dock Quit with no intent now performs a full exit instead of backgrounding to tray.

(Based on your team's feedback about gating platform-specific lifecycle behavior with explicit runtime checks.) [FEEDBACK_USED]</violation>
</file>

<file name="apps/desktop/src/main/windows/main.ts">

<violation number="1">
P1: macOS close now hides the window instead of destroying it, reintroducing stale renderer retention and breaking the intended destroy/recreate lifecycle.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/desktop/src/main/index.ts Outdated
Comment thread apps/desktop/src/main/windows/main.ts
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/desktop/src/main/windows/main.ts (1)

321-352: ⚠️ Potential issue | 🟠 Major

Move cleanup code before the macOS hide check, or handle it on app quit.

The cleanup operations (browserManager.unregisterAll, notificationServer?.close, activeNotificationManager.dispose, notificationsEmitter.removeAllListeners, terminal.detachAllListeners, ipcHandler?.detachWindow) skip entirely on macOS because of the early return at line 342. The same window instance reuses on reopen, and since app.exit(0) bypasses the close handler during quit, these teardown operations never run on macOS at all. Either move the cleanup before the IS_MAC check so it always executes, or add equivalent cleanup to the before-quit handler.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/windows/main.ts` around lines 321 - 352, The cleanup
code inside the window.on("close", ...) handler is skipped on macOS due to the
early return after PLATFORM.IS_MAC, so ensure teardown always runs by moving the
cleanup calls (browserManager.unregisterAll, notificationServer?.close,
activeNotificationManager.dispose, notificationsEmitter.removeAllListeners,
terminal.detachAllListeners, ipcHandler?.detachWindow) to execute before the
macOS hide branch; alternatively, if you want to keep hide-on-close behavior,
add an equivalent cleanup block to the app.on("before-quit", ...) handler so
those same methods run during quit—update the window.on("close") or
app.on("before-quit") handlers accordingly to guarantee cleanup on macOS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/src/main/index.ts`:
- Around line 174-221: The non-full-exit branch currently falls through into the
full-exit sequence; update the handler so when intent is not a full exit (use
isFullExitIntent(intent) check) you call event.preventDefault(), destroy all
renderer windows via BrowserWindow.getAllWindows().forEach(win =>
win.destroy()), and return early to avoid running markExiting(),
getHostServiceManager().releaseAll()/stopAll(), disposeTray(), or app.exit(0);
also guard the full-exit sequence so those calls only run when intent !==
null/when a full intent is present (i.e., ensure markExiting,
manager.releaseAll()/stopAll, disposeTray(), app.relaunch/app.exit are executed
only for explicit intents like "exit_stop", "install_update", "restart").

In `@apps/desktop/src/main/windows/main.ts`:
- Around line 174-175: The shared notificationsEmitter is being fully cleared
with removeAllListeners(), which removes unrelated tRPC subscription handlers;
instead capture the specific AGENT_LIFECYCLE handler you add in the window setup
(the function currently passed to notificationsEmitter.on for AGENT_LIFECYCLE)
into a variable (e.g., agentLifecycleHandler) and replace the
removeAllListeners() calls with
notificationsEmitter.removeListener('AGENT_LIFECYCLE', agentLifecycleHandler)
(or notificationsEmitter.off('AGENT_LIFECYCLE', agentLifecycleHandler)); do this
in both cleanup locations so only the window-specific listener is removed while
leaving the tRPC subscriptions intact (they already call .off() themselves).
- Around line 154-166: The notification HTTP server (notificationServer created
via notificationsApp.listen using env.DESKTOP_NOTIFICATIONS_PORT) is intended to
be a process-scoped singleton but is currently being closed and nulled during
per-window teardown; remove that shutdown/nulling from the per-window teardown
code (do not call notificationServer.close() or set notificationServer = null
there) and instead ensure the only place that stops the server is the explicit
full-exit shutdown path in index.ts—add a guarded shutdown there that checks if
notificationServer exists, calls notificationServer.close(), and then nulls it
so the server survives window destruction as intended.

---

Outside diff comments:
In `@apps/desktop/src/main/windows/main.ts`:
- Around line 321-352: The cleanup code inside the window.on("close", ...)
handler is skipped on macOS due to the early return after PLATFORM.IS_MAC, so
ensure teardown always runs by moving the cleanup calls
(browserManager.unregisterAll, notificationServer?.close,
activeNotificationManager.dispose, notificationsEmitter.removeAllListeners,
terminal.detachAllListeners, ipcHandler?.detachWindow) to execute before the
macOS hide branch; alternatively, if you want to keep hide-on-close behavior,
add an equivalent cleanup block to the app.on("before-quit", ...) handler so
those same methods run during quit—update the window.on("close") or
app.on("before-quit") handlers accordingly to guarantee cleanup on macOS.
🪄 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: 5cf4dc3b-71c2-4baa-96a1-f81fa30da49c

📥 Commits

Reviewing files that changed from the base of the PR and between 432080e and 8144e5b.

📒 Files selected for processing (2)
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/windows/main.ts

Comment thread apps/desktop/src/main/index.ts Outdated
Comment on lines 174 to 221
// Implicit quit (no intent): optionally confirm before exiting
if (!isFullExitIntent(intent)) {
const isDev = process.env.NODE_ENV === "development";
if (!isDev && getConfirmOnQuitSetting()) {
event.preventDefault();

try {
const { response } = await dialog.showMessageBox({
type: "question",
buttons: ["Quit", "Cancel"],
defaultId: 0,
cancelId: 1,
title: "Quit Superset",
message: "Are you sure you want to quit?",
});

if (response === 1) {
return;
}
} catch (error) {
console.error("[main] Quit confirmation dialog failed:", error);
}
} catch (error) {
console.error("[main] Quit confirmation dialog failed:", error);
}
}

isQuitting = true;
if (quitMode === "stop") {
// Full exit path
markExiting();
const manager = getHostServiceManager();

if (intent === "exit_stop") {
manager.stopAll();
} else {
manager.releaseAll();
}

disposeTray();

if (intent === "install_update") {
// Let the updater own the final quit/install sequence.
// Don't call app.exit(0) — autoUpdater.quitAndInstall() handles it.
return;
}

if (intent === "restart") {
app.relaunch();
}

app.exit(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Read the full quit handler to understand control flow
sed -n '150,225p' apps/desktop/src/main/index.ts | cat -n

Repository: superset-sh/superset

Length of output: 2262


🏁 Script executed:

# Find the isFullExitIntent function to understand what it checks
rg -n "isFullExitIntent" apps/desktop/src/main/index.ts -A 5 -B 2

Repository: superset-sh/superset

Length of output: 613


🏁 Script executed:

# Check what intent parameter is and where it comes from in the handler signature
sed -n '145,180p' apps/desktop/src/main/index.ts | cat -n

Repository: superset-sh/superset

Length of output: 1301


🏁 Script executed:

# Find the isFullExitIntent implementation in lifecycle lib
find apps/desktop/src -name "*lifecycle*" -type f

Repository: superset-sh/superset

Length of output: 102


🏁 Script executed:

# Read the lifecycle lib to understand isFullExitIntent
cat -n apps/desktop/src/lib/lifecycle.ts | head -100

Repository: superset-sh/superset

Length of output: 129


🏁 Script executed:

# Read the lifecycle lib with correct path
cat -n apps/desktop/src/main/lib/lifecycle.ts

Repository: superset-sh/superset

Length of output: 2168


Null intent should background to tray, not terminate the app.

The !isFullExitIntent(intent) branch (line 31) currently falls through to markExiting(), releaseAll()/stopAll(), disposeTray(), and app.exit(0) (lines 51-72). On macOS, Cmd+Q and Dock Quit will therefore terminate the process instead of destroying windows and leaving the tray alive, contradicting the lifecycle design documented in lib/lifecycle.ts.

Add event.preventDefault(); BrowserWindow.getAllWindows().forEach((win) => win.destroy()); return; before line 50, and ensure lines 51-72 execute only for explicit intents (when intent !== null).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/index.ts` around lines 174 - 221, The non-full-exit
branch currently falls through into the full-exit sequence; update the handler
so when intent is not a full exit (use isFullExitIntent(intent) check) you call
event.preventDefault(), destroy all renderer windows via
BrowserWindow.getAllWindows().forEach(win => win.destroy()), and return early to
avoid running markExiting(), getHostServiceManager().releaseAll()/stopAll(),
disposeTray(), or app.exit(0); also guard the full-exit sequence so those calls
only run when intent !== null/when a full intent is present (i.e., ensure
markExiting, manager.releaseAll()/stopAll, disposeTray(), app.relaunch/app.exit
are executed only for explicit intents like "exit_stop", "install_update",
"restart").

Comment thread apps/desktop/src/main/windows/main.ts Outdated
Comment on lines +154 to +166
// Notification server is a long-lived singleton that survives window destruction.
// On macOS, windows are destroyed on background-to-tray and recreated on reopen.
if (!notificationServer) {
notificationServer = notificationsApp.listen(
env.DESKTOP_NOTIFICATIONS_PORT,
"127.0.0.1",
() => {
console.log(
`[notifications] Listening on http://127.0.0.1:${env.DESKTOP_NOTIFICATIONS_PORT}`,
);
},
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd apps/desktop/src/main/windows && head -370 main.ts | tail -220 | cat -n

Repository: superset-sh/superset

Length of output: 8145


🏁 Script executed:

cd apps/desktop/src/main/windows && sed -n '1,30p' main.ts && echo "---" && sed -n '330,365p' main.ts

Repository: superset-sh/superset

Length of output: 2111


🏁 Script executed:

cd apps/desktop/src/main/windows && wc -l main.ts

Repository: superset-sh/superset

Length of output: 75


🏁 Script executed:

cd apps/desktop/src/main && grep -n "app.quit\|app.exit\|notificationServer" *.ts

Repository: superset-sh/superset

Length of output: 258


🏁 Script executed:

cd apps/desktop/src/main && rg -n "notificationServer" -A 2 -B 2

Repository: superset-sh/superset

Length of output: 1037


Keep the notification HTTP server process-scoped.

notificationServer is initialized as a module singleton here, but lines 346-347 still close and null it during per-window teardown. That defeats the "survive window destruction" lifecycle on non-macOS platforms (where the close handler runs fully). The macOS code path avoids this by returning early, but the documented design intent is inconsistent with the actual implementation. Shut this server down only from the explicit full-exit path in index.ts, not from per-window teardown.

Also applies to: 346-347

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/windows/main.ts` around lines 154 - 166, The
notification HTTP server (notificationServer created via notificationsApp.listen
using env.DESKTOP_NOTIFICATIONS_PORT) is intended to be a process-scoped
singleton but is currently being closed and nulled during per-window teardown;
remove that shutdown/nulling from the per-window teardown code (do not call
notificationServer.close() or set notificationServer = null there) and instead
ensure the only place that stops the server is the explicit full-exit shutdown
path in index.ts—add a guarded shutdown there that checks if notificationServer
exists, calls notificationServer.close(), and then nulls it so the server
survives window destruction as intended.

Comment thread apps/desktop/src/main/windows/main.ts Outdated
Comment on lines +174 to +175
// Clear stale event listeners from previous window lifecycle
notificationsEmitter.removeAllListeners();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the main.ts file and see the context around lines 174-175 and 352-352
head -n 360 apps/desktop/src/main/windows/main.ts | tail -n +160

Repository: superset-sh/superset

Length of output: 5985


🏁 Script executed:

# Check where notificationsEmitter is defined/imported in main.ts
head -n 50 apps/desktop/src/main/windows/main.ts

Repository: superset-sh/superset

Length of output: 2050


🏁 Script executed:

# Look at the notifications router file mentioned in the comment
cat -n apps/desktop/src/lib/trpc/routers/notifications.ts

Repository: superset-sh/superset

Length of output: 2392


🏁 Script executed:

# Search for other usages of notificationsEmitter in the codebase
rg "notificationsEmitter" apps/desktop/src

Repository: superset-sh/superset

Length of output: 1700


Remove only the window-specific listeners instead of calling removeAllListeners() on the shared notifications bus.

The notificationsEmitter is used by active tRPC subscriptions in the renderer (apps/desktop/src/lib/trpc/routers/notifications.ts:43-62) to deliver AGENT_LIFECYCLE, TERMINAL_EXIT, and FOCUS_TAB events. Calling removeAllListeners() at lines 174 and 352 wipes out those subscriptions alongside the window's own AGENT_LIFECYCLE listener (line 214), breaking delivery to clients.

Store a reference to the handler added at line 214 and remove only that listener during cleanup. The tRPC subscription already properly cleans up its handlers via .off() in the observable cleanup function.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/windows/main.ts` around lines 174 - 175, The shared
notificationsEmitter is being fully cleared with removeAllListeners(), which
removes unrelated tRPC subscription handlers; instead capture the specific
AGENT_LIFECYCLE handler you add in the window setup (the function currently
passed to notificationsEmitter.on for AGENT_LIFECYCLE) into a variable (e.g.,
agentLifecycleHandler) and replace the removeAllListeners() calls with
notificationsEmitter.removeListener('AGENT_LIFECYCLE', agentLifecycleHandler)
(or notificationsEmitter.off('AGENT_LIFECYCLE', agentLifecycleHandler)); do this
in both cleanup locations so only the window-specific listener is removed while
leaving the tRPC subscriptions intact (they already call .off() themselves).

Kitenite added 2 commits April 5, 2026 22:23
Without background-to-tray, the updater doesn't need a special intent —
exit_release works fine since the updater does its install before
triggering quit. Revert window notification server to simple local
variable since windows are hidden (not destroyed) on macOS close.
Replace three separate research/analysis/migration docs with one
consolidated doc reflecting the current decision: all quit paths
fully exit, background-to-tray deferred pending process split.
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 8 files (changes from recent commits).

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/windows/main.ts">

<violation number="1">
P1: The notifications hook server is now tied to window lifetime; closing the main window shuts down notifications infrastructure. Keep the server process-wide (singleton) so tray-resident periods and window recreation don’t drop notification intake.</violation>
</file>

<file name="apps/desktop/plans/20260405-quit-tray-lifecycle.md">

<violation number="1" location="apps/desktop/plans/20260405-quit-tray-lifecycle.md:5">
P2: This decision line documents the opposite of the behavior introduced in this PR (implicit macOS quit/close backgrounding to tray), so the plan becomes immediately stale and misleading.</violation>

<violation number="2" location="apps/desktop/plans/20260405-quit-tray-lifecycle.md:33">
P2: The window-close row describes old macOS hide behavior, not the new destroy-and-recreate flow, so the lifecycle documentation is inaccurate.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

| Cmd+Q | Full exit (release services, dispose tray, exit) |
| Dock right-click Quit | Same |
| App menu Quit | Same |
| Window close (red-X / Cmd+W) | macOS: hide window (standard behavior). Non-macOS: close window, then app quits. |
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The window-close row describes old macOS hide behavior, not the new destroy-and-recreate flow, so the lifecycle documentation is inaccurate.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260405-quit-tray-lifecycle.md, line 33:

<comment>The window-close row describes old macOS hide behavior, not the new destroy-and-recreate flow, so the lifecycle documentation is inaccurate.</comment>

<file context>
@@ -0,0 +1,78 @@
+| Cmd+Q | Full exit (release services, dispose tray, exit) |
+| Dock right-click Quit | Same |
+| App menu Quit | Same |
+| Window close (red-X / Cmd+W) | macOS: hide window (standard behavior). Non-macOS: close window, then app quits. |
+| Tray "Quit Superset" | `requestExit("exit_release")` — release services, full exit |
+| Tray "Quit & Stop Services" | `requestExit("exit_stop")` — stop services, full exit |
</file context>
Fix with Cubic


## Decision (2025-04-05)

All quit paths fully exit the app. No background-to-tray behavior for now.
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This decision line documents the opposite of the behavior introduced in this PR (implicit macOS quit/close backgrounding to tray), so the plan becomes immediately stale and misleading.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260405-quit-tray-lifecycle.md, line 5:

<comment>This decision line documents the opposite of the behavior introduced in this PR (implicit macOS quit/close backgrounding to tray), so the plan becomes immediately stale and misleading.</comment>

<file context>
@@ -0,0 +1,78 @@
+
+## Decision (2025-04-05)
+
+All quit paths fully exit the app. No background-to-tray behavior for now.
+
+The tray exists while the app is running and provides host-service management and explicit quit actions. When the app quits, the tray goes away.
</file context>
Suggested change
All quit paths fully exit the app. No background-to-tray behavior for now.
All implicit macOS quit paths background to tray; explicit lifecycle intents (`exit_release`, `exit_stop`, `install_update`, `restart`) perform full exit behavior.
Fix with Cubic

…to-tray block

Revert lifecycle module, notification server singleton, and all
renaming. The only code change from main is removing the macOS
before-quit block that intercepted quit when services were active.
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 7 files (changes from recent commits).

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/lib/auto-updater.ts">

<violation number="1">
P2: This static import introduces a circular dependency with `main/index` in the startup path. Move quit-state setters to a side-effect-free module (or lazy-load) to avoid brittle initialization order.</violation>
</file>

<file name="apps/desktop/HOST_SERVICE_LIFECYCLE.md">

<violation number="1">
P3: The implicit-quit docs still mention an `active services` condition, but this PR’s target behavior is unconditional macOS background-to-tray for implicit quit.</violation>

<violation number="2">
P2: The updated lifecycle doc still describes the legacy `QuitMode`/`requestQuit` API instead of the new lifecycle-intent model, which will mislead future maintenance and QA.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/desktop/plans/20260405-quit-tray-lifecycle.md`:
- Line 3: Update the decision date string in the markdown header: change the
text "## Decision (2025-04-05)" to "## Decision (2026-04-05)" in the file named
20260405-quit-tray-lifecycle.md so the logged decision date matches the filename
and chronological ordering.

In `@apps/desktop/src/main/index.ts`:
- Around line 194-209: The cleanup sequence (markExiting,
getHostServiceManager/use of manager.stopAll or manager.releaseAll, disposeTray,
app.relaunch) should be wrapped in a try/finally so that app.exit(0) always
runs; update the shutdown flow around the code that calls markExiting(),
getHostServiceManager(), manager.stopAll()/manager.releaseAll(), disposeTray(),
and app.relaunch() to execute those teardown calls inside a try (optionally
catching/logging errors) and put app.exit(0) in the finally block to guarantee
the process exits even if teardown throws.
🪄 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: f1a41ef9-e7ff-4c9b-89e9-87f64831bb61

📥 Commits

Reviewing files that changed from the base of the PR and between 8144e5b and d0eec1f.

📒 Files selected for processing (4)
  • apps/desktop/plans/20260405-quit-tray-lifecycle.md
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/lib/auto-updater.ts
  • apps/desktop/src/main/lib/lifecycle.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/desktop/src/main/lib/auto-updater.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/desktop/src/main/lib/lifecycle.ts

@@ -0,0 +1,78 @@
# macOS Quit & Tray Lifecycle

## Decision (2025-04-05)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix the decision date typo.

Line 3 says 2025-04-05, but the filename is 20260405-quit-tray-lifecycle.md. This should be 2026-04-05 to keep the decision log chronologically correct.

Suggested edit
-## Decision (2025-04-05)
+## Decision (2026-04-05)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## Decision (2025-04-05)
## Decision (2026-04-05)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/plans/20260405-quit-tray-lifecycle.md` at line 3, Update the
decision date string in the markdown header: change the text "## Decision
(2025-04-05)" to "## Decision (2026-04-05)" in the file named
20260405-quit-tray-lifecycle.md so the logged decision date matches the filename
and chronological ordering.

Comment thread apps/desktop/src/main/index.ts Outdated
Comment on lines 194 to 209
markExiting();
const manager = getHostServiceManager();

if (intent === "exit_stop") {
manager.stopAll();
} else {
manager.releaseAll();
}

disposeTray();

if (intent === "restart") {
app.relaunch();
}

app.exit(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep the final exit in a finally block.

After Line 194, Lines 213-220 stop reporting exit-time failures. If any step in this cleanup sequence throws, Line 209 is skipped and the app can get stranded mid-shutdown with no diagnostic log. Wrap the teardown locally and force the final app.exit(0) from finally.

Suggested hardening
  markExiting();
  const manager = getHostServiceManager();
 
- if (intent === "exit_stop") {
- 	manager.stopAll();
- } else {
- 	manager.releaseAll();
- }
-
- disposeTray();
-
- if (intent === "restart") {
- 	app.relaunch();
- }
-
- app.exit(0);
+ try {
+ 	if (intent === "exit_stop") {
+ 		await manager.stopAll();
+ 	} else {
+ 		await manager.releaseAll();
+ 	}
+
+ 	disposeTray();
+
+ 	if (intent === "restart") {
+ 		app.relaunch();
+ 	}
+ } catch (error) {
+ 	console.error("[main] Quit cleanup failed:", error);
+ } finally {
+ 	app.exit(0);
+ }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/src/main/index.ts` around lines 194 - 209, The cleanup sequence
(markExiting, getHostServiceManager/use of manager.stopAll or
manager.releaseAll, disposeTray, app.relaunch) should be wrapped in a
try/finally so that app.exit(0) always runs; update the shutdown flow around the
code that calls markExiting(), getHostServiceManager(),
manager.stopAll()/manager.releaseAll(), disposeTray(), and app.relaunch() to
execute those teardown calls inside a try (optionally catching/logging errors)
and put app.exit(0) in the finally block to guarantee the process exits even if
teardown throws.

@Kitenite Kitenite changed the title refactor(desktop): macOS tray-resident lifecycle with explicit quit intents fix(desktop): remove macOS background-to-tray quit interception Apr 6, 2026
@Kitenite Kitenite merged commit 2573fa2 into main Apr 6, 2026
7 checks passed
@Kitenite Kitenite deleted the review-v2-quit-tray-logic branch April 6, 2026 06:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 6, 2026

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch
  • ⚠️ Electric Fly.io app

Thank you for your contribution! 🎉

MocA-Love pushed a commit to MocA-Love/superset that referenced this pull request Apr 7, 2026
…rset-sh#3205)

* docs(desktop): add tray lifecycle research and migration plan

Add architecture research, current-vs-target analysis, and short-term
migration plan for macOS tray-resident lifecycle behavior.

* simplify: remove macOS background-to-tray, all quit paths fully exit

Cmd+Q / Dock Quit now fully quit like any other app. The lifecycle
intents remain for the updater fix (install_update) and restart, but
implicit quit no longer backgrounds to tray or leaves the dock icon.

* simplify: remove install_update intent and notification server singleton

Without background-to-tray, the updater doesn't need a special intent —
exit_release works fine since the updater does its install before
triggering quit. Revert window notification server to simple local
variable since windows are hidden (not destroyed) on macOS close.

* docs(desktop): consolidate tray lifecycle docs into single decision doc

Replace three separate research/analysis/migration docs with one
consolidated doc reflecting the current decision: all quit paths
fully exit, background-to-tray deferred pending process split.

* refactor(desktop): simplify to minimal fix — remove macOS background-to-tray block

Revert lifecycle module, notification server singleton, and all
renaming. The only code change from main is removing the macOS
before-quit block that intercepted quit when services were active.
MocA-Love added a commit to MocA-Love/superset that referenced this pull request Apr 7, 2026
cherry-pick済み:
- e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190)
- 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177)
- 11ed4f8 V2 terminal env (superset-sh#3184)
- 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196)
- 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199)
- 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198)
- d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197)
- 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205)
- 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194)
- 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218)

フォーク独自対応:
- cleanupMainWindowResources()をexit pathに移動維持 (#3205対応)
- BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線
- BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正(リマップ対応)
- ansi_up依存維持
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.

1 participant