-
Notifications
You must be signed in to change notification settings - Fork 405
Add experimental support for MSC4360: Sliding Sync Threads Extension #19005
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: develop
Are you sure you want to change the base?
Conversation
) -> JsonDict: | ||
"""Get related events of a event, ordered by topological ordering. | ||
TODO Accept a PaginationConfig instead of individual pagination parameters. |
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 has been done already, the comment just wasn't updated.
# Check if this event is part of a thread | ||
relates_to = event.content.get("m.relates_to") | ||
if not isinstance(relates_to, dict): | ||
continue | ||
|
||
rel_type = relates_to.get("rel_type") | ||
|
||
# If this is a thread reply, track the thread | ||
if rel_type == RelationTypes.THREAD: | ||
thread_id = relates_to.get("event_id") |
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.
We don't have a helper for this kind of thing? Feels like something we'd be doing elsewhere already and could be wrapped up as a utility
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.
I didn't see it anywhere, but I could take another look and add it as a helper where appropriate.
if room_result.timeline_events: | ||
for event in room_result.timeline_events: | ||
# Check if this event is part of a thread | ||
relates_to = event.content.get("m.relates_to") |
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.
Probably should add these to EventContentFields
(applies to fields below as well)
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.
Done - take a look & make sure it's as you expected.
# Skip this thread if it already has events in the room timeline | ||
# (unless include_roots=True, in which case we always include it) | ||
if (update.room_id, update.thread_id) in threads_in_timeline: | ||
continue |
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 we structure things to avoid fetching bundled aggregations for thread_root_events
if we end up skipping them anyway?
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 should be restructured as you described now.
time_now = self.clock.time_msec() | ||
response_content = await self.encode_response( | ||
requester, sliding_sync_results, time_now | ||
) |
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.
Good call on using the same time_now
across all of the serialization that needs it 👍
(consistency)
original_event_id: ( | ||
edits.get(edit_id) | ||
if (edit_id := edit_ids.get(original_event_id)) | ||
else None | ||
) |
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 functionally pretty much the same right? Just making this safer?
The difference being that we won't lookup edits.get(None)
where we wouldn't expect there to be a entry with the None
key but possible.
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.
Correct. (And to appease the linter)
We do this pattern elsewhere, this just uses the funky python syntax of inline variable assignment inside an if
FROM local_current_membership AS lcm | ||
WHERE lcm.room_id = threads.room_id | ||
AND lcm.user_id = ? | ||
AND lcm.membership = ? |
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.
We should be using the membership from what we already calculated within the token range. See synapse/handlers/sliding_sync/room_lists.py
for where we originally calculate that.
async def get_thread_updates_for_user( | ||
self, | ||
*, | ||
user_id: str, | ||
from_token: Optional[RoomStreamToken] = None, | ||
to_token: Optional[RoomStreamToken] = None, | ||
limit: int = 5, | ||
include_thread_roots: bool = False, | ||
) -> Tuple[Sequence[ThreadUpdateInfo], Optional[StreamToken]]: | ||
"""Get a list of updated threads, ordered by stream ordering of their | ||
latest reply, filtered to only include threads in rooms where the user | ||
is currently joined. |
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.
Feels like this should be taking in the list of rooms that are being returned in the Sliding Sync response and only returning updates for those.
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.
I thought about that, and even tried implementing it.
The problem becomes what should we return as a token that is usable on the /thread_updates
endpoint?
I assume we would need to follow the same pattern as the receipts extension and handle initial, live, and previously rooms separately. The options for what token to return for pagination would be either the oldest from the three sets, or the newest from the three sets.
In the case of the oldest, further pagination could result in missing updates between the oldest token and the tokens from the other two sets.
In the case of the newest, further pagination could result in receiving duplicate entries.
Then there is the issue of how would you associate the set of rooms/lists with the token such that using it for pagination on /thread_updates
makes sense. And would using the new endpoint update the sliding sync connection state? What if using different rooms/lists on the new endpoint than in sliding sync?
It's not that we couldn't come up with solutions to these things. I just felt they quickly over complicated things.
The overall intention was to just be able to track new events in threads easily and separately from the entire set of events to ensure clients can manage a threads notification center without having to paginate through all of a user's event history since the last time they logged in.
Maybe it's worth the complication to add room/list filtering, but I'm not convinced after having tried to implement it.
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.
We could implement a bulk
/messages
endpoint, where the client would specify multiple rooms andprev_batch
tokens.
Per the above, I think we would probably want to use all of the prev_batch
tokens from all of the rooms timeline
. Perhaps clunky, but maybe good.
And this also gets around the problem where you can't get to the thread updates that you care about without paginating through the bulk. This way, you can specify the rooms you want to get more thread updates in.
# Helper function to extract StreamToken from either StreamToken or SlidingSyncStreamToken format | ||
def extract_stream_token(token_str: str) -> str: |
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.
Where is this used? Where are we using a SlidingSyncStreamToken
for another endpoint?
Just trying to understand the landscape as I think we eventually need this kind of thing (matrix-org/matrix-spec-proposals#4186 (comment))
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.
It gets used in the /thread_updates
companion endpiont #19041
I agree that the compatibility should be specced.
The spec mentions usage of tokens from /sync in the /messages api https://spec.matrix.org/v1.16/client-server-api/#get_matrixclientv3roomsroomidmessages
And the simplified sliding sync MSC says the prev_batch
token is compatible with /messages
https://github.com/matrix-org/matrix-spec-proposals/blob/erikj/sss/proposals/4186-simplified-sliding-sync.md
prev_batch -> A token that can be passed as a start parameter to the /rooms/<room_id>/messages API to retrieve earlier messages.
So it seems like the landscape is already leaning in that direction, but maybe hasn't been formalized.
EXT_NAME = "io.element.msc4360.threads" | ||
|
||
|
||
class SlidingSyncThreadsExtensionTestCase(SlidingSyncBase): |
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.
Needs the standard test_wait_for_new_data
, test_wait_for_new_data_timeout
tests
@@ -0,0 +1 @@ | |||
Add experimental support for MSC4360: Sliding Sync Threads Extension. |
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.
Also comments on the MSC that may change how we approach things here -> matrix-org/matrix-spec-proposals#4360 (review)
Implements sliding sync extension from: matrix-org/matrix-spec-proposals#4360
Companion endpoint added in #19041
Disclaimer: I used Claude Code in a consultative manner to assist with parts of this PR.
I used it mainly for considering options for the SQL query and to help generate test cases.
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.