Explore: Fix graph not updating when changing config (#62473)

* Explore: Fix graph not updating when changing config

* move useStructureRev to a seprate hook and add specific tests

public/app/features/explore/Graph/ExploreGraph.tsx

@@ -1,7 +1,6 @@
 import { css, cx } from '@emotion/css';
 import { identity } from 'lodash';
 import React, { useEffect, useMemo, useState } from 'react';
-import { useCounter } from 'react-use';
 
 import {
   AbsoluteTimeRange,
@@ -37,6 +36,7 @@ import { ExploreGraphStyle } from 'app/types';
 import { seriesVisibilityConfigFactory } from '../../dashboard/dashgrid/SeriesVisibilityConfigFactory';
 
 import { applyGraphStyle } from './exploreGraphStyleUtils';
+import { useStructureRev } from './useStructureRev';
 
 const MAX_NUMBER_OF_TIME_SERIES = 20;
 
@@ -76,7 +76,15 @@ export function ExploreGraph({
   const theme = useTheme2();
   const style = useStyles2(getStyles);
   const [showAllTimeSeries, setShowAllTimeSeries] = useState(false);
-  const [structureRev, { inc }] = useCounter(0);
+
+  const timeRange = {
+    from: dateTime(absoluteRange.from),
+    to: dateTime(absoluteRange.to),
+    raw: {
+      from: dateTime(absoluteRange.from),
+      to: dateTime(absoluteRange.to),
+    },
+  };
 
   const fieldConfigRegistry = useMemo(
     () => createFieldConfigRegistry(getGraphFieldConfig(defaultGraphConfig), 'Explore'),
@@ -98,34 +106,20 @@ export function ExploreGraph({
     overrides: [],
   });
 
-  const timeRange = {
-    from: dateTime(absoluteRange.from),
-    to: dateTime(absoluteRange.to),
-    raw: {
-      from: dateTime(absoluteRange.from),
-      to: dateTime(absoluteRange.to),
-    },
-  };
-
   const styledFieldConfig = useMemo(() => applyGraphStyle(fieldConfig, graphStyle), [fieldConfig, graphStyle]);
 
   const dataWithConfig = useMemo(() => {
     return applyFieldOverrides({
       fieldConfig: styledFieldConfig,
-      data,
+      data: showAllTimeSeries ? data : data.slice(0, MAX_NUMBER_OF_TIME_SERIES),
       timeZone,
       replaceVariables: (value) => value, // We don't need proper replace here as it is only used in getLinks and we use getFieldLinks
       theme,
       fieldConfigRegistry,
     });
-  }, [fieldConfigRegistry, data, timeZone, theme, styledFieldConfig]);
+  }, [fieldConfigRegistry, data, timeZone, theme, styledFieldConfig, showAllTimeSeries]);
 
-  const seriesToShow = showAllTimeSeries ? dataWithConfig : dataWithConfig.slice(0, MAX_NUMBER_OF_TIME_SERIES);
-
-  // We need to increment structureRev when the number of series changes.
-  // the function passed to useMemo runs during rendering, so when we get a different
-  // amount of data, structureRev is incremented before we render it
-  useMemo(inc, [dataWithConfig.length, styledFieldConfig, seriesToShow.length, inc]);
+  const structureRev = useStructureRev(dataWithConfig);
 
   useEffect(() => {
     if (onHiddenSeriesChanged) {
@@ -164,7 +158,7 @@ export function ExploreGraph({
 
   return (
     <PanelContextProvider value={panelContext}>
-      {dataWithConfig.length > MAX_NUMBER_OF_TIME_SERIES && !showAllTimeSeries && (
+      {data.length > MAX_NUMBER_OF_TIME_SERIES && !showAllTimeSeries && (
         <div className={cx([style.timeSeriesDisclaimer])}>
           <Icon className={style.disclaimerIcon} name="exclamation-triangle" />
           Showing only {MAX_NUMBER_OF_TIME_SERIES} time series.
@@ -174,12 +168,12 @@ export function ExploreGraph({
             onClick={() => setShowAllTimeSeries(true)}
             className={style.showAllButton}
           >
-            Show all {dataWithConfig.length}
+            Show all {data.length}
           </Button>
         </div>
       )}
       <PanelRenderer
-        data={{ series: seriesToShow, timeRange, state: loadingState, annotations, structureRev }}
+        data={{ series: dataWithConfig, timeRange, state: loadingState, annotations, structureRev }}
         pluginId="timeseries"
         title=""
         width={width}

public/app/features/explore/Graph/useStructureRev.test.ts

@@ -0,0 +1,117 @@
+import { renderHook } from '@testing-library/react-hooks';
+
+import { DataFrame, FieldType, toDataFrame } from '@grafana/data';
+
+import { useStructureRev } from './useStructureRev';
+
+let lastResult: number = Number.MIN_SAFE_INTEGER;
+
+const resetCounters = () => {
+  lastResult = Number.MIN_SAFE_INTEGER;
+};
+
+const startCounters = (start: number | Error) => {
+  // The if is only to make TypeScript happy
+  if (start instanceof Error) {
+    expect(start).not.toBeInstanceOf(Error);
+    return;
+  }
+
+  lastResult = start;
+};
+
+beforeAll(() => {
+  expect.extend({
+    toHaveIncremented(received: number[]) {
+      if (received.length < 2) {
+        return {
+          message: () => `expected at least 2 elements, got ${received.length}`,
+          pass: false,
+        };
+      }
+
+      const pass = received[received.length - 1] > lastResult;
+      const r = lastResult;
+
+      const message = () =>
+        this.isNot
+          ? `expected ${received[received.length - 1]} to be equal or lesser than ${r}`
+          : `expected ${received[received.length - 1]} to be greater than ${r}`;
+
+      lastResult = received[received.length - 1];
+
+      return {
+        message,
+        pass,
+      };
+    },
+  });
+});
+
+describe('useStructureRev', () => {
+  afterEach(() => resetCounters());
+
+  it('should increment only when relevant fields in frame change', () => {
+    let frames: DataFrame[] = [toDataFrame({ fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }] })];
+    const { result, rerender } = renderHook((frames) => useStructureRev(frames), { initialProps: frames });
+    startCounters(result.current);
+
+    // When changing number of frames, the structureRev should increment
+    frames = [...frames, toDataFrame({ fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }] })];
+    rerender(frames);
+    expect(result.all).toHaveIncremented();
+
+    // Changing RefId should not increment the structure revision
+    frames[0] = toDataFrame({
+      refId: 'A',
+      fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }],
+    });
+    rerender([...frames]);
+    expect(result.all).not.toHaveIncremented();
+
+    // Changing frame name should increment the structure revision
+    frames[0] = toDataFrame({
+      refId: 'A',
+      name: 'Some Name',
+      fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }],
+    });
+    rerender([...frames]);
+    expect(result.all).toHaveIncremented();
+
+    // Changing frame's fields number should increment the structure revision
+    frames[0] = toDataFrame({
+      refId: 'A',
+      name: 'Some Name',
+      fields: [
+        { name: 'time', type: FieldType.time, values: [1, 2, 3] },
+        { name: 'value', type: FieldType.number, values: [1, 2, 3] },
+      ],
+    });
+    rerender([...frames]);
+    expect(result.all).toHaveIncremented();
+
+    // Changing a frame's field's config should increment the structure revision
+    frames[0] = toDataFrame({
+      refId: 'A',
+      name: 'Some Name',
+      fields: [
+        { name: 'time', type: FieldType.time, values: [1, 2, 3] },
+        { name: 'value', type: FieldType.number, values: [1, 2, 3], config: { unit: 'ms' } },
+      ],
+    });
+    rerender([...frames]);
+    expect(result.all).toHaveIncremented();
+
+    // Changing a frame's field's name should increment the structure revision
+    frames[0] = toDataFrame({
+      refId: 'A',
+      name: 'Some Name',
+      fields: [
+        { name: 'time', type: FieldType.time, values: [1, 2, 3] },
+        { name: 'value, but with a different name', type: FieldType.number, values: [1, 2, 3], config: { unit: 'ms' } },
+      ],
+    });
+    rerender([...frames]);
+    expect(result.all).toHaveIncremented();
+  });
+});

public/app/features/explore/Graph/useStructureRev.ts

@@ -0,0 +1,20 @@
+import { useMemo } from 'react';
+import { useCounter, usePrevious } from 'react-use';
+
+import { DataFrame, compareArrayValues, compareDataFrameStructures } from '@grafana/data';
+
+export function useStructureRev(frames: DataFrame[]) {
+  const [structureRev, { inc }] = useCounter(0);
+  const previousFrames = usePrevious(frames);
+
+  // We need to increment structureRev when the number of series changes.
+  // the function passed to useMemo runs during rendering, so when we get a different
+  // amount of data, structureRev is incremented before we render it
+  useMemo(() => {
+    if (previousFrames && !compareArrayValues(frames, previousFrames, compareDataFrameStructures)) {
+      inc();
+    }
+  }, [frames, previousFrames, inc]);
+
+  return structureRev;
+}