WS-2284 - Fix lost view events in Optimizely#13858
WS-2284 - Fix lost view events in Optimizely#13858louisearchibald wants to merge 19 commits intolatestfrom
Conversation
|
|
||
| let mounted = true; | ||
|
|
||
| optimizely.onReady().then(() => { |
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 decideAll here, so tracking can start before the new Optimizely listener has actually fired.
If the listener is meant to be the safeguard, should that callback be the thing that unlocks tracking rather than decideAll?
Maybe worth doing some manual testing if poss to verify
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 isInExperiment I think
There was a problem hiding this comment.
Pull request overview
This PR addresses lost Optimizely page-view events by ensuring tracking in OptimizelyPageMetrics only renders after the user is confirmed to be bucketed into a relevant experiment, including handling late experiment assignment via an Optimizely decision notification listener.
Changes:
- Adds
@optimizely/optimizely-sdkto access Optimizely enums for notification types. - Adds an initial
decideAll-based membership check before rendering tracking components. - Attaches and cleans up a decision notification listener to detect post-load bucketing into relevant experiments.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/app/components/OptimizelyPageMetrics/index.tsx |
Adds initial bucketing check and a decision notification listener to gate tracking rendering until experiment membership is confirmed. |
package.json |
Adds a direct dependency on @optimizely/optimizely-sdk for notification enums. |
yarn.lock |
Updates lockfile to reflect the added Optimizely SDK dependency/version resolution. |
| // 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; | ||
| } |
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.
| expect( | ||
| screen.queryByTestId('page-view-tracking'), | ||
| ).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
I don't think this assertion will be of any value as it will instantly pass even if this element was later added. May be best to remove it 🤔
| ], | ||
| ); | ||
|
|
||
| let decisionListener: NotificationListener<any> | null = null; |
There was a problem hiding this comment.
can this type be addressed or at least explicitly ignored as it's in a test
| !experimentsForPageType | ||
| ) { | ||
| setIsInExperiment(false); | ||
| return undefined; |
There was a problem hiding this comment.
I personally find this control flow a little confusing, I'd prefer the use of else if rather than the early return in this case.
| typeof optimizely.notificationCenter.addNotificationListener === | ||
| 'function' |
There was a problem hiding this comment.
When would this ever not be true?
| const isRelevantExperiment = | ||
| typeof flagKey === 'string' && | ||
| experimentsForPageType.includes(flagKey); |
There was a problem hiding this comment.
can the flagKey ever not be a string in practice?
Resolves JIRA: WS-2284
Summary
Adds a notification listener to our
OptimizelyPageMetricscode to prevent page-view events firing before experiments activate.Code changes
DecisionListenertype.decideAllcheck before rendering tracking.@optimizely/optimizely-sdkas a dependency.Testing