Repository navigation
integrate description generation to donor offer creation + bug fixes - #190
Conversation
❌ Deploy Preview for hope-for-haiti failed. Why did it fail? →
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Disclaimer: This comment was AI-generated and is not necessarily completely accurate. Please take code comments with a grain of salt.
Greptile Overview
Greptile Summary
This PR integrates AI-powered description generation directly into the donor offer creation workflow, eliminating the need for a separate /api/suggest/description endpoint. The changes add an optional description field to the GeneralItem Prisma model and modify the POST /api/donorOffers route to call DescriptionService.getOrGenerateDescriptions inline before creating the donor offer. File parsing logic was refactored to remap donor-provided column names (e.g., "Generic Description") to internal field names (e.g., "title") using a Map-based approach. The donor offer service was changed from createMany bulk inserts to individual upsert operations to handle duplicate items by incrementing initialQuantity. Frontend components (AdminDynamicDonorOfferScreen, PreviewTable) now display the generated descriptions. This consolidation reduces API roundtrips and aligns with the architecture described in issue #181.
PR Description Notes:
- The PR title states "bug fixes" but the description doesn't specify which bugs were fixed beyond the process change integration.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| prisma/schema.prisma | 5/5 | Added optional description field to GeneralItem model for storing LLM-generated descriptions |
| prisma/migrations/20251021023559_add_general_item_description/migration.sql | 5/5 | Migration adds nullable description TEXT column to GeneralItem table |
| src/services/descriptionService.ts | 5/5 | Added deduplication logic using missingIds Set to prevent processing duplicate items |
| package.json | 4/5 | Updated openai dependency from ^4.63.0 to ^4.104.0 (41 minor versions) and reordered alphabetically |
| src/screens/DonorOffersScreens/AdminDynamicDonorOfferScreen.tsx | 5/5 | Added optional description field to type and new table column displaying descriptions |
| src/app/donorOffers/create/page.tsx | 4/5 | Changed endpoint from /api/donorOffers/create to /api/donorOffers and added loading modal; contains debug console.log |
| src/components/DonorOffers/PreviewTable.tsx | 4/5 | Added description field to type and table column; no fallback for undefined values |
| src/app/api/donorOffers/route.ts | 2/5 | Integrated description generation into POST endpoint; missing validation and error handling for critical operations |
| src/services/fileService.ts | 2/5 | Refactored to Map-based column remapping with hardcoded defaults; validation runs before empty row filtering |
| src/services/donorOfferService.ts | 2/5 | Changed from createMany to individual upsert operations; unsafe type casting and performance issues |
Confidence score: 2/5
- This PR contains critical issues in the backend that could cause runtime errors and data integrity problems in production.
- Score reflects three major concerns: (1) unsafe type casting of potentially undefined
expirationDatevalues in upsert operations will cause crashes, (2) missing validation and error handling when mapping descriptions to items could lead to index mismatches, (3) validation logic in file parsing runs before empty row filtering, allowing invalid files to fail prematurely. Additionally, the OpenAI SDK version jump spans 41 minor versions without documented compatibility testing. - Pay close attention to
src/app/api/donorOffers/route.ts,src/services/donorOfferService.ts, andsrc/services/fileService.ts- all three contain logic errors that will cause failures.
Issues Requiring Attention
Critical Issues:
-
src/services/donorOfferService.ts (Line 472): The
expirationDatefield is cast asDatewhen looking up the unique index, but the schema allows it to beundefined. This will cause runtime errors when attempting upsert operations on items without expiration dates. -
src/app/api/donorOffers/route.ts (Lines 38-49): No validation that
descriptionsarray length matchesparsedFileData.datalength before the forEach loop. If description generation fails partially or returns fewer elements,descriptions[index]will be undefined, causing data corruption. Add length validation and error handling for description generation failures. -
src/services/fileService.ts (Lines 163, 177, 183): Validation error checking occurs at lines 163/177 (via
containsRequiredKeys) before empty row filtering at line 183. This means files with valid columns but empty rows will fail validation before cleanup runs. The validation should occur after filtering. -
src/app/donorOffers/create/page.tsx (Line 52): Debug
console.log(result)statement left in production code. Remove before merging. -
src/services/fileService.ts: The
parseFinalizeFilemethod doesn't include the new Map-based remapping logic, creating inconsistency between create and finalize workflows. Both should use the same column mapping approach.
Code Quality Issues:
-
src/services/donorOfferService.ts (Lines 491-494): Commented-out code should be removed:
/* await tx.generalItem.createMany({ ... }) */ -
src/services/donorOfferService.ts (Line 481): The spread operator
...itemmay include fields not in the Prisma schema. Explicitly map only valid schema fields in thecreateblock. -
src/components/DonorOffers/PreviewTable.tsx (Line 73):
item.descriptionhas no fallback for undefined values. Consider adding|| "N/A"or|| "Generating..."to handle missing descriptions gracefully.
PR Checklist Score: 62/100
Areas Needing Improvement:
General:
- Console.logs or commented out code: Remove
console.log(result)at line 52 insrc/app/donorOffers/create/page.tsxand commented-outcreateManycode at lines 491-494 insrc/services/donorOfferService.ts.
Backend:
- Handles edge cases where necessary: Missing edge case handling for undefined
expirationDatevalues in upsert operations (line 472 ofdonorOfferService.ts); missing validation that descriptions array matches items array length (lines38-49 ofroute.ts). - Uses try/catch blocks for error handling: No error handling around description generation in
/api/donorOffersroute; ifDescriptionService.getOrGenerateDescriptionsthrows, the entire donor offer creation fails without cleanup. - Input is appropriately validated: File parsing validation runs before empty row filtering, causing premature failures; no validation of array lengths before mapping descriptions to items.
- Operations are optimized to avoid unnecessary computations: Changed from
createMany(single DB operation) to loop of individualupsertoperations (N operations), significantly degrading performance for large uploads. - Schemas are strict: The spread operator at line 481 in
donorOfferService.tsmay inject fields not in the schema; use explicit field mapping instead.
Sequence Diagram
sequenceDiagram
participant User
participant CreateDonorOfferPage
participant FileUpload
participant API as /api/donorOffers
participant FileService
participant DescriptionService
participant OpenAI as Azure OpenAI
participant DB as Database
participant DonorOfferService
User->>CreateDonorOfferPage: "Fill form (offer name, dates, partners)"
User->>CreateDonorOfferPage: "Upload CSV/XLSX file"
CreateDonorOfferPage->>FileUpload: "handleFileChange(file)"
FileUpload->>API: "POST /api/donorOffers?preview=true"
API->>FileService: "parseDonorOfferFile(file)"
FileService->>FileService: "Parse CSV/XLSX, extract item data"
FileService-->>API: "parsedFileData (items without descriptions)"
API->>API: "Build itemDescriptionInput from parsed data"
API->>DescriptionService: "getOrGenerateDescriptions(itemDescriptionInput)"
DescriptionService->>DB: "Query DescriptionLookup table for cached descriptions"
DB-->>DescriptionService: "Return existing descriptions"
alt "Some descriptions missing from cache"
DescriptionService->>DescriptionService: "Identify missing items"
DescriptionService->>OpenAI: "Generate bilingual descriptions (Haitian/English)"
OpenAI-->>DescriptionService: "Return generated descriptions"
DescriptionService->>DB: "Upsert new descriptions to cache"
end
DescriptionService-->>API: "Array of descriptions"
API->>API: "Attach descriptions to parsedFileData items"
API->>DonorOfferService: "createDonorOffer(formData, parsedFileData, preview=true)"
DonorOfferService-->>API: "Preview result with first 8 items"
API-->>CreateDonorOfferPage: "Success with donorOfferItems including descriptions"
CreateDonorOfferPage->>PreviewTable: "Render preview with descriptions"
PreviewTable-->>User: "Display table with Description column"
User->>CreateDonorOfferPage: "Click Submit Offer"
CreateDonorOfferPage->>API: "POST /api/donorOffers (final submission)"
API->>FileService: "parseDonorOfferFile(file)"
API->>DescriptionService: "getOrGenerateDescriptions(itemDescriptionInput)"
DescriptionService-->>API: "Descriptions (from cache or newly generated)"
API->>API: "Attach descriptions to items"
API->>DonorOfferService: "createDonorOffer(formData, parsedFileData, preview=false)"
DonorOfferService->>DB: "Create DonorOffer record"
DonorOfferService->>DB: "Upsert GeneralItem records with descriptions"
DB-->>DonorOfferService: "Success"
DonorOfferService-->>API: "Success result"
API-->>CreateDonorOfferPage: "Success response"
CreateDonorOfferPage->>CreateDonorOfferPage: "Show success modal"
10 files reviewed, 10 comments
| previewEndpoint: "/api/donorOffers/create?preview=true", | ||
| previewEndpoint: "/api/donorOffers?preview=true", | ||
| onSuccess: (result) => { | ||
| console.log(result); |
There was a problem hiding this comment.
style: Remove debug console.log before merging
| item.visible ? "Visible" : "Hidden", | ||
| item.notes, | ||
| ]), | ||
| item.description, |
There was a problem hiding this comment.
style: description can be undefined - should display fallback like 'N/A' or 'Generating...' instead of blank cell
| const itemDescriptionInput = parsedFileData.data.map((item) => ({ | ||
| title: item["title"] as string, | ||
| type: item["type"] as string, | ||
| unitType: item["unitType"] as string, | ||
| })); |
There was a problem hiding this comment.
logic: Missing validation for required fields. If title, type, or unitType are undefined/null in parsedFileData.data, this will pass invalid data to DescriptionService
| const descriptions = | ||
| await DescriptionService.getOrGenerateDescriptions(itemDescriptionInput); |
There was a problem hiding this comment.
logic: No error handling if getOrGenerateDescriptions fails or returns fewer descriptions than expected. This could lead to undefined values being assigned on line 48
| parsedFileData.data.forEach((item, index) => { | ||
| item.description = descriptions[index]; | ||
| }); |
There was a problem hiding this comment.
logic: If descriptions.length !== parsedFileData.data.length, array index mismatch will assign wrong descriptions or undefined to items
| jsonData = jsonData.filter((row) => { | ||
| return ( | ||
| row["Generic Description"] !== undefined && | ||
| row["Generic Description"] !== null && | ||
| row["Generic Description"] !== "" | ||
| ); //try to filter out invalid rows based on required field | ||
| }); |
There was a problem hiding this comment.
logic: filtering happens after validation check, so invalid files with missing required keys will throw error before filtering empty rows. Move filter before containsRequiredKeys check on line 163
|
|
||
| if (!meta.fields || !containsRequiredKeys(meta.fields)) { | ||
| throw new ArgumentError("CSV does not contain required keys"); | ||
| throw new ArgumentError("XLSX does not contain required keys"); |
There was a problem hiding this comment.
syntax: error message says 'XLSX' but this branch handles xlsx files correctly - probably copy-paste error from line177 CSV branch
| donorOfferId: item.donorOfferId, | ||
| title: item.title, | ||
| type: item.type, | ||
| expirationDate: item.expirationDate as Date, |
There was a problem hiding this comment.
logic: expirationDate can be undefined according to schema (line 44-45), but cast as Date will cause runtime error when undefined. Check for undefined before using in unique index
| for (const item of itemsWithDonorOfferId) { | ||
| await tx.generalItem.upsert({ | ||
| where: { | ||
| donorOfferId_title_type_expirationDate_unitType_quantityPerUnit: { | ||
| donorOfferId: item.donorOfferId, | ||
| title: item.title, | ||
| type: item.type, | ||
| expirationDate: item.expirationDate as Date, | ||
| unitType: item.unitType, | ||
| quantityPerUnit: item.quantityPerUnit, | ||
| }, | ||
| }, | ||
| update: { | ||
| initialQuantity: { increment: item.initialQuantity }, | ||
| }, | ||
| create: { | ||
| ...item, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| title: true, | ||
| type: true, | ||
| unitType: true, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
style: looping with individual upsert operations inside transaction is significantly slower than createMany. For bulk operations with hundreds of items, this could cause transaction timeouts. What is the expected maximum number of items in a single donor offer upload?
| /* | ||
| await tx.generalItem.createMany({ | ||
| data: itemsWithDonorOfferId, | ||
| }); | ||
| });*/ |
There was a problem hiding this comment.
style: remove commented-out code before merging
| quantityPerUnit: item.quantityPerUnit, | ||
| }, | ||
| }, | ||
| update: { |
There was a problem hiding this comment.
update to aggregate quantities if row w/ data already exists
| "unitType", | ||
| "quantityPerUnit", | ||
| ]; | ||
| const requiredKeys = new Map<string, string>([ |
There was a problem hiding this comment.
mapping xlsx column names to schema fields
2 participants
Description
Resolves #181
Checklist