KEMBAR78
Core: handling interventions by mkomorski · Pull Request #13818 · prebid/Prebid.js · GitHub
Skip to content

Conversation

mkomorski
Copy link
Collaborator

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Other information

#13560

@github-actions
Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • creative/crossDomain.js (+1 error)

PB_LOCATOR
} from './constants.js';

const observer = new ReportingObserver((reports) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you put this in the renderer(s) instead? this is supposed to be a thin wrapper that just injects the renderer, because updating it is painful; everyone using it would need to update their creatives, and we'd need to replicate the same logic in PUC as well. Renderers on the other hand are just shipped together with prebid.

target.postMessage(JSON.stringify(Object.assign({message: type, adId}, payload)), pubDomain, [channel.port2]);
}

window.sendMessage = sendMessage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to be set on window?

@mkomorski mkomorski requested a review from dgirardi August 30, 2025 10:03
import {ERROR_NO_AD} from './constants.js';

export function render({ad, adUrl, width, height, instl}, {mkFrame}, win) {
export function render({ad, adUrl, width, height, instl}, {mkFrame, sendMessage}, win) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can extract this into some shared file within creative/. The 'browserIntervention' event can also be defined once instead of copied as a string, similar to how other events are imported in creative/constants.js (the oddity of that file importing just to export is to avoid pulling more of prebid core into the creative).


export function render({ad, adUrl, width, height, instl}, {mkFrame}, win) {
export function render({ad, adUrl, width, height, instl}, {mkFrame, sendMessage}, win) {
if (window.ReportingObserver) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

window here is not the window where the creative will be; however, from my testing using the "correct" win.ReportingObserver doesn't work - so I guess this is OK.

This made me realize however that this likely won't work when the GAM creative is not set up to use safeframe (which is quite common). The logic there:

function renderFn(adData) {
if (adData.ad) {
doc.write(adData.ad);
doc.close();
emitAdRenderSucceeded({doc, bid, id: bid.adId});
} else {
getCreativeRenderer(bid)
.then(render => render(adData, {
sendMessage: (type, data) => messageHandler(type, data, bid),
mkFrame: createIframe,
}, doc.defaultView))
.then(
() => emitAdRenderSucceeded({doc, bid, id: bid.adId}),
(e) => {
fail(e?.reason || AD_RENDER_FAILED_REASON.EXCEPTION, e?.message)
e?.stack && logError(e);
}
);
}

doesn't run this renderer at all when the bid provides ad; and runs it using a different window setup otherwise. I'd test this by calling pbjs.renderAd directly (e.g. following https://github.com/prebid/Prebid.js/blob/master/integrationExamples/noadserver/basic_noadserver.html) - we probably need to add the ReportingObserver logic in core as well.

@patmmccann
Copy link
Collaborator

We're very happy to see the onIntervention bidder method included

@coveralls
Copy link
Collaborator

coveralls commented Sep 4, 2025

Pull Request Test Coverage Report for Build 17499038656

Details

  • 6 of 19 (31.58%) changed or added relevant lines in 6 files are covered.
  • 7 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.002%) to 96.251%

Changes Missing Coverage Covered Lines Changed/Added Lines %
creative/renderers/display/renderer.js 2 3 66.67%
creative/renderers/native/renderer.js 1 2 50.0%
src/adapterManager.ts 0 2 0.0%
creative/reporting.js 2 6 33.33%
src/adRendering.ts 0 5 0.0%
Files with Coverage Reduction New Missed Lines %
libraries/browsiUtils/browsiUtils.js 1 87.88%
libraries/smartyadsUtils/getAdUrlByRegion.js 1 64.29%
libraries/timeToFirstBytesUtils/timeToFirstBytesUtils.js 1 83.33%
modules/eskimiBidAdapter.js 1 75.86%
test/spec/modules/teadsBidAdapter_spec.js 1 98.21%
modules/bidglassBidAdapter.js 2 90.38%
Totals Coverage Status
Change from base Build 17306264540: -0.002%
Covered Lines: 196587
Relevant Lines: 204245

💛 - Coveralls

@mkomorski mkomorski requested a review from dgirardi September 8, 2025 07:22
Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

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

idk what I'm doing wrong but I can't get Chrome to intervene using the examples you added. Code LGTM.

@patmmccann patmmccann merged commit 8dc7b92 into master Sep 23, 2025
20 checks passed
@patmmccann patmmccann deleted the mkomorski/feat/intervention branch September 23, 2025 13:51
@mkomorski
Copy link
Collaborator Author

mkomorski commented Sep 23, 2025

idk what I'm doing wrong but I can't get Chrome to intervene using the examples you added. Code LGTM.

No idea. I'm attaching the recording of how it works on my side. The only thing that comes to my mind is that Chrome won’t apply the intervention to the same ad again after refreshing the page if it has been done before. So you could try using incognito mode

Screen.Recording.2025-09-23.at.15.59.43.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants