Skip to content

Fix right-panel split removal and drop bounds #2486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SawyerHood merged 1 commit into from
Aug 27, 2026
Merged

Fix right-panel split removal and drop bounds #2486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion apps/app/src/components/secondary-panel/SidebarSplitContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
parseSidebarSplitState,
pruneSidebarSplitStorage,
reconcileSidebarSplitState,
removeSidebarSplit,
reorderSidebarTab,
replaceSidebarTab,
resizeSidebarSplit,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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<HTMLElement>("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<HTMLElement>(
'[data-testid="thread-secondary-panel-top-chrome"]',
);
Expand All @@ -308,9 +321,10 @@ export function SidebarSplitContainer({
beginSplitDrag({
ghostLabel: label,
sourceEl: sourceElement,
targetBoundary,
fallback: {
paneId: sourcePaneId,
container: sourceElement.closest<HTMLElement>("aside"),
container: targetBoundary,
},
cancelSidebarReorderOnEngage: true,
shouldEngage: (x, y) => {
Expand Down Expand Up @@ -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,
Expand All @@ -423,6 +438,7 @@ export function SidebarSplitContainer({
state={state}
onBeginTabDrag={beginTabDrag}
onFocusPane={focusPane}
onRemoveSplit={removeSplit}
onMoveActiveTabToSide={activeTabPositionHandler}
onReorderTab={reorderTab}
onResize={resize}
Expand Down Expand Up @@ -450,6 +466,7 @@ interface SidebarSplitTreeProps {
event: ReactPointerEvent<HTMLElement>,
) => void;
onFocusPane: (paneId: string) => void;
onRemoveSplit: (paneId: string) => void;
onMoveActiveTabToSide?: (side: SplitSide) => void;
onReorderTab: (
paneId: string,
Expand Down Expand Up @@ -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,
Expand Down
146 changes: 139 additions & 7 deletions apps/app/src/components/secondary-panel/ThreadSecondaryPanel.collapseControl.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
<Wrapper>
<SidebarProvider>
<TooltipProvider>
<PanelGroup direction="horizontal">
<ThreadSecondaryPanel
activeTab={diffFixedTab}
canUseGitUi
fixedTabs={infoAndDiffFixedTabs}
tabs={[]}
isConversationCollapsed={false}
isOpen
metadataContent={<div>Thread metadata</div>}
onClose={noop}
onCollapse={noop}
onTabReorder={noop}
onOpenNewTab={noop}
onPanelFocus={noop}
onToggleConversationCollapse={noop}
renderAsDrawer={false}
splitPanelStateId={panelStateId}
/>
</PanelGroup>
</TooltipProvider>
</SidebarProvider>
</Wrapper>,
);
}

describe("ThreadSecondaryPanel compact file content", () => {
it("renders arbitrary fixed-tab content through the shared surface", () => {
const { wrapper: Wrapper } = createQueryClientTestHarness();
Expand Down Expand Up @@ -272,6 +325,8 @@ describe("ThreadSecondaryPanel compact file content", () => {
sidebarSplitStorageKey(panelStateId),
serializeSidebarSplitState(split),
);
const closeFirstTab = vi.fn();
const closeSecondTab = vi.fn();

render(
<Wrapper>
Expand All @@ -295,15 +350,19 @@ describe("ThreadSecondaryPanel compact file content", () => {
splitPanelStateId={panelStateId}
tabs={[
{
...createTestRenderableTab(firstTab, () => (
<div>First tab body</div>
)),
...createTestRenderableTab(
firstTab,
() => <div>First tab body</div>,
closeFirstTab,
),
label: "first.ts",
},
{
...createTestRenderableTab(secondTab, () => (
<div>Second tab body</div>
)),
...createTestRenderableTab(
secondTab,
() => <div>Second tab body</div>,
closeSecondTab,
),
label: "second.ts",
},
]}
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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<HTMLElement>("[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();
Expand Down
55 changes: 41 additions & 14 deletions apps/app/src/components/secondary-panel/ThreadSecondaryPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ export function ThreadSecondaryPanel({
event: ReactPointerEvent<HTMLElement>,
) => void;
onMoveActiveTabToSide?: (side: SplitSide) => void;
onRemoveSplit?: () => void;
onFocusPane: () => void;
onSurfaceTabReorder: SecondaryPanelTabReorderHandler;
paneId: string | null;
Expand Down Expand Up @@ -609,6 +610,25 @@ export function ThreadSecondaryPanel({
</Button>
);

const renderRemoveSplitButton = (onRemoveSplit?: () => void) =>
onRemoveSplit ? (
<Button
type="button"
variant="ghost"
size="icon"
className={cn(
HEADER_PANE_ACTION_ICON_BUTTON_CLASS,
CHROME_SUBTLE_ICON_BUTTON_FOREGROUND_CLASS,
"shrink-0",
usesDesktopChrome && MACOS_WINDOW_NO_DRAG_CLASS,
)}
aria-label="Remove split"
onClick={onRemoveSplit}
>
<Icon name="CloseThreadPane" />
</Button>
) : null;

const renderConversationCollapseButton = ({
onMoveActiveTabToSide,
usesPaneArrangementControl,
Expand Down Expand Up @@ -734,6 +754,7 @@ export function ThreadSecondaryPanel({
onBeginTabDrag,
onFocusPane,
onMoveActiveTabToSide,
onRemoveSplit,
onSurfaceTabReorder,
paneId,
reserveLeadingChrome,
Expand Down Expand Up @@ -812,24 +833,29 @@ export function ThreadSecondaryPanel({
showNewTabButton: showNewTabControl,
})}
</div>
{showOuterControls ? (
{showOuterControls || onRemoveSplit ? (
<div
className="flex min-w-0 shrink-0 items-center gap-1"
onPointerDown={(event) => event.stopPropagation()}
>
{renderConversationCollapseButton({
onMoveActiveTabToSide,
usesPaneArrangementControl,
})}
{renderAsDrawer || inlinePanelToggle === "button" ? (
renderHidePanelButton()
) : inlinePanelToggle === "reserved" ? (
<div
aria-hidden
className={getReservedInlinePanelToggleClassName(
usesDesktopChrome,
)}
/>
{showOuterControls
? renderConversationCollapseButton({
onMoveActiveTabToSide,
usesPaneArrangementControl,
})
: null}
{renderRemoveSplitButton(onRemoveSplit)}
{showOuterControls ? (
renderAsDrawer || inlinePanelToggle === "button" ? (
renderHidePanelButton()
) : inlinePanelToggle === "reserved" ? (
<div
aria-hidden
className={getReservedInlinePanelToggleClassName(
usesDesktopChrome,
)}
/>
) : null
) : null}
</div>
) : null}
Expand Down Expand Up @@ -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,
Expand Down
Loading
Loading