Skip to content

Clarify GPT auction diagnostics evidence - #1154

Open
ChristianPavilonis wants to merge 2 commits into
feature/ts-console-improvementsfrom
feature/ts-console-clarity
Open

ChristianPavilonis wants to merge 2 commits 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.

Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread docs/guide/integrations/gpt-diagnostics.md Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/prebid/index.ts

@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 thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/badges.ts Outdated
Comment thread crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/overlay.ts Outdated
@aram356 aram356 added this to the 202609 milestone Sep 14, 2026
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