sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4366360362
##########
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:
Deleting this prop would leave every new deferred-response test green,
because they pass `latestQueryFormData` straight to the inner `Chart`. Without
the forwarding, a country response recorded while the chart was off screen is
drawn with the current region form data. Could a grid-level test supply country
`latestQueryFormData` next to region `formData` and assert the `ChartContainer`
receives the country form data unchanged?
##########
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:
This test mounts directly in `loading`, so `responseFormData` is undefined
and the chart is hidden whatever the `dashboardId` check says. Deleting
`!!dashboardId` from `keepChartOnScreen` in `Chart.tsx` would still pass it,
and the renderer-level Explore test never reaches that branch. Could it first
render an Explore chart successfully, then refresh with the same response, and
assert the slice disappears and "Waiting on database..." appears?
##########
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:
While a refresh is in flight this builds a fresh form data object, so the
retained response is transformed again, and `Echart` still sets options with
`notMerge: true` and animation on unless the dashboard-wide `isRefreshing` flag
is set. For a single-chart refresh or a native-filter change on an animated
ECharts chart (e.g. funnel), the old chart can replay its entrance animation
under the spinner, which is the flicker this PR is meant to remove. Does an
individual refresh get the animation suppression, or only a whole-dashboard
refresh?
##########
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:
For a Deck.gl Multiple Layers chart on a dashboard, a native-filter change
now keeps the old map up and drops this overlay as soon as the empty parent
query returns, but the layers only start refetching afterwards (`Multi.tsx`
reacts when `isRefreshing` flips back to false). While the layer queries are
slow, the map keeps showing the previous, wrongly filtered layers with no
loading indicator. Should this chart type keep the spinner until its layers
land, or be excluded from the kept-on-screen path?
--
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]
