Dashboard: Refactor panel cleanup (#47323)

.betterer.results

@@ -203,7 +203,7 @@ exports[`no enzyme tests`] = {
     "public/app/features/dashboard/components/ShareModal/ShareLink.test.tsx:809006195": [
       [1, 35, 13, "RegExp match", "2409514259"]
     ],
-    "public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx:1798654441": [
+    "public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx:3682134210": [
       [1, 35, 13, "RegExp match", "2409514259"]
     ],
     "public/app/features/dashboard/dashgrid/PanelHeader/PanelHeaderCorner.test.tsx:3293585799": [

public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx

@@ -256,7 +256,6 @@ export class PanelEditorUnconnected extends PureComponent<Props> {
                       lazy={false}
                       width={panelSize.width}
                       height={panelSize.height}
-                      skipStateCleanUp={true}
                     />
                   </div>
                 </div>

public/app/features/dashboard/components/PanelEditor/state/actions.test.ts

@@ -1,7 +1,7 @@
 import { thunkTester } from '../../../../../../test/core/thunk/thunkTester';
 import { closeEditor, initialState, PanelEditorState } from './reducers';
 import { exitPanelEditor, initPanelEditor, skipPanelUpdate } from './actions';
-import { cleanUpPanelState, panelModelAndPluginReady } from 'app/features/panel/state/reducers';
+import { panelModelAndPluginReady, removePanel } from 'app/features/panel/state/reducers';
 import { DashboardModel, PanelModel } from '../../../state';
 import { getPanelPlugin } from 'app/features/plugins/__mocks__/pluginMocks';
 
@@ -48,6 +48,7 @@ describe('panelEditor actions', () => {
       };
 
       const dispatchedActions = await thunkTester({
+        panels: {},
         panelEditor: state,
         dashboard: {
           getModel: () => dashboard,
@@ -57,7 +58,7 @@ describe('panelEditor actions', () => {
         .whenThunkIsDispatched();
 
       expect(dispatchedActions.length).toBe(2);
-      expect(dispatchedActions[0].type).toBe(cleanUpPanelState.type);
+      expect(dispatchedActions[0].type).toBe(removePanel.type);
       expect(dispatchedActions[1].type).toBe(closeEditor.type);
       expect(sourcePanel.getOptions()).toEqual({ prop: true });
       expect(sourcePanel.id).toEqual(12);
@@ -84,6 +85,7 @@ describe('panelEditor actions', () => {
 
       const dispatchedActions = await thunkTester({
         panelEditor: state,
+        panels: {},
         dashboard: {
           getModel: () => dashboard,
         },
@@ -119,6 +121,7 @@ describe('panelEditor actions', () => {
 
       const dispatchedActions = await thunkTester({
         panelEditor: state,
+        panels: {},
         dashboard: {
           getModel: () => dashboard,
         },

public/app/features/dashboard/components/PanelEditor/state/actions.ts

@@ -1,5 +1,8 @@
-import { DashboardModel, PanelModel } from '../../../state';
+import { pick } from 'lodash';
 import { ThunkResult } from 'app/types';
+import store from 'app/core/store';
+import { panelModelAndPluginReady } from 'app/features/panel/state/reducers';
+import { cleanUpPanelState, initPanelState } from 'app/features/panel/state/actions';
 import {
   closeEditor,
   PANEL_EDITOR_UI_STATE_STORAGE_KEY,
@@ -8,10 +11,7 @@ import {
   setPanelEditorUIState,
   updateEditorInitState,
 } from './reducers';
-import { cleanUpPanelState, panelModelAndPluginReady } from 'app/features/panel/state/reducers';
+import { DashboardModel, PanelModel } from '../../../state';
-import store from 'app/core/store';
-import { pick } from 'lodash';
-import { initPanelState } from 'app/features/panel/state/actions';
 
 export function initPanelEditor(sourcePanel: PanelModel, dashboard: DashboardModel): ThunkResult<void> {
   return async (dispatch) => {
@@ -63,9 +63,10 @@ export function updateDuplicateLibraryPanels(
       panel.configRev++;
 
       if (pluginChanged) {
+        const cleanUpKey = panel.key;
         panel.generateNewKey();
 
-        dispatch(panelModelAndPluginReady({ key: panel.key, plugin: panel.plugin! }));
+        dispatch(panelModelAndPluginReady({ key: panel.key, plugin: panel.plugin!, cleanUpKey }));
       }
 
       // Resend last query result on source panel query runner
@@ -125,9 +126,10 @@ export function exitPanelEditor(): ThunkResult<void> {
       if (panelTypeChanged) {
         // Loaded plugin is not included in the persisted properties so is not handled by restoreModel
         sourcePanel.plugin = panel.plugin;
+        const cleanUpKey = sourcePanel.key;
         sourcePanel.generateNewKey();
 
-        await dispatch(panelModelAndPluginReady({ key: sourcePanel.key, plugin: panel.plugin! }));
+        await dispatch(panelModelAndPluginReady({ key: sourcePanel.key, plugin: panel.plugin!, cleanUpKey }));
       }
 
       // Resend last query result on source panel query runner
@@ -138,7 +140,7 @@ export function exitPanelEditor(): ThunkResult<void> {
       }, 20);
     }
 
-    dispatch(cleanUpPanelState({ key: panel.key }));
+    dispatch(cleanUpPanelState(panel.key));
     dispatch(closeEditor());
   };
 }

public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx

@@ -1,6 +1,6 @@
 import React from 'react';
 import { shallow, ShallowWrapper } from 'enzyme';
-import { DashboardGrid, Props } from './DashboardGrid';
+import { DashboardGridUnconnected as DashboardGrid, Props } from './DashboardGrid';
 import { DashboardModel } from '../state';
 
 jest.mock('app/features/dashboard/dashgrid/LazyLoader', () => {
@@ -67,6 +67,7 @@ function dashboardGridScenario(description: string, scenarioFn: (ctx: ScenarioCo
         editPanel: null,
         viewPanel: null,
         dashboard: getTestDashboard(),
+        cleanAndRemoveMany: jest.fn,
       },
       setProps: (props: Partial<Props>) => {
         Object.assign(ctx.props, props);

public/app/features/dashboard/dashgrid/DashboardGrid.tsx

@@ -13,11 +13,13 @@ import { GRID_CELL_HEIGHT, GRID_CELL_VMARGIN, GRID_COLUMN_COUNT } from 'app/core
 import { DashboardPanel } from './DashboardPanel';
 import { DashboardModel, PanelModel } from '../state';
 import { Subscription } from 'rxjs';
+import { connect, ConnectedProps } from 'react-redux';
 import { DashboardPanelsChangedEvent } from 'app/types/events';
+import { cleanAndRemoveMany } from 'app/features/panel/state/actions';
 import { GridPos } from '../state/PanelModel';
 import { config } from '@grafana/runtime';
 
-export interface Props {
+export interface OwnProps {
   dashboard: DashboardModel;
   editPanel: PanelModel | null;
   viewPanel: PanelModel | null;
@@ -27,7 +29,15 @@ export interface State {
   isLayoutInitialized: boolean;
 }
 
-export class DashboardGrid extends PureComponent<Props, State> {
+const mapDispatchToProps = {
+  cleanAndRemoveMany,
+};
+
+const connector = connect(null, mapDispatchToProps);
+
+export type Props = OwnProps & ConnectedProps<typeof connector>;
+
+export class DashboardGridUnconnected extends PureComponent<Props, State> {
   private panelMap: { [key: string]: PanelModel } = {};
   private eventSubs = new Subscription();
   private windowHeight = 1200;
@@ -51,6 +61,7 @@ export class DashboardGrid extends PureComponent<Props, State> {
 
   componentWillUnmount() {
     this.eventSubs.unsubscribe();
+    this.props.cleanAndRemoveMany(Object.keys(this.panelMap));
   }
 
   buildLayout() {
@@ -315,3 +326,5 @@ function translateGridHeightToScreenHeight(gridHeight: number): number {
 }
 
 GrafanaGridItem.displayName = 'GridItemWithDimensions';
+
+export const DashboardGrid = connector(DashboardGridUnconnected);

public/app/features/dashboard/dashgrid/DashboardPanel.tsx

@@ -5,7 +5,7 @@ import { PanelChromeAngular } from './PanelChromeAngular';
 import { DashboardModel, PanelModel } from '../state';
 import { StoreState } from 'app/types';
 import { PanelPlugin } from '@grafana/data';
-import { cleanUpPanelState, setPanelInstanceState } from '../../panel/state/reducers';
+import { setPanelInstanceState } from '../../panel/state/reducers';
 import { initPanelState } from '../../panel/state/actions';
 import { LazyLoader } from './LazyLoader';
 
@@ -17,7 +17,6 @@ export interface OwnProps {
   isViewing: boolean;
   width: number;
   height: number;
-  skipStateCleanUp?: boolean;
   lazy?: boolean;
 }
 
@@ -35,7 +34,6 @@ const mapStateToProps = (state: StoreState, props: OwnProps) => {
 
 const mapDispatchToProps = {
   initPanelState,
-  cleanUpPanelState,
   setPanelInstanceState,
 };
 
@@ -48,8 +46,6 @@ export class DashboardPanelUnconnected extends PureComponent<Props> {
     lazy: true,
   };
 
-  specialPanels: { [key: string]: Function } = {};
-
   componentDidMount() {
     this.props.panel.isInView = !this.props.lazy;
     if (!this.props.plugin) {
@@ -57,13 +53,6 @@ export class DashboardPanelUnconnected extends PureComponent<Props> {
     }
   }
 
-  componentWillUnmount() {
-    // Most of the time an unmount should result in cleanup but in PanelEdit it should not
-    if (!this.props.skipStateCleanUp) {
-      this.props.cleanUpPanelState({ key: this.props.stateKey });
-    }
-  }
-
   onInstanceStateChange = (value: any) => {
     this.props.setPanelInstanceState({ key: this.props.stateKey, value });
   };

public/app/features/dashboard/utils/panel.ts

@@ -1,5 +1,6 @@
 // Store
 import store from 'app/core/store';
+import { dispatch } from 'app/store/store';
 
 // Models
 import { DashboardModel } from 'app/features/dashboard/state/DashboardModel';
@@ -21,6 +22,7 @@ import { ShareModal } from 'app/features/dashboard/components/ShareModal';
 import { ShowConfirmModalEvent, ShowModalReactEvent } from '../../../types/events';
 import { AddLibraryPanelModal } from 'app/features/library-panels/components/AddLibraryPanelModal/AddLibraryPanelModal';
 import { UnlinkModal } from 'app/features/library-panels/components/UnlinkModal/UnlinkModal';
+import { cleanUpPanelState } from 'app/features/panel/state/actions';
 
 export const removePanel = (dashboard: DashboardModel, panel: PanelModel, ask: boolean) => {
   // confirm deletion
@@ -46,6 +48,7 @@ export const removePanel = (dashboard: DashboardModel, panel: PanelModel, ask: b
   }
 
   dashboard.removePanel(panel);
+  dispatch(cleanUpPanelState(panel.key));
 };
 
 export const duplicatePanel = (dashboard: DashboardModel, panel: PanelModel) => {

public/app/features/panel/state/actions.ts

@@ -2,7 +2,13 @@ import { getPanelPluginNotFound } from 'app/features/panel/components/PanelPlugi
 import { PanelModel } from 'app/features/dashboard/state/PanelModel';
 import { loadPanelPlugin } from 'app/features/plugins/admin/state/actions';
 import { ThunkResult } from 'app/types';
-import { changePanelKey, panelModelAndPluginReady } from './reducers';
+import {
+  changePanelKey,
+  cleanUpAngularComponent,
+  panelModelAndPluginReady,
+  removePanel,
+  removePanels,
+} from './reducers';
 import { LibraryElementDTO } from 'app/features/library-panels/types';
 import { toPanelModelLibraryPanel } from 'app/features/library-panels/utils';
 import { PanelOptionsChangedEvent, PanelQueriesChangedEvent } from 'app/types/events';
@@ -31,6 +37,24 @@ export function initPanelState(panel: PanelModel): ThunkResult<void> {
   };
 }
 
+export function cleanUpPanelState(panelKey: string): ThunkResult<void> {
+  return (dispatch, getStore) => {
+    const store = getStore().panels;
+    cleanUpAngularComponent(store[panelKey]);
+    dispatch(removePanel({ key: panelKey }));
+  };
+}
+
+export function cleanAndRemoveMany(panelKeys: string[]): ThunkResult<void> {
+  return (dispatch, getStore) => {
+    const store = getStore().panels;
+    for (const key of panelKeys) {
+      cleanUpAngularComponent(store[key]);
+    }
+    dispatch(removePanels({ keys: panelKeys }));
+  };
+}
+
 export interface ChangePanelPluginAndOptionsArgs {
   panel: PanelModel;
   pluginId: string;

public/app/features/panel/state/reducers.ts

@@ -30,10 +30,14 @@ const panelsSlice = createSlice({
       state[action.payload.newKey] = state[action.payload.oldKey];
       delete state[action.payload.oldKey];
     },
-    cleanUpPanelState: (state, action: PayloadAction<{ key: string }>) => {
+    removePanel: (state, action: PayloadAction<{ key: string }>) => {
-      cleanUpAngularComponent(state[action.payload.key]);
       delete state[action.payload.key];
     },
+    removePanels: (state, action: PayloadAction<{ keys: string[] }>) => {
+      for (const key of action.payload.keys) {
+        delete state[key];
+      }
+    },
     setPanelInstanceState: (state, action: PayloadAction<SetPanelInstanceStatePayload>) => {
       state[action.payload.key].instanceState = action.payload.value;
     },
@@ -45,7 +49,7 @@ const panelsSlice = createSlice({
   },
 });
 
-function cleanUpAngularComponent(panelState?: Draft<PanelState>) {
+export function cleanUpAngularComponent(panelState?: Draft<PanelState>) {
   if (panelState?.angularComponent) {
     panelState.angularComponent.destroy();
   }
@@ -72,8 +76,9 @@ export const {
   panelModelAndPluginReady,
   setPanelAngularComponent,
   setPanelInstanceState,
-  cleanUpPanelState,
   changePanelKey,
+  removePanel,
+  removePanels,
 } = panelsSlice.actions;
 
 export const panelsReducer = panelsSlice.reducer;