fix: filter functional members from DM room display name heroes#2276
fix: filter functional members from DM room display name heroes#2276beezly wants to merge 4 commits into
Conversation
|
|
|
Closing — not ready for review yet. |
beezly
force-pushed
the
fix/dm-hero-filtering
branch
from
067003c to
45997ba
Compare
When a room is marked as a DM (via m.direct account data) and contains bridge bots or service members listed in the io.element.functional_members state event, those users were incorrectly included in the room display name and hero list. This mirrors the JS SDK's getFunctionalMembers() approach: read the service_members list from the io.element.functional_members state event and exclude those user IDs from heroes when computing the room name for a DM room. Adds: - Room.functionalMembers getter to read io.element.functional_members - Filtering in getLocalizedDisplayname() to exclude functional members from heroes when directChatMatrixID is set - Filtering in loadHeroUsers() for the same reason Fixes: krille-chan/fluffychat#2116 See: https://github.com/element-hq/element-meta/blob/develop/spec/functional_members.md
beezly
force-pushed
the
fix/dm-hero-filtering
branch
from
45997ba to
a5093c7
Compare
📝 WalkthroughWalkthroughThis PR adds support for filtering out functional members (bots/bridges) from direct-message rooms. A new ChangesDM Functional Member Filtering
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/src/room.dart (2)
210-218: ⚡ Quick winRedundant variable read for
directChatMatrixID.
directChatMatrixIDis already read at line 202 and can be reused here instead of re-reading it at line 212.♻️ Refactor to reuse the variable from line 202
if (heroes == null) return []; - // Filter out functional members (bots/bridges) when in a DM room, - // so that hero user loading only fetches the real human participant. - final directChatMatrixID = this.directChatMatrixID; - if (directChatMatrixID != null) { + // Filter out functional members (bots/bridges) when in a DM room, + // so that hero user loading only fetches the real human participant. + if (directChatMatrixID != null) { final fMembers = functionalMembers; if (fMembers.isNotEmpty) { heroes = heroes.where((h) => !fMembers.contains(h)).toList(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/room.dart` around lines 210 - 218, The code redundantly re-reads directChatMatrixID; reuse the previously read directChatMatrixID variable (from earlier in the function) instead of re-evaluating it here—locate the existing directChatMatrixID declaration and use that reference in the conditional that filters out functionalMembers from heroes (keep the functionalMembers and heroes logic unchanged), removing the extra read to eliminate redundancy.
391-404: ⚡ Quick winConsider using
tryGetListfor consistency with codebase patterns.The current implementation manually checks the type and filters, but the codebase has a
tryGetListextension method (seelib/matrix_api_lite/utils/try_get_map_extension.dart) for extracting typed lists from event content.♻️ Use tryGetList for idiomatic list extraction
List<String> get functionalMembers { - final event = getState('io.element.functional_members'); - final serviceMembers = event?.content['service_members']; - if (serviceMembers is List) { - return serviceMembers.whereType<String>().toList(); - } - return []; + return getState('io.element.functional_members') + ?.content + .tryGetList<String>('service_members', TryGet.silent) ?? + []; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/room.dart` around lines 391 - 404, Replace the manual type-check/filter in the Room.functionalMembers getter with the project's tryGetList helper: fetch the state event via getState('io.element.functional_members'), then call event?.content.tryGetList('service_members') (or the appropriate extension import) and return that result (or [] if null) so the extraction is consistent and idiomatic with other code using tryGetList.test/room_test.dart (1)
1098-1103: ⚡ Quick winStrengthen negative test assertion.
The negative case uses
isNot('John Doe')to verify the bot appears whenio.element.functional_membersis absent. This assertion is weak because it would pass for any result that isn't exactly "John Doe" (e.g., empty string, error message).A stronger assertion would explicitly check that the bot's display name appears alongside the real user.
💪 Stronger assertion
- // Without functional_members, the bot appears alongside the real user - expect( - dmRoomNoFilter.getLocalizedDisplayname(), - isNot('John Doe'), - reason: - 'Without io.element.functional_members, bot should still appear in room name', - ); + // Without functional_members, the bot appears alongside the real user + final displayName = dmRoomNoFilter.getLocalizedDisplayname(); + expect( + displayName.contains('Signal Bridge Bot') || displayName.contains('signal-bot'), + true, + reason: + 'Without io.element.functional_members, bot should appear in room name', + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/room_test.dart` around lines 1098 - 1103, The negative assertion using isNot('John Doe') is too weak; update the test around dmRoomNoFilter.getLocalizedDisplayname() to explicitly assert that the returned display name contains both the bot's display name and the real user's name (e.g., use contains matchers or a regex against getLocalizedDisplayname() to verify presence of the bot label and "John Doe"), so that the test guarantees the bot appears alongside the real user rather than merely not equaling "John Doe".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/src/room.dart`:
- Around line 210-218: The code redundantly re-reads directChatMatrixID; reuse
the previously read directChatMatrixID variable (from earlier in the function)
instead of re-evaluating it here—locate the existing directChatMatrixID
declaration and use that reference in the conditional that filters out
functionalMembers from heroes (keep the functionalMembers and heroes logic
unchanged), removing the extra read to eliminate redundancy.
- Around line 391-404: Replace the manual type-check/filter in the
Room.functionalMembers getter with the project's tryGetList helper: fetch the
state event via getState('io.element.functional_members'), then call
event?.content.tryGetList('service_members') (or the appropriate extension
import) and return that result (or [] if null) so the extraction is consistent
and idiomatic with other code using tryGetList.
In `@test/room_test.dart`:
- Around line 1098-1103: The negative assertion using isNot('John Doe') is too
weak; update the test around dmRoomNoFilter.getLocalizedDisplayname() to
explicitly assert that the returned display name contains both the bot's display
name and the real user's name (e.g., use contains matchers or a regex against
getLocalizedDisplayname() to verify presence of the bot label and "John Doe"),
so that the test guarantees the bot appears alongside the real user rather than
merely not equaling "John Doe".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b1d6d7f-d57c-420e-b64e-f80ef351ebbe
📒 Files selected for processing (2)
lib/src/room.darttest/room_test.dart
|
I would love to see this PR merged. I am waiting for this fix. |
PR SummaryLow Risk Overview Adds Tests cover DM naming with and without the functional-members state. Reviewed by Cursor Bugbot for commit 2f053eb. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit 2f053eb. Configure here.
| hero.isNotEmpty && | ||
| hero != client.userID && | ||
| (directChatMatrixID == null || | ||
| !functionalMembers.contains(hero)), |
There was a problem hiding this comment.
Blank DM title after filter
Medium Severity
The new functional member filtering in DMs can result in empty hero lists for getLocalizedDisplayname and loadHeroUsers. This occurs because the directChatMatrixID fallback is only applied if m.heroes is empty before filtering, not after, leading to blank display names or no hero users when m.heroes initially contains only functional members.
Reviewed by Cursor Bugbot for commit 2f053eb. Configure here.
3 participants
Problem
When a room is marked as a direct message (via
m.directaccount data) and contains bridge bots or service members listed in theio.element.functional_membersstate event, those bots were incorrectly included in the room hero list and therefore appeared in the room display name.For example, if a WhatsApp bridge bot (
@whatsappbot:server) is a member of a DM room alongside the real contact, the room would be named something like "Alice and @whatsappbot:server" instead of just "Alice".Solution
This PR adds filtering of functional members from the hero list when computing the display name and loading hero users for DM rooms. It mirrors the approach used in the matrix-js-sdk's
getFunctionalMembers().Changes
Room.functionalMembersgetter — reads theservice_memberslist from theio.element.functional_membersstate eventgetLocalizedDisplayname()— excludes functional members from the heroes list whendirectChatMatrixIDis setloadHeroUsers()— applies the same filter for consistencyReferences
Signed-off-by: Andrew Beresford beezly@beez.ly
Summary by CodeRabbit
Bug Fixes
Tests