feat: Add admin ability to mark user as email verified #2137
Conversation
Added the ability for admins to mark/unmark users as email verified: - Updated updateUser tRPC endpoint to accept emailVerified boolean - Modified endpoint to set emailVerified timestamp when true, null when false - Added emailVerified field to users.list output schema - Added Switch component to UpdateUserDialog for toggling email verification - Updated UserList to pass emailVerified status to edit modal
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds email verification state to admin user management: UI toggle in UpdateUserDialog, prop passing from UserList, API schema/type additions, router handling to persist emailVerified (timestamp or null), and OpenAPI request update. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/components/admin/UpdateUserDialog.tsx (1)
93-97: Consider addingdefaultValuesto dependency array or recalculating inside effect.The
defaultValuesobject (defined at lines 59-65) is used inside this effect but not included in the dependency array. While this follows the existing pattern for other fields, it could lead to the form resetting with stale prop values if props change while the dialog is closed.This would ensure the form always resets with current prop values:
useEffect(() => { if (isOpen) { - form.reset(defaultValues); + form.reset({ + userId, + role: currentRole, + bookmarkQuota: currentQuota, + storageQuota: currentStorageQuota, + emailVerified: currentEmailVerified !== null, + }); } -}, [isOpen, form]); +}, [isOpen, form, userId, currentRole, currentQuota, currentStorageQuota, currentEmailVerified]);Note: This is an existing pattern issue affecting all fields, not introduced by this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/components/admin/UpdateUserDialog.tsx(4 hunks)apps/web/components/admin/UserList.tsx(1 hunks)packages/shared/types/admin.ts(1 hunks)packages/trpc/routers/admin.ts(1 hunks)packages/trpc/routers/users.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: typecheck
- GitHub Check: tests
- GitHub Check: format
🔇 Additional comments (7)
packages/shared/types/admin.ts (1)
17-17: LGTM!The schema correctly defines
emailVerifiedas an optional boolean field, consistent with the UI toggle control and backend logic.packages/trpc/routers/users.ts (1)
74-74: LGTM!The output schema correctly defines
emailVerifiedas a nullable number (timestamp), matching the database storage format whereDate.now()is stored for verified users andnullfor unverified users.apps/web/components/admin/UserList.tsx (1)
148-148: LGTM!The prop correctly passes the
emailVerifiedvalue to theUpdateUserDialog, following the established naming convention of othercurrent*props.apps/web/components/admin/UpdateUserDialog.tsx (4)
29-29: LGTM!Appropriate import of the Switch component for the email verification toggle.
46-46: LGTM!The props interface and function signature are correctly extended to accept
currentEmailVerifiedwith the proper type (number | null), matching the API response shape.Also applies to: 54-54
64-64: LGTM!The conversion from nullable timestamp to boolean (
currentEmailVerified !== null) correctly represents verification state for form initialization.
180-196: LGTM!The FormField implementation for email verification is well-structured with proper form control binding and appropriate use of the Switch component for boolean toggle.
| if (input.emailVerified !== undefined) { | ||
| updateData.emailVerified = input.emailVerified ? Date.now() : null; | ||
| } |
There was a problem hiding this comment.
Timestamp is overwritten unnecessarily, losing verification history.
When input.emailVerified is true, the code always sets a new timestamp via Date.now(), even if the user was already verified. This loses the original verification timestamp whenever an admin updates the user (even when not intending to change email verification status).
Consider one of these alternatives:
- Preserve existing timestamp when already verified:
if (input.emailVerified !== undefined) {
- updateData.emailVerified = input.emailVerified ? Date.now() : null;
+ if (input.emailVerified) {
+ // Only set new timestamp if not already verified
+ const user = await ctx.db.query.users.findFirst({
+ where: eq(users.id, input.userId),
+ columns: { emailVerified: true }
+ });
+ updateData.emailVerified = user?.emailVerified ?? Date.now();
+ } else {
+ updateData.emailVerified = null;
+ }
}- Or track form changes to only submit modified fields (requires UpdateUserDialog.tsx changes to detect which fields were actually changed)
This preserves audit trails and original verification dates, which may be important for compliance or business logic.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/trpc/routers/admin.ts around lines 372-374, the code unconditionally
overwrites the emailVerified timestamp with Date.now() whenever
input.emailVerified is true, losing the original verification timestamp; change
the logic so you only set Date.now() when transitioning from unverified to
verified (i.e., input.emailVerified === true AND the existing user.emailVerified
is falsy), preserve the existing timestamp if the user was already verified, and
when input.emailVerified is false set it to null; to implement this either load
the current user record before applying updateData or make the update call
conditional based on the previous value so you don't overwrite a pre-existing
timestamp.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| localUser: z.boolean(), | ||
| bookmarkQuota: z.number().nullable(), | ||
| storageQuota: z.number().nullable(), | ||
| emailVerified: z.number().nullable(), | ||
| }), | ||
| ), | ||
| }), |
There was a problem hiding this comment.
The new users.list output schema declares emailVerified as z.number().nullable() (packages/trpc/routers/users.ts lines 62‑74), but the query returns users.map((u) => u.asPublicUser()) where asPublicUser exposes the raw users.emailVerified column. That column is defined as a timestamp_ms in packages/db/schema.ts, so Drizzle returns a Date | null, and the router is configured with superjson to serialize Dates. Because the procedure has an .output() parser, Zod will now try to coerce a Date into a number and throw whenever any user actually has a verified email, breaking the entire admin users list for normal deployments. The output schema either needs to change to z.date().nullable() or the resolver needs to convert the Date into a number before returning it.
Useful? React with 👍 / 👎.
MohamedBassem
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/trpc/routers/admin.ts (1)
372-374: Timestamp overwrite issue remains unaddressed.The concern raised in the previous review is still present: setting
new Date()unconditionally wheninput.emailVerifiedistrueoverwrites the original verification timestamp, losing audit history. Consider preserving the existing timestamp if the user is already verified.
🧹 Nitpick comments (1)
packages/open-api/karakeep-openapi-spec.json (1)
572-573: Consider adding a description for clarity.The
emailVerifiedfield lacks a description. Adding one would improve API discoverability and help consumers understand that this boolean controls whether the user's email is marked as verified.Example:
"emailVerified": { + "description": "Set to true to mark the user's email as verified, or false to unmark it", "type": "boolean" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/components/admin/UpdateUserDialog.tsx(4 hunks)packages/open-api/karakeep-openapi-spec.json(1 hunks)packages/trpc/routers/admin.ts(1 hunks)packages/trpc/routers/users.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/trpc/routers/users.ts
- apps/web/components/admin/UpdateUserDialog.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests
- GitHub Check: typecheck
Added the ability for admins to mark/unmark users as email verified: