EnxDev commented on code in PR #45234:
URL: https://github.com/apache/superset/pull/45234#discussion_r4283331589
##########
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:
Not intended, thanks. Fixed in 39a9fe245e.
When an auto-refresh lands on a chart that stayed on screen, ChartRenderer
now reports the render itself. It notes whether the chart had renderable
results at `loading`, and if so it calls `handleRenderSuccess` when the status
reaches `success`. That dispatches `chartRenderingSucceeded` and logs the
render, so the chart goes back to `rendered` and stays in
`renderedChartIdsSelector`.
A chart whose previous response had no rows still shows the no-results state
during the refresh, so the plugin mounts for the new response and reports
itself the way it did on master. To keep that path and the new one from both
reporting the same response, `handleRenderSuccess` records which response it
last reported and skips a repeat.
Tests in `ChartRenderer.refresh.test.tsx`:
- a kept-on-screen chart reports nothing while `loading` and exactly one
`chartRenderingSucceeded(chartId)` and one render log once `success` lands
- a chart that refreshes from the no-results state reports once, not twice
##########
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:
Good find. Fixed in 6036c9f9dd. I went with reconciling rather than opting
these charts out, since opting out would bring back the blank map on every tick.
`CategoricalDeckGLContainer` still rebuilds everything when
`payload.form_data` changes. When it stays the same but `payload.data.features`
is a new array, which is what a refresh brings, it now merges the categories
from the new response into the legend with `reconcileCategories`:
- a new category is added with `enabled: true`, so its features get drawn
- a category that is still present keeps the visibility the viewer chose in
the legend
- a category that is gone is dropped
- if that would leave every category hidden, all of them are shown again,
the same rule the legend toggle follows
- if the set of categories is unchanged it returns the previous object, so
there is no extra state update
Tests in `CategoricalDeckGLContainer.test.tsx` cover the reconcile rules,
plus your scenario on the real component: a response with category A, then a
refresh with A and B, and both end up in the drawn layer 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:
Right, it couldn't tell them apart. Fixed in d682b7860a.
The mocked `deck_scatter` transform now builds the layer from
`queriesData[0].data` instead of returning `SF_FEATURES` every time, and the
DeckGL mock exposes each layer's data. The test checks that the first-load
positions are still drawn while the reload is in flight, then resolves the
reload with a different feature and waits until the drawn layer holds exactly
that feature. If the refresh response is ignored, the layer keeps the
first-load positions and the test fails.
--
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]
