sadpandajoe commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4341505862
##########
superset-frontend/src/components/Chart/ChartRenderer.refresh.test.tsx:
##########
@@ -164,7 +177,177 @@ test('shows the new data once the auto-refresh query
returns', async () => {
/>,
);
- expect(await screen.findByText('value=2')).toBeInTheDocument();
+ expect(await screen.findByText(/value=2/)).toBeInTheDocument();
+});
+
+test('keeps a dashboard chart on screen while a manual refresh query is in
flight', async () => {
Review Comment:
This manual-refresh test only asserts that `value=1` stays visible, so it
would still pass if `Chart` unmounted and remounted the plugin on `loading`.
Could a `Chart`-level test cover the self-fetching plugin (`enableNoResults`
false, `queriesResponse=[]`) going `rendered` -> `loading` -> `success` without
spinner suppression, asserting the plugin stays mounted next to the loading
indicator and mounts exactly once? Today the mount-count coverage only goes
through `ChartRenderer` with auto-refresh, so the new gate in `Chart` isn't
exercised for this case.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -543,7 +570,10 @@ function Chart({
height={height}
width={width}
>
- {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
+ {showSpinner && !keepChartOnScreen
Review Comment:
Keeping the chart mounted through a manual refresh means a Matrixify chart
no longer refetches its cells. In the Matrixify branch of `SuperChart` the grid
ignores `queriesData` and `isRefreshing`, and `MatrixifyGridCell` is memoized
on the cell form data, so with unchanged filters the parent request finishes,
the spinner clears, and every cell keeps its old data (before, the loading
branch unmounted and remounted them). Should Matrixify charts be excluded from
the kept-on-screen path, or is there a refresh signal the cells can pick up?
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +307,23 @@ 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 && hasValidPreviousData(queriesResponse, vizType));
Review Comment:
A chart whose plugin threw during render keeps its successful
`queriesResponse` with no query-level `error` (`CHART_RENDERING_FAILED` only
sets status and alert). Clicking Refresh then sets `loading`, this predicate is
true, and the same crashing response is rendered again, which dispatches
`CHART_RENDERING_FAILED` and puts the chart back into the failed state with an
error message while the new request is still in flight. Previously the refresh
showed the spinner without re-rendering the stale rows. Could retention also
exclude a response that already failed to render?
--
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]
