Skip to content

Fix sending heroes in SSS when m.room.name="" #19468

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.

Sign up for GitHub

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

Open
frebib wants to merge 1 commit into
base: develop
Choose a base branch
from
+21 −21
Open

Fix sending heroes in SSS when m.room.name="" #19468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19468.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug in sync that could prevent user avatars from showing if the room had an empty name.
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Fix bug in sync that could prevent user avatars from showing if the room had an empty name.
Fix a bug in [MSC4186: Simplified Sliding Sync](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) that could prevent user avatars from showing if the room had an empty name.

I know it's nitpicky but useful for people reading to understand this is not oldschool sync.

41 changes: 20 additions & 21 deletions synapse/handlers/sliding_sync/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,20 +850,11 @@ async def get_room_sync_data(
# For incremental syncs, we can do this first to determine if something relevant
# has changed and strategically avoid fetching other costly things.
room_state_delta_id_map: MutableStateMap[str] = {}
name_event_id: str | None = None
room_name: str | None = None
membership_changed = False
name_changed = False
avatar_changed = False
if initial:
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, sorry to be a pest but I think it'd be nice to keep the performance properties of the old code:
we didn't get the m.room.name state unless the sync was initial.

I think this is getting run once per room on every sync so it feels important enough to at least try.

Instead of name_event_id: str | None how about pulling out the full event name_event: EventBase | None

I suspect we want to retrieve the name_event in two conditions:

  • initial
  • or name_changed (L908 / R899)

Also thinking that we need to properly handle the case where the room name is initially "My Room" and then someone changes it to "" at which point we need to start using heroes.

Copy link
Copy Markdown
Contributor Author

@frebib frebib Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for keeping the performance here if at all possible. The code is rather complex and I'm scared of introducing a regression 😆

Also thinking that we need to properly handle the case where the room name is initially "My Room" and then someone changes it to "" at which point we need to start using heroes.

Given that sync is stateless, I'm not sure if there's a way to do this without unconditionally fetching this event every time to look at the content to discern whether we need to send heroes or not.

# Check whether the room has a name set
name_state_ids = await self.get_current_state_ids_at(
room_id=room_id,
room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
state_filter=StateFilter.from_types([(EventTypes.Name, "")]),
to_token=to_token,
)
name_event_id = name_state_ids.get((EventTypes.Name, ""))
else:
if not initial:
assert from_bound is not None

# TODO: Limit the number of state events we're about to send down
Expand Down Expand Up @@ -911,6 +902,19 @@ async def get_room_sync_data(
):
avatar_changed = True

# TODO: Should we also check for `EventTypes.CanonicalAlias`
# (`m.room.canonical_alias`) as a fallback for the room name? see
# https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153
Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can also link to https://github.com/matrix-org/matrix-spec-proposals/pull/4186/changes#r2860107511 which I just opened as an equivalent thread on the non-obsolete MSC.

I think you're currently correct in not using the canonical alias

Copy link
Copy Markdown
Contributor

@reivilibre reivilibre Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I just realised this is not your comment, you just moved it, sorry & thanks)

Copy link
Copy Markdown
Contributor Author

@frebib frebib Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll update the comment link

name_state = await self.get_current_state_at(
room_id=room_id,
room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
state_filter=StateFilter.from_types([(EventTypes.Name, "")]),
to_token=to_token,
)
name_event = name_state.get((EventTypes.Name, ""))
if name_event is not None:
room_name = name_event.content.get("name")

# We only need the room summary for calculating heroes, however if we do
# fetch it then we can use it to calculate `joined_count` and
# `invited_count`.
Expand All @@ -932,7 +936,10 @@ async def get_room_sync_data(
# We need to fetch the `heroes` if the room name is not set. But we only need to
# get them on initial syncs (or the first time we send down the room) or if the
# membership has changed which may change the heroes.
if name_event_id is None and (initial or (not initial and membership_changed)):
#
# As per https://spec.matrix.org/v1.17/client-server-api/#mroomname
# empty/missing name values should be treated as not having a name.
if not room_name and (initial or (not initial and membership_changed)):
# We need the room summary to extract the heroes from
if room_membership_for_user_at_to_token.membership != Membership.JOIN:
# TODO: Figure out how to get the membership summary for left/banned rooms
Expand Down Expand Up @@ -1310,15 +1317,7 @@ async def get_room_sync_data(
if required_state_filter != StateFilter.none():
required_room_state = required_state_filter.filter_state(room_state)

# Find the room name and avatar from the state
room_name: str | None = None
# TODO: Should we also check for `EventTypes.CanonicalAlias`
# (`m.room.canonical_alias`) as a fallback for the room name? see
# https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153
name_event = room_state.get((EventTypes.Name, ""))
if name_event is not None:
room_name = name_event.content.get("name")

# Find the room avatar from the state (name is fetched above)
room_avatar: str | None = None
avatar_event = room_state.get((EventTypes.RoomAvatar, ""))
if avatar_event is not None:
Expand Down
Loading