feat: expand programmatic ad placements on /articles - #6529
Conversation
- Internal ads leave the template: PostEngagements gains hideInternalAd (drops AdAsComment) and PostWidgets gains hideAdWidget (drops PostSidebarAdWidget), both opt-in so every other surface is unchanged. - The article body carries an MPU per ~250 words of visible text via splitContentForAds, which only cuts where a top-level block closes — never inside a paragraph, list or code block — and folds a trailing sliver into the previous chunk so a page cannot end on an ad. - An MPU sits directly above the comment section, and the thread interleave becomes an MPU every 7 comments (was a native every 5 that never served for want of its layoutKey). - The rail's mid-column sticky (which slid over the widgets below it) is in flow again; a fixed 300x600 closes the rail as the page's ONLY sticky unit — compliant as a publisher sticky: 300px wide, desktop only, one per viewport, nothing after it to cover. - Every unit carries the policy-permitted "Advertisements" label, small and gray, top-right, inside the collapsing wrapper. - Units request 50px before entering the viewport and render centered on a lighter background block. - The cover-side MPU (slot 3) retires, with 4, 5 and 6; the map now reuses live Display units for the new placements (first-party events split per slot; dedicated units are a TODO for clean AdSense rows). - PostAnsweredQuestions renders after the body, as on the post page — it self-hides for logged-in users, matching this page's audience. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Re-posted from #6528 (same head commit b80fa25), plus one addition on the new "where to verify" note.
Ten-point ad iteration on the /articles template: rail trimmed to two units, in-body MPU cadence, MPU above comments, comment interleave switched from a never-served native to an MPU every 7, a single closing sticky half page, universal ad label, 50px lazy margin, and the questions block. Scope is coherent and splitContentForAds is properly unit-tested.
Four blocking items, all in ProgrammaticAd and in ad density — three of them are regressions to units the PR did not intend to change, because ProgrammaticAd is shared by every slot on the template (leaderboard, rail, in-body, comments).
Blocking
ProgrammaticAd.tsx:490— the newp-2shrinks every unit's usable width below its IAB cap (border-box), which theFORMAT_SPECdocblock says the cap exists to guarantee.ProgrammaticAd.tsx:505— label + padding are not in the reservedminHeight, so the box grows in-viewport when the slot requests.ProgrammaticAd.tsx:326—rootMargin: 50pxmoves both the unfilled collapse and the auction into the visible area.slots.ts:62/70— the phone-density precondition the previous code gated the comment unit behind was deleted rather than satisfied, while repeating in-body units were added with no phone cap.
Non-blocking
Raised inline on splitContentForAds.ts, ArbitragePostContent.tsx and slots.ts: pre-sanitization regex parsing vs. what Markdown actually renders, missing memoisation, per-chunk Markdown/DOMPurify/HoverCard cost, per-occurrence analytics, and doc drift.
Three description claims worth double-checking:
- "the page's only sticky":
ArbitrageTopLeaderboardislaptop:stickyfor its first 10 seconds, so on a short article a fast desktop scroll can have two publisher stickies pinned at once — and the pinned leaderboard is well over the 300px width the same reasoning invokes. - the new verification note says the in-body MPUs "only exist on posts whose article body we host", but
/articlesexplicitly restricts itself toArticle,VideoYouTubeandCollectionand excludes freeform — so "verify on a freeform post" is not reachable on this route. Please name a real article id with a rendered body that a reviewer can open. - point 8, "already sticky on mobile": the leaderboard's sticky class is
laptop:stickyonly. Please confirm where the mobile pinning actually comes from before reporting that point as shipped.
Verification
- Read root/package AGENTS.md-relevant conventions, traced every
ProgrammaticAdconsumer (ArbitrageAdSlot→ read + organic surfaces). - CI:
test_shared,test_webapp,test_extension,lint_shared,typecheck_strict_changedgreen at review time;buildstill pending. - Not verified by me: rendered widths and phone ad-density percentages on the preview — those are the blocking items and need a real measurement.
Verdict
Request changes — the shared-wrapper padding, the in-viewport shift/collapse and the phone density gate need resolving before this goes live.
Reviewed by AI.
| 'mx-auto w-full text-center', | ||
| // Centered on a lighter block, so the unit reads as a deliberate | ||
| // frame rather than a creative floating on the page background. | ||
| 'mx-auto w-full rounded-8 bg-surface-float p-2 text-center', |
There was a problem hiding this comment.
Blocking: p-2 on the wrapper reduces the usable width of every unit below its IAB cap, because Tailwind's preflight makes these boxes border-box. max-w-[300px] becomes 284px of content, max-w-[336px] becomes 320px, max-w-[728px] becomes 712px.
Consequences:
- Responsive units (
data-ad-format="rectangle"etc.) are sized by AdSense from the container width. At 284px a 300x250 no longer fits, so the flagship MPU on the in-body, above-comments, comment and rail placements degrades to a 250x250 — a direct RPM loss on the exact placements this PR is adding. The leaderboard loses 728x90 the same way at 712px. - The fixed
railBottomSticky(width: 300, height: 600,display: inline-block) is now a 300px child inside a 284px content box. The wrapper comment explicitly keeps overflow unclipped, so the creative spills 16px past its own background block — and past the 300px width the sticky-compliance argument inslots.tsrelies on.
This affects units the PR did not intend to touch, since every slot on the template renders through here. Suggested direction: keep the wrapper's box at the IAB width and put the frame/padding on an inner element around the <ins>, or widen maxWidth by the padding (max-w-[316px] / max-w-[352px] / max-w-[744px]) so the ad keeps its full width. Worth checking the rendered <ins> width on the preview before/after.
Reviewed by AI.
| unfilled slot's collapse takes the label down with it. */} | ||
| {isRequested && config.type === 'inFeed' && ( | ||
| <span className="mb-1 block text-left text-text-quaternary typo-caption2"> | ||
| {/* Every unit is labeled so none can be confused with site content — |
There was a problem hiding this comment.
Blocking (layout shift): the label now renders for every unit, and FORMAT_SPEC[format].minHeight reserves space for the creative alone (min-h-[250px] for a 300x250). With the label (~16px) plus p-2 (16px), a filled MPU needs ~282px, so the box grows by roughly 32px at the moment isRequested flips.
Combined with the new 50px rootMargin, that flip now happens when the slot is already on screen, i.e. the article jumps under the reader mid-paragraph — several times per article with the in-body cadence. Previously the label was inFeed-only and no live slot used that type, so nothing shifted.
Suggested direction: fold the label height and the padding into the reserved minHeight per format, or render the label unconditionally so the space is reserved from first paint.
Reviewed by AI.
| { rootMargin: '600px' }, | ||
| // Expert-tuned: request just as the slot approaches rather than a | ||
| // viewport ahead — viewability over prefetch. | ||
| { rootMargin: '50px' }, |
There was a problem hiding this comment.
Blocking (needs evidence or a middle value): two behaviours regress at 50px, both of which the previous 600px margin was deliberately avoiding, and the comment immediately above this line still describes the old value ("roughly a viewport of scroll — enough for the auction round-trip to finish before the slot scrolls into view").
has-[>ins[data-ad-status="unfilled"]]:!hiddencollapses an unfilled slot. At 600px that verdict landed before the box was ever visible; at 50px the reserved 250px box collapses while the reader is looking at it, pulling the rest of the article up. This is the failure mode the collapse rule's own comment was trying to avoid, just moved into the viewport.- The AdSense request now starts ~50px before the slot enters view, so the auction resolves after it is on screen. On a normal scroll the slot is already past by the time the creative lands, which loses the impression rather than improving viewability — the opposite of the stated intent.
If the expert's recommendation is specifically about viewable-impression rate, a middle value (200-300px) gets most of it without collapsing in view. Otherwise, please note the source/measurement so the trade-off is recorded, and update the stale comment above either way.
Reviewed by AI.
| * says 6-8). Short threads never reach the interval, so they stay ad-free. | ||
| */ | ||
| export const COMMENTS_PER_INTERLEAVED_AD = 5; | ||
| export const COMMENTS_PER_INTERLEAVED_AD = 7; |
There was a problem hiding this comment.
Blocking (density gate removed, not satisfied): the version of this map being replaced kept slot 7 collapsed behind an explicit written precondition — "Re-measure phone ad density on a long thread with the interval live. The ~27% figure was measured with this slot inert... Chrome's Better Ads filter applies to the whole domain, direct-sold inventory included."
This PR fills the unit id (activating that repeating unit) and additionally adds an unbounded run of in-body MPUs with no hideOnPhone, plus the above-comments MPU. The RAIL_AD docblock in ArbitragePostContent records that the unfiltered phone run measured ~40% (and ArbitrageAdSlot's hideOnPhone doc says 56%) against the 30% cap, which is why all but one rail unit are desktop-only.
So the page now has more repeating phone inventory than when the gate was written, and the gate itself was deleted along with the comment. Please post a fresh measured phone density on a long article with a long thread before this ships, and consider a hard cap on in-body units (or hideOnPhone on all but the first) so density cannot scale with article length.
Reviewed by AI.
| * A short tail is merged into the chunk before it, so the article never ends | ||
| * on an ad followed by a stray line. | ||
| */ | ||
| export function splitContentForAds(html: string, minChars: number): string[] { |
There was a problem hiding this comment.
Non-blocking (correctness of the split input): this parses post.contentHtml with a regex, but what actually renders is purify.sanitize(chunk) inside Markdown — each chunk is sanitized and repaired independently, so the string the boundaries were computed on is not the string that ships. Two concrete edges:
- HTML comments do not match
TAG_RE(it requires[a-zA-Z]after the optional/), but tags inside a comment do, so<!-- <div> -->in the body permanently offsetsdepthand the function stops finding top-level boundaries for the rest of the article — silently, with no ads and no error. - Any unbalanced markup in the source has the same effect, and DOMPurify would have fixed it before render.
A <template>/DOM pass over the sanitized HTML (or splitting after sanitization) would make boundaries match what renders and remove the depth bookkeeping entirely. If the regex stays, please note why the DOM route was rejected and add specs for a comment in the body, plain text with no tags at all, and text before the first tag.
Reviewed by AI.
| top-level blocks — splitContentForAds cannot cut a paragraph, list | ||
| or code block in half. */} | ||
| {!!post.contentHtml && | ||
| splitContentForAds(post.contentHtml, BODY_CHARS_PER_AD).map( |
There was a problem hiding this comment.
Non-blocking: splitContentForAds runs on every render of this component — the whole article body re-scanned on each comment-sort change, hover state, auth resolution, etc. Wrap it in useMemo keyed on post.contentHtml.
Also worth confirming the per-chunk cost of Markdown: each instance runs its own DOMPurify pass and mounts its own HoverCard (a dynamic(ssr: false) import) plus the mention useQuery. A long article now instantiates that 5-8 times instead of once. If mention hovercards or image-modal delegation behave differently across chunk boundaries, that would show up here first — did you check a body with mentions and images on the preview?
Reviewed by AI.
| /> | ||
| {index < chunks.length - 1 && ( | ||
| <ArbitrageAdSlot | ||
| slot={ARBITRAGE_SLOT.inBodyMpu} |
There was a problem hiding this comment.
Non-blocking (analytics + polish): every in-body occurrence renders with the same slot number, so first-party events cannot distinguish the first in-body MPU from the sixth. Since the whole point of the cadence is finding where in-body inventory pays, consider passing the occurrence index through the log extra. It also means several elements share data-testid="adsense-slot-17".
Minor: this slot has no spacing class while the above-comments MPU gets my-6, and the surrounding <div className="mb-6"><div className="min-w-0 flex-1"> nesting above is left over from the removed flex row — it can collapse to one element now.
Reviewed by AI.
| @@ -292,11 +283,12 @@ export function ArbitragePostContent({ | |||
| post={post} | |||
| onCopyLinkClick={onCopyPostLink} | |||
| logOrigin={Origin.ArticlePage} | |||
There was a problem hiding this comment.
Non-blocking (doc drift): a few comments no longer match the code after this change and will mislead the next reader:
- here: "The only addition is a native unit every few comments" — it is an MPU now.
- the
RAIL_ADdocblock: "One slot per real rail widget, in render order... every widget there earns a unit" and the ~40% phone measurement, both written for the five-unit rail. ArbitrageAdSlot'shideOnPhonedoc still quotes the 56% figure from that layout.refreshesis now only used by the top leaderboard; worth a look at whether the prop still earns its place onArbitrageAdSlot.
Reviewed by AI.
Review blockers: - Wrapper padding is vertical-only — border-box horizontal padding shrank every unit below its IAB width cap, visibly misaligning the fixed 300x600 rail closer. - Every FORMAT_SPEC minHeight now reserves the label row and padding, so a box never grows under the reader when its request lands in-viewport. - rootMargin lands on 250px: the expert's 50px put both the auction and the unfilled collapse inside the visible area. - The phone-density gate is honored, not deleted: the comment MPU is phone-hidden in the template until a long-thread measurement clears it, and the in-body cadence's phone math is written at the map. Product calls folded in: - The TLDR carries the same MPU cadence as a hosted body — for a scraped article it is the only main content, split at sentence/word boundaries by splitTextForAds. - The comment interval counts every comment, replies included, 8 flat. - The ad label stays lowercase-styled "Advertisements" (the policy string; no CSS uppercase). Also: splits memoised on their inputs, HTML comments can no longer skew the splitter's depth count, per-occurrence and per-section analytics on repeated placements via logExtra, and the stale five-unit-rail docs are rewritten. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review + product-round addressed (983b413)Blockers:
Description-claim corrections: "the page's only sticky" → the rail's only sticky (the top leaderboard is a separate 10s-window pin, mobile included — its mobile pinning comes from the wrapper Product changes folded in from this round:
Non-blockers taken: memoised splits, comment-proof tokenizer ( Tests: 134 shared (11 splitter incl. comment-tokenizer + TLDR cases), webapp green except the pre-existing 🤖 Generated with Claude Code |
The 250-word cadence was written for article bodies; a TLDR is dense by construction and at that threshold a typical 150-200 word summary carried no in-content unit at all. Summaries now break every ~100 words (SUMMARY_CHARS_PER_AD = 600), calibrated against a real 160-word TLDR that product expected to carry one unit — it breaks about two-thirds through, at a sentence end. Bodies keep the 250-word brief cadence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
TLDR cadence calibrated against a real summary (see commit): summaries now carry an in-content MPU from roughly 150 words, breaking at a sentence end ~two-thirds through ( 🤖 Generated with Claude Code |
The greedy splitter cut at the first sentence end past the threshold, which on a two-part summary put the unit wherever the budget ran out (~70% through the calibration TLDR). The part count now comes from the target size and each break snaps to the sentence end nearest its even split point — one ad in a two-part summary sits at the middle sentence, as near to 50% as the text's own sentences allow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sity cap Nick's confirmed spec is 250 characters between in-content units, not 250 words. At that cadence the interval no longer bounds density (~3 lines of text per 282px unit), so MAX_CONTENT_ADS_PER_SECTION = 4 does: both splitters take a max-parts argument, the TLDR splitter folds it into its balanced part count and the body splitter merges overflow into the final chunk, so capped units spread evenly instead of front-loading. One constant to change if product wants a different ceiling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Cadence corrected to 250 characters (Nick's confirmed spec — the earlier 250-words reading came from the original brief image). Verified against the calibration TLDR: 4 balanced parts, 3 in-content MPUs, every break on a sentence end. Because a 250-char interval no longer bounds density (~3 lines of text per unit), 🤖 Generated with Claude Code |
The boundary-picking closures referenced the mutable loop cursor, which eslint rightly flags; a per-iteration const carries the same value safely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rebelchris
left a comment
There was a problem hiding this comment.
Re-review of 731f118
Three of the four blockers are resolved cleanly:
py-2instead ofp-2— every unit gets its full IAB width back, and the reasoning is now recorded next to the class. ✅rootMargin: 250pxwith the trade-off written out. ✅- Comment MPU is
hideOnPhoneand the density gate is written intoslots.tsrather than deleted; in-content units are capped. ✅ - Bonus:
logExtragives repeated placements per-occurrence analytics, the splits are memoised, HTML comments no longer corrupt the depth count, and the interleave now counts replies.
Two blocking items remain, both created by the switch to a literal 250-character cadence (which is a product call I'm not disputing — the concern is where the units land and how dense the phone page now is):
splitContentForAds.ts:83— the cap front-loads every in-body MPU into the first ~1000 characters of a long article.ArbitragePostContent.tsx:207— the TLDR now carries up to 4 phone-visible MPUs, which raises phone density above what the earlier measurement covered, and there is still no fresh number.
Non-blocking follow-ups inline: the height reservations are 4px short of the actual chrome, the inline logExtra object re-runs the ad effects on every render, countThrough is O(n²) per render, and one new test passes for the wrong reason.
Also worth updating before merge: the PR body still describes "~250 words (1500 chars)" and "an MPU every 7", and does not mention the TLDR placements, the per-section cap of 4, or that the comment unit is desktop-only. Whoever reads this in three months will be reading the description, not the diff.
Verification
- Traced the full delta
b80fa25...731f118, re-checked every consumer ofProgrammaticAd/PostComments(the interleave stays inert without both props, so the organic post page is unaffected). - Computed the chrome height from
typo-caption2(1rem line-height) +pb-1+py-2= 36px against the +32px applied. - CI at review time:
test_shared,test_webapp,lint_shared,buildand the Vercel webapp deploy pending; extension build/tests and strict typecheck green. - Not verified by me: rendered phone density on a scraped article with a long TLDR. That is blocking item 2 and needs a real measurement on the preview.
Verdict
Close. Fix the front-loading, post a phone density number for the TLDR cadence, and I'd be happy with this.
Reviewed by AI.
|
|
||
| // Density cap: everything past the last allowed boundary folds into the | ||
| // final chunk rather than earning more ads. | ||
| while (chunks.length > maxParts) { |
There was a problem hiding this comment.
Blocking (ads land in the wrong place): this fold makes the HTML path front-load. The scan is greedy — it cuts at the first top-level boundary past 250 visible characters — so a 5,000-character article produces ~20 chunks, and this loop then folds chunks 5..20 into chunk 5. Result: all four in-body MPUs sit inside the first ~1,000 characters (roughly the first four paragraphs), and the remaining 80% of the article carries none.
That is the opposite of the intent recorded in slots.ts: "The balanced splitters spread the capped units evenly through the section instead of front-loading them." That statement is true for splitTextForAds (which picks boundaries nearest even split points) and false for splitContentForAds. It is also the worst shape for the policy risk the same comment cites — a wall of ads at the top of the article, then nothing.
Suggested direction: give the HTML path the same balancing as the text path — collect every depth-0 boundary with its running visible offset, decide the part count from min(round(total / 250), cap), then choose the boundaries nearest each even split point. That reuses the logic you already wrote and makes the slots.ts comment true.
Reviewed by AI.
| refreshes | ||
| className="laptop:order-last" | ||
| /> | ||
| {summaryParts.map((part, index, parts) => ( |
There was a problem hiding this comment.
Blocking (phone density, now larger than at the last review): splitting the TLDR means a scraped article — no contentHtml, so the summary is the only content — can now render 4 MPUs inside ~1,000 characters of summary, all phone-visible, on top of the leaderboard, the above-comments MPU and the phone rail unit.
The last round moved the comment unit to hideOnPhone and wrote the gate into slots.ts, which was the right call, but the same gate applies here: MAX_CONTENT_ADS_PER_SECTION bounds the count per section, not the ad-to-content ratio, and 4 units against a short TLDR is exactly the ratio Better Ads (30% of page height) and AdSense's low-value-content policy act on — domain-wide, per the comment in slots.ts itself.
Two things would close it:
- A measured phone density figure on a scraped post with a long TLDR and on a long hosted article, replacing the old 27%/40% numbers the comments used to quote.
- A minimum-section guard so a section only earns ads once it has the text to carry them — e.g. no split below ~600 visible characters, and a floor on characters-per-ad independent of the cap.
Reviewed by AI.
| label: 'Medium rectangle', | ||
| size: '300x250', | ||
| minHeight: 'min-h-[250px]', | ||
| minHeight: 'min-h-[282px]', |
There was a problem hiding this comment.
Non-blocking (4px short): the reservations add 32px, but the chrome is 36px — typo-caption2 is line-height: 1rem (16px), the label also carries pb-1 (4px), and the wrapper adds py-2 (16px). The docblock above accounts for the label row and the padding but not the pb-1.
So a filled MPU is 286px inside a min-h-[282px] box and still grows 4px when the label appears. Values that match the actual chrome: MediumRectangle 286, Rectangle 286 / tablet 216, Leaderboard 136 / tablet 126, HalfPage 356, Native 132 — or drop pb-1 and keep the current numbers.
Reviewed by AI.
| surface, | ||
| refreshes, | ||
| extra, | ||
| extra: { ...logExtra, ...extra }, |
There was a problem hiding this comment.
Non-blocking (effect churn): logExtra is passed as an inline object literal at the call sites (logExtra={{ section: 'body', occurrence: index + 1 }}), so its identity changes on every render. It is now a dependency of logSlotEvent, which is a dependency of logAdInteraction, which is a dependency of both the request/fill effect and the click-listener effect.
Net effect: every render of ArbitragePostContent (comment sort, hover, auth resolution) tears down and re-creates the MutationObserver and re-subscribes the blur/focus/pagehide listeners for every ad on the page. The hasPushed/hasLogged* latches keep it correct, but it is avoidable churn on the one component whose observers must not miss a mutation.
Suggested direction: take the extras as scalars (section, occurrence) or useMemo them at the call sites, so the callbacks stay stable.
Reviewed by AI.
| // Replies count too: the interval is over everything the reader | ||
| // scrolls past, not just top-level rows, and the boundary can only | ||
| // sit after a top-level block — never inside one. | ||
| const countThrough = (through: number): number => |
There was a problem hiding this comment.
Non-blocking: countThrough runs two full prefix reductions per row, so the thread is O(n²) on every render of the list. A running accumulator across the map (or a memoised prefix-sum array) is both cheaper and easier to read.
Two semantics worth confirming while you are here:
children?.edges?.lengthcounts the replies that are loaded. If replies are ever collapsed or paginated, the interval counts rows the reader never scrolled past, which is the opposite of the stated rationale.- when one top-level block spans two intervals (a comment with 20 replies at an interval of 8), the occurrence jumps 1 → 3 and only one ad renders. Fine as behaviour, but the analytics occurrence will have gaps — worth a line in the docblock so nobody reads a gap as a lost impression.
Reviewed by AI.
| }); | ||
|
|
||
| it('never ends on a sliver', () => { | ||
| const text = `${'a'.repeat(260)}. ${'b'.repeat(30)}`; |
There was a problem hiding this comment.
Non-blocking (test passes for the wrong reason): never ends on a sliver uses a 292-character input against a 250 target, so Math.round(292 / 250) is 1 and the function returns early — the assertion holds without exercising any sliver handling. Something like 700 characters plus a 20-character tail would actually test it.
Also missing for splitTextForAds: maxParts capping (the density cap is the safety property here) and text with no sentence punctuation at all, which takes the word-boundary fallback.
Reviewed by AI.
… leaderboard Review round plus field reports: - splitContentForAds selects cuts like the TLDR splitter now: block boundaries nearest to even split points with the cadence as a hard floor, so the per-section cap spreads units across the whole article instead of front-loading them into the first thousand characters. The same floor lands in splitTextForAds — nearest-sentence snapping alone put an ad after ~130 characters. - Phone density policy, with numbers at the rail map: only the page's first in-content unit keeps a phone placement, the rail is desktop-only throughout, leaving leaderboard + first in-content + above-comments ≈ 17-22% of a scraped page against the 30% cap. - The top leaderboard gets a phone twin requested at a fixed 320x100: a responsive request can answer with expandable video — reported pinning half a phone screen inside the sticky header block — and a fixed-size request can only return its exact size. Neither twin is eager; a display:none ins never intersects, so exactly one requests. - Comment interval: 6 (revised from 8 same day), counted with one prefix pass instead of two reductions per row. - Unit chrome reservation corrected to the real 36px; logExtra rides a ref so inline objects can't re-run the ad effects; sliver test exercises the floor it claims to test, plus maxParts and no-punctuation coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both remaining blockers closed (87b4e84)
Non-blockers all taken: chrome reservation corrected to the true 36px (creative +36 on every format), Also in this push, from field reports: the top leaderboard requests a fixed 320×100 on phones — Google answered the responsive request with expandable video that pinned half a phone screen inside the sticky header block; a fixed-size request can only return its exact size. And the comment interval is 6 (product revision from 8, same day). Tests: 138 shared, full 🤖 Generated with Claude Code |
rebelchris
left a comment
There was a problem hiding this comment.
Re-review of 87b4e84 / 1b6f387
Both blockers are resolved, and resolved properly rather than papered over.
- Front-loading —
splitContentForAdsis now the same shape as the text splitter: one pass collecting depth-0 boundaries with their cumulative visible offset, part count fromround(total / minChars)capped bymaxParts, then boundaries chosen nearest each even split point with the cadence as a floor on both sides. On a 5,000-character body the four units now land at roughly 1k/2k/3k/4k instead of all inside the first 1k. I re-ran the existing specs' inputs against the new algorithm by hand (short content, two-paragraph split, code block, sliver merge, byte preservation) and they all still hold for the right reason. ✅ - Phone density — answered by placement rather than by hoping: all rail units are
hideOnPhone, the comment MPU is desktop-only, only the page's first in-content unit keeps a phone placement (hideOnPhone={summaryParts.length > 1 || index > 0}is a neat way to express "first on the page, whichever section it comes from"), and the leaderboard's phone twin is a fixed 320x100. Three phone units, bounded, with the arithmetic written into the PR body. ✅
Also fixed: the height reservations now match the real 36px chrome, logExtra moved to a ref so the ad effects no longer re-run per render, the comment interleave is a single prefix pass, and the new specs cover the cadence floor, maxParts and the word-boundary fallback.
The phone twin deserves a note: it works because a display:none wrapper never intersects, so only one of the two ever mounts an <ins> and the "first uninitialised ins in document order" invariant holds. That is correct, and it is written down — but it is also the kind of invariant that breaks silently. Four non-blocking follow-ups below.
Non-blocking
- The 17-22% is computed, not measured. 708px of ad against an assumed 3,300-4,100px page. Two things can move it: an unfilled unit collapses and shrinks the page (raising the ratio for the units that did fill), and the leaderboard is pinned for its first 10 seconds, which under the Better Ads methodology counts against density for as long as it is on screen. Worth one spot-check at 375px on a real scraped post with a short TLDR before this goes wide.
eageris gone from the top leaderboard. The above-the-fold unit now waits for hydration plus an IntersectionObserver callback, and the docblock explaining why eager existed ("rides the very firstadsbygoogleprocessing pass") now describes a prop no slot uses. Please confirm leaderboard fill rate and time-to-fill on the preview, and consider dropping the prop if nothing needs it.- Nothing enforces the twin invariant. If anyone later adds
eagerto either leaderboard twin, both push and the mis-binding race theProgrammaticAdcomment describes re-opens. A one-line assertion inArbitrageTopLeaderboard.spec.tsxthat neither twin is eager would make that a test failure instead of a support ticket. - Zero-ad phone edge. If the single phone-visible in-content unit comes back unfilled and collapses, a phone gets no in-content ad at all, even on a long body. Almost certainly the right trade against density, just worth being a deliberate choice rather than a side effect.
Minor: the IIFE wrapping the comment map reads a little heavier than computing the prefix totals above the return — no behavioural difference.
Verification
- Traced the full delta
731f118...1b6f387and re-derived the new split algorithm against every existing spec input. - Confirmed the phone-unit inventory by reading each slot's
hideOnPhoneand the two breakpoint twins, and checked the 320x100 fixed size against the restoredpy-2width caps (320pxinsin a 320px box — this only works because the horizontal padding is gone). - CI green on
1b6f387:test_shared,test_webapp,test_extension,lint_shared,typecheck_strict_changed,build, Vercel webapp. - Still not verified by me: the on-device phone density spot-check and leaderboard fill after the
eagerremoval (items 1 and 2).
Verdict
No blocking findings remain. Good to approve from my side once someone eyeballs the phone page at 375px.
Reviewed by AI.
…s placements Rebuilt on main after #6529 merged (the stacked base squashed away; the final tree applies cleanly, signup work stays out in its own PR). The organic post page now reuses the full placement set the articles template shipped, all only while post_adsense is live for an anonymous visitor: the TLDR splits at the shared 250-char cadence with an MPU between segments (sentence-snapped, capped, phones keep only the first), an MPU sits above the comments, a long thread carries one per interval (replies included), and the leaderboard + rail placements ride the earlier work — top leaderboard with its fixed 320x100 phone twin, rail MPU as true backfill behind the direct-sold widget. Threaded through PostContent/BasePostContent as opt-in props (renderSummarySegments, aboveComments, commentAds) that only the webapp post page passes: post modals and the extension render these components too and keep byte-identical markup. The internal AdAsComment steps aside only when programmatic comment ads are active. Original organic-ads work by Tsahi Matsliah; navigation-boundary, backfill and phone-twin fixes from the review rounds carry over. Co-Authored-By: Tsahi Matsliah <tsmatliah@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the expert consult's ten action points on the
/articlestemplate.hideInternalAd(AdAsComment) +hideAdWidget(PostSidebarAdWidget), opt-in props — every other surface unchangedsplitTextForAds, sentence-snapped) and hosted bodies (splitContentForAds, block boundaries). Cuts aim at even split points with the cadence as a hard floor — no unit within 250 visible chars of the previous one, no front-loading — and each section is capped at 4 units (MAX_CONTENT_ADS_PER_SECTION), the constant that now carries density compliancerootMargin: 250px— 50px puts the auction and the unfilled-collapse jump inside the visible area; 250px keeps both off screen. Units centered on abg-surface-floatblock (vertical padding only, so the IAB width caps hold)PostAnsweredQuestionsrenders after the body, as on the post page; it self-hides for logged-in users, which matches this page's anonymous-only adsPhone density policy (the fresh number): at the 250-char cadence the interval no longer bounds density, so placement does: on phones only the page's first in-content unit renders, the rail and comment units are desktop-only, and the leaderboard is a fixed 320×100. A scraped article on a 375px viewport carries leaderboard (136px) + first in-content (286px) + above-comments (286px) ≈ 708px of ad against a ~3,300-4,100px page — 17-22%, under the Better Ads 30% cap with margin. Desktop carries the full cadence under its 50% cap.
Unit reuse note: the new placements share live Display units (6921226982 / 5249052667), so AdSense-side reporting blends them; our first-party events split by slot number, so per-placement RPM stays queryable in ClickHouse. Dedicated
read_s17/read_s18units are a marked TODO.Verification
splitContentForAdsspecs (nesting, code blocks, void elements, byte preservation, sliver merge)WorldGuideSheetfailure only)Preview domain
https://feat-articles-monetization.preview.app.daily.dev
Where to verify the new placements
The in-body cadence now covers both content forms: the TLDR (
splitTextForAds, sentence/word boundaries — for scraped articles the summary is the main content) and hosted bodies (splitContentForAds, block boundaries). A post whose TLDR is under ~250 words gets no in-body unit — the MPU above the comments is the one that renders on every post.