162 add a modal when submitting an osbl creation to get metadata about the contribution #164
Conversation
- Created ContributionDialog component for capturing contribution details - Implemented nested validation for contribution files and comments - Added dynamic dialog opening with form validation - Enhanced MyFileInput to support multiple file uploads - Refactored contribution form submission to use two-step validation process - Updated types and form handling to support new contribution dialog
- Added support for multiple file attachments validation in AttachableValidation concern - Updated Contribution model to include file size validation - Implemented comprehensive file attachment validation with size and optional content type checks - Added RSpec tests to verify file attachment validation behavior - Updated MyInput component to include id and placeholder props
WalkthroughThis pull request introduces a new modal component for submitting contributions, namely the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Contribution Form
participant D as ContributionDialog
participant S as Server
U->>F: Enter contribution details and select files
F->>F: Validate form data and file size constraints
F->>D: Open confirmation dialog if valid
U->>D: Confirm submission
D->>F: Trigger submission handler
F->>S: Submit contribution data
S-->>F: Acknowledge submission
Possibly related issues
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
app/frontend/components/shared/MyFileInput.tsx (3)
5-12: Unify theonChangehandler to consistently use an array of filesCurrently, the
onChangehandler acceptsFile | File[] | undefined, which requires conditional checks elsewhere in your code. For better consistency and to simplify the handling of files, consider updating theonChangehandler to always provide an array ofFileobjects, even when a single file is selected.Apply this diff to adjust the
onChangetype and related properties:interface MyFileInputProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'onChange' | 'onReset'> { id: string labelText?: string | ReactElement multiple?: boolean required?: boolean error?: string - onChange: (file: File | File[] | undefined) => void // your custom onChange handler - file?: File | File[] + onChange: (files: File[]) => void // your custom onChange handler + files?: File[] }
29-33: SimplifyhandleFileChangeby always returning an array of filesWith the
onChangehandler now accepting an array of files, you can simplify thehandleFileChangefunction to eliminate conditional logic based on themultipleprop.Apply this diff to simplify the
handleFileChangefunction:const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>): void => { const fileAttached = (e.target.files != null) && e.target.files.length > 0 setHasFile(fileAttached) - - if (multiple && e.target.files != null) { - onChange(Array.from(e.target.files)) - } else { - onChange(e.target.files?.[0] ? [e.target.files[0]] : []) - } + const files = e.target.files != null ? Array.from(e.target.files) : [] + onChange(files) }
42-47: SimplifygetFileNamefunctionSince
filesis always an array, you can simplify thegetFileNamefunction by removing the conditional check.Apply this diff to simplify
getFileName:function getFileName (files: File[]): string { - if (Array.isArray(file)) { - return file.map(f => f.name).join(', ') - } - return file.name + return files.map(f => f.name).join(', ') }app/models/concerns/attachable_validation.rb (1)
33-34: Utilize Rails I18n for error messagesThe error messages in
validate_attachment_sizeandvalidate_attachment_content_typeare hardcoded in French. To support internationalization and make your application more adaptable, consider using Rails' I18n framework for error messages.Apply this diff to modify the error messages:
def validate_attachment_size(attachment, name, max_size) if attachment.blob.byte_size > max_size - errors.add(name, "la taille du fichier doit être inférieure à #{max_size / 1.megabyte} MB") + errors.add(name, I18n.t('errors.messages.file_size_too_large', size: max_size / 1.megabyte)) end end def validate_attachment_content_type(attachment, name, content_types) unless attachment.content_type.in?(content_types) formatted_types = content_types.map { |type| type.split("/").last.upcase }.join(", ") - errors.add(name, "le fichier doit être de type : #{formatted_types}") + errors.add(name, I18n.t('errors.messages.invalid_content_type', types: formatted_types)) end endRemember to add the corresponding translations to your locale files:
# config/locales/fr.yml fr: errors: messages: file_size_too_large: "la taille du fichier doit être inférieure à %{size} MB" invalid_content_type: "le fichier doit être de type : %{types}"Also applies to: 40-41
app/frontend/components/shared/MyInput.tsx (1)
7-13: LGTM! Consider adding JSDoc comments.The interface changes improve type safety by extending InputHTMLAttributes and simplify the component's structure. Consider adding JSDoc comments to document the props, especially for the required
idfield.+/** + * Props for the MyInput component + * @property {string} id - Unique identifier for the input element + * @property {string} [error] - Error message to display + * @property {boolean} [showResetButton] - Whether to show the reset button + * @property {() => void} [onReset] - Callback when reset button is clicked + * @property {string | ReactElement} [labelText] - Text or element to display as label + */ interface MyInputProps extends InputHTMLAttributes<HTMLInputElement> { id: string error?: string showResetButton?: boolean onReset?: () => void labelText?: string | ReactElement }spec/models/contribution_spec.rb (1)
42-69: LGTM! Consider adding more test cases.The file attachment tests effectively validate size constraints. Consider adding tests for:
- Multiple file attachments
- Different file types
- Edge cases (exactly 5MB)
it "is valid with multiple files under size limit" do contribution = build(:contribution) allow_any_instance_of(ActiveStorage::Blob).to receive(:byte_size).and_return(1.megabyte) 3.times do |i| contribution.files.attach( io: StringIO.new, filename: "file_#{i}.txt", content_type: "text/plain" ) end expect(contribution).to be_valid end it "handles edge case of exactly 5MB file size" do contribution = build(:contribution) allow_any_instance_of(ActiveStorage::Blob).to receive(:byte_size).and_return(5.megabytes) contribution.files.attach( io: StringIO.new, filename: "edge_case.txt", content_type: "text/plain" ) expect(contribution).to be_valid endapp/frontend/components/pages/contribution/new/ContributionDialog.tsx (2)
44-46: Consider improving screen reader accessibility.The DialogDescription is marked as
sr-onlybut contains important context. Consider making it visible to all users or providing equivalent visible context elsewhere.- <DialogDescription className='sr-only'> + <DialogDescription> Enregistrez votre contribution pour la soumettre à la modération. </DialogDescription>
84-87: Add loading state feedback.The submit button should show a loading state while processing to provide better user feedback.
<Button onClick={onConfirm} disabled={processing}> <Save className='mr-2' /> - Enregistrer + {processing ? 'Enregistrement...' : 'Enregistrer'} </Button>app/frontend/pages/Contribution/New.tsx (6)
19-19: Consider centralizing constants and validation schemas.The validation logic is well-structured, but consider the following improvements for better maintainability:
- Move size constants to a centralized configuration file
- Extract validation schemas to a separate file (e.g.,
validationSchemas.ts)+// app/frontend/constants/files.ts +export const MAX_LOGO_SIZE = 1 * 1024 * 1024 // 1MB +export const MAX_DOCUMENT_SIZE = 5 * 1024 * 1024 // 5MB +export const ALLOWED_LOGO_TYPES = ['image/svg+xml', 'image/png', 'image/webp'] +// app/frontend/validations/contribution.ts +import { MAX_LOGO_SIZE, MAX_DOCUMENT_SIZE, ALLOWED_LOGO_TYPES } from '@/constants/files' +export const osblValidation = z.object({...}) +export const contributionValidation = z.object({...})Also applies to: 39-52
127-132: Enhance type safety of the helper function.The
setContributionFieldfunction could benefit from stricter typing:-function setContributionField (field: keyof OsblCreationData['contribution'], value: OsblCreationData['contribution'][keyof OsblCreationData['contribution']]): void { +function setContributionField<T extends keyof OsblCreationData['contribution']> ( + field: T, + value: OsblCreationData['contribution'][T] +): void { setData('contribution', { ...data.contribution, [field]: value }) }
135-145: Improve error handling in submission function.The error handling in
handleConfirmSubmitcould be more robust:function handleConfirmSubmit (): void { const result = contributionValidation.safeParse(data) if (result.success === false) { const issues = result.error.issues setError('contribution.files' as 'contribution', issues[0].message) + toast.error('Veuillez corriger les erreurs avant de continuer.') return } transform((data) => deepCleanData(data)) - post(`/users/${currentUser.id}/contributions`) + post( + `/users/${currentUser.id}/contributions`, + { + onSuccess: () => { + toast.success('Contribution enregistrée avec succès') + }, + onError: () => { + toast.error('Une erreur est survenue lors de l\'enregistrement') + } + } + ) }
88-98: Consider performance optimizations.The component could benefit from callback memoization and proper cleanup:
+import { useCallback } from 'react' const { data, setData, post, processing, errors, clearErrors, setError, transform } = useForm({ contribution: { body: '', files: [], osbl: { name: '', osbls_causes_attributes: [], tax_reduction: '' } } }) -const [openDialog, setOpenDialog] = useState(false) +const [openDialog, setOpenDialog] = useState(false) + +// Cleanup dialog state on unmount +useEffect(() => { + return () => { + setOpenDialog(false) + } +}, []) + +const handleOpenChange = useCallback((open: boolean) => { + setOpenDialog(open) + if (!open) { + clearErrors('contribution.files' as 'contribution') + } +}, [clearErrors])Also applies to: 108-108
154-166: Enhance form validation and submission handling.Consider these improvements to the form submission logic:
+const [isSubmitting, setIsSubmitting] = useState(false) function validateAndOpenDialog (e: React.FormEvent<HTMLFormElement>): void { e.preventDefault() + if (isSubmitting) return + setIsSubmitting(true) + const result = osblValidation.safeParse(data) if (result.success === false) { const issues = result.error.issues + const errorMessages = issues.map(issue => issue.message) issues.forEach(issue => { setError(issue.path.join('.') as 'contribution', issue.message) }) - toast.error('Veuillez corriger les erreurs avant de continuer.') + toast.error( + <div> + <p>Veuillez corriger les erreurs suivantes:</p> + <ul> + {errorMessages.map((msg, i) => ( + <li key={i}>{msg}</li> + ))} + </ul> + </div> + ) + setIsSubmitting(false) return } setOpenDialog(true) + setIsSubmitting(false) }
172-176: Improve accessibility and loading state handling.Consider these enhancements to the UI implementation:
<form onKeyDown={avoidUnintentionalSubmission} onSubmit={(e) => validateAndOpenDialog(e)} - className='2xl:container mx-auto flex flex-col px-2 sm:px-8 md:px-16 pt-8 pb-16 gap-8' + className='2xl:container mx-auto flex flex-col px-2 sm:px-8 md:px-16 pt-8 pb-16 gap-8' + aria-busy={processing} + aria-describedby="form-error-message" > <ContributionDialog open={openDialog} - onOpenChange={setOpenDialog} + onOpenChange={handleOpenChange} comment={data.contribution.body} files={data.contribution.files} setContributionField={setContributionField} onConfirm={handleConfirmSubmit} error={errors['contribution.files' as 'contribution']} clearError={() => clearErrors('contribution.files' as 'contribution')} processing={processing} + aria-label="Confirmation de la contribution" />Also applies to: 211-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
app/frontend/components/pages/contribution/new/ContributionDialog.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDatasheet.tsx(0 hunks)app/frontend/components/pages/contribution/new/OsblHeader.tsx(0 hunks)app/frontend/components/pages/user/show/UserAvatarEdit.tsx(1 hunks)app/frontend/components/shared/MyFileInput.tsx(1 hunks)app/frontend/components/shared/MyInput.tsx(1 hunks)app/frontend/pages/Contribution/New.tsx(7 hunks)app/frontend/pages/Contribution/types.ts(0 hunks)app/models/concerns/attachable_validation.rb(1 hunks)app/models/contribution.rb(2 hunks)package.json(1 hunks)spec/models/contribution_spec.rb(1 hunks)
💤 Files with no reviewable changes (3)
- app/frontend/components/pages/contribution/new/OsblHeader.tsx
- app/frontend/pages/Contribution/types.ts
- app/frontend/components/pages/contribution/new/OsblDatasheet.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (1)
app/models/contribution.rb (1)
2-22: Ensure validation logic is thoroughly testedIncluding
AttachableValidationand usingvalidates_attachmentsenhances the model's robustness. To maintain high code quality, please ensure that unit tests cover the new validation logic, particularly scenarios where files exceed the size limit or fail other validation criteria.Do you have corresponding tests for these validations? If not, consider adding tests to
spec/models/contribution_spec.rbto cover these cases.
app/frontend/components/pages/contribution/new/ContributionDialog.tsx
Outdated
Show resolved
Hide resolved
- Implemented a validation method to limit file attachments to a maximum of 5 - Added RSpec tests to verify file count validation behavior - Ensures contributions cannot have more than 5 attached files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/models/contribution_spec.rb (1)
42-98: LGTM! The test suite is well-structured and comprehensive.The test cases effectively validate both file size and count constraints.
Consider these improvements for better maintainability and coverage:
RSpec.describe Contribution, type: :model do + # Constants for file validation limits + MAX_FILE_SIZE_MB = 5 + MAX_FILE_COUNT = 5 + + # Helper method for attaching test files + def attach_test_files(contribution, count, size = 1.megabyte) + allow_any_instance_of(ActiveStorage::Blob).to receive(:byte_size).and_return(size) + count.times do |i| + contribution.files.attach( + io: StringIO.new, + filename: "file_#{i}.txt", + content_type: "text/plain" + ) + end + end + describe "file attachments" do # ... existing tests ... + + it "is valid without any files attached" do + contribution = build(:contribution) + expect(contribution).to be_valid + end + + it "is invalid with unsupported file type" do + contribution = build(:contribution) + contribution.files.attach( + io: StringIO.new, + filename: "invalid.exe", + content_type: "application/x-msdownload" + ) + expect(contribution).not_to be_valid + expect(contribution.errors[:files]).to be_present + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/contribution.rb(2 hunks)spec/models/contribution_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/contribution.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
- Added `validate` helper function to centralize form validation logic - Created `getAllowedFormats` utility to generate human-readable file format lists - Implemented `UnsavedChangesAlert` component to highlight unsaved modifications - Enhanced type definitions for contribution and OSBL data - Simplified form validation and error handling across multiple components - Updated file input and validation to use new helper functions
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (1)
135-136: LGTM! Consider enhancing the alert behavior.The UnsavedChangesAlert integration is well-placed and properly configured to track changes between the original and current finance data.
Consider adding a confirmation dialog when users click the "Cancel" button (line 227) if there are unsaved changes. This would prevent accidental data loss. Example implementation:
<SheetClose asChild> - <Button variant='ghost' size='lg'> + <Button + variant='ghost' + size='lg' + onClick={(e) => { + if (JSON.stringify(finance) !== JSON.stringify(sheetFinance)) { + if (!window.confirm('You have unsaved changes. Are you sure you want to cancel?')) { + e.preventDefault() + } + } + }} + > <X className='mr-2' /> Annuler </Button> </SheetClose>app/frontend/lib/getAllowedFormats.ts (1)
2-8: Add error handling for edge cases.The current implementation assumes all MIME types follow a specific format. Consider adding error handling for edge cases:
export default function getAllowedFormats (mimeTypes: string[]): string { return mimeTypes.map((mimeType) => { + if (!mimeType.includes('/')) { + return mimeType.toUpperCase() + } // Remove 'image/' prefix and any '+xml' suffix, then convert to uppercase. const format = mimeType.split('/')[1].replace('+xml', '') return format.toUpperCase() }).join(', ') }app/frontend/components/shared/UnsavedChangesAlert.tsx (2)
10-15: Optimize the change detection logic.The current implementation performs unnecessary stringification. Consider using a more efficient approach:
-const UnsavedChangesAlert: React.FC<UnsavedChangesAlertProps> = ({ originalData, currentData }) => { +const UnsavedChangesAlert = <T extends Record<string, unknown>>({ + originalData, + currentData +}: UnsavedChangesAlertProps<T>) => { // Check if there is unsaved change - const hasChanges: boolean = - Object.keys(originalData).length > 0 && - JSON.stringify(originalData) !== JSON.stringify(currentData) + const hasChanges = Object.keys(originalData).length > 0 && + Object.entries(currentData).some(([key, value]) => + originalData[key] !== value + )
23-25: Consider using i18n for alert messages.The alert message is hardcoded in French. Consider using an i18n solution for better maintainability and internationalization support.
app/frontend/hooks/useFormHandler.ts (2)
6-6: Consider using a more type-safe constraint.The change from
T extends objecttoT extends Record<string, any>makes the type more explicit but less type-safe. Consider using a more restrictive type likeRecord<string, unknown>to prevent potential type coercion issues.-interface UseFormHandlerProps<T extends Record<string, any>> { +interface UseFormHandlerProps<T extends Record<string, unknown>> { -export default function useFormHandler<T extends Record<string, any>> ({ +export default function useFormHandler<T extends Record<string, unknown>> ({Also applies to: 21-21
32-34: Consider adding error handling for validation failures.The validation logic could be improved by providing feedback when validation fails.
- if (validation !== undefined && !validate(validation, data, setError)) { - return - } + if (validation !== undefined) { + const isValid = validate(validation, data, setError) + if (!isValid) { + console.warn('Form validation failed') + return + } + }app/frontend/components/shared/MyFileInput.tsx (1)
42-50: Consider adding type guard for File array.The
fileIsPresentfunction could be improved with a more specific type guard for File arrays.-function fileIsPresent (file: File | File[] | undefined): file is File | File[] { +function fileIsPresent (file: File | File[] | undefined): file is File | File[] { if (file instanceof File) { return true } - if (Array.isArray(file) && file.length > 0) { + if (Array.isArray(file) && file.length > 0 && file.every(f => f instanceof File)) { return true } return false }app/frontend/components/pages/contribution/new/ContributionDialog.tsx (1)
69-90: Consider adding loading state for file uploads.The file upload section could benefit from a loading state indicator during file processing.
<div className='mt-8 gap-8 flex flex-col w-full'> <div className='flex flex-col gap-4'> <Label htmlFor='comment'>Commentaire :</Label> <Textarea id='comment' placeholder='Fournissez vos sources, les liens webs qui vous ont servi de référence, ...' value={contribution.body} onChange={handleCommentChange} className='bg-white focus-visible:ring-0 focus-visible:border-primary placeholder:text-ellipsis placeholder:text-xs md:placeholder:text-sm focus-visible:ring-offset-0 w-auto flex-grow h-40' /> </div> <div className='flex flex-col gap-4'> + {processing && <div className='text-sm text-gray-500'>Traitement des fichiers en cours...</div>} <MyFileInput id='files' labelText='Fichiers complémentaires :' multiple onChange={handleFileChange} file={contribution.files} error={error} + disabled={processing} /> </div> </div>app/frontend/pages/Contribution/New.tsx (1)
119-124: Consider using a more type-safe approach.The function could be improved to avoid type assertions.
-function setContributionField (field: keyof OsblCreationData['contribution'], value: OsblCreationData['contribution'][keyof OsblCreationData['contribution']]): void { +function setContributionField<K extends keyof OsblCreationData['contribution']> ( + field: K, + value: OsblCreationData['contribution'][K] +): void { setData('contribution', { ...data.contribution, [field]: value }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
app/frontend/components/pages/contribution/new/ContributionDialog.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDatasheet.tsx(2 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblFinances.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx(2 hunks)app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx(2 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx(3 hunks)app/frontend/components/pages/user/show/UserAvatarEdit.tsx(4 hunks)app/frontend/components/shared/MyFileInput.tsx(1 hunks)app/frontend/components/shared/UnsavedChangesAlert.tsx(1 hunks)app/frontend/hooks/useFormHandler.ts(3 hunks)app/frontend/lib/getAllowedFormats.ts(1 hunks)app/frontend/lib/inertiaConfig.ts(1 hunks)app/frontend/lib/validate.ts(1 hunks)app/frontend/pages/Contribution/New.tsx(6 hunks)app/frontend/pages/Contribution/types.ts(1 hunks)db/seeds.rb(1 hunks)vite.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- app/frontend/components/pages/contribution/new/OsblDocuments.tsx
- vite.config.ts
- app/frontend/components/pages/contribution/new/OsblFinances.tsx
- app/frontend/components/pages/contribution/new/OsblLocations.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/frontend/components/pages/user/show/UserAvatarEdit.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/frontend/components/shared/UnsavedChangesAlert.tsx
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 7-7: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (16)
app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (1)
3-3: LGTM!The import statement follows the project's conventions and is properly placed with other component imports.
app/frontend/lib/validate.ts (1)
3-21: LGTM! Well-implemented generic validation function.The implementation is clean, type-safe, and follows Zod's best practices for validation and error handling.
app/frontend/lib/inertiaConfig.ts (1)
11-13: LGTM! Improved type safety and validation.The changes improve type safety by removing null type and use a more appropriate length check for empty strings.
app/frontend/pages/Contribution/types.ts (2)
51-51: LGTM! Good use of type alias.Using a type alias from the nested type improves code reusability and maintainability.
59-64: LGTM! Good use of literal types.Using literal types for
tax_reduction,geographical_scale, andosbl_typeimproves type safety and provides better IntelliSense support.app/frontend/components/pages/contribution/new/ContributionDialog.tsx (1)
41-50: 🛠️ Refactor suggestionAdd file validation in handleFileChange.
Consider adding file validation before updating the contribution field.
const handleFileChange = (file: File | File[] | undefined): void => { + const MAX_FILES = 5 + const MAX_SIZE = 5 * 1024 * 1024 // 5MB + if (file === undefined) { setContributionField('files', []) } else if (Array.isArray(file)) { + if (file.length > MAX_FILES) { + console.warn(`Too many files selected: ${file.length}/${MAX_FILES}`) + return + } + if (file.some(f => f.size > MAX_SIZE)) { + console.warn('Some files exceed size limit') + return + } setContributionField('files', file) } else { + if (file.size > MAX_SIZE) { + console.warn('File exceeds size limit') + return + } setContributionField('files', [file]) } clearError() }Likely invalid or redundant comment.
app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx (2)
81-81: LGTM! Enhanced key uniqueness.The key now includes
item.type, making it more unique and stable for React's reconciliation process.
134-134: LGTM! Improved color contrast.The color change from
text-red-500totext-red-600provides better visibility.app/frontend/pages/Contribution/New.tsx (2)
201-210: LGTM! Well-structured dialog integration.The dialog component is properly integrated with all necessary props for state management and error handling.
37-44: LGTM! Well-structured file validation.The validation schema effectively enforces file size and count limits.
✅ Verification successful
🌐 Web query:
What is the recommended maximum file upload size for web applications?💡 Result:
The recommended maximum file upload size for web applications typically balances technical constraints, security considerations, and user needs. Here's a structured analysis: ## General Recommendations - **2 GB** is widely considered a safe upper limit for browser compatibility[1][5], though some modern browsers like Chrome/Opera support up to 4 GB[5]. - Most web servers default to lower limits (**4-30 MB**) that require manual configuration adjustments[3][5]. ## Key Considerations **Technical Factors:** - Server configurations (IIS defaults to 4MB-28.6MB[3]) - Browser limitations (2GB standard, up to 4GB in Chrome/Opera)[5] - File system constraints (NTFS supports files up to 16TB)[1] **Security Best Practices:** - Prevent DoS attacks by limiting upload sizes[1][8] - Mitigate resource exhaustion with chunked uploads[2][5] - Implement client/server-side validation[7][8] **Service-Specific Examples:** | Service Type | Typical Limit | |----------------------|---------------------| | General Web Apps | 2GB (browser max)[5] | | Enterprise Storage | Up to 500GB[9] | | Email Attachments | 20-25MB[5] | | Security-Focused Apps| 10MB[7][8] | ## Implementation Guidance 1. **Server Configuration** Adjust web server settings (e.g., `maxAllowedContentLength` in IIS[3], `upload_max_filesize` in PHP[4]) 2. **Progressive Enhancement** Use chunked uploads for files >100MB[2][5] 3. **Validation Layers** ```javascript // Client-side example const maxSize = 10 * 1024 * 1024; // 10MB if(file.size > maxSize) alert('File too large')[7]; ``` For most applications, **10-100MB** strikes a balance between usability and security[7][8], while specialized services might enable larger uploads through chunking and cloud storage integration[5][9]. Always combine size limits with file type validation and malware scanning[1][8]. Citations: - 1: https://themightyprogrammer.dev/article/uploading-files - 2: https://bluegoatcyber.com/blog/effective-file-upload-validation-techniques-for-web-security/ - 3: https://www.inflectra.com/Support/KnowledgeBase/KB306.aspx - 4: https://community.localwp.com/t/maximum-upload-file-size/15812 - 5: https://uploadcare.com/blog/handling-large-file-uploads/ - 6: https://help.openai.com/en/articles/8983719-what-are-the-file-upload-size-restrictions - 7: https://transloadit.com/devtips/optimizing-file-uploads-in-web-applications/ - 8: https://blog.pixelfreestudio.com/best-practices-for-secure-file-uploads-on-websites/ - 9: https://support.box.com/hc/en-us/articles/360043697314-Understand-the-Maximum-File-Size-You-Can-Upload-to-Box - 10: https://learn.microsoft.com/en-us/azure/web-application-firewall/ag/application-gateway-waf-request-size-limits
Final Verification: File Validation is Well-Structured
The file validation logic in app/frontend/pages/Contribution/New.tsx is correctly implemented. It enforces:
- A maximum of 5 files per upload.
- A per-file size limit of 5 MB (as indicated by the error message), which is a conservative choice aligned with constraints often seen in web applications.
Although general recommendations for file upload sizes can range from 10 MB up to even 2GB depending on the context, the choice here appears to be deliberate and in line with security-focused and resource-aware design.
app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx (1)
105-105: LGTM! Good UX improvement.The UnsavedChangesAlert component helps prevent accidental data loss by warning users about unsaved changes.
app/frontend/components/pages/contribution/new/OsblDatasheet.tsx (2)
53-70: LGTM! Well-structured tax reduction configuration.The TaxReductionList constant improves maintainability and provides clear documentation through tooltips.
113-128: LGTM! Improved radio group implementation.The refactored code is more maintainable and follows DRY principles by mapping over TaxReductionList.
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx (3)
3-3: LGTM!The imports are correctly added and match their usage in the code.
Also applies to: 34-34
47-48: LGTM! Improved validation error messages.The validation schema now provides clearer error messages with dynamic allowed formats.
105-105: LGTM! Added unsaved changes alert.The
UnsavedChangesAlertcomponent is correctly integrated to notify users of unsaved changes.
- Updated UnsavedChangesAlert to use generic type parameter for improved type safety - Modified component signature to support type-specific comparisons - Refactored imports and component definition to use React functional component pattern - Applied generic typing across OSBL sheet components (Documents, Finances, Locations)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx (1)
45-49: Enhance file validation robustness.While the current validation is good, consider these improvements:
- Add null/undefined check
- Add MIME type validation using file signatures
const documentValidation = z.object({ - file: z.instanceof(File) + file: z.instanceof(File) + .refine((file) => file != null, 'Le fichier est requis.') + .refine(async (file) => { + // Read first few bytes to check file signature + const arr = new Uint8Array(await file.slice(0, 4).arrayBuffer()) + const header = Array.from(arr).map(byte => byte.toString(16)).join('') + // PDF signature: %PDF (25 50 44 46) + return header.startsWith('25504446') + }, 'Le fichier n\'est pas un PDF valide.') .refine((file) => file.size <= MAX_DOCUMENT_SIZE, 'La taille du fichier doit être inférieure à 5 MB.') .refine((file) => ALLOWED_DOCUMENT_TYPES.includes(file.type), `Le type de fichier est invalide. Format accepté : ${getAllowedFormats(ALLOWED_DOCUMENT_TYPES)}.`) })app/frontend/components/shared/UnsavedChangesAlert.tsx (2)
10-15: Consider using a more robust deep equality comparison.The current change detection using
JSON.stringifycan be unreliable in certain cases:
- Objects with methods or undefined values
- Objects with different property orders but same content
- Objects with circular references
Consider using a deep equality library like
lodash/isEqualor implementing a custom deep comparison:- Object.keys(originalData).length > 0 && - JSON.stringify(originalData) !== JSON.stringify(currentData) + Object.keys(originalData).length > 0 && + !isEqual(originalData, currentData)Add the import:
import { isEqual } from 'lodash'
20-28: Consider using internationalization for the alert message.The alert message is hardcoded in French. For better maintainability and potential internationalization needs, consider using a translation system.
Example using a translation hook:
+import { useTranslation } from 'react-i18next' function UnsavedChangesAlert<T extends object> ({ originalData, currentData }: UnsavedChangesAlertProps<T>): ReactElement | null { + const { t } = useTranslation() // ... return ( <Alert className='mt-4 border-red-600'> <CircleAlert className='text-red-600' /> <AlertDescription> - Vos modifications doivent être validées pour être enregistrées. + {t('alerts.unsaved_changes')} </AlertDescription> </Alert> ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx(2 hunks)app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx(3 hunks)app/frontend/components/shared/UnsavedChangesAlert.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx
- app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (4)
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx (2)
3-4: LGTM!The new imports are properly organized and placed with related imports.
105-105: LGTM!The UnsavedChangesAlert component is properly integrated with correct type parameters and props.
app/frontend/components/shared/UnsavedChangesAlert.tsx (2)
1-8: LGTM!The imports are appropriate, and the interface is well-defined using generics for type safety.
16-19: LGTM!The early return pattern is well-implemented and follows React best practices.
mpressen
deleted the
162-add-a-modal-when-submitting-an-osbl-creation-to-get-metadata-about-the-contribution
branch
Summary by CodeRabbit
New Features
ContributionDialogcomponent for user contributions.UnsavedChangesAlertcomponents to notify users of unsaved changes across various forms.Enhancements
Dependency Updates
Tests