fix(timeline): maintain aggregations when an event is deduplicated #4576

bnjbvr · 2025-01-22T19:35:28Z

Some context

An aggregation is an event that relates to another event: for instance, a
reaction, a poll response, and so on and so forth.

Some requirements

Because of the sync mechanisms and federation, it can happen that a related
event is received before receiving the event it relates to. Those events
must be accounted for, stashed somewhere, and reapplied later, if/when the
related-to event shows up.

In addition to that, a room's event cache can also decide to move events
around, in its own internal representation (likely because it ran into some
duplicate events, or it managed to decrypt a previously UTD event).
When that happens, a timeline opened on the given room
will see a removal then re-insertion of the given event. If that event was
the target of aggregations, then those aggregations must be re-applied when
the given event is reinserted.

Some solution

To satisfy both requirements, the [Aggregations] "manager" object provided
by this PR will take care of memoizing aggregations, for the entire
lifetime of the timeline (or until it's clear'd by some
caller). Aggregations are saved in memory, and have the same lifetime as
that of a timeline. This makes it possible to apply pending aggregations
to cater for the first use case, and to never lose any aggregations in the
second use case.

Some points for the reviewer

I think the most controversial point is that all aggregations are memoized for the entire lifetime of the timeline. Would that become an issue, we can get back to some incremental scheme, in the future: instead of memoizing aggregations for the entire lifetime of the timeline, we'd attach them to a single timeline item. When that item is removed, we'd put the aggregations back into a "pending" stash of aggregations. If the item is reinserted later, we could peek at the pending stash of aggregations, remove any that's in there, and reapply them to the reinserted event. This is what the first version of this patch did, in a much more adhoc way, for reactions only; based on the current PR, we could do the same in a simpler manner
while the PR has small commits, they don't quite make sense to review individually, I'm afraid, as I was trying to find a way to make a general system that would work not only for reactions, poll responses and ends. As a matter of fact, the first commits may have introduced code that is changed in subsequent commits, making the review a bit hazardous. Happy to have a live reviewing party over Element Call, if that helps, considering the size of the patch.
future work may include using the aggregations manager for edits too, leading to more code removal.

Part of #3280.

codecov · 2025-01-23T17:13:28Z

Codecov Report

Attention: Patch coverage is 79.26421% with 62 lines in your changes missing coverage. Please review.

Project coverage is 85.69%. Comparing base (ce44c6e) to head (4b26a48).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...rix-sdk-ui/src/timeline/controller/aggregations.rs	83.73%	20 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	71.87%	18 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	84.84%	10 Missing ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	61.53%	10 Missing ⚠️
...ix-sdk-ui/src/timeline/event_item/content/polls.rs	55.55%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4576      +/-   ##
==========================================
- Coverage   85.73%   85.69%   -0.05%     
==========================================
  Files         292      292              
  Lines       33492    33558      +66     
==========================================
+ Hits        28715    28756      +41     
- Misses       4777     4802      +25

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

… the event has been redacted

forever = the lifetime of a timeline.

…regation_redaction`

…TestTimeline`

…n happened

…ation

…een sent

…line reactions

…eItemContent::reactions()` I give up; returning a reference would be better to avoid plain cloning, but there's no way to make a Lazy reference work on wasm, because the map transitively uses a field that's not sync/send :( Which is sad, because the default value would be an empty map, and not contain any data that wouldn't be sync/send anyways.

andybalaam · 2025-02-07T09:29:27Z

A live review party sounds good to me, and I've asked some other members of the Rust team whether they'd like to join because I'm not sure I should look at this on my own.

andybalaam

I'd love to see some tests for the Aggregations manager, but looks good!

andybalaam · 2025-02-10T13:36:54Z

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

+        let content = self.inner.content().with_reactions(reactions);
+
+        // Do not use `Self::with_content` which may override the latest_edit_json.
+        // TODO: it's likely incorrect?


I don't understand this comment - can you expand on it? If it's incorrect, do we need to fix it before we can commit this?

Ah yeah, something I noticed, and fixed in another PR; I can remove the comment now, thanks for spotting it!

andybalaam · 2025-02-10T13:40:44Z

crates/matrix-sdk-ui/src/timeline/controller/aggregations.rs

+    ///
+    /// In case of error, returns an error detailing why the aggregation
+    /// couldn't be applied.
+    pub fn apply(&self, content: &mut TimelineItemContent) -> Result<bool, AggregationError> {


As discussed irl, returning an enum instead of the bool here would make it clearer, I think.

Turned out better than I expected! The code in apply/unapply is a bit uglier IMO, but that makes it much clearer for the callers. Thank you and @Hywan both for suggesting and pushing for this!

andybalaam · 2025-02-10T13:42:05Z

crates/matrix-sdk-ui/src/timeline/controller/aggregations.rs

+    /// In case of success, returns a boolean indicating whether unapplying the
+    /// aggregation caused a change in the content. If the aggregation could be
+    /// unapplied, but it didn't cause any change in the passed
+    /// [`TimelineItemContent`], `Ok(false)` will be returned.


Also here, an enum return value would help to understand.

andybalaam · 2025-02-10T13:46:45Z

crates/matrix-sdk-ui/src/timeline/controller/aggregations.rs

+    /// Mark a target event as being sent (i.e. it transitions from an local
+    /// transaction id to its remote event id counterpart), by updating the
+    /// internal mappings.
+    pub fn mark_target_as_sent(&mut self, txn_id: OwnedTransactionId, event_id: OwnedEventId) {


It would be good to have some unit tests for this.

Yep, will write a bunch in a subsequent PR!

andybalaam · 2025-02-10T13:52:24Z

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

            }
+        } else {
+            warn!("missing related-to item ({target:?}) for aggregation {aggregation_id:?}");


This is probably not a warning since it is fine to happen.

…atrix-org#4576) ## Some context An aggregation is an event that relates to another event: for instance, a reaction, a poll response, and so on and so forth. ## Some requirements Because of the sync mechanisms and federation, it can happen that a related event is received *before* receiving the event it relates to. Those events must be accounted for, stashed somewhere, and reapplied later, if/when the related-to event shows up. In addition to that, a room's event cache can also decide to move events around, in its own internal representation (likely because it ran into some duplicate events, or it managed to decrypt a previously UTD event). When that happens, a timeline opened on the given room will see a removal then re-insertion of the given event. If that event was the target of aggregations, then those aggregations must be re-applied when the given event is reinserted. ## Some solution To satisfy both requirements, the [`Aggregations`] "manager" object provided by this PR will take care of memoizing aggregations, **for the entire lifetime of the timeline** (or until it's clear'd by some caller). Aggregations are saved in memory, and have the same lifetime as that of a timeline. This makes it possible to apply pending aggregations to cater for the first use case, and to never lose any aggregations in the second use case. ## Some points for the reviewer - I think the most controversial point is that all aggregations are memoized for the entire lifetime of the timeline. Would that become an issue, we can get back to some incremental scheme, in the future: instead of memoizing aggregations for the entire lifetime of the timeline, we'd attach them to a single timeline item. When that item is removed, we'd put the aggregations back into a "pending" stash of aggregations. If the item is reinserted later, we could peek at the pending stash of aggregations, remove any that's in there, and reapply them to the reinserted event. This is what the [first version of this patch](matrix-org@ec64b9e) did, in a much more adhoc way, for reactions only; based on the current PR, we could do the same in a simpler manner - while the PR has small commits, they don't quite make sense to review individually, I'm afraid, as I was trying to find a way to make a general system that would work not only for reactions, poll responses and ends. As a matter of fact, the first commits may have introduced code that is changed in subsequent commits, making the review a bit hazardous. Happy to have a live reviewing party over Element Call, if that helps, considering the size of the patch. - future work may include using the aggregations manager for edits too, leading to more code removal.

bnjbvr force-pushed the bnjbvr/lost-reactions branch from 6c2393e to 140ab9a Compare January 23, 2025 16:58

bnjbvr force-pushed the bnjbvr/lost-reactions branch 4 times, most recently from e660de4 to 78d32b5 Compare February 5, 2025 17:51

bnjbvr force-pushed the bnjbvr/lost-reactions branch from 095a5ed to 3d7feaa Compare February 6, 2025 17:00

bnjbvr added 21 commits February 6, 2025 18:00

refactor(timeline): always unstash related pending reactions, even if…

9114f29

… the event has been redacted

fix(timeline): when removing an item, re-stash its associated reactions

9061977

refactor(timeline): keep poll aggregations around forever

99a5c36

forever = the lifetime of a timeline.

refactor(timeline): use TimelineEventItemId for aggregated items

2ffd947

refactor!(timeline): put the reactions into the TimelineItemContent

40c83f9

refactor(timeline): apply reactions as aggregations

73691b3

refactor(timeline): remove unused AnnotationKey

5d257b3

refactor(timeline): recall the id of the aggregated event

fad2bcf

refactor(timeline): apply reaction aggregation in place

e52d1bd

refactor(timeline): get rid of two uses of meta.reactions.map

8b88167

refactor(timeline): undo aggregations upon redaction

ba714d8

test(timeline): use the EventFactory for a reaction redaction

fbc6791

refactor(timeline): rename handle_reaction_redaction to `handle_agg…

51c7f77

…regation_redaction`

test(timeline): avoid creating new EventFactory and reuse that in `…

f4b27b3

…TestTimeline`

refactor(timeline): only replace an item when applying the aggregatio…

7fad47f

…n happened

refactor(timeline): store the ReactionStatus in the Reaction aggreg…

44083b0

…ation

refactor(timeline): maintain aggregation mappings after events have b…

0c13347

…een sent

test(timeline): use assert_let_timeout in one specific test of time…

8a48ec1

…line reactions

refactor(timeline): apply clippy lints

1e2d13e

doc(timeline): add doc and changelog for the new aggregation system

4fb6989

bnjbvr force-pushed the bnjbvr/lost-reactions branch from 3d7feaa to c54dbfe Compare February 6, 2025 17:00

bnjbvr marked this pull request as ready for review February 6, 2025 17:01

bnjbvr requested a review from a team as a code owner February 6, 2025 17:01

bnjbvr requested review from andybalaam and removed request for a team February 6, 2025 17:01

refactor(timeline): apply aggregations in a single place

da828c3

bnjbvr force-pushed the bnjbvr/lost-reactions branch from c54dbfe to da828c3 Compare February 6, 2025 17:04

andybalaam approved these changes Feb 10, 2025

View reviewed changes

bnjbvr added 4 commits February 10, 2025 15:52

some review comments

182f500

review: remove outdated comment

8d36423

review: demote log statement to info

2ebcc44

review: return an custom result type for Aggregations::apply/unapply

4b26a48

bnjbvr enabled auto-merge (squash) February 10, 2025 15:12

bnjbvr merged commit 9f2c572 into main Feb 10, 2025
41 checks passed

bnjbvr deleted the bnjbvr/lost-reactions branch February 10, 2025 15:38

zecakeh mentioned this pull request Feb 11, 2025

fix(ui): Demote aggregation target not found log to TRACE level #4654

Merged

bnjbvr mentioned this pull request Mar 20, 2025

timeline: finish the aggregations refactoring #4823

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(timeline): maintain aggregations when an event is deduplicated #4576

fix(timeline): maintain aggregations when an event is deduplicated #4576

Uh oh!

bnjbvr commented Jan 22, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 23, 2025 •

edited

Loading

Uh oh!

andybalaam commented Feb 7, 2025

Uh oh!

andybalaam left a comment

Uh oh!

andybalaam Feb 10, 2025

Uh oh!

bnjbvr Feb 10, 2025

Uh oh!

andybalaam Feb 10, 2025

Uh oh!

bnjbvr Feb 10, 2025

Uh oh!

andybalaam Feb 10, 2025

Uh oh!

andybalaam Feb 10, 2025

Uh oh!

bnjbvr Feb 10, 2025

Uh oh!

andybalaam Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

fix(timeline): maintain aggregations when an event is deduplicated #4576

fix(timeline): maintain aggregations when an event is deduplicated #4576

Uh oh!

Conversation

bnjbvr commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Some context

Some requirements

Some solution

Some points for the reviewer

Uh oh!

codecov bot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andybalaam commented Feb 7, 2025

Uh oh!

andybalaam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading