Repository navigation
Only remove the old export staging directory on iOS - #103623
Conversation
On Android, filesDir/Expensify is where the native share intent handler stages files shared into the app, so the cleanup was deleting them.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@carlosmiceli 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed5d979879
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(mockRNFS.unlink).toHaveBeenCalledWith(OLD_EXPORT_STAGING_DIR); | ||
| }); | ||
|
|
||
| it('keeps the Android share staging directory, which uses the same path as the old iOS export staging directory', async () => { |
There was a problem hiding this comment.
Document the new test's Given/When/Then phases
This newly added test omits the required literal // Given, // When, and // Then comments explaining why its setup, action, and assertion are necessary. Add those three sections so the test complies with the repository's documented testing convention.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
|
@NicolasBonet jest test fialing 🙇 |
…rt-staging-cleanup
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.5.8-0 🚀
|
|
No help site update needed. This is a bug fix inside a startup migration: on Android it stops deleting files that are shared into the app, so sharing now works the way you'd expect. No article sentence is now wrong, and nothing new is visible to users. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.8-5 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Follow-up to #96531 (review comment).
A month after shipping, the
MoveFilesOutOfDocumentsmigration still logsremoved the old export staging directoryabout 100-190 times a day (~1,100 times for ~690 users in the last 8 days), with no downward trend. About 95% of those come from Android, often several times a day for the same user on the same app version.The cleanup removes
${RNFS.DocumentDirectoryPath}/Expensify. On iOS that is the oldDocuments/Expensifyexport staging folder, as intended. On Android,DocumentDirectoryPathis the app's internalfiles/directory, andfiles/Expensifyis where the native share intent handler stages files shared into the app (android/app/src/main/java/com/expensify/chat/utils/FileUtils.kt). So on every launch after a share, the migration deleted the staged file, including on a cold-start share where it can run before the share screen reads it. This is one of the causes discussed in #101425.This change makes the export staging cleanup iOS-only. The other cleanups (attachment cache move, attachment record paths, stale Onyx dump) are unchanged on both platforms.
Fixed Issues
$ #96531 (comment)
PROPOSAL: N/A
Tests
files/Expensifyis not deleted on launch)MoveFilesOutOfDocuments removed the old export staging directoryDocuments/Expensify(e.g. via the Files app or by installing an old build and cancelling a CSV export share sheet), relaunch, and verify the folder is removed and the log line appears onceAndroid emulator results (HybridApp dev build, cold-start share from Google Photos while signed out)
files/Expensify/share-test.pngMoveFilesOutOfDocuments removed the old export staging directoryloggedshareActionHandlerprefs still point at itBefore:
After:
(
FileUtils: Internal storage directory removedis the native share handler clearing previous shares before staging the new file, which is expected.)Offline tests
N/A, this only runs once at startup and does not depend on the network.
QA Steps
Same as tests (1-3) on Android.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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
Cold-start share of a receipt image from Google Photos into the app (signed in):
Before (main): the startup migration deletes the staged image, so the share falls back to text and only the Share tab is offered. The message is the internal file path.
before-android-share.mp4
After (this PR): the staged image survives startup, the Submit tab is offered and the receipt shows on the confirmation page.
after-android-share.mp4