aglinxinyuan commented on code in PR #4997:
URL: https://github.com/apache/texera/pull/4997#discussion_r3231682288
##########
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:
False positive — `workflowVersionStreamSubject` is initialized at the
class-field declaration:
```ts
private workflowVersionStreamSubject: Subject<void> = new Subject<void>();
```
The initializer runs during instance construction (before any user code can
observe the field), so the subject is never `undefined` by the time
`ngOnDestroy` runs. The previous `isDefined(...)` guard was dead code; removing
it doesn't introduce a new failure mode. The field is also not optional
(`Subject<void>`, not `Subject<void> \| undefined`), so the type system
enforces this too.
##########
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:
False positive — `monacoWorkerFactory` is typed as `(logger?: ILogger) =>
void` in `monaco-languageclient/vscodeApiWrapper`:
\`\`\`ts
// node_modules/monaco-languageclient/lib/vscode/config.d.ts:40
monacoWorkerFactory?: (logger?: ILogger) => void;
\`\`\`
The wrapper invokes it for side effects only. We pull the enhanced env via
`getEnhancedMonacoEnvironment()`, which returns the **shared**
`globalThis.MonacoEnvironment` object — mutating `env.getWorker = …` on it
lands on the same object the rest of the codingame stack reads. No return value
is expected or used. Verified the upstream TypeFox angular example does the
same thing (via `configureDefaultWorkerFactory`, which also returns `void`).
##########
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:
Good catch — fixed in 67bcc5b863 (now in 29cbba5a1c after a rebase on top of
main).
Gated both `process.on("uncaughtException", …)` and
`process.on("unhandledRejection", …)` behind the same
`Symbol.for("texera.processErrorHandlersInstalled")` flag on `globalThis` we
use for the CSS loader hook. Without this, vitest re-evaluating `setupFiles`
once per spec was attaching fresh handlers each time — Node warns with
`MaxListenersExceededWarning` after ~11 specs and the benign-error filter
reruns on every captured rejection.
--
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]
