fix(preview-sidebar): fix preview versions behavior (#3203)

* fix(preview-sidebar): fix preview versions behavior

* fix(preview-sidebar): move code to SidebarPanels

* fix(preview-sidebar): update tests; remove types

src/elements/content-sidebar/SidebarPanels.js

@@ -6,7 +6,7 @@
 
 import * as React from 'react';
 import flow from 'lodash/flow';
-import { Redirect, Route, Switch } from 'react-router-dom';
+import { matchPath, Redirect, Route, Switch, type Location } from 'react-router-dom';
 import SidebarUtils from './SidebarUtils';
 import withSidebarAnnotations from './withSidebarAnnotations';
 import { withAnnotatorContext } from '../common/annotator-context';
@@ -47,6 +47,7 @@ type Props = {
     hasSkills: boolean,
     hasVersions: boolean,
     isOpen: boolean,
+    location: Location,
     metadataSidebarProps: MetadataSidebarProps,
     onAnnotationSelect?: Function,
     onVersionChange?: Function,
@@ -87,6 +88,8 @@ const LoadableVersionsSidebar = SidebarUtils.getAsyncSidebarContent(
     MARK_NAME_JS_LOADING_VERSIONS,
 );
 
+const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?';
+
 class SidebarPanels extends React.Component<Props, State> {
     activitySidebar: ElementRefType = React.createRef();
 
@@ -102,6 +105,21 @@ class SidebarPanels extends React.Component<Props, State> {
         this.setState({ isInitialized: true });
     }
 
+    componentDidUpdate(prevProps: Props): void {
+        const { location, onVersionChange } = this.props;
+        const { location: prevLocation } = prevProps;
+
+        // Reset the current version id if the wrapping versions route is no longer active
+        if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) {
+            onVersionChange(null);
+        }
+    }
+
+    getVersionsMatchPath = (location: Location) => {
+        const { pathname } = location;
+        return matchPath(pathname, SIDEBAR_PATH_VERSIONS);
+    };
+
     /**
      * Refreshes the contents of the active sidebar
      * @returns {void}
@@ -250,7 +268,7 @@ class SidebarPanels extends React.Component<Props, State> {
                 )}
                 {hasVersions && (
                     <Route
-                        path="/:sidebar(activity|details)/versions/:versionId?"
+                        path={SIDEBAR_PATH_VERSIONS}
                         render={({ match }) => (
                             <LoadableVersionsSidebar
                                 fileId={fileId}

src/elements/content-sidebar/__tests__/SidebarPanels.test.js

@@ -11,18 +11,23 @@ jest.mock('../SidebarUtils');
 describe('elements/content-sidebar/SidebarPanels', () => {
     const getWrapper = ({ path = '/', ...rest } = {}) =>
         mount(
-            <MemoryRouter initialEntries={[path]} keyLength={0}>
-                <SidebarPanels
-                    file={{ id: '1234' }}
-                    hasActivity
-                    hasDetails
-                    hasMetadata
-                    hasSkills
-                    hasVersions
-                    isOpen
-                    {...rest}
-                />
-            </MemoryRouter>,
+            <SidebarPanels
+                file={{ id: '1234' }}
+                hasActivity
+                hasDetails
+                hasMetadata
+                hasSkills
+                hasVersions
+                isOpen
+                {...rest}
+            />,
+            {
+                wrappingComponent: MemoryRouter,
+                wrappingComponentProps: {
+                    initialEntries: [path],
+                    keyLength: 0,
+                },
+            },
         );
 
     describe('render', () => {
@@ -144,4 +149,38 @@ describe('elements/content-sidebar/SidebarPanels', () => {
             expect(instance.versionsSidebar.current.refresh).toHaveBeenCalledWith();
         });
     });
+
+    describe('componentDidUpdate', () => {
+        const onVersionChange = jest.fn();
+
+        test.each([
+            ['/activity/versions/123', '/activity/versions/456'],
+            ['/activity/versions/123', '/details/versions/456'],
+            ['/activity/versions', '/activity/versions/123'],
+            ['/activity/versions', '/details/versions'],
+        ])('should not reset the current version if the versions route is still active', (prevPathname, pathname) => {
+            const wrapper = getWrapper({ location: { pathname: prevPathname }, onVersionChange });
+            wrapper.setProps({ location: { pathname } });
+            expect(onVersionChange).not.toBeCalled();
+        });
+
+        test.each([true, false])('should not reset the current version if the sidebar is toggled', isOpen => {
+            const wrapper = getWrapper({ isOpen, location: { pathname: '/details/versions/123' }, onVersionChange });
+            wrapper.setProps({ isOpen: !isOpen });
+            expect(onVersionChange).not.toBeCalled();
+        });
+
+        test.each([
+            ['/activity/versions/123', '/metadata'],
+            ['/activity/versions/123', '/activity'],
+            ['/activity/versions', '/metadata'],
+            ['/details/versions/123', '/metadata'],
+            ['/details/versions/123', '/details'],
+            ['/details/versions', '/metadata'],
+        ])('should reset the current version if the versions route is no longer active', (prevPathname, pathname) => {
+            const wrapper = getWrapper({ location: { pathname: prevPathname }, onVersionChange });
+            wrapper.setProps({ location: { pathname } });
+            expect(onVersionChange).toBeCalledWith(null);
+        });
+    });
 });

src/elements/content-sidebar/versions/VersionsSidebarContainer.js

@@ -95,11 +95,6 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
         }
     }
 
-    componentWillUnmount() {
-        // Reset the current version id since the wrapping route is no longer active
-        this.props.onVersionChange(null);
-    }
-
     handleActionDelete = (versionId: string): Promise<void> => {
         this.setState({ isLoading: true });
 

src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js

@@ -58,17 +58,6 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => {
         });
     });
 
-    describe('componentWillUnmount', () => {
-        test('should forward verison id reset to the parent component', () => {
-            const onVersionChange = jest.fn();
-            const wrapper = getWrapper({ onVersionChange });
-
-            wrapper.instance().componentWillUnmount();
-
-            expect(onVersionChange).toBeCalledWith(null);
-        });
-    });
-
     describe('componentDidMount', () => {
         test('should call onLoad after a successful fetchData() call', async () => {
             const onLoad = jest.fn();