Select active GPT responsive slots - #978
Conversation
ChristianPavilonis
changed the title
4d95acc to
340d1ef
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Replaces first-match-in-document-order prefix resolution with "unique active prefix match", failing closed on ambiguity, and mirrors it across the edge bootstrap and the bundle with a shared test table. The direction is right and the parity coverage is good. One blocking issue: the resolver requires geometry before it will pick a sibling, so the single visible responsive sibling is still skipped when it reserves no space — and unlike before this PR, nothing is defined or requested at all in that case.
Blocking
🔧 wrench
-
Geometry gate drops the one visible responsive sibling when it reserves no space:
slotElementHasLayoutrequireswidth > 0 && height > 0on the element or its-container. An empty ad div awaiting its creative commonly has height 0, and its empty wrapper does too, so the active sibling fails the check andactiveMatches.length === 0→null→ the slot is skipped entirely (nodefineSlot, no ad request). Before this PR the first prefix match was defined and requested. Verified against both implementations with a throwaway probe (threedisplay:nonedupes plus one visible sibling, default 0×0 rects):GPT slot prefix did not resolve to one active element { divId: 'ad-probe-', prefixMatchCount: 4, activeMatchCount: 0 } → defineSlot not called, divToSlotId {}Adding a visibility-only tier before the geometry tier resolves the
not-size-a/b/c/dpattern this PR targets without requiring reserved height. Details inline ongpt_bootstrap.js:121andgpt/index.ts:85.
Non-blocking
🤔 thinking
- Forced synchronous layout inside the SPA MutationObserver (
crates/trusted-server-js/lib/src/integrations/gpt/index.ts:897):waitForSlotElementscallsallPresent()on every mutation batch, andfindSlotElementByDivIdnow runsgetComputedStyle+getBoundingClientRectper prefix candidate whenever a prefix has more than one match. During framework hydration that is a forced reflow per batch. The presence check only needs existence, not activeness — consider a geometry-free probe there (or a resolver flag that stops at the prefix-match tier). - Ambiguity warn never fires from the edge bootstrap — see inline on
gpt_bootstrap.js:129.
🌱 seedling
- Resolver logic now exists twice (edge bootstrap JS and bundle TS), kept aligned by a substring assertion in
gpt.rs. Worth a follow-up to generate the bootstrap from one source — see inline ongpt.rs:1216.
⛏ nitpick
- Test table covers ambiguous, none-active, hidden-sibling and empty-
div_idcases, but not "exactly one visible sibling, nothing laid out" — the case that motivates the blocking finding — and nothing asserts the warn payload (prefixMatchCount/activeMatchCount), which is the only diagnostic for the fail-closed path.
👍 praise
- Empty-
div_idguard and the runtime/bootstrap parity table — see inline ongpt/index.ts:74andad_init.test.ts:1298.
CI Status
Only the Integration Tests workflow ran on 340d1efb4 (4 checks, all pass). format.yml and test.yml trigger on pull_request: branches: [main, "feature/**"], so with this PR based on fix/duplicate-gpt-slots the fmt / clippy / Rust test / JS test gates did not run on CI. Verified locally on the head commit instead:
- fmt: NOT RUN on CI
- clippy: NOT RUN on CI
- rust tests: PASS locally (
cargo test -p trusted-server-core --target aarch64-apple-darwin gpt— 31 passed) - js tests: PASS locally (
npx vitest run— 30 files, 430 passed) - integration tests: PASS on CI
Adopts the gating revert and the hardened GPT slot handoff from #978: - Removes the publisher initial-request gate (initialRequestGate, heldPublisherRequests, GptInitialRequestGate) that #978 reverted - Takes matchingHandoff/displayTargetElementId and the responsive-slot helpers with ambiguous-hydration protection - Keeps rc-only content intact: gpt_diagnostics types, the #948 disableInitialLoad sync (syncInitialLoadDisabled wired into the refresh-selection path), and the #945 scheduleInitialAdInit coverage - Drops the obsolete held-display test from schedule_initial_ad_init and ports the two #948 setConfig tests to the new zero-arg runGptBootstrap harness
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Replaces first-prefix-match GPT slot resolution with a visibility/geometry ladder (exact ID → unique prefix → unique visible → unique laid-out → fail closed), mirrored in the edge bootstrap and runtime bundle with a shared parity test suite. The direction is right and the empty-div_id guard fixes a real pre-existing bug; the open question is whether the geometry gate's reserved-height requirement can drop impressions for publishers that don't reserve slot heights.
Blocking
❓ question
- Fail-closed drops the impression when no candidate has reserved height:
slotElementHasLayoutrequires height > 0 on the element or its-container; empty ad divs withoutmin-heightnever qualify, so container-hidden responsive setups without reserved space go from "first match" to no fill at all (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:70, mirrored in crates/trusted-server-core/src/integrations/gpt_bootstrap.js:98). See inline comment for a width-only alternative.
Non-blocking
🤔 thinking
- Container-hidden siblings count as visible: computed
displaydoesn't inherit, so thevisibleMatches.length === 1shortcut can return an element inside adisplay:nonecontainer (index.ts:86).Element.checkVisibility()would cover this. - Render bridge re-resolves the prefix at message time:
candidateSlotRootscan disagree with the elementadInitchose (or return null on later ambiguity), silently dropping the TS creative and beacons; consider deriving roots fromts.divToSlotId(index.ts:101). - rAF gating in background tabs: hidden tabs never run the observer check, so background SPA navigations always pay the full 2 s
SPA_SLOT_WAIT_MS(index.ts:923).
♻️ refactor
- Ambiguity warn fires per animation frame during the SPA slot wait: up to ~120 duplicate warns per navigation; warn once per divId or only from adInit (index.ts:92).
🏕 camp site
- Reuse
runGptBootstrap()in the pre-existing embedded-bootstrap handoff test that still inlines readFileSync + eval (ad_init.test.ts:516).
⛏ nitpick
appendResponsiveSlotElementfive positional booleans: an options object would make the it.each cases self-describing (ad_init.test.ts:56).
CI Status
- GitHub checks on head commit: integration tests, browser integration tests, Fastly EC lifecycle, prepare artifacts — all PASS. The fmt/clippy/unit-test/JS workflows did not trigger for this stacked base.
- Verified locally: JS tests 432/432 PASS; eslint PASS;
trusted-server-coregpt tests 31/31 PASS on wasm32-wasip1 via Viceroy. (Prettier warn onsrc/core/templates/iframe.htmlis pre-existing on the base branch, untouched here.)
| const container = document.getElementById(`${element.id}-container`); | ||
| if (!container || !isElementVisible(container)) return false; | ||
| const containerRect = container.getBoundingClientRect(); | ||
| return containerRect.width > 0 && containerRect.height > 0; |
There was a problem hiding this comment.
❓ question — Fail-closed drops the impression entirely when no candidate has reserved height.
slotElementHasLayout requires width > 0 && height > 0 on the element or its -container. An empty ad div has height 0 until an ad renders, unless the publisher reserves space with min-height. In the container-based responsive-hiding pattern this PR targets (inactive containers display:none), every inner element still passes the element-visibility check (computed display does not inherit from ancestors), so resolution always falls through to geometry — and with no reserved height, activeMatches is empty and the slot is skipped entirely, where the old code served the first match.
Is total no-fill acceptable for publishers that don't reserve slot heights? A width-only container check would disambiguate without requiring reserved height, since a display:none container has a 0×0 rect while the active one keeps its block width:
const containerRect = container.getBoundingClientRect();
return containerRect.width > 0;Same applies to the mirrored slotElementHasLayout in crates/trusted-server-core/src/integrations/gpt_bootstrap.js.
| if (prefixMatches.length === 1) return prefixMatches[0] ?? null; | ||
|
|
||
| const visibleMatches = prefixMatches.filter(isElementVisible); | ||
| if (visibleMatches.length === 1) return visibleMatches[0] ?? null; |
There was a problem hiding this comment.
🤔 thinking — Element-level visibility can't see container-hidden siblings.
isElementVisible reads the element's own computed style, and display doesn't inherit — an element inside a display:none container still counts as visible. When no sibling is actually active and exactly one is hidden only at container level, this visibleMatches.length === 1 shortcut returns that hidden element instead of failing closed, defining a GPT slot in an invisible subtree (unviewable impression).
element.checkVisibility?.() checks ancestor display for exactly this case and could be layered in with the current heuristic as fallback:
function isElementVisible(element: HTMLElement): boolean {
if (typeof element.checkVisibility === 'function') return element.checkVisibility();
const style = window.getComputedStyle(element);
// existing checks…
}| if (activeMatches.length === 1) return activeMatches[0] ?? null; | ||
|
|
||
| if (prefixMatches.length > 1) { | ||
| log.warn('GPT slot prefix did not resolve to one active element', { |
There was a problem hiding this comment.
♻️ refactor — Ambiguity warning fires on every animation frame during the SPA slot wait.
waitForSlotElements' allPresent() calls findSlotElementByDivId per slot per mutation-batched frame for up to 2 s. An ambiguous prefix emits this log.warn on every call — potentially ~120 duplicate console warns per navigation. Move the warn to the adInit resolution site, or add a warn-once guard keyed on divId.
| return null; | ||
| } | ||
|
|
||
| function candidateSlotRoots(divId: string): HTMLElement[] { |
There was a problem hiding this comment.
🤔 thinking — Render bridge re-resolves the prefix at message time and can disagree with adInit.
candidateSlotRoots re-runs the geometry-sensitive resolver when the Prebid Request message arrives. If layout shifted since adInit (hydration, rotation), or the prefix has become ambiguous by then, resolution returns a different element or null, slotIdForMessageSource misses the iframe, and the TS creative and win/billing beacons are silently dropped.
adInit already records the resolved ID in ts.divToSlotId — deriving candidate roots from that map would keep both sites consistent with the element adInit actually chose.
| if (allPresent()) finish(); | ||
| return; | ||
| } | ||
| animationFrame = requestAnimationFrame(() => { |
There was a problem hiding this comment.
🤔 thinking — rAF gating always costs the full 2 s in background tabs.
requestAnimationFrame doesn't fire in hidden tabs, so a background-tab SPA navigation never runs the observer check and applies bids only at the SPA_SLOT_WAIT_MS timeout. Minor, but a document.visibilityState === 'hidden' branch that checks allPresent() directly would restore prompt application.
| return element; | ||
| } | ||
|
|
||
| function runGptBootstrap(): void { |
There was a problem hiding this comment.
🏕 camp site — Reuse this helper in the older bootstrap test.
The pre-existing handoff test ('runs the embedded bootstrap handoff for a hydrated publisher ID', ~line 516) still inlines the identical readFileSync + window.eval that runGptBootstrap() wraps.
| tsjs?: any; | ||
| }; | ||
|
|
||
| function appendResponsiveSlotElement( |
There was a problem hiding this comment.
⛏ nitpick — appendResponsiveSlotElement takes five positional booleans.
Call sites like (id, true, false, true, false) are hard to read; an options object ({ containerHasLayout, elementHidden, … }) would make the parameterized cases self-describing. Test-only.
3 participants
Summary
This PR is stacked on #966, which already preserves GPT's
display(string | Element | Slot)contract in the permanent slot-handoff implementation. It does not depend on #974.Changes
crates/trusted-server-core/src/integrations/gpt_bootstrap.jscrates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.tscrates/trusted-server-core/src/integrations/gpt.rsCloses
Closes #977
Test plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflarecargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(430 passed)npm run lint && npm run formatnpm run buildcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)