From c913522626304ecc00932ebffeef51fb53bc31c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sun, 24 May 2026 14:39:02 +0100 Subject: [PATCH] fix(web): make PR CI status reachable on mobile via tap-activated drawer (#1060) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): make PR CI status reachable on mobile via tap-activated drawer PRStatusChip's HoverCard was hover-only, so touch users in the chat status bar had no way to open the CI status surface. Branch on useIsMobile(): keep the existing HoverCard on desktop, and on mobile render a vaul Drawer hosting the same PRCIPopover body so tapping the chip opens a bottom-sheet "modal" with passes / failures / reviews. The desktop topbar PR button isn't mounted in SessionMobileTopBar (and its click path is Dockview-only), so the chip is the only surface that needs the fix today. New mobile-chrome e2e spec covers tap-to-open, failed-bucket content, and close. Co-Authored-By: Claude Opus 4.7 * docs(e2e): rephrase PR CI accessors section comment The block now spans desktop hover popover + chip + mobile drawer locators, not just the hover popover. Co-Authored-By: Claude Opus 4.7 * test(web): cover PRStatusChip desktop/mobile branching in jsdom Adds focused unit tests around the new mobile drawer path: - desktop branch renders a chip and click does NOT mount the drawer - mobile branch starts with no drawer, click mounts drawer + popover inner + close button (regression guard for the e2e's toHaveCount(0) precondition) - chip data-* attrs preserved across both branches - failed PR maps to data-status='failed' - no-checks PR shows the popover's empty state inside the drawer Close-on-dismiss can only be verified in a real browser (happy-dom doesn't fire vaul's CSS transition events) — covered by the mobile-pr-ci-chip.spec.ts e2e. Co-Authored-By: Claude Opus 4.7 * fix(web): a11y polish on mobile PR/CI chip drawer trigger Addresses bot review feedback on #1060: - Add aria-haspopup="dialog" + aria-expanded={open} on the mobile chip button (mirrors the established mobile-pill-button.tsx pattern). - Add a visually hidden DrawerDescription to satisfy Radix's accessible Dialog contract and suppress the console warning observed during unit tests. - Annotate the e2e failed-bucket count assertion with the arithmetic (3 total - 2 passing = 1 failed) so future seed-value edits stay in sync. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .../components/github/pr-status-chip.test.tsx | 185 ++++++++++++++++++ apps/web/components/github/pr-status-chip.tsx | 101 ++++++++-- apps/web/e2e/pages/session-page.ts | 28 ++- .../e2e/tests/pr/mobile-pr-ci-chip.spec.ts | 161 +++++++++++++++ 4 files changed, 460 insertions(+), 15 deletions(-) create mode 100644 apps/web/components/github/pr-status-chip.test.tsx create mode 100644 apps/web/e2e/tests/pr/mobile-pr-ci-chip.spec.ts diff --git a/apps/web/components/github/pr-status-chip.test.tsx b/apps/web/components/github/pr-status-chip.test.tsx new file mode 100644 index 0000000000..b83bda5da4 --- /dev/null +++ b/apps/web/components/github/pr-status-chip.test.tsx @@ -0,0 +1,185 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { ReactNode } from "react"; +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; +import { PRStatusChip } from "./pr-status-chip"; +import type { AppState } from "@/lib/state/store"; +import type { TaskPR } from "@/lib/types/github"; + +const isMobileMock = vi.fn(() => false); +vi.mock("@/hooks/use-mobile", () => ({ + useIsMobile: () => isMobileMock(), +})); + +vi.mock("@/lib/api/domains/github-api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getPRFeedback: vi.fn().mockResolvedValue(null), + listWorkspaceTaskPRs: vi.fn().mockResolvedValue({ task_prs: {} }), + }; +}); + +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: vi.fn(() => null), +})); + +function renderWithStore(initialState: Partial | undefined, ui: ReactNode) { + return render( + + + {ui} + + , + ); +} + +function makePR(overrides: Partial = {}): TaskPR { + return { + id: "pr-id", + task_id: "task-1", + owner: "acme", + repo: "demo", + pr_number: 42, + pr_url: "https://github.com/acme/demo/pull/42", + pr_title: "Test PR", + head_branch: "feat", + base_branch: "main", + author_login: "alice", + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "clean", + review_count: 1, + pending_review_count: 0, + comment_count: 0, + unresolved_review_threads: 0, + checks_total: 2, + checks_passing: 2, + additions: 0, + deletions: 0, + created_at: "", + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: "", + ...overrides, + }; +} + +beforeEach(() => { + isMobileMock.mockReturnValue(false); +}); + +afterEach(() => { + cleanup(); + isMobileMock.mockReset(); +}); + +const CHIP_TESTID = "pr-status-chip"; +const seededState: Partial = { + taskPRs: { byTaskId: { "task-1": [makePR()] } }, +}; + +describe("PRStatusChip", () => { + it("returns null when the task has no PR", () => { + renderWithStore(undefined, ); + expect(screen.queryByTestId(CHIP_TESTID)).toBeNull(); + }); + + describe("desktop branch", () => { + beforeEach(() => isMobileMock.mockReturnValue(false)); + + it("renders the chip button without a Drawer", () => { + renderWithStore(seededState, ); + const chip = screen.getByTestId(CHIP_TESTID); + expect(chip).toBeTruthy(); + // The chip's HoverCard popover is hover-only on desktop; clicking the + // chip must not surface the mobile Drawer testid. + act(() => { + fireEvent.click(chip); + }); + expect(document.querySelector("[data-testid='pr-status-chip-drawer']")).toBeNull(); + }); + + it("exposes the canonical data attributes that desktop tests rely on", () => { + renderWithStore(seededState, ); + const chip = screen.getByTestId(CHIP_TESTID); + expect(chip.getAttribute("data-pr-number")).toBe("42"); + expect(chip.getAttribute("data-pr-state")).toBe("open"); + expect(chip.getAttribute("data-status")).toBe("passed"); + expect(chip.getAttribute("data-pr-ready-to-merge")).toBe("true"); + }); + }); + + describe("mobile branch", () => { + beforeEach(() => isMobileMock.mockReturnValue(true)); + + it("renders the chip closed and opens the drawer on click", () => { + renderWithStore(seededState, ); + // Drawer must not be in the DOM before the user taps the chip — relied + // on by the e2e spec's `toHaveCount(0)` precondition. + expect(document.querySelector("[data-testid='pr-status-chip-drawer']")).toBeNull(); + + const chip = screen.getByTestId(CHIP_TESTID); + act(() => { + fireEvent.click(chip); + }); + + const drawer = document.querySelector("[data-testid='pr-status-chip-drawer']"); + expect(drawer).not.toBeNull(); + // Inner popover body + close button render inside the drawer. + expect(document.querySelector("[data-testid='pr-topbar-popover-inner']")).not.toBeNull(); + expect(document.querySelector("[data-testid='pr-status-chip-drawer-close']")).not.toBeNull(); + }); + + it("preserves the same data attributes as the desktop chip", () => { + renderWithStore(seededState, ); + const chip = screen.getByTestId(CHIP_TESTID); + expect(chip.getAttribute("data-pr-number")).toBe("42"); + expect(chip.getAttribute("data-pr-state")).toBe("open"); + expect(chip.getAttribute("data-status")).toBe("passed"); + expect(chip.getAttribute("data-pr-ready-to-merge")).toBe("true"); + }); + + it("reflects a failed PR with data-status='failed'", () => { + renderWithStore( + { taskPRs: { byTaskId: { "task-1": [makePR({ checks_state: "failure" })] } } }, + , + ); + expect(screen.getByTestId(CHIP_TESTID).getAttribute("data-status")).toBe("failed"); + }); + + // NOTE: vaul's close animation depends on CSS transition events that + // happy-dom does not fire, so the drawer never unmounts in this env. + // The mobile-pr-ci-chip.spec.ts e2e covers close-button dismissal in a + // real browser. + + it("renders the no-checks empty state in the drawer when the PR has no checks", () => { + renderWithStore( + { + taskPRs: { + byTaskId: { + "task-1": [ + makePR({ + checks_state: "", + checks_total: 0, + checks_passing: 0, + review_state: "", + mergeable_state: "", + }), + ], + }, + }, + }, + , + ); + act(() => { + fireEvent.click(screen.getByTestId(CHIP_TESTID)); + }); + expect(document.querySelector("[data-testid='pr-checks-empty']")).not.toBeNull(); + }); + }); +}); diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx index f45298dce1..80f18b5579 100644 --- a/apps/web/components/github/pr-status-chip.tsx +++ b/apps/web/components/github/pr-status-chip.tsx @@ -1,18 +1,29 @@ "use client"; -import { useRef } from "react"; +import { useRef, useState } from "react"; import { IconCircleCheckFilled, IconCircleXFilled, IconChecklist, IconLoader2, IconPointFilled, + IconX, } from "@tabler/icons-react"; import { HoverCard, HoverCardContent, HoverCardTrigger } from "@kandev/ui/hover-card"; +import { + Drawer, + DrawerClose, + DrawerContent, + DrawerDescription, + DrawerHeader, + DrawerTitle, +} from "@kandev/ui/drawer"; +import { Button } from "@kandev/ui/button"; import { useTaskPR } from "@/hooks/domains/github/use-task-pr"; import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; import { PRCIPopover } from "@/components/github/pr-ci-popover"; import { isPRAwaitingReview, isPRReadyToMerge } from "@/components/github/pr-task-icon"; +import { useIsMobile } from "@/hooks/use-mobile"; import type { TaskPR } from "@/lib/types/github"; const HOVER_OPEN_DELAY_MS = 150; @@ -46,8 +57,11 @@ function chipStatus(pr: TaskPR): ChipStatus { * merged → purple dot * neutral → muted dot * - * Hovering opens the full PRCIPopover anchored to the top edge so the card - * expands upward (the chip sits just above the chat input). + * Desktop: hovering opens the full PRCIPopover anchored to the top edge so the + * card expands upward (the chip sits just above the chat input). + * + * Mobile: tapping opens the same popover content inside a bottom-sheet Drawer + * — hover is unreachable on touch devices. * * Returns null when the task has no PR yet. */ @@ -60,23 +74,41 @@ export function PRStatusChip({ taskId }: { taskId: string | null }) { return ; } +type ChipButtonAttrs = { + "data-testid": "pr-status-chip"; + "data-pr-number": number; + "data-pr-state": string; + "data-status": ChipStatus; + "data-pr-ready-to-merge": "true" | "false"; + "aria-label": string; + className: string; +}; + +function chipButtonAttrs(pr: TaskPR, status: ChipStatus): ChipButtonAttrs { + return { + "data-testid": "pr-status-chip", + "data-pr-number": pr.pr_number, + "data-pr-state": pr.state, + "data-status": status, + "data-pr-ready-to-merge": isPRReadyToMerge(pr) ? "true" : "false", + "aria-label": `Pull request #${pr.pr_number} CI status`, + className: "cursor-pointer inline-flex items-center gap-1 rounded-md px-1 py-0.5 text-xs", + }; +} + function PRStatusChipInner({ pr }: { pr: TaskPR }) { + const isMobile = useIsMobile(); + if (isMobile) return ; + return ; +} + +function PRStatusChipHoverCard({ pr }: { pr: TaskPR }) { const status = chipStatus(pr); const triggerRef = useRef(null); return ( - @@ -102,6 +134,47 @@ function PRStatusChipInner({ pr }: { pr: TaskPR }) { ); } +function PRStatusChipDrawer({ pr }: { pr: TaskPR }) { + const status = chipStatus(pr); + const [open, setOpen] = useState(false); + return ( + + + + + PR #{pr.pr_number} + + Pull request CI status, reviews, and checks summary. + + + + + +
+ +
+ + + ); +} + function ChipStatusGlyph({ status }: { status: ChipStatus }) { switch (status) { case "passed": diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 280b93fb8b..8dab2a8940 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -477,13 +477,39 @@ export class SessionPage { return this.page.getByTestId("pr-approve-button"); } - // --- CI hover popover accessors (kept here so the spec stays declarative) --- + // --- PR CI accessors: desktop hover popover + chip + mobile chip drawer --- /** The single-PR hover popover content (visible after hovering the topbar button). */ prTopbarPopover(): Locator { return this.page.getByTestId("pr-topbar-popover"); } + /** Compact PR/CI status chip rendered in the chat status bar. */ + prStatusChip(): Locator { + return this.page.getByTestId("pr-status-chip"); + } + + /** Mobile bottom-sheet drawer that hosts the PR CI popover. */ + prStatusChipDrawer(): Locator { + return this.page.getByTestId("pr-status-chip-drawer"); + } + + /** Close button inside the chip's mobile drawer. */ + prStatusChipDrawerClose(): Locator { + return this.page.getByTestId("pr-status-chip-drawer-close"); + } + + /** PRCIPopover body when rendered inside the mobile chip drawer. */ + prStatusChipPopoverInner(): Locator { + return this.prStatusChipDrawer().getByTestId("pr-topbar-popover-inner"); + } + + /** Tap the chip and wait for the mobile drawer to be visible. */ + async tapPRStatusChip(): Promise { + await this.prStatusChip().tap(); + await expect(this.prStatusChipDrawer()).toBeVisible({ timeout: 5_000 }); + } + /** Multi-PR aggregate popover content. */ prTopbarPopoverAggregate(): Locator { return this.page.getByTestId("pr-topbar-popover-aggregate"); diff --git a/apps/web/e2e/tests/pr/mobile-pr-ci-chip.spec.ts b/apps/web/e2e/tests/pr/mobile-pr-ci-chip.spec.ts new file mode 100644 index 0000000000..b76d9cf12b --- /dev/null +++ b/apps/web/e2e/tests/pr/mobile-pr-ci-chip.spec.ts @@ -0,0 +1,161 @@ +// Mobile project: tap-activated PR/CI chip in the chat status bar. +// +// On mobile the desktop topbar PRTopbarButton is not mounted, and the chip's +// HoverCard is unreachable by touch. The chip wraps a vaul Drawer that hosts +// the PRCIPopover body — these specs exercise the open/close path and verify +// the popover content renders the same surface as desktop. +// +// File name starts with "mobile-" so it lands in the `mobile-chrome` project +// (Pixel 5 emulation) defined in playwright.config.ts. +import { test, expect } from "../../fixtures/test-base"; +import type { SeedData } from "../../fixtures/test-base"; +import { SessionPage } from "../../pages/session-page"; +import type { ApiClient } from "../../helpers/api-client"; + +const OWNER = "acme"; +const REPO = "demo"; +const PR_NUMBER = 99; +const PR_URL = `https://github.com/${OWNER}/${REPO}/pull/${PR_NUMBER}`; + +type SeedTaskArgs = { + apiClient: ApiClient; + seedData: SeedData; + title: string; + prOverrides?: Partial[0]>; +}; + +async function seedTaskWithPR({ + apiClient, + seedData, + title, + prOverrides = {}, +}: SeedTaskArgs): Promise { + await apiClient.mockGitHubReset(); + await apiClient.mockGitHubSetUser("test-user"); + 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], + }, + ); + await apiClient.mockGitHubAssociateTaskPR({ + task_id: task.id, + owner: OWNER, + repo: REPO, + pr_number: PR_NUMBER, + pr_url: PR_URL, + pr_title: "Add mobile CI drawer", + head_branch: "feat/mobile-drawer", + base_branch: "main", + author_login: "test-user", + state: "open", + additions: 10, + deletions: 1, + ...prOverrides, + }); + return task.id; +} + +async function openTask(testPage: import("@playwright/test").Page, taskId: string) { + await testPage.goto(`/t/${taskId}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + return session; +} + +test.describe("mobile PR CI chip drawer", () => { + test("tapping the chip opens the drawer with PR CI content", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const title = "Mobile CI tap-open"; + const taskId = await seedTaskWithPR({ + apiClient, + seedData, + title, + prOverrides: { + checks_state: "success", + checks_total: 4, + checks_passing: 4, + review_state: "approved", + review_count: 1, + }, + }); + const session = await openTask(testPage, taskId); + + // Desktop topbar PR button never mounts on mobile — sanity check that we + // are exercising the chip path and not a leftover desktop affordance. + await expect(session.prTopbarButton()).toHaveCount(0); + + await expect(session.prStatusChip()).toBeVisible({ timeout: 15_000 }); + await expect(session.prStatusChipDrawer()).toHaveCount(0); + + await session.tapPRStatusChip(); + + await expect(session.prStatusChipPopoverInner()).toBeVisible(); + await expect( + session.prStatusChipDrawer().locator("[data-testid='pr-check-group'][data-kind='passed']"), + ).toBeVisible(); + await expect(session.prStatusChipDrawer().getByTestId("pr-review-row")).toContainText( + "Approved", + ); + }); + + test("failed PR shows failed bucket inside the drawer", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(120_000); + const title = "Mobile CI failed"; + const taskId = await seedTaskWithPR({ + apiClient, + seedData, + title, + prOverrides: { + checks_state: "failure", + checks_total: 3, + checks_passing: 2, + }, + }); + const session = await openTask(testPage, taskId); + await expect(session.prStatusChip()).toBeVisible({ timeout: 15_000 }); + await session.tapPRStatusChip(); + + const failedGroup = session + .prStatusChipDrawer() + .locator("[data-testid='pr-check-group'][data-kind='failed']"); + await expect(failedGroup).toBeVisible(); + // 3 checks_total − 2 checks_passing = 1 in the failed bucket. + await expect(failedGroup.getByTestId("pr-check-group-count")).toHaveText("1"); + }); + + test("close button dismisses the drawer", async ({ testPage, apiClient, seedData }) => { + test.setTimeout(120_000); + const title = "Mobile CI close"; + const taskId = await seedTaskWithPR({ + apiClient, + seedData, + title, + prOverrides: { + checks_state: "success", + checks_total: 1, + checks_passing: 1, + }, + }); + const session = await openTask(testPage, taskId); + await expect(session.prStatusChip()).toBeVisible({ timeout: 15_000 }); + await session.tapPRStatusChip(); + await expect(session.prStatusChipDrawer()).toBeVisible(); + + await session.prStatusChipDrawerClose().tap(); + await expect(session.prStatusChipDrawer()).toHaveCount(0, { timeout: 5_000 }); + }); +});