Fix timeline beacon info replacement path by ganfra · Pull Request #6596 · matrix-org/matrix-rust-sdk

ganfra · 2026-05-20T13:09:58Z

This updates the timeline action flow to support multi-action event handling and fixes the m.beacon_info replacement path.

Replacing a live m.beacon_info session now correctly adds the new timeline item while marking the previous one as stopped.

Fix https://github.com/element-hq/element-internal/issues/714

I've documented the public API changes in the appropriate changelog files
(see Writing changelog entries).
This PR was made with the help of AI.

Signed-off-by:

codecov · 2026-05-20T13:21:19Z

Codecov Report

❌ Patch coverage is 90.60773% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.97%. Comparing base (306b00e) to head (89d59e7).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/event_handler.rs	94.73%	5 Missing and 1 partial ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	60.00%	6 Missing ⚠️
...dk-ui/src/timeline/controller/state_transaction.rs	90.38%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6596      +/-   ##
==========================================
+ Coverage   89.94%   89.97%   +0.02%     
==========================================
  Files         382      382              
  Lines      108012   108107      +95     
  Branches   108012   108107      +95     
==========================================
+ Hits        97152    97267     +115     
+ Misses       7188     7171      -17     
+ Partials     3672     3669       -3

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

codspeed-hq · 2026-05-20T13:34:18Z

Merging this PR will not alter performance

✅ 50 untouched benchmarks

_{Comparing ganfra/fix_timeline_beacon_replaces (89d59e7) with main (72d2157)}

andybalaam

Thank you for working on this! I am not very familiar with this code, so it took me a long time to work out what it was doing. It would help me if it used smaller commits, and gave more detail in the commit descriptions. E.g.:

Make TimelineEventHandler hold TimelineEventContext as a reference
Return a Vec from TimelineAction::from_event so that (later) we can return multiple actions to stop and start beacons
Hold own_id inside BeaconStop so that ... SOMETHING? ...
Simplify TimelineEventHandler::handle_event by factoring out methods
Stop an existing live beacon if we receive a new one

I also have 3 more general questions:

With this implementation does the stop item appear in the timeline below the new one? Is that OK, or should it appear above?
Are we certain of the order in which we receive these two beacon events? Could some clients see them in different orders?
(Probably for @poljar): do we think that adding 2 timeline items for a single beacon, simulating a stop of the old one, and the creation of the new one? I.e. is the timeline the right place to do this? Or e.g. should the underlying code simply understand a new beacon as implicitly meaning the old one is stopped?

andybalaam · 2026-05-21T09:32:44Z

+        let has_add_item =
+            timeline_actions.iter().any(|action| matches!(action, TimelineAction::AddItem { .. }));
+        let should_add_new_items = has_add_item
+            && matches!(position, TimelineItemPosition::UpdateAt { .. })


Why is there a repeat of matches! here and the next line?

Not sure to follow the question, this is not the same match?

There are 2 lines that do:

&& matches!(position, TimelineItemPosition::UpdateAt { .. })

so I think the first one is useless - it is just the same as the second one, but without the trailing if.

Ah yes sorry, I might have messed up with git

andybalaam · 2026-05-21T09:32:59Z

+        // working from `handle_remote_aggregations()`.
+        let has_add_item =
+            timeline_actions.iter().any(|action| matches!(action, TimelineAction::AddItem { .. }));
+        let should_add_new_items = has_add_item


Can you add a comment explaining what this does please?

Something like (please correct if wrong): "If we are adding an item at a position that currently contains a UTD, we should add new items."

andybalaam · 2026-05-21T09:36:36Z

+                == 1;
+
+            if recycled_timeline_id.is_some() && !can_recycle_timeline_id {
+                warn!("Multiple TimelineAction::AddItem detected, can't recycle timeline id");


This doesn't seem like a warning to me. Isn't it quite normal?

Honestly I'm not sure about this recycled_timeline_id thing, I'd need eye from rust expert

Yeah, this would happen every time a beacon is received, so a trace!() or at best debug!() call should be fine.

andybalaam · 2026-05-21T09:37:28Z

+                        None
+                    }
+                };
+                item_added |= TimelineEventHandler::new(self, &ctx)


Should this be ||= ?

I don't think it exists? But I might be wrong

Yeah you're right, but |= is bitwise OR, which is not right, so you should do

item_added = TimelineEventHandler::new(self, &ctx) || item_added

(note the order, so that the ::new always gets called, even if item_added is true.

andybalaam · 2026-05-21T09:38:48Z

                },
                should_add_new_items: should_add,
            };
+            let can_recycle_timeline_id = timeline_actions


So we only recycle the ID when there is exactly 1 add. Why can't we recycle it for the first add, even if there are more?

@poljar do you have an idea how to manage this properly? For now we are not getting more than one TimelineAction::AddItem

That's probably correct, turning off the beacon should be able to reuse the ID, while the addition of the new beacon should create a new one.

Though I don't think it's worth the complexity and it would really special case the logic for the beacon updates.

Perhaps we can simplify this even further instead, to disallow the recycling as soon as there's multiple actions:

let recycled_timeline_id = recycled_timeline_id.take_if(timeline_actions.len() == 1);

We should also extensively comment on why we're doing this.

andybalaam · 2026-05-21T09:52:45Z

+                Ok(None)
+            }
+            [_, _, ..] => {
+                warn!("Multiple actions for an embedded event");


So for this first commit, we never actually return >1 action, right?

If we should never get to this code, it would be good to explain why in a comment here.

andybalaam · 2026-05-21T09:53:29Z


        let timeline_action = TimelineAction::from_content(content, in_reply_to, thread_root, None);
-        TimelineEventHandler::new(&mut txn, ctx)
+        TimelineEventHandler::new(&mut txn, &ctx)


Changing this to a reference would have made a very nice, small, understandable separate commit.

andybalaam · 2026-05-21T09:56:20Z

 }

+/// A new live `beacon_info` replacing a previous live session should implicitly
+/// stop the previous timeline item, even if no explicit stop event was sent.


This test description would make a good commit message. I find it much easier to understand than the current commit message.

andybalaam · 2026-05-21T09:59:31Z

+                                );
+                                Self::HandleAggregation {
+                                    related_event: ev.event_id.clone(),
+                                    kind: HandleAggregationKind::BeaconStop {


Will this stop the previous beacon even if it was already stopped?

Thats a good point, I guess it'll try but won't find a live version matching

I don't think we should add a stop timeline item every time: we should check whether there is a live beacon and only add a stop timeline item if so.

The code here is not adding a stop item, it's mutating another item. If the item is not found then it does nothing.

andybalaam · 2026-05-21T09:59:59Z

+                                }
+                            });
+                            let mut actions = vec![add_item_action];
+                            actions.extend(handle_aggregation_action);


Does this add a None to actions if the beacon has no prev_content?

I don't think so, AFAIK it should just ignore the None

What should ignore the None? This line of code will certainly add an None if handle_aggregation_action is None.

The Option is passed as an Iterator and should ignore None if I'm not mistaken

Ah, I missed that you were using extend - sorry, you're right.

ganfra · 2026-05-21T13:51:24Z

Thanks for the initial review. The PR was created for further discussion, especially with @poljar as we initially started to talk about this in the Live Location Sharing room.

With this implementation does the stop item appear in the timeline below the new one? Is that OK, or should it appear above?

So this code won't add a new stop item by itself, it'll only makes sure the previous live is correctly seen as stop when loaded. If the previous live is not loaded, it will just store the aggregation in memory waiting for the prev event to be loaded.

My response should probably answer your other questions too ^

I will leave this with poljar

poljar

I think @andybalaam raised some great questions and suggestions.

Beside his remarks, what furthermore worries me is that no test has raised a fuss on the |= mistake on the item_added update logic.

ganfra · 2026-06-09T15:46:45Z

Beside his remarks, what furthermore worries me is that no test has raised a fuss on the |= mistake on the item_added update logic.

Regarding |= mistake and according to https://doc.rust-lang.org/std/primitive.bool.html#basic-usage , |= usage on a bool is working?

ganfra · 2026-06-10T18:01:24Z

@poljar I've addressed most of the remarks I think. I'll rewrite the history if you prefer, but when the PR will be ready to be approved.

On the UTD hack thing, I'm wondering if it's still necessary, as the linked issue seems closed.

poljar · 2026-06-25T10:27:21Z

Regarding |= mistake and according to https://doc.rust-lang.org/std/primitive.bool.html#basic-usage , |= usage on a bool is working?

Right, I latched on to the conclusion in this thread, which indeed is wrong. |= will work.

@poljar I've addressed most of the remarks I think. I'll rewrite the history if you prefer, but when the PR will be ready to be approved.

Feel free to rewrite the history now.

On the UTD hack thing, I'm wondering if it's still necessary, as the linked issue seems closed.

There are tests for this so we can try to remove the hack, though I'm not sure if that'll work as we still need to filter out all of the AddItem calls.

poljar

I think this is fine now.

Could you just handle the lint issue. Either add a clippy exception, or even better add a struct for the a function arguments.

ganfra requested a review from a team as a code owner May 20, 2026 13:09

ganfra requested review from andybalaam and poljar and removed request for a team May 20, 2026 13:09

andybalaam previously requested changes May 21, 2026

View reviewed changes

andybalaam requested review from andybalaam and removed request for andybalaam May 21, 2026 13:57

andybalaam requested review from andybalaam and removed request for andybalaam May 29, 2026 10:40

poljar requested changes May 29, 2026

View reviewed changes

ganfra added 2 commits June 9, 2026 09:33

Allow TimelineAction::from_event to return Vector instead of Option

fd8b71e

fix(matrix-sdk-ui) : use prev_content to replace beacon_info

fc11621

ganfra added 2 commits June 10, 2026 18:00

fix(timeline): simplify recycled ID and UTD hackjob logic

f3d4dd6

fix(timeline): clarify multiple-action warn messages

a5e515c

ganfra force-pushed the ganfra/fix_timeline_beacon_replaces branch from b14e40f to a5e515c Compare June 10, 2026 16:10

fix(timeline): minor formatting cleanups

89d59e7

ganfra requested a review from poljar June 10, 2026 17:59

poljar approved these changes Jun 25, 2026

View reviewed changes

Uh oh!

Conversation

ganfra commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codspeed-hq Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

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!

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!

andybalaam May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

ganfra commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading

codspeed-hq Bot commented May 20, 2026 •

edited

Loading

andybalaam May 21, 2026 •

edited

Loading

ganfra commented May 21, 2026 •

edited

Loading