Add event with m.reference relation to unknown parent to the timeline. #4084
Conversation
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
github-actions
bot
added
the
Z-Community-PR
|
@dbkr, @robintown I somehow cannot add a label (adding |
| @@ -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") { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
The main problem in relation to the widgets was resolved by another PR to matrix-react-sdk, although would be nice to have these events in the timeline to be shown by Element as well and not to have a special handling for the widgets. |
|
@maheichyk a very related issue is tracked in element-hq/element-web#27132 |
Yes, it is the same issue to me: event (with custom relation type) is ignored because the parent event is missing (not loaded yet) in the timeline. In our widget case I could see our custom events in the timeline when they are added by the widget with devtools, but when Element is reloaded I could not see any of those. I though that it was a synapse issue, but it was not, all widgets events could be loaded fine via relation to this parent event. |
|
Its a tricky topic, the spec doesn't clearly outline how non-standard relations should interact with threads, and getting this wrong means stuck notifications due to the client & server treating those events as contained in different timelines. Ideally the client would be told which thread/main timeline the event belongs to deterministically (either top level thread_id or a value in unsigned) matrix-org/matrix-spec-proposals#4023 would be such a solution |
Currently event with
m.referencerelation to unknown parent event is not added to the timeline and therefore is not feed into the widget.StopGapWidget.tschecks event to be in the timeline before the marker and if event is not found and marker is there then the event is ignored.We in the widgets rely on this setup that a room event relates to another parent room event via
m.referenceand if parent event is not loaded (that happens when timeline is relatively big and user have to scroll timeline manually to get event loaded) the event is ignored.Therefore this PR suggest to add this event to the timeline.
Checklist
Type: defect