Incorporate feedback from code review.

kbase · Sep 8, 2023 · cd655a2 · cd655a2
1 parent c9e290c
commit cd655a2
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 50 deletions.
diff --git a/src/common/api/authService.ts b/src/common/api/authService.ts
@@ -1,7 +1,8 @@
+// import { store } from '../../app/store';
+import { Me } from '../types/auth';
+import { uriEncodeTemplateTag as encode } from '../utils/stringUtils';
 import { baseApi } from './index';
 import { httpService } from './utils/serviceHelpers';
-import { uriEncodeTemplateTag as encode } from '../utils/stringUtils';
-import { Me } from '../types/auth';
 
 const authService = httpService({
   url: '/services/auth',
@@ -51,14 +52,24 @@ export const authApi = baseApi.injectEndpoints({
         }),
     }),
     getMe: builder.query<AuthResults['getMe'], AuthParams['getMe']>({
-      query: ({ token }) =>
-        authService({
+      query: ({ token }) => {
+        /* I want to do
+        const token = store.getState().auth.token;
+        but authSlice imports revokeToken defined here,
+        so this becomes a circular depenency.
+        Specifically the error is:
+          7022: 'token' implicitly has type 'any' because it does not have a
+          type annotation and is referenced directly or indirectly in its own
+          initializer.
+        */
+        return authService({
           headers: {
             Authorization: token,
           },
           method: 'GET',
           url: '/api/V2/me',
-        }),
+        });
+      },
     }),
     getUsers: builder.query<AuthResults['getUsers'], AuthParams['getUsers']>({
       query: ({ token, users }) =>

diff --git a/src/features/navigator/NarrativeControl/Delete.test.tsx b/src/features/navigator/NarrativeControl/Delete.test.tsx
@@ -1,12 +1,9 @@
 /* Delete.test */
-import { FC } from 'react';
 import { act, render, screen } from '@testing-library/react';
 import fetchMock, {
-  //MockParams,
   disableFetchMocks,
   enableFetchMocks,
 } from 'jest-fetch-mock';
-import { ErrorBoundary, FallbackProps } from 'react-error-boundary';
 import { noOp } from '../../common';
 import { testNarrativeDoc, testNarrativeDocFactory } from '../fixtures';
 import { DeleteTemplate } from './NarrativeControl.stories';
@@ -40,21 +37,11 @@ const testResponseErrorFactory = ({
   { status: 500 },
 ];
 
-const TestingError: FC<FallbackProps> = ({ error }) => {
-  return <>Error: {JSON.stringify(error)}</>;
-};
-
 const consoleError = jest.spyOn(console, 'error');
 // This mockImplementation supresses console.error calls.
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 consoleError.mockImplementation(() => {});
 
-const logError = (error: Error, info: { componentStack: string }) => {
-  console.log({ error }); // eslint-disable-line no-console
-  console.log(info.componentStack); // eslint-disable-line no-console
-  screen.debug();
-};
-
 describe('The <Delete /> component...', () => {
   beforeAll(() => {
     enableFetchMocks();
@@ -122,12 +109,7 @@ describe('The <Delete /> component...', () => {
   test('throws an error if the delete request fails.', async () => {
     fetchMock.mockRejectedValue(null);
     const { container } = render(
-      <ErrorBoundary FallbackComponent={TestingError} onError={logError}>
-        <DeleteTemplate
-          modalClose={noOp}
-          narrativeDoc={testNarrativeDocError}
-        />
-      </ErrorBoundary>
+      <DeleteTemplate modalClose={noOp} narrativeDoc={testNarrativeDocError} />
     );
     expect(container).toBeTruthy();
     const buttonDelete = container.querySelector('button');

diff --git a/src/features/navigator/NarrativeControl/Delete.tsx b/src/features/navigator/NarrativeControl/Delete.tsx
@@ -5,6 +5,7 @@ import toast from 'react-hot-toast';
 import { Button } from '../../../common/components';
 import { useAppDispatch, useAppSelector } from '../../../common/hooks';
 import { isKBaseBaseQueryError } from '../../../common/api/utils/kbaseBaseQuery';
+import { parseError } from '../../../common/api/utils/parseError';
 import { deleteWorkspace } from '../../../common/api/workspaceApi';
 import {
   generatePathWithSearchParams,
@@ -13,6 +14,13 @@ import {
 import { deleteNarrative, loading, setLoading } from '../navigatorSlice';
 import { ControlProps } from './common';
 
+const ErrorMessage: FC<{ err: unknown }> = ({ err }) => (
+  <>
+    <span>There was an error! Guru meditation:</span>
+    <span>{JSON.stringify(err)}</span>
+  </>
+);
+
 export const Delete: FC<ControlProps> = ({ narrativeDoc, modalClose }) => {
   const dispatch = useAppDispatch();
   const loadState = useAppSelector(loading);
@@ -38,13 +46,10 @@ export const Delete: FC<ControlProps> = ({ narrativeDoc, modalClose }) => {
     } catch (err) {
       if (!isKBaseBaseQueryError(err)) {
         console.error({ err }); // eslint-disable-line no-console
-        toast(
-          <>
-            <span>There was an error! Guru meditation:</span>
-            <span>{JSON.stringify(err)}</span>
-          </>
-        );
+        toast(ErrorMessage({ err }));
+        return;
       }
+      toast(ErrorMessage({ err: parseError(err) }));
       dispatch(setLoading(false));
       return;
     }

diff --git a/src/features/navigator/NarrativeControl/index.tsx b/src/features/navigator/NarrativeControl/index.tsx
@@ -1,5 +1,5 @@
 /* NarrativeControl */
-import { FC, useState } from 'react';
+import { FC, useMemo, useState } from 'react';
 import { useParams } from 'react-router-dom';
 import { Dropdown } from '../../../common/components';
 import { NarrativeDoc } from '../../../common/types/NarrativeDoc';
@@ -30,25 +30,27 @@ interface ControlLatestProps {
 const ControlLatest: FC<ControlLatestProps> = ({ narrativeDoc }) => {
   const [modalView, setModalView] = useState('Copy this Narrative');
   const modal = useModalControls();
-  const modalClose = () => modal?.close();
-  const { version } = narrativeDoc;
-  const controlLatestDialogs: Record<string, JSX.Element> = {
-    'Copy this Narrative': (
-      <Copy
-        narrativeDoc={narrativeDoc}
-        modalClose={modalClose}
-        version={version}
-      />
-    ),
-    Delete: <Delete narrativeDoc={narrativeDoc} modalClose={modalClose} />,
-    'Link to Organization': (
-      <LinkOrg narrativeDoc={narrativeDoc} modalClose={modalClose} />
-    ),
-    'Manage Sharing': (
-      <Share narrativeDoc={narrativeDoc} modalClose={modalClose} />
-    ),
-    Rename: <Rename narrativeDoc={narrativeDoc} modalClose={modalClose} />,
-  };
+  const controlLatestDialogs: Record<string, JSX.Element> = useMemo(() => {
+    const modalClose = () => modal?.close();
+    const { version } = narrativeDoc;
+    return {
+      'Copy this Narrative': (
+        <Copy
+          narrativeDoc={narrativeDoc}
+          modalClose={modalClose}
+          version={version}
+        />
+      ),
+      Delete: <Delete narrativeDoc={narrativeDoc} modalClose={modalClose} />,
+      'Link to Organization': (
+        <LinkOrg narrativeDoc={narrativeDoc} modalClose={modalClose} />
+      ),
+      'Manage Sharing': (
+        <Share narrativeDoc={narrativeDoc} modalClose={modalClose} />
+      ),
+      Rename: <Rename narrativeDoc={narrativeDoc} modalClose={modalClose} />,
+    };
+  }, [modal, narrativeDoc]);
   return (
     <>
       <Dropdown