Repository: shlinkio/shlink-web-client (ts)

Issue:
Improve getShortUrlDetail redux action, so that it tries to pick the short URL from the list, and falls back to an API request only if not found
<!--
Before opening an issue, just take into account that this is a completely free of charge and open source project.
I'm always happy to help and provide support, but some understanding will be expected.
I do this in my own free time, so expect some delays when implementing new features and fixing bugs, and don't take it personal if an issue gets eventually closed.
You may also be asked to provide tests or ways to reproduce reported bugs.
Try to be polite, and understand it is impossible for an OSS project to cover all use cases.
-->

This action is used when loading the short URL detail for the visits section, and soon for the new "edit short URL" section.

Usually those sections will be visited from the short URLs list, where that short URL is already loaded from the list.

Change the action's behavior so that it tries to find the short URL from the short URLs list state, which will usually have it, and only when it is not found it falls back to making an API call, which will be needed when linking directly to any of the previously mentioned sections.

Pull request description:
Feature/improve reducer
Closes #398 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 66806cfc..0ca5ced5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 ### Changed
 * [#382](https://github.com/shlinkio/shlink-web-client/issues/382) Ensured short URL tags are edited through the `PATCH /short-urls/{shortCode}` endpoint when using Shlink 2.6.0 or higher.
+* [#398](https://github.com/shlinkio/shlink-web-client/issues/398) Improved performance when loading short URL details by avoiding API calls if the short URL is already present in local state.
 
 ### Deprecated
 * *Nothing*
diff --git a/src/container/types.ts b/src/container/types.ts
index d51764c2..3b4fff27 100644
--- a/src/container/types.ts
+++ b/src/container/types.ts
@@ -11,7 +11,7 @@ import { ShortUrlsList } from '../short-urls/reducers/shortUrlsList';
 import { TagDeletion } from '../tags/reducers/tagDelete';
 import { TagEdition } from '../tags/reducers/tagEdit';
 import { TagsList } from '../tags/reducers/tagsList';
-import { ShortUrlDetail } from '../visits/reducers/shortUrlDetail';
+import { ShortUrlDetail } from '../short-urls/reducers/shortUrlDetail';
 import { ShortUrlVisits } from '../visits/reducers/shortUrlVisits';
 import { TagVisits } from '../visits/reducers/tagVisits';
 import { DomainsList } from '../domains/reducers/domainsList';
diff --git a/src/reducers/index.ts b/src/reducers/index.ts
index 4efcf1c6..29311fa7 100644
--- a/src/reducers/index.ts
+++ b/src/reducers/index.ts
@@ -11,7 +11,7 @@ import shortUrlEditionReducer from '../short-urls/reducers/shortUrlEdition';
 import shortUrlVisitsReducer from '../visits/reducers/shortUrlVisits';
 import tagVisitsReducer from '../visits/reducers/tagVisits';
 import orphanVisitsReducer from '../visits/reducers/orphanVisits';
-import shortUrlDetailReducer from '../visits/reducers/shortUrlDetail';
+import shortUrlDetailReducer from '../short-urls/reducers/shortUrlDetail';
 import tagsListReducer from '../tags/reducers/tagsList';
 import tagDeleteReducer from '../tags/reducers/tagDelete';
 import tagEditReducer from '../tags/reducers/tagEdit';
diff --git a/src/visits/reducers/shortUrlDetail.ts b/src/short-urls/reducers/shortUrlDetail.ts
similarity index 83%
rename from src/visits/reducers/shortUrlDetail.ts
rename to src/short-urls/reducers/shortUrlDetail.ts
index c8fb5bea..42771a92 100644
--- a/src/visits/reducers/shortUrlDetail.ts
+++ b/src/short-urls/reducers/shortUrlDetail.ts
@@ -1,9 +1,10 @@
 import { Action, Dispatch } from 'redux';
-import { ShortUrl } from '../../short-urls/data';
+import { ShortUrl } from '../data';
 import { buildReducer } from '../../utils/helpers/redux';
 import { ShlinkApiClientBuilder } from '../../api/services/ShlinkApiClientBuilder';
 import { OptionalString } from '../../utils/utils';
 import { GetState } from '../../container/types';
+import { shortUrlMatches } from '../helpers';
 
 /* eslint-disable padding-line-between-statements */
 export const GET_SHORT_URL_DETAIL_START = 'shlink/shortUrlDetail/GET_SHORT_URL_DETAIL_START';
@@ -37,10 +38,12 @@ export const getShortUrlDetail = (buildShlinkApiClient: ShlinkApiClientBuilder)
   domain: OptionalString,
 ) => async (dispatch: Dispatch, getState: GetState) => {
   dispatch({ type: GET_SHORT_URL_DETAIL_START });
-  const { getShortUrl } = buildShlinkApiClient(getState);
 
   try {
-    const shortUrl = await getShortUrl(shortCode, domain);
+    const { shortUrlsList } = getState();
+    const shortUrl = shortUrlsList?.shortUrls?.data.find(
+      (shortUrl) => shortUrlMatches(shortUrl, shortCode, domain),
+    ) ?? await buildShlinkApiClient(getState).getShortUrl(shortCode, domain);
 
     dispatch<ShortUrlDetailAction>({ shortUrl, type: GET_SHORT_URL_DETAIL });
   } catch (e) {
diff --git a/src/visits/ShortUrlVisits.tsx b/src/visits/ShortUrlVisits.tsx
index 3d71b8cf..509e34ae 100644
--- a/src/visits/ShortUrlVisits.tsx
+++ b/src/visits/ShortUrlVisits.tsx
@@ -4,9 +4,9 @@ import { boundToMercureHub } from '../mercure/helpers/boundToMercureHub';
 import { ShlinkVisitsParams } from '../api/types';
 import { parseQuery } from '../utils/helpers/query';
 import { Topics } from '../mercure/helpers/Topics';
+import { ShortUrlDetail } from '../short-urls/reducers/shortUrlDetail';
 import { ShortUrlVisits as ShortUrlVisitsState } from './reducers/shortUrlVisits';
 import ShortUrlVisitsHeader from './ShortUrlVisitsHeader';
-import { ShortUrlDetail } from './reducers/shortUrlDetail';
 import VisitsStats from './VisitsStats';
 
 export interface ShortUrlVisitsProps extends RouteComponentProps<{ shortCode: string }> {
diff --git a/src/visits/ShortUrlVisitsHeader.tsx b/src/visits/ShortUrlVisitsHeader.tsx
index 38826b8e..c581f3ed 100644
--- a/src/visits/ShortUrlVisitsHeader.tsx
+++ b/src/visits/ShortUrlVisitsHeader.tsx
@@ -1,7 +1,7 @@
 import { UncontrolledTooltip } from 'reactstrap';
 import Moment from 'react-moment';
 import { ExternalLink } from 'react-external-link';
-import { ShortUrlDetail } from './reducers/shortUrlDetail';
+import { ShortUrlDetail } from '../short-urls/reducers/shortUrlDetail';
 import { ShortUrlVisits } from './reducers/shortUrlVisits';
 import VisitsHeader from './VisitsHeader';
 import './ShortUrlVisitsHeader.scss';
diff --git a/src/visits/services/provideServices.ts b/src/visits/services/provideServices.ts
index f550b68f..5e7775f5 100644
--- a/src/visits/services/provideServices.ts
+++ b/src/visits/services/provideServices.ts
@@ -1,7 +1,7 @@
 import Bottle from 'bottlejs';
 import ShortUrlVisits from '../ShortUrlVisits';
 import { cancelGetShortUrlVisits, getShortUrlVisits } from '../reducers/shortUrlVisits';
-import { getShortUrlDetail } from '../reducers/shortUrlDetail';
+import { getShortUrlDetail } from '../../short-urls/reducers/shortUrlDetail';
 import MapModal from '../helpers/MapModal';
 import { createNewVisits } from '../reducers/visitCreation';
 import TagVisits from '../TagVisits';


Test patch:
diff --git a/test/visits/reducers/shortUrlDetail.test.ts b/test/short-urls/reducers/shortUrlDetail.test.ts
similarity index 62%
rename from test/visits/reducers/shortUrlDetail.test.ts
rename to test/short-urls/reducers/shortUrlDetail.test.ts
index 02d5bf39..a83d1a4d 100644
--- a/test/visits/reducers/shortUrlDetail.test.ts
+++ b/test/short-urls/reducers/shortUrlDetail.test.ts
@@ -5,12 +5,15 @@ import reducer, {
   GET_SHORT_URL_DETAIL_ERROR,
   GET_SHORT_URL_DETAIL,
   ShortUrlDetailAction,
-} from '../../../src/visits/reducers/shortUrlDetail';
+} from '../../../src/short-urls/reducers/shortUrlDetail';
 import { ShortUrl } from '../../../src/short-urls/data';
 import ShlinkApiClient from '../../../src/api/services/ShlinkApiClient';
 import { ShlinkState } from '../../../src/container/types';
+import { ShortUrlsList } from '../../../src/short-urls/reducers/shortUrlsList';
 
 describe('shortUrlDetailReducer', () => {
+  beforeEach(jest.clearAllMocks);
+
   describe('reducer', () => {
     const action = (type: string) => Mock.of<ShortUrlDetailAction>({ type });
 
@@ -45,14 +48,12 @@ describe('shortUrlDetailReducer', () => {
       getShortUrl: jest.fn(async () => returned),
     });
     const dispatchMock = jest.fn();
-    const getState = () => Mock.of<ShlinkState>();
-
-    beforeEach(() => dispatchMock.mockReset());
+    const buildGetState = (shortUrlsList?: ShortUrlsList) => () => Mock.of<ShlinkState>({ shortUrlsList });
 
     it('dispatches start and error when promise is rejected', async () => {
       const ShlinkApiClient = buildApiClientMock(Promise.reject());
 
-      await getShortUrlDetail(() => ShlinkApiClient)('abc123', '')(dispatchMock, getState);
+      await getShortUrlDetail(() => ShlinkApiClient)('abc123', '')(dispatchMock, buildGetState());
 
       expect(dispatchMock).toHaveBeenCalledTimes(2);
       expect(dispatchMock).toHaveBeenNthCalledWith(1, { type: GET_SHORT_URL_DETAIL_START });
@@ -60,16 +61,50 @@ describe('shortUrlDetailReducer', () => {
       expect(ShlinkApiClient.getShortUrl).toHaveBeenCalledTimes(1);
     });
 
-    it('dispatches start and success when promise is resolved', async () => {
-      const resolvedShortUrl = Mock.of<ShortUrl>({ longUrl: 'foo', shortCode: 'bar' });
+    it.each([
+      [ undefined ],
+      [ Mock.all<ShortUrlsList>() ],
+      [
+        Mock.of<ShortUrlsList>({
+          shortUrls: { data: [] },
+        }),
+      ],
+      [
+        Mock.of<ShortUrlsList>({
+          shortUrls: {
+            data: [ Mock.of<ShortUrl>({ shortCode: 'this_will_not_match' }) ],
+          },
+        }),
+      ],
+    ])('performs API call when short URL is not found in local state', async (shortUrlsList?: ShortUrlsList) => {
+      const resolvedShortUrl = Mock.of<ShortUrl>({ longUrl: 'foo', shortCode: 'abc123' });
       const ShlinkApiClient = buildApiClientMock(Promise.resolve(resolvedShortUrl));
 
-      await getShortUrlDetail(() => ShlinkApiClient)('abc123', '')(dispatchMock, getState);
+      await getShortUrlDetail(() => ShlinkApiClient)('abc123', '')(dispatchMock, buildGetState(shortUrlsList));
 
       expect(dispatchMock).toHaveBeenCalledTimes(2);
       expect(dispatchMock).toHaveBeenNthCalledWith(1, { type: GET_SHORT_URL_DETAIL_START });
       expect(dispatchMock).toHaveBeenNthCalledWith(2, { type: GET_SHORT_URL_DETAIL, shortUrl: resolvedShortUrl });
       expect(ShlinkApiClient.getShortUrl).toHaveBeenCalledTimes(1);
     });
+
+    it('avoids API calls when short URL is found in local state', async () => {
+      const foundShortUrl = Mock.of<ShortUrl>({ longUrl: 'foo', shortCode: 'abc123' });
+      const ShlinkApiClient = buildApiClientMock(Promise.resolve(Mock.all<ShortUrl>()));
+
+      await getShortUrlDetail(() => ShlinkApiClient)(foundShortUrl.shortCode, foundShortUrl.domain)(
+        dispatchMock,
+        buildGetState(Mock.of<ShortUrlsList>({
+          shortUrls: {
+            data: [ foundShortUrl ],
+          },
+        })),
+      );
+
+      expect(dispatchMock).toHaveBeenCalledTimes(2);
+      expect(dispatchMock).toHaveBeenNthCalledWith(1, { type: GET_SHORT_URL_DETAIL_START });
+      expect(dispatchMock).toHaveBeenNthCalledWith(2, { type: GET_SHORT_URL_DETAIL, shortUrl: foundShortUrl });
+      expect(ShlinkApiClient.getShortUrl).not.toHaveBeenCalled();
+    });
   });
 });
diff --git a/test/visits/ShortUrlVisits.test.tsx b/test/visits/ShortUrlVisits.test.tsx
index 225c2711..2a3be38e 100644
--- a/test/visits/ShortUrlVisits.test.tsx
+++ b/test/visits/ShortUrlVisits.test.tsx
@@ -6,7 +6,7 @@ import { match } from 'react-router'; // eslint-disable-line @typescript-eslint/
 import ShortUrlVisits, { ShortUrlVisitsProps } from '../../src/visits/ShortUrlVisits';
 import ShortUrlVisitsHeader from '../../src/visits/ShortUrlVisitsHeader';
 import { ShortUrlVisits as ShortUrlVisitsState } from '../../src/visits/reducers/shortUrlVisits';
-import { ShortUrlDetail } from '../../src/visits/reducers/shortUrlDetail';
+import { ShortUrlDetail } from '../../src/short-urls/reducers/shortUrlDetail';
 import VisitsStats from '../../src/visits/VisitsStats';
 import { MercureBoundProps } from '../../src/mercure/helpers/boundToMercureHub';
 
diff --git a/test/visits/ShortUrlVisitsHeader.test.tsx b/test/visits/ShortUrlVisitsHeader.test.tsx
index c2701eba..e6d4c43a 100644
--- a/test/visits/ShortUrlVisitsHeader.test.tsx
+++ b/test/visits/ShortUrlVisitsHeader.test.tsx
@@ -3,7 +3,7 @@ import Moment from 'react-moment';
 import { ExternalLink } from 'react-external-link';
 import { Mock } from 'ts-mockery';
 import ShortUrlVisitsHeader from '../../src/visits/ShortUrlVisitsHeader';
-import { ShortUrlDetail } from '../../src/visits/reducers/shortUrlDetail';
+import { ShortUrlDetail } from '../../src/short-urls/reducers/shortUrlDetail';
 import { ShortUrlVisits } from '../../src/visits/reducers/shortUrlVisits';
 
 describe('<ShortUrlVisitsHeader />', () => {