feat(vscode): add configurable Node.js path for language server#2773
Merged
feat(vscode): add configurable Node.js path for language server#2773
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
🦋 Changeset detectedLatest commit: 2f971b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📝 WalkthroughWalkthroughAdds a configurable Node.js executable path for the VS Code extension's language server, plus a changeset, localization entry, Claude settings file, and client logic to apply the configured runtime, prompt for restarts, and surface startup failures. Changes
Sequence DiagramsequenceDiagram
actor User
participant Config as Extension Config
participant Client as Language Client
participant Server as Language Server
User->>Config: Update likec4.node.path
Config->>Client: Emit configuration change
Client->>Client: Compute new nodeRuntime (watchEffect)
Client->>User: Prompt to restart extension host
User->>Client: Confirm restart
Client->>Server: Start/Initialize using nodeRuntime
Server-->>Client: Ready / Running
alt Server fails to start
Server-->>Client: Error / Stopped
Client->>User: Show error with "Configure Node" action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/vscode/package.json (1)
189-193: Consider adding atitleproperty for consistency with similar settings.The
likec4.graphviz.pathsetting (line 152-156) includes atitleproperty, but this new setting omits it. Adding a title would improve consistency in the VS Code settings UI.♻️ Suggested improvement
"likec4.node.path": { "type": "string", + "title": "%ext.config.node.path.title%", "default": "", "description": "Path to the Node.js executable used to run the language server. If empty, uses 'node' from PATH." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vscode/package.json` around lines 189 - 193, The new VS Code setting "likec4.node.path" lacks a title property; add a "title" field (e.g., "Node.js Path") to the likec4.node.path JSON object to match the style used by the existing likec4.graphviz.path setting so the setting appears with a consistent label in the VS Code UI.packages/vscode/src/node/useLanguageClient.ts (3)
47-47: ThenodeRuntimeis computed once at initialization—it won't react to config changes.The
nodeRuntimevalue is calculated during module initialization and used inserverOptions. If the user changeslikec4.node.pathafter the extension activates, the language client would still use the original runtime path until the extension host restarts. This behavior is acceptable given the restart prompt, but consider adding a comment to clarify this is intentional.Also applies to: 51-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vscode/src/node/useLanguageClient.ts` at line 47, The nodeRuntime variable is currently computed once from config.node?.path and captured into serverOptions so it won't update if the user changes likec4.node.path at runtime; update the file (around the nodeRuntime declaration and serverOptions usage in useLanguageClient.ts, including the related block covering lines 51–90) by adding a clear comment above the nodeRuntime declaration and above serverOptions stating this behavior is intentional and that changes require an extension host restart (or that you chose not to react to runtime config changes), referencing the nodeRuntime symbol, the config object, and serverOptions so future readers understand why it is not recomputed dynamically.
162-178: ThewatchEffect+watchpattern is redundant.The
nodepathref is initialized fromconfig.node.path, thenwatchEffectre-syncs it on every change, andwatchobservesnodepath. This indirection is unnecessary—you can watchconfig.node.pathdirectly via a computed or usewatchEffectalone with the restart logic.Also, with
once: true, users won't see the restart prompt if they change the path multiple times before restarting.♻️ Simplified approach
- const nodepath = ref(config.node.path) - - watchEffect(() => { - nodepath.value = config.node.path - }) - - watch(nodepath, (_newPath) => { + watch(() => config.node.path, (_newPath, oldPath) => { + if (oldPath === undefined) return // Skip initial vscode.window.showInformationMessage( 'Run command "Restart Extension Host" to use the updated Node.js path', 'Restart Now', ).then((selection) => { if (!selection) { return } vscode.commands.executeCommand('workbench.action.restartExtensionHost') }) - }, { - once: true, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vscode/src/node/useLanguageClient.ts` around lines 162 - 178, The current pattern creates an unnecessary indirection by syncing the nodepath ref via watchEffect and then watching nodepath; remove the nodepath ref and its watchEffect and instead watch config.node.path directly (or use a computed that returns config.node.path) in the watch callback that shows the restart prompt; also remove the { once: true } option so the prompt is shown on every change (or explicitly decide the intended one-time behavior) — update the watch call that references nodepath to reference config.node.path (or the computed) and keep the existing restart prompt logic inside that watcher.
143-160: Startup failure detection may trigger false positives.The current logic calls
suggestChangeNode()whenever the client reachesState.Stopped, even if it previously reachedState.Running. If the server starts successfully but stops normally within 5 seconds (e.g., user closes VS Code or deactivates extension), the error message would still appear.Consider tracking whether the server successfully reached
Runningstate before showing the error.♻️ Proposed improvement
+ let hasReachedRunning = false + const onDidChangeState = useDisposable( client.onDidChangeState(({ newState }) => { if (newState === State.Running) { + hasReachedRunning = true // If the server is running for 5 seconds, // we can assume it started successfully and unsubscribe setTimeout(() => { if (client.isRunning()) { onDidChangeState.dispose() } }, 5000).unref() } // If the server stops within the first 5 seconds after starting, suggest checking Node.js version - if (newState === State.Stopped) { + if (newState === State.Stopped && !hasReachedRunning) { suggestChangeNode() onDidChangeState.dispose() } }), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vscode/src/node/useLanguageClient.ts` around lines 143 - 160, The stop handler currently calls suggestChangeNode() on any State.Stopped transition which can false-positive after a successful run; update the onDidChangeState callback (the client.onDidChangeState used with useDisposable) to track whether the server ever reached State.Running (e.g., a boolean reachedRunning set to true when newState === State.Running) and only call suggestChangeNode() when newState === State.Stopped and reachedRunning is false; retain the existing disposal logic (onDidChangeState.dispose()) and the 5s timeout/client.isRunning check unchanged..claude/settings.json (1)
1-18: This file appears unrelated to the PR objective.This Claude Code settings file grants development permissions but isn't part of the "configurable Node.js path" feature. Consider whether it should be in a separate commit or PR to keep the feature commit focused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/settings.json around lines 1 - 18, The .claude/settings.json change (the JSON object containing "permissions" -> "allow" with entries like "Bash(gh *)", "Bash(git *)", etc.) is unrelated to the configurable Node.js path feature; remove this file from the current commit/PR or move it to a separate commit/PR so the feature branch only contains relevant changes. Undo or revert the .claude/settings.json addition from the branch (e.g., remove it from the index or create a new commit that deletes it) and re-run the PR so only files related to the Node.js path feature remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vscode/src/node/useLanguageClient.ts`:
- Line 29: The access to config.node is inconsistent: change the initialization
of nodepath in useLanguageClient.ts (symbol: nodepath) to match the optional
chaining used later (or remove optional chaining later if config.node is
guaranteed); specifically, either initialize nodepath using config.node?.path
(to safely handle undefined config.node) or remove the ?. in any other
references (e.g., where config.node?.path is used) to assume config.node is
always present—pick one approach and apply it consistently across nodepath and
all uses of config.node.path.
- Line 31: The destructured variable logger from useExtensionLogger() in
useLanguageClient.ts is unused; remove logger from the destructuring (leave
output) to eliminate the unused variable warning. Update the line that currently
reads "const { output, logger } = useExtensionLogger()" to only destructure
output so that useLanguageClient no longer references the unused logger symbol.
---
Nitpick comments:
In @.claude/settings.json:
- Around line 1-18: The .claude/settings.json change (the JSON object containing
"permissions" -> "allow" with entries like "Bash(gh *)", "Bash(git *)", etc.) is
unrelated to the configurable Node.js path feature; remove this file from the
current commit/PR or move it to a separate commit/PR so the feature branch only
contains relevant changes. Undo or revert the .claude/settings.json addition
from the branch (e.g., remove it from the index or create a new commit that
deletes it) and re-run the PR so only files related to the Node.js path feature
remain.
In `@packages/vscode/package.json`:
- Around line 189-193: The new VS Code setting "likec4.node.path" lacks a title
property; add a "title" field (e.g., "Node.js Path") to the likec4.node.path
JSON object to match the style used by the existing likec4.graphviz.path setting
so the setting appears with a consistent label in the VS Code UI.
In `@packages/vscode/src/node/useLanguageClient.ts`:
- Line 47: The nodeRuntime variable is currently computed once from
config.node?.path and captured into serverOptions so it won't update if the user
changes likec4.node.path at runtime; update the file (around the nodeRuntime
declaration and serverOptions usage in useLanguageClient.ts, including the
related block covering lines 51–90) by adding a clear comment above the
nodeRuntime declaration and above serverOptions stating this behavior is
intentional and that changes require an extension host restart (or that you
chose not to react to runtime config changes), referencing the nodeRuntime
symbol, the config object, and serverOptions so future readers understand why it
is not recomputed dynamically.
- Around line 162-178: The current pattern creates an unnecessary indirection by
syncing the nodepath ref via watchEffect and then watching nodepath; remove the
nodepath ref and its watchEffect and instead watch config.node.path directly (or
use a computed that returns config.node.path) in the watch callback that shows
the restart prompt; also remove the { once: true } option so the prompt is shown
on every change (or explicitly decide the intended one-time behavior) — update
the watch call that references nodepath to reference config.node.path (or the
computed) and keep the existing restart prompt logic inside that watcher.
- Around line 143-160: The stop handler currently calls suggestChangeNode() on
any State.Stopped transition which can false-positive after a successful run;
update the onDidChangeState callback (the client.onDidChangeState used with
useDisposable) to track whether the server ever reached State.Running (e.g., a
boolean reachedRunning set to true when newState === State.Running) and only
call suggestChangeNode() when newState === State.Stopped and reachedRunning is
false; retain the existing disposal logic (onDidChangeState.dispose()) and the
5s timeout/client.isRunning check unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdbe1446-3b74-4d8a-84a7-a418ee09ba13
📒 Files selected for processing (5)
.changeset/vscode-node-path-setting.md.claude/settings.jsonpackages/vscode/package.jsonpackages/vscode/package.nls.jsonpackages/vscode/src/node/useLanguageClient.ts
- Remove unused `logger` variable - Fix inconsistent optional chaining on config.node - Simplify watchEffect + watch into direct watch on config.node.path - Guard startup failure detection against false positives - Remove unused imports (ref, watchEffect) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vscode/src/node/useLanguageClient.ts`:
- Around line 123-160: The onDidChangeState handler currently disposes itself
after the first stable run so later restarts that fail won’t trigger
suggestChangeNode; update the listener logic on client.onDidChangeState (and the
hasReachedRunning flag) to retain the handler across restarts and detect an
actual failed start transition (Starting -> Stopped) instead of disposing on
first Running/timeout. Concretely: track the previous state (e.g., prevState
variable), remove the onDidChangeState.dispose() calls (and the timeout-based
dispose), call suggestChangeNode() when prevState === State.Starting && newState
=== State.Stopped (or when newState === State.Stopped && !hasReachedRunning),
and rely on the existing once() wrapper around suggestChangeNode to ensure the
prompt only appears once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05798e87-8da3-41ed-b5da-fc4442e72d2e
📒 Files selected for processing (1)
packages/vscode/src/node/useLanguageClient.ts
Comment on lines
+123
to
+160
| const suggestChangeNode = once(() => { | ||
| const message = | ||
| 'Language server failed to start. This may be caused by an incompatible Node.js version. Please make sure you have Node.js 22.22 or later installed and configured in the extension settings.' | ||
| output.error(message) | ||
| output.show(true) | ||
| vscode.window | ||
| .showErrorMessage( | ||
| message, | ||
| 'Configure Node', | ||
| ) | ||
| .then(selection => { | ||
| if (selection === 'Configure Node') { | ||
| vscode.commands.executeCommand('workbench.action.openSettings', 'likec4.node.path') | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| let hasReachedRunning = false | ||
|
|
||
| const onDidChangeState = useDisposable( | ||
| client.onDidChangeState(({ newState }) => { | ||
| if (newState === State.Running) { | ||
| hasReachedRunning = true | ||
| // If the server is running for 5 seconds, | ||
| // we can assume it started successfully and unsubscribe | ||
| setTimeout(() => { | ||
| if (client.isRunning()) { | ||
| onDidChangeState.dispose() | ||
| } | ||
| }, 5000).unref() | ||
| } | ||
| // If the server never reached Running, suggest checking Node.js version | ||
| if (newState === State.Stopped && !hasReachedRunning) { | ||
| suggestChangeNode() | ||
| onDidChangeState.dispose() | ||
| } | ||
| }), | ||
| ) |
Contributor
There was a problem hiding this comment.
This startup-failure detection only covers the first start attempt.
The wrapper in packages/vscode/src/useLanguageClient.ts (Lines 43-53) restarts the same client instance later, but this listener disposes itself after the first stable run. Any later Starting -> Stopped failure will skip suggestChangeNode(), which narrows the new UX to the initial activation only. Since suggestChangeNode is already wrapped in once(), you can watch the actual failed-start transition directly and keep the handler armed across restarts.
♻️ Proposed fix
- let hasReachedRunning = false
-
- const onDidChangeState = useDisposable(
- client.onDidChangeState(({ newState }) => {
- if (newState === State.Running) {
- hasReachedRunning = true
- // If the server is running for 5 seconds,
- // we can assume it started successfully and unsubscribe
- setTimeout(() => {
- if (client.isRunning()) {
- onDidChangeState.dispose()
- }
- }, 5000).unref()
- }
- // If the server never reached Running, suggest checking Node.js version
- if (newState === State.Stopped && !hasReachedRunning) {
- suggestChangeNode()
- onDidChangeState.dispose()
- }
- }),
- )
+ useDisposable(
+ client.onDidChangeState(({ oldState, newState }) => {
+ if (oldState === State.Starting && newState === State.Stopped) {
+ suggestChangeNode()
+ }
+ }),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vscode/src/node/useLanguageClient.ts` around lines 123 - 160, The
onDidChangeState handler currently disposes itself after the first stable run so
later restarts that fail won’t trigger suggestChangeNode; update the listener
logic on client.onDidChangeState (and the hasReachedRunning flag) to retain the
handler across restarts and detect an actual failed start transition (Starting
-> Stopped) instead of disposing on first Running/timeout. Concretely: track the
previous state (e.g., prevState variable), remove the onDidChangeState.dispose()
calls (and the timeout-based dispose), call suggestChangeNode() when prevState
=== State.Starting && newState === State.Stopped (or when newState ===
State.Stopped && !hasReachedRunning), and rely on the existing once() wrapper
around suggestChangeNode to ensure the prompt only appears once.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
likec4.node.pathsetting to configure the Node.js binary used to run the language serverTest plan
nodefrom PATH🤖 Generated with Claude Code
Summary by CodeRabbit