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

Improve behavior of sending messages with attachments #1877

Conversation

Silver-IT
Copy link
Member

@Silver-IT Silver-IT commented Mar 5, 2024

@Silver-IT Silver-IT requested a review from taoeffect March 5, 2024 04:59
@Silver-IT Silver-IT self-assigned this Mar 5, 2024
@Silver-IT Silver-IT linked an issue Mar 5, 2024 that may be closed by this pull request
Copy link

cypress bot commented Mar 5, 2024

Passing run #1980 ↗︎

0 110 8 0 Flakiness 0

Details:

Merge 828e9e4 into ef7f542...
Project: group-income Commit: 4f94ae4bce ℹ️
Status: Passed Duration: 11:01 💡
Started: Mar 14, 2024 12:23 PM Ended: Mar 14, 2024 12:34 PM

Review all test suite changes for PR #1877 ↗︎

@Silver-IT Silver-IT changed the base branch from master to sebin/task/#1846-implement-file-attachment-in-chat March 5, 2024 06:18
@Silver-IT Silver-IT marked this pull request as draft March 6, 2024 00:14
@Silver-IT Silver-IT changed the base branch from sebin/task/#1846-implement-file-attachment-in-chat to master March 6, 2024 11:18
@Silver-IT Silver-IT requested a review from SebinSong March 6, 2024 11:18
@Silver-IT Silver-IT marked this pull request as ready for review March 6, 2024 11:18
@Silver-IT Silver-IT marked this pull request as draft March 6, 2024 12:28
@Silver-IT Silver-IT marked this pull request as ready for review March 6, 2024 22:43
@Silver-IT
Copy link
Member Author

Having tested more about the the error message "Image corrupt or truncated", the main reason is because the file is damaged. This issue is what @SebinSong already reported here.

I guess it's related to the browser because Google Chrome doesn't raise that issue but Firefox and Safari do.
However, I will investigate more.

@taoeffect
Copy link
Member

@Silver-IT

Having tested more about the the error message "Image corrupt or truncated", the main reason is because the file is damaged. This issue is what @SebinSong already reported here.

Sorry, I can't tell what exactly in Sebin's video shows the same behavior as here. In Sebin's video the images seem to upload with no problem.

Also, I don't believe the problem is that the image is corrupted, for a few reasons:

  1. The image opens fine in an image viewer
  2. I tried other PNGs and got the same error. They all open fine in image viewers too. It's unlikely that they're also corrupted.

@Silver-IT
Copy link
Member Author

@taoeffect, it could seem to be different, but I think it's same in basic. It depends on how much the image is damaged while uploading. Also I can say that not all the images are damaged while uploading. Some are damaged, and some are not.

I attached the three screenshots below. Look at the second image which I got in Google Chrome browser. Two images are uploaded in Firefox browser, and they are however opens fine in Google Chrome. That's why I can say it depends on how much the image is damaged.

image

@taoeffect
Copy link
Member

@Silver-IT If the images upload fine in Google Chrome, and then display fine in Firefox, it clearly means the images are not damaged.

This must be some other problem. Please search online to see what the problem could possibly be. Try asking Claude 3. Or maybe @SebinSong or @corrideat can help?

@corrideat
Copy link
Member

corrideat commented Mar 11, 2024

@Silver-IT I've written a small function for testing uploads / downloads, and it seems like the fileUpload / fileDownload selectors are working fine.

async function test() { const x = new Uint8Array((1<<20) + ((Math.random()*512) | 0)); x.forEach((_, i) => x[i] = Math.random()*256|0); console.log('Uploading'); const y = await sbp('chelonia/fileUpload', new Blob([x]), { cipher: 'aes256gcm' }); console.log('Uploaded'); return JSON.stringify(Array.from(new Uint8Array(await ((await sbp('chelonia/fileDownload', y)).arrayBuffer())))) === JSON.stringify(Array.from(x)); }

@corrideat
Copy link
Member

If I had to guess, I'd think that it's got to do with the fetch used to convert back from an object URL to a blob, although I could be wrong. I'd try the following:

  1. Keep a reference to a Blob (or File, which inherits from Blob) around until the upload is complete.
  2. Use that reference directly
  3. Also, this allows you to immediately revoke the object URL after the preview loads, which can be good for performance, because it allows for the object to be garbage collected.

@Silver-IT
Copy link
Member Author

@corrideat, thanks for your kind help. I also think that fileUpload / fileDownload works fine. If it is wrong, it must not be working in Google Chrome browser either.

I also tested keeping the Blob reference until the upload is finished and even more. That Blob (which is used for uploading, and not the one which is created by fileDownload) seems to keep the right content because it doesn't shows the wrong image in the chat page and also I can download the correct image using its ObjectURL.

At the moment, I couldn't guess the which one is making this kinda weird problem.

@corrideat
Copy link
Member

thanks for your kind help. I also think that fileUpload / fileDownload works fine. If it is wrong, it must not be working in Google Chrome browser either.

It could be an issue that's only visible in Firefox. Besides being different implementations, the code is also different. For example, when uploading Chrome may use a stream and Firefox will not. In reality, using a stream in Chrome requires HTTP 2, so unless you've done something to enable it, the code should be the same (although, if you want to be sure, you can disable the check here: https://github.com/okTurtles/group-income/blob/64e8a2b0892cddc6898b10cf16ce272f8fa86d37/shared/domains/chelonia/files.js#L52C3-L52C41, which will make Chrome and Firefox use the same logic). I don't think that this will help, but you can try.

The next thing you can do is upload a file that gets corrupted, download it, and then inspect the original and the downloaded version side-by-side. That could give some information about what's happening. Since the issue happens consistently when showing a file, my guess is that something goes wrong during the upload.

@taoeffect
Copy link
Member

Maybe somehow the image isn't being drawn or displayed incorrectly?

I ask because @corrideat claims that the files are being uploaded and downloaded correctly on Firefox (the same byte sequence is being received...)

@SebinSong
Copy link
Collaborator

@taoeffect , @Silver-IT
My first guess when I ran into this issue was the same as below comment of @corrideat

If I had to guess, I'd think that it's got to do with the fetch used to convert back from an object URL to a blob

I've just done some research and it seems like instead of using URL.createObjectURL for the browser to convert/download Blob we receive via sbp('chelonia/fileDownload'), we can also use FileReader.readAsDataURL() API too, even though apparently the solution will look more verbose.

Let me try this in my local dev and share the result of how it goes.

@SebinSong
Copy link
Collaborator

@taoeffect @Silver-IT
I just created this method that uses FileReader() API and tried downloading using it but got the same issue.
Now it feels like the cause of the issue isn't related to the way front-end handles blob.

async getDateURLOfAttachment (attachment) {
    const blob = await sbp('chelonia/fileDownload', attachment.downloadData)
    const reader = new FileReader()

    return new Promise((resolve) => {
      reader.onload = () => { resolve(reader.result) }
      reader.readAsDataURL(blob)
    })
  }

This is the before / after of the image I had an issue with, when trying in Firefox Dev Edition.

[ before ]

[ after ]

@Silver-IT
Copy link
Member Author

Having tested this issue more in different browsers, I can say it works fine in Google Chrome and Brave browser. And in Opera browser, I can not even upload any files, and in other browsers like Firefox and Safari, the file is damaged.

Based on my poor understanding of this kind of area, I am not sure but my opinion is that the problem is related to this one.

It could be an issue that's only visible in Firefox. Besides being different implementations, the code is also different. For example, when uploading Chrome may use a stream and Firefox will not. In reality, using a stream in Chrome requires HTTP 2, so unless you've done something to enable it, the code should be the same (although, if you want to be sure, you can disable the check here: https://github.com/okTurtles/group-income/blob/64e8a2b0892cddc6898b10cf16ce272f8fa86d37/shared/domains/chelonia/files.js#L52C3-L52C41, which will make Chrome and Firefox use the same logic). I don't think that this will help, but you can try.

cc: @taoeffect, @corrideat, @SebinSong

@corrideat
Copy link
Member

@Silver-IT: I've managed to find the issue. It was an error in the encryption function, that resulted in sometimes (depending on how the input was chunked) skipping over some data. This has been fixed in ApelegHQ/ts-rfc8188#5 and I've also pushed a new commit that updates the package and addresses this issue.

Copy link

socket-security bot commented Mar 14, 2024

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@exact-realty/[email protected]

View full report↗︎

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

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

Nice work @Silver-IT & @corrideat!

@Silver-IT I tested this to see if #1845 is closed, and with throttling enabled we can see that the issue isn't closed:

file-upload.issue.compressed.mov

For whatever reason, the messages sent after the file upload starts don't get sent immediately. They need to be sent immediately instead of waiting for the file upload to finish or fail.

Also, as a separate unrelated issue (but something to not forget), if we are on a cellular connection (or a slow connection), we don't want to autodownload the files. Instead, a download button overlay should appear over the file, and only if it's clicked should the file preview be downloaded. But we can save that for a separate issue and PR.

@Silver-IT
Copy link
Member Author

@taoeffect, I think the delay is because of that throttle works for all the network transactions. Not only for file uploading.
Also based on the record video, I updated the code to handle error which is caught when uploading attachment could be failed.

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

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

Approved!

Great work @Silver-IT, @SebinSong and @corrideat!

It looks like the issue with messages not appearing when they should when throttling is turned on is a bug in Firefox, not in this code, as it works correctly in Brave with throttling enabled.

Copy link
Collaborator

@SebinSong SebinSong left a comment

Choose a reason for hiding this comment

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

Great work @Silver-IT !

@taoeffect taoeffect merged commit 7c8abbd into master Mar 15, 2024
4 checks passed
@taoeffect taoeffect deleted the 1845-improve-behavior-of-sending-messages-with-large-attachments branch March 15, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants