diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts index 83aab9d2f..1fb1bb12d 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts @@ -298,6 +298,49 @@ describe('renderer breadcrumb IPC routing', () => { ]) }) + // Why: the renderer guard is once per tab-id/verdict, so one stale worktree + // map can emit enough crumbs to evict the pre-crash trail. + it('coalesces duplicate-tab-owner notices across tabs', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 3, resolvedToActiveWorktree: true } + }) + + expect(recordCrashBreadcrumbMock).not.toHaveBeenCalled() + expect(recordCoalescedCrashBreadcrumbMock).toHaveBeenCalledTimes(2) + for (const call of recordCoalescedCrashBreadcrumbMock.mock.calls) { + expect(call[0]).toMatchObject({ + coalesceKey: 'terminal_tab_id_owned_by_multiple_worktrees:true' + }) + } + }) + + // Why flag-scoped: coalescing keeps only the newest payload, and the verdict + // flips under a persisting duplicate, so one would erase the other. + it('keeps a non-converging duplicate-tab-owner notice out of the converging one', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: false } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + + expect( + recordCoalescedCrashBreadcrumbMock.mock.calls.map( + (call) => (call[0] as { coalesceKey: string }).coalesceKey + ) + ).toEqual([ + 'terminal_tab_id_owned_by_multiple_worktrees:false', + 'terminal_tab_id_owned_by_multiple_worktrees:true' + ]) + }) + it('records non-error renderer breadcrumbs without coalescing', () => { emitRendererBreadcrumb({ name: 'renderer_bootstrap_started', data: { dev: true } }) diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index 4cdd4bdbb..f028c7bc3 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -326,11 +326,13 @@ function buildUncapturedCrashReportText( // storm, #8260) can flush the whole fixed-size breadcrumb ring in seconds, // erasing the pre-crash trail. Coalesce repeats into one entry that carries a // suppressed count instead. +const DUPLICATE_TAB_OWNER_BREADCRUMB = 'terminal_tab_id_owned_by_multiple_worktrees' const COALESCED_RENDERER_BREADCRUMB_NAMES = new Set([ 'renderer_error', 'renderer_unhandled_rejection', 'terminal_park_verdict_churn', 'terminal_safe_fit_retry_exhausted', + DUPLICATE_TAB_OWNER_BREADCRUMB, TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB ]) const RENDERER_BREADCRUMB_COALESCE_MS = 30_000 @@ -362,6 +364,11 @@ function rendererBreadcrumbCoalesceKey( if (name === TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB) { return `${name}:${String(data?.kind ?? '')}` } + // Why: a stale map can emit once per tab-id/verdict; key by verdict so + // last-write coalescing cannot erase the other signal while remaining bounded. + if (name === DUPLICATE_TAB_OWNER_BREADCRUMB) { + return `${name}:${String(data?.resolvedToActiveWorktree ?? '')}` + } const primaryMessage = name === 'renderer_error' ? data?.message : data?.reasonMessage const fallbackMessage = name === 'renderer_error' ? data?.errorMessage : undefined const message = diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 561dd4e8c..4189b0e44 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -64,7 +64,7 @@ import { import TabGroupSplitLayout from './tab-group/TabGroupSplitLayout' import AiVaultSessionDropLayer from './tab-group/AiVaultSessionDropLayer' import { shouldAutoCreateInitialTerminal } from './terminal/initial-terminal' -import { resolveRepairedActiveTerminalTabId } from './terminal/active-terminal-repair' +import { useActiveTerminalRepair } from './terminal/use-active-terminal-repair' import { scheduleBackgroundTerminalWorktreeMeasure } from './terminal/background-terminal-worktree-visibility' import { applyBackgroundMountTabRestriction, @@ -390,7 +390,10 @@ function Terminal(): React.JSX.Element | null { }, [foregroundTerminalTabIds]) const tabs = useMemo( - () => (renderedActiveWorktreeId ? (tabsByWorktree[renderedActiveWorktreeId] ?? []) : []), + () => + renderedActiveWorktreeId !== null && Object.hasOwn(tabsByWorktree, renderedActiveWorktreeId) + ? tabsByWorktree[renderedActiveWorktreeId] + : [], [renderedActiveWorktreeId, tabsByWorktree] ) useTerminalProviderSnapshotCapability(workspaceSessionReady && hydrationSucceeded) @@ -749,32 +752,15 @@ function Terminal(): React.JSX.Element | null { ) }, [queueEditorCloseRequests]) - useEffect(() => { - const rememberedTabId = renderedActiveWorktreeId - ? (activeTabIdByWorktree[renderedActiveWorktreeId] ?? null) - : null - // Why: prefer the remembered active tab so a repair on a transient switch render doesn't reset selection to Terminal 1. - const repairedTabId = resolveRepairedActiveTerminalTabId({ - activeTabType, - activeTabId, - rememberedTabId, - tabs - }) - if (!repairedTabId) { - return - } - // Why: run in an effect (Zustand mutation during render trips React's cross-component update warning); keep terminal-only so inactive CLI-created tabs can't steal editor/browser focus. - setActiveTab(repairedTabId) - // Why: `tabs` is the dependency so the repair reacts to tab-order/content changes, not just scalar IDs. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ + // Why: repair after render so Zustand mutation cannot trip React's cross-component update warning. + useActiveTerminalRepair({ activeTabId, activeTabType, setActiveTab, tabs, activeTabIdByWorktree, renderedActiveWorktreeId - ]) + }) // Why: only mount TerminalPanes for visited worktrees, else restoring many saved tabs mass-spawns PTYs. const measurableBackgroundWorktreeTimersRef = useRef(new Map()) diff --git a/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx b/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx new file mode 100644 index 000000000..ab3ff3ac4 --- /dev/null +++ b/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx @@ -0,0 +1,327 @@ +/** @vitest-environment happy-dom */ +import { act, useMemo } from 'react' +import { createRoot } from 'react-dom/client' +import { afterEach, describe, expect, it } from 'vitest' +import { useAppStore } from '@/store' +import type { Tab, TabGroup, TerminalTab } from '../../../../shared/types' +import { useActiveTerminalRepair } from './use-active-terminal-repair' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +// Why: React throws #185 at 51 nested commits; 400 proves divergence, not slowness. +const MAX_PASSES = 400 + +function terminalTab(id: string, worktreeId: string): TerminalTab { + return { id, worktreeId, title: id, createdAt: 0, sortOrder: 0 } as unknown as TerminalTab +} + +function unifiedTerminalTab( + id: string, + entityId: string, + worktreeId: string, + groupId: string +): Tab { + return { + id, + entityId, + worktreeId, + groupId, + contentType: 'terminal', + label: id, + customLabel: null, + color: null, + sortOrder: 0, + createdAt: 0 + } +} + +function tabGroup( + id: string, + worktreeId: string, + activeTabId: string, + tabOrder: string[] +): TabGroup { + return { id, worktreeId, activeTabId, tabOrder, recentTabIds: [activeTabId] } +} + +function RepairEffectHarness(): null { + const activeTabId = useAppStore((s) => s.activeTabId) + const activeTabIdByWorktree = useAppStore((s) => s.activeTabIdByWorktree) + const activeTabType = useAppStore((s) => s.activeTabType) + const tabsByWorktree = useAppStore((s) => s.tabsByWorktree) + const renderedActiveWorktreeId = useAppStore((s) => s.activeWorktreeId) + const setActiveTab = useAppStore((s) => s.setActiveTab) + const tabs = useMemo( + () => + renderedActiveWorktreeId !== null && Object.hasOwn(tabsByWorktree, renderedActiveWorktreeId) + ? tabsByWorktree[renderedActiveWorktreeId] + : [], + [renderedActiveWorktreeId, tabsByWorktree] + ) + + useActiveTerminalRepair({ + activeTabId, + activeTabType, + setActiveTab, + tabs, + activeTabIdByWorktree, + renderedActiveWorktreeId + }) + return null +} + +let cleanup: (() => void) | null = null + +afterEach(() => { + cleanup?.() + cleanup = null +}) + +function measureRepairPasses(): number { + let passes = 0 + const setActiveTab = useAppStore.getState().setActiveTab + useAppStore.setState({ + setActiveTab: (tabId) => { + passes += 1 + if (passes <= MAX_PASSES) { + setActiveTab(tabId) + } + } + }) + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + cleanup = () => { + act(() => root.unmount()) + useAppStore.setState({ setActiveTab }) + container.remove() + } + act(() => { + root.render() + }) + return passes +} + +describe('active-terminal repair effect cannot drive a React #185 update loop', () => { + it('settles when the repaired tab is owned by the active worktree', () => { + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { 'wt-active': [terminalTab('t1', 'wt-active')] }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('settles when another worktree reuses the tab id and is scanned first', () => { + // Why regression: first-match ownership skipped activeTabId while reallocating + // activeTabIdByWorktree, retriggering the repair effect indefinitely. + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { + 'wt-other': [terminalTab('t1', 'wt-other')], + 'wt-active': [terminalTab('t1', 'wt-active')] + }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + // Why: settling by refusing to write would leave the repair permanently + // unsatisfied — quiet, but with activeTabId stuck on a tab that is gone. + expect(useAppStore.getState().activeTabId).toBe('t1') + expect(useAppStore.getState().activeTabIdByWorktree['wt-active']).toBe('t1') + }) + + it('activates the active worktree unified tab when another worktree reuses the entity id', () => { + const otherTab = unifiedTerminalTab('t1', 't1', 'wt-other', 'g-other') + const otherPreviousTab = unifiedTerminalTab( + 'other-previous', + 'other-previous', + 'wt-other', + 'g-other' + ) + const activeTab = unifiedTerminalTab('t1', 't1', 'wt-active', 'g-active') + const previousActiveTab = unifiedTerminalTab('u-previous', 't2', 'wt-active', 'g-active') + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabId: 't2', + activeTabIdByWorktree: { 'wt-active': 't2' }, + tabsByWorktree: { + 'wt-other': [terminalTab('t1', 'wt-other')], + 'wt-active': [terminalTab('t1', 'wt-active'), terminalTab('t2', 'wt-active')] + }, + unifiedTabsByWorktree: { + 'wt-other': [otherTab, otherPreviousTab], + 'wt-active': [activeTab, previousActiveTab] + }, + groupsByWorktree: { + 'wt-other': [ + tabGroup('g-other', 'wt-other', otherPreviousTab.id, [otherTab.id, otherPreviousTab.id]) + ], + 'wt-active': [ + tabGroup('g-active', 'wt-active', previousActiveTab.id, [ + activeTab.id, + previousActiveTab.id + ]) + ] + }, + activeGroupIdByWorktree: { 'wt-other': 'g-other', 'wt-active': 'g-active' } + }) + + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + + expect(useAppStore.getState().groupsByWorktree['wt-active'][0].activeTabId).toBe(activeTab.id) + expect(useAppStore.getState().groupsByWorktree['wt-other'][0].activeTabId).toBe( + otherPreviousTab.id + ) + }) + + it('keeps unified-only terminal activation as a fallback', () => { + const targetTab = unifiedTerminalTab('u-target', 't1', 'wt-active', 'g-active') + const previousTab = unifiedTerminalTab('u-previous', 't2', 'wt-active', 'g-active') + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: {}, + unifiedTabsByWorktree: { 'wt-active': [targetTab, previousTab] }, + groupsByWorktree: { + 'wt-active': [ + tabGroup('g-active', 'wt-active', previousTab.id, [targetTab.id, previousTab.id]) + ] + }, + activeGroupIdByWorktree: { 'wt-active': 'g-active' } + }) + + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + + expect(useAppStore.getState().groupsByWorktree['wt-active'][0].activeTabId).toBe(targetTab.id) + expect(useAppStore.getState().activeTabId).toBeNull() + }) + + it('does not reallocate activeTabIdByWorktree when the tab is already active', () => { + // Why: that map is a dependency of both the repair effect and the parked + // watcher sync, so a redundant activation must not re-run either. + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 't1', + activeTabIdByWorktree: {}, + tabsByWorktree: { 'wt-active': [terminalTab('t1', 'wt-active')] }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + const settled = useAppStore.getState().activeTabIdByWorktree + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + expect(useAppStore.getState().activeTabIdByWorktree).toBe(settled) + }) + + it('keeps bell attribution off a background worktree tab', () => { + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'visible-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { + 'wt-active': [terminalTab('visible-tab', 'wt-active')], + 'wt-background': [terminalTab('bg-tab', 'wt-background')] + }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('bg-tab') + }) + expect(useAppStore.getState().activeTabId).toBe('visible-tab') + expect(useAppStore.getState().activeTabIdByWorktree['wt-background']).toBe('bg-tab') + }) + + it('records activation for a falsy-but-valid worktree id', () => { + useAppStore.setState({ + activeWorktreeId: '', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: { '': [terminalTab('t1', '')] }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + expect(useAppStore.getState().activeTabId).toBe('t1') + expect(useAppStore.getState().activeTabIdByWorktree['']).toBe('t1') + }) + + it('repairs a falsy-but-valid active worktree id through the production hook', () => { + useAppStore.setState({ + activeWorktreeId: '', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: { '': 't1' }, + tabsByWorktree: { '': [terminalTab('t1', '')] }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('does not read inherited unified tabs for a prototype-named owner', () => { + useAppStore.setState({ + activeWorktreeId: 'toString', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: { toString: [terminalTab('t1', 'toString')] }, + unifiedTabsByWorktree: {} + }) + expect(() => useAppStore.getState().setActiveTab('t1')).not.toThrow() + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('activates own unified tabs for a prototype-named owner', () => { + const target = unifiedTerminalTab('t1', 't1', 'toString', 'g-target') + const previous = unifiedTerminalTab('t2', 't2', 'toString', 'g-target') + useAppStore.setState({ + activeWorktreeId: 'toString', + activeTabId: 't2', + activeTabIdByWorktree: { toString: 't2' }, + tabsByWorktree: { + toString: [terminalTab('t1', 'toString'), terminalTab('t2', 'toString')] + }, + unifiedTabsByWorktree: { toString: [target, previous] }, + groupsByWorktree: { + toString: [tabGroup('g-target', 'toString', previous.id, [target.id, previous.id])] + }, + activeGroupIdByWorktree: { toString: 'g-target' } + }) + act(() => useAppStore.getState().setActiveTab('t1')) + expect(useAppStore.getState().groupsByWorktree.toString[0].activeTabId).toBe(target.id) + }) + + it('does not activate a tab with no owner when no worktree is active', () => { + useAppStore.setState({ + activeWorktreeId: null, + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: {}, + unifiedTabsByWorktree: {}, + unreadTerminalTabs: { 'missing-tab': true } + }) + act(() => { + useAppStore.getState().setActiveTab('missing-tab') + }) + expect(useAppStore.getState().activeTabId).toBeNull() + expect(useAppStore.getState().activeTabIdByWorktree).toEqual({}) + expect(useAppStore.getState().unreadTerminalTabs['missing-tab']).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal/use-active-terminal-repair.ts b/src/renderer/src/components/terminal/use-active-terminal-repair.ts new file mode 100644 index 000000000..d495c80c3 --- /dev/null +++ b/src/renderer/src/components/terminal/use-active-terminal-repair.ts @@ -0,0 +1,61 @@ +import { useEffect } from 'react' +import type { TerminalTab, WorkspaceVisibleTabType } from '../../../../shared/types' +import { resolveRepairedActiveTerminalTabId } from './active-terminal-repair' + +type ActiveTerminalRepairInput = { + activeTabType: WorkspaceVisibleTabType + activeTabId: string | null + activeTabIdByWorktree: Record + renderedActiveWorktreeId: string | null + setActiveTab: (tabId: string) => void + tabs: TerminalTab[] +} + +export function repairActiveTerminalTab({ + activeTabType, + activeTabId, + activeTabIdByWorktree, + renderedActiveWorktreeId, + setActiveTab, + tabs +}: ActiveTerminalRepairInput): boolean { + const rememberedTabId = + renderedActiveWorktreeId !== null && + Object.hasOwn(activeTabIdByWorktree, renderedActiveWorktreeId) + ? (activeTabIdByWorktree[renderedActiveWorktreeId] ?? null) + : null + const repairedTabId = resolveRepairedActiveTerminalTabId({ + activeTabType, + activeTabId, + rememberedTabId, + tabs + }) + if (!repairedTabId) { + return false + } + setActiveTab(repairedTabId) + return true +} + +export function useActiveTerminalRepair(input: ActiveTerminalRepairInput): void { + const { + activeTabId, + activeTabIdByWorktree, + activeTabType, + renderedActiveWorktreeId, + setActiveTab, + tabs + } = input + useEffect(() => { + repairActiveTerminalTab(input) + // Why: `tabs` is the dependency so repair reacts to order/content changes, not just scalar ids. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ + activeTabId, + activeTabType, + setActiveTab, + tabs, + activeTabIdByWorktree, + renderedActiveWorktreeId + ]) +} diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts b/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts new file mode 100644 index 000000000..2664475db --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts @@ -0,0 +1,224 @@ +/** + * A direct-SSH snapshot can retain the same tab under old and new worktree IDs + * after a path or repo-ID change. This exercises that hydration path and proves + * active-tab repair converges; it deliberately does not remove the duplicate or + * reproduce React's scheduler-level #185 throw. See PR #11950 for that evidence. + */ +import { describe, expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type { RemoteWorkspaceSnapshot } from '../../../shared/remote-workspace-types' +import type { DirectSshAuthority, SshProviderEpoch } from '../../../shared/ssh-types' +import { createTestStore, makeWorktree } from '../store/slices/store-test-helpers' +import { applyDirectSshRemoteWorkspaceSnapshot } from './remote-workspace-snapshot-apply' +import type { DirectSshSnapshotApplyToken } from './direct-ssh-reconnect-coordinator-types' +import { repairActiveTerminalTab } from '../components/terminal/use-active-terminal-repair' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +const TARGET_ID = 'ssh-target-1' +const OLD_PATH = '/srv/proj/wt' +const NEW_PATH = '/srv/proj/wt-renamed' +const OLD_ID = `repoA::${OLD_PATH}` +const NEW_ID = `repoA::${NEW_PATH}` +// Why a cap and not a while(true): on unfixed code this cycle never terminates. +const MAX_REPAIR_PASSES = 200 + +const authority: DirectSshAuthority = { + targetId: TARGET_ID, + providerEpoch: 'provider-epoch-1' as SshProviderEpoch, + connectionGeneration: 1 +} + +function token(snapshotRevision: number): DirectSshSnapshotApplyToken { + return { + authority, + catalogRevision: 0, + repoFingerprint: 'fp', + authorityRequirement: 'required', + snapshotRevision, + outcome: 'complete' + } +} + +function snapshot( + revision: number, + worktreePath: string, + tabIds: readonly string[], + activeTabId: string | null +): RemoteWorkspaceSnapshot { + return { + namespace: 'workspace', + revision, + updatedAt: revision, + schemaVersion: 1, + session: { + activeWorktreePath: worktreePath, + activeTabId, + tabsByWorktreePath: { + [worktreePath]: tabIds.map((tabId, index) => ({ + id: tabId, + worktreePath, + ptyId: `pty-${tabId}`, + title: `Terminal ${index + 1}`, + customTitle: null, + color: null, + sortOrder: index, + createdAt: index + 1 + })) + }, + terminalLayoutsByTabId: {}, + activeWorktreePathsOnShutdown: [], + activeTabIdByWorktreePath: { [worktreePath]: activeTabId }, + remoteSessionIdsByTabId: Object.fromEntries(tabIds.map((id) => [id, `pty-${id}`])), + lastVisitedAtByWorktreePath: { [worktreePath]: revision }, + defaultTerminalTabsAppliedByWorktreePath: { [worktreePath]: true } + } + } satisfies RemoteWorkspaceSnapshot +} + +type TestStore = ReturnType + +async function applySnapshot(store: TestStore, snap: RemoteWorkspaceSnapshot): Promise { + await applyDirectSshRemoteWorkspaceSnapshot({ + store, + snapshot: snap, + token: token(snap.revision), + arrival: 1, + isArrivalCurrent: () => true, + isPreparationTokenCurrent: () => true, + waitForWorkspaceSessionReady: async () => true, + finalizeHydratedTerminals: () => 0 + }) +} + +function worktreeIdsOwningTab(store: TestStore, tabId: string): string[] { + return Object.entries(store.getState().tabsByWorktree) + .filter(([, tabs]) => tabs.some((tab) => tab.id === tabId)) + .map(([worktreeId]) => worktreeId) +} + +function seedCatalog(store: TestStore, worktreePath: string): void { + store.setState({ + worktreesByRepo: { + repoA: [ + makeWorktree({ + id: `repoA::${worktreePath}`, + repoId: 'repoA', + path: worktreePath, + hostId: `ssh:${TARGET_ID}` + } as never) + ] + } + }) +} + +/** + * One turn of the loop in Terminal.tsx's active-terminal repair effect: + * recompute the repaired id from live state, then activate it. Reports how many + * turns it took to stop and how often `activeTabIdByWorktree` — a declared dep + * of that effect, so a fresh identity re-runs it — was reallocated. + */ +function runRepairCycle(store: TestStore): { + converged: boolean + passes: number + depIdentityChurn: number +} { + let passes = 0 + let depIdentityChurn = 0 + for (; passes < MAX_REPAIR_PASSES; passes += 1) { + const live = store.getState() + const depsBefore = live.activeTabIdByWorktree + const repaired = repairActiveTerminalTab({ + activeTabType: 'terminal', + activeTabId: live.activeTabId, + activeTabIdByWorktree: live.activeTabIdByWorktree, + renderedActiveWorktreeId: live.activeWorktreeId, + setActiveTab: live.setActiveTab, + tabs: live.activeWorktreeId ? (live.tabsByWorktree[live.activeWorktreeId] ?? []) : [] + }) + if (!repaired) { + return { converged: true, passes, depIdentityChurn } + } + if (store.getState().activeTabIdByWorktree !== depsBefore) { + depIdentityChurn += 1 + } + } + return { converged: false, passes, depIdentityChurn } +} + +describe('direct-SSH snapshot apply, tab id owned by two worktrees', () => { + it('converges the active-terminal repair instead of re-running it forever', async () => { + const store = createTestStore() + + store.setState({ + repos: [ + { + id: 'repoA', + path: '/srv/proj', + displayName: 'Proj', + badgeColor: '#000', + addedAt: 0, + connectionId: TARGET_ID + } as never + ], + // Load-bearing, do not drop: the IPC attach is the only thing stubbed, and + // it leaves behind exactly what a real reconnect leaves behind — one + // registered live PTY per tab. Without that the orphan sweep on the next + // worktree visit treats the duplicated tab as dead, cleans it up, and the + // bug evaporates before the repair effect ever sees it. + reconnectPersistedTerminals: (async () => { + const live = store.getState() + const registered: Record = { ...live.ptyIdsByTabId } + for (const tabs of Object.values(live.tabsByWorktree)) { + for (const tab of tabs) { + registered[tab.id] = [`pty-${tab.id}`] + } + } + store.setState({ ptyIdsByTabId: registered }) + }) as never, + markRemoteWorkspaceHydrated: (() => {}) as never, + setRemoteWorkspaceSyncStatus: (() => {}) as never + }) + + seedCatalog(store, OLD_PATH) + await applySnapshot(store, snapshot(1, OLD_PATH, ['tab-1', 'tab-2'], 'tab-1')) + store.getState().setActiveWorktree(OLD_ID) + + // The worktree is renamed on the host; the catalog re-detects it at the new + // path, so the worktree id changes while the tab ids do not. + seedCatalog(store, NEW_PATH) + await applySnapshot(store, snapshot(2, NEW_PATH, ['tab-1', 'tab-2'], 'tab-1')) + store.getState().setActiveWorktree(NEW_ID) + + // The remote deselects; importRemoteWorkspaceSession nulls an activeTabId it + // cannot find among the imported tabs, which is what arms the repair effect. + await applySnapshot(store, snapshot(3, NEW_PATH, ['tab-1', 'tab-2'], null)) + + // Why assert the precondition and not its removal: the fix stops the owner + // resolver being fooled by the duplicate, it does not remove the duplicate. + // Pinned so the test cannot pass vacuously if hydration stops producing one. + expect(worktreeIdsOwningTab(store, 'tab-1')).toEqual([OLD_ID, NEW_ID]) + expect(store.getState().activeTabId).toBeNull() + + const repair = runRepairCycle(store) + + expect(repair.converged).toBe(true) + expect(repair.passes).toBeLessThanOrEqual(store.getState().tabsByWorktree[NEW_ID].length) + expect(store.getState().activeTabId).toBe('tab-1') + const activeGroupId = store.getState().activeGroupIdByWorktree[NEW_ID] + expect( + store.getState().groupsByWorktree[NEW_ID].find((group) => group.id === activeGroupId) + ?.activeTabId + ).toBe('tab-1') + + // The dep identity settles: re-running the effect body after convergence + // reallocates nothing, so the effect does not schedule itself again. + const settled = runRepairCycle(store) + expect(settled.depIdentityChurn).toBe(0) + expect(settled.passes).toBe(0) + }) +}) 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 new file mode 100644 index 000000000..7e8c092a6 --- /dev/null +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts @@ -0,0 +1,125 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TerminalTab } from '../../../../shared/types' + +const recordRendererCrashBreadcrumb = vi.fn() +vi.mock('../../lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (...args: unknown[]) => recordRendererCrashBreadcrumb(...args) +})) + +const { resolveActiveTabOwnerWorktreeId, _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 +} + +beforeEach(() => { + recordRendererCrashBreadcrumb.mockClear() + _resetDuplicateTabOwnerBreadcrumbsForTests() +}) + +describe('resolveActiveTabOwnerWorktreeId', () => { + it('returns the sole owner and stays quiet', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t2', 'wt-b')] }, + 'wt-a', + 't1' + ) + expect(owner).toBe('wt-a') + expect(recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + }) + + it('returns null when no worktree owns the tab', () => { + expect(resolveActiveTabOwnerWorktreeId({ 'wt-a': [tab('t1', 'wt-a')] }, 'wt-a', 'gone')).toBe( + null + ) + }) + + it('prefers the active worktree over an earlier-scanned duplicate', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-other': [tab('t1', 'wt-other')], 'wt-active': [tab('t1', 'wt-active')] }, + 'wt-active', + 't1' + ) + expect(owner).toBe('wt-active') + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: true } + ) + }) + + it('falls back to first match when the active worktree is not an owner', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-x': [tab('t1', 'wt-x')], 'wt-y': [tab('t1', 'wt-y')] }, + 'wt-active', + 't1' + ) + expect(owner).toBe('wt-x') + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: false } + ) + }) + + // Why: a truthiness guard on the active id would drop this back to first-match. + it('prefers a falsy-but-valid active worktree id', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-other': [tab('t1', 'wt-other')], '': [tab('t1', '')] }, + '', + 't1' + ) + expect(owner).toBe('') + }) + + it('breadcrumbs a given tab id once per verdict so it cannot flood the ring', () => { + const maps = { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t1', 'wt-b')] } + for (let i = 0; i < 5; i += 1) { + resolveActiveTabOwnerWorktreeId(maps, 'wt-a', 't1') + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(1) + }) + + // Why: the active worktree changes under a persisting duplicate, and coalescing + // keeps only the newest payload — keyed on the tab id alone, whichever verdict + // a tab reported first would suppress the other for the rest of the session. + it('still reports a non-converging verdict after that tab id reported a converging one', () => { + const maps = { 'wt-other': [tab('t1', 'wt-other')], 'wt-active': [tab('t1', 'wt-active')] } + resolveActiveTabOwnerWorktreeId(maps, 'wt-active', 't1') + resolveActiveTabOwnerWorktreeId(maps, 'wt-third', 't1') + resolveActiveTabOwnerWorktreeId(maps, 'wt-third', 't1') + + expect(recordRendererCrashBreadcrumb.mock.calls).toEqual([ + [ + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: true } + ], + [ + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: false } + ] + ]) + }) + + // Why the count and not just "reports twice": a guard keyed on the active + // worktree id passes the two tests above yet emits once per worktree, which is + // the flood this guard exists to prevent. + it('never exceeds two crumbs for one tab id however the active worktree moves', () => { + const maps = { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t1', 'wt-b')] } + const activeWorktreeIds = ['wt-a', 'wt-b', 'wt-c', '', 'wt-d', 'wt-a'] + for (let i = 0; i < 600; i += 1) { + resolveActiveTabOwnerWorktreeId(maps, activeWorktreeIds[i % activeWorktreeIds.length], 't1') + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(2) + }) + + // Why: the guard set is never pruned and tab ids are minted per created tab. + it('stops recording once the per-session sample cap is reached', () => { + for (let i = 0; i < 400; i += 1) { + const id = `t-${i}` + const maps = { 'wt-a': [tab(id, 'wt-a')], 'wt-b': [tab(id, 'wt-b')] } + resolveActiveTabOwnerWorktreeId(maps, 'wt-a', id) + resolveActiveTabOwnerWorktreeId(maps, 'wt-c', id) + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(256) + }) +}) diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.ts new file mode 100644 index 000000000..270c5d44f --- /dev/null +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.ts @@ -0,0 +1,82 @@ +import type { TerminalTab } from '../../../../shared/types' +import { recordRendererCrashBreadcrumb } from '../../lib/crash-breadcrumb-recorder' + +const reportedDuplicateTabVerdicts = new Set() +// Why capped: this set is never pruned and each tab id adds up to two verdict +// keys. 256 keys cover 128–256 duplicated ids, enough evidence for a bundle. +const MAX_REPORTED_DUPLICATE_TAB_VERDICTS = 256 + +/** Test seam: the duplicate breadcrumb is once-per-tab-id-per-verdict per session. */ +export function _resetDuplicateTabOwnerBreadcrumbsForTests(): void { + reportedDuplicateTabVerdicts.clear() +} + +/** + * Resolve which worktree owns a terminal tab, preferring the active worktree. + * + * Why the preference: a stale map can leave one tab id under two worktrees, and + * attributing it to an arbitrary first match leaves `activeTabId` permanently + * unconvergeable — which strands Terminal's active-terminal repair effect in a + * self-retriggering loop (React #185). + */ +export function resolveActiveTabOwnerWorktreeId( + tabsByWorktree: Record, + activeWorktreeId: string | null, + tabId: string +): string | null { + let firstOwnerId: string | null = null + let ownerCount = 0 + // Why tracked in-loop rather than re-read by key: `tabsByWorktree[activeWorktreeId]` + // resolves inherited members for ids like `toString`, and `?.some` would then throw. + // Why the id and not a boolean: a falsy-but-valid active id ('') would fail a + // truthiness guard below and silently fall back to the first match — the very + // misattribution this function exists to remove. + let activeOwnerId: string | null = null + // Why keys and not entries: entries allocates a pair array per worktree on a path + // that runs per tab activation. Own keys stay safe to index by. + for (const worktreeId of Object.keys(tabsByWorktree)) { + const tabs = tabsByWorktree[worktreeId] + if (!tabs.some((tab) => tab.id === tabId)) { + continue + } + ownerCount += 1 + if (firstOwnerId === null) { + firstOwnerId = worktreeId + } + if (worktreeId === activeWorktreeId) { + activeOwnerId = worktreeId + } + } + + // Why breadcrumb: hydration can retain duplicates after a worktree id change, + // but current field reports predate this signal. + // Reading it: `ownerCount > 1` is the load-bearing datum; the verdict only + // hints at the caller. A sustained repair loop shows up as `true`, since that + // effect picks from the active worktree's own list — but it does not prove the + // caller, and the repair effect can emit `false` too: its closure holds the + // worktree from its render while this runs against live state, so a worktree + // switch landing in between (an earlier-flushed effect, or IPC before the + // passive flush) reattributes the tab. So `false` covers that race as well as + // a deliberate background activation such as jump-to-agent — discard neither. + // Why the verdict is in the guard key: it flips under a persisting duplicate, + // and coalescing keeps only the newest payload, so one would erase the other. + // Still at most two crumbs per tab id. + const resolvedToActiveWorktree = activeOwnerId !== null + const verdictKey = `${tabId}:${resolvedToActiveWorktree}` + if ( + ownerCount > 1 && + !reportedDuplicateTabVerdicts.has(verdictKey) && + reportedDuplicateTabVerdicts.size < MAX_REPORTED_DUPLICATE_TAB_VERDICTS + ) { + reportedDuplicateTabVerdicts.add(verdictKey) + recordRendererCrashBreadcrumb('terminal_tab_id_owned_by_multiple_worktrees', { + ownerCount, + resolvedToActiveWorktree + }) + } + + if (ownerCount > 1 && activeOwnerId !== null) { + return activeOwnerId + } + return firstOwnerId +} diff --git a/src/renderer/src/store/slices/tabs.ts b/src/renderer/src/store/slices/tabs.ts index 9574cd3a6..72e22adc1 100644 --- a/src/renderer/src/store/slices/tabs.ts +++ b/src/renderer/src/store/slices/tabs.ts @@ -120,7 +120,7 @@ export type TabsSlice = { entityId: string, contentType?: TabContentType ) => Tab | null - activateTab: (tabId: string, opts?: { preservePreview?: boolean }) => void + activateTab: (tabId: string, opts?: { preservePreview?: boolean; worktreeId?: string }) => void closeUnifiedTab: ( tabId: string, opts?: { recordInteraction?: boolean; terminalRetirementHandled?: boolean } @@ -826,7 +826,17 @@ export const createTabsSlice: StateCreator = (set, activateTab: (tabId, opts) => { set((state) => { - const found = findTabAndWorktree(state.unifiedTabsByWorktree, tabId) + const scopedWorktreeId = opts?.worktreeId + let found: ReturnType + if (scopedWorktreeId !== undefined) { + const scopedTabs = Object.hasOwn(state.unifiedTabsByWorktree, scopedWorktreeId) + ? state.unifiedTabsByWorktree[scopedWorktreeId] + : [] + const scopedTab = scopedTabs.find((tab) => tab.id === tabId) + found = scopedTab ? { tab: scopedTab, worktreeId: scopedWorktreeId } : null + } else { + found = findTabAndWorktree(state.unifiedTabsByWorktree, tabId) + } if (!found) { return {} } diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 11fbb55c1..cb1b58090 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -38,6 +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 { isSameCodexRestartNoticeAccount } from './codex-restart-notice-account-identity' import { getRepoIdFromWorktreeId, @@ -1903,17 +1904,18 @@ export const createTerminalSlice: StateCreator }, setActiveTab: (tabId) => { + let tabOwnerWorktreeId: string | null = null set((s) => { // Why: focusing a terminal tab clears its bell, but only for the active worktree — clearing a not-yet-visible background tab (worktree activation / jump-to-agent) would swallow the signal. - let tabOwnerWorktreeId: string | null = null - for (const [wId, tabs] of Object.entries(s.tabsByWorktree)) { - if (tabs.some((t) => t.id === tabId)) { - tabOwnerWorktreeId = wId - break - } - } + tabOwnerWorktreeId = resolveActiveTabOwnerWorktreeId( + s.tabsByWorktree, + s.activeWorktreeId, + tabId + ) + const isActiveWorktreeTab = + tabOwnerWorktreeId !== null && tabOwnerWorktreeId === s.activeWorktreeId const nextUnreadTerminalTabs = - tabOwnerWorktreeId === s.activeWorktreeId && s.unreadTerminalTabs[tabId] + isActiveWorktreeTab && s.unreadTerminalTabs[tabId] ? (() => { const copy = { ...s.unreadTerminalTabs } delete copy[tabId] @@ -1921,20 +1923,37 @@ export const createTerminalSlice: StateCreator })() : s.unreadTerminalTabs // Why: only pin global activeTabId to active-worktree tabs — markTerminalTabUnread treats it as "the visible tab" and would swallow BELs on a background tab (e.g. jump-to-agent). - const isActiveWorktreeTab = tabOwnerWorktreeId === s.activeWorktreeId return { activeTabId: isActiveWorktreeTab ? tabId : s.activeTabId, - activeTabIdByWorktree: tabOwnerWorktreeId - ? { ...s.activeTabIdByWorktree, [tabOwnerWorktreeId]: tabId } - : s.activeTabIdByWorktree, + // Why: a redundant activation must not reallocate this map — Terminal's + // active-terminal repair effect depends on it, so a re-activation that + // can't converge activeTabId (tab id reused by an earlier-scanned + // worktree) would otherwise re-trigger itself into React error #185. + activeTabIdByWorktree: + tabOwnerWorktreeId !== null && s.activeTabIdByWorktree[tabOwnerWorktreeId] !== tabId + ? { ...s.activeTabIdByWorktree, [tabOwnerWorktreeId]: tabId } + : s.activeTabIdByWorktree, unreadTerminalTabs: nextUnreadTerminalTabs } }) - const item = Object.values(get().unifiedTabsByWorktree) - .flat() - .find((entry) => entry.contentType === 'terminal' && entry.entityId === tabId) + const state = get() + const ownerUnifiedTabs = + tabOwnerWorktreeId !== null && Object.hasOwn(state.unifiedTabsByWorktree, tabOwnerWorktreeId) + ? state.unifiedTabsByWorktree[tabOwnerWorktreeId] + : [] + // Why: a duplicated entity id must activate the same owner chosen above. + const item = + ownerUnifiedTabs.find( + (entry) => entry.contentType === 'terminal' && entry.entityId === tabId + ) ?? + Object.values(state.unifiedTabsByWorktree) + .flat() + .find((entry) => entry.contentType === 'terminal' && entry.entityId === tabId) if (item) { - get().activateTab(item.id) + state.activateTab( + item.id, + tabOwnerWorktreeId !== null ? { worktreeId: tabOwnerWorktreeId } : undefined + ) } },