Fold files diffr finds binary even when git diffed them as text

apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.test.ts

@@ -7,7 +7,8 @@ import assert from 'node:assert/strict';
 import { createRequire, registerHooks } from 'node:module';
 import test from 'node:test';
 
-import type { ReviewDiffFileWire, ReviewDiffProgressFile, StructuralLineCounts } from '../common/reviewProtocol.js';
+import type { ReviewDiffFileWire, ReviewDiffProgressFile } from '../common/reviewProtocol.js';
+import type { ReviewFileCounts } from '../common/reviewStructuralDiff.js';
 
 // jsdom ships no types here; the DOM globals below are all this test reads from it.
 const { JSDOM } = createRequire(import.meta.url)('jsdom');
@@ -26,7 +27,7 @@ const progress = (path: string, state: ReviewDiffProgressFile['state'], remainin
 	path, state, remaining: { additions: remaining, deletions: 0 }, total: { additions: 3, deletions: 1 }, viewedRanges: [], changedRanges: [],
 });
 
-function row(state: ReviewDiffProgressFile['state'] | undefined, counts?: StructuralLineCounts) {
+function row(state: ReviewDiffProgressFile['state'] | undefined, counts?: ReviewFileCounts) {
 	const path = 'src/a.ts';
 	const renderer = new ChangedFilesTreeRenderer(
 		new Map(counts ? [[path, counts]] : []),
@@ -64,3 +65,9 @@ test('without progress, the structural counts show and nothing reads as folded',
 	assert.equal(pending.counts, '+4−1');
 	assert.ok(!pending.row.classList.contains('review-file-folded'));
 });
+
+test('a file git diffed as text but diffr found binary stays folded with its size', () => {
+	const binary = row(undefined, { binary: true, baseSize: 2048, headSize: 4096 });
+	assert.equal(binary.counts, '2.0 KB → 4.0 KB');
+	assert.ok(binary.row.classList.contains('review-file-folded'));
+});

apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.ts

@@ -145,10 +145,12 @@ export class ChangedFilesTreeRenderer
 
     template.row.classList.toggle("review-changed-files-folder", !isFile);
 		const done = isFile ? this.progress.get(element.file.path)?.state : undefined;
+		const stored = isFile ? this.counts.get(element.file.path) : undefined;
+		const binary = isFile && (!!element.file.binary || isBinaryCounts(stored));
 		// Folded files read as done, like viewed ones: greyed, with the reason in place of counts.
 		template.row.classList.toggle("review-file-viewed", done === "viewed");
 		// A binary stays folded: nothing in it can be read, so it looks and counts like a folded file.
-		template.row.classList.toggle("review-file-folded", done === "folded" || (isFile && !!element.file.binary && done !== "viewed"));
+		template.row.classList.toggle("review-file-folded", done === "folded" || (binary && done !== "viewed"));
     template.icon.hidden = !isFile;
     template.icon.className = isFile
       ? `review-changed-files-icon review-changed-files-icon-${element.file.status} ${ThemeIcon.asClassName(fileStatusIcon(element.file.status))}`
@@ -159,11 +161,10 @@ export class ChangedFilesTreeRenderer
 		if (isFile) {
 			const progress = this.progress.get(element.file.path);
 			const compact = (n: number) => new Intl.NumberFormat('en', { notation: 'compact', maximumFractionDigits: 1 }).format(n).toLowerCase();
-			const stored = this.counts.get(element.file.path);
 			const lines = isBinaryCounts(stored) ? undefined : stored;
 			const additions = progress?.remaining.additions ?? lines?.added;
 			const deletions = progress?.remaining.deletions ?? lines?.removed;
-			if (element.file.binary) {
+			if (binary) {
 				// Where a folded file says "Folded", a binary says its size, or "Binary" until diffr reports it.
 				const size = isBinaryCounts(stored) ? binarySizeLabel(stored) : undefined;
 				template.counts.textContent = progress?.state === 'viewed' ? 'Viewed' : progress?.state === 'folded' ? 'Folded' : size ?? 'Binary';
@@ -177,7 +178,7 @@ export class ChangedFilesTreeRenderer
 				const removed = append(template.counts, $('span.review-tree-removed')); removed.textContent = `−${compact(deletions)}`;
 			}
 			const total = progress?.total ?? { additions: lines?.added ?? 0, deletions: lines?.removed ?? 0 };
-			if (!element.file.binary) template.countsTooltip.content = element.file.status === "unchanged"
+			if (!binary) template.countsTooltip.content = element.file.status === "unchanged"
 				? { label: "Referenced context; no changed lines" }
 				: additions === undefined || deletions === undefined
 					? REVIEW_COUNTS_PENDING_TOOLTIP

apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts

@@ -250,9 +250,9 @@ export class ReviewFilesDiffView extends Disposable {
 					? this.input.entries.map((entry) => ({
 							original: entry.original,
 							modified: entry.modified,
-							additions: entry.file.status === "unchanged" || entry.file.binary ? undefined : this.entryProgress(entry)?.remaining.additions ?? (this.fileTreeContainer || this.document ? undefined : this.lineCounts(entry.file.path)?.added),
-							deletions: entry.file.status === "unchanged" || entry.file.binary ? undefined : this.entryProgress(entry)?.remaining.deletions ?? (this.fileTreeContainer || this.document ? undefined : this.lineCounts(entry.file.path)?.removed),
-							collapseLocked: !!entry.file.binary,
+							additions: entry.file.status === "unchanged" || this.isBinary(entry) ? undefined : this.entryProgress(entry)?.remaining.additions ?? (this.fileTreeContainer || this.document ? undefined : this.lineCounts(entry.file.path)?.added),
+							deletions: entry.file.status === "unchanged" || this.isBinary(entry) ? undefined : this.entryProgress(entry)?.remaining.deletions ?? (this.fileTreeContainer || this.document ? undefined : this.lineCounts(entry.file.path)?.removed),
+							collapseLocked: this.isBinary(entry),
 						countsTooltip: this.progressTooltip(entry) ?? this.streamStats.get(entry.file.path)?.tooltip,
 						viewedState: this.entryProgress(entry)?.state,
 						viewedScope: this.viewedScope,
@@ -263,10 +263,10 @@ export class ReviewFilesDiffView extends Disposable {
 						section: entry.sectionStart ? this.progress?.sections?.find(section => section.id === entry.sectionId) : undefined,
 						onToggleSection: () => entry.sectionId && this.onToggleSection?.(entry.sectionId),
 						// A binary is folded like a hidden file, with its size as the reason.
-						note: entry.file.status === "unchanged" ? "Unchanged" : entry.file.binary ? this.binaryNote(entry.file.path) : this.hiddenFiles.get(entry.file.path),
-						noteTooltip: entry.file.binary ? { label: "No text to show · binary files stay folded" } : undefined,
+						note: entry.file.status === "unchanged" ? "Unchanged" : this.isBinary(entry) ? this.binaryNote(entry.file.path) : this.hiddenFiles.get(entry.file.path),
+						noteTooltip: this.isBinary(entry) ? { label: "No text to show · binary files stay folded" } : undefined,
 							// A binary has no text to open.
-							onDidOpen: entry.file.binary ? undefined : () => {
+							onDidOpen: this.isBinary(entry) ? undefined : () => {
 								if (document?.onDidOpen) { document.onDidOpen(); return; }
 								const target = this.widget.tryGetCodeEditor(entry.goToFileResource);
 								const change = isDiffEditor(target?.diffEditor) ? target.diffEditor.getDiffComputationResult()?.changes2[0] : undefined;
@@ -478,6 +478,10 @@ export class ReviewFilesDiffView extends Disposable {
 		return isBinaryCounts(counts) ? undefined : counts;
 	}
 
+	private isBinary(entry: ReviewFilesEditorEntry): boolean {
+		return !!entry.file.binary || isBinaryCounts(this.streamStats.get(entry.file.path)?.counts);
+	}
+
 	private binaryNote(path: string): string {
 		const counts = this.streamStats.get(path)?.counts;
 		const size = isBinaryCounts(counts) ? binarySizeLabel(counts) : undefined;

apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts

@@ -77,8 +77,8 @@ export function createStructuralDiffEditors(
 			} else {
 				const result = await session.fileResult(source.path);
 				if (result.error) throw new Error(result.error);
-				if (result.diff?.type !== "text") throw new Error(`diffr did not supply a text result for ${source.path}.`);
-				text = result.diff[source.side]?.text ?? "";
+				// diffr's binary check is stricter than git's.
+				text = result.diff?.type === "text" ? result.diff[source.side]?.text ?? "" : "";
 			}
 			return modelService.getModel(uri) ?? modelService.createModel(text, languages.createByFilepathOrFirstLine(uri, text.split("\n", 1)[0]), uri);
 		},