From b07c7797f3244614556542d4d685ea410b2c2df1 Mon Sep 17 00:00:00 2001 From: kay delaney <45561153+kaydelaney@users.noreply.github.com> Date: Tue, 26 Apr 2022 12:44:39 +0100 Subject: [PATCH] Dashboard: Refactor panel cleanup (#47323) (#48134) (cherry picked from commit f10047b7085fd9ae76929d57b2f7c5547e0bf5a2) --- .betterer.results | 2 +- .../components/PanelEditor/PanelEditor.tsx | 1 - .../PanelEditor/state/actions.test.ts | 7 +++-- .../components/PanelEditor/state/actions.ts | 12 +++++---- .../dashboard/dashgrid/DashboardGrid.test.tsx | 3 ++- .../dashboard/dashgrid/DashboardGrid.tsx | 17 ++++++++++-- .../dashboard/dashgrid/DashboardPanel.tsx | 13 +--------- public/app/features/dashboard/utils/panel.ts | 5 ++++ public/app/features/panel/state/actions.ts | 26 ++++++++++++++++++- public/app/features/panel/state/reducers.ts | 13 +++++++--- 10 files changed, 70 insertions(+), 29 deletions(-) diff --git a/.betterer.results b/.betterer.results index 00790c37f74..9e71ad4373f 100644 --- a/.betterer.results +++ b/.betterer.results @@ -203,7 +203,7 @@ exports[`no enzyme tests`] = { "public/app/features/dashboard/components/ShareModal/ShareLink.test.tsx:2357087833": [ [0, 35, 13, "RegExp match", "2409514259"] ], - "public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx:3341831113": [ + "public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx:2723773538": [ [0, 35, 13, "RegExp match", "2409514259"] ], "public/app/features/dashboard/dashgrid/PanelHeader/PanelHeaderCorner.test.tsx:2851646279": [ diff --git a/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx b/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx index 1ef88faf019..59c92814742 100644 --- a/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx +++ b/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx @@ -252,7 +252,6 @@ export class PanelEditorUnconnected extends PureComponent { lazy={false} width={panelSize.width} height={panelSize.height} - skipStateCleanUp={true} /> diff --git a/public/app/features/dashboard/components/PanelEditor/state/actions.test.ts b/public/app/features/dashboard/components/PanelEditor/state/actions.test.ts index 527ef6afa8f..b52df7d3ae9 100644 --- a/public/app/features/dashboard/components/PanelEditor/state/actions.test.ts +++ b/public/app/features/dashboard/components/PanelEditor/state/actions.test.ts @@ -1,4 +1,4 @@ -import { cleanUpPanelState, panelModelAndPluginReady } from 'app/features/panel/state/reducers'; +import { panelModelAndPluginReady, removePanel } from 'app/features/panel/state/reducers'; import { getPanelPlugin } from 'app/features/plugins/__mocks__/pluginMocks'; import { thunkTester } from '../../../../../../test/core/thunk/thunkTester'; @@ -50,6 +50,7 @@ describe('panelEditor actions', () => { }; const dispatchedActions = await thunkTester({ + panels: {}, panelEditor: state, dashboard: { getModel: () => dashboard, @@ -59,7 +60,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); @@ -86,6 +87,7 @@ describe('panelEditor actions', () => { const dispatchedActions = await thunkTester({ panelEditor: state, + panels: {}, dashboard: { getModel: () => dashboard, }, @@ -121,6 +123,7 @@ describe('panelEditor actions', () => { const dispatchedActions = await thunkTester({ panelEditor: state, + panels: {}, dashboard: { getModel: () => dashboard, }, diff --git a/public/app/features/dashboard/components/PanelEditor/state/actions.ts b/public/app/features/dashboard/components/PanelEditor/state/actions.ts index 4deb268cd2b..a2c037843eb 100644 --- a/public/app/features/dashboard/components/PanelEditor/state/actions.ts +++ b/public/app/features/dashboard/components/PanelEditor/state/actions.ts @@ -1,8 +1,8 @@ import { pick } from 'lodash'; import store from 'app/core/store'; -import { initPanelState } from 'app/features/panel/state/actions'; -import { cleanUpPanelState, panelModelAndPluginReady } from 'app/features/panel/state/reducers'; +import { cleanUpPanelState, initPanelState } from 'app/features/panel/state/actions'; +import { panelModelAndPluginReady } from 'app/features/panel/state/reducers'; import { ThunkResult } from 'app/types'; import { DashboardModel, PanelModel } from '../../../state'; @@ -66,9 +66,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 @@ -128,9 +129,10 @@ export function exitPanelEditor(): ThunkResult { 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 @@ -141,7 +143,7 @@ export function exitPanelEditor(): ThunkResult { }, 20); } - dispatch(cleanUpPanelState({ key: panel.key })); + dispatch(cleanUpPanelState(panel.key)); dispatch(closeEditor()); }; } diff --git a/public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx b/public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx index 56fe1f40ea3..95963ad97d5 100644 --- a/public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx +++ b/public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx @@ -3,7 +3,7 @@ import React from 'react'; import { DashboardModel } from '../state'; -import { DashboardGrid, Props } from './DashboardGrid'; +import { DashboardGridUnconnected as DashboardGrid, Props } from './DashboardGrid'; jest.mock('app/features/dashboard/dashgrid/LazyLoader', () => { const LazyLoader: React.FC = ({ children }) => { @@ -69,6 +69,7 @@ function dashboardGridScenario(description: string, scenarioFn: (ctx: ScenarioCo editPanel: null, viewPanel: null, dashboard: getTestDashboard(), + cleanAndRemoveMany: jest.fn, }, setProps: (props: Partial) => { Object.assign(ctx.props, props); diff --git a/public/app/features/dashboard/dashgrid/DashboardGrid.tsx b/public/app/features/dashboard/dashgrid/DashboardGrid.tsx index 2c05a67e85b..e5d8e613f20 100644 --- a/public/app/features/dashboard/dashgrid/DashboardGrid.tsx +++ b/public/app/features/dashboard/dashgrid/DashboardGrid.tsx @@ -1,11 +1,13 @@ import classNames from 'classnames'; import React, { PureComponent, CSSProperties } from 'react'; import ReactGridLayout, { ItemCallback } from 'react-grid-layout'; +import { connect, ConnectedProps } from 'react-redux'; import AutoSizer from 'react-virtualized-auto-sizer'; import { Subscription } from 'rxjs'; import { config } from '@grafana/runtime'; import { GRID_CELL_HEIGHT, GRID_CELL_VMARGIN, GRID_COLUMN_COUNT } from 'app/core/constants'; +import { cleanAndRemoveMany } from 'app/features/panel/state/actions'; import { DashboardPanelsChangedEvent } from 'app/types/events'; import { AddPanelWidget } from '../components/AddPanelWidget'; @@ -15,7 +17,7 @@ import { GridPos } from '../state/PanelModel'; import { DashboardPanel } from './DashboardPanel'; -export interface Props { +export interface OwnProps { dashboard: DashboardModel; editPanel: PanelModel | null; viewPanel: PanelModel | null; @@ -25,7 +27,15 @@ export interface State { isLayoutInitialized: boolean; } -export class DashboardGrid extends PureComponent { +const mapDispatchToProps = { + cleanAndRemoveMany, +}; + +const connector = connect(null, mapDispatchToProps); + +export type Props = OwnProps & ConnectedProps; + +export class DashboardGridUnconnected extends PureComponent { private panelMap: { [key: string]: PanelModel } = {}; private eventSubs = new Subscription(); private windowHeight = 1200; @@ -49,6 +59,7 @@ export class DashboardGrid extends PureComponent { componentWillUnmount() { this.eventSubs.unsubscribe(); + this.props.cleanAndRemoveMany(Object.keys(this.panelMap)); } buildLayout() { @@ -313,3 +324,5 @@ function translateGridHeightToScreenHeight(gridHeight: number): number { } GrafanaGridItem.displayName = 'GridItemWithDimensions'; + +export const DashboardGrid = connector(DashboardGridUnconnected); diff --git a/public/app/features/dashboard/dashgrid/DashboardPanel.tsx b/public/app/features/dashboard/dashgrid/DashboardPanel.tsx index bcb36c299fc..773e6de1709 100644 --- a/public/app/features/dashboard/dashgrid/DashboardPanel.tsx +++ b/public/app/features/dashboard/dashgrid/DashboardPanel.tsx @@ -4,7 +4,7 @@ import { connect, ConnectedProps } from 'react-redux'; import { StoreState } from 'app/types'; import { initPanelState } from '../../panel/state/actions'; -import { cleanUpPanelState, setPanelInstanceState } from '../../panel/state/reducers'; +import { setPanelInstanceState } from '../../panel/state/reducers'; import { DashboardModel, PanelModel } from '../state'; import { LazyLoader } from './LazyLoader'; @@ -19,7 +19,6 @@ export interface OwnProps { isViewing: boolean; width: number; height: number; - skipStateCleanUp?: boolean; lazy?: boolean; } @@ -37,7 +36,6 @@ const mapStateToProps = (state: StoreState, props: OwnProps) => { const mapDispatchToProps = { initPanelState, - cleanUpPanelState, setPanelInstanceState, }; @@ -50,8 +48,6 @@ export class DashboardPanelUnconnected extends PureComponent { lazy: true, }; - specialPanels: { [key: string]: Function } = {}; - componentDidMount() { this.props.panel.isInView = !this.props.lazy; if (!this.props.lazy) { @@ -59,13 +55,6 @@ export class DashboardPanelUnconnected extends PureComponent { } } - 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 }); }; diff --git a/public/app/features/dashboard/utils/panel.ts b/public/app/features/dashboard/utils/panel.ts index 9adb63d3e7e..6bd6d17eb9b 100644 --- a/public/app/features/dashboard/utils/panel.ts +++ b/public/app/features/dashboard/utils/panel.ts @@ -1,5 +1,7 @@ import { isString as _isString } from 'lodash'; +// Store + import { TimeRange, AppEvents, rangeUtil, dateMath, PanelModel as IPanelModel } from '@grafana/data'; import { getTemplateSrv } from '@grafana/runtime'; import appEvents from 'app/core/app_events'; @@ -11,6 +13,8 @@ import { DashboardModel } from 'app/features/dashboard/state/DashboardModel'; import { PanelModel } from 'app/features/dashboard/state/PanelModel'; 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'; +import { dispatch } from 'app/store/store'; import { ShowConfirmModalEvent, ShowModalReactEvent } from '../../../types/events'; @@ -38,6 +42,7 @@ export const removePanel = (dashboard: DashboardModel, panel: PanelModel, ask: b } dashboard.removePanel(panel); + dispatch(cleanUpPanelState(panel.key)); }; export const duplicatePanel = (dashboard: DashboardModel, panel: PanelModel) => { diff --git a/public/app/features/panel/state/actions.ts b/public/app/features/panel/state/actions.ts index 45c014b0d09..1bfb01dac2b 100644 --- a/public/app/features/panel/state/actions.ts +++ b/public/app/features/panel/state/actions.ts @@ -8,7 +8,13 @@ import { loadPanelPlugin } from 'app/features/plugins/admin/state/actions'; import { ThunkResult } from 'app/types'; import { PanelOptionsChangedEvent, PanelQueriesChangedEvent } from 'app/types/events'; -import { changePanelKey, panelModelAndPluginReady } from './reducers'; +import { + changePanelKey, + cleanUpAngularComponent, + panelModelAndPluginReady, + removePanel, + removePanels, +} from './reducers'; export function initPanelState(panel: PanelModel): ThunkResult { return async (dispatch, getStore) => { @@ -32,6 +38,24 @@ export function initPanelState(panel: PanelModel): ThunkResult { }; } +export function cleanUpPanelState(panelKey: string): ThunkResult { + return (dispatch, getStore) => { + const store = getStore().panels; + cleanUpAngularComponent(store[panelKey]); + dispatch(removePanel({ key: panelKey })); + }; +} + +export function cleanAndRemoveMany(panelKeys: string[]): ThunkResult { + 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; diff --git a/public/app/features/panel/state/reducers.ts b/public/app/features/panel/state/reducers.ts index a8b86c329db..7c61f4f46cc 100644 --- a/public/app/features/panel/state/reducers.ts +++ b/public/app/features/panel/state/reducers.ts @@ -31,10 +31,14 @@ const panelsSlice = createSlice({ state[action.payload.newKey] = state[action.payload.oldKey]; delete state[action.payload.oldKey]; }, - cleanUpPanelState: (state, action: PayloadAction<{ key: string }>) => { - cleanUpAngularComponent(state[action.payload.key]); + removePanel: (state, action: PayloadAction<{ key: string }>) => { 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) => { state[action.payload.key].instanceState = action.payload.value; }, @@ -46,7 +50,7 @@ const panelsSlice = createSlice({ }, }); -function cleanUpAngularComponent(panelState?: Draft) { +export function cleanUpAngularComponent(panelState?: Draft) { if (panelState?.angularComponent) { panelState.angularComponent.destroy(); } @@ -73,8 +77,9 @@ export const { panelModelAndPluginReady, setPanelAngularComponent, setPanelInstanceState, - cleanUpPanelState, changePanelKey, + removePanel, + removePanels, } = panelsSlice.actions; export const panelsReducer = panelsSlice.reducer;