Skip to content

refactor(desktop): quit-flag consolidation + desktop-action Exec composition (follow-up to #6894) - #6907

Open
Kitenite wants to merge 2 commits into
mainfrom
new-window-followup-cleanup
Open

refactor(desktop): quit-flag consolidation + desktop-action Exec composition (follow-up to #6894)#6907
Kitenite wants to merge 2 commits into
mainfrom
new-window-followup-cleanup

Conversation

@Kitenite

@Kitenite Kitenite commented Aug 27, 2026

Copy link
Copy Markdown
Member

What & why

Follow-up cleanups from the review of #6894 (now merged):

  • Consolidate the two app-quitting flags. main/index.ts kept isQuitting and windows/main.ts kept appQuitting, both set at the same moment in before-quit but read by different guards (crash-log suppression, the second-instance handler, per-window close). They now share one flag in main/lib/quit-state.ts (markAppQuitting() / isAppQuitting()), so the guards can't drift apart. No behavior change.
  • Compose the desktop action's Exec from linux.executableArgs. Declares executableArgs: ["--no-sandbox"] explicitly (identical to the AppImage default when unset, so the built artifact is unchanged) and builds the New Window action's Exec from it — replacing the keep-in-sync comment with actual sharing.

Test plan

  • tsc --noEmit passes in apps/desktop (re-run post-rebase onto main)
  • biome check clean on the four touched files
  • No tests reference the consolidated flags
  • CDP-verified on macOS with this branch running: org-dropdown New window, command-palette New window, native File → New Window, per-window close (×3), and a real Cmd+Q quit — all through the consolidated quit-state path; no console errors, clean exit

Summary by CodeRabbit

  • Bug Fixes
    • Improved application shutdown handling to prevent unnecessary window-state persistence and duplicate quit-related behavior, resulting in a more reliable exit experience.
    • Preserved Linux desktop “New Window” launch behavior while keeping its startup options consistent across application launches and desktop shortcuts.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 35e4b338-6fb4-43de-b7b5-2b3d6bb3f0dd

📥 Commits

Reviewing files that changed from the base of the PR and between ff3f3d1 and 03e0b3c.

📒 Files selected for processing (4)
  • apps/desktop/electron-builder.ts
  • apps/desktop/src/main/index.ts
  • apps/desktop/src/main/lib/quit-state.ts
  • apps/desktop/src/main/windows/main.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • apps/desktop/src/main/windows/main.ts
  • apps/desktop/src/main/lib/quit-state.ts
  • apps/desktop/electron-builder.ts
  • apps/desktop/src/main/index.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

The desktop build shares Linux executable arguments between AppImage configuration and the New Window desktop action. Application quit state now resides in a shared module used by main-process and window lifecycle handlers.

Changes

Linux launch arguments

Layer / File(s) Summary
Shared Linux executable arguments
apps/desktop/electron-builder.ts
The Linux target and New Window desktop action use the shared linuxExecutableArgs constant. The resulting launch arguments remain --no-sandbox and --new-window.

Shared application quit state

Layer / File(s) Summary
Shared quit-state contract and integration
apps/desktop/src/main/lib/quit-state.ts, apps/desktop/src/main/index.ts, apps/desktop/src/main/windows/main.ts
The shared isAppQuitting() and markAppQuitting() helpers replace local quit-state variables. Main-process event handlers and window persistence use the shared state.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 03e0b

This PR consolidates quit-state handling and derives the Linux desktop action command from the configured executable arguments without changing the intended artifact behavior; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title uses conventional commit format and clearly identifies both desktop refactors: quit-state consolidation and desktop-action Exec composition.
Description check ✅ Passed The description includes a clear What & why section and a detailed testing section. It omits the template's Checklist section, but it provides equivalent evidence for the relevant checks and is otherw…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes a clear What & why section and a detailed testing section. It omits the template's Checklist section, but it provides equivalent evidence for the relevant checks and is otherwise complete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch new-window-followup-cleanup

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.

…dule

index.ts kept isQuitting and windows/main.ts kept appQuitting, both set at
the same moment in before-quit and read by different guards (crash-log
suppression, second-instance, per-window close). One flag in
lib/quit-state.ts now serves every reader, so the guards can't drift.
…bleArgs

Declares executableArgs explicitly (same value as the AppImage default) and
builds the New Window action's Exec from it, replacing the keep-in-sync
comment with actual sharing.
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