From 4f5d464ca652d5b5e8a94feae490684e2c27a7fb Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sat, 17 Oct 2026 02:15:39 -0700 Subject: [PATCH] Open into the window's emptied pane beside a split branch in one layout command (dark, layout-owner build only) (#30039) * Open into the window's emptied root beside a split branch in one layout command (dark, layout-owner build only) A tab opened into a group the window kept empty reaches main with the split that places it, now also when the group's neighbour is itself split (named by the branch's first and last groups) or is another empty group main lacks. Main makes the group with its tab beside that branch. Switch-off builds send no such command. * Comment the refusal of a branch main's tree does not hold (dark) --- .../runtime/layout-owner-new-group-split.ts | 37 +++++--- .../store-open-to-side-layout-owner.test.ts | 89 +++++++++++++++++-- .../rpc-contract/workspace-layout-params.ts | 1 + .../workspace-layout/tab-group-layout-tree.ts | 46 ++++++++++ .../workspace-layout-command-types.ts | 10 ++- .../workspace-layout-group-commands.ts | 14 ++- ...workspace-layout-open-in-new-group.test.ts | 39 ++++++++ 7 files changed, 212 insertions(+), 24 deletions(-) diff --git a/src/renderer/src/runtime/layout-owner-new-group-split.ts b/src/renderer/src/runtime/layout-owner-new-group-split.ts index 63a49e01915..4b04e43518e 100644 --- a/src/renderer/src/runtime/layout-owner-new-group-split.ts +++ b/src/renderer/src/runtime/layout-owner-new-group-split.ts @@ -1,13 +1,14 @@ import type { TabGroupLayoutNode } from '../../../shared/tab-types' +import { removeGroupLayoutLeaf } from '../../../shared/workspace-layout/tab-group-layout-tree' import type { NewGroupSplit } from '../../../shared/workspace-layout/workspace-layout-command-types' import type { TabsSlice } from '@/store/slices/tabs/tabs-slice-contract' /** * The split that made a just-opened tab's group, when the group holds no other tab. Main stores no - * empty group, so it lacks that group (the window made it with the tab, or empty a moment before) - * and makes it with the tab, beside the sibling the window split. None for a workspace's only - * group, or a sibling that is itself split, which the window's split never makes. `state` may - * predate the tab (an editor open passes the store from before it). + * empty group, so it lacks that group (the window made it with the tab, or empty before) and makes + * it with the tab, beside the sibling branch, read off the window's tree without the other empty + * groups main lacks too. None for what is then the workspace's only group. `state` may predate + * the tab (an editor open passes the store from before it). */ export function newGroupSplitOf( state: Pick, @@ -15,12 +16,19 @@ export function newGroupSplitOf( groupId: string, tabId: string ): { split: NewGroupSplit } | null { - const group = state.groupsByWorktree[worktreeId]?.find((entry) => entry.id === groupId) + const groups = state.groupsByWorktree[worktreeId] ?? [] + const group = groups.find((entry) => entry.id === groupId) const layout = state.layoutByWorktree[worktreeId] if (!group || group.tabOrder.some((id) => id !== tabId) || !layout) { return null } - return splitBeside(layout, groupId) + let held: TabGroupLayoutNode | null = layout + for (const other of groups) { + if (held && other.id !== groupId && other.tabOrder.length === 0) { + held = removeGroupLayoutLeaf(held, other.id) + } + } + return held ? splitBeside(held, groupId) : null } function splitBeside(node: TabGroupLayoutNode, groupId: string): { split: NewGroupSplit } | null { @@ -33,10 +41,19 @@ function splitBeside(node: TabGroupLayoutNode, groupId: string): { split: NewGro return splitBeside(node.first, groupId) ?? splitBeside(node.second, groupId) } const sibling = isFirst ? node.second : node.first - if (sibling.type !== 'leaf') { - return null - } const sides = node.direction === 'horizontal' ? (['left', 'right'] as const) : (['up', 'down'] as const) - return { split: { besideGroupId: sibling.groupId, direction: sides[isFirst ? 0 : 1] } } + const besideGroupId = edgeGroupId(sibling, 'first') + const besideLastGroupId = edgeGroupId(sibling, 'second') + return { + split: { + besideGroupId, + ...(besideLastGroupId !== besideGroupId ? { besideLastGroupId } : {}), + direction: sides[isFirst ? 0 : 1] + } + } +} + +function edgeGroupId(node: TabGroupLayoutNode, edge: 'first' | 'second'): string { + return node.type === 'leaf' ? node.groupId : edgeGroupId(node[edge], edge) } diff --git a/src/renderer/src/store/slices/store-open-to-side-layout-owner.test.ts b/src/renderer/src/store/slices/store-open-to-side-layout-owner.test.ts index 621fbd24838..b4ec778c8fc 100644 --- a/src/renderer/src/store/slices/store-open-to-side-layout-owner.test.ts +++ b/src/renderer/src/store/slices/store-open-to-side-layout-owner.test.ts @@ -3,6 +3,7 @@ import type * as AgentStatusModule from '@/lib/agent-status' import { createTestStore, makeWorktree, seedStore } from './store-test-helpers' import { createStoreCascadesMockApi } from './store-cascades-test-harness' import { newGroupSplitOf } from '@/runtime/layout-owner-new-group-split' +import { recordOpenedAgentSessionTabInLayoutOwner } from '@/runtime/agent-session-tab-layout-owner' import type { TabGroupLayoutNode } from '../../../../shared/tab-types' import { LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' import { LayoutApplyParams } from '../../../../shared/rpc-contract/workspace-layout-params' @@ -81,10 +82,17 @@ function applySent() { return commands } +/** Groups by id: the detector matches them by id, and main appends a group the window kept. */ +function byId(groups: readonly { id: string; tabOrder: string[] }[]) { + return groups + .map(({ id, tabOrder }) => ({ id, tabOrder })) + .sort((left, right) => left.id.localeCompare(right.id)) +} + function windowView() { const state = store.getState() return { - groups: (state.groupsByWorktree[WT] ?? []).map(({ id, tabOrder }) => ({ id, tabOrder })), + groups: byId(state.groupsByWorktree[WT] ?? []), tree: splitTree(state.layoutByWorktree[WT]) } } @@ -92,7 +100,7 @@ function windowView() { function mainView() { const workspace = main.workspaces[WT]! return { - groups: workspace.groups.map(({ id, tabOrder }) => ({ id, tabOrder })), + groups: byId(workspace.groups), tree: splitTree(workspace.groupLayout) } } @@ -180,11 +188,72 @@ describe('open to the side in a layout-owner build', () => { applySent() // The window keeps its empty root beside it; main stores no empty group. - expect(windowView().groups).toEqual([ - { id: ROOT, tabOrder: [] }, - { id: editorGroup, tabOrder: [onlyTabOf(editorGroup)] } - ]) - expect(mainView()).toEqual({ groups: [windowView().groups[1]], tree: null }) + expect(windowView().groups).toEqual( + byId([ + { id: ROOT, tabOrder: [] }, + { id: editorGroup, tabOrder: [onlyTabOf(editorGroup)] } + ]) + ) + expect(mainView()).toEqual({ + groups: [{ id: editorGroup, tabOrder: [onlyTabOf(editorGroup)] }], + tree: null + }) + }) + + it.each([ + ['terminal', () => store.getState().createTab(WT, ROOT)], + ['editor', () => openFile('/c.ts', { targetGroupId: ROOT })], + [ + 'browser', + () => store.getState().createBrowserTab(WT, 'about:blank', { targetGroupId: ROOT }) + ], + [ + 'chat', + () => { + // As openStructuredAgentSessionProvisionalTab does, on this test's store. + const tab = store.getState().createUnifiedTab(WT, 'agent-session', { + entityId: 'chat-1', + agentSessionAgent: 'codex', + targetGroupId: ROOT + }) + recordOpenedAgentSessionTabInLayoutOwner(store.getState(), tab) + } + ] + ])( + 'records a %s tab opened into the emptied root after its neighbour split again', + (_kind, openIntoRoot) => { + // The window keeps its emptied root; the splits beside it hold tabs, so main has only those. + store.getState().createTab(WT, undefined, undefined, { + split: { sourceGroupId: ROOT, splitDirection: 'right' } + }) + const right = store.getState().groupsByWorktree[WT]!.at(-1)!.id + store.getState().createTab(WT, undefined, undefined, { + split: { sourceGroupId: right, splitDirection: 'down' } + }) + applySent() + expect(windowView().groups).toContainEqual({ id: ROOT, tabOrder: [] }) + + openIntoRoot() + applySent() + expect(mainView()).toEqual(windowView()) + } + ) + + it('records opens into a split made off another empty split, beside the groups main holds', () => { + openFile('/a.ts') + const outer = store.getState().createEmptySplitGroup(WT, ROOT, 'right')! + const inner = store.getState().createEmptySplitGroup(WT, outer, 'down')! + applySent() + + // The inner group's sibling is the outer one, which main lacks until its own first tab. + openFile('/b.ts', { targetGroupId: inner }) + applySent() + store.getState().closeEmptyGroup(WT, outer) + expect(mainView()).toEqual(windowView()) + const again = store.getState().createEmptySplitGroup(WT, inner, 'up')! + openFile('/c.ts', { targetGroupId: again }) + applySent() + expect(mainView()).toEqual(windowView()) }) it('sends no split for a tab that joins a group holding others, or the only group', () => { @@ -210,7 +279,7 @@ describe('open to the side in a layout-owner build', () => { }) describe('newGroupSplitOf', () => { - it('finds no split beside a sibling that is itself split, so main gets none', () => { + it('names a sibling that is itself split by its first and last groups', () => { const group = (id: string, tabOrder: string[]) => ({ id, worktreeId: WT, @@ -234,6 +303,8 @@ describe('newGroupSplitOf', () => { } } - expect(newGroupSplitOf(state, WT, 'N', 'n')).toBeNull() + expect(newGroupSplitOf(state, WT, 'N', 'n')).toEqual({ + split: { besideGroupId: 'A', besideLastGroupId: 'M', direction: 'right' } + }) }) }) diff --git a/src/shared/rpc-contract/workspace-layout-params.ts b/src/shared/rpc-contract/workspace-layout-params.ts index 7df8da33d25..01fcca18532 100644 --- a/src/shared/rpc-contract/workspace-layout-params.ts +++ b/src/shared/rpc-contract/workspace-layout-params.ts @@ -116,6 +116,7 @@ const CloseTabsCommand = z.object({ const NewGroupSplit = z .object({ besideGroupId: z.string().min(1), + besideLastGroupId: z.string().min(1).optional(), direction: z.enum(['left', 'right', 'up', 'down']) }) .optional() diff --git a/src/shared/workspace-layout/tab-group-layout-tree.ts b/src/shared/workspace-layout/tab-group-layout-tree.ts index fe9ae87651a..585d644c08c 100644 --- a/src/shared/workspace-layout/tab-group-layout-tree.ts +++ b/src/shared/workspace-layout/tab-group-layout-tree.ts @@ -143,3 +143,49 @@ export function moveGroupLeafBeside( buildSplitNode(besideGroupId, groupId, direction, position) ) } + +function holdsGroup(node: TabGroupLayoutNode, groupId: string): boolean { + return node.type === 'leaf' + ? node.groupId === groupId + : holdsGroup(node.first, groupId) || holdsGroup(node.second, groupId) +} + +/** + * Moves a group's leaf to `side` of the smallest branch holding both `fromGroupId` and + * `toGroupId` (one group's leaf when they are the same); null when no branch holds both. + */ +export function moveGroupLeafBesideBranch( + root: TabGroupLayoutNode, + groupId: string, + fromGroupId: string, + toGroupId: string, + side: 'left' | 'right' | 'up' | 'down' +): TabGroupLayoutNode | null { + const leaf: TabGroupLayoutNode = { type: 'leaf', groupId } + const direction = side === 'left' || side === 'right' ? 'horizontal' : 'vertical' + const leafFirst = side === 'left' || side === 'up' + const besideBranch = (node: TabGroupLayoutNode): TabGroupLayoutNode | null => { + if (!holdsGroup(node, fromGroupId) || !holdsGroup(node, toGroupId)) { + return null + } + if (node.type === 'split') { + const first = besideBranch(node.first) + if (first) { + return { ...node, first } + } + const second = besideBranch(node.second) + if (second) { + return { ...node, second } + } + } + return { + type: 'split', + direction, + first: leafFirst ? leaf : node, + second: leafFirst ? node : leaf, + ratio: 0.5 + } + } + const rest = removeGroupLayoutLeaf(root, groupId) + return rest && besideBranch(rest) +} diff --git a/src/shared/workspace-layout/workspace-layout-command-types.ts b/src/shared/workspace-layout/workspace-layout-command-types.ts index 0cfa7754d61..8455c49d895 100644 --- a/src/shared/workspace-layout/workspace-layout-command-types.ts +++ b/src/shared/workspace-layout/workspace-layout-command-types.ts @@ -213,9 +213,15 @@ export type GroupSide = 'left' | 'right' | 'up' | 'down' /** * An open into a new group, named by the command's `groupId`, split off `besideGroupId`: the - * group and its first tab are made in one step, so no empty group is ever stored. + * group and its first tab are made in one step, so no empty group is ever stored. With + * `besideLastGroupId`, the split is off the branch from `besideGroupId` to it (its first and last + * groups), as when a group the window kept empty sits beside a branch split since. */ -export type NewGroupSplit = { besideGroupId: string; direction: GroupSide } +export type NewGroupSplit = { + besideGroupId: string + besideLastGroupId?: string + direction: GroupSide +} export type TabTitle = | { kind: 'custom' | 'generated'; title: string | null } diff --git a/src/shared/workspace-layout/workspace-layout-group-commands.ts b/src/shared/workspace-layout/workspace-layout-group-commands.ts index f9295797829..6dcf24fc9fb 100644 --- a/src/shared/workspace-layout/workspace-layout-group-commands.ts +++ b/src/shared/workspace-layout/workspace-layout-group-commands.ts @@ -5,7 +5,7 @@ import type { TabGroupLayoutNode } from '../tab-types' import { buildHeadlessTabGroupMove, buildHeadlessTabGroupSplit } from './tab-group-moves' -import { moveGroupLeafBeside } from './tab-group-layout-tree' +import { moveGroupLeafBeside, moveGroupLeafBesideBranch } from './tab-group-layout-tree' import { applied, findTab, @@ -173,20 +173,28 @@ export function openInNewGroup( if (workspaces.some((entry) => entry.groups.some((group) => group.id === groupId))) { return refuse('group_exists') } - const besideHeld = workspace.groups.some((group) => group.id === split.besideGroupId) + const lastGroupId = split.besideLastGroupId ?? split.besideGroupId + const besideHeld = [split.besideGroupId, lastGroupId].every((id) => + workspace.groups.some((group) => group.id === id) + ) // Beside a group main lacks: refused, unless main has no group here at all (the window split off // its emptied root), where the new group is the only one. if (!besideHeld && workspace.groups.length > 0) { return refuse('group_not_found') } const groupLayout = besideHeld - ? moveGroupLeafBeside( + ? moveGroupLeafBesideBranch( workspace.groupLayout ?? { type: 'leaf', groupId: split.besideGroupId }, groupId, split.besideGroupId, + lastGroupId, split.direction ) : undefined + // Both groups held, but no branch of main's tree holds them both. + if (besideHeld && !groupLayout) { + return refuse('invalid_params') + } const opened = open( withWorkspace(model, command.workspace, { ...workspace, diff --git a/src/shared/workspace-layout/workspace-layout-open-in-new-group.test.ts b/src/shared/workspace-layout/workspace-layout-open-in-new-group.test.ts index 493172604b2..23b0652087c 100644 --- a/src/shared/workspace-layout/workspace-layout-open-in-new-group.test.ts +++ b/src/shared/workspace-layout/workspace-layout-open-in-new-group.test.ts @@ -123,6 +123,36 @@ describe('opening into a new split', () => { }) }) + it('splits off a whole branch named by its first and last groups', () => { + const { ctx, model } = oneGroup() + const opened = build( + ctx, + [ + editor('/w/b.ts', { tabId: 'b', groupId: 'g2', split: right }), + editor('/w/c.ts', { + tabId: 'c', + groupId: 'g3', + split: { besideGroupId: 'g1', besideLastGroupId: 'g2', direction: 'up' } + }) + ], + model + ) + + expect(opened.model.workspaces[WS]!.groupLayout).toEqual({ + type: 'split', + direction: 'vertical', + first: { type: 'leaf', groupId: 'g3' }, + second: { + type: 'split', + direction: 'horizontal', + first: { type: 'leaf', groupId: 'g1' }, + second: { type: 'leaf', groupId: 'g2' }, + ratio: 0.5 + }, + ratio: 0.5 + }) + }) + it('refuses a split it cannot make, and changes nothing', () => { const { ctx, model } = oneGroup() const refusal = (command: LayoutCommand) => { @@ -139,6 +169,15 @@ describe('opening into a new split', () => { }) ) ).toBe('group_not_found') + expect( + refusal( + editor('/w/b.ts', { + tabId: 'b', + groupId: 'g2', + split: { besideGroupId: 'g1', besideLastGroupId: 'missing', direction: 'right' } + }) + ) + ).toBe('group_not_found') // The group id another workspace holds. const other = build( ctx,