test: add DiffViewer and FileCard viewport virtualization tests
Covers IntersectionObserver placeholder rendering for 300-file diffs,
single-file bypass, sidebar ref registration, expandAll batch fetching
(25 files → 3 batches of 10), and all FileCard lazy-load states:
default collapsed, loading, success with HunkRows, error+retry, binary,
no-hunks, detail-prop pre-expanded, and collapse/re-expand UX.
Cherry-picks viewport virtualization implementation commit (f804cb19)
onto this branch so the tests run against the actual new code.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
192
apps/web/src/components/review/DiffViewer.test.tsx
Normal file
192
apps/web/src/components/review/DiffViewer.test.tsx
Normal file
@@ -0,0 +1,192 @@
|
|||||||
|
// @vitest-environment happy-dom
|
||||||
|
import "@testing-library/jest-dom/vitest";
|
||||||
|
import { render, screen, act } from "@testing-library/react";
|
||||||
|
import { vi, describe, it, expect, beforeEach, afterEach } from "vitest";
|
||||||
|
import { DiffViewer } from "./DiffViewer";
|
||||||
|
import type { FileDiff } from "./types";
|
||||||
|
|
||||||
|
// ── Module mocks ──────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
vi.mock("./FileCard", () => ({
|
||||||
|
FileCard: ({ file }: { file: FileDiff }) => (
|
||||||
|
<div data-testid="file-card" data-path={file.newPath} />
|
||||||
|
),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// Hoist the fetch mock so it can be referenced inside vi.mock factories
|
||||||
|
const { mockGetFileDiffFetch } = vi.hoisted(() => ({
|
||||||
|
mockGetFileDiffFetch: vi.fn().mockResolvedValue({ rawDiff: "" }),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@/lib/trpc", () => ({
|
||||||
|
trpc: {
|
||||||
|
useUtils: () => ({
|
||||||
|
getFileDiff: { fetch: mockGetFileDiffFetch },
|
||||||
|
}),
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
// DiffViewer calls useQueryClient() (even though the return value is unused).
|
||||||
|
// Provide a minimal mock so the hook doesn't throw outside a QueryClientProvider.
|
||||||
|
vi.mock("@tanstack/react-query", async (importOriginal) => {
|
||||||
|
const actual =
|
||||||
|
await importOriginal<typeof import("@tanstack/react-query")>();
|
||||||
|
return { ...actual, useQueryClient: () => ({}) };
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── IntersectionObserver mock ─────────────────────────────────────────────────
|
||||||
|
|
||||||
|
let observerCallback: IntersectionObserverCallback | null = null;
|
||||||
|
const observedElements = new Set<Element>();
|
||||||
|
|
||||||
|
// Class (not arrow function) so it can be used with `new IntersectionObserver(...)`
|
||||||
|
class MockIntersectionObserver {
|
||||||
|
constructor(cb: IntersectionObserverCallback) {
|
||||||
|
observerCallback = cb;
|
||||||
|
}
|
||||||
|
observe(el: Element) {
|
||||||
|
observedElements.add(el);
|
||||||
|
}
|
||||||
|
unobserve(el: Element) {
|
||||||
|
observedElements.delete(el);
|
||||||
|
}
|
||||||
|
disconnect() {
|
||||||
|
observedElements.clear();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.stubGlobal("IntersectionObserver", MockIntersectionObserver);
|
||||||
|
observedElements.clear();
|
||||||
|
observerCallback = null;
|
||||||
|
mockGetFileDiffFetch.mockClear();
|
||||||
|
mockGetFileDiffFetch.mockResolvedValue({ rawDiff: "" });
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
vi.unstubAllGlobals();
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── Helpers ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Fire the IntersectionObserver callback with a set of intersecting and
|
||||||
|
* non-intersecting file paths. The target element is simulated by an object
|
||||||
|
* whose dataset.filePath matches the DiffViewer's data-file-path attribute.
|
||||||
|
*/
|
||||||
|
function fireIntersection(
|
||||||
|
intersectingPaths: string[],
|
||||||
|
nonIntersectingPaths: string[] = [],
|
||||||
|
) {
|
||||||
|
if (!observerCallback) return;
|
||||||
|
const entries = [
|
||||||
|
...intersectingPaths.map((p) => ({
|
||||||
|
isIntersecting: true,
|
||||||
|
target: { dataset: { filePath: p } } as unknown as Element,
|
||||||
|
})),
|
||||||
|
...nonIntersectingPaths.map((p) => ({
|
||||||
|
isIntersecting: false,
|
||||||
|
target: { dataset: { filePath: p } } as unknown as Element,
|
||||||
|
})),
|
||||||
|
] as IntersectionObserverEntry[];
|
||||||
|
act(() => {
|
||||||
|
observerCallback!(entries, {} as IntersectionObserver);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
function makeFiles(count: number): FileDiff[] {
|
||||||
|
return Array.from({ length: count }, (_, i) => ({
|
||||||
|
oldPath: `file${i}.ts`,
|
||||||
|
newPath: `file${i}.ts`,
|
||||||
|
status: "modified" as const,
|
||||||
|
additions: 1,
|
||||||
|
deletions: 1,
|
||||||
|
}));
|
||||||
|
}
|
||||||
|
|
||||||
|
const defaultProps = {
|
||||||
|
phaseId: "phase-1",
|
||||||
|
commitMode: false,
|
||||||
|
commentsByLine: new Map(),
|
||||||
|
onAddComment: vi.fn(),
|
||||||
|
onResolveComment: vi.fn(),
|
||||||
|
onUnresolveComment: vi.fn(),
|
||||||
|
};
|
||||||
|
|
||||||
|
// ── Tests ─────────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
describe("DiffViewer", () => {
|
||||||
|
it("renders all FileCards when 5 files are all in viewport", () => {
|
||||||
|
const files = makeFiles(5);
|
||||||
|
render(<DiffViewer files={files} {...defaultProps} />);
|
||||||
|
|
||||||
|
// Trigger all five as intersecting
|
||||||
|
fireIntersection(files.map((f) => f.newPath));
|
||||||
|
|
||||||
|
expect(screen.getAllByTestId("file-card")).toHaveLength(5);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("shows only intersecting FileCards for 300 files, placeholders for the rest", () => {
|
||||||
|
const files = makeFiles(300);
|
||||||
|
render(<DiffViewer files={files} {...defaultProps} />);
|
||||||
|
|
||||||
|
// Only first 5 files enter the viewport
|
||||||
|
fireIntersection(files.slice(0, 5).map((f) => f.newPath));
|
||||||
|
|
||||||
|
expect(screen.getAllByTestId("file-card")).toHaveLength(5);
|
||||||
|
|
||||||
|
// The remaining 295 should be 48px placeholder divs marked aria-hidden
|
||||||
|
const placeholders = document.querySelectorAll(
|
||||||
|
'[aria-hidden][style*="height: 48px"]',
|
||||||
|
);
|
||||||
|
expect(placeholders.length).toBeGreaterThanOrEqual(295);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips IntersectionObserver for single-file diff and renders FileCard directly", () => {
|
||||||
|
render(<DiffViewer files={makeFiles(1)} {...defaultProps} />);
|
||||||
|
|
||||||
|
// Single-file path: isVisible is always true, no intersection event needed
|
||||||
|
expect(screen.getAllByTestId("file-card")).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("calls scrollIntoView on the wrapper div when onRegisterRef is used for sidebar navigation", () => {
|
||||||
|
const files = makeFiles(5);
|
||||||
|
const registeredRefs = new Map<string, HTMLDivElement>();
|
||||||
|
const onRegisterRef = (filePath: string, el: HTMLDivElement | null) => {
|
||||||
|
if (el) registeredRefs.set(filePath, el);
|
||||||
|
};
|
||||||
|
|
||||||
|
render(<DiffViewer files={files} {...defaultProps} onRegisterRef={onRegisterRef} />);
|
||||||
|
|
||||||
|
// All wrapper divs should have been registered (including the last one)
|
||||||
|
const targetFile = files[4].newPath;
|
||||||
|
expect(registeredRefs.has(targetFile)).toBe(true);
|
||||||
|
|
||||||
|
const wrapperEl = registeredRefs.get(targetFile)!;
|
||||||
|
const scrollSpy = vi.fn();
|
||||||
|
Object.defineProperty(wrapperEl, "scrollIntoView", { value: scrollSpy });
|
||||||
|
|
||||||
|
// Simulate a sidebar click that calls scrollIntoView on the wrapper
|
||||||
|
act(() => {
|
||||||
|
wrapperEl.scrollIntoView({ behavior: "smooth", block: "start" });
|
||||||
|
});
|
||||||
|
expect(scrollSpy).toHaveBeenCalledOnce();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("fires getFileDiff queries in batches of 10 when expandAll is toggled", async () => {
|
||||||
|
const files = makeFiles(25); // 3 batches: 10, 10, 5
|
||||||
|
const { rerender } = render(
|
||||||
|
<DiffViewer files={files} {...defaultProps} expandAll={false} />,
|
||||||
|
);
|
||||||
|
|
||||||
|
rerender(<DiffViewer files={files} {...defaultProps} expandAll={true} />);
|
||||||
|
|
||||||
|
// Wait for all async batch iterations to complete
|
||||||
|
await act(async () => {
|
||||||
|
await new Promise((r) => setTimeout(r, 100));
|
||||||
|
});
|
||||||
|
|
||||||
|
// All 25 non-binary files should have been prefetched
|
||||||
|
expect(mockGetFileDiffFetch).toHaveBeenCalledTimes(25);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,5 +1,8 @@
|
|||||||
import type { FileDiffDetail, DiffLine, ReviewComment } from "./types";
|
import { useCallback, useEffect, useRef, useState } from "react";
|
||||||
|
import { useQueryClient } from "@tanstack/react-query";
|
||||||
|
import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types";
|
||||||
import { FileCard } from "./FileCard";
|
import { FileCard } from "./FileCard";
|
||||||
|
import { trpc } from "@/lib/trpc";
|
||||||
|
|
||||||
function getFileCommentMap(
|
function getFileCommentMap(
|
||||||
commentsByLine: Map<string, ReviewComment[]>,
|
commentsByLine: Map<string, ReviewComment[]>,
|
||||||
@@ -13,7 +16,9 @@ function getFileCommentMap(
|
|||||||
}
|
}
|
||||||
|
|
||||||
interface DiffViewerProps {
|
interface DiffViewerProps {
|
||||||
files: FileDiffDetail[];
|
files: (FileDiff | FileDiffDetail)[];
|
||||||
|
phaseId: string;
|
||||||
|
commitMode: boolean;
|
||||||
commentsByLine: Map<string, ReviewComment[]>;
|
commentsByLine: Map<string, ReviewComment[]>;
|
||||||
onAddComment: (
|
onAddComment: (
|
||||||
filePath: string,
|
filePath: string,
|
||||||
@@ -28,10 +33,13 @@ interface DiffViewerProps {
|
|||||||
viewedFiles?: Set<string>;
|
viewedFiles?: Set<string>;
|
||||||
onToggleViewed?: (filePath: string) => void;
|
onToggleViewed?: (filePath: string) => void;
|
||||||
onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void;
|
onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void;
|
||||||
|
expandAll?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function DiffViewer({
|
export function DiffViewer({
|
||||||
files,
|
files,
|
||||||
|
phaseId,
|
||||||
|
commitMode,
|
||||||
commentsByLine,
|
commentsByLine,
|
||||||
onAddComment,
|
onAddComment,
|
||||||
onResolveComment,
|
onResolveComment,
|
||||||
@@ -41,14 +49,141 @@ export function DiffViewer({
|
|||||||
viewedFiles,
|
viewedFiles,
|
||||||
onToggleViewed,
|
onToggleViewed,
|
||||||
onRegisterRef,
|
onRegisterRef,
|
||||||
|
expandAll,
|
||||||
}: DiffViewerProps) {
|
}: DiffViewerProps) {
|
||||||
|
// Set of file paths currently intersecting (or near) the viewport
|
||||||
|
const visibleFiles = useRef<Set<string>>(new Set());
|
||||||
|
// Map from filePath → wrapper div ref
|
||||||
|
const wrapperRefs = useRef<Map<string, HTMLDivElement>>(new Map());
|
||||||
|
// Increment to trigger re-render when visibility changes
|
||||||
|
const [visibilityVersion, setVisibilityVersion] = useState(0);
|
||||||
|
|
||||||
|
// Single IntersectionObserver for all wrappers
|
||||||
|
const observerRef = useRef<IntersectionObserver | null>(null);
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
if (files.length === 1) return; // skip for single file
|
||||||
|
|
||||||
|
observerRef.current = new IntersectionObserver(
|
||||||
|
(entries) => {
|
||||||
|
let changed = false;
|
||||||
|
for (const entry of entries) {
|
||||||
|
const filePath = (entry.target as HTMLDivElement).dataset['filePath'];
|
||||||
|
if (!filePath) continue;
|
||||||
|
if (entry.isIntersecting) {
|
||||||
|
if (!visibleFiles.current.has(filePath)) {
|
||||||
|
visibleFiles.current.add(filePath);
|
||||||
|
changed = true;
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
if (visibleFiles.current.has(filePath)) {
|
||||||
|
visibleFiles.current.delete(filePath);
|
||||||
|
changed = true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (changed) setVisibilityVersion((v) => v + 1);
|
||||||
|
},
|
||||||
|
{ rootMargin: '100% 0px 100% 0px' }, // 1× viewport above and below
|
||||||
|
);
|
||||||
|
|
||||||
|
// Observe all current wrapper divs
|
||||||
|
for (const el of wrapperRefs.current.values()) {
|
||||||
|
observerRef.current.observe(el);
|
||||||
|
}
|
||||||
|
|
||||||
|
return () => {
|
||||||
|
observerRef.current?.disconnect();
|
||||||
|
};
|
||||||
|
}, [files]); // re-create observer when file list changes
|
||||||
|
|
||||||
|
// Register wrapper ref — observes the div, registers with parent
|
||||||
|
const registerWrapper = useCallback(
|
||||||
|
(filePath: string, el: HTMLDivElement | null) => {
|
||||||
|
if (el) {
|
||||||
|
wrapperRefs.current.set(filePath, el);
|
||||||
|
observerRef.current?.observe(el);
|
||||||
|
} else {
|
||||||
|
const prev = wrapperRefs.current.get(filePath);
|
||||||
|
if (prev) observerRef.current?.unobserve(prev);
|
||||||
|
wrapperRefs.current.delete(filePath);
|
||||||
|
}
|
||||||
|
onRegisterRef?.(filePath, el);
|
||||||
|
},
|
||||||
|
[onRegisterRef],
|
||||||
|
);
|
||||||
|
|
||||||
|
// expandAll batch loading
|
||||||
|
const [expandedFiles, setExpandedFiles] = useState<Set<string>>(new Set());
|
||||||
|
const queryClient = useQueryClient();
|
||||||
|
const utils = trpc.useUtils();
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
if (!expandAll || files.length === 0) return;
|
||||||
|
|
||||||
|
const BATCH = 10;
|
||||||
|
let cancelled = false;
|
||||||
|
|
||||||
|
async function batchExpand() {
|
||||||
|
const chunks: (FileDiff | FileDiffDetail)[][] = [];
|
||||||
|
for (let i = 0; i < files.length; i += BATCH) {
|
||||||
|
chunks.push(files.slice(i, i + BATCH));
|
||||||
|
}
|
||||||
|
|
||||||
|
for (const chunk of chunks) {
|
||||||
|
if (cancelled) break;
|
||||||
|
// Mark this batch as expanded (triggers FileCard renders + queries)
|
||||||
|
setExpandedFiles((prev) => {
|
||||||
|
const next = new Set(prev);
|
||||||
|
for (const f of chunk) {
|
||||||
|
if (f.status !== 'binary') next.add(f.newPath);
|
||||||
|
}
|
||||||
|
return next;
|
||||||
|
});
|
||||||
|
// Eagerly prefetch via React Query to saturate network
|
||||||
|
await Promise.all(
|
||||||
|
chunk
|
||||||
|
.filter((f) => f.status !== 'binary' && !('hunks' in f))
|
||||||
|
.map((f) =>
|
||||||
|
utils.getFileDiff
|
||||||
|
.fetch({ phaseId, filePath: encodeURIComponent(f.newPath) })
|
||||||
|
.catch(() => null), // swallow per-file errors; FileCard shows its own error state
|
||||||
|
),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
batchExpand();
|
||||||
|
return () => {
|
||||||
|
cancelled = true;
|
||||||
|
};
|
||||||
|
}, [expandAll]); // only re-run when expandAll toggles
|
||||||
|
// eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally only on expandAll
|
||||||
|
|
||||||
|
// Suppress unused variable warning — used only to force re-render on visibility change
|
||||||
|
void visibilityVersion;
|
||||||
|
|
||||||
|
const isSingleFile = files.length === 1;
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="space-y-4">
|
<div className="space-y-4">
|
||||||
{files.map((file) => (
|
{files.map((file) => {
|
||||||
<div key={file.newPath} ref={(el) => onRegisterRef?.(file.newPath, el)}>
|
const isVisible = isSingleFile || visibleFiles.current.has(file.newPath);
|
||||||
|
const isExpandedOverride = expandedFiles.has(file.newPath) ? true : undefined;
|
||||||
|
return (
|
||||||
|
<div
|
||||||
|
key={file.newPath}
|
||||||
|
ref={(el) => registerWrapper(file.newPath, el)}
|
||||||
|
data-file-path={file.newPath}
|
||||||
|
>
|
||||||
|
{isVisible ? (
|
||||||
<FileCard
|
<FileCard
|
||||||
file={file}
|
file={file as FileDiff}
|
||||||
|
detail={'hunks' in file ? (file as FileDiffDetail) : undefined}
|
||||||
|
phaseId={phaseId}
|
||||||
|
commitMode={commitMode}
|
||||||
commentsByLine={getFileCommentMap(commentsByLine, file.newPath)}
|
commentsByLine={getFileCommentMap(commentsByLine, file.newPath)}
|
||||||
|
isExpandedOverride={isExpandedOverride}
|
||||||
onAddComment={onAddComment}
|
onAddComment={onAddComment}
|
||||||
onResolveComment={onResolveComment}
|
onResolveComment={onResolveComment}
|
||||||
onUnresolveComment={onUnresolveComment}
|
onUnresolveComment={onUnresolveComment}
|
||||||
@@ -57,8 +192,12 @@ export function DiffViewer({
|
|||||||
isViewed={viewedFiles?.has(file.newPath) ?? false}
|
isViewed={viewedFiles?.has(file.newPath) ?? false}
|
||||||
onToggleViewed={() => onToggleViewed?.(file.newPath)}
|
onToggleViewed={() => onToggleViewed?.(file.newPath)}
|
||||||
/>
|
/>
|
||||||
|
) : (
|
||||||
|
<div style={{ height: '48px' }} aria-hidden />
|
||||||
|
)}
|
||||||
</div>
|
</div>
|
||||||
))}
|
);
|
||||||
|
})}
|
||||||
</div>
|
</div>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
270
apps/web/src/components/review/FileCard.test.tsx
Normal file
270
apps/web/src/components/review/FileCard.test.tsx
Normal file
@@ -0,0 +1,270 @@
|
|||||||
|
// @vitest-environment happy-dom
|
||||||
|
import "@testing-library/jest-dom/vitest";
|
||||||
|
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
|
||||||
|
import { vi, describe, it, expect, beforeEach } from "vitest";
|
||||||
|
import { FileCard } from "./FileCard";
|
||||||
|
import type { FileDiff, FileDiffDetail } from "./types";
|
||||||
|
|
||||||
|
// ── Module mocks ──────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
vi.mock("./HunkRows", () => ({
|
||||||
|
HunkRows: ({ hunk }: { hunk: { header: string } }) => (
|
||||||
|
<tr data-testid="hunk-row">
|
||||||
|
<td>{hunk.header}</td>
|
||||||
|
</tr>
|
||||||
|
),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("./use-syntax-highlight", () => ({
|
||||||
|
useHighlightedFile: () => null,
|
||||||
|
}));
|
||||||
|
|
||||||
|
// Hoist mocks so they can be referenced in vi.mock factories
|
||||||
|
const { mockGetFileDiff, mockParseUnifiedDiff } = vi.hoisted(() => ({
|
||||||
|
mockGetFileDiff: vi.fn(),
|
||||||
|
mockParseUnifiedDiff: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("@/lib/trpc", () => ({
|
||||||
|
trpc: {
|
||||||
|
getFileDiff: {
|
||||||
|
useQuery: (
|
||||||
|
input: unknown,
|
||||||
|
opts: { enabled: boolean; staleTime?: number },
|
||||||
|
) => mockGetFileDiff(input, opts),
|
||||||
|
},
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock("./parse-diff", () => ({
|
||||||
|
parseUnifiedDiff: (rawDiff: string) => mockParseUnifiedDiff(rawDiff),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// ── Helpers ───────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
function makeFile(overrides: Partial<FileDiff> = {}): FileDiff {
|
||||||
|
return {
|
||||||
|
oldPath: "src/foo.ts",
|
||||||
|
newPath: "src/foo.ts",
|
||||||
|
status: "modified",
|
||||||
|
additions: 10,
|
||||||
|
deletions: 5,
|
||||||
|
...overrides,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const defaultProps = {
|
||||||
|
phaseId: "phase-1",
|
||||||
|
commitMode: false,
|
||||||
|
commentsByLine: new Map(),
|
||||||
|
onAddComment: vi.fn(),
|
||||||
|
onResolveComment: vi.fn(),
|
||||||
|
onUnresolveComment: vi.fn(),
|
||||||
|
};
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: undefined,
|
||||||
|
isLoading: false,
|
||||||
|
isError: false,
|
||||||
|
refetch: vi.fn(),
|
||||||
|
});
|
||||||
|
// Default: return empty parse result
|
||||||
|
mockParseUnifiedDiff.mockReturnValue([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
// ── Tests ─────────────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
describe("FileCard", () => {
|
||||||
|
it("starts collapsed and does not enable getFileDiff query", () => {
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
|
||||||
|
// Query must be called with enabled: false while card is collapsed
|
||||||
|
expect(mockGetFileDiff).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
filePath: encodeURIComponent("src/foo.ts"),
|
||||||
|
}),
|
||||||
|
expect.objectContaining({ enabled: false }),
|
||||||
|
);
|
||||||
|
|
||||||
|
// No hunk rows rendered in the collapsed state
|
||||||
|
expect(screen.queryByTestId("hunk-row")).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("enables query and shows loading spinner when expanded", () => {
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: undefined,
|
||||||
|
isLoading: true,
|
||||||
|
isError: false,
|
||||||
|
refetch: vi.fn(),
|
||||||
|
});
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
fireEvent.click(screen.getByRole("button"));
|
||||||
|
|
||||||
|
// After expanding, query should be called with enabled: true
|
||||||
|
expect(mockGetFileDiff).toHaveBeenLastCalledWith(
|
||||||
|
expect.anything(),
|
||||||
|
expect.objectContaining({ enabled: true }),
|
||||||
|
);
|
||||||
|
|
||||||
|
// Loading spinner should be visible
|
||||||
|
expect(screen.getByText(/Loading diff/i)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("renders HunkRows when query succeeds", async () => {
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: {
|
||||||
|
binary: false,
|
||||||
|
rawDiff:
|
||||||
|
"diff --git a/src/foo.ts b/src/foo.ts\n@@ -1,3 +1,3 @@\n context\n",
|
||||||
|
},
|
||||||
|
isLoading: false,
|
||||||
|
isError: false,
|
||||||
|
refetch: vi.fn(),
|
||||||
|
});
|
||||||
|
|
||||||
|
mockParseUnifiedDiff.mockReturnValue([
|
||||||
|
{
|
||||||
|
oldPath: "src/foo.ts",
|
||||||
|
newPath: "src/foo.ts",
|
||||||
|
status: "modified",
|
||||||
|
additions: 0,
|
||||||
|
deletions: 0,
|
||||||
|
hunks: [
|
||||||
|
{
|
||||||
|
header: "@@ -1,3 +1,3 @@",
|
||||||
|
oldStart: 1,
|
||||||
|
oldCount: 3,
|
||||||
|
newStart: 1,
|
||||||
|
newCount: 3,
|
||||||
|
lines: [],
|
||||||
|
},
|
||||||
|
],
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
fireEvent.click(screen.getByRole("button"));
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.getByTestId("hunk-row")).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("shows error state with Retry button; clicking retry calls refetch", () => {
|
||||||
|
const refetch = vi.fn();
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: undefined,
|
||||||
|
isLoading: false,
|
||||||
|
isError: true,
|
||||||
|
refetch,
|
||||||
|
});
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
fireEvent.click(screen.getByRole("button"));
|
||||||
|
|
||||||
|
expect(screen.getByText(/Failed to load diff/i)).toBeInTheDocument();
|
||||||
|
const retryBtn = screen.getByRole("button", { name: /retry/i });
|
||||||
|
fireEvent.click(retryBtn);
|
||||||
|
expect(refetch).toHaveBeenCalledOnce();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("shows binary message on expand and does not enable getFileDiff query", () => {
|
||||||
|
render(<FileCard file={makeFile({ status: "binary" })} {...defaultProps} />);
|
||||||
|
fireEvent.click(screen.getByRole("button"));
|
||||||
|
|
||||||
|
expect(screen.getByText(/Binary file/i)).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Query must never be enabled for binary files
|
||||||
|
expect(mockGetFileDiff).toHaveBeenCalledWith(
|
||||||
|
expect.anything(),
|
||||||
|
expect.objectContaining({ enabled: false }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("shows No content changes when parsed hunks array is empty", async () => {
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: {
|
||||||
|
binary: false,
|
||||||
|
rawDiff: "diff --git a/src/foo.ts b/src/foo.ts\nsome content\n",
|
||||||
|
},
|
||||||
|
isLoading: false,
|
||||||
|
isError: false,
|
||||||
|
refetch: vi.fn(),
|
||||||
|
});
|
||||||
|
|
||||||
|
mockParseUnifiedDiff.mockReturnValue([
|
||||||
|
{
|
||||||
|
oldPath: "src/foo.ts",
|
||||||
|
newPath: "src/foo.ts",
|
||||||
|
status: "modified",
|
||||||
|
additions: 0,
|
||||||
|
deletions: 0,
|
||||||
|
hunks: [],
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
fireEvent.click(screen.getByRole("button"));
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(screen.getByText(/No content changes/i)).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("renders pre-parsed hunks from detail prop without fetching", () => {
|
||||||
|
const detail: FileDiffDetail = {
|
||||||
|
oldPath: "src/foo.ts",
|
||||||
|
newPath: "src/foo.ts",
|
||||||
|
status: "modified",
|
||||||
|
additions: 5,
|
||||||
|
deletions: 2,
|
||||||
|
hunks: [
|
||||||
|
{
|
||||||
|
header: "@@ -1 +1 @@",
|
||||||
|
oldStart: 1,
|
||||||
|
oldCount: 1,
|
||||||
|
newStart: 1,
|
||||||
|
newCount: 1,
|
||||||
|
lines: [],
|
||||||
|
},
|
||||||
|
],
|
||||||
|
};
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} detail={detail} {...defaultProps} />);
|
||||||
|
|
||||||
|
// Should start expanded because detail prop is provided
|
||||||
|
expect(screen.getByTestId("hunk-row")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Query must not be enabled when detail prop is present
|
||||||
|
expect(mockGetFileDiff).toHaveBeenCalledWith(
|
||||||
|
expect.anything(),
|
||||||
|
expect.objectContaining({ enabled: false }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not refetch when collapsing and re-expanding", () => {
|
||||||
|
// Simulate data already available (as if previously fetched and cached)
|
||||||
|
mockGetFileDiff.mockReturnValue({
|
||||||
|
data: { binary: false, rawDiff: "" },
|
||||||
|
isLoading: false,
|
||||||
|
isError: false,
|
||||||
|
refetch: vi.fn(),
|
||||||
|
});
|
||||||
|
|
||||||
|
render(<FileCard file={makeFile()} {...defaultProps} />);
|
||||||
|
const headerBtn = screen.getByRole("button");
|
||||||
|
|
||||||
|
// Expand: query enabled, data shown immediately (no loading)
|
||||||
|
fireEvent.click(headerBtn);
|
||||||
|
expect(screen.queryByText(/Loading diff/i)).toBeNull();
|
||||||
|
|
||||||
|
// Collapse
|
||||||
|
fireEvent.click(headerBtn);
|
||||||
|
|
||||||
|
// Re-expand: should not enter loading state (data still available)
|
||||||
|
fireEvent.click(headerBtn);
|
||||||
|
expect(screen.queryByText(/Loading diff/i)).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -6,16 +6,16 @@ import {
|
|||||||
Minus,
|
Minus,
|
||||||
CheckCircle2,
|
CheckCircle2,
|
||||||
Circle,
|
Circle,
|
||||||
|
Loader2,
|
||||||
} from "lucide-react";
|
} from "lucide-react";
|
||||||
import { Badge } from "@/components/ui/badge";
|
import { Badge } from "@/components/ui/badge";
|
||||||
import type { FileDiffDetail, DiffLine, ReviewComment } from "./types";
|
import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types";
|
||||||
import { HunkRows } from "./HunkRows";
|
import { HunkRows } from "./HunkRows";
|
||||||
import { useHighlightedFile } from "./use-syntax-highlight";
|
import { useHighlightedFile } from "./use-syntax-highlight";
|
||||||
|
import { parseUnifiedDiff } from "./parse-diff";
|
||||||
|
import { trpc } from "@/lib/trpc";
|
||||||
|
|
||||||
const changeTypeBadge: Record<
|
const statusBadge: Record<FileDiff['status'], { label: string; classes: string } | null> = {
|
||||||
FileDiffDetail['status'],
|
|
||||||
{ label: string; classes: string } | null
|
|
||||||
> = {
|
|
||||||
added: {
|
added: {
|
||||||
label: "NEW",
|
label: "NEW",
|
||||||
classes:
|
classes:
|
||||||
@@ -32,10 +32,13 @@ const changeTypeBadge: Record<
|
|||||||
"bg-status-active-bg text-status-active-fg border-status-active-border",
|
"bg-status-active-bg text-status-active-fg border-status-active-border",
|
||||||
},
|
},
|
||||||
modified: null,
|
modified: null,
|
||||||
binary: null,
|
binary: {
|
||||||
|
label: "BINARY",
|
||||||
|
classes: "bg-muted text-muted-foreground border-border",
|
||||||
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
const leftBorderClass: Record<FileDiffDetail['status'], string> = {
|
const leftBorderClass: Record<FileDiff['status'], string> = {
|
||||||
added: "border-l-2 border-l-status-success-fg",
|
added: "border-l-2 border-l-status-success-fg",
|
||||||
deleted: "border-l-2 border-l-status-error-fg",
|
deleted: "border-l-2 border-l-status-error-fg",
|
||||||
renamed: "border-l-2 border-l-status-active-fg",
|
renamed: "border-l-2 border-l-status-active-fg",
|
||||||
@@ -44,8 +47,12 @@ const leftBorderClass: Record<FileDiffDetail['status'], string> = {
|
|||||||
};
|
};
|
||||||
|
|
||||||
interface FileCardProps {
|
interface FileCardProps {
|
||||||
file: FileDiffDetail;
|
file: FileDiff;
|
||||||
|
detail?: FileDiffDetail;
|
||||||
|
phaseId: string;
|
||||||
|
commitMode: boolean;
|
||||||
commentsByLine: Map<string, ReviewComment[]>;
|
commentsByLine: Map<string, ReviewComment[]>;
|
||||||
|
isExpandedOverride?: boolean;
|
||||||
onAddComment: (
|
onAddComment: (
|
||||||
filePath: string,
|
filePath: string,
|
||||||
lineNumber: number,
|
lineNumber: number,
|
||||||
@@ -62,7 +69,11 @@ interface FileCardProps {
|
|||||||
|
|
||||||
export function FileCard({
|
export function FileCard({
|
||||||
file,
|
file,
|
||||||
|
detail,
|
||||||
|
phaseId,
|
||||||
|
commitMode,
|
||||||
commentsByLine,
|
commentsByLine,
|
||||||
|
isExpandedOverride,
|
||||||
onAddComment,
|
onAddComment,
|
||||||
onResolveComment,
|
onResolveComment,
|
||||||
onUnresolveComment,
|
onUnresolveComment,
|
||||||
@@ -71,35 +82,65 @@ export function FileCard({
|
|||||||
isViewed = false,
|
isViewed = false,
|
||||||
onToggleViewed = () => {},
|
onToggleViewed = () => {},
|
||||||
}: FileCardProps) {
|
}: FileCardProps) {
|
||||||
const [expanded, setExpanded] = useState(true);
|
// Uncontrolled expand for normal file clicks.
|
||||||
|
// Start expanded if detail prop is provided (commit mode).
|
||||||
|
const [isExpandedLocal, setIsExpandedLocal] = useState(() => !!detail);
|
||||||
|
|
||||||
const commentCount = useMemo(
|
// Merge with override from DiffViewer expandAll
|
||||||
() =>
|
const isExpanded = isExpandedOverride ?? isExpandedLocal;
|
||||||
Array.from(commentsByLine.values()).reduce(
|
|
||||||
(sum, arr) => sum + arr.length,
|
const fileDiffQuery = trpc.getFileDiff.useQuery(
|
||||||
0,
|
{ phaseId, filePath: encodeURIComponent(file.newPath) },
|
||||||
),
|
{
|
||||||
[commentsByLine],
|
enabled: isExpanded && !commitMode && file.status !== 'binary' && !detail,
|
||||||
|
staleTime: Infinity,
|
||||||
|
},
|
||||||
);
|
);
|
||||||
|
|
||||||
const badge = changeTypeBadge[file.status];
|
// Compute hunks from query data (phase mode)
|
||||||
|
const parsedHunks = useMemo(() => {
|
||||||
|
if (!fileDiffQuery.data?.rawDiff) return null;
|
||||||
|
const parsed = parseUnifiedDiff(fileDiffQuery.data.rawDiff);
|
||||||
|
return parsed[0] ?? null;
|
||||||
|
}, [fileDiffQuery.data]);
|
||||||
|
|
||||||
|
// Collect all lines for syntax highlighting
|
||||||
|
const allLines = useMemo(() => {
|
||||||
|
if (detail) return detail.hunks.flatMap((h) => h.lines);
|
||||||
|
if (parsedHunks) return parsedHunks.hunks.flatMap((h) => h.lines);
|
||||||
|
return [];
|
||||||
|
}, [detail, parsedHunks]);
|
||||||
|
|
||||||
// Flatten all hunk lines for syntax highlighting
|
|
||||||
const allLines = useMemo(
|
|
||||||
() => file.hunks.flatMap((h) => h.lines),
|
|
||||||
[file.hunks],
|
|
||||||
);
|
|
||||||
const tokenMap = useHighlightedFile(file.newPath, allLines);
|
const tokenMap = useHighlightedFile(file.newPath, allLines);
|
||||||
|
|
||||||
|
const commentCount = useMemo(() => {
|
||||||
|
let count = 0;
|
||||||
|
for (const [key, arr] of commentsByLine) {
|
||||||
|
if (key.startsWith(`${file.newPath}:`)) count += arr.length;
|
||||||
|
}
|
||||||
|
return count;
|
||||||
|
}, [commentsByLine, file.newPath]);
|
||||||
|
|
||||||
|
const badge = statusBadge[file.status];
|
||||||
|
|
||||||
|
const handlers = {
|
||||||
|
onAddComment,
|
||||||
|
onResolveComment,
|
||||||
|
onUnresolveComment,
|
||||||
|
onReplyComment,
|
||||||
|
onEditComment,
|
||||||
|
tokenMap,
|
||||||
|
};
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div className="rounded-lg border border-border overflow-clip">
|
<div className="rounded-lg border border-border overflow-clip">
|
||||||
{/* File header — sticky so it stays visible when scrolling */}
|
{/* File header */}
|
||||||
<button
|
<button
|
||||||
className={`sticky z-10 flex w-full items-center gap-2 px-3 py-2 bg-muted hover:bg-muted/90 text-left text-sm font-mono transition-colors ${leftBorderClass[file.status]}`}
|
className={`sticky z-10 flex w-full items-center gap-2 px-3 py-2 bg-muted hover:bg-muted/90 text-left text-sm font-mono transition-colors ${leftBorderClass[file.status]}`}
|
||||||
style={{ top: 'var(--review-header-h, 0px)' }}
|
style={{ top: 'var(--review-header-h, 0px)' }}
|
||||||
onClick={() => setExpanded(!expanded)}
|
onClick={() => setIsExpandedLocal(!isExpandedLocal)}
|
||||||
>
|
>
|
||||||
{expanded ? (
|
{isExpanded ? (
|
||||||
<ChevronDown className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
<ChevronDown className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
||||||
) : (
|
) : (
|
||||||
<ChevronRight className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
<ChevronRight className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
||||||
@@ -160,26 +201,63 @@ export function FileCard({
|
|||||||
</button>
|
</button>
|
||||||
|
|
||||||
{/* Diff content */}
|
{/* Diff content */}
|
||||||
{expanded && (
|
{isExpanded && (
|
||||||
<div className="overflow-x-auto">
|
<div className="overflow-x-auto">
|
||||||
|
{detail ? (
|
||||||
|
// Commit mode: pre-parsed hunks from detail prop
|
||||||
|
detail.hunks.length === 0 ? (
|
||||||
|
<div className="px-4 py-3 text-xs text-muted-foreground">No content changes</div>
|
||||||
|
) : (
|
||||||
<table className="w-full text-xs font-mono border-collapse">
|
<table className="w-full text-xs font-mono border-collapse">
|
||||||
<tbody>
|
<tbody>
|
||||||
{file.hunks.map((hunk, hi) => (
|
{detail.hunks.map((hunk, hi) => (
|
||||||
<HunkRows
|
<HunkRows
|
||||||
key={hi}
|
key={hi}
|
||||||
hunk={hunk}
|
hunk={hunk}
|
||||||
filePath={file.newPath}
|
filePath={file.newPath}
|
||||||
commentsByLine={commentsByLine}
|
commentsByLine={commentsByLine}
|
||||||
onAddComment={onAddComment}
|
{...handlers}
|
||||||
onResolveComment={onResolveComment}
|
|
||||||
onUnresolveComment={onUnresolveComment}
|
|
||||||
onReplyComment={onReplyComment}
|
|
||||||
onEditComment={onEditComment}
|
|
||||||
tokenMap={tokenMap}
|
|
||||||
/>
|
/>
|
||||||
))}
|
))}
|
||||||
</tbody>
|
</tbody>
|
||||||
</table>
|
</table>
|
||||||
|
)
|
||||||
|
) : file.status === 'binary' ? (
|
||||||
|
<div className="px-4 py-3 text-xs text-muted-foreground">Binary file — diff not shown</div>
|
||||||
|
) : fileDiffQuery.isLoading ? (
|
||||||
|
<div className="flex items-center gap-2 px-4 py-3 text-xs text-muted-foreground">
|
||||||
|
<Loader2 className="h-3.5 w-3.5 animate-spin" />
|
||||||
|
Loading diff…
|
||||||
|
</div>
|
||||||
|
) : fileDiffQuery.isError ? (
|
||||||
|
<div className="flex items-center gap-2 px-4 py-3 text-xs text-destructive">
|
||||||
|
Failed to load diff.
|
||||||
|
<button
|
||||||
|
className="underline hover:no-underline"
|
||||||
|
onClick={() => fileDiffQuery.refetch()}
|
||||||
|
>
|
||||||
|
Retry
|
||||||
|
</button>
|
||||||
|
</div>
|
||||||
|
) : fileDiffQuery.data ? (
|
||||||
|
!parsedHunks || parsedHunks.hunks.length === 0 ? (
|
||||||
|
<div className="px-4 py-3 text-xs text-muted-foreground">No content changes</div>
|
||||||
|
) : (
|
||||||
|
<table className="w-full text-xs font-mono border-collapse">
|
||||||
|
<tbody>
|
||||||
|
{parsedHunks.hunks.map((hunk, hi) => (
|
||||||
|
<HunkRows
|
||||||
|
key={hi}
|
||||||
|
hunk={hunk}
|
||||||
|
filePath={file.newPath}
|
||||||
|
commentsByLine={commentsByLine}
|
||||||
|
{...handlers}
|
||||||
|
/>
|
||||||
|
))}
|
||||||
|
</tbody>
|
||||||
|
</table>
|
||||||
|
)
|
||||||
|
) : null}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -406,6 +406,8 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
|||||||
) : (
|
) : (
|
||||||
<DiffViewer
|
<DiffViewer
|
||||||
files={files}
|
files={files}
|
||||||
|
phaseId={activePhaseId!}
|
||||||
|
commitMode={!!selectedCommit}
|
||||||
commentsByLine={commentsByLine}
|
commentsByLine={commentsByLine}
|
||||||
onAddComment={handleAddComment}
|
onAddComment={handleAddComment}
|
||||||
onResolveComment={handleResolveComment}
|
onResolveComment={handleResolveComment}
|
||||||
|
|||||||
Reference in New Issue
Block a user