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

fix: handle failure to send mls messages (WPB-2300) #15369

Merged
merged 5 commits into from
Jun 20, 2023

Conversation

V-Gira
Copy link
Contributor

@V-Gira V-Gira commented Jun 20, 2023

BugWPB-2300 [Web] On sending a messages to a conversation owned by an offline b-e with MLS the option to retry does dot appear

Issues

  1. when sending a message to an mls conversation, the option to retry do not appear when the owning b-e is offline

  2. same thing when sending a message while internet is offline using api v4

  3. wrong styling in some cases

Solutions

  1. bump core to 40.5.4, see fix: handle error when sending mls message [WBP-2300] wire-web-packages#5254

  2. handle AxiosError as failure to send

  3. make sure error messages appear in red, and unsent messages gray

Comment on lines -812 to +811
if (isBackendError(error)) {
await this.updateMessageAsFailed(conversation, payload.messageId, error);
}
await this.updateMessageAsFailed(conversation, payload.messageId, error);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could alternatively check for Axios errors with isAxiosError(error) but I think we would want to treat any error as a failure to send with the option to retry

@V-Gira V-Gira marked this pull request as ready for review June 20, 2023 15:34
@V-Gira V-Gira requested review from a team and otto-the-bot as code owners June 20, 2023 15:34
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #15369 (f1fcb28) into dev (d083926) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head f1fcb28 differs from pull request most recent head 30bb215. Consider uploading reports for the commit 30bb215 to get more accurate results

@@            Coverage Diff             @@
##              dev   #15369      +/-   ##
==========================================
- Coverage   43.21%   43.21%   -0.01%     
==========================================
  Files         647      647              
  Lines       21784    21782       -2     
  Branches     5006     5004       -2     
==========================================
- Hits         9415     9414       -1     
+ Misses      11165    11164       -1     
  Partials     1204     1204              

@@ -1187,7 +1184,7 @@ export class MessageRepository {
return undefined;
}

private async updateMessageAsFailed(conversationEntity: Conversation, eventId: string, error: BackendError) {
private async updateMessageAsFailed(conversationEntity: Conversation, eventId: string, error: BackendError | any) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we go with just unknown and check what type of error it is inside the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this?

if (isBackendError(error) && error.label === BackendErrorLabel.FEDERATION_REMOTE_ERROR) 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@V-Gira V-Gira merged commit 864efbd into dev Jun 20, 2023
@V-Gira V-Gira deleted the virgile/failure-to-send-mls branch June 20, 2023 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants