diff --git a/src/renderer/src/runtime/web-runtime-session.ts b/src/renderer/src/runtime/web-runtime-session.ts index f7c65b84c29..64f8c7a990f 100644 --- a/src/renderer/src/runtime/web-runtime-session.ts +++ b/src/renderer/src/runtime/web-runtime-session.ts @@ -527,11 +527,15 @@ export async function createWebRuntimeSessionBrowserTab(args: { if ( state.activeBrowserTabIdByWorktree === previousState.activeBrowserTabIdByWorktree && state.activeFileIdByWorktree === previousState.activeFileIdByWorktree && + // Why: resolveWebSessionVisibleTabId now reads group state, so a group-only focus move + // must re-evaluate it. + state.activeGroupIdByWorktree === previousState.activeGroupIdByWorktree && state.activeTabIdByWorktree === previousState.activeTabIdByWorktree && state.activeTabType === previousState.activeTabType && state.activeTabTypeByWorktree === previousState.activeTabTypeByWorktree && state.activeWorktreeId === previousState.activeWorktreeId && state.activeWorkspaceExecutionHostId === previousState.activeWorkspaceExecutionHostId && + state.groupsByWorktree === previousState.groupsByWorktree && state.unifiedTabsByWorktree === previousState.unifiedTabsByWorktree ) { return diff --git a/src/renderer/src/runtime/web-session-focus-intent-visible-tab.test.ts b/src/renderer/src/runtime/web-session-focus-intent-visible-tab.test.ts new file mode 100644 index 00000000000..09ca8760d7e --- /dev/null +++ b/src/renderer/src/runtime/web-session-focus-intent-visible-tab.test.ts @@ -0,0 +1,248 @@ +import { describe, expect, it } from 'vitest' +import { resolveWebSessionVisibleTabId } from './web-session-focus-intent' +import { + toVisibleTabType, + type Tab, + type TabContentType, + type TabGroup +} from '../../../shared/tab-types' +import { makeState, WT } from './web-session-tabs-sync-test-harness' + +const GROUP_A = 'group-a' +const GROUP_B = 'group-b' + +function tab(overrides: Partial & Pick): Tab { + return { + groupId: GROUP_A, + worktreeId: WT, + label: overrides.id, + sortOrder: 0, + ...overrides + } as Tab +} + +function group(overrides: Partial & Pick): TabGroup { + return { + worktreeId: WT, + activeTabId: null, + tabOrder: [], + ...overrides + } +} + +describe('resolveWebSessionVisibleTabId — grouped state is authoritative', () => { + // Why: the bug. Diff tabs carry contentType 'diff' while the coarse address says 'editor'. + it.each(['diff', 'conflict-review', 'check-details'])( + 'resolves a focused %s tab instead of returning null', + (contentType) => { + const visible = tab({ id: 'tab-1', entityId: 'file-1', contentType }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [visible] }, + groupsByWorktree: { + [WT]: [group({ id: GROUP_A, activeTabId: 'tab-1', tabOrder: ['tab-1'] })] + }, + activeGroupIdByWorktree: { [WT]: GROUP_A }, + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileIdByWorktree: { [WT]: 'file-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('tab-1') + } + ) + + it('returns the group active tab, not the tab the stale coarse address names', () => { + const shown = tab({ id: 'tab-shown', entityId: 'file-shown', contentType: 'editor' }) + const stale = tab({ id: 'tab-stale', entityId: 'file-stale', contentType: 'editor' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [stale, shown] }, + groupsByWorktree: { + [WT]: [ + group({ id: GROUP_A, activeTabId: 'tab-shown', tabOrder: ['tab-stale', 'tab-shown'] }) + ] + }, + activeGroupIdByWorktree: { [WT]: GROUP_A }, + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + // Why: activateTab writes group state only, so the legacy address lags behind. + activeFileIdByWorktree: { [WT]: 'file-stale' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('tab-shown') + }) + + // Why: copyUnifiedTabToGroup duplicates contentType + entityId into another group, so a plain + // array scan can return the background copy and drag group focus to it. + it('prefers the active group copy when one entity exists in two split groups', () => { + const background = tab({ + id: 'tab-bg', + entityId: 'file-1', + contentType: 'diff', + groupId: GROUP_A + }) + const focused = tab({ id: 'tab-fg', entityId: 'file-1', contentType: 'diff', groupId: GROUP_B }) + const state = makeState({ + // Why: background copy first — array order must not decide. + unifiedTabsByWorktree: { [WT]: [background, focused] }, + groupsByWorktree: { + [WT]: [ + group({ id: GROUP_A, activeTabId: 'tab-bg', tabOrder: ['tab-bg'] }), + group({ id: GROUP_B, activeTabId: 'tab-fg', tabOrder: ['tab-fg'] }) + ] + }, + activeGroupIdByWorktree: { [WT]: GROUP_B }, + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileIdByWorktree: { [WT]: 'file-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('tab-fg') + }) + + // Why: a focused empty split must not resolve into some other group. This pins the contract + // only — it does NOT claim empty-split focus survives reconciliation (see plan scope boundary). + it('returns null for a focused empty split rather than a background tab', () => { + const background = tab({ + id: 'tab-bg', + entityId: 'file-1', + contentType: 'editor', + groupId: GROUP_A + }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [background] }, + groupsByWorktree: { + [WT]: [ + group({ id: GROUP_A, activeTabId: 'tab-bg', tabOrder: ['tab-bg'] }), + group({ id: GROUP_B, activeTabId: null, tabOrder: [] }) + ] + }, + activeGroupIdByWorktree: { [WT]: GROUP_B }, + // Why: plain 'editor' — a diff here would already return null pre-fix, making this vacuous. + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileIdByWorktree: { [WT]: 'file-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBeNull() + }) + + it('ignores a tab whose groupId disagrees with the active group', () => { + const mismatched = tab({ + id: 'tab-1', + entityId: 'file-1', + contentType: 'diff', + groupId: GROUP_B + }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [mismatched] }, + groupsByWorktree: { + [WT]: [group({ id: GROUP_A, activeTabId: 'tab-1', tabOrder: ['tab-1'] })] + }, + activeGroupIdByWorktree: { [WT]: GROUP_A } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBeNull() + }) + + // Why: reconcile replaces a local tab with a mirrored one under a new id; focus must follow the + // entity instead of being dropped (which would hand activation to the snapshot). + it('follows the entity when the visible tab is rematerialized under a new id', () => { + const local = tab({ id: 'local-editor', entityId: '/repo/index.html', contentType: 'editor' }) + const mirrored = tab({ id: 'host-editor', entityId: '/repo/index.html', contentType: 'editor' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [local] }, + groupsByWorktree: { + [WT]: [group({ id: GROUP_A, activeTabId: 'local-editor', tabOrder: ['local-editor'] })] + }, + activeGroupIdByWorktree: { [WT]: GROUP_A } + }) + + // Why: the post-materialization tab set no longer contains the local id. + expect(resolveWebSessionVisibleTabId(state, WT, [mirrored])).toBe('host-editor') + }) + + it('does not follow a rematerialized entity into a different group', () => { + const local = tab({ id: 'local-editor', entityId: '/repo/index.html', contentType: 'editor' }) + const elsewhere = tab({ + id: 'host-editor', + entityId: '/repo/index.html', + contentType: 'editor', + groupId: GROUP_B + }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [local] }, + groupsByWorktree: { + [WT]: [group({ id: GROUP_A, activeTabId: 'local-editor', tabOrder: ['local-editor'] })] + }, + activeGroupIdByWorktree: { [WT]: GROUP_A } + }) + + expect(resolveWebSessionVisibleTabId(state, WT, [elsewhere])).toBeNull() + }) + + it('falls back to the first group when the active group id is stale', () => { + const visible = tab({ id: 'tab-1', entityId: 'file-1', contentType: 'diff' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [visible] }, + groupsByWorktree: { + [WT]: [group({ id: GROUP_A, activeTabId: 'tab-1', tabOrder: ['tab-1'] })] + }, + activeGroupIdByWorktree: { [WT]: 'group-that-no-longer-exists' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('tab-1') + }) +}) + +describe('resolveWebSessionVisibleTabId — no-group compatibility path', () => { + it('resolves a diff through the projection when there are no group records', () => { + const visible = tab({ id: 'tab-1', entityId: 'file-1', contentType: 'diff' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [visible] }, + groupsByWorktree: {}, + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileIdByWorktree: { [WT]: 'file-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('tab-1') + }) + + it('keeps remembered-terminal behaviour when there are no group records', () => { + const terminal = tab({ id: 'term-1', entityId: 'term-1', contentType: 'terminal' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [terminal] }, + groupsByWorktree: {}, + activeTabType: 'terminal', + activeTabTypeByWorktree: { [WT]: 'terminal' }, + activeTabIdByWorktree: { [WT]: 'term-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBe('term-1') + }) + + it('does not match a browser tab against an editor coarse address', () => { + const browser = tab({ id: 'tab-1', entityId: 'ws-1', contentType: 'browser' }) + const state = makeState({ + unifiedTabsByWorktree: { [WT]: [browser] }, + groupsByWorktree: {}, + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileIdByWorktree: { [WT]: 'ws-1' } + }) + + expect(resolveWebSessionVisibleTabId(state, WT)).toBeNull() + }) +}) + +describe('toVisibleTabType', () => { + it('collapses every editor-family kind and preserves the rest', () => { + expect(toVisibleTabType('editor')).toBe('editor') + expect(toVisibleTabType('diff')).toBe('editor') + expect(toVisibleTabType('conflict-review')).toBe('editor') + expect(toVisibleTabType('check-details')).toBe('editor') + expect(toVisibleTabType('terminal')).toBe('terminal') + expect(toVisibleTabType('browser')).toBe('browser') + expect(toVisibleTabType('simulator')).toBe('simulator') + }) +}) diff --git a/src/renderer/src/runtime/web-session-focus-intent.ts b/src/renderer/src/runtime/web-session-focus-intent.ts index e185250372f..e69d7edf467 100644 --- a/src/renderer/src/runtime/web-session-focus-intent.ts +++ b/src/renderer/src/runtime/web-session-focus-intent.ts @@ -10,6 +10,7 @@ // snapshots, unlike a transient per-snapshot flag). import { webSessionIntentOwnerKey, type WebSessionIntentOwner } from './web-session-intent-owner' +import { toVisibleTabType } from '../../../shared/tab-types' import type { AppState } from '../store/types' export type WebSessionFocusIntent = { @@ -24,10 +25,12 @@ type WebSessionVisibleTabState = Pick< AppState, | 'activeBrowserTabIdByWorktree' | 'activeFileIdByWorktree' + | 'activeGroupIdByWorktree' | 'activeTabIdByWorktree' | 'activeTabType' | 'activeTabTypeByWorktree' | 'activeWorktreeId' + | 'groupsByWorktree' | 'unifiedTabsByWorktree' > @@ -36,6 +39,42 @@ export function resolveWebSessionVisibleTabId( worktreeId: string, tabs = state.unifiedTabsByWorktree?.[worktreeId] ?? [] ): string | null { + // Why: the coarse (activeTabType, entityId) address inverts a many-to-one projection and cannot + // tell editor-family kinds apart; group state is what is actually on screen. + const groups = state.groupsByWorktree?.[worktreeId] ?? [] + if (groups.length > 0) { + const activeGroupId = state.activeGroupIdByWorktree?.[worktreeId] ?? null + const activeGroup = + (activeGroupId ? groups.find((group) => group.id === activeGroupId) : null) ?? groups[0] + // Why: authoritative that nothing is visible too — never resolve into an unfocused group. + if (activeGroup?.activeTabId == null) { + return null + } + const activeTabId = activeGroup.activeTabId + const direct = tabs.find((tab) => tab.id === activeTabId && tab.groupId === activeGroup.id) + if (direct) { + return direct.id + } + // Why: reconcile can rematerialize the visible tab under a new id (local -> mirrored), so + // follow its entity rather than dropping focus. Stays inside the group to avoid a pane jump. + const previous = (state.unifiedTabsByWorktree?.[worktreeId] ?? []).find( + (tab) => tab.id === activeTabId + ) + if (!previous) { + return null + } + const previousType = toVisibleTabType(previous.contentType) + return ( + tabs.find( + (tab) => + tab.groupId === activeGroup.id && + tab.entityId === previous.entityId && + toVisibleTabType(tab.contentType) === previousType + )?.id ?? null + ) + } + + // Why: no group records at all (fresh slice, or first remote reconcile before groups exist). const currentType = state.activeTabTypeByWorktree?.[worktreeId] ?? (state.activeWorktreeId === worktreeId ? state.activeTabType : null) @@ -50,7 +89,9 @@ export function resolveWebSessionVisibleTabId( ? state.activeFileIdByWorktree?.[worktreeId] : null return ( - tabs.find((tab) => tab.contentType === currentType && tab.entityId === entityId)?.id ?? null + tabs.find( + (tab) => toVisibleTabType(tab.contentType) === currentType && tab.entityId === entityId + )?.id ?? null ) } diff --git a/src/renderer/src/runtime/web-session-tabs-sync-diff-focus-steal.test.ts b/src/renderer/src/runtime/web-session-tabs-sync-diff-focus-steal.test.ts new file mode 100644 index 00000000000..b76a60aaefd --- /dev/null +++ b/src/renderer/src/runtime/web-session-tabs-sync-diff-focus-steal.test.ts @@ -0,0 +1,140 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { toWebTerminalSurfaceTabId } from '../../../shared/terminal-surface-id' +import type { Tab } from '../../../shared/tab-types' +import { applyWebSessionTabsSnapshot, type WebSessionTabsSyncState } from './web-session-tabs-sync' +import { + ENV, + LEAF_ID, + NOW, + WT, + makeSnapshot, + makeState, + resetWebSessionTabsSyncTestState +} from './web-session-tabs-sync-test-harness' + +vi.mock('../store', () => ({ + useAppStore: { + setState: vi.fn() + } +})) + +const HOST_GROUP = 'host-group-1' +const DIFF_TAB_ID = 'local-diff-tab' +const DIFF_FILE_ID = `${WT}::diff::unstaged::src/example.ts` + +function diffTab(): Tab { + return { + id: DIFF_TAB_ID, + entityId: DIFF_FILE_ID, + groupId: HOST_GROUP, + worktreeId: WT, + contentType: 'diff', + label: 'example.ts', + customLabel: null, + color: null, + sortOrder: 1, + createdAt: NOW, + isPreview: false, + isPinned: false + } +} + +function mirroredTerminalTab(terminalTabId: string): Tab { + return { + id: terminalTabId, + entityId: terminalTabId, + groupId: HOST_GROUP, + worktreeId: WT, + contentType: 'terminal', + label: 'codex [working]', + customLabel: null, + color: null, + sortOrder: 0, + createdAt: NOW, + isPreview: false, + isPinned: false + } +} + +// Why: STA-4697. An agent status echo republishes the snapshot every turn; the diff tab must +// keep activation instead of falling through to the mirrored terminal. +describe('applyWebSessionTabsSnapshot — diff tab focus', () => { + beforeEach(resetWebSessionTabsSyncTestState) + + it('does not let a terminal status echo steal activation from an open diff', () => { + const terminalTabId = toWebTerminalSurfaceTabId('host-tab-1') + const state = makeState({ + activeTabType: 'editor', + activeTabTypeByWorktree: { [WT]: 'editor' }, + activeFileId: DIFF_FILE_ID, + activeFileIdByWorktree: { [WT]: DIFF_FILE_ID }, + tabsByWorktree: { + [WT]: [ + { + id: terminalTabId, + ptyId: 'remote:web-env-1@@terminal-1', + worktreeId: WT, + title: 'codex [working]', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: NOW + } + ] + }, + unifiedTabsByWorktree: { [WT]: [mirroredTerminalTab(terminalTabId), diffTab()] }, + tabBarOrderByWorktree: { [WT]: [terminalTabId, DIFF_TAB_ID] }, + groupsByWorktree: { + [WT]: [ + { + id: HOST_GROUP, + worktreeId: WT, + // Why: the diff is what the user is looking at. + activeTabId: DIFF_TAB_ID, + // Why: the mirrored terminal must share the group, or the membership guard in the + // group writers masks the steal and this test passes pre-fix. + tabOrder: [terminalTabId, DIFF_TAB_ID], + recentTabIds: [terminalTabId, DIFF_TAB_ID] + } + ] + }, + activeGroupIdByWorktree: { [WT]: HOST_GROUP } + }) + + const statusEcho = makeSnapshot( + [ + { + type: 'terminal', + id: `host-tab-1::${LEAF_ID}`, + title: 'codex [thinking]', + parentTabId: 'host-tab-1', + leafId: LEAF_ID, + isActive: true, + status: 'ready', + terminal: 'terminal-1' + } + ], + { + activeTabId: `host-tab-1::${LEAF_ID}`, + activeTabType: 'terminal', + tabGroups: [{ id: HOST_GROUP, activeTabId: 'host-tab-1', tabOrder: ['host-tab-1'] }] + } + ) + + const patch = applyWebSessionTabsSnapshot( + state, + statusEcho, + ENV, + NOW + 10 + ) as Partial + + const nextGroups = patch.groupsByWorktree?.[WT] ?? state.groupsByWorktree[WT] + const activeGroupId = patch.activeGroupIdByWorktree?.[WT] ?? state.activeGroupIdByWorktree[WT] + const nextTabs = patch.unifiedTabsByWorktree?.[WT] ?? state.unifiedTabsByWorktree[WT] + + expect(nextGroups?.find((group) => group.id === HOST_GROUP)?.activeTabId).toBe(DIFF_TAB_ID) + expect(activeGroupId).toBe(HOST_GROUP) + // Why: the reported symptom left the tab present but deactivated. + expect(nextTabs?.some((tab) => tab.id === DIFF_TAB_ID)).toBe(true) + }) +}) diff --git a/src/renderer/src/store/slices/tabs.ts b/src/renderer/src/store/slices/tabs.ts index 1c318830e0f..b1f1392d82f 100644 --- a/src/renderer/src/store/slices/tabs.ts +++ b/src/renderer/src/store/slices/tabs.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines -- Why: split-tab group state updates layout, focus, and tab membership atomically in one slice to avoid split-brain. */ import type { StateCreator } from 'zustand' import type { AppState } from '../types' +import { toVisibleTabType } from '../../../../shared/tab-types' import type { Tab, TabContentType, @@ -415,13 +416,6 @@ function collapseGroupLayout( } } -function toVisibleTabType(contentType: TabContentType): WorkspaceVisibleTabType { - if (contentType === 'browser' || contentType === 'terminal' || contentType === 'simulator') { - return contentType - } - return 'editor' -} - function deriveActiveSurfaceForWorktree( state: Pick< AppState, diff --git a/src/renderer/src/store/slices/worktrees/listing/worktree-catalog-visibility.ts b/src/renderer/src/store/slices/worktrees/listing/worktree-catalog-visibility.ts index 90838e5397f..ce7a13c4b25 100644 --- a/src/renderer/src/store/slices/worktrees/listing/worktree-catalog-visibility.ts +++ b/src/renderer/src/store/slices/worktrees/listing/worktree-catalog-visibility.ts @@ -1,5 +1,4 @@ import { catalogRowsEqual } from '../../worktree-catalog-reconciliation' -import type { WorkspaceVisibleTabType } from '../../../../../../shared/tab-types' import type { DetectedWorktreeListResult, Worktree } from '../../../../../../shared/worktree/types' export function areWorktreesEqual(current: Worktree[] | undefined, next: Worktree[]): boolean { @@ -19,13 +18,6 @@ export function areDetectedWorktreeResultsEqual( ) } -export function toVisibleTabType(contentType: string): WorkspaceVisibleTabType { - if (contentType === 'browser' || contentType === 'terminal' || contentType === 'simulator') { - return contentType - } - return 'editor' -} - export function toVisibleWorktree( worktree: DetectedWorktreeListResult['worktrees'][number] ): Worktree { diff --git a/src/renderer/src/store/slices/worktrees/session/active-worktree-surface.ts b/src/renderer/src/store/slices/worktrees/session/active-worktree-surface.ts index 01a969d39d5..6dcfc62d155 100644 --- a/src/renderer/src/store/slices/worktrees/session/active-worktree-surface.ts +++ b/src/renderer/src/store/slices/worktrees/session/active-worktree-surface.ts @@ -1,6 +1,6 @@ import type { AppState } from '../../../types' import type { WorkspaceVisibleTabType } from '../../../../../../shared/tab-types' -import { toVisibleTabType } from '../listing/worktree-catalog-visibility' +import { toVisibleTabType } from '../../../../../../shared/tab-types' export function resolveActivatedWorktreeSurface( s: AppState, diff --git a/src/shared/tab-types.ts b/src/shared/tab-types.ts index e321aaaee6f..e456c1df412 100644 --- a/src/shared/tab-types.ts +++ b/src/shared/tab-types.ts @@ -27,6 +27,15 @@ export type TabContentType = export type WorkspaceVisibleTabType = 'terminal' | 'editor' | 'browser' | 'simulator' export type CtrlTabOrderMode = 'mru' | 'sequential' +// Why: many-to-one — every editor-family kind collapses to 'editor'. Never invert it by equality; +// resolve the concrete tab and project forward instead. +export function toVisibleTabType(contentType: TabContentType): WorkspaceVisibleTabType { + if (contentType === 'browser' || contentType === 'terminal' || contentType === 'simulator') { + return contentType + } + return 'editor' +} + export type Tab = { id: string // UUID for terminals, filePath for editors (preserves current convention) entityId: string // ID of the backing content (terminal tab ID, file path, browser workspace ID)