WS-2284 - Fix lost view events in Optimizely #13858
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { | |
| OptimizelyContext, | ||
| OptimizelyDecideOption, | ||
| } from '@optimizely/react-sdk'; | ||
| import { enums } from '@optimizely/optimizely-sdk'; | ||
| import { RequestContext } from '#contexts/RequestContext'; | ||
| import PageCompleteTracking from './PageCompleteTracking'; | ||
| import ScrollDepthTracking from './ScrollDepthTracking'; | ||
|
|
@@ -16,6 +17,16 @@ type Props = { | |
| trackVisit?: boolean; | ||
| }; | ||
|
|
||
| // Shape expected by the Optimizely decision notification listener for decision events | ||
| type DecisionListener = { | ||
| userId?: string; | ||
| type?: string; | ||
| decisionInfo?: { | ||
| flagKey?: string; | ||
| variationKey?: string; | ||
| }; | ||
| }; | ||
|
|
||
| const OptimizelyPageMetrics = ({ | ||
| trackPageView = false, | ||
| trackPageDepth = false, | ||
|
|
@@ -24,43 +35,119 @@ const OptimizelyPageMetrics = ({ | |
| }: Props) => { | ||
| const { optimizely } = useContext(OptimizelyContext); | ||
| const { isAmp, pageType } = useContext(RequestContext); | ||
| const [isInExperiment, setIsInExperiment] = useState(false); | ||
|
|
||
| const experimentsForPageType = experimentsForPageMetrics.find( | ||
| entry => entry.pageType === pageType, | ||
| )?.activeExperiments; | ||
|
|
||
| const optimizelyExperimentsEnabled = Boolean( | ||
| experimentsForPageType?.length && !isAmp, | ||
| ); | ||
|
|
||
| // on initial load, check if the user is in any relevant experiment and set state accordingly | ||
| useEffect(() => { | ||
| if ( | ||
| !optimizelyExperimentsEnabled || | ||
| !optimizely || | ||
| !experimentsForPageType | ||
| ) { | ||
| setIsInExperiment(false); | ||
| return undefined; | ||
| } | ||
|
Comment on lines
+48
to
+57
There was a problem hiding this comment. The early-exit guard (optimizelyExperimentsEnabled/optimizely/experimentsForPageType checks) and mounted-flag pattern is duplicated across two effects, which makes future changes easy to apply inconsistently. Consider consolidating into a single effect that does the initial decideAll check and attaches the notification listener, returning one shared cleanup. |
||
|
|
||
| let mounted = true; | ||
|
|
||
| optimizely.onReady().then(() => { | ||
|
Contributor
There was a problem hiding this comment. Could this still send a page view too early on first load? We’re marking the user as “in the experiment” from If the listener is meant to be the safeguard, should that callback be the thing that unlocks tracking rather than Maybe worth doing some manual testing if poss to verify
Contributor
There was a problem hiding this comment. I think I'd agree with this point, we should be waiting until the event fires to set |
||
| if (!mounted) return; | ||
|
|
||
| // disable decision event tracking to avoid sending duplicate events for any experiments that the user is bucketed into on page load, since the notification listener will also trigger for those experiments | ||
| const decisions = optimizely.decideAll([ | ||
| OptimizelyDecideOption.DISABLE_DECISION_EVENT, | ||
| ]); | ||
|
|
||
| const userInAnyExperiment = experimentsForPageType.some( | ||
| experimentName => { | ||
| const decision = decisions[experimentName]; | ||
| return Boolean(decision && decision.variationKey !== 'off'); | ||
| }, | ||
| ); | ||
|
|
||
| setIsInExperiment(userInAnyExperiment); | ||
| }); | ||
|
|
||
| return () => { | ||
| mounted = false; | ||
| }; | ||
| }, [optimizelyExperimentsEnabled, optimizely, experimentsForPageType]); | ||
|
|
||
| // Listen for Optimizely decisions after initial load in case the user is bucketed later | ||
| useEffect(() => { | ||
| if ( | ||
| !optimizelyExperimentsEnabled || | ||
| !optimizely || | ||
| !experimentsForPageType | ||
| ) { | ||
| setIsInExperiment(false); | ||
| return undefined; | ||
| } | ||
|
|
||
| let mounted = true; | ||
| let notificationId: number | null = null; | ||
|
|
||
| const attachListener = async () => { | ||
| await optimizely.onReady(); | ||
| if (!mounted) return; | ||
|
|
||
| if ( | ||
| optimizely.notificationCenter && | ||
| typeof optimizely.notificationCenter.addNotificationListener === | ||
| 'function' | ||
|
Comment on lines
+104
to
+105
Contributor
There was a problem hiding this comment. When would this ever not be true? |
||
| ) { | ||
| notificationId = optimizely.notificationCenter.addNotificationListener( | ||
|
Contributor
There was a problem hiding this comment. I'm not sure this is going to work as intended, if we register this notification listener in this useEffect, I don't think it would catch all decisions activations/impressions, because useOptimizelyVariation could still fire before this is registered. We probably need to register this within the optimizely provider, to make sure this is setup before any activations/decisions could occur |
||
| enums.NOTIFICATION_TYPES.DECISION, | ||
| (listener: DecisionListener) => { | ||
| if (!mounted) return; | ||
|
|
||
| const { type, decisionInfo } = listener || {}; | ||
| if (type !== 'flag' || !decisionInfo) return; | ||
|
|
||
| const { flagKey, variationKey } = decisionInfo; | ||
|
|
||
| const isRelevantExperiment = | ||
| typeof flagKey === 'string' && | ||
| experimentsForPageType.includes(flagKey); | ||
|
Comment on lines
+117
to
+119
Contributor
There was a problem hiding this comment. can the |
||
|
|
||
| const isUserBucketedIntoExperiment = | ||
| typeof variationKey === 'string' && variationKey !== 'off'; | ||
|
|
||
| if (isRelevantExperiment && isUserBucketedIntoExperiment) { | ||
| setIsInExperiment(true); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| attachListener(); | ||
|
|
||
| return () => { | ||
| mounted = false; | ||
| // clean up the notification listener on unmount | ||
| if ( | ||
| notificationId !== null && | ||
| optimizely.notificationCenter && | ||
| typeof optimizely.notificationCenter.removeNotificationListener === | ||
| 'function' | ||
| ) { | ||
| optimizely.notificationCenter.removeNotificationListener( | ||
| notificationId, | ||
| ); | ||
| } | ||
| }; | ||
louisearchibald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, [optimizelyExperimentsEnabled, optimizely, experimentsForPageType]); | ||
|
|
||
| // if the user is not in any relevant experiment, do not render the tracking components to avoid sending unintended events | ||
| if (!isInExperiment) { | ||
| return null; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find this control flow a little confusing, I'd prefer the use of
else ifrather than the early return in this case.