sadpandajoe commented on code in PR #45234:
URL: https://github.com/apache/superset/pull/45234#discussion_r4282402185
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -272,14 +272,44 @@ function ChartRendererComponent({
prevQueriesResponseRef.current = queriesResponse;
}, [queriesResponse]);
+ const currentFormData =
+ chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
+ const vizType = currentFormData.viz_type || propVizType;
+ // Some charts fetch their own data and issue no top-level query (e.g.
deck.gl
+ // Multiple Layers, which renders its sub-layer charts), so an empty response
+ // is expected and shouldn't trigger the no-results state. Honor the chart's
+ // own enableNoResults metadata (defaults to true).
+ 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 =
+ Boolean(restProps.suppressLoadingSpinner) && chartStatus === 'loading';
+ const hasRenderableResults =
+ resultsReady || (isRefreshing && hasValidPreviousData);
Review Comment:
With a chart that has data, this now keeps the same SuperChart mounted
through `rendered → loading → success`. `handleRenderSuccess` only fires from
the loadable renderer's post-load effect, so it won't fire again for the
refreshed response. On master the no-results branch remounted the renderer
during refresh, so `chartRenderingSucceeded` and the render log fired when the
new response landed. After this change the chart sits at `success` after the
first auto-refresh and never returns to `rendered`, so
`renderedChartIdsSelector` in `DashboardContainer` drops it and the
render-completion log is skipped. Is that intended, or should the refreshed
render be reported explicitly?
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -272,14 +272,44 @@ function ChartRendererComponent({
prevQueriesResponseRef.current = queriesResponse;
}, [queriesResponse]);
+ const currentFormData =
+ chartIsStale && latestQueryFormData ? latestQueryFormData : formData;
+ const vizType = currentFormData.viz_type || propVizType;
+ // Some charts fetch their own data and issue no top-level query (e.g.
deck.gl
+ // Multiple Layers, which renders its sub-layer charts), so an empty response
+ // is expected and shouldn't trigger the no-results state. Honor the chart's
+ // own enableNoResults metadata (defaults to true).
+ 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 =
+ Boolean(restProps.suppressLoadingSpinner) && chartStatus === 'loading';
+ const hasRenderableResults =
+ resultsReady || (isRefreshing && hasValidPreviousData);
Review Comment:
Keeping the plugin mounted also hides the remount that standalone deck.gl
Scatterplot, Arc and Path used to get on refresh. `CategoricalDeckGLContainer`
only rebuilds `categories` when `payload.form_data` changes, and their v1
transforms build `payload` from `queriesData[0]`, which carries no `form_data`.
If the first response has category A and an auto-refresh brings in category B,
`categories` never gains B, and the `categories[d.cat_color]?.enabled` filter
drops every B feature. Should these charts reconcile categories from the new
response, or opt out of keeping the previous data?
##########
superset-frontend/plugins/preset-chart-deckgl/src/Multi/MultiV1.test.tsx:
##########
@@ -225,3 +225,46 @@ test('surfaces a warning in the chart when a layer fails
to load', async () => {
// The map still renders alongside the warning.
expect(screen.getByTestId('deckgl-container')).not.toBeNull();
});
+
+test('reloads its layers when a dashboard auto-refresh lands, keeping the
current ones on screen', async () => {
+ // ChartRenderer keeps this chart mounted through an auto-refresh, so the
+ // refresh itself has to trigger the reload that a remount used to.
+ const renderWith = (isRefreshing: boolean) => (
+ <Provider store={store}>
+ <ThemeProvider theme={supersetTheme}>
+ <DeckMulti {...v1Props} isRefreshing={isRefreshing} />
+ </ThemeProvider>
+ </Provider>
+ );
+ const getContainer = () => screen.getByTestId('deckgl-container');
+ const { rerender } = render(renderWith(false));
+ await waitFor(() =>
+ expect(getContainer().getAttribute('data-layers-count')).toBe('1'),
+ );
+ expect(SupersetClient.post).toHaveBeenCalledTimes(1);
+
+ rerender(renderWith(true));
+ expect(SupersetClient.post).toHaveBeenCalledTimes(1);
+
+ let resolveReload: (value: unknown) => void = () => {};
+ (SupersetClient.post as jest.Mock).mockReturnValueOnce(
+ new Promise(resolve => {
+ resolveReload = resolve;
+ }),
+ );
+ rerender(renderWith(false));
+
+ await waitFor(() => expect(SupersetClient.post).toHaveBeenCalledTimes(2));
+ // While the new layer data is in flight, the map keeps the previous layer
+ // and its fitted viewport instead of blanking back to the base viewport.
+ expect(getContainer().getAttribute('data-layers-count')).toBe('1');
+ const viewport = JSON.parse(
+ getContainer().getAttribute('data-viewport') || '{}',
+ );
+ expect(viewport.longitude).toBeLessThan(-100);
+
+ resolveReload({ json: { result: [{ data: SF_FEATURES }] } });
+ await waitFor(() =>
+ expect(getContainer().getAttribute('data-layers-count')).toBe('1'),
Review Comment:
This final assertion can't tell a reloaded layer from a retained one.
`data-layers-count` is already `1` from the first load, and the mocked
transform returns `SF_FEATURES` whatever the response holds, so the test also
passes if the refresh response is ignored. Could the reload response carry
distinct features and the test assert that the replacement layer's data is what
ends up rendered?
--
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]
