From 419a48a52a865430ef7321140e4237a1bcaa6ddb Mon Sep 17 00:00:00 2001 From: Farrel Darian <62016900+fdarian@users.noreply.github.com> Date: Mon, 3 Aug 2026 17:16:47 +0700 Subject: [PATCH 1/2] fix(diffs): keep scroll anchor stable when CodeView removes items Scroll partway down a CodeView list, then update the controlled items array to drop or reorder an earlier entry (for example, hiding an already-reviewed file). The viewport jumps to an unrelated position instead of staying on the item that was on screen. reconcileItems reassigns this.items without first capturing a scroll anchor, unlike setOptions, which always captures one before a layout-affecting change. Without a pending anchor, the next render's getScrollAnchor scans the old render window's indices against the freshly reindexed items array, reading position data that recomputeLayout has not refreshed yet, so the anchor it finds is wrong or nonexistent. Capture the anchor right before reassigning this.items, after the method's existing no-op early return rather than unconditionally at the top. A same-length update that changes nothing returns early without rendering, and render is what consumes a pending anchor; capturing before that check would leave a stale anchor on the instance for a later, unrelated render to "correct" against. --- packages/diffs/src/components/CodeView.ts | 12 ++++ .../test/CodeView.scrollAnchoring.test.ts | 63 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/packages/diffs/src/components/CodeView.ts b/packages/diffs/src/components/CodeView.ts index 61dd6ba6e..c8e8a13e4 100644 --- a/packages/diffs/src/components/CodeView.ts +++ b/packages/diffs/src/components/CodeView.ts @@ -2535,6 +2535,18 @@ export class CodeView { return; } + // Capture the scroll anchor against the outgoing items/layout before + // reassigning this.items below. Otherwise the next render's + // getScrollAnchor() reads renderState.firstIndex/lastIndex against + // already-reindexed items through stale top/height metrics, which is + // exactly wrong for removals and reorders. This must stay below the + // early return above: capturing unconditionally at the top of the + // method would strand a pending anchor on the instance after a + // no-op reconcile, since render() (which consumes/clears it) never + // runs when nothing changed, and some later unrelated render would + // then "correct" scroll against that stale anchor. + this.capturePendingLayoutAnchor(); + this.items = nextItems; this.idToItem = nextIdToItem; this.instanceToItem = nextInstanceToItem; diff --git a/packages/diffs/test/CodeView.scrollAnchoring.test.ts b/packages/diffs/test/CodeView.scrollAnchoring.test.ts index 4a7d8e5d3..80e930304 100644 --- a/packages/diffs/test/CodeView.scrollAnchoring.test.ts +++ b/packages/diffs/test/CodeView.scrollAnchoring.test.ts @@ -5,6 +5,7 @@ import { DEFAULT_CODE_VIEW_LAYOUT } from '../src/constants'; import type { CodeViewItem, FileContents } from '../src/types'; import { parseDiffFromFile } from '../src/utils/parseDiffFromFile'; import { + createRoot, dispatchScroll, installDom, makeFile, @@ -251,6 +252,68 @@ describe('CodeView scroll anchoring', () => { } }); + test('keeps a rendered item anchored when reconcileItems removes earlier items', async () => { + const { cleanup } = installDom(); + const viewer = new CodeView(); + const root = createRoot({ height: ROOT_HEIGHT }); + const items = Array.from({ length: 60 }, (_, index) => + makeFileItem(`file:${index}`, 5 + ((index * 7) % 15)) + ); + const anchorId = 'file:30'; + + try { + viewer.setup(root); + await renderItems(viewer, items); + + viewer.scrollTo({ + type: 'item', + id: anchorId, + align: 'start', + behavior: 'instant', + }); + viewer.render(true); + await wait(0); + + const anchorTopBeforeRemoval = viewer.getTopForItem(anchorId) ?? 0; + const scrollTopBeforeRemoval = viewer.getScrollTop(); + + // Remove several earlier items in one call, well outside the current + // render window but still shifting the index (and therefore the top + // offset) of every item at or after them, including the anchor. A + // single removal isn't enough here, even giving the removed item a + // very different height from its neighbors: the stale scan is off by + // exactly one slot, which almost always still resolves to the very + // next real item, and that item's own recomputed shift is identical + // regardless of what was removed, so the error cancels out. Only a + // shift big enough to carry the stale, off-by-N read past the old + // viewport/overscan bounds makes the search fail outright, which is + // what removing 10 items in one call reliably does. + const removedIds = new Set( + Array.from({ length: 10 }, (_, index) => `file:${index}`) + ); + viewer.setItems(items.filter((item) => !removedIds.has(item.id))); + viewer.render(true); + await wait(0); + + const anchorTopAfterRemoval = viewer.getTopForItem(anchorId) ?? 0; + const scrollTopAfterRemoval = viewer.getScrollTop(); + + // The anchor shifted up by the removed items' combined height. A + // correct anchor keeps it fixed in the viewport, so the scroll + // position must shift by the same delta. Without capturing the anchor + // before the reindex, the anchor scan instead finds nothing (it reads + // stale, not-yet-relaid-out positions through the new index mapping) + // and scrollTop is left stuck at its old value. + const anchorShift = anchorTopBeforeRemoval - anchorTopAfterRemoval; + expect(anchorShift).toBeGreaterThan(0); + expect(scrollTopBeforeRemoval - scrollTopAfterRemoval).toBe(anchorShift); + } finally { + viewer.cleanUp(); + await wait(0); + cleanup(); + } + }); + test('moves the physical spacer before applying a programmatic rebase jump', async () => { const { cleanup } = installDom(); const viewer = new CodeView({ From 14c4400258b9be846ec7c512a48887bc41ae6305 Mon Sep 17 00:00:00 2001 From: Amadeus Demarzi Date: Mon, 3 Aug 2026 15:10:13 -0700 Subject: [PATCH 2/2] Harden item deletion scroll anchoring This updated implementation adds more robustness to scroll anchoring to continue to work on the following scenarios: * If the bottom of the item to be removed was visible in the viewport * Prevent anchoring to an element that will be removed * If the removed item took up the full viewport * Attempt to anchor to the next item's top --- packages/diffs/src/components/CodeView.ts | 93 ++++++++++---- .../test/CodeView.scrollAnchoring.test.ts | 113 ++++++++++++++++++ 2 files changed, 182 insertions(+), 24 deletions(-) diff --git a/packages/diffs/src/components/CodeView.ts b/packages/diffs/src/components/CodeView.ts index c8e8a13e4..596062a82 100644 --- a/packages/diffs/src/components/CodeView.ts +++ b/packages/diffs/src/components/CodeView.ts @@ -651,6 +651,11 @@ type PendingScrollTarget = | PendingRangeTarget | PendingItemTarget; +type CodeViewItemMap = Map< + string, + CodeViewContextItem +>; + export class CodeView { static __STOP = false; static __lastScrollPosition = 0; @@ -662,7 +667,7 @@ export class CodeView { resizeDebugging: false, }; private items: CodeViewContextItem[] = []; - private idToItem: Map> = new Map(); + private idToItem: CodeViewItemMap = new Map(); private selectedLines: CodeViewLineSelection | null = null; // One editor per edit-mode item, created lazily via options.createEditor. // Entries survive virtualization unmounts so a remounted item re-attaches @@ -1706,9 +1711,10 @@ export class CodeView { } } - public capturePendingLayoutAnchor(): void { + public capturePendingLayoutAnchor( + nextItems: Readonly> = this.idToItem + ): void { if ( - this.pendingLayoutAnchor != null || this.root == null || this.items.length === 0 || this.pendingScrollTarget != null @@ -1716,7 +1722,10 @@ export class CodeView { return; } - this.pendingLayoutAnchor = this.getScrollAnchor(this.getScrollTop()); + this.pendingLayoutAnchor = this.getScrollAnchor( + this.getScrollTop(), + nextItems + ); } public render(immediate = false): void { @@ -2521,6 +2530,15 @@ export class CodeView { nextInstanceToItem.set(item.instance, item); } + if (firstDirtyIndex == null) { + if (removedItems.size === 0) { + return; + } + firstDirtyIndex = Math.max(nextItems.length - 1, 0); + } + + this.capturePendingLayoutAnchor(nextIdToItem); + for (let index = 0; index < previousItems.length; index++) { const removedItem = previousItems[index]; if (removedItem == null || !removedItems.has(removedItem)) { @@ -2531,22 +2549,6 @@ export class CodeView { firstDirtyIndex = Math.min(firstDirtyIndex ?? dirtyIndex, dirtyIndex); } - if (firstDirtyIndex == null) { - return; - } - - // Capture the scroll anchor against the outgoing items/layout before - // reassigning this.items below. Otherwise the next render's - // getScrollAnchor() reads renderState.firstIndex/lastIndex against - // already-reindexed items through stale top/height metrics, which is - // exactly wrong for removals and reorders. This must stay below the - // early return above: capturing unconditionally at the top of the - // method would strand a pending anchor on the instance after a - // no-op reconcile, since render() (which consumes/clears it) never - // runs when nothing changed, and some later unrelated render would - // then "correct" scroll against that stale anchor. - this.capturePendingLayoutAnchor(); - this.items = nextItems; this.idToItem = nextIdToItem; this.instanceToItem = nextInstanceToItem; @@ -3641,10 +3643,25 @@ export class CodeView { * A scroll anchor represents the first fully visible element (in other * words, the first file or first line who's top is fully in the viewport). */ - private getScrollAnchor(scrollTop: number): ScrollAnchor | undefined { - // If we already have a pendingLayoutAnchor, let's use that. - if (this.pendingLayoutAnchor != null) { - return this.pendingLayoutAnchor; + private getScrollAnchor( + scrollTop: number, + availableItems: ReadonlyMap> = this + .idToItem + ): ScrollAnchor | undefined { + let skippedItem: CodeViewContextItem | undefined; + + const { pendingLayoutAnchor } = this; + if (pendingLayoutAnchor != null) { + const pendingItem = this.idToItem.get(pendingLayoutAnchor.id); + if ( + pendingItem != null && + availableItems.get(pendingLayoutAnchor.id) === pendingItem + ) { + return pendingLayoutAnchor; + } + if (pendingItem != null) { + skippedItem = pendingItem; + } } // We shouldn't scroll anchor when at the top, this way if a custom header @@ -3683,6 +3700,12 @@ export class CodeView { break; } + const itemIsAvailable = availableItems.get(item.item.id) === item; + if (!itemIsAvailable) { + skippedItem ??= item; + continue; + } + if (absoluteItemTop >= scrollTop) { return { type: 'item', @@ -3710,6 +3733,28 @@ export class CodeView { } } + // If we couldn't find an anchored item and we have a skipped item, lets + // attempt to anchor to the top of the item that came after it + if (skippedItem != null) { + for ( + let index = skippedItem.index + 1; + index < this.items.length; + index++ + ) { + const candidate = this.items[index]; + if ( + candidate != null && + availableItems.get(candidate.item.id) === candidate + ) { + return { + type: 'item', + id: candidate.item.id, + viewportOffset: 0, + }; + } + } + } + // I don't think we'll ever make it this far... return undefined; } diff --git a/packages/diffs/test/CodeView.scrollAnchoring.test.ts b/packages/diffs/test/CodeView.scrollAnchoring.test.ts index 80e930304..8b0542fe1 100644 --- a/packages/diffs/test/CodeView.scrollAnchoring.test.ts +++ b/packages/diffs/test/CodeView.scrollAnchoring.test.ts @@ -314,6 +314,119 @@ describe('CodeView scroll anchoring', () => { } }); + test('anchors the next item at the top when reconcileItems removes the only visible anchor', async () => { + const { cleanup } = installDom(); + const viewer = new CodeView(); + const root = createRoot({ height: ROOT_HEIGHT }); + const removedId = 'diff:removed-anchor'; + const nextId = 'file:after-anchor'; + const items: CodeViewItem[] = [ + makeFileItem('file:before-anchor', 120), + makeReplacementDiffItem(removedId, 240), + makeFileItem(nextId, 80), + ...Array.from({ length: 8 }, (_, index) => + makeFileItem(`file:trailing-${index}`, 80) + ), + ]; + + try { + viewer.setup(root); + await renderItems(viewer, items); + + viewer.scrollTo({ + type: 'line', + id: removedId, + lineNumber: 80, + side: 'additions', + align: 'start', + behavior: 'instant', + }); + viewer.render(true); + await wait(0); + + // Keep the successor outside the old render window so reconciliation + // has to anchor its item header at the top instead of finding a visible + // survivor. + expect(viewer.getRenderedItems().some((item) => item.id === nextId)).toBe( + false + ); + + viewer.setItems(items.filter((item) => item.id !== removedId)); + viewer.render(true); + await wait(0); + + const nextTop = viewer.getTopForItem(nextId); + if (nextTop == null) { + throw new Error('expected the promoted successor'); + } + expect(nextTop - viewer.getScrollTop()).toBe(0); + } finally { + viewer.cleanUp(); + await wait(0); + cleanup(); + } + }); + + test('skips a removed line anchor for a visible surviving item', async () => { + const { cleanup } = installDom(); + const viewer = new CodeView(); + const root = createRoot({ height: ROOT_HEIGHT }); + const removedId = 'diff:visible-removed-anchor'; + const nextId = 'file:visible-after-anchor'; + const items: CodeViewItem[] = [ + makeFileItem('file:visible-before-anchor', 100), + makeReplacementDiffItem(removedId, 12), + makeFileItem(nextId, 40), + makeFileItem('file:visible-trailing', 100), + ]; + + try { + viewer.setup(root); + await renderItems(viewer, items); + + viewer.scrollTo({ + type: 'line', + id: removedId, + lineNumber: 4, + side: 'additions', + align: 'start', + behavior: 'instant', + }); + viewer.render(true); + await wait(0); + + expect(viewer.getRenderedItems().some((item) => item.id === nextId)).toBe( + true + ); + const nextTopBeforeRemoval = viewer.getTopForItem(nextId); + if (nextTopBeforeRemoval == null) { + throw new Error('expected the visible successor'); + } + const nextViewportOffset = + DEFAULT_CODE_VIEW_LAYOUT.paddingTop + + nextTopBeforeRemoval - + viewer.getScrollTop(); + + viewer.setItems(items.filter((item) => item.id !== removedId)); + viewer.render(true); + await wait(0); + + const nextTopAfterRemoval = viewer.getTopForItem(nextId); + if (nextTopAfterRemoval == null) { + throw new Error('expected the surviving item'); + } + expect( + DEFAULT_CODE_VIEW_LAYOUT.paddingTop + + nextTopAfterRemoval - + viewer.getScrollTop() + ).toBe(nextViewportOffset); + } finally { + viewer.cleanUp(); + await wait(0); + cleanup(); + } + }); + test('moves the physical spacer before applying a programmatic rebase jump', async () => { const { cleanup } = installDom(); const viewer = new CodeView({