-
Notifications
You must be signed in to change notification settings - Fork 13
feat(user): implement users manager #289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
e2b387f
8b72e3e
2115160
d442101
af5c8ad
668207b
c078b23
4eccfad
f5793a0
d58fb59
ea3e0f0
67d6060
4498bc5
6fdf3e5
197d979
781115d
5b671e5
85166f5
de66cdb
72fa0f8
5f09752
c7807d2
641d61d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ import { | |
| LayoutDashboard, | ||
| SquareTerminal, | ||
| Target, | ||
| User, | ||
| } from 'lucide-react'; | ||
| import { NavUser } from '../../ui/nav-user'; | ||
| import { NewBadge } from '../new-badge'; | ||
|
|
@@ -48,6 +49,17 @@ export function AppSidebar({ ...props }: React.ComponentProps<typeof Sidebar>) { | |
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Admin section route The new Admin > Users menu item links to |
||
| ], | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CRITICAL: Admin section route Even if authenticated, there's no indication that only admin users can access the |
||
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Admin section route The route should validate that the user has the appropriate role (admin) before granting access to user management features. |
||
| { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Admin section route Beyond role validation, there should be specific permission checks for user management actions (view, create, edit, delete users). |
||
| title: 'Admin', | ||
| url: '#', | ||
| items: [ | ||
| { | ||
| title: 'Users', | ||
| icon: <User />, | ||
| url: '/admin/users', | ||
| }, | ||
| ], | ||
| }, | ||
|
Comment on lines
+52
to
+62
|
||
| { | ||
| title: 'Attack surface', | ||
| url: '#', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,25 +58,29 @@ export function useServerDataTable({ | |
| ? urlParams.get('filter') || '' | ||
| : internalParams.filter; | ||
|
|
||
| const setParam = useCallback( | ||
| (key: string, value: string | number | undefined) => { | ||
| const setParams = useCallback( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Changed from |
||
| (newParams: Partial<typeof internalParams>) => { | ||
| if (isUpdateSearchQueryParam) { | ||
| setUrlParams( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: The function calls |
||
| (prev) => { | ||
| const next = new URLSearchParams(prev); | ||
| if (!value) { | ||
| next.delete(key); | ||
| } else { | ||
| next.set(key, value.toString()); | ||
| } | ||
|
|
||
| Object.entries(newParams).forEach(([key, value]) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Consider adding validation for parameter values Before setting URL parameters, consider validating that values are appropriate for each parameter type (e.g., page should be positive integer, sortOrder should be 'ASC' or 'DESC'). |
||
| if (value === undefined || value === null || value === '') { | ||
| next.delete(key); | ||
| } else { | ||
| next.set(key, String(value)); | ||
| } | ||
| }); | ||
|
|
||
| return next; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Add error handling for URL parameter updates The |
||
| }, | ||
| { replace: true }, | ||
| ); | ||
| } else { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Consider preserving unspecificed parameters when using internal state When |
||
| setInternalParams((prev) => ({ | ||
| ...prev, | ||
| [key]: value ?? '', | ||
| ...newParams, | ||
| })); | ||
| } | ||
| }, | ||
|
|
@@ -92,36 +96,29 @@ export function useServerDataTable({ | |
| filter, | ||
| }, | ||
| tableHandlers: { | ||
| setPage: useCallback((v: number) => setParam('page', v), [setParam]), | ||
| setParams, | ||
| setPage: useCallback((v: number) => setParams({ page: v }), [setParams]), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Currently |
||
| setPageSize: useCallback( | ||
| (v: number) => setParam('pageSize', v), | ||
| [setParam], | ||
| (v: number) => setParams({ pageSize: v }), | ||
| [setParams], | ||
| ), | ||
| setSortBy: useCallback( | ||
| (v: string) => setParams({ sortBy: v }), | ||
| [setParams], | ||
| ), | ||
| setSortBy: useCallback((v: string) => setParam('sortBy', v), [setParam]), | ||
| setSortOrder: useCallback( | ||
| (v: 'ASC' | 'DESC') => setParam('sortOrder', v), | ||
| [setParam], | ||
| (v: 'ASC' | 'DESC') => setParams({ sortOrder: v }), | ||
| [setParams], | ||
| ), | ||
| setFilter: useCallback( | ||
| (v: string) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: For search/filter functionality, consider adding debounce to prevent excessive API calls while user is typing. |
||
| if (isUpdateSearchQueryParam) { | ||
| setUrlParams( | ||
| (prev) => { | ||
| const next = new URLSearchParams(prev); | ||
| if (next.get('filter') === v) return prev; | ||
| next.set('page', '1'); | ||
| next.set('filter', v); | ||
| return next; | ||
| }, | ||
| { replace: true }, | ||
| ); | ||
| } else { | ||
| if (internalParams.filter === v) return; | ||
| setParam('page', 1); | ||
| setParam('filter', v); | ||
| } | ||
| if (filter === v) return; | ||
| setParams({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Consider resetting page on filter change When filter changes, it's common practice to reset to page 1 since the filtered results may have different pagination. This is already implemented, which is good. |
||
| filter: v, | ||
| page: 1, | ||
| }); | ||
| }, | ||
| [isUpdateSearchQueryParam, setUrlParams, setParam, internalParams.filter], | ||
| [filter, setParams], | ||
| ), | ||
| }, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| import { Badge } from '@/components/ui/badge'; | ||
| import { type ColumnDef } from '@tanstack/react-table'; | ||
| import { DataTable } from '@/components/ui/data-table'; | ||
| import { useServerDataTable } from '@/hooks/useServerDataTable'; | ||
| import { authClient, type User } from '@/utils/authClient'; | ||
| import { useQuery } from '@tanstack/react-query'; | ||
| import { useState } from 'react'; | ||
| import { UserDetailSheet } from './user-detail-sheet'; | ||
|
|
||
| const userColumns: ColumnDef<User>[] = [ | ||
| { | ||
| accessorKey: 'name', | ||
| header: 'Name', | ||
| enableSorting: true, | ||
| }, | ||
| { | ||
| accessorKey: 'email', | ||
| header: 'Email', | ||
| enableSorting: true, | ||
| }, | ||
| { | ||
| accessorKey: 'role', | ||
| header: 'Role', | ||
| cell: ({ row }) => <Badge>{row.original.role}</Badge>, | ||
| }, | ||
| { | ||
| accessorKey: 'banned', | ||
| header: 'Status', | ||
| cell: ({ row }) => | ||
| row.original.banned ? ( | ||
| <Badge variant="destructive">Banned</Badge> | ||
| ) : ( | ||
| <Badge variant="secondary">Active</Badge> | ||
| ), | ||
| }, | ||
| ]; | ||
|
|
||
| export function ListUsers() { | ||
| const { | ||
| tableParams: { page, pageSize, sortBy, sortOrder, filter }, | ||
| tableHandlers: { setPage, setPageSize, setParams, setFilter }, | ||
| } = useServerDataTable(); | ||
| const [selectedUser, setSelectedUser] = useState<User | null>(null); | ||
|
|
||
| const { data, isLoading } = useQuery({ | ||
| queryKey: ['users', { page, pageSize, sortBy, sortOrder, filter }], | ||
| queryFn: () => | ||
| authClient.admin.listUsers({ | ||
| query: { | ||
| limit: pageSize, | ||
| offset: (page - 1) * pageSize, | ||
| searchField: 'name', | ||
| searchValue: filter, | ||
| filterField: 'role', | ||
| filterOperator: 'ne', | ||
| filterValue: 'bot', | ||
| sortBy: sortBy, | ||
| sortDirection: sortOrder.toLowerCase() as 'asc' | 'desc', | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| if (!data) return null; | ||
mizhm marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
mizhm marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return ( | ||
| <> | ||
| <DataTable | ||
| data={(data.data?.users as User[]) || []} | ||
| columns={userColumns} | ||
| isLoading={isLoading} | ||
| page={page} | ||
| pageSize={pageSize} | ||
| sortBy={sortBy} | ||
| sortOrder={sortOrder} | ||
| onPageChange={setPage} | ||
| onPageSizeChange={setPageSize} | ||
| onSortChange={(col, order) => { | ||
| setParams({ sortBy: col, sortOrder: order }); | ||
| }} | ||
| filterColumnKey="value" | ||
| filterValue={filter} | ||
| onFilterChange={setFilter} | ||
| totalItems={data.data?.total} | ||
| onRowClick={setSelectedUser} | ||
| rowClassName="cursor-pointer hover:bg-muted/50 transition-colors" | ||
| /> | ||
| <UserDetailSheet | ||
| user={selectedUser} | ||
| onOpenChange={(open) => !open && setSelectedUser(null)} | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: Imported
Usericon but admin route lacks proper protectionThe Admin section with Users route is added to the sidebar menu, but there's no indication that this route is protected by authentication or authorization middleware. Anyone could potentially access /admin/users without proper credentials.