From e2f9481fce1a63a2a54806643a3e05fbd9250ef5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 1 Aug 2026 06:53:36 -0700 Subject: [PATCH] fix(terminal): keep a duplicated tab id from re-sorting the active worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setActiveTab now prefers the active worktree when a tab id is held by more than one, but terminals.ts has a second, older owner resolver: getTerminalTabOwnerWorktreeId, a memoized map built last-writer-wins. Two of its callers — setRuntimePaneTitle and clearRuntimePaneTitle — use the result for the same "is this pane in the active worktree" gate, so under a duplicate the two resolvers disagree: the cache names whichever worktree it saw last, which can be a background one for a pane the user is looking at. The gate then fails open and every classified OSC title frame bumps sortEpoch, reinstating the click-driven sidebar re-sort #209 removed — 20 title frames measured 20 bumps, each one a store write that re-renders every sortEpoch subscriber. isTabInActiveWorktree answers from the active worktree's own tab list instead of a tie-break. It stays behind the cheap id equality so the common non-duplicated path is unchanged, and it is a hasOwn lookup plus one scan of that worktree's tabs rather than resolveActiveTabOwnerWorktreeId, whose full scan would run per title frame and whose breadcrumb would fold a second caller into one verdict. Leaves updateTabTitle and clearTabLaunchAgent on the cache: they pick which copy of a duplicated tab to mutate, where no answer is defensible until the duplication itself is fixed. --- .../slices/active-tab-owner-worktree.test.ts | 37 +++++++++- .../store/slices/active-tab-owner-worktree.ts | 26 +++++++ .../runtime-pane-title-sort-epoch.test.ts | 70 +++++++++++++++++++ src/renderer/src/store/slices/terminals.ts | 17 ++++- 4 files changed, 145 insertions(+), 5 deletions(-) diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts index 7e8c092a6c7..fa1f60e2f65 100644 --- a/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts @@ -6,8 +6,11 @@ vi.mock('../../lib/crash-breadcrumb-recorder', () => ({ recordRendererCrashBreadcrumb: (...args: unknown[]) => recordRendererCrashBreadcrumb(...args) })) -const { resolveActiveTabOwnerWorktreeId, _resetDuplicateTabOwnerBreadcrumbsForTests } = - await import('./active-tab-owner-worktree') +const { + resolveActiveTabOwnerWorktreeId, + isTabInActiveWorktree, + _resetDuplicateTabOwnerBreadcrumbsForTests +} = await import('./active-tab-owner-worktree') function tab(id: string, worktreeId: string): TerminalTab { return { id, worktreeId, title: id, createdAt: 0, sortOrder: 0 } as unknown as TerminalTab @@ -123,3 +126,33 @@ describe('resolveActiveTabOwnerWorktreeId', () => { expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(256) }) }) + +describe('isTabInActiveWorktree', () => { + it('sees a duplicated tab id in the active worktree whichever key is scanned last', () => { + const maps = { 'wt-active': [tab('t1', 'wt-active')], 'wt-other': [tab('t1', 'wt-other')] } + expect(isTabInActiveWorktree(maps, 'wt-active', 't1')).toBe(true) + expect(isTabInActiveWorktree(maps, 'wt-other', 't1')).toBe(true) + expect(isTabInActiveWorktree(maps, 'wt-third', 't1')).toBe(false) + }) + + it('is false with no active worktree', () => { + expect(isTabInActiveWorktree({ 'wt-a': [tab('t1', 'wt-a')] }, null, 't1')).toBe(false) + }) + + // Why: a plain index would resolve the inherited member and `.some` would throw. + it('does not reach an inherited member for a prototype-named active worktree id', () => { + expect(isTabInActiveWorktree({ 'wt-a': [tab('t1', 'wt-a')] }, 'toString', 't1')).toBe(false) + }) + + it('reads an own key that shadows a prototype name', () => { + expect(isTabInActiveWorktree({ toString: [tab('t1', 'toString')] }, 'toString', 't1')).toBe( + true + ) + }) + + // Why: it must not silently answer "yes" for a falsy-but-present worktree id. + it('reads an empty-string active worktree id', () => { + expect(isTabInActiveWorktree({ '': [tab('t1', '')] }, '', 't1')).toBe(true) + expect(isTabInActiveWorktree({ '': [tab('t2', '')] }, '', 't1')).toBe(false) + }) +}) diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.ts index 9e7843bc9b3..9057e3203ac 100644 --- a/src/renderer/src/store/slices/active-tab-owner-worktree.ts +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.ts @@ -81,3 +81,29 @@ export function resolveActiveTabOwnerWorktreeId( } return firstOwnerId } + +/** + * Whether the active worktree's own tab list holds this tab. + * + * Why this and not a cached owner id: the tab-owner cache in terminals.ts is + * last-writer-wins, so a duplicated tab id names whichever worktree it saw last + * — which can be a background one for a pane that really is in the active + * worktree. Callers that gate on "is this pane visible" need the active + * worktree's own answer, not a tie-break. + * + * Why not resolveActiveTabOwnerWorktreeId: its full scan runs per OSC title + * frame, and its breadcrumb would report a second caller under one verdict. + */ +export function isTabInActiveWorktree( + tabsByWorktree: Record, + activeWorktreeId: string | null, + tabId: string +): boolean { + // Why hasOwnProperty and not a plain index: ids like `toString` resolve an + // inherited member, and `.some` would then throw. Why not Object.keys: this + // runs per classified title frame, so it must not allocate a key array. + if (activeWorktreeId === null || !Object.hasOwn(tabsByWorktree, activeWorktreeId)) { + return false + } + return tabsByWorktree[activeWorktreeId].some((tab) => tab.id === tabId) +} diff --git a/src/renderer/src/store/slices/runtime-pane-title-sort-epoch.test.ts b/src/renderer/src/store/slices/runtime-pane-title-sort-epoch.test.ts index b6c10e014ed..0ce480558aa 100644 --- a/src/renderer/src/store/slices/runtime-pane-title-sort-epoch.test.ts +++ b/src/renderer/src/store/slices/runtime-pane-title-sort-epoch.test.ts @@ -343,4 +343,74 @@ describe('runtimePaneTitle → sortEpoch', () => { store.getState().clearRuntimePaneTitle('tab-1', 1) expect(store.getState().sortEpoch).toBe(baseline) }) + + // Why: the tab-owner cache is last-writer-wins, so a tab id held under two + // worktrees names a background one for a pane that is in the active worktree + // — and every classified title frame then re-ranks the sidebar. + it('does not bump sortEpoch when a duplicated tab id is also in the active worktree', () => { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo1: [ + makeWorktree({ id: 'wt-a', repoId: 'repo1', path: '/path/wt-a' }), + makeWorktree({ id: 'wt-b', repoId: 'repo1', path: '/path/wt-b' }) + ] + }, + tabsByWorktree: { + 'wt-a': [makeTab({ id: 'tab-1', worktreeId: 'wt-a' })], + 'wt-b': [makeTab({ id: 'tab-1', worktreeId: 'wt-b' })] + }, + activeWorktreeId: 'wt-a' + }) + const baseline = store.getState().sortEpoch + store.getState().setRuntimePaneTitle('tab-1', 1, '⠋ Claude') + store.getState().setRuntimePaneTitle('tab-1', 1, '✋ Gemini CLI') + expect(store.getState().sortEpoch).toBe(baseline) + }) + + it('does not bump sortEpoch clearing a duplicated tab id held by the active worktree', () => { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo1: [ + makeWorktree({ id: 'wt-a', repoId: 'repo1', path: '/path/wt-a' }), + makeWorktree({ id: 'wt-b', repoId: 'repo1', path: '/path/wt-b' }) + ] + }, + tabsByWorktree: { + 'wt-a': [makeTab({ id: 'tab-1', worktreeId: 'wt-a' })], + 'wt-b': [makeTab({ id: 'tab-1', worktreeId: 'wt-b' })] + }, + activeWorktreeId: 'wt-b' + }) + store.getState().setRuntimePaneTitle('tab-1', 1, '✋ Gemini CLI') + store.setState({ activeWorktreeId: 'wt-a' }) + const baseline = store.getState().sortEpoch + store.getState().clearRuntimePaneTitle('tab-1', 1) + expect(store.getState().sortEpoch).toBe(baseline) + }) + + // Why: the duplicate must not flip the gate the other way either — a pane in + // no active worktree still has to re-rank. + it('still bumps sortEpoch when neither owner of a duplicated tab id is active', () => { + const store = createTestStore() + seedStore(store, { + worktreesByRepo: { + repo1: [ + makeWorktree({ id: 'wt-a', repoId: 'repo1', path: '/path/wt-a' }), + makeWorktree({ id: 'wt-b', repoId: 'repo1', path: '/path/wt-b' }), + makeWorktree({ id: 'wt-c', repoId: 'repo1', path: '/path/wt-c' }) + ] + }, + tabsByWorktree: { + 'wt-a': [makeTab({ id: 'tab-1', worktreeId: 'wt-a' })], + 'wt-b': [makeTab({ id: 'tab-1', worktreeId: 'wt-b' })], + 'wt-c': [makeTab({ id: 'tab-2', worktreeId: 'wt-c' })] + }, + activeWorktreeId: 'wt-c' + }) + const baseline = store.getState().sortEpoch + store.getState().setRuntimePaneTitle('tab-1', 1, '⠋ Claude') + expect(store.getState().sortEpoch).toBeGreaterThan(baseline) + }) }) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 89c0ee3e5fc..a3e6161419b 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -38,7 +38,7 @@ import { } from '../../../../shared/stable-pane-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../../../shared/terminal-tab-id' import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' -import { resolveActiveTabOwnerWorktreeId } from './active-tab-owner-worktree' +import { isTabInActiveWorktree, resolveActiveTabOwnerWorktreeId } from './active-tab-owner-worktree' import { isSameCodexRestartNoticeAccount } from './codex-restart-notice-account-identity' import { getRepoIdFromWorktreeId, @@ -2140,7 +2140,13 @@ export const createTerminalSlice: StateCreator const ownerWorktreeId = classificationChanged ? getTerminalTabOwnerWorktreeId(s.tabsByWorktree, tabId) : null - const isActive = ownerWorktreeId !== null && ownerWorktreeId === s.activeWorktreeId + // Why the second test: the owner cache is last-writer-wins, so a tab id + // held under two worktrees can name a background one for a pane that is + // in the active worktree — reinstating the click-driven re-sort #209 removed. + const isActive = + ownerWorktreeId !== null && + (ownerWorktreeId === s.activeWorktreeId || + isTabInActiveWorktree(s.tabsByWorktree, s.activeWorktreeId, tabId)) const shouldBump = classificationChanged && ownerWorktreeId !== null && !isActive return { runtimePaneTitlesByTabId: { @@ -2175,7 +2181,12 @@ export const createTerminalSlice: StateCreator const ownerWorktreeId = hadClassification ? getTerminalTabOwnerWorktreeId(s.tabsByWorktree, tabId) : null - const isActive = ownerWorktreeId !== null && ownerWorktreeId === s.activeWorktreeId + // Why the second test: see setRuntimePaneTitle — a duplicated tab id makes + // the last-writer-wins owner cache name a background worktree. + const isActive = + ownerWorktreeId !== null && + (ownerWorktreeId === s.activeWorktreeId || + isTabInActiveWorktree(s.tabsByWorktree, s.activeWorktreeId, tabId)) const shouldBump = hadClassification && ownerWorktreeId !== null && !isActive return { runtimePaneTitlesByTabId: next,