feat: call to action to subscribe on the comments of the week#4353
Open
cemreinanc wants to merge 2 commits intomainfrom
Open
feat: call to action to subscribe on the comments of the week#4353cemreinanc wants to merge 2 commits intomainfrom
cemreinanc wants to merge 2 commits intomainfrom
Conversation
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
Contributor
📝 WalkthroughWalkthroughAdds a weekly top-comments subscription CTA and dismissal hook, integrates it into the weekly comments feed, adds localized messages in six languages, and wires an optional onSuccess callback through signin/signup modals to enable subscribing post-authentication. Changes
Sequence DiagramsequenceDiagram
participant User
participant Feed as WeeklyCommentsFeed
participant CTA as SubscribeTopCommentsCta
participant Auth as SignIn/SignUp Modal
participant API as Backend (updateProfileAction)
participant Storage as localStorage
User->>Feed: Open comments-of-week page
Feed->>CTA: Render CTA component
CTA->>Storage: Read dismissal & SUBSCRIBE_INTENT_KEY
CTA->>Auth: Check auth state (currentUser)
alt Not authenticated
User->>CTA: Click Subscribe
CTA->>Storage: Set SUBSCRIBE_INTENT_KEY
CTA->>Auth: Open SignIn modal (with onSuccess)
User->>Auth: Complete sign-in
Auth->>CTA: Invoke onSuccess(authenticatedUser)
CTA->>Storage: Read SUBSCRIBE_INTENT_KEY
CTA->>API: Call updateProfileAction to subscribe
API-->>CTA: Success / Error
CTA->>User: Show success or error message
else Authenticated
User->>CTA: Click Subscribe
CTA->>API: Call updateProfileAction (optimistic UI)
API-->>CTA: Success / Error
CTA->>User: Show success or error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/auth/signin.tsx (1)
75-83:⚠️ Potential issue | 🟠 MajorGuard onSuccess failures so post-login actions always run.
If
onSuccessthrows,handlePostLoginActionis skipped and the login flow can become inconsistent. Wrap the callback to preserve post-login handling.Proposed fix
- await onSuccess?.(state.user); - handlePostLoginAction(state.postLoginAction); + try { + await onSuccess?.(state.user); + } catch (err) { + console.error("Post-login onSuccess failed", err); + } finally { + handlePostLoginAction(state.postLoginAction); + }
🤖 Fix all issues with AI agents
In `@front_end/src/components/auth/signup.tsx`:
- Around line 123-126: The current sequence awaits onSuccess and then calls
handlePostLoginAction, but if onSuccess throws the post-login action is skipped;
change this by wrapping the onSuccess call in a try/catch (or try/catch/finally)
so that handlePostLoginAction(response?.postLoginAction) always runs—e.g., call
await onSuccess?.(response.user) inside try, handle or log any error in catch,
and invoke handlePostLoginAction in finally (or after the catch) to guarantee
post-login handling regardless of onSuccess failures; update the code around the
onSuccess and handlePostLoginAction calls in the signup component accordingly.
In
`@front_end/src/components/weekly_top_comments_feed/components/subscribe_top_comments_cta.tsx`:
- Around line 57-66: On success in the try block after calling
updateProfileAction (where setIsSuccess(true) and
safeLocalStorage.removeItem(SUBSCRIBE_INTENT_KEY) are executed), fire a success
toast or open a modal containing the full success copy (use the i18n key used
elsewhere, e.g. t("weeklyTopCommentsSubscribeSuccessFull")) and include an
actionable account settings link that navigates to the user's settings page;
ensure this new toast/modal complements the existing short inline state rather
than replacing error handling (leave the catch with
toast.error(t("weeklyTopCommentsSubscribeError")) intact) and reference
updateProfileAction, setIsSuccess, safeLocalStorage, SUBSCRIBE_INTENT_KEY, and
toast in your change so reviewers can locate the insertion point.
In
`@front_end/src/components/weekly_top_comments_feed/hooks/use_top_comments_cta_dismissed.ts`:
- Around line 10-30: When the storage key changes in useTopCommentsCtaDismissed,
reset loading state so the hook doesn't briefly expose the previous key's
dismissal value; update the hook to setReady(false) and setDismissed(true) when
the computed key changes (before the useEffect read), then let the existing
useEffect read safeLocalStorage for that key and setDismissed(...) and
setReady(true); reference the useTopCommentsCtaDismissed hook, the key variable,
the dismissed/ready state variables, and the existing useEffect/useCallback to
locate where to reset state on key change.
front_end/src/components/weekly_top_comments_feed/components/subscribe_top_comments_cta.tsx
Show resolved
Hide resolved
front_end/src/components/weekly_top_comments_feed/hooks/use_top_comments_cta_dismissed.ts
Show resolved
Hide resolved
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/components/auth/signup.tsx (1)
322-322:⚠️ Potential issue | 🟡 Minor
onSuccessis not propagated throughSocialButtons— social-login users won't auto-subscribe.If a logged-out user triggers the subscribe CTA, gets this signup modal, and completes registration via a social provider (OAuth redirect), the
onSuccesscallback is lost. The user would land back on the page without the subscription action firing, requiring them to click "Subscribe" again.This is likely acceptable given OAuth's redirect-based flow, but worth confirming whether you want to handle this (e.g., by persisting intent in
sessionStorageor via a query parameter on the redirect URL).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3504
Summary by CodeRabbit