From a2114892304d015c77d5bf9ed2c0367eee514178 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:42:11 -0700 Subject: [PATCH] Add saved force-delete preference to workspace deletion menu (#26425) * Give Windows Electron writer probes a larger test budget * Drain queued writer replies across two poll turns before timing out * Check writer shutdown after both reply-drain turns * Add inline always-force-delete preference to workspace recovery toast * Use split-button menu for saved force-delete preference * Await durable force-delete preference writes before deleting * Restore absent settings keys after failed durable writes * Place deletion translations beside worktree copy --- src/main/ipc/settings.test.ts | 19 +++ src/main/ipc/settings.ts | 8 +- .../durable-settings-write.test.ts | 96 +++++++++++ .../loading-store/profile-preferences.ts | 47 ++++++ src/renderer/src/App.tsx | 2 +- .../sidebar/WorktreeForceDeleteButton.tsx | 111 +++++++++++++ ...-worktree-always-force-preference.test.tsx | 153 ++++++++++++++++++ .../sidebar/delete-worktree-failure-toast.tsx | 18 ++- .../sidebar/run-worktree-delete-with-toast.ts | 2 + src/renderer/src/i18n/locales/en.json | 6 + .../worktrees-always-force-delete.test.ts | 117 ++++++++++++++ .../worktrees/teardown/remove-worktree.ts | 23 +-- .../teardown/worktree-removal-preference.ts | 20 +++ src/shared/default-global-settings.ts | 1 + src/shared/global-settings-types.ts | 2 + 15 files changed, 604 insertions(+), 21 deletions(-) create mode 100644 src/main/persistence/loading-store/durable-settings-write.test.ts create mode 100644 src/renderer/src/components/sidebar/WorktreeForceDeleteButton.tsx create mode 100644 src/renderer/src/components/sidebar/delete-worktree-always-force-preference.test.tsx create mode 100644 src/renderer/src/store/slices/worktrees-always-force-delete.test.ts create mode 100644 src/renderer/src/store/slices/worktrees/teardown/worktree-removal-preference.ts diff --git a/src/main/ipc/settings.test.ts b/src/main/ipc/settings.test.ts index 3089af28e74..b1b4a8cdea4 100644 --- a/src/main/ipc/settings.test.ts +++ b/src/main/ipc/settings.test.ts @@ -99,6 +99,7 @@ type SettingsChangedListener = ( const store = { getSettings: vi.fn(), updateSettings: vi.fn(), + updateSettingsAndFlush: vi.fn(), getGitHubCache: vi.fn(), setGitHubCache: vi.fn(), onSettingsChanged: vi.fn(() => () => {}) @@ -107,6 +108,7 @@ const store = { describe('registerSettingsHandlers', () => { beforeEach(() => { handleMock.mockClear() + store.updateSettingsAndFlush.mockReset() onMock.mockClear() applyAppIconMock.mockClear() applyAgentStatusHooksEnabledMock.mockReset().mockResolvedValue([]) @@ -239,6 +241,23 @@ describe('registerSettingsHandlers', () => { ) }) + it('propagates a failed durable force-delete preference write through settings:set', async () => { + store.getSettings.mockReturnValue({ alwaysForceDeleteWorktrees: false }) + store.updateSettingsAndFlush.mockRejectedValue(new Error('Disk full')) + registerSettingsHandlers(store as never) + const handler = handleMock.mock.calls.find((call) => call[0] === 'settings:set')?.[1] + if (typeof handler !== 'function') { + throw new Error('Missing settings:set handler') + } + await expect( + handler(settingsInvokeEvent, { alwaysForceDeleteWorktrees: true, editorWordWrap: false }) + ).rejects.toThrow('Disk full') + expect(store.updateSettingsAndFlush).toHaveBeenCalledWith( + { alwaysForceDeleteWorktrees: true, editorWordWrap: false }, + { notifyListeners: true, originWebContentsId: 1 } + ) + }) + it('persists Active Server only through the dedicated preference channel', () => { store.updateSettings.mockReturnValue({ activeRuntimeEnvironmentId: 'windows-2' }) registerSettingsHandlers(store as never) diff --git a/src/main/ipc/settings.ts b/src/main/ipc/settings.ts index e2a173e21b1..8442bfd8682 100644 --- a/src/main/ipc/settings.ts +++ b/src/main/ipc/settings.ts @@ -199,10 +199,14 @@ export function registerSettingsHandlers( // (e.g. blur after a no-op edit), and a `settings_changed` event for a // no-op flip would inflate the experimental-feature-adoption signal. const before = store.getSettings() - const result = store.updateSettings(sanitizedArgs, { + const updateOptions = { notifyListeners: true, originWebContentsId: event.sender.id - }) + } + const result = + 'alwaysForceDeleteWorktrees' in sanitizedArgs + ? await store.updateSettingsAndFlush(sanitizedArgs, updateOptions) + : store.updateSettings(sanitizedArgs, updateOptions) const proxySettingsChanged = ('httpProxyUrl' in sanitizedArgs && before.httpProxyUrl !== result.httpProxyUrl) || ('httpProxyBypassRules' in sanitizedArgs && diff --git a/src/main/persistence/loading-store/durable-settings-write.test.ts b/src/main/persistence/loading-store/durable-settings-write.test.ts new file mode 100644 index 00000000000..3a415c8bd66 --- /dev/null +++ b/src/main/persistence/loading-store/durable-settings-write.test.ts @@ -0,0 +1,96 @@ +import { expect, it, vi } from 'vitest' +import { + createWorkerMaintenanceFixture, + maintenanceBarrier +} from './profile-state-maintenance-fixture' + +vi.mock('../../telemetry/client', () => ({ track: vi.fn() })) +vi.mock('../../telemetry/cohort-classifier', () => ({ + getCohortAtEmit: () => ({ nth_repo_added: 2 }) +})) +vi.mock('../../ssh/ssh-config-parser', () => ({ + loadUserSshConfig: () => ({ hosts: [] }), + sshConfigHostsToTargets: () => [] +})) + +it('waits for the SQLite write before returning or publishing the preference', async () => { + const { store, authority, readState } = await createWorkerMaintenanceFixture() + const started = maintenanceBarrier() + const release = maintenanceBarrier() + const write = authority.writeSerializedDomains.bind(authority) + vi.spyOn(authority, 'writeSerializedDomains').mockImplementationOnce(async (domains) => { + started.resolve() + await release.promise + await write(domains) + }) + const onChanged = vi.fn() + store.onSettingsChanged(onChanged) + let settled = false + const pending = store.updateSettingsAndFlush( + { alwaysForceDeleteWorktrees: true }, + { notifyListeners: true, originWebContentsId: 7 } + ) + void pending.then(() => { + settled = true + }) + await started.promise + expect(settled).toBe(false) + expect(onChanged).not.toHaveBeenCalled() + store.updateSettings({ theme: 'dark' }) + release.resolve() + expect((await pending).theme).toBe('dark') + expect(readState().settings.alwaysForceDeleteWorktrees).toBe(true) + expect(onChanged).toHaveBeenCalledWith( + { alwaysForceDeleteWorktrees: true }, + expect.objectContaining({ alwaysForceDeleteWorktrees: true }), + 7 + ) +}) + +it('rejects a failed disk write and rolls back without publishing the preference', async () => { + const { store, authority, readState } = await createWorkerMaintenanceFixture() + const previous = store.getSettings().alwaysForceDeleteWorktrees + vi.spyOn(authority, 'writeSerializedDomains').mockRejectedValueOnce(new Error('Disk full')) + const onChanged = vi.fn() + store.onSettingsChanged(onChanged) + await expect( + store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true }, { notifyListeners: true }) + ).rejects.toThrow('Disk full') + expect(store.getSettings().alwaysForceDeleteWorktrees).toBe(previous) + expect(readState().settings.alwaysForceDeleteWorktrees).toBe(previous) + expect(onChanged).not.toHaveBeenCalled() +}) + +it('rolls back the preference while retaining unrelated concurrent settings edits', async () => { + const { store, authority } = await createWorkerMaintenanceFixture() + const previous = store.getSettings().alwaysForceDeleteWorktrees + const started = maintenanceBarrier() + const release = maintenanceBarrier() + vi.spyOn(authority, 'writeSerializedDomains').mockImplementationOnce(async () => { + started.resolve() + await release.promise + throw new Error('Disk full') + }) + const pending = store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true }) + const rejected = expect(pending).rejects.toThrow('Disk full') + await started.promise + store.updateSettings({ theme: 'dark' }) + release.resolve() + await rejected + expect(store.getSettings()).toMatchObject({ + alwaysForceDeleteWorktrees: previous, + theme: 'dark' + }) +}) + +it('removes previously absent optional keys after a failed preference payload write', async () => { + const { store, authority } = await createWorkerMaintenanceFixture() + delete store.getSettings().editorWordWrap + const previous = store.getSettings().alwaysForceDeleteWorktrees + vi.spyOn(authority, 'writeSerializedDomains').mockRejectedValueOnce(new Error('Disk full')) + await expect( + store.updateSettingsAndFlush({ alwaysForceDeleteWorktrees: true, editorWordWrap: false }) + ).rejects.toThrow('Disk full') + expect(Object.hasOwn(store.getSettings(), 'editorWordWrap')).toBe(false) + expect(store.getSettings().alwaysForceDeleteWorktrees).toBe(previous) +}) diff --git a/src/main/persistence/loading-store/profile-preferences.ts b/src/main/persistence/loading-store/profile-preferences.ts index 15315518961..e3760a5d5f6 100644 --- a/src/main/persistence/loading-store/profile-preferences.ts +++ b/src/main/persistence/loading-store/profile-preferences.ts @@ -25,6 +25,7 @@ type ProfilePreferencesRuntime = Pick< | 'githubCacheDirty' | 'githubCacheGeneration' | 'protectedSecrets' + | 'runDurableMutation' | 'settingsChangeListeners' | 'state' | 'uiChangeListeners' @@ -74,6 +75,52 @@ export class ProfilePreferences { return updateSettingsOperation(getSettingsMutationOperations(this), updates, options) } + async updateSettingsAndFlush( + updates: Partial, + options: { notifyListeners?: boolean; originWebContentsId?: number } = {} + ): Promise { + const { runtime } = this[profilePreferencesContext] + let changedUpdates: Partial = {} + await runtime.runDurableMutation(() => { + const previous = runtime.state.settings + const next = this.updateSettings(updates) + const previousEntries = new Map(Object.entries(previous)) + const updateKeys = new Set(Object.keys(updates)) + changedUpdates = Object.fromEntries( + Object.entries(next).filter( + ([key, value]) => updateKeys.has(key) && !Object.is(previousEntries.get(key), value) + ) + ) + return { + value: undefined, + rollback: () => { + const currentEntries = new Map(Object.entries(runtime.state.settings)) + const nextEntries = new Map(Object.entries(next)) + const restoredUpdates = Object.fromEntries( + Object.entries(previous).filter( + ([key]) => + updateKeys.has(key) && Object.is(currentEntries.get(key), nextEntries.get(key)) + ) + ) + const restoredSettings = { ...runtime.state.settings, ...restoredUpdates } + for (const key of updateKeys) { + if ( + !previousEntries.has(key) && + Object.is(currentEntries.get(key), nextEntries.get(key)) + ) { + Reflect.deleteProperty(restoredSettings, key) + } + } + runtime.state.settings = restoredSettings + } + } + }) + if (options.notifyListeners && Object.keys(changedUpdates).length > 0) { + notifySettingsChanged(this, changedUpdates, options.originWebContentsId) + } + return this.getSettings() + } + getUI(): PersistedState['ui'] { return getPersistedUI( this[profilePreferencesContext].runtime.state, diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 892c445d100..1ed1194a66b 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -101,8 +101,8 @@ function App(): React.JSX.Element { + - diff --git a/src/renderer/src/components/sidebar/WorktreeForceDeleteButton.tsx b/src/renderer/src/components/sidebar/WorktreeForceDeleteButton.tsx new file mode 100644 index 00000000000..259176381ee --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeForceDeleteButton.tsx @@ -0,0 +1,111 @@ +import { useState } from 'react' +import { ChevronDown } from 'lucide-react' +import { toast } from 'sonner' +import { translate } from '@/i18n/i18n' +import { Button } from '../ui/button' +import { ButtonGroup } from '../ui/button-group' +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger +} from '../ui/dropdown-menu' +import { Tooltip, TooltipContent, TooltipTrigger } from '../ui/tooltip' + +export function WorktreeForceDeleteButton({ + toastId, + onForceDelete, + onAlwaysForceDelete +}: { + toastId: string + onForceDelete: () => void + onAlwaysForceDelete?: () => Promise +}): React.JSX.Element { + const [isSaving, setIsSaving] = useState(false) + const forceDelete = (): void => { + toast.dismiss(toastId) + onForceDelete() + } + const alwaysForceDelete = async (): Promise => { + if (!onAlwaysForceDelete) { + return + } + setIsSaving(true) + try { + await onAlwaysForceDelete() + } catch (error) { + setIsSaving(false) + toast.error( + translate('workspaceDeletion.preferenceSaveFailed', 'Could not save deletion preference'), + { description: error instanceof Error ? error.message : String(error) } + ) + return + } + forceDelete() + } + + return ( + + { + if ( + event.relatedTarget instanceof HTMLElement && + event.relatedTarget.closest('[data-radix-menu-content]') + ) { + event.stopPropagation() + } + }} + > + + {onAlwaysForceDelete ? ( + + + + ) : null} + + {onAlwaysForceDelete ? ( + event.stopPropagation()} + onBlur={(event) => event.stopPropagation()} + > + + + { + void alwaysForceDelete() + }} + > + {translate('workspaceDeletion.alwaysForceDelete', 'Always force delete')} + + + + {translate( + 'workspaceDeletion.alwaysForceDeleteTooltip', + 'Applies to all future workspace deletions.' + )} + + + + ) : null} + + ) +} diff --git a/src/renderer/src/components/sidebar/delete-worktree-always-force-preference.test.tsx b/src/renderer/src/components/sidebar/delete-worktree-always-force-preference.test.tsx new file mode 100644 index 00000000000..c372f5204c6 --- /dev/null +++ b/src/renderer/src/components/sidebar/delete-worktree-always-force-preference.test.tsx @@ -0,0 +1,153 @@ +// @vitest-environment happy-dom + +import { act, isValidElement } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, expect, it, vi } from 'vitest' +import { toast } from 'sonner' +import { TooltipProvider } from '../ui/tooltip' +import { showDeleteWorktreeFailureToast } from './delete-worktree-failure-toast' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), error: vi.fn(), dismiss: vi.fn() } })) + +let root: Root | undefined + +function renderRecovery(onAlwaysForceDelete: () => Promise, canForceDelete = true) { + const onForceDelete = vi.fn() + const onOutsideBlur = vi.fn() + showDeleteWorktreeFailureToast({ + error: 'branch has changes', + canForceDelete, + forceDeleteReason: canForceDelete ? 'dirty' : null, + onViewChanges: vi.fn(), + onForceDelete, + onAlwaysForceDelete, + onDeleteAnyway: vi.fn(), + worktreeId: 'wt-1', + worktreeName: 'feature/foo' + }) + const description = vi + .mocked(canForceDelete ? toast.info : toast.error) + .mock.calls.at(-1)?.[1]?.description + if (!isValidElement(description)) { + throw new Error('Expected a rendered recovery toast') + } + const container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => + root?.render( + +
{ + if (!event.currentTarget.contains(event.relatedTarget)) { + onOutsideBlur() + } + }} + > + {description} +
+ + ) + ) + return { container, onForceDelete, onOutsideBlur } +} + +function click(container: HTMLElement, selector: string): void { + const element = container.querySelector(selector) + if (!(element instanceof HTMLElement)) { + throw new Error(`Missing ${selector}`) + } + element.click() +} + +async function selectAlwaysForceDelete(container: HTMLElement): Promise { + await act(async () => { + const trigger = container.querySelector('button[aria-label="More force delete options"]') + if (!(trigger instanceof HTMLElement)) { + throw new Error('Missing dropdown trigger') + } + trigger.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + }) + await act(async () => click(document.body, '[role="menuitem"]')) +} + +afterEach(() => { + act(() => root?.unmount()) + root = undefined + document.body.innerHTML = '' + vi.clearAllMocks() +}) + +it('keeps the main Force Delete button a one-time action', async () => { + const savePreference = vi.fn().mockResolvedValue(undefined) + const { container, onForceDelete } = renderRecovery(savePreference) + expect(container.querySelector('button[aria-label="More force delete options"]')).not.toBeNull() + expect(document.querySelector('[role="menuitem"]')).toBeNull() + + await act(async () => click(container, 'button[data-variant="destructive"]')) + + expect(savePreference).not.toHaveBeenCalled() + expect(onForceDelete).toHaveBeenCalledOnce() +}) + +it('keeps portaled menu focus from triggering toast focus restoration', async () => { + const { container, onOutsideBlur } = renderRecovery(vi.fn()) + const trigger = container.querySelector('button[aria-label="More force delete options"]') + if (!(trigger instanceof HTMLElement)) { + throw new Error('Missing dropdown trigger') + } + await act(async () => { + trigger.focus() + trigger.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + }) + const item = document.querySelector('[role="menuitem"]') + if (!(item instanceof HTMLElement)) { + throw new Error('Missing preference menu item') + } + await act(async () => item.focus()) + expect(onOutsideBlur).not.toHaveBeenCalled() + expect(document.querySelector('[role="menu"]')).not.toBeNull() +}) + +it('waits for the preference to persist before force-deleting and dismissing', async () => { + let finishSaving: (() => void) | undefined + const savePreference = vi.fn( + () => + new Promise((resolve) => { + finishSaving = resolve + }) + ) + const { container, onForceDelete } = renderRecovery(savePreference) + await selectAlwaysForceDelete(container) + + expect(savePreference).toHaveBeenCalledOnce() + expect( + container.querySelector('button[data-variant="destructive"]')?.hasAttribute('disabled') + ).toBe(true) + expect(onForceDelete).not.toHaveBeenCalled() + expect(toast.dismiss).not.toHaveBeenCalled() + await act(async () => finishSaving?.()) + expect(onForceDelete).toHaveBeenCalledOnce() + expect(toast.dismiss).toHaveBeenCalledWith('delete-worktree-failure:wt-1') +}) + +it('keeps recovery available when saving the preference fails', async () => { + const { container, onForceDelete } = renderRecovery( + vi.fn().mockRejectedValue(new Error('Disk full')) + ) + await selectAlwaysForceDelete(container) + + expect(onForceDelete).not.toHaveBeenCalled() + expect(toast.dismiss).not.toHaveBeenCalled() + expect(toast.error).toHaveBeenCalledWith('Could not save deletion preference', { + description: 'Disk full' + }) + expect( + container.querySelector('button[data-variant="destructive"]')?.hasAttribute('disabled') + ).toBe(false) +}) + +it('does not offer the preference for a failure that cannot be force-deleted', () => { + const { container } = renderRecovery(vi.fn(), false) + expect(container.querySelector('button[aria-label="More force delete options"]')).toBeNull() +}) diff --git a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx index 5f6e93444fa..8dc934ae426 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx +++ b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx @@ -3,6 +3,7 @@ import { Button } from '../ui/button' import { getDeleteWorktreeToastCopy } from './delete-worktree-toast' import { translate } from '@/i18n/i18n' import { DeleteNestedWorktreesDialog } from './DeleteNestedWorktreesDialog' +import { WorktreeForceDeleteButton } from './WorktreeForceDeleteButton' import { isNestedWorktreeRemovalError } from '../../../../shared/worktree/nested-removal' import { isLockedWorktreeRemovalError, @@ -20,6 +21,7 @@ type DeleteWorktreeFailureToastOptions = { canWaiveArchiveHook?: boolean onViewChanges: () => void onForceDelete: () => void + onAlwaysForceDelete?: () => Promise onDeleteAnyway: () => void worktreeId: string worktreeName: string @@ -38,6 +40,7 @@ function DeleteWorktreeFailureToastBody({ showViewChanges, onViewChanges, onForceDelete, + onAlwaysForceDelete, onDeleteAnyway, toastId, nestedRemovalTarget, @@ -50,6 +53,7 @@ function DeleteWorktreeFailureToastBody({ showViewChanges: boolean onViewChanges: () => void onForceDelete: () => void + onAlwaysForceDelete?: () => Promise onDeleteAnyway: () => void toastId: string nestedRemovalTarget?: WorktreeRemovalTarget @@ -60,10 +64,6 @@ function DeleteWorktreeFailureToastBody({ toast.dismiss(toastId) onViewChanges() } - const forceDelete = (): void => { - toast.dismiss(toastId) - onForceDelete() - } const deleteAnyway = (): void => { toast.dismiss(toastId) onDeleteAnyway() @@ -89,9 +89,11 @@ function DeleteWorktreeFailureToastBody({ /> ) : null} {canForceDelete ? ( - + ) : null} {canWaiveArchiveHook ? (