Krille/refactor-archive-room-handling#2217
Conversation
krille-chan
force-pushed
the
krille/refactor-archive-room-handling
branch
from
7320e86 to
44c54ee
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2217 +/- ##
==========================================
- Coverage 59.29% 59.22% -0.08%
==========================================
Files 161 161
Lines 20300 20280 -20
==========================================
- Hits 12037 12010 -27
- Misses 8263 8270 +7
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
|
krille-chan
force-pushed
the
krille/refactor-archive-room-handling
branch
from
44c54ee to
129b959
Compare
krille-chan
force-pushed
the
krille/refactor-archive-room-handling
branch
2 times, most recently
from
44634d1 to
84eccf8
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
krille-chan
force-pushed
the
krille/refactor-archive-room-handling
branch
3 times, most recently
from
4f2d4cf to
8b3c7bf
Compare
krille-chan
force-pushed
the
krille/refactor-archive-room-handling
branch
from
8b3c7bf to
8bb6523
Compare
PR SummaryMedium Risk Overview Left-room handling is now driven by
Reviewed by Cursor Bugbot for commit 8bb6523. 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 4 potential issues.
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Leave removes room with includeLeave
- Left rooms are now retained in client.rooms with leave membership and prev_batch when includeLeave is enabled.
- ✅ Fixed: Archived getTimeline skips database
- getTimeline now reads persisted events from the database for archived rooms as well as active rooms.
- ✅ Fixed: Leave update skips DB membership
- storeRoomUpdate now updates existing room rows for leave updates, including membership, prev_batch, and lastEvent.
- ✅ Fixed: Re-invite skips membership update
- Existing rooms now update their in-memory and persisted membership when an invite update arrives.
Or push these changes by commenting:
@cursor push 20b0a05559
Preview (20b0a05559)
diff --git a/lib/src/client.dart b/lib/src/client.dart
--- a/lib/src/client.dart
+++ b/lib/src/client.dart
@@ -2983,12 +2983,26 @@
}
// If the membership is "leave" then remove the item and stop here
else if (membership == Membership.leave) {
- if (syncFilter.room?.includeLeave == true && !found) {
- rooms.add(room);
+ final leftUpdate = chatUpdate as LeftRoomUpdate;
+ if (syncFilter.room?.includeLeave == true) {
+ final membershipChanged = found && room.membership != membership;
+ room.membership = membership;
+ room.prev_batch = leftUpdate.timeline?.prevBatch ?? room.prev_batch;
+ if (!found) rooms.add(room);
+ if (membershipChanged) {
+ // ignore: deprecated_member_use_from_same_package
+ room.onUpdate.add(room.id);
+ }
} else if (found) {
rooms.removeAt(roomIndex);
}
}
+ // Update membership for existing invites.
+ else if (found && chatUpdate is InvitedRoomUpdate) {
+ rooms[roomIndex].membership = membership;
+ // ignore: deprecated_member_use_from_same_package
+ rooms[roomIndex].onUpdate.add(rooms[roomIndex].id);
+ }
// Update notification, highlight count and/or additional information
else if (found &&
chatUpdate is JoinedRoomUpdate &&
diff --git a/lib/src/database/matrix_sdk_database.dart b/lib/src/database/matrix_sdk_database.dart
--- a/lib/src/database/matrix_sdk_database.dart
+++ b/lib/src/database/matrix_sdk_database.dart
@@ -1305,8 +1305,10 @@
lastEvent: lastEvent,
).toJson(),
);
- } else if (roomUpdate is JoinedRoomUpdate) {
+ } else {
final currentRoom = Room.fromJson(copyMap(currentRawRoom), client);
+ final joinedUpdate = roomUpdate is JoinedRoomUpdate ? roomUpdate : null;
+ final leftUpdate = roomUpdate is LeftRoomUpdate ? roomUpdate : null;
await _roomsBox.put(
roomId,
Room(
@@ -1314,17 +1316,22 @@
id: roomId,
membership: membership,
highlightCount:
- roomUpdate.unreadNotifications?.highlightCount ??
+ joinedUpdate?.unreadNotifications?.highlightCount ??
currentRoom.highlightCount,
notificationCount:
- roomUpdate.unreadNotifications?.notificationCount ??
+ joinedUpdate?.unreadNotifications?.notificationCount ??
currentRoom.notificationCount,
- prev_batch: roomUpdate.timeline?.prevBatch ?? currentRoom.prev_batch,
- summary: RoomSummary.fromJson(
- currentRoom.summary.toJson()
- ..addAll(roomUpdate.summary?.toJson() ?? {}),
- ),
- lastEvent: lastEvent,
+ prev_batch:
+ joinedUpdate?.timeline?.prevBatch ??
+ leftUpdate?.timeline?.prevBatch ??
+ currentRoom.prev_batch,
+ summary: joinedUpdate == null
+ ? currentRoom.summary
+ : RoomSummary.fromJson(
+ currentRoom.summary.toJson()
+ ..addAll(joinedUpdate.summary?.toJson() ?? {}),
+ ),
+ lastEvent: lastEvent ?? currentRoom.lastEvent,
).toJson(),
);
}
diff --git a/lib/src/room.dart b/lib/src/room.dart
--- a/lib/src/room.dart
+++ b/lib/src/room.dart
@@ -1701,11 +1701,9 @@
var events = <Event>[];
- if (!isArchived) {
- await client.database.transaction(() async {
- events = await client.database.getEventList(this, limit: limit);
- });
- }
+ await client.database.transaction(() async {
+ events = await client.database.getEventList(this, limit: limit);
+ });
var chunk = TimelineChunk(events: events);
// Load the timeline arround eventContextId if set
diff --git a/test/fake_client.dart b/test/fake_client.dart
--- a/test/fake_client.dart
+++ b/test/fake_client.dart
@@ -20,6 +20,7 @@
Future<Client> getClient({
Duration sendTimelineEventTimeout = const Duration(minutes: 1),
String? databasePath,
+ Filter? syncFilter,
}) async {
try {
vodInit ??= vod.init(
@@ -37,6 +38,7 @@
database: await getDatabase(databasePath: databasePath),
onSoftLogout: (client) => client.refreshAccessToken(),
sendTimelineEventTimeout: sendTimelineEventTimeout,
+ syncFilter: syncFilter,
);
FakeMatrixApi.client = client;
await client.checkHomeserver(
diff --git a/test/room_archived_test.dart b/test/room_archived_test.dart
--- a/test/room_archived_test.dart
+++ b/test/room_archived_test.dart
@@ -73,6 +73,122 @@
expect(eventsFromStore.isEmpty, true);
});
+ test('includeLeave keeps left room in rooms and database', () async {
+ await client.dispose().onError((e, s) {});
+ client = await getClient(
+ syncFilter: Filter(
+ room: RoomFilter(
+ state: StateFilter(lazyLoadMembers: true),
+ includeLeave: true,
+ ),
+ ),
+ );
+ client.rooms.clear();
+ await client.database.clearCache();
+
+ const roomId = '!includeLeaveRoom:example.com';
+ await client.handleSync(
+ SyncUpdate(
+ nextBatch: 't_join',
+ rooms: RoomsUpdate(
+ join: {
+ roomId: JoinedRoomUpdate(
+ timeline: TimelineUpdate(
+ prevBatch: 'join_batch',
+ events: [
+ MatrixEvent(
+ type: EventTypes.Message,
+ senderId: '@alice:example.com',
+ eventId: '\$include-leave-join',
+ originServerTs: DateTime.fromMillisecondsSinceEpoch(1),
+ content: {'msgtype': 'm.text', 'body': 'joined'},
+ ),
+ ],
+ ),
+ ),
+ },
+ ),
+ ),
+ );
+
+ await client.handleSync(
+ SyncUpdate(
+ nextBatch: 't_leave',
+ rooms: RoomsUpdate(
+ leave: {
+ roomId: LeftRoomUpdate(
+ timeline: TimelineUpdate(
+ prevBatch: 'leave_batch',
+ events: [
+ MatrixEvent(
+ type: EventTypes.Message,
+ senderId: '@alice:example.com',
+ eventId: '\$include-leave-left',
+ originServerTs: DateTime.fromMillisecondsSinceEpoch(2),
+ content: {'msgtype': 'm.text', 'body': 'left'},
+ ),
+ ],
+ ),
+ ),
+ },
+ ),
+ ),
+ );
+
+ final room = client.getRoomById(roomId);
+ expect(room, isNotNull);
+ expect(room!.membership, Membership.leave);
+ expect(room.prev_batch, 'leave_batch');
+
+ final storedRoom = await client.database.getSingleRoom(client, roomId);
+ expect(storedRoom?.membership, Membership.leave);
+ expect(storedRoom?.prev_batch, 'leave_batch');
+ expect(storedRoom?.lastEvent?.eventId, '\$include-leave-left');
+
+ final timeline = await room.getTimeline();
+ expect(
+ timeline.events.map((event) => event.eventId),
+ contains('\$include-leave-left'),
+ );
+ });
+
+ test('reinvite updates kept left room membership', () async {
+ await client.dispose().onError((e, s) {});
+ client = await getClient(
+ syncFilter: Filter(
+ room: RoomFilter(
+ state: StateFilter(lazyLoadMembers: true),
+ includeLeave: true,
+ ),
+ ),
+ );
+ client.rooms.clear();
+ await client.database.clearCache();
+
+ const roomId = '!reinviteLeftRoom:example.com';
+ await client.handleSync(
+ SyncUpdate(
+ nextBatch: 't_join',
+ rooms: RoomsUpdate(join: {roomId: JoinedRoomUpdate()}),
+ ),
+ );
+ await client.handleSync(
+ SyncUpdate(
+ nextBatch: 't_leave',
+ rooms: RoomsUpdate(leave: {roomId: LeftRoomUpdate()}),
+ ),
+ );
+
+ await client.handleSync(
+ SyncUpdate(
+ nextBatch: 't_invite',
+ rooms: RoomsUpdate(invite: {roomId: InvitedRoomUpdate()}),
+ ),
+ );
+
+ expect(client.getRoomById(roomId)?.membership, Membership.invite);
+ });
+
test('discard room from archives when membership change', () async {
await client.loadArchiveWithTimeline();
await client.handleSync(You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 8bb6523. Configure here.
| if (syncFilter.room?.includeLeave == true && !found) { | ||
| rooms.add(room); | ||
| } else if (found) { | ||
| rooms.removeAt(roomIndex); |
There was a problem hiding this comment.
Leave removes room with includeLeave
High Severity
When a user leaves a room they were previously joined to, the room is incorrectly removed from client.rooms. This prevents the new includeLeave logic from persisting the left room's state in the database and causes getRoomById to fail for these rooms, potentially leaving stale local data.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8bb6523. Configure here.
| await client.database.transaction(() async { | ||
| events = await client.database.getEventList(this, limit: limit); | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Archived getTimeline skips database
High Severity
The getTimeline method no longer loads events for isArchived (left) rooms. The removed else branch previously handled fetching these, but now the database query only runs for non-archived rooms. This results in empty timelines for left rooms, even when events are locally persisted from includeLeave sync.
Reviewed by Cursor Bugbot for commit 8bb6523. Configure here.
| // Leave room if membership is leave | ||
| if (roomUpdate is LeftRoomUpdate) { | ||
| if (roomUpdate is LeftRoomUpdate && | ||
| client.syncFilter.room?.includeLeave != true) { |
There was a problem hiding this comment.
Leave update skips DB membership
Medium Severity
When storeRoomUpdate processes a LeftRoomUpdate with client.syncFilter.room?.includeLeave enabled, it doesn't correctly update the existing room's membership to leave or persist lastEvent and prev_batch.
Reviewed by Cursor Bugbot for commit 8bb6523. Configure here.
| if (syncFilter.room?.includeLeave == true && !found) { | ||
| rooms.add(room); | ||
| } else if (found) { | ||
| rooms.removeAt(roomIndex); |
There was a problem hiding this comment.
Re-invite skips membership update
Medium Severity
When a room is already in client.rooms (e.g. a left room kept via includeLeave), an InvitedRoomUpdate does not update rooms[roomIndex].membership to invite because membership changes run only in the JoinedRoomUpdate branch.
Reviewed by Cursor Bugbot for commit 8bb6523. Configure here.
1 participant
closes #2031
Test out with:
If you don't change the syncFilter, the behavior should be as before, just that you now have to cache the archive by yourself when requesting it.