From 55d9dbf00dd05d440cc07de4d933ec83239cfea5 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 25 Nov 2022 13:22:06 +0100 Subject: [PATCH] Fix thread list jumping back down while scrolling (#9606) * Fix timeline jumping after every thread update * Add tests to prevent this from occuring again --- src/components/structures/MessagePanel.tsx | 4 +- src/components/structures/ThreadPanel.tsx | 14 +- src/components/structures/TimelinePanel.tsx | 27 ++- .../structures/TimelinePanel-test.tsx | 198 +++++++++++++++++- 4 files changed, 218 insertions(+), 25 deletions(-) diff --git a/src/components/structures/MessagePanel.tsx b/src/components/structures/MessagePanel.tsx index d6e37eed18b..26190a24fc9 100644 --- a/src/components/structures/MessagePanel.tsx +++ b/src/components/structures/MessagePanel.tsx @@ -347,8 +347,8 @@ export default class MessagePanel extends React.Component { 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]; diff --git a/src/components/structures/ThreadPanel.tsx b/src/components/structures/ThreadPanel.tsx index 245c604c0af..64e6ff62945 100644 --- a/src/components/structures/ThreadPanel.tsx +++ b/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 = ({ }); }, [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) { diff --git a/src/components/structures/TimelinePanel.tsx b/src/components/structures/TimelinePanel.tsx index d2a3f390b4d..06b866a49e7 100644 --- a/src/components/structures/TimelinePanel.tsx +++ b/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 { 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 { 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 { 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 { 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 => { diff --git a/test/components/structures/TimelinePanel-test.tsx b/test/components/structures/TimelinePanel-test.tsx index 6082c7e7514..3a55fd3fdfd 100644 --- a/test/components/structures/TimelinePanel-test.tsx +++ b/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(); 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( + + + , + ); + 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( + + + , + ); + 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(); + }); + }); });