Copilot commented on code in PR #9021:
URL: https://github.com/apache/texera/pull/9021#discussion_r4319581441
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -367,15 +363,21 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
.pipe(untilDestroyed(this))
.subscribe(() => this.syncSelectionFromHighlight());
- // 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.
+ // A result update re-limits what the form shows to the currently-viewed
set and re-fits the
+ // visualisations. The chart frame itself is left alone: it redraws in
place on its own
+ // operator's updates (see the visualization frame, at most once per two
seconds), and an
+ // operator's result service lives until the page is left -- a re-run
replaces the snapshot
+ // inside it, nothing clears it -- so there is never a new frame to build
mid-session.
+ // This used to bump a per-operator version that keyed the frame, meaning
to rebuild it rather
+ // than reuse it. It could not do that: every visualisation operator takes
its whole input
+ // before it draws (`process_table`, single-snapshot output), so the one
update that carries a
+ // picture is also the one that mounts the frame, and a second one never
comes. Measured on a
+ // run: two updates for the chart's operator, one picture, one frame --
with the key and
+ // without it. Dead machinery, so it goes.
Review Comment:
This comment encodes a lot of implementation assumptions and measured
behavior (backend emission semantics, result-service lifecycle, “nothing clears
it”, snapshot timing). That can become stale and will be hard to maintain
in-line. Consider condensing this to the minimal invariant needed for future
readers and linking to #8759 (and/or the related PR discussion) for the deeper
rationale/measurements.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts:
##########
@@ -2524,13 +2513,20 @@ describe("WorkflowFormComponent", () => {
vi.useRealTimers();
});
- it("bumps the result version and re-fits on a result update", () => {
+ // An update to a result the form already shows reaches the frame in place
(the frame redraws on
+ // its operator's updates); the form only re-limits the shown set and
re-fits. It used to keep a
+ // per-operator version here as well, to key the frame on, which nothing
needs.
+ it("re-limits the shown results and re-fits on a result update, and
rebuilds nothing", () => {
vi.useFakeTimers();
build(formViewWorkflow).ngOnInit();
+ const refresh = vi.spyOn(component as any, "refreshShownResults");
const fit = vi.spyOn(component as any,
"fitVisualisations").mockImplementation(() => {});
h.resultUpdateStream.next({ "op-1": {} });
- expect(component.resultKey("op-1")).toBe("op-1#1");
+ h.resultUpdateStream.next({ "op-1": {} });
+
+ expect(refresh).toHaveBeenCalledTimes(2);
+ expect(fit).not.toHaveBeenCalled();
vi.advanceTimersByTime(300);
expect(fit).toHaveBeenCalled();
vi.useRealTimers();
Review Comment:
The test name claims “rebuilds nothing”, but the assertions only verify
`refreshShownResults` calls and that `fitVisualisations` is delayed. It doesn’t
actually assert that the visualization frame component instance was not
destroyed/recreated (which is the core behavioral change after removing the
keyed `*ngFor`). Consider asserting DOM/component instance stability across
updates (e.g., capture the `texera-visualization-panel-content`
fixture/component instance, emit updates, and verify it’s the same instance /
`ngOnDestroy` wasn’t called).
##########
frontend/src/app/workspace/component/visualization-panel-content/visualization-frame-content.component.ts:
##########
@@ -45,12 +45,15 @@ export class VisualizationFrameContentComponent implements
AfterContentInit {
// attempt to draw chart immediately
this.drawChart();
- // setup an event lister that re-draws the chart content every (n)
milliseconds
- // auditTime makes sure the first re-draw happens after (n) milliseconds
has elapsed
+ // Re-draw on this operator's result updates, at most once per
UPDATE_INTERVAL_MS: auditTime
+ // holds the first re-draw until the interval has elapsed. Filter BEFORE
auditing: an update
+ // event carries only the operators whose result changed, so with the
audit first a window that
+ // closed on another operator's event handed the filter that event, and
this operator's pending
+ // re-draw was lost -- the last update of a run showed the previous
picture for good.
this.workflowResultService
.getResultUpdateStream()
- .pipe(auditTime(VisualizationFrameContentComponent.UPDATE_INTERVAL_MS))
.pipe(filter(rec => this.operatorId !== undefined &&
rec[this.operatorId] !== undefined))
+ .pipe(auditTime(VisualizationFrameContentComponent.UPDATE_INTERVAL_MS))
.pipe(untilDestroyed(this))
Review Comment:
Using multiple sequential `.pipe(...)` calls makes it easier to accidentally
reintroduce the incorrect operator ordering later. Consider collapsing these
into a single `pipe(filter(...), auditTime(...), untilDestroyed(this))` so the
intended order (“filter before audit”) is visually enforced in one place.
--
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]
