mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
fix(terminal): keep a duplicated tab id from re-sorting the active worktree
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.
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, TerminalTab[]>,
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<AppState, [], [], TerminalSlice>
|
||||
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<AppState, [], [], TerminalSlice>
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user