Uh oh!
There was an error while loading. Please reload this page.
Select active GPT responsive slots - #978
Conversation
4d95acc to
340d1efCompare
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
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.)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aram356
left a comment
There was a problem hiding this comment.
Summary
The hardening commit resolves all findings from the previous review: width-only container geometry, ancestor-aware visibility with checkVisibility(), warn-once diagnostics at the adInit call site, render bridge keyed off ts.divToSlotId, and a hidden-document fallback in the SPA slot wait — with parity coverage for both implementations. Remaining comments are non-blocking refinements.
CI Status
- GitHub checks had not run on this head at review time.
- Verified locally: JS tests 440/440 PASS; eslint PASS;
trusted-server-coregpt tests 31/31 PASS on wasm32-wasip1 via Viceroy.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
a5cba2b to
6b1dd6fCompareThis reverts commit b65e1ae.
6b1dd6f to
2ed9f45Compare
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!)