feat(review): navigate to code-review findings from the Changes panel (#1963)
* feat(review): navigate to code-review findings from the Changes panel The findings count in the review top bar was a passive badge, so a user had to scroll the diff to hunt for each inline finding. Turn it into a navigator: it opens a popover listing every open finding grouped by file (most-severe first), and clicking one jumps to the finding's card in the diff, scrolls it into view, and briefly flashes it. - lib/review/navigation.ts: navigateToFinding selects the finding's file, emits an event, then polls across frames (files render lazily) for the card and scrolls + flashes it. Reuses the composite <repo>\x00<path> key so cross-repo same-named files stay distinct. - review-findings-overview + review-findings-button: the grouped list and its click-to-open popover trigger, mirroring the FixCommentsButton / ReviewCommentsOverview pattern. The badge moves out of ReviewRunButton into the top bar, keeping the review-open-count testid and copy. - inline-review-finding: stamps data-review-finding-id + scroll margin so navigation can target the card. - unanchored-findings-banner: auto-expands when a navigation targets one of its stale findings so that card renders and can be scrolled to. - globals.css: review-finding-flash highlight, respecting reduced motion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): harden finding navigation from code-review feedback Address automated review of the findings navigator: - navigation.ts: scope the finding lookup to the open review dialog so a duplicate card rendered by the Changes panel behind it is not scrolled to instead; re-emit the navigate event each poll frame so a lazily mounted stale-findings banner still expands (a one-shot event fired before the section rendered was missed); honor prefers-reduced-motion for the scroll, matching the flash animation. - review-dialog-surface: clear the sidebar file filter before selecting a finding's file, so a finding in a filtered-out file still mounts. - review-findings-button: surface a warning when the card can't be reached instead of discarding the promise silently. - review-findings-overview: compute the file directory once per row. - Tests: unit coverage for the banner auto-expand and the findings button (null render + navigate), plus a mobile-* e2e that taps the navigator and verifies the target card is reached. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
João Salavisa
@@ -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. */
|
||||
|
||||
@@ -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 (
|
||||
<div className="my-1 px-2">
|
||||
<div className="my-1 scroll-mt-24 px-2 rounded-md" {...{ [FINDING_DOM_ATTR]: finding.id }}>
|
||||
<ReviewFindingCard
|
||||
finding={finding}
|
||||
staleReason={staleReason}
|
||||
|
||||
@@ -0,0 +1,92 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { act, cleanup, fireEvent, render, screen } from "@testing-library/react";
|
||||
import type { TaskReviewFinding } from "@/lib/types/review";
|
||||
import { NAVIGATE_FINDING_EVENT } from "@/lib/review/navigation";
|
||||
import { UnanchoredFindingsBanner } from "./unanchored-findings-banner";
|
||||
|
||||
// The real card wires store-backed actions; the banner's own logic (open-filter
|
||||
// + auto-expand) is what this test covers, so stub the child.
|
||||
vi.mock("./inline-review-finding", () => ({
|
||||
InlineReviewFinding: ({ finding }: { finding: TaskReviewFinding }) => (
|
||||
<div data-testid="inline-finding" data-id={finding.id} />
|
||||
),
|
||||
}));
|
||||
|
||||
function finding(overrides: Partial<TaskReviewFinding> = {}): 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(
|
||||
<UnanchoredFindingsBanner findings={[finding({ status: "resolved" })]} />,
|
||||
);
|
||||
expect(container.firstChild).toBeNull();
|
||||
});
|
||||
|
||||
it("is collapsed by default", () => {
|
||||
render(<UnanchoredFindingsBanner findings={[finding()]} />);
|
||||
expect(expandedState()).toBe("false");
|
||||
expect(screen.queryByTestId(INLINE_FINDING_ID)).toBeNull();
|
||||
});
|
||||
|
||||
it("expands and renders its cards on click", () => {
|
||||
render(<UnanchoredFindingsBanner findings={[finding()]} />);
|
||||
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(<UnanchoredFindingsBanner findings={[finding({ id: "s1" })]} />);
|
||||
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(<UnanchoredFindingsBanner findings={[finding({ id: "s1" })]} />);
|
||||
dispatchNavigate("not-mine");
|
||||
expect(expandedState()).toBe("false");
|
||||
expect(screen.queryByTestId(INLINE_FINDING_ID)).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -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<NavigateFindingEventDetail>).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 (
|
||||
|
||||
@@ -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<HTMLDivElement>(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 (
|
||||
<Dialog open={open} onOpenChange={onOpenChange}>
|
||||
<DialogContent
|
||||
@@ -100,6 +112,7 @@ export function ReviewDialogSurface(props: ReviewDialogSurfaceProps) {
|
||||
onToggleWordWrap={state.setWordWrap}
|
||||
onSendComments={state.handleSendComments}
|
||||
onClose={() => onOpenChange(false)}
|
||||
onSelectFile={navigateToFindingFile}
|
||||
onRequestWalkthrough={props.onRequestWalkthrough}
|
||||
requestWalkthroughDisabled={state.allFiles.length === 0}
|
||||
getPendingComments={state.getPendingComments}
|
||||
|
||||
@@ -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> = {}): 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(
|
||||
<TooltipProvider>
|
||||
<ReviewFindingsButton findings={findings} onSelectFile={onSelectFile} />
|
||||
</TooltipProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
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());
|
||||
});
|
||||
});
|
||||
@@ -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 (
|
||||
<Popover open={open} onOpenChange={setOpen}>
|
||||
<PopoverTrigger asChild>
|
||||
<Button
|
||||
size="sm"
|
||||
variant="outline"
|
||||
className="h-6 cursor-pointer gap-1 px-1.5 text-[10px]"
|
||||
aria-label={`Go to ${count} review finding${count === 1 ? "" : "s"}`}
|
||||
data-testid="review-open-count"
|
||||
>
|
||||
{count} finding{count === 1 ? "" : "s"}
|
||||
<IconChevronDown className="h-3 w-3" />
|
||||
</Button>
|
||||
</PopoverTrigger>
|
||||
<PopoverContent align="end" className="w-80 p-0" data-testid="review-findings-popover">
|
||||
<ReviewFindingsOverview findings={findings} onNavigate={handleNavigate} />
|
||||
</PopoverContent>
|
||||
</Popover>
|
||||
);
|
||||
}
|
||||
@@ -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> = {}): 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(
|
||||
<ReviewFindingsOverview findings={[finding({ status: "dismissed" })]} onNavigate={vi.fn()} />,
|
||||
);
|
||||
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(<ReviewFindingsOverview findings={[target]} onNavigate={onNavigate} />);
|
||||
fireEvent.click(screen.getByText("Click me"));
|
||||
expect(onNavigate).toHaveBeenCalledWith(target);
|
||||
});
|
||||
|
||||
it("summarizes the open count across files", () => {
|
||||
render(
|
||||
<ReviewFindingsOverview
|
||||
findings={[
|
||||
finding({ id: "a", file_path: "a.ts" }),
|
||||
finding({ id: "b", file_path: "b.ts" }),
|
||||
]}
|
||||
onNavigate={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("2 open findings")).toBeTruthy();
|
||||
expect(screen.getByText("across 2 files")).toBeTruthy();
|
||||
});
|
||||
});
|
||||
@@ -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<string, TaskReviewFinding[]>();
|
||||
const pathByKey = new Map<string, string>();
|
||||
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 (
|
||||
<div className="px-3 py-4 text-center text-xs text-muted-foreground">No open findings.</div>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<div className="flex max-h-[min(60vh,26rem)] flex-col" data-testid="review-findings-overview">
|
||||
<div className="flex items-center gap-2 border-b border-border/60 px-3 py-2">
|
||||
<IconSparkles className="h-4 w-4 shrink-0 text-primary" />
|
||||
<span className="text-sm font-medium">
|
||||
{total} open finding{total !== 1 ? "s" : ""}
|
||||
</span>
|
||||
<span className="text-xs text-muted-foreground">
|
||||
across {groups.length} file{groups.length !== 1 ? "s" : ""}
|
||||
</span>
|
||||
</div>
|
||||
|
||||
<div
|
||||
className="min-h-0 flex-1 overflow-y-auto overscroll-contain px-2 py-1.5"
|
||||
data-testid="review-findings-overview-scroll"
|
||||
>
|
||||
{groups.map((group) => {
|
||||
const dir = fileDir(group.filePath);
|
||||
return (
|
||||
<div key={group.key} className="mb-2 last:mb-0">
|
||||
<div className="flex items-baseline gap-1.5 px-1 pb-1">
|
||||
<span
|
||||
className="truncate text-xs font-medium text-foreground"
|
||||
title={group.filePath}
|
||||
>
|
||||
{fileName(group.filePath)}
|
||||
</span>
|
||||
{dir && (
|
||||
<span className="min-w-0 flex-1 truncate text-[11px] text-muted-foreground/70">
|
||||
{dir}
|
||||
</span>
|
||||
)}
|
||||
<span className="ml-auto shrink-0 rounded-full bg-muted px-1.5 py-0.5 text-[10px] font-medium text-muted-foreground">
|
||||
{group.findings.length}
|
||||
</span>
|
||||
</div>
|
||||
|
||||
<div className="space-y-1">
|
||||
{group.findings.map((finding) => (
|
||||
<button
|
||||
key={finding.id}
|
||||
type="button"
|
||||
onClick={() => onNavigate(finding)}
|
||||
className="w-full cursor-pointer rounded-md border border-border/50 bg-muted/30 px-2 py-1.5 text-left transition-colors hover:border-border hover:bg-muted/60"
|
||||
data-testid="review-finding-nav-item"
|
||||
data-finding-id={finding.id}
|
||||
>
|
||||
<div className="mb-0.5 flex items-center gap-1.5">
|
||||
<ReviewFindingSeverityBadge severity={finding.severity} />
|
||||
<span className="text-[10px] font-medium text-muted-foreground">
|
||||
{formatLineRange(finding.start_line, finding.end_line)}
|
||||
</span>
|
||||
</div>
|
||||
<p className="line-clamp-2 text-xs font-medium leading-snug text-foreground/90">
|
||||
{finding.title}
|
||||
</p>
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -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({
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{!running && openCount > 0 && (
|
||||
<Badge
|
||||
variant="outline"
|
||||
className="px-1.5 py-0 text-[10px]"
|
||||
data-testid="review-open-count"
|
||||
>
|
||||
{openCount} finding{openCount === 1 ? "" : "s"}
|
||||
</Badge>
|
||||
)}
|
||||
|
||||
{shown && <RunNotice code={shown.code} message={shown.message} />}
|
||||
</div>
|
||||
);
|
||||
|
||||
@@ -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}
|
||||
/>
|
||||
)}
|
||||
<ReviewRunButton
|
||||
taskId={activeTaskId}
|
||||
sessionId={sessionId}
|
||||
activeRun={activeRun}
|
||||
openCount={openCount}
|
||||
/>
|
||||
<ReviewRunButton taskId={activeTaskId} sessionId={sessionId} activeRun={activeRun} />
|
||||
{!reviewRunning && <ReviewFindingsButton findings={findings} onSelectFile={onSelectFile} />}
|
||||
<ReviewWalkthroughButton
|
||||
onRequestWalkthrough={onRequestWalkthrough}
|
||||
disabled={requestWalkthroughDisabled}
|
||||
|
||||
@@ -113,6 +113,26 @@ test.describe("Native code review — on demand", () => {
|
||||
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(
|
||||
|
||||
@@ -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<string, unknown>) => Promise<void>;
|
||||
}) {
|
||||
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,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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> = {}): 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 <repo>\x00<path> 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);
|
||||
});
|
||||
});
|
||||
@@ -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<NavigateFindingEventDetail>(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<HTMLElement>('[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<boolean> {
|
||||
if (typeof document === "undefined") return Promise.resolve(false);
|
||||
return new Promise((resolve) => {
|
||||
let attempts = 0;
|
||||
const tick = () => {
|
||||
emitNavigateFinding(findingId);
|
||||
const el = findingScope().querySelector<HTMLElement>(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<boolean> {
|
||||
selectFile(findingFileKey(finding));
|
||||
return scrollToFinding(finding.id);
|
||||
}
|
||||
Reference in New Issue
Block a user