Conversation
- Added `is_enabled` and `expires_at` fields to the users table to manage user activation status and expiration dates. - Updated user-related actions and forms to handle the new fields, including enabling/disabling users and renewing their expiration dates. - Enhanced user interface components to display user status and expiration information. - Implemented backend logic to mark users as expired and handle authentication based on user status. This update improves user management capabilities and enhances the overall user experience in the dashboard.
- Introduced a new function `validateExpiresAt` to validate expiration dates, ensuring they are in the future and do not exceed 10 years. - Updated user-related actions to incorporate expiration date validation, returning appropriate error messages when validation fails. - Enhanced user forms to handle expiration dates correctly, converting input dates to the end of the day to avoid premature expiration. - Added new error messages in both English and Chinese for better user feedback regarding expiration date issues. This update improves the robustness of user management by enforcing expiration date rules and enhancing user experience with clear error messaging.
Summary of ChangesHello @NightYuYyy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces user expiration functionality, allowing administrators to set expiration dates for user accounts. It includes database schema changes, server-side and client-side validation, UI enhancements, and authentication guard updates to enforce expiration policies. The goal is to improve user management and security by automatically disabling expired accounts. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ding113
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR implements user expiration functionality, adding is_enabled and expires_at fields to users along with renewal and status management features. The implementation is comprehensive but has some issues that should be addressed.
PR Size: L
- Lines changed: 2753 (2721 additions + 32 deletions)
- Files changed: 25
Split suggestion: This is a large PR. Consider splitting into:
- Database migration + schema changes
- Backend user actions and repository changes
- Frontend UI components (forms, lists, dialogs)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Code Quality | 0 | 1 | 2 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Dependencies | 0 | 0 | 0 | 0 |
| Documentation | 0 | 0 | 0 | 0 |
Priority Actions
-
[High] Hardcoded Chinese strings in key-list-header.tsx (line 118-130): The
userStatusInfouseMemo uses hardcoded Chinese strings ("已禁用","已过期", etc.) for badge text instead of using the i18n translation system. This breaks internationalization - users with non-Chinese locale will still see Chinese text.Location:
src/app/[locale]/dashboard/_components/user/key-list-header.tsx:118-130Fix: Use the translation system:
const tList = useTranslations("dashboard.userList"); // ... if (!activeUser.isEnabled) { status = { code: "disabled", badge: tList("status.disabled"), variant: "secondary" }; } else if (exp && exp <= now) { status = { code: "expired", badge: tList("status.expired"), variant: "destructive" }; } // etc.
-
[Medium] Another hardcoded Chinese string (line 143): The expiry label
"过期时间:"in the UI is hardcoded instead of using translations.Location:
src/app/[locale]/dashboard/_components/user/key-list-header.tsx:264Fix:
<span>{tList("expiresAt")}:</span>
-
[Medium] Missing user type update for
expiresAtfield: TheUserinterface insrc/types/user.tswas not updated to include the newisEnabledandexpiresAtfields, though they are used in the codebase viaUserDisplay.Location:
src/types/user.tsFix: Add to User interface:
export interface User { // ... existing fields isEnabled: boolean; expiresAt: Date | null; }
Review Coverage
- Code quality and correctness
- Security (OWASP Top 10) - No security issues found
- PR size assessment - L (Large)
- Dependency changes - No dependency changes
- Documentation changes - No documentation changes (i18n strings added properly for most translations)
Automated review by Claude AI
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive user expiration feature, which is a valuable addition for user management. The changes span the database, backend logic, and frontend UI, and are well-implemented. Key additions include a validateExpiresAt function for robust date validation, updates to user-related server actions to handle expiration and status, and a lazy expiration check in the authentication guard for better performance. The UI has also been enhanced to display user status and provide renewal actions. I have one high-severity suggestion to address an internationalization bug and code duplication in the UI.
| // 获取用户状态和过期信息 | ||
| const userStatusInfo = useMemo(() => { | ||
| if (!activeUser) return null; | ||
|
|
||
| const now = Date.now(); | ||
| const exp = activeUser.expiresAt ? new Date(activeUser.expiresAt).getTime() : null; | ||
|
|
||
| let status: { | ||
| code: string; | ||
| badge: string; | ||
| variant: "default" | "secondary" | "destructive" | "outline"; | ||
| }; | ||
|
|
||
| if (!activeUser.isEnabled) { | ||
| status = { code: "disabled", badge: "已禁用", variant: "secondary" }; | ||
| } else if (exp && exp <= now) { | ||
| status = { code: "expired", badge: "已过期", variant: "destructive" }; | ||
| } else if (exp && exp - now <= 72 * 60 * 60 * 1000) { | ||
| status = { code: "expiringSoon", badge: "即将过期", variant: "outline" }; | ||
| } else { | ||
| status = { code: "active", badge: "已启用", variant: "default" }; | ||
| } | ||
|
|
||
| const expiryText = activeUser.expiresAt | ||
| ? `${formatDateDistance(activeUser.expiresAt, new Date(), locale, { addSuffix: true })} (${formatDate(activeUser.expiresAt, "yyyy-MM-dd", locale)})` | ||
| : tUsers("neverExpires"); | ||
|
|
||
| return { status, expiryText }; | ||
| }, [activeUser, locale, tUsers]); |
There was a problem hiding this comment.
The status badge text is hardcoded in Chinese, which breaks internationalization. It should use the useTranslations hook to get the correct text for the current locale.
Additionally, the logic for determining user status is duplicated in user-list.tsx. It would be beneficial to extract this into a shared utility function to avoid code duplication and ensure consistency.
To fix the immediate issue, you can use the dashboard.userList translations. You'll need to add const tUserList = useTranslations("dashboard.userList"); before this block.
// 获取用户状态和过期信息
const userStatusInfo = useMemo(() => {
if (!activeUser) return null;
const now = Date.now();
const exp = activeUser.expiresAt ? new Date(activeUser.expiresAt).getTime() : null;
let status: {
code: "active" | "expiringSoon" | "expired" | "disabled";
variant: "default" | "secondary" | "destructive" | "outline";
};
if (!activeUser.isEnabled) {
status = { code: "disabled", variant: "secondary" };
} else if (exp && exp <= now) {
status = { code: "expired", variant: "destructive" };
} else if (exp && exp - now <= 72 * 60 * 60 * 1000) {
status = { code: "expiringSoon", variant: "outline" };
} else {
status = { code: "active", variant: "default" };
}
const expiryText = activeUser.expiresAt
? `${formatDateDistance(activeUser.expiresAt, new Date(), locale, { addSuffix: true })} (${formatDate(activeUser.expiresAt, "yyyy-MM-dd", locale)})`
: tUsers("neverExpires");
return {
status: {
...status,
badge: tUserList(`status.${status.code}`),
},
expiryText,
};
}, [activeUser, locale, tUsers, tUserList]);
Summary
Add user-level expiration and enable/disable controls, allowing administrators to manage user access with time-based restrictions and manual status toggles.
Problem
Previously, the platform lacked the ability to:
Solution
Implemented a comprehensive user expiration and status management system:
is_enabled(boolean) andexpires_at(timestamp with timezone) columns to the users table with a composite index for efficient queriesvalidateExpiresAt()function to ensure expiration dates are in the future and don't exceed 10 yearsProxyAuthenticator- expired users are automatically disabled on their next requestChanges
Database
drizzle/0028_abnormal_pretty_boy.sql- Migration addingis_enabledandexpires_atcolumns with indexsrc/drizzle/schema.ts- Schema definition updatesBackend
src/actions/users.ts- AddedvalidateExpiresAt(), integrated status fields in add/edit user actionssrc/app/v1/_lib/proxy/auth-guard.ts- Lazy expiration check with auto-disable on expired userssrc/repository/user.ts- AddedmarkUserExpired()functionsrc/lib/validation/schemas.ts- Extended Create/Update schemas withisEnabledandexpiresAtsrc/types/user.ts- Updated type definitionsFrontend
src/app/[locale]/dashboard/_components/user/*.tsx- Extended user forms and list with status management UIi18n
messages/en/errors.jsonandmessages/zh-CN/errors.jsonTesting
is_enabled=trueandexpires_at=nullMigration Notes
idx_users_enabled_expires_atoptimizes status/expiration queries