From 60456107e65fcf261feb6d9f6ce6adb99bc331f4 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Tue, 8 Sep 2026 19:42:32 -0700 Subject: [PATCH] perf: index closed browser page IDs during focus cleanup (#19449) * perf: index closed browser page IDs during focus cleanup * perf(browser): omit closed page focus keys instead of rebuilding the records --------- Co-authored-by: m4air Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- .../slices/browser-cleanup-close.test.ts | 72 +++++++++++++++++++ .../slices/browser/browser-close-actions.ts | 19 ++--- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/store/slices/browser-cleanup-close.test.ts b/src/renderer/src/store/slices/browser-cleanup-close.test.ts index 0b20bdd3bed..ca9eaa38e6a 100644 --- a/src/renderer/src/store/slices/browser-cleanup-close.test.ts +++ b/src/renderer/src/store/slices/browser-cleanup-close.test.ts @@ -55,6 +55,24 @@ function storeWithOnlyBrowserTab(): { return { store, workspaceId: workspace.id, pageId } } +/** Three browser tabs in a known tab-bar order, so neighbor selection is deterministic. */ +function storeWithThreeBrowserTabs(): { + store: ReturnType + ids: string[] +} { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { repo1: [makeWorktree({ id: WT, repoId: 'repo1', path: '/path/wt1' })] }, + activeWorktreeId: WT, + activeTabType: 'terminal' + }) + const ids = [0, 1, 2].map( + (index) => + store.getState().createBrowserTab(WT, `https://example.test/${index}`, { activate: true }).id + ) + return { store, ids } +} + describe('closeBrowserTab with reason cleanup', () => { beforeEach(() => { vi.clearAllMocks() @@ -151,4 +169,58 @@ describe('closeBrowserTab with reason cleanup', () => { expect(recordFeatureInteraction).toHaveBeenCalledWith('terminal-tabs') }) + it('clears focus for closed pages without rescanning pages for unrelated focus entries', () => { + const { store, workspaceId } = storeWithOnlyBrowserTab() + const original = store.getState().browserPagesByWorkspace[workspaceId][0] + let idReads = 0 + const pages = Array.from({ length: 1000 }, (_, index) => ({ + ...original, + get id() { + idReads++ + return `closed-${index}` + } + })) + const unrelated = Object.fromEntries( + Array.from({ length: 1000 }, (_, i) => [`other-${i}`, true as const]) + ) + store.setState({ + browserPagesByWorkspace: { [workspaceId]: pages }, + pendingAddressBarFocusByPageId: { ...unrelated, 'closed-999': true, [workspaceId]: true }, + pendingAddressBarFocusByTabId: { ...unrelated, 'closed-999': true, [workspaceId]: true } + }) + idReads = 0 + store.getState().closeBrowserTab(workspaceId, { reason: 'cleanup' }) + expect(store.getState().pendingAddressBarFocusByPageId).toEqual({ + ...unrelated, + [workspaceId]: true + }) + expect(store.getState().pendingAddressBarFocusByTabId).toEqual(unrelated) + expect(idReads).toBeLessThan(10_000) + }) + + // Why: the omit path must not hand every pending-focus selector a fresh record on each close. + it('keeps the pending focus records identical when the closed tab had no pending entry', () => { + const { store, workspaceId } = storeWithOnlyBrowserTab() + const pending = { 'other-tab': true as const } + store.setState({ + pendingAddressBarFocusByPageId: pending, + pendingAddressBarFocusByTabId: pending + }) + + store.getState().closeBrowserTab(workspaceId, { reason: 'cleanup' }) + + expect(store.getState().pendingAddressBarFocusByPageId).toBe(pending) + expect(store.getState().pendingAddressBarFocusByTabId).toBe(pending) + }) + + // Why: closing a tab the user is not looking at must never move focus, whatever the + // pending-address-bar bookkeeping does around it. + it('leaves the active browser tab alone when a background tab closes', () => { + const { store, ids } = storeWithThreeBrowserTabs() + store.getState().setActiveBrowserTab(ids[0]!) + + store.getState().closeBrowserTab(ids[2]!) + + expect(store.getState().activeBrowserTabIdByWorktree[WT]).toBe(ids[0]!) + }) }) 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 ad979dae145..33b9c4bb681 100644 --- a/src/renderer/src/store/slices/browser/browser-close-actions.ts +++ b/src/renderer/src/store/slices/browser/browser-close-actions.ts @@ -138,16 +138,17 @@ export function createBrowserCloseActions( } delete nextRecentlyClosedBrowserPagesByWorkspace[tabId] - const nextPendingAddressBarFocusByPageId = Object.fromEntries( - Object.entries(s.pendingAddressBarFocusByPageId).filter( - ([pageId]) => !closedPages.some((page) => page.id === pageId) - ) - ) - const nextPendingAddressBarFocusByTabId = Object.fromEntries( - Object.entries(s.pendingAddressBarFocusByTabId).filter( - ([focusId]) => focusId !== tabId && !closedPages.some((page) => page.id === focusId) - ) + // Why omit rather than rebuild: only the closed ids can match, so this touches the + // closed pages instead of every pending entry and keeps the record identity when none did. + const closedPageIds = closedPages.map((page) => page.id) + const nextPendingAddressBarFocusByPageId = omitRecordKeys( + s.pendingAddressBarFocusByPageId, + closedPageIds ) + const nextPendingAddressBarFocusByTabId = omitRecordKeys(s.pendingAddressBarFocusByTabId, [ + ...closedPageIds, + tabId + ]) return { browserTabsByWorktree: nextBrowserTabsByWorktree,