Feat/lsp improvements - #2632
Conversation
Greptile SummaryThe PR expands the built-in CodeMirror LSP client with URI translation, server-driven workspace edits, definition/reference actions, richer capabilities, delayed client disposal, tooltip navigation, and editable custom-server settings. The workspace-edit and SFTP paths contain correctness defects, and the new code currently introduces a TypeScript build error.
Confidence Score: 4/5The PR should not merge until the TypeScript error and the workspace-edit and SFTP correctness failures are fixed. The changed code references a missing type, can silently drop or falsely acknowledge portions of server-requested workspace edits, and removes authentication parameters while resolving child files in SFTP workspaces. Files Needing Attention: src/cm/lsp/codeActions.ts, src/cm/lsp/transport.ts, src/components/referencesPanel/utils.js, src/cm/lsp/clientManager.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Server as Language Server
participant Transport as transport.ts
participant Workspace as AcodeWorkspace
participant Editor as Target EditorView
Server->>Transport: workspace/applyEdit
Transport->>Transport: normalize changes by URI
loop Each target URI
Transport->>Workspace: getFile / displayFile
Workspace-->>Transport: EditorView
Transport->>Editor: applyTextEdits
end
Transport-->>Server: ApplyWorkspaceEditResponse
Reviews (1): Last reviewed commit: "fix: Removing testing scripts not belong..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function comparePositions(a: LspPosition, b: LspPosition): number { | ||
| if (a.line !== b.line) return a.line - b.line; | ||
| return a.character - b.character; | ||
| } |
There was a problem hiding this comment.
TypeScript checks this exported LSP module with no declaration or import for LspPosition, causing Cannot find name 'LspPosition' and preventing the project from building.
| function comparePositions(a: LspPosition, b: LspPosition): number { | |
| if (a.line !== b.line) return a.line - b.line; | |
| return a.character - b.character; | |
| } | |
| function comparePositions(a: Position, b: Position): number { | |
| if (a.line !== b.line) return a.line - b.line; | |
| return a.character - b.character; | |
| } |
Knowledge Base Used: LSP Integration
| Object.fromEntries( | ||
| (edit.documentChanges ?? []) | ||
| .filter((c): c is { textDocument: { uri: string }; edits: TextEdit[] } => "edits" in c) | ||
| .map((c) => [c.textDocument.uri, c.edits]), |
There was a problem hiding this comment.
When documentChanges contains multiple ordered entries for the same URI, Object.fromEntries retains only the final entry, causing earlier portions of a rename or refactor to be silently omitted while the request is reported as applied.
Knowledge Base Used: LSP Integration
| if (failures.length) { | ||
| return { | ||
| applied: true, | ||
| failureReason: `Applied to ${appliedCount} file(s); failed: ${failures.join(", ")}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
When at least one target file is edited but another cannot be opened or updated, this branch returns applied: true, causing the server to proceed as though the entire rename or refactor completed while source files remain inconsistent.
| if (failures.length) { | |
| return { | |
| applied: true, | |
| failureReason: `Applied to ${appliedCount} file(s); failed: ${failures.join(", ")}`, | |
| }; | |
| } | |
| if (failures.length) { | |
| return { | |
| applied: false, | |
| failureReason: `Applied to ${appliedCount} file(s); failed: ${failures.join(", ")}`, | |
| }; | |
| } |
Knowledge Base Used: LSP Integration
| const base = rootUrl.slice( | ||
| 0, | ||
| rootUrl.indexOf(rootPath) + rootPath.length, | ||
| ); | ||
| return base + suffix; |
There was a problem hiding this comment.
When a definition or reference targets a child of an SFTP workspace configured with keyFile or passPhrase query parameters, slicing the root URL here drops those parameters, causing the reconstructed file URI to lose the credentials needed to open the remote target.
Knowledge Base Used: LSP Integration
| console.log( | ||
| "Current doc\n", | ||
| view.state.doc.toString(), | ||
| ); | ||
|
|
||
| console.log( | ||
| "Synced doc\n", | ||
| plugin.syncedDoc.toString(), | ||
| ); |
There was a problem hiding this comment.
Every LSP formatting request now writes both the current and synchronized document text to the console, exposing source code or credentials in collected WebView logs and adding avoidable overhead for large files.
| console.log( | |
| "Current doc\n", | |
| view.state.doc.toString(), | |
| ); | |
| console.log( | |
| "Synced doc\n", | |
| plugin.syncedDoc.toString(), | |
| ); |
Knowledge Base Used: LSP Integration
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Add workspace/didChangeConfiguration notification after the LSP initialization handshake but was missing from theCodeMirror-based LSP client. Send the notification through Acode's existing LSP transport afterclient.initializing resolves, ensuring the server has completed theinitialization handshake before receiving the configuration. This restores the previous ALC behavior and ensures external languageservers receive their initial workspace configuration immediately afterconnecting.
Disclaimer, these fixes focus more on the custom server configation rather than the built LSP, so that people can use it through termux or even port forward from a server running an lsp ( As I used to do on my phone with Acode-lanuage-client before the CM6 migration) There was a mismatch with the LSP internal line count and the editor lines count, it may have been caused by how codemirror itself impleent it. The fix was to reimplement a safeOffsetLsp method to clamp to a line count both the lsp and editor can agree on. This allowed pylsp through autopep8 to do the formatting successfully. But then there was another issue, codeAction also needed that workedit, and the latter were in the transport.ts which is imported by clientManager. To avoid the cycling situation, applywork edit, and the newly added safeoffset were extarcted into a new textUtils.ts file so both can import it on their own. It's my first actual commit, so I apologize for the format... :(
DISCLAIMER: this work is mostly to add compatibility for custom lsp used via termux or any other terminal, through acode-ls, which was used with Acode-language-client added a menu for go-to functions and remapped the code-action button to that menu. codeactions are now included in that submenu. a lot of items have no code actions,considering then to make the "show code action" buttton grayed ( in other words if list of action = 0). go-to works following acode current file system design. next and final commit, fixing uri translation so lsp can work with termux, ftp and sftp ( like I used to work with Acode Language client). But under one condition, indexing lsp must be running on the same device as where the workdir example, you use sftp, portforward. I don't think it is worth having 1 cross device lsp, that's mind bending and even complicated to use anyway.
I have been using Acode for a while now and it helped me go through a full semester of college without a laptop when I just had a phone and a homeserver.
We are using a patched version of Acode-ls by Thraize to as a websocket pipe.
That way people can use it on termux or even portforward it via ssh from their server where the lsp runs.
- Implemented an LSP menu:
before the toolbox menu was going directly to code action.
with the redesign the buttins opens a dialog allowing to use go-to like functions,
or then press 'show code actions' to then access the code action menu,
which btw auto execute if there is only one action.
- session and hanging lsp handling:
if all the files of a project are closed, there is a 45 sec timer after what the lsp closes.
It is made so to leave some time for the user if they are just swithing files while also avoiding keeping an lsp active
for an uncessary amount of time.
- uri translation fix:
this one is my favorite, previously it was a heachache to to use lsp with anything that isn’t in local shared storage.
it is now possible to use the lsp even if the workdir is in termux, no setup required, it has been implemented at the client level, fully exploiting Acode internal file system features.
SFTP support has also been added. Now lsp should work if a the work dir is an sftp repo.
note that it has not been implemented for ftp, it should be trivial now but sticking to sftp might be better for serious work anyway.
The consequences of that translation fix is also that it allows go-to actions to be if the work dir is in termux or sftp along local shared storage.
Sdcard may or may not be supported, i haven’t taken that in account honestly.
- codeActions & formatting support:
this was one of the first fixes, code action now works accross the different workspaces type mentionned ("local","termux" and "sftp")
however code action in currently active and opened has been prioritize. Need to investigate if we can do code-actions that work accros multiple files.
In theory it should be a feature as well, I may have a look at it later, this the reason why I haven’t exposed "rename symbol" in the lsp menu.
I think that's about it. I really hope that you will consider these fixes as it now allows the usage of eithet a local lsp on termux or a remote one, as long as the lsp and the project are on the same device with no reachabilitied issue.
Hopefully you will consider my contribution to this amazing mobile editor that Acode is.
I never wrote a commit message this long, so I apologizes to whoever finds any incoveniences to read it.
Thanks the Thraize as well, if he had never done Acode-Language-Client and the acode-ls companion, all of this would have never been possible.
It contributed a lot at teaching me how lsp works and how to steer claude for decisives patches
…ent FTP although SFTP works already
cd5e211 to
5a10e5a
Compare
|
@deadlyjack Separately from this PR, I wanted to share an idea I have been experimenting with for SFTP + LSP support. The way I got SFTP projects working reliably was by running the LSP on the same machine as the files, then forwarding the WebSocket connection back to Acode through SSH. This avoids a lot of URI/path translation issues because the language server sees the real filesystem it is indexing. The "acode-ls" script I shared earlier was not actually Termux-specific. It only had a Termux shebang because I used Termux as my testing environment when the built-in terminal was unavailable.
The same concept works with Acode's own WebSocket LSP setup. The idea would be to make this more seamless:
Then when the SFTP workspace is closed, the session can be stopped. The goal is not to replace local LSPs, but to make heavy servers usable remotely. For example, jdtls is often too heavy for a phone, but running it on the same machine as the project worked perfectly for me: indexing, completion, code actions, imports, generation of getters/setters, rename, etc. The general idea is: the LSP should run where the files are. Local projects use local servers, SFTP projects use remote servers through SSH forwarding. I'm not sure if this fits Acode's roadmap, but I thought I would share the approach since SFTP support already exists and this could make large projects much more practical on mobile devices. Happy to discuss it or drop it if it is out of scope. |
note: Thanks to whoever fixed the access to the built in terminal, it is now usable! i# Please enter the commit message for your changes. Lines starting
1 participant
Summary
Improves Acode's built-in CM6 LSP client with fixes for URI handling, workspace edits, client capabilities, session management, and custom server configuration. All changes are client-side; no external tooling or server scripts are included.
Changes
Position & text edit handling
URI resolution
Client capabilities
Workspace edits
LSP actions
Session lifecycle
Custom server configuration
Tooltip file links
Testing