merge: fix(chat): pagination skeleton no layout shift into main
Eliminates the visible push-down/snap-back when the pagination loading skeleton appears at the top of the message list. Achieved by rendering a constant-height (~200px) top-of-list slot above messages whenever hasMore===true; the skeleton's grey-bar contents toggle inside that slot rather than the slot itself mounting/unmounting. Companion changes: load-more trigger raised to fire before the slot enters the viewport with an iOS Safari rubber-band guard, prepend scroll-restore math corrected to handle non-zero prevScrollTop, useDelayedLoading threshold lowered to 50ms for pagination only.
This commit is contained in:
@@ -23,6 +23,16 @@ The 80 px fallback is sized for the desktop case during the brief mount window b
|
||||
|
||||
**Implication for loading UI:** the initial-load skeleton is rendered as an absolutely-positioned overlay on top of the scroll container, NOT as an early return that replaces it. See `MessageList.tsx` (the `showInitialSkeleton` overlay block) for the canonical pattern. Any future loading/empty/error UI added to this component must follow the same rule — overlay, never replace — or the scroll model breaks under slow loads where the skeleton is visible at the moment messages arrive.
|
||||
|
||||
## Top-of-list reservation slot
|
||||
|
||||
When `hasMore === true` for the current channel, a constant-height div is rendered as the first child of the scroll container, above the messages container that holds `contentRef`. This slot exists for the entire `hasMore === true` lifetime of the channel view and its height NEVER changes. The pagination skeleton's grey-bar contents toggle inside the slot based on `showPaginationSkeleton`; the slot itself does not mount or unmount with the skeleton. This is the entire mechanism that eliminates the layout-shift bug where the skeleton's appearance pushed message content down and its disappearance snapped it back up — by keeping slot height invariant, no DOM height ever changes from the skeleton's lifecycle.
|
||||
|
||||
**The constant `PAGINATION_SLOT_HEIGHT_PX`** (defined at module scope in `MessageList.tsx`) is the single source of truth for three values: the slot's inline `height` style, the skeleton's structural total height (the markup is tuned so it fits comfortably inside the constant — the last row's `mb-5` is stripped to give a buffer), and the load-more trigger threshold (`scrollTop < PAGINATION_SLOT_HEIGHT_PX + 50`). It is a TS constant rather than a CSS variable to avoid a `getComputedStyle` round-trip when the threshold computation reads it back. If the skeleton markup changes (different row count, different padding, different bar height), re-measure `getBoundingClientRect().height` in DevTools and update the constant — round UP to the nearest 4-pixel step.
|
||||
|
||||
**Slot ↔ WelcomeHeader transition.** The slot and `WelcomeHeader` are two separate components and are mutually exclusive: slot when `hasMore`, WelcomeHeader when `!hasMore`. On the final page load that flips `hasMore` to false, the slot unmounts and WelcomeHeader mounts in its tree position. The (WelcomeHeader_height − slot_height) delta is automatically absorbed by the prepend math, which sees the total change in `container.scrollHeight` and compensates `scrollTop` accordingly — no special transition handling required. Keeping the two as separate components is a deliberate clarity choice: each component is unaware of the other and the formula handles the join.
|
||||
|
||||
**Why not `overflow-anchor`.** CSS scroll-anchoring (`overflow-anchor: auto`, the default in modern browsers) is not the mechanism in use here. It cannot help: the browser's anchor-pick algorithm requires a stable element ABOVE the changing content in the viewport, and at `scrollTop ≈ 0` the slot is the topmost in-viewport element AND the one whose contents change — no stable anchor to pick. Additionally, iOS Safari (until 17) had no support, and historical edge cases interact with rubber-band overscroll at the top. Manual scroll-position compensation in the load-completion `rAF` is the reliable path.
|
||||
|
||||
## Auto-scroll model
|
||||
|
||||
Three effects cooperate. Their ordering is established by the 2026-03-25 race-fix and the 2026-04-25 sentinel addendum.
|
||||
@@ -107,3 +117,4 @@ These items were considered and rejected for the 2026-04-25 work; they live here
|
||||
- 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 on federated channels (parallel loadMessages/loadMoreMessages dedup): the 2026-05-05 hook + clamp-scroll fix above did not cover the federated-LAN reproduction. Playwright trace against the live deployment showed every channel mount firing TWO parallel `GET /channels/:id/messages?limit=50` requests, and every fast-scroll burst firing N parallel `loadMoreMessages` requests. Two cooperating root causes: (a) `AppLayout.tsx:289` AND `MessageList.tsx:267` both call `loadMessages(channelId)` in their channel-mount effects — `MessageList`'s call is required for surfaces that don't have the AppLayout chrome (`VoiceChatPanel`), so neither can be removed unilaterally; (b) `handleScroll`'s load-more block sees `!isLoadingMore` as true in every scroll event of a fast burst because all events share one closure created before the first `setIsLoadingMore(true)` commits. The chatStore guard `if (!force && get().hasMore.has(channelId)) return;` only deduplicates calls *after* the first completes — it does not collapse parallel callers. On a NAT hairpin'd LAN where some federated TCP connections succeed and others hang, having N hung parallel fetches per channel mount keeps `chatStore.isLoading` and component-level `isLoadingMore` true while any one of them is still waiting on the 30 s api-client timeout, producing the visually-stuck pagination skeleton across channel switches. Fix: in-flight Promise dedup at the chatStore level. `loadMessages` and `loadMoreMessages` each consult a module-level `Map<channelId, Promise>`; concurrent callers receive the same Promise. The map entry is cleared in a `finally` block, with a `inFlightLoads.get(channelId) === promise` self-check so a force-reload (WS reconnect) scheduled mid-flight isn't dropped. `force=true` callers bypass dedup intentionally — WS reconnect needs a fresh fetch even if a stale one is still pending. Net effect: one federated fetch per channel mount, one per fast-scroll burst, predictable timeout window, skeleton clears within ≤ 30 s + minDisplay even on full hairpin failure.
|
||||
- 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`).
|
||||
- 2026-05-15 — pagination skeleton layout-shift elimination: the pagination loading skeleton was rendered as a transient inline child at the top of the scroll content (`MessageList.tsx:652-664`). Each mount pushed message content down by the skeleton's height; each unmount snapped it back up. The existing `c.scrollTop = c.scrollHeight - prevScrollHeight` compensation at line 618 only ran at load completion and only against the message prepend — it never saw the skeleton's own mount/unmount as an event to compensate. Fix: render a constant-height slot above the messages whenever `hasMore === true` (defined by the `PAGINATION_SLOT_HEIGHT_PX` constant; see "Top-of-list reservation slot" above). The skeleton's grey-bar contents toggle inside the slot; slot height never changes from the skeleton's lifecycle. Companion changes: (a) raise the load-more trigger from `scrollTop < 50` to `scrollTop < PAGINATION_SLOT_HEIGHT_PX + 50` so the load fires while the slot is still off-screen — the skeleton's appearance overlaps with the user still off-slot — with a `scrollTop >= 0` guard added to neutralize iOS Safari rubber-band overscroll producing briefly-negative `scrollTop` values that would otherwise satisfy the upper bound and fire a spurious load during a rubber-band gesture; (b) correct the prepend math from `c.scrollTop = c.scrollHeight - prevScrollHeight` (an "anchor from bottom" formula that only holds when `prevScrollTop ≈ 0`) to `c.scrollTop = prevScrollTop + (c.scrollHeight - prevScrollHeight)`, which is correct for any prior scroll position. Without the math correction the raised threshold would cause a jump of up to ~`PAGINATION_SLOT_HEIGHT_PX + 50` on every load. The corrected formula also automatically absorbs the (WelcomeHeader_height − slot_height) delta on the final page (slot unmounts, WelcomeHeader mounts), since both pieces are visible in `c.scrollHeight - prevScrollHeight`. (c) Lower `useDelayedLoading`'s threshold to 50ms for the pagination case (initial-load skeleton keeps the default 200ms) — with no layout shift to flicker, the original purpose of the 200ms delay (avoid showing skeleton on near-instant cache hits) survives but the value was over-tuned for the new mechanic. **Manual repro recipe** (regression check): DevTools → Network → Slow 3G → open a channel with deep history → scroll to top. Pre-fix: content visibly pushed down when skeleton appears, snaps back up when skeleton leaves. Post-fix: skeleton fades into pre-reserved slot, no motion of content below; new messages appear above previously-visible content without shifting it.
|
||||
|
||||
@@ -26,6 +26,13 @@ import { SystemMessage } from './SystemMessage';
|
||||
const EMPTY_MESSAGES: MessageWithUser[] = [];
|
||||
const EMPTY_PENDING_BUBBLES: PendingBubble[] = [];
|
||||
|
||||
// Constant-height slot rendered above messages whenever hasMore === true.
|
||||
// Value derived from the pagination skeleton's analytical rendered height
|
||||
// (pt-4 + 3 × (h-10 row) + 2 × mb-5 = 176px after stripping the last row's
|
||||
// mb-5), rounded UP to the nearest 4-pixel step for a buffer. See
|
||||
// docs/systems/message-list.md "Top-of-list reservation slot".
|
||||
const PAGINATION_SLOT_HEIGHT_PX = 200;
|
||||
|
||||
interface MessageListProps {
|
||||
channelId: string;
|
||||
jumpToMessageId?: string | null;
|
||||
@@ -92,7 +99,13 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
||||
const SMOOTH_SCROLL_DEADLINE_MS = 800;
|
||||
const [isLoadingMore, setIsLoadingMore] = useState(false);
|
||||
const showInitialSkeleton = useDelayedLoading(isLoading && messages.length === 0);
|
||||
const showPaginationSkeleton = useDelayedLoading(isLoadingMore);
|
||||
// 50 ms threshold (vs the 200 ms default on showInitialSkeleton above) is
|
||||
// safe here because Task 2's constant-height slot eliminated the layout
|
||||
// shift the 200 ms originally hid. 50 ms is below the ~100 ms visual
|
||||
// perception threshold so near-instant cache hits still complete without
|
||||
// ever rendering the skeleton, while slow loads see the skeleton appear
|
||||
// before the user's eye can register the slot as empty.
|
||||
const showPaginationSkeleton = useDelayedLoading(isLoadingMore, { threshold: 50 });
|
||||
const prevMessagesLength = useRef(0);
|
||||
const prevChannelIdRef = useRef<string>(channelId);
|
||||
const visibleMsgIdRef = useRef<string | null>(null);
|
||||
@@ -597,10 +610,24 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
||||
suppressLoadMore = true;
|
||||
}
|
||||
|
||||
if (!suppressLoadMore && container.scrollTop < 50 && hasMore && !isLoadingMore) {
|
||||
// scrollTop >= 0 guards against iOS Safari rubber-band overscroll producing
|
||||
// briefly-negative scrollTop values, which would otherwise satisfy the
|
||||
// upper bound and fire a spurious load during a rubber-band gesture.
|
||||
if (
|
||||
!suppressLoadMore &&
|
||||
container.scrollTop >= 0 &&
|
||||
container.scrollTop < PAGINATION_SLOT_HEIGHT_PX + 50 &&
|
||||
hasMore &&
|
||||
!isLoadingMore
|
||||
) {
|
||||
const requestChannelId = channelId;
|
||||
setIsLoadingMore(true);
|
||||
// Capture BOTH synchronously, before the await — `prevScrollTop` must
|
||||
// be the pre-await value for the anchor-from-bottom formula in the
|
||||
// rAF callback below to hold. Moving this capture inside the rAF or
|
||||
// after the await silently breaks the math.
|
||||
const prevScrollHeight = container.scrollHeight;
|
||||
const prevScrollTop = container.scrollTop;
|
||||
try {
|
||||
const loaded = await loadMoreMessages(requestChannelId);
|
||||
if (!loaded) return;
|
||||
@@ -615,7 +642,12 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
||||
if (currentChannelIdRef.current !== requestChannelId) return;
|
||||
const c = containerRef.current;
|
||||
if (!c) return;
|
||||
c.scrollTop = c.scrollHeight - prevScrollHeight;
|
||||
// Anchor-from-bottom: keep the user's viewport at the same distance
|
||||
// from the new bottom of content as it was from the old bottom.
|
||||
// `(c.scrollHeight - prevScrollHeight)` is the height of freshly
|
||||
// prepended messages; adding it to `prevScrollTop` keeps the visible
|
||||
// content stationary across the prepend.
|
||||
c.scrollTop = prevScrollTop + (c.scrollHeight - prevScrollHeight);
|
||||
});
|
||||
} finally {
|
||||
setIsLoadingMore(false);
|
||||
@@ -649,17 +681,25 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
|
||||
className="h-full overflow-y-auto overflow-x-hidden no-scrollbar"
|
||||
onScroll={handleScroll}
|
||||
>
|
||||
{showPaginationSkeleton && (
|
||||
<div className="px-4 pt-4" role="status" aria-label="Loading older messages">
|
||||
{Array.from({ length: 3 }, (_, i) => (
|
||||
<div key={i} className="flex gap-3 mb-5" style={{ animationDelay: `${i * 0.15}s` }}>
|
||||
<div className="skeleton skeleton-circle w-10 h-10 flex-shrink-0" style={{ animationDelay: `${i * 0.15}s` }} />
|
||||
<div className="flex-1 space-y-2 pt-1">
|
||||
<div className="skeleton skeleton-bar" style={{ width: `${22 + (i * 9) % 18}%`, animationDelay: `${i * 0.15}s` }} />
|
||||
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${55 + (i * 11) % 35}%`, animationDelay: `${i * 0.15}s` }} />
|
||||
</div>
|
||||
{hasMore && (
|
||||
<div style={{ height: PAGINATION_SLOT_HEIGHT_PX }}>
|
||||
{showPaginationSkeleton && (
|
||||
<div className="px-4 pt-4" role="status" aria-label="Loading older messages">
|
||||
{Array.from({ length: 3 }, (_, i) => (
|
||||
<div
|
||||
key={i}
|
||||
className={`flex gap-3 ${i < 2 ? 'mb-5' : ''}`}
|
||||
style={{ animationDelay: `${i * 0.15}s` }}
|
||||
>
|
||||
<div className="skeleton skeleton-circle w-10 h-10 flex-shrink-0" style={{ animationDelay: `${i * 0.15}s` }} />
|
||||
<div className="flex-1 space-y-2 pt-1">
|
||||
<div className="skeleton skeleton-bar" style={{ width: `${22 + (i * 9) % 18}%`, animationDelay: `${i * 0.15}s` }} />
|
||||
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${55 + (i * 11) % 35}%`, animationDelay: `${i * 0.15}s` }} />
|
||||
</div>
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
))}
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
||||
@@ -139,4 +139,26 @@ describe('useDelayedLoading', () => {
|
||||
// the 300 ms minDisplay floor. The skeleton should be hidden by now.
|
||||
expect(result.current).toBe(false);
|
||||
});
|
||||
|
||||
it('honors a custom threshold passed via options', () => {
|
||||
const { result } = renderHook(
|
||||
({ loading }) => useDelayedLoading(loading, { threshold: 50 }),
|
||||
{ initialProps: { loading: true } },
|
||||
);
|
||||
expect(result.current).toBe(false);
|
||||
act(() => { vi.advanceTimersByTime(49); });
|
||||
expect(result.current).toBe(false);
|
||||
act(() => { vi.advanceTimersByTime(1); });
|
||||
expect(result.current).toBe(true);
|
||||
});
|
||||
|
||||
it('default 200ms threshold does not fire at t=50ms (negative control for the custom-threshold test above)', () => {
|
||||
// Negative control: same conditions with the default would still be false at t=50.
|
||||
const { result: defaultThresholdResult } = renderHook(
|
||||
({ loading }) => useDelayedLoading(loading),
|
||||
{ initialProps: { loading: true } },
|
||||
);
|
||||
act(() => { vi.advanceTimersByTime(50); });
|
||||
expect(defaultThresholdResult.current).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user