Merge branch 'cw/review-tab-performance-phase-frontend-viewport-virtualization-for-diffviewer-per-file-lazy-loading' into cw/review-tab-performance
This commit is contained in:
Lukas May
@@ -11,7 +11,7 @@ interface ConflictResolutionPanelProps {
|
||||
}
|
||||
|
||||
export function ConflictResolutionPanel({ initiativeId, conflicts, onResolved }: ConflictResolutionPanelProps) {
|
||||
const { state, agent, questions, spawn, resume, stop, dismiss } = useConflictAgent(initiativeId);
|
||||
const { state, agent: _agent, questions, spawn, resume, stop, dismiss } = useConflictAgent(initiativeId);
|
||||
const [showManual, setShowManual] = useState(false);
|
||||
const prevStateRef = useRef<string | null>(null);
|
||||
|
||||
|
||||
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 { FileDiff, 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 { trpc } from "@/lib/trpc";
|
||||
|
||||
function getFileCommentMap(
|
||||
commentsByLine: Map<string, ReviewComment[]>,
|
||||
@@ -13,7 +16,9 @@ function getFileCommentMap(
|
||||
}
|
||||
|
||||
interface DiffViewerProps {
|
||||
files: FileDiff[];
|
||||
files: (FileDiff | FileDiffDetail)[];
|
||||
phaseId: string;
|
||||
commitMode: boolean;
|
||||
commentsByLine: Map<string, ReviewComment[]>;
|
||||
onAddComment: (
|
||||
filePath: string,
|
||||
@@ -28,10 +33,13 @@ interface DiffViewerProps {
|
||||
viewedFiles?: Set<string>;
|
||||
onToggleViewed?: (filePath: string) => void;
|
||||
onRegisterRef?: (filePath: string, el: HTMLDivElement | null) => void;
|
||||
expandAll?: boolean;
|
||||
}
|
||||
|
||||
export function DiffViewer({
|
||||
files,
|
||||
phaseId,
|
||||
commitMode,
|
||||
commentsByLine,
|
||||
onAddComment,
|
||||
onResolveComment,
|
||||
@@ -41,24 +49,156 @@ export function DiffViewer({
|
||||
viewedFiles,
|
||||
onToggleViewed,
|
||||
onRegisterRef,
|
||||
expandAll,
|
||||
}: 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;
|
||||
void queryClient; // imported for type alignment; actual prefetch goes through trpc utils
|
||||
|
||||
const isSingleFile = files.length === 1;
|
||||
|
||||
return (
|
||||
<div className="space-y-4">
|
||||
{files.map((file) => (
|
||||
<div key={file.newPath} ref={(el) => onRegisterRef?.(file.newPath, el)}>
|
||||
<FileCard
|
||||
file={file}
|
||||
commentsByLine={getFileCommentMap(commentsByLine, file.newPath)}
|
||||
onAddComment={onAddComment}
|
||||
onResolveComment={onResolveComment}
|
||||
onUnresolveComment={onUnresolveComment}
|
||||
onReplyComment={onReplyComment}
|
||||
onEditComment={onEditComment}
|
||||
isViewed={viewedFiles?.has(file.newPath) ?? false}
|
||||
onToggleViewed={() => onToggleViewed?.(file.newPath)}
|
||||
/>
|
||||
</div>
|
||||
))}
|
||||
{files.map((file) => {
|
||||
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
|
||||
file={file as FileDiff}
|
||||
detail={'hunks' in file ? (file as FileDiffDetail) : undefined}
|
||||
phaseId={phaseId}
|
||||
commitMode={commitMode}
|
||||
commentsByLine={getFileCommentMap(commentsByLine, file.newPath)}
|
||||
isExpandedOverride={isExpandedOverride}
|
||||
onAddComment={onAddComment}
|
||||
onResolveComment={onResolveComment}
|
||||
onUnresolveComment={onUnresolveComment}
|
||||
onReplyComment={onReplyComment}
|
||||
onEditComment={onEditComment}
|
||||
isViewed={viewedFiles?.has(file.newPath) ?? false}
|
||||
onToggleViewed={() => onToggleViewed?.(file.newPath)}
|
||||
/>
|
||||
) : (
|
||||
<div style={{ height: '48px' }} aria-hidden />
|
||||
)}
|
||||
</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,
|
||||
CheckCircle2,
|
||||
Circle,
|
||||
Loader2,
|
||||
} from "lucide-react";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import type { FileDiff, FileChangeType, DiffLine, ReviewComment } from "./types";
|
||||
import type { FileDiff, FileDiffDetail, DiffLine, ReviewComment } from "./types";
|
||||
import { HunkRows } from "./HunkRows";
|
||||
import { useHighlightedFile } from "./use-syntax-highlight";
|
||||
import { parseUnifiedDiff } from "./parse-diff";
|
||||
import { trpc } from "@/lib/trpc";
|
||||
|
||||
const changeTypeBadge: Record<
|
||||
FileChangeType,
|
||||
{ label: string; classes: string } | null
|
||||
> = {
|
||||
const statusBadge: Record<FileDiff['status'], { label: string; classes: string } | null> = {
|
||||
added: {
|
||||
label: "NEW",
|
||||
classes:
|
||||
@@ -32,18 +32,27 @@ const changeTypeBadge: Record<
|
||||
"bg-status-active-bg text-status-active-fg border-status-active-border",
|
||||
},
|
||||
modified: null,
|
||||
binary: {
|
||||
label: "BINARY",
|
||||
classes: "bg-muted text-muted-foreground border-border",
|
||||
},
|
||||
};
|
||||
|
||||
const leftBorderClass: Record<FileChangeType, string> = {
|
||||
const leftBorderClass: Record<FileDiff['status'], string> = {
|
||||
added: "border-l-2 border-l-status-success-fg",
|
||||
deleted: "border-l-2 border-l-status-error-fg",
|
||||
renamed: "border-l-2 border-l-status-active-fg",
|
||||
modified: "border-l-2 border-l-primary/40",
|
||||
binary: "border-l-2 border-l-primary/40",
|
||||
};
|
||||
|
||||
interface FileCardProps {
|
||||
file: FileDiff;
|
||||
detail?: FileDiffDetail;
|
||||
phaseId: string;
|
||||
commitMode: boolean;
|
||||
commentsByLine: Map<string, ReviewComment[]>;
|
||||
isExpandedOverride?: boolean;
|
||||
onAddComment: (
|
||||
filePath: string,
|
||||
lineNumber: number,
|
||||
@@ -60,7 +69,11 @@ interface FileCardProps {
|
||||
|
||||
export function FileCard({
|
||||
file,
|
||||
detail,
|
||||
phaseId,
|
||||
commitMode,
|
||||
commentsByLine,
|
||||
isExpandedOverride,
|
||||
onAddComment,
|
||||
onResolveComment,
|
||||
onUnresolveComment,
|
||||
@@ -69,35 +82,65 @@ export function FileCard({
|
||||
isViewed = false,
|
||||
onToggleViewed = () => {},
|
||||
}: 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(
|
||||
() =>
|
||||
Array.from(commentsByLine.values()).reduce(
|
||||
(sum, arr) => sum + arr.length,
|
||||
0,
|
||||
),
|
||||
[commentsByLine],
|
||||
// Merge with override from DiffViewer expandAll
|
||||
const isExpanded = isExpandedOverride ?? isExpandedLocal;
|
||||
|
||||
const fileDiffQuery = trpc.getFileDiff.useQuery(
|
||||
{ phaseId, filePath: encodeURIComponent(file.newPath) },
|
||||
{
|
||||
enabled: isExpanded && !commitMode && file.status !== 'binary' && !detail,
|
||||
staleTime: Infinity,
|
||||
},
|
||||
);
|
||||
|
||||
const badge = changeTypeBadge[file.changeType];
|
||||
// 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 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 (
|
||||
<div className="rounded-lg border border-border overflow-clip">
|
||||
{/* File header — sticky so it stays visible when scrolling */}
|
||||
{/* File header */}
|
||||
<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.changeType]}`}
|
||||
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)' }}
|
||||
onClick={() => setExpanded(!expanded)}
|
||||
onClick={() => setIsExpandedLocal(!isExpandedLocal)}
|
||||
>
|
||||
{expanded ? (
|
||||
{isExpanded ? (
|
||||
<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" />
|
||||
@@ -158,26 +201,63 @@ export function FileCard({
|
||||
</button>
|
||||
|
||||
{/* Diff content */}
|
||||
{expanded && (
|
||||
{isExpanded && (
|
||||
<div className="overflow-x-auto">
|
||||
<table className="w-full text-xs font-mono border-collapse">
|
||||
<tbody>
|
||||
{file.hunks.map((hunk, hi) => (
|
||||
<HunkRows
|
||||
key={hi}
|
||||
hunk={hunk}
|
||||
filePath={file.newPath}
|
||||
commentsByLine={commentsByLine}
|
||||
onAddComment={onAddComment}
|
||||
onResolveComment={onResolveComment}
|
||||
onUnresolveComment={onUnresolveComment}
|
||||
onReplyComment={onReplyComment}
|
||||
onEditComment={onEditComment}
|
||||
tokenMap={tokenMap}
|
||||
/>
|
||||
))}
|
||||
</tbody>
|
||||
</table>
|
||||
{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">
|
||||
<tbody>
|
||||
{detail.hunks.map((hunk, hi) => (
|
||||
<HunkRows
|
||||
key={hi}
|
||||
hunk={hunk}
|
||||
filePath={file.newPath}
|
||||
commentsByLine={commentsByLine}
|
||||
{...handlers}
|
||||
/>
|
||||
))}
|
||||
</tbody>
|
||||
</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>
|
||||
|
||||
@@ -308,7 +308,7 @@ export function InitiativeReview({ initiativeId, onCompleted }: InitiativeReview
|
||||
) : (
|
||||
<DiffViewer
|
||||
files={files}
|
||||
comments={[]}
|
||||
commentsByLine={new Map()}
|
||||
onAddComment={() => {}}
|
||||
onResolveComment={() => {}}
|
||||
onUnresolveComment={() => {}}
|
||||
|
||||
@@ -42,6 +42,9 @@ interface ReviewHeaderProps {
|
||||
preview: PreviewState | null;
|
||||
viewedCount?: number;
|
||||
totalCount?: number;
|
||||
totalAdditions?: number;
|
||||
totalDeletions?: number;
|
||||
onExpandAll?: () => void;
|
||||
}
|
||||
|
||||
export function ReviewHeader({
|
||||
@@ -62,9 +65,12 @@ export function ReviewHeader({
|
||||
preview,
|
||||
viewedCount,
|
||||
totalCount,
|
||||
totalAdditions: totalAdditionsProp,
|
||||
totalDeletions: totalDeletionsProp,
|
||||
onExpandAll,
|
||||
}: ReviewHeaderProps) {
|
||||
const totalAdditions = files.reduce((s, f) => s + f.additions, 0);
|
||||
const totalDeletions = files.reduce((s, f) => s + f.deletions, 0);
|
||||
const totalAdditions = totalAdditionsProp ?? files.reduce((s, f) => s + f.additions, 0);
|
||||
const totalDeletions = totalDeletionsProp ?? files.reduce((s, f) => s + f.deletions, 0);
|
||||
const [showConfirmation, setShowConfirmation] = useState(false);
|
||||
const [showRequestConfirm, setShowRequestConfirm] = useState(false);
|
||||
const confirmRef = useRef<HTMLDivElement>(null);
|
||||
@@ -186,6 +192,16 @@ export function ReviewHeader({
|
||||
|
||||
{/* Right: preview + actions */}
|
||||
<div className="flex items-center gap-3 shrink-0">
|
||||
{onExpandAll && (
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="sm"
|
||||
onClick={onExpandAll}
|
||||
className="h-7 text-xs px-2 text-muted-foreground"
|
||||
>
|
||||
Expand all
|
||||
</Button>
|
||||
)}
|
||||
{/* Preview controls */}
|
||||
{preview && <PreviewControls preview={preview} />}
|
||||
|
||||
|
||||
226
apps/web/src/components/review/ReviewTab.test.tsx
Normal file
226
apps/web/src/components/review/ReviewTab.test.tsx
Normal file
@@ -0,0 +1,226 @@
|
||||
// @vitest-environment happy-dom
|
||||
import "@testing-library/jest-dom/vitest";
|
||||
import { render, screen, act } from "@testing-library/react";
|
||||
import { vi, describe, it, expect, beforeEach } from "vitest";
|
||||
|
||||
// ── Capture props passed to stubs ─────────────────────────────────────────────
|
||||
// These are module-level so the vi.mock factories (which are hoisted) can close over them.
|
||||
let diffViewerProps: Record<string, unknown> = {};
|
||||
let reviewSidebarProps: Record<string, unknown> = {};
|
||||
|
||||
vi.mock("./DiffViewer", () => ({
|
||||
DiffViewer: (props: Record<string, unknown>) => {
|
||||
diffViewerProps = props;
|
||||
return <div data-testid="diff-viewer" />;
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("./ReviewSidebar", () => ({
|
||||
ReviewSidebar: (props: Record<string, unknown>) => {
|
||||
reviewSidebarProps = props;
|
||||
return <div data-testid="review-sidebar" />;
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("./ReviewHeader", () => ({
|
||||
ReviewHeader: (props: Record<string, unknown>) => (
|
||||
<div data-testid="review-header">
|
||||
{props.onExpandAll && (
|
||||
<button
|
||||
data-testid="expand-all-btn"
|
||||
onClick={props.onExpandAll as () => void}
|
||||
>
|
||||
Expand all
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
),
|
||||
}));
|
||||
|
||||
vi.mock("./InitiativeReview", () => ({
|
||||
InitiativeReview: () => <div data-testid="initiative-review" />,
|
||||
}));
|
||||
|
||||
vi.mock("./comment-index", () => ({
|
||||
buildCommentIndex: vi.fn(() => new Map()),
|
||||
}));
|
||||
|
||||
vi.mock("sonner", () => ({
|
||||
toast: { success: vi.fn(), error: vi.fn() },
|
||||
}));
|
||||
|
||||
// ── parseUnifiedDiff spy ───────────────────────────────────────────────────────
|
||||
const mockParseUnifiedDiff = vi.fn((_raw: string) => [
|
||||
{
|
||||
oldPath: "a.ts",
|
||||
newPath: "a.ts",
|
||||
status: "modified" as const,
|
||||
additions: 3,
|
||||
deletions: 1,
|
||||
hunks: [],
|
||||
},
|
||||
]);
|
||||
|
||||
vi.mock("./parse-diff", () => ({
|
||||
get parseUnifiedDiff() {
|
||||
return mockParseUnifiedDiff;
|
||||
},
|
||||
}));
|
||||
|
||||
// ── tRPC mock factory ─────────────────────────────────────────────────────────
|
||||
|
||||
const noopMutation = () => ({
|
||||
mutate: vi.fn(),
|
||||
isPending: false,
|
||||
});
|
||||
|
||||
const noopQuery = (data: unknown = undefined) => ({
|
||||
data,
|
||||
isLoading: false,
|
||||
isError: false,
|
||||
refetch: vi.fn(),
|
||||
});
|
||||
|
||||
const mockUtils = {
|
||||
listReviewComments: { invalidate: vi.fn() },
|
||||
};
|
||||
|
||||
// Server format (FileStatEntry): uses `path` not `newPath`
|
||||
const PHASE_FILES = [
|
||||
{
|
||||
path: "a.ts",
|
||||
status: "modified" as const,
|
||||
additions: 5,
|
||||
deletions: 2,
|
||||
},
|
||||
];
|
||||
|
||||
// trpcMock is a let so tests can override it. The getter in the mock reads the current value.
|
||||
let trpcMock = buildTrpcMock();
|
||||
|
||||
function buildTrpcMock(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
getInitiative: { useQuery: vi.fn(() => noopQuery({ status: "in_progress" })) },
|
||||
listPhases: {
|
||||
useQuery: vi.fn(() =>
|
||||
noopQuery([{ id: "phase-1", name: "Phase 1", status: "pending_review" }])
|
||||
),
|
||||
},
|
||||
getInitiativeProjects: { useQuery: vi.fn(() => noopQuery([{ id: "proj-1" }])) },
|
||||
getPhaseReviewDiff: {
|
||||
useQuery: vi.fn(() =>
|
||||
noopQuery({
|
||||
phaseName: "Phase 1",
|
||||
sourceBranch: "cw/phase-1",
|
||||
targetBranch: "main",
|
||||
files: PHASE_FILES,
|
||||
totalAdditions: 5,
|
||||
totalDeletions: 2,
|
||||
})
|
||||
),
|
||||
},
|
||||
getPhaseReviewCommits: {
|
||||
useQuery: vi.fn(() =>
|
||||
noopQuery({ commits: [], sourceBranch: "cw/phase-1", targetBranch: "main" })
|
||||
),
|
||||
},
|
||||
getCommitDiff: {
|
||||
useQuery: vi.fn(() => noopQuery({ rawDiff: "" })),
|
||||
},
|
||||
listPreviews: { useQuery: vi.fn(() => noopQuery([])) },
|
||||
getPreviewStatus: { useQuery: vi.fn(() => noopQuery(null)) },
|
||||
listReviewComments: { useQuery: vi.fn(() => noopQuery([])) },
|
||||
startPreview: { useMutation: vi.fn(() => noopMutation()) },
|
||||
stopPreview: { useMutation: vi.fn(() => noopMutation()) },
|
||||
createReviewComment: { useMutation: vi.fn(() => noopMutation()) },
|
||||
resolveReviewComment: { useMutation: vi.fn(() => noopMutation()) },
|
||||
unresolveReviewComment: { useMutation: vi.fn(() => noopMutation()) },
|
||||
replyToReviewComment: { useMutation: vi.fn(() => noopMutation()) },
|
||||
updateReviewComment: { useMutation: vi.fn(() => noopMutation()) },
|
||||
approvePhaseReview: { useMutation: vi.fn(() => noopMutation()) },
|
||||
requestPhaseChanges: { useMutation: vi.fn(() => noopMutation()) },
|
||||
useUtils: vi.fn(() => mockUtils),
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
vi.mock("@/lib/trpc", () => ({
|
||||
get trpc() {
|
||||
return trpcMock;
|
||||
},
|
||||
}));
|
||||
|
||||
// ── Import component after mocks ──────────────────────────────────────────────
|
||||
import { ReviewTab } from "./ReviewTab";
|
||||
|
||||
// ── Tests ─────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe("ReviewTab", () => {
|
||||
beforeEach(() => {
|
||||
diffViewerProps = {};
|
||||
reviewSidebarProps = {};
|
||||
mockParseUnifiedDiff.mockClear();
|
||||
trpcMock = buildTrpcMock();
|
||||
});
|
||||
|
||||
it("1. phase diff loads metadata: DiffViewer receives files array and commitMode=false", () => {
|
||||
render(<ReviewTab initiativeId="init-1" />);
|
||||
|
||||
expect(screen.getByTestId("diff-viewer")).toBeInTheDocument();
|
||||
const files = diffViewerProps.files as unknown[];
|
||||
expect(files).toHaveLength(1);
|
||||
expect(diffViewerProps.commitMode).toBe(false);
|
||||
});
|
||||
|
||||
it("2. no rawDiff parsing in phase mode: parseUnifiedDiff is NOT called", () => {
|
||||
render(<ReviewTab initiativeId="init-1" />);
|
||||
|
||||
expect(mockParseUnifiedDiff).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("3. commit view parses rawDiff: parseUnifiedDiff called and DiffViewer gets commitMode=true", async () => {
|
||||
trpcMock = buildTrpcMock({
|
||||
getCommitDiff: {
|
||||
useQuery: vi.fn(() =>
|
||||
noopQuery({ rawDiff: "diff --git a/a.ts b/a.ts\nindex 000..111 100644\n--- a/a.ts\n+++ b/a.ts\n@@ -1,1 +1,1 @@\n-old\n+new\n" })
|
||||
),
|
||||
},
|
||||
});
|
||||
|
||||
render(<ReviewTab initiativeId="init-1" />);
|
||||
|
||||
// Select a commit via the sidebar stub's onSelectCommit prop
|
||||
const { onSelectCommit } = reviewSidebarProps as {
|
||||
onSelectCommit: (hash: string | null) => void;
|
||||
};
|
||||
|
||||
await act(async () => {
|
||||
onSelectCommit("abc123");
|
||||
});
|
||||
|
||||
expect(diffViewerProps.commitMode).toBe(true);
|
||||
expect(mockParseUnifiedDiff).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("4. allFiles uses metadata for sidebar: ReviewSidebar receives files from diffQuery.data.files", () => {
|
||||
render(<ReviewTab initiativeId="init-1" />);
|
||||
|
||||
const sidebarFiles = reviewSidebarProps.files as Array<{ newPath: string }>;
|
||||
expect(sidebarFiles).toHaveLength(1);
|
||||
expect(sidebarFiles[0].newPath).toBe("a.ts");
|
||||
});
|
||||
|
||||
it("5. expandAll prop passed: clicking Expand all button causes DiffViewer to receive expandAll=true", async () => {
|
||||
render(<ReviewTab initiativeId="init-1" />);
|
||||
|
||||
// Before clicking, expandAll should be false
|
||||
expect(diffViewerProps.expandAll).toBe(false);
|
||||
|
||||
const expandBtn = screen.getByTestId("expand-all-btn");
|
||||
await act(async () => {
|
||||
expandBtn.click();
|
||||
});
|
||||
|
||||
expect(diffViewerProps.expandAll).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -8,7 +8,7 @@ import { ReviewSidebar } from "./ReviewSidebar";
|
||||
import { ReviewHeader } from "./ReviewHeader";
|
||||
import { InitiativeReview } from "./InitiativeReview";
|
||||
import { buildCommentIndex } from "./comment-index";
|
||||
import type { ReviewStatus, DiffLine } from "./types";
|
||||
import type { ReviewStatus, DiffLine, FileDiff, FileDiffDetail } from "./types";
|
||||
|
||||
interface ReviewTabProps {
|
||||
initiativeId: string;
|
||||
@@ -18,6 +18,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
const [status, setStatus] = useState<ReviewStatus>("pending");
|
||||
const [selectedCommit, setSelectedCommit] = useState<string | null>(null);
|
||||
const [viewedFiles, setViewedFiles] = useState<Set<string>>(new Set());
|
||||
const [expandAll, setExpandAll] = useState(false);
|
||||
const fileRefs = useRef<Map<string, HTMLDivElement>>(new Map());
|
||||
const headerRef = useRef<HTMLDivElement>(null);
|
||||
const [headerHeight, setHeaderHeight] = useState(0);
|
||||
@@ -74,7 +75,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
const projectsQuery = trpc.getInitiativeProjects.useQuery({ initiativeId });
|
||||
const firstProjectId = projectsQuery.data?.[0]?.id ?? null;
|
||||
|
||||
// Fetch full branch diff for active phase
|
||||
// Fetch full branch diff for active phase (metadata only, no rawDiff)
|
||||
const diffQuery = trpc.getPhaseReviewDiff.useQuery(
|
||||
{ phaseId: activePhaseId! },
|
||||
{ enabled: !!activePhaseId && !isInitiativePendingReview },
|
||||
@@ -96,7 +97,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
// Preview state
|
||||
const previewsQuery = trpc.listPreviews.useQuery({ initiativeId });
|
||||
const existingPreview = previewsQuery.data?.find(
|
||||
(p) => p.phaseId === activePhaseId || p.initiativeId === initiativeId,
|
||||
(p: { phaseId?: string; initiativeId?: string }) => p.phaseId === activePhaseId || p.initiativeId === initiativeId,
|
||||
);
|
||||
const [activePreviewId, setActivePreviewId] = useState<string | null>(null);
|
||||
const previewStatusQuery = trpc.getPreviewStatus.useQuery(
|
||||
@@ -107,12 +108,12 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
const sourceBranch = diffQuery.data?.sourceBranch ?? commitsQuery.data?.sourceBranch ?? "";
|
||||
|
||||
const startPreview = trpc.startPreview.useMutation({
|
||||
onSuccess: (data) => {
|
||||
onSuccess: (data: { id: string; url: string }) => {
|
||||
setActivePreviewId(data.id);
|
||||
previewsQuery.refetch();
|
||||
toast.success(`Preview running at ${data.url}`);
|
||||
},
|
||||
onError: (err) => toast.error(`Preview failed: ${err.message}`),
|
||||
onError: (err: { message: string }) => toast.error(`Preview failed: ${err.message}`),
|
||||
});
|
||||
|
||||
const stopPreview = trpc.stopPreview.useMutation({
|
||||
@@ -121,7 +122,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
toast.success("Preview stopped");
|
||||
previewsQuery.refetch();
|
||||
},
|
||||
onError: (err) => toast.error(`Failed to stop: ${err.message}`),
|
||||
onError: (err: { message: string }) => toast.error(`Failed to stop: ${err.message}`),
|
||||
});
|
||||
|
||||
const previewState = firstProjectId && sourceBranch
|
||||
@@ -157,7 +158,17 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
{ enabled: !!activePhaseId && !isInitiativePendingReview },
|
||||
);
|
||||
const comments = useMemo(() => {
|
||||
return (commentsQuery.data ?? []).map((c) => ({
|
||||
return (commentsQuery.data ?? []).map((c: {
|
||||
id: string;
|
||||
filePath: string;
|
||||
lineNumber: number | null;
|
||||
lineType: string;
|
||||
body: string;
|
||||
author: string;
|
||||
createdAt: string | number;
|
||||
resolved: boolean;
|
||||
parentCommentId?: string | null;
|
||||
}) => ({
|
||||
id: c.id,
|
||||
filePath: c.filePath,
|
||||
lineNumber: c.lineNumber,
|
||||
@@ -179,7 +190,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
onSuccess: () => {
|
||||
utils.listReviewComments.invalidate({ phaseId: activePhaseId! });
|
||||
},
|
||||
onError: (err) => toast.error(`Failed to save comment: ${err.message}`),
|
||||
onError: (err: { message: string }) => toast.error(`Failed to save comment: ${err.message}`),
|
||||
});
|
||||
|
||||
const resolveCommentMutation = trpc.resolveReviewComment.useMutation({
|
||||
@@ -198,14 +209,14 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
onSuccess: () => {
|
||||
utils.listReviewComments.invalidate({ phaseId: activePhaseId! });
|
||||
},
|
||||
onError: (err) => toast.error(`Failed to post reply: ${err.message}`),
|
||||
onError: (err: { message: string }) => toast.error(`Failed to post reply: ${err.message}`),
|
||||
});
|
||||
|
||||
const editCommentMutation = trpc.updateReviewComment.useMutation({
|
||||
onSuccess: () => {
|
||||
utils.listReviewComments.invalidate({ phaseId: activePhaseId! });
|
||||
},
|
||||
onError: (err) => toast.error(`Failed to update comment: ${err.message}`),
|
||||
onError: (err: { message: string }) => toast.error(`Failed to update comment: ${err.message}`),
|
||||
});
|
||||
|
||||
const approveMutation = trpc.approvePhaseReview.useMutation({
|
||||
@@ -214,23 +225,48 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
toast.success("Phase approved and merged");
|
||||
phasesQuery.refetch();
|
||||
},
|
||||
onError: (err) => toast.error(err.message),
|
||||
onError: (err: { message: string }) => toast.error(err.message),
|
||||
});
|
||||
|
||||
// Determine which diff to display
|
||||
const activeDiffRaw = selectedCommit
|
||||
? commitDiffQuery.data?.rawDiff
|
||||
: diffQuery.data?.rawDiff;
|
||||
// Phase branch diff — metadata only, no parsing
|
||||
const phaseFiles: FileDiff[] = useMemo(
|
||||
() => {
|
||||
const serverFiles = diffQuery.data?.files ?? [];
|
||||
// Map server FileStatEntry (path) to frontend FileDiff (newPath)
|
||||
return serverFiles.map((f: {
|
||||
path: string;
|
||||
oldPath?: string;
|
||||
status: FileDiff['status'];
|
||||
additions: number;
|
||||
deletions: number;
|
||||
projectId?: string;
|
||||
}) => ({
|
||||
newPath: f.path,
|
||||
oldPath: f.oldPath ?? f.path,
|
||||
status: f.status,
|
||||
additions: f.additions,
|
||||
deletions: f.deletions,
|
||||
projectId: f.projectId,
|
||||
}));
|
||||
},
|
||||
[diffQuery.data?.files],
|
||||
);
|
||||
|
||||
const files = useMemo(() => {
|
||||
if (!activeDiffRaw) return [];
|
||||
return parseUnifiedDiff(activeDiffRaw);
|
||||
}, [activeDiffRaw]);
|
||||
// Commit diff — still raw, parse client-side
|
||||
const commitFiles: FileDiffDetail[] = useMemo(() => {
|
||||
if (!commitDiffQuery.data?.rawDiff) return [];
|
||||
return parseUnifiedDiff(commitDiffQuery.data.rawDiff);
|
||||
}, [commitDiffQuery.data?.rawDiff]);
|
||||
|
||||
const isDiffLoading = selectedCommit
|
||||
? commitDiffQuery.isLoading
|
||||
: diffQuery.isLoading;
|
||||
|
||||
// All files for sidebar — always from phase metadata
|
||||
const allFiles = phaseFiles;
|
||||
|
||||
const activeFiles: FileDiff[] | FileDiffDetail[] = selectedCommit ? commitFiles : phaseFiles;
|
||||
|
||||
const handleAddComment = useCallback(
|
||||
(filePath: string, lineNumber: number, lineType: DiffLine["type"], body: string) => {
|
||||
if (!activePhaseId) return;
|
||||
@@ -273,7 +309,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
toast.success("Changes requested — revision task dispatched");
|
||||
phasesQuery.refetch();
|
||||
},
|
||||
onError: (err) => toast.error(err.message),
|
||||
onError: (err: { message: string }) => toast.error(err.message),
|
||||
});
|
||||
|
||||
const handleRequestChanges = useCallback(() => {
|
||||
@@ -303,6 +339,11 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
setSelectedCommit(null);
|
||||
setStatus("pending");
|
||||
setViewedFiles(new Set());
|
||||
setExpandAll(false);
|
||||
}, []);
|
||||
|
||||
const handleExpandAll = useCallback(() => {
|
||||
setExpandAll(v => !v);
|
||||
}, []);
|
||||
|
||||
const unresolvedCount = comments.filter((c) => !c.resolved && !c.parentCommentId).length;
|
||||
@@ -312,12 +353,6 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
reviewablePhases.find((p) => p.id === activePhaseId)?.name ??
|
||||
"Phase";
|
||||
|
||||
// All files from the full branch diff (for sidebar file list)
|
||||
const allFiles = useMemo(() => {
|
||||
if (!diffQuery.data?.rawDiff) return [];
|
||||
return parseUnifiedDiff(diffQuery.data.rawDiff);
|
||||
}, [diffQuery.data?.rawDiff]);
|
||||
|
||||
// Initiative-level review takes priority
|
||||
if (isInitiativePendingReview) {
|
||||
return (
|
||||
@@ -363,6 +398,9 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
preview={previewState}
|
||||
viewedCount={viewedFiles.size}
|
||||
totalCount={allFiles.length}
|
||||
totalAdditions={selectedCommit ? undefined : diffQuery.data?.totalAdditions}
|
||||
totalDeletions={selectedCommit ? undefined : diffQuery.data?.totalDeletions}
|
||||
onExpandAll={handleExpandAll}
|
||||
/>
|
||||
|
||||
{/* Main content area — sidebar always rendered to preserve state */}
|
||||
@@ -382,7 +420,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
onFileClick={handleFileClick}
|
||||
onCommentClick={handleCommentClick}
|
||||
selectedCommit={selectedCommit}
|
||||
activeFiles={files}
|
||||
activeFiles={activeFiles}
|
||||
commits={commits}
|
||||
onSelectCommit={setSelectedCommit}
|
||||
viewedFiles={viewedFiles}
|
||||
@@ -397,7 +435,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
<Loader2 className="h-4 w-4 animate-spin" />
|
||||
Loading diff...
|
||||
</div>
|
||||
) : files.length === 0 ? (
|
||||
) : activeFiles.length === 0 ? (
|
||||
<div className="flex h-32 items-center justify-center text-muted-foreground text-sm">
|
||||
{selectedCommit
|
||||
? "No changes in this commit"
|
||||
@@ -405,7 +443,9 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
</div>
|
||||
) : (
|
||||
<DiffViewer
|
||||
files={files}
|
||||
files={activeFiles}
|
||||
phaseId={activePhaseId!}
|
||||
commitMode={!!selectedCommit}
|
||||
commentsByLine={commentsByLine}
|
||||
onAddComment={handleAddComment}
|
||||
onResolveComment={handleResolveComment}
|
||||
@@ -415,6 +455,7 @@ export function ReviewTab({ initiativeId }: ReviewTabProps) {
|
||||
viewedFiles={viewedFiles}
|
||||
onToggleViewed={toggleViewed}
|
||||
onRegisterRef={registerFileRef}
|
||||
expandAll={expandAll}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
import type { FileDiff, FileChangeType, DiffHunk, DiffLine } from "./types";
|
||||
import type { FileDiffDetail, FileDiff, DiffHunk, DiffLine } from "./types";
|
||||
|
||||
/**
|
||||
* Parse a unified diff string into structured FileDiff objects.
|
||||
* Parse a unified diff string into structured FileDiffDetail objects.
|
||||
*/
|
||||
export function parseUnifiedDiff(raw: string): FileDiff[] {
|
||||
const files: FileDiff[] = [];
|
||||
export function parseUnifiedDiff(raw: string): FileDiffDetail[] {
|
||||
const files: FileDiffDetail[] = [];
|
||||
const fileChunks = raw.split(/^diff --git /m).filter(Boolean);
|
||||
|
||||
for (const chunk of fileChunks) {
|
||||
@@ -90,19 +90,19 @@ export function parseUnifiedDiff(raw: string): FileDiff[] {
|
||||
hunks.push({ header, oldStart, oldCount, newStart, newCount, lines: hunkLines });
|
||||
}
|
||||
|
||||
// Derive changeType from header markers and path comparison
|
||||
let changeType: FileChangeType;
|
||||
// Derive status from header markers and path comparison
|
||||
let status: FileDiff['status'];
|
||||
if (hasOldDevNull) {
|
||||
changeType = "added";
|
||||
status = "added";
|
||||
} else if (hasNewDevNull) {
|
||||
changeType = "deleted";
|
||||
status = "deleted";
|
||||
} else if (oldPath !== newPath) {
|
||||
changeType = "renamed";
|
||||
status = "renamed";
|
||||
} else {
|
||||
changeType = "modified";
|
||||
status = "modified";
|
||||
}
|
||||
|
||||
files.push({ oldPath, newPath, hunks, additions, deletions, changeType });
|
||||
files.push({ oldPath, newPath, hunks, additions, deletions, status });
|
||||
}
|
||||
|
||||
return files;
|
||||
|
||||
29
apps/web/src/components/review/types.test.ts
Normal file
29
apps/web/src/components/review/types.test.ts
Normal file
@@ -0,0 +1,29 @@
|
||||
// @vitest-environment happy-dom
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import type { FileDiff, FileDiffDetail } from './types';
|
||||
|
||||
describe('FileDiff types', () => {
|
||||
it('FileDiff accepts binary status', () => {
|
||||
const f: FileDiff = {
|
||||
oldPath: 'a.png',
|
||||
newPath: 'a.png',
|
||||
status: 'binary',
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
};
|
||||
expect(f.status).toBe('binary');
|
||||
});
|
||||
|
||||
it('FileDiffDetail extends FileDiff with hunks', () => {
|
||||
const d: FileDiffDetail = {
|
||||
oldPath: 'a.ts',
|
||||
newPath: 'a.ts',
|
||||
status: 'modified',
|
||||
additions: 5,
|
||||
deletions: 2,
|
||||
hunks: [],
|
||||
};
|
||||
expect(d.hunks).toEqual([]);
|
||||
expect(d.additions).toBe(5);
|
||||
});
|
||||
});
|
||||
@@ -14,15 +14,20 @@ export interface DiffLine {
|
||||
newLineNumber: number | null;
|
||||
}
|
||||
|
||||
export type FileChangeType = 'added' | 'modified' | 'deleted' | 'renamed';
|
||||
|
||||
/** Metadata returned by getPhaseReviewDiff — no hunk content */
|
||||
export interface FileDiff {
|
||||
oldPath: string;
|
||||
newPath: string;
|
||||
hunks: DiffHunk[];
|
||||
/** 'binary' is new — prior changeType used FileChangeType which had no 'binary' */
|
||||
status: 'added' | 'modified' | 'deleted' | 'renamed' | 'binary';
|
||||
additions: number;
|
||||
deletions: number;
|
||||
changeType: FileChangeType;
|
||||
projectId?: string; // present in multi-project initiatives
|
||||
}
|
||||
|
||||
/** Full diff with parsed hunks — returned by getFileDiff, parsed client-side */
|
||||
export interface FileDiffDetail extends FileDiff {
|
||||
hunks: DiffHunk[];
|
||||
}
|
||||
|
||||
export interface ReviewComment {
|
||||
|
||||
Reference in New Issue
Block a user