Video - Invalid file is downloaded when downloading video #37092

lanitochka17 · 2024-02-22T17:08:35Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

Version Number: 1.4.43-12
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
**Issue reported by:**Applause - Internal Team
Slack conversation:

Action Performed:

Go to staging.new.expensify.com
Go to any chat
Send a video
Right click on the video > Download

Expected Result:

The video is downloaded

Actual Result:

On web, mweb and desktop app, it downloads invalid file
On Android and iOS app, it shows error when downloading the video

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

Bug6388512_1708620575450.20240223_002702.mp4

View all open jobs on GitHub

melvin-bot · 2024-02-22T17:08:39Z

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot · 2024-02-22T17:08:44Z

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

github-actions · 2024-02-22T17:08:51Z

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

lanitochka17 · 2024-02-22T17:09:16Z

We think that this bug might be related to #vip-vsp
CC @quinthar

BhuvaneshPatil · 2024-02-22T18:37:35Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

Video - Invalid file is downloaded when downloading video

What is the root cause of that problem?

The issue with download being not working as expected for videos is that - in case of video originalFileName is null.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Lines 168 to 176 in 7f51d03

    
           const html = getActionHtml(reportAction); 
        
           const {originalFileName, sourceURL} = getAttachmentDetails(html); 
        
           const sourceURLWithAuth = addEncryptedAuthTokenToURL(sourceURL ?? ''); 
        
           const sourceID = (sourceURL?.match(CONST.REGEX.ATTACHMENT_ID) ?? [])[1]; 
        
           Download.setDownload(sourceID, true); 
        
           fileDownload(sourceURLWithAuth, originalFileName ?? '').then(() => Download.setDownload(sourceID, false)); 
        
           if (closePopover) { 
        
               hideContextMenu(true, ReportActionComposeFocusManager.focus); 
        
           }

It works in case of images cause it parses the image for html tag.

App/src/libs/fileDownload/index.ts

Line 32 in 7f51d03

    
           link.download = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(url));

Here as fileName for video is null we use getFileName() for getting name. and it doesn't have the correct file extension (.mov or .mp4 etc)

What changes do you think we should make in order to solve the problem?

We can change the code for onPress handler of download, so that if filename is null. we use reportAction?.attachmentInfo?.name and pass that to download function (fileDownload)

const fileName = originalFileName ?? (reportAction?.attachmentInfo?.name ?? '')

For popover menu in video player
we have currentlyPlayingURL in usePlaybackContext. That does have URL without auth token. We can use it to extract file name and pass it in

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.js

Lines 26 to 31 in 72bdedb

    
           const downloadAttachment = useCallback(() => { 
        
               currentVideoPlayerRef.current.getStatusAsync().then((status) => { 
        
                   const sourceURI = `/${Url.getPathFromURL(status.uri)}`; 
        
                   fileDownload(sourceURI); 
        
               }); 
        
           }, [currentVideoPlayerRef]);

const fName = currentlyPlayingURL.split('/').pop() || '';

This will ensure that we are using things that are already present without passing prop upto three levels and changing already existing things.

Result for popver

Screen.Recording.2024-02-23.at.8.00.43.AM.mov

What alternative solutions did you explore? (Optional)

Alternatively, we can apply file extension manually based on type of blob.

Result

Screen.Recording.2024-02-23.at.12.06.43.AM.mov

BhuvaneshPatil · 2024-02-22T18:39:38Z

finding offending PR

Krishna2323 · 2024-02-22T19:18:09Z

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

The originalFileName we get from getAttachmentDetails is null because we don't handle video case inside the util function to return correct originalFileName, like we do for images.

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Line 169 in 7f51d03

const {originalFileName, sourceURL} = getAttachmentDetails(html);

Also in fileDownload function we try to get a fallback fileName if it was not provided, we use getFileName, but if there is auth token is attached we will get ?encryptedAuthToken=longtoken' in url and the code below splits urls based on #?/, and ? is present for query parameter so we split and use encryptedAuthToken=longtoken for filename, which is incorrect.

App/src/libs/fileDownload/FileUtils.ts

Line 90 in 4a885f5

const fileName = url.split(/[#?/]/).pop() ?? '';

What changes do you think we should make in order to solve the problem?

We could do something similar to extractAttachmentsFromReport. We will extract fileName from the source

App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

Lines 22 to 27 in 392efc7

    
           const splittedUrl = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE].split('/'); 
        
           attachments.unshift({ 
        
               reportActionID: null, 
        
               source: tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]), 
        
               isAuthTokenRequired: Boolean(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]), 
        
               file: {name: splittedUrl[splittedUrl.length - 1]},

Demo code:

    const IS_VIDEO_TAG = /<video([\w\W]+?)<\/video>/i.test(html);

    const splittedUrl = html.match(SOURCE_REGEX)?.[1]?.split('/');
    let videoOriginalFileName = '';

    if (IS_VIDEO_TAG) {
        videoOriginalFileName = splittedUrl ? splittedUrl[splittedUrl.length - 1] : '';
    }

    const originalFileName = IS_VIDEO_TAG ? videoOriginalFileName : html.match(ORIGINAL_FILENAME_REGEX)?.[1] ?? null;

We need to do the same thing in VideoPopoverMenuContextProvider because we don't pass the fileName there also. Also we can create a util function to do similar things.

    const downloadAttachment = useCallback(() => {
        currentVideoPlayerRef.current.getStatusAsync().then((status) => {
            const sourceURI = `/${Url.getPathFromURL(status.uri)}`;

            const parts = sourceURI.split('?');
            const splittedURL = parts[0].split('/');
            const filename = splittedURL[splittedURL.length - 1];
            fileDownload(sourceURI, filename);

Result

video_download_fix.mp4

Alternative

Inside ContextMenuActions download button, we will use reportAction?.attachmentInfo?.name to get the fileName.
For VideoPopoverMenuContext download button we need to first set a state for fileName the pas the setState function through context provider.
Now we need to get the fileName inside VideoPopoverMenu to setState for fileName inside context provider. We need to pass fileName from VideoPlayerPreview > VideoPlayer > VideoPopoverMenu.
We also need to pass fileName from AttachmentView > AttachmentViewVideo > VideoPlayer.

Alternative 2

Update getFileName to remove query parameter if present so we can get the correct fileName.

function getFileName(url: string): string {
    let fileName = url;

    if (fileName.includes('?encryptedAuthToken=')) {
        fileName = fileName.split('?encryptedAuthToken=')[0];
    }
    
    fileName = fileName.split(/[#?/]/).pop() ?? '';

    if (!fileName) {
        Log.warn('[FileUtils] Could not get attachment name', {url});
    }

    return decodeURIComponent(fileName).replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_');
}

Alternative 3

If we need to use the original fileName of the video then we have 2 options.

Include data-name attribute on video tag returned from backend.

Steps to use that data returned from backend:

Get the fileName from data-name attribute in VideoRenderer and pass it to VideoPlayerPreview > BaseVideoPlayer > VideoPopoverMenu and set state inside VideoPopoverMenuContext for fileName.

In AttachmentView we already got the filename we just need to pass it to AttachmentViewVideo > BaseVideoPlayer > VideoPopoverMenu.

We need to also update extractAttachmentsFromReport to use video fileName returned from _.get(action, ['attachmentInfo', 'name'], ''), for that first we need update the html first like.

In ContextMenuAction we can simply use const fileName = originalFileName ?? (reportAction?.attachmentInfo?.name ?? '').

        const fileName = _.get(action, ['attachmentInfo', 'name'], '');
        
        const html = _.get(action, ['message', 0, 'html'], '')
            .replace('/>', `data-flagged="${hasBeenFlagged}" data-id="${action.reportActionID}"/>`)
            .replace('<video', `<video ${CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE}="${fileName}"`);
            
         const fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`);
         
         attachments.unshift({
                    reportActionID: null,
                    source,
                    isAuthTokenRequired: Boolean(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]),
                    file: {name: fileName},

Instead updating backend, we can simply use text inside <video> tags because it always uses fileName as text.
const fileName = _.get(tnode, ['domNode', 'children', 0, 'data'], urlFileName);

s77rt · 2024-02-22T21:29:09Z

@BhuvaneshPatil Thanks for the proposal. Your RCA is correct. The solution looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

Edit: We have another case that we need to cover: the download button in the playback control buttons

melvin-bot · 2024-02-22T21:29:23Z

Triggered auto assignment to @deetergp, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

s77rt · 2024-02-22T21:30:32Z

@Krishna2323 Thanks for the proposal. Your RCA is correct. As for the solution it makes sense to extract the file name but I think it's better to just use the one available in attachmentInfo field.

Krishna2323 · 2024-02-22T21:31:30Z

@s77rt, selected proposal doesn't work with the popover in VideoPlayerControls. Also the file name will be different if we download from modal and context menu. My proposal will work for each download option with consistency.

Krishna2323 · 2024-02-22T21:39:49Z

We won't get the attachmentInfo field here:

App/src/components/VideoPlayerContexts/VideoPopoverMenuContext.js

Lines 26 to 31 in 437d75c

    
           const downloadAttachment = useCallback(() => { 
        
               currentVideoPlayerRef.current.getStatusAsync().then((status) => { 
        
                   const sourceURI = `/${Url.getPathFromURL(status.uri)}`; 
        
                   fileDownload(sourceURI); 
        
               }); 
        
           }, [currentVideoPlayerRef]);

Also, we are already filename from url in AttachmentModal, so to maintain consistency we should use that in other places also.

App/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js

Lines 22 to 27 in 392efc7

    
           const splittedUrl = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE].split('/'); 
        
           attachments.unshift({ 
        
               reportActionID: null, 
        
               source: tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]), 
        
               isAuthTokenRequired: Boolean(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]), 
        
               file: {name: splittedUrl[splittedUrl.length - 1]},

cc: @s77rt

s77rt · 2024-02-22T22:55:04Z

@Krishna2323 Thanks for brining this case up. I think we should cover that one too but I'd prefer to avoid such extraction. Can we instead pass the attachmentInfo through the context values?

Krishna2323 · 2024-02-22T23:07:43Z

@s77rt, we can set state for filename and update it when using useVideoPopoverMenuContext with the new function which will update the fileName. We need to pass the fileName down three levels to the children components.

Krishna2323 · 2024-02-22T23:45:28Z

@s77rt, Proposal Update

Included an alternative solution

s77rt · 2024-02-23T02:01:37Z

@Krishna2323 Thanks for the update. I was checking again and I think we should just update fileDownload util so that we supply the source url without the auth token to FileUtils.getFileName. Can you look into that approach?

BhuvaneshPatil · 2024-02-23T02:33:50Z

@s77rt I have updated proposal that is easy tom implement without making lot of changes

My thoughts on updating fileDownload utils is that it may be used at may places and can cause regression (very low probability, but still should be considered), by changing in VideoPopovercontext we are ensuring the change is limited to our scope for this issue.

Krishna2323 · 2024-02-23T02:57:19Z

@BhuvaneshPatil, I already mentioned about getting fileName from url.

jeremy-croff · 2024-02-23T03:06:50Z

We should also look into any service changes that have occurred.

Staging vs Prod different html for the video:
Staging:

Prod:

You only download the corrupt file on the context menu picker. The attachment download works fine.

Prod does not have the download on the context menu picker, because isAttachment check doesn't allow says it's false in prod, true in staging

Prod html
"html": "<video data-expensify-source=\"https:\/\/www.expensify.com\/chat-attachments\/6973500176401041544\/w_4f1f4047c9f81aaeec4c636b644dfae469e0c558.mov\" data-expensify-width=640 data-expensify-height=368 data-expensify-duration=53 data-expensify-thumbnail-url=\"https:\/\/www.expensify.com\/chat-attachments\/6973500176401041544\/w_4f1f4047c9f81aaeec4c636b644dfae469e0c558.jpg\">Before_additional_changes-small.mov<\/video>", "text": "Before_additional_changes-small.mov",

staging html
"html": "<video data-expensify-source=\"https:\/\/staging.expensify.com\/chat-attachments\/5593190361139373290\/w_0bcbf0160801b56e75512a67d0e52a182c4e7d0d.mov\" data-expensify-width=1280 data-expensify-height=732 data-expensify-duration=53 data-expensify-thumbnail-url=\"https:\/\/staging.expensify.com\/chat-attachments\/5593190361139373290\/w_0bcbf0160801b56e75512a67d0e52a182c4e7d0d.jpg\">Before_additional_changes2.mov<\/video>", "text": "[Attachment]",

Check the logic here:

App/src/libs/isReportMessageAttachment.ts

Line 6 in 72bdedb

    
            * Ignore messages containing [Attachment] as the main content. Attachments are actions with only text as [Attachment].

This is why now, in staging, it's marking it as an attachment and previously not.

This is the real RCA for the regression. How to solve it is another topic

s77rt · 2024-02-23T03:57:49Z

@BhuvaneshPatil Thanks for the update. I still think we should update fileDownload lib or the FileUtils lib since it will also prevent similar bugs from occurring and it should be an easier fix.

melvin-bot · 2024-02-27T18:04:10Z

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

deetergp · 2024-02-27T18:04:40Z

Adding the Bug label so we get a BugZero agent.

A wild @dylanexpensify appears…

dylanexpensify · 2024-02-28T09:00:51Z

HELLO!

dylanexpensify · 2024-02-28T09:01:13Z

Let's get this movin!

Krishna2323 · 2024-03-04T07:26:26Z

@dylanexpensify, we are ready for payments here 😅, PR was deployed to production on 27th Feb.

deetergp · 2024-03-06T07:48:48Z

Sounds like we're just waiting on payments.

dylanexpensify · 2024-03-06T10:44:24Z

Apologies, I've been OOO sick, but getting to this today!

Krishna2323 · 2024-03-08T08:16:44Z

@dylanexpensify friendly bump for payments here :)

dylanexpensify · 2024-03-13T10:08:49Z

Apologies, paying now!

Krishna2323 · 2024-03-15T19:26:42Z

@dylanexpensify, I'm sorry if I'm disturbing you, but I still haven't been paid. 🙇🏻

dylanexpensify · 2024-03-19T10:11:40Z

Ahh no no not bothering me! @Krishna2323 paying literally right now!

dylanexpensify · 2024-03-19T10:14:00Z

Payment summary:

Contributor: @Krishna2323 $500
Contributor+: @s77rt $500

Please apply here asap or request!

Krishna2323 · 2024-03-19T10:36:13Z

@dylanexpensify, applied.

dylanexpensify · 2024-03-19T10:38:10Z

@Krishna2323 offer sent!

Krishna2323 · 2024-03-19T10:43:31Z

Accepted

dylanexpensify · 2024-03-19T10:44:33Z

@Krishna2323 paid!

Krishna2323 · 2024-03-19T10:44:54Z

Thanks :)

dylanexpensify · 2024-03-19T10:45:15Z

thank YOU!

s77rt · 2024-03-19T16:29:21Z

@dylanexpensify Applied! Thanks!

dylanexpensify · 2024-03-20T11:44:37Z

woooot!

dylanexpensify · 2024-03-20T14:32:58Z

@s77rt sent offer!!

s77rt · 2024-03-20T14:37:51Z

Accepted!

dylanexpensify · 2024-03-20T14:48:17Z

done!

lanitochka17 added DeployBlockerCash This issue or pull request should block deployment External Added to denote the issue can be worked on by a contributor labels Feb 22, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 22, 2024

melvin-bot bot assigned s77rt Feb 22, 2024

melvin-bot bot added the Daily KSv2 label Feb 22, 2024

github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Feb 22, 2024

github-actions bot mentioned this issue Feb 22, 2024

Deploy Checklist: New Expensify 2024-02-19 #36820

Closed

melvin-bot bot assigned deetergp Feb 22, 2024

deetergp added the Bug Something is broken. Auto assigns a BugZero manager. label Feb 27, 2024

melvin-bot bot assigned dylanexpensify Feb 27, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 27, 2024

dylanexpensify added this to [#whatsnext] #vip-vsb Mar 19, 2024

dylanexpensify closed this as completed Mar 20, 2024

github-project-automation bot moved this to CRITICAL in [#whatsnext] #vip-vsb Mar 20, 2024

Video - Invalid file is downloaded when downloading video #37092

Video - Invalid file is downloaded when downloading video #37092

Comments

lanitochka17 commented Feb 22, 2024 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Feb 22, 2024

melvin-bot bot commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

lanitochka17 commented Feb 22, 2024

BhuvaneshPatil commented Feb 22, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

BhuvaneshPatil commented Feb 22, 2024

Krishna2323 commented Feb 22, 2024 • edited Loading

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Result

Alternative

Alternative 2

Alternative 3

s77rt commented Feb 22, 2024 • edited Loading

melvin-bot bot commented Feb 22, 2024

s77rt commented Feb 22, 2024

Krishna2323 commented Feb 22, 2024 • edited Loading

Krishna2323 commented Feb 22, 2024

s77rt commented Feb 22, 2024

Krishna2323 commented Feb 22, 2024 • edited Loading

Krishna2323 commented Feb 22, 2024

s77rt commented Feb 23, 2024

BhuvaneshPatil commented Feb 23, 2024 • edited Loading

Krishna2323 commented Feb 23, 2024

jeremy-croff commented Feb 23, 2024 • edited Loading

s77rt commented Feb 23, 2024

melvin-bot bot commented Feb 27, 2024

deetergp commented Feb 27, 2024

dylanexpensify commented Feb 28, 2024

dylanexpensify commented Feb 28, 2024

Krishna2323 commented Mar 4, 2024 • edited Loading

deetergp commented Mar 6, 2024

dylanexpensify commented Mar 6, 2024

Krishna2323 commented Mar 8, 2024

dylanexpensify commented Mar 13, 2024

Krishna2323 commented Mar 15, 2024

dylanexpensify commented Mar 19, 2024

dylanexpensify commented Mar 19, 2024

Krishna2323 commented Mar 19, 2024

dylanexpensify commented Mar 19, 2024

Krishna2323 commented Mar 19, 2024

dylanexpensify commented Mar 19, 2024

Krishna2323 commented Mar 19, 2024

dylanexpensify commented Mar 19, 2024

s77rt commented Mar 19, 2024

dylanexpensify commented Mar 20, 2024

dylanexpensify commented Mar 20, 2024

s77rt commented Mar 20, 2024

dylanexpensify commented Mar 20, 2024

lanitochka17 commented Feb 22, 2024 •

edited

Loading

BhuvaneshPatil commented Feb 22, 2024 •

edited

Loading

Krishna2323 commented Feb 22, 2024 •

edited

Loading

s77rt commented Feb 22, 2024 •

edited

Loading

Krishna2323 commented Feb 22, 2024 •

edited

Loading

Krishna2323 commented Feb 22, 2024 •

edited

Loading

BhuvaneshPatil commented Feb 23, 2024 •

edited

Loading

jeremy-croff commented Feb 23, 2024 •

edited

Loading

Krishna2323 commented Mar 4, 2024 •

edited

Loading