feat: Added email verification system #1069
Conversation
|
@Nil2000 is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request enhance the email management functionality within the application. Key modifications include the introduction of new components and API endpoints for updating and verifying user email addresses. The Changes
Assessment against linked issues
Possibly related PRs
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 (
|
|
Hello @Nil2000, thanks for opening your first Pull Request. The maintainers will review this Pull Request and provide feedback as soon as possible. Keep up the great work! |
|
@NiallJoeMaher @John-Paul-Larkin Let me know what you think about this. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (10)
schema/token.ts (2)
3-9: TokenSchema structure is good, but consider enhancing security and data integrity.The overall structure of the TokenSchema is appropriate for a token system. However, consider the following enhancements:
- Add a minimum length requirement for the
tokenfield to ensure security.- Specify maximum lengths for
userIdandtokento prevent potential storage issues.- Add validation to ensure
expiresAtis always in the future.Here's a suggested improvement:
export const TokenSchema = z.object({ id: z.string().uuid(), userId: z.string().max(255), // Adjust max length as needed token: z.string().min(32).max(255), // Adjust min and max as per your security requirements createdAt: z.date().default(() => new Date()), expiresAt: z.date().refine(val => val > new Date(), { message: "Expiration date must be in the future" }), });
1-11: Consider adding email-specific fields to align with PR objectives.The token schema is well-structured for general token management, which is crucial for the email verification system mentioned in the PR objectives. However, to more closely align with the specific use case of email verification, consider adding the following fields:
type: To distinguish between different token uses (e.g., 'email_verification', 'password_reset', etc.).Here's a suggested enhancement:
export const TokenSchema = z.object({ id: z.string().uuid(), userId: z.string().max(255), token: z.string().min(32).max(255), type: z.enum(['email_verification', 'password_reset']), // Add other types as needed email: z.string().email().optional(), // Optional to allow for non-email related tokens createdAt: z.date().default(() => new Date()), expiresAt: z.date().refine(val => val > new Date(), { message: "Expiration date must be in the future" }), });This enhancement would make the token schema more specific to the email verification use case while maintaining flexibility for other potential token uses.
drizzle/0007_shocking_rachel_grey.sql (1)
1-9: LGTM! Consider adding an index on theuserIdcolumn.The
EmailVerificationTokentable structure is well-designed and aligns with the PR objectives. It includes all necessary fields for email verification, with appropriate constraints for data integrity and security.Consider adding an index on the
userIdcolumn to optimize queries that look up tokens for a specific user:CREATE INDEX IF NOT EXISTS "EmailVerificationToken_userId_idx" ON "EmailVerificationToken" ("userId");This can improve performance, especially if you expect to query tokens by user frequently.
app/verify_email/page.tsx (2)
11-30: Improve user experience with more appropriate redirectsRedirecting to "/not-found" might be confusing for users who are not authenticated or whose session has expired. Consider redirecting unauthenticated users to the login page and handling the case when a user is not found differently.
Apply this diff to update the redirects:
if (!session || !session.user) { - redirect("/not-found"); + redirect("/login"); } const existingUser = await db.query.user.findUnique({ // ... }); if (!existingUser) { - redirect("/not-found"); + redirect("/error?message=User%20not%20found"); }This change improves navigation flow and provides clearer feedback to the user.
3-3: Remove unnecessary import of ReactIn Next.js with the App Router (
appdirectory), importing React is not necessary, as it's automatically handled. Removing unused imports keeps the code clean.Apply this diff to remove the import:
-import React from "react";app/api/verify_email/route.ts (1)
17-17: Remove console.log statements from production codeUsing
console.login production code can expose sensitive information and clutter the logs. It's better to remove it or use a proper logging mechanism with configurable log levels.Apply this diff to remove the console log:
-console.log("tokenFromDb", tokenFromDb);app/verify_email/_client.tsx (1)
44-44: Remove console.log statements in production codeThe
console.logstatement is useful for debugging but should be removed to avoid cluttering the console and potential exposure of sensitive information in production environments.Apply this diff to remove the console.log statement:
- console.log("data", data);utils/emailToken.ts (1)
34-103: Update the email content to reflect email address updatesThe email template is currently tailored for new user registrations. Since this feature is for updating a user's email address, consider adjusting the content to reflect this context. This will provide clarity to the user about why they are receiving the email.
Apply the following changes to the email content:
<h1>Confirm Your Email Address</h1> <p>Hello,</p> - <p>Thank you for registering with us! To complete your registration, please verify your email address by clicking the button below:</p> + <p>You have requested to update your email address. Please confirm your new email by clicking the button below:</p> <a href="${verificationLink}" class="btn">Verify Email</a> <p>Please note that this link is valid for 1 hour only. If it expires, you will need to request a new one.</p> <p>If you did not request this change, please ignore this email.</p>app/(app)/settings/_client.tsx (2)
425-425: Ensure consistent loader sizes for visual consistency.The
Loadercomponents have different sizes (h-4 w-4andh-8 w-8). Unless there's a specific reason for this difference, consider using consistent sizes to maintain a uniform user interface.If you wish to standardize the loader sizes, apply this diff (choose the appropriate size):
- <Loader className="text-primary h-4 w-4 animate-spin" /> + <Loader className="text-primary h-6 w-6 animate-spin" />And:
- <Loader className="text-primary h-8 w-8 animate-spin" /> + <Loader className="text-primary h-6 w-6 animate-spin" />Also applies to: 441-441
401-404: Consider adding a 'readOnly' attribute instead of 'disabled' for the current email input.Using
readOnlyallows the input to be focusable, letting users copy the current email if needed, while still preventing modifications.Apply this diff to update the input:
{...register("email")} - disabled + readOnly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
drizzle/meta/0007_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (11)
- app/(app)/settings/_client.tsx (6 hunks)
- app/(app)/settings/page.tsx (4 hunks)
- app/api/update_email/route.ts (1 hunks)
- app/api/verify_email/route.ts (1 hunks)
- app/verify_email/_client.tsx (1 hunks)
- app/verify_email/page.tsx (1 hunks)
- drizzle/0007_shocking_rachel_grey.sql (1 hunks)
- schema/profile.ts (1 hunks)
- schema/token.ts (1 hunks)
- server/db/schema.ts (1 hunks)
- utils/emailToken.ts (1 hunks)
🧰 Additional context used
🪛 Biome
app/(app)/settings/_client.tsx
[error] 421-421: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
[error] 437-437: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (22)
schema/token.ts (2)
1-1: LGTM: Zod import is correct.The import statement for Zod is correct and follows common practices.
11-11: LGTM: TokenInput type is well-defined.The
TokenInputtype is correctly derived from theTokenSchemausing Zod'sTypeOfutility. This is a good practice as it ensures type consistency between the schema validation and TypeScript types. Exporting this type will make it easy to use in other parts of the application where token data needs to be handled.app/api/update_email/route.ts (3)
1-5: LGTM: Appropriate utility functions imported.The imported utility functions from "@/utils/emailToken" are relevant to the email update process. The use of a path alias (@) is a good practice for maintaining clean import statements.
6-6: LGTM: Correct Next.js server imports.The import of NextRequest and NextResponse from "next/server" is correct for handling server-side API routes in Next.js. This ensures proper typing for the request and response objects.
8-25: Overall implementation aligns well with PR objectivesThe POST function successfully implements the core functionality described in the PR objectives. It provides a mechanism for users to update their email addresses, which was the main goal of the linked issue #1049. The implementation includes:
- Receiving a new email address from the user
- Generating a verification token
- Storing the token and new email temporarily
- Sending a verification email to the new address
This approach ensures that the old email remains associated with the account until the verification process is completed, as specified in the objectives.
To further improve the implementation and fully meet the objectives:
- Ensure that the verification link sent in the email correctly handles the token verification process.
- Implement the final step of updating the user's email in the database after successful verification.
- Consider adding a mechanism to handle token expiration and cleanup of unverified email changes.
These additions will complete the email update process and provide a robust solution for users to change their email addresses.
schema/profile.ts (2)
Line range hint
1-38: Overall schema structure remains consistentThe addition of the
saveSettingsSchemais the only change in this file. The overall structure and other schemas remain unchanged, which is good for maintaining consistency. However, ensure that any components or functions usingsaveSettingsSchemaare updated to handle the newTo ensure all usages of
saveSettingsSchemaare updated, run the following script:#!/bin/bash # Search for usages of saveSettingsSchema rg --type typescript 'saveSettingsSchema'Review the results to identify any areas that might need updates due to the new
21-21: 🛠️ Refactor suggestionRefine email field definition for better alignment with PR objectives
The addition of the
The current definition allows for an empty string, which might not be ideal for email updates. Consider removing the
or(z.literal(""))part to ensure a valid email is always provided.To better align with the PR objectives, consider separating the current email and the new email to be verified. This could be achieved by using two fields:
currentEmail: The user's verified email addressnewEmail: The email address pending verificationThe schema doesn't enforce the temporary storage of the new email until verification. This logic might need to be handled in the application code.
Here's a suggested refinement:
email: z.object({ current: z.string().email(), new: z.string().email().optional(), }),This structure clearly separates the current and new email addresses, aligning better with the PR objectives and the email verification process.
To ensure this change doesn't break existing functionality, let's check how the
Please review the results of this search to ensure the proposed changes are compatible with existing code.
app/(app)/settings/page.tsx (4)
Line range hint
26-35: LGTM: Email field added to user queryThe addition of the 'email' field to the user query is appropriate and aligns with the PR objective of implementing an email verification system. This change ensures that the user's email is available for the new email update functionality.
Line range hint
55-64: LGTM: Email field added to user update returnThe inclusion of the 'email' field in the returning clause of the user update operation is consistent with the earlier change and ensures that the email data is available for the Content component. This modification supports the new email verification functionality.
Line range hint
71-83: Verify newsletter subscription with new email systemWhile the newsletter subscription check remains unchanged, it's crucial to ensure that the new email verification system doesn't negatively impact this functionality.
Please verify that:
- The newsletter subscription status is correctly maintained when a user updates their email.
- The process of subscribing/unsubscribing to the newsletter works correctly with both verified and unverified email addresses.
Consider adding tests to cover these scenarios if they don't already exist.
To assist in verifying this functionality, you can run the following script:
#!/bin/bash # Description: Check for newsletter-related functionality in other files # Test: Search for newsletter-related operations rg --type typescript -i 'newsletter|subscribe' -g '!app/(app)/settings/page.tsx'
Line range hint
67-69: Consider revising the email presence checkWhile checking for the presence of an email is generally good practice, the current implementation might conflict with the PR objective of allowing users to add an email if they don't have one. Additionally, its placement after the new user creation logic could prevent new users from setting their email.
Consider the following suggestions:
- Move this check before the new user creation logic.
- Instead of returning a "not found" response, redirect the user to an "add email" page or section if the email is missing.
This approach would better align with the PR objective of implementing an email verification system and allowing users to update their email addresses.
To verify the impact of this change, please run the following script:
drizzle/0007_shocking_rachel_grey.sql (5)
1-53: Overall, the migration script looks good and aligns well with the PR objectives.This migration script successfully implements the necessary changes for the email verification system, including:
- Creating the
EmailVerificationTokentable- Updating
updatedAtcolumn defaults- Adding appropriate constraints and indexes
The script maintains data integrity and should provide good query performance. However, please consider the following recommendations:
- Add an index on the
userIdcolumn in theEmailVerificationTokentable- Verify the impact of dropping and recreating indexes, especially on large tables
- Check for any unexpected effects on existing data after altering the
updatedAtcolumns- Confirm that the new constraints are correctly applied and don't conflict with existing data
- Monitor index usage after deployment to ensure optimal performance
By addressing these points, you'll ensure a smooth deployment and optimal performance of the new email verification system.
26-31: LGTM! Verify the impact on existing data.Setting the default value of
updatedAtcolumns toCURRENT_TIMESTAMPis a good practice. It ensures that the last-modified timestamp is automatically updated when a row changes.To ensure this change doesn't affect existing data unexpectedly, run the following script:
#!/bin/bash # Description: Check for null or unexpected values in updatedAt columns after the migration. # Test: Check for null or unexpected updatedAt values psql -c "SELECT table_name, COUNT(*) as null_or_unexpected_count FROM ( SELECT 'BannedUsers' as table_name, updatedAt FROM \"BannedUsers\" UNION ALL SELECT 'Comment', updatedAt FROM \"Comment\" UNION ALL SELECT 'Flagged', updatedAt FROM \"Flagged\" UNION ALL SELECT 'Notification', updatedAt FROM \"Notification\" UNION ALL SELECT 'Post', updatedAt FROM \"Post\" UNION ALL SELECT 'user', updatedAt FROM \"user\" ) as combined WHERE updatedAt IS NULL OR updatedAt < '2000-01-01' GROUP BY table_name;"This will help identify any rows where
updatedAtmight have unexpected values after the migration.
39-53: LGTM! Consider verifying index usage after deployment.The recreation of indexes, including unique constraints and the additional non-unique index on the user table, is well-structured and should maintain data integrity and query performance.
After deploying these changes, it's important to verify that the indexes are being used effectively. Run the following script periodically in your production environment to monitor index usage:
#!/bin/bash # Description: Check index usage statistics for the newly created indexes. # Test: Retrieve index usage statistics psql -c "SELECT schemaname, relname, indexrelname, idx_scan, idx_tup_read, idx_tup_fetch FROM pg_stat_user_indexes WHERE indexrelname IN ( 'BannedUsers_userId_key', 'Bookmark_userId_postId_key', 'Community_id_key', 'Community_slug_key', 'Event_id_key', 'Event_slug_key', 'Like_userId_commentId_key', 'Like_userId_postId_key', 'Post_id_key', 'Post_slug_key', 'PostTag_tagId_postId_key', 'Tag_title_key', 'User_username_key', 'User_email_key', 'User_username_id_idx' ) ORDER BY idx_scan DESC;"This will help you identify any unused or underutilized indexes, allowing for further optimization if needed.
32-37: LGTM! Verify the newly added constraints.The addition of primary and foreign key constraints enhances data integrity and properly enforces relationships between tables. The composite primary key on the
accounttable is appropriate for handling multiple authentication providers.To ensure the constraints are correctly applied and don't conflict with existing data, run the following script:
#!/bin/bash # Description: Verify the newly added constraints on account and EmailVerificationToken tables. # Test 1: Check if there are any rows in the account table that violate the new primary key constraint psql -c "SELECT provider, providerAccountId, COUNT(*) FROM account GROUP BY provider, providerAccountId HAVING COUNT(*) > 1;" # Test 2: Check if there are any rows in the EmailVerificationToken table that violate the new foreign key constraint psql -c "SELECT COUNT(*) FROM \"EmailVerificationToken\" evt LEFT JOIN \"user\" u ON evt.\"userId\" = u.id WHERE u.id IS NULL;" # Test 3: Verify the constraints are properly created psql -c "\d+ account" psql -c "\d+ \"EmailVerificationToken\""This script will help identify any potential issues with the new constraints and confirm they are correctly applied.
11-25: Verify the impact of dropping and recreating indexes.The script drops several indexes before recreating them later. While this approach works, it may lead to temporary performance degradation during the migration process, especially on large tables.
Consider the following:
- Verify that this migration won't significantly impact production performance.
- For future migrations, consider using
CREATE OR REPLACE INDEXinstead of dropping and recreating indexes, where possible.To assess the potential impact, run the following script to check the size of affected tables:
This will help estimate the time and resources required for index recreation.
server/db/schema.ts (1)
116-125: Summary: NewemailVerificationTokentable aligns well with PR objectivesThe addition of the
emailVerificationTokentable is a solid step towards implementing the email verification system as outlined in the PR objectives. This table provides a structure to securely manage temporary tokens for email verification, allowing users to update their email addresses safely.Key points:
- The table design supports the temporary storage of new email addresses until verified.
- It maintains a link to the user account via the
userIdfield.- The inclusion of
createdAtandexpiresAtfields allows for proper token management and expiration.These changes lay a good foundation for the email verification feature. Ensure that the corresponding application logic is implemented to utilize this table effectively in the email update and verification process.
app/verify_email/page.tsx (2)
29-29: EnsureContentcomponent handles user data securelyWhen passing
userIdto theContentcomponent, ensure that the component doesn't expose sensitive user information or render data that should remain confidential.Review the
Contentcomponent to confirm it adheres to security best practices.
14-16:⚠️ Potential issueEnsure type safety by guarding
session.userwithout non-null assertionYou've already checked that
sessionandsession.userexist, so the non-null assertionsession.user!.idisn't necessary. Removing it improves type safety and avoids potential runtime errors.Apply this diff to remove the non-null assertion:
if (!session || !session.user) { redirect("/not-found"); } - const existingUser = await db.query.user.findFirst({ + const existingUser = await db.query.user.findFirst({ // ... - where: (users, { eq }) => eq(users.id, session.user!.id), + where: (users, { eq }) => eq(users.id, session.user.id), });Likely invalid or redundant comment.
app/verify_email/_client.tsx (1)
62-104: LGTM!The rendering logic is well-structured, and the component appropriately handles different verification statuses.
utils/emailToken.ts (2)
31-31: Ensure theNEXT_PUBLIC_BACKEND_URLis correctly configuredThe
NEXT_PUBLIC_BACKEND_URLenvironment variable is used to construct the verification link. Please verify that this variable is correctly set to the front-end application URL that will handle the email verification process. Misconfiguration could lead to incorrect or insecure links being sent to users.
31-31: Verify user identification during email verificationThe verification link only includes the token as a query parameter, but the
getTokenFromDbfunction requires both the token anduserId. Ensure that theuserIdcan be securely obtained when processing the verification request, such as from the authenticated session or by including it in the verification link in a secure manner (e.g., encoding or hashing).Consider whether it is appropriate to include the user ID in the verification link or adjust the
getTokenFromDbfunction to retrieve the token using only the token value. Including sensitive information likeuserIdin URLs should be handled carefully to prevent security risks.
app/api/update_email/route.ts
Outdated
| export async function POST(req: NextRequest, res: NextResponse) { | ||
| const { userId, newEmail } = await req.json(); | ||
|
|
||
| if (!newEmail || !userId) | ||
| return NextResponse.json({ message: "Invalid request" }, { status: 400 }); | ||
|
|
||
| const token = generateEmailToken(); | ||
| const expiresAt = new Date(Date.now() + 1000 * 60 * 60); //1 hour | ||
|
|
||
| await storeTokenInDb(userId, token, expiresAt, newEmail); | ||
|
|
||
| await sendVerificationEmail(newEmail, token); | ||
|
|
||
| return NextResponse.json( | ||
| { message: "Verification email sent" }, | ||
| { status: 200 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Enhance error handling and input validation
While the overall structure of the POST function is good, there are several areas where it can be improved:
- Error handling: Add try-catch blocks around database operations and email sending to handle potential errors gracefully.
- Input validation:
- Check if the new email is different from the current one to avoid unnecessary processing.
- Validate the email format before proceeding.
- Verify that the user exists in the database before updating their email.
- Response consistency: Ensure all responses follow the same structure for consistency.
Here's a suggested improvement:
import { isValidEmail, getUserById } from "@/utils/validators"; // Assume these exist
export async function POST(req: NextRequest, res: NextResponse) {
try {
const { userId, newEmail } = await req.json();
if (!newEmail || !userId) {
return NextResponse.json({ error: "Invalid request" }, { status: 400 });
}
if (!isValidEmail(newEmail)) {
return NextResponse.json({ error: "Invalid email format" }, { status: 400 });
}
const user = await getUserById(userId);
if (!user) {
return NextResponse.json({ error: "User not found" }, { status: 404 });
}
if (user.email === newEmail) {
return NextResponse.json({ message: "New email is the same as the current email" }, { status: 200 });
}
const token = generateEmailToken();
const expiresAt = new Date(Date.now() + 1000 * 60 * 60); // 1 hour
await storeTokenInDb(userId, token, expiresAt, newEmail);
await sendVerificationEmail(newEmail, token);
return NextResponse.json({ message: "Verification email sent" }, { status: 200 });
} catch (error) {
console.error("Error updating email:", error);
return NextResponse.json({ error: "Internal server error" }, { status: 500 });
}
}This implementation adds error handling, more comprehensive input validation, and consistent response structures.
There was a problem hiding this comment.
Agreed! A try catch is useful here.
There was a problem hiding this comment.
@NiallJoeMaher, glad to hear that you're implementing the try-catch block for improved error handling!
(^_^/)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
app/api/update_email/route.ts
Outdated
| const token = generateEmailToken(); | ||
| const expiresAt = new Date(Date.now() + 1000 * 60 * 60); //1 hour |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making token expiration configurable
The token expiration is currently hardcoded to 1 hour. Consider making this configurable to allow for flexibility in different environments or use cases.
Here's a suggested improvement:
import { TOKEN_EXPIRATION_TIME } from "@/config/constants"; // Assume this exists
// ...
const expiresAt = new Date(Date.now() + TOKEN_EXPIRATION_TIME);This change allows you to easily adjust the token expiration time across your application by modifying a single constant.
| export const emailVerificationToken = pgTable("EmailVerificationToken", { | ||
| id: serial("id").primaryKey(), | ||
| token: text("token").unique().notNull(), | ||
| createdAt: timestamp("createdAt").defaultNow().notNull(), | ||
| expiresAt: timestamp("expiresAt").notNull(), | ||
| email: text("email").notNull(), | ||
| userId: text("userId") | ||
| .notNull() | ||
| .references(() => user.id), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify user table setup and add relation to emailVerificationToken
The new emailVerificationToken table correctly references the user table. To complete this relationship:
- Verify that the
usertable is properly set up to handle this new relationship. - Add a relation in the
userRelationsdefinition to include theemailVerificationToken.
Add the following relation to the userRelations definition:
export const userRelations = relations(user, ({ one, many }) => ({
// ... existing relations ...
+ emailVerificationTokens: many(emailVerificationToken),
}));This will establish a one-to-many relationship between users and email verification tokens, allowing for easier querying and management of these tokens per user.
Committable suggestion was skipped due to low confidence.
🛠️ Refactor suggestion
Consider enhancing the emailVerificationToken table definition
The new table structure aligns well with the PR objectives. However, consider the following improvements:
- Add a length constraint to the
tokenfield for consistency and potential performance benefits. - Create an index on the
userIdand/oremailfields to improve query performance, especially if you'll be querying by these fields frequently. - Add a constraint or default value to ensure the
expiresAtfield is always set in the future.
Here's a suggested modification:
export const emailVerificationToken = pgTable("EmailVerificationToken", {
id: serial("id").primaryKey(),
- token: text("token").unique().notNull(),
+ token: varchar("token", { length: 64 }).unique().notNull(),
createdAt: timestamp("createdAt").defaultNow().notNull(),
- expiresAt: timestamp("expiresAt").notNull(),
+ expiresAt: timestamp("expiresAt").notNull().$check(sql`"expiresAt" > CURRENT_TIMESTAMP`),
email: text("email").notNull(),
userId: text("userId")
.notNull()
.references(() => user.id),
+ }, (table) => ({
+ userIdIdx: index("EmailVerificationToken_userId_idx").on(table.userId),
+ emailIdx: index("EmailVerificationToken_email_idx").on(table.email),
});These changes will:
- Limit the token length to 64 characters (adjust as needed).
- Add a check constraint to ensure
expiresAtis always in the future. - Create indexes on
userIdandemailfor better query performance.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const emailVerificationToken = pgTable("EmailVerificationToken", { | |
| id: serial("id").primaryKey(), | |
| token: text("token").unique().notNull(), | |
| createdAt: timestamp("createdAt").defaultNow().notNull(), | |
| expiresAt: timestamp("expiresAt").notNull(), | |
| email: text("email").notNull(), | |
| userId: text("userId") | |
| .notNull() | |
| .references(() => user.id), | |
| }); | |
| export const emailVerificationToken = pgTable("EmailVerificationToken", { | |
| id: serial("id").primaryKey(), | |
| token: varchar("token", { length: 64 }).unique().notNull(), | |
| createdAt: timestamp("createdAt").defaultNow().notNull(), | |
| expiresAt: timestamp("expiresAt").notNull().$check(sql`"expiresAt" > CURRENT_TIMESTAMP`), | |
| email: text("email").notNull(), | |
| userId: text("userId") | |
| .notNull() | |
| .references(() => user.id), | |
| }, (table) => ({ | |
| userIdIdx: index("EmailVerificationToken_userId_idx").on(table.userId), | |
| emailIdx: index("EmailVerificationToken_email_idx").on(table.email), | |
| }); |
app/verify_email/page.tsx
Outdated
|
|
||
| export default async function Page() { | ||
| const session = await getServerAuthSession(); | ||
| console.log("session", session); |
There was a problem hiding this comment.
Remove console.log to prevent logging sensitive information
The console.log("session", session); statement logs the entire session object, which may contain sensitive user information. Logging such data can lead to security vulnerabilities if logs are exposed.
Apply this diff to remove the debug statement:
const session = await getServerAuthSession();
- console.log("session", session);
if (!session || !session.user) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("session", session); | |
| const session = await getServerAuthSession(); | |
| if (!session || !session.user) { |
There was a problem hiding this comment.
@NiallJoeMaher, thanks for addressing this!
((^_^)/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const existingUser = await db.query.user.findFirst({ | ||
| columns: { | ||
| id: true, | ||
| }, | ||
| where: (users, { eq }) => eq(users.id, session.user!.id), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify user retrieval by using findUnique
Since you're querying by a unique identifier (id), you can use findUnique instead of findFirst for clarity and potential performance benefits.
Apply this diff to use findUnique:
- const existingUser = await db.query.user.findFirst({
- columns: {
- id: true,
- },
- where: (users, { eq }) => eq(users.id, session.user!.id),
- });
+ const existingUser = await db.query.user.findUnique({
+ where: { id: session.user.id },
+ select: { id: true },
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const existingUser = await db.query.user.findFirst({ | |
| columns: { | |
| id: true, | |
| }, | |
| where: (users, { eq }) => eq(users.id, session.user!.id), | |
| }); | |
| const existingUser = await db.query.user.findUnique({ | |
| where: { id: session.user.id }, | |
| select: { id: true }, | |
| }); |
app/(app)/settings/_client.tsx
Outdated
| import { zodResolver } from "@hookform/resolvers/zod"; | ||
| import type { SubmitHandler } from "react-hook-form"; | ||
| import { useForm } from "react-hook-form"; | ||
| import { set, useForm } from "react-hook-form"; |
There was a problem hiding this comment.
Remove unnecessary import 'set' from 'react-hook-form'.
The set function is not used anywhere in the code. Removing unused imports helps keep the code clean and maintainable.
Apply this diff to fix the issue:
-import { set, useForm } from "react-hook-form";
+import { useForm } from "react-hook-form";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { set, useForm } from "react-hook-form"; | |
| import { useForm } from "react-hook-form"; |
app/(app)/settings/_client.tsx
Outdated
| {!sendForVerification ? ( | ||
| <Button | ||
| className="w-[200px]" | ||
| disabled={newEmail || loading ? false : true} |
There was a problem hiding this comment.
Simplify the 'disabled' prop expression in the 'Send verification link' button.
The use of a ternary operator with boolean literals is unnecessary. You can simplify the expression by using the logical NOT operator for clarity.
Apply this diff to fix the issue:
- disabled={newEmail || loading ? false : true}
+ disabled={!(newEmail || loading)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disabled={newEmail || loading ? false : true} | |
| disabled={!(newEmail || loading)} |
🧰 Tools
🪛 Biome
[error] 421-421: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
app/(app)/settings/_client.tsx
Outdated
| </h2> | ||
| <Button | ||
| className="w-[200px]" | ||
| disabled={newEmail || loading ? false : true} |
There was a problem hiding this comment.
Simplify the 'disabled' prop expression in the 'Resend verification link' button.
Similarly, simplify the ternary expression to make the code more readable.
Apply this diff to fix the issue:
- disabled={newEmail || loading ? false : true}
+ disabled={!(newEmail || loading)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disabled={newEmail || loading ? false : true} | |
| disabled={!(newEmail || loading)} |
🧰 Tools
🪛 Biome
[error] 437-437: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
| const handleNewEmailUpdate = async () => { | ||
| setLoading(true); | ||
| try { | ||
| await fetch("/api/update_email", { | ||
| method: "POST", | ||
| body: JSON.stringify({ userId: profile.id, newEmail }), | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| }) | ||
| .then((res) => res.json()) | ||
| .then((res) => { | ||
| console.log(res); | ||
| if (res.status === 400) { | ||
| setLoading(false); | ||
| toast.error( | ||
| "Something went wrong while sending the verification link", | ||
| ); | ||
| } else { | ||
| toast.success( | ||
| "Verification link has been sent to your new email address.", | ||
| ); | ||
| setLoading(false); | ||
| setSendForVerification(true); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| setLoading(false); | ||
| toast.error( | ||
| "Something went wrong while sending the verification link.", | ||
| ); | ||
| }); | ||
| } catch (error) { | ||
| setLoading(false); | ||
| toast.error("Something went wrong while sending the verification link."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix incorrect response handling in 'handleNewEmailUpdate' function.
The response handling in your fetch call is incorrect. After calling res.json(), you lose access to the res.status property, as res now contains the parsed JSON body. Additionally, mixing async/await with .then() and .catch() can lead to confusion and is not considered best practice.
Here's how you can refactor the function:
- Use
async/awaitconsistently for better readability. - Access the response status code before parsing the JSON.
- Improve error handling to accurately reflect the response.
Apply this diff to fix the issue:
const handleNewEmailUpdate = async () => {
setLoading(true);
try {
- await fetch("/api/update_email", {
+ const res = await fetch("/api/update_email", {
method: "POST",
body: JSON.stringify({ userId: profile.id, newEmail }),
headers: {
"Content-Type": "application/json",
},
- })
- .then((res) => res.json())
- .then((res) => {
- console.log(res);
- if (res.status === 400) {
- setLoading(false);
- toast.error(
- "Something went wrong while sending the verification link",
- );
- } else {
- toast.success(
- "Verification link has been sent to your new email address.",
- );
- setLoading(false);
- setSendForVerification(true);
- }
- })
- .catch((error) => {
- setLoading(false);
- toast.error(
- "Something went wrong while sending the verification link.",
- );
- });
+ });
+ if (res.status === 400) {
+ toast.error(
+ "Something went wrong while sending the verification link"
+ );
+ } else if (res.ok) {
+ toast.success(
+ "Verification link has been sent to your new email address."
+ );
+ setSendForVerification(true);
+ } else {
+ toast.error(
+ "Unexpected error occurred while sending the verification link."
+ );
+ }
+ } catch (error) {
+ toast.error(
+ "Something went wrong while sending the verification link."
+ );
} finally {
setLoading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleNewEmailUpdate = async () => { | |
| setLoading(true); | |
| try { | |
| await fetch("/api/update_email", { | |
| method: "POST", | |
| body: JSON.stringify({ userId: profile.id, newEmail }), | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| }) | |
| .then((res) => res.json()) | |
| .then((res) => { | |
| console.log(res); | |
| if (res.status === 400) { | |
| setLoading(false); | |
| toast.error( | |
| "Something went wrong while sending the verification link", | |
| ); | |
| } else { | |
| toast.success( | |
| "Verification link has been sent to your new email address.", | |
| ); | |
| setLoading(false); | |
| setSendForVerification(true); | |
| } | |
| }) | |
| .catch((error) => { | |
| setLoading(false); | |
| toast.error( | |
| "Something went wrong while sending the verification link.", | |
| ); | |
| }); | |
| } catch (error) { | |
| setLoading(false); | |
| toast.error("Something went wrong while sending the verification link."); | |
| } | |
| }; | |
| const handleNewEmailUpdate = async () => { | |
| setLoading(true); | |
| try { | |
| const res = await fetch("/api/update_email", { | |
| method: "POST", | |
| body: JSON.stringify({ userId: profile.id, newEmail }), | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| }); | |
| if (res.status === 400) { | |
| toast.error( | |
| "Something went wrong while sending the verification link" | |
| ); | |
| } else if (res.ok) { | |
| toast.success( | |
| "Verification link has been sent to your new email address." | |
| ); | |
| setSendForVerification(true); | |
| } else { | |
| toast.error( | |
| "Unexpected error occurred while sending the verification link." | |
| ); | |
| } | |
| } catch (error) { | |
| toast.error( | |
| "Something went wrong while sending the verification link." | |
| ); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
NiallJoeMaher
left a comment
NiallJoeMaher
left a comment
There was a problem hiding this comment.
Wowza! Lots of code on this one so great work so far. I added a good few comments but nothing mental. You have the guts of everything here. Well done!
app/api/update_email/route.ts
Outdated
| import { NextRequest, NextResponse } from "next/server"; | ||
|
|
||
| export async function POST(req: NextRequest, res: NextResponse) { | ||
| const { userId, newEmail } = await req.json(); |
There was a problem hiding this comment.
This could let someone change someone elses email. You would need to grab the user/userId from the current users session. Also for mutating requests like this we use trpc. You could add this here: https://github.com/codu-code/codu/blob/develop/server/api/router/profile.ts
And you'll see the pattern we have for cleaning data with schemas and getting the user from ctx.session.user
There was a problem hiding this comment.
Yes followed this way now.
app/api/update_email/route.ts
Outdated
| export async function POST(req: NextRequest, res: NextResponse) { | ||
| const { userId, newEmail } = await req.json(); | ||
|
|
||
| if (!newEmail || !userId) | ||
| return NextResponse.json({ message: "Invalid request" }, { status: 400 }); | ||
|
|
||
| const token = generateEmailToken(); | ||
| const expiresAt = new Date(Date.now() + 1000 * 60 * 60); //1 hour | ||
|
|
||
| await storeTokenInDb(userId, token, expiresAt, newEmail); | ||
|
|
||
| await sendVerificationEmail(newEmail, token); | ||
|
|
||
| return NextResponse.json( | ||
| { message: "Verification email sent" }, | ||
| { status: 200 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Agreed! A try catch is useful here.
app/api/verify_email/route.ts
Outdated
| @@ -0,0 +1,33 @@ | |||
| import { | |||
There was a problem hiding this comment.
On the verify email route we could probably create a GET call which takes the token ID and redirects the user to a success or an error page depending on what they do (that's the normal pattern)
So have a route that might be /verify-email?token=eyJhbGciOiJIUzI1NiIsInR
app/api/verify_email/route.ts
Outdated
| export async function POST(req: NextRequest, res: NextResponse) { | ||
| const token = req.nextUrl.searchParams.get("token"); | ||
| const { userId } = await req.json(); | ||
|
|
||
| if (!token) | ||
| return NextResponse.json({ message: "Invalid request" }, { status: 400 }); | ||
|
|
||
| const tokenFromDb = await getTokenFromDb(token, userId); | ||
|
|
||
| console.log("tokenFromDb", tokenFromDb); | ||
| if (!tokenFromDb.length) | ||
| return NextResponse.json({ message: "Invalid token" }, { status: 400 }); | ||
|
|
||
| const { expiresAt, email } = tokenFromDb[0]; | ||
| if (expiresAt < new Date()) | ||
| return NextResponse.json({ message: "Token expired" }, { status: 400 }); | ||
|
|
||
| await updateEmail(userId, email); | ||
|
|
||
| await deleteTokenFromDb(token); | ||
|
|
||
| return NextResponse.json( | ||
| { message: "Email successfully verified" }, | ||
| { status: 200 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Good suggestions here!
app/verify_email/_client.tsx
Outdated
| import { useRouter, useSearchParams } from "next/navigation"; | ||
| import React, { useEffect, useState } from "react"; | ||
|
|
||
| function Content({ userId }: { userId: string }) { |
There was a problem hiding this comment.
Good comment again. Nitpick, but the url would look better if it was verify-email.
app/verify_email/_client.tsx
Outdated
| const res = await fetch(`/api/verify_email?token=${token}`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ userId }), | ||
| }); |
There was a problem hiding this comment.
The user should be able to click a link to a GET route which will auto-magically validate the token (instead of having to post a request). This ensures that the user clicked their email always.
app/verify_email/page.tsx
Outdated
|
|
||
| export default async function Page() { | ||
| const session = await getServerAuthSession(); | ||
| console.log("session", session); |
| const existingUser = await db.query.user.findFirst({ | ||
| columns: { | ||
| id: true, | ||
| }, | ||
| where: (users, { eq }) => eq(users.id, session.user!.id), | ||
| }); | ||
|
|
| "userId" text NOT NULL, | ||
| CONSTRAINT "EmailVerificationToken_token_unique" UNIQUE("token") | ||
| ); | ||
| --> statement-breakpoint |
There was a problem hiding this comment.
This migration seems to drop a lot of indexes that we might need. Also the file name is different than the ones we usually have which are descriptive.
There was a problem hiding this comment.
Yeah saw that. But it's also creating those later.
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (9)
schema/token.ts (1)
1-15: Great job: File aligns well with PR objectives and follows best practices.This file successfully introduces the necessary schemas and types for the email verification system:
- The
emailTokenReqSchemavalidates the new email input.- The
TokenSchemadefines the structure for storing verification tokens.- The
TokenInputtype alias ensures type safety in TypeScript.These components directly support the PR objective of implementing an email verification system, allowing users to update their email addresses securely. The use of Zod for schema validation is a great choice, providing both runtime checks and TypeScript type inference.
One minor suggestion to consider:
Consider adding a brief comment at the top of the file explaining its purpose in the context of the email verification system. This would enhance code readability and maintainability.
drizzle/0007_added-emailtokenverification-schema.sql (3)
1-9: LGTM! Consider adding an index on the userId column.The structure of the EmailVerificationToken table looks good and includes all necessary fields for managing email verification tokens. The unique constraint on the token field is a good security measure.
Consider adding an index on the userId column to improve query performance when looking up tokens for a specific user:
CREATE INDEX IF NOT EXISTS "EmailVerificationToken_userId_idx" ON "EmailVerificationToken" ("userId");
33-37: Consider CASCADE on delete for the foreign key constraint.The addition of the foreign key constraint is good for maintaining referential integrity. The use of a DO block with exception handling is a nice touch to make the migration more robust.
However, consider changing the ON DELETE behavior to CASCADE. This would automatically delete the verification tokens when a user is deleted, which is likely the desired behavior:
ALTER TABLE "EmailVerificationToken" ADD CONSTRAINT "EmailVerificationToken_userId_user_id_fk" FOREIGN KEY ("userId") REFERENCES "public"."user"("id") ON DELETE CASCADE ON UPDATE NO ACTION;This change would help maintain data consistency and avoid orphaned records in the EmailVerificationToken table.
39-53: LGTM! Consider using consistent IF NOT EXISTS clauses.The recreation of the previously dropped indexes is good. This addresses the earlier concern about potential performance impacts from removing these indexes.
For consistency, consider using the IF NOT EXISTS clause for all index creations, not just some of them. This makes the migration more idempotent and robust. For example:
CREATE UNIQUE INDEX IF NOT EXISTS "BannedUsers_userId_key" ON "BannedUsers" USING btree ("userId"); CREATE UNIQUE INDEX IF NOT EXISTS "Bookmark_userId_postId_key" ON "Bookmark" USING btree ("postId","userId"); -- Apply this pattern to all index creationsThis change would make the migration more consistent and easier to maintain.
utils/emailToken.ts (1)
34-100: LGTM: Well-structured and styled email template.The email template is well-designed, responsive, and follows good practices for email HTML. It includes all necessary information and a clear call-to-action button.
Minor suggestion: Consider adding alt text to the verification button for improved accessibility:
- <a href="${verificationLink}" class="btn">Verify Email</a> + <a href="${verificationLink}" class="btn" role="button" aria-label="Verify your email address">Verify Email</a>This change will improve the experience for users relying on screen readers.
app/api/verify-email/route.ts (1)
38-43: Log errors in the catch block for debuggingCurrently, errors are not logged, which can make debugging difficult. Consider logging the error details:
} catch (error) { + console.error("Error during email verification:", error); return NextResponse.json( { error: "Internal server error" }, { status: 500 }, ); }Ensure that sensitive information is not exposed in logs.
app/verify-email/_client.tsx (1)
63-93: Enhance user experience on error stateWhen the verification fails, consider providing a retry option or guidance on the next steps the user can take. This improves the user experience by giving them a clear path forward.
For example, you could add a "Retry" button or a link to contact support.
server/api/router/profile.ts (1)
136-164: Add unit tests for theupdateEmailprocedureTo ensure the new email update functionality works correctly and to prevent future regressions, consider adding unit tests for the
updateEmailprocedure.app/(app)/settings/_client.tsx (1)
407-407: Keep loader icon sizes consistent for uniform UI.The
Loadericons used in the "Send verification link" and "Resend verification link" buttons have different sizes. In line 407, the icon hash-4 w-4, while in line 423, it usesh-8 w-8. For a consistent user experience, consider standardizing the icon sizes.Apply this diff to unify the loader icon sizes:
// In the "Resend verification link" button {loading && ( - <Loader className="text-primary h-8 w-8 animate-spin" /> + <Loader className="text-primary h-4 w-4 animate-spin" /> )}Also applies to: 423-423
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
drizzle/meta/0007_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
- app/(app)/settings/_client.tsx (7 hunks)
- app/api/verify-email/route.ts (1 hunks)
- app/verify-email/_client.tsx (1 hunks)
- app/verify-email/page.tsx (1 hunks)
- config/contants.ts (1 hunks)
- drizzle/0007_added-emailtokenverification-schema.sql (1 hunks)
- schema/token.ts (1 hunks)
- server/api/router/profile.ts (2 hunks)
- utils/emailToken.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (15)
config/contants.ts (1)
1-3: ```shell#!/bin/bash
Description: Search for time-related constants or configurations related to email verification in TypeScript and JavaScript files
Search for time-related constants or configurations
rg --type ts --type js -i '(expir|timeout|duration|ttl).(\d+\s(*\s60\s){2}|\d+\s*hours?|3600000)'
</blockquote></details> <details> <summary>schema/token.ts (4)</summary><blockquote> `1-1`: **LGTM: Zod import looks good.** The import statement for Zod is correct and follows the common convention of importing it as 'z'. --- `3-5`: **LGTM: Email token request schema is well-defined.** The `emailTokenReqSchema` correctly validates the new email input, ensuring it's a string in email format. This aligns well with the PR objective of implementing an email verification system. --- `7-13`: **LGTM: Token schema is comprehensive and well-structured.** The `TokenSchema` includes all necessary fields for a token object: - `id`: Correctly uses UUID format. - `userId`: Stored as a string. - `token`: Stored as a string. - `createdAt`: Uses a date with a default value of the current date. - `expiresAt`: Uses a date for token expiration. This structure supports the email verification process outlined in the PR objectives. --- `15-15`: **LGTM: TokenInput type alias is correctly defined.** The `TokenInput` type alias is properly inferred from the `TokenSchema` using Zod's `TypeOf` utility. This ensures type safety and consistency between the schema validation and TypeScript types. </blockquote></details> <details> <summary>app/verify-email/page.tsx (4)</summary><blockquote> `1-5`: **LGTM: Imports are well-organized and relevant.** The imports are logically ordered and include all necessary modules for the component's functionality. --- `7-9`: **LGTM: Metadata export is correct and descriptive.** The metadata export follows Next.js best practices and provides an appropriate title for the page. --- `1-30`: **Overall implementation looks good, but verify complete email verification flow.** The `page.tsx` file for the "Verify Email" page is well-structured and follows Next.js and TypeScript best practices. It correctly handles authentication and user verification before rendering the main content. However, this file doesn't directly implement email update or verification functionality. To ensure the complete email verification system is working as intended: 1. Verify that the `Content` component (imported from `./_client`) implements the necessary UI and logic for email updates and verification. 2. Check that the API endpoints for updating and verifying email addresses are correctly implemented and integrated. 3. Ensure that the email verification token generation and validation are securely implemented in the backend. To help verify the complete email verification flow, you can run the following script to check for related implementations: ```shell #!/bin/bash # Description: Check for email verification related implementations # Test 1: Search for email update functionality in the Content component echo "Checking Content component for email update functionality:" rg --type typescript -A 10 'updateEmail|verifyEmail' app/verify-email/_client.tsx # Test 2: Search for API endpoints related to email verification echo "Checking for API endpoints related to email verification:" rg --type typescript -A 5 'router\.(post|put|patch)\s*\([\'"]\/api\/.*email' app/api # Test 3: Search for email verification token handling in the backend echo "Checking for email verification token handling:" rg --type typescript -A 10 'generateEmailVerificationToken|validateEmailVerificationToken' server/
11-30: LGTM: Page function handles authentication and rendering correctly.The Page function effectively manages authentication, user verification, and rendering. It follows good practices such as early returns for error cases and uses type-safe database queries.
Could you verify if the Content component requires any props? Currently, it's rendered without passing any data:
return <Content />;If the Content component needs access to user data or session information, consider passing relevant props. Here's a script to check the Content component's implementation:
drizzle/0007_added-emailtokenverification-schema.sql (2)
26-32: LGTM! Good improvements to data integrity and tracking.The changes to set CURRENT_TIMESTAMP as the default for updatedAt columns in various tables is a good practice. It ensures that the last modification time is automatically tracked without requiring explicit updates in application code.
Adding a primary key constraint to the account table using the provider and providerAccountId columns is also a good improvement for data integrity.
11-25: Please clarify the reason for dropping these indexes.The migration is dropping several indexes from various tables. While this might be part of a larger schema restructuring, it's important to understand the reasoning behind these changes and their potential impact on query performance.
Could you provide more context on why these indexes are being removed? Are they being replaced with different indexes, or has the query pattern changed to make them unnecessary?
To assess the impact, let's check if these indexes are recreated later in the migration:
✅ Verification successful
Index Dropping and Recreation Verified
All dropped indexes are being consistently recreated within this migration, ensuring no disruption to query performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if dropped indexes are recreated echo "Indexes recreated in this migration:" grep -n "CREATE.*INDEX" drizzle/0007_added-emailtokenverification-schema.sql echo "\nIndexes not recreated:" for index in "BannedUsers_userId_key" "Bookmark_userId_postId_key" "Community_id_key" "Community_slug_key" "Event_id_key" "Event_slug_key" "Like_userId_commentId_key" "Like_userId_postId_key" "Post_id_key" "Post_slug_key" "PostTag_tagId_postId_key" "Tag_title_key" "User_username_key" "User_email_key" "User_username_id_idx" do if ! grep -q "CREATE.*INDEX.*$index" drizzle/0007_added-emailtokenverification-schema.sql; then echo $index fi doneLength of output: 8365
utils/emailToken.ts (3)
1-5: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary modules for the file's functionality.
7-9: LGTM: Secure token generation implemented.The
generateEmailTokenfunction uses a cryptographically secure method to generate a random token, which is appropriate for email verification purposes.
1-126: Overall assessment: Good implementation with room for improvements.This file provides a comprehensive set of utilities for managing email verification tokens. The functions are well-structured and cover the necessary operations (generate, store, send, retrieve, update, and delete). The email template is also well-designed and responsive.
However, there are several areas where the code can be improved:
- Error handling should be added to all database operations.
- The
getTokenFromDbfunction should check for token expiration.- The
updateEmailanddeleteTokenFromDbfunctions should provide confirmation of successful operations.- The company name in the email template should be updated for consistency.
Implementing the suggested changes will significantly enhance the robustness, security, and maintainability of this module.
app/verify-email/_client.tsx (1)
8-97: Well-structured implementation of the email verification componentThe
Contentcomponent effectively implements the email verification process, handling different states and providing appropriate feedback to the user. The code is clean, readable, and aligns with the PR objectives.
| const TOKEN_EXPIRATION_TIME = 1000 * 60 * 60; // 1 hour | ||
|
|
||
| export { TOKEN_EXPIRATION_TIME }; |
There was a problem hiding this comment.
💡 Codebase verification
Filename Typo Detected
The file config/contants.ts appears to contain a typo. Consider renaming it to constants.ts to maintain consistency and avoid confusion.
🔗 Analysis chain
Potential typo in file name
The file name appears to be "contants.ts" which might be a typo. Did you mean "constants.ts"?
Let's verify the file name:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's a typo in the file name
# List files with names similar to "constants.ts" or "contants.ts"
fd -t f -g '*on?tants.ts' -d 1 config
Length of output: 268
Script:
#!/bin/bash
# Description: Correctly check for "constants.ts" or "contants.ts" in the config directory
# List files named "constants.ts" or "contants.ts" in the config directory
fd -t f -e ts -g 'constants.ts' config || fd -t f -e ts -g 'contants.ts' config
Length of output: 179
| export const storeTokenInDb = async ( | ||
| userId: string, | ||
| token: string, | ||
| expiresAt: Date, | ||
| email: string, | ||
| ) => { | ||
| const newToken = await db | ||
| .insert(emailVerificationToken) | ||
| .values({ | ||
| userId, | ||
| token, | ||
| expiresAt, | ||
| email, | ||
| }) | ||
| .returning(); | ||
|
|
||
| return newToken[0]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to storeTokenInDb.
While the function correctly stores the token in the database, it lacks error handling. Consider wrapping the database operation in a try-catch block to handle potential errors gracefully.
Here's a suggested improvement:
export const storeTokenInDb = async (
userId: string,
token: string,
expiresAt: Date,
email: string,
) => {
try {
const newToken = await db
.insert(emailVerificationToken)
.values({
userId,
token,
expiresAt,
email,
})
.returning();
return newToken[0];
} catch (error) {
console.error('Error storing token in database:', error);
throw new Error('Failed to store email verification token');
}
};This change will provide better error handling and logging, making it easier to debug issues in production.
| export const getTokenFromDb = async (token: string, userId: string) => { | ||
| const tokenFromDb = await db | ||
| .select() | ||
| .from(emailVerificationToken) | ||
| .where( | ||
| and( | ||
| eq(emailVerificationToken.token, token), | ||
| eq(emailVerificationToken.userId, userId), | ||
| ), | ||
| ); | ||
| return tokenFromDb; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance getTokenFromDb for better security and consistency.
The current implementation has two areas for improvement:
- It doesn't check if the token has expired, which could be a security risk.
- It returns an array of tokens instead of a single token, which might be unexpected for the caller.
Consider applying these changes:
import { gt } from "drizzle-orm";
export const getTokenFromDb = async (token: string, userId: string) => {
const tokenFromDb = await db
.select()
.from(emailVerificationToken)
.where(
and(
eq(emailVerificationToken.token, token),
eq(emailVerificationToken.userId, userId),
gt(emailVerificationToken.expiresAt, new Date()) // Check if token is not expired
),
)
.limit(1); // Ensure we only get one result
return tokenFromDb[0] || null; // Return null if no token found
};These changes will ensure that only valid, non-expired tokens are returned, and the function consistently returns a single token object or null.
| export const updateEmail = async (userId: string, newEmail: string) => { | ||
| await db.update(user).set({ email: newEmail }).where(eq(user.id, userId)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve updateEmail with error handling and success confirmation.
The current implementation lacks error handling and doesn't provide confirmation of a successful update. This could lead to silent failures and make debugging difficult.
Consider applying these changes:
export const updateEmail = async (userId: string, newEmail: string): Promise<boolean> => {
try {
const result = await db
.update(user)
.set({ email: newEmail })
.where(eq(user.id, userId))
.returning({ updatedId: user.id });
return result.length > 0;
} catch (error) {
console.error('Error updating email:', error);
throw new Error('Failed to update email');
}
};These changes will:
- Add error handling to catch and log any database errors.
- Return a boolean indicating whether the update was successful.
- Use the
returningclause to confirm that a row was actually updated.
This will make the function more robust and easier to use in the calling code.
| export const deleteTokenFromDb = async (token: string) => { | ||
| await db | ||
| .delete(emailVerificationToken) | ||
| .where(eq(emailVerificationToken.token, token)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance deleteTokenFromDb for improved security and error handling.
The current implementation has two main areas for improvement:
- It doesn't include the user ID in the deletion criteria, which could be a security risk.
- It lacks error handling and doesn't confirm if a deletion actually occurred.
Consider applying these changes:
export const deleteTokenFromDb = async (token: string, userId: string): Promise<boolean> => {
try {
const result = await db
.delete(emailVerificationToken)
.where(
and(
eq(emailVerificationToken.token, token),
eq(emailVerificationToken.userId, userId)
)
)
.returning({ deletedId: emailVerificationToken.id });
return result.length > 0;
} catch (error) {
console.error('Error deleting token from database:', error);
throw new Error('Failed to delete email verification token');
}
};These changes will:
- Include the user ID in the deletion criteria for improved security.
- Add error handling to catch and log any database errors.
- Return a boolean indicating whether a token was actually deleted.
- Use the
returningclause to confirm that a row was deleted.
This will make the function more secure, robust, and informative for the calling code.
server/api/router/profile.ts
Outdated
| } catch (error) { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Internal server error", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log the error before throwing INTERNAL_SERVER_ERROR
Currently, the caught error is not logged, which can make debugging difficult. Consider logging the error to aid in troubleshooting, ensuring that no sensitive information is exposed.
server/api/router/profile.ts
Outdated
| sendVerificationEmail, | ||
| storeTokenInDb, | ||
| } from "@/utils/emailToken"; | ||
| import { TOKEN_EXPIRATION_TIME } from "@/config/contants"; |
There was a problem hiding this comment.
Fix the typo in the import path for TOKEN_EXPIRATION_TIME
The import path has a typo; 'contants' should be 'constants'.
Apply this diff to correct the import path:
-import { TOKEN_EXPIRATION_TIME } from "@/config/contants";
+import { TOKEN_EXPIRATION_TIME } from "@/config/constants";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { TOKEN_EXPIRATION_TIME } from "@/config/contants"; | |
| import { TOKEN_EXPIRATION_TIME } from "@/config/constants"; |
| if (!newEmail) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid request", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add email format validation for newEmail
Currently, the code checks if newEmail is provided but does not validate its format. It's important to ensure that newEmail is a valid email address to prevent invalid data from being processed.
Apply this diff to add email format validation:
if (!newEmail) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Invalid request",
});
}
+ const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+ if (!emailRegex.test(newEmail)) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Invalid email format",
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(newEmail)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid email format", | |
| }); | |
| } |
| const userId = ctx.session.user.id; | ||
|
|
||
| const token = generateEmailToken(); |
There was a problem hiding this comment.
Check if newEmail is already in use
To prevent users from changing their email to one that is already associated with another account, you should check if newEmail exists in the database.
Apply this diff to add the check:
+ const existingUser = await ctx.db
+ .select()
+ .from(user)
+ .where(eq(user.email, newEmail))
+ .limit(1);
+
+ if (existingUser.length > 0) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "Email is already in use",
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const userId = ctx.session.user.id; | |
| const token = generateEmailToken(); | |
| const userId = ctx.session.user.id; | |
| const token = generateEmailToken(); | |
| const existingUser = await ctx.db | |
| .select() | |
| .from(user) | |
| .where(eq(user.email, newEmail)) | |
| .limit(1); | |
| if (existingUser.length > 0) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email is already in use", | |
| }); | |
| } |
There was a problem hiding this comment.
This is definitely needed!
There was a problem hiding this comment.
@NiallJoeMaher, glad you find it helpful!
(=^･^=)
| const handleNewEmailUpdate = async () => { | ||
| setLoading(true); | ||
| try { | ||
| await updateEmail({ newEmail }); | ||
|
|
||
| toast.success("Verification link sent to your email."); | ||
| setSendForVerification(true); | ||
| } catch (error: any) { | ||
| if ( | ||
| error.data.code === "BAD_REQUEST" || | ||
| error.data?.code === "INTERNAL_SERVER_ERROR" | ||
| ) { | ||
| toast.error(error.data.message); | ||
| } else { | ||
| toast.error("Something went wrong"); | ||
| } | ||
| } | ||
| setLoading(false); | ||
| }; |
There was a problem hiding this comment.
Ensure robust error handling in handleNewEmailUpdate function.
In the catch block, accessing error.data.code and error.data.message without confirming that error.data exists may lead to runtime errors if error.data is undefined. To prevent potential issues, use optional chaining when accessing these properties.
Apply this diff to enhance error handling:
const handleNewEmailUpdate = async () => {
setLoading(true);
try {
await updateEmail({ newEmail });
toast.success("Verification link sent to your email.");
setSendForVerification(true);
} catch (error: any) {
- if (
- error.data.code === "BAD_REQUEST" ||
- error.data?.code === "INTERNAL_SERVER_ERROR"
- ) {
- toast.error(error.data.message);
+ const errorCode = error.data?.code;
+ if (
+ errorCode === "BAD_REQUEST" ||
+ errorCode === "INTERNAL_SERVER_ERROR"
+ ) {
+ toast.error(error.data?.message || "An error occurred.");
} else {
toast.error("Something went wrong");
}
} finally {
setLoading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleNewEmailUpdate = async () => { | |
| setLoading(true); | |
| try { | |
| await updateEmail({ newEmail }); | |
| toast.success("Verification link sent to your email."); | |
| setSendForVerification(true); | |
| } catch (error: any) { | |
| if ( | |
| error.data.code === "BAD_REQUEST" || | |
| error.data?.code === "INTERNAL_SERVER_ERROR" | |
| ) { | |
| toast.error(error.data.message); | |
| } else { | |
| toast.error("Something went wrong"); | |
| } | |
| } | |
| setLoading(false); | |
| }; | |
| const handleNewEmailUpdate = async () => { | |
| setLoading(true); | |
| try { | |
| await updateEmail({ newEmail }); | |
| toast.success("Verification link sent to your email."); | |
| setSendForVerification(true); | |
| } catch (error: any) { | |
| const errorCode = error.data?.code; | |
| if ( | |
| errorCode === "BAD_REQUEST" || | |
| errorCode === "INTERNAL_SERVER_ERROR" | |
| ) { | |
| toast.error(error.data?.message || "An error occurred."); | |
| } else { | |
| toast.error("Something went wrong"); | |
| } | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
config/constants.ts (1)
1-3: LGTM! Consider adding a purpose comment.The implementation of
TOKEN_EXPIRATION_TIMEis clear and correct. The constant is well-named, accurately calculated, and properly exported.Consider adding a brief comment explaining the purpose of this constant. For example:
// Expiration time for email verification tokens const TOKEN_EXPIRATION_TIME = 1000 * 60 * 60; // 1 hourThis would provide more context for other developers working with this constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- config/constants.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
config/constants.ts (1)
1-1: Verify the token expiration time with the team.The current expiration time of 1 hour (3,600,000 milliseconds) seems reasonable for a token. However, it's important to ensure this aligns with the project's security requirements and user experience expectations.
Please confirm with the team if this duration is appropriate for the email verification process. Consider factors such as:
- How long users typically take to verify their email.
- The balance between security (shorter expiration) and user convenience (longer expiration).
- Any compliance requirements that might affect token lifetimes.
If adjustments are needed, this constant can be easily modified.
|
@NiallJoeMaher Can you check if any more changes require? Key changes
|
NiallJoeMaher
left a comment
NiallJoeMaher
left a comment
There was a problem hiding this comment.
A few errors to catch anywhere you are doing an async/await operation. But nearly there! Well done! Tonne of work.
| CONSTRAINT "EmailVerificationToken_token_unique" UNIQUE("token") | ||
| ); | ||
| --> statement-breakpoint | ||
| DROP INDEX IF EXISTS "BannedUsers_userId_key";--> statement-breakpoint |
There was a problem hiding this comment.
Not sure what is going on here because we seem to be deleting and re-adding indexes (I know this is autocreated so I need to test this locally).
There was a problem hiding this comment.
Yeah even I was not sure why just creating a new schema allows to have a migration which deletes prev indexes and again adding them later
utils/emailToken.ts
Outdated
| <a href="${verificationLink}" class="btn">Verify Email</a> | ||
| <p>Please note that this link is valid for 1 hour only. If it expires, you will need to request a new one.</p> | ||
| <p>If you did not create an account, please ignore this email.</p> | ||
| <p>Best regards, <br>The Codu Team</p> |
There was a problem hiding this comment.
Spellcheck - Codú (we need the accent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
utils/emailToken.ts (1)
1-146: Overall assessment: Good implementation with room for improvementThe
utils/emailToken.tsfile provides a comprehensive set of functions for managing email verification tokens. While the overall implementation is solid, there are several areas where improvements can be made:
- Update the company name in the email template for consistency.
- Enhance the
getTokenFromDbfunction to check for token expiration and return a single token.- Improve the
updateEmailfunction to confirm successful updates.- Strengthen the
deleteTokenFromDbfunction by including the user ID in the deletion criteria and confirming deletions.These improvements will enhance the security, consistency, and reliability of the email verification system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/api/verify-email/route.ts (1 hunks)
- utils/emailToken.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/verify-email/route.ts
🧰 Additional context used
🔇 Additional comments (7)
utils/emailToken.ts (7)
1-5: LGTM: Import statements are well-organized and relevant.The import statements are correctly structured and include all necessary dependencies for the module's functionality.
7-9: LGTM: Secure token generation implementation.The
generateEmailTokenfunction usescrypto.randomBytesto generate a secure random token, which is a best practice for creating unpredictable tokens.
11-33: LGTM: Robust implementation of token storage with error handling.The
storeTokenInDbfunction effectively stores the token in the database and includes proper error handling. This implementation addresses the previous review comment suggestion.
35-108:⚠️ Potential issueUpdate company name and consider using APP_URL environment variable.
The company name in the email template still references "Codú". Please update it to "CodeRabbit Inc." for consistency with your branding.
Consider using
NEXT_PUBLIC_APP_URLinstead ofNEXT_PUBLIC_BACKEND_URLfor the verification link. This makes it clearer that it's the frontend URL where the verification page is hosted.Apply these changes:
- <p>Best regards, <br>The Codú Team</p> + <p>Best regards, <br>The CodeRabbit Inc. Team</p> ... - <p>© ${new Date().getFullYear()} Codú. All rights reserved.</p> + <p>© ${new Date().getFullYear()} CodeRabbit Inc. All rights reserved.</p> ... - const verificationLink = `${process.env.NEXT_PUBLIC_BACKEND_URL}/verify-email?token=${token}`; + const verificationLink = `${process.env.NEXT_PUBLIC_APP_URL}/verify-email?token=${token}`;Make sure to add
NEXT_PUBLIC_APP_URLto your environment variables if it's not already present.
110-126: 🛠️ Refactor suggestionEnhance
getTokenFromDbfor better security and consistency.The current implementation can be improved in two ways:
- It doesn't check if the token has expired, which could be a security risk.
- It returns an array of tokens instead of a single token, which might be unexpected for the caller.
Consider applying these changes:
import { gt } from "drizzle-orm"; export const getTokenFromDb = async (token: string, userId: string) => { try { const tokenFromDb = await db .select() .from(emailVerificationToken) .where( and( eq(emailVerificationToken.token, token), eq(emailVerificationToken.userId, userId), gt(emailVerificationToken.expiresAt, new Date()) // Check if token is not expired ), ) .limit(1); // Ensure we only get one result return tokenFromDb[0] || null; // Return null if no token found } catch (error) { console.error("Error fetching token from database:", error); throw new Error("Failed to fetch email verification token"); } };These changes will ensure that only valid, non-expired tokens are returned, and the function consistently returns a single token object or null.
128-135: 🛠️ Refactor suggestionImprove
updateEmailwith success confirmation.The current implementation lacks confirmation of a successful update. This could make it difficult for the caller to know if the update was successful.
Consider applying these changes:
export const updateEmail = async (userId: string, newEmail: string): Promise<boolean> => { try { const result = await db .update(user) .set({ email: newEmail }) .where(eq(user.id, userId)) .returning({ updatedId: user.id }); return result.length > 0; } catch (error) { console.error("Error updating email in database:", error); throw new Error("Failed to update email"); } };These changes will:
- Return a boolean indicating whether the update was successful.
- Use the
returningclause to confirm that a row was actually updated.This will make the function more informative for the calling code.
137-146: 🛠️ Refactor suggestionEnhance
deleteTokenFromDbfor improved security and confirmation.The current implementation has two main areas for improvement:
- It doesn't include the user ID in the deletion criteria, which could be a security risk.
- It doesn't confirm if a deletion actually occurred.
Consider applying these changes:
export const deleteTokenFromDb = async (token: string, userId: string): Promise<boolean> => { try { const result = await db .delete(emailVerificationToken) .where( and( eq(emailVerificationToken.token, token), eq(emailVerificationToken.userId, userId) ) ) .returning({ deletedId: emailVerificationToken.id }); return result.length > 0; } catch (error) { console.error("Error deleting token from database:", error); throw new Error("Failed to delete email verification token"); } };These changes will:
- Include the user ID in the deletion criteria for improved security.
- Return a boolean indicating whether a token was actually deleted.
- Use the
returningclause to confirm that a row was deleted.This will make the function more secure and informative for the calling code.
NiallJoeMaher
left a comment
NiallJoeMaher
left a comment
There was a problem hiding this comment.
A handful of more changes. This is a huge PR so hopefully the next one we can break into different steps to make it easier for you (and get you some Hacktoberfest points to make up for the pain of this one!)
| const userId = ctx.session.user.id; | ||
|
|
||
| const token = generateEmailToken(); |
There was a problem hiding this comment.
This is definitely needed!
Key changes
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
drizzle/0007_added_emailtokenverification_schema.sql (1)
1-10: LGTM! Consider adding a foreign key constraint for userId.The
EmailVerificationTokentable structure is well-designed for its purpose. It includes all necessary columns with appropriate data types and constraints. The unique constraints ontokenandConsider adding a foreign key constraint for the
userIdcolumn to enforce referential integrity with theusertable. This can be done with an ALTER TABLE statement after the table creation:ALTER TABLE "EmailVerificationToken" ADD CONSTRAINT "EmailVerificationToken_userId_fkey" FOREIGN KEY ("userId") REFERENCES "user"("id") ON DELETE CASCADE;This ensures that every
userIdin theEmailVerificationTokentable corresponds to a valid user and automatically deletes verification tokens when a user is deleted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
drizzle/meta/0007_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
- app/(app)/settings/_client.tsx (7 hunks)
- drizzle/0007_added_emailtokenverification_schema.sql (1 hunks)
- server/api/router/profile.ts (2 hunks)
- server/db/schema.ts (1 hunks)
- utils/emailToken.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/db/schema.ts
🧰 Additional context used
🔇 Additional comments (9)
drizzle/0007_added_emailtokenverification_schema.sql (3)
27-33: LGTM! Good improvements to data integrity and tracking.The changes to set
CURRENT_TIMESTAMPas the default forupdatedAtcolumns in various tables is a good practice. It ensures that the last modification time is automatically tracked without requiring explicit updates in application code.Adding a primary key constraint to the
accounttable usingproviderandproviderAccountIdcolumns is also a positive change for data integrity.These changes will improve data consistency and make it easier to track when records were last modified.
12-26: Verify the impact of dropping existing indexes.The migration drops several indexes from various tables. While this may be necessary for schema changes, it could potentially impact query performance.
Please ensure that:
- The removal of these indexes doesn't negatively impact query performance.
- Any necessary indexes are recreated after the schema changes.
Run the following script to check if any of these indexes are still being used:
#!/bin/bash # Description: Check for queries that might be affected by the removed indexes # Test: Search for queries using the tables with removed indexes rg --type sql -i "FROM\s+(BannedUsers|Bookmark|Community|Event|Like|Post|PostTag|Tag|User|user)" # Note: This is a basic check. A thorough analysis would require examining query plans # and performance metrics in the production environment.Consider consulting with the database administrator or conducting thorough performance testing before applying these changes to the production environment.
40-54: LGTM! Verify if all necessary indexes are recreated.The creation of these unique indexes is a good practice. They enforce data uniqueness and can significantly improve query performance for the covered columns.
Please ensure that:
- All necessary indexes that were dropped earlier have been recreated.
- The new index definitions match the requirements of your most common and performance-critical queries.
Run the following script to compare the dropped and recreated indexes:
Review the output to ensure all necessary indexes are present. If any critical indexes are missing, consider adding them to maintain query performance.
✅ Verification successful
All necessary indexes have been successfully recreated.
The creation of these unique indexes ensures data uniqueness and enhances query performance as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare dropped and recreated indexes # Test: Extract dropped and recreated index names dropped_indexes=$(sed -n '/DROP INDEX/s/.*IF EXISTS "\([^"]*\)".*/\1/p' drizzle/0007_added_emailtokenverification_schema.sql) recreated_indexes=$(sed -n '/CREATE.*INDEX/s/.*IF NOT EXISTS "\([^"]*\)".*/\1/p' drizzle/0007_added_emailtokenverification_schema.sql) # Compare the lists echo "Dropped indexes:" echo "$dropped_indexes" echo "Recreated indexes:" echo "$recreated_indexes" # Find indexes that were dropped but not recreated echo "Indexes dropped but not recreated:" comm -23 <(echo "$dropped_indexes" | sort) <(echo "$recreated_indexes" | sort) # Note: This is a basic comparison. Ensure that the recreated indexes # serve the same purpose as the dropped ones, even if names differ.Length of output: 484
Script:
#!/bin/bash # Description: Compare dropped and recreated indexes # Extract dropped and recreated index names dropped_indexes=$(sed -n '/DROP INDEX/s/.*IF EXISTS "\([^"]*\)".*/\1/p' drizzle/0007_added_emailtokenverification_schema.sql) recreated_indexes=$(sed -n '/CREATE.*INDEX/s/.*IF NOT EXISTS "\([^"]*\)".*/\1/p' drizzle/0007_added_emailtokenverification_schema.sql) # Sort the lists and save to temporary files echo "$dropped_indexes" | sort > dropped_sorted.txt echo "$recreated_indexes" | sort > recreated_sorted.txt # Find indexes that were dropped but not recreated using diff echo "Indexes dropped but not recreated:" diff dropped_sorted.txt recreated_sorted.txt | grep '^<' | sed 's/^< //'Length of output: 1582
app/(app)/settings/_client.tsx (2)
49-52: LGTM: Improved form initialization.The change to spread the entire
profileobject indefaultValuesensures that all profile fields are correctly initialized. This is a good practice for maintaining consistency between the form state and the actual profile data.
Line range hint
1-563: Overall assessment: Well-implemented email update feature.The addition of the email update functionality to the Settings component is well-implemented and integrates smoothly with the existing code. The new feature provides a good user experience with appropriate feedback mechanisms.
Key points:
- The email update UI is clear and user-friendly.
- Error handling and success notifications are in place.
- The new functionality doesn't disrupt existing features.
Consider implementing the suggested improvements to further enhance the user experience and code quality. Great job on this feature implementation!
utils/emailToken.ts (4)
36-36: Previous comment still appliesThe previous review comment about using an appropriate environment variable for the base URL (e.g.,
NEXT_PUBLIC_APP_URLinstead ofNEXT_PUBLIC_BACKEND_URL) still applies. Updating this will enhance flexibility across different environments.
110-126: Previous comment still appliesThe previous review comment regarding enhancing
getTokenFromDbfor better security and consistency still applies. Implementing the suggested changes will ensure expired tokens are not used and that the function consistently returns a single token.
128-135: Previous comment still appliesThe previous review comment about improving
updateEmailwith error handling and success confirmation still applies. Modifying the function to confirm that the update occurred will make it more robust.
137-146: Previous comment still appliesThe previous review comment regarding enhancing
deleteTokenFromDbfor improved security and error handling still applies. Including theuserIdin the deletion criteria and confirming the deletion will strengthen the function's reliability.
| DO $$ BEGIN | ||
| ALTER TABLE "EmailVerificationToken" ADD CONSTRAINT "EmailVerificationToken_userId_user_id_fk" FOREIGN KEY ("userId") REFERENCES "public"."user"("id") ON DELETE no action ON UPDATE no action; | ||
| EXCEPTION | ||
| WHEN duplicate_object THEN null; | ||
| END $$; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider changing ON DELETE action to CASCADE for EmailVerificationToken foreign key.
The addition of a foreign key constraint for the userId column in the EmailVerificationToken table is good for maintaining referential integrity. However, the current ON DELETE and ON UPDATE actions are set to "no action".
Consider changing the ON DELETE action to CASCADE. This would automatically delete the verification tokens when a user is deleted, preventing orphaned records. Here's the suggested modification:
ALTER TABLE "EmailVerificationToken"
ADD CONSTRAINT "EmailVerificationToken_userId_user_id_fk"
FOREIGN KEY ("userId")
REFERENCES "public"."user"("id")
ON DELETE CASCADE
ON UPDATE NO ACTION;This change ensures that the EmailVerificationToken table remains consistent with the user table without manual intervention.
| import { Button } from "@/components/ui-components/button"; | ||
| import { CheckCheck, Loader2 } from "lucide-react"; |
There was a problem hiding this comment.
Remove unused import 'CheckCheck' from 'lucide-react'.
The CheckCheck component is imported but not used in the code. Removing unused imports helps maintain a clean and efficient codebase.
Apply this diff to fix the issue:
-import { CheckCheck, Loader2 } from "lucide-react";
+import { Loader2 } from "lucide-react";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { Button } from "@/components/ui-components/button"; | |
| import { CheckCheck, Loader2 } from "lucide-react"; | |
| import { Button } from "@/components/ui-components/button"; | |
| import { Loader2 } from "lucide-react"; |
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | ||
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | ||
| Update email | ||
| </h2> | ||
| <p className="mt-1 text-sm">Change your email here.</p> | ||
| <div className="mt-2 flex flex-col gap-2"> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="currEmail">Current email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="currEmail" | ||
| value={profile.email!} | ||
| disabled | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="newEmail">Update email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="newEmail" | ||
| onChange={(e) => setNewEmail(e.target.value)} | ||
| value={newEmail} | ||
| /> | ||
| </div> | ||
| </div> | ||
| {!sendForVerification ? ( | ||
| <Button | ||
| className="w-[200px]" | ||
| disabled={!(newEmail || loading)} | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Send verification link | ||
| </Button> | ||
| ) : ( | ||
| <div className="mt-2 flex flex-row gap-2"> | ||
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | ||
| <CheckCheck /> | ||
| Verification link sent | ||
| </h2> | ||
| <Button | ||
| className="w-[250px]" | ||
| disabled={!(newEmail || loading)} | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Resend verification link | ||
| </Button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button disabled state logic.
The current implementation of the disabled state for the "Send verification link" and "Resend verification link" buttons can be improved for better user experience and logic clarity.
Consider the following changes:
- The button should be disabled when there's no new email entered or when it's the same as the current email.
- The loading state should also be considered in the disabled logic.
Apply this diff to improve the logic:
- disabled={!(newEmail || loading)}
+ disabled={!newEmail || newEmail === profile.email || loading}This change ensures that the button is disabled when:
- No new email is entered
- The new email is the same as the current email
- The form is in a loading state
This provides a more intuitive user experience and prevents unnecessary API calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={!(newEmail || loading)} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={!(newEmail || loading)} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={!newEmail || newEmail === profile.email || loading} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={!newEmail || newEmail === profile.email || loading} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> | |
| </div> |
| throw new TRPCError({ | ||
| code: error.code || "INTERNAL_SERVER_ERROR", | ||
| message: error.message || "Internal server error", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid exposing internal error details to the client
Returning the original error code and message could potentially leak sensitive information. It's safer to log the error internally and return a generic error message to the client.
Apply this diff to fix the error handling:
} catch (error: any) {
+ // Log the error internally for debugging purposes
+ console.error('Error updating email:', error);
- throw new TRPCError({
- code: error.code || "INTERNAL_SERVER_ERROR",
- message: error.message || "Internal server error",
- });
+ throw new TRPCError({
+ code: "INTERNAL_SERVER_ERROR",
+ message: "An unexpected error occurred",
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new TRPCError({ | |
| code: error.code || "INTERNAL_SERVER_ERROR", | |
| message: error.message || "Internal server error", | |
| }); | |
| } | |
| } catch (error: any) { | |
| // Log the error internally for debugging purposes | |
| console.error('Error updating email:', error); | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: "An unexpected error occurred", | |
| }); | |
| } |
| if (!newEmail) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid request", | ||
| }); | ||
| } | ||
|
|
||
| const ifEmailExists = await checkIfEmailExists(newEmail); | ||
|
|
||
| if (ifEmailExists) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Email already exists", | ||
| }); | ||
| } | ||
|
|
||
| const userId = ctx.session.user.id; | ||
|
|
||
| const token = generateEmailToken(); | ||
| const expiresAt = new Date(Date.now() + TOKEN_EXPIRATION_TIME); | ||
|
|
||
| await storeTokenInDb(userId, token, expiresAt, newEmail); | ||
| await sendVerificationEmail(newEmail, token); | ||
|
|
There was a problem hiding this comment.
Ensure email addresses are normalized and comparisons are case-insensitive
Email addresses are case-insensitive, so it's important to normalize them to lower case before processing. This avoids issues where [email protected] and [email protected] are treated differently.
Apply this diff to normalize the email address to lower case:
const { newEmail } = input;
if (!newEmail) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Invalid request",
});
}
+ const normalizedEmail = newEmail.toLowerCase();
- const ifEmailExists = await checkIfEmailExists(newEmail);
+ const ifEmailExists = await checkIfEmailExists(normalizedEmail);
if (ifEmailExists) {
throw new TRPCError({
code: "BAD_REQUEST",
message: "Email already exists",
});
}
const userId = ctx.session.user.id;
const token = generateEmailToken();
const expiresAt = new Date(Date.now() + TOKEN_EXPIRATION_TIME);
- await storeTokenInDb(userId, token, expiresAt, newEmail);
- await sendVerificationEmail(newEmail, token);
+ await storeTokenInDb(userId, token, expiresAt, normalizedEmail);
+ await sendVerificationEmail(normalizedEmail, token);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| const ifEmailExists = await checkIfEmailExists(newEmail); | |
| if (ifEmailExists) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email already exists", | |
| }); | |
| } | |
| const userId = ctx.session.user.id; | |
| const token = generateEmailToken(); | |
| const expiresAt = new Date(Date.now() + TOKEN_EXPIRATION_TIME); | |
| await storeTokenInDb(userId, token, expiresAt, newEmail); | |
| await sendVerificationEmail(newEmail, token); | |
| const { newEmail } = input; | |
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| const normalizedEmail = newEmail.toLowerCase(); | |
| const ifEmailExists = await checkIfEmailExists(normalizedEmail); | |
| if (ifEmailExists) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email already exists", | |
| }); | |
| } | |
| const userId = ctx.session.user.id; | |
| const token = generateEmailToken(); | |
| const expiresAt = new Date(Date.now() + TOKEN_EXPIRATION_TIME); | |
| await storeTokenInDb(userId, token, expiresAt, normalizedEmail); | |
| await sendVerificationEmail(normalizedEmail, token); |
| export const sendVerificationEmail = async (email: string, token: string) => { | ||
| const verificationLink = `${process.env.NEXT_PUBLIC_BACKEND_URL}/verify-email?token=${token}`; | ||
| const subject = "Verify Your Email Address"; | ||
|
|
||
| const htmlMessage = ` | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Email Verification</title> | ||
| <style> | ||
| body { | ||
| font-family: Arial, sans-serif; | ||
| background-color: #f4f4f4; | ||
| color: #333; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| .container { | ||
| width: 100%; | ||
| max-width: 600px; | ||
| margin: 0 auto; | ||
| background-color: #ffffff; | ||
| padding: 20px; | ||
| border-radius: 8px; | ||
| box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1); | ||
| } | ||
| h1 { | ||
| font-size: 24px; | ||
| margin-bottom: 20px; | ||
| } | ||
| p { | ||
| font-size: 16px; | ||
| line-height: 1.6; | ||
| margin-bottom: 20px; | ||
| } | ||
| .btn { | ||
| display: inline-block; | ||
| padding: 12px 24px; | ||
| font-size: 16px; | ||
| color: #fff !important; | ||
| font-weight: bold; | ||
| background-color: #007bff; | ||
| text-decoration: none; | ||
| border-radius: 4px; | ||
| margin-top: 20px; | ||
| } | ||
| .footer { | ||
| margin-top: 40px; | ||
| font-size: 12px; | ||
| color: #777; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <h1>Confirm Your Email Address</h1> | ||
| <p>Hello,</p> | ||
| <p>Thank you for registering with us! To complete your registration, please verify your email address by clicking the button below:</p> | ||
| <a href="${verificationLink}" class="btn">Verify Email</a> | ||
| <p>Please note that this link is valid for 1 hour only. If it expires, you will need to request a new one.</p> | ||
| <p>If you did not create an account, please ignore this email.</p> | ||
| <p>Best regards, <br>The Codú Team</p> | ||
| <div class="footer"> | ||
| <p>© ${new Date().getFullYear()} Codú. All rights reserved.</p> | ||
| </div> | ||
| </div> | ||
| </body> | ||
| </html> | ||
| `; | ||
|
|
||
| return sendEmail({ recipient: email, htmlMessage, subject }); | ||
| }; |
There was a problem hiding this comment.
Add error handling to sendVerificationEmail
Currently, the sendVerificationEmail function does not handle potential errors from the sendEmail function. Adding error handling will ensure that any issues during the email sending process are caught and managed appropriately.
Consider implementing error handling as follows:
export const sendVerificationEmail = async (email: string, token: string) => {
const verificationLink = `${process.env.NEXT_PUBLIC_BACKEND_URL}/verify-email?token=${token}`;
const subject = "Verify Your Email Address";
const htmlMessage = `...`; // Existing HTML content
try {
await sendEmail({ recipient: email, htmlMessage, subject });
} catch (error) {
console.error("Error sending verification email:", error);
throw new Error("Failed to send verification email");
}
};| const htmlMessage = ` | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Email Verification</title> | ||
| <style> | ||
| body { | ||
| font-family: Arial, sans-serif; | ||
| background-color: #f4f4f4; | ||
| color: #333; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| .container { | ||
| width: 100%; | ||
| max-width: 600px; | ||
| margin: 0 auto; | ||
| background-color: #ffffff; | ||
| padding: 20px; | ||
| border-radius: 8px; | ||
| box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1); | ||
| } | ||
| h1 { | ||
| font-size: 24px; | ||
| margin-bottom: 20px; | ||
| } | ||
| p { | ||
| font-size: 16px; | ||
| line-height: 1.6; | ||
| margin-bottom: 20px; | ||
| } | ||
| .btn { | ||
| display: inline-block; | ||
| padding: 12px 24px; | ||
| font-size: 16px; | ||
| color: #fff !important; | ||
| font-weight: bold; | ||
| background-color: #007bff; | ||
| text-decoration: none; | ||
| border-radius: 4px; | ||
| margin-top: 20px; | ||
| } | ||
| .footer { | ||
| margin-top: 40px; | ||
| font-size: 12px; | ||
| color: #777; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <h1>Confirm Your Email Address</h1> | ||
| <p>Hello,</p> | ||
| <p>Thank you for registering with us! To complete your registration, please verify your email address by clicking the button below:</p> | ||
| <a href="${verificationLink}" class="btn">Verify Email</a> | ||
| <p>Please note that this link is valid for 1 hour only. If it expires, you will need to request a new one.</p> | ||
| <p>If you did not create an account, please ignore this email.</p> | ||
| <p>Best regards, <br>The Codú Team</p> | ||
| <div class="footer"> | ||
| <p>© ${new Date().getFullYear()} Codú. All rights reserved.</p> | ||
| </div> | ||
| </div> | ||
| </body> | ||
| </html> | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a template engine for email construction
The HTML content for the email is currently defined within a template string. For better maintainability and readability, consider using a templating engine like Handlebars or EJS. This approach will make it easier to manage and update the email content, especially as it grows in complexity.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/(app)/settings/_client.tsx (7 hunks)
- utils/emailToken.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/emailToken.ts
🧰 Additional context used
🔇 Additional comments (3)
app/(app)/settings/_client.tsx (3)
32-33: LGTM: User type update.The addition of 'email' and 'id' fields to the User type is consistent with the new email-related features.
49-52: LGTM: New state variables and API mutation for email update.The addition of new state variables (newEmail, sendForVerification, loading) and the updateEmail mutation are consistent with the new email update functionality. The defaultValues update in the useForm hook ensures proper initialization of the email field.
Also applies to: 62-64, 76-76
15-16:⚠️ Potential issueRemove unused import 'CheckCheck' from 'lucide-react'.
The
CheckCheckcomponent is imported but not used in the code. Removing unused imports helps maintain a clean and efficient codebase.Apply this diff to fix the issue:
-import { CheckCheck, Loader2 } from "lucide-react"; +import { Loader2 } from "lucide-react";Likely invalid or redundant comment.
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | ||
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | ||
| Update email | ||
| </h2> | ||
| <p className="mt-1 text-sm">Change your email here.</p> | ||
| <div className="mt-2 flex flex-col gap-2"> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="currEmail">Current email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="currEmail" | ||
| value={profile.email!} | ||
| disabled | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="newEmail">Update email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="newEmail" | ||
| onChange={(e) => setNewEmail(e.target.value)} | ||
| value={newEmail} | ||
| /> | ||
| </div> | ||
| </div> | ||
| {!sendForVerification ? ( | ||
| <Button | ||
| className="w-[200px]" | ||
| disabled={!(newEmail || loading)} | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Send verification link | ||
| </Button> | ||
| ) : ( | ||
| <div className="mt-2 flex flex-row gap-2"> | ||
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | ||
| <CheckCheck /> | ||
| Verification link sent | ||
| </h2> | ||
| <Button | ||
| className="w-[250px]" | ||
| disabled={ | ||
| !newEmail || newEmail === profile.email || loading | ||
| } | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Resend verification link | ||
| </Button> | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button disabled state logic.
The current implementation of the disabled state for the "Send verification link" and "Resend verification link" buttons can be improved for better user experience and logic clarity.
Consider the following changes:
- The button should be disabled when there's no new email entered or when it's the same as the current email.
- The loading state should also be considered in the disabled logic.
Apply this diff to improve the logic:
- disabled={!(newEmail || loading)}
+ disabled={!newEmail || newEmail === profile.email || loading}This change ensures that the button is disabled when:
- No new email is entered
- The new email is the same as the current email
- The form is in a loading state
This provides a more intuitive user experience and prevents unnecessary API calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={!(newEmail || loading)} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={ | |
| !newEmail || newEmail === profile.email || loading | |
| } | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> | |
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={!newEmail || newEmail === profile.email || loading} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={ | |
| !newEmail || newEmail === profile.email || loading | |
| } | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/(app)/settings/_client.tsx (7 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/(app)/settings/_client.tsx (4)
32-33: LGTM: User type update is consistent with new functionality.The addition of
idfields to theUsertype is appropriate for the new email verification feature.
49-52: LGTM: Proper initialization and setup for email verification feature.The new state variables, updated defaultValues, and new API mutation are well-implemented and necessary for the email verification functionality.
Also applies to: 62-64, 76-76
Line range hint
1-563: Overall implementation of email verification feature is well-done.The changes to implement the email verification feature are generally well-structured and integrated into the existing component. The new UI elements and functionality enhance the user experience by allowing email updates with proper verification.
A few minor improvements have been suggested:
- Removing an unused import.
- Enhancing error handling in the
handleNewEmailUpdatefunction.- Improving the disabled state logic for the verification link buttons.
With these small adjustments, the code will be more robust and provide an even better user experience.
15-16:⚠️ Potential issueRemove unused import 'CheckCheck' from 'lucide-react'.
The
CheckCheckcomponent is imported but not used in the code. Removing unused imports helps maintain a clean and efficient codebase.Apply this diff to fix the issue:
-import { CheckCheck, Loader2 } from "lucide-react"; +import { Loader2 } from "lucide-react";Likely invalid or redundant comment.
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | ||
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | ||
| Update email | ||
| </h2> | ||
| <p className="mt-1 text-sm">Change your email here.</p> | ||
| <div className="mt-2 flex flex-col gap-2"> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="currEmail">Current email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="currEmail" | ||
| value={profile.email!} | ||
| disabled | ||
| /> | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-col"> | ||
| <label htmlFor="newEmail">Update email</label> | ||
| <div> | ||
| <input | ||
| type="email" | ||
| id="newEmail" | ||
| onChange={(e) => setNewEmail(e.target.value)} | ||
| value={newEmail} | ||
| /> | ||
| </div> | ||
| </div> | ||
| {!sendForVerification ? ( | ||
| <Button | ||
| className="w-[200px]" | ||
| disabled={ | ||
| !newEmail || newEmail === profile.email || loading | ||
| } | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Send verification link | ||
| </Button> | ||
| ) : ( | ||
| <div className="mt-2 flex flex-row gap-2"> | ||
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | ||
| <CheckCheck /> | ||
| Verification link sent | ||
| </h2> | ||
| <Button | ||
| className="w-[250px]" | ||
| disabled={ | ||
| !newEmail || newEmail === profile.email || loading | ||
| } | ||
| onClick={handleNewEmailUpdate} | ||
| > | ||
| {loading && ( | ||
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | ||
| )} | ||
| Resend verification link | ||
| </Button> | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button disabled state logic.
The current implementation of the disabled state for the "Send verification link" and "Resend verification link" buttons can be improved for better user experience and logic clarity.
Consider the following changes:
- The button should be disabled when there's no new email entered or when it's the same as the current email.
- The loading state should also be considered in the disabled logic.
Apply this diff to improve the logic:
- disabled={!(newEmail || loading)}
+ disabled={!newEmail || newEmail === profile.email || loading}This change ensures that the button is disabled when:
- No new email is entered
- The new email is the same as the current email
- The form is in a loading state
This provides a more intuitive user experience and prevents unnecessary API calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={ | |
| !newEmail || newEmail === profile.email || loading | |
| } | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={ | |
| !newEmail || newEmail === profile.email || loading | |
| } | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> | |
| <div className="mt-6 text-neutral-600 dark:text-neutral-400"> | |
| <h2 className="text-xl font-bold tracking-tight text-neutral-800 dark:text-white"> | |
| Update email | |
| </h2> | |
| <p className="mt-1 text-sm">Change your email here.</p> | |
| <div className="mt-2 flex flex-col gap-2"> | |
| <div className="flex flex-col"> | |
| <label htmlFor="currEmail">Current email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="currEmail" | |
| value={profile.email!} | |
| disabled | |
| /> | |
| </div> | |
| </div> | |
| <div className="flex flex-col"> | |
| <label htmlFor="newEmail">Update email</label> | |
| <div> | |
| <input | |
| type="email" | |
| id="newEmail" | |
| onChange={(e) => setNewEmail(e.target.value)} | |
| value={newEmail} | |
| /> | |
| </div> | |
| </div> | |
| {!sendForVerification ? ( | |
| <Button | |
| className="w-[200px]" | |
| disabled={!newEmail || newEmail === profile.email || loading} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Send verification link | |
| </Button> | |
| ) : ( | |
| <div className="mt-2 flex flex-row gap-2"> | |
| <h2 className="flex items-center gap-2 text-sm italic text-green-400"> | |
| <CheckCheck /> | |
| Verification link sent | |
| </h2> | |
| <Button | |
| className="w-[250px]" | |
| disabled={!newEmail || newEmail === profile.email || loading} | |
| onClick={handleNewEmailUpdate} | |
| > | |
| {loading && ( | |
| <Loader2 className="text-primary h-6 w-6 animate-spin" /> | |
| )} | |
| Resend verification link | |
| </Button> | |
| </div> | |
| )} | |
| </div> |
|
A few conflicts, if you could delete the generated migration file and the meta files that were generated, pull the changes and re-run with |
Yeah sure. Let me do that once. Edit : Done ✅ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
drizzle/0009_email-verification.sql (1)
1-10: Table structure looks good, consider additional optimizations.The
EmailVerificationTokentable structure is well-designed for its purpose. All necessary columns are present with appropriate constraints. However, consider the following optimizations:
- Add an index on the
userIdcolumn to improve query performance when looking up tokens for a specific user.- Consider using
uuidinstead ofserialfor theidcolumn to prevent potential enumeration attacks.- The
tokencolumn could be of typevarchar(n)with an appropriate length limit instead oftextto optimize storage.Would you like me to provide the SQL statements for these optimizations?
app/(app)/settings/_client.tsx (1)
62-64: Consider initializing 'newEmail' with the current email.Initializing
newEmailwith the current email can improve user experience by providing a starting point for editing.Consider applying this change:
-const [newEmail, setNewEmail] = useState(""); +const [newEmail, setNewEmail] = useState(profile.email || "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
drizzle/meta/0009_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (3)
- app/(app)/settings/_client.tsx (7 hunks)
- drizzle/0009_email-verification.sql (1 hunks)
- server/db/schema.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/db/schema.ts
🧰 Additional context used
🔇 Additional comments (6)
drizzle/0009_email-verification.sql (2)
8-9: Unique constraints are appropriately defined.The unique constraints on both
tokenand
EmailVerificationToken_token_uniqueensures each token is unique, preventing potential conflicts or security issues.EmailVerificationToken_email_uniqueprevents multiple verification tokens for the same email, which is logical for this system.These constraints contribute to data integrity and system security.
12-16: 🛠️ Refactor suggestionForeign key constraint is good, but consider ON DELETE CASCADE.
The foreign key constraint
EmailVerificationToken_userId_user_id_fkis well-implemented:
- It ensures referential integrity between the
EmailVerificationTokenandusertables.- The DO block with exception handling is a good practice for idempotent migrations.
However, consider changing the ON DELETE behavior:
Replace:
ON DELETE no action ON UPDATE no actionWith:
ON DELETE CASCADE ON UPDATE no actionThis change will automatically remove associated verification tokens when a user is deleted, preventing orphaned records.
To check the current constraints on the
EmailVerificationTokentable, run:This will display the table structure, including constraints, allowing you to verify the current setup and any changes made.
app/(app)/settings/_client.tsx (4)
32-33: LGTM: User type updated correctly.The addition of
idfields to theUsertype is appropriate for the new email update functionality.
372-433: LGTM: Email update UI implemented correctly.The new UI elements for email update are well-implemented. The component correctly handles different states (initial, loading, and after sending verification) and provides appropriate feedback to the user.
However, there's a small improvement we can make to the button disabled state:
-disabled={!(newEmail || loading)} +disabled={!newEmail || newEmail === profile.email || loading}This change ensures that the button is disabled when:
- No new email is entered
- The new email is the same as the current email
- The form is in a loading state
This provides a more intuitive user experience and prevents unnecessary API calls.
Line range hint
1-605: LGTM: Email update feature well-implemented.The new email update feature has been successfully integrated into the
Settingscomponent. The implementation follows existing patterns and maintains code quality. Good job on adding this functionality while keeping the component structure clean and organized.
15-16:⚠️ Potential issueRemove unused import 'CheckCheck' from 'lucide-react'.
The
CheckCheckicon is imported but not used in the component. To keep the codebase clean and optimize bundle size, it's best to remove unused imports.Apply this diff to remove the unused import:
-import { CheckCheck, Loader2 } from "lucide-react"; +import { Loader2 } from "lucide-react";Likely invalid or redundant comment.
| const handleNewEmailUpdate = async () => { | ||
| setLoading(true); | ||
| await updateEmail( | ||
| { newEmail }, | ||
| { | ||
| onError(error) { | ||
| setLoading(false); | ||
| if (error) return toast.error(error.message); | ||
| return toast.error( | ||
| "Something went wrong sending the verification link.", | ||
| ); | ||
| }, | ||
| onSuccess() { | ||
| setLoading(false); | ||
| toast.success("Verification link sent to your email."); | ||
| setSendForVerification(true); | ||
| }, | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in handleNewEmailUpdate.
The current error handling in handleNewEmailUpdate is generic. Consider providing more specific error messages based on the error type or code returned by the API.
Here's a suggested improvement:
const handleNewEmailUpdate = async () => {
setLoading(true);
await updateEmail(
{ newEmail },
{
onError(error: any) {
setLoading(false);
if (error.data?.code === "INVALID_EMAIL") {
return toast.error("The provided email is invalid. Please check and try again.");
} else if (error.data?.code === "EMAIL_IN_USE") {
return toast.error("This email is already in use. Please use a different email.");
}
return toast.error(
"Something went wrong sending the verification link.",
);
},
onSuccess() {
setLoading(false);
toast.success("Verification link sent to your email.");
setSendForVerification(true);
},
},
);
};This change provides more specific error messages based on potential API error codes, improving the user experience.
|
Your |
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This could be a breaking change (I think it's one too many) would you unstage this file (I think Praveen on Discord left the command).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NiallJoeMaher
left a comment
NiallJoeMaher
left a comment
There was a problem hiding this comment.
LGTM! Great work! 🌮 🦖
2 participants
✨ Codu Pull Request 💻
Fixes #1049
Pull Request details
Any Breaking changes
Associated Screenshots