Skip to content

refactor(lsp): simplify server management and installer flow#1929

Merged
bajrangCoder merged 2 commits intoAcode-Foundation:mainfrom
bajrangCoder:refactor-lsp-management
Mar 8, 2026
Merged

refactor(lsp): simplify server management and installer flow#1929
bajrangCoder merged 2 commits intoAcode-Foundation:mainfrom
bajrangCoder:refactor-lsp-management

Conversation

@bajrangCoder
Copy link
Copy Markdown
Member

Summary

  • Refactors Acode’s LSP system to separate registry/runtime concerns from builtin server definitions and installer behavior.
  • Simplifies the public LSP API and reduces stale compatibility surface.
  • Improves server installation/update/uninstall flows, including better handling for non-repo servers and Luau.
  • Cleans up the LSP server settings UX to focus on install state, useful actions, and feature toggles.

What Changed

  • Split builtin language server definitions out of serverRegistry into dedicated server manifest modules.
  • Added a catalog/bundle layer so builtin and external server bundles can register through a cleaner model.
  • Simplified the public acode.lsp API around server/bundle registration helpers.
  • Improved installer structure so install behavior is less dependent on ad hoc shell blobs.
  • Added structured install/update/uninstall handling for managed servers.
  • Added architecture/helper utilities for installer logic.
  • Implemented GitHub-release based Luau installation with runtime validation and compatibility handling.
  • Added uninstall support to the LSP launcher/install flow.
  • Improved custom server support and settings persistence.
  • Reworked lspServerDetail :
    • install state
    • install / update / uninstall actions
    • startup timeout
    • initialization options edit/view
    • direct built-in feature toggles

Why

  • The previous model mixed registry data, builtin manifests, installer behavior, and launcher concerns in ways that were hard to maintain.
  • Installing LSP servers outside repo packages was brittle and overly shell-driven.
  • Luau specifically needed a more reliable download/install path and runtime compatibility checks.
  • The settings UI exposed too much low-signal internal detail and lacked the right user-facing actions.

@bajrangCoder bajrangCoder marked this pull request as ready for review March 8, 2026 08:46
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 8, 2026

Greptile Summary

This is a substantial, well-motivated refactor that separates the monolithic LSP system into a layered architecture: a public lspApi facade, a serverCatalog for bundle ownership, per-language server manifest modules, and a typed installers.* DSL. The PR also adds structured install/update/uninstall flows, runtime validation for Luau, and a significantly improved server settings UI. The overall design is sound and the separation of concerns is a clear improvement.

Issues found:

  • Logic: buildInstallCommand in serverLauncher.ts (line 583) falls back to "luau-lsp" as the default extractFile name inside the github-release branch. This is Luau-specific and will silently produce a broken install script for any future github-release server that omits extractFile.
  • Logic: In lspServerDetail.js, if installServer() or uninstallServer() throws, the refreshVisibleState call on line 558 is never reached. The install status display remains stale after a failed install or uninstall attempt.
  • Style: lspSettings.js still imports and uses serverRegistry directly (lines 1 and 109). Every other consumer was migrated to lspApi in this PR, making this an inconsistent outlier.
  • Style: Inside callback() in lspServerDetail.js, $list is resolved as this?.parentElement. If settingsPage calls the callback without a bound receiver (e.g. via an arrow-function call site), this is undefined and all post-action DOM updates are silently skipped. The show() override uses the more reliable document.querySelector("#settings .main.list").
  • Style: In lspSettings.js, the "shell" install method pre-fills the update command prompt with the full install command. Since install and update scripts are often different, this increases the risk of users accidentally submitting the same destructive command for both.

Confidence Score: 3/5

  • Generally safe to merge, but two concrete logic issues should be addressed before shipping: the stale-UI-after-error bug and the hardcoded "luau-lsp" fallback in the generic github-release installer path.
  • The architecture and the vast majority of the logic are correct. The issues are scoped to the settings UI error handling and a misplaced server-specific constant in the generic install command builder. None of the issues are data-loss risks, but the stale UI issue will confuse users after a failed install, and the extractFile fallback would silently produce broken install scripts for future github-release servers.
  • Pay close attention to src/cm/lsp/serverLauncher.ts (line 583, github-release extractFile fallback) and src/settings/lspServerDetail.js (error handling in the install/uninstall callback actions).

Important Files Changed

Filename Overview
src/cm/lsp/serverLauncher.ts Major expansion — added install/update/uninstall orchestration, architecture-based asset resolution, and bundle hooks. Logic bug: hardcoded "luau-lsp" fallback for extractFile in github-release handler (line 583).
src/settings/lspServerDetail.js Significant rework of the server detail UI to show install state and allow install/update/uninstall actions. Two issues: install/uninstall errors prevent refreshVisibleState from running, and $list resolved inconsistently between show() and callback().
src/settings/lspSettings.js Added "Add Custom Server" flow with a step-by-step installer prompt. Still imports serverRegistry directly (inconsistent with refactor); shell update command UX issue where it pre-fills with the install command.
src/cm/lsp/servers/luau.ts New Luau bundle with custom install/check hooks, glibc compatibility detection, and runtime validation. Install flow is well-structured; version reporting always returns null even after successful install.
src/cm/lsp/serverCatalog.ts New catalog/bundle layer that correctly handles bundle ownership, deregistration of removed servers on re-registration, and lazy builtin initialization. Logic is clean.
src/cm/lsp/api.ts New clean public LSP API facade exposing servers, bundles, register/upsert, and defineServer/defineBundle helpers. Well-structured with good type safety.
src/settings/lspConfigUtils.js New utility module cleanly centralizing server config persistence and custom server management. Correctly validates managed installers require binaryPath/executable. Logic is straightforward and well-organized.
src/cm/lsp/providerUtils.ts New builder helpers (defineServer, defineBundle, installers.*) providing a clean typed DSL for declaring servers and their installers. Well-structured with sensible defaults.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["acode.js / editorManager.js"] --> B["lspApi (api.ts)"]
    B --> C["serverRegistry.ts\n(register/list/update)"]
    B --> D["serverCatalog.ts\n(bundle ownership)"]
    D -->|"bindServerRegistry"| C
    D -->|"ensureBuiltinBundlesRegistered"| E["servers/index.ts"]
    E --> F["javascript.ts\nJavaScript / TS / ESLint"]
    E --> G["python.ts\nPython pylsp"]
    E --> H["luau.ts\nLuau (custom hooks)"]
    E --> I["web.ts\nHTML / CSS / JSON"]
    E --> J["systems.ts\nC/C++, Go, Rust"]

    K["serverLauncher.ts"] -->|"checkServerInstallation\ninstallServer\nuninstallServer"| L["installRuntime.ts\nshell execution"]
    K -->|"getServerBundle"| D
    K -->|"buildShellArchCase"| M["installerUtils.ts\narch normalization"]

    N["lspSettings.js"] -->|"add custom server"| O["lspConfigUtils.js\npersist settings"]
    N -->|"list servers"| C
    O --> B

    P["lspServerDetail.js"] -->|"install / update / uninstall"| K
    P -->|"persist config"| O
    P -->|"get server"| B
Loading

Comments Outside Diff (5)

  1. src/cm/lsp/serverLauncher.ts, line 583 (link)

    Hardcoded Luau-specific fallback for extractFile

    The default value "luau-lsp" is a Luau-specific filename used as a fallback when spec.extractFile is not set. Any future github-release installer that omits extractFile will silently generate a broken install script that tries to extract a file named luau-lsp from an unrelated archive.

    The fallback should derive from the executable or binary path rather than hardcoding a server-specific string:

    Even better, consider making extractFile a required field when archiveType is not "binary", since there's no sensible generic default. The validation at registration time (in serverRegistry.ts) could enforce this.

  2. src/settings/lspServerDetail.js, line 423-454 (link)

    Install/uninstall errors leave the UI state stale

    installServer and uninstallServer both call throw error on failure. Since these await calls are inside the switch statement and there is no try/catch in callback, a thrown error will unwind the function before reaching refreshVisibleState on line 558. The install status row in the UI will remain unchanged after a failed operation, showing the pre-operation state indefinitely.

    This can be fixed by wrapping the operations in a try/catch or by using finally:

    case "install_server":
      if (!snapshot.installCommand) {
        toast("Install command not available");
        break;
      }
      try {
        await installServer(snapshot.merged, "install");
      } catch {
        // error toast is already shown by installServer
      }
      break;

    The same pattern should apply to update_server and uninstall_server.

  3. src/settings/lspSettings.js, line 1 (link)

    Inconsistent API usage — serverRegistry still imported directly

    lspSettings.js still imports from "cm/lsp/serverRegistry" and calls serverRegistry.listServers() on line 109, while the stated goal of this PR is to route all external access through the new lspApi. Every other settings file (lspServerDetail.js, lspConfigUtils.js, editorManager.js, acode.js) was migrated to use lspApi, leaving this file as an inconsistent outlier.

    Then at line 109:

    const servers = lspApi.servers.list();
  4. src/settings/lspServerDetail.js, line 396-397 (link)

    $list resolved inconsistently between show() and callback()

    In the show() override (line 391), the list element is found via a specific DOM query: document.querySelector("#settings .main.list"). However, inside callback() it is resolved as this?.parentElement. These two approaches are not guaranteed to refer to the same element.

    If settingsPage calls the callback with this unbound (which can happen in strict-mode arrow callbacks or if the call site doesn't set a receiver), this will be undefined and $list will be undefined. refreshVisibleState has an early-return guard for !$list, so all post-action DOM updates would silently be skipped after every user interaction.

    For consistency and robustness, use the same selector as show():

  5. src/settings/lspSettings.js, line 96-99 (link)

    shell update command pre-filled with install command

    In promptInstaller, the "Update Command" prompt is pre-populated with the full installCommand value:

    const updateCommand = await prompt(
      "Update Command (optional)",
      String(installCommand || ""),  // pre-filled with the install command
      "textarea",
    );

    For shell-based installers, install and update commands are often different (e.g. the install command might delete and recreate state, while update is incremental). Pre-filling with the install command increases the risk that a user accepts it without changing it, resulting in an install script being run again during an update. A safer default would be an empty string or a short comment like # same as install.

Last reviewed commit: a64c194

@bajrangCoder
Copy link
Copy Markdown
Member Author

@greptileai review it

@bajrangCoder bajrangCoder self-assigned this Mar 8, 2026
@bajrangCoder bajrangCoder merged commit cab25e4 into Acode-Foundation:main Mar 8, 2026
7 checks passed
@bajrangCoder bajrangCoder deleted the refactor-lsp-management branch March 8, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant