This is an automated email from the ASF dual-hosted git repository. kgabryje pushed a commit to branch what-if in repository https://gitbox.apache.org/repos/asf/superset.git
commit d86628918b08b69c25d30ca1784dd88c006b50e5 Author: Kamil Gabryjelski <[email protected]> AuthorDate: Thu Dec 18 12:19:36 2025 +0100 Fix race condition and double loading spinner --- .../components/WhatIfDrawer/WhatIfAIInsights.tsx | 33 +++++++++--- .../components/WhatIfDrawer/useChartComparison.ts | 27 ++++++++-- .../components/gridComponents/Chart/Chart.jsx | 24 --------- .../src/dashboard/util/useWhatIfHighlightStyles.ts | 62 +++++++++++----------- 4 files changed, 81 insertions(+), 65 deletions(-) diff --git a/superset-frontend/src/dashboard/components/WhatIfDrawer/WhatIfAIInsights.tsx b/superset-frontend/src/dashboard/components/WhatIfDrawer/WhatIfAIInsights.tsx index 8c276b7643..4cf94865e5 100644 --- a/superset-frontend/src/dashboard/components/WhatIfDrawer/WhatIfAIInsights.tsx +++ b/superset-frontend/src/dashboard/components/WhatIfDrawer/WhatIfAIInsights.tsx @@ -24,6 +24,7 @@ import { styled, Alert } from '@apache-superset/core/ui'; import { Icons } from '@superset-ui/core/components/Icons'; import { Skeleton } from '@superset-ui/core/components/'; import { RootState, WhatIfModification } from 'src/dashboard/types'; +import { whatIfHighlightStyles } from 'src/dashboard/util/useWhatIfHighlightStyles'; import { fetchWhatIfInterpretation } from './whatIfApi'; import { useChartComparison, useAllChartsLoaded } from './useChartComparison'; import { @@ -108,6 +109,7 @@ const Summary = styled.div` padding: ${({ theme }) => theme.sizeUnit * 3}px; background-color: ${({ theme }) => theme.colorBgElevated}; border-radius: ${({ theme }) => theme.borderRadius}px; + ${whatIfHighlightStyles} `; interface WhatIfAIInsightsProps { @@ -137,7 +139,13 @@ const WhatIfAIInsights = ({ affectedChartIds }: WhatIfAIInsightsProps) => { const modificationsKey = getModificationsKey(whatIfModifications); const prevModificationsKeyRef = useRef<string>(modificationsKey); - // Debug logging + // Debug logging for race condition diagnosis + const willTriggerFetch = + whatIfModifications.length > 0 && + chartComparisons.length > 0 && + allChartsLoaded && + status === 'idle'; + console.log('[WhatIfAIInsights] State:', { affectedChartIds, allChartsLoaded, @@ -145,13 +153,26 @@ const WhatIfAIInsights = ({ affectedChartIds }: WhatIfAIInsightsProps) => { whatIfModificationsLength: whatIfModifications.length, status, modificationsKey, - willTriggerFetch: - whatIfModifications.length > 0 && - chartComparisons.length > 0 && - allChartsLoaded && - status === 'idle', + willTriggerFetch, }); + // Log chart comparison details when about to fetch (helps diagnose race conditions) + if (willTriggerFetch && chartComparisons.length > 0) { + console.log( + '[WhatIfAIInsights] Chart comparisons to send:', + chartComparisons.map(c => ({ + chartId: c.chartId, + chartName: c.chartName, + metrics: c.metrics.map(m => ({ + name: m.metricName, + original: m.originalValue, + modified: m.modifiedValue, + change: `${m.percentageChange.toFixed(2)}%`, + })), + })), + ); + } + // Reset status when modifications change (user adjusts the slider) useEffect(() => { if ( diff --git a/superset-frontend/src/dashboard/components/WhatIfDrawer/useChartComparison.ts b/superset-frontend/src/dashboard/components/WhatIfDrawer/useChartComparison.ts index f3c2d035a1..a5257b2c76 100644 --- a/superset-frontend/src/dashboard/components/WhatIfDrawer/useChartComparison.ts +++ b/superset-frontend/src/dashboard/components/WhatIfDrawer/useChartComparison.ts @@ -229,11 +229,19 @@ export function useChartComparison( if (!chartState || !displayData) continue; - const originalData = chartState.originalData; - const modifiedData = chartState.modifiedData; + const { originalData, modifiedData } = chartState; if (!originalData || !modifiedData) continue; + // Skip if original and modified data are the same reference + // This indicates the what-if query hasn't completed yet (race condition guard) + if (originalData === modifiedData) { + console.warn( + `[useChartComparison] Chart ${chartId}: originalData === modifiedData (same reference), skipping`, + ); + continue; + } + // Get column names from the response const colnames = chartState.colnames || []; const metrics: ChartMetricComparison[] = []; @@ -296,7 +304,9 @@ function useChartLoadingStatuses( /** * Check if all affected charts (in active tabs) have finished loading. - * Returns true if no visible chart is currently in 'loading' status. + * Returns true only if ALL visible charts are in a definitive complete state + * ('success' or 'rendered'). This prevents race conditions where charts + * might briefly be in an intermediate state. */ export function useAllChartsLoaded(chartIds: number[]): boolean { const visibleChartIds = useChartsInActiveTabs(chartIds); @@ -309,6 +319,15 @@ export function useAllChartsLoaded(chartIds: number[]): boolean { })); console.log('[useAllChartsLoaded] Chart statuses:', statuses); - return visibleChartIds.every(id => chartStatuses[id] !== 'loading'); + // Require explicit completion status, not just "not loading" + // This prevents race conditions during state transitions + // Include 'failed' to avoid waiting indefinitely for charts that errored + const completeStatuses = ['success', 'rendered', 'failed']; + return ( + visibleChartIds.length > 0 && + visibleChartIds.every(id => + completeStatuses.includes(chartStatuses[id] ?? ''), + ) + ); }, [chartStatuses, visibleChartIds]); } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx index 9cb458718e..abf7da98c4 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx @@ -21,7 +21,6 @@ import { useCallback, useEffect, useRef, useMemo, useState, memo } from 'react'; import PropTypes from 'prop-types'; import { t, logging } from '@superset-ui/core'; import { styled } from '@apache-superset/core/ui'; -import { Loading } from '@superset-ui/core/components'; import { debounce } from 'lodash'; import { useHistory } from 'react-router-dom'; import { bindActionCreators } from 'redux'; @@ -99,17 +98,6 @@ const ChartWrapper = styled.div` } `; -const ChartOverlay = styled.div` - position: absolute; - top: 0; - left: 0; - z-index: 5; - background-color: ${({ theme }) => theme.colorBgMask}; - display: flex; - align-items: center; - justify-content: center; -`; - const SliceContainer = styled.div` display: flex; flex-direction: column; @@ -611,7 +599,6 @@ const Chart = props => { return <MissingChart height={getChartHeight()} />; } - const isLoading = chartStatus === 'loading'; const cachedDttm = // eslint-disable-next-line camelcase queriesResponse?.map(({ cached_dttm }) => cached_dttm) || []; @@ -683,17 +670,6 @@ const Chart = props => { className={cx('dashboard-chart')} aria-label={slice.description} > - {isLoading && ( - <ChartOverlay - style={{ - width, - height: getChartHeight(), - }} - > - <Loading size="s" muted /> - </ChartOverlay> - )} - <ChartContainer width={width} height={getChartHeight()} diff --git a/superset-frontend/src/dashboard/util/useWhatIfHighlightStyles.ts b/superset-frontend/src/dashboard/util/useWhatIfHighlightStyles.ts index 1db666a7f8..07182642fc 100644 --- a/superset-frontend/src/dashboard/util/useWhatIfHighlightStyles.ts +++ b/superset-frontend/src/dashboard/util/useWhatIfHighlightStyles.ts @@ -36,16 +36,16 @@ const rainbowSlide = keyframes` const pulse = keyframes` 0%, 100% { - opacity: 0.7; - filter: blur(0px); + opacity: 0.6; + filter: blur(2px); } 50% { - opacity: 1; - filter: blur(2px); + opacity: 0.9; + filter: blur(6px); } `; -const whatIfHighlightStyles = css` +export const whatIfHighlightStyles = css` position: relative; &::before { @@ -53,34 +53,34 @@ const whatIfHighlightStyles = css` position: absolute; inset: -2px; border-radius: 10px; - padding: 4px; + padding: 3px; background: linear-gradient( 90deg, - #ff0000, - #ff8000, - #ffff00, - #80ff00, - #00ff00, - #00ff80, - #00ffff, - #0080ff, - #0000ff, - #8000ff, - #ff00ff, - #ff0080, - #ff0000, - #ff8000, - #ffff00, - #80ff00, - #00ff00, - #00ff80, - #00ffff, - #0080ff, - #0000ff, - #8000ff, - #ff00ff, - #ff0080, - #ff0000 + #ffb3b3, + #ffd9b3, + #ffffb3, + #d9ffb3, + #b3ffb3, + #b3ffd9, + #b3ffff, + #b3d9ff, + #b3b3ff, + #d9b3ff, + #ffb3ff, + #ffb3d9, + #ffb3b3, + #ffd9b3, + #ffffb3, + #d9ffb3, + #b3ffb3, + #b3ffd9, + #b3ffff, + #b3d9ff, + #b3b3ff, + #d9b3ff, + #ffb3ff, + #ffb3d9, + #ffb3b3 ); background-size: 300% 100%; animation:
