Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

Closed
wants to merge 12 commits into from
Closed

Fix getLatestTimeline not working when the latest event in the room is a threaded message #2521

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5f4d10d
Fix getLatestTimeline not working when the latest even in the room wa…
MadLittleMods Jul 13, 2022
e8dc590
Adjust tests to not include problem events instead of undefined timeline
MadLittleMods Jul 13, 2022
9882eb6
Add tests for not being able to add mixed events
MadLittleMods Jul 13, 2022
fdeb9e0
Correct JSDoc and types for getEventTimeline
MadLittleMods Jul 13, 2022
78f8019
No longer need check since it should always return a timeline
MadLittleMods Jul 13, 2022
50b5bb1
Add getLatestTimeline test for latest event being thread
MadLittleMods Jul 13, 2022
a3457e1
Add non-optional client to Room instances
MadLittleMods Jul 13, 2022
cdef5cb
Fix lint
MadLittleMods Jul 13, 2022
cd41d7e
We expect void so return void
MadLittleMods Jul 19, 2022
a3ea213
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods Aug 29, 2022
b97de19
Merge branch 'develop' into madlittlemods/refresh-timeline-when-we-se…
MadLittleMods Oct 14, 2022
f82ea0c
Fix some lints
MadLittleMods Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ node_modules
/*.log
package-lock.json
.lock-wscript
.DS_Store
build/Release
coverage
lib-cov
Expand Down
50 changes: 40 additions & 10 deletions spec/integ/matrix-client-event-timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ describe("MatrixClient event timelines", function() {
expect(timeline.getEvents().find(e => e.getId() === THREAD_ROOT.event_id)).toBeTruthy();
});

it("should return undefined when event is not in the thread that the given timelineSet is representing", () => {
it("should not include main timeline event when timelineSet is representing a thread", async () => {
// @ts-ignore
client.clientOpts.experimentalThreadSupport = true;
Thread.setServerSideSupport(true, false);
Expand All @@ -649,13 +649,40 @@ describe("MatrixClient event timelines", function() {
};
});

return Promise.all([
expect(client.getEventTimeline(timelineSet, EVENTS[0].event_id)).resolves.toBeUndefined(),
httpBackend.flushAllExpected(),
]);
// getEventTimeline -> thread.fetchInitialEvents
httpBackend.when("GET", "/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id) + "/" +
Copy link
Contributor Author

@MadLittleMods MadLittleMods Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some lints in CI: https://github.com/matrix-org/matrix-js-sdk/runs/8077711103?check_suite_focus=true

But I don't see them locally even after re-installing node_modules to ensure correct versions.

$ yarn lint
yarn run v1.22.18
$ yarn lint:types && yarn lint:js
$ tsc --noEmit
$ eslint --max-warnings 0 src spec
✨  Done in 21.60s.

(on the correct branch, madlittlemods/refresh-timeline-when-we-see-msc2716-marker-events-v2)

This is also the same pattern we use in the existing tests here but don't appear because they're not in the diff so will need to refactor this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are shown if you use tsc --strict - hence being reported by the Typescript Strict Error Checker

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably merge develop to resolve all of these unrelated errors now that #2835 fixed them up

encodeURIComponent(THREAD_RELATION_TYPE.name) + "?limit=20&direction=b")
.respond(200, function() {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
// no next batch as this is the oldest end of the timeline
};
});

// getEventTimeline -> thread.fetchEvents
httpBackend.when("GET", "/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id) + "/" +
encodeURIComponent(THREAD_RELATION_TYPE.name) + "?direction=b&limit=50")
.respond(200, function() {
return {
original_event: THREAD_ROOT,
chunk: [THREAD_REPLY],
// no next batch as this is the oldest end of the timeline
};
});

const timelinePromise = client.getEventTimeline(timelineSet, EVENTS[0].event_id);
await httpBackend.flushAllExpected();

const timeline = await timelinePromise;

// The main timeline event should not be in the timelineSet representing a thread
expect(timeline.getEvents().find(e => e.getId() === EVENTS[0].event_id)).toBeFalsy();
});

it("should return undefined when event is within a thread but timelineSet is not", () => {
it("should not include threaded reply when timelineSet is representing the main room", async () => {
// @ts-ignore
client.clientOpts.experimentalThreadSupport = true;
Thread.setServerSideSupport(true, false);
Expand All @@ -675,10 +702,13 @@ describe("MatrixClient event timelines", function() {
};
});

return Promise.all([
expect(client.getEventTimeline(timelineSet, THREAD_REPLY.event_id)).resolves.toBeUndefined(),
httpBackend.flushAllExpected(),
]);
const timelinePromise = client.getEventTimeline(timelineSet, THREAD_REPLY.event_id);
await httpBackend.flushAllExpected();

const timeline = await timelinePromise;

// The threaded reply should not be in a main room timeline
expect(timeline.getEvents().find(e => e.getId() === THREAD_REPLY.event_id)).toBeFalsy();
});

it("should should add lazy loading filter when requested", async () => {
Expand Down
2 changes: 1 addition & 1 deletion spec/integ/matrix-client-room-timeline.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ describe("MatrixClient room timelines", function() {
});

// Wait for the timeline to reset(when it goes blank) which means
// it's in the middle of the refrsh logic right before the
// it's in the middle of the refresh logic right before the
// `getEventTimeline()` -> `/context`. Then simulate a racey `/sync`
// to happen in the middle of all of this refresh timeline logic. We
// want to make sure the sync pagination still works as expected
Expand Down
76 changes: 59 additions & 17 deletions spec/unit/event-timeline-set.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,23 @@ describe('EventTimelineSet', () => {
});
};

const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: userA,
room: roomId,
content: {
"body": "Thread response :: " + Math.random(),
"m.relates_to": {
"event_id": root.getId(),
"m.in_reply_to": {
"event_id": root.getId(),
},
"rel_type": "m.thread",
},
},
}, room.client);

beforeEach(() => {
client = utils.mock(MatrixClient, 'MatrixClient');
client.reEmitter = utils.mock(ReEmitter, 'ReEmitter');
Expand Down Expand Up @@ -116,6 +133,13 @@ describe('EventTimelineSet', () => {
});

describe('addEventToTimeline', () => {
let thread: Thread;

beforeEach(() => {
(client.supportsExperimentalThreads as jest.Mock).mockReturnValue(true);
thread = new Thread("!thread_id:server", messageEvent, { room, client });
});

it("Adds event to timeline", () => {
const liveTimeline = eventTimelineSet.getLiveTimeline();
expect(liveTimeline.getEvents().length).toStrictEqual(0);
Expand Down Expand Up @@ -143,6 +167,41 @@ describe('EventTimelineSet', () => {
);
}).not.toThrow();
});

it("should not add an event to a timeline that does not belong to the timelineSet", () => {
const eventTimelineSet2 = new EventTimelineSet(room);
const liveTimeline2 = eventTimelineSet2.getLiveTimeline();
expect(liveTimeline2.getEvents().length).toStrictEqual(0);

expect(() => {
eventTimelineSet.addEventToTimeline(messageEvent, liveTimeline2, {
toStartOfTimeline: true,
});
}).toThrowError();
});

it("should not add a threaded reply to the main room timeline", () => {
const liveTimeline = eventTimelineSet.getLiveTimeline();
expect(liveTimeline.getEvents().length).toStrictEqual(0);

const threadedReplyEvent = mkThreadResponse(messageEvent);

eventTimelineSet.addEventToTimeline(threadedReplyEvent, liveTimeline, {
toStartOfTimeline: true,
});
expect(liveTimeline.getEvents().length).toStrictEqual(0);
});

it("should not add a normal message to the timelineSet representing a thread", () => {
const eventTimelineSetForThread = new EventTimelineSet(room, {}, client, thread);
const liveTimeline = eventTimelineSetForThread.getLiveTimeline();
expect(liveTimeline.getEvents().length).toStrictEqual(0);

eventTimelineSetForThread.addEventToTimeline(messageEvent, liveTimeline, {
toStartOfTimeline: true,
});
expect(liveTimeline.getEvents().length).toStrictEqual(0);
});
Comment on lines +184 to +205
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests the two main mixing scenarios. We could add tests for the other mixing scenarios but we're using canContain which already has it's own set of tests,

matrix-js-sdk/spec/unit/event-timeline-set.spec.ts

Lines 227 to 293 in 5112340

describe("canContain", () => {
const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: userA,
room: roomId,
content: {
"body": "Thread response :: " + Math.random(),
"m.relates_to": {
"event_id": root.getId(),
"m.in_reply_to": {
"event_id": root.getId(),
},
"rel_type": "m.thread",
},
},
}, room.client);
let thread: Thread;
beforeEach(() => {
(client.supportsExperimentalThreads as jest.Mock).mockReturnValue(true);
thread = new Thread("!thread_id:server", messageEvent, { room, client });
});
it("should throw if timeline set has no room", () => {
const eventTimelineSet = new EventTimelineSet(undefined, {}, client);
expect(() => eventTimelineSet.canContain(messageEvent)).toThrowError();
});
it("should return false if timeline set is for thread but event is not threaded", () => {
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
expect(eventTimelineSet.canContain(replyEvent)).toBeFalsy();
});
it("should return false if timeline set it for thread but event it for a different thread", () => {
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
const event = mkThreadResponse(replyEvent);
expect(eventTimelineSet.canContain(event)).toBeFalsy();
});
it("should return false if timeline set is not for a thread but event is a thread response", () => {
const eventTimelineSet = new EventTimelineSet(room, {}, client);
const event = mkThreadResponse(replyEvent);
expect(eventTimelineSet.canContain(event)).toBeFalsy();
});
it("should return true if the timeline set is not for a thread and the event is a thread root", () => {
const eventTimelineSet = new EventTimelineSet(room, {}, client);
expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
});
it("should return true if the timeline set is for a thread and the event is its thread root", () => {
const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
messageEvent.setThread(thread);
expect(eventTimelineSet.canContain(messageEvent)).toBeTruthy();
});
it("should return true if the timeline set is for a thread and the event is a response to it", () => {
const thread = new Thread(messageEvent.getId(), messageEvent, { room, client });
const eventTimelineSet = new EventTimelineSet(room, {}, client, thread);
messageEvent.setThread(thread);
const event = mkThreadResponse(messageEvent);
expect(eventTimelineSet.canContain(event)).toBeTruthy();
});
});

});

describe('aggregateRelations', () => {
Expand Down Expand Up @@ -225,23 +284,6 @@ describe('EventTimelineSet', () => {
});

describe("canContain", () => {
const mkThreadResponse = (root: MatrixEvent) => utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: userA,
room: roomId,
content: {
"body": "Thread response :: " + Math.random(),
"m.relates_to": {
"event_id": root.getId(),
"m.in_reply_to": {
"event_id": root.getId(),
},
"rel_type": "m.thread",
},
},
}, room.client);

let thread: Thread;

beforeEach(() => {
Expand Down
8 changes: 2 additions & 6 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5234,15 +5234,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
* <p>If the EventTimelineSet object already has the given event in its store, the
* corresponding timeline will be returned. Otherwise, a /context request is
* made, and used to construct an EventTimeline.
* If the event does not belong to this EventTimelineSet then undefined will be returned.
* If the event does not belong to this EventTimelineSet then it will ignored.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event will be ignored? What does that even mean? This method isn't meant to process the given event, just use it as a pointer. What event timeline will be returned to the caller? The caller would now have need to manually check that the returned timeline is valid for what they asked for, I guess by your other change lower down by attempting to add an event and by asserting that it worked, that seems rather strange

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's first discuss the previous behavior. Previously, when the event didn't belong to the timelineSet, we would return undefined and create no additional timelines. Returning undefined has no special meaning given that all of the usage is always fire and forget meaning we don't use the timeline returned by client.getEventTimeline() and only use the function to load the event so it's available to the client.

In our usage, there is only a single spot where the caller uses the timeline returned by client.getEventTimeline(). This usage should be replaced by the fire and forget pattern and use room.findEventById(eventId) because it doesn't even use the timeline, it just wants the event that was loaded in as well.

It's probably a misnomer to call it getEventTimeline(): timeline in the first place as it's more accurately used as loadEventInTimeline(): Promise<void> everywhere.

And my new refreshLiveTimeline and getLatestTimeline usage is the only one where it needs an actually timeline.

With the updates, we're only working within the given timelineSet that was passed in (seems reasonable). If client.getEventTimeline() is really meant to just give the timeline for the eventId, then we should just provide the room instead which can look at all of the timelineSets (room.timelineSets).

Previously, we would return undefined in the case where the event doesn't belong in the timelineSet. Now we're returning a timeline in the timelineSet where all of the events returned by /context can go. This means events that we fetched, are actually added and not wasted. And it means that the main room timeline can be populated regardless if the eventId passed in was a threaded reply.

By ignored, I mean if the eventId is a threaded reply, it won't be added to the main room timelineSet that was passed in for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our usage might be to be a fire-and-forget pattern (or wrong, in the case of needing to use findEventById instead), but as a public function and SDK we have to maintain a rationalized contract for the function name: it says it gets an event timeline, so it should do that (returning undefined if needed)

We can adjust our code to instead use a new updateEventTimeline() function or similar, but the existing getEventTimeline function can't realistically have a behavioural change like this.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return undefined part is part of supportsExperimentalThreads which changed in https://github.com/matrix-org/matrix-js-sdk/pull/2444/files

Can we change the experimental implementation?

We can adjust our code to instead use a new updateEventTimeline() function or similar, but the existing getEventTimeline function can't realistically have a behavioural change like this.

In any case, this can work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not comfortable with the behavioural change here, sorry. While our usage might be fire-and-forget, we can't guarantee that all usages of the function are fire-and-forget. The documentation and function itself are not experimental in nature as well, preventing us from making arbitrary breaking changes.

Adding a function is more code, but I think it's worthwhile here. It can even call getEventTimeline() and ignore the return value - it looks a bit silly, but it's how we avoid unnecessary major version releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversation continued at #2852 (comment)

*
* @param {EventTimelineSet} timelineSet The timelineSet to look for the event in, must be bound to a room
* @param {string} eventId The ID of the event to look for
*
* @return {Promise} Resolves:
* {@link module:models/event-timeline~EventTimeline} including the given event
*/
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline | undefined> {
public async getEventTimeline(timelineSet: EventTimelineSet, eventId: string): Promise<EventTimeline> {
// don't allow any timeline support unless it's been enabled.
if (!this.timelineSupport) {
throw new Error("timeline support is disabled. Set the 'timelineSupport'" +
Expand Down Expand Up @@ -5288,10 +5288,6 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
];

if (this.supportsExperimentalThreads()) {
if (!timelineSet.canContain(event)) {
return undefined;
}
Comment on lines -5213 to -5215
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined, we now just ignore the event and don't add it to the given timelineSet.

Did returning undefined have some other special meaning? The getEventTimeline usage didn't stand out to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did returning undefined have some other special meaning? The getEventTimeline usage didn't stand out to me.

I don't think it has any special meaning, see #2521 (comment)

Comment on lines -5213 to -5215
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make sure we're not regressing #2444 and #2454 for sure, do we have a specific testing strategy in the Element app to reproduce previously (main timeline events in a thread for example)? How do we know that those previous PR's fixed the problem besides reasoning about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t3chguy Any hints here?


// Where the event is a thread reply (not a root) and running in MSC-enabled mode the Thread timeline only
// functions contiguously, so we have to jump through some hoops to get our target event in it.
// XXX: workaround for https://github.com/vector-im/element-meta/issues/150
Expand Down
12 changes: 12 additions & 0 deletions src/models/event-timeline-set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,18 @@ export class EventTimelineSet extends TypedEventEmitter<EmittedEvents, EventTime
);
}

if (timeline.getTimelineSet() !== this) {
throw new Error(`EventTimelineSet.addEventToTimeline: Timeline=${timeline.toString()} does not belong " +
"in timelineSet(threadId=${this.thread?.id})`);
}

// Make sure events don't get mixed in timelines they shouldn't be in
// (e.g. a threaded message should not be in the main timeline).
if (!this.canContain(event)) {
logger.warn(`EventTimelineSet.addEventToTimeline: Ignoring event=${event.getId()} that does not belong in timeline=${timeline.toString()} timelineSet(threadId=${this.thread?.id})`);
return;
}

const eventId = event.getId();
timeline.addEvent(event, {
toStartOfTimeline,
Expand Down