Fix thread list jumping back down while scrolling (#9606)

* Fix timeline jumping after every thread update

* Add tests to prevent this from occuring again

Author: justjanne

src/components/structures/MessagePanel.tsx

@@ -347,8 +347,8 @@ export default class MessagePanel extends React.Component<IProps, IState> {
         return this.eventTiles[eventId]?.ref?.current;
     }
 
-    public getTileForEventId(eventId: string): UnwrappedEventTile {
-        if (!this.eventTiles) {
+    public getTileForEventId(eventId?: string): UnwrappedEventTile | undefined {
+        if (!this.eventTiles || !eventId) {
             return undefined;
         }
         return this.eventTiles[eventId];

src/components/structures/ThreadPanel.tsx

@@ -16,7 +16,7 @@ limitations under the License.
 
 import React, { useContext, useEffect, useRef, useState } from 'react';
 import { EventTimelineSet } from 'matrix-js-sdk/src/models/event-timeline-set';
-import { Thread, ThreadEvent } from 'matrix-js-sdk/src/models/thread';
+import { Thread } from 'matrix-js-sdk/src/models/thread';
 import { Room } from 'matrix-js-sdk/src/models/room';
 
 import BaseCard from "../views/right_panel/BaseCard";
@@ -206,18 +206,6 @@ const ThreadPanel: React.FC<IProps> = ({
         });
     }, [mxClient, roomId]);
 
-    useEffect(() => {
-        function refreshTimeline() {
-            timelinePanel?.current.refreshTimeline();
-        }
-
-        room?.on(ThreadEvent.Update, refreshTimeline);
-
-        return () => {
-            room?.removeListener(ThreadEvent.Update, refreshTimeline);
-        };
-    }, [room, mxClient, timelineSet]);
-
     useEffect(() => {
         if (room) {
             if (filterOption === ThreadFilterType.My) {

src/components/structures/TimelinePanel.tsx

@@ -27,7 +27,7 @@ import { RoomMember, RoomMemberEvent } from 'matrix-js-sdk/src/models/room-membe
 import { debounce, throttle } from 'lodash';
 import { logger } from "matrix-js-sdk/src/logger";
 import { ClientEvent } from "matrix-js-sdk/src/client";
-import { Thread } from 'matrix-js-sdk/src/models/thread';
+import { Thread, ThreadEvent } from 'matrix-js-sdk/src/models/thread';
 import { ReceiptType } from "matrix-js-sdk/src/@types/read_receipts";
 import { MatrixError } from 'matrix-js-sdk/src/http-api';
 import { ReadReceipt } from 'matrix-js-sdk/src/models/read-receipt';
@@ -297,6 +297,8 @@ class TimelinePanel extends React.Component<IProps, IState> {
         cli.on(MatrixEventEvent.Decrypted, this.onEventDecrypted);
         cli.on(MatrixEventEvent.Replaced, this.onEventReplaced);
         cli.on(ClientEvent.Sync, this.onSync);
+
+        this.props.timelineSet.room?.on(ThreadEvent.Update, this.onThreadUpdate);
     }
 
     public componentDidMount() {
@@ -325,6 +327,9 @@ class TimelinePanel extends React.Component<IProps, IState> {
             logger.warn("Replacing timelineSet on a TimelinePanel - confusion may ensue");
         }
 
+        this.props.timelineSet.room?.off(ThreadEvent.Update, this.onThreadUpdate);
+        this.props.timelineSet.room?.on(ThreadEvent.Update, this.onThreadUpdate);
+
         const differentEventId = prevProps.eventId != this.props.eventId;
         const differentHighlightedEventId = prevProps.highlightedEventId != this.props.highlightedEventId;
         const differentAvoidJump = prevProps.eventScrollIntoView && !this.props.eventScrollIntoView;
@@ -366,6 +371,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
             client.removeListener(MatrixEventEvent.Replaced, this.onEventReplaced);
             client.removeListener(MatrixEventEvent.VisibilityChange, this.onEventVisibilityChange);
             client.removeListener(ClientEvent.Sync, this.onSync);
+            this.props.timelineSet.room?.removeListener(ThreadEvent.Update, this.onThreadUpdate);
         }
     }
 
@@ -742,6 +748,25 @@ class TimelinePanel extends React.Component<IProps, IState> {
         this.forceUpdate();
     };
 
+    private onThreadUpdate = (thread: Thread): void => {
+        if (this.unmounted) {
+            return;
+        }
+
+        // ignore events for other rooms
+        const roomId = thread.roomId;
+        if (roomId !== this.props.timelineSet.room?.roomId) {
+            return;
+        }
+
+        // we could skip an update if the event isn't in our timeline,
+        // but that's probably an early optimisation.
+        const tile = this.messagePanel.current?.getTileForEventId(thread.id);
+        if (tile) {
+            tile.forceUpdate();
+        }
+    };
+
     // Called whenever the visibility of an event changes, as per
     // MSC3531. We typically need to re-render the tile.
     private onEventVisibilityChange = (ev: MatrixEvent): void => {

test/components/structures/TimelinePanel-test.tsx

@@ -14,25 +14,34 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React from 'react';
+import { render, RenderResult } from "@testing-library/react";
 // eslint-disable-next-line deprecate/import
 import { mount, ReactWrapper } from "enzyme";
-import { EventTimeline } from "matrix-js-sdk/src/models/event-timeline";
 import { MessageEvent } from 'matrix-events-sdk';
+import { ReceiptType } from "matrix-js-sdk/src/@types/read_receipts";
 import {
     EventTimelineSet,
     EventType,
+    MatrixClient,
     MatrixEvent,
     PendingEventOrdering,
     Room,
 } from 'matrix-js-sdk/src/matrix';
-import { ReceiptType } from "matrix-js-sdk/src/@types/read_receipts";
-import { render, RenderResult } from "@testing-library/react";
+import { EventTimeline } from "matrix-js-sdk/src/models/event-timeline";
+import {
+    FeatureSupport,
+    Thread,
+    THREAD_RELATION_TYPE,
+    ThreadEvent,
+    ThreadFilterType,
+} from "matrix-js-sdk/src/models/thread";
+import React from 'react';
 
-import { mkRoom, stubClient } from "../../test-utils";
 import TimelinePanel from '../../../src/components/structures/TimelinePanel';
+import MatrixClientContext from "../../../src/contexts/MatrixClientContext";
 import { MatrixClientPeg } from '../../../src/MatrixClientPeg';
 import SettingsStore from "../../../src/settings/SettingsStore";
+import { mkRoom, stubClient } from "../../test-utils";
 
 const newReceipt = (eventId: string, userId: string, readTs: number, fullyReadTs: number): MatrixEvent => {
     const receiptContent = {
@@ -52,7 +61,7 @@ const getProps = (room: Room, events: MatrixEvent[]): TimelinePanel["props"] =>
     timelineSet.getLiveTimeline = () => timeline;
     timelineSet.getTimelineForEvent = () => timeline;
     timelineSet.getPendingEvents = () => events;
-    timelineSet.room.getEventReadUpTo = () => events[1].getId();
+    timelineSet.room!.getEventReadUpTo = () => events[1].getId() ?? null;
 
     return {
         timelineSet,
@@ -67,7 +76,7 @@ const renderPanel = (room: Room, events: MatrixEvent[]): RenderResult => {
 };
 
 const mockEvents = (room: Room, count = 2): MatrixEvent[] => {
-    const events = [];
+    const events: MatrixEvent[] = [];
     for (let index = 0; index < count; index++) {
         events.push(new MatrixEvent({
             room_id: room.roomId,
@@ -89,7 +98,7 @@ describe('TimelinePanel', () => {
     describe('read receipts and markers', () => {
         it('should forget the read marker when asked to', () => {
             const cli = MatrixClientPeg.get();
-            const readMarkersSent = [];
+            const readMarkersSent: string[] = [];
 
             // Track calls to setRoomReadMarkers
             cli.setRoomReadMarkers = (_roomId, rmEventId, _a, _b) => {
@@ -111,7 +120,7 @@ describe('TimelinePanel', () => {
             });
 
             const roomId = "#room:example.com";
-            const userId = cli.credentials.userId;
+            const userId = cli.credentials.userId!;
             const room = new Room(
                 roomId,
                 cli,
@@ -192,4 +201,175 @@ describe('TimelinePanel', () => {
         rerender(<TimelinePanel {...props} />);
         expect(props.onEventScrolledIntoView).toHaveBeenCalledWith(events[1].getId());
     });
+
+    describe("when a thread updates", () => {
+        let client: MatrixClient;
+        let room: Room;
+        let allThreads: EventTimelineSet;
+        let root: MatrixEvent;
+        let reply1: MatrixEvent;
+        let reply2: MatrixEvent;
+
+        beforeEach(() => {
+            client = MatrixClientPeg.get();
+
+            Thread.hasServerSideSupport = FeatureSupport.Stable;
+            client.supportsExperimentalThreads = () => true;
+            const getValueCopy = SettingsStore.getValue;
+            SettingsStore.getValue = jest.fn().mockImplementation((name: string) => {
+                if (name === "feature_thread") return true;
+                return getValueCopy(name);
+            });
+
+            room = new Room("roomId", client, "userId");
+            allThreads = new EventTimelineSet(room, {
+                pendingEvents: false,
+            }, undefined, undefined, ThreadFilterType.All);
+            const timeline = new EventTimeline(allThreads);
+            allThreads.getLiveTimeline = () => timeline;
+            allThreads.getTimelineForEvent = () => timeline;
+
+            reply1 = new MatrixEvent({
+                room_id: room.roomId,
+                event_id: 'event_reply_1',
+                type: EventType.RoomMessage,
+                user_id: "userId",
+                content: MessageEvent.from(`ReplyEvent1`).serialize().content,
+            });
+
+            reply2 = new MatrixEvent({
+                room_id: room.roomId,
+                event_id: 'event_reply_2',
+                type: EventType.RoomMessage,
+                user_id: "userId",
+                content: MessageEvent.from(`ReplyEvent2`).serialize().content,
+            });
+
+            root = new MatrixEvent({
+                room_id: room.roomId,
+                event_id: 'event_root_1',
+                type: EventType.RoomMessage,
+                user_id: "userId",
+                content: MessageEvent.from(`RootEvent`).serialize().content,
+            });
+
+            const eventMap: { [key: string]: MatrixEvent } = {
+                [root.getId()!]: root,
+                [reply1.getId()!]: reply1,
+                [reply2.getId()!]: reply2,
+            };
+
+            room.findEventById = (eventId: string) => eventMap[eventId];
+            client.fetchRoomEvent = async (roomId: string, eventId: string) =>
+                roomId === room.roomId ? eventMap[eventId]?.event : {};
+        });
+
+        it('updates thread previews', async () => {
+            root.setUnsigned({
+                "m.relations": {
+                    [THREAD_RELATION_TYPE.name]: {
+                        "latest_event": reply1.event,
+                        "count": 1,
+                        "current_user_participated": true,
+                    },
+                },
+            });
+
+            const thread = room.createThread(root.getId()!, root, [], true);
+            // So that we do not have to mock the thread loading
+            thread.initialEventsFetched = true;
+            // @ts-ignore
+            thread.fetchEditsWhereNeeded = () => Promise.resolve();
+            await thread.addEvent(reply1, true);
+            await allThreads.getLiveTimeline().addEvent(thread.rootEvent!, true);
+            const replyToEvent = jest.spyOn(thread, "replyToEvent", "get");
+
+            const dom = render(
+                <MatrixClientContext.Provider value={client}>
+                    <TimelinePanel
+                        timelineSet={allThreads}
+                        manageReadReceipts
+                        sendReadReceiptOnLoad
+                    />
+                </MatrixClientContext.Provider>,
+            );
+            await dom.findByText("RootEvent");
+            await dom.findByText("ReplyEvent1");
+            expect(replyToEvent).toHaveBeenCalled();
+
+            root.setUnsigned({
+                "m.relations": {
+                    [THREAD_RELATION_TYPE.name]: {
+                        "latest_event": reply2.event,
+                        "count": 2,
+                        "current_user_participated": true,
+                    },
+                },
+            });
+
+            replyToEvent.mockClear();
+            await thread.addEvent(reply2, false, true);
+            await dom.findByText("RootEvent");
+            await dom.findByText("ReplyEvent2");
+            expect(replyToEvent).toHaveBeenCalled();
+        });
+
+        it('ignores thread updates for unknown threads', async () => {
+            root.setUnsigned({
+                "m.relations": {
+                    [THREAD_RELATION_TYPE.name]: {
+                        "latest_event": reply1.event,
+                        "count": 1,
+                        "current_user_participated": true,
+                    },
+                },
+            });
+
+            const realThread = room.createThread(root.getId()!, root, [], true);
+            // So that we do not have to mock the thread loading
+            realThread.initialEventsFetched = true;
+            // @ts-ignore
+            realThread.fetchEditsWhereNeeded = () => Promise.resolve();
+            await realThread.addEvent(reply1, true);
+            await allThreads.getLiveTimeline().addEvent(realThread.rootEvent!, true);
+            const replyToEvent = jest.spyOn(realThread, "replyToEvent", "get");
+
+            // @ts-ignore
+            const fakeThread1: Thread = {
+                id: undefined!,
+                get roomId(): string {
+                    return room.roomId;
+                },
+            };
+
+            const fakeRoom = new Room("thisroomdoesnotexist", client, "userId");
+            // @ts-ignore
+            const fakeThread2: Thread = {
+                id: root.getId()!,
+                get roomId(): string {
+                    return fakeRoom.roomId;
+                },
+            };
+
+            const dom = render(
+                <MatrixClientContext.Provider value={client}>
+                    <TimelinePanel
+                        timelineSet={allThreads}
+                        manageReadReceipts
+                        sendReadReceiptOnLoad
+                    />
+                </MatrixClientContext.Provider>,
+            );
+            await dom.findByText("RootEvent");
+            await dom.findByText("ReplyEvent1");
+            expect(replyToEvent).toHaveBeenCalled();
+
+            replyToEvent.mockClear();
+            room.emit(ThreadEvent.Update, fakeThread1);
+            room.emit(ThreadEvent.Update, fakeThread2);
+            await dom.findByText("ReplyEvent1");
+            expect(replyToEvent).not.toHaveBeenCalled();
+            replyToEvent.mockClear();
+        });
+    });
 });