Clarify GPT auction diagnostics evidence - #1154
Conversation
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid direction: the page-bids handler now mirrors the SSAT dispatch path and shares the request-scoped RequestTimings T0, the store only classifies auctions from explicit evidence, and the new dictionary's bounds all match the store constants. The w/h removal in AuctionBidData also fixes two duplicate-identifier type errors that existed on the base. The blocking items are all on the overlay and docs: two new <details> sections lose their open state on every store update, the delivery switch introduces a strict-mode type error, and the delivery table still documents the pre-rename wording.
3 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- Technical details and help sections collapse on every store update — see inline at
overlay.ts:872 deliveryFactdefault arm returnsundefinedfrom astringfunction — see inline atoverlay.ts:164- Delivery table still documents the pre-rename panel wording — see inline at
gpt-diagnostics.md:273
Non-blocking
♻️ refactor / 🤔 thinking / 📝 note / ⛏ nitpick / 🌱 seedling
- ♻️ Badge
aria-labelhides the status text from assistive tech — see inline atbadges.ts:221 - ♻️
prebidAuctiondeep-clone is written three times — see inline atapi.ts:83 - 🤔 "(currency not supplied)" renders on every price line — see inline at
overlay.ts:256 - 🤔 Selecting a previous request pins its history open with no way to clear — see inline at
overlay.ts:770 - 📝
recordPrebidAuctiondepends on being called afterrecordPrebidRefresh— see inline atstore.ts:485 - 📝 Badges now intercept clicks over the creative — see inline at
overlay.ts:104 - ⛏ Binding line duplicated between "Size and visibility" and Technical details — see inline at
overlay.ts:878 - 🌱
bidWonexpiry and navigation-generation guards are untested — see inline atprebid/index.ts:534
Cross-cutting / body-level findings
- 🏕
api.test.tsfake stores no longer satisfyApiStore— the object literals passed toGptDiagnosticsApiController(around lines 82, 107, 150, 193, 262, 491) lackrecordPrebidAuctionandrecordPrebidWin, sonpx tsc --noEmitreports newTS2345errors in that file. CI does not runtsc, so this passed, but extending the fakes keeps the test file honest against the interface it exercises.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (ts CLI, native): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo test (axum native): PASS
- format-docs: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
Branch protection reports no required checks for this PR.
| container.append(history); | ||
| } | ||
|
|
||
| const technical = this.document.createElement('details'); |
There was a problem hiding this comment.
🔧 wrench — render() calls panel.replaceChildren() on every store notification and only restores .tsgd-history[open], so this new Technical details <details> and the How to read this evidence <details> in the toolbar both snap shut on the next GPT callback. I confirmed it with a scratch vitest: open both, call store.recordSlotResponseReceived(slot), run the frame, and both read open === false. On a live page that is every slotRequested/slotResponseReceived/slotRenderEnded, so an operator cannot keep either section open.
Proposed fix (apply manually — touches the class fields, the capture block at the top of render(), the help block, and this technical block, so it spans several hunks):
// class field
private helpOpen = false;
// render(): capture next to openHistorySlots, before panel.replaceChildren()
const openTechnicalSlots = new Set(
Array.from(panel.querySelectorAll<HTMLDetailsElement>('.tsgd-technical[open]'))
.map((details) => details.closest<HTMLElement>('.tsgd-slot')?.dataset.runtimeSlot)
.filter((runtimeSlot): runtimeSlot is string => runtimeSlot !== undefined)
);
// help block in render()
help.open = this.helpOpen;
help.addEventListener('toggle', () => {
this.helpOpen = help.open;
});
// renderSlot(slot, historyOpen, technicalOpen)
const technical = this.document.createElement('details');
technical.className = 'tsgd-technical';
technical.open = technicalOpen;Then pass openTechnicalSlots.has(String(slot.runtimeSlotNumber)) from the render loop, and add a case next to the existing "preserves history open state" test that opens both sections, records a callback, and asserts they stay open.
| return undefined; | ||
| return 'Delivery evidence: Not observed'; | ||
| default: | ||
| return unhandledCase(cycle.delivery); |
There was a problem hiding this comment.
🔧 wrench — deliveryFact is now declared to return string, but unhandledCase returns undefined, so this arm is a strict-mode error (TS2322: Type 'undefined' is not assignable to type 'string'). It is new in this PR (tsc --noEmit on the base has no error in this file). CI only runs eslint, which is why it passed, but it defeats the exhaustiveness guard: anyone who runs tsc now sees a permanent error on the one line whose job is to surface a missing case.
Keeping the guard and satisfying the declared type costs one ??; the runtime path is unchanged because the default arm is unreachable once the switch is exhaustive.
(compile-verified: tsc error count drops from 187 to 186, eslint and prettier clean, the 175 gpt_diagnostics tests pass)
| return unhandledCase(cycle.delivery); | |
| return unhandledCase(cycle.delivery) ?? 'Delivery evidence: Not observed'; |
| | `no_candidate` | adInit observed no direct Trusted Server candidate for this request | | ||
| | `unknown` | Delivery status unknown — required GPT or direct-candidate evidence was not observed | |
There was a problem hiding this comment.
🔧 wrench — These two rows still carry the pre-rename panel wording. deliveryFact now renders No direct Trusted Server candidate and Delivery status unknown — required evidence was not observed, and the new dictionary page documents those strings, so this table contradicts both.
Two more spots in this file sit outside the diff and need a manual edit:
- line 151 still says the label “Competing auctions”; the panel label is now “Multiple auction paths observed”.
- row 276 (
not_applicable) says no delivery conclusion is displayed, but the panel now printsDelivery evidence: Not applicable(and the overlay test asserts it).
(prettier-normalized; npm run format in docs/ passes with these bytes)
| | `no_candidate` | adInit observed no direct Trusted Server candidate for this request | | |
| | `unknown` | Delivery status unknown — required GPT or direct-candidate evidence was not observed | | |
| | `no_candidate` | No direct Trusted Server candidate | | |
| | `unknown` | Delivery status unknown — required evidence was not observed | |
| badge.textContent = `Ad #${slot.runtimeSlotNumber} · Request #${cycle.requestNumber} · ${badgeText(cycle)}`; | ||
| badge.setAttribute( | ||
| 'aria-label', | ||
| `Open diagnostics for Ad #${slot.runtimeSlotNumber}, Request #${cycle.requestNumber}` | ||
| ); |
There was a problem hiding this comment.
♻️ refactor — aria-label replaces the button's accessible name, so a screen-reader user hears only “Open diagnostics for Ad #1, Request #2” while a sighted user sees “Filled · SSAT · TS response sent”. That drops the status the badge exists to show, and the visible text is no longer contained in the accessible name (WCAG 2.5.3 Label in Name). Appending the same badgeText to the label keeps the verb and restores the status; the existing regex assertions in badges.test.ts and the Playwright spec still match.
(verified: gpt_diagnostics tests, eslint and prettier pass)
| badge.textContent = `Ad #${slot.runtimeSlotNumber} · Request #${cycle.requestNumber} · ${badgeText(cycle)}`; | |
| badge.setAttribute( | |
| 'aria-label', | |
| `Open diagnostics for Ad #${slot.runtimeSlotNumber}, Request #${cycle.requestNumber}` | |
| ); | |
| const text = badgeText(cycle); | |
| badge.textContent = `Ad #${slot.runtimeSlotNumber} · Request #${cycle.requestNumber} · ${text}`; | |
| badge.setAttribute( | |
| 'aria-label', | |
| `Open diagnostics for Ad #${slot.runtimeSlotNumber}, Request #${cycle.requestNumber}: ${text}` | |
| ); |
| technical.append(technicalSummary); | ||
| appendFacts(this.document, technical, [ | ||
| slot.adUnitPath ? `Ad unit ${slot.adUnitPath}` : 'Ad unit: Unavailable', | ||
| binding.binding.status === 'bound' |
There was a problem hiding this comment.
⛏ nitpick — The “Size and visibility” group above already renders Binding: Bound · Visible / Binding: <status> · <reason> for the same slot, so this line repeats it in Technical details. The new dictionary page says “Technical details does not repeat those grouped facts”, so one of the two should go (or the dictionary sentence should). Note the existing overlay test asserts the literal Ambiguous binding, which only this copy produces, so dropping it needs the test updated too.
| content.append(this.renderSlot(slot, openHistorySlots.has(String(slot.runtimeSlotNumber)))); | ||
| const selectedPreviousRequest = | ||
| this.selectedRequest?.runtimeSlotNumber === slot.runtimeSlotNumber && | ||
| this.selectedRequest.requestNumber !== latestCycle(slot)?.requestNumber; |
There was a problem hiding this comment.
🤔 thinking — Once a badge selects a previous request, selectedPreviousRequest forces that slot's history <details> open on every render, and nothing ever clears this.selectedRequest. If the operator collapses the history, the next store update re-opens it, and there is no control to deselect. Either respect the user's collapse after the first render (only force-open when the selection changes) or add a way to clear the selection.
| const normalizedId = normalizedAuctionId(auctionId); | ||
| if (!normalizedId) return; | ||
| const candidate = normalizedAuctionWinner(targetingCandidate); | ||
| this.recordRequestIntentSource(slot, 'prebid_refresh', { |
There was a problem hiding this comment.
📝 note — recordRequestIntentSource does intent.sources.set(source, { observedAtMs, ...facts }), so a later recordPrebidRefresh([slot]) would wipe the prebidAuction recorded here. It works today only because completeRefresh in prebid/index.ts calls recordPrebidRefreshForDiagnostics before recordCompletedPrebidAuction. Worth either merging into the existing entry ({ ...intent.sources.get(source), observedAtMs, ...facts }) or a comment plus a store test that pins the order dependency.
| .tsgd-badge-layer { position: fixed; z-index: 2147483646; inset: 0; pointer-events: none; } | ||
| .tsgd-badge { | ||
| position: fixed; | ||
| pointer-events: auto; |
There was a problem hiding this comment.
📝 note — With pointer-events: auto the badge (now a <button>) sits over the creative's top-left corner and swallows clicks there while a diagnostics session is active; previously the layer let them through to the ad. Fine for an opt-in operator tool, but worth one sentence in the docs so nobody reads a click-through gap in a diagnostics session as a delivery bug.
| @@ -70,6 +81,17 @@ function cloneExportSnapshot(snapshot: GptDiagnosticsExportV1): GptDiagnosticsEx | |||
| size: cycle.size ? [...cycle.size] : undefined, | |||
| observedSlotSize: cycle.observedSlotSize ? [...cycle.observedSlotSize] : undefined, | |||
| ...(cycle.auctionWinner ? { auctionWinner: { ...cycle.auctionWinner } } : {}), | |||
There was a problem hiding this comment.
♻️ refactor — This prebidAuction spread-clone block is now written three times: here in cloneExportSnapshot, again in the controller's snapshot() (around line 235), and in copyCycle in store.ts (around line 371). A small clonePrebidAuction(evidence) helper next to the type would keep the three from drifting. Apply manually — can't be auto-applied as a suggestion because it touches two files.
| prebidDiagnosticAttempts.delete(key); | ||
| if ( | ||
| !attempt || | ||
| performance.now() > attempt.expiresAtMs || |
There was a problem hiding this comment.
🌱 seedling — The bidWon listener rejects late events (expiresAtMs) and events from a prior navigation (navGeneration mismatch), and the 128-entry eviction loop above bounds the map, but prebid/index.test.ts only exercises the wrong-auction-id path. A follow-up test that advances performance.now() past 30 s and one that bumps navGeneration before firing bidWon would pin those guards, since the dictionary documents both as rejection reasons.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The evidence model and request-relative timing changes are supported by passing tests. This pass confirms the existing details-state and TypeScript findings and identifies a separate keyboard-focus regression in the new interactive badges.
Blocking
- 🔧 [P2] Preserve help and technical-details expansion across live updates — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:657. - 🔧 [P2] Keep keyboard focus when refreshing badge positions — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts:220. - 🔧 [P2] Return a string from the delivery fallback — see inline at
crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts:164.
Validation and scope
At the original head, 445 focused GPT/Prebid/diagnostics tests passed. Scratch DOM probes reproduced both UI failures. The exact one-line suggestion was applied in isolation: all 904 JavaScript tests, full JavaScript formatting, and the 13-module bundle build passed. An isolated strict-type probe fails before and passes after that change; full-project TypeScript has other existing errors. Rust/browser gates were not repeated locally. Coverage includes the changed runtime paths and relevant surrounding code, not every unchanged line of the large publisher and test files.
One inline comment includes a verified one-click suggestion. The remaining fixes span multiple locations and need manual changes. The existing delivery-table wording and API-fake typing observations remain open; this review does not duplicate those inline threads.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (ts CLI, native): PASS
- cargo test: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- cargo test (axum native): PASS
- format-docs: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo fmt: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
| const openHistorySlots = new Set( | ||
| Array.from(panel.querySelectorAll<HTMLDetailsElement>('.tsgd-slot details[open]')) | ||
| Array.from(panel.querySelectorAll<HTMLDetailsElement>('.tsgd-history[open]')) | ||
| .map((details) => details.closest<HTMLElement>('.tsgd-slot')?.dataset.runtimeSlot) | ||
| .filter((runtimeSlot): runtimeSlot is string => runtimeSlot !== undefined) | ||
| ); |
There was a problem hiding this comment.
🔧 [P2] Preserve help and technical-details expansion across live updates
Only .tsgd-history[open] is saved before replacing every panel child. The newly added help and technical-details elements are recreated closed, so a slot response, visibility callback, or binding update closes the material the operator is reading. A scratch JSDOM probe against the actual bundled head opened both sections and called store.recordSlotResponseReceived(slot): both switched from open=true to open=false. This confirms Aram's existing finding on this exact head.
Fix: retain the help state and technical-details state keyed by runtime slot, restoring them after rendering, just as request-history expansion is retained. Add a live-update regression for both sections.
Illustrative implementation shape:
const helpOpen = panel.querySelector<HTMLDetailsElement>('.tsgd-help')?.open ?? false;
const openTechnicalSlots = new Set(
Array.from(panel.querySelectorAll<HTMLDetailsElement>('.tsgd-technical[open]'))
.map((details) => details.closest<HTMLElement>('.tsgd-slot')?.dataset.runtimeSlot)
);
// Pass the stored state through renderSlot; restore help.open and technical.open.Apply manually: spans state capture, renderSlot plumbing, details creation, and regression tests in separate locations; not a one-click suggestion.
| const badge = this.document.createElement('button'); | ||
| badge.type = 'button'; | ||
| badge.className = 'tsgd-badge'; | ||
| badge.dataset.runtimeSlot = String(slot.runtimeSlotNumber); | ||
| badge.textContent = `Ad #${slot.runtimeSlotNumber} · ${badgeText(cycle)}`; | ||
| badge.dataset.requestNumber = String(cycle.requestNumber); |
There was a problem hiding this comment.
🔧 [P2] Keep keyboard focus when refreshing badge positions
The PR makes badges keyboard-operable buttons, but update() still recreates all badges and removes the originals at lines 246–247. Updates run on scroll, resize, binding changes, and every store notification. Consequently a focused button is detached and focus returns to the document before Enter/Space can activate it. Keyboard navigation can itself scroll the viewport and trigger this. The new browser test checks immediate focus/Enter only, without an intervening update.
Scratch probe on actual bundled head: focus the visible badge, dispatch a window scroll event, flush the scheduled frame. Result: old badge isConnected=false, document.activeElement.tagName='BODY', replacement badge unfocused. This is separate from the existing aria-label content observation.
Fix: reconcile badge DOM nodes by stable slot identity, updating data/text/geometry in place. If replacing nodes, capture focus within the actual closed ShadowRoot and restore it to the same surviving slot/request without stealing focus when a badge disappears. Add scroll and store-update keyboard tests.
Illustrative reconciliation shape:
const key = String(slot.runtimeSlotNumber);
const badge = existingBadges.get(key) ?? this.document.createElement('button');
// Update the retained button and its current request activation target in place.
// Remove only badges that no longer correspond to visible bound slots.Apply manually: requires changing badge creation/removal and listener ownership plus regression coverage outside this hunk.
| return undefined; | ||
| return 'Delivery evidence: Not observed'; | ||
| default: | ||
| return unhandledCase(cycle.delivery); |
There was a problem hiding this comment.
🔧 [P2] Return a string from the delivery fallback
deliveryFact now promises string, but unhandledCase returns undefined. Strict TypeScript reports TS2322 at this return. All valid enum members are handled, but the helper's return signature still makes the changed function ill-typed. The explicit safe fallback preserves the never-parameter exhaustiveness check and the declared string contract. This confirms Aram's existing finding.
Proposed correction:
| return unhandledCase(cycle.delivery); | |
| return unhandledCase(cycle.delivery) ?? 'Delivery evidence: Not observed'; |
Verification: isolated strict-type probe fails before and passes after this change. Applied this exact line alone: all 904 JavaScript tests, full JavaScript formatting, and all 13 bundle builds passed. Other existing full-project TypeScript errors remain.
Summary
Ad #N · Request #Mnavigation between page badges and request panelsThis PR is stacked on #1121.
Validation
cargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincargo fmt --all -- --checkThe Playwright browser tests could not execute locally because Docker access was denied at
/var/run/docker.sock.Closes #1081