Fix requestVerificationDM with chronological pendingEventOrdering

[freaktechnik]

Signed-off-by: Martin Giger martin@humanoids.be

This PR is the result of an issue I ran into while implementing user verification for Thunderbird. I'm not sure this is the correct way to handle the chronological timeline case, but it makes sure that the handler won't run with an unsent temporary event ID, and thus assuming the wrong transaction ID.

CC @uhoreg

---

Here's what your changelog entry will look like:

🐛 Bug Fixes

• Fix requestVerificationDM with chronological pendingEventOrdering (#1943). Contributed by freaktechnik.

[uhoreg]

Thanks for the PR. I think that this almost works, but not quite. If the event status is SENT (the event has been sent to the server, but we haven't received the local echo yet), then isSending will return true, but the event will already have the final event ID, which means that Event.localEventIdReplaced won't get triggered. So I think that it needs to be if (event.isSending() || event.status != EventStatus.SENT) .... Which is, unfortunately, kind of ugly.

[freaktechnik]

Thanks for the PR. I think that this almost works, but not quite. If the event status is SENT (the event has been sent to the server, but we haven't received the local echo yet), then isSending will return true, but the event will already have the final event ID, which means that Event.localEventIdReplaced won't get triggered. So I think that it needs to be if (event.isSending() || event.status != EventStatus.SENT) .... Which is, unfortunately, kind of ugly.

Based on https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/event.ts#L1051 the SDK thinks the ID could still change from the remote echo? Or is that code path irrelevant for events sent by the local session?

[uhoreg]

Yeah, it's a bit confusing there. That event only gets emitted only if this.getId() !== oldId, that is, if the ID actually changes. However, the ID for the event supposed to be set at https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/room.ts#L1655 when the status is also set to SENT.

Unfortunately, it looks like it sets the status to SENT before setting the correct event ID, so you can't just listen for the Event.status event, because when it gets triggered, it won't have the right ID yet. So I think you may need to listen to both Event.status and Event.localEventIdReplaced. Resolve on Event.localEventIdReplaced and reject if Event.status gets triggered with a status of CANCELLED.

[uhoreg]

Sorry, I meant to look at this earlier. It looks good to me. I'll ask for review from someone else on the web team to check it over, but I think it should basically be ready.