feat: when closing a tab, go to previous tab (MRU) not nearest neighbor (#784)

This commit is contained in:
Neil
2026-04-17 19:43:22 -07:00
committed by GitHub
parent 8d3494fe07
commit 8dbfec4e20
11 changed files with 372 additions and 26 deletions
+4
View File
@@ -4,6 +4,10 @@
When writing or modifying code driven by a design doc or non-obvious constraint, you **must** add a comment explaining **why** the code behaves the way it does.
## File and Module Naming
Never use vague names like `helpers`, `utils`, `common`, `misc`, or `shared-stuff` for files, folders, or modules. They carry zero information and tend to become dumping grounds. Name files after what they *actually* contain — prefer the concrete domain concept (e.g. `tab-group-state.ts`, `terminal-orphan-cleanup.ts`) over the generic role (`tabs-helpers.ts`, `terminal-utils.ts`). If you find yourself reaching for `helpers`, the file probably has more than one responsibility and should be split, or there's a better name hiding in the code that describes what the functions operate on.
## Worktree Safety
Always use the primary working directory (the worktree) for all file reads and edits. Never follow absolute paths from subagent results that point to the main repo.
+1 -1
View File
@@ -12,7 +12,7 @@ import type {
WorkspaceSessionState
} from '../../../../shared/types'
import { ORCA_BROWSER_BLANK_URL } from '../../../../shared/constants'
import { pickNeighbor } from './tabs-helpers'
import { pickNeighbor } from './tab-group-state'
type CreateBrowserTabOptions = {
activate?: boolean
+6 -5
View File
@@ -700,11 +700,12 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
}
})
// Why: the unified tab model drives visual tabbar order and neighbor
// selection via pickNeighbor(group.tabOrder). Without this, closing an
// editor/diff tab picks the next active file from the openFiles array
// instead of the visual tab order, producing inconsistent behavior vs
// terminal/browser tab closes which already go through closeUnifiedTab.
// Why: the unified tab model drives visual tab-bar order and next-active
// selection (MRU-based, falling back to the visual neighbor). Without
// this, closing an editor/diff tab picks the next active file from the
// openFiles array instead of running the unified close path, producing
// inconsistent behavior vs terminal/browser tab closes which already go
// through closeUnifiedTab.
for (const tabs of Object.values(get().unifiedTabsByWorktree ?? {})) {
const unifiedTab = tabs.find(
(entry) =>
@@ -5,9 +5,12 @@ import {
findGroupForTab,
ensureGroup,
pickNeighbor,
pickNextActiveTab,
pushRecentTabId,
sanitizeRecentTabIds,
updateGroup,
patchTab
} from './tabs-helpers'
} from './tab-group-state'
function makeTab(overrides: Partial<Tab> & { id: string; worktreeId: string }): Tab {
return {
@@ -89,6 +92,63 @@ describe('pickNeighbor', () => {
})
})
describe('pushRecentTabId', () => {
it('appends a new id to the tail', () => {
expect(pushRecentTabId(['a', 'b'], 'c')).toEqual(['a', 'b', 'c'])
})
it('moves an existing id to the tail', () => {
expect(pushRecentTabId(['a', 'b', 'c'], 'b')).toEqual(['a', 'c', 'b'])
})
it('is a no-op when the id is already at the tail', () => {
const input = ['a', 'b']
expect(pushRecentTabId(input, 'b')).toBe(input)
})
it('handles undefined as empty', () => {
expect(pushRecentTabId(undefined, 'a')).toEqual(['a'])
})
})
describe('sanitizeRecentTabIds', () => {
it('drops ids not present in tabOrder', () => {
expect(sanitizeRecentTabIds(['a', 'b', 'c'], ['a', 'c'])).toEqual(['a', 'c'])
})
it('keeps only the last occurrence of duplicates', () => {
expect(sanitizeRecentTabIds(['a', 'b', 'a', 'c', 'b'], ['a', 'b', 'c'])).toEqual([
'a',
'c',
'b'
])
})
it('returns empty for undefined or empty input', () => {
expect(sanitizeRecentTabIds(undefined, ['a'])).toEqual([])
expect(sanitizeRecentTabIds([], ['a'])).toEqual([])
})
})
describe('pickNextActiveTab', () => {
it('returns the most-recent non-closing id', () => {
expect(pickNextActiveTab(['a', 'b', 'c'], ['a', 'c', 'b'], 'b')).toBe('c')
})
it('skips the closing id if it appears in MRU', () => {
expect(pickNextActiveTab(['a', 'b', 'c'], ['a', 'b', 'c'], 'c')).toBe('b')
})
it('falls back to visual neighbor when MRU is empty or has only the closing id', () => {
expect(pickNextActiveTab(['a', 'b', 'c'], [], 'b')).toBe('c')
expect(pickNextActiveTab(['a', 'b', 'c'], ['b'], 'b')).toBe('c')
})
it('falls back to left neighbor when closing the rightmost and MRU is empty', () => {
expect(pickNextActiveTab(['a', 'b', 'c'], undefined, 'c')).toBe('b')
})
})
describe('updateGroup', () => {
it('replaces the matching group', () => {
const g1: TabGroup = { id: 'g1', worktreeId: 'w1', activeTabId: null, tabOrder: [] }
@@ -93,6 +93,62 @@ export function pickNeighbor(tabOrder: string[], closingTabId: string): string |
return null
}
/** Normalize an MRU stack: drop ids not in `tabOrder` and keep only the last
* occurrence of each id (tail = most recent). */
export function sanitizeRecentTabIds(recent: string[] | undefined, tabOrder: string[]): string[] {
if (!recent || recent.length === 0) {
return []
}
const valid = new Set(tabOrder)
// Walk right-to-left so we keep only the latest occurrence of each id, then
// reverse back to oldest-→-newest order.
const seen = new Set<string>()
const reversed: string[] = []
for (let i = recent.length - 1; i >= 0; i--) {
const id = recent[i]
if (!valid.has(id) || seen.has(id)) {
continue
}
seen.add(id)
reversed.push(id)
}
return reversed.reverse()
}
/** Push `tabId` to the tail of the MRU stack (most-recently-active) after
* removing any prior occurrence. Returns a new array. */
export function pushRecentTabId(recent: string[] | undefined, tabId: string): string[] {
const base = recent ?? []
if (base.length > 0 && base.at(-1) === tabId) {
return base
}
const filtered = base.filter((id) => id !== tabId)
filtered.push(tabId)
return filtered
}
/** Choose the tab to activate when `closingTabId` closes. Prefers the most-
* recently-active tab before it (MRU behavior); falls back to the nearest
* visual neighbor when the MRU stack is empty (e.g. newly hydrated groups
* where only the current active tab has been recorded). */
export function pickNextActiveTab(
tabOrder: string[],
recentTabIds: string[] | undefined,
closingTabId: string
): string | null {
const sanitized = sanitizeRecentTabIds(recentTabIds, tabOrder)
// The closing tab is typically at the tail (it's the active tab). Walk back
// from the tail looking for the most-recent *other* tab still present.
for (let i = sanitized.length - 1; i >= 0; i--) {
if (sanitized[i] !== closingTabId) {
return sanitized[i]
}
}
// No prior tab has been visited in this group — fall back to neighbor
// selection so the user still lands somewhere sensible.
return pickNeighbor(tabOrder, closingTabId)
}
export function updateGroup(groups: TabGroup[], updated: TabGroup): TabGroup[] {
return groups.map((g) => (g.id === updated.id ? updated : g))
}
@@ -203,7 +203,13 @@ describe('buildHydratedTabState unified format', () => {
expect.objectContaining({ id: 'editor-1', groupId: 'g2', contentType: 'editor' })
])
expect(result.groupsByWorktree.w1).toEqual([
{ id: 'g2', worktreeId: 'w1', activeTabId: 'editor-1', tabOrder: ['editor-1'] }
{
id: 'g2',
worktreeId: 'w1',
activeTabId: 'editor-1',
tabOrder: ['editor-1'],
recentTabIds: ['editor-1']
}
])
expect(result.activeGroupIdByWorktree.w1).toBe('g2')
expect(result.layoutByWorktree.w1).toEqual({ type: 'leaf', groupId: 'g2' })
@@ -8,8 +8,9 @@ import {
dedupeTabOrder,
getPersistedEditFileIdsByWorktree,
isTransientEditorContentType,
sanitizeRecentTabIds,
selectHydratedActiveGroupId
} from './tabs-helpers'
} from './tab-group-state'
type HydratedTabState = {
unifiedTabsByWorktree: Record<string, Tab[]>
@@ -88,10 +89,21 @@ function hydrateUnifiedFormat(
// writes. Deduping during hydration restores the store invariant before
// later group operations branch on tab counts or neighbors.
const tabOrder = dedupeTabOrder(g.tabOrder.filter((tid) => validTabIds.has(tid)))
const activeTabId = g.activeTabId && validTabIds.has(g.activeTabId) ? g.activeTabId : null
// Why: persisted MRU may reference tabs that no longer exist. Sanitize
// against the live tabOrder, then ensure the current active tab sits at
// the tail so the first close after restore jumps back to the previous
// tab rather than falling through to neighbor selection.
const sanitizedRecent = sanitizeRecentTabIds(g.recentTabIds, tabOrder)
const recentTabIds =
activeTabId && sanitizedRecent.at(-1) !== activeTabId
? [...sanitizedRecent.filter((id) => id !== activeTabId), activeTabId]
: sanitizedRecent
return {
...g,
tabOrder,
activeTabId: g.activeTabId && validTabIds.has(g.activeTabId) ? g.activeTabId : null
activeTabId,
recentTabIds
}
})
const hydratedGroups = validatedGroups.filter((group, index) => {
@@ -203,7 +215,18 @@ function hydrateLegacyFormat(
}
tabsByWorktree[worktreeId] = tabs
groupsByWorktree[worktreeId] = [{ id: groupId, worktreeId, activeTabId, tabOrder }]
groupsByWorktree[worktreeId] = [
{
id: groupId,
worktreeId,
activeTabId,
tabOrder,
// Why: legacy sessions don't persist MRU; seed with the active tab so
// the first close after a legacy restore still behaves MRU-ish (falls
// back to neighbor selection if only one tab is in the stack).
recentTabIds: activeTabId ? [activeTabId] : []
}
]
activeGroupIdByWorktree[worktreeId] = groupId
layoutByWorktree[worktreeId] = { type: 'leaf', groupId }
}
+120
View File
@@ -244,6 +244,126 @@ describe('TabsSlice', () => {
const result = store.getState().closeUnifiedTab('nonexistent')
expect(result).toBeNull()
})
it('activates the previously-active tab (MRU) instead of the visual neighbor', () => {
const t1 = store.getState().createUnifiedTab(WT, 'terminal')
const t2 = store.getState().createUnifiedTab(WT, 'terminal')
const t3 = store.getState().createUnifiedTab(WT, 'terminal')
// Visit order: ...→t3 (last created)→t1→t3. Closing t3 should jump
// back to t1 (previous), not t2 (the visual right-neighbor after t3's
// removal fallback or left-neighbor).
store.getState().activateTab(t1.id)
store.getState().activateTab(t3.id)
store.getState().closeUnifiedTab(t3.id)
expect(store.getState().groupsByWorktree[WT][0].activeTabId).toBe(t1.id)
// t2 should still exist and not be active
expect(
store
.getState()
.unifiedTabsByWorktree[WT].map((t) => t.id)
.sort()
).toEqual([t1.id, t2.id].sort())
})
it('falls back to neighbor selection when the MRU stack has no prior tab', () => {
// Build state manually so no prior activations have been recorded. This
// mirrors a freshly-hydrated session with only an active tab known.
const groupId = 'mru-fallback-group'
store.setState({
unifiedTabsByWorktree: {
[WT]: [
{
id: 'a',
entityId: 'a',
groupId,
worktreeId: WT,
contentType: 'terminal',
label: 'a',
customLabel: null,
color: null,
sortOrder: 0,
createdAt: 1
},
{
id: 'b',
entityId: 'b',
groupId,
worktreeId: WT,
contentType: 'terminal',
label: 'b',
customLabel: null,
color: null,
sortOrder: 1,
createdAt: 2
},
{
id: 'c',
entityId: 'c',
groupId,
worktreeId: WT,
contentType: 'terminal',
label: 'c',
customLabel: null,
color: null,
sortOrder: 2,
createdAt: 3
}
]
},
groupsByWorktree: {
[WT]: [
{
id: groupId,
worktreeId: WT,
activeTabId: 'b',
tabOrder: ['a', 'b', 'c'],
recentTabIds: ['b']
}
]
},
activeGroupIdByWorktree: { [WT]: groupId }
})
store.getState().closeUnifiedTab('b')
// MRU only contains 'b' itself, so fallback picks the right neighbor 'c'.
expect(store.getState().groupsByWorktree[WT][0].activeTabId).toBe('c')
})
it('tracks an independent MRU history per tab group', () => {
const t1 = store.getState().createUnifiedTab(WT, 'terminal')
const sourceGroupId = store.getState().groupsByWorktree[WT][0].id
const secondGroupId = store.getState().createEmptySplitGroup(WT, sourceGroupId, 'right')
expect(secondGroupId).toBeTruthy()
// Create two tabs in the second (right) group and visit them in order.
const t2 = store.getState().createUnifiedTab(WT, 'terminal', {
targetGroupId: secondGroupId!
})
const t3 = store.getState().createUnifiedTab(WT, 'terminal', {
targetGroupId: secondGroupId!
})
// Second group's MRU tail should be t3.
// Switch focus to the source group so subsequent activations in the
// source group don't pollute the second group's MRU.
store.getState().activateTab(t1.id)
// Re-focus second group by activating t2, then close t2 — we expect the
// previous tab within the same group (t3), not a neighbor from the
// source group.
store.getState().activateTab(t3.id)
store.getState().activateTab(t2.id)
store.getState().closeUnifiedTab(t2.id)
const secondGroup = store.getState().groupsByWorktree[WT].find((g) => g.id === secondGroupId)
expect(secondGroup?.activeTabId).toBe(t3.id)
// Source group's active tab must remain untouched.
const sourceGroup = store.getState().groupsByWorktree[WT].find((g) => g.id === sourceGroupId)
expect(sourceGroup?.activeTabId).toBe(t1.id)
})
})
// ─── activateTab ──────────────────────────────────────────────────
+82 -14
View File
@@ -20,9 +20,11 @@ import {
findTabAndWorktree,
findTabByEntityInGroup,
patchTab,
pickNeighbor,
pickNextActiveTab,
pushRecentTabId,
sanitizeRecentTabIds,
updateGroup
} from './tabs-helpers'
} from './tab-group-state'
import { buildHydratedTabState } from './tabs-hydration'
import { buildOrphanTerminalCleanupPatch, getOrphanTerminalIds } from './terminal-orphan-helpers'
@@ -432,6 +434,14 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
}
nextOrder = dedupeTabOrder([...nextOrder, created.id])
// Why: creating a tab implicitly activates it, so extend the group's MRU
// stack with the new id. Keeping MRU updates colocated with activation
// writes preserves the invariant that `activeTabId` equals the tail of
// `recentTabIds` for any tab we've actually seen.
const nextRecent = pushRecentTabId(
sanitizeRecentTabIds(group.recentTabIds, nextOrder),
created.id
)
return {
unifiedTabsByWorktree: {
...state.unifiedTabsByWorktree,
@@ -442,7 +452,8 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
[worktreeId]: updateGroup(groupsByWorktree[worktreeId] ?? [], {
...group,
activeTabId: created.id,
tabOrder: nextOrder
tabOrder: nextOrder,
recentTabIds: nextRecent
})
},
activeGroupIdByWorktree,
@@ -492,7 +503,20 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
groupsByWorktree: {
...state.groupsByWorktree,
[worktreeId]: (state.groupsByWorktree[worktreeId] ?? []).map((group) =>
group.id === tab.groupId ? { ...group, activeTabId: tabId } : group
group.id === tab.groupId
? {
...group,
activeTabId: tabId,
// Why: MRU tracks every activation within the group so
// closeUnifiedTab can jump back to the previous tab instead
// of the visual neighbor. Sanitize first to prune ids from
// removed tabs that may have lingered in persisted state.
recentTabIds: pushRecentTabId(
sanitizeRecentTabIds(group.recentTabIds, group.tabOrder),
tabId
)
}
: group
)
},
activeGroupIdByWorktree: {
@@ -518,12 +542,20 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
const dedupedGroupOrder = dedupeTabOrder(group.tabOrder)
const remainingOrder = dedupeTabOrder(dedupedGroupOrder.filter((id) => id !== tabId))
const wasLastTab = remainingOrder.length === 0
// Why: when closing the active tab, walk the group's MRU stack back to the
// previously-active tab instead of the visual neighbor. `pickNextActiveTab`
// falls back to pickNeighbor when the MRU is empty (hydrated sessions,
// never-visited siblings) so behavior degrades gracefully.
const nextActiveTabId =
group.activeTabId === tabId
? wasLastTab
? null
: pickNeighbor(dedupedGroupOrder, tabId)
: pickNextActiveTab(dedupedGroupOrder, group.recentTabIds, tabId)
: group.activeTabId
const nextRecentTabIds = sanitizeRecentTabIds(
(group.recentTabIds ?? []).filter((id) => id !== tabId),
remainingOrder
)
set((current) => {
const nextTabs = (current.unifiedTabsByWorktree[worktreeId] ?? []).filter(
@@ -531,7 +563,12 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
)
let nextGroups = (current.groupsByWorktree[worktreeId] ?? []).map((candidate) =>
candidate.id === group.id
? { ...candidate, activeTabId: nextActiveTabId, tabOrder: remainingOrder }
? {
...candidate,
activeTabId: nextActiveTabId,
tabOrder: remainingOrder,
recentTabIds: nextRecentTabIds
}
: candidate
)
let nextLayoutByWorktree = current.layoutByWorktree
@@ -872,22 +909,34 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
...state.activeGroupIdByWorktree,
[worktreeId]: opts?.activate ? targetGroupId : state.activeGroupIdByWorktree[worktreeId]
}
const sourceRecentTabIds = sanitizeRecentTabIds(
(sourceGroup.recentTabIds ?? []).filter((id) => id !== tabId),
sourceOrder
)
const nextGroups = (state.groupsByWorktree[worktreeId] ?? []).map((group) => {
if (group.id === sourceGroup.id) {
return {
...group,
activeTabId:
group.activeTabId === tabId
? pickNeighbor(dedupedSourceGroupOrder, tabId)
? // Why: when the moved tab was active in the source, keep
// MRU-aware selection so the user lands on their previously
// focused tab rather than a visual neighbor.
pickNextActiveTab(dedupedSourceGroupOrder, sourceGroup.recentTabIds, tabId)
: group.activeTabId,
tabOrder: sourceOrder
tabOrder: sourceOrder,
recentTabIds: sourceRecentTabIds
}
}
if (group.id === targetGroupId) {
const sanitizedTargetRecent = sanitizeRecentTabIds(group.recentTabIds, targetOrder)
return {
...group,
activeTabId: opts?.activate ? tabId : group.activeTabId,
tabOrder: targetOrder
tabOrder: targetOrder,
recentTabIds: opts?.activate
? pushRecentTabId(sanitizedTargetRecent, tabId)
: sanitizedTargetRecent
}
}
return group
@@ -991,22 +1040,34 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
)
targetOrder.splice(targetIndex, 0, tabId)
const sourceRecentTabIds = sanitizeRecentTabIds(
(sourceGroup.recentTabIds ?? []).filter((id) => id !== tabId),
sourceOrder
)
nextGroups = nextGroups.map((group) => {
if (group.id === sourceGroup.id) {
return {
...group,
activeTabId:
group.activeTabId === tabId
? pickNeighbor(dedupedSourceGroupOrder, tabId)
? // Why: same MRU-aware fallback as moveUnifiedTabToGroup so
// the pane left behind by a drag keeps the user on their
// previously-active tab.
pickNextActiveTab(dedupedSourceGroupOrder, sourceGroup.recentTabIds, tabId)
: group.activeTabId,
tabOrder: sourceOrder
tabOrder: sourceOrder,
recentTabIds: sourceRecentTabIds
}
}
if (group.id === resolvedTargetGroupId) {
return {
...group,
activeTabId: tabId,
tabOrder: targetOrder
tabOrder: targetOrder,
recentTabIds: pushRecentTabId(
sanitizeRecentTabIds(group.recentTabIds, targetOrder),
tabId
)
}
}
return group
@@ -1222,9 +1283,16 @@ export const createTabsSlice: StateCreator<AppState, [], [], TabsSlice> = (set,
const tabOrderUnchanged =
tabOrder.length === group.tabOrder.length &&
tabOrder.every((tabId, index) => tabId === group.tabOrder[index])
return tabOrderUnchanged && activeTabId === group.activeTabId
// Why: reconciliation can drop backing tabs (stale persisted ids, dead
// PTYs, closed editor files). Keep the MRU stack in sync so the next
// close doesn't try to activate a tab the renderer no longer owns.
const recentTabIds = sanitizeRecentTabIds(group.recentTabIds, tabOrder)
const recentUnchanged =
recentTabIds.length === (group.recentTabIds ?? []).length &&
recentTabIds.every((id, index) => id === (group.recentTabIds ?? [])[index])
return tabOrderUnchanged && activeTabId === group.activeTabId && recentUnchanged
? group
: { ...group, tabOrder, activeTabId }
: { ...group, tabOrder, activeTabId, recentTabIds }
})
const currentActiveGroupId =
+7
View File
@@ -99,6 +99,13 @@ export type TabGroup = {
worktreeId: string
activeTabId: string | null
tabOrder: string[] // canonical visual order of tab IDs
/** Per-group MRU stack (oldest → most-recent at the tail). Drives which tab
* becomes active when the current active tab closes: we pop back to the
* previously-active tab instead of jumping to a visual neighbor. Scoped to
* the group so split panes keep independent histories. Optional because
* sessions persisted before this field was added still hydrate cleanly —
* hydration seeds from activeTabId. */
recentTabIds?: string[]
}
// ─── Terminal Tab (legacy — used by persistence and TerminalContentSlice) ─
+2 -1
View File
@@ -87,7 +87,8 @@ const tabGroupSchema = z.object({
id: z.string(),
worktreeId: z.string(),
activeTabId: z.string().nullable(),
tabOrder: z.array(z.string())
tabOrder: z.array(z.string()),
recentTabIds: z.array(z.string()).optional()
})
const tabGroupSplitDirectionSchema = z.enum(['horizontal', 'vertical'])