feat(shared): expert-round ad iteration on /articles - #6528
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.
|
|
Superseded by #6529 — same commit, renamed branch so the preview domain carries neither codename. |
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Summary
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.
Two 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. - 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.
1 participant
Implements the expert consult's ten action points on the
/articlestemplate.hideInternalAd(AdAsComment) +hideAdWidget(PostSidebarAdWidget), opt-in props — every other surface unchangedsplitContentForAds: cuts only at top-level block boundaries (never inside a paragraph/list/code block), merges trailing slivers. Cadence is ~250 words (1500 chars) — see note belowrootMargin: 50px; units centered on abg-surface-floatblockPostAnsweredQuestionsrenders after the body, as on the post page; it self-hides for logged-in users, which matches this page's anonymous-only adsJudgment call to confirm: the brief's cadence was taken as ~250 words (≈1500 characters) per MPU, matching the original partner brief. A literal 250 characters is one short paragraph per ad — an ad-to-content ratio far past the Better Ads 30% cap that gets ads filtered domain-wide. If the expert truly meant characters, that needs a written risk sign-off first.
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)🤖 Generated with Claude Code
Preview domain
https://claude-articles-ad-iteration.preview.app.daily.dev