diff --git a/docs/systems/message-list.md b/docs/systems/message-list.md index 94159ca7..ff93c944 100644 --- a/docs/systems/message-list.md +++ b/docs/systems/message-list.md @@ -91,3 +91,4 @@ These items were considered and rejected for the 2026-04-25 work; they live here - 2026-03-25 — `embed-dimension-reservation` spec: server probes image embeds for dimensions; client renderers reserve via `aspect-ratio`. Server side shipped. Client side partly shipped, then reverted in `0c84029` because the 4/3 fallback caused dark letterbox bars. - 2026-04-25 — `message-list-scroll-completion-and-addendum` spec: restored the *known-dimension-only* branch of the client reservation in `ImageEmbed.tsx`; added the `lastProgrammaticBottomScrollRef` sentinel to close the residual `handleScroll` race; created this file. - 2026-04-27 — smooth-scroll intent + `scrollend` final pin: typed `smoothScrollIntentRef` (`'bottom' | 'message' | null`) with an 800ms deadline. `handleScroll` suppresses the at-bottom flip while a `'bottom'` intent is active and the user hasn't wheeled past the 5000px threshold; the gate stays open so Effects B/C re-pin to bottom as media loads mid-animation. Effect D fires a final defensive instant pin via the native `scrollend` event (Chrome 114+, Safari 18+) or a `setTimeout(800)` fallback. `'message'` intent (jump-to-message from search) does NOT suppress — the gate flips honestly so the user is left at the targeted message. Closes the "Jump to Present doesn't fully reach the bottom" residue reproducible on `nova.ddns.net` Orbit → general. UX call: smooth-scroll animation preserved, not replaced with an instant jump. +- 2026-04-27 — pagination cross-channel race fix: `handleScroll`'s load-more block (`scrollTop < 50 && hasMore`) leaked `isLoadingMore = true` and the `prevScrollHeight` value across channel switches that raced the async `loadMoreMessages` await. The `useState` slot is the same component instance across channel changes, so the new channel inherited the flag (phantom pagination skeleton) and the post-await `requestAnimationFrame` applied the outgoing channel's `prevScrollHeight` to the incoming channel's container DOM (wrong-position scroll). Fix: capture `channelId` into a `requestChannelId` local at the start of the load block, mirror the live `channelId` prop into `currentChannelIdRef` synchronously each render, and compare twice — once before scheduling the rAF and once *inside* the rAF callback (the ~16ms gap between scheduling and firing is enough time for a click to switch channels). Wrap the await in `try/finally` so `setIsLoadingMore(false)` always runs even on throw. Belt-and-suspenders: the channel-switch effect (Effect 3) also calls `setIsLoadingMore(false)` so a never-resolving await (network hang) cannot strand the flag on the new channel. The store's `currentChannelId` was rejected as the live source — it lags one render behind a URL-driven channel switch (set in an `AppLayout` effect that fires after `MessageList` renders with the new prop), which would let the guard mis-fire during that one-frame window. Request cancellation was deliberately deferred — the channelId guard already silently drops stale results, and `AbortController` plumbing through `chatStore.loadMoreMessages` is a larger refactor; network waste on an abandoned page is not a correctness issue. diff --git a/packages/web/src/components/chat/MessageList.tsx b/packages/web/src/components/chat/MessageList.tsx index 89f6f1f0..6c347523 100644 --- a/packages/web/src/components/chat/MessageList.tsx +++ b/packages/web/src/components/chat/MessageList.tsx @@ -85,6 +85,15 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess const prevChannelIdRef = useRef(channelId); const visibleMsgIdRef = useRef(null); const ackTimerRef = useRef>(); + // Live mirror of the current `channelId` prop. Updated synchronously each render so + // that async callbacks (notably the `loadMoreMessages` await in `handleScroll` and the + // `requestAnimationFrame` it schedules) can compare a captured channel against the + // current channel and bail if the user switched away mid-flight. We don't read the + // store's `currentChannelId` because it lags one render behind a URL-driven channel + // switch (it's set in an `AppLayout` effect that fires after MessageList renders with + // the new prop), which would let the guard mis-fire during that single-frame window. + const currentChannelIdRef = useRef(channelId); + currentChannelIdRef.current = channelId; // Final defensive pin after a bottom-bound smooth scroll completes. // Runs from either the native `scrollend` handler (preferred) or the timeout fallback @@ -204,6 +213,14 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess prevMessagesLength.current = 0; lastProgrammaticBottomScrollRef.current = null; + // Belt-and-suspenders: clear any in-flight pagination flag from the outgoing channel. + // `handleScroll`'s try/finally normally clears it when the await resolves, but the + // captured-channelId guard only silently drops the stale result — if the network + // hangs and the await never resolves, the new channel would inherit the flag and + // render a phantom pagination skeleton. Resetting here costs nothing and covers + // the never-resolves case. Idempotent w.r.t. the finally block. + setIsLoadingMore(false); + // If we have a saved position for the incoming channel, don't mark as near/at-bottom // — this prevents the ResizeObserver from snapping to bottom before the restore rAF fires const willRestore = useChatStore.getState().scrollPositions.has(channelId); @@ -438,18 +455,41 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess visibleMsgIdRef.current = null; } - // Load more when scrolled to top + // Load more when scrolled to top. + // Capture the channelId locally so we can detect a channel switch that races the + // async load. Two guard points: + // 1. Before scheduling the rAF — if the user already switched, we have no business + // touching scroll on the outgoing channel's (now-unmounted-from-view) container, + // and `prevScrollHeight` is meaningless against the new channel's DOM. + // 2. *Inside* the rAF callback — the rAF runs ~16ms after we schedule it, so the + // channel can switch in that window even if it was still current at schedule time. + // The try/finally guarantees `setIsLoadingMore(false)` runs even if `loadMoreMessages` + // throws (defense in depth — `chatStore.loadMoreMessages` currently catches and returns + // false, but we don't want a future refactor to leak the flag). The Effect-3 reset on + // channel switch is the third safety net for the "await never resolves" case. if (container.scrollTop < 50 && hasMore && !isLoadingMore) { + const requestChannelId = channelId; setIsLoadingMore(true); const prevScrollHeight = container.scrollHeight; - const loaded = await loadMoreMessages(channelId); - if (loaded) { - // Maintain scroll position + try { + const loaded = await loadMoreMessages(requestChannelId); + if (!loaded) return; + // Channel-switch guard #1: skip the rAF entirely if the user moved away during + // the await. The container ref now points at the new channel's scroller, so + // applying `scrollHeight - prevScrollHeight` would yank it to a wrong position. + if (currentChannelIdRef.current !== requestChannelId) return; requestAnimationFrame(() => { - container.scrollTop = container.scrollHeight - prevScrollHeight; + // Channel-switch guard #2: re-check inside the rAF callback. The frame between + // scheduling and firing (~16ms) is enough time for a click to switch channels, + // and the same wrong-position outcome would result. + if (currentChannelIdRef.current !== requestChannelId) return; + const c = containerRef.current; + if (!c) return; + c.scrollTop = c.scrollHeight - prevScrollHeight; }); + } finally { + setIsLoadingMore(false); } - setIsLoadingMore(false); } }, [channelId, hasMore, isLoadingMore, loadMoreMessages]);