Fix WSL local project LSP URIs#669
Open
koxudaxi wants to merge 11 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds WSL UNC <-> file:// URI conversion helpers, integrates them into LSP server support and Ruff execution/formatting flows on Windows, threads resolved Ruff executable through command-arg/stdin-filename logic, adds tests, logging, CI job, and a changelog entry documenting the fix. Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as Client/IDE
participant LSP as Ruff LSP Provider
participant Helpers as WSL URI Helpers
participant FS as Filesystem/Windows UNC
participant RuffProc as Ruff Executable
IDE->>LSP: initialize (rootUri/rootPath)
LSP->>Helpers: normalizeWslInitializeParams(params)
Helpers-->>LSP: normalized params (rootPath → UNC)
LSP->>RuffProc: resolve executable & generateCommandArgs(...)
RuffProc-->>LSP: CommandArgs (stdin filename determined)
IDE->>LSP: request file resolution (fileUri)
LSP->>Helpers: buildWslUncPath(fileUri)
Helpers-->>LSP: \\wsl.localhost\distro\path or null
LSP->>FS: findLocalFileByPath(UNC path)
FS-->>LSP: VirtualFile
LSP-->>IDE: resolved VirtualFile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodana for JVM46 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2024.3.4
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testSrc/com/koxudaxi/ruff/RuffWslUriSupportTest.kt (1)
7-53: Add coverage forwsl$and escaped path segments.Current tests validate
wsl.localhostwell, but the implementation also supportswsl$; add that variant plus a case containing spaces/special chars to prevent URI conversion regressions.✅ Suggested additional tests
class RuffWslUriSupportTest { + `@Test` + fun `builds and restores URI for wsl dollar host`() { + val unc = """\\wsl$\Ubuntu\home\test\project""" + val uri = "file://wsl$/Ubuntu/home/test/project" + assertEquals(uri, buildWslFileUri(unc)) + assertEquals("//wsl$/Ubuntu/home/test/project", buildWslUncPath(uri)) + assertEquals("""\\wsl$\Ubuntu\home\test\project""", toWindowsWslUncPath("//wsl$/Ubuntu/home/test/project")) + } + + `@Test` + fun `handles encoded URI segments when converting back to UNC`() { + assertEquals( + "//wsl.localhost/Ubuntu/home/test/My Project", + buildWslUncPath("file://wsl.localhost/Ubuntu/home/test/My%20Project") + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testSrc/com/koxudaxi/ruff/RuffWslUriSupportTest.kt` around lines 7 - 53, Add tests to RuffWslUriSupportTest covering the alternate WSL authority "wsl$" and path segments that require escaping (e.g., spaces or percent-encoded characters) to ensure conversions round-trip; specifically, add assertions for buildWslFileUri, buildWslUncPath, and toWindowsWslUncPath using inputs that use "wsl$" instead of "wsl.localhost" and at least one path containing a space or encoded character so the implementation in buildWslFileUri, buildWslUncPath, and toWindowsWslUncPath is exercised for both authority variants and escaped segments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/com/koxudaxi/ruff/lsp/intellij/RuffLspServerSupportProvider.kt`:
- Around line 93-97: The createInitializeParams() override only converts
InitializeParams.rootPath for WSL; update it to also convert
InitializeParams.rootUri and every entry in InitializeParams.workspaceFolders
using toWindowsWslUncPath() when SystemInfo.isWindows is true so the LSP client
sends WSL-UNC-compatible URIs/paths to the Ruff server; locate
createInitializeParams(), call toWindowsWslUncPath() on rootUri (if non-null)
and map/replace workspaceFolders' uri/path fields accordingly (keeping existing
null-safety and using the same conversion helper).
In `@src/com/koxudaxi/ruff/RuffWslUriSupport.kt`:
- Around line 7-11: The buildWslFileUri function constructs a file:// URI via
string interpolation which skips percent-encoding for spaces/reserved
characters; change it to construct a java.net.URI (like buildWslUncPath does) by
creating a URI with scheme "file", the detected host (host from wslHosts), and
the normalized path (ensure it begins with "/" before passing as the path
argument) so the URI constructor performs proper percent-encoding; update
buildWslFileUri to return uri.toString() (or null on failure) consistent with
buildWslUncPath.
---
Nitpick comments:
In `@testSrc/com/koxudaxi/ruff/RuffWslUriSupportTest.kt`:
- Around line 7-53: Add tests to RuffWslUriSupportTest covering the alternate
WSL authority "wsl$" and path segments that require escaping (e.g., spaces or
percent-encoded characters) to ensure conversions round-trip; specifically, add
assertions for buildWslFileUri, buildWslUncPath, and toWindowsWslUncPath using
inputs that use "wsl$" instead of "wsl.localhost" and at least one path
containing a space or encoded character so the implementation in
buildWslFileUri, buildWslUncPath, and toWindowsWslUncPath is exercised for both
authority variants and escaped segments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a28690-65d4-44b1-a3e6-80af85d665f9
📒 Files selected for processing (4)
CHANGELOG.mdsrc/com/koxudaxi/ruff/RuffWslUriSupport.ktsrc/com/koxudaxi/ruff/lsp/intellij/RuffLspServerSupportProvider.kttestSrc/com/koxudaxi/ruff/RuffWslUriSupportTest.kt
|
@koxudaxi lsp4ij 0.19.2 should support wsl, right? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/com/koxudaxi/ruff/Ruff.kt`:
- Around line 722-731: The current resolveStdinFileName may return the original
filePath when executableRunsInWsl but wslPath is null, which can leave a
Windows/UNC path into WSL; change the branch in resolveStdinFileName (and the
similar code block around the other occurrence noted) so that when
executableRunsInWsl and wslPath is null it falls back to projectRelativeFilePath
(not filePath); locate resolveStdinFileName and the analogous logic that handles
getWslPath failures and replace the fallback return value to use
projectRelativeFilePath (or null if that is also null) to ensure Ruff receives a
WSL-valid stdin filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41f8cf8a-0839-41bc-8e77-3d0196046372
📒 Files selected for processing (3)
src/com/koxudaxi/ruff/Ruff.ktsrc/com/koxudaxi/ruff/RuffApplyService.kttestSrc/com/koxudaxi/ruff/RuffStdinFilenameArgsTest.kt
Owner
Author
|
@angelozerr The WSL URI/path conversion in this PR is for the IntelliJ built-in LSP client path only, not for the LSP4IJ client path. For LSP4IJ, the client-side WSL handling should already be covered by LSP4IJ itself. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/com/koxudaxi/ruff/lsp/intellij/RuffLspServerSupportProvider.kt`:
- Around line 121-130: findFileByUri currently returns null if
resolveWslLspLocalPath(fileUri) yields a path but
findLocalFileByPath(resolvedPath) returns null, preventing the superclass
fallback; change the logic in findFileByUri so that after resolving resolvedPath
you call findLocalFileByPath(resolvedPath) and only return it if non-null,
otherwise call and return super.findFileByUri(fileUri) (keep the
RuffLoggingService.log call for the resolved attempt or add a log on miss as
needed).
In `@src/com/koxudaxi/ruff/Ruff.kt`:
- Around line 722-732: The non-WSL branch of resolveStdinFileName omits the
final filePath fallback; change the return to use toWindowsWslUncPath(filePath)
?: projectRelativeFilePath ?: filePath so it mirrors the executableRunsInWsl
branch and always returns a filename when possible; update any affected tests
(or add a test) that cover the non-WSL case without projectRelativeFilePath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc7f0b6a-c878-4c35-9b4b-3f5630f22f51
📒 Files selected for processing (7)
gradle/libs.versions.tomlsrc/com/koxudaxi/ruff/Ruff.ktsrc/com/koxudaxi/ruff/lsp/intellij/RuffLspServerSupportProvider.ktsrc/com/koxudaxi/ruff/lsp/lsp4ij/RuffLanguageClient.ktsrc/com/koxudaxi/ruff/lsp/lsp4ij/RuffLanguageServerFactory.kttestSrc/RuffLspServerSupportProviderTest.kttestSrc/com/koxudaxi/ruff/RuffStdinFilenameArgsTest.kt
✅ Files skipped from review due to trivial changes (3)
- src/com/koxudaxi/ruff/lsp/lsp4ij/RuffLanguageServerFactory.kt
- gradle/libs.versions.toml
- testSrc/com/koxudaxi/ruff/RuffStdinFilenameArgsTest.kt
Comment on lines
+121
to
+130
| override fun findFileByUri(fileUri: String): VirtualFile? { | ||
| if (!SystemInfo.isWindows) { | ||
| return super.findFileByUri(fileUri) | ||
| } | ||
| val resolvedPath = resolveWslLspLocalPath(fileUri) | ||
| if (resolvedPath != null) { | ||
| RuffLoggingService.log(project, "LSP resolve URI: $fileUri -> $resolvedPath") | ||
| return findLocalFileByPath(resolvedPath) | ||
| } | ||
| return super.findFileByUri(fileUri) |
There was a problem hiding this comment.
Preserve the superclass fallback when WSL path lookup misses.
If resolveWslLspLocalPath(fileUri) succeeds but findLocalFileByPath(resolvedPath) returns null, we currently stop there and never try super.findFileByUri(fileUri). That turns this WSL mapping into a hard failure instead of a best-effort optimization.
💡 Suggested fix
override fun findFileByUri(fileUri: String): VirtualFile? {
if (!SystemInfo.isWindows) {
return super.findFileByUri(fileUri)
}
val resolvedPath = resolveWslLspLocalPath(fileUri)
if (resolvedPath != null) {
RuffLoggingService.log(project, "LSP resolve URI: $fileUri -> $resolvedPath")
- return findLocalFileByPath(resolvedPath)
+ findLocalFileByPath(resolvedPath)?.let { return it }
}
return super.findFileByUri(fileUri)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/com/koxudaxi/ruff/lsp/intellij/RuffLspServerSupportProvider.kt` around
lines 121 - 130, findFileByUri currently returns null if
resolveWslLspLocalPath(fileUri) yields a path but
findLocalFileByPath(resolvedPath) returns null, preventing the superclass
fallback; change the logic in findFileByUri so that after resolving resolvedPath
you call findLocalFileByPath(resolvedPath) and only return it if non-null,
otherwise call and return super.findFileByUri(fileUri) (keep the
RuffLoggingService.log call for the resolved attempt or add a log on miss as
needed).
Comment on lines
+722
to
+732
| internal fun resolveStdinFileName( | ||
| filePath: String, | ||
| projectRelativeFilePath: String?, | ||
| executableRunsInWsl: Boolean, | ||
| wslPath: String? | ||
| ): String? { | ||
| if (executableRunsInWsl) { | ||
| return wslPath ?: projectRelativeFilePath ?: filePath | ||
| } | ||
| return toWindowsWslUncPath(filePath) ?: projectRelativeFilePath | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are tests covering the non-WSL case with file outside project
rg -n "resolveStdinFileName" --type=kotlin -A 10Repository: koxudaxi/ruff-pycharm-plugin
Length of output: 5297
🏁 Script executed:
#!/bin/bash
# Read the complete test file
cat -n testSrc/com/koxudaxi/ruff/RuffStdinFilenameArgsTest.ktRepository: koxudaxi/ruff-pycharm-plugin
Length of output: 2003
🏁 Script executed:
#!/bin/bash
# Find and examine the toWindowsWslUncPath function
rg -n "fun toWindowsWslUncPath" --type=kotlin -A 5Repository: koxudaxi/ruff-pycharm-plugin
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Check if there are any other test cases we might have missed
rg -n "fun \`" testSrc/com/koxudaxi/ruff/RuffStdinFilenameArgsTest.ktRepository: koxudaxi/ruff-pycharm-plugin
Length of output: 352
Add fallback to filePath in non-WSL case for consistency.
The WSL executable branch (line 729) includes a final filePath fallback, while the non-WSL branch (line 731) does not. For files outside the project with a non-WSL executable, the function returns null, omitting --stdin-filename and losing filename context.
Suggest adding the same fallback for consistency:
Diff
- return toWindowsWslUncPath(filePath) ?: projectRelativeFilePath
+ return toWindowsWslUncPath(filePath) ?: projectRelativeFilePath ?: filePathThis issue is not covered by existing tests (which all provide projectRelativeFilePath).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| internal fun resolveStdinFileName( | |
| filePath: String, | |
| projectRelativeFilePath: String?, | |
| executableRunsInWsl: Boolean, | |
| wslPath: String? | |
| ): String? { | |
| if (executableRunsInWsl) { | |
| return wslPath ?: projectRelativeFilePath ?: filePath | |
| } | |
| return toWindowsWslUncPath(filePath) ?: projectRelativeFilePath | |
| } | |
| internal fun resolveStdinFileName( | |
| filePath: String, | |
| projectRelativeFilePath: String?, | |
| executableRunsInWsl: Boolean, | |
| wslPath: String? | |
| ): String? { | |
| if (executableRunsInWsl) { | |
| return wslPath ?: projectRelativeFilePath ?: filePath | |
| } | |
| return toWindowsWslUncPath(filePath) ?: projectRelativeFilePath ?: filePath | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/com/koxudaxi/ruff/Ruff.kt` around lines 722 - 732, The non-WSL branch of
resolveStdinFileName omits the final filePath fallback; change the return to use
toWindowsWslUncPath(filePath) ?: projectRelativeFilePath ?: filePath so it
mirrors the executableRunsInWsl branch and always returns a filename when
possible; update any affected tests (or add a test) that cover the non-WSL case
without projectRelativeFilePath.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #627
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Documentation
Chores