diff --git a/apps/app/src/components/secondary-panel/SidebarSplitContainer.tsx b/apps/app/src/components/secondary-panel/SidebarSplitContainer.tsx index 5b045fcd3a..7b0851ac56 100644 --- a/apps/app/src/components/secondary-panel/SidebarSplitContainer.tsx +++ b/apps/app/src/components/secondary-panel/SidebarSplitContainer.tsx @@ -38,6 +38,7 @@ import { parseSidebarSplitState, pruneSidebarSplitStorage, reconcileSidebarSplitState, + removeSidebarSplit, reorderSidebarTab, replaceSidebarTab, resizeSidebarSplit, @@ -69,6 +70,7 @@ export interface SidebarSplitPaneRenderArgs { ) => void; onReorderTab: (request: SecondaryPanelTabReorderRequest) => void; onFocusPane: () => void; + onRemoveSplit?: () => void; onMoveActiveTabToSide?: (side: SplitSide) => void; onSelectTab: (tabId: string) => void; paneId: string; @@ -230,6 +232,13 @@ export function SidebarSplitContainer({ [commitState], ); + const removeSplit = useCallback( + (paneId: string) => { + commitState((current) => removeSidebarSplit(current, paneId), true); + }, + [commitState], + ); + const moveActiveTabToSide = useCallback( (side: SplitSide) => { commitState((current) => { @@ -298,6 +307,10 @@ export function SidebarSplitContainer({ if (event.button !== 0) return; const sourceGroup = getSidebarGroupForPane(state, sourcePaneId); const sourceElement = event.currentTarget; + const targetBoundary = sourceElement.closest("aside"); + // Sidebar tab splits only target their owning right panel. Fail closed if + // the shared container is ever rendered outside that product boundary. + if (targetBoundary === null) return; const chrome = sourceElement.closest( '[data-testid="thread-secondary-panel-top-chrome"]', ); @@ -308,9 +321,10 @@ export function SidebarSplitContainer({ beginSplitDrag({ ghostLabel: label, sourceEl: sourceElement, + targetBoundary, fallback: { paneId: sourcePaneId, - container: sourceElement.closest("aside"), + container: targetBoundary, }, cancelSidebarReorderOnEngage: true, shouldEngage: (x, y) => { @@ -399,6 +413,7 @@ export function SidebarSplitContainer({ beginTabDrag(firstPane.paneId, tabId, event), onReorderTab: (request) => reorderTab(firstPane.paneId, request), onFocusPane: () => focusPane(firstPane.paneId), + onRemoveSplit: undefined, onMoveActiveTabToSide: activeTabPositionHandler, onSelectTab: (tabId) => selectTab(firstPane.paneId, tabId), paneId: firstPane.paneId, @@ -423,6 +438,7 @@ export function SidebarSplitContainer({ state={state} onBeginTabDrag={beginTabDrag} onFocusPane={focusPane} + onRemoveSplit={removeSplit} onMoveActiveTabToSide={activeTabPositionHandler} onReorderTab={reorderTab} onResize={resize} @@ -450,6 +466,7 @@ interface SidebarSplitTreeProps { event: ReactPointerEvent, ) => void; onFocusPane: (paneId: string) => void; + onRemoveSplit: (paneId: string) => void; onMoveActiveTabToSide?: (side: SplitSide) => void; onReorderTab: ( paneId: string, @@ -553,6 +570,7 @@ function SidebarSplitLeaf( props.onBeginTabDrag(pane.paneId, tabId, event), onReorderTab: (request) => props.onReorderTab(pane.paneId, request), onFocusPane: () => props.onFocusPane(pane.paneId), + onRemoveSplit: () => props.onRemoveSplit(pane.paneId), onMoveActiveTabToSide: props.onMoveActiveTabToSide, onSelectTab: (tabId) => props.onSelectTab(pane.paneId, tabId), paneId: pane.paneId, diff --git a/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.collapseControl.test.tsx b/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.collapseControl.test.tsx index 78e9372878..5950abb70b 100644 --- a/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.collapseControl.test.tsx +++ b/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.collapseControl.test.tsx @@ -57,11 +57,12 @@ const infoAndDiffFixedTabs = [ function createTestRenderableTab( tab: SecondaryFileFixedPanelTab, renderContent: SecondaryPanelRenderableTab["renderContent"] = () => null, + onClose: () => void = noop, ): SecondaryPanelRenderableTab { return { label: "index.ts", leadingVisual: null, - onClose: noop, + onClose, onSelect: noop, renderContent, statusLabel: null, @@ -100,6 +101,58 @@ function renderPanel(args: { ); } +function renderFixedTabSplit({ + keyboardKey, +}: { + keyboardKey?: "Enter" | " "; +} = {}) { + const { wrapper: Wrapper } = createQueryClientTestHarness(); + const panelStateId = `fixed-tab-remove-split-${keyboardKey ?? "pointer"}`; + const initial = createSidebarSplitState( + [infoFixedTab.id, diffFixedTab.id], + diffFixedTab.id, + ); + const split = moveSidebarTab( + initial, + initial.layout.focusedPaneId, + diffFixedTab.id, + { paneId: initial.layout.focusedPaneId, zone: "right" }, + { groupId: "group-diff" }, + ); + window.localStorage.setItem( + sidebarSplitStorageKey(panelStateId), + serializeSidebarSplitState(split), + ); + + return render( + + + + + Thread metadata} + onClose={noop} + onCollapse={noop} + onTabReorder={noop} + onOpenNewTab={noop} + onPanelFocus={noop} + onToggleConversationCollapse={noop} + renderAsDrawer={false} + splitPanelStateId={panelStateId} + /> + + + + , + ); +} + describe("ThreadSecondaryPanel compact file content", () => { it("renders arbitrary fixed-tab content through the shared surface", () => { const { wrapper: Wrapper } = createQueryClientTestHarness(); @@ -272,6 +325,8 @@ describe("ThreadSecondaryPanel compact file content", () => { sidebarSplitStorageKey(panelStateId), serializeSidebarSplitState(split), ); + const closeFirstTab = vi.fn(); + const closeSecondTab = vi.fn(); render( @@ -295,15 +350,19 @@ describe("ThreadSecondaryPanel compact file content", () => { splitPanelStateId={panelStateId} tabs={[ { - ...createTestRenderableTab(firstTab, () => ( -
First tab body
- )), + ...createTestRenderableTab( + firstTab, + () =>
First tab body
, + closeFirstTab, + ), label: "first.ts", }, { - ...createTestRenderableTab(secondTab, () => ( -
Second tab body
- )), + ...createTestRenderableTab( + secondTab, + () =>
Second tab body
, + closeSecondTab, + ), label: "second.ts", }, ]} @@ -317,6 +376,12 @@ describe("ThreadSecondaryPanel compact file content", () => { expect(screen.getByText("First tab body")).toBeTruthy(); expect(screen.getByText("Second tab body")).toBeTruthy(); expect(document.querySelectorAll("[data-split-pane-id]")).toHaveLength(2); + expect( + screen.getAllByRole("button", { name: "Remove split" }), + ).toHaveLength(2); + fireEvent.click(screen.getByRole("button", { name: "Close first.ts" })); + expect(closeFirstTab).toHaveBeenCalledTimes(1); + expect(closeSecondTab).not.toHaveBeenCalled(); }); it("retains the active file body after the persistent drawer closes", () => { @@ -448,6 +513,73 @@ describe("ThreadSecondaryPanel compact file content", () => { }); }); +describe("ThreadSecondaryPanel remove-split control", () => { + it("is absent when unsplit and appears at the trailing edge of every split pane", () => { + const unsplit = renderPanel({ + isConversationCollapsed: false, + onToggleConversationCollapse: noop, + }); + expect(unsplit.queryByRole("button", { name: "Remove split" })).toBeNull(); + unsplit.unmount(); + + renderFixedTabSplit(); + + const panes = Array.from( + document.querySelectorAll("[data-split-pane-id]"), + ); + const removeControls = screen.getAllByRole("button", { + name: "Remove split", + }); + expect(panes).toHaveLength(2); + expect(removeControls).toHaveLength(2); + expect( + panes.every((pane) => { + const chrome = pane.querySelector( + '[data-testid="thread-secondary-panel-top-chrome"]', + ); + const removeControl = pane.querySelector('[aria-label="Remove split"]'); + return ( + chrome?.lastElementChild instanceof HTMLElement && + removeControl instanceof HTMLButtonElement && + chrome.lastElementChild.contains(removeControl) + ); + }), + ).toBe(true); + }); + + it.each(["Enter", " "] as const)( + "keeps Info and Diff open when removing their split with %j", + (key) => { + renderFixedTabSplit({ keyboardKey: key }); + + const removeControl = screen.getAllByRole("button", { + name: "Remove split", + })[1]; + expect(removeControl).toBeInstanceOf(HTMLButtonElement); + if (!(removeControl instanceof HTMLButtonElement)) return; + removeControl.focus(); + expect(document.activeElement).toBe(removeControl); + expect(removeControl.tabIndex).toBe(0); + + // jsdom does not synthesize a button's browser-default click from key + // events. Dispatch the key pair, then the detail=0 click a browser emits + // for keyboard activation so this still exercises the native button path. + fireEvent.keyDown(removeControl, { key }); + fireEvent.keyUp(removeControl, { key }); + fireEvent.click(removeControl, { detail: 0 }); + + expect(document.querySelectorAll("[data-split-pane-id]")).toHaveLength(0); + expect(screen.queryByRole("button", { name: "Remove split" })).toBeNull(); + expect( + screen.getByRole("button", { name: "Show thread info panel" }), + ).toBeTruthy(); + expect( + screen.getByRole("button", { name: "Show diff panel" }), + ).toBeTruthy(); + }, + ); +}); + describe("ThreadSecondaryPanel Diff eligibility", () => { it("falls back from an ineligible active Diff tab to Info", () => { const { wrapper: Wrapper } = createQueryClientTestHarness(); diff --git a/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.tsx b/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.tsx index ae071bda39..5bfcc3feec 100644 --- a/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.tsx +++ b/apps/app/src/components/secondary-panel/ThreadSecondaryPanel.tsx @@ -560,6 +560,7 @@ export function ThreadSecondaryPanel({ event: ReactPointerEvent, ) => void; onMoveActiveTabToSide?: (side: SplitSide) => void; + onRemoveSplit?: () => void; onFocusPane: () => void; onSurfaceTabReorder: SecondaryPanelTabReorderHandler; paneId: string | null; @@ -609,6 +610,25 @@ export function ThreadSecondaryPanel({ ); + const renderRemoveSplitButton = (onRemoveSplit?: () => void) => + onRemoveSplit ? ( + + ) : null; + const renderConversationCollapseButton = ({ onMoveActiveTabToSide, usesPaneArrangementControl, @@ -734,6 +754,7 @@ export function ThreadSecondaryPanel({ onBeginTabDrag, onFocusPane, onMoveActiveTabToSide, + onRemoveSplit, onSurfaceTabReorder, paneId, reserveLeadingChrome, @@ -812,24 +833,29 @@ export function ThreadSecondaryPanel({ showNewTabButton: showNewTabControl, })} - {showOuterControls ? ( + {showOuterControls || onRemoveSplit ? (
event.stopPropagation()} > - {renderConversationCollapseButton({ - onMoveActiveTabToSide, - usesPaneArrangementControl, - })} - {renderAsDrawer || inlinePanelToggle === "button" ? ( - renderHidePanelButton() - ) : inlinePanelToggle === "reserved" ? ( -
+ {showOuterControls + ? renderConversationCollapseButton({ + onMoveActiveTabToSide, + usesPaneArrangementControl, + }) + : null} + {renderRemoveSplitButton(onRemoveSplit)} + {showOuterControls ? ( + renderAsDrawer || inlinePanelToggle === "button" ? ( + renderHidePanelButton() + ) : inlinePanelToggle === "reserved" ? ( +
+ ) : null ) : null}
) : null} @@ -1002,6 +1028,7 @@ export function ThreadSecondaryPanel({ onBeginTabDrag: pane.onBeginTabDrag, onFocusPane: pane.onFocusPane, onMoveActiveTabToSide: pane.onMoveActiveTabToSide, + onRemoveSplit: pane.onRemoveSplit, onSurfaceTabReorder: pane.onReorderTab, paneId: pane.paneId, reserveLeadingChrome: pane.isTopRow && pane.isLeftEdge, diff --git a/apps/app/src/components/secondary-panel/sidebarSplitLayout.test.ts b/apps/app/src/components/secondary-panel/sidebarSplitLayout.test.ts index 639918cac8..029e0e74bb 100644 --- a/apps/app/src/components/secondary-panel/sidebarSplitLayout.test.ts +++ b/apps/app/src/components/secondary-panel/sidebarSplitLayout.test.ts @@ -18,6 +18,7 @@ import { parseSidebarSplitState, pruneSidebarSplitStorage, reconcileSidebarSplitState, + removeSidebarSplit, reorderSidebarTab, replaceSidebarTab, resizeSidebarSplit, @@ -450,6 +451,59 @@ describe("sidebar split layout", () => { expect(collided).toBe(state); }); + it("removes the focused split while rehoming every tab and its active selection", () => { + const split = splitOff( + createSidebarSplitState(TABS, SIDEBAR_FIXED_INFO_TAB_ID), + "file-a", + ); + const removedPaneId = split.layout.focusedPaneId; + + const unsplit = removeSidebarSplit(split, removedPaneId); + const survivor = getSidebarGroupForPane( + unsplit, + unsplit.layout.focusedPaneId, + ); + + expect(countPanes(unsplit.layout.root)).toBe(1); + expect(survivor?.tabIds).toEqual([ + SIDEBAR_FIXED_INFO_TAB_ID, + SIDEBAR_FIXED_DIFF_TAB_ID, + "file-a", + ]); + expect(survivor?.activeTabId).toBe("file-a"); + }); + + it("removes an unfocused split without stealing focus or active selection", () => { + const split = splitOff( + createSidebarSplitState(TABS, SIDEBAR_FIXED_INFO_TAB_ID), + "file-a", + ); + const focusedPaneId = split.layout.focusedPaneId; + const removedPaneId = listPanes(split.layout.root).find( + (pane) => pane.paneId !== focusedPaneId, + )?.paneId; + expect(removedPaneId).toBeDefined(); + if (removedPaneId === undefined) return; + + const unsplit = removeSidebarSplit(split, removedPaneId); + const survivor = getSidebarGroupForPane(unsplit, focusedPaneId); + + expect(unsplit.layout.focusedPaneId).toBe(focusedPaneId); + expect(survivor?.tabIds).toEqual([ + "file-a", + SIDEBAR_FIXED_INFO_TAB_ID, + SIDEBAR_FIXED_DIFF_TAB_ID, + ]); + expect(survivor?.activeTabId).toBe("file-a"); + }); + + it("does not remove the only sidebar pane or an unknown pane", () => { + const state = createSidebarSplitState(TABS, SIDEBAR_FIXED_INFO_TAB_ID); + + expect(removeSidebarSplit(state, state.layout.focusedPaneId)).toBe(state); + expect(removeSidebarSplit(state, "pane-missing")).toBe(state); + }); + it("moves panes through the shared split operations", () => { const split = splitOff( createSidebarSplitState(TABS, SIDEBAR_FIXED_INFO_TAB_ID), diff --git a/apps/app/src/components/secondary-panel/sidebarSplitLayout.ts b/apps/app/src/components/secondary-panel/sidebarSplitLayout.ts index 8884e2e7e9..a1683181f9 100644 --- a/apps/app/src/components/secondary-panel/sidebarSplitLayout.ts +++ b/apps/app/src/components/secondary-panel/sidebarSplitLayout.ts @@ -416,6 +416,49 @@ function removeEmptySidebarPane( return { ...state, groups, layout: removePane(state.layout, paneId) }; } +/** + * Removes one pane without closing any of its tabs. The removed group's tabs + * join the pane that owns focus after the layout collapses. When the removed + * pane was focused, its active tab stays active in that survivor; removing an + * unfocused pane leaves the survivor's current selection unchanged. + */ +export function removeSidebarSplit( + state: SidebarSplitState, + paneId: string, +): SidebarSplitState { + if (countPanes(state.layout.root) <= 1) return state; + const pane = findPane(state.layout.root, paneId); + const removedGroupId = pane === null ? null : sidebarPaneGroupId(pane); + const removedGroup = + removedGroupId === null ? undefined : state.groups[removedGroupId]; + if (removedGroupId === null || removedGroup === undefined) return state; + + const removedFocusedPane = state.layout.focusedPaneId === paneId; + const layout = removePane(state.layout, paneId); + const survivorPane = findPane(layout.root, layout.focusedPaneId); + const survivorGroupId = + survivorPane === null ? null : sidebarPaneGroupId(survivorPane); + const survivorGroup = + survivorGroupId === null ? undefined : state.groups[survivorGroupId]; + if (survivorGroupId === null || survivorGroup === undefined) return state; + + const groups = { ...state.groups }; + delete groups[removedGroupId]; + groups[survivorGroupId] = { + ...survivorGroup, + tabIds: [ + ...survivorGroup.tabIds, + ...removedGroup.tabIds.filter( + (tabId) => !survivorGroup.tabIds.includes(tabId), + ), + ], + activeTabId: removedFocusedPane + ? removedGroup.activeTabId + : survivorGroup.activeTabId, + }; + return { ...state, groups, layout }; +} + export function moveSidebarPaneToSide( state: SidebarSplitState, paneId: string, diff --git a/apps/app/src/lib/split-drag/splitDragSession.test.ts b/apps/app/src/lib/split-drag/splitDragSession.test.ts index ae780b240f..5952de7354 100644 --- a/apps/app/src/lib/split-drag/splitDragSession.test.ts +++ b/apps/app/src/lib/split-drag/splitDragSession.test.ts @@ -194,4 +194,88 @@ describe("beginSplitDrag — sidebar gesture arbitration and fallback", () => { }); container.remove(); }); + + it("rejects marked panes outside a supplied target boundary", () => { + const boundary = document.createElement("aside"); + document.body.append(boundary); + const config = baseConfig({ targetBoundary: boundary }); + + beginSplitDrag(config); + fireWindowPointer("pointermove", 900, 400); + fireWindowPointer("pointerup", 900, 400); + + expect(config.onDrop).not.toHaveBeenCalled(); + boundary.remove(); + }); + + it("accepts marked panes inside a supplied target boundary", () => { + const boundary = document.createElement("aside"); + boundary.append(paneEl); + document.body.append(boundary); + const config = baseConfig({ targetBoundary: boundary }); + + beginSplitDrag(config); + fireWindowPointer("pointermove", 900, 400); + fireWindowPointer("pointerup", 900, 400); + + expect(config.onDrop).toHaveBeenCalledWith({ + paneId: "pane-1", + zone: "center", + }); + boundary.remove(); + }); + + it("preserves fallback targeting when its container is inside the boundary", () => { + document.elementsFromPoint = vi.fn( + () => [], + ) as typeof document.elementsFromPoint; + const boundary = document.createElement("aside"); + const container = document.createElement("main"); + Object.defineProperty(container, "getBoundingClientRect", { + value: () => PANE_RECT, + configurable: true, + }); + boundary.append(container); + document.body.append(boundary); + const config = baseConfig({ + targetBoundary: boundary, + fallback: { paneId: "pane-1", container }, + }); + + beginSplitDrag(config); + fireWindowPointer("pointermove", 900, 400); + fireWindowPointer("pointermove", 1150, 400); + fireWindowPointer("pointerup", 1150, 400); + + expect(config.onDrop).toHaveBeenCalledWith({ + paneId: "pane-1", + zone: "right", + }); + boundary.remove(); + }); + + it("rejects a fallback target outside a supplied target boundary", () => { + document.elementsFromPoint = vi.fn( + () => [], + ) as typeof document.elementsFromPoint; + const boundary = document.createElement("aside"); + const container = document.createElement("main"); + Object.defineProperty(container, "getBoundingClientRect", { + value: () => PANE_RECT, + configurable: true, + }); + document.body.append(boundary, container); + const config = baseConfig({ + targetBoundary: boundary, + fallback: { paneId: "pane-1", container }, + }); + + beginSplitDrag(config); + fireWindowPointer("pointermove", 900, 400); + fireWindowPointer("pointerup", 900, 400); + + expect(config.onDrop).not.toHaveBeenCalled(); + boundary.remove(); + container.remove(); + }); }); diff --git a/apps/app/src/lib/split-drag/splitDragSession.ts b/apps/app/src/lib/split-drag/splitDragSession.ts index 634f0cb753..2c2e92a190 100644 --- a/apps/app/src/lib/split-drag/splitDragSession.ts +++ b/apps/app/src/lib/split-drag/splitDragSession.ts @@ -51,6 +51,11 @@ export interface SplitDragConfig { * the pointer (the wrapper-less single-pane surface). */ fallback?: SplitDragFallbackTarget; + /** + * Optional container that confines marked panes and the fallback target. + * Omit it to preserve the shared session's existing document-wide hit test. + */ + targetBoundary?: HTMLElement; /** * When true, engaging the drag cancels any in-flight dnd-kit reorder so a * sidebar tear-out can't both split and reorder the same row (Finding 1). @@ -116,11 +121,20 @@ export function beginSplitDrag(config: SplitDragConfig): void { ): ResolvedTarget | null => { const paneEl = paneElementAt(clientX, clientY); const paneId = paneEl?.getAttribute(SPLIT_PANE_DATA_ATTR) ?? null; - if (paneEl && paneId !== null) { + if ( + paneEl && + paneId !== null && + (config.targetBoundary == null || config.targetBoundary.contains(paneEl)) + ) { return { paneId, rect: paneEl.getBoundingClientRect() }; } const fallback = config.fallback; - if (fallback && fallback.container) { + if ( + fallback && + fallback.container && + (config.targetBoundary == null || + config.targetBoundary.contains(fallback.container)) + ) { const rect = fallback.container.getBoundingClientRect(); if ( clientX >= rect.left &&