fix(gitlab): stop MR badge specs racing workflow auto-advance (#2443)

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

apps/web/components/gitlab/mr-topbar-button.layout-settle.test.tsx

@@ -0,0 +1,195 @@
+/**
+ * Regression cover for the MR detail panel being silently discarded when the
+ * user clicks the topbar button while a task's session is still loading.
+ *
+ * Navigating straight to /t/:id runs dockview's `onReady` before the
+ * session→env mapping hydrates, so it builds a throwaway DEFAULT layout with a
+ * null env; `switchEnvLayout` then replaces that layout wholesale via
+ * `api.fromJSON` once the env arrives. A panel added in that gap is thrown
+ * away and the click appears to do nothing. `useMRDesktopReviewOpener` must
+ * therefore re-apply the open once the env's layout has settled.
+ */
+import { createElement, useSyncExternalStore } from "react";
+import { act, cleanup, fireEvent, render, screen } from "@testing-library/react";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { MRTopbarButton } from "./mr-topbar-button";
+import type { TaskMR } from "@/lib/types/gitlab";
+
+const TRIGGER_TESTID = "mr-topbar-button";
+
+const gitlabMocks = vi.hoisted(() => ({ mrs: [] as TaskMR[] }));
+// A genuinely reactive stand-in for the dockview store: MRTopbarButton is
+// memo()-wrapped, so a plain rerender with identical props would never re-read
+// module-level mock values. Subscribers make store changes drive the re-render
+// the same way zustand does in the app.
+const dockviewMocks = vi.hoisted(() => {
+  const listeners = new Set<() => void>();
+  const state = {
+    addMRPanel: vi.fn(),
+    api: {} as object | null,
+    currentLayoutEnvId: null as string | null,
+    isRestoringLayout: false,
+  };
+  return {
+    state,
+    listeners,
+    get addMRPanel() {
+      return state.addMRPanel;
+    },
+    set(patch: Partial<typeof state>) {
+      Object.assign(state, patch);
+      listeners.forEach((l) => l());
+    },
+  };
+});
+
+vi.mock("@/lib/state/dockview-store", () => ({
+  useDockviewStore: (selector: (state: unknown) => unknown) =>
+    useSyncExternalStore(
+      (onChange: () => void) => {
+        dockviewMocks.listeners.add(onChange);
+        return () => dockviewMocks.listeners.delete(onChange);
+      },
+      () => selector(dockviewMocks.state),
+    ),
+}));
+
+vi.mock("@/components/state-provider", () => ({
+  useAppStore: (selector: (state: unknown) => unknown) =>
+    selector({
+      tasks: { activeTaskId: "task-1", activeSessionId: "session-1" },
+      workspaces: { activeId: "workspace-1" },
+      repositories: { itemsByWorkspaceId: { "workspace-1": [] } },
+      removeTaskMR: vi.fn(),
+      setMobileSessionReview: vi.fn(),
+    }),
+}));
+
+vi.mock("@/hooks/domains/gitlab/use-task-mr", () => ({
+  useGitLabAvailable: () => true,
+  useTaskMRs: () => gitlabMocks.mrs,
+  useWorkspaceMRs: vi.fn(),
+}));
+
+vi.mock("@/hooks/domains/kanban/use-task-by-id", () => ({
+  useTaskById: () => ({ id: "task-1", repositories: [] }),
+}));
+
+vi.mock("@/components/toast-provider", () => ({ useToast: () => ({ toast: vi.fn() }) }));
+vi.mock("./task-mr-link-dialog", () => ({ TaskMRLinkDialog: () => null }));
+vi.mock("./mr-automation-controls", () => ({ MRAutomationControls: () => null }));
+vi.mock("./mr-ci-popover", () => ({ MRCIPopover: () => null }));
+vi.mock("@/hooks/use-compact-task-chrome", () => ({ useTouchDrawer: () => false }));
+
+function singleMR(): TaskMR {
+  return {
+    id: "association-1",
+    task_id: "task-1",
+    host: "https://gitlab.example",
+    project_path: "group/project",
+    mr_iid: 81,
+    mr_url: "https://gitlab.example/group/project/-/merge_requests/81",
+    mr_title: "Test MR",
+    head_branch: "feature",
+    base_branch: "main",
+    author_username: "alice",
+    state: "opened",
+    approval_state: "",
+    pipeline_state: "",
+    merge_status: "",
+    draft: false,
+    approval_count: 0,
+    required_approvals: 0,
+    pipeline_jobs_total: 0,
+    pipeline_jobs_pass: 0,
+    reviewer_count: 0,
+    unapproved_reviewers: 0,
+    unresolved_discussions: 0,
+    created_at: "",
+    updated_at: "",
+  } as TaskMR;
+}
+
+describe("MRTopbarButton desktop review opener — layout settling", () => {
+  beforeEach(() => {
+    gitlabMocks.mrs = [singleMR()];
+    dockviewMocks.addMRPanel.mockClear();
+    dockviewMocks.set({ api: {}, currentLayoutEnvId: null, isRestoringLayout: false });
+  });
+
+  afterEach(cleanup);
+
+  it("re-opens the panel after the env-switch restore that would have discarded it", () => {
+    // Session still loading: dockview api exists, but no env has been adopted.
+    render(createElement(MRTopbarButton));
+    act(() => {
+      fireEvent.click(screen.getByTestId(TRIGGER_TESTID));
+    });
+    // Opened immediately so the click gives instant feedback...
+    expect(dockviewMocks.addMRPanel).toHaveBeenCalled();
+    const callsBeforeRestore = dockviewMocks.addMRPanel.mock.calls.length;
+
+    // ...then the env hydrates and switchEnvLayout replaces the layout,
+    // discarding that panel. Once it settles, the open must be re-applied.
+    act(() => {
+      dockviewMocks.set({ currentLayoutEnvId: "env-1", isRestoringLayout: false });
+    });
+
+    expect(dockviewMocks.addMRPanel.mock.calls.length).toBeGreaterThan(callsBeforeRestore);
+  });
+
+  it("does not re-open the panel on a later layout change once the env has settled", () => {
+    // Env already adopted at click time — the common case for an established
+    // session. The intent must not be retained, or a later preset switch or
+    // maximize would resurrect a panel the user has since closed.
+    dockviewMocks.set({ currentLayoutEnvId: "env-1" });
+    render(createElement(MRTopbarButton));
+    act(() => {
+      fireEvent.click(screen.getByTestId(TRIGGER_TESTID));
+    });
+    const callsAfterClick = dockviewMocks.addMRPanel.mock.calls.length;
+    expect(callsAfterClick).toBeGreaterThan(0);
+
+    // A later user-initiated layout restore (preset switch / maximize).
+    act(() => {
+      dockviewMocks.set({ isRestoringLayout: true });
+    });
+    act(() => {
+      dockviewMocks.set({ isRestoringLayout: false });
+    });
+
+    expect(dockviewMocks.addMRPanel.mock.calls.length).toBe(callsAfterClick);
+  });
+
+  it("retains the intent when a stale env id makes the layout look settled mid-remount", () => {
+    // `setApi(null)` does not reset `currentLayoutEnvId`, so navigating between
+    // tasks leaves the previous env id in place while the new dockview mounts.
+    // "Settled" must not be trusted on its own — without the dockviewReady
+    // check the click would be dropped and no later API init could open it.
+    dockviewMocks.set({ api: null, currentLayoutEnvId: "stale-env", isRestoringLayout: false });
+    render(createElement(MRTopbarButton));
+    act(() => {
+      fireEvent.click(screen.getByTestId(TRIGGER_TESTID));
+    });
+    expect(dockviewMocks.addMRPanel).not.toHaveBeenCalled();
+
+    act(() => {
+      dockviewMocks.set({ api: {}, currentLayoutEnvId: "env-1" });
+    });
+    expect(dockviewMocks.addMRPanel).toHaveBeenCalled();
+  });
+
+  it("defers the open until dockview exists when clicked before onReady", () => {
+    dockviewMocks.set({ api: null });
+    render(createElement(MRTopbarButton));
+    act(() => {
+      fireEvent.click(screen.getByTestId(TRIGGER_TESTID));
+    });
+    expect(dockviewMocks.addMRPanel).not.toHaveBeenCalled();
+
+    act(() => {
+      dockviewMocks.set({ api: {}, currentLayoutEnvId: "env-1" });
+    });
+    expect(dockviewMocks.addMRPanel).toHaveBeenCalled();
+  });
+});

apps/web/components/gitlab/mr-topbar-button.tsx

@@ -242,26 +242,43 @@ function useMRDesktopReviewOpener(mobile: boolean) {
   const addMRPanel = useDockviewStore((state) => state.addMRPanel);
   const dockviewReady = useDockviewStore((state) => state.api !== null);
   const isRestoringLayout = useDockviewStore((state) => state.isRestoringLayout);
+  // Not restoring right now is necessary but NOT sufficient. Navigating
+  // straight to /t/:id runs dockview's `onReady` while the session→env mapping
+  // is still hydrating, so it builds a throwaway DEFAULT layout with a null env
+  // (see tryRestoreLayout) and `isRestoringLayout` is false the whole time.
+  // `switchEnvLayout` only replaces that layout wholesale via `api.fromJSON`
+  // once the env arrives, discarding any panel added in the gap — the user
+  // clicks the MR button during session load and nothing ever opens. The env
+  // being adopted is what marks the layout as actually settled.
+  const envAdopted = useDockviewStore((state) => state.currentLayoutEnvId !== null);
+  const layoutSettled = envAdopted && !isRestoringLayout;
   const activeSessionId = useAppStore((state) => state.tasks.activeSessionId);
   const setMobileSessionReview = useAppStore((state) => state.setMobileSessionReview);
   const [pendingDesktopMR, setPendingDesktopMR] = useState<TaskMR | null>(null);
 
   useEffect(() => {
-    if (!dockviewReady || isRestoringLayout || !pendingDesktopMR) return;
+    if (!dockviewReady || !layoutSettled || !pendingDesktopMR) return;
+    // Only re-apply once the layout has actually settled: at that point no
+    // switchEnvLayout restore can land afterwards and discard this add. The
+    // intent is cleared here so a later user-initiated layout change (preset
+    // switch, maximize) cannot resurrect a panel the user has since closed.
     openDesktopMRReview(addMRPanel, pendingDesktopMR);
     setPendingDesktopMR(null);
-  }, [addMRPanel, dockviewReady, isRestoringLayout, pendingDesktopMR]);
+  }, [addMRPanel, dockviewReady, layoutSettled, pendingDesktopMR]);
 
   return (mr: TaskMR) => {
     if (mobile) {
       if (activeSessionId) openMobileMRReview(setMobileSessionReview, activeSessionId, mr);
       return;
     }
-    if (!dockviewReady || isRestoringLayout) {
-      setPendingDesktopMR(mr);
-    } else {
-      openDesktopMRReview(addMRPanel, mr);
-    }
+    // Open straight away when we can, so the panel appears on click rather
+    // than after the session finishes loading — but never into a layout that
+    // is being replaced right now. Retain the intent until dockview exists AND
+    // the env layout has settled: `currentLayoutEnvId` outlives `setApi(null)`,
+    // so a stale env id can leave `layoutSettled` true while dockview is still
+    // remounting, and dropping the intent there would lose the click entirely.
+    if (dockviewReady && !isRestoringLayout) openDesktopMRReview(addMRPanel, mr);
+    if (!dockviewReady || !layoutSettled) setPendingDesktopMR(mr);
   };
 }
 

apps/web/e2e/tests/gitlab/mobile-mr-task-card-badge.spec.ts

@@ -43,17 +43,15 @@ async function seedBoardTaskWithMR(apiClient: ApiClient, seedData: SeedData, tit
       updated_at: now,
     },
   ]);
-  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],
-    },
-  );
+  // No agent, deliberately — see the desktop spec's seedBoardTask. An
+  // auto-started session's `on_turn_complete` moves the card out of the start
+  // column mid-test, which is what made the desktop badge specs flaky.
+  const task = await apiClient.createTask(seedData.workspaceId, title, {
+    description: "MR badge fixture task",
+    workflow_id: seedData.workflowId,
+    workflow_step_id: seedData.startStepId,
+    repository_ids: [seedData.repositoryId],
+  });
   await apiClient.linkTaskGitLabMR(seedData.workspaceId, {
     task_id: task.id,
     repository_id: seedData.repositoryId,
@@ -73,9 +71,7 @@ test.describe("mobile GitLab MR badge on the Kanban card", () => {
 
     const kanban = new KanbanPage(testPage);
     await kanban.goto();
-    await expect(kanban.taskCardInColumn("Mobile MR badge task", seedData.startStepId)).toBeVisible(
-      { timeout: 45_000 },
-    );
+    await expect(kanban.taskCard(task.id)).toBeVisible({ timeout: 45_000 });
 
     const icon = kanban.board.getByTestId(`mr-task-icon-${task.id}`);
     await expect(icon).toBeVisible({ timeout: 15_000 });

apps/web/e2e/tests/gitlab/mr-task-card-badge.spec.ts

@@ -74,9 +74,24 @@ async function ensureGitLabConfigured(apiClient: ApiClient, seedData: SeedData):
   });
 }
 
+/**
+ * Seeds a board card with NO agent, deliberately.
+ *
+ * `createTaskWithAgent` auto-starts a session on the start step (the Kanban
+ * template's `on_enter: auto_start_agent`), and that same step carries
+ * `on_turn_complete: move_to_step review` — so the card leaves the start
+ * column the instant the mock agent's turn ends. Every assertion here was
+ * therefore racing that transition: on a loaded CI runner the agent won and
+ * the card was already in Review, and when it moved mid-assertion the badge
+ * detached from under `.hover()` and the test burned its full timeout.
+ *
+ * The badge is a pure function of the task's linked MRs, so an agent turn
+ * adds nothing to these ACs. Without one, no turn ever completes and the
+ * card stays put.
+ */
 async function seedBoardTask(apiClient: ApiClient, seedData: SeedData, title: string) {
-  return apiClient.createTaskWithAgent(seedData.workspaceId, title, seedData.agentProfileId, {
-    description: "/e2e:simple-message",
+  return apiClient.createTask(seedData.workspaceId, title, {
+    description: "MR badge fixture task",
     workflow_id: seedData.workflowId,
     workflow_step_id: seedData.startStepId,
     repository_ids: [seedData.repositoryId],
@@ -98,9 +113,10 @@ test.describe("GitLab MR badge on the Kanban card", () => {
 
     const kanban = new KanbanPage(testPage);
     await kanban.goto();
-    await expect(kanban.taskCardInColumn("Single MR badge task", seedData.startStepId)).toBeVisible(
-      { timeout: 45_000 },
-    );
+    // Located by task id rather than by (title, column): which column a card
+    // sits in is irrelevant to the badge, and coupling to it makes the
+    // assertion race any workflow transition (see seedBoardTask).
+    await expect(kanban.taskCard(task.id)).toBeVisible({ timeout: 45_000 });
 
     const icon = kanban.board.getByTestId(`mr-task-icon-${task.id}`);
     await expect(icon).toBeVisible({ timeout: 15_000 });
@@ -118,9 +134,7 @@ test.describe("GitLab MR badge on the Kanban card", () => {
 
     const kanban = new KanbanPage(testPage);
     await kanban.goto();
-    await expect(kanban.taskCardInColumn("No MR badge task", seedData.startStepId)).toBeVisible({
-      timeout: 45_000,
-    });
+    await expect(kanban.taskCard(task.id)).toBeVisible({ timeout: 45_000 });
 
     await expect(kanban.board.getByTestId(`mr-task-icon-${task.id}`)).toHaveCount(0);
   });
@@ -159,9 +173,7 @@ test.describe("GitLab MR badge on the Kanban card", () => {
 
     const kanban = new KanbanPage(testPage);
     await kanban.goto();
-    await expect(kanban.taskCardInColumn("Multi MR badge task", seedData.startStepId)).toBeVisible({
-      timeout: 45_000,
-    });
+    await expect(kanban.taskCard(task.id)).toBeVisible({ timeout: 45_000 });
 
     const icon = kanban.board.getByTestId(`mr-task-icon-${task.id}`);
     await expect(icon).toBeVisible({ timeout: 15_000 });
@@ -200,11 +212,9 @@ test.describe("GitLab MR badge on the Kanban card", () => {
 
     const kanban = new KanbanPage(testPage);
     await kanban.goto();
-    await expect(kanban.taskCardInColumn("PR and MR badge task", seedData.startStepId)).toBeVisible(
-      { timeout: 45_000 },
-    );
+    const card = kanban.taskCard(task.id);
+    await expect(card).toBeVisible({ timeout: 45_000 });
 
-    const card = kanban.taskCardInColumn("PR and MR badge task", seedData.startStepId);
     const prIcon = card.getByTestId(`pr-task-icon-${task.id}`);
     const mrIcon = card.getByTestId(`mr-task-icon-${task.id}`);
     await expect(prIcon).toBeVisible({ timeout: 15_000 });