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:
Claude Code
2026-05-19 00:30:50 -05:00
parent 7bd107fcdc
commit 664a891f84

View File

@@ -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