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

packages/grafana-runtime/src/services/LocationService.ts

@@ -11,7 +11,7 @@ import { config } from '../config';
 export interface LocationService {
   partial: (query: Record<string, any>, replace?: boolean) => void;
   push: (location: H.Path | H.LocationDescriptor<any>) => void;
-  replace: (location: H.Path | H.LocationDescriptor<any>, forceRouteReload?: boolean) => void;
+  replace: (location: H.Path | H.LocationDescriptor<any>) => 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 },
     });
   }
 

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 <div />;
-      },
-    } as any;
-
-    const history = locationService.getHistory();
-
-    const { rerender } = render(
-      <GrafanaRoute location={history.location} history={history} match={{} as any} route={route} />
-    );
-
-    expect(renderSpy).toBeCalledTimes(1);
-    locationService.replace('/test', true);
-    expect(history.location.state).toMatchInlineSnapshot(`
-      Object {
-        "forceRouteReload": true,
-      }
-    `);
-
-    rerender(<GrafanaRoute location={history.location} history={history} match={{} as any} route={route} />);
-
-    expect(history.location.state).toMatchInlineSnapshot(`Object {}`);
-    expect(renderSpy).toBeCalledTimes(2);
-  });
 });

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<GrafanaRouteComponentProps, 'queryParams'> {}
@@ -13,7 +12,6 @@ export class GrafanaRoute extends React.Component<Props> {
   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<Props> {
 
   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);
   }

public/app/core/navigation/utils.ts

@@ -1,5 +0,0 @@
-import * as H from 'history';
-
-export function shouldReloadPage(location: H.Location<any>) {
-  return !!location.state?.forceRouteReload;
-}

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 };
 };

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<Props, State> {
+  private forceRouteReloadCounter = 0;
   state: State = this.getCleanState();
 
   getCleanState(): State {
@@ -82,6 +82,7 @@ export class DashboardPage extends PureComponent<Props, State> {
 
   componentDidMount() {
     this.initDashboard();
+    this.forceRouteReloadCounter = (this.props.history.location.state as any)?.routeReloadCounter || 0;
   }
 
   componentWillUnmount() {
@@ -116,6 +117,8 @@ export class DashboardPage extends PureComponent<Props, State> {
     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<Props, State> {
       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;
     }