Repository navigation
Stop exposing app data to the iOS Files app - #96531
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
|
🚧 NicolasBonet has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![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: 959f786962
ℹ️ 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".
|
This PR adds a new |
|
This is WIP, will re-open when it's in a testable status |
|
Heads up on a user-facing tradeoff in this PR that could use a product opinion — cc @Expensify/Product: This PR hides the app's entire Documents folder from the iOS Files app, that includes any PDFs/documents users previously downloaded through the old flow and saved in the visible "New Expensify" folder. Those files are not migrated or surfaced anywhere — after this update they still exist on disk but become unreachable to the user. The new download flow hands files off through the share sheet ("Save to Files"), so anything downloaded going forward is unaffected. The question is only about previously downloaded files. Options:
Is option 1 acceptable, or should we handle existing downloads more gracefully? If 1, we should note it in the PR body/QA steps as expected behavior. |
|
Woof, I don't want people to get angry thinking we've lost their curated list of downloads and force them to trawl through chats, reports, threads etc to re-download them. What would it take to move them somewhere user accessible before hiding the folder? |
|
I agree with Tom. I think it's dangerous to not give them someplace obvious to find them, at least for now. This is the number one thing that our users are counting on us for, keeping their digitized receipt data. |
|
Sounds good, let me work in a hybrid proposal and try to keep the folder while keeping onyx and similar data out of sight. |
a71ba62 to
af14bb0
Compare
Disable UIFileSharingEnabled and LSSupportsOpeningDocumentsInPlace and move
internal files out of the user-visible Documents directory:
- OnyxDB moves to Library/Application Support via a react-native-nitro-sqlite
patch that also migrates existing database files
- The attachment cache moves to Library/Caches
- Queued receipt uploads move to Library/Application Support
- Non-media downloads now go through the share sheet ("Save to Files")
instead of being written to Documents, with a startup migration cleaning
up files left behind by older versions
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The global react-native-fs mock lacked the functions and directory constants the MoveFilesOutOfDocuments migration uses, which crashed app-boot test suites, and AttachmentTest still expected the attachment cache under the documents directory. The migration also guards against synchronous throws so a file-system error can never block startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚧 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! 🧪🧪
|
|
Hi @NicolasBonet. QA team can't check the console on the Native app. Could you please share QA steps that don't require console checking? If that's not possible, could you please check this PR internally? |
|
🚀 Deployed to staging by https://github.com/NicolasBonet in version: 9.4.60-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site content under Why: This PR is an internal iOS storage fix. It relocates internal/implementation files (the Onyx database, attachment cache, and debug/export staging files) out of the iOS Expensify's help articles document how to use features (e.g. downloading/exporting expenses and reports, attaching and editing receipts), not the internal on-device file paths or which iOS folder a file is stored in. I checked the relevant candidates — export/download articles like Search-and-Download-Expenses, How-to-Export-Reports, and Attach-and-edit-receipts-on-expenses — and none of them describe the behavior this PR touches. The user-visible outcome (downloads and receipts remain accessible in the Files app) is unchanged. Since there is no documentable behavior change, I did not create a draft help site PR (so there is no linked PR to review). If you'd still like an article that explains where downloaded files land on iOS — and that internal app data is no longer exposed to the Files app — let me know and I'll draft one. @NicolasBonet, if you agree there's nothing to document here, no further action is needed. If you'd like a help site PR anyway, reply and I'll create one for you to review. |
You can ignore the console and do the rest of tests, specially for iOS. |
|
@NicolasBonet, could you please share clear QA steps for the team? |
What is wrong with the testing section that we have in the PR description? It's indeed a lot, but I have detailed there all the possible scenarios. |
|
iOS was passed already by the team |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.60-2 🚀
Bundle Size Analysis (Sentry): |
|
@MelvinBot, could you please create a payment issue for me? I was the reviewer of this PR. |
| // exists. If it was purged, fall back to the current source and re-cache it. | ||
| const localFileExists = await RNFS.exists(localSource); | ||
| if (localFileExists) { | ||
| return localSource; |
| * later launches skip it entirely. | ||
| */ | ||
| function updateAttachmentRecordPaths(): Promise<void> { | ||
| return OnyxUtils.get(ONYXKEYS.ATTACHMENT_RECORD_PATHS_MIGRATED).then((hasMigrated) => { |
There was a problem hiding this comment.
Sorry I didn't catch this in the review but OnyxUtils.get() is forbidden. We're in the middle of adding an ESLint rule to prevent it, but that hasn't been merged yet. Would you please do a followup PR to replace this with Onyx.connectWithoutView()?
There was a problem hiding this comment.
mmm this is quite sensitive, are there any ongoing issues with get? This change alone would mean re-running all the 19 test scenarios we described in this PR and this migration will get deleted in a month, so not sure if it's worth all the trouble unless we have something broken with its usage.
There was a problem hiding this comment.
I have a draft open: https://github.com/Expensify/App/pull/100531/changes, but I would like to see if we can ignore this one and just get it removed when the time is due.
Explanation of Change
The iOS app enables
UIFileSharingEnabledandLSSupportsOpeningDocumentsInPlace, which publishes the entireDocuments/directory to the iOS Files app. That exposed the Onyx database, attachment caches, and receipt images to the user (and any app with Files access), all of it user-deletable — deleting the OnyxDB breaks the app.The
Documents/folder is also where downloaded PDFs/documents land, and the Files-app exposure is how users reach them (added in [Mobile-Expensify#2380728e0a]). Hiding the folder would make previously downloaded files unreachable (discussion), so this PR keeps the folder visible and instead moves every internal file out ofDocuments/, leaving it to hold only files the user expects to see: their downloads and queued receipt uploads.src/libs/fileDownload/index.ios.ts): non-media downloads keep saving toDocuments/. Temporary files move out of it: video downloads stage inLibrary/Cachesbefore being saved to Photos, and the CSV-export flow writes its share-sheet hand-off file toLibrary/Cachesinstead ofDocuments/Expensify.Library/Application Supportvia areact-native-nitro-sqlitepatch ([patches/react-native-nitro-sqlite+9.6.0.patch]) that also migrates existingOnyxDB*files out ofDocuments/at startup. The DB path is hardcoded toNSDocumentDirectoryin the library's native+load, so this cannot be fixed at the app level; the change should be offered upstream so the patch can eventually be dropped.Library/Caches(re-downloadable, so purgeable is fine).src/libs/ExportOnyxState/index.native.ts) and temporary share files (src/libs/localFileCreate/index.native.ts) →Library/Caches.src/libs/migrations/MoveFilesOutOfDocuments) moves the old attachment cache intoLibrary/Cachesand repoints the Onyx attachment records at it (offline-composed attachments have no remote source to re-download from), and removes a stale Onyx state dump and the oldDocuments/Expensifyexport staging folder if older versions left them behind. The same internal files lived under the same relative paths on Android, so the migration runs there too. Queued receipt uploads stay inDocuments/on purpose — they are user content, so they remain visible in the Files app.Companion Mobile-Expensify PR (moves OldDot app data out of
Documents/while leaving user downloads in place):MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14023
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/643611
PROPOSAL:
Tests
All file-visibility checks below refer to the iOS Files app under On My iPhone → New Expensify.
A. Fresh install (no migration)
OnyxDB*files, noattachmentsfolder, and no other internal files.B. Downloads (must stay user-visible)
Expensifyfolder is left behind in the Files app folder.2024.pdf). Relaunch the app and verify the file is still visible in the Files app folder (the OldDot migration must not sweep numeric-named user downloads).C. Receipt uploads (user content, must stay visible)
Receipts-Upload. Kill and reopen the app, go online, and verify the receipt uploads.D. Attachment cache
attachmentsfolder appears in the Files app folder.E. Internal/debug files
F. Upgrade migration (NewDot)
mainand sign in. Open image attachments in a chat (populates the attachment cache), download a PDF attachment, and export Onyx state (the old build leaves the dump in Documents). Install this build over it and verify:OnyxDB*including-wal/-shmmigrated to Application Support).OnyxDB*files, theattachmentsfolder, or the Onyx state dump.G. Upgrade migration (OldDot / HybridApp)
mainHybridApp build, use Expensify Classic (OldDot): sign in and capture a receipt in Classic. Upgrade to this build, open Classic again, and verify:Application Support/ExpensifyData)./Documents/paths are re-rooted onto the new data directory).*.jsonfiles,gpsLocations.txt,logPacketsQueue,<UUID>.jpgreceipts, or hash-named image caches (e.g.93244983.jpg) — while user downloads remain in place.ExpensifyData).H. Resilience (the original bug)
Android sanity
On Android, download a PDF and an image, open attachments, and scan a receipt. Verify behavior is unchanged (all path changes are iOS-scoped).
Verify that no errors appear in the JS console throughout.
Offline tests
QA Steps
Same as tests
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
ScreenRecording_07-21-2026.15-23-31_1.MP4
iOS: mWeb Safari