set up baml with lsp4ij #2001
set up baml with lsp4ij #2001
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@angelozerr I've got some basic implementation work down for a BAML extension using lsp4ij for LSP support. I don't appear to have codelenses working though, which it looks like you had working in #889 - can you help take a look and see what's going on? |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to e6b3a21 in 1 minute and 56 seconds. Click for details.
- Reviewed
367lines of code in13files - Skipped
2files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. jetbrains/README.md:15
- Draft comment:
Verify the naming update: 'baml placeholder' now replaces the original template name. Ensure this change aligns with the intended branding. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify and ensure that the naming update aligns with the intended branding. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
2. jetbrains/build.gradle.kts:62
- Draft comment:
Confirm that the README file path '../README.md' is correct for extracting the plugin description. This relative path may cause issues if the repository structure changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to confirm the correctness of a file path, which is against the rules. It does not provide a specific suggestion or point out a clear issue with the code. The comment is not useful as it stands.
3. integ-tests/baml_src/test-files/functions/input/named-args/single/named-enum.baml:15
- Draft comment:
Typo detected: It looks like 'fuctns' is a typo for 'functions'. Please correct this before merging. - Reason this comment was not posted:
Marked as duplicate.
4. jetbrains/src/main/java/com/boundaryml/jetbrains_ext/BamlLanguageServer.java:9
- Draft comment:
Typographical note: The debug message on this line reads "baml language server started via osprocess". Consider verifying if "osprocess" is intended or if it should be revised to something like "OS process" for clarity and consistency with the class name (OSProcessStreamConnectionProvider). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor stylistic change to a debug print statement. Debug messages are typically informal and don't need to follow strict style guidelines. The message is clear enough as is, and this kind of nitpick doesn't warrant a PR comment. This falls under the "Do NOT make comments that are obvious or unimportant" rule. The inconsistency with the class name could indicate a broader style guide violation. Maybe there's a company style guide that requires consistent capitalization. Even if there is a style guide, enforcing it on debug print messages is overly pedantic and doesn't improve code quality in any meaningful way. Delete this comment as it's an unimportant stylistic suggestion about a debug message that doesn't impact code quality or functionality.
5. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlProjectService.kt:19
- Draft comment:
Typographical note: In thedownloadClifunction, the string literal "Downloading CLI version={cliVersion}" appears to be intended for Kotlin string interpolation. In Kotlin, you should use$cliVersion(or${cliVersion}if needed) rather than{cliVersion}. Please update this to correctly interpolate the variable. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_mSqXf5CbLqvLb4uY
You can customize 
| @@ -12,7 +12,7 @@ function FnTestNamedArgsSingleEnum(myArg: NamedArgsSingleEnum) -> string { | |||
| } | |||
|
|
|||
| test FnTestNamedArgsSingleEnum { | |||
| functions [FnTestNamedArgsSingleEnum] | |||
| fuctns [FnTestNamedArgsSingleEnum] | |||
There was a problem hiding this comment.
Typo detected: 'fuctns' should be 'functions'.
| fuctns [FnTestNamedArgsSingleEnum] | |
| functions [FnTestNamedArgsSingleEnum] |
|
|
||
| public BamlLanguageServer() { | ||
| System.out.printf("baml language server started via osprocess\n"); | ||
| GeneralCommandLine commandLine = new GeneralCommandLine("/Users/sam/baml3/engine/target/debug/baml-cli", "lsp"); |
There was a problem hiding this comment.
Avoid hard-coding the absolute path in GeneralCommandLine; consider parameterizing it to improve portability across environments.
| // Step 2: Extract the archive | ||
| extractTarGz(TAR_GZ_PATH, BAML_CACHE_DIR); | ||
| // TODO: make extracted files writable | ||
| // Files.writeString(BREADCRUMB_PATH, "baml-cli installed", StandardOpenOption.CREATE, StandardOpenOption.APPEND); |
There was a problem hiding this comment.
Ensure that a breadcrumb file is created to mark a successful installation. Currently, the write operation is commented out, which may lead to repeated installations.
| // Files.writeString(BREADCRUMB_PATH, "baml-cli installed", StandardOpenOption.CREATE, StandardOpenOption.APPEND); | |
| Files.writeString(BREADCRUMB_PATH, "baml-cli installed", StandardOpenOption.CREATE, StandardOpenOption.APPEND); |
| thisLogger().info("BAML Jetbrains extension service has started") | ||
| } | ||
|
|
||
| fun getRandomNumber() = (1..100).random() | ||
|
|
||
| fun downloadCli(cliVersion: String) { | ||
| println("Downloading CLI version={cliVersion}") |
There was a problem hiding this comment.
The string interpolation in downloadCli is ineffective. Replace "Downloading CLI version={cliVersion}" with a proper interpolation, for example: "Downloading CLI version=$cliVersion".
| println("Downloading CLI version={cliVersion}") | |
| println("Downloading CLI version=$cliVersion") |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jetbrains/src/main/java/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.java
Fixed
Show fixed
Hide fixed
It is a great news! For codelens, is it working with my template use defined? I suggest that you enable LSP console as verbose https://github.com/redhat-developer/lsp4ij/blob/main/docs/UserGuide.md#lsp-console and check you have some textDocument/codeLens traces. |
|
|
||
| @Override | ||
| protected void install(@NotNull ProgressIndicator indicator) throws Exception { | ||
| progress("Downloading server components...", 0.25, indicator); |
There was a problem hiding this comment.
As you download from github releases I think you could use my current work with installation (not released) https://github.com/redhat-developer/lsp4ij/blob/main/docs/UserDefinedLanguageServerTemplate.md#github-asset-download
I need to write more doc
There was a problem hiding this comment.
@angelozerr is this released yet?
There was a problem hiding this comment.
No sorry, I need to fix some install issues again. I hope I could do the release next week.
There was a problem hiding this comment.
Got it.
Looking through the code, BTW, I'm concerned that it won't have enough flexibility for us: we also need the ability to switch the version of the server we download.
Our current implementation in vscode is that, if your baml_src is at v0.81.0 but your baml LSP is at v0.82.0, the LSP will hand a message back to the client that says "baml_src is at 0.81.0" and then our client goes and fetches baml LSP 0.81.0. We're also in the process of deciding what this will look like in Zed (which we're implementing now), c.f. how csharp LSP is implemented for Zed.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3d284c2 in 37 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. jetbrains/src/main/java/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.java:55
- Draft comment:
Updated TODO comment to 'executable' seems more appropriate. Consider adding logic (e.g., using Files.setPosixFilePermissions) to explicitly set executable permissions on relevant extracted files. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_mnh9mgvu9Zxj6hIs
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5a63181 in 1 minute and 56 seconds. Click for details.
- Reviewed
582lines of code in7files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_server/src/playground/mod.rs:2
- Draft comment:
Removal of the 'playground' module and addition of 'playground_server_helpers' improves modularity. Verify that all functionality from the removed module is now fully covered in the helpers module. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
2. engine/language_server/src/playground/playground_server.rs:10
- Draft comment:
The refactoring to have PlaygroundServer only wrap the run function (with helpers moved) cleans up responsibilities. Ensure that no needed helper functionality was omitted during the split. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
3. engine/language_server/src/playground/playground_server_helpers.rs:24
- Draft comment:
Consider handling potential lock failures when calling unwrap() on the baml_src_projects Mutex. Using a safer error message or .expect() with context might improve debuggability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. engine/language_server/src/playground/playground_server_helpers.rs:45
- Draft comment:
Consider logging or handling errors when serializing the FrontendMessage instead of silently ignoring them. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. jetbrains/gradle.properties:26
- Draft comment:
The LSP plugin version is updated to 0.14.0. Confirm that this upgrade is compatible with existing APIs as changes could require adjustments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to confirm compatibility with existing APIs after a version upgrade. It doesn't provide a specific suggestion or point out a specific issue with the code. It falls under the rule of not asking the author to confirm or ensure behavior, which is not allowed.
6. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServer.kt:12
- Draft comment:
Using a debug command line (with hardcoded path) is useful for local testing; ensure this temporary change is not pushed into production accidentally. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlToolWindowFactory.kt:57
- Draft comment:
The browser panel creation code has been moved to avoid duplication, which improves clarity. Consider removing any leftover commented sample code if it is no longer required. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. engine/language_server/src/playground/playground_server_helpers.rs:109
- Draft comment:
Typo: In the comment, "Adds a "/" route which servers the static files of the frontend" should likely read "serves" rather than "servers". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is technically correct, our rules say not to make purely informative comments or unimportant changes. A typo in a doc comment doesn't affect functionality and is a very minor issue. The comment doesn't suggest any meaningful code improvements. The typo could be confusing to future readers since "servers" vs "serves" changes the meaning significantly in a web context. Documentation quality is important. While documentation is important, this is an extremely minor typo that doesn't significantly impact understanding. The meaning is still clear from context. Following our rules about avoiding trivial comments, this typo fix comment should be removed as it doesn't suggest any meaningful code improvements.
Workflow ID: wflow_okQVZwCKoonVxPbs
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8da4663 in 1 minute and 36 seconds. Click for details.
- Reviewed
71lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_server/src/playground/definitions.rs:41
- Draft comment:
Default implementation for PlaygroundState using new() is clear and idiomatic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. engine/language_server/src/server/api/requests/code_action.rs:47
- Draft comment:
Removed the '&' in the argument for DocumentKey::from_url. Ensure that the function now correctly handles ownership or borrowing of the root path. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the function handles ownership or borrowing correctly after a change. This is a request for confirmation of behavior, which violates the rules.
3. engine/language_server/src/server/api/requests/code_action.rs:54
- Draft comment:
Replacing 'unwrap_or(vec![])' with 'unwrap_or_default()' and using 'find()' instead of 'filter().next()' improves clarity and conciseness. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. engine/language_server/src/server/api/requests/code_action.rs:63
- Draft comment:
Using to_string() for static "Open Playground" strings avoids the overhead of format!; this is a neat micro-optimization. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply points out a micro-optimization without suggesting any changes or confirming intentions.
5. engine/language_server/src/server/api/requests/go_to_definition.rs:106
- Draft comment:
Refactoring the list_functions call by replacing 'unwrap_or(vec![])' with 'unwrap_or_default()' and using 'find()' instead of 'filter().next()' is a good move towards more idiomatic code. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_UXan11I5uYOd96wq
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed bd4c9c6 in 1 minute and 29 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServer.kt:14
- Draft comment:
Remove or refactor the commented-out debug code to keep the codebase clean. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_FmiMGi8HLOgtwRdW
You can customize
| // super.setCommandLine(commandLine) | ||
|
|
||
| val cacheDir = Path.of(System.getProperty("user.home"), ".baml/jetbrains") | ||
| val version = Files.readString(cacheDir.resolve("baml-cli-installed.txt")).trim() |
There was a problem hiding this comment.
Consider adding error handling for file reading; Files.readString may throw if 'baml-cli-installed.txt' is missing.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ac15c8a in 50 seconds. Click for details.
- Reviewed
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/zed/src/lib.rs:43
- Draft comment:
Good refactor: Replacingmap_orwith an explicitif let Ok(stat)improves clarity by clearly separating error cases. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. engine/zed/src/lib.rs:111
- Draft comment:
Usingis_ok_andis more concise, but ensure your minimum Rust version supports this method for Result. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests using a more concise method but also asks the author to ensure compatibility with a specific Rust version. The suggestion to useis_ok_andis valid, but the part about ensuring compatibility is not allowed according to the rules.
Workflow ID: wflow_WzKGyOGlycVeQlvt
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed f2cc679 in 1 minute and 10 seconds. Click for details.
- Reviewed
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_HozbzP4e6ifDIqns
You can customize
| val tmpDir: Path = Files.createTempDirectory(Path.of(PathManager.getTempPath()), "textmate-baml") | ||
| val tempPath = Path.of(PathManager.getTempPath()) | ||
| // Ensure the temp directory exists | ||
| if (!Files.exists(tempPath)) { |
There was a problem hiding this comment.
Instead of checking if the temp directory exists and then creating it, you can call Files.createDirectories(tempPath) directly. This method is idempotent and will create the directory if it doesn't exist without additional checks.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 095caac in 31 seconds. Click for details.
- Reviewed
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. jetbrains/gradle.properties:44
- Draft comment:
Consider making -Xmx2g configurable (e.g., via an environment variable) so that machines with different resources can adjust accordingly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_LEGrqdzjPnddgBUT
You can customize
jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.kt
Fixed
Show fixed
Hide fixed
jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.kt
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 8555e60 in 1 minute and 33 seconds. Click for details.
- Reviewed
48lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.kt:165
- Draft comment:
Using sanitizePath to mitigate directory traversal is good, but the implementation (normalizing then replacing '..') is too naive—it may remove valid substrings. Consider a more robust approach. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid security concern. The current implementation could potentially remove legitimate ".." substrings that are part of filenames. For example, a file named "hello..world.txt" would become "helloworld.txt". However, the code has a second safety check that verifies the final resolved path is within destDir, which provides strong protection against directory traversal regardless of the sanitization. The comment doesn't acknowledge the second layer of protection (the startsWith check). Also, real-world archive files rarely contain ".." in legitimate filenames. While the sanitization could be more robust, the overall security approach with dual protection makes this a minor issue. The current implementation is likely sufficient for real-world use. The comment should be deleted because the security concern is already adequately addressed by the combination of path sanitization and directory boundary checking.
Workflow ID: wflow_vCeLP9floCM9RZE0
You can customize
| TarArchiveInputStream(gzipIn).use { tarIn -> | ||
| var entry: TarArchiveEntry? = tarIn.nextTarEntry | ||
| while (entry != null) { | ||
| val sanitizedName = sanitizePath(entry.name) |
There was a problem hiding this comment.
Duplicate sanitization and path-check logic in the ZIP extraction block. Extracting this into a helper function would improve maintainability.
|
|
||
| private fun sanitizePath(path: String): String { | ||
| // Normalize the path and remove any ".." components | ||
| return Path.of(path).normalize().toString().replace("..", "") |
There was a problem hiding this comment.
The sanitizePath function’s use of replace('..', '') may inadvertently modify legitimate file names or not fully guard against absolute paths. Consider iterating path components to filter out dangerous segments.
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Integrates BAML with JetBrains IDEs using LSP4IJ, adding a language server, client-server communication, and IntelliJ plugin setup. > > - **Behavior**: > - Adds `PlaygroundState` in `definitions.rs` to manage client connections and event buffering. > - Implements `broadcast_function_change`, `broadcast_project_update`, and `broadcast_test_run` in `playground_server_helpers.rs`. > - Adds WebSocket handling in `start_client_connection()` in `playground_server_helpers.rs`. > - Updates `Server` in `server.rs` to start a playground server and handle port notifications. > - **IntelliJ Plugin**: > - Adds `BamlLanguageServer`, `BamlLanguageClient`, and `BamlLanguageServerFactory` for LSP integration. > - Implements `BamlGetPortService` to manage port communication. > - Configures `BamlToolWindowFactory` to display a tool window with a JCEF browser. > - Updates `plugin.xml` and `baml-lsp4ij.xml` for plugin actions and server registration. > - **Build and Configuration**: > - Updates `build.gradle.kts` and `gradle.properties` for Kotlin serialization and plugin dependencies. > - Adds `libs.versions.toml` for version management. > - Modifies `README.md` to reflect BAML setup. > - **Misc**: > - Adds `ExampleJavaClass.java` as a placeholder for Java code. > - Renames `MyBundle.kt` to `BamlBundle.kt`. > - Fixes WebSocket connection logic in `EventListener.tsx`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup> for 8555e60. You can [customize](https://app.ellipsis.dev/BoundaryML/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Co-authored-by: egol <egoriluk@gmail.com>
Important
Integrates BAML with JetBrains IDEs using LSP4IJ, adding a language server, client-server communication, and IntelliJ plugin setup.
PlaygroundStateindefinitions.rsto manage client connections and event buffering.broadcast_function_change,broadcast_project_update, andbroadcast_test_runinplayground_server_helpers.rs.start_client_connection()inplayground_server_helpers.rs.Serverinserver.rsto start a playground server and handle port notifications.BamlLanguageServer,BamlLanguageClient, andBamlLanguageServerFactoryfor LSP integration.BamlGetPortServiceto manage port communication.BamlToolWindowFactoryto display a tool window with a JCEF browser.plugin.xmlandbaml-lsp4ij.xmlfor plugin actions and server registration.build.gradle.ktsandgradle.propertiesfor Kotlin serialization and plugin dependencies.libs.versions.tomlfor version management.README.mdto reflect BAML setup.ExampleJavaClass.javaas a placeholder for Java code.MyBundle.kttoBamlBundle.kt.EventListener.tsx.This description was created by
for 8555e60. You can customize this summary. It will automatically update as commits are pushed.