Copilot commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4340017175
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -177,6 +177,28 @@ interface ChartHooks extends AsyncChartHooks {
onChartStateChange?: (chartState: AgGridChartState) => void;
}
+/**
+ * Whether a chart has a previous response it can stay on screen with while a
+ * new query is in flight.
+ */
+export function hasValidPreviousData(
+ queriesResponse: QueryData[] | null | undefined,
+ vizType: string,
+): boolean {
+ // 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 =
+ (getChartMetadataRegistry().get(vizType)?.enableNoResults ?? true)
+ ? (queriesResponse?.length ?? 0) > 0
+ : Array.isArray(queriesResponse);
Review Comment:
For charts with `enableNoResults === false`, `hasPreviousResponse` becomes
`Array.isArray(queriesResponse)`, which evaluates to `true` even for an empty
array. If the pre-first-load state for such charts is `queriesResponse: []` (as
opposed to `null/undefined`), this will incorrectly treat the chart as having
“previous data” and may keep/render the chart during its initial
load—contradicting the stated behavior (“first load … still shows the
spinner”). A more robust approach would distinguish “never successfully loaded
yet” from “self-fetching chart that has loaded once but keeps an empty
response,” e.g., by tracking a separate “hasEverRendered/hasEverSucceeded” flag
in state or by requiring `queriesResponse` to transition from null/undefined to
an array before counting it as previous data.
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -543,7 +549,10 @@ function Chart({
height={height}
width={width}
>
- {showSpinner ? renderSpinner(databaseName) : renderChartContainer()}
+ {showSpinner && !keepChartOnScreen
+ ? renderSpinner(databaseName)
+ : renderChartContainer()}
+ {showSpinner && keepChartOnScreen && <Loading size="s" muted />}
Review Comment:
The overlay spinner is rendered as a plain `<Loading />` without an explicit
overlay/centering mechanism (e.g., `position="..."` like the existing
inline-centered usage, or a wrapper with absolute positioning). This is likely
to affect layout (taking up space) and may not actually appear “on top of” the
chart as intended. Consider rendering it with an overlay positioning mode
supported by `Loading`, or wrapping it in a positioned container (absolute,
centered, higher z-index) so it truly floats over the existing chart without
shifting content.
--
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]
