AppRouting should preserve hash on same-RouteId navigations

tensorboard/webapp/app_routing/effects/app_routing_effects.ts

@@ -33,7 +33,7 @@ import {
   navigationRequested,
   stateRehydratedFromUrl,
 } from '../actions';
-import {areRoutesEqual} from '../internal_utils';
+import {areRoutesEqual, getRouteId} from '../internal_utils';
 import {Location} from '../location';
 import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module';
 import {RouteConfigs} from '../route_config';
@@ -229,8 +229,26 @@ export class AppRoutingEffects {
         withLatestFrom(this.store.select(getActiveRoute)),
         filter(([, route]) => Boolean(route)),
         map(([navigatedAction, route]) => {
+          // The URL hash can be set via HashStorageComponent (which uses
+          // Polymer's tf-storage). DeepLinkProviders also modify the URL when
+          // a provider's serializeStateToQueryParams() emits. These result in
+          // the URL updated without the previous hash. HashStorageComponent
+          // makes no attempt to restore the hash, so it is dropped.
+
+          // This results in bad behavior when refreshing (e.g. lost active
+          // plugin) and when changing dashboards (e.g. lost tagFilter).
+
+          // TODO(b/169799696): either AppRouting should manage the URL entirely
+          // (including hash), or we make the app wait for AppRouting to
+          // initialize before setting the active plugin hash.
+          // See https://github.com/tensorflow/tensorboard/issues/4207.
+          const oldRoute = navigatedAction.before;
+          const preserveHash =
+            oldRoute === null ||
+            getRouteId(oldRoute.routeKind, oldRoute.params) ===
+              getRouteId(route!.routeKind, route!.params);
           return {
-            isFirstNavigation: navigatedAction.before === null,
+            preserveHash,
             route: route!,
           };
         }),
@@ -240,25 +258,14 @@ export class AppRoutingEffects {
             queryParams: this.location.getSearch(),
           });
         }),
-        tap(({isFirstNavigation, route}) => {
-          // AppRouting's effect to dispatch the initial navigated action is
-          // asynchronous. It is possible to see a race condition, where the
-          // network request for plugins_listing returns first, which sets the
-          // URL hash to '#scalars', only to be discarded by the replaceState
-          // below. Preserving the hash upon initial navigation is a workaround.
-
-          // TODO(b/169799696): either AppRouting should manage the URL entirely
-          // (including hash), or we make the app wait for AppRouting to
-          // initialize before setting the active plugin hash.
-          // See https://github.com/tensorflow/tensorboard/issues/4207.
-          const shouldPreserveHash = isFirstNavigation;
+        tap(({preserveHash, route}) => {
           if (route.navigationOptions.replaceState) {
             this.location.replaceState(
-              this.location.getFullPathFromRouteOrNav(route, shouldPreserveHash)
+              this.location.getFullPathFromRouteOrNav(route, preserveHash)
             );
           } else {
             this.location.pushState(
-              this.location.getFullPathFromRouteOrNav(route, shouldPreserveHash)
+              this.location.getFullPathFromRouteOrNav(route, preserveHash)
             );
           }
         })

tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts

@@ -651,7 +651,45 @@ describe('app_routing_effects', () => {
       expect(replaceStateSpy).toHaveBeenCalledWith('/experiments#foo');
     });
 
-    it('does not preserve hash upon replace for non-initial navigation', () => {
+    // This hash preservation spec may become obsolete. If we enable app_routing
+    // to properly set the URL hash, and all TB embedders use app_routing, then
+    // this spec can be removed.
+    it('preserves hash upon navigations to the same route id', () => {
+      const activeRoute = buildRoute({
+        routeKind: RouteKind.EXPERIMENT,
+        pathname: '/experiment',
+        queryParams: [],
+        navigationOptions: {
+          replaceState: true,
+        },
+      });
+      const nextActiveRoute = buildRoute({
+        routeKind: RouteKind.EXPERIMENT,
+        pathname: '/experiment',
+        queryParams: [{key: 'q', value: 'new_value'}],
+        navigationOptions: {
+          replaceState: true,
+        },
+      });
+      store.overrideSelector(getActiveRoute, nextActiveRoute);
+      store.refreshState();
+      getHashSpy.and.returnValue('#foo');
+      getPathSpy.and.returnValue('meow');
+      getSearchSpy.and.returnValue([]);
+
+      action.next(
+        actions.navigated({
+          before: activeRoute,
+          after: nextActiveRoute,
+        })
+      );
+
+      expect(replaceStateSpy).toHaveBeenCalledWith(
+        '/experiment?q=new_value#foo'
+      );
+    });
+
+    it('discards hash upon navigations to a new route id', () => {
       const activeRoute = buildRoute({
         routeKind: RouteKind.EXPERIMENTS,
         pathname: '/experiments',
@@ -663,6 +701,8 @@ describe('app_routing_effects', () => {
       const nextActiveRoute = buildRoute({
         routeKind: RouteKind.EXPERIMENT,
         pathname: '/experiment',
+        // Changing route params produces a new route id.
+        params: {experimentId: '123'},
         queryParams: [],
         navigationOptions: {
           replaceState: true,