-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix duplicate toasts appearing for the same call if two events appear. #31693
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
Conversation
| beforeEach(() => { | ||
| jest.spyOn(SettingsStore, "getValue").mockReturnValue(true); | ||
| jest.spyOn(SettingsStore, "getValue").mockImplementation((key, ...params) => { | ||
| if (key === "notificationsEnabled") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flyby fix: jest.spyOn(SettingsStore, "getValue").mockReturnValue(true); is evil and not to be used.
BillCarsonFr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions on the logic about getting the parent event
src/Notifier.ts
Outdated
| const rtcSession = room && cli.matrixRTC.getRoomSession(room); | ||
| if (rtcSession?.slotDescription?.application == "m.call") { | ||
| // If we're already joined to the session, don't notify. | ||
| if (rtcSession.memberships.some((m) => m.userId === cli.getUserId())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked when this was extracted to a const with a good name ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix'd. I agree :)
src/Notifier.ts
Outdated
| key: getIncomingCallToastKey(eventId, roomId), | ||
|
|
||
| const callMembership = room?.findEventById(getReferencedMembershipEventId); | ||
| if (!callMembership) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it tries to get the parent event, and if it is not there it just logs and silently continue?
Why trying to get it? Maybe it should use fetchRoomEvent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this just to catch the edge case. I suspect in 99.9999% of cases the client will have the call event before the notif as that's the sent order, and sync should be storing them in sequence. Just in case though there is now a call to fetchRoomEvent.
src/Notifier.ts
Outdated
| // If we cannot determine the key, we'll accept it but assume it's empty string. | ||
| // This means if you have malformed notifications or call memberships your notifications | ||
| // will overwrite, but the solution to that is to use well-formed events. | ||
| const callId = callMembership?.getContent<SessionMembershipData>().call_id ?? ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be a left membership content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be a bit weird to send a notif on for a left call membership?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
butt call? Immedialty stopped :D
BillCarsonFr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for me.
Maybe we want to create a separate issue to talk about handling of wild rtc notifications sent by misbehaving clients? not related to any membership? when no slots?
Aye, let's do that. |
…880) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [element-hq/element-web](https://github.com/element-hq/element-web) | patch | `v1.12.9` → `v1.12.10` | --- ### Release Notes <details> <summary>element-hq/element-web (element-hq/element-web)</summary> ### [`v1.12.10`](https://github.com/element-hq/element-web/releases/tag/v1.12.10) [Compare Source](element-hq/element-web@v1.12.9...v1.12.10) #### ✨ Features - Support additional\_creators in /upgraderoom (MSC4289) ([#​31934](element-hq/element-web#31934)). Contributed by [@​andybalaam](https://github.com/andybalaam). - Update room header icon for world\_readable rooms ([#​31915](element-hq/element-web#31915)). Contributed by [@​richvdh](https://github.com/richvdh). - Show an icon in the room header for shared history ([#​31879](element-hq/element-web#31879)). Contributed by [@​richvdh](https://github.com/richvdh). - Remove "history may be shared" banner. ([#​31881](element-hq/element-web#31881)). Contributed by [@​kaylendog](https://github.com/kaylendog). - Allow dismissing 'Key storage out of sync' temporarily ([#​31455](element-hq/element-web#31455)). Contributed by [@​andybalaam](https://github.com/andybalaam). - Add `resolutions` entry for `matrix-widget-api` to package.json ([#​31851](element-hq/element-web#31851)). Contributed by [@​toger5](https://github.com/toger5). - Improve visibility under contrast control mode ([#​31847](element-hq/element-web#31847)). Contributed by [@​t3chguy](https://github.com/t3chguy). - Unread Sorting - Add option for sorting in `OptionsMenuView` ([#​31754](element-hq/element-web#31754)). Contributed by [@​MidhunSureshR](https://github.com/MidhunSureshR). - Unread sorting - Implement sorter and use it in the room list store ([#​31723](element-hq/element-web#31723)). Contributed by [@​MidhunSureshR](https://github.com/MidhunSureshR). - Allow Element Call widgets to receive sticky events ([#​31843](element-hq/element-web#31843)). Contributed by [@​robintown](https://github.com/robintown). - Improve icon rendering accessibility ([#​31791](element-hq/element-web#31791)). Contributed by [@​t3chguy](https://github.com/t3chguy). - Add message preview toggle to room list header option ([#​31821](element-hq/element-web#31821)). Contributed by [@​florianduros](https://github.com/florianduros). #### 🐛 Bug Fixes - \[Backport staging] Fix room list not being cleared ([#​32438](element-hq/element-web#32438)). Contributed by [@​RiotRobot](https://github.com/RiotRobot). - Fix failure to update room info panel on joinrule change ([#​31938](element-hq/element-web#31938)). Contributed by [@​richvdh](https://github.com/richvdh). - Throttle space notification state calculation ([#​31922](element-hq/element-web#31922)). Contributed by [@​dbkr](https://github.com/dbkr). - Fix emoji verification responsive layout ([#​31899](element-hq/element-web#31899)). Contributed by [@​t3chguy](https://github.com/t3chguy). - Add patch for linkify to fix doctype handling ([#​31900](element-hq/element-web#31900)). Contributed by [@​dbkr](https://github.com/dbkr). - Fix rooms with no messages appearing at the top of the room list ([#​31798](element-hq/element-web#31798)). Contributed by [@​MidhunSureshR](https://github.com/MidhunSureshR). - Fix room list menu flashes when menu is closed ([#​31868](element-hq/element-web#31868)). Contributed by [@​florianduros](https://github.com/florianduros). - Message preview toggle is inverted in room list header ([#​31865](element-hq/element-web#31865)). Contributed by [@​florianduros](https://github.com/florianduros). - Fix duplicate toasts appearing for the same call if two events appear. ([#​31693](element-hq/element-web#31693)). Contributed by [@​Half-Shot](https://github.com/Half-Shot). - Fix ability to send rageshake during session restore failure ([#​31848](element-hq/element-web#31848)). Contributed by [@​t3chguy](https://github.com/t3chguy). - Fix mis-alignment of `Threads` right panel title ([#​31849](element-hq/element-web#31849)). Contributed by [@​t3chguy](https://github.com/t3chguy). - Unset buttons does not include color inherit ([#​31801](element-hq/element-web#31801)). Contributed by [@​Philldomd](https://github.com/Philldomd). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4zLjYiLCJ1cGRhdGVkSW5WZXIiOiI0My4zLjYiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbImF1dG9tZXJnZSIsImltYWdlIl19--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/3880 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
We can follow the referenced event to find the true call ID, and de-duplicate upon that rather than the notification eventId which is obviously unique each time.
Checklist
public/exportedsymbols have accurate TSDoc documentation.