Prevent duplicate GPT slot requests

[ChristianPavilonis]

Summary

• Prevent competing GPT requests when TS initializes a placement before the publisher defines its inner div.
• Hand the TS-created inner-div slot to the late publisher definition and preserve SPA ownership cleanup.

Changes

File	Change
crates/trusted-server-js/lib/src/integrations/gpt/index.ts	Define fallback slots on the inner div and add scoped late-publisher definition/display/refresh handoff wrappers.
crates/trusted-server-js/lib/src/core/types.ts	Add shared window.tsjs handoff lifecycle state.
crates/trusted-server-core/src/integrations/gpt_bootstrap.js	Mirror the initial-load handoff behavior in the head bootstrap.
crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts	Add request-level tests for late definitions and disableInitialLoad().
crates/trusted-server-core/src/integrations/gpt.rs	Assert the injected bootstrap uses inner-div handoff behavior.
docs/superpowers/{specs,plans}/	Add the design specification and implementation plan.

Closes

Closes #944

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: cargo test-axum; target-matched trusted-server-core GPT tests; cargo clippy-axum

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]

Summary

The inner-div fallback plus narrowly scoped defineSlot/display/refresh wrappers with a shared window.tsjs registry is a clean solution to the competing container-slot problem, and the test suite is thorough (options preservation, global-refresh filtering, ambiguity rejection, bootstrap-eval parity). However, the wrappers introduce two reproducible crash paths — one of which breaks publisher defineSlot calls for slots unrelated to TS placements — so this needs another pass before merge. Details inline.

Non-blocking

🌱 seedling

• Handoff registry is never pruned: gptSlotHandoffs entries and hydration aliases accumulate for the page lifetime across SPA navigations; correctness is protected only by the live-slot lookup (findGptSlotByElementId) on every claim. Consider deleting entries when adInit() destroys prevGptSlots, as future-proofing against a matching path that forgets the liveness check.

CI Status

No GitHub checks have run on this branch. Verified locally:

• JS tests (npx vitest run): PASS (418/418)
• Core GPT Rust tests (cargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt): PASS (31/31)
• Prettier on gated lib/ files: PASS

Both inline crash findings were reproduced with Vitest against the built module.

[aram356]

Summary

Solid, conservative design for the late-publisher slot handoff: single-candidate ambiguity guard, internal-call guard, idempotence markers shared between bootstrap and bundle, and ownership transfer out of destroySlots(). One blocking correctness issue in the prefix-matching path, plus five non-blocking items.

Blocking

🔧 wrench

• Prefix handoff can hijack a sibling placement's slot: the prefix path can alias a publisher defineSlot() for a different sibling div to the TS slot and suppress its display(), leaving that placement blank (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:495, crates/trusted-server-core/src/integrations/gpt_bootstrap.js:63). See inline comments — a DOM-presence guard on handoff.slotElementId fixes it without breaking the new hydration tests.

Non-blocking

🤔 thinking

• One-shot suppressPublisherRefresh can swallow a genuinely-later refresh (index.ts:554): documented spec tradeoff, no action requested.
• JSON.stringify size comparison defeats valid GPT shorthand (index.ts:497): a publisher passing [300, 250] un-nested skips the handoff and reintroduces the duplicate slot; consider normalizing before comparing.

♻️ refactor

• Bootstrap parity gap in tests (ad_init.test.ts:589): the eval'd-bootstrap suite covers only the hydrated handoff and div-less define; the disabled-load global-refresh filter, refresh-options preservation, and ambiguity cases run only against the bundle.

📌 out of scope

• gptInitialLoadDisabled recorded even when the call was too late to take effect (index.ts:451, unchanged by this PR): GPT ignores disableInitialLoad() called after enableServices(), but the detector still records true. On exactly this PR's target pages (hydration-deferred GPT setup), TS's adInit() enables services first, so the publisher's later disableInitialLoad() is a GPT no-op — yet every subsequent SPA-navigation adInit() both display()s (which really requests) and refresh()es TS-owned slots: a double request, the bug class this PR fixes. Pre-existing detector behavior — worth a follow-up issue (record only when !ts.servicesEnabled at call time).

⛏ nitpick

• gpt_bootstrap.js is no longer prettier-clean (gpt_bootstrap.js:105): two over-width lines; outside the CI format glob but main's copy is clean.

CI Status

GitHub checks: only CodeQL Analyze ran — PASS; fmt/clippy/test workflows have not run on this PR.

Local verification:

• fmt: PASS (cargo fmt --all -- --check)
• rust tests: PASS (cargo test-fastly gpt, 31/31 including the new bootstrap assertion test)
• js tests: PASS (npx vitest run, 477/477)
• clippy: not run (only Rust change is a test)

[aram356]

Summary

Re-review at b200be53c. All findings from the previous rounds are verified fixed with regression coverage (div-less defineSlot guard with native two-argument passthrough, duck-typed display targets, native null contract for repeat definitions of a claimed div, SPA handoff pruning, sibling-slot DOM-absence guard, shorthand size normalization, bootstrap-eval parity coverage, Prettier-clean bootstrap). One new blocking issue remains: the DOM-absence guard added for the sibling case does not distinguish a hydration rename from an SPA navigation, which lets a cross-route publisher definition claim a dead slot — details inline, reproduced with a Vitest against the module.

CI Status

Only CodeQL ran on GitHub (pass). Verified locally at this head:

• JS tests (npx vitest run): PASS (484/484)
• Core GPT Rust tests (cargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt): PASS (31/31)
• Prettier on gpt_bootstrap.js and the changed lib/ files: PASS

[aram356]

Summary

Round-3 review at 72ff8cbf2. The cross-route mis-claim from the previous round is properly fixed: onNavigate() now synchronously deletes unclaimed handoffs (aliases included) before the page-bids fetch, with bundle and bootstrap regression coverage — I re-ran the previous round's reproduction against this head and it passes. However, the branch is red: vitest fails 6 tests in CI and locally. The head is a self-merge (Merge origin/fix/duplicate-gpt-slots into fix/duplicate-gpt-slots) of the old branch line into a rebased duplicate, and the conflict resolution dropped newer main-side GPT work — one inline finding per implementation, plus the test-reconciliation finding below (that file is outside this PR's diff, so it cannot carry an inline comment).

Content-wise the diff against main is still exactly the 8 PR files (GitHub's live compare confirms; the PR page's "106 files changed" stat is stale from before main advanced and should settle).

Blocking

🔧 wrench

• main's new gpt_bootstrap.test.ts contract tests are not reconciled with the handoff wrappers (2 of the 6 failures; crates/trusted-server-js/lib/test/integrations/gpt/gpt_bootstrap.test.ts:197 and :244). Two distinct problems: (1) the tests assert on googletag.defineSlot / mockPubads.refresh after the bootstrap has replaced them with patchedDefineSlot/patchedRefresh, so vitest throws [Function patchedDefineSlot] is not a spy — capture the native spy references before evaluating the bootstrap, or assert on the local spy variables; (2) the first test still encodes the pre-PR container behavior, expecting defineSlot('/123/atf', [[300, 250]], 'div-atf-sidebar-container') — this PR deliberately defines the TS fallback on the inner div, so the expected element ID must become 'div-atf-sidebar'. That changed expectation is part of this PR's behavioral contract and belongs in this diff.

Non-blocking

📝 note

• Branch topology: the head merges a rebased duplicate of this branch with its old self — 16 commits, two parallel lines with identical commit messages (3637ebb0e..41ea00232 vs 76c56263f..b200be53c). The net diff is correct, but consider collapsing to a single line before merge (or plan on a squash-merge) so the history doesn't permanently carry both copies of every commit.

CI Status

• vitest (GitHub CI): FAIL — 6 tests, reproduced locally (570/576 pass)
• CodeQL (actions, javascript-typescript, rust): PASS
• Core GPT Rust tests (cargo test -p trusted-server-core --target aarch64-apple-darwin integrations::gpt): PASS locally (39/39)
• Prettier on the five touched JS/TS files: PASS locally

[aram356]

Summary

Round-4 review at bda19d304. Both round-3 findings are verified fixed: syncInitialLoadDisabled is restored at the refresh decision in both implementations, and the bootstrap contract tests now assert on native spy references with the inner-div expectations this PR owns. CI is fully green (vitest included); locally the JS suite passes 611/611 and the core GPT Rust tests pass 39/39.

This round's findings are all in the newly stacked-in #978 responsive-slot resolution, which is now part of this PR's diff. The tiered visible/layout resolution design is sound and well-tested, but it introduces one reproducible SPA latency regression plus two smaller items — all inline.

CI Status

• All GitHub checks: PASS (vitest, browser integration, parity, adapters, format, CodeQL)
• JS tests locally: PASS (611/611)
• Core GPT Rust tests locally: PASS (39/39)
• Prettier: one deviation in gpt_bootstrap.js (inline nitpick; file is not CI-gated)

[aram356]

🔧 wrench — allPresent (just above, line 1171) now runs through the tiered resolver, which returns null for a unique prefix match that is display:none. A breakpoint-hidden placement whose div exists in the DOM (a mobile-only slot config while on desktop, a consent-gated section) can never "appear", so every SPA navigation to such a route waits out the full 2s SPA_SLOT_WAIT_MS before applying bids to every slot on the route — visible ones included — and adInit then skips the hidden slot anyway. Reproduced with a Vitest against this module: with one display:none sibling placement, adInit fires 0 times before the timeout and once after ~2.1s. Pre-#978, the first prefix match resolved immediately and there was no stall — this is a systematic 2-second render delay for affected routes.

Fix: for the wait (not for adInit), presence and eligibility are different questions — count a slot as present when its prefix matches any element:

const allPresent = (): boolean =>
  slots.every((slot) => {
    const resolution = resolveSlotElementByDivId(slot.div_id);
    return resolution.element !== null || resolution.prefixMatchCount > 0;
  });

adInit still applies the strict tiers when it runs; the wait just stops stalling on placements that exist but are ineligible. Please add a regression test: route with one visible and one display:none prefix-configured placement must apply bids without waiting for the timeout.

[aram356]

Fixed in 8bfb442. allPresent now counts a slot as present when its prefix matches any element (resolution.element !== null || resolution.prefixMatchCount > 0), so a breakpoint-hidden placement no longer stalls the route for the full timeout; adInit still applies the strict tiers when it runs. Added the requested regression test (one visible + one display:none prefix placement applies bids without waiting) and verified it fails against the previous allPresent.

[aram356]

🤔 thinking — This warning only fires when prefixMatchCount > 1. The single-hidden-match case (prefixMatchCount === 1, activeMatchCount === 0 — the common breakpoint-hidden config) is skipped with no log at all, and the exact-id tier above skips visibility checks entirely, so a hidden exact-id match still gets a slot while a hidden prefix match silently does not. Given how much production time goes into diagnosing blank placements, consider logging the prefixMatchCount >= 1 && activeMatchCount === 0 case too (debug level is fine), and a short comment documenting the exact-vs-prefix visibility asymmetry as intentional.

[aram356]

Added in 8bfb442. The prefixMatchCount === 1 && activeMatchCount === 0 case now logs at debug level in both the bundle and the bootstrap, and the resolver documents the exact-vs-prefix visibility asymmetry as intentional at the exact-id tier.

[aram356]

⛏ nitpick — This ts.log.warn(...) call isn't Prettier-formatted (Prettier wants the argument list broken). The file isn't covered by the CI format gate, but it was made Prettier-clean earlier in this PR — one npx prettier --write away from staying that way.

[aram356]

Fixed in 8bfb442; the block is Prettier-formatted again.