Fix receipts dropping after iOS app upgrade due to stale container paths - #97814
Fix receipts dropping after iOS app upgrade due to stale container paths#97814adhorodyski wants to merge 12 commits into
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@MelvinBot review |
|
🤖 Review — solid, well-designed refactor. The core idea is right: stop persisting the absolute path, store only the bare filename, and re-root it onto the folder's current location at the moment of use. That both fixes the stale-iOS-container drop and recovers receipts already sitting in the offline queue. Test coverage is good ( One thing worth deciding before merge: Odometer gallery import silently drops the image when Minor / verified-fine
Testing note: this fix is iOS-native (the whole point is the rotating iOS data container), so it can't be exercised on web — it needs iOS native QA: capture/queue a receipt offline, upgrade/relaunch so the container UUID rotates, then confirm the queued expense uploads with the receipt instead of a $0 no-receipt expense. As a web regression check I did confirm the Create expense → Scan/upload screen (which now goes through the modified shared Create expense Scan tab renders correctly on webAlso note the PR's |
…ay read RNFS.moveFile falls back to copy-then-delete when the rename crosses filesystems, which react-native-blob-util's mv does not — on Android the receipts folder sits on external storage while picked and cropped files sit in the app cache, so the move there is cross-device. Gallery pick and crop/rotate now fall back to the original file when adopt fails, matching main, instead of silently losing the image. Camera capture keeps the loud failure since the user can retake. Receipt previews re-root the stored path at read through getThumbnailAndImageURIs, so a receipt queued before an upgrade renders correctly, not just uploads correctly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # src/pages/media/AttachmentModalScreen/routes/TransactionReceiptModalContent.tsx
The resolve() call sites, the second copy of the vision-camera note, and the catch blocks whose Log.alert already says "using original URI" all carried comments that repeat what the line below them does. The type and module docblocks were essay-length for a three-method module. What stays is what you cannot read off the code: why RNFS.moveFile rather than blob-util's mv, why adopt has a no-move branch, why cleanFileName matters here, and what a numeric receipt source means. The module docblock was a floating /** */ that JSDoc would bind to verify(), and it quoted react-native-blob-util docs for a file that no longer imports it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolve() took `unknown`, so a caller could pass the wrong field and get undefined with no type error. Every call site already passes receipt.source, so ReceiptSource | undefined typechecks with no casts. prepareRequestPayload now builds the FormData uri from resolve(source) rather than the receipt's own uri field, and no test covered that. A revert to the old field passes every other test in the file and uploads the stale path. The assertion runs through validateFormDataParameter because the FormData polyfill stringifies the entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The receipt sets uri to the stale path two lines above the assertion that expects the current one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolve() sat at three call sites, so every reader of a stored receipt path had to remember to call it. useRestartOnOdometerImagesFailure did not, and it deletes a good odometer draft when the check fails. The three hooks and validators all reach the filesystem through checkIfLocalFileIsAccessible or validateReceiptFile, so resolving there covers them, covers the odometer hook without touching it, and covers the next caller nobody remembers to patch. navigateToStartStepIfScanFileCannotBeRead reads the same field through readFileAsync and gets the same treatment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getTransactionWithPreservedLocalReceiptSource copies receipt.source into receipt.localSource for a queued scan expense, and ReportActionItemImage prefers localSource for both the thumbnail and the full image. That read bypassed getThumbnailAndImageURIs, so the preview of a queued expense still pointed at the container the upgrade left behind while the upload itself recovered. resolve() now takes null so localSource passes straight in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
adopt asked whether a file was already in the folder with startsWith(dir), while toDurableName asks by folder name because RNFS and blob-util disagree about the container prefix (/private/var and /var). adopt is exposed to exactly that disagreement: dir comes from blob-util and the camera path comes from vision-camera. Using the one matcher also lets a stale stored path verify in place instead of moving a file from a directory the device no longer has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bundled placeholder is a require() asset id, which is a number on native. validateReceiptFile already checks for exactly that with typeof. The regex was here because the global File type declares source as string, so widening the cast to Receipt['source'] lets the typed guard compile and drops the String() around the resolved uri. The fixture used '686' as a string, which native never produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four of these repeated the call right under them: resolve() already names what it does. What survives is the part a reader cannot get from the code — vision-camera writing into the folder itself, and a require() asset id being a number. The stale-container reasoning lives on toDurableName, which is the function that acts on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@ikevin127 @mountiny One of you needs to 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] |
![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: a16afb32e1
ℹ️ 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".
|
🚧 mountiny 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! 🧪🧪
|
|
|
||
| captureReceipt(camera.current, {flash, hasFlash, isPlatformMuted, path, isInLandscapeMode}) | ||
| .then((photo: PhotoFile) => { | ||
| .then((photo: PhotoFile) => ReceiptStorage.adopt(photo.path)) |
There was a problem hiding this comment.
I suppose now we have a bit different behaviour here
Adopt() rejects, while the old moveReceiptToDurableStorage fell back to the original URI. Here, a rejection lands in the .catch below and shows the camera error alert, so the photo is lost
Should this get a .catch falling back to photo.path?
There was a problem hiding this comment.
adopt runs no move here: vision-camera already wrote into the receipts folder, so a rejection means photo.path names a file that is gone.
| cropImageToAspectRatio(imageObject, viewfinderLayout.current?.width, viewfinderLayout.current?.height, undefined, photo.orientation) | ||
| .then(({file, filename, source}) => moveReceiptToDurableStorage(source, filename).then((durableSource) => ({file, filename, source: durableSource}))) | ||
| .then(({file, filename, source}) => | ||
| ReceiptStorage.adopt(source, filename).then((durableName) => ({file, filename, source: ReceiptStorage.toLocalUri(durableName)})), |
There was a problem hiding this comment.
The user stands at the camera here, so the alert lets them retake; gallery pick and crop/rotate cannot retake, and both already fall back.
An iOS receipt captured on an earlier build is stored without a file:// scheme, because getPhotoSource returns the bare path there. resolve() sat inside the branch that only a blob: or file: string can reach, so those receipts fell through to the remote branch and the preview kept pointing at the container the upgrade left behind, while the upload recovered. Resolving where path is computed covers every reader of getThumbnailAndImageURIs and leaves the branch condition alone, so a cache path that never belonged to the receipts folder is treated as it was before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
4 participants
Explanation of Change
On iOS the app stores the full path to a receipt:
Most app upgrades change the
<UUID>. iOS moves the file into the new container. The stored path still names the old container. When a queued expense uploads, the file check fails and the app sends the expense with no receipt. The user gets a $0 expense and no error. The file was on disk the whole time.This PR stops storing the path. The new
ReceiptStoragemodule owns theReceipts-Uploadfolder.adoptmoves a receipt into the folder at capture and returns the bare filename.resolvejoins that filename to the folder's current location, at the moment of upload or display. The number of upgrades in between does not matter, so receipts already in the offline queue recover too.adoptnow rejects when a file does not land in the folder. Capture fails at once, where the user can retake the photo, instead of storing a path that loses the receipt hours later.Fixed Issues
$ #97979
PROPOSAL:
Tests
iOS: recover a queued receipt across an app upgrade (the core fix)
npm run iosagain) — iOS rotates the app's data container path on install-over, which is exactly what an App Store upgrade doesGallery pick (iOS and Android native)
Crop / rotate
Odometer (native)
Distance placeholder regression
Web regression
Offline tests
The core scenario above runs offline by design. Additionally:
QA Steps
iOS upgrade recovery (the core fix)
Regression sweep
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