Refactor multi-diff virtualized scrolling - #333394
Merged
Henning Dieterichs (hediet) merged 4 commits intoAug 30, 2026
Merged
Conversation
Introduce revisioned logical geometry, semantic scroll anchoring, pooled virtualized item management, layout diagnostics, and shared fake/real component fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628b1c91-5489-4cfc-baf3-ca556c6e50bc
Copilot started reviewing on behalf of
Henning Dieterichs (hediet)
August 30, 2026 13:39
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 5
New issues introduced by this change (5)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/contrib/multiDiffEditor/browser/multiDiffEditorLayoutDebug.ts — Properties whose runtime value toggles through undefined are never represented in the patch. For… |
|
src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts — A valid content-size event is first stored in _observedEditorContentHeight and only copied into… |
|
src/vs/editor/browser/widget/multiDiffEditor/compressedVirtualizedScrollView.ts — Replacing the source with different items that happen to have the same heights leaves… |
|
src/vs/editor/browser/widget/multiDiffEditor/multiDiffEditorWidgetImpl.ts — scrollTop here is a logical item offset, while setScrollPosition accepts the physical scrollbar… |
|
src/vs/workbench/test/browser/componentFixtures/multiDiffEditorScroll.fixture.ts — When the observable changes while this input is focused, this autorun performs no observable read,… |
What changed in this PR
Refactors multi-diff virtualization to preserve scroll stability during geometry changes and adds layout diagnostics and regression fixtures.
Changes:
- Introduces revisioned compressed scrolling with semantic anchors and transient slack.
- Replaces generic pooling with typed virtualized item management.
- Adds JSONL diagnostics, fixtures, and focused regression tests.
| File | Description |
|---|---|
multiDiffEditorScroll.fixture.ts |
Adds interactive scrolling fixtures. |
multiDiffEditorScroll.fixture.css |
Styles the new fixtures. |
editor/multiDiffEditorFixtureUtils.ts |
Shares fixture setup utilities. |
editor/multiDiffEditor.fixture.ts |
Adopts shared fixture utilities. |
multiDiffEditorLayoutDebug.test.ts |
Tests JSON patch diagnostics. |
multiDiffEditorLayoutDebug.ts |
Implements JSONL layout logging. |
multiDiffEditor.ts |
Exposes layout debug state. |
multiDiffEditor.contribution.ts |
Registers the debug action. |
actions.ts |
Opens layout diagnostics. |
sessionChangesEditor.ts |
Exposes session layout diagnostics. |
virtualizedItemManager.test.ts |
Tests typed template pooling. |
multiDiffEditorWidget.test.ts |
Tests recycled template heights. |
compressedVirtualizedScrollLayout.test.ts |
Tests layout and anchoring behavior. |
virtualizedItemManager.ts |
Adds typed binding and pooling management. |
objectPool.ts |
Removes the superseded generic pool. |
multiDiffEditorWidgetImpl.ts |
Integrates compressed virtualization. |
multiDiffEditorWidget.ts |
Publishes debug-state access. |
multiDiffEditorViewModel.ts |
Tracks expanded content height. |
diffEditorItemTemplate.ts |
Implements strict virtualized bindings. |
compressedVirtualizedScrollView.ts |
Manages anchored scrolling and slack. |
compressedVirtualizedScrollLayout.ts |
Computes revisioned logical geometry. |
hideUnchangedRegionsFeature.ts |
Preserves anchors during region expansion. |
diffEditorWidget.ts |
Adds scroll-anchor callbacks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+76
to
+89
| for (const key of Object.keys(previous)) { | ||
| if (!hasKey(next, { [key]: true })) { | ||
| operations.push({ op: 'remove', path: toJsonPointer([...path, key]) }); | ||
| } | ||
| } | ||
| for (const [key, value] of Object.entries(next)) { | ||
| if (!hasKey(previous, { [key]: true })) { | ||
| if (value !== undefined) { | ||
| operations.push({ op: 'add', path: toJsonPointer([...path, key]), value }); | ||
| } | ||
| } else if (value !== undefined && previous[key] !== undefined) { | ||
| appendJsonPatchOperations(operations, createJsonPatch(previous[key], value, [...path, key])); | ||
| } | ||
| } |
Comment on lines
+251
to
+253
| public getExpandedContentHeight(): number { | ||
| return this._editorContentHeight.get() + this._outerEditorHeight; | ||
| } |
| leadingScrollSlack: this._leadingScrollSlack, | ||
| trailingScrollSlack: this._trailingScrollSlack, | ||
| }, tx); | ||
| } else if (geometryChanged) { |
| range: options?.range, | ||
| }); | ||
| this._scrollableElement.setScrollPosition({ scrollTop }); | ||
| this._scrollView.setScrollPosition({ scrollTop }); |
Comment on lines
+1474
to
+1478
| store.add(autorun(reader => { | ||
| if (dom.getActiveElement() !== input) { | ||
| input.value = formatNumber(value.read(reader)); | ||
| } | ||
| })); |
Contributor
Screenshot ChangesBase: Changed (2)
|
| Before | After |
|---|---|
 |
 |
editor/multiDiffEditor/MultiDiffEditorIncrementalResolved/Light
| Before | After |
|---|---|
 |
 |
Added (6)
editor/multiDiff/multiDiffEditorScroll/ScrollModel/Dark
editor/multiDiff/multiDiffEditorScroll/ScrollModel/Light
editor/multiDiff/multiDiffEditorScroll/TemplatePool/Dark
editor/multiDiff/multiDiffEditorScroll/TemplatePool/Light
editor/multiDiff/multiDiffEditorScroll/RealWidget/Dark
editor/multiDiff/multiDiffEditorScroll/RealWidget/Light
Detach manager output from source observables during disposal so ref-counted diff view models are released regardless of downstream teardown order. Move dependent class-field initialization into constructors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628b1c91-5489-4cfc-baf3-ca556c6e50bc
Wait for the responsive-layout diff computation before disposing the test widget, and revert the unrelated virtualized item manager experiment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628b1c91-5489-4cfc-baf3-ca556c6e50bc
Tie references retained across diff model transactions to the widget lifetime so synchronous teardown releases them immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628b1c91-5489-4cfc-baf3-ca556c6e50bc
Ladislau Szomoru (lszomoru)
approved these changes
Aug 30, 2026
Henning Dieterichs (hediet)
deleted the
hediet/multi-diff-virtualized-scrolling
branch
August 30, 2026 19:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
git diff --check origin/main...HEAD