Conversation
|
@MelvinBot create a GH issue for this PR and assign me to it. Update the PR description to link to this issue. |
|
Done — created #100707 and assigned Issue: Single folder attachment shows the generic corrupted-file error instead of the folder-specific error I also updated this PR description:
I searched both |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-09-10.at.17.04.55.mov |
|
🟡 The receipt drop flow still shows the wrong error for a single folder, so the fix is only half the story
if (isValidatingReceipts && !isValidReceiptExtension(file)) {
return {isValid: false, error: CONST.FILE_VALIDATION_ERRORS.WRONG_FILE_TYPE};
}
// ...
if (isValidatingReceipts && file.size < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
return {isValid: false, error: CONST.FILE_VALIDATION_ERRORS.FILE_TOO_SMALL};
}
// ...
if (isDataTransferItemDirectory(item)) { // never reached for receipts
return {isValid: false, error: CONST.FILE_VALIDATION_ERRORS.FOLDER_NOT_ALLOWED};
}A dropped folder has no extension, so
Bug in the app: user drags a folder onto the receipt zone, gets told the file type is unsupported, and reasonably tries renaming or converting the folder instead of learning that folders are simply not accepted. Same confusion the linked issue describes, just in a different entry point. Suggested fix, hoist the directory check above the receipt-specific checks: async function validateAttachmentFile(file: FileObject, item?: DataTransferItem, isValidatingReceipts = false): Promise<ValidateAttachmentResult> {
if (!file.name || file.size == null) {
return {isValid: false, error: CONST.FILE_VALIDATION_ERRORS.FILE_INVALID};
}
// A folder is never a valid attachment, so classify it before any extension or size checks.
// Otherwise receipt validation rejects it as WRONG_FILE_TYPE (no extension) or FILE_TOO_SMALL (size 0).
if (isDataTransferItemDirectory(item)) {
return {isValid: false, error: CONST.FILE_VALIDATION_ERRORS.FOLDER_NOT_ALLOWED};
}
if (isValidatingReceipts && !isValidReceiptExtension(file)) {
// ...Note this needs @chrispader If you'd rather keep this PR scoped to the issue, that's fine, but please call it out in the description or spin off a follow-up issue, because the PR title reads like all single-folder entry points are covered and they aren't. |
|
🟢 Optional: the multi-item folder error keeps the singular title The other multi-file errors use |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - Small change, tests well - only 3 non-blocking comments.
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #100707 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
chrispader
changed the title
|
@ikevin127 I addressed all three comments in
I also broadened the PR title, description, and manual test steps to cover the receipt and mixed-selection paths. The PR is marked WIP until the expanded flow is retested. Answered by Codex, instructed by Chris. |
chrispader
changed the title
|
🚧 mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.4.77-0 🚀
|
|
🤖 No help site changes are required for this PR — I did not create a docs PR. This PR only corrects which in-app error title and reason are shown when a folder is dropped. It does not change what users can do, any setting, tab, label, or button, and it does not change the folder restriction itself — folders were already rejected before this PR. How I checkedWhat changed: What I searched in
Because no published article states the old behavior, there is nothing to correct. Adding a new FAQ about folder uploads would document a pre-existing restriction rather than this change, so it is outside the scope of a docs update for this PR.
view run · no recording available |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.77-4 🚀
Bundle Size Analysis (Sentry): |
5 participants
Folder drops can surface misleading errors depending on where and how the user drops them. A single folder in the attachment zone falls back to corrupted-file copy, the receipt zone reports an unsupported file type, and mixed selections use a title that implies every file failed. This PR makes folder classification and error titles consistent across those paths.
Reproduction
Explanation of Change
This originated while re-evaluating the unfinished attachment-validation work in #85751. Most of that work was later covered by #86189, which corrected receipt attachment validation, #93234, which made multi-file validation sequential and cleaned up object URLs, and #92236, which expanded
useFilesValidationcoverage. Related fixes in #96024 and #96940 also addressed HEIC conversion failures and file snapshot reliability.Comparing those changes with the current implementation exposed three remaining gaps.
getFileValidationErrorText()fell back to the corrupted-file copy for a single folder, receipt validation checked the extension and size before checking whether the item was a directory, and a folder in a mixed selection used the singular attachment-error title.Folder detection now runs before receipt-specific extension and size validation. The error mapper returns the folder-specific reason for any folder and uses Some files can't be uploaded for a mixed selection. The multi-file limit case also documents why it intentionally has no single-file counterpart. Unit tests cover single and mixed folder errors plus receipt folders with and without a valid-looking extension.
Fixed Issues
$ #100707
PROPOSAL: N/A
Tests
Offline tests
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Screenshots are provided by C+ review already