diff --git a/.changeset/fix-timeline-refresh-scroll.md b/.changeset/fix-timeline-refresh-scroll.md new file mode 100644 index 000000000..d042bd01e --- /dev/null +++ b/.changeset/fix-timeline-refresh-scroll.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +Fix spurious scroll-to-bottom and MaxListeners warnings on sync gap: stable callback refs and prevEventsLength guard in RoomTimeline, correct CallEmbed .bind(this) listener leak, stable refs in useCallSignaling, and unreadInfoRef to stop per-message listener churn diff --git a/src/app/features/room/RoomTimeline.tsx b/src/app/features/room/RoomTimeline.tsx index 6365f851f..748162cb3 100644 --- a/src/app/features/room/RoomTimeline.tsx +++ b/src/app/features/room/RoomTimeline.tsx @@ -460,6 +460,12 @@ const useTimelinePagination = ( }; const useLiveEventArrive = (room: Room, onArrive: (mEvent: MatrixEvent) => void) => { + // Stable ref so the effect dep array only contains `room`. The listener is + // registered once per room mount; onArrive can change freely without causing + // listener churn during rapid re-renders (e.g. sync error/retry cycles). + const onArriveRef = useRef(onArrive); + onArriveRef.current = onArrive; + useEffect(() => { // Capture the live timeline and registration time. Events appended to the // live timeline AFTER this point can be genuinely new even when @@ -486,14 +492,14 @@ const useLiveEventArrive = (room: Room, onArrive: (mEvent: MatrixEvent) => void) data.timeline === liveTimeline && mEvent.getTs() >= registeredAt - 60_000); if (!isLive) return; - onArrive(mEvent); + onArriveRef.current(mEvent); }; const handleRedaction: RoomEventHandlerMap[RoomEvent.Redaction] = ( mEvent: MatrixEvent, eventRoom: Room | undefined ) => { if (eventRoom?.roomId !== room.roomId) return; - onArrive(mEvent); + onArriveRef.current(mEvent); }; room.on(RoomEvent.Timeline, handleTimelineEvent); @@ -502,10 +508,13 @@ const useLiveEventArrive = (room: Room, onArrive: (mEvent: MatrixEvent) => void) room.removeListener(RoomEvent.Timeline, handleTimelineEvent); room.removeListener(RoomEvent.Redaction, handleRedaction); }; - }, [room, onArrive]); + }, [room]); // stable: re-register only when room changes, not on callback identity changes }; const useRelationUpdate = (room: Room, onRelation: () => void) => { + const onRelationRef = useRef(onRelation); + onRelationRef.current = onRelation; + useEffect(() => { const handleTimelineEvent: EventTimelineSetHandlerMap[RoomEvent.Timeline] = ( mEvent: MatrixEvent, @@ -519,42 +528,61 @@ const useRelationUpdate = (room: Room, onRelation: () => void) => { // also need to trigger a re-render so makeReplaced state is reflected. if (eventRoom?.roomId !== room.roomId || data.liveEvent) return; if (mEvent.getRelation()?.rel_type === RelationType.Replace) { - onRelation(); + onRelationRef.current(); } }; room.on(RoomEvent.Timeline, handleTimelineEvent); return () => { room.removeListener(RoomEvent.Timeline, handleTimelineEvent); }; - }, [room, onRelation]); + }, [room]); }; const useLiveTimelineRefresh = (room: Room, onRefresh: () => void) => { + const onRefreshRef = useRef(onRefresh); + onRefreshRef.current = onRefresh; + useEffect(() => { const handleTimelineRefresh: RoomEventHandlerMap[RoomEvent.TimelineRefresh] = (r: Room) => { if (r.roomId !== room.roomId) return; - onRefresh(); + onRefreshRef.current(); }; + // The SDK fires RoomEvent.TimelineReset on the EventTimelineSet (not the Room) + // when a limited sliding-sync response replaces the live EventTimeline with a + // fresh one. Without this handler, the stored linkedTimelines reference the old + // detached chain and back-pagination silently no-ops, freezing the room. + const handleTimelineReset: EventTimelineSetHandlerMap[RoomEvent.TimelineReset] = () => { + onRefreshRef.current(); + }; + const unfilteredTimelineSet = room.getUnfilteredTimelineSet(); room.on(RoomEvent.TimelineRefresh, handleTimelineRefresh); + unfilteredTimelineSet.on(RoomEvent.TimelineReset, handleTimelineReset); return () => { room.removeListener(RoomEvent.TimelineRefresh, handleTimelineRefresh); + unfilteredTimelineSet.removeListener(RoomEvent.TimelineReset, handleTimelineReset); }; - }, [room, onRefresh]); + }, [room]); }; // Trigger re-render when thread reply counts change so the thread chip updates. const useThreadUpdate = (room: Room, onUpdate: () => void) => { + const onUpdateRef = useRef(onUpdate); + onUpdateRef.current = onUpdate; + useEffect(() => { - room.on(ThreadEvent.New, onUpdate); - room.on(ThreadEvent.Update, onUpdate); - room.on(ThreadEvent.NewReply, onUpdate); + // Stable wrapper: the same function identity is kept for the lifetime of + // the room so add/removeListener calls always match. + const handler = () => onUpdateRef.current(); + room.on(ThreadEvent.New, handler); + room.on(ThreadEvent.Update, handler); + room.on(ThreadEvent.NewReply, handler); return () => { - room.removeListener(ThreadEvent.New, onUpdate); - room.removeListener(ThreadEvent.Update, onUpdate); - room.removeListener(ThreadEvent.NewReply, onUpdate); + room.removeListener(ThreadEvent.New, handler); + room.removeListener(ThreadEvent.Update, handler); + room.removeListener(ThreadEvent.NewReply, handler); }; - }, [room, onUpdate]); + }, [room]); }; // Returns the number of replies in a thread, counting actual reply events @@ -765,6 +793,10 @@ export function RoomTimeline({ const imagePackRooms: Room[] = useImagePackRooms(room.roomId, roomToParents); const [unreadInfo, setUnreadInfo] = useState(() => getRoomUnreadInfo(room, true)); + // Stable ref so listeners that only need to *read* unreadInfo don't force + // effect re-registration (and listener churn) every time a new message arrives. + const unreadInfoRef = useRef(unreadInfo); + unreadInfoRef.current = unreadInfo; const readUptoEventIdRef = useRef(); if (unreadInfo) { readUptoEventIdRef.current = unreadInfo.readUptoEventId; @@ -779,6 +811,12 @@ export function RoomTimeline({ atBottomRef.current = val; }, []); + // Set to true by the useLiveTimelineRefresh callback when the timeline is + // re-initialised (TimelineRefresh or TimelineReset). Allows the range self-heal + // effect below to run even when atBottom=false, so the virtual paginator window + // is restored to the live end without forcing a viewport scroll. + const timelineJustResetRef = useRef(false); + const scrollRef = useRef(null); const scrollToBottomRef = useRef({ count: 0, @@ -957,14 +995,17 @@ export function RoomTimeline({ // otherwise we update timeline without paginating // so timeline can be updated with evt like: edits, reactions etc if (atBottomRef.current && atLiveEndRef.current) { - if (document.hasFocus() && (!unreadInfo || mEvt.getSender() === mx.getUserId())) { + if ( + document.hasFocus() && + (!unreadInfoRef.current || mEvt.getSender() === mx.getUserId()) + ) { // Check if the document is in focus (user is actively viewing the app), // and either there are no unread messages or the latest message is from the current user. // If either condition is met, trigger the markAsRead function to send a read receipt. requestAnimationFrame(() => markAsRead(mx, mEvt.getRoomId()!, hideReads)); } - if (!document.hasFocus() && !unreadInfo) { + if (!document.hasFocus() && !unreadInfoRef.current) { setUnreadInfo(getRoomUnreadInfo(room)); } @@ -983,11 +1024,11 @@ export function RoomTimeline({ return; } setTimeline((ct) => ({ ...ct })); - if (!unreadInfo) { + if (!unreadInfoRef.current) { setUnreadInfo(getRoomUnreadInfo(room)); } }, - [mx, room, unreadInfo, hideReads] + [mx, room, hideReads] ) ); @@ -998,7 +1039,7 @@ export function RoomTimeline({ ) => { if (eventRoom?.roomId !== room.roomId) return; setTimeline((ct) => ({ ...ct })); - if (!unreadInfo) { + if (!unreadInfoRef.current) { setUnreadInfo(getRoomUnreadInfo(room)); } }; @@ -1007,7 +1048,7 @@ export function RoomTimeline({ return () => { room.removeListener(RoomEvent.LocalEchoUpdated, handleLocalEchoUpdated); }; - }, [room, unreadInfo, setTimeline, setUnreadInfo]); + }, [room, setTimeline, setUnreadInfo]); const handleOpenEvent = useCallback( async ( @@ -1058,33 +1099,33 @@ export function RoomTimeline({ useLiveTimelineRefresh( room, useCallback(() => { - // Always reinitialize on TimelineRefresh. With sliding sync, a limited - // response replaces the room's live EventTimeline with a brand-new object, - // firing TimelineRefresh. At that moment liveTimelineLinked is stale-false - // (the stored linkedTimelines still reference the old detached object), - // so the previous guard `if (liveTimelineLinked || ...)` would silently - // skip reinit. Back-pagination then calls paginateEventTimeline against - // the dead old timeline, which no-ops, and the IntersectionObserver never - // re-fires because intersection state didn't change — causing a permanent - // hang at the top of the timeline with no spinner and no history loaded. - // Unconditionally reinitializing is correct: TimelineRefresh signals that - // the SDK has replaced the timeline chain, so any stored range/indices - // against the old chain are invalid anyway. + // Always reinitialize on TimelineRefresh/TimelineReset. With sliding sync, + // a limited response replaces the room's live EventTimeline with a brand-new + // object. At that moment liveTimelineLinked is stale-false (stored + // linkedTimelines reference the old detached chain), so any guard on that + // flag would skip reinit, causing back-pagination to no-op silently and the + // room to appear frozen. Unconditional reinit is correct: both events signal + // that stored range/indices against the old chain are invalid. + // + // Only force the viewport to the bottom if the user was already there. + // When the user has scrolled up to read history and a sync gap fires, we + // must still reinit (the old timeline is gone), but scrolling them back to + // the bottom is jarring. Instead we set timelineJustResetRef=true so the + // self-heal effect below can advance the range as events arrive on the fresh + // timeline, without atBottom=true being required. // - // Also force atBottom=true and queue a scroll-to-bottom. The SDK fires - // TimelineRefresh before adding new events to the fresh live timeline, so - // getInitialTimeline captures range.end=0. Once events arrive the - // rangeAtEnd self-heal useEffect needs atBottom=true to run; the - // IntersectionObserver may have transiently fired isIntersecting=false - // during the render transition, leaving atBottom=false and causing the - // "Jump to Latest" button to stick permanently. Forcing atBottom here is - // correct: TimelineRefresh always reinits to the live end, so the user - // should be repositioned to the bottom regardless. + // When the user WAS at the bottom we still call setAtBottom(true) so a + // transient isIntersecting=false from the IntersectionObserver during the + // DOM transition cannot stick the "Jump to Latest" button on-screen. debugLog.info('timeline', 'Live timeline refresh triggered', { roomId: room.roomId }); + const wasAtBottom = atBottomRef.current; + timelineJustResetRef.current = true; setTimeline(getInitialTimeline(room)); - setAtBottom(true); - scrollToBottomRef.current.count += 1; - scrollToBottomRef.current.smooth = false; + if (wasAtBottom) { + setAtBottom(true); + scrollToBottomRef.current.count += 1; + scrollToBottomRef.current.smooth = false; + } }, [room, setAtBottom]) ); @@ -1111,9 +1152,17 @@ export function RoomTimeline({ // position we want to display. Without this, loading more history makes it look // like we've scrolled up because the range (0, 10) is now showing the old events // instead of the latest ones. + // + // Also runs after a timeline reset (timelineJustResetRef=true) even when + // atBottom=false. After TimelineReset the SDK fires the event before populating + // the fresh timeline, so getInitialTimeline sees range.end=0. When events + // arrive eventsLength grows and we need to heal the range back to the live end + // regardless of the user's scroll position. useEffect(() => { - if (atBottom && liveTimelineLinked && eventsLength > timeline.range.end) { - // More events exist than our current range shows. Adjust to stay at bottom. + const resetPending = timelineJustResetRef.current; + if ((atBottom || resetPending) && liveTimelineLinked && eventsLength > timeline.range.end) { + if (resetPending) timelineJustResetRef.current = false; + // More events exist than our current range shows. Adjust to the live end. setTimeline((ct) => ({ ...ct, range: { diff --git a/src/app/hooks/useCallSignaling.ts b/src/app/hooks/useCallSignaling.ts index cd657e477..1e8a7dfef 100644 --- a/src/app/hooks/useCallSignaling.ts +++ b/src/app/hooks/useCallSignaling.ts @@ -29,6 +29,13 @@ export function useCallSignaling() { const mutedRoomId = useAtomValue(mutedCallRoomIdAtom); const setMutedRoomId = useSetAtom(mutedCallRoomIdAtom); + // Stable refs so volatile values (mutedRoomId, ring callbacks) don't force + // the listener registration effect to re-run — which would cause the + // SessionEnded and RoomState.events listeners to accumulate when muting + // or when call state changes rapidly during a sync retry cycle. + const mutedRoomIdRef = useRef(mutedRoomId); + mutedRoomIdRef.current = mutedRoomId; + useEffect(() => { const inc = new Audio(RingtoneSound); inc.loop = true; @@ -72,6 +79,16 @@ export function useCallSignaling() { [setIncomingCall] ); + // Must be declared after the callbacks above so the initial useRef(value) call + // sees their current identity. Updated on every render so the effect closure + // always calls the latest version without needing them in the dep array. + const playRingingRef = useRef(playRinging); + playRingingRef.current = playRinging; + const stopRingingRef = useRef(stopRinging); + stopRingingRef.current = stopRinging; + const playOutgoingRingingRef = useRef(playOutgoingRinging); + playOutgoingRingingRef.current = playOutgoingRinging; + useEffect(() => { if (!mx || !mx.matrixRTC) return undefined; @@ -81,7 +98,7 @@ export function useCallSignaling() { const signal = Array.from(mDirects).reduce( (acc, roomId) => { - if (acc.incoming || mutedRoomId === roomId) return acc; + if (acc.incoming || mutedRoomIdRef.current === roomId) return acc; const room = mx.getRoom(roomId); if (!room) return acc; @@ -141,11 +158,11 @@ export function useCallSignaling() { ); if (signal.incoming) { - playRinging(signal.incoming); + playRingingRef.current(signal.incoming); } else if (signal.outgoing) { - playOutgoingRinging(signal.outgoing); + playOutgoingRingingRef.current(signal.outgoing); } else { - stopRinging(); + stopRingingRef.current(); if (!signal.outgoing) outgoingStartRef.current = null; } }; @@ -155,7 +172,7 @@ export function useCallSignaling() { const handleUpdate = () => checkDMsForActiveCalls(); const handleSessionEnded = (roomId: string) => { - if (mutedRoomId === roomId) setMutedRoomId(null); + if (mutedRoomIdRef.current === roomId) setMutedRoomId(null); callPhaseRef.current[roomId] = 'IDLE'; checkDMsForActiveCalls(); }; @@ -171,9 +188,9 @@ export function useCallSignaling() { mx.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionStarted, handleUpdate); mx.matrixRTC.off(MatrixRTCSessionManagerEvents.SessionEnded, handleSessionEnded); mx.off(RoomStateEvent.Events, handleUpdate); - stopRinging(); + stopRingingRef.current(); }; - }, [mx, mDirects, playRinging, stopRinging, mutedRoomId, setMutedRoomId, playOutgoingRinging]); + }, [mx, mDirects, setMutedRoomId]); // stable: volatile deps accessed via refs above return null; } diff --git a/src/app/plugins/call/CallEmbed.ts b/src/app/plugins/call/CallEmbed.ts index 570b66f76..960f21ced 100644 --- a/src/app/plugins/call/CallEmbed.ts +++ b/src/app/plugins/call/CallEmbed.ts @@ -218,11 +218,24 @@ export class CallEmbed { this.readUpToMap[room.roomId] = roomEvent.getId()!; }); - // Attach listeners for feeding events - the underlying widget classes handle permissions for us - this.mx.on(ClientEvent.Event, this.onEvent.bind(this)); - this.mx.on(MatrixEventEvent.Decrypted, this.onEventDecrypted.bind(this)); - this.mx.on(RoomStateEvent.Events, this.onStateUpdate.bind(this)); - this.mx.on(ClientEvent.ToDeviceEvent, this.onToDeviceEvent.bind(this)); + // Attach listeners for feeding events - the underlying widget classes handle permissions for us. + // Bind once and store via disposables so the same function reference is used for removal. + // Using .bind(this) at call-site would create a new function every time, making .off() a no-op + // and causing MaxListeners warnings when the embed is recreated during sync retries. + const boundOnEvent = this.onEvent.bind(this); + const boundOnEventDecrypted = this.onEventDecrypted.bind(this); + const boundOnStateUpdate = this.onStateUpdate.bind(this); + const boundOnToDeviceEvent = this.onToDeviceEvent.bind(this); + this.mx.on(ClientEvent.Event, boundOnEvent); + this.mx.on(MatrixEventEvent.Decrypted, boundOnEventDecrypted); + this.mx.on(RoomStateEvent.Events, boundOnStateUpdate); + this.mx.on(ClientEvent.ToDeviceEvent, boundOnToDeviceEvent); + this.disposables.push(() => { + this.mx.off(ClientEvent.Event, boundOnEvent); + this.mx.off(MatrixEventEvent.Decrypted, boundOnEventDecrypted); + this.mx.off(RoomStateEvent.Events, boundOnStateUpdate); + this.mx.off(ClientEvent.ToDeviceEvent, boundOnToDeviceEvent); + }); } /** @@ -239,11 +252,7 @@ export class CallEmbed { this.container.removeChild(this.iframe); this.control.dispose(); - this.mx.off(ClientEvent.Event, this.onEvent.bind(this)); - this.mx.off(MatrixEventEvent.Decrypted, this.onEventDecrypted.bind(this)); - this.mx.off(RoomStateEvent.Events, this.onStateUpdate.bind(this)); - this.mx.off(ClientEvent.ToDeviceEvent, this.onToDeviceEvent.bind(this)); - + // Listener removal is handled by the disposables pushed in start(). // Clear internal state this.readUpToMap = {}; this.eventsToFeed = new WeakSet();