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

Path -> URI #3247

Merged
merged 74 commits into from
Dec 19, 2024
Merged

Path -> URI #3247

merged 74 commits into from
Dec 19, 2024

Conversation

RomneyDa
Copy link
Collaborator

@RomneyDa RomneyDa commented Dec 8, 2024

This PR is to fix path issues from trying to use paths instead of URIs when working with workspace files.

Goals:

  • Simplify code and reduce overhead of parsing different os paths all over
  • Adds support for SSH, WSL, etc.
  • Adds support for multiple workspaces (not perfect, see workspaces note below)

Approach

  • Add URI utils to correspond with current path utils
  • Remove pretty much all paths from IDEs - use built in IDE uri functionality (this mostly consists of deletions, since we mostly were converting to paths anyways)
  • Add some passing of paths around for display purposes

Thoughts

  • Renaming: After a few lengthy and messy iterations, I decided to not try to do a bunch of renaming of e.g. "filepath" to "uri" or "fileUri" because it would take it from a mega-PR to an ultra-mega-PR of 300+ files. The main downside of this is that it doesn't allow using built-in type errors to tell me where updates might be needed. Fortunately, renames can be done piecewise much more easily than the actual functionality. It's not terrible to just walk through all ~700 ts/tsx files in a few hours and see where actual changes need to be made
  • Workspaces and passing URIs to LLMS
    • Unlike relative paths, we shouldn't pass full URIs to LLMs because the model shouldn't see outside the workspace
    • I looked at several ways to anonymously denote workspace in prompts, including hashing, replacing workspace prefix with "a/", "b/", or similar, etc. but wasn't satisfied with any, decided to stick with path relative to workspace for now
    • The only place this will fail is when it tries to guess which workspace a relative filepath is in and two workspaces contain some identical filepaths. This isn't a super-edge case but I believe would only effect tools and multifile edit with multiple workspaces. Can be a future problem to solve

⚠️ Things to look out for/fix in the future

  • Some tests might still assume running on a local device, especially mocha IDE tests
  • Lazy deterministic diff tests are old and not using recen
  • The vscode activate generates a .continuerc.json to fix possibility of inifinite index looping. Jetbrains doesn't seem to have this fix. Same with config.ts - generated on start in vscode but not jetbrains
  • SecretStorage may have issues with remote? not sure, didn't change
  • Share slash command may still not work with remote Changed minimally for this PR
  • GreptileContextProvider is written to use local files, and will not work with SSH. I did not fix on this branch because it's a bit involved

Copy link

netlify bot commented Dec 8, 2024

Deploy Preview for continuedev canceled.

Name Link
🔨 Latest commit e0385d3
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/67637cdb387adc0008b15cf6

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