Skip to content
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

Disable "Show Local" button, add --disable-file-uploads option and add file operation trace level logs #6557

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

jeffmax
Copy link
Contributor

@jeffmax jeffmax commented Nov 29, 2023

This PR replaces PR #6529. It adds an option to disable file uploads (--disable-file-uploads) and removes the "Show Local" button on the dialog box for relevant file actions for both --disable-file-uploads and --disable-file-downloads options (fix #5118). It also adds trace level log statements for files that are read from or saved to disk.

…ble and add trace logs for file read/write operations.
@jeffmax jeffmax requested a review from a team as a code owner November 29, 2023 21:57
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thank you again.

Some unrelated e2e tests are failing, maybe the switch from text to text-is? Could be it was matching on partial strings before.

@code-asher
Copy link
Member

One quirk: seems I can edit local files by dragging them to the editor pane, do we want to block that as well?

@jeffmax
Copy link
Contributor Author

jeffmax commented Dec 4, 2023

I was looking at that as well.. best I could tell that was actually editing them in place (on the client machine with the client needing to give the browser permissions on the directory) and not actually uploading them to the server. Does that match what you are seeing?

@code-asher
Copy link
Member

Yup, that is what I was seeing. My reasoning was that since we remove the "show local" button for opening files (which seems to be doing the same thing) we would want to prevent this as well otherwise it is possible to bypass the button removal by dragging instead.

You bring up a good point that it is not actually uploading anything though so leaving it as-is seems fine to me.

@jeffmax
Copy link
Contributor Author

jeffmax commented Dec 6, 2023

Is there anything else I need to do, is that prettier error real?
Re: other failing tests- what is failing? I did check other code for use of the navigateMenus and thought I fixed things but it may have been lost when I rebased off the old PR.

@code-asher
Copy link
Member

It claims "should show the Integrated Terminal" and "should have access to VSCODE_PROXY_URI" are failing. Maybe focusTerminal() is not working? Although, looking at the code, it does seem like it should work.

For the Prettier errors, I ran yarn fmt locally and it did update those files so it looks real.

@code-asher
Copy link
Member

Oh wait I think Command Palette needs to be Command Palette... now.

@jeffmax
Copy link
Contributor Author

jeffmax commented Dec 7, 2023

@code-asher I ran yarn fmt and updated the call to navigateMenus for 'Command Palette'.

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #6557 (4203c02) into main (9ba66ec) will not change coverage.
Report is 5 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6557   +/-   ##
=======================================
  Coverage   73.13%   73.13%           
=======================================
  Files          31       31           
  Lines        1906     1906           
  Branches      409      409           
=======================================
  Hits         1394     1394           
  Misses        433      433           
  Partials       79       79           
Files Coverage Δ
src/node/cli.ts 90.90% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c98611...4203c02. Read the comment docs.

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! Awesome to have this in.

@code-asher code-asher merged commit 9622830 into coder:main Dec 7, 2023
10 checks passed
yiliang114 pushed a commit to yiliang114/code-server that referenced this pull request Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat]: Add option to disable "Show Local" while saving
2 participants