Skip to content

Commit 748794c

Browse files
authored
fix: scroll-to-bottom regression and timeline pagination fixes (#529)
### Description Fixes several timeline bugs introduced after the timeline-reset-on-navigation refactor: - Scroll-to-bottom no longer gets stuck after navigating to a room - Timeline pagination no longer uses a stale timeline chain after a sliding sync reset - URL preview requests are deduplicated so the same URL only fires one network request - Removed a redundant spidering call that was generating an extra sliding sync request per room visit Fixes # #### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings ### AI disclosure: - [x] Partially AI assisted (GitHub Copilot) `RoomTimeline.tsx`: Added a `liveTimelineLinked` guard so the initial scroll doesn't fire with stale data from the previous room, and a `pendingReadyRef` that defers `setIsReady(true)` when `processedEvents` is still empty (preventing the room opening at position 0). `useTimelineSync.ts`: A `useEffect` resets the timeline state on room change so revisited rooms aren't stuck with `liveTimelineLinked = false`. Reads `linkedTimelines` fresh after each `await` in pagination so a sliding sync reset mid-paginate doesn't cause count comparisons on a detached chain. A `continuing` flag prevents two concurrent pagination calls from racing on the lock. URL preview dedup uses a module-level `Map` to share in-flight promises for the same URL. Removed a redundant `startSpidering` subscription call in the sync setup path.
2 parents e0a666c + 7ac1935 commit 748794c

9 files changed

Lines changed: 297 additions & 44 deletions

File tree

.changeset/fix-bug-fixes.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
default: patch
3+
---
4+
5+
Fix scroll-to-bottom after room navigation, timeline pagination reliability, and URL preview deduplication.

src/app/components/url-preview/UrlPreviewCard.tsx

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,23 @@ import { ImageViewer } from '../image-viewer';
1414

1515
const linkStyles = { color: color.Success.Main };
1616

17+
// Module-level in-flight deduplication: prevents N+1 concurrent requests when a
18+
// large event batch renders many UrlPreviewCard instances for the same URL.
19+
// Scoped by MatrixClient to avoid cross-account dedup if multiple clients exist.
20+
// Inner cache keyed by URL only (not ts) — the same URL shows the same preview
21+
// regardless of which message referenced it. Promises are evicted after settling
22+
// so a later render can retry after network recovery.
23+
const previewRequestCache = new WeakMap<any, Map<string, Promise<IPreviewUrlResponse>>>();
24+
25+
const getClientCache = (mx: any): Map<string, Promise<IPreviewUrlResponse>> => {
26+
let clientCache = previewRequestCache.get(mx);
27+
if (!clientCache) {
28+
clientCache = new Map();
29+
previewRequestCache.set(mx, clientCache);
30+
}
31+
return clientCache;
32+
};
33+
1734
const openMediaInNewTab = async (url: string | undefined) => {
1835
if (!url) {
1936
console.warn('Attempted to open an empty url');
@@ -34,7 +51,13 @@ export const UrlPreviewCard = as<'div', { url: string; ts: number; mediaType?: s
3451
const [previewStatus, loadPreview] = useAsyncCallback(
3552
useCallback(() => {
3653
if (isDirect) return Promise.resolve(null);
37-
return mx.getUrlPreview(url, ts);
54+
const clientCache = getClientCache(mx);
55+
const cached = clientCache.get(url);
56+
if (cached !== undefined) return cached;
57+
const urlPreview = mx.getUrlPreview(url, ts);
58+
clientCache.set(url, urlPreview);
59+
urlPreview.finally(() => clientCache.delete(url));
60+
return urlPreview;
3861
}, [url, ts, mx, isDirect])
3962
);
4063

src/app/features/room/RoomTimeline.tsx

Lines changed: 71 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,14 @@ export function RoomTimeline({
209209
const topSpacerHeightRef = useRef(0);
210210
const mountScrollWindowRef = useRef<number>(Date.now() + 3000);
211211
const hasInitialScrolledRef = useRef(false);
212+
// Stored in a ref so eventsLength fluctuations (e.g. onLifecycle timeline reset
213+
// firing within the window) cannot cancel it via useLayoutEffect cleanup.
214+
const initialScrollTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
215+
// Set to true when the 80 ms timer fires but processedEvents is still empty
216+
// (e.g. the onLifecycle reset cleared the timeline before events refilled it).
217+
// A recovery useLayoutEffect watches for processedEvents becoming non-empty
218+
// and performs the final scroll + setIsReady when this flag is set.
219+
const pendingReadyRef = useRef(false);
212220
const currentRoomIdRef = useRef(room.roomId);
213221

214222
const [isReady, setIsReady] = useState(false);
@@ -217,6 +225,11 @@ export function RoomTimeline({
217225
hasInitialScrolledRef.current = false;
218226
mountScrollWindowRef.current = Date.now() + 3000;
219227
currentRoomIdRef.current = room.roomId;
228+
pendingReadyRef.current = false;
229+
if (initialScrollTimerRef.current !== undefined) {
230+
clearTimeout(initialScrollTimerRef.current);
231+
initialScrollTimerRef.current = undefined;
232+
}
220233
setIsReady(false);
221234
}
222235

@@ -272,18 +285,45 @@ export function RoomTimeline({
272285
!eventId &&
273286
!hasInitialScrolledRef.current &&
274287
timelineSync.eventsLength > 0 &&
288+
// Guard: only scroll once the timeline reflects the current room's live
289+
// timeline. Without this, a render with stale data from the previous room
290+
// (before the room-change reset propagates) fires the scroll at the wrong
291+
// position and marks hasInitialScrolledRef = true, preventing the correct
292+
// scroll when the right data arrives.
293+
timelineSync.liveTimelineLinked &&
275294
vListRef.current
276295
) {
277296
vListRef.current.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' });
278-
const t = setTimeout(() => {
279-
vListRef.current?.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' });
280-
setIsReady(true);
297+
// Store in a ref rather than a local so subsequent eventsLength changes
298+
// (e.g. the onLifecycle timeline reset firing within 80 ms) do NOT
299+
// cancel this timer through the useLayoutEffect cleanup.
300+
initialScrollTimerRef.current = setTimeout(() => {
301+
initialScrollTimerRef.current = undefined;
302+
if (processedEventsRef.current.length > 0) {
303+
vListRef.current?.scrollToIndex(processedEventsRef.current.length - 1, { align: 'end' });
304+
// Only mark ready once we've successfully scrolled. If processedEvents
305+
// was empty when the timer fired (e.g. the onLifecycle reset cleared the
306+
// timeline within the 80 ms window), defer setIsReady until the recovery
307+
// effect below fires once events repopulate.
308+
setIsReady(true);
309+
} else {
310+
pendingReadyRef.current = true;
311+
}
281312
}, 80);
282313
hasInitialScrolledRef.current = true;
283-
return () => clearTimeout(t);
284314
}
285-
return () => {};
286-
}, [timelineSync.eventsLength, eventId, room.roomId]);
315+
// No cleanup return — the timer must survive eventsLength fluctuations.
316+
// It is cancelled on unmount by the dedicated effect below.
317+
}, [timelineSync.eventsLength, timelineSync.liveTimelineLinked, eventId, room.roomId]);
318+
319+
// Cancel the initial-scroll timer on unmount (the useLayoutEffect above
320+
// intentionally does not cancel it when deps change).
321+
useEffect(
322+
() => () => {
323+
if (initialScrollTimerRef.current !== undefined) clearTimeout(initialScrollTimerRef.current);
324+
},
325+
[]
326+
);
287327

288328
const recalcTopSpacer = useCallback(() => {
289329
const v = vListRef.current;
@@ -357,6 +397,11 @@ export function RoomTimeline({
357397

358398
useEffect(() => {
359399
if (eventId) return;
400+
// Guard: once the timeline is visible to the user, do not override their
401+
// scroll position. Without this, a later timeline refresh (e.g. the
402+
// onLifecycle reset delivering a new linkedTimelines reference) can fire
403+
// this effect after isReady and snap the view back to the read marker.
404+
if (isReady) return;
360405
const { readUptoEventId, inLiveTimeline, scrollTo } = unreadInfo ?? {};
361406
if (readUptoEventId && inLiveTimeline && scrollTo) {
362407
const evtTimeline = getEventTimeline(room, readUptoEventId);
@@ -368,19 +413,24 @@ export function RoomTimeline({
368413
)
369414
: undefined;
370415

371-
if (absoluteIndex !== undefined && vListRef.current) {
416+
if (absoluteIndex !== undefined) {
372417
const processedIndex = getRawIndexToProcessedIndex(absoluteIndex);
373-
if (processedIndex !== undefined) {
418+
if (processedIndex !== undefined && vListRef.current) {
374419
vListRef.current.scrollToIndex(processedIndex, { align: 'start' });
375-
setUnreadInfo((prev) => (prev ? { ...prev, scrollTo: false } : prev));
376420
}
421+
// Always consume the scroll intent once the event is located in the
422+
// linked timelines, even if its processedIndex is undefined (filtered
423+
// event). Without this, each linkedTimelines reference change retries
424+
// the scroll indefinitely.
425+
setUnreadInfo((prev) => (prev ? { ...prev, scrollTo: false } : prev));
377426
}
378427
}
379428
}, [
380429
room,
381430
unreadInfo,
382431
timelineSync.timeline.linkedTimelines,
383432
eventId,
433+
isReady,
384434
getRawIndexToProcessedIndex,
385435
]);
386436

@@ -674,6 +724,18 @@ export function RoomTimeline({
674724

675725
processedEventsRef.current = processedEvents;
676726

727+
// Recovery: if the 80 ms initial-scroll timer fired while processedEvents was
728+
// empty (timeline was mid-reset), scroll to bottom and reveal the timeline once
729+
// events repopulate. Fires on every processedEvents.length change but is
730+
// guarded by pendingReadyRef so it only acts once per initial-scroll attempt.
731+
useLayoutEffect(() => {
732+
if (!pendingReadyRef.current) return;
733+
if (processedEvents.length === 0) return;
734+
pendingReadyRef.current = false;
735+
vListRef.current?.scrollToIndex(processedEvents.length - 1, { align: 'end' });
736+
setIsReady(true);
737+
}, [processedEvents.length]);
738+
677739
useEffect(() => {
678740
if (!onEditLastMessageRef) return;
679741
const ref = onEditLastMessageRef;

src/app/hooks/timeline/useTimelineSync.test.tsx

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,18 @@ function createTimeline(events: unknown[] = [{}]): FakeTimeline {
4040
};
4141
}
4242

43-
function createRoom(events: unknown[] = [{}]): {
43+
function createRoom(
44+
roomId = '!room:test',
45+
events: unknown[] = [{}]
46+
): {
4447
room: FakeRoom;
4548
timelineSet: FakeTimelineSet;
4649
events: unknown[];
4750
} {
48-
const timeline = createTimeline(events);
51+
const timeline = {
52+
...createTimeline(events),
53+
getRoomId: () => roomId,
54+
};
4955
const timelineSet = new EventEmitter() as FakeTimelineSet;
5056
timelineSet.getLiveTimeline = () => timeline;
5157
timelineSet.getTimelineForEvent = () => undefined;
@@ -55,7 +61,7 @@ function createRoom(events: unknown[] = [{}]): {
5561
on: roomEmitter.on.bind(roomEmitter),
5662
removeListener: roomEmitter.removeListener.bind(roomEmitter),
5763
emit: roomEmitter.emit.bind(roomEmitter),
58-
roomId: '!room:test',
64+
roomId,
5965
getUnfilteredTimelineSet: () => timelineSet as never,
6066
getEventReadUpTo: () => null,
6167
getThread: () => null,
@@ -125,4 +131,110 @@ describe('useTimelineSync', () => {
125131

126132
expect(scrollToBottom).toHaveBeenCalledWith('instant');
127133
});
134+
135+
it('resets timeline state when room.roomId changes and eventId is not set', async () => {
136+
const roomOne = createRoom('!room:one');
137+
const roomTwo = createRoom('!room:two');
138+
const scrollToBottom = vi.fn();
139+
140+
const { result, rerender } = renderHook(
141+
({ room, eventId }) =>
142+
useTimelineSync({
143+
room,
144+
mx: { getUserId: () => '@alice:test' } as never,
145+
eventId,
146+
isAtBottom: false,
147+
isAtBottomRef: { current: false },
148+
scrollToBottom,
149+
unreadInfo: undefined,
150+
setUnreadInfo: vi.fn(),
151+
hideReadsRef: { current: false },
152+
readUptoEventIdRef: { current: undefined },
153+
}),
154+
{
155+
initialProps: {
156+
room: roomOne.room as Room,
157+
eventId: undefined as string | undefined,
158+
},
159+
}
160+
);
161+
162+
expect(result.current.timeline.linkedTimelines[0]).toBe(roomOne.timelineSet.getLiveTimeline());
163+
164+
await act(async () => {
165+
rerender({ room: roomTwo.room as Room, eventId: undefined });
166+
await Promise.resolve();
167+
});
168+
169+
expect(result.current.timeline.linkedTimelines[0]).toBe(roomTwo.timelineSet.getLiveTimeline());
170+
});
171+
172+
it('does not reset timeline when eventId is set during a room change', async () => {
173+
const roomOne = createRoom('!room:one');
174+
const roomTwo = createRoom('!room:two');
175+
const scrollToBottom = vi.fn();
176+
177+
const { result, rerender } = renderHook(
178+
({ room, eventId }) =>
179+
useTimelineSync({
180+
room,
181+
mx: { getUserId: () => '@alice:test' } as never,
182+
eventId,
183+
isAtBottom: false,
184+
isAtBottomRef: { current: false },
185+
scrollToBottom,
186+
unreadInfo: undefined,
187+
setUnreadInfo: vi.fn(),
188+
hideReadsRef: { current: false },
189+
readUptoEventIdRef: { current: undefined },
190+
}),
191+
{
192+
initialProps: {
193+
room: roomOne.room as Room,
194+
eventId: undefined as string | undefined,
195+
},
196+
}
197+
);
198+
199+
await act(async () => {
200+
rerender({ room: roomTwo.room as Room, eventId: '$event:one' });
201+
await Promise.resolve();
202+
});
203+
204+
expect(result.current.timeline.linkedTimelines[0]).toBe(roomOne.timelineSet.getLiveTimeline());
205+
});
206+
207+
it('does not reset timeline when the roomId stays the same', async () => {
208+
const roomOne = createRoom('!room:one');
209+
const sameRoomId = createRoom('!room:one');
210+
const scrollToBottom = vi.fn();
211+
212+
const { result, rerender } = renderHook(
213+
({ room }) =>
214+
useTimelineSync({
215+
room,
216+
mx: { getUserId: () => '@alice:test' } as never,
217+
eventId: undefined,
218+
isAtBottom: false,
219+
isAtBottomRef: { current: false },
220+
scrollToBottom,
221+
unreadInfo: undefined,
222+
setUnreadInfo: vi.fn(),
223+
hideReadsRef: { current: false },
224+
readUptoEventIdRef: { current: undefined },
225+
}),
226+
{
227+
initialProps: {
228+
room: roomOne.room as Room,
229+
},
230+
}
231+
);
232+
233+
await act(async () => {
234+
rerender({ room: sameRoomId.room as Room });
235+
await Promise.resolve();
236+
});
237+
238+
expect(result.current.timeline.linkedTimelines[0]).toBe(roomOne.timelineSet.getLiveTimeline());
239+
});
128240
});

0 commit comments

Comments
 (0)