Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a password change feature for authenticated users, adding a new password change form, API integration, validation schema, and UI navigation. While the title mentions "reset," only the password change functionality is implemented in this PR.
Changes:
- Added password change feature with form validation (minimum 8 characters, passwords must match, new password must differ from old)
- Integrated new API endpoint for password changes with error handling
- Added navigation menu item and dedicated page for password changes
- Modified backend logging to use warnings instead of errors for validation failures
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/password-change/schemas/change-password-schema.ts | Defines validation schema with password matching and difference checks |
| src/features/password-change/index.ts | Feature module exports for components, API, and schemas |
| src/features/password-change/components/change-password-form.tsx | React form component with error handling and toast notifications |
| src/features/password-change/api/change-password.ts | API integration function for password change endpoint |
| src/features/backend/utils/handle-response.ts | Modified to log validation errors as warnings instead of errors |
| src/components/presentation/navbar.tsx | Added "Change Password" menu item to user dropdown |
| src/app/(private)/change-password/page.tsx | New page component for password change functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| oldPassword: RequiredStringSchema, | ||
| newPassword: RequiredStringSchema.min(8, { | ||
| message: "Hasło musi mieć co najmniej 8 znaków", | ||
| }), |
There was a problem hiding this comment.
The password validation only enforces a minimum length of 8 characters without additional complexity requirements. While 8 characters meets basic standards, consider whether the application requires additional password strength validation such as:
- Requiring a mix of character types (uppercase, lowercase, numbers, special characters)
- Checking against common password lists
- Providing user feedback on password strength
The current implementation is acceptable for basic security, but enhancement should be considered based on the application's security requirements.
| }), | |
| }) | |
| .regex(/[A-Z]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną wielką literę", | |
| }) | |
| .regex(/[a-z]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną małą literę", | |
| }) | |
| .regex(/[0-9]/, { | |
| message: "Hasło musi zawierać przynajmniej jedną cyfrę", | |
| }) | |
| .regex(/[^A-Za-z0-9]/, { | |
| message: "Hasło musi zawierać przynajmniej jeden znak specjalny", | |
| }), |
| mutationFn: async (data: ChangePasswordFormValues) => | ||
| changePassword({ | ||
| oldPassword: data.oldPassword, | ||
| newPassword: data.newPassword, | ||
| newPasswordConfirm: data.newPasswordConfirm, | ||
| }), |
There was a problem hiding this comment.
The mutation function redundantly passes all three fields (oldPassword, newPassword, newPasswordConfirm) to the changePassword API function when the data parameter already contains these exact fields. This can be simplified to just pass the data object directly:
mutationFn: changePassword
This assumes the changePassword function accepts the ChangePasswordFormValues type, which it does based on the schema. This simplification reduces code duplication and improves maintainability.
| mutationFn: async (data: ChangePasswordFormValues) => | |
| changePassword({ | |
| oldPassword: data.oldPassword, | |
| newPassword: data.newPassword, | |
| newPasswordConfirm: data.newPasswordConfirm, | |
| }), | |
| mutationFn: changePassword, |
| /** | ||
| * Calls POST /api/v1/auth/change_password | ||
| * Body: { oldPassword, newPassword, newPasswordConfirm } | ||
| * Requires authentication; fetchMutation should attach tokens automatically |
There was a problem hiding this comment.
The comment states "fetchMutation should attach tokens automatically" but doesn't specify which tokens or explain the authentication mechanism. Consider adding more detail about:
- What tokens are attached (access token, refresh token, etc.)
- How the authentication is handled (e.g., via cookies, Authorization header)
- What happens if authentication fails
This would make the API documentation more complete and helpful for other developers.
| * Requires authentication; fetchMutation should attach tokens automatically | |
| * | |
| * Authentication: | |
| * - Requires an authenticated user access token. | |
| * - The token is attached automatically by `fetchMutation` using the standard | |
| * project auth mechanism (for example, an Authorization header or HTTP-only | |
| * auth cookies). | |
| * - If the request is unauthenticated or the token is invalid/expired, the | |
| * backend is expected to return an authentication error (such as HTTP 401/403), | |
| * which `fetchMutation` will surface to the caller. |
| "use client"; | ||
|
|
||
| import { zodResolver } from "@hookform/resolvers/zod"; | ||
| import { useMutation } from "@tanstack/react-query"; | ||
| import { useForm } from "react-hook-form"; | ||
| import { toast } from "sonner"; | ||
|
|
||
| import { PasswordInput } from "@/components/inputs/password-input"; | ||
| import { Button } from "@/components/ui/button"; | ||
| import { Form, FormField } from "@/components/ui/form"; | ||
| import { FetchError } from "@/features/backend"; | ||
|
|
||
| import { changePassword } from "../api/change-password"; | ||
| import { ChangePasswordSchema } from "../schemas/change-password-schema"; | ||
| import type { ChangePasswordFormValues } from "../schemas/change-password-schema"; | ||
|
|
||
| export function ChangePasswordForm() { | ||
| const form = useForm<ChangePasswordFormValues>({ | ||
| resolver: zodResolver(ChangePasswordSchema), | ||
| defaultValues: { | ||
| oldPassword: "", | ||
| newPassword: "", | ||
| newPasswordConfirm: "", | ||
| }, | ||
| }); | ||
|
|
||
| const { mutate, isPending } = useMutation({ | ||
| mutationFn: async (data: ChangePasswordFormValues) => | ||
| changePassword({ | ||
| oldPassword: data.oldPassword, | ||
| newPassword: data.newPassword, | ||
| newPasswordConfirm: data.newPasswordConfirm, | ||
| }), | ||
| onSuccess: () => { | ||
| toast.success("Hasło zmienione poprawnie"); | ||
| form.reset(); | ||
| }, | ||
| onError: (error) => { | ||
| if (error instanceof FetchError) { | ||
| const validationIssues = error.errorReport?.error.validationIssues; | ||
| if (Array.isArray(validationIssues)) { | ||
| for (const issue of validationIssues) { | ||
| const fieldName = | ||
| (issue as Record<string, unknown>).field ?? | ||
| (issue as Record<string, unknown>).rule; | ||
| const message = (issue as Record<string, unknown>).message; | ||
| if (fieldName === "oldPassword" && typeof message === "string") { | ||
| form.setError("oldPassword", { | ||
| type: "server", | ||
| message, | ||
| }); | ||
| toast.error(message); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| toast.error(error.getCodedMessage("Nie udało się zmienić hasła")); | ||
| } else if (error instanceof Error) { | ||
| toast.error(error.message); | ||
| } else { | ||
| toast.error("Nie udało się zmienić hasła"); | ||
| } | ||
| }, | ||
| retry: false, | ||
| }); | ||
|
|
||
| return ( | ||
| <Form {...form}> | ||
| <form | ||
| noValidate | ||
| onSubmit={form.handleSubmit((data) => { | ||
| mutate(data); | ||
| })} | ||
| className="bg-background w-full max-w-md space-y-4 rounded-xl px-6 py-8" | ||
| > | ||
| <FormField | ||
| control={form.control} | ||
| name="oldPassword" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Aktualne hasło" | ||
| placeholder="Aktualne hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="newPassword" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Nowe hasło" | ||
| placeholder="Nowe hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="newPasswordConfirm" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Potwierdź nowe hasło" | ||
| placeholder="Potwierdź nowe hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> | ||
|
|
||
| <div className="flex justify-end"> | ||
| <Button type="submit" loading={isPending}> | ||
| Zmień hasło | ||
| </Button> | ||
| </div> | ||
| </form> | ||
| </Form> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The ChangePasswordForm component lacks test coverage. The similar LoginForm component in the authentication feature has comprehensive tests (login-page.test.tsx). Consider adding tests to verify:
- Form rendering with all three password fields
- Client-side validation (password length, password matching, old vs new password difference)
- Successful password change flow with form reset
- Server-side validation error handling (especially oldPassword errors)
- Generic error handling paths
| const { mutate, isPending } = useMutation({ | ||
| mutationFn: async (data: ChangePasswordFormValues) => | ||
| changePassword({ | ||
| oldPassword: data.oldPassword, | ||
| newPassword: data.newPassword, | ||
| newPasswordConfirm: data.newPasswordConfirm, | ||
| }), | ||
| onSuccess: () => { | ||
| toast.success("Hasło zmienione poprawnie"); | ||
| form.reset(); | ||
| }, | ||
| onError: (error) => { | ||
| if (error instanceof FetchError) { | ||
| const validationIssues = error.errorReport?.error.validationIssues; | ||
| if (Array.isArray(validationIssues)) { | ||
| for (const issue of validationIssues) { | ||
| const fieldName = | ||
| (issue as Record<string, unknown>).field ?? | ||
| (issue as Record<string, unknown>).rule; | ||
| const message = (issue as Record<string, unknown>).message; | ||
| if (fieldName === "oldPassword" && typeof message === "string") { | ||
| form.setError("oldPassword", { | ||
| type: "server", | ||
| message, | ||
| }); | ||
| toast.error(message); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| toast.error(error.getCodedMessage("Nie udało się zmienić hasła")); | ||
| } else if (error instanceof Error) { | ||
| toast.error(error.message); | ||
| } else { | ||
| toast.error("Nie udało się zmienić hasła"); | ||
| } | ||
| }, | ||
| retry: false, | ||
| }); |
There was a problem hiding this comment.
The error handling approach is inconsistent with the existing pattern used in LoginForm. The LoginForm uses toast.promise with getToastMessages for cleaner error handling, while this implementation manually handles errors in onSuccess/onError callbacks. Consider refactoring to use the toast.promise pattern for consistency:
- Add a changePassword entry to getToastMessages in lib/get-toast-messages.ts
- Replace the mutate/onSuccess/onError pattern with toast.promise(mutateAsync(data), getToastMessages.changePassword)
This would simplify the error handling code and maintain consistency across the codebase.
| export const ChangePasswordSchema = z | ||
| .object({ | ||
| oldPassword: RequiredStringSchema, | ||
| newPassword: RequiredStringSchema.min(8, { | ||
| message: "Hasło musi mieć co najmniej 8 znaków", | ||
| }), | ||
| newPasswordConfirm: RequiredStringSchema, | ||
| }) | ||
| .refine((data) => data.newPassword === data.newPasswordConfirm, { | ||
| message: "Hasła muszą być identyczne", | ||
| path: ["newPasswordConfirm"], | ||
| }) | ||
| .refine((data) => data.oldPassword !== data.newPassword, { | ||
| message: "Nowe hasło musi się różnić od starego", |
There was a problem hiding this comment.
The hardcoded error messages in Polish should be moved to a centralized location for consistency. The authentication feature uses FORM_ERROR_MESSAGES from data/form-error-messages.ts for validation messages. Consider:
- Adding password-specific messages to FORM_ERROR_MESSAGES or creating a similar constants file
- This makes translation management easier and ensures consistency across the application
| export const ChangePasswordSchema = z | |
| .object({ | |
| oldPassword: RequiredStringSchema, | |
| newPassword: RequiredStringSchema.min(8, { | |
| message: "Hasło musi mieć co najmniej 8 znaków", | |
| }), | |
| newPasswordConfirm: RequiredStringSchema, | |
| }) | |
| .refine((data) => data.newPassword === data.newPasswordConfirm, { | |
| message: "Hasła muszą być identyczne", | |
| path: ["newPasswordConfirm"], | |
| }) | |
| .refine((data) => data.oldPassword !== data.newPassword, { | |
| message: "Nowe hasło musi się różnić od starego", | |
| export const CHANGE_PASSWORD_ERROR_MESSAGES = { | |
| minLength: "Hasło musi mieć co najmniej 8 znaków", | |
| passwordsMustMatch: "Hasła muszą być identyczne", | |
| passwordMustDiffer: "Nowe hasło musi się różnić od starego", | |
| } as const; | |
| export const ChangePasswordSchema = z | |
| .object({ | |
| oldPassword: RequiredStringSchema, | |
| newPassword: RequiredStringSchema.min(8, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.minLength, | |
| }), | |
| newPasswordConfirm: RequiredStringSchema, | |
| }) | |
| .refine((data) => data.newPassword === data.newPasswordConfirm, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.passwordsMustMatch, | |
| path: ["newPasswordConfirm"], | |
| }) | |
| .refine((data) => data.oldPassword !== data.newPassword, { | |
| message: CHANGE_PASSWORD_ERROR_MESSAGES.passwordMustDiffer, |
| onError: (error) => { | ||
| if (error instanceof FetchError) { | ||
| const validationIssues = error.errorReport?.error.validationIssues; | ||
| if (Array.isArray(validationIssues)) { | ||
| for (const issue of validationIssues) { | ||
| const fieldName = | ||
| (issue as Record<string, unknown>).field ?? | ||
| (issue as Record<string, unknown>).rule; | ||
| const message = (issue as Record<string, unknown>).message; | ||
| if (fieldName === "oldPassword" && typeof message === "string") { | ||
| form.setError("oldPassword", { | ||
| type: "server", | ||
| message, | ||
| }); | ||
| toast.error(message); | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling logic has complex nested conditions and type assertions that could be simplified. The repeated use of 'as Record<string, unknown>' and optional chaining makes the code harder to maintain. Consider:
- Defining a proper TypeScript interface for validation issues
- Creating a helper function to extract field-specific errors
- This would improve type safety and make the error handling logic more maintainable
| const isValidationError = | ||
| code === "E_VALIDATION_ERROR" || | ||
| Array.isArray(errorReport?.error.validationIssues); | ||
| const logFunction = isValidationError ? logger.warn : logger.error; | ||
|
|
||
| logFunction( |
There was a problem hiding this comment.
błędy walidacji powinny dalej być jako błędy. jeśli chcesz zrobić wyjątek dla niepoprawnego hasła to możesz to złapać osobno, wtedy nie dawaj warna tylko po prostu nic, ale musi to być bardziej precyzyjne żeby łapać tylko ten konkretny case
| export default function ChangePasswordPage() { | ||
| return ( | ||
| <div className="container mx-auto flex h-full flex-col items-center justify-center p-4 sm:p-8"> | ||
| <h1 className="mb-4 text-2xl font-semibold">Tutaj zmienisz hasło</h1> |
| onError: (error) => { | ||
| if (error instanceof FetchError) { | ||
| const validationIssues = error.errorReport?.error.validationIssues; | ||
| if (Array.isArray(validationIssues)) { | ||
| for (const issue of validationIssues) { | ||
| const fieldName = | ||
| (issue as Record<string, unknown>).field ?? | ||
| (issue as Record<string, unknown>).rule; | ||
| const message = (issue as Record<string, unknown>).message; | ||
| if (fieldName === "oldPassword" && typeof message === "string") { | ||
| form.setError("oldPassword", { | ||
| type: "server", | ||
| message, | ||
| }); | ||
| toast.error(message); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| toast.error(error.getCodedMessage("Nie udało się zmienić hasła")); | ||
| } else if (error instanceof Error) { | ||
| toast.error(error.message); | ||
| } else { | ||
| toast.error("Nie udało się zmienić hasła"); | ||
| } |
There was a problem hiding this comment.
tu jest zdecydowanie za dużo logiki, zwłaszcza że aplikacja w innych miejscach ma już error handling. koncept jest fajny ale raczej zrezygnujemy z tego ponieważ od backendu otrzymujemy błędy po angielsku, a interfejs mamy po polsku. wystarczy wywalić modala z odpowiednim tekstem, który powinien zostać zdefiniowany w get-toast-messages.ts. powinien być odrębny przypadek dla błędnego hasła i generyczny dla innych błędów, żeby użytkownik wiedział czy coś się popsuło czy źle wpisał.
| toast.error("Nie udało się zmienić hasła"); | ||
| } | ||
| }, | ||
| retry: false, |
There was a problem hiding this comment.
czemu to tu jest? jeśli nie ma konkretnego powodu to nie zalecam ustawiania tego, bo wtedy byle błąd sieciowy uniemożliwi zmianę hasła
| export async function changePassword(body: { | ||
| oldPassword: string; | ||
| newPassword: string; | ||
| newPasswordConfirm: string; | ||
| }) { | ||
| const response = await fetchMutation<MessageResponse>( | ||
| "auth/change_password", | ||
| { | ||
| method: "POST", | ||
| body, | ||
| }, | ||
| ); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
tutaj wskazane byłoby ręczne skonstruowanie tego obiektu, aby nie zostały wysłane nadmiarowe argumenty. daj za przykład takie wywołanie:
changePassword({ oldPassword: "...", newPassword: "...", newPasswordConfirm: "...", otherSensitiveField: "..." });W obecnej implementacji przez sieć również przejdzie pole i wartość otherSensitiveField. osobiście uważam to za lekki błąd, bo może doprowadzić do podatności.
sugerowane polepszenie:
| export async function changePassword(body: { | |
| oldPassword: string; | |
| newPassword: string; | |
| newPasswordConfirm: string; | |
| }) { | |
| const response = await fetchMutation<MessageResponse>( | |
| "auth/change_password", | |
| { | |
| method: "POST", | |
| body, | |
| }, | |
| ); | |
| return response; | |
| } | |
| export async function changePassword({ | |
| oldPassword, | |
| newPassword, | |
| newPasswordConfirm, | |
| }: { | |
| oldPassword: string; | |
| newPassword: string; | |
| newPasswordConfirm: string; | |
| }) { | |
| const response = await fetchMutation<MessageResponse>( | |
| "auth/change_password", | |
| { | |
| method: "POST", | |
| body: { | |
| oldPassword, | |
| newPassword, | |
| newPasswordConfirm, | |
| }, | |
| }, | |
| ); | |
| return response; | |
| } |
poza tym ogólnie utworzenie tej funkcji dla deduplikacji wywołań do api jest jak najbardziej na plus
| export * from "./api/change-password"; | ||
| export * from "./schemas/change-password-schema"; |
There was a problem hiding this comment.
dlaczego to jest eksportowane poza feature? jedynym kodem, który jest konsumowany poza tym featurem jest formularz do zmiany
|
|
||
| /** | ||
| * Calls POST /api/v1/auth/change_password | ||
| * Body: { oldPassword, newPassword, newPasswordConfirm } |
There was a problem hiding this comment.
niekonwencjonalny sposób opisania parametrów. albo to usuń albo użyj JSDoc
| /** | ||
| * Calls POST /api/v1/auth/change_password | ||
| * Body: { oldPassword, newPassword, newPasswordConfirm } | ||
| * Requires authentication; fetchMutation should attach tokens automatically |
There was a problem hiding this comment.
zbędna informacja która może ulec rozbieżności. usunąć
4d293c6 to
720a346
Compare
720a346 to
e2e1cce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <FormField | ||
| control={form.control} | ||
| name="oldPassword" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Aktualne hasło" | ||
| placeholder="Aktualne hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="newPassword" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Nowe hasło" | ||
| placeholder="Nowe hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> | ||
|
|
||
| <FormField | ||
| control={form.control} | ||
| name="newPasswordConfirm" | ||
| render={({ field }) => ( | ||
| <PasswordInput | ||
| label="Potwierdź nowe hasło" | ||
| placeholder="Potwierdź nowe hasło" | ||
| {...field} | ||
| /> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
The password fields don’t set appropriate autocomplete hints (e.g. current-password for old password and new-password for the new/confirm fields). Without this, browser/password-manager autofill can behave poorly and users may accidentally overwrite stored credentials. Consider extending PasswordInput to accept input props (or an autoComplete prop) and pass the correct values from this form.
| export * from "./components/change-password-form"; | ||
| export * from "./api/change-password"; | ||
| export * from "./schemas/change-password-schema"; |
There was a problem hiding this comment.
PR title mentions both password change and reset, but this PR only introduces the change-password flow (schema/form/page) and no password reset (e.g., forgot-password request + token-based reset). Please either implement the reset flow in this PR or update the PR title/scope to match what's delivered.
No description provided.