diff --git a/dotcom-rendering/src/web/components/CommercialMetrics.tsx b/dotcom-rendering/src/web/components/CommercialMetrics.tsx index b777d6fe74d..3c39562722d 100644 --- a/dotcom-rendering/src/web/components/CommercialMetrics.tsx +++ b/dotcom-rendering/src/web/components/CommercialMetrics.tsx @@ -1,10 +1,13 @@ import type { ABTest } from '@guardian/ab-core'; import { tests } from '@frontend/web/experiments/ab-tests'; -import { useEffect, useState } from 'react'; import { sendCommercialMetrics } from '@guardian/commercial-core'; +import { useOnce } from '@root/src/web/lib/useOnce'; import { useAB } from '@guardian/ab-react'; import { useDocumentVisibilityState } from '../lib/useDocumentHidden'; +import { useAdBlockInUse } from '../lib/useAdBlockInUse'; +// TODO disallow undefined browserIds by placing conditional in App.tsx +// so that we wait to render this component until browserId is defined. export const CommercialMetrics: React.FC<{ pageViewId: string; browserId: string | undefined; @@ -12,35 +15,28 @@ export const CommercialMetrics: React.FC<{ const ABTestAPI = useAB(); const visibilityState = useDocumentVisibilityState(); - const [sentCommercialMetrics, setSentCommercialMetrics] = useState( - false, - ); - - useEffect(() => { - if (visibilityState !== 'hidden') return; - if (sentCommercialMetrics) return; + const adBlockerInUse = useAdBlockInUse(); + // only send metrics when visibility state changes to hidden; + const isHidden = visibilityState === 'hidden' || undefined + useOnce(() => { const testsToForceMetrics: ABTest[] = []; - const shouldForceMetrics = ABTestAPI.allRunnableTests( - tests, - ).some((test) => - testsToForceMetrics.map((t) => t.id).includes(test.id), + const shouldForceMetrics = ABTestAPI.allRunnableTests(tests).some( + (test) => testsToForceMetrics.map((t) => t.id).includes(test.id), ); const userIsInSamplingGroup = Math.random() <= 1 / 100; const isDev = window.guardian.config.page.isDev || window.location.hostname.includes('localhost'); - if (isDev || shouldForceMetrics || userIsInSamplingGroup) - setSentCommercialMetrics( - sendCommercialMetrics(pageViewId, browserId, Boolean(isDev)), - ); + if (isDev || shouldForceMetrics || userIsInSamplingGroup) { + sendCommercialMetrics(pageViewId, browserId, Boolean(isDev), adBlockerInUse); + } }, [ ABTestAPI, pageViewId, - browserId, - visibilityState, - sentCommercialMetrics, + adBlockerInUse, + isHidden ]); // We don’t render anything diff --git a/dotcom-rendering/src/web/components/WhenAdBlockInUse.tsx b/dotcom-rendering/src/web/components/WhenAdBlockInUse.tsx index 21d0b0e3f74..07ed88f884c 100644 --- a/dotcom-rendering/src/web/components/WhenAdBlockInUse.tsx +++ b/dotcom-rendering/src/web/components/WhenAdBlockInUse.tsx @@ -13,7 +13,7 @@ type Props = { * detection checks and returns a promise. * * Sure, you could just use this custom hook directly but sometimes code is easier - * to read and undertand when conditional rendering is done using composition + * to read and understand when conditional rendering is done using composition * * @param {ReactNode} children - What gets rendered if an ad blocker is in use * */ diff --git a/dotcom-rendering/src/web/lib/useAdBlockInUse.ts b/dotcom-rendering/src/web/lib/useAdBlockInUse.ts index deadb846e20..504250aad4e 100644 --- a/dotcom-rendering/src/web/lib/useAdBlockInUse.ts +++ b/dotcom-rendering/src/web/lib/useAdBlockInUse.ts @@ -12,9 +12,7 @@ export const useAdBlockInUse = () => { useEffect(() => { // eslint-disable-next-line @typescript-eslint/no-floating-promises isAdBlockInUse().then((blockerDetected) => { - // We're using react state here to trigger the rerender, but only - // if a blocker was detected - if (blockerDetected) setIsInUse(true); + setIsInUse(blockerDetected); }); }, []);