From 4a03b1228738f5ee229a5308fb172a12ef3e8a80 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Thu, 24 Feb 2022 16:48:39 +0000 Subject: [PATCH] client/server: set 'initial' flag on a per-room basis and use it when rendering Previously the 'initial' flag was set on the entire response which was pointless as the client can detect this by the presence/absence of `?pos=`. Instead, move the `initial` flag to be present on the Room object, so clients know when to replace or update their databases. Use this to fix a bug where the timeline would show incorrectly when clicking on rooms due to appending to the timeline when the room subscription data was collected. --- client/index.html | 4 +- client/index.js | 131 +++++++++++++++++++------------------ client/sync.js | 15 ++--- sync3/handler/connstate.go | 2 +- sync3/response.go | 5 +- sync3/room.go | 1 + timeline_test.go | 36 ++++++---- v3_test.go | 9 +++ 8 files changed, 110 insertions(+), 93 deletions(-) diff --git a/client/index.html b/client/index.html index 968cc68..0af7015 100644 --- a/client/index.html +++ b/client/index.html @@ -21,9 +21,7 @@ -
- -
+
diff --git a/client/index.js b/client/index.js index c1d3301..c5b1ed7 100644 --- a/client/index.js +++ b/client/index.js @@ -16,6 +16,9 @@ import * as matrix from "./matrix.js"; let syncv2ServerUrl; // will be populated with the URL of the CS API e.g 'https://matrix-client.matrix.org' let slidingSync; let syncConnection = new SlidingSyncConnection(); + +// The lists present on the UI. +// You can add/remove these at will to experiment with sliding sync filters. let activeLists = [ new SlidingList("Direct Messages", { is_dm: true, @@ -34,59 +37,72 @@ let rooms = { window.rooms = rooms; window.activeLists = activeLists; -const accumulateRoomData = (r, isUpdate) => { - let room = r; - if (isUpdate) { - // use what we already have, if any - let existingRoom = rooms.roomIdToRoom[r.room_id]; - if (existingRoom) { - if (r.name) { - existingRoom.name = r.name; - } - if (r.highlight_count !== undefined) { - existingRoom.highlight_count = r.highlight_count; - } - if (r.notification_count !== undefined) { - existingRoom.notification_count = r.notification_count; - } - if (r.timeline) { - r.timeline.forEach((e) => { - existingRoom.timeline.push(e); - }); - } - room = existingRoom; +/** + * Set top-level properties on the response which corresponds to state fields the UI requires. + * The normal response returns whole state events in an array which isn't useful when rendering as we + * often just need a single key within the content, so pre-emptively find the fields we want and set them. + * Modifies the input object. + * @param {object} r The room response JSON + */ +const setRoomStateFields = (r) => { + if (!r.required_state) { + return; + } + for (let i = 0; i < r.required_state.length; i++) { + const ev = r.required_state[i]; + switch (ev.type) { + case "m.room.avatar": + r.avatar = ev.content.url; + break; + case "m.room.topic": + r.topic = ev.content.topic; + break; + case "m.room.tombstone": + r.obsolete = ev.content.body || "m.room.tombstone"; + break; } } - // pull out avatar and topic if it exists - let avatar; - let topic; - let obsolete; - if (r.required_state) { - for (let i = 0; i < r.required_state.length; i++) { - const ev = r.required_state[i]; - switch (ev.type) { - case "m.room.avatar": - avatar = ev.content.url; - break; - case "m.room.topic": - topic = ev.content.topic; - break; - case "m.room.tombstone": - obsolete = ev.content.body || "m.room.tombstone"; - break; - } +}; + +/** + * Accumulate room data for this room. This is done by inspecting the 'initial' flag on the response. + * If it is set, the entire room is replaced with this data. If it is false, the room data is + * merged together with whatever is there in the store. + * @param {object} r The room response JSON + */ +const accumulateRoomData = (r) => { + setRoomStateFields(r); + if (r.initial) { + rooms.roomIdToRoom[r.room_id] = r; + return; + } + // use what we already have, if any + let existingRoom = rooms.roomIdToRoom[r.room_id]; + if (existingRoom) { + if (r.name) { + existingRoom.name = r.name; } + if (r.highlight_count !== undefined) { + existingRoom.highlight_count = r.highlight_count; + } + if (r.notification_count !== undefined) { + existingRoom.notification_count = r.notification_count; + } + if (r.timeline) { + r.timeline.forEach((e) => { + existingRoom.timeline.push(e); + }); + } + } else { + // we don't know this room but apparently we should. Whine about it and use what we have. + console.error( + "room initial flag not set but we have no memory of this room", + r + ); + existingRoom = r; } - if (avatar !== undefined) { - room.avatar = avatar; - } - if (topic !== undefined) { - room.topic = topic; - } - if (obsolete !== undefined) { - room.obsolete = obsolete; - } - rooms.roomIdToRoom[room.room_id] = room; + + rooms.roomIdToRoom[existingRoom.room_id] = existingRoom; }; let debounceTimeoutId; @@ -165,11 +181,6 @@ const intersectionObserver = new IntersectionObserver( const renderMessage = (container, ev) => { const eventIdKey = "msg" + ev.event_id; - // try to find the element. If it exists then don't re-render. - const existing = document.getElementById(eventIdKey); - if (existing) { - return; - } const msgCell = render.renderEvent(eventIdKey, ev); container.appendChild(msgCell); }; @@ -350,19 +361,14 @@ const doSyncLoop = async (accessToken) => { } }); - slidingSync.addRoomDataListener((roomId, roomData, isIncremental) => { - accumulateRoomData( - roomData, - isIncremental - ? isIncremental - : rooms.roomIdToRoom[roomId] !== undefined - ); + slidingSync.addRoomDataListener((roomId, roomData) => { + accumulateRoomData(roomData); // render the right-hand side section with the room timeline if we are viewing it. if (roomId !== slidingSync.roomSubscription) { return; } let room = rooms.roomIdToRoom[slidingSync.roomSubscription]; - renderRoomTimeline(room, false); + renderRoomTimeline(room, roomData.initial); }); // begin the sliding sync loop @@ -450,6 +456,7 @@ window.addEventListener("load", async (event) => { "Error sending message: " + err; } ev.target.removeAttribute("disabled"); + ev.target.focus(); } }); }); diff --git a/client/sync.js b/client/sync.js index 9a5ec34..6ae078d 100644 --- a/client/sync.js +++ b/client/sync.js @@ -20,6 +20,7 @@ const REQUIRED_STATE_EVENTS_IN_LIST = [ ]; const REQUIRED_STATE_EVENTS_IN_ROOM = [ ["m.room.avatar", ""], // need the avatar to display next to the room name + ["m.room.tombstone", ""], // need to know if this room has been replaced ["m.room.topic", ""], // need the topic to display on the right-hand-side ]; @@ -178,11 +179,10 @@ export class SlidingSync { * Invoke all attached room data listeners. * @param {string} roomId The room which received some data. * @param {object} roomData The raw sliding sync response JSON. - * @param {boolean} isIncremental True if the roomData is a delta. False if this is the complete snapshot. */ - _invokeRoomDataListeners(roomId, roomData, isIncremental) { + _invokeRoomDataListeners(roomId, roomData) { this.roomDataCallbacks.forEach((callback) => { - callback(roomId, roomData, isIncremental); + callback(roomId, roomData); }); } @@ -278,7 +278,6 @@ export class SlidingSync { ); } catch (err) { if (err.name !== "AbortError") { - // XXX: request failed this._invokeLifecycleListeners( LifecycleSyncRequestFinished, null, @@ -373,13 +372,7 @@ export class SlidingSync { op.room.room_id, ";" ); - // TODO: move - // XXX: room data, room ID - this._invokeRoomDataListeners( - op.room.room_id, - op.room, - true - ); + this._invokeRoomDataListeners(op.room.room_id, op.room); } else if (op.op === "SYNC") { let syncRooms = []; const startIndex = op.range[0]; diff --git a/sync3/handler/connstate.go b/sync3/handler/connstate.go index 50d3132..82af6e6 100644 --- a/sync3/handler/connstate.go +++ b/sync3/handler/connstate.go @@ -161,7 +161,6 @@ func (s *ConnState) onIncomingRequest(ctx context.Context, req *sync3.Request) ( response := &sync3.Response{ RoomSubscriptions: s.updateRoomSubscriptions(int(sync3.DefaultTimelineLimit), newSubs, newUnsubs), Extensions: s.extensionsHandler.Handle(ex), - Initial: prevReq == nil, } responseOperations := []sync3.ResponseOp{} // empty not nil slice @@ -513,6 +512,7 @@ func (s *ConnState) getInitialRoomData(timelineLimit int, roomIDs ...string) []s HighlightCount: int64(userRoomData.HighlightCount), Timeline: userRoomData.Timeline, RequiredState: s.globalCache.LoadRoomState(roomID, s.loadPosition, s.muxedReq.GetRequiredState(0, roomID)), + Initial: true, } } return rooms diff --git a/sync3/response.go b/sync3/response.go index b74b0aa..bea1500 100644 --- a/sync3/response.go +++ b/sync3/response.go @@ -17,8 +17,7 @@ const ( ) type Response struct { - Ops []ResponseOp `json:"ops"` - Initial bool `json:"initial,omitempty"` + Ops []ResponseOp `json:"ops"` RoomSubscriptions map[string]Room `json:"room_subscriptions"` Counts []int `json:"counts"` @@ -44,7 +43,6 @@ func (r *Response) UnmarshalJSON(b []byte) error { Extensions extensions.Response `json:"extensions"` Pos string `json:"pos"` - Initial bool `json:"initial"` Session string `json:"session_id,omitempty"` }{} if err := json.Unmarshal(b, &temporary); err != nil { @@ -53,7 +51,6 @@ func (r *Response) UnmarshalJSON(b []byte) error { r.RoomSubscriptions = temporary.RoomSubscriptions r.Counts = temporary.Counts r.Pos = temporary.Pos - r.Initial = temporary.Initial r.Session = temporary.Session r.Extensions = temporary.Extensions diff --git a/sync3/room.go b/sync3/room.go index 918ec38..76151d7 100644 --- a/sync3/room.go +++ b/sync3/room.go @@ -13,6 +13,7 @@ type Room struct { Timeline []json.RawMessage `json:"timeline,omitempty"` NotificationCount int64 `json:"notification_count"` HighlightCount int64 `json:"highlight_count"` + Initial bool `json:"initial,omitempty"` } type RoomConnMetadata struct { diff --git a/timeline_test.go b/timeline_test.go index 2f34bbd..3a09e4a 100644 --- a/timeline_test.go +++ b/timeline_test.go @@ -248,11 +248,12 @@ func TestInitialFlag(t *testing.T) { defer v3.close() alice := "@TestInitialFlag_alice:localhost" aliceToken := "ALICE_BEARER_TOKEN_TestInitialFlag" + roomID := "!a:localhost" v2.addAccount(alice, aliceToken) v2.queueResponse(alice, sync2.SyncResponse{ Rooms: sync2.SyncRoomsResponse{ Join: v2JoinTimeline(roomEvents{ - roomID: "!a:localhost", + roomID: roomID, state: createRoomState(t, alice, time.Now()), }), }, @@ -265,12 +266,26 @@ func TestInitialFlag(t *testing.T) { TimelineLimit: 10, }}, }) - MatchResponse(t, res, func(res *sync3.Response) error { - if !res.Initial { - return fmt.Errorf("initial flag was not set") - } - return nil + MatchResponse(t, res, MatchV3Ops( + MatchV3SyncOpWithMatchers(MatchRoomRange( + []roomMatcher{MatchRoomInitial(true)}, + )), + )) + // send an update + v2.queueResponse(alice, sync2.SyncResponse{ + Rooms: sync2.SyncRoomsResponse{ + Join: v2JoinTimeline( + roomEvents{ + roomID: roomID, + events: []json.RawMessage{ + testutils.NewEvent(t, "m.room.message", alice, map[string]interface{}{}, time.Now()), + }, + }, + ), + }, }) + v2.waitUntilEmpty(t, alice) + res = v3.mustDoV3RequestWithPos(t, aliceToken, res.Pos, sync3.Request{ Lists: []sync3.RequestList{{ Ranges: sync3.SliceRanges{ @@ -278,12 +293,9 @@ func TestInitialFlag(t *testing.T) { }, }}, }) - MatchResponse(t, res, func(res *sync3.Response) error { - if res.Initial { - return fmt.Errorf("initial flag was set") - } - return nil - }) + MatchResponse(t, res, MatchV3Ops( + MatchV3UpdateOp(0, 0, roomID, MatchRoomInitial(false)), + )) } // Regression test for https://github.com/matrix-org/sliding-sync/commit/39d6e99f967e55b609f8ef8b4271c04ebb053d37 diff --git a/v3_test.go b/v3_test.go index 60f734e..a3a66a0 100644 --- a/v3_test.go +++ b/v3_test.go @@ -361,6 +361,15 @@ func MatchRoomNotificationCount(count int64) roomMatcher { } } +func MatchRoomInitial(initial bool) roomMatcher { + return func(r sync3.Room) error { + if r.Initial != initial { + return fmt.Errorf("MatchRoomInitial: got %v want %v", r.Initial, initial) + } + return nil + } +} + type roomEvents struct { roomID string name string