This is an automated email from the ASF dual-hosted git repository. msyavuz pushed a commit to branch msyavuz/fix/chart-rendering-strict in repository https://gitbox.apache.org/repos/asf/superset.git
commit 48461f04396ac59cb11b8601f6b1df2b1749b65e Author: Mehmet Salih Yavuz <[email protected]> AuthorDate: Tue May 12 17:40:42 2026 +0300 fix: chart rendering --- .../src/chart/components/createLoadableRenderer.ts | 130 ++++++++++++++------- .../src/components/Chart/chartAction.ts | 20 ++++ superset-frontend/src/embedded/index.tsx | 8 +- superset-frontend/src/views/index.tsx | 7 +- superset-frontend/src/views/menu.tsx | 39 +++---- 5 files changed, 131 insertions(+), 73 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<Exports> = { + [K in keyof Exports]: () => Promise<Exports[K]> | Exports[K]; }; -export interface LoadableRenderer<Props> - extends - ComponentClass<Props & LoadableRendererProps>, - Loadable.LoadableComponent {} +export interface LoadingProps { + error?: { toString(): string }; +} -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>; +export interface LoadableOptions<Props, Exports> { + loader: LoaderMap<Exports>; + loading: (loadingProps: LoadingProps) => ReactElement | null; + render: (loaded: Exports, props: Props) => ReactElement; +} - // Extends the behavior of LoadableComponent to provide post-render listeners - class CustomLoadableRenderer extends LoadableRenderer { - static defaultProps: object; +export interface LoadableRenderer<Props> { + (props: Props & LoadableRendererProps): ReactElement | null; + preload: () => Promise<unknown>; + displayName?: string; +} - componentDidMount() { - this.afterRender(); - } +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; - componentDidUpdate() { - this.afterRender(); - } + 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; + }; - 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)(); - } + 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); + }; - CustomLoadableRenderer.defaultProps = defaultProps; - CustomLoadableRenderer.preload = LoadableRenderer.preload; + Renderer.preload = load; - return CustomLoadableRenderer; + return Renderer; } diff --git a/superset-frontend/src/components/Chart/chartAction.ts b/superset-frontend/src/components/Chart/chartAction.ts index 40d8ed79b16..17be5f09b2c 100644 --- a/superset-frontend/src/components/Chart/chartAction.ts +++ b/superset-frontend/src/components/Chart/chartAction.ts @@ -781,6 +781,16 @@ 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( @@ -826,6 +836,16 @@ 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/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( - <StrictMode> - <EmbeddedApp /> - </StrictMode>, - ); + root.render(<EmbeddedApp />); }, 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( - <StrictMode> - <App /> - </StrictMode>, - ); + root.render(<App />); } })().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 = ( - <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');
