Copilot commented on code in PR #4997:
URL: https://github.com/apache/texera/pull/4997#discussion_r3223609260
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -158,36 +199,34 @@ class InertWebSocket {
onerror: AnyFn | null = null;
onmessage: AnyFn | null = null;
onclose: AnyFn | null = null;
- send(): void {}
- close(): void {}
- addEventListener(): void {}
- removeEventListener(): void {}
- dispatchEvent(): boolean {
- return false;
- }
+ send = () => undefined;
+ close = () => undefined;
+ addEventListener = () => undefined;
+ removeEventListener = () => undefined;
+ dispatchEvent = () => false;
constructor(_url?: string, _protocols?: string | string[]) {}
}
-(globalThis as unknown as { WebSocket: typeof InertWebSocket }).WebSocket =
InertWebSocket;
+G.WebSocket = InertWebSocket;
-/**
- * NgZorro's NzIconService dynamically fetches icon SVGs over HTTP from
- * `/assets/...` when the icon isn't pre-registered. jsdom's XHR
- * implementation rejects those requests with an `AggregateError`, and
- * downstream the icon lookup re-throws as `IconNotFoundError`. Vitest
- * catches both as unhandled errors, which CI treats as a hard failure
- * (locally Vitest only reports them as non-fatal warnings).
- *
- * Stubbing every spec with `NzIconModule.forChild([...])` for every
- * icon its template uses is impractical — there are dozens. Instead,
- * suppress the two specific error patterns at the process level: they
- * originate inside ngZorro's icon plumbing and don't affect the
- * assertions specs actually make.
- */
+// Process-level error suppression for benign ngZorro icon / codingame
+// extension fetches. NzIconService fetches icon SVGs from `/assets/...` when
+// the icon isn't pre-registered; jsdom's XHR rejects with `AggregateError`
+// and the lookup re-throws as `IconNotFoundError`. Vitest catches both as
+// unhandled errors and CI treats that as a hard failure. Stubbing every spec
+// with `NzIconModule.forChild([...])` is impractical — dozens of icons.
+// Suppress just these patterns at the process level.
function isBenignIconError(err: unknown): boolean {
const msg = err instanceof Error ? err.message : String(err);
+ const stack = err instanceof Error ? err.stack ?? "" : "";
return (
msg.includes("[@ant-design/icons-angular]") ||
- (err instanceof Error && err.name === "AggregateError" &&
/xhr-utils/.test(err.stack ?? ""))
+ (err instanceof Error && err.name === "AggregateError" &&
/xhr-utils/.test(stack)) ||
+ // codingame v25 default extensions try to fetch their bundled themes /
+ // language configs over `extension-file://` URIs at activation time.
+ // jsdom can't resolve the scheme so the fetch rejects, but it's cosmetic
+ // — the spec body never depends on the theme/grammar being applied.
+ msg.includes("extension-file://") ||
+
/workbenchThemeService|monaco-vscode-theme|monaco-vscode-.*-default-extension/.test(stack)
);
}
process.on("uncaughtException", err => {
Review Comment:
This setup file notes it can be re-evaluated once per spec file, but the
`process.on(...)` handler registration is not gated like the CSS loader hook
is. That can lead to duplicate listeners (memory leak warnings, repeated
filtering/side effects). Gate the `process.on` registrations behind the same
global flag pattern used for the loader hook so listeners are only attached
once per process.
##########
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}`);
+ }
+ };
Review Comment:
The `monacoWorkerFactory` callback configures `env.getWorker` but does not
return `env`. If the `MonacoVscodeApiWrapper` expects the factory to return the
enhanced environment, the worker wiring will be ignored and worker creation
will fail at runtime. Return `env` from the factory (or set the expected
global/field per the wrapper’s API).
##########
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();
Review Comment:
This previously guarded `workflowVersionStreamSubject` before calling
`.next()`/`.complete()`. With the guard removed, `ngOnDestroy` can throw if the
subject is ever left uninitialized. Either restore the guard or make the
subject non-optional by initializing it at declaration/construction time.
##########
frontend/LICENSE-binary:
##########
@@ -290,11 +301,10 @@ Angular / npm packages:
- [email protected]
- [email protected]
- [email protected]
- - [email protected]
+ - [email protected]
Review Comment:
`yarn.lock` shows both `[email protected]` (transitive) and `[email protected]`
(direct dependency in `package.json`) are present, but `LICENSE-binary` now
lists only `[email protected]`. Please include both versions (or regenerate the
license listing from the lockfile) to keep the third-party attribution accurate.
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
@@ -17,128 +17,169 @@
* 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).
+import { register as registerLoader } from "node:module";
+
+const CSS_HOOK_FLAG = Symbol.for("texera.cssLoaderHookRegistered");
+const flagHolder = globalThis as Record<symbol, boolean | undefined>;
+if (!flagHolder[CSS_HOOK_FLAG]) {
+ const cssLoaderHookSource = `
+export function resolve(specifier, context, nextResolve) {
+ if (specifier.endsWith(".css") || /\\.css(\\?|$)/.test(specifier)) {
+ return { url: "data:text/javascript,export%20default%20%7B%7D%3B",
shortCircuit: true, format: "module" };
+ }
+ return nextResolve(specifier, context);
+}
+`;
Review Comment:
The CSS loader hook resolves every `.css` import to a module exporting `{}`.
If any runtime code reads the default export (e.g., expecting a `CSSStyleSheet`
from `css-style-sheet` exports) this can cause follow-on type/shape errors.
Consider exporting a more compatible stub (e.g., a `CSSStyleSheet` instance
when available) so transitive consumers don’t break if they start using the
imported value.
--
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]
