Defer initial adInit until after React hydration on Next.js App Router#945
Defer initial adInit until after React hydration on Next.js App Router#945aram356 wants to merge 1 commit into
Conversation
Brings the three hydration-safety changes: defer the initial adInit() until after hydration, append the tsjs head bundle at the end of <head>, and rewrite integration hosts in the streamed RSC flight. Merged cleanly with no conflicts.
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
6a1db09 to
3ebcf8f
Compare
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
🔧 Requesting changes. The diff is narrow, but it changes initial ad initialization for every matched-slot publisher and introduces high-risk navigation and GPT request-ordering regressions. CI is green, but the current automated coverage does not exercise these lifecycle paths.
| "<script>(window.tsjs=window.tsjs||{{}}).bids=JSON.parse(\"{}\");(function(){{var f=window.tsjs.adInit;if(typeof f===\"function\")f();}})();</script>", | ||
| "<script>(window.tsjs=window.tsjs||{{}}).bids=JSON.parse(\"{}\");\ | ||
| (function(){{\ | ||
| var p=location.pathname+location.search;\ |
There was a problem hiding this comment.
🔧 P1 — URL equality is not a safe navigation identity
The guard compares pathname + search, while the SPA auction hook intentionally identifies routes using only pathname (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:715-724). This creates two failures:
- A query-only
pushState/replaceStatebeforeloadmakes this callback abort, but the SPA hook also declines to auction because the pathname is unchanged. InitialadInit()is therefore never called. - An
/a → /b → /atransition before this callback defeats the stale-route check because the URL equals/aagain. The SPA hook auctions the new/aroute and callsadInit(), after which this initial callback can call it again, destroying/redefining slots and issuing duplicate requests.
This can leave all initial ads uninitialized after query normalization or double-refresh inventory after a rapid round trip. Please use a shared monotonic navigation generation maintained synchronously by the SPA hook, capture that generation here, and run only if it is unchanged. Keep query-only changes ignored unless /__ts/page-bids and the SPA hook are deliberately changed to treat them as auction-relevant.
| var f=function(){{\ | ||
| if(location.pathname+location.search!==p)return;\ | ||
| var a=window.tsjs.adInit;if(typeof a===\"function\")a();}};\ | ||
| var d=function(){{window.requestAnimationFrame(function(){{window.requestAnimationFrame(f);}});}};\ |
There was a problem hiding this comment.
🔧 P1 — The global load gate moves TS targeting behind normal publisher GPT requests
This generic bootstrap now waits for every load-blocking resource and then two animation frames. A publisher that defines/displays GPT before or during window.load sends its first request before TS targeting is applied. adInit() subsequently finds the publisher-owned slot (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:512-518) and refreshes it (index.ts:621-633), turning the TS-targeted request into a second impression. This behavior applies to every creative-opportunity page, not only the tested Next.js App Router publisher.
That can lose the server-auction bid on the first impression, create a duplicate request, delay TS-owned slots behind unrelated resources, and age the auction result. Testing one publisher whose first request happened after this callback does not establish compatibility with other supported initialization patterns.
Please preserve pre-request targeting rather than delaying the entire operation: defer only the hydration-unsafe TS-owned slot definition/display work while intercepting or targeting existing publisher slots before their first display(). If that cannot be done safely here, make delayed initialization an explicit App Router/publisher opt-in and retain synchronous behavior elsewhere.
| } | ||
|
|
||
| #[test] | ||
| fn bids_script_defers_ad_init_until_after_hydration() { |
There was a problem hiding this comment.
🔧 P2 — This test does not execute the lifecycle being changed
The assertions only check for substrings such as requestAnimationFrame, "load", and location.pathname. The test would still pass if adInit() ran synchronously, if only one frame were used, or with both navigation bugs noted above. The central guarantees—post-load ordering, exactly-once invocation, stale-navigation cancellation, and interaction with the SPA hook—remain unprotected.
Please add an executable browser/JavaScript test covering loading and already-complete documents, two-frame ordering, query-only history changes, /a → /b → /a, and a publisher GPT display() during load. Assert both the adInit() count and first-request targeting.
Summary
On a Next.js App Router publisher,
adInit()defines GPT slots on the publisher's-containerwrappers, mutating those ad-slot subtrees. The</body>bids bootstrap called it synchronously at parse time, landing that mutation inside React's hydration window — 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 count tracks whether
adInitprocesses ad slots — not the injection position.adInitis now deferred to after hydration: gated on windowload, then a doublerequestAnimationFrame. Run-once, no retry timer.Scope
Deliberately reduced to the single change with a measured effect on #418. Two other hydration-safety changes were considered and dropped from this PR, because neither moved the observed #418 count and review surfaced real defects in both:
disableInitialLoaddetector too late.lol_htmltext-chunk or cross-script boundaries, and rewrote every matching URL in flight data rather than only scriptsrcvalues.A correct flight rewrite needs the existing RSC stream state machine, raw (non-escaping) insertion, and
src-targeted parsing — that belongs in its own PR.Review findings addressed
location.pathname + location.search) and no-ops if the route changed, so it cannot re-runadInitagainst a newer route's live slots/bids and double-refresh it.window.requestAnimationFrame/window.addEventListenerare now window-qualified.ts_initial=1,hb_pb, andhb_bidder, so server-side targeting is applied to the first impression on the tested publisher. Residual risk remains in principle for a publisher whose GPT activation precedes windowload;load+ rAF is a pragmatic signal, as React exposes no hydration-complete event.Verification
bids_script_defers_ad_init_until_after_hydrationcovers the deferral, the route guard, the qualified globals, and the absence of a retry timer.trusted-server-corelib suite: 1646 passed, 0 failed.cargo fmtclean.servicesEnabled: true, TS container slot defined, ads rendering, and the first ad request fully targeted.Refs #938