Copilot commented on code in PR #8839:
URL: https://github.com/apache/texera/pull/8839#discussion_r4319600777
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -563,6 +563,32 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
*/
@HostListener("focusout")
public onFocusOut(): void {
+ this.flushHeldRebuild();
+ }
+
+ /**
+ * A click or a key also ends a hold, because a focus change is not the only
thing that does:
+ * closing an nz-select leaves the focus in its search box (ng-zorro
re-focuses it), so no focusout
+ * fires, and a rebuild held while the list was open would have waited for
an unrelated event
+ * (Copilot on #8839). The same later() re-check as below decides it after
the current tick, by
+ * when the dropdown has lost its open class.
+ *
+ * The click is listened for on the document, not on this host. A dropdown's
options are drawn in
+ * the CDK's overlay container, a sibling of the whole app rather than a
descendant of this page,
+ * so the click that picks one -- or that picks the option already chosen,
which changes no value
+ * and moves no focus -- never bubbles through here (Copilot). A click
anywhere else ends the hold
+ * for the same reason a focusout does: the reader is no longer typing into
the card. The handler
+ * returns at once unless a rebuild is actually held. Keys stay on the host:
what is typed goes
+ * into controls this page owns (nz-select keeps its search box in the
trigger, not the overlay),
+ * and a document-wide key listener would run on every keystroke in the
workspace.
+ */
+ @HostListener("document:click")
+ @HostListener("keyup")
+ public onInteraction(): void {
+ this.flushHeldRebuild();
+ }
+
+ private flushHeldRebuild(): void {
if (this.rebuildDeferred) {
this.later(() => {
if (this.rebuildDeferred) {
Review Comment:
When `rebuildDeferred` is true, every `keyup` will schedule a `later()`
callback. During sustained typing this can enqueue many redundant deferred
checks (each still runs and calls into the typing guard later). Recommendation
(optional but strongly suggested): coalesce/single-flight these checks (e.g.,
keep a `flushScheduled` boolean or store/clear a pending timer) so only one
deferred re-check is queued at a time while a rebuild is held.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -1006,22 +1042,44 @@ 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,
};
Review Comment:
This hunk removes the previous `field.key = binding.id` assignment, but does
not set `field.key` to `binding.propertyKey` (despite the comment/PR intent).
If `field.key` is left undefined or relies on jsonschema defaults, widgets that
look up controls by key and tests expecting a deterministic key can break for
schemas that don’t provide a key. Recommendation (mandatory): explicitly assign
`field.key = binding.propertyKey` here to guarantee consistent behavior with
the panel.
##########
frontend/src/app/workspace/service/form-binding/form-binding.service.ts:
##########
@@ -209,13 +209,23 @@ export class FormBindingService {
* why a run started from either one behaves identically.
*/
public writeValue(binding: FormFieldBinding, value: unknown): void {
- const operator = this.getOperator(binding.operatorID);
+ this.writeProperties(binding.operatorID, { [binding.propertyKey]: value });
+ }
+
+ /**
+ * Write several properties of one operator at once, over its current ones:
what a card's widget
+ * changed, which is the bound property and whatever siblings it touches the
way it does on the
+ * operator property panel (the HuggingFace picker resets the task-scoped
fields on a task
+ * change). One edit, as the panel makes one.
+ */
+ public writeProperties(operatorID: string, patch: Record<string, unknown>):
void {
+ const operator = this.getOperator(operatorID);
if (!operator) {
return;
}
- this.workflowActionService.setOperatorProperty(binding.operatorID, {
+ this.workflowActionService.setOperatorProperty(operatorID, {
...operator.operatorProperties,
- [binding.propertyKey]: value,
+ ...patch,
});
Review Comment:
Spreading `operator.operatorProperties` and `patch` into a plain object
literal can be vulnerable to prototype-pollution style keys (e.g., `__proto__`,
`constructor`, `prototype`) if operator properties / schema-derived keys can be
influenced by imported workflow content or other untrusted inputs.
Recommendation (mandatory if any untrusted input can reach these keys):
sanitize/disallow those special keys before merging, or merge into an object
created with a null prototype to avoid triggering the `__proto__` setter.
--
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]
