From 97175ed92b4d99dcc58335af1ccf7ea5a03808b8 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:11:42 -0700 Subject: [PATCH] fix(diff): keep scroll restore armed through layout shifts (#10615) * fix(diff): keep scroll restore armed through layout shifts * test: add scroll-restore convergence and user-scroll disarm cases Verify that a converging restore withstands layout shifts and continues retrying, while unmarked user scroll disarms the restore attempt. Stabilize marks and offset objects across renders to preserve the bookkeeping state that guards restoration retries. --- ...dScrollAnchor.marks-restore-signal.test.ts | 107 ++++++++++++++++++ .../src/hooks/useVirtualizedScrollAnchor.ts | 3 +- 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/hooks/useVirtualizedScrollAnchor.marks-restore-signal.test.ts b/src/renderer/src/hooks/useVirtualizedScrollAnchor.marks-restore-signal.test.ts index 0e7ec50cf..b043754bd 100644 --- a/src/renderer/src/hooks/useVirtualizedScrollAnchor.marks-restore-signal.test.ts +++ b/src/renderer/src/hooks/useVirtualizedScrollAnchor.marks-restore-signal.test.ts @@ -135,6 +135,113 @@ describe('useVirtualizedScrollAnchor with marks + restoreSignal', () => { expect(virtualizer.scrollToIndex).not.toHaveBeenCalled() }) + it('keeps retrying a restore that is still converging when layout moves the viewport', async () => { + const { harness, useVirtualizedScrollAnchor } = await loadHook() + let rowTop = -3_000 + const rowElement: FakeRowElement = { + getBoundingClientRect: () => ({ + bottom: rowTop + 4_000, + height: 4_000, + top: rowTop + }), + isConnected: true, + key: 'row-1' + } + const { el } = createScrollElement({ + rowElements: [rowElement], + scrollTop: 746 + }) + const anchorRef = { current: { key: 'row-1', offset: 3_358, scrollTop: 746 } } + const virtualizer = virtualizerWithRow1() + // Stable across rerenders, like the real caller: fresh objects would reset + // the marks and offset bookkeeping the first pass left behind. + const programmaticScrollMarks = createProgrammaticScrollMarks() + const scrollElementRef = { current: el } + const scrollOffsetRef = { current: 746 } + + const render = (): void => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + useVirtualizedScrollAnchor({ + anchorRef, + getItemElementKey: (element: FakeRowElement) => element.key, + getRowKey: (row: string) => row, + itemElementSelector: '[data-row]', + programmaticScrollMarks, + recordAnchorOnScroll: false, + restoreSignal: 'signal-a', + rows: ['row-0', 'row-1'], + scrollElementRef, + scrollOffsetRef, + totalSize: 30_000, + virtualizer + } as never) + harness.effects[1]?.effect() + } + + render() + const attemptsAfterFirstPass = el.querySelectorAll.mock.calls.length + expect(attemptsAfterFirstPass).toBeGreaterThan(0) + expect(el.scrollTop).toBe(1_104) + + // Monaco grows content above while browser anchoring keeps the row pinned. + rowTop = -3_358 + el.scrollTop += 196 + + render() + expect(el.querySelectorAll.mock.calls.length).toBeGreaterThan(attemptsAfterFirstPass) + const attemptsAfterConfirm = el.querySelectorAll.mock.calls.length + expect(anchorRef.current.scrollTop).toBe(1_300) + + render() + expect(el.querySelectorAll.mock.calls.length).toBe(attemptsAfterConfirm) + }) + + it('lets an unmarked user scroll disarm a restore that is still converging', async () => { + const { harness, useVirtualizedScrollAnchor } = await loadHook() + const { el, emitScroll } = createScrollElement({ + rowElements: [anchoredRowElement('row-1', -3_000)], + scrollTop: 746 + }) + const anchorRef = { current: { key: 'row-1', offset: 3_358, scrollTop: 746 } } + const programmaticScrollMarks = createProgrammaticScrollMarks() + const scrollElementRef = { current: el } + const scrollOffsetRef = { current: 746 } + const virtualizer = virtualizerWithRow1() + + const render = (): void => { + harness.beginRender() + // oxlint-disable-next-line react-hooks/rules-of-hooks -- test harness mocks React's hook dispatcher directly. + useVirtualizedScrollAnchor({ + anchorRef, + getItemElementKey: (element: FakeRowElement) => element.key, + getRowKey: (row: string) => row, + itemElementSelector: '[data-row]', + programmaticScrollMarks, + recordAnchorOnScroll: false, + restoreSignal: 'signal-a', + rows: ['row-0', 'row-1'], + scrollElementRef, + scrollOffsetRef, + totalSize: 30_000, + virtualizer + } as never) + } + + render() + harness.effects[0]?.effect() + harness.effects[1]?.effect() + const attemptsAfterFirstPass = el.querySelectorAll.mock.calls.length + expect(el.scrollTop).toBe(1_104) + + emitScroll(1_400) + render() + harness.effects[1]?.effect() + + expect(el.scrollTop).toBe(1_400) + expect(el.querySelectorAll.mock.calls.length).toBe(attemptsAfterFirstPass) + }) + it('lets the user position win when the viewport moved after the anchor was recorded', async () => { const { harness, useVirtualizedScrollAnchor } = await loadHook() const { el } = createScrollElement({ diff --git a/src/renderer/src/hooks/useVirtualizedScrollAnchor.ts b/src/renderer/src/hooks/useVirtualizedScrollAnchor.ts index 4ffcdf7ba..56353fbee 100644 --- a/src/renderer/src/hooks/useVirtualizedScrollAnchor.ts +++ b/src/renderer/src/hooks/useVirtualizedScrollAnchor.ts @@ -254,7 +254,8 @@ export function useVirtualizedScrollAnchor< // adjustment; restoring here would fight concurrent user scrolling. return } - if (anchor.scrollTop !== undefined) { + // Why: pending restores own layout shifts; unmarked user scrolls disarm them in the listener. + if (anchor.scrollTop !== undefined && !pendingRestoreRef.current) { const maxScrollTop = Math.max(0, el.scrollHeight - el.clientHeight) const clampExplained = anchor.scrollTop > maxScrollTop + 1 && el.scrollTop >= maxScrollTop - 2