From b05779573503c042f532bf5064b80efd60f10ee1 Mon Sep 17 00:00:00 2001 From: byGalax Date: Tue, 2 Jun 2026 22:31:39 +0200 Subject: [PATCH] fix(desktop): stop chat opening at top + flicker on switch Make stick-to-bottom intent the single source of truth in MessageList and drive onAtBottomChange from intent, not raw scroll position. A measurement reflow can no longer flip the intent off (RC1), the second scrollPositions writer no longer persists a drifting topmost index while stuck (RC2), and a pin-on-rows layout effect re-pins through the two-phase data swap (RC3). - scrollController: add tested nextStickIntent() state machine - MessageList: input-event-based unstick (wheel/key/touch + scrollbar drag), reveal after 2 stable frames, tabIndex for keyboard nav, remove debug overlay - ConversationPage: reuse resolveInitialAnchor; harden handleRangeChanged Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/desktop/src/components/MessageList.tsx | 239 ++++++++++-------- apps/desktop/src/lib/scrollController.test.ts | 18 +- apps/desktop/src/lib/scrollController.ts | 19 ++ apps/desktop/src/pages/ConversationPage.tsx | 27 +- 4 files changed, 187 insertions(+), 116 deletions(-) diff --git a/apps/desktop/src/components/MessageList.tsx b/apps/desktop/src/components/MessageList.tsx index 50e1b61..b10a3c4 100644 --- a/apps/desktop/src/components/MessageList.tsx +++ b/apps/desktop/src/components/MessageList.tsx @@ -7,12 +7,10 @@ import { useLayoutEffect, useRef, useState, - type MutableRefObject, type ReactNode, - type RefObject, } from 'react'; -import { isNearBottom, isNearTop } from '../lib/scrollController'; +import { isNearBottom, isNearTop, nextStickIntent } from '../lib/scrollController'; import type { VirtuosoRow } from '../pages/ConversationPage'; export interface MessageListHandle { @@ -26,8 +24,8 @@ export interface MessageListProps { computeKey: (row: VirtuosoRow) => string; /** Initial scroll target for a freshly-mounted list. */ initialAnchor: { type: 'bottom' } | { type: 'row'; index: number }; - /** Reveal gate — the list stays hidden behind a spinner until true, so the - * post-paint height cascade (reactions/divider) is never visible. */ + /** Reveal gate — the list stays hidden until reactions/heights are loaded, so + * the post-paint height cascade is never visible. */ ready: boolean; estimateRowHeight?: number; atBottomThreshold?: number; @@ -53,19 +51,30 @@ export const MessageList = forwardRef(funct ) { const scrollElRef = useRef(null); const [revealed, setRevealed] = useState(false); - const atBottomRef = useRef(true); - // Intent: keep the view pinned to the bottom? Only a genuine user scroll flips - // this — measurement reflows must not. + + // THE single source of truth: should the view stay pinned to the bottom? + // Only a genuine user up-input (wheel / key / touch / scrollbar drag) turns + // this OFF; only reaching the bottom turns it ON. A measurement reflow must + // never flip it — that was the root cause of the chat-switch bug. const stickRef = useRef(true); - // Guard: scroll events we cause (pin / measure re-pin) fire a tick after we set - // scrollTop. handleScroll ignores events within this window so a measurement - // reflow is never mistaken for the user scrolling up. + // Debounce for onAtBottomChange — fire the parent only on a real transition. + const lastReportedAtBottomRef = useRef(null); + // Guard: scrolls WE cause (pin / measure re-pin / scrollToIndex) fire onScroll + // a tick later. Within this window we don't treat a scrollTop decrease as the + // user dragging up. const programmaticRef = useRef(0); + // Previous scrollTop, to detect a genuine scrollbar/keyboard up-drag. + const lastScrollTopRef = useRef(0); // Load-older preservation: remember the first row key + scrollHeight so a // prepend can be detected and the viewport restored. const prevFirstKeyRef = useRef(null); const prevScrollHeightRef = useRef(0); + // Latest onAtBottomChange, read through a ref so the input-listener effect + // can stay mounted once (deps []) without capturing a stale callback. + const onAtBottomChangeRef = useRef(onAtBottomChange); + onAtBottomChangeRef.current = onAtBottomChange; + const virtualizer = useVirtualizer({ count: rows.length, getScrollElement: () => scrollElRef.current, @@ -86,13 +95,28 @@ export const MessageList = forwardRef(funct if (!el) return; programmaticRef.current = performance.now(); el.scrollTop = el.scrollHeight; + lastScrollTopRef.current = el.scrollTop; }, []); + // Report at-bottom to the parent only on a true transition, always driven by + // the INTENT (stickRef) — never the raw position. This is what kills the + // feedback loop: a transient "not at bottom" mid-reflow is never persisted. + const reportAtBottom = useCallback((atBottom: boolean) => { + if (lastReportedAtBottomRef.current === atBottom) return; + lastReportedAtBottomRef.current = atBottom; + onAtBottomChangeRef.current?.(atBottom); + }, []); + + // A genuine user up-input: drop the stick intent immediately. + const markUserMovedUp = useCallback(() => { + if (!stickRef.current) return; + stickRef.current = false; + reportAtBottom(false); + }, [reportAtBottom]); + // Re-pin to the true bottom whenever the content (or viewport) resizes while - // sticking. ResizeObserver fires after layout / before paint, so as rows measure - // and the list grows the bottom stays pinned with no stale frame — THIS is what - // carries the list to the bottom through the measure settle (the old - // getTotalSize effect lagged a frame, letting onScroll wrongly flip atBottom). + // sticking. ResizeObserver fires after layout / before paint, so as rows + // measure and the list grows the bottom stays pinned with no stale frame. useEffect(() => { const el = scrollElRef.current; if (!el) return; @@ -101,6 +125,7 @@ export const MessageList = forwardRef(funct if (stickRef.current && e) { programmaticRef.current = performance.now(); e.scrollTop = e.scrollHeight; + lastScrollTopRef.current = e.scrollTop; } }); ro.observe(el); @@ -109,10 +134,42 @@ export const MessageList = forwardRef(funct return () => ro.disconnect(); }, []); + // Genuine-user-intent listeners. These are the ONLY way (besides reaching the + // bottom) the stick intent turns off, so a reflow can never unstick the list. + useEffect(() => { + const el = scrollElRef.current; + if (!el) return; + const onWheel = (e: WheelEvent) => { + if (e.deltaY < 0) markUserMovedUp(); + }; + const onKeyDown = (e: KeyboardEvent) => { + if (e.key === 'PageUp' || e.key === 'Home' || e.key === 'ArrowUp') markUserMovedUp(); + }; + let touchStartY = 0; + const onTouchStart = (e: TouchEvent) => { + touchStartY = e.touches[0]?.clientY ?? 0; + }; + const onTouchMove = (e: TouchEvent) => { + const y = e.touches[0]?.clientY ?? 0; + // Finger dragged DOWN (content scrolls up toward older messages). Guard on + // scrollTop>0 so an overscroll bounce at the bottom doesn't unstick. + if (y - touchStartY > 8 && (scrollElRef.current?.scrollTop ?? 0) > 0) markUserMovedUp(); + }; + el.addEventListener('wheel', onWheel, { passive: true }); + el.addEventListener('keydown', onKeyDown); + el.addEventListener('touchstart', onTouchStart, { passive: true }); + el.addEventListener('touchmove', onTouchMove, { passive: true }); + return () => { + el.removeEventListener('wheel', onWheel); + el.removeEventListener('keydown', onKeyDown); + el.removeEventListener('touchstart', onTouchStart); + el.removeEventListener('touchmove', onTouchMove); + }; + }, [markUserMovedUp]); + // Deferred reveal: when ready, pin to the anchor and keep pinning each frame - // until the list height has SETTLED (rows measure over 1-2 frames and grow it), - // THEN reveal — so what appears is already at the final bottom, with no - // top-then-jump flicker. The ResizeObserver above keeps it pinned afterwards. + // until the list height has SETTLED over two consecutive frames, THEN reveal — + // so what appears is already at its final position with no top-then-jump. useLayoutEffect(() => { if (!ready || revealed || rows.length === 0) return; const el = scrollElRef.current; @@ -123,36 +180,42 @@ export const MessageList = forwardRef(funct ); if (initialAnchor.type === 'bottom') { stickRef.current = true; - atBottomRef.current = true; pinToBottom(); } else { stickRef.current = false; - atBottomRef.current = false; + programmaticRef.current = performance.now(); virtualizer.scrollToIndex(rowIdx, { align: 'start' }); } - onAtBottomChange?.(atBottomRef.current); + reportAtBottom(stickRef.current); + let prevSH = -1; + let stableFrames = 0; const settle = (attempts: number): void => { const e = scrollElRef.current; if (!e) { setRevealed(true); return; } + programmaticRef.current = performance.now(); if (stickRef.current) { - programmaticRef.current = performance.now(); e.scrollTop = e.scrollHeight; + lastScrollTopRef.current = e.scrollTop; } else { virtualizer.scrollToIndex(rowIdx, { align: 'start' }); } const sh = e.scrollHeight; - if (sh === prevSH || attempts <= 0) { + // Require TWO consecutive stable-height frames: a single stable frame can + // land mid-cascade (between reactions and the unread divider measuring) + // and reveal a not-yet-final layout that then jumps. + stableFrames = sh === prevSH ? stableFrames + 1 : 0; + prevSH = sh; + if (stableFrames >= 2 || attempts <= 0) { setRevealed(true); } else { - prevSH = sh; requestAnimationFrame(() => settle(attempts - 1)); } }; - requestAnimationFrame(() => settle(10)); + requestAnimationFrame(() => settle(12)); // eslint-disable-next-line react-hooks/exhaustive-deps }, [ready, rows.length]); @@ -166,6 +229,7 @@ export const MessageList = forwardRef(funct const delta = el.scrollHeight - prevScrollHeightRef.current; if (delta > 0 && el.scrollTop < atBottomThreshold * 4) { el.scrollTop += delta; + lastScrollTopRef.current = el.scrollTop; } } prevFirstKeyRef.current = firstKey; @@ -173,56 +237,69 @@ export const MessageList = forwardRef(funct // eslint-disable-next-line react-hooks/exhaustive-deps }, [rows]); + // Re-pin on any rows change while sticking. Covers the two-phase data swap + // (the cached array is replaced by the freshly-decrypted one ~100ms after + // reveal) which the ResizeObserver can miss when the new content happens to + // measure to the same height. + useLayoutEffect(() => { + if (revealed && stickRef.current) pinToBottom(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [rows]); + const handleScroll = useCallback(() => { - // Ignore scroll events we triggered (pin / measure re-pin); they fire a tick - // after we set scrollTop. Only a genuine user scroll updates the stick intent — - // otherwise a measurement reflow wrongly flips atBottom and stops the pinning. - if (performance.now() - programmaticRef.current < 120) return; const m = readMetrics(); - const atBottom = isNearBottom(m, atBottomThreshold); - stickRef.current = atBottom; - if (atBottom !== atBottomRef.current) { - atBottomRef.current = atBottom; - onAtBottomChange?.(atBottom); - } + const programmatic = performance.now() - programmaticRef.current < 120; + const nearBottom = isNearBottom(m, atBottomThreshold); + // A scrollbar drag or keyboard scroll surfaces here as a scrollTop decrease. + // Suppress it inside the programmatic window so our own re-pin / settle is + // never mistaken for the user moving up. 2px deadzone absorbs sub-pixel jitter. + const userMovedUp = !programmatic && m.scrollTop < lastScrollTopRef.current - 2; + lastScrollTopRef.current = m.scrollTop; + + stickRef.current = nextStickIntent(stickRef.current, { nearBottom, userMovedUp }); + reportAtBottom(stickRef.current); + if (isNearTop(m, atBottomThreshold * 4)) onReachTop?.(); const first = virtualizer.getVirtualItems()[0]; if (first) onTopRowChange?.(first.index); - }, [atBottomThreshold, onAtBottomChange, onReachTop, onTopRowChange, readMetrics, virtualizer]); + }, [atBottomThreshold, onReachTop, onTopRowChange, readMetrics, reportAtBottom, virtualizer]); useImperativeHandle( ref, () => ({ scrollToBottom: () => { stickRef.current = true; - atBottomRef.current = true; + reportAtBottom(true); pinToBottom(); }, scrollToRow: (index, align = 'center') => { + // The user is jumping to a specific row — drop the stick intent first so + // the ResizeObserver doesn't immediately drag the target back to the bottom. + stickRef.current = false; + reportAtBottom(false); + programmaticRef.current = performance.now(); virtualizer.scrollToIndex(index, { align }); }, }), // eslint-disable-next-line react-hooks/exhaustive-deps - [virtualizer, rows.length], + [virtualizer, rows.length, reportAtBottom, pinToBottom], ); const items = virtualizer.getVirtualItems(); return ( - <> - virtualizer.getTotalSize()} - atBottomRef={atBottomRef} - revealed={revealed} - ready={ready} - /> -
+
{items.map((vi) => (
(funct
))}
- {/* 12px bottom breathing space (matches the old Virtuoso Footer). */} -
-
- - ); -}); - -interface ScrollDebugOverlayProps { - scrollElRef: RefObject; - getTotal: () => number; - atBottomRef: MutableRefObject; - revealed: boolean; - ready: boolean; -} - -// TEMP on-screen debug readout — remove after the chat-switch scroll bug is fixed. -function ScrollDebugOverlay({ - scrollElRef, - getTotal, - atBottomRef, - revealed, - ready, -}: ScrollDebugOverlayProps) { - const [, force] = useState(0); - useEffect(() => { - let raf = 0; - const loop = () => { - force((n) => (n + 1) % 1_000_000); - raf = requestAnimationFrame(loop); - }; - raf = requestAnimationFrame(loop); - return () => cancelAnimationFrame(raf); - }, []); - const el = scrollElRef.current; - const sh = el?.scrollHeight ?? 0; - const ch = el?.clientHeight ?? 0; - const st = el ? Math.round(el.scrollTop) : 0; - return ( -
- {`scrollHeight=${sh} clientHeight=${ch}\nscrollTop=${st}\ntotalSize=${Math.round(getTotal())}\nSCROLLABLE=${sh > ch + 4 ? 'YES' : 'NO'}\natBottom=${atBottomRef.current} revealed=${revealed} ready=${ready}`} + {/* 12px bottom breathing space (matches the old Virtuoso Footer). */} +
); -} +}); diff --git a/apps/desktop/src/lib/scrollController.test.ts b/apps/desktop/src/lib/scrollController.test.ts index 8d8e208..37c7a76 100644 --- a/apps/desktop/src/lib/scrollController.test.ts +++ b/apps/desktop/src/lib/scrollController.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { isNearBottom, isNearTop, resolveInitialAnchor } from './scrollController'; +import { isNearBottom, isNearTop, nextStickIntent, resolveInitialAnchor } from './scrollController'; const m = (scrollTop: number, scrollHeight: number, clientHeight: number) => ({ scrollTop, @@ -55,3 +55,19 @@ describe('resolveInitialAnchor', () => { expect(resolveInitialAnchor(null, 0)).toEqual({ index: 0, align: 'end' }); }); }); + +describe('nextStickIntent', () => { + it('turns ON when the bottom is reached', () => { + expect(nextStickIntent(false, { nearBottom: true, userMovedUp: false })).toBe(true); + }); + it('turns OFF when the user genuinely moves up', () => { + expect(nextStickIntent(true, { nearBottom: false, userMovedUp: true })).toBe(false); + }); + it('keeps the previous intent on a neutral scroll (measurement reflow)', () => { + expect(nextStickIntent(true, { nearBottom: false, userMovedUp: false })).toBe(true); + expect(nextStickIntent(false, { nearBottom: false, userMovedUp: false })).toBe(false); + }); + it('reaching the bottom wins over a simultaneous move-up signal', () => { + expect(nextStickIntent(false, { nearBottom: true, userMovedUp: true })).toBe(true); + }); +}); diff --git a/apps/desktop/src/lib/scrollController.ts b/apps/desktop/src/lib/scrollController.ts index 95f08d5..78f9614 100644 --- a/apps/desktop/src/lib/scrollController.ts +++ b/apps/desktop/src/lib/scrollController.ts @@ -41,3 +41,22 @@ export function resolveInitialAnchor(saved: SavedPosition | null, rowCount: numb } return { index: rowCount - 1, align: 'end' }; } + +/** + * The next stick-to-bottom intent given the current intent and the latest + * scroll signal. Intent only flips on a *definitive* signal: + * - reaching the bottom turns it ON, + * - a genuine user move-up turns it OFF. + * A neutral scroll — e.g. a measurement reflow that grows the content while + * rows settle — leaves the intent unchanged. This is the core fix for the + * chat-switch bug: a reflow must never be mistaken for the user scrolling up + * and so must never silently unstick the list. + */ +export function nextStickIntent( + prev: boolean, + signal: { nearBottom: boolean; userMovedUp: boolean }, +): boolean { + if (signal.nearBottom) return true; + if (signal.userMovedUp) return false; + return prev; +} diff --git a/apps/desktop/src/pages/ConversationPage.tsx b/apps/desktop/src/pages/ConversationPage.tsx index d8b00ed..d4c143c 100644 --- a/apps/desktop/src/pages/ConversationPage.tsx +++ b/apps/desktop/src/pages/ConversationPage.tsx @@ -54,6 +54,7 @@ import { createWatchTogetherPayload, createGamePayload, } from '../lib/conversationFeatures'; +import { resolveInitialAnchor } from '../lib/scrollController'; const WhiteboardModal = lazy(() => import('../components/WhiteboardModal').then((m) => ({ default: m.WhiteboardModal })), ); @@ -485,11 +486,14 @@ export function ConversationPage() { // previous visit to this chat AND the user wasn't sticking to the // bottom, restore the saved row index (clamped to the current row // count in case the cache was trimmed). + // Reuses the unit-tested resolveInitialAnchor so the "where do I open" rule + // lives in one tested place. MessageList only reads this at reveal time (when + // rows are loaded), so depending on the row count clamps a stale saved index + // correctly without freezing a mount-time count of 0. const initialAnchor = useMemo<{ type: 'bottom' } | { type: 'row'; index: number }>(() => { - const saved = savedPositionRef.current; - if (saved && !saved.stickToBottom) return { type: 'row', index: saved.topmostIndex }; - return { type: 'bottom' }; - }, []); + const anchor = resolveInitialAnchor(savedPositionRef.current, virtuosoRows.length); + return anchor.align === 'end' ? { type: 'bottom' } : { type: 'row', index: anchor.index }; + }, [virtuosoRows.length]); const jumpToMessage = useCallback( (targetId: string) => { @@ -712,15 +716,22 @@ export function ConversationPage() { const handleRangeChanged = useCallback( (range: { startIndex: number; endIndex: number }) => { topmostIndexRef.current = range.startIndex; - if (id) { + if (!id) return; + if (stickToBottom) { + // Pinned to the bottom: the topmost-visible row drifts as the + // virtualizer mounts/unmounts rows. Persisting it would later reopen the + // chat at that arbitrary row (the second writer behind the scrollTop=0 + // bug). Keep the last real reading position and only refresh the flag. const prev = scrollPositions.get(id); scrollPositions.set(id, { - topmostIndex: range.startIndex, - stickToBottom: prev?.stickToBottom ?? true, + topmostIndex: prev?.topmostIndex ?? range.startIndex, + stickToBottom: true, }); + } else { + scrollPositions.set(id, { topmostIndex: range.startIndex, stickToBottom: false }); } }, - [id], + [id, stickToBottom], ); const jumpToBottom = useCallback(() => {