fix(chat): keep scroll container mounted during initial-load skeleton

The initial-load skeleton was rendered as an early return that replaced
the JSX containing `containerRef` / `contentRef`. On slow loads, the
200ms threshold flipped the skeleton on before messages arrived, so when
messages did arrive every scroll-affecting effect (Effect A, the
ResizeObserver, the load handler, scrollend) re-fired exactly once
against null refs and bailed — and never re-attached because no dep
changed when the skeleton finally cleared. Net: chat opened scrolled to
the top instead of the bottom; saved-anchor restore was equally broken.

Render the skeleton as an absolutely-positioned overlay alongside the
(always-mounted) scroll container so all refs stay live across the
loading transition. Encode the constraint in the spec as the
"ContainerRef invariant" so future loading/empty/error UI doesn't
reintroduce the early-return pattern.
This commit is contained in:
Jannis Braun
2026-05-05 00:19:00 +02:00
parent b3cbe9403f
commit c05c04181d
2 changed files with 38 additions and 18 deletions
+7
View File
@@ -11,6 +11,12 @@ The chat message list (`packages/web/src/components/chat/MessageList.tsx`) is re
| `packages/web/src/components/chat/embeds/*.tsx` | Embed renderers — must obey the dimension reservation contract below. | | `packages/web/src/components/chat/embeds/*.tsx` | Embed renderers — must obey the dimension reservation contract below. |
| `packages/web/src/components/chat/AttachmentRenderer.tsx` | Reference for the dimension reservation pattern (`AttachmentRenderer.tsx:81-100`). | | `packages/web/src/components/chat/AttachmentRenderer.tsx` | Reference for the dimension reservation pattern (`AttachmentRenderer.tsx:81-100`). |
## ContainerRef invariant
`containerRef.current` (and `contentRef.current`) MUST be non-null for the entire lifetime of any chat view where messages may arrive or scroll-affecting effects can run. Every effect in this file (initial snap A, ResizeObserver B, capture-phase load handler C, scrollend final-pin D, jump-to-message) reads these refs and bails on a null guard. Each of those effects is keyed on `[messages.length, channelId]` or `[hasMessages, channelId]`, and the inbound transition (`0 → N` / `false → true`) is the *only* re-fire signal during a channel-load lifecycle. If a ref is null at the moment that signal fires, the effect bails — and no later dep change will retry it, leaving the channel permanently broken (initial scroll never lands at bottom, ResizeObserver never observes, scrollend never registers, saved-anchor restore never happens).
**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.
## Auto-scroll model ## Auto-scroll model
Three effects cooperate. Their ordering is established by the 2026-03-25 race-fix and the 2026-04-25 sentinel addendum. Three effects cooperate. Their ordering is established by the 2026-03-25 race-fix and the 2026-04-25 sentinel addendum.
@@ -91,4 +97,5 @@ 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-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.
@@ -569,24 +569,16 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
); );
} }
if (showInitialSkeleton) { // The initial-load skeleton is rendered as an absolutely-positioned overlay
return ( // (NOT an early return) so that the scroll container below — and its
<div className="flex-1 flex flex-col justify-end px-4 pb-6" role="status" aria-label="Loading messages"> // `containerRef` / `contentRef` — stay mounted across the loading transition.
{Array.from({ length: 7 }, (_, i) => ( // Auto-scroll effects (initial snap, ResizeObserver, load-handler, scrollend)
<div key={i} className="flex gap-3 mb-5" style={{ animationDelay: `${i * 0.15}s` }}> // are keyed on `[messages.length, channelId]` / `[hasMessages, channelId]`,
<div className="skeleton skeleton-circle w-10 h-10 flex-shrink-0" style={{ animationDelay: `${i * 0.15}s` }} /> // and each commits its only re-fire signal during the load window. If the
<div className="flex-1 space-y-2 pt-1"> // refs were null at that moment (which they are if the skeleton replaces the
<div className="skeleton skeleton-bar" style={{ width: `${20 + (i * 7) % 20}%`, animationDelay: `${i * 0.15}s` }} /> // container via early-return), every effect bails on its null guard and never
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${50 + (i * 13) % 40}%`, animationDelay: `${i * 0.15}s` }} /> // re-attaches once the skeleton clears, leaving the user scrolled to the top.
{i % 2 === 0 && ( // See docs/systems/message-list.md "ContainerRef invariant".
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${30 + (i * 11) % 35}%`, animationDelay: `${i * 0.15}s` }} />
)}
</div>
</div>
))}
</div>
);
}
return ( return (
<div className="flex-1 relative min-h-0"> <div className="flex-1 relative min-h-0">
@@ -658,6 +650,27 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess
<div ref={bottomRef} /> <div ref={bottomRef} />
</div> </div>
{showInitialSkeleton && (
<div
className="absolute inset-0 z-10 bg-surface-chat flex flex-col justify-end px-4 pb-6 pointer-events-none"
role="status"
aria-label="Loading messages"
>
{Array.from({ length: 7 }, (_, 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: `${20 + (i * 7) % 20}%`, animationDelay: `${i * 0.15}s` }} />
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${50 + (i * 13) % 40}%`, animationDelay: `${i * 0.15}s` }} />
{i % 2 === 0 && (
<div className="skeleton skeleton-bar h-2.5" style={{ width: `${30 + (i * 11) % 35}%`, animationDelay: `${i * 0.15}s` }} />
)}
</div>
</div>
))}
</div>
)}
{!isNearBottom && messages.length > 0 && ( {!isNearBottom && messages.length > 0 && (
<button <button
onClick={() => { onClick={() => {