EnxDev commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4375248629
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -543,7 +592,10 @@ function Chart({
height={height}
width={width}
>
- {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
+ {showSpinner && !keepChartOnScreen
+ ? renderSpinner(databaseName)
+ : renderChartContainer()}
+ {showSpinner && keepChartOnScreen && <Loading size="s" muted />}
Review Comment:
Excluded it. On a manual refresh or a filter change, a chart that fetches
its own data (`enableNoResults: false`, which on a dashboard means deck.gl
Multiple Layers) gets the regular spinner again, so it remounts and loads its
layers from scratch as on master. Keeping the spinner up until the layers land
would need Multi to report back once they're loaded, and nothing carries that
today. An auto-refresh still keeps it mounted, as it has since #45234. The
check is `chartFetchesOwnData` in `keepChartOnScreen` in `Chart.tsx`.
This turns around the self-fetching test from the earlier round: `replaces a
self-fetching dashboard chart with the spinner through a manual refresh`
asserts the spinner and the remount, and `keeps a self-fetching dashboard chart
mounted through an auto-refresh` asserts it mounts once. In f1c72f0.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +328,40 @@ 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. 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.
+ const renderedFormData = useMemo(() => {
Review Comment:
Only a whole-dashboard refresh got it, and the old chart did redraw. Two
props changed when loading started: `isRefreshing` flipped, and the form data
was a new object. Either one makes SuperChart transform again. I fixed those
rather than the animation: `ChartRenderer` keeps the form data object the chart
is drawn with while the values are unchanged, and passes `isRefreshing` to the
plugin only for an auto-refresh, as on master. So a single-chart refresh or a
dashboard refresh no longer redraws the chart under the spinner at all. `does
not redraw a dashboard chart under the spinner when a refresh starts with
nothing changed` counts `transformProps` calls across the refresh.
A filter change still redraws once when its query starts. The render where
`formData` has already moved on but the query hasn't started yet draws the old
rows with the new filters (on master too), and the redraw under the spinner is
what pairs them back with their own form data. Skipping the animation for that
one means telling `Echart` about a single chart refreshing. It only reads the
dashboard-wide flag from the store, so that touches every ECharts chart
component. I'd keep it for a follow-up unless you think it should block this
PR. In f1c72f0.
##########
superset-frontend/src/components/Chart/Chart.test.tsx:
##########
@@ -224,3 +224,68 @@ 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', () => {
+ render(
+ <Chart
+ {...baseProps}
+ dashboardId={17}
+ chartStatus="loading"
+ queriesResponse={previousResponse}
+ suppressLoadingSpinner
+ />,
+ );
+ expect(screen.getByTestId('slice-container')).toBeInTheDocument();
+ expect(screen.queryByRole('status')).not.toBeInTheDocument();
+});
+
+test('shows only the spinner for a dashboard chart that has no previous data',
() => {
+ render(
+ <Chart
+ {...baseProps}
+ dashboardId={17}
+ chartStatus="loading"
+ queriesResponse={null}
+ />,
+ );
+ expect(screen.queryByTestId('slice-container')).not.toBeInTheDocument();
+ expect(screen.getByText('Waiting on database...')).toBeInTheDocument();
+});
+
+test('replaces an Explore chart with the spinner while it refreshes', () => {
Review Comment:
Done. The test renders the Explore chart as `rendered` first, so `Chart`
knows the response's form data, then switches it to `loading` and asserts the
slice is gone and "Waiting on database..." is shown. Deleting `!!dashboardId`
fails it. In f1c72f0.
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -877,6 +877,7 @@ const Chart = (props: ChartProps) => {
}
ownState={ownState}
queriesResponse={chart.queriesResponse ?? null}
+ latestQueryFormData={chart.latestQueryFormData}
Review Comment:
Added `passes the form data of the last request to ChartContainer next to
the current form data` to the grid `Chart.test.tsx`. The store has
`latestQueryFormData` grouped by country and `form_data` grouped by region, and
the test asserts `ChartContainer` gets the store's `latestQueryFormData` object
unchanged, next to the region `formData`. Dropping the prop fails it. In
f1c72f0.
--
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]
