Settings screen update / Fix Drizzle Kit Studio #1109
Conversation
Also optimizes some indexes for fetching data and then also fixes schema to get drizzlekit working
|
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces extensive modifications to the email management system and the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (6)
drizzle/0010_email-tokens-and-indexes.sql (2)
1-9: LGTM! Consider adding column length limits.The
EmailChangeHistorytable structure is well-designed for tracking email changes, including useful fields for security auditing. However, consider adding length limits to thetextfields (e.g.,userId,oldEmail,newEmail,ipAddress,userAgent) to prevent potential storage issues.Example of adding a length limit:
"userId" varchar(255) NOT NULL,
11-19: LGTM! Consider adding column length limits and an index on userId.The
EmailChangeRequesttable is well-structured for managing email change requests. The unique constraint on thetokenand the inclusion of an expiration time are good security practices.Consider the following improvements:
- Add length limits to the
textfields to prevent potential storage issues:"userId" varchar(255) NOT NULL, "newEmail" varchar(255) NOT NULL, "token" varchar(64) NOT NULL,- Add an index on the
userIdcolumn to improve query performance when looking up requests for a specific user:CREATE INDEX IF NOT EXISTS "EmailChangeRequest_userId_index" ON "EmailChangeRequest" USING btree ("userId");server/api/router/profile.ts (3)
1-1: Organize Import Statements for ClarityConsider grouping and ordering the import statements for better readability. Group related imports together, and separate third-party modules from local modules.
Apply this diff to improve import organization:
+import { z } from "zod"; +import { TRPCError } from "@trpc/server"; +import { nanoid } from "nanoid"; +import { and, eq, gte } from "drizzle-orm"; import { user, emailChangeHistory } from "@/server/db/schema"; +import { emailChangeRequest } from "@/server/db/schema"; import { createTRPCRouter, publicProcedure, protectedProcedure, } from "../trpc"; import { isUserSubscribedToNewsletter, manageNewsletterSubscription, } from "@/server/lib/newsletter"; import { getPresignedUrl } from "@/server/common/getPresignedUrl"; import { generateEmailToken, sendVerificationEmail } from "@/utils/emailToken"; import { saveSettingsSchema, getProfileSchema, uploadPhotoUrlSchema, updateProfilePhotoUrlSchema, } from "@/schema/profile"; import { emailTokenReqSchema } from "@/schema/token"; import { TOKEN_EXPIRATION_TIME } from "@/config/constants";
20-20: Remove Unused Import:generateEmailTokenThe function
generateEmailTokenseems to be imported but not used in this file. Removing unused imports can help keep the codebase clean.Apply this diff to remove the unused import:
-import { generateEmailToken, sendVerificationEmail } from "@/utils/emailToken"; +import { sendVerificationEmail } from "@/utils/emailToken";
190-196: Enhance Error Logging for Failed Email DispatchWhen logging errors, include more context to aid debugging, such as the user's ID or email. Ensure sensitive information is not logged.
Apply this diff to improve error logging:
try { await sendVerificationEmail(newEmail, token); } catch (error) { - console.error("Failed to send verification email:", error); + console.error(`Failed to send verification email to ${newEmail}:`, error); throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to send verification email", }); }app/(app)/settings/_client.tsx (1)
85-86: Consider enhancing email validation with a more robust methodThe current regular expression for email validation is simple and may not catch all invalid email formats. For improved accuracy, consider using a more comprehensive validation method or an established library like
validator.js.For example, using the
validatorlibrary:+import isEmail from 'validator/lib/isEmail'; -const isValidEmail = (email: string) => /\S+@\S+\.\S+/.test(email); +const isValidEmail = (email: string) => isEmail(email);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
drizzle/meta/0010_snapshot.jsonis excluded by!**/*.jsondrizzle/meta/_journal.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (5)
- app/(app)/settings/_client.tsx (7 hunks)
- app/api/verify-email/route.ts (1 hunks)
- drizzle/0010_email-tokens-and-indexes.sql (1 hunks)
- server/api/router/profile.ts (3 hunks)
- server/db/schema.ts (7 hunks)
🧰 Additional context used
🔇 Additional comments (18)
drizzle/0010_email-tokens-and-indexes.sql (3)
23-33: LGTM! Note the CASCADE delete behavior.The foreign key constraints are correctly implemented, ensuring referential integrity between the new tables and the
usertable. The exception handling for duplicate constraints is a good practice for migration scripts.Please confirm that the CASCADE delete behavior is intended. This means that when a user is deleted, all their email change history and requests will also be deleted. If this is not the desired behavior, consider using SET NULL or RESTRICT instead.
#!/bin/bash # Verify the foreign key constraint details echo "Checking foreign key constraints for EmailChangeHistory:" psql -c "SELECT conname, confdeltype FROM pg_constraint WHERE conname = 'EmailChangeHistory_userId_user_id_fk';" echo "Checking foreign key constraints for EmailChangeRequest:" psql -c "SELECT conname, confdeltype FROM pg_constraint WHERE conname = 'EmailChangeRequest_userId_user_id_fk';" # Output explanation: # confdeltype: a = no action, r = restrict, c = cascade, n = set null, d = set default
35-39: LGTM! Verify impact on write performance.The addition of these indexes is a good optimization for read performance on frequently queried columns. The use of
IF NOT EXISTSensures idempotency, which is excellent for migration scripts.While these indexes will improve read performance, they may have an impact on write performance. Please verify that this trade-off is acceptable for your use case. Consider running performance tests to ensure that write-heavy operations are not significantly impacted.
#!/bin/bash # Verify the existing indexes and their size echo "Checking indexes on Comment table:" psql -c "SELECT indexname, pg_size_pretty(pg_relation_size(indexname::regclass)) AS index_size FROM pg_indexes WHERE tablename = 'Comment';" echo "Checking indexes on Notification table:" psql -c "SELECT indexname, pg_size_pretty(pg_relation_size(indexname::regclass)) AS index_size FROM pg_indexes WHERE tablename = 'Notification';" echo "Checking indexes on Post table:" psql -c "SELECT indexname, pg_size_pretty(pg_relation_size(indexname::regclass)) AS index_size FROM pg_indexes WHERE tablename = 'Post';" echo "Checking indexes on user table:" psql -c "SELECT indexname, pg_size_pretty(pg_relation_size(indexname::regclass)) AS index_size FROM pg_indexes WHERE tablename = 'user';" # This will help you understand the current state of indexes and their sizes
21-22: Verify intentional removal of tables and consider data migration.The removal of
EmailVerificationTokenandverificationTokentables suggests a significant change in the email verification process. While this is consistent with the new email-related tables, it's important to ensure that no critical data is being lost.Please confirm:
- Is the removal of these tables intentional?
- Has any important data from these tables been migrated to the new structure?
If data migration is needed, consider running a script to transfer relevant data before dropping the tables.
app/api/verify-email/route.ts (1)
24-29:⚠️ Potential issueReconsider authentication requirement for email verification
Currently, the email verification process requires the user to be authenticated (
sessionandsession.usermust exist). This might cause issues if a user attempts to verify their email without being logged in, which is a common scenario. Consider modifying the verification logic to allow unauthenticated users to verify their email addresses.⛔ Skipped due to learnings
Learnt from: NiallJoeMaher PR: codu-code/codu#1069 File: app/verify_email/_client.tsx:36-42 Timestamp: 2024-10-06T07:41:04.094Z Learning: In the email verification process in `app/verify_email/_client.tsx`, we use a GET route that automatically validates the token when the user clicks the email link, instead of making a POST request. This ensures that the user clicked their email.server/api/router/profile.ts (1)
178-179: Use Secure Token GenerationEnsure that the token generation method produces cryptographically secure tokens. Using
nanoidwith a secure random generator is recommended.[security]
Run the following script to verify the token generation method:
app/(app)/settings/_client.tsx (1)
96-101:⚠️ Potential issueInclude 'cooldown' in useEffect dependency array to ensure proper countdown
The
useEffecthook responsible for the cooldown timer at lines 96-101 is missing a dependency array. Without including[cooldown]as a dependency, the effect might not re-run whencooldownchanges, leading to the countdown not functioning as intended.Apply the following change to include
cooldownin the dependency array:useEffect(() => { if (cooldown > 0) { const timer = setTimeout(() => setCooldown(cooldown - 1), 1000); return () => clearTimeout(timer); } - }); + }, [cooldown]);Likely invalid or redundant comment.
server/db/schema.ts (12)
31-38: AddedsessionRelationsfor thesessiontableThe new relation correctly associates sessions with users, ensuring referential integrity and simplifying queries involving session data.
63-66: AddedaccountRelationsfor theaccounttableThis addition establishes a clear relationship between accounts and users, which is essential for account management and authentication processes.
158-159: Added indexes onsluganduserIdinPosttable to enhance query performanceThe new indexes
slugIndexanduserIdIndexwill improve the efficiency of queries filtering posts bysluganduserId, respectively.
227-227: Added index onusernameinUsertable for faster lookupsIntroducing
usernameIndexoptimizes queries involving usernames, benefiting user authentication and profile retrieval operations.
244-249: AddednotificationsCreatedandnotificationsReceivedrelations toUserThese relations enhance the notification system by clearly distinguishing between notifications sent by the user (
notificationsCreated) and those received (notificationsReceived).
251-253: Addedsessionsand email change relations toUserIncluding
sessions,emailChangeRequests, andemailChangeHistoryrelations allows for comprehensive tracking of user sessions and email change activities, improving account security and management.
319-319: Added index onpostIdinCommenttable to improve query speedThe
postIdIndexwill enhance performance when retrieving comments associated with a specific post.
466-476: Updated relation names inflaggedRelationsfor better readabilityRenaming relations to
flaggedByUserandflaggedContentenhances clarity, making it easier to understand the relationships between users and flagged items.
479-538: AddednotifierIdfield and relations toNotificationtableIncluding
notifierIdenables the tracking of which user generated a notification, enhancing the traceability and functionality of the notification system.
540-559: ImplementedEmailChangeRequesttable and relationsThe new
EmailChangeRequesttable securely manages pending email change requests, storing tokens and expiration dates to prevent abuse and enhance user security.
561-577: ImplementedEmailChangeHistorytable and relationsThe
EmailChangeHistorytable records email changes, providing an audit trail that improves account security and aids in troubleshooting any issues related to email updates.
233-235:⚠️ Potential issueVerify the correctness of the
bannedUsersrelation inuserRelationsThe
bannedUsersrelation is defined as aonerelation:bannedUsers: one(banned_users, { fields: [user.id], references: [banned_users.userId], }),If a user can have multiple ban records, consider changing this to a
manyrelation to accurately represent the one-to-many relationship between users and their ban records.
| .update(user) | ||
| .set({ | ||
| email: request.newEmail, | ||
| emailVerified: new Date().toISOString(), |
There was a problem hiding this comment.
Ensure correct data type for emailVerified field
The emailVerified field is being set using new Date().toISOString(), which returns a string. If the database schema expects a Date object, consider passing new Date() instead to ensure type consistency.
Apply this diff to fix the issue:
- emailVerified: new Date().toISOString(),
+ emailVerified: new Date(),📝 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.
| emailVerified: new Date().toISOString(), | |
| emailVerified: new Date(), |
| userId, | ||
| oldEmail: currentUser.email ?? "", | ||
| newEmail: request.newEmail, | ||
| ipAddress: req.headers.get("x-forwarded-for")?.split(",")[0].trim() ?? "", |
There was a problem hiding this comment.
💡 Codebase verification
IP Address Extraction Validation Needs Improvement
The current implementation directly accesses the x-forwarded-for header without ensuring its presence or handling potential malformed values. Additionally, there is no indication of using trusted proxies or middleware to obtain the client's IP address reliably.
- File:
app/api/verify-email/route.tsat line 74
🔗 Analysis chain
Validate client IP address extraction
When extracting the client's IP address from the x-forwarded-for header, you're assuming the header is present and contains a comma-separated list of IPs. To enhance reliability:
- Ensure the header exists before splitting and trimming.
- Consider using a trusted proxy or middleware to obtain the client's IP.
- Handle scenarios where the header might be missing or malformed.
You can verify the presence of the header with the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of 'x-forwarded-for' header in codebase
# Test: Search for instances where 'x-forwarded-for' is used
rg --type typescript '"x-forwarded-for"'
Length of output: 77
Script:
#!/bin/bash
# Description: Check for usage of 'x-forwarded-for' header in TypeScript codebase
# Test: Search for instances where 'x-forwarded-for' is used
rg --type ts '"x-forwarded-for"'
Length of output: 145
Script:
#!/bin/bash
# Description: Check for trusted proxy or middleware configurations handling client IP extraction in TypeScript codebase
# Test: Search for usage of 'trust proxy' or related middleware configurations
rg --type ts 'trust proxy|middleware'
Length of output: 589
| const { newEmail } = input; | ||
| const userId = ctx.session.user.id; | ||
|
|
||
| if (!newEmail) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid request", | ||
| }); | ||
| } | ||
| if (!newEmail) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid request", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Simplify Null or Undefined Checks
The check for !newEmail can be more precise. If newEmail is expected to be a string, consider validating its format using zod schemas or regular expressions.
Apply this diff to enhance email validation:
- if (!newEmail) {
+ if (!newEmail || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(newEmail)) {
throw new TRPCError({
code: "BAD_REQUEST",
- message: "Invalid request",
+ message: "Invalid email address",
});
}📝 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 { newEmail } = input; | |
| const userId = ctx.session.user.id; | |
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| if (!newEmail) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid request", | |
| }); | |
| } | |
| const { newEmail } = input; | |
| const userId = ctx.session.user.id; | |
| if (!newEmail || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(newEmail)) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Invalid email address", | |
| }); | |
| } |
| const existingUser = await ctx.db.query.user.findFirst({ | ||
| where: eq(user.email, newEmail), | ||
| }); | ||
|
|
||
| if (ifEmailExists) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Email already exists", | ||
| }); | ||
| } | ||
| if (existingUser) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Email already in use", | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Prevent Timing Attacks in User Existence Checks
When checking if the new email is already in use, the error message reveals whether an email is registered in the system. This can be a security concern, as it might allow attackers to validate email addresses. Consider returning a generic error message instead.
[security]
Apply this diff to return a generic error:
if (existingUser) {
throw new TRPCError({
code: "BAD_REQUEST",
- message: "Email already in use",
+ message: "Unable to process the request",
});
}📝 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.
| // Check if the new email is already in use | |
| const existingUser = await ctx.db.query.user.findFirst({ | |
| where: eq(user.email, newEmail), | |
| }); | |
| if (ifEmailExists) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email already exists", | |
| }); | |
| } | |
| if (existingUser) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Email already in use", | |
| }); | |
| } | |
| // Check if the new email is already in use | |
| const existingUser = await ctx.db.query.user.findFirst({ | |
| where: eq(user.email, newEmail), | |
| }); | |
| if (existingUser) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "Unable to process the request", | |
| }); | |
| } |
| userId, | ||
| newEmail, | ||
| token, | ||
| expiresAt, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Handle Possible Database Insertion Errors
After inserting the new email change request, it's good practice to handle any potential errors or confirm that the insertion was successful.
Apply this diff to handle insertion results:
await ctx.db.insert(emailChangeRequest).values({
userId,
newEmail,
token,
expiresAt,
});
+ // Optionally check if the insertion was successful
+ // If needed, include error handling logic hereCommittable suggestion was skipped due to low confidence.
| const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({ | ||
| where: and( | ||
| eq(emailChangeRequest.userId, userId), | ||
| gte( | ||
| emailChangeRequest.createdAt, | ||
| new Date(Date.now() - 5 * 60 * 1000), | ||
| ), // 5 minutes | ||
| ), | ||
| }); | ||
|
|
||
| if (recentRequest) { | ||
| throw new TRPCError({ | ||
| code: "TOO_MANY_REQUESTS", | ||
| message: "Please wait before requesting another email change", | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Rate Limiting Logic
The rate-limiting logic could be simplified by using a database constraint or by moving the time calculation into the database query for better performance.
Apply this diff to optimize rate limiting:
- const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({
- where: and(
- eq(emailChangeRequest.userId, userId),
- gte(
- emailChangeRequest.createdAt,
- new Date(Date.now() - 5 * 60 * 1000),
- ), // 5 minutes
- ),
- });
+ const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000);
+ const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({
+ where: and(
+ eq(emailChangeRequest.userId, userId),
+ gte(emailChangeRequest.createdAt, fiveMinutesAgo), // 5 minutes
+ ),
+ });Alternatively, consider adding a unique index on (userId, newEmail) with a condition on createdAt to enforce rate limiting at the database level.
📝 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.
| // Rate limiting: Check for recent requests | |
| const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({ | |
| where: and( | |
| eq(emailChangeRequest.userId, userId), | |
| gte( | |
| emailChangeRequest.createdAt, | |
| new Date(Date.now() - 5 * 60 * 1000), | |
| ), // 5 minutes | |
| ), | |
| }); | |
| if (recentRequest) { | |
| throw new TRPCError({ | |
| code: "TOO_MANY_REQUESTS", | |
| message: "Please wait before requesting another email change", | |
| }); | |
| } | |
| // Rate limiting: Check for recent requests | |
| const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000); | |
| const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({ | |
| where: and( | |
| eq(emailChangeRequest.userId, userId), | |
| gte(emailChangeRequest.createdAt, fiveMinutesAgo), // 5 minutes | |
| ), | |
| }); | |
| if (recentRequest) { | |
| throw new TRPCError({ | |
| code: "TOO_MANY_REQUESTS", | |
| message: "Please wait before requesting another email change", | |
| }); | |
| } |
| onChange={(e) => { | ||
| setNewEmail(e.target.value); | ||
| if (sendForVerification) { | ||
| setEmailError(""); // Clear error when user starts typing again | ||
| } | ||
| }} |
There was a problem hiding this comment.
Clear email error whenever the user modifies the email input
Currently, emailError is cleared only when sendForVerification is true. To enhance user experience and provide immediate feedback, consider clearing emailError whenever the user changes the newEmail input, regardless of sendForVerification.
Update the onChange handler to remove the condition:
onChange={(e) => {
setNewEmail(e.target.value);
- if (sendForVerification) {
setEmailError(""); // Clear error when user starts typing again
- }
}}📝 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.
| onChange={(e) => { | |
| setNewEmail(e.target.value); | |
| if (sendForVerification) { | |
| setEmailError(""); // Clear error when user starts typing again | |
| } | |
| }} | |
| onChange={(e) => { | |
| setNewEmail(e.target.value); | |
| setEmailError(""); // Clear error when user starts typing again | |
| }} |
| {emailError && sendForVerification && ( | ||
| <ErrorMessage>{emailError}</ErrorMessage> | ||
| )} |
There was a problem hiding this comment.
Display email error messages regardless of verification status
The error message for the email input is shown only when both emailError and sendForVerification are true. To ensure users are promptly aware of input errors, display the error message whenever emailError is set, regardless of sendForVerification.
Modify the condition to display the error message based solely on emailError:
-{emailError && sendForVerification && (
+{emailError && (
<ErrorMessage>{emailError}</ErrorMessage>
)}📝 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.
| {emailError && sendForVerification && ( | |
| <ErrorMessage>{emailError}</ErrorMessage> | |
| )} | |
| {emailError && ( | |
| <ErrorMessage>{emailError}</ErrorMessage> | |
| )} |
| export const banned_usersRelations = relations(banned_users, ({ one }) => ({ | ||
| bannedBy: one(user, { | ||
| fields: [banned_users.bannedById], | ||
| references: [user.id], | ||
| relationName: "bans", | ||
| }), | ||
| ); | ||
| user: one(user, { | ||
| fields: [banned_users.userId], | ||
| references: [user.id], | ||
| }), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming relations in banned_usersRelations for clarity
The use of relationName: "bans" for the bannedBy relation may cause confusion due to its similarity with the bans relation in userRelations. To avoid ambiguity, consider more descriptive names such as bannedByUser or banInitiator.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
server/api/router/profile.ts (3)
137-157: Improved input validation and security, consider enhancing email format checkThe changes improve input validation and security by checking for the existence of
newEmailand using a generic error message when an email is already in use. This aligns with the PR objectives of refactoring the email verification system.However, consider implementing a more precise email format check as suggested in a previous review:
- if (!newEmail) { + if (!newEmail || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(newEmail)) { throw new TRPCError({ code: "BAD_REQUEST", - message: "Invalid request", + message: "Invalid email address", }); }This change would provide more specific feedback to users and prevent invalid email formats from being processed further.
159-173: Effective rate limiting implemented, consider optimizationThe implementation of rate limiting to prevent frequent email change requests within a two-minute window is a good addition. It aligns with the PR objectives of preventing spamming and avoiding locking up email accounts.
However, consider optimizing the rate limiting logic as suggested in a previous review:
- const twoMinutesAgo = new Date(Date.now() - 2 * 60 * 1000); - const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({ - where: and( - eq(emailChangeRequest.userId, userId), - gte(emailChangeRequest.createdAt, twoMinutesAgo), // 2 minutes - ), - }); + const recentRequest = await ctx.db.query.emailChangeRequest.findFirst({ + where: and( + eq(emailChangeRequest.userId, userId), + gte(emailChangeRequest.createdAt, new Date(Date.now() - 2 * 60 * 1000)), + ), + });This change would simplify the code and potentially improve performance by moving the time calculation into the database query.
175-185: Token generation and database insertion implemented, consider error handlingThe implementation of token generation, expiration date calculation, and insertion of the new email change request into the database is correct and aligns with the PR objectives.
However, consider adding error handling for the database insertion as suggested in a previous review:
await ctx.db.insert(emailChangeRequest).values({ userId, newEmail, token, expiresAt, }); + // Handle potential insertion errors + .catch((error) => { + console.error("Failed to insert email change request:", error); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to process email change request", + }); + });This addition would improve the robustness of the email change process by explicitly handling potential database errors.
app/(app)/settings/_client.tsx (4)
53-53: LGTM: Improved form handling and email verificationThe changes to the form state handling, including the addition of
isSubmittingandcooldown, improve the overall functionality and align with the PR objective of refactoring email verification to prevent spamming. The cooldown mechanism is a good addition to prevent excessive requests.Consider extracting the email validation regex to a constant or utility function for better maintainability:
const EMAIL_REGEX = /\S+@\S+\.\S+/; const isValidEmail = (email: string) => EMAIL_REGEX.test(email);This would make it easier to update the email validation logic in the future if needed.
Also applies to: 70-70, 77-77, 85-86, 96-102
240-344: LGTM: Improved layout and consistency for personal information sectionsThe redesigned personal information sections (name, username, bio, location, website) now use a consistent grid layout and new UI components, resulting in better visual hierarchy and improved user experience. This change aligns well with the PR objectives of enhancing the settings screen and making it responsive.
For consistency, consider adding a
placeholderprop to thenameandusernameinputs, similar to thelocationandwebsiteUrlinputs:<Input id="name" type="text" placeholder="Your full name" autoComplete="given-name" invalid={!!errors?.name} {...register("name")} /> <Input id="username" type="text" placeholder="Your username" autoComplete="username" invalid={!!errors?.username} {...register("username")} />This would provide a consistent user experience across all input fields.
348-398: LGTM with suggestions: Improved email update sectionThe redesigned email update section with separate current and new email inputs, along with the cooldown mechanism, aligns well with the PR objective of refactoring email verification to prevent spamming. The improved error handling enhances the user experience.
However, there are a few suggestions for improvement:
Consider using a more descriptive variable name instead of
sendForVerification, such asisVerificationSentorisAwaitingVerification.The error message display condition can be simplified:
{emailError && <ErrorMessage>{emailError}</ErrorMessage>}
- The button text could be more consistent:
{loading ? "Sending..." : cooldown > 0 ? `Wait ${cooldown}s` : "Send Verification Email"}
- Consider adding a success message when the verification email is sent successfully.
These changes would further improve the user experience and code readability.
Line range hint
1-444: Great job on the settings screen redesign!The extensive refactoring of the Settings component has resulted in a significantly improved user interface that aligns well with the PR objectives. Key improvements include:
- Consistent use of new UI components throughout the form.
- Improved layout with a responsive grid structure.
- Enhanced visual hierarchy and spacing.
- Better error handling and user feedback.
- Implementation of a cooldown mechanism for email verification.
These changes collectively contribute to a more organized, visually appealing, and user-friendly settings screen.
For future enhancements, consider implementing form state persistence (e.g., using
localStorage) to prevent loss of user input if they accidentally navigate away from the page. This could further improve the user experience, especially for longer forms.Overall, excellent work on improving both the functionality and aesthetics of the settings screen!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/(app)/settings/_client.tsx (7 hunks)
- server/api/router/profile.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
server/api/router/profile.ts (3)
1-1: LGTM: Import changes align with new functionalityThe new imports for
emailChangeHistory,and,gte,emailChangeRequest, andzare appropriate for the email change functionality and database operations. The modified import forgenerateEmailTokenandsendVerificationEmailalso aligns with the refactoring of the email verification system.Also applies to: 18-18, 20-20, 22-23
187-198: LGTM: Robust email sending implementationThe implementation of sending the verification email, including the try-catch block and error handling, is well done. It aligns with the PR objectives of refactoring the email verification system and improves the robustness of the process. The error logging will help with debugging in case of email sending failures.
137-198: Overall: Solid implementation meeting PR objectivesThe changes to the
updateEmailprocedure inserver/api/router/profile.tssuccessfully address the PR objectives:
- The email verification system has been refactored to prevent spamming and avoid locking up email accounts.
- Input validation, rate limiting, and error handling have been implemented to enhance security and robustness.
- The changes support the overall goal of updating the settings screen and fixing issues related to the Drizzle Kit Studio.
The suggested improvements (email format check, rate limiting optimization, and database insertion error handling) are minor and aimed at further enhancing the code quality. Overall, this is a well-implemented feature that significantly improves the email change process.
app/(app)/settings/_client.tsx (4)
Line range hint
15-27: LGTM: Improved imports for enhanced UI componentsThe addition of new UI component imports, such as
Loader2,Subheading,Divider, andText, indicates an improved and more organized UI structure. This change aligns well with the PR objective of enhancing the settings screen.
196-237: LGTM: Improved profile picture upload sectionThe redesigned profile picture upload section uses new UI components, resulting in a more visually appealing and user-friendly interface. This change aligns well with the PR objective of enhancing the settings screen. The functionality remains intact while improving the overall user experience.
400-432: LGTM: Improved notification and newsletter settingsThe redesigned notification and newsletter settings using the
Switchcomponent provide a more intuitive and visually appealing interface. The improved layout with better spacing and visual hierarchy enhances the overall user experience. This change aligns well with the PR objective of enhancing the settings screen.
434-444: LGTM: Improved form submission and reset buttonsThe redesigned and repositioned form submission and reset buttons provide a better user experience. Disabling the "Save Changes" button during form submission prevents multiple submissions and provides clear visual feedback to the user. The positioning of these buttons at the end of the form follows common UI patterns, making it intuitive for users.
2 participants
A few fun changes here.