Skip to content

Enhance message picker & fire new event for banner:sign-in-gate #15380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cemms1 wants to merge 10 commits into
base: main
Choose a base branch
from
+99 −62
Open

Enhance message picker & fire new event for banner:sign-in-gate #15380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
cff5fd6
WIP - clearer messagePicker result
tomrf1 Feb 17, 2026
d2e75f7
fix tests
tomrf1 Feb 17, 2026
90991ec
fix epic
tomrf1 Feb 17, 2026
084b531
tidy up + add logging to view picked message in browser
cemms1 Feb 17, 2026
89b4eaa
fix types
cemms1 Feb 17, 2026
df188e3
temporarily skip failing tests
cemms1 Feb 17, 2026
d3bb64b
add additional metadata to PickMessageResult so we can fire banner:no…
cemms1 Feb 18, 2026
f52caa6
fire separate event for the sign in gate + update comment explaining …
cemms1 Feb 19, 2026
5a059da
update tests for messagePicker & remove console logs from the functio…
cemms1 Feb 19, 2026
4f5b364
return NoMessageSelected if errors in result handling
cemms1 Feb 19, 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
18 changes: 10 additions & 8 deletions dotcom-rendering/src/components/SlotBodyEnd.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,7 @@ import type {
import type { EpicProps } from '@guardian/support-dotcom-components/dist/shared/types';
import { useEffect, useState } from 'react';
import { getArticleCounts } from '../lib/articleCount';
import type {
CandidateConfig,
MaybeFC,
SlotConfig,
} from '../lib/messagePicker';
import type { CandidateConfig, SlotConfig } from '../lib/messagePicker';
import { pickMessage } from '../lib/messagePicker';
import { useIsSignedIn } from '../lib/useAuthStatus';
import { useBraze } from '../lib/useBraze';
Expand Down Expand Up @@ -60,7 +56,7 @@ const buildReaderRevenueEpicConfig = (
candidate: {
id: 'reader-revenue-banner',
canShow: () => canShowReaderRevenueEpic(canShowData),
show: (data: ModuleData<EpicProps>) => () => {
show: (data: ModuleData<EpicProps>) => {
return <ReaderRevenueEpic {...data} />;
},
},
Expand Down Expand Up @@ -88,7 +84,7 @@ const buildBrazeEpicConfig = (
tags,
shouldHideReaderRevenue,
),
show: (meta: any) => () => (
show: (meta: any) => (
<MaybeBrazeEpic
meta={meta}
countryCode={countryCode}
Expand Down Expand Up @@ -183,7 +179,13 @@ export const SlotBodyEnd = ({
name: 'slotBodyEnd',
};
pickMessage(epicConfig, renderingTarget)
.then((PickedEpic: () => MaybeFC) => setSelectedEpic(PickedEpic))
.then((result) => {
if (result.type === 'MessageSelected') {
setSelectedEpic(result.SelectedMessage);
} else {
setSelectedEpic(() => null);
}
})
Comment on lines +182 to +188
Copy link
Contributor

@dskamiotis dskamiotis Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is a really sensible approach, and it makes the process much clearer 👍

It might be helpful to add a link to Tom's PR in the description for context, so others can easily reference the related implementation.

.catch((e) =>
console.error(`SlotBodyEnd pickMessage - error: ${String(e)}`),
);
Expand Down
49 changes: 29 additions & 20 deletions dotcom-rendering/src/components/StickyBottomBanner.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import type { ArticleCounts } from '../lib/articleCount';
import { getArticleCounts } from '../lib/articleCount';
import type {
CandidateConfig,
MaybeFC,
PickMessageResult,
SlotConfig,
} from '../lib/messagePicker';
import { pickMessage } from '../lib/messagePicker';
Expand Down Expand Up @@ -170,9 +170,9 @@ const buildRRBannerConfigWith = ({
ophanPageViewId,
pageId,
}),
show:
({ name, props }: ModuleData<BannerProps>) =>
() => <BannerComponent name={name} props={props} />,
show: ({ name, props }: ModuleData<BannerProps>) => (
<BannerComponent name={name} props={props} />
),
},
timeoutMillis: DEFAULT_BANNER_TIMEOUT_MILLIS,
};
Expand All @@ -188,7 +188,7 @@ const buildSignInGateConfig = (
canShow: async () => {
return await canShowSignInGatePortal(canShowProps);
},
show: (meta: AuxiaGateDisplayData) => () => (
show: (meta: AuxiaGateDisplayData) => (
<SignInGatePortal
host={host}
isPaidContent={canShowProps.isPaidContent}
Expand Down Expand Up @@ -226,7 +226,7 @@ const buildBrazeBanner = (
tags,
shouldHideReaderRevenue,
),
show: (meta: BrazeMeta) => () => (
show: (meta: BrazeMeta) => (
<BrazeBanner meta={meta} idApiUrl={idApiUrl} />
),
},
Expand Down Expand Up @@ -274,9 +274,9 @@ export const StickyBottomBanner = ({
'control',
);

const [SelectedBanner, setSelectedBanner] = useState<MaybeFC | null>(null);
const [hasPickMessageCompleted, setHasPickMessageCompleted] =
useState<boolean>(false);
const [pickMessageResult, setPickMessageResult] =
useState<PickMessageResult | null>(null);
Copy link
Contributor

@dskamiotis dskamiotis Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In addition, this is super helpful in addressing the ambiguity around the null value issue.


const [asyncArticleCounts, setAsyncArticleCounts] =
useState<Promise<ArticleCounts | undefined>>();

Expand Down Expand Up @@ -367,9 +367,8 @@ export const StickyBottomBanner = ({
};

pickMessage(bannerConfig, renderingTarget)
.then((PickedBanner: () => MaybeFC) => {
setSelectedBanner(PickedBanner);
setHasPickMessageCompleted(true);
.then((result) => {
setPickMessageResult(result);
})
.catch((e) => {
// Report error to Sentry
Expand All @@ -380,7 +379,6 @@ export const StickyBottomBanner = ({
new Error(msg),
'sticky-bottom-banner',
);
setHasPickMessageCompleted(true);
});
}, [
isSignedIn,
Expand All @@ -406,16 +404,27 @@ export const StickyBottomBanner = ({
isInAuxiaControlGroup,
]);

// Dispatches 'banner:none' event for mobile sticky ad integration (see @guardian/commercial-dev).
// Ensures ads only insert when no banner will be shown.
// hasPickMessageCompleted distinguishes between initial state (not picked yet) and final state (picked nothing).
/**
* Custom events for commercial purposes to avoid the mobile-sticky ad slot clashing with these banners
* Dispatches events when no banner is due to appear and when the sign in gate is the chosen banner
* Please talk to @guardian/commercial-dev before changing this logic
*/
useEffect(() => {
if (hasPickMessageCompleted && SelectedBanner == null) {
if (pickMessageResult?.type === 'NoMessageSelected') {
Copy link
Contributor

@dskamiotis dskamiotis Feb 19, 2026

Choose a reason for hiding this comment

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

I'm assuming this will indirectly prevent the mobile-sticky ad from loading when the CMP banner renders, as it will infer NoMessageSelected.

Copy link
Contributor Author

@cemms1 cemms1 Feb 19, 2026

Choose a reason for hiding this comment

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

Not quite. In the commercial PR we choose to launch the mobile-sticky slot when the banner:none event is received (so when the result.type is NoMessageSelected).
NoMessageSelected means that we could show the mobile-sticky ad.

What happens for the CMP is that the picked message is cmpUi so the messagePicked would return the following:

{
  type: "MessageSelected",
  id: "cmpUi",
  SelectedMessage: () => null, // or whatever actually happens when the CMP launches!
}

This results in no mobile-sticky ad allowed to be shown because we are not firing any of the following events: banner:none, banner:close, banner:sign-in-gate

Copy link
Contributor

@dskamiotis dskamiotis Feb 19, 2026

Choose a reason for hiding this comment

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

Thanks for clarifying, that makes sense now!
So for the CMP case, since the cmpUi message is selected, no banner:none event is fired, which ensures the mobile-sticky ad doesn't load.
This distinction between NoMessageSelected and cmpUi is really helpful to understand the behaviour. Thanks for explaining! 👍

document.dispatchEvent(new CustomEvent('banner:none'));
}
}, [SelectedBanner, hasPickMessageCompleted]);
if (SelectedBanner) {
return <SelectedBanner />;

if (
pickMessageResult?.type === 'MessageSelected' &&
pickMessageResult.messageId === 'sign-in-gate-portal'
) {
document.dispatchEvent(new CustomEvent('banner:sign-in-gate'));
}
}, [pickMessageResult]);

if (pickMessageResult?.type === 'MessageSelected') {
const { SelectedMessage } = pickMessageResult;
return <SelectedMessage />;
}

return null;
Expand Down
62 changes: 37 additions & 25 deletions dotcom-rendering/src/lib/messagePicker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ afterEach(async () => {
describe('pickMessage', () => {
it('resolves with the highest priority message which can show', async () => {
const MockComponent = () => <div />;
const ChosenMockComponent = () => <div />;
const ChosenMockComponent = () => <div id="chosen" />;
const config: SlotConfig = {
name: 'banner',
candidates: [
{
candidate: {
id: 'banner-1',
canShow: () => Promise.resolve({ show: false }),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
},
Expand All @@ -40,7 +40,7 @@ describe('pickMessage', () => {
id: 'banner-2',
canShow: () =>
Promise.resolve({ show: true, meta: undefined }),
show: () => ChosenMockComponent,
show: ChosenMockComponent,
},
timeoutMillis: null,
},
Expand All @@ -49,16 +49,20 @@ describe('pickMessage', () => {
id: 'banner-3',
canShow: () =>
Promise.resolve({ show: true, meta: undefined }),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
},
],
};

const got = await pickMessage(config, 'Web');

expect(got()).toEqual(ChosenMockComponent);
const pickMessageResult = await pickMessage(config, 'Web');
expect(pickMessageResult.type).toEqual('MessageSelected');
if (pickMessageResult.type === 'MessageSelected') {
expect(pickMessageResult.SelectedMessage()).toEqual(
ChosenMockComponent(),
);
}
});

it('resolves with null if no messages can show', async () => {
Expand All @@ -71,29 +75,29 @@ describe('pickMessage', () => {
candidate: {
id: 'banner-1',
canShow: () => Promise.resolve({ show: false }),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
},
{
candidate: {
id: 'banner-2',
canShow: () => Promise.resolve({ show: false }),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
},
],
};

const got = await pickMessage(config, 'Web');
const pickMessageResult = await pickMessage(config, 'Web');
Copy link
Contributor

@dskamiotis dskamiotis Feb 19, 2026

Choose a reason for hiding this comment

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

This is really clear naming and helpful for debugging.


expect(got()).toEqual(null);
expect(pickMessageResult.type).toEqual('NoMessageSelected');
});

it('falls through to a lower priority message when a higher one times out', async () => {
const MockComponent = () => <div />;
const ChosenMockComponent = () => <div />;
const ChosenMockComponent = () => <div id="chosen" />;
const config: SlotConfig = {
name: 'banner',
candidates: [
Expand All @@ -111,7 +115,7 @@ describe('pickMessage', () => {
500,
),
),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: 250,
},
Expand All @@ -120,7 +124,7 @@ describe('pickMessage', () => {
id: 'banner-2',
canShow: () =>
Promise.resolve({ show: true, meta: undefined }),
show: () => ChosenMockComponent,
show: ChosenMockComponent,
},
timeoutMillis: null,
},
Expand All @@ -129,9 +133,14 @@ describe('pickMessage', () => {

const messagePromise = pickMessage(config, 'Web');
jest.advanceTimersByTime(260);
const got = await messagePromise;

expect(got()).toEqual(ChosenMockComponent);
const pickMessageResult = await messagePromise;
expect(pickMessageResult.type).toEqual('MessageSelected');
if (pickMessageResult.type === 'MessageSelected') {
expect(pickMessageResult.SelectedMessage()).toEqual(
ChosenMockComponent(),
);
}
});

it('resolves with null if all messages time out', async () => {
Expand All @@ -155,7 +164,7 @@ describe('pickMessage', () => {
500,
);
}),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: 250,
},
Expand All @@ -173,7 +182,7 @@ describe('pickMessage', () => {
500,
);
}),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: 250,
},
Expand All @@ -184,14 +193,14 @@ describe('pickMessage', () => {
jest.advanceTimersByTime(260);
const got = await messagePromise;

expect(got()).toEqual(null);
expect(got.type).toEqual('NoMessageSelected');

clearTimeout(timer1);
clearTimeout(timer2);
});

it('passes metadata returned by canShow to show', async () => {
const renderComponent = jest.fn(() => () => <div />);
const renderComponent = jest.fn(() => <div />);
const meta = { extra: 'info' };
const config: SlotConfig = {
name: 'banner',
Expand All @@ -212,7 +221,10 @@ describe('pickMessage', () => {
};

const show = await pickMessage(config, 'Web');
show();
expect(show.type).toEqual('MessageSelected');
if (show.type === 'MessageSelected') {
expect(show.SelectedMessage()).toEqual(renderComponent());
}

expect(renderComponent).toHaveBeenCalledWith(meta);
});
Expand All @@ -237,7 +249,7 @@ describe('pickMessage', () => {
300,
);
}),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: 200,
},
Expand All @@ -248,7 +260,7 @@ describe('pickMessage', () => {
jest.advanceTimersByTime(250);
const got = await messagePromise;

expect(got()).toEqual(null);
expect(got.type).toEqual('NoMessageSelected');

expect(ophanRecordSpy).toHaveBeenCalledWith({
component: 'banner-picker-timeout-dcr',
Expand Down Expand Up @@ -278,7 +290,7 @@ describe('pickMessage', () => {
120,
);
}),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
reportTiming: true,
Expand All @@ -297,7 +309,7 @@ describe('pickMessage', () => {
100,
);
}),
show: () => MockComponent,
show: MockComponent,
},
timeoutMillis: null,
},
Expand Down
Loading
Loading