fix(timeline): Don't add events to the pinned events timeline on paginations/syncs

[jmartinesp]

This will cause index out of bounds errors in some scenarios, and the events that do belong in this timeline would already have been loaded before.

• Public API changes documented in changelogs (optional)

Signed-off-by:

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.73%. Comparing base (83dd11e) to head (fd8f90e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4645   +/-   ##
=======================================
  Coverage   85.72%   85.73%           
=======================================
  Files         292      292           
  Lines       33492    33493    +1     
=======================================
+ Hits        28712    28714    +2     
+ Misses       4780     4779    -1     

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

[jmartinesp]

The broken tests were fixed to reflect the new behaviour, so it should be ready for review now.

[bnjbvr]

I'll steal this review. We need to think a bit more about the implications of this change, this may break aggregations targeting pinned events received in real-time, among other things (although I see you've opened an issue that they are not received :|).

[bnjbvr]

The broken tests were fixed to reflect the new behaviour

Sorry, I'm not following here. Can you explain why some tests have been entirely removed, please?

[jmartinesp]

The broken tests were fixed to reflect the new behaviour

Sorry, I'm not following here. Can you explain why some tests have been entirely removed, please?

test_new_pinned_events_are_added_on_sync was removed by mistake, since we could just invert its behaviour to make sure those events aren't added on sync (although the other edit, redact tests also test this indirectly). For test_edited_events_survive_pinned_event_ids_change, since we already test that edits aren't taken into account in another test I don't see why we'd have to test this behaviour again.

Also, I'm not against having the live updates for edits/redactions in the pinned event timeline. I just don't know how to properly make it happen anymore and I'm not sure I have the bandwidth to work on this if the answer turns out to be quite complex.

[bnjbvr]

Thanks for the explanation about the test removal. I agree that if we could keep the test, and invert its semantics (what it's checking), it would be super nice. The fix sounds good for the time being, and we should try to think about it a bit more thoroughly as a group, but let's not have this PR wait for this to happen before getting merged. Thank you for the patch!

[bnjbvr]

Having thought about it: i think we want to react to room event cache timeline updates if and only if this is a live timeline:

• a pinned events timeline handles its events by itself
• ditto for the permalink timeline
• a future focus like "threads" would likely also manage its events by itself

We need to find a more general solution to this problem, so as to be able to handle sync events live for those timeline focus, but, in the meanwhile, let's disable it entirely with a condition like if !is_live { continue ; }, and a comment similar (but more general) to what you've written.

cc @Hywan

[jmartinesp]

I'm trying adding the suggested change, but that seems to cause the timeline::focus_event::test_focused_timeline_reacts test to never finish. Wouldn't this mean it's breaking reactions in the focused event timeline?