[copilot] refactor OptimizelyPageMetrics notification listener to use… by HarveyPeachey · Pull Request #13893 · bbc/simorgh

HarveyPeachey · 2026-04-09T09:51:36Z

… useSyncExternalStore

Resolves JIRA:

Summary

A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.

Code changes

List key code changes that have been made.

Testing

List the steps required to test this PR.

Useful Links

Coding Standards
Repository use guidelines
Add Links to useful resources related to this PR if applicable.

… useSyncExternalStore

HarveyPeachey · 2026-04-09T11:10:38Z

src/app/components/OptimizelyPageMetrics/index.tsx

-  const [isInExperiment, setIsInExperiment] = useState(false);
+  const activatedExperiments = useActivatedExperiments();

  const experimentsForPageType = experimentsForPageMetrics.find(


I think we can actually remove the reliance on the maintained list, now that we have an external store that informs us which experiments have been activated

Copilot

Pull request overview

Refactors Optimizely Page Metrics experiment-gating to use a shared decision store backed by useSyncExternalStore, with decisions captured via a central Optimizely notification listener.

Changes:

Added optimizelyDecisionStore (subscribe/snapshot/notify + hook) to track activated experiments.
Moved DECISION notification handling to withOptimizelyProvider and forwards activations into the store.
Updated OptimizelyPageMetrics and its tests to rely on the store rather than local effects/listeners.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/app/lib/optimizelyDecisionStore.ts`	New external store + `useSyncExternalStore` hook for activated experiments.
`src/app/lib/optimizelyDecisionStore.test.ts`	Unit tests covering store snapshot, subscriptions, and reset.
`src/app/legacy/containers/PageHandlers/withOptimizelyProvider/index.tsx`	Adds a global DECISION notification listener that calls `notifyDecision`.
`src/app/components/OptimizelyPageMetrics/index.tsx`	Switches gating logic to store-driven activated experiments; removes local listener/decide logic.
`src/app/components/OptimizelyPageMetrics/index.test.tsx`	Updates tests to drive behaviour via decision store notifications.

Copilot · 2026-04-09T14:24:58Z

src/app/lib/optimizelyDecisionStore.ts

+let snapshot: ReadonlySet<string> = new Set<string>();
+const subscribers = new Set<() => void>();
+
+const subscribe = (callback: () => void) => {
+  subscribers.add(callback);
+  return () => {
+    subscribers.delete(callback);
+  };
+};
+
+const getSnapshot = (): ReadonlySet<string> => snapshot;
+
+const notifyDecision = (flagKey: string) => {
+  if (activatedExperiments.has(flagKey)) return;


This store is a module-level singleton (activatedExperiments/snapshot/subscribers). Because withOptimizelyProvider sets isServerSide and this code can be evaluated during SSR, state can leak between server requests/users if notifyDecision is ever called on the server process. Consider making updates client-only (e.g., guard notifyDecision/listener with typeof window !== 'undefined') and/or scoping the store per request rather than per module.

Suggested change

let snapshot: ReadonlySet<string> = new Set<string>();

const subscribers = new Set<() => void>();

const subscribe = (callback: () => void) => {

subscribers.add(callback);

return () => {

subscribers.delete(callback);

};

};

const getSnapshot = (): ReadonlySet<string> => snapshot;

const notifyDecision = (flagKey: string) => {

if (activatedExperiments.has(flagKey)) return;

const emptySnapshot: ReadonlySet<string> = new Set<string>();

let snapshot: ReadonlySet<string> = new Set<string>();

const subscribers = new Set<() => void>();

const isBrowser = typeof window !== 'undefined';

const subscribe = (callback: () => void) => {

if (!isBrowser) {

return () => {};

}

subscribers.add(callback);

return () => {

subscribers.delete(callback);

};

};

const getSnapshot = (): ReadonlySet<string> =>

isBrowser ? snapshot : emptySnapshot;

const notifyDecision = (flagKey: string) => {

if (!isBrowser || activatedExperiments.has(flagKey)) return;

In our setup I don't think this would be the case currently, as decisions that we make using the SDK that send events to the optimizely metrics endpoint are carried out client side. But it's a good point, I guess do we wanna be YAGNI with this?

Imo with this sort of thing we should be aware of the context it runs in and not bloat it with defensive code as it makes it less readable

Copilot · 2026-04-09T14:24:59Z

src/app/lib/optimizelyDecisionStore.ts

+const notifyDecision = (flagKey: string) => {
+  if (activatedExperiments.has(flagKey)) return;
+  activatedExperiments.add(flagKey);
+  snapshot = new Set(activatedExperiments);
+  subscribers.forEach(cb => cb());
+};
+
+const resetDecisionStore = () => {
+  activatedExperiments.clear();
+  snapshot = new Set<string>();


resetDecisionStore clears internal state but doesn’t notify current subscribers. Components using useActivatedExperiments() would keep rendering with a stale snapshot until another decision is notified. If resetDecisionStore is intended as a real reset (not test-only), it should update snapshot and trigger subscriber callbacks (or otherwise document/rename it to clarify test-only usage).

Suggested change

const notifyDecision = (flagKey: string) => {

if (activatedExperiments.has(flagKey)) return;

activatedExperiments.add(flagKey);

snapshot = new Set(activatedExperiments);

subscribers.forEach(cb => cb());

};

const resetDecisionStore = () => {

activatedExperiments.clear();

snapshot = new Set<string>();

const notifySubscribers = () => {

subscribers.forEach(callback => callback());

};

const notifyDecision = (flagKey: string) => {

if (activatedExperiments.has(flagKey)) return;

activatedExperiments.add(flagKey);

snapshot = new Set(activatedExperiments);

notifySubscribers();

};

const resetDecisionStore = () => {

activatedExperiments.clear();

snapshot = new Set<string>();

notifySubscribers();

In our case I don't think this really matters

Do we even need the reset functionality in our case?

Copilot · 2026-04-09T14:24:59Z

src/app/legacy/containers/PageHandlers/withOptimizelyProvider/index.tsx

+optimizely.notificationCenter.addNotificationListener(
+  enums.NOTIFICATION_TYPES.DECISION,
+  (
+    notification: ListenerPayload & {
+      decisionInfo: {
+        flagKey: string;
+        variationKey: string;
+        decisionEventDispatched: boolean;
+      };
+    },
+  ) => {
+    const { flagKey, variationKey, decisionEventDispatched } =
+      notification.decisionInfo;
+
+    if (decisionEventDispatched && variationKey !== 'off') {
+      notifyDecision(flagKey);
+    }


The decision notification handler assumes notification.decisionInfo is always present and destructures it unconditionally. If Optimizely emits other DECISION payload shapes (or decisionInfo is absent), this will throw at runtime and break page rendering. Please add runtime guards (e.g., verify notification?.type/notification?.decisionInfo exists) before reading flagKey/variationKey.

Yeah good point, better to use optional chaining for this just in case, although highly unlikely to happen

Copilot · 2026-04-09T14:24:59Z

src/app/components/OptimizelyPageMetrics/index.tsx

@@ -45,116 +30,16 @@ const OptimizelyPageMetrics = ({
    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;
-    }
-
-    let mounted = true;
-
-    optimizely.onReady().then(() => {
-      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 isInExperiment =
+    optimizelyExperimentsEnabled &&
+    Boolean(
+      experimentsForPageType?.some(name => activatedExperiments.has(name)),
+    );


This refactor removes the initial decideAll(...DISABLE_DECISION_EVENT) check that previously detected experiment membership on first load. isInExperiment now depends entirely on activatedExperiments being populated by a prior decision notification; if no other code evaluates these flags, the tracking components will never mount for already-bucketed users. If the intent is to preserve prior behaviour, consider seeding the store (or local state) by evaluating the relevant flags once on mount.

I don't think we need to preserve prior behaviour as decisions will always be made on a page load via useOptimizelyVariation I think

andrewscfc · 2026-04-09T15:44:58Z

src/app/components/OptimizelyPageMetrics/index.test.tsx

-      expect(
-        screen.queryByTestId('page-complete-tracking'),
-      ).not.toBeInTheDocument();
+  it('should return null when experiment names do not match activated experiments', () => {


Suggested change

it('should return null when experiment names do not match activated experiments', () => {

it('should not include tracking when experiment names do not match activated experiments', () => {

Other test descriptions seem a bit unclear, this is just one example

andrewscfc · 2026-04-09T15:53:34Z

src/app/lib/optimizelyDecisionStore.ts

+const subscribe = (callback: () => void) => {
+  subscribers.add(callback);
+  return () => {
+    subscribers.delete(callback);
+  };
+};


I find this subscription API very unintuitive, is this a common approach?

andrewscfc · 2026-04-09T15:54:27Z

src/app/lib/optimizelyDecisionStore.ts

+let snapshot: ReadonlySet<string> = new Set<string>();
+const subscribers = new Set<() => void>();
+
+const subscribe = (callback: () => void) => {
+  subscribers.add(callback);
+  return () => {
+    subscribers.delete(callback);
+  };
+};
+
+const getSnapshot = (): ReadonlySet<string> => snapshot;
+
+const notifyDecision = (flagKey: string) => {
+  if (activatedExperiments.has(flagKey)) return;


Imo with this sort of thing we should be aware of the context it runs in and not bloat it with defensive code as it makes it less readable

andrewscfc · 2026-04-09T15:54:54Z

src/app/lib/optimizelyDecisionStore.ts

+const notifyDecision = (flagKey: string) => {
+  if (activatedExperiments.has(flagKey)) return;
+  activatedExperiments.add(flagKey);
+  snapshot = new Set(activatedExperiments);
+  subscribers.forEach(cb => cb());
+};
+
+const resetDecisionStore = () => {
+  activatedExperiments.clear();
+  snapshot = new Set<string>();


Do we even need the reset functionality in our case?

[copilot] refactor OptimizelyPageMetrics notification listener to use…

086ecc4

… useSyncExternalStore

HarveyPeachey commented Apr 9, 2026

View reviewed changes

HarveyPeachey marked this pull request as ready for review April 9, 2026 14:19

Copilot AI review requested due to automatic review settings April 9, 2026 14:19

Copilot started reviewing on behalf of HarveyPeachey April 9, 2026 14:20 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

HarveyPeachey marked this pull request as draft April 9, 2026 14:26

andrewscfc reviewed Apr 9, 2026

View reviewed changes

	it('should return null when experiment names do not match activated experiments', () => {
	it('should not include tracking when experiment names do not match activated experiments', () => {

Conversation

HarveyPeachey commented Apr 9, 2026

Summary

Code changes

Testing

Useful Links

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants