Clarify GPT auction diagnostics evidence - #1154
ChristianPavilonis wants to merge 2 commits into
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.
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
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