-
Notifications
You must be signed in to change notification settings - Fork 357
feat(ui): Add the new latest_event room list sorter, and update the recency sorter
#5617
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5617 +/- ##
==========================================
+ Coverage 88.89% 88.93% +0.04%
==========================================
Files 349 350 +1
Lines 96546 96854 +308
Branches 96546 96854 +308
==========================================
+ Hits 85826 86140 +314
+ Misses 6682 6676 -6
Partials 4038 4038 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5617 will not alter performanceComparing Summary
|
5800753 to
d163d1c
Compare
poljar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of it makes sense and thanks for the comprehensive test suite.
I left some questions and recommendations that I think make sense.
crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs
Outdated
Show resolved
Hide resolved
crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs
Outdated
Show resolved
Hide resolved
| /// Get the timestamp of the [`LatestEventValue`]. | ||
| /// | ||
| /// If it's [`None`], it returns `None`. If it's [`Remote`], it returns the | ||
| /// `origin_server_ts`. If it's [`LocalIsSending`] or [`LocalCannotBeSent`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in chat but the discussion died down, so I'll repeat here.
To avoid malicious homeservers and users manipulating our room ordering using the origin_server_ts can't we record a local timestamp whenever we create a new LatestEventValue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I've been thinking about that.
When we receive an event via the sync, we can record the now() as the local timestamp. However, it means that all events from all rooms for a particular sync will have the same local timestamp, and thus, we can't use that to sort the rooms (because all “recorded timestamps” will be equal).
I think we have to keep origin_server_ts for the moment. We can warn it's a known limitations. It's also used to display the event's time in the timeline for example. If we can't trust this value, it's a protocol issue I suppose. I don't see how to solve that for the moment.
f09d0ea to
127f677
Compare
poljar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, looks good.
There's still the question of using origin_server_ts exclusively for this, but as you said, this can be a separate PR.
This patch implements the new `latest_event` sorter for the room list which puts the local latest events before the other kinds (like `Remote` or `None`).
This patch implements a `LatestEventValue::timestamp` method to fetch the timestamp of a latest event value.
This patch updates the `recency` sorter of the room list to rely on the `LatestEventValue`'s timestamp, or on the `bump_stamp` returned by the sync. Using the `LatestEventValue`'s timestamp is more reliable as we don't rely on the server. However, we must be careful to compare values of the same nature because the timetamp from the `LatestEventValue` and the `bump_stamp` doesn't represent the same thing! The `bump_stamp` is only used when the value for the `LatestEventValue` is `None`. It's a compromise to get a more accurate listing. Though, `LatestEventValue::timestamp` returns the `origin_server_ts` value, which can be forged by a malicious user (then a room could be _sticked_ at the top or at the bottom of the room list). Note that this problem already existed in the past before the server computed a `bump_stamp`. Also note that some homeservers use the `origin_server_ts` as the `bump_stamp` value. Anyway, it's not a security risk as far as I know.
This patch installs the `new_sorter_latest_event` in the room list.
This patch adds the `RoomRecencyStamp` type to avoid confusion with other `u64` values.
…orters`. This patch renames all the structure `*Matcher` to `*Sorter`. And their `matches` method become `cmp`. It was copy-pasted from `room_list_service::filters` probably, but the semantics here are not _matcher_ but _sorter_. It's more consistent that `cmp` returns an `Ordering`.
This patch adds the notion of _rank_ in the `recency` sorter to avoid confusion around `u64`: is it a timestamp or a recency stamp? It's purely semantics, but I hope it clarify the code.
This patch removes intermediate structs and uses a function directly.
This patch removes intermediate structs and uses a function directly.
590c075 to
d9db53e
Compare
This set of patches updates the room list to sort rooms by their “latest event”. What does it mean?
recency_stamp(also known as thebump_stampin sliding sync terminology):recency_stampinaccurate? Because the server might bump a room based on certain events, so the room will bump in the room list, but the latest event won't change because the event causing the bump is not a latest event candidate. Because the latest event is used by som apps to display the room “last updated time” in the room, the user might see unordered rooms!origin_server_rsvalue, which is a mistake because it can be forged by malicious servers! Yes, that's true, but:origin_server_tsvalue of the last event in the room as the value forbump_stamp. We can't control this, it's an opaque value.All-in-all, this set of patches creates a new
latest_eventsorter for the room list. Then it updates therecencysorter to be “latest-event aware”. Finally, it installs the newlatest_eventsorter.