Migrate DuplicatedServersModal to tailwind

src/servers/CreateServer.tsx

@@ -71,7 +71,7 @@ const CreateServer: FCWithDeps<CreateServerProps, CreateServerDeps> = ({ servers
     <NoMenuLayout>
       <ServerForm title="Add new server" onSubmit={onSubmit}>
         {!hasServers && (
-          <ImportServersBtn tooltipPlacement="top" onImport={setServersImported} onImportError={setErrorImporting} />
+          <ImportServersBtn tooltipPlacement="top" onImport={setServersImported} onError={setErrorImporting} />
         )}
         {hasServers && <Button variant="secondary" onClick={goBack}>Cancel</Button>}
         <Button type="submit">Create server</Button>
@@ -81,10 +81,10 @@ const CreateServer: FCWithDeps<CreateServerProps, CreateServerDeps> = ({ servers
       {errorImporting && <ImportResult variant="error" />}
 
       <DuplicatedServersModal
-        isOpen={isConfirmModalOpen}
+        open={isConfirmModalOpen}
         duplicatedServers={serverData ? [serverData] : []}
-        onDiscard={goBack}
-        onSave={() => serverData && saveNewServer(serverData)}
+        onClose={goBack}
+        onConfirm={() => serverData && saveNewServer(serverData)}
       />
     </NoMenuLayout>
   );

src/servers/ManageServers.tsx

@@ -48,7 +48,7 @@ const ManageServers: FCWithDeps<ManageServersProps, ManageServersDeps> = ({ serv
 
       <div className="tw:flex tw:flex-col tw:md:flex-row tw:gap-2">
         <div className="tw:flex tw:gap-2">
-          <ImportServersBtn className="tw:flex-grow" onImportError={setErrorImporting}>Import servers</ImportServersBtn>
+          <ImportServersBtn className="tw:flex-grow" onError={setErrorImporting}>Import servers</ImportServersBtn>
           {filteredServers.length > 0 && (
             <Button variant="secondary" className="tw:flex-grow" onClick={async () => serversExporter.exportServers()}>
               <FontAwesomeIcon icon={exportIcon} /> Export servers

src/servers/helpers/DuplicatedServersModal.tsx

@@ -1,41 +1,42 @@
+import { CardModal } from '@shlinkio/shlink-frontend-kit/tailwind';
 import type { FC } from 'react';
 import { Fragment } from 'react';
-import { Button, Modal, ModalBody, ModalFooter, ModalHeader } from 'reactstrap';
 import type { ServerData } from '../data';
 
-interface DuplicatedServersModalProps {
+export type DuplicatedServersModalProps = {
   duplicatedServers: ServerData[];
-  isOpen: boolean;
-  onDiscard: () => void;
-  onSave: () => void;
-}
+  open: boolean;
+  onClose: () => void;
+  onConfirm: () => void;
+};
 
 export const DuplicatedServersModal: FC<DuplicatedServersModalProps> = (
-  { isOpen, duplicatedServers, onDiscard, onSave },
+  { open, duplicatedServers, onClose, onConfirm },
 ) => {
   const hasMultipleServers = duplicatedServers.length > 1;
 
   return (
-    <Modal centered isOpen={isOpen}>
-      <ModalHeader>Duplicated server{hasMultipleServers && 's'}</ModalHeader>
-      <ModalBody>
-        <p>{hasMultipleServers ? 'The next servers already exist:' : 'There is already a server with:'}</p>
-        <ul>
-          {duplicatedServers.map(({ url, apiKey }, index) => (!hasMultipleServers ? (
-            <Fragment key={index}>
-              <li>URL: <b>{url}</b></li>
-              <li>API key: <b>{apiKey}</b></li>
-            </Fragment>
-          ) : <li key={index}><b>{url}</b> - <b>{apiKey}</b></li>))}
-        </ul>
-        <span>
-          {hasMultipleServers ? 'Do you want to ignore duplicated servers' : 'Do you want to save this server anyway'}?
-        </span>
-      </ModalBody>
-      <ModalFooter>
-        <Button color="link" onClick={onDiscard}>{hasMultipleServers ? 'Ignore duplicates' : 'Discard'}</Button>
-        <Button color="primary" onClick={onSave}>Save anyway</Button>
-      </ModalFooter>
-    </Modal>
+    <CardModal
+      size="lg"
+      title={`Duplicated server${hasMultipleServers ? 's' : ''}`}
+      open={open}
+      onClose={onClose}
+      onConfirm={onConfirm}
+      confirmText={`Save duplicate${hasMultipleServers ? 's' : ''}`}
+      cancelText={hasMultipleServers ? 'Ignore duplicates' : 'Discard'}
+    >
+      <p>{hasMultipleServers ? 'The next servers already exist:' : 'There is already a server with:'}</p>
+      <ul className="tw:list-disc tw:mt-4">
+        {duplicatedServers.map(({ url, apiKey }, index) => (!hasMultipleServers ? (
+          <Fragment key={index}>
+            <li>URL: <b>{url}</b></li>
+            <li>API key: <b>{apiKey}</b></li>
+          </Fragment>
+        ) : <li key={index}><b>{url}</b> - <b>{apiKey}</b></li>))}
+      </ul>
+      <span>
+        {hasMultipleServers ? 'Do you want to save duplicated servers' : 'Do you want to save this server'}?
+      </span>
+    </CardModal>
   );
 };

src/servers/helpers/ImportServersBtn.tsx

@@ -3,7 +3,7 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import { useElementRef, useToggle } from '@shlinkio/shlink-frontend-kit';
 import { Button } from '@shlinkio/shlink-frontend-kit/tailwind';
 import type { ChangeEvent, PropsWithChildren } from 'react';
-import { useCallback, useRef, useState } from 'react';
+import { useCallback, useRef , useState } from 'react';
 import { UncontrolledTooltip } from 'reactstrap';
 import type { FCWithDeps } from '../../container/utils';
 import { componentFactory, useDependencies } from '../../container/utils';
@@ -14,7 +14,7 @@ import { dedupServers, ensureUniqueIds } from './index';
 
 export type ImportServersBtnProps = PropsWithChildren<{
   onImport?: () => void;
-  onImportError?: (error: Error) => void;
+  onError?: (error: Error) => void;
   tooltipPlacement?: 'top' | 'bottom';
   className?: string;
 }>;
@@ -32,8 +32,8 @@ const ImportServersBtn: FCWithDeps<ImportServersBtnConnectProps, ImportServersBt
   createServers,
   servers,
   children,
-  onImport = () => {},
-  onImportError = () => {},
+  onImport,
+  onError = () => {},
   tooltipPlacement = 'bottom',
   className = '',
 }) => {
@@ -41,41 +41,46 @@ const ImportServersBtn: FCWithDeps<ImportServersBtnConnectProps, ImportServersBt
   const ref = useElementRef<HTMLInputElement>();
   const [duplicatedServers, setDuplicatedServers] = useState<ServerData[]>([]);
   const [isModalOpen,, showModal, hideModal] = useToggle();
+  const newServersCreatedRef = useRef(false);
 
-  const importedServersRef = useRef<ServerWithId[]>([]);
-  const newServersRef = useRef<ServerWithId[]>([]);
-
-  const create = useCallback((serversData: ServerWithId[]) => {
-    createServers(serversData);
-    hideModal();
-    onImport();
-  }, [createServers, hideModal, onImport]);
   const onFile = useCallback(
     async ({ target }: ChangeEvent<HTMLInputElement>) =>
       serversImporter.importServersFromFile(target.files?.[0])
         .then((importedServers) => {
           const { duplicatedServers, newServers } = dedupServers(servers, importedServers);
 
-          importedServersRef.current = ensureUniqueIds(servers, importedServers);
-          newServersRef.current = ensureUniqueIds(servers, newServers);
+          // Immediately create new servers
+          newServersCreatedRef.current = newServers.length > 0;
+          createServers(ensureUniqueIds(servers, newServers));
 
-          if (duplicatedServers.length === 0) {
-            create(importedServersRef.current);
-          } else {
+          // For duplicated servers, ask for confirmation
+          if (duplicatedServers.length > 0) {
             setDuplicatedServers(duplicatedServers);
             showModal();
+          } else {
+            onImport?.();
           }
         })
         .then(() => {
           // Reset file input after processing file
           (target as { value: string | null }).value = null;
         })
-        .catch(onImportError),
-    [create, onImportError, servers, serversImporter, showModal],
+        .catch(onError),
+    [createServers, onError, onImport, servers, serversImporter, showModal],
   );
 
-  const createAllServers = useCallback(() => create(importedServersRef.current), [create]);
-  const createNonDuplicatedServers = useCallback(() => create(newServersRef.current), [create]);
+  const createDuplicatedServers = useCallback(() => {
+    createServers(ensureUniqueIds(servers, duplicatedServers));
+    hideModal();
+    onImport?.();
+  }, [createServers, duplicatedServers, hideModal, onImport, servers]);
+  const discardDuplicatedServers = useCallback(() => {
+    hideModal();
+    // If duplicated servers were discarded but some non-duplicated servers were created, call onImport
+    if (newServersCreatedRef.current) {
+      onImport?.();
+    }
+  }, [hideModal, onImport]);
 
   return (
     <>
@@ -97,10 +102,10 @@ const ImportServersBtn: FCWithDeps<ImportServersBtnConnectProps, ImportServersBt
       />
 
       <DuplicatedServersModal
-        isOpen={isModalOpen}
+        open={isModalOpen}
         duplicatedServers={duplicatedServers}
-        onDiscard={createNonDuplicatedServers}
-        onSave={createAllServers}
+        onClose={discardDuplicatedServers}
+        onConfirm={createDuplicatedServers}
       />
     </>
   );

test/servers/helpers/DuplicatedServersModal.test.tsx

@@ -6,10 +6,10 @@ import { checkAccessibility } from '../../__helpers__/accessibility';
 import { renderWithEvents } from '../../__helpers__/setUpTest';
 
 describe('<DuplicatedServersModal />', () => {
-  const onDiscard = vi.fn();
-  const onSave = vi.fn();
+  const onClose = vi.fn();
+  const onConfirm = vi.fn();
   const setUp = (duplicatedServers: ServerData[] = []) => act(() => renderWithEvents(
-    <DuplicatedServersModal isOpen duplicatedServers={duplicatedServers} onDiscard={onDiscard} onSave={onSave} />,
+    <DuplicatedServersModal open duplicatedServers={duplicatedServers} onClose={onClose} onConfirm={onConfirm} />,
   ));
   const mockServer = (data: Partial<ServerData> = {}) => fromPartial<ServerData>(data);
 
@@ -32,8 +32,9 @@ describe('<DuplicatedServersModal />', () => {
       {
         header: 'Duplicated server',
         firstParagraph: 'There is already a server with:',
-        lastParagraph: 'Do you want to save this server anyway?',
+        lastParagraph: 'Do you want to save this server?',
         discardBtn: 'Discard',
+        confirmButton: 'Save duplicate',
       },
     ],
     [
@@ -41,8 +42,9 @@ describe('<DuplicatedServersModal />', () => {
       {
         header: 'Duplicated servers',
         firstParagraph: 'The next servers already exist:',
-        lastParagraph: 'Do you want to ignore duplicated servers?',
+        lastParagraph: 'Do you want to save duplicated servers?',
         discardBtn: 'Ignore duplicates',
+        confirmButton: 'Save duplicates',
       },
     ],
   ])('renders expected texts based on amount of servers', async (duplicatedServers, assertions) => {
@@ -52,6 +54,7 @@ describe('<DuplicatedServersModal />', () => {
     expect(screen.getByText(assertions.firstParagraph)).toBeInTheDocument();
     expect(screen.getByText(assertions.lastParagraph)).toBeInTheDocument();
     expect(screen.getByRole('button', { name: assertions.discardBtn })).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: assertions.confirmButton })).toBeInTheDocument();
   });
 
   it.each([
@@ -80,19 +83,19 @@ describe('<DuplicatedServersModal />', () => {
     }
   });
 
-  it('invokes onDiscard when appropriate button is clicked', async () => {
+  it('invokes onClose when appropriate button is clicked', async () => {
     const { user } = await setUp();
 
-    expect(onDiscard).not.toHaveBeenCalled();
+    expect(onClose).not.toHaveBeenCalled();
     await user.click(screen.getByRole('button', { name: 'Discard' }));
-    expect(onDiscard).toHaveBeenCalled();
+    expect(onClose).toHaveBeenCalled();
   });
 
-  it('invokes onSave when appropriate button is clicked', async () => {
+  it('invokes onConfirm when appropriate button is clicked', async () => {
     const { user } = await setUp();
 
-    expect(onSave).not.toHaveBeenCalled();
-    await user.click(screen.getByRole('button', { name: 'Save anyway' }));
-    expect(onSave).toHaveBeenCalled();
+    expect(onConfirm).not.toHaveBeenCalled();
+    await user.click(screen.getByRole('button', { name: 'Save duplicate' }));
+    expect(onConfirm).toHaveBeenCalled();
   });
 });

test/servers/helpers/ImportServersBtn.test.tsx

@@ -65,9 +65,9 @@ describe('<ImportServersBtn />', () => {
   });
 
   it.each([
-    { btnName: 'Save anyway',savesDuplicatedServers: true },
+    { btnName: 'Save duplicate', savesDuplicatedServers: true },
     { btnName: 'Discard', savesDuplicatedServers: false },
-  ])('creates expected servers depending on selected option in modal', async ({ btnName, savesDuplicatedServers }) => {
+  ])('creates duplicated servers depending on selected option in modal', async ({ btnName, savesDuplicatedServers }) => {
     const existingServerData: ServerData = {
       name: 'existingServer',
       url: 'http://s.test/existingUrl',
@@ -84,14 +84,20 @@ describe('<ImportServersBtn />', () => {
 
     expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
     await user.upload(screen.getByTestId('csv-file-input'), csvFile);
+
+    // Once the file is uploaded, non-duplicated servers are immediately created
+    expect(createServersMock).toHaveBeenCalledExactlyOnceWith([expect.objectContaining(newServer)]);
+
     expect(screen.getByRole('dialog')).toBeInTheDocument();
     await user.click(screen.getByRole('button', { name: btnName }));
 
-    expect(createServersMock).toHaveBeenCalledWith(
-      savesDuplicatedServers
-        ? [expect.objectContaining(existingServerData), expect.objectContaining(newServer)]
-        : [expect.objectContaining(newServer)],
-    );
-    expect(onImportMock).toHaveBeenCalledTimes(1);
+    // If duplicated servers are saved, there's one extra call
+    if (savesDuplicatedServers) {
+      expect(createServersMock).toHaveBeenLastCalledWith([expect.objectContaining(existingServerData)]);
+    }
+
+    // On import is called only once, no matter what
+    expect(onImportMock).toHaveBeenCalledOnce();
+    expect(createServersMock).toHaveBeenCalledTimes(savesDuplicatedServers ? 2 : 1);
   });
 });