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

Reactions can trigger stuck notifications if read receipt above own messages #10208

Closed
3 tasks done
jryans opened this issue Jun 28, 2019 · 7 comments
Closed
3 tasks done

Comments

@jryans
Copy link
Collaborator

jryans commented Jun 28, 2019

  1. Create a room with two people
  2. Ensure room is set to noisy notifications
  3. Alice sends several messages
  4. For every reaction Bob sends, Alice gets a progressively incrementing notification count
    • Viewing the room temporarily clears the notification badge, but the count will keep accumulating for the next reaction
  5. Bob has to speak and be read by Alice (advancing Alice's read receipt) to clear Alice's notification count

We've decided to address this by default disabling notifications for reactions:

@turt2live
Copy link
Member

almost certainly because Alice's read receipt isn't advanced, and Riot optimistically clears the count. The return of the badge is because the room is synced and the server still hasn't received a RR, therefore Riot trusts the counts.

@jryans jryans self-assigned this Jul 1, 2019
@jryans jryans added this to In Progress in Web App Team via automation Jul 1, 2019
@jryans
Copy link
Collaborator Author

jryans commented Jul 2, 2019

It's true that Alice's read receipt isn't advanced, and yes, advancing it would clear the count on the server.

However, all of the regular messages (non-reaction events) in the room are from Alice. So why is the notification count growing...?

The growing notification count seems to be tied to step 2 ("room is set to noisy notifications"). Setting a room to noisy means we set a room-specific push rule that notifies for all events (and room-specific rules do not support conditions), so this means Bob's reaction events are incrementing Alice's notification count.

It's not clear how to best resolve this... It could make sense to do one of:

  • Alter room-specific push rules to only affect m.room.message events
  • Support conditions on room-specific push rules

@turt2live
Copy link
Member

The messages causing notifications are from Bob in the original post, not Alice. If Alice is sending messages and the notification count isn't going down, it sounds like a Synapse bug. If Bob is sending messages (or reactions, for that matter) and Alice has noisy notifications, then push rules are somewhat working as intended.

We probably just need a push rule for "notify on reactions" which overrides the noisy setting.

@jryans
Copy link
Collaborator Author

jryans commented Jul 2, 2019

The messages causing notifications are from Bob in the original post, not Alice.

Alice is sending only regular messages (see step 3). (I edited the original post to add "to Bob".)
Bob is sending only reactions (see step 4).

If Alice is sending messages and the notification count isn't going down, it sounds like a Synapse bug.

Alice sending more messages will not clear their own notification count, as currently only advancing Alice's read receipt will do that, and sending messages doesn't advance your read receipt. (Should it...?)

If Bob is sending messages (or reactions, for that matter) and Alice has noisy notifications, then push rules are somewhat working as intended.

Bob is sending only reactions.

If Alice uses only the global option "Messages in one-to-one chats" set to "Noisy" (which is the default for a new account), there is no issue because the .m.rule.room_one_to_one underride rule includes a condition to check that the event type is m.room.message. (However, this differs from the spec's definition of the push rule... Filed spec issue.)

If however Alice sets the room-specific notification settings to "All message (noisy)", then this sets the room-specific push rule which does not support conditions (such as on the event type).

From a product perspective, we believe the default behaviour should be that reactions do not trigger notifications. What's the best route to achieve that?

We probably just need a push rule for "notify on reactions" which overrides the noisy setting.

To achieve the product goal then, would you recommend adding a "don't notify on reactions" rule by default, or instead tweaking room-specific rules somehow to only match m.room.message?

@turt2live
Copy link
Member

Alice sending more messages will not clear their own notification count, as currently only advancing Alice's read receipt will do that, and sending messages doesn't advance your read receipt. (Should it...?)

The server should be synthesizing a receipt for the user on their own messages.


On the product goal: This stinks of needing to recosnider what "All Messages (noisy)" means entirely. Currently it'll fire notifications for any event, regardless of type, which is the root cause of the issue here (I think). My understanding is that the product wants to maintain the "all events cause notifications" requirement for the notification level, however reactions have somehow ended up being a special case (whereas any hidden event is not, despite creating the same problem).

@jryans
Copy link
Collaborator Author

jryans commented Jul 3, 2019

After chatting with @dbkr and @lampholder today, we're thinking a good approach would be to add a new default override push rule that disables notifications for reactions.

I'll create an MSC similar to matrix-org/matrix-spec-proposals#1930 for m.room.tombstone events.

@jryans jryans removed the X-Blocked label Jul 3, 2019
jryans added a commit to matrix-org/matrix-js-sdk that referenced this issue Jul 4, 2019
This adds a default push rule to ignore reactions as proposed in
[MSC2153](matrix-org/matrix-spec-proposals#2153). By adding it here
in the client directly, we can try out the idea early even if it hasn't appeared
in the user's HS yet.

Part of element-hq/element-web#10208
jryans added a commit to matrix-org/synapse that referenced this issue Jul 5, 2019
@jryans
Copy link
Collaborator Author

jryans commented Jul 5, 2019

Considering this done from a Riot Web perspective. The Synapse PR and MSC are still outstanding, but the original issue is effectively fixed in Riot.

@jryans jryans closed this as completed Jul 5, 2019
Web App Team automation moved this from In Progress to In Test Jul 5, 2019
jryans added a commit to matrix-org/synapse that referenced this issue Jul 5, 2019
@jryans jryans moved this from In Test to Done in Web App Team Jul 18, 2019
su-ex added a commit to SchildiChat/element-web that referenced this issue Mar 15, 2023
* Remove experimental PWA support for Firefox and Safari ([\element-hq#24630](element-hq#24630)).
* Fix block code styling in rich text editor ([\element-hq#10246](matrix-org/matrix-react-sdk#10246)). Contributed by @alunturner.
* Poll history: fetch more poll history ([\element-hq#10235](matrix-org/matrix-react-sdk#10235)). Contributed by @kerryarchibald.
* Sort short/exact emoji matches before longer incomplete matches ([\element-hq#10212](matrix-org/matrix-react-sdk#10212)). Fixes element-hq#23210. Contributed by @grimhilt.
* Poll history: detail screen ([\element-hq#10172](matrix-org/matrix-react-sdk#10172)). Contributed by @kerryarchibald.
* Provide a more detailed error message than "No known servers" ([\element-hq#6048](matrix-org/matrix-react-sdk#6048)). Fixes element-hq#13247. Contributed by @aaronraimist.
* Say when a call was answered from a different device ([\element-hq#10224](matrix-org/matrix-react-sdk#10224)).
* Widget permissions customizations using module api ([\element-hq#10121](matrix-org/matrix-react-sdk#10121)). Contributed by @maheichyk.
* Fix copy button icon overlapping with copyable text ([\element-hq#10227](matrix-org/matrix-react-sdk#10227)). Contributed by @Adesh-Pandey.
* Support joining non-peekable rooms via the module API ([\element-hq#10154](matrix-org/matrix-react-sdk#10154)). Contributed by @maheichyk.
* The "new login" toast does now display the same device information as in the settings. "No" does now open the device settings. "Yes, it was me" dismisses the toast. ([\element-hq#10200](matrix-org/matrix-react-sdk#10200)).
* Do not prompt for a password when doing a „reset all“ after login ([\element-hq#10208](matrix-org/matrix-react-sdk#10208)).
* Display "The sender has blocked you from receiving this message" error message instead of "Unable to decrypt message" ([\element-hq#10202](matrix-org/matrix-react-sdk#10202)). Contributed by @florianduros.
* Polls: show warning about undecryptable relations ([\element-hq#10179](matrix-org/matrix-react-sdk#10179)). Contributed by @kerryarchibald.
* Poll history: fetch last 30 days of polls ([\element-hq#10157](matrix-org/matrix-react-sdk#10157)). Contributed by @kerryarchibald.
* Poll history - ended polls list items ([\element-hq#10119](matrix-org/matrix-react-sdk#10119)). Contributed by @kerryarchibald.
* Remove threads labs flag and the ability to disable threads ([\element-hq#9878](matrix-org/matrix-react-sdk#9878)). Fixes element-hq#24365.
* Show a success dialog after setting up the key backup ([\element-hq#10177](matrix-org/matrix-react-sdk#10177)). Fixes element-hq#24487.
* Release Sign in with QR out of labs ([\element-hq#10182](matrix-org/matrix-react-sdk#10182)). Contributed by @hughns.
* Hide indent button in rte ([\element-hq#10149](matrix-org/matrix-react-sdk#10149)). Contributed by @alunturner.
* Add option to find own location in map views ([\element-hq#10083](matrix-org/matrix-react-sdk#10083)).
* Render poll end events in timeline ([\element-hq#10027](matrix-org/matrix-react-sdk#10027)). Contributed by @kerryarchibald.
* Use the room avatar as a placeholder in calls ([\element-hq#10231](matrix-org/matrix-react-sdk#10231)).
* Fix calls showing as 'connecting' after hangup ([\element-hq#10223](matrix-org/matrix-react-sdk#10223)).
* Stop access token overflowing the box ([\element-hq#10069](matrix-org/matrix-react-sdk#10069)). Fixes element-hq#24023. Contributed by @sbjaj33.
* Prevent multiple Jitsi calls started at the same time ([\element-hq#10183](matrix-org/matrix-react-sdk#10183)). Fixes element-hq#23009.
* Make localization keys compatible with agglutinative and/or SOV type languages ([\element-hq#10159](matrix-org/matrix-react-sdk#10159)). Contributed by @luixxiul.
* Add link to next file in the export ([\element-hq#10190](matrix-org/matrix-react-sdk#10190)). Fixes element-hq#20272. Contributed by @grimhilt.
* Ended poll tiles: add ended the poll message ([\element-hq#10193](matrix-org/matrix-react-sdk#10193)). Fixes element-hq#24579. Contributed by @kerryarchibald.
* Fix accidentally inverted condition for room ordering ([\element-hq#10178](matrix-org/matrix-react-sdk#10178)). Fixes element-hq#24527. Contributed by @justjanne.
* Re-focus the composer on dialogue quit ([\element-hq#10007](matrix-org/matrix-react-sdk#10007)). Fixes element-hq#22832. Contributed by @Ashu999.
* Try to resolve emails before creating a DM ([\element-hq#10164](matrix-org/matrix-react-sdk#10164)).
* Disable poll response loading test ([\element-hq#10168](matrix-org/matrix-react-sdk#10168)). Contributed by @justjanne.
* Fix email lookup in invite dialog ([\element-hq#10150](matrix-org/matrix-react-sdk#10150)). Fixes element-hq#23353.
* Remove duplicate white space characters from translation keys ([\element-hq#10152](matrix-org/matrix-react-sdk#10152)). Contributed by @luixxiul.
* Fix the caption of new sessions manager on Labs settings page for localization ([\element-hq#10143](matrix-org/matrix-react-sdk#10143)). Contributed by @luixxiul.
* Prevent start another DM with a user if one already exists ([\element-hq#10127](matrix-org/matrix-react-sdk#10127)). Fixes element-hq#23138.
* Remove white space characters before the horizontal ellipsis ([\element-hq#10130](matrix-org/matrix-react-sdk#10130)). Contributed by @luixxiul.
* Fix Selectable Text on 'Delete All' and 'Retry All' Buttons ([\element-hq#10128](matrix-org/matrix-react-sdk#10128)). Fixes element-hq#23232. Contributed by @akshattchhabra.
* Correctly Identify emoticons ([\element-hq#10108](matrix-org/matrix-react-sdk#10108)). Fixes element-hq#19472. Contributed by @adarsh-sgh.
* Remove a redundant white space ([\element-hq#10129](matrix-org/matrix-react-sdk#10129)). Contributed by @luixxiul.
su-ex added a commit to SchildiChat/element-web that referenced this issue Mar 15, 2023
* Remove experimental PWA support for Firefox and Safari ([\element-hq#24630](element-hq#24630)).
* Only allow to start a DM with one email if encryption by default is enabled ([\element-hq#10253](matrix-org/matrix-react-sdk#10253)). Fixes element-hq#23133.
* DM rooms are now encrypted if encryption by default is enabled and only inviting a single email address. Any action in the result DM room will be blocked until the other has joined. ([\element-hq#10229](matrix-org/matrix-react-sdk#10229)).
* Reduce bottom margin of ReplyChain on compact modern layout ([\element-hq#8972](matrix-org/matrix-react-sdk#8972)). Fixes element-hq#22748. Contributed by @luixxiul.
* Support for v2 of MSC3903 ([\element-hq#10165](matrix-org/matrix-react-sdk#10165)). Contributed by @hughns.
* When starting a DM, existing rooms with pending third-party invites will be reused. ([\element-hq#10256](matrix-org/matrix-react-sdk#10256)). Fixes element-hq#23139.
* Polls push rules: synchronise poll rules with message rules ([\element-hq#10263](matrix-org/matrix-react-sdk#10263)). Contributed by @kerryarchibald.
* New verification request toast button labels ([\element-hq#10259](matrix-org/matrix-react-sdk#10259)).
* Remove padding around integration manager iframe ([\#10148](matrix-org/matrix-react-sdk#10148)).
* Fix block code styling in rich text editor ([\element-hq#10246](matrix-org/matrix-react-sdk#10246)). Contributed by @alunturner.
* Poll history: fetch more poll history ([\element-hq#10235](matrix-org/matrix-react-sdk#10235)). Contributed by @kerryarchibald.
* Sort short/exact emoji matches before longer incomplete matches ([\element-hq#10212](matrix-org/matrix-react-sdk#10212)). Fixes element-hq#23210. Contributed by @grimhilt.
* Poll history: detail screen ([\element-hq#10172](matrix-org/matrix-react-sdk#10172)). Contributed by @kerryarchibald.
* Provide a more detailed error message than "No known servers" ([\element-hq#6048](matrix-org/matrix-react-sdk#6048)). Fixes element-hq#13247. Contributed by @aaronraimist.
* Say when a call was answered from a different device ([\element-hq#10224](matrix-org/matrix-react-sdk#10224)).
* Widget permissions customizations using module api ([\element-hq#10121](matrix-org/matrix-react-sdk#10121)). Contributed by @maheichyk.
* Fix copy button icon overlapping with copyable text ([\element-hq#10227](matrix-org/matrix-react-sdk#10227)). Contributed by @Adesh-Pandey.
* Support joining non-peekable rooms via the module API ([\element-hq#10154](matrix-org/matrix-react-sdk#10154)). Contributed by @maheichyk.
* The "new login" toast does now display the same device information as in the settings. "No" does now open the device settings. "Yes, it was me" dismisses the toast. ([\element-hq#10200](matrix-org/matrix-react-sdk#10200)).
* Do not prompt for a password when doing a „reset all“ after login ([\element-hq#10208](matrix-org/matrix-react-sdk#10208)).
* Fix incorrect copy in space creation flow ([\element-hq#10296](matrix-org/matrix-react-sdk#10296)). Fixes element-hq#24741.
* Fix space settings dialog having rogue title tooltip ([\element-hq#10293](matrix-org/matrix-react-sdk#10293)). Fixes element-hq#24740.
* Show spinner when starting a DM from the user profile (right panel) ([\element-hq#10290](matrix-org/matrix-react-sdk#10290)).
* Reduce height of toggle on expanded view source event ([\element-hq#10283](matrix-org/matrix-react-sdk#10283)). Fixes element-hq#22873. Contributed by @luixxiul.
* Pillify http and non-prefixed matrix.to links ([\element-hq#10277](matrix-org/matrix-react-sdk#10277)). Fixes element-hq#20844.
* Fix some features not being configurable via `features` ([\element-hq#10276](matrix-org/matrix-react-sdk#10276)).
* Fix starting a DM from the right panel in some cases ([\element-hq#10278](matrix-org/matrix-react-sdk#10278)). Fixes element-hq#24722.
* Align info EventTile and normal EventTile on IRC layout ([\element-hq#10197](matrix-org/matrix-react-sdk#10197)). Fixes element-hq#22782. Contributed by @luixxiul.
* Fix blowout of waveform of the voice message player on narrow UI ([\element-hq#8861](matrix-org/matrix-react-sdk#8861)). Fixes element-hq#22604. Contributed by @luixxiul.
* Fix the hidden view source toggle on IRC layout ([\element-hq#10266](matrix-org/matrix-react-sdk#10266)). Fixes element-hq#22872. Contributed by @luixxiul.
* Fix buttons on the room header being compressed due to long room name ([\element-hq#10155](matrix-org/matrix-react-sdk#10155)). Contributed by @luixxiul.
* Use the room avatar as a placeholder in calls ([\element-hq#10231](matrix-org/matrix-react-sdk#10231)).
* Fix calls showing as 'connecting' after hangup ([\element-hq#10223](matrix-org/matrix-react-sdk#10223)).
* Prevent multiple Jitsi calls started at the same time ([\element-hq#10183](matrix-org/matrix-react-sdk#10183)). Fixes element-hq#23009.
* Make localization keys compatible with agglutinative and/or SOV type languages ([\element-hq#10159](matrix-org/matrix-react-sdk#10159)). Contributed by @luixxiul.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants