Detect GPT initial load configured through setConfig#948
Detect GPT initial load configured through setConfig#948ChristianPavilonis wants to merge 5 commits into
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Extends initial-load detection to googletag.setConfig({ disableInitialLoad: true }) alongside the legacy pubads().disableInitialLoad(), in both the edge bootstrap and the bundle. The approach is right and the idempotency markers are correctly scoped per object, but the new setConfig hook is write-only: it can set gptInitialLoadDisabled and never clear it, which turns a publisher re-enabling initial load into a duplicate ad request on TS-owned slots.
Blocking
🔧 wrench
setConfignever clearsgptInitialLoadDisabled→ duplicate ad request:setConfigis a settings merge API, so{ disableInitialLoad: false }re-enables and{ disableInitialLoad: null }resets to default. Matching only=== trueleaves the flag stuck on, andadInit()then doesdisplay()andrefresh()on a TS-owned slot — the exact double-request the code comment warns against. Verified with a scratch vitest against this branch. Needs the same key-presence fix in both copies (gpt/index.ts:447,gpt_bootstrap.js:37).
Non-blocking
🤔 thinking
- Detection remains ordering-dependent: both hooks install from the
googletag.cmdqueue, so a publisher that configures GPT ahead of the TS injection point still lands in the old blank-slot failure mode.setConfigwidens coverage but does not remove the race. Longer term it may be more robust for TS to own the fetch for its own slots outright rather than inferring publisher state through wrappers.
♻️ refactor
- Missing negative coverage for the new hook (
ad_init.test.ts:246) — no test that asetConfigcall withoutdisableInitialLoadleaves the flag unset, and none that the__tsInitialLoadConfigHookedguard prevents double-wrapping. - Wrapper drops extra arguments (
gpt/index.ts:446) — forward with rest/spread instead of a fixed single parameter.
🏕 camp site / 📌 out of scope
- The new test case is a ~40-line verbatim clone of the preceding legacy test; a shared
setupGptMocks()helper would keep them in sync. gpt_bootstrap.jsstill has no behavioral tests — correctness rests on Rust substring assertions while the duplicated hooking logic grows. Follow-up issue suggested (gpt.rs:1265).
⛏ nitpick
setConfigis declared required onGoogleTagwhile every sibling runtime-guarded API is optional (gpt/index.ts:127).GoogleTagConfig extends Record<string, unknown>suppresses excess-property checking entirely (gpt/index.ts:112).
CI Status
GitHub, at time of review:
- format-typescript / format-docs: PASS
- vitest: PASS
- cargo test (ts CLI, native): PASS
- CodeQL (javascript-typescript, actions): PASS
- cargo fmt / clippy / test (fastly, axum, cloudflare, spin, parity): PENDING
Verified locally in a worktree at the PR head:
npx vitest run test/integrations/gpt/— 74 passedcargo test -p trusted-server-core --lib integrations::gpt::tests— 30 passedtsc --noEmit— no new errors in the changed files (pre-existing errors only, on untouched lines)
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The PR adds detection for the modern googletag.setConfig({ disableInitialLoad: true }) path, but the new hook tracks attempted setter calls instead of GPT's effective configuration. That state can become stale and cause duplicate requests for TS-owned slots.
Blocking
🔧 wrench
- Initial-load state can diverge from GPT's effective configuration: the flag is only set to
true, never cleared, and it is updated before GPT processes the configuration. Both the bundle and inline bootstrap need to read the authoritative GPT state and cover re-enabling in regression tests.
CI Status
- All GitHub checks currently report PASS, including formatting, Rust and JavaScript analysis, adapter tests, browser integration tests, and Vitest.
Summary
googletag.setConfig({ disableInitialLoad: true })API as well as the legacy pubads method.refresh()afterdisplay(), preventing GPT slots from stopping at fetch count zero.Changes
crates/trusted-server-core/src/integrations/gpt_bootstrap.jscrates/trusted-server-js/lib/src/integrations/gpt/index.tscrates/trusted-server-js/lib/src/core/types.tscrates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.tsgoogletag.setConfig().crates/trusted-server-core/src/integrations/gpt.rsCloses
Closes #946
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servesetConfig()call now setsgptInitialLoadDisabled.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)