mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 16:02:45 +00:00
fix: prompt to discard unsaved Settings AI prompts on quit instead of silently blocking (#5335)
This commit is contained in:
@@ -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<void> => {
|
||||
@@ -376,11 +382,11 @@ function Settings(): React.JSX.Element {
|
||||
cancelPendingSettingsSubsectionScrollFrame(pendingSubsectionScrollFrameRef)
|
||||
}, [])
|
||||
|
||||
const confirmDiscardSourceControlAiPromptChanges = useCallback(async (): Promise<boolean> => {
|
||||
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<boolean> => {
|
||||
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<boolean> => {
|
||||
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<void> => {
|
||||
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 => {
|
||||
|
||||
@@ -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<boolean>): 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<boolean>((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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<boolean>
|
||||
|
||||
let activeHandler: WindowCloseRequestHandler | null = null
|
||||
const closeGuards = new Set<WindowCloseGuard>()
|
||||
// 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<boolean> {
|
||||
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<void> {
|
||||
if (closeInFlight) {
|
||||
return
|
||||
}
|
||||
closeInFlight = true
|
||||
try {
|
||||
if (!(await runWindowCloseGuards())) {
|
||||
return
|
||||
}
|
||||
} finally {
|
||||
closeInFlight = false
|
||||
}
|
||||
if (activeHandler) {
|
||||
activeHandler(data)
|
||||
return
|
||||
|
||||
Reference in New Issue
Block a user