-
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 all 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, page: 1 }), | ||
|
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: When changing page size, resetting to page 1 is common but consider if this behavior should be configurable or if users expect to stay on the same visual page. |
||
| [setParams], | ||
| ), | ||
| setSortBy: useCallback( | ||
| (v: string) => setParams({ sortBy: v, page: 1 }), | ||
|
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: The function accepts any string for sortBy but should validate against allowed column names to prevent potential injection or errors. |
||
| [setParams], | ||
| ), | ||
| setSortBy: useCallback((v: string) => setParam('sortBy', v), [setParam]), | ||
| setSortOrder: useCallback( | ||
| (v: 'ASC' | 'DESC') => setParam('sortOrder', v), | ||
| [setParam], | ||
| (v: 'ASC' | 'DESC') => setParams({ sortOrder: v, page: 1 }), | ||
|
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: The function accepts 'ASC' | 'DESC' but consider adding runtime validation to ensure only valid values are accepted. |
||
| [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,211 @@ | ||
| import { Button } from '@/components/ui/button'; | ||
| import { | ||
| Dialog, | ||
| DialogContent, | ||
| DialogDescription, | ||
| DialogFooter, | ||
| DialogHeader, | ||
| DialogTitle, | ||
| DialogTrigger, | ||
| } from '@/components/ui/dialog'; | ||
| import { | ||
| Form, | ||
| FormControl, | ||
| FormField, | ||
| FormItem, | ||
| FormLabel, | ||
| FormMessage, | ||
| } from '@/components/ui/form'; | ||
| import { Input } from '@/components/ui/input'; | ||
| import { | ||
| Select, | ||
| SelectContent, | ||
| SelectItem, | ||
| SelectTrigger, | ||
| SelectValue, | ||
| } from '@/components/ui/select'; | ||
| import { authClient } from '@/utils/authClient'; | ||
| import { zodResolver } from '@hookform/resolvers/zod'; | ||
| import { useMutation, useQueryClient } from '@tanstack/react-query'; | ||
| import { Loader2Icon, UserPlus } from 'lucide-react'; | ||
| import { useState } from 'react'; | ||
| import { useForm } from 'react-hook-form'; | ||
| import { toast } from 'sonner'; | ||
| import { z } from 'zod'; | ||
|
|
||
| const formSchema = z.object({ | ||
| name: z.string().min(1, 'Name is required'), | ||
| email: z.string().min(1, 'Email is required').email('Invalid email address'), | ||
| password: z.string().min(8, 'Password must be at least 8 characters'), | ||
|
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 password confirmation field for better UX and security The password field lacks a confirmation field which is standard practice for user creation to prevent typos. Consider adding a 'confirmPassword' field with validation that it matches the password field. |
||
| role: z.enum(['user', 'admin']), | ||
| }); | ||
|
|
||
| type FormValues = z.infer<typeof formSchema>; | ||
|
|
||
| export function AddUserDialog() { | ||
| const [open, setOpen] = useState(false); | ||
| const queryClient = useQueryClient(); | ||
|
|
||
| const form = useForm<FormValues>({ | ||
| resolver: zodResolver(formSchema), | ||
| defaultValues: { | ||
| name: '', | ||
| email: '', | ||
| password: '', | ||
| role: 'user', | ||
| }, | ||
| }); | ||
|
|
||
| const { mutate: createUser, isPending } = useMutation({ | ||
| mutationFn: (values: FormValues) => | ||
| authClient.admin.createUser({ | ||
| name: values.name, | ||
| email: values.email, | ||
| password: values.password, | ||
| role: values.role, | ||
| }), | ||
| onSuccess: () => { | ||
| toast.success('User created successfully.'); | ||
| setOpen(false); | ||
| form.reset(); | ||
| return queryClient.invalidateQueries({ queryKey: ['users'] }); | ||
| }, | ||
| onError: () => { | ||
| toast.error('Failed to create user.'); | ||
| }, | ||
| }); | ||
|
|
||
| function onSubmit(values: FormValues) { | ||
| createUser(values); | ||
| } | ||
|
|
||
| function handleOpenChange(nextOpen: boolean) { | ||
| if (!nextOpen) { | ||
| form.reset(); | ||
| } | ||
| setOpen(nextOpen); | ||
| } | ||
|
|
||
| return ( | ||
| <Dialog open={open} onOpenChange={handleOpenChange}> | ||
| <DialogTrigger asChild> | ||
| <Button size="sm" className="gap-1.5"> | ||
| <UserPlus className="h-4 w-4" /> | ||
| Add User | ||
| </Button> | ||
| </DialogTrigger> | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Add User</DialogTitle> | ||
| <DialogDescription> | ||
| Create a new user account. They will be able to sign in immediately. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <Form {...form}> | ||
| <form | ||
| id="add-user-form" | ||
| onSubmit={form.handleSubmit(onSubmit)} | ||
| className="space-y-4 py-2" | ||
| > | ||
| <FormField | ||
| control={form.control} | ||
| name="name" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Name</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| placeholder="Jane Smith" | ||
| {...field} | ||
| disabled={isPending} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name="email" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Email</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
| type="email" | ||
| placeholder="jane@example.com" | ||
| {...field} | ||
| disabled={isPending} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name="password" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Password</FormLabel> | ||
| <FormControl> | ||
| <Input | ||
|
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 showing password strength indicator For better user experience, consider adding a password strength indicator to help users create stronger passwords. |
||
| type="password" | ||
| placeholder="Min. 8 characters" | ||
|
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 'show password' toggle for accessibility Consider adding a toggle to show/hide the password field content for better accessibility and user experience. |
||
| {...field} | ||
| disabled={isPending} | ||
| /> | ||
| </FormControl> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| <FormField | ||
| control={form.control} | ||
| name="role" | ||
| render={({ field }) => ( | ||
| <FormItem> | ||
| <FormLabel>Role</FormLabel> | ||
| <Select | ||
| onValueChange={field.onChange} | ||
| defaultValue={field.value} | ||
| disabled={isPending} | ||
| > | ||
| <FormControl> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select a role" /> | ||
| </SelectTrigger> | ||
| </FormControl> | ||
| <SelectContent> | ||
| <SelectItem value="user">User</SelectItem> | ||
| <SelectItem value="admin">Admin</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| <FormMessage /> | ||
| </FormItem> | ||
| )} | ||
| /> | ||
| </form> | ||
| </Form> | ||
| <DialogFooter> | ||
| <Button | ||
| variant="outline" | ||
| onClick={() => handleOpenChange(false)} | ||
| disabled={isPending} | ||
| > | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| type="submit" | ||
| form="add-user-form" | ||
| disabled={isPending} | ||
| className="gap-1.5" | ||
| > | ||
| {isPending && <Loader2Icon className="h-4 w-4 animate-spin" />} | ||
| Create User | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| } | ||
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.