Gate initial adInit on ad-slot container hydration - #969
Conversation
On a Next.js App Router publisher, `adInit()` defines GPT slots on the publisher's `-container` wrappers, mutating those ad-slot subtrees. The `</body>` bids bootstrap called it synchronously at parse time, landing that mutation inside React's hydration window, so React threw #418 and re-rendered the affected subtrees (visible flashes/reflow). A live A/B — toggling TS on the same page via the tester cookie — isolated the trigger: the pure publisher throws 0 #418, TS activation introduces it, and the count tracks whether adInit processes ad slots (not the injection position). adInit is now deferred to after hydration: gated on window `load`, then a double `requestAnimationFrame`. Run-once, no retry timer. Deferring opens a window in which an SPA navigation can commit a new route (and run its own adInit via the SPA auction hook) before the callback fires, so the callback captures the route it was scheduled for and no-ops when the route has changed — otherwise it would re-run adInit against the newer route's live slots/bids, destroying and redefining that route's TS slots and refreshing it twice. Browser globals are window-qualified so a page-level lexical binding cannot shadow them. Verified end to end through the dev proxy against a live App Router publisher: #418 goes from 1-2 to 0 with TS still defining its container slots and ads rendering. Refs #938
PR #945 deferred adInit to window.load to avoid a React #418 hydration mismatch, but window.load is dominated by page weight (~52s on heavy publishers), leaving Trusted Server ad slots empty until then. Wait instead for the async /_next/static/chunks/ hydration scripts to finish (resource timing for the already-loaded ones, load/error for the rest), then a double requestAnimationFrame, then adInit. window.load stays as an unconditional can't-hang fallback and as the unchanged path for non-Next publishers. The emitted script uses forEach and stays free of < / >. Adds the spec and implementation plan under docs/superpowers/.
…n chunks The chunk-await gate never fired early on a live publisher — chunk completion isn't reliably observable (already-fired load events, evicted resource-timing entries, phantom prefetch tags), so it always fell back to window.load (~40s). Poll for window.__next_f.push being replaced by the RSC runtime (~9s) instead, then double requestAnimationFrame; window.load stays as the can't-hang fallback and the non-Next path. Fires once via clearInterval — not a retry timer. Tests and design docs updated. Safety pending the #418 A/B.
Address PR #945 review findings: - Replace the URL-equality stale-route guard with a monotonic navigation generation (tsjs.navGeneration) maintained synchronously by the SPA auction hook. URL equality diverged from the hook's pathname-only route identity: a query-only replaceState before load cancelled the initial adInit entirely, and an /a -> /b -> /a round trip defeated the guard and double-ran adInit against the round-tripped route's live slots. - Move the deferral bootstrap (window load, double requestAnimationFrame, stale-navigation cancellation) from the Rust-emitted inline script into the GPT bundle module as tsjs.scheduleInitialAdInit, where the lifecycle is executable under Vitest and the navigation generation is shared with the SPA hook. The </body> bids script now only assigns tsjs.bids and delegates to the scheduler; the GPT module ships in the synchronous head bundle, so it has always run by the time the inline script executes. - Add executable lifecycle coverage (schedule_initial_ad_init.test.ts): load plus two-frame ordering, already-complete documents, exactly-once invocation across duplicate load events, query-only history changes, /a -> /b -> /a cancellation, and a publisher-displayed slot receiving setTargeting before the refresh that delivers it. The Rust tests now pin only the inline script's delegation and XSS safety.
…nto 958-adinit-hydration-chunk-gate # Conflicts: # crates/trusted-server-core/src/publisher.rs
Base's scheduler gated the deferred initial adInit on window `load`, which lands ~40s after first byte on heavy Next.js App Router publishers, leaving TS ad slots empty until then. Add the App Router runtime signal as an earlier trigger: poll for window.__next_f.push being replaced by the RSC runtime (~9s), and fire adInit on whichever arrives first. window.load stays as the unconditional fallback and the only signal on non-Next publishers, where __next_f is never patched so the poll never fires. The navigation- generation guard and the post-trigger double requestAnimationFrame are unchanged, so the fire-after-commit and stale-navigation semantics hold. Cover the runtime-signal early fire, the non-Next load fallback, and the exactly-once guarantee across both signals in schedule_initial_ad_init.test.ts.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the scheduler change against fix/react418-hydration-safety. The integration checks reported on this stacked PR pass, but I found one high-risk hydration-safety concern and one documentation/validation discrepancy; both are called out inline and should be addressed before merge. The blocking live React #418 A/B and the full JS test/format gates are still outstanding.
…llback Address the second-round review findings on PR #945: - scheduleInitialAdInit now receives the SSR bids payload from the </body> script and applies it only while the page is still on navigation generation 0 (the SSR document). Capturing the counter at body end adopted a navigation that committed while the HTML was still streaming, and the unconditional tsjs.bids assignment clobbered the live route's bids with the stale SSR payload. - adInit captures its generation and rechecks it as the queued googletag.cmd callback's first act, so a navigation that commits between the invocation and GPT's queue drain (e.g. consent-gated GPT) cancels the stale slot mutation. Applied in both the bundle and the head bootstrap. - gpt_bootstrap.js installs a minimal fallback scheduleInitialAdInit so a failed TSJS bundle load still initializes initial server-side ads through the head bootstrap's fallback adInit, restoring the degradation path the scheduler delegation had bypassed. - New executable coverage: navigation before scheduling, an applied page-bids response before scheduling, deferred command-queue drain cancellation, and a gpt_bootstrap.test.ts suite that evaluates the shipped bootstrap verbatim (fallback scheduler and adInit, generation cancellation, targeting and display through the command queue).
aram356
left a comment
There was a problem hiding this comment.
Summary
Replaces #945's window.load gate for the initial adInit() with a first-signal-wins hybrid: poll for the Next.js App Router runtime patching window.__next_f.push, with window.load retained as the can't-hang fallback. The gate logic itself is sound — exactly-once semantics (fired guard + clearInterval in both signal orders), navGeneration cancellation, and the readyState === 'complete' short-circuit all check out, and the new Vitest lifecycle tests are executable rather than string-matched. The blocking issues are descriptive, not functional: the PR title/body/docs describe two superseded implementations, and the #418 A/B this PR self-gates on is still outstanding.
Blocking
🔧 wrench
- PR title, body, and docs describe the superseded implementation. The title says "hydration chunks"; the body says the change is
crates/trusted-server-core/src/publisher.rs::build_bids_scriptand citesbids_script_is_xss_safe/bids_script_defers_ad_init_until_after_hydration— none of which is in this diff. The actual change is the__next_f.pushpoll inscheduleInitialAdInit(crates/trusted-server-js/lib/src/integrations/gpt/index.ts) plus its Vitest suite; there are no Rust changes at all. The docs' UPDATE banners cover the chunk-await →__next_fpivot but not thepublisher.rs→gpt/index.tsmove, so both docs still point future readers at the wrong file (the plan callspublisher.rs"the only production change"). Please retitle (e.g. "…on the Next.js runtime signal…"), rewrite the Change/Test-plan sections, and add one line to each doc's banner noting the final implementation location.
❓ question
- Is the #418 A/B still blocked, and will its evidence land on this PR? The spec says the blocking A/B was stuck behind the rc/july
elementId.startsWithcrash (fix = adopt #966's revert in the deploy). Has that deploy happened, and will the A/B result (confirming #418 = 0 and the ~9s vs ~52s timing) be attached here before merge?
Non-blocking
🤔 thinking
- Runtime-boot signal can still fire mid-hydration — see inline comment on
gpt/index.ts; considerrequestIdleCallbackafter the signal before the A/B run.
♻️ refactor
- Check
nextRuntimeReady()synchronously before installing the poll — see inline comment ongpt/index.ts.
🌱 seedling
__next_fis a Next.js internal. Worth documenting which Next versions the signal is verified against (App Router 13+); Pages Router publishers and any future Next version that renames the flight global silently take thewindow.loadpath. The failure mode is safe (slow, not broken), but a canary check per major Next upgrade of a gated publisher is warranted.
🏕 camp site
- Stale gate description in
publisher.rs. The base'sbuild_bids_scriptdoc comment (aroundcrates/trusted-server-core/src/publisher.rs:3320) still says the deferral is "gate on windowload, then a doublerequestAnimationFrame". This PR changes that behavior; since the file isn't in this diff the comment can't take an inline note, but updating that cross-reference here would avoid shipping a wrong comment at merge.
⛏ nitpick
- Untested signal order — load first, runtime signal later; see inline comment on the test file.
CI Status
- GitHub checks (browser integration, integration, EC lifecycle, artifacts): PASS
- JS tests (
npx vitest run, full suite at PR head): PASS (435/435; new scheduler suite 9/9) - JS + docs format (prettier): PASS
- fmt / clippy / Rust tests: not re-run — the net diff vs base contains no Rust changes.
… docs Review follow-ups on the __next_f adInit gate: - scheduleInitialAdInit now checks the runtime signal once before installing the 50ms interval. This script runs at </body>, and on a streamed App Router page the chunks can already have executed by then, so the poll idled a tick for nothing. The load listener stays registered but is inert (once: true plus the fired guard). - Document __next_f as a Next.js App Router internal: Pages Router publishers and any future release that renames the flight global never patch it and take the window.load path, so the failure mode is slow rather than broken. Worth re-checking on a gated publisher's major Next upgrade. - Cover the reverse signal order (load before the runtime signal) and the already-patched synchronous path, completing the signal-order matrix. - build_bids_script's doc comment still described PR #945's window.load-only gate. Comment only; the emitted script is unchanged. - Both docs' UPDATE banners now record the gate's move out of publisher.rs into gpt/index.ts, since the superseded sections still call publisher.rs the only production change. The spec also records requestIdleCallback-after-signal as considered and deferred, with the condition that would adopt it.
TsjsApi.scheduleInitialAdInit still documented PR #945's window.load-only deferral, which the __next_f runtime-signal gate replaced.
|
Review items resolved in a7fa1d0 and 01e7621. Summary, including one item where I'm answering rather than changing code. Blocking 1 — title/body/docs described the superseded implementationFixed. Title is now "Gate initial adInit on the Next.js runtime signal instead of window load"; the body's Change and Test-plan sections are rewritten around Blocking 2 — is the #418 A/B still blocked?The A/B evidence exists; it just does not read as "#418 = 0", so stating it precisely. The Live headless correlation capture on the real App Router publisher (tester cookie + DataDome, GAM SSAT reservation line
So: #418 is not zero, it is ~5% and costs no impressions on this build. The timing side of the claim holds — the runtime signal lands ~9s versus ~40–52s for I've left the A/B unchecked in the test plan and would rather it be signed off explicitly than have me mark it green on this, since the acceptance line as originally written asked for #418 = 0 and this is "#418 present but non-costing". If ~5% non-impression-costing #418 is acceptable, the blocking item is met and I'll update the checkbox; if the bar is literally zero, this gate does not clear it and the Two non-hydration findings from the same capture, both publisher-agnostic and out of scope here: Non-blocking
Verification
|
…n-chunk-gate Base's generation-0 pin (c3e8df3) rewrote the same scheduler regions the __next_f gate touches. Resolution keeps both sides' behavior: - gpt/index.ts: base's generation-0 early return and initialBids payload apply come first, then this branch's first-signal-wins gate (the __next_f poll, the synchronous pre-check, and the load fallback). Doc comment carries base's generation-0 paragraph plus this branch's Next-internal caveat. - types.ts: scheduleInitialAdInit's doc describes both the first-signal gate and the generation-0 pin, and notes that gpt_bootstrap.js's fallback scheduler stays on load only. - publisher.rs: build_bids_script's comment likewise merges the first-signal wording into base's generation-0 and payload-handoff text. - schedule_initial_ad_init.test.ts: union of both test sets — base's three generation-0 cases and this branch's five signal cases, 14 in the suite. Verified: vitest 487/487 (scheduler suite 14/14), JS prettier clean, cargo fmt clean, clippy fastly/axum/cloudflare/spin-native clean, core lib 1736/1736, test-axum green.
…chunk-gate Main landed #945, which defers the initial adInit past React hydration by gating on window `load` plus a double requestAnimationFrame. This branch replaces that gate with first-signal-wins — the Next.js App Router runtime patching `window.__next_f`, or `load` as the fallback and the only signal on non-Next publishers — so the two implementations collided across the scheduler, its documentation, and both test files (add/add on each). Resolution: - Keep this branch's scheduler. It registers the same `load` listener as its fallback, so main's behavior is preserved as the non-Next path. - Keep main's hidden-document reasoning in both the bundle scheduler docs and the gpt_bootstrap.js fallback, and record that the runtime poll fires independently of rAF while `afterHydrationFrames` still gates the call, so hidden-tab behavior is unchanged. - Take main's `navGeneration` doc wording: it describes the generation-0 pin this branch already implements, where this branch's text was stale. - Take main's removal of the `!contains("setTimeout")` assertion in gpt.rs. The no-retry-timer property is covered by gpt_bootstrap.test.ts, and a substring check over the joined head-inserts would misattribute any future unrelated timer to the scheduler. - Port main's hidden-document tests into both test files on top of this branch's runtime-signal cases, along with the `document.hidden` cleanup. - Drop main's publisher.rs pointer to this branch as pending follow-up work, since it lands here.
The handle was declared as an undefined `let` and assigned exactly once, which trips eslint `prefer-const` and failed the JS lint CI gate. Install the interval as a `const` and clear it from inside the tick instead of from `fire`; the `fired` guard still enforces once-only firing, and the poll now stops within one 50ms tick after either signal wins.
The `__next_f` runtime signal fires at hydration start, not completion: measured on a live App Router publisher it lands a median 16ms after React's first commit and sometimes before it, while hydration continues for seconds afterward. adInit's GPT slot definitions then mutate containers React has not yet claimed, React regenerates the mismatched subtree, and the creative that just rendered is destroyed. Gate instead on React having hydrated the ad-slot containers adInit is about to mutate, checked on those elements via the `__reactFiber$` / `__reactProps$` own property React attaches to host nodes it owns. This targets the actual mutation sites rather than a page-global framework flag, and introduces no timing constant. `window.load` stays as the unconditional fallback and remains the only signal where containers never report hydrated, so the failure mode is still slow rather than broken. Controlled capture on the same article page, 6-8 loads per arm, varying only what drives adInit: publisher alone (TS off) 0/8 #418 adInit suppressed 0/8 #418 window.load gate 0/6 #418, 5/6 slots rendered __next_f gate 8/8 #418, ad destroyed on 3/6 runs container-hydrated gate 0/8 #418, 8/8 slots rendered The container gate holds adInit to a median 1096ms after the hydration commit with a minimum of 983ms, never before it on any run. Locally this shows no latency win over `window.load`, which fires at ~11.8s through the dev proxy — roughly when the containers hydrate, so there is no gap to win. The win the runtime signal was chasing exists only where `load` is dominated by images and trackers (~52s on the deployed publisher versus ~9s to hydrate); that remains unmeasured and needs a deploy A/B.
Summary
Follow-up to #945, which defers the initial
adInit()past React hydration to fix a#418 hydration mismatch on Next.js App Router, gating on
window.load.window.loadwaits for every image and tracker, so on a heavy publisher it fires ~52s after
navigation and the Trusted Server slots sit empty until then.
An earlier revision of this branch replaced that gate with the Next.js App Router
runtime signal (
window.__next_f.pushbeing patched). Measurement has since shownthat gate is unsafe, and it has been replaced. This PR now gates on React having
hydrated the ad-slot containers
adInitis about to mutate, withwindow.loadretained as the unconditional fallback.
Why the runtime signal was dropped
A controlled capture against a live App Router article page, varying only what drives
adInitand holding the page, proxy and build identical:adInitsuppressedwindow.loadgate (#945)__next_fgate (previous revision here)Three things this establishes that the earlier evidence on this PR did not:
adInitis the sole cause of the JS bundle hash recomputed on every HTML response instead of cached #418. The publisher throws none on its own,and TS's
<head>/</body>script injection throws none — only theadInitDOMmutation does.
__next_fpatched means hydration started. It lands a median 16ms afterReact's first commit and on some runs before it, while hydration continues for
seconds (830–21,876 commits observed per run). The double
requestAnimationFrameis a ~32ms margin against a multi-second tail.
creative that just rendered is destroyed — on 3/6 runs the
in_contentiframeappeared and was then removed ~80ms after the error.
The earlier "#418 ≈5%, not impression-costing" reading came from pages whose ad slots
are not React-owned, and had no control arm. On an article page, whose containers are
React-rendered with
useIdids (ad-in_content-<hash>-in_content-0-container), the__next_fgate reproduces at 100%.Change
crates/trusted-server-js/lib/src/integrations/gpt/index.ts— the gate polls forevery currently-matched slot container reporting React-hydrated, via the
__reactFiber$/__reactProps$own property React attaches to host nodes it owns.New
isReactHydratedhelper.window.loadlistener,firedguard +clearIntervalexactly-once semantics, synchronous pre-check,
navGenerationcancellation and thedouble
requestAnimationFrameare all unchanged. The pass stays a singleadInitcall so GPT still issues one batched ad request.
crates/trusted-server-js/lib/test/integrations/gpt/schedule_initial_ad_init.test.ts— the five signal-specific tests moved onto the container signal, plus a new case
where one container hydrates and another never does (the batch must wait for the
laggard, then fall back to
load).crates/trusted-server-core/src/publisher.rs,crates/trusted-server-js/lib/src/core/types.ts— comments only, describing the gate that is actually shipped.
Why containers and not a page-global signal
adInitdefines GPT slots on the publisher's ad-slot subtrees, so what matters iswhether React has hydrated those elements — not whether the framework runtime booted
or every subresource finished. This introduces no timing constant: each page gates on
its own DOM. If React renames the internals, nothing reports hydrated and the gate
degrades to the
loadpath, so the failure mode stays slow rather than broken. Non-Reactpublishers take the
loadpath for the same reason, exactly as before.Latency — measured and unmeasured
Measured locally through
ts dev proxy, the container gate fires at ~11.85s versus~11.80s for
window.load— indistinguishable, because through the proxyloadalready fires at roughly the moment the containers hydrate, so there is no gap to win.
SSAT still beat the publisher's own client-side GAM request by ~2.2s.
The latency win this branch exists for is therefore unverified. It only appears
where
loadis dominated by images and trackers — ~52s on the deployed publisheragainst ~9s to hydrate. The container gate should fire at hydration time regardless of
what
loadis waiting for, but that is reasoning, not measurement, and reasoning iswhat put the previous revision here at 8/8 #418.
Note on the previous acceptance bar
The prior test plan asked to "confirm #418 = 0". That zero was measured on the light
publisher #945 tested; the
window.loadgate's #418 rate on a heavy publisher had neverbeen measured. In the capture above it is 0/6 for
loadand 0/8 for this gate, so thebar is now both measured and met on this page.
Test plan
cd crates/trusted-server-js/lib && npx vitest run— 562/562; scheduler suite 16/16cd crates/trusted-server-js/lib && npm run format— cleancargo fmt --all -- --check— cleancargo clippy-fastly— cleants dev proxy:0/8 JS bundle hash recomputed on every HTML response instead of cached #418, 0/8 destroyed creatives, 8/8 slots rendered,
adInita median 1096msafter the hydration commit (min 983ms, never before)
(~seconds) rather than at
window.load(~52s) on the deployed heavy publisher,with
ts_initial=1on the first GPT request and JS bundle hash recomputed on every HTML response instead of cached #418 still 0test-axum/test-cloudflare/test-spin, adapter clippyvariants, parity suite
Follow-ups (out of scope here)
in_contentcontainers(
display:none) — 19 present on the tested article page. Wasted GAM traffic,independent of the gate.
Refs #958, #938. Builds on #945.