From a62bf2b0bb73b8c0f354d78e8f70e6ffae32533f Mon Sep 17 00:00:00 2001 From: Mehmet Salih Yavuz Date: Thu, 14 May 2026 17:10:11 +0300 Subject: [PATCH] fix: chart rendering race condition and homepage connection reset (#40065) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Geidō <60598000+geido@users.noreply.github.com> --- .../components/createLoadableRenderer.ts | 148 ++++++++++++------ .../src/components/Chart/chartAction.ts | 18 +++ .../src/components/Chart/chartActions.test.ts | 72 +++++++++ superset-frontend/src/embedded/index.tsx | 8 +- superset-frontend/src/views/index.tsx | 7 +- superset-frontend/src/views/menu.tsx | 39 +++-- 6 files changed, 210 insertions(+), 82 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts b/superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts index 535d0129748..c96349eb709 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/createLoadableRenderer.ts +++ b/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 = { + [K in keyof Exports]: () => Promise | Exports[K]; }; -export interface LoadableRenderer - extends - ComponentClass, - Loadable.LoadableComponent {} - -export default function createLoadableRenderer< - Props, - Exports extends { [key: string]: any }, ->(options: Loadable.OptionsWithMap): LoadableRenderer { - const LoadableRenderer = Loadable.Map( - options, - ) as LoadableRenderer; - - // 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 { + loader: LoaderMap; + loading: (loadingProps: LoadingProps) => ReactElement | null; + render: (loaded: Exports, props: Props) => ReactElement; +} + +export interface LoadableRenderer { + (props: Props & LoadableRendererProps): ReactElement | null; + preload: () => Promise; + displayName?: string; +} + +export default function createLoadableRenderer( + options: LoadableOptions, +): LoadableRenderer { + let promise: Promise | null = null; + let cachedResult: Exports | null = null; + let cachedError: Error | null = null; + + const load = (): Promise => { + 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 => { + 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; } diff --git a/superset-frontend/src/components/Chart/chartAction.ts b/superset-frontend/src/components/Chart/chartAction.ts index e5d9345b1cf..9ba8b8dfd55 100644 --- a/superset-frontend/src/components/Chart/chartAction.ts +++ b/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( diff --git a/superset-frontend/src/components/Chart/chartActions.test.ts b/superset-frontend/src/components/Chart/chartActions.test.ts index f91a9d75194..6379326ad5e 100644 --- a/superset-frontend/src/components/Chart/chartActions.test.ts +++ b/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 = { + 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'; diff --git a/superset-frontend/src/embedded/index.tsx b/superset-frontend/src/embedded/index.tsx index c49b282bda9..1a4a9eebcc9 100644 --- a/superset-frontend/src/embedded/index.tsx +++ b/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( - - - , - ); + root.render(); }, err => { // something is most likely wrong with the guest token; reset the guard diff --git a/superset-frontend/src/views/index.tsx b/superset-frontend/src/views/index.tsx index 456c1f09e92..e583f2bad86 100644 --- a/superset-frontend/src/views/index.tsx +++ b/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( - - - , - ); + root.render(); } })().catch(err => { logging.error('Unhandled error during app initialization', err); diff --git a/superset-frontend/src/views/menu.tsx b/superset-frontend/src/views/menu.tsx index 5ba511c7032..dda01a8e27e 100644 --- a/superset-frontend/src/views/menu.tsx +++ b/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 = ( - - - - - - ) => - querystring.stringify(object, { encode: false }), - }} - > - - - - - - - + + + + + ) => + querystring.stringify(object, { encode: false }), + }} + > + + + + + + ); const menuMountPoint = document.getElementById('app-menu');