Skip to content

Add ortb values for new pause ads - #108

Draft
spacanowskiopenx wants to merge 9 commits into
masterfrom
pause-ad-ortb
Draft

spacanowskiopenx wants to merge 9 commits into
masterfrom
pause-ad-ortb

Conversation

@spacanowskiopenx

Copy link
Copy Markdown

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

Add new oRTB signals according to new oRTB format specification

Be sure to test the integration with your adserver using the Hello World sample page. -->

Other information

@github-actions

Copy link
Copy Markdown

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Remove unsupported value 7 and add regression coverage for the new signals.

Review effort: Lite
Findings: 1 Low severity

Open (1)
What changed in this PR

Adds OpenRTB video enum support for pause and screensaver ad signals in the MediaKeys adapter.

Changes:

  • Expands accepted plcmt, playbackmethod, and pos values.
  • Updates video parameter validation.
File Summary
modules/​mediakeysBidAdapter.js Remove unsupported playbackmethod value 7 and add focused request assertions for the new signals.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread modules/mediakeysBidAdapter.js Outdated
Copilot AI and others added 2 commits September 28, 2026 08:19
Co-authored-by: spacanowskiopenx <171801863+spacanowskiopenx@users.noreply.github.com>
Co-authored-by: spacanowskiopenx <171801863+spacanowskiopenx@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Add tests for the new values and update outdated OpenRTB documentation.

Review effort: Lite
Findings: 2 Low severity

Open (2)

Comment thread modules/mediakeysBidAdapter.js Outdated
@github-actions

Copy link
Copy Markdown

This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:

  • Changes to modules/mediakeysBidAdapter.js may need:
    • es.iterator.filter
    • es.iterator.map
    • esnext.iterator.filter
    • esnext.iterator.map

The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The instream example needs to document the configuration required to prevent inline-only VAST bids from being rejected.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread modules/mediakeysBidAdapter.md
…g for inline instream bids'

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@KT-Trez Bartłomiej Wąś (KT-Trez) left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't know what's the policy from this repo, but some changes seem to be not tested (ORTB_VIDEO_PARAMS, createBannerImp, createVideoImp)

rest, code-wise LGTM

Comment thread modules/mediakeysBidAdapter.js Outdated
maxbitrate: value => isInteger(value),
boxingallowed: value => [0, 1].indexOf(value) !== -1,
playbackmethod: value => Array.isArray(value) && value.every(v => [1, 2, 3, 4, 5, 6].indexOf(v) !== -1),
playbackmethod: value => Array.isArray(value) && value.every(v => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11].indexOf(v) !== -1),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't we also want to use 1 >= v && v <= 11? looks quite clean

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

};
}

function addRequestSignals(payload, validBidRequests, bidderRequest) {

@KT-Trez Bartłomiej Wąś (KT-Trez) Oct 19, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can see that this isn't a widely used practice in the repo, but you could type this function using JSDoc:

same for constants inside

example: #108 (comment)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

Comment thread modules/mediakeysBidAdapter.js Outdated
const { gdprConsent, uspConsent } = bidderRequest || {};
const userExt = cleanObj({
consent: gdprConsent?.consentString,
eids: deepAccess(validBidRequests[0], 'userIdAsEids')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is it safe to read only the first element of this array, is rest the same?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, it's safe. Every bid in validBidRequests comes from the same bidder request, and core gives them all the same eids value.

Comment thread modules/mediakeysBidAdapter.js Outdated
return banner;
return {
...sizeTupleToRtbSize([
parseInt(sizes[0][0], 10),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this deep array access looks a bit unreadable, could you provide proper typing for the tuple and then destructure it to a named constants?

ex.:

/**
 * @typedef {[width: number|string, height: number|string]} MediakeysBannerSize
 */

/**
 * @typedef {Object} MediakeysBannerBid
 * @property {{banner: {sizes: Array<MediakeysBannerSize>}}} mediaTypes
 * @property {{adUnit?: string, pos?: number}} [params]
 */

/**
 * Returns an openRtb 2.5 banner object, or undefined when the ad unit has no banner sizes.
 *
 * @param {MediakeysBannerBid} bid Prebid bid object from request
 * @returns {object}
 */
function createBannerImp(bid) {
  // ...

  const [width, height] = sizes[0];

  return {
    ...sizeTupleToRtbSize([parseInt(width, 10), parseInt(height, 10)]),
    // ...
  }
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

Comment thread modules/mediakeysBidAdapter.js Outdated
Comment on lines +425 to +427
const computedParams = Array.isArray(playerSize)
? parseGPTSingleSizeArrayToRtbSize(Array.isArray(playerSize[0]) ? playerSize[0] : playerSize) || {}
: {};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can be split to improve a readability a bit

Suggested change
const computedParams = Array.isArray(playerSize)
? parseGPTSingleSizeArrayToRtbSize(Array.isArray(playerSize[0]) ? playerSize[0] : playerSize) || {}
: {};
const [size] = Array.isArray(playerSize?.[0]) ? playerSize : [playerSize];
const computedParams = parseGPTSingleSizeArrayToRtbSize(size) || {};

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

Comment thread modules/mediakeysBidAdapter.js Outdated
spacanowskiopenx and others added 2 commits October 19, 2026 14:18
Co-authored-by: Bartłomiej Wąś <63346093+KT-Trez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants