Notification counts may be incorrectly reset by sent messages

[richvdh]

Suppose:

• You invite Bob to a room. Bob immediately joins and sends a message.
• /sync is slow for whatever reason, and Bob's message is returned in the /sync response as the remote echo of your invite event.

In that scenario, Room.getUnreadNotificationCount() should return 1. However, it currently returns 0. This causes the client to show the room as fully read, whereas in fact there are unread messages.

[richvdh]

The problem here appears to be as follows:

• When we process the /sync response, we first process the notification count (https://github.com/matrix-org/matrix-js-sdk/blob/v28.0.0/src/sync.ts#L1330-L1333) from the server. This (correctly) sets the notification count to 1.
• Next, we call injectRoomEvents with the events we received from sync: https://github.com/matrix-org/matrix-js-sdk/blob/v28.0.0/src/sync.ts#L1447. This calls room.addLiveEvents, which calls addLiveEvent for each event.
• For each event we sent (including, in the example, the invite), Room.addLiveEvent calls Room.addReceipt with a synthetic receipt, meaning that any event from us makes the room read. https://github.com/matrix-org/matrix-js-sdk/blob/v28.0.0/src/models/room.ts#L2421
• Room.addReceipt calls setUnread(0) when it receives a receipt from ourselves for the last event in the room: https://github.com/matrix-org/matrix-js-sdk/blob/v28.0.0/src/models/room.ts#L2944-L2950.
  At the time of processing, the invite appears to be the last event in the room, because Room.addLiveEvents has not yet processed the message from Bob.

The code in Room.addReceipt was added in #3139, and it replaced a previous hack in matrix-react-sdk (https://github.com/matrix-org/matrix-react-sdk/blob/v3.72.0/src/components/structures/TimelinePanel.tsx#L1090-L1103). It is needed to mark a room as read quickly when we send a read receipt, without waiting for the remote echo of that receipt (in contrast what the comment says).

[richvdh]

I think the proposed fix is to update the code in Room.addReceipt (https://github.com/matrix-org/matrix-js-sdk/blob/v28.0.0/src/models/room.ts#L2944-L2950) to ignore synthetic receipts.

[andybalaam]

OK, I had some further discussion with @richvdh over DM and want to try and summarise here:

I was getting confused because I was thinking the correct time for this code to run was when we sent a message, but the correct time is when we send a receipt (because the user read something).

So, if we ignore synthetic receipts, we avoid times when we either send events or receive events via sync. In both of these cases we don't need to modify the unread count.

The last question for me is: what happens when we receive receipts over sync? Do we need to avoid running this logic at that time? We think the answer is no, because it will run after the messages have been processed, so the logic checking for the last event will be work as intended.

[andybalaam]

Answer: yes, we call Room.addEphemeralEvents during sync (src/sync.ts:1460) after we've called injectRoomEvents.

addEphemeralEvents does call addReceipt, but it is already late enough that the logic in there about checking for the last event in the room is OK.

Conclusion: just don't do this (reset unread status) for synthetic receipts.