Conversation
|
|
|
@rayane-d I already got assigned to the issue, let me review this PR |
|
BUG: "Choose recipient" RHP still display when closing the error modal Screen.Recording.2025-07-22.at.15.22.52.mov |
|
BUG: The PDF file displays at first, although I select it in the third position Screen.Recording.2025-07-22.at.15.27.25.mov |
@DylanDylann It won't work this way - look at how the files are dragged - the pdf you selected as last is first in the dragged batch. I have no means to control the order of selection from the folder to drag. That's why I wrote in the test steps - in the order they were dragged onto the dropzone (I had no better idea how to explain that) |
|
BUG:
--> It seems inconsistent. And in the second time, there are no file larger than 24MB Screen.Recording.2025-07-22.at.15.39.53.mov |
Ah oke, got it |
|
@DylanDylann the selection order is preserved when you select from the Add Attachment picker |
|
BUG: In the video below, the Continue button doesn't display in the second error modal. As I understand, we always display the Continue button if there are still next error modals or if there is any valid file. We only don't display the Continue button if this is the last error modal and there are no valid files Screen.Recording.2025-07-22.at.15.47.59.mov |
|
@koko57 Could we disable the folder in the picker? It means that users can't drag a folder to our dropzone |
the picker open the folders to choose a file from the folder. You cannot restrict dragging a folder onto the dropzone. Why would you want that? |
|
@DylanDylann but what part do you mean - you can drag anything on the dropzone - I can't restrict that because it's dragging files from the system UI (finder, file explorer, whatever you call it). I don't have an access there. The user can select anything and drag anything. For the pickers - it's how the pickers work and I also cannot do anything about that - you can open a folder not select it. If I understood you correctly you mean this behavior of selecting the files to drag vs selecting the files from picker - I cannot make them work the same bc it's how the system works. Dragging on the dropzone works the same way for attachments and receipts. Same with pickers - should work the same for both attachments and receipts. But I can't make selecting files for drop and selecting files from the picker work the same |
@DylanDylann I'm having a problem with reproducing this one, could you please share a bit more details of the files you used? |
|
Sent via Slack |
|
@DylanDylann thank you! the bug is now fixed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-07-23.at.16.48.17.movAndroid: mWeb ChromeScreen.Recording.2025-07-23.at.16.50.08.moviOS: HybridAppMy IOS crashed when opening the file selection and I still try to fix. It isn;t related to our PR iOS: mWeb SafariScreen.Recording.2025-07-23.at.16.40.24.movMacOS: Chrome / SafariScreen.Recording.2025-07-23.at.16.09.45.movMacOS: DesktopScreen.Recording.2025-07-23.at.16.08.09.mov |
| <> | ||
| <Modal | ||
| type={modalType} | ||
| type={CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE} |
There was a problem hiding this comment.
@koko57 Why do you change this line?
I see a comment
If our attachment is a PDF, return the unswipeable Modal type.
There was a problem hiding this comment.
because modal type was chosen right after validation. As we removed validation it will always be CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE (as a default state value) so we don't need to have a state for that as it won't be changed anymore
src/hooks/useFilesValidation.tsx
Outdated
| setIsLoaderVisible(true); | ||
|
|
||
| return Promise.all(otherFiles.map((file) => convertHeicImageToJpegPromise(file))).then((convertedImages) => { | ||
| // Update originalFileOrder map with converted files |
There was a problem hiding this comment.
| // Update originalFileOrder map with converted files |
There was a problem hiding this comment.
@koko57 Could you remove some redundant comment?
src/hooks/useFilesValidation.tsx
Outdated
| collectedErrors.current = []; | ||
|
|
||
| Promise.all(files.map((file) => isValidFile(file, files.length > 1).then((isValid) => (isValid ? file : null)))) | ||
| // Store original file order using URI as key |
There was a problem hiding this comment.
| // Store original file order using URI as key |
src/hooks/useFilesValidation.tsx
Outdated
| // Check if we need to resize images | ||
| if (convertedImages.some((file) => (file.size ?? 0) > CONST.API_ATTACHMENT_VALIDATIONS.RECEIPT_MAX_SIZE)) { | ||
| return Promise.all(convertedImages.map((file) => resizeImageIfNeeded(file))).then((processedFiles) => { | ||
| // Update originalFileOrder map with resized files |
There was a problem hiding this comment.
| // Update originalFileOrder map with resized files |
src/hooks/useFilesValidation.tsx
Outdated
| if (otherFiles.some((file) => (file.size ?? 0) > CONST.API_ATTACHMENT_VALIDATIONS.RECEIPT_MAX_SIZE)) { | ||
| setIsLoaderVisible(true); | ||
| return Promise.all(otherFiles.map((file) => resizeImageIfNeeded(file))).then((processedFiles) => { | ||
| // Update originalFileOrder map with resized files |
There was a problem hiding this comment.
| // Update originalFileOrder map with resized files |
|
@koko57 Some minor comments. The rest looks fine to me |
|
@koko57 There is one more problem, both attachment and receipt modal are displayed when trying to drag many type of files (Only happen on Desktop App) Screen.Recording.2025-07-22.at.17.54.36.mov |
|
@DylanDylann does it happen consistently? What OS are you on? I cannot repro |
|
@koko57 Yes, It happens consistently on desktop App. I am using macOS 15.5 |
|
hmmm, I was suspecting Windows. Running the same OS, cannot recreate it. Any more details why it might happen? Can you compare it with the production version? I guess if the problem was persistent it would be already reported. The code doesn't touch the dropzones behavior on drop, only the validation, so it shouldn't be the problem |
|
@koko57 Let me investigate on my side |
|
@DylanDylann ok, thanks! |
|
I will give an update tomorrow |
|
Hmmm, I tried many times but can't reproduce the above bug anymore. It's so weird. Anyways, I will continue processing this PR. We may handle it later if it happens again |
|
@DylanDylann great, thanks! yeah, we'll handle it later if it still occurs or someone reports it |
|
@koko57 Kindly bump: #66812 (comment) |
|
@DylanDylann done! |
|
checkspell failing but not because of this PR |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.1.86-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.86-3 🚀
|

Explanation of Change
This PR covers follow-ups and cleanup from #65722
The main improvement is that the pdf files are no longer displayed as last - the order of the files selected is preserved.
🚨 VERY IMPORTANT NOTE 🚨
For iOS (native app/ mobile web app) files are mixed up after selecting them from the Files folder. So the files won't be in the order they were originally marked as selected. It is a native iOS problem - you can check WhatsApp for reference. It usually happens when selecting more than 2 files. It didn't happen for me for just 2 files.
So for the file ordering on iOS (native / mobile safari):
Before changes:
Expected result after changes:
🚨 VERY IMPORTANT NOTE 2 🚨
Another known issue #66794 - will be handled separately.
When selecting mixed images/pdfs on iOS the "Corrupted file" error is thrown ocasionally (it's not 100% times reproducible)
As the validation hook required substantial modifications, I would greatly appreciate a detailed and in-depth review. Both adding attachments and IOU/receipts flow are affected, so I included all the test steps from the previous Stage 3 and Stage 5 PRs.
Fixed Issues
$ #65722
PROPOSAL: -
Tests
[WEB / DESKTOP]
[MOBILE]
[CHECK THE ERROR HANDLING]
RECEIPTS
[WEB / DESKTOP]
Adding a new file
Starting the flow from the Chat
Error handling
[MOBILE]
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-07-21.at.19.24.17.mp4
Android: mWeb Chrome
Screen.Recording.2025-07-21.at.16.40.20.mp4
iOS: Native
Screen.Recording.2025-07-21.at.13.05.00.mp4
Screen.Recording.2025-07-21.at.12.50.00.mp4
iOS: mWeb Safari
Screen.Recording.2025-07-21.at.13.12.53.mp4
Screen.Recording.2025-07-21.at.13.11.24.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-07-21.at.11.37.45.mp4
Screen.Recording.2025-07-21.at.11.36.19.mp4
MacOS: Desktop
Screen.Recording.2025-07-21.at.12.06.26.mp4
Screen.Recording.2025-07-21.at.12.05.48.mp4