diff --git a/packages/app/src/stores/workspace-layout-actions.ts b/packages/app/src/stores/workspace-layout-actions.ts index e4724f2f3..045c9c33f 100644 --- a/packages/app/src/stores/workspace-layout-actions.ts +++ b/packages/app/src/stores/workspace-layout-actions.ts @@ -468,6 +468,21 @@ function findGroupPathById( return null; } +/** + * The group holding the node at `targetPath`, or null when that node is the whole tree. + * + * Ask `targetPath`, never the parent path: `targetPath.slice(0, -1)` is empty both for "this node + * is the root" and for "this node is a direct child of the root group". Treating the second case as + * parentless wraps root-level panes in a redundant group on every split, which reparents the + * existing pane and remounts its whole subtree. + */ +function findParentGroup(root: SplitNodeInternal, targetPath: number[]): SplitNodeInternal | null { + if (targetPath.length === 0) { + return null; + } + return getNodeAtPath(root, targetPath.slice(0, -1)); +} + function getNodeAtPath(node: SplitNodeInternal, path: number[]): SplitNodeInternal { let current = node; for (const index of path) { @@ -869,7 +884,7 @@ function insertSplitInternal(input: InsertSplitInternalInput): InsertSplitIntern const parentPath = targetPath.slice(0, -1); const targetIndex = targetPath[targetPath.length - 1] ?? 0; - const parentNode = parentPath.length > 0 ? getNodeAtPath(detached.root, parentPath) : null; + const parentNode = findParentGroup(detached.root, targetPath); if (parentNode?.kind === "group" && parentNode.group.direction === direction) { const targetSize = parentNode.group.sizes[targetIndex] ?? 0; @@ -1448,7 +1463,7 @@ export function splitPaneEmptyInLayout( const parentPath = targetPath.slice(0, -1); const targetIndex = targetPath[targetPath.length - 1] ?? 0; - const parentNode = parentPath.length > 0 ? getNodeAtPath(layout.root, parentPath) : null; + const parentNode = findParentGroup(layout.root, targetPath); let nextRoot: SplitNodeInternal; if (parentNode?.kind === "group" && parentNode.group.direction === direction) { diff --git a/packages/app/src/stores/workspace-layout-store.test.ts b/packages/app/src/stores/workspace-layout-store.test.ts index 71d3e7d95..e43672f3d 100644 --- a/packages/app/src/stores/workspace-layout-store.test.ts +++ b/packages/app/src/stores/workspace-layout-store.test.ts @@ -218,11 +218,8 @@ describe("workspace-layout-store tree transforms", () => { workspaceLayoutIds.reset(); }); - it("insertSplit wraps root-level same-direction splits in a nested group", () => { - useWorkspaceLayoutIds( - "11111111-1111-1111-1111-111111111111", - "22222222-2222-2222-2222-222222222222", - ); + it("insertSplit adds root-level same-direction splits as a flat sibling", () => { + useWorkspaceLayoutIds("11111111-1111-1111-1111-111111111111"); const root: SplitNode = { kind: "group", @@ -239,14 +236,12 @@ describe("workspace-layout-store tree transforms", () => { const nextRoot = insertSplit(root, "right", "tab-c", "right", workspaceLayoutIds.createNodeId); const nextGroup = expectGroup(nextRoot); - const nestedGroup = expectGroup(nextGroup.group.children[1]); + expect(nextGroup.group.id).toBe("group-root"); expect(nextGroup.group.direction).toBe("horizontal"); - expect(nextGroup.group.children).toHaveLength(2); - expect(nextGroup.group.sizes).toEqual([0.25, 0.75]); - expect(nestedGroup.group.id).toBe("group_22222222-2222-2222-2222-222222222222"); - expect(nestedGroup.group.direction).toBe("horizontal"); - expect(nestedGroup.group.sizes).toEqual([0.5, 0.5]); + expect(nextGroup.group.children).toHaveLength(3); + expect(nextGroup.group.sizes).toEqual([0.25, 0.375, 0.375]); + expect(getTreeDepth(nextRoot)).toBe(getTreeDepth(root)); expect(collectAllPanes(nextRoot).map((pane) => pane.id)).toEqual([ "left", "right", @@ -258,6 +253,91 @@ describe("workspace-layout-store tree transforms", () => { ]); }); + /** + * The renderer keys panes by id and reconciles them positionally, so a pane that changes render + * path is unmounted and rebuilt — the composer, terminal, and scroll state in it are lost. A + * same-direction split has no reason to move anything: it appends a sibling into the group that + * is already running in that direction. Perpendicular splits are excluded because nesting is the + * only way to change axis. + */ + describe("same-direction splits never move an existing pane", () => { + function panePathsById(root: SplitNode, path: string[] = []): Map { + if (root.kind === "pane") { + return new Map([[root.pane.id, path.join("/")]]); + } + const paths = new Map(); + for (const child of root.group.children) { + const childKey = child.kind === "pane" ? child.pane.id : child.group.id; + for (const [paneId, childPath] of panePathsById(child, [...path, childKey])) { + paths.set(paneId, childPath); + } + } + return paths; + } + + function withPaneTabs(root: SplitNode, paneId: string, tabIds: string[]): SplitNode { + if (root.kind === "pane") { + return root.pane.id === paneId ? createPane({ id: paneId, tabIds }) : root; + } + return { + kind: "group", + group: { + ...root.group, + children: root.group.children.map((child) => withPaneTabs(child, paneId, tabIds)), + }, + }; + } + + function expectExistingPanesUnmoved(before: SplitNode, after: SplitNode) { + const beforePaths = panePathsById(before); + const afterPaths = panePathsById(after); + for (const [paneId, panePath] of beforePaths) { + expect(afterPaths.get(paneId)).toBe(panePath); + } + expect(getTreeDepth(after)).toBe(getTreeDepth(before)); + } + + for (const position of ["left", "right"] as const) { + it(`keeps every pane in place when a tab is dragged ${position} from the default layout`, () => { + workspaceLayoutIds.reset(); + const mainPaneId = collectAllPanes(createWorkspaceLayoutWithExplorer().root)[0]?.id; + expect(mainPaneId).toBeTruthy(); + const root = withPaneTabs(createWorkspaceLayoutWithExplorer().root, mainPaneId as string, [ + "tab-a", + "tab-b", + ]); + + const nextRoot = insertSplit( + root, + mainPaneId as string, + "tab-a", + position, + workspaceLayoutIds.createNodeId, + ); + + expectExistingPanesUnmoved(root, nextRoot); + }); + } + + it("keeps every pane in place when splitPaneEmpty opens the first split pane", () => { + workspaceLayoutIds.reset(); + const workspaceKey = createWorkspaceKey(); + const store = workspaceLayoutStore.getState(); + const before = createWorkspaceLayoutWithExplorer(); + const mainPaneId = collectAllPanes(before.root)[0]?.id; + expect(mainPaneId).toBeTruthy(); + + const newPaneId = store.splitPaneEmpty(workspaceKey, { + targetPaneId: mainPaneId as string, + position: "right", + }); + + expect(newPaneId).not.toBeNull(); + const after = workspaceLayoutStore.getState().layoutByWorkspace[workspaceKey]; + expectExistingPanesUnmoved(before.root, after.root); + }); + }); + it("removePaneFromTree unwraps single-child groups and renormalizes siblings", () => { const root: SplitNode = { kind: "group", @@ -1357,10 +1437,12 @@ describe("workspace-layout-store actions", () => { }); const layout = workspaceLayoutStore.getState().layoutByWorkspace[workspaceKey]; + // The first split joins the root group instead of nesting under it, so it spends a pane id and + // no group id, and the four user splits all fit inside the depth cap. expect(pane1).toBe("pane_11111111-1111-1111-1111-111111111111"); - expect(pane2).toBe("pane_33333333-3333-3333-3333-333333333333"); - expect(pane3).toBe("pane_55555555-5555-5555-5555-555555555555"); - expect(pane4).toBeNull(); + expect(pane2).toBe("pane_22222222-2222-2222-2222-222222222222"); + expect(pane3).toBe("pane_44444444-4444-4444-4444-444444444444"); + expect(pane4).toBe("pane_66666666-6666-6666-6666-666666666666"); expect(getTreeDepth(layout.root)).toBe(5); }); @@ -1428,7 +1510,7 @@ describe("workspace-layout-store actions", () => { const layout = workspaceLayoutStore.getState().layoutByWorkspace[workspaceKey]; expect(paneBId).toBe("pane_78787878-7878-7878-7878-787878787878"); - expect(paneCId).toBe("pane_9a9a9a9a-9a9a-9a9a-9a9a-9a9a9a9a9a9a"); + expect(paneCId).toBe("pane_89898989-8989-8989-8989-898989898989"); expect(layout.focusedPaneId).toBe(paneCId); expect(findPaneById(layout.root, "main")?.tabIds).toEqual(["file_/repo/worktree/a.ts"]); expect(findPaneById(layout.root, paneCId)?.tabIds).toEqual(["file_/repo/worktree/c.ts"]); @@ -1577,7 +1659,7 @@ describe("workspace-layout-store actions", () => { const total = resizedNestedGroup.group.sizes.reduce((sum, size) => sum + size, 0); expect(rightPaneId).toBe("pane_eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"); - expect(farRightPaneId).toBe("pane_11111111-1111-1111-1111-111111111111"); + expect(farRightPaneId).toBe("pane_ffffffff-ffff-ffff-ffff-ffffffffffff"); expect(resizedNestedGroup.group.sizes[0]).toBeGreaterThanOrEqual(0.1); expect(resizedNestedGroup.group.sizes[1]).toBeGreaterThanOrEqual(0.1); expect(total).toBeCloseTo(1, 10);