diff --git a/apps/web/app/globals.css b/apps/web/app/globals.css index e5bb7ad47e..8464d873f6 100644 --- a/apps/web/app/globals.css +++ b/apps/web/app/globals.css @@ -652,6 +652,7 @@ .border-flash-animation::after, .node-border-running::after, .search-flash, + .review-finding-flash, .animate-queue-open, .animate-queue-close { animation: none; @@ -1731,6 +1732,22 @@ } } +/* Highlights a review finding card after "go to finding" scrolls it into view, + * so the eye lands on the right card among several inline in the diff. */ +.review-finding-flash { + animation: review-finding-flash 1.4s ease-out; +} +@keyframes review-finding-flash { + 0% { + box-shadow: 0 0 0 2px color-mix(in oklab, var(--primary) 70%, transparent); + background-color: color-mix(in oklab, var(--primary) 16%, transparent); + } + 100% { + box-shadow: 0 0 0 2px transparent; + background-color: transparent; + } +} + /* Queue panel collapsible — animate height + opacity using the Radix * Collapsible CSS variable. Used by QueueAffordance so opening/closing the * queued-messages panel feels intentional rather than popping. */ diff --git a/apps/web/components/diff/inline-review-finding.tsx b/apps/web/components/diff/inline-review-finding.tsx index 01db740a7d..406e7ab15c 100644 --- a/apps/web/components/diff/inline-review-finding.tsx +++ b/apps/web/components/diff/inline-review-finding.tsx @@ -3,6 +3,7 @@ import { useAppStore } from "@/components/state-provider"; import { useFindingActions } from "@/hooks/domains/review/use-finding-actions"; import { useSendFindingToAgent } from "@/hooks/domains/review/use-send-finding-to-agent"; +import { FINDING_DOM_ATTR } from "@/lib/review/navigation"; import type { TaskReviewFinding } from "@/lib/types/review"; import { ReviewFindingCard } from "./review-finding-card"; @@ -28,7 +29,7 @@ export function InlineReviewFinding({ const sendToAgent = useSendFindingToAgent({ taskId, sessionId }); return ( -
+
({ + InlineReviewFinding: ({ finding }: { finding: TaskReviewFinding }) => ( +
+ ), +})); + +function finding(overrides: Partial = {}): TaskReviewFinding { + return { + id: "s1", + run_id: "r1", + task_id: "t1", + repository_id: "", + repository_name: "", + file_path: "a.ts", + start_line: 1, + end_line: 1, + side: "additions", + severity: "major", + category: "correctness", + title: "stale finding", + body: "body", + suggestion: "", + anchor_text: "", + file_diff_hash: "h", + status: "open", + created_at: "2026-07-24T10:00:00Z", + updated_at: "2026-07-24T10:00:00Z", + ...overrides, + }; +} + +const TOGGLE_ID = "unanchored-findings-toggle"; +// Must match the data-testid on the stub above; the mock factory is hoisted so +// it can't reference this constant, hence the literal there. +const INLINE_FINDING_ID = "inline-finding"; + +function dispatchNavigate(findingId: string) { + act(() => { + window.dispatchEvent(new CustomEvent(NAVIGATE_FINDING_EVENT, { detail: { findingId } })); + }); +} + +/** Reads the toggle's aria-expanded state ("true" / "false"). */ +function expandedState() { + return screen.getByTestId(TOGGLE_ID).getAttribute("aria-expanded"); +} + +afterEach(cleanup); + +describe("UnanchoredFindingsBanner", () => { + it("renders nothing when there are no open findings", () => { + const { container } = render( + , + ); + expect(container.firstChild).toBeNull(); + }); + + it("is collapsed by default", () => { + render(); + expect(expandedState()).toBe("false"); + expect(screen.queryByTestId(INLINE_FINDING_ID)).toBeNull(); + }); + + it("expands and renders its cards on click", () => { + render(); + fireEvent.click(screen.getByTestId(TOGGLE_ID)); + expect(expandedState()).toBe("true"); + expect(screen.getByTestId(INLINE_FINDING_ID)).toBeTruthy(); + }); + + it("auto-expands when a navigate event targets one of its open findings", () => { + render(); + dispatchNavigate("s1"); + expect(expandedState()).toBe("true"); + expect(screen.getByTestId(INLINE_FINDING_ID).getAttribute("data-id")).toBe("s1"); + }); + + it("ignores a navigate event for a finding it does not own", () => { + render(); + dispatchNavigate("not-mine"); + expect(expandedState()).toBe("false"); + expect(screen.queryByTestId(INLINE_FINDING_ID)).toBeNull(); + }); +}); diff --git a/apps/web/components/diff/unanchored-findings-banner.tsx b/apps/web/components/diff/unanchored-findings-banner.tsx index 14cbfadef8..3f434fd013 100644 --- a/apps/web/components/diff/unanchored-findings-banner.tsx +++ b/apps/web/components/diff/unanchored-findings-banner.tsx @@ -1,9 +1,10 @@ "use client"; -import { useState } from "react"; +import { useEffect, useState } from "react"; import { IconAlertTriangle, IconChevronDown, IconChevronRight } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; import { sortFindings } from "@/lib/review/findings"; +import { NAVIGATE_FINDING_EVENT, type NavigateFindingEventDetail } from "@/lib/review/navigation"; import type { TaskReviewFinding } from "@/lib/types/review"; import { InlineReviewFinding } from "./inline-review-finding"; @@ -21,6 +22,21 @@ const STALE_REASON = export function UnanchoredFindingsBanner({ findings }: { findings: TaskReviewFinding[] }) { const [expanded, setExpanded] = useState(false); const open = findings.filter((f) => f.status === "open"); + + // A stale finding lives inside this collapsed banner, so a "go to finding" + // from the overview can't scroll to a card that isn't in the DOM. Expand when + // the navigation target is one of ours so its card renders and can be found. + useEffect(() => { + const onNavigate = (event: Event) => { + const { findingId } = (event as CustomEvent).detail ?? {}; + if (findingId && findings.some((f) => f.status === "open" && f.id === findingId)) { + setExpanded(true); + } + }; + window.addEventListener(NAVIGATE_FINDING_EVENT, onNavigate); + return () => window.removeEventListener(NAVIGATE_FINDING_EVENT, onNavigate); + }, [findings]); + if (open.length === 0) return null; return ( diff --git a/apps/web/components/review/review-dialog-surface.tsx b/apps/web/components/review/review-dialog-surface.tsx index 0eec210cdf..7affa4ac95 100644 --- a/apps/web/components/review/review-dialog-surface.tsx +++ b/apps/web/components/review/review-dialog-surface.tsx @@ -1,6 +1,6 @@ "use client"; -import { useRef } from "react"; +import { useCallback, useRef } from "react"; import { Dialog, DialogContent, DialogTitle } from "@kandev/ui/dialog"; import { useReviewSidebarResize } from "@/hooks/use-review-sidebar-resize"; import type { TaskPR } from "@/lib/types/github"; @@ -80,6 +80,18 @@ export function ReviewDialogSurface(props: ReviewDialogSurfaceProps) { const splitRowRef = useRef(null); const sidebar = useReviewSidebarResize(splitRowRef, open, state.reviewSourceKey); + // Clear the sidebar filter before selecting a finding's file: a finding may + // belong to a file the current filter hides, and ReviewDiffList only renders + // filtered files, so the target section would never mount to scroll to. + const { setFilter, handleSelectFile } = state; + const navigateToFindingFile = useCallback( + (fileKey: string) => { + setFilter(""); + handleSelectFile(fileKey); + }, + [setFilter, handleSelectFile], + ); + return ( onOpenChange(false)} + onSelectFile={navigateToFindingFile} onRequestWalkthrough={props.onRequestWalkthrough} requestWalkthroughDisabled={state.allFiles.length === 0} getPendingComments={state.getPendingComments} diff --git a/apps/web/components/review/review-findings-button.test.tsx b/apps/web/components/review/review-findings-button.test.tsx new file mode 100644 index 0000000000..32d1447d70 --- /dev/null +++ b/apps/web/components/review/review-findings-button.test.tsx @@ -0,0 +1,77 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import type { TaskReviewFinding } from "@/lib/types/review"; +import { ReviewFindingsButton } from "./review-findings-button"; + +const { navigateToFinding } = vi.hoisted(() => ({ + navigateToFinding: vi.fn((..._args: unknown[]) => Promise.resolve(true)), +})); +vi.mock("@/lib/review/navigation", () => ({ navigateToFinding })); + +function finding(overrides: Partial = {}): TaskReviewFinding { + return { + id: "f1", + run_id: "r1", + task_id: "t1", + repository_id: "", + repository_name: "", + file_path: "a.ts", + start_line: 5, + end_line: 5, + side: "additions", + severity: "blocker", + category: "correctness", + title: "A finding", + body: "body", + suggestion: "", + anchor_text: "", + file_diff_hash: "h", + status: "open", + created_at: "2026-07-24T10:00:00Z", + updated_at: "2026-07-24T10:00:00Z", + ...overrides, + }; +} + +function renderButton(findings: TaskReviewFinding[], onSelectFile = vi.fn()) { + return render( + + + , + ); +} + +afterEach(() => { + cleanup(); + navigateToFinding.mockClear(); +}); + +describe("ReviewFindingsButton", () => { + it("renders nothing when there are no open findings", () => { + const { container } = renderButton([finding({ status: "dismissed" })]); + expect(container.firstChild).toBeNull(); + expect(screen.queryByTestId("review-open-count")).toBeNull(); + }); + + it("shows the open-finding count", () => { + renderButton([finding({ id: "a" }), finding({ id: "b", status: "resolved" })]); + expect(screen.getByTestId("review-open-count").textContent).toContain("1 finding"); + }); + + it("opens the navigator and jumps to the clicked finding", async () => { + const onSelectFile = vi.fn(); + const target = finding({ id: "clickme", title: "Click me" }); + renderButton([target], onSelectFile); + + fireEvent.click(screen.getByTestId("review-open-count")); + const item = await screen.findByText("Click me"); + fireEvent.click(item); + + expect(navigateToFinding).toHaveBeenCalledTimes(1); + expect(navigateToFinding.mock.calls[0][0]).toBe(target); + expect(navigateToFinding.mock.calls[0][1]).toBe(onSelectFile); + // The popover closes on navigate. + await waitFor(() => expect(screen.queryByTestId("review-findings-popover")).toBeNull()); + }); +}); diff --git a/apps/web/components/review/review-findings-button.tsx b/apps/web/components/review/review-findings-button.tsx new file mode 100644 index 0000000000..091aa2ff12 --- /dev/null +++ b/apps/web/components/review/review-findings-button.tsx @@ -0,0 +1,63 @@ +"use client"; + +import { useCallback, useState } from "react"; +import { IconChevronDown } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; +import { openFindingCount } from "@/lib/review/findings"; +import { navigateToFinding } from "@/lib/review/navigation"; +import type { TaskReviewFinding } from "@/lib/types/review"; +import { ReviewFindingsOverview } from "./review-findings-overview"; + +export type ReviewFindingsButtonProps = { + findings: TaskReviewFinding[]; + /** Selects a file in the review diff so its section expands and scrolls in. */ + onSelectFile: (fileKey: string) => void; +}; + +/** + * The findings-count control on the review top bar. Instead of a passive badge, + * it opens a popover listing every open finding grouped by file; clicking one + * jumps to and flashes that finding's card in the diff. Renders nothing when + * there is nothing to navigate to. + */ +export function ReviewFindingsButton({ findings, onSelectFile }: ReviewFindingsButtonProps) { + const [open, setOpen] = useState(false); + const count = openFindingCount(findings); + + const handleNavigate = useCallback( + (finding: TaskReviewFinding) => { + setOpen(false); + // Fire-and-forget: the popover is already closing. If the card never + // renders within the retry budget, warn rather than fail silently. + navigateToFinding(finding, onSelectFile).then((reached) => { + if (!reached) { + console.warn("[review] could not scroll to finding:", finding.id); + } + }); + }, + [onSelectFile], + ); + + if (count === 0) return null; + + return ( + + + + + + + + + ); +} diff --git a/apps/web/components/review/review-findings-overview.test.tsx b/apps/web/components/review/review-findings-overview.test.tsx new file mode 100644 index 0000000000..1e5c62df8c --- /dev/null +++ b/apps/web/components/review/review-findings-overview.test.tsx @@ -0,0 +1,85 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import type { TaskReviewFinding } from "@/lib/types/review"; +import { ReviewFindingsOverview, groupOpenFindingsByFile } from "./review-findings-overview"; + +function finding(overrides: Partial = {}): TaskReviewFinding { + return { + id: "f1", + run_id: "r1", + task_id: "t1", + repository_id: "", + repository_name: "", + file_path: "apps/web/a.ts", + start_line: 12, + end_line: 12, + side: "additions", + severity: "minor", + category: "correctness", + title: "A finding", + body: "body", + suggestion: "", + anchor_text: "", + file_diff_hash: "h", + status: "open", + created_at: "2026-07-24T10:00:00Z", + updated_at: "2026-07-24T10:00:00Z", + ...overrides, + }; +} + +afterEach(cleanup); + +describe("groupOpenFindingsByFile", () => { + it("drops non-open findings and groups by file with the most-severe file first", () => { + const groups = groupOpenFindingsByFile([ + finding({ id: "a", file_path: "minor.ts", severity: "minor" }), + finding({ id: "b", file_path: "blocker.ts", severity: "blocker" }), + finding({ id: "c", file_path: "blocker.ts", severity: "major", start_line: 3 }), + finding({ id: "d", file_path: "resolved.ts", status: "resolved" }), + ]); + expect(groups.map((g) => g.filePath)).toEqual(["blocker.ts", "minor.ts"]); + // Within the blocker file, the blocker sorts before the major. + expect(groups[0].findings.map((f) => f.id)).toEqual(["b", "c"]); + }); + + it("keeps same-named files in different repos separate", () => { + const groups = groupOpenFindingsByFile([ + finding({ id: "a", repository_name: "web", file_path: "x.ts" }), + finding({ id: "b", repository_name: "api", file_path: "x.ts" }), + ]); + expect(groups).toHaveLength(2); + expect(groups.map((g) => g.filePath).sort()).toEqual(["api/x.ts", "web/x.ts"]); + }); +}); + +describe("ReviewFindingsOverview", () => { + it("shows an empty state when there is nothing open", () => { + render( + , + ); + expect(screen.getByText("No open findings.")).toBeTruthy(); + }); + + it("navigates to the clicked finding", () => { + const onNavigate = vi.fn(); + const target = finding({ id: "clickme", title: "Click me" }); + render(); + fireEvent.click(screen.getByText("Click me")); + expect(onNavigate).toHaveBeenCalledWith(target); + }); + + it("summarizes the open count across files", () => { + render( + , + ); + expect(screen.getByText("2 open findings")).toBeTruthy(); + expect(screen.getByText("across 2 files")).toBeTruthy(); + }); +}); diff --git a/apps/web/components/review/review-findings-overview.tsx b/apps/web/components/review/review-findings-overview.tsx new file mode 100644 index 0000000000..5e0fc20103 --- /dev/null +++ b/apps/web/components/review/review-findings-overview.tsx @@ -0,0 +1,137 @@ +"use client"; + +import { IconSparkles } from "@tabler/icons-react"; +import { ReviewFindingSeverityBadge } from "@/components/diff/review-finding-severity"; +import { findingFileKey, sortFindings } from "@/lib/review/findings"; +import { formatLineRange } from "@/lib/diff"; +import type { TaskReviewFinding } from "@/lib/types/review"; + +type FindingFileGroup = { key: string; filePath: string; findings: TaskReviewFinding[] }; + +/** + * Groups open findings by file, most-severe file first. Findings are pre-sorted + * by severity → repo → file → line, then grouped by their composite file key so + * two same-named files in different repos stay distinct and the highest-severity + * finding drives where its file appears in the list. + */ +export function groupOpenFindingsByFile(findings: TaskReviewFinding[]): FindingFileGroup[] { + const order: string[] = []; + const byKey = new Map(); + const pathByKey = new Map(); + for (const finding of sortFindings(findings.filter((f) => f.status === "open"))) { + const key = findingFileKey(finding); + const existing = byKey.get(key); + if (existing) { + existing.push(finding); + } else { + order.push(key); + pathByKey.set(key, displayPath(finding)); + byKey.set(key, [finding]); + } + } + return order.map((key) => ({ key, filePath: pathByKey.get(key)!, findings: byKey.get(key)! })); +} + +function displayPath(finding: TaskReviewFinding): string { + return finding.repository_name + ? `${finding.repository_name}/${finding.file_path}` + : finding.file_path; +} + +function fileName(filePath: string): string { + const idx = filePath.lastIndexOf("/"); + return idx === -1 ? filePath : filePath.slice(idx + 1); +} + +function fileDir(filePath: string): string { + const idx = filePath.lastIndexOf("/"); + return idx === -1 ? "" : filePath.slice(0, idx); +} + +/** + * Scrollable list of open findings, grouped per file, each row a jump target. + * Rendered inside the findings-count popover on the review top bar so the user + * can go straight to a finding instead of scrolling the diff to hunt for it. + */ +export function ReviewFindingsOverview({ + findings, + onNavigate, +}: { + findings: TaskReviewFinding[]; + onNavigate: (finding: TaskReviewFinding) => void; +}) { + const groups = groupOpenFindingsByFile(findings); + const total = groups.reduce((sum, g) => sum + g.findings.length, 0); + + if (total === 0) { + return ( +
No open findings.
+ ); + } + + return ( +
+
+ + + {total} open finding{total !== 1 ? "s" : ""} + + + across {groups.length} file{groups.length !== 1 ? "s" : ""} + +
+ +
+ {groups.map((group) => { + const dir = fileDir(group.filePath); + return ( +
+
+ + {fileName(group.filePath)} + + {dir && ( + + {dir} + + )} + + {group.findings.length} + +
+ +
+ {group.findings.map((finding) => ( + + ))} +
+
+ ); + })} +
+
+ ); +} diff --git a/apps/web/components/review/review-run-button.tsx b/apps/web/components/review/review-run-button.tsx index 910d9a1683..49e1da4627 100644 --- a/apps/web/components/review/review-run-button.tsx +++ b/apps/web/components/review/review-run-button.tsx @@ -2,7 +2,6 @@ import { useCallback, useState } from "react"; import { IconLoader2, IconSparkles, IconX } from "@tabler/icons-react"; -import { Badge } from "@kandev/ui/badge"; import { Button } from "@kandev/ui/button"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import Link from "@/components/routing/app-link"; @@ -14,7 +13,6 @@ export type ReviewRunButtonProps = { taskId: string | null | undefined; sessionId?: string | null; activeRun: TaskReviewRun | null; - openCount: number; /** Compact presentation for a toolbar with limited width. */ compact?: boolean; }; @@ -135,7 +133,6 @@ export function ReviewRunButton({ taskId, sessionId, activeRun, - openCount, compact = false, }: ReviewRunButtonProps) { const { notice, starting, start, clearNotice } = useRunReview(taskId, sessionId); @@ -172,16 +169,6 @@ export function ReviewRunButton({ )} - {!running && openCount > 0 && ( - - {openCount} finding{openCount === 1 ? "" : "s"} - - )} - {shown && }
); diff --git a/apps/web/components/review/review-top-bar.tsx b/apps/web/components/review/review-top-bar.tsx index e3024b9517..9fcd2d4e03 100644 --- a/apps/web/components/review/review-top-bar.tsx +++ b/apps/web/components/review/review-top-bar.tsx @@ -24,8 +24,10 @@ import { useTaskReview } from "@/hooks/domains/review/use-task-review"; import { getWebSocketClient } from "@/lib/ws/connection"; import { updateUserSettings } from "@/lib/api"; import { VcsSplitButton } from "@/components/vcs-split-button"; +import { isRunActive } from "@/lib/review/findings"; import { FixCommentsButton } from "./review-fix-comments-button"; import { ReviewRunButton } from "./review-run-button"; +import { ReviewFindingsButton } from "./review-findings-button"; import { ReviewPRSelector } from "./review-pr-selector"; import type { TaskPR } from "@/lib/types/github"; @@ -41,6 +43,8 @@ type ReviewTopBarProps = { onToggleWordWrap: (wrap: boolean) => void; onSendComments: (comments: DiffComment[]) => void; onClose: () => void; + /** Selects a file in the review diff — used to jump to a finding's file. */ + onSelectFile: (fileKey: string) => void; onRequestWalkthrough?: () => void; requestWalkthroughDisabled?: boolean; getPendingComments: () => DiffComment[]; @@ -202,6 +206,7 @@ export const ReviewTopBar = memo(function ReviewTopBar({ onToggleWordWrap, onSendComments, onClose, + onSelectFile, onRequestWalkthrough, requestWalkthroughDisabled, getPendingComments, @@ -212,7 +217,8 @@ export const ReviewTopBar = memo(function ReviewTopBar({ prDiffLoading, }: ReviewTopBarProps) { const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); - const { activeRun, openCount } = useTaskReview(activeTaskId); + const { activeRun, findings } = useTaskReview(activeTaskId); + const reviewRunning = isRunActive(activeRun); const reviewAutoMarkOnScroll = useAppStore((state) => state.userSettings.reviewAutoMarkOnScroll); const setUserSettings = useAppStore((state) => state.setUserSettings); const userSettings = useAppStore((state) => state.userSettings); @@ -261,12 +267,8 @@ export const ReviewTopBar = memo(function ReviewTopBar({ onFixComments={handleFixComments} /> )} - + + {!reviewRunning && } { fullPage: false, }); + // The findings count is a navigator: open it and jump straight to a finding + // instead of scrolling the diff to hunt for it. The popover portals to the + // document body, so it is scoped to the page rather than the dialog. + await dialog.getByTestId("review-open-count").click(); + const navItem = testPage.getByTestId("review-finding-nav-item").first(); + await expect(navItem).toBeVisible(); + await expect(testPage.getByTestId("review-findings-popover")).toBeVisible(); + await testPage.screenshot({ + path: "e2e-artifacts/review-04-findings-navigator.png", + fullPage: false, + }); + await navItem.click(); + await expect(dialog.locator("[data-review-finding-id]").first()).toBeInViewport({ + timeout: 15_000, + }); + await testPage.screenshot({ + path: "e2e-artifacts/review-05-jumped-to-finding.png", + fullPage: false, + }); + // A finding is advisory: resolving it is the human's call and it persists. await findingCard.getByTestId("review-finding-resolve").click(); await expect(dialog.getByTestId("review-finding-card").first()).toHaveAttribute( diff --git a/apps/web/e2e/tests/review/mobile-review-findings-nav.spec.ts b/apps/web/e2e/tests/review/mobile-review-findings-nav.spec.ts new file mode 100644 index 0000000000..d399593ba6 --- /dev/null +++ b/apps/web/e2e/tests/review/mobile-review-findings-nav.spec.ts @@ -0,0 +1,97 @@ +// Filename starts with "mobile-" so the mobile-chrome project exercises the +// touch path for the review findings navigator (the new "N findings" control). +import path from "node:path"; +import { test, expect } from "../../fixtures/test-base"; +import { GitHelper, makeGitEnv } from "../../helpers/git-helper"; +import { SessionPage } from "../../pages/session-page"; + +const REVIEWED_FILE = "review-target.ts"; + +/** Points the review runner at the mock agent via the user's default pair. */ +async function configureReviewer(apiClient: { + listInferenceAgents: () => Promise<{ + agents: Array<{ id: string; models: Array<{ id: string }> }>; + }>; + saveUserSettings: (settings: Record) => Promise; +}) { + const { agents } = await apiClient.listInferenceAgents(); + const agent = agents.find((candidate) => candidate.models.length > 0) ?? agents[0]; + if (!agent) throw new Error("mobile review nav e2e: no inference-capable agent was discovered"); + await apiClient.saveUserSettings({ + default_utility_agent_id: agent.id, + default_utility_model: agent.models[0]?.id ?? "", + }); +} + +test.describe("Review findings navigator on mobile", () => { + test.describe.configure({ retries: 2, timeout: 180_000 }); + + test("tapping the findings count jumps to a finding", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + await configureReviewer(apiClient); + + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Review Findings Nav Mobile E2E", + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle(); + + // Real working-tree changes for the reviewer to anchor findings to. + const git = new GitHelper( + path.join(backend.tmpDir, "repos", "e2e-repo"), + makeGitEnv(backend.tmpDir), + ); + git.createFile(REVIEWED_FILE, "export const existing = 1;\n"); + git.stageAll(); + git.commit("seed review target"); + git.modifyFile( + REVIEWED_FILE, + "export const existing = 1;\nexport const added = value;\nexport const other = 2;\n", + ); + + // Open the changes panel via the mobile nav, then the review dialog. + const mobileChangesButton = testPage.getByRole("button", { name: /Changes/ }).last(); + await expect(mobileChangesButton).toBeVisible({ timeout: 15_000 }); + await mobileChangesButton.tap(); + await expect(testPage.getByTestId("mobile-changes-panel")).toBeVisible({ timeout: 15_000 }); + await expect(testPage.getByTestId(`file-row-${REVIEWED_FILE}`)).toBeVisible({ + timeout: 30_000, + }); + await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog"))); + const dialog = testPage.getByRole("dialog", { name: "Review Changes" }); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + + // Run the review; findings render inline once it completes. + await dialog.getByTestId("review-run-changes").tap(); + await expect(dialog.getByTestId("review-finding-card").first()).toBeVisible({ + timeout: 90_000, + }); + + // The findings count is the touch navigator: tap it, tap a finding, and the + // target card is scrolled into view. + const findingsCount = dialog.getByTestId("review-open-count"); + await expect(findingsCount).toContainText("finding"); + await findingsCount.tap(); + const navItem = testPage.getByTestId("review-finding-nav-item").first(); + await expect(navItem).toBeVisible(); + await navItem.tap(); + await expect(dialog.locator("[data-review-finding-id]").first()).toBeInViewport({ + timeout: 15_000, + }); + }); +}); diff --git a/apps/web/lib/review/navigation.test.ts b/apps/web/lib/review/navigation.test.ts new file mode 100644 index 0000000000..9670de649a --- /dev/null +++ b/apps/web/lib/review/navigation.test.ts @@ -0,0 +1,99 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { TaskReviewFinding } from "@/lib/types/review"; +import { + FINDING_DOM_ATTR, + FINDING_FLASH_CLASS, + NAVIGATE_FINDING_EVENT, + findingSelector, + navigateToFinding, + scrollToFinding, +} from "./navigation"; + +function finding(overrides: Partial = {}): TaskReviewFinding { + return { + id: "f1", + run_id: "r1", + task_id: "t1", + repository_id: "", + repository_name: "", + file_path: "apps/web/a.ts", + start_line: 12, + end_line: 12, + side: "additions", + severity: "blocker", + category: "correctness", + title: "Nil dereference", + body: "x can be nil", + suggestion: "", + anchor_text: "", + file_diff_hash: "h", + status: "open", + created_at: "2026-07-24T10:00:00Z", + updated_at: "2026-07-24T10:00:00Z", + ...overrides, + }; +} + +function mountFindingCard(id: string): HTMLElement { + const el = document.createElement("div"); + el.setAttribute(FINDING_DOM_ATTR, id); + el.scrollIntoView = vi.fn(); + document.body.appendChild(el); + return el; +} + +afterEach(() => { + document.body.innerHTML = ""; + vi.restoreAllMocks(); +}); + +describe("findingSelector", () => { + it("escapes ids for use in a CSS selector", () => { + expect(findingSelector("a:b")).toBe(`[${FINDING_DOM_ATTR}="a\\:b"]`); + }); +}); + +describe("scrollToFinding", () => { + it("scrolls to and flashes a present card, resolving true", async () => { + const el = mountFindingCard("f1"); + const found = await scrollToFinding("f1", (cb) => cb()); + expect(found).toBe(true); + expect(el.scrollIntoView).toHaveBeenCalled(); + expect(el.classList.contains(FINDING_FLASH_CLASS)).toBe(true); + }); + + it("finds a card that renders after a few frames", async () => { + let frames = 0; + const found = await scrollToFinding("late", (cb) => { + frames += 1; + if (frames === 3) mountFindingCard("late"); + cb(); + }); + expect(found).toBe(true); + }); + + it("resolves false when the card never appears", async () => { + const found = await scrollToFinding("missing", (cb) => cb()); + expect(found).toBe(false); + }); +}); + +describe("navigateToFinding", () => { + it("selects the finding's file and dispatches the navigate event", async () => { + const target = finding({ id: "f9", repository_name: "kandev", file_path: "a.ts" }); + mountFindingCard("f9"); + const selectFile = vi.fn(); + const events: string[] = []; + const listener = (e: Event) => { + events.push((e as CustomEvent<{ findingId: string }>).detail.findingId); + }; + window.addEventListener(NAVIGATE_FINDING_EVENT, listener); + + await navigateToFinding(target, selectFile); + + // Composite \x00 key so cross-repo same-named files stay distinct. + expect(selectFile).toHaveBeenCalledWith("kandev\x00a.ts"); + expect(events).toEqual(["f9"]); + window.removeEventListener(NAVIGATE_FINDING_EVENT, listener); + }); +}); diff --git a/apps/web/lib/review/navigation.ts b/apps/web/lib/review/navigation.ts new file mode 100644 index 0000000000..791940a314 --- /dev/null +++ b/apps/web/lib/review/navigation.ts @@ -0,0 +1,134 @@ +import { findingFileKey } from "@/lib/review/findings"; +import type { TaskReviewFinding } from "@/lib/types/review"; + +/** + * DOM attribute stamped on the wrapper of every rendered finding (inline in the + * diff and inside the stale-findings banner). It is the hook `scrollToFinding` + * uses to locate a finding's card once its file section has rendered. + */ +export const FINDING_DOM_ATTR = "data-review-finding-id"; + +/** + * Window event carrying a finding id the user asked to jump to. The stale + * findings banner listens for it so it can expand itself when the target is one + * of its own — otherwise a stale finding could never be scrolled to because its + * card is collapsed out of the DOM. + */ +export const NAVIGATE_FINDING_EVENT = "review:navigate-finding"; + +/** Class that briefly highlights a finding card after it is scrolled into view. */ +export const FINDING_FLASH_CLASS = "review-finding-flash"; + +export type NavigateFindingEventDetail = { findingId: string }; + +/** CSS selector for a rendered finding card by id. */ +export function findingSelector(findingId: string): string { + return `[${FINDING_DOM_ATTR}="${CSS.escape(findingId)}"]`; +} + +const FLASH_DURATION_MS = 1400; +// A file section renders lazily once selected + scrolled into view; poll a few +// animation frames so navigation still lands after that render, then give up +// rather than leak a running loop. +const MAX_SCROLL_ATTEMPTS = 60; + +/** Emits the navigate event so a mounted stale-findings banner can expand. */ +export function emitNavigateFinding(findingId: string) { + if (typeof window === "undefined") return; + window.dispatchEvent( + new CustomEvent(NAVIGATE_FINDING_EVENT, { + detail: { findingId }, + }), + ); +} + +/** + * The subtree to search for a finding card. The Changes panel behind the review + * dialog can render the same finding inline, leaving two elements with the same + * id in the document; scoping to the open dialog keeps navigation from scrolling + * to (and flashing) the hidden background card. Falls back to the whole document + * when no dialog is open. + */ +function findingScope(): ParentNode { + if (typeof document === "undefined") return document; + const dialogs = document.querySelectorAll('[role="dialog"][data-state="open"]'); + return dialogs.length > 0 ? dialogs[dialogs.length - 1] : document; +} + +function prefersReducedMotion(): boolean { + return ( + typeof window !== "undefined" && + typeof window.matchMedia === "function" && + window.matchMedia("(prefers-reduced-motion: reduce)").matches + ); +} + +/** + * Scrolls a finding card into view and flashes it, retrying across frames while + * its file section renders. Resolves true once the card was found, false if it + * never appeared. Safe to call outside the browser (resolves false). + * + * The navigate event is re-emitted on every frame, not just once up front: the + * target file's section — and a stale finding's collapsed banner — mount lazily + * after the file is selected, so a single event fired before that render would + * be missed and the banner would never expand. + */ +export function scrollToFinding( + findingId: string, + requestFrame: (cb: () => void) => void = defaultRequestFrame, +): Promise { + if (typeof document === "undefined") return Promise.resolve(false); + return new Promise((resolve) => { + let attempts = 0; + const tick = () => { + emitNavigateFinding(findingId); + const el = findingScope().querySelector(findingSelector(findingId)); + if (el) { + el.scrollIntoView({ + behavior: prefersReducedMotion() ? "auto" : "smooth", + block: "center", + }); + flashFinding(el); + resolve(true); + return; + } + attempts += 1; + if (attempts >= MAX_SCROLL_ATTEMPTS) { + resolve(false); + return; + } + requestFrame(tick); + }; + tick(); + }); +} + +function defaultRequestFrame(cb: () => void) { + if (typeof requestAnimationFrame === "function") requestAnimationFrame(cb); + else setTimeout(cb, 16); +} + +/** Retriggers the flash animation by clearing and re-adding the class. */ +function flashFinding(el: HTMLElement) { + el.classList.remove(FINDING_FLASH_CLASS); + // Force a reflow so re-adding the class restarts the animation even when the + // same finding is navigated to twice in a row. + void el.offsetWidth; + el.classList.add(FINDING_FLASH_CLASS); + window.setTimeout(() => el.classList.remove(FINDING_FLASH_CLASS), FLASH_DURATION_MS); +} + +/** + * Jumps to a finding in the review diff: selects its file (so the section + * expands and scrolls into view), then scrolls the finding's card into view and + * flashes it. `scrollToFinding` re-emits the navigate event each frame so a + * lazily-mounted stale-findings banner still expands. Resolves false if the + * card never rendered within the retry budget. + */ +export function navigateToFinding( + finding: TaskReviewFinding, + selectFile: (fileKey: string) => void, +): Promise { + selectFile(findingFileKey(finding)); + return scrollToFinding(finding.id); +}