fix(webapp): virtualize MessageFeed to fix slow dialog open (release/v1.46.1)#548
Closed
conradkoh wants to merge 17 commits into
Closed
fix(webapp): virtualize MessageFeed to fix slow dialog open (release/v1.46.1)#548conradkoh wants to merge 17 commits into
conradkoh wants to merge 17 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ge chat history Replace the unbounded `displayMessages.map(...)` render with a windowed list powered by @tanstack/react-virtual. Only messages near the viewport are mounted to the DOM, so total node count stays bounded (~tens) instead of scaling with chat history length (thousands). This fixes the slow dialog open reported in backlog ps7etzv4zd1e2ewr02g6e5et0d87b0wm — Radix's aria-hidden sibling walk and RemoveScroll setup are both O(DOM size). Also improves scroll perf, memory usage, and re-render cost on edit/theme toggle. Trade-off: TaskHeader's `position: sticky` no longer works while inside the absolutely-positioned virtual row. Documented for a follow-up. Refs backlog ps7etzv4zd1e2ewr02g6e5et0d87b0wm. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Use a message-ID anchor instead of scrollHeight delta to preserve scroll position when loading older messages. The virtualizer's estimate size makes scrollHeight unreliable. Capture topmost visible message _id before pagination, then scrollToIndex after messages grow.
Purging collapses virtualizer total size, invalidating scrollTop and causing scroll jumps. Remove the purge block from handleScroll and stop destructuring purgeOldMessages from useMessages (the hook definition is untouched).
Replace the fixed 200px estimateSize with a running average of the last 50 measured row heights. Prevents scrollbar thumb resizing and improves scroll-position stability when rows range from 60 to 800+ px.
snapImmediate() sets scrollTop = scrollHeight before the virtualizer has measured the new row, causing a jiggle. After the snap, wait one animation frame then scrollToIndex the last message with align:'end'.
Wrap load-more button, 'Beginning of conversation', and loading spinner in a ResizeObserver-tracked banner div. Pass measured height as paddingStart to useVirtualizer so the virtual list doesn't shift when banner visibility toggles.
Compute the latest visible user message from virtualizer.scrollOffset and render its TaskHeader as a sticky bar above the virtualized list. Suppress the duplicate header inside the virtualized row. Replaces the broken CSS position:sticky which doesn't work in absolutely-positioned virtualizer rows.
The previous useEffect compared against prevMessageCountRef, which is always equal to displayMessages.length by the time effects run because useLayoutEffect updates it first. Use a separate ref updated within this effect so the 'grew' comparison works correctly.
conradkoh
force-pushed
the
fix/virtualize-message-feed
branch
from
628b9ab to
948c825
Compare
…infinite re-render Inline arrow refs create a new function identity every render, causing React to call cleanup(null) then re-invoke measureElement on each DOM node. When estimateSize uses a running average that drifts, each measureElement triggers setState, which re-renders, and the loop never converges → 'Maximum update depth exceeded'. Fix: pass virtualizer.measureElement as the ref directly. Move per-row size sampling into a useEffect that reads virtualizer.getVirtualItems() to avoid setState as a side-effect of measureElement. Regression test: VirtualizerRefStability.test.tsx verifies the correct ref pattern. The full loop requires browser layout and is documented as a manual verification case in the test file's it.todo.
…atch Move the theme init script from a <script> tag inside ThemeProvider (which triggers React's 'Encountered a script tag' warning) to next/script with strategy='beforeInteractive' in layout.tsx. Remove the mounted gate in ThemeProvider that caused server/client to render different trees (hydration mismatch). Always render the provider tree; the init script sets window.__theme before hydration so the SSR output is consistent.
Replace feedRef.scrollHeight (driven by virtualizer estimates, too small on first render) with virtualizer.getTotalSize() for the auto-load check. Gate on measuredSizesByIdRef having at least one entry so the check uses real measured heights, preventing runaway pagination that loads the entire history immediately. Move the effect after useVirtualizer so it has access to both virtualizer and measuredSizesByIdRef.
The auto-load effect (scrollHeight <= clientHeight) is obsolete in virtualized mode. Even after gating on first-row-measurement in commit 8f1f0ea, the running-average estimate can still be small enough for short messages to fall under viewport height, causing runaway pagination. The 'Load older messages' button + scroll-near-top trigger cover all real use cases. The user's manual scroll to the top is the intended trigger.
Owner
Author
|
Superseded by greenfield reimplementation targeting release/v1.50.0 (linear event timeline, new virtualized panel, no sticky headers). Closing per team decision. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause
MessageFeed.tsxrenders every message as a DOM node viadisplayMessages.map(...)with no windowing. Per-message DOM is heavy: TaskHeader + TaskProgress + MessageItem (header + Markdown content + attachments × Markdown + footer with action buttons). Easily 100+ nodes per message → tens of thousands of nodes for an active chatroom.Radix Dialog/AlertDialog/Popover on open runs an O(N)
aria-hiddensibling walk viaaria-hidden-walkand appliesRemoveScroll; both scale with total DOM node count.Fix
Replace the unbounded
displayMessages.map(...)render with a windowed list powered by@tanstack/react-virtual. Only messages near the viewport are mounted to the DOM, so total node count stays bounded (~tens) instead of scaling with chat history length (thousands).Trade-off
TaskHeader's
position: stickyno longer works while inside the absolutely-positioned virtual row. Header becomes an inline section divider instead. Sticky header behavior can be added later via a rangeExtractor.Verification
pnpm --filter webapp typecheck→ cleanpnpm --filter webapp test→ 63 test files, 606 tests, all passingpnpm --filter webapp lint→ cleanManual Smoke Test Plan
Refs backlog
ps7etzv4zd1e2ewr02g6e5et0d87b0wm.