Conversation
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
The approach makes interactively attached packages available to the existing completion machinery, and the current CI checks pass. I found three synchronization/routing issues that should be addressed before merging; the inline comments include their triggers and suggested fixes.
Validation: reviewed head 4cc6b7b; TypeScript type checking, ESLint for all changed TypeScript files, and lintr for R/languageServer.R passed locally. Focused reproductions exercised the PR's attach handler and LanguageService startup with mocked VS Code/client dependencies, and its R synchronization handler with languageserver 0.3.20, including the actual workspace-folder notification path. These reproduced incorrect routing for a wrapped R process, a lost update during client startup, and missing completion in a workspace added after synchronization. Please add regression coverage for these cases.
| session.info = (params.info as SessionInfo | undefined) ?? { version: session.rVer, command: '', start_time: '' }; | ||
| session.sessionDir = String(params.tempdir); | ||
| session.workingDir = String(params.wd); | ||
| session.resource = terminal ? rTerminal.getTerminalResource(terminal) : undefined; |
There was a problem hiding this comment.
[P2] Resolve the workspace for sessions whose PID differs from the terminal PID
terminal is found only when Terminal.processId equals params.pid, but VS Code reports the shell/console process PID and sess reports Sys.getpid(). With a wrapped console or R started inside a shell, those can differ even when both the terminal cwd and params.wd are inside the workspace. This makes session.resource undefined, so multi-server mode sends that session's packages to unscoped instead of its workspace server; it can also replace package state for unrelated unscoped files. I reproduced this with terminal PID 41000, R PID 41001, and both paths in the same workspace. Please resolve the workspace independently of exact PID equality, using a reliable terminal/session association or the local session's reported working directory as a fallback, while preserving truly unscoped sessions.
| const sessionState = this.sessionStates.get(sessionScope)?.state; | ||
| if (sessionState) { | ||
| await this.applySessionState(client, sessionState); |
There was a problem hiding this comment.
[P2] Register the client before awaiting its initial state synchronization
The caller adds this client to clients/clientScopes only after createClient returns. If a package update or disconnect arrives while this initial sendRequest is pending, syncSessionState updates/deletes the cache but cannot deliver the change to this client. The client is then registered with the old server state, and identical later workspace refreshes are skipped by the state-key check. In a controlled reproduction, synchronizing [base], then [dplyr, base] before the first request completed left the cache at [dplyr, base] but sent only [base]. Please register the client and its scope before the asynchronous sync, or reconcile the latest state (including a cleared state) before making startup complete.
| for (workspace in self$workspaces$values()) { | ||
| workspace$startup_packages <- if (length(attached_packages)) { | ||
| # languageserver resolves package conflicts from the end of this list. | ||
| rev(attached_packages) |
There was a problem hiding this comment.
[P2] Apply the current session state to workspaces created after this request
This updates only the workspaces that already exist when r/syncSessionState runs. In single-server mode, adding another folder to an existing multi-root workspace creates a new Workspace through workspace/didChangeWorkspaceFolders, and that workspace starts with the default packages. Since the session/package state is unchanged, the TypeScript deduplication does not send another sync. I reproduced this by syncing dplyr, then invoking the real workspace-folder notification handler: the existing workspace offered mutate, while the newly added workspace did not. Please retain the active state on the server and apply it when adding a workspace, or explicitly replay it after workspace-folder additions so completion remains available across all documents handled by the single server.
2 participants
Related to #1536, #1593
Summary
Use packages attached in the active R session for editor language completion, even when those packages are not referenced or through
source()in the active script.Currently, completion mainly relies on package information that
languageservercan discover from the script or its static source dependencies. This means a package attached interactively in the R session will not receive completion suggestions when it is not mentioned in the script.This change synchronises the active R session package state, provided via
sess, with the existing language server.Behaviour after this change
Single-server mode: packages attached in the active R session are available for completion across documents handled by the single language server, even when they are not referenced in the script.
Multi-server mode: packages attached in an R session are synchronised with the language server associated with that session's workspace. Package state remains isolated between different workspace language servers.
Unscoped files: scripts opened outside an existing workspace or no workspace folder is open, use the active unscoped R session package state for completion.
When the active R session changes, its attached package state replaces the previous session state. When the session disconnects, the session-derived package state is cleared.
Existing static package discovery, including resolvable
source()dependencies, remains unchanged.