EnxDev commented on code in PR #45541:
URL: https://github.com/apache/superset/pull/45541#discussion_r4366045229
##########
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:
Yes. The snapshot now takes `latestQueryFormData`, which `exploreJSON`
records as soon as the request starts, so it is the form data of the only
request whose response can still land: the controller check drops stale ones.
In your scenario the region query never starts while the chart is off screen,
so `latestQueryFormData` stays country while `formData` moves on, and the
country rows are paired with country form data. The dashboard wasn't passing
`latestQueryFormData` to `Chart` before; it does now. When it's empty, for a
chart rendered outside the store, the snapshot falls back to `formData`. `pairs
a response that lands while the chart is off screen with the request that
produced it` walks the sequence with both virtualization flags on. In 78d9dfa.
##########
superset-frontend/src/components/Chart/ChartRenderer.test.tsx:
##########
@@ -434,13 +435,64 @@ test('renders chart during loading when
suppressLoadingSpinner has valid data',
);
});
-test('does not render chart during loading when the spinner is not
suppressed', () => {
+test('renders a dashboard chart during loading when the spinner is not
suppressed', () => {
const props = {
...requiredProps,
chartStatus: 'loading' as const,
chartAlert: undefined,
suppressLoadingSpinner: false,
queriesResponse: [{ data: [{ value: 1 }] }],
+ responseFormData: requiredProps.formData,
+ };
+
+ const { getByTestId } = render(<ChartRenderer {...props} />);
+ expect(getByTestId('mock-super-chart')).toHaveAttribute(
+ 'data-is-refreshing',
+ 'true',
+ );
+});
+
+test('does not render a previous response whose form data is unknown', () => {
Review Comment:
Added `responseFormData: requiredProps.formData` to that test; `chartAlert`
was already undefined there. Removing the `!hasAnyErrors` guard fails it now.
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]
