EnxDev commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4377418700
##########
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];
+ }
+ },
+ );
+ }
+ return isEqual(nextFormData, drawnFormDataRef.current)
Review Comment:
The callback should stay stable. `setDataMaskCallback` now reads the form
data through a ref, so `hooks` no longer changes when the store records new
form data objects. `does not redraw a dashboard chart under the spinner when
its query records new form data objects` starts a query whose
`latestQueryFormData` is a new object with the same values and asserts the
plugin isn't transformed again. Putting `formData` and `latestQueryFormData`
back in the deps fails it.
On a native-filter change that alone doesn't remove the redraw under the
spinner, though. `Dashboard` triggers the query from an effect, so there is a
render where `formData` already has the new filters and the chart is still
`rendered`. That render draws the old rows with the new filters, on master as
well. When the query starts, the kept chart goes back to the form data its rows
were queried with, which differs from what it was just drawn with, so it
transforms in that render whatever `hooks` does. That is the one redraw I
mentioned in the animation thread.
Removing it means drawing a dashboard chart with its response's form data
even when no query is in flight, so the render before the query never draws the
new filters. That changes every dashboard chart at rest, not only one under the
spinner: any value that changes without a query and isn't a display control
would keep its old value until the next query lands, and in edit mode
`applyCharts` doesn't query on a filter change at all. I'd rather do that in
its own PR. In 71e9e40.
##########
superset-frontend/src/components/Chart/Chart.test.tsx:
##########
@@ -224,3 +224,78 @@ test('shows the semantic-layer disabled response while
dashboard metadata is una
screen.getByText('Semantic layers are not enabled.'),
).toBeInTheDocument();
});
+
+const previousResponse = [{ data: [{ value: 1 }] }] as Parameters<
+ typeof Chart
+>[0]['queriesResponse'];
+
+test('keeps a dashboard chart on screen under the spinner while it refreshes',
() => {
+ const { rerender } = render(
+ <Chart
+ {...baseProps}
+ dashboardId={17}
+ chartStatus="rendered"
+ queriesResponse={previousResponse}
+ />,
+ );
+ rerender(
+ <Chart
+ {...baseProps}
+ dashboardId={17}
+ chartStatus="loading"
+ queriesResponse={previousResponse}
+ />,
+ );
+ expect(screen.getByTestId('slice-container')).toBeInTheDocument();
+ expect(screen.getByRole('status')).toBeInTheDocument();
+ expect(screen.queryByText('Waiting on database...')).not.toBeInTheDocument();
+});
+
+test('shows no spinner over a dashboard chart during an auto-refresh', () => {
Review Comment:
Done. It renders as `rendered` with the response first, then rerenders as
`loading` with `suppressLoadingSpinner`, and asserts the slice is still there
and `queryByRole('status')` is null. Dropping `showSpinner` from the overlay
condition fails it. In 71e9e40.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -435,6 +474,25 @@ function Chart({
const isLoading = chartStatus === 'loading';
// Suppress spinner during auto-refresh to avoid visual flicker
const showSpinner = isLoading && !suppressLoadingSpinner;
+ // A dashboard chart with a previous response stays on screen while it
+ // refreshes, and the spinner is drawn over it instead of replacing it.
+ // A chart that fetches its own data (deck.gl Multiple Layers) only starts
+ // reloading it once its own query lands and the spinner is gone, so kept on
+ // screen it would show the previous data with no sign of the reload. It gets
+ // the regular spinner and loads its data again when it mounts.
+ const chartFetchesOwnData =
+ getChartMetadataRegistry().get(formData.viz_type || vizType)
+ ?.enableNoResults === false;
+ const keepChartOnScreen =
+ isLoading &&
+ !!dashboardId &&
+ !chartFetchesOwnData &&
+ responseFormData !== undefined &&
+ hasValidPreviousData(
Review Comment:
Added `replaces a Matrixify dashboard chart with the spinner while it
refreshes` to `Chart.test.tsx`. It renders a Matrixify dashboard chart as
`rendered` with a response, rerenders it as `loading`, and asserts "Waiting on
database..." shows and the slice is gone. Deleting the `hasValidPreviousData`
call from `keepChartOnScreen` fails it. In 71e9e40.
--
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]
