fix: correct language server CLI arguments#81
fix: correct language server CLI arguments#81pmig wants to merge 1 commit intonathansbradshaw:mainfrom
Conversation
- Return probe locations as single comma-separated string instead of separate argv entries. The Angular LS CLI parser (cmdline_utils.ts) reads argv[index+1] and splits on commas, so multiple separate args caused only the first path to be used. - Make --tsdk path absolute by joining with current_dir, removing ambiguity when the server process runs in the user's project directory. - Add early return in server_script_path() when packages are already installed, preventing unnecessary reinstallation on every file open. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes three critical issues with the Angular Language Server extension's CLI argument handling that prevented proper language server functionality. The fixes address argument parsing issues that caused only the first probe location to be used, incorrect tsdk path resolution, and unnecessary download attempts on every file open.
Changes:
- Modified probe location functions to return comma-separated strings instead of arrays, fixing CLI argument parsing
- Changed tsdk path from relative to absolute by joining with current directory
- Added early return in
server_script_path()to prevent redundant downloads when server is already installed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| args.extend(Self::get_ng_probe_locations(Some(worktree))); | ||
| args.push(Self::get_ng_probe_locations(Some(worktree))); | ||
|
|
||
| let tsdk_path = current_dir.join(TYPESCRIPT_TSDK_PATH); |
There was a problem hiding this comment.
The tsdk_path construction relies on current_dir from line 174, which uses unwrap_or(PathBuf::new()). If env::current_dir() fails, current_dir will be an empty PathBuf, causing this join operation to produce a relative path instead of an absolute path, which defeats the purpose of this fix. Consider checking if current_dir is empty and returning an error, or using the existing Self::get_current_dir() helper method that properly handles errors.
| if self.did_find_server && server_exists { | ||
| zed::set_language_server_installation_status( | ||
| language_server_id, | ||
| &zed::LanguageServerInstallationStatus::CheckingForUpdate, | ||
| ); | ||
|
|
||
| // TODO only install new version if there are change | ||
| return Ok(SERVER_PATH.to_string()); | ||
| } |
There was a problem hiding this comment.
The early return prevents version updates when the user changes angular_language_server_version or typescript_version in their settings. If the server is already installed with version A, and the user changes settings to version B, the early return will skip the installation of version B because did_find_server is true and the old files still exist. Consider checking if the requested versions match the installed versions before returning early, or storing the installed versions in the struct to compare against.
| @@ -147,7 +142,7 @@ impl AngularExtension { | |||
| paths.push(worktree.root_path()); | |||
| } | |||
|
|
|||
| paths | |||
| paths.join(",") | |||
| } | |||
There was a problem hiding this comment.
The functions get_ng_probe_locations and get_ts_probe_locations are identical in implementation. Consider consolidating them into a single function like get_probe_locations to reduce code duplication and improve maintainability.
This actually a series a fixes to make that extension work again. You can try out the extension locally over at https://github.com/pmig/zed-angular. Although most of the code was written by claude, I (@pmig) manually reviewed and tested it.
Summary
--tsProbeLocationsand--ngProbeLocationswere passed as separate argv entries, but the Angular LS CLI parser (cmdline_utils.ts) readsargv[index+1]and splits on commas. Only the first path was used; the second became a dangling positional arg that shifted all subsequent flags. Fixed by returning a single comma-separated string.node_modules/typescript/libwas passed as a relative path. Since the language server process runs with cwd set to the user's project, this resolved incorrectly. Fixed by joining withcurrent_dirto produce an absolute path.server_script_path()had no early return when packages were already installed, causing visible "Downloading" status and network requests on every file open. Added early return whendid_find_server && server_exists.Relates to #77, #69