feat: AI code completion & intellisense for python editor #1263
Conversation
📝 WalkthroughWalkthroughAdds Python LSP support and an AI completion service: backend LSP manager and Writer-based completion handler, new API endpoints, extensive frontend Monaco/VSCode integration (WebSocket LSP client, diagnostics, model sync, completion providers), build/plugin updates, and UI toggles for diagnostics and AI completion. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (Monaco)
participant Frontend as Frontend LSP Setup
participant LSPClient as Monaco LSP Client
participant Backend as Writer App
participant PyLSP as pylsp Server
participant WriterAPI as Writer Palmyra
Browser->>Frontend: open editor (edit mode)
Frontend->>Backend: GET /api/lsp-config
Backend-->>Frontend: { host, port }
Frontend->>LSPClient: initialize WebSocket -> ws://host:port
LSPClient->>PyLSP: initialize (LSP handshake)
PyLSP-->>LSPClient: ready
Browser->>LSPClient: textDocument/didOpen (model)
Browser->>LSPClient: textDocument/didChange (edits)
PyLSP-->>LSPClient: publishDiagnostics
LSPClient->>Browser: apply markers
Browser->>Frontend: completion request
alt AI completion enabled
Frontend->>Backend: POST /api/code-completion
Backend->>WriterAPI: chat/completion call
WriterAPI-->>Backend: completion text
Backend-->>Frontend: AI completion
Frontend->>Browser: show completion
else
Browser->>LSPClient: textDocument/completion
LSPClient-->>Browser: LSP completions (converted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (1)
99-110: Potential null access if editor is not initialized.The
watch(disabled, ...)callback callseditor.updateOptions()without checking ifeditoris null. Ifdisabledprop changes beforeonMountedcompletes, this could throw.🐛 Suggested fix
watch(disabled, (isNewDisabled) => { + if (!editor) return; if (isNewDisabled) { editor.updateOptions({ readOnly: true }); return; } editor.updateOptions({ readOnly: false }); });
🤖 Fix all issues with AI agents
In @package.json:
- Around line 9-12: package.json specifies Node engine "node": "22.x.x" but
ci-ui-test.yml currently sets node-version to 20.x; update the CI workflow to
use node-version: "22.x" so the CI environment matches the package.json engine
constraint (change the node-version entry in ci-ui-test.yml from 20.x to 22.x).
In @src/ui/src/builder/builderEditorWorker.ts:
- Around line 16-33: The worker loaders (workerLoaders) create ES module workers
via new Worker(new URL(..., import.meta.url), { type: "module" }) for
TextEditorWorker and TextMateWorker, so update your Vite configuration export
(defineConfig) to set worker.format = 'es' (e.g., export default defineConfig({
worker: { format: 'es' }, /* rest of config */ })); this ensures Vite emits
workers as ES modules and resolves the runtime incompatibility.
In @src/ui/src/builder/lspModelSync.ts:
- Around line 39-53: The setTimeout that sends textDocument/didOpen can fire
after dispose() and cause didOpen to be sent after didClose; store the timeout
id when calling setTimeout (e.g., let openTimeout = setTimeout(...)) and update
dispose() to clearTimeout(openTimeout) (and null it) before performing any
didClose/sendNotification actions; ensure the code checks that the timeout
hasn’t already executed (e.g., clear the reference when the callback runs) and
keep using lspClient.sendNotification with the same uri, model.getVersionId(),
model.getValue(), and logger.error handling.
In @src/ui/src/composables/useMonacopilot.ts:
- Around line 16-30: The useMonacopilot function accepts a language parameter
but ignores it by hardcoding "python" when calling registerCompletion; update
the call to registerCompletion inside useMonacopilot to pass the language
parameter (use the language variable) instead of the literal "python", or if
language truly isn't needed remove it from useMonacopilot's signature and all
callers; ensure you update any tests or callers of useMonacopilot if you change
the signature.
In @src/writer/ai/code_completion.py:
- Around line 137-148: The docstring currently references "Claude" but this
handler uses Writer Palmyra: update the top summary and any mentions in the
docstring to say "Writer Palmyra" (e.g., change "Build the prompt for Claude to
generate code completion." to "Build the prompt for Writer Palmyra to generate
code completion.") in the prompt-building function (e.g.,
build_prompt_for_code_completion) and keep the existing Args/Returns text
unchanged.
- Around line 106-116: The async method _get_writer_completion is using the
synchronous Writer client (self.client) and calling self.client.chat.chat()
synchronously, which blocks the event loop; replace the sync client with the
AsyncWriter class when initializing (so self.client is an AsyncWriter instance)
and change the call in _get_writer_completion to await
self.client.chat.chat(...) (preserving model, messages, max_tokens, temperature)
so the chat request is awaited non-blockingly; ensure any imports and client
setup (where self.client is created) are updated to use AsyncWriter and any
async context or transport required by the SDK is configured.
In @src/writer/lsp_manager.py:
- Around line 40-47: The current loop that tests ports by calling
s.bind((self.host, port)) (iterating start_port..end_port) is vulnerable to a
TOCTOU race; to fix it either let the OS pick an ephemeral port by binding to
port 0 and then read the assigned port via s.getsockname()[1] (replace the loop
and s.bind usage with bind((self.host, 0)) and return the assigned port), or if
you must restrict to a range, keep the existing loop but document the race
condition and its low probability in the method docstring and surrounding
comments so callers are aware; update references to self.host, start_port,
end_port and the socket bind logic accordingly.
🧹 Nitpick comments (13)
src/writer/ai/code_completion.py (1)
52-60: Uselogging.exceptionfor better stack traces.As flagged by static analysis,
logging.exceptionautomatically includes the stack trace, which aids debugging.♻️ Suggested improvement
except ImportError: - logger.error( + logger.exception( "writer-sdk not properly installed. " "Install it with: pip install writer-sdk" ) self.enabled = False except Exception as e: - logger.error(f"Failed to initialize Writer client: {e}") + logger.exception("Failed to initialize Writer client") self.enabled = Falsepyproject.toml (1)
58-58: Consider making python-lsp-server an optional dependency group.
python-lsp-server[all]is currently in main dependencies but only used in edit mode. The[all]extras pull in black, autopep8, yapf, rope, flake8, and related plugins, significantly increasing the package footprint for all users regardless of usage mode. Since the LSP manager is conditionally initialized and only started whenserve_mode == "edit", this is a good candidate for an optional dependency group.Move to an optional group following the existing project pattern (similar to
redis):♻️ Suggested change
[tool.poetry.dependencies] # ... existing deps ... -python-lsp-server = {extras = ["all"], version = "^1.12.0"} +[tool.poetry.group.lsp] +optional = true + +[tool.poetry.group.lsp.dependencies] +python-lsp-server = {extras = ["all"], version = "^1.12.0"} [tool.poetry.extras] redis = ["redis"] +lsp = ["python-lsp-server"]src/ui/src/builder/lspSetup.ts (1)
70-72: Consider removing the arbitrary 100ms delay.The
setTimeout(resolve, 100)is a fragile timing assumption. If DOM readiness is actually required, consider using a proper readiness check or document state listener. If this delay is only to allow prior async operations to settle, it may be unnecessary sinceinitializeVSCodeServicesalready awaits completion.♻️ Suggested improvement
If a delay is truly needed, document why, or remove it if it was added speculatively:
- // Step 2: Wait for DOM to be fully ready - await new Promise((resolve) => setTimeout(resolve, 100)); - - // Step 3: Initialize LSP client (now waits for full initialization) + // Step 2: Initialize LSP client (now waits for full initialization)src/ui/src/main.ts (1)
69-82: Consider handlingsetupLSP()promise rejection.The
setupLSP()call is fire-and-forget. While the function returns a boolean and logs errors internally, any unhandled promise rejection could surface in console. Consider adding.catch()for explicit error suppression or logging.♻️ Suggested improvement
- setupLSP(); + setupLSP().catch((error) => { + logger.error("LSP setup failed:", error); + });src/ui/src/builder/lspCompletionProvider.ts (1)
217-224: Refactor to avoid relying onwindow.monaco.The function accesses
window.monacowhich may not be available, and returning0as a fallback doesn't guarantee it maps toCompletionItemKind.Text. SinceregisterLSPCompletionProvideralready receivesmonacoInstance, consider passing it to this function or moving the mapping inline.♻️ Suggested refactor
Pass
monacoInstanceas a parameter:function convertCompletionItemKind( lspKind: number | undefined, + monacoInstance: typeof monaco, ): monaco.languages.CompletionItemKind { - // Import monaco dynamically to avoid circular dependencies - const monacoInstance = (window as { monaco?: typeof monaco }).monaco; - if (!monacoInstance) { - return 0; // Text - } - const CompletionItemKind = monacoInstance.languages.CompletionItemKind; // LSP CompletionItemKind mappingThen update the call site at line 167:
- kind: convertCompletionItemKind(completionItem.kind), + kind: convertCompletionItemKind(completionItem.kind, monacoInstance),src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (1)
136-143: URI file extension should match the language.The model URI always uses
.pyextension (${Date.now()}.py) regardless of the actual language. While the URI is only passed tocreateModelfor Python (line 142), the hardcoded.pyextension could cause confusion if this logic changes. Consider making the extension dynamic.♻️ Suggested improvement
- const modelUri = monaco.Uri.parse(`inmemory://model/${Date.now()}.py`); + const extension = props.language === "python" ? "py" : props.language || "txt"; + const modelUri = monaco.Uri.parse(`inmemory://model/${Date.now()}.${extension}`);src/writer/lsp_manager.py (2)
106-110: Uselogging.exceptionfor better stack traces.When catching exceptions,
logging.exceptionautomatically includes the traceback, which aids debugging. This aligns with the static analysis hint.♻️ Suggested fix
except Exception as e: - logger.error(f"Failed to start LSP server: {e}") + logger.exception("Failed to start LSP server") self.process = None self.port = None return Falseexcept Exception as e: - logger.error(f"Error stopping LSP server: {e}") + logger.exception("Error stopping LSP server")Also applies to: 127-128
143-162: Consider documenting auto-restart behavior.The
get_config()method silently auto-restarts the server if it's not running. This could surprise callers. Consider adding a log message or making auto-restart opt-in.src/writer/serve.py (2)
371-388: Uselogging.exceptionfor better error diagnostics.Per static analysis hints,
logging.exceptionincludes the traceback which aids debugging. Also, catching broadExceptionis acceptable here since we want to ensure the endpoint always returns a response.♻️ Suggested fix
except Exception as e: - logging.error(f"Error in code completion endpoint: {e}") + logging.exception("Error in code completion endpoint") # Return empty completion on error return JSONResponse(content={"completion": ""})
231-232: Remove empty lines if unintentional.These appear to be formatting artifacts. If intentional for readability, ignore this comment.
src/ui/src/builder/lspClient.ts (3)
19-23:hostproperty is declared but unused.The
hostfield inLSPConfigis never used—constructWebSocketURLrelies solely onwindow.locationandconfig.port. Consider removing the unused field or utilizing it in URL construction if the backend can specify a different host.
221-222: InconsistentMAX_RETRIESwith shared constant.A shared constant
MAX_RETRIES = 5exists insrc/ui/src/constants/retry.ts, but a localMAX_RETRIES = 3is defined here. For consistency across the codebase, consider importing and using the shared constant, or deliberately choosing different values with a comment explaining why.♻️ Suggested refactor to use shared constant
+import { MAX_RETRIES } from "../constants/retry.js"; + export async function initializeLSPClient(retryCount = 0): Promise<boolean> { - const MAX_RETRIES = 3; const RETRY_DELAY = 1000; // 1 secondIf a different value is intentional, add a comment:
- const MAX_RETRIES = 3; + // Fewer retries than default since LSP is non-critical for basic editing + const LSP_MAX_RETRIES = 3; const RETRY_DELAY = 1000; // 1 second
285-291: Consider awaitingstop()or making the function async.
MonacoLanguageClient.stop()returns aPromise. Currently, the promise is not awaited, so cleanup (sending shutdown/exit LSP notifications) may not complete before references are nulled. This could cause warnings or incomplete cleanup.If sync signature is required for lifecycle hooks, consider fire-and-forget with error handling.
♻️ Option 1: Make async
-export function stopLSPClient(): void { - languageClient?.stop(); +export async function stopLSPClient(): Promise<void> { + await languageClient?.stop(); languageClient = null; webSocket?.close(); webSocket = null; }♻️ Option 2: Fire-and-forget with error handling
export function stopLSPClient(): void { - languageClient?.stop(); + languageClient?.stop().catch((error) => { + logger.warn("Error stopping LSP client:", error); + }); languageClient = null; webSocket?.close(); webSocket = null; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpoetry.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
package.jsonpyproject.tomlsrc/ui/package.jsonsrc/ui/src/builder/BuilderEmbeddedCodeEditor.vuesrc/ui/src/builder/builderEditorWorker.tssrc/ui/src/builder/lspClient.tssrc/ui/src/builder/lspCompletionProvider.tssrc/ui/src/builder/lspDiagnostics.tssrc/ui/src/builder/lspModelSync.tssrc/ui/src/builder/lspSetup.tssrc/ui/src/composables/useMonacopilot.tssrc/ui/src/main.tssrc/writer/ai/code_completion.pysrc/writer/lsp_manager.pysrc/writer/serve.py
🧰 Additional context used
🧬 Code graph analysis (8)
src/ui/src/builder/lspDiagnostics.ts (2)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (1)
getLSPClient(279-279)
src/ui/src/builder/lspClient.ts (2)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/constants/retry.ts (1)
MAX_RETRIES(2-2)
src/ui/src/builder/lspSetup.ts (4)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (2)
initializeLSPClient(220-261)stopLSPClient(285-291)src/ui/src/builder/lspCompletionProvider.ts (1)
registerLSPCompletionProvider(114-212)src/ui/src/builder/lspDiagnostics.ts (1)
setupLSPDiagnostics(58-109)
src/ui/src/builder/lspCompletionProvider.ts (2)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (1)
getLSPClient(279-279)
src/ui/src/builder/lspModelSync.ts (2)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (1)
getLSPClient(279-279)
src/writer/ai/code_completion.py (1)
src/writer/ai/__init__.py (1)
split(2917-2934)
src/ui/src/main.ts (1)
src/ui/src/builder/lspSetup.ts (2)
setupLSP(62-91)cleanupLSP(96-108)
src/writer/serve.py (2)
src/writer/ai/code_completion.py (2)
get_completion_handler(205-215)get_completion(62-86)src/writer/lsp_manager.py (4)
LSPManager(18-162)start(53-110)stop(112-131)get_config(143-162)
🪛 GitHub Actions: ci
src/ui/src/composables/useMonacopilot.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/lspDiagnostics.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/lspClient.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/lspSetup.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/lspCompletionProvider.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/lspModelSync.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/builderEditorWorker.ts
[error] 1-1: RollupError: [vite:worker-import-meta-url] Invalid value "iife" for option "output.format" - UMD and IIFE output formats are not supported for code-splitting builds.
package.json
[error] 1-1: npm run build failed: 'vite build' exited with code 1 during writer-ui build. The operation was canceled.
src/ui/package.json
[error] 1-1: npm run build failed: 'vite build' exited with code 1 during writer-ui build. The operation was canceled.
src/ui/src/main.ts
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
src/ui/src/builder/BuilderEmbeddedCodeEditor.vue
[error] 1-1: Build failed: npm run ui:build (vite build) failed with RollupError in writer-framework UI code path.
🪛 GitHub Actions: ci-e2e
src/ui/src/builder/builderEditorWorker.ts
[error] 1-1: RollupError during vite build: Invalid value 'iife' for option 'output.format'. UMD/IIFE formats are not supported for code-splitting builds. This occurred while executing 'npm run ui:build' (vite build).
🪛 Ruff (0.14.10)
src/writer/ai/code_completion.py
53-56: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
58-58: Do not catch blind exception: Exception
(BLE001)
59-59: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
126-126: Do not catch blind exception: Exception
(BLE001)
127-127: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer/lsp_manager.py
44-44: Consider moving this statement to an else block
(TRY300)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
83-83: subprocess call: check for execution of untrusted input
(S603)
84-84: Starting a process with a partial executable path
(S607)
104-104: Consider moving this statement to an else block
(TRY300)
106-106: Do not catch blind exception: Exception
(BLE001)
107-107: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
127-127: Do not catch blind exception: Exception
(BLE001)
128-128: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
src/writer/serve.py
385-385: Do not catch blind exception: Exception
(BLE001)
386-386: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (31)
src/ui/package.json (2)
21-24: Ensure @CodinGame package versions are aligned.All
@codingame/monaco-vscode-*packages should use the same version for compatibility. This looks correct at^24.2.0, but verify these work with the remappedmonaco-editorin the rootpackage.json.
49-51: The build failure is a Vite configuration issue, not a version compatibility problem.The
monaco-editorat^0.47.0and@codingame/monaco-vscode-*at^24.2.0are intentionally published at different versions and work together through the rootpackage.jsonremap. This is the intended design.The pipeline error (
Invalid value 'iife' for option 'output.format') is caused by Vite/Rollup rejecting IIFE format when code-splitting is enabled. This is a known constraint and requires adjusting the build configuration—either by (A) removing IIFE from the output formats, (B) preventing code-splitting withinlineDynamicImports, or (C) configuring workers to use ES format instead.Likely an incorrect or invalid review comment.
package.json (1)
45-48: NPM aliasing for Monaco/VSCode API compatibility looks correct.This is the standard approach for
monaco-languageclientintegration, remappingmonaco-editorandvscodeto the@codingameequivalents. This ensures consistent package resolution across the workspace.src/ui/src/builder/builderEditorWorker.ts (1)
8-10: Python language registration guard is correct.This prevents duplicate registration errors when the module is re-imported.
src/ui/src/builder/lspModelSync.ts (1)
30-33: Early return with no-op dispose is correct.Gracefully handles the case when LSP client isn't available.
src/ui/src/composables/useMonacopilot.ts (1)
33-37: Cleanup function correctly handles deregistration.The null check before calling
deregisteris appropriate defensive coding.src/writer/ai/code_completion.py (1)
23-24: Unusedlanguageparameter inuseMonacopilot.The
languageparameter is accepted but line 24 hardcodes"python"instead of using the parameter.🐛 Fix to use the parameter
const disposable = registerCompletion(monacoInstance, editor, { - language: "python", + language: language, // API endpoint for code completions endpoint: "/api/code-completion",Likely an incorrect or invalid review comment.
src/ui/src/builder/lspSetup.ts (3)
26-54: LGTM! Service initialization with idempotency guard.The
vscodeServicesReadyflag correctly prevents redundant initialization. Dynamic imports for service overrides help with code-splitting. Error handling logs failures and returns a boolean status.
62-91: LGTM! Clean orchestration of LSP setup steps.The sequential initialization flow (services → client → providers) with early return on failure is well-structured. Resource registration for completion and diagnostics properly stores disposables for cleanup.
96-108: LGTM! Proper cleanup implementation.The cleanup function correctly disposes both disposables (completion provider and diagnostics) before stopping the LSP client. Null assignments after disposal prevent double-dispose issues.
src/ui/src/main.ts (3)
12-12: LGTM! Import changes are correct.The type-only import for
CollaborationManageris appropriate since it's only used for type annotation. The early Monaco worker import ensures workers are registered before editor instantiation.Also applies to: 17-18
35-40: LGTM! Strict equality improvements.Changing from loose (
==) to strict (===) equality for mode checks is a good practice that prevents type coercion bugs.
113-114: LGTM! Clear documentation of cleanup consolidation.The comment clarifies that
beforeunloadcleanup was intentionally moved to theload()function, preventing confusion about the removed code.src/ui/src/builder/lspCompletionProvider.ts (2)
34-106: LGTM! Well-structured Python snippets.The snippet definitions use proper tab stop syntax (
${1:placeholder}), include helpful documentation, and cover common Python constructs. The eslint disable/enable comments for template curly strings are appropriate.
114-212: LGTM! Completion provider implementation.The provider correctly:
- Returns empty suggestions when LSP client is unavailable
- Converts line/column between Monaco (1-based) and LSP (0-based) coordinate systems
- Handles both array and object response formats from LSP
- Properly sets
insertTextRulesfor snippet formatting- Combines LSP suggestions with custom snippets
- Logs and gracefully handles errors
src/ui/src/builder/lspDiagnostics.ts (3)
58-109: LGTM! Robust diagnostics bridge implementation.The implementation correctly:
- Returns a no-op disposable when LSP client is unavailable (graceful degradation)
- Handles closed models without error (line 75-78)
- Converts LSP 0-based positions to Monaco 1-based positions
- Uses consistent owner ID ("pylsp") for marker management
- Wraps processing in try-catch with error logging
34-50: LGTM! Severity mapping is correct.The mapping from LSP DiagnosticSeverity to Monaco MarkerSeverity follows the LSP specification. Defaulting to Error for unknown values is a safe choice.
117-120: LGTM! Clean utility function.The
clearModelDiagnosticsfunction provides a simple API for clearing markers on a model.src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (3)
112-134: LGTM! Language change handling with proper cleanup.The watch correctly:
- Disposes the old LSP sync before changing language
- Only re-syncs when the new language is Python
- Handles errors with logging
164-175: LGTM! Conditional LSP and AI completion setup.The code correctly initializes LSP sync and monacopilot only for Python language and excludes single-line variant (which wouldn't benefit from these features). Error handling with logging is appropriate.
191-215: LGTM! Comprehensive cleanup on unmount.The cleanup sequence properly disposes:
- LSP sync disposable
- Model diagnostics (before model disposal)
- Editor instance
- Model instance
- Monacopilot cleanup function
- Resize observer
The order ensures resources are released correctly.
src/writer/lsp_manager.py (2)
53-104: LGTM! Robust server lifecycle management.The
start()method:
- Checks for already-running process (idempotent)
- Cleans up dead processes before restart
- Verifies pylsp availability with helpful error message
- Checks if process terminated immediately after spawn
- Properly resets state on failure
112-131: LGTM! Graceful shutdown with force-kill fallback.The
stop()method correctly implements graceful termination with a 5-second timeout, falling back tokill()if needed. State is properly reset in thefinallyblock.src/writer/serve.py (4)
139-144: LGTM! LSP lifecycle integration with lifespan.The LSP server is correctly started during edit-mode startup and stopped during shutdown. The defensive
hasattrchecks andis not Noneguards ensure robustness.Also applies to: 164-168
181-186: LGTM! Conditional LSP manager initialization.The LSP manager is only instantiated in edit mode, avoiding unnecessary resource allocation in run mode.
270-288: LGTM! LSP config endpoint with proper access control.The endpoint correctly:
- Restricts access to edit mode with 403 response
- Handles missing LSP manager gracefully
- Delegates to LSP manager for configuration
380-384: No changes needed —get_completionis correctly defined as async.The
CodeCompletionHandler.get_completion()method atsrc/writer/ai/code_completion.py:62is an async method, so theawaitusage in the request handler is correct.src/ui/src/builder/lspClient.ts (4)
37-47: LGTM!The protocol replacement logic correctly handles both HTTP and HTTPS cases by checking HTTPS first. Clearing hash and search parameters is appropriate for WebSocket URLs.
54-67: LGTM!Solid error handling with appropriate logging for both HTTP errors and exceptions.
76-101: LGTM!The language client configuration is well-structured with appropriate document selectors and a deliberate non-restarting error policy to prevent connection loops.
268-279: LGTM!The active check correctly validates all three conditions (client exists, socket exists, socket is open). The getter provides safe access to the client instance.
HackerOne Code Security Review🟢 Scan Complete: 10 Issue(s) *We want to surface issues only when necessary and actionable. If we didn't get something right, or if there's more context we should take into account, reply to the comment so we'll know for the future. Here's how the code changes were interpreted and info about the tools used for scanning. 📖 Summary of ChangesThe changes introduce comprehensive Language Server Protocol (LSP) and Monaco Editor integration for Python development. New TypeScript and Python modules enable advanced code editing features, including LSP synchronization, AI code completion, and diagnostic support. The implementation adds dependencies, creates client and worker modules, and introduces new composables and components to manage editor settings and interactions.
ℹ️ Issues DetectedNOTE: These may not require action! Below are unvalidated results from the Analysis Tools that ran during the latest scan for transparency. We investigate each of these for accuracy and relevance before surfacing them as a potential problem. How will I know if something is a problem?
🧰 Analysis tools
⏱️ Latest scan covered changes up to commit 49375ff (latest) |
|
Graham C has submitted feedback.
Reviewed with ❤️ by PullRequest |
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/ui/package.json:
- Around line 21-23: The package.json lists incompatible editor/language-client
versions; update the monaco stack so versions are compatible: either upgrade
"monaco-editor" to 0.52+ (recommended — pin to the latest stable) and keep
"monaco-languageclient" at 10.x, or downgrade "monaco-languageclient" to a
7.x/8.x release that matches "monaco-editor" 0.47.0; edit the dependency entries
for "monaco-editor" and "monaco-languageclient" (and ensure the related
@codingame monaco packages remain compatible) so the two packages are aligned,
then run install and verify the build.
In @src/ui/src/builder/lspClient.ts:
- Around line 193-206: If the WebSocket fails before onopen,
clientReadyPromise/clientReadyResolve can remain unresolved and cause
initializeLSPClient to hang; in ws.onclose detect if clientReadyResolve is still
set and invoke it (resolve or reject consistently with initializeLSPClient's
contract), then clear clientReadyPromise and clientReadyResolve and proceed with
cleanup (stop languageClient, unregister via registerLSPClient(null), null out
webSocket). Update the ws.onclose handler around the existing languageClient
cleanup to also handle the unresolved clientReadyResolve path so failed
connections always settle the initialization promise.
In @src/ui/src/builder/lspModelSync.ts:
- Around line 38-53: Replace the brittle setTimeout-based readiness hack around
setTimeout(...) that sends the didOpen notification: instead of waiting 100ms,
wait for the LSP client's readiness (e.g., await lspClient.onReady() if provided
or check lspClient.initializeResult) before calling
lspClient.sendNotification("textDocument/didOpen", ...); if the client is not
initialized, log a warning via logger.warn("LSP client not initialized, skipping
didOpen") and skip sending, ensuring the model's uri/languageId/version/text are
used only after readiness is confirmed.
🧹 Nitpick comments (9)
src/ui/vite.config.ts (2)
10-39: Consider making the path resolution more robust.The hardcoded
../../node_modules/path assumes a specific repository structure and may fail in different environments (e.g., monorepos, different build contexts, or when the working directory changes). Additionally, the plugin unconditionally appends.jswhich may not handle TypeScript definition files correctly.💡 Suggested improvements
Consider using Node.js resolution or Vite's built-in resolver:
function fixMonacoVSCodePaths(): Plugin { return { name: "fix-monaco-vscode-paths", enforce: "pre", - resolveId(source) { + resolveId(source, importer) { if (source.startsWith("@codingame/monaco-vscode-api/vscode/vs/")) { const pathAfterVs = source.slice( "@codingame/monaco-vscode-api/vscode/vs/".length, ); - const absolutePath = fileURLToPath( - new URL( - `../../node_modules/@codingame/monaco-vscode-api/vscode/src/vs/${pathAfterVs}.js`, - import.meta.url, - ), - ); + // Use require.resolve or this.resolve for proper module resolution + const targetPath = `@codingame/monaco-vscode-api/vscode/src/vs/${pathAfterVs}.js`; + try { + return this.resolve(targetPath, importer, { skipSelf: true }); + } catch (e) { + // Fallback to original behavior + const absolutePath = fileURLToPath( + new URL( + `../../node_modules/@codingame/monaco-vscode-api/vscode/src/vs/${pathAfterVs}.js`, + import.meta.url, + ), + ); + return { id: absolutePath, external: false }; + } - return { - id: absolutePath, - external: false, - }; } return null; }, }; }
71-71: Consider deduplicating additional Monaco packages.Deduplicating
monaco-editoris essential. Consider also adding the related Monaco-vscode packages to prevent duplicate instances, which can cause runtime issues with singleton services.💡 Optional dedupe additions
- dedupe: ["monaco-editor"], + dedupe: [ + "monaco-editor", + "@codingame/monaco-vscode-api", + "monaco-languageclient", + "vscode-languageclient" + ],src/ui/src/builder/lspModelSync.ts (1)
56-72: Consider implementing incremental document synchronization.The current implementation uses full-text synchronization, sending the entire document content on every change. For large Python files, this can be inefficient and generate significant network traffic on every keystroke.
⚡ Implement incremental sync for better performance
The LSP protocol supports incremental synchronization where only the changed ranges are sent. This requires:
- Set
textDocumentSynccapability toIncrementalduring initialization- Send only the changed ranges in
didChange:model.onDidChangeContent((event) => { const contentChanges = event.changes.map(change => ({ range: { start: { line: change.range.startLineNumber - 1, character: change.range.startColumn - 1 }, end: { line: change.range.endLineNumber - 1, character: change.range.endColumn - 1 } }, rangeLength: change.rangeLength, text: change.text })); lspClient.sendNotification("textDocument/didChange", { textDocument: { uri, version: model.getVersionId() }, contentChanges }); });This reduces payload size significantly, especially for large files.
src/ui/src/main.ts (2)
66-97: Clarify "synchronous" comment for Monaco worker setup.Line 68 states "MUST be synchronous" but the code uses
await(asynchronous). The comment likely means "Monaco workers must complete initialization before editor instances are created," rather than requiring synchronous (non-async) execution. Consider clarifying:- // Setup Monaco editor workers first - MUST be synchronous to ensure - // MonacoEnvironment is set before any editor instances are created + // Setup Monaco editor workers first - must complete initialization + // to ensure MonacoEnvironment is set before editor instances are createdAlso, consider whether LSP initialization failures should prevent the app from mounting or if graceful degradation (editor without LSP) is intended.
84-93: Consider beforeunload reliability and event listener cleanup.The
beforeunloadevent handler provides LSP cleanup, but note:
beforeunloadcan be unreliable in mobile browsers and some scenarios (e.g., force-close, crash)- The event listener is never removed, which could cause issues if the app re-initializes without a full page reload
Consider:
- Adding
visibilitychangeevent as a fallback for cleanup- Storing the listener reference for potential cleanup
- Using
pagehideevent as a more reliable alternative in modern browsersconst handleCleanup = () => { cleanupLSP(); if (collaborationManager) { collaborationManager.updateOutgoingPing({ action: "leave" }); collaborationManager.sendCollaborationPing(); } }; // More reliable than beforeunload in some browsers window.addEventListener("pagehide", handleCleanup); // Keep beforeunload for compatibility window.addEventListener("beforeunload", handleCleanup);src/ui/src/builder/lspClientRegistry.ts (1)
21-23: Note: DuplicategetLSPClientfunction exists in lspClient.ts.Both
lspClientRegistry.tsandlspClient.tsexport agetLSPClientfunction. While the module comment explains this separation is intentional (to avoid heavy dependencies), having identically-named exports could confuse developers about which to import.Consider:
- Deprecating/removing the
lspClient.tsexport (if all callers should use the registry), or- Using distinct names like
getLSPClientFromRegistry()vsgetActiveLSPClient()Based on the module comment, it appears the registry version is the intended public API, so removing or marking the
lspClient.tsexport as internal may be clearest.src/ui/src/builder/lspCompletionProvider.ts (2)
178-197: Snippet sortText won't place items "near the top" as intended.The comment says snippets should sort "near the top", but
_${snippet.label}(e.g.,_class) will actually sort after uppercase labels and before lowercase labels due to ASCII ordering (_= 95). If you want snippets at the top, use a leading character with a lower ASCII value like!or a space.♻️ Suggested fix to sort snippets at top
- sortText: `_${snippet.label}`, // Sort snippets near the top + sortText: `!${snippet.label}`, // Sort snippets near the top
217-224: Inconsistent Monaco access pattern.The
registerLSPCompletionProviderfunction receivesmonacoInstanceas a parameter, butconvertCompletionItemKindaccesses Monaco fromwindow.monacoinstead. This is fragile and inconsistent. Ifwindow.monacois undefined, returning0may not reliably map toCompletionItemKind.Textacross Monaco versions.Consider passing the Monaco instance as a parameter for consistency and reliability.
♻️ Suggested refactor
-function convertCompletionItemKind( - lspKind: number | undefined, -): monaco.languages.CompletionItemKind { - // Import monaco dynamically to avoid circular dependencies - const monacoInstance = (window as { monaco?: typeof monaco }).monaco; - if (!monacoInstance) { - return 0; // Text - } +function convertCompletionItemKind( + lspKind: number | undefined, + monacoInstance: typeof monaco, +): monaco.languages.CompletionItemKind { + const CompletionItemKind = monacoInstance.languages.CompletionItemKind; + if (lspKind === undefined) { + return CompletionItemKind.Text; + }Then update the call site in
registerLSPCompletionProvider:- kind: convertCompletionItemKind(completionItem.kind), + kind: convertCompletionItemKind(completionItem.kind, monacoInstance),src/ui/src/builder/lspClient.ts (1)
276-282: DuplicategetLSPClientexport across modules.Both this file (line 282) and
lspClientRegistry.ts(lines 20-22) export agetLSPClient()function. This could lead to confusion about which to import. The completion provider imports from the registry, which is the intended pattern.Consider whether this export is needed here, or document that the registry is the canonical source for other modules.
If this is intentionally kept for internal use within the LSP modules, consider renaming or removing the export to avoid ambiguity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
src/ui/package.jsonsrc/ui/src/builder/builderEditorWorker.tssrc/ui/src/builder/lspClient.tssrc/ui/src/builder/lspClientRegistry.tssrc/ui/src/builder/lspCompletionProvider.tssrc/ui/src/builder/lspDiagnostics.tssrc/ui/src/builder/lspModelSync.tssrc/ui/src/builder/lspSetup.tssrc/ui/src/main.tssrc/ui/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/src/builder/builderEditorWorker.ts
- src/ui/src/builder/lspDiagnostics.ts
- src/ui/src/builder/lspSetup.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/ui/src/builder/lspCompletionProvider.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (1)
getLSPClient(282-282)src/ui/src/builder/lspClientRegistry.ts (1)
getLSPClient(21-23)
src/ui/src/builder/lspModelSync.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (1)
getLSPClient(282-282)src/ui/src/builder/lspClientRegistry.ts (1)
getLSPClient(21-23)
src/ui/src/builder/lspClient.ts (2)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClientRegistry.ts (1)
registerLSPClient(17-19)
src/ui/src/builder/lspClientRegistry.ts (1)
src/ui/src/builder/lspClient.ts (1)
getLSPClient(282-282)
src/ui/src/main.ts (5)
src/ui/src/builder/builderManager.ts (1)
generateBuilderManager(107-466)src/ui/src/core/useNotesManager.ts (1)
useNotesManager(34-309)src/ui/src/core/useSecretsManager.ts (1)
useSecretsManager(6-83)src/ui/src/composables/useCollaborationManager.ts (1)
useCollaborationManager(16-147)src/ui/src/builder/lspSetup.ts (2)
setupLSP(59-88)cleanupLSP(93-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.9)
- GitHub Check: build (3.13)
- GitHub Check: build (3.11)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
- GitHub Check: tests (firefox)
🔇 Additional comments (14)
src/ui/package.json (2)
7-7: Build memory increase is appropriate for Monaco/VSCode integration.The 8GB heap size allocation is reasonable given the substantial Monaco-vscode-api dependencies being integrated. These packages can be memory-intensive during bundling and tree-shaking.
28-29: The LaunchDarkly dependencies (@launchdarkly/observability ^0.4.0 and @launchdarkly/session-replay ^0.4.5) were added in this commit. Git history shows 82 insertions and 0 deletions in package.json, confirming these packages are new additions to the file with no removal involved. There is no actual inconsistency.Likely an incorrect or invalid review comment.
src/ui/src/builder/lspModelSync.ts (1)
27-36: LGTM: Clean early-return pattern for missing client.The function correctly handles the case where no LSP client is available by returning a no-op disposable, preventing null reference errors.
src/ui/src/main.ts (1)
12-12: LGTM: Type-only import and strict equality.Good improvements:
- Type-only import of
CollaborationManagerimproves tree-shaking- Replacing
==with===for mode comparisons prevents type coercion issuesAlso applies to: 33-33, 35-35, 37-37
src/ui/src/builder/lspClientRegistry.ts (1)
11-27: LGTM: Clean registry pattern for LSP client access.The lightweight registry provides a clean separation of concerns, allowing modules to access the LSP client without importing heavy dependencies. The null return pattern allows callers to handle client absence gracefully.
src/ui/src/builder/lspCompletionProvider.ts (3)
8-10: LGTM!Clean imports with proper module resolution. Using the registry's
getLSPClientis the right approach for loose coupling between modules.
12-25: LGTM!The type definition is sufficient for the mapping logic. Using
unknownfortextEdit.rangeis reasonable since the range is not used (Monaco determines the range).
33-106: LGTM!Well-structured Python snippets with proper tab stop ordering. The
eslint-disable/enablecomments are appropriately scoped.src/ui/src/builder/lspClient.ts (6)
8-18: LGTM!Clean imports and appropriate use of module-level state for singleton LSP client management.
38-48:LSPConfig.hostis fetched but never used.The
LSPConfiginterface defines ahostproperty (line 23), butconstructWebSocketURLalways useswindow.location.hrefas the base. If the LSP server runs on a different host, this won't work as expected.Is this intentional? If cross-origin LSP is needed in the future, consider using
config.hostwhen provided:♻️ Potential enhancement
-function constructWebSocketURL(port: number): string { +function constructWebSocketURL(port: number, host?: string | null): string { // Use relative URL construction like the main WebSocket const url = new URL(window.location.href); url.protocol = url.protocol.replace("https", "wss"); url.protocol = url.protocol.replace("http", "ws"); + if (host) { + url.hostname = host; + } url.port = port.toString();
55-68: LGTM!Clean fetch implementation with appropriate error handling. The logging distinguishes between endpoint unavailability (warn) and fetch failures (error).
77-102: LGTM!Well-configured language client with appropriate document selectors for in-memory Python editing. The
DoNotRestartclose action correctly delegates lifecycle management to the calling code.
223-264: LGTM with minor observation.The retry logic is clear and appropriate. The early return when
languageClient !== nullprevents duplicate initialization.One edge case: if a previous initialization left the client in a broken state (e.g., WebSocket closed but
languageClientnot yet nulled), this check would returntrueincorrectly. Theonclosehandler should handle this, but there's a small timing window. Consider usingisLSPClientActive()for the check if this becomes an issue.
288-295: LGTM!Clean shutdown implementation with proper cleanup of client, registry, and WebSocket.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/ui/src/builder/lspSetup.ts:
- Around line 74-79: The comment and log claim diagnostics are registered but
only the completion provider is explicitly set up here; clarify whether
diagnostics are handled as a side effect of initializeLSPClient() or
registerLSPCompletionProvider() by updating the comment and logger to state the
true source (e.g., "completion provider registered; diagnostics are initialized
by initializeLSPClient()"), or if diagnostics are missing, register them here
(create and store a diagnostics/disposable alongside
completionProviderDisposable and call the appropriate LSP client diagnostic
registration function). Reference registerLSPCompletionProvider,
completionProviderDisposable and initializeLSPClient when making the change.
- Line 66: Replace the arbitrary await new Promise((resolve) =>
setTimeout(resolve, 100)) in src/ui/src/builder/lspSetup.ts with an explicit
DOM-ready check: either remove it if there is no DOM dependency, or wait for
document.readyState to be "interactive" or "complete" or attach a one-time
"DOMContentLoaded" listener and resolve only when the DOM is actually ready;
update the code in the lsp setup initialization (the spot that currently uses
the 100ms timeout) to use that deterministic check and add a short comment if
you intentionally keep a delay for a specific reason.
In @src/ui/src/wds/WdsDropdownMenu.vue:
- Around line 230-238: The current filter keeps non-selectable items
(option.type === "divider" || option.type === "header") so optionsFiltered can
be non-empty even when no selectable items match the query; update the logic so
hasNoResults reflects whether any selectable items remain: compute a
selectableFiltered (filtering only options where type is not "divider"/"header"
and whose label/detail includes the query) and set hasNoResults =
selectableFiltered.length === 0; then build optionsFiltered by combining
selectableFiltered with dividers/headers only if selectableFiltered is non-empty
(or else hide them), referencing props.options, option.type, option.label,
option.detail, optionsFiltered and hasNoResults.
🧹 Nitpick comments (9)
src/ui/src/composables/useCodeEditorSettings.ts (1)
54-66: Consider eliminating type assertions with guaranteed non-null initialization.The watchers use type assertions (
as CodeEditorSettings) when updating localStorage, but sincesettings.valueis guaranteed to be initialized at lines 41-43, these assertions are defensive rather than necessary. While functionally correct, you could make the types stricter to avoid the need for assertions.♻️ Optional: Strengthen type safety
One approach is to assert non-null immediately after initialization:
// Initialize with defaults if not set if (!settings.value) { settings.value = { ...DEFAULT_SETTINGS }; } + +// At this point, settings.value is guaranteed to be non-null +const guaranteedSettings = settings.value!; // Sync changes to localStorage watch(diagnosticsEnabledRef, (value) => { - settings.value = { - ...settings.value, + guaranteedSettings = { + ...guaranteedSettings, diagnosticsEnabled: value, - } as CodeEditorSettings; + }; }); watch(aiCompletionEnabledRef, (value) => { - settings.value = { - ...settings.value, + guaranteedSettings = { + ...guaranteedSettings, aiCompletionEnabled: value, - } as CodeEditorSettings; + }; });However, since the current implementation works correctly and the pattern is clear from comments, this is purely a stylistic consideration.
src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (5)
140-157: Inconsistent property access:props.languagevslanguage.value.Line 143 uses
props.languagewhile line 150 useslanguage.value. Both should work sincelanguageis a ref created fromprops.languageviatoRefs, but for consistency and clarity, use one approach throughout.Additionally, when diagnostics are disabled, the model value is set to itself (line 155) which seems intended to trigger re-validation, but this won't clear existing diagnostics. Consider explicitly clearing markers when disabling.
Suggested fix for consistency
watch(diagnosticsEnabled, (enabled) => { if ( !editor || - props.language !== "python" || + language.value !== "python" || props.variant === "single-line" ) { return; } const model = editor.getModel(); if (model && language.value === "python") { diagnosticsDisposable?.dispose(); if (enabled) { diagnosticsDisposable = setupLSPDiagnostics(monaco); + } else { + // Clear existing diagnostics when disabled + setModelDiagnostics(monaco, model, []); } - model.setValue(model.getValue()); } });
159-188: Same inconsistency withprops.languagevslanguage.value.Line 163 uses
props.languagebut line 175 also usesprops.language. For consistency with the rest of the file (e.g., line 270 useslanguage.value), consider standardizing to one approach.
190-244: Model URI generation could cause collisions in edge cases.Using
Date.now()for model URIs (line 192) might cause collisions if multiple editors are mounted within the same millisecond. Consider using a more unique identifier.Also, the initial
diagnosticsDisposable?.dispose()on line 240 will always be null at mount time since it's only assigned on line 242. This dispose call is unnecessary.Suggested improvements
+let modelCounter = 0; + onMounted(async () => { // Create model with proper URI for LSP - const modelUri = monaco.Uri.parse(`inmemory://model/${Date.now()}.py`); + const modelUri = monaco.Uri.parse(`inmemory://model/${Date.now()}-${++modelCounter}.py`); const model = monaco.editor.createModel(And remove the unnecessary dispose:
- diagnosticsDisposable?.dispose(); if (diagnosticsEnabled.value) { diagnosticsDisposable = setupLSPDiagnostics(monaco); }
260-288: Cleanup order and missing null assignment for monacopilotCleanup.Unlike other cleanup patterns in this block,
monacopilotCleanupis called but not set tonullafterward. For consistency and to prevent potential double-cleanup issues:Suggested fix
if (monacopilotCleanup) { monacopilotCleanup(); + monacopilotCleanup = null; }
116-138: Language change watcher doesn't clear diagnostics when switching away from Python.When changing from Python to another language, the LSP sync is disposed, but any existing diagnostics markers remain on the model. Consider clearing diagnostics before changing the language.
Suggested fix
watch(language, (newLang) => { if (!editor) return; const model = editor.getModel(); if (model.getLanguageId() === newLang) return; + // Clear diagnostics before changing language (if was Python) + if (model.getLanguageId() === "python") { + setModelDiagnostics(monaco, model, []); + } + // Dispose old LSP sync before changing language if (lspSyncDisposable) {src/ui/src/builder/lspClient.ts (3)
38-48: Protocol replacement could fail for already-secure protocols.The chained
replacecalls work for http→ws and https→wss, but if the protocol is alreadywss:orws:, the second replace would incorrectly transformwss:→wss:(no change, which is fine) but conceptually the logic is fragile.More importantly, there's no validation that the resulting URL is valid before returning.
Cleaner approach
function constructWebSocketURL(port: number): string { const url = new URL(window.location.href); - url.protocol = url.protocol.replace("https", "wss"); - url.protocol = url.protocol.replace("http", "ws"); + url.protocol = url.protocol === "https:" ? "wss:" : "ws:"; url.port = port.toString(); url.pathname = "/"; url.hash = ""; url.search = ""; return url.href; }
291-298: stopLSPClient doesn't reset the ready promise state.If
stopLSPClientis called and theninitializeLSPClientis called again, the staleclientReadyPromiseandclientReadyResolvemight cause issues. These are reset inws.onclose, but ifstopLSPClientis called before the WebSocket fully closes, there could be a race.Suggested fix
export function stopLSPClient(): void { languageClient?.stop(); languageClient = null; registerLSPClient(null); webSocket?.close(); webSocket = null; + clientReadyPromise = null; + clientReadyResolve = null; }
226-247: Use the shared MAX_RETRIES constant instead of redefining locally.The codebase exports a shared
MAX_RETRIES = 5constant fromsrc/ui/src/constants/retry.ts, which is already used inmain.ts. TheinitializeLSPClientfunction redefines this as3locally without documented justification. Either use the shared constant for consistency, or add a comment explaining why LSP initialization requires fewer retries.Use shared constant
+import { MAX_RETRIES } from "../constants/retry"; + export async function initializeLSPClient(retryCount = 0): Promise<boolean> { - const MAX_RETRIES = 3; const RETRY_DELAY = 1000; // 1 second
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/ui/src/builder/BuilderEmbeddedCodeEditor.vuesrc/ui/src/builder/BuilderHeaderMoreDropdown.vuesrc/ui/src/builder/lspClient.tssrc/ui/src/builder/lspDiagnostics.tssrc/ui/src/builder/lspSetup.tssrc/ui/src/components/shared/SharedMoreDropdown.vuesrc/ui/src/composables/useCodeEditorSettings.tssrc/ui/src/wds/WdsDropdownMenu.vuesrc/ui/src/wds/WdsSwitch.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/src/builder/lspDiagnostics.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T14:01:53.761Z
Learnt from: sheikhuzairhussain
Repo: writer/writer-framework PR: 1147
File: src/ui/src/components/core/input/CoreSelectInput.vue:170-172
Timestamp: 2025-10-28T14:01:53.761Z
Learning: In CoreSelectInput.vue (src/ui/src/components/core/input/CoreSelectInput.vue), the watcher on fields.allowMultiSelect intentionally resets formValue to defaultValue when the multi-select mode toggles, rather than preserving the user's current selection.
Applied to files:
src/ui/src/components/shared/SharedMoreDropdown.vue
🧬 Code graph analysis (3)
src/ui/src/builder/lspSetup.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClient.ts (2)
initializeLSPClient(226-267)stopLSPClient(291-298)src/ui/src/builder/lspCompletionProvider.ts (1)
registerLSPCompletionProvider(114-212)
src/ui/src/builder/lspClient.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lspClientRegistry.ts (1)
registerLSPClient(17-19)src/ui/src/constants/retry.ts (1)
MAX_RETRIES(2-2)
src/ui/src/composables/useCodeEditorSettings.ts (1)
src/ui/src/composables/useStorageJSON.ts (1)
useLocalStorageJSON(9-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.9)
- GitHub Check: build (3.10)
- GitHub Check: tests (firefox)
- GitHub Check: tests (chromium)
- GitHub Check: tests (webkit)
🔇 Additional comments (11)
src/ui/src/components/shared/SharedMoreDropdown.vue (1)
108-121: LGTM! Well-implemented UX improvement.The conditional logic correctly keeps the dropdown open for switch-type options while preserving the close-on-select behavior for regular items. The null-safe check and early return pattern are clean and appropriate.
src/ui/src/builder/BuilderHeaderMoreDropdown.vue (1)
34-34: LGTM! Clean integration with code editor settings.The code editor settings integration is well-structured:
- Proper reactive bindings between switch states and the settings composable
- Clear section organization with headers and dividers
- Toggle handlers correctly update the shared settings
Also applies to: 41-81, 189-194
src/ui/src/composables/useCodeEditorSettings.ts (1)
1-91: Well-designed singleton composable with localStorage persistence.The implementation correctly achieves cross-component reactive state sharing with persistence:
- Proper validation ensures type safety for stored data
- Module-level state ensures all instances share the same reactive refs
- Computed properties with getters/setters provide clean API
- Watchers keep localStorage in sync with reactive changes
src/ui/src/wds/WdsSwitch.vue (1)
1-77: LGTM! Accessible and well-implemented switch component.The implementation includes all essential accessibility features:
- Proper ARIA attributes (
role="switch",aria-checked)- Keyboard navigation (Enter/Space with appropriate preventDefault)
- Focus management (tabindex handling, focus-visible outline)
- Disabled state handling (cursor, opacity, interaction blocking)
- Smooth visual transitions and clear on/off states
src/ui/src/builder/lspSetup.ts (1)
57-85: Well-structured LSP initialization flow with proper error handling.The orchestration sequence is logical and includes appropriate error handling at each step. The lazy loading of VSCode services and Monaco helps with code splitting.
src/ui/src/wds/WdsDropdownMenu.vue (4)
61-135: Well-structured template with clear type branching.The template cleanly handles different option types (divider, header, switch, checkbox, regular) with appropriate conditional rendering. The use of
SharedLazyLoaderfor interactive items is a good pattern.One minor observation: the switch button has
@click.stopon the button itself (line 93) but also@click.stopon theWdsSwitch(line 109). The button's click handler does nothing, so clicks on the button area (but not on the switch) won't trigger any action, which might be the intended behavior to prevent accidental dropdown closure.
140-179: Type definitions are well-designed.The type hierarchy with
WdsDropdownMenuOptionBaseextended by specific option types provides good type safety. The union typeWdsDropdownMenuOptioncorrectly encompasses all variants.Minor note:
WdsDropdownMenuOptionRegularhastype?: "regular"(optional), making it backward-compatible with existing options that don't specify a type.
256-262: Good defensive filtering for optionsValues.Excluding dividers and headers from
optionsValuesensures they won't be included in multi-selection emissions. This is correct.
429-486: Styling is consistent and well-structured.The new CSS classes for divider, section header, and switch items follow the existing BEM-like naming convention and provide appropriate visual styling including hover and disabled states.
src/ui/src/builder/lspClient.ts (2)
77-102: Language client configuration looks correct.The document selector includes both language-based and scheme-based patterns for Python files, which should cover Monaco's in-memory models. The error handling with
DoNotRestartis appropriate for a browser context where reconnection is managed externally.
141-188: Good pattern: Configuration sent before resolving ready promise.The code correctly waits for
workspace/didChangeConfigurationto complete before resolvingclientReadyPromise, ensuring downstream code can rely on the client being fully configured.The pylsp plugin configuration (disabling pyflakes/pycodestyle/mccabe in favor of flake8) is well-documented with comments.
| // Plugin to fix monaco-vscode-api internal path resolution | ||
| // The package's exports use wildcards: "./vscode/*" -> "./vscode/src/*.js" | ||
| // Rollup doesn't respect these, so we manually construct absolute paths | ||
| function fixMonacoVSCodePaths(): Plugin { |
There was a problem hiding this comment.
This is needed to remap folder structure of monaco plugins, it is vite building problem fix and won't affect another libraries.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/ui/src/builder/lsp/lspClient.ts:
- Around line 20-24: LSPConfig declares a host but constructWebSocketURL ignores
it; update constructWebSocketURL to use config.host when non-null/ non-empty
(falling back to window.location.hostname or similar) and preserve existing
behavior for port and protocol selection (ws/wss) so the client connects to the
backend-specified host when provided; reference the LSPConfig interface and the
constructWebSocketURL function to locate and modify the logic, and ensure
null-safe handling of host and port.
- Around line 226-267: initializeLSPClient has a race: concurrent callers see
languageClient === null and each creates a WebSocket; fix by introducing an
initialization guard so only the first caller performs setup and others await it
(e.g., set a module-scoped isInitializing flag or assign clientReadyPromise
immediately at function start when no init in progress). Ensure
initializeLSPClient sets clientReadyPromise (or isInitializing) before calling
initWebSocketAndStartClient, awaits that promise for subsequent callers, and
clears/reset the guard/promise on failure so retries work; reference
initializeLSPClient, languageClient, webSocket, clientReadyPromise, and
initWebSocketAndStartClient when implementing.
🧹 Nitpick comments (12)
src/ui/src/main.ts (1)
66-97: Consider registeringbeforeunloadhandler unconditionally for edit mode.The
beforeunloadhandler that cleans up collaboration is nested inside the LSP try block. If LSP initialization fails (line 94-96), the collaboration cleanup still needs to happen on page unload, but the handler won't be registered.♻️ Suggested refactor
// Initialize Monaco editor workers and LSP client in edit mode if (mode === "edit") { + // Setup cleanup on browser unload (tab close, refresh, navigation away) + let cleanupLSPRef: (() => void) | null = null; + window.addEventListener("beforeunload", () => { + cleanupLSPRef?.(); + if (collaborationManager) { + collaborationManager.updateOutgoingPing({ + action: "leave", + }); + collaborationManager.sendCollaborationPing(); + } + }); + // Setup Monaco editor workers first - MUST be synchronous to ensure // MonacoEnvironment is set before any editor instances are created try { await import("./builder/builderEditorWorker.js"); logger.log("Monaco editor workers initialized"); } catch (error) { logger.error("Failed to initialize Monaco workers:", error); } // Then initialize LSP try { const { setupLSP, cleanupLSP } = await import( "./builder/lsp/lspSetup.js" ); + cleanupLSPRef = cleanupLSP; await setupLSP(); - - // Setup cleanup on browser unload (tab close, refresh, navigation away) - window.addEventListener("beforeunload", () => { - cleanupLSP(); - if (collaborationManager) { - collaborationManager.updateOutgoingPing({ - action: "leave", - }); - collaborationManager.sendCollaborationPing(); - } - }); } catch (error) { logger.error("Failed to initialize LSP:", error); } }src/ui/src/builder/lsp/lspCompletionProvider.ts (1)
217-224: Inconsistent monaco instance access pattern.The function accesses
window.monacowhile the caller (registerLSPCompletionProvider) already hasmonacoInstancepassed as a parameter. Consider passingmonacoInstancetoconvertCompletionItemKindinstead of relying on a global that may not exist.♻️ Suggested refactor
-function convertCompletionItemKind( - lspKind: number | undefined, -): monaco.languages.CompletionItemKind { - // Import monaco dynamically to avoid circular dependencies - const monacoInstance = (window as { monaco?: typeof monaco }).monaco; - if (!monacoInstance) { - return 0; // Text - } - - const CompletionItemKind = monacoInstance.languages.CompletionItemKind; +function convertCompletionItemKind( + lspKind: number | undefined, + monacoInstance: typeof monaco, +): monaco.languages.CompletionItemKind { + const CompletionItemKind = monacoInstance.languages.CompletionItemKind;Then update the call site at line 167:
- kind: convertCompletionItemKind(completionItem.kind), + kind: convertCompletionItemKind(completionItem.kind, monacoInstance),src/ui/src/builder/lsp/lspSetup.ts (1)
65-66: Magic delay for DOM readiness is fragile.The 100ms timeout is a timing-based workaround that may be insufficient on slower systems or unnecessary on faster ones. Consider using a more deterministic approach if possible, or at minimum document why this delay is needed.
- // Step 2: Wait for DOM to be fully ready - await new Promise((resolve) => setTimeout(resolve, 100)); + // Step 2: Wait for DOM to be fully ready + // Note: This delay ensures Monaco editor instances are mounted before LSP client initialization. + // A more robust approach would be to wait for a specific DOM element or event. + await new Promise((resolve) => setTimeout(resolve, 100));src/ui/src/builder/lsp/lspModelSync.ts (1)
56-72: Consider debouncingdidChangenotifications for rapid typing.Each keystroke triggers a
textDocument/didChangenotification with the full document content. This could overwhelm the LSP server during rapid typing. Consider adding a debounce (e.g., 100-300ms) to batch changes.♻️ Example with debounce
+let changeTimeout: ReturnType<typeof setTimeout> | null = null; + // Listen for content changes and send textDocument/didChange const changeDisposable = model.onDidChangeContent(() => { + if (changeTimeout) { + clearTimeout(changeTimeout); + } + changeTimeout = setTimeout(() => { lspClient .sendNotification("textDocument/didChange", { textDocument: { uri, version: model.getVersionId(), }, contentChanges: [ { text: model.getValue(), }, ], }) .catch((error: unknown) => { logger.error("Failed to send textDocument/didChange:", error); }); + }, 150); });src/ui/src/builder/lsp/lspDiagnostics.ts (1)
11-27: Framework globals filter list may need maintenance.This hardcoded set of Writer framework globals filters out false positive "undefined name" diagnostics. Consider adding a comment indicating where this list should be maintained or whether it's auto-generated.
/** * Writer framework globals that are available in code blocks and event handlers. * These should not be flagged as undefined by the linter. + * + * NOTE: Keep this list in sync with the Writer framework runtime globals. + * See: [link to source of truth, e.g., runtime/globals.py or docs] */ const WRITER_FRAMEWORK_GLOBALS = new Set([src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (2)
190-197: Model URI may collide in rapid editor creation.Using
Date.now()for the model URI could cause collisions if multiple editors are created within the same millisecond. Consider using a more unique identifier.♻️ Suggested fix using crypto.randomUUID
- const modelUri = monaco.Uri.parse(`inmemory://model/${Date.now()}.py`); + const modelUri = monaco.Uri.parse(`inmemory://model/${crypto.randomUUID()}.py`);
149-157: Forcing diagnostics refresh withsetValueis a workaround.Setting the model value to itself (
model.setValue(model.getValue())) triggers content change events to force a diagnostics refresh. This works but is indirect. Consider triggering the LSP directly or documenting the workaround.if (model && language.value === "python") { diagnosticsDisposable?.dispose(); if (enabled) { diagnosticsDisposable = setupLSPDiagnostics(monaco); } - model.setValue(model.getValue()); + // Trigger a content change to force diagnostics refresh + // TODO: Consider sending textDocument/didChange directly via LSP + model.setValue(model.getValue()); }src/ui/src/builder/lsp/lspClient.ts (5)
55-68: Consider adding a timeout to the fetch request.The fetch call has no timeout, which could cause
initializeLSPClientto hang indefinitely if the server is unresponsive.♻️ Suggested improvement with AbortController
async function fetchLSPConfig(): Promise<LSPConfig | null> { try { - const response = await fetch("/api/lsp-config"); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + const response = await fetch("/api/lsp-config", { + signal: controller.signal, + }); + clearTimeout(timeoutId); if (!response.ok) { logger.warn("LSP config endpoint not available:", response.status); return null; }
190-209: Potential doublestop()call on client.When the WebSocket closes, both
reader.onClose()(line 192) andws.onclose(line 202) will execute, each callingstop()on the client. Whilestop()is likely idempotent, this creates redundant cleanup logic.♻️ Suggested consolidation
Consider removing the
reader.onClosehandler sincews.onclosealready handles cleanup:- // Stop client when connection closes - reader.onClose(() => { - client.stop(); - }); };Or alternatively, guard the
ws.oncloseto only clean up state without stopping (since reader.onClose handles it):ws.onclose = () => { - if (languageClient) { - languageClient.stop(); - languageClient = null; + if (languageClient !== null) { + // Client stop is handled by reader.onClose + languageClient = null; registerLSPClient(null); }
227-228: Consider using the sharedMAX_RETRIESconstant.A
MAX_RETRIESconstant with value5is already defined insrc/ui/src/constants/retry.ts, but this function defines a local one with value3. Consider using the shared constant for consistency, or explicitly document why a different value is needed here.
285-285: DuplicategetLSPClientfunction name across modules.This module exports
getLSPClient, butlspClientRegistry.tsalso exports a function with the same name. Both should return the same client (sinceregisterLSPClientkeeps them in sync), but this creates potential confusion for consumers about which to import. Consider whether both exports are necessary, or rename one for clarity.
291-298: Cleanup of ready promise state may be incomplete.
stopLSPClientrelies onws.oncloseto clean upclientReadyPromiseandclientReadyResolve. However, if the WebSocket never connected (e.g., connection failed immediately),onclosemay not fire, leaving stale promise state. Consider explicitly resetting these:♻️ Suggested fix
export function stopLSPClient(): void { languageClient?.stop(); languageClient = null; registerLSPClient(null); webSocket?.close(); webSocket = null; + clientReadyPromise = null; + clientReadyResolve = null; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/ui/src/builder/BuilderEmbeddedCodeEditor.vuesrc/ui/src/builder/lsp/lspClient.tssrc/ui/src/builder/lsp/lspClientRegistry.tssrc/ui/src/builder/lsp/lspCompletionProvider.tssrc/ui/src/builder/lsp/lspDiagnostics.tssrc/ui/src/builder/lsp/lspModelSync.tssrc/ui/src/builder/lsp/lspSetup.tssrc/ui/src/main.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/ui/src/builder/lsp/lspCompletionProvider.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lsp/lspClient.ts (1)
getLSPClient(285-285)src/ui/src/builder/lsp/lspClientRegistry.ts (1)
getLSPClient(21-23)
src/ui/src/builder/lsp/lspClientRegistry.ts (1)
src/ui/src/builder/lsp/lspClient.ts (1)
getLSPClient(285-285)
src/ui/src/builder/lsp/lspModelSync.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lsp/lspClient.ts (1)
getLSPClient(285-285)src/ui/src/builder/lsp/lspClientRegistry.ts (1)
getLSPClient(21-23)
src/ui/src/builder/lsp/lspDiagnostics.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lsp/lspClient.ts (1)
getLSPClient(285-285)src/ui/src/builder/lsp/lspClientRegistry.ts (1)
getLSPClient(21-23)
src/ui/src/builder/lsp/lspSetup.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lsp/lspClient.ts (2)
initializeLSPClient(226-267)stopLSPClient(291-298)src/ui/src/builder/lsp/lspCompletionProvider.ts (1)
registerLSPCompletionProvider(114-212)
src/ui/src/builder/lsp/lspClient.ts (3)
src/ui/src/composables/useLogger.ts (1)
useLogger(8-15)src/ui/src/builder/lsp/lspClientRegistry.ts (2)
registerLSPClient(17-19)getLSPClient(21-23)src/ui/src/constants/retry.ts (1)
MAX_RETRIES(2-2)
src/ui/src/main.ts (2)
src/ui/src/composables/useCollaborationManager.ts (1)
useCollaborationManager(16-147)src/ui/src/builder/lsp/lspSetup.ts (2)
setupLSP(57-85)cleanupLSP(90-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (3.9)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.13)
- GitHub Check: build (3.10)
- GitHub Check: tests (firefox)
- GitHub Check: tests (webkit)
- GitHub Check: tests (chromium)
🔇 Additional comments (9)
src/ui/src/main.ts (1)
32-37: LGTM!Good use of strict equality (
===) for mode comparisons and proper conditional initialization of managers.src/ui/src/builder/lsp/lspCompletionProvider.ts (1)
114-212: LGTM!The completion provider implementation correctly handles:
- LSP position conversion (1-based to 0-based)
- Both array and
CompletionListresponse formats- Snippet insertion with proper
insertTextRules- Graceful error handling with empty suggestions fallback
src/ui/src/builder/lsp/lspSetup.ts (1)
90-96: LGTM!The cleanup function properly disposes the completion provider and stops the LSP client. The
vscodeServicesReadyflag not being reset appears intentional since VSCode services typically don't need re-initialization once loaded.src/ui/src/builder/lsp/lspModelSync.ts (1)
27-34: LGTM!Good defensive check for LSP client availability with a no-op disposable fallback. The error handling with
.catch()on async notifications is appropriate.src/ui/src/builder/lsp/lspDiagnostics.ts (1)
107-163: LGTM!The diagnostics setup correctly:
- Subscribes to LSP
publishDiagnosticsnotifications- Filters framework-specific false positives
- Converts LSP positions to Monaco positions (0-based → 1-based)
- Handles missing models gracefully
- Returns a proper disposable for cleanup
src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (1)
260-288: LGTM!The cleanup sequence properly:
- Disposes LSP sync first
- Clears diagnostics before disposing model
- Disposes editor and model
- Cleans up monacopilot
- Disposes diagnostics listener
- Disconnects resize observer
Good defensive cleanup with null checks throughout.
src/ui/src/builder/lsp/lspClientRegistry.ts (1)
1-27: No actionable issue identified.Both modules do export
getLSPClient, but explicit import paths in the codebase prevent accidental imports. All current usage correctly imports fromlspClientRegistry.js, and there is no evidence of confusion in practice. The two exports serve different purposes: the registry provides lightweight access, whilelspClient.tsdocuments its export for manual document synchronization. The module separation is functioning as designed.Likely an incorrect or invalid review comment.
src/ui/src/builder/lsp/lspClient.ts (2)
77-102: LGTM!The language client configuration is well-structured with appropriate document selectors and a sensible error handler that prevents restart loops.
274-277: LGTM!The active state check correctly verifies both the client and WebSocket connection status.
madeindjs
left a comment
madeindjs
left a comment
There was a problem hiding this comment.
I didn't finish the review; here is a first pass, which points to some cases wherein you could load Monaco and other big modules in the main bundle.
|
Graham C has submitted feedback.
Reviewed with ❤️ by PullRequest |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/package.json (1)
1-82: Critical: Lockfile out of sync with package.json.The CI pipeline is failing because
package-lock.jsonis not synchronized with the changes inpackage.json. Runnpm installto regenerate the lockfile and commit it.
🤖 Fix all issues with AI agents
In `@src/ui/package.json`:
- Line 7: The build script in package.json uses Unix-style single quotes for
NODE_OPTIONS which breaks on Windows; update the "build" script entry and
devDependencies so it works cross-platform by adding cross-env to
devDependencies and changing the "build" script to use cross-env to set
NODE_OPTIONS for vite build, or alternatively set the node-options in a
project-level .npmrc to avoid script changes, or at minimum replace the single
quotes with double quotes for better Windows compatibility; modify the "build"
script key and package.json devDependencies accordingly (refer to the "build"
script in package.json and the devDependencies section).
- Around line 21-23: Remove the direct "monaco-editor" dependency and replace it
with the `@codingame` package aliased as monaco-editor so the three
`@codingame/monaco-vscode-`* packages match the expected editor API; specifically,
delete the "monaco-editor": "^0.52.0" entry and add the
"@codingame/monaco-vscode-editor-api" package aliased to the monaco-editor name
(using your package manager's alias syntax, e.g., npm aliasing) so
`@codingame/monaco-vscode-api`,
`@codingame/monaco-vscode-languages-service-override` and
`@codingame/monaco-vscode-textmate-service-override` all target the correct editor
API version.
In `@src/writer/lsp_manager.py`:
- Around line 26-55: The loop in _find_available_port uses range(start_port,
end_port) which skips testing end_port; update the range to include the end
bound by using range(start_port, end_port + 1) (or alternatively adjust the
docstring to state the end is exclusive) so the function behavior matches its
documentation; change the range call inside the _find_available_port method
accordingly and ensure the RuntimeError message still accurately reflects the
tested range.
- Around line 91-96: The subprocess.Popen call that assigns self.process and
launches "pylsp" with ["pylsp", "--ws", "--port", str(self.port), "--host",
self.host] is piping stdout/stderr which can fill buffers and block the
long‑lived LSP process; change the launch to either redirect stdout and stderr
to subprocess.DEVNULL or attach a background thread/async reader to drain
self.process.stdout and self.process.stderr (and ensure text=True handling) so
the buffers never block; update the code that constructs self.process to use the
chosen mitigation and keep references to self.process for termination/cleanup.
♻️ Duplicate comments (7)
src/ui/src/builder/lsp/lspSetup.ts (1)
65-67: Replace arbitrary 100ms delay with DOM‑ready check.A fixed timeout is flaky and can add unnecessary latency. Prefer a deterministic DOMContentLoaded gate (or document why a delay is required).
✅ DOM‑ready alternative
- // Step 2: Wait for DOM to be fully ready - await new Promise((resolve) => setTimeout(resolve, 100)); + // Step 2: Ensure DOM is ready + if (document.readyState === "loading") { + await new Promise((resolve) => + document.addEventListener("DOMContentLoaded", resolve, { once: true }), + ); + }src/writer/ai/code_completion.py (2)
62-103: Validate/truncate completion inputs before prompt build.Unbounded
textBeforeCursor/textAfterCursorand unconstrainedlanguage/filenamecan inflate token usage and enable prompt‑injection patterns. Add allowlist + length caps before building the prompt.🛡️ Suggested guardrails
language = request_body.get("language", "") text_before = request_body.get("textBeforeCursor", "") text_after = request_body.get("textAfterCursor", "") filename = request_body.get("filename", "untitled") + + allowed_languages = {"python", "javascript", "typescript"} + if language not in allowed_languages: + return {"completion": ""} + + max_context = 10000 + max_filename = 255 + if len(text_before) > max_context: + text_before = text_before[-max_context:] + if len(text_after) > max_context: + text_after = text_after[:max_context] + if len(filename) > max_filename: + filename = filename[:max_filename]
105-116: Add timeout/cancellation around Writer API call.An unbounded await can hang the request and tie up the event loop. Wrap with
asyncio.wait_foror pass a SDK timeout (confirm supported API).⏱️ Example using asyncio.wait_for
- response = await self.client.chat.chat( - model=self.model, - messages=[ - { - "role": "user", - "content": prompt - } - ], - max_tokens=200, - temperature=0.3, - ) + response = await asyncio.wait_for( + self.client.chat.chat( + model=self.model, + messages=[ + { + "role": "user", + "content": prompt + } + ], + max_tokens=200, + temperature=0.3, + ), + timeout=float(os.getenv("WRITER_COPILOT_TIMEOUT", "10")), + )+import asynciosrc/writer/serve.py (2)
270-289: Protect /api/lsp-config with session auth (and rate limiting).This endpoint exposes internal port/host without auth. Please gate it with edit‑session validation and add throttling if possible.
🔒 Example session check
- `@app.get`("/api/lsp-config") - async def lsp_config(): + `@app.get`("/api/lsp-config") + async def lsp_config(request: Request): """ Returns LSP server configuration for the frontend. Only available in edit mode. """ if serve_mode != "edit": raise HTTPException(status_code=403, detail="LSP config only available in edit mode.") + + session_id = request.cookies.get("session") + if not session_id or not await app_runner.check_session(session_id): + raise HTTPException(status_code=401, detail="Unauthorized")
371-424: Harden /api/code-completion against unauthenticated or oversized requests.This endpoint can burn API credits and be abused. Add edit‑session auth, rate limiting, and per‑field size caps; also enforce body size even when
Content-Lengthis missing.🔐 Minimal hardening sketch
`@app.post`("/api/code-completion") async def code_completion(request: Request): """ Handles AI-powered code completion requests from monacopilot. @@ if serve_mode != "edit": raise HTTPException( status_code=403, detail="Code completion only available in edit mode" ) + + session_id = request.cookies.get("session") + if not session_id or not await app_runner.check_session(session_id): + raise HTTPException(status_code=401, detail="Unauthorized")src/ui/src/builder/lsp/lspClient.ts (1)
116-214: Add a WebSocket connection timeout to avoid hanging initialization.If the socket never opens or closes,
clientReadyPromisecan wait indefinitely. Add a short timeout that closes the socket and clears the promise.⏲️ Timeout example
try { const ws = new WebSocket(url); + + const connectionTimeout = setTimeout(() => { + if (ws.readyState === WebSocket.CONNECTING) { + logger.warn("LSP WebSocket connection timed out"); + ws.close(); + } + }, 10000); @@ ws.onopen = () => { + clearTimeout(connectionTimeout); // Python LSP client connected @@ ws.onclose = () => { + clearTimeout(connectionTimeout); // Resolve clientReadyPromise if it was never resolved // This prevents hanging when WebSocket fails to connectsrc/ui/package.json (1)
49-50: Monaco version incompatibility has been resolved.The upgrade of
monaco-editorto^0.52.0addresses the previously flagged compatibility issue withmonaco-languageclient ^10.5.0. The 10.x series requires monaco-editor 0.52+, which is now satisfied.
bybash
force-pushed
the
feat/code-completion
branch
from
ae5d364 to
148eba0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/src/wds/WdsDropdownMenu.vue (1)
218-224: Multi-select count denominator includes non-selectable items.
With headers/dividers added toprops.options, the total count is inflated and the “x/y selected” text becomes misleading.✅ Proposed fix
const headerSelectCountText = computed(() => { const selectedArray = Array.isArray(props.selected) ? props.selected : [props.selected]; const count = selectedArray.filter(Boolean).length; - return `${count}/${props.options.length} selected`; + const selectableCount = props.options.filter( + (o) => o.type !== "divider" && o.type !== "header", + ).length; + return `${count}/${selectableCount} selected`; });
🤖 Fix all issues with AI agents
In `@src/ui/src/builder/BuilderEmbeddedCodeEditor.vue`:
- Around line 159-188: The watch handler for aiCompletionEnabled is incorrectly
checking props.language instead of the reactive language.value; update the
conditional inside the watch on aiCompletionEnabled to use language.value (the
same reactive reference used elsewhere) when comparing to "python" and keep all
other logic unchanged so monacopilotCleanup, useMonacopilot, and the
enable/disable flow continue to work as expected.
- Around line 140-157: The watch callback uses inconsistent language checks
(props.language at the top vs. language.value inside) which can cause mismatched
behavior; update the condition so both checks reference the same source of truth
(either always use props.language or always use the reactive language.value)
inside the watch that observes diagnosticsEnabled and editor, e.g., make the
early-return use the same symbol used later (language.value or props.language),
and keep diagnosticsDisposable, setupLSPDiagnostics(monaco), and
model.setValue(model.getValue()) logic unchanged.
In `@src/ui/src/main.ts`:
- Around line 84-96: The beforeunload handler that calls cleanupLSP() and sends
the collaboration "leave" ping is currently registered inside the try block that
calls setupLSP(), so if setupLSP() throws the collaboration leave ping is never
registered; move or duplicate the window.addEventListener("beforeunload", ...)
registration so it runs regardless of setupLSP() success (i.e., register the
handler outside/after the try/catch) and ensure it still calls cleanupLSP() and,
if collaborationManager exists, calls collaborationManager.updateOutgoingPing({
action: "leave" }) and collaborationManager.sendCollaborationPing().
In `@src/ui/src/wds/WdsDropdownMenu.vue`:
- Around line 84-111: The outer <button> wrapping the WdsSwitch must be replaced
with a non-interactive container that assumes the interactive role to avoid
nested controls: change the outer element to a <div> with an appropriate role
(e.g., role="menuitemcheckbox" or role="button"), add tabindex when not
disabled, move the `@click.stop` handler from the nested WdsSwitch to that
container (keep calling onSelect(option.value)), remove the nested click on
WdsSwitch, and surface disabled state via aria-disabled and the existing
'WdsDropdownMenu__switchItem--disabled' class while still binding option.checked
to WdsSwitch (WdsSwitch, onSelect, option.checked, and the CSS class names
identify where to make these edits).
♻️ Duplicate comments (11)
src/ui/src/wds/WdsDropdownMenu.vue (1)
226-247: Search “No results” can be masked by headers/dividers.
optionsFilteredalways keeps dividers/headers, sohasNoResultsstays false even when no selectable items match. This is the same issue flagged previously.💡 Suggested fix
-const optionsFiltered = computed(() => { - if (!props.enableSearch) return props.options; - - const query = searchTerm.value.toLowerCase(); - return props.options.filter((option) => { - if (option.type === "divider" || option.type === "header") { - return true; - } - return ( - option.label.toLowerCase().includes(query) || - option.detail?.toLowerCase().includes(query) - ); - }); -}); +const selectableFiltered = computed(() => { + if (!props.enableSearch) return props.options; + const query = searchTerm.value.toLowerCase(); + return props.options.filter( + (o) => + o.type !== "divider" && + o.type !== "header" && + (o.label.toLowerCase().includes(query) || + o.detail?.toLowerCase().includes(query)), + ); +}); + +const optionsFiltered = computed(() => { + if (!props.enableSearch) return props.options; + if (selectableFiltered.value.length === 0) return []; + return props.options.filter( + (o) => + o.type === "divider" || + o.type === "header" || + selectableFiltered.value.includes(o), + ); +}); const hasNoResults = computed(() => { return ( props.enableSearch && searchTerm.value.trim() !== "" && - optionsFiltered.value.length === 0 && + selectableFiltered.value.length === 0 && !props.loading ); });src/ui/package.json (2)
7-7: Cross-platform build script issue persists.The
NODE_OPTIONS='--max-old-space-size=8192'uses Unix-style single quotes which fail on Windows. This was flagged in a previous review.
21-23: Monaco editor version compatibility concern persists.The
@codingame/monaco-vscode-*packages (v24.2.0) are designed to work with@codingame/monaco-vscode-editor-apialiased asmonaco-editor, not the directmonaco-editor@0.52.0dependency. This was flagged in a previous review.Also applies to: 49-49
src/writer/lsp_manager.py (2)
40-55: End port in range is still skipped.The docstring at lines 31-32 implies
end_portis checked, butrange(start_port, end_port)at line 48 excludes it. This was flagged in a previous review.
91-96: Piping stdout/stderr without draining risks blocking.For a long-lived process like pylsp, if stdout/stderr buffers fill, the process can block. This was flagged in a previous review. Consider redirecting to
DEVNULLor draining in a background thread.src/writer/ai/code_completion.py (2)
106-116: Missing timeout on Writer API call.The
await self.client.chat.chat(...)call has no timeout, risking indefinite hangs if the API becomes unresponsive. This was flagged in a previous review.
88-103: Missing input validation before building prompt.The
text_before,text_after, andfilenamefields are used without length validation. Large inputs could cause resource exhaustion or expensive API calls. This was flagged in a previous review.src/writer/serve.py (2)
270-288: Authentication not enforced on LSP config endpoint.The
/api/lsp-configendpoint checksserve_modebut doesn't verify user session authorization. This was flagged in a previous review.
371-424: Authentication not enforced on code completion endpoint.While size validation is implemented (lines 390-403), the endpoint lacks session-based authentication and rate limiting. Any client reaching the server can consume API credits. This was flagged in a previous review.
src/ui/src/composables/useCodeEditorSettings.ts (1)
54-66: Dangling module-level watchers are problematic.These watchers are created at module load time and never cleaned up. This can lead to memory leaks and unexpected behavior, especially during hot module replacement (HMR) in development. Consider wrapping the state and watchers inside a function, or use Vue's
provide/injectpattern to manage lifecycle properly.src/ui/src/builder/lsp/lspSetup.ts (1)
65-66: Replace arbitrary timeout with explicit DOM ready check.The 100ms
setTimeoutis unreliable. If the DOM is already ready, it adds unnecessary delay. If DOM initialization takes longer, it may be insufficient.🔧 Recommended fix
// Step 2: Wait for DOM to be fully ready - await new Promise((resolve) => setTimeout(resolve, 100)); + if (document.readyState === 'loading') { + await new Promise((resolve) => { + document.addEventListener('DOMContentLoaded', resolve, { once: true }); + }); + }
🧹 Nitpick comments (7)
src/ui/vite.config.ts (1)
7-39: Implementation looks reasonable for the Monaco VSCode API path workaround.The plugin correctly intercepts and rewrites module paths. A few observations:
The hardcoded
.jsextension on line 26 assumes the target files are always JavaScript. This aligns with current@codingame/monaco-vscode-apipackage structure.The path
../../node_modulesassumes this config runs fromsrc/ui/. If the project structure changes or this is used in a monorepo with hoisted dependencies, the resolution could break.Consider adding a fallback or existence check for resilience:
♻️ Optional: Add existence validation
+import { existsSync } from "fs"; + function fixMonacoVSCodePaths(): Plugin { return { name: "fix-monaco-vscode-paths", enforce: "pre", resolveId(source) { if (source.startsWith("@codingame/monaco-vscode-api/vscode/vs/")) { const pathAfterVs = source.slice( "@codingame/monaco-vscode-api/vscode/vs/".length, ); const absolutePath = fileURLToPath( new URL( `../../node_modules/@codingame/monaco-vscode-api/vscode/src/vs/${pathAfterVs}.js`, import.meta.url, ), ); + if (!existsSync(absolutePath)) { + console.warn(`[fix-monaco-vscode-paths] Could not resolve: ${source}`); + return null; + } + return { id: absolutePath, external: false, }; } return null; }, }; }src/writer/lsp_manager.py (1)
114-118: Uselogging.exceptionfor better stack trace capture.When catching exceptions,
logging.exceptionautomatically includes the stack trace, which aids debugging.♻️ Suggested improvement
except Exception as e: - logger.error(f"Failed to start LSP server: {e}") + logger.exception("Failed to start LSP server") self.process = None self.port = None return FalseAnd similarly at line 136:
except Exception as e: - logger.error(f"Error stopping LSP server: {e}") + logger.exception("Error stopping LSP server")Also applies to: 135-136
src/writer/ai/code_completion.py (1)
52-60: Uselogging.exceptionfor better error diagnostics.When catching exceptions,
logging.exceptionincludes the stack trace automatically.♻️ Suggested improvement
except ImportError: - logger.error( + logger.exception( "writer-sdk not properly installed. " "Install it with: pip install writer-sdk" ) self.enabled = False except Exception as e: - logger.error(f"Failed to initialize Writer client: {e}") + logger.exception("Failed to initialize Writer client") self.enabled = FalseAnd at line 127:
except Exception as e: - logger.error(f"Error getting completion from Writer: {e}") + logger.exception("Error getting completion from Writer") return {"completion": ""}Also applies to: 126-128
src/writer/serve.py (2)
399-403: Chain exception withfromfor proper traceback.When re-raising a new exception within an
exceptblock, useraise ... from errto preserve the exception chain.♻️ Suggested fix
except ValueError: - raise HTTPException( + raise HTTPException( status_code=400, detail="Invalid content-length header" - ) + ) from None
421-424: Uselogging.exceptionfor error diagnostics.Replace
logging.errorwithlogging.exceptionto capture the full stack trace.♻️ Suggested fix
except Exception as e: - logging.error(f"Error in code completion endpoint: {e}") + logging.exception("Error in code completion endpoint") # Return empty completion on error return JSONResponse(content={"completion": ""})src/ui/src/builder/lsp/lspCompletionProvider.ts (1)
217-224: Inconsistent Monaco access pattern.
convertCompletionItemKindaccesseswindow.monacoat runtime, whileregisterLSPCompletionProviderreceivesmonacoInstanceas a parameter. This is inconsistent and could fail ifwindow.monacoisn't set. PassmonacoInstanceto this function or use a closure.♻️ Suggested refactor
-function convertCompletionItemKind( +function convertCompletionItemKind( + monacoInstance: typeof monaco, lspKind: number | undefined, ): monaco.languages.CompletionItemKind { - // Import monaco dynamically to avoid circular dependencies - const monacoInstance = (window as { monaco?: typeof monaco }).monaco; - if (!monacoInstance) { - return 0; // Text - } - const CompletionItemKind = monacoInstance.languages.CompletionItemKind; // ... rest of switchThen update the call site at line 167:
- kind: convertCompletionItemKind(completionItem.kind), + kind: convertCompletionItemKind(monacoInstance, completionItem.kind),src/ui/src/builder/BuilderEmbeddedCodeEditor.vue (1)
274-282: Consider cleanup order: monacopilot before editor disposal.
monacopilotCleanuplikely holds references to the editor. Cleaning it up aftereditor.dispose()may cause issues if the cleanup logic tries to access the disposed editor.♻️ Suggested fix
+ // Clean up monacopilot before editor disposal + if (monacopilotCleanup) { + monacopilotCleanup(); + monacopilotCleanup = null; + } + if (editor) { editor.dispose(); } if (model) { model.dispose(); } - if (monacopilotCleanup) { - monacopilotCleanup(); - }
Intellisense and code diagnostics
In this PR we have IntelliSense feature with the help of pylsp LSP server which communicates between monaco editor with websocket to bring code suggestions, snippets. And furthermore, we have LSP diagnostics enabled now, so we will receive the highlighting for problematic codes while writing the code which will show immediate errors to help better coding.
Here is how IntelliSense and Diagnostics are working with LSP server:
AI code completion using Palymra X5
Also, I added AI code completion feature to our python code editor with Monacopliot library.
Here is the diagram to show how it works.
Component diagram architecture for AI code completion:
To disable CoPilot we can set
WRITER_COPILOT_ENABLEDenv variable to false. (default: true)To change the model we can use
WRITER_COPILOT_MODELenv variable. (default: palmyra-x5)Dependencies added:
Python (via Poetry)
python-lsp-server = {extras = ["all"], version = "^1.12.0"}
Includes:
pyflakes (3.2.0) - Logic error detection
pycodestyle (2.12.1) - PEP 8 style checking
jedi (0.19.2) - Code intelligence
isort (7.0.0) - Import sorting
flake8 - error detection (we only use this one for now)
TypeScript/JavaScript
"monaco-languageclient": "^10.5.0",
"vscode-languageclient": "^9.0.1",
"vscode-ws-jsonrpc": "^3.5.0",
"@codingame/monaco-vscode-api": "^24.2.0",
"@codingame/monaco-vscode-textmate-service-override": "^24.2.0",
"@codingame/monaco-vscode-languages-service-override": "^24.2.0"
Summary by CodeRabbit
New Features
Chores
Tests / CI
✏️ Tip: You can customize this high-level summary in your review settings.