Skip to content

fix(shared): preserve card sequence with in-feed reminder hero#5780

Open
tomeredlich wants to merge 5 commits intomainfrom
fix/in-feed-hero-row-sequence
Open

fix(shared): preserve card sequence with in-feed reminder hero#5780
tomeredlich wants to merge 5 commits intomainfrom
fix/in-feed-hero-row-sequence

Conversation

@tomeredlich
Copy link
Contributor

@tomeredlich tomeredlich commented Mar 23, 2026

Summary

  • adjust in-feed hero insertion index to account for pre-feed slot cards (brief/profile completion)
  • keep hero rendering in flow while preserving card sequence and row boundaries
  • scope change to feed placement logic only (Feed.tsx)

Test plan

  • Run pnpm --filter shared exec eslint \"src/components/Feed.tsx\"
  • On homepage, verify in-feed hero appears in its own row
  • Confirm cards before/after hero keep correct sequence with no missing items
  • Verify behavior remains correct when brief/profile completion first slot is present

Made with Cursor

Preview domain

https://fix-in-feed-hero-row-sequence.preview.app.daily.dev

Adjust in-feed hero insertion index to account for pre-feed slot cards so the hero renders in its own row without breaking card order.

Made-with: Cursor
@vercel
Copy link

vercel bot commented Mar 23, 2026

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

Project Deployment Actions Updated (UTC)
daily-webapp Ready Ready Preview Mar 23, 2026 3:44pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
storybook Ignored Ignored Mar 23, 2026 3:44pm

Request Review

@tomeredlich
Copy link
Contributor Author

@claude check pr

@claude
Copy link
Contributor

claude bot commented Mar 23, 2026

Claude finished @tomeredlich's task in 3m 16s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

Summary

The fix adjusts the in-feed hero insertion index by -1 when a pre-feed slot card (profile completion or brief) is present. This is consistent with the existing indexWhenShowingPromoBanner pattern (line 600) which already applies the same Number(showFirstSlotCard) offset.

Why the fix is correct: heroInsertIndex is calculated as Math.ceil(HERO_INSERT_INDEX / itemsPerRow) * itemsPerRow — a multiple of itemsPerRow, targeting a row boundary. When a pre-feed card occupies 1 grid slot before the items, every item shifts right by 1 slot in the grid. Subtracting 1 from the insertion index re-aligns the hero to the correct row boundary. Verified for 1, 2, 3, and 4 column layouts.


Issues

Minor: shouldShowInFeedHero guard is stale after the adjustment

In useReadingReminderFeedHero.ts:97:

itemCount > heroInsertIndex;  // e.g. itemCount > 6

But now the hero is actually inserted at adjustedHeroInsertIndex (e.g., 5 when showFirstSlotCard is true). The guard should be itemCount > adjustedHeroInsertIndex in that case. The practical impact: when showFirstSlotCard === true and itemCount === heroInsertIndex exactly (e.g., exactly 6 items), the hero won't show even though the adjusted position (index 5) is valid. This is a narrow edge case and doesn't block the PR — just worth noting.

Fix this →


Looks good

  • Math.max(..., 0) guard correctly prevents negative indices
  • Mutually exclusive pre-feed card rendering (showBriefCard && !showProfileCompletionCard) means firstGridSlotOffset is always 0 or 1 — no double-counting
  • Scope is tight — only Feed.tsx touched
  • Approach is consistent with how indexWhenShowingPromoBanner already handles the same offset

Pass the first feed-slot offset into the reminder hook and use the adjusted index for inline hero visibility checks, so edge cases with a pre-feed card still render consistently.

Made-with: Cursor
Align reminder widget interactions with latest UI updates by removing the inline notification CTA, updating quick option labels, and preventing overflow in the options row. Also normalize enable-notification action alignment in modal CTA rendering.

Made-with: Cursor
Keep in-feed hero placement after card rows while allowing debug mode to bypass homepage and scroll gates for easier QA. Also ensure the close action still hides the hero during the current session in debug mode.

Made-with: Cursor
@tomeredlich
Copy link
Contributor Author

@claude check pr

@claude
Copy link
Contributor

claude bot commented Mar 23, 2026

Claude finished @tomeredlich's task in 2m 5s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

Summary

The PR fixes in-feed hero row alignment when a pre-feed slot card (profile completion or brief) is present. The approach is sound and consistent with the existing indexWhenShowingPromoBanner offset pattern.


Core Fix — Feed.tsx + useReadingReminderFeedHero.ts

The fix is correct. firstSlotOffset is now passed into the hook, which computes adjustedHeroInsertIndex = Math.max(heroInsertIndex - firstSlotOffset, 0) and uses it both for the shouldShowInFeedHero guard (line 106) and for the render index check in Feed.tsx (lines 594–597). The two computations use the same firstSlotOffset, so they stay in sync.

One minor note: adjustedHeroInsertIndex is computed twice — once inside the hook (for the guard) and once in Feed.tsx (for rendering). The hook still returns the unadjusted heroInsertIndex, which Feed.tsx only uses to re-derive the adjusted value. It would be slightly cleaner to return adjustedHeroInsertIndex from the hook directly and remove the recomputation in Feed.tsx, but this is low-priority. The current behavior is correct.


notificationExperimentDebug.ts (new file)

Well-structured debug utility. Properly gates behind isDevelopment, uses query params with localStorage persistence, and handles localStorage failures gracefully. No issues.


Remaining Files

  • EnableNotification.tsx (+45/-26): Notification experiment copy/layout additions. Logic is coherent, source-based branching is consistent with the existing pattern.
  • PostReminderOptions.tsx, PostContentReminder.tsx, useReadingReminderHero.ts: Minor cleanups, no issues observed.

Scope Note

The PR description says "scope change to feed placement logic only (Feed.tsx)" but 7 files changed. The additional changes appear to be legitimate polish bundled in the same branch — worth keeping the description accurate for future reference, but not a blocker.


Overall: looks good. The stale-guard issue from the previous review has been addressed. The fix is correct and well-scoped.

label,
onEnableAction,
}: EnableNotificationProps): ReactElement | null {
const [isDismissedLocally, setIsDismissedLocally] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you introduce state instead of using onDismiss directly?

@@ -0,0 +1,136 @@
import { isDevelopment } from '../../lib/constants';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should delete this file

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.

2 participants