Yicong-Huang commented on code in PR #4997:
URL: https://github.com/apache/texera/pull/4997#discussion_r3263919915
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.spec.ts:
##########
@@ -52,7 +52,6 @@ const synthesizeSchema = (operatorType: string):
OperatorSchema => ({ ...baseSch
const augmentedSchemas: OperatorSchema[] = [
...mockOperatorMetaData.operators,
...PYTHON_OPERATOR_TYPES.map(synthesizeSchema),
- ...R_OPERATOR_TYPES.map(synthesizeSchema),
Review Comment:
ditto
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.spec.ts:
##########
@@ -31,10 +31,10 @@ import { OperatorSchema } from
"../../types/operator-schema.interface";
import { of } from "rxjs";
// Operator types that the constructor's language-detection branch must map
-// to a specific language. `RUDFSource` / `RUDF` -> `r`; the three V2 Python
-// types -> `python`; everything else -> `java`. Local to this spec so we
-// don't perturb the shared mock-workflow-data fixtures.
-const R_OPERATOR_TYPES = ["RUDFSource", "RUDF"];
+// to a specific language. The three V2 Python types -> `python`; everything
+// else (including the legacy `RUDF*` types) -> `java`, since R UDF editor
+// support was retired in this branch. Local to this spec so we don't
Review Comment:
Let's not remove R UDF (or R language) support. R UDF can be installed via a
non-apache plugin. And frontend editor should work for it.
##########
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:
why can we remove
```
if (isDefined(this.workflowVersionStreamSubject)) {
```
?
##########
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:
please simplify this comment.
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.spec.ts:
##########
@@ -117,19 +116,11 @@ describe("CodeEditorComponent", () => {
expect(fixture.componentInstance.currentOperatorId).toBe(mockJavaUDFPredicate.operatorID);
});
- // Language detection — the constructor maps `RUDFSource` / `RUDF` to `r`,
- // the three V2-era Python operator types to `python`, and anything else
- // to `java`. The exact branch lives in the constructor; the public
- // `language` field is what the rest of the editor (LSP wiring, file-
- // suffix selection) keys off.
-
- R_OPERATOR_TYPES.forEach((operatorType, index) => {
- it(`picks language="r" for operatorType=${operatorType}`, () => {
- const fixture = makeFixture(buildPredicate(`r-${index}`, operatorType));
- expect(fixture.componentInstance.language).toBe("r");
- expect(fixture.componentInstance.languageTitle).toBe("R UDF");
- });
- });
Review Comment:
ditto
##########
frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts:
##########
@@ -143,18 +145,8 @@ export class CodeEditorComponent implements AfterViewInit,
SafeStyle, OnDestroy
) {
this.currentOperatorId =
this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs()[0];
const operatorType =
this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId).operatorType;
-
- if (operatorType === "RUDFSource" || operatorType === "RUDF") {
- this.setLanguage("r");
- } else if (
- operatorType === "PythonUDFV2" ||
- operatorType === "PythonUDFSourceV2" ||
- operatorType === "DualInputPortsPythonUDFV2"
- ) {
- this.setLanguage("python");
- } else {
- this.setLanguage("java");
- }
+ this.language =
CodeEditorComponent.PYTHON_OPERATOR_TYPES.has(operatorType) ? "python" : "java";
+ this.languageTitle =
`${this.language[0].toUpperCase()}${this.language.slice(1)} UDF`;
Review Comment:
ditto
##########
frontend/src/jsdom-svg-polyfill.ts:
##########
Review Comment:
Is this file related to upgrade Monaco-language client at all?
##########
frontend/LICENSE-binary:
##########
@@ -247,30 +248,40 @@ Angular / npm packages:
- @ant-design/[email protected]
- @auth0/[email protected]
- @babel/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
- - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
+ - @codingame/[email protected]
Review Comment:
do we need all of those?
##########
frontend/LICENSE-binary:
##########
@@ -283,18 +294,16 @@ Angular / npm packages:
- [email protected]
- [email protected]
- [email protected]
- - [email protected]
- [email protected]
- [email protected]
- [email protected]
- [email protected]
- [email protected]
- [email protected]
- - [email protected]
+ - [email protected]
Review Comment:
why are we downgrading this?
##########
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:
will these appear in license-BINARY? these seems to be libraries loaded
during runtime... can we avoid this?
##########
frontend/vitest.config.ts:
##########
@@ -29,17 +29,6 @@ export default defineConfig({
// which Angular's `fakeAsync` requires. Karma+Jasmine installed this
// implicitly; the @angular/build:unit-test path doesn't.
setupFiles: ["src/test-zone-setup.ts"],
- // monaco-breakpoints' entry does `import './style.css'`. By default
- // Vitest leaves third-party deps externalized, so Node's ESM loader
- // tries to import the .css and crashes with
- // `TypeError: Unknown file extension ".css"`. Inlining the package
- // routes its imports through Vite/esbuild, which rewrites the CSS
- // import to a no-op.
- server: {
- deps: {
- inline: [/monaco-breakpoints/],
- },
Review Comment:
does Monaco-breakpoint still work? we need it for debugger. We might not
have enough tests against it.
##########
frontend/package.json:
##########
@@ -9,7 +9,6 @@
"start": "concurrently --kill-others \"npx y-websocket\" \"ng serve\"",
"build": "node --max-old-space-size=8192
./node_modules/@angular/cli/bin/ng build --configuration=production
--progress=false --source-map=false",
"build:ci": "node --max-old-space-size=8192
./node_modules/nx/dist/bin/nx.js build --configuration=production
--progress=false --source-map=false",
- "analyze": "ng build --configuration=production --stats-json &&
webpack-bundle-analyzer dist/stats.json",
Review Comment:
why remove this?
--
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]
