LSP Downloader #1738
Open
LSP Downloader #1738
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
If you would like me to review this PR, either tag me in a comment or check this box:
You can disable automatic comments on my settings page |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 29f7e31 in 2 minutes and 32 seconds
More details
- Looked at
185lines of code in4files - Skipped
1files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. integ-tests/baml_src/generators.baml:30
- Draft comment:
Add newline at end-of-file for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%
This comment is purely informative and does not provide a specific code suggestion or ask for a test to be written. It simply suggests adding a newline for consistency, which is not a critical issue.
2. typescript/vscode-ext/packages/language-server/src/server.ts:395
- Draft comment:
Using a hardcoded version ('0.1.0') for downloadCli; consider parameterizing the CLI version. - 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%
The hardcoded version number is explicitly marked as temporary with a TODO. The comment is pointing out something that the code author already knows needs to be fixed. The comment doesn't add any new information or insight beyond what's already in the TODO comment.
The comment could be valuable if the TODO wasn't already there, as hardcoded version numbers are generally bad practice. But given the TODO, is this comment adding any value?
The comment is redundant with the existing TODO comment and doesn't provide any additional actionable information about how to fix it. The author clearly already knows this needs to be parameterized.
The comment should be deleted since it's redundant with the existing TODO comment and doesn't provide any new information or guidance.
3. integ-tests/baml_src/generators.baml:29
- Draft comment:
Ensure the file ends with a newline for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. typescript/vscode-ext/packages/language-server/package.json:37
- Draft comment:
New dependencies (axios, extract-zip, tar) are added for CLI download functionality. Confirm these versions align with project requirements. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
5. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:109
- Draft comment:
The tar extraction stream is piped but not awaited. This may lead to a race condition where the function returns before extraction completes. - Reason this comment was not posted:
Marked as duplicate.
6. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:112
- Draft comment:
Overwriting the archive entry's path via onReadEntry is brittle. Consider a more robust method to rename the extracted file when archive structures change. - 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 code is clearly in a temporary state using ripgrep for testing. The path modification is being used to rename the extracted file, and while this might be brittle, the entire section is marked as temporary with TODOs. The comment is technically correct about the brittleness, but it's premature since this code is clearly not in its final form.
The code might stay in this form longer than expected, and the brittleness could cause issues in production. The comment could help prevent technical debt.
While true, the TODOs and temporary nature of the code suggest this entire section will be rewritten. Making architectural suggestions about temporary test code isn't productive.
The comment should be deleted as it's critiquing temporary test code that has explicit TODOs indicating it will be changed.
7. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:87
- Draft comment:
A hardcoded test URL (ripgrep release) is used instead of constructing the URL with BASE_URL and dynamic parameters. Ensure this is updated before production. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:18
- Draft comment:
Consider capitalizing 'GitHub' correctly in the function comments (e.g., 'Returns the architecture name correctly formatted for the GitHub release.') for consistency with the official styling. - 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 purely stylistic comment that doesn't affect functionality. While technically correct (GitHub's official styling is "GitHub"), this kind of minor documentation formatting issue doesn't warrant a PR comment. It's not suggesting any meaningful code changes or quality improvements. It's the kind of thing that could be handled by a style guide or linter if it's really important.
The comment is technically correct about GitHub's official styling, and consistency in documentation is a valid concern. Documentation is part of code quality.
While documentation quality matters, this is too minor of an issue to warrant a PR comment. It doesn't affect code functionality or maintainability, and doesn't meet our threshold for "clearly a code change required."
Delete this comment as it's too minor and doesn't meet our criteria for requiring a clear code change.
9. typescript/vscode-ext/packages/language-server/src/server.ts:56
- Draft comment:
Typographical error: Consider changing "cant load webcrypto" to "can't load webcrypto" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. typescript/vscode-ext/packages/language-server/src/server.ts:307
- Draft comment:
Typographical error: The word 'mutliple' appears in a comment. Please correct it to 'multiple'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_37wihTa1xDftRXcH
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| // unzip.pipe(createWriteStream(path.join(INSTALL_PATH, binaryFileName))) | ||
| // await extractZip(compressedFilePath, { dir: INSTALL_PATH }) | ||
|
|
||
| return binaryFilePath |
There was a problem hiding this comment.
Extraction stream is not awaited; ensure extraction completes before returning the path.
| @@ -391,6 +392,11 @@ export function startServer(options?: LSOptions): void { | |||
| return | |||
| } | |||
|
|
|||
| // TODO: Grab correct version, this is for testing. | |||
| const cliPath = await downloadCli(process.platform.toString(), process.arch, '0.1.0') | |||
There was a problem hiding this comment.
The CLI download call uses a hardcoded version '0.1.0'. Update this and consider caching the binary to prevent unnecessary downloads on every save.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dde0c84 in 2 minutes and 5 seconds
More details
- Looked at
169lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:152
- Draft comment:
Consider awaiting the tar extraction process. Currently, piping the stream to tar.extract() may not ensure completion before the function resolves, potentially leading to race conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:150
- Draft comment:
The downloadCli function pipes the response stream into tar.extract without awaiting completion. Wrap the stream in a Promise or use a pipeline utility to ensure extraction finishes before the function resolves. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. typescript/vscode-ext/packages/language-server/src/cliDownloader.ts:130
- Draft comment:
The download URL is hard-coded to a ripgrep release for testing. Remember to switch back to using BASE_URL (or a configurable URL) when moving to production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:80
- Draft comment:
In checkVersionOnSave, using Array.forEach with a 'return' inside does not exit the loop. Consider using a for...of loop if you intend to short-circuit on version mismatch. - Reason this comment was not posted:
Marked as duplicate.
5. typescript/vscode-ext/packages/language-server/src/server.ts:395
- Draft comment:
When constructing cliVersion, the code directly accesses proj.list_generators()[0].version. Add a check to ensure that the generators array is not empty to avoid runtime errors. - Reason this comment was not posted:
Marked as duplicate.
6. typescript/vscode-ext/packages/language-server/src/server.ts:396
- Draft comment:
Using process.platform.toString() is unnecessary since process.platform is already a string. Remove redundant conversion for consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:156
- Draft comment:
Typo: It appears that 'BAML_PATH_SPLTTER' is used to split file paths. Did you mean 'BAML_PATH_SPLITTER'? Please correct the spelling for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. typescript/vscode-ext/packages/language-server/src/server.ts:56
- Draft comment:
Typo in log message: Consider changing "cant load webcrypto" to "can't load webcrypto" for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. typescript/vscode-ext/packages/language-server/src/server.ts:306
- Draft comment:
Typographical error in comment: 'mutliple baml_src dirs' should be corrected to 'multiple baml_src dirs'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_R6CpJn8YxM0xEIrj
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -79,7 +79,16 @@ class Project { | |||
|
|
|||
| checkVersionOnSave() { | |||
| let firstErrorMessage: undefined | string = undefined | |||
| let version: undefined | string = undefined | |||
|
|
|||
| this.list_generators().forEach((g) => { | |||
There was a problem hiding this comment.
Using forEach with a return in checkVersionOnSave does not break the loop. Consider switching to a for...of loop if an early exit upon version mismatch is desired.
| architecture: process.arch, | ||
| platform: process.platform.toString(), | ||
| // TODO: If no generators found then do something. | ||
| version: proj.list_generators()[0].version, |
There was a problem hiding this comment.
Directly accessing proj.list_generators()[0].version assumes the generators list is non-empty. Consider adding a check to ensure that the list is not empty before accessing the first element.
Suggested change
| version: proj.list_generators()[0].version, | |
| version: proj.list_generators().length > 0 ? proj.list_generators()[0].version : 'default_version', |
| /** | ||
| * Checks if the CLI binary exists in the installation path. | ||
| * | ||
| * @param binaryFileName The filename of the CLI binary. |
There was a problem hiding this comment.
Typographical/documentation issue: In the JSDoc for checkIfCliBinaryExists, the parameter is incorrectly documented as binaryFileName. Please update it to cliVersion to match the actual function signature.
Suggested change
| * @param binaryFileName The filename of the CLI binary. | |
| * @param cliVersion The version of the CLI. |
|
heads-up, the downloading must happen on the client-side, not in language-server/ |
|
check out greg/lsp |
|
Yeah I'm dumb, downloading logic is self contained anyway it's just moving the file to the client package. |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9285970 in 1 minute and 34 seconds
More details
- Looked at
131lines of code in5files - Skipped
1files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. vscode/src/plugins/language-server/cliDownloader.ts:21
- Draft comment:
Hardcoded URL for downloading (currently using ripgrep release) is likely for testing. Replace with the actual baml-cli release URL. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. vscode/src/plugins/language-server/cliDownloader.ts:161
- Draft comment:
The tar extraction path is hardcoded to a ripgrep subdirectory. Update this to target the actual baml-cli binary when available. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. vscode/src/plugins/language-server/cliDownloader.ts:155
- Draft comment:
Piping the tar extraction stream lacks error handling and completion confirmation. Consider awaiting the extraction completion to avoid potential race conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. vscode/src/plugins/language-server/index.ts:431
- Draft comment:
The cliVersion is hardcoded as "0.82.0". Consider dynamically determining the CLI version from configuration or environment. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. typescript/vscode-ext/packages/language-server/package.json:37
- Draft comment:
Removed dependencies (axios, extract-zip, tar) should be confirmed as no longer needed in this package. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the removed dependencies are no longer needed. This falls under the rule of not asking the author to confirm their intention or to double-check things. Therefore, this comment should be removed.
6. typescript/vscode-ext/packages/language-server/src/server.ts:392
- Draft comment:
CLI binary download code removal: ensure this functionality now moved to the client is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This comment violates several rules:
- It asks for verification/confirmation ("ensure this...is intentional")
- It's not clearly actionable - doesn't suggest a specific code change
- The code changes appear intentional and well-structured - the old download code is replaced with a config-based approach
- Understanding if this is correct would require more context about the client code that we don't have access to
The code change could potentially break functionality if the client hasn't properly taken over the CLI download responsibility.
While that's true, asking for verification isn't helpful - we should trust that the author has tested this change. If there are issues, they'll be caught in testing.
This comment should be deleted as it violates the rules by asking for verification without suggesting specific changes.
7. typescript/vscode-ext/packages/vscode/package.json:37
- Draft comment:
Dependency updates (axios to ^1.7.9 and addition of tar) appear deliberate; verify consistency with other packages. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
8. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:135
- Draft comment:
Hardcoded URL for download (using ripgrep release) is for testing; replace with the production baml-cli URL before release. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:156
- Draft comment:
Tar extraction is piped but not awaited; consider wrapping extraction in a promise to ensure completion and add error handling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:159
- Draft comment:
Modifying entry.path via onReadEntry is clever but may not be robust; test with various archives. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:431
- Draft comment:
CLI version is hardcoded as '0.82.0'; update dynamically or remove TODO before production. - 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%
The code already has a TODO comment indicating this needs to be updated. The PR comment essentially just restates what's already noted in the code itself. The author is clearly aware this is temporary since they added the TODO. Additionally, this could be an intentional first step in the implementation.
The hardcoded version could cause issues in production if not addressed, so maybe it's worth emphasizing.
The existing TODO comment already captures this concern, and the author is clearly aware. The PR comment adds no new information or actionable feedback.
Delete the comment since it just restates what's already noted in the TODO comment without providing additional actionable feedback.
12. typescript/vscode-ext/packages/language-server/src/server.ts:55
- Draft comment:
Typo: Consider changing "cant load webcrypto" to "can't load webcrypto" for proper grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. typescript/vscode-ext/packages/language-server/src/server.ts:305
- Draft comment:
Typo in comment: 'mutliple baml_src dirs' should be corrected to 'multiple baml_src dirs'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:139
- Draft comment:
Typo detected: please change 'dont delete this.' to 'don't delete this.' for correct punctuation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:242
- Draft comment:
Typo detected: please change 'root path doesnt match current file' to 'root path doesn't match current file'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:249
- Draft comment:
Typo detected: please change 'hasnt' to 'hasn't' in the comment for correct punctuation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_5dRTFQ5kcpD9Blr4
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| // TODO: Send notification, loading state, etc. | ||
| if (!(await checkIfCliBinaryExists(cliVersion))) { | ||
| await downloadCli(cliVersion) |
There was a problem hiding this comment.
No error handling for downloadCli failure; consider adding user feedback if CLI download fails.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on eccfc07 in 2 minutes and 6 seconds
More details
- Looked at
106lines of code in3files - Skipped
1files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/package.json:37
- Draft comment:
Ensure version consistency: Verify that the specified versions of adm-zip and its types align with other dependencies in the project. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:136
- Draft comment:
Hard-coded test URLs for ripgrep may lead to confusion. Consider using conditional logic for platform-specific releases or isolating testing code. - 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%
The code clearly shows this is temporary test code marked with TODO. The proper URL construction logic already exists but is commented out. This appears to be intentional testing/development code that the author is aware needs to be changed, as evidenced by multiple TODOs. The comment isn't providing new information or suggesting a specific fix that isn't already known.
The comment could be valid if this was production code, but given the TODO markers, the author is already aware this needs to be changed. Maybe there's a deeper architectural issue I'm missing?
No, the TODOs and commented out production code make it clear this is temporary test code that the author plans to fix. The comment doesn't add value beyond what's already indicated.
Delete the comment as it's pointing out an issue the author is already aware of, as indicated by TODO comments and the commented out production code.
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:173
- Draft comment:
Consider cleaning up the temporary ZIP file after extraction to avoid cluttering the INSTALL_PATH. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:163
- Draft comment:
Avoid hard-coded extraction paths in both tar and zip branches. Ensure dynamic determination of binary file paths based on version and platform. - 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%
The code is clearly in a testing/development state using ripgrep paths as placeholders. The TODOs already indicate the paths need to be changed. The comment is essentially restating what's already marked as TODO. The suggestion for "dynamic determination" is vague and the code already has functions like cliBinaryFileName() for dynamic path generation.
The comment might be pointing to a broader architectural concern about path handling that goes beyond just the TODOs. The suggestion for dynamic determination could prevent similar issues in future.
While the architectural suggestion has merit, the code already has dynamic path generation functions in place, and the hardcoded paths are clearly temporary test fixtures marked with TODOs. The comment doesn't provide specific actionable feedback beyond what's already noted.
Delete the comment as it restates existing TODOs without providing additional actionable insights. The temporary test paths are already marked for change.
5. typescript/vscode-ext/packages/vscode/package.json:37
- Draft comment:
Verify that the added dependency 'adm-zip' (^0.5.16) and its type definitions (^0.5.7) are appropriate and compatible with the project. This addition is fine if used only for CLI extraction. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:136
- Draft comment:
Hard-coded testing URL. Ensure to replace with a dynamic URL built from cliVersion/BASE_URL before production. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:167
- Draft comment:
No error handling is attached to the write stream for the zip extraction. Consider adding an 'error' event listener to catch and handle any stream errors. - Reason this comment was not posted:
Marked as duplicate.
8. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:432
- Draft comment:
The change from double to single quotes for the CLI version is a stylistic update; ensure consistency with the project’s coding style. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:139
- Draft comment:
Typo: Consider changing "dont delete this" to "don't delete this". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:242
- Draft comment:
Typo: Consider correcting "doesnt" to "doesn't" in the log message for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_MsB5efLFlN1KS6xx
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| ), | ||
| ) | ||
| } else if (extension === 'zip') { | ||
| const compressedFilePath = path.join(INSTALL_PATH, compressedFileName) |
There was a problem hiding this comment.
ZIP branch extraction lacks error handling. Consider adding error listeners to the stream and handling exceptions from AdmZip.
|
|
||
| // Due to the zip format, we can't use streaming APIs, we need the entire | ||
| // content to be downloaded before we can extract the binary. | ||
| stream.on('finish', () => { |
There was a problem hiding this comment.
The zip extraction callback on the write stream isn’t awaited, which may lead to a race condition. Consider wrapping the extraction in a Promise and awaiting its completion.
| cwd: INSTALL_PATH, | ||
| onReadEntry: (entry) => (entry.path = binaryFileName), | ||
| }, | ||
| ['ripgrep-14.1.1-x86_64-unknown-linux-musl/rg'], // TODO: Change this to ['./baml-cli'] |
There was a problem hiding this comment.
Hard-coded archive entry paths are used for both tar.gz and zip extractions; parameterize these using cliVersion for better maintainability.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d3725be in 1 minute and 16 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:80
- Draft comment:
Removed generator version consistency check. Confirm that relying solely on checkVersion(g, false) is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The old code enforced that all generators had the same version. The new code validates each generator's version individually against the VSCode version. This seems like an intentional architectural change to allow generators to have different versions, as long as they are each compatible with VSCode. The comment is asking for confirmation of intent, which violates our rules.
Maybe there was an important technical reason why all generators needed to be on the same version that isn't obvious from the code.
If there was a critical technical requirement for version consistency, it would have been preserved or documented. The change appears to be an intentional loosening of version constraints.
The comment should be deleted because it asks for confirmation of intent without identifying a clear technical issue. The change appears to be an intentional architectural decision.
2. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:80
- Draft comment:
Removed aggregated generator version check. Confirm that relying solely on individual checkVersion() calls yields the expected behavior (especially for version mismatches). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the behavior is as expected after removing a version check. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
3. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:146
- Draft comment:
Typo: The constant string 'BAML_PATH_SPLTTER' appears in several places (e.g. in update_runtime() and files()). Consider checking if this should be changed to 'BAML_PATH_SPLITTER' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_UwwAiZKdAiFnWkCI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0e71702 in 1 minute and 34 seconds
More details
- Looked at
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:174
- Draft comment:
Parameterize and comment on hard-coded entry path. Hard-coding paths like 'ripgrep-14.1.1-x86_64-pc-windows-msvc/rg.exe' makes maintenance harder. - Reason this comment was not posted:
Comment was on unchanged code.
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:172
- Draft comment:
Wrap async extraction operations in try/catch. - Reason this comment was not posted:
Marked as duplicate.
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:174
- Draft comment:
Verify AdmZip.extractEntryTo parameter order. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to verify the parameter order of a function call. This is similar to asking the author to double-check something, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue, making it unhelpful.
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:186
- Draft comment:
Consider platform-checking before forcing permissions change. - 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 Windows has a different permission model than Unix systems, chmod() is actually supported by Node.js fs module on Windows - it just maps to the closest equivalent Windows permissions. Setting executable permissions is necessary for CLI tools on all platforms. The comment seems to suggest extra complexity that isn't needed since Node.js handles the platform differences.
Maybe there are edge cases with Windows permissions that could cause issues? Maybe some Windows versions handle chmod differently?
Node.js fs.chmod is specifically designed to work cross-platform. Adding platform checks would add complexity without benefit since Node.js already handles the platform differences.
The comment should be deleted. The current code using fs.chmod is the correct cross-platform approach, and adding platform-specific handling would be unnecessary complexity.
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:189
- Draft comment:
Add error handling for file removal. - 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%
While error handling is generally good practice, in this case failing to delete a temporary file is not critical - the binary has already been extracted successfully. If the deletion fails, it's just leaving behind a temp file which isn't ideal but won't break functionality. The comment doesn't provide specific guidance on what error handling should look like.
Error handling could prevent uncaught promise rejections which might crash the process. Temp files building up could cause disk space issues over time.
The error would be caught by the outer promise rejection handler, and disk space from occasional temp files is not a critical concern for this use case. The comment doesn't provide enough value to justify the added complexity.
While error handling could be added, the benefit is minimal and the comment doesn't provide clear guidance. The comment should be removed.
Workflow ID: wflow_6hZrn7W8e28OxhBU
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| // Due to the zip format, we can't use streaming APIs, we need the entire | ||
| // content to be downloaded before we can extract the binary. | ||
| stream.on('finish', async () => { |
There was a problem hiding this comment.
Wrap async callback in try/catch. Async errors in stream 'finish' handler won’t propagate and might be swallowed.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5607794 in 1 minute and 58 seconds
More details
- Looked at
37lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:191
- Draft comment:
Removed commented-out code blocks. If these are not needed, consider cleaning them up entirely or adding an explanation if they're temporarily disabled. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:191
- Draft comment:
Consider removing the commented-out extraction code permanently (or linking it to an issue) if zip support isn’t planned immediately. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:438
- Draft comment:
Nice use of withProgress for download feedback. Consider passing incremental progress updates or handling cancellation if the download might be lengthy. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:139
- Draft comment:
Typo: In the comment '// dont delete this.', please add an apostrophe to change it to "// don't delete this." for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_fQ1S8AUJ4bpvU9oq
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| cancellable: false, | ||
| title: 'Downloading BAML LSP', | ||
| }, | ||
| async (progress, token) => { |
There was a problem hiding this comment.
Consider using progress.report inside the withProgress callback to update the UI as the download/extraction proceeds, or remove the unused progress and token parameters if not needed.
Suggested change
| async (progress, token) => { | |
| async (progress) => { |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 40444f6 in 2 minutes and 25 seconds
More details
- Looked at
161lines of code in2files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:160
- Draft comment:
Verify that the tar extraction mapping ('./baml-cli') correctly corresponds to the internal archive structure. - 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 something, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:172
- Draft comment:
Check that the zip extraction parameters (using './baml-cli.exe' and binary filename mapping) match the archive's file layout and the expected output binary name. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the author to check if the parameters match the expected layout and output, which is similar to asking them to double-check or ensure something. This violates the rule against asking the author to confirm or ensure things.
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:437
- Draft comment:
Review the commented-out 'if (version)' block. Decide whether to remove or reinstate it to avoid confusion regarding fallback behavior when no version is returned. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment raises a valid concern - there's unclear behavior when version is undefined, and the code currently assumes version exists by casting it to string. The TODO and commented-out if suggest this was intentionally left as a known issue. However, the comment is phrased as a suggestion to "review" rather than providing clear direction on what should be fixed.
The code does have a real issue with type safety and undefined handling, but the comment doesn't clearly specify what action should be taken. It's more of a "please think about this" than an actionable suggestion.
While the comment could be more specific, it points to a legitimate code quality issue that should be addressed - the unsafe type casting and unclear fallback behavior.
The comment should be kept since it identifies a real code quality issue around type safety and error handling, even though it could be more prescriptive in its suggestion.
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:126
- Draft comment:
Avoid mutating the input parameter with a hardcoded test version ('0.1.0'). Use a separate testing flag or variable instead. - Reason this comment was not posted:
Marked as duplicate.
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:69
- Draft comment:
Passing 'platform' through getReleasePlatform inside getCliCompressedFileExtension may be redundant if already transformed; consider reusing the formatted value. - 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%
The comment raises a valid point about potential redundancy, but looking at the actual usage, cliVersion.platform is passed directly from the raw platform string. The transformation is actually necessary here. The comment is speculative ("may be redundant if...") rather than identifying a definite issue. The code as written is correct and clear.
The comment identifies a potential code quality issue. Maybe there's value in making the code more robust against double-transformation?
The current code is correct as-is - the transformation is needed since we're getting raw platform strings. Making it more robust against double-transformation would add complexity without clear benefit.
The comment should be deleted as it's speculative and the current code is working correctly as intended.
6. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:432
- Draft comment:
Guard against an undefined 'version' from client.sendRequest('bamlCliVersion') before casting to string. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:438
- Draft comment:
There’s a version mismatch: the CLI version retrieved is used here, but downloadCli() overrides it to '0.1.0'. Ensure consistent version handling. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:40
- Draft comment:
Suggestion: Consider capitalizing the warning message for consistency. Instead of "no current baml file", consider using "No current BAML file". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:140
- Draft comment:
Suggestion: Fix the comment to read "don't delete this." instead of "dont delete this." to correct the typographical error. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_eDhTL8NUcGorPxPf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| export async function downloadCli(cliVersion: CliVersion): Promise<void> { | ||
| // TODO: Testing | ||
| cliVersion.version = '0.1.0' |
There was a problem hiding this comment.
Remove hardcoded version ('0.1.0') used for testing. This may override valid version values in production.
Suggested change
| cliVersion.version = '0.1.0' |
| const cliVersion = { | ||
| architecture: process.arch, | ||
| platform: process.platform, | ||
| version: version as string, |
There was a problem hiding this comment.
Validate that version is defined before casting it to a string; handling an undefined version may prevent downstream issues.
Suggested change
| version: version as string, | |
| version: version ? version as string : 'undefined', |
| stream.on('finish', async () => { | ||
| const zip = new AdmZip(compressedFilePath) | ||
| zip.extractEntryTo( | ||
| './baml-cli.exe', |
There was a problem hiding this comment.
The archive entry name ./baml-cli.exe is hardcoded. Parameterize or verify this to avoid breakage if archive structure changes.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f36d56b in 2 minutes and 2 seconds
More details
- Looked at
178lines of code in2files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:132
- Draft comment:
Hardcoded version ('0.1.0') is used. Remove or update this before production. - Reason this comment was not posted:
Comment was on unchanged code.
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:183
- Draft comment:
Overwriting entry.path in onReadEntry may affect extraction if multiple files exist. Verify intended behavior. - Reason this comment was not posted:
Comment was on unchanged code.
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:185
- Draft comment:
Revisit the use of onReadEntry to force 'entry.path' to the binary name; ensure it works robustly if the tar contains multiple entries. - Reason this comment was not posted:
Comment was on unchanged code.
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:196
- Draft comment:
Investigate why preserving original permissions with AdmZip.extractEntryTo isn't working and consider alternative approaches. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:176
- Draft comment:
Clarify the type of 'source' in extractFile. Axios returns a NodeJS.ReadableStream, so consider using the appropriate type instead of ReadableStream. - Reason this comment was not posted:
Marked as duplicate.
6. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:445
- Draft comment:
Consider implementing a fallback mechanism if downloadCli fails, rather than only showing an error message. - Reason this comment was not posted:
Comment was on unchanged code.
7. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:141
- Draft comment:
Minor typographical issue: The comment at this location reads "dont delete this.". Consider updating it to "don't delete this." for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_SqKJEtDiA7IGT6x8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| * to disk. | ||
| */ | ||
| async function extractFile( | ||
| source: ReadableStream, |
There was a problem hiding this comment.
The source parameter is typed as ReadableStream but axios returns a NodeJS.ReadableStream. Adjust the type for clarity.
Suggested change
| source: ReadableStream, | |
| source: NodeJS.ReadableStream, |
| */ | ||
| export async function downloadCli(cliVersion: CliVersion): Promise<void> { | ||
| // TODO: Testing | ||
| cliVersion.version = '0.1.0' |
There was a problem hiding this comment.
Avoid hardcoding the CLI version ('0.1.0'); use the passed version or configure it externally.
Suggested change
| cliVersion.version = '0.1.0' |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5ecd516 in 2 minutes and 29 seconds
More details
- Looked at
211lines of code in4files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:80
- Draft comment:
generatorVersion returns only the first generator's version. Consider checking that all generators have the same version. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:93
- Draft comment:
checkVersionOnSave compares each generator's version to the first one. Confirm this is sufficient or if a more robust version consistency check is needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. typescript/vscode-ext/packages/language-server/src/server.ts:329
- Draft comment:
onDidOpen sends 'setLspVersion' on every document open if version exists; consider caching the last version to avoid redundant requests. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. typescript/vscode-ext/packages/language-server/src/server.ts:391
- Draft comment:
TODO comment mentions checking version differences; ensure this is implemented before production to avoid unnecessary generator runs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:139
- Draft comment:
The download URL is mocked using version '0.1.0'. Remember to remove this mock before releasing to production. - 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 is addressing code that was just changed in this PR. The change appears to be intentionally adding a mock/temporary solution, as evidenced by the TODO comment. The comment correctly identifies a potential issue - this mock shouldn't make it to production. However, the code's own TODO comment already makes this clear, making the review comment redundant.
The comment might be helpful as an extra safeguard against accidentally releasing with mock code. Also, it provides the exact code suggestion for how to fix it.
While the comment is technically correct, it's redundant with the TODO comment already in the code. The author is clearly aware this is temporary since they added the TODO themselves.
The comment should be deleted because it's redundant with the TODO comment already in the code and doesn't provide additional value.
6. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:264
- Draft comment:
In the 'setLspVersion' handler, consider checking if the new LSP version differs from the current version before triggering a download. - Reason this comment was not posted:
Marked as duplicate.
7. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:80
- Draft comment:
Consider caching the result of this.list_generators() in generatorVersion() to avoid duplicate method calls. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. typescript/vscode-ext/packages/vscode/src/plugins/language-server/cliDownloader.ts:142
- Draft comment:
A mock URL is used for CLI download. Ensure this is replaced with the actual versioned URL before production release. - 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 a mock URL is replaced with a real one before production. This is a reminder to make a change before release, which is not a specific code suggestion or a request for a test. It falls under the category of asking the author to ensure something, which is against the rules.
9. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:264
- Draft comment:
Before initiating the CLI binary download in the 'setLspVersion' handler, verify that the new version differs from the currently running version to prevent unnecessary downloads. - Reason this comment was not posted:
Marked as duplicate.
10. typescript/vscode-ext/packages/language-server/src/lib/baml_project_manager.ts:166
- Draft comment:
The string literal 'BAML_PATH_SPLTTER' might be a typographical error. It probably should be 'BAML_PATH_SPLITTER' for clarity and consistency, unless it is intentionally spelled that way. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. typescript/vscode-ext/packages/language-server/src/server.ts:55
- Draft comment:
Typo: Consider changing "cant load webcrypto" to "can't load webcrypto". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. typescript/vscode-ext/packages/language-server/src/server.ts:305
- Draft comment:
Typo: In the comment, 'mutliple' should be corrected to 'multiple'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:140
- Draft comment:
Minor typo: please change "dont delete this." to "don't delete this." for proper punctuation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:243
- Draft comment:
Typographical error: please update "root path doesnt match current file" to "root path doesn't match current file". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Dzfn3moanYEqN17O
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| break | ||
| } | ||
|
|
||
| // const message = this.checkVersion(g, false) |
There was a problem hiding this comment.
Remove or update the commented-out checkVersion() calls in checkVersionOnSave() to keep the code clean.
| if (project) { | ||
| const version = project.generatorVersion() | ||
| if (version) { | ||
| connection.sendRequest('setLspVersion', version) |
There was a problem hiding this comment.
Consider sending setLspVersion only when the new version differs from the current version to avoid redundant requests. (See TODO comment.)
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5adfea1 in 2 minutes and 2 seconds
More details
- Looked at
81lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:17
- Draft comment:
Imported cliBinaryPath is currently unused (only in commented code). Remove if not needed. - 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%
The code shows clear intent to use cliBinaryPath in the near future with a TODO comment. The import is not an oversight or mistake, but rather preparation for implementing the commented out section. Suggesting to remove it would be counterproductive since it will be needed soon.
The comment is technically correct that the import is currently unused. Maybe keeping unused imports, even if intended for future use, is bad practice.
While unused imports are generally bad practice, in this case there is clear documented intent to use it imminently. The TODO comment and commented implementation show this is intentional preparation, not an oversight.
The comment should be deleted. The unused import is intentionally included for an upcoming implementation that is already drafted in comments.
2. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:300
- Draft comment:
Commented-out block for LSP restart should be revisited. Either activate restart logic or remove TODO. - Reason this comment was not posted:
Marked as duplicate.
3. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:17
- Draft comment:
The new import 'cliBinaryPath' is currently unused (only referenced in commented code). Consider removing it until it's needed. - 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%
The import is part of a clear work-in-progress feature - there's a TODO comment and commented-out code that will use this import. The author likely added the import proactively while working on the LSP restart feature. Suggesting to remove it now would be counterproductive since it will be needed very soon when the TODO is addressed.
The comment is technically correct that the import is currently unused. There's a small chance the TODO might never be implemented.
The presence of detailed commented-out code and clear TODO strongly suggests this is active work in progress, not abandoned code. The import will be needed very soon.
We should delete this comment. The unused import is intentional and will be used when the clearly marked TODO is implemented.
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:271
- Draft comment:
The 'restartLsp' flag is always set to true (both after a successful download and in the else branch) but is never used because the restart logic remains commented out. Clarify intended behavior or remove the redundant flag. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:300
- Draft comment:
The LSP restart logic is commented out. If it’s planned for future work, consider referencing a tracking issue; otherwise, remove dead code to keep the codebase clean. - Reason this comment was not posted:
Marked as duplicate.
6. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:141
- Draft comment:
Minor typographical error: Consider changing "dont delete this." to "don't delete this." for proper grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. typescript/vscode-ext/packages/vscode/src/plugins/language-server/index.ts:245
- Draft comment:
Typographical note: Consider changing "root path doesnt match current file" to "root path doesn't match current file". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_isLEu9rsOEvJbMyN
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| version: version as string, | ||
| } | ||
|
|
||
| let restartLsp = false |
There was a problem hiding this comment.
restartLsp is set via withProgress call but not acted upon. Consider implementing or removing related logic.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bbd9e90 in 1 minute and 23 seconds
More details
- Looked at
1895lines of code in25files - Skipped
5files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. typescript/fiddle-frontend/package.json:25
- Draft comment:
Updated '@radix-ui/react-alert-dialog' version. Ensure that the upgrade does not introduce breaking changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that the upgrade does not introduce breaking changes, which violates the rule against asking the author to ensure behavior is intended or tested. It also relates to dependency changes, which should not be commented on unless there's a specific issue identified.
2. typescript/playground-common/src/shared/baml-project-panel/playground-panel/side-bar/env-vars.tsx:82
- Draft comment:
Using requestAnimationFrame for syncing state is clever, but consider potential edge cases. A debounced or throttled update might provide more predictable behavior. - 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%
The current rAF implementation is actually a good choice here - it batches updates to happen on the next frame, prevents multiple updates in the same frame, and includes proper cleanup. Debounce/throttle would add complexity without clear benefits. The current code follows React best practices for performance optimization.
The comment raises a valid point about potential edge cases, and debounce/throttle are common solutions for rate-limiting updates. Maybe there are scenarios where rapid updates could cause issues.
While debounce/throttle are useful patterns, rAF is perfectly suitable here and matches React's rendering model. The code already handles the key edge case by cleaning up pending frames.
The current implementation using rAF is appropriate and follows React best practices. The suggested change would add unnecessary complexity.
3. typescript/playground-common/src/shared/baml-project-panel/playground-panel/side-bar/env-vars.tsx:93
- Draft comment:
Toggling visibility based on index may be unstable if the environment variables order changes. Consider using a key-based update. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While using array indices can sometimes be dangerous, in this case the index is only used transiently to look up the key, which is then used for the actual state update. The visibility state is stored by key in envVarVisibilityAtom, not by index. Even if the order changes, the correct key-value mapping will be maintained. The UI also uses key={env.key} for React rendering stability.
The comment raises a valid general concern about array index usage. Could there be edge cases where the array is modified between reading the index and using it?
The index usage is safe here because it's used synchronously within a single function call to look up a key, and the actual state is maintained by key not index. React's rendering model prevents race conditions.
The comment raises a theoretical concern but the actual implementation is safe because indices are only used transiently to look up keys, which are then used for the persistent state.
4. typescript/playground-common/src/index.ts:10
- Draft comment:
Exporting 'EnvVars' from the side-bar might be confusing when multiple variants exist. Ensure the naming and usage in consumers are consistent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. vscode-ext/packages/web-panel/.eslintrc.cjs:4
- Draft comment:
Including 'plugin:storybook/recommended' extends advisable Storybook linting rules. Verify that these rules do not conflict with project-specific configurations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. playground-common/src/shared/baml-project-panel/playground-panel/side-bar/env-vars.tsx:27
- Draft comment:
Using 'parse' from 'dotenv' in client-side code may be risky. Verify that it bundles and functions correctly in the browser environment. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. playground-common/src/shared/baml-project-panel/playground-panel/side-bar/env-vars.tsx:80
- Draft comment:
The useEffect with requestAnimationFrame sync for updating the atom is clever, but ensure rapid state changes are handled as expected. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. vscode-ext/packages/web-panel/.storybook/main.ts:9
- Draft comment:
Consider specifying a more precise return type for getAbsolutePath instead of 'any' for improved type safety. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
Workflow ID: wflow_toKDHzGGR9osRa3Z
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Skipped PR review on 2582351 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
Skipped PR review on d1ae6e8 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 6afe988 in 3 minutes and 20 seconds. Click for details.
- Reviewed
1426lines of code in15files - Skipped
1files when reviewing. - Skipped posting
17draft 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. vscode-ext/packages/vscode/src/plugins/language-server-client/cliDownloader.ts:142
- Draft comment:
Nice change: usingcliVersion.versiondynamically in the URL instead of a hardcoded '0.1.0' improves maintainability. Ensure the Github release structure remains consistent with the constructed URL. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:292
- Draft comment:
In thedownloadLspfunction, the asynchronous download is initiated and its result determines whether to restart the LSP. Consider awaitingdownloadLsp()to ensure proper sequential execution, and possibly handling errors or retries. Also, the fallback using an executeCommand trigger might need further review. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. vscode-ext/packages/vscode/src/util.ts:130
- Draft comment:
The re-registration of event handlers after restarting the client inrestartClientis clear and ensures diagnostics continue as expected. Make sure to monitor if duplicate handlers could be registered on multiple restarts. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/cliDownloader.ts:142
- Draft comment:
Good update: the URL now dynamically uses cliVersion. Ensure that the download stream is robustly handled and errors properly propagated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:229
- Draft comment:
Duplicate registration for 'baml_settings_updated' event detected. Remove one instance to avoid redundant handling. - Reason this comment was not posted:
Comment was on unchanged code.
6. engine/language_server/src/baml_project/mod.rs:361
- Draft comment:
It appears that the delimiter string 'BAML_PATH_SPLTTER' may be a typographical error. It might be intended to be 'BAML_PATH_SPLITTER' (with an extra 'I'). Please confirm and correct in all instances if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. engine/language_server/src/server/api/diagnostics.rs:417
- Draft comment:
Typographical error: In the documentation for theensure_absolutefunction, the word 'taylored' should be corrected to 'tailored' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. test-files/aliases/enums.baml:7
- Draft comment:
The comment 'tests whether k1 doesnt incorrectly get matched with k11' is missing an apostrophe in 'doesnt'. It should be "doesn't" to be grammatically correct. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. integ-tests/typescript/baml_client/inlinedbaml.ts:5
- Draft comment:
Typographical error: In the comment near the alias for 'k1', 'doesnt' should be corrected to "doesn't" for proper punctuation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. integ-tests/typescript/baml_client/inlinedbaml.ts:2
- Draft comment:
Typographical concern: In the description for 'key-dash', the phrase 'af asdf' appears, which looks like placeholder text or an unintended typographical error. Please review and update if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/cliDownloader.ts:27
- Draft comment:
Consider updating 'Github' to 'GitHub' in the documentation comment to properly reflect the official branding. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/cliDownloader.ts:44
- Draft comment:
Please change 'Github' to 'GitHub' in the comment to adhere to proper branding conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/cliDownloader.ts:63
- Draft comment:
Consider revising 'Github' to 'GitHub' in the comment to maintain consistency with the official naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:137
- Draft comment:
Typo: Please change "dont delete this." to "don't delete this." for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically about a changed line, it's only suggesting a minor grammatical fix to a comment. Comments about code comments are generally not valuable unless they point out serious issues with documentation. This is just about adding an apostrophe, which doesn't affect functionality or clarity. The comment is technically correct - it is a real typo. And some might argue that maintaining proper grammar in comments is important for professionalism. However, this kind of minor grammatical nitpick in comments creates noise without adding value. The meaning is perfectly clear without the apostrophe. Delete this comment as it's an unimportant grammatical nitpick about a code comment that doesn't affect understanding.
15. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:253
- Draft comment:
Typo: Change "root path doesnt match current file" to "root path doesn't match current file". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a very minor change to a debug logging message that only developers would see. It's not affecting functionality or code quality in any meaningful way. The message is perfectly understandable as is. According to the rules, we should not make comments that are obvious or unimportant. The typo could be seen as unprofessional, and consistent use of apostrophes is part of good coding style. Maybe this is worth fixing for consistency? While consistency is good, this is just a debug log message. The rules specifically say not to make comments that are obvious or unimportant. This is definitely in the "unimportant" category. Delete this comment. It's suggesting a trivial change to a debug message that doesn't impact code quality or functionality.
16. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:383
- Draft comment:
Typo: Update "hasnt been picked up yet" to "hasn't been picked up yet" in the comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the missing apostrophe, comments about typos in comments are extremely low value. They don't affect functionality and are purely cosmetic. The rules state to only keep comments that clearly require code changes, and to not make purely informative comments. The typo could be seen as reducing code quality and professionalism. Some might argue that maintaining proper grammar in comments is important for readability. While clean comments are nice, this is an extremely minor issue that doesn't impact understanding. The rules explicitly say not to make purely informative comments and to only comment if there's clearly a code change required. Delete this comment. It's about a trivial typo in a comment that doesn't affect functionality or understanding.
17. typescript/vscode-ext/packages/vscode/src/util.ts:29
- Draft comment:
Typographical error: The message 'You have both both versions...' should be corrected to 'You have both versions...'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_zWeJKKsTXG7Drgak
You can customize 
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The async function downloadLsp() is invoked without being awaited. Consider awaiting it or handling its promise chain to capture potential errors.
| // We need the clientOptions here, which are not in scope. Pass them or adjust restartClient | ||
| // For now, assuming restartClient has access or doesn't need them updated dynamically here | ||
| // const clientOptions: LanguageClientOptions = { /* ... */ }; // This is tricky | ||
| await commands.executeCommand('baml.restartLanguageServer') // Trigger restart via command |
There was a problem hiding this comment.
The LSP restart mechanism currently triggers a command (baml.restartLanguageServer) as a workaround. Consider refactoring to centralize the LSP restart logic (e.g. via the restartClient function) for consistency.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
Introduces LSP version management and downloader for BAML, ensuring generator version consistency and handling LSP binary downloads.
get_common_generator_version()inbaml_project/mod.rsto check generator version consistency.server.tsandbaml_project_manager.ts.cliDownloader.tsfor downloading LSP binaries based on version.did_open.rsanddid_save_text_document.rsto handle LSP version notifications.semvertoCargo.tomlandCargo.lockfor version parsing.adm-zipandtartopackage.jsonfor handling compressed files.baml_clientversion ingenerators.baml,inlinedbaml.py, andindex.tsto0.84.4.partial_types.pyfrombaml_client.server.tsandindex.ts.This description was created by
for 6afe988. You can customize this summary. It will automatically update as commits are pushed.