Fix Share button being unresponsive when sharing an image to a new email#94905
Conversation
…s on ShareRootPage Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Core fix LGTM. Please: (1) keep |
…, normalize native stat path Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@MelvinBot Still needed on this PR:
|
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@MelvinBot Please scope this PR back to the approved proposal for #93657 — Keep:
Revert / remove (out of scope):
Update PR description to match — only the two bullets above. |
…temPath/checkFileExists/getFileSize.native changes Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot merge main |
|
@MelvinBot Please investigate ErrorModal / validation handling on What I saw in QA (without the size fix):
How other pages handle it:
Possible fixes to investigate (if you agree):
Core #93657 fix ( |
Validation-consistency analysis for
|
|
@MelvinBot implement #1 and #2 and also merge main |
|
@MelvinBot implement #1 and #2 and also merge main |
|
@MelvinBot merge main |
|
@MelvinBot merge main branch |
|
🤖 Merged the latest
The branch is up to date and still shows as mergeable. Note: |
|
@MelvinBot implement |
…+ no duplicate corruption check) Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🤖 Implemented #1 and #2 from your comment — pushed to #1 — Validate as an attachment, not a receipt #2 — Drop the duplicate corruption check Scope note
Checks run locally:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1496b4757e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| getFileSize(file.content).then((size) => { | ||
| validateFiles( |
There was a problem hiding this comment.
When a user shares a HEIC/image/* file after a previous successful share, VALIDATED_FILE_OBJECT still contains the old file because clearShareData() does not clear it. This new validation starts only after the async getFileSize() call, while the page marks the file ready immediately afterwards; if the user selects a recipient before validateFiles() overwrites Onyx, ShareDetailsPage sees a non-null validatedFile and uploads the previous image. Clear VALIDATED_FILE_OBJECT at the start of the share or keep the UI loading until validation finishes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this is a real bug, now fixed in 357ac04.
clearShareData() only reset SHARE_TEMP_FILE and SHARE_UNKNOWN_USER_DETAILS, never VALIDATED_FILE_OBJECT. Meanwhile ShareDetailsPage uses the pre-validated file as its source (ShareDetailsPage.tsx:80) and handleShare only guards on it being non-null (ShareDetailsPage.tsx:120). Since setIsFileReady(true) fires synchronously while the new validation is still awaiting getFileSize() (ShareRootPage.tsx:139-140), a HEIC/image/* share after a previous successful one could pass the guard against the stale file and upload the previous image.
Fix: added [ONYXKEYS.VALIDATED_FILE_OBJECT]: null to the clearShareData() multiSet. It's called once on ShareRootPage mount at the start of the flow, so the stale file is cleared before the new one is validated. If the user now taps Share before validation finishes, handleShare just no-ops (guard returns early) instead of uploading the wrong file.
Went with option 1 (clear on start) over keeping the UI in a loading state, since it's the minimal change and directly removes the data-correctness risk. typecheck-tsgo and Prettier pass locally; ESLint couldn't complete in this environment (OOM on the full-project type-aware run), so relying on CI lint for that gate.
…revious share's file Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@Krishna2323 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] |
|
✅ Marked this PR ready for review. On "add tests" and ticking the checklist — I've not ticked the manual-testing checklist items or filled the To make it quick, here are ready-to-paste steps that cover this fix: Tests / QA Steps
Offline steps
Unit test: I can add one if you'd like — reply Once you've run the steps and added the recordings, please tick the author checklist items yourself so they accurately reflect what was verified. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppweb_chrome.copy.mp4Android: mWeb ChromeiOS: HybridAppios_hybrid.copy.mp4iOS: mWeb SafariMacOS: Chrome / Safari |
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM! @blimpich, I think you'll need to handle the author checklist. 📋
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| type: file.mimeType, | ||
| }, | ||
| ]); | ||
| getFileSize(file.content).then((size) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
getFileSize(file.content) resolves from RNFS.stat, which rejects when the file is missing or the path is malformed (exactly the image/*/HEIC edge cases this PR targets). The promise chain has no .catch(), so on rejection the .then() never runs, validateFiles() is never called, VALIDATED_FILE_OBJECT is never written, and the Share button silently becomes a no-op again — the same silent failure this PR is fixing. This is a critical data-mutation path with no error handling and no feedback to the user or logs. (An earlier commit in this PR added a .catch() here and it was reverted.)
Add a .catch() that logs and still proceeds to validation (so a genuine error surfaces via ErrorModal instead of failing silently):
getFileSize(file.content)
.catch((error: unknown) => {
Log.warn('[ShareRootPage] Failed to get file size for validation', {error});
return undefined;
})
.then((size) => {
validateFiles(
[{name: file.id, uri: file.content, type: file.mimeType, size}],
undefined,
{isValidatingReceipts: false},
);
});Reviewed at: 357ac04 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@MelvinBot can you please address this comment? #94905 (comment)
There was a problem hiding this comment.
Done — addressed in 1cc914c.
Confirmed the concern is valid: on native, getFileSize resolves from RNFS.stat(uri), which rejects for a missing or malformed path — the exact image/*/HEIC cases this PR targets. Without a .catch(), that rejection skips .then(), validateFiles() never runs, and the Share button silently no-ops again.
Added a .catch() that logs and resolves to undefined so the chain still proceeds to validation — a genuine failure now surfaces via ErrorModal instead of failing silently (ShareRootPage.tsx:54-71):
getFileSize(file.content)
.catch((error: unknown) => {
Log.warn('[ShareRootPage] Failed to get file size for validation', {error});
return undefined;
})
.then((size) => {
validateFiles(
[{name: file.id, uri: file.content, type: file.mimeType, size}],
undefined,
{isValidatingReceipts: false},
);
});typecheck-tsgo and Prettier pass. ESLint couldn't complete locally (the sandbox's 2 GB Node heap OOMs on the full project config) — CI lint will cover it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 357ac04602
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| ], | ||
| undefined, | ||
| {isValidatingReceipts: false}, |
There was a problem hiding this comment.
For shared files whose MIME type is also scannable (notably Android image/* and HEIC), the Submit tab is shown, but forcing isValidatingReceipts: false validates them with the generic 24 MB attachment limit instead of the receipt 10 MB limit/resize path. SubmitDetailsPage then seeds and uploads the receipt from the shared file source, so a 10–24 MB image receipt that used to be rejected or resized can now reach the submit flow and fail server-side or create an oversized receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot I'm certain this is existing behavior, not introduced by this PR — can you investigate and confirm? Submit uses the raw shared file + 24 MB client check, ShareRootPage receipt pre-validation never succeeded on main for image/*/HEIC anyway.
…alidateFileSize Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
@Krishna2323 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] |
|
🤖 Merged the latest Note: the branch was originally cloned shallowly, which caused an initial |
Confirmed — you're right, no receipt/Submit-path regressionI traced both claims against the code. Both hold, and the Claim: "receipt pre-validation never succeeded on Claim: "Submit uses the raw shared file + 24 MB client check" — ✅ Verified (with one precision note). The uploaded receipt is the raw shared file, and the ~24 MB check is enforced on the Submit path independently of the pre-validated object:
The decisive fact: Two precision notes (neither is a regression from this PR)
Bottom line: confirmed existing behavior — flipping to |
…ently no-oping Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🚧 blimpich has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.4.27-1 🚀
|
|
🤖 I reviewed the changes in this PR against the help site files under Why:
Because no documentation update is needed, I did not create a draft help site PR. @Krishna2323, please confirm you agree no help site changes are needed here. If you believe an article should be updated (or created) for this behavior, let me know and I'll draft one. |
|
Hi @Krishna2323 @blimpich Do we need QA tests here, can I check it off if no ? Thanks |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.27-2 🚀
Bundle Size Analysis (Sentry): |
@izarutskaya, sorry for the late reply. This does require QA testing. @blimpich, I'm not sure how the checks passed without the author checklist. Either way, I think we need to test this to make sure the issue is actually fixed. It would also be great if it could be tested by the same tester using the same device model mentioned in the OP: #93657 (comment) |
|
Will request a retest 👍 |
5 participants
Explanation of Change
When a file whose mimetype requires pre-validation (
image/heic, or the genericimage/*that Android share sheets like the Samsung A31 Gallery emit) is shared, the Share Details page rendered a blank preview and the Share button became a no-op, so the chat was never created.The root cause is that
ShareRootPage.validateFileIfNecessarybuilt the validation request without asizefield.validateAttachmentFilerejects any file withsize == nullasFILE_INVALIDon its first guard, so the validated file object (VALIDATED_FILE_OBJECT) was never written. With no validated file,ShareDetailsPageshowed an empty preview andhandleShareearly-returned. The failure was also silent becauseShareRootPagenever rendereduseFilesValidation'sErrorModal.This PR implements Option 2 from the approved proposal, scoped to
ShareRootPage.tsxonly:ShareRootPage.validateFileIfNecessary, populate the file'ssizeviagetFileSize(file.content)before callingvalidateFiles(), sovalidateAttachmentFileno longer rejects it asFILE_INVALID.ErrorModalreturned byuseFilesValidationonShareRootPage, so any genuine validation failures (e.g. too large/too small) surface to the user instead of failing silently.Fixed Issues
$ #93657
PROPOSAL: #93657 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Offline tests
// TODO: The human co-author must fill out the offline tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari