feat(frontend): add Python UDF UI parameter form support#5043
feat(frontend): add Python UDF UI parameter form support#5043carloea2 wants to merge 19 commits into
Conversation
github-actions
Bot
added
dependencies
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5043 +/- ##
============================================
+ Coverage 43.16% 43.32% +0.15%
Complexity 2212 2212
============================================
Files 1045 1049 +4
Lines 40363 40547 +184
Branches 4268 4318 +50
============================================
+ Hits 17423 17567 +144
- Misses 21866 21890 +24
- Partials 1074 1090 +16
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@carloea2 How do I test the features of this PR? In general, just relying on unit tests is not enough, and we should explicitly explain how a new feature can be tested manually. |
|
Hi @Xiao-zhen-Liu, this PR cannot be manually tested yet because it only adds the components that will be used later to update the UI, specifically the Parser and Sync Services. I intentionally kept this PR isolated to reduce the number of lines changed. The Formly components are tightly coupled to the operator schemas, so introducing them will require backend changes and user-facing updates that will only work once all four PRs are merged. |
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Thanks for splitting this off cleanly, @carloea2. The parser tests are well written and the overall scope is small enough to review in one sitting. Inline comments are left on specific lines. Most of them are foundation issues that will pay off when PRs #2 to #4 are built on top, so it is worth addressing them here rather than later. A few notes that do not belong on a specific line:
PR description: The "Type definitions" row in the table ("Extends frontend workflow-compiling types to represent UI parameter metadata") is confusing. UiUdfParameter is actually defined inline in the parser service file, and workflow-compiling.interface.ts is not modified. This row would be clearer if either the interface gets moved into workflow-compiling.interface.ts (since it is shared between the two services), or the row is rephrased to describe what was actually added.
Test gaps to fill:
- Parser: subclass case (see comment on
CLASSES), multiple classes in one file, empty arg list, extra positional arg, duplicate parameter names (currently silently deduped). - Sync service:
attachToYCodehappy path (subscribe, mutate YText, observe emission, call cleanup), the operator-type filter, the path wherecodeFromEditoris omitted. - Component: there is no spec at all. A minimal test asserting that name and type fields are disabled and the value field is editable would catch the next refactor accident.
On manual testing: Since this PR does not affect any user-facing behavior, shipping it without a manual testing flow is fine. For future PRs in this stack, please think about including a small dev-only setup (a hidden route, an internal test page, a minimal example schema) so reviewers can exercise the new piece in isolation. It is much easier to give good feedback when the new component can be seen rendering.
|
@Xiao-zhen-Liu thanks, I will ping you when I have the fixes ready. |
This reverts commit 99bb449.
…arameter-frontend
|
@Xiao-zhen-Liu It is ready for the next review pass. Thanks. |
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Thanks @carloea2, the rewrite is a real improvement. All previous comments are addressed, and the new error-surfacing design with UiUdfParametersParseError plus the expanded test coverage for both services are nice additions beyond what was asked. Three small things below that do not need to block this PR. Approving.
| ngOnInit(): void { | ||
| this.field.fieldGroup?.forEach(rowField => { | ||
| this.fieldColumns.forEach(column => { | ||
| this.configureDisabledState(this.getColumnField(rowField, column), column.disabled); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The disable logic runs in ngOnInit, which only iterates the rows that exist at component init time. When PR #2 wires the uiParametersChanged$ subscriber and writes back into operatorProperties.uiParameters, FieldArrayType will generate new rows from this.field.fieldArray (the row template). Those new rows do not go through ngOnInit, and the onInit hooks registered inside configureDisabledState are attached to the existing field configs, not to the template. New rows would then render with Name and Type editable.
This is not visible today because no subscriber exists in this PR. But it will be a regression as soon as PR #2 lands. Two cleaner options: (1) set disabled: true directly on field.fieldArray.fieldGroup entries for Name and Type so every new row inherits the state from the template; or (2) override FieldArrayType.onPopulate so the disable logic runs whenever rows are populated.
The component spec also only constructs a static field.fieldGroup and never adds rows, so this case is not exercised. Please either pick a fix now or add a spec that adds a row after ngOnInit and asserts the new row's Name and Type are disabled, so this regression is caught when PR #2 lands.
| const operatorProperties = sharedOperatorType.get("operatorProperties") as any; | ||
| const yCode = operatorProperties.get("code") as YText; | ||
| return yCode?.toString(); | ||
| } catch { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Two small things in this helper. (1) The catch {} block silently returns undefined for every error type, which could hide real bugs during shared-editing debugging. A console.warn on the caught error would help. (2) sharedOperatorType.get("operatorProperties") as any was previously typed as as YType<Readonly<{ [key: string]: any }>>. Restoring that (or a tighter type) keeps the editor-to-graph boundary type-safe.
| throw error; | ||
| } | ||
|
|
||
| this.uiParametersParseErrorSubject.next({ operatorId }); |
There was a problem hiding this comment.
This emits { operatorId, message: undefined } to clear errors, so subscribers have to check message to distinguish "no error" from "error". This works, but reads a bit awkwardly. When PR #2 consumes this, a clearParseError(operatorId) method (or a separate uiParametersParseErrorCleared$ observable) would express the intent more directly.
|
@carloea2 one more pass before merging: the new code (parser service, sync service, and component) has very little documentation beyond the license header. For a foundation PR that PRs #2 to #4 will build on top of, the public contracts need to be readable without diving into method bodies. Please add JSDoc on: Parser service
Sync service
Component
The bodies are well-written and not hard to follow, but the next person extending this code should not have to read every function to understand what each promises. This does not need to be exhaustive: one or two sentences per public surface is enough. |
3 participants
What changes were proposed in this PR?
This PR adds frontend building blocks for Python UDF UI parameters.
It introduces:
self.UiParameter(...)declarations.ui-udf-parametersFormly component for displaying parameter rows.UiUdfParametershape in the parser service and imports it from the sync service.@lezer/python@1.1.18for Python syntax parsing. This dependency is MIT licensed.This PR does not complete the end-to-end feature by itself. The parser/sync utilities and Formly field type are added here, but full wiring with backend operator descriptors and runtime injection is handled by later PRs in the stack.
Any related issues, documentation, discussions?
Part of the Python UDF UI parameter feature split from
feat/ui-parameter.#5044
Intended stack order:
This PR introduces:
sequenceDiagram autonumber participant Code as Python UDF Code participant Parser as UiUdfParametersParserService participant Sync as UiUdfParametersSyncService participant Type as UiUdfParameter type participant Component as UiUdfParametersComponent participant Formly as Formly Config participant Later as Later PRs Code->>Parser: self.UiParameter(...) declarations Parser-->>Type: name/type/value metadata Parser-->>Sync: inferred parameter structure Sync->>Sync: preserve existing edited values Component->>Formly: register ui-udf-parameters field type Type-->>Later: available for descriptor wiring Formly-->>Later: field can be used when schema exposes it Note over Parser,Formly: This PR adds frontend building blocks only. Note over Later: Rendering in the operator panel depends on later backend/schema wiring.How was this PR tested?
Added frontend unit tests for the parser, sync service, and Formly component.
Commands run:
Manual UI testing is not available in this PR alone because the backend/operator schema wiring that makes the field visible in the operator panel is intentionally handled in later PRs.
Was this PR authored or co-authored using generative AI tooling?
No