feat: MRU tab close behavior with file rename and naming guidance

- Switch tab-close from 'nearest neighbor' to most-recently-used (MRU) tab within the group
- Add recentTabIds: string[] to TabGroup (MRU stack, oldest→newest at tail)
- Add sanitizeRecentTabIds, pushRecentTabId, pickNextActiveTab helpers in tab-group-state
- Integrate MRU tracking in createUnifiedTab, activateTab, closeUnifiedTab, moveTab, hydration
- Rename tabs-helpers.ts → tab-group-state.ts (reflects actual content: tab-group state operations)
- Update AGENTS.md: forbid vague names like helpers/utils/common; use concrete domain concepts
- Persist recentTabIds via existing session write path; Zod schema validates on read
- Memory bounded: MRU length ≤ tab count (sanitizeRecentTabIds filters against tabOrder)
- All 83 tests passing
This commit is contained in:
Neil 2026-04-17 18:17:23 -07:00
parent 9826d94485
commit 73640b02f5
11 changed files with 372 additions and 26 deletions

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.

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

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) =>

View file

@ -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: [] }

View file

@ -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))
}

View file

@ -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' })

View file

@ -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 }
}

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 ──────────────────────────────────────────────────

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 =

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) ─

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'])