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

Add event with m.reference relation to unknown parent to the timeline. #4084

Closed
wants to merge 2 commits into from
+20 −1
Closed

Add event with m.reference relation to unknown parent to the timeline. #4084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3523c3
Add event with m.reference relation to unknown parent to the timeline.
Feb 26, 2024
88c7461
Merge branch 'develop' into nic/feat/add_relation_reference_unknown_t…
maheichyk Feb 26, 2024
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
19 changes: 19 additions & 0 deletions spec/unit/room.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3199,6 +3199,25 @@ describe("Room", function () {
expect(room.eventShouldLiveIn(reply).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(reply).shouldLiveInThread).toBeFalsy();
});

it("an event with m.reference relation to unknown parent event should live in the main timeline only", async () => {
const event = utils.mkEvent({
event: true,
type: "org.example.child",
user: "@alice:example.org",
content: {
"key": "value",
"m.relates_to": {
event_id: "$unknown-parent",
rel_type: "m.reference",
},
},
room: roomId,
});

expect(room.eventShouldLiveIn(event).shouldLiveInRoom).toBeTruthy();
expect(room.eventShouldLiveIn(event).shouldLiveInThread).toBeFalsy();
});
});

describe("getEventReadUpTo()", () => {
Expand Down
2 changes: 1 addition & 1 deletion src/models/room.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2197,7 +2197,7 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
// Due to replies not being typical relations and being used as fallbacks for threads relations
// If we bypass the if case above then we know we are not a thread, so if we are still a reply
// then we know that we must be in the main timeline. Same goes if we have no associated parent event.
if (!parentEventId || !!event.replyEventId) {
if (!parentEventId || !!event.replyEventId || event.getRelation()?.rel_type === "m.reference") {
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 not sure I really see why an m.reference relation needs special treatment here: doesn't the logic apply to any relation that's not a thread (or possibly reply?) relation? Seems like any event with a kind of relation we don't know about, we should assume lives in the main thread? That said, I'd appreciate a second pair of eyes on this before actually suggesting this.

Also, the comment above (apart from having half a sentence) explains both clauses in the if condition, so if you add another, please update the comment to reflect it.

Copy link
Member

Choose a reason for hiding this comment

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

This change breaks the public API of eventShouldLiveIn because it states:

  • Relations, redactions, replies where the parent cannot be found live in no timelines but should be aggregated regardless.

That being said, the comment on this if statement clearly intends for some events to hit this but not m.reference. It does make sense to me to chuck it in the main timeline rather than dropping it entirely IMO.

We are warned on :2209:

adding the event in the wrong timeline causes stuck notifications and can break ability to send read receipts

can we be sure this PR won't do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not that deep into this stuck notification topic and therefore currently could not really prove that adding m.reference or other event relations (that sounds reasonable) will not produce this issue.

But not adding the event to the timeline produces the problem that event is not sent to the widget. That was working some time ago because it was previously possible to see these events in the timeline via devtools.

Another solution could be to explicitly check these events in StopGapWidget and feed them to the widget without checking timeline, here is a PR for that : matrix-org/matrix-react-sdk#12283 Could that be a more real option?

Copy link
Member

Choose a reason for hiding this comment

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

At the moment I'm more inclined to go with the other PR, which seems broadly sensible. The more I think about this, the more I think that actually there's no way we can legitimately say events live in any timeline if we don't know about their parent, so what we should actually be doing is keeping them in a holding area somewhere until we can fetch their parent. That or use matrix-org/matrix-spec-proposals#4023

return {
shouldLiveInRoom: true,
shouldLiveInThread: false,
Expand Down
Loading