From f10047b7085fd9ae76929d57b2f7c5547e0bf5a2 Mon Sep 17 00:00:00 2001 From: kay delaney <45561153+kaydelaney@users.noreply.github.com> Date: Tue, 12 Apr 2022 14:12:03 +0100 Subject: [PATCH] Dashboard: Refactor panel cleanup (#47323) --- .betterer.results | 2 +- .../components/PanelEditor/PanelEditor.tsx | 1 - .../PanelEditor/state/actions.test.ts | 7 +++-- .../components/PanelEditor/state/actions.ts | 18 +++++++------ .../dashboard/dashgrid/DashboardGrid.test.tsx | 3 ++- .../dashboard/dashgrid/DashboardGrid.tsx | 17 ++++++++++-- .../dashboard/dashgrid/DashboardPanel.tsx | 13 +--------- public/app/features/dashboard/utils/panel.ts | 3 +++ public/app/features/panel/state/actions.ts | 26 ++++++++++++++++++- public/app/features/panel/state/reducers.ts | 13 +++++++--- 10 files changed, 71 insertions(+), 32 deletions(-) diff --git a/.betterer.results b/.betterer.results index 57cc3ad8dce..b57253c101f 100644 --- a/.betterer.results +++ b/.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": [ diff --git a/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx b/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx index 9f61c323bf6..51d91021993 100644 --- a/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx +++ b/public/app/features/dashboard/components/PanelEditor/PanelEditor.tsx @@ -256,7 +256,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 a41ece9b4de..1e864f160ac 100644 --- a/public/app/features/dashboard/components/PanelEditor/state/actions.test.ts +++ b/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, }, diff --git a/public/app/features/dashboard/components/PanelEditor/state/actions.ts b/public/app/features/dashboard/components/PanelEditor/state/actions.ts index f604c29128f..7093dd47cdc 100644 --- a/public/app/features/dashboard/components/PanelEditor/state/actions.ts +++ b/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 store from 'app/core/store'; -import { pick } from 'lodash'; -import { initPanelState } from 'app/features/panel/state/actions'; +import { DashboardModel, PanelModel } from '../../../state'; export function initPanelEditor(sourcePanel: PanelModel, dashboard: DashboardModel): ThunkResult { 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 { 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 { }, 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 891a0f0a1f6..e12caba291c 100644 --- a/public/app/features/dashboard/dashgrid/DashboardGrid.test.tsx +++ b/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) => { Object.assign(ctx.props, props); diff --git a/public/app/features/dashboard/dashgrid/DashboardGrid.tsx b/public/app/features/dashboard/dashgrid/DashboardGrid.tsx index 34a5eeb6926..29d6578dc9a 100644 --- a/public/app/features/dashboard/dashgrid/DashboardGrid.tsx +++ b/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 { +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; @@ -51,6 +61,7 @@ export class DashboardGrid extends PureComponent { 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); diff --git a/public/app/features/dashboard/dashgrid/DashboardPanel.tsx b/public/app/features/dashboard/dashgrid/DashboardPanel.tsx index 668ed6e8d42..0401ecf9397 100644 --- a/public/app/features/dashboard/dashgrid/DashboardPanel.tsx +++ b/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 { 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 { } } - 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 2c68b00cd5b..d92497a43f0 100644 --- a/public/app/features/dashboard/utils/panel.ts +++ b/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) => { diff --git a/public/app/features/panel/state/actions.ts b/public/app/features/panel/state/actions.ts index f3c9d51fa9e..0864c763632 100644 --- a/public/app/features/panel/state/actions.ts +++ b/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 { }; } +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 4fe2f8dce70..b809a223ba4 100644 --- a/public/app/features/panel/state/reducers.ts +++ b/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 }>) => { - 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; }, @@ -45,7 +49,7 @@ const panelsSlice = createSlice({ }, }); -function cleanUpAngularComponent(panelState?: Draft) { +export function cleanUpAngularComponent(panelState?: Draft) { 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;