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.

Sign up for GitHub

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

Jump to bottom

feat: implement retry mechanism for asset messages [FS-1634] #14890

Merged
merged 15 commits into from
Mar 28, 2023
Merged

feat: implement retry mechanism for asset messages [FS-1634] #14890

merged 15 commits into from
Mar 28, 2023

Conversation

V-Gira
Copy link
Contributor

@V-Gira V-Gira commented Mar 24, 2023 •
edited by github-actions bot

Sub-taskFS-1634 [web] implement retry mechanism for asset messages that have completely failed to send

Feature

Peek 2023-03-24 18-23

Changes

  • We store the asset blob in an observable in db
  • The blob is stored when a message fails to be injected
  • When retrying to send the asset message, the upload file method is called with the stored blob and the original message id is conserved to replace the message in db

@codecov
Copy link

codecov bot commented Mar 24, 2023 •
edited

Codecov Report

Merging #14890 (8d84b4d) into dev (46f5b50) will decrease coverage by 0.03%.
The diff coverage is 43.47%.

❗ Current head 8d84b4d differs from pull request most recent head 09c05d5. Consider uploading reports for the commit 09c05d5 to get more accurate results

@@            Coverage Diff             @@
##              dev   #14890      +/-   ##
==========================================
- Coverage   42.94%   42.91%   -0.03%     
==========================================
  Files         623      623              
  Lines       21297    21320      +23     
  Branches     4891     4900       +9     
==========================================
+ Hits         9145     9150       +5     
- Misses      10993    11001       +8     
- Partials     1159     1169      +10

@V-Gira V-Gira marked this pull request as ready for review March 27, 2023 10:23
@V-Gira V-Gira requested review from a team and otto-the-bot as code owners March 27, 2023 10:23
atomrc
atomrc reviewed Mar 27, 2023
View reviewed changes
src/script/components/MessagesList/Message/MessageWrapper.tsx
@@ -110,6 +111,8 @@ export const MessageWrapper: React.FC<MessageParams & {hasMarker: boolean; isMes
incomingQuote && isOutgoingQuote(incomingQuote) ? (incomingQuote as OutgoingQuote) : undefined;

await messageRepository.sendTextWithLinkPreview(conversation, messageText, mentions, quote, messageId);
} else if (file) {
await messageRepository.retryUploadFile(conversation, file, firstAsset.isImage(), message.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm that this call will actually remove the blob from the database if successful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, I adress it in this commit: 8d84b4d

przemvs
przemvs reviewed Mar 28, 2023
View reviewed changes
src/script/conversation/MessageRepository.ts
fileData: file,
});
} catch (error) {
if ((error as any).type !== ConversationError.TYPE.MESSAGE_NOT_FOUND) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should handle error better, instead of use error as any :) And we should change it everywhere :)

@V-Gira V-Gira merged commit 0727207 into dev Mar 28, 2023
@V-Gira V-Gira deleted the virgile/resend-assets branch March 28, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@przemvs przemvs przemvs left review comments

@atomrc atomrc atomrc approved these changes

@otto-the-bot otto-the-bot Awaiting requested review from otto-the-bot otto-the-bot is a code owner

Assignees
No one assigned
Labels
👕 size: M type: feature / request ✨
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@V-Gira @atomrc @przemvs @otto-the-bot