fix: change InplaceTextEditor style and add optimistic update (#1953) (#2014)

* Optimistic update for renaming Components, Collections and Containers
* Change the InplaceTextEditor to show the new text until the onSave promise resolves
* Change the InplaceTextEditor style to: Always show the rename button

src/generic/inplace-text-editor/InplaceTextEditor.test.tsx

@@ -1,6 +1,11 @@
 import React from 'react';
 import { IntlProvider } from '@edx/frontend-platform/i18n';
-import { fireEvent, render as baseRender, screen } from '@testing-library/react';
+import {
+  act,
+  fireEvent,
+  render as baseRender,
+  screen,
+} from '@testing-library/react';
 import { InplaceTextEditor } from '.';
 
 const mockOnSave = jest.fn();
@@ -24,8 +29,8 @@ describe('<InplaceTextEditor />', () => {
     expect(screen.queryByRole('button', { name: /edit/ })).not.toBeInTheDocument();
   });
 
-  it('should render the edit button if alwaysShowEditButton is true', () => {
-    render(<InplaceTextEditor text="Test text" onSave={mockOnSave} alwaysShowEditButton />);
+  it('should render the edit button', () => {
+    render(<InplaceTextEditor text="Test text" onSave={mockOnSave} />);
 
     expect(screen.getByText('Test text')).toBeInTheDocument();
     expect(screen.getByRole('button', { name: /edit/i })).toBeInTheDocument();
@@ -36,7 +41,10 @@ describe('<InplaceTextEditor />', () => {
 
     const title = screen.getByText('Test text');
     expect(title).toBeInTheDocument();
-    fireEvent.click(title);
+
+    const editButton = screen.getByRole('button', { name: /edit/i });
+    expect(editButton).toBeInTheDocument();
+    fireEvent.click(editButton);
 
     const textBox = screen.getByRole('textbox');
 
@@ -52,7 +60,10 @@ describe('<InplaceTextEditor />', () => {
 
     const title = screen.getByText('Test text');
     expect(title).toBeInTheDocument();
-    fireEvent.click(title);
+
+    const editButton = screen.getByRole('button', { name: /edit/i });
+    expect(editButton).toBeInTheDocument();
+    fireEvent.click(editButton);
 
     const textBox = screen.getByRole('textbox');
 
@@ -62,4 +73,41 @@ describe('<InplaceTextEditor />', () => {
     expect(textBox).not.toBeInTheDocument();
     expect(mockOnSave).not.toHaveBeenCalled();
   });
+
+  it('should show the new text while processing and roolback in case of error', async () => {
+    let rejecter: (err: Error) => void;
+    const longMockOnSave = jest.fn().mockReturnValue(
+      new Promise<void>((_resolve, reject) => {
+        rejecter = reject;
+      }),
+    );
+    render(<InplaceTextEditor text="Test text" onSave={longMockOnSave} />);
+
+    const text = screen.getByText('Test text');
+    expect(text).toBeInTheDocument();
+
+    const editButton = screen.getByRole('button', { name: /edit/i });
+    expect(editButton).toBeInTheDocument();
+    fireEvent.click(editButton);
+
+    const textBox = screen.getByRole('textbox');
+
+    fireEvent.change(textBox, { target: { value: 'New text' } });
+    fireEvent.keyDown(textBox, { key: 'Enter', code: 'Enter', charCode: 13 });
+
+    expect(textBox).not.toBeInTheDocument();
+    expect(longMockOnSave).toHaveBeenCalledWith('New text');
+
+    // Show pending new text
+    const newText = screen.getByText('New text');
+    expect(newText).toBeInTheDocument();
+
+    await act(async () => { rejecter(new Error('error')); });
+
+    // Remove pending new text on error
+    expect(newText).not.toBeInTheDocument();
+
+    // Show original text
+    expect(screen.getByText('Test text')).toBeInTheDocument();
+  });
 });

src/generic/inplace-text-editor/index.tsx

@@ -1,14 +1,11 @@
 import React, {
   useCallback,
-  useEffect,
   useState,
-  forwardRef,
 } from 'react';
 import {
   Form,
   Icon,
   IconButton,
-  OverlayTrigger,
   Stack,
 } from '@openedx/paragon';
 import { Edit } from '@openedx/paragon/icons';
@@ -16,33 +13,11 @@ import { useIntl } from '@edx/frontend-platform/i18n';
 
 import messages from './messages';
 
-interface IconWrapperProps {
-  popper: any;
-  children: React.ReactNode;
-  [key: string]: any;
-}
-
-const IconWrapper = forwardRef<HTMLDivElement, IconWrapperProps>(({ popper, children, ...props }, ref) => {
-  useEffect(() => {
-    // This is a workaround to force the popper to update its position when
-    // the editor is opened.
-    // Ref: https://react-bootstrap.netlify.app/docs/components/overlays/#updating-position-dynamically
-    popper.scheduleUpdate();
-  }, [popper, children]);
-
-  return (
-    <div ref={ref} {...props}>
-      {children}
-    </div>
-  );
-});
-
 interface InplaceTextEditorProps {
   text: string;
-  onSave: (newText: string) => void;
+  onSave: (newText: string) => Promise<void>;
   readOnly?: boolean;
   textClassName?: string;
-  alwaysShowEditButton?: boolean;
 }
 
 export const InplaceTextEditor: React.FC<InplaceTextEditorProps> = ({
@@ -50,18 +25,29 @@ export const InplaceTextEditor: React.FC<InplaceTextEditorProps> = ({
   onSave,
   readOnly = false,
   textClassName,
-  alwaysShowEditButton = false,
 }) => {
   const intl = useIntl();
   const [inputIsActive, setIsActive] = useState(false);
+  const [pendingSaveText, setPendingSaveText] = useState<string>(); // state with the new text while updating
 
   const handleOnChangeText = useCallback(
-    (event) => {
-      const newText = event.target.value;
-      if (newText && newText !== text) {
-        onSave(newText);
-      }
+    async (event: React.ChangeEvent<HTMLInputElement> | React.KeyboardEvent<HTMLInputElement>) => {
+      const inputText = event.currentTarget.value;
       setIsActive(false);
+      if (inputText && inputText !== text) {
+        // NOTE: While using react query for optimistic updates would be the best approach,
+        // it could not be possible in some cases. For that reason, we use the `pendingSaveText` state
+        // to show the new text while saving.
+        setPendingSaveText(inputText);
+        try {
+          await onSave(inputText);
+        } catch {
+          // don't propagate the exception
+        } finally {
+          // reset the pending save text
+          setPendingSaveText(undefined);
+        }
+      }
     },
     [text],
   );
@@ -78,86 +64,44 @@ export const InplaceTextEditor: React.FC<InplaceTextEditorProps> = ({
     }
   };
 
-  if (readOnly) {
+  // If we have the `pendingSaveText` state it means that we are in the process of saving the new text.
+  // In that case, we show the new text instead of the original in read-only mode as an optimistic update.
+  if (readOnly || pendingSaveText) {
     return (
       <span className={textClassName}>
-        {text}
+        {pendingSaveText || text}
       </span>
     );
   }
 
-  if (alwaysShowEditButton) {
-    return (
-      <Stack
-        direction="horizontal"
-        gap={1}
-      >
-        {inputIsActive
-          ? (
-            <Form.Control
-              autoFocus
-              type="text"
-              aria-label="Text input"
-              defaultValue={text}
-              onBlur={handleOnChangeText}
-              onKeyDown={handleOnKeyDown}
-            />
-          )
-          : (
-            <span className={textClassName}>
-              {text}
-            </span>
-          )}
-        <IconButton
-          src={Edit}
-          iconAs={Icon}
-          alt={intl.formatMessage(messages.editTextButtonAlt)}
-          onClick={handleEdit}
-          size="inline"
-        />
-      </Stack>
-    );
-  }
-
   return (
-    <OverlayTrigger
-      trigger={['hover', 'focus']}
-      placement="right"
-      overlay={(
-        <IconWrapper>
-          <Icon
-            id="edit-text-icon"
-            src={Edit}
-            className="ml-1.5"
-            onClick={handleEdit}
-          />
-        </IconWrapper>
-      )}
+    <Stack
+      direction="horizontal"
+      gap={1}
     >
-      <div>
-        {inputIsActive
-          ? (
-            <Form.Control
-              autoFocus
-              type="text"
-              aria-label="Text input"
-              defaultValue={text}
-              onBlur={handleOnChangeText}
-              onKeyDown={handleOnKeyDown}
-            />
-          )
-          : (
-            <span
-              onClick={handleEdit}
-              onKeyDown={handleEdit}
-              className={textClassName}
-              role="button"
-              tabIndex={0}
-            >
-              {text}
-            </span>
-          )}
-      </div>
-    </OverlayTrigger>
+      {inputIsActive
+        ? (
+          <Form.Control
+            autoFocus
+            type="text"
+            aria-label="Text input"
+            defaultValue={text}
+            onBlur={handleOnChangeText}
+            onKeyDown={handleOnKeyDown}
+          />
+        )
+        : (
+          <span className={textClassName}>
+            {text}
+          </span>
+        )}
+      <IconButton
+        src={Edit}
+        iconAs={Icon}
+        alt={intl.formatMessage(messages.editTextButtonAlt)}
+        onClick={handleEdit}
+        size="inline"
+      />
+    </Stack>
   );
 };

src/library-authoring/collections/CollectionInfoHeader.tsx

@@ -26,14 +26,16 @@ const CollectionInfoHeader = () => {
   const updateMutation = useUpdateCollection(libraryId, collectionId);
   const { showToast } = useContext(ToastContext);
 
-  const handleSaveTitle = (newTitle: string) => {
-    updateMutation.mutateAsync({
-      title: newTitle,
-    }).then(() => {
+  const handleSaveTitle = async (newTitle: string) => {
+    try {
+      await updateMutation.mutateAsync({
+        title: newTitle,
+      });
       showToast(intl.formatMessage(messages.updateCollectionSuccessMsg));
-    }).catch(() => {
+    } catch (err) {
       showToast(intl.formatMessage(messages.updateCollectionErrorMsg));
-    });
+      throw err;
+    }
   };
 
   if (!collection) {
@@ -46,7 +48,6 @@ const CollectionInfoHeader = () => {
       text={collection.title}
       readOnly={readOnly}
       textClassName="font-weight-bold m-1.5"
-      alwaysShowEditButton
     />
   );
 };

src/library-authoring/component-info/ComponentInfoHeader.tsx

@@ -26,16 +26,18 @@ const ComponentInfoHeader = () => {
   const updateMutation = useUpdateXBlockFields(usageKey);
   const { showToast } = useContext(ToastContext);
 
-  const handleSaveDisplayName = (newDisplayName: string) => {
-    updateMutation.mutateAsync({
-      metadata: {
-        display_name: newDisplayName,
-      },
-    }).then(() => {
+  const handleSaveDisplayName = async (newDisplayName: string) => {
+    try {
+      await updateMutation.mutateAsync({
+        metadata: {
+          display_name: newDisplayName,
+        },
+      });
       showToast(intl.formatMessage(messages.updateComponentSuccessMsg));
-    }).catch(() => {
+    } catch (err) {
       showToast(intl.formatMessage(messages.updateComponentErrorMsg));
-    });
+      throw err;
+    }
   };
 
   if (!xblockFields) {
@@ -48,7 +50,6 @@ const ComponentInfoHeader = () => {
       text={xblockFields?.displayName}
       readOnly={readOnly}
       textClassName="font-weight-bold m-1.5"
-      alwaysShowEditButton
     />
   );
 };

src/library-authoring/containers/ContainerInfoHeader.tsx

@@ -25,14 +25,16 @@ const ContainerInfoHeader = () => {
   const updateMutation = useUpdateContainer(containerId);
   const { showToast } = useContext(ToastContext);
 
-  const handleSaveDisplayName = (newDisplayName: string) => {
-    updateMutation.mutateAsync({
-      displayName: newDisplayName,
-    }).then(() => {
+  const handleSaveDisplayName = async (newDisplayName: string) => {
+    try {
+      await updateMutation.mutateAsync({
+        displayName: newDisplayName,
+      });
       showToast(intl.formatMessage(messages.updateContainerSuccessMsg));
-    }).catch(() => {
+    } catch (err) {
       showToast(intl.formatMessage(messages.updateContainerErrorMsg));
-    });
+      throw err;
+    }
   };
 
   if (!container) {
@@ -45,7 +47,6 @@ const ContainerInfoHeader = () => {
       text={container.displayName}
       readOnly={readOnly}
       textClassName="font-weight-bold m-1.5"
-      alwaysShowEditButton
     />
   );
 };

src/library-authoring/containers/UnitInfo.test.tsx

@@ -106,6 +106,6 @@ describe('<UnitInfo />', () => {
   it('show only published content', async () => {
     render(true);
     expect(await screen.findByTestId('unit-info-menu-toggle')).toBeInTheDocument();
-    expect(screen.getByRole('button', { name: /text block published 1/i })).toBeInTheDocument();
+    expect(screen.getByText(/text block published 1/i)).toBeInTheDocument();
   });
 });

src/library-authoring/data/apiHooks.ts

@@ -472,15 +472,28 @@ export const useCollection = (libraryId: string, collectionId: string) => (
  */
 export const useUpdateCollection = (libraryId: string, collectionId: string) => {
   const queryClient = useQueryClient();
+  const collectionQueryKey = libraryAuthoringQueryKeys.collection(libraryId, collectionId);
   return useMutation({
     mutationFn: (data: api.UpdateCollectionComponentsRequest) => (
       api.updateCollectionMetadata(libraryId, collectionId, data)
     ),
+    onMutate: (data) => {
+      const previousData = queryClient.getQueryData(collectionQueryKey) as api.CollectionMetadata;
+      queryClient.setQueryData(collectionQueryKey, {
+        ...previousData,
+        ...data,
+      });
+
+      return { previousData };
+    },
+    onError: (_err, _data, context) => {
+      queryClient.setQueryData(collectionQueryKey, context?.previousData);
+    },
     onSettled: () => {
       // NOTE: We invalidate the library query here because we need to update the library's
       // collection list.
       queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, libraryId) });
-      queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.collection(libraryId, collectionId) });
+      queryClient.invalidateQueries({ queryKey: collectionQueryKey });
     },
   });
 };
@@ -598,13 +611,26 @@ export const useContainer = (containerId?: string) => (
 export const useUpdateContainer = (containerId: string) => {
   const libraryId = getLibraryId(containerId);
   const queryClient = useQueryClient();
+  const containerQueryKey = libraryAuthoringQueryKeys.container(containerId);
   return useMutation({
     mutationFn: (data: api.UpdateContainerDataRequest) => api.updateContainerMetadata(containerId, data),
+    onMutate: (data) => {
+      const previousData = queryClient.getQueryData(containerQueryKey) as api.CollectionMetadata;
+      queryClient.setQueryData(containerQueryKey, {
+        ...previousData,
+        ...data,
+      });
+
+      return { previousData };
+    },
+    onError: (_err, _data, context) => {
+      queryClient.setQueryData(containerQueryKey, context?.previousData);
+    },
     onSettled: () => {
       // NOTE: We invalidate the library query here because we need to update the library's
       // container list.
       queryClient.invalidateQueries({ predicate: (query) => libraryQueryPredicate(query, libraryId) });
-      queryClient.invalidateQueries({ queryKey: libraryAuthoringQueryKeys.container(containerId) });
+      queryClient.invalidateQueries({ queryKey: containerQueryKey });
     },
   });
 };

src/library-authoring/units/LibraryUnitBlocks.tsx

@@ -63,16 +63,18 @@ const BlockHeader = ({ block }: ComponentBlockProps) => {
 
   const updateMutation = useUpdateXBlockFields(block.originalId);
 
-  const handleSaveDisplayName = (newDisplayName: string) => {
-    updateMutation.mutateAsync({
-      metadata: {
-        display_name: newDisplayName,
-      },
-    }).then(() => {
+  const handleSaveDisplayName = async (newDisplayName: string) => {
+    try {
+      await updateMutation.mutateAsync({
+        metadata: {
+          display_name: newDisplayName,
+        },
+      });
       showToast(intl.formatMessage(messages.updateComponentSuccessMsg));
-    }).catch(() => {
+    } catch (err) {
       showToast(intl.formatMessage(messages.updateComponentErrorMsg));
-    });
+      throw err;
+    }
   };
 
   /* istanbul ignore next */

src/library-authoring/units/LibraryUnitPage.test.tsx

@@ -106,12 +106,12 @@ describe('<LibraryUnitPage />', () => {
   it('can rename unit', async () => {
     renderLibraryUnitPage();
     expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument();
-    // Unit title
-    const unitTitle = screen.getAllByRole(
+
+    const editUnitTitleButton = screen.getAllByRole(
       'button',
-      { name: mockGetContainerMetadata.containerData.displayName },
-    )[0];
-    fireEvent.click(unitTitle);
+      { name: /edit/i },
+    )[0]; // 0 is the Unit Title, 1 is the first component on the list
+    fireEvent.click(editUnitTitleButton);
 
     const url = getLibraryContainerApiUrl(mockGetContainerMetadata.containerId);
     axiosMock.onPatch(url).reply(200);
@@ -137,12 +137,12 @@ describe('<LibraryUnitPage />', () => {
   it('show error if renaming unit fails', async () => {
     renderLibraryUnitPage();
     expect((await screen.findAllByText(libraryTitle))[0]).toBeInTheDocument();
-    // Unit title
-    const unitTitle = screen.getAllByRole(
+
+    const editUnitTitleButton = screen.getAllByRole(
       'button',
-      { name: mockGetContainerMetadata.containerData.displayName },
-    )[0];
-    fireEvent.click(unitTitle);
+      { name: /edit/i },
+    )[0]; // 0 is the Unit Title, 1 is the first component on the list
+    fireEvent.click(editUnitTitleButton);
 
     const url = getLibraryContainerApiUrl(mockGetContainerMetadata.containerId);
     axiosMock.onPatch(url).reply(400);
@@ -210,11 +210,11 @@ describe('<LibraryUnitPage />', () => {
     // Wait loading of the component
     await screen.findByText('text block 0');
 
-    const componentTitle = screen.getAllByRole(
+    const editButton = screen.getAllByRole(
       'button',
-      { name: 'text block 0' },
-    )[0];
-    fireEvent.click(componentTitle);
+      { name: /edit/i },
+    )[1]; // 0 is the Unit Title, 1 is the first component on the list
+    fireEvent.click(editButton);
 
     await waitFor(() => {
       expect(screen.getByRole('textbox', { name: /text input/i })).toBeInTheDocument();
@@ -244,11 +244,11 @@ describe('<LibraryUnitPage />', () => {
     // Wait loading of the component
     await screen.findByText('text block 0');
 
-    const componentTitle = screen.getAllByRole(
+    const editButton = screen.getAllByRole(
       'button',
-      { name: 'text block 0' },
-    )[0];
-    fireEvent.click(componentTitle);
+      { name: /edit/i },
+    )[1]; // 0 is the Unit Title, 1 is the first component on the list
+    fireEvent.click(editButton);
 
     await waitFor(() => {
       expect(screen.getByRole('textbox', { name: /text input/i })).toBeInTheDocument();

src/library-authoring/units/LibraryUnitPage.tsx

@@ -41,14 +41,16 @@ const EditableTitle = ({ unitId }: EditableTitleProps) => {
   const updateMutation = useUpdateContainer(unitId);
   const { showToast } = useContext(ToastContext);
 
-  const handleSaveDisplayName = (newDisplayName: string) => {
-    updateMutation.mutateAsync({
-      displayName: newDisplayName,
-    }).then(() => {
+  const handleSaveDisplayName = async (newDisplayName: string) => {
+    try {
+      await updateMutation.mutateAsync({
+        displayName: newDisplayName,
+      });
       showToast(intl.formatMessage(messages.updateContainerSuccessMsg));
-    }).catch(() => {
+    } catch (err) {
       showToast(intl.formatMessage(messages.updateContainerErrorMsg));
-    });
+      throw err;
+    }
   };
 
   // istanbul ignore if: this should never happen