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 cancel download, content not clearing, search preview content type #1717

Merged
merged 8 commits into from
Nov 10, 2021

Conversation

FSM1
Copy link
Contributor

@FSM1 FSM1 commented Nov 8, 2021

In attempting to cause #1499 to occur, I fixed a couple of other small issues.

@render
Copy link

render bot commented Nov 8, 2021

@render
Copy link

render bot commented Nov 8, 2021

@render
Copy link

render bot commented Nov 8, 2021

@github-actions github-actions bot added the Type: Bug Fix Added to PRs if they are addressing a bug label Nov 8, 2021
@FSM1 FSM1 enabled auto-merge November 9, 2021 09:45
@asnaith
Copy link
Member

asnaith commented Nov 10, 2021

I wasn't too sure what you meant by the content type being guessed if not explicitly set. I tried removing some of the file extensions (eg remove .jpg) to see if it would still show an image icon and it did but it also currently does that in prod some I'm assuming it's not in relation to that?

Also, the part about the preview window being cleared after the effect is run - which effect is that and how do I invoke that?

@FSM1
Copy link
Contributor Author

FSM1 commented Nov 10, 2021

I wasn't too sure what you meant by the content type being guessed if not explicitly set. I tried removing some of the file extensions (eg remove .jpg) to see if it would still show an image icon and it did but it also currently does that in prod some I'm assuming it's not in relation to that?

The content-type guessing only happens if the file's content-type field gets mangled (for instance all HEIC images that are uploaded from a windows machine have their content-type set to the generic application/octet-stream instead of image/heic which leads to the preview failing to display. In these cases we need to try and guess the content-type based on the file's extension. This had been wired up to happen for bucket/ls responses, but was not implemented for search results, leading to previews of certain files failing when trying to preview these from the search result list.

Also, the part about the preview window being cleared after the effect is run - which effect is that and how do I invoke that?
This error (and the fix) can be tested in the following manner:
On staging:

  1. Preview a file with a valid preview renderer
  2. Preview next/previous file until one with no preview renderer is selected.
  3. Attempt to download the binary of the file with no preview
  4. No download is initiated, and the binary saved to disk is actually the binary of the last previewed file

After this fix, each time the user switches to a new file to be previewed, the file contents are cleared and only populated if a valid preview renderer is available. When the user downloads the file with no preview, the download request is correctly initiated and the correct binary is saved to disk.

@@ -215,8 +215,8 @@ const FilePreviewModal = ({ file, nextFile, previousFile, closePreview, filePath
bucketId = bucket.id
}

setFileContent(undefined)
Copy link
Collaborator

@Tbaut Tbaut Nov 10, 2021

Choose a reason for hiding this comment

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

I'm not sure if this is related, but when I browse a folder content from the file preview, using the arrows, I keep getting an error "an error occurred while generating the preview" if I don't wait for the initial file to load.

Here an example. I browse slowly (e.g I wait for the file to be previewed), all image show. Then I start clicking faster, so the file preview don't have time to load and show the page, then the error appears.

preview.mp4

Copy link
Contributor Author

@FSM1 FSM1 Nov 10, 2021

Choose a reason for hiding this comment

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

Unrelated to this, and should be dealt with in a separate issue - #1725

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Thanks, then we're good to go!

@FSM1 FSM1 merged commit 9ad4c69 into dev Nov 10, 2021
@FSM1 FSM1 deleted the fix/file-name-repeated-in-the-path-1499 branch November 10, 2021 12:14
@asnaith
Copy link
Member

asnaith commented Nov 10, 2021

@FSM1 Thank you for the explanation of the change. I wasn't aware of the "guessing" type or the other issue with wrong file being downloaded so that was very insightful :)

@FSM1
Copy link
Contributor Author

FSM1 commented Nov 10, 2021

@FSM1 Thank you for the explanation of the change. I wasn't aware of the "guessing" type or the other issue with wrong file being downloaded so that was very insightful :)

Yeah the content-type guessing was a temporary work around from the early days of the project, when the API was not storing this data. Once this got resolved and the content-type was correctly being returned, I was going to remove the guessing code, but eventually decided to leave it in for weird cases like the above referenced one (HEIC images from windows).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix Added to PRs if they are addressing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error.error is undefined when canceling a download
5 participants