Variables: Make renamed or missing variable section expandable (#41964) (#42025)

* Variables: Make renamed or missing variable section expandable * Chore: feedback after PR comments (cherry picked from commit 44d7d6546f) Co-authored-by: Hugo Häggmark <hugo.haggmark@grafana.com>
2025-02-25 18:55:37 -06:00 · 2021-11-22 02:04:34 -05:00 · 2021-11-22 02:04:34 -05:00 · fcd0c382b6
commit fcd0c382b6
parent 272f850fb2
9 changed files with 309 additions and 96 deletions
--- a/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx
+++ b/packages/grafana-ui/src/components/Collapse/CollapsableSection.tsx
@ -5,20 +5,26 @@ import { Icon } from '..';
 import { GrafanaTheme2 } from '@grafana/data';

 export interface Props {
-  label: string;
+  label: ReactNode;
  isOpen: boolean;
+  /** Callback for the toggle functionality */
+  onToggle?: (isOpen: boolean) => void;
  children: ReactNode;
 }

-export const CollapsableSection: FC<Props> = ({ label, isOpen, children }) => {
+export const CollapsableSection: FC<Props> = ({ label, isOpen, onToggle, children }) => {
  const [open, toggleOpen] = useState<boolean>(isOpen);
  const styles = useStyles2(collapsableSectionStyles);
  const headerStyle = open ? styles.header : styles.headerCollapsed;
  const tooltip = `Click to ${open ? 'collapse' : 'expand'}`;
+  const onClick = () => {
+    onToggle?.(!open);
+    toggleOpen(!open);
+  };

  return (
    <div>
-      <div onClick={() => toggleOpen(!open)} className={headerStyle} title={tooltip}>
+      <div onClick={onClick} className={headerStyle} title={tooltip}>
        {label}
        <Icon name={open ? 'angle-down' : 'angle-right'} size="xl" className={styles.icon} />
      </div>
--- a/public/app/features/variables/editor/VariableEditorContainer.tsx
+++ b/public/app/features/variables/editor/VariableEditorContainer.tsx
@ -17,10 +17,7 @@ const mapStateToProps = (state: StoreState) => ({
  variables: getEditorVariables(state),
  idInEditor: state.templating.editor.id,
  dashboard: state.dashboard.getModel(),
-  unknownsNetwork: state.templating.inspect.unknownsNetwork,
-  unknownExists: state.templating.inspect.unknownExits,
  usagesNetwork: state.templating.inspect.usagesNetwork,
-  unknown: state.templating.inspect.unknown,
  usages: state.templating.inspect.usages,
 });

@ -119,7 +116,7 @@ class VariableEditorContainerUnconnected extends PureComponent<Props> {
              usages={this.props.usages}
              usagesNetwork={this.props.usagesNetwork}
            />
-            {this.props.unknownExists ? <VariablesUnknownTable usages={this.props.unknownsNetwork} /> : null}
+            <VariablesUnknownTable variables={this.props.variables} dashboard={this.props.dashboard} />
          </>
        )}
        {variableToEdit && <VariableEditorEditor identifier={toVariableIdentifier(variableToEdit)} />}
--- a/public/app/features/variables/editor/actions.ts
+++ b/public/app/features/variables/editor/actions.ts
@ -109,12 +109,10 @@ export const switchToListMode = (): ThunkResult<void> => (dispatch, getState) =>
  const state = getState();
  const variables = getEditorVariables(state);
  const dashboard = state.dashboard.getModel();
-  const { unknown, usages } = createUsagesNetwork(variables, dashboard);
-  const unknownsNetwork = transformUsagesToNetwork(unknown);
-  const unknownExits = Object.keys(unknown).length > 0;
+  const { usages } = createUsagesNetwork(variables, dashboard);
  const usagesNetwork = transformUsagesToNetwork(usages);

-  dispatch(initInspect({ unknown, usages, usagesNetwork, unknownsNetwork, unknownExits }));
+  dispatch(initInspect({ usages, usagesNetwork }));
 };

 export function getNextAvailableId(type: VariableType, variables: VariableModel[]): string {
--- a/public/app/features/variables/inspect/VariablesUnknownButton.tsx
+++ b/public/app/features/variables/inspect/VariablesUnknownButton.tsx
@ -25,7 +25,14 @@ export const VariablesUnknownButton: FC<Props> = ({ id, usages }) => {
  return (
    <NetworkGraphModal show={false} title={`Showing usages for: $${id}`} nodes={nodes} edges={network.edges}>
      {({ showModal }) => {
-        return <IconButton onClick={() => showModal()} name="code-branch" title="Show usages" />;
+        return (
+          <IconButton
+            onClick={() => showModal()}
+            name="code-branch"
+            title="Show usages"
+            data-testid="VariablesUnknownButton"
+          />
+        );
      }}
    </NetworkGraphModal>
  );
--- a/public/app/features/variables/inspect/VariablesUnknownTable.test.tsx
+++ b/public/app/features/variables/inspect/VariablesUnknownTable.test.tsx
@ -0,0 +1,137 @@
+import React from 'react';
+import * as runtime from '@grafana/runtime';
+import { render, screen, waitFor, waitForElementToBeRemoved } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+
+import { VariablesUnknownTable, VariablesUnknownTableProps } from './VariablesUnknownTable';
+import { customBuilder } from '../shared/testing/builders';
+import * as utils from './utils';
+import { UsagesToNetwork } from './utils';
+
+async function getTestContext(
+  overrides: Partial<VariablesUnknownTableProps> | undefined = {},
+  usages: UsagesToNetwork[] = []
+) {
+  jest.clearAllMocks();
+  const reportInteractionSpy = jest.spyOn(runtime, 'reportInteraction').mockImplementation();
+  const getUnknownsNetworkSpy = jest.spyOn(utils, 'getUnknownsNetwork').mockResolvedValue(usages);
+  const defaults: VariablesUnknownTableProps = {
+    variables: [],
+    dashboard: null,
+  };
+  const props = { ...defaults, ...overrides };
+  const { rerender } = render(<VariablesUnknownTable {...props} />);
+  await waitFor(() =>
+    expect(screen.getByRole('heading', { name: /renamed or missing variables/i })).toBeInTheDocument()
+  );
+
+  return { reportInteractionSpy, getUnknownsNetworkSpy, rerender };
+}
+
+describe('VariablesUnknownTable', () => {
+  describe('when rendered', () => {
+    it('then it should render the section header', async () => {
+      await getTestContext();
+    });
+  });
+
+  describe('when expanding the section', () => {
+    it('then it should show loading spinner', async () => {
+      await getTestContext();
+
+      userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+      await waitFor(() => expect(screen.getByText('Loading...')).toBeInTheDocument());
+    });
+
+    it('then it should call getUnknownsNetwork', async () => {
+      const { getUnknownsNetworkSpy } = await getTestContext();
+
+      userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+      await waitFor(() => expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1));
+    });
+
+    it('then it should report the interaction', async () => {
+      const { reportInteractionSpy } = await getTestContext();
+
+      userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+      await waitFor(() => expect(screen.getByText('Loading...')).toBeInTheDocument());
+
+      expect(reportInteractionSpy).toHaveBeenCalledTimes(1);
+      expect(reportInteractionSpy).toHaveBeenCalledWith('Unknown variables section expanded');
+    });
+
+    describe('but when expanding it again without changes to variables or dashboard', () => {
+      it('then it should not call getUnknownsNetwork', async () => {
+        const { getUnknownsNetworkSpy } = await getTestContext();
+
+        userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+        await waitFor(() => expect(screen.getByTitle('Click to collapse')).toBeInTheDocument());
+        expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1);
+
+        userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+        await waitFor(() => expect(screen.getByTitle('Click to expand')).toBeInTheDocument());
+
+        userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+        await waitFor(() => expect(screen.getByTitle('Click to collapse')).toBeInTheDocument());
+
+        expect(getUnknownsNetworkSpy).toHaveBeenCalledTimes(1);
+      });
+    });
+
+    describe('and there are no renamed or missing variables', () => {
+      it('then it should render the correct message', async () => {
+        await getTestContext();
+
+        userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+        await waitForElementToBeRemoved(() => screen.getByText('Loading...'));
+
+        expect(screen.getByText('No renamed or missing variables found.')).toBeInTheDocument();
+      });
+    });
+
+    describe('and there are renamed or missing variables', () => {
+      it('then it should render the table', async () => {
+        const variable = customBuilder().withId('Renamed Variable').withName('Renamed Variable').build();
+        const usages = [{ variable, nodes: [], edges: [], showGraph: false }];
+        const { reportInteractionSpy } = await getTestContext({}, usages);
+
+        userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+        await waitForElementToBeRemoved(() => screen.getByText('Loading...'));
+
+        expect(screen.queryByText('No renamed or missing variables found.')).not.toBeInTheDocument();
+        expect(screen.getByText('Renamed Variable')).toBeInTheDocument();
+        expect(screen.getAllByTestId('VariablesUnknownButton')).toHaveLength(1);
+
+        // make sure we don't report the interaction for slow expansion
+        expect(reportInteractionSpy).toHaveBeenCalledTimes(1);
+        expect(reportInteractionSpy).toHaveBeenCalledWith('Unknown variables section expanded');
+      });
+
+      describe('but when the unknown processing takes a while', () => {
+        const origDateNow = Date.now;
+
+        afterEach(() => {
+          Date.now = origDateNow;
+        });
+
+        it('then it should report slow expansion', async () => {
+          const variable = customBuilder().withId('Renamed Variable').withName('Renamed Variable').build();
+          const usages = [{ variable, nodes: [], edges: [], showGraph: false }];
+          const { reportInteractionSpy } = await getTestContext({}, usages);
+          const dateNowStart = 1000;
+          const dateNowStop = 2000;
+          Date.now = jest.fn().mockReturnValueOnce(dateNowStart).mockReturnValue(dateNowStop);
+
+          userEvent.click(screen.getByRole('heading', { name: /renamed or missing variables/i }));
+          await waitForElementToBeRemoved(() => screen.getByText('Loading...'));
+
+          // make sure we report the interaction for slow expansion
+          expect(reportInteractionSpy).toHaveBeenCalledTimes(2);
+          expect(reportInteractionSpy.mock.calls[0][0]).toEqual('Unknown variables section expanded');
+          expect(reportInteractionSpy.mock.calls[1][0]).toEqual('Slow unknown variables expansion');
+          expect(reportInteractionSpy.mock.calls[1][1]).toEqual({ elapsed: 1000 });
+        });
+      });
+    });
+  });
+});
--- a/public/app/features/variables/inspect/VariablesUnknownTable.tsx
+++ b/public/app/features/variables/inspect/VariablesUnknownTable.tsx
@ -1,63 +1,129 @@
-import React, { FC } from 'react';
+import React, { ReactElement, useEffect, useState } from 'react';
 import { css } from '@emotion/css';
-import { Icon, Tooltip, useStyles } from '@grafana/ui';
+import { useAsync } from 'react-use';
+import { CollapsableSection, HorizontalGroup, Icon, Spinner, Tooltip, useStyles, VerticalGroup } from '@grafana/ui';
 import { GrafanaTheme } from '@grafana/data';
-import { UsagesToNetwork } from './utils';
-import { VariablesUnknownButton } from './VariablesUnknownButton';
+import { reportInteraction } from '@grafana/runtime';

-interface Props {
-  usages: UsagesToNetwork[];
+import { VariableModel } from '../types';
+import { DashboardModel } from '../../dashboard/state';
+import { VariablesUnknownButton } from './VariablesUnknownButton';
+import { getUnknownsNetwork, UsagesToNetwork } from './utils';
+
+export const SLOW_VARIABLES_EXPANSION_THRESHOLD = 1000;
+
+export interface VariablesUnknownTableProps {
+  variables: VariableModel[];
+  dashboard: DashboardModel | null;
 }

-export const VariablesUnknownTable: FC<Props> = ({ usages }) => {
+export function VariablesUnknownTable({ variables, dashboard }: VariablesUnknownTableProps): ReactElement {
+  const [open, setOpen] = useState(false);
+  const [changed, setChanged] = useState(0);
+  const [usages, setUsages] = useState<UsagesToNetwork[]>([]);
  const style = useStyles(getStyles);
+  useEffect(() => setChanged((prevState) => prevState + 1), [variables, dashboard]);
+  const { loading } = useAsync(async () => {
+    if (open && changed > 0) {
+      // make sure we only fetch when opened and variables or dashboard have changed
+      const start = Date.now();
+      const unknownsNetwork = await getUnknownsNetwork(variables, dashboard);
+      const stop = Date.now();
+      const elapsed = stop - start;
+      if (elapsed >= SLOW_VARIABLES_EXPANSION_THRESHOLD) {
+        reportInteraction('Slow unknown variables expansion', { elapsed });
+      }
+      setChanged(0);
+      setUsages(unknownsNetwork);
+      return unknownsNetwork;
+    }
+
+    return [];
+  }, [variables, dashboard, open, changed]);
+
+  const onToggle = (isOpen: boolean) => {
+    if (isOpen) {
+      reportInteraction('Unknown variables section expanded');
+    }
+
+    setOpen(isOpen);
+  };
+
  return (
    <div className={style.container}>
-      <h5>
-        Unknown Variables
-        <Tooltip content="This table lists all variable references that no longer exist in this dashboard.">
-          <Icon name="info-circle" className={style.infoIcon} />
-        </Tooltip>
-      </h5>
-
-      <div>
-        <table className="filter-table filter-table--hover">
-          <thead>
-            <tr>
-              <th>Variable</th>
-              <th colSpan={5} />
-            </tr>
-          </thead>
-          <tbody>
-            {usages.map((usage) => {
-              const { variable } = usage;
-              const { id, name } = variable;
-              return (
-                <tr key={id}>
-                  <td className={style.firstColumn}>
-                    <span>{name}</span>
-                  </td>
-                  <td className={style.defaultColumn} />
-                  <td className={style.defaultColumn} />
-                  <td className={style.defaultColumn} />
-                  <td className={style.lastColumn}>
-                    <VariablesUnknownButton id={variable.id} usages={usages} />
-                  </td>
-                </tr>
-              );
-            })}
-          </tbody>
-        </table>
-      </div>
+      <CollapsableSection label={<CollapseLabel />} isOpen={open} onToggle={onToggle}>
+        {loading && (
+          <VerticalGroup justify="center">
+            <HorizontalGroup justify="center">
+              <span>Loading...</span>
+              <Spinner size={16} />
+            </HorizontalGroup>
+          </VerticalGroup>
+        )}
+        {!loading && usages && (
+          <>
+            {usages.length === 0 && <NoUnknowns />}
+            {usages.length > 0 && <UnknownTable usages={usages} />}
+          </>
+        )}
+      </CollapsableSection>
    </div>
  );
-};
+}
+
+function CollapseLabel(): ReactElement {
+  const style = useStyles(getStyles);
+  return (
+    <h5>
+      Renamed or missing variables
+      <Tooltip content="Click to expand a list with all variable references that have been renamed or are missing from the dashboard.">
+        <Icon name="info-circle" className={style.infoIcon} />
+      </Tooltip>
+    </h5>
+  );
+}
+
+function NoUnknowns(): ReactElement {
+  return <span>No renamed or missing variables found.</span>;
+}
+
+function UnknownTable({ usages }: { usages: UsagesToNetwork[] }): ReactElement {
+  const style = useStyles(getStyles);
+  return (
+    <table className="filter-table filter-table--hover">
+      <thead>
+        <tr>
+          <th>Variable</th>
+          <th colSpan={5} />
+        </tr>
+      </thead>
+      <tbody>
+        {usages.map((usage) => {
+          const { variable } = usage;
+          const { id, name } = variable;
+          return (
+            <tr key={id}>
+              <td className={style.firstColumn}>
+                <span>{name}</span>
+              </td>
+              <td className={style.defaultColumn} />
+              <td className={style.defaultColumn} />
+              <td className={style.defaultColumn} />
+              <td className={style.lastColumn}>
+                <VariablesUnknownButton id={variable.id} usages={usages} />
+              </td>
+            </tr>
+          );
+        })}
+      </tbody>
+    </table>
+  );
+}

 const getStyles = (theme: GrafanaTheme) => ({
  container: css`
    margin-top: ${theme.spacing.xl};
    padding-top: ${theme.spacing.xl};
-    border-top: 1px solid ${theme.colors.panelBorder};
  `,
  infoIcon: css`
    margin-left: ${theme.spacing.sm};
--- a/public/app/features/variables/inspect/reducer.test.ts
+++ b/public/app/features/variables/inspect/reducer.test.ts
@ -10,19 +10,13 @@ describe('variableInspectReducer', () => {
        .givenReducer(variableInspectReducer, { ...initialVariableInspectState })
        .whenActionIsDispatched(
          initInspect({
-            unknownExits: true,
-            unknownsNetwork: [{ edges: [], nodes: [], showGraph: true, variable }],
            usagesNetwork: [{ edges: [], nodes: [], showGraph: true, variable }],
            usages: [{ variable, tree: {} }],
-            unknown: [{ variable, tree: {} }],
          })
        )
        .thenStateShouldEqual({
-          unknownExits: true,
-          unknownsNetwork: [{ edges: [], nodes: [], showGraph: true, variable }],
          usagesNetwork: [{ edges: [], nodes: [], showGraph: true, variable }],
          usages: [{ variable, tree: {} }],
-          unknown: [{ variable, tree: {} }],
        });
    });
  });
--- a/public/app/features/variables/inspect/reducer.ts
+++ b/public/app/features/variables/inspect/reducer.ts
@ -2,40 +2,22 @@ import { UsagesToNetwork, VariableUsageTree } from './utils';
 import { createSlice, PayloadAction } from '@reduxjs/toolkit';

 export interface VariableInspectState {
-  unknown: VariableUsageTree[];
  usages: VariableUsageTree[];
-  unknownsNetwork: UsagesToNetwork[];
  usagesNetwork: UsagesToNetwork[];
-  unknownExits: boolean;
 }

 export const initialVariableInspectState: VariableInspectState = {
-  unknown: [],
  usages: [],
-  unknownsNetwork: [],
  usagesNetwork: [],
-  unknownExits: false,
 };

 const variableInspectReducerSlice = createSlice({
  name: 'templating/inspect',
  initialState: initialVariableInspectState,
  reducers: {
-    initInspect: (
-      state,
-      action: PayloadAction<{
-        unknown: VariableUsageTree[];
-        usages: VariableUsageTree[];
-        unknownsNetwork: UsagesToNetwork[];
-        usagesNetwork: UsagesToNetwork[];
-        unknownExits: boolean;
-      }>
-    ) => {
-      const { unknown, usages, unknownExits, unknownsNetwork, usagesNetwork } = action.payload;
+    initInspect: (state, action: PayloadAction<{ usages: VariableUsageTree[]; usagesNetwork: UsagesToNetwork[] }>) => {
+      const { usages, usagesNetwork } = action.payload;
      state.usages = usages;
-      state.unknown = unknown;
-      state.unknownsNetwork = unknownsNetwork;
-      state.unknownExits = unknownExits;
      state.usagesNetwork = usagesNetwork;
    },
  },
--- a/public/app/features/variables/inspect/utils.ts
+++ b/public/app/features/variables/inspect/utils.ts
@ -179,29 +179,18 @@ export interface VariableUsageTree {

 export interface VariableUsages {
  unUsed: VariableModel[];
-  unknown: VariableUsageTree[];
  usages: VariableUsageTree[];
 }

 export const createUsagesNetwork = (variables: VariableModel[], dashboard: DashboardModel | null): VariableUsages => {
  if (!dashboard) {
-    return { unUsed: [], unknown: [], usages: [] };
+    return { unUsed: [], usages: [] };
  }

  const unUsed: VariableModel[] = [];
  let usages: VariableUsageTree[] = [];
-  let unknown: VariableUsageTree[] = [];
  const model = dashboard.getSaveModelClone();

-  const unknownVariables = getUnknownVariableStrings(variables, model);
-  for (const unknownVariable of unknownVariables) {
-    const props = getPropsWithVariable(unknownVariable, { key: 'model', value: model }, {});
-    if (Object.keys(props).length) {
-      const variable = ({ id: unknownVariable, name: unknownVariable } as unknown) as VariableModel;
-      unknown.push({ variable, tree: props });
-    }
-  }
-
  for (const variable of variables) {
    const variableId = variable.id;
    const props = getPropsWithVariable(variableId, { key: 'model', value: model }, {});
@ -214,9 +203,46 @@ export const createUsagesNetwork = (variables: VariableModel[], dashboard: Dashb
    }
  }

-  return { unUsed, unknown, usages };
+  return { unUsed, usages };
 };

+export async function getUnknownsNetwork(
+  variables: VariableModel[],
+  dashboard: DashboardModel | null
+): Promise<UsagesToNetwork[]> {
+  return new Promise((resolve, reject) => {
+    // can be an expensive call so we avoid blocking the main thread
+    setTimeout(() => {
+      try {
+        const unknowns = createUnknownsNetwork(variables, dashboard);
+        resolve(transformUsagesToNetwork(unknowns));
+      } catch (e) {
+        reject(e);
+      }
+    }, 200);
+  });
+}
+
+function createUnknownsNetwork(variables: VariableModel[], dashboard: DashboardModel | null): VariableUsageTree[] {
+  if (!dashboard) {
+    return [];
+  }
+
+  let unknown: VariableUsageTree[] = [];
+  const model = dashboard.getSaveModelClone();
+
+  const unknownVariables = getUnknownVariableStrings(variables, model);
+  for (const unknownVariable of unknownVariables) {
+    const props = getPropsWithVariable(unknownVariable, { key: 'model', value: model }, {});
+    if (Object.keys(props).length) {
+      const variable = ({ id: unknownVariable, name: unknownVariable } as unknown) as VariableModel;
+      unknown.push({ variable, tree: props });
+    }
+  }
+
+  return unknown;
+}
+
 /*
  getAllAffectedPanelIdsForVariableChange is a function that extracts all the panel ids that are affected by a single variable
  change. It will traverse all chained variables to identify all cascading changes too.