Skip to content

feat(gui): add the Form View edit mode, with instruction and result authoring - #8516

Merged
mengw15 merged 6 commits into
apache:mainfrom
yangzhang75:formview-pr15a
Sep 12, 2026
Merged

feat(gui): add the Form View edit mode, with instruction and result authoring#8516
mengw15 merged 6 commits into
apache:mainfrom
yangzhang75:formview-pr15a

Conversation

@yangzhang75

@yangzhang75 yangzhang75 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Closes #8026. Part of the Form View stack (parent issue #8011), on main. This is the first half of what was #8455 (closed, split in two at 2200 lines); the second half, authoring the inputs themselves, is #8517 stacked on this one. Four commits: the first is the feature, the second and third are test-only follow-ups (the Windows CI leg and the template coverage report), and the last opens the result picker to everyone with one saved list of shown results (the design changes asked for in review, mengw15's list shape included). The review commit is the branch tip.

Adds the edit mode of the page and the page-level authoring it enables.

  • toggleAuthoring: with write access, Edit turns the page into in-place authoring; Done returns it to the reader state. Edit mode is the only state that enables workflow modification here (applyEditability), and entering it is refused without write access at the method itself, not only by hiding the button.
  • The inspect panel goes live in edit mode: it acts as an editor (actsAsEditor), its expose tick boxes are on (exposeChoosing), and inert comes off, so the author opens a step on the embedded preview and chooses which of its settings people fill in right there. It stays inert and read-only for everyone else, as before.
  • The embedded preview's right-click menu carries the structure lock. The menu's re-shaping commands followed the modification flag alone, so re-enabling it for the panel would have offered cut, paste, delete and disable on the structure-locked preview. texera-context-menu now takes [structureLocked] from the editor and gates those on modification AND no lock (canModify); copy, the result toggles, execute-to and export do not re-shape the graph and are unchanged.
  • Write the author's instruction in place: the heading is edited in the header (a row rather than one button, so the input is not nested in a control) and the body as markdown with Write / Preview tabs. A reader still sees it rendered, only when there is text.
  • Pick which results to show, for everyone: the picker lists the final steps and the intermediate steps that have view-result on the canvas, and rebuilds live when an eye is toggled or the graph's shape or names change (a step deleted, disabled, given a downstream link, or renamed, a co-editor's edit included), from the graph's own streams rather than only after the debounced compilation. What shows is one saved list, shownResultIds (the shape mengw15 proposed): absent, every final step shows, as on the canvas; once the author has chosen, exactly the listed steps show, and [] means none, a choice the earlier pair of opposite-signed lists (featured / turned off) could not store and could contradict each other on. In edit mode a pill adds the step to or removes it from that list; the first choice starts the list from the final steps at that moment, so nothing the author saw disappears. The accepted cost: a step that becomes final after the author has chosen does not appear by itself. A definition whose only content is that list, an empty one included, is still saved. A listed step whose eye has since been turned off stays offered in edit mode only, so the author can take it off the list; a reader is not offered a pill that could never turn on. Anyone else, a writer merely viewing included, gets the same picker but changes only their own view of the page; nothing is written, so a reader without write access can choose too, and the choice lasts for the page. A disabled step is neither offered nor shown, eye or no eye: the compiled plan leaves it out.
  • Edit mode is the only state that unlocks the graph from this page, and only while no run is in flight (the canvas rule, kept so that entering edit mode mid-run cannot undo it). The lock is a root-level flag with writers that know nothing of this page: the execute service unlocks it whenever a run ends (completed, failed, killed, reset), and the computing-unit selector embedded here unlocks it when it finds no run on the chosen unit. Rather than chase each caller, the page clamps at the stream they all report to (getWorkflowModificationEnabledStream): whenever the flag turns on while the page must stay locked, it is turned off again, so a writer merely viewing can never reach the preview's view-result command. In edit mode the canvas rule stands: locked while running, unlocked when the run ends. The clamp runs a microtask after the unlocking call, never inside it: enableWorkflowModification enables undo/redo after it emits, and the stream still has other subscribers to reach, so a nested disable would leave them on the stale "true"; run afterwards, the disable is the last word and every consumer sees one locked state. The execute service flips the lock before it emits the new state; by the time the clamp looks, the execution-state handler has re-applied the same rule with the final state, so in edit mode the unlock stands. The step panel follows the same rule (panelLive), not edit mode alone: the property frame does not consult the lock before its own writes (the version sync on mount, the schema defaults ajv fills in, the editing marker), so a step selected while a run is in flight stays a read-only, inert mount even in edit mode and turns live when the run ends. Done dismisses the step panel before leaving edit mode, while the frame is still an editor: that is the only state in which the property editor clears the "currently editing" marker co-editors see.
  • The embedded preview grows no editing buttons: no link tools on hover (remove, breakpoint), and a selected operator unfolds its state and port counts but not its delete, chat and add/remove-port buttons. None of them can act on the structure-locked preview, so they only suggested it could be edited.
  • Open canvas saves first and hands over only once the save has completed: the switch is a full-page load, which aborts a request still in flight. Saves go out one at a time, in order: two persists in flight at once can reach the backend out of order and the older content would win, so the switch's save waits for an autosave already on its way, and the page hands over only once the queue has drained: that save and any asked for while it was in flight (the page stays interactive until the hand-over) have completed; each request carries the workflow as it was when the save was asked for, so the last one enqueued is the latest. The drain outlives the page: the final save on the way out joins the same queue rather than racing an autosave still in flight, and the queue is closed after it. A failed save keeps the author on the form with the error shown and does not stop the queue; a reader with nothing to save goes straight through. A save's response feeds back the server-owned metadata (the timestamp, and the normalised name when nothing changed) but never undoes a rename made while it was in flight, and repaints nothing once the page is gone. (feat(gui): wire the Form View entry points #8456 does the save-then-navigate part on the canvas side.)
  • Keyboard focus is visible on the pill and tab buttons (:focus-visible). The picker's pills are tracked by step (trackBy): a toggle re-reads the config and rebuilds the choices as new objects, and re-created buttons would have dropped the keyboard focus from the pill just pressed. The instruction body carries an aria-label, since its placeholder is gone as soon as there is text.
  • 29c54bb85 (test only, workspace menu): the menu's export test module-mocked the CommonJS file-saver package with vi.mock. Under the Angular unit-test builder that call is not reliably hoisted (Vitest warns about it on every platform), and with this stack's shared-chunk graph it stopped applying on the Windows leg (7 of the last 8 Windows runs of the unsplit PR, while main passes). The export now goes through the existing injectable FileSaverService, as the dashboard downloads already do, and the spec stubs that with TestBed. No behaviour change.
  • 695c2fb4f (test only, Form View rendered spec): the spec swapped the property panel for a stub by overriding the page's imports, which JIT-recompiles the page; a JIT template has no mapping back to the .component.html, so the page's template read as 0% covered on codecov from feat(gui): open a step to inspect its settings read-only on the Form View #8442 on (the 10 "missing" template lines there, 132 on the unsplit PR). The real panel's template is blanked instead, with its lifecycle hooks switched off, so the page stays AOT-compiled and its template is measured again. The edit-mode markup this PR adds is then covered through the DOM: Edit / Done, the Write / Preview tabs, heading and body writing through, the picker's pills and empty hint.

The diff is about 1400 added lines because 725 of them are spec against 694 of source (of which 192 are stylesheet); the two test-only commits and the review-driven redesign of the picker are a large part of it, and the feature itself is under 700 lines.

Not in this PR: renaming, hiding, reordering, help text and removal of the exposed inputs, and the author's view of a broken input. Those are #8517.

Any related issues, documentation, discussions?

Closes #8026. Part of the Form View feature (parent issue #8011). Replaces the first half of #8455.

How was this PR tested?

Unit tests (vitest). Direct-construction tests cover the authoring gate (enter, leave, refused without write access, always allowed to leave), the result picker's range (final steps until the author chooses, then exactly the saved list, [] included; viewed and listed intermediates; disabled steps left out), its live rebuild on an eye toggle, the edit-mode toggle writing the default (the saved list started from the final steps on the first choice), a viewer's own toggle writing nothing and giving way on entering edit mode, a reader not offered a saved pick whose eye is off, the lock clamped back whenever anything else unlocks the graph outside edit mode, unlocked in edit mode once a run has ended in the execute service's real order (unlock before state), and kept locked when edit mode is entered mid-run, the switch's save queued behind an autosave in flight with navigation after both, the final save on the way out drained behind an autosave in flight after the page is gone, a failed save not stopping the queue, a failed final save still reported without throwing, an older save's response not undoing a rename made meanwhile and no metadata repaint after the page is gone, a shown step dropped from the cards and the picker the moment the graph's shape makes it unavailable, a pill renamed the moment its step's display name changes, the hand-over waiting for a save queued behind the switch's and staying put when that save fails, the step panel dismissed on Done while the frame is still an editor, the instruction writes going through the binding service, and the switch's save-then-navigate order (navigates on complete, stays on error, straight through for a reader). The rendered spec covers the edit-mode header shape (title input outside the toggle, aria-controls), the panel turning live in edit mode (editor on, tick boxes on, inert off), and the panel staying read-only while a run is in flight even in edit mode, turning live when the run ends. The context menu's spec renders it under a structure lock with modification enabled and checks cut, paste, delete, disable and enable are off while copy and the result toggle stay; the editor's spec opens the right-click menu on a locked editor and checks the lock arrived, and checks a locked preview grows no link tools on hover or on add, and keeps a selected operator's delete, chat and port buttons hidden while its state still unfolds, while the canvas shows all of them. The binding service's spec checks the first choice starts the list from the default handed in, later choices flip within the list ignoring the default, and turning off the only shown step leaves [] rather than an absent list; the action service's spec checks a definition whose only content is shownResultIds: [] is still carried in the saved content. Each new guard was deletion-checked (removing it turns the corresponding test red). eslint, prettier and the production (AOT) build pass; every changed line, template lines included, is statement and function covered (5830 tests).

Video

Screen.Recording.2026-09-11.at.12.20.36.PM.mov

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.ai/code/session_01FVvP3ttj22f9LB4p9u2anY

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @PG1204, @Yicong-Huang, @mengw15
    You can notify them by mentioning @PG1204, @Yicong-Huang, @mengw15 in a comment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new authoring UI has unresolved keyboard-focus and labeling accessibility issues, and required visual evidence is missing.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Form View edit mode for instruction and result authoring while preserving the embedded workflow’s structure lock.

Changes:

  • Adds authoring controls, editable instructions, result selection, and save-before-navigation.
  • Enables property editing while preventing structural context-menu actions.
  • Replaces direct file-saver mocking with FileSaverService.
File summaries
File Description
form-binding.service.ts Clarifies featured-result behavior.
workflow-form.spec-harness.ts Adds authoring service mocks.
workflow-form.rendered.spec.ts Tests authoring UI and live property panel.
workflow-form.component.ts Implements authoring, result choices, and save handoff.
workflow-form.component.spec.ts Tests authoring and navigation behavior.
workflow-form.component.scss Styles authoring controls.
workflow-form.component.html Renders the authoring interface.
workflow-editor.component.ts Documents structure-lock propagation.
workflow-editor.component.spec.ts Tests context-menu lock propagation.
workflow-editor.component.html Passes the structure lock to the menu.
context-menu.component.ts Adds structure-aware modification gating.
context-menu.component.spec.ts Tests locked structural commands.
context-menu.component.html Hides structural actions under lock.
property-editor.component.ts Preserves teardown behavior without early return.
menu.component.ts Uses injectable file-saving service.
menu.component.spec.ts Stubs file saving through TestBed.
Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov-commenter

codecov-commenter commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.64253% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.88%. Comparing base (069cd20) to head (d5bf9af).

Files with missing lines Patch % Lines
...component/workflow-form/workflow-form.component.ts 98.11% 0 Missing and 2 partials ⚠️
...mponent/workflow-form/workflow-form.component.html 98.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8516      +/-   ##
============================================
+ Coverage     93.63%   93.88%   +0.25%     
+ Complexity     4858     4837      -21     
============================================
  Files          1212     1211       -1     
  Lines         50037    50172     +135     
  Branches       6132     6156      +24     
============================================
+ Hits          46850    47106     +256     
+ Misses         1675     1550     -125     
- Partials       1512     1516       +4     
Flag Coverage Δ *Carryforward flag
access-control-service 80.18% <ø> (ø) Carriedforward from c510067
agent-service 99.32% <ø> (ø) Carriedforward from c510067
amber 89.84% <ø> (-0.03%) ⬇️ Carriedforward from c510067
computing-unit-managing-service 77.14% <ø> (ø) Carriedforward from c510067
config-service 87.12% <ø> (ø) Carriedforward from c510067
file-service 83.65% <ø> (ø) Carriedforward from c510067
frontend 96.72% <98.64%> (+0.55%) ⬆️
notebook-migration-service 83.73% <ø> (ø) Carriedforward from c510067
pyamber 98.41% <ø> (-0.07%) ⬇️ Carriedforward from c510067
workflow-compiling-service 75.00% <ø> (+0.90%) ⬆️ Carriedforward from c510067

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yangzhang75
yangzhang75 marked this pull request as ready for review September 11, 2026 19:21
@yangzhang75

Copy link
Copy Markdown
Contributor Author

/request-review: @mengw15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The result picker currently offers disabled operators whose results cannot be materialized.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts Outdated
yangzhang75 and others added 3 commits September 11, 2026 12:44
…uthoring

With write access, an Edit toggle turns the Form View page into in-place
authoring: the author writes the instruction (markdown, write/preview),
picks which extra results to feature, and opens a step on the embedded
preview live (the property panel acts as an editor with its expose tick
boxes on), so exposing a setting happens where it is chosen. Leaving edit
mode returns the page to the reader state.

Edit mode re-enables workflow modification for the panel, so the structure
lock the embedded preview already carries is handed to the right-click
menu as well (cut, paste, delete and disable stay off there). Open canvas
saves first and hands over only once the save has completed, since the
full-page load would abort a request still in flight.

Renaming, hiding, reordering and removing the exposed inputs themselves
follow in the next PR.

Closes apache#8026.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
The workspace menu's export test module-mocked the CommonJS file-saver
package (vi.mock("file-saver")). Under the Angular unit-test builder that
call ends up inside the built spec chunk rather than at its top level
(Vitest warns about it on every platform), and once this stack shifted the
shared chunk graph the mock stopped applying on the Windows leg: the real
saveAs ran and the spy saw 0 calls (7 of the last 8 Windows runs on apache#8455
and apache#8456, while main and apache#8499 pass).

Route the download through the existing injectable FileSaverService, as
the dashboard downloads already do, and stub that with TestBed, which does
not depend on mock hoisting. No behaviour change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
…ate is covered

The rendered spec swapped the property panel for a stub by overriding the
page's imports. That JIT-recompiles the page, and a JIT template has no
mapping back to the .component.html, so every line of the page's template
read as uncovered (0%) in the coverage report from apache#8442 on, hiding the
template gaps of this PR. Blank the real panel's template instead and
switch off its lifecycle hooks; the page stays AOT-compiled and its
template is measured again.

With the template measured, cover the edit-mode markup this PR adds through
the DOM: the Edit / Done control, the instruction's Write / Preview tabs,
heading and body writing through, and the result picker's pills and empty
hint.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
@yangzhang75
yangzhang75 force-pushed the formview-pr15a branch 2 times, most recently from a34dbc6 to 0c039aa Compare September 11, 2026 22:17
Comment thread frontend/src/app/workspace/service/form-binding/form-binding.service.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Result preferences can be lost or become nonfunctional, and execution completion can re-enable modifications outside edit mode.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 20/20 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread frontend/src/app/common/type/workflow.ts Outdated
Comment thread frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts Outdated
@mengw15

mengw15 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Before this merges — the only zero-cost window, since neither field has ever shipped — I'd like to change the two lists into one: shownResultIds?: string[]. undefined = the terminal default (an untouched form behaves exactly as now); a value = exactly what shows (first curation materialises the current shown set); [] = explicitly nothing, which today's shape cannot express. The viewer-local layer is orthogonal and unaffected.

Three reasons. (1) The disjoint invariant now lives only in the two toggle methods, but the config is data: anything else that writes it (a hand edit, a future consumer) can still produce overlapping lists, and the read side still resolves them wrong — authorsDefault adds chosen back without subtracting hidden. With one list the conflict is unrepresentable rather than discouraged. (2) [] as "no results section" is a real authoring choice the two-list shape can never store. (3) #8517 and every later consumer learns one concept instead of two opposite-signed ones.

The cost is that a new terminal no longer auto-shows once an author has curated — for a curated page I'd argue that is the more predictable behaviour: a canvas edit should not silently add a card to a page someone laid out. Happy to review it as part of this PR or a follow-up commit on it.

@mengw15 mengw15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for addressing the comments

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Result availability can become stale after shared graph edits, and queued save responses can overwrite newer metadata.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Property editing can bypass the run lock, authoring awareness can remain stale after Done, and newer edits can be lost during save-then-navigate.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:1548

  • afterwards runs when this particular snapshot completes, not when the queue is drained. While a slow switch save is in flight, the still-interactive page can accept another edit; its debounced autosave is then queued after this request (or has not fired yet), but this callback starts the full-page navigation first, so that newer save can be aborted and the latest edit lost. Freeze further edits when handover starts, or navigate only after a queue/debounce barrier confirms no newer snapshot remains.
            // `afterwards` runs only when this save completed; a failed save has reported itself and
            // must not let a caller navigate away from changes that were never stored.
            tap({ complete: () => afterwards?.() }),
            // A failed save must not take the queue down with it: the next one still goes out.
            catchError(() => EMPTY)
  • Files reviewed: 22/22 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread frontend/src/app/workspace/component/workflow-form/workflow-form.component.html Outdated
…list of shown results

The picker was the author's alone and never offered the final step. Now
everyone sees it: in edit mode a toggle sets the default for all readers
(saved in the config); anyone else, a writer merely viewing included,
changes only their own view of the page, so a reader without write access
can choose too. Nothing is written for that; the choice lasts for the page.

The final steps are offered as well, under their plain name like any other
step. What shows is one saved list, shownResultIds (mengw15's shape):
absent, every final step shows, as on the canvas; once the author has
chosen, exactly the listed steps show, and an empty list means none, a
choice the previous two opposite-signed lists (featured / turned off) could
not store and could contradict each other on. The first choice starts the
list from the final steps at that moment, so nothing the author saw
disappears; the accepted cost is that a step which becomes final after the
author has chosen does not appear by itself. A definition whose only
content is that list, an empty one included, is still persisted.

Only edit mode unlocks the graph from this page, and only while no run is
in flight (the canvas rule, kept so that entering edit mode mid-run cannot
undo it). The lock is a root-level flag with writers that know nothing of
this page -- the execute service unlocks it when a run ends, the
computing-unit selector when it finds no run on the chosen unit -- so the
page clamps at the stream they all report to: whenever the flag turns on
while the page must stay locked, it is turned off again. The clamp runs a
microtask after the unlocking call, never inside it: that call enables
undo/redo after it emits and the stream still has other subscribers to
reach, so a nested disable would leave them on the stale "true". The
execute service flips the lock before it emits the new state; by the time
the clamp looks, the execution-state handler has re-applied the same rule
with the final state, so in edit mode the unlock stands. The step panel
follows the same rule (panelLive), not edit mode alone: the property frame
does not consult the lock before its own writes, so a step selected while
a run is in flight stays a read-only, inert mount even in edit mode and
turns live when the run ends. Done dismisses the step panel before leaving
edit mode, while the frame is still an editor: the only state in which the
property editor clears the "currently editing" marker co-editors see. A reader is
offered only steps whose result exists (final, or viewed on the canvas); a
listed step whose eye has gone stays offered in edit mode only, to be
taken off.

Saves go out one at a time, in order: two persists in flight at once can
land out of order and the older content would win, so the save behind the
Canvas switch waits for an autosave already on its way, and the page hands
over only once the queue has drained -- that save and any asked for while
it was in flight, the page staying interactive until the hand-over; each
request carries the workflow
as it was when the save was asked for, so the last one enqueued is the
latest. The drain outlives the page: the final save on the way out joins
the same queue instead of racing an autosave still in flight, and the
queue is closed after it. A failed save reports itself and does not stop
the queue. A save's response feeds back the server-owned metadata (the
timestamp) but never undoes a rename made while it was in flight, and
repaints nothing once the page is gone.

What has a result, and what the picker offers, follows the graph's shape
and names as well as the eye: a step deleted, disabled, given a downstream
link, or renamed (a co-editor's edit included) refreshes both at once from
the graph's own streams, not only after the debounced compilation.

The structure-locked preview no longer grows editing buttons: no link tools
on hover (remove, breakpoint), and a selected operator unfolds its state
and port counts but not its delete, chat and add/remove-port buttons. None
of them can act there, so they only suggested the preview could be edited.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Canvas navigation can discard edits still waiting in the autosave debounce.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 1
  • Review effort level: Balanced

…vas hand-over

The hand-over waits for the persist queue to drain, but the queue only
counts enqueued saves: an edit made after the switch click sits in the
autosave debounce, so the drain could reach zero and the full-page load
would kill that edit unsaved. workflowChanged now marks the page dirty
before the debounce, an enqueued snapshot clears the mark, and the
drain flushes one more save instead of handing over while the mark is
set. Spec: an edit made after the click, still in the debounce, goes
out before navigation; disabling the flush turns it red.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Editor transitions can lose recent property edits or leave stale awareness, and a deferred canvas callback can redirect after the page has been left.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:378

  • Making this panel editable exposes a stale-run path: OperatorPropertyEditFrameComponent debounces property writes by 150 ms, while ExecuteWorkflowService.executeWorkflowWithEmailNotification() builds the logical plan synchronously before its own 150 ms send delay. If the author types a value and immediately clicks Run, the request therefore contains the previous property value. Flush pending panel edits before running, or build the logical plan after the debounce has elapsed.
    frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:447
  • When a run starts with a selected live panel, panelLive becomes false and PropertyEditorComponent.ngOnChanges remounts the frame as a viewer. That teardown does not clear the currentlyEditing awareness value, and the viewer-side highlight handler deliberately cannot clear it, so co-editors continue to see this session as editing the step throughout the run (and after Done during the run). Clear the marker before the live-to-read-only transition while retaining the selection.

This issue also appears in the following locations of the same file:

  • line 1179
  • line 1730

frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:1734

  • A pending Canvas handoff remains in afterDrain if the component is destroyed by another navigation while its save is in flight. Because the queue intentionally outlives the component, that save can later complete and invoke openCanvasPage(), unexpectedly redirecting the user away from their new page. Cancel pending handoff callbacks during teardown while still allowing queued saves to drain.
    this.destroyed = true;
    // The final save joins the queue behind anything still in flight, then the queue is closed: the
    // drain (not tied to this component) sends what is left in order and ends by itself.
    this.save();
    this.persistQueue.complete();

frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:1183

  • Closing the selected panel here destroys OperatorPropertyEditFrameComponent immediately. Its property-change stream is debounced by 150 ms and teardown unsubscribes it, so clicking Done immediately after changing a property drops that last edit before it reaches the shared graph or autosave. Flush the editor's current value before unhighlighting/remounting, or defer teardown through an explicit editor flush contract.
      // Leaving: dismiss the step panel BEFORE the frame stops being an editor. The property editor
      // clears the "currently editing" marker it published only while it acts as one (the same gate as
      // the publish), and a remount as a viewer clears nothing -- co-editors would keep seeing this
      // session as editing the step after Done.
      this.closeOperatorPanel();
  • Files reviewed: 22/22 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@mengw15
mengw15 enabled auto-merge September 12, 2026 15:21
@mengw15
mengw15 added this pull request to the merge queue Sep 12, 2026
Merged via the queue into apache:main with commit 0b103f8 Sep 12, 2026
24 of 25 checks passed
renovate-bot pushed a commit to renovate-bot/apache-_-texera that referenced this pull request Sep 12, 2026
### What changes were proposed in this PR?

Closes apache#8027. Part of the Form View stack (parent issue apache#8011), on main
now that apache#8516 (the edit mode) has merged. This is the second half of
what was apache#8455 (closed, split in two at 2200 lines). The review commit
is the branch's single commit, on main.

Lets the author shape the exposed inputs where they appear, in the edit
mode apache#8516 adds.

- Rename an input or any of its sub-fields, and hide a sub-field,
through a new `editable-label-wrapper` formly wrapper: the label is the
input, so what the author types is what the reader reads; leaving it
empty falls back to the schema label (captured before the stored
override is applied, so a renamed sub-field's placeholder and tooltip
promise what clearing the box really yields). The control keeps a
programmatic name in both modes (a visually hidden `<label for>` while
authoring, the static label for readers); a repeated field has no
labelable control carrying its id, so its title names the rows as a
group (`role="group"` + `aria-labelledby`) instead of pointing a label
at nothing. Overrides are stored per binding and reapplied on every
rebuild. In a repeated section every row shares one override, so the
controls (name box, eye) sit on the first row only; later rows show the
same name and hidden state statically and follow the first row's edits
at once, and a scalar array's rows are walked as rows, never as the
input's root, so the input's own title box appears once above them.
- Reorder inputs by drag (CDK drag-drop, mapped by binding id so a card
the config no longer holds cannot move the wrong field) or from the
keyboard with Move up / Move down on each card, disabled at the ends.
- Write per-input help text (saved without rebuilding the form on every
keystroke), see "From X" attribution while authoring, and remove an
input. A rename is written through on every keystroke (input, not
change), so the hidden label and the saved name follow the box as it is
typed and nothing is lost if the page is left while the box still has
the focus. A step renamed on the canvas or in the live panel (a
co-editor's rename included) rebuilds the cards, held while the reader
is typing like the compilation path, so the "From X" attribution never
goes stale; nothing else emits for a rename.
- A binding whose operator no longer exists renders as a removable card
carrying the reason for the author and is hidden from readers; it is
never deleted silently on a re-read.
- A repeated (array) input keeps its title above its rows in reader mode
too: the shared array widget prints its label at the bottom beside its
add button, so without this the title jumped from above the rows in edit
mode to below them on Done.
- Keyboard focus is visible on the Move, Remove and eye buttons
(`:focus-visible`), and it is never dropped by the edit it triggers: a
rename, a hide or help text is presentation only and is shown by the
control that took it, and the page does not rebuild on such a write's
own announcement: every config write announces on `formBindingChanged$`
and the page rebuilds on that stream, so a presentation write is marked
while it is made and its announcement skipped (it still reaches the
autosave), while structural writes (expose from the panel, remove,
reorder) rebuild as before, once (the callers that re-read themselves
mark their write too). The test harness's form-binding mock now
announces like the real service, so this chain is under test rather than
severed; the Move buttons at the ends are `aria-disabled` rather than
disabled, so a move that reaches the top or bottom keeps the focus on
the button; after Remove the focus goes to the next card's Remove, else
the previous one's, else the Inputs heading. The Move and Remove buttons
are named with their input (every card has the same three), and the eye
is a proper toggle (constant name, state in `aria-pressed`).

The diff is about 1650 added lines because 830 of them are spec against
829 of source (of which 246 are stylesheet); the wrapper and the card's
author row are one feature, and the wrapper's four files plus the
review-driven fixes (once-per-path controls, self-reflected writes) are
the bulk of the rest.

### Any related issues, documentation, discussions?

Closes apache#8027. Part of the Form View feature (parent issue apache#8011).
Replaces the second half of apache#8455.

### How was this PR tested?

Unit tests (vitest). Direct-construction tests cover rename and hide
overrides reaching the rendered fields (root title and sub-fields, keyed
by path), the cards rebuilt when a step's display name changes, the name
box writing through on each input event, a repeated section's controls
on its first row only with later rows following a rename and a hide, the
schema label kept as the fallback of an already renamed sub-field, a
scalar array's rows walked as rows (one title box), the page not
rebuilding on its own presentation writes though each is announced while
a structural announcement still rebuilds, and the wrapper naming a
repeated field as a group and fading a hidden field in a follower row,
drag and keyboard reorder with the id-mapped indices (including the end
stops and a card the config no longer holds), help text saved without a
rebuild, broken bindings shown to an author only and kept for explicit
removal, and the repeated input's static title. The rendered spec drives
the card's author row through the DOM: the provenance line and drag
handle, the Move buttons in both directions with their end states, the
help-text box writing through, Remove, the broken card's reason, and the
drop hand-off to `onDrop`. The editable-label wrapper has its own
TestBed spec (real component: decorate, the name box's change renaming,
the eye hiding, the reader's label association and the hidden label
while authoring). Each new guard was deletion-checked (removing it turns
the corresponding test red). eslint, prettier and the production (AOT)
build pass; every changed line, template lines included, is statement
and function covered.

#### video


https://github.com/user-attachments/assets/566f8bd7-5596-4085-9317-79974cfdfad2


### 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 <noreply@anthropic.com>
renovate-bot pushed a commit to renovate-bot/apache-_-texera that referenced this pull request Sep 13, 2026
### What changes were proposed in this PR?

Closes apache#8028. Part of the Form View stack (parent issue apache#8011), on main
now that apache#8516 and apache#8517 have merged. The review commit is the branch's
single commit.

Wires the Form View entry points. The flag stays off here; the stack's
closing PR, apache#8528, flips it.

- The dashboard opens a workflow in its `default_view` (form or canvas),
with a toggle that persists the choice; the deep link goes to the
existing `/workflow/:id/form` route. Both renderers of the dashboard,
the list row and the card, follow one shared rule
(`default-view-landing.ts`: mark, deep link, toggle), so switching the
view mode does not lose the entry point. The toggle is offered only with
WRITE access, which the endpoint requires, and the handler checks the
same rule rather than trusting the template; it is a proper toggle
button (constant accessible name, state in `aria-pressed`, the hover
title spelling out what a click does); hub links are left untouched.
- The canvas menu gains the same Canvas / Form View switch the form
already shows, so the two views swap in place. It saves first and hands
over only once the save has completed: the switch is a full-page load,
which aborts a request still in flight. Two more things the hand-over
must not lose: an autosave already in flight when the switch is clicked
(`WorkflowPersistService` now sends saves one at a time and in call
order, at the one place every save goes through, so the switch's save
lands and completes after it; each caller still gets only its own result
and a failed save does not hold up the next), and an edit made while the
switch's save is out (the page stays editable until the load;
`workflowChanged` marks it and the hand-over saves once more before
leaving). A reader, who cannot save, goes straight over. A workflow the
canvas holds but has never saved (the default id) is created by that
save, and the hand-over opens the id the save answered with. On the card
the toggle sits in the always-visible action footer, in the same slot as
on the row (right after Detail); the row's hover-revealed action group
also appears while the row has the keyboard focus, so the toggle can be
reached without a pointer there too. A second click while the hand-over
is in progress is a no-op. A failed save keeps the user on the canvas
with the error shown. Every workflow offers both views whenever the flag
is on: `default_view` only decides the landing view, and neither view
gates the other.
- Download/upload round-trips `defaultView` as a sibling key next to the
workflow content, in one shared export shape (`exportedWorkflow`) used
by the dashboard download and the canvas menu's export alike; an old
export without the key imports unchanged.
- The computing unit the user picks is remembered per workflow
(localStorage) so it survives switching between the two views; a unit
selected on load (the remembered one, the last execution's, a running
one) is derived rather than chosen and is not stored, or a derived unit
would later outrank a fresher last execution. On load the remembered
unit is honoured only once the unit list has arrived and still holds it:
a unit that has since been terminated is forgotten and the last
execution's unit is used instead, and a decision still pending when the
workflow changes underneath it is dropped (the remembered-unit check,
the last-execution lookup and its running-unit fallback alike).

### Any related issues, documentation, discussions?

Closes apache#8028. Part of the Form View feature (parent issue apache#8011).

### How was this PR tested?

Unit tests (vitest) cover the menu's Canvas / Form View switch through
the DOM (absent with the flag off, Canvas pressed, Form View handing
over, hidden while an older version is displayed), the row's and the
card's default-view behavior (mark and deep link, hub link untouched,
flag off leaves the dashboard as today, WRITE-only toggle in the DOM,
toggle on / off / failed request / no cached row), the menu switch
(navigates only once the save completes, stays on the canvas with the
error when it fails, saves once more when an edit lands while its save
is out, takes a reader straight over without a save, ignores a second
click mid hand-over), the canvas export carrying `defaultView` next to
the content and omitting it when unset, the hand-over opening the id the
save assigned when the canvas held a never-saved workflow, the persist
service sending saves one at a time in order with each caller getting
its own result and a failure not holding up the next, the dashboard
toggle handlers refusing without WRITE access, the toggle's aria-pressed
following the state, the export/import round-trip including a legacy
file without `defaultView`, and the computing-unit recall (waits for the
first non-empty unit list, forgets a terminated unit and falls back,
drops a stale decision after the workflow changed, a late last-execution
answer or fallback included, positive-integer validation, storage
failures, only an explicit pick remembered). Each new guard was
deletion-checked (removing it turns the corresponding test red). eslint,
prettier and the production (AOT) build pass; every changed line,
template lines included, is statement and function covered.

#### Video

##### 1. Default view on the dashboard
(row and card toggle, Form View icon, deep link into the form, toggle
off again, no toggle without write access)


https://github.com/user-attachments/assets/04f71ba5-27ad-489d-a5be-51cdd8d811aa



##### 2. Canvas / Form View switch
(save first, then the hand-over; and back)


https://github.com/user-attachments/assets/39c3d395-6329-403d-9719-ca7ef179889f


##### 3. Computing unit remembered across the switch


https://github.com/user-attachments/assets/6fe70b5f-d936-4333-bcf0-b3fa44f84323


##### 4. Download / upload keeps the default view


https://github.com/user-attachments/assets/0519bfcd-0677-478f-866c-6eb0350d842c


##### 5. Default view on the card view
(the same toggle in the card's action row, Form View icon, deep link
into the form)


https://github.com/user-attachments/assets/471192b3-7935-4da9-be87-fa08a7dd6c13



### 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 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(workflow-form): Form View edit mode, with instruction and result authoring

4 participants