sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4387749634
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -221,6 +222,42 @@ function Chart({
// Update on each render to accurately track render duration
renderStartTimeRef.current = Logger.getTimestamp();
+ // The form data the response in the store was queried with. A filter change
+ // updates formData as soon as the new query starts, so a chart kept on
screen
+ // draws the previous response with its own form data until the new one
+ // lands. This lives here rather than in ChartRenderer, which unmounts while
a
+ // virtualized chart is out of view. A chart mounted while its query is
+ // already in flight can't tell which form data that response belongs to.
+ // A response that failed to render keeps no form data either: drawn again
+ // while the retry is in flight, it would fail the same way.
+ //
+ // The store records the form data of the request that produced the response
+ // as latestQueryFormData. That can lag behind formData: with deferred data,
+ // a filter change moves formData on while the chart is off screen, and the
+ // response of the request already in flight lands against the newer form
+ // data. Charts rendered outside the store have no latestQueryFormData.
+ const queriedFormData =
+ latestQueryFormData && Object.keys(latestQueryFormData).length > 0
Review Comment:
The `Object.keys(latestQueryFormData).length > 0` check is what keeps an
empty request record from being used as the form data a retained response is
drawn with, but no test reaches it: every refresh test passes either a
populated `latestQueryFormData` or omits the prop. Removing the length check
would leave the suite green, and a chart whose store entry holds `{}` would
then redraw its previous rows against an empty form data object (no viz
settings) while the refresh runs. Could a `Chart`-level test render
successfully with `latestQueryFormData={{}}` and a labelled `formData`, refresh
with the same response, and assert the retained chart still draws with the
labelled `formData`?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
