fix(StoreDevTools): out of bounds when actions are filtered (#1532)

Closes #1522

modules/store-devtools/spec/integration.spec.ts

@@ -1,41 +1,96 @@
 import { NgModule } from '@angular/core';
 import { TestBed } from '@angular/core/testing';
-import { StoreModule, Store, ActionsSubject } from '@ngrx/store';
-import { StoreDevtoolsModule, StoreDevtools } from '@ngrx/store-devtools';
+import { StoreModule, Store, Action } from '@ngrx/store';
+import {
+  StoreDevtoolsModule,
+  StoreDevtools,
+  StoreDevtoolsOptions,
+} from '@ngrx/store-devtools';
 
 describe('Devtools Integration', () => {
-  let store: Store<any>;
-
-  @NgModule({
-    imports: [StoreModule.forFeature('a', (state: any, action: any) => state)],
-  })
-  class EagerFeatureModule {}
-
-  @NgModule({
-    imports: [
-      StoreModule.forRoot({}),
-      EagerFeatureModule,
-      StoreDevtoolsModule.instrument(),
-    ],
-  })
-  class RootModule {}
-
-  beforeEach(() => {
+  function setup(options: Partial<StoreDevtoolsOptions> = {}) {
+    @NgModule({
+      imports: [
+        StoreModule.forFeature('a', (state: any, action: any) => state),
+      ],
+    })
+    class EagerFeatureModule {}
+
+    @NgModule({
+      imports: [
+        StoreModule.forRoot({}),
+        EagerFeatureModule,
+        StoreDevtoolsModule.instrument(options),
+      ],
+    })
+    class RootModule {}
+
     TestBed.configureTestingModule({
       imports: [RootModule],
     });
+
+    const store = TestBed.get(Store) as Store<any>;
+    const devtools = TestBed.get(StoreDevtools) as StoreDevtools;
+    return { store, devtools };
+  }
+
+  afterEach(() => {
+    const devtools = TestBed.get(StoreDevtools) as StoreDevtools;
+    devtools.reset();
   });
 
   it('should load the store eagerly', () => {
     let error = false;
 
     try {
-      let store = TestBed.get(Store);
+      const { store } = setup();
       store.subscribe();
     } catch (e) {
       error = true;
     }
 
     expect(error).toBeFalsy();
   });
+
+  it('should not throw if actions are ignored', (done: any) => {
+    const { store, devtools } = setup({
+      predicate: (_, { type }: Action) => type !== 'FOO',
+    });
+    store.subscribe();
+    devtools.dispatcher.subscribe((action: Action) => {
+      if (action.type === 'REFRESH') {
+        done();
+      }
+    });
+    store.dispatch({ type: 'FOO' });
+    devtools.refresh();
+  });
+
+  it('should not throw if actions are blacklisted', (done: any) => {
+    const { store, devtools } = setup({
+      actionsBlacklist: ['FOO'],
+    });
+    store.subscribe();
+    devtools.dispatcher.subscribe((action: Action) => {
+      if (action.type === 'REFRESH') {
+        done();
+      }
+    });
+    store.dispatch({ type: 'FOO' });
+    devtools.refresh();
+  });
+
+  it('should not throw if actions are whitelisted', (done: any) => {
+    const { store, devtools } = setup({
+      actionsWhitelist: ['BAR'],
+    });
+    store.subscribe();
+    devtools.dispatcher.subscribe((action: Action) => {
+      if (action.type === 'REFRESH') {
+        done();
+      }
+    });
+    store.dispatch({ type: 'FOO' });
+    devtools.refresh();
+  });
 });

modules/store-devtools/spec/store.spec.ts

@@ -438,6 +438,125 @@ describe('Store Devtools', () => {
     });
   });
 
+  describe('Filtered actions', () => {
+    it('should respect the predicate option', () => {
+      const fixture = createStore(counter, {
+        predicate: (s, a) => a.type !== 'INCREMENT',
+      });
+
+      expect(fixture.getState()).toBe(0);
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      expect(fixture.getState()).toBe(5);
+
+      // init, decrement, decrement
+      const {
+        stagedActionIds,
+        actionsById,
+        computedStates,
+        currentStateIndex,
+      } = fixture.getLiftedState();
+      expect(stagedActionIds.length).toBe(3);
+      expect(Object.keys(actionsById).length).toBe(3);
+      expect(computedStates.length).toBe(3);
+      expect(currentStateIndex).toBe(2);
+
+      fixture.devtools.jumpToAction(0);
+      expect(fixture.getState()).toBe(1);
+
+      fixture.devtools.jumpToAction(1);
+      expect(fixture.getState()).toBe(6);
+
+      fixture.devtools.jumpToAction(2);
+      expect(fixture.getState()).toBe(5);
+    });
+
+    it('should respect the blacklist option', () => {
+      const fixture = createStore(counter, {
+        actionsBlacklist: ['INCREMENT'],
+      });
+
+      expect(fixture.getState()).toBe(0);
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      expect(fixture.getState()).toBe(5);
+
+      // init, decrement, decrement
+      const {
+        stagedActionIds,
+        actionsById,
+        computedStates,
+        currentStateIndex,
+      } = fixture.getLiftedState();
+      expect(stagedActionIds.length).toBe(3);
+      expect(Object.keys(actionsById).length).toBe(3);
+      expect(computedStates.length).toBe(3);
+      expect(currentStateIndex).toBe(2);
+
+      fixture.devtools.jumpToAction(0);
+      expect(fixture.getState()).toBe(1);
+
+      fixture.devtools.jumpToAction(1);
+      expect(fixture.getState()).toBe(6);
+
+      fixture.devtools.jumpToAction(2);
+      expect(fixture.getState()).toBe(5);
+    });
+
+    it('should respect the whitelist option', () => {
+      const fixture = createStore(counter, {
+        actionsWhitelist: ['DECREMENT'],
+      });
+
+      expect(fixture.getState()).toBe(0);
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'INCREMENT' });
+      fixture.store.dispatch({ type: 'DECREMENT' });
+      expect(fixture.getState()).toBe(5);
+
+      // init, decrement, decrement
+      const {
+        stagedActionIds,
+        actionsById,
+        computedStates,
+        currentStateIndex,
+      } = fixture.getLiftedState();
+      expect(stagedActionIds.length).toBe(3);
+      expect(Object.keys(actionsById).length).toBe(3);
+      expect(computedStates.length).toBe(3);
+      expect(currentStateIndex).toBe(2);
+
+      fixture.devtools.jumpToAction(0);
+      expect(fixture.getState()).toBe(1);
+
+      fixture.devtools.jumpToAction(1);
+      expect(fixture.getState()).toBe(6);
+
+      fixture.devtools.jumpToAction(2);
+      expect(fixture.getState()).toBe(5);
+    });
+  });
+
   describe('maxAge option', () => {
     it('should auto-commit earliest non-@@INIT action when maxAge is reached', () => {
       const fixture = createStore(counter, { maxAge: 3 });

modules/store-devtools/src/reducer.ts

@@ -7,7 +7,7 @@ import {
   UPDATE,
   INIT,
 } from '@ngrx/store';
-import { difference, liftAction } from './utils';
+import { difference, liftAction, isActionFiltered } from './utils';
 import * as DevtoolsActions from './actions';
 import { StoreDevtoolsConfig, StateSanitizer } from './config';
 import { PerformAction } from './actions';
@@ -362,8 +362,18 @@ export function liftReducerWith(
           return liftedState || initialLiftedState;
         }
 
-        if (isPaused) {
-          // If recording is paused, overwrite the last state
+        if (
+          isPaused ||
+          (liftedState &&
+            isActionFiltered(
+              liftedState.computedStates[currentStateIndex],
+              liftedAction,
+              options.predicate,
+              options.actionsWhitelist,
+              options.actionsBlacklist
+            ))
+        ) {
+          // If recording is paused or if the action should be ignored, overwrite the last state
           // (corresponds to the pause action) and keep everything else as is.
           // This way, the app gets the new current state while the devtools
           // do not record another action.

modules/store-devtools/src/utils.ts

@@ -24,8 +24,17 @@ export function difference(first: any[], second: any[]) {
  */
 export function unliftState(liftedState: LiftedState) {
   const { computedStates, currentStateIndex } = liftedState;
-  const { state } = computedStates[currentStateIndex];
 
+  // At start up NgRx dispatches init actions,
+  // When these init actions are being filtered out by the predicate or black/white list options
+  // we don't have a complete computed states yet.
+  // At this point it could happen that we're out of bounds, when this happens we fall back to the last known state
+  if (currentStateIndex >= computedStates.length) {
+    const { state } = computedStates[computedStates.length - 1];
+    return state;
+  }
+
+  const { state } = computedStates[currentStateIndex];
   return state;
 }
 
@@ -155,9 +164,10 @@ export function isActionFiltered(
   whitelist?: string[],
   blacklist?: string[]
 ) {
-  return (
-    (predicate && !predicate(state, action.action)) ||
-    (whitelist && !action.action.type.match(whitelist.join('|'))) ||
-    (blacklist && action.action.type.match(blacklist.join('|')))
-  );
+  const predicateMatch = predicate && !predicate(state, action.action);
+  const whitelistMatch =
+    whitelist && !action.action.type.match(whitelist.join('|'));
+  const blacklistMatch =
+    blacklist && action.action.type.match(blacklist.join('|'));
+  return predicateMatch || whitelistMatch || blacklistMatch;
 }