Skip to content

fix(lsp): improve custom server transport setup#1998

Merged
bajrangCoder merged 3 commits intoAcode-Foundation:mainfrom
bajrangCoder:fix/lsp-custom-server-transports
Mar 31, 2026
Merged

fix(lsp): improve custom server transport setup#1998
bajrangCoder merged 3 commits intoAcode-Foundation:mainfrom
bajrangCoder:fix/lsp-custom-server-transports

Conversation

@bajrangCoder
Copy link
Copy Markdown
Member

Changes

  • add transport selection when creating a custom LSP server
  • fix stdio transport to work with auto-discovered bridge ports
  • hide install/update/uninstall controls for direct websocket servers
  • add a remove action for custom LSP servers from the server detail page
  • auto-fill the check command prompt with a sensible default

Fixes: #1993

@github-actions github-actions bot added the translations Anything related to Translations Whether a Issue or PR label Mar 31, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 31, 2026

Greptile Summary

This PR improves the custom LSP server workflow in several meaningful ways: it adds a transport-type selection step (STDIO vs direct WebSocket) when registering a custom server, fixes the STDIO transport so it can work with auto-discovered bridge ports (not just a statically configured URL), hides install/update/uninstall controls for externally-managed WebSocket servers, adds a "Remove custom server" action from the detail page, and pre-fills the check-command prompt with a sensible default derived from the binary/installer config.

Key changes:

  • transport.ts: createStdioTransport now accepts a valid context.dynamicPort as an alternative to server.transport.url, then delegates to createWebSocketTransport which already knows how to use the dynamic port. The fix is correct and minimal.
  • lspConfigUtils.js: upsertCustomServer now uses hasOwnProperty checks instead of falsy guards for transport and launcher, so passing undefined as either field correctly clears it rather than falling back to the existing stored value. removeCustomServer stops the settings entry and unregisters the server from the API.
  • lspServerDetail.js: isDirectWebSocketServer correctly identifies transport.kind === \"websocket\" servers without a launcher bridge and hides install controls for them. The remove_custom_server handler calls stopManagedServer before removeCustomServer, then navigates back to the LSP settings page.
  • lspSettings.js: The transport selection flow is well-structured. The only minor note is that buildDefaultCheckCommand generates unquoted paths which may produce an invalid default if the binary path contains spaces.
  • lang/*.json: All 32 locale files receive the same English-fallback strings for the new keys, keeping the bundle consistent."

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style suggestions with no impact on runtime correctness.

The core logic changes in transport.ts, lspConfigUtils.js, and lspServerDetail.js are correct: the dynamic-port bypass is properly guarded, the hasOwnProperty-based transport/launcher merging correctly handles explicit undefined, and the remove flow stops the server before unregistering. The only issue found is an unquoted-path default in a user-editable prompt, which cannot cause data loss or runtime breakage.

src/settings/lspSettings.js — the buildDefaultCheckCommand helper generates unquoted paths; low severity since it is only a pre-filled default the user can edit.

Important Files Changed

Filename Overview
src/cm/lsp/transport.ts Fix allows stdio transport to proceed without a static URL when a dynamic port is available; delegates correctly to createWebSocketTransport which handles the dynamic port.
src/settings/lspConfigUtils.js Adds hasOwnProperty-based presence checks for transport/launcher so an explicit null/undefined value correctly clears the field instead of falling through to the existing config; adds removeCustomServer that stops the server from settings and unregisters it.
src/settings/lspServerDetail.js Adds isDirectWebSocketServer check to hide install/update/uninstall controls for externally-managed WebSocket servers; adds remove_custom_server action with confirmation dialog; getMergedConfig(initialServer) is called repeatedly for initial snapshot setup.
src/settings/lspSettings.js Adds transport selection (stdio vs WebSocket) when creating a custom server; introduces parseWebSocketUrl validator and buildDefaultCheckCommand helper where unquoted paths in generated shell strings could be malformed if paths contain spaces.
src/lang/en-us.json Adds 10 new i18n keys for transport selection UI, WebSocket URL prompts, and custom server removal; all other locale files receive English fallbacks for the same keys.

Sequence Diagram

sequenceDiagram
    participant User
    participant lspSettings as lspSettings.js
    participant lspConfigUtils as lspConfigUtils.js
    participant lspServerDetail as lspServerDetail.js
    participant transport as transport.ts
    participant lspApi as lspApi

    User->>lspSettings: Add custom server
    lspSettings->>User: Prompt: Server ID / Languages
    lspSettings->>User: Select transport (STDIO or WebSocket)

    alt WebSocket
        lspSettings->>User: Prompt: WebSocket URL (validated)
        lspSettings->>lspConfigUtils: upsertCustomServer({transport:{kind:"websocket",url}, launcher:undefined})
    else STDIO
        lspSettings->>User: Prompt: binary command, args, installer
        lspSettings->>User: Prompt: check command (pre-filled via buildDefaultCheckCommand)
        lspSettings->>lspConfigUtils: upsertCustomServer({transport:{kind:"stdio",...}, launcher:{bridge:{...}}})
    end

    lspConfigUtils->>lspApi: upsert(definition)
    lspConfigUtils->>appSettings: persist config

    User->>lspServerDetail: Open server detail
    lspServerDetail->>lspConfigUtils: isCustomServer(id)
    lspServerDetail->>lspServerDetail: isDirectWebSocketServer(merged)

    alt Direct WebSocket
        lspServerDetail-->>User: Hide install/update/uninstall controls
    else STDIO / bridged
        lspServerDetail-->>User: Show install/update/uninstall controls
        lspServerDetail->>transport: createStdioTransport(server, context)
        transport->>transport: check url OR dynamicPort>0
        transport->>transport: createWebSocketTransport(server, context)
    end

    User->>lspServerDetail: Remove custom server
    lspServerDetail->>User: Confirm dialog
    lspServerDetail->>lspServerDetail: stopManagedServer(serverId)
    lspServerDetail->>lspConfigUtils: removeCustomServer(serverId)
    lspConfigUtils->>appSettings: delete server from config
    lspConfigUtils->>lspApi: servers.unregister(key)
    lspServerDetail-->>User: Toast + navigate back to LSP settings
Loading

Comments Outside Diff (1)

  1. src/settings/lspSettings.js, line 980-988 (link)

    P2 Unquoted paths in generated shell commands

    buildDefaultCheckCommand interpolates user-supplied paths directly into shell command strings without quoting. If installer.binaryPath, installer.executable, or binaryCommand contains spaces or shell-special characters, the generated default will be syntactically broken (e.g. test -x /home/user/my tools/lsp instead of test -x '/home/user/my tools/lsp').

    This is a default value pre-filled in a user-editable prompt, so it won't silently break anything — but it creates a confusing starting point. Consider quoting the interpolated value:

Reviews (1): Last reviewed commit: "remove the lsp document highlights" | Re-trigger Greptile

@bajrangCoder bajrangCoder merged commit 228a339 into Acode-Foundation:main Mar 31, 2026
6 checks passed
@bajrangCoder bajrangCoder deleted the fix/lsp-custom-server-transports branch March 31, 2026 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

translations Anything related to Translations Whether a Issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Websocket LSP support

1 participant