diff --git a/src/browser/components/ChatPane/ChatPane.tsx b/src/browser/components/ChatPane/ChatPane.tsx index 2537848b38..29e2a086dc 100644 --- a/src/browser/components/ChatPane/ChatPane.tsx +++ b/src/browser/components/ChatPane/ChatPane.tsx @@ -386,6 +386,7 @@ export const ChatPane: React.FC = (props) => { jumpToBottom, handleScroll, markUserScrollIntent, + handleScrollContainerWheel, handleScrollContainerMouseDown, handleScrollContainerMouseMove, handleScrollContainerMouseUp, @@ -836,7 +837,7 @@ export const ChatPane: React.FC = (props) => {
; } +function createWheelEvent( + element: HTMLDivElement, + options: { deltaX?: number; deltaY?: number } = {} +): WheelEvent { + return { + currentTarget: element, + target: element, + deltaX: options.deltaX ?? 0, + deltaY: options.deltaY ?? 0, + } as unknown as WheelEvent; +} + let scheduledFrames: Array<{ id: number; callback: FrameRequestCallback }> = []; let nextFrameId = 1; @@ -167,6 +179,264 @@ describe("useAutoScroll", () => { } }); + test("small wheel-up tick releases the lock on the first event without snap-back", () => { + // Regression: a slow wheel-up gesture from the very bottom (~3-7 px per + // notch) used to keep the lock engaged because the user-intent branch + // treated "still ≤ USER_BOTTOM_RELOCK_THRESHOLD_PX from bottom" as + // "relock". The rAF settle tick then wrote scrollTop = max on the next + // frame, snapping the user back to the bottom mid-gesture. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + initialScrollTop: 600, + }); + + const dateNowSpy = spyOn(Date, "now"); + try { + let now = 1_000_000; + dateNowSpy.mockImplementation(() => now); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + }); + expect(metrics.scrollTop).toBe(metrics.maxScrollTop); + + // Single small wheel notch: scrollTop drops 5 px, well within the 8 px + // relock threshold. Lock must release on the first event. + metrics.setScrollTop(metrics.maxScrollTop - 5); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + expect(result.current.autoScroll).toBe(false); + + // Subsequent rAF ticks must not snap the user back to the bottom. + act(() => { + flushFrames(5); + }); + expect(metrics.scrollTop).toBe(595); + } finally { + dateNowSpy.mockRestore(); + } + }); + + test("continued slow wheel-up does not relock while still moving away from bottom", () => { + // Even after the lock releases, geometry alone (≤ 8 px from bottom) must + // not relock while the user is still scrolling upward in the same intent + // window. Direction matters: the second tick lands at 6 px from bottom + // but the user is moving away, so the lock must stay off. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + initialScrollTop: 600, + }); + + const dateNowSpy = spyOn(Date, "now"); + try { + let now = 1_000_000; + dateNowSpy.mockImplementation(() => now); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + }); + + // First tick: 3 px up. Releases the lock. + metrics.setScrollTop(metrics.maxScrollTop - 3); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + expect(result.current.autoScroll).toBe(false); + + // Second tick: 3 px further up (still within 8 px of bottom, but moving + // away). Lock must remain released. + metrics.setScrollTop(metrics.maxScrollTop - 6); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + expect(result.current.autoScroll).toBe(false); + + act(() => { + flushFrames(5); + }); + expect(metrics.scrollTop).toBe(594); + } finally { + dateNowSpy.mockRestore(); + } + }); + + test("scrolling back toward bottom within user intent re-engages the lock", () => { + // The release path must not regress the "scroll back to bottom and the + // lock re-engages immediately" behavior. After the user scrolls up, a + // direction reversal toward the bottom that lands within 8 px must relock + // without waiting for the intent window to expire. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + initialScrollTop: 600, + }); + + const dateNowSpy = spyOn(Date, "now"); + try { + let now = 1_000_000; + dateNowSpy.mockImplementation(() => now); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + }); + + // User scrolls up far enough to release. + metrics.setScrollTop(400); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + expect(result.current.autoScroll).toBe(false); + + // User reverses direction and scrolls back within 8 px of the bottom + // while the intent window is still open. + metrics.setScrollTop(metrics.maxScrollTop - 4); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + expect(result.current.autoScroll).toBe(true); + } finally { + dateNowSpy.mockRestore(); + } + }); + + test("disableAutoScroll seeds direction baseline so a later wheel-up does not relock", () => { + // Regression: without seeding lastScrollTopRef inside disableAutoScroll, + // the released-branch user-intent direction check compared the next + // user-driven scroll event against a stale value (often 0 from cold-start + // or the previous workspace's residue). A small wheel-up notch (~5 px) + // landing within 8 px of the new bottom would then look like "moving + // toward bottom" (e.g. 895 > 0) and spuriously relock the lock that the + // explicit programmatic disable just turned off. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + initialScrollTop: 600, + }); + + const dateNowSpy = spyOn(Date, "now"); + try { + let now = 1_000_000; + dateNowSpy.mockImplementation(() => now); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + }); + // Pin lastScrollTopRef to 0 by skipping any prior handleScroll calls, + // mimicking the pre-mount or workspace-switch cold-start path. Then + // disableAutoScroll is invoked from a hypothetical "edit last user + // message" or "navigate to message" flow. + act(() => { + result.current.disableAutoScroll(); + }); + expect(result.current.autoScroll).toBe(false); + + // User then bumps the trackpad: wheel handler primes intent (which + // also clears programmaticDisableRef), and a small wheel-up notch + // brings scrollTop to 595 — still within 8 px of the new bottom but + // strictly moving away from it. + metrics.setScrollTop(metrics.maxScrollTop - 5); + act(() => { + result.current.markUserScrollIntent(); + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + // The released branch must read previousScrollTop = 600 (the seeded + // value), see currentScrollTop = 595, and conclude the user is moving + // away from bottom — no relock. + expect(result.current.autoScroll).toBe(false); + expect(metrics.scrollTop).toBe(595); + } finally { + dateNowSpy.mockRestore(); + } + }); + + test("delta-0 wheel events do not open a user-intent window", () => { + // Modifier-key wheel (Cmd-wheel zoom on macOS), Shift-wheel for horizontal + // scroll, and Bluetooth-mouse jitter all dispatch wheel events with + // deltaY === 0 (and often deltaX === 0). These must not clear + // programmaticDisableRef or refresh the 750 ms intent window — otherwise + // any layout drift that follows takes the wrong branch in handleScroll. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + }); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + // Phantom wheel: zero deltas. Must be a no-op for the intent window. + result.current.handleScrollContainerWheel(createWheelEvent(element, { deltaY: 0 })); + }); + + // Now produce a layout drift. With no intent window open, handleScroll + // takes the no-intent locked branch and snaps back to bottom — proving + // the wheel did not open an intent window (otherwise we'd see the + // user-intent locked branch release the lock instead). + metrics.setScrollTop(metrics.maxScrollTop - 5); + act(() => { + result.current.handleScroll(createScrollEvent(element)); + }); + + expect(metrics.scrollTop).toBe(metrics.maxScrollTop); + expect(result.current.autoScroll).toBe(true); + }); + + test("non-zero wheel events do open a user-intent window", () => { + // Sanity-check that the wheel filter doesn't over-block. A real wheel + // event with deltaY != 0 must still mark intent and let the next scroll + // event release the lock. + const { result } = renderHook(() => useAutoScroll()); + const element = document.createElement("div"); + const metrics = attachScrollMetrics(element, { + scrollHeight: 1000, + clientHeight: 400, + }); + + const dateNowSpy = spyOn(Date, "now"); + try { + let now = 1_000_000; + dateNowSpy.mockImplementation(() => now); + + act(() => { + (result.current.contentRef as MutableRefObject).current = element; + result.current.handleScrollContainerWheel(createWheelEvent(element, { deltaY: -100 })); + }); + + metrics.setScrollTop(metrics.maxScrollTop - 5); + act(() => { + now += 1; + result.current.handleScroll(createScrollEvent(element)); + }); + + expect(result.current.autoScroll).toBe(false); + expect(metrics.scrollTop).toBe(metrics.maxScrollTop - 5); + } finally { + dateNowSpy.mockRestore(); + } + }); + test("returning to bottom geometry re-acquires the lock and rAF resumes pinning", () => { const { result } = renderHook(() => useAutoScroll()); const element = document.createElement("div"); diff --git a/src/browser/hooks/useAutoScroll.ts b/src/browser/hooks/useAutoScroll.ts index 21270d0e31..dfe455c3c7 100644 --- a/src/browser/hooks/useAutoScroll.ts +++ b/src/browser/hooks/useAutoScroll.ts @@ -1,4 +1,4 @@ -import type { KeyboardEvent, MouseEvent, UIEvent } from "react"; +import type { KeyboardEvent, MouseEvent, UIEvent, WheelEvent } from "react"; import { useCallback, useEffect, useRef, useState } from "react"; const BOTTOM_LOCK_EPSILON_PX = 1; @@ -71,6 +71,12 @@ export function useAutoScroll() { const autoScrollRef = useRef(true); const programmaticDisableRef = useRef(false); const userScrollIntentUntilRef = useRef(0); + // Tracks the scrollTop observed during the previous handleScroll call so the + // user-intent branch can tell "user moving away from bottom" from "user + // moving toward bottom" without consulting wheel/touch deltas. Direction is + // what lets a slow wheel-up gesture release the lock on the first tick + // without the relock heuristic snapping it back to the bottom mid-gesture. + const lastScrollTopRef = useRef(0); const setAutoScrollEnabled = useCallback((enabled: boolean) => { autoScrollRef.current = enabled; @@ -136,6 +142,12 @@ export function useAutoScroll() { programmaticDisableRef.current = false; setAutoScrollEnabled(true); stickToBottom(); + // Seed the direction baseline used by handleScroll's released-branch + // user-intent path. stickToBottom doesn't always emit a scroll event + // (it skips the write when scrollTop is already max), so without this + // seed the next user-driven scroll event could compare against a stale + // value carried across workspace switches or earlier sessions. + lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0; startBottomLockFrameLoop(); }, [setAutoScrollEnabled, startBottomLockFrameLoop, stickToBottom]); @@ -143,6 +155,14 @@ export function useAutoScroll() { userScrollIntentUntilRef.current = 0; programmaticDisableRef.current = true; setAutoScrollEnabled(false); + // Seed the direction baseline. The released-branch user-intent path in + // handleScroll compares the next scroll event's scrollTop against + // lastScrollTopRef. disableAutoScroll never fires a scroll event itself, + // so without this seed a small wheel-up notch following a programmatic + // disable would be misread as "moving toward bottom" (because + // previousScrollTop was 0 or some unrelated earlier value), spuriously + // relocking the lock that was just disabled. + lastScrollTopRef.current = contentRef.current?.scrollTop ?? 0; stopBottomLockFrameLoop(); }, [setAutoScrollEnabled, stopBottomLockFrameLoop]); @@ -151,6 +171,21 @@ export function useAutoScroll() { userScrollIntentUntilRef.current = Date.now() + USER_SCROLL_INTENT_WINDOW_MS; }, []); + const handleScrollContainerWheel = useCallback( + (event: WheelEvent) => { + // Filter delta-0 wheel events before opening a scroll-intent window. + // Modifier-key wheel (Cmd-wheel browser zoom on macOS, Shift-wheel for + // horizontal-only on some setups), pinch gestures, and Bluetooth-mouse + // jitter all dispatch wheel events with deltaY === 0 (and often + // deltaX === 0). Without this filter every such phantom wheel would + // clear programmaticDisableRef and refresh the 750 ms intent window, + // weakening every downstream gate that relies on those refs. + if (event.deltaY === 0 && event.deltaX === 0) return; + markUserScrollIntent(); + }, + [markUserScrollIntent] + ); + const contentMouseDownCandidateRef = useRef(false); const handleScrollContainerMouseDown = useCallback( @@ -214,6 +249,9 @@ export function useAutoScroll() { (e: UIEvent) => { const scrollContainer = e.currentTarget; const now = Date.now(); + const previousScrollTop = lastScrollTopRef.current; + const currentScrollTop = scrollContainer.scrollTop; + lastScrollTopRef.current = currentScrollTop; if (now > userScrollIntentUntilRef.current) { if ( autoScrollRef.current && @@ -235,15 +273,37 @@ export function useAutoScroll() { return; } - // Keep momentum/scrollbar drags in the user-owned window without direction - // bookkeeping. The geometry alone determines whether the tail is owned. + // User-intent window is open (wheel/touch/key/scrollbar within the last + // USER_SCROLL_INTENT_WINDOW_MS). Refresh the window first so momentum + // and scrollbar drags stay user-owned across multiple scroll events. userScrollIntentUntilRef.current = now + USER_SCROLL_INTENT_WINDOW_MS; - const shouldEnableBottomLock = isWithinBottomThreshold( - scrollContainer, - USER_BOTTOM_RELOCK_THRESHOLD_PX - ); - setAutoScrollEnabled(shouldEnableBottomLock); - if (shouldEnableBottomLock) { + + if (autoScrollRef.current) { + // Currently locked. Release on the first pixel of drift away from the + // bottom. Using USER_BOTTOM_RELOCK_THRESHOLD_PX here would let small + // wheel deltas (~3-7 px, typical for a single mousewheel notch) keep + // the lock engaged, and the next rAF settle tick would write + // `scrollTop = max` again — perceived as scroll-up resistance / jitter + // at the start of the gesture until the user accumulates enough delta + // to break past the relock threshold. + if (!isWithinBottomThreshold(scrollContainer, BOTTOM_LOCK_EPSILON_PX)) { + setAutoScrollEnabled(false); + } + return; + } + + // Currently released. Re-engage the lock only when the user is scrolling + // toward the bottom and lands within the relock window. The direction + // check prevents a relock mid-gesture when the user is still scrolling + // up but happens to be ≤ USER_BOTTOM_RELOCK_THRESHOLD_PX from the bottom + // (e.g., the second small wheel tick after the first one already + // released the lock). + const userScrollingTowardBottom = currentScrollTop > previousScrollTop; + if ( + userScrollingTowardBottom && + isWithinBottomThreshold(scrollContainer, USER_BOTTOM_RELOCK_THRESHOLD_PX) + ) { + setAutoScrollEnabled(true); startBottomLockFrameLoop(); } }, @@ -291,6 +351,7 @@ export function useAutoScroll() { jumpToBottom, handleScroll, markUserScrollIntent, + handleScrollContainerWheel, handleScrollContainerMouseDown, handleScrollContainerMouseMove, handleScrollContainerMouseUp,