57 image scroping in inscription form image upload error management#59
Conversation
YoungMame
commented
Dec 1, 2025
There was a problem hiding this comment.
Pull request overview
This PR implements image cropping, zoom, and rotation functionality for the onboarding inscription form with improved error management. The changes enable users to customize their uploaded images before submission.
Key Changes
- Added
react-easy-croplibrary for interactive image cropping with zoom and rotation controls - Implemented crop/rotation settings storage in onboarding data structure
- Extended backend API to accept and process crop/rotation parameters using Sharp
- Updated gender values from
male/femaletomen/womenacross frontend and backend
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| nextjs/matcha/src/utils/cropImage.ts | New utility for client-side image cropping with canvas manipulation |
| nextjs/matcha/src/utils/getImageUrl.ts | Helper to create blob URLs from File objects |
| nextjs/matcha/src/components/common/ImageCropper.tsx | New cropper component with zoom and rotation controls |
| nextjs/matcha/src/components/onboarding/steps/PicturesStep.tsx | Enhanced picture upload step with cropping UI integration |
| nextjs/matcha/src/hooks/useOnboarding.ts | Updated to handle crop/rotation settings submission |
| nextjs/matcha/src/types/onboarding.ts | Added crop and rotation settings to onboarding data type |
| nextjs/matcha/src/mocks/browsing_mocks.ts | Updated gender values to use new men/women convention |
| nextjs/matcha/src/contexts/MeContext.tsx | New context provider for user profile data management |
| nextjs/matcha/src/app/(logged)/me/page.tsx | New profile settings page for viewing/editing user data |
| fastify/assets/srcs/controllers/private/me/profilePictures.ts | Enhanced to process crop/rotation with Sharp library |
| fastify/assets/srcs/plugins/checkImageConformity.ts | Updated validation to support crop dimensions |
| fastify/assets/srcs/services/UserService.ts | Added firstName/lastName to profile responses |
| package.json | Added react-easy-crop and react-image-crop dependencies |
Files not reviewed (1)
- nextjs/matcha/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
nextjs/matcha/src/components/onboarding/steps/PicturesStep.tsx:199
- Using
==instead of===for comparison. In JavaScript/TypeScript, it's best practice to use strict equality (===) to avoid type coercion issues.
const picture = currentCroppingIndex == 0 ? profilePicture : additionalPictures[currentCroppingIndex - 1];
nextjs/matcha/src/app/(logged)/me/page.tsx:11
- Unused function onChange.
function onChange(field: string, value: any) {
nextjs/matcha/src/hooks/useOnboarding.ts:4
- Unused import Fira_Sans_Extra_Condensed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const heightensAgo = today.setFullYear(today.getFullYear() - 18); | ||
|
|
||
| if (heightensAgo - (me.birthdate as Date).getTime() < 0) |
There was a problem hiding this comment.
Typo in variable name: heightensAgo should be eighteenYearsAgo to correctly represent "18 years ago".
| const heightensAgo = today.setFullYear(today.getFullYear() - 18); | |
| if (heightensAgo - (me.birthdate as Date).getTime() < 0) | |
| const eighteenYearsAgo = today.setFullYear(today.getFullYear() - 18); | |
| if (eighteenYearsAgo - (me.birthdate as Date).getTime() < 0) |
| { value: "heterosexual", label: "Hétérosexuel" }, | ||
| { value: "homosexual", label: "Homosexuel" }, | ||
| { value: "bisexual", label: "Bisexuel" }, | ||
| ] |
There was a problem hiding this comment.
Missing semicolon at the end of the ORIENTATION_OPTIONS array declaration.
| onClick={() => setRotation((rotation + 90) % 180)} | ||
| size="small" | ||
| aria-label="Rotate right" | ||
| > | ||
| <div>\-</div> | ||
| </IconButton> | ||
| <IconButton | ||
| onClick={() => setRotation((rotation + 90) % 180)} |
There was a problem hiding this comment.
The rotation button handlers both use the same logic (rotation + 90) % 180, which will cause both buttons to rotate in the same direction. Additionally, the modulo 180 means rotations will jump from 90° to -90° instead of completing a full rotation. Consider:
- Using
(rotation - 90) % 360for one button and(rotation + 90) % 360for the other - Or using different increment/decrement values to differentiate the buttons' behavior
| onClick={() => setRotation((rotation + 90) % 180)} | |
| size="small" | |
| aria-label="Rotate right" | |
| > | |
| <div>\-</div> | |
| </IconButton> | |
| <IconButton | |
| onClick={() => setRotation((rotation + 90) % 180)} | |
| onClick={() => setRotation((rotation - 90 + 360) % 360)} | |
| size="small" | |
| aria-label="Rotate left" | |
| > | |
| <div>\-</div> | |
| </IconButton> | |
| <IconButton | |
| onClick={() => setRotation((rotation + 90) % 360)} |
| ); | ||
| } | ||
|
|
||
| export function useMe() { |
There was a problem hiding this comment.
Debug console.log statements should be removed from production code.
| function onChange(field: string, value: any) { | ||
| console.log(`Field ${field} changed to`, value); | ||
| } |
There was a problem hiding this comment.
The function onChange on lines 11-13 is defined but never used. Consider removing it or implementing its functionality if it was meant to be used in the form fields.
| function onChange(field: string, value: any) { | |
| console.log(`Field ${field} changed to`, value); | |
| } |
| crop: Area; | ||
| } | ||
|
|
||
| const CROP_AREA_ASPECT = 9 / 16; |
There was a problem hiding this comment.
Unused variable CROP_AREA_ASPECT.
| const CROP_AREA_ASPECT = 9 / 16; |
|
|
||
| import { createContext, useContext, ReactNode, useState, useEffect } from "react"; | ||
| import { Location } from "../types/location"; | ||
| import axios, { Axios } from "axios"; |
There was a problem hiding this comment.
Unused import Axios.
| let rotation = Number((request.body as { rotation?: { value: string } }).rotation?.value) || 0; | ||
| if (rotation > 180) rotation = 180; | ||
| if (rotation < -180) rotation = -180; |
There was a problem hiding this comment.
The value assigned to rotation here is unused.
| let rotation = Number((request.body as { rotation?: { value: string } }).rotation?.value) || 0; | |
| if (rotation > 180) rotation = 180; | |
| if (rotation < -180) rotation = -180; |
|
|
||
| const onCropComplete = (croppedArea: Area, croppedAreaPixels: Area) => { | ||
| setCroppedAreaPixels(croppedAreaPixels) | ||
| } |
There was a problem hiding this comment.
Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
| } | |
| }; |
| setRotation(0); | ||
| setZoom(1); | ||
| } catch (e) { | ||
| console.error(e) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
| console.error(e) | |
| console.error(e); |
…upload-error-management
* Initial plan * Fix image rotation to use 360 degrees instead of 180 Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> * Remove package-lock.json from tracking Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> * Address code review feedback: use strict equality, proper types, and rotation icons Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> * Remove npm package-lock.json (project uses pnpm) Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> * Extract image selection logic and add configurable output format Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> * Restore original package-lock.json from main branch Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: YoungMame <134452452+YoungMame@users.noreply.github.com> Co-authored-by: ValentinMalassigne <valentin.malassigne3@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 37 changed files in this pull request and generated 16 comments.
Files not reviewed (1)
- nextjs/matcha/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
nextjs/matcha/src/hooks/useOnboarding.ts:5
- Unused import Fira_Sans_Extra_Condensed.
fastify/assets/srcs/routes/private/user/me/profile.ts:64 - This property is overwritten by another property in the same object literal.
firstName: { type: 'string' },
fastify/assets/srcs/routes/private/user/me/profile.ts:65
- This property is overwritten by another property in the same object literal.
lastName: { type: 'string' },
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| firstName: { type: 'string' }, | ||
| lastName: { type: 'string' }, |
There was a problem hiding this comment.
Duplicate property definitions for 'firstName' and 'lastName' in the schema. Lines 64-65 duplicate lines 67-68, which will cause validation issues.
| firstName: { type: 'string' }, | |
| lastName: { type: 'string' }, |
| ]; | ||
|
|
||
| const MALE_PICTURES = [ | ||
| const men_PICTURES = [ |
There was a problem hiding this comment.
Inconsistent naming convention: 'men_PICTURES' mixes snake_case with UPPER_CASE. Should be either 'MALE_PICTURES' or 'menPictures' to match the codebase style.
| // console.log('width', width); | ||
| // console.log('width > maxWidth', width > maxWidth); | ||
| // console.log('width < minWidth', width < minWidth); | ||
| // console.log(ratio) | ||
| // console.log(expectedRatio) |
There was a problem hiding this comment.
Commented out console.log statements should be removed rather than left in the code. This clutters the codebase.
| // console.log('width', width); | |
| // console.log('width > maxWidth', width > maxWidth); | |
| // console.log('width < minWidth', width < minWidth); | |
| // console.log(ratio) | |
| // console.log(expectedRatio) |
| @@ -0,0 +1,105 @@ | |||
| /** | |||
There was a problem hiding this comment.
There's a typo in the PR title: "scroping" should be "cropping".
| formData.append('rotation', data.additionalPicturesSettings[data.additionalPictures.indexOf(picture)].rotation.toString()); | ||
| formData.append('crop', JSON.stringify(data.additionalPicturesSettings[data.additionalPictures.indexOf(picture)].crop)); |
There was a problem hiding this comment.
Using indexOf inside a loop is inefficient and can return incorrect indices. If the same picture appears multiple times in the array, indexOf will always return the first occurrence. Store the index separately or use map with index parameter instead.
| try { | ||
| if (currentCroppingIndex === null || croppedAreaPixels === null) return; | ||
|
|
||
| const picture = currentCroppingIndex == 0 ? profilePicture : additionalPictures[currentCroppingIndex - 1]; |
There was a problem hiding this comment.
Using loose equality (==) instead of strict equality (===). This should use === for type-safe comparison.
| } | ||
|
|
||
| update = async (id: number, user: UserProfile, location?: UserLocation) => { | ||
| console.log("Updating user ID:", id, "with data:", user, "and location:", location); |
There was a problem hiding this comment.
Debug console.log statement left in production code. This should be removed or wrapped in a development-only check.
| console.log("Updating user ID:", id, "with data:", user, "and location:", location); | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.debug("Updating user ID:", id, "with data:", user, "and location:", location); | |
| } |
| throw new BadRequestError('Failed to process image with given crop/rotation'); | ||
| } | ||
|
|
||
| console.log('Saving new profile picture to', newFilePath); |
There was a problem hiding this comment.
Debug console.log statement left in production code. This should be removed or wrapped in a development-only check.
| console.log('Saving new profile picture to', newFilePath); | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.log('Saving new profile picture to', newFilePath); | |
| } |
| const CROP_AREA_ASPECT = 9 / 16; | ||
|
|
There was a problem hiding this comment.
Unused variable CROP_AREA_ASPECT.
| const CROP_AREA_ASPECT = 9 / 16; |
| } catch (e) { | ||
| console.error(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 40 changed files in this pull request and generated 11 comments.
Files not reviewed (1)
- nextjs/matcha/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
nextjs/matcha/src/app/(logged)/me/page.tsx:170
- These console.log statements on lines 148, 170, and should be removed before merging to production. Debug logging statements clutter the production console and may expose sensitive information.
const { data: profile, isLoading, error, refetch } = useMyProfile();
const [isUpdating, setIsUpdating] = useState(false);
const [isUploading, setIsUploading] = useState(false);
const [isDeleting, setIsDeleting] = useState(false);
const [isSettingMain, setIsSettingMain] = useState(false);
const [isEditing, setIsEditing] = useState(false);
const [validationError, setValidationError] = useState<string | null>(null);
const [formData, setFormData] = useState({
firstName: "",
lastName: "",
email: "",
bio: "",
tags: [] as string[],
gender: "",
orientation: "",
bornAt: "",
});
useEffect(() => {
if (profile) {
setFormData({
nextjs/matcha/src/hooks/useOnboarding.ts:5
- Unused import Fira_Sans_Extra_Condensed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { value: "heterosexual", label: "Hétérosexuel" }, | ||
| { value: "homosexual", label: "Homosexuel" }, | ||
| { value: "bisexual", label: "Bisexuel" }, | ||
| ] |
There was a problem hiding this comment.
Missing semicolon at the end of the array. This violates JavaScript/TypeScript coding standards and could cause issues with automatic semicolon insertion.
| firstName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | ||
| lastName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | ||
| email: { type: 'string', format: 'email' }, | ||
| bio: { type: 'string', minLength: 50, maxLength: 100 }, | ||
| tags: { type: 'array', items: { type: 'string' }, minItems: 1 }, | ||
| bio: { type: 'string', minLength: 50, maxLength: 500 }, | ||
| tags: { type: 'array', items: { type: 'string', minLength: 1, maxLength: 30, pattern: '[a-zA-Z_]' }, minItems: 3 }, |
There was a problem hiding this comment.
The regex patterns in the schema definitions are missing delimiters. Patterns like '[a-zA-Z-' ]' and '[a-zA-Z_]' should be '^[a-zA-Z-' ]+$' and '^[a-zA-Z_]+$' respectively to properly validate the entire string. Without anchors, these patterns will match if any part of the string contains valid characters, allowing invalid input to pass validation.
| } | ||
|
|
||
| update = async (id: number, user: UserProfile, location?: UserLocation) => { | ||
| console.log("Updating user ID:", id, "with data:", user, "and location:", location); |
There was a problem hiding this comment.
Console.log statement should be removed before merging to production. This debug log on line 135 exposes potentially sensitive user data and should not be in production code.
| console.log("Updating user ID:", id, "with data:", user, "and location:", location); |
| import { OnboardingData, OnboardingStep } from '@/types/onboarding'; | ||
| import { STEPS, MIN_INTERESTS } from '@/constants/onboarding'; | ||
| import { profileApi } from '@/lib/api/profile'; | ||
| import { Fira_Sans_Extra_Condensed } from 'next/font/google'; |
There was a problem hiding this comment.
This import of 'Fira_Sans_Extra_Condensed' from 'next/font/google' appears to be unused. There's no reference to this font anywhere in the file. This should be removed to keep the codebase clean.
| export default function getImageUrl(file: File | null): string | null { | ||
| return file ? URL.createObjectURL(file) : null; |
There was a problem hiding this comment.
There's a potential memory leak here. URL.createObjectURL creates a blob URL that needs to be explicitly revoked with URL.revokeObjectURL when no longer needed. Otherwise, the browser will keep the blob in memory until the page is closed. Consider implementing cleanup logic or using a useEffect hook with cleanup in components that use this function.
| if (query.error) { | ||
| query.error = new Error("Failed to fetch user profile"); | ||
| } |
There was a problem hiding this comment.
The error assignment on line 80 is incorrect. You're trying to assign to a readonly property. The query.error property from useQuery is readonly and cannot be reassigned. This code will cause a runtime error. Instead, handle the error appropriately in the component or remove this line.
| firstName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | ||
| lastName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | ||
| bio: { type: 'string', minLength: 50, maxLength: 500 }, | ||
| tags: { type: 'array', items: { type: 'string', minLength: 1, maxLength: 30, pattern: '[a-zA-Z_]' }, minItems: 3 }, |
There was a problem hiding this comment.
The regex patterns in the schema definitions are missing delimiters. Patterns like '[a-zA-Z-' ]' should be '^[a-zA-Z-' ]+$' to properly validate the entire string. Without anchors (^ and $), the pattern will match if any part of the string contains valid characters, not the entire string.
| firstName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | |
| lastName: { type: 'string', minLength: 1, maxLength: 50, pattern: '[a-zA-Z-\' ]' }, | |
| bio: { type: 'string', minLength: 50, maxLength: 500 }, | |
| tags: { type: 'array', items: { type: 'string', minLength: 1, maxLength: 30, pattern: '[a-zA-Z_]' }, minItems: 3 }, | |
| firstName: { type: 'string', minLength: 1, maxLength: 50, pattern: '^[a-zA-Z-\' ]+$' }, | |
| lastName: { type: 'string', minLength: 1, maxLength: 50, pattern: '^[a-zA-Z-\' ]+$' }, | |
| bio: { type: 'string', minLength: 50, maxLength: 500 }, | |
| tags: { type: 'array', items: { type: 'string', minLength: 1, maxLength: 30, pattern: '^[a-zA-Z_]+' + '$' }, minItems: 3 }, |
| const gender = Math.random() > 0.5 ? "female" : "men"; | ||
| const interestedInGenders = gender === "female" ? ["men"] : ["female"]; |
There was a problem hiding this comment.
There's a logic error in the gender assignment. Line 370 assigns either "female" or "men" to the gender variable, mixing the old and new gender values. This should consistently use either the old values ("male"/"female") or new values ("men"/"women"). Based on other changes in the PR, it should be "women" instead of "female".
| const router = useRouter(); | ||
| const queryClient = useQueryClient(); | ||
| const { data: profile, isLoading, error } = useMyProfile(); | ||
| const { data: profile, isLoading, error, refetch } = useMyProfile(); |
There was a problem hiding this comment.
Unused variable refetch.
| const { data: profile, isLoading, error, refetch } = useMyProfile(); | |
| const { data: profile, isLoading, error } = useMyProfile(); |
| import IconButton from "@/components/common/IconButton"; | ||
| import ImageCropper from "@/components/common/ImageCropper"; | ||
|
|
||
| declare type Area = { | ||
| width: number; | ||
| height: number; |
There was a problem hiding this comment.
Unused import IconButton.
| import IconButton from "@/components/common/IconButton"; | |
| import ImageCropper from "@/components/common/ImageCropper"; | |
| declare type Area = { | |
| width: number; | |
| height: number; | |
| import ImageCropper from "@/components/common/ImageCropper"; | |
| declare type Area = { | |
| width: number; | |
| height: number; | |
| height: number; |