Osbl creation fourth panel locations #153
Conversation
- Implemented OsblLocations component for managing organization locations - Created Location and Address models with polymorphic associations - Added database migrations for locations and addresses - Updated Osbl model to support nested location attributes - Integrated location input with address autocomplete using French address API - Enhanced contribution form to include location details with type selection and optional information This commit introduces comprehensive location management for organizations, improving data collection and representation in the contribution form.
- Added support for multiple annual finance entries in the contribution form - Introduced add and remove functionality for finance records - Updated OsblFinance component to handle dynamic finance entries - Improved error handling for multiple finance records - Enhanced UI with card-based layout and action buttons for finance entries
…ness - Updated OsblHeader, OsblDataSheet, OsblDocuments, OsblFinance, and OsblLocations components with consistent layout and styling - Simplified container structure and removed unnecessary wrapper divs - Improved responsive design with grid layout in the main contribution form - Enhanced button styling and added responsive text visibility - Removed unused imports and simplified component structure - Updated button variant to improve hover state
…oved validation - Introduced OsblFinanceSheet component for detailed finance entry management - Added dynamic sheet-based editing for annual finance records - Improved form validation with more granular year and percentage checks - Enhanced error handling for finance entries with better error propagation - Updated MyNumberInput to support optional error message display - Refactored finance form to support more robust data entry and validation
- Implemented year-based sorting for annual finance records - Added a toggle button to sort finance entries in ascending or descending order - Introduced state management for sorting direction - Disabled sorting when there are validation errors - Updated OsblFinance component to support dynamic sorting of finance entries
- Modified sidebar component to use relative positioning - Updated inset and height classes for better layout - Removed top border in sidebar to improve visual consistency - Fine-tuned transition and positioning classes
…tion control - Created a new MyLink component to wrap Inertia Link - Added onBefore handler to prevent unnecessary page reloads - Replaced all Link components with MyLink in Header, Footer, and AppSidebar - Imported MyLink in layout and navigation components
- Integrated useSidebar hook in AppSidebar component - Implemented handleClick method to close mobile sidebar when menu item is selected - Ensured sidebar closes automatically on mobile devices after navigation
… support - Added SheetTitle with screen reader-only class to provide a descriptive title - Included SheetDescription with screen reader-only class to explain sidebar navigation purpose - Imported additional components from sheet UI component
- Refactored FundManagementSection component for better responsiveness - Added Separator component between fund management entries - Adjusted layout classes for improved mobile and desktop display - Simplified icon rendering and button styling - Made sheet description screen reader-only in OsblFinanceSheet - Updated button size and margin in finance sheet footer
- Integrated Sonner toast library for displaying validation error messages - Added error toast when form validation fails - Improved user feedback for form submission errors
- Adjusted header and button layout for improved responsiveness - Uncommented and added an informative alert for contribution submission - Added a second submit button at the bottom of the form for easier access - Fine-tuned text and gap sizes for better mobile and desktop display
- Updated validation schema for more precise error messages - Added visual error indicators for description length and cause selection - Modified AnnualFinance model to exclude 'certified' field from validation - Added validation for causes presence in Osbl model - Refined error styling across various form components
- Introduced OsblDocumentSheet component for detailed document management - Replaced complex card-based document form with a more user-friendly sheet interface - Improved document type handling and validation - Added dynamic document display with edit and remove functionality - Enhanced file input and additional information sections - Updated form validation to support more precise document entry requirements
…nd UX - Refactored MyInput to support more flexible input attributes - Updated MyFileInput to use new MyInput component - Added reset button functionality for file and text inputs - Extracted document type list to a constants file - Improved type definitions for form components - Simplified document handling in various form sections
- Simplified FundManagementSection component by removing redundant error handling - Updated OsblFinanceSheet to use local state and improved validation - Moved fund type lists to constants file - Removed unnecessary error tracking and simplified sheet interactions - Enhanced type safety and reduced complexity in finance-related components
- Added custom CSS animations for CollapsibleContent - Increased transition duration for ChevronDown icon - Added class to CollapsibleContent for better overflow handling - Implemented slide up and down animations for collapsible section
…ved validation - Introduced OsblLocationSheet component for detailed location management - Replaced complex card-based location form with a more user-friendly sheet interface - Added dynamic location display with edit and remove functionality - Improved location type and address validation - Extracted location type list to constants file - Updated location model and database constraints for more precise validation - Enhanced MyAsyncSelect component to support pre-filled values
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 RuboCop (1.69.1)app/models/location.rb[!] There was an error parsing from /Gemfile:17-------------------------------------------# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
-------------------------------------------/usr/lib/ruby/3.1.0/bundler/dsl.rb:357:in WalkthroughThe changes update the contribution controller’s parameter handling, expand nested location and address attributes, and remove the contact email. Multiple frontend components have been refactored to use shared UI elements (e.g., MyLink, MyInput, MyFileInput) and improved import paths. New components and modifications enhance form interactivity in user authentication and OSBL contribution pages (datasheet, documents, finances, locations). Additionally, models, migrations, and schema have been updated with new validations, associations, and constraints while the test suites, factories, and CI configurations have been revised to match the updated codebase structure. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant F as Contribution Form
participant C as ContributionsController
participant DB as Database
U->>F: Fill in OSBL form (datasheet, documents, finances, locations)
F->>C: Submit form data with nested attributes
C->>DB: Validate and save OSBL + related records
DB-->>C: Return persisted record or errors
C-->>F: Send success response or validation errors
F->>U: Display outcome / error messages
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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: 10
🧹 Nitpick comments (40)
app/frontend/components/forms/MyNumberInput.tsx (1)
86-86: Consider using null coalescing for error check.The error presence check could be simplified using the null coalescing operator.
-{Boolean(error) && !noErrorMessage && <InputError>{error}</InputError>} +{error && !noErrorMessage && <InputError>{error}</InputError>}app/frontend/components/pages/contribution/new/FundManagementSection.tsx (3)
37-47: Add value validation before clearing errors.The error clearing in handleFundChange could be premature as it doesn't validate the value before clearing the total_percent error.
Consider validating the value before clearing errors:
function handleFundChange (index: number, field: keyof FundRecord, value: any): void { const updatedItems = items.map((item: FundRecord, i: number) => i === index ? { ...item, [field]: value } : item ) onUpdate(updatedItems) if (field === 'percent') { - clearErrors(`${baseErrorPath}.total_percent`) + const isValidPercent = !isNaN(value) && value >= 0.01 && value <= 100 + if (isValidPercent) { + clearErrors(`${baseErrorPath}.total_percent`) + } } }
91-95: Extract error class to improve maintainability.The error class string could be extracted to improve readability and maintainability.
Consider refactoring like this:
+ const errorClass = errors[`${baseErrorPath}.${index}.type`] !== undefined ? 'border-red-600' : '' <SelectTrigger - className={`w-60 data-[placeholder]:text-muted-foreground mt-4 ${ - errors[`${baseErrorPath}.${index}.type`] !== undefined ? 'border-red-600' : '' - }`} + className={`w-60 data-[placeholder]:text-muted-foreground mt-4 ${errorClass}`} >
64-72: Add tooltip to explain disabled state.When the add button is disabled, users might not understand why. A tooltip would improve the user experience.
Consider adding a tooltip:
+ import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' - <Button - onClick={handleAdd} - variant='outline' - className='bg-white' - disabled={items.length >= typeList.length} - > - <PlusIcon /> - Ajouter - </Button> + <Tooltip> + <TooltipTrigger asChild> + <Button + onClick={handleAdd} + variant='outline' + className='bg-white' + disabled={items.length >= typeList.length} + > + <PlusIcon /> + Ajouter + </Button> + </TooltipTrigger> + <TooltipContent> + {items.length >= typeList.length ? 'Tous les types de fonds ont été ajoutés' : 'Ajouter un nouveau fonds'} + </TooltipContent> + </Tooltip>app/frontend/components/shared/MyFileInput.tsx (2)
24-30: Consider using controlled state pattern.The component mixes controlled (
fileprop) and uncontrolled (hasFilestate) patterns. Consider derivinghasFilefrom thefileprop to maintain a single source of truth.-const [hasFile, setHasFile] = useState(false) +const hasFile = file !== undefined const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>): void => { const fileAttached = (e.target.files != null) && e.target.files.length > 0 - setHasFile(!!fileAttached) onChange(e.target.files?.[0] ?? undefined) }
32-41: Simplify reset logic.The event creation and property definition is unnecessary since we're now using direct file handling.
const handleReset = (): void => { if (inputRef.current != null) { inputRef.current.value = '' - setHasFile(false) - // Trigger onChange with a fake event to notify parent component - const event = new Event('change', { bubbles: true }) as unknown as React.ChangeEvent<HTMLInputElement> - Object.defineProperty(event, 'target', { value: { value: '', files: null } }) } onChange(undefined) }app/frontend/components/forms/MyInput.tsx (1)
55-64: Add keyboard interaction for reset button.While the reset button is accessible via mouse, consider adding keyboard interaction handling.
<button type='button' onClick={onReset} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + onReset() + } + }} className='absolute right-2 top-1/2 -translate-y-1/2 rounded-full p-1 hover:bg-gray-100' aria-label='Reset input value' > <X className='h-4 w-4 text-gray-500' /> </button>app/frontend/components/pages/contribution/new/OsblHeader.tsx (1)
64-77: Consider adding character count feedback.While the maxLength is enforced, consider providing visual feedback when approaching the limit.
<div className='text-xs text-right'> - {(data.description ?? '').length}/300 caractères + <span className={`${(data.description?.length ?? 0) >= 250 ? 'text-orange-500' : ''}`}> + {(data.description ?? '').length}/300 caractères + </span> </div>app/frontend/entrypoints/application.css (1)
171-179: Consider adding vendor prefixes and reduced motion support.The CollapsibleContent implementation looks good but could be enhanced for better browser support and accessibility.
Consider adding:
- Vendor prefixes for wider browser support
- Reduced motion media query for accessibility:
.CollapsibleContent { overflow: hidden; + -webkit-overflow-scrolling: touch; } .CollapsibleContent[data-state="open"] { animation: slideDown 300ms ease-out; + -webkit-animation: slideDown 300ms ease-out; } .CollapsibleContent[data-state="closed"] { animation: slideUp 300ms ease-out; + -webkit-animation: slideUp 300ms ease-out; } + +@media (prefers-reduced-motion: reduce) { + .CollapsibleContent[data-state="open"], + .CollapsibleContent[data-state="closed"] { + animation: none; + -webkit-animation: none; + } +}app/frontend/components/pages/contribution/new/OsblDocuments.tsx (2)
41-61: UI block for adding a new document.
The usage of<Sheet>and<SheetTrigger>is well-structured. The inline keydocument-${documents.length}is fine here, but a stable unique identifier may be more robust if documents can be rearranged or updated in different orders.
63-93: Efficient mapping of document items.
Your mapping approach for displaying, editing, and removing each document is clear. However, usingindexin thekeymight cause unintended re-renders if the array is reordered. If each document object has an ID, it might be preferable to use that as a key.app/frontend/components/pages/contribution/new/OsblFinance.tsx (2)
27-34: Sorting logic is clear yet watch for ties.
TogglingsortAscendingand performing a straightforward numeric sort is fine. If multiple items have the sameyear, note that their relative order is not guaranteed. This might not matter for your use case, but be mindful if a stable sort becomes necessary.
76-113: Validate keys and data continuity in the finances array.
Using theyearas a key is acceptable if each year is guaranteed to be unique. If your domain allows for multiple items with the same year, consider a different unique identifier.spec/factories/locations.rb (1)
2-7: Enhance factory flexibility with traits and associations.The factory could be improved in two ways:
- Add traits for different location types to make testing various scenarios easier
- Include the
osblassociation mentioned in the AI summaryConsider this implementation:
factory :location do - type { 1 } + association :osbl + name { "MyString" } description { "MyText" } website { "MyString" } + + trait :type_one do + type { 1 } + end + + trait :type_two do + type { 2 } + end + + trait :type_three do + type { 3 } + end + + trait :other_type do + type { 4 } + name { nil } # Name can be null for other types + end enddb/migrate/20250203115228_add_more_constraint_to_location.rb (1)
1-5: Consider using constants/enums for type values.The constraint uses magic numbers (1, 2, 3) for types. This could be made more maintainable by:
- Using constants or enums to define these types
- Adding a comment explaining what each type represents
Example approach:
# In app/models/location.rb class Location < ApplicationRecord enum type: { headquarters: 1, branch: 2, warehouse: 3, other: 4 } end # Then in the migration class AddMoreConstraintToLocation < ActiveRecord::Migration[8.0] def change # Types 1-3 (headquarters, branch, warehouse) require a name add_check_constraint :locations, "type NOT IN (1, 2, 3) OR (type IN (1, 2, 3) AND name IS NOT NULL)", name: "name_required_for_specific_types" end endspec/factories/addresses.rb (1)
2-11: Enhance data realism with Faker and add validation.Consider these improvements:
- Use Faker for more realistic address data
- Add validation for country format
- Add traits for different scenarios
Example implementation:
factory :address do - street_number { "MyString" } - street_name { "MyString" } - postal_code { "MyString" } - city { "MyString" } - country { "MyString" } - latitude { 1.5 } - longitude { 1.5 } + street_number { Faker::Address.building_number } + street_name { Faker::Address.street_name } + postal_code { Faker::Address.postcode } + city { Faker::Address.city } + country { Faker::Address.country_code } + latitude { Faker::Address.latitude } + longitude { Faker::Address.longitude } addressable { nil } + + trait :with_location do + association :addressable, factory: :location + end + + trait :invalid_country do + country { "Invalid" } + end endapp/frontend/components/shared/MyLink.tsx (2)
4-4: Add prop types documentation.Consider adding JSDoc comments to document the component's props for better maintainability.
+/** + * Props for MyLink component, extending all props from Inertia's Link component + */ type MyLinkProps = ComponentProps<typeof Link>
6-14: Optimize component performance and readability.Consider these improvements:
- Simplify the handleBefore function
- Add memoization for performance
-export default function MyLink ({ href, ...props }: MyLinkProps): ReactElement { +const MyLink = ({ href, ...props }: MyLinkProps): ReactElement => { const { url } = usePage() - const handleBefore = (): boolean => { - return !(href === url) - } + const handleBefore = React.useCallback((): boolean => href !== url, [href, url]) return <Link href={href} onBefore={handleBefore} {...props} /> } + +export default React.memo(MyLink)db/migrate/20250126104700_create_locations.rb (1)
14-18: Consider adding a general index on osbl_id.While you have a partial index for siege_social type, a general index on osbl_id would improve query performance when fetching all locations for an osbl.
Add this index:
add_index :locations, :osbl_id, unique: true, where: "type = 0", name: "index_locations_on_osbl_id_siege_social" + add_index :locations, :osbl_id # Add general index for all queriesapp/models/location.rb (1)
17-17: Simplify name presence validation.The validation condition can be simplified using the enum scope.
- validates :name, presence: true, if: -> { type.in?(%i[antenne_locale lieu_d_activite autre]) } + validates :name, presence: true, if: :requires_name? + + private + + def requires_name? + !siege_social? + enddb/migrate/20250126140903_create_addresses.rb (1)
12-12: Remove commented code.The commented reference line can be removed as you've implemented the polymorphic association manually.
- # t.references :addressable, polymorphic: true, null: falseapp/frontend/lib/deepCleanData.ts (1)
6-10: LGTM! Consider extracting the filter conditions into a helper function.The changes consistently enhance filtering logic by removing empty strings in both array and object processing sections.
Consider extracting the common filter conditions into a helper function to improve maintainability:
+function shouldKeepValue(value: any): boolean { + return value !== undefined && + value !== '' && + !(typeof value === 'object' && !isFile(value) && Object.keys(value).length === 0) +} export default function deepCleanData (obj: any): any { if (Array.isArray(obj)) { return obj .map(deepCleanData) - .filter((item) => - item !== undefined && - item !== '' && - !(typeof item === 'object' && !isFile(item) && Object.keys(item).length === 0) - ) + .filter(shouldKeepValue) } else if (typeof obj === 'object' && obj !== null) { if (isFile(obj)) { return obj } const cleanedObject = Object.fromEntries( Object.entries(obj) .map(([key, value]) => [key, deepCleanData(value)]) - .filter(([_, value]) => - value !== undefined && - value !== '' && - !(typeof value === 'object' && !isFile(value) && Object.keys(value).length === 0) - ) + .filter(([_, value]) => shouldKeepValue(value)) )Also applies to: 20-24
app/frontend/lib/constants.ts (1)
1-28: LGTM! Consider adding TypeScript interfaces for better type safety.The constants are well-structured with consistent naming conventions and proper French capitalization.
Consider adding TypeScript interfaces to improve type safety:
interface ListItem { value: string label: string group?: string } export const DocumentTypeList: readonly ListItem[] = [ { value: 'statuts', label: 'Statuts' }, // ... ] as const export const FundSourceTypeList: readonly ListItem[] = [ { value: 'dons', label: 'Dons', group: 'main' }, // ... ] as const // Apply similar changes to other constantsapp/frontend/pages/Contribution/types.ts (2)
33-41: Consider making latitude/longitude optional in Address interface.These coordinates might not be immediately available when creating an address and could be populated asynchronously through geocoding.
export interface Address { street_number?: string street_name: string additional_info?: string postal_code: string city: string - latitude: number - longitude: number + latitude?: number + longitude?: number }
43-49: Consider using a union type for the Location type field.This would provide better type safety by restricting the allowed values to those defined in
LocationTypeList.import { LocationTypeList } from '../../lib/constants' type LocationType = typeof LocationTypeList[number]['value'] export interface Location { type: LocationType address_attributes: Address name?: string description?: string website?: string }app/frontend/components/layout/AppSidebar.tsx (1)
37-43: LGTM! Mobile sidebar handling looks good.The mobile sidebar state management is well-implemented, with proper cleanup of the mobile state when navigating.
Consider memoizing the
handleClickfunction withuseCallbacksince it's passed as a prop to child components:- const handleClick = (): void => { + const handleClick = useCallback((): void => { if (isMobile) { setOpenMobile(false) } - } + }, [isMobile, setOpenMobile])app/frontend/components/shared/MyAsyncSelect.tsx (1)
41-41: Remove console.log statement.Debug statements should not be present in production code.
- console.log('inputValue', inputValue)app/frontend/components/pages/contribution/new/OsblLocations.tsx (1)
16-21: Consider using immutable state updates.The current implementation mutates the array directly which could lead to React state issues.
Consider this alternative implementation:
-function addLocation (location: Location, index: number): void { - const updatedLocations = [...locations] - updatedLocations[index] = location - - setData('locations_attributes', updatedLocations) -} +function addLocation (location: Location, index: number): void { + setData('locations_attributes', + locations.map((loc, i) => i === index ? location : loc) + ) +}app/frontend/pages/Contribution/New.tsx (1)
33-33: Consider using more descriptive error message.The error message for tax_reduction could be more specific about the expected values.
- tax_reduction: z.enum(['intérêt_général', 'aide_aux_personnes_en_difficulté'], { message: 'Veuillez sélectionner un pourcentage.' }) + tax_reduction: z.enum(['intérêt_général', 'aide_aux_personnes_en_difficulté'], { message: 'Veuillez sélectionner entre "intérêt général" ou "aide aux personnes en difficulté".' })app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx (4)
57-62: Consider typing the value parameter.The
valueparameter in updateSheetLocation is typed asanywhich could lead to type safety issues.-function updateSheetLocation (field: keyof Location, value: any): void { +function updateSheetLocation<T extends Location[keyof Location]> (field: keyof Location, value: T): void { setSheetLocation(prev => ({ ...prev, [field]: value })) }
64-71: Type the handleAddressChange parameters.Similar to the previous comment, the function uses
anytype.-function handleAddressChange (value: any, field?: string): void { +function handleAddressChange (value: Record<string, unknown>, field?: keyof Location['address_attributes']): void { setSheetLocation(prev => ({ ...prev, address_attributes: field != null ? { ...prev.address_attributes, [field]: value } : value })) }
73-81: Consider extracting form validation logic.The form validation logic could be extracted into a separate function for better maintainability.
+function validateForm(formRef: React.RefObject<HTMLFormElement>): boolean { + return formRef.current?.reportValidity() ?? false +} + function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): void { - if (formRef.current?.reportValidity() === false) { + if (!validateForm(formRef)) { e.preventDefault() return } onUpdate(deepCleanData(sheetLocation)) }
82-90: Consider memoizing getLabel function.The getLabel function is used in the render method and could benefit from memoization to prevent unnecessary recalculations.
+const getLabel = useMemo(() => { + return [ + sheetLocation.address_attributes?.street_number, + sheetLocation.address_attributes?.street_name, + sheetLocation.address_attributes?.postal_code, + sheetLocation.address_attributes?.city + ].filter(Boolean).join(' ') +}, [sheetLocation.address_attributes])Also applies to: 137-145
app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx (3)
28-72: Consider improving type safety and validation logic.The validation schema has a few areas that could be improved:
- The
anyreturn type in the validation schema should be replaced with a specific type.- The validation for fund sources and allocations is duplicated.
Apply this diff to improve type safety and reduce duplication:
-const financeValidation = (data: AnnualFinance[], currentIndex: number): z.ZodType<any> => z.object({ +type FinanceSchema = { + year: string; + fund_sources_attributes?: Array<{ percent: string }>; + fund_allocations_attributes?: Array<{ percent: string }>; +}; + +const validatePercentages = (items: Array<{ percent: string }> | undefined, errorPath: string[]) => + z.array(z.object({ + percent: z.string() + })).optional() + .refine((items): items is typeof items => { + if (items === undefined || items.length === 0) return true; + const sum = items.reduce((acc, item) => acc + (Number(item.percent) || 0), 0); + return sum === 100; + }, { + message: 'La somme des pourcentages doit être égale à 100%.', + path: errorPath + }); + +const financeValidation = (data: AnnualFinance[], currentIndex: number): z.ZodType<FinanceSchema> => z.object({ year: z.string().refine( (year) => !data.some((f, i) => i !== currentIndex && f.year === Number(year)), { message: 'Un bilan comptable est déjà enregistré pour cette année.', path: ['annual_finances_attributes', currentIndex, 'year'] } ), - fund_sources_attributes: z.array(z.object({ - percent: z.string() - })).optional() - .refine((sources): sources is typeof sources => { - if (sources === undefined || sources.length === 0) return true - - const sum = sources?.reduce((acc: number, source) => { - const percent = source.percent !== undefined ? Number(source.percent) : 0 - return acc + percent - }, 0) - return sum === 100 - }, { - message: 'La somme des pourcentages doit être égale à 100%.', - path: ['total_percent'] - }), - fund_allocations_attributes: z.array(z.object({ - percent: z.string() - })).optional() - .refine((allocations): allocations is typeof allocations => { - if (allocations === undefined || allocations.length === 0) return true - - const sum = allocations?.reduce((acc: number, allocation) => { - const percent = allocation.percent !== undefined ? Number(allocation.percent) : 0 - return acc + percent - }, 0) - return sum === 100 - }, { - message: 'La somme des pourcentages doit être égale à 100%.', - path: ['total_percent'] - }) + fund_sources_attributes: validatePercentages(sources, ['total_percent']), + fund_allocations_attributes: validatePercentages(allocations, ['total_percent'])
104-121: Consider improving error handling in form submission.The form submission logic could be improved by:
- Adding a loading state during submission
- Handling multiple validation errors more gracefully
Apply this diff to improve error handling:
+const [isSubmitting, setIsSubmitting] = useState(false); function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): void { + setIsSubmitting(true); if (formRef.current?.reportValidity() === false) { e.preventDefault(); + setIsSubmitting(false); return; } const result = financeValidation(data.annual_finances_attributes ?? [], index).safeParse(sheetFinance) if (!result.success) { e.preventDefault(); + clearErrors(); // Clear previous errors const issues = result.error.issues issues.forEach(issue => { setError(issue.path.join('.'), issue.message) }) + setIsSubmitting(false); return; } onUpdate(deepCleanData(sheetFinance) as AnnualFinance) + setIsSubmitting(false); }Then update the submit button to show loading state:
-<Button onClick={handleSubmit} variant='default' size='lg'> +<Button + onClick={handleSubmit} + variant='default' + size='lg' + disabled={isSubmitting} +> <CheckIcon className='mr-2' /> - Valider + {isSubmitting ? 'Validation...' : 'Valider'} </Button>
133-154: Consider improving accessibility for the year input.The year input could be improved by:
- Adding aria-describedby for the tooltip
- Using aria-invalid for error state
Apply this diff to improve accessibility:
<MyNumberInput autoFocus id={`year-${index}`} + aria-describedby={`year-tooltip-${index}`} + aria-invalid={!!errors[`annual_finances_attributes.${index}.year`] || !!errors[`annual_finances_attributes.${index}.missing_information`]} labelText={ <> Année * - <HelpTooltip size='small' className='mx-2'> + <HelpTooltip id={`year-tooltip-${index}`} size='small' className='mx-2'> <p>Année du bilan comptable.</p> <p>Si à cheval sur deux ans, entrez la dernière année.</p> </HelpTooltip> : </> }app/frontend/components/pages/contribution/new/OsblDocumentSheet.tsx (2)
40-51: Consider improving file validation.The file validation could be enhanced by:
- Using constants for error messages
- Adding more file type validations
- Adding a minimum file size check
Apply this diff to improve file validation:
const MAX_DOCUMENT_SIZE = 5 * 1024 * 1024 // 5MB +const MIN_DOCUMENT_SIZE = 1024 // 1KB const ALLOWED_DOCUMENT_TYPES = ['application/pdf'] +const ERROR_MESSAGES = { + FILE_TOO_LARGE: 'La taille du fichier doit être inférieure à 5 MB.', + FILE_TOO_SMALL: 'La taille du fichier doit être supérieure à 1 KB.', + INVALID_TYPE: 'Le type de fichier est invalide. Format accepté : PDF.', + CORRUPTED: 'Le fichier semble être corrompu.', +} as const; const documentValidation = z.object({ file: z.instanceof(File) + .refine((file) => file.size >= MIN_DOCUMENT_SIZE, ERROR_MESSAGES.FILE_TOO_SMALL) - .refine((file) => { - return file.size <= MAX_DOCUMENT_SIZE - }, 'La taille du fichier doit être inférieure à 5 MB.') + .refine((file) => file.size <= MAX_DOCUMENT_SIZE, ERROR_MESSAGES.FILE_TOO_LARGE) - .refine((file) => { - return ALLOWED_DOCUMENT_TYPES.includes(file.type) - }, 'Le type de fichier est invalide. Format accepté : PDF.') + .refine((file) => ALLOWED_DOCUMENT_TYPES.includes(file.type), ERROR_MESSAGES.INVALID_TYPE) + .refine(async (file) => { + try { + const arrayBuffer = await file.arrayBuffer(); + const bytes = new Uint8Array(arrayBuffer); + // Check PDF magic number (%PDF-) + return bytes.length >= 5 && + bytes[0] === 0x25 && // % + bytes[1] === 0x50 && // P + bytes[2] === 0x44 && // D + bytes[3] === 0x46 && // F + bytes[4] === 0x2D; // - + } catch { + return false; + } + }, ERROR_MESSAGES.CORRUPTED) })
79-94: Consider improving form submission error handling.The form submission error handling could be improved by:
- Adding a loading state
- Handling multiple validation errors
- Adding error boundary for unexpected errors
Apply this diff to improve error handling:
+const [isSubmitting, setIsSubmitting] = useState(false); async function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): Promise<void> { + setIsSubmitting(true); if (formRef.current?.reportValidity() === false) { e.preventDefault(); + setIsSubmitting(false); return; } - const result = documentValidation.safeParse({ file: sheetDocument.file }) - if (!result.success) { - e.preventDefault(); - const errorMessage = result.error.issues[0].message - setError(`document_attachments_attributes.${index}.document_attributes.file`, errorMessage) - return; - } + try { + const result = await documentValidation.safeParseAsync({ file: sheetDocument.file }); + if (!result.success) { + e.preventDefault(); + clearErrors(); // Clear previous errors + result.error.issues.forEach(issue => { + setError(`document_attachments_attributes.${index}.document_attributes.file`, issue.message); + }); + return; + } - onUpdate(deepCleanData(sheetDocument)) + onUpdate(deepCleanData(sheetDocument)); + } catch (error) { + console.error('Unexpected error during document validation:', error); + setError( + `document_attachments_attributes.${index}.document_attributes.file`, + 'Une erreur inattendue est survenue lors de la validation du document.' + ); + } finally { + setIsSubmitting(false); + } }db/schema.rb (2)
45-60: Consider adding additional constraints to the addresses table.The addresses table could benefit from:
- A check constraint for postal code format
- A check constraint for latitude/longitude ranges
- An index on city for potential city-based searches
Apply this diff to add constraints:
create_table "addresses", force: :cascade do |t| t.string "street_number" t.string "street_name", null: false t.string "additional_info" t.string "postal_code", null: false t.string "city", null: false t.string "country", default: "France", null: false t.decimal "latitude", precision: 10, scale: 6, null: false t.decimal "longitude", precision: 10, scale: 6, null: false t.string "addressable_type", null: false t.integer "addressable_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.index ["addressable_type", "addressable_id"], name: "index_addresses_on_addressable_type_and_addressable_id", unique: true t.index ["latitude", "longitude"], name: "index_addresses_on_latitude_and_longitude", where: "addressable_type = 'Location'" + t.index ["city"], name: "index_addresses_on_city" + t.check_constraint "postal_code ~ '^[0-9]{5}$'", name: "postal_code_format" + t.check_constraint "latitude >= -90 AND latitude <= 90", name: "latitude_range" + t.check_constraint "longitude >= -180 AND longitude <= 180", name: "longitude_range" end
165-176: Consider adding additional constraints to the locations table.The locations table could benefit from:
- A check constraint for website format
- An index on name for potential name-based searches
Apply this diff to add constraints:
create_table "locations", force: :cascade do |t| t.integer "type", null: false t.string "name" t.text "description" t.string "website" t.datetime "created_at", null: false t.datetime "updated_at", null: false t.integer "osbl_id", null: false t.index ["osbl_id"], name: "index_locations_on_osbl_id" t.index ["osbl_id"], name: "index_locations_on_osbl_id_siege_social", unique: true, where: "type = 0" + t.index ["name"], name: "index_locations_on_name" t.check_constraint "type NOT IN (1, 2, 3) OR (type IN (1, 2, 3) AND name IS NOT NULL)", name: "name_required_for_specific_types" + t.check_constraint "website IS NULL OR website ~ '^https?://.*'", name: "website_format" end
📜 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 (41)
app/controllers/users/contributions_controller.rb(1 hunks)app/frontend/components/forms/MyInput.tsx(2 hunks)app/frontend/components/forms/MyNumberInput.tsx(3 hunks)app/frontend/components/layout/AppSidebar.tsx(4 hunks)app/frontend/components/layout/Footer.tsx(4 hunks)app/frontend/components/layout/Header.tsx(15 hunks)app/frontend/components/pages/contribution/new/FundManagementSection.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblDataSheet.tsx(2 hunks)app/frontend/components/pages/contribution/new/OsblDocumentSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinance.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblHeader.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/pages/user/show/UserAvatarEdit.tsx(1 hunks)app/frontend/components/shared/MyAsyncSelect.tsx(1 hunks)app/frontend/components/shared/MyFileInput.tsx(1 hunks)app/frontend/components/shared/MyLink.tsx(1 hunks)app/frontend/components/ui/button.tsx(1 hunks)app/frontend/components/ui/sheet.tsx(0 hunks)app/frontend/components/ui/sidebar.tsx(4 hunks)app/frontend/entrypoints/application.css(1 hunks)app/frontend/lib/constants.ts(1 hunks)app/frontend/lib/deepCleanData.ts(2 hunks)app/frontend/pages/Contribution/New.tsx(5 hunks)app/frontend/pages/Contribution/types.ts(3 hunks)app/models/address.rb(1 hunks)app/models/annual_finance.rb(1 hunks)app/models/location.rb(1 hunks)app/models/osbl.rb(2 hunks)db/migrate/20250126104700_create_locations.rb(1 hunks)db/migrate/20250126140903_create_addresses.rb(1 hunks)db/migrate/20250126180000_remove_contact_email_from_osbl.rb(1 hunks)db/migrate/20250203115228_add_more_constraint_to_location.rb(1 hunks)db/schema.rb(4 hunks)package.json(1 hunks)spec/factories/addresses.rb(1 hunks)spec/factories/locations.rb(1 hunks)spec/models/address_spec.rb(1 hunks)spec/models/location_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/frontend/components/ui/sheet.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 GitHub Check: standard_ruby
spec/models/address_spec.rb
[failure] 1-1:
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
spec/models/location_spec.rb
[failure] 1-1:
Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (43)
app/frontend/components/forms/MyNumberInput.tsx (2)
19-19: LGTM! Well-typed interface extension.The new optional
noErrorMessageproperty is properly typed and follows the existing interface structure.
49-50: LGTM! Clean props destructuring.Props are well-organized with appropriate default values.
app/frontend/components/pages/contribution/new/FundManagementSection.tsx (2)
16-16: LGTM! Interface and import changes are well-structured.The addition of the Separator component and the new clearErrors function in the interface enhance the component's error handling capabilities while maintaining type safety.
Also applies to: 24-25
1-156: Overall implementation looks solid!The component is well-structured with proper error handling, type safety, and clear UI organization. The changes improve both functionality and user experience while maintaining code quality.
app/frontend/components/shared/MyFileInput.tsx (2)
10-11: LGTM! Props interface changes improve type safety.The updated props interface now correctly handles File objects directly instead of raw events, making the component more type-safe and easier to use.
62-63: Verify file input validation.The
requiredattribute's condition might not work as expected with controlled components. Consider adding explicit validation handling.app/frontend/components/ui/button.tsx (1)
16-16: LGTM! Hover styles enhance visual feedback.The updated hover styles for the outline variant provide better visual feedback with white background and primary border color.
app/frontend/components/forms/MyInput.tsx (1)
7-12: LGTM! Props interface enhancement improves flexibility.Extending from React.InputHTMLAttributes provides better type safety and allows passing through all standard input props.
app/frontend/components/pages/contribution/new/OsblHeader.tsx (2)
9-9: LGTM! Props type refinement.Omitting setError from FormProps makes the component's requirements more precise.
52-57: LGTM! Improved file handling.The file handling is now more direct and type-safe, aligning with the MyFileInput component changes.
app/frontend/components/pages/user/show/UserAvatarEdit.tsx (1)
150-153: LGTM! Clean and simplified file handling.The refactored onChange handler now directly accepts a File object, making the code more concise and type-safe.
app/frontend/components/pages/contribution/new/OsblDataSheet.tsx (3)
53-53: LGTM! Improved type safety.The props type now correctly omits the unused 'setError' prop, making the component's interface more precise.
69-82: Enhanced user feedback for MultiSelect.Good addition of error styling and error message display for the MultiSelect component.
94-128: Improved accessibility for radio buttons.Good improvements:
- Added id attributes for RadioGroupItem components
- Made labels clickable with onClick handlers
- Added helpful tooltips
app/frontend/entrypoints/application.css (1)
181-197: LGTM! Well-implemented animations.The keyframe animations are properly defined and use CSS custom properties for dynamic height calculations.
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (4)
2-10: All imports look good.
The updated imports for Document, icons, Sheet, and DocumentTypeList appear consistent and correctly referenced. No immediate issues found.
12-12: EnsuresetErroris integrated as intended.
AddingsetErrorto the function signature is useful for flexible error handling. However, it does not appear to be invoked within this file. Consider either leveraging the function to handle validation or discarding it if not required.
15-17: Validate array index usage inaddDocument.
You are spreading the array and assigningupdatedDocuments[index] = { document_attributes: document }. Double-check that theindexbeing passed is always valid (i.e., not out of bounds).
22-24: Removal logic looks correct.
The approach for removing the document object by filtering out the index is straightforward and efficient.app/frontend/components/pages/contribution/new/OsblFinance.tsx (6)
1-4: Imports appear consistent and correct.
The new references to icons,Button, anduseStatefor sorting are consistent with the rest of the file. No immediate issues found.
11-11: LeveragesetErrorif necessary.
Similar toOsblDocuments, ensure you actually invokesetErrorto surface validation or submission issues, or consider removing it from the parameters if no errors are expected at this level.
12-13: Sorting state initialization recognized.
sortAscendingis clearly named, and disabling the sort button until there are at least two finance records is good. No issues.
15-20: Check out-of-bounds access inaddFinance.
Like inOsblDocuments, you’re directly assigningupdatedFinances[index] = finance. Confirm that theindexis valid and consider validations to avoid silent failures.
22-25: Deletion logic is correct.
Filtering out the finance at a given index is straightforward and clear.
37-74: Sheet-based interface for adding finances is consistent.
Using<Sheet>and<SheetTrigger>to spawn editable forms is consistent with your approach inOsblDocuments. Good reusability.app/models/address.rb (1)
1-3: NewAddressmodel.
Defines a polymorphic association withaddressable. This is a standard pattern. If you plan to rely on address validations (e.g., city, country), consider introducing those at the model level.db/migrate/20250126180000_remove_contact_email_from_osbl.rb (1)
1-11: LGTM! Migration looks well-structured.The migration correctly handles both the removal and restoration of the contact_email column and its associated format check constraint.
Run this script to verify no remaining references to contact_email:
✅ Verification successful
Migration is correct; remaining references are expected.
The verification shows that aside from the migration in question, the only other instance is a commented-out validation in the model and an earlier migration that added the field. No unintended references exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to contact_email in models, controllers, views, and specs rg -i "contact_email" --type rubyLength of output: 1024
app/models/location.rb (1)
15-17: Address commented validations.There are commented validations that should either be implemented or removed. If they're needed for future implementation, consider adding a TODO comment explaining why they're commented out.
Would you like me to help implement these validations or create an issue to track them?
db/migrate/20250126140903_create_addresses.rb (1)
19-20: LGTM! Indexes look well-designed.The unique index on the polymorphic association and the conditional index for Location coordinates are appropriate for the expected query patterns.
app/models/annual_finance.rb (1)
21-21: LGTM! Excluding "certified" from validation check is appropriate.The change ensures that the presence of only the "certified" attribute is not sufficient to pass validation, maintaining the requirement for at least one substantive piece of information.
app/frontend/pages/Contribution/types.ts (1)
9-13: LGTM! Making type and percent required improves type safety.The change ensures that fund records always have the necessary fields.
app/frontend/components/layout/AppSidebar.tsx (1)
54-65: LGTM! Consistent use of MyLink component.The replacement of Link with MyLink is implemented correctly, maintaining the same functionality while adding mobile-specific behavior.
app/models/osbl.rb (2)
24-24: LGTM! Proper association setup with cleanup.The locations association is correctly set up with
dependent: :destroyto ensure proper cleanup of associated records.
59-59: LGTM! Proper validation and nested attributes setup.The validation for required causes and nested attributes for locations are correctly implemented.
Let's verify the locations model setup:
Also applies to: 67-67
✅ Verification successful
Verification: Nested Attributes and Validation in Location & Osbl Models
The Location model appears to be correctly set up with associations (e.g.,belongs_to :osblandhas_one :address), and no issues were identified in its implementation based on the search output. Similarly, the Osbl model lines (including line 59 with causes validation and line 67, which likely relates to the nested attributes for locations) seem to be consistent with the intended design.The provided output confirms that the Location model’s association setup is standard, and there’s no evidence of missing validations or misconfigurations affecting nested attribute behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Location model and its validations rg -A 10 "class Location" app/models/Length of output: 514
app/controllers/users/contributions_controller.rb (1)
80-94: LGTM! Comprehensive nested attributes setup.The nested attributes for locations and addresses are properly structured, including all necessary fields for geocoding support.
Let's verify the model validations for these attributes:
✅ Verification successful
Nested Attributes and Model Validations Verified
- The Location model includes validations that ensure a valid address is present and accepts nested attributes for address.
- The Address model does not include explicit validations, which appears to be by design given the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for validations in Location and Address models echo "Checking Location model validations:" rg -A 5 "validates" app/models/location.rb echo "Checking Address model validations:" rg -A 5 "validates" app/models/address.rbLength of output: 550
app/frontend/components/pages/contribution/new/OsblLocations.tsx (1)
35-89: LGTM! Well-structured render logic.The component's render logic is well-organized with proper conditional rendering and key usage.
app/frontend/components/layout/Footer.tsx (1)
60-69: LGTM! Consistent component replacement.The Link component has been consistently replaced with MyLink throughout the file, maintaining the same functionality.
Also applies to: 75-81, 105-111
app/frontend/pages/Contribution/New.tsx (2)
71-78: LGTM! Improved form layout structure.The form layout has been well-organized with proper flex container usage for header and submit button alignment.
88-98: LGTM! Well-structured grid layout.The grid layout provides good responsiveness and organization of form sections.
app/frontend/components/layout/Header.tsx (1)
1-2: LGTM! Consistent replacement of Link with MyLink.The refactoring to use
MyLinkinstead ofLinkis consistent throughout the file and maintains the same functionality.Also applies to: 93-107, 112-124, 130-142, 149-161, 175-187, 194-206, 247-259, 262-274, 278-290, 316-321
db/schema.rb (1)
178-192: Verify the impact of removing contact_email.The removal of the contact_email field from the osbls table might affect existing code that relies on this field.
Run the following script to check for potential issues:
app/frontend/components/ui/sidebar.tsx (2)
207-210: Great addition of accessibility features!The inclusion of semantic title and description for the sidebar navigation enhances screen reader support.
220-220: LGTM: Layout refinementsThe positioning and styling updates improve the sidebar's layout structure and border handling.
Also applies to: 239-239, 246-246
app/frontend/components/pages/contribution/new/OsblDocuments.tsx
Outdated
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblLocations.tsx
Outdated
Show resolved
Hide resolved
- Mocked Inertia components and sessionStorage in frontend tests - Created factory for Cause model to support OSBL factory - Updated OSBL factory to automatically add a cause - Minor formatting updates in model spec files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
spec/factories/causes.rb (1)
1-5: LGTM! Basic factory implementation looks good.The factory follows FactoryBot's best practices with a clean sequence implementation for unique names.
Consider enhancing the factory with:
- Additional required attributes for the cause model
- Associations with locations, as they seem to be related based on the context
Example:
factory :cause do sequence(:name) { |n| "Cause #{n}" } description { "A description for #{name}" } trait :with_location do after(:create) do |cause| create(:location, cause: cause) end end endapp/frontend/tests/Layout.test.tsx (2)
20-32: Enhance the Inertia mock implementation for better test coverage.The current mock implementation could be improved in several ways:
- Consider adding commonly used props to avoid test failures.
- Add TypeScript types for better type safety.
Here's a suggested improvement:
vi.mock('@inertiajs/react', async () => { const actual = await vi.importActual('@inertiajs/react') + type PageProps = { + auth?: { user: null | { name: string } } + flash?: { success?: string; error?: string } + [key: string]: unknown + } return { ...actual, - usePage: () => ({ + usePage: (): { props: PageProps } => ({ props: { - // Mock your page props here + auth: { user: null }, + flash: {} } }), - Head: ({ children }: { children: React.ReactNode }) => <>{children}</>, + Head: ({ children, title }: { children?: React.ReactNode; title?: string }) => ( + <>{title ?? children}</> + ) } })
41-61: Add test cases for comprehensive coverage.While the current tests cover sidebar visibility, consider adding test cases for:
- Flash message handling
- Children content rendering
- Header component rendering
Here's a suggested addition:
it('renders flash messages when provided', () => { render( <Layout flash={{ success: 'Test success' }} showSidebar={false}> <div>Test Content</div> </Layout> ) expect(screen.getByText('Test success')).toBeInTheDocument() }) it('renders the header component', () => { renderLayout(false) expect(screen.getByText('Mock Header')).toBeInTheDocument() }) it('renders children content', () => { renderLayout(false) expect(screen.getByText('Test Content')).toBeInTheDocument() })app/frontend/tests/forms/SignInForm.test.tsx (3)
6-19: Consider enhancing the sessionStorage mock implementation.While the mock includes all required methods, consider these improvements:
- Add basic implementations to mock methods to simulate storage behavior
- Reset the mock state between tests to prevent state leakage
Here's an improved implementation:
// Mock sessionStorage const mockSessionStorage = { + store: new Map<string, string>(), getItem: vi.fn(), setItem: vi.fn(), clear: vi.fn(), removeItem: vi.fn(), key: vi.fn(), length: 0 } +// Add implementations +mockSessionStorage.getItem.mockImplementation((key: string) => mockSessionStorage.store.get(key) ?? null) +mockSessionStorage.setItem.mockImplementation((key: string, value: string) => { + mockSessionStorage.store.set(key, value) + mockSessionStorage.length = mockSessionStorage.store.size +}) +mockSessionStorage.clear.mockImplementation(() => { + mockSessionStorage.store.clear() + mockSessionStorage.length = 0 +}) +mockSessionStorage.removeItem.mockImplementation((key: string) => { + mockSessionStorage.store.delete(key) + mockSessionStorage.length = mockSessionStorage.store.size +}) +mockSessionStorage.key.mockImplementation((index: number) => Array.from(mockSessionStorage.store.keys())[index] ?? null) // Mock window object before tests Object.defineProperty(window, 'sessionStorage', { value: mockSessionStorage })
38-49: Improve type safety in useFormHandler mock.Consider adding proper TypeScript types to the mock implementation to ensure type safety and better IDE support.
+interface FormData { + email: string + code: string +} vi.mock('@/hooks/useFormHandler', () => ({ - default: vi.fn(() => ({ + default: vi.fn((): { + data: FormData + updateField: (field: keyof FormData, value: string) => void + submit: (e: Event) => void + processing: boolean + errors: Record<string, string> + } => ({ data: { email: '[email protected]', code: '' }, updateField: vi.fn(), submit: vi.fn((e: Event) => e.preventDefault()), processing: false, errors: {} })) }))
51-117: Add test coverage for sessionStorage interactions and edge cases.Consider adding these test cases to improve coverage:
- Test sessionStorage interactions
- Test error message display
- Test edge cases for OTP input
it('stores email in sessionStorage on mount', () => { render(<SignInForm email='[email protected]' />) expect(mockSessionStorage.setItem).toHaveBeenCalledWith('email', '[email protected]') }) it('displays error message when present', () => { const useFormHandlerMock = vi.mocked(useFormHandler) useFormHandlerMock.mockReturnValueOnce({ data: { email: '[email protected]', code: '' }, updateField: vi.fn(), submit: vi.fn(), processing: false, errors: { code: 'Invalid code' } }) render(<SignInForm email='[email protected]' />) expect(screen.getByText('Invalid code')).toBeInTheDocument() }) it('handles invalid OTP input', () => { render(<SignInForm email='[email protected]' />) const input = screen.getByRole('textbox') fireEvent.change(input, { target: { value: 'abc' } }) expect(mockedUpdateField).not.toHaveBeenCalled() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.yml(4 hunks).github/workflows/deploy-review.yml(1 hunks).github/workflows/deploy-staging.yml(1 hunks).github/workflows/deploy.yml(1 hunks).github/workflows/remove-review.yml(1 hunks)Gemfile(1 hunks)app/frontend/tests/Layout.test.tsx(1 hunks)app/frontend/tests/forms/SignInForm.test.tsx(1 hunks)spec/factories/causes.rb(1 hunks)spec/factories/osbls.rb(1 hunks)spec/models/address_spec.rb(1 hunks)spec/models/location_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/deploy-review.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/models/location_spec.rb
- spec/models/address_spec.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (10)
Gemfile (1)
11-11: Puma Gem Version Update.The Puma gem version requirement has been updated to ">= 6.6.0". Please verify that this version upgrade is compatible with your application's dependencies and that no breaking changes have been introduced with this new minimum version. A quick check with the Puma changelog or running dependency tests would be beneficial.
spec/factories/osbls.rb (1)
7-9: Well-implemented factory callback!The
after(:build)callback is a good addition that ensures data consistency by guaranteeing each OSBL has at least one cause. The implementation follows good practices:
- Uses
buildinstead ofcreatefor better performance- Only adds a cause when needed (lazy loading)
- Maintains single responsibility
.github/workflows/deploy.yml (1)
20-21: Ruby Setup Action Version Update Confirmed.
The Ruby setup action has been upgraded fromv1.213.0tov1.215.0in the deploy workflow. This change is consistent with similar updates in other workflow files and ensures your production deployment uses the latest action version..github/workflows/deploy-staging.yml (1)
27-29: Ruby Setup Action Version Update for Staging.
The version update fromv1.213.0tov1.215.0in the staging workflow aligns with other deployment environments, ensuring consistent configuration across the board..github/workflows/remove-review.yml (1)
20-22: Ruby Setup Action Version Updated in Remove Review App Workflow.
Upgrading the Ruby setup action tov1.215.0in the remove-review workflow brings it in line with other deployment pipelines and maintains consistency..github/workflows/ci.yml (5)
20-25: Updated Ruby Setup Action in Standard Ruby Job.
The Ruby setup action is now upgraded tov1.215.0in the standard ruby job. The configuration parameters (ruby-version and bundler-cache) remain correctly set, ensuring environment consistency in your CI pipeline.
37-42: Ruby Setup Action Upgrade in Scan Ruby Job.
The upgrade toruby/[email protected]in the scan_ruby job is consistent with other jobs in the CI pipeline and helps avoid potential discrepancies in Ruby environments.
54-56: Node.js Setup Action Version Updated in Scan JS Job.
Switching toactions/[email protected]ensures the Scan JS job uses the updated Node environment (Node 20), which should offer improved stability and new features.
76-81: Ruby Setup Action Updated in Test Job.
The test job now utilizesruby/[email protected], aligning it with the rest of the workflows and ensuring a consistent Ruby runtime across your CI processes.
82-86: Node.js Setup Action Updated in Test Job.
By upgrading toactions/[email protected]in the test job, the CI configuration now consistently supports Node 20, mirroring the settings in the scan_js job.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
28-39:⚠️ Potential issueGuard against undefined document types.
When resolving the label via
DocumentTypeList.find(...), consider the scenario wheredocumentTypeis undefined. Attempting to accessdocumentType.labelwould cause a runtime error.Apply this defensive fix:
if (['rapport_activite', 'rapport_financier'].includes(document.type)) { - const documentType = DocumentTypeList.find(type => type.value === document.type) as SelectOption - return `${documentType.label} ${document.year as number}` + const documentType = DocumentTypeList.find(type => type.value === document.type) + if (documentType?.label) { + return `${documentType.label} ${document.year ?? ''}` + } + return document.type }
🧹 Nitpick comments (10)
db/migrate/20250203140445_add_more_constraint_to_address.rb (1)
3-4: Consider adding NOT NULL constraints for coordinates.For better data integrity, consider adding NOT NULL constraints if these coordinates are required fields. This ensures that addresses always have valid geographical positions.
- add_check_constraint :addresses, "latitude >= -90 AND latitude <= 90", name: "check_latitude_range" - add_check_constraint :addresses, "longitude >= -180 AND longitude <= 180", name: "check_longitude_range" + add_check_constraint :addresses, "latitude IS NOT NULL AND latitude >= -90 AND latitude <= 90", name: "check_latitude_range" + add_check_constraint :addresses, "longitude IS NOT NULL AND longitude >= -180 AND longitude <= 180", name: "check_longitude_range"app/frontend/components/shared/MyAsyncSelect.tsx (5)
29-29: Consider improving type safety forsetData.The
anytype in thesetDataprop could be replaced with a more specific type to improve type safety.- setData: (key: string, value: any) => void + setData: (key: string, value: Address | undefined) => void
61-61: Consider moving the API URL to configuration.The API URL is hardcoded which could make it difficult to change in different environments.
- const response = await fetch(`https://api-adresse.data.gouv.fr/search/?q=${inputValue}`) + const response = await fetch(`${process.env.REACT_APP_ADDRESS_API_URL}/search/?q=${inputValue}`)
67-70: Consider adding user feedback for API errors.Currently, errors are only logged to console. Consider showing a user-friendly error message.
} catch (error) { console.error(error) + // You can use your app's toast/notification system here + // showErrorToast('Failed to fetch address suggestions. Please try again.') return [] }
88-88: Simplify the required prop logic.The required prop logic can be simplified.
- required={required && value === undefined} + required={required && !value}
90-93: Consider memoizing the onChange handler.The onChange handler could be memoized using useCallback to prevent unnecessary re-renders.
+ const handleChange = useCallback((newValue: Option | null) => { + if (newValue === null) return + setData(attributeName, newValue.value) + }, [setData, attributeName]) onChange={(newValue: Option | null) => { if (newValue === null) return setData(attributeName, newValue.value) - }} + onChange={handleChange}db/schema.rb (1)
45-62: Consider adding postal code format validation and city index.The addresses table is well-structured with proper constraints and indices. Consider these enhancements:
- Add a check constraint for postal code format
- Add an index on city for location-based queries
create_table "addresses", force: :cascade do |t| # ... existing fields ... t.index ["addressable_type", "addressable_id"], name: "index_addresses_on_addressable_type_and_addressable_id", unique: true t.index ["latitude", "longitude"], name: "index_addresses_on_latitude_and_longitude", where: "addressable_type = 'Location'" + t.index ["city"], name: "index_addresses_on_city" t.check_constraint "latitude >= -90 AND latitude <= 90", name: "check_latitude_range" t.check_constraint "longitude >= -180 AND longitude <= 180", name: "check_longitude_range" + t.check_constraint "postal_code ~ '^[0-9]{5}$'", name: "check_postal_code_format" endapp/frontend/components/shared/MyAsyncCreatableSelect.tsx (3)
38-54: Enhance error handling infetchOptions.Consider improving error handling by:
- Using more descriptive error messages.
- Handling specific error types.
Apply this diff to improve error handling:
const fetchOptions = async (inputValue: string): Promise<SelectOption[]> => { if (inputValue.length < minInputLength) return [] try { const response = await fetch(`/${resource}?query=${encodeURIComponent(inputValue)}`) if (!response.ok) { - throw new Error('Network response was not ok') + throw new Error(`Failed to fetch ${resource}: ${response.status} ${response.statusText}`) } const data = await response.json() as BackendData[] const options = data.map((data) => createOption(data)) setOptions(options) return options } catch (error) { - console.error(error) + if (error instanceof Error) { + console.error(`Error fetching ${resource}:`, error.message) + } else { + console.error(`Unknown error fetching ${resource}:`, error) + } return [] } }
69-95: Enhance error handling inhandleCreate.Consider improving error handling by:
- Using more descriptive error messages.
- Handling specific error types.
- Using a more flexible response status check.
Apply this diff to improve error handling:
const handleCreate = (inputValue: string): void => { void (async () => { try { const csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') const response = await axios.post( `/${resource}`, { name: inputValue }, { headers: { 'X-CSRF-Token': csrfToken } } ) - if (response.status !== 201) { - throw new Error('Network response was not 201') + if (response.status < 200 || response.status >= 300) { + throw new Error(`Failed to create ${resource}: ${response.status} ${response.statusText}`) } const newOption = createOption(response.data) setValue((prev) => [...prev, newOption]) setData(attributeName, [...(data[attributeName] ?? []), { [`${resource.slice(0, -1)}_id`]: newOption.value }]) } catch (error) { - console.log(error) + if (axios.isAxiosError(error)) { + console.error(`Error creating ${resource}:`, error.response?.data || error.message) + } else if (error instanceof Error) { + console.error(`Error creating ${resource}:`, error.message) + } else { + console.error(`Unknown error creating ${resource}:`, error) + } toast.error(`Une erreur est survenue lors de la création de ${inputValue}`) } })() }
97-100: Improve readability ofisValidNewOption.Consider improving readability by:
- Splitting the long condition.
- Extracting magic numbers as constants.
Apply this diff to improve readability:
+ const MIN_INPUT_LENGTH = 3 + const MAX_INPUT_LENGTH = 30 + function isValidNewOption (inputValue: string): boolean { const normalizedInput = inputValue.trim().toLowerCase().replace(/^./, char => char.toUpperCase()) - return inputValue.length >= 3 && inputValue.length <= 30 && value.some(option => option.label === normalizedInput) === false && !options.some(option => option.label === normalizedInput) + const isValidLength = inputValue.length >= MIN_INPUT_LENGTH && inputValue.length <= MAX_INPUT_LENGTH + const isNotInValue = !value.some(option => option.label === normalizedInput) + const isNotInOptions = !options.some(option => option.label === normalizedInput) + return isValidLength && isNotInValue && isNotInOptions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/frontend/components/pages/contribution/new/FundManagementSection.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/pages/user/show/UserAvatarEdit.tsx(2 hunks)app/frontend/components/shared/MyAsyncCreatableSelect.tsx(6 hunks)app/frontend/components/shared/MyAsyncSelect.tsx(1 hunks)app/frontend/types/types.ts(1 hunks)db/migrate/20250203140445_add_more_constraint_to_address.rb(1 hunks)db/migrate/20250203160027_add_check_constraint_to_keywords.rb(1 hunks)db/migrate/20250203160100_add_check_constraint_to_intervention_areas.rb(1 hunks)db/schema.rb(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/frontend/components/pages/contribution/new/OsblLocations.tsx
- app/frontend/components/pages/contribution/new/FundManagementSection.tsx
- app/frontend/components/pages/user/show/UserAvatarEdit.tsx
🧰 Additional context used
🪛 GitHub Actions: Deploy to Review
db/migrate/20250203160027_add_check_constraint_to_keywords.rb
[error] 3-3: CHECK constraint failed: keywords_name_length_check
[error] An error has occurred, this and all later migrations canceled: SQLite3::ConstraintException: CHECK constraint failed: keywords_name_length_check
🔇 Additional comments (11)
db/migrate/20250203140445_add_more_constraint_to_address.rb (1)
1-6: Well-structured migration with appropriate geographical constraints!The implementation correctly enforces valid ranges for geographical coordinates at the database level:
- Latitude: -90° to 90° (poles)
- Longitude: -180° to 180° (meridians)
This provides robust data integrity protection that complements any application-level validations.
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (4)
13-13: LGTM! Good improvement in error handling.The addition of
setErrorto the component props enhances error handling capabilities.
16-21: LGTM! Clean implementation of document addition.The function correctly handles document updates while maintaining immutability.
23-26: LGTM! Proper event handling.Good practice to prevent form submission and maintain immutability when removing documents.
41-98: LGTM! Clean and responsive UI implementation.Good practices observed:
- Proper key usage in lists
- Responsive design with tailwind classes
- Clean component composition with Sheet components
app/frontend/components/shared/MyAsyncSelect.tsx (2)
37-47: LGTM! Well-structured option creation.The
createOptionfunction is well-implemented with proper typing and clear mapping of address properties.
73-84: LGTM! Efficient debouncing implementation.The debouncing implementation with useCallback and Promise handling is well-structured and follows React best practices.
db/schema.rb (1)
149-149: LGTM! Consistent name length constraints.The name length constraints (3-30 characters) are reasonable and consistently applied to both intervention_areas and keywords tables.
Also applies to: 157-157
app/frontend/components/shared/MyAsyncCreatableSelect.tsx (3)
17-25: LGTM!The
BackendDatainterface andcreateOptionfunction are well-typed and correctly implemented.
35-36: LGTM!The state variables are correctly typed and initialized.
102-154: LGTM!The component props and styles are well-organized and correctly implemented.
db/migrate/20250203160100_add_check_constraint_to_intervention_areas.rb
Outdated
Show resolved
Hide resolved
mpressen
force-pushed
the
osbl-creation-fourth-panel-locations
branch
from
641e80a to
a963ad6
Compare
- Added latitude and longitude range check constraints for addresses - Implemented name length constraints for keywords and intervention areas - Created new migration files to add these database-level validations - Ensures data integrity by enforcing stricter input validation at the database level
mpressen
force-pushed
the
osbl-creation-fourth-panel-locations
branch
from
a963ad6 to
4e4f989
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
28-39:⚠️ Potential issueGuard against undefined document types.
When resolving the label via
DocumentTypeList.find(...), consider the scenario wheredocumentTypeis undefined. Attempting to accessdocumentType.labelwould cause a runtime error.Apply this defensive fix:
if (['rapport_activite', 'rapport_financier'].includes(document.type)) { - const documentType = DocumentTypeList.find(type => type.value === document.type) as SelectOption - return `${documentType.label} ${document.year as number}` + const documentType = DocumentTypeList.find(type => type.value === document.type) + if (documentType?.label) { + return `${documentType.label} ${document.year ?? ''}` + } + return document.type }db/migrate/20250203160027_add_check_constraint_to_keywords.rb (2)
1-5:⚠️ Potential issueFix migration failures by handling existing data.
The migration needs data cleanup before adding constraints to prevent failures with existing records.
class AddCheckConstraintToKeywords < ActiveRecord::Migration[8.0] def change + # Clean up existing invalid data + execute <<-SQL + UPDATE keywords + SET name = SUBSTR(name, 1, 100) + WHERE length(name) > 100 + SQL + + execute <<-SQL + UPDATE keywords + SET name = CONCAT(name, 'xxx') + WHERE length(name) < 3 + SQL + add_check_constraint :keywords, "length(name) >= 3 AND length(name) <= 100", name: "keywords_name_length_check" end end
3-3: 🛠️ Refactor suggestionUse database-agnostic functions for better compatibility.
The
length()function might not work consistently across different databases.- add_check_constraint :keywords, "length(name) >= 3 AND length(name) <= 100", name: "keywords_name_length_check" + add_check_constraint :keywords, "CHAR_LENGTH(name) >= 3 AND CHAR_LENGTH(name) <= 100", name: "keywords_name_length_check"db/migrate/20250203160100_add_check_constraint_to_intervention_areas.rb (2)
1-5:⚠️ Potential issueFix potential migration failures by handling existing data.
The migration needs data cleanup before adding constraints to prevent potential failures.
class AddCheckConstraintToInterventionAreas < ActiveRecord::Migration[8.0] def change + # Clean up existing invalid data + execute <<-SQL + UPDATE intervention_areas + SET name = SUBSTR(name, 1, 100) + WHERE length(name) > 100 + SQL + + execute <<-SQL + UPDATE intervention_areas + SET name = CONCAT(name, 'xxx') + WHERE length(name) < 3 + SQL + add_check_constraint :intervention_areas, "length(name) >= 3 AND length(name) <= 100", name: "intervention_areas_name_length_check" end end
3-3: 🛠️ Refactor suggestionUse database-agnostic functions for better compatibility.
The
length()function might not work consistently across different databases.- add_check_constraint :intervention_areas, "length(name) >= 3 AND length(name) <= 100", name: "intervention_areas_name_length_check" + add_check_constraint :intervention_areas, "CHAR_LENGTH(name) >= 3 AND CHAR_LENGTH(name) <= 100", name: "intervention_areas_name_length_check"app/models/intervention_area.rb (1)
5-5:⚠️ Potential issueUncomment the validation to ensure application-level checks.
The validation is currently commented out, which means there are no application-level checks for name length.
- # validates :name, presence: true, uniqueness: true, length: {minimum: 3, maximum: 100} + validates :name, presence: true, uniqueness: true, length: {minimum: 3, maximum: 100}
🧹 Nitpick comments (6)
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
41-98: Enhance accessibility for document management UI.Consider the following accessibility improvements:
- Add aria-label to buttons for screen readers
- Add role="list" to the documents container
- <Button variant='outline'> + <Button variant='outline' aria-label="Add new document"> <PlusIcon className='w-4 h-4' /> <span className='ml-2 hidden sm:block lg:hidden xl:block'>Ajouter</span> </Button> - <div className='flex flex-col gap-4'> + <div className='flex flex-col gap-4' role="list"> - <Button variant='outline' className='bg-white text-primary border-none'> + <Button variant='outline' className='bg-white text-primary border-none' aria-label="Edit document"> <PencilIcon /> </Button> - <Button onClick={(e) => handleDocumentRemove(e, index)} variant='outline' className='bg-white text-red-500 border-none'> + <Button onClick={(e) => handleDocumentRemove(e, index)} variant='outline' className='bg-white text-red-500 border-none' aria-label="Remove document"> <TrashIcon className='w-4 h-4' /> </Button>app/frontend/components/shared/MyAsyncCreatableSelect.tsx (4)
38-54: Enhance error handling specificity.The error handling is good but could be more specific about the type of error that occurred.
Apply this diff to improve error handling:
const fetchOptions = async (inputValue: string): Promise<SelectOption[]> => { if (inputValue.length < minInputLength) return [] try { const response = await fetch(`/${resource}?query=${encodeURIComponent(inputValue)}`) if (!response.ok) { - throw new Error('Network response was not ok') + throw new Error(`Failed to fetch options: ${response.status} ${response.statusText}`) } const data = await response.json() as BackendData[] const options = data.map((data) => createOption(data)) setOptions(options) return options } catch (error) { - console.error(error) + console.error('Error fetching options:', error) return [] } }
97-99: Extract magic numbers into named constants.The maximum length validation (100) should be extracted into a named constant for better maintainability.
Apply this diff to improve code readability:
+const MIN_INPUT_LENGTH = 3; +const MAX_INPUT_LENGTH = 100; function isValidNewOption (inputValue: string): boolean { const normalizedInput = inputValue.trim().toLowerCase().replace(/^./, char => char.toUpperCase()) - return inputValue.length >= 3 && inputValue.length <= 100 && value.some(option => option.label === normalizedInput) === false && !options.some(option => option.label === normalizedInput) + return inputValue.length >= MIN_INPUT_LENGTH && + inputValue.length <= MAX_INPUT_LENGTH && + !value.some(option => option.label === normalizedInput) && + !options.some(option => option.label === normalizedInput) }
69-95: Enhance error handling and add request timeout.The error handling in handleCreate could be more specific, and the axios request should have a timeout.
Apply this diff to improve error handling and add timeout:
const handleCreate = (inputValue: string): void => { void (async () => { try { const csrfToken = document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') const response = await axios.post( `/${resource}`, { name: inputValue }, { headers: { 'X-CSRF-Token': csrfToken }, + timeout: 5000 // 5 seconds timeout } ) if (response.status !== 201) { - throw new Error('Network response was not 201') + throw new Error(`Failed to create: ${response.status} ${response.statusText}`) } const newOption = createOption(response.data) setValue((prev) => [...prev, newOption]) setData(attributeName, [...(data[attributeName] ?? []), { [`${resource.slice(0, -1)}_id`]: newOption.value }]) } catch (error) { - console.log(error) + console.error('Error creating option:', error) + const errorMessage = axios.isAxiosError(error) && error.response + ? `Status: ${error.response.status} - ${error.response.statusText}` + : error instanceof Error ? error.message : 'Unknown error' - toast.error(`Une erreur est survenue lors de la création de ${inputValue}`) + toast.error(`Une erreur est survenue lors de la création de "${inputValue}": ${errorMessage}`) } })() }
150-152: Externalize hardcoded French text for internationalization.The French text should be moved to a translation file to support multiple languages.
Consider using a translation library like
react-intlori18nextand moving these strings to a translation file:- formatCreateLabel={(inputValue) => `Ajouter "${inputValue}"`} - loadingMessage={() => 'Recherche...'} - noOptionsMessage={() => 'Aucune suggestion'} + formatCreateLabel={(inputValue) => t('select.create_label', { value: inputValue })} + loadingMessage={() => t('select.loading')} + noOptionsMessage={() => t('select.no_options')}db/schema.rb (1)
45-62: Add postal code format validation.Consider adding a check constraint to validate the postal code format, especially since the default country is France which has a specific format (5 digits).
t.check_constraint "latitude >= -90 AND latitude <= 90", name: "check_latitude_range" t.check_constraint "longitude >= -180 AND longitude <= 180", name: "check_longitude_range" + t.check_constraint "country != 'France' OR postal_code ~ '^[0-9]{5}$'", name: "french_postal_code_format"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/frontend/components/forms/MyInput.tsx(2 hunks)app/frontend/components/pages/contribution/new/FundManagementSection.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/pages/user/show/UserAvatarEdit.tsx(2 hunks)app/frontend/components/shared/MyAsyncCreatableSelect.tsx(6 hunks)app/frontend/components/shared/MyAsyncSelect.tsx(1 hunks)app/frontend/types/types.ts(1 hunks)app/models/intervention_area.rb(1 hunks)app/models/keyword.rb(1 hunks)db/migrate/20250203140445_add_more_constraint_to_address.rb(1 hunks)db/migrate/20250203160027_add_check_constraint_to_keywords.rb(1 hunks)db/migrate/20250203160100_add_check_constraint_to_intervention_areas.rb(1 hunks)db/schema.rb(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/frontend/components/pages/user/show/UserAvatarEdit.tsx
- db/migrate/20250203140445_add_more_constraint_to_address.rb
- app/frontend/types/types.ts
- app/frontend/components/pages/contribution/new/FundManagementSection.tsx
- app/frontend/components/pages/contribution/new/OsblLocations.tsx
- app/frontend/components/forms/MyInput.tsx
- app/frontend/components/shared/MyAsyncSelect.tsx
🔇 Additional comments (5)
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (2)
1-12: LGTM! Import statements are well-organized.The imports are properly structured and include all necessary dependencies for the component's functionality.
13-14: LGTM! Function signature properly includes error handling.The addition of
setErrorto the props enhances the component's error handling capabilities.app/frontend/components/shared/MyAsyncCreatableSelect.tsx (1)
5-7: Fix the import path forMultiValue.The import path for
MultiValueshould not referencenode_modulesdirectly.Apply this diff to fix the import path:
import { SelectOption } from '@/types/types' -import { MultiValue } from 'node_modules/react-select/dist/declarations/src/types' +import { MultiValue } from 'react-select' import { toast } from 'sonner'db/schema.rb (2)
169-180: Previous review suggestions are still applicable.The following suggestions from previous reviews remain valid:
- Document the meaning of each type value (0=headquarters, etc.)
- Add website format validation
267-267: Previous suggestion about ON DELETE behavior is still applicable.The foreign key from locations to osbls should specify ON DELETE behavior for consistency with other foreign keys in the schema.
…st coverage - Updated MyAsyncCreatableSelect to fix option validation logic - Modified SelectOption type to use number for value - Updated Document model with year validation and commented out associations - Added dependent destroy for Location's address - Created comprehensive test suites for Document, Location, InterventionArea, and Keyword models - Added sample PDF fixture for testing - Improved database constraints and validation checks - Updated foreign key constraint for locations to cascade on delete
- Added cascade delete to foreign keys for annual finances, documents, users, and related tables - Implemented cleanup mechanism for orphaned documents in DocumentAttachment model - Created factory and spec for DocumentAttachment to test document deletion behavior - Updated User model to add contributions association - Improved Address factory with more realistic data and dynamic addressable creation - Added comprehensive tests for Address and DocumentAttachment models
…ents - Updated Document, Location, FundSource, FundAllocation models to use human-readable enum types - Removed constants file and replaced type lists with dynamic prop fetching from backend - Modified frontend components to work with new string-based type system - Updated factories and specs to use new type representations - Simplified type selection and filtering logic in various form components
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
spec/requests/keywords_spec.rb (1)
58-63: Uncomment and enhance the duplicate keyword test.The commented-out test for duplicate keywords tests an important database constraint and should be uncommented.
-# context "when keyword already exists" do -# before { create(:keyword, name: "test") } -# it "returns an error" do -# expect { subject }.to raise_error(ActiveRecord::RecordNotUnique) -# end -# end +context "when keyword already exists" do + before { create(:keyword, name: "test") } + + it "returns an error for exact match" do + expect { subject }.to raise_error(ActiveRecord::RecordNotUnique) + end + + it "returns an error for case-insensitive match" do + expect { + post keywords_path, params: { name: "TEST" } + }.to raise_error(ActiveRecord::RecordNotUnique) + end + + it "returns an error for match with different whitespace" do + expect { + post keywords_path, params: { name: " test " } + }.to raise_error(ActiveRecord::RecordNotUnique) + end +end
🧹 Nitpick comments (34)
spec/models/document_attachment_spec.rb (3)
4-29: Consider adding tests for edge cases.The current tests cover the main scenarios well. Consider adding tests for:
- Edge case where document reference is nil
- Explicit naming of the callback method being tested (cleanup_orphaned_document)
Would you like me to help generate the additional test cases?
37-41: Remove unused variable assignment.The
duplicate_attachmentvariable is assigned but never used.expect { - duplicate_attachment = create(:document_attachment, document: document, attachable: osbl) + create(:document_attachment, document: document, attachable: osbl) }.to raise_error(ActiveRecord::RecordNotUnique)🧰 Tools
🪛 GitHub Check: standard_ruby
[failure] 39-39:
Lint/UselessAssignment: Useless assignment to variable -duplicate_attachment.
31-51: Consider adding more specific constraint details.The tests verify the uniqueness constraint well, but could be more explicit about the specific database constraint being tested.
Consider adding a comment or updating the description to specify the exact constraint:
# Tests the unique_document_per_attachable constraint # (index: index_document_attachments_on_document_id_and_attachable) describe "db constraints" do🧰 Tools
🪛 GitHub Check: standard_ruby
[failure] 39-39:
Lint/UselessAssignment: Useless assignment to variable -duplicate_attachment.db/schema.rb (1)
45-62: Add website format validation for addressable locations.Consider adding a check constraint to validate website format when the addressable type is 'Location'.
t.check_constraint "latitude >= -90 AND latitude <= 90", name: "check_latitude_range" t.check_constraint "longitude >= -180 AND longitude <= 180", name: "check_longitude_range" + t.check_constraint "addressable_type != 'Location' OR website IS NULL OR website ~ '^https?://.*'", name: "location_website_format_check"app/frontend/components/shared/MyAsyncCreatableSelect.tsx (2)
38-54: Consider enhancing error messaging for better UX.While the error handling is good, consider providing more user-friendly error messages through toast notifications, similar to the handleCreate function.
} catch (error) { console.error(error) + toast.error('Une erreur est survenue lors de la recherche') return [] }
97-100: Consider extracting validation constants.The validation logic is good, but consider extracting the magic numbers into named constants for better maintainability.
+const MIN_INPUT_LENGTH = 3; +const MAX_INPUT_LENGTH = 100; + function isValidNewOption (inputValue: string): boolean { const normalizedInput = inputValue.trim().toLowerCase().replace(/^./, char => char.toUpperCase()) - return inputValue.length >= 3 && inputValue.length <= 100 && !value.some(option => option.label === normalizedInput) && !options.some(option => option.label === normalizedInput) + return inputValue.length >= MIN_INPUT_LENGTH && + inputValue.length <= MAX_INPUT_LENGTH && + !value.some(option => option.label === normalizedInput) && + !options.some(option => option.label === normalizedInput) }spec/models/intervention_area.rb (5)
4-8: Consider adding more comprehensive factory tests.While the basic factory validation is good, consider adding tests for different scenarios such as:
- Factory with minimum length name
- Factory with maximum length name
- Factory with special characters in name
describe "factory" do it "is valid" do expect(create(:intervention_area)).to be_valid end + + it "is valid with minimum length name" do + expect(create(:intervention_area, name: "ABC")).to be_valid + end + + it "is valid with maximum length name" do + expect(create(:intervention_area, name: "A" * 100)).to be_valid + end + + it "is valid with special characters" do + expect(create(:intervention_area, name: "Asia & Pacific")).to be_valid + end end
10-38: Consider using transaction blocks for database constraint tests.While the tests are comprehensive, using
create!directly might mask validation errors. Consider wrapping the tests in transactions to ensure proper cleanup:describe "database constraints" do it "raises error when name is null" do - expect { - InterventionArea.create!(name: nil) - }.to raise_error(ActiveRecord::NotNullViolation) + ActiveRecord::Base.transaction do + expect { + InterventionArea.create!(name: nil) + }.to raise_error(ActiveRecord::NotNullViolation) + raise ActiveRecord::Rollback + end end # Apply similar pattern to other tests in this block end
40-48: Enhance association test coverage.The current test only verifies the count change. Consider adding more comprehensive checks:
describe "associations" do it "cascades deletion of osbls_intervention_areas when deleted" do intervention_area = InterventionArea.create!(name: "Asia") osbl = create(:osbl) intervention_area.osbls << osbl + join_record_id = intervention_area.osbls_intervention_areas.first.id expect { intervention_area.destroy }.to change { OsblsInterventionArea.count }.by(-1) + expect(OsblsInterventionArea.find_by(id: join_record_id)).to be_nil + expect(Osbl.find_by(id: osbl.id)).to be_present # Verify OSBL still exists end + + it "allows multiple OSBLs to be associated" do + intervention_area = InterventionArea.create!(name: "Asia") + osbl1 = create(:osbl) + osbl2 = create(:osbl) + intervention_area.osbls << [osbl1, osbl2] + + expect(intervention_area.osbls.count).to eq(2) + end end
50-55: Add more comprehensive normalization test cases.Consider testing additional edge cases for name normalization:
describe "normalization" do it "normalizes the name" do intervention_area = InterventionArea.create!(name: " ASIA LoL YoLo ") expect(intervention_area.name).to eq("Asia lol yolo") end + + it "handles multiple consecutive spaces" do + intervention_area = InterventionArea.create!(name: "Asia Pacific") + expect(intervention_area.name).to eq("Asia pacific") + end + + it "handles special characters" do + intervention_area = InterventionArea.create!(name: "Asia & PACIFIC") + expect(intervention_area.name).to eq("Asia & pacific") + end + + it "handles accented characters" do + intervention_area = InterventionArea.create!(name: "AMÉRICA LATINA") + expect(intervention_area.name).to eq("América latina") + end end
1-56: Consider using shared examples for common behavior.The test suite is well-structured, but could benefit from shared examples for common behavior testing patterns:
require "rails_helper" + +RSpec.shared_examples "name normalization" do |model_class| + it "normalizes the name" do + instance = model_class.create!(name: " TEST NAME ") + expect(instance.name).to eq("Test name") + end +end RSpec.describe InterventionArea, type: :model do # ... existing tests ... describe "normalization" do + it_behaves_like "name normalization", InterventionArea # ... existing normalization tests ... end endThis allows reuse of common test patterns across different models that share similar behavior.
app/frontend/components/pages/contribution/new/OsblDocumentSheet.tsx (5)
39-50: Consider enhancing the validation schema.While the current validation is good, consider these improvements:
- Move constants to a shared configuration file for reusability.
- Add validation for empty files.
- Consider using i18n for error messages.
+// Move to shared config file +export const FILE_CONSTRAINTS = { + MAX_SIZE: 5 * 1024 * 1024, + ALLOWED_TYPES: ['application/pdf'] as const +} const documentValidation = z.object({ file: z.instanceof(File) + .refine((file) => file.size > 0, 'Le fichier ne peut pas être vide.') .refine((file) => { - return file.size <= MAX_DOCUMENT_SIZE + return file.size <= FILE_CONSTRAINTS.MAX_SIZE }, 'La taille du fichier doit être inférieure à 5 MB.') .refine((file) => { - return ALLOWED_DOCUMENT_TYPES.includes(file.type) + return FILE_CONSTRAINTS.ALLOWED_TYPES.includes(file.type) }, 'Le type de fichier est invalide. Format accepté : PDF.') })
63-70: Simplify the initial state calculation.The complex boolean checks for determining the initial state could be simplified for better readability.
- const [isOpen, setIsOpen] = useState(() => { - const checks = [ - (!['Rapport d\'activité', 'Rapport financier'].includes(sheetDocument.type ?? '') && sheetDocument.year !== undefined), - (!['Procès verbal', 'Autre'].includes(sheetDocument.type ?? '') && sheetDocument.name !== undefined), - sheetDocument.description !== undefined - ] - return checks.some(check => check) - }) + const [isOpen, setIsOpen] = useState(() => + sheetDocument.description !== undefined || + (sheetDocument.year !== undefined && !['Rapport d\'activité', 'Rapport financier'].includes(sheetDocument.type ?? '')) || + (sheetDocument.name !== undefined && !['Procès verbal', 'Autre'].includes(sheetDocument.type ?? '')) + )
79-94: Add error handling to form submission.The handleSubmit function should include try-catch for better error handling.
function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): void { + try { if (formRef.current?.reportValidity() === false) { e.preventDefault() return } const result = documentValidation.safeParse({ file: sheetDocument.file }) if (!result.success) { e.preventDefault() const errorMessage = result.error.issues[0].message setError(`document_attachments_attributes.${index}.document_attributes.file`, errorMessage) return } onUpdate(deepCleanData(sheetDocument)) + } catch (error) { + e.preventDefault() + setError(`document_attachments_attributes.${index}.document_attributes.file`, 'Une erreur est survenue') + } }
127-148: Extract repeated document type checks into constants.The document type checks are duplicated in multiple places. Consider extracting them into constants for better maintainability.
+const ACTIVITY_FINANCIAL_REPORTS = ['Rapport d\'activité', 'Rapport financier'] as const +const MINUTES_OTHER_DOCS = ['Procès verbal', 'Autre'] as const - {['Rapport d\'activité', 'Rapport financier'].includes(sheetDocument.type ?? '') && ( + {ACTIVITY_FINANCIAL_REPORTS.includes(sheetDocument.type ?? '') && ( <MyNumberInput // ... year input props /> )} - {['Procès verbal', 'Autre'].includes(sheetDocument.type ?? '') && ( + {MINUTES_OTHER_DOCS.includes(sheetDocument.type ?? '') && ( <MyInput // ... name input props /> )} {/* Similar changes in CollapsibleContent */}Also applies to: 175-194
166-213: Enhance accessibility for interactive elements.The collapsible section and character count could benefit from improved accessibility.
<Collapsible open={isOpen} onOpenChange={setIsOpen}> - <CollapsibleTrigger asChild> + <CollapsibleTrigger asChild aria-expanded={isOpen}> <div className='flex justify-between items-center cursor-pointer text-muted-foreground hover:text-muted-foreground/80'> - <span>Informations additionnelles</span> + <span role="heading" aria-level={2}>Informations additionnelles</span> <ChevronDown className={`transition-transform duration-300 ${isOpen ? 'rotate-180' : ''}`} /> </div> </CollapsibleTrigger> <CollapsibleContent className='CollapsibleContent'> {/* ... content ... */} - <div className='text-xs text-right'> + <div className='text-xs text-right' aria-live="polite" aria-atomic="true"> {(sheetDocument.description ?? '').length}/300 caractères </div> </CollapsibleContent> </Collapsible>app/models/fund_source.rb (1)
11-15: Consider documenting or removing commented enum values.The commented-out enum values appear to be alternative categorizations. If these are planned for future use:
- Document the plan in a TODO comment or issue
- Consider using a more structured approach like feature flags
- If not needed, remove to maintain code clarity
app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx (4)
38-49: Simplify fund sources validation logic.The validation for empty arrays and percentage calculation could be more concise using optional chaining and early returns.
Consider this refactor:
.refine((sources): sources is typeof sources => { - if (sources === undefined || sources.length === 0) return true + if (!sources?.length) return true - const sum = sources?.reduce((acc: number, source) => { + const sum = sources.reduce((acc: number, source) => { - const percent = source.percent !== undefined ? Number(source.percent) : 0 + const percent = Number(source.percent ?? 0) return acc + percent }, 0) return sum === 100
66-72: Make deep cleaning validation more explicit.The validation logic using
deepCleanDatais not immediately clear about what constitutes valid data.Consider making the validation more explicit:
.refine( (finance) => { const financeCleaned = deepCleanData(finance) - return !(Object.keys(financeCleaned).length === 1) + const requiredFields = ['year', 'budget', 'treasury'] + return requiredFields.some(field => field in financeCleaned) }, { message: 'Complétez les comptes pour cette année.', path: ['annual_finances_attributes', currentIndex, 'missing_information'] } )
229-234: Enhance button accessibility and loading state.The submit button should indicate loading state and have proper ARIA labels.
Consider this enhancement:
<SheetClose asChild> - <Button onClick={handleSubmit} variant='default' size='lg'> + <Button + onClick={handleSubmit} + variant='default' + size='lg' + disabled={isSubmitting} + aria-label="Valider les informations financières" + > <CheckIcon className='mr-2' /> - Valider + {isSubmitting ? 'Validation...' : 'Valider'} </Button> </SheetClose>
130-132: Remove sr-only class from SheetDescription.The description provides important context and should be visible to all users.
- <SheetDescription className='sr-only'> + <SheetDescription> Renseignez les informations financières pour cette année. </SheetDescription>app/models/location.rb (1)
7-12: Consider using I18n for location type strings.The location type strings are hardcoded in French. Consider using I18n to support multiple languages and maintain consistency.
Example implementation:
# config/locales/fr.yml fr: activerecord: attributes: location: types: siege_social: "Siège social" antenne_locale: "Antenne locale" lieu_d_activite: "Lieu d'activité" autre: "Autre" # app/models/location.rb enum :type, { siege_social: 0, antenne_locale: 1, lieu_d_activite: 2, autre: 3 }.freeze def type_name I18n.t("activerecord.attributes.location.types.#{type}") endspec/models/keyword_spec.rb (1)
50-55: Enhance normalization test coverage.While the current test case verifies basic normalization, consider adding more test cases to cover edge cases:
describe "normalization" do it "normalizes the name" do keyword = Keyword.create!(name: " TECHNOLOGY LoL YoLo ") expect(keyword.name).to eq("Technology lol yolo") end + + it "handles special characters and multiple spaces" do + keyword = Keyword.create!(name: " WEB 3.0 & AI ") + expect(keyword.name).to eq("Web 3.0 & ai") + end + + it "handles non-ASCII characters" do + keyword = Keyword.create!(name: " CAFÉ ") + expect(keyword.name).to eq("Café") + end + + it "handles empty string" do + expect { + Keyword.create!(name: " ") + }.to raise_error(ActiveRecord::NotNullViolation) + end endspec/requests/keywords_spec.rb (1)
15-17: Add more test cases for query validation.The current test only checks for query length less than 3 characters. Consider adding more test cases for query validation:
it "raises an error if query is less than 3 characters" do expect { subject }.to raise_error(RuntimeError) end + +it "raises an error if query contains only whitespace" do + expect { + get keywords_path, params: { query: " " } + }.to raise_error(RuntimeError) +end + +it "raises an error if query contains special characters" do + expect { + get keywords_path, params: { query: "<script>" } + }.to raise_error(RuntimeError) +endapp/controllers/users/contributions_controller.rb (1)
84-98: Consider adding validations for location attributes.The nested attributes for locations and addresses are properly structured. However, consider adding validations to ensure data integrity:
- Validate website URLs
- Validate postal codes
- Validate latitude/longitude ranges
# Add to the Location model +validates :website, format: { with: URI::DEFAULT_PARSER.make_regexp(%w[http https]), allow_blank: true } +validates_associated :address # Add to the Address model +validates :postal_code, format: { with: /\A\d{5}\z/, message: "should be a 5-digit number" } +validates :latitude, numericality: { greater_than_or_equal_to: -90, less_than_or_equal_to: 90 }, allow_nil: true +validates :longitude, numericality: { greater_than_or_equal_to: -180, less_than_or_equal_to: 180 }, allow_nil: trueapp/frontend/components/pages/contribution/new/OsblLocations.tsx (1)
66-72: Add confirmation dialog for location removal.Consider adding a confirmation dialog before removing locations to prevent accidental deletions.
+import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, + AlertDialogTrigger, +} from "@/components/ui/alert-dialog" <Button onClick={(e) => handleLocationRemove(e, index)} variant='outline' className='bg-white text-red-500 border-none' > - <TrashIcon className='w-4 h-4' /> + <AlertDialog> + <AlertDialogTrigger> + <TrashIcon className='w-4 h-4' /> + </AlertDialogTrigger> + <AlertDialogContent> + <AlertDialogHeader> + <AlertDialogTitle>Are you sure?</AlertDialogTitle> + <AlertDialogDescription> + This action cannot be undone. This will permanently delete the location. + </AlertDialogDescription> + </AlertDialogHeader> + <AlertDialogFooter> + <AlertDialogCancel>Cancel</AlertDialogCancel> + <AlertDialogAction onClick={(e) => handleLocationRemove(e, index)}> + Continue + </AlertDialogAction> + </AlertDialogFooter> + </AlertDialogContent> + </AlertDialog> </Button>app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx (1)
193-195: Enhance character count display.Consider adding color indication when approaching the character limit.
-<div className='text-xs text-right'> +<div className={`text-xs text-right ${ + (sheetLocation.description?.length ?? 0) > 250 + ? 'text-yellow-500' + : (sheetLocation.description?.length ?? 0) > 280 + ? 'text-red-500' + : '' +}`}> {sheetLocation.description?.length ?? 0}/300 caractères </div>spec/factories/document_attachments.rb (1)
1-6: Consider using build strategy for better test performance.The factory looks good, but using
createfor theattachableassociation will hit the database for every factory instantiation. Consider usingbuildstrategy unless persistence is specifically needed:- attachable { create(:osbl) } + attachable { build(:osbl) }spec/factories/document.rb (1)
3-3: Add sequence/trait for type flexibility.Consider using a sequence or traits to allow creating documents with different types:
- type { "Statuts" } + sequence(:type) { |n| ["Statuts", "Report", "Contract"][n % 3] } + + trait :statuts do + type { "Statuts" } + endspec/support/factory_bot.rb (1)
5-12: Enhance file upload helper robustness.The helper could be improved to handle edge cases and be more platform-independent:
module FactoryBotFileUploadHelper def fixture_file_upload(path, mime_type) + file_path = Rails.root.join(*path.split('/')) + raise ArgumentError, "File not found: #{file_path}" unless File.exist?(file_path) + Rack::Test::UploadedFile.new( - Rails.root.join(path), + file_path, mime_type ) + rescue Errno::ENOENT => e + raise ArgumentError, "Invalid file path: #{e.message}" end endapp/models/document.rb (1)
11-17: Consider using i18n for enum labels.The enum values have been changed to human-readable French strings. While this improves readability, consider using i18n translations instead for better maintainability and internationalization support.
enum :type, { - "Statuts" => 0, - "Rapport d'activité" => 1, - "Rapport financier" => 2, - "Procès verbal" => 3, - "Autre" => 4 + statuts: 0, + rapport_activite: 1, + rapport_financier: 2, + proces_verbal: 3, + autre: 4 }.freezeAdd to config/locales/fr.yml:
fr: activerecord: attributes: document: types: statuts: "Statuts" rapport_activite: "Rapport d'activité" rapport_financier: "Rapport financier" proces_verbal: "Procès verbal" autre: "Autre"app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
21-24: Consider using filter with type predicate.The document removal logic could be more type-safe using a type predicate.
function handleDocumentRemove (e: React.MouseEvent<HTMLButtonElement>, index: number): void { e.preventDefault() // prevent the form from being submitted - setData('document_attachments_attributes', documents.filter((_, i) => i !== index)) + setData('document_attachments_attributes', documents.filter((doc): doc is typeof doc => doc !== null && doc !== undefined && documents.indexOf(doc) !== index)) }spec/models/document_spec.rb (2)
92-112: Consolidate duplicate validation blocks.There are two
describe "validations"blocks. Consider consolidating them for better organization.- describe "validations" do - subject(:document) do - build(:document, file: fixture_file_upload("spec/fixtures/files/sample.pdf", "application/pdf")) - end - # ... existing tests - end describe "validations" do + subject(:document) do + build(:document, file: fixture_file_upload("spec/fixtures/files/sample.pdf", "application/pdf")) + end it "is invalid when year is greater than the current year" do # ... existing test end + # ... move other validation tests here end
48-54: Add edge case tests for year validation.The year validation tests could be more comprehensive. Consider adding tests for edge cases:
- Year exactly equal to current year
- Year as string value
- Year as decimal/float value
describe "validations" do it "is valid when year equals current year" do document = build(:document, year: Time.current.year) expect(document).to be_valid end it "handles year as string value" do document = build(:document, year: Time.current.year.to_s) expect(document).to be_valid end it "rejects decimal year values" do document = build(:document, year: 2024.5) expect(document).not_to be_valid expect(document.errors[:year]).to be_present end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/fixtures/files/sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (35)
app/controllers/users/contributions_controller.rb(2 hunks)app/frontend/components/pages/contribution/new/FundManagementSection.tsx(4 hunks)app/frontend/components/pages/contribution/new/OsblDocumentSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/shared/MyAsyncCreatableSelect.tsx(6 hunks)app/frontend/types/types.ts(1 hunks)app/models/document.rb(1 hunks)app/models/document_attachment.rb(1 hunks)app/models/fund_allocation.rb(1 hunks)app/models/fund_source.rb(1 hunks)app/models/location.rb(1 hunks)app/models/user.rb(1 hunks)db/migrate/20250204104735_modify_foreign_key_on_locations.rb(1 hunks)db/migrate/20250204105521_add_cascade_delete_to_foreign_keys.rb(1 hunks)db/schema.rb(6 hunks)spec/factories/addresses.rb(1 hunks)spec/factories/create_documents.rb(0 hunks)spec/factories/document.rb(1 hunks)spec/factories/document_attachments.rb(1 hunks)spec/factories/fund_allocation.rb(1 hunks)spec/factories/fund_sources.rb(1 hunks)spec/factories/locations.rb(1 hunks)spec/models/address_spec.rb(1 hunks)spec/models/annual_finance_spec.rb(10 hunks)spec/models/document_attachment_spec.rb(1 hunks)spec/models/document_spec.rb(1 hunks)spec/models/intervention_area.rb(1 hunks)spec/models/keyword_spec.rb(1 hunks)spec/models/location_spec.rb(1 hunks)spec/rails_helper.rb(1 hunks)spec/requests/keywords_spec.rb(1 hunks)spec/support/factory_bot.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/factories/create_documents.rb
✅ Files skipped from review due to trivial changes (3)
- spec/factories/fund_allocation.rb
- spec/factories/fund_sources.rb
- spec/models/annual_finance_spec.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- spec/models/address_spec.rb
- app/frontend/types/types.ts
- spec/models/location_spec.rb
- app/frontend/components/pages/contribution/new/FundManagementSection.tsx
🧰 Additional context used
📓 Learnings (2)
spec/models/keyword_spec.rb (1)
Learnt from: mpressen
PR: Benefactorum/app#153
File: app/models/keyword.rb:5-5
Timestamp: 2025-02-04T08:55:41.831Z
Learning: The Keyword model relies on database-level constraints for name validation (presence, uniqueness, and length between 3-100 characters) instead of ActiveRecord validations.
db/schema.rb (2)
Learnt from: mpressen
PR: Benefactorum/app#153
File: db/migrate/20250203160100_add_check_constraint_to_intervention_areas.rb:0-0
Timestamp: 2025-02-04T09:11:41.424Z
Learning: For database compatibility between SQLite and PostgreSQL:
- Use `length()` for string length
- Use `substr()` for substring operations
- Use `||` operator for string concatenation
- Function names are case-insensitive in both databases
Learnt from: mpressen
PR: Benefactorum/app#153
File: db/migrate/20250203160027_add_check_constraint_to_keywords.rb:0-0
Timestamp: 2025-02-04T09:12:09.509Z
Learning: The `length()` function in SQL is compatible with both SQLite and PostgreSQL databases in the stack, making it a safe choice for database constraints.
🪛 GitHub Check: standard_ruby
spec/models/document_attachment_spec.rb
[failure] 39-39:
Lint/UselessAssignment: Useless assignment to variable - duplicate_attachment.
🔇 Additional comments (26)
spec/models/document_attachment_spec.rb (2)
1-3: LGTM! Test setup follows RSpec best practices.
4-29: LGTM! Well-structured callback tests with clear scenarios.The tests effectively verify the document lifecycle management through attachments.
db/schema.rb (3)
169-180: Document type enum values and add website format validation.These issues were previously identified and remain valid:
- Add comments documenting the meaning of each type value
- Add website format validation
149-149: LGTM! Consistent use of database-compatible functions.The check constraints correctly use the
length()function for database compatibility between SQLite and PostgreSQL, as recommended.Also applies to: 157-157
262-277: LGTM! Consistent cascade behavior.The foreign key definitions consistently use
on_delete: :cascadeacross related tables, ensuring proper cleanup of dependent records.app/frontend/components/shared/MyAsyncCreatableSelect.tsx (5)
5-7: LGTM! Import paths have been fixed.The import paths are now correctly defined, and the necessary types are properly imported.
17-20: LGTM! Well-defined interface.The
BackendDatainterface properly types the backend response data structure.
22-25: LGTM! Properly typed helper function.The
createOptionfunction is now properly typed withBackendDatainput andSelectOptionoutput.
35-36: LGTM! Well-typed state management.The state management is properly typed with
MultiValue<SelectOption>and includes a separate state for options.
82-93: LGTM! Robust error handling.The error handling in
handleCreateis well-implemented with proper status code checking and user feedback through toast notifications.app/frontend/components/pages/contribution/new/OsblDocumentSheet.tsx (1)
1-37: LGTM! Well-structured imports and type definitions.The imports are logically organized, and the Props interface is well-defined with proper TypeScript types.
app/models/fund_allocation.rb (1)
6-15:⚠️ Breaking Change: Enum key format change requires codebase updatesConverting enum keys from symbols to strings introduces breaking changes that will affect:
- Predicate methods (e.g.,
missions_sociales?)- Scopes (e.g.,
FundAllocation.missions_sociales)- Existing code using symbol keys
Consider using symbols with i18n translations instead:
enum :type, { - "Missions sociales" => 0, - "Frais de fonctionnement" => 1, - "Frais de recherche de fonds" => 2, - "Autre" => 3 + missions_sociales: 0, + frais_de_fonctionnement: 1, + frais_de_recherche_de_fonds: 2, + autre: 3 }.freezeAdd translations in
config/locales/fr.yml:fr: activerecord: attributes: fund_allocation: types: missions_sociales: "Missions sociales" frais_de_fonctionnement: "Frais de fonctionnement" frais_de_recherche_de_fonds: "Frais de recherche de fonds" autre: "Autre"Let's verify the impact of this change:
app/models/fund_source.rb (2)
6-11: Verify the impact of changing enum values from symbols to strings.This change from symbol-based keys to string-based keys is a breaking change that could affect existing records and code. Please ensure:
- All existing records in the database are migrated correctly
- All code referencing these enum values is updated
- Any cached or serialized data using these values is handled
17-20: Verify if these validations should be enabled.These commented validations appear essential for data integrity:
- Type presence ensures valid enum values
- Percent and value validations ensure valid numerical ranges
- Uniqueness scope prevents duplicate fund sources per annual finance
Please verify if these should be uncommented or if validation is handled elsewhere.
app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx (1)
1-26: LGTM! Well-structured imports and type definitions.The imports are logically organized and the Props interface is well-defined with clear type safety.
db/migrate/20250204104735_modify_foreign_key_on_locations.rb (1)
1-6: LGTM! Verify the impact of cascade delete.The migration correctly modifies the foreign key to add cascade delete behavior. This means that when an OSBL is deleted, all its associated locations will be automatically deleted.
Run the following script to verify the foreign key constraints:
spec/factories/addresses.rb (1)
1-15: LGTM! Well-structured factory with clear documentation.The address factory is well-implemented with:
- Reasonable default values for testing
- Clear documentation of usage patterns
- Proper handling of the polymorphic association
spec/factories/locations.rb (1)
1-33: LGTM! Well-organized factory with comprehensive traits.The location factory is well-structured with:
- Proper association handling
- Comprehensive traits for all location types
- Consistent type values matching the model's enum
app/models/location.rb (1)
7-12: Enum definition still uses:typeinstead of:location_type.The enum is still using the
:typecolumn name, which was previously flagged as a potential issue.db/migrate/20250204105521_add_cascade_delete_to_foreign_keys.rb (1)
1-27: LGTM! Consider adding a safety check for existing records.The migration correctly implements cascade delete behavior for all relevant foreign key relationships. The comments clearly explain the purpose of each modification.
Consider adding a safety check to ensure no orphaned records exist before applying the cascade delete constraints:
def change + # Safety check to ensure no orphaned records exist + tables = { + "annual_finances" => "osbls", + "fund_allocations" => "annual_finances", + "fund_sources" => "annual_finances", + "document_attachments" => "documents", + "otps" => "users", + "sessions" => "users" + } + + tables.each do |child_table, parent_table| + orphaned = execute(<<-SQL).to_a + SELECT COUNT(*) FROM #{child_table} + LEFT JOIN #{parent_table} ON #{child_table}.#{parent_table.singularize}_id = #{parent_table}.id + WHERE #{parent_table}.id IS NULL + SQL + + if orphaned[0]['count'].to_i > 0 + raise "Found orphaned records in #{child_table}. Please clean up before adding cascade delete." + end + end + # Remove and re-add foreign keys with cascade delete ...✅ Verification successful
Action Required: Optional Enhancement – Safety Check for Orphaned Records
The migration correctly removes and re-adds foreign keys with cascade delete behavior. However, as suggested, it's worth considering a precautionary safety check to validate that no orphaned records exist before applying the new constraints. For example, you might iterate over each child table, join on the parent, and raise an error if any orphaned rows are found. This additional check helps ensure data integrity during the migration.
- Location:
db/migrate/20250204105521_add_cascade_delete_to_foreign_keys.rb- Suggestion: Add a safety check (as illustrated in the provided diff) to count orphaned records and halt the migration if any are detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
app/models/user.rb (1)
6-6: Verify the intended behavior of nullifying contributions.The
dependent: :nullifyoption means that when a user is deleted, their contributions will remain but will have theiruser_idset toNULL. This differs from other associations in the model that usedependent: :destroy. Consider if this is the intended behavior or ifdependent: :destroywould be more appropriate for data integrity.Run the following script to check if there are any existing contributions with NULL user_id:
spec/requests/keywords_spec.rb (1)
23-23: LGTM! Explicit keyword creation improves test readability.The change from
create_listto explicit creation makes the test more readable and the expected results clearer.app/controllers/users/contributions_controller.rb (1)
19-23: LGTM! Props enriched with necessary type information.The addition of various type information (location, document, fund source, and fund allocation types) enhances the frontend's ability to validate and display appropriate options.
app/frontend/components/pages/contribution/new/OsblLocations.tsx (1)
27-31: Add fallback for undefined location type.The function might return undefined if the location type is not set.
spec/factories/document.rb (1)
4-4: Verify sample.pdf fixture existence.The factory assumes "spec/fixtures/files/sample.pdf" exists. Let's verify this to prevent test failures.
✅ Verification successful
Sample Fixture Verified
The
sample.pdffixture exists in thespec/fixtures/files/directory, so no changes are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the sample.pdf fixture exists if [ -f "spec/fixtures/files/sample.pdf" ]; then echo "✅ sample.pdf fixture exists" else echo "❌ sample.pdf fixture is missing" fiLength of output: 108
app/models/document.rb (1)
9-9: Verify the impact of commenting out the attachables association.The commented association suggests a change in the relationship model. Ensure that:
- All code using the
attachablesassociation has been updated- No orphaned records will be created
✅ Verification successful
I'll generate a new shell script to further search for any usage of “attachables” and related attachment references throughout the codebase. This will help us verify if the commented association is safe to remove and whether any orphaned records might be created.
Impact Verified: No active usage of the
attachablesassociation appears in production code.
- The only remaining references are either in comments or in outdated spec scenarios, indicating that the removal was intentional.
- The related
document_attachmentsfunctionality remains intact, so no orphaned records are expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of attachables rg "\.attachables" --type rubyLength of output: 32
Script:
#!/bin/bash # Search for any usage of "attachables" (not just chained calls, but any textual occurrence) echo "Searching for 'attachables' usage in Ruby files..." rg -n "attachables" --type ruby # Additionally, search for any references to document_attachments that may relate to this association echo "Searching for 'document_attachment' usage in Ruby files..." rg -n "document_attachment" --type rubyLength of output: 3637
app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx
Outdated
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblFinanceSheet.tsx
Outdated
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx
Outdated
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblLocationSheet.tsx
Outdated
Show resolved
Hide resolved
- Enhanced AnnualFinance, FundAllocation, FundSource, and Osbl models with detailed validations - Added database-level constraints for year, percent, and amount fields - Improved test coverage for model validations and uniqueness constraints - Removed pending specs and added meaningful test scenarios - Updated factories to support new validation requirements
- Moved authentication and contribution form components to more organized directory structure - Refactored shared components like MyInput, MyCheckbox, and InputError - Updated import paths across frontend files to reflect new component locations - Improved type safety and component reusability - Cleaned up test files to match new component paths
- Updated type definitions for form errors to use Partial<Record> - Added StrictForm type to enforce FormDataConvertible constraints - Refined type annotations in contribution form and related components
- Updated import paths to use absolute imports with @/ prefix - Simplified and improved type definitions in Layout and inertiaConfig - Adjusted component props and type annotations for better type safety - Cleaned up import statements across multiple frontend components
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (36)
app/frontend/components/shared/MyNumberInput.tsx (2)
19-19: Consider adding JSDoc comment for the new prop.The new
noErrorMessageprop is a good addition for flexible error handling. Consider adding a JSDoc comment to document its purpose and default behavior.+ /** When true, suppresses the display of error messages. @default false */ noErrorMessage?: boolean
86-86: Simplify the error message conditional.The error message rendering logic can be simplified by removing the redundant
Boolean()conversion.- {Boolean(error) && !noErrorMessage && <InputError>{error}</InputError>} + {error && !noErrorMessage && <InputError>{error}</InputError>}app/frontend/components/shared/MyInput.tsx (5)
7-24: Consider strengthening type definitions.The type definitions are well-structured, but could be improved for better type safety:
interface BaseMyInputProps extends React.InputHTMLAttributes<HTMLInputElement> { - error?: string + error?: string | null - ref?: React.Ref<HTMLInputElement> + ref?: React.RefObject<HTMLInputElement> showResetButton?: boolean onReset?: () => void }
37-37: Simplify error class logic.The error class condition can be simplified using Boolean shorthand:
- const errorClass = (error !== null && error !== undefined && error !== '') ? 'border-red-600' : '' + const errorClass = Boolean(error) ? 'border-red-600' : ''
48-54: Improve accessibility and className management.Consider these improvements:
- Add aria-invalid for better accessibility
- Use a utility function or clsx/classnames library for className management
<Input id={id} placeholder={placeholder} + aria-invalid={Boolean(error)} - className={`bg-white focus-visible:ring-0 focus-visible:border-primary focus:border-primary focus:outline-none placeholder:text-ellipsis placeholder:text-xs md:placeholder:text-sm focus-visible:ring-offset-0 ${errorClass} ${className ?? ''} ${showResetButton ? 'pr-10' : ''}`} + className={clsx( + 'bg-white focus-visible:ring-0 focus-visible:border-primary', + 'focus:border-primary focus:outline-none', + 'placeholder:text-ellipsis placeholder:text-xs md:placeholder:text-sm', + 'focus-visible:ring-offset-0', + errorClass, + className, + showResetButton && 'pr-10' + )} {...props} ref={ref} />
55-64: Simplify reset button visibility condition.The condition for showing the reset button can be simplified:
- {showResetButton && (onReset !== undefined) && ( + {showResetButton && onReset && (
66-66: Enhance error message accessibility.Consider adding role="alert" to the error message for better screen reader support:
- {Boolean(error) && <InputError>{error}</InputError>} + {Boolean(error) && <InputError role="alert">{error}</InputError>}app/frontend/components/shared/MyFileInput.tsx (2)
14-25: Consider deriving initial hasFile state from file prop.The hasFile state could be initialized based on the file prop to maintain consistency:
- const [hasFile, setHasFile] = useState(false) + const [hasFile, setHasFile] = useState(file !== undefined)
43-70: Simplify required prop logic.The required prop logic can be simplified since it's only relevant when in file input mode:
- required={required && file === undefined} + required={required}The file existence check is redundant here because this input is only rendered when we're in file input mode (file === undefined || hasFile === true).
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx (4)
39-50: Consider enhancing validation and constants management.While the validation is good, consider these improvements:
- Move constants to a shared configuration file for reusability
- Add validation for minimum file size to prevent empty files
const MAX_DOCUMENT_SIZE = 5 * 1024 * 1024 // 5MB +const MIN_DOCUMENT_SIZE = 1 // 1 byte const ALLOWED_DOCUMENT_TYPES = ['application/pdf'] const documentValidation = z.object({ file: z.instanceof(File) + .refine((file) => { + return file.size >= MIN_DOCUMENT_SIZE + }, 'Le fichier ne peut pas être vide.') .refine((file) => { return file.size <= MAX_DOCUMENT_SIZE }, 'La taille du fichier doit être inférieure à 5 MB.')
63-70: Simplify the isOpen state initialization logic.The current implementation is complex and could be simplified for better readability.
- const [isOpen, setIsOpen] = useState(() => { - const checks = [ - (!['Rapport d\'activité', 'Rapport financier'].includes(sheetDocument.type ?? '') && sheetDocument.year !== undefined), - (!['Procès verbal', 'Autre'].includes(sheetDocument.type ?? '') && sheetDocument.name !== undefined), - sheetDocument.description !== undefined - ] - return checks.some(check => check) - }) + const [isOpen, setIsOpen] = useState(() => { + const hasOptionalYear = !['Rapport d\'activité', 'Rapport financier'].includes(sheetDocument.type ?? '') && sheetDocument.year !== undefined + const hasOptionalName = !['Procès verbal', 'Autre'].includes(sheetDocument.type ?? '') && sheetDocument.name !== undefined + const hasDescription = sheetDocument.description !== undefined + return hasOptionalYear || hasOptionalName || hasDescription + })
79-94: Enhance error handling in form submission.The current error handling only shows the first validation error. Consider handling multiple validation errors and improving error feedback.
function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): void { + e.preventDefault() + clearErrors() + if (formRef.current?.reportValidity() === false) { - e.preventDefault() return } const result = documentValidation.safeParse({ file: sheetDocument.file }) if (!result.success) { - e.preventDefault() - const errorMessage = result.error.issues[0].message - setError(`document_attachments_attributes.${index}.document_attributes.file`, errorMessage) + result.error.issues.forEach(issue => { + setError(`document_attachments_attributes.${index}.document_attributes.file`, issue.message) + }) return } onUpdate(deepCleanData(sheetDocument)) }
167-172: Improve accessibility of the collapsible trigger.The current implementation could be enhanced for better accessibility.
- <CollapsibleTrigger asChild> - <div className='flex justify-between items-center cursor-pointer text-muted-foreground hover:text-muted-foreground/80'> - <span>Informations additionnelles</span> - <ChevronDown className={`transition-transform duration-300 ${isOpen ? 'rotate-180' : ''}`} /> - </div> - </CollapsibleTrigger> + <CollapsibleTrigger asChild> + <button + type="button" + className='w-full flex justify-between items-center cursor-pointer text-muted-foreground hover:text-muted-foreground/80' + aria-expanded={isOpen} + aria-controls="additional-info-content" + > + <span>Informations additionnelles</span> + <ChevronDown className={`transition-transform duration-300 ${isOpen ? 'rotate-180' : ''}`} aria-hidden="true" /> + </button> + </CollapsibleTrigger>spec/models/osbl_spec.rb (3)
13-58: Consider adding edge case tests for more robust validation coverage.While the validation tests are well-structured, consider adding:
- Edge cases for description (empty, nil, exactly 300 chars)
- Website format validation tests
- Additional creation_year scenarios (current year, past years)
Example additions:
describe "description" do it "allows empty description" do osbl.description = "" expect(osbl).to be_valid end it "allows exactly 300 characters" do osbl.description = "a" * 300 expect(osbl).to be_valid end end describe "website" do it "validates website format" do invalid_websites = ["not-a-url", "ftp://invalid.com"] invalid_websites.each do |website| osbl.website = website expect(osbl).not_to be_valid expect(osbl.errors[:website]).to be_present end end end describe "creation_year" do it "allows current year" do osbl.creation_year = Time.current.year expect(osbl).to be_valid end it "allows past years" do osbl.creation_year = 1900 expect(osbl).to be_valid end end
68-89: Enhance logo attachment test coverage.Consider adding tests for:
- Valid file types and sizes
- Edge cases around size limits
- Different image formats
Example additions:
describe "logo attachment" do it "accepts valid image types and sizes" do valid_types = %w[image/jpeg image/png image/gif] valid_types.each do |content_type| osbl.logo.attach( io: StringIO.new("a" * 1.megabyte), filename: "logo#{content_type.split('/').last}", content_type: content_type ) expect(osbl).to be_valid end end it "accepts files just under size limit" do osbl.logo.attach( io: StringIO.new("a" * (2.megabytes - 1)), filename: "logo.png", content_type: "image/png" ) expect(osbl).to be_valid end end
91-97: Add boundary tests for creation_year constraint.Consider testing:
- Boundary values (999, 10000)
- Non-numeric values
Example additions:
describe "db constraints" do it "rejects 3-digit years" do osbl.creation_year = 999 expect { osbl.save! }.to raise_error(ActiveRecord::StatementInvalid, /creation_year_as_4_digits/) end it "rejects 5-digit years" do osbl.creation_year = 10000 expect { osbl.save! }.to raise_error(ActiveRecord::StatementInvalid, /creation_year_as_4_digits/) end endapp/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx (3)
17-25: Enhance type safety in the props interface.Consider improving type safety by:
- Making
itemsarray elements required usingRequired<FundRecord>[]- Being more specific with the error types instead of using
Partial<Record<string, string>>interface FundManagementSectionProps { title: string - items: FundRecord[] + items: Required<FundRecord>[] typeList: string[] baseErrorPath: string onUpdate: (items: Array<Partial<FundRecord>>) => void - errors: Partial<Record<string, string>> + errors: Record<`${typeof baseErrorPath}.${string}`, string> clearErrors: (path: string) => void }
96-99: Extract type filtering logic for better readability.The filtering logic inside the JSX makes the code harder to read.
+ const getAvailableTypes = (currentType: string, allItems: FundRecord[], types: string[]) => + types.filter(type => currentType === type || !allItems.some(source => source.type === type)) {typeList.filter(type => - item.type === type || - !items.some(source => source.type === type) + getAvailableTypes(item.type, items, typeList) ).map((type) => (
80-141: Consider memoizing filtered type list for better performance.The type filtering operation is performed on every render. Consider using
useMemoto optimize performance, especially with larger type lists.+ const filteredTypeList = useMemo( + () => typeList.filter(type => + item.type === type || !items.some(source => source.type === type) + ), + [item.type, items, typeList] + ); {items.map((item, index) => ( // ... - {typeList.filter(type => - item.type === type || - !items.some(source => source.type === type) - ).map((type) => ( + {filteredTypeList.map((type) => (app/frontend/components/pages/contribution/new/OsblFinances.tsx (4)
11-14: Consider enhancing type safety for finances initialization.While the null coalescing operator is a good practice, consider adding explicit typing for better type safety.
- const finances = data.annual_finances_attributes ?? [] + const finances: AnnualFinance[] = data.annual_finances_attributes ?? []
43-53: Improve accessibility for sort button.The sort button needs accessibility improvements for screen readers.
<Button variant='ghost' size='icon' + aria-label={`Sort finances ${sortAscending ? 'descending' : 'ascending'}`} onClick={(e) => { e.preventDefault() handleSort() }} disabled={finances.length < 2} > - <ArrowUpDown /> + <ArrowUpDown className="h-4 w-4" /> + <span className="sr-only">Sort by year</span> </Button>
81-81: Use more unique key for Sheet components.Using just the year as part of the key might cause issues if multiple finances exist for the same year.
- <Sheet key={`finance-${finance.year}`}> + <Sheet key={`finance-${finance.year}-${index}`}>
85-89: Add accessible label for edit button.The edit button needs an accessible label for screen readers.
<SheetTrigger asChild> <Button variant='outline' className='bg-white text-primary border-none'> - <PencilIcon /> + <PencilIcon className="h-4 w-4" /> + <span className="sr-only">Edit finance for year {finance.year}</span> </Button> </SheetTrigger>app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (3)
29-72: Refactor validation function for better maintainability.The validation function is complex and could benefit from being broken down into smaller, more focused functions. Consider extracting the fund sources and allocations validation into separate functions.
+const validateFundSources = (sources: Array<{ percent: string }> | undefined): boolean => { + if (sources === undefined || sources.length === 0) return true + const sum = sources.reduce((acc, source) => + acc + (source.percent !== undefined ? Number(source.percent) : 0), 0) + return sum === 100 +} +const validateFundAllocations = (allocations: Array<{ percent: string }> | undefined): boolean => { + if (allocations === undefined || allocations.length === 0) return true + const sum = allocations.reduce((acc, allocation) => + acc + (allocation.percent !== undefined ? Number(allocation.percent) : 0), 0) + return sum === 100 +} const financeValidation = (data: AnnualFinance[], currentIndex: number): z.ZodType<AnnualFinance> => z.object({ // ... year validation ... fund_sources_attributes: z.array(z.object({ percent: z.string() })).optional() - .refine((sources): sources is typeof sources => { - if (sources === undefined || sources.length === 0) return true - const sum = sources?.reduce((acc: number, source) => { - const percent = source.percent !== undefined ? Number(source.percent) : 0 - return acc + percent - }, 0) - return sum === 100 - }, { + .refine(validateFundSources, { message: 'La somme des pourcentages doit être égale à 100%.', path: ['total_percent'] }), fund_allocations_attributes: z.array(z.object({ percent: z.string() })).optional() - .refine((allocations): allocations is typeof allocations => { - if (allocations === undefined || allocations.length === 0) return true - const sum = allocations?.reduce((acc: number, allocation) => { - const percent = allocation.percent !== undefined ? Number(allocation.percent) : 0 - return acc + percent - }, 0) - return sum === 100 - }, { + .refine(validateFundAllocations, { message: 'La somme des pourcentages doit être égale à 100%.', path: ['total_percent'] })
88-104: Optimize updateSheetFinance with useCallback.Consider memoizing the update function to prevent unnecessary re-renders.
-function updateSheetFinance (field: keyof AnnualFinance, value: any): void { +const updateSheetFinance = useCallback((field: keyof AnnualFinance, value: any): void => { setSheetFinance(prev => ({ ...prev, [field]: value })) if (field === 'year' && value === '') { clearErrors(`annual_finances_attributes.${index}.missing_information`) } if (field !== 'year' && value !== '') { clearErrors(`annual_finances_attributes.${index}.missing_information`) } if (field === 'year') { clearErrors(`annual_finances_attributes.${index}.year`) } -} +}, [clearErrors, index])
127-236: Enhance form accessibility.The form could benefit from improved accessibility features.
- <form ref={formRef}> + <form + ref={formRef} + aria-labelledby="finance-sheet-title" + role="form" + > <SheetHeader> - <SheetTitle>Comptes annuels</SheetTitle> + <SheetTitle id="finance-sheet-title">Comptes annuels</SheetTitle> <SheetDescription className='sr-only'> Renseignez les informations financières pour cette année. </SheetDescription> </SheetHeader>Also consider adding aria-live regions for error messages to improve the accessibility of form validation feedback.
app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx (3)
44-44: Add error handling for location types.Consider adding error handling for when
location_typesis undefined in page props.- const locationTypes = usePage().props.location_types as string[] + const { location_types = [] } = usePage().props + const locationTypes = location_types as string[]
47-55: Simplify isOpen state calculation.The current implementation can be simplified using Array.some() directly on the object fields.
- const [isOpen, setIsOpen] = useState(() => { - const checks = [ - sheetLocation.name !== undefined, - sheetLocation.address_attributes?.additional_info !== undefined, - sheetLocation.website !== undefined, - sheetLocation.description !== undefined - ] - return checks.some(check => check) - }) + const [isOpen, setIsOpen] = useState(() => + Object.entries({ + name: sheetLocation.name, + additional_info: sheetLocation.address_attributes?.additional_info, + website: sheetLocation.website, + description: sheetLocation.description + }).some(([_, value]) => value !== undefined) + )
91-220: Enhance form accessibility and validation.Consider the following improvements:
- Add aria-labels to interactive elements
- Add input validation patterns where applicable
- Add error messages for validation failures
Example improvements for the website input:
<MyInput labelText='Site web' type='url' id={`location-website-${index}`} value={sheetLocation.website ?? ''} onChange={(e) => updateSheetLocation('website', e.target.value)} + pattern="https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*)" + aria-label="Website URL" + title="Please enter a valid URL starting with http:// or https://" />For the description textarea:
<Textarea id={`location-description-${index}`} value={sheetLocation.description ?? ''} maxLength={300} onChange={(e) => updateSheetLocation('description', e.target.value)} 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-full h-40' + aria-label="Location description" + aria-describedby={`location-description-${index}-count`} /> <div + id={`location-description-${index}-count`} className='text-xs text-right' > {sheetLocation.description?.length ?? 0}/300 caractères </div>app/frontend/pages/Contribution/types.ts (1)
70-76: Consider typing the error value in setError.The
errorparameter insetErroris typed asany, which could lead to inconsistent error handling.- setError: (field: string, error: any) => void + setError: (field: string, error: string) => voidapp/frontend/components/pages/contribution/new/OsblLocations.tsx (2)
11-14: Consider initializing locations as an empty array.Using nullish coalescing for initialization is good, but consider moving it to a constant for better readability.
- const locations = data.locations_attributes ?? [] + const DEFAULT_LOCATIONS: Location[] = [] + const locations = data.locations_attributes ?? DEFAULT_LOCATIONS
44-50: Consider memoizing the key prop.The
keyprop is dynamically generated on each render, which could lead to unnecessary re-renders.+ const newLocationKey = `location-${locations.length}` <OsblLocationSheet - key={`location-${locations.length}`} + key={newLocationKey} location={{}} index={locations.length} onUpdate={(loc) => addLocation(loc, locations.length)} hasSiegeSocial={hasSiegeSocial} />app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
49-57: Consider memoizing callback functions.The
onUpdatecallback is recreated on every render.+import { useCallback } from 'react' + export default function OsblDocuments ({ data, setData, errors, clearErrors, setError }: FormProps): ReactElement { const documents = data.document_attachments_attributes ?? [] + + const handleDocumentUpdate = useCallback((doc: Document, index: number) => { + addDocument(doc, index) + }, []) // ... rest of the component <OsblDocumentSheet key={`document-${documents.length}`} document={{}} index={documents.length} - onUpdate={(doc) => addDocument(doc, documents.length)} + onUpdate={(doc) => handleDocumentUpdate(doc, documents.length)} errors={errors} clearErrors={clearErrors} setError={setError} />app/frontend/pages/Contribution/New.tsx (1)
78-85: Consider consolidating duplicate save buttons.The form has two identical save buttons (one at the top and one at the bottom). While this can improve UX for long forms, consider:
- Extracting the button to a reusable component to avoid duplication
- Using different variants to distinguish primary/secondary actions
+const SaveButton = ({ processing }: { processing: boolean }): ReactElement => ( + <Button type='submit' disabled={processing} className='text-lg'> + <Save className='mr-2' /> + Enregistrer + </Button> +) -<Button type='submit' disabled={processing} className='text-lg'> - <Save className='mr-2' /> - Enregistrer -</Button> +<SaveButton processing={processing} />Also applies to: 106-109
app/frontend/components/pages/contribution/new/OsblDatasheet.tsx (2)
24-38: Consider moving constants to a separate configuration file.The
CausesListconstant contains business logic that might need to be maintained separately. Consider moving it to a dedicated configuration file for better maintainability.// app/frontend/config/causes.ts +import { Trees, Baby, Stethoscope, Coins, BookMarked, PawPrint, Microscope, Brush, Globe, Accessibility, Scale, Church, Shuffle } from 'lucide-react' + +export const CausesList = [ + { value: 'environnement', label: 'Environnement', icon: Trees }, + // ... rest of the causes +]
94-128: Improve accessibility of radio group labels.The click handlers on labels are redundant as the
htmlForattribute already provides this functionality. Also, consider adding ARIA labels for better screen reader support.<Label htmlFor='interet-general' className='flex items-center justify-between w-16 cursor-pointer' - onClick={() => { - setData('tax_reduction', 'intérêt_général') - clearErrors('tax_reduction') - }} + aria-label="Réduction d'impôt de 66% pour les associations d'intérêt général" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
app/frontend/Layout.tsx(1 hunks)app/frontend/components/pages/auth/connection/ConnectionForm.tsx(1 hunks)app/frontend/components/pages/auth/signIn/SignInForm.tsx(1 hunks)app/frontend/components/pages/auth/signUp/SignUpForm.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDatasheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDocuments.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblHeader.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx(1 hunks)app/frontend/components/shared/MyCheckbox.tsx(1 hunks)app/frontend/components/shared/MyFileInput.tsx(1 hunks)app/frontend/components/shared/MyInput.tsx(1 hunks)app/frontend/components/shared/MyNumberInput.tsx(4 hunks)app/frontend/lib/inertiaConfig.ts(1 hunks)app/frontend/pages/Auth/Connection.tsx(1 hunks)app/frontend/pages/Auth/SignIn.tsx(1 hunks)app/frontend/pages/Auth/SignUp.tsx(1 hunks)app/frontend/pages/Contribution/New.tsx(5 hunks)app/frontend/pages/Contribution/types.ts(3 hunks)app/frontend/tests/components/InputError.test.tsx(1 hunks)app/frontend/tests/forms/ConnectionForm.test.tsx(1 hunks)app/frontend/tests/forms/SignInForm.test.tsx(1 hunks)app/models/annual_finance.rb(2 hunks)app/models/fund_allocation.rb(1 hunks)spec/factories/fund_sources.rb(1 hunks)spec/factories/osbls.rb(1 hunks)spec/models/annual_finance_spec.rb(12 hunks)spec/models/create_document_spec.rb(0 hunks)spec/models/document_attachment_spec.rb(1 hunks)spec/models/fund_allocation_spec.rb(1 hunks)spec/models/fund_source_spec.rb(1 hunks)spec/models/osbl_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/models/create_document_spec.rb
✅ Files skipped from review due to trivial changes (9)
- app/frontend/tests/components/InputError.test.tsx
- app/frontend/components/pages/auth/signIn/SignInForm.tsx
- app/frontend/components/pages/auth/signUp/SignUpForm.tsx
- app/frontend/pages/Auth/SignUp.tsx
- app/frontend/components/shared/MyCheckbox.tsx
- app/frontend/components/pages/auth/connection/ConnectionForm.tsx
- app/frontend/pages/Auth/Connection.tsx
- app/frontend/pages/Auth/SignIn.tsx
- app/frontend/tests/forms/ConnectionForm.test.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- app/models/annual_finance.rb
- spec/factories/osbls.rb
- spec/factories/fund_sources.rb
- spec/models/document_attachment_spec.rb
- app/frontend/tests/forms/SignInForm.test.tsx
- app/frontend/components/pages/contribution/new/OsblHeader.tsx
🔇 Additional comments (30)
app/frontend/components/shared/MyNumberInput.tsx (2)
4-4: LGTM! Import path update aligns with component organization.The move of
InputErrorto the shared components directory reflects good architectural organization for reusable components.
49-50: LGTM! Props destructuring with appropriate defaults.The destructuring follows the existing pattern and sets an appropriate default value for
noErrorMessage.app/frontend/components/shared/MyFileInput.tsx (1)
4-12: LGTM! Props interface changes improve type safety.The updated prop types better align with React's controlled component pattern by directly handling File objects and allowing external control of the selected file.
app/frontend/components/pages/contribution/new/OsblDocuments/OsblDocumentSheet.tsx (2)
1-37: LGTM! Well-structured imports and interface definition.The imports are logically organized, and the Props interface is well-defined with proper TypeScript typing.
1-234: Overall, this is a well-implemented React component.The component demonstrates good practices in terms of:
- Type safety with TypeScript
- Form validation with Zod
- State management with React hooks
- UI organization and conditional rendering
While some improvements were suggested, they are mostly refinements to an already solid implementation.
spec/models/osbl_spec.rb (1)
4-11: LGTM! Well-structured factory test setup.Good use of RSpec's subject helper and basic factory validation.
app/models/fund_allocation.rb (2)
6-15: LGTM! Improved enum readability.The change from symbol-based to string-based enum values makes the code more maintainable and user-friendly.
17-19: Verify if validations should be uncommented.These commented validations appear to be implemented at the database level (as evidenced by the test files). Consider uncommenting them to add an additional layer of validation at the model level.
✅ Verification successful
Action: Uncomment validations in app/models/fund_allocation.rb
The search output confirms that all validations (for:type,:percent, and:amount) remain commented out. Since tests and database constraints are already enforcing these rules, adding model-level validations would provide an extra layer of data integrity and improve error messaging.
- Verify that enabling validation on the
typefield will not interfere with any STI behavior or reserved-word issues in Rails.- Once confirmed, uncomment these validations to enhance early error detection before database operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these validations are enforced elsewhere in the codebase rg -A 5 "validates.*:type.*:percent.*presence: true" --type ruby rg -A 5 "validates.*:amount.*numericality" --type rubyLength of output: 793
spec/models/fund_source_spec.rb (1)
4-34: LGTM! Well-structured database constraint tests.The test suite comprehensively covers all database constraints with clear error expectations.
spec/models/fund_allocation_spec.rb (1)
4-40: LGTM! Comprehensive test coverage.The test suite follows consistent patterns and thoroughly validates all database constraints. The factory validity test is a good addition.
spec/models/annual_finance_spec.rb (4)
4-8: LGTM! Added factory validation.Good practice to ensure the factory produces valid instances.
14-20: LGTM! Important business logic validation.The year validation ensures that future years cannot be entered, which is a crucial business rule.
193-208: LGTM! Essential database constraints.The tests ensure that critical business values (year, employees_count, budget) remain within valid ranges.
39-41: LGTM! Consistent enum value updates.The string-based enum values are consistently updated throughout the test file, improving readability.
Also applies to: 52-54, 81-83, 91-93, 94-97, 104-111, 119-126, 139-141, 149-155, 163-169, 178-184
app/frontend/components/pages/contribution/new/OsblFinances.tsx (1)
1-10: LGTM! Well-organized imports.The imports are properly organized and follow a good structure, separating React core, types, UI components, and custom components.
app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (1)
1-21: LGTM! Well-organized imports.The imports are properly organized and demonstrate good separation of concerns with shared components.
app/frontend/Layout.tsx (3)
22-28: LGTM! Clean implementation of flash message handling.The useEffect hook properly handles all flash types with appropriate type checking before showing toasts.
34-46: LGTM! Clean conditional rendering.The sidebar rendering logic is well-structured with appropriate semantic HTML tags and consistent class naming.
14-14: Consider making children required.While making
childrenoptional follows React patterns, the component always renders children. An empty layout might not be desirable.Let's verify if there are any cases where the Layout is used without children:
✅ Verification successful
Let's run a new check to look for self-closing
<Layout … />usages (which implies no children are provided) throughout the codebase.
Let's try a slightly different approach to search for self-closing usages of the Layout component (which would indicate cases where no children are provided):
This command will list any files where Layout is used in a self-closing manner.
Verification Outcome: All Layout usages include children
Our additional script searches for self‑closing
<Layout ... />usages returned no results, indicating that the Layout component is never used without children. This supports the suggestion to make thechildrenprop required since all current usages provide children.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Layout usage without children ast-grep --pattern 'Layout showSidebar={$_} flash={$_} />'Length of output: 60
Script:
#!/bin/bash # Search for self-closing usage of Layout which indicates missing children rg '<Layout\s[^>]*\/>' --with-filenameLength of output: 138
Script:
#!/bin/bash # Search for any line containing "<Layout" and then filter for "/>" rg '<Layout' --with-filename | grep '/>'Length of output: 42
app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx (1)
1-36: Verify the Location type definition.The component relies on the
Locationtype imported from@/pages/Contribution/types. Please ensure that all required fields are properly typed, especiallyaddress_attributeswhich is used extensively in the component.Let's check the Location type definition:
✅ Verification successful
Location type definition verified.
The search confirms that in app/frontend/pages/Contribution/types.ts the
Locationinterface is defined with the required fields—most notably,address_attributesis correctly required. No issues were detected with the type definition in relation to its usage in the component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the Location type definition ast-grep --pattern 'type Location = { $$$ }' # Or if it's an interface ast-grep --pattern 'interface Location { $$$ }'Length of output: 541
app/frontend/lib/inertiaConfig.ts (2)
5-9: LGTM! Well-defined interface for component resolution.The
ResolvedComponentinterface clearly defines the expected structure for components with layout functions.
11-12: LGTM! Improved null check clarity.The explicit null check in
getTitleenhances code readability and type safety.app/frontend/pages/Contribution/types.ts (3)
9-23: LGTM! Improved type safety with required fields.Making
type,percent, andyearrequired fields enhances type safety and makes the contract clearer.
51-68: LGTM! Well-structured nested attributes.The
FormDatainterface properly handles nested attributes for documents and locations.
25-49: Consider validating geographical coordinates.The
Addressinterface includes latitude and longitude fields but lacks validation constraints.Consider adding runtime validation to ensure coordinates are within valid ranges:
- Latitude: -90 to 90
- Longitude: -180 to 180
app/frontend/components/pages/contribution/new/OsblLocations.tsx (1)
27-31: Add fallback for undefined location type.The function might return undefined if no matching type is found.
app/frontend/components/pages/contribution/new/OsblDocuments.tsx (1)
26-36: Guard against undefined document attributes.The
getDocumentDisplayNamefunction needs better type checking.app/frontend/pages/Contribution/New.tsx (3)
9-14: LGTM! Import statements are well-organized.The imports are logically grouped and the path updates reflect the new project structure.
37-41: Great addition of type safety with StrictForm!The
StrictFormtype ensures that form data is properly typed and compatible with FormData conversion.
95-104: Well-structured responsive grid layout!The grid layout with responsive breakpoints provides a good user experience across different screen sizes.
app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx
Outdated
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx
Show resolved
Hide resolved
app/frontend/components/pages/contribution/new/OsblDatasheet.tsx
Outdated
Show resolved
Hide resolved
- Enhanced type definitions in Layout, OsblDatasheet, and other components - Updated input types for website and URL fields with placeholders - Refined type annotations in MyAsyncSelect and other shared components - Improved error handling and type safety in various form input methods - Updated Inertia page resolution to throw an error for missing pages - Cleaned up commented-out validation and code comments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx (1)
48-51:⚠️ Potential issueInitialize new fund records with default values.
Adding an empty object
{}could lead to runtime errors as it doesn't match the expectedFundRecordshape.function handleAdd (e: React.MouseEvent<HTMLButtonElement>): void { e.preventDefault() // prevent form submission - onUpdate([...items, {}]) + onUpdate([...items, { type: '', percent: null, amount: null }]) }
🧹 Nitpick comments (11)
app/frontend/lib/inertiaConfig.ts (1)
5-9: Consider adding a more specific type for the flash prop.The
flashproperty is currently typed asany. Consider defining a more specific interface for the flash messages to improve type safety.+interface FlashMessage { + type: 'success' | 'error' | 'info' + message: string +} interface ResolvedComponent { default: { - layout: (pageContent: ReactElement & { props: { flash?: any } }) => ReactElement + layout: (pageContent: ReactElement & { props: { flash?: FlashMessage } }) => ReactElement } }app/frontend/components/shared/MyFileInput.tsx (2)
24-30: Consider removing redundant state.The
hasFilestate might be redundant since the file's presence can be determined from thefileprop. Consider simplifying the component by removing this state and using!!fileinstead.- const [hasFile, setHasFile] = useState(false) const handleFileChange = (e: React.ChangeEvent<HTMLInputElement>): void => { - const fileAttached = (e.target.files != null) && e.target.files.length > 0 - setHasFile(!!fileAttached) onChange(e.target.files?.[0] ?? undefined) }
41-66: Simplify conditional rendering and reduce prop duplication.The render logic could be simplified by:
- Using a simpler condition
- Extracting common MyInput props to reduce duplication
- <div className='flex flex-col'> - {file !== undefined && !hasFile + <div className='flex flex-col'> + {file && !hasFile ? ( + const commonProps = { + labelText, + error, + showResetButton: true, + onReset: handleReset, + } <MyInput - labelText={labelText} + {...commonProps} value={file.name} readOnly - showResetButton - onReset={handleReset} - error={error} /> ) : ( <MyInput - labelText={labelText} + {...commonProps} ref={inputRef} id={id} type='file' required={required && file === undefined} accept={accept} onChange={handleFileChange} - error={error} - showResetButton={hasFile} - onReset={handleReset} /> )}app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx (1)
110-120: Add input validation for percentage field.The percentage input should validate that the total across all funds doesn't exceed 100%.
<MyNumberInput id='percent' min={0.01} max={100} step={0.01} value={item.percent ?? ''} - onChange={(e) => handleFundChange(index, 'percent', e.target.value)} + onChange={(e) => { + const newValue = parseFloat(e.target.value); + const totalPercent = items.reduce((sum, curr, i) => + i === index ? sum : sum + (curr.percent ?? 0), 0) + newValue; + if (totalPercent <= 100) { + handleFundChange(index, 'percent', e.target.value); + } + }} placeholder='% *' suffix='%' required />app/frontend/components/shared/MyAsyncSelect.tsx (2)
29-29: Replaceanytype with a more specific type.The
setDataprop uses theanytype which should be replaced with a more specific type to improve type safety.- setData: (key: string, value: any) => void + setData: (key: string, value: Address) => void
68-69: Improve error handling UX.Consider displaying a user-friendly error message instead of just logging to console.
- console.error(error) - return [] + console.error('Error fetching addresses:', error) + throw new Error('Une erreur est survenue lors de la recherche d\'adresses. Veuillez réessayer.')app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (3)
28-71: Enhance validation error handling.The validation logic could be improved in several ways:
- Add type guards for the percentage calculations
- Provide more descriptive error messages for validation failures
- Consider extracting the percentage validation into a reusable function
const financeValidation = (data: AnnualFinance[], currentIndex: number): z.ZodType<AnnualFinance> => z.object({ year: z.string().refine( (year) => !data.some((f, i) => i !== currentIndex && f.year === Number(year)), { message: 'Un bilan comptable est déjà enregistré pour cette année.', path: ['annual_finances_attributes', currentIndex, 'year'] } ), + // Extract percentage validation into a reusable function + const validatePercentageTotal = (items?: Array<{ percent?: string }>, errorPath: string) => { + if (!items?.length) return true; + const sum = items.reduce((acc, item) => { + const percent = Number(item.percent ?? '0'); + return isNaN(percent) ? acc : acc + percent; + }, 0); + return Math.abs(sum - 100) < 0.01; // Use epsilon comparison for floating point + }; + fund_sources_attributes: z.array(z.object({ - percent: z.string() + percent: z.string().refine( + (val) => !isNaN(Number(val)), + { message: 'Le pourcentage doit être un nombre valide.' } + ) })).optional() .refine( - (sources): sources is typeof sources => { - if (sources === undefined || sources.length === 0) return true - - const sum = sources?.reduce((acc: number, source) => { - const percent = source.percent !== undefined ? Number(source.percent) : 0 - return acc + percent - }, 0) - return sum === 100 - }, + (sources) => validatePercentageTotal(sources, 'fund_sources'), { message: 'La somme des pourcentages doit être égale à 100%.', path: ['total_percent'] } ), // Similar changes for fund_allocations_attributes...
87-103: Improve type safety in updateSheetFinance.The function uses
anytype which should be replaced with a more specific type.- function updateSheetFinance (field: keyof AnnualFinance, value: any): void { + function updateSheetFinance<K extends keyof AnnualFinance> ( + field: K, + value: AnnualFinance[K] + ): void { setSheetFinance(prev => ({ ...prev, [field]: value }))
213-213: Fix typo in label text.The label "Nombre d'employé" should be pluralized.
- labelText="Nombre d'employé" + labelText="Nombre d'employés"app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx (2)
31-36: Consider adding unit test coverage for this new component.The component introduces a number of props, state transitions, and validation logic that would benefit from automated testing. It helps ensure the form behaves as expected, especially on edge cases (e.g., invalid location type, missing address).
64-71: Improve the type safety ofhandleAddressChange.Using
anyfor thevalueparameter could allow inadvertent type mismatches. Consider specifying a more precise type (e.g.,Partial<Address>or a union type) to better guard against invalid assignments.-function handleAddressChange (value: any, field?: string): void { +function handleAddressChange (value: Partial<Address>, field?: keyof Address): void { setSheetLocation(prev => ({ ...prev, address_attributes: field != null ? { ...prev.address_attributes, [field]: value } : value })) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/frontend/Layout.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblDatasheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblHeader.tsx(1 hunks)app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx(1 hunks)app/frontend/components/shared/MyAsyncSelect.tsx(1 hunks)app/frontend/components/shared/MyFileInput.tsx(1 hunks)app/frontend/lib/inertiaConfig.ts(1 hunks)app/models/location.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/frontend/Layout.tsx
- app/models/location.rb
- app/frontend/components/pages/contribution/new/OsblDatasheet.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (20)
app/frontend/lib/inertiaConfig.ts (2)
11-12: LGTM! Clear and explicit null check.The explicit null check improves code clarity and maintainability.
14-31: Improved error handling implemented as suggested.The function now properly throws an error when a page is not found, preventing potential runtime errors. The optional chaining for the flash prop adds additional safety.
app/frontend/components/pages/contribution/new/OsblHeader.tsx (4)
3-3: LGTM! Component structure improvements.The changes improve the component organization by:
- Moving MyInput to shared components
- Removing unused setError prop using TypeScript's Omit utility
Also applies to: 9-9
11-13: LGTM! Improved layout structure.The updated layout provides better spacing and visual hierarchy using flexbox and consistent padding.
53-56: LGTM! Simplified file input handler.The file input handler now directly accepts a file parameter instead of an event object, making the code more concise and type-safe.
65-78: LGTM! Enhanced description field with proper validation.Good addition of:
- Character limit validation (300 chars)
- Real-time character count display
- Consistent error handling
app/frontend/components/shared/MyFileInput.tsx (2)
10-11: LGTM! Props interface changes improve type safety.The updated prop types make the component's API more intuitive by directly handling File objects instead of events.
18-21: LGTM! Component signature changes align with interface updates.The default value for
requiredand the addition of thefileprop in the destructuring are consistent with the interface changes.app/frontend/components/pages/contribution/new/OsblFinances/FundManagementSection.tsx (2)
1-25: LGTM! Well-structured imports and interface definition.The imports are properly organized, and the interface is comprehensive with clear type definitions.
85-108: Enhance keyboard navigation for the Select component.The Select component should support keyboard navigation for better accessibility.
<Select value={item.type} onValueChange={(value) => handleFundChange(index, 'type', value)} + aria-label={`Select fund type ${index + 1}`} required > <SelectTrigger className='w-60 data-[placeholder]:text-muted-foreground mt-4' + aria-label={`Fund type ${index + 1}`} + onKeyDown={(e) => { + if (e.key === 'ArrowDown' || e.key === 'ArrowUp') { + e.preventDefault(); + const availableTypes = typeList.filter(type => + item.type === type || !items.some(source => source.type === type) + ); + const currentIndex = availableTypes.indexOf(item.type); + const nextIndex = e.key === 'ArrowDown' + ? (currentIndex + 1) % availableTypes.length + : (currentIndex - 1 + availableTypes.length) % availableTypes.length; + handleFundChange(index, 'type', availableTypes[nextIndex]); + } + }} >app/frontend/components/shared/MyAsyncSelect.tsx (2)
37-47: LGTM! Well-structured option creation.The
createOptionfunction is well-typed and correctly maps the API response to the required format.
86-114: LGTM! Well-implemented async select component.The component implementation is solid with:
- Proper debouncing of API calls
- Input validation
- Clean styling
- Localized messages
app/frontend/components/pages/contribution/new/OsblFinances/OsblFinanceSheet.tsx (5)
1-26: LGTM! Well-structured imports and interface definition.The imports are well-organized, and the Props interface is clearly defined with appropriate types.
28-28: Replaceanytype with a specific type.The
anytype should be replaced with a specific type for better type safety.
73-86: LGTM! Clean component setup with proper hooks usage.The component is well-structured with appropriate use of hooks and clear props destructuring.
105-122: Add defensive checks in handleSubmit.The submit handler should validate that required props exist before proceeding.
124-238: LGTM! Well-structured UI with proper accessibility.The UI implementation is clean, uses shared components effectively, and includes proper accessibility features.
app/frontend/components/pages/contribution/new/OsblLocations/OsblLocationSheet.tsx (3)
57-62: Well-typedupdateSheetLocation.Great job using a generic with
K extends keyof Locationto maintain type safety and reduce the risk of runtime errors!
73-80: Add error handling in the form submission.Catching errors from
deepCleanDataoronUpdateprovides better user feedback if something fails. This was previously suggested in an older review.-function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): void { +async function handleSubmit (e: React.MouseEvent<HTMLButtonElement>): Promise<void> { if (formRef.current?.reportValidity() === false) { e.preventDefault() return } + try { onUpdate(deepCleanData(sheetLocation)) + } catch (error) { + console.error('Failed to update location:', error) + // Optionally display an error message to the user + } }
213-218: Confirm whether invalid form submission still closes the sheet.Because
SheetCloseis applied as a parent to the “Valider” button, verify that returning early aftere.preventDefault()prevents the sheet from closing if the form is invalid. If not, consider removing theSheetClosewrapper or controlling the sheet’s open state using the component’s state instead.
back and front work.
Summary by CodeRabbit
New Features
UI Enhancements
User Experience Improvements