From 1635ac9971e05b822cd6700677b8ed95e6c8017e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 10 Aug 2022 12:32:28 +0100 Subject: [PATCH 1/3] sliding sync bugfix: ensure history is treated as history and not live events with tests --- spec/integ/sliding-sync-sdk.spec.ts | 31 +++++++++++++++++++++ src/sliding-sync-sdk.ts | 42 ++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/spec/integ/sliding-sync-sdk.spec.ts b/spec/integ/sliding-sync-sdk.spec.ts index 12ff0ae2e08..34d84bddaed 100644 --- a/spec/integ/sliding-sync-sdk.spec.ts +++ b/spec/integ/sliding-sync-sdk.spec.ts @@ -28,6 +28,7 @@ import { import { SlidingSyncSdk } from "../../src/sliding-sync-sdk"; import { SyncState } from "../../src/sync"; import { IStoredClientOpts } from "../../src/client"; +import { logger } from "../../src/logger"; describe("SlidingSyncSdk", () => { let client: MatrixClient = null; @@ -372,6 +373,36 @@ describe("SlidingSyncSdk", () => { gotRoom.getUnreadNotificationCount(NotificationCountType.Total), ).toEqual(1); }); + + // Regression test for a bug which caused the timeline entries to be out-of-order + // when the same room appears twice with different timeline limits. E.g appears in + // the list with timeline_limit:1 then appears again as a room subscription with + // timeline_limit:50 + it("can return history with a larger timeline_limit", async () => { + const timeline = data[roomA].timeline; + const oldTimeline = [ + mkOwnEvent(EventType.RoomMessage, { body: "old event A" }), + mkOwnEvent(EventType.RoomMessage, { body: "old event B" }), + mkOwnEvent(EventType.RoomMessage, { body: "old event C" }), + ...timeline, + ]; + mockSlidingSync.emit(SlidingSyncEvent.RoomData, roomA, { + timeline: oldTimeline, + required_state: [], + name: data[roomA].name, + initial: true, // e.g requested via room subscription + }); + const gotRoom = client.getRoom(roomA); + expect(gotRoom).toBeDefined(); + + logger.log("want:", oldTimeline.map((e) => (e.type + " : " + (e.content || {}).body))); + logger.log("got:", gotRoom.getLiveTimeline().getEvents().map( + (e) => (e.getType() + " : " + e.getContent().body)), + ); + + // we expect the timeline now to be oldTimeline (so the old events are in fact old) + assertTimelineEvents(gotRoom.getLiveTimeline().getEvents(), oldTimeline); + }); }); }); }); diff --git a/src/sliding-sync-sdk.ts b/src/sliding-sync-sdk.ts index 39fafec9056..a32a86e1b38 100644 --- a/src/sliding-sync-sdk.ts +++ b/src/sliding-sync-sdk.ts @@ -406,9 +406,49 @@ export class SlidingSyncSdk { // this helps large account to speed up faster // room::decryptCriticalEvent is in charge of decrypting all the events // required for a client to function properly - const timelineEvents = mapEvents(this.client, room.roomId, roomData.timeline, false); + let timelineEvents = mapEvents(this.client, room.roomId, roomData.timeline, false); const ephemeralEvents = []; // TODO this.mapSyncEventsFormat(joinObj.ephemeral); + // TODO: handle threaded / beacon events + + if (roomData.initial) { + // we should not know about any of these timeline entries if this is a genuinely new room. + // If we do, then we've effectively done scrollback (e.g requesting timeline_limit: 1 for + // this room, then timeline_limit: 50). + const knownEvents: Record = {}; + room.getLiveTimeline().getEvents().forEach((e) => { + knownEvents[e.getId()] = true; + }); + // all unknown events BEFORE a known event must be scrollback e.g: + // D E <-- what we know + // A B C D E F <-- what we just received + // means: + // A B C <-- scrollback + // D E <-- dupes + // F <-- new event + // We bucket events based on if we have seen a known event yet. + const oldEvents: MatrixEvent[] = []; + const newEvents: MatrixEvent[] = []; + let seenKnownEvent = false; + for (let i = timelineEvents.length-1; i >= 0; i--) { + const recvEvent = timelineEvents[i]; + if (knownEvents[recvEvent.getId()]) { + seenKnownEvent = true; + continue; // don't include this event, it's a dupe + } + if (seenKnownEvent) { + // old -> new + oldEvents.push(recvEvent); + } else { + // old -> new + newEvents.unshift(recvEvent); + } + } + timelineEvents = newEvents; + // old events are scrollback, insert them now + room.addEventsToTimeline(oldEvents, true, room.getLiveTimeline()); + } + const encrypted = this.client.isRoomEncrypted(room.roomId); // we do this first so it's correct when any of the events fire if (roomData.notification_count != null) { From edcef9364ce6a003f1973ebeab44bb288bc80c59 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 10 Aug 2022 12:43:47 +0100 Subject: [PATCH 2/3] Only add events if there are some; set the pagination token for faster scrollback --- src/sliding-sync-sdk.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sliding-sync-sdk.ts b/src/sliding-sync-sdk.ts index a32a86e1b38..0b8e860e918 100644 --- a/src/sliding-sync-sdk.ts +++ b/src/sliding-sync-sdk.ts @@ -445,8 +445,10 @@ export class SlidingSyncSdk { } } timelineEvents = newEvents; - // old events are scrollback, insert them now - room.addEventsToTimeline(oldEvents, true, room.getLiveTimeline()); + if (oldEvents.length > 0) { + // old events are scrollback, insert them now + room.addEventsToTimeline(oldEvents, true, room.getLiveTimeline(), roomData.prev_batch); + } } const encrypted = this.client.isRoomEncrypted(room.roomId); From 2728d7477138a4ef1a559b0026e41c387d71fd6e Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Wed, 10 Aug 2022 19:53:36 +0100 Subject: [PATCH 3/3] Review comments --- src/sliding-sync-sdk.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sliding-sync-sdk.ts b/src/sliding-sync-sdk.ts index 0b8e860e918..74a03b99b45 100644 --- a/src/sliding-sync-sdk.ts +++ b/src/sliding-sync-sdk.ts @@ -415,9 +415,9 @@ export class SlidingSyncSdk { // we should not know about any of these timeline entries if this is a genuinely new room. // If we do, then we've effectively done scrollback (e.g requesting timeline_limit: 1 for // this room, then timeline_limit: 50). - const knownEvents: Record = {}; + const knownEvents = new Set(); room.getLiveTimeline().getEvents().forEach((e) => { - knownEvents[e.getId()] = true; + knownEvents.add(e.getId()); }); // all unknown events BEFORE a known event must be scrollback e.g: // D E <-- what we know @@ -432,7 +432,7 @@ export class SlidingSyncSdk { let seenKnownEvent = false; for (let i = timelineEvents.length-1; i >= 0; i--) { const recvEvent = timelineEvents[i]; - if (knownEvents[recvEvent.getId()]) { + if (knownEvents.has(recvEvent.getId())) { seenKnownEvent = true; continue; // don't include this event, it's a dupe }