Merge branch 'cw/review-tab-performance-task-SP2g6-ypklu72GHVUBhty' into cw-merge-1772825187869
This commit is contained in:
Lukas May
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);
|
||||
});
|
||||
});
|
||||
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();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user