From 482d756bd0d56d194eea2e9a0ea3cc1531316997 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Tue, 1 Mar 2022 08:33:07 +0000 Subject: [PATCH] Ensure EventListSummary key does not change during backpagination (#7915) --- src/components/structures/MessagePanel.tsx | 48 +++++++----- .../structures/MessagePanel-test.js | 76 +++++++++++++++++++ 2 files changed, 105 insertions(+), 19 deletions(-) diff --git a/src/components/structures/MessagePanel.tsx b/src/components/structures/MessagePanel.tsx index 7407ff220f..98057e32cd 100644 --- a/src/components/structures/MessagePanel.tsx +++ b/src/components/structures/MessagePanel.tsx @@ -254,6 +254,9 @@ export default class MessagePanel extends React.Component { private readonly showTypingNotificationsWatcherRef: string; private eventTiles: Record = {}; + // A map to allow groupers to maintain consistent keys even if their first event is uprooted due to back-pagination. + public grouperKeyMap = new WeakMap(); + constructor(props, context) { super(props, context); @@ -684,14 +687,13 @@ export default class MessagePanel extends React.Component { break; // break on first grouper } } + if (!grouper) { - const wantTile = this.shouldShowEvent(mxEv); - const isGrouped = false; - if (wantTile) { + if (this.shouldShowEvent(mxEv)) { // make sure we unpack the array returned by getTilesForEvent, - // otherwise react will auto-generate keys and we will end up - // replacing all of the DOM elements every time we paginate. - ret.push(...this.getTilesForEvent(prevEvent, mxEv, last, isGrouped, nextEvent, nextTile)); + // otherwise React will auto-generate keys, and we will end up + // replacing all the DOM elements every time we paginate. + ret.push(...this.getTilesForEvent(prevEvent, mxEv, last, false, nextEvent, nextTile)); prevEvent = mxEv; } @@ -1127,7 +1129,7 @@ class CreationGrouper extends BaseGrouper { if (!this.events || !this.events.length) return []; const panel = this.panel; - const ret = []; + const ret: ReactNode[] = []; const isGrouped = true; const createEvent = this.event; const lastShownEvent = this.lastShownEvent; @@ -1135,7 +1137,7 @@ class CreationGrouper extends BaseGrouper { if (panel.wantsDateSeparator(this.prevEvent, createEvent.getDate())) { const ts = createEvent.getTs(); ret.push( -
    • , +
    • , ); } @@ -1262,6 +1264,10 @@ class MainGrouper extends BaseGrouper { this.events.push(ev); } + private generateKey(): string { + return "eventlistsummary-" + this.events[0].getId(); + } + public getTiles(): ReactNode[] { // If we don't have any events to group, don't even try to group them. The logic // below assumes that we have a group of events to deal with, but we might not if @@ -1271,24 +1277,28 @@ class MainGrouper extends BaseGrouper { const isGrouped = true; const panel = this.panel; const lastShownEvent = this.lastShownEvent; - const ret = []; + const ret: ReactNode[] = []; if (panel.wantsDateSeparator(this.prevEvent, this.events[0].getDate())) { const ts = this.events[0].getTs(); ret.push( -
    • , +
    • , ); } - // Ensure that the key of the EventListSummary does not change with new events. - // This will prevent it from being re-created unnecessarily, and - // instead will allow new props to be provided. In turn, the shouldComponentUpdate - // method on ELS can be used to prevent unnecessary renderings. - // - // Whilst back-paginating with an ELS at the top of the panel, prevEvent will be null, - // so use the key "eventlistsummary-initial". Otherwise, use the ID of the first - // membership event, which will not change during forward pagination. - const key = "eventlistsummary-" + (this.prevEvent ? this.events[0].getId() : "initial"); + // Ensure that the key of the EventListSummary does not change with new events in either direction. + // This will prevent it from being re-created unnecessarily, and instead will allow new props to be provided. + // In turn, the shouldComponentUpdate method on ELS can be used to prevent unnecessary renderings. + const keyEvent = this.events.find(e => this.panel.grouperKeyMap.get(e)); + const key = keyEvent ? this.panel.grouperKeyMap.get(keyEvent) : this.generateKey(); + if (!keyEvent) { + // Populate the weak map with the key that we are using for all related events. + this.events.forEach(e => { + if (!this.panel.grouperKeyMap.has(e)) { + this.panel.grouperKeyMap.set(e, key); + } + }); + } let highlightInSummary = false; let eventTiles = this.events.map((e, i) => { diff --git a/test/components/structures/MessagePanel-test.js b/test/components/structures/MessagePanel-test.js index c0ca398ce1..baaf262ace 100644 --- a/test/components/structures/MessagePanel-test.js +++ b/test/components/structures/MessagePanel-test.js @@ -496,4 +496,80 @@ describe('MessagePanel', function() { expect(Dates.length).toEqual(1); }); + + it('appends events into summaries during forward pagination without changing key', () => { + const events = mkMelsEvents().slice(1, 11); + + const res = mount(); + let els = res.find("EventListSummary"); + expect(els.length).toEqual(1); + expect(els.key()).toEqual("eventlistsummary-" + events[0].getId()); + expect(els.prop("events").length).toEqual(10); + + res.setProps({ + events: [ + ...events, + TestUtilsMatrix.mkMembership({ + event: true, + room: "!room:id", + user: "@user:id", + target: { + userId: "@user:id", + name: "Bob", + getAvatarUrl: () => { + return "avatar.jpeg"; + }, + getMxcAvatarUrl: () => 'mxc://avatar.url/image.png', + }, + ts: Date.now(), + mship: 'join', + prevMship: 'join', + name: 'A user', + }), + ], + }); + + els = res.find("EventListSummary"); + expect(els.length).toEqual(1); + expect(els.key()).toEqual("eventlistsummary-" + events[0].getId()); + expect(els.prop("events").length).toEqual(11); + }); + + it('prepends events into summaries during backward pagination without changing key', () => { + const events = mkMelsEvents().slice(1, 11); + + const res = mount(); + let els = res.find("EventListSummary"); + expect(els.length).toEqual(1); + expect(els.key()).toEqual("eventlistsummary-" + events[0].getId()); + expect(els.prop("events").length).toEqual(10); + + res.setProps({ + events: [ + TestUtilsMatrix.mkMembership({ + event: true, + room: "!room:id", + user: "@user:id", + target: { + userId: "@user:id", + name: "Bob", + getAvatarUrl: () => { + return "avatar.jpeg"; + }, + getMxcAvatarUrl: () => 'mxc://avatar.url/image.png', + }, + ts: Date.now(), + mship: 'join', + prevMship: 'join', + name: 'A user', + }), + ...events, + ], + }); + + els = res.find("EventListSummary"); + expect(els.length).toEqual(1); + expect(els.key()).toEqual("eventlistsummary-" + events[0].getId()); + expect(els.prop("events").length).toEqual(11); + }); });