Skip to content
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

Stuck notifications: Reacting to a not-latest message in a thread causes a stuck notification #24000

Closed
Tracked by #24392
turt2live opened this issue Dec 14, 2022 · 4 comments · Fixed by matrix-org/matrix-js-sdk#3146
Assignees
Labels
A-Threads O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@turt2live
Copy link
Member

Steps to reproduce

  1. See a thread with many messages in it (no need for explicit participation)
  2. React to one of the messages in the thread, that isn't the latest message
  3. Room goes from read to unread
  4. Scroll down on the main timeline, even if already scrolled down, with the thread open
  5. Room resets to read

Outcome

What did you expect?

For a reaction to not cause the room to go unread (as I would have sent the latest event in the room).

What happened instead?

The room was marked as unread. Switching rooms back & forth, opening and closing the thread, etc did not clear the notification: only forcefully scrolling in the main timeline.

Operating system

Windows 10

Application version

Nightly (2022-12-14)

How did you install the app?

The Internet

Homeserver

t2l.io

Will you send logs?

Yes

@germain-gg germain-gg added X-Cannot-Reproduce S-Minor Impairs non-critical functionality or suitable workarounds exist O-Uncommon Most users are unlikely to come across this or unexpected workflow A-Threads labels Dec 15, 2022
@germain-gg
Copy link
Contributor

I have not been able to reproduce this issue. Tried in both an encrypted and unencrypted rooms.
I suspect that there's a difference in pushrules between our two accounts.

Could you try and refine the steps to repro?

Screen.Recording.2022-12-15.at.08.35.40.mov

@turt2live
Copy link
Member Author

Unfortunately that's as refined as I could get it :(

If it helps, the room was New Vector Internal (encrypted, medium traffic), and I have it set to Mentions Only. I'm not sure if there would have been a badge count or not.

@Johennes Johennes changed the title Reacting to a not-latest message in a thread causes a stuck notification Stuck notifications: Reacting to a not-latest message in a thread causes a stuck notification Feb 1, 2023
@weeman1337
Copy link
Contributor

I am able to reproduce this with two users:

  • A starts a thread
  • A posts some message in the thread
  • B reacts to any of the message in the middle

→ stuck unread

unread_react

@justjanne
Copy link
Contributor

justjanne commented Feb 13, 2023

The following is only to document the testing that I’ve done on this issue for myself, don’t expect it to be 100% accurate:

  • neither the thread that was reacted on nor any of the messages reacted on have not been followed by any own events
  • the root of the thread was created by someone else
  • the message that was reacted on was created by someone else
  • the last message in the thread was created by someone else

but it does not matter whether the thread that was reacted on was the most recently updated thread, or followed by any other thread or any other event. It also does not matter whether the message is the genuinely last message in the thread or not.

It seems to be caused by a synthetic read receipt for a thread being added to the room itself instead of the thread.

I've further investigated the cause, and have determined that https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/thread.ts#L204-L206 is at fault. It will generate a synthetic read receipt for thread events regardless of whether threadRootId is set or not. But https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/read-receipt.ts#L37 uses threadRootId to determine whether a read receipt should belong to a thread or the main room.

This in turn seems to be caused by MatrixEvent.setThread/Thread.setEventMetadata/Thread.processEvent only being called after this synthetic read receipt has already been generated.

Possible solutions could be

Additional questions that remain: Is this caused by the original local echo, or the homeservers' remote echo version of the message? Depending on that, there may be an underlying root cause of the thread id not being correctly set earlier on in the process.

EDIT: It’s definitely the handleRemoteEcho, specifically https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/room.ts#L2470 being called before MatrixEvent.setThread/Thread.setEventMetadata, which means the event is in somewhat of a limbo state, where some parts of our code think the event belongs to a thread, and other parts will think it doesn't.

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue Mar 15, 2023
* Add easy way to determine if the decryption failure is due to "DecryptionError: The sender has disabled encrypting to unverified devices." ([\matrix-org#3167](matrix-org#3167)). Contributed by @florianduros.
* Polls: expose end event id on poll model ([\matrix-org#3160](matrix-org#3160)). Contributed by @kerryarchibald.
* Polls: count undecryptable poll relations ([\matrix-org#3163](matrix-org#3163)). Contributed by @kerryarchibald.
* Fix spec compliance issue around encrypted `m.relates_to` ([\matrix-org#3178](matrix-org#3178)).
* Fix reactions in threads sometimes causing stuck notifications ([\matrix-org#3146](matrix-org#3146)). Fixes element-hq/element-web#24000. Contributed by @justjanne.
* Better type guard parseTopicContent ([\matrix-org#3165](matrix-org#3165)). Fixes matrix-org/element-web-rageshakes#20177 and matrix-org/element-web-rageshakes#20178.
* Fix a bug where events in encrypted rooms would sometimes erroneously increment the total unread counter after being processed locally. ([\matrix-org#3130](matrix-org#3130)). Fixes element-hq/element-web#24448. Contributed by @Half-Shot.
* Stop the ICE disconnected timer on call terminate ([\matrix-org#3147](matrix-org#3147)).
* Clear notifications when we can infer read status from receipts ([\matrix-org#3139](matrix-org#3139)). Fixes element-hq/element-web#23991.
* Messages sent out of order after one message fails ([\matrix-org#3131](matrix-org#3131)). Fixes element-hq/element-web#22885 and element-hq/element-web#18942. Contributed by @justjanne.
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue Mar 15, 2023
* Implement MSC3758: a push rule condition to match event properties exactly ([\matrix-org#3179](matrix-org#3179)).
* Enable group calls without video and audio track by configuration of MatrixClient ([\matrix-org#3162](matrix-org#3162)). Contributed by @EnricoSchw.
* Updates to protocol used for Sign in with QR code ([\matrix-org#3155](matrix-org#3155)). Contributed by @hughns.
* Implement MSC3873 to handle escaped dots in push rule keys ([\matrix-org#3134](matrix-org#3134)). Fixes undefined/matrix-js-sdk#1454.
* Fix spec compliance issue around encrypted `m.relates_to` ([\matrix-org#3178](matrix-org#3178)).
* Fix reactions in threads sometimes causing stuck notifications ([\matrix-org#3146](matrix-org#3146)). Fixes element-hq/element-web#24000. Contributed by @justjanne.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Threads O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants