aminghadersohi commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4362746450
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +326,36 @@ function ChartRendererComponent({
const chartEnableNoResults =
getChartMetadataRegistry().get(vizType)?.enableNoResults ?? true;
- // A failed load leaves its errored response in the store, and the next
- // CHART_UPDATE_STARTED clears chartAlert but not queriesResponse. Checking
- // the response itself keeps that stale error from being drawn as chart data
- // while the retry is in flight.
- const hasAnyErrors = queriesResponse?.some(item => item?.error);
- // The response of a chart that fetches its own data stays empty once it has
- // loaded, so for those any response that landed counts as previous data.
- const hasPreviousResponse = chartEnableNoResults
- ? (queriesResponse?.length ?? 0) > 0
- : Array.isArray(queriesResponse);
- const hasValidPreviousData = hasPreviousResponse && !hasAnyErrors;
-
- // An auto-refresh keeps the chart on screen while its query is in flight.
- // The store holds the previous response until the new one lands, so that
- // response is what the chart renders in the meantime.
+ // A dashboard keeps the chart on screen while its query is in flight, be it
+ // an auto-refresh, a manual refresh or a filter change. The store holds the
+ // previous response until the new one lands, so that response is what the
+ // chart renders in the meantime.
const isRefreshing =
- Boolean(restProps.suppressLoadingSpinner) && chartStatus === 'loading';
+ chartStatus === 'loading' &&
+ (Boolean(restProps.suppressLoadingSpinner) ||
+ source === ChartSource.Dashboard);
const hasRenderableResults =
- resultsReady || (isRefreshing && hasValidPreviousData);
+ resultsReady ||
+ (isRefreshing &&
+ responseFormData !== undefined &&
+ hasValidPreviousData(queriesResponse, responseFormData, vizType));
+
+ // A filter change updates formData as soon as the new query starts, so the
+ // previous response is drawn with the form data it was queried with until
+ // the new one lands. The dashboard's colours change without a new query, so
+ // those always come from the current form data.
+ const renderedFormData = useMemo(
+ () =>
+ isRefreshing && responseFormData
+ ? {
+ ...responseFormData,
+ ...Object.fromEntries(
+ DASHBOARD_COLOR_KEYS.map(key => [key, currentFormData[key]]),
Review Comment:
Only the colour keys follow current formData here, so a value the chart sets
itself via setControlValue (Area chart stack toggle, point-cluster viewport) is
redrawn with its old value until the response lands. On master an auto-refresh
drew the current value.
##########
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:
Forcing this snapshot never to advance still passes all 51 Chart tests, so a
second refresh that draws the last response with the first query's form data
would go unnoticed; two refreshes in a row in one test would pin it.
--
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]
