sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4364674907
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -221,6 +221,29 @@ 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.
+ const responseFormDataRef = useRef<{
+ queriesResponse: typeof queriesResponse;
+ formData: QueryFormData | undefined;
+ }>({
+ queriesResponse,
+ formData: chartStatus === 'loading' ? undefined : formData,
+ });
+ if (responseFormDataRef.current.queriesResponse !== queriesResponse) {
+ responseFormDataRef.current = { queriesResponse, formData };
Review Comment:
With `DASHBOARD_VIRTUALIZATION_DEFER_DATA` on, `runQuery()` returns early
while the chart is off screen, but a query that was already in flight keeps
running. If a Dynamic Group By change (country -> region) lands while the chart
is off screen and that earlier country query then completes, this line records
the country rows against the current region `formData`. After scrolling back
in, the refresh path draws those country rows with region config (e.g. a pie
looks up a field that isn't in the rows and mislabels). Could the snapshot take
the form data of the request that produced the response, rather than whatever
`formData` is current when the response object changes?
##########
superset-frontend/src/components/Chart/ChartRenderer.test.tsx:
##########
@@ -434,13 +435,64 @@ test('renders chart during loading when
suppressLoadingSpinner has valid data',
);
});
-test('does not render chart during loading when the spinner is not
suppressed', () => {
+test('renders a dashboard chart during loading when the spinner is not
suppressed', () => {
const props = {
...requiredProps,
chartStatus: 'loading' as const,
chartAlert: undefined,
suppressLoadingSpinner: false,
queriesResponse: [{ data: [{ value: 1 }] }],
+ responseFormData: requiredProps.formData,
+ };
+
+ const { getByTestId } = render(<ChartRenderer {...props} />);
+ expect(getByTestId('mock-super-chart')).toHaveAttribute(
+ 'data-is-refreshing',
+ 'true',
+ );
+});
+
+test('does not render a previous response whose form data is unknown', () => {
Review Comment:
Now that `ChartRenderer` requires `responseFormData` to keep a chart on
screen, the existing `does not render chart during loading when last data has
errors` test (~line 562) no longer passes `responseFormData`, so it is rejected
before `hasValidPreviousData` ever checks `error`; deleting the `!hasAnyErrors`
guard would leave it green. The tests added here also omit it, so nothing
covers the stale-error guard. Could you pass `responseFormData:
requiredProps.formData` with an errored `queriesResponse` and `chartAlert`
cleared, so removing the guard fails that case?
--
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]
