Copilot commented on code in PR #8455: URL: https://github.com/apache/texera/pull/8455#discussion_r3971600915
########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.ts: ########## @@ -0,0 +1,75 @@ +/** + * 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 } from "@angular/core"; +import { NgIf } from "@angular/common"; +import { NzIconDirective } from "ng-zorro-antd/icon"; +import { FieldWrapper, FormlyFieldConfig } from "@ngx-formly/core"; +import { merge } from "lodash-es"; + +/** + * Lets an author rename or hide one field of the form in place: the label itself becomes + * the input, so what they type is exactly what the reader sees, where they see it (the + * schema's own labels -- "File Key", "Alias" -- describe the operator, not the reader's + * task). Renders as a plain label for anyone not authoring. + */ +@Component({ + selector: "texera-editable-label-wrapper", + templateUrl: "./editable-label-wrapper.component.html", + styleUrls: ["./editable-label-wrapper.component.scss"], Review Comment: This component declares `imports` but is missing `standalone: true`. In Angular, `imports` in component metadata is only valid for standalone components; as-is this will fail compilation. Add `standalone: true` (and keep `imports`) or remove `imports` and declare dependencies via an NgModule. ########## frontend/src/app/workspace/component/property-editor/property-editor.component.ts: ########## @@ -205,6 +228,11 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { @HostListener("window:beforeunload") ngOnDestroy(): void { + // The Form View's read-only copy (persistPlacement=false) must not persist geometry: it is not + // the docked canvas panel, so writing these keys would overwrite the real panel's saved size. + if (!this.persistPlacement) { + return; + } localStorage.setItem("right-panel-width", String(this.width)); localStorage.setItem("right-panel-height", String(this.height)); Review Comment: Avoid an early `return` in `ngOnDestroy()` to ensure any future teardown logic added later in this method still runs even when `persistPlacement` is false. Prefer guarding only the localStorage persistence block with `if (this.persistPlacement) { ... }` and letting the rest of `ngOnDestroy` execute normally. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -221,6 +361,136 @@ <h2>{{ instructionTitle || "How to use this" }}</h2> <texera-mini-map *ngIf="workflowEverOpened" class="box"></texera-mini-map> + + <!-- Sibling of the panel, not nested inside it: the property editor opens its own stacking + context, and a button inside that context paints under its content -- visible but + unclickable. As a sibling the close button is simply above. --> + <button + class="panel-close" + *ngIf="selectedOperatorId" + type="button" + aria-label="Close step details" + (click)="closeOperatorPanel()"> + <i + nz-icon + nzType="close" + aria-hidden="true"></i> + </button> + + <!-- Reader / inspect: the property editor carries the `inert` attribute (blocks pointer AND + keyboard AND focus) and does not broadcast or write, so a step opens read-only. + Authoring: the panel goes live -- inert off, exposeChoosing on (tick boxes to pick what + the form exposes), and it broadcasts/edits like the canvas, since edit mode has enabled + workflow modification. persistPlacement stays off either way (this is not the docked + canvas panel). + [hidden], not *ngIf: the property editor shows its operator by REACTING to the highlight + stream (no initial pull), so it must already be mounted and subscribed when the click + highlights a step. Mounting it on selection (*ngIf) subscribes too late, misses that + emission, and the panel opens empty. So keep it mounted and just hide it. --> + <div + class="panel" + [hidden]="!selectedOperatorId"> + <texera-property-editor + [exposeChoosing]="authoring" + [persistPlacement]="false" + [broadcastEditing]="authoring" + [attr.inert]="authoring ? null : ''"></texera-property-editor> + </div> + </div> + </section> + + <!-- Results, under the workflow that produced them. The section keeps its place before a run so + the answer has a visible destination. --> + <section class="results"> + <div class="pc-section-head"><span class="label">Results</span></div> + <p + class="results-empty" + *ngIf="resultIdsToShow.length === 0"> + {{ isRunning ? "Working…" : hasRunFinished ? "This run produced no results to show." : "Press Run and the + results appear here." }} + </p> + <!-- A card only for a step that actually produced a result. Whether a Python UDF + yields one cannot be told from the graph, so a step earns its card at runtime rather than + sitting on a permanent "No result yet." (a download/publish step never would). --> + <ng-container *ngFor="let id of resultIdsToShow"> + <div class="card result"> + <div class="result-head"> + <span>{{ resultLabel(id) }}</span> + <!-- Zooming only means something once there is a picture to zoom. --> + <span + class="result-zoom" + *ngIf="vizHasContent(id)"> + <button + (click)="zoomResult(id, -1)" + [disabled]="resultZoom(id) <= 0" + nz-tooltip="Smaller" + aria-label="Smaller"> + <i + nz-icon + nzType="minus" + aria-hidden="true"></i> + </button> + <button + (click)="zoomResult(id, 1)" + [disabled]="resultZoom(id) >= 2" + nz-tooltip="Bigger" + aria-label="Bigger"> Review Comment: The tooltip directive usage looks incorrect for ng-zorro: `nz-tooltip=\"Smaller\"` typically enables the directive but does not set the tooltip title (which is usually bound via `nzTooltipTitle`). As written, this may render an empty tooltip or none at all. Update to the supported API (e.g., enable `nz-tooltip` and set `nzTooltipTitle` accordingly). ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } Review Comment: `pruneBrokenBindings` mutates the form-binding config (`setFields`) based only on `authoring`, not on write access (`canEdit`). Since `toggleAuthoring()` can be called even when `canEdit` is false (and your tests do this), this method can still modify config state for a read-only viewer. Guard mutation with `this.canEdit` as well (or ensure `authoring` cannot be enabled without write access in the component method, not just the template). ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } + + /** + * The author's picker for the extra results: every non-terminal step with view-result ("the eye") + * on the canvas is offered, plus any already-chosen step (so a pick never vanishes from its own + * picker). The terminal step is not offered -- its result always shows and is not the author's to + * toggle. The shown flag mirrors the saved resultOperatorIds. + */ + private rebuildResultChoices(): void { + // The picker is only shown while authoring, so a reader does no per-operator work. + if (!this.authoring) { + this.resultChoices = []; + return; + } + const viewed = this.workflowActionService.getTexeraGraph().getOperatorsToViewResult(); + const chosen = this.formBindingService.getConfig().resultOperatorIds; + // The terminal result always shows and is not the author's to toggle, so it is not offered here. + // The picker curates only the extra intermediate steps -- those given view-result (the eye) on the + // canvas. Reuse the one terminal rule (terminalOperatorIds) rather than a second copy. Already-chosen + // ids stay listed so the author can un-pick them. + const terminals = new Set(this.terminalOperatorIds()); + this.resultChoices = this.operators() + .filter(op => !terminals.has(op.operatorID) && (viewed.has(op.operatorID) || chosen.includes(op.operatorID))) + .map(op => ({ + operatorID: op.operatorID, + label: this.formBindingService.operatorLabel(op), + shown: chosen.includes(op.operatorID), Review Comment: `chosen.includes(op.operatorID)` is evaluated multiple times per operator, making this O(n*m) for n operators and m chosen ids. Convert `chosen` to a `Set` once (e.g., `const chosenSet = new Set(chosen)`) and use `chosenSet.has(...)` in both the filter and map. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.html: ########## @@ -221,6 +361,136 @@ <h2>{{ instructionTitle || "How to use this" }}</h2> <texera-mini-map *ngIf="workflowEverOpened" class="box"></texera-mini-map> + + <!-- Sibling of the panel, not nested inside it: the property editor opens its own stacking + context, and a button inside that context paints under its content -- visible but + unclickable. As a sibling the close button is simply above. --> + <button + class="panel-close" + *ngIf="selectedOperatorId" + type="button" + aria-label="Close step details" + (click)="closeOperatorPanel()"> + <i + nz-icon + nzType="close" + aria-hidden="true"></i> + </button> + + <!-- Reader / inspect: the property editor carries the `inert` attribute (blocks pointer AND + keyboard AND focus) and does not broadcast or write, so a step opens read-only. + Authoring: the panel goes live -- inert off, exposeChoosing on (tick boxes to pick what + the form exposes), and it broadcasts/edits like the canvas, since edit mode has enabled + workflow modification. persistPlacement stays off either way (this is not the docked + canvas panel). + [hidden], not *ngIf: the property editor shows its operator by REACTING to the highlight + stream (no initial pull), so it must already be mounted and subscribed when the click + highlights a step. Mounting it on selection (*ngIf) subscribes too late, misses that + emission, and the panel opens empty. So keep it mounted and just hide it. --> + <div + class="panel" + [hidden]="!selectedOperatorId"> + <texera-property-editor + [exposeChoosing]="authoring" + [persistPlacement]="false" + [broadcastEditing]="authoring" + [attr.inert]="authoring ? null : ''"></texera-property-editor> + </div> + </div> + </section> + + <!-- Results, under the workflow that produced them. The section keeps its place before a run so + the answer has a visible destination. --> + <section class="results"> + <div class="pc-section-head"><span class="label">Results</span></div> + <p + class="results-empty" + *ngIf="resultIdsToShow.length === 0"> + {{ isRunning ? "Working…" : hasRunFinished ? "This run produced no results to show." : "Press Run and the + results appear here." }} + </p> + <!-- A card only for a step that actually produced a result. Whether a Python UDF + yields one cannot be told from the graph, so a step earns its card at runtime rather than + sitting on a permanent "No result yet." (a download/publish step never would). --> + <ng-container *ngFor="let id of resultIdsToShow"> + <div class="card result"> + <div class="result-head"> + <span>{{ resultLabel(id) }}</span> + <!-- Zooming only means something once there is a picture to zoom. --> + <span + class="result-zoom" + *ngIf="vizHasContent(id)"> + <button + (click)="zoomResult(id, -1)" + [disabled]="resultZoom(id) <= 0" + nz-tooltip="Smaller" + aria-label="Smaller"> Review Comment: The tooltip directive usage looks incorrect for ng-zorro: `nz-tooltip=\"Smaller\"` typically enables the directive but does not set the tooltip title (which is usually bound via `nzTooltipTitle`). As written, this may render an empty tooltip or none at all. Update to the supported API (e.g., enable `nz-tooltip` and set `nzTooltipTitle` accordingly). ########## frontend/src/app/common/formly/editable-label-wrapper/editable-label-wrapper.component.html: ########## @@ -0,0 +1,58 @@ +<!-- + 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. +--> + +<!-- Authoring: the label is the input, so what you type is what the reader reads. --> +<div + class="lbl-row" + *ngIf="props.authoring"> + <input + class="lbl-input" + [value]="props.authorName" + [placeholder]="props.schemaLabel" + (change)="onRename($event)" Review Comment: Using the `(change)` event means renames only propagate on blur/commit, not as the author types. For in-place authoring this is often unintuitive and can make previewing/validation lag behind. Consider switching to `(input)` (or `(ngModelChange)` if you move to ngModel) so renames apply immediately. ########## frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts: ########## @@ -371,16 +526,108 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } + private operators(): OperatorPredicate[] { + return this.workflowActionService.getTexeraGraph().getAllOperators(); + } + + /** + * Drop exposed inputs whose operator was deleted: they can never be filled, and a re-added + * operator gets a fresh id so they could not reconnect. Guarded to edit mode and after load, so a + * reader never mutates the workflow and a not-yet-seeded mid-load graph never deletes a still-valid + * input. Only the operator-gone case, not a transiently missing property schema. + */ + private pruneBrokenBindings(): void { + if (this.loading || !this.authoring) { + return; + } + const graph = this.workflowActionService.getTexeraGraph(); + const configFields = this.formBindingService.getConfig().fields; + const alive = configFields.filter(p => graph.hasOperator(p.operatorID)); + if (alive.length !== configFields.length) { + this.formBindingService.setFields(alive); + } + } + + /** + * The author's picker for the extra results: every non-terminal step with view-result ("the eye") + * on the canvas is offered, plus any already-chosen step (so a pick never vanishes from its own + * picker). The terminal step is not offered -- its result always shows and is not the author's to + * toggle. The shown flag mirrors the saved resultOperatorIds. + */ + private rebuildResultChoices(): void { + // The picker is only shown while authoring, so a reader does no per-operator work. + if (!this.authoring) { + this.resultChoices = []; + return; + } + const viewed = this.workflowActionService.getTexeraGraph().getOperatorsToViewResult(); + const chosen = this.formBindingService.getConfig().resultOperatorIds; + // The terminal result always shows and is not the author's to toggle, so it is not offered here. + // The picker curates only the extra intermediate steps -- those given view-result (the eye) on the + // canvas. Reuse the one terminal rule (terminalOperatorIds) rather than a second copy. Already-chosen + // ids stay listed so the author can un-pick them. + const terminals = new Set(this.terminalOperatorIds()); + this.resultChoices = this.operators() + .filter(op => !terminals.has(op.operatorID) && (viewed.has(op.operatorID) || chosen.includes(op.operatorID))) + .map(op => ({ + operatorID: op.operatorID, + label: this.formBindingService.operatorLabel(op), + shown: chosen.includes(op.operatorID), Review Comment: `chosen.includes(op.operatorID)` is evaluated multiple times per operator, making this O(n*m) for n operators and m chosen ids. Convert `chosen` to a `Set` once (e.g., `const chosenSet = new Set(chosen)`) and use `chosenSet.has(...)` in both the filter and map. -- 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]
