Repository navigation
feat(workflow-form): render the exposed inputs and write values back - #8437
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8437 +/- ##
==========================================
Coverage 94.03% 94.03%
Complexity 4820 4820
==========================================
Files 1208 1208
Lines 49219 49339 +120
Branches 5982 6001 +19
==========================================
+ Hits 46284 46397 +113
Misses 1464 1464
- Partials 1471 1478 +7
*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:
|
9eb0ea0 to
69df849
Compare
|
👋 Thanks for opening this pull request, @yangzhang75! It looks like the pull request description doesn't quite follow our template yet:
Filling out the template helps reviewers understand and triage your contribution faster. Please edit the description to complete it. This message will disappear automatically once the template is followed. You can find the template prompts by editing the description, or see CONTRIBUTING.md for the full contribution flow. |
|
/request-review: @mengw15 |
There was a problem hiding this comment.
🟡 Changes recommended
Pending edits can be lost, read-only custom widgets remain interactive, and collaboration/schema refresh behavior is incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds schema-driven exposed inputs to Form View and writes edits back to workflow operators.
Changes:
- Builds Formly controls from operator schemas with custom widget support.
- Adds write-back, read-only handling, and rebuild triggers.
- Adds component and rendered-template tests plus input styling.
File summaries
| File | Description |
|---|---|
custom-formly-type.ts |
Defines canvas-only widget types. |
custom-formly-type.spec.ts |
Tests canvas-only classification. |
workflow-form.spec-harness.ts |
Extends mocks for form rendering. |
workflow-form.rendered.spec.ts |
Tests input-section markup. |
workflow-form.component.ts |
Implements rendering, synchronization, and write-back. |
workflow-form.component.spec.ts |
Tests exposed-input behavior. |
workflow-form.component.scss |
Styles input cards and controls. |
workflow-form.component.html |
Renders exposed Formly fields. |
Review details
Suppressed comments (2)
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:185
- This rebuild is unconditional, unlike the compilation path's typing guard. Because
buildForm()emitsformsRebuilt, a binding event while the user is editing tears down the field and cancels its pending debounced write, losing the in-progress value. Defer the rebuild until editing ends rather than rebuilding immediately (and preserve the pending event).
this.workflowActionService.formBindingChanged$.pipe(untilDestroyed(this)).subscribe(() => {
this.readConfig();
this.cdr.detectChanges();
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:322
- The debounced write is canceled by
untilDestroyed(this). If a user edits a field and navigates away or switches to the canvas within 150 ms,save()runs beforewriteValue, destruction cancels this subscription, and the final value is permanently lost. Flush pending field values before saving/destroying, or update the graph synchronously and debounce only persistence/compilation.
form.valueChanges
.pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), takeUntil(this.formsRebuilt), untilDestroyed(this))
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mengw15
left a comment
There was a problem hiding this comment.
Left two comments. Copilot's four all check out against the sources (for one, the HuggingFace widget reads and writes model.task — hugging-face.component.ts:539/557 — so the isolated per-binding model does break it; and form.disable() indeed doesn't reach a picker's own buttons). One question on top: binding.instruction is in the type but not rendered here, and the schema description is deliberately blanked — which slice gives the reader per-field guidance?
69df849 to
a42a373
Compare
a42a373 to
ac4cb96
Compare
Turn each input an author exposed into its operator's own formly field, built from the operator's JSON schema, so a file property gets the real file picker and an attribute a column dropdown instead of degrading to a text box. A shared customFormlyFieldType decides the widget; CANVAS_ONLY_FORMLY_TYPES (the code editor and the drag-reorder list) falls back to formly's default control since those cannot function on a form. A filled-in value writes straight back to its operator -- the same edit the canvas makes -- guarded so formly's build-time empty default never wipes a real value, and only with write access; a read-only viewer sees the values disabled and the page never persists for them. The inputs rebuild on the compilation stream (so attribute boxes become dropdowns once columns are known) and on formBindingChanged$ (so exposing/un-exposing, a co-editor's included, follows at once), skipped while the cursor is in a field. Broken bindings are left out of what a reader sees; nested sub-field overrides, running and results follow in later PRs. Co-Authored-By: Claude Opus 4.8 <[email protected]>
ac4cb96 to
0ba9c6b
Compare
|
@mengw15 on your question about per-field guidance: the schema's own field descriptions are the operator author's notes to whoever wires the operator up on the canvas ("Attribute name in the schema", "Multiple string key/value pairs"), not guidance to a form reader, which is why they are dropped. The reader's per-field guidance is the form author's help text, carried on the binding and, as of the latest push, rendered under each input. The workflow-level markdown instruction above the inputs (the next PR, #8023) is the other half. |
mengw15
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the comments
…#8438) ### What changes were proposed in this PR? Closes apache#8022. Part of the Form View stack (parent issue apache#8011), stacked on apache#8437 (PR10) and apache#8436 (PR9). The render PR (apache#8437) shows the top-level exposed inputs. This one handles a property that carries sub-fields: an object (a nested group) or a repeated section (an array). It renders each sub-field and applies the author's per-sub-field setup. - A walk over the built formly field and its sub-fields drops the operator schema's own per-field descriptions (author notes about the operator, not guidance to a form reader, and shown once per field by formly), and applies the author's stored overrides -- rename and hide -- keyed by field path, with array indices dropped so one entry covers every row. - A repeated section builds its row template on demand, so the walk wraps the builder rather than the single object it returns, decorating every row formly ever creates. - Two small statics, `childPath` (the override path for a child, indices dropped) and `arrayItemOf` (a repeated section's row template, whether formly gives it as a value or a builder), back the keying and are unit tested directly. Writing a filled-in value back to its operator is already handled by apache#8437: formly's model carries the nested value, so a sub-field edit persists through the same write-back. This slice is the sub-field rendering and overrides only. ### Any related issues, documentation, discussions? Closes apache#8022. Part of the Form View feature (parent issue apache#8011). ### How was this PR tested? Unit tests (vitest). `sub-fields.spec.ts` covers the two statics directly (path joining, index dropping, non-name keys; the value/builder/throwing/none row-template cases). `workflow-form.component.spec.ts` covers the walk against the component: renaming and hiding an overridden sub-field of both an object property and a repeated section (per row, through the wrapped builder), dropping the schema descriptions, leaving an un-overridden sub-field untouched, and a scalar array's leaf row template. 100% statement and function coverage on the changed source; the remaining uncovered branches are defensive `??`/`||` fallbacks. `ng build gui`, eslint and prettier are clean, and the full workflow-form suite (73 tests) is green in both the single-user and collaboration paths. The visible effect (with the flag on) is that an exposed nested or array property renders its sub-fields, each renamed or hidden as the author set up. A screenshot can be added on request. #### Screenshot An exposed nested or array property rendered with its **sub-fields**, each renamed or hidden as the author set up. <img width="1271" height="785" alt="Screenshot 2026-09-05 at 11 42 53 AM" src="https://github.com/user-attachments/assets/62233d72-c64b-421d-b637-2dce4e742405" /> ### Was this PR authored or co-authored using generative AI tooling? Yes. Co-authored with Claude (Anthropic), reviewed line by line by the author before submission. Co-authored-by: Claude Opus 4.8 <[email protected]>
…or's instruction (apache#8440) ### What changes were proposed in this PR? Closes apache#8023. Part of the Form View stack (parent issue apache#8011), stacked on apache#8438 (PR11) and apache#8437 (PR10). The Form View can now render inputs and write them back. This PR makes it usable end to end: it runs the workflow, and shows the author's instruction above the inputs. - **Run.** A Run button that shares the operator canvas's disable conditions (an invalid or empty workflow, a unit still connecting, or no unit chosen each disable it and say why), but deliberately simplifies the execution states a reader needs down to Run and Stop, with no pause/resume. The unit is chosen in the embedded computing-unit selector. Running is the same execution call the canvas makes, so empty or invalid inputs surface as a real engine error rather than a client-side gate; a run clock counts off the engine's own event, and a failed run collapses an opaque trace to one plain sentence (a required input left empty reads as "please fill in the required fields"). - **Instruction.** The author's optional instruction renders above the inputs as a collapsible, read-only markdown card, shown only when there is text. It is bound through `[innerHTML]` with Angular's default sanitizer, so author markdown cannot inject script. Editing it is part of the authoring PR. Showing the chosen results is the next PR (apache#8024). ### Any related issues, documentation, discussions? Closes apache#8023. Part of the Form View feature (parent issue apache#8011). ### How was this PR tested? Unit tests (vitest). Direct-construction tests cover every run-button precedence branch, `onRun` (kill while running, no-op when disabled, else execute and clear the error), the five failed-run message paths, the run clock and its tick, and the status/validation/connection subscriptions; the instruction is covered for the markdown render, `hasInstruction`, and the toggle. TestBed template tests stand up the instruction card, the run bar, and the run-button click binding. 100% statement and function coverage on the changed source (the remaining uncovered branches are the pre-existing defensive `??`/`||` fallbacks). `ng test` (99 tests), `ng build gui`, eslint and prettier all pass, in both the single-user and collaboration paths. #### Screenshot The run bar (Run button + computing-unit selector + clock) and the author's instruction card above the inputs. <img width="1323" height="488" alt="Screenshot 2026-09-06 at 12 07 57 PM" src="https://github.com/user-attachments/assets/024d3bf5-be01-4449-8a0f-7538961cb83a" /> ### Was this PR authored or co-authored using generative AI tooling? Yes. Co-authored with Claude (Anthropic), reviewed line by line by the author before submission. Co-authored-by: Claude Opus 4.8 <[email protected]> Co-authored-by: Meng Wang <[email protected]>
…of dropping it (apache#8499) ### What changes were proposed in this PR? Closes apache#8497. The Form View skips rebuilding its input cards while the reader is typing, so a rebuild cannot throw away a half-entered value (from apache#8437). Two things were wrong with that skip. - It counted any focused `INPUT` as typing, tick boxes included. Ticking a property in the step panel focuses the tick box, so the rebuild that should add the card was skipped and the tick looked like it did nothing until something else rebuilt the page (the way apache#8455's edit mode surfaces it). - A skipped rebuild was dropped, not postponed. A schema refresh that landed while someone was typing (the column names an attribute dropdown offers, after a compile) never reached the cards until the next compile or a reload. Now only text-like inputs, textareas, selects and content-editables count as typing, and a rebuild that does arrive mid-typing is held and runs once the focus leaves the text control: a `focusout` host listener decides one tick later, so tabbing to the next text field keeps it held while any other focus change runs it. Both subscribers (compilation state, form-binding changes) go through the one `rebuildFormOrDefer`. apache#8455 carries the same change so it stays self-contained and testable on its own; whichever merges first, the other rebases and the duplicate hunk disappears. ### Any related issues, documentation, discussions? Closes apache#8497. Found while verifying apache#8455 on a flag-on instance (parent apache#8011). ### How was this PR tested? Unit tests (vitest): a focused tick box is not typing; a held rebuild runs once on focusout for both the compilation and the binding stream; it stays held when the focus only moves to another text field; a focusout with nothing held rebuilds nothing; a queued focusout callback does not rebuild a second time when the control that took the focus already rebuilt (the tick box case). The rendered spec dispatches a real bubbling `focusout` from a control inside the page and asserts the host listener drains the held rebuild. Each was checked to fail with its half of the fix removed, including removing the `@HostListener` decorator. Full suite passes (5780), changed lines fully covered, eslint, prettier and the production (AOT) build pass. In a headless browser against a dev server built from this branch: typing a value into an exposed input and tabbing out keeps the value through the held rebuild, with no console errors and every autosave answering 200. The tick-box path was exercised the same way on apache#8455, which carries the identical change: the card follows the tick within a second, in both directions. ### Was this PR authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Fable 5.1, Anthropic). Co-authored with Claude, reviewed line by line by the author before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY Co-authored-by: Claude Fable 5.1 <[email protected]>
…not only the pointer (apache#9016) ### What changes were proposed in this PR? Closes apache#8943. Part of the Form View feature (apache#8011); from the apache#8439 refinements (item 5). A read-only input card -- what a viewer without write access sees -- is locked with `pointer-events: none`. That is hit testing only. The bound formly controls are disabled through `props.disabled`, but the controls a custom widget draws of its own are not: the HuggingFace audio and image uploaders each render a plain file input that nothing disables. A reader could Tab into one inside a locked card, open it with Enter and choose a file. For the audio input that is a real write. The widget POSTs the chosen file to `/api/huggingface/upload-audio`, which authorises any signed-in user and never asks who may write this workflow, so the file lands in the server's temp store. Checked on a workflow shared read-only: two presses of Tab reach the uploader inside the locked card, Enter opens the file dialog, and the POST comes back 200 with the stored path. The image uploader is client-side, so there the reader only makes the card display a picture of their choosing -- a read-only card wires no write-back and a reader's save is refused, so the operator keeps the author's value and a run still runs it. The card now also keeps the keyboard out, through a small directive on it (`texeraReadOnlyCard`, applied as `[texeraReadOnlyCard]="!canEdit"` beside the existing `read-only` class): - Every focusable descendant is taken out of the tab order (`tabindex="-1"`, its own value remembered) and marked `aria-disabled="true"`, so there is nothing for Tab to reach and assistive tech says why rather than going quiet. - Every control the browser will refuse outright -- button, input, select, textarea -- is also disabled natively, its previous state remembered. Out of the tab order is not out of reach: `tabindex="-1"` ends only sequential navigation and `aria-disabled` only announces, so a reader's assistive tech could still focus a widget's file input and activate it, which for the audio one posts the file (Copilot). - A card that becomes editable gets its controls back, each with the tabindex and the disabled state it had, and only those this directive took. - A control that leaves the card is let go of on the next pass: a widget redraws its own controls while the card lives on, so a set that only grew held every detached node for as long as the reader stayed on the page (Copilot). - Widgets render their own controls after the card is built and redraw them as they go, so the locking is re-applied on every change inside the card: a control added, one whose own tabindex is written back (ng-zorro writes one when it enables a control), and one a widget enables again (the audio uploader's `[disabled]="isUploading"`). The observer runs outside Angular and sets only what is not already set, so one pass settles it. `inert` is what the step panel uses for its read-only mount, and the issue suggested following it; this does not, deliberately. `inert` also removes the subtree from the accessibility tree, and a reader's card exists to be read -- the values are what a reader runs the workflow with -- so that would trade this bug for a worse one. Disabled controls stay in the accessibility tree and keep being announced. The directive's doc comment says so, next to the code. Nothing else changes: the pointer lock, the styles, the card's content and the author's editing path are untouched, and the directive does nothing at all while a card is editable. ### Any related issues, documentation, discussions? Closes apache#8943, which re-files apache#8758 (closed). Part of apache#8439 (item 5) and the Form View feature apache#8011. Raised by Copilot on apache#8437, apache#8438 and apache#8440. Three neighbouring things are deliberately left alone. The widgets' own controls still do not consult the field's disabled state in their own handlers -- fixing that is per-widget work (the HuggingFace uploaders, the dataset picker, the resource value selector), and the card-level lock covers every widget at once, including ones added later. The upload endpoint itself authorises on being signed in and nothing else (`@RolesAllowed("REGULAR","ADMIN")`, no workflow-access check), so it would still accept a file posted by hand; closing the UI route does not close that one, and it is worth its own issue. And the step panel's `inert` mount has the same accessibility-tree problem described above; changing it is not this issue's scope, and it is worth its own look. ### How was this PR tested? Unit tests (vitest). The directive's own spec, on a host shaped like a card (a bound input, a widget's Browse button, a link, a span, a control with its own `tabindex`): every focusable control in a read-only card is out of the tab order; each is marked `aria-disabled` while the card is not inert and its text is still there; an editable card is left alone, keeping a control's own `tabindex="3"`; a card that becomes editable gets that `3` back, the untouched control gets no `tabindex`, and the `aria-disabled` marks go; a control a widget adds after the card was built is locked; a control whose widget writes its `tabindex` back, or enables again, is locked again; a click on a locked control dispatches no activation and the controls report disabled (jsdom does not model a disabled control refusing focus, so that half is asserted through the property the browser acts on); the disabled state is given back on unlock; five rounds of a control being added and removed leave the tracked set where it started, rather than holding the detached nodes; the observer is disconnected when the card goes away. The page's rendered spec wires it through the real template: a read-only viewer's card, with a button appended the way a widget renders one, has it out of the tab order and marked, and the card is not inert. By hand, on two dev builds of the same base commit, one with this change and one without, signed in as a second account the workflow is shared read-only with. Before: from the view toggle, two presses of Tab land on the uploader inside the locked card, Enter opens the file dialog, and choosing a file POSTs it (200). After: the same two presses land on Run, the card is skipped, and no request goes out. The mouse reaches nothing in either build, and the stored workflow is unchanged in both. Each guard was deletion-checked and turned a named test red (the directive not applied; the tabindex write; the aria-disabled write; the native disable; the editable check; the unlock; the original-tabindex restore; the original-disabled restore; the release of what has left the card; the childList and both attribute halves of the observer; the disconnect). eslint, prettier and the production (AOT) build pass; every statement, function and branch this PR adds is covered, and the full frontend suite passes. #### Video **Before** https://github.com/user-attachments/assets/cd1b58ff-6cc4-4958-ad78-dd00bb86f14e **After** https://github.com/user-attachments/assets/8f524126-d370-48f3-bc70-68497791cf5f ### Was this PR authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Anthropic). Co-authored with Claude, reviewed line by line by the author before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Purpose
Closes #8370. Part of the Form View stack (#8011), stacked on #8436 (PR9).
The Form View page already has the title bar and the collapsible read-only workflow preview. This PR fills in the middle: it renders the inputs an author exposed and writes filled-in values straight back to their operators.
Changes
customFormlyFieldType(extracted in feat(workflow-form): share the operator property panel's field-type rules #8436). Two widgets that only work on the operator canvas, the code editor and the drag-reorder list, are collected in a newCANVAS_ONLY_FORMLY_TYPESand fall back to formly's default editable control, since they cannot function on a form.formBindingChanged$(exposing or un-exposing a property, a co-editor's change included, is reflected at once), both skipped while the cursor is in a field so an in-progress edit is not thrown away.Nested and array sub-field overrides are added in the next PR (#8022); running the workflow and showing results follow after that.
Tests
workflow-form.component.spec.ts: direct-construction unit tests for rendering, widget selection, the write-back guards, read-only locking, the compilation/binding rebuild triggers, and the typing guard, for both the single-user and the collaboration paths.workflow-form.rendered.spec.ts: TestBed test standing up the real template so the inputs markup (section head, empty state, input card and form wrapper) is covered.custom-formly-type.spec.ts: covers the newCANVAS_ONLY_FORMLY_TYPES.ng test(71 tests here),ng build gui, eslint and prettier all pass.Was AI used?
Yes, co-authored with Claude (Claude Code).
Screenshot
The Inputs area: each exposed property rendered as its operator's own control (the workflow preview and mini-map below it are the earlier PR's).