Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Fix thread list jumping back down while scrolling (#9606)
Browse files Browse the repository at this point in the history 
* Fix timeline jumping after every thread update

* Add tests to prevent this from occuring again
  • Loading branch information
@justjanne
justjanne authored Nov 25, 2022
1 parent 5c60211 commit 55d9dbf
Show file tree
Hide file tree
Showing 4 changed files with 218 additions and 25 deletions.
4 changes: 2 additions & 2 deletions src/components/structures/MessagePanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
14 changes: 1 addition & 13 deletions src/components/structures/ThreadPanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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) {
Expand Down
27 changes: 26 additions & 1 deletion src/components/structures/TimelinePanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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 => {
Expand Down
198 changes: 189 additions & 9 deletions test/components/structures/TimelinePanel-test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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) => {
Expand All @@ -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,
Expand Down Expand Up @@ -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();
});
});
});

0 comments on commit 55d9dbf

Please sign in to comment.