-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
draft: impl workspace trust #14668
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
base: master
Are you sure you want to change the base?
draft: impl workspace trust #14668
Conversation
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 was working on this feature as well but never got the branch over the finish line https://github.com/the-mikedavis/helix/tree/workspace-trust. We need a new UI component for selecting whether or not you want to trust. The picker is not the right tool. See the Select component in that branch. I may just work on landing that change in master first since we can use it for LSP window/showMessageRequest independently of this feature
fixes config test not passing when the lock file's parent directory doesn't exist
It does seem much better. I'll try to integrate it.
Yes that would be nice if you could. For now I'll add it temporarily and remove it when/if it gets merged into master. If I can help with it in any way let me know. |
to be removed when an equivalent feature gets merged to master source: https://github.com/the-mikedavis/helix/tree/workspace-trust
|
Edited the original pull request with new screenshots. The select component looks much better. Only problem is the implementation currently ignores the styling of the elements. |
|
I think there should be something to disallow config overrides. |
The implementation already ignores workspace config if the workspace is untrusted. |
|
^^ |
…instead of pushing components themselves
|
added a small todo list |
8a33647 to
9758b8a
Compare
Currently it is a proof concept of workspace trust. It adds a configuration option
editor.workspace-trustwhich can be either:always- always trust the workspace (current behavior of helix)ask- show a trust dialog whenever you open a new workspace (new default)manual- same thing as ask but without asking (i.e. the user has to bring up the trust dialog themselves via:trust-dialog, or use the commands:trust-workspace/:untrust-workspace)never- never trust workspaces.When a workspace is untrusted, you can't use LSPs/debuggers/formatters and workspace config.
It's implemented via a global trust database which is backed by a file.
Some things I'd like feedback about:
Currently it's implemented in a TOML file in
XDG_STATE_HOMEwith fall back to the data directory on windows. Writes are guarded by a.lockfile and are done atomically via fsync and tempfile persistence. There's no guards for reading since the writes are atomic.The first read/write to the database result in it being cached in memory. Any updates to the database from outside the process are not reflected on this cache. Any write to the database within the process updates the in memory cache. The in memory cache is implemented with arc_swap.
Note: if we bump rust to 1.89.0 the dependency on
fs2can be removed in favor of theFile::lockAPI.Old database
A TOML file in `~/.local/share/trust_db.toml` (and appropriate path on windows) guarded by a `.lock` file on each read & write. When you write to the database, it serializes the entire database and writes it (obviously not ideal). The lock mechanism could be replaced with https://github.com/untitaker/rust-atomicwrites (or with a custom implementation for atomic files via rename) for more reliablity and read performance.Uncolored (old)
Old trust dialog
For workspaces:Perhaps there should be a new widget for dialogs?Currently using the select component from https://github.com/the-mikedavis/helix/tree/workspace-trustShould we auto start LSPs and auto reload config when the user trusts/untrusts the workspace, or should we let them do :lsp-restart and :config-reload manually?
Perhaps the
neveroption should be removed and users should just use themanualoption if they want this behavior?and of course any other feedback is also welcome.
Todo list for what needs to be done before this is ready for review:
Do not panic when serialization/deserialization of the database failInvestigate whyjob::dispatch_blockingin theDocumentDidOpenhook hangs when opening a singular document withhx(currently it usestokio::spawnwithjob::dispatchto work around this)Make the lsp server turn on when you trust for the first time & same thing with but with reloading configworkspace-trustoption)UseOnceLock<ArcSwap>instead ofArcSwapOptioninSimpleDbcheck the reliablity of testing that a document is in a workspace withPath::starts_with(inhelix-view/src/editor.rs)auto trust newly created files the user is writing? (if they're not already in a workspace of course)Selectcomponent or something equivalent to be added to master and use it instead of the currentSelectcomponent