routing: improve compare navigation (#4908)

When wanting to navigate to a compare route with a programmatical API,
we need to form the navigation parameter correctly, knowing its encoding
scheme. Not wanting to leak the business logic for encoding alias and
id, we now changed the interface for the compare route navigation with a
translation to an AppRouting Route in the AppRouting effects.

Author: stephanwlee

tensorboard/webapp/app_routing/effects/app_routing_effects.ts

@@ -12,7 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
-import {Inject, Injectable} from '@angular/core';
+import {Injectable} from '@angular/core';
 import {Actions, createEffect, ofType} from '@ngrx/effects';
 import {Action, createAction, Store} from '@ngrx/store';
 import {merge, Observable, of} from 'rxjs';
@@ -34,13 +34,17 @@ import {
   stateRehydratedFromUrl,
 } from '../actions';
 import {AppRootProvider} from '../app_root';
-import {areRoutesEqual, getRouteId} from '../internal_utils';
+import {
+  areRoutesEqual,
+  getRouteId,
+  serializeCompareExperimentParams,
+} from '../internal_utils';
 import {Location} from '../location';
 import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module';
 import {RouteConfigs} from '../route_config';
 import {RouteRegistryModule} from '../route_registry_module';
 import {getActiveRoute} from '../store/app_routing_selectors';
-import {Navigation, Route} from '../types';
+import {Navigation, Route, RouteKind, RouteParams} from '../types';
 
 /** @typehack */ import * as _typeHackNgrxEffects from '@ngrx/effects';
 /** @typehack */ import * as _typeHackModels from '@ngrx/store/src/models';
@@ -144,7 +148,26 @@ export class AppRoutingEffects {
       return nav !== null;
     }),
     map((programmaticalNavigation) => {
-      const {routeKind, routeParams} = programmaticalNavigation!;
+      const nav = programmaticalNavigation!;
+      const routeKind = nav.routeKind;
+
+      // TODO(stephanwlee): currently, the RouteParams is ill-typed and you can
+      // currently add any property without any type error. Better type it.
+      let routeParams: RouteParams;
+      switch (nav.routeKind) {
+        case RouteKind.COMPARE_EXPERIMENT:
+          routeParams = {
+            experimentIds: serializeCompareExperimentParams(
+              nav.routeParams.aliasAndExperimentIds
+            ),
+          };
+          break;
+        default:
+          routeParams = nav.routeParams;
+      }
+      return {routeKind, routeParams};
+    }),
+    map(({routeKind, routeParams}) => {
       const routeMatch = this.routeConfigs
         ? this.routeConfigs.matchByRouteKind(routeKind, routeParams)
         : null;

tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts

@@ -16,7 +16,7 @@ limitations under the License.
 import {Component} from '@angular/core';
 import {fakeAsync, flush, TestBed, tick} from '@angular/core/testing';
 import {provideMockActions} from '@ngrx/effects/testing';
-import {Action, createAction, Store} from '@ngrx/store';
+import {Action, createAction, props, Store} from '@ngrx/store';
 import {MockStore, provideMockStore} from '@ngrx/store/testing';
 import {of, ReplaySubject} from 'rxjs';
 
@@ -26,6 +26,7 @@ import {navigationRequested} from '../actions';
 import {AppRootProvider, TestableAppRootProvider} from '../app_root';
 import {Location} from '../location';
 import {
+  NavigateToCompare,
   NavigateToExperiments,
   ProgrammaticalNavigationModule,
 } from '../programmatical_navigation_module';
@@ -40,6 +41,10 @@ import {AppRoutingEffects, TEST_ONLY} from './app_routing_effects';
 class TestableComponent {}
 
 const testAction = createAction('[TEST] test actions');
+const testNavToCompareAction = createAction(
+  '[TEST] test nav to compare',
+  props<NavigateToCompare['routeParams']>()
+);
 
 describe('app_routing_effects', () => {
   let effects: AppRoutingEffects;
@@ -87,10 +92,10 @@ describe('app_routing_effects', () => {
       ];
     }
 
-    function programmaticalNavigationFactory() {
+    function programmaticalNavToListviewFactory() {
       return {
         actionCreator: testAction,
-        lambda: (action: typeof testAction) => {
+        lambda: (action: ReturnType<typeof testAction>) => {
           return {
             routeKind: RouteKind.EXPERIMENTS,
             routeParams: {},
@@ -99,11 +104,26 @@ describe('app_routing_effects', () => {
       };
     }
 
+    function programmaticalCompareNavigationFactory() {
+      return {
+        actionCreator: testNavToCompareAction,
+        lambda: (action: ReturnType<typeof testNavToCompareAction>) => {
+          return {
+            routeKind: RouteKind.COMPARE_EXPERIMENT,
+            routeParams: action,
+          } as NavigateToCompare;
+        },
+      };
+    }
+
     await TestBed.configureTestingModule({
       imports: [
         RouteRegistryModule.registerRoutes(routeFactory),
         ProgrammaticalNavigationModule.registerProgrammaticalNavigation(
-          programmaticalNavigationFactory
+          programmaticalNavToListviewFactory
+        ),
+        ProgrammaticalNavigationModule.registerProgrammaticalNavigation(
+          programmaticalCompareNavigationFactory
         ),
       ],
       providers: [
@@ -539,6 +559,33 @@ describe('app_routing_effects', () => {
           }),
         ]);
       });
+
+      it('translates programmatical compare nav correctly', () => {
+        store.overrideSelector(getActiveRoute, null);
+        store.refreshState();
+
+        action.next(
+          testNavToCompareAction({
+            aliasAndExperimentIds: [
+              {alias: 'bar', id: 'foo'},
+              {alias: 'omega', id: 'alpha'},
+            ],
+          })
+        );
+
+        expect(actualActions).toEqual([
+          actions.navigating({
+            after: buildRoute({
+              routeKind: RouteKind.COMPARE_EXPERIMENT,
+              params: {
+                experimentIds: 'bar:foo,omega:alpha',
+              },
+              pathname: '/compare',
+              queryParams: [],
+            }),
+          }),
+        ]);
+      });
     });
   });
 

tensorboard/webapp/app_routing/internal_utils.ts

@@ -54,6 +54,12 @@ export function parseCompareExperimentStr(
   });
 }
 
+export function serializeCompareExperimentParams(
+  params: Array<{alias: string; id: string}>
+): string {
+  return params.map(({alias, id}) => `${alias}:${id}`).join(',');
+}
+
 /**
  * Returns experimentIds from route parameter. For a route that does not contain
  * any experiment ids, it returns null.

tensorboard/webapp/app_routing/internal_utils_test.ts

@@ -44,6 +44,35 @@ describe('app_routing/utils', () => {
     });
   });
 
+  describe('#serializeCompareExperimentParams', () => {
+    it('serializes to empty string with an empty input', () => {
+      expect(utils.serializeCompareExperimentParams([])).toBe('');
+    });
+
+    it('serializes alias and displayName', () => {
+      const input = [
+        {alias: 'foo', id: 'bar'},
+        {alias: 'baz', id: 'baz'},
+      ];
+      expect(utils.serializeCompareExperimentParams(input)).toBe(
+        'foo:bar,baz:baz'
+      );
+    });
+
+    it('does not deduplicate or validate', () => {
+      const input = [
+        {alias: 'foo', id: 'bar'},
+        // does not deduplicate
+        {alias: 'tar', id: 'bar'},
+        {alias: 'foo', id: 'bang'},
+        {alias: '', id: ''},
+      ];
+      expect(utils.serializeCompareExperimentParams(input)).toBe(
+        'foo:bar,tar:bar,foo:bang,:'
+      );
+    });
+  });
+
   describe('#getExperimentIdsFromRouteParams', () => {
     it('returns ids from compare route', () => {
       const actual = utils.getExperimentIdsFromRouteParams(

tensorboard/webapp/app_routing/programmatical_navigation_module_test.ts

@@ -101,11 +101,18 @@ describe('programmatical navigation module test', () => {
     function provider2() {
       return {
         actionCreator: testAction,
-        lambda: (action: typeof testAction) => {
+        lambda: (action: typeof testAction): NavigateToCompare => {
           return {
             routeKind: RouteKind.COMPARE_EXPERIMENT,
-            routeParams: {experimentIds: 'foo'},
-          } as NavigateToCompare;
+            routeParams: {
+              aliasAndExperimentIds: [
+                {
+                  alias: 'Foo',
+                  id: 'foo',
+                },
+              ],
+            },
+          };
         },
       };
     }

tensorboard/webapp/app_routing/programmatical_navigation_types.ts

@@ -15,7 +15,7 @@ limitations under the License.
 import {InjectionToken} from '@angular/core';
 import {Action, ActionCreator, Creator} from '@ngrx/store';
 
-import {CompareRouteParams, ExperimentRouteParams, RouteKind} from './types';
+import {ExperimentRouteParams, RouteKind} from './types';
 
 export const NAVIGATION_PROVIDER = new InjectionToken<NavigationLambda[]>(
   '[App Routing] Programmatical Navigation Provider'
@@ -28,7 +28,9 @@ export interface NavigateToExperiment {
 
 export interface NavigateToCompare {
   routeKind: RouteKind.COMPARE_EXPERIMENT;
-  routeParams: CompareRouteParams;
+  routeParams: {
+    aliasAndExperimentIds: Array<{alias: string; id: string}>;
+  };
 }
 
 export interface NavigateToExperiments {