diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index cdb2d02d1fe..049f17943d4 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -29,9 +29,13 @@ import { import { isUpdaterQuitAndInstallInProgress } from '@/lib/updater-beforeunload' import EditorAutosaveController from './editor/EditorAutosaveController' import type { TabGroupLayoutNode } from '../../../shared/types' -import BrowserPane, { destroyPersistentWebview } from './browser-pane/BrowserPane' +import BrowserPane from './browser-pane/BrowserPane' +import { destroyPersistentWebview } from './browser-pane/webview-registry' import BrowserPaneOverlayLayer from './browser-pane/BrowserPaneOverlayLayer' -import { collectBrowserWebviewIds } from './browser-pane/browser-webview-cleanup' +import { + collectBrowserWebviewIds, + destroyWorkspaceWebviews +} from '../store/slices/browser-webview-cleanup' import { handleSwitchTab, handleSwitchTerminalTab } from '../hooks/ipc-tab-switch' import TabGroupSplitLayout from './tab-group/TabGroupSplitLayout' import { shouldAutoCreateInitialTerminal } from './terminal/initial-terminal' @@ -536,7 +540,7 @@ function Terminal(): React.JSX.Element | null { } const currentTabs = state.browserTabsByWorktree[owningWorktreeId] ?? [] if (currentTabs.length <= 1) { - destroyPersistentWebview(tabId) + destroyWorkspaceWebviews(state.browserPagesByWorkspace, tabId) closeBrowserTab(tabId) if (state.activeWorktreeId === owningWorktreeId) { const worktreeFile = state.openFiles.find((file) => file.worktreeId === owningWorktreeId) @@ -562,7 +566,7 @@ function Terminal(): React.JSX.Element | null { setActiveBrowserTab(nextTab.id) } } - destroyPersistentWebview(tabId) + destroyWorkspaceWebviews(state.browserPagesByWorkspace, tabId) closeBrowserTab(tabId) }, [ @@ -611,7 +615,7 @@ function Terminal(): React.JSX.Element | null { } else if ( (state.browserTabsByWorktree[activeWorktreeId] ?? []).some((tab) => tab.id === id) ) { - destroyPersistentWebview(id) + destroyWorkspaceWebviews(state.browserPagesByWorkspace, id) closeBrowserTab(id) } } @@ -641,7 +645,7 @@ function Terminal(): React.JSX.Element | null { } else if ( (state.browserTabsByWorktree[activeWorktreeId] ?? []).some((tab) => tab.id === id) ) { - destroyPersistentWebview(id) + destroyWorkspaceWebviews(state.browserPagesByWorkspace, id) closeBrowserTab(id) } } diff --git a/src/renderer/src/components/browser-pane/BrowserPane.tsx b/src/renderer/src/components/browser-pane/BrowserPane.tsx index ae35fd16455..0d9ec953754 100644 --- a/src/renderer/src/components/browser-pane/BrowserPane.tsx +++ b/src/renderer/src/components/browser-pane/BrowserPane.tsx @@ -38,11 +38,18 @@ import { normalizeExternalBrowserUrl } from '../../../../shared/browser-url' import { - clearLiveBrowserUrl, consumeEvictedBrowserTab, markEvictedBrowserTab, rememberLiveBrowserUrl } from './browser-runtime' +import { + destroyPersistentWebview, + getHiddenContainer, + MAX_PARKED_WEBVIEWS, + parkedAtByTabId, + registeredWebContentsIds, + webviewRegistry +} from './webview-registry' import type { BrowserDownloadRequestedEvent, BrowserDownloadProgressEvent, @@ -84,84 +91,7 @@ type BrowserDownloadState = BrowserDownloadRequestedEvent & { status: 'requested' | 'downloading' } -const webviewRegistry = new Map() -const registeredWebContentsIds = new Map() -const parkedAtByTabId = new Map() -let hiddenContainer: HTMLDivElement | null = null -const DRAG_LISTENER_KEY = '__orcaBrowserPaneDragListeners' -const MAX_PARKED_WEBVIEWS = 6 const EMPTY_BROWSER_PAGES: BrowserPageState[] = [] -function getHiddenContainer(): HTMLDivElement { - if (!hiddenContainer) { - hiddenContainer = document.createElement('div') - hiddenContainer.style.position = 'fixed' - hiddenContainer.style.left = '-9999px' - hiddenContainer.style.top = '-9999px' - hiddenContainer.style.width = '100vw' - hiddenContainer.style.height = '100vh' - hiddenContainer.style.overflow = 'hidden' - hiddenContainer.style.pointerEvents = 'none' - document.body.appendChild(hiddenContainer) - } - return hiddenContainer -} - -function setWebviewsDragPassthrough(passthrough: boolean): void { - for (const webview of webviewRegistry.values()) { - webview.style.pointerEvents = passthrough ? 'none' : '' - } -} - -if (typeof window !== 'undefined') { - type DragListenerRegistry = { - dragstart: () => void - dragend: () => void - drop: () => void - } - const listenerHost = window as Window & { [DRAG_LISTENER_KEY]?: DragListenerRegistry } - const existingListeners = listenerHost[DRAG_LISTENER_KEY] - if (existingListeners) { - window.removeEventListener('dragstart', existingListeners.dragstart, true) - window.removeEventListener('dragend', existingListeners.dragend, true) - window.removeEventListener('drop', existingListeners.drop, true) - } - - const dragstart = (): void => setWebviewsDragPassthrough(true) - const dragend = (): void => setWebviewsDragPassthrough(false) - const drop = (): void => setWebviewsDragPassthrough(false) - - window.addEventListener('dragstart', dragstart, true) - window.addEventListener('dragend', dragend, true) - window.addEventListener('drop', drop, true) - // Why: BrowserPane installs process-wide drag listeners so parked webviews - // stop swallowing drop targets. We store/remove the previous handlers on - // window to keep Vite HMR from stacking duplicates across module reloads. - listenerHost[DRAG_LISTENER_KEY] = { dragstart, dragend, drop } -} - -export function destroyPersistentWebview(browserTabId: string): void { - const webview = webviewRegistry.get(browserTabId) - if (!webview) { - registeredWebContentsIds.delete(browserTabId) - parkedAtByTabId.delete(browserTabId) - clearLiveBrowserUrl(browserTabId) - return - } - void window.api.browser.unregisterGuest({ browserPageId: browserTabId }) - // Why: if this webview currently owns focus, removing it lets macOS hand - // activation back to the previously-active app (Slack, etc.) because the - // focused webContents is gone with no replacement. Move focus back into the - // main renderer first so Electron keeps focus inside the Orca window. - if (webview === document.activeElement || webview.contains(document.activeElement)) { - ;(document.activeElement as HTMLElement | null)?.blur?.() - window.focus() - } - webview.remove() - webviewRegistry.delete(browserTabId) - registeredWebContentsIds.delete(browserTabId) - parkedAtByTabId.delete(browserTabId) - clearLiveBrowserUrl(browserTabId) -} function buildLoadError(event: { errorCode?: number diff --git a/src/renderer/src/components/browser-pane/browser-webview-cleanup.test.ts b/src/renderer/src/components/browser-pane/browser-webview-cleanup.test.ts deleted file mode 100644 index de439ef80df..00000000000 --- a/src/renderer/src/components/browser-pane/browser-webview-cleanup.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { collectBrowserWebviewIds } from './browser-webview-cleanup' -import type { BrowserPage, BrowserWorkspace } from '../../../../shared/types' - -function workspace(id: string): BrowserWorkspace { - return { - id, - worktreeId: 'wt-1', - label: id, - sessionProfileId: null, - pageIds: [], - activePageId: null, - url: 'about:blank', - title: id, - loading: false, - faviconUrl: null, - canGoBack: false, - canGoForward: false, - loadError: null, - createdAt: 1 - } -} - -function page(id: string, workspaceId: string): BrowserPage { - return { - id, - workspaceId, - worktreeId: 'wt-1', - url: 'about:blank', - title: id, - loading: false, - faviconUrl: null, - canGoBack: false, - canGoForward: false, - loadError: null, - createdAt: 1 - } -} - -describe('collectBrowserWebviewIds', () => { - it('tracks browser page ids because webviews are keyed by page id', () => { - const ids = collectBrowserWebviewIds( - { 'wt-1': [workspace('workspace-1')] }, - { 'workspace-1': [page('page-1', 'workspace-1'), page('page-2', 'workspace-1')] } - ) - - expect([...ids].sort()).toEqual(['page-1', 'page-2']) - }) - - it('keeps legacy workspace ids only when no page records exist', () => { - const ids = collectBrowserWebviewIds({ 'wt-1': [workspace('legacy-workspace')] }, {}) - - expect([...ids]).toEqual(['legacy-workspace']) - }) -}) diff --git a/src/renderer/src/components/browser-pane/webview-registry.ts b/src/renderer/src/components/browser-pane/webview-registry.ts new file mode 100644 index 00000000000..e59c0c6a042 --- /dev/null +++ b/src/renderer/src/components/browser-pane/webview-registry.ts @@ -0,0 +1,88 @@ +import { clearLiveBrowserUrl } from './browser-runtime' + +// Why: the webview registry is shared coordination state between BrowserPane +// (React component) and store-layer cleanup helpers (shutdownWorktreeBrowsers, +// subscriber diff). Keeping it in its own non-React module breaks the cycle +// store/slices → components → @/store that would otherwise appear if +// destroyPersistentWebview lived in BrowserPane.tsx. +export const webviewRegistry = new Map() +export const registeredWebContentsIds = new Map() +export const parkedAtByTabId = new Map() + +export const MAX_PARKED_WEBVIEWS = 6 + +let hiddenContainer: HTMLDivElement | null = null + +export function getHiddenContainer(): HTMLDivElement { + if (!hiddenContainer) { + hiddenContainer = document.createElement('div') + hiddenContainer.style.position = 'fixed' + hiddenContainer.style.left = '-9999px' + hiddenContainer.style.top = '-9999px' + hiddenContainer.style.width = '100vw' + hiddenContainer.style.height = '100vh' + hiddenContainer.style.overflow = 'hidden' + hiddenContainer.style.pointerEvents = 'none' + document.body.appendChild(hiddenContainer) + } + return hiddenContainer +} + +export function setWebviewsDragPassthrough(passthrough: boolean): void { + for (const webview of webviewRegistry.values()) { + webview.style.pointerEvents = passthrough ? 'none' : '' + } +} + +const DRAG_LISTENER_KEY = '__orcaBrowserPaneDragListeners' + +if (typeof window !== 'undefined') { + type DragListenerRegistry = { + dragstart: () => void + dragend: () => void + drop: () => void + } + const listenerHost = window as Window & { [DRAG_LISTENER_KEY]?: DragListenerRegistry } + const existingListeners = listenerHost[DRAG_LISTENER_KEY] + if (existingListeners) { + window.removeEventListener('dragstart', existingListeners.dragstart, true) + window.removeEventListener('dragend', existingListeners.dragend, true) + window.removeEventListener('drop', existingListeners.drop, true) + } + + const dragstart = (): void => setWebviewsDragPassthrough(true) + const dragend = (): void => setWebviewsDragPassthrough(false) + const drop = (): void => setWebviewsDragPassthrough(false) + + window.addEventListener('dragstart', dragstart, true) + window.addEventListener('dragend', dragend, true) + window.addEventListener('drop', drop, true) + // Why: BrowserPane installs process-wide drag listeners so parked webviews + // stop swallowing drop targets. We store/remove the previous handlers on + // window to keep Vite HMR from stacking duplicates across module reloads. + listenerHost[DRAG_LISTENER_KEY] = { dragstart, dragend, drop } +} + +export function destroyPersistentWebview(browserTabId: string): void { + const webview = webviewRegistry.get(browserTabId) + if (!webview) { + registeredWebContentsIds.delete(browserTabId) + parkedAtByTabId.delete(browserTabId) + clearLiveBrowserUrl(browserTabId) + return + } + void window.api.browser.unregisterGuest({ browserPageId: browserTabId }) + // Why: if this webview currently owns focus, removing it lets macOS hand + // activation back to the previously-active app (Slack, etc.) because the + // focused webContents is gone with no replacement. Move focus back into the + // main renderer first so Electron keeps focus inside the Orca window. + if (webview === document.activeElement || webview.contains(document.activeElement)) { + ;(document.activeElement as HTMLElement | null)?.blur?.() + window.focus() + } + webview.remove() + webviewRegistry.delete(browserTabId) + registeredWebContentsIds.delete(browserTabId) + parkedAtByTabId.delete(browserTabId) + clearLiveBrowserUrl(browserTabId) +} diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts new file mode 100644 index 00000000000..38266e97d2d --- /dev/null +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => { + const state = { + activeWorktreeId: null as string | null, + setActiveWorktree: vi.fn(), + shutdownWorktreeBrowsers: vi.fn().mockResolvedValue(undefined), + shutdownWorktreeTerminals: vi.fn().mockResolvedValue(undefined) + } + const toastError = vi.fn() + return { state, toastError } +}) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => mocks.state + } +})) + +vi.mock('sonner', () => ({ toast: { error: mocks.toastError } })) + +import { runSleepWorktree } from './sleep-worktree-flow' + +describe('runSleepWorktree', () => { + beforeEach(() => { + mocks.state.setActiveWorktree.mockClear() + mocks.state.shutdownWorktreeBrowsers.mockClear().mockResolvedValue(undefined) + mocks.state.shutdownWorktreeTerminals.mockClear().mockResolvedValue(undefined) + mocks.toastError.mockClear() + mocks.state.activeWorktreeId = null + }) + + it('tears down browsers before terminals on the sleep path', async () => { + mocks.state.activeWorktreeId = 'wt-1' + + await runSleepWorktree('wt-1') + + // Why: browsers must run first so destroyPersistentWebview can unregister + // the Chromium guests while browserTabsByWorktree/browserPagesByWorkspace + // are still populated. If terminals ran first and kept its old + // browserTabsByWorktree delete, browsers would no-op and leak webviews. + expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-1') + expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-1') + const browsersCallOrder = mocks.state.shutdownWorktreeBrowsers.mock.invocationCallOrder[0] + const terminalsCallOrder = mocks.state.shutdownWorktreeTerminals.mock.invocationCallOrder[0] + expect(browsersCallOrder).toBeLessThan(terminalsCallOrder) + }) + + it('clears activeWorktreeId before teardown when the slept worktree is active', async () => { + mocks.state.activeWorktreeId = 'wt-1' + + await runSleepWorktree('wt-1') + + expect(mocks.state.setActiveWorktree).toHaveBeenCalledWith(null) + const activeClear = mocks.state.setActiveWorktree.mock.invocationCallOrder[0] + const browsersCall = mocks.state.shutdownWorktreeBrowsers.mock.invocationCallOrder[0] + expect(activeClear).toBeLessThan(browsersCall) + }) + + it('leaves activeWorktreeId alone when sleeping a background worktree', async () => { + mocks.state.activeWorktreeId = 'wt-other' + + await runSleepWorktree('wt-1') + + expect(mocks.state.setActiveWorktree).not.toHaveBeenCalled() + }) + + it('surfaces a toast and skips terminals when browsers throws', async () => { + mocks.state.activeWorktreeId = 'wt-1' + mocks.state.shutdownWorktreeBrowsers.mockRejectedValueOnce(new Error('boom')) + + await runSleepWorktree('wt-1') + + expect(mocks.state.shutdownWorktreeTerminals).not.toHaveBeenCalled() + expect(mocks.toastError).toHaveBeenCalledWith( + 'Failed to sleep workspace', + expect.objectContaining({ description: 'boom' }) + ) + }) +}) diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts index b8e2f6f5f8f..af643976628 100644 --- a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts @@ -15,11 +15,22 @@ import { useAppStore } from '@/store' * a new caller can't accidentally skip it. */ export async function runSleepWorktree(worktreeId: string): Promise { - const { activeWorktreeId, setActiveWorktree, shutdownWorktreeTerminals } = useAppStore.getState() + const { + activeWorktreeId, + setActiveWorktree, + shutdownWorktreeBrowsers, + shutdownWorktreeTerminals + } = useAppStore.getState() if (activeWorktreeId === worktreeId) { setActiveWorktree(null) } try { + // Why: sleep mirrors removeWorktree's shutdown sequence — browsers first + // so destroyPersistentWebview unregisters the Chromium guests before any + // other teardown runs, terminals second so the PTY kill uses the same + // ordering on both paths. Without the browser thunk here, sleep leaks + // browserPagesByWorkspace entries and live webviews for the slept worktree. + await shutdownWorktreeBrowsers(worktreeId) await shutdownWorktreeTerminals(worktreeId) } catch (err) { // Why: callers are fire-and-forget; surface the failure as a toast and diff --git a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts index 7c7a660a97b..107e3888160 100644 --- a/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts +++ b/src/renderer/src/components/tab-group/useTabGroupWorkspaceModel.ts @@ -16,7 +16,7 @@ import { useAllWorktrees } from '../../store/selectors' import { createUntitledMarkdownFile } from '../../lib/create-untitled-markdown' import { getConnectionId } from '../../lib/connection-context' import { extractIpcErrorMessage } from '../../lib/ipc-error' -import { destroyPersistentWebview } from '../browser-pane/BrowserPane' +import { destroyWorkspaceWebviews } from '../../store/slices/browser-webview-cleanup' import { focusTerminalTabSurface } from '../../lib/focus-terminal-tab-surface' export type GroupEditorItem = OpenFile & { tabId: string } @@ -196,7 +196,7 @@ export function useTabGroupWorkspaceModel({ if (item.contentType === 'terminal') { closeTab(item.entityId) } else if (item.contentType === 'browser') { - destroyPersistentWebview(item.entityId) + destroyWorkspaceWebviews(useAppStore.getState().browserPagesByWorkspace, item.entityId) closeBrowserTab(item.entityId) } else { closeEditorIfUnreferenced(item.entityId, item.id) @@ -226,7 +226,7 @@ export function useTabGroupWorkspaceModel({ if (item.contentType === 'terminal') { closeTab(item.entityId) } else if (item.contentType === 'browser') { - destroyPersistentWebview(item.entityId) + destroyWorkspaceWebviews(useAppStore.getState().browserPagesByWorkspace, item.entityId) closeBrowserTab(item.entityId) } else { closeEditorIfUnreferenced(item.entityId, item.id) diff --git a/src/renderer/src/store/slices/browser-webview-cleanup.test.ts b/src/renderer/src/store/slices/browser-webview-cleanup.test.ts new file mode 100644 index 00000000000..3d8d4196bfa --- /dev/null +++ b/src/renderer/src/store/slices/browser-webview-cleanup.test.ts @@ -0,0 +1,92 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { BrowserPage, BrowserWorkspace } from '../../../../shared/types' + +vi.mock('../../components/browser-pane/webview-registry', () => ({ + destroyPersistentWebview: vi.fn() +})) + +import { collectBrowserWebviewIds, destroyWorkspaceWebviews } from './browser-webview-cleanup' +import { destroyPersistentWebview } from '../../components/browser-pane/webview-registry' + +function workspace(id: string): BrowserWorkspace { + return { + id, + worktreeId: 'wt-1', + label: id, + sessionProfileId: null, + pageIds: [], + activePageId: null, + url: 'about:blank', + title: id, + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } +} + +function page(id: string, workspaceId: string): BrowserPage { + return { + id, + workspaceId, + worktreeId: 'wt-1', + url: 'about:blank', + title: id, + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } +} + +describe('collectBrowserWebviewIds', () => { + it('tracks browser page ids because webviews are keyed by page id', () => { + const ids = collectBrowserWebviewIds( + { 'wt-1': [workspace('workspace-1')] }, + { 'workspace-1': [page('page-1', 'workspace-1'), page('page-2', 'workspace-1')] } + ) + + expect([...ids].sort()).toEqual(['page-1', 'page-2']) + }) + + it('keeps legacy workspace ids only when no page records exist', () => { + const ids = collectBrowserWebviewIds({ 'wt-1': [workspace('legacy-workspace')] }, {}) + + expect([...ids]).toEqual(['legacy-workspace']) + }) +}) + +describe('destroyWorkspaceWebviews', () => { + beforeEach(() => { + vi.mocked(destroyPersistentWebview).mockClear() + }) + + it('destroys every page id for a multi-page workspace', () => { + destroyWorkspaceWebviews( + { 'workspace-1': [page('page-1', 'workspace-1'), page('page-2', 'workspace-1')] }, + 'workspace-1' + ) + + expect(destroyPersistentWebview).toHaveBeenCalledTimes(2) + expect(destroyPersistentWebview).toHaveBeenCalledWith('page-1') + expect(destroyPersistentWebview).toHaveBeenCalledWith('page-2') + }) + + it('falls back to the workspace id when no pages exist (legacy sessions)', () => { + destroyWorkspaceWebviews({}, 'legacy-workspace') + + expect(destroyPersistentWebview).toHaveBeenCalledTimes(1) + expect(destroyPersistentWebview).toHaveBeenCalledWith('legacy-workspace') + }) + + it('falls back to the workspace id when the workspace key is present but empty', () => { + destroyWorkspaceWebviews({ 'workspace-1': [] }, 'workspace-1') + + expect(destroyPersistentWebview).toHaveBeenCalledTimes(1) + expect(destroyPersistentWebview).toHaveBeenCalledWith('workspace-1') + }) +}) diff --git a/src/renderer/src/components/browser-pane/browser-webview-cleanup.ts b/src/renderer/src/store/slices/browser-webview-cleanup.ts similarity index 51% rename from src/renderer/src/components/browser-pane/browser-webview-cleanup.ts rename to src/renderer/src/store/slices/browser-webview-cleanup.ts index 5b9a4c45807..d16726bc030 100644 --- a/src/renderer/src/components/browser-pane/browser-webview-cleanup.ts +++ b/src/renderer/src/store/slices/browser-webview-cleanup.ts @@ -1,4 +1,5 @@ import type { BrowserPage, BrowserWorkspace } from '../../../../shared/types' +import { destroyPersistentWebview } from '../../components/browser-pane/webview-registry' export function collectBrowserWebviewIds( browserTabsByWorktree: Record, @@ -20,3 +21,19 @@ export function collectBrowserWebviewIds( } return ids } + +export function destroyWorkspaceWebviews( + browserPagesByWorkspace: Record, + workspaceId: string +): void { + const pages = browserPagesByWorkspace[workspaceId] ?? [] + if (pages.length === 0) { + // Why: legacy sessions persisted before pages existed still key their + // webview by workspace id. Preserve the legacy destroy as a fallback. + destroyPersistentWebview(workspaceId) + return + } + for (const page of pages) { + destroyPersistentWebview(page.id) + } +} diff --git a/src/renderer/src/store/slices/browser.test.ts b/src/renderer/src/store/slices/browser.test.ts index 6a5b9b0d9a7..91013ea18c8 100644 --- a/src/renderer/src/store/slices/browser.test.ts +++ b/src/renderer/src/store/slices/browser.test.ts @@ -1,5 +1,16 @@ -import { describe, expect, it } from 'vitest' +/* eslint-disable max-lines -- + * Why: this slice test keeps closely related browser-slice scenarios + * (create/close, reopen, hydrate, shutdown) in one file so the shared + * webview-registry mock setup stays consistent across behaviors. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('../../components/browser-pane/webview-registry', () => ({ + destroyPersistentWebview: vi.fn() +})) + import { createTestStore, makeTabGroup, makeWorktree, seedStore } from './store-test-helpers' +import { destroyPersistentWebview } from '../../components/browser-pane/webview-registry' describe('browser slice', () => { it('places a new tab in the target group when targetGroupId is provided', () => { @@ -228,3 +239,390 @@ describe('browser slice', () => { expect(store.getState().pendingAddressBarFocusByTabId[created.id]).toBeUndefined() }) }) + +describe('hydrateBrowserSession', () => { + beforeEach(() => { + vi.mocked(destroyPersistentWebview).mockClear() + }) + + // Why: design §3. hydrateBrowserSession drops workspaces whose worktree no + // longer exists. Reducers are pure; destroy calls must bracket the set() + // to mirror closeBrowserTab's contract and keep future callers (that might + // run hydrate after webviews are live) safe without surgery. + it('destroys webviews for dropped workspaces before committing state', () => { + const store = createTestStore() + const survivingWorktreeId = 'repo1::/tmp/wt-1' + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: survivingWorktreeId, repoId: 'repo1', path: '/tmp/wt-1' })] + }, + // Why: destroyWorkspaceWebviews reads pre-hydrate browserPagesByWorkspace + // to resolve page ids. If the store is empty at hydrate time, the helper + // falls back to the legacy workspace-id destroy (design §2 fallback). To + // prove the page-id branch runs when pages exist in-store, seed them + // first. Today's boot-only caller hits the fallback because hydrate is + // the first thing to populate the map — the assertion here covers a + // future re-hydrate after webviews are already live. + browserPagesByWorkspace: { + 'workspace-drop': [ + { + id: 'page-drop-a', + workspaceId: 'workspace-drop', + worktreeId: 'repo1::/tmp/wt-gone', + url: 'about:blank', + title: 'a', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + }, + { + id: 'page-drop-b', + workspaceId: 'workspace-drop', + worktreeId: 'repo1::/tmp/wt-gone', + url: 'about:blank', + title: 'b', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ] + } as never + }) + + store.getState().hydrateBrowserSession({ + browserTabsByWorktree: { + [survivingWorktreeId]: [ + { + id: 'workspace-keep', + worktreeId: survivingWorktreeId, + label: 'keep', + sessionProfileId: null, + pageIds: ['page-keep'], + activePageId: 'page-keep', + url: 'about:blank', + title: 'keep', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ], + 'repo1::/tmp/wt-gone': [ + { + id: 'workspace-drop', + worktreeId: 'repo1::/tmp/wt-gone', + label: 'drop', + sessionProfileId: null, + pageIds: ['page-drop-a', 'page-drop-b'], + activePageId: 'page-drop-a', + url: 'about:blank', + title: 'drop', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ] + }, + browserPagesByWorkspace: { + 'workspace-drop': [ + { + id: 'page-drop-a', + workspaceId: 'workspace-drop', + worktreeId: 'repo1::/tmp/wt-gone', + url: 'about:blank', + title: 'a', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + }, + { + id: 'page-drop-b', + workspaceId: 'workspace-drop', + worktreeId: 'repo1::/tmp/wt-gone', + url: 'about:blank', + title: 'b', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ], + 'workspace-keep': [ + { + id: 'page-keep', + workspaceId: 'workspace-keep', + worktreeId: survivingWorktreeId, + url: 'about:blank', + title: 'keep', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ] + } + } as never) + + expect(destroyPersistentWebview).toHaveBeenCalledWith('page-drop-a') + expect(destroyPersistentWebview).toHaveBeenCalledWith('page-drop-b') + expect(destroyPersistentWebview).not.toHaveBeenCalledWith('page-keep') + expect(store.getState().browserTabsByWorktree['repo1::/tmp/wt-gone']).toBeUndefined() + expect(store.getState().browserPagesByWorkspace['workspace-drop']).toBeUndefined() + expect(store.getState().browserTabsByWorktree[survivingWorktreeId]).toHaveLength(1) + }) +}) + +// Why: the leak this PR fixes lives inside the thunk itself. removeWorktree +// and runSleepWorktree tests stub this thunk, so destroy-call regressions in +// its body would be invisible there. These tests pin the thunk directly. +describe('shutdownWorktreeBrowsers', () => { + beforeEach(() => { + vi.mocked(destroyPersistentWebview).mockClear() + }) + + function seedWorktree(store: ReturnType, worktreeId: string): void { + seedStore(store, { + activeRepoId: 'repo1', + activeWorktreeId: worktreeId, + activeTabType: 'browser', + worktreesByRepo: { + repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1', path: '/tmp/wt-1' })] + }, + groupsByWorktree: { + [worktreeId]: [makeTabGroup({ id: 'group-1', worktreeId, activeTabId: null, tabOrder: [] })] + }, + activeGroupIdByWorktree: { [worktreeId]: 'group-1' }, + browserTabsByWorktree: {}, + unifiedTabsByWorktree: {} + }) + } + + it('destroys every page in every workspace of the target worktree', async () => { + const store = createTestStore() + const worktreeId = 'repo1::/tmp/wt-1' + seedWorktree(store, worktreeId) + + const ws1 = store.getState().createBrowserTab(worktreeId, 'https://example.com/a', { + title: 'A' + }) + const ws2 = store.getState().createBrowserTab(worktreeId, 'https://example.com/b', { + title: 'B' + }) + const ws1PageIds = (store.getState().browserPagesByWorkspace[ws1.id] ?? []).map((p) => p.id) + const ws2PageIds = (store.getState().browserPagesByWorkspace[ws2.id] ?? []).map((p) => p.id) + expect(ws1PageIds).toHaveLength(1) + expect(ws2PageIds).toHaveLength(1) + + await store.getState().shutdownWorktreeBrowsers(worktreeId) + + // Why: this is the regression guard. If the thunk ever stops calling + // destroyWorkspaceWebviews (or reverts to the old workspace-id keying), + // this assertion fails loudly. + for (const pageId of [...ws1PageIds, ...ws2PageIds]) { + expect(destroyPersistentWebview).toHaveBeenCalledWith(pageId) + } + expect(store.getState().browserTabsByWorktree[worktreeId]).toBeUndefined() + expect(store.getState().browserPagesByWorkspace[ws1.id]).toBeUndefined() + expect(store.getState().browserPagesByWorkspace[ws2.id]).toBeUndefined() + }) + + it('destroys every page id of a multi-page workspace', async () => { + const store = createTestStore() + const worktreeId = 'repo1::/tmp/wt-1' + seedWorktree(store, worktreeId) + + const ws = store.getState().createBrowserTab(worktreeId, 'https://example.com/a', { + title: 'A' + }) + store + .getState() + .createBrowserPage(ws.id, 'https://example.com/b', { title: 'B', activate: false }) + store + .getState() + .createBrowserPage(ws.id, 'https://example.com/c', { title: 'C', activate: false }) + const pageIds = (store.getState().browserPagesByWorkspace[ws.id] ?? []).map((p) => p.id) + expect(pageIds).toHaveLength(3) + + await store.getState().shutdownWorktreeBrowsers(worktreeId) + + for (const pageId of pageIds) { + expect(destroyPersistentWebview).toHaveBeenCalledWith(pageId) + } + }) + + it('leaves other worktrees untouched', async () => { + const store = createTestStore() + const targetWorktreeId = 'repo1::/tmp/wt-1' + const otherWorktreeId = 'repo1::/tmp/wt-2' + seedStore(store, { + activeRepoId: 'repo1', + activeWorktreeId: targetWorktreeId, + activeTabType: 'browser', + worktreesByRepo: { + repo1: [ + makeWorktree({ id: targetWorktreeId, repoId: 'repo1', path: '/tmp/wt-1' }), + makeWorktree({ id: otherWorktreeId, repoId: 'repo1', path: '/tmp/wt-2' }) + ] + }, + groupsByWorktree: { + [targetWorktreeId]: [ + makeTabGroup({ + id: 'group-1', + worktreeId: targetWorktreeId, + activeTabId: null, + tabOrder: [] + }) + ], + [otherWorktreeId]: [ + makeTabGroup({ + id: 'group-2', + worktreeId: otherWorktreeId, + activeTabId: null, + tabOrder: [] + }) + ] + }, + activeGroupIdByWorktree: { + [targetWorktreeId]: 'group-1', + [otherWorktreeId]: 'group-2' + }, + browserTabsByWorktree: {}, + unifiedTabsByWorktree: {} + }) + + const targetWs = store.getState().createBrowserTab(targetWorktreeId, 'https://example.com/a', { + title: 'A' + }) + const otherWs = store.getState().createBrowserTab(otherWorktreeId, 'https://example.com/b', { + title: 'B' + }) + const otherPageIds = (store.getState().browserPagesByWorkspace[otherWs.id] ?? []).map( + (p) => p.id + ) + + await store.getState().shutdownWorktreeBrowsers(targetWorktreeId) + + expect(store.getState().browserTabsByWorktree[targetWorktreeId]).toBeUndefined() + expect(store.getState().browserPagesByWorkspace[targetWs.id]).toBeUndefined() + expect(store.getState().browserTabsByWorktree[otherWorktreeId]).toHaveLength(1) + expect(store.getState().browserPagesByWorkspace[otherWs.id]).toHaveLength(1) + for (const pageId of otherPageIds) { + expect(destroyPersistentWebview).not.toHaveBeenCalledWith(pageId) + } + }) + + it('clears activeBrowserTabId and activeTabType when the target is the active worktree', async () => { + const store = createTestStore() + const worktreeId = 'repo1::/tmp/wt-1' + seedWorktree(store, worktreeId) + + const ws = store.getState().createBrowserTab(worktreeId, 'https://example.com/a', { + title: 'A', + activate: true + }) + expect(store.getState().activeBrowserTabId).toBe(ws.id) + + await store.getState().shutdownWorktreeBrowsers(worktreeId) + + expect(store.getState().activeBrowserTabId).toBeNull() + expect(store.getState().activeTabType).toBe('terminal') + expect(store.getState().activeBrowserTabIdByWorktree[worktreeId]).toBeUndefined() + }) + + it('leaves global active browser alone when shutting down a background worktree', async () => { + const store = createTestStore() + const activeWorktreeId = 'repo1::/tmp/wt-1' + const backgroundWorktreeId = 'repo1::/tmp/wt-2' + seedStore(store, { + activeRepoId: 'repo1', + activeWorktreeId, + activeTabType: 'browser', + worktreesByRepo: { + repo1: [ + makeWorktree({ id: activeWorktreeId, repoId: 'repo1', path: '/tmp/wt-1' }), + makeWorktree({ id: backgroundWorktreeId, repoId: 'repo1', path: '/tmp/wt-2' }) + ] + }, + groupsByWorktree: { + [activeWorktreeId]: [ + makeTabGroup({ + id: 'group-1', + worktreeId: activeWorktreeId, + activeTabId: null, + tabOrder: [] + }) + ], + [backgroundWorktreeId]: [ + makeTabGroup({ + id: 'group-2', + worktreeId: backgroundWorktreeId, + activeTabId: null, + tabOrder: [] + }) + ] + }, + activeGroupIdByWorktree: { + [activeWorktreeId]: 'group-1', + [backgroundWorktreeId]: 'group-2' + }, + browserTabsByWorktree: {}, + unifiedTabsByWorktree: {} + }) + + const activeWs = store + .getState() + .createBrowserTab(activeWorktreeId, 'https://example.com/active', { + title: 'Active', + activate: true + }) + store.getState().createBrowserTab(backgroundWorktreeId, 'https://example.com/bg', { + title: 'Background', + // Why: createBrowserTab defaults to activate=true which would replace + // activeBrowserTabId globally. This test needs the original active tab + // to stay active so we can prove shutdown of the background worktree + // doesn't disturb it. + activate: false + }) + expect(store.getState().activeBrowserTabId).toBe(activeWs.id) + + await store.getState().shutdownWorktreeBrowsers(backgroundWorktreeId) + + // Why: §1 shouldResetGlobalBrowser is gated on activeWorktreeId===target. + // Shutting down a background worktree must not disturb what the user is + // currently looking at. + expect(store.getState().activeBrowserTabId).toBe(activeWs.id) + expect(store.getState().activeTabType).toBe('browser') + }) + + it('is a no-op when the worktree has no browser tabs', async () => { + const store = createTestStore() + const worktreeId = 'repo1::/tmp/wt-1' + seedWorktree(store, worktreeId) + // Override: this test needs activeTabType to start as terminal so we can + // prove shutdown doesn't flip it. + store.setState({ activeTabType: 'terminal' }) + + await store.getState().shutdownWorktreeBrowsers(worktreeId) + + expect(destroyPersistentWebview).not.toHaveBeenCalled() + expect(store.getState().activeTabType).toBe('terminal') + }) +}) diff --git a/src/renderer/src/store/slices/browser.ts b/src/renderer/src/store/slices/browser.ts index 60466c4138f..32300158d7d 100644 --- a/src/renderer/src/store/slices/browser.ts +++ b/src/renderer/src/store/slices/browser.ts @@ -13,6 +13,7 @@ import type { } from '../../../../shared/types' import { ORCA_BROWSER_BLANK_URL } from '../../../../shared/constants' import { pickNeighbor } from './tab-group-state' +import { destroyWorkspaceWebviews } from './browser-webview-cleanup' type CreateBrowserTabOptions = { activate?: boolean @@ -64,6 +65,7 @@ export type BrowserSlice = { options?: CreateBrowserTabOptions ) => BrowserWorkspace closeBrowserTab: (tabId: string) => void + shutdownWorktreeBrowsers: (worktreeId: string) => Promise reopenClosedBrowserTab: (worktreeId: string) => BrowserWorkspace | null setActiveBrowserTab: (tabId: string) => void createBrowserPage: ( @@ -520,6 +522,37 @@ export const createBrowserSlice: StateCreator = } }, + shutdownWorktreeBrowsers: async (worktreeId) => { + const workspaces = get().browserTabsByWorktree[worktreeId] ?? [] + // Why: snapshot pre-loop so the post-loop set() can reproduce the original + // `hadBrowserTabs` semantics. Reading `s.browserTabsByWorktree[worktreeId]` + // inside set() would always be empty here because each closeBrowserTab call + // above has already removed the workspace from that array. + const hadBrowserTabs = workspaces.length > 0 + for (const workspace of workspaces) { + destroyWorkspaceWebviews(get().browserPagesByWorkspace, workspace.id) + get().closeBrowserTab(workspace.id) + } + set((s) => { + const nextBrowserTabsByWorktree = { ...s.browserTabsByWorktree } + delete nextBrowserTabsByWorktree[worktreeId] + const nextActiveBrowserTabIdByWorktree = { ...s.activeBrowserTabIdByWorktree } + delete nextActiveBrowserTabIdByWorktree[worktreeId] + // Why: mirror shutdownWorktreeTerminals' `hadBrowserTabs && isActive` + // guard. Only reset the globally-visible active browser surface when the + // worktree being shut down is the one the user is looking at AND it + // actually had browser tabs to tear down. + const shouldResetGlobalBrowser = s.activeWorktreeId === worktreeId && hadBrowserTabs + return { + browserTabsByWorktree: nextBrowserTabsByWorktree, + activeBrowserTabIdByWorktree: nextActiveBrowserTabIdByWorktree, + ...(shouldResetGlobalBrowser + ? { activeBrowserTabId: null, activeTabType: 'terminal' as const } + : {}) + } + }) + }, + reopenClosedBrowserTab: (worktreeId) => { // Why: read and pop atomically inside set() to prevent a TOCTOU race // where two rapid Cmd+Shift+T presses both restore the same entry. @@ -955,8 +988,33 @@ export const createBrowserSlice: StateCreator = }), hydrateBrowserSession: (session) => { + const persistedTabsByWorktree = session.browserTabsByWorktree ?? {} + const currentState = get() + const validWorktreeIdsForCleanup = new Set( + Object.values(currentState.worktreesByRepo) + .flat() + .map((worktree) => worktree.id) + ) + + // Why: mirror closeBrowserTab's contract — reducers are pure, imperative + // side effects bracket them. Compute dropped workspaces first, destroy + // their webviews, then run the state reducer unchanged. hydrate is called + // once at boot (App.tsx) when the webview registry is empty, so this loop + // is a no-op today; it's defense-in-depth for any future caller that + // re-hydrates after webviews are live. + const droppedWorkspaceIds: string[] = [] + for (const [worktreeId, tabs] of Object.entries(persistedTabsByWorktree)) { + if (!validWorktreeIdsForCleanup.has(worktreeId)) { + for (const tab of tabs) { + droppedWorkspaceIds.push(tab.id) + } + } + } + for (const workspaceId of droppedWorkspaceIds) { + destroyWorkspaceWebviews(currentState.browserPagesByWorkspace, workspaceId) + } + set((s) => { - const persistedTabsByWorktree = session.browserTabsByWorktree ?? {} const persistedPagesByWorkspace = session.browserPagesByWorkspace ?? {} const persistedActiveBrowserTabIdByWorktree = session.activeBrowserTabIdByWorktree ?? {} const persistedActiveTabTypeByWorktree = session.activeTabTypeByWorktree ?? {} diff --git a/src/renderer/src/store/slices/store-cascades.test.ts b/src/renderer/src/store/slices/store-cascades.test.ts index 1f4e85af124..2870e65952f 100644 --- a/src/renderer/src/store/slices/store-cascades.test.ts +++ b/src/renderer/src/store/slices/store-cascades.test.ts @@ -978,6 +978,54 @@ describe('setActiveWorktree', () => { expect(s.unreadTerminalTabs[tabB.id]).toBeUndefined() }) + // Why: ownership regression (design §1.3). shutdownWorktreeTerminals used to + // delete browserTabsByWorktree[worktreeId] and reset + // activeBrowserTabId/activeTabType as a side effect — now those mutations + // belong exclusively to shutdownWorktreeBrowsers. If a refactor reintroduces + // the side effect, both thunks will write the same keys and race. + it('leaves browser state untouched when shutting down terminals', async () => { + const store = createTestStore() + const wt = 'repo1::/path/wt1' + + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt1' })] + }, + activeWorktreeId: wt, + activeBrowserTabId: 'workspace-1', + activeTabType: 'browser', + browserTabsByWorktree: { + [wt]: [ + { + id: 'workspace-1', + worktreeId: wt, + label: 'ws1', + sessionProfileId: null, + pageIds: [], + activePageId: null, + url: 'about:blank', + title: 'ws1', + loading: false, + faviconUrl: null, + canGoBack: false, + canGoForward: false, + loadError: null, + createdAt: 1 + } + ] + } as never, + activeBrowserTabIdByWorktree: { [wt]: 'workspace-1' } + }) + + await store.getState().shutdownWorktreeTerminals(wt) + + const s = store.getState() + expect(s.browserTabsByWorktree[wt]).toBeDefined() + expect(s.activeBrowserTabIdByWorktree[wt]).toBe('workspace-1') + expect(s.activeBrowserTabId).toBe('workspace-1') + expect(s.activeTabType).toBe('browser') + }) + it('returns to the landing state when closing the last terminal tab in the active worktree', () => { const store = createTestStore() const wt = 'repo1::/path/wt1' diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 49c24bdbc6e..bab1becec62 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -1027,24 +1027,6 @@ export const createTerminalSlice: StateCreator } } - // Why: browser tabs are factored into getWorktreeStatus — leaving them - // behind after shutdown keeps the sidebar dot green even though all - // terminals are dead. Clearing them here ensures the status indicator - // transitions to inactive. - const nextBrowserTabsByWorktree = { ...s.browserTabsByWorktree } - const hadBrowserTabs = (nextBrowserTabsByWorktree[worktreeId] ?? []).length > 0 - delete nextBrowserTabsByWorktree[worktreeId] - const nextActiveBrowserTabIdByWorktree = { ...s.activeBrowserTabIdByWorktree } - delete nextActiveBrowserTabIdByWorktree[worktreeId] - - // Why: when shutting down the active worktree, the global - // activeBrowserTabId and activeTabType may still point at a browser - // surface that no longer exists. Reset them so the workspace does not - // render a blank browser pane. Background worktrees do not own the - // global surface, so we leave them untouched. - const isActiveWorktree = s.activeWorktreeId === worktreeId - const shouldResetGlobalBrowser = isActiveWorktree && hadBrowserTabs - // Why: intentional shutdown kills the relay PTY. Remove the tab's // lastKnown entry so session-save does not persist a dead session ID // into remoteSessionIdsByTabId, which would cause the next restart @@ -1070,11 +1052,6 @@ export const createTerminalSlice: StateCreator // of full-state selectors. Mirrors the sibling pattern in tabs.ts. ...(nextUnreadTerminalTabs !== s.unreadTerminalTabs ? { unreadTerminalTabs: nextUnreadTerminalTabs } - : {}), - browserTabsByWorktree: nextBrowserTabsByWorktree, - activeBrowserTabIdByWorktree: nextActiveBrowserTabIdByWorktree, - ...(shouldResetGlobalBrowser - ? { activeBrowserTabId: null, activeTabType: 'terminal' as const } : {}) } }) diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index eeec95cd21e..232ce7a108b 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -37,6 +37,7 @@ function createTestStore() { repos: [], openModal: vi.fn(), shutdownWorktreeTerminals: vi.fn().mockResolvedValue(undefined), + shutdownWorktreeBrowsers: vi.fn().mockResolvedValue(undefined), tabsByWorktree: {}, tabBarOrderByWorktree: {}, pendingReconnectTabByWorktree: {}, @@ -58,6 +59,7 @@ function createTestStore() { activeFileIdByWorktree: {}, activeBrowserTabIdByWorktree: {}, browserTabsByWorktree: {}, + recentlyClosedBrowserTabsByWorktree: {}, activeTabTypeByWorktree: {}, activeWorktreeId: null, activeTabId: null, @@ -393,6 +395,31 @@ describe('removeWorktree state cleanup', () => { }) }) + it('clears recentlyClosedBrowserTabsByWorktree for the removed worktree', async () => { + // Why: closeBrowserTab (which shutdownWorktreeBrowsers delegates to) pushes + // each closed workspace into recentlyClosedBrowserTabsByWorktree for the + // Cmd+Shift+T undo path. When the owning worktree is deleted those + // snapshots reference entities that can never be restored — removeWorktree + // must clear the worktree key symmetrically with browserTabsByWorktree + // (design §1.1). + const store = createTestStore() + const wt = makeWorktree({ id: 'repo1::/path/wt1', repoId: 'repo1', path: '/path/wt1' }) + + store.setState({ + worktreesByRepo: { repo1: [wt] }, + recentlyClosedBrowserTabsByWorktree: { + 'repo1::/path/wt1': [{ workspace: { id: 'workspace-1' }, pages: [] }], + 'repo1::/path/wt2': [{ workspace: { id: 'workspace-2' }, pages: [] }] + } + } as unknown as Partial) + + await store.getState().removeWorktree('repo1::/path/wt1') + + expect(store.getState().recentlyClosedBrowserTabsByWorktree).toEqual({ + 'repo1::/path/wt2': [{ workspace: { id: 'workspace-2' }, pages: [] }] + }) + }) + it('skips editorDrafts shallow copy when no files belong to the removed worktree', async () => { const store = createTestStore() const wt = makeWorktree({ id: 'repo1::/path/wt1', repoId: 'repo1', path: '/path/wt1' }) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 8a47ddaa39a..8682c2460d9 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -159,6 +159,15 @@ export const createWorktreeSlice: StateCreator // they are created. We must tear those PTYs down before asking Git to remove // the working tree or Windows and some shells can keep the directory in use // and make delete look broken even though the git state itself is fine. + // + // Why browsers first: `shutdownWorktreeTerminals` used to own the + // `browserTabsByWorktree[worktreeId]` delete as a side effect, which would + // race `shutdownWorktreeBrowsers`' read of the same map. After the §1.3 + // split, terminals no longer touches browser state, but we still call + // browsers first so destroyPersistentWebview sees the workspaces in place + // and the Chromium guests are unregistered before any other teardown work + // can intercept them. + await get().shutdownWorktreeBrowsers(worktreeId) await get().shutdownWorktreeTerminals(worktreeId) await window.api.worktrees.remove({ worktreeId, force, skipArchive }) const tabs = get().tabsByWorktree[worktreeId] ?? [] @@ -189,6 +198,17 @@ export const createWorktreeSlice: StateCreator delete nextActiveFileIdByWorktree[worktreeId] const nextActiveBrowserTabIdByWorktree = { ...s.activeBrowserTabIdByWorktree } delete nextActiveBrowserTabIdByWorktree[worktreeId] + // Why: closeBrowserTab — which shutdownWorktreeBrowsers delegates to — + // pushes a snapshot into recentlyClosedBrowserTabsByWorktree for the + // Cmd+Shift+T undo path. That is correct for UI close, but wrong when + // the owning worktree itself is being deleted: the snapshots reference + // workspaces and pages that can never be restored. Purge the worktree + // key symmetrically with browserTabsByWorktree. Per-workspace page + // snapshots are already cleared upstream by closeBrowserTab. + const nextRecentlyClosedBrowserTabsByWorktree = { + ...s.recentlyClosedBrowserTabsByWorktree + } + delete nextRecentlyClosedBrowserTabsByWorktree[worktreeId] const nextActiveTabTypeByWorktree = { ...s.activeTabTypeByWorktree } delete nextActiveTabTypeByWorktree[worktreeId] const nextActiveTabIdByWorktree = { ...s.activeTabIdByWorktree } @@ -273,6 +293,7 @@ export const createWorktreeSlice: StateCreator activeTabId: s.activeTabId && tabIds.has(s.activeTabId) ? null : s.activeTabId, openFiles: newOpenFiles, browserTabsByWorktree: nextBrowserTabsByWorktree, + recentlyClosedBrowserTabsByWorktree: nextRecentlyClosedBrowserTabsByWorktree, activeFileIdByWorktree: nextActiveFileIdByWorktree, activeBrowserTabIdByWorktree: nextActiveBrowserTabIdByWorktree, activeTabTypeByWorktree: nextActiveTabTypeByWorktree,