fix(web): make PR CI status reachable on mobile via tap-activated drawer (#1060)

* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-24 14:39:02 +01:00
parent 3bc092bf20
commit c913522626
4 changed files with 460 additions and 15 deletions
@@ -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<typeof import("@/lib/api/domains/github-api")>();
+  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<AppState> | undefined, ui: ReactNode) {
+  return render(
+    <StateProvider initialState={initialState}>
+      <ToastProvider>
+        <TooltipProvider>{ui}</TooltipProvider>
+      </ToastProvider>
+    </StateProvider>,
+  );
+}
+
+function makePR(overrides: Partial<TaskPR> = {}): 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<AppState> = {
+  taskPRs: { byTaskId: { "task-1": [makePR()] } },
+};
+
+describe("PRStatusChip", () => {
+  it("returns null when the task has no PR", () => {
+    renderWithStore(undefined, <PRStatusChip taskId="missing" />);
+    expect(screen.queryByTestId(CHIP_TESTID)).toBeNull();
+  });
+
+  describe("desktop branch", () => {
+    beforeEach(() => isMobileMock.mockReturnValue(false));
+
+    it("renders the chip button without a Drawer", () => {
+      renderWithStore(seededState, <PRStatusChip taskId="task-1" />);
+      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, <PRStatusChip taskId="task-1" />);
+      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, <PRStatusChip taskId="task-1" />);
+      // 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, <PRStatusChip taskId="task-1" />);
+      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" })] } } },
+        <PRStatusChip taskId="task-1" />,
+      );
+      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: "",
+                }),
+              ],
+            },
+          },
+        },
+        <PRStatusChip taskId="task-1" />,
+      );
+      act(() => {
+        fireEvent.click(screen.getByTestId(CHIP_TESTID));
+      });
+      expect(document.querySelector("[data-testid='pr-checks-empty']")).not.toBeNull();
+    });
+  });
+});
@@ -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 <PRStatusChipInner pr={pr} />;
 }

+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 <PRStatusChipDrawer pr={pr} />;
+  return <PRStatusChipHoverCard pr={pr} />;
+}
+
+function PRStatusChipHoverCard({ pr }: { pr: TaskPR }) {
  const status = chipStatus(pr);
  const triggerRef = useRef<HTMLButtonElement>(null);
  return (
    <HoverCard openDelay={HOVER_OPEN_DELAY_MS} closeDelay={HOVER_CLOSE_DELAY_MS}>
      <HoverCardTrigger asChild>
-        <button
-          ref={triggerRef}
-          type="button"
-          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"
-        >
+        <button ref={triggerRef} type="button" {...chipButtonAttrs(pr, status)}>
          <IconChecklist className="h-3.5 w-3.5 text-muted-foreground" aria-hidden="true" />
          <ChipStatusGlyph status={status} />
        </button>
@@ -102,6 +134,47 @@ function PRStatusChipInner({ pr }: { pr: TaskPR }) {
  );
 }

+function PRStatusChipDrawer({ pr }: { pr: TaskPR }) {
+  const status = chipStatus(pr);
+  const [open, setOpen] = useState(false);
+  return (
+    <Drawer open={open} onOpenChange={setOpen}>
+      <button
+        type="button"
+        aria-haspopup="dialog"
+        aria-expanded={open}
+        onClick={() => setOpen(true)}
+        {...chipButtonAttrs(pr, status)}
+      >
+        <IconChecklist className="h-3.5 w-3.5 text-muted-foreground" aria-hidden="true" />
+        <ChipStatusGlyph status={status} />
+      </button>
+      <DrawerContent data-testid="pr-status-chip-drawer" className="max-h-[80vh] flex flex-col">
+        <DrawerHeader className="flex flex-row items-center justify-between border-b py-2">
+          <DrawerTitle className="text-sm">PR #{pr.pr_number}</DrawerTitle>
+          <DrawerDescription className="sr-only">
+            Pull request CI status, reviews, and checks summary.
+          </DrawerDescription>
+          <DrawerClose asChild>
+            <Button
+              data-testid="pr-status-chip-drawer-close"
+              variant="ghost"
+              size="icon-sm"
+              aria-label="Close PR status"
+              className="cursor-pointer"
+            >
+              <IconX className="h-4 w-4" />
+            </Button>
+          </DrawerClose>
+        </DrawerHeader>
+        <div className="flex-1 min-h-0 overflow-y-auto p-3" data-vaul-no-drag>
+          <PRCIPopover pr={pr} enabled={open} />
+        </div>
+      </DrawerContent>
+    </Drawer>
+  );
+}
+
 function ChipStatusGlyph({ status }: { status: ChipStatus }) {
  switch (status) {
    case "passed":