yangzhang75 commented on issue #8011: URL: https://github.com/apache/texera/issues/8011#issuecomment-5482541641
> Read through the series (and the `formview-mvp` branch) ahead of reviewing the PRs. Three design-level points, raised here once rather than piecemeal on the slices: > > **1. agent-service will erase the form definition on save.** `agent-service` keeps its own copy of `WorkflowContent` (`agent-service/src/types/workflow.ts`) and rebuilds the content object from that allowlist before POSTing `/workflow/persist` (`workflow-state.ts`, `getWorkflowContent()`). Any agent edit to a parameterized workflow drops `parameterization`, while `is_parameterized` stays on — the form becomes an empty shell. The same copy already lacks `settings.executionMode`, so an agent save silently resets the execution mode today. The plan probably needs an agent-service slice (or fold it into formview-pr3). > > **2. The read-only path needs an explicit decision.** For a user with READ access, the MVP form is fully editable: the input cards are standalone form groups not covered by the modification lock, so typed values go into the shared-editing doc (co-editors see them live), every autosave is then rejected by `/workflow/persist` with 403 and surfaces the "Could not save" toast on each debounce, and Run still works because execution is gated on computing-unit access rather than workflow access. This path is reachable from the normal UI: the dashboard entry-link rewrite and the form page's redirect both check `formViewEnabled && isParameterized` but not `readonly`, so a workflow shared read-only (or any public one, since `hasReadAccess` is true for those) opens straight into it. Worth deciding once at the design level — redirect read-only users to the canvas like the flag-off case, render the inputs read-only (view and run with the stored values), or scope the form to write-access us ers for the first release. > > **3. Question, not blocking: why an `is_parameterized` column instead of deriving from content?** The single-workflow paths don't need it — `retrieveWorkflow` returns the content, and the form page already redirects based on the loaded workflow. The column seems load-bearing only for the dashboard listing affordances (icon, entry-link rewrite, list-page toggle), since listings don't select `content`. If that is the rationale it's a reasonable trade — worth recording it in formview-pr2 together with the cost it imposes on `/persist` (the extra fetch-before-update needed so a plain save doesn't reset the flag). 1. Good catch, and agreed it belongs with the types slice. It's handled in #8275 (formview-pr3): rather than add `formBinding` to agent-service's typed allowlist, `WorkflowState` carries it as an opaque value, capturing `content.formBinding` on load and re-emitting it in `getWorkflowContent()` on save only when the loaded content actually carried one. A plain workflow's serialized content therefore stays byte-identical (no `"formBinding": null` creeps in) while a form workflow survives any agent edit intact, and because it's opaque, later shape changes to the definition need no agent-service change. It's covered by a test, `carries the Form View definition through an agent save untouched`. The `settings.executionMode` drop you noted is a real but separate, pre-existing issue, since agent-service's `WorkflowSettings` type only declares `dataTransferBatchSize`, so I'll file it on its own rather than widen this slice. 2. Agreed this needs deciding once. Decision: render the inputs read-only for read-access users (your middle option), i.e. view the stored values and run, with no redirect. Concretely the form inputs will subscribe to the same `getWorkflowModificationEnabledStream()` the property editor already uses, so they render disabled for a read-only user exactly the way operator properties do on the canvas. That closes the gap you identified, since the input cards are standalone form groups today that sit outside the modification lock, and it removes the 403-on-every-debounce loop at the source, because with no editable values there is nothing to autosave and `/workflow/persist` is never called. Run still works because execution is gated on computing-unit access, so a read-only or public workflow opens into a coherent view-and-run surface that behaves just like the read-only canvas. 3. Yes, that's exactly the rationale. The dashboard listing and search paths deliberately don't select `content` (it's a large blob), so anything the listing needs, such as the icon, the entry-link rewrite, and the list-page toggle, has to live on the workflow row rather than be derived per-item from content. It's the same reason `is_public` is a column, and the form-view bit is a natural peer of it. On the `/persist` cost you flagged, it's already avoided in #8125 (formview-pr2): `saveWorkflowFields` issues an explicit-column UPDATE of just `name`, `description`, `content`, and `is_public` and never writes the form-view column, so a plain save leaves it untouched with no fetch-before-update, since that column is owned solely by the toggle endpoint. I'll record this rationale in the PR description as you suggested. Forward note: per the gating-model thread this column is becoming a `default_view` preference (canvas or form) rather than a capability flag, but the column-vs-derive r easoning is unchanged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
