feat(desktop): add confirm-on-quit setting with behavior settings page#524
Conversation
- Add confirmOnQuit boolean column to settings table with migration - Add getConfirmOnQuit/setConfirmOnQuit tRPC procedures - Create new "Behavior" section in settings sidebar - Add BehaviorSettings component with toggle switch - Implement native quit confirmation dialog in main process - Default to enabled (confirm before quitting) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (6)apps/desktop/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/main/index.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/**/src/**/**/[A-Z]*.tsx📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-12-28T01:56:39.021ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.021ZApplied to files:
📚 Learning: 2025-12-28T01:56:39.021ZApplied to files:
🧬 Code graph analysis (2)apps/desktop/src/lib/trpc/routers/settings/index.ts (4)
apps/desktop/src/main/lib/auto-updater.ts (1)
🔇 Additional comments (16)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (1)
5-8: Consider adding error handling and user feedback.The component doesn't handle query errors or provide feedback on mutation success/failure. Users toggling the switch would have no indication if the update failed or succeeded.
Consider destructuring error states and providing user feedback:
🔎 Suggested enhancement
export function BehaviorSettings() { - const { data: confirmOnQuit, isLoading } = + const { data: confirmOnQuit, isLoading, error } = trpc.settings.getConfirmOnQuit.useQuery(); const setConfirmOnQuit = trpc.settings.setConfirmOnQuit.useMutation(); const handleToggle = (enabled: boolean) => { setConfirmOnQuit.mutate({ enabled }); }; + + // Show error state if query fails + if (error) { + return ( + <div className="p-6 max-w-4xl w-full"> + <p className="text-sm text-destructive"> + Failed to load behavior settings + </p> + </div> + ); + }Additionally, consider using
onSuccess/onErrorcallbacks on the mutation or a toast notification to inform users of the result.apps/desktop/src/main/index.ts (1)
99-111: Consider logging database errors for debugging.The function correctly defaults to
trueon any database error, providing a safe fallback. However, silently catching errors could make troubleshooting difficult.🔎 Optional enhancement
function getConfirmOnQuitSetting(): boolean { try { const row = localDb.select().from(settings).get(); // Default to true if not set return row?.confirmOnQuit ?? true; - } catch { + } catch (error) { + console.error("[main] Failed to read confirmOnQuit setting:", error); // If we can't read the setting, default to showing the dialog return true; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/stores/app-state.tspackages/local-db/drizzle/0003_add_confirm_on_quit_setting.sqlpackages/local-db/drizzle/meta/0003_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/main/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxpackages/local-db/src/schema/schema.tsapps/desktop/src/main/index.ts
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/stores/app-state.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/main/index.ts
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in
src/main/index.tswithoverride: truebefore any other imports in the desktop app
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (3)
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx
📚 Learning: 2025-12-28T01:56:39.021Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.021Z
Learning: Applies to packages/db/src/schema/**/*.ts : Create database migrations by changing Drizzle schema and running `drizzle-kit generate --name="<sample_name_snake_case>"` on a new Neon branch
Applied to files:
packages/local-db/drizzle/meta/0003_snapshot.json
📚 Learning: 2025-12-28T01:56:39.021Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.021Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/main/index.ts
🧬 Code graph analysis (4)
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (2)
packages/ui/src/components/ui/label.tsx (1)
Label(24-24)packages/ui/src/components/ui/switch.tsx (1)
Switch(31-31)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (1)
BehaviorSettings(5-43)
apps/desktop/src/lib/trpc/routers/settings/index.ts (3)
packages/trpc/src/index.ts (1)
publicProcedure(12-12)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (1)
settings(117-130)
apps/desktop/src/main/index.ts (4)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(82-82)packages/local-db/src/schema/schema.ts (1)
settings(117-130)apps/desktop/src/main/lib/terminal/manager.ts (1)
terminalManager(437-437)apps/desktop/src/main/lib/analytics/index.ts (1)
posthog(4-4)
🔇 Additional comments (14)
packages/local-db/drizzle/meta/0003_snapshot.json (1)
330-336: LGTM!The snapshot correctly captures the new
confirm_on_quitcolumn as an integer type with nullable constraint, consistent with the SQL migration and schema definition.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
4-4: LGTM!The import and conditional rendering follow the established pattern for settings sections. Integration with
BehaviorSettingsis clean and consistent.Also applies to: 25-25
packages/local-db/drizzle/0003_add_confirm_on_quit_setting.sql (1)
1-1: LGTM!The migration correctly adds the column. The lack of a database default is acceptable since the tRPC layer provides an application-level default of
truewhen the value is null.apps/desktop/src/renderer/stores/app-state.ts (1)
12-13: LGTM!The
SettingsSectiontype is cleanly extended to include"behavior". The zustand store structure remains consistent with existing patterns.packages/local-db/drizzle/meta/_journal.json (1)
26-31: LGTM!The journal entry correctly records the new migration with proper sequencing and metadata.
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (1)
3-3: LGTM!The new "Behavior" section entry follows the established pattern with appropriate icon choice and consistent styling.
Also applies to: 47-51
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
162-181: LGTM!The new
getConfirmOnQuitandsetConfirmOnQuitprocedures correctly follow the established patterns in this router:
- Query uses
getSettings()helper with nullish coalescing for the default value- Mutation uses upsert semantics consistent with other settings mutations
- Zod validation is properly applied
packages/local-db/src/schema/schema.ts (1)
129-129: LGTM!The schema correctly defines
confirmOnQuitas an integer with boolean mode, which is the standard Drizzle ORM pattern for SQLite boolean columns. The nullable design aligns with the application-level default handling in the tRPC router.apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (3)
1-3: LGTM: Imports follow project conventions.The imports correctly use UI components from the shared package and the TRPC client via the configured alias.
10-12: LGTM: Handler correctly invokes the mutation.The toggle handler properly passes the new state to the mutation.
35-35: Default value consistency verified.The UI defaults to
truewhenconfirmOnQuitis undefined. This aligns with the TRPCgetConfirmOnQuitquery which explicitly defaults totrue, and the main processgetConfirmOnQuitSetting()function which also defaults totrue. No issues found.apps/desktop/src/main/index.ts (3)
6-7: LGTM: Imports are appropriate for the feature.The
dialogmodule from Electron is needed for the confirmation prompt, andsettingsschema is required for reading the user preference from the database.
95-96: LGTM: Clear state machine design.The
QuitStatetype defines a clear lifecycle for the quit process. The state transitions will be validated in the handler review.
113-165: State machine logic is sound and handles quit flow correctly.Verification confirms the state machine properly coordinates confirmation and cleanup:
- idle → confirming → confirmed → cleaning → ready-to-quit transitions work as intended
- User confirmation dialog prevents concurrent quit attempts (lines 117-120)
- Cleanup runs before allowing app exit (lines 159-164)
- Recursive app.quit() calls handled correctly by state checks
- Dependencies (terminalManager, posthog, localDb) are properly imported and available
The empty before-quit handler in setup.ts (line 67) is overridden by the index.ts handler and has no impact.
No automated tests exist for this quit flow. Given the complexity and multiple state transitions, consider adding tests covering:
- Quit with confirmation enabled → confirm → app quits
- Quit with confirmation enabled → cancel → app stays open
- Quit with confirmation disabled → app quits immediately
- Rapid multiple quit attempts during confirmation dialog
- Cleanup failures and error recovery
|
Great idea! Honestly we should pretty much always ask to quit since it can be pretty destructive (lots of terminals to close), wanna switch this to always asking on close? Thanks for the PR btw! |
@saddlepaddle Enabled by default! https://github.com/superset-sh/superset/pull/524/changes#diff-d21adf32a04f683039509115823792173782ddfe72680f3fd546a2bf18af75dfR162 
|
- P0: Use app.exit(0) for !gotTheLock to avoid confirmation dialog on second instance (prevents headless zombie process) - P1: Wrap dialog.showMessageBox in try/catch to prevent stuck "confirming" state if dialog fails - P1: Add optimistic update with rollback for Switch toggle so UI updates immediately - Add setSkipQuitConfirmation() for auto-updater to bypass confirmation during quitAndInstall 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…tion - Add DEFAULT_CONFIRM_ON_QUIT to shared/constants.ts - Update main/index.ts to use shared constant instead of hardcoded true - Update trpc settings router to use shared constant - Prevents maintenance issues when default value needs to change - Addresses PR review comment about duplicate defaults
- Fix import order in apps/desktop/src/main/index.ts - Fix import order in apps/desktop/src/main/lib/auto-updater.ts - Run biome check --write to automatically organize imports
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
Works great, thanks @andreasasprou !
3 participants
Summary
Changes
confirmOnQuitboolean column to settings table with migrationgetConfirmOnQuit/setConfirmOnQuitproceduresTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.