Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8ee986f
Merge branch 'latest' of ssh://github.com/bbc/simorgh into latest
louisearchibald Mar 18, 2026
37a69f6
Merge branch 'latest' of ssh://github.com/bbc/simorgh into latest
louisearchibald Mar 26, 2026
6165b2c
Merge branch 'latest' of ssh://github.com/bbc/simorgh into latest
louisearchibald Mar 31, 2026
ac8ab21
add notification listener to optimizely page metrics file
louisearchibald Mar 31, 2026
bfc3927
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
louisearchibald Apr 7, 2026
c9e02c1
Merge branch 'WS-2284-fix-lost-view-events-in-optimizely' of ssh://gi…
louisearchibald Apr 7, 2026
842a8b4
add optimizely-sdk as a dependency
louisearchibald Apr 7, 2026
3c90e5c
Revert "add optimizely-sdk as a dependency"
louisearchibald Apr 7, 2026
66c9b86
add optimizely-sdk as a dependency
louisearchibald Apr 7, 2026
7fde059
change version
louisearchibald Apr 7, 2026
9d72a6d
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
LilyL0u Apr 7, 2026
c7f6a58
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
louisearchibald Apr 8, 2026
284abce
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
louisearchibald Apr 8, 2026
d93ce11
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
louisearchibald Apr 8, 2026
797d85c
Merge branch 'WS-2284-fix-lost-view-events-in-optimizely' of ssh://gi…
louisearchibald Apr 8, 2026
412040c
Merge branch 'latest' into WS-2284-fix-lost-view-events-in-optimizely
louisearchibald Apr 8, 2026
4c4291d
adds unit tests to test new behaviour
louisearchibald Apr 8, 2026
762c4d7
Merge branch 'WS-2284-fix-lost-view-events-in-optimizely' of ssh://gi…
louisearchibald Apr 8, 2026
06074e2
fix import
louisearchibald Apr 8, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
"@emotion/styled": "11.14.1",
"@loadable/component": "5.16.7",
"@loadable/server": "5.16.7",
"@optimizely/optimizely-sdk": "5.4.1",
"@optimizely/react-sdk": "3.3.1",
"aws-embedded-metrics": "4.2.0",
"compression": "1.8.1",
Expand Down
105 changes: 104 additions & 1 deletion src/app/components/OptimizelyPageMetrics/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { PropsWithChildren } from 'react';
import { act, PropsWithChildren } from 'react';
import { screen, waitFor } from '@testing-library/react';
import {
OptimizelyDecision,
Expand All @@ -8,6 +8,7 @@
import { RequestContextProvider } from '#app/contexts/RequestContext';
import { PageTypes, Services } from '#app/models/types/global';
import { ARTICLE_PAGE, HOME_PAGE } from '#app/routes/utils/pageTypes';
import { NotificationListener } from '@optimizely/optimizely-sdk';
import { render } from '../react-testing-library-with-providers';
import OptimizelyPageMetrics from '.';
import experimentsForPageMetrics from './experimentsForPageMetrics';
Expand Down Expand Up @@ -411,4 +412,106 @@
});
});
});
describe('Notification listener', () => {
it('should mount trackers when user is bucketed after load', async () => {
experimentsForPageMetrics.push(
...[
{
pageType: ARTICLE_PAGE,
activeExperiments: ['mockExperiment1'],
},
],
);

let decisionListener: NotificationListener<any> | null = null;

Check warning on line 426 in src/app/components/OptimizelyPageMetrics/index.test.tsx

View workflow job for this annotation

GitHub Actions / build (22.x)

Unexpected any. Specify a different type
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this type be addressed or at least explicitly ignored as it's in a test


const mockOptimizely = {
...optimizely,
decideAll: jest.fn(() => ({
mockExperiment1: { variationKey: 'off' } as OptimizelyDecision,
})),
notificationCenter: {
addNotificationListener: jest.fn((_, callback) => {
decisionListener = callback;
return 1;
}),
removeNotificationListener: jest.fn(),
clearNotificationListeners: jest.fn(),
clearAllNotificationListeners: jest.fn(),
},
} satisfies Partial<ReactSDKClient>;

render(
<ContextWrap
pageType={ARTICLE_PAGE}
service="news"
mockOptimizely={mockOptimizely}
>
<OptimizelyPageMetrics trackPageView />
</ContextWrap>,
);

await waitFor(() => {
expect(
screen.queryByTestId('page-view-tracking'),
).not.toBeInTheDocument();
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔


act(() => {
decisionListener?.({
type: 'flag',
decisionInfo: {
flagKey: 'mockExperiment1',
variationKey: 'variation_1',
},
});
});

await waitFor(() => {
expect(screen.getByTestId('page-view-tracking')).toBeInTheDocument();
});
});

it('should remove the decision listener on unmount', async () => {
experimentsForPageMetrics.push(
...[
{
pageType: ARTICLE_PAGE,
activeExperiments: ['mockExperiment1'],
},
],
);

const addNotificationListener = jest.fn(() => 1);
const removeNotificationListener = jest.fn();

const mockOptimizely = {
...optimizely,
notificationCenter: {
addNotificationListener,
removeNotificationListener,
clearNotificationListeners: jest.fn(),
clearAllNotificationListeners: jest.fn(),
},
} satisfies Partial<ReactSDKClient>;

const { unmount } = render(
<ContextWrap
pageType={ARTICLE_PAGE}
service="news"
mockOptimizely={mockOptimizely}
>
<OptimizelyPageMetrics trackPageView />
</ContextWrap>,
);

await waitFor(() => {
expect(addNotificationListener).toHaveBeenCalled();
});

unmount();

expect(removeNotificationListener).toHaveBeenCalledWith(1);
});
});
});
139 changes: 113 additions & 26 deletions src/app/components/OptimizelyPageMetrics/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand All @@ -24,43 +35,119 @@ const OptimizelyPageMetrics = ({
}: Props) => {
const { optimizely } = useContext(OptimizelyContext);
const { isAmp, pageType } = useContext(RequestContext);
const [isInExperiment, setisInExperiment] = useState(false);
const [isInExperiment, setIsInExperiment] = useState(false);

const experimentsForPageType = experimentsForPageMetrics.find(
entry => entry.pageType === pageType,
)?.activeExperiments;

const optimizelyExperimentsEnabled =
experimentsForPageType && !isAmp && !isInExperiment;
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;
Copy link
Copy Markdown
Contributor

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 if rather than the early return in this case.

}
Comment on lines +48 to +57
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

let mounted = true;

optimizely.onReady().then(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd agree with this point, we should be waiting until the event fires to set isInExperiment I think

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?.onReady().then(() => {
const decisions = optimizely.decideAll([
OptimizelyDecideOption.DISABLE_DECISION_EVENT,
]);
const isUserInAnyExperiments = experimentsForPageType.some(
experimentName => {
const decision = decisions[experimentName];
return decision && decision.variationKey !== 'off';
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this ever not be true?

) {
notificationId = optimizely.notificationCenter.addNotificationListener(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can the flagKey ever not be a string in practice?


const isUserBucketedIntoExperiment =
typeof variationKey === 'string' && variationKey !== 'off';

if (isRelevantExperiment && isUserBucketedIntoExperiment) {
setIsInExperiment(true);
}
},
);
}
};

if (isUserInAnyExperiments) {
setisInExperiment(true);
}
});
}
}, [
optimizelyExperimentsEnabled,
optimizely,
trackPageComplete,
trackPageDepth,
trackPageView,
trackVisit,
experimentsForPageType,
]);
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,
);
}
};
}, [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;
}
Expand Down
3 changes: 2 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4861,7 +4861,7 @@ __metadata:
languageName: node
linkType: hard

"@optimizely/optimizely-sdk@npm:^5.4.1":
"@optimizely/optimizely-sdk@npm:5.4.1, @optimizely/optimizely-sdk@npm:^5.4.1":
version: 5.4.1
resolution: "@optimizely/optimizely-sdk@npm:5.4.1"
dependencies:
Expand Down Expand Up @@ -17587,6 +17587,7 @@ __metadata:
"@loadable/component": "npm:5.16.7"
"@loadable/server": "npm:5.16.7"
"@loadable/webpack-plugin": "npm:5.15.2"
"@optimizely/optimizely-sdk": "npm:5.4.1"
"@optimizely/react-sdk": "npm:3.3.1"
"@storybook/addon-a11y": "npm:10.3.3"
"@storybook/addon-docs": "npm:10.3.3"
Expand Down
Loading