-
Notifications
You must be signed in to change notification settings - Fork 906
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
Add limit on the file size that can be opened with Open XEL feature #23841
Conversation
Just to add context here, here are the numbers from testing the open XEL feature: Immediately opens for 200KB file. |
this._notificationService.error(nls.localize('FileTooLarge', "The file is too large to open in profiler. The profiler can open files that are less than 1GB.")); | ||
return false; | ||
} else if (fileSize > fileOpenWarningSize) { | ||
this._notificationService.info(nls.localize('LargeFileWait', "Loading the file might take a moment, because the file is large.")); |
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.
What's the experience like now? Is there a loading spinner already? This notification seems unnecessary if that's the case. Generally we don't try to guess whether an operation is going to take a long time - the closest we may do to something like that is show a progress notification after a short delay saying something like "Opening file..." to show that it's working in the background.
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.
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.
That's unfortunate - alright then taking this for now and creating an issue to follow up post-release with for improving this seems fine. Thanks for verifying!
…23841) * Add limit on the file size that can be opened with Open XEL feature * Add limit on the file size that can be opened and post a notification for large files * Update wording * Use FileService interface instead of fs to fix layering rules
This PR works on fixing #23806.
For files within 100MB, no notification is posted.
For files from 100MB-1GB, a notification is posted:
For files more than 1GB, an error is thrown and file is not loaded: