Repository navigation
feat: delegate manual attachment to external terminals - #1871
Conversation
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed commit 5a99394. The delegated attach path and lifecycle coverage look solid, but there is one remaining late-registration race, detailed inline.
Validation: TypeScript and oxlint passed; all 85 tests in the terminal integration, binding, lifecycle, and workspace viewer suites passed; all 9 httpgd client tests passed. Two additional regression probes show that registering an integration does not release an already pending PID wait (native discovery and manual activation). With this PR's sess installed in a temporary library, the R workspace communication test passed; the standard plot viewer test timed out with both the PR and base session implementations, so I have not attributed that failure to this change.
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed the updates through b846038. The earlier P2 finding is fixed: registering an integration now releases an existing native discovery or manual-attach PID wait without needing processId to resolve. The new regression tests leave the PID unresolved, and both independent reproductions from my previous review now pass.
I also checked terminal/lifecycle invalidation, stale PID results, late rejection handling, discovery publication guards, and replacement handshakes. I found no new actionable issues in these changes.
Validation on macOS with VS Code 1.141.0: TypeScript compilation and oxlint passed; 123 extension-host tests passed across the session, terminal integration, binding, lifecycle, and workspace viewer suites; all 18 PID-wait/httpgd Node tests passed. Five additional focused probes passed, covering the original two failures, repeated handshakes on the same socket, closing a terminal during native discovery, and focus cancellation during a manual PID wait. The standard plot viewer test was excluded because its timeout previously reproduced with the base session implementation; the R workspace communication test passed using the bundled sess in a temporary library.
|
@eitsupi Thanks so much for creating those APIs to support downstream applications, especially for my R console. I have updated and tested using the new APIs, they work well with the latest |
#1868 and #1870 provide managed sess preparation, connection references, and explicit terminal binding, but manual attachment still has a gap. When an external terminal has no live session binding, the status-bar attach action can send the native command to vscode-R's global endpoint. In vscode-R-console, that bypasses SessProxy and replaces the proxy-backed transport. PID discovery can also wait indefinitely on an extension-owned pseudoterminal before its frontend gets a chance to recover.
This adds
session.registerTerminalIntegration(terminal, { attach(token) })so the frontend can restore its own transport and bind the accepted connection using the existing APIs:Consumers can prepare through their proxy, observe the matching connection, and call
activate(sessionId, { terminal, connectionId, select: false })before returningtrue. vscode-R then verifies the binding and selects/focuses the terminal. Consumers retain responsibility for prompt readiness, submission queues, failure explanations, and R code already submitted when cancellation occurs.This completes the third part of the external session API for the intended 3.2.0 release alongside #1868 and #1870, so consumers can adopt managed sess with a supported manual recovery path. This change is on the TypeScript side and does not change the bundled sess revision or introduce another reinstall/restart transition. External frontends must adopt the hook; pasting a copied native attach command remains outside this API and can still bypass a proxy.