Fetch events that read receipts point to if we don't have them #3960
Conversation
As the comment hopefully explains, this is a giant hack to work around lack of MSC3981 support and hopefully remove some spurious unread dots on threads. With this code, the unread dots appear briefly and then go away again once the event in question is fetched which is not fantastic but better then a perma-stuck dot.
| * in the delicious Typescript fumes. | ||
| */ | ||
| private async checkForMissingReceiptEvent(): Promise<void> { | ||
| if (this.isCheckingForMissingReceiptEvent) return; |
There was a problem hiding this comment.
This feels like it might want to queue/debounce rather than drop given the async action may be looking for eventId123 when the RR has moved onto eventId456 via a gappy sync during startup
There was a problem hiding this comment.
I have to say I'm not enthusiastic about this hack. We already have millions of /event/ requests to work around some other failure to land an MSC; it causes load on the server to serve all the requests, and quite a lot of load on the client in terms of making the requests, attempting to decrypt them, failing to decrypt them, checking key backup, etc etc.
I can see the need for a temporary workaround for lack of MSC3981 but my concern is that, as soon as this lands, the problem is out of sight and it's just another large nail in the coffin of Element's performance that we never get round to doing anything about.
Is there a world in which we can instead prioritise enabling MSC3981, and maybe make this hack opt-in to help test it?
| @@ -624,6 +626,8 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM | |||
| } | |||
| } | |||
|
|
|||
| this.checkForMissingReceiptEvent(); | |||
There was a problem hiding this comment.
is it deliberate that this lacks an await? If so, might be good to comment it to highlight and explain why.
There was a problem hiding this comment.
It wasn't, but it now has the throttle where the wrapper doesn't return a promise.
src/models/thread.ts
Outdated
|
|
||
| try { | ||
| const myThreadedReceipt = this.getLatestReceipt(this.client.getUserId()!, true); | ||
| if (myThreadedReceipt && this.timeline.length > 0 && !this.findEventById(myThreadedReceipt.eventId)) { |
There was a problem hiding this comment.
why the condition on timeline.length ? A comment would help.
involves adding a throttle function (manually to avoid a dependency on lodash).
Mm, I share your thoughts, especially having the existing set of thread related things which have gained odd workarounds for lack of server support and then presumably been forgotten. I've picked up 3981 and I don't think anything should actually stand in its way now. Synapse's impl is, afacis, correct, so it really should just be a question of FCPing, getting ticks and then turning the flag on in Synapse by default I think. As a counterpoint, it should be fairly minimal nail as it will only trigger for specific threads that are broken, but yeah, straws and camels and all that. If we were to make it opt-in I think it would just be for people that were desperate to unstick the unread dots: I don't really think it would help test anything as the impl is sufficiently different between this and actually having MSC3981. |
to make sonarcloud happy
|
So the conclusion here is that we're not going to merge this. Obviously it's still here if we do need to change our minds, but lets concentrate on trying to get the MSC through so we can get a synapse out with 3981 on by default in Jan. |
As the comment hopefully explains, this is a giant hack to work around lack of MSC3981 support and hopefully remove some spurious unread dots on threads. With this code, the unread dots appear briefly and then go away again once the event in question is fetched which is not fantastic but better then a perma-stuck dot.
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes