diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.test.ts b/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.test.ts index 83fe0f557..b6131a10d 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.test.ts +++ b/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')); +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.ts b/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.ts index 7bd1b1858..ef09a6c88 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/reviewChangedFilesTree.ts +++ b/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 diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts index 71b4a4907..bec7d7617 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewFilesDiffView.ts +++ b/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; diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts index f1ea1fb64..14591f923 100644 --- a/apps/review-desktop/code-oss/src/vs/review/services/reviewStructuralDiff.ts +++ b/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); },