diff --git a/apps/web/app/dockview-theme.css b/apps/web/app/dockview-theme.css index 0d840ec189..4d1c0d2163 100644 --- a/apps/web/app/dockview-theme.css +++ b/apps/web/app/dockview-theme.css @@ -50,6 +50,19 @@ align-items: center; } +/* The layout editor can place several reusable panels in one group. Let its + * tab strip shrink so Dockview can surface overflowing tabs in its menu rather + * than extending past a narrow mobile editor. */ +[data-testid="layout-editor"] .dv-tabs-and-actions-container .dv-scrollable { + flex: 1 1 0; + min-width: 0; +} + +[data-testid="layout-editor"] .dv-tabs-and-actions-container .dv-tabs-container { + max-width: 100%; + min-width: 0; +} + .dockview-theme-kandev .dv-tabs-container.dv-horizontal .dv-tab::before { display: none !important; } diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index bba5ae19ff..931948d741 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -130,7 +130,6 @@ function usePopoverInteractions() { function PRSingleButton({ pr, refreshTaskPR }: { pr: TaskPR; refreshTaskPR: () => void }) { const addPRPanel = useDockviewStore((s) => s.addPRPanel); - const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); const tooltip = `${pr.owner}/${pr.repo} #${pr.pr_number} — ${pr.pr_title}`; const { usesTouchDrawer, @@ -165,7 +164,7 @@ function PRSingleButton({ pr, refreshTaskPR }: { pr: TaskPR; refreshTaskPR: () = onFocus={onTriggerEnter} onBlur={onTriggerLeave} onClick={() => { - addPRPanel(prTaskKey(pr), activeSessionId); + addPRPanel(prTaskKey(pr)); onOpenChange(false); }} > @@ -209,7 +208,6 @@ function PRMultiButton({ prs, refreshTaskPR }: { prs: TaskPR[]; refreshTaskPR: ( // CI popover with a tab per PR — desktop only, suppressed on touch where // there is no hover. const addPRPanel = useDockviewStore((s) => s.addPRPanel); - const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); const { usesTouchDrawer, open, @@ -291,7 +289,7 @@ function PRMultiButton({ prs, refreshTaskPR }: { prs: TaskPR[]; refreshTaskPR: ( enabled={open} refreshTaskPR={refreshTaskPR} onOpenDetailPanel={(pr) => { - addPRPanel(prTaskKey(pr), activeSessionId); + addPRPanel(prTaskKey(pr)); onOpenChange(false); }} /> @@ -302,7 +300,6 @@ function PRMultiButton({ prs, refreshTaskPR }: { prs: TaskPR[]; refreshTaskPR: ( function MultiPRMenuContent({ prs }: { prs: TaskPR[] }) { const addPRPanel = useDockviewStore((s) => s.addPRPanel); - const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); return ( Pull requests @@ -310,7 +307,7 @@ function MultiPRMenuContent({ prs }: { prs: TaskPR[] }) { {prs.map((pr) => ( addPRPanel(prTaskKey(pr), activeSessionId)} + onClick={() => addPRPanel(prTaskKey(pr))} className="cursor-pointer gap-2" data-testid={`pr-topbar-menu-item-${prIdentitySlug(pr)}`} > diff --git a/apps/web/components/gitlab/mr-topbar-button.test.ts b/apps/web/components/gitlab/mr-topbar-button.test.ts index ae604fc61b..9a973166bf 100644 --- a/apps/web/components/gitlab/mr-topbar-button.test.ts +++ b/apps/web/components/gitlab/mr-topbar-button.test.ts @@ -89,12 +89,12 @@ describe("mrTriggerClass", () => { mr_iid: 22, } as TaskMR; - openDesktopMRReview(addMRPanel, "session-1", mr, schedule); + openDesktopMRReview(addMRPanel, mr, schedule); expect(addMRPanel).toHaveBeenCalledTimes(1); scheduled.shift()?.(0); expect(addMRPanel).toHaveBeenCalledTimes(1); scheduled.shift()?.(0); - expect(addMRPanel).toHaveBeenLastCalledWith("https://gitlab.example|group/b|22", "session-1"); + expect(addMRPanel).toHaveBeenLastCalledWith("https://gitlab.example|group/b|22"); expect(addMRPanel).toHaveBeenCalledTimes(2); }); }); diff --git a/apps/web/components/gitlab/mr-topbar-button.tsx b/apps/web/components/gitlab/mr-topbar-button.tsx index 4c2d5e4a5c..cc2363d9c5 100644 --- a/apps/web/components/gitlab/mr-topbar-button.tsx +++ b/apps/web/components/gitlab/mr-topbar-button.tsx @@ -76,15 +76,14 @@ export function openMobileMRReview( } export function openDesktopMRReview( - addMRPanel: (mrKey: string, sessionId?: string | null) => void, - sessionId: string | null, + addMRPanel: (mrKey: string) => void, mr: TaskMR, schedule: (callback: FrameRequestCallback) => number = requestAnimationFrame, ) { const mrKey = mrTaskKey(mr); - addMRPanel(mrKey, sessionId); + addMRPanel(mrKey); schedule(() => { - schedule(() => addMRPanel(mrKey, sessionId)); + schedule(() => addMRPanel(mrKey)); }); } @@ -139,9 +138,9 @@ function MRMenuButton({ useEffect(() => { if (!dockviewReady || !pendingDesktopMR) return; - openDesktopMRReview(addMRPanel, activeSessionId, pendingDesktopMR); + openDesktopMRReview(addMRPanel, pendingDesktopMR); setPendingDesktopMR(null); - }, [activeSessionId, addMRPanel, dockviewReady, pendingDesktopMR]); + }, [addMRPanel, dockviewReady, pendingDesktopMR]); const openReview = (mr: TaskMR) => { if (mobile) { @@ -151,7 +150,7 @@ function MRMenuButton({ if (!dockviewReady) { setPendingDesktopMR(mr); } else { - openDesktopMRReview(addMRPanel, activeSessionId, mr); + openDesktopMRReview(addMRPanel, mr); } }; return ( diff --git a/apps/web/components/settings/layouts/layout-editor-actions.test.ts b/apps/web/components/settings/layouts/layout-editor-actions.test.ts index ac42ef2276..c97da94ca1 100644 --- a/apps/web/components/settings/layouts/layout-editor-actions.test.ts +++ b/apps/web/components/settings/layouts/layout-editor-actions.test.ts @@ -72,6 +72,20 @@ describe("layout editor actions", () => { expect(addReusablePanel(api, "chat")).toBe(false); }); + it("adds PR Details through the same reusable-panel catalog", () => { + const { api, group } = fakeApi(["chat"]); + + expect(addReusablePanel(api, "pr-detail")).toBe(true); + expect(api.addPanel).toHaveBeenCalledWith( + expect.objectContaining({ + id: "pr-detail", + component: "pr-detail", + title: "PR Details", + position: { referenceGroup: group }, + }), + ); + }); + it("protects Agent while removing another reusable panel", () => { const { api, panels } = fakeApi(); diff --git a/apps/web/components/settings/layouts/layout-editor.tsx b/apps/web/components/settings/layouts/layout-editor.tsx index 9b21262748..e844f61d2d 100644 --- a/apps/web/components/settings/layouts/layout-editor.tsx +++ b/apps/web/components/settings/layouts/layout-editor.tsx @@ -43,6 +43,7 @@ const placeholderComponents = { chat: PlaceholderPanel, files: PlaceholderPanel, changes: PlaceholderPanel, + "pr-detail": PlaceholderPanel, terminal: PlaceholderPanel, plan: PlaceholderPanel, browser: PlaceholderPanel, diff --git a/apps/web/components/task/__tests__/use-auto-pr-panel.test.ts b/apps/web/components/task/__tests__/use-auto-pr-panel.test.ts deleted file mode 100644 index 8cd94c54c5..0000000000 --- a/apps/web/components/task/__tests__/use-auto-pr-panel.test.ts +++ /dev/null @@ -1,220 +0,0 @@ -import { describe, it, expect } from "vitest"; -import type { DockviewApi, AddPanelOptions } from "dockview-react"; -import { - shouldAutoAddPRPanel, - resolvePRPanelTargetGroup, - runAutoPRPanelEffect, -} from "../dockview-session-tabs"; -import { CENTER_GROUP, RIGHT_TOP_GROUP } from "@/lib/state/layout-manager"; - -function makeApi(panels: Array<{ id: string; groupId: string }>): DockviewApi { - return { - getPanel(id: string) { - const p = panels.find((x) => x.id === id); - return p ? { id: p.id, group: { id: p.groupId } } : undefined; - }, - } as unknown as DockviewApi; -} - -describe("shouldAutoAddPRPanel", () => { - const base = { - hasPR: true, - panelExists: false, - isRestoringLayout: false, - isMaximized: false, - wasOffered: false, - }; - - it("returns 'add' when task has PR and panel does not exist", () => { - expect(shouldAutoAddPRPanel(base)).toBe("add"); - }); - - it("returns 'none' when task has no PR", () => { - expect(shouldAutoAddPRPanel({ ...base, hasPR: false })).toBe("none"); - }); - - it("returns 'remove' when task has no PR but panel exists", () => { - expect(shouldAutoAddPRPanel({ ...base, hasPR: false, panelExists: true })).toBe("remove"); - }); - - it("returns 'none' when panel already exists", () => { - expect(shouldAutoAddPRPanel({ ...base, panelExists: true })).toBe("none"); - }); - - it("returns 'none' during layout restoration", () => { - expect(shouldAutoAddPRPanel({ ...base, isRestoringLayout: true })).toBe("none"); - }); - - it("returns 'none' during maximize state", () => { - expect(shouldAutoAddPRPanel({ ...base, isMaximized: true })).toBe("none"); - }); - - it("returns 'none' when panel was already offered and dismissed", () => { - expect(shouldAutoAddPRPanel({ ...base, wasOffered: true })).toBe("none"); - }); - - it("returns 'add' when all conditions are met", () => { - expect( - shouldAutoAddPRPanel({ - hasPR: true, - panelExists: false, - isRestoringLayout: false, - isMaximized: false, - wasOffered: false, - }), - ).toBe("add"); - }); -}); - -describe("resolvePRPanelTargetGroup", () => { - it("returns the session chat panel's live group when it exists", () => { - // Regression: previously the PR panel was anchored to the store's - // centerGroupId, which could lag behind layout transitions and drop the - // PR panel in a split instead of as a tab next to the session. - const api = makeApi([{ id: "session:abc", groupId: "group-live-center" }]); - expect(resolvePRPanelTargetGroup(api, "abc", "stale-center-id")).toBe("group-live-center"); - }); - - it("falls back to centerGroupId when the session panel is missing", () => { - const api = makeApi([]); - expect(resolvePRPanelTargetGroup(api, "abc", "center-id")).toBe("center-id"); - }); - - it("prefers the session panel even when its group differs from centerGroupId", () => { - // centerGroupId still points at the old session's group during a switch; - // the new session's chat panel is the authoritative anchor. - const api = makeApi([{ id: "session:new", groupId: "group-new" }]); - expect(resolvePRPanelTargetGroup(api, "new", "group-old")).toBe("group-new"); - }); - - it("falls back to the center group when the live session panel is in a right group", () => { - // Corrupted layouts can briefly leave the session panel in the right tools - // column. The PR panel must not follow it there. - const api = makeApi([{ id: "session:abc", groupId: RIGHT_TOP_GROUP }]); - expect(resolvePRPanelTargetGroup(api, "abc", "group-center")).toBe("group-center"); - }); - - it("uses the well-known center group when both candidates are right groups", () => { - const api = makeApi([{ id: "session:abc", groupId: RIGHT_TOP_GROUP }]); - expect(resolvePRPanelTargetGroup(api, "abc", RIGHT_TOP_GROUP)).toBe(CENTER_GROUP); - }); -}); - -// --------------------------------------------------------------------------- -// runAutoPRPanelEffect — per-PR tab stamping/backfill -// --------------------------------------------------------------------------- - -type FullMockPanel = { - id: string; - params: Record; - group: { id: string }; - api: { - setActive: () => void; - updateParameters: (p: Record) => void; - close: () => void; - }; -}; - -function makeFullApi(): { api: DockviewApi; panels: FullMockPanel[] } { - const panels: FullMockPanel[] = []; - const groups = [{ id: CENTER_GROUP }]; - const api = { - get groups() { - return groups; - }, - getPanel(id: string) { - return panels.find((p) => p.id === id); - }, - addPanel(opts: AddPanelOptions & { id: string }) { - const panel: FullMockPanel = { - id: opts.id, - params: { ...(opts.params ?? {}) }, - group: { id: CENTER_GROUP }, - api: { - setActive() {}, - updateParameters(p: Record) { - Object.assign(panel.params, p); - }, - close() { - const i = panels.indexOf(panel); - if (i >= 0) panels.splice(i, 1); - }, - }, - }; - panels.push(panel); - return panel; - }, - } as unknown as DockviewApi; - return { api, panels }; -} - -const LEGACY_PR_ID = "pr-detail"; -const DEFAULT_PR_KEY = "org/repo/1"; - -const BASE_EFFECT_PARAMS = { - isRestoringLayout: false, - isMaximized: false, - centerGroupId: CENTER_GROUP, -}; - -describe("runAutoPRPanelEffect", () => { - it("stamps the newly auto-shown panel with the default PR's key", () => { - const { api } = makeFullApi(); - - runAutoPRPanelEffect(api, "session-add", { - ...BASE_EFFECT_PARAMS, - hasPR: true, - defaultPRKey: DEFAULT_PR_KEY, - }); - - const panel = api.getPanel(LEGACY_PR_ID) as unknown as FullMockPanel; - expect(panel).toBeDefined(); - expect(panel.params.prKey).toBe(DEFAULT_PR_KEY); - }); - - it("backfills the key on a legacy panel restored without one", () => { - const { api } = makeFullApi(); - api.addPanel({ - id: LEGACY_PR_ID, - component: LEGACY_PR_ID, - title: "Pull Request", - position: { referenceGroup: CENTER_GROUP }, - }); - - runAutoPRPanelEffect(api, "session-backfill", { - ...BASE_EFFECT_PARAMS, - hasPR: true, - defaultPRKey: DEFAULT_PR_KEY, - }); - - const panel = api.getPanel(LEGACY_PR_ID) as unknown as FullMockPanel; - expect(panel.params.prKey).toBe(DEFAULT_PR_KEY); - }); - - it("resyncs a legacy panel whose stamped key no longer matches the current default", () => { - // Regression (Greptile P1 / cubic-dev-ai on PR #1636): the legacy panel - // must track whichever PR is CURRENTLY the task's default — reused - // across a task switch or after the primary PR changes for the same - // task. Nothing else ever stamps a deliberately different key onto this - // specific panel (a manual "+" menu pick of another PR always creates - // its own separate `pr-detail|` tab instead), so resyncing here can - // never clobber a real user choice. - const { api } = makeFullApi(); - api.addPanel({ - id: LEGACY_PR_ID, - component: LEGACY_PR_ID, - title: "Pull Request", - params: { prKey: "org/repo/2" }, - position: { referenceGroup: CENTER_GROUP }, - }); - - runAutoPRPanelEffect(api, "session-resync", { - ...BASE_EFFECT_PARAMS, - hasPR: true, - defaultPRKey: DEFAULT_PR_KEY, - }); - - const panel = api.getPanel(LEGACY_PR_ID) as unknown as FullMockPanel; - expect(panel.params.prKey).toBe(DEFAULT_PR_KEY); - }); -}); diff --git a/apps/web/components/task/dockview-add-panel-items.tsx b/apps/web/components/task/dockview-add-panel-items.tsx index 6b992aa3c2..43ac7625af 100644 --- a/apps/web/components/task/dockview-add-panel-items.tsx +++ b/apps/web/components/task/dockview-add-panel-items.tsx @@ -11,7 +11,6 @@ import { import { DropdownMenuItem } from "@kandev/ui/dropdown-menu"; import { prPanelLabel, prIdentitySlug, prTaskKey } from "@/components/github/pr-utils"; import { useDockviewStore } from "@/lib/state/dockview-store"; -import { useAppStore } from "@/components/state-provider"; import type { TaskPR } from "@/lib/types/github"; import type { TaskMR } from "@/lib/types/gitlab"; import { mrTaskKey } from "@/components/gitlab/mr-detail-panel"; @@ -56,7 +55,6 @@ export function AddPanelMenuItems({ const addChangesPanel = useDockviewStore((s) => s.addChangesPanel); const addPRPanel = useDockviewStore((s) => s.addPRPanel); const addMRPanel = useDockviewStore((s) => s.addMRPanel); - const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); return ( <> @@ -100,7 +98,7 @@ export function AddPanelMenuItems({ {state.prs.map((pr) => ( addPRPanel(prTaskKey(pr), activeSessionId)} + onClick={() => addPRPanel(prTaskKey(pr))} className={MENU_ITEM_CLASS} data-testid={`add-panel-pr-item-${prIdentitySlug(pr)}`} > @@ -113,7 +111,7 @@ export function AddPanelMenuItems({ {state.mrs.map((mr) => ( addMRPanel(mrTaskKey(mr), activeSessionId)} + onClick={() => addMRPanel(mrTaskKey(mr))} className={MENU_ITEM_CLASS} data-testid={`add-panel-mr-item-${mr.id}`} > diff --git a/apps/web/components/task/dockview-auto-mr-panel.test.ts b/apps/web/components/task/dockview-auto-mr-panel.test.ts deleted file mode 100644 index 4fc47e4da2..0000000000 --- a/apps/web/components/task/dockview-auto-mr-panel.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - isLiveAutoMRPanelIdentity, - resolveAutoMRPanelAction, - scheduleAfterTwoFrames, -} from "./dockview-auto-mr-panel"; - -describe("resolveAutoMRPanelAction", () => { - it("adds a GitLab panel for a linked MR that has not been offered", () => { - expect( - resolveAutoMRPanelAction({ - hasMR: true, - panelExists: false, - restoring: false, - maximized: false, - offered: false, - }), - ).toBe("add"); - }); - - it("removes only the auto panel when GitLab is no longer the primary review", () => { - expect( - resolveAutoMRPanelAction({ - hasMR: false, - panelExists: true, - restoring: false, - maximized: false, - offered: true, - }), - ).toBe("remove"); - }); - - it("does not reopen a dismissed panel", () => { - expect( - resolveAutoMRPanelAction({ - hasMR: true, - panelExists: false, - restoring: false, - maximized: false, - offered: true, - }), - ).toBe("none"); - }); -}); - -describe("scheduleAfterTwoFrames", () => { - it("cancels the pending inner frame before it can mutate the next task layout", () => { - const callbacks = new Map(); - let nextID = 1; - const schedule = (callback: FrameRequestCallback) => { - const id = nextID++; - callbacks.set(id, callback); - return id; - }; - const cancel = (id: number) => callbacks.delete(id); - let calls = 0; - - const cleanup = scheduleAfterTwoFrames(() => calls++, schedule, cancel); - const first = callbacks.get(1); - callbacks.delete(1); - first?.(0); - cleanup(); - const second = callbacks.get(2); - second?.(0); - - expect(calls).toBe(0); - expect(callbacks.has(2)).toBe(false); - }); -}); - -describe("isLiveAutoMRPanelIdentity", () => { - it("rejects a task switch between scheduled frames", () => { - expect( - isLiveAutoMRPanelIdentity( - { taskId: "task-a", sessionId: "session-a", workspaceId: "workspace-a" }, - { taskId: "task-b", sessionId: "session-b", workspaceId: "workspace-a" }, - ), - ).toBe(false); - }); -}); diff --git a/apps/web/components/task/dockview-auto-mr-panel.ts b/apps/web/components/task/dockview-auto-mr-panel.ts deleted file mode 100644 index a79365f178..0000000000 --- a/apps/web/components/task/dockview-auto-mr-panel.ts +++ /dev/null @@ -1,123 +0,0 @@ -import { useEffect } from "react"; -import { useAppStore, useAppStoreApi } from "@/components/state-provider"; -import { mrTaskKey } from "@/components/gitlab/mr-detail-panel"; -import { markPRPanelOffered, wasPRPanelOffered } from "@/lib/local-storage"; -import { focusOrAddPanel } from "@/lib/state/dockview-layout-builders"; -import { useDockviewStore } from "@/lib/state/dockview-store"; -import type { AppState } from "@/lib/state/store"; -import type { TaskMR } from "@/lib/types/gitlab"; -import { resolvePRPanelTargetGroup } from "./dockview-session-tabs"; - -export function resolveAutoMRPanelAction(params: { - hasMR: boolean; - panelExists: boolean; - restoring: boolean; - maximized: boolean; - offered: boolean; -}): "add" | "remove" | "none" { - if (!params.hasMR) return params.panelExists ? "remove" : "none"; - if (params.panelExists || params.restoring || params.maximized || params.offered) return "none"; - return "add"; -} - -type AutoMRPanelIdentity = { - taskId: string | null; - sessionId: string | null; - workspaceId: string | null; -}; - -export function isLiveAutoMRPanelIdentity( - expected: AutoMRPanelIdentity, - live: AutoMRPanelIdentity, -): boolean { - return ( - expected.taskId === live.taskId && - expected.sessionId === live.sessionId && - expected.workspaceId === live.workspaceId - ); -} - -export function scheduleAfterTwoFrames( - run: FrameRequestCallback, - schedule: typeof requestAnimationFrame = requestAnimationFrame, - cancel: typeof cancelAnimationFrame = cancelAnimationFrame, -): () => void { - let innerFrame: number | null = null; - const outerFrame = schedule((time) => { - innerFrame = schedule(run); - void time; - }); - return () => { - cancel(outerFrame); - if (innerFrame !== null) cancel(innerFrame); - }; -} - -function livePrimaryMR(app: AppState, expected: AutoMRPanelIdentity): TaskMR | null | undefined { - const live = { - taskId: app.tasks.activeTaskId, - sessionId: app.tasks.activeSessionId, - workspaceId: app.workspaces.activeId, - }; - if (!isLiveAutoMRPanelIdentity(expected, live) || !live.workspaceId || !live.taskId) - return undefined; - if ((app.taskPRs.byTaskId[live.taskId]?.length ?? 0) > 0) return null; - return app.taskMRs.byWorkspaceId[live.workspaceId]?.[live.taskId]?.[0] ?? null; -} - -function applyAutoMRPanel(expected: AutoMRPanelIdentity, app: AppState) { - const liveMR = livePrimaryMR(app, expected); - if (liveMR === undefined) return; - const api = useDockviewStore.getState().api; - if (!api || !expected.sessionId) return; - const panel = api.getPanel("mr-detail"); - const dockview = useDockviewStore.getState(); - const action = resolveAutoMRPanelAction({ - hasMR: !!liveMR, - panelExists: !!panel, - restoring: dockview.isRestoringLayout, - maximized: dockview.preMaximizeLayout !== null, - offered: wasPRPanelOffered(expected.sessionId), - }); - if (action === "remove") return panel?.api.close(); - if (!liveMR) return; - const key = mrTaskKey(liveMR); - if (panel) { - if (panel.params?.mrKey !== key) panel.api.updateParameters({ mrKey: key }); - } else if (action === "add") { - focusOrAddPanel(api, { - id: "mr-detail", - component: "mr-detail", - title: "Merge Request", - position: { - referenceGroup: resolvePRPanelTargetGroup(api, expected.sessionId, dockview.centerGroupId), - }, - inactive: true, - params: { mrKey: key }, - }); - } else { - return; - } - markPRPanelOffered(expected.sessionId); -} - -/** Auto-show a GitLab MR panel when the task has no GitHub PR panel to prefer. */ -export function useAutoMRPanel() { - const appStore = useAppStoreApi(); - const taskId = useAppStore((state) => state.tasks.activeTaskId); - const sessionId = useAppStore((state) => state.tasks.activeSessionId); - const workspaceId = useAppStore((state) => state.workspaces.activeId); - const primaryMR = useAppStore((state) => { - if (!taskId || !workspaceId) return null; - const hasGitHub = (state.taskPRs.byTaskId[taskId]?.length ?? 0) > 0; - if (hasGitHub) return null; - return state.taskMRs.byWorkspaceId[workspaceId]?.[taskId]?.[0] ?? null; - }); - const hasApi = useDockviewStore((state) => !!state.api); - - useEffect(() => { - if (!taskId || !sessionId || !hasApi) return; - const expected = { taskId, sessionId, workspaceId }; - return scheduleAfterTwoFrames(() => applyAutoMRPanel(expected, appStore.getState())); - }, [appStore, taskId, sessionId, workspaceId, hasApi, primaryMR]); -} diff --git a/apps/web/components/task/dockview-desktop-layout.tsx b/apps/web/components/task/dockview-desktop-layout.tsx index ea3f480484..827e7463bd 100644 --- a/apps/web/components/task/dockview-desktop-layout.tsx +++ b/apps/web/components/task/dockview-desktop-layout.tsx @@ -36,12 +36,8 @@ import { SessionTab } from "./session-tab"; import { TerminalTab } from "./terminal-tab"; import { useTabMaximizeOnDoubleClick } from "./use-tab-maximize"; import { setupSessionTabSync } from "./dockview-session-tab-sync"; -import { - setupChatPanelSafetyNet, - useAutoSessionTab, - useAutoPRPanel, -} from "./dockview-session-tabs"; -import { useAutoMRPanel } from "./dockview-auto-mr-panel"; +import { setupChatPanelSafetyNet, useAutoSessionTab } from "./dockview-session-tabs"; +import { useSyncReviewPanel } from "./dockview-review-panel-sync"; import { useCompactDockviewDefault, useDockviewUnmountCleanup, @@ -412,8 +408,7 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ useAutoSessionTab(effectiveSessionId); useChangesPanelAutoFocus(changesFocusKey); - useAutoPRPanel(); - useAutoMRPanel(); + useSyncReviewPanel(); useDockviewUnmountCleanup(saveTimerRef, readyDisposersRef); // Visual masking: hide the dockview container during slow-path layout diff --git a/apps/web/components/task/dockview-review-panel-sync.test.ts b/apps/web/components/task/dockview-review-panel-sync.test.ts new file mode 100644 index 0000000000..d228fb772f --- /dev/null +++ b/apps/web/components/task/dockview-review-panel-sync.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it, vi } from "vitest"; +import type { DockviewApi } from "dockview-react"; +import type { TaskPR } from "@/lib/types/github"; +import type { TaskMR } from "@/lib/types/gitlab"; +import { + resolveCanonicalReviewParams, + syncCanonicalReviewPanel, +} from "./dockview-review-panel-sync"; + +function makeApi(panel?: { params?: Record; groupId?: string }): { + api: DockviewApi; + updateParameters: ReturnType; +} { + const updateParameters = vi.fn((next: Record) => { + Object.assign(panel?.params ?? {}, next); + }); + const reviewPanel = panel + ? { + id: "pr-detail", + params: panel.params ?? {}, + group: { id: panel.groupId ?? "group-right-top" }, + api: { updateParameters }, + } + : undefined; + return { + api: { + getPanel: (id: string) => (id === "pr-detail" ? reviewPanel : undefined), + addPanel: vi.fn(), + removePanel: vi.fn(), + } as unknown as DockviewApi, + updateParameters, + }; +} + +const githubPR = { + owner: "kandev", + repo: "kandev", + pr_number: 42, +} as TaskPR; + +const gitlabMR = { + host: "https://gitlab.example.test", + project_path: "group/project", + mr_iid: 7, +} as TaskMR; + +describe("resolveCanonicalReviewParams", () => { + it("prefers the primary GitHub pull request when both providers are linked", () => { + expect(resolveCanonicalReviewParams([githubPR], [gitlabMR])).toEqual({ + provider: "github", + prKey: "kandev/kandev/42", + mrKey: undefined, + }); + }); + + it("selects the first linked GitLab merge request when GitHub is absent", () => { + expect(resolveCanonicalReviewParams([], [gitlabMR])).toEqual({ + provider: "gitlab", + prKey: undefined, + mrKey: "https://gitlab.example.test|group/project|7", + }); + }); + + it("clears review identity when the active task has no linked review", () => { + expect(resolveCanonicalReviewParams([], [])).toEqual({ + provider: undefined, + prKey: undefined, + mrKey: undefined, + }); + }); +}); + +describe("syncCanonicalReviewPanel", () => { + it("leaves a layout without PR Details structurally untouched", () => { + const { api, updateParameters } = makeApi(); + + expect(syncCanonicalReviewPanel(api, resolveCanonicalReviewParams([githubPR], []))).toBe(false); + expect(updateParameters).not.toHaveBeenCalled(); + expect(api.addPanel).not.toHaveBeenCalled(); + expect(api.removePanel).not.toHaveBeenCalled(); + }); + + it("updates an existing panel's identity without changing its configured group", () => { + const params: Record = { provider: "gitlab", mrKey: "old/mr" }; + const { api, updateParameters } = makeApi({ params, groupId: "custom-review-group" }); + + expect(syncCanonicalReviewPanel(api, resolveCanonicalReviewParams([githubPR], []))).toBe(true); + expect(updateParameters).toHaveBeenCalledWith({ + provider: "github", + prKey: "kandev/kandev/42", + mrKey: undefined, + }); + expect(api.getPanel("pr-detail")?.group.id).toBe("custom-review-group"); + expect(params).toMatchObject({ provider: "github", prKey: "kandev/kandev/42" }); + expect(params.mrKey).toBeUndefined(); + }); +}); diff --git a/apps/web/components/task/dockview-review-panel-sync.ts b/apps/web/components/task/dockview-review-panel-sync.ts new file mode 100644 index 0000000000..c564440899 --- /dev/null +++ b/apps/web/components/task/dockview-review-panel-sync.ts @@ -0,0 +1,102 @@ +"use client"; + +import { useEffect } from "react"; +import type { DockviewApi } from "dockview-react"; +import { prTaskKey } from "@/components/github/pr-utils"; +import { mrTaskKey } from "@/components/gitlab/mr-detail-panel"; +import { useAppStore, useAppStoreApi } from "@/components/state-provider"; +import { getPrimaryTaskPR } from "@/hooks/domains/github/use-task-pr"; +import { useDockviewStore } from "@/lib/state/dockview-store"; +import type { TaskPR } from "@/lib/types/github"; +import type { TaskMR } from "@/lib/types/gitlab"; + +export type CanonicalReviewParams = { + provider: "github" | "gitlab" | undefined; + prKey: string | undefined; + mrKey: string | undefined; +}; + +export function resolveCanonicalReviewParams( + prs: TaskPR[] | undefined, + mrs: TaskMR[] | undefined, +): CanonicalReviewParams { + const pr = getPrimaryTaskPR(prs); + if (pr) return { provider: "github", prKey: prTaskKey(pr), mrKey: undefined }; + + const mr = mrs?.[0]; + if (mr) return { provider: "gitlab", prKey: undefined, mrKey: mrTaskKey(mr) }; + + return { provider: undefined, prKey: undefined, mrKey: undefined }; +} + +function hasSameReviewParams( + current: Record | undefined, + next: CanonicalReviewParams, +): boolean { + return ( + current?.provider === next.provider && + current?.prKey === next.prKey && + current?.mrKey === next.mrKey + ); +} + +/** + * Update only the review identity of a layout-owned PR Details panel. + * + * Layout profile and task-layout restoration own panel existence and position. + * This helper deliberately never calls add, close, move, or activate APIs. + */ +export function syncCanonicalReviewPanel(api: DockviewApi, next: CanonicalReviewParams): boolean { + const panel = api.getPanel("pr-detail"); + if (!panel || hasSameReviewParams(panel.params, next)) return false; + panel.api.updateParameters(next); + return true; +} + +function reviewIdentity(params: CanonicalReviewParams): string { + return `${params.provider ?? "none"}:${params.prKey ?? ""}:${params.mrKey ?? ""}`; +} + +/** Keep an existing canonical PR Details panel in sync with the active task. */ +export function useSyncReviewPanel() { + const appStore = useAppStoreApi(); + const taskId = useAppStore((state) => state.tasks.activeTaskId); + const workspaceId = useAppStore((state) => state.workspaces.activeId); + const identity = useAppStore((state) => { + if (!taskId || !workspaceId) return "none"; + return reviewIdentity( + resolveCanonicalReviewParams( + state.taskPRs.byTaskId[taskId], + state.taskMRs.byWorkspaceId[workspaceId]?.[taskId], + ), + ); + }); + const hasApi = useDockviewStore((state) => !!state.api); + + useEffect(() => { + if (!taskId || !workspaceId || !hasApi) return; + + let innerFrame: number | null = null; + const outerFrame = requestAnimationFrame(() => { + innerFrame = requestAnimationFrame(() => { + const live = appStore.getState(); + if (live.tasks.activeTaskId !== taskId || live.workspaces.activeId !== workspaceId) return; + + const api = useDockviewStore.getState().api; + if (!api) return; + syncCanonicalReviewPanel( + api, + resolveCanonicalReviewParams( + live.taskPRs.byTaskId[taskId], + live.taskMRs.byWorkspaceId[workspaceId]?.[taskId], + ), + ); + }); + }); + + return () => { + cancelAnimationFrame(outerFrame); + if (innerFrame !== null) cancelAnimationFrame(innerFrame); + }; + }, [appStore, hasApi, identity, taskId, workspaceId]); +} diff --git a/apps/web/components/task/dockview-session-tabs.test.ts b/apps/web/components/task/dockview-session-tabs.test.ts index 5afd5cbb54..0b37d7c9b0 100644 --- a/apps/web/components/task/dockview-session-tabs.test.ts +++ b/apps/web/components/task/dockview-session-tabs.test.ts @@ -3,7 +3,6 @@ import type { DockviewApi } from "dockview-react"; import type { TaskSession } from "@/lib/types/http"; import { ensureSessionTabPrecedesNonSessionTabs, - findSessionAnchorGroupId, reconcileRemovedSessionPanels, resolveInitialPosition, resolveSessionTabSyncTarget, @@ -294,42 +293,6 @@ describe("reconcileRemovedSessionPanels", () => { }); }); -describe("findSessionAnchorGroupId", () => { - function makeApiWithPanels(panels: Array<{ id: string; groupId: string }>): DockviewApi { - const enriched = panels.map((p) => ({ id: p.id, group: { id: p.groupId } })); - return { - panels: enriched, - getPanel: (id: string) => enriched.find((p) => p.id === id) ?? null, - } as unknown as DockviewApi; - } - - it("returns the group id of a pr-detail anchor panel", () => { - // Regression: when a saved layout's session was sanitized away (page load) - // or replaced (env switch) but pr-detail remained, the new session would - // be added as a right-of-sidebar split instead of joining pr-detail's - // group — pulling pr-detail out of the user's grouping with the agent. - const api = makeApiWithPanels([{ id: "pr-detail", groupId: "saved-center-group" }]); - - expect(findSessionAnchorGroupId(api)).toBe("saved-center-group"); - }); - - it("matches keyed pr-detail panels used by multi-repo PR flows", () => { - // Multi-repo tasks open one PR tab per PR with id `pr-detail|owner/repo/N` - // (see addPRPanel in dockview-panel-actions.ts). Without prefix matching, - // findSessionAnchorGroupId would miss them and the new session would land - // as a right-of-sidebar split — losing the user's PR/session grouping. - const api = makeApiWithPanels([{ id: "pr-detail|owner/repo/123", groupId: "keyed-pr-group" }]); - - expect(findSessionAnchorGroupId(api)).toBe("keyed-pr-group"); - }); - - it("returns null when no anchor panel exists", () => { - const api = makeApiWithPanels([]); - - expect(findSessionAnchorGroupId(api)).toBeNull(); - }); -}); - describe("resolveInitialPosition", () => { it("replaces Chat inside its live custom-layout group", () => { const api = makePositionApi({ diff --git a/apps/web/components/task/dockview-session-tabs.ts b/apps/web/components/task/dockview-session-tabs.ts index 86145cd882..5cf2b500c8 100644 --- a/apps/web/components/task/dockview-session-tabs.ts +++ b/apps/web/components/task/dockview-session-tabs.ts @@ -3,20 +3,15 @@ import type { DockviewApi, DockviewReadyEvent, AddPanelOptions } from "dockview- import type { StoreApi } from "zustand"; import type { AppState } from "@/lib/state/store"; import { releaseLayoutToDefault, useDockviewStore } from "@/lib/state/dockview-store"; -import { focusOrAddPanel } from "@/lib/state/dockview-layout-builders"; import { CENTER_GROUP, isCenterCandidateGroupId, RIGHT_TOP_GROUP, } from "@/lib/state/layout-manager"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; -import { wasPRPanelOffered, markPRPanelOffered } from "@/lib/local-storage"; import { sessionId as toSessionId } from "@/lib/types/ids"; import { createDebugLogger, isDebug } from "@/lib/debug/log"; import type { TaskSession } from "@/lib/types/http"; -import type { TaskPR } from "@/lib/types/github"; -import { getPrimaryTaskPR } from "@/hooks/domains/github/use-task-pr"; -import { prTaskKey } from "@/components/github/pr-utils"; import { activateChatReplacement, isChatPlaceholderSelected, @@ -148,224 +143,6 @@ export function setupChatPanelSafetyNet( }); } -// --------------------------------------------------------------------------- -// Auto-show PR detail panel -// --------------------------------------------------------------------------- - -/** Pure decision function for whether the PR panel should be auto-added or removed. */ -export function shouldAutoAddPRPanel(params: { - hasPR: boolean; - panelExists: boolean; - isRestoringLayout: boolean; - isMaximized: boolean; - wasOffered: boolean; -}): "add" | "remove" | "none" { - if (!params.hasPR && params.panelExists) return "remove"; - if (!params.hasPR) return "none"; - if (params.panelExists) return "none"; - if (params.isRestoringLayout) return "none"; - if (params.isMaximized) return "none"; - if (params.wasOffered) return "none"; - return "add"; -} - -/** - * Resolve the group ID to anchor the PR detail panel to. - * - * Preference: the live session chat panel's group. It's the group the user is - * actively looking at, and reading it directly avoids the stale-id window the - * store's centerGroupId has across layout transitions (which caused the PR - * panel to land in a split instead of as a tab next to the session). - */ -export function resolvePRPanelTargetGroup( - api: DockviewApi, - sessionId: string, - centerGroupId: string, -): string { - const sessionPanel = api.getPanel(`session:${sessionId}`); - const sessionGroupId = sessionPanel?.group?.id; - if (sessionGroupId && isCenterCandidateGroupId(sessionGroupId)) return sessionGroupId; - return isCenterCandidateGroupId(centerGroupId) ? centerGroupId : CENTER_GROUP; -} - -/** - * Derive the auto-PR-panel decision inputs from one task's live PR list. - * - * @param taskPRs - The task's associated PRs, in creation order; `undefined` - * when the task has none loaded. - * @returns `hasPR` — whether the task has any linked PR; `defaultPRKey` — - * the key of the primary/first PR (matches `PRDetailPanelComponent`'s - * fallback when no explicit `prKey` param is set), or `undefined` when - * there's no PR. - */ -function resolveAutoPRPanelState(taskPRs: TaskPR[] | undefined): { - hasPR: boolean; - defaultPRKey: string | undefined; -} { - const primary = getPrimaryTaskPR(taskPRs); - return { - hasPR: !!taskPRs && taskPRs.length > 0, - defaultPRKey: primary ? prTaskKey(primary) : undefined, - }; -} - -/** - * Pure effect logic for `useAutoPRPanel`: decides whether to add, remove, - * or leave alone the auto-shown PR detail panel, and mutates the given - * dockview `api` accordingly. Extracted for unit testing. - * - * @param api - The live dockview API to add/remove/update panels on. - * @param sessionId - The active session, used for the offered/dismissed - * sessionStorage flag and to resolve the panel's target group. - * @param params.hasPR - Whether the active task has any linked PR. - * @param params.defaultPRKey - Key of the PR the legacy unkeyed - * "pr-detail" panel should render (and stay resynced to). - * @param params.isRestoringLayout - Suppresses auto-add while a saved - * layout is being restored. - * @param params.isMaximized - Suppresses auto-add while a group is - * maximized. - * @param params.centerGroupId - Fallback group when no live session panel - * can anchor the new PR panel. - */ -export function runAutoPRPanelEffect( - api: DockviewApi, - sessionId: string, - params: { - hasPR: boolean; - /** Key of the PR the legacy unkeyed "pr-detail" panel should render. */ - defaultPRKey: string | undefined; - isRestoringLayout: boolean; - isMaximized: boolean; - centerGroupId: string; - }, -): void { - const decision = shouldAutoAddPRPanel({ - hasPR: params.hasPR, - panelExists: !!api.getPanel("pr-detail"), - isRestoringLayout: params.isRestoringLayout, - isMaximized: params.isMaximized, - wasOffered: wasPRPanelOffered(sessionId), - }); - if (decision === "remove") { - api.getPanel("pr-detail")?.api.close(); - return; - } - - if (decision === "add") { - const targetGroupId = resolvePRPanelTargetGroup(api, sessionId, params.centerGroupId); - focusOrAddPanel(api, { - id: "pr-detail", - component: "pr-detail", - title: "Pull Request", - position: { referenceGroup: targetGroupId }, - inactive: true, - // Stamp the panel's params so addPRPanel can tell a matching menu - // click (reuse this tab) apart from a different PR's click (open a - // new tab) — see addPRPanel in dockview-panel-actions.ts. - params: params.defaultPRKey ? { prKey: params.defaultPRKey } : undefined, - }); - markPRPanelOffered(sessionId); - return; - } - - // "none" — panel already present or conditions not met. - // Mark as offered if the panel exists (e.g. restored from saved layout). - const legacy = api.getPanel("pr-detail"); - if (params.hasPR && legacy) { - markPRPanelOffered(sessionId); - // Keep the legacy tab's stamped key in sync with the CURRENT default PR. - // Nothing else ever writes a different key onto this specific panel — a - // manual "+" menu pick of a different PR always creates its own keyed - // `pr-detail|` tab instead (see addPRPanel in - // dockview-panel-actions.ts) — so unconditionally resyncing here is safe - // and fixes staleness both when the primary PR changes for this task and - // when this panel is reused across a task switch (see Greptile/cubic - // review on PR #1636). - if (params.defaultPRKey && legacy.params?.prKey !== params.defaultPRKey) { - legacy.api.updateParameters({ prKey: params.defaultPRKey }); - } - } -} - -/** - * Auto-add the PR detail panel to the center group when the active task - * has an associated pull request. The panel is added as a background tab - * (the session/agent tab stays focused). - * - * Dismissal is persisted to sessionStorage: if the user closes the PR panel, - * it won't be re-added for that session — even after a page refresh. - */ -export function useAutoPRPanel() { - const taskId = useAppStore((s) => s.tasks.activeTaskId); - const sessionId = useAppStore((s) => s.tasks.activeSessionId); - const hasPR = useAppStore((s) => { - const tid = s.tasks.activeTaskId; - return resolveAutoPRPanelState(tid ? s.taskPRs.byTaskId[tid] : undefined).hasPR; - }); - // Key of the PR the legacy unkeyed "pr-detail" panel renders — mirrors - // PRDetailPanelComponent's fallback of the primary/first TaskPR. - const defaultPRKey = useAppStore((s) => { - const tid = s.tasks.activeTaskId; - return resolveAutoPRPanelState(tid ? s.taskPRs.byTaskId[tid] : undefined).defaultPRKey; - }); - const hasApi = useDockviewStore((s) => !!s.api); - const appStore = useAppStoreApi(); - - useEffect(() => { - if (!taskId || !hasApi || !sessionId) return; - - requestAnimationFrame(() => { - requestAnimationFrame(() => { - const api = useDockviewStore.getState().api; - if (!api) return; - - // Re-read live task/session/PR state before mutating dockview — a - // task or session switch during this two-frame delay must not stamp - // the panel with a stale task's PR key (cubic-dev-ai review on PR - // #1636). If the active task/session moved on, bail: the effect - // instance already scheduled for the new task/session (its deps - // changed) will handle it correctly. - const liveTasks = appStore.getState().tasks; - if (liveTasks.activeTaskId !== taskId || liveTasks.activeSessionId !== sessionId) return; - const live = resolveAutoPRPanelState(appStore.getState().taskPRs.byTaskId[taskId]); - - runAutoPRPanelEffect(api, sessionId, { - hasPR: live.hasPR, - defaultPRKey: live.defaultPRKey, - isRestoringLayout: useDockviewStore.getState().isRestoringLayout, - isMaximized: useDockviewStore.getState().preMaximizeLayout !== null, - centerGroupId: useDockviewStore.getState().centerGroupId, - }); - }); - }); - }, [taskId, hasPR, hasApi, sessionId, defaultPRKey, appStore]); -} - -/** - * Panels that are added co-tabbed with a session panel (see `useAutoPRPanel`). - * When a saved layout's session was stripped (phantom-session sanitize on - * page load, or stale removal during env switch), these siblings end up alone - * in a group with no session. Prefer joining that group when adding the new - * active session — without this fallback we'd add the session as a fresh - * split next to the sidebar, breaking the user's grouping. - * - * Each entry matches either the bare id (e.g. `pr-detail`) or a keyed - * variant `|` (multi-repo PR panels use `pr-detail|owner/repo/N`, - * see `addPRPanel` in dockview-panel-actions.ts). - */ -const SESSION_ANCHOR_PANEL_IDS = ["pr-detail", "mr-detail"]; - -export function findSessionAnchorGroupId(api: DockviewApi): string | null { - for (const id of SESSION_ANCHOR_PANEL_IDS) { - const exact = api.getPanel(id); - if (exact) return exact.group.id; - const keyedPrefix = `${id}|`; - const keyed = api.panels.find((p) => p.id.startsWith(keyedPrefix)); - if (keyed) return keyed.group.id; - } - return null; -} - export function resolveInitialPosition( api: DockviewApi, chatWasSelected = false, @@ -391,16 +168,6 @@ export function resolveInitialPosition( isCenterCandidateGroupId(centerGroupId) && api.groups.some((g) => g.id === centerGroupId); if (centerGroupExists) return { referenceGroup: centerGroupId }; - const anchorGroupId = findSessionAnchorGroupId(api); - // index:0 matches the project's session-on-the-left convention (agent tab - // first, pr-detail/etc. to the right). Worth noting: if a user had - // rearranged a previous layout to put pr-detail first, that ordering is - // lost here — but the alternative (appending) would put the agent tab - // to the right of pr-detail, which contradicts the default placement - // every other code path produces. Pick the consistent default. - if (anchorGroupId && isCenterCandidateGroupId(anchorGroupId)) { - return { referenceGroup: anchorGroupId, index: 0 }; - } const rightTopExists = api.groups.some((g) => g.id === RIGHT_TOP_GROUP); if (rightTopExists) return { referenceGroup: RIGHT_TOP_GROUP, direction: "left" }; return undefined; diff --git a/apps/web/components/task/task-select-helpers.test.ts b/apps/web/components/task/task-select-helpers.test.ts index a8dda13c28..13f3a7ff07 100644 --- a/apps/web/components/task/task-select-helpers.test.ts +++ b/apps/web/components/task/task-select-helpers.test.ts @@ -5,6 +5,13 @@ import { selectTaskWithLayout, } from "./task-select-helpers"; +const { dockviewState } = vi.hoisted(() => ({ + dockviewState: { + api: null as unknown, + buildDefaultLayout: vi.fn(), + }, +})); + vi.mock("@/lib/services/session-launch-service", () => ({ launchSession: vi.fn(), })); @@ -14,10 +21,7 @@ vi.mock("@/lib/services/session-launch-helpers", () => ({ vi.mock("@/lib/state/dockview-store", () => ({ performLayoutSwitch: vi.fn(), releaseLayoutToDefault: vi.fn(), - useDockviewStore: { getState: () => ({ api: null, buildDefaultLayout: vi.fn() }) }, -})); -vi.mock("@/lib/state/layout-manager", () => ({ - INTENT_PR_REVIEW: "pr-review", + useDockviewStore: { getState: () => dockviewState }, })); vi.mock("@/lib/links", () => ({ replaceTaskUrl: vi.fn(), @@ -33,10 +37,12 @@ import type { TaskSession } from "@/lib/types/http"; const NEW_TASK_ID = "task-new"; const OLD_SESSION_ID = "old-session"; -function makeStore(activeSessionId: string | null): StoreApi { +function makeStore(activeSessionId: string | null, hasLinkedPR = false): StoreApi { const state = { tasks: { activeSessionId }, - taskPRs: { byTaskId: {} as Record }, + taskPRs: { + byTaskId: hasLinkedPR ? { [NEW_TASK_ID]: [{}] } : ({} as Record), + }, environmentIdBySessionId: activeSessionId ? { [activeSessionId]: "env-old" } : {}, }; return { @@ -186,6 +192,7 @@ function makeDeferredSessionLoader() { describe("prepareAndSwitchTask — outgoing-env panel cleanup", () => { beforeEach(() => { vi.clearAllMocks(); + dockviewState.api = null; }); it("releases the outgoing env's panels before awaiting launchSession", async () => { @@ -257,6 +264,26 @@ describe("prepareAndSwitchTask — outgoing-env panel cleanup", () => { expect(switchToSession).not.toHaveBeenCalled(); expect(setPreparingTaskId).toHaveBeenLastCalledWith(null); }); + + it("does not rebuild a user's layout when the task has a linked PR", async () => { + vi.mocked(launchSession).mockResolvedValue({ + success: true, + task_id: NEW_TASK_ID, + session_id: "new-session", + state: "ready", + }); + dockviewState.api = {}; + + const result = await prepareAndSwitchTask( + NEW_TASK_ID, + makeStore(OLD_SESSION_ID, true), + vi.fn(), + vi.fn(), + ); + + expect(result).toBe(true); + expect(dockviewState.buildDefaultLayout).not.toHaveBeenCalled(); + }); }); describe("buildSwitchToSession", () => { diff --git a/apps/web/components/task/task-select-helpers.ts b/apps/web/components/task/task-select-helpers.ts index a68711ae7c..ee92e07302 100644 --- a/apps/web/components/task/task-select-helpers.ts +++ b/apps/web/components/task/task-select-helpers.ts @@ -7,12 +7,7 @@ import type { StoreApi } from "zustand"; import type { AppState } from "@/lib/state/store"; import type { TaskSession } from "@/lib/types/http"; -import { - performLayoutSwitch, - releaseLayoutToDefault, - useDockviewStore, -} from "@/lib/state/dockview-store"; -import { INTENT_PR_REVIEW } from "@/lib/state/layout-manager"; +import { performLayoutSwitch, releaseLayoutToDefault } from "@/lib/state/dockview-store"; import { replaceTaskUrl } from "@/lib/links"; import { launchSession } from "@/lib/services/session-launch-service"; import { buildPrepareRequest } from "@/lib/services/session-launch-helpers"; @@ -154,10 +149,6 @@ export async function prepareAndSwitchTask( // switchEnvLayout would call saveOutgoingEnv(envA) a second time and // overwrite envA's correctly-persisted layout with the default. switchToSession(taskId, resp.session_id, null); - if ((store.getState().taskPRs.byTaskId[taskId]?.length ?? 0) > 0) { - const { api, buildDefaultLayout } = useDockviewStore.getState(); - if (api) buildDefaultLayout(api, INTENT_PR_REVIEW); - } return true; } return false; diff --git a/apps/web/e2e/helpers/layout-assertions.ts b/apps/web/e2e/helpers/layout-assertions.ts index adc4f1e1f4..5fe105b18a 100644 --- a/apps/web/e2e/helpers/layout-assertions.ts +++ b/apps/web/e2e/helpers/layout-assertions.ts @@ -5,9 +5,10 @@ import { expect, type Locator } from "@playwright/test"; // duplicating the DOM walk caused review churn. /** - * Asserts that no descendant of `root` has a right edge past `root`'s right - * edge. `label` shows up in the failure message so the offending step is - * identifiable. + * Asserts that no visible descendant of `root` has a right edge past `root`'s + * right edge. DOM bounds that are clipped inside a nested scroll container do + * not constitute a visual overflow. `label` shows up in the failure message + * so the offending step is identifiable. */ export async function assertNoDescendantOverflowsRight( root: Locator, @@ -26,12 +27,25 @@ export async function assertNoDescendantOverflowsRight( // SVG elements in HTML documents report `tagName` lowercase (per the // SVG namespace) while HTML elements report uppercase, so normalize. const skip = new Set(["svg", "path", "circle", "rect", "line", "g"]); + const clippingOverflow = new Set(["auto", "clip", "hidden", "scroll"]); + const isClippedBeforeLimit = (element: Element): boolean => { + for ( + let parent = element.parentElement; + parent && parent !== node; + parent = parent.parentElement + ) { + const overflowX = getComputedStyle(parent).overflowX; + if (!clippingOverflow.has(overflowX)) continue; + if (parent.getBoundingClientRect().right <= limit + 1) return true; + } + return false; + }; const all = node.querySelectorAll("*"); for (const el of all) { if (skip.has(el.tagName.toLowerCase())) continue; const rect = el.getBoundingClientRect(); if (rect.width === 0 || rect.height === 0) continue; - if (rect.right > limit + 1) { + if (rect.right > limit + 1 && !isClippedBeforeLimit(el)) { results.push({ tag: el.tagName.toLowerCase(), text: (el.textContent ?? "").trim().slice(0, 80), diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 07f45e2857..832280611d 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -600,7 +600,7 @@ export class SessionPage { return this.page.getByTestId("pr-topbar-button"); } - /** PR detail panel (auto-shown when task has an associated PR). */ + /** PR detail panel content when a linked GitHub pull request is selected. */ prDetailPanel(): Locator { return this.page.getByTestId("pr-detail-panel"); } @@ -794,81 +794,11 @@ export class SessionPage { }).toPass({ timeout: 10_000 }); } - /** - * Assert the `pr-detail` panel's dockview group contains at least one - * `session:{sessionId}` panel — i.e. the PR opened as a tab next to a - * session chat, not as a split in a separate group. Regression guard for - * the "PR opens in a split instead of the center tab" bug. - * - * Checks group membership of the PR panel directly rather than picking a - * session panel first, so the assertion is deterministic even when outgoing - * and incoming session panels briefly coexist during a task switch. - */ - async expectPrPanelAndSessionShareGroup(): Promise { - const result = await this.page.evaluate(() => { - type Panel = { id: string; group?: { id?: string } }; - type Api = { panels: Panel[]; getPanel: (i: string) => Panel | undefined }; - const api = (window as unknown as { __dockviewApi__?: Api }).__dockviewApi__; - if (!api) return { error: "dockview api not exposed" }; - const pr = api.getPanel("pr-detail"); - if (!pr) return { error: "pr-detail panel missing" }; - const prGroupId = pr.group?.id ?? null; - const sessionPanels = api.panels.filter((p) => p.id.startsWith("session:")); - if (sessionPanels.length === 0) return { error: "no session panel" }; - const sessionInPrGroup = sessionPanels.some((p) => p.group?.id === prGroupId); - return { - sessionInPrGroup, - prGroupId, - sessionLocations: sessionPanels.map((p) => `${p.id}@${p.group?.id ?? "?"}`), - }; - }); - expect(result.error, result.error).toBeUndefined(); - expect( - result.sessionInPrGroup, - `PR panel landed in a dockview group that contains no session chat. ` + - `PR group=${result.prGroupId} sessions=[${result.sessionLocations?.join(", ")}]`, - ).toBe(true); - } - - /** - * Like `expectPrPanelAndSessionShareGroup`, but matches any pr-detail panel - * (legacy `pr-detail` or keyed `pr-detail|owner/repo/N`). Use for flows that - * exercise the manual click path, which always creates a keyed panel. - */ - async expectAnyPrPanelAndSessionShareGroup(): Promise { - const result = await this.page.evaluate(() => { - type Panel = { id: string; group?: { id?: string } }; - type Api = { panels: Panel[]; getPanel: (i: string) => Panel | undefined }; - const api = (window as unknown as { __dockviewApi__?: Api }).__dockviewApi__; - if (!api) return { error: "dockview api not exposed" }; - const prPanels = api.panels.filter( - (p) => p.id === "pr-detail" || p.id.startsWith("pr-detail|"), - ); - if (prPanels.length === 0) return { error: "no pr-detail panel" }; - const sessionPanels = api.panels.filter((p) => p.id.startsWith("session:")); - if (sessionPanels.length === 0) return { error: "no session panel" }; - const sessionGroupIds = new Set(sessionPanels.map((p) => p.group?.id).filter(Boolean)); - const allPrsInSessionGroup = prPanels.every((p) => { - const gid = p.group?.id; - return gid !== undefined && sessionGroupIds.has(gid); - }); - return { - allPrsInSessionGroup, - prLocations: prPanels.map((p) => `${p.id}@${p.group?.id ?? "?"}`), - sessionLocations: sessionPanels.map((p) => `${p.id}@${p.group?.id ?? "?"}`), - }; - }); - expect(result.error, result.error).toBeUndefined(); - expect( - result.allPrsInSessionGroup, - `PR panel(s) landed outside the session's group. ` + - `prs=[${result.prLocations?.join(", ")}] sessions=[${result.sessionLocations?.join(", ")}]`, - ).toBe(true); - } - - /** Dockview tab for the PR detail panel (title starts as "Pull Request", updated to "PR #N"). */ + /** Dockview tab for canonical or keyed PR detail content. */ prDetailTab(): Locator { - return this.page.locator(".dv-default-tab").filter({ hasText: /^(Pull Request|PR #\d+)$/ }); + return this.page + .locator(".dv-default-tab") + .filter({ hasText: /^(PR Details|Pull Request|PR #\d+)$/ }); } /** Click a dockview tab by its visible label (e.g. "Changes", "Files", "Terminal"). */ diff --git a/apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts b/apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts deleted file mode 100644 index 42278ea9a5..0000000000 --- a/apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts +++ /dev/null @@ -1,858 +0,0 @@ -import { test, expect } from "../../fixtures/test-base"; -import { KanbanPage } from "../../pages/kanban-page"; -import { SessionPage } from "../../pages/session-page"; - -test.describe("PR detail panel", () => { - /** - * Verifies that the PR detail panel can be opened via the topbar button - * when a task has an associated pull request, and renders PR content. - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (with PR #101) - */ - test("opens PR detail panel via topbar button", async ({ testPage, apiClient, seedData }) => { - test.setTimeout(120_000); - - // --- Seed workflow --- - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Panel Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - // --- Seed mock GitHub data --- - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - - await apiClient.mockGitHubAddPRs([ - { - number: 101, - title: "Fix auth bug", - state: "open", - head_branch: "fix/auth", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 10, - deletions: 2, - }, - ]); - - // --- Create task --- - const taskA = await apiClient.createTask(seedData.workspaceId, "Auth Fix Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - // Navigate to kanban BEFORE moving task - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - // Move task to Working → auto_start → mock agent completes → Done - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - - // Associate PR with Task A - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: "testorg", - repo: "testrepo", - pr_number: 101, - pr_url: "https://github.com/testorg/testrepo/pull/101", - pr_title: "Fix auth bug", - head_branch: "fix/auth", - base_branch: "main", - author_login: "test-user", - additions: 10, - deletions: 2, - }); - - // Wait for task to reach Done - await expect(kanban.taskCardInColumn("Auth Fix Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - // --- Open Task A --- - await kanban.taskCardInColumn("Auth Fix Task", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - - // Verify PR topbar button appears with PR number - await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); - await expect(session.prTopbarButton()).toContainText("#101"); - - // Click PR topbar button to open the PR detail panel - await session.prTopbarButton().click(); - - // Verify PR detail panel renders with content - await expect(session.prDetailPanel()).toBeVisible({ timeout: 10_000 }); - }); - - /** - * Verifies that the PR detail panel auto-appears when a task has an - * associated PR, and that dismissing it persists across page reload. - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (with PR #201) - */ - test("auto-shows PR panel and persists dismissal across reload", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Dismiss Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAddPRs([ - { - number: 201, - title: "Add feature", - state: "open", - head_branch: "feat/new", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 20, - deletions: 5, - }, - ]); - - const taskA = await apiClient.createTask(seedData.workspaceId, "Feature Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: "testorg", - repo: "testrepo", - pr_number: 201, - pr_url: "https://github.com/testorg/testrepo/pull/201", - pr_title: "Add feature", - head_branch: "feat/new", - base_branch: "main", - author_login: "test-user", - additions: 20, - deletions: 5, - }); - - await expect(kanban.taskCardInColumn("Feature Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - // Open task - await kanban.taskCardInColumn("Feature Task", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - - // Wait for PR data to arrive (topbar button confirms PR is loaded) - await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); - - // 1. PR panel should auto-appear (no button click needed) - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - - // 2. Dismiss the PR panel by closing its tab (hover to reveal close button) - await session.prDetailTab().hover(); - const closeBtn = session.prDetailTab().locator(".dv-default-tab-action"); - await closeBtn.click(); - await expect(session.prDetailTab()).not.toBeVisible({ timeout: 10_000 }); - - // 3. Reload page — panel should stay dismissed - await testPage.reload(); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - - // Give the auto-show hook time to fire (double rAF) — panel should NOT reappear - await testPage.waitForTimeout(1_000); - await expect(session.prDetailTab()).not.toBeVisible(); - - // 4. Can still open it manually via topbar button - await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); - await session.prTopbarButton().click(); - await expect(session.prDetailPanel()).toBeVisible({ timeout: 10_000 }); - }); - - /** - * Verifies that the PR panel is removed when switching to a task without a PR, - * and re-appears when switching back to a task with a PR. - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (with PR #301), Task B (no PR) - */ - test("removes PR panel when switching to task without PR", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Switch Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAddPRs([ - { - number: 301, - title: "Refactor API", - state: "open", - head_branch: "refactor/api", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 30, - deletions: 10, - }, - ]); - - // Create two tasks — only Task A gets a PR - const taskA = await apiClient.createTask(seedData.workspaceId, "API Refactor Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - const taskB = await apiClient.createTask(seedData.workspaceId, "Plain Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - // Move both tasks and associate PR only with Task A - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - await apiClient.moveTask(taskB.id, workflow.id, workingStep.id); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: "testorg", - repo: "testrepo", - pr_number: 301, - pr_url: "https://github.com/testorg/testrepo/pull/301", - pr_title: "Refactor API", - head_branch: "refactor/api", - base_branch: "main", - author_login: "test-user", - additions: 30, - deletions: 10, - }); - - // Wait for both tasks to reach Done - await expect(kanban.taskCardInColumn("API Refactor Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - await expect(kanban.taskCardInColumn("Plain Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - // Open Task A (has PR) — PR panel should auto-appear - await kanban.taskCardInColumn("API Refactor Task", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - - // Switch to Task B (no PR) — PR panel should be removed - await session.clickTaskInSidebar("Plain Task"); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).not.toBeVisible({ timeout: 10_000 }); - - // Switch back to Task A — PR panel should re-appear - await session.clickTaskInSidebar("API Refactor Task"); - await session.waitForLoad(); - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - }); - - /** - * Regression: the PR detail panel must auto-open as a tab inside the - * session's dockview group, not as a split in a separate group. The bug - * triggered intermittently when opening/switching PR-linked tasks because - * `useAutoPRPanel` used a stale `centerGroupId` from the store and fell - * through dockview's default placement (a new group to the right). - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (with PR #401), Task B (with PR #402) - */ - test("auto-shows PR panel as a tab in the session group, not a split", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Placement Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAddPRs([ - { - number: 401, - title: "First PR", - state: "open", - head_branch: "feat/first", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 10, - deletions: 2, - }, - { - number: 402, - title: "Second PR", - state: "open", - head_branch: "feat/second", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 20, - deletions: 4, - }, - ]); - - const taskA = await apiClient.createTask(seedData.workspaceId, "First PR Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - const taskB = await apiClient.createTask(seedData.workspaceId, "Second PR Task", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - await apiClient.moveTask(taskB.id, workflow.id, workingStep.id); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: "testorg", - repo: "testrepo", - pr_number: 401, - pr_url: "https://github.com/testorg/testrepo/pull/401", - pr_title: "First PR", - head_branch: "feat/first", - base_branch: "main", - author_login: "test-user", - additions: 10, - deletions: 2, - }); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskB.id, - owner: "testorg", - repo: "testrepo", - pr_number: 402, - pr_url: "https://github.com/testorg/testrepo/pull/402", - pr_title: "Second PR", - head_branch: "feat/second", - base_branch: "main", - author_login: "test-user", - additions: 20, - deletions: 4, - }); - - await expect(kanban.taskCardInColumn("First PR Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - await expect(kanban.taskCardInColumn("Second PR Task", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - // Open Task A and wait for the PR panel to auto-show. - await kanban.taskCardInColumn("First PR Task", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - - // Invariant: PR panel shares the session's dockview group (is a tab, not a split). - await session.expectPrPanelAndSessionShareGroup(); - - // Switch to Task B — the race path that triggered the original bug. The - // session changes, a new layout is resolved, and the PR auto-add hook runs - // against the new session. Assert the invariant still holds. - await session.clickTaskInSidebar("Second PR Task"); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - await session.expectPrPanelAndSessionShareGroup(); - - // Switch BACK to Task A — regression: the fast-path layout switch used to - // remove the old session tab then look for a chat/session panel to anchor - // the new tab. With the old tab gone and the PR panel being non-chat, the - // lookup failed and fell through to a sidebar split, placing the session - // tab in a new group while the PR panel stayed in the old center group. - await session.clickTaskInSidebar("First PR Task"); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - await session.expectPrPanelAndSessionShareGroup(); - }); - - /** - * General correctness coverage for the auto-shown PR panel across task - * switches (each task keeps its own dockview layout in this environment, - * so this does not reproduce the exact stale-panel-reuse window flagged - * by Greptile/cubic-dev-ai on PR #1636 — that fix is unit-tested directly - * against `runAutoPRPanelEffect`'s resync logic in - * use-auto-pr-panel.test.ts). This still guards the user-visible - * contract: the panel must show the CURRENTLY active task's PR. - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (with PR #401), Task B (with PR #402) - */ - test("shows the correct task's PR after switching between two PR-linked tasks", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Refresh Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAddPRs([ - { - number: 401, - title: "First task PR", - state: "open", - head_branch: "feat/first", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 5, - deletions: 1, - }, - { - number: 402, - title: "Second task PR", - state: "open", - head_branch: "feat/second", - base_branch: "main", - author_login: "test-user", - repo_owner: "testorg", - repo_name: "testrepo", - additions: 8, - deletions: 2, - }, - ]); - - const taskA = await apiClient.createTask(seedData.workspaceId, "Refresh Task A", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - const taskB = await apiClient.createTask(seedData.workspaceId, "Refresh Task B", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - await apiClient.moveTask(taskB.id, workflow.id, workingStep.id); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: "testorg", - repo: "testrepo", - pr_number: 401, - pr_url: "https://github.com/testorg/testrepo/pull/401", - pr_title: "First task PR", - head_branch: "feat/first", - base_branch: "main", - author_login: "test-user", - additions: 5, - deletions: 1, - }); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskB.id, - owner: "testorg", - repo: "testrepo", - pr_number: 402, - pr_url: "https://github.com/testorg/testrepo/pull/402", - pr_title: "Second task PR", - head_branch: "feat/second", - base_branch: "main", - author_login: "test-user", - additions: 8, - deletions: 2, - }); - - await expect(kanban.taskCardInColumn("Refresh Task A", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - await expect(kanban.taskCardInColumn("Refresh Task B", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - await kanban.taskCardInColumn("Refresh Task A", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toHaveText("PR #401", { timeout: 15_000 }); - - await session.clickTaskInSidebar("Refresh Task B"); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toHaveText("PR #402", { timeout: 15_000 }); - - await session.clickTaskInSidebar("Refresh Task A"); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toHaveText("PR #401", { timeout: 15_000 }); - }); - - /** - * Contract coverage for switching to a different task's PR while its - * feedback (reviews/comments/checks) is still in flight: the tab/header - * must reflect the new task's PR immediately, and once the held fetch - * resolves, its "Bot comments" disclosure must render collapsed (not - * inheriting whatever the previous PR's disclosure was left in). - * - * This harness gives each task its own isolated worktree/env, so the - * auto-shown "pr-detail" panel is freshly mounted per task here and this - * does not reproduce the exact same-React-instance staleness window - * reported in production (reused instance carrying over `usePRFeedback` - * state and `CommentsSection`'s bot-comments toggle across tasks sharing - * one env — same category of limitation already called out on the - * "switching between two PR-linked tasks" test above). That reused-instance - * regression is covered deterministically by - * `hooks/domains/github/use-pr-feedback.test.ts` (masks stale feedback by - * PR identity) and `components/github/pr-detail-panel.test.tsx` (proves the - * bot-comments toggle doesn't leak across a switch on one panel instance). - * This spec still guards the real browser flow end-to-end. - * - * Setup: - * Inbox → Working (auto_start, on_turn_complete → Done) → Done - * Task A (PR #601, one bot comment), Task B (PR #602, one bot comment, - * feedback fetch held back until explicitly released) - */ - test("shows the new task's PR immediately and renders its bot-comments disclosure collapsed while switching with feedback in flight", async ({ - testPage, - apiClient, - seedData, - }) => { - test.setTimeout(120_000); - - const workflow = await apiClient.createWorkflow(seedData.workspaceId, "PR Leak Workflow"); - - const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0); - const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1); - const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2); - - await apiClient.updateWorkflowStep(workingStep.id, { - prompt: 'e2e:message("done")\n{{task_prompt}}', - events: { - on_enter: [{ type: "auto_start_agent" }], - on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }], - }, - }); - - await apiClient.saveUserSettings({ - workspace_id: seedData.workspaceId, - workflow_filter_id: workflow.id, - enable_preview_on_click: false, - }); - - const OWNER = "testorg"; - const REPO = "testrepo"; - const PR_A = 601; - const PR_B = 602; - - await apiClient.mockGitHubReset(); - await apiClient.mockGitHubSetUser("test-user"); - await apiClient.mockGitHubAddPRs([ - { - number: PR_A, - title: "Leak Task A PR", - state: "open", - head_branch: "feat/leak-a", - base_branch: "main", - author_login: "test-user", - repo_owner: OWNER, - repo_name: REPO, - additions: 3, - deletions: 1, - }, - { - number: PR_B, - title: "Leak Task B PR", - state: "open", - head_branch: "feat/leak-b", - base_branch: "main", - author_login: "test-user", - repo_owner: OWNER, - repo_name: REPO, - additions: 4, - deletions: 2, - }, - ]); - - const taskA = await apiClient.createTask(seedData.workspaceId, "Leak Task A", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - const taskB = await apiClient.createTask(seedData.workspaceId, "Leak Task B", { - workflow_id: workflow.id, - workflow_step_id: inboxStep.id, - agent_profile_id: seedData.agentProfileId, - repository_ids: [seedData.repositoryId], - }); - - const kanban = new KanbanPage(testPage); - await kanban.goto(); - - await apiClient.moveTask(taskA.id, workflow.id, workingStep.id); - await apiClient.moveTask(taskB.id, workflow.id, workingStep.id); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskA.id, - owner: OWNER, - repo: REPO, - pr_number: PR_A, - pr_url: `https://github.com/${OWNER}/${REPO}/pull/${PR_A}`, - pr_title: "Leak Task A PR", - head_branch: "feat/leak-a", - base_branch: "main", - author_login: "test-user", - additions: 3, - deletions: 1, - }); - await apiClient.mockGitHubAssociateTaskPR({ - task_id: taskB.id, - owner: OWNER, - repo: REPO, - pr_number: PR_B, - pr_url: `https://github.com/${OWNER}/${REPO}/pull/${PR_B}`, - pr_title: "Leak Task B PR", - head_branch: "feat/leak-b", - base_branch: "main", - author_login: "test-user", - additions: 4, - deletions: 2, - }); - - await apiClient.mockGitHubSeedPRFeedback({ - owner: OWNER, - repo: REPO, - pr_number: PR_A, - comments: [ - { - id: 1, - author: "review-bot", - author_is_bot: true, - body: "PR A bot comment", - }, - ], - }); - await apiClient.mockGitHubSeedPRFeedback({ - owner: OWNER, - repo: REPO, - pr_number: PR_B, - comments: [ - { - id: 2, - author: "review-bot", - author_is_bot: true, - body: "PR B bot comment", - }, - ], - }); - - await expect(kanban.taskCardInColumn("Leak Task A", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - await expect(kanban.taskCardInColumn("Leak Task B", doneStep.id)).toBeVisible({ - timeout: 45_000, - }); - - // Hold PR B's feedback fetch until explicitly released, so the switch - // below lands while its comments are still loading. - let releasePRBFeedback!: () => void; - const prBFeedbackHeld = new Promise((resolve) => { - releasePRBFeedback = resolve; - }); - let observePRBFeedbackRequested!: () => void; - const prBFeedbackRequested = new Promise((resolve) => { - observePRBFeedbackRequested = resolve; - }); - await testPage.route( - (url) => url.pathname === `/api/v1/github/prs/${OWNER}/${REPO}/${PR_B}`, - async (route) => { - const response = await route.fetch(); - observePRBFeedbackRequested(); - await prBFeedbackHeld; - await route.fulfill({ response }); - }, - ); - - await kanban.taskCardInColumn("Leak Task A", doneStep.id).click(); - await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); - - const session = new SessionPage(testPage); - await session.waitForLoad(); - await session.waitForChatIdle({ timeout: 30_000 }); - await expect(session.prDetailTab()).toHaveText(`PR #${PR_A}`, { timeout: 15_000 }); - await session.prDetailTab().click(); - - const panel = session.prDetailPanel(); - await expect(panel).toBeVisible({ timeout: 10_000 }); - await expect(panel.getByText("Bot comments (1)")).toBeVisible({ timeout: 15_000 }); - await panel.getByText("Bot comments (1)").click(); - await expect(panel.getByText("PR A bot comment")).toBeVisible(); - - // Switch to Task B — its feedback request is held back by the route above. - await session.clickTaskInSidebar("Leak Task B"); - await prBFeedbackRequested; - - // The header/tab already reflects Task B's PR (comes from the task-PR - // association, not the held feedback fetch) … - await expect(session.prDetailTab()).toHaveText(`PR #${PR_B}`, { timeout: 15_000 }); - await session.prDetailTab().click(); - await expect(panel).toBeVisible({ timeout: 10_000 }); - // … but PR A's comment, and its expanded bot-comments disclosure, must - // be gone immediately — not left on screen for the several seconds the - // feedback fetch is still in flight. - await expect(panel.getByText("PR A bot comment")).toHaveCount(0); - - releasePRBFeedback(); - - // PR B's bot comment must stay collapsed by default — the disclosure - // must not inherit Task A's expanded state — and must never be PR A's. - await expect(panel.getByText("Bot comments (1)")).toBeVisible({ timeout: 15_000 }); - await expect(panel.getByText("PR B bot comment")).toHaveCount(0); - await panel.getByText("Bot comments (1)").click(); - await expect(panel.getByText("PR B bot comment")).toBeVisible(); - await expect(panel.getByText("PR A bot comment")).toHaveCount(0); - }); -}); diff --git a/apps/web/e2e/tests/pr/pr-detail-dedup.spec.ts b/apps/web/e2e/tests/pr/pr-detail-dedup.spec.ts index aba517c904..6944d56d03 100644 --- a/apps/web/e2e/tests/pr/pr-detail-dedup.spec.ts +++ b/apps/web/e2e/tests/pr/pr-detail-dedup.spec.ts @@ -4,17 +4,15 @@ import { SessionPage } from "../../pages/session-page"; test.describe("PR detail panel dedup", () => { /** - * Regression: clicking the topbar PR button when the PR panel is already - * auto-shown must focus the existing tab, not create a duplicate. The bug - * was that auto-add used panel id `pr-detail` while the topbar button used - * the keyed id `pr-detail|//`, so `focusOrAddPanel`'s id - * dedup missed and the user ended up with two "Pull Request" tabs. + * Clicking the topbar PR button for the review already shown by the + * layout-owned canonical panel must focus that tab instead of creating a + * keyed duplicate. * * Setup: * Inbox → Working (auto_start, on_turn_complete → Done) → Done * Task A (with PR #501) */ - test("topbar PR button focuses the auto-shown panel instead of duplicating it", async ({ + test("topbar PR button focuses the canonical panel instead of duplicating it", async ({ testPage, apiClient, seedData, @@ -87,7 +85,7 @@ test.describe("PR detail panel dedup", () => { timeout: 45_000, }); - // Open task — auto-show should produce the PR tab. + // Open task — the Default layout supplies the canonical PR Details tab. await kanban.taskCardInColumn("Dedup PR Task", doneStep.id).click(); await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); @@ -95,7 +93,7 @@ test.describe("PR detail panel dedup", () => { await session.waitForLoad(); await session.waitForChatIdle({ timeout: 30_000 }); - // PR auto-panel is open before any click on the topbar button. + // Canonical panel is open before any click on the topbar button. await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); await expect(session.prDetailTab()).toHaveCount(1); await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); diff --git a/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts b/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts new file mode 100644 index 0000000000..734426a2b0 --- /dev/null +++ b/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts @@ -0,0 +1,172 @@ +import { expect, type Page } from "@playwright/test"; +import { test, type SeedData } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import { SessionPage } from "../../pages/session-page"; + +const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"]; +const PR_NUMBER = 701; + +type ReviewLayout = { + canonicalGroupId: string | null; + canonicalPRKey: string | null; + keyedPanelIds: string[]; + rightTopOrder: string[]; +}; + +async function createTaskWithSession(apiClient: ApiClient, seedData: SeedData, title: string) { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + if (!task.session_id) throw new Error(`${title} did not create a session`); + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(task.id); + return DONE_STATES.includes(sessions[0]?.state ?? ""); + }, + { timeout: 45_000, message: `Waiting for ${title} session to settle` }, + ) + .toBe(true); + return task; +} + +async function openTask(page: Page, taskId: string): Promise { + await page.goto(`/t/${taskId}`); + const session = new SessionPage(page); + await session.waitForLoad(); + await session.waitForDockviewReady(); + return session; +} + +async function readReviewLayout(page: Page): Promise { + return page.evaluate(() => { + type Panel = { + id: string; + params?: { prKey?: string }; + group?: { id?: string; panels?: Panel[] }; + }; + type Api = { getPanel: (id: string) => Panel | undefined; panels: Panel[] }; + const api = (window as unknown as { __dockviewApi__?: Api }).__dockviewApi__; + if (!api) return null; + const canonical = api.getPanel("pr-detail"); + const files = api.getPanel("files"); + return { + canonicalGroupId: canonical?.group?.id ?? null, + canonicalPRKey: canonical?.params?.prKey ?? null, + keyedPanelIds: api.panels + .filter((panel) => panel.id.startsWith("pr-detail|")) + .map((panel) => panel.id), + rightTopOrder: files?.group?.panels?.map((panel) => panel.id) ?? [], + }; + }); +} + +async function seedMockPR(apiClient: ApiClient): Promise { + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("test-user"); + await apiClient.mockGitHubAddPRs([ + { + number: PR_NUMBER, + title: "Layout-owned review", + state: "open", + head_branch: "feat/pr-details-layout", + base_branch: "main", + author_login: "test-user", + repo_owner: "testorg", + repo_name: "testrepo", + additions: 10, + deletions: 2, + }, + ]); +} + +async function linkPR(apiClient: ApiClient, taskId: string): Promise { + await apiClient.mockGitHubAssociateTaskPR({ + task_id: taskId, + owner: "testorg", + repo: "testrepo", + pr_number: PR_NUMBER, + pr_url: `https://github.com/testorg/testrepo/pull/${PR_NUMBER}`, + pr_title: "Layout-owned review", + head_branch: "feat/pr-details-layout", + base_branch: "main", + author_login: "test-user", + additions: 10, + deletions: 2, + }); +} + +test.describe("PR Details layout panel", () => { + test("keeps the Default panel beside Agent and syncs linked review content", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await seedMockPR(apiClient); + const task = await createTaskWithSession(apiClient, seedData, "PR Details default layout"); + const session = await openTask(testPage, task.id); + + await expect + .poll(() => readReviewLayout(testPage), { timeout: 15_000 }) + .toMatchObject({ + canonicalGroupId: "group-center", + rightTopOrder: ["files", "changes"], + }); + + await expect(session.prDetailTab()).toBeVisible(); + await session.prDetailTab().click(); + await expect(testPage.getByText("No pull request linked to this session.")).toBeVisible(); + + await linkPR(apiClient, task.id); + await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); + await expect + .poll(() => readReviewLayout(testPage), { timeout: 15_000 }) + .toMatchObject({ canonicalPRKey: `testorg/testrepo/${PR_NUMBER}`, keyedPanelIds: [] }); + + await session.prTopbarButton().click(); + await expect(session.prDetailPanel()).toBeVisible(); + await expect.poll(() => readReviewLayout(testPage)).toMatchObject({ keyedPanelIds: [] }); + }); + + test("does not recreate PR Details after the user removes it", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + await seedMockPR(apiClient); + const task = await createTaskWithSession(apiClient, seedData, "Removed PR Details layout"); + const session = await openTask(testPage, task.id); + + const panelTab = session.prDetailTab(); + await expect(panelTab).toBeVisible(); + await panelTab.hover(); + await panelTab.locator(".dv-default-tab-action").click(); + await expect(panelTab).not.toBeVisible(); + await expect + .poll(() => readReviewLayout(testPage)) + .toMatchObject({ + canonicalGroupId: null, + keyedPanelIds: [], + }); + + await linkPR(apiClient, task.id); + await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); + await testPage.waitForTimeout(500); + await expect + .poll(() => readReviewLayout(testPage)) + .toMatchObject({ + canonicalGroupId: null, + keyedPanelIds: [], + }); + }); +}); diff --git a/apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts b/apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts index 92f74ecc8b..2734f59c14 100644 --- a/apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts +++ b/apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts @@ -4,9 +4,9 @@ import { SessionPage } from "../../pages/session-page"; test.describe("PR detail panel — manual open", () => { /** - * Regression: when the user dismisses the auto-shown PR panel and re-opens - * it manually via the topbar PR button, the new panel must land as a tab - * inside the session's dockview group — not as a separate split group. + * Regression: when the user removes the layout-owned PR Details panel and + * opens a pull request manually from the topbar, the keyed review tab must + * use the center fallback — not create a separate split. * * The manual path uses `addPRPanel(prKey)` which historically anchored on * the store's `centerGroupId`. That value can be stale across layout @@ -17,7 +17,7 @@ test.describe("PR detail panel — manual open", () => { * Inbox → Working (auto_start, on_turn_complete → Done) → Done * Task A (with PR #501) */ - test("places PR panel as a tab in the session group when opened from topbar", async ({ + test("uses the center fallback when PR Details is absent", async ({ testPage, apiClient, seedData, @@ -93,7 +93,7 @@ test.describe("PR detail panel — manual open", () => { timeout: 45_000, }); - // Open the task — the auto-show hook adds the legacy `pr-detail` panel. + // Open the task — the Default layout provides the canonical PR Details panel. await kanban.taskCardInColumn("Order Basket Task", doneStep.id).click(); await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); @@ -103,22 +103,35 @@ test.describe("PR detail panel — manual open", () => { await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 }); await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 }); - // Dismiss the auto-shown PR panel. The auto-show hook already marked it - // as "offered" on add, so it won't re-add it — the next open path is the - // manual one. + // Remove the canonical panel from the task layout. Its absence is a user + // choice, so review data must not recreate it. await session.prDetailTab().hover(); const closeBtn = session.prDetailTab().locator(".dv-default-tab-action"); await closeBtn.click(); await expect(session.prDetailTab()).not.toBeVisible({ timeout: 10_000 }); - // Click the topbar PR button — exercises addPRPanel(prKey, activeSessionId) - // and creates a keyed `pr-detail|owner/repo/N` panel. + // Click the topbar PR button — this creates a keyed `pr-detail|owner/repo/N` + // panel because the canonical one is no longer present. await session.prTopbarButton().click(); await expect(session.prDetailPanel()).toBeVisible({ timeout: 10_000 }); await expect(session.prDetailTab()).toBeVisible({ timeout: 10_000 }); - // Invariant: PR panel is a tab in the session's dockview group, not a - // split into a separate group. Matches keyed (pr-detail|...) panel id. - await session.expectAnyPrPanelAndSessionShareGroup(); + const location = await testPage.evaluate(() => { + type Panel = { id: string; group?: { id?: string } }; + type Api = { panels: Panel[]; getPanel: (id: string) => Panel | undefined }; + const api = (window as unknown as { __dockviewApi__?: Api }).__dockviewApi__; + if (!api) return { error: "dockview api not exposed" }; + const keyed = api.panels.find((panel) => panel.id.startsWith("pr-detail|")); + const sessionPanel = api.panels.find((panel) => panel.id.startsWith("session:")); + return { + canonicalExists: !!api.getPanel("pr-detail"), + keyedGroupId: keyed?.group?.id ?? null, + sessionGroupId: sessionPanel?.group?.id ?? null, + }; + }); + + expect(location.error, location.error).toBeUndefined(); + expect(location.canonicalExists).toBe(false); + expect(location.keyedGroupId).toBe(location.sessionGroupId); }); }); diff --git a/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts b/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts index 2b072594d6..4a2be7cd98 100644 --- a/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts +++ b/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts @@ -303,13 +303,12 @@ test.describe("Multi-PR CI popover", () => { await associateTwoPRs(apiClient, seed.taskId); const session = await openTaskAndWait(testPage, apiClient, seed, title); - // Auto-shown panel defaults to the primary/first-associated PR (web#42). + // The layout-owned canonical panel follows the primary/first-associated PR (web#42). await expect(session.prDetailTab()).toHaveCount(1, { timeout: 15_000 }); // Regression: selecting the OTHER PR from the "+" add-panel menu must - // open a second, distinct tab instead of repurposing the auto-shown one. - // (Dedup when re-selecting the same PR is covered by the - // runAutoPRPanelEffect / addPRPanel unit tests.) + // open a second, distinct tab instead of repurposing the canonical one. + // (Dedup when re-selecting the same PR is covered by addPRPanel unit tests.) await session.addPanelButton().click(); await testPage.getByTestId(`add-panel-pr-item-${OWNER}-api-77`).click(); await expect(session.prDetailTab()).toHaveCount(2, { timeout: 15_000 }); diff --git a/apps/web/e2e/tests/settings/layout-profiles.spec.ts b/apps/web/e2e/tests/settings/layout-profiles.spec.ts index 56f0c3be11..11e59816dc 100644 --- a/apps/web/e2e/tests/settings/layout-profiles.spec.ts +++ b/apps/web/e2e/tests/settings/layout-profiles.spec.ts @@ -10,6 +10,7 @@ type DockviewSnapshot = { panelIds: string[]; filesGroupId: string | null; changesGroupId: string | null; + prDetailsGroupId: string | null; rightGroupOrder: string[]; }; @@ -98,10 +99,12 @@ async function dockviewSnapshot(page: Page): Promise { if (!api) throw new Error("dockview api not exposed"); const files = api.getPanel("files"); const changes = api.getPanel("changes"); + const prDetails = api.getPanel("pr-detail"); return { panelIds: api.panels.map((panel) => panel.id), filesGroupId: files?.group?.id ?? null, changesGroupId: changes?.group?.id ?? null, + prDetailsGroupId: prDetails?.group?.id ?? null, rightGroupOrder: files?.group?.panels.map((panel) => panel.id) ?? [], }; }); @@ -201,6 +204,43 @@ test.describe("Task layout profile defaults", () => { expect((await apiClient.getUserSettings()).settings.saved_layouts).toEqual([]); }); + test("moves PR Details through the editor and uses the saved group for fresh tasks", async ({ + testPage, + apiClient, + seedData, + prCapture, + }) => { + test.setTimeout(120_000); + const layouts = new LayoutSettingsPage(testPage); + await layouts.open(); + await expect(layouts.editor.locator(".dv-tab", { hasText: "PR Details" })).toBeVisible(); + await layouts.selectPanel("PR Details"); + await prCapture.screenshot("default-pr-details-agent-group", { + caption: "Default layout puts PR Details beside Agent in the center pane", + }); + + await layouts.actions.getByRole("button", { name: "Move tab to split" }).click(); + await testPage.getByRole("menuitem", { name: "Files", exact: true }).click(); + await prCapture.screenshot("pr-details-moved-to-files-group", { + caption: "Layout editor moves PR Details into the Files/Changes pane before saving", + }); + await layouts.save(); + + const saved = (await apiClient.getUserSettings()).settings.saved_layouts[0]; + expect(JSON.stringify(saved.layout)).toContain("pr-detail"); + + const task = await createTaskWithSession(apiClient, seedData, "Moved PR Details Layout Task"); + await openTask(testPage, task.id); + await expect + .poll(() => dockviewSnapshot(testPage), { timeout: 15_000 }) + .toMatchObject({ + filesGroupId: "group-right-top", + changesGroupId: "group-right-top", + prDetailsGroupId: "group-right-top", + rightGroupOrder: ["files", "changes", "pr-detail"], + }); + }); + test("fresh tasks use the no-terminal default while existing tasks wait for Reset Layout", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts b/apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts index 4f2c93d28c..2ccc639aba 100644 --- a/apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts +++ b/apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts @@ -17,6 +17,40 @@ type SavedProfile = { }; test.describe("Mobile layout profiles", () => { + test("moves PR Details with touch controls without horizontal overflow", async ({ + testPage, + apiClient, + prCapture, + }) => { + const layouts = new LayoutSettingsPage(testPage); + await layouts.openFromMobileMenu(); + + const prDetailsTab = layouts.editor.locator(".dv-tab", { hasText: "PR Details" }); + await expect(prDetailsTab).toBeVisible(); + await prDetailsTab.tap(); + await prCapture.screenshot("pr-details-touch-layout-editor", { + caption: "Mobile layout editor selects PR Details through touch controls", + }); + const moveLeft = layouts.actions.getByRole("button", { name: "Move tab left" }); + await expect(moveLeft).toBeEnabled(); + await moveLeft.tap(); + await layouts.save(); + + const response = await apiClient.getUserSettings(); + const profile = (response.settings.saved_layouts as SavedProfile[]).find( + (candidate) => candidate.is_default, + ); + expect(profile, "Default saved layout is present").toBeDefined(); + if (!profile) return; + const prDetailsGroup = profile.layout.columns + .flatMap((column) => column.groups) + .find((group) => group.panels.some((panel) => panel.id === "pr-detail")); + expect(prDetailsGroup?.panels.map((panel) => panel.id)).toEqual(["pr-detail", "chat"]); + + await assertNoDocumentHorizontalOverflow(testPage, "PR Details mobile layout edit"); + await assertNoDescendantOverflowsRight(layouts.root, "PR Details mobile layout settings"); + }); + test("edits and reorders a profile without horizontal overflow", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts b/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts index af1f674110..162aa2110c 100644 --- a/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts +++ b/apps/web/e2e/tests/task/sessionless-switch-panel-leak.spec.ts @@ -13,7 +13,7 @@ import { SessionPage } from "../../pages/session-page"; /** * Regression: switching from a task with env-scoped panels open (file-editor, - * diff-viewer, commit-detail, browser, vscode, pr-detail) to a task that + * diff-viewer, commit-detail, browser, vscode) to a task that * needs an env prepared used to leave those panels mounted in the dockview * for the entire `await launchSession(...)` round-trip + WS env-id * propagation. They surfaced as stray tabs (e.g. a file-editor tab) on the @@ -21,6 +21,8 @@ import { SessionPage } from "../../pages/session-page"; * * Fix: `prepareAndSwitchTask` calls `releaseLayoutToDefault` BEFORE awaiting * the launch, dropping env-scoped portals so the user sees a clean slate. + * `pr-detail` is intentionally excluded: it is now a layout-owned default + * panel, so a fresh default layout correctly includes it during preparation. */ async function setupTaskWithFilePanel(args: { @@ -65,8 +67,7 @@ async function setupTaskWithFilePanel(args: { return { session, filename }; } -/** Read the live dockview component names — file-editor / diff-viewer / - * commit-detail are env-scoped and must NOT survive into a different task. */ +/** Read live component names that are not part of the default layout. */ async function readLiveDockviewComponents(testPage: Page): Promise { return testPage.evaluate(() => { type Panel = { id: string; api?: { component?: string } }; @@ -77,13 +78,12 @@ async function readLiveDockviewComponents(testPage: Page): Promise { }); } -const ENV_SCOPED_COMPONENTS = [ +const NON_DEFAULT_ENV_SCOPED_COMPONENTS = [ "file-editor", "diff-viewer", "commit-detail", "browser", "vscode", - "pr-detail", ]; test.describe("Sessionless task switch — env-scoped panel cleanup", () => { @@ -129,7 +129,7 @@ test.describe("Sessionless task switch — env-scoped panel cleanup", () => { .poll( async () => { const components = await readLiveDockviewComponents(testPage); - return components.some((c) => ENV_SCOPED_COMPONENTS.includes(c)); + return components.some((c) => NON_DEFAULT_ENV_SCOPED_COMPONENTS.includes(c)); }, { timeout: 5_000, diff --git a/apps/web/e2e/tests/task/task-default-layout.spec.ts b/apps/web/e2e/tests/task/task-default-layout.spec.ts index dcb3083949..70dda3a7db 100644 --- a/apps/web/e2e/tests/task/task-default-layout.spec.ts +++ b/apps/web/e2e/tests/task/task-default-layout.spec.ts @@ -39,7 +39,7 @@ test.describe("Task default layout shape", () => { }), { message: "Waiting for every built-in default panel" }, ) - .toEqual(["changes", "files", `session:${task.session_id}`, "terminal-default"]); + .toEqual(["changes", "files", "pr-detail", `session:${task.session_id}`, "terminal-default"]); }); test("entering a task is horizontal (right column beside chat, not stacked)", async ({ diff --git a/apps/web/lib/layout/layout-profiles.test.ts b/apps/web/lib/layout/layout-profiles.test.ts index 3a46153381..be98f827a2 100644 --- a/apps/web/lib/layout/layout-profiles.test.ts +++ b/apps/web/lib/layout/layout-profiles.test.ts @@ -78,8 +78,10 @@ describe("built-in layout profiles", () => { first.layout.columns[0].groups[0].panels.length = 0; expect( - getBuiltInLayoutProfile(DEFAULT_LAYOUT_ID).layout.columns[0].groups[0].panels, - ).toHaveLength(1); + getBuiltInLayoutProfile(DEFAULT_LAYOUT_ID).layout.columns[0].groups[0].panels.map( + (panel) => panel.id, + ), + ).toEqual(["chat", "pr-detail"]); }); }); @@ -90,6 +92,12 @@ describe("validateReusableLayout", () => { expect(result).toMatchObject({ valid: true, issues: [] }); }); + it("accepts canonical PR Details as reusable layout content", () => { + const result = validateReusableLayout(reusableLayout(["chat", "pr-detail"])); + + expect(result).toMatchObject({ valid: true, issues: [] }); + }); + it("normalizes a saved session Agent without changing the input", () => { const input = reusableLayout(); input.columns[0].groups[0].panels[0] = { @@ -110,7 +118,9 @@ describe("validateReusableLayout", () => { }); expect(input).toEqual(snapshot); }); +}); +describe("validateReusableLayout failures", () => { it.each([ { name: "a missing Agent", @@ -124,7 +134,27 @@ describe("validateReusableLayout", () => { }, { name: "an unsupported panel", - layout: reusableLayout(["chat", "pr-detail"]), + layout: { + columns: [ + { + id: CENTER_COLUMN_ID, + groups: [ + { + id: "group-center", + panels: [ + panel("chat"), + { + id: "pr-detail|owner/repository/123", + component: "pr-detail", + title: "Pull Request", + }, + ], + activePanel: "chat", + }, + ], + }, + ], + }, code: "unsupported-panel", }, { diff --git a/apps/web/lib/layout/layout-profiles.ts b/apps/web/lib/layout/layout-profiles.ts index 8b5b1ba356..b28d866b5d 100644 --- a/apps/web/lib/layout/layout-profiles.ts +++ b/apps/web/lib/layout/layout-profiles.ts @@ -27,7 +27,7 @@ export const BUILT_IN_LAYOUT_PROFILES: readonly BuiltInLayoutProfileDescriptor[] { id: "default", name: "Default", - description: "Agent with Files, Changes, and Terminal", + description: "Agent with Files, Changes, PR Details, and Terminal", }, { id: "plan", diff --git a/apps/web/lib/local-storage.ts b/apps/web/lib/local-storage.ts index f006c4f603..112857aaf9 100644 --- a/apps/web/lib/local-storage.ts +++ b/apps/web/lib/local-storage.ts @@ -403,30 +403,6 @@ export function removeEnvMaximizeState(envId: string): void { } } -// PR panel "offered" flag — tracks whether the auto-show PR panel was offered -// for a session. If offered and then closed by the user, we respect the dismissal. -const PR_PANEL_OFFERED_PREFIX = "kandev.pr-panel-offered."; - -/** Whether the auto-show PR panel was already offered for this session. */ -export function wasPRPanelOffered(sessionId: string): boolean { - if (typeof window === "undefined") return false; - try { - return window.sessionStorage.getItem(`${PR_PANEL_OFFERED_PREFIX}${sessionId}`) === "1"; - } catch { - return false; - } -} - -/** Record that the auto-show PR panel was offered for this session. */ -export function markPRPanelOffered(sessionId: string): void { - if (typeof window === "undefined") return; - try { - window.sessionStorage.setItem(`${PR_PANEL_OFFERED_PREFIX}${sessionId}`, "1"); - } catch { - // Ignore write failures - } -} - // PR merged banner dismissal — per-task, survives reload + task switch within // the tab session, resets on tab close. const PR_MERGED_BANNER_DISMISSED_PREFIX = "kandev.pr-merged-banner-dismissed."; @@ -765,7 +741,6 @@ export function cleanupTaskStorage( // Session-keyed storage — drafts, files panel state, scroll, etc. for (const sessionId of sessionIds) { removeStoredQuickChatName(sessionId); - removeSessionStorage(`${PR_PANEL_OFFERED_PREFIX}${sessionId}`); removeSessionStorage(`${CHAT_DRAFT_TEXT_KEY}.${sessionId}`); removeSessionStorage(`${CHAT_DRAFT_CONTENT_KEY}.${sessionId}`); removeSessionStorage(`${CHAT_DRAFT_ATTACHMENTS_KEY}.${sessionId}`); diff --git a/apps/web/lib/state/dockview-env-switch-right-width.test.ts b/apps/web/lib/state/dockview-env-switch-right-width.test.ts new file mode 100644 index 0000000000..3915508938 --- /dev/null +++ b/apps/web/lib/state/dockview-env-switch-right-width.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; +import type { SerializedDockview } from "dockview-react"; +import { savedRightColumnWidth } from "./dockview-env-switch"; + +const CENTER_GROUP_ID = "group-center"; + +function makeSaved(children: Array<{ id: string; size: number }>): SerializedDockview { + return { + grid: { + root: { + type: "branch", + data: children.map((child) => ({ + type: "leaf", + data: { id: child.id, views: [] }, + size: child.size, + })), + }, + height: 600, + width: 1600, + orientation: "HORIZONTAL", + }, + panels: {}, + activeGroup: undefined, + } as unknown as SerializedDockview; +} + +describe("savedRightColumnWidth", () => { + it("returns the saved right size for a 3-column layout (sidebar+center+right)", () => { + const saved = makeSaved([ + { id: "group-sidebar", size: 300 }, + { id: CENTER_GROUP_ID, size: 1000 }, + { id: "group-right-top", size: 300 }, + ]); + + expect(savedRightColumnWidth(saved)).toBe(300); + }); + + it("returns the saved right size for a 2-column layout with sidebar hidden", () => { + const saved = makeSaved([ + { id: CENTER_GROUP_ID, size: 1380 }, + { id: "group-right-top", size: 220 }, + ]); + + expect(savedRightColumnWidth(saved)).toBe(220); + }); + + it("returns undefined when the last 2-column child is not a right column", () => { + const saved = makeSaved([ + { id: "group-sidebar", size: 300 }, + { id: CENTER_GROUP_ID, size: 1300 }, + ]); + + expect(savedRightColumnWidth(saved)).toBeUndefined(); + }); + + it("returns undefined for null input", () => { + expect(savedRightColumnWidth(null)).toBeUndefined(); + }); +}); diff --git a/apps/web/lib/state/dockview-env-switch.test.ts b/apps/web/lib/state/dockview-env-switch.test.ts index 9bf7435702..0894634745 100644 --- a/apps/web/lib/state/dockview-env-switch.test.ts +++ b/apps/web/lib/state/dockview-env-switch.test.ts @@ -1,10 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { - performEnvSwitch, - savedRightColumnWidth, - type EnvSwitchParams, -} from "./dockview-env-switch"; -import type { SerializedDockview } from "dockview-react"; +import { performEnvSwitch, type EnvSwitchParams } from "./dockview-env-switch"; const { CANONICAL_CENTER_GROUP_ID, RIGHT_TOP_GROUP_ID, RIGHT_BOTTOM_GROUP_ID } = vi.hoisted(() => ({ CANONICAL_CENTER_GROUP_ID: "group-center", @@ -233,6 +228,33 @@ describe("performEnvSwitch", () => { }); }); +describe("performEnvSwitch layout-owned review panel", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("keeps layout-owned PR Details during a fresh fast switch", () => { + vi.mocked(layoutStructuresMatch).mockReturnValueOnce(true); + const close = vi.fn(); + const prDetails = { + id: "pr-detail", + api: { component: "pr-detail", close }, + group: { id: RIGHT_TOP_GROUP_ID, panels: [] }, + }; + const params = makeParams({ + api: { + ...makeMockApi(), + panels: [prDetails], + groups: [prDetails.group], + } as unknown as EnvSwitchParams["api"], + }); + + performEnvSwitch(params); + + expect(close).not.toHaveBeenCalled(); + }); +}); + describe("performEnvSwitch fast-path group survival", () => { beforeEach(() => { vi.clearAllMocks(); @@ -596,6 +618,37 @@ describe("performEnvSwitch missing Agent restoration", () => { ); expect(setActive).not.toHaveBeenCalled(); }); + + it("does not use a configured PR Details group as the Agent restoration anchor", () => { + const savedLayout = makeHealthyLayoutWith({ "pr-detail": { contentComponent: "pr-detail" } }); + vi.mocked(getEnvLayout).mockReturnValueOnce(savedLayout).mockReturnValueOnce(savedLayout); + vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(false); + + const fallbackGroup = { id: "fallback-center", panels: [] }; + const reviewGroup = { id: "custom-pr-details-group", panels: [] }; + const reviewPanel = { id: "pr-detail", api: { component: "pr-detail" }, group: reviewGroup }; + const addPanel = vi.fn(() => ({ + id: NEW_SESSION_PANEL_ID, + api: { component: "chat", setActive: vi.fn() }, + group: fallbackGroup, + })); + const api = { + ...makeMockApi(), + panels: [reviewPanel], + groups: [fallbackGroup, reviewGroup], + getPanel: vi.fn((id: string) => (id === "pr-detail" ? reviewPanel : null)), + addPanel, + } as unknown as EnvSwitchParams["api"]; + + performEnvSwitch(makeParams({ api })); + + expect(addPanel).toHaveBeenCalledWith( + expect.objectContaining({ + id: NEW_SESSION_PANEL_ID, + position: expect.objectContaining({ referenceGroup: fallbackGroup.id }), + }), + ); + }); }); describe("performEnvSwitch fast-path active view restoration", () => { @@ -645,59 +698,3 @@ describe("performEnvSwitch fast-path active view restoration", () => { expect(lastRightCall).toBeGreaterThan(lastCenterCall); }); }); - -describe("savedRightColumnWidth", () => { - function makeSaved(children: Array<{ id: string; size: number }>): SerializedDockview { - return { - grid: { - root: { - type: "branch", - data: children.map((c) => ({ - type: "leaf", - data: { id: c.id, views: [] }, - size: c.size, - })), - }, - height: 600, - width: 1600, - orientation: "HORIZONTAL", - }, - panels: {}, - activeGroup: undefined, - } as unknown as SerializedDockview; - } - - it("returns the saved right size for a 3-column layout (sidebar+center+right)", () => { - const saved = makeSaved([ - { id: "group-sidebar", size: 300 }, - { id: CANONICAL_CENTER_GROUP_ID, size: 1000 }, - { id: "group-right-top", size: 300 }, - ]); - expect(savedRightColumnWidth(saved)).toBe(300); - }); - - it("returns the saved right size for a 2-column layout with sidebar hidden", () => { - // Regression: pre-fix this returned undefined (column-count gate), which - // caused the right column to fall back to ~450 default on env switch - // instead of restoring the user's narrow width. - const saved = makeSaved([ - { id: CANONICAL_CENTER_GROUP_ID, size: 1380 }, - { id: "group-right-top", size: 220 }, - ]); - expect(savedRightColumnWidth(saved)).toBe(220); - }); - - it("returns undefined for a 2-column layout where the last child is not a right column", () => { - // 2-column layouts can also be sidebar+center (right hidden); we must NOT - // mistake the center for the right column. - const saved = makeSaved([ - { id: "group-sidebar", size: 300 }, - { id: CANONICAL_CENTER_GROUP_ID, size: 1300 }, - ]); - expect(savedRightColumnWidth(saved)).toBeUndefined(); - }); - - it("returns undefined for null input", () => { - expect(savedRightColumnWidth(null)).toBeUndefined(); - }); -}); diff --git a/apps/web/lib/state/dockview-env-switch.ts b/apps/web/lib/state/dockview-env-switch.ts index 6c73404664..04b806b2b4 100644 --- a/apps/web/lib/state/dockview-env-switch.ts +++ b/apps/web/lib/state/dockview-env-switch.ts @@ -146,7 +146,9 @@ export type EnvSwitchParams = { /** Close non-session env-scoped panels before the new env's panels are restored. */ function removeEphemeralPanels(api: DockviewApi): void { - const toRemove = api.panels.filter((p) => EPHEMERAL_COMPONENTS.has(p.api.component)); + const toRemove = api.panels.filter( + (p) => p.id !== "pr-detail" && EPHEMERAL_COMPONENTS.has(p.api.component), + ); for (const p of toRemove) { try { p.api.close(); @@ -224,7 +226,7 @@ export function replaceStaleSessionPanels( addCurrentSessionSiblings(api, keepSessionId, currentSessionIds); } -const RESTORED_SESSION_ANCHOR_IDS = ["plan", "pr-detail", "mr-detail"]; +const RESTORED_SESSION_ANCHOR_IDS = ["plan"]; function findRestoredSessionGroup(api: DockviewApi): DockviewApi["groups"][number] | undefined { const canonicalCenter = api.groups.find((group) => group.id === CENTER_GROUP); diff --git a/apps/web/lib/state/dockview-panel-actions.test.ts b/apps/web/lib/state/dockview-panel-actions.test.ts index be1b10f2be..ecb6d7bee1 100644 --- a/apps/web/lib/state/dockview-panel-actions.test.ts +++ b/apps/web/lib/state/dockview-panel-actions.test.ts @@ -484,12 +484,7 @@ describe("addPRPanel — dedup with legacy auto-shown panel", () => { return { api, actions: buildExtraPanelActions(store.get) }; } - /** - * Mirrors how useAutoPRPanel seeds the legacy panel in production: same - * unkeyed "pr-detail" id, but stamped with the PR it's currently showing - * so later `addPRPanel` calls can tell whether a menu click targets that - * same PR (reuse the tab) or a different one (open a distinct tab). - */ + /** Canonical PR Details panels retain the key currently shown for the task. */ function seedLegacyPanel(api: DockviewApi, prKey: string): void { api.addPanel({ id: LEGACY_PR_ID, @@ -559,21 +554,19 @@ describe("addPRPanel — dedup with legacy auto-shown panel", () => { }); }); -describe("addPRPanel — group placement", () => { +describe("addPRPanel — layout-owned group placement", () => { const PR_KEY = "testorg/testrepo/202"; const KEYED_PR_ID = `pr-detail|${PR_KEY}`; const SESSION_ID = "s-1"; const SESSION_PANEL_ID = `session:${SESSION_ID}`; const SESSION_GROUP = "group-session-host"; + const PR_DETAILS_GROUP = "group-configured-pr-details"; function buildExtra(api: DockviewApi) { const store = makeStore(api); return { api, actions: buildExtraPanelActions(store.get) }; } - // Seed the api with a session panel in a group that is NOT the store's - // centerGroupId. This mirrors the post-transition state where the store's - // tracked centerGroupId has gone stale and the live session sits elsewhere. function seedSessionInGroup(api: DockviewApi, groupId: string): void { api.addPanel({ id: SESSION_PANEL_ID, @@ -584,44 +577,57 @@ describe("addPRPanel — group placement", () => { }); } - it("places the PR panel in the same group as the active session panel", () => { + function seedCanonicalPRDetails(api: DockviewApi, groupId = PR_DETAILS_GROUP): void { + api.addPanel({ + id: "pr-detail", + component: "pr-detail", + title: "PR Details", + position: { referenceGroup: groupId }, + }); + } + + it("places a new PR tab in PR Details' configured group, not beside Agent", () => { const { api, actions } = buildExtra(makeApi()); seedSessionInGroup(api, SESSION_GROUP); + seedCanonicalPRDetails(api); - actions.addPRPanel(PR_KEY, SESSION_ID); - - const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; - const session = api.getPanel(SESSION_PANEL_ID) as unknown as MockPanel; - expect(pr).toBeDefined(); - expect(pr.group.id).toBe(session.group.id); - expect(pr.group.id).toBe(SESSION_GROUP); - // Critical: must NOT have landed in the store's centerGroupId (the bug). - expect(pr.group.id).not.toBe(CENTER_GROUP); - }); - - it("falls back to centerGroupId when no session panel exists for the id", () => { - const { api, actions } = buildExtra(makeApi()); - // No session panel seeded — resolver should fall back. - actions.addPRPanel(PR_KEY, SESSION_ID); + actions.addPRPanel(PR_KEY); const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; expect(pr).toBeDefined(); - expect(pr.group.id).toBe(CENTER_GROUP); + expect(pr.group.id).toBe(PR_DETAILS_GROUP); + expect(pr.group.id).not.toBe(SESSION_GROUP); }); - it("falls back to centerGroupId when sessionId is not provided", () => { + it("falls back to centerGroupId when no PR Details panel exists", () => { const { api, actions } = buildExtra(makeApi()); - // Even with a session panel present, omitting the id keeps legacy behavior. - seedSessionInGroup(api, SESSION_GROUP); actions.addPRPanel(PR_KEY); const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; expect(pr).toBeDefined(); expect(pr.group.id).toBe(CENTER_GROUP); }); + + it("focuses an existing keyed PR tab without relocating it", () => { + const { api, actions } = buildExtra(makeApi()); + seedCanonicalPRDetails(api); + api.addPanel({ + id: KEYED_PR_ID, + component: "pr-detail", + title: "Pull Request", + params: { prKey: PR_KEY }, + position: { referenceGroup: SESSION_GROUP }, + }); + + actions.addPRPanel(PR_KEY); + + const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(pr.group.id).toBe(SESSION_GROUP); + expect(pr.isActive).toBe(true); + }); }); -describe("addMRPanel — dedup with legacy auto-shown panel", () => { +describe("addMRPanel — layout-owned review tabs", () => { const MR_KEY = "https://gitlab.example.test|platform/kandev|81"; const OTHER_MR_KEY = "https://gitlab.example.test|platform/kandev|82"; const LEGACY_MR_ID = "mr-detail"; @@ -641,24 +647,32 @@ describe("addMRPanel — dedup with legacy auto-shown panel", () => { }); } - it("replaces a matching auto-shown panel with an active keyed panel", () => { + it("focuses a matching legacy panel in place", () => { const { api, actions } = buildExtra(makeApi()); seedLegacyPanel(api, MR_KEY); actions.addMRPanel(MR_KEY); expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(1); - expect(api.getPanel(LEGACY_MR_ID)).toBeUndefined(); - expect((api.getPanel(`${LEGACY_MR_ID}|${MR_KEY}`) as unknown as MockPanel).isActive).toBe(true); + expect(api.getPanel(`${LEGACY_MR_ID}|${MR_KEY}`)).toBeUndefined(); + expect((api.getPanel(LEGACY_MR_ID) as unknown as MockPanel).isActive).toBe(true); }); - it("opens a keyed panel when the auto-shown panel displays a different MR", () => { + it("opens a keyed panel in the canonical PR Details group", () => { const { api, actions } = buildExtra(makeApi()); seedLegacyPanel(api, MR_KEY); + api.addPanel({ + id: "pr-detail", + component: "pr-detail", + title: "PR Details", + position: { referenceGroup: "group-configured-pr-details" }, + }); actions.addMRPanel(OTHER_MR_KEY); - expect(api.getPanel(`${LEGACY_MR_ID}|${OTHER_MR_KEY}`)).toBeDefined(); + const keyed = api.getPanel(`${LEGACY_MR_ID}|${OTHER_MR_KEY}`) as unknown as MockPanel; + expect(keyed).toBeDefined(); + expect(keyed.group.id).toBe("group-configured-pr-details"); expect(api.panels.filter((panel) => panel.id.startsWith(LEGACY_MR_ID))).toHaveLength(2); }); }); diff --git a/apps/web/lib/state/dockview-panel-actions.ts b/apps/web/lib/state/dockview-panel-actions.ts index c7d087cbe4..f13c71d9cf 100644 --- a/apps/web/lib/state/dockview-panel-actions.ts +++ b/apps/web/lib/state/dockview-panel-actions.ts @@ -39,19 +39,6 @@ function focusMatchingLegacyPanel( return true; } -function removeMatchingLegacyPanel( - api: DockviewApi, - keyedPanelId: string, - legacyPanelId: string, - paramName: string, - key: string, -): void { - if (api.getPanel(keyedPanelId)) return; - const legacy = api.getPanel(legacyPanelId); - const legacyKey = (legacy?.params as Record | undefined)?.[paramName]; - if (legacy && legacyKey === key) api.removePanel(legacy); -} - // --------------------------------------------------------------------------- // Preview-tab machinery // --------------------------------------------------------------------------- @@ -455,6 +442,63 @@ export function removeSessionPanel(api: DockviewApi, sessionId: string): void { if (panel) api.removePanel(panel); } +function buildReviewPanelActions(get: StoreGet) { + return { + /** + * Focus an existing PR tab in place, or add a keyed tab beside the + * layout-owned canonical PR Details panel. Fall back to the center group + * when the current layout intentionally omits PR Details. + */ + addPRPanel: (prKey?: string) => { + const { api, centerGroupId } = get(); + if (!api) return; + // Multi-repo: each TaskPR opens in its own panel keyed by + // owner/repo/pr_number so multiple PRs can be tabbed side-by-side. + // Legacy single-repo callers (no key) get the historical panel id. + const id = prKey ? `pr-detail|${prKey}` : "pr-detail"; + // A canonical panel already rendering this key is the task's primary + // review tab; focus it instead of creating a duplicate keyed tab. + const existing = api.getPanel(id); + if (existing) { + existing.api.setActive(); + return; + } + if (prKey && focusMatchingLegacyPanel(api, id, "pr-detail", "prKey", prKey)) return; + const targetGroupId = api.getPanel("pr-detail")?.group.id ?? centerGroupId; + focusOrAddPanel(api, { + id, + component: "pr-detail", + title: prKey ? "Pull Request" : "PR Details", + position: { referenceGroup: targetGroupId }, + params: prKey ? { prKey } : undefined, + }); + }, + addMRPanel: (mrKey: string) => { + const { api, centerGroupId } = get(); + if (!api) return; + const id = `mr-detail|${mrKey}`; + const existing = api.getPanel(id); + if (existing) { + existing.api.setActive(); + return; + } + if (focusMatchingLegacyPanel(api, id, "mr-detail", "mrKey", mrKey)) return; + const canonical = api.getPanel("pr-detail"); + if (canonical?.params?.mrKey === mrKey) { + canonical.api.setActive(); + return; + } + focusOrAddPanel(api, { + id, + component: "mr-detail", + title: "Merge Request", + position: { referenceGroup: canonical?.group.id ?? centerGroupId }, + params: { mrKey }, + }); + }, + }; +} + export function buildExtraPanelActions(get: StoreGet) { return { addVscodePanel: () => { @@ -495,70 +539,7 @@ export function buildExtraPanelActions(get: StoreGet) { opts?.quiet ?? false, ); }, - /** - * Opens the PR detail panel for a given key, or focuses the tab already - * showing that exact PR. - * - * @param prKey - `//` identifying the PR to - * show; `undefined` targets the legacy single-repo panel id - * ("pr-detail"). - * @param activeSessionId - Session to anchor the panel next to as a - * tab; falls back to `centerGroupId` when omitted or when no matching - * session panel exists. - * - * Reuses the legacy unkeyed "pr-detail" panel only when it's already - * showing this exact PR (tracked via its stamped `params.prKey` — see - * `runAutoPRPanelEffect` in dockview-session-tabs.ts, which keeps that - * key in sync with the task's current default PR). A different PR - * always gets its own `pr-detail|` tab instead of overwriting - * the one already open. - */ - addPRPanel: (prKey?: string, activeSessionId?: string | null) => { - const { api, centerGroupId } = get(); - if (!api) return; - // Multi-repo: each TaskPR opens in its own panel keyed by - // owner/repo/pr_number so multiple PRs can be tabbed side-by-side. - // Legacy single-repo callers (no key) get the historical panel id. - const id = prKey ? `pr-detail|${prKey}` : "pr-detail"; - // If a legacy "pr-detail" panel is already open (auto-shown on task - // open or restored from a saved layout) AND it's currently showing - // this exact PR (see useAutoPRPanel, which stamps the panel's params - // with the PR it renders), reuse it instead of adding a second tab. - // A legacy panel showing a DIFFERENT PR (multi-repo "+" menu click) - // must NOT be repurposed — that would silently swap its content - // instead of opening a distinct tab for the newly requested PR. - if (prKey && focusMatchingLegacyPanel(api, id, "pr-detail", "prKey", prKey)) return; - // Prefer the live session panel's group over the store's centerGroupId - // — the latter can be stale across layout transitions and lands the PR - // panel in a separate split group instead of as a tab next to the - // session. Mirrors the resolution used by useAutoPRPanel. - const targetGroupId = activeSessionId - ? (api.getPanel(`session:${activeSessionId}`)?.group?.id ?? centerGroupId) - : centerGroupId; - focusOrAddPanel(api, { - id, - component: "pr-detail", - title: "Pull Request", - position: { referenceGroup: targetGroupId }, - params: prKey ? { prKey } : undefined, - }); - }, - addMRPanel: (mrKey: string, activeSessionId?: string | null) => { - const { api, centerGroupId } = get(); - if (!api) return; - const id = `mr-detail|${mrKey}`; - removeMatchingLegacyPanel(api, id, "mr-detail", "mrKey", mrKey); - const targetGroupId = activeSessionId - ? (api.getPanel(`session:${activeSessionId}`)?.group?.id ?? centerGroupId) - : centerGroupId; - focusOrAddPanel(api, { - id, - component: "mr-detail", - title: "Merge Request", - position: { referenceGroup: targetGroupId }, - params: { mrKey }, - }); - }, + ...buildReviewPanelActions(get), addTerminalPanel: ( terminalId?: string, groupId?: string, diff --git a/apps/web/lib/state/dockview-preset-persistence.test.ts b/apps/web/lib/state/dockview-preset-persistence.test.ts index 22cb3f19c7..e58eaf87e9 100644 --- a/apps/web/lib/state/dockview-preset-persistence.test.ts +++ b/apps/web/lib/state/dockview-preset-persistence.test.ts @@ -79,7 +79,7 @@ vi.mock("./layout-manager", async (importOriginal) => { import { removeEnvMaximizeState, setEnvLayout } from "@/lib/local-storage"; import { persistEnvLayoutNow, useDockviewStore } from "./dockview-store"; -import { applyLayout } from "./layout-manager"; +import { applyLayout, defaultLayout, fromDockviewApi } from "./layout-manager"; function makeApi(snapshot: object = { columns: [] }): DockviewApi { return { @@ -357,6 +357,130 @@ describe("resetLayout — effective default persistence", () => { }); }); +describe("toggleRightPanels — center fallback", () => { + beforeEach(resetStoreForIntegration); + + it("keeps a center fallback PR Details tab when hiding right panels", async () => { + const api = makeStoreApi(); + vi.mocked(fromDockviewApi).mockReturnValue({ + columns: [ + { + id: "center", + groups: [ + { + id: "group-center", + panels: [ + { id: "chat", component: "chat", title: "Agent" }, + { id: "pr-detail", component: "pr-detail", title: "PR Details" }, + ], + }, + ], + }, + { + id: "right", + pinned: true, + groups: [ + { + id: "group-right-top", + panels: [{ id: "files", component: "files", title: "Files" }], + }, + ], + }, + ], + }); + useDockviewStore.setState({ api, rightPanelsVisible: true, defaultPreset: "default" }); + + useDockviewStore.getState().toggleRightPanels(); + + const appliedState = vi.mocked(applyLayout).mock.calls.at(-1)?.[1]; + expect(appliedState?.columns.map((column) => column.id)).toEqual(["center"]); + expect(appliedState?.columns[0]?.groups[0]?.panels.map((panel) => panel.id)).toEqual([ + "chat", + "pr-detail", + ]); + await flushRaf(); + }); + + it("keeps a center fallback PR Details tab when showing right panels", async () => { + const api = makeStoreApi(); + vi.mocked(fromDockviewApi).mockReturnValue({ + columns: [ + { + id: "center", + groups: [ + { + id: "group-center", + panels: [ + { id: "chat", component: "chat", title: "Agent" }, + { id: "pr-detail", component: "pr-detail", title: "PR Details" }, + ], + }, + ], + }, + ], + }); + vi.mocked(defaultLayout).mockReturnValue({ + columns: [ + { + id: "right", + pinned: true, + groups: [ + { + id: "group-right-top", + panels: [{ id: "files", component: "files", title: "Files" }], + }, + ], + }, + ], + }); + useDockviewStore.setState({ api, rightPanelsVisible: false, defaultPreset: "default" }); + + useDockviewStore.getState().toggleRightPanels(); + + const appliedState = vi.mocked(applyLayout).mock.calls.at(-1)?.[1]; + expect(appliedState?.columns[0]?.groups[0]?.panels.map((panel) => panel.id)).toEqual([ + "chat", + "pr-detail", + ]); + await flushRaf(); + }); +}); + +describe("toggleRightPanels", () => { + beforeEach(resetStoreForIntegration); + + it("removes a PR Details-only right column when hiding right panels", async () => { + const api = makeStoreApi(); + vi.mocked(fromDockviewApi).mockReturnValue({ + columns: [ + { + id: "center", + groups: [ + { id: "group-center", panels: [{ id: "chat", component: "chat", title: "Agent" }] }, + ], + }, + { + id: "right", + pinned: true, + groups: [ + { + id: "group-right-top", + panels: [{ id: "pr-detail", component: "pr-detail", title: "PR Details" }], + }, + ], + }, + ], + }); + useDockviewStore.setState({ api, rightPanelsVisible: true, defaultPreset: "default" }); + + useDockviewStore.getState().toggleRightPanels(); + + const appliedState = vi.mocked(applyLayout).mock.calls.at(-1)?.[1]; + expect(appliedState?.columns.map((column) => column.id)).toEqual(["center"]); + await flushRaf(); + }); +}); + describe("applyCustomLayout — session panel normalization", () => { beforeEach(resetStoreForIntegration); diff --git a/apps/web/lib/state/dockview-store.ts b/apps/web/lib/state/dockview-store.ts index 62f54b97e1..7d8cc70005 100644 --- a/apps/web/lib/state/dockview-store.ts +++ b/apps/web/lib/state/dockview-store.ts @@ -158,12 +158,10 @@ type DockviewStore = { addVscodePanel: () => void; openInternalVscode: (goto_: { file: string; line: number; col: number } | null) => void; addPlanPanel: (opts?: { groupId?: string; quiet?: boolean; inCenter?: boolean }) => void; - /** Open a PR detail panel. prKey (owner/repo/pr_number) gives multi-repo tasks one tab per PR. - * activeSessionId anchors the new panel to the session's current group so it lands as a tab - * next to the session, not as a split. Falls back to centerGroupId when omitted. */ - addPRPanel: (prKey?: string, activeSessionId?: string | null) => void; + /** Open a PR detail panel. prKey (owner/repo/pr_number) gives multi-repo tasks one tab per PR. */ + addPRPanel: (prKey?: string) => void; /** Open a GitLab merge request detail panel keyed by host/project/iid. */ - addMRPanel: (mrKey: string, activeSessionId?: string | null) => void; + addMRPanel: (mrKey: string) => void; addTerminalPanel: ( terminalId?: string, groupId?: string, @@ -447,12 +445,34 @@ function applyLayoutAndSet( return ids; } +function isRightColumn(column: LayoutState["columns"][number]): boolean { + return ( + column.id === "right" || + column.groups.some((group) => group.id === RIGHT_TOP_GROUP || group.id === RIGHT_BOTTOM_GROUP) + ); +} + +function columnHasRightPanel(column: LayoutState["columns"][number]): boolean { + const includesLayoutOwnedPRDetails = isRightColumn(column); + return column.groups.some((group) => + group.panels.some( + (panel) => + RIGHT_PANEL_IDS.has(panel.id) || (includesLayoutOwnedPRDetails && panel.id === "pr-detail"), + ), + ); +} + function removeRightPanelTabs(state: LayoutState): LayoutState { const columns = state.columns .map((col) => { + const includesLayoutOwnedPRDetails = isRightColumn(col); const groups = col.groups .map((group) => { - const panels = group.panels.filter((panel) => !RIGHT_PANEL_IDS.has(panel.id)); + const panels = group.panels.filter( + (panel) => + !RIGHT_PANEL_IDS.has(panel.id) && + !(includesLayoutOwnedPRDetails && panel.id === "pr-detail"), + ); if (panels.length === group.panels.length) return group; const activePanel = panels.some((panel) => panel.id === group.activePanel) ? group.activePanel @@ -484,10 +504,7 @@ function buildVisibilityActions(set: StoreSet, get: StoreGet) { if (rightPanelsVisible) { const current = fromDockviewApi(api); const withoutRight: LayoutState = { - columns: current.columns.filter( - (c) => - !c.groups.some((g) => g.panels.some((p) => p.id === "files" || p.id === "changes")), - ), + columns: current.columns.filter((column) => !columnHasRightPanel(column)), }; set({ isRestoringLayout: true, rightPanelsVisible: false }); applyLayoutAndSet(api, withoutRight, liveWidths, set); diff --git a/apps/web/lib/state/layout-manager/constants.ts b/apps/web/lib/state/layout-manager/constants.ts index 7241bda674..0fbef04602 100644 --- a/apps/web/lib/state/layout-manager/constants.ts +++ b/apps/web/lib/state/layout-manager/constants.ts @@ -23,6 +23,7 @@ export const REUSABLE_PANEL_IDS = [ "chat", "files", "changes", + "pr-detail", TERMINAL_DEFAULT_ID, "plan", "browser", @@ -76,7 +77,7 @@ export const PANEL_REGISTRY: Record> = { title: "Terminal", params: { terminalId: "shell-default" }, }, - "pr-detail": { component: "pr-detail", title: "Pull Request" }, + "pr-detail": { component: "pr-detail", title: "PR Details" }, "mr-detail": { component: "mr-detail", title: "Merge Request" }, }; diff --git a/apps/web/lib/state/layout-manager/index.ts b/apps/web/lib/state/layout-manager/index.ts index f74ba194d5..37935bd646 100644 --- a/apps/web/lib/state/layout-manager/index.ts +++ b/apps/web/lib/state/layout-manager/index.ts @@ -86,7 +86,6 @@ export { layoutStructuresMatch, savedLayoutMatchesLive } from "./comparator"; export { INTENT_COMPACT, INTENT_PLAN, - INTENT_PR_REVIEW, injectIntentPanels, applyActivePanelOverrides, resolveNamedIntent, diff --git a/apps/web/lib/state/layout-manager/intent.ts b/apps/web/lib/state/layout-manager/intent.ts index c4df8a0902..04baa3aa91 100644 --- a/apps/web/lib/state/layout-manager/intent.ts +++ b/apps/web/lib/state/layout-manager/intent.ts @@ -4,26 +4,11 @@ import { CENTER_GROUP, RIGHT_TOP_GROUP, RIGHT_BOTTOM_GROUP } from "./constants"; /** Well-known intent name constants (used as URL ?layout= values). */ export const INTENT_COMPACT = "compact"; export const INTENT_PLAN = "plan"; -export const INTENT_PR_REVIEW = "pr-review"; /** Registry of named layout intents, keyed by URL param value. */ const NAMED_INTENTS: Record = { [INTENT_COMPACT]: { preset: "compact" }, [INTENT_PLAN]: { preset: "plan" }, - [INTENT_PR_REVIEW]: { - panels: [ - { - id: "pr-detail", - component: "pr-detail", - title: "Pull Request", - targetGroup: "center", - }, - ], - activePanels: { - [CENTER_GROUP]: "pr-detail", - [RIGHT_TOP_GROUP]: "changes", - }, - }, }; /** Resolve a named intent string (from URL param) to a LayoutIntent, or null if unknown. */ diff --git a/apps/web/lib/state/layout-manager/merger.test.ts b/apps/web/lib/state/layout-manager/merger.test.ts index afb3522e3e..2ab3d5053b 100644 --- a/apps/web/lib/state/layout-manager/merger.test.ts +++ b/apps/web/lib/state/layout-manager/merger.test.ts @@ -132,6 +132,32 @@ describe("mergePanelsIntoPreset", () => { expect(panelIdsIn(result, "right")).toEqual(["files", "changes"]); }); + it("keeps Agent selected when leaving Plan for Default with PR Details", () => { + const currentState = makeLayoutWithSide( + [ + { id: SESSION_ABC, component: "chat" }, + { id: "pr-detail", component: "pr-detail" }, + ], + "plan", + [{ id: "plan", component: "plan" }], + ); + const targetPreset = makeLayoutWithSide( + [ + { id: "chat", component: "chat" }, + { id: "pr-detail", component: "pr-detail" }, + ], + "right", + [{ id: "files", component: "files" }], + ); + targetPreset.columns[1]!.groups[0]!.activePanel = "chat"; + + const result = mergePanelsIntoPreset(currentState, targetPreset); + const center = result.columns.find((column) => column.id === "center")!.groups[0]!; + + expect(center.panels.map((panel) => panel.id)).toEqual([SESSION_ABC, "pr-detail", "plan"]); + expect(center.activePanel).toBe(SESSION_ABC); + }); + it("places surviving browser/vscode/pr-detail in center, files/changes in the side column", () => { // Switching from a content preset (vscode/preview/etc.) back to default: // main-content surfaces (browser, vscode, pr-detail) must follow the chat diff --git a/apps/web/lib/state/layout-manager/merger.ts b/apps/web/lib/state/layout-manager/merger.ts index 845438c2b4..6ceaee5d15 100644 --- a/apps/web/lib/state/layout-manager/merger.ts +++ b/apps/web/lib/state/layout-manager/merger.ts @@ -69,6 +69,38 @@ function appendToFirstGroup( }); } +/** Replace the reusable Chat slot with live session tabs in place. + * + * Keeping the replacement at Chat's original index preserves both the tab + * order and its selected state. Appending session tabs after the target + * preset's other panels would put PR Details ahead of Agent after a + * Plan → Default transition, making the background review tab visible while + * the agent input is hidden. + */ +function replaceChatWithSessionPanels( + groups: LayoutState["columns"][number]["groups"], + sessionPanels: LayoutPanel[], + toAdd: LayoutPanel[], +): LayoutState["columns"][number]["groups"] { + return groups.map((group, idx) => { + if (idx !== 0) return group; + + let insertedSessions = false; + const basePanels = group.panels.flatMap((panel) => { + if (panel.id !== "chat") return [panel]; + insertedSessions = true; + return sessionPanels; + }); + if (!insertedSessions) basePanels.unshift(...sessionPanels); + + const existingIds = new Set(basePanels.map((panel) => panel.id)); + const additions = toAdd.filter((panel) => !existingIds.has(panel.id)); + const activePanel = group.activePanel === "chat" ? sessionPanels[0]?.id : group.activePanel; + + return { ...group, panels: [...basePanels, ...additions], activePanel }; + }); +} + /** * Pure merge logic: merge extra panels from the current state into a target * preset layout. Session panels (`session:*`) replace the generic `chat` @@ -105,10 +137,15 @@ export function mergePanelsIntoPreset( // Center-affinity extras (e.g. "plan") sit alongside the chat. When // the side column doesn't exist, side extras land here too. const fallbackSide = extrasColumnId === "center" ? sideExtras : []; - const filterChat = hasSessionPanels ? (p: LayoutPanel) => p.id !== "chat" : undefined; - const additions = [...sessionExtras, ...centerExtras, ...fallbackSide]; - if (additions.length === 0 && !filterChat) return col; - return { ...col, groups: appendToFirstGroup(col.groups, additions, filterChat) }; + const additions = [...centerExtras, ...fallbackSide]; + if (hasSessionPanels) { + return { + ...col, + groups: replaceChatWithSessionPanels(col.groups, sessionExtras, additions), + }; + } + if (additions.length === 0) return col; + return { ...col, groups: appendToFirstGroup(col.groups, additions) }; } if (col.id === extrasColumnId && sideExtras.length > 0) { return { ...col, groups: appendToFirstGroup(col.groups, sideExtras) }; diff --git a/apps/web/lib/state/layout-manager/presets.test.ts b/apps/web/lib/state/layout-manager/presets.test.ts index 3d6919c42a..70f82a68c5 100644 --- a/apps/web/lib/state/layout-manager/presets.test.ts +++ b/apps/web/lib/state/layout-manager/presets.test.ts @@ -7,6 +7,16 @@ describe("layout presets", () => { expect(layout.columns.map((c) => c.id)).toEqual(["center", "right"]); }); + it("places PR Details in the Agent group", () => { + const layout = defaultLayout(); + const center = layout.columns.find((column) => column.id === "center")?.groups[0]; + const rightTop = layout.columns.find((column) => column.id === "right")?.groups[0]; + + expect(center?.panels.map((panel) => panel.id)).toEqual(["chat", "pr-detail"]); + expect(center?.activePanel).toBe("chat"); + expect(rightTop?.panels.map((panel) => panel.id)).toEqual(["files", "changes"]); + }); + it("compact preset is a single center column with everything tabbed", () => { const layout = compactLayout(); expect(layout.columns.map((c) => c.id)).toEqual(["center"]); @@ -14,6 +24,7 @@ describe("layout presets", () => { "chat", "files", "changes", + "pr-detail", "terminal-default", ]); }); diff --git a/apps/web/lib/state/layout-manager/presets.ts b/apps/web/lib/state/layout-manager/presets.ts index 9c40a6de0f..725bb34d22 100644 --- a/apps/web/lib/state/layout-manager/presets.ts +++ b/apps/web/lib/state/layout-manager/presets.ts @@ -6,7 +6,13 @@ export function defaultLayout(): LayoutState { columns: [ { id: "center", - groups: [{ id: CENTER_GROUP, panels: [panel("chat")] }], + groups: [ + { + id: CENTER_GROUP, + activePanel: "chat", + panels: [panel("chat"), panel("pr-detail")], + }, + ], }, { id: "right", @@ -29,7 +35,13 @@ export function compactLayout(): LayoutState { groups: [ { id: CENTER_GROUP, - panels: [panel("chat"), panel("files"), panel("changes"), panel("terminal-default")], + panels: [ + panel("chat"), + panel("files"), + panel("changes"), + panel("pr-detail"), + panel("terminal-default"), + ], }, ], }, diff --git a/docs/plans/pr-details-layout-panel/plan.md b/docs/plans/pr-details-layout-panel/plan.md new file mode 100644 index 0000000000..07977dcae9 --- /dev/null +++ b/docs/plans/pr-details-layout-panel/plan.md @@ -0,0 +1,127 @@ +--- +spec: docs/specs/ui/task-layout-profiles.md +created: 2026-07-31 +status: complete +--- + +# Implementation Plan: Layout-owned PR Details Panel + +## Overview + +Replace this branch's global Appearance preference for pull-request tab placement with the existing task-layout profile system. Promote the canonical `pr-detail` runtime panel to the reusable **PR Details** panel, put it beside Agent in the built-in Default layout's `CENTER_GROUP`, and let users place or remove it with `Settings > General > Layouts`. Keep review identity synchronization separate from layout mutation: runtime code may update which review the canonical panel renders, but only layouts decide whether the panel exists and where it lives. + +No backend schema, user-settings field, endpoint, or ADR is required. `users.settings.saved_layouts` already owns portable layout placement, and `docs/specs/ui/task-layout-profiles.md` owns the product contract. + +## Architecture boundary + +- `LayoutState` owns the canonical `pr-detail` panel's existence, tab order, group, geometry, and initial active tab. +- Review-domain state owns only the canonical panel's provider and primary PR/MR key. +- Keyed `pr-detail|...` and `mr-detail|...` panels remain task-specific transient review tabs; the Layout editor never serializes them. +- Existing custom profiles and environment-scoped task layouts are not backfilled. Fresh environments and Reset Layout use the updated built-in/default profile. +- Mobile and tablet task Review surfaces remain unchanged. Mobile settings continues to edit desktop profiles through the existing responsive Layout editor. + +## Frontend + +### Remove the parallel global setting + +- Restore the backend user-settings contract in `apps/backend/internal/user/**` and `apps/backend/internal/backendapp/**` by removing this branch's `pr_panel_placement` model, DTO, persistence, boot-state, and validation changes. +- Remove `prPanelPlacement` from frontend HTTP/SSR/store/WebSocket mappings in `apps/web/lib/types/**`, `apps/web/lib/ssr/user-settings.ts`, `apps/web/lib/state/slices/settings/**`, `apps/web/lib/ws/handlers/users.ts`, and related tests. +- Remove the Pull Request Tabs card and draft/save wiring from `apps/web/components/settings/general-settings.tsx` and `apps/web/components/settings/editors-settings-state.tsx`. +- Delete `apps/web/lib/state/pr-panel-placement.ts` and its tests. Remove placement options from `pr-topbar-button.tsx`, `dockview-add-panel-items.tsx`, and the Dockview action signatures. + +### Register PR Details as reusable layout content + +- In `apps/web/lib/state/layout-manager/constants.ts`, add `pr-detail` to `REUSABLE_PANEL_IDS` and rename its registry title to `PR Details`. Keep `mr-detail` supported for legacy/keyed runtime layouts, but do not make it reusable. +- In `apps/web/lib/state/layout-manager/presets.ts`, add `pr-detail` after Chat in `CENTER_GROUP` for `defaultLayout()`. Keep Chat active by default, and keep `RIGHT_TOP_GROUP` limited to Files and Changes. Add `pr-detail` to `compactLayout()` because compact desktop has one tab group. +- Update the built-in Default description in `apps/web/lib/layout/layout-profiles.ts` and let its existing reusable-panel validation accept canonical `pr-detail` while rejecting keyed review IDs. +- Register a side-effect-free `pr-detail` placeholder in `apps/web/components/settings/layouts/layout-editor.tsx`; the editor's existing reusable-panel catalog then exposes **PR Details** for add, move, split, remove, keyboard, and touch actions. + +### Separate review identity from layout structure + +- Replace `useAutoPRPanel` in `apps/web/components/task/dockview-session-tabs.ts` and `useAutoMRPanel` in `dockview-auto-mr-panel.ts` with one focused `dockview-review-panel-sync.ts` hook. Its pure helper updates params on an existing canonical `pr-detail` only; it never adds, closes, moves, or activates a panel. +- Prefer a linked GitHub PR over a GitLab MR as today, clear stale opposite-provider params during task/provider changes, and clear both identities when no review exists so `ReviewDetailPanelComponent` renders its normal empty state. +- Remove obsolete PR-panel-offered session-storage state from `apps/web/lib/local-storage.ts`. +- Remove PR/MR panels as implicit Agent-session anchors in `dockview-session-tabs.ts`. Their group is now layout-owned and may intentionally be remote from Agent. +- Refactor `addPRPanel` and `addMRPanel` in `apps/web/lib/state/dockview-panel-actions.ts`: focus an exact existing panel in place; reuse the canonical panel when it already renders the requested key; otherwise add a keyed panel to the canonical `pr-detail` group, falling back to `centerGroupId` when canonical is absent. Never move an existing tab and never create a new split. +- Mount the unified synchronization hook from `dockview-desktop-layout.tsx`. Keep `ReviewDetailPanelComponent` as the provider-aware renderer for canonical `pr-detail`, and keep keyed provider-specific components compatible with restored layouts. + +## Tests + +- `apps/web/lib/state/layout-manager/presets.test.ts`: exact Default Agent-group, active Agent tab, and right-top order plus compact membership. +- `apps/web/lib/state/layout-manager/merger.test.ts`: Plan-to-Default session replacement preserves Agent before PR Details and keeps Agent selected. +- `apps/web/lib/layout/layout-profiles.test.ts`: canonical `pr-detail` accepted; keyed PR/MR IDs rejected. +- `apps/web/components/settings/layouts/layout-editor-actions.test.ts`: PR Details appears once in the add catalog and participates in existing panel operations. +- `apps/web/components/task/dockview-review-panel-sync.test.ts`: GitHub/GitLab/empty identity transitions mutate params only and ignore a missing canonical panel. +- `apps/web/lib/state/dockview-panel-actions.test.ts`: canonical-group anchoring, center fallback, matching canonical reuse, and focus-in-place without relocation. +- `apps/web/components/task/dockview-session-tabs.test.ts`: review groups are not treated as Agent anchors. +- Remove obsolete global-setting, placement-geometry, auto-add/remove, and offered-session tests instead of preserving superseded behavior. + +## E2E + +- Extend `apps/web/e2e/tests/settings/layout-profiles.spec.ts` to prove the built-in Default contains PR Details beside Agent and a saved move changes fresh/reset desktop placement. +- Extend `apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts` to add or move PR Details with touch controls and assert no document-level horizontal overflow. +- Replace `apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts` with focused layout-owned coverage: canonical content follows the active task, no-review state leaves the tab present, and removing the canonical tab is not undone by linked-review updates. +- Update `pr-detail-manual-open.spec.ts` and `pr-detail-dedup.spec.ts` for canonical-group anchoring, center fallback, exact-tab focus, and no relocation. +- Remove global Appearance-setting flows from `mobile-general-settings.spec.ts` and `settings-manual-save.spec.ts`; keep their unrelated coverage intact. + +## Mobile design contract + +- Desktop outcome: Default layout shows PR Details as a background tab beside Agent in the center group; custom placement uses existing Dockview editor controls. +- Phone entry: `Settings > General > Layouts`, using the existing settings drawer and Layout editor surface. +- Nearest mobile exemplar: `apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts`; no new drawer, overlay, or navigation model is introduced. +- Scroll ownership: existing settings page remains the vertical scroll owner; editor controls must not add horizontal page scrolling. +- Touch parity: existing contextual add/move/split/remove controls must expose PR Details. Mobile/tablet task-detail Review navigation does not change. + +## Public documentation + +- Update `docs/public/sessions-and-review.md`: remove Appearance-based placement instructions and describe PR Details as a reusable Layout panel, including built-in Default placement, fresh/reset scope, and existing-layout behavior. + +## Implementation waves + +Wave 1: + +- [x] [Task 01 - Reusable panel and settings cleanup](task-01-reusable-panel-and-settings-cleanup.md) + +Wave 2 (depends on Wave 1): + +- [x] [Task 02 - Review synchronization and opening behavior](task-02-review-sync-and-opening.md) + +Wave 3 (depends on Wave 2): + +- [x] [Task 03 - E2E, docs, and verification](task-03-e2e-docs-verification.md) + +## Risks + +- Existing saved layouts intentionally lack `pr-detail`; auto-inserting it would violate task-layout precedence and user ownership. +- Dockview parameter updates merge values. Provider switches must explicitly clear stale `prKey`, `mrKey`, and provider markers. +- Existing Agent-restoration logic assumes review tabs were Agent siblings. Leaving that heuristic intact could restore Agent into the configured PR Details group. +- Default and compact preset changes affect fresh environments and Reset Layout; ordering must keep Agent and PR Details together without focus theft. +- This branch currently changes backend and frontend settings contracts. Cleanup must remove the entire unused contract so no dead API field or persistence path remains. +- E2E runs use production Vite assets; every frontend E2E run must use the managed runner with a fresh build after source changes. + +## Verification + +```bash +make -C apps/backend test +cd apps && pnpm --filter @kandev/web test lib/state/layout-manager/presets.test.ts lib/layout/layout-profiles.test.ts components/settings/layouts/layout-editor-actions.test.ts components/task/dockview-review-panel-sync.test.ts components/task/dockview-session-tabs.test.ts lib/state/dockview-panel-actions.test.ts components/task/review-panel-provider.test.ts +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web lint +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +cd apps/web && pnpm e2e:run tests/settings/layout-profiles.spec.ts tests/pr/pr-detail-layout.spec.ts tests/pr/pr-detail-manual-open.spec.ts tests/pr/pr-detail-dedup.spec.ts +cd apps/web && pnpm e2e:run tests/settings/mobile-layout-profiles.spec.ts -- --project=mobile-chrome +``` + +## Completion evidence + +- 2026-07-31: focused frontend unit suite passed (165 tests); frontend typecheck and lint passed. +- 2026-07-31: `make -C apps/backend test` and both public-doc validators passed. +- 2026-07-31: fresh production-build desktop PR/Layout E2E suite passed (8 tests), and focused desktop Layout capture passed (4 tests). +- 2026-07-31: mobile Layout E2E passed (2 tests), including touch selection and no horizontal overflow. +- 2026-07-31: Default PR Details placement moved beside Agent. Focused preset unit test, + fresh production desktop E2E, mobile Layout E2E (2 tests), typecheck, lint, and public-doc + validation passed. +- 2026-07-31: CI exposed Plan-to-Default focus theft: live session tabs were appended after + PR Details. The merge now replaces Chat in place and preserves Agent selection; focused + unit tests, the fresh-build 14-test workflow E2E suite, typecheck, lint, and public-doc + validation passed. diff --git a/docs/plans/pr-details-layout-panel/task-01-reusable-panel-and-settings-cleanup.md b/docs/plans/pr-details-layout-panel/task-01-reusable-panel-and-settings-cleanup.md new file mode 100644 index 0000000000..af718c0e53 --- /dev/null +++ b/docs/plans/pr-details-layout-panel/task-01-reusable-panel-and-settings-cleanup.md @@ -0,0 +1,78 @@ +--- +id: "01-reusable-panel-and-settings-cleanup" +title: "Reusable PR Details panel and settings cleanup" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/task-layout-profiles.md" +--- + +# Task 01: Reusable PR Details panel and settings cleanup + +Remove the branch's parallel placement preference, then make canonical `pr-detail` valid reusable layout content with code-defined Default and compact placement. + +## Acceptance + +- No backend, boot-state, frontend HTTP, SSR, Zustand, WebSocket, or Appearance-settings contract contains `pr_panel_placement` / `prPanelPlacement`. +- `REUSABLE_PANEL_IDS` contains canonical `pr-detail`, whose editor/runtime registry title is `PR Details`; `mr-detail` and keyed review IDs remain non-reusable. +- `defaultLayout()` puts PR Details beside Agent in `CENTER_GROUP` with Agent selected, while Files and Changes remain in `RIGHT_TOP_GROUP`. +- `compactLayout()` includes PR Details in its single group. +- Layout-profile validation accepts canonical `pr-detail`, rejects keyed review IDs, and still enforces one instance per reusable panel. +- Layout editor renders a lightweight PR Details placeholder and exposes it through existing add/move/remove controls without mounting live review data. +- No schema migration or replacement user setting is added. + +## TDD sequence + +1. Change preset, profile-validation, and editor-action tests first; run them and confirm failures against current registry/presets. +2. Remove global-setting assertions and add a search/check proving no branch-only contract remains. +3. Implement registry, preset, profile-description, editor-placeholder, and contract cleanup. +4. Re-run focused frontend tests, backend tests affected by restored settings shapes, and typecheck. + +## Files likely touched + +- `apps/web/lib/state/layout-manager/constants.ts` +- `apps/web/lib/state/layout-manager/presets.ts` +- `apps/web/lib/state/layout-manager/presets.test.ts` +- `apps/web/lib/layout/layout-profiles.ts` +- `apps/web/lib/layout/layout-profiles.test.ts` +- `apps/web/components/settings/layouts/layout-editor.tsx` +- `apps/web/components/settings/layouts/layout-editor-actions.test.ts` +- `apps/web/components/settings/general-settings.tsx` +- `apps/web/components/settings/editors-settings-state.tsx` +- `apps/web/lib/types/backend.ts` +- `apps/web/lib/types/http-user-settings.ts` +- `apps/web/lib/types/http.ts` +- `apps/web/lib/ssr/user-settings.ts` +- `apps/web/lib/ssr/user-settings.test.ts` +- `apps/web/lib/state/slices/settings/settings-slice.ts` +- `apps/web/lib/state/slices/settings/types.ts` +- `apps/web/lib/ws/handlers/users.ts` +- `apps/web/lib/ws/handlers/users.test.ts` +- `apps/web/hooks/use-user-display-settings.ts` +- `apps/web/hooks/use-ensure-user-settings.test.ts` +- `apps/backend/internal/backendapp/boot_state_routes.go` +- `apps/backend/internal/backendapp/boot_state_user_settings_test.go` +- `apps/backend/internal/user/controller/controller.go` +- `apps/backend/internal/user/dto/dto.go` +- `apps/backend/internal/user/dto/dto_test.go` +- `apps/backend/internal/user/models/models.go` +- `apps/backend/internal/user/service/service.go` +- `apps/backend/internal/user/service/service_test.go` +- `apps/backend/internal/user/store/sqlite.go` +- `apps/backend/internal/user/store/sqlite_test.go` + +## Verification + +- `cd apps && pnpm --filter @kandev/web test lib/state/layout-manager/presets.test.ts lib/layout/layout-profiles.test.ts components/settings/layouts/layout-editor-actions.test.ts lib/ssr/user-settings.test.ts lib/ws/handlers/users.test.ts hooks/use-ensure-user-settings.test.ts` +- `cd apps/web && pnpm run typecheck` +- `make -C apps/backend test` +- `rg -n "pr_panel_placement|prPanelPlacement" apps docs/public` + +## Dependencies + +None. + +## Output contract + +Report changed files, red/green test evidence, backend/typecheck results, blockers, and residual risks; set this task to `done` and tick it in `plan.md`. diff --git a/docs/plans/pr-details-layout-panel/task-02-review-sync-and-opening.md b/docs/plans/pr-details-layout-panel/task-02-review-sync-and-opening.md new file mode 100644 index 0000000000..f61a6096b9 --- /dev/null +++ b/docs/plans/pr-details-layout-panel/task-02-review-sync-and-opening.md @@ -0,0 +1,65 @@ +--- +id: "02-review-sync-and-opening" +title: "Review synchronization and opening behavior" +status: done +wave: 2 +depends_on: ["01-reusable-panel-and-settings-cleanup"] +plan: "plan.md" +spec: "../../specs/ui/task-layout-profiles.md" +--- + +# Task 02: Review synchronization and opening behavior + +Keep canonical PR Details content current without changing layout, and anchor explicit keyed review tabs to the canonical panel's configured group. + +## Acceptance + +- One focused review-sync hook updates params only when canonical `pr-detail` exists; it never adds, removes, moves, or activates any panel. +- GitHub PR remains preferred when both providers have linked reviews. Task/provider switches clear stale opposing params, and no-review state clears both identities while leaving the panel present. +- Closing/removing canonical PR Details is respected across later review-data updates; obsolete PR-panel-offered session storage is removed. +- Agent restoration no longer treats `pr-detail`, `mr-detail`, or keyed review panels as proof that their group belongs to Agent. +- Explicit PR/MR opening focuses an exact existing tab in its current group. +- A matching canonical panel is reused when it already renders the requested key. A new keyed panel joins canonical `pr-detail`'s group; if canonical is absent, it uses `centerGroupId`. No path creates a new split or moves an existing tab. +- Topbar and add-panel callers no longer pass global placement options or use the active Agent as a spatial policy. + +## TDD sequence + +1. Add pure sync tests for GitHub, GitLab, provider switch, empty review, and missing canonical panel; confirm current auto-add/remove code fails the contract. +2. Rewrite panel-action tests for canonical-group anchoring, fallback, reuse, and focus-in-place; confirm current relocation behavior fails. +3. Add an Agent-restoration regression where canonical PR Details lives in the right group. +4. Implement the unified hook and action changes; remove auto-panel and offered-storage code. +5. Re-run focused tests and typecheck before touching E2E. + +## Files likely touched + +- add `apps/web/components/task/dockview-review-panel-sync.ts` +- add `apps/web/components/task/dockview-review-panel-sync.test.ts` +- `apps/web/components/task/dockview-desktop-layout.tsx` +- `apps/web/components/task/dockview-session-tabs.ts` +- `apps/web/components/task/dockview-session-tabs.test.ts` +- remove `apps/web/components/task/dockview-auto-mr-panel.ts` +- remove `apps/web/components/task/dockview-auto-mr-panel.test.ts` +- replace/remove `apps/web/components/task/__tests__/use-auto-pr-panel.test.ts` +- `apps/web/lib/state/dockview-panel-actions.ts` +- `apps/web/lib/state/dockview-panel-actions.test.ts` +- `apps/web/lib/state/dockview-store.ts` +- remove `apps/web/lib/state/pr-panel-placement.ts` +- remove `apps/web/lib/state/pr-panel-placement.test.ts` +- `apps/web/lib/local-storage.ts` +- `apps/web/components/github/pr-topbar-button.tsx` +- `apps/web/components/gitlab/mr-topbar-button.tsx` +- `apps/web/components/task/dockview-add-panel-items.tsx` + +## Verification + +- `cd apps && pnpm --filter @kandev/web test components/task/dockview-review-panel-sync.test.ts components/task/dockview-session-tabs.test.ts lib/state/dockview-panel-actions.test.ts components/task/review-panel-provider.test.ts` +- `cd apps/web && pnpm run typecheck` +- `rg -n "useAutoPRPanel|useAutoMRPanel|PR_PANEL_OFFERED|resolvePRPanelTargetGroup|prPanelPlacement" apps/web` + +## Dependencies + +Task 01. + +## Output contract + +Report changed files, red/green test evidence, typecheck result, blockers, and residual risks; set this task to `done` and tick it in `plan.md`. diff --git a/docs/plans/pr-details-layout-panel/task-03-e2e-docs-verification.md b/docs/plans/pr-details-layout-panel/task-03-e2e-docs-verification.md new file mode 100644 index 0000000000..e8f36800cc --- /dev/null +++ b/docs/plans/pr-details-layout-panel/task-03-e2e-docs-verification.md @@ -0,0 +1,65 @@ +--- +id: "03-e2e-docs-verification" +title: "PR Details E2E, docs, and verification" +status: done +wave: 3 +depends_on: ["02-review-sync-and-opening"] +plan: "plan.md" +spec: "../../specs/ui/task-layout-profiles.md" +--- + +# Task 03: PR Details E2E, docs, and verification + +Prove layout-owned PR Details behavior through production-build desktop/mobile flows, update public guidance, and remove obsolete global-setting test support. + +## Acceptance + +- Desktop Layout settings show PR Details in the built-in Default Agent group and can save it in another group; fresh/reset and Plan Mode exit workbenches honor that placement with Agent selected. +- Canonical PR Details remains present with no linked review, follows the active task's provider/key, and is not auto-created after the user removes it. +- Explicit secondary PR/MR tabs join the canonical group, fall back to center when canonical is absent, and focus without relocation when already open. +- Mobile Layout settings can add/move PR Details using existing touch controls and produce no document-level horizontal overflow. +- Mobile/tablet task Review UI remains unchanged. +- Appearance settings and E2E helpers contain no PR placement preference flow. +- Public Sessions and Review docs describe Layouts-based placement and fresh/reset versus existing-layout behavior. + +## E2E sequence + +1. Update tests for the intended behavior and run the focused files against the current production build to capture expected failures. +2. Finish only testability changes needed by stable `data-testid` selectors or page-object helpers. +3. Run desktop PR/Layout specs with a fresh managed build. +4. Run mobile Layout settings under `mobile-chrome` and assert touch operation plus overflow bounds. +5. Run lint, typecheck, backend tests, public-doc validation, and the focused unit suite before delivery. + +## Files likely touched + +- `apps/web/e2e/tests/settings/layout-profiles.spec.ts` +- `apps/web/e2e/tests/settings/mobile-layout-profiles.spec.ts` +- add `apps/web/e2e/tests/pr/pr-detail-layout.spec.ts` +- remove `apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts` +- `apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts` +- `apps/web/e2e/tests/pr/pr-detail-dedup.spec.ts` +- `apps/web/e2e/tests/settings/mobile-general-settings.spec.ts` +- `apps/web/e2e/tests/settings/settings-manual-save.spec.ts` +- `apps/web/e2e/fixtures/test-base.ts` +- `apps/web/e2e/helpers/api-client.ts` +- `apps/web/e2e/pages/session-page.ts` +- `docs/public/sessions-and-review.md` + +## Verification + +- `cd apps/web && pnpm e2e:run tests/settings/layout-profiles.spec.ts tests/pr/pr-detail-layout.spec.ts tests/pr/pr-detail-manual-open.spec.ts tests/pr/pr-detail-dedup.spec.ts` +- `cd apps/web && pnpm e2e:run tests/settings/mobile-layout-profiles.spec.ts -- --project=mobile-chrome` +- `make -C apps/backend test` +- `cd apps/web && pnpm run typecheck` +- `cd apps && pnpm --filter @kandev/web lint` +- `node --test scripts/validate-public-docs.test.mjs` +- `node scripts/validate-public-docs.mjs` +- `rg -n "pr_panel_placement|prPanelPlacement|Pull Request Tabs" apps docs/public` + +## Dependencies + +Task 02. + +## Output contract + +Report changed files, exact E2E/unit/lint/typecheck/docs results, failure artifacts or blockers, public-doc impact, and residual risks; set this task to `done` and tick it in `plan.md`. diff --git a/docs/public/sessions-and-review.md b/docs/public/sessions-and-review.md index 789101a229..426f32d33d 100644 --- a/docs/public/sessions-and-review.md +++ b/docs/public/sessions-and-review.md @@ -100,7 +100,9 @@ the workspace-search shortcuts untouched. Click a mode or press **Tab** / search field. File matches are grouped by repository. Hover a mode to see its direct shortcut. -Open **Settings > General > Layouts** to configure reusable desktop workbench profiles. Select a tab in a built-in layout to reveal its nearby edit controls, arrange or remove tabs and splits, then use the floating **Save changes** control. Kandev keeps the built-in row visible, marks it **Customized**, and stores your override without requiring a duplicate. Choose **Reset** beside a customized built-in to restore its original definition. Removing Terminal from the Default layout also prevents Kandev from creating its initial user shell. Changing the default does not replace a layout already saved for a task; choose **Reset Layout** from the workbench layout menu when you want that task to adopt the latest default. +Open **Settings > General > Layouts** to configure reusable desktop workbench profiles. Select a tab in a built-in layout to reveal its nearby edit controls, arrange or remove tabs and splits, then use the floating **Save changes** control. Kandev keeps the built-in row visible, marks it **Customized**, and stores your override without requiring a duplicate. Choose **Reset** beside a customized built-in to restore its original definition. + +**PR Details** is a reusable Layouts panel. The built-in Default puts it beside **Agent** in the center group, with Agent selected first; **Files** and **Changes** remain in the top-right group. You can move PR Details to another group or remove it with the same editor controls. When included in a layout, it shows the active task's linked GitHub pull request or GitLab merge request and remains as an empty panel when no review is linked. Removing it is respected: Kandev does not re-add it when a review appears. Changing the default applies to fresh task environments and **Reset Layout**, not a layout already saved for a task. Removing Terminal from the Default layout also prevents Kandev from creating its initial user shell. All panels for a task point at the same task environment. In a multi-repository task, check the repository label before editing, committing, or reviewing. A preview also requires the application to listen on a reachable interface and expose a forwarded port. diff --git a/docs/specs/ui/task-layout-profiles.md b/docs/specs/ui/task-layout-profiles.md index 1e8b896022..10e3db9921 100644 --- a/docs/specs/ui/task-layout-profiles.md +++ b/docs/specs/ui/task-layout-profiles.md @@ -8,7 +8,7 @@ owner: kandev ## Why -Users can arrange and save the desktop task workbench only while a task is open, which makes the default layout difficult to discover or configure. Users who do not want an initial terminal, or who prefer a different Files and Changes arrangement, need a durable settings surface that does not disturb layouts already customized for individual tasks. +Users can arrange and save the desktop task workbench only while a task is open, which makes the default layout difficult to discover or configure. Users who do not want an initial terminal, who prefer a different Files and Changes arrangement, or who want Pull Request details in a chosen pane need one durable layout surface that does not disturb layouts already customized for individual tasks. Panel placement belongs to the layout itself instead of a separate global appearance preference. ## What @@ -16,7 +16,11 @@ Users can arrange and save the desktop task workbench only while a task is open, - The page lists the built-in Default, Plan Mode, Preview Mode, and VS Code layouts as stable rows. A user edits a built-in directly; Kandev stores a hidden override while keeping the built-in row selected and marks it `Customized`. Reset removes the override and restores the code-defined layout. - A user can create, rename, duplicate, edit, delete, and select the default custom profile. Names must be non-empty; profile IDs must be unique. - Exactly one layout is effective as the user default. A saved profile, including a reserved built-in override, marked `is_default` wins; when none is marked, the built-in Default layout is effective. -- The visual editor supports one instance of each reusable panel: Agent, Files, Changes, Terminal, Plan, Browser, and VS Code. Agent is required and cannot be removed. +- The visual editor supports one instance of each reusable panel: Agent, Files, Changes, PR Details, Terminal, Plan, Browser, and VS Code. Agent is required and cannot be removed. +- PR Details is the canonical reusable `pr-detail` panel. When the selected layout contains it, it renders the active task's linked GitHub pull request or GitLab merge request through the existing provider-aware review surface and remains in its configured location with its normal empty state when no review is linked. +- The built-in Default layout places PR Details beside Agent in the center group, with Agent initially selected. Files remains the initially selected tab in the top-right Files and Changes group. The compact desktop layout includes PR Details in its single tab group because it has no separate right column. +- Runtime review-data synchronization may update the canonical PR Details panel's content identity only when the selected layout contains it; it never adds, removes, or moves the panel. Removing PR Details from a profile or task layout therefore remains a user-owned layout choice. +- Explicitly opening a specific pull request or merge request focuses a matching tab in place. A new task-specific review tab joins the canonical PR Details panel's configured group when that panel exists and otherwise falls back to the existing center group; opening never relocates an already-open tab. - Selecting a tab makes it active and shows contextual controls next to its split. Users can reorder or remove the tab, move it between groups, create splits, and move, merge, or resize the selected split. Adding a missing panel remains a separate floating action. Every editor action provides a hover/focus description. - Layout changes use the shared Settings floating save control and navigation guard. The page does not render its own Save or Cancel buttons. - Removing Terminal from the effective default prevents the default terminal panel and its backing user shell from being created when a fresh task environment is first opened. @@ -47,7 +51,7 @@ Layout profiles remain in the backend-owned `users.settings.saved_layouts` JSON The built-in layouts are code-defined templates. A customization is stored in `saved_layouts` under the reserved stable ID `layout-override-`, but is hidden from the Custom list and presented as the same built-in row. Reserved overrides participate in the same single-`is_default` invariant as custom profiles. A Default override replaces the code-defined Default as the effective default only when that override owns `is_default`; editing it claims the default when no saved profile currently owns it and otherwise preserves the existing custom default. If no saved profile has `is_default: true`, the code-defined Default template is the effective default. Resetting a built-in removes only its reserved override. -The editor persists the existing declarative `LayoutState`: ordered columns contain ordered groups, groups contain ordered panels and an active panel, and captured tree/size data preserves split placement and proportions. New editor-created profiles use only the reusable panel registry. A legacy profile with an unreadable layout remains listed for rename, duplication, deletion, or default removal, but cannot enter the visual editor or become a new default until replaced with a valid reusable layout. +The editor persists the existing declarative `LayoutState`: ordered columns contain ordered groups, groups contain ordered panels and an active panel, and captured tree/size data preserves split placement and proportions. New editor-created profiles use only the reusable panel registry. The canonical `pr-detail` ID is reusable; keyed panels such as `pr-detail|owner/repository/123` and `mr-detail|host/project/123` remain task-specific runtime tabs and are not accepted by the profile editor. A legacy profile with an unreadable layout remains listed for rename, duplication, deletion, or default removal, but cannot enter the visual editor or become a new default until replaced with a valid reusable layout. Task-specific restored layouts remain device-local environment state and take precedence over the user default. They are not copied into or overwritten by layout-profile edits. The serialized Dockview layout preserves panel structure and transient geometry. A companion environment-scoped preference stores a raw right-column width only after a genuine user sash drag; legacy layouts and layouts without that preference are responsive defaults rather than manual overrides. @@ -55,6 +59,8 @@ Task-specific restored layouts remain device-local environment state and take pr No new endpoint is introduced. +No `pr_panel_placement` user setting or other second placement contract is introduced. `saved_layouts` remains the only portable source for PR Details placement. + - `GET /api/v1/user/settings` returns `settings.saved_layouts`. - `PATCH /api/v1/user/settings` accepts `saved_layouts` as a complete replacement list and returns the updated user settings. - A `saved_layouts` update returns `400 Bad Request` when it exceeds the existing limit, contains an empty ID or name, contains duplicate IDs, or marks more than one saved profile, including reserved overrides, as default. @@ -74,6 +80,7 @@ The frontend treats the returned settings payload as authoritative after each su - Custom profiles and the selected custom default survive browser and Kandev restarts through backend user settings and are portable across the user's devices. - An unsaved editor draft does not survive navigation or restart. - Per-task layout state continues to use its existing environment-scoped persistence and is not made portable by this feature. +- Existing saved profiles and task-specific layouts are not backfilled with PR Details. The updated built-in templates affect fresh environments and explicit Reset Layout actions; users opt existing custom profiles into the panel through the editor. - A saved default right-column geometry adapts to the current workbench width on reload, monitor switch, and return to a wider monitor. A manual right-column width keeps its raw requested width across those events and is only clamped while the current screen cannot accommodate it. - A task handoff within the same environment does not persist an intermediate panel-removal state or change the root split orientation. - Completing a normal, non-maximized desktop task-layout restore re-establishes a live Agent panel for the active session before the workbench is revealed. If the restored center group is empty, Agent is inserted there and activated; if another valid saved center tab such as Plan is active, that tab remains active. @@ -86,6 +93,13 @@ The frontend treats the returned settings payload as authoritative after each su - **GIVEN** a customized built-in layout, **WHEN** the user chooses Reset and saves, **THEN** its hidden override is removed and the original code-defined layout is restored. - **GIVEN** a customized built-in layout, **WHEN** the user selects that built-in from the task workbench layout menu, **THEN** the saved override is applied instead of the original code-defined template. - **GIVEN** a valid custom profile, **WHEN** the user reorders tabs or moves a panel into a new split and saves, **THEN** reopening the profile shows the same tab order, active tab, split order, and proportions. +- **GIVEN** the code-defined Default layout, **WHEN** a fresh desktop task environment opens or returns from Plan Mode, **THEN** Agent is selected with PR Details as its background tab in the center group and Files remains selected in the top-right Files and Changes group. +- **GIVEN** a compact desktop task environment, **WHEN** the built-in compact layout is applied, **THEN** PR Details is available as a tab in the single workbench group. +- **GIVEN** a user moves PR Details to another group in the Layout editor and saves that profile, **WHEN** the profile is used for a fresh task or through Reset Layout, **THEN** the canonical panel opens in the saved group without a separate appearance setting. +- **GIVEN** PR Details is present but the active task has no linked pull request or merge request, **WHEN** review data settles, **THEN** the panel remains in the configured group and shows its normal empty state. +- **GIVEN** a user removes PR Details from a profile or task layout, **WHEN** a pull request or merge request is later linked, **THEN** Kandev does not recreate or move the canonical panel automatically. +- **GIVEN** the canonical PR Details panel is configured in a non-Agent group, **WHEN** the user explicitly opens a second pull request or merge request, **THEN** the new keyed review tab joins that group; reopening an existing keyed tab focuses it without moving it. +- **GIVEN** a layout without the canonical PR Details panel, **WHEN** the user explicitly opens a specific pull request or merge request, **THEN** the keyed review tab opens in the center fallback group without creating a new split. - **GIVEN** a default profile without Terminal and a task environment with no saved layout, **WHEN** the user first opens that task, **THEN** the workbench has no Terminal tab and no default user shell is created. - **GIVEN** an existing task with a task-specific layout and no pending inactive-task changes, **WHEN** the user changes the default profile and returns to that task, **THEN** the task-specific layout and saved active panel are unchanged. - **GIVEN** an existing desktop task with a saved active panel and a Changes panel outside the Agent group, **WHEN** a meaningful Git or commit update is detected while the task is inactive and the user returns to it, **THEN** the task-specific geometry is restored and Changes becomes the active panel after restoration completes. @@ -107,6 +121,6 @@ The frontend treats the returned settings payload as authoritative after each su - Auto-activating Changes merely because a task has an already-known non-zero change count. - Changing the global app sidebar width or other layout-profile split proportions. - Forcing a changed default onto existing task-specific layouts without Reset Layout. -- Configuring task-specific panels such as individual files, diffs, commits, pull requests, extra sessions, or extra terminals. +- Configuring task-specific panels such as individual files, diffs, commits, keyed pull-request or merge-request tabs, extra sessions, or extra terminals. The canonical PR Details panel is reusable and remains in scope. - Mutating the code-defined built-in definitions; direct edits are persisted as hidden user overrides. - Sharing profiles between users or scoping profiles to a workspace, repository, agent, or executor.