feat(sdk): Live updates for pinned events timeline with replacements and redactions #3840

jmartinesp · 2024-08-14T11:49:14Z

This is a follow-up PR for #3870.

Changes

Make TimelineEventBuilder process synced events for TimelineFocus::PinnedEvents too.
Use the relationship cache added in sdk: add relationships cache to EventCache #3870 to load pinned events and other events related to them so their final timeline event results contain the changes those would add (edits, redactions, reactions, etc.).
Make Room::event_with_context also save its results to the cache.
Add new tests for pinned events and related events.
Fix the pinned events benchmark in room_bench.

The end result of this is the pinned events timeline now reacts to edits and redactions, and those values are kept even when the event list reloads.

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-08-14T12:04:54Z

Codecov Report

Attention: Patch coverage is 83.33333% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.08%. Comparing base (fb63c8d) to head (7cfcbae).
Report is 7 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk/src/event_cache/paginator.rs	28.57%	5 Missing ⚠️
...matrix-sdk-ui/src/timeline/pinned_events_loader.rs	89.47%	2 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	50.00%	1 Missing ⚠️
crates/matrix-sdk/src/event_cache/mod.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3840      +/-   ##
==========================================
+ Coverage   84.06%   84.08%   +0.01%     
==========================================
  Files         266      266              
  Lines       27779    27812      +33     
==========================================
+ Hits        23352    23385      +33     
  Misses       4427     4427

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Looks like you're on the right track! I'd like a few test changes, and the main logic in the EventCache itself isn't tested independently. Please add some tests for that too, since it's a different layer than the timeline.

Also, please reword the PR title to follow our conventional commit guidelines :)
Thanks!

bnjbvr · 2024-08-14T12:28:37Z

crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

+                if let Err(err) = semaphore.acquire().await {
+                    warn!("error when loading pinned event: {err}");
+                    return None;
+                }


Pretty sure this is a no-op when the semaphore could be acquired, because the guard (in the Ok() value) is not bound, so it's immediately dropped. Can you add a test for this? (I would even recommend making a separate PR for this feature, since it seems unrelated to the current PR's purpose, and could land separately.)

I have no idea how to test that, to be honest. Since there is no timeout I think the execution will just get stuck there until a new permit is available.

I replaced this with a buffered stream now. It seems to work just as fine and there is no need for the semaphore.

bnjbvr · 2024-08-14T12:29:17Z

crates/matrix-sdk/src/event_cache/mod.rs

@@ -396,6 +404,49 @@ impl EventCacheInner {
                continue;
            };

+            // Handle and cache events and relations
+            let mut relations = self.relations.write().await;


Unfortunately, this is not the right place to add this; I suppose this should be in the same place where I inserted events into Self::events. Follow the breadcrumbs :)

I think I'll have to do that in a separate method then? I don't think I can have 2 RwLockWriteGuards at the same time, one for all_events and another one for relations, so I don't think I'll be able to add this on append_events_locked_impl.

Oh, I just saw #3840 (comment).

bnjbvr · 2024-08-14T12:29:28Z

crates/matrix-sdk/src/event_cache/mod.rs

+            let mut events = self.events.write().await;
+            for ev in &joined_room_update.timeline.events {
+                if let Some(ev_id) = ev.event_id() {
+                    events.insert(ev_id, (room_id.clone(), ev.clone()));
+                }
+            }
+            drop(events);


This can entirely be removed, because it's already done a few layers below.

bnjbvr · 2024-08-14T12:30:36Z

crates/matrix-sdk/src/event_cache/mod.rs

@@ -501,6 +558,29 @@ impl RoomEventCache {
        None
    }

+    /// Try to find an event by id in this room, along with all relations.
+    pub async fn event_with_relations(&self, event_id: &EventId) -> Vec<SyncTimelineEvent> {


Can you return an (SyncTimelineEvent, Vec<SyncTimelineEvent>) instead, so it's clear where the target event is? (Right now there's nothing either in the signature or the doc-comment that makes it obvious it's the first one.)

bnjbvr · 2024-08-14T12:31:36Z

crates/matrix-sdk/src/event_cache/mod.rs

+    /// Try to find an event by id in this room, along with all relations.
+    pub async fn event_with_relations(&self, event_id: &EventId) -> Vec<SyncTimelineEvent> {
+        let relations = self.inner.relations.read().await;
+        let relation_ids = relations.get(event_id);


Can you inline this where it's used, instead? It's not useful to preload it, if the event couldn't be found.

bnjbvr · 2024-08-14T12:49:24Z

crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

+    test_helper.server.reset().await;
+
+    // Load new pinned event contents from sync, where $2 is and edit on $1
+    let _ = Box::pin(


Why is the Box::pin needed? Can you do without?

Oops, leftover from one of my tests to check why the tests were stuck.

bnjbvr · 2024-08-14T12:51:01Z

crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

+    .await;
+
+    // The list is reloaded, so it's reset
+    assert_matches!(timeline_stream.next().await.unwrap(), VectorDiff::Clear);


I think there's an assert_next that exists and could replace most of these assert_matches invokes here and below, and in the other test(s) too?

For some reason changing assert_matches!(timeline_stream.next().await.unwrap(), *) to assert_next_matches!(timeline_stream, *) makes one of the tests fail with:

assertion failed: stream is not ready, it's pending

Maybe I need to add a delay before? It's quite weird.

Oh, it's because under the hood it calls Stream::next_now_or_never... That seems dangerous. I don't see any similar function for doing it while awaiting for the next item.

We need to add a timeout then, otherwise if the stream item doesn't come immediately, then the test will be timing out with cargo-nextest's delay, which is a minute or longer.

Added a new assert_next_matches_with_timeout! macro for this. It's based on the assert_next_matches, but instead of checking the immediate result of the stream, it adds a small timeout which can be customised: b1fe70a

bnjbvr · 2024-08-14T12:52:21Z

crates/matrix-sdk/src/event_cache/mod.rs

+            relations
+                .insert(orig_ev_id.to_owned(), BTreeSet::from_iter(vec![related_ev_id.to_owned()]));
+        }
+        drop(relations);


nit: no need to drop explicitly, it'll happen automatically at the end of the function's body.

Yep, copy&paste issue. It's gone now.

bnjbvr · 2024-08-14T12:52:39Z

crates/matrix-sdk/src/event_cache/mod.rs

@@ -496,6 +504,18 @@ impl EventCacheInner {
            }
        }
    }
+
+    async fn insert_or_update_relationship(&self, orig_ev_id: &EventId, related_ev_id: &EventId) {


nit: please fully spell out event_id for consistency, and original too

(I for one would call these two parameters original and related, because including the parameter's type is spurious, since the parameter type is next to the variable name, but that's my preference :))

I'm removing this code so it shouldn't be needed anymore.

bnjbvr · 2024-08-14T12:54:12Z

benchmarks/benches/room_bench.rs

@@ -186,6 +187,8 @@ pub fn load_pinned_events_benchmark(c: &mut Criterion) {

    group.bench_function(BenchmarkId::new("load_pinned_events", name), |b| {
        b.to_async(&runtime).iter(|| async {
+            client.event_cache().subscribe().unwrap();


Is there really a need to do it for each iteration of the benchmark? I don't think so, it should be subscribed to once and for all.

It seems like client.event_cache().subscribe().unwrap(); does some async operations, and the benchmark panics with:

there is no reactor running, must be called from the context of a Tokio 1.x runtime

Maybe I can wrap Client in an Arc and use runtime.spawn_blocking to run that code? Is there a better solution?

Tried fixing this in 807c875, please let me know if there's a better solution.

jmartinesp · 2024-08-20T09:55:04Z

So, funny thing: it seems like when the devs reported /event didn't return the sender info it was a mistake. So I'm wondering if I should go back to using /event for loading the pinned events since it was faster...

jmartinesp · 2024-08-21T12:38:17Z

Ok, so I re-used this PR for holding only the part of the code related to the live updates to the pinned events timeline. It depends on a previous PR being merged and it'll have a following PR containing the semaphore/buffered futures changes.

jmartinesp · 2024-08-22T13:38:51Z

The PR this one depended on has been merged and this one has been rebased, so it's ready to review again.

jmartinesp · 2024-08-23T08:16:33Z

Well, apparently #3879 just broke this PR, another rebase will be needed.

EDIT: hoping 7e51a78 fixes it 🤞 .

Hywan

We are almost good!

Hywan · 2024-08-23T09:33:59Z

crates/matrix-sdk/src/event_cache/mod.rs

+    // This doesn't insert the event into the linked chunk. In the future there'll
+    // be no distinction between the linked chunk and the separate cache.


Why?
Can we add a TODO and open an issue to track this please?

The comment is just a copy of the one above it written by @bnjbvr since it follows the same logic, but I think it's probably because we lack the context of where the LinkedChunk containing events to save would go, or if it even makes sense to add the events to one (these events can come from very different sources and may not be related enough to be added as a chunk of a timeline, if I understood how LinkedChunk works properly).

I guess the 'in the future' part means once we have a persistent event cache, but I'm not 100% sure.

Anyway, can create an issue for that.

@Hywan I added the TODO here and opened the issue here.

Excellent, thanks!

Hywan

Good to me, thanks! Please rebase and you can merge.

…ler`

…::PinnedEvents`

This way any reactions/redactions/edits, etc. will be taken into account when building the timeline event.

This commit contains a new `assert_next_matches_with_timeout!` macro that will take a `Stream` and wait for a little while until its next item is ready, then match it to a pattern.

…g the same `RwLock` for the cache, which should be faster

…::event`, save its results to the event cache too.

jmartinesp requested a review from a team as a code owner August 14, 2024 11:49

jmartinesp requested review from Hywan and removed request for a team August 14, 2024 11:49

jmartinesp force-pushed the jme/update-pinned-events branch from 664fb74 to 00199d9 Compare August 14, 2024 12:14

bnjbvr requested review from bnjbvr and removed request for Hywan August 14, 2024 12:25

bnjbvr requested changes Aug 14, 2024

View reviewed changes

jmartinesp changed the title ~~Live update pinned events timeline with replacements and redactions~~ feat(sdk): Live updates for pinned events timeline with replacements and redactions Aug 14, 2024

jmartinesp force-pushed the jme/update-pinned-events branch from d9880bd to b4f147f Compare August 20, 2024 08:46

jmartinesp requested a review from bnjbvr August 20, 2024 09:58

jmartinesp force-pushed the jme/update-pinned-events branch from a1d1a90 to b93ed11 Compare August 20, 2024 11:22

jmartinesp marked this pull request as draft August 21, 2024 09:09

jmartinesp force-pushed the jme/update-pinned-events branch from b93ed11 to 7f97fba Compare August 21, 2024 12:31

jmartinesp changed the base branch from main to feat/jme/add-event-relationships-cache August 21, 2024 12:32

jmartinesp force-pushed the jme/update-pinned-events branch from 7f97fba to 335f94b Compare August 21, 2024 12:37

jmartinesp marked this pull request as ready for review August 21, 2024 12:38

jmartinesp force-pushed the jme/update-pinned-events branch from 335f94b to c86c842 Compare August 21, 2024 13:00

jmartinesp mentioned this pull request Aug 22, 2024

pinned events: limit the max amount of concurrent requests to load pinned events #3878

Merged

1 task

Base automatically changed from feat/jme/add-event-relationships-cache to main August 22, 2024 12:24

jmartinesp force-pushed the jme/update-pinned-events branch from c86c842 to b8f24bd Compare August 22, 2024 12:51

jmartinesp force-pushed the jme/update-pinned-events branch from b8f24bd to b6f4748 Compare August 23, 2024 08:15

Hywan requested changes Aug 23, 2024

View reviewed changes

jmartinesp requested a review from Hywan August 23, 2024 10:12

Hywan approved these changes Aug 23, 2024

View reviewed changes

sdk-ui: handle events related to a pinned event in `TimelineEventHand…

74be61c

…ler`

jmartinesp added 6 commits August 23, 2024 13:58

sdk-ui: enable handling new synced timeline events for `TimelineFocus…

8896690

…::PinnedEvents`

sdk-ui: load pinned events with their related events

31e0f9d

This way any reactions/redactions/edits, etc. will be taken into account when building the timeline event.

sdk-ui: fix and add new pinned events tests.

4aa5ef7

This commit contains a new `assert_next_matches_with_timeout!` macro that will take a `Stream` and wait for a little while until its next item is ready, then match it to a pattern.

sdk: add fn RoomEventCache::save_events to save several events usin…

ff67a34

…g the same `RwLock` for the cache, which should be faster

sdk: add RequestConfig to Room::event_with_context to match `Room…

7ad7e04

…::event`, save its results to the event cache too.

sdk-ui: fix pinned events benchmark

7cfcbae

jmartinesp force-pushed the jme/update-pinned-events branch from 4f57f54 to 7cfcbae Compare August 23, 2024 11:58

jmartinesp merged commit c78639d into main Aug 23, 2024
38 checks passed

jmartinesp deleted the jme/update-pinned-events branch August 23, 2024 12:12

		// This doesn't insert the event into the linked chunk. In the future there'll
		// be no distinction between the linked chunk and the separate cache.

feat(sdk): Live updates for pinned events timeline with replacements and redactions #3840

feat(sdk): Live updates for pinned events timeline with replacements and redactions #3840

Conversation

jmartinesp commented Aug 14, 2024 • edited Loading

Changes

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Aug 20, 2024

jmartinesp commented Aug 21, 2024

jmartinesp commented Aug 22, 2024

jmartinesp commented Aug 23, 2024 • edited Loading

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan left a comment

Choose a reason for hiding this comment

jmartinesp commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

jmartinesp Aug 20, 2024 •

edited

Loading

jmartinesp commented Aug 23, 2024 •

edited

Loading

jmartinesp Aug 23, 2024 •

edited

Loading