refactor(feed): source the hero from the dedicated feedHero query - #6659
rebelchris wants to merge 3 commits into
Conversation
The section fetched majorHeadlines, sliced the leading four and hydrated them through feedByIds, then re-keyed the answer by id to get its own order back. Both counts lived in the client, and feedByIds is @auth, so the cards never rendered for a logged-out reader on Popular. feedHero returns the posts and the headlines together, already in order and index-aligned, so the section only asks and renders. How many of each the hero shows is now the server's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
There was a problem hiding this comment.
Summary
Collapsing the two client calls (majorHeadlines + feedByIds re-keyed by id) into one feedHero query is a clear simplification, and it fixes the real bug that feedByIds is @auth so the hero came back empty for logged-out readers on Popular. CI is green (shared/webapp/extension tests, strict changed typecheck, lint). No blocking findings; a few non-blocking points below and inline.
Non-blocking
- Dead consumer check - with
FeedHerono longer importingmajorHeadlinesQueryOptions, please confirm whether anything else still consumes it (andRequestKey.FeedByIdswith the'hero'segment). IfFeedHerowas the only caller, drop the now-unused query options in this PR rather than leaving a stranded helper. - Deploy order / soft failure - the description covers it: until dailydotdev/daily-api#4272 is deployed the query fails on validation,
herostaysundefinedand the section renders nothing. Worth noting that TanStack will still retry the failing request three times per mount for every flagged-on reader in that window, so please land the API side first as stated rather than in parallel. - Stacked base - this targets
claude/hero-carousel-layout-e5e06u(#6515, still open). Re-target or rebase ontomainonce #6515 merges so the merge commit does not carry the base branch diff. - Explanatory comments - the new inline comments (server owns the counts, staleTime rationale, index alignment) restate the PR description. Per the repo's style preference the code is readable without them; consider trimming so the rationale lives in the PR/commit rather than the component.
Verification
- Diff against the actual base (
claude/hero-carousel-layout-e5e06u) reviewed, scope matches the stated refactor - Query/error/empty paths traced (empty posts ->
null, query error ->null, background refetch failure keeps on-screen data) - CI: build, tests, lint, strict changed typecheck all pass
- Not verified: preview render logged-out on Popular once the API query is deployed
Reviewed by AI.
| $loggedIn: Boolean! = false | ||
| $first: Int | ||
| $featured: Int | ||
| ${SUPPORTED_TYPES} |
There was a problem hiding this comment.
Non-blocking: $first and $featured are declared and forwarded here but no caller ever passes them, so they are dead variables in the document today. Either pass them from FeedHero (which keeps the tuning knob visible client-side) or drop them until a surface actually needs a different mix - speculative arguments tend to drift from the server defaults they shadow.
Reviewed by AI.
There was a problem hiding this comment.
Dropped them in bf2e432. The mix is the server's call, so the document should not carry knobs that shadow its defaults and then drift from them.
query FeedHero($loggedIn: Boolean! = false, $supportedTypes: [String!] = [...]) {
feedHero(supportedTypes: $supportedTypes) {The args stay in the schema for a surface that one day wants a different mix; nothing sends them today.
| enabled: tokenRefreshed, | ||
| // Breaking headlines, so the same minute the headline query kept rather | ||
| // than the five the post hydration used to. | ||
| staleTime: StaleTime.OneMinute, |
There was a problem hiding this comment.
Non-blocking: moving the post hydration under the headline query's OneMinute staleTime means every window-focus refetch after a minute now re-pulls the four full FeedPost (+contentHtml) cards too, where before only the light headline list refetched that often and posts sat at StaleTime.Default. Probably acceptable for the payload size, but worth being deliberate: if the cards do not need to churn that quickly, consider keeping the previous default here and letting the headline order refresh on the next mount.
Reviewed by AI.
There was a problem hiding this comment.
Agreed, and reverted to StaleTime.Default in bf2e432.
OneMinute came from the headline query, which was cheap enough to refetch at that cadence precisely because the posts sat at Default behind it. Collapsing the two queries dissolved that split, and I carried the wrong half forward. refetchOnWindowFocus is on outside development, so as written every tab refocus after a minute re-pulled four full FeedPost + contentHtml cards.
Headline order now refreshes on the same five minutes as the cards. The section sits above the feed rather than being a ticker, and HighlightPostSidebarWidget still carries the one-minute list for the surfaces that want it.
first and featured were declared and forwarded but never passed, shadowing the server defaults they would have to be kept in step with. The server owns the mix; the document no longer pretends otherwise. staleTime back to Default. OneMinute was the headline query's, from when the posts sat at Default behind it — under one query it would re-pull four full cards on every window focus after a minute, and the section is not a ticker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed in bf2e432. Replies inline on both threads; the summary points: Dead consumer check — checked, nothing is stranded. Deploy order — good catch on the retry cost; three retries per mount for every flagged-on reader is not a free window. dailydotdev/daily-api#4272 lands first, and I will not merge this until it is deployed rather than just merged. Stacked base — yes. Targeting #6515's branch keeps the diff to "what changed about the hero's data source" instead of re-showing the whole section. GitHub retargets to Explanatory comments — fair, and it is the repo rule ( /** In headline order and index-aligned with the highlights, per `feedHero`. */That one is not restating the description — it is the invariant the code below depends on. Logged-out on Popular — still unverified here too. It needs #4272 deployed, so it comes off the preview rather than this diff. |
rebelchris
left a comment
There was a problem hiding this comment.
Re-review (follow-up to the earlier review)
Checked bf2e432 against the prior findings and the replies. CI on the new head is green (build, shared/webapp/extension tests, lint, strict changed typecheck).
Resolved
- Dead
$first/$featuredvariables inFEED_HERO_QUERY— dropped; the document now only carries$loggedInand$supportedTypes. StaleTime.OneMinuteon the hero query — reverted toStaleTime.Default, so the four fullFeedPostcards no longer refetch on every window focus after a minute.- Dead-consumer check — confirmed independently:
majorHeadlinesQueryOptionsis still used byHighlightPostSidebarWidget, andRequestKey.FeedByIdsbyDigestPostContentandFeedByIdsLayout. Nothing stranded. - Explanatory comments — trimmed to the one index-alignment invariant line; that one is fine to keep.
Still open (not new findings, just status)
- Deploy order: dailydotdev/daily-api#4272 is still open, so this remains blocked on that landing and being deployed, as you said.
- Stacked base: #6515 is still open; please re-check the diff after GitHub retargets to
main. - Logged-out render on Popular is still unverified and needs the API side deployed.
No new findings in the follow-up commit. Not approving formally at this stage.
Reviewed by AI.
feedHero now grades its cards across editorial highlights and lifecycle states while the rail stays major headlines, so the two are separate lists and highlights[0] is no longer guaranteed to be the lead card's story. The stacked layout dropped its first headline on position. It now drops the one whose post is actually on the card, and keeps the rest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Updated in 67ff0ea for the change to Per Ido's review there, the hero's cards now come from the The consequence here is that shape.layout === 'stacked' ? highlights.slice(1) : highlightswhich assumed Two specs cover it: a stacked rail drops only the lead story, and a rail keeps a headline whose post is not the lead card. |
What
Stacked on #6515 — targets its branch, not
main.Replaces the hero's two client calls with the dedicated
feedHeroquery added in dailydotdev/daily-api#4272.Why
FeedHerofetchedmajorHeadlines(first: 6), sliced the leading four post ids, hydrated them throughFEED_BY_IDS_QUERY, and then rebuilt a map by id to get the headline order back —feedByIdsanswers in its own order.Both counts were constants in the component, so retuning the mix meant shipping a client. And
feedByIdsis@auth: the hero renders on Popular, where the reader may be logged out, so its cards would have come back empty there.How
One
useQuery.feedHeroreturns the posts and the headlines together, already in headline order and index-aligned, so the component only asks and renders — no slicing, no re-keying.The
HIGHLIGHT_COUNT/FEATURED_POST_COUNTconstants are gone rather than moved. The server defaults to the same 6 and 4, so nothing changes on screen, but the mix is now retunable from the backend. The args stay in the schema for per-surface tuning later.staleTimeisOneMinute— the freshness the headline query kept, not the five minutes the post hydration used. These are breaking headlines.Testing
New
FeedHero.spec.tsx: the cards and the rail both render from the single query, and the section renders nothing when it comes back without posts.The 2 failures in
Feed.spec.tsx(anonymous login modal) fail identically on #6515's branch without this change.Deploy order
dailydotdev/daily-api#4272 ships first, or the hero queries a field that is not there yet.
feed_herois off by default, so the window is a soft failure.Not in scope
#6515's "Known behaviour" note — a headline can appear in the carousel and again as a card in the grid below. Deduping is cheap now that
feedHeroknows which posts it handed over, but it changes what the experiment measures, so it is a product call rather than a refactor.🤖 Generated with Claude Code
Preview domain
https://feat-feed-hero-query.preview.app.daily.dev