fix(timeline): stable refs, scroll guard, and listener leak fixes#279
Open
Just-Insane wants to merge 3 commits intoSableClient:devfrom
Open
fix(timeline): stable refs, scroll guard, and listener leak fixes#279Just-Insane wants to merge 3 commits intoSableClient:devfrom
Just-Insane wants to merge 3 commits intoSableClient:devfrom
Conversation
When a sliding-sync limited response fires RoomEvent.TimelineReset (or
the app fires RoomEvent.TimelineRefresh) the useLiveTimelineRefresh
callback was unconditionally calling setAtBottom(true) and incrementing
scrollToBottomRef, scrolling the user to the bottom even if they had
scrolled up to read history.
Three coordinated changes:
1. Add RoomEvent.TimelineReset handler to useLiveTimelineRefresh.
The SDK emits TimelineReset on the EventTimelineSet (not the Room)
when a limited sync response replaces the live EventTimeline. Without
this listener the stored linkedTimelines reference the old detached
chain; back-pagination silently no-ops, freezing the room.
2. Gate the viewport scroll on atBottomRef.current (prior position).
Capture wasAtBottom before calling setTimeline(getInitialTimeline).
Only call setAtBottom(true) and increment scrollToBottomRef when the
user was already at the bottom. When scrolled up we still reinit the
timeline (the old chain is gone) but avoid the forced scroll.
3. Add timelineJustResetRef to allow the self-heal effect to run when
atBottom=false after a reset. The SDK fires TimelineReset before
populating the fresh timeline, so getInitialTimeline sees range.end=0.
Without the atBottom guard bypass the range stays at {0,0} as events
arrive, leaving the virtual paginator rendering nothing. The ref is
cleared on first successful heal, after which normal atBottom-gated
behaviour resumes.
useLiveEventArrive, useRelationUpdate, useLiveTimelineRefresh, and
useThreadUpdate each passed their callback argument into the useEffect
dependency array. This caused the listener to be removed and re-added
every time the callback identity changed (e.g. when unreadInfo toggled
or when any other parent-scope value included in the useCallback deps
changed).
Under normal conditions React's synchronous effect cleanup guarantees
one listener at a time. But during sliding-sync error/retry cycles the
client emits MaxListenersExceededWarning (11 session_ended, 51
RoomState.events) confirming that listeners accumulate. When the
callbacks change fast enough — or React defers cleanup in concurrent
mode — multiple handleTimelineEvent instances are live simultaneously.
Each one independently calls setTimeline({ range: start+1, end+1 }) for
the same arriving event, producing the spurious scroll jumps reported.
Fix: use the stable callback ref pattern in all four hooks.
- Add a ref (onArriveRef / onRelationRef / onRefreshRef / onUpdateRef)
- Assign ref.current = callback on every render (always up-to-date)
- Remove the callback from the useEffect dep array (effect runs only
when `room` changes)
- Inside handlers, delegate via ref.current() instead of the captured
closure value
The event listener identity is now stable for the entire lifetime of
each room mount. Listener counts stay at 1 regardless of how many
re-renders occur or how many sync error/retry cycles fire.
- Add unreadInfoRef in RoomTimeline; remove unreadInfo from useLiveEventArrive callback deps and LocalEchoUpdated effect deps to stop per-message listener churn - Fix CallEmbed .bind(this) leak: bind handlers once in start(), push cleanup to disposables so dispose() correctly removes all listeners - Add stable refs in useCallSignaling for mutedRoomId/ring callbacks; tighten effect deps to [mx, mDirects, setMutedRoomId] to prevent SessionEnded listener accumulation during sync retry cycles
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.
Five related fixes for scroll stability and event-listener accumulation triggered by sync error/retry cycles.
Stable callback ref pattern for room event listener hooks (
1bd5281c)eventsLengthwas included in theuseCallbackdeps ofonRangeChange, causing a cascade: every new event → newonRangeChange→ newpaginatefn → newIntersectionObserver(and a stale one firing in the brief overlap window) → spuriouspaginate(Forward)calls with an old range. Fixed by storingeventsLengthin a ref and removing it from the dep array.Don't force scroll-to-bottom on sync gap when scrolled up (
832f597d)When adaptive sliding-sync bulk-loads historical events the large jump in
eventsLengthcauses a brief DOM layout shift →IntersectionObserverfires a spuriousisIntersecting=truefor the bottom anchor → the "stay at bottom" effect resets the user's scroll position even if they were reading old messages. Fixed by trackingprevEventsLengthRefand only running the effect whentimeline.range.endwas already tracking the live end before the jump.Eliminate
LocalEchoUpdatedanduseLiveEventArrivelistener churn fromunreadInfostate changes (76fa7f30)unreadInfo(a piece of React state) was included in theuseCallbackdep array foruseLiveEventArriveand in theLocalEchoUpdateduseEffectdep array.unreadInfotransitions nearly every received message, causing both closures to be recreated and — in the case ofLocalEchoUpdatedwhich has no stable-ref wrapper — the listener to be detached and re-attached every time. Fixed by introducingunreadInfoRefalongside the existingreadUptoEventIdRefand removingunreadInfofrom both dep arrays.Fix
MaxListenersaccumulation inCallEmbed—RoomState.eventsleak (76fa7f30)CallEmbed.start()registered fourMatrixClientlisteners using.bind(this)inline, anddispose()tried to remove them with fresh.bind(this)calls. Every.bind()creates a new function object, so theremoveListenerwas a no-op and listeners leaked on every call embed creation (51RoomState.eventslisteners). Fixed by binding once insidestart(), capturing the four refs as localconsts, and pushing a single disposable for cleanup.Fix
MaxListenersaccumulation inuseCallSignaling—session_ended/RoomState.eventsleak (76fa7f30)The main
useEffectinuseCallSignalingincludedmutedRoomId,playRinging,stopRinging, andplayOutgoingRingingin its dep array.mutedRoomIdtoggles on every call session end, causing the effect to re-run and re-register theSessionEndedandRoomState.eventslisteners in rapid succession during sync retry cycles (11session_endedlisteners). Fixed by storing all four volatile values in stable refs and tightening the dep array to[mx, mDirects, setMutedRoomId].Closes the MaxListeners warnings reported in the Sentry bug:
Supersedes #277 and #278.