feat(frontend): remove an operator property when its field is cleared - #7584
Open
kz930 wants to merge 1 commit into
Open
feat(frontend): remove an operator property when its field is cleared#7584kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
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 apache#7394 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
Author
|
@aglinxinyuan @carloea2 May you take a look at it? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7584 +/- ##
=========================================
Coverage 87.87% 87.87%
Complexity 4275 4275
=========================================
Files 1176 1176
Lines 46890 46894 +4
Branches 5228 5229 +1
=========================================
+ Hits 41205 41209 +4
Misses 3961 3961
Partials 1724 1724
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, whichonFormChangesfilters out.updateYTypeFromObjectupdates 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 explicitundefinedstill 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
setOperatorPropertygains 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
Xand cleared came back asX; 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)