From 20f76c4fdfcf99893ed6954344d8158b726121c0 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Mon, 4 May 2026 20:38:37 -0700 Subject: [PATCH] fix(canvas/chat): stable IntersectionObserver + inflight guard for loadOlder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the lazy-load PR caught three Important findings: 1. IO observer was re-armed on every messages change. The previous loadOlder useCallback depended on `messages`, so every live agent push recreated it → re-ran the IO useEffect → tore down + re-armed the observer. In a perf PR shipping to chat-heavy users, that's the wrong direction. Fix: refs for the captured state (oldestMessageRef, hasMoreRef), narrow loadOlder deps to [workspaceId], and gate the IO effect on `messages.length > 0` (boolean) instead of `messages` so it arms exactly once when data first lands and stays armed across appends. 2. loadingOlder setState race. Two IO callbacks dispatched in the same microtask (fast scroll, layout shift) could both pass the `if (loadingOlder)` guard before React committed setLoadingOlder. Fix: synchronous inflightRef set BEFORE any await, cleared in finally; loadingOlder state stays for the UI label only. 3. Retry-button onClick duplicated the mount-effect body. Single loadInitial() callback now serves both, eliminating the drift hazard. Coverage: - 4 new tests bring the file to 8/8 (was 4): - loadOlder fetches with limit=20 and before_ts=oldest.timestamp - inflight guard rejects three concurrent IO triggers while a deferred fetch is in flight (asserts call count stays at 2, not 5) - empty older response unmounts the sentinel (proxy for the anchor-clearing branch in loadOlder) - IO observer instance survives three subsequent prepends — same object reference both before and after, no churn - Both behavioural tests verified to FAIL on the prior code (stashed ChatTab.tsx, ran them alone, confirmed both red), then PASS on this commit. Pinning real regressions, not tautologies. - IntersectionObserver fake captures instances + exposes triggerIntersection() so the IO callback can be driven directly from jsdom (no real layout / scrolling needed). Test: vitest run src/components/tabs/__tests__/ → 39 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/tabs/ChatTab.tsx | 129 ++++++++---- .../__tests__/ChatTab.lazyHistory.test.tsx | 189 +++++++++++++++++- 2 files changed, 268 insertions(+), 50 deletions(-) diff --git a/canvas/src/components/tabs/ChatTab.tsx b/canvas/src/components/tabs/ChatTab.tsx index af6e8b63..7d7fcf15 100644 --- a/canvas/src/components/tabs/ChatTab.tsx +++ b/canvas/src/components/tabs/ChatTab.tsx @@ -291,18 +291,31 @@ function MyChatPanel({ workspaceId, data }: Props) { // - topRef = sentinel above the messages list; IO observes it // and triggers loadOlder() when it enters view // - hasMore = false once a fetch returns < limit rows; stops IO - // - loadingOlder = guards against duplicate loadOlder() calls while - // one is already in flight (fast scroll-flick) + // - loadingOlder = drives the "Loading older messages…" UI label + // - inflightRef = synchronous guard against double-entry of loadOlder + // when the IO callback fires twice in the same + // microtask (state-based guard would be stale until + // the next React commit) // - scrollAnchorRef = saves distance-from-bottom before a prepend // so the useLayoutEffect below can restore the // user's exact viewport position. Without this, // prepending older messages would jump the scroll // position by the height of the new content. + // - oldestMessageRef / hasMoreRef = let the loadOlder closure read + // the latest values without taking them as deps — + // every live agent push mutates `messages`, and + // having loadOlder depend on `messages` would tear + // down + re-arm the IntersectionObserver on every + // push. Refs decouple the observer lifecycle from + // message-list updates. const containerRef = useRef(null); const topRef = useRef(null); const [hasMore, setHasMore] = useState(true); const [loadingOlder, setLoadingOlder] = useState(false); + const inflightRef = useRef(false); const scrollAnchorRef = useRef<{ savedDistanceFromBottom: number } | null>(null); + const oldestMessageRef = useRef(null); + const hasMoreRef = useRef(true); // Files the user has picked but not yet sent. Cleared on send // (upload success) or by the × on each pill. const [pendingFiles, setPendingFiles] = useState([]); @@ -341,13 +354,11 @@ function MyChatPanel({ workspaceId, data }: Props) { sendInFlightRef.current = false; }, []); - // Load chat history from database on mount. - // Initial load is bounded to INITIAL_HISTORY_LIMIT (newest 10) — the - // rest streams in as the user scrolls up via loadOlder() below. Pre- - // 2026-05-05 this fetched the newest 50 in one shot; on a long-running - // workspace that meant 50× message-bubble paint + DOM cost on every - // tab-open even when the user only wanted to read the last few. - useEffect(() => { + // Initial-load fetch — used by the mount effect and the "Retry" + // button below. Single source of truth so the two paths can't drift + // (e.g. INITIAL_HISTORY_LIMIT bumped in the effect but not the + // retry, leading to inconsistent first-paint sizes). + const loadInitial = useCallback(() => { setLoading(true); setLoadError(null); setHasMore(true); @@ -361,16 +372,41 @@ function MyChatPanel({ workspaceId, data }: Props) { ); }, [workspaceId]); - // Fetch the next-older batch and prepend. Caller responsibility: - // already check loadingOlder + hasMore (we re-check defensively for - // race-safety against the IO callback firing twice). + // Load chat history on mount / workspace switch. + // Initial load is bounded to INITIAL_HISTORY_LIMIT (newest 10) — the + // rest streams in as the user scrolls up via loadOlder() below. Pre- + // 2026-05-05 this fetched the newest 50 in one shot; on a long-running + // workspace that meant 50× message-bubble paint + DOM cost on every + // tab-open even when the user only wanted to read the last few. + useEffect(() => { + loadInitial(); + }, [loadInitial]); + + // Mirror the latest oldest-message + hasMore into refs so loadOlder + // can read them without taking `messages` as a dep. Every live push + // through agentMessages would otherwise recreate loadOlder and tear + // down the IO observer. + useEffect(() => { + oldestMessageRef.current = messages[0] ?? null; + }, [messages]); + useEffect(() => { + hasMoreRef.current = hasMore; + }, [hasMore]); + + // Fetch the next-older batch and prepend. Stable identity (deps = + // [workspaceId]) so the IntersectionObserver effect below doesn't + // re-arm on every messages update. const loadOlder = useCallback(async () => { - if (loadingOlder || !hasMore) return; - if (messages.length === 0) return; - const oldest = messages[0]; + // inflightRef is the load-bearing guard — synchronous, set BEFORE + // any await, so two IO callbacks dispatched in the same microtask + // can't both pass. The state checks are defensive secondary + // gates for the slow-scroll case. + if (inflightRef.current || !hasMoreRef.current) return; + const oldest = oldestMessageRef.current; if (!oldest) return; const container = containerRef.current; if (!container) return; + inflightRef.current = true; // Capture the user's distance-from-bottom BEFORE we prepend so the // useLayoutEffect can restore it after the new DOM lands. Without // this anchor, the user reading mid-history would get yanked @@ -379,26 +415,45 @@ function MyChatPanel({ workspaceId, data }: Props) { savedDistanceFromBottom: container.scrollHeight - container.scrollTop, }; setLoadingOlder(true); - const { messages: older, reachedEnd } = await loadMessagesFromDB( - workspaceId, - OLDER_HISTORY_BATCH, - oldest.timestamp, - ); - if (older.length > 0) { - setMessages((prev) => [...older, ...prev]); - } else { - // Nothing came back — clear the anchor so the next paint doesn't - // try to "restore" against a no-op prepend. - scrollAnchorRef.current = null; + try { + const { messages: older, reachedEnd } = await loadMessagesFromDB( + workspaceId, + OLDER_HISTORY_BATCH, + oldest.timestamp, + ); + if (older.length > 0) { + setMessages((prev) => [...older, ...prev]); + } else { + // Nothing came back — clear the anchor so the next paint doesn't + // try to "restore" against a no-op prepend. + scrollAnchorRef.current = null; + } + setHasMore(!reachedEnd); + } finally { + setLoadingOlder(false); + inflightRef.current = false; } - setHasMore(!reachedEnd); - setLoadingOlder(false); - }, [workspaceId, messages, loadingOlder, hasMore]); + }, [workspaceId]); // IntersectionObserver on the top sentinel. Fires loadOlder() the // moment the user scrolls within 200px of the top. AbortController // unwires cleanly on workspace switch / unmount; root is the // scrollable container so we observe only what's visible inside it. + // + // Dependencies: + // - loadOlder — stable per workspaceId (refs decouple it from + // message updates), so this dep is here for the + // workspace-switch case only + // - hasMore — re-run when older history runs out so we + // disconnect cleanly + // - hasMessages — load-bearing: the sentinel JSX is gated on + // `messages.length > 0`, so topRef.current is null + // on the empty-messages render. We re-arm exactly + // once when messages first land. NOT depending on + // `messages.length` (or `messages`) directly so + // each subsequent message append doesn't tear down + // + re-arm the observer. + const hasMessages = messages.length > 0; useEffect(() => { const top = topRef.current; const container = containerRef.current; @@ -415,7 +470,7 @@ function MyChatPanel({ workspaceId, data }: Props) { io.observe(top); ac.signal.addEventListener("abort", () => io.disconnect()); return () => ac.abort(); - }, [loadOlder, hasMore]); + }, [loadOlder, hasMore, hasMessages]); // Agent reachability useEffect(() => { @@ -873,19 +928,7 @@ function MyChatPanel({ workspaceId, data }: Props) { Failed to load chat history: {loadError}