diff --git a/apps/web/components/gitlab/mr-topbar-button.layout-settle.test.tsx b/apps/web/components/gitlab/mr-topbar-button.layout-settle.test.tsx new file mode 100644 index 0000000000..cbe8a62f18 --- /dev/null +++ b/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) { + 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(); + }); +}); diff --git a/apps/web/components/gitlab/mr-topbar-button.tsx b/apps/web/components/gitlab/mr-topbar-button.tsx index 4b1e9b9d4f..ef393f4156 100644 --- a/apps/web/components/gitlab/mr-topbar-button.tsx +++ b/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(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); }; } diff --git a/apps/web/e2e/tests/gitlab/mobile-mr-task-card-badge.spec.ts b/apps/web/e2e/tests/gitlab/mobile-mr-task-card-badge.spec.ts index 770a71e61f..ef3a257047 100644 --- a/apps/web/e2e/tests/gitlab/mobile-mr-task-card-badge.spec.ts +++ b/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 }); diff --git a/apps/web/e2e/tests/gitlab/mr-task-card-badge.spec.ts b/apps/web/e2e/tests/gitlab/mr-task-card-badge.spec.ts index a670613fdf..b6686d33cd 100644 --- a/apps/web/e2e/tests/gitlab/mr-task-card-badge.spec.ts +++ b/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 });