perf(timeline): memoize VList timeline items to prevent mass re-renders#660
Open
Just-Insane wants to merge 10 commits intoSableClient:devfrom
Open
perf(timeline): memoize VList timeline items to prevent mass re-renders#660Just-Insane wants to merge 10 commits intoSableClient:devfrom
Just-Insane wants to merge 10 commits intoSableClient:devfrom
Conversation
- useTimelineSync: add mutationVersion counter, incremented only on mutations (reactions, edits, local-echo, thread updates) via a new triggerMutation() callback. Live event arrivals do NOT bump it — the eventsLength change already signals those. - useProcessedTimeline: add stableRefsCache (useRef<Map>) that reuses the same ProcessedEvent object across renders when mutationVersion is unchanged and structural fields (collapsed, dividers, eventSender) are identical. New mutationVersion bypasses the cache so fresh objects reach React on actual content mutations. - RoomTimeline: define TimelineItem as React.memo outside the component function so the type is stable. Render via renderFnRef (synchronously updated each cycle) to avoid stale closures without adding to deps. Per-item boolean props (isHighlighted, isEditing, isReplying, isOpenThread) and a settingsEpoch object let memo skip re-renders on unchanged items while still re-rendering the one item that changed. vListIndices deps changed from timelineSync.timeline (always a new object from spread) to timelineSync.timeline.linkedTimelines + timelineSync.mutationVersion. Expected gains: Scrolling: 0 item re-renders (was: all visible items) New message: 1 item re-renders (was: all) focusItem/editId change: 1-2 items (was: all) Reactions/edits/mutations: all items (same as before, content changed) Settings change: all items via settingsEpoch (same as before)
…ptions ThreadDrawer calls useProcessedTimeline without an equivalent mutation counter (it doesn't use useTimelineSync). Making the field optional with a default of 0 means: - ThreadDrawer gets stable-ref caching for free on subsequent renders (isMutation=false after first render), which is correct — it doesn't wrap items in React.memo TimelineItem. - RoomTimeline continues to pass the real mutationVersion so its TimelineItem memo components are refreshed when content mutates. - pnpm typecheck / pnpm build no longer fail with TS2345.
Contributor
Author
|
Added a follow-up fix in the latest commit: |
…mmatic scroll-to-bottom After setIsReady(true) commits, virtua can fire onScroll events with isNowAtBottom=false during its height-correction pass (particularly on first visit when item heights above the viewport haven't been rendered yet). These intermediate events were driving atBottomState to false while isReady=true, flashing the 'Jump to Latest' button. Add programmaticScrollToBottomRef: set it before each scrollToIndex bottom-scroll, suppress the first intermediate false event (clearing the guard immediately), so the next event — the corrected position or a real user scroll — is processed normally.
…ctive through all intermediate VList events Previously programmaticScrollToBottomRef was only set at a few specific call-sites and cleared after the first suppressed intermediate event. VList fires several height-correction scroll events after scrollTo(); the second one (after the clear) would call setAtBottom(false) and flash "Jump to Latest". - Move programmaticScrollToBottomRef.current = true into scrollToBottom() itself so all callers (live message arrival, timeline refresh, auto-scroll effect) are automatically guarded without missing a call-site. - Remove the guard clear in the else branch; the guard now stays active until VList explicitly confirms isNowAtBottom = true. fix(notifications): skip loadEventTimeline when event is already in live timeline When a notification tap opens a room, NotificationJumper was always navigating with the eventId URL path which triggered loadEventTimeline → roomInitialSync. If sliding sync had already delivered the event to the live timeline this produced a sparse historical slice that (a) looked like a brand-new chat and (b) left the room empty when the user navigated away and returned without the eventId. Check whether the event is in the live timeline before navigating; if it is present, open the room at the live bottom instead. Historical events still use the eventId path.
…n-empty When the app wakes from a killed state and the user taps a notification, performJump fires during the initial sync window before the room's live timeline has been populated by sliding sync. Previously, eventInLive was always false in this case, so we fell back to loadEventTimeline → roomInitialSync which loaded a sparse historical slice. On subsequent visits to the room without the eventId the room appeared empty because the live timeline was never populated for the initial roomInitialSync result. Two changes: 1. Guard: if the live timeline is completely empty, return early and wait rather than navigating — the RoomEvent.Timeline listener below retries once events start arriving. 2. Listen on RoomEvent.Timeline for the target room so performJump re-runs as soon as the first event arrives in the room, at which point the notification event is almost certainly also present so we can navigate without eventId (avoiding loadEventTimeline entirely).
…Id slice When the user taps a push notification and the app loads a historical sparse timeline via loadEventTimeline (eventId URL path), the VList item heights for those few events were being written to the room's scroll cache. On the next visit to the room (live timeline, many more events), the RoomTimeline mount read the stale cache and passed its heights to the new VList instance. The height mismatch between the sparse and live timelines caused incorrect scroll-position restoration, making the room appear to show stale or mispositioned messages. Guard the roomScrollCache.save call with !eventId so historical views never overwrite the live-timeline cache. The next live visit will either use the pre-existing (untouched) live cache or fall back to the first-visit 80 ms measurement path.
…ndex in stable-ref check
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.
Description
Wraps each VList timeline item in
React.memoso that only the items whose props actually changed re-render when state updates occur (e.g. typing indicators arriving, read receipts updating, or new messages landing while the user is scrolled away from the bottom).Props passed to each item beyond
dataandrenderRef—isHighlighted,isEditing,isReplying,isOpenThread,settingsEpoch— exist solely to giveReact.memo's shallow-equality comparator fine-grained control: changing any of them causes only the one affected item to re-render rather than the entire visible list.Also makes
mutationVersionoptional (default0) inUseProcessedTimelineOptionsso call sites that don't participate in the mutation tracking (e.g.ThreadDrawer) continue to compile without changes.Fixes #
Type of change
Checklist:
AI disclosure:
useProcessedTimeline.ts— added astableRefsCache(auseRef<Map<string, ProcessedEvent>>) that keeps the previous render'sProcessedEventobjects alive. On eachuseMemorun, ifmutationVersionhasn't changed, events whose identity hasn't changed are returned from the cache (same object reference) rather than newly constructed. This meansReact.memosees no prop change for stable items and skips their render.RoomTimeline.tsx—TimelineItemis aReact.memo-wrapped component declared outside the parent function so it is never re-created. ArenderFnRefref captures the render function soTimelineItemitself only needs a stable ref in props, not the function.settingsEpochis auseRef({}).currentthat is replaced with{}whenever any layout/display setting changes, forcing all visible items to pick up the new settings in one pass.