diff --git a/desktop/src/features/messages/lib/scrollConvergence.test.mjs b/desktop/src/features/messages/lib/scrollConvergence.test.mjs index d85d96146..53c96eaa6 100644 --- a/desktop/src/features/messages/lib/scrollConvergence.test.mjs +++ b/desktop/src/features/messages/lib/scrollConvergence.test.mjs @@ -9,6 +9,7 @@ function input(overrides) { indexByMessageId: new Map([["target", 100]]), lastIssuedIndex: null, librarySettled: false, + stalledOffTarget: false, framesUsed: 0, ...overrides, }; @@ -88,10 +89,59 @@ test("convergenceStep: aiming at current but not yet settled keeps waiting", () input({ lastIssuedIndex: 100, librarySettled: false }), ); assert.equal(step.nextIndex, 100); + assert.equal(step.reissue, false); assert.equal(step.done, false); assert.equal(step.converged, false); }); +// --- off-target stall (liveness) --------------------------------------------- + +test("convergenceStep: stalled off-target while aiming at current re-issues same index", () => { + // The library's offset stopped moving but never reached the current index's + // target (its internal reconcile deadlocked after rows re-measured). The + // reducer signals a same-index re-issue to kick it — the loop continues. + const step = convergenceStep( + input({ lastIssuedIndex: 100, stalledOffTarget: true }), + ); + assert.equal(step.nextIndex, 100); + assert.equal(step.reissue, true); + assert.equal(step.done, false); + assert.equal(step.converged, false); +}); + +test("convergenceStep: a stall reported WHILE re-aiming does not re-issue", () => { + // The index just moved (105) but the library reports a stall on the OLD index + // (100). The reducer re-aims at the new index normally; the stale stall must + // NOT trigger a same-index kick (there is no current-index stall to kick). + const step = convergenceStep( + input({ + indexByMessageId: new Map([["target", 105]]), + lastIssuedIndex: 100, + stalledOffTarget: true, + }), + ); + assert.equal(step.nextIndex, 105); + assert.equal(step.reissue, false); + assert.equal(step.done, false); + assert.equal(step.converged, false); +}); + +test("convergenceStep: a settle takes priority over a concurrent stall flag", () => { + // Defensive: the adapter computes settle and stall as mutually exclusive, but + // if both arrive, a genuine settle must win (converge) rather than spin on a + // pointless re-issue. + const step = convergenceStep( + input({ + lastIssuedIndex: 100, + librarySettled: true, + stalledOffTarget: true, + }), + ); + assert.equal(step.done, true); + assert.equal(step.converged, true); + assert.equal(step.reissue, false); +}); + // --- frame cap --------------------------------------------------------------- test("convergenceStep: terminates at the frame cap without converging", () => { diff --git a/desktop/src/features/messages/lib/scrollConvergence.ts b/desktop/src/features/messages/lib/scrollConvergence.ts index 9ad86e27e..0b8650a3f 100644 --- a/desktop/src/features/messages/lib/scrollConvergence.ts +++ b/desktop/src/features/messages/lib/scrollConvergence.ts @@ -23,6 +23,12 @@ * new index instead of stranding it on the old one. * - If the target id leaves the data mid-settle (deleted), the loop terminates * with `converged: false` rather than chasing a vanished row to the cap. + * + * Plus one liveness property: a large windowed-out jump can leave the library's + * own reconcile deadlocked off-target (offset stable but short of the target + * after rows re-measured under it). When that happens the reducer signals a + * same-index re-issue (`reissue: true`) to restart the library's reconcile — + * the single case where re-issuing an unchanged index is correct. */ /** Where a scroll target should land in the viewport. Mirrors the library's align. */ @@ -40,11 +46,18 @@ export type ConvergenceInput = { */ lastIssuedIndex: number | null; /** - * Whether the library reports its scroll has settled this frame - * (`virtualizer.scrollState === null`). Only meaningful once the library is - * chasing the CURRENT index; a settle reported while re-aiming is ignored. + * Whether the library's offset reached the current index's target this frame + * and stopped moving. Only meaningful once the library is chasing the CURRENT + * index; a settle reported while re-aiming is ignored. */ librarySettled: boolean; + /** + * Whether the library's offset has stopped moving but is NOT at the current + * index's target — it stalled mid-reconcile (its internal re-aim deadlocked + * after rows re-measured under it). The reducer kicks it with a fresh re-issue + * at the same index. Mutually exclusive with `librarySettled`. + */ + stalledOffTarget: boolean; /** Frames already spent in the loop (the adapter increments per rAF). */ framesUsed: number; }; @@ -57,6 +70,13 @@ export type ConvergenceDecision = { * would reset the library's `stableFrames` and prevent it from ever settling). */ nextIndex: number | null; + /** + * True when the adapter must re-issue `scrollToIndex(nextIndex)` even though + * the index is unchanged — used to kick the library out of an off-target + * stall. A normal steady settle leaves this false so no redundant scroll + * resets the library's `stableFrames`. + */ + reissue: boolean; /** True once the loop must stop (settled, target gone, or frame cap hit). */ done: boolean; /** True only when the loop stopped because the target row actually settled. */ @@ -80,7 +100,7 @@ export function convergenceStep(input: ConvergenceInput): ConvergenceDecision { // Target left the data mid-settle (deleted) — stop without converging so the // adapter clears the highlight instead of chasing a vanished row. if (currentIndex === undefined) { - return { nextIndex: null, done: true, converged: false }; + return { nextIndex: null, reissue: false, done: true, converged: false }; } const aimingAtCurrent = input.lastIssuedIndex === currentIndex; @@ -88,16 +108,44 @@ export function convergenceStep(input: ConvergenceInput): ConvergenceDecision { // The library only settles meaningfully once it is chasing the CURRENT index. // A settle reported while we are still re-aiming (index just moved) is stale. if (aimingAtCurrent && input.librarySettled) { - return { nextIndex: currentIndex, done: true, converged: true }; + return { + nextIndex: currentIndex, + reissue: false, + done: true, + converged: true, + }; } // Frame cap: accept the best index we have rather than spin forever on a row // whose height never settles or a target whose index keeps shifting. if (input.framesUsed + 1 >= CONVERGENCE_FRAME_CAP) { - return { nextIndex: currentIndex, done: true, converged: false }; + return { + nextIndex: currentIndex, + reissue: false, + done: true, + converged: false, + }; + } + + // Library stalled off-target: its offset stopped moving but never reached the + // current index (its internal reconcile deadlocked after rows re-measured). + // Re-issue the SAME index to restart its reconcile — the one case where a + // same-index re-issue is correct rather than a stableFrames-resetting bug. + if (aimingAtCurrent && input.stalledOffTarget) { + return { + nextIndex: currentIndex, + reissue: true, + done: false, + converged: false, + }; } // Either the index moved (adapter will re-issue scrollToIndex) or the library // is still settling on the current index (adapter issues nothing, just waits). - return { nextIndex: currentIndex, done: false, converged: false }; + return { + nextIndex: currentIndex, + reissue: false, + done: false, + converged: false, + }; } diff --git a/desktop/src/features/messages/ui/MessageThreadPanel.tsx b/desktop/src/features/messages/ui/MessageThreadPanel.tsx index 46dbf215c..64fddc306 100644 --- a/desktop/src/features/messages/ui/MessageThreadPanel.tsx +++ b/desktop/src/features/messages/ui/MessageThreadPanel.tsx @@ -307,10 +307,6 @@ export function MessageThreadPanel({ }: MessageThreadPanelProps) { const threadBodyRef = React.useRef(null); const threadContentRef = React.useRef(null); - // Threads don't paginate older history, so this sentinel is never observed - // (the hook's older-history effect bails without a `fetchOlder`). It exists - // only to satisfy the hook's required ref contract. - const threadTopSentinelRef = React.useRef(null); const threadComposerWrapperRef = React.useRef(null); const isOverlay = useIsThreadPanelOverlay(); const isFloatingOverlay = isOverlay && !isSinglePanelView; @@ -372,7 +368,6 @@ export function MessageThreadPanel({ messages: threadMessages, onTargetReached: onScrollTargetResolved, scrollContainerRef: threadBodyRef, - sentinelRef: threadTopSentinelRef, targetMessageId: scrollTargetId, }); @@ -392,7 +387,6 @@ export function MessageThreadPanel({ ref={threadBodyRef} >
-
void; @@ -109,6 +111,10 @@ type ChannelIntro = { * message list. Must be module-level so its identity never changes. */ const EMPTY_MESSAGES: TimelineMessage[] = []; +/** Stable empty id->index map for the convergence adapter before the first + * item stream arrives. Module-level so its identity never changes. */ +const EMPTY_INDEX_MAP: Map = new Map(); + type DirectMessageIntroParticipant = { avatarUrl: string | null; displayName: string; @@ -165,6 +171,17 @@ const MessageTimelineBase = React.forwardRef< const contentRef = React.useRef(null); const topSentinelRef = React.useRef(null); + // The convergence fallback for a windowed-out deep-link target. It's defined + // below (it depends on the anchored-scroll result), so `useAnchoredScroll` + // reads it through a ref via a stable wrapper — letting the hook stay + // virtualizer-agnostic while the consumer owns the convergence machinery. + const convergeToTargetRef = React.useRef<(messageId: string) => boolean>( + () => false, + ); + const convergeToTarget = React.useCallback((messageId: string) => { + return convergeToTargetRef.current(messageId); + }, []); + // The virtualizer instance and the flattened item stream are owned by the // child TimelineMessageList (which mounts the VirtualizedList) and reported // up here so the scroll manager can resolve scroll targets by index. The @@ -237,22 +254,19 @@ const MessageTimelineBase = React.forwardRef< isAtBottom, newMessageCount, onScroll, + restoreScrollPosition, scrollToBottom, scrollToBottomOnNextUpdate, scrollToMessage, } = useAnchoredScroll({ channelId, contentRef, - fetchOlder, - hasOlderMessages, - isFetchingOlder, + convergeToTarget, isLoading: showTimelineSkeleton, messages: deferredMessages, onTargetReached, scrollContainerRef, - sentinelRef: topSentinelRef, targetMessageId, - virtualizer: virtualizerOption, }); React.useImperativeHandle( @@ -263,6 +277,48 @@ const MessageTimelineBase = React.forwardRef< [scrollToBottomOnNextUpdate], ); + // Role 3 — jump-to-message into windowed-out history. The DOM-based + // `scrollToMessage` no-ops when the target row isn't mounted (virtualized + // out), so when it fails and the timeline is virtualized we drive the + // convergence adapter: it scrolls the virtualizer to the target index, + // re-aiming each frame as rows mount and measure, then on settle the row is + // in the DOM and `scrollToMessage` centers + highlights it. When there's no + // virtualizer (e.g. the thread panel), there's nothing to converge — the DOM + // path is the whole story and a missing row simply isn't reachable. + const { scrollToMessage: convergeToMessage, cancel: cancelConvergence } = + useConvergentScrollToMessage(getVirtualizer, { + indexByMessageId: timelineItems?.indexByMessageId ?? EMPTY_INDEX_MAP, + align: "center", + onConverged: (messageId) => { + scrollToMessage(messageId, { highlight: true }); + onTargetReached?.(messageId); + }, + }); + const jumpToMessage = React.useCallback( + (messageId: string, options?: { behavior?: ScrollBehavior }) => { + if (scrollToMessage(messageId, { highlight: true, ...options })) { + return; + } + if (virtualizerOption) { + convergeToMessage(messageId); + } + }, + [convergeToMessage, scrollToMessage, virtualizerOption], + ); + // Feed the windowed-out deep-link fallback back into `useAnchoredScroll`, + // which calls it when a target row isn't in the DOM. Gated on the virtualizer + // so the thread panel (no virtualizer) never converges. Assigned in an effect + // because `useAnchoredScroll` reads it asynchronously from a post-mount effect. + React.useEffect(() => { + convergeToTargetRef.current = virtualizerOption + ? convergeToMessage + : () => false; + }, [convergeToMessage, virtualizerOption]); + // Abandon any in-flight convergence on channel switch so a stale loop can't + // hijack the new channel's scroll position. + // biome-ignore lint/correctness/useExhaustiveDependencies: cancel on channel switch only + React.useEffect(() => cancelConvergence, [channelId, cancelConvergence]); + // The unread pill is a transient, per-open affordance: dismiss it once the // user acts on it (jumps to the oldest unread) or catches up by reaching the // bottom of the timeline. Reset when the channel changes so a freshly opened @@ -292,14 +348,14 @@ const MessageTimelineBase = React.forwardRef< const handleJumpToOldestUnread = React.useCallback(() => { setIsUnreadPillDismissed(true); if (firstUnreadMessageId) { - scrollToMessage(firstUnreadMessageId); + jumpToMessage(firstUnreadMessageId); } - }, [firstUnreadMessageId, scrollToMessage]); + }, [firstUnreadMessageId, jumpToMessage]); - // Scroll to the active search match when it changes. `scrollToMessage` - // updates the scroll anchor (so the post-commit restore won't yank the view - // back off the match) and, when virtualized, resolves the target through the - // index model — the row may be windowed out of the DOM. + // Scroll to the active search match when it changes. `jumpToMessage` updates + // the scroll anchor (so the post-commit restore won't yank the view back off + // the match) and, when virtualized, converges on the target through the index + // model — the row may be windowed out of the DOM. const prevSearchActiveRef = React.useRef(null); React.useEffect(() => { if (showTimelineSkeleton) return; @@ -311,8 +367,8 @@ const MessageTimelineBase = React.forwardRef< return; } prevSearchActiveRef.current = searchActiveMessageId; - scrollToMessage(searchActiveMessageId, { behavior: "smooth" }); - }, [scrollToMessage, searchActiveMessageId, showTimelineSkeleton]); + jumpToMessage(searchActiveMessageId, { behavior: "smooth" }); + }, [jumpToMessage, searchActiveMessageId, showTimelineSkeleton]); useLoadOlderOnScroll({ fetchOlder, @@ -368,11 +424,17 @@ const MessageTimelineBase = React.forwardRef< >
- {isFetchingOlder ? ( -
+ {/* Fixed-height slot: an always-mounted height keeps the virtual + spacer's offset stable across the load-older fetch toggle, so + `scrollMargin` doesn't shift mid-fetch and yank the restore. */} +
+ {isFetchingOlder ? ( -
- ) : null} + ) : null} +
; - /** Small zero-height element near the very top of the content. When it - * intersects the viewport (with some rootMargin) we trigger fetchOlder. */ - sentinelRef: React.RefObject; /** Resets when changed; lets us drop anchor + scroll state across channels. */ channelId?: string | null; /** Suppresses initial scroll-to-bottom while a skeleton is showing. */ @@ -30,20 +27,17 @@ type UseAnchoredScrollOptions = { /** Source of truth for the rendered list. Used to detect new-at-bottom * arrivals and to seed/refresh the anchor pre-render. */ messages: TimelineMessage[]; - /** Optional callback to fetch older history. The hook handles intersection, - * debouncing, and post-prepend scroll restoration via the anchor. */ - fetchOlder?: () => Promise; - hasOlderMessages?: boolean; - /** True while an older-history fetch is in flight. The fetch spinner renders - * above the anchor, so toggling it shifts every row below it. The spinner - * toggles on its own commit (no message change), so without this signal the - * restoration effect — keyed on `messages` — wouldn't re-run to correct the - * shift, leaving a visible one-frame jump. Threading it through makes the - * anchor the single owner of every layout change above the reader's eye. */ - isFetchingOlder?: boolean; /** When set, scroll to and highlight this message on mount and on change. */ targetMessageId?: string | null; onTargetReached?: (messageId: string) => void; + /** Optional convergence fallback for a `targetMessageId` whose row is not in + * the DOM (windowed out of a virtualized list). When the DOM lookup fails, + * the hook delegates to this instead of waiting for a later commit that may + * never render the row. The consumer drives the virtualizer to the target, + * warming up if it's still being fetched, and fires `onTargetReached` itself + * on settle. Returns `true` once it owns the target (the hook marks it + * handled, no further dispatch). Absent (thread panel) → DOM-only retry. */ + convergeToTarget?: (messageId: string) => boolean; }; type UseAnchoredScrollResult = { @@ -67,6 +61,11 @@ type UseAnchoredScrollResult = { messageId: string, options?: { highlight?: boolean; behavior?: ScrollBehavior }, ) => boolean; + /** Single-writer scroll restore for the load-older index path. Sets + * `scrollTop` directly (no scroll event fires for a programmatic write), + * then re-seats the anchor + at-bottom bookkeeping so the next passive + * restore and `isAtBottom` read agree with where we put the scroll. */ + restoreScrollPosition: (scrollTop: number) => void; }; function isAtBottomNow(container: HTMLDivElement) { @@ -208,15 +207,12 @@ function restoreAnchorToMessage( export function useAnchoredScroll({ scrollContainerRef, contentRef, - sentinelRef, channelId, isLoading, messages, - fetchOlder, - hasOlderMessages = false, - isFetchingOlder = false, targetMessageId = null, onTargetReached, + convergeToTarget, }: UseAnchoredScrollOptions): UseAnchoredScrollResult { // Anchor lives in a ref because it must survive renders and is updated // both on scroll (commit-time read) and in the layout effect (post-render @@ -234,10 +230,24 @@ export function useAnchoredScroll({ >(null); const hasInitializedRef = React.useRef(false); + // Mirror the convergence fallback into a ref so the target effects read the + // live callback without re-subscribing on every consumer render. + const convergeToTargetRef = React.useRef(convergeToTarget); + convergeToTargetRef.current = convergeToTarget; const prevLastMessageIdRef = React.useRef(undefined); + // Tracks the FRONT (oldest) rendered id so the restore effect can detect a + // load-older prepend (front changed, tail unchanged) and cede it to the + // index path — see IMPORTANT #1 in the restore effect below. + const prevFirstMessageIdRef = React.useRef(undefined); const prevMessageCountRef = React.useRef(0); - const fetchingOlderRef = React.useRef(false); const handledTargetIdRef = React.useRef(null); + // Set while a convergence loop owns the scroll position (jump-to-message into + // windowed-out history). The library's reconcile loop is the sole writer + // during convergence, so the anchored restore below must cede — otherwise its + // at-bottom `scrollTo` would yank the view back as the target row splices in, + // the same two-writer contention the prepend bail prevents. Cleared when the + // target settles (consumer clears the route param → `targetMessageId` null). + const convergingTargetIdRef = React.useRef(null); const highlightTimeoutRef = React.useRef(null); // One-shot: the consumer calls `scrollToBottomOnNextUpdate()` right before // it sends a message (see ChannelPane). When the user's own message then @@ -256,9 +266,10 @@ export function useAnchoredScroll({ setHighlightedMessageId(null); hasInitializedRef.current = false; prevLastMessageIdRef.current = undefined; + prevFirstMessageIdRef.current = undefined; prevMessageCountRef.current = 0; - fetchingOlderRef.current = false; handledTargetIdRef.current = null; + convergingTargetIdRef.current = null; forceBottomOnNextAppendRef.current = false; if (highlightTimeoutRef.current !== null) { window.clearTimeout(highlightTimeoutRef.current); @@ -330,6 +341,36 @@ export function useAnchoredScroll({ [scrollContainerRef], ); + // Re-seat the anchor + at-bottom bookkeeping after a programmatic scrollTop + // write. A programmatic write fires no scroll event, so `onScroll` won't run + // to refresh `anchorRef`/`isAtBottom` — we run the same derivation here so + // the next passive restore and at-bottom read agree with the new position. + // We deliberately do NOT touch `newMessageCount`: a load-older restore keeps + // the reader mid-history, so the unread-count affordance must be untouched. + const syncAnchorAfterProgrammaticScroll = React.useCallback( + (container: HTMLDivElement) => { + anchorRef.current = computeAnchor(container); + const atBottom = anchorRef.current.kind === "at-bottom"; + setIsAtBottom((prev) => (prev === atBottom ? prev : atBottom)); + }, + [], + ); + + // Single-writer restore for the load-older index path (IMPORTANT #2). The + // index path resolves the exact target `scrollTop` off the virtualizer's + // settled measurement cache (`getOffsetForIndex`), so a bare assignment is + // correct on the first write — no rAF re-assert loop, no manager scroll-state + // machine. Re-seating the anchor afterwards keeps this the sole owner. + const restoreScrollPosition = React.useCallback( + (scrollTop: number) => { + const container = scrollContainerRef.current; + if (!container) return; + container.scrollTop = scrollTop; + syncAnchorAfterProgrammaticScroll(container); + }, + [scrollContainerRef, syncAnchorAfterProgrammaticScroll], + ); + // Scroll handler: recompute anchor + bottom state from the current // scroll position. Cheap enough to run on every scroll event — a single // `getBoundingClientRect` walk plus rect reads. @@ -347,10 +388,11 @@ export function useAnchoredScroll({ // --------------------------------------------------------------------------- // Anchor restoration: after every render, if the anchor was a message, // realign so that message sits at the same top-relative offset it had - // before the render. This is the single mechanism for keeping scroll - // stable across prepends, appends, image loads, embed expansions, etc. + // before the render. This keeps scroll stable across appends, image loads, + // and embed expansions. Load-older prepends are NOT handled here — they are + // ceded to `useLoadOlderOnScroll`'s index anchor (see the prepend bail + // below) so a single writer owns `scrollTop` on the prepend commit. // --------------------------------------------------------------------------- - // biome-ignore lint/correctness/useExhaustiveDependencies: `isFetchingOlder` is an intentional re-run trigger, not a read — the fetch spinner renders above the anchor on its own commit (with `messages` unchanged), so we re-run restoration on its toggle to correct the spinner-induced shift via the existing anchor. React.useLayoutEffect(() => { const container = scrollContainerRef.current; if (!container) return; @@ -387,16 +429,49 @@ export function useAnchoredScroll({ } hasInitializedRef.current = true; prevLastMessageIdRef.current = messages[messages.length - 1]?.id; + prevFirstMessageIdRef.current = messages[0]?.id; prevMessageCountRef.current = messages.length; return; } const anchor = anchorRef.current; const lastMessage = messages[messages.length - 1]; + const firstMessage = messages[0]; const prevLastId = prevLastMessageIdRef.current; + const prevFirstId = prevFirstMessageIdRef.current; const prevCount = prevMessageCountRef.current; const newLatestArrived = lastMessage !== undefined && lastMessage.id !== prevLastId; + // A convergence loop owns the scroll position while jumping to a windowed-out + // target (its library reconcile is the sole writer). Cede every restore + // branch to it — an at-bottom `scrollTo` here would chase the view back to + // the bottom as the target's neighbours splice into the window mid-converge. + // Refresh the tracked refs so the first post-settle commit isn't misread as + // a prepend/append. Cleared when the target settles (targetMessageId null). + if (convergingTargetIdRef.current !== null) { + prevLastMessageIdRef.current = lastMessage?.id; + prevFirstMessageIdRef.current = firstMessage?.id; + prevMessageCountRef.current = messages.length; + return; + } + // A load-older prepend grows the list at the FRONT while the tail is + // unchanged. `useLoadOlderOnScroll` owns the prepend restore via its index + // anchor (the single `scrollTop` writer). If this restore effect also ran + // its anchored `scrollBy` on the same commit, two writers would fight over + // `scrollTop`. So cede the prepend to the index path: refresh the tracked + // refs and bail before the anchored branch. (Append and in-window reflow + // leave the front id unchanged, so they fall through as before.) + const isPrepend = + firstMessage !== undefined && + prevFirstId !== undefined && + firstMessage.id !== prevFirstId && + !newLatestArrived; + if (isPrepend) { + prevLastMessageIdRef.current = lastMessage?.id; + prevFirstMessageIdRef.current = firstMessage.id; + prevMessageCountRef.current = messages.length; + return; + } // One-shot: an outbound send armed `scrollToBottomOnNextUpdate`. When the // resulting append lands, snap to bottom regardless of the current anchor, @@ -409,6 +484,7 @@ export function useAnchoredScroll({ setIsAtBottom(true); setNewMessageCount(0); prevLastMessageIdRef.current = lastMessage?.id; + prevFirstMessageIdRef.current = firstMessage?.id; prevMessageCountRef.current = messages.length; return; } @@ -435,9 +511,9 @@ export function useAnchoredScroll({ } prevLastMessageIdRef.current = lastMessage?.id; + prevFirstMessageIdRef.current = firstMessage?.id; prevMessageCountRef.current = messages.length; }, [ - isFetchingOlder, isLoading, messages, onTargetReached, @@ -447,71 +523,6 @@ export function useAnchoredScroll({ targetMessageId, ]); - // --------------------------------------------------------------------------- - // Older-history loader. IntersectionObserver on the top sentinel; when it - // crosses into view (with a 200px rootMargin so we preload a bit early) - // we fire `fetchOlder`. The anchor restoration above handles the prepend - // — we don't need to compute or apply a scrollHeight delta ourselves. - // --------------------------------------------------------------------------- - React.useEffect(() => { - const sentinel = sentinelRef.current; - const container = scrollContainerRef.current; - if ( - !sentinel || - !container || - !fetchOlder || - isLoading || - !hasOlderMessages - ) { - return; - } - - let disposed = false; - let observer: IntersectionObserver | null = null; - - const start = () => { - if (disposed) return; - observer = new IntersectionObserver( - ([entry]) => { - if (!entry?.isIntersecting || disposed || fetchingOlderRef.current) { - return; - } - fetchingOlderRef.current = true; - observer?.disconnect(); - - // Before the fetch, capture the anchor from the current scroll - // position. The layout effect after re-render will use it. - anchorRef.current = computeAnchor(container); - - void fetchOlder() - .catch(() => { - // Swallow; the next intersection will retry. We don't want - // to crash the observer chain on a transient relay error. - }) - .finally(() => { - fetchingOlderRef.current = false; - // Re-observe in case there's more history to load. - start(); - }); - }, - { root: container, rootMargin: "200px 0px 0px 0px" }, - ); - observer.observe(sentinel); - }; - - start(); - return () => { - disposed = true; - observer?.disconnect(); - }; - }, [ - fetchOlder, - hasOlderMessages, - isLoading, - scrollContainerRef, - sentinelRef, - ]); - // --------------------------------------------------------------------------- // Content resize: when fonts load late, an image decodes, an embed expands, // or any in-viewport reflow happens that React isn't driving (so the @@ -532,7 +543,17 @@ export function useAnchoredScroll({ if (!container) return; const anchor = anchorRef.current; if (anchor.kind === "at-bottom") { - container.scrollTo({ top: container.scrollHeight, behavior: "auto" }); + // Pin to bottom only when the viewport is GENUINELY at the bottom + // right now — read live geometry, don't trust the cached anchor kind. + // After a programmatic jump into windowed-out history, the virtualizer + // needs a frame to render rows at the new offset; in that gap + // `computeAnchor` finds no crossing row and falls back to `at-bottom`, + // which would make this observer yank a mid-history restore down to + // the floor as the prepended rows measure. `isAtBottomNow` is the + // authoritative read; when it disagrees we leave the scroll alone. + if (isAtBottomNow(container)) { + container.scrollTo({ top: container.scrollHeight, behavior: "auto" }); + } return; } // Use the same restore primitive as the layout effect so the @@ -567,6 +588,7 @@ export function useAnchoredScroll({ React.useEffect(() => { if (!targetMessageId) { handledTargetIdRef.current = null; + convergingTargetIdRef.current = null; return; } if (handledTargetIdRef.current === targetMessageId || isLoading) return; @@ -577,7 +599,23 @@ export function useAnchoredScroll({ const el = container.querySelector( `[data-message-id="${targetMessageId}"]`, ); - if (!el) return; // Row not rendered yet; a later `messages` commit retries. + if (!el) { + // Row not in the DOM. In a virtualized list it may be windowed out and + // never render from a passive commit, so delegate to the convergence + // fallback: it drives the virtualizer to the target (warming up if a + // deep-link target is still being fetched in) and, on settle, centers + + // highlights it and fires `onTargetReached`. We mark the target handled + // here so this effect stops re-dispatching, but deliberately do NOT fire + // `onTargetReached` yet — clearing the route param now would cancel the + // in-flight target fetch the loop is waiting on. Without a fallback + // (thread panel), leave the target for a later `messages` commit. + const converge = convergeToTargetRef.current; + if (converge?.(targetMessageId)) { + handledTargetIdRef.current = targetMessageId; + convergingTargetIdRef.current = targetMessageId; + } + return; + } handledTargetIdRef.current = targetMessageId; scrollToMessageImperative(targetMessageId, { highlight: true }); onTargetReached?.(targetMessageId); @@ -606,5 +644,6 @@ export function useAnchoredScroll({ scrollToBottom: scrollToBottomImperative, scrollToBottomOnNextUpdate, scrollToMessage: scrollToMessageImperative, + restoreScrollPosition, }; } diff --git a/desktop/src/features/messages/ui/useConvergentScrollToMessage.ts b/desktop/src/features/messages/ui/useConvergentScrollToMessage.ts index 084cc25a2..4a51c8c63 100644 --- a/desktop/src/features/messages/ui/useConvergentScrollToMessage.ts +++ b/desktop/src/features/messages/ui/useConvergentScrollToMessage.ts @@ -2,6 +2,7 @@ import * as React from "react"; import { type ConvergenceAlign, + CONVERGENCE_FRAME_CAP, convergenceStep, } from "@/features/messages/lib/scrollConvergence"; import type { ListVirtualizer } from "@/shared/ui/VirtualizedList"; @@ -22,10 +23,13 @@ type ConvergentScrollOptions = { type ConvergentScrollController = { /** - * Begins a convergence loop toward `messageId`. Returns `true` when the id is - * present in the data (loop started), `false` when it is absent (never - * off-screen-false — only data-absent-false, matching the deep-link contract). - * A new call cancels any in-flight loop. + * Begins a convergence loop toward `messageId`. Returns `true` (the loop + * always starts and owns the target). When the id isn't yet in the data — a + * deep-link target the route screen fetches asynchronously — the loop warms + * up by polling the live map, then converges once it lands; if it never + * lands within the frame cap it abandons (clears the highlight), the same + * terminal state as a target deleted mid-settle. A new call cancels any + * in-flight loop. */ scrollToMessage: (messageId: string) => boolean; /** Cancels any in-flight convergence loop (e.g. on unmount or channel switch). */ @@ -80,16 +84,18 @@ export function useConvergentScrollToMessage( const scrollToMessage = React.useCallback( (messageId: string) => { - const startIndex = mapRef.current.get(messageId); - if (startIndex === undefined) { - return false; - } - cancel(); let lastIssuedIndex: number | null = null; let previousOffset: number | null = null; let framesUsed = 0; + // The id->index map is rebuilt one render after `messages` changes, so a + // freshly-spliced deep-link target (the route screen fetches it by id + // asynchronously) can be absent from the map on the frame this starts. + // Poll the live map during a bounded warmup instead of bailing; once it + // resolves, hand off to the normal convergence loop. If it never resolves + // within the cap, abandon — same terminal state as a deleted target. + let resolved = mapRef.current.has(messageId); const frame = () => { rafIdRef.current = null; @@ -99,10 +105,25 @@ export function useConvergentScrollToMessage( } const currentIndex = mapRef.current.get(messageId); + if (!resolved) { + if (currentIndex === undefined) { + if (framesUsed + 1 >= CONVERGENCE_FRAME_CAP) { + onAbandonedRef.current?.(messageId); + return; + } + framesUsed += 1; + previousOffset = virtualizer.scrollOffset ?? 0; + rafIdRef.current = requestAnimationFrame(frame); + return; + } + resolved = true; + } + // The library has settled this frame when its offset reached the target // index's offset (within tolerance) and stopped moving. `currentIndex` // is re-read so a settle on a stale index never counts as converged. let librarySettled = false; + let stalledOffTarget = false; if (currentIndex !== undefined && lastIssuedIndex === currentIndex) { const offset = virtualizer.scrollOffset ?? 0; const target = virtualizer.getOffsetForIndex( @@ -116,6 +137,7 @@ export function useConvergentScrollToMessage( previousOffset !== null && Math.abs(offset - previousOffset) <= SETTLE_TOLERANCE_PX; librarySettled = reachedTarget && offsetStable; + stalledOffTarget = offsetStable && !reachedTarget; previousOffset = offset; } else { previousOffset = virtualizer.scrollOffset ?? 0; @@ -126,19 +148,23 @@ export function useConvergentScrollToMessage( indexByMessageId: mapRef.current, lastIssuedIndex, librarySettled, + stalledOffTarget, framesUsed, }); if ( decision.nextIndex !== null && - decision.nextIndex !== lastIssuedIndex + (decision.nextIndex !== lastIssuedIndex || decision.reissue) ) { - // Re-aim only when the index actually moved — re-issuing the same - // index would reset the library's stable-frame counter forever. + // Issue when the index moved (re-aim at the new row) OR the reducer + // asks for a same-index re-issue to kick an off-target stall. Reset + // `previousOffset` so the next frame's stability check restarts from + // the post-issue offset rather than treating the stall as settled. virtualizer.scrollToIndex(decision.nextIndex, { align: alignRef.current, }); lastIssuedIndex = decision.nextIndex; + previousOffset = null; } if (decision.done) { diff --git a/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts b/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts index bbf190111..54d42ad9c 100644 --- a/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts +++ b/desktop/src/features/messages/ui/useLoadOlderOnScroll.ts @@ -1,5 +1,6 @@ import * as React from "react"; +import { CONVERGENCE_FRAME_CAP } from "@/features/messages/lib/scrollConvergence"; import type { ListVirtualizer } from "@/shared/ui/VirtualizedList"; type UseLoadOlderOnScrollOptions = { @@ -10,13 +11,15 @@ type UseLoadOlderOnScrollOptions = { scrollContainerRef: React.RefObject; sentinelRef: React.RefObject; /** - * When the timeline is virtualized, prepended rows shift every index and are - * mounted at an estimate (80px) before they measure, so the `scrollHeight` - * delta anchor drifts. Supplying the virtualizer switches to an index anchor: - * we hold the first-visible item across the prepend by its NEW index. + * When the timeline is virtualized, a prepend shifts every index and a large + * one pushes the anchored row out of the window before it can be re-measured. + * Supplying the virtualizer switches to an index anchor: we hold the + * first-visible row across the prepend by re-aiming `scrollToIndex` at its new + * index (resolved from `indexByMessageId`) until the library settles it. */ virtualizer?: { getVirtualizer: () => ListVirtualizer | null; + indexByMessageId: Map; itemCount: number; } | null; }; @@ -75,53 +78,113 @@ export function useLoadOlderOnScroll({ const virt = virtualizerRef.current; if (virt) { - // Index anchor: hold the first rendered item across the prepend. - // Capture its index + the gap between its top and the viewport top - // BEFORE the fetch; after the prepend shifts indices by N, re-aim at - // `oldIndex + N` and restore that same intra-row gap. This is immune - // to the estimate->measured height churn that makes a scrollHeight - // delta drift. + // Hold the first VISIBLE row across the prepend. After N older rows + // are prepended the anchored row's INDEX shifts by N and — with + // scrollTop unchanged near the top — it's pushed below the window + // and recycled out of the DOM, so a pure DOM re-read can't find it. + // We therefore drive the virtualizer: capture the row's id + its top + // offset in the viewport now, and after the prepend re-aim + // `scrollToIndex(newIndex, "start")` each frame (re-issued only when + // the resolved index moves, so the library's internal settle loop is + // never reset — same single-issue discipline as the convergence + // adapter). `scrollToIndex` re-aims internally as rows mount and + // measure, landing the row's TOP at the viewport top; once it settles + // we apply the captured intra-viewport gap with ONE scrollTop write. + // Single writer throughout: one mechanism re-aims, the gap is a final + // one-shot, never an overlapping second target. const instance = virt.getVirtualizer(); - const firstVisible = instance?.getVirtualItems()[0]; + const container = scrollContainerRef.current; const previousCount = virt.itemCount; - const anchorIndex = firstVisible?.index ?? null; - const anchorOffsetIntoRow = - firstVisible && instance - ? (instance.scrollOffset ?? 0) - firstVisible.start - : 0; void fetchOlder().then(() => { - requestAnimationFrame(() => { - requestAnimationFrame(() => { - const after = virtualizerRef.current?.getVirtualizer(); - const prepended = - (virtualizerRef.current?.itemCount ?? previousCount) - - previousCount; - if (after && anchorIndex !== null && prepended > 0) { - // Restore by scrollTop ONLY — a single writer. Compute the - // anchored row's top via getOffsetForIndex (a pure read of - // the measurement cache, no scrollState) and add back the - // captured intra-row gap. Calling scrollToIndex here too - // would set the library's scrollState aiming at the row TOP - // while this restore aims at row top + gap; the two write - // scrollTop to different values on overlapping rAF frames, - // so the library's reconcile never reaches approxEqual, - // never re-scrolls (its target is unchanged), and spins one - // rAF/frame for the full 5s MAX_RECONCILE_MS valve on every - // prepend. One mechanism, no fight. - const target = after.getOffsetForIndex( - anchorIndex + prepended, - "start", + // Capture the anchor at fetch-RESOLVE time, not sentinel-fire + // time: the history request can be in flight for a while and the + // user may keep scrolling during it (the e2e scrolls further down + // mid-fetch). The row+offset we must preserve is wherever the + // reader actually is the instant before the prepend commits, read + // from the live DOM here while every visible row is still mounted. + const containerRect = container?.getBoundingClientRect(); + const containerTop = containerRect?.top ?? 0; + const containerBottom = containerRect?.bottom ?? 0; + // First row intersecting the viewport — the reader's eye-line row. + // Geometry matches the test's getFirstVisibleMessage exactly: its + // bottom is below the viewport top and its top is above the + // viewport bottom. + const anchorRow = container + ? Array.from( + container.querySelectorAll( + "[data-message-id]", + ), + ).find((row) => { + const rect = row.getBoundingClientRect(); + return ( + rect.bottom > containerTop && rect.top < containerBottom ); - if (target !== undefined) { - restoreScrollPositionRef.current( - target[0] + anchorOffsetIntoRow, - ); + }) + : undefined; + const anchorId = anchorRow?.dataset.messageId ?? null; + // The anchored row's top relative to the viewport top — held + // constant across the prepend. + const anchorTop = anchorRow + ? anchorRow.getBoundingClientRect().top - containerTop + : 0; + + // The timeline drives its rows off a `useDeferredValue` of the + // message list, so the prepended items commit on a LOW-priority + // render that can land several frames after `fetchOlder` resolves. + // Poll rAF until the live id->index map actually shifts the anchor + // (the prepend is observable), capped so an empty fetch can't spin. + const maxFrames = CONVERGENCE_FRAME_CAP; + let frame = 0; + let lastTarget: number | null = null; + let stableFrames = 0; + const waitForPrepend = () => { + const after = virtualizerRef.current; + const grew = + (after?.itemCount ?? previousCount) > previousCount; + const newIndex = + anchorId !== null + ? after?.indexByMessageId.get(anchorId) + : undefined; + if (instance && grew && newIndex !== undefined) { + // Target offset that puts the anchored row back at its captured + // viewport gap: the row's start (top at viewport top) minus the + // gap that was above it. We drive `scrollToOffset` — NOT + // `scrollToIndex` — so the library's reconcile holds a FIXED + // offset (`scrollState.index` is null) instead of re-resolving + // the index to row-top each frame and overwriting our gap. As + // the prepended rows measure, `getOffsetForIndex` grows and we + // recompute the target and re-issue. Re-issue ONLY when the + // target moves — re-issuing an unchanged offset resets the + // library's stable-frame counter and spins. Same single-issue + // discipline as the convergence adapter; one mechanism. + const start = instance.getOffsetForIndex(newIndex, "start"); + if (start !== undefined) { + const target = start[0] - anchorTop; + if (target !== lastTarget) { + instance.scrollToOffset(target, { align: "start" }); + lastTarget = target; + stableFrames = 0; + } else if ((instance.scrollOffset ?? 0) === target) { + // The offset reached its target and the target stopped + // moving (measurement settled). Two stable frames guards + // against ending before the last row measures. + stableFrames += 1; + if (stableFrames >= 2) { + observe(); + return; + } } } + } + frame += 1; + if (frame >= maxFrames) { observe(); - }); - }); + return; + } + requestAnimationFrame(waitForPrepend); + }; + requestAnimationFrame(waitForPrepend); }); return; } diff --git a/desktop/tests/e2e/scroll-history.spec.ts b/desktop/tests/e2e/scroll-history.spec.ts index 71e6b1b7a..f19797128 100644 --- a/desktop/tests/e2e/scroll-history.spec.ts +++ b/desktop/tests/e2e/scroll-history.spec.ts @@ -523,28 +523,75 @@ test("deep-link to a message in older history scrolls and highlights it", async const targetRow = timeline.locator(`[data-message-id="${targetId}"]`); await expect(targetRow).toBeVisible({ timeout: 5_000 }); - // (b) Geometry: target row sits inside the timeline viewport AND near - // the vertical center. "Near center" is defined as within one row-height - // worth of slack of half the timeline's clientHeight -- generous enough - // to tolerate centering implementation choices but tight enough to catch - // "row is technically visible but at the top edge" regressions. + // (b)/(c) Geometry + highlight. The convergence adapter re-aims the + // virtualizer across several frames (scrollToIndex on an estimated index, + // then re-resolved once rows measure), and only applies the highlight on + // the settled frame via `onConverged`. The row therefore becomes DOM- + // visible (passing `toBeVisible` above) at an intermediate overshoot + // position one or more frames before it is centered and highlighted. We + // poll the row's placement until it satisfies the full settled contract -- + // centered AND carrying the highlight class -- inside the 2s highlight-fade + // window. A single synchronous read here would race the convergence loop. const placement = await timeline.evaluate((timelineEl, id) => { const t = timelineEl as HTMLDivElement; - const row = t.querySelector( - `[data-message-id="${CSS.escape(id)}"]`, - ); - if (!row) { - return null; - } - const tRect = t.getBoundingClientRect(); - const rRect = row.getBoundingClientRect(); - return { - rowTopRelative: rRect.top - tRect.top, - rowBottomRelative: rRect.bottom - tRect.top, - timelineHeight: tRect.height, - rowHeight: rRect.height, - className: row.className, - }; + return new Promise<{ + rowTopRelative: number; + rowBottomRelative: number; + timelineHeight: number; + rowHeight: number; + className: string; + } | null>((resolve) => { + const deadline = performance.now() + 3_000; + const tick = () => { + const row = t.querySelector( + `[data-message-id="${CSS.escape(id)}"]`, + ); + if (row) { + const tRect = t.getBoundingClientRect(); + const rRect = row.getBoundingClientRect(); + const result = { + rowTopRelative: rRect.top - tRect.top, + rowBottomRelative: rRect.bottom - tRect.top, + timelineHeight: tRect.height, + rowHeight: rRect.height, + className: row.className, + }; + const centered = + Math.abs( + (result.rowTopRelative + result.rowBottomRelative) / 2 - + result.timelineHeight / 2, + ) <= + result.timelineHeight / 2; + if ( + centered && + result.className.includes("route-target-highlight-fade") + ) { + resolve(result); + return; + } + } + if (performance.now() > deadline) { + resolve( + row + ? { + rowTopRelative: + row.getBoundingClientRect().top - + t.getBoundingClientRect().top, + rowBottomRelative: + row.getBoundingClientRect().bottom - + t.getBoundingClientRect().top, + timelineHeight: t.getBoundingClientRect().height, + rowHeight: row.getBoundingClientRect().height, + className: row.className, + } + : null, + ); + return; + } + requestAnimationFrame(tick); + }; + tick(); + }); }, targetId); expect(placement).not.toBeNull(); const p = placement as NonNullable;