fix: keep shared receipts out of the folder the iOS share extension wipes - #98974
fix: keep shared receipts out of the folder the iOS share extension wipes#98974adhorodyski wants to merge 3 commits into
Conversation
A receipt shared into the app stays in the app-group `sharedFiles` folder until it uploads. The iOS share extension clears that whole folder on every new share, so sharing a second receipt deletes the first one while it is still queued, and the request goes out with no receipt: a $0 expense and no error. Shared receipts drop at ~2% against a 0.6% iOS baseline, and they are the only drops that do not cross an app upgrade. Adopt the shared file into the receipts folder at submit, the way every other capture path already does, so the wipe cannot reach a pending receipt. A failed move falls back to the shared path, because turning a 2% silent drop into a 100% blocked submit is the worse trade. The extension also wrote those files with `.completeFileProtection`, which makes them unreadable while the device is locked. `RNFS.moveFile` is a rename and preserves the protection class, so the adopt alone does not fix it. Drop the option here; the same one-line change ships separately in Mobile-Expensify's `SmartScanExtension`, which is the copy HybridApp actually builds. `checkFileExists` turned every stat rejection into a plain `false`, so a locked device and a deleted file were indistinguishable. Log the error code on the `[Receipt]` spine so the residual drops can be attributed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
|
@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: 88bb3a6d46
ℹ️ 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".
The adopt is a move, so the app-group path is empty once it lands. performUpload kept reading the source off the draft, which still named the shared path, so a retry after a failed read adopted a file that no longer existed, fell back to the same dead path, and could never recover. Write the durable uri back to the draft, the way every other capture path already hands the durable uri downstream. The preview and the size check read the same field, so they stop pointing at the file the move took away too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep only the lines naming a constraint the code cannot show: the extension wiping its folder, and the move emptying the shared path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@MelvinBot review this PR |
ReviewLooks solid — ship it. The change correctly fixes two independent iOS receipt‑drop mechanisms (the share‑extension folder wipe, and I traced the submit path and it holds up:
A few non‑blocking notes: Details
None of these block merge. Nice, tightly‑scoped fix with good test coverage. |
|
@ZhenjaHorbach 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] |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28eca70098
ℹ️ 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".
| return false; | ||
| }; | ||
|
|
||
| return statIsFile(decodedPath).catch((error: unknown) => (decodedPath === rawPath ? statFailed(error) : statIsFile(rawPath).catch(statFailed))); |
There was a problem hiding this comment.
When a normal percent-encoded URI is checked while its actual decoded file is locked, the decoded RNFS.stat can fail with EPERM, but this branch discards that error and probes the literal %20/%23 path next; that nonexistent fallback then reports ENOENT, which is the only code sent to receipt telemetry. This makes locked files look deleted in precisely the encoded-path scenario the new telemetry is intended to diagnose, so preserve the decoded-path error or report both failures when the fallback also fails.
Useful? React with 👍 / 👎.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
2 participants
Explanation of Change
Two things destroy a shared receipt before its expense uploads: the share extension wipes its folder on every new share, and it writes files with
.completeFileProtection, which makes them unreadable while the device is locked. Either way the request goes out without the receipt and the user gets a $0 expense with no error.Shared receipts drop at 2.05% against a 0.59% iOS baseline (Aug 8 to Aug 18, 101 of 4,938).
SubmitDetailsPagemoves the file intoDocuments/Receipts-Uploadbefore reading it, through the sameReceiptStorage.adoptthe camera and gallery paths use, and points the draft receipt at the moved copy.ShareViewControllerdrops the protection option.checkFileExistslogs thestatcode, so a locked device stops looking like a deleted file.The protection fix that reaches iOS users is https://github.com/Expensify/Mobile-Expensify/pull/14063, because HybridApp builds
Mobile-Expensify/iOS/SmartScanExtension. The copy here only keeps the two from diverging.Fixed Issues
$ #98980
PROPOSAL: N/A
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14063
Tests
iOS native, HybridApp build.
Share A, then share B before A uploads
Locked device
Share regression sweep
Other capture paths
Automated
tests/ui/SubmitDetailsPageTest.tsxcovers three cases of the share submit: the adopted uri reachesreadFileAsync, a failed adopt logs and still submits with the shared path, and a retry after a failed read adopts and reads the durable copy rather than the emptied shared path.tests/unit/checkFileExistsTest.tscovers the locked-device case, where anEPERMstill reports missing but the code reaches telemetry.Offline tests
The share A then share B case above runs offline by design. Also:
QA Steps
The steps above, on the staging HybridApp build, on a physical iPhone with a passcode set.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari