EnxDev commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4366044768
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -221,6 +221,29 @@ function Chart({
// Update on each render to accurately track render duration
renderStartTimeRef.current = Logger.getTimestamp();
+ // The form data the response in the store was queried with. A filter change
+ // updates formData as soon as the new query starts, so a chart kept on
screen
+ // draws the previous response with its own form data until the new one
+ // lands. This lives here rather than in ChartRenderer, which unmounts while
a
+ // virtualized chart is out of view. A chart mounted while its query is
+ // already in flight can't tell which form data that response belongs to.
+ // A response that failed to render keeps no form data either: drawn again
+ // while the retry is in flight, it would fail the same way.
+ const responseFormDataRef = useRef<{
+ queriesResponse: typeof queriesResponse;
+ formData: QueryFormData | undefined;
+ }>({
+ queriesResponse,
+ formData: chartStatus === 'loading' ? undefined : formData,
+ });
+ if (responseFormDataRef.current.queriesResponse !== queriesResponse) {
+ responseFormDataRef.current = { queriesResponse, formData };
Review Comment:
Extended `draws each previous response with the form data it was queried
with` with a second refresh: after the second response lands, a third query
starts and the test asserts `value=2 label=second`. Freezing the snapshot fails
it. The snapshot also comes from `latestQueryFormData` now instead of
`formData`, see the deferred-data thread. In 78d9dfa.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -282,25 +326,36 @@ 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. The dashboard's colours change without a new query, so
+ // those always come from the current form data.
+ const renderedFormData = useMemo(
+ () =>
+ isRefreshing && responseFormData
+ ? {
+ ...responseFormData,
+ ...Object.fromEntries(
+ DASHBOARD_COLOR_KEYS.map(key => [key, currentFormData[key]]),
Review Comment:
Yes, and deck.gl's viewport is the worst case of it: `DeckGLContainer`
resets its view state whenever the viewport it's given changes, so the retained
render snapped the map back to the viewport of the previous request, the kind
of jump #45234 was removing. `ChartRenderer` now takes every control that
redraws without a new query from the current form data, not only the colour
keys: the viz type's controls flagged `renderTrigger` or `dontRefreshOnChange`,
the same set Explore's staleness check ignores. `getDisplayControlNames` in
`src/explore/controlUtils` reads them from the control panel registry per viz
type. The five dashboard colour keys stay explicit since they sit in no control
panel. `draws the previous data with the values the chart set on itself`
toggles one control of each kind without querying and then refreshes. In
78d9dfa.
--
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]
