fix: LSP diagnostics #7877
fix: LSP diagnostics #7877
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes LSP diagnostics by ensuring that event subscriptions are properly maintained across WebSocket reconnections. The fix addresses an issue where LSP event handlers (for notifications, responses, etc.) were lost when the WebSocket connection was re-established.
Changes:
- Added subscription tracking and re-registration on reconnection in
ReconnectingWebSocketTransport - Added comprehensive test coverage for subscribe/unsubscribe functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/core/lsp/transport.ts | Added Subscription interface, pendingSubscriptions tracking array, and overridden subscribe/unsubscribe methods to maintain subscriptions across reconnections |
| frontend/src/core/lsp/tests/transport.test.ts | Added mock methods for subscribe/unsubscribe and comprehensive test suites covering subscription tracking, delegate registration, and reconnection scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -134,6 +146,42 @@ export class ReconnectingWebSocketTransport extends Transport { | |||
| this.connectionPromise = undefined; | |||
There was a problem hiding this comment.
Consider clearing the pendingSubscriptions array in the close method to prevent potential memory leaks. When the transport is closed, there's no need to keep the subscription handlers in memory, as they won't be used for future reconnections (since isClosed prevents reconnection). Adding this.pendingSubscriptions = []; would ensure cleanup.
| this.connectionPromise = undefined; | |
| this.connectionPromise = undefined; | |
| this.pendingSubscriptions = []; |
There was a problem hiding this comment.
This won't hit, but good to reset.
There was a problem hiding this comment.
I think this gets called once. I think it's safer having this around in case it reconnects
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.19.5-dev13 |
Pretty sure i fixed this at some point but don't see that work at all. This fixes diagnostics by fixing the reconnecting LSP logic.
Fixes #7867