Merge pull request #1130 from dis-moi/perf/filter_matching_lists_at_once

Perf/filter matching lists at once
dis-moi · Sep 22, 2021 · 4ae2774 · 4ae2774
2 parents c2ed87c + c0da152
commit 4ae2774
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 121 deletions.
diff --git a/src/app/background/store/sagas/tab.ts b/src/app/background/store/sagas/tab.ts
@@ -26,10 +26,7 @@ import {
   TabAction
 } from 'libs/store/actions';
 import { createErrorAction } from 'libs/store/actions/helpers';
-import {
-  filterContextsMatchingTabContent,
-  MatchingContext
-} from 'libs/domain/matchingContext';
+import { MatchingContext, filterContexts } from 'libs/domain/matchingContext';
 import {
   NoticeWithContributor,
   StatefulNoticeWithContributor,
@@ -46,9 +43,11 @@ import {
   getIgnoredNotices,
   getNoticesToDisplay
 } from 'app/background/store/selectors/prefs';
-import { getContextsMatchingUrl } from 'app/background/store/selectors';
 import { getInstallationDetails } from 'app/background/store/selectors/installationDetails';
-import { isTabAuthorized } from 'app/background/store/selectors/resources';
+import {
+  getMatchingContexts,
+  isTabAuthorized
+} from 'app/background/store/selectors/resources';
 import { getNbSubscriptions } from 'app/background/store/selectors/subscriptions.selectors';
 import serviceMessageSaga from './serviceMessage.saga';
 import sendToTabSaga from './lib/sendToTab.saga';
@@ -83,20 +82,19 @@ export function* tabSaga({ meta: { tab } }: TabAction) {
 export function* matchContextSaga({ meta: { tab } }: MatchContextAction) {
   try {
     yield put(clearServiceMessage(tab));
-    const contextsMatchingUrl = yield select(state =>
-      getContextsMatchingUrl(state)(tab.url)
-    );
 
-    if (contextsMatchingUrl.length >= 1) {
-      const contextsMatchingContent = yield call(
-        filterContextsMatchingTabContent,
-        tab,
-        contextsMatchingUrl
-      );
+    const matchingContexts = yield select(getMatchingContexts);
+
+    const contextsMatchingUrlAndContent: MatchingContext[] = yield call(
+      filterContexts,
+      matchingContexts,
+      tab
+    );
 
-      yield put(contextTriggered(contextsMatchingContent, tab));
+    if (contextsMatchingUrlAndContent.length >= 1) {
+      yield put(contextTriggered(contextsMatchingUrlAndContent, tab));
     } else {
-      yield put(contextNotTriggered(contextsMatchingUrl, tab));
+      yield put(contextNotTriggered(contextsMatchingUrlAndContent, tab));
     }
   } catch (e) {
     yield put(matchContextFailure(e, tab));

diff --git a/src/app/background/store/selectors/index.ts b/src/app/background/store/selectors/index.ts
@@ -1,18 +1,12 @@
 import { PersistedState } from 'redux-persist';
 import { createSelector } from 'reselect';
 import { InstallationDetails } from 'libs/domain/installation';
-import { filterContextsMatchingUrl } from 'libs/domain/matchingContext';
 import { getNotice } from 'libs/domain/notice';
 import { BackgroundState } from 'app/background/store/reducers';
-import { getMatchingContexts } from './resources';
 import { getInstallationDetails } from './installationDetails';
 import { areTosAccepted, getRead } from './prefs';
 import { getNoticesIdsOnTab } from './tabs';
 
-export const getContextsMatchingUrl = (state: BackgroundState) => (
-  url: string
-) => filterContextsMatchingUrl(url, getMatchingContexts(state));
-
 export const isAnUpdate = createSelector(
   getInstallationDetails,
   ({ reason }: InstallationDetails) => reason !== 'install'

diff --git a/src/libs/domain/matchingContext.ts b/src/libs/domain/matchingContext.ts
@@ -37,14 +37,6 @@ export const urlMatchesContext = (
   }
 };
 
-export const filterContextsMatchingUrl = (
-  url: string,
-  matchingContexts: MatchingContext[]
-) =>
-  matchingContexts.filter((context: MatchingContext) =>
-    urlMatchesContext(url, context)
-  );
-
 export const doesTabContentMatchExpression = async (
   tab: Tab,
   xpath: string,
@@ -57,18 +49,27 @@ export const doesTabContentMatchExpression = async (
       ])
     : Promise.resolve(true);
 
-export const filterContextsMatchingTabContent = async (
-  tab: Tab,
-  matchingContexts: MatchingContext[]
-) => {
+export const filterContexts = async (
+  matchingContexts: MatchingContext[],
+  tab: Tab
+): Promise<MatchingContext[]> => {
   const responses = await Promise.all(
-    matchingContexts.map(({ xpath, id }: MatchingContext) => {
-      return (
-        typeof xpath === 'undefined' ||
-        doesTabContentMatchExpression(tab, xpath, id)
-      );
-    })
-  );
+    matchingContexts.map(
+      (matchingContext: MatchingContext): Promise<MatchingContext | false> => {
+        if (urlMatchesContext(tab.url, matchingContext)) {
+          if (!matchingContext.xpath) return Promise.resolve(matchingContext);
 
-  return matchingContexts.filter((_, index) => responses[index]);
+          return doesTabContentMatchExpression(
+            tab,
+            matchingContext.xpath,
+            matchingContext.id
+          ).then((matchesContent: boolean) =>
+            matchesContent ? matchingContext : false
+          );
+        }
+        return Promise.resolve(false);
+      }
+    )
+  );
+  return responses.filter(Boolean) as MatchingContext[];
 };
diff --git a/test/app/lmem/findMatchingOffersAccordingToPreferences.ts b/test/app/lmem/findMatchingOffersAccordingToPreferences.ts
@@ -1,7 +1,7 @@
 import chai from 'chai';
 import {
-  filterContextsMatchingUrl,
-  MatchingContext
+  MatchingContext,
+  urlMatchesContext
 } from 'libs/domain/matchingContext';
 import generateMatchingContext from 'test/fakers/generateMatchingContext';
 
@@ -15,110 +15,84 @@ const offers: MatchingContext[] = [
 const matchingURL = 'https://www.samsung.com/blabla';
 const nonMatchingURL = 'https://soundcloud.com/capt-lovelace/meteo-marine';
 
-describe('filterContextsMatchingUrl', function() {
+describe('urlMatchesContext', function() {
   it('should be case insensitive', () => {
-    const offersWithWeirdCase: MatchingContext[] = [
-      generateMatchingContext({ urlRegex: 's.*' }),
-      generateMatchingContext({ urlRegex: 'SamSung' }),
-      generateMatchingContext({ urlRegex: 'doesNotMatch' })
-    ];
-
-    const matches = filterContextsMatchingUrl(matchingURL, offersWithWeirdCase);
-
-    expect(matches).to.be.an('array');
-    expect(matches).to.be.of.length(2);
-    expect(matches[0]).to.equal(offersWithWeirdCase[0]);
-    expect(matches[1]).to.equal(offersWithWeirdCase[1]);
+    expect(
+      urlMatchesContext(
+        matchingURL,
+        generateMatchingContext({ urlRegex: 's.*' })
+      )
+    ).to.equal(true);
+
+    expect(
+      urlMatchesContext(
+        matchingURL,
+        generateMatchingContext({ urlRegex: 'SamSung' })
+      )
+    ).to.equal(true);
+
+    expect(
+      urlMatchesContext(
+        matchingURL,
+        generateMatchingContext({ urlRegex: 'doesNotMatch' })
+      )
+    ).to.equal(false);
   });
 
   describe('exclusion', () => {
     it('should exclude matching exclusion of otherwise matching url', () => {
-      const offersWithExclusion: MatchingContext[] = [
-        generateMatchingContext({
-          urlRegex: 'samsung',
-          excludeUrlRegex: 'blabla'
-        })
-      ];
-
-      const matches = filterContextsMatchingUrl(
-        matchingURL,
-        offersWithExclusion
-      );
+      const offersWithExclusion: MatchingContext = generateMatchingContext({
+        urlRegex: 'samsung',
+        excludeUrlRegex: 'blabla'
+      });
 
-      expect(matches).to.be.an('array');
-      expect(matches).to.be.of.length(0);
+      const matches = urlMatchesContext(matchingURL, offersWithExclusion);
+
+      expect(matches).to.equal(false);
     });
 
     it('should not exclude non matching exclusion of matching url', () => {
-      const offersWithExclusion = [
-        generateMatchingContext({
-          urlRegex: 'samsung',
-          excludeUrlRegex: 'nono'
-        })
-      ];
-
-      const matches = filterContextsMatchingUrl(
-        matchingURL,
-        offersWithExclusion
-      );
+      const offersWithExclusion = generateMatchingContext({
+        urlRegex: 'samsung',
+        excludeUrlRegex: 'nono'
+      });
 
-      expect(matches).to.be.an('array');
-      expect(matches).to.be.of.length(1);
-      expect(matches[0]).to.equal(offersWithExclusion[0]);
+      const matches = urlMatchesContext(matchingURL, offersWithExclusion);
+
+      expect(matches).to.equal(true);
     });
 
     it('should exclude its matching context if regex is invalid', () => {
-      const offersWithExclusion: MatchingContext[] = [
-        generateMatchingContext({
-          urlRegex: 'samsung',
-          excludeUrlRegex: 'isNasty)'
-        }), // SyntaxError: Invalid RegExp: Unmatched ')',
-        ...offers
-      ];
-
-      const matches = filterContextsMatchingUrl(
-        matchingURL,
-        offersWithExclusion
-      );
+      const offersWithExclusion: MatchingContext = generateMatchingContext({
+        urlRegex: 'samsung',
+        excludeUrlRegex: 'isNasty)'
+      });
 
-      expect(matches).to.be.an('array');
-      expect(matches).to.be.of.length(1);
-      expect(matches[0]).to.equal(offersWithExclusion[1]);
+      const matches = urlMatchesContext(matchingURL, offersWithExclusion);
+
+      expect(matches).to.equal(false);
     });
   });
 
   describe('invalid regex', () => {
-    const nastyOffers = [
-      generateMatchingContext({
-        urlRegex: 'isNasty'
-      })
-    ].concat(offers); // SyntaxError: Invalid RegExp: Unmatched ')'
+    const nastyOffers = generateMatchingContext({
+      urlRegex: 'isNasty)'
+    });
 
     it('should not screw up the matching engine', () => {
-      const matches = filterContextsMatchingUrl(matchingURL, nastyOffers);
-
-      expect(filterContextsMatchingUrl).to.not.throw(SyntaxError);
-
-      expect(matches).to.be.an('array');
-      expect(matches).to.be.of.length(1);
-      expect(matches[0]).to.equal(nastyOffers[1]);
+      expect(urlMatchesContext(matchingURL, nastyOffers)).to.equal(false);
     });
   });
 
   describe('empty prefs', () => {
     it('should match when the url matches an offer', () => {
-      const matching = filterContextsMatchingUrl(matchingURL, offers);
-
-      expect(matching).to.be.an('array');
-      expect(matching).to.be.of.length(1);
-      expect(matching[0]).to.equal(offers[0]);
+      expect(urlMatchesContext(matchingURL, offers[0])).to.equal(true);
+      expect(urlMatchesContext(matchingURL, offers[1])).to.equal(false);
     });
 
     it('should not match when the url does not match any offer', () => {
-      const matching = filterContextsMatchingUrl(nonMatchingURL, offers);
-
-      expect(matching).to.be.an('array');
-      expect(matching).to.be.of.length(0);
+      expect(urlMatchesContext(nonMatchingURL, offers[0])).to.equal(false);
+      expect(urlMatchesContext(nonMatchingURL, offers[1])).to.equal(false);
     });
   });
 });