From c07b045934e6a021bd0f138342ee308aa4872dae Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 11 Oct 2026 14:14:01 -0700 Subject: [PATCH] Replace three copied new-terminal tab-order blocks with the shared helper (#26771) * Keep browser tabs in place when the + button opens a terminal The + new-terminal path kept its own copy of the tab-order placement and left browser tabs out, so in a workspace with no saved order a browser tab could land after the new terminal. Use the shared persistAgentLaunchTabOrder helper instead. * Use the shared tab-order helper for the remaining new-terminal paths handleNewTab's shell-override fallback and the menu new-terminal IPC fallback each kept an inline copy of the "put the new tab last" order logic. Both now call persistAgentLaunchTabOrder. The one behaviour difference: the shared reconcile drops ids repeated in a saved order, where the copies kept them. * Drop unchecked casts from the new tab-order tests --- .../terminal/terminal-tab-actions.test.ts | 22 ++++++ .../terminal/terminal-tab-create.ts | 23 ++----- .../use-terminal-create-actions.test.tsx | 26 ++++++- .../components/use-terminal-create-actions.ts | 33 ++------- .../use-terminal-workspace-store-bindings.ts | 2 - .../tab-lifecycle-ipc-bridge.test.ts | 69 +++++++++++++++++++ .../ipc-events/tab-lifecycle-ipc-bridge.ts | 23 +------ 7 files changed, 124 insertions(+), 74 deletions(-) create mode 100644 src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.test.ts diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts index 51a582a2e4b..9d8d6d41d84 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts @@ -82,6 +82,28 @@ describe('createNewTerminalTab', () => { expect(createWebRuntimeSessionTerminalMock).not.toHaveBeenCalled() }) + it('keeps open browser tabs before the new terminal when no order is saved yet', () => { + const setTabBarOrder = vi.fn() + getStateMock + .mockReturnValueOnce({ + settings: { activeRuntimeEnvironmentId: null }, + createTab: vi.fn(() => ({ id: 'term-new' })), + setActiveTabType: vi.fn(), + setTabBarOrder + }) + .mockReturnValueOnce({ + tabsByWorktree: { 'wt-1': [{ id: 'term-a' }, { id: 'term-new' }] }, + openFiles: [], + browserTabsByWorktree: { 'wt-1': [{ id: 'browser-1' }] }, + tabBarOrderByWorktree: {}, + setTabBarOrder + }) + + createNewTerminalTab('wt-1') + + expect(setTabBarOrder).toHaveBeenCalledWith('wt-1', ['term-a', 'browser-1', 'term-new']) + }) + it('delegates terminal creation to the host runtime in paired web clients', () => { const createTab = vi.fn(() => ({ id: 'tab-1' })) const setActiveTabType = vi.fn() diff --git a/src/renderer/src/components/terminal/terminal-tab-create.ts b/src/renderer/src/components/terminal/terminal-tab-create.ts index 7add094bb91..aff5357da5e 100644 --- a/src/renderer/src/components/terminal/terminal-tab-create.ts +++ b/src/renderer/src/components/terminal/terminal-tab-create.ts @@ -1,10 +1,10 @@ import { useAppStore } from '@/store' -import { reconcileTabOrder } from '../tab-bar/reconcile-order' import { createWebRuntimeSessionTerminal, isWebRuntimeSessionActive } from '@/runtime/web-runtime-session' import { resolveTerminalWorktreeRoute } from '@/lib/terminal-worktree-route' +import { persistAgentLaunchTabOrder } from '@/lib/launch-agent-tab-order' export function createNewTerminalTab( activeWorktreeId: string | null, @@ -40,22 +40,7 @@ export function createNewTerminalTab( options?.startupCwd ? { startupCwd: options.startupCwd } : undefined ) state.setActiveTabType('terminal', activeWorktreeId) - // Why: persist the tab bar order with the new terminal at the end of the - // current visual order. Without this, reconcileTabOrder falls back to - // terminals-first when tabBarOrderByWorktree is unset, causing a new - // terminal to jump to index 0 instead of appending after editor tabs. - const freshState = useAppStore.getState() - const termIds = (freshState.tabsByWorktree[activeWorktreeId] ?? []).map((t) => t.id) - const editorIds = freshState.openFiles - .filter((f) => f.worktreeId === activeWorktreeId) - .map((f) => f.id) - const base = reconcileTabOrder( - freshState.tabBarOrderByWorktree[activeWorktreeId], - termIds, - editorIds - ) - // The new tab is already in base via termIds; move it to the end - const order = base.filter((id) => id !== newTab.id) - order.push(newTab.id) - state.setTabBarOrder(activeWorktreeId, order) + // Why: without a saved order the tab bar falls back to terminals-first, so + // persist the current visual order with the new terminal last. + persistAgentLaunchTabOrder(activeWorktreeId, newTab.id) } diff --git a/src/renderer/src/components/use-terminal-create-actions.test.tsx b/src/renderer/src/components/use-terminal-create-actions.test.tsx index 0c05e2a172c..873a598bbca 100644 --- a/src/renderer/src/components/use-terminal-create-actions.test.tsx +++ b/src/renderer/src/components/use-terminal-create-actions.test.tsx @@ -18,6 +18,7 @@ const mocks = vi.hoisted(() => ({ state: {} as Record, toastError: vi.fn(), createBrowserTab: vi.fn(), + createTab: vi.fn(), openNewBrowserTabInActiveWorkspace: vi.fn(), openMobileEmulatorTab: vi.fn() })) @@ -58,15 +59,15 @@ const WORKTREE_ID = 'repo-1::/repo/worktree' function renderActions() { return renderHook(() => + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the hook reads only these controller fields. useTerminalCreateActions({ activeWorktreeId: WORKTREE_ID, createBrowserTab: mocks.createBrowserTab, - createTab: vi.fn(), + createTab: mocks.createTab, openNewBrowserTabInActiveWorkspace: mocks.openNewBrowserTabInActiveWorkspace, openNewMarkdownInActiveWorkspace: vi.fn(), openNewTerminalTabInActiveWorkspace: vi.fn(), - setActiveTabType: vi.fn(), - setTabBarOrder: vi.fn() + setActiveTabType: vi.fn() } as unknown as TerminalColdActivationController) ).result.current } @@ -116,4 +117,23 @@ describe('useTerminalCreateActions creation gates', () => { await new Promise((resolve) => setTimeout(resolve, 0)) expect(mocks.toastError).toHaveBeenCalledWith('emulator says no') }) + + it('appends a shell-override terminal to the saved order, dropping repeated ids', () => { + const setTabBarOrder = vi.fn() + const terminals = [{ id: 'term-a' }] + mocks.state = { + ...mocks.state, + tabsByWorktree: { [WORKTREE_ID]: terminals }, + openFiles: [], + tabBarOrderByWorktree: { [WORKTREE_ID]: ['browser-1', 'term-a', 'browser-1'] }, + setTabBarOrder + } + mocks.createTab.mockImplementation(() => { + terminals.push({ id: 'term-new' }) + return { id: 'term-new' } + }) + renderActions().handleNewTab('/bin/zsh') + expect(mocks.createTab).toHaveBeenCalledWith(WORKTREE_ID, undefined, '/bin/zsh') + expect(setTabBarOrder).toHaveBeenCalledWith(WORKTREE_ID, ['browser-1', 'term-a', 'term-new']) + }) }) diff --git a/src/renderer/src/components/use-terminal-create-actions.ts b/src/renderer/src/components/use-terminal-create-actions.ts index 2af2f068a3f..d358baffd8c 100644 --- a/src/renderer/src/components/use-terminal-create-actions.ts +++ b/src/renderer/src/components/use-terminal-create-actions.ts @@ -10,6 +10,7 @@ import { } from '@/runtime/web-runtime-session' import { openMobileEmulatorTab } from '@/lib/open-mobile-emulator-tab' import { launchAgentInNewTab } from '@/lib/launch-agent-in-new-tab' +import { persistAgentLaunchTabOrder } from '@/lib/launch-agent-tab-order' import { newAgentLaunchRequestId } from '@/lib/agent-launch-request-id' import { buildDuplicatedBrowserTabOptions } from '@/lib/duplicate-browser-tab-options' import { browserWorkspaceHasRemoteOwner } from '@/runtime/remote-browser-tab-ownership' @@ -28,8 +29,7 @@ export function useTerminalCreateActions(controller: TerminalColdActivationContr openNewBrowserTabInActiveWorkspace, openNewMarkdownInActiveWorkspace, openNewTerminalTabInActiveWorkspace, - setActiveTabType, - setTabBarOrder + setActiveTabType } = controller const handleNewTab = useCallback( (shellOverride?: string) => { @@ -56,35 +56,10 @@ export function useTerminalCreateActions(controller: TerminalColdActivationContr } const newTab = createTab(activeWorktreeId, undefined, shellOverride) setActiveTabType('terminal', activeWorktreeId) - const state = useAppStore.getState() - const currentTerminals = state.tabsByWorktree[activeWorktreeId] ?? [] - const currentEditors = state.openFiles.filter((file) => file.worktreeId === activeWorktreeId) - const currentBrowsers = state.browserTabsByWorktree[activeWorktreeId] ?? [] - const stored = state.tabBarOrderByWorktree[activeWorktreeId] - const termIds = currentTerminals.map((tab) => tab.id) - const editorIds = currentEditors.map((file) => file.id) - const browserIds = currentBrowsers.map((tab) => tab.id) - const validIds = new Set([...termIds, ...editorIds, ...browserIds]) - const base = (stored ?? []).filter((id) => validIds.has(id)) - const inBase = new Set(base) - for (const id of [...termIds, ...editorIds, ...browserIds]) { - if (!inBase.has(id)) { - base.push(id) - inBase.add(id) - } - } - const order = base.filter((id) => id !== newTab.id) - order.push(newTab.id) - setTabBarOrder(activeWorktreeId, order) + persistAgentLaunchTabOrder(activeWorktreeId, newTab.id) focusTerminalTabSurface(newTab.id) }, - [ - activeWorktreeId, - createTab, - openNewTerminalTabInActiveWorkspace, - setActiveTabType, - setTabBarOrder - ] + [activeWorktreeId, createTab, openNewTerminalTabInActiveWorkspace, setActiveTabType] ) const handleNewAgentTab = useCallback( diff --git a/src/renderer/src/components/use-terminal-workspace-store-bindings.ts b/src/renderer/src/components/use-terminal-workspace-store-bindings.ts index 5a20a33b273..54de946b29d 100644 --- a/src/renderer/src/components/use-terminal-workspace-store-bindings.ts +++ b/src/renderer/src/components/use-terminal-workspace-store-bindings.ts @@ -57,7 +57,6 @@ export function useTerminalWorkspaceStoreBindings(controller: TerminalWorkspaceF const activeGroupIdByWorktree = useAppStore((state) => state.activeGroupIdByWorktree) const ensureWorktreeRootGroup = useAppStore((state) => state.ensureWorktreeRootGroup) const reconcileWorktreeTabModel = useAppStore((state) => state.reconcileWorktreeTabModel) - const setTabBarOrder = useAppStore((state) => state.setTabBarOrder) const tabBarOrderByWorktree = useAppStore((state) => state.tabBarOrderByWorktree) const tabBarOrder = renderedActiveWorktreeId ? tabBarOrderByWorktree[renderedActiveWorktreeId] @@ -105,7 +104,6 @@ export function useTerminalWorkspaceStoreBindings(controller: TerminalWorkspaceF activeGroupIdByWorktree, ensureWorktreeRootGroup, reconcileWorktreeTabModel, - setTabBarOrder, tabBarOrderByWorktree, tabBarOrder, activityTerminalPortals diff --git a/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.test.ts b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.test.ts new file mode 100644 index 00000000000..65f5a4d88a9 --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.test.ts @@ -0,0 +1,69 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => { + const state: Record = {} + const listeners: Record void> = {} + return { state, listeners } +}) + +vi.mock('../../store', () => ({ useAppStore: { getState: () => mocks.state } })) +vi.mock('@/lib/focus-terminal-tab-surface', () => ({ focusTerminalTabSurface: vi.fn() })) +vi.mock('@/lib/worktree-runtime-owner', () => ({ + getRuntimeEnvironmentIdForWorktree: () => null +})) +vi.mock('@/runtime/web-runtime-session', () => ({ + createWebRuntimeSessionTerminal: vi.fn().mockResolvedValue({ status: 'failed', message: '' }), + isWebRuntimeSessionActive: () => false +})) +vi.mock('@/lib/workspace-tab-commands', () => ({ dispatchWorkspaceTabCommand: vi.fn() })) +vi.mock('@/lib/floating-workspace-terminal-actions', () => ({ + createFloatingWorkspaceTerminalTab: vi.fn(), + isFloatingWorkspacePanelFocused: () => false, + resolveFloatingWorkspaceBrowserWorkspaceId: vi.fn() +})) +vi.mock('@/lib/floating-workspace-guest-bridge', () => ({ + dispatchFloatingWorkspaceGuestClose: vi.fn(), + dispatchFloatingWorkspaceGuestSelectIndex: vi.fn() +})) + +import { registerTabLifecycleIpcBridge } from './tab-lifecycle-ipc-bridge' + +const WORKTREE_ID = 'repo-1::/repo/worktree' + +describe('registerTabLifecycleIpcBridge new terminal tab', () => { + beforeEach(() => { + mocks.listeners = {} + const ui = new Proxy( + {}, + { + get: (_target, name) => (listener: (...args: unknown[]) => void) => { + mocks.listeners[String(name)] = listener + return () => {} + } + } + ) + vi.stubGlobal('window', { api: { ui } }) + }) + + it('appends the new terminal to the saved order, dropping repeated ids', async () => { + const setTabBarOrder = vi.fn() + const terminals = [{ id: 'term-a' }] + mocks.state = { + activeWorktreeId: WORKTREE_ID, + tabsByWorktree: { [WORKTREE_ID]: terminals }, + openFiles: [], + browserTabsByWorktree: { [WORKTREE_ID]: [{ id: 'browser-1' }] }, + tabBarOrderByWorktree: { [WORKTREE_ID]: ['browser-1', 'term-a', 'browser-1'] }, + createTab: () => { + terminals.push({ id: 'term-new' }) + return { id: 'term-new' } + }, + setActiveTabType: vi.fn(), + setTabBarOrder + } + registerTabLifecycleIpcBridge([]) + mocks.listeners.onNewTerminalTab() + await vi.waitFor(() => expect(setTabBarOrder).toHaveBeenCalled()) + expect(setTabBarOrder).toHaveBeenCalledWith(WORKTREE_ID, ['browser-1', 'term-a', 'term-new']) + }) +}) diff --git a/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts index f6e842bf071..24114a6d8db 100644 --- a/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/tab-lifecycle-ipc-bridge.ts @@ -1,4 +1,5 @@ import { focusTerminalTabSurface } from '@/lib/focus-terminal-tab-surface' +import { persistAgentLaunchTabOrder } from '@/lib/launch-agent-tab-order' import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { createWebRuntimeSessionTerminal, @@ -44,27 +45,7 @@ export function registerTabLifecycleIpcBridge(unsubs: (() => void)[]): void { } const newTab = store.createTab(worktreeId) store.setActiveTabType('terminal', worktreeId) - // Why: mirror Terminal.tsx handleNewTab so a new tab appends at the end, not index 0, when tabBarOrder is unset. - const freshStore = useAppStore.getState() - const currentTerminals = freshStore.tabsByWorktree[worktreeId] ?? [] - const currentEditors = freshStore.openFiles.filter((f) => f.worktreeId === worktreeId) - const currentBrowsers = freshStore.browserTabsByWorktree[worktreeId] ?? [] - const stored = freshStore.tabBarOrderByWorktree[worktreeId] - const termIds = currentTerminals.map((t) => t.id) - const editorIds = currentEditors.map((f) => f.id) - const browserIds = currentBrowsers.map((tab) => tab.id) - const validIds = new Set([...termIds, ...editorIds, ...browserIds]) - const base = (stored ?? []).filter((id) => validIds.has(id)) - const inBase = new Set(base) - for (const id of [...termIds, ...editorIds, ...browserIds]) { - if (!inBase.has(id)) { - base.push(id) - inBase.add(id) - } - } - const order = base.filter((id) => id !== newTab.id) - order.push(newTab.id) - freshStore.setTabBarOrder(worktreeId, order) + persistAgentLaunchTabOrder(worktreeId, newTab.id) focusTerminalTabSurface(newTab.id) })() })