Copilot commented on code in PR #4997:
URL: https://github.com/apache/texera/pull/4997#discussion_r3212704192
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -228,53 +224,121 @@ export class CodeEditorComponent implements
AfterViewInit, SafeStyle, OnDestroy
}
}
+ /**
+ * Lazily start the global monaco-vscode-api wrapper. The vscode API
services are
+ * a process-wide singleton in v10; calling start() twice would throw, so we
share
+ * a single Promise across every CodeEditorComponent instance.
+ */
+ private static async ensureVscodeApiStarted(): Promise<void> {
+ if (CodeEditorComponent.apiWrapperStarted) {
+ return;
+ }
+ CodeEditorComponent.apiWrapperStartPromise ??= (async () => {
+ const apiConfig: MonacoVscodeApiConfig = {
+ $type: "extended",
+ viewsConfig: { $type: "EditorService" },
+ userConfiguration: {
+ json: JSON.stringify({ "workbench.colorTheme": "Default Dark Modern"
}),
+ },
+ // Wire up the workers monaco-vscode-api spawns at runtime (editor,
+ // extension host, textmate). Each `new Worker(new URL(...))` literal
+ // points at a thin trampoline in `./workers/` that just re-imports the
+ // codingame-shipped worker entry. Two reasons for the indirection:
+ // 1. webpack 5 only treats `new Worker(new URL("./relative",
import.meta.url))`
+ // as a worker entry point (so it bundles the dep tree into a
chunk);
+ // `new URL("@codingame/...", import.meta.url)` would just emit a
+ // static asset whose own relative imports 404 at runtime.
+ // 2. the test pipeline (esbuild via @angular/build:unit-test)
resolves
+ // `new URL(spec, import.meta.url)` literally relative to the
source
+ // file, so the spec needs to point at a real on-disk file. The
+ // trampolines satisfy both bundlers.
+ monacoWorkerFactory: () => {
+ const env = getEnhancedMonacoEnvironment();
+ env.getWorker = (_workerId: string, label: string): Worker => {
+ switch (label) {
+ case "editorWorkerService":
+ return new Worker(new URL("./workers/editor.worker",
import.meta.url), { type: "module" });
+ case "extensionHostWorkerMain":
+ return new Worker(new URL("./workers/extension-host.worker",
import.meta.url), { type: "module" });
+ case "TextMateWorker":
+ return new Worker(new URL("./workers/textmate.worker",
import.meta.url), { type: "module" });
+ default:
+ throw new Error(`No worker configured for label: ${label}`);
+ }
+ };
+ },
+ };
+ await new MonacoVscodeApiWrapper(apiConfig).start();
+
+ // Load AND fully activate the default language extensions. Each module
+ // exports a `whenReady()` that resolves after its TextMate grammar /
+ // configuration files are registered with the host — without waiting,
+ // the editor opens with every token rendered as the default `mtk1`
+ // class (no syntax colours). Dynamic `import(...)` is used so the
+ // Angular build pipeline doesn't tree-shake the side-effect imports.
+ const extensions = await Promise.all([
+ import("@codingame/monaco-vscode-python-default-extension"),
+ import("@codingame/monaco-vscode-java-default-extension"),
+ ]);
+ await Promise.all(extensions.map(ext => ext.whenReady?.()));
+
+ CodeEditorComponent.apiWrapperStarted = true;
+ })();
+ return CodeEditorComponent.apiWrapperStartPromise;
+ }
Review Comment:
`ensureVscodeApiStarted()` caches the first `apiWrapperStartPromise`, but if
wrapper startup or extension activation fails once, the cached promise stays
rejected and future calls will never retry (and `apiWrapperStarted` will remain
false). Consider wrapping the startup sequence in try/catch and clearing
`apiWrapperStartPromise` (and/or setting a failure flag) on rejection so a
later editor open can retry or surface a clearer error.
##########
frontend/custom-webpack.config.js:
##########
@@ -19,15 +19,85 @@
const { LicenseWebpackPlugin } = require("license-webpack-plugin");
+// Workaround for license-webpack-plugin v4 crashing with `ENOENT: scandir ''`
when a
+// bundled module has no resolvable on-disk directory. Some
@codingame/monaco-vscode-*
+// sub-modules surface as virtual entries (descriptionFileRoot === '') after
the v25
+// upgrade. Make listPaths/readFileAsUtf8/isDirectory tolerant of empty paths.
+{
+ const fs = require("fs");
Review Comment:
`const fs = require("fs")` is declared but unused in this file. Removing it
avoids confusion about whether filesystem calls are expected here.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -228,53 +224,121 @@ export class CodeEditorComponent implements
AfterViewInit, SafeStyle, OnDestroy
}
}
+ /**
+ * Lazily start the global monaco-vscode-api wrapper. The vscode API
services are
+ * a process-wide singleton in v10; calling start() twice would throw, so we
share
+ * a single Promise across every CodeEditorComponent instance.
+ */
+ private static async ensureVscodeApiStarted(): Promise<void> {
+ if (CodeEditorComponent.apiWrapperStarted) {
+ return;
+ }
+ CodeEditorComponent.apiWrapperStartPromise ??= (async () => {
+ const apiConfig: MonacoVscodeApiConfig = {
+ $type: "extended",
+ viewsConfig: { $type: "EditorService" },
+ userConfiguration: {
+ json: JSON.stringify({ "workbench.colorTheme": "Default Dark Modern"
}),
+ },
+ // Wire up the workers monaco-vscode-api spawns at runtime (editor,
+ // extension host, textmate). Each `new Worker(new URL(...))` literal
+ // points at a thin trampoline in `./workers/` that just re-imports the
+ // codingame-shipped worker entry. Two reasons for the indirection:
+ // 1. webpack 5 only treats `new Worker(new URL("./relative",
import.meta.url))`
+ // as a worker entry point (so it bundles the dep tree into a
chunk);
+ // `new URL("@codingame/...", import.meta.url)` would just emit a
+ // static asset whose own relative imports 404 at runtime.
+ // 2. the test pipeline (esbuild via @angular/build:unit-test)
resolves
+ // `new URL(spec, import.meta.url)` literally relative to the
source
+ // file, so the spec needs to point at a real on-disk file. The
+ // trampolines satisfy both bundlers.
+ monacoWorkerFactory: () => {
+ const env = getEnhancedMonacoEnvironment();
+ env.getWorker = (_workerId: string, label: string): Worker => {
+ switch (label) {
+ case "editorWorkerService":
+ return new Worker(new URL("./workers/editor.worker",
import.meta.url), { type: "module" });
+ case "extensionHostWorkerMain":
+ return new Worker(new URL("./workers/extension-host.worker",
import.meta.url), { type: "module" });
+ case "TextMateWorker":
+ return new Worker(new URL("./workers/textmate.worker",
import.meta.url), { type: "module" });
+ default:
+ throw new Error(`No worker configured for label: ${label}`);
+ }
+ };
+ },
+ };
+ await new MonacoVscodeApiWrapper(apiConfig).start();
+
+ // Load AND fully activate the default language extensions. Each module
+ // exports a `whenReady()` that resolves after its TextMate grammar /
+ // configuration files are registered with the host — without waiting,
+ // the editor opens with every token rendered as the default `mtk1`
+ // class (no syntax colours). Dynamic `import(...)` is used so the
+ // Angular build pipeline doesn't tree-shake the side-effect imports.
+ const extensions = await Promise.all([
+ import("@codingame/monaco-vscode-python-default-extension"),
+ import("@codingame/monaco-vscode-java-default-extension"),
+ ]);
+ await Promise.all(extensions.map(ext => ext.whenReady?.()));
+
+ CodeEditorComponent.apiWrapperStarted = true;
+ })();
+ return CodeEditorComponent.apiWrapperStartPromise;
+ }
+
/**
* Create a Monaco editor and connect it to MonacoBinding.
* @private
*/
private initializeMonacoEditor() {
const fileSuffix = this.getFileSuffixByLanguage(this.language);
- const userConfig: UserConfig = {
- wrapperConfig: {
- editorAppConfig: {
- $type: "extended",
- codeResources: {
- main: {
- text: this.code?.toString() ?? "",
- uri: `in-memory-${this.currentOperatorId}.${fileSuffix}`,
- },
- },
- userConfiguration: {
- json: JSON.stringify({
- "workbench.colorTheme": "Default Dark Modern",
- }),
- },
+ const editorAppConfig: EditorAppConfig = {
+ codeResources: {
+ modified: {
+ text: this.code?.toString() ?? "",
+ uri: `in-memory-${this.currentOperatorId}${fileSuffix}`,
},
},
};
- // optionally, configure python language client.
- // it may fail if no valid connection is established, yet the failure
would be ignored.
const languageServerWebsocketUrl = getWebsocketUrl(
"/python-language-server",
this.config.env.pythonLanguageServerPort
);
- if (this.language === "python") {
- userConfig.languageClientConfig = {
- languageId: this.language,
- options: {
- $type: "WebSocketUrl",
- url: languageServerWebsocketUrl,
- },
- };
- }
- // init monaco editor, optionally with attempt on language client.
- from(this.editorWrapper.initAndStart(userConfig,
this.editorElement.nativeElement))
+ const startEditor = async (): Promise<MonacoEditor | undefined> => {
+ await CodeEditorComponent.ensureVscodeApiStarted();
+ this.editorApp = new EditorApp(editorAppConfig);
+ await this.editorApp.start(this.editorElement.nativeElement);
+
+ // optionally, configure python language client.
+ // it may fail if no valid connection is established, yet the failure
would be ignored.
+ if (this.language === "python") {
+ const lcConfig: LanguageClientConfig = {
+ languageId: this.language,
+ connection: {
+ options: {
+ $type: "WebSocketUrl",
+ url: languageServerWebsocketUrl,
+ },
+ },
+ clientOptions: {
+ documentSelector: [this.language],
+ },
+ };
+ this.languageClientWrapper = new LanguageClientWrapper(lcConfig);
+ await this.languageClientWrapper.start();
+ }
+ return this.editorApp.getEditor();
+ };
+
+ from(startEditor())
.pipe(
timeout(LANGUAGE_SERVER_CONNECTION_TIMEOUT_MS),
- switchMap(() => of(this.editorWrapper.getEditor())),
- catchError(() => of(this.editorWrapper.getEditor())),
+ // Language-server connection may time out (or fail) — fall back to
the editor
+ // that was already mounted before `languageClientWrapper.start()` was
awaited.
+ catchError(() => of(this.editorApp?.getEditor())),
filter(isDefined),
untilDestroyed(this)
)
Review Comment:
`LANGUAGE_SERVER_CONNECTION_TIMEOUT_MS` is now applied to the entire
`startEditor()` promise, which includes `ensureVscodeApiStarted()`
(MonacoVscodeApiWrapper.start + extension activation) and `EditorApp.start()`.
On first load, that initialization can easily exceed 1s; if the timeout fires
before `EditorApp.start()` completes, `catchError(() =>
of(this.editorApp?.getEditor()))` will likely emit `undefined` and the
subscription never runs (so MonacoBinding, actions, debugger setup never
attach). Consider moving the timeout to only wrap
`languageClientWrapper.start()` (or otherwise ensure the editor start path is
not subject to this timeout), and keep the editor initialization always awaited.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -187,38 +184,37 @@ export class CodeEditorComponent implements
AfterViewInit, SafeStyle, OnDestroy
this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("editingCode",
false);
localStorage.setItem(this.currentOperatorId,
this.containerElement.nativeElement.style.cssText);
- if (isDefined(this.monacoBinding)) {
- this.monacoBinding.destroy();
- }
-
- this.editorWrapper.dispose(true);
+ this.monacoBinding?.destroy();
+ this.languageClientWrapper?.dispose().catch(() => {});
+ this.languageClientWrapper = undefined;
+ this.editorApp?.dispose().catch(() => {});
+ this.editorApp = undefined;
- if (isDefined(this.workflowVersionStreamSubject)) {
- this.workflowVersionStreamSubject.next();
- this.workflowVersionStreamSubject.complete();
- }
+ this.workflowVersionStreamSubject.next();
+ this.workflowVersionStreamSubject.complete();
}
/**
* Specify the co-editor's cursor style. This step is missing from
MonacoBinding.
* @param coeditor
*/
public getCoeditorCursorStyles(coeditor: Coeditor) {
- const textCSS =
- "<style>" +
- `.yRemoteSelection-${coeditor.clientId} { background-color:
${coeditor.color?.replace("0.8", "0.5")}}` +
- `.yRemoteSelectionHead-${coeditor.clientId}::after { border-color:
${coeditor.color}}` +
- `.yRemoteSelectionHead-${coeditor.clientId} { border-color:
${coeditor.color}}` +
- "</style>";
- return this.sanitizer.bypassSecurityTrustHtml(textCSS);
+ const id = coeditor.clientId;
+ const color = coeditor.color;
+ const selectionBg = color?.replace("0.8", "0.5");
+ return this.sanitizer.bypassSecurityTrustHtml(
+ `<style>` +
+ `.yRemoteSelection-${id} { background-color: ${selectionBg}}` +
+ `.yRemoteSelectionHead-${id}::after { border-color: ${color}}` +
+ `.yRemoteSelectionHead-${id} { border-color: ${color}}` +
+ `</style>`
+ );
Review Comment:
`getCoeditorCursorStyles` uses `bypassSecurityTrustHtml` to inject a
`<style>` tag containing interpolated `coeditor.clientId` and `coeditor.color`.
In collaborative mode, those values originate from shared awareness state and
can be tampered with by a malicious client, enabling CSS/HTML injection (e.g.,
breaking out of the style tag). Prefer generating styles via DOM APIs (e.g.,
`CSSStyleSheet`/`adoptedStyleSheets` in the real app) or strictly
validating/escaping `clientId` (CSS identifier) and `color` (allow only
known-safe formats like `hsl(...)`/`#RRGGBB`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
