From 0d92425bb8012aa5971a17a25861e6db77b7caba Mon Sep 17 00:00:00 2001 From: Dominik Prokop Date: Tue, 16 Mar 2021 09:27:32 +0100 Subject: [PATCH] RoutingNG: Fix infinite loop caused by history state not being cleaned up (#31952) * Fix infinite loop caused by history state not being cleaned up * Ude forceRouteReload counter instead of bool flag * Review fix --- .../src/services/LocationService.ts | 18 +++-------- .../app/core/navigation/GrafanaRoute.test.tsx | 32 ------------------- public/app/core/navigation/GrafanaRoute.tsx | 9 ------ public/app/core/navigation/utils.ts | 5 --- .../VersionHistory/useDashboardRestore.tsx | 10 +++++- .../dashboard/containers/DashboardPage.tsx | 11 +++++-- 6 files changed, 23 insertions(+), 62 deletions(-) delete mode 100644 public/app/core/navigation/utils.ts diff --git a/packages/grafana-runtime/src/services/LocationService.ts b/packages/grafana-runtime/src/services/LocationService.ts index 518b37f4dcf..d113a9adbbe 100644 --- a/packages/grafana-runtime/src/services/LocationService.ts +++ b/packages/grafana-runtime/src/services/LocationService.ts @@ -11,7 +11,7 @@ import { config } from '../config'; export interface LocationService { partial: (query: Record, replace?: boolean) => void; push: (location: H.Path | H.LocationDescriptor) => void; - replace: (location: H.Path | H.LocationDescriptor, forceRouteReload?: boolean) => void; + replace: (location: H.Path | H.LocationDescriptor) => void; reload: () => void; getLocation: () => H.Location; getHistory: () => H.History; @@ -95,23 +95,15 @@ export class HistoryWrapper implements LocationService { this.history.push(location); } - replace(location: H.Path | H.LocationDescriptor, forceRouteReload?: boolean) { - const state = forceRouteReload ? { forceRouteReload: true } : undefined; - - if (typeof location === 'string') { - this.history.replace(location, state); - } else { - this.history.replace({ - ...location, - state, - }); - } + replace(location: H.Path | H.LocationDescriptor) { + this.history.replace(location); } reload() { + const prevState = (this.history.location.state as any)?.routeReloadCounter; this.history.replace({ ...this.history.location, - state: { forceRouteReload: true }, + state: { routeReloadCounter: prevState ? prevState + 1 : 1 }, }); } diff --git a/public/app/core/navigation/GrafanaRoute.test.tsx b/public/app/core/navigation/GrafanaRoute.test.tsx index 67e60955f6b..26f6552857b 100644 --- a/public/app/core/navigation/GrafanaRoute.test.tsx +++ b/public/app/core/navigation/GrafanaRoute.test.tsx @@ -1,7 +1,6 @@ import React from 'react'; import { render } from '@testing-library/react'; import { GrafanaRoute } from './GrafanaRoute'; -import { locationService } from '@grafana/runtime'; describe('GrafanaRoute', () => { it('Parses search', () => { @@ -22,35 +21,4 @@ describe('GrafanaRoute', () => { expect(capturedProps.queryParams.query).toBe('hello'); }); - - it('Should clear history forceRouteReload state after route change', () => { - const renderSpy = jest.fn(); - - const route = { - /* eslint-disable-next-line react/display-name */ - component: () => { - renderSpy(); - return
; - }, - } as any; - - const history = locationService.getHistory(); - - const { rerender } = render( - - ); - - expect(renderSpy).toBeCalledTimes(1); - locationService.replace('/test', true); - expect(history.location.state).toMatchInlineSnapshot(` - Object { - "forceRouteReload": true, - } - `); - - rerender(); - - expect(history.location.state).toMatchInlineSnapshot(`Object {}`); - expect(renderSpy).toBeCalledTimes(2); - }); }); diff --git a/public/app/core/navigation/GrafanaRoute.tsx b/public/app/core/navigation/GrafanaRoute.tsx index c45ac21703e..b20100480f5 100644 --- a/public/app/core/navigation/GrafanaRoute.tsx +++ b/public/app/core/navigation/GrafanaRoute.tsx @@ -4,7 +4,6 @@ import Drop from 'tether-drop'; import { GrafanaRouteComponentProps } from './types'; import { locationSearchToObject, navigationLogger } from '@grafana/runtime'; import { keybindingSrv } from '../services/keybindingSrv'; -import { shouldReloadPage } from './utils'; import { analyticsService } from '../services/analytics'; export interface Props extends Omit {} @@ -13,7 +12,6 @@ export class GrafanaRoute extends React.Component { componentDidMount() { this.updateBodyClassNames(); this.cleanupDOM(); - // unbinds all and re-bind global keybindins keybindingSrv.reset(); keybindingSrv.initGlobals(); @@ -23,13 +21,6 @@ export class GrafanaRoute extends React.Component { componentDidUpdate(prevProps: Props) { this.cleanupDOM(); - - // Clear force reload state when route updates - if (shouldReloadPage(this.props.location)) { - navigationLogger('GrafanaRoute', false, 'Force reload', this.props, prevProps); - delete (this.props.history.location.state as any)?.forceRouteReload; - } - analyticsService.track(); navigationLogger('GrafanaRoute', false, 'Updated', this.props, prevProps); } diff --git a/public/app/core/navigation/utils.ts b/public/app/core/navigation/utils.ts deleted file mode 100644 index d6bfe40529c..00000000000 --- a/public/app/core/navigation/utils.ts +++ /dev/null @@ -1,5 +0,0 @@ -import * as H from 'history'; - -export function shouldReloadPage(location: H.Location) { - return !!location.state?.forceRouteReload; -} diff --git a/public/app/features/dashboard/components/VersionHistory/useDashboardRestore.tsx b/public/app/features/dashboard/components/VersionHistory/useDashboardRestore.tsx index f115f48fac5..e2f769be0da 100644 --- a/public/app/features/dashboard/components/VersionHistory/useDashboardRestore.tsx +++ b/public/app/features/dashboard/components/VersionHistory/useDashboardRestore.tsx @@ -15,12 +15,20 @@ const restoreDashboard = async (version: number, dashboard: DashboardModel) => { export const useDashboardRestore = (version: number) => { const dashboard = useSelector((state: StoreState) => state.dashboard.getModel()); const [state, onRestoreDashboard] = useAsyncFn(async () => await restoreDashboard(version, dashboard!), []); + useEffect(() => { if (state.value) { + const location = locationService.getLocation(); const newUrl = locationUtil.stripBaseFromUrl(state.value.url); - locationService.replace(newUrl, true); + const prevState = (location.state as any)?.routeReloadCounter; + locationService.replace({ + ...location, + pathname: newUrl, + state: { routeReloadCounter: prevState ? prevState + 1 : 1 }, + }); appEvents.emit(AppEvents.alertSuccess, ['Dashboard restored', 'Restored from version ' + version]); } }, [state]); + return { state, onRestoreDashboard }; }; diff --git a/public/app/features/dashboard/containers/DashboardPage.tsx b/public/app/features/dashboard/containers/DashboardPage.tsx index b718219dee2..224b04707a3 100644 --- a/public/app/features/dashboard/containers/DashboardPage.tsx +++ b/public/app/features/dashboard/containers/DashboardPage.tsx @@ -25,7 +25,6 @@ import { findTemplateVarChanges } from '../../variables/utils'; import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; import { getTimeSrv } from '../services/TimeSrv'; -import { shouldReloadPage } from 'app/core/navigation/utils'; import { getKioskMode } from 'app/core/navigation/kiosk'; import { UrlQueryValue } from '@grafana/data'; @@ -68,6 +67,7 @@ export interface State { } export class DashboardPage extends PureComponent { + private forceRouteReloadCounter = 0; state: State = this.getCleanState(); getCleanState(): State { @@ -82,6 +82,7 @@ export class DashboardPage extends PureComponent { componentDidMount() { this.initDashboard(); + this.forceRouteReloadCounter = (this.props.history.location.state as any)?.routeReloadCounter || 0; } componentWillUnmount() { @@ -116,6 +117,8 @@ export class DashboardPage extends PureComponent { const { dashboard, match, queryParams, templateVarsChangedInUrl } = this.props; const { editPanel, viewPanel } = this.state; + const routeReloadCounter = (this.props.history.location.state as any)?.routeReloadCounter; + if (!dashboard) { return; } @@ -125,8 +128,12 @@ export class DashboardPage extends PureComponent { document.title = dashboard.title + ' - ' + Branding.AppTitle; } - if (prevProps.match.params.uid !== match.params.uid || shouldReloadPage(this.props.location)) { + if ( + prevProps.match.params.uid !== match.params.uid || + (routeReloadCounter !== undefined && this.forceRouteReloadCounter !== routeReloadCounter) + ) { this.initDashboard(); + this.forceRouteReloadCounter = routeReloadCounter; return; }