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

feat: add option for disabling file downloads #5055

Merged
merged 18 commits into from
Apr 13, 2022
Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Mar 31, 2022

This PR adds a new CLI flag called --disable-file-downloads which removes the "Download..." option from the File Actions (right-clicking on file).

Screenshot

As you can see, there is no "Download..." option here:

image

e2e test ensuring the behavior works as well.
image

e2e test ensuring default behavior (downloads enabled) works:
image

Fixes #2426

Notes

Click to expand

What is the current behavior?

Currently, the default behavior is to allow any file to be downloaded (note: not folders). To do this,

  1. Right-click any file in code-server
  2. Select "Download"

download

What is the desired behavior?

A way to disable this behavior.

The best solution we've come up with is a CLI option such as --disable-file-downloads which would be a boolean flag meaning the default is false, but when you pass it, it is true. You could also explicitly say --disable-file-downloads=true. And because of the way code-server is designed, you'll also be able to set it in your config.yaml like disable-file-downloads: true.

How do we implement this?

There are two pieces of logic to implement:

  1. code-server needs to support a flag called --disable-file-downloads
  2. Code needs to show/hide "Download" based on flag boolean. Default should match current behavior aka true

For 1, we can take a TDD approach and write a test to ensure that:

  • code-server supports a --disable-file-downloads flag
  • code-server defaults to --disable-file-downlaods=true

For 2, we can take TDD but it's a little bit more work:

  • write e2e test that checks for "Download" in menu
  • write failing e2e test that checks for "Download" is not in the menu
  • add logic to remove it from menu based on CLI flags
    • create new context key
    • use in fileActions.contributions.ts
    • test to make sure it works.
    • add disable-file-downloads as arg to serverEnvironmentService
    • add logic based on CLI flag

To do this, I need to create a new context key like this then add it here

Running e2e tests: VSCODE_DEV=1 LOG_LEVEL=debug yarn test:e2e --grep "disabled" --quiet --retries 0 --debug

Seeing these type errors:

[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/services/workingCopy/test/electron-browser/workingCopyBackupService.test.ts(60,9): Argument of type 'TestWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ]   Property 'isEnabledFileDownloads' is missing in type 'TestWorkbenchEnvironmentService' but required in type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/services/environment/electron-sandbox/environmentService.ts(54,14): Class 'NativeWorkbenchEnvironmentService' incorrectly implements interface 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ]   Property 'isEnabledFileDownloads' is missing in type 'NativeWorkbenchEnvironmentService' but required in type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(169,61): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService | SyncDescriptor<INativeWorkbenchEnvironmentService>'.
[VS Code] [watch-client    ]   Property 'isEnabledFileDownloads' is missing in type 'NativeWorkbenchEnvironmentService' but required in type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(177,167): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(217,68): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'IWorkbenchEnvironmentService'.
[VS Code] [watch-client    ]   Property 'isEnabledFileDownloads' is missing in type 'NativeWorkbenchEnvironmentService' but required in type 'IWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(252,62): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(255,41): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(266,39): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'INativeWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(284,101): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'IWorkbenchEnvironmentService'.
[VS Code] [watch-client    ] [16:58:27] Error: /Users/jp/dev/coder/code-server/lib/vscode/src/vs/workbench/electron-sandbox/desktop.main.ts(287,169): Argument of type 'NativeWorkbenchEnvironmentService' is not assignable to parameter of type 'IWorkbenchEnvironmentService'.

Now it's not working 🤔

@jsjoeio jsjoeio added the feature New user visible feature label Mar 31, 2022
@jsjoeio jsjoeio self-assigned this Mar 31, 2022
@github-actions
Copy link

github-actions bot commented Mar 31, 2022

✨ code-server docs for PR #5055 is ready! It will be updated on every commit.

@jsjoeio jsjoeio temporarily deployed to npm March 31, 2022 16:49 Inactive
@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #5055 (5ba6882) into main (3bf470f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5055      +/-   ##
==========================================
+ Coverage   71.30%   71.33%   +0.03%     
==========================================
  Files          30       30              
  Lines        1683     1685       +2     
  Branches      373      374       +1     
==========================================
+ Hits         1200     1202       +2     
  Misses        413      413              
  Partials       70       70              
Impacted Files Coverage Δ
src/node/cli.ts 91.66% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

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

@github-actions
Copy link

github-actions bot commented Mar 31, 2022

✨ code-server dev build published to npm for PR #5055!

  • Last publish status: success
  • Commit: 5ba6882

To install in a local project, run:

npm install @coder/code-server-pr@5055

To install globally, run:

npm install -g @coder/code-server-pr@5055

@jsjoeio jsjoeio force-pushed the jsjoeio/issue-2426 branch 3 times, most recently from 22859c7 to f7c6000 Compare March 31, 2022 17:38
@jsjoeio jsjoeio temporarily deployed to npm March 31, 2022 17:42 Inactive
@jsjoeio jsjoeio temporarily deployed to npm March 31, 2022 20:06 Inactive
@jsjoeio jsjoeio force-pushed the jsjoeio/issue-2426 branch from 53a14d5 to dd9a2c3 Compare March 31, 2022 22:03
@jsjoeio jsjoeio temporarily deployed to npm March 31, 2022 22:08 Inactive
patches/disable-downloads.diff Outdated Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio marked this pull request as ready for review April 4, 2022 21:06
@jsjoeio jsjoeio requested a review from a team April 4, 2022 21:06
src/node/cli.ts Outdated Show resolved Hide resolved
test/e2e/models/CodeServer.ts Show resolved Hide resolved
test/e2e/downloadsDisabled.test.ts Outdated Show resolved Hide resolved
patches/disable-downloads.diff Show resolved Hide resolved
@jsjoeio jsjoeio requested a review from code-asher April 11, 2022 22:43
@jsjoeio jsjoeio temporarily deployed to npm April 11, 2022 22:54 Inactive
code-asher
code-asher previously approved these changes Apr 12, 2022
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.

Looks great!

I am not sure if we should try this now or if this is good enough but we might want to disable the download command itself because it is possible to bind the download command to a keybinding and then just use that to download files.

patches/disable-downloads.diff Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio requested a review from code-asher April 12, 2022 23:19
@jsjoeio jsjoeio temporarily deployed to npm April 12, 2022 23:21 Inactive
src/node/cli.ts Outdated Show resolved Hide resolved
src/node/cli.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio requested a review from code-asher April 12, 2022 23:32
@jsjoeio jsjoeio dismissed code-asher’s stale review April 12, 2022 23:33

Changes made, need a new approval

@jsjoeio jsjoeio enabled auto-merge (squash) April 12, 2022 23:34
src/node/cli.ts Show resolved Hide resolved
test/unit/node/cli.test.ts Show resolved Hide resolved
@jsjoeio jsjoeio temporarily deployed to npm April 12, 2022 23:38 Inactive
@jsjoeio jsjoeio disabled auto-merge April 12, 2022 23:40
@jsjoeio jsjoeio temporarily deployed to npm April 12, 2022 23:46 Inactive
@jsjoeio jsjoeio merged commit 0e1f396 into main Apr 13, 2022
@jsjoeio jsjoeio deleted the jsjoeio/issue-2426 branch April 13, 2022 16:39
TinLe pushed a commit to TinLe/code-server that referenced this pull request Apr 23, 2022
* feat(cli): add disable-file-downloads to cli

* feat(e2e): add download test

* feat(e2e): add downloads disabled test

* refactor(e2e): explain how to debug unexpected close

* feat(patches): add disable file downloads

* wip: update diff

* Update src/node/cli.ts

Co-authored-by: Asher <[email protected]>

* fixup! add missing common/contextkeys file to patch

* fixup!: update patch

* fixup!: default disable-file-downloads undefined

* fixup!: combine e2e tests

* fixup!: use different test names

* feat: add CS_DISABLE_FILE_DOWNLOADS

* fixup!: make explicit and cleanup test

* fixup!: use beforeEach

Co-authored-by: Asher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New user visible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option for disabling file downloads
2 participants