Skip to content

feat: post hero ad placement#6166

Open
capJavert wants to merge 15 commits into
mainfrom
post-hero-ads-placement
Open

feat: post hero ad placement#6166
capJavert wants to merge 15 commits into
mainfrom
post-hero-ads-placement

Conversation

@capJavert

@capJavert capJavert commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Changes

  • adds configurable feature flag for highlight cards and settings
    • tag/chip labels (major, evergreen...)
    • spacing
  • ads placement is now aware of hero cards and ads are placed accordingly
    • eg. if one hero post occupies 3 slots and ad was supposed to be placed in 3 posts, its placed behind hero post immediately as if 3 posts were shown
    • this preserves ad places and should preserve ad impressions
    • also added startIndex (default 4) so that first ad is never pushed by first hero card
  • recommended size of hero cards is now returned from API for better control
  • fixed few circular imports (.../hooks barrel)

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://post-hero-ads-placement.preview.app.daily.dev

@vercel

vercel Bot commented Jun 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
daily-webapp Ready Ready Preview Jun 10, 2026 1:42pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
storybook Ignored Ignored Jun 10, 2026 1:42pm

Request Review

@capJavert

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @capJavert's task in 4m 29s —— View job


Review: post hero ad placement

Solid refactor — extracting a single stateful createPlacementBuilder so both useFeed (ad cadence) and Feed (rendering) share one placement rule is the right call, and the test coverage for the cadence shift is appreciated. A few things worth a look before merge:

🔴 High — likely TS regression: deleted module augmentation

packages/shared/src/graphql/types.ts previously contained only a global augmentation:

declare module 'graphql-request/dist/types' {
  interface GraphQLError {
    extensions?: Record<string, unknown>;
    message?: string;
  }
}

This PR replaces the entire file with the PostHero* types, silently removing that augmentation (it wasn't relocated — I confirmed only src/types.ts's unrelated graphql-request augmentation remains). Several files read .extensions / .message off GraphQLError-typed values that relied on it:

  • packages/shared/src/lib/form.ts:82originalError.response?.errors?.[0]?.extensions?.code
  • packages/webapp/pages/game-center/index.tsx:118errors?.some(({ message }) => message?.includes(...))
  • InlineContentEditor.tsx, InlineRoleInfoEditor.tsx, and the OpportunityEdit*Modal editors — same .extensions?.code / .message pattern

Please run pnpm --filter @dailydotdev/shared exec tsc --noEmit (deps aren't installed in my env so I couldn't verify). If it breaks, keep the augmentation — e.g. move the PostHero* types into a new file and leave graphql/types.ts as-is, or append rather than overwrite. Fix this →

🔴 High — an ad can be dropped when a wide card straddles the cadence point

getAd matches only on an exact modulo: adIndex % adRepeat === 0 (useFeed.ts:378). Now that the index is visualCellsSoFar (which jumps by the wide card's colSpan), a wide card whose span steps over a multiple of adRepeat means that index is never checked, so the ad slot is skipped entirely:

e.g. adStart=4, adRepeat=8 → match at vcs 4, 12, 20… If items reach vcs 11, then a span‑3 hero pushes vcs to 14, the check at 12 never happens and that ad is lost.

minSpacing (default 10) and adRepeat are close enough that wide cards and the ad cadence will drift into each other over a long feed, so this isn't purely theoretical — and it directly undercuts the PR's "preserve ad impressions" goal. The added tests only cover spans that land exactly on the boundary (the size 4 at index 0 → vcs 4 case), not the straddle case. Worth either making getAd catch up when the index crosses a multiple since the last ad, or asserting a straddling-span test that proves the ad still appears.

🟡 Medium — layout inputs now derived independently in two places

The two builders must receive identical numCards / isMobile / isList / isEnabled / minSpacing / startIndex or the ad cadence (computed in useFeed) won't match the rendered layout (Feed). But:

  • useFeed.ts derives numCards from useContext(FeedContext).numCards.eco, while Feed.tsx:210 uses currentSettings.numCards.eco — two different sources that happen to agree today. A shared helper/hook would prevent silent drift.
  • useFeed's builder is fed items without fullRowInsertionBeforeIndex, while Feed's computePlacements resets the column on full-row banners. A wide card following a banner can therefore compute a different colSpan in useFeed than what's actually rendered → cadence drift around briefs/promos. Worth a comment noting the limitation, or threading banner positions through.

🟡 Medium — settings toggle now always rendered

appearance.tsx dropped the useConditionalFeature gate, so "Show featured cards for highlighted news" now shows for all users, including those not in the experiment (where toggling it is a no-op). Per CLAUDE.md ("when removing a feature flag, don't assume the gated UI should become always-on") — confirm this is intentional, otherwise gate it on the config's enabled.

🟢 Nits

  • HighlightChip (ArticleFeaturedWideGridCard.tsx) now calls useConditionalFeature per card just to read chipLabels. The config is already fetched in useFeed and passed into Feed — consider threading chipLabels down instead of a 3rd evaluation site.
  • numCards = numCardsBySpaciness?.eco ?? 1 hardcodes .eco; matches Feed.tsx today but the coupling is implicit — a one-line comment would help the next reader.

  • Read changed files & understand ad placement logic
  • Review useFeed.ts ad placement changes
  • Review feedHighlightColSpan.ts changes
  • Review feature flag & settings changes
  • Review GraphQL/type changes
  • Post review feedback
    · branch post-hero-ads-placement

@capJavert capJavert force-pushed the post-hero-ads-placement branch from 4d42a36 to a6e55d3 Compare June 10, 2026 10:53
@capJavert

Copy link
Copy Markdown
Contributor Author

@claude review

likely TS regression: deleted module augmentation - its intended
settings toggle now always rendered - also intended

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @capJavert's task in 1m 46s —— View job


Re-review: post hero ad placement

Thanks for the clarifications — noting both as intended and dropping them:

  • Deleted graphql-request/dist/types augmentation — intended.
  • Settings toggle now always rendered (appearance.tsx) — intended.

The branch has also changed since my first pass, and the headline issue I flagged is now fixed. Walking the current code:

✅ Resolved — wide card straddling an ad slot (was my 🔴 #2)

The "an ad can be dropped when a wide card steps over the cadence point" bug is handled now. Both placement sites clamp the wide card so its right edge can't cross the next scheduled ad slot:

  • useFeed.ts:572-577pushAndAdvance computes nextAdVcs = adStart + adsFired * adRepeat and passes maxColSpan = Math.max(1, nextAdVcs - visualCellsSoFar) into the builder.
  • feedHighlightColSpan.ts:213-216computePlacements does the same against its own vcs/adsFired walk.
  • createPlacementBuilder folds maxColSpan into the fit clamp at feedHighlightColSpan.ts:160 (Math.min(clampedToGrid, remainingInRow, maxColSpan)).

So a span‑3 hero at vcs=11 with nextAdVcs=12 shrinks to span‑1, visualCellsSoFar lands exactly on 12, and getAd fires (adIndex % adRepeat === 0). The impression is preserved (at the cost of downgrading that one hero), which is the documented trade-off. 👍

✅ Resolved — layout-input drift between the two builders (was my 🟡 #1)

This was my other main concern and it's largely gone:

  • Rendering now consumes placements straight from useFeed (Feed.tsx:262-263) instead of recomputing — single source of truth for colSpan.
  • useFeed's pushAndAdvance now threads fullRowBefore from fullRowInsertionBeforeIndex (useFeed.ts:571), and computePlacements honors the same set, so banner/hero/promo full-row insertions are banner-aware on both the cadence and the render path. The "wide card after a banner computes a different colSpan" gap is closed.

Both useFeed.ts:246 and Feed.tsx:206 still read numCards.eco from FeedContext, so they agree by construction now — the only residual difference is useFeed's ?? 1 guard, which is harmless.

🟢 Remaining nit (non-blocking)

  • HighlightChip evaluates the flag per cardArticleFeaturedWideGridCard.tsx:57-60 still calls useConditionalFeature(featurePostHighlightCards) inside every wide card just to read chipLabels/enabled. The config is already resolved once in useFeed and flows into Feed. Threading chipLabels (or the whole postHighlightCardsConfig) down as a prop would avoid an Nth evaluation site and keep the card in sync with the feed's config. Minor — shouldEvaluate: !!significance keeps it cheap — but worth a follow-up. Fix this →

Net: the two items you called out are acknowledged as intentional, the high-severity ad-drop case is fixed with symmetric clamps on both paths, and the cadence/render drift is resolved by centralizing placements in useFeed. Only the per-card flag evaluation remains as an optional cleanup. Nice work on the maxColSpan approach. ✅

  • Acknowledge the two intended items
  • Re-verify the straddle ad-drop case
  • Re-verify layout-input drift concerns
  • Re-check remaining nits
    · branch post-hero-ads-placement

@capJavert

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @capJavert's task in 2m 38s —— View job


Re-review: post hero ad placement

Walked the current branch end-to-end. The two big items from my earlier passes are still in good shape, and the items you flagged as intentional are noted below. One new finding worth a look.

🟡 Medium — adsFired desyncs when the first ad slot is skipped, suppressing wide cards for the rest of the feed

In useFeed.ts:634, when shouldSkipAdForMarketingCta is true the code intentionally pushes nothing — but that also means pushAndAdvance is never called for that slot, so adsFired stays 0 and visualCellsSoFar doesn't advance.

getAd fires on a pure modulo of visualCellsSoFar (useFeed.ts:520), so subsequent ads are not dropped — good. But the maxColSpan clamp keys off adsFired:

const nextAdVcs = adStart + adsFired * adRepeat; // useFeed.ts:604
const maxColSpan = Math.max(1, nextAdVcs - visualCellsSoFar);

Because the skipped first ad never increments adsFired, nextAdVcs stays one slot behind visualCellsSoFar for the entire rest of the feed. Once visualCellsSoFar passes adStart, nextAdVcs - visualCellsSoFar goes negative and maxColSpan collapses to 1 — so every wide hero card downstream is clamped to a regular card. Net effect: users who get a marketing CTA / plus entry as their first card never see wide hero cards.

computePlacements shares the same lag (the absent ad isn't in items, so its adsFired walk also stays behind), so render and cadence stay consistent — this is hero suppression, not an ad drop or layout drift. But it does quietly undercut the feature for that cohort.

Cleanest fix is to derive the next ad slot from visualCellsSoFar directly rather than from a running adsFired counter, so it's robust to skipped slots — e.g.:

const slotsPassed = Math.max(0, Math.ceil((visualCellsSoFar - adStart) / adRepeat));
const nextAdVcs = adStart + slotsPassed * adRepeat;

(applied symmetrically in computePlacements). Either that, or advance adsFired even when the slot is skipped. Fix this →

🟢 Nits (non-blocking)

  • HighlightChip still evaluates the flag per card (ArticleFeaturedWideGridCard.tsx:57) — same nit as last pass. postHighlightCardsConfig is already resolved once in useFeed; threading chipLabels/enabled down avoids an Nth evaluation site. Cheap (shouldEvaluate: !!significance), so fine as a follow-up.
  • Type homesPostHighlightSignificance/PostHeroSignificance/PostHero consolidated into graphql/types.ts (good, removes the prior dupes in posts.ts/highlights.ts). All the new cross-file imports (types.tsgraphql/types.ts, featureManagement.tstypes.ts) are type-only, so no runtime cycle. 👍

✅ Confirmed / acknowledged

  • Wide card straddling an ad slot — still fixed; symmetric maxColSpan clamp on both useFeed.ts:604-609 and feedHighlightColSpan.ts:213-216.
  • Single source of truth for placementsFeed.tsx now consumes placements/bannerInsertions straight from useFeed; the hero/banner/highlight logic was hoisted into the hook cleanly, and numCards.eco reads agree on both paths.
  • Deleted graphql-request/dist/types augmentation — intended.
  • Settings toggle now always rendered (appearance.tsx) — intended.
  • Circular-import fixes (Feed.tsx, FeedContext.tsx, WritePostContext.tsx, useSearchResultsLayout.ts now import from direct paths instead of the ../hooks/../contexts barrels) — consistent with the no-barrel rule. 👍

Net: core ad-cadence logic is solid and the resolved items hold. The one thing I'd look at before merge is the adsFired desync on the skipped-first-ad path — it's consistent across both placement paths (so no drift/ad loss), but it does silently disable wide cards for the marketing-CTA-first-card cohort.

  • Read changed files & re-check prior findings
  • Review ad placement / colSpan logic
  • Review feature flag, settings & GraphQL changes
  • Post review
    · branch post-hero-ads-placement

@capJavert

Copy link
Copy Markdown
Contributor Author

@claude review

HighlightChip useConditionalFeature call is fine, it has no cost

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @capJavert's task in 1m 30s —— View job


Re-review: post hero ad placement

Walked the current branch. The one open finding from my last pass is now fixed, and the nit you called out is acknowledged.

✅ Resolved — adsFired desync on the skipped-first-ad path (was my 🟡 from last pass)

The maxColSpan clamp no longer keys off a running adsFired counter — both placement paths now derive the next ad slot from visualCellsSoFar/vcs directly:

  • useFeed.ts:603-608slotsPassed = Math.floor((visualCellsSoFar - adStart + adRepeat) / adRepeat)nextAdVcs = adStart + slotsPassed * adRepeat.
  • feedHighlightColSpan.ts:213-218 — same formula in computePlacements, symmetric.

Walking the previously-broken cohort (marketing-CTA / plus-entry as first card, adStart=4, adRepeat=8): the first slot is skipped, nothing is pushed, vcs stays 4. The next post hits pushAndAdvance with vcs=4slotsPassed=1, nextAdVcs=12, maxColSpan=8 — so wide hero cards are no longer suppressed for that cohort. The regression test at feedHighlightColSpan.spec.ts:592 ("does not suppress wide cards when the first ad slot is missing from items") asserts exactly this (placements[4].colSpan === 4). 👍

Boundary checks hold too: at vcs=3 (adStart=4) the formula gives nextAdVcs=4, maxColSpan=1, so a wide card still can't straddle the very first slot — covered by feedHighlightColSpan.spec.ts:575.

✅ Acknowledged

  • HighlightChip per-card useConditionalFeature — noted as intentional / no cost (shouldEvaluate: !!significance keeps it cheap). Dropping it.
  • Deleted graphql-request/dist/types augmentation — intended.
  • Settings toggle now always rendered (appearance.tsx) — intended.

✅ Still confirmed from prior passes

  • Wide card straddling an ad slot — fixed via symmetric maxColSpan clamp on both paths.
  • Single source of truth for placements — Feed.tsx consumes placements/bannerInsertions from useFeed; numCards.eco reads agree on both paths.
  • Circular-import fixes (direct imports instead of ../hooks/../contexts barrels) — consistent with the no-barrel rule.
  • New PostHero* types consolidated in graphql/types.ts, all cross-file imports type-only (no runtime cycle).

Net: all substantive findings from earlier passes are resolved, the desync fix is symmetric across the cadence and render paths with a dedicated regression test, and the remaining items are confirmed intentional. LGTM. ✅

  • Read changed files & re-check prior findings
  • Re-verify the adsFired desync issue (skipped first ad slot)
  • Review ad placement / colSpan logic
  • Post review
    · branch post-hero-ads-placement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant