diff --git a/src/renderer/components/thread/ChatPane/ChatPane.test.tsx b/src/renderer/components/thread/ChatPane/ChatPane.test.tsx index 1cdf2a3f..e32a187c 100644 --- a/src/renderer/components/thread/ChatPane/ChatPane.test.tsx +++ b/src/renderer/components/thread/ChatPane/ChatPane.test.tsx @@ -41,6 +41,8 @@ vi.mock("@tanstack/react-virtual", () => ({ getTotalSize: () => options.count * 96, measure: vi.fn<() => void>(), measureElement: vi.fn<(element: HTMLDivElement | null) => void>(), + resizeItem: vi.fn<(index: number, size: number) => void>(), + options: { measureElement: () => 96 }, scrollToIndex: ( index: number, scrollOptions?: { align?: "auto" | "center" | "end" | "start" }, diff --git a/src/renderer/components/thread/ChatPane/parts/MessageList.test.tsx b/src/renderer/components/thread/ChatPane/parts/MessageList.test.tsx index a96c5b1d..8052a82e 100644 --- a/src/renderer/components/thread/ChatPane/parts/MessageList.test.tsx +++ b/src/renderer/components/thread/ChatPane/parts/MessageList.test.tsx @@ -1,7 +1,11 @@ import { act, fireEvent, render, screen } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { useAppStore } from "@/renderer/state/appStore"; -import { ChatPaneActionsContext, useChatPaneActions } from "../chatPaneActionsContext"; +import { + ChatPaneActionsContext, + useChatPaneActions, + type ChatPaneActions, +} from "../chatPaneActionsContext"; import { MessageList } from "./MessageList"; type MockVirtualRow = { @@ -15,6 +19,8 @@ type MockVirtualizer = { getTotalSize: () => number; measure: () => void; measureElement: (element: HTMLDivElement | null) => void; + resizeItem: (index: number, size: number) => void; + options: { measureElement: (element: Element, entry: undefined, instance: unknown) => number }; scrollToIndex: (index: number, options?: { align?: "start" | "center" | "end" | "auto" }) => void; shouldAdjustScrollPositionOnItemSizeChange?: ( item: { start: number; size: number }, @@ -34,6 +40,8 @@ const { useVirtualizerMock, measureMock, measureElementMock, + resizeItemMock, + optionsMeasureElementMock, scrollToIndexMock, getVirtualItemsMock, getTotalSizeMock, @@ -41,6 +49,9 @@ const { useVirtualizerMock: vi.fn<(options: MockVirtualizerOptions) => MockVirtualizer>(), measureMock: vi.fn<() => void>(), measureElementMock: vi.fn<(element: HTMLDivElement | null) => void>(), + resizeItemMock: vi.fn<(index: number, size: number) => void>(), + optionsMeasureElementMock: + vi.fn<(element: Element, entry: undefined, instance: unknown) => number>(), scrollToIndexMock: vi.fn<(index: number, options?: { align?: "start" | "center" | "end" | "auto" }) => void>(), getVirtualItemsMock: vi.fn<() => MockVirtualRow[]>(), @@ -77,11 +88,14 @@ describe("MessageList", () => { { key: "row-3", index: 2, start: 192 }, ]); getTotalSizeMock.mockReturnValue(384); + optionsMeasureElementMock.mockReturnValue(96); useVirtualizerMock.mockReturnValue({ getVirtualItems: getVirtualItemsMock, getTotalSize: getTotalSizeMock, measure: measureMock, measureElement: measureElementMock, + resizeItem: resizeItemMock, + options: { measureElement: optionsMeasureElementMock }, scrollToIndex: scrollToIndexMock, }); }); @@ -140,78 +154,40 @@ describe("MessageList", () => { expect(document.querySelector("[data-item-id='item-2']")).not.toHaveAttribute("style"); }); - it("only adjusts scroll for measured rows fully above the viewport", () => { - const scrollElement = document.createElement("div"); - scrollElement.scrollTop = 160; - - render( - , - ); + it("never lets TanStack adjust scroll itself and compensates rows fully above the viewport on the next commit", () => { + const { scrollElement, shouldAdjust, commit } = renderCompensationList(); - const virtualizer = useVirtualizerMock.mock.results[0]!.value; - const shouldAdjust = virtualizer.shouldAdjustScrollPositionOnItemSizeChange!; + expect(shouldAdjust({ start: 0, size: 80 }, 40, idleVirtualizer)).toBe(false); - const idleVirtualizer = { isScrolling: false, scrollDirection: null } as const; - expect(shouldAdjust({ start: 0, size: 80 }, 40, idleVirtualizer)).toBe(true); - expect(shouldAdjust({ start: 96, size: 100 }, 40, idleVirtualizer)).toBe(false); + commit(); + expect(scrollElement.scrollTop).toBe(200); }); - it("does not adjust scroll for rows above the viewport during active upward scroll", () => { - const scrollElement = document.createElement("div"); - scrollElement.scrollTop = 160; + it("does not compensate rows that overlap or sit below the viewport", () => { + const { scrollElement, shouldAdjust, commit } = renderCompensationList(); - render( - , - ); - - const virtualizer = useVirtualizerMock.mock.results[0]!.value; - const shouldAdjust = virtualizer.shouldAdjustScrollPositionOnItemSizeChange!; + expect(shouldAdjust({ start: 96, size: 100 }, 40, idleVirtualizer)).toBe(false); - expect( - shouldAdjust({ start: 0, size: 80 }, -40, { - isScrolling: true, - scrollDirection: "backward", - }), - ).toBe(false); + commit(); + expect(scrollElement.scrollTop).toBe(160); }); - it("does not adjust scroll for delayed row measurements after scrolling upward", () => { - const scrollElement = document.createElement("div"); - scrollElement.scrollTop = 160; - - render( - , - ); - - const virtualizer = useVirtualizerMock.mock.results[0]!.value; - const shouldAdjust = virtualizer.shouldAdjustScrollPositionOnItemSizeChange!; + it("compensates rows above the viewport during active upward scroll", () => { + const { scrollElement, shouldAdjust, commit } = renderCompensationList(); scrollElement.scrollTop = 120; - fireEvent.scroll(scrollElement); - expect( shouldAdjust({ start: 0, size: 80 }, -40, { - isScrolling: false, - scrollDirection: null, + isScrolling: true, + scrollDirection: "backward", }), ).toBe(false); + + commit(); + expect(scrollElement.scrollTop).toBe(80); }); - it("adjusts streaming row height changes when bottom-sticky", () => { - const scrollElement = document.createElement("div"); - scrollElement.scrollTop = 160; + it("compensates streaming row height changes when bottom-sticky", () => { const actions = { openProjectRelativePath: vi.fn<(path: string, lineNumber?: number) => void>(), revealProjectFolderInTree: vi.fn<(path: string) => void>(), @@ -221,26 +197,28 @@ describe("MessageList", () => { projectLocation: { kind: "windows" as const, path: "C:\\repo" }, projectRootNames: new Set(), }; + const { scrollElement, shouldAdjust, commit } = renderCompensationList(actions); + + expect(shouldAdjust({ start: 96, size: 100 }, 24, idleVirtualizer)).toBe(false); + + commit(); + expect(scrollElement.scrollTop).toBe(184); + }); + + it("measures newly mounted rows synchronously so the size correction lands in the mount commit", () => { + const scrollElement = document.createElement("div"); + optionsMeasureElementMock.mockReturnValue(132); render( - - - , + , ); - const virtualizer = useVirtualizerMock.mock.results[0]!.value; - const shouldAdjust = virtualizer.shouldAdjustScrollPositionOnItemSizeChange!; - - expect( - shouldAdjust({ start: 96, size: 100 }, 24, { - isScrolling: false, - scrollDirection: null, - }), - ).toBe(true); + expect(resizeItemMock).toHaveBeenCalledWith(1, 132); + expect(resizeItemMock).toHaveBeenCalledWith(2, 132); }); it("registers TanStack scrollToIndex as the bottom scroll handler", () => { @@ -479,3 +457,35 @@ describe("MessageList", () => { function makeEntries(itemIds: readonly string[]) { return itemIds.map((id) => ({ kind: "item" as const, id })); } + +const idleVirtualizer = { isScrolling: false, scrollDirection: null } as const; + +/** + * Renders a MessageList wired for the scroll-compensation tests and returns + * the intercepted size-change predicate plus a `commit` that re-renders so the + * pending compensation layout effect applies. + */ +function renderCompensationList(actions?: ChatPaneActions) { + const scrollElement = document.createElement("div"); + scrollElement.scrollTop = 160; + const entries = makeEntries(["item-1", "item-2", "item-3", "item-4"]); + // A fresh element per render: re-passing the identical element would let + // React bail out and skip the commit the compensation effect runs in. + const makeUi = () => { + const list = ( + + ); + return actions ? ( + {list} + ) : ( + list + ); + }; + const { rerender } = render(makeUi()); + const virtualizer = useVirtualizerMock.mock.results[0]!.value; + return { + scrollElement, + shouldAdjust: virtualizer.shouldAdjustScrollPositionOnItemSizeChange!, + commit: () => rerender(makeUi()), + }; +} diff --git a/src/renderer/components/thread/ChatPane/parts/MessageList.tsx b/src/renderer/components/thread/ChatPane/parts/MessageList.tsx index f9c147c0..ef66f659 100644 --- a/src/renderer/components/thread/ChatPane/parts/MessageList.tsx +++ b/src/renderer/components/thread/ChatPane/parts/MessageList.tsx @@ -47,7 +47,6 @@ interface MessageListProps { const CHAT_TRANSCRIPT_OVERSCAN = 8; const DEFAULT_ROW_ESTIMATE_PX = 96; -const UPWARD_SCROLL_MEASUREMENT_SUPPRESSION_MS = 750; const SKIP_REVERT_CONFIRM_PREF_KEY = "lightcode-chat-checkpoint-revert-skip-confirm"; // Intentionally not wrapped in `React.memo`: pane swaps preserve this fiber @@ -65,8 +64,7 @@ export function MessageList({ const parentActions = useChatPaneActions(); const virtualSizeBoxRef = useRef(null); const rowElementsRef = useRef(new Map()); - const lastScrollTopRef = useRef(0); - const suppressUpwardSizeAdjustmentUntilRef = useRef(0); + const pendingScrollCompensationRef = useRef(0); const [measureEpoch, setMeasureEpoch] = useState(0); const [pendingRevertItemId, setPendingRevertItemId] = useState(null); const [dontAskAgain, setDontAskAgain] = useState(false); @@ -97,42 +95,35 @@ export function MessageList({ }, [entries.length, parentActions, virtualizer]); useLayoutEffect(() => { - if (!scrollElement) return; - lastScrollTopRef.current = scrollElement.scrollTop; - - const handleScroll = () => { - const prevScrollTop = lastScrollTopRef.current; - const nextScrollTop = scrollElement.scrollTop; - lastScrollTopRef.current = nextScrollTop; - if (nextScrollTop < prevScrollTop) { - suppressUpwardSizeAdjustmentUntilRef.current = - performance.now() + UPWARD_SCROLL_MEASUREMENT_SUPPRESSION_MS; - } - }; - - scrollElement.addEventListener("scroll", handleScroll, { passive: true }); - return () => scrollElement.removeEventListener("scroll", handleScroll); - }, [scrollElement]); - - useLayoutEffect(() => { - // Adjust scroll only when the resized row is fully above the viewport. - // This is what keeps visible content anchored as overscan rows replace - // their estimated heights with real measurements — critical for smooth - // idle reading. During and shortly after upward scrolling, suppress the - // compensation so delayed row measurements do not pull the transcript. - virtualizer.shouldAdjustScrollPositionOnItemSizeChange = (item, _delta, instance) => { + // Compensate scroll when a row above the viewport (or any row while + // bottom-sticky) trades its estimated height for a real measurement — + // otherwise the estimate error shifts the visible content. Always return + // false so TanStack never calls scrollTo itself: that adjustment runs + // outside React's commit and paints one frame apart from the translateY / + // row-height change, which reads as flicker. Instead the delta is + // accumulated here and applied in the layout effect below, in the same + // commit (and therefore the same paint) as the DOM change it compensates. + virtualizer.shouldAdjustScrollPositionOnItemSizeChange = (item, delta) => { if (!scrollElement) return false; - const now = performance.now(); - if (instance.isScrolling && instance.scrollDirection === "backward") return false; - if (now <= suppressUpwardSizeAdjustmentUntilRef.current) return false; - if (parentActions?.isStickToBottom?.()) return true; - return item.start + item.size <= scrollElement.scrollTop; + if (parentActions?.isStickToBottom?.() || item.start + item.size <= scrollElement.scrollTop) { + pendingScrollCompensationRef.current += delta; + } + return false; }; return () => { virtualizer.shouldAdjustScrollPositionOnItemSizeChange = undefined; }; }, [parentActions, scrollElement, virtualizer]); + // Intentionally dependency-free: runs after every commit, once row refs have + // synchronously measured newly mounted rows, so the scroll correction lands + // before the browser paints the row's real height. + useLayoutEffect(() => { + if (pendingScrollCompensationRef.current === 0 || !scrollElement) return; + scrollElement.scrollTop += pendingScrollCompensationRef.current; + pendingScrollCompensationRef.current = 0; + }); + useLayoutEffect(() => { const virtualSizeBox = virtualSizeBoxRef.current; if (!virtualSizeBox) return; @@ -182,10 +173,20 @@ export function MessageList({ (index: number, element: HTMLDivElement | null) => { if (element) { rowElementsRef.current.set(index, element); + virtualizer.measureElement(element); + // TanStack defers ref-time measurement to the next ResizeObserver + // frame while scrolling, but the row's real DOM height is already on + // screen this commit — a frame with the old scroll offset would paint + // shifted. Measure synchronously so the estimate correction (and the + // pending scroll compensation it accumulates) applies pre-paint. + virtualizer.resizeItem( + index, + virtualizer.options.measureElement(element, undefined, virtualizer), + ); } else { rowElementsRef.current.delete(index); + virtualizer.measureElement(element); } - virtualizer.measureElement(element); }, [virtualizer], );