feat: file upload (minio, s3, frontend...) #443
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
ntatoud
force-pushed
the
feat/upload-files-minio-s3
branch
from
0305a22 to
2ec53e9
Compare
|
TODO :
|
ntatoud
force-pushed
the
feat/upload-files-minio-s3
branch
from
bfda1a2 to
88c14de
Compare
|
Here is the idea for the refactor I did in commit : refactor(file-upload): Remove hook abstraction layer and regroup feature's files |
ntatoud
force-pushed
the
feat/upload-files-minio-s3
branch
from
4352848 to
48a85ea
Compare
ntatoud
force-pushed
the
feat/upload-files-minio-s3
branch
from
07a8868 to
baf7bce
Compare
ntatoud
force-pushed
the
feat/upload-files-minio-s3
branch
from
07aeb97 to
e1fa6d3
Compare
wip: docker compose for minio # Conflicts: # docker-compose.yml wip: add postgres to docker compose wip: improve env var and docker compose tested with R2 from Cloudflare # Conflicts: # .env.example wip: configurable key wip: add image direct upload component add healthcheck to minio making sure it is up to create bucket wip: improve rights on the minio setup feat: Add FieldUploadPreview component on AccountProfileForm feat: add optional user metadata on signedUrl S3 calls remove ACL, implement getSignedUrl, minio env variables feat: improve file uploads + useAvatarUpload and useFileUpload hooks fix: change POST http word by GET to get presigned urls feat: fetch avatar in front-end to get metadata wip: FieldUploadPreview example in Storybook fix: docker version minio and example order refactor(file-upload): Remove hook abstraction layer and regroup feature's files fix(file-upload): Remove unused code to simplify fix(file-upload): Feedback + more simplifcation wip: refactor(file-upload): with react-hook-form fix(file-upload): clean zod schema + file type validation docs(file-upload): Update docs.stories for RHF FieldUpload test(file-upload): Vitest update and default value for FieldUpload refactor(file-upload): move avatar file check before call to mutation feat(file-upload): FieldUploadPreview with RHF fix(file-upload): Delete previous Formiz FieldUpload fix(file-upload): remove control prop and defaultValues fix(file-upload): Apply feedbacks fix: sonar cloud code smells fix(file-upload): Upload input schema can't be a record wip: tests(file-upload): Test e2e and update yml CI for minio feat: update after rebase feat: full init using docker compose
ivan-dalmet
force-pushed
the
feat/upload-files-minio-s3
branch
from
3fd5239 to
36922cb
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/lib/s3/client.ts (1)
42-47: 🛠️ Refactor suggestionAdd URL sanitization for public file URLs
The current implementation directly concatenates the key with the public URL without sanitization, which could lead to URL injection issues.
export const getFilePublicUrl = (key: string | null | undefined) => { if (!key) { return undefined; } - return `${env.NEXT_PUBLIC_S3_BUCKET_PUBLIC_URL}/${key}`; + const sanitizedKey = encodeURIComponent(key.replace(/^\/+/, '')); + return `${env.NEXT_PUBLIC_S3_BUCKET_PUBLIC_URL}/${sanitizedKey}`; };docker-compose.yml (1)
38-38:⚠️ Potential issueFix inconsistent environment variable naming.
The command uses
FORWARD_MINIO_CONSOLE_PORTwhile the port mapping above usesDOCKER_FORWARD_MINIO_CONSOLE_PORT. This inconsistency might lead to unexpected behavior.Use a consistent variable name throughout:
- command: minio server /data/minio --console-address ":${FORWARD_MINIO_CONSOLE_PORT:-9090}" + command: minio server /data/minio --console-address ":${DOCKER_FORWARD_MINIO_CONSOLE_PORT:-9090}"src/lib/s3/schemas.ts (1)
45-53: 🛠️ Refactor suggestionAdd security-focused validation rules.
The schema for upload signed URL input is currently quite permissive and lacks important security validations such as file size limits, type validation, and more strict metadata controls.
Enhance the validation schema:
export const zUploadSignedUrlInput = () => z.object({ /** * Must be a string as trpc-openapi requires that attributes must be serialized */ - metadata: z.string().optional(), + metadata: z.string().max(1024).optional(), // Limit metadata size - name: z.string(), + name: z.string().max(255).regex(/^[^/\\]*$/), // Prevent path traversal - type: z.string(), + type: z.string().regex(/^[a-zA-Z]+\/[-+.a-zA-Z0-9]+$/), // Valid MIME type format - size: z.number(), + size: z.number().min(1).max(5 * 1024 * 1024), // Limit file size (e.g., 5MB) collection: zFilesCollectionName(), });
🧹 Nitpick comments (6)
src/components/Form/FieldUpload/FieldUpload.spec.tsx (1)
12-14: Fix typo in mock contentThere's a typo in the mock file content: "mock-contet" should be "mock-content".
-const mockFileRaw = new File(['mock-contet'], 'FileTest', { +const mockFileRaw = new File(['mock-content'], 'FileTest', { type: 'image/png', });README.md (1)
64-64: Fix grammar in setup instructionThe sentence should start with a verb instead of a noun for better clarity.
-> Setup a PostgreSQL database (locally or online) and replace the **DATABASE_URL** environment variable. Then you can run `pnpm db:push` to update your database schema and then run `pnpm db:seed` to seed your database. +> Set up a PostgreSQL database (locally or online) and replace the **DATABASE_URL** environment variable. Then you can run `pnpm db:push` to update your database schema and then run `pnpm db:seed` to seed your database.🧰 Tools
🪛 LanguageTool
[grammar] ~64-~64: This sentence should probably be started with a verb instead of the noun ‘Setup’. If not, consider inserting a comma for better clarity.
Context: ...TE] > Don't want to use docker? > > Setup a PostgreSQL database (locally or onlin...(SENT_START_NN_DT)
src/components/Form/FieldUpload/FieldUploadPreview.tsx (3)
7-39: Enhance accessibility for the image preview component.The image preview lacks proper accessibility attributes which are important for screen readers and other assistive technologies.
Consider adding these accessibility improvements:
<Box position="relative" bg="gray.200" height="100%" aspectRatio="1" overflow="hidden" rounded="md" + role="img" + aria-label={`Preview of uploaded image`} > - <Box as="img" width="100%" height="100%" objectFit="cover" src={image} /> + <Box + as="img" + width="100%" + height="100%" + objectFit="cover" + src={image} + alt="" + />
66-75: Improve error handling for file reading failures.The current implementation handles file reading errors using a simple reject without providing specific error information or user feedback.
Add more robust error handling:
const uploadedFileToPreview = await new Promise<string>( (resolve, reject) => { const reader = new FileReader(); reader.onloadend = () => resolve(reader.result?.toString() ?? ''); - reader.onerror = reject; + reader.onerror = (error) => { + console.error('Error reading file for preview:', error); + reject(new Error('Failed to read file for preview')); + }; if (value.file) { reader.readAsDataURL(value.file); } } );
52-56: Add loading state during file preview generation.There's no loading state indicator while the file is being read, which might lead to a poor user experience for larger files.
Consider adding a loading state:
const [fileToPreview, setFileToPreview] = useState<string>(); +const [isLoading, setIsLoading] = useState(false); const value = watch(uploaderName); const previewFile = useCallback(async () => { + setIsLoading(true); if (!value || (!value.fileUrl && !value.file)) { setFileToPreview(undefined); + setIsLoading(false); return; } // ...rest of the function setFileToPreview(uploadedFileToPreview); + setIsLoading(false); }, [value]);src/lib/s3/schemas.ts (1)
28-38: Add error logging for file validation failures.The current implementation adds an issue to the context but doesn't log the error, which could make debugging difficult.
Consider adding debug logging:
.superRefine((input, ctx) => { const config = FILES_COLLECTIONS_CONFIG[collection]; const validateFileReturn = validateFile({ input, config }); if (!validateFileReturn.success) { + console.debug(`File validation failed for collection ${collection}:`, validateFileReturn.error); ctx.addIssue({ code: z.ZodIssueCode.custom, message: t(`files.errors.${validateFileReturn.error.key}`), }); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.dockerignore(1 hunks).env.example(2 hunks).github/workflows/code-quality.yml(0 hunks).github/workflows/e2e-tests.yml(2 hunks).storybook/preview.tsx(3 hunks)README.md(2 hunks)docker-compose.yml(1 hunks)docker/initialize-database.dockerfile(1 hunks)e2e/avatar-upload.spec.ts(1 hunks)package.json(3 hunks)src/components/Form/FieldUpload/FieldUpload.spec.tsx(1 hunks)src/components/Form/FieldUpload/FieldUploadPreview.tsx(1 hunks)src/components/Form/FieldUpload/docs.stories.tsx(1 hunks)src/components/Form/FieldUpload/index.tsx(1 hunks)src/components/Form/FieldUpload/utils.ts(1 hunks)src/components/Form/FormFieldController.tsx(3 hunks)src/components/ImageUpload/docs.stories.tsx(1 hunks)src/components/ImageUpload/index.tsx(1 hunks)src/env.mjs(3 hunks)src/features/account/AccountProfileForm.tsx(4 hunks)src/features/account/schemas.ts(2 hunks)src/features/admin/AdminNavBar.tsx(2 hunks)src/features/app/AppNavBarDesktop.tsx(3 hunks)src/features/users/PageAdminUsers.tsx(1 hunks)src/features/users/schemas.ts(2 hunks)src/lib/s3/client.ts(1 hunks)src/lib/s3/config.ts(1 hunks)src/lib/s3/schemas.ts(1 hunks)src/lib/s3/utils.ts(1 hunks)src/locales/ar/common.json(1 hunks)src/locales/en/account.json(2 hunks)src/locales/en/common.json(1 hunks)src/locales/fr/account.json(2 hunks)src/locales/fr/common.json(1 hunks)src/locales/sw/common.json(1 hunks)src/server/config/s3.ts(1 hunks)src/server/router.ts(2 hunks)src/server/routers/account.tsx(4 hunks)src/server/routers/files.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/code-quality.yml
🚧 Files skipped from review as they are similar to previous changes (30)
- src/locales/fr/common.json
- docker/initialize-database.dockerfile
- src/server/router.ts
- src/locales/sw/common.json
- src/features/admin/AdminNavBar.tsx
- src/locales/en/common.json
- .dockerignore
- src/components/ImageUpload/index.tsx
- e2e/avatar-upload.spec.ts
- src/locales/ar/common.json
- .storybook/preview.tsx
- src/lib/s3/config.ts
- src/features/users/PageAdminUsers.tsx
- src/server/routers/files.ts
- src/locales/en/account.json
- src/components/Form/FieldUpload/utils.ts
- src/server/routers/account.tsx
- src/lib/s3/utils.ts
- .github/workflows/e2e-tests.yml
- src/features/users/schemas.ts
- .env.example
- src/features/app/AppNavBarDesktop.tsx
- src/features/account/schemas.ts
- src/components/ImageUpload/docs.stories.tsx
- src/locales/fr/account.json
- src/server/config/s3.ts
- package.json
- src/env.mjs
- src/components/Form/FieldUpload/docs.stories.tsx
- src/components/Form/FieldUpload/index.tsx
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~64-~64: This sentence should probably be started with a verb instead of the noun ‘Setup’. If not, consider inserting a comma for better clarity.
Context: ...TE] > Don't want to use docker? > > Setup a PostgreSQL database (locally or onlin...
(SENT_START_NN_DT)
🔇 Additional comments (7)
src/components/Form/FormFieldController.tsx (1)
24-24: Integration of file upload functionality looks good!The changes to add the new
uploadfield type follow the established pattern in the codebase - import the component, add its props to the union type, and add the case to render it when needed.Also applies to: 53-53, 120-121
src/components/Form/FieldUpload/FieldUpload.spec.tsx (1)
25-54: Add test cases for error scenariosThe test suite should include cases for invalid file types and upload failures to ensure robust error handling.
+test('rejects invalid file type', async () => { + const user = setupUser(); + const mockedSubmit = vi.fn(); + const invalidFile = new File(['content'], 'test.txt', { type: 'text/plain' }); + + render( + <FormMocked + schema={z.object({ file: zFieldUploadValue(['image']).optional() })} + useFormOptions={{ defaultValues: { file: undefined } }} + onSubmit={mockedSubmit} + > + {({ form }) => ( + <FormField> + <FormFieldLabel>File</FormFieldLabel> + <FormFieldController + type="upload" + name="file" + control={form.control} + placeholder="Upload" + /> + </FormField> + )} + </FormMocked> + ); + + const input = screen.getByLabelText<HTMLInputElement>('Upload'); + await user.upload(input, invalidFile); + await user.click(screen.getByRole('button', { name: 'Submit' })); + expect(mockedSubmit).not.toHaveBeenCalled(); +});README.md (1)
28-28: Clear documentation of S3 requirementsThe README changes effectively communicate the new S3 requirements and setup instructions.
Also applies to: 56-56, 65-66
src/lib/s3/client.ts (1)
7-40: Enhance error handling, security, and performanceThe current implementation works but could be improved with better error handling, file validation, and upload progress tracking.
Consider implementing:
- File size and type validation before upload
- Upload progress tracking
- More specific error messages for different failure scenarios
export const uploadFile = async (params: { file: File; trpcClient: ReturnType<typeof trpc.useUtils>['client']; collection: RouterInputs['files']['uploadPresignedUrl']['collection']; metadata?: Record<string, string>; onError?: (file: File, error: unknown) => void; + onProgress?: (progress: number) => void; }) => { try { + // Validate file size and type + const maxSize = 5 * 1024 * 1024; // 5MB + if (params.file.size > maxSize) { + throw new Error('File size exceeds maximum limit'); + } + const presignedUrlOutput = await params.trpcClient.files.uploadPresignedUrl.mutate({ // Metadata is a Record<string, string> but should be serialized for trpc-openapi metadata: params.metadata ? stringify(params.metadata) : undefined, collection: params.collection, type: params.file.type, size: params.file.size, name: params.file.name, }); - const response = await fetch(presignedUrlOutput.signedUrl, { - method: 'PUT', - headers: { 'Content-Type': params.file.type }, - body: params.file, - }); - - if (!response.ok) { - throw new Error('Failed to upload file'); - } + const xhr = new XMLHttpRequest(); + xhr.upload.addEventListener('progress', (event) => { + if (event.lengthComputable) { + params.onProgress?.(Math.round((event.loaded / event.total) * 100)); + } + }); + + await new Promise<void>((resolve, reject) => { + xhr.open('PUT', presignedUrlOutput.signedUrl); + xhr.setRequestHeader('Content-Type', params.file.type); + xhr.onload = () => { + if (xhr.status === 200) { + resolve(); + } else { + reject(new Error(`Upload failed with status ${xhr.status}`)); + } + }; + xhr.onerror = () => reject(new Error('Network error during upload')); + xhr.send(params.file); + }); return presignedUrlOutput.key; } catch (error) { params.onError?.(params.file, error); - throw error; + throw error instanceof Error ? error : new Error('Failed to upload file'); } };src/features/account/AccountProfileForm.tsx (3)
37-54: Well implemented mutation function with proper file handling.The mutation function correctly handles file uploads by:
- Using conditional logic to check if a file exists before attempting upload
- Properly passing the collection name and error handling callbacks
- Falling back to the existing image URL if no new file is provided
The implementation is robust and follows good practices.
93-106: Great integration of the avatar upload field.The avatar upload field is properly integrated with:
- Appropriate label and placeholder
- Configured to accept only allowed file types from configuration
- Connected to the form's control for proper validation
This enhances the user experience while maintaining proper form validation.
86-91: Simplified form submission handler.The form submission handler has been streamlined to directly call the mutation with form values, which is much cleaner than the previous implementation.
| /bin/sh -c " | ||
| /usr/bin/mc alias set myminio http://minio:${DOCKER_FORWARD_MINIO_PORT:-9000} $DOCKER_MINIO_USERNAME $DOCKER_MINIO_PASSWORD; | ||
| /usr/bin/mc admin user add myminio $S3_ACCESS_KEY_ID $S3_SECRET_ACCESS_KEY; | ||
| /usr/bin/mc admin policy attach myminio readwrite --user $S3_ACCESS_KEY_ID; | ||
| /usr/bin/mc mb myminio/$S3_BUCKET_NAME; | ||
| /usr/bin/mc anonymous set download myminio/$S3_BUCKET_NAME; | ||
| exit 0; | ||
| " |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling to createbuckets service.
The current shell script doesn't check for errors or validate that operations have succeeded, which could lead to silent failures.
Add error checking to ensure each command completes successfully:
entrypoint: >
/bin/sh -c "
- /usr/bin/mc alias set myminio http://minio:${DOCKER_FORWARD_MINIO_PORT:-9000} $DOCKER_MINIO_USERNAME $DOCKER_MINIO_PASSWORD;
- /usr/bin/mc admin user add myminio $S3_ACCESS_KEY_ID $S3_SECRET_ACCESS_KEY;
- /usr/bin/mc admin policy attach myminio readwrite --user $S3_ACCESS_KEY_ID;
- /usr/bin/mc mb myminio/$S3_BUCKET_NAME;
- /usr/bin/mc anonymous set download myminio/$S3_BUCKET_NAME;
- exit 0;
+ /usr/bin/mc alias set myminio http://minio:${DOCKER_FORWARD_MINIO_PORT:-9000} $DOCKER_MINIO_USERNAME $DOCKER_MINIO_PASSWORD || exit 1;
+ /usr/bin/mc admin user add myminio $S3_ACCESS_KEY_ID $S3_SECRET_ACCESS_KEY 2>/dev/null || echo 'User already exists';
+ /usr/bin/mc admin policy attach myminio readwrite --user $S3_ACCESS_KEY_ID || exit 1;
+ /usr/bin/mc mb myminio/$S3_BUCKET_NAME 2>/dev/null || echo 'Bucket already exists';
+ /usr/bin/mc anonymous set download myminio/$S3_BUCKET_NAME || exit 1;
+ echo 'S3 configuration completed successfully';
"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /bin/sh -c " | |
| /usr/bin/mc alias set myminio http://minio:${DOCKER_FORWARD_MINIO_PORT:-9000} $DOCKER_MINIO_USERNAME $DOCKER_MINIO_PASSWORD; | |
| /usr/bin/mc admin user add myminio $S3_ACCESS_KEY_ID $S3_SECRET_ACCESS_KEY; | |
| /usr/bin/mc admin policy attach myminio readwrite --user $S3_ACCESS_KEY_ID; | |
| /usr/bin/mc mb myminio/$S3_BUCKET_NAME; | |
| /usr/bin/mc anonymous set download myminio/$S3_BUCKET_NAME; | |
| exit 0; | |
| " | |
| entrypoint: > | |
| /bin/sh -c " | |
| /usr/bin/mc alias set myminio http://minio:${DOCKER_FORWARD_MINIO_PORT:-9000} $DOCKER_MINIO_USERNAME $DOCKER_MINIO_PASSWORD || exit 1; | |
| /usr/bin/mc admin user add myminio $S3_ACCESS_KEY_ID $S3_SECRET_ACCESS_KEY 2>/dev/null || echo 'User already exists'; | |
| /usr/bin/mc admin policy attach myminio readwrite --user $S3_ACCESS_KEY_ID || exit 1; | |
| /usr/bin/mc mb myminio/$S3_BUCKET_NAME 2>/dev/null || echo 'Bucket already exists'; | |
| /usr/bin/mc anonymous set download myminio/$S3_BUCKET_NAME || exit 1; | |
| echo 'S3 configuration completed successfully'; | |
| " |
Describe your changes
closes #<issue_id>
Simplified version of the Avatar Upload process :
Notes :
keyof the fileLeft to do / Lines of thought
Screenshots
Documentation
Checklist
pnpm storybookcommand and everything is workingSummary by CodeRabbit
ImageUploadcomponent facilitates image file uploads with improved user interaction.FieldUploadandFieldUploadPreviewcomponents enhance file upload functionality within forms.validateFileutility function improves error handling for file uploads.