From 7e2a460f59338cde20939b34a2fdc4a8a78810a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Thu, 9 Jul 2026 20:23:17 +0200 Subject: [PATCH] fix(web): open each linked PR in its own tab from the "+" menu (#1636) --- .../components/github/multi-pr-ci-popover.tsx | 13 +- .../components/github/pr-status-chip.test.tsx | 8 +- .../components/github/pr-topbar-button.tsx | 3 +- apps/web/components/github/pr-utils.ts | 11 ++ .../task/__tests__/use-auto-pr-panel.test.ts | 127 +++++++++++++- .../task/dockview-add-panel-items.tsx | 3 +- .../components/task/dockview-session-tabs.ts | 161 ++++++++++++++---- apps/web/e2e/pages/session-page.ts | 6 +- .../e2e/tests/pr/pr-detail-auto-show.spec.ts | 140 +++++++++++++++ .../web/e2e/tests/pr/pr-multi-popover.spec.ts | 55 +++++- .../lib/state/dockview-panel-actions.test.ts | 47 ++++- apps/web/lib/state/dockview-panel-actions.ts | 30 +++- 12 files changed, 537 insertions(+), 67 deletions(-) diff --git a/apps/web/components/github/multi-pr-ci-popover.tsx b/apps/web/components/github/multi-pr-ci-popover.tsx index 0eee2a8e79..c989214b56 100644 --- a/apps/web/components/github/multi-pr-ci-popover.tsx +++ b/apps/web/components/github/multi-pr-ci-popover.tsx @@ -5,6 +5,7 @@ import { IconGitPullRequest } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; import { PRCIPopover } from "@/components/github/pr-ci-popover"; import { getPRStatusColor, pickDefaultPR } from "@/components/github/pr-task-icon"; +import { prIdentitySlug } from "@/components/github/pr-utils"; import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; import type { TaskPR } from "@/lib/types/github"; @@ -18,8 +19,16 @@ function PRFeedbackWarmer({ pr }: { pr: TaskPR }) { return null; } +/** + * DOM id for a PR tab's roving-tabindex button. + * + * @param uid - The popover instance's `useId()` value, so ids stay unique + * across multiple popovers mounted at once. + * @param pr - The PR the tab renders; identity-scoped so two PRs never + * collide within one popover. + */ function prTabDomId(uid: string, pr: TaskPR): string { - return `${uid}-pr-tab-${pr.repo}-${pr.pr_number}`; + return `${uid}-pr-tab-${prIdentitySlug(pr)}`; } function PRTab({ @@ -45,7 +54,7 @@ function PRTab({ tabIndex={active ? 0 : -1} aria-selected={active} aria-controls={panelId} - data-testid={`pr-popover-tab-${pr.repo}-${pr.pr_number}`} + data-testid={`pr-popover-tab-${prIdentitySlug(pr)}`} data-active={active ? "true" : "false"} onClick={() => onSelect(pr)} className={cn( diff --git a/apps/web/components/github/pr-status-chip.test.tsx b/apps/web/components/github/pr-status-chip.test.tsx index d6e5ab94e2..e01d03cf06 100644 --- a/apps/web/components/github/pr-status-chip.test.tsx +++ b/apps/web/components/github/pr-status-chip.test.tsx @@ -624,10 +624,10 @@ describe("PRStatusChip — multiple PRs", () => { }); expect(document.querySelector(DRAWER_SELECTOR)).not.toBeNull(); expect(document.querySelector("[data-testid='pr-multi-popover']")).not.toBeNull(); - // One tab per PR (testid is repo-scoped so same-number PRs on - // different repos stay unique). - expect(document.querySelector("[data-testid='pr-popover-tab-demo-1']")).not.toBeNull(); - expect(document.querySelector("[data-testid='pr-popover-tab-demo-2']")).not.toBeNull(); + // One tab per PR (testid is owner+repo-scoped so same-number PRs on + // different repos, or repos sharing a name across owners, stay unique). + expect(document.querySelector("[data-testid='pr-popover-tab-acme-demo-1']")).not.toBeNull(); + expect(document.querySelector("[data-testid='pr-popover-tab-acme-demo-2']")).not.toBeNull(); }); }); }); diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index cc475082c8..ff0ddbfc58 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -34,6 +34,7 @@ import { isPRWaitingOnBranchProtection, } from "@/components/github/pr-task-icon"; import { prTaskKey } from "@/components/github/pr-detail-panel"; +import { prIdentitySlug } from "@/components/github/pr-utils"; import { PR_CI_DESKTOP_POPOVER_SCROLL_CLASS, PRCIPopover } from "@/components/github/pr-ci-popover"; import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover"; import { useAppStore } from "@/components/state-provider"; @@ -307,7 +308,7 @@ function MultiPRMenuContent({ prs }: { prs: TaskPR[] }) { key={pr.id} onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)} className="cursor-pointer gap-2" - data-testid={`pr-topbar-menu-item-${pr.pr_number}`} + data-testid={`pr-topbar-menu-item-${prIdentitySlug(pr)}`} >
diff --git a/apps/web/components/github/pr-utils.ts b/apps/web/components/github/pr-utils.ts index eb70d47391..7b11411e92 100644 --- a/apps/web/components/github/pr-utils.ts +++ b/apps/web/components/github/pr-utils.ts @@ -1,4 +1,15 @@ +import type { TaskPR } from "@/lib/types/github"; + /** Shared label for the PR dockview panel tab and dropdown items. */ export function prPanelLabel(prNumber: number): string { return `PR #${prNumber}`; } + +/** + * Stable per-PR identifier for React keys, DOM ids, and e2e test ids. + * Includes owner + repo + PR number so two repos that share a name (or two + * owners that share a repo name) never collide on PR number alone. + */ +export function prIdentitySlug(pr: TaskPR): string { + return `${pr.owner}-${pr.repo}-${pr.pr_number}`; +} 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 index d49791c3c5..8cd94c54c5 100644 --- a/apps/web/components/task/__tests__/use-auto-pr-panel.test.ts +++ b/apps/web/components/task/__tests__/use-auto-pr-panel.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect } from "vitest"; -import type { DockviewApi } from "dockview-react"; -import { shouldAutoAddPRPanel, resolvePRPanelTargetGroup } from "../dockview-session-tabs"; +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 { @@ -95,3 +99,122 @@ describe("resolvePRPanelTargetGroup", () => { 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 870ff94826..9c9a2624e7 100644 --- a/apps/web/components/task/dockview-add-panel-items.tsx +++ b/apps/web/components/task/dockview-add-panel-items.tsx @@ -9,7 +9,7 @@ import { IconGitPullRequest, } from "@tabler/icons-react"; import { DropdownMenuItem } from "@kandev/ui/dropdown-menu"; -import { prPanelLabel } from "@/components/github/pr-utils"; +import { prPanelLabel, prIdentitySlug } 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"; @@ -99,6 +99,7 @@ export function AddPanelMenuItems({ key={pr.id} onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)} className={MENU_ITEM_CLASS} + data-testid={`add-panel-pr-item-${prIdentitySlug(pr)}`} > {state.prs.length > 1 diff --git a/apps/web/components/task/dockview-session-tabs.ts b/apps/web/components/task/dockview-session-tabs.ts index 644f67b1d7..461cee286d 100644 --- a/apps/web/components/task/dockview-session-tabs.ts +++ b/apps/web/components/task/dockview-session-tabs.ts @@ -14,6 +14,9 @@ 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-detail-panel"; const debug = createDebugLogger("dockview:session-tabs"); @@ -177,6 +180,105 @@ export function resolvePRPanelTargetGroup( 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 @@ -190,9 +292,16 @@ export function useAutoPRPanel() { const sessionId = useAppStore((s) => s.tasks.activeSessionId); const hasPR = useAppStore((s) => { const tid = s.tasks.activeTaskId; - return tid ? (s.taskPRs.byTaskId[tid]?.length ?? 0) > 0 : false; + 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; @@ -202,44 +311,26 @@ export function useAutoPRPanel() { const api = useDockviewStore.getState().api; if (!api) return; - const decisionParams = { - hasPR, - panelExists: !!api.getPanel("pr-detail"), + // 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, - wasOffered: wasPRPanelOffered(sessionId), - }; - const decision = shouldAutoAddPRPanel(decisionParams); - if (decision === "remove") { - api.getPanel("pr-detail")?.api.close(); - return; - } - - if (decision === "add") { - const targetGroupId = resolvePRPanelTargetGroup( - api, - sessionId, - useDockviewStore.getState().centerGroupId, - ); - focusOrAddPanel(api, { - id: "pr-detail", - component: "pr-detail", - title: "Pull Request", - position: { referenceGroup: targetGroupId }, - inactive: true, - }); - markPRPanelOffered(sessionId); - return; - } - - // "none" — panel already present or conditions not met. - // Mark as offered if the panel exists (e.g. restored from saved layout). - if (hasPR && api.getPanel("pr-detail")) { - markPRPanelOffered(sessionId); - } + centerGroupId: useDockviewStore.getState().centerGroupId, + }); }); }); - }, [taskId, hasPR, hasApi, sessionId]); + }, [taskId, hasPR, hasApi, sessionId, defaultPRKey, appStore]); } /** diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 6f3632b989..384517da56 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -608,9 +608,9 @@ export class SessionPage { return this.page.getByTestId("pr-multi-popover"); } - /** A single PR tab inside the multi-PR aggregate popover, by repo + PR number. */ - prMultiPopoverTab(repo: string, prNumber: number): Locator { - return this.page.getByTestId(`pr-popover-tab-${repo}-${prNumber}`); + /** A single PR tab inside the multi-PR aggregate popover, by owner + repo + PR number. */ + prMultiPopoverTab(owner: string, repo: string, prNumber: number): Locator { + return this.page.getByTestId(`pr-popover-tab-${owner}-${repo}-${prNumber}`); } /** 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 index 6fa754e397..cc78186808 100644 --- a/apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts +++ b/apps/web/e2e/tests/pr/pr-detail-auto-show.spec.ts @@ -490,4 +490,144 @@ test.describe("PR detail panel", () => { 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 }); + }); }); 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 3d7f5823a5..02aa5bace1 100644 --- a/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts +++ b/apps/web/e2e/tests/pr/pr-multi-popover.spec.ts @@ -142,14 +142,23 @@ test.describe("Multi-PR CI popover", () => { await expect(session.prTopbarPopoverAggregate()).toBeVisible({ timeout: 10_000 }); // Default tab = worst status: failing web#42, not first-created green api#77. - await expect(session.prMultiPopoverTab("web", 42)).toHaveAttribute("data-active", "true"); - await expect(session.prMultiPopoverTab("api", 77)).toHaveAttribute("data-active", "false"); + await expect(session.prMultiPopoverTab(OWNER, "web", 42)).toHaveAttribute( + "data-active", + "true", + ); + await expect(session.prMultiPopoverTab(OWNER, "api", 77)).toHaveAttribute( + "data-active", + "false", + ); // Body shows the failing PR's bucket groups (failed group present). await expect(session.prCheckGroup("failed")).toBeVisible(); // Switch to the green PR: failed group disappears, passed shows 4. - await session.prMultiPopoverTab("api", 77).click(); - await expect(session.prMultiPopoverTab("api", 77)).toHaveAttribute("data-active", "true"); + await session.prMultiPopoverTab(OWNER, "api", 77).click(); + await expect(session.prMultiPopoverTab(OWNER, "api", 77)).toHaveAttribute( + "data-active", + "true", + ); await expect(session.prCheckGroup("failed")).toHaveCount(0); await expect(session.prCheckGroupCount("passed")).toHaveText("4"); }); @@ -179,7 +188,10 @@ test.describe("Multi-PR CI popover", () => { await chip.hover(); await expect(session.prTopbarPopoverAggregate()).toBeVisible({ timeout: 10_000 }); - await expect(session.prMultiPopoverTab("web", 42)).toHaveAttribute("data-active", "true"); + await expect(session.prMultiPopoverTab(OWNER, "web", 42)).toHaveAttribute( + "data-active", + "true", + ); }); test("clicking the multi-PR button still opens the dropdown with per-PR rows", async ({ @@ -200,7 +212,36 @@ test.describe("Multi-PR CI popover", () => { const session = await openTaskAndWait(testPage, apiClient, seed, title); await session.prTopbarButton().click(); - await expect(testPage.getByTestId("pr-topbar-menu-item-42")).toBeVisible(); - await expect(testPage.getByTestId("pr-topbar-menu-item-77")).toBeVisible(); + await expect(testPage.getByTestId(`pr-topbar-menu-item-${OWNER}-web-42`)).toBeVisible(); + await expect(testPage.getByTestId(`pr-topbar-menu-item-${OWNER}-api-77`)).toBeVisible(); + }); + + test('selecting a different PR from the "+" add-panel menu opens its own tab', async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const title = "Multi Popover Add Panel Tabs"; + const seed = await seedTask( + apiClient, + seedData.workspaceId, + seedData.agentProfileId, + seedData.repositoryId, + title, + ); + 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). + 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.) + 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/lib/state/dockview-panel-actions.test.ts b/apps/web/lib/state/dockview-panel-actions.test.ts index 52a49afa42..bc1960e85c 100644 --- a/apps/web/lib/state/dockview-panel-actions.test.ts +++ b/apps/web/lib/state/dockview-panel-actions.test.ts @@ -457,22 +457,36 @@ describe("preview slots are independent across types", () => { describe("addPRPanel — dedup with legacy auto-shown panel", () => { const PR_KEY = "testorg/testrepo/101"; + const OTHER_PR_KEY = "testorg/testrepo/202"; const LEGACY_PR_ID = "pr-detail"; const KEYED_PR_ID = `pr-detail|${PR_KEY}`; + const OTHER_KEYED_PR_ID = `pr-detail|${OTHER_PR_KEY}`; function buildExtra(api: DockviewApi) { const store = makeStore(api); return { api, actions: buildExtraPanelActions(store.get) }; } - it("focuses the legacy auto-shown panel instead of creating a keyed duplicate", () => { + /** + * 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). + */ + function seedLegacyPanel(api: DockviewApi, prKey: string): void { + api.addPanel({ + id: LEGACY_PR_ID, + component: "pr-detail", + title: "Pull Request", + params: { prKey }, + position: { referenceGroup: CENTER_GROUP }, + }); + } + + it("reuses the legacy panel when the requested PR is what it's already showing", () => { const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, PR_KEY); - // Auto-show creates the legacy (unkeyed) panel. - actions.addPRPanel(); - expect(api.getPanel(LEGACY_PR_ID)).toBeDefined(); - - // Topbar click with a prKey must reuse the legacy panel, not add a second tab. actions.addPRPanel(PR_KEY); const prPanels = api.panels.filter( @@ -480,11 +494,28 @@ describe("addPRPanel — dedup with legacy auto-shown panel", () => { ); expect(prPanels).toHaveLength(1); expect(api.getPanel(KEYED_PR_ID)).toBeUndefined(); + expect((api.getPanel(LEGACY_PR_ID) as unknown as MockPanel).isActive).toBe(true); + }); + + it("opens a distinct tab for a different PR instead of overwriting the legacy tab", () => { + const { api, actions } = buildExtra(makeApi()); + seedLegacyPanel(api, PR_KEY); + + // Multi-repo "+" menu click for a PR other than the one the legacy tab + // is showing must NOT repurpose that tab — it must open its own tab. + actions.addPRPanel(OTHER_PR_KEY); - // Legacy panel was updated with the prKey so it renders the requested PR. const legacy = api.getPanel(LEGACY_PR_ID) as unknown as MockPanel; expect(legacy.params.prKey).toBe(PR_KEY); - expect(legacy.isActive).toBe(true); + + const other = api.getPanel(OTHER_KEYED_PR_ID) as unknown as MockPanel; + expect(other).toBeDefined(); + expect(other.isActive).toBe(true); + + const prPanels = api.panels.filter( + (p) => p.id === LEGACY_PR_ID || p.id.startsWith(`${LEGACY_PR_ID}|`), + ); + expect(prPanels).toHaveLength(2); }); it("creates a new keyed panel when no legacy panel exists", () => { diff --git a/apps/web/lib/state/dockview-panel-actions.ts b/apps/web/lib/state/dockview-panel-actions.ts index b5aec2fbb8..3bc8116b8b 100644 --- a/apps/web/lib/state/dockview-panel-actions.ts +++ b/apps/web/lib/state/dockview-panel-actions.ts @@ -465,6 +465,24 @@ 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; @@ -473,12 +491,16 @@ export function buildExtraPanelActions(get: StoreGet) { // 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), reuse it instead of adding a - // second tab. Update its params so it renders the requested PR. + // 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 && !api.getPanel(id)) { const legacy = api.getPanel("pr-detail"); - if (legacy) { - legacy.api.updateParameters({ prKey }); + const legacyKey = (legacy?.params as { prKey?: string } | undefined)?.prKey; + if (legacy && legacyKey === prKey) { legacy.api.setActive(); return; }