Add LSP jar file system support - #8329
Open
Manykeys wants to merge 3 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:
📝 WalkthroughWalkthroughAdds a metalsfs virtual filesystem: client capability, WorkspaceURIMapper and FolderURIMapper, JarFileSystemCache, LSPFileSystemProvider (read/stat/readDirectory with .class decoding), server commands, and integrates URI translation across LSP services and handlers. ChangesLibrary filesystem support and URI mapping
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala (1)
358-368:⚠️ Potential issue | 🟡 MinorStrip
metalsDecode:beforeconvertToLocal, not after.For a hypothetical
metalsDecode:metalsfs:/metalsLibraries/...URI,convertToLocalsees the outer scheme ismetalsDecode:(not starting withmetalsfs:/metalsLibraries), returns unchanged; then thestripPrefixyields ametalsfs:URI whichtoAbsolutePathSafe()cannot resolve. The ordering should be reversed so the mapper operates on the inner scheme:♻️ Proposed fix
- val strippedUri = uriMapper.convertToLocal(uri).stripPrefix("metalsDecode:") + val strippedUri = uriMapper.convertToLocal(uri.stripPrefix("metalsDecode:"))Low probability in today's flow, but cheap to make robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala` around lines 358 - 368, In getServiceForOpt fix the ordering so the "metalsDecode:" wrapper is removed before calling uriMapper.convertToLocal: first stripPrefix("metalsDecode:") from the incoming uri to produce an innerUri, then call uriMapper.convertToLocal(innerUri) and use that result for toAbsolutePathSafe() and the subsequent jar:/folderServices logic (references: getServiceForOpt, strippedUri variable, uriMapper.convertToLocal, toAbsolutePathSafe, folderServices.find/buildTargets.inverseSources); this ensures nested metalsfs:/... URIs are converted to local paths correctly.
🧹 Nitpick comments (4)
metals/src/main/scala/scala/meta/internal/metals/Indexer.scala (1)
206-208: Duplicate capability guard.
sendLibraryFileSystemReady()already checksclientConfig.isLibraryFileSystemSupported()internally (seeLSPFileSystemProvider.scalalines 47–53), so the outerifhere is redundant. Either drop the wrapper here, or drop the inner guard inLSPFileSystemProviderso the invariant lives in exactly one place.♻️ Proposed simplification
- if (clientConfig.isLibraryFileSystemSupported()) { - lspFileSystemProvider.sendLibraryFileSystemReady() - } + lspFileSystemProvider.sendLibraryFileSystemReady()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/Indexer.scala` around lines 206 - 208, The outer duplicate guard around clientConfig.isLibraryFileSystemSupported() in Indexer.scala is redundant because LSPFileSystemProvider.sendLibraryFileSystemReady() already performs this check; remove the if wrapper and replace the block with a direct call to lspFileSystemProvider.sendLibraryFileSystemReady() in the Indexer class to keep the capability check single-sourced (alternatively, if you prefer the check in Indexer, remove the internal guard in LSPFileSystemProvider.sendLibraryFileSystemReady() so the invariant lives in only one place).metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala (1)
125-131: CatchNonFatal, not bareException, inclassifyLocalUri.
catch { case _: Exception => None }will swallow genuinely unexpected errors (e.g.,InterruptedExceptionwrapping, subclasses that should propagate). Usescala.util.control.NonFatalto match the convention used elsewhere in this file/module.♻️ Proposed fix
- val path = - try Some(AbsolutePath(java.nio.file.Paths.get(new URI(decodedJarPath)))) - catch { case _: Exception => None } + val path = + try Some(AbsolutePath(java.nio.file.Paths.get(new URI(decodedJarPath)))) + catch { case NonFatal(_) => None }(and add
import scala.util.control.NonFatal)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala` around lines 125 - 131, In classifyLocalUri, replace the broad catch of Exception with a NonFatal-only match to avoid swallowing fatal/errors that should propagate; import scala.util.control.NonFatal and change the catch block in classifyLocalUri to match case NonFatal(_) => None so only non-fatal throwables are handled and others still propagate.metals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scala (1)
17-30: Theerrorfield on response classes is dead code.
FSReadDirectoriesResponse,FSReadFileResponse, andFSStatResponseall carry anerror: String, but every call site sets it to""and any real error propagates instead as a failedFuture(uncaught exception fromFiles.list, NIO I/O,resolveInnerPath, etc.). The client ends up with an RPC error rather than a structured response. Either:
- populate
errorinside atry/catchand always return a successful Future with either content or error, or- remove the
errorfield and document that failures come back as RPC errors.Not blocking, but today the API surface is misleading.
Also applies to: 63-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scala` around lines 17 - 30, The response classes FSReadDirectoriesResponse, FSReadFileResponse, and FSStatResponse expose an unused error: String field; remove the misleading error fields from those case classes (and any related constructors/usages) and update call sites that always pass "" to instead rely on existing exception propagation (i.e., keep throwing failures so RPC returns errors), ensuring you adjust pattern matches/serializers that referenced the error field (search for FSReadDirectoriesResponse, FSReadFileResponse, FSStatResponse and any JSON/RPC codecs) to compile without the removed field; alternatively, if you prefer structured errors, wrap the filesystem logic in try/catch and populate a single non-empty error string instead of throwing—pick one approach and make the case class shape and all call sites consistent.metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala (1)
380-530:toLocaloverloads mutate input params viasetUri; prefer returning a copy.Most
toLocalhelpers callparams.getTextDocument.setUri(...)and return the same mutatedparamsinstance. This has two downsides:
- It silently mutates the LSP4J DTO the caller still holds a reference to (e.g.,
val localParams = toLocal(params)also mutatesparams). This is fine today because JSON-RPC gives us fresh instances per request, but it's surprising and fragile for any future caller that assumes value semantics.- Many downstream methods call
toLocal(params)twice — once to extract the URI forgetServiceFor, once to pass to the service (e.g., lines 690-694, 699-703, 708-712, 720-721, …). Because mutation is idempotent this works, but it's wasted work and easy to misread.Minor refactor: have the helpers clone+update (using the
Hover/CodeActioncopy pattern you already use) and bind once at the call site. Not blocking, but will make this layer easier to reason about as it grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala` around lines 380 - 530, The toLocal overloads (e.g., the ones handling TextDocumentPositionParams, CompletionParams, ReferenceParams, DidOpenTextDocumentParams, etc.) currently mutate the incoming LSP4J DTOs via params.getTextDocument.setUri(...) and return the same instance; change each helper to produce and return a new copy with the converted URI instead of mutating the input (follow the copy pattern used for Hover/CodeAction or use uriMapper.convertToLocal where appropriate), update callers to bind the returned localParams once (avoid calling toLocal(params) multiple times), and ensure you copy the TextDocumentIdentifier with the updated URI rather than calling setUri on the original object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala`:
- Around line 173-174: The metalsfs branch currently calls
uriMapper.convertToLocal(...) directly inside decodedFileContents which can
throw and escape instead of returning a failed DecoderResponse; wrap the
convertToLocal invocation in a try/catch (or equivalent safe construct) inside
the "metalsfs" case of decodedFileContents so that any thrown exceptions are
caught and returned as Future.successful(DecoderResponse.failed(...)) using the
caught exception message/exception, preserving the method's error contract.
In
`@metals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scala`:
- Around line 152-169: The pattern match in resolveInnerPath is not exhaustive
and can throw a MatchError for unexpected URIs; add a final fallback case (case
_ => ...) that throws a typed, descriptive exception (e.g.
IllegalArgumentException or a custom exception) containing the decoded URI and
mention of the expected prefixes (URIMapper.jdkURI, URIMapper.workspaceJarURI,
URIMapper.sourceJarURI) so callers get a clear error instead of a MatchError;
update resolveInnerPath to return the same (FileSystemInfo, Option[String])
signature but always handle unknown URIs by throwing that descriptive exception.
- Around line 107-119: The readFile implementation blocks an EC thread by
calling Await.result inside Future; instead, make readFile fully asynchronous by
composing the decodedFileContents Future with flatMap/for-comprehension: call
resolveInnerPath and compute the non-blocking path handling, and when the file
endsWith ".class" return
fileDecoderProvider.decodedFileContents(path.toUri.toString).map(res =>
Option(res.value).filter(_.nonEmpty).getOrElse(res.error)) and otherwise wrap
the synchronous Files.readAllBytes branch in Future(blocking(...)) or use
IO-friendly API, then map both branches to FSReadFileResponse(uri, contents,
""); remove the Await.result and related Duration imports. Focus changes around
the readFile method, resolveInnerPath usage, and
fileDecoderProvider.decodedFileContents call.
- Around line 86-96: Files.list(path) returns a Stream that must be closed to
avoid leaking directory handles; wrap the stream returned by Files.list(path) in
a try/finally (or use an AutoCloseable resource pattern) and close it after
collecting/mapping to FSReadDirectoryResponse so the underlying directory stream
is released; specifically, in LSPFileSystemProvider where Files.list(path) is
collected and mapped to FSReadDirectoryResponse (using p.getFileName.toString
and Files.isRegularFile(p)), open the stream into a local val, perform the
.collect/.asScala/.map/.toArray work inside the try block, then ensure
stream.close() in finally.
In `@metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala`:
- Around line 170-181: The encodeUri method currently throws an
IllegalStateException for any scheme other than file:, jar:, or metalsfs:, which
causes convertToMetalsFS to blow up for legitimate non-local schemes; update
encodeUri (and callers if needed) to simply return the original uri unchanged
for unrecognized schemes (or alternatively throw a descriptive, typed exception
if you want explicit failure), and replace the unhelpful "Why here?" message
with a clear message including the scheme and uri; reference encodeUri,
convertToMetalsFS, and classifyLocalUri when making this change so passing
through http:, untitled:, or custom schemes is safe and logged clearly.
- Around line 191-213: The pattern match in convertToLocal on
URIEncoderDecoder.decode(uri) is non-exhaustive (only handles jdkURI,
workspaceJarURI, sourceJarURI) and can throw MatchError for other metalsfs:
paths; add a fallback case (e.g., case other => return uri or throw a typed
IllegalArgumentException) within the match so unknown or bare parentURI paths
are handled deterministically. Update the match in convertToLocal (referencing
URIMapper.parentURI, URIEncoderDecoder.decode, URIMapper.jdkURI,
URIMapper.workspaceJarURI, URIMapper.sourceJarURI and fsPath handling) to
include this default branch that either returns the original uri unchanged or
raises a descriptive exception instead of letting a MatchError escape. Ensure
behavior is consistent with callers like FileDecoderProvider.decodedFileContents
and LSPFileSystemProvider read* methods.
- Around line 72-84: getOrCreateFileSystem has a TOCTOU race: wrap its body in a
synchronization (e.g., this.synchronized) so only one thread at a time attempts
to create a FileSystem and handle the race when another thread already created
it by catching FileSystemAlreadyExistsException around
FileSystems.newFileSystem(zipURI, ...) and retrying
FileSystems.getFileSystem(zipURI) (returning FileSystemInfo with the resolved fs
and fileUri); reference getOrCreateFileSystem, FileSystemInfo, zipURI,
FileSystems.getFileSystem, FileSystems.newFileSystem and
FileSystemAlreadyExistsException when locating where to add the synchronized
block and the retry/catch.
In `@metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala`:
- Around line 130-133: The current uriMapper uses folderServices.headOption
which can misclassify metalsfs: URIs in multi-folder workspaces; change
selection so URI classification delegates to the correct per-folder service
instead of always using the head: either (A) replace usages of uriMapper with a
call that finds the service via getServiceForOpt(uri) and then uses that
service.uriMapper when converting (e.g., inside convertToLocal/convertToMetalsFS
paths and LSPFileSystemProvider interactions), or (B) implement an aggregated
URIMapper that iterates folderServices.map(_.uriMapper) and calls
URIMapper.classifyLocalUri on each until one matches (falling back to
fallbackService.uriMapper only if none match); update references to uriMapper in
WorkspaceLspService accordingly so classification uses the matched/aggregated
mapper.
---
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala`:
- Around line 358-368: In getServiceForOpt fix the ordering so the
"metalsDecode:" wrapper is removed before calling uriMapper.convertToLocal:
first stripPrefix("metalsDecode:") from the incoming uri to produce an innerUri,
then call uriMapper.convertToLocal(innerUri) and use that result for
toAbsolutePathSafe() and the subsequent jar:/folderServices logic (references:
getServiceForOpt, strippedUri variable, uriMapper.convertToLocal,
toAbsolutePathSafe, folderServices.find/buildTargets.inverseSources); this
ensures nested metalsfs:/... URIs are converted to local paths correctly.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Indexer.scala`:
- Around line 206-208: The outer duplicate guard around
clientConfig.isLibraryFileSystemSupported() in Indexer.scala is redundant
because LSPFileSystemProvider.sendLibraryFileSystemReady() already performs this
check; remove the if wrapper and replace the block with a direct call to
lspFileSystemProvider.sendLibraryFileSystemReady() in the Indexer class to keep
the capability check single-sourced (alternatively, if you prefer the check in
Indexer, remove the internal guard in
LSPFileSystemProvider.sendLibraryFileSystemReady() so the invariant lives in
only one place).
In
`@metals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scala`:
- Around line 17-30: The response classes FSReadDirectoriesResponse,
FSReadFileResponse, and FSStatResponse expose an unused error: String field;
remove the misleading error fields from those case classes (and any related
constructors/usages) and update call sites that always pass "" to instead rely
on existing exception propagation (i.e., keep throwing failures so RPC returns
errors), ensuring you adjust pattern matches/serializers that referenced the
error field (search for FSReadDirectoriesResponse, FSReadFileResponse,
FSStatResponse and any JSON/RPC codecs) to compile without the removed field;
alternatively, if you prefer structured errors, wrap the filesystem logic in
try/catch and populate a single non-empty error string instead of throwing—pick
one approach and make the case class shape and all call sites consistent.
In `@metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala`:
- Around line 125-131: In classifyLocalUri, replace the broad catch of Exception
with a NonFatal-only match to avoid swallowing fatal/errors that should
propagate; import scala.util.control.NonFatal and change the catch block in
classifyLocalUri to match case NonFatal(_) => None so only non-fatal throwables
are handled and others still propagate.
In `@metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala`:
- Around line 380-530: The toLocal overloads (e.g., the ones handling
TextDocumentPositionParams, CompletionParams, ReferenceParams,
DidOpenTextDocumentParams, etc.) currently mutate the incoming LSP4J DTOs via
params.getTextDocument.setUri(...) and return the same instance; change each
helper to produce and return a new copy with the converted URI instead of
mutating the input (follow the copy pattern used for Hover/CodeAction or use
uriMapper.convertToLocal where appropriate), update callers to bind the returned
localParams once (avoid calling toLocal(params) multiple times), and ensure you
copy the TextDocumentIdentifier with the updated URI rather than calling setUri
on the original object.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d832c09-059f-4b66-a897-0417677b4bbf
📒 Files selected for processing (15)
metals/src/main/scala/scala/meta/internal/metals/ClientCommands.scalametals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/FallbackMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scalametals/src/main/scala/scala/meta/internal/metals/IndexProviders.scalametals/src/main/scala/scala/meta/internal/metals/Indexer.scalametals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scalametals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ServerCommands.scalametals/src/main/scala/scala/meta/internal/metals/URIMapper.scalametals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scalametals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scalametals/src/main/scala/scala/meta/metals/MetalsLanguageServer.scala
zielinsky
self-requested a review
April 28, 2026 23:10
zielinsky
requested changes
Apr 29, 2026
zielinsky
left a comment
zielinsky
left a comment
Member
There was a problem hiding this comment.
Hey, amazing job - thanks for that!
A few comments from me - if you will have some questions, feel free to ask.
Comment on lines
+86
to
+93
| private def findJdkByName(name: String): Option[AbsolutePath] = | ||
| jdkSources.find(jdk => getDecentJDKName(jdk.toNIO) == name) | ||
|
|
||
| private def findWorkspaceJarByName(name: String): Option[AbsolutePath] = | ||
| buildTargets.allWorkspaceJars.find(_.filename == name) | ||
|
|
||
| private def findSourceJarByName(name: String): Option[AbsolutePath] = | ||
| buildTargets.allSourceJars.find(_.filename == name) |
Member
There was a problem hiding this comment.
These methods scan all JARs on every call - I think we can create a lookup maps once after indexing completes.
private var jdkIndex: Map[String, AbsolutePath] = Map.empty
private var workspaceJarIndex: Map[String, AbsolutePath] = Map.empty
private var sourceJarIndex: Map[String, AbsolutePath] = Map.empty| * Maintains lazy NIO file system handles for JAR archives and provides | ||
| * overloaded conversions for common LSP4J parameter types. | ||
| */ | ||
| final case class URIMapper( |
Member
There was a problem hiding this comment.
In this case, is it necessary to use a case class, or would a regular class work?
Comment on lines
+53
to
+66
| private def jdkSources: Option[AbsolutePath] = | ||
| JdkSources(userJavaHome()).toOption | ||
|
|
||
| /** Returns the display names of all available JDK source archives. */ | ||
| def getJDKs: Iterator[String] = | ||
| jdkSources.iterator.map(jdk => getDecentJDKName(jdk.toNIO)) | ||
|
|
||
| /** Returns the filenames of all workspace dependency JARs. */ | ||
| def getWorkspaceJars: Iterator[String] = | ||
| buildTargets.allWorkspaceJars.map(_.filename) | ||
|
|
||
| /** Returns the filenames of all source JARs. */ | ||
| def getSourceJars: Iterator[String] = | ||
| buildTargets.allSourceJars.map(_.filename) |
Member
There was a problem hiding this comment.
After adding indexes these methods can look like (POC):
def getJDKs: Iterator[String] = jdkIndex.keys.iterator
def getWorkspaceJars: Iterator[String] = workspaceJarIndex.keys.iterator
def getSourceJars: Iterator[String] = sourceJarIndex.keys.iteratorjdkSources can be removed then also.
| * Returns an existing NIO [[FileSystem]] for the given archive, | ||
| * creating one if it does not yet exist. | ||
| */ | ||
| private def getOrCreateFileSystem(localPath: AbsolutePath): FileSystemInfo = { |
Member
There was a problem hiding this comment.
Currently opened FileSystems are never closed. I think we should close them on re-indexing
| * Returns an existing NIO [[FileSystem]] for the given archive, | ||
| * creating one if it does not yet exist. | ||
| */ | ||
| private def getOrCreateFileSystem(localPath: AbsolutePath): FileSystemInfo = { |
Member
There was a problem hiding this comment.
Also wondering if this is the right place for this method - URIMapper should only handle URI translation between metalsfs:// and local paths.
| ): Option[String] = { | ||
| val path = | ||
| try Some(AbsolutePath(java.nio.file.Paths.get(new URI(decodedJarPath)))) | ||
| catch { case _: Exception => None } |
Comment on lines
+125
to
+162
| private def classifyLocalUri( | ||
| decodedJarPath: String | ||
| ): Option[String] = { | ||
| val path = | ||
| try Some(AbsolutePath(java.nio.file.Paths.get(new URI(decodedJarPath)))) | ||
| catch { case _: Exception => None } | ||
|
|
||
| path.flatMap { absPath => | ||
| val filename = absPath.filename | ||
| jdkSources match { | ||
| case Some(jdk) | ||
| if changeCase( | ||
| absPath.toURI.toString.stripSuffix("/") | ||
| ) == changeCase(jdk.toURI.toString.stripSuffix("/")) => | ||
| val name = getDecentJDKName(jdk.toNIO) | ||
| Some(s"${URIMapper.jdkURI}/$name") | ||
| case _ => | ||
| if ( | ||
| buildTargets.allWorkspaceJars.exists(j => | ||
| changeCase(j.toURI.toString.stripSuffix("/")) == changeCase( | ||
| absPath.toURI.toString.stripSuffix("/") | ||
| ) | ||
| ) | ||
| ) | ||
| Some(s"${URIMapper.workspaceJarURI}/$filename") | ||
| else if ( | ||
| buildTargets.allSourceJars.exists(j => | ||
| changeCase(j.toURI.toString.stripSuffix("/")) == changeCase( | ||
| absPath.toURI.toString.stripSuffix("/") | ||
| ) | ||
| ) | ||
| ) | ||
| Some(s"${URIMapper.sourceJarURI}/$filename") | ||
| else | ||
| None | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
You are calculating absPath.toURI.toString.stripSuffix("/") multiple times
val absPathUri = changeCase(absPath.toURI.toString.stripSuffix("/"))also this method will be simpler after adding indexes.
| val (fs, innerPath) = resolveInnerPath(uri) | ||
| val path = fs.fs.getPath(innerPath.getOrElse("/")) | ||
| Files | ||
| .list(path) |
Member
There was a problem hiding this comment.
We are not closing stream here.
Comment on lines
+152
to
+169
| private def resolveInnerPath( | ||
| uri: String | ||
| ): (FileSystemInfo, Option[String]) = { | ||
| val decoded = URIEncoderDecoder.decode(uri) | ||
| decoded match { | ||
| case jdk if jdk.startsWith(URIMapper.jdkURI) => | ||
| val (name, path) = URIMapper.getURIParts(jdk, URIMapper.jdkURI) | ||
| (uriMapper.getJDKFileSystem(name), path) | ||
| case jar if jar.startsWith(URIMapper.workspaceJarURI) => | ||
| val (name, path) = | ||
| URIMapper.getURIParts(jar, URIMapper.workspaceJarURI) | ||
| (uriMapper.getWorkspaceJarFileSystem(name), path) | ||
| case src if src.startsWith(URIMapper.sourceJarURI) => | ||
| val (name, path) = | ||
| URIMapper.getURIParts(src, URIMapper.sourceJarURI) | ||
| (uriMapper.getSourceJarFileSystem(name), path) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
non-exhaustive pattern match, can you add a case like
case other => throw new IllegalArgumentException(s"Unknown metalsfs URI: $other"
Comment on lines
+107
to
+119
| def readFile(uri: String): Future[FSReadFileResponse] = Future { | ||
| val (fs, innerPath) = resolveInnerPath(uri) | ||
| val path = fs.fs.getPath(innerPath.getOrElse("/")) | ||
| val contents = | ||
| if (path.getFileName.toString.endsWith(".class")) { | ||
| val fut = fileDecoderProvider.decodedFileContents(path.toUri.toString) | ||
| val res = Await.result(fut, 10.minutes) | ||
| Option(res.value).filter(_.nonEmpty).getOrElse(res.error) | ||
| } else { | ||
| new String(Files.readAllBytes(path), StandardCharsets.UTF_8) | ||
| } | ||
| FSReadFileResponse(uri, contents, "") | ||
| } |
Member
There was a problem hiding this comment.
Agreed with coderabbitai (POC)
def readFile(uri: String): Future[FSReadFileResponse] = {
val (fs, innerPath) = resolveInnerPath(uri)
val path = fs.fs.getPath(innerPath.getOrElse("/"))
if (path.getFileName.toString.endsWith(".class"))
fileDecoderProvider.decodedFileContents(path.toUri.toString)
.map { res =>
val contents = Option(res.value).filter(_.nonEmpty).getOrElse("")
val err = Option(res.error).getOrElse("")
FSReadFileResponse(uri, contents, err)
}
else
Future(FSReadFileResponse(uri, new String(Files.readAllBytes(path), StandardCharsets.UTF_8), ""))
} instead of using Await inside Future
Author
There was a problem hiding this comment.
coderabbit made some good points, I'll fix it
Author
There was a problem hiding this comment.
Sorry for going quiet on this. Latest push addresses most of the review threads. Also added a toggle (off by default) for the feature to metals-vscode
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/builds/DederBuildTool.scala`:
- Around line 13-18: The BSP install arg generation currently checks
workspace.resolve(DederBuildTool.buildFile) in createBspFileArgs which can miss
nested/custom Deder roots; change the existence check to use
projectRoot.resolve(DederBuildTool.buildFile).isFile instead of
workspace.resolve(...), leaving the returned args List(DederBuildTool.name,
"bsp", "install") unchanged so BSP is generated when projectRoot/deder.pkl
exists.
In `@tests/unit/src/test/scala/tests/mcp/McpConfigSuite.scala`:
- Line 288: The test currently asserts that the old path ".kilocode/mcp.json"
does not exist but the code uses val kiloConfigFile =
projectPath.resolve(".kilo/kilo.json"); update the no-extension/non-existence
assertion to check kiloConfigFile (i.e., ".kilo/kilo.json") instead of the old
".kilocode/mcp.json" so the test verifies the new Kilo path is not inadvertently
generated (locate the assertion in McpConfigSuite.scala that refers to
".kilocode/mcp.json" and replace it to use kiloConfigFile or the string
".kilo/kilo.json").
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e061019c-84a8-4dd1-bee9-734d0d1bf8e7
⛔ Files ignored due to path filters (1)
website/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
metals/src/main/scala/scala/meta/internal/builds/BuildTools.scalametals/src/main/scala/scala/meta/internal/builds/DederBuildTool.scalametals/src/main/scala/scala/meta/internal/builds/DederDigest.scalametals/src/main/scala/scala/meta/internal/builds/SbtBuildTool.scalametals/src/main/scala/scala/meta/internal/metals/Embedded.scalametals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scalametals/src/main/scala/scala/meta/internal/metals/mcp/McpConfig.scalametals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scalatests/cross/src/test/scala/tests/pc/InlayHintsSuite.scalatests/unit/src/test/scala/tests/SupportedScalaSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/mcp/McpConfigSuite.scalawebsite/docusaurus.config.tswebsite/package.json
✅ Files skipped from review due to trivial changes (4)
- website/docusaurus.config.ts
- tests/unit/src/test/scala/tests/UserConfigurationSuite.scala
- website/package.json
- metals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scala
Comment on lines
+13
to
+18
| override def createBspFileArgs( | ||
| workspace: AbsolutePath | ||
| ): Option[List[String]] = | ||
| Option.when(workspace.resolve(DederBuildTool.buildFile).isFile)( | ||
| List(DederBuildTool.name, "bsp", "install") | ||
| ) |
There was a problem hiding this comment.
Use projectRoot for Deder build-file detection in BSP install args.
Line 16 checks workspace.resolve(...), which can miss nested/custom Deder roots and prevent BSP config generation even when projectRoot/deder.pkl exists.
Proposed fix
override def createBspFileArgs(
workspace: AbsolutePath
): Option[List[String]] =
- Option.when(workspace.resolve(DederBuildTool.buildFile).isFile)(
+ Option.when(projectRoot.resolve(DederBuildTool.buildFile).isFile)(
List(DederBuildTool.name, "bsp", "install")
)📝 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
| override def createBspFileArgs( | |
| workspace: AbsolutePath | |
| ): Option[List[String]] = | |
| Option.when(workspace.resolve(DederBuildTool.buildFile).isFile)( | |
| List(DederBuildTool.name, "bsp", "install") | |
| ) | |
| override def createBspFileArgs( | |
| workspace: AbsolutePath | |
| ): Option[List[String]] = | |
| Option.when(projectRoot.resolve(DederBuildTool.buildFile).isFile)( | |
| List(DederBuildTool.name, "bsp", "install") | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@metals/src/main/scala/scala/meta/internal/builds/DederBuildTool.scala` around
lines 13 - 18, The BSP install arg generation currently checks
workspace.resolve(DederBuildTool.buildFile) in createBspFileArgs which can miss
nested/custom Deder roots; change the existence check to use
projectRoot.resolve(DederBuildTool.buildFile).isFile instead of
workspace.resolve(...), leaving the returned args List(DederBuildTool.name,
"bsp", "install") unchanged so BSP is generated when projectRoot/deder.pkl
exists.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala`:
- Around line 86-87: newWs/newSrc currently build a Map[String, Jar] by mapping
filename -> jar which drops jars with duplicate filenames; change the
construction to preserve all entries by grouping by filename (e.g. use
buildTargets.allWorkspaceJars.groupBy(_.filename) and
buildTargets.allSourceJars.groupBy(_.filename)) so the resulting types are
Map[String, Seq[Jar]] (or List[Jar]); then update any subsequent uses of
newWs/newSrc (lookups in URIMapper) to handle multiple jars per filename (choose
the correct jar or iterate over the Seq) so no jars are silently overwritten.
- Around line 60-65: The catch block in readReleaseName uses a non-local return
(return None) which must be removed; instead make the try/catch an expression
that yields an Option[String] by returning None from the catch branch without
using return. Concretely, refactor the try
Using.resource(Files.newBufferedReader(release))(props.load) block so the catch
{ case NonFatal(e) => scribe.warn(...); None } (or wrap the successful path in
Some(value)) returns an Option, ensuring readReleaseName returns that Option
rather than performing a non-local return; reference readReleaseName,
Files.newBufferedReader, props.load and scribe.warn to locate where to change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 728a14d6-5856-4851-9f00-4322aa1da429
📒 Files selected for processing (18)
metals/src/main/scala/scala/meta/internal/metals/ClientCommands.scalametals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/FallbackMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scalametals/src/main/scala/scala/meta/internal/metals/IndexProviders.scalametals/src/main/scala/scala/meta/internal/metals/Indexer.scalametals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scalametals/src/main/scala/scala/meta/internal/metals/JarFileSystemCache.scalametals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ServerCommands.scalametals/src/main/scala/scala/meta/internal/metals/TargetData.scalametals/src/main/scala/scala/meta/internal/metals/URIMapper.scalametals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scalametals/src/main/scala/scala/meta/internal/tvp/MetalsTreeViewProvider.scalametals/src/main/scala/scala/meta/metals/MetalsLanguageServer.scala
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/JarFileSystemCache.scala`:
- Around line 24-29: The open method duplicates URI creation logic; replace the
manual construction of fileUri and zipURI in JarFileSystemCache.open with a call
to the companion method JarFileSystemCache.jarUriFor(localPath) to get the jar
URI and reuse its fileUri logic, then use
openFileSystems.getOrElseUpdate(zipURI, openFileSystem(zipURI)) and return
FileSystemInfo(fs, fileUri) as before; ensure you reference the same localPath
parameter and preserve use of openFileSystems and openFileSystem so behavior is
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e15e2aaa-9c31-4fa8-bbbe-628508b9d546
📒 Files selected for processing (14)
metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scalametals/src/main/scala/scala/meta/internal/metals/FallbackMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scalametals/src/main/scala/scala/meta/internal/metals/IndexProviders.scalametals/src/main/scala/scala/meta/internal/metals/Indexer.scalametals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scalametals/src/main/scala/scala/meta/internal/metals/JarFileSystemCache.scalametals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/TargetData.scalametals/src/main/scala/scala/meta/internal/metals/URIMapper.scalametals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala
🚧 Files skipped from review as they are similar to previous changes (12)
- metals/src/main/scala/scala/meta/internal/metals/Indexer.scala
- metals/src/main/scala/scala/meta/internal/metals/FallbackMetalsLspService.scala
- metals/src/main/scala/scala/meta/internal/metals/FileDecoderProvider.scala
- metals/src/main/scala/scala/meta/internal/metals/IndexProviders.scala
- metals/src/main/scala/scala/meta/internal/metals/TargetData.scala
- metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala
- metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala
- metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
- metals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
- metals/src/main/scala/scala/meta/internal/metals/LSPFileSystemProvider.scala
- metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala
- metals/src/main/scala/scala/meta/internal/metals/URIMapper.scala
…indexing, read JDK name from JAVA_HOME/release, make readFile non-blocking (flatMap, no Await), close Files.list via Using.resource, add non-exhaustive match fallbacks, move isUriEqual to MetalsEnrichments
Author
|
Hey, sorry for writing so late! Could you check out the PR when you have a sec @zielinsky |
Author
|
@tgodzik would you have time to review this when you get a chance? |
Contributor
hey, sorry for not working on it, we are quite busy this week. We will probably have time to look at it next week, but can't promise anything. |
|
LGTM |
tgodzik
reviewed
Aug 4, 2026
tgodzik
left a comment
tgodzik
left a comment
Contributor
There was a problem hiding this comment.
I spent some time looking at it and it seems pretty complicated to add in order (in reality) to just have breadcrumbs. I think a much easier thing would be to revert to using .readonly directory and put that behind an option. We would just use the normal filesystem in that case.
Alternatively, this could be a client only solution as well. I came up with:
It adds a new file system, but it's simple enough code that should be easy to debug if needed.
| params | ||
| } | ||
|
|
||
| private def toLocal(params: CompletionParams): CompletionParams = { |
Contributor
There was a problem hiding this comment.
This is very fragile, we would have to remember to use toLocal on each method and it's super easy to miss. I would try to limit the amount of code added.
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
4 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.
Hi!
The idea and the architecture come from Arturm1's PR #4114. Decided to pick it up and try to finish it
Plan is to add it as an experimental feature and get feedback on what to improve :). I also plan to add a setting in VSCode to turn the feature on and off
Feature:
Adds a virtual file system
metalsfs://to the Metals LSP server. It allows you to browse JAR file contents directly in the VS Code sidebar — similar to "External Libraries" in IntelliJ IDEA. After project indexing, a folder called "Metals - Libraries" appears in Explorer with three sections: JDK sources, workspace JARs, and source JARsHow it works:
Server side (Scala):
URIMapper— converts jar:file:///path/cats.jar!/com/Foo.scala -> metalsfs:/metalsLibraries/jar/cats.jar/com/Foo.scala. It translates between internal Metals paths and virtual URIs for the clientLSPFileSystemProvider— handles three LSP commands from the client:readDirectory,readFile,stat. Uses Java NIO FileSystems to read JAR as a file system..classfiles are decompiled using CFRWorkspaceLspServicegot routing for the new commands and URI conversion in responses, so Go to Definition opens files throughmetalsfs://metals-library-filesystem-readynotification to the clientClient side (TypeScript):
MetalsFileSystemProviderimplementsvscode.FileSystemProviderand sends requests to the server via LSPexecuteCommandLibraryFileSystemReady, it registers the provider and adds the folder to the workspaceP.S
metals-vscode pr scalameta/metals-vscode#1939
Summary by CodeRabbit