Detect GPT initial load configured through setConfig - #948
Detect GPT initial load configured through setConfig#948ChristianPavilonis wants to merge 7 commits into
Conversation
aram356
left a comment
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
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
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
| ts.gptInitialLoadDisabled = true; | ||
| return original(); | ||
| const result = originalDisableInitialLoad(); | ||
| if (!syncInitialLoadDisabled(gpt, ts)) { |
There was a problem hiding this comment.
🔧 wrench — getConfig() is now allowed to override the legacy disableInitialLoad() signal, but GPT never reports the legacy API through getConfig(). A page that mixes both APIs loses the disabled state and TS-owned slots render blank again.
GPT's reference says getConfig() "gets a frozen copy of the general configuration options for the page set by setConfig" — pubads().disableInitialLoad() does not populate it. The comment above this function says the same thing ("may not report state set through the legacy API"), yet syncInitialLoadDisabled() returning true short-circuits the fallback, so any defined value from getConfig() — including a stale false written earlier by setConfig() — wins over a legacy call that actually disabled initial load.
Reproduced on this branch with a scratch vitest (getConfig backed only by setConfig, matching the documented behavior):
setConfig({ disableInitialLoad: false }); // publisher re-enables via modern API
pubads().disableInitialLoad(); // ...then disables via legacy API
→ ts.gptInitialLoadDisabled === false // expected true
→ adInit(): display('div-atf-sidebar'), pubads.refresh NOT called
The TS-owned first-impression slot is registered but never requested — the blank-slot regression this detector exists to prevent. main is not affected: the legacy wrapper there sets true unconditionally.
The syncInitialLoadDisabled(g, ts) call added before slotsNeedingRefresh widens the window: it re-clobbers legacy state at adInit() time even when the legacy call came last.
Fix — keep the legacy signal sticky and let getConfig() be authoritative only for setConfig()-driven state:
service.disableInitialLoad = function () {
const result = originalDisableInitialLoad();
// The legacy API is not reflected by getConfig(), and GPT offers no way to
// undo it, so record it separately and never let a sync downgrade it.
ts.gptLegacyInitialLoadDisabled = true;
ts.gptInitialLoadDisabled = true;
return result;
};and in syncInitialLoadDisabled:
if (ts.gptLegacyInitialLoadDisabled && value !== true) return false;Please add the mixed-API ordering (setConfig(false) → legacy disableInitialLoad() → adInit()) as regression coverage; none of the new tests exercise both APIs on the same page.
| if (typeof gpt.getConfig !== 'function') return false; | ||
|
|
||
| const config = gpt.getConfig('disableInitialLoad'); | ||
| if (!config || config.disableInitialLoad === undefined) return false; |
There was a problem hiding this comment.
🔧 wrench — The unset guard checks only undefined, so a null from getConfig() is treated as an authoritative false.
GoogleTagConfig.disableInitialLoad is typed boolean | null right here in this PR, and null is GPT's documented "reset to default" value — the new test even asserts setConfig({ disableInitialLoad: null }) behavior. But when getConfig() returns { disableInitialLoad: null }, this function returns true after writing false, which suppresses the legacy fallback below.
Reproduced on this branch:
getConfig('disableInitialLoad') → { disableInitialLoad: null }
pubads().disableInitialLoad();
→ ts.gptInitialLoadDisabled === false // expected true
Fix — treat anything that is not a boolean as unset:
const value = config?.disableInitialLoad;
if (typeof value !== 'boolean') return false;
ts.gptInitialLoadDisabled = value;
return true;Same change is needed in gpt_bootstrap.js:33.
| ts.gptInitialLoadDisabled = true; | ||
| return original(); | ||
| var result = originalDisableInitialLoad.apply(pubads, arguments); | ||
| if (!syncInitialLoadDisabled(gpt)) { |
There was a problem hiding this comment.
🔧 wrench — Same two defects as the bundle (see the comments on gpt/index.ts:483 and :443): a defined-but-stale getConfig() value — or a null reset, which line 33 accepts as a real value — suppresses the ts.gptInitialLoadDisabled = true fallback, and GPT never reports legacy disableInitialLoad() state through getConfig(). syncInitialLoadDisabled(window.googletag) at line 189 re-applies the clobber at adInit() time.
Fix:
function syncInitialLoadDisabled(gpt) {
if (typeof gpt.getConfig !== "function") return false;
var config = gpt.getConfig("disableInitialLoad");
var value = config && config.disableInitialLoad;
if (typeof value !== "boolean") return false;
// The legacy API is invisible to getConfig() and cannot be undone.
if (ts.gptLegacyInitialLoadDisabled && value !== true) return false;
ts.gptInitialLoadDisabled = value;
return true;
}
pubads.disableInitialLoad = function () {
var result = originalDisableInitialLoad.apply(pubads, arguments);
ts.gptLegacyInitialLoadDisabled = true;
ts.gptInitialLoadDisabled = true;
return result;
};| expect(mockPubads.refresh).toHaveBeenCalledWith([mockSlot]); | ||
| }); | ||
|
|
||
| it('keeps the legacy disabled state in the edge bootstrap when getConfig is unavailable', async () => { |
There was a problem hiding this comment.
♻️ refactor — The new jsdom harness only drives the bootstrap's legacy path, so the bootstrap setConfig wrapper — the feature this PR adds — still has no behavioral coverage, only the Rust substring assertions. Now that the harness exists, the setConfig({ disableInitialLoad: true }) → adInit() case and the true → false re-enable case are a few lines each, and they would pin down the bootstrap/bundle pair that the earlier 📌 finding flagged as prone to divergence.
⛏ Also, the title says "when getConfig is unavailable" but the fixture does provide getConfig; it just returns undefined. "when getConfig does not report legacy state" matches what is asserted.
| } as any; | ||
|
|
||
| const bootstrap = await readFile( | ||
| path.resolve(process.cwd(), '../../trusted-server-core/src/integrations/gpt_bootstrap.js'), |
There was a problem hiding this comment.
⛏ nitpick — Resolving the bootstrap relative to process.cwd() ties the test to being launched from crates/trusted-server-js/lib. new URL(...) against the module location is invocation-independent:
const bootstrapPath = new URL(
'../../../../trusted-server-core/src/integrations/gpt_bootstrap.js',
import.meta.url
);
const bootstrap = await readFile(bootstrapPath, 'utf8');| enableServices(): void; | ||
| display(elementId: string): void; | ||
| setConfig?(config: GoogleTagConfig): void; | ||
| getConfig?(key: 'disableInitialLoad'): GoogleTagConfig | undefined; |
There was a problem hiding this comment.
⛏ nitpick — GPT's actual signature is variadic (getConfig(...keys)) and returns a frozen PageSettingsConfig whose values may be null. getConfig?(...keys: string[]): GoogleTagConfig | undefined models the surface more honestly, and — given the null finding above — makes the missing null case visible at the type level.
| // first-impression slot renders blank on initial-load-disabled pages. Only | ||
| // add them in that case; otherwise display() + refresh() would | ||
| // double-request the impression. | ||
| syncInitialLoadDisabled(g, ts); |
There was a problem hiding this comment.
👍 praise — Re-reading the effective state immediately before computing slotsNeedingRefresh is the right place for it: it closes the gap where configuration lands after the detector installs but before adInit() runs. Sharing __tsInitialLoadConfigHooked with the edge bootstrap so exactly one wrapper is installed across the two scripts, and forwarding all arguments through both wrappers, are equally solid.
3 participants
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!)