Skip to content

impl: remove support for data dir fallback setting#287

Merged
fioan89 merged 4 commits intomainfrom
impl-simplify-cli-resolution
Apr 3, 2026
Merged

impl: remove support for data dir fallback setting#287
fioan89 merged 4 commits intomainfrom
impl-simplify-cli-resolution

Conversation

@fioan89
Copy link
Copy Markdown
Collaborator

@fioan89 fioan89 commented Apr 2, 2026

This PR is a result of discussion that happened in the #286. Basically it as an attempt of simplifying the CLI resolution and trying to be more aligned with the VS Code extension. The existing implementation was too cumbersome to understand, brittle and a lot of tedious work needed to solve all of it's usecases.

This PR removes the enable fallback to data dir setting, which was used only for access denied exceptions. The CLI resolution uses the binary destination if it was configured by the user, or it automatically falls back to data dir if binary destination was not configured. The implementation respects the user's choice and no longer tries to make smart choices on behalf of the user. For example if the user configured the binary destination but disabled downloads we just prompt him with an error instead of trying to fall on data dir.

There were a couple of other small improvements left from the previous PR that are now addressed.

This PR is a result of discussion that happened in the
#286.
Basically it as an attempt of simplifying the CLI resolution
and trying to be more aligned with the VS Code extension.
The existing implementation was too cumbersome to understand, brittle
and a lot of tedious work needed to solve all of it's usecases.

This PR removes the enable fallback to data dir setting, which was used
only for access denied exceptions. The CLI resolution uses the binary destination
if it was configured by the user, or it automatically falls back to data dir
if binary destination was not configured. The implementation respects the user's
choice and no longer tries to make smart choices on behalf of the user. For example
if the user configured the binary destination but disabled downloads we just prompt
him with an error instead of trying to fall on data dir.

There were a couple of other small improvements left from the previous PR that are
now addressed.

- resolves #285
@fioan89 fioan89 requested review from EhabY and code-asher April 2, 2026 21:55
@fioan89
Copy link
Copy Markdown
Collaborator Author

fioan89 commented Apr 2, 2026

Screenshot 2026-04-03 at 00 45 49 Screenshot 2026-04-03 at 00 18 22

@EhabY
Copy link
Copy Markdown

EhabY commented Apr 3, 2026

Code review

Found 1 issue:

  1. prettify() uses this.message for AccessDeniedException and FileSystemException, which produces redundant error messages. FileSystemException.getMessage() returns a formatted string like "/path/to/file: Permission denied", so the output becomes "Access denied to /path/to/file: Permission denied". The existing humanizeConnectionError in Error.kt uses e.file instead (just the path), producing the cleaner "Access denied to /path/to/file.". Since the fallback is now removed and these exceptions will surface directly to users, consider using this.file (cast to FileSystemException) for both the AccessDeniedException and FileSystemException branches to stay consistent with the existing pattern.

} else this.message
is AccessDeniedException,
is java.nio.file.AccessDeniedException -> "Access denied to ${this.message}"
is java.nio.file.FileSystemException -> "A file system operation failed when trying to access ${this.message}"

Existing pattern for reference:

return when (e) {
is java.nio.file.AccessDeniedException -> "Access denied to ${e.file}."
is UnknownHostException -> "Unknown host ${e.message ?: deploymentURL.host}."

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Copy link
Copy Markdown

@EhabY EhabY left a comment

Choose a reason for hiding this comment

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

Honestly this is so straightforward compared to the previous approach!

fioan89 added 3 commits April 3, 2026 22:50
We already had a complex exception prettifier that was no longer
used (for quite some time now). We repurposed it for the error reporter
used by the authentication wizard.
Reflect the new changes in the CLI resolution logic.
@fioan89 fioan89 merged commit e23c432 into main Apr 3, 2026
6 checks passed
@fioan89 fioan89 deleted the impl-simplify-cli-resolution branch April 3, 2026 21:03
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.

If coder is installed via winget, local binary fallback fails because plugin expects release artifact name instead of installed executable name

2 participants