[Canvas] Fixes issue with undo/redo

x-pack/plugins/canvas/public/components/workpad_header/view_menu/view_menu.component.tsx

@@ -85,10 +85,6 @@ export interface Props {
    * Current autoplay interval
    */
   autoplayInterval: number;
-  /**
-   * Enables autoplay
-   */
-  enableAutoplay: (autoplay: boolean) => void;
   /**
    * Sets autoplay interval
    */
@@ -110,7 +106,6 @@ export const ViewMenu: FunctionComponent<Props> = ({
   setRefreshInterval,
   autoplayEnabled,
   autoplayInterval,
-  enableAutoplay,
   setAutoplayInterval,
 }) => {
   const setRefresh = (val: number) => setRefreshInterval(val);
@@ -259,6 +254,5 @@ ViewMenu.propTypes = {
   setRefreshInterval: PropTypes.func.isRequired,
   autoplayEnabled: PropTypes.bool.isRequired,
   autoplayInterval: PropTypes.number.isRequired,
-  enableAutoplay: PropTypes.func.isRequired,
   setAutoplayInterval: PropTypes.func.isRequired,
 };

x-pack/plugins/canvas/public/routes/workpad/hooks/use_workpad_history.test.ts

@@ -26,76 +26,234 @@ describe('useRestoreHistory', () => {
     jest.resetAllMocks();
   });
 
-  test('replaces undefined state with current state', () => {
-    const history = {
-      location: {
-        state: undefined,
-        pathname: 'somepath',
-      },
-      push: jest.fn(),
-      replace: jest.fn(),
-    };
-
-    const state = {
-      persistent: { some: 'state' },
-    };
-
-    mockGetState.mockReturnValue(state);
-    mockGetHistory.mockReturnValue(history);
-
-    renderHook(() => useWorkpadHistory());
-
-    expect(history.replace).toBeCalledWith(history.location.pathname, encode(state.persistent));
+  describe('initial run', () => {
+    test('with undefined location state ', () => {
+      const history = {
+        location: {
+          state: undefined,
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
+
+      const state = {
+        persistent: { some: 'state' },
+      };
+
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      renderHook(() => useWorkpadHistory());
+
+      expect(history.replace).toBeCalledWith(history.location.pathname, encode(state.persistent));
+      expect(history.push).not.toBeCalled();
+    });
+
+    test('with location state not matching store state', () => {
+      const history = {
+        location: {
+          state: encode({ prior: 'state' }),
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
+
+      const state = {
+        persistent: { some: 'state' },
+      };
+
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      renderHook(() => useWorkpadHistory());
+
+      expect(history.push).not.toBeCalled();
+      expect(history.replace).not.toBeCalled();
+    });
+
+    test('with location state matching store state', () => {
+      const state = { some: 'state' };
+      const history = {
+        location: {
+          state: encode(state),
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      renderHook(() => useWorkpadHistory());
+
+      expect(history.push).not.toBeCalled();
+      expect(history.replace).not.toBeCalled();
+    });
   });
 
-  test('does not do a push on initial render if states do not match', () => {
-    const history = {
-      location: {
-        state: encode({ old: 'state' }),
-        pathname: 'somepath',
-      },
-      push: jest.fn(),
-      replace: jest.fn(),
-    };
+  describe('state changes', () => {
+    it('does a replace if location state is undefined', () => {
+      const push = jest.fn();
+      const replace = jest.fn();
+
+      const history = {
+        location: {
+          state: encode({ old: 'state' }),
+          pathname: 'somepath',
+          search: '',
+        },
+        push,
+        replace,
+      };
+
+      const state = {
+        persistent: { some: 'state' },
+      };
+
+      const newState = {
+        persistent: { new: 'state' },
+      };
+
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      const { rerender } = renderHook(() => useWorkpadHistory());
+
+      mockGetState.mockReturnValue(newState);
+      // History object from react router will not change, so just modifying here
+      history.location.state = undefined;
+      history.location.pathname = 'newpath';
+      rerender();
 
-    const state = {
-      persistent: { some: 'state' },
-    };
+      expect(history.replace).toBeCalledWith('newpath', encode(newState.persistent));
+    });
 
-    mockGetState.mockReturnValue(state);
-    mockGetHistory.mockReturnValue(history);
+    test('does a push if location state does not match store state', () => {
+      const history = {
+        location: {
+          state: encode({ old: 'state' }),
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
 
-    renderHook(() => useWorkpadHistory());
+      const oldState = {
+        persistent: { some: 'state' },
+      };
 
-    expect(history.push).not.toBeCalled();
+      const newState = {
+        persistent: { new: 'state' },
+      };
+
+      mockGetState.mockReturnValue(oldState);
+      mockGetHistory.mockReturnValue(history);
+
+      const { rerender } = renderHook(() => useWorkpadHistory());
+
+      mockGetState.mockReturnValue(newState);
+      rerender();
+
+      expect(history.push).toBeCalledWith(history.location.pathname, encode(newState.persistent));
+    });
+
+    test('does nothing if new state matches location state', () => {
+      const state = {
+        persistent: { some: 'state' },
+      };
+
+      const newState = { ...state };
+
+      const history = {
+        location: {
+          state: encode(state.persistent),
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
+
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      const { rerender } = renderHook(() => useWorkpadHistory());
+
+      mockGetState.mockReturnValue(newState);
+      rerender();
+
+      expect(history.push).not.toBeCalled();
+      expect(history.replace).not.toBeCalled();
+    });
   });
 
-  test('rerender does a push if location state does not match store state', () => {
-    const history = {
-      location: {
-        state: encode({ old: 'state' }),
-        pathname: 'somepath',
-      },
-      push: jest.fn(),
-      replace: jest.fn(),
-    };
+  describe('changes to location', () => {
+    test('changes to pathname have no effect', () => {
+      // This is equivalent of navigating to a new page.
+      // The location state will initially be undefined, but
+      // we don't want to take any action because it will cause a state change
+      // and that will be picked up and do the replace
+      const state = {
+        persistent: { some: 'state' },
+      };
+
+      const history = {
+        location: {
+          state: encode(state.persistent),
+          pathname: 'somepath',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
+
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
+
+      const { rerender } = renderHook(() => useWorkpadHistory());
+
+      history.location.state = undefined;
+      history.location.pathname = 'newpath';
+
+      rerender();
+
+      expect(history.push).not.toBeCalled();
+      expect(history.replace).not.toBeCalled();
+    });
 
-    const oldState = {
-      persistent: { some: 'state' },
-    };
+    test('changes to search does a replace', () => {
+      // This is equivalent of going from full screen to not full screen
+      // There is no state change that will occur, but we still need to update
+      // the location state
+      const state = {
+        persistent: { some: 'state' },
+      };
 
-    const newState = {
-      persistent: { new: 'state' },
-    };
+      const history = {
+        location: {
+          state: encode(state.persistent),
+          pathname: 'somepath',
+          search: '',
+        },
+        push: jest.fn(),
+        replace: jest.fn(),
+      };
 
-    mockGetState.mockReturnValue(oldState);
-    mockGetHistory.mockReturnValue(history);
+      mockGetState.mockReturnValue(state);
+      mockGetHistory.mockReturnValue(history);
 
-    const { rerender } = renderHook(() => useWorkpadHistory());
+      const { rerender } = renderHook(() => useWorkpadHistory());
+      history.location.pathname = 'somepath';
+      history.location.search = 'newsearch';
+      history.location.state = undefined;
 
-    mockGetState.mockReturnValue(newState);
-    rerender();
+      rerender();
 
-    expect(history.push).toBeCalledWith(history.location.pathname, encode(newState.persistent));
+      expect(history.push).not.toBeCalled();
+      expect(history.replace).toBeCalledWith(
+        `somepath?${history.location.search}`,
+        encode(state.persistent)
+      );
+    });
   });
 });

x-pack/plugins/canvas/public/routes/workpad/hooks/use_workpad_history.ts

@@ -31,11 +31,10 @@ export const useWorkpadHistory = () => {
     // This will happen when navigating directly to a url (there will be no state on that link click)
     if (locationState === undefined) {
       history.replace(fullPath, encode(historyState));
-    } else if (!doesStateMatchLocationState && !isInitialRun) {
+    } else if (!isInitialRun && !doesStateMatchLocationState) {
       // There was a state change here
-
       // If the state of the route that we are on does not match this new state, then we are going to push
       history.push(fullPath, encode(historyState));
     }
-  }, [history, historyState]);
+  }, [history, historyState, history.location.search]);
 };