Repository navigation
Give downloaded attachments a file extension - #101711
Conversation
Recover a missing extension from the MIME type at upload, and pass the stored file name into the video player's download action instead of falling back to the extensionless storage key. Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Web test steps verification results: ❌ fail Blocked before reaching the feature under test: the session is stuck on the onboarding "work email" screen and every mutating command (click/press) fails with
Blocker detail1. Onboarding could not be cleared. The CI prestart handed over a signed-in but onboarding-incomplete session. Its log records the same failure the tester then hit live: Read-only commands ( 2. These steps are out of scope for web regardless. Android coverage for the same steps is in the earlier Android results comment — 5 of 6 passed there, with step 4 failing. |
|
I'll take a look at this PR today, @MelvinBot merge main & resolve conflict |
…loadFileExtension Co-authored-by: Cong Pham <suneox@users.noreply.github.com> # Conflicts: # tests/unit/FileUtilsTest.ts
|
I merged Conflict: only one file had a conflict, Checks: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
When a video is played inline in a wide report view, this new fileName prop stops at VideoPlayerPreview: its VideoPlayer render does not receive the existing fileName prop. The overflow Download action therefore still passes undefined through MoreMenuTrigger and falls back to the extensionless storage key. Pass fileName to that VideoPlayer as well so inline-player downloads get the same corrected name as attachment-modal downloads.
ℹ️ 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".
|
@MelvinBot Two issues before we can move forward:
|
…l name Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
1. Fixed. 2. Proposal: detect the real type from the file's first bytes. The
Why not reuse the existing magic-byte check
Not verified yet: what the iOS document picker returns as |
|
No fix needed. All code checks pass on the latest commit ( The only failing check is |
|
@MelvinBot apply fix |
…t-stream attachments Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
Applied the proposal in d0dc5d8. An extensionless file reported as
One deviation: HEIC and AVIF brands ( Tests
|
|
@MelvinBot merge main |
…loadFileExtension Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
@MelvinBot verify pipeline failed & merge main. |
…loadFileExtension Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
…tection Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
I merged
|
|
I made both fixes and pushed them in a23340c:
I added a unit test for each change. The 103 tests in |
|
@MelvinBot merge main and verify typecheck |
…loadFileExtension Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
I merged Typecheck fails, but not because of this PR. It reports one error, and that error is also on
Cause and fixMatch the self DM by the localized You/Me in report search added a test that calls Two open PRs fix it:
After one of them merges, ask me to merge |
|
@suneox I think the broken checks on main were resolved just after you merged main. Could you try again? |
|
@MelvinBot please merge main |
|
I didn't merge To have me merge |
|
@MelvinBot Merge main |
…loadFileExtension Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
@Julesssss All comments and pipeline issues have been resolved |
|
🚧 Julesssss 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/Julesssss in version: 9.5.8-0 🚀
|
|
No help site update needed. This is a bug fix: video downloads and extensionless uploads now keep a proper file extension, which is what users already expect. The existing article (Chat Features and Moderation Tools) already says Expensify adds the original extension back when you download, so no sentence became incorrect. |
|
@suneox For step 4 the filenames are not the same. This is reproducible on Android, iOS and Web and the same in Prod. Not sure we need to create an issue for this, let me know 1791891669324.2-ios.mp41791891923936.3-AND.mp4
|
|
@MelvinBot Update the test steps so QA isn't asked to verify the pre-existing behavior:
and remove step 5,6 |
|
@izarutskaya Thanks for checking! For step 4 the key expectation is that the downloaded file is playable, the extension is preserved and the file opens in the OS video player, I've requested an update to the test steps. |
|
Updated the Tests section. Step 4 now reads "Repeat with the message context menu download, verify the file is named after the uploaded file and opens in the OS video player", and steps 5 and 6 are removed. QA Steps still say "Same as tests", so QA picks up the change. |
@Julesssss following up on this thread: the downloaded video file name shows the storage hash ( I'd like to separate it into another issue: this PR's scope is already large, and the naming issue is outside it. Could you create another issue and assign me? cc: @izarutskaya |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.5.8-5 🚀
Bundle Size Analysis (Sentry): |






Explanation of Change
Some attachments were saved with no file extension, so the OS treated a valid video as a generic document. This PR fixes both the download side and the upload side.
AttachmentView(and the inlineVideoPlayerPreview) down tofileDownload.VideoRendereralso prefers the storeddata-name, matchingImageRenderer.validateAttachmentFilenow adds an extension from the MIME type (newgetExtensionFromMimeType/appendExtensionFromMimeTypehelpers inFileUtils). This covers every upload flow: picker, drag and drop, paste, and share. If the type can't be resolved, the name is left unchanged.application/octet-stream(or an empty type),getMimeTypeFromFileHeaderreads the first 32 bytes to identify the type. Web usesBlob.slice, Android usesRNFS.read, and iOS copies the header into a temp file withreact-native-blob-util, becauseRNFS.readreturns an empty string on iOS.mime-dbis now a direct dependency, pinned to the version already in the lockfile. Existing attachments are not backfilled.Fixed Issues
$ #100579
PROPOSAL: #100579 (comment)
Tests
Precondition: Have a video (for example, a screen recording) whose file name has no extension, such as
recordinginstead ofrecording.mp4.<name>.mp4and opens in the OS video player.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
iOS: mWeb Safari
MacOS: Chrome / Safari