Copilot commented on code in PR #8441:
URL: https://github.com/apache/texera/pull/8441#discussion_r3945680849
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -141,5 +221,70 @@
class="box"></texera-mini-map>
</div>
</section>
+
+ <!-- Results, under the workflow that produced them. The section keeps its
place before a run so
+ the answer has a visible destination. -->
+ <section class="results">
+ <div class="pc-section-head"><span class="label">Results</span></div>
+ <p
+ class="results-empty"
+ *ngIf="resultIdsToShow.length === 0">
+ {{ isRunning ? "Working…" : "Press Run and the results appear here." }}
Review Comment:
After a completed run that yields zero rows, `resultIdsToShow` is empty and
this still tells the reader to press Run. Since cards are prefiltered to
non-empty results, the `No result yet.` switch branch below is unreachable in
real use, so the required post-run empty state is never shown. Distinguish the
uninitialized state from completed/failed/killed runs here.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -141,5 +221,70 @@
class="box"></texera-mini-map>
</div>
</section>
+
+ <!-- Results, under the workflow that produced them. The section keeps its
place before a run so
+ the answer has a visible destination. -->
+ <section class="results">
+ <div class="pc-section-head"><span class="label">Results</span></div>
+ <p
+ class="results-empty"
+ *ngIf="resultIdsToShow.length === 0">
+ {{ isRunning ? "Working…" : "Press Run and the results appear here." }}
+ </p>
+ <!-- A card only for a chosen step that actually produced a result.
Whether a Python UDF
+ yields one cannot be told from the graph, so a step earns its card
at runtime rather than
+ sitting on a permanent "No result yet." (a download/publish step
never would). -->
+ <ng-container *ngFor="let id of resultIdsToShow">
+ <div class="card result">
+ <div class="result-head">
+ <span>{{ resultLabel(id) }}</span>
+ <!-- Zooming only means something once there is a picture to zoom.
-->
+ <span
+ class="result-zoom"
+ *ngIf="vizHasContent(id)">
+ <button
+ (click)="zoomResult(id, -1)"
+ [disabled]="resultZoom(id) <= 0"
+ nz-tooltip="Smaller">
Review Comment:
This icon-only button has no accessible name; `nz-tooltip` is not part of
the button's accessible-name computation. Add an `aria-label` so screen-reader
users can identify the zoom-out action.
This issue also appears on line 253 of the same file.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.html:
##########
@@ -141,5 +221,70 @@
class="box"></texera-mini-map>
</div>
</section>
+
+ <!-- Results, under the workflow that produced them. The section keeps its
place before a run so
+ the answer has a visible destination. -->
+ <section class="results">
+ <div class="pc-section-head"><span class="label">Results</span></div>
+ <p
+ class="results-empty"
+ *ngIf="resultIdsToShow.length === 0">
+ {{ isRunning ? "Working…" : "Press Run and the results appear here." }}
+ </p>
+ <!-- A card only for a chosen step that actually produced a result.
Whether a Python UDF
+ yields one cannot be told from the graph, so a step earns its card
at runtime rather than
+ sitting on a permanent "No result yet." (a download/publish step
never would). -->
+ <ng-container *ngFor="let id of resultIdsToShow">
+ <div class="card result">
+ <div class="result-head">
+ <span>{{ resultLabel(id) }}</span>
+ <!-- Zooming only means something once there is a picture to zoom.
-->
+ <span
+ class="result-zoom"
+ *ngIf="vizHasContent(id)">
+ <button
+ (click)="zoomResult(id, -1)"
+ [disabled]="resultZoom(id) <= 0"
+ nz-tooltip="Smaller">
+ <i
+ nz-icon
+ nzType="minus"></i>
+ </button>
+ <button
+ (click)="zoomResult(id, 1)"
+ [disabled]="resultZoom(id) >= 2"
+ nz-tooltip="Bigger">
+ <i
+ nz-icon
+ nzType="plus"></i>
+ </button>
+ </span>
+ </div>
+ <!-- Three cases, in order: a table (it shows its own "Empty result
set" when it has no
+ rows), a visualisation that actually drew something, or nothing
yet -- which
+ collapses to a compact line instead of a tall blank canvas. -->
+ <div
+ class="result-body"
+ [attr.data-zoom]="resultZoom(id)"
+ [ngSwitch]="true">
+ <texera-result-table-frame
+ *ngSwitchCase="isTabularResult(id)"
+ [operatorId]="id"></texera-result-table-frame>
+ <ng-container *ngSwitchCase="vizHasContent(id)">
+ <!-- Keyed on the result's version so a new result builds a new
frame: the component
+ reads its content once, at creation. -->
+ <texera-visualization-panel-content
+ *ngFor="let key of [resultKey(id)]; trackBy: trackByKey"
+ [operatorId]="id"></texera-visualization-panel-content>
Review Comment:
`VisualizationFrameContentComponent` already subscribes to result updates
and throttles redraws with a 2-second `auditTime`. Changing this key on every
update destroys and recreates the component/iframe immediately, bypassing that
throttle and making progressive results repeatedly rebuild an expensive chart.
Render the child directly and let its existing subscription redraw it.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -161,8 +228,95 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return;
}
this.wid = wid;
+ // Give the result tables a realistic height to page against, so they show
a screenful of rows
+ // instead of one. (~7 rows; the card scrolls for the rest.)
+ this.panelResizeService.changePanelSize(900, 560);
this.load(wid);
+ // A result changing bumps that operator's version (so its chart frame is
rebuilt, not reused),
+ // re-limits what the form shows to the currently-viewed set, and re-fits
the visualisations.
+ this.workflowResultService
+ .getResultUpdateStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(update => {
+ for (const operatorID of Object.keys(update ?? {})) {
+ this.resultVersion.set(operatorID,
(this.resultVersion.get(operatorID) ?? 0) + 1);
+ }
Review Comment:
`getResultUpdateStream()` emits progressive updates, but every emission
increments this key and therefore destroys/recreates the visualization iframe.
`VisualizationFrameContentComponent` already subscribes to this stream and
intentionally throttles redraws with `auditTime(2000)`, so this bypasses that
throttle and can cause repeated iframe parsing, rendering, and flicker. Keep
the frame identity stable for progressive updates and only rebuild on a true
result reset/new execution if that is still required.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -161,8 +228,95 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return;
}
this.wid = wid;
+ // Give the result tables a realistic height to page against, so they show
a screenful of rows
+ // instead of one. (~7 rows; the card scrolls for the rest.)
+ this.panelResizeService.changePanelSize(900, 560);
this.load(wid);
+ // A result changing bumps that operator's version (so its chart frame is
rebuilt, not reused),
+ // re-limits what the form shows to the currently-viewed set, and re-fits
the visualisations.
+ this.workflowResultService
+ .getResultUpdateStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(update => {
+ for (const operatorID of Object.keys(update ?? {})) {
+ this.resultVersion.set(operatorID,
(this.resultVersion.get(operatorID) ?? 0) + 1);
+ }
+ this.refreshShownResults();
Review Comment:
The displayed set is refreshed only when result data arrives. Toggling
`viewResult` after the last result update—or deleting an operator—emits graph
change streams, not a result update, so the form can keep showing a stale card
until some unrelated refresh occurs. Subscribe to the graph's view-result and
operator-delete streams (or derive this list reactively) and refresh
immediately.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -401,6 +681,268 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return rendered.resolved.binding.id;
}
+ //
---------------------------------------------------------------------------
+ // Results: the chosen steps' output, shown under the workflow that produced
it
+ //
---------------------------------------------------------------------------
+
+ public get hasResults(): boolean {
+ return this.shownResultIds.some(id =>
this.workflowResultService.hasNonEmptyResult(id));
+ }
+
+ /**
+ * The chosen steps that actually produced a result, so only those get a
card. Whether a Python
+ * UDF yields a result cannot be known from the graph -- some (a
download/publish step) never do --
+ * so a chosen step earns its card at runtime rather than sitting on a
permanent "No result yet.".
+ */
+ public get resultIdsToShow(): string[] {
+ return this.shownResultIds.filter(id =>
this.workflowResultService.hasNonEmptyResult(id));
+ }
+
+ public isTabularResult(operatorID: string): boolean {
+ return this.workflowResultService.hasPaginatedResult(operatorID);
+ }
+
+ /**
+ * Whether this step's visualisation drew something. A visualiser reserves a
fixed canvas even
+ * when empty, so gating on real content lets an empty result collapse to
the compact "No result
+ * yet" line instead of a tall blank box. Tables are excluded (they take the
tabular branch).
+ */
+ public vizHasContent(operatorID: string): boolean {
+ if (this.isTabularResult(operatorID)) {
+ return false;
+ }
+ const snapshot =
this.workflowResultService.getResultService(operatorID)?.getCurrentResultSnapshot();
+ return !!snapshot && snapshot.length > 0;
+ }
+
+ /** The operator's friendly label for a result card, falling back to its raw
id. */
+ public resultLabel(operatorID: string): string {
+ const operator =
this.workflowActionService.getTexeraGraph().getOperator(operatorID);
+ return operator ? this.formBindingService.operatorLabel(operator) :
operatorID;
+ }
+
+ public trackByKey(_: number, key: string): string {
+ return key;
+ }
+
+ /**
+ * A per-result identity that changes only when that operator's result is
genuinely new, used as
+ * the chart's *ngFor key so the frame is rebuilt (not reused) on a new
result: the chart reads
+ * its content once at creation, so a reused frame kept showing the old (or
"undefined") picture.
+ */
+ public resultKey(operatorID: string): string {
+ return operatorID + "#" + (this.resultVersion.get(operatorID) ?? 0);
+ }
+
+ public resultZoom(operatorID: string): number {
+ return this.zoomByResult.get(operatorID) ?? 1;
+ }
+
+ public zoomResult(operatorID: string, delta: number): void {
+ const next = Math.min(2, Math.max(0, this.resultZoom(operatorID) + delta));
+ this.zoomByResult.set(operatorID, next);
+ // Let the new card height land, then have the chart redraw into it --
growing the frame alone
+ // leaves the picture at its old size until something asks it to
re-measure.
+ this.cdr.detectChanges();
+ this.later(() => this.fitVisualisations(), 60);
+ }
+
+ /**
+ * Scale each visualisation to its card. They render in a same-origin srcdoc
iframe at natural
+ * size, so we inject a stylesheet to fit the content to the card and fire a
resize so chart
+ * libraries re-lay out. The operator's output is untouched.
+ */
+ /* v8 ignore start -- iframe/Plotly DOM fitting; no coverage in jsdom */
+ private fitVisualisations(): void {
+ const frames =
this.host.nativeElement.querySelectorAll<HTMLIFrameElement>(".result-body
iframe");
+ frames.forEach(frame => {
+ const apply = () => {
+ try {
+ const doc = frame.contentDocument;
+ if (!doc?.body) {
+ return;
+ }
+ if (!doc.getElementById("pc-fit")) {
+ const style = doc.createElement("style");
+ style.id = "pc-fit";
+ style.textContent = `
+ html, body { margin: 0; padding: 8px; overflow-x: hidden; }
+ .js-plotly-plot, .plot-container, .plotly, .svg-container {
width: 100% !important; height: 100% !important; }
+ img, svg, canvas, video { max-width: 100% !important; height:
auto !important; }
+ table { max-width: 100%; }
+ `;
+ doc.head?.appendChild(style);
+ }
+ const win = frame.contentWindow as (Window & { Plotly?: any }) |
null;
+ const plots = doc.querySelectorAll<HTMLElement>(".js-plotly-plot");
+ if (win?.Plotly?.Plots?.resize && plots.length) {
+ plots.forEach(plot => {
+ plot.style.width = "100%";
+ plot.style.height = "100%";
+ try {
+ win.Plotly.Plots.resize(plot);
+ } catch {
+ // A chart mid-render cannot be resized; the next call will
catch it.
+ }
+ });
+ }
+ win?.dispatchEvent(new Event("resize"));
+ } catch {
+ // A cross-origin document cannot be styled from here; leave it as
it came.
+ }
+ };
+ apply();
+ frame.addEventListener("load", apply, { once: true });
Review Comment:
Every fit call creates a new `load` listener. When the iframe has already
loaded—as with repeated zoom actions—the `{ once: true }` listener never fires
and remains attached, so callbacks accumulate and all run on a later reload.
Reuse a single load handler instead of appending one per fit.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -161,8 +228,95 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
return;
}
this.wid = wid;
+ // Give the result tables a realistic height to page against, so they show
a screenful of rows
+ // instead of one. (~7 rows; the card scrolls for the rest.)
+ this.panelResizeService.changePanelSize(900, 560);
this.load(wid);
+ // A result changing bumps that operator's version (so its chart frame is
rebuilt, not reused),
+ // re-limits what the form shows to the currently-viewed set, and re-fits
the visualisations.
+ this.workflowResultService
+ .getResultUpdateStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(update => {
+ for (const operatorID of Object.keys(update ?? {})) {
+ this.resultVersion.set(operatorID,
(this.resultVersion.get(operatorID) ?? 0) + 1);
+ }
+ this.refreshShownResults();
+ this.cdr.detectChanges();
+ this.later(() => this.fitVisualisations(), 300);
+ });
+
+ // The run clock, reusing the operator canvas's source outright rather
than timing anything
+ // here: the engine is the only thing that knows when the run really
began, so a stopwatch
+ // started at the click would drift and would be wrong after a reload.
+ this.workflowWebsocketService
+ .subscribeToEvent("ExecutionDurationUpdateEvent")
+ .pipe(
+ tap(event => (this.executionDuration = event.duration)),
+ switchMap(event => (event.isRunning ? timer(1000, 1000) : EMPTY)),
+ untilDestroyed(this)
+ )
+ .subscribe(() => {
+ this.executionDuration += 1000;
+ this.cdr.markForCheck();
+ });
+
+ // The run button's state is read from getters, so a change in
unit/connection/validity has to
+ // repaint the view. markForCheck, not detectChanges: a synchronous pass
can be thrown out of by
+ // an unrelated component's NG0100, killing the subscription.
+ this.computingUnitStatusService
+ .getSelectedComputingUnit()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => this.cdr.markForCheck());
+ this.computingUnitStatusService
+ .getStatus()
+ .pipe(untilDestroyed(this))
+ .subscribe(status => {
+ this.computingUnitStatus = status;
+ this.cdr.markForCheck();
+ });
+ this.workflowWebsocketService
+ .getConnectionStatusStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(() => this.cdr.markForCheck());
+ // Validity from the canvas's own stream, so a broken graph disables Run
("Invalid") here
+ // exactly as it does there.
+ this.validationWorkflowService
+ .getWorkflowValidationErrorStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(value => {
+ this.isWorkflowEmpty = value.workflowEmpty;
+ this.isWorkflowValid = Object.keys(value.errors).length === 0;
+ this.cdr.markForCheck();
+ });
+
+ this.executeWorkflowService
+ .getExecutionStateStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(({ current }) => {
+ this.executionState = current.state;
+ // Surface a failed run. Without this the spinner just stops and the
form gives zero
+ // feedback -- the opposite of what a reader needs. onRun() clears
runError before the next
+ // run, so a stale error never lingers.
+ if (current.state === ExecutionState.Failed) {
+ // A required input left empty is by far the commonest reason a run
fails here, and the
+ // engine reports it as an opaque "... is not contained in the
schema". Answer with the
+ // same word the field itself already shows ("required"), so the two
messages are
+ // consistent -- and it covers every operator, not just this one.
+ this.runError = this.hasEmptyRequiredInputs()
+ ? "Run failed: please fill in the required fields."
+ :
this.friendlyRunError(current.errorMessages?.[0]?.message?.trim() ?? "");
+ }
Review Comment:
A failure is only cleared by this page's `onRun()`. If a co-editor starts
the next run, the shared execution stream moves to an in-flight state without
calling `onRun()`, leaving the previous failure displayed instead of the
running status. Clear `runError` whenever the stream enters a new in-flight
state.
--
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]
