Merge pull request #889 from acelaya-forks/feature/improve-hooks

Feature/improve hooks

.eslintrc

@@ -8,7 +8,6 @@
   },
   "ignorePatterns": ["src/service*.ts"],
   "rules": {
-    "react-hooks/rules-of-hooks": "off",
-    "react-hooks/exhaustive-deps": "off"
+    "react-hooks/rules-of-hooks": "off"
   }
 }

package-lock.json

@@ -15,7 +15,7 @@
         "@fortawesome/react-fontawesome": "^0.2.0",
         "@json2csv/plainjs": "^7.0.3",
         "@reduxjs/toolkit": "^1.9.5",
-        "@shlinkio/shlink-frontend-kit": "^0.2.0",
+        "@shlinkio/shlink-frontend-kit": "^0.2.1",
         "@shlinkio/shlink-js-sdk": "^0.1.0",
         "@shlinkio/shlink-web-component": "^0.3.3",
         "bootstrap": "5.2.3",
@@ -2801,9 +2801,9 @@
       }
     },
     "node_modules/@shlinkio/shlink-frontend-kit": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/@shlinkio/shlink-frontend-kit/-/shlink-frontend-kit-0.2.0.tgz",
-      "integrity": "sha512-8kGaae0bTiGzbLzPsolLvJ5ud37BR2b1WeDy8lyXIiwoFiSAMIgWpqro0nMdBVBQXovjmMbtiS6BFYsaoBo9/g==",
+      "version": "0.2.1",
+      "resolved": "https://registry.npmjs.org/@shlinkio/shlink-frontend-kit/-/shlink-frontend-kit-0.2.1.tgz",
+      "integrity": "sha512-5mRpQII9bGHAJQ1ghgGY+jFC5tD0y0ufgPqco9vLVGXGVf+BSNQrTiw/Cx6f9eCInqFPgFo8CdJzWUHbIzvC+Q==",
       "dependencies": {
         "classnames": "^2.3.2",
         "qs": "^6.11.2",
@@ -12390,9 +12390,9 @@
       }
     },
     "@shlinkio/shlink-frontend-kit": {
-      "version": "0.2.0",
-      "resolved": "https://registry.npmjs.org/@shlinkio/shlink-frontend-kit/-/shlink-frontend-kit-0.2.0.tgz",
-      "integrity": "sha512-8kGaae0bTiGzbLzPsolLvJ5ud37BR2b1WeDy8lyXIiwoFiSAMIgWpqro0nMdBVBQXovjmMbtiS6BFYsaoBo9/g==",
+      "version": "0.2.1",
+      "resolved": "https://registry.npmjs.org/@shlinkio/shlink-frontend-kit/-/shlink-frontend-kit-0.2.1.tgz",
+      "integrity": "sha512-5mRpQII9bGHAJQ1ghgGY+jFC5tD0y0ufgPqco9vLVGXGVf+BSNQrTiw/Cx6f9eCInqFPgFo8CdJzWUHbIzvC+Q==",
       "requires": {
         "classnames": "^2.3.2",
         "qs": "^6.11.2",

package.json

@@ -31,7 +31,7 @@
     "@fortawesome/react-fontawesome": "^0.2.0",
     "@json2csv/plainjs": "^7.0.3",
     "@reduxjs/toolkit": "^1.9.5",
-    "@shlinkio/shlink-frontend-kit": "^0.2.0",
+    "@shlinkio/shlink-frontend-kit": "^0.2.1",
     "@shlinkio/shlink-js-sdk": "^0.1.0",
     "@shlinkio/shlink-web-component": "^0.3.3",
     "bootstrap": "5.2.3",

src/app/App.tsx

@@ -1,7 +1,7 @@
 import { changeThemeInMarkup } from '@shlinkio/shlink-frontend-kit';
 import classNames from 'classnames';
 import type { FC } from 'react';
-import { useEffect } from 'react';
+import { useEffect, useRef } from 'react';
 import { Route, Routes, useLocation } from 'react-router-dom';
 import { AppUpdateBanner } from '../common/AppUpdateBanner';
 import { NotFound } from '../common/NotFound';
@@ -29,16 +29,20 @@ export const App = (
   ShlinkVersionsContainer: FC,
 ) => ({ fetchServers, servers, settings, appUpdated, resetAppUpdate }: AppProps) => {
   const location = useLocation();
+  const initialServers = useRef(servers);
   const isHome = location.pathname === '/';
 
   useEffect(() => {
-    // On first load, try to fetch the remote servers if the list is empty
-    if (Object.keys(servers).length === 0) {
+    // Try to fetch the remote servers if the list is empty at first
+    // We use a ref because we don't care if the servers list becomes empty later
+    if (Object.keys(initialServers.current).length === 0) {
       fetchServers();
     }
+  }, [fetchServers]);
 
+  useEffect(() => {
     changeThemeInMarkup(settings.ui?.theme ?? 'light');
-  }, []);
+  }, [settings.ui?.theme]);
 
   return (
     <div className="container-fluid app-container">

src/common/Home.tsx

@@ -23,7 +23,7 @@ export const Home = ({ servers }: HomeProps) => {
     // Try to redirect to the first server marked as auto-connect
     const autoConnectServer = serversList.find(({ autoConnect }) => autoConnect);
     autoConnectServer && navigate(`/server/${autoConnectServer.id}`);
-  }, []);
+  }, [serversList, navigate]);
 
   return (
     <div className="home">

src/common/MainHeader.tsx

@@ -10,14 +10,15 @@ import { ShlinkLogo } from './img/ShlinkLogo';
 import './MainHeader.scss';
 
 export const MainHeader = (ServersDropdown: FC) => () => {
-  const [isOpen, toggleOpen, , close] = useToggle();
+  const [isNotCollapsed, toggleCollapse, , collapse] = useToggle();
   const location = useLocation();
   const { pathname } = location;
 
-  useEffect(close, [location]);
+  // In mobile devices, collapse the navbar when location changes
+  useEffect(collapse, [location, collapse]);
 
   const settingsPath = '/settings';
-  const toggleClass = classNames('main-header__toggle-icon', { 'main-header__toggle-icon--opened': isOpen });
+  const toggleClass = classNames('main-header__toggle-icon', { 'main-header__toggle-icon--opened': isNotCollapsed });
 
   return (
     <Navbar color="primary" dark fixed="top" className="main-header" expand="md">
@@ -25,11 +26,11 @@ export const MainHeader = (ServersDropdown: FC) => () => {
         <ShlinkLogo className="main-header__brand-logo" color="white" /> Shlink
       </NavbarBrand>
 
-      <NavbarToggler onClick={toggleOpen}>
+      <NavbarToggler onClick={toggleCollapse}>
         <FontAwesomeIcon icon={arrowIcon} className={toggleClass} />
       </NavbarToggler>
 
-      <Collapse navbar isOpen={isOpen}>
+      <Collapse navbar isOpen={isNotCollapsed}>
         <Nav navbar className="ms-auto">
           <NavItem>
             <NavLink tag={Link} to={settingsPath} active={pathname.startsWith(settingsPath)}>

src/servers/CreateServer.tsx

@@ -1,6 +1,6 @@
 import { Result, useToggle } from '@shlinkio/shlink-frontend-kit';
 import type { FC } from 'react';
-import { useEffect, useState } from 'react';
+import { useCallback, useEffect, useState } from 'react';
 import { useNavigate } from 'react-router-dom';
 import { Button } from 'reactstrap';
 import { v4 as uuid } from 'uuid';
@@ -37,25 +37,25 @@ export const CreateServer = (ImportServersBtn: FC<ImportServersBtnProps>, useTim
   const [serversImported, setServersImported] = useTimeoutToggle(false, SHOW_IMPORT_MSG_TIME);
   const [errorImporting, setErrorImporting] = useTimeoutToggle(false, SHOW_IMPORT_MSG_TIME);
   const [isConfirmModalOpen, toggleConfirmModal] = useToggle();
-  const [serverData, setServerData] = useState<ServerData | undefined>();
-  const save = () => {
+  const [serverData, setServerData] = useState<ServerData>();
+  const saveNewServer = useCallback((theServerData: ServerData) => {
+    const id = uuid();
+
+    createServers([{ ...theServerData, id }]);
+    navigate(`/server/${id}`);
+  }, [createServers, navigate]);
+
+  useEffect(() => {
     if (!serverData) {
       return;
     }
 
-    const id = uuid();
-
-    createServers([{ ...serverData, id }]);
-    navigate(`/server/${id}`);
-  };
-
-  useEffect(() => {
     const serverExists = Object.values(servers).some(
       ({ url, apiKey }) => serverData?.url === url && serverData?.apiKey === apiKey,
     );
 
-    serverExists ? toggleConfirmModal() : save();
-  }, [serverData]);
+    serverExists ? toggleConfirmModal() : saveNewServer(serverData);
+  }, [saveNewServer, serverData, servers, toggleConfirmModal]);
 
   return (
     <NoMenuLayout>
@@ -74,7 +74,7 @@ export const CreateServer = (ImportServersBtn: FC<ImportServersBtnProps>, useTim
         isOpen={isConfirmModalOpen}
         duplicatedServers={serverData ? [serverData] : []}
         onDiscard={goBack}
-        onSave={save}
+        onSave={() => serverData && saveNewServer(serverData)}
       />
     </NoMenuLayout>
   );

src/servers/helpers/DuplicatedServersModal.tsx

@@ -33,7 +33,7 @@ export const DuplicatedServersModal: FC<DuplicatedServersModalProps> = (
         </span>
       </ModalBody>
       <ModalFooter>
-        <Button color="link" onClick={onDiscard}>{hasMultipleServers ? 'Ignore duplicated' : 'Discard'}</Button>
+        <Button color="link" onClick={onDiscard}>{hasMultipleServers ? 'Ignore duplicates' : 'Discard'}</Button>
         <Button color="primary" onClick={onSave}>Save anyway</Button>
       </ModalFooter>
     </Modal>

src/servers/helpers/ImportServersBtn.scss

@@ -1,5 +0,0 @@
-.import-servers-btn__csv-select {
-  position: absolute;
-  left: -9999px;
-  top: -9999px;
-}

src/servers/helpers/ImportServersBtn.tsx

@@ -1,14 +1,13 @@
 import { faFileUpload as importIcon } from '@fortawesome/free-solid-svg-icons';
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import { useElementRef, useToggle } from '@shlinkio/shlink-frontend-kit';
-import { complement, pipe } from 'ramda';
+import { complement } from 'ramda';
 import type { ChangeEvent, FC, PropsWithChildren } from 'react';
-import { useEffect, useState } from 'react';
+import { useCallback, useRef, useState } from 'react';
 import { Button, UncontrolledTooltip } from 'reactstrap';
 import type { ServerData, ServersMap } from '../data';
 import type { ServersImporter } from '../services/ServersImporter';
 import { DuplicatedServersModal } from './DuplicatedServersModal';
-import './ImportServersBtn.scss';
 
 export type ImportServersBtnProps = PropsWithChildren<{
   onImport?: () => void;
@@ -25,7 +24,7 @@ interface ImportServersBtnConnectProps extends ImportServersBtnProps {
 const serversFiltering = (servers: ServerData[]) =>
   ({ url, apiKey }: ServerData) => servers.some((server) => server.url === url && server.apiKey === apiKey);
 
-export const ImportServersBtn = ({ importServersFromFile }: ServersImporter): FC<ImportServersBtnConnectProps> => ({
+export const ImportServersBtn = (serversImporter: ServersImporter): FC<ImportServersBtnConnectProps> => ({
   createServers,
   servers,
   children,
@@ -35,36 +34,43 @@ export const ImportServersBtn = ({ importServersFromFile }: ServersImporter): FC
   className = '',
 }) => {
   const ref = useElementRef<HTMLInputElement>();
-  const [serversToCreate, setServersToCreate] = useState<ServerData[] | undefined>();
   const [duplicatedServers, setDuplicatedServers] = useState<ServerData[]>([]);
   const [isModalOpen,, showModal, hideModal] = useToggle();
-  const create = pipe(createServers, onImport);
-  const createAllServers = pipe(() => create(serversToCreate ?? []), hideModal);
-  const createNonDuplicatedServers = pipe(
-    () => create((serversToCreate ?? []).filter(complement(serversFiltering(duplicatedServers)))),
-    hideModal,
-  );
-  const onFile = async ({ target }: ChangeEvent<HTMLInputElement>) =>
-    importServersFromFile(target.files?.[0])
-      .then(setServersToCreate)
+
+  const serversToCreate = useRef<ServerData[]>([]);
+  const create = useCallback((serversData: ServerData[]) => {
+    createServers(serversData);
+    onImport();
+  }, [createServers, onImport]);
+  const onFile = useCallback(
+    async ({ target }: ChangeEvent<HTMLInputElement>) =>
+      serversImporter.importServersFromFile(target.files?.[0])
+        .then((newServers) => {
+          serversToCreate.current = newServers;
+
+          const existingServers = Object.values(servers);
+          const dupServers = newServers.filter(serversFiltering(existingServers));
+          const hasDuplicatedServers = !!dupServers.length;
+
+          !hasDuplicatedServers ? create(newServers) : setDuplicatedServers(dupServers);
+          hasDuplicatedServers && showModal();
+        })
         .then(() => {
           // Reset input after processing file
           (target as { value: string | null }).value = null; // eslint-disable-line no-param-reassign
         })
-      .catch(onImportError);
+        .catch(onImportError),
+    [create, onImportError, servers, showModal],
+  );
 
-  useEffect(() => {
-    if (!serversToCreate) {
-      return;
-    }
-
-    const existingServers = Object.values(servers);
-    const dupServers = serversToCreate.filter(serversFiltering(existingServers));
-    const hasDuplicatedServers = !!dupServers.length;
-
-    !hasDuplicatedServers ? create(serversToCreate) : setDuplicatedServers(dupServers);
-    hasDuplicatedServers && showModal();
-  }, [serversToCreate]);
+  const createAllServers = useCallback(() => {
+    create(serversToCreate.current);
+    hideModal();
+  }, [create, hideModal, serversToCreate]);
+  const createNonDuplicatedServers = useCallback(() => {
+    create(serversToCreate.current.filter(complement(serversFiltering(duplicatedServers))));
+    hideModal();
+  }, [create, duplicatedServers, hideModal]);
 
   return (
     <>
@@ -72,16 +78,10 @@ export const ImportServersBtn = ({ importServersFromFile }: ServersImporter): FC
         <FontAwesomeIcon icon={importIcon} fixedWidth /> {children ?? 'Import from file'}
       </Button>
       <UncontrolledTooltip placement={tooltipPlacement} target="importBtn">
-        You can create servers by importing a CSV file with columns <b>name</b>, <b>apiKey</b> and <b>url</b>.
+        You can create servers by importing a CSV file with <b>name</b>, <b>apiKey</b> and <b>url</b> columns.
       </UncontrolledTooltip>
 
-      <input
-        type="file"
-        accept="text/csv"
-        className="import-servers-btn__csv-select"
-        ref={ref}
-        onChange={onFile}
-      />
+      <input type="file" accept="text/csv" className="d-none" ref={ref} onChange={onFile} />
 
       <DuplicatedServersModal
         isOpen={isModalOpen}

src/servers/helpers/withSelectedServer.tsx

@@ -18,7 +18,7 @@ export function withSelectedServer<T = {}>(WrappedComponent: FC<WithSelectedServ
 
     useEffect(() => {
       params.serverId && selectServer(params.serverId);
-    }, [params.serverId]);
+    }, [params.serverId, selectServer]);
 
     if (!selectedServer) {
       return (

src/servers/helpers/withoutSelectedServer.tsx

@@ -10,7 +10,7 @@ export function withoutSelectedServer<T = {}>(WrappedComponent: FC<WithoutSelect
     const { resetSelectedServer } = props;
     useEffect(() => {
       resetSelectedServer();
-    }, []);
+    }, [resetSelectedServer]);
 
     return <WrappedComponent {...props} />;
   };

src/servers/services/ServersImporter.ts

@@ -8,32 +8,20 @@ const validateServers = (servers: any): servers is ServerData[] =>
   Array.isArray(servers) && servers.every(validateServer);
 
 export class ServersImporter {
-  public constructor(private readonly csvToJson: CsvToJson, private readonly fileReaderFactory: () => FileReader) {}
+  public constructor(private readonly csvToJson: CsvToJson) {}
 
-  public readonly importServersFromFile = async (file?: File | null): Promise<ServerData[]> => {
+  public async importServersFromFile(file: File | null | undefined): Promise<ServerData[]> {
     if (!file) {
       throw new Error('No file provided');
     }
 
-    const reader = this.fileReaderFactory();
-
-    return new Promise((resolve, reject) => {
-      reader.addEventListener('loadend', async (e: ProgressEvent<FileReader>) => {
-        try {
-          // TODO Read as stream, otherwise, if the file is too big, this will block the browser tab
-          const content = e.target?.result?.toString() ?? '';
+    const content = await file.text();
     const servers = await this.csvToJson(content);
 
     if (!validateServers(servers)) {
       throw new Error('Provided file does not have the right format.');
     }
 
-          resolve(servers);
-        } catch (error) {
-          reject(error);
+    return servers;
   }
-      });
-      reader.readAsText(file);
-    });
-  };
 }

src/servers/services/provideServices.ts

@@ -62,8 +62,7 @@ export const provideServices = (bottle: Bottle, connect: ConnectDecorator) => {
   bottle.decorator('ServerError', connect(['servers', 'selectedServer']));
 
   // Services
-  bottle.constant('fileReaderFactory', () => new FileReader());
-  bottle.service('ServersImporter', ServersImporter, 'csvToJson', 'fileReaderFactory');
+  bottle.service('ServersImporter', ServersImporter, 'csvToJson');
   bottle.service('ServersExporter', ServersExporter, 'Storage', 'window', 'jsonToCsv');
 
   // Actions

src/settings/UserInterfaceSettings.tsx

@@ -1,7 +1,7 @@
 import { faMoon, faSun } from '@fortawesome/free-solid-svg-icons';
 import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
 import type { Theme } from '@shlinkio/shlink-frontend-kit';
-import { changeThemeInMarkup, SimpleCard, ToggleSwitch } from '@shlinkio/shlink-frontend-kit';
+import { SimpleCard, ToggleSwitch } from '@shlinkio/shlink-frontend-kit';
 import type { FC } from 'react';
 import type { AppSettings, UiSettings } from './reducers/settings';
 import './UserInterfaceSettings.scss';
@@ -18,9 +18,7 @@ export const UserInterfaceSettings: FC<UserInterfaceProps> = ({ settings: { ui }
       checked={ui?.theme === 'dark'}
       onChange={(useDarkTheme) => {
         const theme: Theme = useDarkTheme ? 'dark' : 'light';
-
         setUiSettings({ ...ui, theme });
-        changeThemeInMarkup(theme);
       }}
     >
       Use dark theme.

test/servers/CreateServer.test.tsx

@@ -2,7 +2,7 @@ import { fireEvent, screen, waitFor } from '@testing-library/react';
 import { fromPartial } from '@total-typescript/shoehorn';
 import { useNavigate } from 'react-router-dom';
 import { CreateServer as createCreateServer } from '../../src/servers/CreateServer';
-import type { ServerWithId } from '../../src/servers/data';
+import type { ServersMap } from '../../src/servers/data';
 import { renderWithEvents } from '../__helpers__/setUpTest';
 
 vi.mock('react-router-dom', async () => ({
@@ -10,11 +10,19 @@ vi.mock('react-router-dom', async () => ({
   useNavigate: vi.fn(),
 }));
 
+type SetUpOptions = {
+  serversImported?: boolean;
+  importFailed?: boolean;
+  servers?: ServersMap;
+};
+
 describe('<CreateServer />', () => {
   const createServersMock = vi.fn();
   const navigate = vi.fn();
-  const servers = { foo: fromPartial<ServerWithId>({ url: 'https://existing_url.com', apiKey: 'existing_api_key' }) };
-  const setUp = (serversImported = false, importFailed = false) => {
+  const defaultServers: ServersMap = {
+    foo: fromPartial({ url: 'https://existing_url.com', apiKey: 'existing_api_key' }),
+  };
+  const setUp = ({ serversImported = false, importFailed = false, servers = defaultServers }: SetUpOptions = {}) => {
     (useNavigate as any).mockReturnValue(navigate);
 
     let callCount = 0;
@@ -29,16 +37,17 @@ describe('<CreateServer />', () => {
   };
 
   it('shows success message when imported is true', () => {
-    setUp(true);
+    setUp({ serversImported: true });
 
     expect(screen.getByText('Servers properly imported. You can now select one from the list :)')).toBeInTheDocument();
     expect(
       screen.queryByText('The servers could not be imported. Make sure the format is correct.'),
     ).not.toBeInTheDocument();
+    expect(screen.queryByText('ImportServersBtn')).not.toBeInTheDocument();
   });
 
   it('shows error message when import failed', () => {
-    setUp(false, true);
+    setUp({ importFailed: true });
 
     expect(
       screen.queryByText('Servers properly imported. You can now select one from the list :)'),
@@ -46,6 +55,11 @@ describe('<CreateServer />', () => {
     expect(screen.getByText('The servers could not be imported. Make sure the format is correct.')).toBeInTheDocument();
   });
 
+  it('shows import button when no servers exist yet', () => {
+    setUp({ servers: {} });
+    expect(screen.queryByText('ImportServersBtn')).toBeInTheDocument();
+  });
+
   it('creates server data when form is submitted', async () => {
     const { user } = setUp();
 

test/servers/helpers/DuplicatedServersModal.test.tsx

@@ -39,7 +39,7 @@ describe('<DuplicatedServersModal />', () => {
         header: 'Duplicated servers',
         firstParagraph: 'The next servers already exist:',
         lastParagraph: 'Do you want to ignore duplicated servers?',
-        discardBtn: 'Ignore duplicated',
+        discardBtn: 'Ignore duplicates',
       },
     ],
   ])('renders expected texts based on amount of servers', (duplicatedServers, assertions) => {

test/servers/services/ServersImporter.test.ts

@@ -5,18 +5,13 @@ import { ServersImporter } from '../../../src/servers/services/ServersImporter';
 describe('ServersImporter', () => {
   const servers: RegularServer[] = [fromPartial<RegularServer>({}), fromPartial<RegularServer>({})];
   const csvjsonMock = vi.fn().mockResolvedValue(servers);
-  const readAsText = vi.fn();
-  const fileReaderMock = fromPartial<FileReader>({
-    readAsText,
-    addEventListener: ((_eventName: string, listener: (e: ProgressEvent<FileReader>) => void) => listener(
-      fromPartial({ target: { result: '' } }),
-    )) as any,
-  });
-  const importer = new ServersImporter(csvjsonMock, () => fileReaderMock);
+  const text = vi.fn().mockReturnValue('');
+  const fileMock = () => fromPartial<File>({ text });
+  const importer = new ServersImporter(csvjsonMock);
 
   describe('importServersFromFile', () => {
-    it('rejects with error if no file was provided', async () => {
-      await expect(importer.importServersFromFile()).rejects.toEqual(
+    it.each([[null], [undefined]])('rejects with error if no file was provided', async (file) => {
+      await expect(importer.importServersFromFile(file)).rejects.toEqual(
         new Error('No file provided'),
       );
     });
@@ -26,7 +21,7 @@ describe('ServersImporter', () => {
 
       csvjsonMock.mockRejectedValue(expectedError);
 
-      await expect(importer.importServersFromFile(fromPartial({ type: 'text/html' }))).rejects.toEqual(expectedError);
+      await expect(importer.importServersFromFile(fileMock())).rejects.toEqual(expectedError);
     });
 
     it.each([
@@ -55,7 +50,7 @@ describe('ServersImporter', () => {
     ])('rejects with error if provided file does not parse to valid list of servers', async (parsedObject) => {
       csvjsonMock.mockResolvedValue(parsedObject);
 
-      await expect(importer.importServersFromFile(fromPartial({ type: 'text/html' }))).rejects.toEqual(
+      await expect(importer.importServersFromFile(fileMock())).rejects.toEqual(
         new Error('Provided file does not have the right format.'),
       );
     });
@@ -76,10 +71,10 @@ describe('ServersImporter', () => {
 
       csvjsonMock.mockResolvedValue(expectedServers);
 
-      const result = await importer.importServersFromFile(fromPartial({}));
+      const result = await importer.importServersFromFile(fileMock());
 
       expect(result).toEqual(expectedServers);
-      expect(readAsText).toHaveBeenCalledTimes(1);
+      expect(text).toHaveBeenCalledTimes(1);
       expect(csvjsonMock).toHaveBeenCalledTimes(1);
     });
   });