fix(chat): dedup parallel loadMessages/loadMoreMessages per channel
Live network trace via playwright showed every channel mount firing TWO parallel GET /channels/:id/messages requests (AppLayout + MessageList both call loadMessages on mount; neither can be removed because MessageList is also rendered by VoiceChatPanel which doesn't call it), and every fast-scroll burst firing N parallel loadMoreMessages calls (handleScroll closure shares !isLoadingMore=true across the burst). The hasMore-based guard only deduplicates calls AFTER the first completes — it does not collapse parallel callers. On a NAT hairpin'd LAN where some federated TCP connections hang, having N hung parallel fetches keeps the pagination skeleton stuck while any one is still waiting on the 30 s api-client timeout. Fix: in-flight Promise dedup at the chatStore level. Module-level Maps keyed on channelId; concurrent callers receive the same Promise. Cleared in finally with self-check so a force-reload mid-flight isn't dropped. force=true bypasses dedup (WS reconnect intent).
This commit is contained in:
@@ -99,4 +99,5 @@ 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 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-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`).
|
||||||
|
|||||||
@@ -11,6 +11,23 @@ const MAX_MESSAGES_PER_CHANNEL = 200;
|
|||||||
const MAX_CACHED_CHANNELS = 20;
|
const MAX_CACHED_CHANNELS = 20;
|
||||||
const EVICT_TO_CHANNELS = 15;
|
const EVICT_TO_CHANNELS = 15;
|
||||||
|
|
||||||
|
// In-flight Promise dedup. The store has multiple call sites that may invoke
|
||||||
|
// `loadMessages` / `loadMoreMessages` for the same channel in parallel before
|
||||||
|
// the first call's awaited result has populated `hasMore`/`messages` (which is
|
||||||
|
// the only state-based dedup the original guards relied on):
|
||||||
|
// - `AppLayout` and `MessageList` BOTH fire `loadMessages` on channel mount
|
||||||
|
// because `MessageList` is also rendered by surfaces that don't have the
|
||||||
|
// `AppLayout` chrome (`VoiceChatPanel`).
|
||||||
|
// - `MessageList.handleScroll`'s load-more block can fire multiple times in
|
||||||
|
// a single fast-scroll burst before React commits the `isLoadingMore=true`
|
||||||
|
// state update, sharing one closure with `!isLoadingMore` still true.
|
||||||
|
// Without dedup, each parallel call opens its own federated fetch — on a NAT
|
||||||
|
// hairpin'd LAN this means N hung TCP connections per channel mount, and the
|
||||||
|
// pagination skeleton (driven by component-level `isLoadingMore`) sticks while
|
||||||
|
// any of them are still waiting on the 30 s api-client timeout.
|
||||||
|
const inFlightLoads = new Map<string, Promise<void>>();
|
||||||
|
const inFlightLoadMores = new Map<string, Promise<boolean>>();
|
||||||
|
|
||||||
interface TypingUser {
|
interface TypingUser {
|
||||||
userId: string;
|
userId: string;
|
||||||
username: string;
|
username: string;
|
||||||
@@ -161,30 +178,55 @@ export const useChatStore = create<ChatState>((set, get) => ({
|
|||||||
// For server channels, bail if we don't know which instance owns this channel yet.
|
// For server channels, bail if we don't know which instance owns this channel yet.
|
||||||
// The remote WS ready handler will call loadMessages once the map is populated.
|
// The remote WS ready handler will call loadMessages once the map is populated.
|
||||||
if (!isDm && !useSpaceStore.getState().channelOriginMap.has(channelId)) return;
|
if (!isDm && !useSpaceStore.getState().channelOriginMap.has(channelId)) return;
|
||||||
set({ isLoading: true, loadError: null });
|
|
||||||
try {
|
|
||||||
const origin = getChannelOrigin(channelId);
|
|
||||||
const client = getApiForOrigin(origin);
|
|
||||||
const messages = isDm
|
|
||||||
? await client.dm.messages(channelId)
|
|
||||||
: await client.channels.messages(channelId);
|
|
||||||
|
|
||||||
// Normalize remote asset URLs (avatars, attachment filenames)
|
// Parallel-call dedup: if a non-forced load for this channel is already in
|
||||||
if (origin) {
|
// flight, return that Promise instead of starting a second fetch. `force`
|
||||||
for (const msg of messages) normalizeMessageAssets(msg, origin);
|
// bypasses the dedup because callers using it (WS reconnect) explicitly
|
||||||
|
// want a fresh fetch even if one is already pending. See `inFlightLoads`
|
||||||
|
// declaration for the full rationale.
|
||||||
|
if (!force) {
|
||||||
|
const existing = inFlightLoads.get(channelId);
|
||||||
|
if (existing) return existing;
|
||||||
|
}
|
||||||
|
|
||||||
|
const promise = (async () => {
|
||||||
|
set({ isLoading: true, loadError: null });
|
||||||
|
try {
|
||||||
|
const origin = getChannelOrigin(channelId);
|
||||||
|
const client = getApiForOrigin(origin);
|
||||||
|
const messages = isDm
|
||||||
|
? await client.dm.messages(channelId)
|
||||||
|
: await client.channels.messages(channelId);
|
||||||
|
|
||||||
|
// Normalize remote asset URLs (avatars, attachment filenames)
|
||||||
|
if (origin) {
|
||||||
|
for (const msg of messages) normalizeMessageAssets(msg, origin);
|
||||||
|
}
|
||||||
|
|
||||||
|
set((state) => {
|
||||||
|
const newMessages = new Map(state.messages);
|
||||||
|
newMessages.set(channelId, messages as MessageWithUser[]);
|
||||||
|
const newHasMore = new Map(state.hasMore);
|
||||||
|
newHasMore.set(channelId, messages.length >= 50);
|
||||||
|
const newAccessTimes = new Map(state.channelAccessTimes);
|
||||||
|
newAccessTimes.set(channelId, Date.now());
|
||||||
|
return { messages: newMessages, hasMore: newHasMore, channelAccessTimes: newAccessTimes, isLoading: false, loadError: null };
|
||||||
|
});
|
||||||
|
} catch (err) {
|
||||||
|
set({ isLoading: false, loadError: (err as Error).message || 'Failed to load messages' });
|
||||||
}
|
}
|
||||||
|
})();
|
||||||
|
|
||||||
set((state) => {
|
inFlightLoads.set(channelId, promise);
|
||||||
const newMessages = new Map(state.messages);
|
try {
|
||||||
newMessages.set(channelId, messages as MessageWithUser[]);
|
await promise;
|
||||||
const newHasMore = new Map(state.hasMore);
|
} finally {
|
||||||
newHasMore.set(channelId, messages.length >= 50);
|
// Only clear the entry if it still points at our Promise — a force-reload
|
||||||
const newAccessTimes = new Map(state.channelAccessTimes);
|
// scheduled while we were in flight may have replaced it, and we don't
|
||||||
newAccessTimes.set(channelId, Date.now());
|
// want to drop the newer entry.
|
||||||
return { messages: newMessages, hasMore: newHasMore, channelAccessTimes: newAccessTimes, isLoading: false, loadError: null };
|
if (inFlightLoads.get(channelId) === promise) {
|
||||||
});
|
inFlightLoads.delete(channelId);
|
||||||
} catch (err) {
|
}
|
||||||
set({ isLoading: false, loadError: (err as Error).message || 'Failed to load messages' });
|
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|
||||||
@@ -196,30 +238,52 @@ export const useChatStore = create<ChatState>((set, get) => ({
|
|||||||
const oldestMessage = existing[0];
|
const oldestMessage = existing[0];
|
||||||
if (!oldestMessage) return false;
|
if (!oldestMessage) return false;
|
||||||
|
|
||||||
try {
|
// Parallel-call dedup. `MessageList.handleScroll`'s load-more block can
|
||||||
const isDm = isDmChannel(channelId);
|
// fire several times in one fast-scroll burst before React commits the
|
||||||
const origin = getChannelOrigin(channelId);
|
// `isLoadingMore=true` setState — every call sees the same closure with
|
||||||
const client = getApiForOrigin(origin);
|
// `!isLoadingMore` still true. Without dedup, each spawns its own
|
||||||
const olderMessages = isDm
|
// federated fetch, and on a NAT hairpin'd LAN that means N hung TCP
|
||||||
? await client.dm.messages(channelId, oldestMessage.id)
|
// connections per scroll burst, each independently waiting on the 30 s
|
||||||
: await client.channels.messages(channelId, oldestMessage.id);
|
// api-client timeout. Dedup collapses them to one, and every caller's
|
||||||
|
// `await` resolves together.
|
||||||
|
const existingPromise = inFlightLoadMores.get(channelId);
|
||||||
|
if (existingPromise) return existingPromise;
|
||||||
|
|
||||||
// Normalize remote asset URLs (avatars, attachment filenames)
|
const promise = (async () => {
|
||||||
if (origin) {
|
try {
|
||||||
for (const msg of olderMessages) normalizeMessageAssets(msg, origin);
|
const isDm = isDmChannel(channelId);
|
||||||
|
const origin = getChannelOrigin(channelId);
|
||||||
|
const client = getApiForOrigin(origin);
|
||||||
|
const olderMessages = isDm
|
||||||
|
? await client.dm.messages(channelId, oldestMessage.id)
|
||||||
|
: await client.channels.messages(channelId, oldestMessage.id);
|
||||||
|
|
||||||
|
// Normalize remote asset URLs (avatars, attachment filenames)
|
||||||
|
if (origin) {
|
||||||
|
for (const msg of olderMessages) normalizeMessageAssets(msg, origin);
|
||||||
|
}
|
||||||
|
|
||||||
|
set((state) => {
|
||||||
|
const newMessages = new Map(state.messages);
|
||||||
|
const current = newMessages.get(channelId) ?? [];
|
||||||
|
newMessages.set(channelId, [...(olderMessages as MessageWithUser[]), ...current]);
|
||||||
|
const newHasMore = new Map(state.hasMore);
|
||||||
|
newHasMore.set(channelId, olderMessages.length >= 50);
|
||||||
|
return { messages: newMessages, hasMore: newHasMore };
|
||||||
|
});
|
||||||
|
return olderMessages.length > 0;
|
||||||
|
} catch {
|
||||||
|
return false;
|
||||||
}
|
}
|
||||||
|
})();
|
||||||
|
|
||||||
set((state) => {
|
inFlightLoadMores.set(channelId, promise);
|
||||||
const newMessages = new Map(state.messages);
|
try {
|
||||||
const current = newMessages.get(channelId) ?? [];
|
return await promise;
|
||||||
newMessages.set(channelId, [...(olderMessages as MessageWithUser[]), ...current]);
|
} finally {
|
||||||
const newHasMore = new Map(state.hasMore);
|
if (inFlightLoadMores.get(channelId) === promise) {
|
||||||
newHasMore.set(channelId, olderMessages.length >= 50);
|
inFlightLoadMores.delete(channelId);
|
||||||
return { messages: newMessages, hasMore: newHasMore };
|
}
|
||||||
});
|
|
||||||
return olderMessages.length > 0;
|
|
||||||
} catch {
|
|
||||||
return false;
|
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user