Skip to content

Clarify GPT auction diagnostics evidence - #1154

Open
ChristianPavilonis wants to merge 1 commit into
feature/ts-console-improvementsfrom
feature/ts-console-clarity
Open

Clarify GPT auction diagnostics evidence#1154
ChristianPavilonis wants to merge 1 commit into
feature/ts-console-improvementsfrom
feature/ts-console-clarity

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • classify GPT requests only from observed auction evidence and keep server and browser timing clocks separate
  • distinguish auction winners, Prebid candidates, delivery evidence, and confirmed served creatives
  • add accessible Ad #N · Request #M navigation between page badges and request panels
  • correlate Prebid diagnostics with Prebid's auction IDs without changing behavior when diagnostics are inactive
  • reorganize panel facts and document every visible diagnostics label

This PR is stacked on #1121.

Validation

  • cargo test-fastly
  • cargo test-axum
  • cargo test-cloudflare
  • cargo test-spin
  • parity integration tests
  • all configured Clippy targets
  • cargo fmt --all -- --check
  • JavaScript Vitest suite, 904 tests
  • JavaScript ESLint, Prettier, and bundle build
  • documentation formatting, lint, and VitePress build
  • Playwright discovered all 3 GPT diagnostics browser tests

The Playwright browser tests could not execute locally because Docker access was denied at /var/run/docker.sock.

Closes #1081

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  • deliveryFact default arm returns undefined from a string function — see inline at overlay.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-label hides the status text from assistive tech — see inline at badges.ts:221
  • ♻️ prebidAuction deep-clone is written three times — see inline at api.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
  • 📝 recordPrebidAuction depends on being called after recordPrebidRefresh — see inline at store.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
  • 🌱 bidWon expiry and navigation-generation guards are untested — see inline at prebid/index.ts:534

Cross-cutting / body-level findings

  • 🏕 api.test.ts fake stores no longer satisfy ApiStore — the object literals passed to GptDiagnosticsApiController (around lines 82, 107, 150, 193, 262, 491) lack recordPrebidAuction and recordPrebidWin, so npx tsc --noEmit reports new TS2345 errors in that file. CI does not run tsc, 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');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrenchrender() 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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrenchdeliveryFact 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)

Suggested change
return unhandledCase(cycle.delivery);
return unhandledCase(cycle.delivery) ?? 'Delivery evidence: Not observed';

Comment on lines 273 to 274
| `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 |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 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 prints Delivery evidence: Not applicable (and the overlay test asserts it).

(prettier-normalized; npm run format in docs/ passes with these bytes)

Suggested change
| `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 |

Comment on lines +221 to +225
badge.textContent = `Ad #${slot.runtimeSlotNumber} · Request #${cycle.requestNumber} · ${badgeText(cycle)}`;
badge.setAttribute(
'aria-label',
`Open diagnostics for Ad #${slot.runtimeSlotNumber}, Request #${cycle.requestNumber}`
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactoraria-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)

Suggested change
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'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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', {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 noterecordRequestIntentSource 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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 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 } } : {}),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ 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 ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 653 to 657
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)
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 [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.

Comment on lines +216 to +220
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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 [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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 [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:

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improvements to TS_CONSOLE for ad observability

3 participants