zed icon indicating copy to clipboard operation
zed copied to clipboard

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

Open gahooa opened this issue 8 months ago • 3 comments

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

gahooa avatar May 31 '25 20:05 gahooa

Warnings
:warning:

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 :no_entry_sign: dangerJS against d49165ab949cf069c0c296dad9730ee6ea361d98

zed-industries-bot avatar May 31 '25 22:05 zed-industries-bot

Updated PR with Release Notes.

gahooa avatar Jun 01 '25 03:06 gahooa

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 avatar Jun 03 '25 03:06 ConradIrwin

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.

gahooa avatar Jun 05 '25 03:06 gahooa

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

Looks like maybe the issue is here: https://github.com/zed-industries/zed/blob/3b344532202d9ab3d723d310f0c62ba418963ae3/crates/terminal/src/terminal.rs#L1037, where we strip off the prefix but don't decode the result.

ConradIrwin avatar Jun 05 '25 03:06 ConradIrwin

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?

SomeoneToIgnore avatar Jun 05 '25 05:06 SomeoneToIgnore

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.

ConradIrwin avatar Jun 05 '25 06:06 ConradIrwin

@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 avatar Jun 06 '25 01:06 gahooa

I rebased on origin/main to resolve conflicts.

gahooa avatar Jun 07 '25 04:06 gahooa

Hey @ConradIrwin can we get this merged in?

gahooa avatar Jun 14 '25 20:06 gahooa

Sorry for dropping this!

ConradIrwin avatar Jun 15 '25 19:06 ConradIrwin