Copilot commented on code in PR #8839:
URL: https://github.com/apache/texera/pull/8839#discussion_r4197169592
##########
frontend/src/app/workspace/component/hugging-face/hugging-face.component.ts:
##########
@@ -200,6 +203,25 @@ export class HuggingFaceComponent extends
FieldType<FieldTypeConfig> implements
);
}
+ /**
+ * The task can change under this widget: a co-editor's edit, or the Form
View's, lands on the
+ * operator and from there on this field's model (the panel hands formly a
fresh model object; the
+ * form card is rebuilt). The selector shows this widget's own state, and on
the panel the schema's
+ * `task` field is hidden, so there is no control to listen to: the model is
checked on every
+ * change detection instead, and the selector follows it as a local pick is
followed -- show the
+ * task, load its models -- rather than keeping what it showed when it was
built.
+ */
+ ngDoCheck(): void {
+ const tag = this.model?.task;
+ if (!this.initialized || typeof tag !== "string" || !tag || tag ===
this.selectedTaskTag) {
+ return;
+ }
+ this.selectedTaskTag = tag;
+ this.searchText = "";
+ this.filteredModels = null;
+ this.loadAllModels();
Review Comment:
On an external task change, `loadAllModels()` can take a
cached/error/in-flight fast path before it cancels this component's previous
request or polling interval. For example, if task A is still loading and task B
is cached, B is shown immediately, then A's late response overwrites the list
while the selector still says B. Cancel the prior loader before any fast path,
or ignore completions whose task is no longer `selectedTaskTag`.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -887,40 +894,67 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
// 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;
+ // The field keeps the property's own key, as on the operator property
panel: a widget that
+ // reaches a sibling or itself by key (the HuggingFace picker resets
`modelId` and its
+ // task-scoped siblings on a task change) must find the same controls and
model keys here.
field.props = {
...(field.props ?? {}),
label: binding.displayName || schemaLabel,
};
const form = new FormGroup({});
- // Seed the model with the operator's other properties as read-only
context, not just this
- // input's own value: some custom widgets read a sibling to decide what to
show -- the
- // HuggingFace model picker reads `task` to load the right models and
label the field. Only
- // this binding's value is ever written back (see below); the context is
never persisted, and
- // it is cloned so a widget that mutates it cannot reach through to the
real operator.
+ // The card's model is the operator's properties, as the panel's is: the
exposed value, and
+ // every other property as context a widget may read or change -- the
HuggingFace model picker
+ // reads `task` to load the right models and, when the task is changed,
resets the task-scoped
+ // siblings as it does on the panel. The copy is cloned so nothing reaches
through to the real
+ // operator; what changes is written back below, as one edit, the way the
panel writes.
const model: Record<string, unknown> = {
...cloneDeep(operator?.operatorProperties ?? {}),
- [binding.id]: cloneDeep(resolved.value),
+ [binding.propertyKey]: cloneDeep(resolved.value),
};
+ // The other properties ride along as plain controls (formly builds only
the exposed field), so
+ // a widget finds its siblings on the card's form exactly as on the
panel's -- the HuggingFace
+ // picker writes `task` through its control, which is also what announces
the change -- and a
+ // control-level write lands in the model, which is what is written back.
+ const schemaKeys = Object.keys((schema as { properties?: Record<string,
unknown> }).properties ?? {});
+ const siblingKeys = [...new Set([...schemaKeys,
...Object.keys(model)])].filter(key => key !== binding.propertyKey);
+ for (const key of siblingKeys) {
+ const control = new FormControl(model[key] ?? null);
+ control.valueChanges.pipe(takeUntil(this.formsRebuilt),
untilDestroyed(this)).subscribe(value => {
+ model[key] = value;
+ });
+ form.addControl(key, control);
+ }
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);
+ // Whatever differs from the operator is written back: the bound
property, and the
+ // siblings a widget changed with it. Formly emits the schema's
empty default for the bound
+ // control while building it, before any edit; writing that back
silently wiped the
+ // operator's real value (both views edit one workflow). So the
bound property is taken
+ // only from a dirtied form, or when it differs without being
emptier (some controls set
+ // values without marking dirty).
+ const current =
(this.workflowActionService.getTexeraGraph().getOperator(binding.operatorID)
+ ?.operatorProperties ?? {}) as Record<string, unknown>;
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))) {
+ // An empty string and an absent property mean the same to an
operator, and formly fills a
+ // blank for a string the operator never had: neither is a change of
the other.
+ const asJson = (value: unknown) => JSON.stringify(value === "" ?
null : value ?? null);
+ const differs = (key: string) => asJson(model[key]) !==
asJson(current[key]);
+ const bound = binding.propertyKey;
+ const blankBeforeEdit = !form.dirty && isEmpty(model[bound]) &&
!isEmpty(current[bound]);
+ const patch = Object.fromEntries(
+ Object.keys(model)
+ .filter(key => differs(key) && !(key === bound &&
blankBeforeEdit))
+ .map(key => [key, model[key]])
Review Comment:
This computes the patch from every difference between the card's build-time
model and the live operator, not from properties changed by this card. If
another card, the canvas, or a co-editor updates any sibling before this card
rebuilds, the next local form emission includes the stale sibling and silently
reverts that newer edit. Track a per-card baseline/local dirty keys (including
the HuggingFace model-only resets) and patch only locally changed keys, then
advance that baseline after a successful write.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -724,6 +724,13 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
if (!active || !this.host.nativeElement.contains(active)) {
return false;
}
+ // A dropdown (nz-select) keeps the focus on its search box after a pick.
That is typing only
+ // while the list is open and a filter can be typed; a closed dropdown
that merely holds the
+ // focus held every rebuild, so a change made elsewhere (the canvas, a
co-editor) never showed.
+ const dropdown = active.closest(".ant-select");
+ if (dropdown) {
+ return dropdown.classList.contains("ant-select-open");
Review Comment:
A rebuild that arrives while this dropdown is open is still stored in
`rebuildDeferred`, but closing `nz-select` keeps focus on the search input, so
`onFocusOut` never runs. Changing this predicate only lets later rebuild
requests through; it does not flush the already-deferred one, leaving the card
stale until an unrelated event or blur. Flush deferred work when the dropdown
closes (and add a test that defers while open, then closes without focusout).
--
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]
