Add mechanism to prevent state to be persisted in local storage. (#6267)

For Google-internal versions of TensorBoard, there are some scenarios
where we do not want to read or write user settings to local storage.
This PR adds a mechanism to facilitate turning persistence off, if
desired.

The PR introduces persistent_settings State with one property,
shouldPersistSettings, and one selector, getShouldPersistSettings. In
this codebase the value of this is always true. Google-internal versions
of TensorBoard, however, may have their own reducers that set this
property to false.

The PR also modifies persistent_settings effects to read the value of
shouldPersistSettings before determining whether to read and write to
the persistent data source. The effects now wait until the first
navigating() event before attempting to read from the data source, in
order to give other layers a small amount of time to set the
shouldPersistSettings state before the first read attempt. There's
nothing particularly special about using the navigating() event for
this, other than it is fired late enough for our current use cases. We
could consider using a different, later, action in the future.

Googlers, see how one internal TensorBoard overrides this property in
cl/518939981.

Author: bmd3k

tensorboard/webapp/persistent_settings/BUILD

@@ -12,8 +12,9 @@ tf_ng_module(
         ":config_module",
         "//tensorboard/webapp/persistent_settings/_data_source",
         "//tensorboard/webapp/persistent_settings/_data_source:types",
+        "//tensorboard/webapp/persistent_settings/_redux",
         "//tensorboard/webapp/persistent_settings/_redux:persistent_settings_actions",
-        "//tensorboard/webapp/persistent_settings/_redux:persistent_settings_effects",
+        "//tensorboard/webapp/persistent_settings/_redux:types",
         "@npm//@angular/core",
         "@npm//@ngrx/effects",
         "@npm//@ngrx/store",
@@ -66,6 +67,6 @@ tf_ng_web_test_suite(
     deps = [
         ":config_module_test_lib",
         "//tensorboard/webapp/persistent_settings/_data_source:_data_source_test_lib",
-        "//tensorboard/webapp/persistent_settings/_redux:persistent_settings_effects_test_lib",
+        "//tensorboard/webapp/persistent_settings/_redux:_redux_test_lib",
     ],
 )

tensorboard/webapp/persistent_settings/_redux/BUILD

@@ -1,4 +1,4 @@
-load("//tensorboard/defs:defs.bzl", "tf_ng_module")
+load("//tensorboard/defs:defs.bzl", "tf_ng_module", "tf_ts_library")
 
 package(default_visibility = ["//tensorboard:internal"])
 
@@ -14,12 +14,16 @@ tf_ng_module(
 )
 
 tf_ng_module(
-    name = "persistent_settings_effects",
+    name = "_redux",
     srcs = [
         "persistent_settings_effects.ts",
+        "persistent_settings_reducers.ts",
+        "persistent_settings_selectors.ts",
     ],
     deps = [
         ":persistent_settings_actions",
+        ":types",
+        "//tensorboard/webapp/app_routing/actions",
         "//tensorboard/webapp/persistent_settings:config_module",
         "//tensorboard/webapp/persistent_settings/_data_source",
         "//tensorboard/webapp/persistent_settings/_data_source:types",
@@ -30,17 +34,39 @@ tf_ng_module(
     ],
 )
 
+tf_ts_library(
+    name = "types",
+    srcs = [
+        "persistent_settings_types.ts",
+    ],
+)
+
+tf_ts_library(
+    name = "testing",
+    testonly = True,
+    srcs = [
+        "testing.ts",
+    ],
+    deps = [
+        ":types",
+    ],
+)
+
 tf_ng_module(
-    name = "persistent_settings_effects_test_lib",
+    name = "_redux_test_lib",
     testonly = True,
     srcs = [
         "persistent_settings_effects_test.ts",
+        "persistent_settings_selectors_test.ts",
     ],
     deps = [
+        ":_redux",
         ":persistent_settings_actions",
-        ":persistent_settings_effects",
+        ":testing",
         "//tensorboard/webapp/angular:expect_angular_core_testing",
         "//tensorboard/webapp/angular:expect_ngrx_store_testing",
+        "//tensorboard/webapp/app_routing:testing",
+        "//tensorboard/webapp/app_routing/actions",
         "//tensorboard/webapp/persistent_settings:config_module",
         "//tensorboard/webapp/persistent_settings/_data_source:testing",
         "//tensorboard/webapp/testing:utils",

tensorboard/webapp/persistent_settings/_redux/persistent_settings_effects.ts

@@ -15,23 +15,27 @@ limitations under the License.
 import {Injectable} from '@angular/core';
 import {Actions, createEffect, ofType, OnInitEffects} from '@ngrx/effects';
 import {Action, createAction, Store} from '@ngrx/store';
-import {merge, Observable} from 'rxjs';
+import {EMPTY, merge, Observable} from 'rxjs';
 import {
   buffer,
   debounceTime,
   delay,
   distinctUntilChanged,
+  filter,
   mergeMap,
   share,
   skip,
+  take,
   tap,
+  withLatestFrom,
 } from 'rxjs/operators';
 import {PersistentSettingsConfigModule} from '../persistent_settings_config_module';
 import {PersistentSettingsDataSource} from '../_data_source/persistent_settings_data_source';
 import {PersistableSettings} from '../_data_source/types';
 import {globalSettingsLoaded} from './persistent_settings_actions';
+import {getShouldPersistSettings} from './persistent_settings_selectors';
+import * as appRoutingActions from '../../app_routing/actions';
 
-const initAction = createAction('[Persistent Settings] Effects Init');
 const DEBOUNCE_PERIOD_IN_MS = 500;
 
 /**
@@ -40,12 +44,15 @@ const DEBOUNCE_PERIOD_IN_MS = 500;
  * emit.
  */
 @Injectable()
-export class PersistentSettingsEffects implements OnInitEffects {
+export class PersistentSettingsEffects {
   /** @export */
   readonly initializeAndUpdateSettings$: Observable<void> = createEffect(
     () => {
       const selectorsEmit$ = this.actions$.pipe(
-        ofType(initAction),
+        ofType(appRoutingActions.navigating),
+        take(1),
+        withLatestFrom(this.store.select(getShouldPersistSettings)),
+        filter(([, shouldPersistSettings]) => shouldPersistSettings),
         mergeMap(() => this.dataSource.getSettings()),
         tap((partialSettings) => {
           this.store.dispatch(globalSettingsLoaded({partialSettings}));
@@ -85,14 +92,18 @@ export class PersistentSettingsEffects implements OnInitEffects {
         // Buffers changes from all selectors and only emit when debounce period
         // is over.
         buffer(selectorsEmit$.pipe(debounceTime(DEBOUNCE_PERIOD_IN_MS))),
-        mergeMap((partialSettings) => {
-          const partialSetting: Partial<PersistableSettings> = {};
+        mergeMap((stateSettings) => {
+          if (stateSettings.length === 0) {
+            return EMPTY;
+          }
+
+          const dataSourceSettings: Partial<PersistableSettings> = {};
           // Combine buffered setting changes. Last settings change would
           // overwrite earlier changes.
-          for (const setting of partialSettings) {
-            Object.assign(partialSetting, setting);
+          for (const setting of stateSettings) {
+            Object.assign(dataSourceSettings, setting);
           }
-          return this.dataSource.setSettings(partialSetting);
+          return this.dataSource.setSettings(dataSourceSettings);
         })
       );
     },
@@ -108,11 +119,6 @@ export class PersistentSettingsEffects implements OnInitEffects {
     >,
     private readonly dataSource: PersistentSettingsDataSource<PersistableSettings>
   ) {}
-
-  /** @export */
-  ngrxOnInitEffects(): Action {
-    return initAction();
-  }
 }
 
-export const TEST_ONLY = {initAction, DEBOUNCE_PERIOD_IN_MS};
+export const TEST_ONLY = {DEBOUNCE_PERIOD_IN_MS};

tensorboard/webapp/persistent_settings/_redux/persistent_settings_effects_test.ts

@@ -33,6 +33,9 @@ import {
   PersistentSettingsEffects,
   TEST_ONLY,
 } from './persistent_settings_effects';
+import {getShouldPersistSettings} from './persistent_settings_selectors';
+import * as appRoutingActions from '../../app_routing/actions';
+import {buildRoute} from '../../app_routing/testing';
 
 describe('persistent_settings effects test', () => {
   let action: ReplaySubject<Action>;
@@ -95,13 +98,13 @@ describe('persistent_settings effects test', () => {
     });
 
     describe('on init', () => {
-      it('fetches user settings and emits an aciton', () => {
+      it('fetches user settings and emits an action', () => {
         getSettingsSpy.and.returnValue(
           of({
             ignoreOutliers: false,
           })
         );
-        action.next(TEST_ONLY.initAction());
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
 
         expect(actualActions).toEqual([
           globalSettingsLoaded({
@@ -112,12 +115,47 @@ describe('persistent_settings effects test', () => {
         ]);
       });
 
+      it('does not fetch user settings if it should not', () => {
+        store.overrideSelector(getShouldPersistSettings, false);
+        store.refreshState();
+        getSettingsSpy.and.returnValue(
+          of({
+            ignoreOutliers: false,
+          })
+        );
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
+
+        expect(actualActions).toEqual([]);
+        expect(getSettingsSpy).not.toHaveBeenCalled();
+      });
+
+      it('does not fetch again after first navigating event', () => {
+        getSettingsSpy.and.returnValue(
+          of({
+            ignoreOutliers: false,
+          })
+        );
+
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
+        expect(actualActions).toEqual([
+          globalSettingsLoaded({
+            partialSettings: {
+              ignoreOutliers: false,
+            },
+          }),
+        ]);
+
+        actualActions = [];
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
+        expect(actualActions).toEqual([]);
+      });
+
       it('subscribes to selector changes after initial setting is read', fakeAsync(() => {
         store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.1});
         const getSettingsSubject = new ReplaySubject(1);
 
         getSettingsSpy.and.returnValue(getSettingsSubject);
-        action.next(TEST_ONLY.initAction());
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
 
         tick();
         store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.3});
@@ -140,7 +178,7 @@ describe('persistent_settings effects test', () => {
         store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.1});
 
         getSettingsSpy.and.returnValue(of({}));
-        action.next(TEST_ONLY.initAction());
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
 
         tick(TEST_ONLY.DEBOUNCE_PERIOD_IN_MS * 2);
         expect(setSettingsSpy).not.toHaveBeenCalled();
@@ -151,8 +189,9 @@ describe('persistent_settings effects test', () => {
       function initializeAndSubscribe() {
         store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.1});
         store.overrideSelector(setIgnoreOutliers, {ignoreOutliers: false});
+        store.refreshState();
         getSettingsSpy.and.returnValue(of({}));
-        action.next(TEST_ONLY.initAction());
+        action.next(appRoutingActions.navigating({after: buildRoute()}));
         tick();
       }
 
@@ -208,6 +247,30 @@ describe('persistent_settings effects test', () => {
           ignoreOutliers: true,
         });
       }));
+
+      it('persists settings when it should', fakeAsync(() => {
+        store.overrideSelector(getShouldPersistSettings, true);
+        initializeAndSubscribe();
+
+        store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.3});
+        store.refreshState();
+
+        tick(TEST_ONLY.DEBOUNCE_PERIOD_IN_MS);
+        expect(setSettingsSpy).toHaveBeenCalledOnceWith({
+          scalarSmoothing: 0.3,
+        });
+      }));
+
+      it('does not persist settings when it should not', fakeAsync(() => {
+        store.overrideSelector(getShouldPersistSettings, false);
+        initializeAndSubscribe();
+
+        store.overrideSelector(setSmoothingSelector, {scalarSmoothing: 0.3});
+        store.refreshState();
+
+        tick(TEST_ONLY.DEBOUNCE_PERIOD_IN_MS);
+        expect(setSettingsSpy).not.toHaveBeenCalled();
+      }));
     });
   });
 });

tensorboard/webapp/persistent_settings/_redux/persistent_settings_reducers.ts

@@ -0,0 +1,32 @@
+/* Copyright 2023 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+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 {Action, createReducer} from '@ngrx/store';
+
+import {PersistentSettingsState} from './persistent_settings_types';
+
+const initialState: PersistentSettingsState = {
+  // In the OSS code base this value is always true but Google-internal
+  // extensions may override it in certain circumstances.
+  shouldPersistSettings: true,
+};
+
+const reducer = createReducer(initialState);
+
+export function reducers(
+  state: PersistentSettingsState | undefined,
+  action: Action
+) {
+  return reducer(state, action);
+}

tensorboard/webapp/persistent_settings/_redux/persistent_settings_selectors.ts

@@ -0,0 +1,31 @@
+/* Copyright 2023 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+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 {createFeatureSelector, createSelector} from '@ngrx/store';
+
+import {
+  PERSISTENT_SETTINGS_FEATURE_KEY,
+  PersistentSettingsState,
+} from './persistent_settings_types';
+
+const selectPersistentSettings = createFeatureSelector<PersistentSettingsState>(
+  PERSISTENT_SETTINGS_FEATURE_KEY
+);
+
+export const getShouldPersistSettings = createSelector(
+  selectPersistentSettings,
+  (state: PersistentSettingsState): boolean => {
+    return state.shouldPersistSettings;
+  }
+);