Add GPT runtime diagnostics overlay - #974
Conversation
Give publisher operators an opt-in, tab-local view of GPT-observed slot lifecycles and exact DOM bindings without changing auction or publisher behavior. Keep capture bounded, export allowlisted facts only, and cover activation, correlation, presentation, and non-interference across unit and browser tests.
Scope the diagnostics navigation assertion to the fixture navigation landmark so Playwright does not match the separate site-wide Home link in CI.
|
Fixed the browser integration failure in 79d0832. The test used an unscoped Local verification: |
# Conflicts: # crates/trusted-server-js/lib/src/core/types.ts
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
A well-built, self-contained diagnostics module: bounded memory, exact-ID binding that refuses to guess, ambiguity kept as a first-class outcome, closed shadow DOM, and no mutation of publisher slot elements. No correctness or security defects found, so everything below is a suggestion rather than a blocker.
Suggestions
🤔 thinking
- Enabling diagnostics ships 30 KB of tester-only code to every visitor:
tsjs-gpt_diagnostics.jsis 30,343 bytes minified, the largest module in the bundle and 3× core, concatenated synchronously for 100% of traffic while every tab stays inactive until?ts_console=1. Immediate loading is the right call and is test-locked; the tension is thatsessionStorage-only activation gives the server no way to scope the payload to activated sessions. (crates/trusted-server-core/src/integrations/gpt_diagnostics.rs:56) - Unfiltered childList mutations drive full snapshots and forced layout on every DOM insertion, amplified by four independent per-frame snapshot consumers. (
badges.ts:183) slotOnload/impressionViewableare discarded when GPT omitsisEmpty, because both matchers requireisEmpty === falsewhile the overlay treatsundefinedas pending. (store.ts:245,store.ts:264)
♻️ refactor
- A missing
CSS.escapesurfaces asduplicate_dom_idin the exported binding reason, reporting a collision that does not exist. (binding.ts:180)
⛏ nitpick
- Badge clamp hardcodes the stylesheet width (
264vsmax-width: 260px). (badges.ts:134)
🌱 seedling
- Confirmed auction-to-render traceability is the natural next layer.
slotRenderEndedalready carrieslineItemId,campaignId,advertiserIdand thesourceAgnostic*pair on the same event object the observer reads, and the render bridge already knows when the rendered creative requests markup from Trusted Server. Together those answer whether the line item Trusted Server won is the one that rendered, or whether a house ad, direct-sold line item, or Ad Manager default took the slot — on observed evidence, never inferred from targeting or price. Implemented and opening stacked on this PR; it keeps the export clear of the privacy regex this PR's spec asserts.
👍 praise
- The
observed = matched + unmatched + ambiguousinvariant asserted directly in both the unit and browser tests. - Binding refuses prefix and container-ID guesses, and keeps ambiguity visible instead of picking a winner.
referencesUnchanged()pins the non-invasiveness claim to an assertion —cmd.push,display,defineSlot,refresh,fetch,XHR.open, and the history methods all verified unpatched — and the export is checked against a privacy regex.- Remount handling for hydration-driven removal, with an explicit host-collision guard.
CI Status
All 19 checks pass on 79d0832.
- fmt: PASS
- clippy / cargo check (fastly, axum, cloudflare, spin): PASS
- rust tests (incl. cross-adapter parity, ts CLI): PASS
- js tests (vitest) and format-typescript: PASS
- browser integration tests: PASS
- format-docs: PASS
| let integration = Arc::new(GptDiagnosticsIntegration); | ||
| Ok(Some( | ||
| IntegrationRegistration::builder(GPT_DIAGNOSTICS_INTEGRATION_ID) | ||
| .with_head_injector(integration) |
There was a problem hiding this comment.
🤔 thinking — Worth weighing: enabling diagnostics ships 30 KB of tester-only code to every visitor.
Built from a clean worktree of 79d0832, tsjs-gpt_diagnostics.js is 30,343 bytes minified — the largest module in the bundle and 3× tsjs-core.js (10,031 B). With no .with_deferred_js(), js_module_ids_immediate() includes it in the synchronous first-party bundle injected at <head> start, so once an operator sets enabled = true every visitor downloads and parses it even though every tab stays inactive until ?ts_console=1.
enabled_registry_includes_diagnostics_in_immediate_modules shows this is deliberate, and the reasoning holds: deferring would delay listener installation past the first slotRequested/slotRenderEnded and lose the initial impression's cycle, which is the one operators most want. So this is not a suggestion to defer.
The tension comes from sessionStorage-only activation leaving the server no signal, which forces an all-or-nothing payload decision. A server-set session cookie on the activation directive would resolve both at once — the module is concatenated only for an activated tester, and for that tester it still loads immediately with listeners installed exactly as early as today. That is what #961 did with __Host-ts-console, and this test could keep asserting immediate loading, just scoped to activated sessions.
If shipping to all visitors is an accepted cost for now, it may be worth recording the rationale in gpt-diagnostics.md beside the deployment configuration so operators know what enabling it costs their page weight.
| ); | ||
| const relevant = records.some( | ||
| (record) => | ||
| record.type === 'childList' || |
There was a problem hiding this comment.
🤔 thinking — Any DOM insertion anywhere on the page triggers a full snapshot and a forced layout.
Attribute records are correctly filtered to known slot element IDs, but record.type === 'childList' matches every childList mutation in the document. On an ad-heavy page that fires more or less continuously, and each resulting update() deep-clones the whole store and calls getBoundingClientRect() for every bound slot.
It compounds because four consumers snapshot independently, each cloning the entire store per frame: binding.ts:109, badges.ts:119, overlay.ts:353, api.ts:78.
Filtering childList records the way the attribute records already are — added or removed nodes intersecting a known slot element ID — and/or sharing one snapshot per frame would remove most of the churn.
| slot, | ||
| timestampMs, | ||
| (cycle) => | ||
| cycle.renderAtMs !== undefined && cycle.isEmpty === false && cycle.loadAtMs === undefined, |
There was a problem hiding this comment.
🤔 thinking — A load or viewable callback is dropped when GPT omits isEmpty.
This matcher and the impressionViewable one at line 264 both require cycle.isEmpty === false. The observer normalizes a non-boolean isEmpty to undefined, and the overlay already treats that state as "Pending render" rather than "not filled" — so a legitimate slotOnload on such a cycle becomes unmatched plus a no_compatible_request_cycle issue instead of being recorded.
isEmpty !== true keeps the conservative intent — never attach a load to a known-empty cycle — without discarding the ambiguous one:
| cycle.renderAtMs !== undefined && cycle.isEmpty === false && cycle.loadAtMs === undefined, | |
| cycle.renderAtMs !== undefined && cycle.isEmpty !== true && cycle.loadAtMs === undefined, |
|
|
||
| const element = candidate as HTMLElement; | ||
| const escape = this.window.CSS?.escape; | ||
| if (typeof escape !== 'function') { |
There was a problem hiding this comment.
♻️ refactor — A missing CSS.escape is reported to the operator as a duplicate DOM ID.
When CSS.escape is unavailable this returns ambiguous / duplicate_dom_id, and GptDiagnosticsBindingReason is part of the exported JSON, so an operator debugging an unbound slot is told they have an ID collision that does not exist.
A distinct reason in core/types.ts would keep the export honest:
export type GptDiagnosticsBindingReason =
| 'missing_slot_element_id'
| 'missing_element'
| 'duplicate_dom_id'
| 'duplicate_check_unavailable'
| 'duplicate_gpt_slot_id';| badge.className = 'tsgd-badge'; | ||
| badge.dataset.runtimeSlot = String(slot.runtimeSlotNumber); | ||
| badge.textContent = badgeText(cycle); | ||
| badge.style.left = `${Math.max(4, Math.min(rectangle.left, this.window.innerWidth - 264))}px`; |
There was a problem hiding this comment.
⛏ nitpick — 264 duplicates the stylesheet's max-width: 260px plus margin. The two will drift the next time the badge width changes; a shared constant would keep them together.
2 participants
Summary
Changes
crates/trusted-server-core/src/integrations/gpt_diagnostics*ts_consoleactivation bootstrap.crates/trusted-server-core/src/{config,html_processor,migration_guards}.rscrates/trusted-server-js/lib/src/integrations/gpt_diagnostics/crates/trusted-server-js/lib/src/core/crates/trusted-server-js/lib/test/integrations/gpt_diagnostics/crates/trusted-server-integration-tests/docs/guide/integrations/gpt-diagnostics.mddocs/superpowers/{specs,plans}/2026-07-28-gpt-runtime-diagnostics-overlay*trusted-server.example.tomlCloses
Closes #973
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 servecargo test-cloudflare && cargo test-spincargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity./scripts/test-cli.shCI=1 ./scripts/integration-tests.sh --nocapture./scripts/integration-tests-browser.shcd crates/trusted-server-js/lib && node build-all.mjscd docs && npm run buildLive-publisher acceptance remains pending because no source-controlled publisher URL or credentials are available. The deterministic browser fixture covers the same activation, lifecycle, binding, export, privacy, and non-interference boundaries locally.
Checklist
unwrap()in production code — useexpect("should ...")println!