-
-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/add cooldown varify email #4931
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces a new email verification cooldown feature across multiple language files and the Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant EmailService
User->>Component: Request Verification
alt No Cooldown Active
Component->>EmailService: Send Verification Email
Component-->User: Start 3-minute Cooldown
Component->Component: Update UI with Cooldown Timer
else Cooldown Active
Component-->User: Display Cooldown Message
Note over Component: Prevent Further Requests
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/InputUserEmailVerify.tsx (1)
239-246
: Improve error handling in verification handlerThe error handling could be more specific and production-ready.
Consider these improvements:
const verificationEmailHandler = async () => { if (isCooldown) { return; } setIsCooldown(true); setCooldownTime(180); try { const { data } = await client.mutate({ mutation: SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW, variables: { email: email, }, }); // ... rest of the code } catch (error) { - if (error instanceof Error) { + if (error instanceof ApolloError) { resetCoolDown(); - showToastError(error.message); + showToastError(error.message || 'Failed to send verification email'); } - console.log(error); + // Use proper error logging service in production + if (process.env.NODE_ENV === 'development') { + console.error('Verification email error:', error); + } } };Also applies to: 289-298
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lang/ca.json
(1 hunks)lang/en.json
(1 hunks)lang/es.json
(1 hunks)package.json
(1 hunks)src/components/InputUserEmailVerify.tsx
(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- lang/ca.json
🔇 Additional comments (5)
src/components/InputUserEmailVerify.tsx (2)
125-126
: LGTM! State management for cooldownThe state management implementation for the cooldown feature is clean and follows React conventions.
439-467
: LGTM! Clean UI implementation with i18n supportThe cooldown message UI implementation is well-structured:
- Uses FormattedMessage for internationalization
- Correctly formats time display
- Clean conditional rendering
package.json (1)
7-7
: Verify impact of removing sitemap generationThe sitemap generation step has been removed from the dev script. Let's verify this won't impact development workflow.
Run this script to check if sitemap.ts is used elsewhere:
✅ Verification successful
Removing sitemap generation from dev script is correct
The sitemap generation is implemented as a protected API endpoint (
/api/generate-sitemap
) designed to be triggered by a cron job in production. It's not part of the development workflow and doesn't need to run during local development.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if sitemap.ts is referenced in other scripts or build processes echo "Checking sitemap.ts references..." rg "sitemap\.ts" echo "Checking if sitemap generation is moved to other scripts..." rg "node.*sitemap" package.jsonLength of output: 258
Script:
#!/bin/bash # Search for any sitemap-related files echo "Searching for sitemap files..." fd sitemap # Check for sitemap-related code or configurations echo -e "\nSearching for sitemap references in code..." rg -i sitemap # Display all scripts from package.json echo -e "\nChecking all package.json scripts..." jq .scripts package.jsonLength of output: 3185
lang/en.json (1)
392-392
: LGTM! Well-structured translation for cooldown messageThe translation entry is properly formatted with placeholders for dynamic content (button and time).
lang/es.json (1)
390-390
: LGTM! The Spanish translation for email cooldown is accurate and consistent.The translation properly maintains HTML tags and clearly communicates the verification flow to Spanish-speaking users.
Let's verify consistency with other language files:
✅ Verification successful
Translations are consistent across all language files
The email cooldown message maintains consistent structure, HTML tags, and components across English, Spanish, and Catalan translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if email cooldown translation exists in all language files and follows same structure rg -A 1 'label\.email_cooldown' lang/Length of output: 931
const intervalId = setInterval(() => { | ||
setCooldownTime(prev => { | ||
if (prev <= 1) { | ||
resetCoolDown(); | ||
setDisableVerifyButton(false); | ||
return 0; | ||
} | ||
return prev - 1; | ||
}); | ||
}, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix potential memory leak in timer implementation
The current timer implementation using setInterval
could lead to memory leaks because:
- The interval is not cleaned up if the component unmounts
- Multiple intervals could be created on re-renders
Wrap the timer logic in a useEffect
hook to properly manage the interval:
+ useEffect(() => {
+ let intervalId: NodeJS.Timeout | undefined;
+
+ if (isCooldown) {
+ intervalId = setInterval(() => {
+ setCooldownTime(prev => {
+ if (prev <= 1) {
+ resetCoolDown();
+ setDisableVerifyButton(false);
+ return 0;
+ }
+ return prev - 1;
+ });
+ }, 1000);
+ }
+
+ return () => {
+ if (intervalId) {
+ clearInterval(intervalId);
+ }
+ };
+ }, [isCooldown]);
const verificationEmailHandler = async () => {
if (isCooldown) {
return;
}
setIsCooldown(true);
setCooldownTime(180);
- const intervalId = setInterval(() => {
- setCooldownTime(prev => {
- if (prev <= 1) {
- resetCoolDown();
- setDisableVerifyButton(false);
- return 0;
- }
- return prev - 1;
- });
- }, 1000);
try {
const { data } = await client.mutate({
mutation: SEND_USER_EMAIL_CONFIRMATION_CODE_FLOW,
variables: {
email: email,
},
});
// ... rest of the code
- clearInterval(intervalId);
} catch (error) {
if (error instanceof Error) {
- clearInterval(intervalId);
resetCoolDown();
showToastError(error.message);
}
console.log(error);
}
};
Also applies to: 294-294, 297-297
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Kechy
Adding cool-down counter to the verification process.
@maryjaf or @LatifatAbdullahi when you start testing this, please ping me I will walk you through how to setup network throttling to use slow network setup inside developers tools.
Summary by CodeRabbit
Release Notes
New Features
Localization
Development