RecommendedContent: fix ad-words filtering (#1007)

## Issues
Ad-filtering:
- Filtering was done whether or not ads are injected.
- Constants are repeatedly calculated.
- No short-circuiting in the for-loop.
- No memoization (being called 5-6 times on average due to redux updates, can't avoid that).

Others:
- Potentially passing null claimID to recsys (I think this is the issue that Johnny reported in Slack).

## Changes
- Moved 1-time calculations outside of the function.
- Optimized the filtering function and memoize it.
- Reduce unnecessary props since we are passing the whole `Claim` object already.
- Fix recsys being called when claim is not resolved yet (null claimId).
- Move away from the incorrect `makeSelect*` selectors.
This commit is contained in:
infinite-persistence 2022-03-02 07:10:29 -08:00 committed by GitHub
parent 712e02db16
commit 5098b7cd87
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 29 additions and 48 deletions

View file

@ -1,25 +1,20 @@
import { connect } from 'react-redux';
import { makeSelectClaimForUri, makeSelectMetadataForUri } from 'redux/selectors/claims';
import { selectClaimForUri } from 'redux/selectors/claims';
import { doFetchRecommendedContent } from 'redux/actions/search';
import { selectRecommendedContentForUri, selectIsSearching } from 'redux/selectors/search';
import { selectUserVerifiedEmail } from 'redux/selectors/user';
import RecommendedContent from './view';
const select = (state, props) => {
const claim = makeSelectClaimForUri(props.uri)(state);
const { claim_id: claimId } = claim;
const recommendedContentUris = selectRecommendedContentForUri(state, props.uri);
const nextRecommendedUri = recommendedContentUris && recommendedContentUris[0];
const metadata = makeSelectMetadataForUri(props.uri)(state);
return {
claim,
claimId,
claim: selectClaimForUri(state, props.uri),
recommendedContentUris,
nextRecommendedUri,
isSearching: selectIsSearching(state),
isAuthenticated: selectUserVerifiedEmail(state),
metadata,
};
};

View file

@ -10,9 +10,12 @@ import { useIsMobile, useIsMediumScreen } from 'effects/use-screensize';
import Button from 'component/button';
import classnames from 'classnames';
import RecSys from 'recsys';
import { getClaimMetadata } from 'util/claim';
const VIEW_ALL_RELATED = 'view_all_related';
const VIEW_MORE_FROM = 'view_more_from';
const BLOCKED_WORDS: ?Array<string> = AD_KEYWORD_BLOCKLIST && AD_KEYWORD_BLOCKLIST.toLowerCase().split(',');
const CHECK_DESCRIPTION: boolean = AD_KEYWORD_BLOCKLIST_CHECK_DESCRIPTION === 'true';
type Props = {
uri: string,
@ -22,8 +25,6 @@ type Props = {
doFetchRecommendedContent: (string) => void,
isAuthenticated: boolean,
claim: ?StreamClaim,
claimId: string,
metadata: any,
};
export default React.memo<Props>(function RecommendedContent(props: Props) {
@ -35,47 +36,33 @@ export default React.memo<Props>(function RecommendedContent(props: Props) {
isSearching,
isAuthenticated,
claim,
claimId,
metadata,
} = props;
let { description, title } = metadata;
const claimId: ?string = claim && claim.claim_id;
const injectAds = SHOW_ADS && IS_WEB && !isAuthenticated;
if (description) {
description = description.toLowerCase();
}
function claimContainsBlockedWords(claim: ?StreamClaim) {
if (BLOCKED_WORDS) {
const hasBlockedWords = (str) => BLOCKED_WORDS.some((bw) => str.includes(bw));
const metadata = getClaimMetadata(claim);
// $FlowFixMe - flow does not support chaining yet, but we know for sure these fields are '?string'.
const title = metadata?.title?.toLowerCase();
// $FlowFixMe
const description = metadata?.description?.toLowerCase();
// $FlowFixMe
const name = claim?.name?.toLowerCase();
if (title) {
title = title.toLowerCase();
}
let claimNameToCheckAgainst;
if (claim) {
claimNameToCheckAgainst = claim.name && claim.name.toLowerCase();
}
const checkDescriptionForBlacklistWords = AD_KEYWORD_BLOCKLIST_CHECK_DESCRIPTION === 'true';
let triggerBlacklist = false;
if (AD_KEYWORD_BLOCKLIST) {
const termsToCheck = AD_KEYWORD_BLOCKLIST.toLowerCase().split(',');
// eslint-disable-next-line no-unused-vars
if (title) {
for (const term of termsToCheck) {
if (claimNameToCheckAgainst && claimNameToCheckAgainst.includes(term)) {
triggerBlacklist = true;
}
if (title.includes(term)) {
triggerBlacklist = true;
}
if (description && checkDescriptionForBlacklistWords && description.includes(term)) {
triggerBlacklist = true;
}
}
return Boolean(
(title && hasBlockedWords(title)) ||
(name && hasBlockedWords(name)) ||
(CHECK_DESCRIPTION && description && hasBlockedWords(description))
);
}
return false;
}
const triggerBlacklist = React.useMemo(() => injectAds && claimContainsBlockedWords(claim), [injectAds, claim]);
const [viewMode, setViewMode] = React.useState(VIEW_ALL_RELATED);
const signingChannel = claim && claim.signing_channel;
const channelName = signingChannel ? signingChannel.name : null;
@ -90,6 +77,7 @@ export default React.memo<Props>(function RecommendedContent(props: Props) {
React.useEffect(() => {
// Right now we only want to record the recs if they actually saw them.
if (
claimId &&
recommendedContentUris &&
recommendedContentUris.length &&
nextRecommendedUri &&
@ -145,11 +133,9 @@ export default React.memo<Props>(function RecommendedContent(props: Props) {
loading={isSearching}
uris={recommendedContentUris}
hideMenu={isMobile}
injectedItem={
SHOW_ADS &&
IS_WEB &&
!isAuthenticated && <Ads small type={'video'} triggerBlacklist={triggerBlacklist} />
}
// TODO: Since 'triggerBlacklist' is handled by clients of <Ads> instead of internally by <Ads>, we don't
// need that parameter and can just not mount it when 'true', instead of mount-then-hide.
injectedItem={injectAds && <Ads small type={'video'} triggerBlacklist={triggerBlacklist} />}
empty={__('No related content found')}
onClick={handleRecommendationClicked}
/>