feat(workflow-form): show the chosen results - #8441
Conversation
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8441 +/- ##
============================================
- Coverage 93.94% 93.91% -0.03%
Complexity 4824 4824
============================================
Files 1209 1209
Lines 49618 49715 +97
Branches 6056 6070 +14
============================================
+ Hits 46615 46692 +77
- Misses 1522 1538 +16
- Partials 1481 1485 +4
*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:
|
085513f to
ecf666e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Multiple moderate issues remain in state synchronization, accessibility, visualization lifecycle, and execution-state handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds selected, non-empty workflow results to Form View as tables or visualizations without modifying canvas result-view settings.
Changes:
- Adds non-empty result detection and display filtering.
- Adds result cards, visualization zoom, and refresh handling.
- Expands component, template, service, and utility tests.
Required fixes:
workflow-form.component.html:229(moderate; 1 vote): Show the correct post-run empty state instead of “press Run.”workflow-form.component.html:245,253(moderate; 1 vote): Add accessible names to both zoom buttons.workflow-form.component.html:274(moderate; 1 vote): Avoid recreating the visualization component on every progressive update.workflow-form.component.ts:238(moderate; 2 votes): Refresh displayed results on view-result changes and operator deletion.workflow-form.component.ts:242(moderate; 1 vote): Keep visualization frames stable during progressive updates.workflow-form.component.ts:794(moderate; 1 vote): Prevent iframeloadlisteners from accumulating.workflow-form.component.ts:302(moderate; 1 vote): Clear stale run errors when a new shared execution begins.
File summaries
| File | Description |
|---|---|
frontend/src/app/workspace/service/workflow-result/workflow-result.service.ts |
Detects non-empty workflow results. |
frontend/src/app/workspace/service/workflow-result/workflow-result.service.spec.ts |
Tests result detection. |
frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts |
Expands Form View test mocks. |
frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts |
Tests rendered controls and result states. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts |
Implements execution and result-display behavior. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts |
Tests component behavior. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss |
Styles Form View sections and result cards. |
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html |
Renders run controls and workflow results. |
frontend/src/app/workspace/component/workflow-form/sub-fields.spec.ts |
Tests nested-field utilities. |
Review details
Suppressed comments (1)
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:256
- This second icon-only zoom control also has no accessible name. Give it an
aria-labelthat identifies the zoom-in action.
<button
(click)="zoomResult(id, 1)"
[disabled]="resultZoom(id) >= 2"
nz-tooltip="Bigger">
- Files reviewed: 9/9 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2b2b319 to
b29335d
Compare
b29335d to
8c2b011
Compare
Render each chosen, currently-viewed step's output under the workflow that produced it -- a table, a visualization, or a compact "no result yet" -- with a per-result zoom and visualizations fitted to their cards after a run. The chosen set is a pure display filter that follows the canvas's view-result set (getOperatorsToViewResult) and never writes it, per the settled result-panel design. Add WorkflowResultService.hasNonEmptyResult so a view-result step that produced zero tuples (e.g. a download/publish UDF) reads as "no result" rather than an empty card. Opening a step to inspect it, and the authoring mode that picks what to show, follow in later PRs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
8c2b011 to
89e7b52
Compare
|
/request-review: @mengw15 |
|
Could you bring the codecov patch coverage (currently 80.6%) closer to 100%? |
There was a problem hiding this comment.
🔵 Needs a closer look
Deleted operators can leave displayed result IDs stale until another update occurs.
Review details
Suppressed comments (1)
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:266
- Operator deletion does not emit
getViewResultOperatorsChangedStream(); the shared-model handler emits onlygetOperatorDeleteStream()for a root-map deletion. Therefore deleting a chosen/viewed operator leavesshownResultIdsstale until an unrelated result or config update, despite the method contract saying deleted operators drop immediately. Subscribe to the delete stream as well and add the corresponding deletion test.
this.workflowActionService
.getTexeraGraph()
.getViewResultOperatorsChangedStream()
.pipe(untilDestroyed(this))
.subscribe(() => {
this.refreshShownResults();
this.cdr.markForCheck();
});
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| */ | ||
| private refreshShownResults(): void { | ||
| const viewed = this.workflowActionService.getTexeraGraph().getOperatorsToViewResult(); | ||
| this.shownResultIds = this.formBindingService.getConfig().resultOperatorIds.filter(id => viewed.has(id)); |
There was a problem hiding this comment.
A question first: if the workflow's last operator has no eye (view-result) on the canvas, can a Form View reader ever see its result here?
Digging into that: a chosen sink can never pass this filter — a sink never enters the view-result set (the eye toggle excludes sinks outright, highlightedOperatorIdsExcludingSinks), while its output is materialized unconditionally (the result panel even auto-highlights a sink on completion). So an author who picks the "View Results" sink — the most natural choice — gets a card that never appears. The filter needs a sink branch (a chosen sink passes while it still exists on the graph), and #8026's picker range will need to be viewed-∪-sinks for the same reason.
There was a problem hiding this comment.
Correction to my own mechanism description after reading the compiler: the sink story above is frontend leftovers — SimpleSink no longer exists in the backend at all. The real rule is in WorkflowCompiler.expandLogicalPlan: storage = terminal operators ∪ opsToViewResult, so a workflow's last operator (any type, no eye) is materialized unconditionally. That makes the gap broader than a sink: an author who picks the workflow's final operator — the most common choice, and one that has no reason to carry the eye — is filtered out here. The fix is a terminal branch (viewed.has(id) or the operator has no outgoing links), and #8026's picker range is viewed ∪ terminals.
What changes were proposed in this PR?
Closes #8024. Part of the Form View stack (parent issue #8011), stacked on #8440 (PR12).
Shows the chosen results under the workflow that produced them, as a display filter over the canvas's view-result set (settled design): the form reads which operators the canvas already views and shows those, and it NEVER writes the canvas's view-result flags, so a normal canvas user's result-viewing is unaffected.
texera-result-table-frame), a fit-to-card visualisation (texera-visualization-panel-content) with per-result zoom, or the section shows a compact "press Run" / "no result yet" state. A step that produced nothing collapses out rather than sitting on a permanent empty card.shownResultIds= the author'sresultOperatorIdskept to only those still ingraph.getOperatorsToViewResult(); it refreshes on every result update, so a view-result toggle on the canvas (a co-editor's included) is reflected without a reload.WorkflowResultService.hasNonEmptyResult(main only hadhasAnyResult= a service exists; this checks the tuple/snapshot count so an empty result reads as "no result").Picking which results to show is an authoring action added by a later PR (#8026); opening a step to inspect it read-only is #8025. This PR is display-only.
Any related issues, documentation, discussions?
Closes #8024. Part of the Form View feature (parent issue #8011).
How was this PR tested?
Unit tests (vitest). Direct-construction tests cover the display filter (shown = chosen intersect currently-viewed; a de-viewed or deleted operator drops out; never writes view-result), the non-empty gating, the zoom clamp,
resultKey/resultVersionrebuilding a frame on a new result, and the result-update subscription.workflow-result.service.spec.tscovershasNonEmptyResult(empty vs full, paginated vs snapshot). A TestBed test covers the results section and card markup (the real table/visualisation children are covered by their own specs; their websocket-backed DI does not run in jsdom). 100% statement and function coverage on the changed source.ng test(146 tests),ng build gui, eslint and prettier all pass.Screenshot
The results section under the workflow: a table and a visualisation card after a run.
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.