diff --git a/apps/web/src/DiffWorkspace.tsx b/apps/web/src/DiffWorkspace.tsx index db9cc671..6bbd072f 100644 --- a/apps/web/src/DiffWorkspace.tsx +++ b/apps/web/src/DiffWorkspace.tsx @@ -698,6 +698,26 @@ export function DiffWorkspace() { +

{selectionFeedback}

diff --git a/apps/web/src/use-diff.ts b/apps/web/src/use-diff.ts index 25b1e2a0..379456ce 100644 --- a/apps/web/src/use-diff.ts +++ b/apps/web/src/use-diff.ts @@ -8,12 +8,20 @@ import { api, errorDetail, type ApiChangedFile as ChangedFile, + type ApiFilePatch as FilePatch, type ApiRepositoryDiff as RepositoryDiff, } from "@diffx/api"; import type { DiffMode } from "./diff-mode.ts"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { CommentAnnotation } from "./review-model.ts"; import { languageOverride } from "./display-options.ts"; +import { filesInTreeOrder } from "./file-tree.ts"; + +// Each load or "Load more" places previews in file order until the next one +// would pass this many bytes, bounding browser memory per step. +const previewWindowBytes = 16 * 1024 * 1024; +const notLoadedMessage = + "Not loaded: 16 MiB of previews loaded. Use Load more to continue."; async function getDiff(contextId: string, mode: DiffMode, signal: AbortSignal) { const { data, error } = await api.GET( @@ -42,12 +50,17 @@ interface Preview { additions: number; deletions: number; } +type Fetched = + | { ok: true; bytes: number; preview: Preview } + | { ok: false; item: CodeViewItem }; interface DiffState { repository: RepositoryDiff | null; items: Map>; busy: boolean; notice: string; connected: boolean; + /** Files after the last loaded preview, waiting for Load more. */ + unloaded: number; } const initialState: DiffState = { repository: null, @@ -55,8 +68,70 @@ const initialState: DiffState = { busy: false, notice: "", connected: false, + unloaded: 0, }; +function buildPreview( + file: ChangedFile, + body: FilePatch, + cacheKey: string, +): Preview { + const contents = body.contents ?? null; + const parsed = + contents && body.message === null + ? parseDiffFromFile( + { name: file.path, contents: contents.before }, + { name: file.path, contents: contents.after }, + ) + : body.patch + ? parsePatchFiles(body.patch, file.fingerprint, true).flatMap( + (patch) => patch.files, + )[0] + : undefined; + if (parsed && parsed.hunks.length > 0) { + const firstLine = + parsed.hunks[0]?.additionStart === 1 + ? parsed.additionLines[0] + : parsed.hunks[0]?.deletionStart === 1 + ? parsed.deletionLines[0] + : undefined; + const language = languageOverride(file.path, firstLine); + const displayDiff = language + ? setLanguageOverride(parsed, language) + : parsed; + // Reuse worker highlighting across navigation and scope reloads. + displayDiff.cacheKey = cacheKey; + displayDiff.name = file.path; + if (file.oldPath) displayDiff.prevName = file.oldPath; + return { + fingerprint: file.fingerprint, + item: { id: file.path, type: "diff", fileDiff: displayDiff }, + additions: displayDiff.hunks.reduce( + (sum, hunk) => sum + hunk.additionLines, + 0, + ), + deletions: displayDiff.hunks.reduce( + (sum, hunk) => sum + hunk.deletionLines, + 0, + ), + }; + } + return { + fingerprint: file.fingerprint, + item: messageItem( + file, + body.message ?? + (body.patch.includes("Binary files") + ? "Binary file changed. No text preview available." + : file.oldPath + ? `Renamed from ${file.oldPath}. No text changes.` + : "No text changes (empty file or file metadata changed)."), + ), + additions: file.additions, + deletions: file.deletions, + }; +} + // Observations are immutable. Scope changes abort stale requests; retries reload // the same stored snapshot and preserve successfully loaded previews. export function useDiff(contextId: string, mode: DiffMode, enabled = true) { @@ -65,13 +140,16 @@ export function useDiff(contextId: string, mode: DiffMode, enabled = true) { const [loadedKey, setLoadedKey] = useState(""); const cache = useRef(new Map()); const refreshRef = useRef<() => void>(() => {}); + const loadMoreRef = useRef<() => void>(() => {}); const refresh = useCallback(() => refreshRef.current(), []); + const loadMore = useCallback(() => loadMoreRef.current(), []); useEffect(() => { let disposed = false; let controller = new AbortController(); cache.current.clear(); setState(initialState); setLoadedKey(key); + loadMoreRef.current = () => {}; if (!enabled) { refreshRef.current = () => {}; return; @@ -81,6 +159,7 @@ export function useDiff(contextId: string, mode: DiffMode, enabled = true) { controller.abort(); controller = new AbortController(); const signal = controller.signal; + loadMoreRef.current = () => {}; setState((previous) => ({ ...previous, busy: true })); try { const data: RepositoryDiff = await getDiff(contextId, mode, signal); @@ -89,22 +168,41 @@ export function useDiff(contextId: string, mode: DiffMode, enabled = true) { for (const path of cache.current.keys()) if (!paths.has(path)) cache.current.delete(path); const items = new Map>(); + // Pierre requires one item object per rendered contents; files that + // return to "Loading" or "Not loaded" reuse their earlier item. + const messages = new Map>(); + function status(file: ChangedFile, message: string) { + const key = `${file.path}\n${message}`; + const item = messages.get(key) ?? messageItem(file, message); + messages.set(key, item); + items.set(file.path, item); + } const pending: ChangedFile[] = []; - for (const file of data.files) { + // Load in the viewer's file order. + for (const file of filesInTreeOrder(data.files)) { const cached = cache.current.get(file.path); if (cached?.fingerprint === file.fingerprint) { items.set(file.path, cached.item); file.additions = cached.additions; file.deletions = cached.deletions; } else { - items.set(file.path, messageItem(file, "Loading diff…")); + status(file, "Loading diff…"); pending.push(file); } } + // Fetches run ahead concurrently and render as they arrive. A window + // keeps the leading queued files whose previews fit its budget; a + // later preview shown early is withdrawn when an earlier one fills + // the window, and stays fetched for the next window. + let queue = pending; + const fetched = new Map(); let next = 0; + let cut = -1; + let loading = false; let failed = 0; - let loadedBytes = 0; + let windowFailed = 0; function publish(busy: boolean) { + const failures = failed + windowFailed; setState({ repository: { ...data, @@ -112,131 +210,116 @@ export function useDiff(contextId: string, mode: DiffMode, enabled = true) { }, items: new Map(items), busy, - notice: failed - ? `${failed} file previews could not load. Refresh to retry.` + notice: failures + ? `${failures} file previews could not load. Refresh to retry.` : "", connected: true, + unloaded: loading ? 0 : queue.length, }); } - publish(true); - async function loadNext() { - while (next < pending.length && !signal.aborted) { - const file = pending[next++]; - if (!file) continue; - try { - const { data: body, error } = await api.GET( - "/api/v2/contexts/{contextId}/diffs/{diffId}/files/{fileId}/patch", - { - params: { - path: { contextId, diffId: data.id, fileId: file.id }, - query: { - scope: mode, - versionId: data.versionId, - fileVersion: file.fingerprint, - }, + function layout() { + let bytes = 0; + cut = -1; + windowFailed = 0; + for (const [index, file] of queue.entries()) { + const result = fetched.get(file.path); + if (!result) status(file, "Loading diff…"); + else if (!result.ok) { + windowFailed++; + items.set(file.path, result.item); + } else if (bytes > 0 && bytes + result.bytes > previewWindowBytes) { + cut = index; + break; + } else { + bytes += result.bytes; + file.additions = result.preview.additions; + file.deletions = result.preview.deletions; + items.set(file.path, result.preview.item); + } + } + if (cut >= 0) + for (const file of queue.slice(cut)) status(file, notLoadedMessage); + } + async function fetchPreview(file: ChangedFile): Promise { + try { + const { data: body, error } = await api.GET( + "/api/v2/contexts/{contextId}/diffs/{diffId}/files/{fileId}/patch", + { + params: { + path: { contextId, diffId: data.id, fileId: file.id }, + query: { + scope: mode, + versionId: data.versionId, + fileVersion: file.fingerprint, }, - signal, }, - ); - if (!body) - throw new Error(errorDetail(error, "Unable to load diff")); - if (signal.aborted || disposed) return; - const contents = body.contents ?? null; - loadedBytes += - body.patch.length + - (contents?.before.length ?? 0) + - (contents?.after.length ?? 0); - let item: CodeViewItem; - if (loadedBytes > 16 * 1024 * 1024) { - item = messageItem( - file, - "Preview budget exceeded (16 MiB per refresh). Narrow the diff scope to load more.", - ); - } else { - const parsed = - contents && body.message === null - ? parseDiffFromFile( - { name: file.path, contents: contents.before }, - { name: file.path, contents: contents.after }, - ) - : body.patch - ? parsePatchFiles( - body.patch, - file.fingerprint, - true, - ).flatMap((patch) => patch.files)[0] - : undefined; - if (parsed && parsed.hunks.length > 0) { - const firstLine = - parsed.hunks[0]?.additionStart === 1 - ? parsed.additionLines[0] - : parsed.hunks[0]?.deletionStart === 1 - ? parsed.deletionLines[0] - : undefined; - const language = languageOverride(file.path, firstLine); - const displayDiff = language - ? setLanguageOverride(parsed, language) - : parsed; - // Reuse worker highlighting across navigation and scope reloads. - displayDiff.cacheKey = JSON.stringify([ - contextId, - data.id, - mode, - file.path, - file.fingerprint, - ]); - displayDiff.name = file.path; - if (file.oldPath) displayDiff.prevName = file.oldPath; - file.additions = displayDiff.hunks.reduce( - (sum, hunk) => sum + hunk.additionLines, - 0, - ); - file.deletions = displayDiff.hunks.reduce( - (sum, hunk) => sum + hunk.deletionLines, - 0, - ); - item = { - id: file.path, - type: "diff", - fileDiff: displayDiff, - }; - } else - item = messageItem( - file, - body.message ?? - (body.patch.includes("Binary files") - ? "Binary file changed. No text preview available." - : file.oldPath - ? `Renamed from ${file.oldPath}. No text changes.` - : "No text changes (empty file or file metadata changed)."), - ); - } - items.set(file.path, item); - cache.current.set(file.path, { - fingerprint: file.fingerprint, - item, - additions: file.additions, - deletions: file.deletions, - }); - } catch (error) { - if (signal.aborted || disposed) return; - failed++; - items.set( - file.path, - messageItem( - file, - error instanceof Error - ? error.message - : "Unable to load diff", - ), - ); - } + signal, + }, + ); + if (!body) + throw new Error(errorDetail(error, "Unable to load diff")); + const bytes = + body.patch.length + + (body.contents?.before.length ?? 0) + + (body.contents?.after.length ?? 0); + const cacheKey = JSON.stringify([ + contextId, + data.id, + mode, + file.path, + file.fingerprint, + ]); + return { + ok: true, + bytes, + preview: buildPreview(file, body, cacheKey), + }; + } catch (error) { + return { + ok: false, + item: messageItem( + file, + error instanceof Error ? error.message : "Unable to load diff", + ), + }; + } + } + async function fetchNext() { + while (cut < 0 && next < queue.length && !signal.aborted) { + const file = queue[next++]; + if (!file || fetched.has(file.path)) continue; + const result = await fetchPreview(file); + if (signal.aborted || disposed) return; + fetched.set(file.path, result); + layout(); publish(true); } } - await Promise.all(Array.from({ length: 4 }, loadNext)); - if (signal.aborted || disposed) return; - publish(false); + async function loadWindow() { + loading = true; + next = 0; + layout(); + publish(true); + await Promise.all(Array.from({ length: 4 }, fetchNext)); + loading = false; + if (signal.aborted || disposed) return; + // Claims stop at the cut, so every file before it has a result. + const end = cut < 0 ? queue.length : cut; + for (const file of queue.slice(0, end)) { + const result = fetched.get(file.path); + if (result?.ok) cache.current.set(file.path, result.preview); + fetched.delete(file.path); + } + failed += windowFailed; + windowFailed = 0; + queue = queue.slice(end); + publish(false); + } + loadMoreRef.current = () => { + if (!loading && queue.length > 0 && !signal.aborted && !disposed) + void loadWindow(); + }; + await loadWindow(); } catch (error) { if (signal.aborted || disposed) return; setState((previous) => ({ @@ -262,5 +345,8 @@ export function useDiff(contextId: string, mode: DiffMode, enabled = true) { }, [contextId, mode, enabled, key]); // Never expose the previous scope during the render before effect cleanup. const current = loadedKey === key ? state : initialState; - return useMemo(() => ({ ...current, refresh }), [current, refresh]); + return useMemo( + () => ({ ...current, refresh, loadMore }), + [current, refresh, loadMore], + ); } diff --git a/apps/web/test/browser/app.spec.ts b/apps/web/test/browser/app.spec.ts index 46ce055c..4c1f600e 100644 --- a/apps/web/test/browser/app.spec.ts +++ b/apps/web/test/browser/app.spec.ts @@ -2550,6 +2550,44 @@ test("file comments remain available without a text diff", async ({ page }) => { ).toContainText("Review this binary asset"); }); +test("previews load in file order 16 MiB at a time", async ({ page }) => { + // Each preview counts 6 MiB, so a 16 MiB window holds two files. + const padding = "a".repeat(6 * 1024 * 1024); + await page.route("**/files/*/patch?*", (route) => + route.fulfill({ + contentType: "application/json", + body: JSON.stringify({ + patch: "", + message: "Loaded preview", + contents: { before: padding, after: "" }, + }), + }), + ); + await page.setViewportSize({ width: 1280, height: 4_000 }); + await page.goto("/"); + const files = page.locator("#viewer diffs-container"); + const notice = page + .locator('[data-slot="notice"]') + .filter({ hasText: "not loaded" }); + // Several 6 MiB previews parse before the window pauses. + await expect(notice).toHaveText(/10 files not loaded/, { timeout: 15_000 }); + await expect(files).toHaveCount(12); + for (const [index, text] of [ + "Loaded preview", + "Loaded preview", + "Not loaded", + "Not loaded", + ].entries()) + await expect(files.nth(index)).toContainText(text); + await expect(files.nth(11)).toContainText("Not loaded"); + + await notice.getByRole("button", { name: "Load more" }).click(); + await expect(notice).toHaveText(/8 files not loaded/); + for (const index of [2, 3]) + await expect(files.nth(index)).toContainText("Loaded preview"); + await expect(files.nth(4)).toContainText("Not loaded"); +}); + test("diff file and folder collapse state survives reload within the tab", async ({ page, }) => { diff --git a/docs-website/src/pages/piped-diffs.md b/docs-website/src/pages/piped-diffs.md index 6419725c..e26618b3 100644 --- a/docs-website/src/pages/piped-diffs.md +++ b/docs-website/src/pages/piped-diffs.md @@ -21,5 +21,6 @@ combining redirected stdin with a path argument or `--path` is rejected. Collection does not inspect Git or attach repository identity. Piped diffs have only the all scope and do not include full file contents. Standard Git patches are limited to 16 MiB total -and 2 MiB per file. Combined merge diffs are shown against the first parent. +and 2 MiB per file; a file whose patch exceeds 1 MiB is listed without a +preview. Combined merge diffs are shown against the first parent. Use `git show --diff-merges=separate` to review every parent separately. diff --git a/docs/system.md b/docs/system.md index 86d90e23..6a5746eb 100644 --- a/docs/system.md +++ b/docs/system.md @@ -108,10 +108,17 @@ Uncommitted. Object-only branch recovery and piped patches retain only `all`, meaning Branch changes and supplied patch respectively. Every new manifest declares `comparisonKind`; old manifests receive a read-time interpretation without changing stored data. Unsupported scopes are unavailable, not empty. -The 16 MiB aggregate preview budget is divided equally across captured scopes; -this prevents one comparison consuming the others' previews, at the cost of -earlier truncation within a large scope. Full-content identity remains independent -of preview truncation. +Previews are bounded per file and per scope first: a patch over 1 MiB, or full +contents over 1 MiB on either side, are omitted with an explicit message, and +files after the first 1000 of a scope are listed without previews. Because an +observation is one ingestion request and one transaction, a total limit keeps +its encoded JSON within the 64 MiB request limit with 1 MiB of headroom. +Collection captures every scope's patches before any full contents, so full-file +views degrade before files lose their patch. Each pass splits the remaining total +equally between scopes in rounds, in file order, skipping a file that does not +fit. No comparison can consume another's share, and a share a scope does not need +goes to the others. Full-content identity remains independent of preview +truncation. Local composition calls `contextservice.Ingest` directly as each checkout finishes collection. Discovery feeds up to four concurrent Git reads, while one serial diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 60fb90e3..5a4a19af 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -21,8 +21,6 @@ import ( "github.com/google/uuid" ) -const previewBudget = 16 << 20 - type Options struct { SourceID string Hostname string @@ -145,19 +143,14 @@ func collectRepositoryChanged(ctx context.Context, source diffsource.Source, opt observation.ContentHash = contentHash fingerprint := Fingerprint(observation) unchanged := fingerprint != "" && fingerprint == previousFingerprint - for index, snapshot := range snapshots { - remaining := previewBudget / len(snapshots) - if unchanged { - break - } - scope, err := collectScope(ctx, source, snapshot, &remaining) + if !unchanged { + observation, err = withPreviews(ctx, source, observation, defaultPreviewLimits) if err != nil { if isChanged(err) { return ingestion.Request{}, "", errChanged } return ingestion.Request{}, "", err } - observation.Scopes[index] = scope } for _, snapshot := range snapshots { latest, err := source.Snapshot(ctx, snapshot.Mode) @@ -237,62 +230,6 @@ func Fingerprint(request ingestion.Request) string { return hash(string(encoded)) } -func collectScope(ctx context.Context, source diffsource.Source, snapshot review.RepositoryDiff, remaining *int) (ingestion.Scope, error) { - patches := make(map[string]review.FilePatch, len(snapshot.Files)) - for _, file := range snapshot.Files { - if err := ctx.Err(); err != nil { - return ingestion.Scope{}, err - } - if *remaining == 0 { - patches[file.ID] = review.FilePatch{Message: diffsource.Message("Comparison exceeds its share of the 16 MiB aggregate preview limit.")} - continue - } - patch, err := source.Patch(ctx, snapshot.Mode, file, snapshot.Head) - if err != nil { - return ingestion.Scope{}, err - } - if source.Support().FileContents && patch.Contents == nil && !file.Binary && file.Status != "U" && !nonTextPreview(patch) { - contents, err := source.Contents(ctx, snapshot.Mode, file, snapshot.Head) - if err == nil { - patch.Contents = &contents - patch.Message = nil - } else if diffsource.PreviewUnavailable(err) { - if err := ctx.Err(); err != nil { - return ingestion.Scope{}, err - } - if patch.Message == nil { - patch.Message = diffsource.Message("Full contents unavailable: file is binary or exceeds the preview limit.") - } - } else { - return ingestion.Scope{}, err - } - } else if !source.Support().FileContents && patch.Message == nil { - patch.Message = diffsource.Message("Full contents unavailable for this snapshot.") - } - applyBudget(&patch, remaining) - patches[file.ID] = patch - } - return ingestion.Scope{Snapshot: snapshot, Patches: patches}, nil -} - -func applyBudget(patch *review.FilePatch, remaining *int) { - size := len(patch.Patch) - if patch.Contents != nil { - size += len(patch.Contents.Before) + len(patch.Contents.After) - } - if size > *remaining { - patch.Contents = nil - patch.Message = diffsource.Message("Full contents unavailable: comparison exceeds its share of the 16 MiB aggregate preview limit.") - size = len(patch.Patch) - if size > *remaining { - patch.Patch = "" - patch.Message = diffsource.Message("Comparison exceeds its share of the 16 MiB aggregate preview limit.") - size = 0 - } - } - *remaining -= size -} - func nonTextPreview(patch review.FilePatch) bool { if patch.Message == nil { return false @@ -330,14 +267,9 @@ func CollectPatch(ctx context.Context, raw, directory string, options Options) ( return ingestion.Request{}, err } } - remaining := previewBudget - scope, err := collectScope(ctx, source, snapshot, &remaining) - if err != nil { - return ingestion.Request{}, err - } - observation := request(metadata, []ingestion.Scope{scope}, options) + observation := request(metadata, []ingestion.Scope{{Snapshot: snapshot}}, options) observation.ContentHash = hash("piped-content-v1", raw, string(snapshot.ComparisonKind)) - return observation, nil + return withPreviews(ctx, source, observation, defaultPreviewLimits) } func request(metadata ingestion.Metadata, scopes []ingestion.Scope, options Options) ingestion.Request { diff --git a/internal/collector/collector_test.go b/internal/collector/collector_test.go index d3fceca1..2b9021c5 100644 --- a/internal/collector/collector_test.go +++ b/internal/collector/collector_test.go @@ -202,19 +202,6 @@ func TestBinaryAndOversizeContentsRemainExplicit(t *testing.T) { } } -func TestPreviewBudgetPreservesPatchWhenContentsDoNotFit(t *testing.T) { - patch := review.FilePatch{Patch: "patch", Contents: &review.FileContents{Before: "old contents", After: "new contents"}} - remaining := 8 - applyBudget(&patch, &remaining) - if patch.Patch != "patch" || patch.Contents != nil || patch.Message == nil || remaining != 3 { - t.Fatalf("budget fallback: %+v remaining %d", patch, remaining) - } - applyBudget(&patch, &remaining) - if patch.Patch != "" || patch.Message == nil || remaining != 3 { - t.Fatalf("budget unavailable: %+v remaining %d", patch, remaining) - } -} - type changingSource struct { diffsource.Source calls int @@ -350,7 +337,7 @@ func TestContentIdentityIncludesUnpreviewableBytes(t *testing.T) { } } -func TestContentIdentityIncludesFilesAfterAggregatePreviewBudget(t *testing.T) { +func TestContentIdentityIncludesFilesOverPreviewLimits(t *testing.T) { root := repository(t) value := strings.Repeat("a", 3<<19) for _, name := range []string{"a", "b", "c", "d", "e", "f", "g", "z-last"} { @@ -359,7 +346,7 @@ func TestContentIdentityIncludesFilesAfterAggregatePreviewBudget(t *testing.T) { first := collect(t, root) preview := patchFor(t, first, review.DiffAll, "z-last") if preview.Patch != "" || preview.Contents != nil || preview.Message == nil { - t.Fatalf("test must exhaust aggregate budget before last file: %+v", preview) + t.Fatalf("test must omit the last file's preview: %+v", preview) } write(t, root, "z-last", value[:len(value)-1]+"b") second := collect(t, root) diff --git a/internal/collector/previews.go b/internal/collector/previews.go new file mode 100644 index 00000000..9c18a1ee --- /dev/null +++ b/internal/collector/previews.go @@ -0,0 +1,228 @@ +package collector + +import ( + "context" + "encoding/json" + "slices" + + "github.com/flexdinesh/diffx/internal/diffsource" + "github.com/flexdinesh/diffx/internal/ingestion" + "github.com/flexdinesh/diffx/internal/review" +) + +// previewLimits bound one observation's previews. Per-file and file-count +// limits are primary; requestBytes is a safety net measured on the encoded +// request so every observation fits one ingestion request. +type previewLimits struct { + requestBytes int + patchBytes int + contentsBytes int + files int +} + +var defaultPreviewLimits = previewLimits{ + requestBytes: ingestion.MaxRequestBytes - 1<<20, + patchBytes: 1 << 20, + contentsBytes: 1 << 20, + files: 1000, +} + +var ( + overBudgetMessage = "Comparison exceeds the preview size limit." + contentsOverBudgetMessage = "Full contents unavailable: comparison exceeds the preview size limit." +) + +// previewCandidate is a captured patch that may still receive full contents; +// base is its preview before the contents budget fallback message. +type previewCandidate struct { + position int + file review.ChangedFile + base review.FilePatch +} + +// withPreviews captures every scope's patches before any full contents, so +// full-file views degrade before files lose their patch. Each pass shares +// what remains of the request budget between scopes; see allocate. +func withPreviews(ctx context.Context, source diffsource.Source, request ingestion.Request, limits previewLimits) (ingestion.Request, error) { + scopes := make([]ingestion.Scope, len(request.Scopes)) + counts := make([]int, len(scopes)) + for index, scope := range request.Scopes { + patches := make(map[string]review.FilePatch, len(scope.Snapshot.Files)) + for position, file := range scope.Snapshot.Files { + patches[file.ID] = review.FilePatch{Message: diffsource.Message(overBudgetMessage)} + if position >= limits.files { + patches[file.ID] = review.FilePatch{Message: diffsource.Message("Not captured: comparison exceeds 1000 files.")} + } + } + scopes[index] = ingestion.Scope{Snapshot: scope.Snapshot, Patches: patches} + counts[index] = min(len(scope.Snapshot.Files), limits.files) + } + request.Scopes = scopes + skeleton, err := json.Marshal(request) + if err != nil { + return ingestion.Request{}, err + } + budget := max(limits.requestBytes-len(skeleton), 0) + candidates := make([][]previewCandidate, len(scopes)) + budget, err = allocate(counts, budget, func(index, position, limit int) (int, bool, error) { + if err := ctx.Err(); err != nil { + return 0, false, err + } + snapshot := scopes[index].Snapshot + file := snapshot.Files[position] + direct, err := source.Patch(ctx, snapshot.Mode, file, snapshot.Head) + if err != nil { + return 0, false, err + } + patch := direct + if len(patch.Patch) > limits.patchBytes { + patch = review.FilePatch{Message: diffsource.Message("Diff exceeds the 1 MiB preview limit.")} + } + wantsContents := source.Support().FileContents && patch.Contents == nil && !file.Binary && file.Status != "U" && !nonTextPreview(patch) + if patch.Contents != nil && !contentsFit(*patch.Contents, limits) { + patch.Contents, patch.Message = nil, diffsource.Message("Full contents unavailable: file exceeds the 1 MiB preview limit.") + } + base := patch + if wantsContents && patch.Message == nil { + patch.Message = diffsource.Message(contentsOverBudgetMessage) + } else if !source.Support().FileContents && patch.Message == nil { + patch.Message = diffsource.Message("Full contents unavailable for this snapshot.") + } + cost := growth(file.ID, scopes[index].Patches[file.ID], patch) + if cost > limit { + return cost, false, nil + } + scopes[index].Patches[file.ID] = patch + if wantsContents { + candidates[index] = append(candidates[index], previewCandidate{position: position, file: file, base: base}) + } + return cost, true, nil + }) + if err != nil { + return ingestion.Request{}, err + } + for index := range candidates { + slices.SortFunc(candidates[index], func(left, right previewCandidate) int { return left.position - right.position }) + counts[index] = len(candidates[index]) + } + _, err = allocate(counts, budget, func(index, position, limit int) (int, bool, error) { + if err := ctx.Err(); err != nil { + return 0, false, err + } + snapshot := scopes[index].Snapshot + candidate := candidates[index][position] + patch := candidate.base + contents, err := source.Contents(ctx, snapshot.Mode, candidate.file, snapshot.Head) + switch { + case err == nil && !contentsFit(contents, limits): + if patch.Message == nil { + patch.Message = diffsource.Message("Full contents unavailable: file exceeds the 1 MiB preview limit.") + } + case err == nil: + patch.Contents, patch.Message = &contents, nil + case diffsource.PreviewUnavailable(err): + if err := ctx.Err(); err != nil { + return 0, false, err + } + if patch.Message == nil { + patch.Message = diffsource.Message("Full contents unavailable: file is binary or exceeds the preview limit.") + } + default: + return 0, false, err + } + cost := growth(candidate.file.ID, scopes[index].Patches[candidate.file.ID], patch) + if cost > limit { + return cost, false, nil + } + scopes[index].Patches[candidate.file.ID] = patch + return cost, true, nil + }) + if err != nil { + return ingestion.Request{}, err + } + return request, nil +} + +// allocate spends budget on each scope's candidates in file order and returns +// what is left. Rounds split the remaining budget equally between scopes with +// candidates that might still fit, so no scope can consume another's share and +// a share a scope does not need goes to the others in later rounds. Within its +// share a scope skips a candidate that does not fit and continues. When no +// share fits anything, one shared pool gives the rest to scopes in order. +// try measures a candidate and keeps it when its cost fits limit; known costs +// avoid measuring a candidate again until it can fit. +func allocate(counts []int, budget int, try func(scope, position, limit int) (int, bool, error)) (int, error) { + costs := make([][]int, len(counts)) + taken := make([][]bool, len(counts)) + for index, count := range counts { + costs[index], taken[index] = make([]int, count), make([]bool, count) + for position := range costs[index] { + costs[index][position] = -1 + } + } + pooled := false + for { + active := make([]int, 0, len(counts)) + for index := range counts { + for position := range counts[index] { + if !taken[index][position] && costs[index][position] <= budget { + active = append(active, index) + break + } + } + } + if len(active) == 0 { + return budget, nil + } + share := budget / len(active) + progress := false + for _, index := range active { + limit := share + if pooled { + limit = budget + } + for position := range counts[index] { + if limit == 0 { + break + } + if taken[index][position] || costs[index][position] > limit { + continue + } + cost, kept, err := try(index, position, limit) + if err != nil { + return budget, err + } + costs[index][position] = cost + if kept { + taken[index][position], progress = true, true + limit -= cost + budget -= cost + } + } + } + if !progress { + if pooled || len(active) == 1 { + return budget, nil + } + pooled = true + } + } +} + +func contentsFit(contents review.FileContents, limits previewLimits) bool { + return len(contents.Before) <= limits.contentsBytes && len(contents.After) <= limits.contentsBytes +} + +// growth is the encoded request bytes replacing one file's preview adds. +func growth(id string, current, next review.FilePatch) int { + return max(encodedPatch(id, next)-encodedPatch(id, current), 0) +} + +// encodedPatch measures a file's patches map entry as json.Marshal encodes +// the request, including HTML escaping. Encoding strings and FilePatch values +// cannot fail. +func encodedPatch(id string, patch review.FilePatch) int { + key, _ := json.Marshal(id) + value, _ := json.Marshal(patch) + return len(key) + len(value) +} diff --git a/internal/collector/previews_test.go b/internal/collector/previews_test.go new file mode 100644 index 00000000..2b16de6b --- /dev/null +++ b/internal/collector/previews_test.go @@ -0,0 +1,212 @@ +package collector + +import ( + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/flexdinesh/diffx/internal/diffsource" + "github.com/flexdinesh/diffx/internal/ingestion" + "github.com/flexdinesh/diffx/internal/review" +) + +func encodedBytes(t *testing.T, request ingestion.Request) int { + t.Helper() + raw, err := json.Marshal(request) + if err != nil { + t.Fatal(err) + } + return len(raw) +} + +// skeletonBytes is the encoded request with every file's preview omitted. +func skeletonBytes(t *testing.T, request ingestion.Request) int { + t.Helper() + scopes := make([]ingestion.Scope, len(request.Scopes)) + for index, scope := range request.Scopes { + patches := make(map[string]review.FilePatch, len(scope.Patches)) + for id := range scope.Patches { + patches[id] = review.FilePatch{Message: diffsource.Message(overBudgetMessage)} + } + scopes[index] = ingestion.Scope{Snapshot: scope.Snapshot, Patches: patches} + } + request.Scopes = scopes + return encodedBytes(t, request) +} + +func recollect(t *testing.T, root string, request ingestion.Request, limits previewLimits) ingestion.Request { + t.Helper() + source, err := diffsource.OpenComparison(t.Context(), root, "auto", "") + if err != nil { + t.Fatal(err) + } + result, err := withPreviews(t.Context(), source, request, limits) + if err != nil { + t.Fatal(err) + } + return result +} + +func TestAllocateKeepsSharesAndRedistributesUnused(t *testing.T) { + for _, test := range []struct { + name string + costs [][]int + budget int + kept [][]bool + }{ + {"share bounds a large scope", [][]int{{10, 10, 10, 10}, {10}}, 20, [][]bool{{true, false, false, false}, {true}}}, + {"unused share moves on", [][]int{{30, 30}, {10}, {}}, 70, [][]bool{{true, true}, {true}}}, + {"misfit skipped in file order", [][]int{{40, 10, 10}}, 25, [][]bool{{false, true, true}}}, + {"pool fits what no share can", [][]int{{50}, {50}}, 60, [][]bool{{true}, {false}}}, + } { + t.Run(test.name, func(t *testing.T) { + counts := make([]int, len(test.costs)) + kept := make([][]bool, len(test.costs)) + for index, costs := range test.costs { + counts[index], kept[index] = len(costs), make([]bool, len(costs)) + } + left, err := allocate(counts, test.budget, func(scope, position, limit int) (int, bool, error) { + cost := test.costs[scope][position] + if cost > limit { + return cost, false, nil + } + kept[scope][position] = true + return cost, true, nil + }) + if err != nil { + t.Fatal(err) + } + spent := 0 + for index := range test.costs { + if len(test.kept) > index && len(test.kept[index]) > 0 && fmt.Sprint(kept[index]) != fmt.Sprint(test.kept[index]) { + t.Fatalf("scope %d kept %v, want %v", index, kept[index], test.kept[index]) + } + for position, cost := range test.costs[index] { + if kept[index][position] { + spent += cost + } + } + } + if left != test.budget-spent { + t.Fatalf("left %d after spending %d of %d", left, spent, test.budget) + } + }) + } +} + +func TestPreviewSharesKeepEveryComparisonAndRedistributeUnused(t *testing.T) { + root := repository(t) + writeFiles := func(value string, count int) { + for i := range count { + write(t, root, fmt.Sprintf("file-%d", i), strings.Repeat(value, 1<<14)+"\n") + } + } + writeFiles("a", 4) + git(t, root, "add", ".") + git(t, root, "commit", "-m", "base") + git(t, root, "switch", "-c", "feature") + writeFiles("b", 4) + git(t, root, "commit", "-am", "committed") + writeFiles("c", 1) + request := collect(t, root) + if len(request.Scopes) != 5 || len(scopeFor(t, request, review.DiffStaged).Snapshot.Files) != 0 { + t.Fatalf("fixture must capture five scopes with nothing staged: %+v", request.Scopes) + } + extra := 200 << 10 + limits := defaultPreviewLimits + limits.requestBytes = skeletonBytes(t, request) + extra + result := recollect(t, root, request, limits) + if size := encodedBytes(t, result); size > limits.requestBytes { + t.Fatalf("encoded request %d exceeds limit %d", size, limits.requestBytes) + } + for _, mode := range []review.DiffMode{review.DiffAll, review.DiffCommitted, review.DiffUncommitted, review.DiffUnstaged} { + if patchFor(t, result, mode, "file-0").Patch == "" { + t.Fatalf("%s starved by another scope", mode) + } + } + if patchFor(t, result, review.DiffAll, "file-1").Patch == "" { + t.Fatal("unused shares were not redistributed to the combined scope") + } + missing, withContents := false, false + for _, scope := range result.Scopes { + for _, patch := range scope.Patches { + missing = missing || patch.Patch == "" && patch.Contents == nil + withContents = withContents || patch.Contents != nil + } + } + if !missing || withContents { + t.Fatalf("full contents captured before every patch: missing %v, contents %v", missing, withContents) + } +} + +func TestPreviewFileAndCountLimits(t *testing.T) { + root := repository(t) + lines := make([]string, 100) + for i := range lines { + lines[i] = fmt.Sprintf("line %d", i) + } + write(t, root, "b", strings.Join(lines, "\n")+"\n") + git(t, root, "add", ".") + git(t, root, "commit", "-m", "base") + lines[0] = "changed" + write(t, root, "b", strings.Join(lines, "\n")+"\n") + write(t, root, "a", "small\n") + write(t, root, "c", strings.Repeat("c", 600)+"\n") + write(t, root, "d", "after the file limit\n") + request := collect(t, root) + result := recollect(t, root, request, previewLimits{requestBytes: defaultPreviewLimits.requestBytes, patchBytes: 512, contentsBytes: 512, files: 3}) + if patch := patchFor(t, result, review.DiffAll, "a"); patch.Patch == "" || patch.Contents == nil { + t.Fatalf("small file preview: %+v", patch) + } + if patch := patchFor(t, result, review.DiffAll, "b"); patch.Patch == "" || patch.Contents != nil || patch.Message == nil || !strings.HasPrefix(*patch.Message, "Full contents unavailable: file exceeds") { + t.Fatalf("large contents keep their patch: %+v", patch) + } + if patch := patchFor(t, result, review.DiffAll, "c"); patch.Patch != "" || patch.Contents != nil || patch.Message == nil || !strings.HasPrefix(*patch.Message, "Diff exceeds") { + t.Fatalf("oversize patch: %+v", patch) + } + if patch := patchFor(t, result, review.DiffAll, "d"); patch.Patch != "" || patch.Contents != nil || patch.Message == nil || *patch.Message != "Not captured: comparison exceeds 1000 files." { + t.Fatalf("file past the count limit: %+v", patch) + } +} + +func TestOversizePatchStillShowsFullContentsThatFit(t *testing.T) { + root := repository(t) + write(t, root, "c", strings.Repeat("c", 600)+"\n") + request := collect(t, root) + result := recollect(t, root, request, previewLimits{requestBytes: defaultPreviewLimits.requestBytes, patchBytes: 512, contentsBytes: 1024, files: 3}) + if patch := patchFor(t, result, review.DiffAll, "c"); patch.Patch != "" || patch.Contents == nil || patch.Message != nil { + t.Fatalf("full contents within limit: %+v", patch) + } +} + +func TestPreviewBudgetCountsEncodedBytes(t *testing.T) { + root := repository(t) + write(t, root, "markup", strings.Repeat("<>", 20000)+"\n") + write(t, root, "plain", "plain\n") + request := collect(t, root) + limits := defaultPreviewLimits + limits.requestBytes = skeletonBytes(t, request) + 100<<10 + result := recollect(t, root, request, limits) + if size := encodedBytes(t, result); size > limits.requestBytes { + t.Fatalf("encoded request %d exceeds limit %d", size, limits.requestBytes) + } + if patch := patchFor(t, result, review.DiffAll, "markup"); patch.Patch != "" || patch.Message == nil || *patch.Message != overBudgetMessage { + t.Fatalf("escaped markup must not fit by raw size: %+v", patch) + } + if patch := patchFor(t, result, review.DiffAll, "plain"); patch.Patch == "" || patch.Contents == nil { + t.Fatalf("later file kept after skipped one: %+v", patch) + } +} + +func TestPipedFilesOverPreviewLimitRemainAccepted(t *testing.T) { + raw := "diff --git a/large b/large\nnew file mode 100644\n--- /dev/null\n+++ b/large\n@@ -0,0 +1 @@\n+" + strings.Repeat("a", 3<<19) + "\n" + request, err := CollectPatch(t.Context(), raw, "", Options{SourceID: "source", Hostname: "host"}) + if err != nil { + t.Fatal(err) + } + patch := request.Scopes[0].Patches[request.Scopes[0].Snapshot.Files[0].ID] + if patch.Patch != "" || patch.Message == nil || *patch.Message != "Diff exceeds the 1 MiB preview limit." { + t.Fatalf("piped file over the preview limit: %+v", patch) + } +} diff --git a/internal/collector/scopes_test.go b/internal/collector/scopes_test.go index b9e0c030..0ffa49b8 100644 --- a/internal/collector/scopes_test.go +++ b/internal/collector/scopes_test.go @@ -3,7 +3,6 @@ package collector import ( "context" "errors" - "fmt" "strings" "testing" @@ -121,49 +120,6 @@ func TestUncommittedRecreatedAndUntrackedContents(t *testing.T) { } } -func TestBranchScopePreviewQuotasPreserveEveryComparison(t *testing.T) { - root := repository(t) - writeFiles := func(value string) { - for i := range 4 { - write(t, root, fmt.Sprintf("file-%d", i), strings.Repeat(value, 1<<19)+"\n") - } - } - writeFiles("a") - git(t, root, "add", ".") - git(t, root, "commit", "-m", "base") - git(t, root, "switch", "-c", "feature") - writeFiles("b") - git(t, root, "commit", "-am", "committed") - writeFiles("c") - git(t, root, "add", ".") - writeFiles("d") - request := collect(t, root) - quota := previewBudget / len(request.Scopes) - total := 0 - for _, scope := range request.Scopes { - size, unavailable := 0, 0 - for _, patch := range scope.Patches { - size += len(patch.Patch) - if patch.Contents != nil { - size += len(patch.Contents.Before) + len(patch.Contents.After) - } - if patch.Patch == "" && patch.Contents == nil && patch.Message != nil { - unavailable++ - } - } - if size == 0 || size > quota || unavailable == 0 { - t.Fatalf("%s preview bytes %d, quota %d, unavailable %d", scope.Snapshot.Mode, size, quota, unavailable) - } - if patch := patchFor(t, request, scope.Snapshot.Mode, "file-0"); patch.Contents == nil { - t.Fatalf("%s starved by an earlier scope", scope.Snapshot.Mode) - } - total += size - } - if total > previewBudget { - t.Fatalf("aggregate bytes %d exceed budget %d", total, previewBudget) - } -} - type changingScopeSource struct { diffsource.Source mode review.DiffMode