diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index b4f57653fb6f..22fa1bc5d506 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -382,24 +382,40 @@ export function ReviewSheet(props: ReviewSheetProps) { useEffect(() => { showAuxiliaryPane("inspector"); }, [environmentId, showAuxiliaryPane, threadId]); - const { error, reviewSections, selectedSection, refreshSelectedSection, selectSection } = - useReviewSections({ - enabled: isEnvironmentReady, - environmentId, - threadId, - reviewCache, - }); + const { + error, + reviewSections, + selectedSection, + refreshSelectedSection, + selectSection, + isSelectedSectionPending, + diffPreviewRevision, + } = useReviewSections({ + enabled: isEnvironmentReady, + environmentId, + threadId, + reviewCache, + }); useReviewDiffPrewarming({ threadKey: reviewCache.threadKey, sections: reviewSections, selectedSectionId: selectedSection?.id ?? null, }); - const { headerDiffSummary, nativeReviewDiffData, parsedDiff, pendingReviewCommentCount } = - useReviewDiffData({ - threadKey: reviewCache.threadKey, - selectedSection, - draftMessage, - }); + const { + headerDiffSummary, + nativeReviewDiffData, + parsedDiff, + pendingReviewCommentCount, + loadVisibleFile, + isPending: areFilePatchesPending, + } = useReviewDiffData({ + threadKey: reviewCache.threadKey, + environmentId, + cwd: selectedThreadCwd, + selectedSection, + revision: diffPreviewRevision, + draftMessage, + }); // Resolution returns null while Expo registers the native view (or forever // when the binary lacks it). Rendering a null component type crashes the // app, so callers must fall back — ThreadFeed's ReviewCommentCard does the @@ -469,6 +485,7 @@ export function ReviewSheet(props: ReviewSheetProps) { const handleSelectFile = useCallback( (fileId: string | null) => { + loadVisibleFile(fileId, true); commentSelection.clearSelection(); if (fileId !== null && collapsedFileIds.includes(fileId)) { toggleExpandedFile(fileId); @@ -481,13 +498,14 @@ export function ReviewSheet(props: ReviewSheetProps) { console.error("[review] Failed to navigate to diff file", error); }); }, - [collapsedFileIds, commentSelection, toggleExpandedFile], + [collapsedFileIds, commentSelection, toggleExpandedFile, loadVisibleFile], ); const handleVisibleFileChange = useCallback( (event: NativeSyntheticEvent<{ readonly fileId?: string | null }>) => { + loadVisibleFile(event.nativeEvent.fileId ?? null); reviewFileNavigatorRef.current?.setVisibleFile(event.nativeEvent.fileId ?? null); }, - [], + [loadVisibleFile], ); const renderInspector = useCallback( () => ( @@ -508,10 +526,11 @@ export function ReviewSheet(props: ReviewSheetProps) { (event: NativeSyntheticEvent<{ readonly fileId?: string }>) => { const { fileId } = event.nativeEvent; if (fileId) { + loadVisibleFile(fileId, true); toggleExpandedFile(fileId); } }, - [toggleExpandedFile], + [toggleExpandedFile, loadVisibleFile], ); const handleNativeToggleViewedFile = useCallback( @@ -574,12 +593,12 @@ export function ReviewSheet(props: ReviewSheetProps) { (event: { nativeEvent: { event: string } }) => { const id = event.nativeEvent.event; if (id === "refresh") { - void refreshSelectedSection(); + void handlePullToRefresh(); } else if (id.startsWith("section:")) { selectSection(id.slice("section:".length)); } }, - [refreshSelectedSection, selectSection], + [handlePullToRefresh, selectSection], ); const handleRetryEnvironment = useCallback(() => { void retryEnvironment(environmentId); @@ -818,7 +837,7 @@ export function ReviewSheet(props: ReviewSheetProps) { void handlePullToRefresh()} style={StyleSheet.absoluteFill} appearanceScheme={selectedTheme} @@ -862,7 +881,7 @@ export function ReviewSheet(props: ReviewSheetProps) { // iOS has no other refresh affordance here (the explicit // "Refresh current diff" menu is Android-only). void handlePullToRefresh()} /> } diff --git a/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts b/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts index 1c6699b4d718..7387adb567e7 100644 --- a/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts +++ b/apps/mobile/src/features/review/nativeReviewDiffAdapter.test.ts @@ -98,6 +98,29 @@ function appTheme(themeId: MobileThemeId, appearance: MobileThemeAppearance) { } describe("getCachedNativeReviewDiffData", () => { + it.each([true, false])( + "preserves available diff rows before a notice (has excerpt: %s)", + (hasExcerpt) => { + if (parsedDiff.kind !== "files") throw new Error("Expected a parsed file diff"); + const notice = "This file preview was truncated."; + const result = buildNativeReviewDiffData({ + parsedDiff: { + ...parsedDiff, + files: parsedDiff.files.map((file) => ({ + ...file, + rows: hasExcerpt ? file.rows : [], + notice, + })), + }, + }); + const original = buildNativeReviewDiffData({ parsedDiff }); + expect(result.rows.slice(0, -1)).toEqual( + hasExcerpt ? original.rows : original.rows.filter((row) => row.kind === "file"), + ); + expect(result.rows.at(-1)).toMatchObject({ kind: "notice", text: notice }); + }, + ); + it("reuses the row model for equivalent empty comment arrays", () => { const first = getCachedNativeReviewDiffData(buildInput([])); const second = getCachedNativeReviewDiffData(buildInput([])); diff --git a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts index d783557f7b60..6cc56e8ceab9 100644 --- a/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts +++ b/apps/mobile/src/features/review/nativeReviewDiffAdapter.ts @@ -125,6 +125,8 @@ interface PreparedNativeReviewFileRows { readonly filePath: string; readonly lineCount: number; readonly rows: ReadonlyArray; + readonly commentTargetsByRowId: ReadonlyMap; + readonly rowIdByCommentLineId: ReadonlyMap; commentedRows: { readonly commentsKey: string; readonly rows: ReadonlyArray; @@ -136,6 +138,7 @@ interface PreparedNativeReviewDiffData extends Omit(); +const nativeReviewFileRowsCache = new WeakMap(); function buildReviewCommentsCacheKey(comments: ReadonlyArray): string { if (comments.length === 0) { @@ -261,6 +264,7 @@ function createNoticeRow(fileId: string, suffix: string, text: string): NativeRe } function noticeRowsForFile(file: ReviewRenderableFile): ReadonlyArray { + if (file.notice) return [createNoticeRow(file.id, "loading", file.notice)]; if (file.rows.length > 0) { return []; } @@ -406,11 +410,11 @@ function mapLineRow( }; } -function prepareFileRows( - file: ReviewRenderableFile, - commentTargetsByRowId: Map, - rowIdByCommentLineId: Map, -): PreparedNativeReviewFileRows { +function prepareFileRows(file: ReviewRenderableFile): PreparedNativeReviewFileRows { + const cached = nativeReviewFileRowsCache.get(file); + if (cached) return cached; + const commentTargetsByRowId = new Map(); + const rowIdByCommentLineId = new Map(); const rows: NativeReviewDiffRow[] = [ { kind: "file", @@ -449,14 +453,18 @@ function prepareFileRows( }); rows.push(...noticeRowsForFile(file)); - return { + const prepared: PreparedNativeReviewFileRows = { fileId: file.id, filePath: file.path, lineCount: lineRows.length, // Comments must not split the source deletion/addition runs used for word matching. rows: addNativeWordDiffRanges(rows), + commentTargetsByRowId, + rowIdByCommentLineId, commentedRows: null, }; + nativeReviewFileRowsCache.set(file, prepared); + return prepared; } function insertFileComments( @@ -526,9 +534,15 @@ function prepareNativeReviewDiffData(parsedDiff: ReviewParsedDiff): PreparedNati })); const commentTargetsByRowId = new Map(); const rowIdByCommentLineId = new Map(); - const fileRows = parsedDiff.files.map((file) => - prepareFileRows(file, commentTargetsByRowId, rowIdByCommentLineId), - ); + const fileRows = parsedDiff.files.map(prepareFileRows); + for (const file of fileRows) { + for (const [rowId, target] of file.commentTargetsByRowId) { + commentTargetsByRowId.set(rowId, target); + } + for (const [lineId, rowId] of file.rowIdByCommentLineId) { + rowIdByCommentLineId.set(lineId, rowId); + } + } return { fileRows, diff --git a/apps/mobile/src/features/review/reviewModel.test.ts b/apps/mobile/src/features/review/reviewModel.test.ts index 8a9aadd6d26d..aadddfc9f001 100644 --- a/apps/mobile/src/features/review/reviewModel.test.ts +++ b/apps/mobile/src/features/review/reviewModel.test.ts @@ -8,6 +8,7 @@ import { } from "@t3tools/contracts"; import { + applyReviewDiffMetadata, buildReviewParsedDiff, buildReviewSectionItems, getDefaultReviewSectionId, @@ -135,6 +136,35 @@ describe("buildReviewSectionItems", () => { }); describe("buildReviewParsedDiff", () => { + it.each([ + ["a/example.ts", "a/example.ts"], + ["b/example.ts", "b/example.ts"], + ["a/before.ts", "b/after.ts"], + ])("preserves repository paths from %s to %s", (previousPath, path) => { + const parsed = buildReviewParsedDiff( + [ + `diff --git a/${previousPath} b/${path}`, + ...(previousPath === path + ? [] + : ["similarity index 50%", `rename from ${previousPath}`, `rename to ${path}`]), + `--- a/${previousPath}`, + `+++ b/${path}`, + "@@ -1 +1 @@", + "-before", + "+after", + ].join("\n"), + "repository-paths", + ); + expect(parsed.kind).toBe("files"); + if (parsed.kind !== "files") return; + expect(parsed.files[0]).toMatchObject({ + path, + previousPath: previousPath === path ? null : previousPath, + additions: 1, + deletions: 1, + }); + }); + it("builds renderable rows from a unified patch", () => { const parsed = buildReviewParsedDiff( [ @@ -271,3 +301,34 @@ describe("buildReviewParsedDiff", () => { }); }); }); + +describe("applyReviewDiffMetadata", () => { + it("uses complete counts even when the preview contains only part of one file", () => { + const parsed = buildReviewParsedDiff( + [ + "diff --git a/large.txt b/large.txt", + "--- a/large.txt", + "+++ b/large.txt", + "@@ -1 +1 @@", + "-before", + "+after", + ].join("\n"), + "partial", + ); + const result = applyReviewDiffMetadata(parsed, { + truncated: true, + files: [ + { path: "large.txt", previousPath: null, additions: 4000, deletions: 3000 }, + { path: "unseen.txt", previousPath: null, additions: 100, deletions: 20 }, + ], + }); + expect(result.kind).toBe("files"); + if (result.kind !== "files") return; + expect(result.fileCount).toBe(2); + expect(result.additions).toBe(4100); + expect(result.deletions).toBe(3020); + expect(result.files[0]?.additions).toBe(4000); + expect(result.notice).toContain("Counts include all changes"); + expect(applyReviewDiffMetadata(parsed, null)).toEqual(parsed); + }); +}); diff --git a/apps/mobile/src/features/review/reviewModel.ts b/apps/mobile/src/features/review/reviewModel.ts index 202157b837cc..2cb8c61a1610 100644 --- a/apps/mobile/src/features/review/reviewModel.ts +++ b/apps/mobile/src/features/review/reviewModel.ts @@ -18,6 +18,9 @@ export interface ReviewSectionItem { readonly subtitle: string | null; readonly diff: string | null; readonly isLoading: boolean; + readonly files?: ReviewDiffPreviewSource["files"]; + readonly truncated?: boolean; + readonly source?: ReviewDiffPreviewSource; } export interface ReviewRenderableHunkRow { @@ -47,6 +50,7 @@ export type ReviewRenderableRow = ReviewRenderableHunkRow | ReviewRenderableLine export interface ReviewRenderableFile { readonly id: string; readonly cacheKey: string; + readonly notice?: string; readonly path: string; readonly previousPath: string | null; readonly changeType: ChangeTypes; @@ -126,16 +130,6 @@ function gitSubtitle(section: ReviewDiffPreviewSource): string | null { return "Base branch unavailable"; } -function stripGitPrefix(pathValue: string | undefined): string | null { - if (!pathValue) { - return null; - } - if (pathValue.startsWith("a/") || pathValue.startsWith("b/")) { - return pathValue.slice(2); - } - return pathValue; -} - function stripTrailingNewline(value: string): string { return value.endsWith("\n") ? value.slice(0, -1) : value; } @@ -378,8 +372,8 @@ function buildRenderableRows(file: FileDiffMetadata): ReadonlyArray total + hunk.additionLines, 0); const deletions = file.hunks.reduce((total, hunk) => total + hunk.deletionLines, 0); const cacheKey = file.cacheKey ?? `${previousPath ?? "none"}:${path}:${file.type}`; @@ -442,6 +436,9 @@ export function buildReviewSectionItems(input: { title: section.title, subtitle: gitSubtitle(section), diff: section.diff, + source: section, + ...(section.files ? { files: section.files } : {}), + truncated: section.truncated, isLoading: false, })); const hasDirtyWorktreeItem = gitItems.some((item) => item.id === DIRTY_WORKTREE_SECTION_ID); @@ -527,3 +524,27 @@ export function buildReviewParsedDiff( }; } } + +export function applyReviewDiffMetadata( + previewDiff: ReviewParsedDiff, + selectedSection: Pick | null, +): ReviewParsedDiff { + if (previewDiff.kind === "empty") return previewDiff; + const notice = selectedSection?.truncated + ? `This preview exceeds the size limit. Changes shown are incomplete.${selectedSection.files ? " Counts include all changes." : ""}` + : previewDiff.notice; + if (previewDiff.kind !== "files" || !selectedSection?.files) return { ...previewDiff, notice }; + const totals = selectedSection.files.reduce( + (total, file) => ({ + additions: total.additions + file.additions, + deletions: total.deletions + file.deletions, + }), + { additions: 0, deletions: 0 }, + ); + const stats = new Map(selectedSection.files.map((file) => [file.path, file])); + const files = previewDiff.files.map((file) => { + const stat = stats.get(file.path); + return stat ? { ...file, additions: stat.additions, deletions: stat.deletions } : file; + }); + return { ...previewDiff, ...totals, files, fileCount: selectedSection.files.length, notice }; +} diff --git a/apps/mobile/src/features/review/useReviewDiffData.ts b/apps/mobile/src/features/review/useReviewDiffData.ts index 85aa6b032fae..54fa89387fa7 100644 --- a/apps/mobile/src/features/review/useReviewDiffData.ts +++ b/apps/mobile/src/features/review/useReviewDiffData.ts @@ -1,12 +1,87 @@ -import { useEffect, useMemo } from "react"; +import { useCallback, useContext, useEffect, useMemo, useRef, useState } from "react"; import { countReviewCommentContexts, parseReviewInlineComments } from "./reviewCommentSelection"; import { getCachedNativeReviewDiffData } from "./nativeReviewDiffAdapter"; import { markReviewEvent, measureReviewWork } from "./reviewPerf"; import { getCachedReviewParsedDiff } from "./reviewState"; -import type { ReviewParsedDiff, ReviewSectionItem } from "./reviewModel"; +import { + applyReviewDiffMetadata, + buildReviewParsedDiff, + type ReviewParsedDiff, + type ReviewRenderableFile, + type ReviewSectionItem, +} from "./reviewModel"; + +import type { + EnvironmentId, + ReviewDiffFileStat, + ReviewDiffPreviewSource, +} from "@t3tools/contracts"; +import { RegistryContext, useAtomValue } from "@effect/atom-react"; +import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; +import * as Atom from "effect/unstable/reactivity/Atom"; +import { reviewEnvironment } from "../../state/review"; const EMPTY_INLINE_REVIEW_COMMENTS = Object.freeze([]); +type ParsedFilePatch = AsyncResult.AsyncResult< + { source: ReviewDiffPreviewSource; parsed: ReviewParsedDiff }, + unknown +>; +const normalizedFiles = new WeakMap< + ParsedFilePatch, + { stat: ReviewDiffFileStat; diffHash: string; file: ReviewRenderableFile } +>(); + +function getCachedReviewFile( + stat: ReviewDiffFileStat, + diffHash: string, + patch: ParsedFilePatch | null | undefined, +): ReviewRenderableFile { + const cached = patch && normalizedFiles.get(patch); + if (cached && cached.stat === stat && cached.diffHash === diffHash) return cached.file; + const parsed = patch?._tag === "Success" ? patch.value.parsed : null; + const sourceFiles = patch?._tag === "Success" ? patch.value.source.files : undefined; + const loaded = + parsed?.kind === "files" + ? (parsed.files.find((file) => file.path === stat.path) ?? + (parsed.files.length === 1 && + sourceFiles?.length === 1 && + sourceFiles[0]?.path === stat.path + ? parsed.files[0] + : undefined)) + : undefined; + const file: ReviewRenderableFile = { + ...(loaded ?? { + path: stat.path, + previousPath: stat.previousPath, + changeType: "change" as const, + languageHint: null, + additionLines: [], + deletionLines: [], + rows: [], + cacheKey: `${diffHash}:${stat.path}`, + }), + id: stat.path, + path: stat.path, + previousPath: stat.previousPath, + additions: stat.additions, + deletions: stat.deletions, + ...(patch?._tag === "Success" + ? patch.value.source.truncated + ? { notice: "File preview exceeds the size limit. Counts include all changes." } + : loaded + ? {} + : { notice: "Could not display file preview." } + : { + notice: + patch?._tag === "Failure" + ? "Could not load diff. Select the file to retry." + : "Loading diff…", + }), + }; + if (patch) normalizedFiles.set(patch, { stat, diffHash, file }); + return file; +} function isReviewDiffDebugLoggingEnabled(): boolean { return typeof __DEV__ !== "undefined" ? __DEV__ : false; @@ -25,39 +100,156 @@ function logReviewDiffDiagnostic(message: string, details?: Record total + file.additions, 0)}`, + deletions: `-${files.reduce((total, file) => total + file.deletions, 0)}`, + }; } - - return { - additions: `+${parsedDiff.additions}`, - deletions: `-${parsedDiff.deletions}`, - }; + if (parsedDiff.kind !== "files") return { additions: null, deletions: null }; + return { additions: `+${parsedDiff.additions}`, deletions: `-${parsedDiff.deletions}` }; } export function useReviewDiffData(input: { readonly threadKey: string | null; + readonly environmentId: EnvironmentId | undefined; + readonly cwd: string | null; readonly selectedSection: ReviewSectionItem | null; + readonly revision: string | undefined; readonly draftMessage: string; }) { const { draftMessage, selectedSection, threadKey } = input; const selectedSectionId = selectedSection?.id ?? null; - const parsedDiff = useMemo( + const source = selectedSection?.source; + const lazySource = source?.truncated && source.files ? source : null; + const previewDiff = useMemo( () => - measureReviewWork("parse-diff", () => - getCachedReviewParsedDiff({ - threadKey, - sectionId: selectedSection?.id ?? null, - diff: selectedSection?.diff, - }), + lazySource + ? { kind: "empty" } + : measureReviewWork("parse-diff", () => + getCachedReviewParsedDiff({ + threadKey, + sectionId: selectedSection?.id ?? null, + diff: selectedSection?.diff, + }), + ), + [lazySource, selectedSection?.diff, selectedSection?.id, threadKey], + ); + const registry = useContext(RegistryContext); + const { environmentId, cwd } = input; + const scope = JSON.stringify([ + environmentId, + cwd, + source?.kind, + source?.baseRef, + source?.diffHash, + ]); + const [requested, setRequested] = useState({ scope, indices: [0, 1, 2] }); + const indices = useMemo( + () => new Set(requested.scope === scope ? requested.indices : [0, 1, 2]), + [requested, scope], + ); + const queries = useMemo( + () => + !environmentId || !cwd || !lazySource + ? [] + : (lazySource.files ?? []).map((file, index) => + indices.has(index) + ? reviewEnvironment.diffFilePatch({ + environmentId, + input: { + cacheKey: scope, + request: { + cwd, + ...(lazySource.baseRef ? { baseRef: lazySource.baseRef } : {}), + file: { + path: file.path, + previousPath: file.previousPath, + sourceKind: lazySource.kind, + }, + }, + }, + }) + : null, + ), + [environmentId, cwd, lazySource, indices, scope], + ); + const parsedQuery = useMemo( + () => + Atom.family((query: ReturnType) => + Atom.map(query, (result) => + AsyncResult.map(result, (source) => ({ + source, + parsed: buildReviewParsedDiff(source.diff, source.diffHash), + })), + ), ), - [selectedSection?.diff, selectedSection?.id, threadKey], + [], + ); + const patches = useAtomValue( + useMemo( + () => Atom.make((get) => queries.map((query) => (query ? get(parsedQuery(query)) : null))), + [queries, parsedQuery], + ), + ); + const previousPreview = useRef({ + scope, + revision: input.revision, + queries: [] as typeof queries, + }); + useEffect(() => { + const previous = previousPreview.current; + previousPreview.current = { scope, revision: input.revision, queries }; + for (const query of queries) { + if (!query) continue; + const changed = previous.scope === scope && previous.revision !== input.revision; + const cached = + !previous.queries.includes(query) && + registry.get(query)._tag !== "Initial" && + !registry.get(query).waiting; + if (changed || cached) registry.refresh(query); + } + }, [input.revision, queries, registry, scope]); + const loadVisibleFile = useCallback( + (fileId: string | null, retry = false) => { + const index = + fileId === null ? 0 : (lazySource?.files?.findIndex((file) => file.path === fileId) ?? -1); + if (index < 0) return; + setRequested((current) => { + const previous = current.scope === scope ? current.indices : [0, 1, 2]; + const added = [index, index + 1, index + 2].filter((next) => !previous.includes(next)); + return added.length === 0 ? current : { scope, indices: [...previous, ...added] }; + }); + if (retry && patches[index]?._tag === "Failure" && queries[index]) + registry.refresh(queries[index]); + }, + [lazySource, scope, patches, queries, registry], + ); + const parsedDiff = useMemo(() => { + if (!lazySource?.files) return applyReviewDiffMetadata(previewDiff, selectedSection); + const files = lazySource.files.map((stat, index) => + getCachedReviewFile(stat, lazySource.diffHash, patches[index]), + ); + return { + kind: "files", + files, + fileCount: files.length, + additions: files.reduce((total, file) => total + file.additions, 0), + deletions: files.reduce((total, file) => total + file.deletions, 0), + notice: null, + }; + }, [lazySource, previewDiff, selectedSection, patches]); + const headerDiffSummary = useMemo( + () => formatHeaderDiffSummary(parsedDiff, selectedSection?.files), + [parsedDiff, selectedSection?.files], ); - const headerDiffSummary = useMemo(() => formatHeaderDiffSummary(parsedDiff), [parsedDiff]); const inlineReviewComments = useMemo( () => parseReviewInlineComments(draftMessage), [draftMessage], @@ -104,6 +296,8 @@ export function useReviewDiffData(input: { return { parsedDiff, + loadVisibleFile, + isPending: patches.some((patch) => patch?._tag === "Initial" || patch?.waiting), headerDiffSummary, nativeReviewDiffData, pendingReviewCommentCount, diff --git a/apps/mobile/src/features/review/useReviewSections.ts b/apps/mobile/src/features/review/useReviewSections.ts index 87325490990c..f233ccbf8eb2 100644 --- a/apps/mobile/src/features/review/useReviewSections.ts +++ b/apps/mobile/src/features/review/useReviewSections.ts @@ -1,4 +1,5 @@ import { useCallback, useEffect, useMemo } from "react"; +import * as DateTime from "effect/DateTime"; import type { EnvironmentId, OrchestrationCheckpointSummary, ThreadId } from "@t3tools/contracts"; @@ -66,13 +67,14 @@ export function useReviewSections(input: { () => buildReviewSectionItems({ checkpoints: readyCheckpoints, - gitSections: reviewCache.gitSections, + gitSections: diffPreview.data?.sources ?? reviewCache.gitSections, turnDiffById: reviewCache.turnDiffById, loadingTurnIds, loadingGitSections: diffPreview.isPending, }), [ diffPreview.isPending, + diffPreview.data?.sources, loadingTurnIds, readyCheckpoints, reviewCache.gitSections, @@ -173,7 +175,12 @@ export function useReviewSections(input: { return { error: diffPreview.error ?? activeTurnDiff.error ?? reviewCache.asyncState.error, + isSelectedSectionPending: + selectedSection?.kind === "turn" ? activeTurnDiff.isPending : diffPreview.isPending, loadingGitDiffs: diffPreview.isPending, + diffPreviewRevision: diffPreview.data + ? DateTime.formatIso(diffPreview.data.generatedAt) + : undefined, loadingTurnIds, reviewSections, selectedSection, diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index f9e7b51f862a..5cf3b7c4b4b9 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -15,12 +15,17 @@ import * as Queue from "effect/Queue"; import * as Ref from "effect/Ref"; import * as Result from "effect/Result"; import * as Scope from "effect/Scope"; +import * as Schema from "effect/Schema"; import * as Sink from "effect/Sink"; import * as Stream from "effect/Stream"; import * as TestClock from "effect/testing/TestClock"; import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; -import { GitCommandError, type ReviewDiffFileContentsInput } from "@t3tools/contracts"; +import { + GitCommandError, + ReviewDiffPreviewInput, + type ReviewDiffFileContentsInput, +} from "@t3tools/contracts"; import { ServerConfig } from "../config.ts"; import { gitCommandDuration } from "../observability/Metrics.ts"; import { @@ -958,6 +963,119 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }); describe("review diff previews", () => { + it.effect("loads repository-relative files from a nested project directory", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const { initialBranch } = yield* initRepoWithCommit(cwd); + yield* git(cwd, ["checkout", "-b", "feature/nested"]); + yield* writeTextFile(cwd, "nested/tracked.txt", "committed\n"); + yield* git(cwd, ["add", "."]); + yield* git(cwd, ["commit", "-m", "nested file"]); + yield* writeTextFile(cwd, "nested/tracked.txt", "changed\n"); + yield* writeTextFile(cwd, "untracked.txt", "new\n"); + const path = yield* Path.Path; + const driver = yield* GitVcsDriver.GitVcsDriver; + const nestedCwd = path.join(cwd, "nested"); + const preview = yield* driver.getReviewDiffPreview({ + cwd: nestedCwd, + baseRef: initialBranch, + }); + assert.equal( + preview.sources.find((source) => source.kind === "working-tree")!.files!.length, + 2, + ); + for (const source of preview.sources) { + for (const file of source.files ?? []) { + const scoped = yield* driver.getReviewDiffPreview({ + cwd: nestedCwd, + baseRef: initialBranch, + file: { path: file.path, previousPath: file.previousPath, sourceKind: source.kind }, + }); + const patch = scoped.sources.find((item) => item.kind === source.kind)!; + assert.deepStrictEqual(patch.files, [file]); + assert.include(patch.diff, `b/${file.path}`); + } + } + }), + ); + + it.effect("reads complete tracked and untracked manifests beyond 1 MB", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const { initialBranch } = yield* initRepoWithCommit(cwd); + yield* writeTextFile(cwd, "untracked.txt", "untracked content\n"); + const paths = Array.from({ length: 5000 }, (_, index) => `${"a".repeat(220)}-${index}.txt`); + const stats = paths.map((path) => `1\t0\t${path}\0`).join(""); + const untracked = [...paths, "untracked.txt"].join("\0") + "\0"; + assert.isAbove(stats.length, 1024 * 1024); + assert.isAbove(untracked.length, 1024 * 1024); + let readLargeUntracked = false; + const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; + const spawner = ChildProcessSpawner.make((command) => { + if (ChildProcess.isStandardCommand(command)) { + if ( + command.args.includes("--numstat") && + command.args.includes(`${initialBranch}...HEAD`) + ) { + return Effect.succeed(makeSuccessfulHandle(stats)); + } + if (command.args.includes("ls-files") && command.args.includes("--others")) { + return Effect.succeed(makeSuccessfulHandle(readLargeUntracked ? untracked : "")); + } + } + return delegate.spawn(command); + }); + const driver = yield* makeGitVcsDriverCore().pipe( + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.provide(ServerConfigLayer), + ); + const branch = yield* driver.getReviewDiffPreview({ + cwd, + baseRef: initialBranch, + }); + const files = branch.sources.find((source) => source.kind === "branch-range")!.files!; + assert.equal(files.length, paths.length); + assert.equal(files.at(-1)?.path, paths.at(-1)); + assert.equal( + files.reduce((total, file) => total + file.additions, 0), + paths.length, + ); + readLargeUntracked = true; + const dirty = yield* driver.getReviewDiffPreview({ + cwd, + file: { path: "untracked.txt", previousPath: null, sourceKind: "working-tree" }, + }); + const source = dirty.sources.find((source) => source.kind === "working-tree")!; + assert.deepStrictEqual(source.files, [ + { path: "untracked.txt", previousPath: null, additions: 1, deletions: 0 }, + ]); + assert.include(source.diff, "+untracked content"); + }), + ); + + it.effect("propagates patch failures instead of reporting an empty complete diff", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + yield* writeTextFile(cwd, "README.md", "changed\n"); + const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; + const spawner = ChildProcessSpawner.make((command) => + ChildProcess.isStandardCommand(command) && command.args.includes("--patch") + ? Effect.succeed(makeNonRepositoryHandle()) + : delegate.spawn(command), + ); + const driver = yield* makeGitVcsDriverCore().pipe( + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.provide(ServerConfigLayer), + ); + const result = yield* driver.getReviewDiffPreview({ cwd }).pipe(Effect.result); + assert.isTrue(Result.isFailure(result)); + if (Result.isFailure(result)) { + assert.equal(result.failure.operation, "GitVcsDriver.getReviewDiffPreview.patch"); + } + }), + ); + it.effect("drops an unterminated path from truncated NUL-separated git output", () => Effect.sync(() => { const paths = splitNullSeparatedGitStdoutPaths({ @@ -1008,6 +1126,14 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { ignored.sources.find((source) => source.kind === "working-tree")?.diff, "", ); + assert.deepStrictEqual( + ignored.sources.find((source) => source.kind === "working-tree")?.files, + [], + ); + assert.deepStrictEqual( + ignored.sources.find((source) => source.kind === "branch-range")?.files, + [], + ); assert.strictEqual( ignored.sources.find((source) => source.kind === "branch-range")?.diff, "", @@ -1057,6 +1183,20 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { assert.include(diff, "+literal pathspec contents"); assert.include(diff, "+ordinary contents"); + const scoped = yield* driver.getReviewDiffPreview({ + cwd, + file: { + path: ":(exclude)after.ts", + previousPath: null, + sourceKind: "working-tree", + }, + }); + const scopedSource = scoped.sources.find((source) => source.kind === "working-tree")!; + assert.deepStrictEqual(scopedSource.files, [ + { path: ":(exclude)after.ts", previousPath: null, additions: 1, deletions: 0 }, + ]); + assert.include(scopedSource.diff, "+literal pathspec contents"); + assert.notInclude(scopedSource.diff, "ordinary.ts"); assert.strictEqual(yield* git(cwd, ["ls-files", "--stage"]), indexBefore); }), ); @@ -1096,6 +1236,17 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { .filter((entry) => entry.startsWith("sharedindex.")) .sort(); + const source = preview.sources.find((candidate) => candidate.kind === "working-tree")!; + assert.deepStrictEqual(source.files, [ + { path: "after.ts", previousPath: "before.ts", additions: 1, deletions: 1 }, + ]); + const scoped = yield* driver.getReviewDiffPreview({ + cwd, + file: { path: "after.ts", previousPath: "before.ts", sourceKind: "working-tree" }, + }); + const scopedSource = scoped.sources.find((candidate) => candidate.kind === "working-tree")!; + assert.deepStrictEqual(scopedSource.files, source.files); + assert.equal(scopedSource.diff, diff); assert.include(diff, "rename from before.ts"); assert.include(diff, "rename to after.ts"); assert.include(diff, "-three"); @@ -1169,6 +1320,142 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }), ); + it.effect("keeps complete stats for files beyond the combined patch limit", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const { initialBranch } = yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + const largeContents = "a long line of changed content for the diff preview\n".repeat(4000); + yield* git(cwd, ["checkout", "-b", "feature/large"]); + yield* writeTextFile(cwd, "a-large.txt", largeContents); + yield* writeTextFile(cwd, "z-last.txt", "last file\n"); + yield* git(cwd, ["add", "."]); + yield* git(cwd, ["commit", "-m", "large change"]); + yield* writeTextFile(cwd, "a-large.txt", largeContents.replaceAll("changed", "updated")); + yield* writeTextFile(cwd, "z-last.txt", "last file updated\n"); + yield* writeTextFile(cwd, "untracked.txt", largeContents); + + const preview = yield* driver.getReviewDiffPreview({ cwd, baseRef: initialBranch }); + const branch = preview.sources.find((source) => source.kind === "branch-range")!; + const dirty = preview.sources.find((source) => source.kind === "working-tree")!; + assert.isTrue(branch.truncated); + assert.isTrue(dirty.truncated); + assert.notInclude(branch.diff, "z-last.txt"); + for (const source of [branch, dirty]) { + for (const file of source.files ?? []) { + const individual = yield* driver.getReviewDiffPreview({ + cwd, + baseRef: initialBranch, + file: { path: file.path, previousPath: file.previousPath, sourceKind: source.kind }, + }); + const patch = individual.sources.find((candidate) => candidate.kind === source.kind)!; + assert.isFalse(patch.truncated); + assert.deepStrictEqual(patch.files, [file]); + assert.include(patch.diff, `b/${file.path}`); + assert.isEmpty( + individual.sources.find((candidate) => candidate.kind !== source.kind)!.diff, + ); + } + } + + assert.deepStrictEqual(branch.files, [ + { path: "a-large.txt", previousPath: null, additions: 4000, deletions: 0 }, + { path: "z-last.txt", previousPath: null, additions: 1, deletions: 0 }, + ]); + assert.deepStrictEqual(dirty.files, [ + { path: "a-large.txt", previousPath: null, additions: 4000, deletions: 4000 }, + { path: "untracked.txt", previousPath: null, additions: 4000, deletions: 0 }, + { path: "z-last.txt", previousPath: null, additions: 1, deletions: 1 }, + ]); + }), + ); + + it.effect("preserves renames, unusual paths, modes, and binary statistics", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const { initialBranch } = yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + yield* writeTextFile(cwd, "mode-only.sh", "echo unchanged\n"); + yield* git(cwd, ["add", "mode-only.sh"]); + yield* git(cwd, ["commit", "-m", "add executable candidate"]); + yield* git(cwd, ["checkout", "-b", "feature/paths"]); + yield* git(cwd, ["mv", "README.md", "renamed.md"]); + yield* writeTextFile(cwd, "[literal].txt", "literal\n"); + yield* writeTextFile(cwd, " leading.txt", "whitespace path\n"); + yield* writeTextFile(cwd, "l.txt", "other\n"); + yield* writeTextFile(cwd, "binary.dat", "binary\0data"); + if ((yield* HostProcessPlatform) !== "win32") { + yield* writeTextFile(cwd, "tab\tand\nnewline.txt", "unusual path\n"); + } + yield* git(cwd, ["add", "."]); + yield* git(cwd, ["update-index", "--chmod=+x", "mode-only.sh"]); + yield* git(cwd, ["commit", "-m", "rename and add files"]); + const preview = yield* driver.getReviewDiffPreview({ + cwd, + baseRef: initialBranch, + }); + const branch = preview.sources.find((source) => source.kind === "branch-range")!; + for (const path of ["renamed.md", "[literal].txt", " leading.txt", "mode-only.sh"]) { + const stat = branch.files!.find((file) => file.path === path)!; + const request = yield* Schema.decodeEffect(ReviewDiffPreviewInput)({ + cwd, + baseRef: initialBranch, + file: { path, previousPath: stat.previousPath, sourceKind: "branch-range" }, + }); + const result = yield* driver.getReviewDiffPreview(request); + const scoped = result.sources.find((source) => source.kind === "branch-range")!; + assert.deepStrictEqual(scoped.files, [stat]); + assert.notInclude(scoped.diff, "b/l.txt"); + if (path === "renamed.md") assert.include(scoped.diff, "rename from README.md"); + if (path === "mode-only.sh") { + assert.include(scoped.diff, "old mode 100644"); + assert.include(scoped.diff, "new mode 100755"); + } + } + assert.include(branch.diff, "rename from README.md"); + assert.include(branch.diff, "rename to renamed.md"); + assert.deepInclude(branch.files ?? [], { + path: "renamed.md", + previousPath: "README.md", + additions: 0, + deletions: 0, + }); + assert.deepInclude(branch.files ?? [], { + path: "binary.dat", + previousPath: null, + additions: 0, + deletions: 0, + }); + if ((yield* HostProcessPlatform) !== "win32") { + assert.deepInclude(branch.files ?? [], { + path: "tab\tand\nnewline.txt", + previousPath: null, + additions: 1, + deletions: 0, + }); + } + }), + ); + + it.effect("reports staged and untracked changes before the first commit", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* git(cwd, ["init"]); + yield* writeTextFile(cwd, "staged.txt", "staged\n"); + yield* git(cwd, ["add", "staged.txt"]); + yield* writeTextFile(cwd, "untracked.txt", "untracked\n"); + const driver = yield* GitVcsDriver.GitVcsDriver; + const preview = yield* driver.getReviewDiffPreview({ cwd }); + const dirty = preview.sources.find((source) => source.kind === "working-tree")!; + assert.deepStrictEqual(dirty.files, [ + { path: "staged.txt", previousPath: null, additions: 1, deletions: 0 }, + { path: "untracked.txt", previousPath: null, additions: 1, deletions: 0 }, + ]); + assert.include(dirty.diff, "b/staged.txt"); + assert.include(dirty.diff, "b/untracked.txt"); + }), + ); + it.effect("loads full file contents for working-tree diff expansion", () => Effect.gen(function* () { const cwd = yield* makeTmpDir(); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index f11471235f1b..9367d027a210 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -1,4 +1,3 @@ -import * as Arr from "effect/Array"; import * as Cache from "effect/Cache"; import * as Data from "effect/Data"; import * as Crypto from "effect/Crypto"; @@ -23,10 +22,12 @@ import { GitCommandError, type ReviewDiffFileContentsInput, type ReviewDiffPreviewInput, + type ReviewDiffFileStat, type ReviewDiffPreviewSource, type VcsRef, } from "@t3tools/contracts"; import { dedupeRemoteBranchesWithLocalMatches, normalizeGitRemoteUrl } from "@t3tools/shared/git"; +import { HostProcessPlatform } from "@t3tools/shared/hostProcess"; import { compactTraceAttributes } from "@t3tools/shared/observability"; import { decodeJsonResult } from "@t3tools/shared/schemaJson"; import { gitCommandDuration, gitCommandsTotal, withMetrics } from "../observability/Metrics.ts"; @@ -52,13 +53,12 @@ const RANGE_COMMIT_SUMMARY_MAX_OUTPUT_BYTES = 19_000; const RANGE_DIFF_SUMMARY_MAX_OUTPUT_BYTES = 19_000; const RANGE_DIFF_PATCH_MAX_OUTPUT_BYTES = 59_000; const REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES = 120_000; -const REVIEW_UNTRACKED_DIFF_MAX_OUTPUT_BYTES = 80_000; +const REVIEW_METADATA_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; const REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; // Patches the clients render are parsed against git's default a/ and b/ path // prefixes. A repository or global diff.noprefix or diff.mnemonicPrefix would // otherwise leak into the patch and leave every parsed file unnamed. export const PATCH_RENDER_PREFIX_ARGS = ["--src-prefix=a/", "--dst-prefix=b/"] as const; -const WORKSPACE_FILES_MAX_OUTPUT_BYTES = 120_000; const STATUS_UPSTREAM_REFRESH_INTERVAL = Duration.seconds(15); const STATUS_UPSTREAM_REFRESH_TIMEOUT = Duration.seconds(5); @@ -192,6 +192,26 @@ function parseNumstatEntries( return entries; } +// -z preserves tabs/newlines in paths and gives renames two separate path fields. +function parseReviewNumstat(stdout: string): ReviewDiffFileStat[] { + const fields = stdout.split("\0"); + const files: ReviewDiffFileStat[] = []; + for (let index = 0; index < fields.length; index++) { + const field = fields[index]!; + const match = /^(\d+|-)\t(\d+|-)\t([\s\S]*)$/.exec(field); + if (!match) continue; + const previousPath = match[3] === "" ? fields[++index]! : null; + const path = previousPath !== null ? fields[++index]! : match[3]!; + files.push({ + path, + previousPath, + additions: match[1] === "-" ? 0 : Number(match[1]), + deletions: match[2] === "-" ? 0 : Number(match[2]), + }); + } + return files; +} + function parsePorcelainPath(line: string): string | null { if (line.startsWith("? ") || line.startsWith("! ")) { const simple = line.slice(2).trim(); @@ -2258,102 +2278,19 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* }; }); - const readUntrackedReviewDiffs = Effect.fn("readUntrackedReviewDiffs")(function* (cwd: string) { - const untrackedResult = yield* executeGit( - "GitVcsDriver.readUntrackedReviewDiffs.list", - cwd, - ["ls-files", "--others", "--exclude-standard", "-z"], - { - maxOutputBytes: WORKSPACE_FILES_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ); - const untrackedPaths = splitNullSeparatedGitStdoutPaths(untrackedResult); - if (untrackedPaths.length === 0) { - return { diff: "", truncated: untrackedResult.stdoutTruncated }; - } - - const diffs = yield* Effect.forEach( - untrackedPaths, - (relativePath) => - executeGit( - "GitVcsDriver.readUntrackedReviewDiffs.diff", - cwd, - [ - "diff", - "--no-index", - "--patch", - "--no-color", - "--no-ext-diff", - "--no-textconv", - "--minimal", - ...PATCH_RENDER_PREFIX_ARGS, - "--", - "/dev/null", - relativePath, - ], - { - allowNonZeroExit: true, - maxOutputBytes: REVIEW_UNTRACKED_DIFF_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ), - { concurrency: 4 }, - ); - - return { - diff: Arr.filterMap(diffs, (result) => - result.stdout.trim().length > 0 ? Result.succeed(result.stdout) : Result.failVoid, - ).join("\n"), - truncated: untrackedResult.stdoutTruncated || diffs.some((result) => result.stdoutTruncated), - }; - }); - - const readTrackedReviewDiff = Effect.fn("readTrackedReviewDiff")(function* ( - cwd: string, - ignoreWhitespace: boolean | undefined, - ) { - const result = yield* executeGit( - "GitVcsDriver.readTrackedReviewDiff", - cwd, - [ - "diff", - "--patch", - "--no-color", - "--no-ext-diff", - "--no-textconv", - "--minimal", - ...PATCH_RENDER_PREFIX_ARGS, - "--find-renames", - ...(ignoreWhitespace ? ["--ignore-all-space"] : []), - "HEAD", - "--", - ], - { - maxOutputBytes: REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ); - return { diff: result.stdout, truncated: result.stdoutTruncated }; - }); - - const readUnifiedWorkingTreeReviewDiff = Effect.fn("readUnifiedWorkingTreeReviewDiff")(function* ( + // Use the same temporary index for patch and statistics so unstaged renames agree. + const prepareReviewIndex = Effect.fn("prepareReviewIndex")(function* ( cwd: string, untrackedPaths: ReadonlyArray, - pathsTruncated: boolean, - ignoreWhitespace: boolean | undefined, ) { const [stagedDeletionsStdout, indexValue] = yield* Effect.all( [ - runGitStdout("GitVcsDriver.readUnifiedWorkingTreeReviewDiff.stagedDeletions", cwd, [ - "diff", - "--cached", - "--name-only", - "--diff-filter=D", - "-z", - "HEAD", - "--", - ]), + runGitStdoutWithOptions( + "GitVcsDriver.readUnifiedWorkingTreeReviewDiff.stagedDeletions", + cwd, + ["diff", "--cached", "--name-only", "--diff-filter=D", "-z", "HEAD", "--"], + { allowNonZeroExit: true, maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES }, + ), runGitStdout("GitVcsDriver.readUnifiedWorkingTreeReviewDiff.indexPath", cwd, [ "rev-parse", "--git-path", @@ -2364,10 +2301,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ); const stagedDeletions = new Set(stagedDeletionsStdout.split("\0").filter(Boolean)); const pathsToAdd = untrackedPaths.filter((relativePath) => !stagedDeletions.has(relativePath)); - if (pathsToAdd.length === 0) { - const tracked = yield* readTrackedReviewDiff(cwd, ignoreWhitespace); - return { ...tracked, truncated: pathsTruncated || tracked.truncated }; - } + if (pathsToAdd.length === 0) return undefined; const indexPath = path.isAbsolute(indexValue.trim()) ? indexValue.trim() @@ -2375,7 +2309,8 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* const tempIndexPath = yield* fileSystem.makeTempFileScoped({ prefix: `t3code-review-index-${process.pid}-`, }); - yield* fileSystem.copyFile(indexPath, tempIndexPath); + const indexExists = yield* fileSystem.exists(indexPath); + if (indexExists) yield* fileSystem.copyFile(indexPath, tempIndexPath); const env = { GIT_INDEX_FILE: tempIndexPath } satisfies NodeJS.ProcessEnv; const tempIndexConfig = [ "-c", @@ -2383,6 +2318,9 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* "-c", "splitIndex.sharedIndexExpire=never", ]; + if (!indexExists) { + yield* executeGit("GitVcsDriver.review.emptyIndex", cwd, ["read-tree", "--empty"], { env }); + } yield* executeGit( "GitVcsDriver.readUnifiedWorkingTreeReviewDiff.expandSplitIndex", cwd, @@ -2402,86 +2340,27 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ], { env, stdin: `${pathsToAdd.join("\0")}\0` }, ); - const result = yield* executeGit( - "GitVcsDriver.readUnifiedWorkingTreeReviewDiff.diff", - cwd, - [ - ...tempIndexConfig, - "diff", - "--patch", - "--no-color", - "--no-ext-diff", - "--no-textconv", - "--minimal", - ...PATCH_RENDER_PREFIX_ARGS, - "--find-renames", - ...(ignoreWhitespace ? ["--ignore-all-space"] : []), - "HEAD", - "--", - ], - { - env, - maxOutputBytes: REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ); - return { diff: result.stdout, truncated: pathsTruncated || result.stdoutTruncated }; - }); - - const readWorkingTreeReviewDiff = Effect.fn("readWorkingTreeReviewDiff")(function* ( - cwd: string, - ignoreWhitespace: boolean | undefined, - ) { - const untrackedResult = yield* executeGit( - "GitVcsDriver.readWorkingTreeReviewDiff.listUntracked", - cwd, - ["ls-files", "--others", "--exclude-standard", "-z"], - { - maxOutputBytes: WORKSPACE_FILES_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ).pipe(Effect.option); - if (untrackedResult._tag === "None") { - return yield* readTrackedReviewDiff(cwd, ignoreWhitespace); - } - const untrackedPaths = splitNullSeparatedGitStdoutPaths(untrackedResult.value); - if (untrackedPaths.length === 0) { - const tracked = yield* readTrackedReviewDiff(cwd, ignoreWhitespace); - return { ...tracked, truncated: untrackedResult.value.stdoutTruncated || tracked.truncated }; - } - - return yield* readUnifiedWorkingTreeReviewDiff( - cwd, - untrackedPaths, - untrackedResult.value.stdoutTruncated, - ignoreWhitespace, - ).pipe( - Effect.scoped, - Effect.catch(() => - Effect.all([ - readTrackedReviewDiff(cwd, ignoreWhitespace).pipe( - Effect.orElseSucceed(() => ({ diff: "", truncated: false })), - ), - readUntrackedReviewDiffs(cwd).pipe( - Effect.orElseSucceed(() => ({ diff: "", truncated: false })), - ), - ]).pipe( - Effect.map(([tracked, untracked]) => ({ - diff: [tracked.diff.trimEnd(), untracked.diff.trimEnd()] - .filter((diff) => diff.length > 0) - .join("\n"), - truncated: tracked.truncated || untracked.truncated, - })), - ), - ), - ); + return env; }); const getReviewDiffPreview = Effect.fn("getReviewDiffPreview")(function* ( input: ReviewDiffPreviewInput, ) { - const details = yield* statusDetailsLocal(input.cwd); - if (!details.isRepo) { + const pathArgs = input.file + ? [input.file.path, ...(input.file.previousPath ? [input.file.previousPath] : [])].map( + (path) => `:(top,literal)${path}`, + ) + : []; + const patchLimit = input.file + ? REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES + : REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES; + const repository = yield* resolveRepositoryPathsUncached(input.cwd).pipe( + Effect.catchTags({ + GitCommandError: (error) => + isMissingGitCwdError(error) ? Effect.succeed(null) : Effect.fail(error), + }), + ); + if (!repository?.worktreeRoot) { return { cwd: input.cwd, generatedAt: yield* DateTime.now, @@ -2489,71 +2368,143 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* }; } - const branch = details.branch; + const cwd = repository.worktreeRoot; + const branch = repository.currentBranch; const baseRef = input.baseRef ?? (branch - ? yield* resolveBaseBranchForNoUpstream(input.cwd, branch).pipe( - Effect.orElseSucceed(() => null), - ) + ? yield* resolveBaseBranchForNoUpstream(cwd, branch).pipe(Effect.orElseSucceed(() => null)) : null); - const dirtyResult = yield* readWorkingTreeReviewDiff(input.cwd, input.ignoreWhitespace).pipe( - Effect.orElseSucceed(() => ({ - diff: "", - truncated: false, - })), + const diffArgs = [ + "diff", + "--find-renames", + "--no-color", + "--no-ext-diff", + "--no-textconv", + "--minimal", + ...PATCH_RENDER_PREFIX_ARGS, + ...(input.ignoreWhitespace ? ["--ignore-all-space"] : []), + ]; + const readStats = Effect.fn("GitVcsDriver.getReviewDiffPreview.stat")(function* ( + ref: string, + env?: NodeJS.ProcessEnv, + ) { + const args = [...diffArgs, "--numstat", "-z"]; + const result = yield* executeGit( + "GitVcsDriver.getReviewDiffPreview.stat", + cwd, + [...args, ref, "--", ...pathArgs], + { allowNonZeroExit: true, maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES, env }, + ); + if (result.exitCode === 0) return { ref, files: parseReviewNumstat(result.stdout) }; + if (ref === "HEAD" && isUnbornHeadStderr(result.stderr)) { + const emptyTree = (yield* runGitStdout("GitVcsDriver.getReviewDiffPreview.emptyTree", cwd, [ + "hash-object", + "-t", + "tree", + (yield* HostProcessPlatform) === "win32" ? "NUL" : "/dev/null", + ])).trim(); + const stdout = yield* runGitStdoutWithOptions( + "GitVcsDriver.getReviewDiffPreview.unbornStat", + cwd, + [...args, emptyTree, "--", ...pathArgs], + { maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES, env }, + ); + return { ref: emptyTree, files: parseReviewNumstat(stdout) }; + } + return yield* new GitCommandError({ + operation: "GitVcsDriver.getReviewDiffPreview.stat", + cwd, + command: "git diff --numstat", + detail: "Could not read complete diff statistics.", + exitCode: result.exitCode, + }); + }); + const readTrackedDiff = Effect.fn("GitVcsDriver.getReviewDiffPreview.tracked")(function* ( + ref: string | null, + env?: NodeJS.ProcessEnv, + ) { + if (ref === null) return { stdout: "", stdoutTruncated: false, files: [] }; + const stat = yield* readStats(ref, env); + if (stat.files.length === 0) return { stdout: "", stdoutTruncated: false, files: [] }; + const patch = yield* executeGit( + "GitVcsDriver.getReviewDiffPreview.patch", + cwd, + [...diffArgs, "--patch", stat.ref, "--", ...pathArgs], + { maxOutputBytes: patchLimit, appendTruncationMarker: true, env }, + ); + return { ...patch, files: stat.files }; + }); + const readDirty = Effect.gen(function* () { + if (input.file?.sourceKind === "branch-range") return yield* readTrackedDiff(null); + const untracked = yield* executeGit( + "GitVcsDriver.review.listUntracked", + cwd, + ["ls-files", "--others", "--exclude-standard", "-z", "--", ...pathArgs], + { maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES }, + ).pipe( + Effect.catchIf( + (error) => error.outputLength === undefined, + () => Effect.succeed(null), + ), + ); + if (untracked === null) { + const tracked = yield* readTrackedDiff("HEAD"); + return { ...tracked, files: undefined, stdoutTruncated: true }; + } + const paths = splitNullSeparatedGitStdoutPaths(untracked).filter( + (candidate) => !input.file || candidate === input.file.path, + ); + if (paths.length === 0) return yield* readTrackedDiff("HEAD"); + const env = yield* prepareReviewIndex(cwd, paths).pipe( + Effect.catchTags({ + PlatformError: (cause) => + Effect.fail( + new GitCommandError({ + operation: "GitVcsDriver.prepareReviewIndex", + cwd, + command: "git diff", + detail: "Could not prepare the review index.", + cause, + }), + ), + }), + ); + return yield* readTrackedDiff("HEAD", env); + }).pipe(Effect.scoped); + const [dirtyTrackedResult, baseResult] = yield* Effect.all( + [ + readDirty, + readTrackedDiff( + baseRef && branch && input.file?.sourceKind !== "working-tree" + ? `${baseRef}...HEAD` + : null, + ), + ], + { concurrency: 2 }, ); - const dirtyDiff = dirtyResult.diff; - - const baseResult = - baseRef && branch - ? yield* executeGit( - "GitVcsDriver.getReviewDiffPreview.base", - input.cwd, - [ - "diff", - "--patch", - "--no-color", - "--no-ext-diff", - "--no-textconv", - "--minimal", - ...PATCH_RENDER_PREFIX_ARGS, - ...(input.ignoreWhitespace ? ["--ignore-all-space"] : []), - `${baseRef}...HEAD`, - ], - { - maxOutputBytes: REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES, - appendTruncationMarker: true, - }, - ).pipe( - Effect.orElseSucceed(() => ({ - exitCode: 0, - stdout: "", - stderr: "", - stdoutTruncated: false, - stderrTruncated: false, - })), - ) - : null; - const baseDiff = baseResult?.stdout ?? ""; - const hashDiff = (diff: string) => - crypto.digest("SHA-256", new TextEncoder().encode(diff)).pipe( + const dirtyFiles = dirtyTrackedResult.files; + const baseFiles = baseResult.files; + const dirtyDiff = dirtyTrackedResult.stdout; + const baseDiff = baseResult.stdout; + const hashDiff = (diff: string, files: ReadonlyArray) => + crypto.digest("SHA-256", new TextEncoder().encode(JSON.stringify([diff, files]))).pipe( Effect.map(Encoding.encodeHex), Effect.mapError( (cause) => new GitCommandError({ operation: "GitVcsDriver.getReviewDiffPreview.hash", command: "crypto.digest SHA-256", - cwd: input.cwd, + cwd, detail: "Failed to hash review diff.", cause, }), ), ); const [dirtyDiffHash, baseDiffHash] = yield* Effect.all([ - hashDiff(dirtyDiff), - hashDiff(baseDiff), + hashDiff(dirtyDiff, dirtyFiles ?? []), + hashDiff(baseDiff, baseFiles), ]); const sources: ReviewDiffPreviewSource[] = [ @@ -2564,8 +2515,9 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* baseRef: "HEAD", headRef: null, diff: dirtyDiff, + ...(dirtyFiles === undefined ? {} : { files: dirtyFiles }), diffHash: dirtyDiffHash, - truncated: dirtyResult.truncated, + truncated: dirtyTrackedResult.stdoutTruncated, }, { id: "branch-range", @@ -2574,8 +2526,9 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* baseRef, headRef: branch ?? "HEAD", diff: baseDiff, + files: baseFiles, diffHash: baseDiffHash, - truncated: baseResult?.stdoutTruncated ?? false, + truncated: baseResult.stdoutTruncated, }, ]; diff --git a/apps/web/src/components/DiffPanel.tsx b/apps/web/src/components/DiffPanel.tsx index ca0bdaa7b4d5..795ddf03d2ae 100644 --- a/apps/web/src/components/DiffPanel.tsx +++ b/apps/web/src/components/DiffPanel.tsx @@ -1,6 +1,6 @@ import { RefreshIcon } from "~/components/ui/refresh-icon"; import { useAtomValue } from "@effect/atom-react"; -import type { FileDiffContentsLoader } from "@pierre/diffs"; +import type { FileDiffContentsLoader, FileDiffMetadata } from "@pierre/diffs"; import { useParams } from "@tanstack/react-router"; import { isAtomCommandInterrupted, @@ -23,6 +23,7 @@ import { TextWrapIcon, } from "lucide-react"; import * as Schema from "effect/Schema"; +import * as DateTime from "effect/DateTime"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useCodeViewFileReveal } from "./diffs/useCodeViewFileReveal"; import { useOpenInPreferredEditor } from "../editorPreferences"; @@ -86,9 +87,29 @@ import { vcsEnvironment } from "../state/vcs"; import { buildBaseRefChoices, filterBaseRefChoices } from "../lib/baseRefChoices"; import { createGitDiffFileContentsLoader } from "../lib/diffFileContents"; +import { useReviewFilePatches } from "./diffs/useReviewFilePatches"; +import { DiffFileLoadingBoundary } from "./diffs/DiffFileLoadingBoundary"; +import { DiffFileStatus } from "./diffs/DiffFileStatus"; + type DiffThemeType = "light" | "dark"; const AUTOMATIC_BASE_REF = "__automatic_base_ref__"; const DIFF_FILE_TREE_STORAGE_KEY = "t3code.diffFileTreeOpen"; +const fileEntryCache = new WeakMap< + FileDiffMetadata, + { fileDiff: FileDiffMetadata; fileKey: string; fileVersion: number } +>(); + +function getCachedFileEntry(fileDiff: FileDiffMetadata) { + const cached = fileEntryCache.get(fileDiff); + if (cached) return cached; + const entry = { + fileDiff, + fileKey: buildFileDiffIdentityKey(fileDiff), + fileVersion: buildFileDiffContentVersion(fileDiff), + }; + fileEntryCache.set(fileDiff, entry); + return entry; +} interface CollapsedDiffFilesState { readonly scopeKey: string | null; @@ -284,30 +305,18 @@ export default function DiffPanel({ const branchDiffPreview = shouldRetryBranchDiffAtEnvironmentCwd ? fallbackBranchDiffPreview : primaryBranchDiffPreview; - const refreshBranchDiffPreview = branchDiffPreview.refresh; const canRefreshGitDiff = isGitRepo && selectedTurnId === null && activeThread != null && activeCwd != null; const activeThreadRefreshKey = routeThreadRef ? `${routeThreadRef.environmentId}:${routeThreadRef.threadId}` : null; - useEffect(() => { - if (!canRefreshGitDiff) return; - const refreshOnFocus = () => refreshBranchDiffPreview(); - window.addEventListener("focus", refreshOnFocus); - return () => window.removeEventListener("focus", refreshOnFocus); - }, [canRefreshGitDiff, refreshBranchDiffPreview]); - - useWorkspaceMutationRefresh({ - enabled: canRefreshGitDiff, - mutationId: workspaceMutationId, - refresh: refreshBranchDiffPreview, - resourceKey: `diff:${activeThreadRefreshKey ?? ""}`, - }); - const selectedGitSource = branchDiffPreview.data?.sources.find( (source) => source.kind === (selectedGitScope === "unstaged" ? "working-tree" : "branch-range"), ); + const refreshPreviewQuery = branchDiffPreview.refresh; + const refreshDiffFromUserAction = refreshPreviewQuery; + const currentLoadDiffFiles = useMemo(() => { const preview = branchDiffPreview.data; if (selectedTurnId !== null || !activeThread || !preview || !selectedGitSource) { @@ -394,31 +403,64 @@ export default function DiffPanel({ const selectedPatchError = selectedTurn ? activeCheckpointDiff.error : branchDiffPreview.error; const hasResolvedPatch = typeof selectedPatch === "string"; const hasNoNetChanges = hasResolvedPatch && selectedPatch.trim().length === 0; + const lazySource = + !selectedTurn && selectedGitSource?.truncated && selectedGitSource.files + ? selectedGitSource + : null; const renderablePatch = useMemo( () => - getRenderablePatch(selectedPatch, `diff-panel:${resolvedTheme}`, { - compactPartialHunkOffsets: selectedTurnId === null, - }), - [resolvedTheme, selectedPatch, selectedTurnId], + lazySource + ? null + : getRenderablePatch(selectedPatch, `diff-panel:${resolvedTheme}`, { + compactPartialHunkOffsets: selectedTurnId === null, + }), + [lazySource, resolvedTheme, selectedPatch, selectedTurnId], ); - const renderableFiles = useMemo(() => { - if (!renderablePatch || renderablePatch.kind !== "files") { - return []; - } - return renderablePatch.files.toSorted((left, right) => - resolveFileDiffPath(left).localeCompare(resolveFileDiffPath(right), undefined, { - numeric: true, - sensitivity: "base", - }), - ); - }, [renderablePatch]); + const fileStats = useMemo( + () => new Map(lazySource?.files?.map((file) => [file.path, file])), + [lazySource?.files], + ); + const { + scope: filePatchScope, + isPending: areFilePatchesPending, + fileStates, + retry, + requestFile, + readyFilePaths, + renderableFiles, + settledFileCount, + loadNextFiles, + } = useReviewFilePatches({ + environmentId: activeThread?.environmentId, + cwd: branchDiffPreview.data?.cwd, + source: lazySource, + baseRef: lazySource?.baseRef ?? selectedBaseRef, + ignoreWhitespace: diffIgnoreWhitespace, + theme: resolvedTheme, + revision: branchDiffPreview.data + ? DateTime.formatIso(branchDiffPreview.data.generatedAt) + : undefined, + preview: renderablePatch, + }); + const refreshBranchDiffPreview = refreshPreviewQuery; + + useEffect(() => { + if (!canRefreshGitDiff) return; + const refreshOnFocus = () => refreshBranchDiffPreview(); + window.addEventListener("focus", refreshOnFocus); + return () => window.removeEventListener("focus", refreshOnFocus); + }, [canRefreshGitDiff, refreshBranchDiffPreview]); + + useWorkspaceMutationRefresh({ + enabled: canRefreshGitDiff, + mutationId: workspaceMutationId, + refresh: refreshBranchDiffPreview, + resourceKey: `diff:${activeThreadRefreshKey ?? ""}`, + }); + + const isRefreshingDiff = branchDiffPreview.isPending || areFilePatchesPending; const renderableFileEntries = useMemo( - () => - renderableFiles.map((fileDiff) => ({ - fileDiff, - fileKey: buildFileDiffIdentityKey(fileDiff), - fileVersion: buildFileDiffContentVersion(fileDiff), - })), + () => renderableFiles.map(getCachedFileEntry), [renderableFiles], ); const defaultCollapsedDiffFileKeys = useMemo( @@ -432,22 +474,51 @@ export default function DiffPanel({ collapsedDiffFiles.scopeKey === collapseScopeKey ? collapsedDiffFiles.fileKeys : defaultCollapsedDiffFileKeys; + const renderLoadingBoundary = useCallback( + () => + settledFileCount < renderableFiles.length ? ( + + ) : null, + [settledFileCount, renderableFiles.length, loadNextFiles], + ); const codeViewFiles = useMemo( () => - renderableFileEntries.map(({ fileDiff, fileKey, fileVersion }) => { - return { - fileDiff, - filePath: resolveFileDiffPath(fileDiff), - fileKey, - fileVersion, - collapsed: collapsedDiffFileKeys.has(fileKey), - }; - }), - [collapsedDiffFileKeys, renderableFileEntries], + renderableFileEntries + .filter(({ fileDiff }) => !lazySource || readyFilePaths.has(resolveFileDiffPath(fileDiff))) + .map(({ fileDiff, fileKey, fileVersion }) => { + return { + fileDiff, + filePath: resolveFileDiffPath(fileDiff), + fileKey, + fileVersion, + // Header-only placeholders use the viewer's collapsed geometry until their patch arrives. + collapsed: + collapsedDiffFileKeys.has(fileKey) || + fileDiff.cacheKey?.endsWith(":pending") === true, + }; + }), + [collapsedDiffFileKeys, renderableFileEntries, lazySource, readyFilePaths], + ); + const diffFileKeys = useMemo( + () => renderableFileEntries.map((file) => file.fileKey), + [renderableFileEntries], ); - const diffFileKeys = useMemo(() => codeViewFiles.map((file) => file.fileKey), [codeViewFiles]); const allDiffFilesCollapsed = areAllDiffFilesCollapsed(diffFileKeys, collapsedDiffFileKeys); - const diffLineStat = useMemo(() => getDiffLineStat(renderableFiles), [renderableFiles]); + const diffLineStat = useMemo(() => { + if (!selectedTurn && selectedGitSource?.files) { + return selectedGitSource.files.reduce( + (total, file) => ({ + additions: total.additions + file.additions, + deletions: total.deletions + file.deletions, + }), + { additions: 0, deletions: 0 }, + ); + } + return getDiffLineStat(renderableFiles); + }, [renderableFiles, selectedGitSource, selectedTurn]); const fileTreeEntries = useMemo(() => diffFileTreeEntries(renderableFiles), [renderableFiles]); const selectedDiffFileKey = selectedFilePath ? (codeViewFiles.find((candidate) => candidate.filePath === selectedFilePath)?.fileKey ?? null) @@ -462,25 +533,54 @@ export default function DiffPanel({ () => ({ collapseScopeKey, diffSelection }), [collapseScopeKey, diffSelection], ); - const requestTreeReveal = useCodeViewFileReveal(codeView, treeRevealScope); + const requestTreeReveal = useCodeViewFileReveal( + codeView, + treeRevealScope, + codeViewFiles.map((file) => file.fileKey), + ); const revealDiffFile = useCallback( (filePath: string) => { - const file = codeViewFiles.find((candidate) => candidate.filePath === filePath); + const index = renderableFileEntries.findIndex( + (candidate) => resolveFileDiffPath(candidate.fileDiff) === filePath, + ); + const file = renderableFileEntries[index]; if (!file) return; - if (file.collapsed) { - setCollapsedDiffFiles((current) => { - const next = new Set( - current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys, - ); - next.delete(file.fileKey); - return { scopeKey: collapseScopeKey, fileKeys: next }; - }); + setCollapsedDiffFiles((current) => { + const next = new Set( + current.scopeKey === collapseScopeKey ? current.fileKeys : defaultCollapsedDiffFileKeys, + ); + next.delete(file.fileKey); + return { scopeKey: collapseScopeKey, fileKeys: next }; + }); + if (lazySource && index >= settledFileCount) { + requestFile(index); } requestTreeReveal(file.fileKey); }, - [codeViewFiles, collapseScopeKey, defaultCollapsedDiffFileKeys, requestTreeReveal], + [ + renderableFileEntries, + collapseScopeKey, + defaultCollapsedDiffFileKeys, + requestTreeReveal, + lazySource, + settledFileCount, + requestFile, + ], ); + const externalRevealRef = useRef<{ cache: string; key: string } | null>(null); + useEffect(() => { + if (!lazySource || !selectedFilePath) return; + const key = `${selectedFilePath}:${selectedFileRevealRequestId}`; + if ( + externalRevealRef.current?.cache === filePatchScope && + externalRevealRef.current.key === key + ) + return; + externalRevealRef.current = { cache: filePatchScope, key }; + revealDiffFile(selectedFilePath); + }, [lazySource, selectedFilePath, selectedFileRevealRequestId, filePatchScope, revealDiffFile]); + const openDiffFile = useCallback( (filePath: string) => { openDiffFilePrimaryAction({ @@ -754,14 +854,14 @@ export default function DiffPanel({ )}
- {codeViewFiles.length > 0 && ( + {codeViewFiles.length > 0 || (!selectedTurn && selectedGitSource?.files?.length) ? ( - )} + ) : null} {canRefreshGitDiff && ( } > - + - {branchDiffPreview.isPending ? "Refreshing diff…" : "Refresh diff"} + {isRefreshingDiff ? "Refreshing diff…" : "Refresh diff"} )} - {codeViewFiles.length > 0 && ( + {diffFileKeys.length > 0 && ( - {codeViewFiles.length > 0 && ( + {diffFileKeys.length > 0 && (
- {isSelectedPatchTruncated && ( + {isSelectedPatchTruncated && !lazySource && (

- This diff was truncated because it exceeded the preview limit. The changes shown are - incomplete. + This preview exceeds the size limit. Changes shown are incomplete. + {selectedGitSource?.files ? " Totals include all changes." : ""}

)} {selectedPatchError && !renderablePatch && ( @@ -919,7 +1019,7 @@ export default function DiffPanel({

{selectedPatchError}

)} - {!renderablePatch ? ( + {!renderablePatch && !lazySource ? ( isLoadingSelectedPatch ? (
) - ) : renderablePatch.kind === "files" ? ( + ) : lazySource || renderablePatch?.kind === "files" ? (
node instanceof HTMLElement && node.hasAttribute("data-title"), ); - const filePath = title?.textContent?.trim(); + const filePath = title?.textContent; // The filename remains the explicit "open in editor" affordance. if (filePath) { openDiffFile(filePath); @@ -967,9 +1067,7 @@ export default function DiffPanel({ (node): node is HTMLElement => node instanceof HTMLElement && node.hasAttribute("data-diffs-header"), ); - const headerFilePath = header - ?.querySelector("[data-title]") - ?.textContent?.trim(); + const headerFilePath = header?.querySelector("[data-title]")?.textContent; if (!headerFilePath) return; const file = codeViewFiles.find( (candidate) => candidate.filePath === headerFilePath, @@ -980,16 +1078,43 @@ export default function DiffPanel({ ( - - )} - renderHeaderPrefix={(fileDiff, fileKey, collapsed) => { + renderHeaderFilenameSuffix={(fileDiff) => { + const path = resolveFileDiffPath(fileDiff); + const stat = fileStats.get(path); + return ( + <> + + {stat ? ( + retry(path)} /> + ) : null} + + ); + }} + {...(lazySource + ? { + unsafeCSSExtra: + "[data-additions-count], [data-deletions-count] { display: none; }", + renderHeaderMetadata: (fileDiff: FileDiffMetadata) => { + const stat = fileStats.get(resolveFileDiffPath(fileDiff)); + return stat ? ( + + ) : null; + }, + } + : {})} + renderHeaderPrefix={(fileDiff, fileKey) => { + const unavailable = fileDiff.cacheKey?.endsWith(":pending") === true; + const collapsed = unavailable || collapsedDiffFileKeys.has(fileKey); const filePath = resolveFileDiffPath(fileDiff); return ( @@ -1006,6 +1131,7 @@ export default function DiffPanel({ collapsed ? `Expand ${filePath}` : `Collapse ${filePath}` } aria-expanded={!collapsed} + disabled={unavailable} onClick={(event) => { event.stopPropagation(); toggleDiffFileCollapsed(fileKey); @@ -1052,7 +1178,9 @@ export default function DiffPanel({ ) : (
-

{renderablePatch.reason}

+

+ {renderablePatch?.kind === "raw" ? renderablePatch.reason : null} + 

-                    {renderablePatch.text}
+                    {renderablePatch?.kind === "raw" ? renderablePatch.text : null}
diff --git a/apps/web/src/components/DiffPanelShell.tsx b/apps/web/src/components/DiffPanelShell.tsx index 6ca4ff1b4983..7bc4ad7acfb7 100644 --- a/apps/web/src/components/DiffPanelShell.tsx +++ b/apps/web/src/components/DiffPanelShell.tsx @@ -46,7 +46,7 @@ export function DiffPanelShell(props: { ); } -function DiffFileHeaderSkeleton({ titleClassName }: { titleClassName: string }) { +export function DiffFileHeaderSkeleton({ titleClassName }: { titleClassName: string }) { return (
diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index b8ace2340557..253a0f3d215e 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -86,6 +86,9 @@ interface AnnotatableCodeViewProps { options: StyledDiffCodeViewOptions; viewerRef?: Ref; className?: string; + renderCodeViewFooter?: () => ReactNode; + unsafeCSSExtra?: string; + renderHeaderMetadata?: (fileDiff: FileDiffMetadata) => ReactNode; renderHeaderFilenameSuffix: (fileDiff: FileDiffMetadata) => ReactNode; renderHeaderPrefix: ( fileDiff: FileDiffMetadata, @@ -107,6 +110,9 @@ export function AnnotatableCodeView({ options, viewerRef, className, + renderCodeViewFooter, + unsafeCSSExtra, + renderHeaderMetadata, renderHeaderFilenameSuffix, renderHeaderPrefix, }: AnnotatableCodeViewProps) { @@ -245,6 +251,14 @@ export function AnnotatableCodeView({ key={codeViewKey} {...(viewerRef ? { viewerRef } : {})} {...(className ? { className } : {})} + {...(unsafeCSSExtra ? { unsafeCSSExtra } : {})} + {...(renderHeaderMetadata + ? { + renderHeaderMetadata: (item: CodeViewItem) => + item.type === "diff" ? renderHeaderMetadata(item.fileDiff) : null, + } + : {})} + {...(renderCodeViewFooter ? { renderCodeViewFooter } : {})} items={items} selectedLines={selectedLines} onSelectedLinesChange={setSelectedLines} diff --git a/apps/web/src/components/diffs/DiffFileLoadingBoundary.tsx b/apps/web/src/components/diffs/DiffFileLoadingBoundary.tsx new file mode 100644 index 000000000000..0ce87e142e2e --- /dev/null +++ b/apps/web/src/components/diffs/DiffFileLoadingBoundary.tsx @@ -0,0 +1,27 @@ +import { useEffect, useRef } from "react"; +import { DiffFileHeaderSkeleton } from "../DiffPanelShell"; + +/** Load the next batch before the reader reaches the end of the current files. */ +export function DiffFileLoadingBoundary({ load, count }: { load: () => void; count: number }) { + const ref = useRef(null); + useEffect(() => { + if (!ref.current) return; + const observer = new IntersectionObserver( + (entries) => { + if (entries.some((entry) => entry.isIntersecting)) load(); + }, + { rootMargin: "600px" }, + ); + observer.observe(ref.current); + return () => observer.disconnect(); + }, [load]); + return ( +
+ {Array.from({ length: Math.min(count, 4) }, (_, index) => ( +
+ +
+ ))} +
+ ); +} diff --git a/apps/web/src/components/diffs/DiffFileStatus.tsx b/apps/web/src/components/diffs/DiffFileStatus.tsx new file mode 100644 index 000000000000..c37d92789c87 --- /dev/null +++ b/apps/web/src/components/diffs/DiffFileStatus.tsx @@ -0,0 +1,39 @@ +import { InfoIcon, RotateCwIcon } from "lucide-react"; +import { Button } from "../ui/button"; +import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; + +export function DiffFileStatus({ + error, + truncated, + retry, +}: { + error?: boolean | undefined; + truncated?: boolean | undefined; + retry: () => void; +}) { + if (!error && !truncated) return null; + return ( + + { + event.stopPropagation(); + if (error) retry(); + }} + /> + } + > + {error ? : } + + + {error + ? "Retry loading diff" + : "This file is too large to show in full. Counts include all changes."} + + + ); +} diff --git a/apps/web/src/components/diffs/diffFileTree.logic.test.ts b/apps/web/src/components/diffs/diffFileTree.logic.test.ts index b21deedb8e54..4aa1fd7208b1 100644 --- a/apps/web/src/components/diffs/diffFileTree.logic.test.ts +++ b/apps/web/src/components/diffs/diffFileTree.logic.test.ts @@ -11,21 +11,21 @@ import { } from "./diffFileTree.logic"; function file(type: FileDiffMetadata["type"], name: string, prevName = name): FileDiffMetadata { - return { type, name: `b/${name}`, prevName: `a/${prevName}` } as FileDiffMetadata; + return { type, name, prevName } as FileDiffMetadata; } describe("diffFileTreeEntries", () => { it("maps each change type to its git status under the file's current path", () => { expect( diffFileTreeEntries([ - file("new", "src/a.ts"), + file("new", "a/src/a.ts"), file("deleted", "src/b.ts"), file("rename-pure", "src/c.ts", "src/old-c.ts"), file("rename-changed", "src/d.ts", "src/old-d.ts"), file("change", "README.md"), ]), ).toEqual([ - { path: "src/a.ts", status: "added" }, + { path: "a/src/a.ts", status: "added" }, { path: "src/b.ts", status: "deleted" }, { path: "src/c.ts", status: "renamed" }, { path: "src/d.ts", status: "renamed" }, diff --git a/apps/web/src/components/diffs/useCodeViewFileReveal.ts b/apps/web/src/components/diffs/useCodeViewFileReveal.ts index 6107c524c7b1..2f3746f8454d 100644 --- a/apps/web/src/components/diffs/useCodeViewFileReveal.ts +++ b/apps/web/src/components/diffs/useCodeViewFileReveal.ts @@ -8,7 +8,11 @@ interface FileRevealHandle { // Wait for a mounted viewer and expanded rows, then apply each tree click once. // Keep scope stable until the diff or external file selection changes. -export function useCodeViewFileReveal(viewer: FileRevealHandle | null, scope: TScope) { +export function useCodeViewFileReveal( + viewer: FileRevealHandle | null, + scope: TScope, + readyFileKeys?: ReadonlyArray, +) { const [request, setRequest] = useState<{ fileKey: string; scope: TScope } | null>(null); const handledRequest = useRef(null); @@ -18,11 +22,12 @@ export function useCodeViewFileReveal(viewer: FileRevealHandle | null, s handledRequest.current = request; return; } - if (!viewer?.getInstance()) return; + if (!viewer?.getInstance() || (readyFileKeys && !readyFileKeys.includes(request.fileKey))) + return; viewer.scrollTo({ type: "item", id: request.fileKey, align: "start" }); handledRequest.current = request; - }, [request, scope, viewer]); + }, [request, scope, viewer, readyFileKeys]); return useCallback((fileKey: string) => setRequest({ fileKey, scope }), [scope]); } diff --git a/apps/web/src/components/diffs/useReviewFilePatches.ts b/apps/web/src/components/diffs/useReviewFilePatches.ts new file mode 100644 index 000000000000..8ac353e86be8 --- /dev/null +++ b/apps/web/src/components/diffs/useReviewFilePatches.ts @@ -0,0 +1,229 @@ +import { RegistryContext, useAtomValue } from "@effect/atom-react"; +import type { FileDiffMetadata } from "@pierre/diffs"; +import type { EnvironmentId, ReviewDiffPreviewSource } from "@t3tools/contracts"; +import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; +import * as Atom from "effect/unstable/reactivity/Atom"; +import { useCallback, useContext, useEffect, useMemo, useRef, useState } from "react"; +import { getRenderablePatch, resolveFileDiffPath, type RenderablePatch } from "~/lib/diffRendering"; +import { reviewEnvironment } from "~/state/review"; + +export function useReviewFilePatches({ + environmentId, + cwd, + source, + baseRef, + ignoreWhitespace, + theme, + revision, + preview, +}: { + environmentId: EnvironmentId | undefined; + cwd: string | undefined; + source: ReviewDiffPreviewSource | null; + baseRef: string | null; + ignoreWhitespace: boolean; + theme: "light" | "dark"; + revision: string | undefined; + preview: RenderablePatch | null; +}) { + const registry = useContext(RegistryContext); + const scope = JSON.stringify([ + environmentId, + cwd, + source?.kind, + source?.diffHash, + baseRef, + ignoreWhitespace, + ]); + const [requested, setRequested] = useState({ scope, indices: [0, 1, 2, 3] }); + const indices = useMemo( + () => (requested.scope === scope ? requested.indices : [0, 1, 2, 3]), + [requested, scope], + ); + const files = useMemo( + () => + source?.files?.toSorted((a, b) => + a.path.localeCompare(b.path, undefined, { numeric: true, sensitivity: "base" }), + ) ?? [], + [source?.files], + ); + const queries = useMemo( + () => + !environmentId || !cwd || !source + ? [] + : indices + .filter((index) => index < files.length) + .map((index) => { + const file = files[index]!; + return { + index, + query: reviewEnvironment.diffFilePatch({ + environmentId, + input: { + cacheKey: scope, + request: { + cwd, + ...(baseRef ? { baseRef } : {}), + ignoreWhitespace, + file: { + path: file.path, + previousPath: file.previousPath, + sourceKind: source.kind, + }, + }, + }, + }), + }; + }), + [environmentId, cwd, source, files, indices, scope, baseRef, ignoreWhitespace], + ); + const previousPreview = useRef({ scope, revision, queries: [] as typeof queries }); + useEffect(() => { + const previous = previousPreview.current; + previousPreview.current = { scope, revision, queries }; + for (const { query } of queries) { + const changed = previous.scope === scope && previous.revision !== revision; + const cached = + !previous.queries.some((entry) => entry.query === query) && + registry.get(query)._tag !== "Initial" && + !registry.get(query).waiting; + if (changed || cached) registry.refresh(query); + } + }, [scope, revision, queries, registry]); + // Derived atoms parse each query result once, even when another file finishes loading. + const parsedQuery = useMemo( + () => + Atom.family((query: ReturnType) => + Atom.map(query, (result) => + AsyncResult.map(result, (source) => { + let patch = getRenderablePatch(source.diff, `diff-panel:${theme}`, { + compactPartialHunkOffsets: true, + }); + if (patch?.kind === "files" && patch.files.length === 1 && source.files?.length === 1) { + const stat = source.files[0]!; + const file = { ...patch.files[0]!, name: stat.path }; + if (stat.previousPath !== null) file.prevName = stat.previousPath; + else delete file.prevName; + patch = { ...patch, files: [file] }; + } + return { source, patch }; + }), + ), + ), + [theme], + ); + const patches = useAtomValue( + useMemo( + () => + Atom.make( + (get) => new Map(queries.map(({ index, query }) => [index, get(parsedQuery(query))])), + ), + [queries, parsedQuery], + ), + ); + const pendingIndex = files.findIndex((_, index) => { + const patch = patches.get(index); + return !patch || patch._tag === "Initial"; + }); + const settledFileCount = source + ? pendingIndex < 0 + ? files.length + : pendingIndex + : preview?.kind === "files" + ? preview.files.length + : 0; + const requestFiles = useCallback( + (indices: number[]) => + setRequested((current) => { + const previous = current.scope === scope ? current.indices : [0, 1, 2, 3]; + const added = indices.filter((index) => !previous.includes(index)); + return added.length === 0 ? current : { scope, indices: [...previous, ...added] }; + }), + [scope], + ); + const loadNextFiles = useCallback( + () => requestFiles(Array.from({ length: 4 }, (_, index) => settledFileCount + index)), + [requestFiles, settledFileCount], + ); + const requestFile = useCallback((index: number) => requestFiles([index]), [requestFiles]); + const retry = useCallback( + (path: string) => { + const query = queries.find(({ index }) => files[index]?.path === path)?.query; + if (query) registry.refresh(query); + }, + [queries, files, registry], + ); + const renderableFiles = useMemo( + () => + source + ? files.map((file, index): FileDiffMetadata => { + const result = patches.get(index); + if (result?._tag === "Success" && result.value.patch?.kind === "files") { + const loaded = result.value.patch.files.find( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ); + if (loaded) return loaded; + } + return { + name: file.path, + ...(file.previousPath ? { prevName: file.previousPath } : {}), + type: file.previousPath ? "rename-changed" : "change", + hunks: [], + additionLines: [], + deletionLines: [], + splitLineCount: 0, + unifiedLineCount: 0, + isPartial: true, + cacheKey: `${scope}:${file.path}:pending`, + }; + }) + : (preview?.kind === "files" ? preview.files : []).toSorted((a, b) => + resolveFileDiffPath(a).localeCompare(resolveFileDiffPath(b), undefined, { + numeric: true, + sensitivity: "base", + }), + ), + [source, files, patches, scope, preview], + ); + const fileStates = new Map( + files.map((file, index) => { + const patch = patches.get(index); + return [ + file.path, + { + error: + patch?._tag === "Failure" || + (patch?._tag === "Success" && + (patch.value.patch?.kind !== "files" || + !patch.value.patch.files.some( + (candidate) => resolveFileDiffPath(candidate) === file.path, + ))), + truncated: patch?._tag === "Success" && patch.value.source.truncated, + }, + ] as const; + }), + ); + const readyFilePaths = useMemo( + () => + new Set( + files + .filter((_, index) => { + const patch = patches.get(index); + return patch && patch._tag !== "Initial"; + }) + .map((file) => file.path), + ), + [files, patches], + ); + return { + scope, + fileStates, + isPending: [...patches.values()].some((patch) => patch._tag === "Initial" || patch.waiting), + retry, + requestFile, + readyFilePaths, + renderableFiles, + settledFileCount, + loadNextFiles, + }; +} diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index b4a8e898d7bf..203ff37c8886 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -18,8 +18,8 @@ const SOURCE = { function fileDiff(type: FileDiffMetadata["type"] = "rename-changed"): FileDiffMetadata { return { type, - prevName: "a/src/old-name.ts", - name: "b/src/new-name.ts", + prevName: "src/old-name.ts", + name: "src/new-name.ts", } as FileDiffMetadata; } diff --git a/apps/web/src/lib/diffRendering.test.ts b/apps/web/src/lib/diffRendering.test.ts index 6ba83cc7a9a4..33d015d52d32 100644 --- a/apps/web/src/lib/diffRendering.test.ts +++ b/apps/web/src/lib/diffRendering.test.ts @@ -6,6 +6,8 @@ import { buildPatchCacheKey, getDiffLineStat, getRenderablePatch, + resolveFileDiffPath, + resolveFileDiffPreviousPath, } from "./diffRendering"; describe("buildPatchCacheKey", () => { @@ -32,6 +34,34 @@ describe("buildPatchCacheKey", () => { }); describe("getRenderablePatch", () => { + it.each([ + ["a/example.ts", "a/example.ts"], + ["b/example.ts", "b/example.ts"], + ["a/before.ts", "b/after.ts"], + ])("preserves repository paths from %s to %s", (previousPath, path) => { + const parsed = getRenderablePatch( + [ + `diff --git a/${previousPath} b/${path}`, + ...(previousPath === path + ? [] + : ["similarity index 50%", `rename from ${previousPath}`, `rename to ${path}`]), + `--- a/${previousPath}`, + `+++ b/${path}`, + "@@ -1 +1 @@", + "-before", + "+after", + ].join("\n"), + ); + expect(parsed?.kind).toBe("files"); + if (parsed?.kind !== "files") return; + const file = parsed.files[0]; + expect(file).toBeDefined(); + if (!file) return; + expect(resolveFileDiffPath(file)).toBe(path); + expect(resolveFileDiffPreviousPath(file)).toBe(previousPath); + expect(buildFileDiffIdentityKey(file)).toBe(`${previousPath}\0${path}`); + }); + it("compacts partial hunk render offsets for virtualized review diffs", () => { const patch = [ "diff --git a/example.ts b/example.ts", diff --git a/apps/web/src/lib/diffRendering.ts b/apps/web/src/lib/diffRendering.ts index 90c456aa9da1..2af48db53a8f 100644 --- a/apps/web/src/lib/diffRendering.ts +++ b/apps/web/src/lib/diffRendering.ts @@ -144,11 +144,7 @@ export function getRenderablePatch( } export function resolveFileDiffPath(fileDiff: FileDiffMetadata): string { - const raw = fileDiff.name ?? fileDiff.prevName ?? ""; - if (raw.startsWith("a/") || raw.startsWith("b/")) { - return raw.slice(2); - } - return raw; + return fileDiff.name ?? fileDiff.prevName ?? ""; } /** @@ -156,11 +152,7 @@ export function resolveFileDiffPath(fileDiff: FileDiffMetadata): string { * path, and the hosts that resolve a diff position against both sides need both names. */ export function resolveFileDiffPreviousPath(fileDiff: FileDiffMetadata): string { - const raw = fileDiff.prevName ?? fileDiff.name ?? ""; - if (raw.startsWith("a/") || raw.startsWith("b/")) { - return raw.slice(2); - } - return raw; + return fileDiff.prevName ?? fileDiff.name ?? ""; } export function buildFileDiffIdentityKey(fileDiff: FileDiffMetadata): string { diff --git a/packages/client-runtime/src/state/review.ts b/packages/client-runtime/src/state/review.ts index ddd13db834bf..24fe3fb6fa76 100644 --- a/packages/client-runtime/src/state/review.ts +++ b/packages/client-runtime/src/state/review.ts @@ -1,9 +1,17 @@ -import { WS_METHODS } from "@t3tools/contracts"; +import { + type ReviewDiffPreviewInput, + VcsUnsupportedOperationError, + WS_METHODS, +} from "@t3tools/contracts"; +import * as Effect from "effect/Effect"; +import * as Semaphore from "effect/Semaphore"; +import { request } from "../rpc/client.ts"; import { Atom } from "effect/unstable/reactivity"; import { createAtomCommandScheduler, createEnvironmentRpcCommand, + createEnvironmentQueryAtomFamily, createEnvironmentRpcQueryAtomFamily, } from "./runtime.ts"; import type { EnvironmentRegistry } from "../connection/registry.ts"; @@ -11,6 +19,7 @@ import type { EnvironmentRegistry } from "../connection/registry.ts"; export function createReviewEnvironmentAtoms( runtime: Atom.AtomRuntime, ) { + const patchReads = Semaphore.makeUnsafe(4); const diffFileScheduler = createAtomCommandScheduler(); return { diffPreview: createEnvironmentRpcQueryAtomFamily(runtime, { @@ -18,6 +27,31 @@ export function createReviewEnvironmentAtoms( tag: WS_METHODS.reviewGetDiffPreview, staleTimeMs: 5_000, }), + diffFilePatch: createEnvironmentQueryAtomFamily(runtime, { + label: "environment-data:review:diff-file-patch", + staleTimeMs: 5 * 60_000, + execute: (input: { + request: ReviewDiffPreviewInput & { file: NonNullable }; + cacheKey: string; + }) => + request(WS_METHODS.reviewGetDiffPreview, input.request).pipe( + patchReads.withPermit, + Effect.flatMap((result) => { + const source = result.sources.find( + (source) => source.kind === input.request.file.sourceKind, + ); + return source + ? Effect.succeed(source) + : Effect.fail( + new VcsUnsupportedOperationError({ + operation: "review.diffFilePatch", + kind: "git", + detail: "Diff no longer available. Refresh the comparison.", + }), + ); + }), + ), + }), diffFileContents: createEnvironmentRpcCommand(runtime, { label: "environment-data:review:diff-file-contents", tag: WS_METHODS.reviewGetDiffFileContents, diff --git a/packages/contracts/src/review.ts b/packages/contracts/src/review.ts index 93f100b5fbf6..eee17de4e6f7 100644 --- a/packages/contracts/src/review.ts +++ b/packages/contracts/src/review.ts @@ -7,12 +7,27 @@ export const ReviewDiffPreviewInput = Schema.Struct({ cwd: TrimmedNonEmptyString, baseRef: Schema.optional(TrimmedNonEmptyString), ignoreWhitespace: Schema.optionalKey(Schema.Boolean), + file: Schema.optionalKey( + Schema.Struct({ + path: Schema.NonEmptyString, + previousPath: Schema.NullOr(Schema.NonEmptyString), + sourceKind: Schema.Literals(["working-tree", "branch-range"]), + }), + ), }); export type ReviewDiffPreviewInput = typeof ReviewDiffPreviewInput.Type; export const ReviewDiffPreviewSourceKind = Schema.Literals(["working-tree", "branch-range"]); export type ReviewDiffPreviewSourceKind = typeof ReviewDiffPreviewSourceKind.Type; +export const ReviewDiffFileStat = Schema.Struct({ + path: Schema.String, + previousPath: Schema.NullOr(Schema.String), + additions: Schema.Number, + deletions: Schema.Number, +}); +export type ReviewDiffFileStat = typeof ReviewDiffFileStat.Type; + export const ReviewDiffPreviewSource = Schema.Struct({ id: TrimmedNonEmptyString, kind: ReviewDiffPreviewSourceKind, @@ -22,6 +37,8 @@ export const ReviewDiffPreviewSource = Schema.Struct({ diff: Schema.String, diffHash: TrimmedNonEmptyString, truncated: Schema.Boolean, + /** Complete statistics, independent of patch limits. Absent on older servers. */ + files: Schema.optionalKey(Schema.Array(ReviewDiffFileStat)), }); export type ReviewDiffPreviewSource = typeof ReviewDiffPreviewSource.Type;