Select active GPT responsive slots - #978
Conversation
4d95acc to
340d1ef
Compare
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
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!)