From 9c2dbab2fc305f5c0298478e0e91df54920988eb Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 28 Apr 2026 12:45:27 -0500 Subject: [PATCH] PAP-2660: Bind issue chat virtualizer to
scroll container The Phase 3 virtualizer (PAP-2645) used useWindowVirtualizer, but on the real issue page the chat lives inside `
`. Window scroll never moves on desktop, so the virtualizer only ever mounted the first 7 rows and the chat area went blank past the first viewport. Detect the actual scroll container at mount via DOM walk and rekey the inner virtualizer to use element-mode observers/scroll-fn when an overflow ancestor exists; fall back to window mode for mobile and the auth-free perf fixture, where the document scrolls. Adds a regression test that mounts IssueChatThread inside an overflow-auto ancestor and asserts jump-to-latest drives that element's scrollTo, not window.scrollTo. Co-Authored-By: Paperclip --- ui/src/components/IssueChatThread.test.tsx | 62 ++++++++++ ui/src/components/IssueChatThread.tsx | 126 +++++++++++++++++++-- 2 files changed, 180 insertions(+), 8 deletions(-) diff --git a/ui/src/components/IssueChatThread.test.tsx b/ui/src/components/IssueChatThread.test.tsx index 429ff516f0d..43cc8f97f53 100644 --- a/ui/src/components/IssueChatThread.test.tsx +++ b/ui/src/components/IssueChatThread.test.tsx @@ -469,6 +469,68 @@ describe("IssueChatThread", () => { }); }); + // Regression for PAP-2660: on the real issue page the chat thread is wrapped + // in `
`, so the virtualizer must bind + // to that ancestor's scroll instead of `window` (which never moves on + // desktop). When mounted inside an overflow-auto ancestor the jump-to-latest + // action must drive that element's scrollTo, not window.scrollTo. + it("targets an overflow-auto ancestor instead of window scroll on jump-to-latest", () => { + container.remove(); + const scrollHost = document.createElement("main"); + scrollHost.id = "main-content"; + scrollHost.style.overflowY = "auto"; + scrollHost.style.overflow = "auto"; + scrollHost.style.height = "640px"; + document.body.appendChild(scrollHost); + container = document.createElement("div"); + scrollHost.appendChild(container); + + const root = createRoot(container); + const windowScrollToMock = vi.spyOn(window, "scrollTo").mockImplementation(() => {}); + const elementScrollToMock = vi.fn(); + scrollHost.scrollTo = elementScrollToMock as unknown as typeof scrollHost.scrollTo; + + act(() => { + root.render( + + {}} + enableLiveTranscriptPolling={false} + transcriptsByRunId={issueChatLongThreadTranscriptsByRunId} + hasOutputForRun={(runId) => issueChatLongThreadTranscriptsByRunId.has(runId)} + /> + , + ); + }); + + const jump = Array.from(container.querySelectorAll("button")).find( + (button) => button.textContent === "Jump to latest", + ) as HTMLButtonElement | undefined; + expect(jump).toBeDefined(); + + windowScrollToMock.mockClear(); + elementScrollToMock.mockClear(); + + act(() => { + jump?.click(); + }); + + expect(elementScrollToMock.mock.calls.some(([arg]) => hasSmoothScrollBehavior(arg))).toBe(true); + expect(windowScrollToMock.mock.calls.some(([arg]) => hasSmoothScrollBehavior(arg))).toBe(false); + + windowScrollToMock.mockRestore(); + act(() => { + root.unmount(); + }); + scrollHost.remove(); + }); + it("keeps the direct render path for short threads under the virtualization threshold", () => { const root = createRoot(container); const directComments = issueChatLongThreadComments.slice(0, 12); diff --git a/ui/src/components/IssueChatThread.tsx b/ui/src/components/IssueChatThread.tsx index 6d61a0d8fb6..7a04ef0d65d 100644 --- a/ui/src/components/IssueChatThread.tsx +++ b/ui/src/components/IssueChatThread.tsx @@ -13,6 +13,7 @@ import { Component, forwardRef, memo, + useCallback, useContext, useEffect, useImperativeHandle, @@ -27,7 +28,12 @@ import { type ReactNode, } from "react"; import { Link, useLocation } from "@/lib/router"; -import { useWindowVirtualizer } from "@tanstack/react-virtual"; +import { + observeWindowOffset, + observeWindowRect, + useVirtualizer, + windowScroll, +} from "@tanstack/react-virtual"; import type { Agent, FeedbackDataSharingPreference, @@ -2148,23 +2154,105 @@ type VirtualizedVisibleAnchorSnapshot = { viewportTop: number; }; -const VirtualizedIssueChatThreadList = forwardRef(function VirtualizedIssueChatThreadList({ +type VirtualizedScrollMode = + | { kind: "window" } + | { kind: "element"; element: HTMLElement }; + +// The chat thread renders inside `
` on the real issue +// page (overflow-auto on desktop), but lives at document scope on mobile (main +// is overflow-visible) and in the auth-free perf fixture. Walk the DOM to find +// the actual scroll container so the virtualizer binds to the right offset +// source — otherwise it stays anchored at offset 0 forever and the visible +// chat area renders blank past the first viewport (PAP-2660). +function findScrollContainer(el: HTMLElement | null): HTMLElement | null { + if (!el || typeof window === "undefined") return null; + let current: HTMLElement | null = el.parentElement; + while (current && current !== document.body && current !== document.documentElement) { + const overflowY = window.getComputedStyle(current).overflowY; + if (overflowY === "auto" || overflowY === "scroll" || overflowY === "overlay") { + return current; + } + current = current.parentElement; + } + return null; +} + +const VirtualizedIssueChatThreadList = forwardRef(function VirtualizedIssueChatThreadList(props, ref) { + const probeRef = useRef(null); + // Default to window scroll on first render so the imperative handle is + // available immediately for hash-target / submit-scroll effects. After mount + // we probe the DOM and remount via key={modeKey} if the actual scroll + // container is an element ancestor (e.g. desktop
). + const [mode, setMode] = useState({ kind: "window" }); + + useLayoutEffect(() => { + if (typeof window === "undefined") return; + const detect = () => { + const probe = probeRef.current; + if (!probe) return; + const container = findScrollContainer(probe); + setMode((prev) => { + if (container === null) { + return prev.kind === "window" ? prev : { kind: "window" }; + } + if (prev.kind === "element" && prev.element === container) return prev; + return { kind: "element", element: container }; + }); + }; + detect(); + window.addEventListener("resize", detect); + return () => { + window.removeEventListener("resize", detect); + }; + }, []); + + return ( + + ); +}); + +interface VirtualizedIssueChatThreadListInnerProps extends VirtualizedIssueChatThreadListProps { + mode: VirtualizedScrollMode; + probeRef: React.MutableRefObject; +} + +const VirtualizedIssueChatThreadListInner = forwardRef< + VirtualizedIssueChatThreadListHandle, + VirtualizedIssueChatThreadListInnerProps +>(function VirtualizedIssueChatThreadListInner({ messages, feedbackVoteByTargetId, activeRunIds, stoppingRunId, interruptingQueuedRunId, variant, + mode, + probeRef, }, ref) { const parentRef = useRef(null); const [scrollMargin, setScrollMargin] = useState(0); const pendingPrependAnchorRef = useRef(null); + const setRefs = useCallback((element: HTMLDivElement | null) => { + parentRef.current = element; + probeRef.current = element; + }, [probeRef]); + useLayoutEffect(() => { const element = parentRef.current; if (!element || typeof window === "undefined") return; const update = () => { - const offset = element.getBoundingClientRect().top + window.scrollY; + if (!parentRef.current) return; + const rect = parentRef.current.getBoundingClientRect(); + const offset = mode.kind === "window" + ? rect.top + window.scrollY + : rect.top - mode.element.getBoundingClientRect().top + mode.element.scrollTop; setScrollMargin((previous) => (Math.abs(previous - offset) < 0.5 ? previous : offset)); }; update(); @@ -2172,13 +2260,31 @@ const VirtualizedIssueChatThreadList = forwardRef { window.removeEventListener("resize", update); }; - }, []); + }, [mode]); const gap = variant === "embedded" ? VIRTUALIZED_THREAD_GAP_EMBEDDED_PX : VIRTUALIZED_THREAD_GAP_FULL_PX; - const virtualizer = useWindowVirtualizer({ + // Window-mode passes window-specific observers/scroll-fn through the same + // useVirtualizer hook (the `Window` vs `Element` type mismatch in the + // observer signatures is invisible at runtime — both reach into + // instance.scrollElement/targetWindow uniformly — so we widen via `any` + // rather than splitting into two component variants). + const virtualizerOptions: Record = mode.kind === "window" + ? { + getScrollElement: () => (typeof document !== "undefined" ? window : null), + observeElementRect: observeWindowRect, + observeElementOffset: observeWindowOffset, + scrollToFn: windowScroll, + initialOffset: () => typeof window !== "undefined" ? window.scrollY : 0, + } + : { + getScrollElement: () => mode.element, + }; + + const virtualizer = useVirtualizer({ + ...(virtualizerOptions as Parameters[0]), count: messages.length, estimateSize: () => VIRTUALIZED_THREAD_ROW_ESTIMATE_PX, overscan: VIRTUALIZED_THREAD_OVERSCAN, @@ -2241,18 +2347,22 @@ const VirtualizedIssueChatThreadList = forwardRef 1) { - window.scrollBy({ top: delta, behavior: "auto" }); + if (mode.kind === "window") { + window.scrollBy({ top: delta, behavior: "auto" }); + } else { + mode.element.scrollBy({ top: delta, behavior: "auto" }); + } } virtualizer.measure(); }); - }, [messages, virtualizer]); + }, [messages, virtualizer, mode]); const virtualItems = virtualizer.getVirtualItems(); const totalSize = virtualizer.getTotalSize(); return (