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
This commit is contained in:
Brennan Benson
2026-10-11 14:14:01 -07:00
committed by GitHub
parent e0090f9bda
commit c07b045934
7 changed files with 124 additions and 74 deletions
@@ -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()
@@ -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)
}
@@ -18,6 +18,7 @@ const mocks = vi.hoisted(() => ({
state: {} as Record<string, unknown>,
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'])
})
})
@@ -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(
@@ -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
@@ -0,0 +1,69 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
const mocks = vi.hoisted(() => {
const state: Record<string, unknown> = {}
const listeners: Record<string, (...args: unknown[]) => 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'])
})
})
@@ -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)
})()
})