mirror of
https://github.com/apache/superset.git
synced 2026-05-21 15:55:10 +00:00
fix(ChartRenderer): address review on class→function refactor
Per @Copilot's review on #39459: 1. **showContextMenu staleness.** Moved out of `useState` (where it was computed once at initializer time and would go stale on source / viz- type changes for the same mounted instance) into a `useMemo` derived from props + feature flags. Renames `state.showContextMenu` to plain `showContextMenu` at all four call sites. Matches the pre-refactor semantic of recomputing on every render. 2. **cloneDeep runs during loading.** Gated the deep clone of `queriesResponse` on `resultsReady` so it only runs when results are about to render. Previously it ran on every `queriesResponse` identity change including loading/idle transitions. Not changed in this commit (responses to come in separate replies): - Chart.tsx prop spreading change — needs an audit of `ChartProps` surface to confirm nothing dropped silently. - `memo()` vs custom `shouldComponentUpdate` — would need a tailored `areEqual` to faithfully port the old SCU; deferring as a follow-up since the default-memo behavior is the more idiomatic React 18+ shape. - Test cast `props as ChartRendererProps` — fixture re-typing is a test-cleanup pass that's out of scope here.
This commit is contained in:
@@ -178,7 +178,6 @@ const BIG_NO_RESULT_MIN_HEIGHT = 220;
|
||||
const behaviors = [Behavior.InteractiveChart];
|
||||
|
||||
interface ChartRendererState {
|
||||
showContextMenu: boolean;
|
||||
inContextMenu: boolean;
|
||||
legendState: LegendState | undefined;
|
||||
legendIndex: number;
|
||||
@@ -221,11 +220,19 @@ function ChartRendererComponent({
|
||||
formData.viz_type ?? propVizType,
|
||||
)?.suppressContextMenu;
|
||||
|
||||
const [state, setState] = useState<ChartRendererState>({
|
||||
showContextMenu:
|
||||
// Derived from props/feature-flags: must NOT live in state, otherwise a
|
||||
// `source` or viz-type change on the same mounted instance would leave
|
||||
// it stale. (Pre-refactor this was a class-instance field recomputed on
|
||||
// every render — preserve that semantic by using a memo here.)
|
||||
const showContextMenu = useMemo(
|
||||
() =>
|
||||
source === ChartSource.Dashboard &&
|
||||
!suppressContextMenu &&
|
||||
isFeatureEnabled(FeatureFlag.DrillToDetail),
|
||||
[source, suppressContextMenu],
|
||||
);
|
||||
|
||||
const [state, setState] = useState<ChartRendererState>({
|
||||
inContextMenu: false,
|
||||
legendState: undefined,
|
||||
legendIndex: 0,
|
||||
@@ -259,10 +266,13 @@ function ChartRendererComponent({
|
||||
}, [queriesResponse]);
|
||||
|
||||
// Clone queriesResponse to protect against plugin mutation of Redux state.
|
||||
// Gate on `resultsReady` so the deep clone doesn't run for every
|
||||
// queriesResponse identity change during loading/idle (only when results
|
||||
// are actually about to render). Matches the pre-refactor gating.
|
||||
// TODO: remove once reducers use Redux Toolkit with Immer.
|
||||
const mutableQueriesResponse = useMemo(
|
||||
() => cloneDeep(queriesResponse),
|
||||
[queriesResponse],
|
||||
() => (resultsReady ? cloneDeep(queriesResponse) : undefined),
|
||||
[queriesResponse, resultsReady],
|
||||
);
|
||||
|
||||
// Handler functions
|
||||
@@ -373,7 +383,7 @@ function ChartRendererComponent({
|
||||
const hooks = useMemo<ChartHooks>(
|
||||
() => ({
|
||||
onAddFilter: handleAddFilter,
|
||||
onContextMenu: state.showContextMenu ? handleOnContextMenu : undefined,
|
||||
onContextMenu: showContextMenu ? handleOnContextMenu : undefined,
|
||||
onError: handleRenderFailure,
|
||||
setControlValue: handleSetControlValue,
|
||||
onFilterMenuOpen,
|
||||
@@ -394,7 +404,7 @@ function ChartRendererComponent({
|
||||
onFilterMenuClose,
|
||||
onFilterMenuOpen,
|
||||
setDataMaskCallback,
|
||||
state.showContextMenu,
|
||||
showContextMenu,
|
||||
],
|
||||
);
|
||||
|
||||
@@ -490,7 +500,7 @@ function ChartRendererComponent({
|
||||
|
||||
return (
|
||||
<>
|
||||
{state.showContextMenu && (
|
||||
{showContextMenu && (
|
||||
<ChartContextMenu
|
||||
ref={contextMenuRef}
|
||||
id={chartId}
|
||||
@@ -501,7 +511,7 @@ function ChartRendererComponent({
|
||||
)}
|
||||
<div
|
||||
onContextMenu={
|
||||
state.showContextMenu ? onContextMenuFallback : undefined
|
||||
showContextMenu ? onContextMenuFallback : undefined
|
||||
}
|
||||
>
|
||||
<SuperChart
|
||||
|
||||
Reference in New Issue
Block a user