feat(ads): measure the in-banner video too, on the path that can carry it - #320
Merged
Merged
Conversation
…y it #315 measured the streaming pre-roll. The other video ad is the one /ad.js serves: #316's media rotation fills a display slot as a muted looping MP4, and that unit reported exactly what a static banner does — one impression, one click, nothing about whether a frame ever ran. It is live: 6 of the last 3 hours' impressions were media='video'. An in-banner video is not a pre-roll and is deliberately not measured as one. It loops, so only the FIRST loop counts (a unit left on screen would otherwise report a completion every five seconds and a rate over 100%, which this network has been bitten by before). It autoplays wherever it is, so a start requires the unit to be at least half visible. And `placement` is now a funnel column rather than something averaged away: a pre-roll someone waited through and a muted loop in the corner of a page do not share a completion rate. Where the script goes was the real constraint. ad.js injects creatives into a srcdoc iframe sandboxed WITHOUT allow-scripts and WITHOUT allow-same-origin, so nothing can run inside it and the parent cannot reach the media element either. Granting scripts to advertiser-derived markup to collect a statistic is not a trade worth making, so that path is left alone and stays unmeasurable. /api/ads/frame is a real cross-origin document with its own CSP, so the beacon lives there — and only on a video or audio fill, so every static banner is as script-free as it has always been, and the publisher's page still runs none of our JavaScript either way. Beacons are image requests, governed by the img-src an ad unit already needs, rather than a fetch needing a connect-src nobody granted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan49 finding(s) HIGH/CRITICAL: 2 | MEDIUM: 32 | LOW: 15
Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Sep 25, 2026
…ate (#322) #316 records which medium every fill was served as. Nothing read it, which made the rotation randomised delivery that taught nothing. This adds the read. A rollup rather than a query over raw events, for the reason 20260902140000 exists: ad_impressions is ~376k rows growing ~90k/day, a 30-day window selects about half the table, and the 8s statement_timeout on `authenticated` was already cancelling reporting RPCs that scanned it. A `group by media` over the same range walks straight back into that. Grain is (owner_id, day, media) — daily, not the account series' hourly, because this is a comparison table and nothing here is plotted at 4-hour buckets. The part worth arguing about is what the card does NOT show. CTR per medium is the obvious report and it is unreadable on this network: every slot and every campaign belong to one account, so clicks book as free self-deal and there has not been a valid click since 2026-07-29. Five rows of 0.000% is not a neutral presentation of that — it reads as a finished experiment that found motion worthless. So the rate column is withheld below 30 attributed clicks and replaced by a note naming the structural cause, because "not enough data yet" would tell the reader to wait and waiting will not fix it. Playback is the signal that does discriminate today (#320), and the note says so. Two denominators that would each have read as a real number if got wrong: * share is denominated on ROTATED delivery, not on everything in the window. The pre-rotation archive is far larger than anything the rotation has served, so including it would show all five arms at ~0% indefinitely. * a NULL media is 'unknown', never 'static'. Folding the archive into static would make static the permanent winner of an experiment it never ran in. Clicks whose impression_id no longer resolves land there too — ad_clicks is `on delete set null` and serveAd synthesises an id when its insert failed. A click's medium comes from the impression it came from, not from its creative: the same creative serves several arms, so inferring it would be wrong by construction. Migration applied on dev2 ahead of this, with the PostgREST schema reload. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #315/#317, after Anthony pointed at
/ad.jsrather thanstats.js.Two corrections to what I said earlier in that thread, both mine:
ad_creatives.mediainstead of the real columns,ad_impressions.mediaandad_slots.media_mix.static; the actual last-3-hours split is static 336, image 56, gif 26, audio 8, video 6.The gap
#315 measured the streaming pre-roll. The other video ad is the one
/ad.jsserves: #316's media rotation fills a display slot as a muted looping MP4, and that unit reports exactly what a static banner does — one impression, one click, nothing about whether a frame ever ran.An in-banner video is not a pre-roll
Measuring it as one would corrupt the numbers, so it isn't:
startrequires the unit at least half visible. A muted video playing below the fold is not a view, and counting it as one turns the funnel into a measure of how much inventory is off-screen.abandonis recorded but means "the page went away", not "the viewer bailed".placementis now a funnel column rather than something averaged away, across the RPCs, the API, the CLI, MCP and the dashboard card.Where the script goes was the real constraint
ad.jsinjects creatives into a srcdoc iframe sandboxed withoutallow-scriptsand withoutallow-same-origin. Nothing can run inside it, and the parent cannot reach the media element either (opaque origin). Granting scripts to advertiser-derived markup to collect a statistic is not a trade worth making, so that path is deliberately left unmeasurable — see the open question below./api/ads/frameis a real cross-origin document with its own CSP, so the beacon lives there, and only on a video or audio fill — every static banner stays exactly as script-free as it is today, and the publisher's page runs none of our JavaScript either way, which is that route's whole promise.Beacons are image requests, not fetches: governed by the
img-srcan ad unit already needs, rather than aconnect-srcno publisher has granted. Hence the newGET /api/ads/video/eventspixel form alongside the existing POST.Verification
npm run typecheckcleannpx vitest run— 2,686 passed, 2 skipped, 0 failed (11 new)next build --webpackcompilesplacement, and it ends withnotify pgrst, 'reload schema'so it cannot repeat the silent no-op from fix(ads): say so when a pre-roll decision cannot be recorded #317Open question for a human
In-banner video on the
ad.jssrcdoc path stays unmeasurable unless we addallow-scriptsto that sandbox. That widens what advertiser-derived markup can do, so I did not make that call. The alternatives are to leave it (publishers on/api/ads/frameget playback,ad.jspublishers get medium-level impressions only), or to move more publishers to the frame embed.🤖 Generated with Claude Code