fix: chart rendering race condition and homepage connection reset (#40065)

Co-authored-by: Geidō <60598000+geido@users.noreply.github.com>

superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts

@@ -17,59 +17,109 @@
  * under the License.
  */
 
-import Loadable from 'react-loadable';
-import { ComponentClass } from 'react';
+import { ReactElement, useEffect, useRef, useState } from 'react';
 
 export type LoadableRendererProps = {
-  onRenderFailure?: Function;
-  onRenderSuccess?: Function;
+  onRenderFailure?: (error: Error) => void;
+  onRenderSuccess?: () => void;
 };
 
-const defaultProps = {
-  onRenderFailure() {},
-  onRenderSuccess() {},
+type LoaderMap<Exports> = {
+  [K in keyof Exports]: () => Promise<Exports[K]> | Exports[K];
 };
 
-export interface LoadableRenderer<Props>
-  extends
-    ComponentClass<Props & LoadableRendererProps>,
-    Loadable.LoadableComponent {}
-
-export default function createLoadableRenderer<
-  Props,
-  Exports extends { [key: string]: any },
->(options: Loadable.OptionsWithMap<Props, Exports>): LoadableRenderer<Props> {
-  const LoadableRenderer = Loadable.Map<Props, Exports>(
-    options,
-  ) as LoadableRenderer<Props>;
-
-  // Extends the behavior of LoadableComponent to provide post-render listeners
-  class CustomLoadableRenderer extends LoadableRenderer {
-    static defaultProps: object;
-
-    componentDidMount() {
-      this.afterRender();
-    }
-
-    componentDidUpdate() {
-      this.afterRender();
-    }
-
-    afterRender() {
-      const { loaded, loading, error } = this.state;
-      const { onRenderFailure, onRenderSuccess } = this.props;
-      if (!loading) {
-        if (error) {
-          (onRenderFailure as Function)(error);
-        } else if (loaded && Object.keys(loaded).length > 0) {
-          (onRenderSuccess as Function)();
-        }
-      }
-    }
-  }
-
-  CustomLoadableRenderer.defaultProps = defaultProps;
-  CustomLoadableRenderer.preload = LoadableRenderer.preload;
-
-  return CustomLoadableRenderer;
+export interface LoadingProps {
+  error?: { toString(): string };
+}
+
+export interface LoadableOptions<Props, Exports> {
+  loader: LoaderMap<Exports>;
+  loading: (loadingProps: LoadingProps) => ReactElement | null;
+  render: (loaded: Exports, props: Props) => ReactElement;
+}
+
+export interface LoadableRenderer<Props> {
+  (props: Props & LoadableRendererProps): ReactElement | null;
+  preload: () => Promise<unknown>;
+  displayName?: string;
+}
+
+export default function createLoadableRenderer<Props, Exports>(
+  options: LoadableOptions<Props, Exports>,
+): LoadableRenderer<Props> {
+  let promise: Promise<Exports> | null = null;
+  let cachedResult: Exports | null = null;
+  let cachedError: Error | null = null;
+
+  const load = (): Promise<Exports> => {
+    if (promise) return promise;
+    const keys = Object.keys(options.loader) as (keyof Exports)[];
+    promise = Promise.all(
+      keys.map(key => Promise.resolve(options.loader[key]())),
+    ).then(
+      values => {
+        const loaded = {} as Exports;
+        keys.forEach((key, i) => {
+          loaded[key] = values[i] as Exports[typeof key];
+        });
+        cachedResult = loaded;
+        return loaded;
+      },
+      err => {
+        cachedError = err instanceof Error ? err : new Error(String(err));
+        throw cachedError;
+      },
+    );
+    return promise;
+  };
+
+  const Renderer: LoadableRenderer<Props> = props => {
+    const [state, setState] = useState<{
+      loaded: Exports | null;
+      error: Error | null;
+    }>(() => ({ loaded: cachedResult, error: cachedError }));
+
+    useEffect(() => {
+      if (state.loaded || state.error) return undefined;
+      let cancelled = false;
+      load().then(
+        loaded => {
+          if (!cancelled) setState({ loaded, error: null });
+        },
+        err => {
+          if (!cancelled) setState({ loaded: null, error: err });
+        },
+      );
+      return () => {
+        cancelled = true;
+      };
+    }, [state.loaded, state.error]);
+
+    // Keep callback refs current without retriggering the post-load effect on
+    // every prop update.
+    const onRenderSuccessRef = useRef(props.onRenderSuccess);
+    const onRenderFailureRef = useRef(props.onRenderFailure);
+    onRenderSuccessRef.current = props.onRenderSuccess;
+    onRenderFailureRef.current = props.onRenderFailure;
+
+    useEffect(() => {
+      if (state.error) {
+        onRenderFailureRef.current?.(state.error);
+      } else if (state.loaded && Object.keys(state.loaded).length > 0) {
+        onRenderSuccessRef.current?.();
+      }
+    }, [state.loaded, state.error]);
+
+    if (state.error) {
+      return options.loading({ error: state.error });
+    }
+    if (!state.loaded) {
+      return options.loading({});
+    }
+    return options.render(state.loaded, props as Props);
+  };
+
+  Renderer.preload = load;
+
+  return Renderer;
 }

superset-frontend/src/components/Chart/chartAction.ts

@@ -780,6 +780,15 @@ export function exploreJSON(
         handleChartDataResponse(response, json, useLegacyApi),
       )
       .then(queriesResponse => {
+        // Drop stale responses: if a newer query has started for this chart,
+        // its controller will have replaced ours in state, so ignore this
+        // response to avoid clobbering newer data with older results.
+        if (key != null) {
+          const currentController = getState().charts?.[key]?.queryController;
+          if (currentController && currentController !== controller) {
+            return undefined;
+          }
+        }
         (queriesResponse as QueryData[]).forEach(
           (resultItem: QueryData & { applied_filters?: JsonObject[] }) =>
             dispatch(
@@ -825,6 +834,15 @@ export function exploreJSON(
             );
           }
 
+          // Drop stale failures the same way we drop stale successes,
+          // so a slow earlier request can't mark a newer one as failed.
+          if (key != null) {
+            const currentController = getState().charts?.[key]?.queryController;
+            if (currentController && currentController !== controller) {
+              return undefined;
+            }
+          }
+
           if (isFeatureEnabled(FeatureFlag.GlobalAsyncQueries)) {
             // In async mode we just pass the raw error response through
             return dispatch(

superset-frontend/src/components/Chart/chartActions.test.ts

@@ -156,6 +156,78 @@ describe('chart actions', () => {
       .mockImplementation((data: unknown) => Promise.resolve(data));
   });
 
+  test('should drop stale success dispatches when a newer controller has replaced ours in state', async () => {
+    const chartKey = 'stale_success_test';
+    const formData: Partial<QueryFormData> = {
+      slice_id: 456,
+      datasource: 'table__1',
+      viz_type: 'table',
+    };
+    // A controller belonging to a *newer* in-flight request, already stored
+    // in state by the time this thunk's response resolves.
+    const newerController = new AbortController();
+    const state: MockState = {
+      charts: {
+        [chartKey]: {
+          queryController: newerController,
+        },
+      },
+      common: {
+        conf: {
+          SUPERSET_WEBSERVER_TIMEOUT: 60,
+        },
+      },
+    };
+    const getState = jest.fn(() => state);
+    const dispatchMock = jest.fn();
+    const getChartDataRequestSpy = jest
+      .spyOn(actions, 'getChartDataRequest')
+      .mockResolvedValue({
+        response: { status: 200 } as Response,
+        json: { result: [{ data: [{ stale: true }] }] },
+      });
+    const handleChartDataResponseSpy = jest
+      .spyOn(actions, 'handleChartDataResponse')
+      .mockResolvedValue([{ data: [{ stale: true }] }]);
+    const updateDataMaskSpy = jest
+      .spyOn(dataMaskActions, 'updateDataMask')
+      .mockReturnValue({ type: 'UPDATE_DATA_MASK' } as ReturnType<
+        typeof dataMaskActions.updateDataMask
+      >);
+    const getQuerySettingsStub = jest
+      .spyOn(exploreUtils, 'getQuerySettings')
+      .mockReturnValue([false, () => {}] as unknown as ReturnType<
+        typeof exploreUtils.getQuerySettings
+      >);
+
+    try {
+      const thunkAction = actions.exploreJSON(
+        formData as QueryFormData,
+        false,
+        undefined,
+        chartKey,
+      );
+      await thunkAction(
+        dispatchMock as unknown as actions.ChartThunkDispatch,
+        getState as unknown as () => actions.RootState,
+        undefined,
+      );
+
+      // CHART_UPDATE_STARTED is fine (it ran before the gate),
+      // but CHART_UPDATE_SUCCEEDED must NOT have fired with the stale data.
+      const dispatchedTypes = dispatchMock.mock.calls.map(
+        ([action]) => action?.type,
+      );
+      expect(dispatchedTypes).toContain(actions.CHART_UPDATE_STARTED);
+      expect(dispatchedTypes).not.toContain(actions.CHART_UPDATE_SUCCEEDED);
+    } finally {
+      getChartDataRequestSpy.mockRestore();
+      handleChartDataResponseSpy.mockRestore();
+      updateDataMaskSpy.mockRestore();
+      getQuerySettingsStub.mockRestore();
+    }
+  });
+
   test('should defer abort of previous controller to avoid Redux state mutation', async () => {
     jest.useFakeTimers();
     const chartKey = 'defer_abort_test';

superset-frontend/src/embedded/index.tsx

@@ -18,7 +18,7 @@
  */
 import 'src/public-path';
 
-import { lazy, StrictMode, Suspense, useEffect } from 'react';
+import { lazy, Suspense, useEffect } from 'react';
 import { createRoot, type Root } from 'react-dom/client';
 import { BrowserRouter as Router, Route } from 'react-router-dom';
 import { Global } from '@emotion/react';
@@ -197,11 +197,7 @@ function start() {
       if (!root) {
         root = createRoot(appMountPoint);
       }
-      root.render(
-        <StrictMode>
-          <EmbeddedApp />
-        </StrictMode>,
-      );
+      root.render(<EmbeddedApp />);
     },
     err => {
       // something is most likely wrong with the guest token; reset the guard

superset-frontend/src/views/index.tsx

@@ -18,7 +18,6 @@
  */
 import 'src/public-path';
 
-import { StrictMode } from 'react';
 import { createRoot } from 'react-dom/client';
 import { logging } from '@apache-superset/core/utils';
 import initPreamble from 'src/preamble';
@@ -32,11 +31,7 @@ if (appMountPoint) {
       await initPreamble();
     } finally {
       const { default: App } = await import(/* webpackMode: "eager" */ './App');
-      root.render(
-        <StrictMode>
-          <App />
-        </StrictMode>,
-      );
+      root.render(<App />);
     }
   })().catch(err => {
     logging.error('Unhandled error during app initialization', err);

superset-frontend/src/views/menu.tsx

@@ -20,7 +20,6 @@ import 'src/public-path';
 
 // Menu App. Used in views that do not already include the Menu component in the layout.
 // eg, backend rendered views
-import { StrictMode } from 'react';
 import { Provider } from 'react-redux';
 import { createRoot } from 'react-dom/client';
 import { BrowserRouter } from 'react-router-dom';
@@ -46,26 +45,24 @@ const emotionCache = createCache({
 });
 
 const app = (
-  <StrictMode>
-    <CacheProvider value={emotionCache}>
-      <ThemeProvider theme={theme}>
-        <Provider store={store}>
-          <BrowserRouter>
-            <QueryParamProvider
-              adapter={ReactRouter5Adapter}
-              options={{
-                searchStringToObject: querystring.parse,
-                objectToSearchString: (object: Record<string, any>) =>
-                  querystring.stringify(object, { encode: false }),
-              }}
-            >
-              <Menu data={menu} />
-            </QueryParamProvider>
-          </BrowserRouter>
-        </Provider>
-      </ThemeProvider>
-    </CacheProvider>
-  </StrictMode>
+  <CacheProvider value={emotionCache}>
+    <ThemeProvider theme={theme}>
+      <Provider store={store}>
+        <BrowserRouter>
+          <QueryParamProvider
+            adapter={ReactRouter5Adapter}
+            options={{
+              searchStringToObject: querystring.parse,
+              objectToSearchString: (object: Record<string, any>) =>
+                querystring.stringify(object, { encode: false }),
+            }}
+          >
+            <Menu data={menu} />
+          </QueryParamProvider>
+        </BrowserRouter>
+      </Provider>
+    </ThemeProvider>
+  </CacheProvider>
 );
 
 const menuMountPoint = document.getElementById('app-menu');