fix(ui): Introduce `Timeline` regions #5000

Hywan · 2025-05-05T09:13:02Z

This patch is twofold.

First part

This patch fixes the insertion of a new TimelineItem in the presence of a TimelineStart that shifts/offsets the timeline index of 1. It fixes #4976. It also contains a regression test.

Second part

Based on #4816 (review), I'm proposing a new concept in timeline::controller::ObservableItems: regions.

The ObservableItems holds all the invariants about the position of the items. It defines three regions where items can live:

the start region, which can only contain a single TimelineStart,
the remotes region, which can only contain many Remote timeline items with their decorations (only DateDividers and ReadMarkers),
the locals region, which can only contain many Local timeline items with their decorations (only DateDividers).

The iter_all_regions method allows to iterate over all regions. iter_remotes_region will restrict the iterator over the remotes region, and so on. These iterators provide the absolute indices of the items, so that it's harder to make mistakes when manipulating the indices of items with operations like insert, remove, replace etc.

Other methods like push_local or push_date_divider insert the items in the correct region, and check a couple of invariants. I've first introduced the invariants, then 12 tests were failing, which indicates that the TimelineStart could have been a problem in multiple situations. Moving the code to use iter_remotes_regions & siblings have fixed the tests, which indicates that this class of bugs has been fixed.

Review

It's better to review patch-by-patch. There are quite small and I hope easy to understand. The concept of regions removes an entire class of bugs in our Timeline, and I'm pretty happy with that 🙂.

This patch fixes the insertion of a new `TimelineItem` in the presence of a `TimelineStart` that shifts/offsets the timeline index of 1.

codecov · 2025-05-05T09:27:28Z

Codecov Report

Attention: Patch coverage is 81.25000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 85.85%. Comparing base (8be0a7d) to head (4b0a2e6).
Report is 52 commits behind head on main.

Files with missing lines	Patch %	Lines
...sdk-ui/src/timeline/controller/observable_items.rs	79.26%	17 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/date_dividers.rs	70.58%	5 Missing ⚠️
...dk-ui/src/timeline/controller/state_transaction.rs	75.00%	1 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5000      +/-   ##
==========================================
- Coverage   85.86%   85.85%   -0.01%     
==========================================
  Files         325      325              
  Lines       35851    35929      +78     
==========================================
+ Hits        30783    30848      +65     
- Misses       5068     5081      +13

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This patch improves the `assert_timeline_stream` macro by adding a bunch of assert messages in case it fails.

…line_stream!`.

This patch adds a regression test ensuring [this bug][4976] cannot happen anymore. [4976]: matrix-org#4976

…_missing`. This patch adds the `push_timeline_start_if_missing` method on `ObservableItemsTransaction` to add semantics and hardcode the invariant in a single place for the different timeline items.

This patch adds the `push_local` method on `ObservableItemsTransaction` to add semantics and hardcode the invariant in a single place for the different timeline items.

This patch implements the `has_local` method on `ObservableItemsTransaction`, which is way faster than the previous the previous solution which was to iterate over all items to find at least one local timeline item.

This patch defines a new concept in the `Timeline`: Regions. The `ObservableItems` holds all the invariants about the _position_ of the items. It defines three regions where items can live: 1. the _start_ region, which can only contain a single `TimelineStart`, 2. the _remotes_ region, which can only contain many `Remote` timeline items with their decorations (only `DateDivider`s and `ReadMarker`s), 3. the _locals_ region, which can only contain many `Local` timeline items with their decorations (only `DateDivider`s). The `iter_all_regions` method allows to iterate over all regions. `iter_remotes_region` will restrict the iterator over the _remotes_ region, and so on. These iterators provide the absolute indices of the items, so that it's harder to make mistakes when manipulating the indices of items with operations like `insert`, `remove`, `replace` etc. Other methods like `push_local` or `push_date_divider` insert the items in the correct region, and check a couple of invariants.

…egions. This patch updates `DateDividerAdjuster` to work on _remotes_ and _locals_ regions only, excluding the _start_ region. It helps to reduce the risk of inserting a `DateDivider` inside the _start_ region. This patch also uses the new `push_date_divider` method, which provides a couple of invariants.

This patch updates `ReadReceiptTimelineUpdate` to work on the _remotes_ region only, excluding the _start_ and the _lcoals_ regions. It helps to reduce the risk of inserting a `ReadMarker` inside the _start_ or the _locals_ regions.

This patch updates `TimelineMetadata` to work on the _remotes_ region only, excluding the _start_ and the _locals_ regions. It helps to reduce the risk of inserting items in an incorrect regions. This patch also removes on more `rfind_event_by_id` usage, which is nice.

… regions. This patch updates `TimelineStateTransaction` to work on the correct regions, _remotes_ in one place, and all regions in another place.

…id bugs. This patch updates `EventHandler` to use the correct regions where appropriate, thus reducing the complexity of the code, and removing classes of bugs. In the case of `Flow::Remote { position: TimelineItemPosition::At { … }}`, we no longer need to skip the local timeline items, and to handle the presence of the `TimelineStart` timeline item. The code is less complex. In the case of `Flow::Remote { position: TimelineItemPosition::End { … }}`, that's exactly the same at the previous case. In the case of `recycle_local_or_create_item`, the `try_fold` approach is replaced entirely with a simple `iter_locals_region`, reducing the size of the comments explaining the code, reducing the complexity of the code, and reducing the surface of bugs.

andybalaam

Really good change, thanks!

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

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

andybalaam · 2025-05-12T12:15:40Z

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

+    /// Return the index where to insert the first remote timeline
+    /// item.
+    pub fn first_remotes_region_index(&self) -> usize {
+        if self.items.get(0).is_some_and(|item| item.is_timeline_start()) {


It might be nice to have a method for this check too?

What check would you like to add?

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

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

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

fix(ui): Offset the timeline index in the presence of a TimelineStart.

c012d12

This patch fixes the insertion of a new `TimelineItem` in the presence of a `TimelineStart` that shifts/offsets the timeline index of 1.

Hywan added 3 commits May 6, 2025 10:16

test: Add assert messages in the assert_timeline_stream macro.

b3ca0a4

This patch improves the `assert_timeline_stream` macro by adding a bunch of assert messages in case it fails.

test(ui): Support index [$nth] --- date divider --- in `assert_time…

cfc50dc

…line_stream!`.

test(ui): Add a regression test.

3871ddf

This patch adds a regression test ensuring [this bug][4976] cannot happen anymore. [4976]: matrix-org#4976

Hywan changed the title ~~fix(ui): Offset the timeline index in the presence of a TimelineStart~~ fix(ui): Introduce Timeline regions May 7, 2025

Hywan added 10 commits May 7, 2025 17:41

refactor(ui): Add `ObservableItemsTransaction::push_timeline_start_if…

d30dfec

…_missing`. This patch adds the `push_timeline_start_if_missing` method on `ObservableItemsTransaction` to add semantics and hardcode the invariant in a single place for the different timeline items.

refactor(ui): Add ObservableItemsTransaction::push_local.

0b73f52

This patch adds the `push_local` method on `ObservableItemsTransaction` to add semantics and hardcode the invariant in a single place for the different timeline items.

refactor(ui): Add ObservableItemsTransaction::has_local.

a5d9eec

This patch implements the `has_local` method on `ObservableItemsTransaction`, which is way faster than the previous the previous solution which was to iterate over all items to find at least one local timeline item.

chore(base): Move the bitflags dependency in the workspace.

f8967b2

refactor(ui): TimelineStateTransaction works on _remotes_ and _all_…

52b8ac0

… regions. This patch updates `TimelineStateTransaction` to work on the correct regions, _remotes_ in one place, and all regions in another place.

Hywan force-pushed the fix-ui-timeline-insert-end-with-timelinestart branch from 466eee2 to 0122d11 Compare May 7, 2025 15:42

chore(ui): Make Clippy happy.

29ae0eb

Hywan force-pushed the fix-ui-timeline-insert-end-with-timelinestart branch from 4f88a34 to 29ae0eb Compare May 12, 2025 08:43

Hywan marked this pull request as ready for review May 12, 2025 08:56

Hywan requested a review from a team as a code owner May 12, 2025 08:56

Hywan requested review from andybalaam and removed request for a team May 12, 2025 08:56

doc(ui): Add matrix-org#5000 in the CHANGELOG.md.

d108829

andybalaam approved these changes May 12, 2025

View reviewed changes

Hywan added 2 commits May 12, 2025 16:54

test(ui): Add tests for push_local and push_date_divider.

b7149b0

doc(ui): Fix a typo in a comment.

4b0a2e6

Hywan merged commit 08aa9c8 into matrix-org:main May 12, 2025
43 checks passed

Hywan added a commit that referenced this pull request May 12, 2025

doc(ui): Add #5000 in the CHANGELOG.md.

d727111

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ui): Introduce `Timeline` regions #5000

fix(ui): Introduce `Timeline` regions #5000

Uh oh!

Hywan commented May 5, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 5, 2025 •

edited

Loading

Uh oh!

andybalaam left a comment

Uh oh!

Uh oh!

Uh oh!

andybalaam May 12, 2025

Uh oh!

Hywan May 12, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(ui): Introduce Timeline regions #5000

fix(ui): Introduce Timeline regions #5000

Uh oh!

Conversation

Hywan commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First part

Second part

Review

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

andybalaam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andybalaam May 12, 2025

Choose a reason for hiding this comment

Uh oh!

Hywan May 12, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(ui): Introduce `Timeline` regions #5000

fix(ui): Introduce `Timeline` regions #5000

Hywan commented May 5, 2025 •

edited

Loading

codecov bot commented May 5, 2025 •

edited

Loading

Hywan May 12, 2025 •

edited

Loading