From 20a60b5b1017be6214810ede39ab9c621d85592b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 16 Sep 2026 14:56:16 +0000 Subject: [PATCH] fix(settings): stop create-admin sheet crash after save Creating an admin succeeded, then closing the sheet re-ran a dirty-form effect and looped AlertProvider. Keep inputs controlled and confirm discard from onOpenChange instead. --- src/components/providers/AlertProvider.tsx | 2 +- .../admin-users/AddAdminSheet.test.tsx | 112 ++++++++++++++++++ .../settings/admin-users/AddAdminSheet.tsx | 61 ++++++---- src/lib/api/settings/admins/index.ts | 43 ++++--- src/lib/logic/api-error.test.ts | 59 +++++++++ src/lib/logic/api-error.ts | 9 ++ 6 files changed, 245 insertions(+), 41 deletions(-) create mode 100644 src/components/settings/admin-users/AddAdminSheet.test.tsx create mode 100644 src/lib/logic/api-error.test.ts diff --git a/src/components/providers/AlertProvider.tsx b/src/components/providers/AlertProvider.tsx index 40543fc9f..8b27be069 100644 --- a/src/components/providers/AlertProvider.tsx +++ b/src/components/providers/AlertProvider.tsx @@ -42,7 +42,7 @@ export function AlertProvider({ children }: { children: React.ReactNode }) { }; useEffect(() => { if (!isOpen) setAlert(undefined); - }, [open]); + }, [isOpen]); return ( ({ + useRouter: () => ({ refresh }), +})); + +vi.mock('@/lib/hooks/use-toast', () => ({ + toast: (...args: unknown[]) => toast(...args), +})); + +vi.mock('@/lib/api/settings/admins', () => ({ + postNewAdminUser: (...args: unknown[]) => postNewAdminUser(...args), +})); + +function OpenSheetHarness({ + onOpenChange, +}: { + onOpenChange?: (open: boolean) => void; +}) { + const [open, setOpen] = useState(true); + return ( + + { + setOpen(next); + onOpenChange?.(next); + }} + > + + + + ); +} + +function renderSheet() { + return render(); +} + +describe('AddAdminSheet', () => { + beforeAll(() => { + if (typeof globalThis.ResizeObserver === 'undefined') { + globalThis.ResizeObserver = class ResizeObserver { + observe() {} + unobserve() {} + disconnect() {} + }; + } + Element.prototype.scrollIntoView = () => {}; + Element.prototype.hasPointerCapture = () => false; + Element.prototype.setPointerCapture = () => {}; + Element.prototype.releasePointerCapture = () => {}; + }); + + afterEach(() => { + cleanup(); + refresh.mockReset(); + toast.mockReset(); + postNewAdminUser.mockReset(); + }); + + it('keeps username and password inputs controlled with empty defaults', () => { + renderSheet(); + expect(screen.getByPlaceholderText('Enter a username')).toHaveAttribute( + 'value', + '' + ); + const [password, confirm] = screen.getAllByPlaceholderText('very secret'); + expect(password).toHaveAttribute('value', ''); + expect(confirm).toHaveAttribute('value', ''); + }); + + it('closes after a successful create without tripping the unsaved-changes alert', async () => { + postNewAdminUser.mockResolvedValue(undefined); + const onOpenChange = vi.fn(); + + render(); + + fireEvent.change(screen.getByPlaceholderText('Enter a username'), { + target: { value: 'uiadmin1' }, + }); + const [password, confirm] = screen.getAllByPlaceholderText('very secret'); + fireEvent.change(password, { target: { value: 'secretpass' } }); + fireEvent.change(confirm, { target: { value: 'secretpass' } }); + fireEvent.click(screen.getByRole('button', { name: 'Create' })); + + await waitFor(() => { + expect(postNewAdminUser).toHaveBeenCalledWith('uiadmin1', 'secretpass'); + }); + await waitFor(() => { + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + expect( + screen.queryByText(/unsaved changes will be lost/i) + ).not.toBeInTheDocument(); + expect(refresh).toHaveBeenCalled(); + }); +}); diff --git a/src/components/settings/admin-users/AddAdminSheet.tsx b/src/components/settings/admin-users/AddAdminSheet.tsx index fe7a61b1c..501e661be 100644 --- a/src/components/settings/admin-users/AddAdminSheet.tsx +++ b/src/components/settings/admin-users/AddAdminSheet.tsx @@ -1,3 +1,5 @@ +'use client'; + import { Sheet, SheetContent, @@ -29,11 +31,17 @@ import { LucideX } from 'lucide-react'; import { ErrorPre } from '@/components/ui/error-pre'; import { useRouter } from 'next/navigation'; +const emptyValues = { + username: '', + password: '', + confirmPassword: '', +}; + const FormSchema = z .object({ - username: z.string(), - password: z.string(), - confirmPassword: z.string(), + username: z.string().min(1, 'Username is required'), + password: z.string().min(1, 'Password is required'), + confirmPassword: z.string().min(1, 'Confirm your password'), }) .refine( schema => { @@ -66,29 +74,32 @@ export const AddAdminSheet = ({ ); const isControlled = controlledOpen !== undefined; const open = isControlled ? controlledOpen : uncontrolledOpen; - const setOpen = (next: boolean) => { + const setOpenState = (next: boolean) => { if (!isControlled) setUncontrolledOpen(next); controlledOnOpenChange?.(next); }; const { addAlert } = useAlerts(); const form = useForm>({ resolver: rhfZodResolver(FormSchema), + defaultValues: emptyValues, }); const router = useRouter(); const { formState, reset, control, handleSubmit } = form; + const { isDirty, isSubmitted } = formState; - useEffect(() => { - if (defaultOpen !== undefined && !isControlled) - setUncontrolledOpen(defaultOpen); - }, [defaultOpen, isControlled]); + const discardAndClose = () => { + reset(emptyValues); + setOpenState(false); + onClose?.(); + }; - useEffect(() => { - if (!open && formState.isSubmitted) { - onClose?.(); - return reset(); + const requestOpenChange = (next: boolean) => { + if (next) { + setOpenState(true); + return; } - if (!open && formState.isDirty) { + if (isDirty && !isSubmitted) { addAlert({ title: 'Add Admin', description: @@ -96,22 +107,24 @@ export const AddAdminSheet = ({ cancelText: 'Cancel', actionText: 'Close', onDecision: cancel => { - if (!cancel) { - onClose?.(); - return reset(); - } - setOpen(true); + if (!cancel) discardAndClose(); }, }); - } else if (!open) { - onClose?.(); + return; } - }, [open, setOpen]); + discardAndClose(); + }; + + useEffect(() => { + if (defaultOpen !== undefined && !isControlled) + setUncontrolledOpen(defaultOpen); + }, [defaultOpen, isControlled]); function onSubmit(data: z.infer) { postNewAdminUser(data.username, data.password) - .then(res => { - setOpen(false); + .then(() => { + reset(emptyValues); + setOpenState(false); toast({ title: 'New Admin', description: ( @@ -139,7 +152,7 @@ export const AddAdminSheet = ({ } return ( - + {children}
diff --git a/src/lib/api/settings/admins/index.ts b/src/lib/api/settings/admins/index.ts index a781bd8b9..f38ae3c7b 100644 --- a/src/lib/api/settings/admins/index.ts +++ b/src/lib/api/settings/admins/index.ts @@ -1,40 +1,51 @@ 'use server'; import { Admin } from '@/lib/models/User'; import { getApiClient } from '@/lib/api'; +import { withAdminApiError } from '@/lib/logic/api-error'; export const getAdminById = async (id: string) => { - const res = await (await getApiClient()).get(`/admins/${id}`); - return res.data; + return withAdminApiError(async () => { + const res = await (await getApiClient()).get(`/admins/${id}`); + return res.data; + }); }; export const getAdmins = async ( skip: number, limit: number ): Promise<{ admins: Admin[]; count: number }> => { - const res = await ( - await getApiClient() - ).get(`/admins`, { - params: { - skip, - limit, - }, + return withAdminApiError(async () => { + const res = await ( + await getApiClient() + ).get(`/admins`, { + params: { + skip, + limit, + }, + }); + return res.data; }); - return res.data; }; export const postNewAdminUser = async (username: string, password: string) => { - await (await getApiClient()).post(`/admins`, { username, password }); + await withAdminApiError(async () => { + await (await getApiClient()).post(`/admins`, { username, password }); + }); }; export const changeAdminsPasswordById = async ( adminId: string, newPassword: string ) => { - await ( - await getApiClient() - ).put(`/admins/${adminId}/change-password`, { - newPassword, + await withAdminApiError(async () => { + await ( + await getApiClient() + ).put(`/admins/${adminId}/change-password`, { + newPassword, + }); }); }; export const deleteAdmin = async (id: string) => { - await (await getApiClient()).delete(`/admins/${id}`); + await withAdminApiError(async () => { + await (await getApiClient()).delete(`/admins/${id}`); + }); }; diff --git a/src/lib/logic/api-error.test.ts b/src/lib/logic/api-error.test.ts new file mode 100644 index 000000000..32eb45ccb --- /dev/null +++ b/src/lib/logic/api-error.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; +import { + formatAdminApiError, + isNextNavigationError, + withAdminApiError, +} from '@/lib/logic/api-error'; + +describe('formatAdminApiError', () => { + it('prefers the API response message from axios-like errors', () => { + expect( + formatAdminApiError({ + message: 'Request failed with status code 500', + response: { + status: 500, + data: { + message: + 'Admin validation failed: username: Path `username` is required.', + }, + }, + }) + ).toBe('Admin validation failed: username: Path `username` is required.'); + }); +}); + +describe('withAdminApiError', () => { + it('rethrows Next.js navigation errors unchanged', async () => { + const redirect = Object.assign(new Error('NEXT_REDIRECT'), { + digest: 'NEXT_REDIRECT;replace;/login;307;', + }); + await expect( + withAdminApiError(async () => { + throw redirect; + }) + ).rejects.toBe(redirect); + expect(isNextNavigationError(redirect)).toBe(true); + }); + + it('converts axios-like failures into a plain Error so Server Actions can serialize them', async () => { + const axiosLike = { + message: 'Request failed with status code 500', + response: { + status: 500, + data: { message: 'Username already exists' }, + }, + }; + await expect( + withAdminApiError(async () => { + throw axiosLike; + }) + ).rejects.toEqual( + expect.objectContaining({ message: 'Username already exists' }) + ); + await expect( + withAdminApiError(async () => { + throw axiosLike; + }) + ).rejects.toBeInstanceOf(Error); + }); +}); diff --git a/src/lib/logic/api-error.ts b/src/lib/logic/api-error.ts index c95fc1508..4a22d9d54 100644 --- a/src/lib/logic/api-error.ts +++ b/src/lib/logic/api-error.ts @@ -41,6 +41,15 @@ export function formatAdminApiError(err: unknown): string { return err instanceof Error ? err.message : 'Request failed'; } +export async function withAdminApiError(fn: () => Promise): Promise { + try { + return await fn(); + } catch (err) { + if (isNextNavigationError(err)) throw err; + throw new Error(formatAdminApiError(err)); + } +} + export function formatCommunicationsApiError(err: unknown): string { if (isAxiosLikeError(err)) { if (err.response?.status === 404) {