(texera) 01/02: feat(frontend): remove an operator property when its field is cleared (#7584)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git

commit 7db229c41f9a28b8e6ca6c107dd21e820e03dbb3
Author: Kary Zheng <15...@users.noreply.github.com>
AuthorDate: Wed Sep 2 00:53:52 2026 +0000

    feat(frontend): remove an operator property when its field is cleared (#7584)
    
    ### What changes were proposed in this PR?
    
    Clearing a text or number field in the property panel left the box empty
    while the operator kept its previous value, so the workflow ran on a
    value the user had removed and could no longer see. Setting CSV File
    Scan's Limit to 5 and clearing it left the scan reading 5 rows, and the
    value came back on the next reload.
    
    A cleared field is simply not among the properties the panel sends:
    formly drops a cleared text field from the model, and a cleared number
    arrives as `null`, which `onFormChanges` filters out.
    `updateYTypeFromObject` updates a value in place or sets a new one, and
    for a key the new object no longer carries it did neither, so the old
    entry stayed in the Y.Map. It now deletes such a key, deciding on
    membership rather than on the value so that an explicit `undefined`
    still takes the update path.
    
    Clearing a property that has a schema default now shows that default
    when the panel is next rendered; one without a default stays empty.
    
    ### Any related issues, documentation, discussions?
    
    Closes #7394
    
    The issue also read this as evidence that an empty string cannot be
    stored from the UI. That part has been withdrawn: an absent property
    deserializes to its Scala field initializer, which already gives an
    empty value where one is meaningful.
    
    ### How was this PR tested?
    
    The test that pinned the old behaviour is updated, since it described
    what the function did rather than what it should do, and
    `setOperatorProperty` gains a case that fails without this change. The
    specs that write operator properties pass together, 465 cases.
    
    The issue's reproduction was run before and after. Delimiter set to `X`
    and cleared came back as `X`; it now leaves `,`, and Limit set to 5 and
    cleared stays empty.
    
    ### Was this PR authored or co-authored using generative AI tooling?
    
    Generated-by: Claude Code (Claude Opus 5)
    
    Co-authored-by: Claude Opus 5 (1M context) <no...@anthropic.com>
---
 .../workflow-graph/model/workflow-graph.spec.ts      | 14 ++++++++++++++
 .../workspace/types/shared-editing.interface.spec.ts | 19 ++++++++++++++++---
 .../app/workspace/types/shared-editing.interface.ts  | 20 +++++++++++++++-----
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
index 2e1e6b2ef2..41942cd7a5 100644
--- a/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
+++ b/frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.spec.ts
@@ -199,6 +199,20 @@ describe("WorkflowGraph", () => {
     expect(operator.operatorProperties).toEqual(testProperty);
   });
 
+  // The property panel clears a field by sending the properties without it, so a
+  // key left out has to be dropped here. Retaining it leaves the operator running
+  // on a value the user removed and the panel no longer shows.
+  it("should drop a property the new object leaves out", () => {
+    workflowGraph.addOperator(mockScanPredicate);
+    workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable", limit: 5 });
+
+    workflowGraph.setOperatorProperty(mockScanPredicate.operatorID, { tableName: "testTable" });
+
+    expect(workflowGraph.getOperator(mockScanPredicate.operatorID).operatorProperties).toEqual({
+      tableName: "testTable",
+    });
+  });
+
   it("should throw an error when trying to set the property of an nonexist operator", () => {
     expect(() => {
       const testProperty = { tableName: "testTable" };
diff --git a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
index 40141d0006..83af7d2fda 100644
--- a/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
+++ b/frontend/src/app/workspace/types/shared-editing.interface.spec.ts
@@ -142,7 +142,7 @@ describe("updateYTypeFromObject", () => {
     expect(originalText.toJSON()).toBe("world");
   });
 
-  it("adds new object keys but leaves keys absent from the new object in place", () => {
+  it("adds new object keys and removes the ones the new object no longer carries", () => {
     const doc = new Y.Doc();
     const yMap = attach(doc, "m", createYTypeFromObject({ keep: 1, drop: 2 }));
 
@@ -153,8 +153,21 @@ describe("updateYTypeFromObject", () => {
     // A key present only in the new object is added.
     expect(asMap.has("add")).toBe(true);
     expect(asMap.get("add")).toBe(3);
-    // A key omitted from the new object is retained (an undefined new value never deletes).
-    expect(asMap.get("drop")).toBe(2);
+    // A key the new object has dropped is deleted rather than left behind. Clearing
+    // an operator property is exactly this: the editor sends the properties without
+    // it, and anything retained here is a value the user removed and can no longer see.
+    expect(asMap.has("drop")).toBe(false);
+  });
+
+  it("keeps a key whose new value is explicitly undefined", () => {
+    const doc = new Y.Doc();
+    const yMap = attach(doc, "m", createYTypeFromObject({ a: 1 }));
+
+    updateYTypeFromObject(yMap, { a: undefined } as any);
+
+    // Carrying the key with an undefined value is not the same as dropping it, and
+    // only the latter means removal.
+    expect((yMap as unknown as Y.Map<any>).get("a")).toBe(1);
   });
 
   it("appends new items to a Y.Array in place", () => {
diff --git a/frontend/src/app/workspace/types/shared-editing.interface.ts b/frontend/src/app/workspace/types/shared-editing.interface.ts
index fc7843992d..f68beef1e4 100644
--- a/frontend/src/app/workspace/types/shared-editing.interface.ts
+++ b/frontend/src/app/workspace/types/shared-editing.interface.ts
@@ -246,11 +246,21 @@ export function updateYTypeFromObject<T extends object>(oldYObj: YType<T>, newOb
     const keySet = new Set([...Object.keys(oldObj), ...Object.keys(newObj)]);
     keySet.forEach((k: string) => {
       const newValue = newObj[k as keyof T] as any;
-      if (!_.isEqual(oldObj[k as keyof T], newValue)) {
-        if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) {
-          if (newValue !== undefined) {
-            oldYObjAsYMap.set(k, createYTypeFromObject(newValue));
-          }
+      if (_.isEqual(oldObj[k as keyof T], newValue)) {
+        return;
+      }
+      // A key the new object does not carry is one the caller removed, and deleting
+      // it is the only way that removal can reach the shared type: the update below
+      // changes a value or adds one, and for a key that is simply gone it did
+      // neither, leaving the old value in place. `in` rather than an undefined check,
+      // so a key explicitly carrying undefined still takes the path below.
+      if (!(k in newObj)) {
+        oldYObjAsYMap.delete(k);
+        return;
+      }
+      if (!updateYTypeFromObject(oldYObjAsYMap.get(k), newValue)) {
+        if (newValue !== undefined) {
+          oldYObjAsYMap.set(k, createYTypeFromObject(newValue));
         }
       }
     });