From 6312f067cd8a968f0aa862f828df5052b72f9b13 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sat, 13 Jun 2026 18:07:09 -0700 Subject: [PATCH] fix: prompt to discard unsaved Settings AI prompts on quit instead of silently blocking (#5335) --- .../src/components/settings/Settings.tsx | 49 ++++++++---- .../window-close-request-coordinator.test.ts | 78 ++++++++++++++++++- .../window-close-request-coordinator.ts | 51 ++++++++++-- 3 files changed, 154 insertions(+), 24 deletions(-) diff --git a/src/renderer/src/components/settings/Settings.tsx b/src/renderer/src/components/settings/Settings.tsx index afdcc9a1435..f42c502df56 100644 --- a/src/renderer/src/components/settings/Settings.tsx +++ b/src/renderer/src/components/settings/Settings.tsx @@ -53,6 +53,7 @@ import { ActiveSettingsSectionProvider, SettingsSection } from './SettingsSectio import { matchesSettingsSearch } from './settings-search' import { cn } from '@/lib/utils' import { isIntentionalAppRestartInProgress } from '@/lib/updater-beforeunload' +import { registerWindowCloseGuard } from '../window-close-request-coordinator' import { checkRuntimeHooks } from '@/runtime/runtime-hooks-client' import { getWindowsTerminalCapabilityOwnerKey, @@ -334,6 +335,11 @@ function Settings(): React.JSX.Element { const hasUnsavedSourceControlAiPromptChanges = hasUnsavedCommitPromptChanges || hasUnsavedBranchPromptChanges + // Why: the window-close guard registers once for Settings' lifetime, so it + // reads the latest dirty state from a ref instead of a closure that would lag + // behind the draft state until the next effect commit. + const hasUnsavedSourceControlAiPromptChangesRef = useRef(hasUnsavedSourceControlAiPromptChanges) + hasUnsavedSourceControlAiPromptChangesRef.current = hasUnsavedSourceControlAiPromptChanges const writeSourceControlAiSettings = useCallback( (patch: SourceControlAiSettingsPatch): Promise => { @@ -376,11 +382,11 @@ function Settings(): React.JSX.Element { cancelPendingSettingsSubsectionScrollFrame(pendingSubsectionScrollFrameRef) }, []) - const confirmDiscardSourceControlAiPromptChanges = useCallback(async (): Promise => { - if (!hasUnsavedSourceControlAiPromptChanges) { - return true - } - const shouldDiscard = await confirm({ + // Pure "discard and leave?" prompt — no side effects. Why separate from the + // discard helper below: the window-close guard must ask without clearing the + // drafts, since a later guard/handler can still cancel the close. + const promptDiscardSourceControlAiPromptChanges = useCallback((): Promise => { + return confirm({ title: translate( 'auto.components.settings.Settings.17bdee4ff1', 'Discard unsaved Git AI Author changes?' @@ -392,13 +398,20 @@ function Settings(): React.JSX.Element { confirmLabel: translate('auto.components.settings.Settings.65358016ea', 'Discard'), confirmVariant: 'destructive' }) + }, [confirm]) + + const confirmDiscardSourceControlAiPromptChanges = useCallback(async (): Promise => { + if (!hasUnsavedSourceControlAiPromptChanges) { + return true + } + const shouldDiscard = await promptDiscardSourceControlAiPromptChanges() if (shouldDiscard) { setSourceControlAiPromptDiscardSignal((signal) => signal + 1) setHasUnsavedCommitPromptChanges(false) setHasUnsavedBranchPromptChanges(false) } return shouldDiscard - }, [confirm, hasUnsavedSourceControlAiPromptChanges]) + }, [promptDiscardSourceControlAiPromptChanges, hasUnsavedSourceControlAiPromptChanges]) const closeSettingsPageWithPromptGuard = useCallback(async (): Promise => { if (!(await confirmDiscardSourceControlAiPromptChanges())) { @@ -499,19 +512,25 @@ function Settings(): React.JSX.Element { return () => document.removeEventListener('keydown', handleKeyDown) }, [activeSectionId, closeSettingsPageWithPromptGuard]) + // Why: route window close / quit through the same discard dialog as in-app + // navigation. A raw beforeunload preventDefault only silently vetoes the close + // (no UI), which on the no-workspace Settings page reads as an unquittable + // window. Register one stable guard for Settings' lifetime, reading the latest + // dirty state from a ref. Why the pure prompt (no discard side effect): a + // downstream guard/handler can still cancel the close (e.g. a dirty-editor save + // dialog), and clearing the drafts up front would lose them while the window + // stays open; on an actual close they fall away with the renderer anyway. useEffect(() => { - const handleBeforeUnload = (event: BeforeUnloadEvent): void => { + return registerWindowCloseGuard(() => { if (isIntentionalAppRestartInProgress()) { - return + return true } - if (!hasUnsavedSourceControlAiPromptChanges) { - return + if (!hasUnsavedSourceControlAiPromptChangesRef.current) { + return true } - event.preventDefault() - } - window.addEventListener('beforeunload', handleBeforeUnload) - return () => window.removeEventListener('beforeunload', handleBeforeUnload) - }, [hasUnsavedSourceControlAiPromptChanges]) + return promptDiscardSourceControlAiPromptChanges() + }) + }, [promptDiscardSourceControlAiPromptChanges]) useEffect(() => { const handleFindShortcut = (event: KeyboardEvent): void => { diff --git a/src/renderer/src/components/window-close-request-coordinator.test.ts b/src/renderer/src/components/window-close-request-coordinator.test.ts index 397f8f1a6cd..ee197842707 100644 --- a/src/renderer/src/components/window-close-request-coordinator.test.ts +++ b/src/renderer/src/components/window-close-request-coordinator.test.ts @@ -2,11 +2,17 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { dispatchWindowCloseRequest, getWindowCloseRequestHandler, + registerWindowCloseGuard, setWindowCloseRequestHandler } from './window-close-request-coordinator' describe('window-close-request-coordinator', () => { const confirmWindowClose = vi.fn() + const unregisterFns: (() => void)[] = [] + + const addGuard = (guard: () => boolean | Promise): void => { + unregisterFns.push(registerWindowCloseGuard(guard)) + } beforeEach(() => { confirmWindowClose.mockClear() @@ -19,6 +25,7 @@ describe('window-close-request-coordinator', () => { afterEach(() => { setWindowCloseRequestHandler(null) + unregisterFns.splice(0).forEach((fn) => fn()) }) it('has no handler by default, so the App root falls back to confirming the close', () => { @@ -40,21 +47,84 @@ describe('window-close-request-coordinator', () => { }) // The #5144 contract: a close request must always be acted on. - it('confirms the close directly when no rich handler is registered (no-workspace path)', () => { - dispatchWindowCloseRequest({ isQuitting: true }) + it('confirms the close directly when no rich handler is registered (no-workspace path)', async () => { + await dispatchWindowCloseRequest({ isQuitting: true }) expect(confirmWindowClose).toHaveBeenCalledTimes(1) }) - it('delegates to the rich handler and does NOT confirm directly when one is registered', () => { + it('delegates to the rich handler and does NOT confirm directly when one is registered', async () => { const handler = vi.fn() setWindowCloseRequestHandler(handler) - dispatchWindowCloseRequest({ isQuitting: false }) + await dispatchWindowCloseRequest({ isQuitting: false }) expect(handler).toHaveBeenCalledWith({ isQuitting: false }) // Why: confirmation is the rich handler's responsibility (after save dialogs // / running-process checks) — dispatch must not short-circuit it. expect(confirmWindowClose).not.toHaveBeenCalled() }) + + // Pre-close guards (e.g. unsaved Settings prompt drafts). + it('cancels the close — no confirm, no handler — when a guard vetoes', async () => { + const handler = vi.fn() + setWindowCloseRequestHandler(handler) + addGuard(() => false) + + await dispatchWindowCloseRequest({ isQuitting: true }) + + expect(confirmWindowClose).not.toHaveBeenCalled() + expect(handler).not.toHaveBeenCalled() + }) + + it('proceeds to confirm when all guards allow the close', async () => { + addGuard(() => true) + addGuard(async () => true) + + await dispatchWindowCloseRequest({ isQuitting: true }) + + expect(confirmWindowClose).toHaveBeenCalledTimes(1) + }) + + it('short-circuits on the first vetoing guard', async () => { + const second = vi.fn(() => true) + addGuard(() => false) + addGuard(second) + + await dispatchWindowCloseRequest({ isQuitting: true }) + + expect(second).not.toHaveBeenCalled() + expect(confirmWindowClose).not.toHaveBeenCalled() + }) + + it('ignores a re-entrant close request while a guard is still pending', async () => { + let resolveGuard: (value: boolean) => void = () => {} + const guard = vi.fn( + () => + new Promise((resolve) => { + resolveGuard = resolve + }) + ) + addGuard(guard) + + const first = dispatchWindowCloseRequest({ isQuitting: true }) + // Second request arrives while the first guard's dialog is still open. + await dispatchWindowCloseRequest({ isQuitting: true }) + expect(guard).toHaveBeenCalledTimes(1) + + resolveGuard(true) + await first + expect(confirmWindowClose).toHaveBeenCalledTimes(1) + }) + + it('stops consulting a guard once it is unregistered', async () => { + const guard = vi.fn(() => false) + const unregister = registerWindowCloseGuard(guard) + unregister() + + await dispatchWindowCloseRequest({ isQuitting: true }) + + expect(guard).not.toHaveBeenCalled() + expect(confirmWindowClose).toHaveBeenCalledTimes(1) + }) }) diff --git a/src/renderer/src/components/window-close-request-coordinator.ts b/src/renderer/src/components/window-close-request-coordinator.ts index db4c293928d..f4939aeb01e 100644 --- a/src/renderer/src/components/window-close-request-coordinator.ts +++ b/src/renderer/src/components/window-close-request-coordinator.ts @@ -4,10 +4,22 @@ // the no-workspace landing page — where Terminal (and its listener) is not // mounted — sends 'window:close-requested' to a renderer with no handler, so // confirmWindowClose() is never called and the window never closes (#5144). +// +// It also runs pre-close guards: surfaces with unsaved work (e.g. the Settings +// Git AI Author prompt editors) register a guard so quitting prompts the user to +// save/discard instead of being silently vetoed by a beforeunload handler. export type WindowCloseRequestHandler = (data: { isQuitting: boolean }) => void +/** Returns true to allow the close to proceed, false to cancel it (e.g. the user + * picked "Cancel" in an unsaved-changes prompt). */ +export type WindowCloseGuard = () => boolean | Promise + let activeHandler: WindowCloseRequestHandler | null = null +const closeGuards = new Set() +// Why: a guard can await a dialog; ignore re-entrant close requests (main resends +// 'window:close-requested' on each attempt) so we don't stack duplicate prompts. +let closeInFlight = false /** Terminal registers its rich handler while mounted; passing null on unmount * hands the decision back to the App-root fallback. */ @@ -19,11 +31,40 @@ export function getWindowCloseRequestHandler(): WindowCloseRequestHandler | null return activeHandler } -/** Route a main-process close request: delegate to Terminal's rich handler when - * mounted, else confirm directly. Why confirm directly: with no workbench - * mounted there are no terminals or editor tabs to protect, so blocking would - * just deadlock the window (#5144). */ -export function dispatchWindowCloseRequest(data: { isQuitting: boolean }): void { +/** Register a pre-close guard. Returns an unregister function for effect cleanup. */ +export function registerWindowCloseGuard(guard: WindowCloseGuard): () => void { + closeGuards.add(guard) + return () => { + closeGuards.delete(guard) + } +} + +async function runWindowCloseGuards(): Promise { + for (const guard of closeGuards) { + if (!(await guard())) { + return false + } + } + return true +} + +/** Route a main-process close request: run pre-close guards first (cancel if any + * vetoes), then delegate to Terminal's rich handler when mounted, else confirm + * directly. Why confirm directly: with no workbench mounted there are no + * terminals or editor tabs to protect, so blocking would just deadlock the + * window (#5144). */ +export async function dispatchWindowCloseRequest(data: { isQuitting: boolean }): Promise { + if (closeInFlight) { + return + } + closeInFlight = true + try { + if (!(await runWindowCloseGuards())) { + return + } + } finally { + closeInFlight = false + } if (activeHandler) { activeHandler(data) return