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.

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

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

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Jul 13, 2022

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


See matrix-org/matrix-react-sdk#8354 (comment)

We also have to keep in mind that we don't want to mix messages in the wrong timelines (main vs threaded timeline):

Dev notes

  • TimelineSet.addEventsToTimeline -> TimelineSet.addEventToTimeline -> Timeline.addEvent
  • TimelineSet.addEventToTimeline -> Timeline.addEvent
  • TimelineSet.addLiveEvent -> TimelineSet.addEventToTimeline -> Timeline.addEvent

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fix getLatestTimeline not working when the latest event in the room is a threaded message (#2521). Contributed by @MadLittleMods.

Comment on lines +183 to +204
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);
});
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,

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();
});
});

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

@MadLittleMods MadLittleMods Jul 13, 2022

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 -5291 to -5215
if (!timelineSet.canContain(event)) {
return undefined;
}
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jul 13, 2022

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?

@@ -5338,9 +5334,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
// There is no guarantee that the event ended up in "timeline" (we might have switched to a neighbouring
// timeline) - so check the room's index again. On the other hand, there's no guarantee the event ended up
// anywhere, if it was later redacted, so we just return the timeline we first thought of.
return timelineSet.getTimelineForEvent(eventId)
?? timelineSet.room.findThreadForEvent(event)?.liveTimeline // for Threads degraded support
Copy link
Contributor Author

Choose a reason for hiding this comment

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

?? timelineSet.room.findThreadForEvent(event)?.liveTimeline // for Threads degraded support was added in #2261. Is it necessary? I'm just trying to get the context behind why we have it.

What does Threads degraded support exactly map to?

If supportsExperimentalThreads is disabled all events will be added to the main timeline (eventShouldLiveIn always returns shouldLiveInRoom: true)

Copy link
Member

@t3chguy t3chguy Jul 13, 2022

Choose a reason for hiding this comment

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

Threads degraded support

Server has no APIs to help the client so the client has to build the threads objects without filters & relations support on the server based on /sync and /messages data alone, which is a mode we have to continue to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the context!

I think it's safe to drop this case given getTimelineForEvent is always used in a fire and forget manner, see #2521 (comment)

@MadLittleMods MadLittleMods changed the title Draft: Fix getLatestTimeline not working when the latest event in the room is a threaded message Fix getLatestTimeline not working when the latest event in the room is a threaded message Jul 13, 2022
@MadLittleMods MadLittleMods marked this pull request as ready for review July 13, 2022 02:58
@MadLittleMods MadLittleMods requested a review from a team as a code owner July 13, 2022 02:58
@@ -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

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

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)

MadLittleMods added a commit to matrix-org/matrix-react-sdk that referenced this pull request Jul 19, 2022
@MadLittleMods MadLittleMods requested review from t3chguy and a team and removed request for t3chguy July 21, 2022 20:59
@@ -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.

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.

]);
// getEventTimeline -> thread.fetchInitialEvents
httpBackend.when("GET", "/rooms/!foo%3Abar/relations/" +
encodeURIComponent(THREAD_ROOT.event_id) + "/" +
Copy link
Contributor Author

@MadLittleMods MadLittleMods Aug 29, 2022

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

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

apologies for the late review on this - it somehow got lost in my queue :(

@@ -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.

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.

…msc2716-marker-events-v2

Conflicts:
	spec/integ/matrix-client-event-timeline.spec.ts
	src/client.ts
} else {
const tl = this.timelineSet.getLiveTimeline();
initFields(tl);
return Promise.resolve();
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split out to #2849

Comment on lines +698 to +709
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;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split out to #2848

MadLittleMods added a commit that referenced this pull request Nov 4, 2022
Add checks to `addEventToTimeline` as extra insurance that we don't mix events in the wrong timelines (main timeline vs thread timeline).

Split out from #2521

Previously, we just relied on the callers to make sure they're doing the right thing and since it's easy to get it wrong, we mixed and bugs happened.

Call stacks for how events get added to a timeline:

 - `TimelineSet.addEventsToTimeline` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
 - `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
 - `TimelineSet.addLiveEvent` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
@MadLittleMods MadLittleMods deleted the madlittlemods/refresh-timeline-when-we-see-msc2716-marker-events-v2 branch November 4, 2022 11:13
MadLittleMods added a commit that referenced this pull request Nov 4, 2022
@MadLittleMods
Copy link
Contributor Author

Closed as I went a different direction in this v2 PR, #2852

MadLittleMods added a commit that referenced this pull request Nov 7, 2022
…2856)

Add checks to `addEventToTimeline` as extra insurance that we don't mix events in the wrong timelines (main timeline vs thread timeline).

Split out from #2521

This PR is a v2 of #2848 since it was reverted in #2853

Previously, we just relied on the callers to make sure they're doing the right thing and since it's easy to get it wrong, we mixed and bugs happened.

Call stacks for how events get added to a timeline:

 - `TimelineSet.addEventsToTimeline` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
 - `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
 - `TimelineSet.addLiveEvent` -> `TimelineSet.addEventToTimeline` -> `Timeline.addEvent`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants