-
-
Notifications
You must be signed in to change notification settings - Fork 668
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
msg list: Download file on clicking file url. #3331
Conversation
8f08cfb
to
e96bb43
Compare
Thanks @jainkuniya ! Quick comments (copying what I told you today in PMs and audio):
|
This is implementing #3303 |
@@ -172,3 +172,9 @@ export const autocompleteUrl = (value: string = '', protocol: string, append: st | |||
: ''; | |||
|
|||
export const isValidUrl = (url: string): boolean => urlRegex({ exact: true }).test(url); | |||
|
|||
export const isUrlFilePath = (url: string) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not robust enough.
For example, mysite.com/welcome.php
does match it but should not be downloaded.
Usually, the distinction between a downloadable file and viewable file is made based on the mime type, and often the server can 'enforce' (or maybe more correctly 'suggest') the downloadability of a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! I agree it's not robust.
But to get mime type
(from header) we need to do a fetch (send a request), and let's say if file is 1mb, then first 1mb (and time) is gone for checking mime type
and if it is downloadable file then download it (again 1mb + time).
It is not clear to me if the behavior, even implemented perfectly, is desirable. This does not match the behavior of serval chat apps I did test, including Slack I am elaborating in #3333 |
I consider this a better (though different) solution to the same problem #3495 |
So that we can use this url to download file instead of opening it in browser.
Next we also need to add a method for downloading files. After extracting it to utils, downloadFile can be added here.
For realms hosted on zulipchat, auth is required to access files. So all the uploaded files can't be opened in the browser just by the link. On opening such link in browser, redirection to the login screen happens. So to make file accessing easier, let's download them. Fixes: zulip#3303
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnprice I have rebased it.
Let's discuss on few points:
- Logic for deciding, if url is downloadable file or not.
- Should we always download file on clicking url.
Cool, thanks @borisyankov for the review and the debugging on #3303 , and thanks @jainkuniya for the updates. I agree with Boris that this isn't the right direction for a fix; we'll continue on the issue #3303 and on #3495 . |
No description provided.