[Review] feat: AI code completion & intellisense for python editor#4
[Review] feat: AI code completion & intellisense for python editor#4govambam wants to merge 15 commits intobase-for-pr-1263from
Conversation
govambam
force-pushed
the
review-pr-1263
branch
from
421e788 to
746f520
Compare
Add AI code completion and Python IntelliSense to the Builder editor by starting
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const uri = model.uri.toString(); | ||
| const languageId = model.getLanguageId(); | ||
|
|
||
| let timeoutId: ReturnType<typeof setTimeout> | null = null; |
There was a problem hiding this comment.
🟢 Low
lsp/lspModelSync.ts:38 Consider tracking whether didOpen was sent before sending didClose. If dispose() is called before the 100ms timeout fires, didClose is sent for a document that was never opened, which violates the LSP specification.
🚀 Want me to fix this? Reply ex: "fix it for me".
| // Intercept: @codingame/monaco-vscode-api/vscode/vs/... | ||
| // Transform to absolute path in node_modules | ||
| if (source.startsWith("@codingame/monaco-vscode-api/vscode/vs/")) { | ||
| // Extract the path after /vscode/vs/ |
There was a problem hiding this comment.
🟡 Medium
ui/vite.config.ts:18 If the import source contains a query string (e.g., ?worker), appending .js creates an invalid path like foo?worker.js. Consider stripping query parameters before constructing the path.
🚀 Want me to fix this? Reply ex: "fix it for me".
| port 0 (letting the OS assign a port) would require parsing pylsp's output | ||
| to discover the assigned port, adding complexity for minimal benefit. | ||
| """ | ||
| for port in range(start_port, end_port): |
There was a problem hiding this comment.
🟢 Low
writer/lsp_manager.py:48 range(start_port, end_port) excludes end_port, so port 5099 is never checked despite the error message implying it should be. Consider using end_port + 1.
| for port in range(start_port, end_port): | |
| for port in range(start_port, end_port + 1): |
🚀 Want me to fix this? Reply ex: "fix it for me".
| logger.error("LSP WebSocket error:", error); | ||
| }; | ||
|
|
||
| ws.onclose = () => { |
There was a problem hiding this comment.
🟡 Medium
lsp/lspClient.ts:206 Race condition: onclose handler accesses global state without verifying the closing socket matches the current one. If stopLSPClient then initializeLSPClient are called quickly, the old socket's onclose can corrupt the new session's state. Consider capturing ws reference and checking webSocket === ws before modifying globals.
- ws.onclose = () => {
+ ws.onclose = () => {
+ // Guard against stale handler from previous connection
+ if (webSocket !== ws) return;
+🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
I'm working on the fix to add a guard against the race condition in the onclose handler. The changes will be pushed to a new branch and automatically merged when checks pass.
There was a problem hiding this comment.
Added a guard at the beginning of the ws.onclose handler in initWebSocketAndStartClient that checks if the global webSocket matches the current ws instance. If they don't match, the handler returns early without modifying any global state, preventing race conditions where a quickly closed old socket could corrupt a new session's state.
Commit cc68ac0 pushed to #5. When checks pass, it will merge automatically into this PR.
Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>
1 participant
Recreated from writer#1263 for Macroscope review.
Original PR: writer#1263 by @bybash
Status: open (open)
Recreated using direct PR head fetch - exact current state preserved.
Original PR: writer#1263