Fix share extension handling for .HEIC files#68348
Conversation
|
@war-in Please check out this code when you have a moment. |
# Conflicts: # src/pages/Share/ShareDetailsPage.tsx # src/pages/Share/SubmitDetailsPage.tsx
|
The PR is open and ready for review. |
sumo-slonik
changed the title
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreenrecorder-2025-08-14-21-46-41-627.mp4shareAndroidDocxAndMp4.movScreenrecorder-2025-08-14-21-52-04-741.mp4Android: mWeb ChromeiOS: HybridAppshareiOSRemaining.movshareiOSUntilSecondFileShare.movsubmitiOS.mp4iOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
|
||
| const fileUri = shouldUsePreValidatedFile ? (validFilesToUpload?.uri ?? '') : (currentAttachment?.content ?? ''); | ||
| const fileName = shouldUsePreValidatedFile ? getFileName(validFilesToUpload?.uri ?? 'shared_image.png') : getFileName(currentAttachment?.content ?? ''); | ||
| const fileType = shouldUsePreValidatedFile ? (validFilesToUpload?.type ?? 'image/jpeg') : (currentAttachment?.mimeType ?? ''); |
There was a problem hiding this comment.
| const fileType = shouldUsePreValidatedFile ? (validFilesToUpload?.type ?? 'image/jpeg') : (currentAttachment?.mimeType ?? ''); | |
| const fileType = shouldUsePreValidatedFile ? (validFilesToUpload?.type ?? CONST.SHARE_FILE_MIMETYPE.JPEG) : (currentAttachment?.mimeType ?? ''); |
| const shouldGenerateTransactionThreadReport = !isBetaEnabled(CONST.BETAS.NO_OPTIMISTIC_TRANSACTION_THREADS); | ||
|
|
||
| const fileUri = shouldUsePreValidatedFile ? (validFilesToUpload?.uri ?? '') : (currentAttachment?.content ?? ''); | ||
| const fileName = shouldUsePreValidatedFile ? getFileName(validFilesToUpload?.uri ?? 'shared_image.png') : getFileName(currentAttachment?.content ?? ''); |
There was a problem hiding this comment.
Perhaps, shared_image.png can be extracted to a CONST too.
| const [currentAttachment] = useOnyx(ONYXKEYS.SHARE_TEMP_FILE, {canBeMissing: true}); | ||
|
|
||
| const {validateFiles} = useFilesValidation(addValidatedShareFile); | ||
| const isTextShared = currentAttachment?.mimeType === 'txt'; |
There was a problem hiding this comment.
| const isTextShared = currentAttachment?.mimeType === 'txt'; | |
| const isTextShared = currentAttachment?.mimeType === CONST.SHARE_FILE_MIMETYPE.TXT; |
src/pages/Share/ShareRootPage.tsx
Outdated
|
|
||
| // Currently, we use the pre-validated format for HEIC/other IMG files. If this changes, | ||
| // we need to ensure that in ShareDetailsPage and SubmitDetailsPage, | ||
| // the shouldUsePreValidatedFile condition is consistent with the one below. |
There was a problem hiding this comment.
Would it be better then to extract this check of HEIC or IMG to ReceiptUtils to avoid missing changes in those pages?
|
Share button does not work when sharing a video (or any other attachment) with the share tab to a chat. This works well on main. Screen.Recording.2025-08-14.at.12.56.33.PM.mov |
I’m already looking into it. I’m convinced it was working during my tests, but I must have broken something while last merg main. I’m on it now. |
|
@c3024 The bug you found indeed occurred, but it required the Onyx validated files to be empty. In the flow where I tested it, they were not empty, so everything worked fine on my side. It seems that everything should be fine now. Physical Android – all types of files (video, jpg, heic): 1000043215.mp4Sample file – iOS emulator: Screen.Recording.2025-08-14.at.15.07.10.mov |
c3024
left a comment
c3024
left a comment
There was a problem hiding this comment.
Suggestions for fixing the failing workflow checks.
src/libs/ReceiptUtils.ts
Outdated
| import type {Transaction} from '@src/types/onyx'; | ||
| import type {ShareTempFile, Transaction} from '@src/types/onyx'; | ||
| import type {ReceiptError, ReceiptSource} from '@src/types/onyx/Transaction'; | ||
| import * as FileUtils from './fileDownload/FileUtils'; |
There was a problem hiding this comment.
| import * as FileUtils from './fileDownload/FileUtils'; | |
| import {isLocalFileUtils, splitExtensionFromFileName} from './fileDownload/FileUtils'; |
There was a problem hiding this comment.
Lint asks to replace namespace imports with named imports for files we touch. This is required for fixing the workflow check failing.
src/libs/ReceiptUtils.ts
Outdated
| import type {ShareTempFile, Transaction} from '@src/types/onyx'; | ||
| import type {ReceiptError, ReceiptSource} from '@src/types/onyx/Transaction'; | ||
| import * as FileUtils from './fileDownload/FileUtils'; | ||
| import * as TransactionUtils from './TransactionUtils'; |
There was a problem hiding this comment.
| import * as TransactionUtils from './TransactionUtils'; | |
| import {hasReceipt, isFetchingWaypointsFromServer, hasReceiptSource} from './TransactionUtils'; |
src/libs/ReceiptUtils.ts
Outdated
| return {isThumbnail: true, fileExtension: Object.values(CONST.IOU.FILE_TYPES).find((type) => type === fileExtension), image: path, isLocalFile, filename}; | ||
| } | ||
|
|
||
| const checkNesesarityOfFileValidation = (file: ShareTempFile | undefined) => { |
There was a problem hiding this comment.
| const checkNesesarityOfFileValidation = (file: ShareTempFile | undefined) => { | |
| const shouldValidateFile = (file: ShareTempFile | undefined) => { |
There was a problem hiding this comment.
I think it should be checkNecessityOfFileValidation but shouldValidateFile sounds more on point IMHO.
|
On Android, if we share the HEIC file through Gallery, the Expensify App shows up among the apps through which it can be shared. But if we share through the File Manager, the Expensify App does not appear. This looks like Android behaviour and need not be a blocker IMO. noExpensifyFromFileManager.mp4 |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
| ATTACHMENT_THUMBNAIL_HEIGHT_ATTRIBUTE: 'data-expensify-height', | ||
| ATTACHMENT_DURATION_ATTRIBUTE: 'data-expensify-duration', | ||
|
|
||
| ATTACHMENT_IMAGE_DEFAULT_NAME: 'shared_image.png', |
There was a problem hiding this comment.
why do we need this btw? What scenario does the validatedFile?.uri not exist?
There was a problem hiding this comment.
If an error occurs during validation, or if we are not in a case where validation should be performed, this field will be empty. That’s why we need to guard against such cases.
|
|
||
| const isLocalFile = FileUtils.isLocalFile(path); | ||
| const {fileExtension} = FileUtils.splitExtensionFromFileName(filename); | ||
| const isLocalFile = isLocalFileUtils(path); |
There was a problem hiding this comment.
Why did we import this as isLocalFile as isLocalFileUtils
There was a problem hiding this comment.
due to a naming conflict with a previously existing variable with the name isLocalFile
src/pages/Share/ShareDetailsPage.tsx
Outdated
| const report: OnyxEntry<ReportType> = getReportOrDraftReport(reportOrAccountID); | ||
| const displayReport = useMemo(() => getReportDisplayOption(report, unknownUserDetails, reportAttributesDerived), [report, unknownUserDetails, reportAttributesDerived]); | ||
|
|
||
| const fileUri = shouldUsePreValidatedFile ? (validatedFile?.uri ?? '') : (currentAttachment?.content ?? ''); |
There was a problem hiding this comment.
We do this shouldUsePreValidatedFile check on the three lines below. Is there a way to consolidate for clarity?
There was a problem hiding this comment.
also what is the PreValidatedFile case?
There was a problem hiding this comment.
Not all files need to be validated, this mainly applies to images, with special attention to HEIC files. Validating all attachments led to errors, which caused the previous PR to be reverted. The shouldValidateFile check is also executed to determine whether validation should already be triggered at the src/pages/Share/ShareRootPage.tsx.
src/pages/Share/ShareDetailsPage.tsx
Outdated
| const report: OnyxEntry<ReportType> = getReportOrDraftReport(reportOrAccountID); | ||
| const displayReport = useMemo(() => getReportDisplayOption(report, unknownUserDetails, reportAttributesDerived), [report, unknownUserDetails, reportAttributesDerived]); | ||
|
|
||
| const fileUri = shouldUsePreValidatedFile ? (validatedFile?.uri ?? '') : (currentAttachment?.content ?? ''); |
There was a problem hiding this comment.
Is currentAttachment?.content a URI?
Wondering if fileSource is clearer here
There was a problem hiding this comment.
Yes, content is the path to the file, but if you think another name would make it clearer, I’ll change the variable name.
|
@grgia It seems to me that everything is ready here |
|
@sumo-slonik, could you please retest #68615 as part of this PR? Or alternatively as a follow up? |
|
As we agreed here, I think it would be best to do this as a separate follow-up PR. I'm already working on it. |
|
@grgia Can we merge this? The attachment bug has already been fixed, and the link issue will come as a follow-up, so I think this PR is ready to merge. |
|
@sumo-slonik could you please merge main for performance tests? |
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
@sumo-slonik We just need jest to pass 🤞
|
Both the previous issue with performance tests and this one with Jest tests do not come from this branch. They appeared on main and are also occurring in other PRs. The tests will be fixed in this PR: |
|
The PR is merged now. Please merge main @sumo-slonik :) |
|
@grgia Looks like we're ready here 🎆 😄 |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Product flow looks good still and agree we want to support .HEIC files for upload. 👍
(P.s this PR is linked to an issue that's closed. Why is that? CC: @Christinadobrzyn I think you need to reopen it).
|
@trjExpensify we reverted the fix to attempt to fix a blocker, but it didn't end up being the cause of the blocker #68074 (comment) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.2.1-0 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.1-20 🚀
|
This comment was marked as outdated.
This comment was marked as outdated.
| const displayReport = useMemo(() => getReportDisplayOption(report, unknownUserDetails, reportAttributesDerived), [report, unknownUserDetails, reportAttributesDerived]); | ||
|
|
||
| const fileSource = shouldUsePreValidatedFile ? (validatedFile?.uri ?? '') : (currentAttachment?.content ?? ''); | ||
| const validateFileName = shouldUsePreValidatedFile ? getFileName(validatedFile?.uri ?? CONST.ATTACHMENT_IMAGE_DEFAULT_NAME) : getFileName(currentAttachment?.content ?? ''); |
There was a problem hiding this comment.
The direct access of filename check caused this issue - #74542
8 participants
Explanation of Change
I fixed the share extension so that file transfers work for files other than images, and moved the validation logic to run before selecting the recipient. This way, the user can get the impression that the transfer works faster.
Fixed Issues
$ #68074 (comment)
PROPOSAL:
Tests
Offline tests
unnecessary
QA Steps
same as test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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-08-12.at.16.31.40.mov
iOS: Native
Screen.Recording.2025-08-12.at.16.18.56.mov