From 39406fa778cd498cd5e81dfa57176506e7de80f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 13 May 2026 23:46:36 +0100 Subject: [PATCH] fix(web): anchor manual PR panel open to the session's live group (#901) --- .../components/github/pr-topbar-button.tsx | 6 +- .../task/dockview-add-panel-items.tsx | 4 +- apps/web/e2e/pages/session-page.ts | 36 +++++ .../tests/pr/pr-detail-manual-open.spec.ts | 124 ++++++++++++++++++ .../lib/state/dockview-panel-actions.test.ts | 62 +++++++++ apps/web/lib/state/dockview-panel-actions.ts | 11 +- apps/web/lib/state/dockview-store.ts | 6 +- 7 files changed, 242 insertions(+), 7 deletions(-) create mode 100644 apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index fc1983d3e2..8ac1bd6380 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -140,6 +140,7 @@ function usePopoverInteractions() { function PRSingleButton({ pr }: { pr: TaskPR }) { 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 { isMobile, open, onOpenChange, handleEnter, handleLeave } = usePopoverInteractions(); // Background sync lives on PRStatusChip (always mounted in the chat @@ -158,7 +159,7 @@ function PRSingleButton({ pr }: { pr: TaskPR }) { onMouseEnter={handleEnter} onMouseLeave={handleLeave} onClick={() => { - addPRPanel(prTaskKey(pr)); + addPRPanel(prTaskKey(pr), activeSessionId); onOpenChange(false); }} > @@ -204,6 +205,7 @@ function PRMultiButton({ prs }: { prs: TaskPR[] }) { function PRMultiDropdown({ prs }: { prs: TaskPR[] }) { const addPRPanel = useDockviewStore((s) => s.addPRPanel); + const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); const aggColor = aggregatePRStatusColor(prs); return ( @@ -231,7 +233,7 @@ function PRMultiDropdown({ prs }: { prs: TaskPR[] }) { {prs.map((pr) => ( addPRPanel(prTaskKey(pr))} + onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)} className="cursor-pointer gap-2" data-testid={`pr-topbar-menu-item-${pr.pr_number}`} > diff --git a/apps/web/components/task/dockview-add-panel-items.tsx b/apps/web/components/task/dockview-add-panel-items.tsx index 8338997c3c..e9a129ba5c 100644 --- a/apps/web/components/task/dockview-add-panel-items.tsx +++ b/apps/web/components/task/dockview-add-panel-items.tsx @@ -14,6 +14,7 @@ import { DropdownMenuItem, DropdownMenuSeparator } from "@kandev/ui/dropdown-men import { prPanelLabel } from "@/components/github/pr-utils"; import { prTaskKey } from "@/components/github/pr-detail-panel"; import { useDockviewStore } from "@/lib/state/dockview-store"; +import { useAppStore } from "@/components/state-provider"; import type { TaskPR } from "@/lib/types/github"; import { RepositoryScriptsMenuItems } from "./repository-scripts-menu"; import { SessionReopenMenuItems } from "./session-reopen-menu"; @@ -53,6 +54,7 @@ export function AddPanelMenuItems({ const addFilesPanel = useDockviewStore((s) => s.addFilesPanel); const addChangesPanel = useDockviewStore((s) => s.addChangesPanel); const addPRPanel = useDockviewStore((s) => s.addPRPanel); + const activeSessionId = useAppStore((s) => s.tasks.activeSessionId); return ( <> @@ -106,7 +108,7 @@ export function AddPanelMenuItems({ {state.prs.map((pr) => ( addPRPanel(prTaskKey(pr))} + onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)} className={MENU_ITEM_CLASS} > diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 962f988e3f..1bce03a59a 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -552,6 +552,42 @@ export class SessionPage { ).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"). */ prDetailTab(): Locator { return this.page.locator(".dv-default-tab").filter({ hasText: /^(Pull Request|PR #\d+)$/ }); 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 new file mode 100644 index 0000000000..fbf6c29b93 --- /dev/null +++ b/apps/web/e2e/tests/pr/pr-detail-manual-open.spec.ts @@ -0,0 +1,124 @@ +import { test, expect } from "../../fixtures/test-base"; +import { KanbanPage } from "../../pages/kanban-page"; +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. + * + * The manual path uses `addPRPanel(prKey)` which historically anchored on + * the store's `centerGroupId`. That value can be stale across layout + * transitions and produces the "PR opens in a split" bug captured in the + * screenshot from the issue. + * + * Setup: + * 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 ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + "PR Manual Open 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: 501, + title: "Migrate Order Basket API", + state: "open", + head_branch: "feat/order-basket", + base_branch: "main", + author_login: "test-user", + repo_owner: "testorg", + repo_name: "testrepo", + additions: 953, + deletions: 20, + }, + ]); + + const taskA = await apiClient.createTask(seedData.workspaceId, "Order Basket 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: 501, + pr_url: "https://github.com/testorg/testrepo/pull/501", + pr_title: "Migrate Order Basket API", + head_branch: "feat/order-basket", + base_branch: "main", + author_login: "test-user", + additions: 953, + deletions: 20, + }); + + await expect(kanban.taskCardInColumn("Order Basket Task", doneStep.id)).toBeVisible({ + timeout: 45_000, + }); + + // Open the task — the auto-show hook adds the legacy `pr-detail` panel. + await kanban.taskCardInColumn("Order Basket Task", doneStep.id).click(); + await expect(testPage).toHaveURL(/\/t\//, { timeout: 15_000 }); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.idleInput().waitFor({ state: "visible", timeout: 30_000 }); + 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. + 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. + 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(); + }); +}); diff --git a/apps/web/lib/state/dockview-panel-actions.test.ts b/apps/web/lib/state/dockview-panel-actions.test.ts index 9b41a6f672..6e976b63db 100644 --- a/apps/web/lib/state/dockview-panel-actions.test.ts +++ b/apps/web/lib/state/dockview-panel-actions.test.ts @@ -448,3 +448,65 @@ describe("addPRPanel — dedup with legacy auto-shown panel", () => { expect(keyed.isActive).toBe(true); }); }); + +describe("addPRPanel — 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"; + + 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, + component: "chat", + title: "Session", + params: { sessionId: SESSION_ID }, + position: { referenceGroup: groupId }, + }); + } + + it("places the PR panel in the same group as the active session panel", () => { + const { api, actions } = buildExtra(makeApi()); + seedSessionInGroup(api, SESSION_GROUP); + + 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); + + const pr = api.getPanel(KEYED_PR_ID) as unknown as MockPanel; + expect(pr).toBeDefined(); + expect(pr.group.id).toBe(CENTER_GROUP); + }); + + it("falls back to centerGroupId when sessionId is not provided", () => { + 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); + }); +}); diff --git a/apps/web/lib/state/dockview-panel-actions.ts b/apps/web/lib/state/dockview-panel-actions.ts index 4726e7a9ed..d3c5988ce6 100644 --- a/apps/web/lib/state/dockview-panel-actions.ts +++ b/apps/web/lib/state/dockview-panel-actions.ts @@ -425,7 +425,7 @@ export function buildExtraPanelActions(get: StoreGet) { opts?.quiet ?? false, ); }, - addPRPanel: (prKey?: string) => { + addPRPanel: (prKey?: string, activeSessionId?: string | null) => { const { api, centerGroupId } = get(); if (!api) return; // Multi-repo: each TaskPR opens in its own panel keyed by @@ -443,11 +443,18 @@ export function buildExtraPanelActions(get: StoreGet) { 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: centerGroupId }, + position: { referenceGroup: targetGroupId }, params: prKey ? { prKey } : undefined, }); }, diff --git a/apps/web/lib/state/dockview-store.ts b/apps/web/lib/state/dockview-store.ts index 08a93ed776..46cfe392dc 100644 --- a/apps/web/lib/state/dockview-store.ts +++ b/apps/web/lib/state/dockview-store.ts @@ -115,8 +115,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. */ - addPRPanel: (prKey?: string) => 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; addTerminalPanel: (terminalId?: string, groupId?: string, environmentId?: string) => void; selectedDiff: { path: string; content?: string } | null; setSelectedDiff: (diff: { path: string; content?: string } | null) => void;