Skip to content

terminal: Fix file paths links with URL escapes not being clickable #31830

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

Merged

Conversation

gahooa
Copy link
Contributor

@gahooa gahooa commented May 31, 2025

For #31827

URL Decoding Fix for Terminal File Path Clicking

Discussion

This change does not allow for paths that literally have %XX inside of them. If any such paths exist, they will fail to ctrl+click. A larger change would be needed to handle that.

Problem

In the terminal, you could ctrl+click file paths to open them in the editor, but this didn't work when the paths contained URL-encoded characters (percent-encoded sequences like %CE%BB for Greek letter λ).

Example Issue

  • This worked: dashboardλ.mts:3:8
  • This didn't work: dashboard%CE%BB.mts:3:8

The URL-encoded form %CE%BB represents the Greek letter λ (lambda), but the terminal wasn't decoding these sequences before trying to open the files.

Solution

Added URL decoding functionality to the terminal path detection system:

  1. Added urlencoding dependency to crates/terminal/Cargo.toml
  2. Created decode_file_path function in crates/terminal/src/terminal.rs that:
    • Attempts to decode URL-encoded paths using urlencoding::decode()
    • Falls back to the original string if decoding fails
    • Handles malformed encodings gracefully
  3. Applied decoding to PathLikeTarget creation for both:
    • Regular file paths detected by word regex
    • File:// URLs that are treated as paths

Code Changes

New Function

/// Decodes URL-encoded file paths to handle cases where terminal output contains
/// percent-encoded characters (e.g., %CE%BB for λ).
/// Falls back to the original string if decoding fails.
fn decode_file_path(path: &str) -> String {
    urlencoding::decode(path)
        .map(|decoded| decoded.into_owned())
        .unwrap_or_else(|_| path.to_string())
}

Modified PathLikeTarget Creation

The function is now called when creating PathLikeTarget instances:

  • For file:// URLs: decode_file_path(path)
  • For regular paths: decode_file_path(&maybe_url_or_path)

Testing

Added comprehensive test coverage in test_decode_file_path() that verifies:

  • Normal paths remain unchanged
  • URL-encoded characters are properly decoded (λ, spaces, slashes)
  • Paths with line numbers work correctly
  • Invalid encodings fall back gracefully
  • Mixed encoding scenarios work

Impact

This fix enables ctrl+click functionality for file paths containing non-ASCII characters that appear URL-encoded in terminal output, making the feature work consistently with tools that output percent-encoded file paths.

The change is backward compatible - all existing functionality continues to work unchanged, and the fix only activates when URL-encoded sequences are detected.

Release Notes:

  • File paths printed in the terminal that have %XX escape sequences will now be properly decoded so that ctrl+click will open them

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 31, 2025
@maxdeviant maxdeviant changed the title Fix: File paths links with urlescapes are not clickable #31827 terminal: Fix file paths links with URL escapes not being clickable May 31, 2025
@zed-industries-bot
Copy link

zed-industries-bot commented May 31, 2025

Warnings
⚠️

This PR is missing release notes.

Please add a "Release Notes" section that describes the change:

Release Notes:

- Added/Fixed/Improved ...

If your change is not user-facing, you can use "N/A" for the entry:

Release Notes:

- N/A

Generated by 🚫 dangerJS against d49165a

@gahooa
Copy link
Contributor Author

gahooa commented Jun 1, 2025

Updated PR with Release Notes.

@ConradIrwin
Copy link
Member

Thanks for this! Out of interest, what tools output filenames with url-encoding?

Although it's unusual, dashboard%CE%BB.mts is a valid filename (and is distinct from dashboardλ.mts), so I'd prefer to fix the upstream tool to output valid filenames if possible. (For comparison, neither iTerm2 nor VSCodium do URL decoding like this).

Relatedly, in the case of file:// URLs, it seems like we should (but don't) support url-encoding.

@ConradIrwin ConradIrwin self-assigned this Jun 3, 2025
@gahooa
Copy link
Contributor Author

gahooa commented Jun 5, 2025

Thanks for the response @ConradIrwin. The tool in question is dprint the rust library. It is specifically with file:// paths where encoding is used, not regular paths.

The behavior of vscode is to support file:// urls with encoding. Here is an actual error off of my terminal:

DprintError("Expected '{', got 'namespace' at file:///home/jason/code/acp7r/p-sprint-2025-q1/smart/df4l-admin/src/api/test2%CE%BB.mts:3:8

It would make sense to me to have this only apply to file://, and leave /regular/paths/alone

Do you agree with that assessment? I could revise the patch accordingly if you do.

@ConradIrwin
Copy link
Member

Yes, that makes a lot more sense, thanks for clarifying!

Looks like maybe the issue is here:

if let Some(path) = maybe_url_or_path.strip_prefix("file://") {
, where we strip off the prefix but don't decode the result.

@SomeoneToIgnore
Copy link
Contributor

but don't decode the result

Later, MaybeNavigationTarget::PathLike is parsed as PathWithPosition::from_path.
Would not it be more beneficial to alter PathWithPosition::from_path to handle decodes?
I imagine, parsing such strings into file finder is also expected to be opened?

@ConradIrwin
Copy link
Member

I don't think so. I have two files: %61.txt and a.txt, and I need to be able to open either with zed %61.txt or zed a.txt, so we can't treat those as the same.

On the other hand, in a file URL the former can only be represented as file:///blah/%2561.txt, so we do need to decode when parsing URLs.

@gahooa gahooa force-pushed the fix-31827-file-paths-click-fix branch from 3ae0d15 to 147f48d Compare June 6, 2025 01:54
@gahooa
Copy link
Contributor Author

gahooa commented Jun 6, 2025

@ConradIrwin I adjusted the implementation to keep it very focused on just decoding file:// urls, with a fallback to not decoding if there are unicode errors.

I have tested this on all my use cases, and it does not affect paths that are NOT file://

Please check it out!

@gahooa gahooa force-pushed the fix-31827-file-paths-click-fix branch from 147f48d to d91e6cf Compare June 7, 2025 04:10
@gahooa
Copy link
Contributor Author

gahooa commented Jun 7, 2025

I rebased on origin/main to resolve conflicts.

@gahooa gahooa force-pushed the fix-31827-file-paths-click-fix branch from d91e6cf to d49165a Compare June 8, 2025 16:51
@gahooa
Copy link
Contributor Author

gahooa commented Jun 14, 2025

Hey @ConradIrwin can we get this merged in?

@ConradIrwin
Copy link
Member

Sorry for dropping this!

@ConradIrwin ConradIrwin enabled auto-merge (squash) June 15, 2025 19:05
@ConradIrwin ConradIrwin merged commit 02da466 into zed-industries:main Jun 15, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants