Fix clicking on context files with absolute paths on Windows

[DanTup]

Fixes #2399 by removing a duplicate call to open a file which expects a file URI but was given a path, and only tries to join a path with the workspace URI if it's a relative path.

fix_click.mp4

I'm not certain if there really should be absolute paths in this list if things are inside my workspace, but it probably makes sense for this code to tolerate them.

@kalanchan

Test plan

• @-mention a file on Windows
• expand the list of context files
• click on an absolute path

[DanTup]

@abeatrix looks like this call was added in 343f626 but as far as I can tell it is redundant (because the code below opens the file).

My fix here is removing it because I don't think it's doing anything, but if there's a reason to have it we can also fix by changing it from a path to a file URI.

[dominiccooney]

There's a hard coded path in here?

[dominiccooney]

???

[DanTup]

😱

Thanks for catching this. I've added a test that clicks one of these links and verifies an editor tab opens. The test is broken before removing this and passing on Windows when I removed this.

However, on Mac/Linux it's failing because apparently we have a full path without the leading slash:

Looking into that..

[DanTup]

Ok, I tracked down the Linux/macOS test failures down to this code which is removing the leading slash from absolute file paths:

cody/vscode/src/chat/chat-view/chat-helpers.ts

Lines 74 to 77 in bcd49b3

	let relFsPath = item.uri.fsPath
	if (relFsPath.startsWith('/')) {
	relFsPath = relFsPath.slice(1)
	}

It's not clear to me what the purpose of this is.. on Windows it's not being done and the tests are passing (although it's not clear to me if we wanted absolute or relative paths in the context file list). On Linux/Mac this is stripping the first / which makes it seem like a relative path, but it's not clear what it's relative to).

I think this needs some input from someone who is more familiar with this (perhaps @abeatrix ?). And I know I've said this before, but I think we need to get better at being very explicit with comments on all "filename-like" fields what they really are... Are they URI paths (always forward slashes), native paths (backslashes on Windows), absolute, relative (if so, relative to what).

[DanTup]

FWIW if I just completely remove the code noted above, then Mac behaves the same as Windows (we show an absolute path, and clicking it works):

I don't know what implications there are of doing this (for files that come from places other than these @-mentions) though.

[DanTup]

@beyang the code quoted above was added in c958112 but it's a big change so I don't know if it was just moved/copied from elsewhere. If you authored it, do you remember what the reason was for just removing the first /?

It's causing issues because it looks like a relative path not absolute, and since Windows paths don't start with / it means the paths are inconsistent across the platforms.

[DanTup]

The test here will be impacted by the question at #2408 - it turns out user-selected files weren't intended to show up here, so the test added for this issue won't work once they're filtered out (when #2408 lands).

Depending on the response there, we may need to change the test for this (assuming the original issue is even still valid with that change).

[DanTup]

The test here will be impacted by the question at #2408 - it turns out user-selected files weren't intended to show up here, so the test added for this issue won't work once they're filtered out (when #2408 lands).

This decision was reverted in #2679 so user-mentioned files will show up again. However this change might be impacted by #2653 so I'll come back to this after that merges.

[DanTup]

This change is no longer necessary, the big was already fixed on main by changes @sqs made to tidy up use of paths vs URIs etc. :-)

2400.mp4