-
Notifications
You must be signed in to change notification settings - Fork 12
Defer initial adInit until after React hydration on Next.js App Router #945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2180,8 +2180,31 @@ pub(crate) fn build_bids_script(bid_map: &serde_json::Map<String, serde_json::Va | |
| let json = serde_json::to_string(bid_map) | ||
| .expect("serde_json::to_string of Map<String,Value> should be infallible"); | ||
| let escaped = html_escape_for_script(&json); | ||
| // adInit() defines GPT slots on the publisher's `-container` wrappers, which | ||
| // mutates those ad-slot subtrees. Calling it synchronously here (this script | ||
| // runs at body-parse time) lands those mutations inside React's hydration | ||
| // window and trips a #418 hydration mismatch. Defer adInit until after the | ||
| // page has hydrated: gate on window `load` (client bundles that hydrate the | ||
| // tree have executed by then), then a double `requestAnimationFrame` so it | ||
| // runs after React has committed. Not a retry timer — a single deferred call. | ||
| // | ||
| // 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 this callback | ||
| // fires. The callback therefore captures the route it was scheduled for and | ||
| // no-ops when the route has since changed; without that guard it would run | ||
| // `adInit` a second time against the newer route's live slots/bids, which | ||
| // destroys and redefines that route's TS slots and refreshes it twice. | ||
| format!( | ||
| "<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;\ | ||
| 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);}});}};\ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P1 — The global This generic bootstrap now waits for every load-blocking resource and then two animation frames. A publisher that defines/displays GPT before or during 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 |
||
| if(document.readyState===\"complete\")d();\ | ||
| else window.addEventListener(\"load\",d,{{once:true}});\ | ||
| }})();</script>", | ||
| escaped | ||
| ) | ||
| } | ||
|
|
@@ -4709,6 +4732,58 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn bids_script_defers_ad_init_until_after_hydration() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 P2 — This test does not execute the lifecycle being changed The assertions only check for substrings such as Please add an executable browser/JavaScript test covering loading and already-complete documents, two-frame ordering, query-only history changes, |
||
| let mut map = serde_json::Map::new(); | ||
| map.insert("atf".to_string(), serde_json::json!({"hb_pb": "1.00"})); | ||
|
|
||
| let script = build_bids_script(&map); | ||
|
|
||
| // adInit() mutates ad-slot subtrees (GPT defineSlot on the | ||
| // `-container` wrapper). Running it synchronously at body-parse time | ||
| // lands those mutations inside React's hydration window and trips a | ||
| // #418 hydration mismatch. The bootstrap must defer adInit until | ||
| // after hydration: gate on window `load`, then a `requestAnimationFrame`. | ||
| assert!( | ||
| script.contains("requestAnimationFrame"), | ||
| "should defer adInit to a post-hydration animation frame" | ||
| ); | ||
| assert!( | ||
| script.contains("\"load\""), | ||
| "should gate adInit on the window load event" | ||
| ); | ||
| // Deferral must not regress into a retry timer. | ||
| assert!( | ||
| !script.contains("setTimeout"), | ||
| "should not retry adInit on a timer" | ||
| ); | ||
| assert!( | ||
| script.contains("window.tsjs.adInit"), | ||
| "should still hand off bids to adInit" | ||
| ); | ||
|
|
||
| // Browser globals are window-qualified so a page-level lexical | ||
| // binding of the same name cannot shadow them. | ||
| assert!( | ||
| script.contains("window.requestAnimationFrame"), | ||
| "should qualify requestAnimationFrame on window" | ||
| ); | ||
| assert!( | ||
| script.contains("window.addEventListener"), | ||
| "should qualify addEventListener on window" | ||
| ); | ||
|
|
||
| // Deferring opens a window in which an SPA navigation can commit a | ||
| // new route (and its own adInit) before this callback fires. The | ||
| // callback must capture the route it was scheduled for and no-op if | ||
| // the route has since changed, otherwise it re-runs adInit against | ||
| // the newer route's live slots/bids and double-refreshes it. | ||
| assert!( | ||
| script.contains("location.pathname"), | ||
| "should capture route identity to discard a stale deferred callback" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn auction_request_without_ec_id_omits_user_id_and_uses_non_ec_request_id() { | ||
| let slot = make_slot(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 P1 — URL equality is not a safe navigation identity
The guard compares
pathname + search, while the SPA auction hook intentionally identifies routes using onlypathname(crates/trusted-server-js/lib/src/integrations/gpt/index.ts:715-724). This creates two failures:pushState/replaceStatebeforeloadmakes this callback abort, but the SPA hook also declines to auction because the pathname is unchanged. InitialadInit()is therefore never called./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-bidsand the SPA hook are deliberately changed to treat them as auction-relevant.