Users admin tab #2672
Users admin tab #2672
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce several enhancements to user management functionality across both the backend and frontend. The backend updates include improved user search capabilities and type safety in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AdminPanel
participant UsersService
participant UserRepository
User->>AdminPanel: Access User Management
AdminPanel->>UsersService: list(query)
UsersService->>UserRepository: findAndCountAll(filter.query)
UserRepository-->>UsersService: Return user data
UsersService-->>AdminPanel: Return user data
AdminPanel->>User: Display user data
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: 8
🧹 Outside diff range and nitpick comments (13)
frontend/src/modules/admin/models/User.model.ts (1)
1-6: Consider enhancing type safety and documentation.
- Add JSDoc comments to document the interface and its properties
- Consider using a string literal union type for roles to restrict valid values
Here's a suggested improvement:
+/** + * Represents a user in the admin interface + */ export interface UserModel { + /** Unique identifier for the user */ id: string; + /** User's email address */ email: string; + /** User's full name */ fullName: string; + /** User's assigned roles */ - roles: string[]; + roles: Array<'admin' | 'user' | 'moderator'>; // adjust roles as per your needs }frontend/src/ui-kit/search/Search.vue (4)
1-12: Consider extracting clear functionality to a methodThe clear functionality is currently implemented directly in the template. For better maintainability and reusability, consider extracting it to a method.
<template> <lf-input v-model="valueProxy"> <template #prefix> <lf-icon name="search" /> </template> <template v-if="valueProxy.length" #suffix> - <div @click="valueProxy = ''"> + <div @click="clearSearch"> <lf-icon name="xmark" /> </div> </template> </lf-input> </template>Add the method in the script section:
const clearSearch = () => { valueProxy.value = ''; };
20-25: Consider using more specific emit return typeThe emit type currently uses
anyas the return type. Consider being more explicit about the return type for better type safety.-const emit = defineEmits<{(e: 'update:modelValue', value: string | number): any }>(); +const emit = defineEmits<{(e: 'update:modelValue', value: string | number): void }>();
31-31: Consider making debounce delay configurableThe debounce delay is currently hardcoded to 300ms. Consider making it configurable through props to allow for different use cases.
+const props = defineProps<{ + modelValue: string | number, + lazy?: boolean, + debounceDelay?: number, +}>(); -const debouncedEmitValue = debounce(emitValue, 300); +const debouncedEmitValue = debounce(emitValue, props.debounceDelay ?? 300);
42-46: Consider removing redundant script tagWhen using
<script setup>, the component name can be defined using a single script block with thedefineOptionsmacro.-<script lang="ts"> -export default { - name: 'LfSearch', -}; -</script> <script setup lang="ts"> +defineOptions({ + name: 'LfSearch', +});frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue (1)
59-59: Consider relocating the users component to maintain architectural consistency.The
LfAdminUserscomponent is imported from thepagesdirectory but is being used as an embedded component within a tab. This might indicate a architectural inconsistency, as components in thepagesdirectory typically represent route-level components.Consider:
- Moving the component to a more appropriate location like
@/modules/admin/components/users/- Or, if this is intended to be a standalone page, consider using Vue Router to handle the tab navigation instead of embedding it.
This would help maintain a clearer separation between pages and components while improving maintainability.
frontend/src/modules/admin/pages/users.page.vue (4)
7-37: Enhance table accessibility.The table lacks proper ARIA attributes and scope definitions for better screen reader support.
<lf-table> <thead> <tr> - <th>User</th> - <th>Email</th> - <th>Role</th> + <th scope="col" role="columnheader">User</th> + <th scope="col" role="columnheader">Email</th> + <th scope="col" role="columnheader">Role</th> </tr> </thead> - <tbody> + <tbody role="rowgroup">
72-78: Enhance type safety for reactive state.Consider adding more specific types for the reactive state variables.
-const search = ref(''); -const loading = ref<boolean>(false); -const offset = ref(0); -const limit = ref(20); -const total = ref(0); +const search = ref<string>(''); +const loading = ref<boolean>(false); +const offset = ref<number>(0); +const limit = ref<number>(20); +const total = ref<number>(0);
110-118: Optimize pagination handling.Consider adding a computed property for determining if more data is available and handling edge cases.
+const canLoadMore = computed(() => { + return !loading.value && users.value.length < total.value; +}); + const loadMore = () => { + if (!canLoadMore.value) return; offset.value = users.value.length; fetchUsers(); };Then update the template:
- <div v-if="users.length < total" class="pt-4"> + <div v-if="canLoadMore" class="pt-4">
136-145: Consider adding cleanup on component unmount.Add cleanup logic to prevent memory leaks and unnecessary API calls.
+import { onMounted, onUnmounted } from 'vue'; + +const isComponentMounted = ref(true); + onMounted(() => { searchUsers(); }); + +onUnmounted(() => { + isComponentMounted.value = false; +});Then update the fetchUsers function:
const fetchUsers = () => { - if (loading.value) { + if (loading.value || !isComponentMounted.value) { return; }backend/src/database/repositories/userRepository.ts (3)
Line range hint
39-39: Consider improving return type safety.While adding the
stringtype annotation fortenantIdis a good improvement, the return typePromise<any[]>could be made more type-safe. Consider defining an interface for the user record structure and using it as the return type.Example interface:
interface UserWithTenant { id: string; email: string; firstName?: string; lastName?: string; tenants: { tenantId: string; roles: string[]; status: string; }[]; }Then update the return type:
-static async findAllUsersOfTenant(tenantId: string): Promise<any[]> { +static async findAllUsersOfTenant(tenantId: string): Promise<UserWithTenant[]> {
413-420: Consider performance optimization for search queries.The implementation of the query filter is functionally correct, but there are potential performance considerations for large datasets:
- Using case-insensitive LIKE queries (
ILIKE) on multiple columns without proper indexes could impact performance.- Consider adding database indexes for frequently searched columns.
Consider these improvements:
- Add indexes to improve search performance:
CREATE INDEX idx_user_fullname_gin ON users USING gin (lower(full_name) gin_trgm_ops); CREATE INDEX idx_user_email_gin ON users USING gin (lower(email) gin_trgm_ops);
- If full-text search capabilities are needed in the future, consider using PostgreSQL's full-text search features with
tsvectorcolumns and GiST indexes.
Line range hint
607-624: Enhance security measures for user data handling.While the implementation handles basic security concerns, consider these security improvements:
- The raw SQL query could be vulnerable to SQL injection if the
idparameter is not properly sanitized.- The cleanup of sensitive data could be more systematic.
Consider these improvements:
- Replace raw SQL with Sequelize's query builder:
const record = await options.database.user.findOne({ where: { id, deletedAt: null }, transaction });
- Create a systematic approach to remove sensitive data:
const SENSITIVE_FIELDS = [ 'password', 'emailVerificationToken', 'emailVerificationTokenExpiresAt', 'providerId', 'passwordResetToken', 'passwordResetTokenExpiresAt', 'jwtTokenInvalidBefore' ] as const; // Use in cleanup SENSITIVE_FIELDS.forEach(field => delete record[field]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
backend/src/database/repositories/userRepository.ts(1 hunks)frontend/src/modules/admin/models/User.model.ts(1 hunks)frontend/src/modules/admin/pages/users.page.vue(1 hunks)frontend/src/modules/admin/services/users.service.ts(1 hunks)frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue(2 hunks)frontend/src/ui-kit/search/Search.vue(1 hunks)
🧰 Additional context used
🪛 Biome
frontend/src/modules/admin/services/users.service.ts
[error] 4-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (6)
frontend/src/modules/admin/models/User.model.ts (1)
1-6: LGTM! The interface provides a clear contract for user data.
The interface correctly defines the essential user properties with appropriate types.
frontend/src/modules/admin/services/users.service.ts (2)
1-2: LGTM! Imports are correctly defined.
The necessary dependencies for HTTP requests and authentication are properly imported.
4-17: Verify service usage across the application.
Let's ensure consistent implementation across the codebase.
✅ Verification successful
Let me check the usage in users.page.vue to understand how the service is being used and verify if the changes are compatible.
Service implementation and usage are consistent
The verification shows:
- The service is used correctly in
users.page.vuewith the expected query parameters - Similar pattern is followed in
DashboardApiService, confirming consistent implementation across services - No breaking changes or inconsistencies found in the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar patterns and usage of this service
# Check for components using this service
echo "Checking components using UsersService:"
rg -l "UsersService" frontend/src/modules
# Check for similar service implementations
echo "\nChecking for similar service patterns:"
ast-grep --pattern 'export class $SERVICE_NAME {
static async $METHOD_NAME($$$) {
$$$
}
}' frontend/src/modules
Length of output: 3190
Script:
#!/bin/bash
# Check how UsersService is used in the page component
echo "Checking UsersService usage in users.page.vue:"
rg "UsersService" frontend/src/modules/admin/pages/users.page.vue -A 5
Length of output: 634
🧰 Tools
🪛 Biome
[error] 4-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
frontend/src/ui-kit/search/Search.vue (1)
14-19: LGTM! Well-structured imports and setup
The component uses modern Vue.js practices with Composition API and has all necessary imports properly organized.
frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue (2)
35-37: Verify backend API security for the new Users admin functionality.
While the frontend properly restricts access using isAdminUser, we should ensure corresponding backend API endpoints are properly secured.
Let's verify the backend security implementation:
#!/bin/bash
# Description: Check for proper admin-only middleware in related API routes
# Look for user-related route definitions
rg -A 5 "users.*router\.(get|post|put|delete)"
# Look for admin middleware usage
rg "admin.*middleware" Also applies to: 59-59
35-37: LGTM! Implementation follows established patterns.
The new Users tab implementation correctly follows the existing pattern for admin-only tabs with proper conditional rendering.
Let's verify the tab order matches the intended UX design:
✅ Verification successful
Tab order follows logical grouping and is consistent with admin features
The new Users tab is being added to a well-structured set of admin tabs that follow a logical order:
- Project Groups (general access)
- Organizations (admin only)
- Automations (admin only)
- API Keys (admin only)
- Audit logs (admin only)
Adding the Users tab alongside other admin-only features is appropriate, and the implementation follows the established pattern correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any UX/design documents specifying the tab order
rg -l "admin.*panel.*tab.*order" --type md
Length of output: 73
Script:
#!/bin/bash
# Let's check the existing tab order in the template
rg -A 10 '<el-tabs' frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue
Length of output: 549
Script:
#!/bin/bash
# Let's see the complete tabs structure to understand the full order
rg -B 2 -A 15 '<el-tabs' frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue
Length of output: 750
Script:
#!/bin/bash
# Let's see if there are any more tabs after the shown ones
rg -B 0 -A 10 'label="Automations"' frontend/src/modules/lf/segments/pages/lf-admin-panel-page.vue
Length of output: 533
| import { AuthService } from '@/modules/auth/services/auth.service'; | ||
|
|
||
| export class UsersService { | ||
| static async list(query: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for query parameters.
The any type for the query parameter reduces type safety. Consider creating a proper interface for the query parameters.
| const response = await authAxios.get( | ||
| `/tenant/${tenantId}/user`, | ||
| { | ||
| params: query, | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add proper error handling.
The HTTP request lacks error handling. Consider implementing try-catch blocks and proper error handling for different scenarios (network errors, 404, etc.).
| }, | ||
| ); | ||
|
|
||
| return response.data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type definition for the response data.
The response data is returned without type information. Consider defining an interface for the response structure to improve type safety and documentation.
| export class UsersService { | ||
| static async list(query: any) { | ||
| const tenantId = AuthService.getTenantId(); | ||
|
|
||
| const response = await authAxios.get( | ||
| `/tenant/${tenantId}/user`, | ||
| { | ||
| params: query, | ||
| }, | ||
| ); | ||
|
|
||
| return response.data; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert class to a standalone function module.
The static analysis tool correctly identifies that this class only contains static methods. Following TypeScript best practices, we should convert this to a module with standalone functions.
Here's the suggested implementation:
-export class UsersService {
- static async list(query: any) {
- const tenantId = AuthService.getTenantId();
-
- const response = await authAxios.get(
- `/tenant/${tenantId}/user`,
- {
- params: query,
- },
- );
-
- return response.data;
- }
-}
+interface UserQuery {
+ search?: string;
+ page?: number;
+ limit?: number;
+ // add other query parameters as needed
+}
+
+interface UserResponse {
+ data: User[];
+ total: number;
+ // add other response fields as needed
+}
+
+export async function listUsers(query: UserQuery): Promise<UserResponse> {
+ const tenantId = AuthService.getTenantId();
+
+ try {
+ const response = await authAxios.get<UserResponse>(
+ `/tenant/${tenantId}/user`,
+ {
+ params: query,
+ },
+ );
+ return response.data;
+ } catch (error) {
+ // Handle specific error cases
+ if (error.response?.status === 404) {
+ throw new Error('Users not found');
+ }
+ throw new Error('Failed to fetch users');
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 4-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| const debouncedEmitValue = debounce(emitValue, 300); | ||
|
|
||
| watch(valueProxy, (newVal) => { | ||
| if (props.lazy) { | ||
| debouncedEmitValue(newVal); | ||
| } else { | ||
| emitValue(newVal); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for debounced function
The debounced function should be cleaned up when the component is unmounted to prevent memory leaks and unexpected behavior.
+import { ref, watch, onUnmounted } from 'vue';
const debouncedEmitValue = debounce(emitValue, 300);
+onUnmounted(() => {
+ debouncedEmitValue.cancel();
+});📝 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 debouncedEmitValue = debounce(emitValue, 300); | |
| watch(valueProxy, (newVal) => { | |
| if (props.lazy) { | |
| debouncedEmitValue(newVal); | |
| } else { | |
| emitValue(newVal); | |
| } | |
| }); | |
| import { ref, watch, onUnmounted } from 'vue'; | |
| const debouncedEmitValue = debounce(emitValue, 300); | |
| onUnmounted(() => { | |
| debouncedEmitValue.cancel(); | |
| }); | |
| watch(valueProxy, (newVal) => { | |
| if (props.lazy) { | |
| debouncedEmitValue(newVal); | |
| } else { | |
| emitValue(newVal); | |
| } | |
| }); |
| const fetchUsers = () => { | ||
| if (loading.value) { | ||
| return; | ||
| } | ||
| loading.value = true; | ||
| UsersService.list({ | ||
| filter: { | ||
| query: search.value, | ||
| }, | ||
| offset: offset.value, | ||
| limit: limit.value, | ||
| }) | ||
| .then((res) => { | ||
| if (offset.value > 0) { | ||
| users.value = [...users.value, ...res.rows]; | ||
| } else { | ||
| users.value = res.rows; | ||
| } | ||
|
|
||
| if (res.rows.length > 0) { | ||
| total.value = res.count; | ||
| } else { | ||
| total.value = users.value.length; | ||
| } | ||
| }) | ||
| .finally(() => { | ||
| loading.value = false; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Improve error handling and edge cases in data fetching.
The current implementation has several areas for improvement:
- No error state management for failed API calls
- Edge case in total count logic could be misleading
const fetchUsers = () => {
if (loading.value) {
return;
}
loading.value = true;
+ const errorMessage = ref<string>('');
UsersService.list({
filter: {
query: search.value,
},
offset: offset.value,
limit: limit.value,
})
.then((res) => {
+ errorMessage.value = '';
if (offset.value > 0) {
users.value = [...users.value, ...res.rows];
} else {
users.value = res.rows;
}
- if (res.rows.length > 0) {
- total.value = res.count;
- } else {
- total.value = users.value.length;
- }
+ total.value = res.count;
+ })
+ .catch((error) => {
+ errorMessage.value = 'Failed to fetch users. Please try again.';
+ console.error('Error fetching users:', error);
})
.finally(() => {
loading.value = false;
});
};Also, consider adding an error state in the template:
<div v-if="errorMessage" class="text-red-500 mt-2">
{{ errorMessage }}
</div>| const roleDisplay = (roles: string[]) => { | ||
| const role = roles?.[0]; | ||
|
|
||
| if (role === 'admin') { | ||
| return 'Admin'; | ||
| } | ||
| if (role === 'projectAdmin') { | ||
| return 'Project Admin'; | ||
| } | ||
|
|
||
| if (role === 'readonly') { | ||
| return 'Read-only'; | ||
| } | ||
| return role; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve role mapping maintainability.
Consider using a constant map for role display values and adding type safety.
+const ROLE_DISPLAY_MAP: Record<string, string> = {
+ admin: 'Admin',
+ projectAdmin: 'Project Admin',
+ readonly: 'Read-only',
+} as const;
+
-const roleDisplay = (roles: string[]) => {
+const roleDisplay = (roles: string[] | undefined): string => {
const role = roles?.[0];
-
- if (role === 'admin') {
- return 'Admin';
- }
- if (role === 'projectAdmin') {
- return 'Project Admin';
- }
-
- if (role === 'readonly') {
- return 'Read-only';
- }
- return role;
+ return role ? (ROLE_DISPLAY_MAP[role] ?? role) : 'No role';
};📝 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 roleDisplay = (roles: string[]) => { | |
| const role = roles?.[0]; | |
| if (role === 'admin') { | |
| return 'Admin'; | |
| } | |
| if (role === 'projectAdmin') { | |
| return 'Project Admin'; | |
| } | |
| if (role === 'readonly') { | |
| return 'Read-only'; | |
| } | |
| return role; | |
| }; | |
| const ROLE_DISPLAY_MAP: Record<string, string> = { | |
| admin: 'Admin', | |
| projectAdmin: 'Project Admin', | |
| readonly: 'Read-only', | |
| } as const; | |
| const roleDisplay = (roles: string[] | undefined): string => { | |
| const role = roles?.[0]; | |
| return role ? (ROLE_DISPLAY_MAP[role] ?? role) : 'No role'; | |
| }; |
| <div class="pt-6 pb-6"> | ||
| <lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add debounce to search input to prevent excessive API calls.
The search input's lazy prop might not be sufficient to prevent rapid API calls as users type.
- <lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" />
+ <lf-search
+ v-model="search"
+ class="h-9"
+ :lazy="true"
+ :debounce="300"
+ placeholder="Search users..."
+ @update:model-value="searchUsers()"
+ />📝 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 class="pt-6 pb-6"> | |
| <lf-search v-model="search" class="h-9" :lazy="true" placeholder="Search users..." @update:model-value="searchUsers()" /> | |
| </div> | |
| <div class="pt-6 pb-6"> | |
| <lf-search | |
| v-model="search" | |
| class="h-9" | |
| :lazy="true" | |
| :debounce="300" | |
| placeholder="Search users..." | |
| @update:model-value="searchUsers()" | |
| /> | |
| </div> |
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
UserModelinterface.Refactor