Snapshot picked files into memory-backed Files to prevent request queue poisoning - #96940
Conversation
…ning A File picked from disk only references its OS file. If that file is modified or deleted before the queued request is persisted (e.g. while the preview modal is open), the IndexedDB write fails with "DataError: Failed to write blobs" (InvalidBlob/IOError) and the dead File poisons the persisted request queue, failing every later rewrite. Copy the bytes into a memory-backed File at validation time, so the queued request can never reference a stale OS file. If the file is already unreadable at validation, surface FILE_INVALID to the user instead of failing silently later. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
eslint-seatbelt caps no-unsafe-type-assertion at the grandfathered count for this file; use Object.defineProperty instead of casting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the arrayBuffer feature-detect with snapshotPickedFile/index.ts (web: copy bytes into a memory-backed File) and index.native.ts (keep the original rename-only behavior; native has no IndexedDB blob path). Snapshot tests move to their own file that pins the web implementation, since Jest resolves the .native variant by default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
5974a7f to
04ed14c
Compare
|
@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: 04ed14ced5
ℹ️ 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".
Mobile browsers hand over sandboxed temp copies the OS won't modify after picking, so the queue-poisoning scenario doesn't apply there — and copying every file's bytes would multiply peak memory by batch size on memory-constrained mobile Safari. Keep the lazy File on mobile and only clean the name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@nyomanjyotisa 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: af128f9f72
ℹ️ 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".
iPadOS Safari defaults to "Request Desktop Website", reporting a Macintosh user agent that isMobile() can't recognize, so a memory-constrained iPad would copy every picked file's bytes. Detect it via Macintosh UA plus touch points (real Macs report zero) and keep the lazy File there too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents web request-queue persistence failures (“Failed to write blobs”) caused by OS-backed File objects becoming unreadable after a user picks them but before the first IndexedDB write, by snapshotting picked files into memory-backed Files during attachment validation.
Changes:
- Snapshot desktop-web picked files into a memory-backed
FileduringvalidateAttachmentFile()so later disk changes can’t poisonnetworkRequestQueue. - Add a platform-split
snapshotPickedFilehelper (web copies bytes; native preserves existing rename-only behavior). - Add unit tests covering snapshot success, unreadable-file failure, and mobile/iPadOS desktop-mode exceptions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/libs/validateAttachmentFile.ts |
Uses snapshotPickedFile() during validation to ensure queued requests serialize reliably on web. |
src/libs/snapshotPickedFile/index.ts |
Web implementation: copies bytes into memory-backed File, with mobile/iPadOS desktop-mode opt-outs. |
src/libs/snapshotPickedFile/index.native.ts |
Native implementation: rename-only behavior (no byte snapshotting). |
tests/unit/ValidateAttachmentFileSnapshotTest.ts |
Adds tests for the new snapshotting behavior and error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,28 @@ | |||
| import {isMobile} from '@libs/Browser'; | |||
nyomanjyotisa
left a comment
nyomanjyotisa
left a comment
There was a problem hiding this comment.
I agree with this review comment. updatedFile is guaranteed to be a fresh File only on desktop web, while the mobile and iPad paths preserve the existing behavior. The implementation looks correct. Could you please update the comment accordingly?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid-pr96940.mp4Android: mWeb ChromeAndroid-mWeb-pr96940.mp4iOS: HybridAppios-pr96940.mp4iOS: mWeb Safariios-mWeb-pr96940.mp4MacOS: Chrome / SafariChrome-pr96940.mp4 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sure, done |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.46-0 🚀
|
|
🤖 No help site changes required — I did not create a draft PR. I reviewed the changes in this PR against the help articles under Why no docs update is needed: This is an internal engineering bug fix, not a product/behavior change that the help site documents:
Since nothing in the documented user experience changed, there's no accurate help-site content to add or revise here. @elirangoshen — since no help-site changes are required, I didn't open a docs PR. If you believe a specific article should still be updated, let me know which one and what behavior you'd like documented, and I'll draft it. |
Explanation of Change
On web, a
Filepicked from disk (file picker, drag-and-drop) is only a lazy reference to its OS file. If the user modifies, deletes, renames, or moves that file after picking it but before the queued request is first persisted (for example while the attachment preview or the expense confirmation screen is open — whether by user action, a download still completing, or background software touching the file), the first IndexedDB write of the persisted request queue fails with:The dead
Filethen poisonsnetworkRequestQueue: the storage write is retried 5×, and every subsequent queue rewrite re-serializes the same dead blob and fails again. Production logs show ~46k of these events over 3 days acrossAddAttachment,ReplaceReceipt, andRequestMoney(SmartScan) — all failing on the very first persist (retryAttempt: 0/5), which pins the failure to the pick → first-write window.This PR snapshots the picked file's bytes into a memory-backed
FileinvalidateAttachmentFile(the single choke point all attachment/receipt flows go through viauseFilesValidation). A memory-backedFilecannot be invalidated by later disk changes, so the queued request always serializes. If the backing file is already unreadable at validation time, the user gets an immediate "file invalid" error instead of a silent failure later.The snapshot lives in a platform-split module (
src/libs/snapshotPickedFile/index.ts+index.native.ts): the web variant copies the bytes, the native variant keeps the existing rename-only behavior — native file objects never enter the IndexedDB blob path, and production logs show this error class exclusively from web user agents.Related follow-up (independent, no dependency): Expensify/react-native-onyx#814 classifies this error as never-retriable at the storage layer, so any remaining inline File/Blob path fails once instead of retrying 5x. It ships separately via #96957.
Fixed Issues
$ #97050
PROPOSAL:
Tests
+→ Add attachment and pick the file. Keep the preview modal open.Failed to write blobs/[Onyx] Failed to save to storageerrors.npx jest tests/unit/ValidateAttachmentFileTest.tsand verify all tests pass.Offline tests
Failed to write blobserrors appear in the console.QA Steps
Same as Tests (steps 1–7).
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
before
before.mov
after
after.mov