diff --git a/.changeset/fix-bug-fixes.md b/.changeset/fix-bug-fixes.md new file mode 100644 index 000000000..aea8ed44e --- /dev/null +++ b/.changeset/fix-bug-fixes.md @@ -0,0 +1,5 @@ +--- +default: patch +--- + +Fix timeline pagination lock bug, deduplicate concurrent URL preview requests, and remove redundant sliding sync spidering call causing N+1 requests. diff --git a/src/app/components/url-preview/UrlPreviewCard.tsx b/src/app/components/url-preview/UrlPreviewCard.tsx index 62106a00a..edc3f0f9d 100644 --- a/src/app/components/url-preview/UrlPreviewCard.tsx +++ b/src/app/components/url-preview/UrlPreviewCard.tsx @@ -14,6 +14,13 @@ import { ImageViewer } from '../image-viewer'; const linkStyles = { color: color.Success.Main }; +// Module-level in-flight deduplication: prevents N+1 concurrent requests when a +// large event batch renders many UrlPreviewCard instances for the same URL. +// Keyed by URL only (not ts) — the same URL shows the same preview regardless +// of which message referenced it. Rejected promises are evicted so a later +// render can retry after network recovery. +const previewRequestCache = new Map>(); + const openMediaInNewTab = async (url: string | undefined) => { if (!url) { console.warn('Attempted to open an empty url'); @@ -34,7 +41,12 @@ export const UrlPreviewCard = as<'div', { url: string; ts: number; mediaType?: s const [previewStatus, loadPreview] = useAsyncCallback( useCallback(() => { if (isDirect) return Promise.resolve(null); - return mx.getUrlPreview(url, ts); + const cached = previewRequestCache.get(url); + if (cached !== undefined) return cached; + const urlPreview = mx.getUrlPreview(url, ts); + previewRequestCache.set(url, urlPreview); + urlPreview.catch(() => previewRequestCache.delete(url)); + return urlPreview; }, [url, ts, mx, isDirect]) ); diff --git a/src/app/features/room/RoomTimeline.tsx b/src/app/features/room/RoomTimeline.tsx index dd655deec..d4aaebbd9 100644 --- a/src/app/features/room/RoomTimeline.tsx +++ b/src/app/features/room/RoomTimeline.tsx @@ -207,6 +207,9 @@ export function RoomTimeline({ const topSpacerHeightRef = useRef(0); const mountScrollWindowRef = useRef(Date.now() + 3000); const hasInitialScrolledRef = useRef(false); + // Stored in a ref so eventsLength fluctuations (e.g. onLifecycle timeline reset + // firing within the window) cannot cancel it via useLayoutEffect cleanup. + const initialScrollTimerRef = useRef | undefined>(undefined); const currentRoomIdRef = useRef(room.roomId); const [isReady, setIsReady] = useState(false); @@ -273,16 +276,31 @@ export function RoomTimeline({ vListRef.current ) { vListRef.current.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' }); - const t = setTimeout(() => { - vListRef.current?.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' }); + // Store in a ref rather than a local so subsequent eventsLength changes + // (e.g. the onLifecycle timeline reset firing within 80 ms) do NOT + // cancel this timer through the useLayoutEffect cleanup. + initialScrollTimerRef.current = setTimeout(() => { + initialScrollTimerRef.current = undefined; + if (processedEventsRef.current.length > 0) { + vListRef.current?.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' }); + } setIsReady(true); }, 80); hasInitialScrolledRef.current = true; - return () => clearTimeout(t); } - return () => {}; + // No cleanup return — the timer must survive eventsLength fluctuations. + // It is cancelled on unmount by the dedicated effect below. }, [timelineSync.eventsLength, eventId, room.roomId]); + // Cancel the initial-scroll timer on unmount (the useLayoutEffect above + // intentionally does not cancel it when deps change). + useEffect( + () => () => { + if (initialScrollTimerRef.current !== undefined) clearTimeout(initialScrollTimerRef.current); + }, + [] + ); + const recalcTopSpacer = useCallback(() => { const v = vListRef.current; if (!v) return; @@ -355,6 +373,11 @@ export function RoomTimeline({ useEffect(() => { if (eventId) return; + // Guard: once the timeline is visible to the user, do not override their + // scroll position. Without this, a later timeline refresh (e.g. the + // onLifecycle reset delivering a new linkedTimelines reference) can fire + // this effect after isReady and snap the view back to the read marker. + if (isReady) return; const { readUptoEventId, inLiveTimeline, scrollTo } = unreadInfo ?? {}; if (readUptoEventId && inLiveTimeline && scrollTo) { const evtTimeline = getEventTimeline(room, readUptoEventId); @@ -366,12 +389,16 @@ export function RoomTimeline({ ) : undefined; - if (absoluteIndex !== undefined && vListRef.current) { + if (absoluteIndex !== undefined) { const processedIndex = getRawIndexToProcessedIndex(absoluteIndex); - if (processedIndex !== undefined) { + if (processedIndex !== undefined && vListRef.current) { vListRef.current.scrollToIndex(processedIndex, { align: 'start' }); - setUnreadInfo((prev) => (prev ? { ...prev, scrollTo: false } : prev)); } + // Always consume the scroll intent once the event is located in the + // linked timelines, even if its processedIndex is undefined (filtered + // event). Without this, each linkedTimelines reference change retries + // the scroll indefinitely. + setUnreadInfo((prev) => (prev ? { ...prev, scrollTo: false } : prev)); } } }, [ @@ -379,6 +406,7 @@ export function RoomTimeline({ unreadInfo, timelineSync.timeline.linkedTimelines, eventId, + isReady, getRawIndexToProcessedIndex, ]); diff --git a/src/app/hooks/timeline/useTimelineSync.ts b/src/app/hooks/timeline/useTimelineSync.ts index 395d6fc46..5c99bf6c2 100644 --- a/src/app/hooks/timeline/useTimelineSync.ts +++ b/src/app/hooks/timeline/useTimelineSync.ts @@ -147,6 +147,13 @@ const useTimelinePagination = ( (backwards ? setBackwardStatus : setForwardStatus)('loading'); } + // `continuing` tracks whether we hand the fetchingRef lock to a recursive + // continuation call below. The finally block must NOT reset the lock if + // the recursive call has already claimed it, otherwise there is a brief + // window where fetchingRef is false while the recursive paginate is in + // flight, allowing a third overlapping call to start on sparse pages. + let continuing = false; + try { const countBefore = getTimelinesEventsCount(lTimelines); @@ -184,13 +191,24 @@ const useTimelinePagination = ( Direction.Backward ) === 'string'; if (stillHasToken) { + // Release lock so inner paginate can claim it, then mark continuing + // so the finally block below does NOT reset it after inner claims. fetchingRef.current[directionKey] = false; + continuing = true; paginate(backwards); + // At this point the inner paginate has synchronously set + // fetchingRef.current[directionKey] = true before hitting its own + // await. The finally below will skip the reset. } } } } finally { - fetchingRef.current[directionKey] = false; + // Only release the lock if we did NOT hand it to a recursive continuation. + // If `continuing` is true the recursive call owns the lock and will release + // it in its own finally block. + if (!continuing) { + fetchingRef.current[directionKey] = false; + } } }; }, [mx, alive, setTimeline, limit, setBackwardStatus, setForwardStatus]); diff --git a/src/app/hooks/useSlidingSyncActiveRoom.ts b/src/app/hooks/useSlidingSyncActiveRoom.ts index 8468f4024..c86914d56 100644 --- a/src/app/hooks/useSlidingSyncActiveRoom.ts +++ b/src/app/hooks/useSlidingSyncActiveRoom.ts @@ -24,16 +24,7 @@ export const useSlidingSyncActiveRoom = (): void => { const manager = getSlidingSyncManager(mx); if (!manager) return undefined; - // Wait for the room to be initialized from list sync before subscribing - // with the full timeline limit. This prevents timeline ordering issues where - // the room might be receiving events from list expansion while we're also - // trying to load a large timeline, causing events to be added out of order. - const timeoutId = setTimeout(() => { - manager.subscribeToRoom(roomId); - }, 100); - - return () => { - clearTimeout(timeoutId); - }; + manager.subscribeToRoom(roomId); + return undefined; }, [mx, roomId]); }; diff --git a/src/app/utils/timeline.ts b/src/app/utils/timeline.ts index 4ed92ab16..0934e5b02 100644 --- a/src/app/utils/timeline.ts +++ b/src/app/utils/timeline.ts @@ -22,18 +22,14 @@ export const getFirstLinkedTimeline = ( return current; }; -const collectTimelines = ( - tl: EventTimeline | null, - dir: Direction, - acc: EventTimeline[] = [] -): EventTimeline[] => { - if (!tl) return acc; - return collectTimelines(tl.getNeighbouringTimeline(dir), dir, [...acc, tl]); -}; - export const getLinkedTimelines = (timeline: EventTimeline): EventTimeline[] => { - const firstTimeline = getFirstLinkedTimeline(timeline, Direction.Backward); - return collectTimelines(firstTimeline, Direction.Forward); + const result: EventTimeline[] = []; + let current: EventTimeline | null = getFirstLinkedTimeline(timeline, Direction.Backward); + while (current) { + result.push(current); + current = current.getNeighbouringTimeline(Direction.Forward); + } + return result; }; export const timelineToEventsCount = (t: EventTimeline) => { diff --git a/src/client/initMatrix.ts b/src/client/initMatrix.ts index 18809c5d2..093093ba2 100644 --- a/src/client/initMatrix.ts +++ b/src/client/initMatrix.ts @@ -563,9 +563,6 @@ export const startClient = async (mx: MatrixClient, config?: StartClientConfig): pollTimeoutMs: slidingConfig?.pollTimeoutMs ?? SLIDING_SYNC_POLL_TIMEOUT_MS, }); manager.attach(); - // Begin background spidering so all rooms are eventually indexed. - // Not awaited — this runs incrementally in the background. - manager.startSpidering(100, 50); slidingSyncByClient.set(mx, manager); syncTransportByClient.set(mx, { transport: 'sliding', diff --git a/src/client/slidingSync.ts b/src/client/slidingSync.ts index 84622b0e2..058b87cf1 100644 --- a/src/client/slidingSync.ts +++ b/src/client/slidingSync.ts @@ -8,6 +8,7 @@ import { MSC3575List, MSC3575RoomData, MSC3575RoomSubscription, + MSC3575SlidingSyncResponse, MSC3575_WILDCARD, RoomMemberEvent, SlidingSync, @@ -350,6 +351,27 @@ export class SlidingSyncManager { return; } + // Before room data is processed, reset live timelines for active rooms that + // are receiving a full refresh (initial: true) or a post-gap update + // (limited: true). The SDK deliberately does not call resetLiveTimeline() for + // sliding sync, so events from previous visits accumulate in the live + // timeline alongside new events. Resetting here — before the SDK's + // onRoomData listener runs — ensures the fresh batch lands on a clean + // timeline with a correct backward pagination token. + if (state === SlidingSyncState.RequestFinished && resp && !err) { + const rooms = (resp as MSC3575SlidingSyncResponse).rooms ?? {}; + Object.entries(rooms) + .filter(([, roomData]) => roomData.initial || roomData.limited) + .filter(([roomId]) => this.activeRoomSubscriptions.has(roomId)) + .forEach(([roomId]) => { + const room = this.mx.getRoom(roomId); + if (!room) return; + const timelineSet = room.getUnfilteredTimelineSet(); + if (timelineSet.getLiveTimeline().getEvents().length === 0) return; + timelineSet.resetLiveTimeline(); + }); + } + if (err || !resp || state !== SlidingSyncState.Complete) return; // Track what changed in this sync cycle