diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentDiffLayout.test.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentDiffLayout.test.ts new file mode 100644 index 000000000..088afb447 --- /dev/null +++ b/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(); + 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(); + } +}); diff --git a/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentDiffLayout.ts b/apps/review-desktop/code-oss/src/vs/review/services/reviewDocumentDiffLayout.ts new file mode 100644 index 000000000..7ca0d3c98 --- /dev/null +++ b/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; + 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, + 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); + }); +} 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..811f17bd1 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 @@ -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(); private readonly fileStates = new Map(); private readonly settleHold = this._register(new MutableDisposable()); + 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 { 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" });