feat(webapp): ad-monetised post template for paid and SEO traffic - #6500
feat(webapp): ad-monetised post template for paid and SEO traffic#6500tsahimatsliah wants to merge 45 commits into
Conversation
Adds a third post-page template on its own route so the existing two are untouched. Built for landing traffic where the goal is impressions, not signups. Route: /posts/[id]/read (noindexed, canonical points at /posts/[slug] so it never competes with the standard page in search). Forked from the classic PostContent layout rather than the focus card: for scraped articles neither renders a body, so the focus card's only advantage does not apply, while the widget column it lacks carries three always-viewable slots. Removed relative to the standard template — all by omission, no flags: - PostAuthBanner (300-400px of bottom viewport) - CustomAuthBanner (never passed as layoutProps.customBanner) - PostSignupWidget (top of the widget column) Header login/signup buttons are unaffected. Adds an opt-in `expandSidebar` layout prop so the rail renders expanded regardless of the stored collapse preference, carrying slot 1. Defaults preserve current behaviour everywhere else. Ad slots render as reserved placeholders — no ad tag exists in the app yet, so this is for reviewing density and layout. Slot heights are reserved the way live slots must be to avoid layout shift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Preview links — verified liveUse the assigned preview domain, not the raw Vercel URL (the Vercel one can't complete boot, so the sidebar and header stay empty there):
More posts to try: What the side-by-side showsCurrent template renders the geo-personalised New template has neither: no banner, sidebar expanded to 240px with slot 1 pinned at its bottom, and the article starting immediately under the leaderboard. Verified on the preview at 1440×900
CIAll 12 checks green — lint, strict typecheck, and the shared/webapp/extension test suites. Still open
|
… that route Slots go live per-slot via the read_adsense_slots GrowthBook JSON value (slot number -> unit id/type); the empty default renders the existing placeholders and loads no Google code anywhere. The adsbygoogle script is rendered only by the /read page, and leaving the route forces a full page load so Auto ads overlay state can never follow client-side navigation into the rest of the app. Units lazy-request via IntersectionObserver for viewability; non-production builds serve test creatives via data-adtest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live AdSense layer (b5350c4)The template can now serve real ads, strictly scoped to `/posts/[id]/read`:
Go-live = paste this into GrowthBook `read_adsense_slots` with the real unit ids{
"1": { "id": "…", "type": "display", "width": 240, "height": 400 },
"2": { "id": "…", "type": "display" },
"3": { "id": "…", "type": "inArticle" },
"5": { "id": "…", "type": "inArticle" },
"6": { "id": "…", "type": "inArticle" },
"7": { "id": "…", "type": "inFeed", "layoutKey": "…from the generated snippet…" },
"8": { "id": "…", "type": "inArticle" },
"9": { "id": "…", "type": "multiplex" },
"10": { "id": "…", "type": "display", "width": 300, "height": 600 },
"11": { "id": "…", "type": "display" },
"12": { "id": "…", "type": "display" }
}Slots 13 (anchor) + 14 (vignette) come from AdSense Auto ads → overlay formats only, scoped in the AdSense UI to a URL group matching `/posts/*/read`. Verified: 7 new ArbitrageAdSlot/Anchor tests, shared post (42) + sidebar (37) suites, strict typecheck guard, lint — all green. 🤖 Generated with Claude Code |
noindex and a canonical to the parent post are mixed signals Google warns against; this page must never rank, so it keeps only noindex. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dashed density-review placeholders were the draft state; production with an empty read_adsense_slots value now renders no ad surface at all. Placeholders remain a local-development tool only. Test creatives are now decided by hostname at request time instead of build env, since preview deployments are production builds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Update (e0c3245): the placeholder boxes are now dev-only. Production with an empty 🤖 Generated with Claude Code |
A sticky element slides down over siblings that follow it once the page scrolls, so slot 10 sitting first in the column painted over the source card. Last in the column it sticks for the whole read with nothing below to cover. Also drop overflow-hidden from the live slot wrapper: Google resizes responsive creatives post-request and clipping cut their bottom edge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Ad slots now use the feed Card treatment (rounded-16, subtle border and surface) instead of a dashed outline, so a filled slot reads as page furniture rather than a bolted-on frame. Slot number and size stay as small labels for review. - Anchor gets the same radius and a shadow so it reads as a floating card. - FurtherReading takes an opt-in `hideToc`; the ad template passes it so the rail's vertical space goes to slots 10-12 instead of the table of contents. Default behaviour is unchanged everywhere else. - Clears the pre-existing strict-null violations in FurtherReading that the changed-file guard surfaced once the file was touched. All behaviour preserving; the component's 7 tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Applies the same rounded-16 treatment to the live AdSense container that the placeholder already had, so a filled slot reads as page furniture rather than a pasted-in iframe. Safe against the resize-after-request problem the previous comment warned about: the box has no fixed height, so a creative that grows pushes the container taller instead of being clipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
border-radius plus overflow-hidden on the wrapper alone does not reliably clip an ad: WebKit paints the injected iframe on its own compositing layer that escapes the rounded clip, so the corners come back square. - `isolate` on the wrapper forces a stacking context so the clip applies. - The radius is repeated on the <ins>, the closest ancestor of the iframe, so the creative's own corners are clipped rather than just the wrapper's. - overflow-hidden on the anchor shell for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two placements from the ad partner's brief were specified but never built. - Floating leaderboard (slot 13) now renders in live mode. It previously returned null there, so there was no bottom leaderboard once ads were on. It mounts ten seconds after load, per the brief, and the delay gates the mount rather than visibility so the ad request fires when it appears. Renders only when slot 13 is configured, leaving the viewport bottom free for an Auto ads anchor otherwise — the two can never stack. - Top leaderboard (slot 2) now sticks while scrolling and releases after ten seconds, also per the brief. Sticky rather than fixed so it pins within its own container and cannot overlap the article. Also names every slot with the partner's own terminology (topLeaderboard, railMpu1, floatingLeaderboard...) so the remote config, this code and their brief all refer to the same units. Ad sizing and layout: - Each format is capped at its standard IAB width and centred. Slots were falling through to responsive `auto`, so Google picked whatever creative fit and two units came back different widths. - The template widens from 69.25rem to 72rem: the main column had 704px and a 728x90 leaderboard could not render at full size. - Unfilled slots collapse via data-ad-status, instead of leaving their reserved height as an empty band in the comment thread. Template fixes: - Restores the post action bar (upvote/comment/bookmark/copy) and engagement counts, which the standard template gets from PostEngagements. - Adds the missing tag-to-metadata spacing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tions The top leaderboard never stuck: PostContainer is overflow-hidden, which makes it the sticky element's scroll container, and that container never scrolls. Move the leaderboard above the two-column shell, which also gives it full page width — the partner asked for a leaderboard in the right rail, where a 728px unit cannot fit. Rebuild the action bar on PostUpvotesCommentsCount and PostActions in the same order PostEngagements uses, and move the read button to the header actions next to the options menu. Anchor the rail units to named widgets instead of the brief's block indices, which resolve to nothing in a column of this length: source card, MPU 1, You might like, MPU 2, Best discussions, half page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Full-bleed placement was only needed because PostContainer is overflow-hidden, which made it the sticky scroll container. overflow-x: clip with overflow-y: visible clips the column the same way without creating one, so the leaderboard can sit in the content flow above the source row and still pin for ten seconds. The column is 745px wide inside its padding at the layout's full width, so a 728x90 renders at its booked size; narrower viewports get a smaller responsive creative rather than a clipped one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured on a phone the template ran 12 in-flow slots against 2171px of content: 56% ad density, where the Better Ads Standards cap mobile at 30% and Chrome filters ads on sites that fail. Scraped posts have no body text to dilute the slots, so the fix is fewer units, not more content. The three rail MPUs, the second and third in-content MPUs, the comment native unit and the end-of-article unit now drop below tablet, leaving leaderboard, MPU 1, video, comment MPU and the anchor at 28%. Hidden rather than skipped so they never request either — the push only fires on intersection. Below laptop the page also carried no navigation at all, because MainLayoutHeader renders the feed nav there and a post route has nothing to fill it. Add the mobile footer nav, and pin the top leaderboard at top-0 below laptop where there is no fixed header to clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y has On layout v2 the sidebar owns the header, so <main> carries no top padding and the leaderboard's natural position is 0. A hardcoded top of 4rem then pushed the sticky element 4rem *below* where it sits in flow, painting it over the source row and the read button until the ten-second timer released it. MainLayout now publishes --sticky-header-offset alongside the padding it already computes, so the offset always matches the chrome on screen: 4rem for the v1 header, 0 under the v2 sidebar, 0 below laptop, more again with a banner. Because it is driven by the same flag as the header itself, it stays correct through the variant swap instead of flashing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… unit SidebarRail, Video and RichMedia had no call sites left. The organic half page pinned at a hardcoded 5rem, wrong whenever a top banner adds 2rem above the header — it reads MainLayout's --sticky-header-offset like every other pinned unit.
tsahimatsliah
left a comment
tsahimatsliah
left a comment
There was a problem hiding this comment.
Summary
A third post template plus flagged AdSense units on the organic post page. The layout work is careful and the shared-component additions are opt-in, so existing consumers are unchanged. Two things block: internal commercial data committed to this public repo, and the flag/ad-request mechanics. Detail inline.
Note: this is a draft — reviewed on explicit request.
Blocking
plans/ad-arbitrage/**— internal commercial strategy docs in a public repository.FORMAT_SPEC.cpm— internal rate expectations shipped in the production bundle.useOrganicAdsenseSlots— unconditional GrowthBook evaluation enrolls users who can never see the feature (including every/readslot, a surface the flag does not govern).- Organic AdSense script loads with
post_adsenseon while both unit ids are empty. adsbygoogle.push({})binds to the first uninitialised<ins>in document order, not to the slot that triggered it.
Non-blocking
Route interception and shallow navigations, ads in the shared sidebar, nullability widening in FurtherReading, missing analytics.
Verification
- CI green (shared/webapp/extension tests, lint, strict typecheck on changed files)
- Shared-component consumers enumerated (
PostWidgets,PostComments,PostEngagements,FurtherReading,MainLayout,Sidebar) - Not verified: behaviour with
post_adsenseon for an anonymous visitor once real unit ids exist;/readon a real phone; extension build with the sidebar change
Verdict
Changes needed — items 1–5. (Posted as a comment review: the review credential is the PR author, so a formal change request is not possible.)
Reviewed by AI.
| @@ -0,0 +1,454 @@ | |||
| # Ad Arbitrage Initiative: Asset Map, Analytics, Economics & Launch Plan | |||
There was a problem hiding this comment.
Blocking (privacy). dailydotdev/apps is public. These files carry internal commercial data: named advertisers with their booked deal values and CPMs, internal remnant RPM/fill figures, per-surface impression volumes, revenue modelling, and paths/references to an internal private knowledge repo. That is exactly the category we keep out of public PRs, and anyone can read the branch today.
Note .gitignore has plans/*.md, which only covers the top level — the plans/ad-arbitrage/** subtree slipped past it. plans/ in main contains only .gitkeep, so this is the first content added there.
Please drop plans/ad-arbitrage/** (docs + asset-volume-queries.sql) from the branch, keep the material in the internal knowledge repo, and extend the ignore to plans/**/*.md so this cannot recur. Since the branch is already public, rewrite the commits rather than adding a deletion commit on top.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: history rewritten — plans/ad-arbitrage/** (docs + SQL) removed from every commit on the branch, not deleted on top — and .gitignore now covers plans/**/*.md + plans/**/*.sql. The material is preserved locally for the internal knowledge repo. Note: the pre-rewrite commit objects may remain fetchable by SHA on GitHub until garbage collection; if that matters, a support request to purge them is the remaining step.
| [ArbitrageAdFormat.Leaderboard]: { | ||
| label: 'Leaderboard', | ||
| size: '728x90 · 320x100 mobile', | ||
| cpm: '$2.50', |
There was a problem hiding this comment.
Blocking. cpm (and reach on the props) are internal rate expectations, and FORMAT_SPEC is a module-level constant. The dashed placeholder only renders in development, but the map itself is in the production bundle — and since SidebarDesktop now imports this module, that bundle is shipped on effectively every page of the webapp and the extension. Flag-gating hides UI, not shipped constants.
Suggest dropping cpm/reach from the shipped code entirely (they are review aids, not runtime data), or moving the placeholder plus its spec into a module that is only imported behind isDevelopment so it tree-shakes out.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: cpm is gone from FORMAT_SPEC and reach from the props and every call site — dropped from the shipped code entirely rather than moved behind a dev-only import.
| // Raw context rather than useAuthContext: this runs inside every slot | ||
| // (read surface included), where an AuthContext provider isn't guaranteed. | ||
| const isAnonymous = !useContext(AuthContext)?.user; | ||
| const enabled = useFeature(featurePostAdsense); |
There was a problem hiding this comment.
Blocking. useFeature evaluates unconditionally, and evaluation enrolls. Two consequences:
- On
/posts/[id]the hook runs for every visitor, including logged-in and Plus users who returnNO_SLOTSright after — they get enrolled inpost_adsensewhile being structurally unable to see a unit, so the experiment population is diluted. ArbitrageAdSlotcalls both hooks regardless ofsurface, so every slot on/read(13+ instances, and one per interleaved comment unit) evaluatespost_adsensetoo — a flag that does not govern that route at all.
useConditionalFeature is the established primitive and is already used a few lines away from the new code in pages/posts/[id]/index.tsx for featurePostRedesign. Please gate on shouldEvaluate: isAnonymous, and in ArbitrageAdSlot skip the organic hook when surface === 'read' (a small split into two components, or passing the slot map in as a prop, avoids the conditional-hook problem).
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: useOrganicAdsenseSlots now goes through useConditionalFeature with shouldEvaluate: isAnonymous, and ArbitrageAdSlot splits per surface (ReadArbitrageAdSlot / OrganicArbitrageAdSlot, shared MappedAdSlot body) so /read slots never call the organic hook at all — no conditional-hook problem, no enrollment from either direction.
| // Empty for Plus members and while post_adsense is off; the slot components | ||
| // check the same hook, so with it empty neither markup nor script exists. | ||
| const adsenseSlots = useOrganicAdsenseSlots(); | ||
| const adsenseActive = Object.keys(adsenseSlots).length > 0; |
There was a problem hiding this comment.
Blocking. adsenseActive is derived from key count, but both entries in ORGANIC_ADSENSE_SLOTS currently have id: ''. So flipping post_adsense on today loads adsbygoogle.js plus the preconnects/preload on every anonymous post page and renders zero units — cost and third-party surface with no inventory, and the slot components correctly return null, so nothing shows the problem.
Derive the script gate from at least one non-empty id (Object.values(slots).some((s) => !!s.id)) rather than key presence, and apply the same to ArbitrageAdSlot's isLive and ArbitrageAnchor.
Also, the comment above says "Empty for Plus members" — the hook actually gates on anonymous, i.e. all logged-in users, not just Plus.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: added hasLiveAdsenseUnits (some non-empty id) in adsense.ts and gated adsenseActive, ArbitrageAdSlot's live mode and ArbitrageAnchor on it — flipping post_adsense on with empty ids now loads nothing. The "Plus members" comment is corrected to every logged-in visitor.
| const requestAd = (): void => { | ||
| try { | ||
| window.adsbygoogle = window.adsbygoogle || []; | ||
| window.adsbygoogle.push({}); |
There was a problem hiding this comment.
Blocking (needs verification on the preview with all units live). adsbygoogle.push({}) does not bind to insRef.current. The tag processes the pushed request against the first uninitialised ins.adsbygoogle in document order. With per-slot lazy pushes firing in intersection order across 13 placements plus repeated comment natives, a push triggered by a slot low on the page can initialise an earlier, not-yet-pushed slot instead: the request goes out with the wrong unit id and the wrong format, the triggering slot stays unprocessed, and the querySelector('iframe') check then collapses it as empty. Reporting is attributed to the wrong placement too.
This is easy to miss in manual QA because most slots do eventually fill.
Options: keep one shared queue that pushes strictly in DOM order as slots become eligible, or make every slot eager and rely on the tag's own lazy loading, or pass the element explicitly via the params object if we confirm that is supported for this account. Worth confirming empirically (log data-ad-slot on each filled iframe's parent) before merge.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f, structurally rather than by queueing: the <ins> no longer mounts until its slot becomes eligible (eager → mount, lazy → on intersection), so at any moment every uninitialised ins in the document is one that should be processed right now — which makes the pushes interchangeable and the first-in-document-order binding harmless. Each processed ins uses its own data-ad-slot, so attribution follows the element, and the wrapper keeps the format's min-height so reserved space is unchanged. Pinned by a test (mounts no <ins> before the slot becomes eligible). Will verify on the preview by checking every filled iframe sits under the ins whose id requested it.
There was a problem hiding this comment.
Verified empirically on the preview deploy of dfbe38f: at load, only eligible slots have an <ins> mounted (the second grid's is absent until scroll); after making every placement eligible — 10 ins total, 0 unprocessed, 0 iframes outside their own ins, and the late-mounted grid processed and filled (447px). No stranded slots, attribution follows each element.
| return undefined; | ||
| } | ||
| const forceHardNavigation = (url: string): void => { | ||
| if (/^\/posts\/[^/]+\/read(?:[/?#]|$)/.test(url)) { |
There was a problem hiding this comment.
Non-blocking, but please handle shallow navigations. This handler treats every routeChangeStart as a departure. It does not receive/inspect the second { shallow } argument, so a same-page query update — a comment permalink, a URL-masking modal, or anything doing router.replace(..., { shallow: true }) — becomes a full page reload mid-interaction, and the thrown error lands in the console/error reporting once per navigation.
Suggest (url, { shallow }) => { if (shallow) return; ... } plus a same-pathname check, so only genuine departures from /read force the hard load. Global routing interceptors have bitten us before (the middleware matcher that broke feed-card modals), so it is worth enumerating what on this page can push a route: the comment flows, the auth modal, and PostHeaderActions.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: the handler now receives { shallow } and returns early on shallow navigations, so comment permalinks and URL-masking modals stay client-side; only genuine departures from /read force the hard load. The same-pathname case is covered by the existing /read URL test.
| </Nav> | ||
| </SidebarScrollWrapper> | ||
|
|
||
| {forceExpanded && <ArbitrageSidebarAd />} |
There was a problem hiding this comment.
Non-blocking (architecture). The global desktop nav — shared by the webapp and the extension — now imports the arbitrage module and decides to render an ad based on forceExpanded. Two costs: the extension bundle pulls in the AdSense slot code even though AdSense is prohibited there, and forceExpanded silently means "also show an ad", so any future caller of expandSidebar inherits an ad placement (with no script loaded, since only /read renders it).
Inverting it keeps the nav dumb: MainLayout already takes nodes (topBanner), so a sidebarTrailing?: ReactNode passed from the /read page would put the ad policy on the page that owns it.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f as suggested: SidebarDesktop no longer imports the arbitrage module — it renders an opaque trailing node threaded from MainLayoutProps.sidebarTrailing, and the /read page passes <ArbitrageSidebarAd /> via its layoutProps. The nav stays dumb and the extension bundle no longer pulls AdSense slot code through the sidebar.
|
|
||
| const publicSquadProps: Partial<SimilarPostsProps> = { | ||
| title: `More posts from ${currentPost.source.name}`, | ||
| title: `More posts from ${currentPost.source?.name}`, |
There was a problem hiding this comment.
Non-blocking. These ?. additions widen a shared component (all post pages and modals) to tolerate a missing source rather than guarding at the boundary. As written, a post without a source renders the literal title "More posts from undefined", a More button with href={undefined}, and — line 54 — a similar-posts query with source: undefined, which drops the source filter instead of skipping the query.
If strict typecheck on the changed file is what forced this, prefer an early guard (const squad = isPublicSquad ? currentPost.source : undefined and only build publicSquadProps when it exists) so the types narrow without changing rendered output.
Reviewed by AI.
There was a problem hiding this comment.
Resolved in dfbe38f: replaced the optional chains with a single boundary narrow — publicSquad is set only when the source is a public squad, the query uses publicSquad.id (no silently-dropped filter), and publicSquadProps is built only when it exists, so "More posts from undefined" can't render. Output for existing consumers is unchanged.
| addition is a native unit every few comments in a long thread. */} | ||
| <PostEngagements | ||
| post={post} | ||
| onCopyLinkClick={onCopyPostLink} |
There was a problem hiding this comment.
Non-blocking (analytics). The template ships with no instrumentation: no impression, fill, or dismiss events for the slots, and no event distinguishing /read sessions from organic ones. Combined with the shared unit ids (slot 3 borrowing the rail unit, slot 8 reusing the first grid, slot 13 reusing the top leaderboard), AdSense reporting will blend placements and we will have no first-party signal at all on which placement earned what or how often the anchor/sidebar units get dismissed.
Worth at least a dismissal event on the two closable units and a per-slot fill event before this carries paid traffic.
Reviewed by AI.
There was a problem hiding this comment.
Partially resolved in dfbe38f: added LogEvent.FillAdsenseSlot (once per slot, extra carries {slot, unit, format} — the first-party per-placement signal the blended AdSense reporting can't give) and LogEvent.DismissAdsenseSlot on both closable units (anchor + sidebar). A /read-vs-organic session marker beyond the page path, and impression/viewability events, are left for a follow-up before paid traffic ramps.
| // TODO(chris): create the three new rail units (suggested names | ||
| // read_s04_rail_creator, read_s05_rail_share, read_s06_rail_highlights) as | ||
| // Display 300x250. They stay collapsed until their ids are filled in. | ||
| [ARBITRAGE_SLOT.railAfterCreator]: { id: '', type: 'display' }, |
There was a problem hiding this comment.
Question. Five /read slots (4, 5, 6, 7) and both organic slots ship with empty ids, so a decent share of the placements described in the PR body are inert at merge, and the post_adsense flag has nothing to turn on. Can you state in the description which placements are actually live on merge, and whether the empty ones should land in this PR at all versus following once the units exist in AdSense? The TODO(chris) markers also point at a person rather than a ticket — a Linear reference would survive better.
Reviewed by AI.
There was a problem hiding this comment.
Answered in the updated PR description: live at merge on /read are slots 1 (sidebar), 2 (top leaderboard), 3 (beside cover, borrowing the rail unit id), 9 (grid — slot 8's second grid shares its unit), 10 (rail half page), 11/12 (rail rectangles), 13 (anchor, borrowing the leaderboard unit id). Inert until their AdSense units exist: 4/5/6 (new rail positions), 7 (comment native, needs layoutKey) and both organic slots — kept in this PR so the placements and their gating ship reviewed once, with each empty id collapsing to nothing. On the TODO(chris) markers: agreed a ticket reference is better; I can't create Linear issues from this repo, so that's on us to file and swap in.
Review blockers, in order:
Internal data out of the public repo: plans/ad-arbitrage/** is removed
from every commit (history rewritten, not deleted on top — the branch
is public), .gitignore covers the whole plans subtree, and the shipped
FORMAT_SPEC loses its cpm figures along with the reach annotations —
review aids, not runtime data.
Flag hygiene: useOrganicAdsenseSlots evaluates post_adsense through
useConditionalFeature gated on the visitor being anonymous, and
ArbitrageAdSlot splits per surface so /read slots never touch the
organic hook — no more enrolling users who structurally cannot see a
unit.
Script gating: live-ness now means at least one unit id, not key
presence, via hasLiveAdsenseUnits — flipping post_adsense on while the
organic ids are still empty no longer loads adsbygoogle.js for
inventory that cannot fill. Same gate on /read and the anchor.
Push binding: adsbygoogle.push({}) initialises the first uninitialised
<ins> in document order, not the slot that pushed — so with every ins
mounted up front and lazy pushes firing in intersection order, a push
from low on the page could initialise an earlier slot instead. The ins
now mounts only when its slot becomes eligible, keeping the invariant
that every uninitialised ins is one that should be processed right now,
which makes the pushes interchangeable. The wrapper keeps the min-height
so the page reserves the same space.
And the non-blocking notes: the /read departure interceptor ignores
shallow navigations; the sidebar unit rides in from the /read page as
MainLayout's sidebarTrailing node instead of living in the shared nav
(which the extension also bundles); FurtherReading narrows its missing-
source guard at the boundary instead of optional-chaining into "More
posts from undefined"; and the slots get first-party analytics — a
per-placement fill event and dismiss events on the two closable units.
4040c90 to
dfbe38f
Compare
Found in final self-review: a fill arriving after the four second grace found its slot already display:none, where everything measures zero, so the height check re-judged the creative as empty and the slot stayed collapsed forever — the mutation observer's whole reopen path was dead. The height check now applies only while the ins is actually displayed (getClientRects, empty exactly under display:none); a hidden slot reopens on the creative alone and the resize observer re-judges it at real geometry. Regression test simulates the late fill.
3d35381 to
aada78a
Compare
aada78a to
55bee60
Compare
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Re-review of the current head (organic slots + boot-light config). Round 1's five blockers are genuinely fixed: plans/** is out of the tree and ignored subtree-wide, no cpm/reach values remain in shipped code, post_adsense is now evaluated conditionally, the script is gated on a real unit id, and the adsbygoogle.push ordering race is handled by mounting each <ins> only at eligibility. The layout engineering is careful and the shared-component additions are all opt-in, so existing consumers stay unchanged.
What blocks now is not the layout — it is the risk this puts on the whole domain and the loss of the kill switch.
Blocking
1. The mobile ad density is knowingly over the Better Ads Standards cap. ArbitragePostContent.tsx:43-48 states the phone run measures ~40% of page height against the 30% cap and is kept "by product decision". The PR description says the opposite ("mobile keeps density near the 30% cap"). Chrome's ad filter is applied site-wide to the domain, not per route, so a /read violation filters ads across app.daily.dev — including the in-feed inventory we sell. Either bring the phone inside the cap or get an explicit, written product sign-off on filtering the whole domain, and fix the description so nobody merges this on the wrong understanding.
2. AdSense policy exposure on scraped-article pages. /read is generated for any post id with fallback: 'blocking'. For scraped articles contentHtml is empty, so the page is a title, a summary, a cover image and 13 ad placements. That is the shape AdSense's low-value/scraped-content policy targets, and enforcement is account-level. Please state which post types /read is allowed to render for (and enforce it in getStaticProps) rather than shipping it for the entire catalogue.
3. There is no longer a kill switch. useReadAdsenseSlots returns the hardcoded map in every non-development build, and the read_adsense boolean from the previous round is gone. Turning /read ads off — policy warning, bad creative, broken layout, revenue anomaly — now needs a code change, a deploy and ISR revalidation. Keep the boolean (default on if that is the product call); the boot-payload argument was about the JSON map, not about a single flag.
4. /read serves programmatic ads to logged-in and Plus members. The organic surface correctly restricts to anonymous visitors; /read has no auth check at all. Any logged-in user who lands on a /read link — and any Plus subscriber, for whom ad-free is a paid promise — gets the full 13-slot page. Please gate /read the same way, or say explicitly that Plus users are in scope.
5. post_adsense enrolls a population that cannot possibly see the feature. Every id in ORGANIC_ADSENSE_SLOTS is empty, so hasLiveAdsenseUnits is false and nothing renders — yet useOrganicAdsenseSlots is called at /posts/[id] page level and evaluates the flag for every anonymous post-page visitor. useConditionalFeature evaluation is enrollment, so the experiment fills with users for whom variant and control are byte-identical. Add hasLiveAdsenseUnits(ORGANIC_ADSENSE_SLOTS) to shouldEvaluate.
6. The anonymity check races boot. useReadAdsenseSlots.ts:28 reads useContext(AuthContext)?.user with no readiness gate. user is undefined until boot resolves, so a logged-in visitor is classified anonymous first, and the gate depends on GrowthBook's ready and boot landing in a specific order — an implicit invariant, not a guarantee. Once real unit ids exist this means an ad rendered then removed on a Plus user's post page (plus the layout shift). Gate on isAuthReady/isBootLoaded rather than on user being falsy.
7. Route interception breaks browser back/forward. read/index.tsx:95-110 intercepts routeChangeStart and calls window.location.assign(url). routeChangeStart also fires for popstate, so pressing Back from /read performs a forward navigation to the previous URL, leaving /read in the forward stack — Back appears broken and can trap. The throw inside the handler also surfaces as an uncaught error in the console and in error reporting. Please handle popstate via router.beforePopState (and/or history.replaceState + location.replace) and confirm Back, Forward and a browser refresh from /read on desktop and a phone.
Non-blocking
ArbitragePostContentis a 391-line fork of the classic layout. Every future fix toPostContentnow has to be applied twice, with nothing in CI asserting parity. Worth recording why variant props onPostContentwere rejected — and if the /read experiment wins, this fork is the follow-up debt.- Shared unit ids across placements (2/13, 3/12, 8/9, with 8's unit rendered twice on desktop) blend reporting and rest on the "one uninitialised
<ins>at a time" invariant, which any future eager slot or same-tick intersection can violate. The TODOs cover the reporting side; a short comment on the invariant at the push site would stop someone addingeagerand quietly mis-binding requests. data-adtestderives fromwebappUrl. A misconfiguredwebappUrlin production silently turns every impression into a test impression — zero revenue, no alarm. Log once when test mode engages so it is visible in production telemetry.FooterWrappernow reads--arbitrage-anchor-heighton every page for one route's benefit. Harmless with the fallback, but it is compensating CSS in a shared component; a wrapper on /read would keep the shared footer clean.useDelayedRevealanduseStickyReleaseare near-identical timer hooks. One hook with an optional trigger would do..gitignoreignoresplans/**/*.mdandplans/**/*.sqlonly. Any other extension underplans/is still tracked — ignore the directory and un-ignore.gitkeep.- CI is still in flight on this head (lint_shared, test_shared, test_webapp, build pending). Not blocking on its own, just not yet green as described.
Verification
- Round 1 blockers re-checked against the current head — all five fixed
- Shared consumers enumerated (
MainLayout,Sidebar,PostWidgets,PostComments,PostEngagements,PostContent,FurtherReading,FooterWrapper) - Extension path checked — sidebar unit is injected from the page via
sidebarTrailing, so the nav no longer pulls slot code into the extension bundle - Flag/enrollment semantics traced through
useConditionalFeature - Not verified: back/forward from /read, a real phone at the measured density, /read as a logged-in Plus member, organic slots with real unit ids
Verdict
Changes needed — items 1-7. The layout work is solid; the objections are about domain-level ad-policy risk, the missing kill switch, and who sees the ads.
Reviewed by AI.
| * product decision — the phone gets the same run of slots the desktop rail | ||
| * does. The second closing grid is still desktop only, which is what holds | ||
| * the phone anywhere near the cap. | ||
| */ |
There was a problem hiding this comment.
Blocking. This says the phone run measures ~40% of page height against the Better Ads Standards' 30% cap and is kept anyway. The PR description claims the opposite ("mobile keeps density near the 30% cap").
Chrome's ad filter is applied to the domain, not the route: a /read violation filters ads across app.daily.dev, including the in-feed inventory we sell to advertisers. That is a blast radius far beyond this template.
Either drop enough phone slots to land under the cap, or attach an explicit written product sign-off that accepts site-wide ad filtering as the downside — and correct the description either way, so this does not get merged on the wrong premise.
Reviewed by AI.
There was a problem hiding this comment.
Resolved on the branch (0d82b56): only the first rail unit keeps its phone placement, bringing the phone run to roughly 27% against the 30% cap — under it rather than signed-off over it, given the domain-wide blast radius. The comment at ArbitragePostContent now states the numbers and the reason, and the PR description's density line is corrected to match.
| * map there. | ||
| */ | ||
| export const useReadAdsenseSlots = (): ReadAdsenseSlots => | ||
| isDevelopment ? NO_SLOTS : READ_ADSENSE_SLOTS; |
There was a problem hiding this comment.
Blocking. This is the kill switch gone: every non-development build serves the hardcoded map, and the read_adsense boolean from the previous round no longer exists. An AdSense policy warning, a broken creative, a layout regression or a revenue anomaly now needs a code change, a deploy and ISR revalidation before ads stop.
The boot-payload argument that justified moving the map into code applies to the JSON map, not to a single boolean. Please keep read_adsense (default on if that is the product call) so the route can be switched off from GrowthBook.
Related: nothing here checks auth, so a logged-in visitor — including a Plus member, for whom ad-free is a paid promise — gets the full 13-slot page on /read. The organic surface restricts to anonymous; /read should either do the same or the exception should be stated.
Reviewed by AI.
There was a problem hiding this comment.
Resolved (0d82b56): read_adsense is back as the emergency kill switch, default on and documented as a non-experiment — /read ads can be stopped from GrowthBook without a deploy or ISR wait. The related point too: both surfaces now serve anonymous visitors only, so no logged-in member — Plus included — sees a programmatic unit on /read; a logged-in visitor gets the plain template. Pinned by tests ('never renders for logged-in users', 'goes dark when the read_adsense kill switch is off').
| export const useOrganicAdsenseSlots = (): ReadAdsenseSlots => { | ||
| // Raw context rather than useAuthContext: this runs inside every slot | ||
| // (read surface included), where an AuthContext provider isn't guaranteed. | ||
| const isAnonymous = !useContext(AuthContext)?.user; |
There was a problem hiding this comment.
Blocking. Two problems in this gate:
-
Boot race.
AuthContext.useris undefined until boot resolves, so a logged-in visitor is classified anonymous on the first passes. The gate only holds because GrowthBook'sreadyhappens to land after boot — an implicit ordering invariant, not a guarantee. Once real unit ids exist, the failure mode is an ad rendered and then removed on a Plus member's post page, with the layout shift that comes with it. Gate onisAuthReady/isBootLoadedinstead of onuserbeing falsy. -
Enrollment with zero possible exposure. Every id in
ORGANIC_ADSENSE_SLOTSis empty, sohasLiveAdsenseUnitsis false and nothing can render — but this hook is called at/posts/[id]page level, sopost_adsenseis evaluated (and therefore enrolled) for every anonymous post-page visitor while variant and control are byte-identical. AddhasLiveAdsenseUnits(ORGANIC_ADSENSE_SLOTS)toshouldEvaluate.
Reviewed by AI.
There was a problem hiding this comment.
Resolved (0d82b56): the gate is isAuthReady && !user via a shared useIsAnonymous — nobody gets slots while boot is in flight, so a member's session can never land after an ad rendered. And post_adsense only evaluates when exposure is possible: shouldEvaluate: isAnonymous && hasLiveAdsenseUnits(ORGANIC_ADSENSE_SLOTS), so the empty-id map enrolls no one ('never enrolls into post_adsense while no unit has an id' test).
| return; | ||
| } | ||
| router.events.emit('routeChangeError'); | ||
| window.location.assign(url); |
There was a problem hiding this comment.
Blocking. routeChangeStart also fires for popstate, so this intercepts browser Back and Forward. window.location.assign pushes a new entry, so pressing Back from /read navigates forward to the previous URL and leaves /read in the forward stack — Back reads as broken and repeated presses can trap the user.
The throw also escapes as an uncaught error into the console and error reporting on every intercepted navigation, which makes real errors harder to see.
Suggested direction: handle popstate through router.beforePopState (returning false after a location.replace), and keep assign only for genuine forward navigations. Please confirm Back, Forward and a refresh from /read on desktop and on a phone before this lands — this is the same class of global-routing interception that broke every feed-card modal in apps#6439.
Reviewed by AI.
There was a problem hiding this comment.
Resolved (0d82b56): popstate goes through router.beforePopState — /read-internal moves pass through, genuine departures cancel the SPA transition and load the URL the history pointer already moved to in place, so Back keeps meaning back and /read doesn't linger in the forward stack. location.assign remains only for link/push navigations, where a push is what was asked for, with the shallow guard intact. Will confirm Back/Forward/refresh on the preview desktop + phone once it deploys.
There was a problem hiding this comment.
Verified on the deployed preview (head with 0d82b56 + 53c93c9), desktop 1440px and phone 375px, same results on both:
- Back from /read → lands on
/posts/qojM1enSN(the actual previous entry),performance.navigation.type = back_forward,window.adsbygooglegone (full unload), history length unchanged — Back means back, nothing pushed. - Forward → returns to /read with ads reloading, history length still unchanged — /read is not stranded in the forward stack.
- Refresh on /read → stays on /read,
type = reload, ads reinitialise.
Also confirmed while there: the phone renders the reduced density set (rail units' wrappers display:none below laptop).
- Mobile density: only the first rail unit keeps its phone placement, bringing the phone run from ~40% of page height to ~27%, inside the Better Ads Standards 30% cap whose violation filters ads domain-wide. - /read is generated only for articles, videos and collections that carry a summary or body; title-plus-ads shells 404, because AdSense's low-value-content enforcement is account-level. - read_adsense boolean returns as an emergency kill switch, default on by design (documented as a non-experiment). - Both surfaces now serve anonymous visitors only, gated on isAuthReady so a logged-in boot can never flash an ad before the user resolves. - post_adsense enrollment additionally requires a live unit id, so the experiment population can't fill with users who could never see it. - Back/forward from /read goes through beforePopState with an in-place load, keeping history intact instead of navigating forward. - Non-blocking picks: plans/ ignored wholesale, one-shot telemetry event when data-adtest engages so silent test-mode in production is visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-review blockers resolved (0d82b56)All seven blocking items from the changes-requested review:
Non-blocking picks in the same push: Verification: 109 shared post+sidebar tests (7 new: logged-in/read, auth-not-ready, kill switch, empty-id enrollment, and the existing gating set), webapp suite green except the pre-existing 🤖 Generated with Claude Code |
On top of the blocker fixes already on the branch: the fork's rationale and fold-back debt recorded on ArbitragePostContent; the push-ordering invariant documented at the push site so nobody adds `eager` and quietly re-opens the mis-binding race; the two near-identical timer hooks merged into useTimedRelease; and the anchor offset on the shared footer made an explicit opt-in from /read instead of compensating CSS in a component every page renders.
|
@rebelchris — round 2 is resolved on the branch. All seven blockers are addressed in
Remaining from your "not verified" list, on us: Back/Forward/refresh on the deployed preview (desktop + phone) once this head deploys, and the organic slots can only be exercised once real unit ids exist. |
The box rendered whether or not a creative arrived, so the bottom of the nav carried a bordered empty band with a dismiss button and nothing to dismiss — measured live at 41px of border-plus-padding around a collapsed slot, and taller while the format still reserved a min-height. It now stays out of the layout until the slot reports a creative, and reserves nothing while hidden; the slot itself stays mounted so it can still request and report. Fill is reported before the collapse grace, so the box reveals when the ad lands rather than on the four second timer. Both dismiss buttons go subtle and extra small — the anchor's solid primary was the loudest thing on the page.
The /read container padded its bottom by 7rem plus the anchor's height. The 7rem predates the anchor measuring itself and also double-counted the mobile footer nav, which FooterNavBarLayout already spaces for — so on desktop, where neither exists, the page simply ended in 112px of nothing below the rail's last slot. Measured live: rail bottom 788, container bottom 900. Now the container's own pb-6 plus whatever the floating leaderboard actually occupies, which is nothing while it is absent or unfilled.
Cancelling it with p-0 did not work — Tailwind emits the padding shorthand before the per-side utilities, so px-2/pt-1/pb-3 won and the hidden box still measured 16px under the nav, h-0 notwithstanding. The padding now only exists in the filled state, so hidden is genuinely zero.
It behaved like layout, not like a floating placement. The page padded its bottom by the ad's measured height and the footer wrapper was lifted by it at every width — so on desktop, where the ad is confined to the article column and the wrapper's only occupant is the scroll-to-top button over on the right, the button was parked in mid-air above a gap it had no reason to clear. The offset now applies only below laptop, where the ad does span the width and the nav bar would otherwise sit on top of it, and the page reserves nothing. Also stops slots collapsing mid-request. Emptiness was judged four seconds after the push whether or not AdSense had touched the element, so a slow auction looked exactly like a declined one — and once collapsed the slot is display:none, which Google will not render into, so it never came back. That is the "loads very slowly, or not at all" behaviour. A slot now collapses on the tag's own data-adsbygoogle-status being 'done', with a far longer window before an untouched slot (blocked or still-loading script) is written off.
2 participants
Adds a third post-page template at
/posts/[id]/read, built for paid-acquisition landing traffic where the goal is ad impressions rather than signups, plus two flagged AdSense units on the organic post page. The two existing templates' layouts are untouched.Live AdSense units on the preview (test creatives on any non-production host via
data-adtest):/posts/qojM1enSN/read.Audience and switches: both surfaces serve anonymous visitors only (gated on
isAuthReady, so a logged-in boot never flashes an ad; Plus stays ad-free).read_adsense(default on) is the /read kill switch — not an experiment;post_adsense(default off) gates the organic units and only enrolls when a real unit id exists. /read renders only Article/Video/Collection posts carrying a summary or body; other ids 404.Architecture
PostWidgets,PostEngagements,PostComments,GoBackHeaderMobile) so everything between the ads is pixel-identical to/posts/[id]. For scraped articlescontentHtmlis empty on both templates, so the focus card's body-rendering advantage never applies to this traffic, while its missing widget column would cost three always-viewable slots.arbitrage/slots.ts), not remote config — they're public in any page's source, and a GrowthBook JSON value shipped the map in every surface's boot payload./readunits are always on (the route is only reachable via paid placements); the organic units sit behindpost_adsense(default off) and render for anonymous visitors only.adsbygoogleloads only on routes that render units, and leaving/readforces a full page load so Google globals can never follow a client-side navigation into the app./read(PostAuthBanner,CustomAuthBanner,PostSignupWidget); header login/signup unaffected. Route isnoindex(no canonical — mixed signals) until it's decided as the organic canonical.Placements
/read — leaderboard above the article (pins for 10s after first scroll, then releases; on mobile it pins as one block with the back/Read-post header and hands the top over to it); 336×280 beside the cover, bottom-aligned with it; native unit every 5 comments in long threads; two multiplex grids after the discussion; rail: one slot after every real widget (none after the house ad or sponsored-tools card — already commercial) closed by a sticky 300×600 (desktop) / multiplex grid (mobile); dismissible compact unit under the sidebar nav; dismissible floating leaderboard aligned to the article column, delayed 10s, offsetting the mobile footer nav by its measured height.
Organic (
post_adsense+ anonymous) — leaderboard above the container (never in post modals), sticky half page closing the widget column.Live at merge
Empty ids collapse to nothing and never load the script; the placements ship reviewed once and switch on by filling in
slots.ts.Review round 1 — resolved
All five blockers from the internal review are addressed in
dfbe38ff1: internal strategy docs removed via history rewrite (+ subtree-wide.gitignore); cpm/reach stripped from shipped code;post_adsenseevaluated conditionally (anonymous only) and never from /read slots; script gated on a real unit id existing; and theadsbygoogle.pushordering race fixed by mounting each<ins>only at eligibility. Non-blocking notes handled too: shallow-navigation guard, sidebar inverted to asidebarTrailingnode (extension no longer bundles slot code via the nav),FurtherReadingboundary guard, and first-party fill/dismiss analytics.Mechanics worth knowing
--sticky-header-offsetis published byMainLayoutfor every pinned unit; hardcoded offsets overshoot wherever the chrome differs (v2 sidebar, banner, mobile).data-ad-format: rectangle|horizontal|vertical) — a width cap alone happily returns a 300×600 into a 300×250 placement. A unit's type is fixed at creation in AdSense: in-article units are fluid regardless of the<ins>attributes, which is why slot 3 borrows a Display unit id (TODO inslots.tsto create dedicated units for s03, and per-placement ids for s13/second grid to unblend reporting).Shared-component changes (all gated, existing consumers unchanged)
PostWidgetsgainshideSignupWidget/hideToc/getRailAd(position)/trailing;PostComments/PostEngagementsgaininterleaveEvery/renderInterleaved;FurtherReadinggainshideToc/betweenSections;MainLayout/Sidebargain opt-inexpandSidebar/forceExpanded.Verification
Preview domain
https://claude-ad-arbitrage-assets-analy.preview.app.daily.dev