Skip to content

ezra/325-negating-duplicate-offer-error - #332

Merged
ND68 merged 2 commits into
mainfrom
ezra/325-duplicate-finalized-offer-error-messages
Apr 11, 2026
Merged

ND68 merged 2 commits into
mainfrom
ezra/325-duplicate-finalized-offer-error-messages

Conversation

@ezraexe

@ezraexe ezraexe commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Description

Resolves ticket number: #325

Explain what your code changes: There are two sources where errors are stored within the create/page.tsx and finalize/page.tsx. It is ultimately stored within ErrorDisplay and fileErrorMessage. Because we want to keep the error display(the error at the top), just negated the fileErrorMessage.

On creation:
image

On finalization:
List the steps you took to test your code: Manually uploading invalid csv files on both creation and finalization
image

Also changed the "Sign Offs" to "Completed Sign Offs" in a following commit

Checklist

  • The ticket is mentioned above
  • The changes fulfill the success criteria of the ticket
  • The changes were self-reviewed
  • A review was requested

@ezraexe
ezraexe requested a review from ND68 April 11, 2026 19:46
@ezraexe ezraexe linked an issue Apr 11, 2026 that may be closed by this pull request
@netlify

netlify Bot commented Apr 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for hope-for-haiti ready!

Name Link
🔨 Latest commit 20b6655
🔍 Latest deploy log https://app.netlify.com/projects/hope-for-haiti/deploys/69daa5aa2c88d10008d1beae
😎 Deploy Preview https://deploy-preview-332--hope-for-haiti.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@greptile-apps

greptile-apps Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR suppresses the duplicate error display that occurred when a file upload failed — previously both FileInfoDisplay (inline, in the upload row) and ErrorDisplay (above, card-style) rendered the same message simultaneously. The fix adds !fileErrorMessage to the ErrorDisplay render condition in both create/page.tsx and finalize/page.tsx.

  • The deduplication itself is correct, but when the API returns multiple errors useFileUpload stores the full list in errors[] and only the first entry in fileErrorMessage. Suppressing ErrorDisplay when fileErrorMessage is set therefore silently drops all errors after the first — users uploading a CSV with several row-level validation errors will only ever see one message at a time.

Confidence Score: 4/5

Fix correctly removes duplicate error display but introduces a regression that silently hides multi-error API responses — should be addressed before merging.

Two P1 findings: the !fileErrorMessage guard suppresses ErrorDisplay even when the API returns additional errors beyond the first, meaning users will miss validation details. The duplicate-display problem is solved, but the fix trades one UX bug for another. Once the multi-error case is handled, this is safe to merge.

Both changed files carry the same issue; the root cause lives in src/hooks/useFileUpload.ts (sets fileErrorMessage to only errorMessages[0]).

Important Files Changed

Filename Overview
src/app/donorOffers/create/page.tsx Adds !fileErrorMessage guard to suppress duplicate ErrorDisplay, but hides multi-error API responses — only the first error remains visible to the user.
src/app/donorOffers/[donorOfferId]/finalize/page.tsx Identical one-line fix as create/page.tsx; same multi-error truncation regression applies.

Sequence Diagram

sequenceDiagram
    participant User
    participant Page as Page (create / finalize)
    participant Hook as useFileUpload
    participant API as Preview API

    User->>Page: selects CSV file
    Page->>Hook: handleFileChange(e)
    Hook->>API: POST/PATCH (previewEndpoint)
    alt API returns errors (e.g. multiple CSV row errors)
        API-->>Hook: { errors: ["err1", "err2", "err3"] }
        Hook->>Hook: setErrors(["err1","err2","err3"])
        Hook->>Hook: setFileErrorMessage("err1")
        Hook-->>Page: errors=[...], fileErrorMessage="err1"
        Note over Page: FileInfoDisplay shows "err1" ✅
        Note over Page: !fileErrorMessage=false → ErrorDisplay hidden ❌ err2, err3 lost
    else API succeeds
        API-->>Hook: { donorOfferItems: [...] }
        Hook->>Hook: setFileUploaded(true)
        Hook-->>Page: errors=[], fileErrorMessage=null
        Note over Page: No error displayed ✅
    else validateBeforeUpload fails (create only)
        Hook->>Hook: setErrors(["Fill required fields..."])
        Hook-->>Page: errors=[...], fileErrorMessage=null
        Note over Page: ErrorDisplay shows message ✅
    end
Loading

Reviews (1): Last reviewed commit: "negating out fileErrorMessage" | Re-trigger Greptile

/>

{errors && errors.length > 0 && <ErrorDisplay errors={errors} />}
{errors && errors.length > 0 && !fileErrorMessage && <ErrorDisplay errors={errors} />}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Multi-error responses silently truncated

When the API returns multiple validation errors (e.g. several invalid CSV rows), useFileUpload stores them all in errors[] but only puts the first message in fileErrorMessage. Because !fileErrorMessage now suppresses ErrorDisplay, errors 2-N are never shown — FileInfoDisplay only renders fileErrorMessage, which is errorMessages[0].

In useFileUpload.ts lines 85-86:

setErrors(errorMessages);        // full list
setFileErrorMessage(errorMessages[0]); // first entry only

A user uploading a CSV with multiple row-level errors would only ever see the first error and would have to fix and re-upload iteratively rather than fixing everything at once.

Consider showing ErrorDisplay when there are more errors than fileErrorMessage covers:

Suggested change
{errors && errors.length > 0 && !fileErrorMessage && <ErrorDisplay errors={errors} />}
{errors && errors.length > 0 && (!fileErrorMessage || errors.length > 1) && <ErrorDisplay errors={fileErrorMessage ? errors.slice(1) : errors} />}

Or alternatively, set fileErrorMessage to a generic summary (e.g. "File contains errors — see details below") so the detailed list is always rendered via ErrorDisplay.

/>

{errors && errors.length > 0 && <ErrorDisplay errors={errors} />}
{errors && errors.length > 0 && !fileErrorMessage && <ErrorDisplay errors={errors} />}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Same multi-error truncation issue as create/page.tsx

The same logic applies here: when the finalize API returns several row-level CSV errors, fileErrorMessage only captures the first, so suppressing ErrorDisplay with !fileErrorMessage hides all subsequent errors from the user. See the comment on create/page.tsx for the full analysis and a suggested remediation.

@ND68

ND68 commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

Ignore greptile, multi level errors still function properly. Nice work

@ND68
ND68 merged commit a9f4e8d into main Apr 11, 2026
5 checks passed
@ND68
ND68 deleted the ezra/325-duplicate-finalized-offer-error-messages branch April 11, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate Finalized Offer Error Messages

2 participants