fix(message-list): sticky pagination skeleton on channel switch

Two cooperating bugs let the pagination skeleton stick at the top of the
chat across channel switches: (1) the browser's post-clamp scroll event
on channel switch fired the load-more block on the new channel, and
(2) useDelayedLoading's threshold timer refreshed displayStart on every
fire, extending the minDisplay window unboundedly when isLoading cycled.

- useDelayedLoading: stamp displayStart only on the false→true show
  transition (functional setShow form to avoid stale closures).
- MessageList: suppressNextLoadMoreRef armed in Effect 3, consumed by
  the load-more block on the next scroll event, with a 250 ms fallback
  disarm so legitimate user scrolls aren't silently dropped. Suppression
  scoped to the load-more block only.
- Regression test for the displayStart refresh bug.
- Updated docs/systems/message-list.md history.
This commit is contained in:
Jannis Braun
2026-05-05 19:56:26 +02:00
parent 97a8d982d5
commit d793ab8f78
4 changed files with 209 additions and 3 deletions
+1
View File
@@ -99,3 +99,4 @@ These items were considered and rejected for the 2026-04-25 work; they live here
- 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-05-05 — skeleton-as-overlay fix: the initial-load skeleton was rendered as an early return that replaced the JSX containing `containerRef` / `contentRef`. On slow loads (mobile hotspot, throttled connection), `useDelayedLoading`'s 200ms threshold flipped `showInitialSkeleton=true` before messages arrived, unmounting the scroll container. When messages then arrived (`messages.length` 0→N), Effect A's rAF ran against `containerRef.current === null` and bailed at the null guard at the top of its callback. Effects B/C/D and the scrollend listener — all keyed on `[hasMessages, channelId]` — re-fired exactly once when `hasMessages` flipped false→true (which happened *while the skeleton was still up* due to `useDelayedLoading.minDisplay`'s 300ms enforcement), hit the same null guard, and never re-attached because no dep changed when the skeleton finally cleared. Net effect: chat opened scrolled to the top instead of the bottom; ResizeObserver never observed the content; scrollend never registered; saved-anchor restore was equally broken. Fix: render the skeleton as an `absolute inset-0 z-10 bg-surface-chat pointer-events-none` overlay alongside the (always-mounted) scroll container, so all refs stay live across the loading transition. Added the "ContainerRef invariant" section above to encode the constraint for future loading-UI additions. **Manual repro recipe** (regression check): DevTools → Network tab → throttle to "Slow 3G" → click into a channel that hasn't been opened this session → pre-fix: list lands scrolled to the top; post-fix: list lands at the bottom. Saved-anchor variant: scroll up in a channel, switch away, return under throttle — pre-fix: lands at top; post-fix: lands at the saved anchor. - 2026-05-05 — skeleton-as-overlay fix: the initial-load skeleton was rendered as an early return that replaced the JSX containing `containerRef` / `contentRef`. On slow loads (mobile hotspot, throttled connection), `useDelayedLoading`'s 200ms threshold flipped `showInitialSkeleton=true` before messages arrived, unmounting the scroll container. When messages then arrived (`messages.length` 0→N), Effect A's rAF ran against `containerRef.current === null` and bailed at the null guard at the top of its callback. Effects B/C/D and the scrollend listener — all keyed on `[hasMessages, channelId]` — re-fired exactly once when `hasMessages` flipped false→true (which happened *while the skeleton was still up* due to `useDelayedLoading.minDisplay`'s 300ms enforcement), hit the same null guard, and never re-attached because no dep changed when the skeleton finally cleared. Net effect: chat opened scrolled to the top instead of the bottom; ResizeObserver never observed the content; scrollend never registered; saved-anchor restore was equally broken. Fix: render the skeleton as an `absolute inset-0 z-10 bg-surface-chat pointer-events-none` overlay alongside the (always-mounted) scroll container, so all refs stay live across the loading transition. Added the "ContainerRef invariant" section above to encode the constraint for future loading-UI additions. **Manual repro recipe** (regression check): DevTools → Network tab → throttle to "Slow 3G" → click into a channel that hasn't been opened this session → pre-fix: list lands scrolled to the top; post-fix: list lands at the bottom. Saved-anchor variant: scroll up in a channel, switch away, return under throttle — pre-fix: lands at top; post-fix: lands at the saved anchor.
- 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. - 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.
- 2026-05-05 — sticky pagination skeleton (clamp-scroll trigger + minDisplay deadline drift): two cooperating bugs produced a "ghost" pagination skeleton that stuck at the top of the chat across channel switches, sometimes until reload. (1) When the user switched from a tall channel to a shorter one, the browser clamped `scrollTop` and dispatched a synthetic scroll event after Effect 3 ran. `handleScroll` saw `scrollTop < 50 && hasMore && !isLoadingMore` — for any channel where `hasMore` defaulted to `true` (unvisited or LRU-evicted; see `MessageList.tsx` `hasMore` selector with the `?? true` fallback) the load-more block fired even though the user never scrolled. (2) `useDelayedLoading`'s threshold-timer callback unconditionally wrote `displayStartRef.current = Date.now()`, including when `show` was already true. With `isLoading` cycling true→false→true at periods near `threshold` (200 ms), each re-fire refreshed the deadline and the next `false` rescheduled `minDisplay` from a fresh start — extending the visible skeleton by up to one full `minDisplay` window per cycle. Together: the clamp event re-armed `isLoadingMore` against an empty-bail `loadMoreMessages` call, the hook over-extended the visible window, and rapid channel switches kept feeding the cycle. Fix: (a) `useDelayedLoading.ts` — only write `displayStartRef.current` on the false→true `show` transition, using `setShow`'s functional form to avoid stale closures. (b) `MessageList.tsx``suppressNextLoadMoreRef` is armed in Effect 3 on every channel change and consumed by the load-more block on the first scroll event after the switch. A 250 ms `setTimeout` fallback disarms the flag if no clamp event fires (new channel's content fit without clamping), so a real user scroll-to-top isn't permanently suppressed. Suppression is scoped to the load-more block only — `isAtBottomRef`/`isNearBottomRef` updates and visible-message tracking still run because the clamped scroll genuinely changes scroll position. Regression test: `useDelayedLoading.test.ts` (`REGRESSION: minDisplay deadline measured from FIRST show, not refreshed by re-fired threshold`).
@@ -103,6 +103,30 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
// the new prop), which would let the guard mis-fire during that single-frame window. // the new prop), which would let the guard mis-fire during that single-frame window.
const currentChannelIdRef = useRef(channelId); const currentChannelIdRef = useRef(channelId);
currentChannelIdRef.current = channelId; currentChannelIdRef.current = channelId;
// Suppress the first scroll event after a channel switch from triggering
// pagination. When the new channel's content is shorter than the outgoing
// channel's, the browser clamps `scrollTop` to its new max and dispatches a
// synthetic scroll event. That event lands in `handleScroll` with
// `scrollTop < 50`, and for any channel where `hasMore` is `true` (default
// for unvisited channels per the `?? true` fallback at the `hasMore`
// selector) it would fire `loadMoreMessages` even though the user never
// scrolled. The flag is armed in Effect 3 on every channel change and
// consumed by the load-more block on the next scroll event. A 250 ms
// setTimeout disarms it as a fallback in case no clamp event fires (new
// channel's content fit without clamping), so a real user scroll-to-top
// shortly after a channel switch isn't permanently suppressed.
const suppressNextLoadMoreRef = useRef(false);
const suppressNextLoadMoreTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
// Unmount cleanup — clear the disarm timer so its callback doesn't run after
// the component is gone. Refs survive unmount, so the callback would still
// execute harmlessly, but explicit cleanup is the convention used by sibling
// timer refs in this file (`smoothScrollFallbackTimerRef`).
useEffect(() => () => {
if (suppressNextLoadMoreTimerRef.current) {
clearTimeout(suppressNextLoadMoreTimerRef.current);
suppressNextLoadMoreTimerRef.current = null;
}
}, []);
// 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
@@ -289,6 +313,19 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
// the never-resolves case. Idempotent w.r.t. the finally block. // the never-resolves case. Idempotent w.r.t. the finally block.
setIsLoadingMore(false); setIsLoadingMore(false);
// Arm the clamp-scroll suppression flag. See `suppressNextLoadMoreRef`
// declaration for rationale. The 250 ms fallback timer disarms it in case
// no clamp event fires (new content fit without clamping) so legitimate
// user scrolls aren't silently dropped.
suppressNextLoadMoreRef.current = true;
if (suppressNextLoadMoreTimerRef.current) {
clearTimeout(suppressNextLoadMoreTimerRef.current);
}
suppressNextLoadMoreTimerRef.current = setTimeout(() => {
suppressNextLoadMoreRef.current = false;
suppressNextLoadMoreTimerRef.current = null;
}, 250);
// 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);
@@ -535,7 +572,24 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
// throws (defense in depth — `chatStore.loadMoreMessages` currently catches and returns // 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 // 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. // channel switch is the third safety net for the "await never resolves" case.
if (container.scrollTop < 50 && hasMore && !isLoadingMore) { // Consume the post-channel-switch suppression flag. The first scroll event
// after a channel change is almost always the browser-clamp event (when
// the new channel's content is shorter than the outgoing channel's
// scrollTop) and must NOT be treated as a user scroll-to-top. We only
// skip the load-more block — the at-bottom/near-bottom recomputation and
// the visible-message tracking above must still run (the clamp event
// genuinely changes scroll position, and the new value should be reflected).
let suppressLoadMore = false;
if (suppressNextLoadMoreRef.current) {
suppressNextLoadMoreRef.current = false;
if (suppressNextLoadMoreTimerRef.current) {
clearTimeout(suppressNextLoadMoreTimerRef.current);
suppressNextLoadMoreTimerRef.current = null;
}
suppressLoadMore = true;
}
if (!suppressLoadMore && container.scrollTop < 50 && hasMore && !isLoadingMore) {
const requestChannelId = channelId; const requestChannelId = channelId;
setIsLoadingMore(true); setIsLoadingMore(true);
const prevScrollHeight = container.scrollHeight; const prevScrollHeight = container.scrollHeight;
@@ -0,0 +1,142 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { renderHook, act } from '@testing-library/react';
import { useDelayedLoading } from '../useDelayedLoading';
const THRESHOLD = 200;
const MIN_DISPLAY = 300;
beforeEach(() => {
vi.useFakeTimers();
});
afterEach(() => {
vi.useRealTimers();
});
describe('useDelayedLoading', () => {
it('does not show before threshold elapses', () => {
const { result } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
expect(result.current).toBe(false);
act(() => { vi.advanceTimersByTime(THRESHOLD - 1); });
expect(result.current).toBe(false);
});
it('shows after threshold elapses while loading is true', () => {
const { result } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
act(() => { vi.advanceTimersByTime(THRESHOLD); });
expect(result.current).toBe(true);
});
it('cancels threshold if loading flips false before it fires', () => {
const { result, rerender } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
act(() => { vi.advanceTimersByTime(THRESHOLD - 50); });
rerender({ loading: false });
act(() => { vi.advanceTimersByTime(500); });
expect(result.current).toBe(false);
});
it('keeps shown for at least minDisplay after first becoming visible', () => {
const { result, rerender } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
act(() => { vi.advanceTimersByTime(THRESHOLD); });
expect(result.current).toBe(true);
rerender({ loading: false });
act(() => { vi.advanceTimersByTime(MIN_DISPLAY - 1); });
expect(result.current).toBe(true);
act(() => { vi.advanceTimersByTime(1); });
expect(result.current).toBe(false);
});
// ---------------------------------------------------------------------------
// Hypothesised bug: every threshold-timer fire calls
// displayStartRef.current = Date.now()
// unconditionally — even when `show` is already true. If `isLoading` cycles
// true → false → true → false with each `true` segment ≥ threshold, every
// cycle fires the threshold timer again, refreshes displayStart, and the
// following `false` schedules minDisplay from that refreshed timestamp. The
// skeleton therefore never reaches its real minDisplay deadline.
//
// Intent of the hook: once shown, hide as soon as `isLoading` goes false AND
// at least minDisplay ms have passed since the *first* time it became visible.
// ---------------------------------------------------------------------------
it('REGRESSION: rapid true→false→true→false cycles must clear within bounded time', () => {
const { result, rerender } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
// First show: threshold elapses, show becomes true at t = 200.
act(() => { vi.advanceTimersByTime(THRESHOLD); });
expect(result.current).toBe(true);
// Cycle pattern:
// - 50 ms of false (short, never long enough to clear minDisplay alone)
// - 200 ms of true (exactly enough to re-fire the threshold timer)
// Each iteration is 250 ms of wall time. After ~10 cycles (2.5 s of wall
// time) the skeleton has been visible for two and a half full seconds with
// many opportunities to clear, since `loading=false` segments occur
// repeatedly and each is well past the minDisplay deadline measured from
// the *original* displayStart at t=200.
for (let i = 0; i < 10; i++) {
rerender({ loading: false });
act(() => { vi.advanceTimersByTime(50); });
rerender({ loading: true });
act(() => { vi.advanceTimersByTime(THRESHOLD); });
}
// Now end on `loading=false` and let any pending minDisplay timer drain.
rerender({ loading: false });
act(() => { vi.advanceTimersByTime(MIN_DISPLAY * 2); });
// After all cycling stops and we wait long enough for any scheduled
// minDisplay timer to fire, the skeleton MUST be hidden. If this fails
// with `result.current === true`, the displayStart-refresh bug is real:
// the hook keeps moving its own deadline forward and the skeleton stays
// visible indefinitely under this input pattern.
expect(result.current).toBe(false);
});
it('REGRESSION: minDisplay deadline measured from FIRST show, not refreshed by re-fired threshold', () => {
// Simpler, more focused variant of the above. One re-fire of the threshold
// timer is enough to demonstrate the bug:
// t=0 loading=true → schedule threshold (fires t=200)
// t=200 threshold fires → show=true, displayStart=200
// t=250 loading=false → schedule minDisplay (remaining=250, fires t=500)
// t=300 loading=true → cancel minDisplay, schedule threshold (fires t=500)
// t=500 threshold fires AGAIN → show=true (no-op), displayStart=500 (BUG: refreshed)
// t=550 loading=false → schedule minDisplay (remaining=250 from refreshed start, fires t=800)
//
// Intent: skeleton was first shown at t=200. minDisplay window is 300 ms,
// so it should be eligible to hide at t=500. With the final `loading=false`
// at t=550 (well past t=500), it should hide essentially immediately.
//
// Bug: hook waits until t=800 because displayStart was clobbered.
const { result, rerender } = renderHook(({ loading }) => useDelayedLoading(loading), {
initialProps: { loading: true },
});
act(() => { vi.advanceTimersByTime(200); }); // t=200, threshold fires
expect(result.current).toBe(true);
rerender({ loading: false }); // t=200, schedule M1
act(() => { vi.advanceTimersByTime(100); }); // t=300
rerender({ loading: true }); // t=300, cancel M1, schedule T2
act(() => { vi.advanceTimersByTime(200); }); // t=500, T2 fires (refreshes displayStart)
rerender({ loading: false }); // t=500, schedule M2
act(() => { vi.advanceTimersByTime(50); }); // t=550
// At t=550 we are 350 ms past the original show-time (t=200), well beyond
// the 300 ms minDisplay floor. The skeleton should be hidden by now.
expect(result.current).toBe(false);
});
});
+11 -2
View File
@@ -20,8 +20,17 @@ export function useDelayedLoading(
useEffect(() => { useEffect(() => {
if (isLoading) { if (isLoading) {
thresholdRef.current = setTimeout(() => { thresholdRef.current = setTimeout(() => {
displayStartRef.current = Date.now(); // Only stamp displayStart on the first false→true transition. Re-firing
setShow(true); // the threshold while `show` is already true (which happens when
// `isLoading` cycles true→false→true with each `true` segment ≥ threshold)
// must not refresh the deadline — otherwise the next `false` reschedules
// minDisplay from a fresh start and the skeleton never reaches its real
// hide point. setShow's functional form lets us read the latest value
// without a closure-stale `show`.
setShow((prev) => {
if (!prev) displayStartRef.current = Date.now();
return true;
});
}, threshold); }, threshold);
} else { } else {
// Loading finished — clear threshold timer if it hasn't fired yet // Loading finished — clear threshold timer if it hasn't fired yet