-
Notifications
You must be signed in to change notification settings - Fork 321
feat(threads): automatically subscribe a user to a thread under the semantics of MSC4306 #5462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5462 +/- ##
==========================================
+ Coverage 88.56% 88.59% +0.02%
==========================================
Files 340 340
Lines 93686 93780 +94
Branches 93686 93780 +94
==========================================
+ Hits 82975 83082 +107
+ Misses 6578 6565 -13
Partials 4133 4133 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5462 will not alter performanceComparing Summary
|
e95d796
to
8da211c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I don't understand all the details for the moment. I've left a couple of questions. Thanks for your contributions!
@@ -298,6 +303,8 @@ impl RoomPagination { | |||
}); | |||
} | |||
|
|||
subscribe_to_new_threads(&room, new_thread_subs).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here to explain why we call subscribe_to_new_threads
at this place please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading this, I don't understand why we subscribe to threads here :-].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "main" events timeline, which has threaded and non-threaded events. So when we're back-paginating, we might run into threaded for which no subscriptions have ever been computed (i.e. the threads existed before we created the subscription mechanism). In this case, we still want to be able to "catch up" and automatically subscribe the user.
/// [`push_context_for_threads_subscriptions`] function, available in the same | ||
/// module. | ||
#[derive(Debug, Default)] | ||
pub struct ThreadPushContext(Option<PushContext>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not type ThreadPushContext = Option<PushContext>
? What the benefits of having a new type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newtypes allow hiding the inner workings. I don't want users to pass a general ThreadPushContext
to the method that determines whether we should subscribe to a thread or not, because the push context shouldn't have the thread-subscription check. So it has to be a specific flavor of PushContext
to be passed in there, and I don't want the user to care about this; they can create a ThreadPushContext
with the other function, and not think too much about the implementation details.
d0f4d67
to
2376763
Compare
…its own function, and introduce a select! at the top level
…be sent, for extracting its thread root
d2c99cb
to
a682a46
Compare
Ready for another round of review 🥳 This also implements support for manually subscribing when an event is sent via the send queue, as per the semantics of MSC4306. PTAL 🥳 |
Based on #5455 + #5461, this implements automatic subscription to threads which explicitly mention the current user (via an intentional room or user mention), and likewise sends a manual subscription to a thread when replying into it.
There's a single background task, owned by the event cache, that will listen to two input streams, and create automatic thread subscriptions based on these streams.
RoomEventCacheGenericUpdate
stream). Based on the new events propagated in a linked chunk update, we can compute the notifications for those, and decide to subscribe to a thread, based on those.Sent
updates. We could embed the sent event in theSent
variant, as a simplification; otherwise, we keep just a bit of state, to link the to-be-sent event (transaction id) to its thread root.Topics to discuss:
Sent
could embed the sent event, which would avoid the mapping of to-be-sent event to a thread root.PR is a bit big, but mostly because of the tests; the core code isn't that big.
Part of #4869.