sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4378148901
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -177,6 +184,44 @@ interface ChartHooks extends AsyncChartHooks {
onChartStateChange?: (chartState: AgGridChartState) => void;
}
+/**
+ * Whether a chart has a previous response it can stay on screen with while a
+ * new query is in flight.
+ */
+export function hasValidPreviousData(
+ queriesResponse: QueryData[] | null | undefined,
+ formData: QueryFormData,
+ vizType: string,
+): boolean {
+ // A Matrixify grid ignores the response: each cell fetches its own data when
+ // it mounts. Kept on screen, the cells would never pick up the refresh.
+ if (isMatrixifyEnabled(formData as MatrixifyFormData)) {
Review Comment:
On a dashboard with a refresh interval, a Matrixify chart used to stay drawn
during each auto-refresh (the old `hasValidPreviousData` had no Matrixify
check). Now this returns false, but auto-refresh passes
`suppressLoadingSpinner`, so `Chart.tsx` skips the regular spinner and renders
`ChartRenderer`, which hits `chartStatus === 'loading' &&
!hasRenderableResults` and returns `null`. The chart area is an empty container
with no indicator until the top-level query lands and the grid remounts. Should
`Chart` fall back to the regular spinner for charts that can't be kept on
screen even when `suppressLoadingSpinner` is set, or keep the grid drawn during
auto-refresh as before? The new Matrixify tests cover only a manual refresh and
a direct `ChartRenderer` render, not `suppressLoadingSpinner`.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +328,53 @@ 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.
- const isRefreshing =
+ // 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 isAutoRefreshing =
Boolean(restProps.suppressLoadingSpinner) && chartStatus === 'loading';
+ const isRefreshing =
+ chartStatus === 'loading' &&
+ (isAutoRefreshing || 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. What changes without a new query always comes from the
+ // current form data: the dashboard's colours, and the controls a chart sets
+ // on itself, such as an Area chart's stack toggle or a map's viewport.
+ //
+ // A plugin transforms and redraws the chart whenever it gets a new form data
+ // object, and an ECharts chart replays its entrance animation when it does.
+ // When the chart is already drawn with the same values, as it is when a
+ // refresh starts with nothing changed, it keeps the object it has.
+ const drawnFormDataRef = useRef(currentFormData);
+ const renderedFormData = useMemo(() => {
+ let nextFormData = currentFormData;
+ if (isRefreshing && responseFormData) {
+ nextFormData = { ...responseFormData };
+ [...DASHBOARD_COLOR_KEYS, ...getDisplayControlNames(vizType)].forEach(
+ key => {
+ if (key in currentFormData) {
+ nextFormData[key] = currentFormData[key];
+ } else {
+ delete nextFormData[key];
Review Comment:
Nothing exercises this `delete` branch: the refresh tests only replace one
defined `color_scheme` with another, so deleting the `else` would leave them
green while a colour setting cleared on the dashboard (no new query) is redrawn
from the old response's form data under the spinner. Could a `Chart`-level test
render a response with `color_scheme`, rerender with that key removed from
`formData` and the response unchanged, start the refresh, and assert the plugin
receives no `color_scheme`?
--
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]
