Skip to content

Detect GPT initial load configured through setConfig#948

Open
ChristianPavilonis wants to merge 5 commits into
mainfrom
fix/gpt-set-config-initial-load-detection
Open

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

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

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
aram356 requested a review from prk-Jr July 22, 2026 20:58

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated
Comment thread crates/trusted-server-core/src/integrations/gpt_bootstrap.js Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated
Comment thread crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts
Comment thread crates/trusted-server-core/src/integrations/gpt.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/trusted-server-js/lib/src/integrations/gpt/index.ts Outdated
@ChristianPavilonis
ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 and prk-Jr July 23, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Detect GPT initial load configured through setConfig

3 participants