Repository navigation
fix(workflow-form): redraw a result's chart in place, and drop the key that was meant to rebuild it - #9021
Open
yangzhang75 wants to merge 2 commits into
Open
fix(workflow-form): redraw a result's chart in place, and drop the key that was meant to rebuild it#9021yangzhang75 wants to merge 2 commits into
yangzhang75 wants to merge 2 commits into
Conversation
Contributor
Automated Reviewer SuggestionsBased on the
|
Yicong-Huang
added
release/v1.3
Contributor
Backport auto-label reportThis
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9021 +/- ##
============================================
+ Coverage 91.75% 91.77% +0.02%
Complexity 5024 5024
============================================
Files 1281 1281
Lines 56747 56740 -7
Branches 7452 7449 -3
============================================
+ Hits 52067 52072 +5
+ Misses 2802 2792 -10
+ Partials 1878 1876 -2
*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:
|
mengw15
removed
release/v1.2
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
refreshShownResultscalls… · New.pipe(...)calls makes it easier to accidentally reintroduce the… · New
What changed in this PR
Fixes a latent redraw ordering issue in visualization frames and removes the unused “result version key” mechanism from the Form View chart frames.
Changes:
- Reorders RxJS operators in
VisualizationFrameContentComponentso per-operator filtering happens beforeauditTime, preventing missed redraws. - Removes per-operator
resultVersion/resultKey/trackByKeylogic and simplifies the Form View template to render the visualization frame directly. - Updates/adjusts unit tests to cover the redraw ordering case and remove the now-obsolete keying behavior.
| File | Description |
|---|---|
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts | Removes result-version keying logic and updates result-update handling/comments. |
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.html | Drops the *ngFor keyed frame workaround and renders the visualization panel content directly. |
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts | Removes tests for the deleted keying API and refocuses result-update assertions. |
| frontend/src/app/workspace/component/visualization-panel-content/visualization-frame-content.component.ts | Filters result-update events before throttling to avoid losing pending redraws. |
| frontend/src/app/workspace/component/visualization-panel-content/visualization-frame-content.component.spec.ts | Adds coverage for the “interleaving operator update” redraw-loss scenario. |
Review effort: Lite
Copilot stopped reviewing on behalf of
mengw15 due to an error
October 19, 2026 22:41
yangzhang75
added a commit
to yangzhang75/texera
that referenced
this pull request
Nov 2, 2026
…and tidy the subscription (review) Copilot on apache#9021: the unit test's "rebuilds nothing" was only a name, so the rendered spec now holds the frame's component instance and host element across two updates for its step and checks both are the same objects afterwards; the frame's three .pipe() calls are one pipe, so the filter-before-audit order is visible in one place; and the result-update comment is cut to the invariant, with the measurements left on apache#8759. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
…y that was meant to rebuild it The visualization frame audited before it filtered. An update event carries only the operators whose result changed, so a window that closed on another operator's event was handed that event by the filter and dropped, taking this operator's pending redraw with it: the last update of a run could leave the previous picture showing for good. Filter first. The canvas result panel uses the same frame and gains the same reliability. The Form View keyed each chart frame by a per-operator version, bumped on every result update, meaning to rebuild the frame 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 update that carries a picture is the one that mounts the frame, and a second one never arrives; measured on a run, the chart's operator sees two updates, one picture and one frame, with the key and without it. A re-run replaces the snapshot inside the operator's result service, which lives until the page is left, so there is no new frame to key on there either. The machinery never fired, so it goes. No visible change: this fixes a latent ordering bug and removes code that cannot run. Closes apache#8759 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
…and tidy the subscription (review) Copilot on apache#9021: the unit test's "rebuilds nothing" was only a name, so the rendered spec now holds the frame's component instance and host element across two updates for its step and checks both are the same objects afterwards; the frame's three .pipe() calls are one pipe, so the filter-before-audit order is visible in one place; and the result-update comment is cut to the invariant, with the measurements left on apache#8759. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY
yangzhang75
force-pushed
the
fix/form-result-frame-churn
branch
from
November 12, 2026 21:43
0b43bf1 to
1a216cd
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Closes #8759. Part of the Form View feature (#8011); from the #8439 refinements (the result-frame item, raised in the #8441 review and deferred there).
Two things about the chart frame a result card shows, neither of which a reader can see today. The issue reads as a visible fault -- a chart recreated on every emission, flickering through a run -- and that is not what happens: it was written from the code, and the code cannot do it. What is really there is a latent ordering bug and a piece of machinery that never fires.
The frame audited before it filtered.
VisualizationFrameContentComponentredraws on the result-update stream, throttled withauditTime, and the filter for its own operator ran after the audit. An update event carries only the operators whose result changed, so a window that closed on another operator's event handed the filter that event, which dropped it -- and this operator's pending redraw went with it. The last update of a run could leave the previous picture showing for good. The filter now runs first, so only this operator's updates enter the audit and what comes out is the redraw it was waiting on. The canvas result panel mounts the same component and gains the same reliability.The Form View keyed each chart frame on a per-operator version, bumped on every result update, so that a changed result would rebuild the frame rather than reuse it (the frame reads its content once, at creation). It could not do that. Every visualisation operator takes its whole input before it draws (
process_table, single-snapshot output ports), so the update that carries a picture is the same one that mounts the frame, and a second one never arrives. A re-run does not need the key either: it replaces the snapshot inside the operator's result service, which lives until the page is left, and a result that is cleared takes the frame down throughvizHasContent. The key, the map behind it and the*ngForthat carried it go.So: no visible change. A latent correctness fix, and the removal of code that cannot run.
Any related issues, documentation, discussions?
Closes #8759, which came out of the #8441 review (where the key was introduced, with the Copilot threads about it left open for this fix). Part of #8439 and the Form View feature #8011.
The issue's "expected" -- bump the key only for a genuinely new result, and handle
clearResults-- is not what this does, because measuring showed there is nothing for a key to do.clearResults()runs when a workspace is torn down and when a computing unit is switched, not during a run.Worth knowing, not changed here: the backend emits an update only for an operator whose tuple count changed, so a single-row visualiser that replaces its HTML at the same count produces no event at all, on the canvas and the form alike. That is a separate gap and a follow-up candidate.
How was this PR tested?
Unit tests (vitest). The frame's spec: a redraw pending for this operator survives a later emission for another operator -- this operator's update, half the window, another operator's event, the rest of the window, and the picture that lands is this operator's new one -- which is the case the old order lost; the existing redraw-after-the-window and ignore-other-operators tests still pass. The form's spec: a result update re-limits the shown results and re-fits the visualisations and rebuilds nothing, and the test that pinned the removed key is gone with it.
By hand, on two dev builds of the same base commit, one with this change and one without: a slow Python UDF source (40 rows, one every 0.4 s, batch size 1) feeding a pie chart, both results shown on the Form View. Counted off the websocket, the chart's operator receives two result updates per run and only the last carries a picture; counted in the DOM, the chart component is mounted exactly once per run -- on both builds, and twice over two runs on both builds. That measurement is why the key is described above as machinery that never fires, and why this PR has no before/after recording: there is nothing on screen to tell the two builds apart.
Each guard was deletion-checked and turned a named test red (the audit/filter order; the redraw itself). eslint, prettier and the production (AOT) build pass; the full frontend suite passes.
Was this PR authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Code (Anthropic). Co-authored with Claude, reviewed line by line by the author before submission.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY