-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix/Email verification bugs #4872
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request involve modifications to language translation files ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (5)
src/components/modals/EditUserModal.tsx (2)
195-197
: Consider improving type safety for the email input propsWhile the implementation works, consider:
- Adding proper TypeScript interface for the new prop
- Using a more descriptive name like
onVerificationStatusChange
instead ofverifiedSaveButton
+ interface IEmailInputProps extends InputProps { + onVerificationStatusChange?: (verified: boolean) => void; + } { - ...(field.type === 'email' && { - verifiedSaveButton: setVerified, - })} + ...(field.type === 'email' && { + onVerificationStatusChange: setVerified, + })}
206-206
: Consider adding user feedback for disabled save buttonWhile the button disabling logic is correct, users might not understand why the button is disabled. Consider adding a tooltip or helper text explaining that email verification is required to save changes.
<ButtonEditSave buttonType='secondary' label={formatMessage({ id: 'label.save', })} disabled={isLoading || !verified} + title={!verified ? formatMessage({ id: 'label.verify_email_to_save' }) : ''} type='submit' />
src/components/views/project/ProjectIndex.tsx (1)
139-139
: LGTM! Consider extracting the condition for better readability.The change correctly ensures that the email verification banner is only shown to admin users who haven't verified their email. This is a good improvement that aligns with fixing the email verification bugs.
Consider extracting the condition to a descriptive variable for better readability:
- {!isAdminEmailVerified && isAdmin && <VerifyEmailBanner />} + {const shouldShowVerificationBanner = !isAdminEmailVerified && isAdmin; + {shouldShowVerificationBanner && <VerifyEmailBanner />}src/components/InputUserEmailVerify.tsx (2)
296-296
: Use optional chaining for consistencyFor consistency with other callback invocations, use optional chaining.
- props.verifiedSaveButton && props.verifiedSaveButton(true); + props.verifiedSaveButton?.(true);🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
262-310
: Improve error handling in verification processThe error handling in the verification process could be enhanced:
- Remove console.log statements from catch blocks
- Consider adding specific error handling for different error types
- Add retry logic for network failures
} catch (error) { - if (error instanceof Error) { - showToastError(error.message); - } - console.log(error); + if (error instanceof Error) { + const errorMessage = error.message; + showToastError(errorMessage); + setValidationCodeStatus(EInputValidation.ERROR); + // Consider adding retry logic here + } else { + showToastError('An unexpected error occurred'); + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
lang/ca.json
(1 hunks)lang/en.json
(1 hunks)lang/es.json
(1 hunks)src/components/InputUserEmailVerify.tsx
(3 hunks)src/components/modals/EditUserModal.tsx
(3 hunks)src/components/views/project/ProjectIndex.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lang/ca.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/InputUserEmailVerify.tsx
[error] 215-215: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 218-218: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 221-221: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
src/components/modals/EditUserModal.tsx (2)
61-61
: LGTM: State management for email verification
The state initialization is well-implemented, using the user's current email verification status.
Line range hint 15-15
: Verify InputUserEmailVerify component implementation
Let's ensure the imported component properly implements the verification callback.
✅ Verification successful
InputUserEmailVerify component implementation is complete and functional
The component properly implements email verification functionality with:
- Email input validation and verification state management
- Verification code sending and validation
- Proper callback handling through
verifiedSaveButton
prop - Complete error handling and user feedback
- Email verification status persistence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of InputUserEmailVerify component
ast-grep --pattern 'const InputUserEmailVerify = $_' -f 'src/components/InputUserEmailVerify.tsx'
Length of output: 275
Script:
#!/bin/bash
# Search for InputUserEmailVerify component file
fd "InputUserEmailVerify" --type f
# Search for InputUserEmailVerify component implementation
ast-grep --pattern 'const InputUserEmailVerify = $$$'
Length of output: 25951
src/components/InputUserEmailVerify.tsx (1)
99-106
: LGTM: Type definition changes are well-structured
The addition of the verifiedSaveButton
callback to both variants of the InputType
union maintains backward compatibility while adding new functionality.
lang/en.json (1)
393-393
: LGTM! Good security improvement.
The change from "Verification Link" to "Verification Code" reflects a more secure approach to email verification.
Let's verify this change is consistent across the codebase:
✅ Verification successful
Change is consistent across language files and verification system
The verification shows that:
- The change from "link" to "code" is consistently implemented across all language files (English, Spanish, Catalan)
- All related email verification strings use "code" terminology
- No legacy "verification link" references remain in the JSON files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of "verification link" that might need updating
rg -i "verification link" --type-add 'web:*.{js,jsx,ts,tsx}' -t web
# Search for related email verification code handling
ast-grep --pattern 'email_get_resend'
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search for verification-related strings in all files
rg -i "verification (link|code)" --type json
# Also search for email verification related keys in language files
rg "email.*verification" --type json
# Search for the specific key in all JSON files
rg "label\.email_get_resend" --type json
Length of output: 875
lang/es.json (1)
393-393
: LGTM! Translation update improves clarity
The change from "enlace de verificación" to "código de verificación" more accurately reflects that a verification code (not a link) is being resent.
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.
Thanks Kechy
Summary by CodeRabbit
Release Notes
New Features
InputUserEmailVerify
component to improve email verification status handling.EditUserModal
to ensure users cannot save profile changes without verified email.VerifyEmailBanner
to target admin users specifically.Bug Fixes