fix(web): anchor manual PR panel open to the session's live group (#901)

2026-05-13 23:46:36 +01:00
parent 8d593bcddb
commit 39406fa778
7 changed files with 242 additions and 7 deletions
@@ -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 (
    <DropdownMenu>
@@ -231,7 +233,7 @@ function PRMultiDropdown({ prs }: { prs: TaskPR[] }) {
        {prs.map((pr) => (
          <DropdownMenuItem
            key={pr.id}
-            onClick={() => addPRPanel(prTaskKey(pr))}
+            onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)}
            className="cursor-pointer gap-2"
            data-testid={`pr-topbar-menu-item-${pr.pr_number}`}
          >
@@ -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) => (
        <DropdownMenuItem
          key={pr.id}
-          onClick={() => addPRPanel(prTaskKey(pr))}
+          onClick={() => addPRPanel(prTaskKey(pr), activeSessionId)}
          className={MENU_ITEM_CLASS}
        >
          <IconGitPullRequest className={MENU_ICON_CLASS} />
@@ -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<void> {
+    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+)$/ });
@@ -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();
+  });
+});
@@ -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);
+  });
+});
@@ -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,
      });
    },
@@ -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;