feat(feed): add the feed hero section behind a flag - #6515
tsahimatsliah wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
The failure is The Running the full shared suite locally on this branch's exact tree: 2598 passed, 1 failed — that one. Re-running the job would fail identically, since the failure is deterministic on the current date rather than flaky, so I have not spent a re-run on it. The fix is one line, and I verified it locally (all 8 tests in the file pass, including the expiry case, which sets its own relative date and is unaffected): - endDate: '2026-09-01T00:00:00.000Z',
+ // Relative so the fixture can't expire on the calendar: a running campaign is
+ // one whose end date is still ahead. The expiry case below sets its own.
+ endDate: new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(),I have deliberately not pushed it here — it is unrelated to this PR and belongs in its own change so it can land for everyone rather than waiting on this review. Happy to open that separately if useful. Generated by Claude Code |
Removes the hero's narration of its own history — rejected approaches, pixel measurements from debugging, and a line-clamp note repeated once per card — leaving why-notes for the constraints that are unguessable from the code. Also drops `useFeedHeroAd`'s `enabled` parameter, which had one caller passing a literal true. Mockup-to-eng-pass: 1 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Feed hero section (headline carousel + Happening Now rail + its own ad column) behind feed_hero, default off, with three new Feed settings to make room for it. The layout reasoning is sound — deriving the row from the feed grid's own numCards.eco rather than fresh viewport thresholds is the right call, the skipFirstAd shift (rather than drop-and-discard) is the right ad behaviour, reduced motion genuinely stops the rotation rather than just shortening it, and the isAdShown gate on the impression effect avoids marking a cached ad LOGGED for a render that never happened. Tests cover the shape table, the carousel paging and the two feed-settings behaviours; CI is green.
Findings below. Two are blocking, both about behaviour that leaks outside the flag or outside the hero's own success path.
Blocking
MainFeedLayout.tsx:829— a non-nulltopContentsilently suppresses the reading-reminder top hero (and leaves its impression firing).cards/highlight/common.tsx:86— theHighlightRowrestyle is unconditional, so the in-feed highlights card changes for everyone with the flag off.
Non-blocking
MainFeedLayout.tsx:913—disableHighlightCards/deferWideCardsfollow the flag, not the hero's actual render.useFeed.ts:594—skipFirstAdflips mid-session when the hero's ad resolves.useFittedLineClamp.ts:48— a clamp of0is an invalid declaration, which unclamps the text it was meant to clamp.useFittedLineClamp.ts/FeedHeroCarousel.tsx— measurement hook and a second carousel; worth stating why the static/existing options do not fit.FeedHero.tsx:151— hero post clicks are indistinguishable from in-feed clicks, and hero posts log no impression.FeedHero.tsx:73— dependent round trip on the most latency-sensitive surface.FeedHeroSection.tsx:15— the column-class maps stop at the current maximum column count.
Verification
- Root and package
AGENTS.mdreviewed; scope matches the stated task - Flag-off path traced (
featureFeedHerodefaultsfalse,shouldEvaluatelimited to My Feed / Popular) - Shared consumers enumerated (
HighlightGrid,HighlightList, the in-feed featured-wide cards,FeedItemComponentcard maps) - Ad cadence, impression logging and
topContentinteraction traced againstFeed.tsx/useFeed.ts - CI inspected (green) — I did not run the suite locally
- Cold load on a non-v2 laptop cohort, and a load where
majorHeadlinesreturns nothing, still need manual confirmation
Verdict
Request changes for 1 and 2; the rest are questions and polish.
Reviewed by AI.
Addresses review on #6515. The feed's top slot: a non-null `topContent` shadowed `ReadingReminderFeedHero` for the whole variant cohort while its impression kept firing. `Feed` now takes `disableTopHero`, so the suppression is explicit and stops the impression with it. The highlights row: the drawn rule and the inline timestamp were unconditional, so the in-feed card changed shape for readers with the flag off. Both are now scoped to `compact`, with a test pinning the in-feed row. `disableHighlightCards` and `deferWideCards` followed the flag rather than the hero's render, so a hero that found no headlines left the reader with neither it nor the grid's highlights card. The hero reports `onRenderedChange` and the grid takes its content back. `useFittedLineClamp` could emit `-webkit-line-clamp: 0`, which is invalid and unclamps the text its parent then slices through a glyph row. It returns a style that hides the block instead. Hero posts carry their own origin and log an impression for each slide brought on screen, so the experiment can measure the section against the grid. The carousel accepts a swipe, and `aria-live` returns to `off` on a timer rather than waiting for an automatic advance that never comes while the reader is hovering. An unknown column count falls back to the stacked layout rather than rendering an unclassed grid. Mockup-to-eng-pass: 1 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rebelchris
left a comment
There was a problem hiding this comment.
Re-review of 5a8152fca
Both blockers are fixed, and fixed properly rather than papered over. Checked each one against the code rather than the reply:
- Reading-reminder top hero —
disableTopHerois threadedMainFeedLayout→Feed→useFeed→useReadingReminderFeedHero, composed asisV2 || disableTopHero. Driving it off the flag rather than the hero's render is the right call for the reason given: the hook logs the impression, so it has to be suppressed from the first paint. The placement is now explicitly off for the variant cohort instead of implicitly shadowed by a non-nulltopContent, and no impression fires for a hidden CTA. HighlightRow— non-compact is back to byte-equivalent classes (rounded-8 border-b border-border-subtlest-tertiary px-3 py-2) with the timestamp on its own line carrying its originalmt-0.5 text-text-tertiary typo-footnote; theafter:divider and the inline timestamp arecompact-only. The regression test inHighlightCards.spec.tsxpins both, includingclassNamenot matchingafter:, which is the assertion that would actually catch a re-leak.
Also verified: disableHighlightCards / deferWideCards now follow isHeroRendered; HERO_ORIGIN is shared by the ad and post events; hero post impressions are reported per slide with a per-mount dedupe; useSwipeable and the other hooks are hoisted above both early returns, so the rules-of-hooks violation is gone; FeedHeroSection falls back to stacked for an unknown column count. Your useFeed.ts trace is right and better than my guess — the shared Ad object's mutated impressionStatus does prevent the double log, and a lost log on the position-keyed event is the real risk. Agreed it can stay.
One new blocking finding, from the fix rather than the original code, plus two follow-ups. CI green; I did not run the suite locally.
Blocking
useFittedLineClamp.ts:79—display: nonecollapses the box the hook measures from, so the zero case can oscillate.
Non-blocking
FeedHeroSection.tsx:66— the stacked fallback is applied in the section but not in the shape, so the two disagree.FeedHeroCarousel.tsx:49— slide impressions are not gated on the hero being on screen.
Residual, for whoever owns the reading-reminder placement
The TopHero placement now has no impressions at all from the feed_hero variant cohort on /, including sessions where the hero itself rendered nothing. That is the right trade-off here, but it is a denominator change in someone else's experiment and worth a heads-up rather than a surprise in their next readout.
Verdict
Comment, not a change request — 1 is a small fix and the rest are follow-ups. Happy to look again once 1 lands.
Reviewed by AI.
Addresses the re-review on #6515. `useFittedLineClamp` hid the text with `display: none`, which deletes the box the hook measures its own top edge from: the hidden element reports 0x0 at the origin, that measures as a full viewport of room, the text comes back, and it oscillates. `visibility: hidden` keeps the box in flow so the next measurement reads the same geometry and the count settles. Covered by a new spec that fails on the previous behaviour. The unsupported-column fallback moves from the section into `feedHeroShape`, so `layout` cannot say `wide` while the container stacks — which would have handed the carousel a wide card inside a list column and stopped `railHighlights` dropping the lead. `MAX_HERO_COLUMNS` is now the one place the supported range is stated, pinned both ways in the spec. Hero slide impressions and the rotation itself now gate on the same `useInView` threshold the grid's cards use, so a hero impression means what a card impression means, and the carousel stops burning through posts while the reader is further down the feed. Mockup-to-eng-pass: 1 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # packages/shared/src/components/MainFeedLayout.tsx
rebelchris
left a comment
There was a problem hiding this comment.
Re-review of 79c4f146 (feedHero query)
The new commit does what the latency thread asked for: majorHeadlines → postIds → feedByIds collapses into one feedHero round trip, the client-side re-keying and the two useMemos that kept the orders aligned are gone, and the rail dedupe is now matched on the post rather than on the position — which is the right call once the two lists are genuinely separate pools. Checked the query against daily-api's feedHero on main: field names, nullability and the supportedTypes arg line up, the server's own defaults (first 6, featured 4, capped at 12) replace the client's HIGHLIGHT_COUNT / FEATURED_POST_COUNT, and applyFeedWhere still applies the visibility/blocked filters. The post selection (...FeedPost contentHtml ...UserPost @include(if: $loggedIn)) is byte-equivalent to getFeedPostFragment('contentHtml')'s node body, so the hero's cards get the same payload the grid's do, hero { significance } included. A failed query leaves posts empty, so the hero returns null and hands the highlights card and row one back to the grid — the failure path degrades the way the earlier fix set up. FeedHero.spec.tsx covers the one-query render, both dedupe cases and the empty case.
One blocking finding, which is not from this commit — it has been there since the section was added and neither of my earlier passes caught it. Three follow-ups.
Blocking
FeedHero.tsx:61— hero cards run the bareuseVotePost/useBookmarkPost, whose optimistic update only writes the single-post cache key, which thefeedHeroquery never reads. Upvote and bookmark do nothing visible on the hero's own action row.
Non-blocking
FeedHero.tsx:155— vote/bookmark/downvote events default toOrigin.Feedwith no feed extras, so the hero's engagement is indistinguishable from the grid's — the gap that was closed for clicks and impressions is still open here.FeedHero.tsx:75— headline freshness went fromONE_MINUTE(majorHeadlinesQueryOptions) toStaleTime.Default(five minutes) with this refactor. Intentional for a Happening Now surface?graphql/feed.ts:427— the node selection is hand-inlined rather than shared withgetFeedPostFragment, so the hero can silently drift from the feed's post payload.
Verification
- New commit's diff read in full;
daily-apifeedHeroschema and resolver checked against the client query - Post payload parity with
FEED_POST_CONNECTION_FRAGMENTconfirmed field by field (contentHtml,UserPost,hero { significance },userState { vote }) - Error/empty path traced through
isRendered→onRenderedChange→disableHighlightCards/deferWideCards - Flag-off path re-checked:
FeedHerois only mounted underisFeedHeroEnabled, so the query does not fire for control - Earlier threads re-read; the
useFeed.ts:596skipFirstAdtrace is still the accepted answer and the latency thread is now closed by this commit -
test_sharedwas still running on79c4f146when I wrote this; I did not run the suite locally - Vote/bookmark behaviour on the hero still wants a manual check on the preview once 1 lands
Verdict
Request changes for 1; 2–4 are follow-ups. Happy to re-review as soon as the hero's cards patch their own cache.
Reviewed by AI.
# Conflicts: # packages/shared/src/components/cards/highlight/common.tsx
Addresses the third review on #6515. Upvote and bookmark on a hero card ran the bare hooks, whose optimistic update writes only the single-post key — which the `feedHero` query never reads — so the mutation succeeded and the button stayed grey. `useFeedHeroPostActions` gives the hero the treatment the grid gives itself: an optimistic patch of the query the cards render from, keyed mutations so that patch is not applied a second time when the hero's own subscription sees the mutation succeed, and a subscription so a vote fired from the post page still lands here. Two new specs click the real buttons and assert they press; both fail against the bare hooks. `Origin.FeedHero` replaces the loose string so vote, bookmark, click, impression and ad events all carry one typed origin, and engagement is no longer pooled with the grid's. Headline freshness returns to the one-minute window the in-feed card uses, so the two surfaces the experiment compares are equally stale. The hero's post selection is built from the same `feedPostNodeSelection` the feed's connection fragment uses, so it cannot silently fall a field behind the grid. Mockup-to-eng-pass: 1 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What
A hero section above My Feed and Popular: a carousel of the current headlines, the Happening Now list beside it, and an ad in a column of its own.
Behind
feed_hero, default off.Layout
The section lays out on the feed grid's own column count rather than on viewport thresholds of its own, so it reflows exactly when the feed does and its column edges land on the grid's rather than beside them.
A feed rendering as a list takes the stacked shape whatever its column count says, so the section reads as the same kind of thing as the rows under it.
The featured card
The existing featured-wide cards gain a
heroprop. The cover fills its column instead of being letterboxed, and the card sizes its split from container queries on its own width — the hero is only ever as wide as the reader's feed grid, so a wide monitor set to three cards gives it half the room a five-card feed does. Under 40rem the cover moves under the copy; from 52rem the copy keeps two of five for the 40/60 split.The card height is fixed, so the summary is clamped to the whole lines actually left under the headline (
useFittedLineClamp). The action row keeps its place under a headline of any length, and the copy stops on a line rather than being sliced through the middle.Making room in the feed
Three new
Feedsettings, all off unless the hero is on:disableHighlightCards— the hero already shows those headlines, so the grid does not repeat them.skipFirstAd— the hero seats a placement, so the grid stands its first one down and the reader does not meet two before the first post. The slot is shifted, not dropped: the creative moves to the next slot instead of being fetched and discarded.deferWideCards— the hero already leads with a full-size card, so the grid's first row stays single-column.skipFirstAdfollows the hero's actual placement, which only exists once its column exists and an ad has come back for it — so the feed gets its own placement back whenever the hero has none.Also
PostTypeToTagCard/PostTypeToTagListmove out ofFeedItemComponentintocards/common/gridCards.tsandlistCards.ts, so the hero uses the same mapping rather than a second copy of it.Headline clicks in the hero report the same
feedHighlightsLogEventthe in-feed highlights card does, so the events do not disappear when the flag is on.Data
One
feedHeroquery returns both the featured posts and the Happening Now headlines, so the section paints on a single round trip. Its post selection is built from the samefeedPostNodeSelectionthe feed's connection fragment uses, so the hero's cards carry exactly the grid's payload and cannot silently fall a field behind it. Headlines refresh on the same one-minute window the in-feed highlights card uses.Vote and bookmark on a hero card go through
useFeedHeroPostActions, which patches the hero's own query the wayuseFeedVotePost/useFeedBookmarkPostpatch the grid's — an optimistic write, keyed mutations so the hero's own subscription does not double-apply it, and a subscription so a vote from the post page still lands. Every hero event (click, impression, vote, bookmark, ad) carriesOrigin.FeedHero, so the experiment can separate hero engagement from the grid's.Implementation notes
Why
useFittedLineClamprather than staticline-clamp-Nper container band. The line count does not follow the card's width band — it follows how many lines the headline above it took, which varies per post at the same width. A static clamp would have to assume a worst-case headline and short-change every post with a shorter one, or assume the best case and clip.-webkit-line-clamptakes a number, not a height, so the free space has to be measured once the flex column above is laid out.Why a third carousel rather than extending
containers/Carousel. The overlap is "show one of N with dots"; the differences are structural. That one is a translate filmstrip with items in normal flow atw-[inherit], with an unbounded index whose edges callonClose/onEnd. The hero crossfades two stacked slides in a fixed-height grid row, fills a column of variable width, and wraps. Extending it would mean a second positioning model, a second height model and different index semantics, withSquadTour,PromotionTourandQuestOfferCarouselexposed to the change. Unifying them is worth doing — as its own PR.Heads-up for the reading-reminder placement
While
feed_herois on,MainFeedLayoutpassesdisableTopHero, so theTopHeroreading-reminder placement renders — and logs — nothing for the variant cohort on/, including sessions where the hero itself found no headlines. That is deliberate (an impression for a CTA nobody saw is worse), but it is a denominator change in a different experiment. Whoever owns that placement should know before their next readout rather than after.Known behaviour
Only the highlights card is removed from the grid, not the underlying posts, so a headline can appear in the carousel and again as a card below it. Deliberate: filtering client-side would silently shrink the first page and skew the hero-versus-grid comparison the experiment exists to make. If product wants them deduped it belongs in the feed query.
Testing
feedHeroShape.spec.ts— every column count fills its row exactly; list feeds stack.FeedHeroCarousel.spec.tsx— stacked leads with one story and pages nothing; split and wide page through all of them.Feed.spec.tsx— highlights dropped, and the first ad slot dropped while the survivor keeps its index.feedHighlightColSpan.spec.ts— the wide-card row floor is measured in rows, not items.Features/Feed/Hero.Preview domain
https://claude-hero-carousel-layout-e5e0.preview.app.daily.dev
🤖 Generated with Claude Code