aglinxinyuan commented on code in PR #4997:
URL: https://github.com/apache/texera/pull/4997#discussion_r3264003975
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -187,97 +179,174 @@ 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();
Review Comment:
The field has a class-field initializer:
```ts
private workflowVersionStreamSubject: Subject<void> = new Subject<void>();
```
so it's always defined by the time `ngOnDestroy` runs — the initializer
fires during instance construction, before any user code can observe the
instance. The type is also non-optional (`Subject<void>`, not `Subject<void> |
undefined`), so the type system enforces this too. The `isDefined(...)` guard
was dead code; removing it has no behavior change.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -187,97 +179,174 @@ 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.
+ *
+ * `coeditor.clientId` and `coeditor.color` come from yjs awareness state,
+ * which any peer can write to. Both are interpolated into a `<style>` tag
+ * passed through `bypassSecurityTrustHtml`, so anything that escapes the
+ * tag would land in the page as raw HTML. Validate both to a tight
+ * allow-list (digits-only id, hex / `rgb(a)` / `hsl(a)` colour) and bail
+ * out otherwise; nothing else should reach the sanitiser.
* @param coeditor
*/
public getCoeditorCursorStyles(coeditor: Coeditor) {
- const textCSS =
+ if (!CodeEditorComponent.SAFE_CLIENT_ID.test(coeditor.clientId)) {
+ return this.sanitizer.bypassSecurityTrustHtml("");
+ }
+ if (!coeditor.color ||
!CodeEditorComponent.SAFE_CSS_COLOR.test(coeditor.color)) {
+ return this.sanitizer.bypassSecurityTrustHtml("");
+ }
+ const id = coeditor.clientId;
+ const color = coeditor.color;
+ const selectionBg = color.replace("0.8", "0.5");
+ return this.sanitizer.bypassSecurityTrustHtml(
"<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);
+ `.yRemoteSelection-${id} { background-color: ${selectionBg}}` +
+ `.yRemoteSelectionHead-${id}::after { border-color: ${color}}` +
+ `.yRemoteSelectionHead-${id} { border-color: ${color}}` +
+ "</style>"
+ );
}
- private getFileSuffixByLanguage(language: string): string {
- switch (language.toLowerCase()) {
- case "python":
- return ".py";
- case "r":
- return ".r";
- case "javascript":
- return ".js";
- case "java":
- return ".java";
- default:
- return ".py";
- }
+ // Allow-lists for the two awareness-derived values that flow into a
`<style>`
+ // tag in `getCoeditorCursorStyles`. yjs serialises clientIDs as the decimal
+ // form of a 32-bit integer, and the colours we generate elsewhere only use
+ // these notations — anything outside these patterns is rejected.
+ private static readonly SAFE_CLIENT_ID = /^\d{1,10}$/;
+ private static readonly SAFE_CSS_COLOR =
/^(?:#[0-9a-fA-F]{3,8}|rgba?\([\d.,\s]+\)|hsla?\([\d.,%\s]+\))$/;
+
+ /**
+ * 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> {
+ CodeEditorComponent.apiWrapperStartPromise ??= (async () => {
+ try {
+ const apiConfig: MonacoVscodeApiConfig = {
+ $type: "extended",
+ viewsConfig: { $type: "EditorService" },
+ userConfiguration: {
+ json: JSON.stringify({ "workbench.colorTheme": "Default Dark
Modern" }),
+ },
+ // Wire workers via thin trampolines in `./workers/`. Webpack 5 only
+ // treats `new Worker(new URL("./relative", import.meta.url))` as a
+ // worker entry point and bundles the dep tree into a chunk; bare
+ // package URLs `new URL("@codingame/...", import.meta.url)` become
+ // static assets whose relative imports 404 at runtime. Esbuild
+ // (used by @angular/build:unit-test) also requires a real on-disk
+ // file for the URL spec — 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"),
+ ]);
Review Comment:
They're the two codingame default-extension packages
(`@codingame/monaco-vscode-python-default-extension` and
`…-java-default-extension`). They register the TextMate grammar + language
configuration the editor uses at runtime to syntax-highlight Python / Java code
— the editor can technically open without them, but you'd lose colours and
language detection.
They're dynamic `import(...)` rather than top-level `import "..."` so the
Angular build pipeline doesn't tree-shake the side-effect imports — and yes,
they appear in `dist/3rdpartylicenses.json` and therefore must stay in
`LICENSE-binary`. No way to avoid the LICENSE-binary entry while keeping syntax
highlighting; both packages are MIT.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -17,128 +17,183 @@
* under the License.
*/
-/**
- * jsdom doesn't implement the SVG geometry APIs
(`SVGSVGElement#createSVGMatrix`,
- * `createSVGPoint`, `createSVGTransform`, `getScreenCTM`, `getCTM`,
- * `getBBox`). jointjs reaches into these during graph layout and crashes
- * the spec build with `TypeError: svgDocument.createSVGMatrix is not a
- * function` etc.
- *
- * The stubs below return identity-ish geometry: matrices/points behave like
- * the identity, bounding boxes report zero dimensions. That's enough for
- * jointjs construction code to not throw; specs that actually depend on
- * accurate geometry should run under Vitest browser mode rather than
- * jsdom (tracked in #4861), but the bulk of the texera specs only need
- * jointjs to instantiate cleanly.
- */
+// Test-environment polyfills + setup hooks for jsdom + the Angular
+// `@angular/build:unit-test` builder. Pulled in via `setupFiles` in
+// `angular.json`. Each block below patches one specific gap that surfaces
+// when the codingame monaco-vscode-* v25 stack or jointjs runs under jsdom.
+
+// Node ESM loader hook so every transitive `.css` import resolves to an empty
+// module. The unit-test builder pre-bundles spec files with `externalPackages:
+// true`, so imports like `monaco-languageclient` reach Node's native ESM
+// loader instead of Vite's transform pipeline — without the hook, every spec
+// that transitively loads the codingame v25 stack crashes with
+// `Unknown file extension ".css"`. Inline as a `data:` URL so we don't carry
+// a sidecar `.mjs`. Vitest re-evaluates this setup file once per spec file,
+// so we gate the registration with a `globalThis` flag — `module.register`
+// chains every call, and we don't want N hooks doing identical work for N
+// specs. Must run before any spec body imports the affected packages;
+// `module.register` needs Node 20.6+ (project pins Node ≥ 24).
Review Comment:
Tightened in f6536d790d. 14 lines → 7 lines. Same content, less prose.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
Review Comment:
Yes — most of the polyfill is monaco-languageclient-v10-driven. The
codingame v25 stack the new client pulls in calls into a bunch of browser APIs
jsdom doesn't ship: Constructable Stylesheets (`new
CSSStyleSheet().replaceSync(...)`), `CSS.escape`, `matchMedia`,
`requestIdleCallback`, and the `extension-file://` fetch in language-extension
activation. Each was crashing spec discovery on the v10 branch until the stub
landed.
The only blocks that predate this PR are the SVG geometry stubs (jointjs
needed those long before the monaco upgrade) and the ngZorro icon error
suppressor. Added a top-of-file comment in f6536d790d that calls this out
explicitly.
--
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]
