Utilize EventStatus for message state
#1170
Conversation
There was a problem hiding this comment.
Can you describe which scenarios you identified that would mean a failed message send? Is it only a network error or something else too?
There are duplicated messages. If I write "I am writing this without internet connection" while being offline it looks like one message got sent and one failed:
| this.messagesToSend.set(roomId, undefined); | ||
| this.cardsToSend.set(roomId, undefined); |
There was a problem hiding this comment.
I know it's not directly related to your PR, but i think the initialization of these two Maps is probably better performed inside of this.setPendingMessage(). Is there ever a time when you set a pending message that you would not want to initialize this?
There was a problem hiding this comment.
Good idea! I can move these initializations to setPendingMessage
| @@ -377,114 +379,149 @@ export default class MatrixService extends Service { | |||
| body: string | undefined, | |||
| attachedCards: CardDef[] = [], | |||
| context?: OperatorModeContext, | |||
| clientGeneratedId?: string, // used to resend failed messages | |||
There was a problem hiding this comment.
clientGeneratedId is generated in setPendingMessage and so for non-"resend" scenario, clientGeneratedId will be undefined.
|
Please hold off the review. I was investigating the bug that @jurgenwerk found, then I noticed that events have status ("sending", "sent", and "not_sent"). I think we can use it for pending messages and messages failed to send. And it will look cleaner than adding states in matrix-service. |
|
This PR is ready for review again, @habdelra , @jurgenwerk . I've made a few updates:
I've also tested scenarios involving pending messages and messages that failed to send in staging. Here are screenshots of those tests. Screen.Recording.2024-04-18.at.13.10.40.movScreen.Recording.2024-04-18.at.13.11.42.mov |
FadhlanR
changed the title
EventStatus for message state
| event.content.data = JSON.parse(event.content.data); | ||
| } | ||
| let { event_id: eventId, room_id: roomId, state_key: stateKey } = event; | ||
| eventId = eventId ?? stateKey; // room state may not necessary have an event ID |
There was a problem hiding this comment.
This is confusing to me - how can event id be two different things?
There was a problem hiding this comment.
I think this is highlighting the difference between a message event and room state. the event ID is not necessarily guranteed. the state key is the closest thing room state has to an event ID. (consider an "event" where a user joins a room--this is actually a room state event, and we receive these objects thru the matrix event listener the same as actuall message events)
There was a problem hiding this comment.
Thanks @habdelra for the explanation. I didn't add this line, it comes from legacy code.
|
|
||
| export function onUpdateEventStatus(context: Context) { | ||
| return (e: MatrixEvent, _room: Room, oldEventId?: string) => { | ||
| if (typeof oldEventId !== 'string' || !e.status) { |
There was a problem hiding this comment.
Why are you checking for the type of the oldEventId? Isn't the event id always a string?
There was a problem hiding this comment.
I am curious about this as well--what other type are we expecting here? if this is a fancy check for undefined or null then let's use:
| if (typeof oldEventId !== 'string' || !e.status) { | |
| if (oldEvent == null || !e.status) { |
this is more clear about what you mean then a typeof check
There was a problem hiding this comment.
This is the event listener type: type AnyListener = (...args: any) => any;. I noticed that sometimes the third item in ...args is not oldEventId. However, I updated the parameter to maybeOldEventId: unknown to mitigate confusion e0932ee.
delete-merged-branch
bot
deleted the
cs-6704-error-message-if-fails-to-send
branch
Ticket: CS-6704