From 6798c3c701a409a7059df9ef978189bedfc201a7 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sat, 17 Oct 2026 01:22:58 -0400 Subject: [PATCH] fix(browser): keep slept workspace browser tabs off the reopen-closed list Sleeping a workspace closed its browser tabs as if the user had closed them, so Reopen Closed Tab could bring back a tab that was only slept. Sleep now closes them with a 'sleep' reason that skips the reopen stack while still releasing host pages, doc grants and webviews as before. --- .../sidebar/sleep-worktree-flow.test.ts | 13 ++- .../components/sidebar/sleep-worktree-flow.ts | 4 +- .../store/slices/browser-sleep-close.test.ts | 96 +++++++++++++++++++ .../slices/browser/browser-close-actions.ts | 16 ++-- .../slices/browser/browser-slice-contract.ts | 12 ++- 5 files changed, 127 insertions(+), 14 deletions(-) create mode 100644 src/renderer/src/store/slices/browser-sleep-close.test.ts diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts index 34497baf390..4dfc406fccd 100644 --- a/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts @@ -88,7 +88,8 @@ describe('runSleepWorktree', () => { // 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') + // Why the reason: a slept tab must not land on the reopen-closed list. + expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-1', { reason: 'sleep' }) expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-1', { keepIdentifiers: true }) @@ -300,7 +301,7 @@ describe('runSleepWorktree', () => { expect(mocks.state.shutdownWorktreeTerminals).not.toHaveBeenCalledWith('wt-1', { keepIdentifiers: true }) - expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-2') + expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-2', { reason: 'sleep' }) expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-2', { keepIdentifiers: true }) @@ -333,11 +334,15 @@ describe('runSleepWorktree', () => { expect(mocks.state.setActiveWorktree).toHaveBeenCalledTimes(1) expect(mocks.state.setActiveWorktree).toHaveBeenCalledWith(null) - expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(1, 'wt-1') + expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(1, 'wt-1', { + reason: 'sleep' + }) expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenNthCalledWith(1, 'wt-1', { keepIdentifiers: true }) - expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(2, 'wt-2') + expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(2, 'wt-2', { + reason: 'sleep' + }) expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenNthCalledWith(2, 'wt-2', { keepIdentifiers: true }) diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts index 24fb2a707b6..2e2d82a48b0 100644 --- a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts @@ -197,7 +197,9 @@ export async function sleepWorktreesReportingFailures( // 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 withWorktreeSleepTeardown(worktreeId, () => shutdownWorktreeBrowsers(worktreeId)) + await withWorktreeSleepTeardown(worktreeId, () => + shutdownWorktreeBrowsers(worktreeId, { reason: 'sleep' }) + ) } catch (err) { console.error('[sleep-worktree] browser shutdown failed', { worktreeId, error: err }) failedWorktreeIds.add(worktreeId) diff --git a/src/renderer/src/store/slices/browser-sleep-close.test.ts b/src/renderer/src/store/slices/browser-sleep-close.test.ts new file mode 100644 index 00000000000..49e6230d13a --- /dev/null +++ b/src/renderer/src/store/slices/browser-sleep-close.test.ts @@ -0,0 +1,96 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type * as RuntimeRpcClientModule from '@/runtime/runtime-rpc-client' +import { createTestStore, makeWorktree, seedStore } from './store-test-helpers' +import { createStoreCascadesMockApi } from './store-cascades-test-harness' + +const mockCallRuntimeRpc = vi.hoisted(() => vi.fn()) + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } +})) + +vi.mock('@/components/terminal-pane/pty-dispatcher', () => ({ + restorePtyDataHandlersAfterFailedShutdown: vi.fn(), + unregisterPtyDataHandlers: vi.fn(() => []) +})) + +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +vi.mock('@/runtime/runtime-rpc-client', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, callRuntimeRpc: mockCallRuntimeRpc } +}) + +createStoreCascadesMockApi() + +const WT = 'repo1::/path/wt1' + +function storeWithRemoteBrowserTab(): { + store: ReturnType + workspaceId: string +} { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { repo1: [makeWorktree({ id: WT, repoId: 'repo1', path: '/path/wt1' })] }, + activeWorktreeId: WT, + activeTabType: 'terminal' + }) + const workspace = store.getState().createBrowserTab(WT, 'about:blank', { + activate: true, + browserPageId: 'remote-page-1', + browserRuntimeEnvironmentId: 'env-1' + }) + const pageId = store.getState().browserPagesByWorkspace[workspace.id]?.[0]?.id ?? '' + store.getState().setRemoteBrowserPageHandle(pageId, { + environmentId: 'env-1', + remotePageId: 'remote-page-1', + staged: true + }) + return { store, workspaceId: workspace.id } +} + +describe('browser tabs closed by workspace sleep', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCallRuntimeRpc.mockResolvedValue({}) + }) + + // Why: reopen-closed is the user's undo history; a slept tab was never closed by the user. + it('does not enter the reopen stack', async () => { + const { store } = storeWithRemoteBrowserTab() + + await store.getState().shutdownWorktreeBrowsers(WT, { reason: 'sleep' }) + + const s = store.getState() + expect(s.browserTabsByWorktree[WT] ?? []).toEqual([]) + expect(s.recentlyClosedBrowserTabsByWorktree[WT] ?? []).toEqual([]) + expect(s.recentlyClosedTabKindsByWorktree[WT] ?? []).toEqual([]) + expect(s.reopenClosedBrowserTab(WT)).toBeNull() + }) + + // Why: unlike an unwound create, a slept tab's host page exists and must still be released. + it('still closes the host page', async () => { + const { store } = storeWithRemoteBrowserTab() + + await store.getState().shutdownWorktreeBrowsers(WT, { reason: 'sleep' }) + + expect(mockCallRuntimeRpc).toHaveBeenCalledWith( + { kind: 'environment', environmentId: 'env-1' }, + 'browser.tabClose', + expect.objectContaining({ page: 'remote-page-1' }), + expect.anything() + ) + }) + + it('leaves the reopen stack for a user close', () => { + const { store, workspaceId } = storeWithRemoteBrowserTab() + + store.getState().closeBrowserTab(workspaceId) + + expect(store.getState().recentlyClosedBrowserTabsByWorktree[WT]).toHaveLength(1) + }) +}) diff --git a/src/renderer/src/store/slices/browser/browser-close-actions.ts b/src/renderer/src/store/slices/browser/browser-close-actions.ts index 016a217f2ac..06c28ab1f45 100644 --- a/src/renderer/src/store/slices/browser/browser-close-actions.ts +++ b/src/renderer/src/store/slices/browser/browser-close-actions.ts @@ -25,6 +25,8 @@ export function createBrowserCloseActions( // Why: a cleanup close unwinds a tab that never finished being created — it owns no host // page to close and must not enter the reopen stack as if the user had closed something. const isCleanup = options?.reason === 'cleanup' + // Why: a slept tab is released, not closed by the user, so it must not be reopenable. + const recordsReopen = !isCleanup && options?.reason !== 'sleep' let remotePagesToClose: { worktreeId: string; handle: RemoteBrowserPageHandle }[] = [] // Why collected rather than released inside the reducer: revoking is main-process work, and a // grant is the only authority the preview scheme honors — a closed document must stop being @@ -114,7 +116,7 @@ export function createBrowserCloseActions( } const nextRecentlyClosedBrowserTabsByWorktree = { ...s.recentlyClosedBrowserTabsByWorktree } - if (!isCleanup) { + if (recordsReopen) { const existingSnapshots = nextRecentlyClosedBrowserTabsByWorktree[owningWorktreeId] ?? [] const position = getRecentlyClosedTabPosition(s, owningWorktreeId, tabId) nextRecentlyClosedBrowserTabsByWorktree[owningWorktreeId] = [ @@ -126,13 +128,13 @@ export function createBrowserCloseActions( ...existingSnapshots.filter((entry) => entry.workspace.id !== closedWorkspace.id) ].slice(0, 10) } - const nextRecentlyClosedTabKindsByWorktree = isCleanup - ? s.recentlyClosedTabKindsByWorktree - : pushRecentlyClosedTabKind( + const nextRecentlyClosedTabKindsByWorktree = recordsReopen + ? pushRecentlyClosedTabKind( s.recentlyClosedTabKindsByWorktree, owningWorktreeId, 'browser' ) + : s.recentlyClosedTabKindsByWorktree const nextRecentlyClosedBrowserPagesByWorkspace = { ...s.recentlyClosedBrowserPagesByWorkspace @@ -196,7 +198,7 @@ export function createBrowserCloseActions( get(), workspaceItem.worktreeId, tabId, - isCleanup ? 'cleanup' : 'user' + recordsReopen ? 'user' : 'cleanup' ) } if (workspaceItem) { @@ -237,13 +239,13 @@ export function createBrowserCloseActions( } }, - shutdownWorktreeBrowsers: async (worktreeId) => { + shutdownWorktreeBrowsers: async (worktreeId, options) => { const workspaces = get().browserTabsByWorktree[worktreeId] ?? [] // Why: snapshot before the loop — closeBrowserTab empties the array, so set() below couldn't recompute hadBrowserTabs. const hadBrowserTabs = workspaces.length > 0 for (const workspace of workspaces) { const browserPagesByWorkspace = get().browserPagesByWorkspace - get().closeBrowserTab(workspace.id) + get().closeBrowserTab(workspace.id, options) destroyWorkspaceWebviews(browserPagesByWorkspace, workspace.id) } set((s) => { diff --git a/src/renderer/src/store/slices/browser/browser-slice-contract.ts b/src/renderer/src/store/slices/browser/browser-slice-contract.ts index 40175ef8a32..6f9fdea10d8 100644 --- a/src/renderer/src/store/slices/browser/browser-slice-contract.ts +++ b/src/renderer/src/store/slices/browser/browser-slice-contract.ts @@ -32,6 +32,9 @@ import type { ExecutionHostId } from '../../../../../shared/execution-host' import type { TabSplitTarget } from '../tabs/tabs-slice-contract' import type { RuntimeBrowserPlacement } from '../../../../../shared/runtime-browser-placement' +/** A close the user did not ask for. */ +export type BrowserTabCloseReason = 'cleanup' | 'sleep' + export type CreateBrowserTabOptions = { activate?: boolean browserPageId?: string @@ -148,8 +151,13 @@ export type BrowserSlice = { url: string, profileId: string | null ) => Promise - closeBrowserTab: (tabId: string, options?: { reason?: 'cleanup' }) => void - shutdownWorktreeBrowsers: (worktreeId: string) => Promise + /** `cleanup` unwinds an unfinished create; `sleep` releases a slept workspace's tab. Neither + * enters the reopen-closed list. */ + closeBrowserTab: (tabId: string, options?: { reason?: BrowserTabCloseReason }) => void + shutdownWorktreeBrowsers: ( + worktreeId: string, + options?: { reason?: BrowserTabCloseReason } + ) => Promise reopenClosedBrowserTab: (worktreeId: string) => BrowserWorkspace | null /** Activation uses the tab's workspace unless a target is supplied; global selection * moves only when that workspace is active. */