Detect GPT initial load configured through setConfig by ChristianPavilonis · Pull Request #948 · IABTechLab/trusted-server

ChristianPavilonis · 2026-07-22T02:19:34Z

Summary

Detect GPT initial-load disabling through the modern googletag.setConfig({ disableInitialLoad: true }) API as well as the legacy pubads method.
Ensure TS-owned slots receive the required refresh() after display(), preventing GPT slots from stopping at fetch count zero.

Changes

File	Change
`crates/trusted-server-core/src/integrations/gpt_bootstrap.js`	Wrap both GPT initial-load configuration APIs in the early page bootstrap.
`crates/trusted-server-js/lib/src/integrations/gpt/index.ts`	Add equivalent detection to the bundled GPT integration and preserve the existing refresh behavior.
`crates/trusted-server-js/lib/src/core/types.ts`	Document both supported initial-load configuration paths.
`crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts`	Add regression coverage for `googletag.setConfig()`.
`crates/trusted-server-core/src/integrations/gpt.rs`	Assert that the injected bootstrap includes both detectors.

Closes

Closes #946

Test plan

cargo test-fastly && cargo test-axum
cargo clippy-fastly && cargo clippy-axum
cargo fmt --all -- --check
JS tests: cd crates/trusted-server-js/lib && npx vitest run
JS format: cd crates/trusted-server-js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
Manual testing via fastly compute serve
Other: Cloudflare and Spin tests/clippy passed; Playwright confirmed the current publisher setConfig() call now sets gptInitialLoadDisabled.

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses tracing macros (not println!)
New code has tests
No secrets or credentials committed

aram356

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

setConfig never clears gptInitialLoadDisabled → duplicate ad request: setConfig is a settings merge API, so { disableInitialLoad: false } re-enables and { disableInitialLoad: null } resets to default. Matching only === true leaves the flag stuck on, and adInit() then does display() and refresh() 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.cmd queue, so a publisher that configures GPT ahead of the TS injection point still lands in the old blank-slot failure mode. setConfig widens 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 a setConfig call without disableInitialLoad leaves the flag unset, and none that the __tsInitialLoadConfigHooked guard 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.js still has no behavioral tests — correctness rests on Rust substring assertions while the duplicated hooking logic grows. Follow-up issue suggested (gpt.rs:1265).

⛏ nitpick

setConfig is declared required on GoogleTag while 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 passed
cargo test -p trusted-server-core --lib integrations::gpt::tests — 30 passed
tsc --noEmit — no new errors in the changed files (pre-existing errors only, on untouched lines)

aram356 · 2026-07-22T21:09:48Z

+    if (typeof gpt.setConfig === 'function' && !gpt.__tsInitialLoadConfigHooked) {
+      const originalSetConfig = gpt.setConfig.bind(gpt);
+      gpt.setConfig = function (config: GoogleTagConfig) {
+        if (config?.disableInitialLoad === true) {


🔧 wrench — setConfig can only ever set the flag, never clear it, which produces a duplicate ad request.

setConfig is a settings merge API: { disableInitialLoad: false } re-enables initial load and { disableInitialLoad: null } resets it to the default. Matching only === true means that once a publisher disables and later re-enables initial load (a real pattern — disable while waiting on the CMP, re-enable after consent), ts.gptInitialLoadDisabled is stuck true. adInit() then takes the slotsToRefresh.concat(newSlots) branch and issues display() and refresh() on a TS-owned slot — exactly the double-request the comment below at line 641 says to avoid.

The asymmetry is new to this PR: the legacy path is safe only because GPT has no enableInitialLoad().

Verified against this branch with a scratch vitest, not theoretical:

setConfig({ disableInitialLoad: true }); setConfig({ disableInitialLoad: false }); → gptInitialLoadDisabled === true → adInit(): display('div-atf-sidebar') AND pubads.refresh([slot])

Fix — key-presence, which matches GPT's merge semantics (an absent key means "unchanged"):

gpt.setConfig = function (config: GoogleTagConfig) { if (config && 'disableInitialLoad' in config) { ts.gptInitialLoadDisabled = config.disableInitialLoad === true; } return originalSetConfig(config); };

The same change is needed in gpt_bootstrap.js. Please add the re-enable case as a regression test alongside the new one.

aram356 · 2026-07-22T21:09:48Z

+    ) {
+      var originalSetConfig = gpt.setConfig.bind(gpt);
+      gpt.setConfig = function (config) {
+        if (config && config.disableInitialLoad === true) {


🔧 wrench — Same defect as the bundle (see the comment on gpt/index.ts:447): config.disableInitialLoad === true never clears the flag, so setConfig({ disableInitialLoad: false }) (or null) leaves ts.gptInitialLoadDisabled stuck true and adInit() double-requests TS-owned slots via display() + refresh().

Fix:

gpt.setConfig = function (config) { if (config && "disableInitialLoad" in config) { ts.gptInitialLoadDisabled = config.disableInitialLoad === true; } return originalSetConfig(config); };

aram356 · 2026-07-22T21:09:48Z

+
+    if (typeof gpt.setConfig === 'function' && !gpt.__tsInitialLoadConfigHooked) {
+      const originalSetConfig = gpt.setConfig.bind(gpt);
+      gpt.setConfig = function (config: GoogleTagConfig) {


♻️ refactor — The wrapper is signature-locked to today's single-argument API and silently drops anything else GPT may pass. Forwarding all arguments is future-proof at no cost:

gpt.setConfig = function (...args: [GoogleTagConfig]) { const config = args[0]; // ... return originalSetConfig(...args); };

aram356 · 2026-07-22T21:09:48Z

    expect(mockPubads.refresh).toHaveBeenCalledWith([mockSlot]);
  });

+  it('refreshes TS-defined slots when setConfig disables GPT initial load', async () => {


♻️ refactor — Missing negative coverage for the new hook. This test only exercises the positive path; two cheap additions would have caught the wrench finding above and would pin down the wrapper's other contracts:

setConfig({ singleRequest: true }) must leave gptInitialLoadDisabled unset and must not add TS-owned slots to refresh().

Calling installTsAdInit() twice must not double-wrap setConfig — __tsInitialLoadConfigHooked is currently untested, and a double wrap would invoke the original setConfig twice.

Also 🏕 — this case is a ~40-line verbatim clone of the preceding legacy test, differing only in the setConfig mock. A shared setupGptMocks() helper would keep the pair from drifting.

aram356 · 2026-07-22T21:09:48Z

  destroySlots(slots?: GoogleTagSlot[]): boolean;
  enableServices(): void;
  display(elementId: string): void;
+  setConfig(config: GoogleTagConfig): void;


⛏ nitpick — Every sibling GPT API the shim guards at runtime is declared optional (disableInitialLoad?(), _loaded_?), and the detector does check typeof gpt.setConfig === 'function'. setConfig?(config: GoogleTagConfig): void would be more honest about the surface actually being probed.

aram356 · 2026-07-22T21:09:48Z

  disableInitialLoad?(): void;
 }

+interface GoogleTagConfig extends Record<string, unknown> {


⛏ nitpick — Extending Record<string, unknown> suppresses excess-property checking entirely, so the type documents disableInitialLoad without enforcing anything else about the config. Reasonable as an escape hatch for a partially-modelled third-party API — just noting the trade-off.

aram356 · 2026-07-22T21:09:49Z

        assert!(
-            combined.contains("disableInitialLoad"),
-            "bootstrap should wrap disableInitialLoad() to detect the disabled state"
+            combined.contains("gpt.setConfig"),


📌 out of scope — gpt_bootstrap.js correctness is still asserted only by substring matching from Rust. This PR duplicates a second piece of non-trivial hooking logic across the bootstrap and the bundle, so the two can now silently diverge — and per the wrench finding, they need the same fix applied twice.

Worth a follow-up issue for a jsdom harness that evals GPT_BOOTSTRAP_JS and drives it the way ad_init.test.ts drives the bundle.

Detect GPT initial load configuration

436a932

ChristianPavilonis mentioned this pull request Jul 22, 2026

July release candidate (DO NOT MERGE) #919

Draft

4 tasks

ChristianPavilonis self-assigned this Jul 22, 2026

Merge branch 'main' into fix/gpt-set-config-initial-load-detection

b5eff8b

ChristianPavilonis requested a review from aram356 July 22, 2026 20:51

aram356 requested a review from prk-Jr July 22, 2026 20:58

aram356 requested changes Jul 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Detect GPT initial load configured through setConfig#948

Detect GPT initial load configured through setConfig#948
ChristianPavilonis wants to merge 2 commits into
mainfrom
fix/gpt-set-config-initial-load-detection

ChristianPavilonis commented Jul 22, 2026

Uh oh!

aram356 left a comment

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

aram356 Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ChristianPavilonis commented Jul 22, 2026

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

🤔 thinking

♻️ refactor

🏕 camp site / 📌 out of scope

⛏ nitpick

CI Status

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants