fix(message-list): pagination flag and scroll restore leak across channel switches
Two bugs in handleScroll's loadMoreMessages flow surfaced after the smooth-scroll race fix. (1) isLoadingMore stuck across channels. setIsLoadingMore(true) → await loadMoreMessages → setIsLoadingMore(false) was unguarded. If the user switched channels during the await, the new channel inherited the flag (same component instance, same useState slot) and rendered the pagination skeleton even with no load in flight. Cleared only when the original await resolved or the component remounted (e.g., navigating to Friends and back). (2) Wrong-channel scroll restore. The post-await rAF set container.scrollTop = container.scrollHeight - prevScrollHeight against the new channel's container with the old channel's prevScrollHeight, yanking the new channel to a wrong position. Fix: - try/finally around the await so setIsLoadingMore(false) always runs. - currentChannelIdRef tracks the live channelId; capture requestChannelId at load start and compare both before scheduling the rAF and inside the rAF callback (the 16ms frame gap is enough for a switch). - Belt-and-suspenders: setIsLoadingMore(false) in the channel-switch effect covers the case where the await never resolves (network hang). Without it, a stuck await would leave the new channel inheriting the flag indefinitely. No request cancellation — out of scope; AbortController plumbing through chatStore is a bigger refactor and the channelId guard already silently drops stale results. Spec updated. Smooth-scroll fix from the previous commit untouched.
This commit is contained in:
@@ -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-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-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 — 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.
|
||||||
|
|||||||
@@ -85,6 +85,15 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
|||||||
const prevChannelIdRef = useRef<string>(channelId);
|
const prevChannelIdRef = useRef<string>(channelId);
|
||||||
const visibleMsgIdRef = useRef<string | null>(null);
|
const visibleMsgIdRef = useRef<string | null>(null);
|
||||||
const ackTimerRef = useRef<ReturnType<typeof setTimeout>>();
|
const ackTimerRef = useRef<ReturnType<typeof setTimeout>>();
|
||||||
|
// 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.
|
// Final defensive pin after a bottom-bound smooth scroll completes.
|
||||||
// Runs from either the native `scrollend` handler (preferred) or the timeout fallback
|
// 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;
|
prevMessagesLength.current = 0;
|
||||||
lastProgrammaticBottomScrollRef.current = null;
|
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
|
// 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
|
// — this prevents the ResizeObserver from snapping to bottom before the restore rAF fires
|
||||||
const willRestore = useChatStore.getState().scrollPositions.has(channelId);
|
const willRestore = useChatStore.getState().scrollPositions.has(channelId);
|
||||||
@@ -438,19 +455,42 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
|||||||
visibleMsgIdRef.current = null;
|
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) {
|
if (container.scrollTop < 50 && hasMore && !isLoadingMore) {
|
||||||
|
const requestChannelId = channelId;
|
||||||
setIsLoadingMore(true);
|
setIsLoadingMore(true);
|
||||||
const prevScrollHeight = container.scrollHeight;
|
const prevScrollHeight = container.scrollHeight;
|
||||||
const loaded = await loadMoreMessages(channelId);
|
try {
|
||||||
if (loaded) {
|
const loaded = await loadMoreMessages(requestChannelId);
|
||||||
// Maintain scroll position
|
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(() => {
|
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]);
|
}, [channelId, hasMore, isLoadingMore, loadMoreMessages]);
|
||||||
|
|
||||||
if (!canReadHistory) {
|
if (!canReadHistory) {
|
||||||
|
|||||||
Reference in New Issue
Block a user