Copilot commented on code in PR #9023: URL: https://github.com/apache/texera/pull/9023#discussion_r4319590955
########## frontend/src/app/workspace/component/code-editor-dialog/code-editor-host.directive.spec.ts: ########## @@ -0,0 +1,89 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component, OnDestroy } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; +import { CodeEditorHostDirective } from "./code-editor-host.directive"; +import { CodeEditorService } from "../../service/code-editor/code-editor.service"; + +/** A view that hosts the editor, as the operator canvas and the Form View do. */ +@Component({ + template: "<ng-template texeraCodeEditorHost></ng-template>", + imports: [CodeEditorHostDirective], +}) +class HostingViewComponent {} + +/** Stands in for the editor: what the host holds, and what must be torn down with the view. */ +@Component({ template: "editor" }) +class HostedComponent implements OnDestroy { + static destroyed = 0; + ngOnDestroy(): void { + HostedComponent.destroyed++; + } +} Review Comment: `HostingViewComponent` uses an `imports` array but is not marked `standalone: true`, and `HostedComponent` is also provided via `imports` despite not being standalone. In Angular, `imports` on `@Component` requires `standalone: true`, and TestBed `imports` expects NgModules or standalone declarations; this test will fail to compile in a standard Angular setup. Make these components standalone (and include required imports there), or move them to `declarations` and keep `CodeEditorHostDirective` in `imports`. ########## frontend/src/app/workspace/component/code-editor-dialog/code-editor-host.directive.spec.ts: ########## @@ -0,0 +1,89 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component, OnDestroy } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; +import { CodeEditorHostDirective } from "./code-editor-host.directive"; +import { CodeEditorService } from "../../service/code-editor/code-editor.service"; + +/** A view that hosts the editor, as the operator canvas and the Form View do. */ +@Component({ + template: "<ng-template texeraCodeEditorHost></ng-template>", + imports: [CodeEditorHostDirective], +}) +class HostingViewComponent {} + +/** Stands in for the editor: what the host holds, and what must be torn down with the view. */ +@Component({ template: "editor" }) +class HostedComponent implements OnDestroy { + static destroyed = 0; + ngOnDestroy(): void { + HostedComponent.destroyed++; + } +} + +describe("CodeEditorHostDirective", () => { + let service: CodeEditorService; + + beforeEach(async () => { + HostedComponent.destroyed = 0; + await TestBed.configureTestingModule({ imports: [HostingViewComponent, HostedComponent] }).compileComponents(); Review Comment: `HostingViewComponent` uses an `imports` array but is not marked `standalone: true`, and `HostedComponent` is also provided via `imports` despite not being standalone. In Angular, `imports` on `@Component` requires `standalone: true`, and TestBed `imports` expects NgModules or standalone declarations; this test will fail to compile in a standard Angular setup. Make these components standalone (and include required imports there), or move them to `declarations` and keep `CodeEditorHostDirective` in `imports`. ########## frontend/src/app/workspace/component/code-editor-dialog/code-editor-host.directive.ts: ########## @@ -0,0 +1,47 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Directive, OnDestroy, ViewContainerRef } from "@angular/core"; +import { CodeEditorService } from "../../service/code-editor/code-editor.service"; + +/** + * Marks the outlet the code editor opens into, on an `<ng-template>` in a view's template. Each view + * that shows operator properties -- the operator canvas and the Form View -- carries one, and the + * view in front is the editor's host: its outlet is attached to CodeEditorService when the view's + * template is built, and detached when the view is destroyed -- which closes an editor still open in + * it too, since the view's teardown destroys what its outlet holds, before this hook runs. The views + * are sibling routes and never overlap, so there is one host at a time; a late destroy still lets go + * only of its own. + */ +@Directive({ + selector: "[texeraCodeEditorHost]", + standalone: true, +}) +export class CodeEditorHostDirective implements OnDestroy { + constructor( + private readonly outlet: ViewContainerRef, + private readonly codeEditorService: CodeEditorService + ) { + this.codeEditorService.attachHost(this.outlet); + } Review Comment: Attaching the host in the constructor introduces a side effect during DI/construction rather than during a lifecycle hook. Prefer moving `attachHost(...)` to `ngOnInit` (and implement `OnInit`) so the directive’s behavior is aligned with Angular lifecycle conventions and is less surprising during instantiation/testing. ########## frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts: ########## @@ -93,14 +95,18 @@ export const LANGUAGE_SERVER_CONNECTION_TIMEOUT_MS = 1000; AnnotationSuggestionComponent, ], }) -export class CodeEditorComponent implements AfterViewInit, SafeStyle, OnDestroy { +export class CodeEditorComponent implements OnInit, AfterViewInit, SafeStyle, OnDestroy { @ViewChild("editor", { static: true }) editorElement!: ElementRef; @ViewChild("container", { static: true }) containerElement!: ElementRef; @ViewChild(AnnotationSuggestionComponent) annotationSuggestion!: AnnotationSuggestionComponent; @ViewChild(BreakpointConditionInputComponent) breakpointConditionInput!: BreakpointConditionInputComponent; private code?: YText; private workflowVersionStreamSubject: Subject<void> = new Subject<void>(); - public currentOperatorId!: string; + /** + * The operator whose code this edits, given by the opener: the operator property panel's button + * reads the highlighted operator, the Form View's the operator its field is bound to. + */ + @Input() currentOperatorId!: string; Review Comment: `ngOnInit` assumes `currentOperatorId` is always set and that `getOperator(...)` returns a valid operator; if a caller creates the component without providing this input, this will throw with a non-obvious error. Add an explicit guard at the start of `ngOnInit` that fails fast with a clear message (or safely returns) when `currentOperatorId` is missing/invalid, to make the new required-input contract robust and easier to debug. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss: ########## @@ -1104,3 +1104,11 @@ $shell: #fafafa; } } } + +/* The code editor opens into this page's outlet (texeraCodeEditorHost), so it is a descendant and + can be dragged under the sticky title row (z-index 20) and the panel's close button; keep the + editor above them. The canvas's chrome never exceeds that, so the shared editor keeps its own + value and only this page lifts it. The class is what beats the editor's own rule in specificity. */ +:host ::ng-deep #code-editor-container.box { + z-index: 21; +} Review Comment: This introduces a new `::ng-deep` usage, which is deprecated and can be brittle across Angular upgrades. Consider an alternative that doesn’t rely on deep styling (e.g., make the editor container’s z-index configurable via an input/host class, or apply this rule in a global stylesheet scoped to `texera-workflow-form`), so the Form View override remains maintainable. ########## frontend/src/app/workspace/component/code-editor-dialog/code-editor.component.ts: ########## @@ -150,8 +156,10 @@ export class CodeEditorComponent implements AfterViewInit, SafeStyle, OnDestroy private config: GuiConfigService, private uiUdfParametersSyncService: UiUdfParametersSyncService, private notificationService: NotificationService - ) { - this.currentOperatorId = this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs()[0]; + ) {} + + ngOnInit(): void { + // Everything here keys on currentOperatorId, an input set after construction. const operatorType = this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId).operatorType; Review Comment: `ngOnInit` assumes `currentOperatorId` is always set and that `getOperator(...)` returns a valid operator; if a caller creates the component without providing this input, this will throw with a non-obvious error. Add an explicit guard at the start of `ngOnInit` that fails fast with a clear message (or safely returns) when `currentOperatorId` is missing/invalid, to make the new required-input contract robust and easier to debug. -- 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]
