Copilot commented on code in PR #8437:
URL: https://github.com/apache/texera/pull/8437#discussion_r3941692539
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -175,6 +240,149 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
this.workflowActionService.disableWorkflowModification();
}
+ //
---------------------------------------------------------------------------
+ // Inputs: the exposed properties, rendered as their operators' own fields
+ //
---------------------------------------------------------------------------
+
+ /** Whether the cursor is currently inside one of this page's inputs. */
+ private isTypingInTheForm(): boolean {
+ const active = document.activeElement as HTMLElement | null;
+ if (!active || !this.host.nativeElement.contains(active)) {
+ return false;
+ }
+ return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) ||
active.isContentEditable;
+ }
+
+ private readConfig(): void {
+ this.parameters = this.formBindingService.resolveFields();
+ this.buildForm();
+ }
+
+ /**
+ * Build the form from the operators' JSON schemas (FormlyJsonschema),
keeping the one field per
+ * exposed property. Each input gets its own form keyed by binding id.
+ */
+ private buildForm(): void {
+ this.formsRebuilt.next();
+ this.rendered = this.visibleFields
+ .map(field => this.renderField(field))
+ .filter((r): r is RenderedField => r !== undefined);
+ }
+
+ private renderField(resolved: ResolvedField): RenderedField | undefined {
+ const { binding } = resolved;
+ const schema = this.operatorSchemaFor(binding.operatorID);
+ if (!schema) {
+ return undefined;
+ }
+ const operatorType =
this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)?.operatorType;
+ const full = this.formlyJsonschema.toFieldConfig(cloneDeep(schema) as
never, {
+ map: (mapped, source) => {
+ // Render the exact custom widget the operator property panel would
(file/model/dataset
+ // pickers, image/audio uploaders, ...), shared via
customFormlyFieldType so an exposed
+ // property shows its real control instead of degrading to a text box.
+ const customType = customFormlyFieldType({
+ key: mapped.key,
+ operatorType,
+ description: (source as { description?: string })?.description,
+ currentType: mapped.type,
+ });
+ // Canvas-only widgets (code editor, drag-reorder) do not work here;
an older workflow may
+ // already carry one, so leave it to formly's default editable control
rather than a widget
+ // that cannot function on a form.
+ if (customType && !CANVAS_ONLY_FORMLY_TYPES.has(customType)) {
+ mapped.type = customType;
+ }
+ return mapped;
+ },
+ });
+ const source = (full.fieldGroup ?? []).find(child => child.key ===
binding.propertyKey);
+ if (!source) {
+ return undefined;
+ }
+
+ const field = cloneDeep(source);
+ // The schema's own title ("Attributes", "Limit", "File") -- the reader's
title when unnamed.
+ // Falls back to this, not the lower-camel key ("fileName"), which would
read inconsistently.
+ const schemaLabel = (source.props?.label as string) || binding.propertyKey;
+ field.key = binding.id;
+ field.props = {
+ ...(field.props ?? {}),
+ label: binding.displayName || schemaLabel,
+ // The schema's own description is the operator author's note to whoever
wired the operator
+ // up; it is not guidance to a form reader, and formly shows it once per
scalar field. Drop it
+ // here so it does not appear unbidden under the input.
+ description: "",
+ };
+
+ const form = new FormGroup({});
+ const model: Record<string, unknown> = { [binding.id]:
cloneDeep(resolved.value) };
Review Comment:
The isolated model omits operator context required by the `huggingface`
field type. `HuggingFaceComponent` reads and writes `model.task` and sibling
controls such as `modelId` (`hugging-face.component.ts:538-579, 639-679`), but
this model contains only the renamed binding-id key. Consequently an exposed
`modelId` opens on the default `text-generation` task rather than the
operator's task, and changing the widget's task is not written back, allowing
an incompatible model/task pair. This widget needs a Form View mode/context
that preserves the operator task while exposing only the bound property.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -121,6 +162,28 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
}
this.wid = wid;
this.load(wid);
+
+ // Attribute boxes become dropdowns only after compilation writes the
column enums into each
+ // operator's dynamic schema -- which lands after these cards were built.
Rebuild on the
+ // compilation-state stream, a ReplaySubject(1) so a late subscriber (this
page reloads fresh
+ // on every Canvas<->Form switch) gets the current state at once. Skip it
while someone is
+ // typing, so a rebuild does not throw away a half-entered value under the
cursor.
+ this.workflowCompilingService
+ .getCompilationStateInfoChangedStream()
+ .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this))
+ .subscribe(() => {
+ if (this.isTypingInTheForm()) {
+ return;
Review Comment:
Returning here permanently drops this compilation update; `ReplaySubject(1)`
does not emit again when focus later leaves the field. If this was the update
that supplied upstream columns, the attribute control remains stale until some
unrelated compilation occurs. Record a pending rebuild and execute it on blur
instead of discarding the event.
This issue also appears on line 183 of the same file.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -175,6 +240,149 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
this.workflowActionService.disableWorkflowModification();
}
+ //
---------------------------------------------------------------------------
+ // Inputs: the exposed properties, rendered as their operators' own fields
+ //
---------------------------------------------------------------------------
+
+ /** Whether the cursor is currently inside one of this page's inputs. */
+ private isTypingInTheForm(): boolean {
+ const active = document.activeElement as HTMLElement | null;
+ if (!active || !this.host.nativeElement.contains(active)) {
+ return false;
+ }
+ return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) ||
active.isContentEditable;
+ }
+
+ private readConfig(): void {
+ this.parameters = this.formBindingService.resolveFields();
+ this.buildForm();
+ }
+
+ /**
+ * Build the form from the operators' JSON schemas (FormlyJsonschema),
keeping the one field per
+ * exposed property. Each input gets its own form keyed by binding id.
+ */
+ private buildForm(): void {
+ this.formsRebuilt.next();
+ this.rendered = this.visibleFields
+ .map(field => this.renderField(field))
+ .filter((r): r is RenderedField => r !== undefined);
+ }
+
+ private renderField(resolved: ResolvedField): RenderedField | undefined {
+ const { binding } = resolved;
+ const schema = this.operatorSchemaFor(binding.operatorID);
+ if (!schema) {
+ return undefined;
+ }
+ const operatorType =
this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)?.operatorType;
+ const full = this.formlyJsonschema.toFieldConfig(cloneDeep(schema) as
never, {
+ map: (mapped, source) => {
+ // Render the exact custom widget the operator property panel would
(file/model/dataset
+ // pickers, image/audio uploaders, ...), shared via
customFormlyFieldType so an exposed
+ // property shows its real control instead of degrading to a text box.
+ const customType = customFormlyFieldType({
+ key: mapped.key,
+ operatorType,
+ description: (source as { description?: string })?.description,
+ currentType: mapped.type,
+ });
+ // Canvas-only widgets (code editor, drag-reorder) do not work here;
an older workflow may
+ // already carry one, so leave it to formly's default editable control
rather than a widget
+ // that cannot function on a form.
+ if (customType && !CANVAS_ONLY_FORMLY_TYPES.has(customType)) {
+ mapped.type = customType;
+ }
+ return mapped;
+ },
+ });
+ const source = (full.fieldGroup ?? []).find(child => child.key ===
binding.propertyKey);
+ if (!source) {
+ return undefined;
+ }
+
+ const field = cloneDeep(source);
+ // The schema's own title ("Attributes", "Limit", "File") -- the reader's
title when unnamed.
+ // Falls back to this, not the lower-camel key ("fileName"), which would
read inconsistently.
+ const schemaLabel = (source.props?.label as string) || binding.propertyKey;
+ field.key = binding.id;
+ field.props = {
+ ...(field.props ?? {}),
+ label: binding.displayName || schemaLabel,
+ // The schema's own description is the operator author's note to whoever
wired the operator
+ // up; it is not guidance to a form reader, and formly shows it once per
scalar field. Drop it
+ // here so it does not appear unbidden under the input.
+ description: "",
+ };
+
+ const form = new FormGroup({});
+ const model: Record<string, unknown> = { [binding.id]:
cloneDeep(resolved.value) };
+ if (this.canEdit) {
+ form.valueChanges
+ .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS),
takeUntil(this.formsRebuilt), untilDestroyed(this))
+ .subscribe(() => {
+ // Formly emits the schema's empty default while building the
control, before any edit;
+ // writing that back silently wiped the operator's real value (both
views edit one
+ // workflow). So only accept a dirtied form, or a value that differs
from the operator's
+ // without being emptier (some controls set values without marking
dirty).
+ const next = model[binding.id];
+ const current =
this.formBindingService.readValue(binding.operatorID, binding.propertyKey);
+ const isEmpty = (v: unknown) => v === undefined || v === null || v
=== "";
+ const unchanged = JSON.stringify(next ?? null) ===
JSON.stringify(current ?? null);
+ if (unchanged || (!form.dirty && isEmpty(next) &&
!isEmpty(current))) {
+ return;
+ }
+ // Write straight onto the operator (the same edit the canvas makes)
and refresh this
+ // card's snapshot, which the template reads.
+ this.formBindingService.writeValue(binding, next);
+ this.parameters = this.formBindingService.resolveFields();
+ const refreshed = this.parameters.find(p => p.binding.id ===
binding.id);
+ const card = this.rendered.find(r => r.resolved.binding.id ===
binding.id);
+ if (refreshed && card) {
+ card.resolved = refreshed;
+ }
+ this.cdr.detectChanges();
+ });
+ } else {
+ // A read-only viewer sees the author's values and can run with them,
but cannot change them:
+ // disable the control so it renders non-editable, and wire no
write-back at all.
+ form.disable();
Review Comment:
Disabling this empty `FormGroup` does not make all rendered custom widgets
read-only. Several selected widgets expose controls outside the bound
FormControl—for example, dataset selector buttons and Hugging Face file
inputs/task selector—and their templates do not consult the form's disabled
state, so a read-only viewer can still interact with them. Propagate a
read-only flag into every custom widget (or render a non-interactive
representation) and verify the actual rendered controls, not only
`form.disabled`.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -121,6 +162,28 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
}
this.wid = wid;
this.load(wid);
+
+ // Attribute boxes become dropdowns only after compilation writes the
column enums into each
+ // operator's dynamic schema -- which lands after these cards were built.
Rebuild on the
+ // compilation-state stream, a ReplaySubject(1) so a late subscriber (this
page reloads fresh
+ // on every Canvas<->Form switch) gets the current state at once. Skip it
while someone is
+ // typing, so a rebuild does not throw away a half-entered value under the
cursor.
+ this.workflowCompilingService
+ .getCompilationStateInfoChangedStream()
+ .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this))
+ .subscribe(() => {
+ if (this.isTypingInTheForm()) {
+ return;
+ }
+ this.readConfig();
+ });
+
+ // Exposing or un-exposing a property (from the panel, or a co-editor)
changes the definition;
+ // the inputs above have to follow at once, which is the whole point of
editing side by side.
+
this.workflowActionService.formBindingChanged$.pipe(untilDestroyed(this)).subscribe(()
=> {
Review Comment:
This does not receive a co-editor's binding changes. `WorkflowActionService`
explicitly keeps `formBinding` outside the shared model, and
`formBindingChanged$` is only emitted by the local `setFormBinding` call
(`workflow-action.service.ts:108-117, 781-783`). A collaborator exposing or
removing a property therefore leaves this page stale, contrary to the PR's
collaboration behavior; the binding definition needs a real cross-client
synchronization path before this subscription can provide that guarantee.
This issue also appears on line 321 of the same file.
--
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]
