From c6f9c7c95e60e8365798b27a59d9f15e12a9f337 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Tue, 11 Aug 2026 17:24:54 -0700 Subject: [PATCH] feat(frontend): remove an operator property when its field is cleared Clearing a text or number field in the property panel left the box empty on screen while the operator kept the value it held before, 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, with nothing on screen saying so; the value came back on the next reload. The panel is not at fault. It sends the properties it has, and a cleared field is simply not among them: formly drops a cleared text field from the model outright, and a cleared number arrives as null, which onFormChanges filters out. Either way the properties reaching the shared model no longer carry that key. updateYTypeFromObject had no way to act on that. It walks the union of old and new keys and, for each, updates a value in place or sets a new one. For a key the new object no longer carries it did neither: the in-place update refuses an undefined value, and the set that follows is guarded on the value not being undefined. The old entry stayed in the Y.Map, and a removal could not be expressed at all. It now deletes such a key. Membership decides, not the value: a key carried with an explicit undefined still takes the update path, and only a key that is gone is a removal. The test that pinned the old behaviour is updated rather than removed, since it described what the function did rather than what it should do, and setOperatorProperty gains the case that fails without this change. Clearing now does what the panel shows. A property with a schema default comes back as that default when the panel is next rendered, which is one of the two behaviours the issue asks for; one without a default stays empty. Closes #7394 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) --- .../model/workflow-graph.spec.ts | 14 +++++++++++++ .../types/shared-editing.interface.spec.ts | 19 +++++++++++++++--- .../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 2e1e6b2ef2a..41942cd7a5c 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 40141d00060..83af7d2fda4 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).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 fc7843992dc..f68beef1e45 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(oldYObj: YType, 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)); } } });