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

Fixing file path for saving new files from chat in windows #6489

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arafatkatze
Copy link
Collaborator

@arafatkatze arafatkatze commented Dec 30, 2024

Solves Linear issue

Problem

On Windows systems, file operations were failing due to incorrect URI handling, specifically:

  • Windows paths (e.g., C:\path\to\file) were being incorrectly interpreted as URIs
  • Backslashes were being URL-encoded (%5C) causing path resolution failures
  • The system was interpreting drive letters (e.g., "C:") as URI schemes instead of part of the path

This resulted in errors like:

  • Provider "C" not installed
  • The network path was not found
  • URISyntaxException with "Expected scheme-specific part"

Solution

Added proper Windows path normalization in CodyEditorUtil.kt to handle file paths correctly before URI creation:

  1. Added path normalization for Windows-style paths:

  2. Applied this normalization in both findFileOrScratch and createFileOrScratchFromUntitled methods

Test plan

Changelog

@arafatkatze arafatkatze enabled auto-merge (squash) December 30, 2024 12:10
fileName = defaultUriPath.fileName.toString()
VfsUtil.findFile(defaultUriPath.parent, true)
try {
val defaultUriPath = Paths.get(params.defaultUri)
Copy link
Member

Choose a reason for hiding this comment

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

defaultUri is a URI so it should be parsed into a path like this

Paths.get(URI.create(params.defaultUri))

This has accidentally worked on other platforms because it assumes file:// is part of the path name. Wrapping in try/catch won't fix the root problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olafurpg maybe it's a good time to rewrite agent API to use Path objects instead of strings? There is quite a few corner cases in the way paths can be encoded (especially on windows) and discrepancies between agent and client path parsing hit us many times in past.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the auto-generated bindings? We encode these as TypeScript string on the cody side and don't have a static type to distinguish between URIs and normal strings.

Copy link
Member

Choose a reason for hiding this comment

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

For good and bad, we rely on JSON.stringify, which doesn't expose hooks to customize serialization of URI instances, so we need to rely on some form of discipline

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.

3 participants