Copilot commented on code in PR #8456:
URL: https://github.com/apache/texera/pull/8456#discussion_r3982471349
##########
frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts:
##########
@@ -631,10 +636,19 @@ export class OperatorPropertyEditFrameComponent
implements OnInit, OnChanges, On
this.currentOperatorSchema =
this.dynamicSchemaService.getDynamicSchema(this.currentOperatorId);
this.currentOperatorStatus =
this.workflowStatusSerivce.getCurrentStatus()[this.currentOperatorId];
-
this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ if (this.broadcastEditing) {
+ this.workflowActionService
+ .getTexeraGraph()
+ .updateSharedModelAwareness("currentlyEditing",
this.currentOperatorId);
+ }
const operator =
this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId);
- // set the operator data needed
- this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
+ // Syncing the operator to the current schema version writes the new
version into the Yjs shared
+ // model (changeOperatorVersion), which broadcasts and persists. That is
right on the canvas, but
+ // a read-only inspect (broadcastEditing=false) must not mutate the
workflow just by opening a
+ // step, so skip the sync there and show the version as stored.
+ if (this.broadcastEditing) {
+ this.workflowActionService.setOperatorVersion(operator.operatorID,
this.currentOperatorSchema.operatorVersion);
Review Comment:
`broadcastEditing=false` does not make opening the frame read-only:
`rerenderEditorForm()` still runs AJV defaults and calls `onFormChanges()`
(lines 705–714), whose debounced subscriber writes via `setOperatorProperty()`
(lines 797–804). A reader can therefore mutate shared operator properties
merely by inspecting a step whose schema added a default. The read-only mode
must also suppress all form-change/shared-model writes, not only the awareness
and version writes.
##########
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)"
+ [attr.aria-label]="'Label shown above this input'"
+ [title]="'Shown above this box. Empty keeps ' + props.schemaLabel" />
+ <!-- An input the reader can only lose by removing it altogether has no eye:
offering
+ one here would be a second place to decide the same thing. -->
+ <button
+ *ngIf="props.canHide !== false"
+ type="button"
+ class="lbl-eye"
+ [class.off]="props.authorHidden"
+ (click)="onToggleHidden()"
+ [attr.aria-pressed]="props.authorHidden"
+ [attr.aria-label]="props.authorHidden ? 'Hidden from the form. Click to
show' : 'Shown on the form. Click to hide'"
+ [title]="props.authorHidden ? 'Hidden from the form' : 'Shown on the
form'">
+ <i
+ nz-icon
+ [nzType]="props.authorHidden ? 'eye-invisible' : 'eye'"
+ nzTheme="outline"></i>
+ </button>
+</div>
+
+<!-- Everyone else just reads it. -->
+<label
+ class="lbl-static"
+ *ngIf="!props.authoring && (props.authorName || props.schemaLabel)">
+ {{ props.authorName || props.schemaLabel }}
+</label>
Review Comment:
The wrapper clears Formly's original label, but this replacement `<label>`
has no association with the generated control. As a result, a reader's input
loses its accessible name even though a visual label is present. Bind the
label's `for` attribute to the wrapped field ID.
##########
frontend/src/app/dashboard/component/user/list-item/list-item.component.html:
##########
@@ -194,6 +195,17 @@
nz-icon
nzType="eye"></i>
</button>
+ <button
+ *ngIf="entry.type==='workflow' && config.env.formViewEnabled"
+ nz-button
+ nzType="text"
+ [title]="defaultsToForm ? 'Open on the canvas by default' : 'Open in the
Form View by default'"
+ [class.defaults-to-form-on]="defaultsToForm"
+ (click)="onToggleDefaultView()">
Review Comment:
This action is shown for every workflow in private search, including entries
with READ access, but the backend endpoint requires WRITE access
(`WorkflowResource.scala:760-762`). Read-only collaborators therefore get a
control that can only fail with a 403. Gate it on `entry.accessLevel ===
'WRITE'`, as other editable workflow actions do.
##########
frontend/src/app/workspace/component/menu/menu.component.scss:
##########
@@ -210,8 +211,63 @@ texera-coeditor-user-icon {
}
}
-.jupyter-notebook-icon {
- height: 1.1em;
- width: auto;
- vertical-align: -0.2em;
+/* One workflow, two ways of working on it. Rendered identically in the
operator canvas
Review Comment:
This replacement removes the `.jupyter-notebook-icon` sizing rule, although
the menu still uses that class at `menu.component.html:170`. The SVG will now
render at its intrinsic dimensions and lose its prior vertical alignment.
Preserve the existing rule alongside the new view-switch styles.
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -622,6 +622,24 @@ export class MenuComponent implements OnInit, OnDestroy {
saveAs(new Blob([workflowContentJson], { type: "text/plain;charset=utf-8"
}), fileName);
}
+ /**
+ * Open the Form View -- a full page load, not a route: the two views share
root-level
+ * singletons (graph, Yjs shared model), and routing left the old
collaboration client
+ * alive (you appeared as your own coeditor). A fresh document is the clean
handover.
+ */
+ public onClickOpenFormView(): void {
+ const wid = this.workflowActionService.getWorkflowMetadata().wid;
+ if (wid !== undefined) {
+ // Persist before the full-page nav, matching the form's
openRegularCanvas (which saves
+ // first). The workspace's beforeunload handler also persists, but
saving here shrinks the
+ // race between an unsaved debounced edit and the unload so the other
view opens up to date.
+ this.persistWorkflow();
+ /* v8 ignore start -- full-document navigation; jsdom cannot navigate */
+ window.location.href = `${USER_WORKSPACE}/${wid}/form`;
Review Comment:
`persistWorkflow()` starts an asynchronous HTTP request and returns
immediately, so assigning `location.href` in the next statement can unload the
document and abort the request. This does not guarantee the promised “persists
before navigating” behavior for the exact unsaved edit this switch is intended
to preserve. Navigate from the persist observable's successful completion (and
handle failure) instead.
##########
frontend/src/app/dashboard/component/user/list-item/list-item.component.ts:
##########
@@ -163,6 +174,46 @@ export class ListItemComponent implements OnChanges {
}
}
+ /**
+ * A workflow opens in its default view. A form-default one is marked with
the Form View icon
+ * and its owner's card deep-links straight into the form; the operator
canvas is still one
+ * click away from there. A canvas-default one is left as the descriptor set
it. Hub links are
+ * untouched; only the owner's own entry point moves.
+ */
+ private applyDefaultView(): void {
+ if (this.entry.type !== "workflow" || !this.config.env.formViewEnabled) {
+ return;
+ }
+ this.defaultsToForm = this.entry.workflow?.workflow?.defaultView ===
DefaultView.FORM;
+ if (this.defaultsToForm) {
+ this.iconType = "solution";
+ }
+ if (this.entryLink[0] === USER_WORKSPACE) {
+ this.entryLink = this.defaultsToForm
+ ? [USER_WORKSPACE, String(this.entry.id), "form"]
+ : [USER_WORKSPACE, String(this.entry.id)];
+ }
+ }
+
+ public onToggleDefaultView(): void {
+ const next = this.defaultsToForm ? DefaultView.CANVAS : DefaultView.FORM;
+ this.workflowPersistService
+ .setDefaultView(this.entry.id as number, next)
Review Comment:
The Form View landing/toggle behavior is implemented only in
`ListItemComponent`, but the dashboard also renders `CardItemComponent`
whenever the persisted view mode is `card`
(`user-workflow.component.html:157-177`). That mode still uses the canvas link
and has no default-view toggle, so the PR does not provide the described
per-card entry point for card-view users. Apply the same behavior to the card
renderer (ideally through shared logic).
##########
frontend/src/app/workspace/component/property-editor/property-editor.component.ts:
##########
@@ -248,7 +276,11 @@ export class PropertyEditorComponent implements OnInit,
OnDestroy, OnChanges {
if (highlightedOperators.length === 1 && highlightLinks.length === 0
&& highlightedPorts.length === 0) {
this.currentComponent = OperatorPropertyEditFrameComponent;
- this.componentInputs = { currentOperatorId: highlightedOperators[0],
exposeChoosing: this.choosing };
+ this.componentInputs = {
+ currentOperatorId: highlightedOperators[0],
+ exposeChoosing: this.choosing,
+ broadcastEditing: this.broadcastEditing,
+ };
Review Comment:
This value becomes stale when Edit is toggled while a step is already
selected. `ngOnChanges` remounts the operator frame for `exposeChoosing`, but
`remountOperatorFrame()` refreshes only that property and reuses the old
`componentInputs.broadcastEditing`; entering Edit can stay non-broadcasting,
while leaving Edit can remount a reader frame with broadcasting enabled.
Refresh `broadcastEditing` during the remount as well.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -86,7 +110,16 @@
nzType="info-circle"
class="lead"
aria-hidden="true"></i>
- <h2>{{ instructionTitle || "How to use this" }}</h2>
+ <h2 *ngIf="!authoring">{{ instructionTitle || "How to use this" }}</h2>
+ <!-- An author edits the heading in place here, not in a separate
Title box. stopPropagation
+ so clicking into it does not collapse the section. -->
+ <input
+ *ngIf="authoring"
+ class="instr-title-input"
+ [(ngModel)]="instructionTitle"
+ (ngModelChange)="onInstructionChange()"
+ (click)="$event.stopPropagation()"
+ placeholder="How to use this" />
Review Comment:
This `<input>` is nested inside the instruction header `<button>`, which is
invalid interactive-content nesting and creates conflicting keyboard/click
semantics for assistive technology. Move the title input outside the toggle
button and associate the toggle with the collapsible panel via
`aria-expanded`/`aria-controls`.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -294,9 +300,53 @@ export class ComputingUnitSelectionComponent implements
OnInit {
selectComputingUnit(wid: number | undefined, cuid: number | undefined): void
{
if (isDefined(cuid) && wid !== DEFAULT_WORKFLOW.wid) {
this.computingUnitStatusService.selectComputingUnit(wid, cuid);
+ this.rememberComputingUnit(wid, cuid);
}
}
+ /**
+ * The live selection lives only in ComputingUnitStatusService, re-derived
on load from the
+ * last execution -- but that only exists once the workflow has run (pick a
unit, reload
+ * before running, and it is gone). Canvas<->Form View switches reload, so
we remember the
+ * last explicit choice per workflow to keep the two views agreeing. One
unit per workflow.
+ */
+ private static computingUnitStorageKey(wid: number): string {
+ return `computing-unit-of-workflow-${wid}`;
+ }
+
+ private rememberComputingUnit(wid: number | undefined, cuid: number): void {
+ if (!isDefined(wid)) {
+ return;
+ }
+ try {
+
localStorage.setItem(ComputingUnitSelectionComponent.computingUnitStorageKey(wid),
String(cuid));
+ } catch {
+ // Private browsing or a full quota; remembering is an optimisation, not
a
+ // requirement -- the last-execution lookup still applies on the next
load.
+ }
+ }
+
+ private recallComputingUnit(wid: number): number | undefined {
+ let stored: string | null = null;
+ try {
+ stored =
localStorage.getItem(ComputingUnitSelectionComponent.computingUnitStorageKey(wid));
+ } catch {
+ return undefined;
+ }
+ // A cuid is a positive integer. Number() would also accept "0" and "1.5",
and handing
+ // either on would mean chasing a unit that cannot exist.
+ const cuid = Number(stored);
+ if (!stored || !Number.isInteger(cuid) || cuid <= 0) {
+ return undefined;
+ }
+ // A remembered unit that has since been terminated must not win over the
fallbacks,
+ // but an empty list means the units have not arrived yet rather than that
it is gone.
+ if (this.allComputingUnits.length > 0 && !this.allComputingUnits.some(u =>
u.computingUnit.cuid === cuid)) {
+ return undefined;
Review Comment:
Treating an empty unit list as validation success can strand the selector on
a stale remembered ID. At startup the list is initially empty;
`selectComputingUnit` then waits only for that ID to appear, and if the unit
was terminated it never selects anything and the latest-execution/running-unit
fallback is permanently skipped. Wait for the unit list to load before
validating the remembered ID, then run the existing fallback when it is absent.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -446,14 +526,69 @@ 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:
This eagerly deletes bindings whose operator is gone before
`resolveFields()` can expose them, so authors never receive the removable
broken cards described by this PR; merely entering Edit permanently prunes
those bindings. Keep broken bindings in author mode and only filter them from
the reader view, or make deletion an explicit author action.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -99,45 +132,149 @@ <h2>{{ instructionTitle || "How to use this" }}</h2>
[hidden]="!instructionOpen">
<div
class="md"
+ *ngIf="!authoring"
[innerHTML]="instructionPreviewHtml"></div>
+
+ <ng-container *ngIf="authoring">
+ <div class="tabs">
+ <button
+ type="button"
+ [attr.aria-current]="instructionMode === 'write'"
+ (click)="setInstructionMode('write')">
+ Write
+ </button>
+ <button
+ type="button"
+ [attr.aria-current]="instructionMode === 'preview'"
+ (click)="setInstructionMode('preview')">
+ Preview
+ </button>
+ </div>
+
+ <ng-container *ngIf="instructionMode === 'write'">
+ <textarea
+ class="md-input"
+ [(ngModel)]="instructionBody"
+ (ngModelChange)="onInstructionChange()"
+ placeholder="Explain what this does and what to fill
in."></textarea>
+ <p class="hint">Markdown. Add a picture with
<code></code>.</p>
+ </ng-container>
+
+ <div
+ class="md"
+ *ngIf="instructionMode === 'preview'"
+ [innerHTML]="instructionPreviewHtml"></div>
+ </ng-container>
</div>
</section>
<!-- The inputs an author exposed, each rendered as its operator's own
field. -->
<div class="pc-section-head">
<span class="label">Inputs</span>
+ <span
+ class="hint"
+ *ngIf="authoring"
+ >Drag to reorder. Click a step in the workflow to add more</span
+ >
</div>
<div
class="empty"
*ngIf="visibleFields.length === 0">
- This workflow has no inputs to fill in.
+ {{ authoring ? "No inputs yet. Open the workflow below and click a step
to expose its settings." : "This workflow
+ has no inputs to fill in." }}
</div>
- <div class="params">
+ <div
+ class="params"
+ cdkDropList
+ (cdkDropListDropped)="onDrop($event)">
<section
class="card param"
[class.read-only]="!canEdit"
- *ngFor="let r of rendered; trackBy: trackByRendered">
- <!-- The operator's own field, so a file property gets the real file
picker and an
- attribute property a column dropdown. -->
- <form
- [formGroup]="r.form"
- class="param-form">
- <formly-form
- [model]="r.model"
- [fields]="r.fields"
- [form]="r.form"></formly-form>
- </form>
+ *ngFor="let r of rendered; trackBy: trackByRendered"
+ cdkDrag
+ [cdkDragDisabled]="!authoring">
+ <!-- Author row: the drag handle and the "From <step>" provenance
line. Hidden for a broken
+ input, whose operatorLabel is the raw id; the broken card below
says what happened. -->
+ <div
+ class="field-top"
+ *ngIf="authoring">
+ <i
+ nz-icon
+ nzType="holder"
+ class="grip"
+ cdkDragHandle
+ aria-hidden="true"></i>
Review Comment:
Reordering is exposed only through an `aria-hidden` drag handle, so
keyboard-only users cannot reorder inputs. CDK drag/drop does not provide
keyboard reordering automatically; add focusable Move up/Move down controls (or
equivalent keyboard handling) that invoke the same reorder operation.
--
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]
