Collapse identical comparisons into a single pane

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

@@ -0,0 +1,93 @@
+import assert from "node:assert/strict";
+import test from "node:test";
+
+import { Emitter } from "../../base/common/event.js";
+import { observableValue, transaction } from "../../base/common/observable.js";
+import type { ReviewDiffLayout } from "../common/reviewProtocol.js";
+import type { ReviewDiffLayoutSetting } from "./reviewDiffLayout.js";
+import {
+	bindDocumentDiffLayout,
+	type DocumentDiffLayoutItem,
+} from "./reviewDocumentDiffLayout.js";
+
+test("identical documents collapse and changed or pending documents follow the preference", () => {
+	const changes = new Emitter<ReviewDiffLayout>();
+	let preference: ReviewDiffLayout = "split";
+	const layout = {
+		get: () => preference,
+		onDidChange: changes.event,
+	} as ReviewDiffLayoutSetting;
+	const isDiffUpToDate = observableValue("ready", false);
+	const diff = observableValue<
+		{ identical: boolean; quitEarly: boolean } | undefined
+	>("diff", undefined);
+	const item = { isDiffUpToDate, diff };
+	const items = observableValue<
+		readonly (DocumentDiffLayoutItem | undefined)[]
+	>("items", []);
+	let split: boolean | undefined;
+	const binding = bindDocumentDiffLayout(layout, items, (value) => {
+		split = value;
+	});
+	try {
+		assert.equal(
+			split,
+			true,
+			"an empty/loading view must not count as identical",
+		);
+		items.set([item], undefined);
+		assert.equal(split, true);
+		transaction((tx) => {
+			diff.set({ identical: true, quitEarly: false }, tx);
+			isDiffUpToDate.set(true, tx);
+		});
+		assert.equal(split, false);
+		preference = "unified";
+		changes.fire(preference);
+		preference = "split";
+		changes.fire(preference);
+		assert.equal(
+			split,
+			false,
+			"choosing split must not duplicate identical documents",
+		);
+		isDiffUpToDate.set(false, undefined);
+		assert.equal(
+			split,
+			true,
+			"a stale identical result must not collapse a changed document",
+		);
+		transaction((tx) => {
+			diff.set({ identical: false, quitEarly: false }, tx);
+			isDiffUpToDate.set(true, tx);
+		});
+		assert.equal(split, true);
+		preference = "unified";
+		changes.fire(preference);
+		assert.equal(
+			split,
+			false,
+			"changed documents still respect the unified preference",
+		);
+		preference = "split";
+		changes.fire(preference);
+		diff.set({ identical: true, quitEarly: true }, undefined);
+		assert.equal(
+			split,
+			true,
+			"an incomplete comparison must not count as identical",
+		);
+		diff.set({ identical: true, quitEarly: false }, undefined);
+		items.set([item, undefined], undefined);
+		assert.equal(
+			split,
+			true,
+			"missing sides and non-text entries must not collapse",
+		);
+		items.set([item], undefined);
+		assert.equal(split, false);
+	} finally {
+		binding.dispose();
+		changes.dispose();
+	}
+});

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

@@ -0,0 +1,37 @@
+import type { IDisposable } from "../../base/common/lifecycle.js";
+import {
+	autorun,
+	observableFromEvent,
+	type IObservable,
+} from "../../base/common/observable.js";
+import type { ReviewDiffLayoutSetting } from "./reviewDiffLayout.js";
+
+export interface DocumentDiffLayoutItem {
+	readonly isDiffUpToDate: IObservable<boolean>;
+	readonly diff: IObservable<
+		{ identical: boolean; quitEarly: boolean } | undefined
+	>;
+}
+
+/** Only completed, identical document comparisons override the user's layout. */
+export function bindDocumentDiffLayout(
+	layout: ReviewDiffLayoutSetting,
+	items: IObservable<readonly (DocumentDiffLayoutItem | undefined)[]>,
+	setRenderSideBySide: (split: boolean) => void,
+): IDisposable {
+	const preference = observableFromEvent(layout.onDidChange, () =>
+		layout.get(),
+	);
+	return autorun((reader) => {
+		const current = items.read(reader);
+		const identical =
+			current.length > 0 &&
+			current.every((item) => {
+				if (!item) return false;
+				const upToDate = item.isDiffUpToDate.read(reader);
+				const diff = item.diff.read(reader);
+				return upToDate && diff?.identical === true && !diff.quitEarly;
+			});
+		setRenderSideBySide(preference.read(reader) === "split" && !identical);
+	});
+}

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

@@ -11,11 +11,12 @@ import { Orientation, SplitView } from "../../base/browser/ui/splitview/splitvie
 import { disposableTimeout } from "../../base/common/async.js";
 import { Emitter, Event } from "../../base/common/event.js";
 import { Disposable, DisposableStore, MutableDisposable, toDisposable } from "../../base/common/lifecycle.js";
-import { autorun, observableValue } from "../../base/common/observable.js";
+import { autorun, derived, observableValue } from "../../base/common/observable.js";
 import { isEqual } from "../../base/common/resources.js";
 import { URI } from "../../base/common/uri.js";
 import { ElementSizeObserver } from "../../editor/browser/config/elementSizeObserver.js";
 import { isDiffEditor, type IDiffEditor } from "../../editor/browser/editorBrowser.js";
+import { DiffEditorViewModel } from "../../editor/browser/widget/diffEditor/diffEditorViewModel.js";
 import { MultiDiffEditorViewModel } from "../../editor/browser/widget/multiDiffEditor/multiDiffEditorViewModel.js";
 import { MultiDiffEditorWidget, type RevealOptions } from "../../editor/browser/widget/multiDiffEditor/multiDiffEditorWidget.js";
 import type { IMultiDiffEditorViewState } from "../../editor/browser/widget/multiDiffEditor/multiDiffEditorWidgetImpl.js";
@@ -37,6 +38,7 @@ import { REVIEW_COUNTS_PENDING_TOOLTIP, reviewChangesTooltip, reviewCountsToolti
 import { type ReviewDiffFileWire } from "../common/reviewProtocol.js";
 import { binarySizeLabel, isBinaryCounts, type ReviewFileCounts } from "../common/reviewStructuralDiff.js";
 import type { ReviewDiffLayoutSetting } from "./reviewDiffLayout.js";
+import { bindDocumentDiffLayout } from "./reviewDocumentDiffLayout.js";
 import { reviewMultiDiffLabelUris, ReviewMultiDiffUIElementFactory } from "./reviewMultiDiff.js";
 import type { StructuralDiffSession } from "./reviewStructuralDiffSession.js";
 
@@ -198,6 +200,7 @@ export class ReviewFilesDiffView extends Disposable {
 	private readonly readyFiles = new Set<string>();
 	private readonly fileStates = new Map<string, string>();
 	private readonly settleHold = this._register(new MutableDisposable<DisposableStore>());
+	private readonly documentLayout = this._register(new MutableDisposable());
 	/** Full structural counts for views without persisted coverage. */
 	private readonly streamStats = new Map<
 		string,
@@ -224,7 +227,7 @@ export class ReviewFilesDiffView extends Disposable {
 	constructor(
 		private readonly container: HTMLElement,
 		overflowWidgetsDomNode: HTMLElement | undefined,
-		layout: ReviewDiffLayoutSetting,
+		private readonly diffLayout: ReviewDiffLayoutSetting,
 		private readonly fileTreeContainer: HTMLElement | undefined,
 		private readonly onToggleViewed: ((path: string, sectionId?: string) => void) | undefined,
 		private readonly onToggleSection: ((id: string) => void) | undefined,
@@ -300,8 +303,8 @@ export class ReviewFilesDiffView extends Disposable {
 		);
 		// The widget's own switch, not the per-item option refresh: it pins the
 		// width heuristic off, so the chosen layout is what renders at any width.
-		const applyLayout = () => this.widget.setRenderSideBySide(layout.get() === "split");
-		this._register(layout.onDidChange(applyLayout));
+		const applyLayout = () => this.widget.setRenderSideBySide(this.diffLayout.get() === "split");
+		if (!document) this._register(this.diffLayout.onDidChange(applyLayout));
 		applyLayout();
 		if (document) {
 			this._register(this.widget.onDidChangeContentHeight(() => {
@@ -385,12 +388,22 @@ export class ReviewFilesDiffView extends Disposable {
 
 	async setInput(input: ReviewFilesEditorInput, viewState: IMultiDiffEditorViewState | undefined): Promise<void> {
 		this.settleHold.clear();
+		this.documentLayout.clear();
 		this.pendingViewState = viewState;
 		this.input = input;
 		this.changedFilesTree?.setFiles(Array.from(new Map(input.entries.map(entry => [entry.file.path, entry.file])).values()));
 		const viewModel = await input.getViewModel();
 		if (this._store.isDisposed) return;
 		this.viewModel = viewModel;
+		if (this.document) {
+			const comparisons = derived(reader => viewModel.items.read(reader).map(item => {
+				// Missing sides and binaries are not identical text documents.
+				const entry = input.entries.find(entry => sameResource(item.originalUri, entry.original) && sameResource(item.modifiedUri, entry.modified));
+				return item.originalUri && item.modifiedUri && entry && !entry.file.binary && entry.file.status !== "added" && entry.file.status !== "deleted" && item.diffEditorViewModel instanceof DiffEditorViewModel
+					? item.diffEditorViewModel : undefined;
+			}));
+			this.documentLayout.value = bindDocumentDiffLayout(this.diffLayout, comparisons, split => this.widget.setRenderSideBySide(split));
+		}
 		// The canvas mounts this view without a user gesture, so the widget's
 		// first-change navigation must never take keyboard focus.
 		this.widget.setViewModel(viewModel, { preserveFocus: true, viewState, initialScrollPosition: this.document ? "top" : "firstChange" });