Detect GPT initial load configured through setConfig - #948
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.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The move from "record what was attempted" to "read GPT's effective state" is the right direction, and the idempotency guards, argument forwarding, and jsdom bootstrap harness all land well. One blocking defect remains: getConfig() is now treated as authoritative for the legacy pubads().disableInitialLoad() path too, and per GPT's own documentation it never reflects that API. When a page mixes both APIs, the legacy detection this PR is meant to preserve is silently overwritten with false and TS-owned slots go back to rendering blank — the exact failure #946 fixes.
Blocking
🔧 wrench
getConfig()overrides legacydisableInitialLoad()state:syncInitialLoadDisabled()wins over the legacy fallback, but GPT'sgetConfig()only reportssetConfig()state, so a mixed-API page loses the disabled flag (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:483,crates/trusted-server-core/src/integrations/gpt_bootstrap.js:73).nullis treated as an authoritativefalse: the unset guard checks onlyundefined, whileGoogleTagConfig.disableInitialLoadis typedboolean | nullandnullis GPT's documented "reset to default" (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:443,crates/trusted-server-core/src/integrations/gpt_bootstrap.js:33).
Both were reproduced against this branch with a scratch vitest run; details and suggested fixes are inline.
Non-blocking
♻️ refactor
- Bootstrap coverage stops at the legacy path: the new jsdom harness never drives the bootstrap's
setConfigwrapper, which is the feature this PR adds (crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:252).
⛏ nitpick
- Test resolves the bootstrap through
process.cwd(): breaks if vitest is invoked from the repo root (crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts:291). getConfigtype is narrower than the real API: GPT's signature is variadic and values may benull(crates/trusted-server-js/lib/src/integrations/gpt/index.ts:128).
👍 praise
- Effective-state re-sync immediately before
slotsNeedingRefresh, plus__tsInitialLoadConfigHookedsharing between bootstrap and bundle so the wrapper is installed exactly once, and full argument forwarding on both wrappers (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:668).
CI Status
- fmt: PASS
- clippy / CodeQL analyze: PASS
- rust tests (fastly, cloudflare, CLI, cross-adapter parity): PASS
- js tests (vitest) + TS/docs format: PASS
- pending at review time: axum native, spin native + wasm32-wasip1, integration artifacts
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review after the update responding to the earlier REQUEST_CHANGES. The prior blocking issue — the setConfig hook could only ever set gptInitialLoadDisabled, never clear it, producing a duplicate ad request on re-enable — is resolved: the code now reads GPT's authoritative getConfig('disableInitialLoad') at detection and at adInit() decision time, and falls back to key-presence ('disableInitialLoad' in config) with === true. I traced the true → false → null sequence and confirmed the flag now clears and adInit() no longer double-requests. Verified against Google's GPT reference that getConfig('disableInitialLoad') returns a frozen object { disableInitialLoad: ... } and that disableInitialLoad is a real page-level setConfig/getConfig key, so the read is sound.
Approving. The remaining notes are non-blocking.
Non-blocking
♻️ refactor / test-coverage
- The
getConfig-absent fallback is untested (gpt/index.ts:466,gpt_bootstrap.js:51) — every new setConfig test injects agetConfigmock, sosyncInitialLoadDisabledalways returnstrueand the fallback branch never runs. That branch exists precisely for older GPT that hassetConfigbut notgetConfig; a regression test withgetConfigabsent would lock it in. Verified working via a scratch probe.
🤔 thinking
- Residual ordering dependency (
gpt/index.ts:668) — the authoritativegetConfigre-read atadInit()now catches setConfig state regardless of call order (a real gain). The only remaining blind spot is a publisher using the legacypubads().disableInitialLoad()before the detector wraps it, on a GPT build wheregetConfigdoes not reflect legacy state. Narrow; noting for completeness.
⛏ nitpick
getConfig?(key: 'disableInitialLoad')types the key as only that literal; real GPT acceptsstring | string[](gpt/index.ts:128).syncInitialLoadDisabledis duplicated near-identically between the bootstrap (JS) and the bundle (TS) — inherent to the two-implementation design and now partly guarded by the new bootstrap eval-test, but the divergence risk remains (gpt_bootstrap.js:30).
CI Status
All checks green on GitHub: cargo fmt, clippy (all adapters), cargo test (fastly / axum / cloudflare / spin / parity), vitest, browser + Fastly EC integration, CodeQL, format-typescript, format-docs.
Verified locally at the PR head: integrations::gpt::tests 30 passed; GPT vitest 90 passed; tsc --noEmit clean on the changed files.
# Conflicts: # crates/trusted-server-core/src/integrations/gpt_bootstrap.js # crates/trusted-server-js/lib/src/integrations/gpt/index.ts # crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts
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
Brings in the #988 browser-spec fix through its PR lineage and, because #988 stacks on #963's head, refreshes rc's stale #963 absorption with the July 29-30 rework: - bid.meta second descriptor carrier and bidAccepted registration replacing the requestId stash (prebid shim and Rust provider) - Hardened APS auction delivery: sanitized publisher page identity (query/fragment stripped), delivery drop telemetry with dropped_winner_count/reasons, imp disposition counters - ProviderLaunchState/ProviderRequestOutcome orchestrator refactor with parse_state threading and Immediate outcomes - as_aps() Option accessor, fail-closed render-bridge stop, responsive slot-root helpers, case-insensitive APS exclusion tests Preserved rc-only systems the #963 branch predates: #956 opt-in creative processing (process_auction_creative, sanitize_creatives), #967 decoupled prebid shim (public markWinningBidAsUsed instead of prebid.js internals), #948/#912 GPT sync, #865 platform timeout canonicalization (restored at both launch paths and both mediator paths, with the duplicate backend-name pre/post-launch guards and their test suite ported to the new provider API), and the provider-validation startup checks.
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!)