vscode icon indicating copy to clipboard operation
vscode copied to clipboard

File dialogs: validate `defaultFilePath` and `defaultFolderPath` for being valid

Open babakfp opened this issue 1 year ago • 5 comments
trafficstars

I was able to reproduce this issue, https://github.com/microsoft/vscode/issues/210344.

Does this issue occur when all extensions are disabled?: Yes

Version: 1.92.2 (user setup) Commit: fee1edb8d6d72a0ddff41e5f71a671c23ed924b9 Date: 2024-08-14T17:29:30.058Z Electron: 30.1.2 ElectronBuildId: 9870757 Chromium: 124.0.6367.243 Node.js: 20.14.0 V8: 12.4.254.20-electron.0 OS: Windows_NT x64 10.0.22631

Steps to Reproduce:

https://github.com/user-attachments/assets/ee239dff-528e-40f0-9431-3d155ecddd7a

babakfp avatar Aug 26 '24 07:08 babakfp

@bpasero this is probably coming from the history service. We could add a check to see if the file exists. Would you like to fix this in the history service or should I fix it the file dialog service?

alexr00 avatar Aug 26 '24 18:08 alexr00

I believe the native dialog would refuse to accept an invalid path that does not exist, so I think the simple dialog could do the same. But I worry that we waste a fs call everytime the dialog opens...

bpasero avatar Aug 27 '24 04:08 bpasero

Yes, I would need to make an fs call each time the dialog opens.

alexr00 avatar Aug 27 '24 17:08 alexr00

Hi 👋

Sorry for the late response.

This is what I expect to happen:

  1. The name of the deleted folder not to get appended constantly.
  2. If the path doesn't exist, the suggested path to be what is in "files.dialog.defaultPath".

babakfp avatar Aug 27 '24 18:08 babakfp

I think its probably best to do some kind of validation at these methods:

https://github.com/microsoft/vscode/blob/4cec36a9d9bdfa4312921e8eb5e6b5c60f82c577/src/vs/workbench/services/dialogs/browser/abstractFileDialogService.ts#L60

https://github.com/microsoft/vscode/blob/4cec36a9d9bdfa4312921e8eb5e6b5c60f82c577/src/vs/workbench/services/dialogs/browser/abstractFileDialogService.ts#L79

And not do it in each component that asks for this path.

Treating as feature request.

bpasero avatar Aug 28 '24 13:08 bpasero

Given that we would need to do an fs call to validate the paths (resulting in a slower experience every time the file picker is opened) and that the scenario shown in the issue would only happen infrequently, I'm going to close this as won't-fix.

alexr00 avatar Dec 12 '24 15:12 alexr00

It's difficult to understand some of the decisions that are made by the vsCode them.

  1. What I reported here, is a bug! (and a UX issue).
  2. It's being treated as a feature!
  3. It's not going to be fixed because of a simple fs call!
  4. This isn't even going to get a config option so that only the people that want it can enable it!

vsCode is already pretty heavy. If you guys are looking for performance improvement, this is not the way.

I request this to be opened and fixed in whatever way possible instead.

I already deal with slow things, having 300ms delay (just as example) is nothing.


I did this:

const fs = require("node:fs")

console.time()
fs.existsSync("C:/Users/Babak/Documents/Code/index.js")
fs.existsSync("C:/Program Files (x86)/Google")
fs.existsSync("C:/laragon/www")
console.timeEnd()

Result:

node index
default: 0.139ms
node index
default: 0.138ms
node index
default: 0.216ms
node index
default: 0.151ms
node index
default: 0.155ms

I watched the video again, and I don't think this has anything to do with fs and whatever. vsCode already failed at opening the deleted folder, which means it knows about that. Also, it keeps suggesting the folder name every time. I don't know about the code behind it, but this looks like a problem that can be solved without extra steps.

Neverming, I'll try to fix this myself rather than asking you guys.

babakfp avatar Dec 12 '24 16:12 babakfp