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

Feeds event with relation to unknown to the widget #12283

Merged

Conversation

maheichyk
Copy link
Contributor

@maheichyk maheichyk commented Feb 23, 2024

Events that relate to unknown parent are not added to the timeline and therefore are not passed to the widget. Room timeline is used in StopGapWidget to check if event is before the marker in order to pass it.

This PR suggests to ignore marker timeline logic for these events.

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)

Type: defect

if (!isRelationToUnknown) {
// Ignore the event: it is before our interest.
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this seems reasonable, although maybe we should default to sending the event on if we can't find either the event or the receipt, rather than special-casing events whose parent we can't find? (ie. rename isBeforeMark to shouldForward or something, set it to true to start with and set it to false in the first clause rather than the second... and comment it, obviously).

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 have changed isBeforeMark to shouldForward that makes easier to understand this logic, but I can’t follow on the suggestion to change:

although maybe we should default to sending the event on if we can't find either the event or the receipt, rather than special-casing events whose parent we can't find

It not looks clear to me how event or the receipt find could resolve the same issue. Could you please explain more exactly what you mean with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbkr could you please have a look on the changes?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I was just suggesting that, since the problem here is that we can't prove that the event is after the read marker because we can't find the event, we could also just default to forwarding if we can't prove the event hasn't been read, which I think would probably be safe enough. That said, this is a smaller change, so it seems fine.

@dbkr dbkr added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 27, 2024
Signed-off-by: Mikhail Aheichyk <[email protected]>
@dbkr dbkr added this pull request to the merge queue Feb 29, 2024
Merged via the queue into matrix-org:develop with commit 71cece7 Feb 29, 2024
19 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants