-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix(trace): Allow zip files of other mime types on paste #37907
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
Conversation
| return; | ||
| const file = files.item(0)!; | ||
|
|
||
| // Do best effort to select the first valid trace valid, if not, rely on error reporting of sw |
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.
Pasting multiple files looks like a user error, we can bail sooner or leave it as is.
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.
It definitely is a user error. However, why not try to find/load the valid trace file from this fileslist? I think that's preferable over not showing anything.
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.
I wonder if this regresses users that renamed their traces to foo.trace, what do we do to those?
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.
Good point.
However, in this case we just try to find the first .zip file, if not found it will just pick the first one and send it to the server to handle (possibly returning an error). So, processing will just work fine for them. It is just that those users don't 'benefit' from this 'best effort'
| if (!e.clipboardData?.files.length) | ||
| return; | ||
| for (const file of e.clipboardData.files) { | ||
| if (file.type !== 'application/zip') |
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.
I'd just add application/x-zip-compressed for old Windows systems.
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.
It's not just for old windows systems, I am experiencing this mime-type behaviour on Windows 11.
But then, there are probably other cases we need to consider depending on browser/operating system (https://mime-type.com/file-extension/zip/). So checking file extension seems easiest and most reliable here?
|
For potentially breaking changes, we should definitely start with an issue |
|
@pavelfeldman This PR already references to existing issue: #37906. |
Detection of zip files with just mime type is not quite enough, use filename instead to get mimetype. Moved this logic into
processTraceFiles. Here we now select the first valid ZIP file. If none found, send to sw anyway, and show error message later.Also removed unused state
uploadedTraceName.Fixes: #37906