[HOLD for payment 2024-06-28] [$500] HIGH: [Polish] Editing a comment with a video removes the video

[quinthar]

Reported by @zsgreenwald here: https://expensify.slack.com/archives/C066HJM2CAZ/p1715280747390969

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019eb5f4d74fc52968
• Upwork Job ID: 1788937751790014464
• Last Price Increase: 2024-06-27
• Automatic offers:
• DylanDylann | Reviewer | 0

Issue Owner

Current Issue Owner: @alexpensify

[quinthar]

Optimistically assigning to @tgolen

[tgolen]

It looks like this is because we don't support <video> tags in our HTML to markdown conversion.

@kidroca Do you agree that we just need to add support for <video> tags, like what you did with the <img /> ones?

[kidroca]

It looks like this is because we don't support <video> tags in our HTML to markdown conversion.

@kidroca Do you agree that we just need to add support for <video> tags, like what you did with the <img /> ones?

Yes, this seems to be the problem

We have to convert <video> to ![vid](src.mp4) and back to <video> in order to preserve the correct behaviour

[melvin-bot]

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019eb5f4d74fc52968

[melvin-bot]

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

[tgolen]

Great, thanks! Let's get this one worked on by a contributor.

[dominictb]

Proposal

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

Editing a comment with a video removes the video

What is the root cause of that problem?

We don't support <video> tags in our HTML to markdown conversion, and conversion back from markdown to <video>

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

1. Convert <video> to ![vid](src.mp4), we can add a rule in expensify-common to do this, similar to images.
   Some differences to note:

• The video will use data-expensify-source, not src like image
• The video will have closing tags, unlike image which doesn't

So the Regex will need to accommodate those differences

Pseudo-code:

{
    name: 'video',
    regex: /<video[^><]*data-expensify-source\s*=\s*(['"])(\S*?)\1(.*?)>([^><]*)<\/video>*(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
    replacement: (match, g1, g2, g3, g4) => {
        if (g4) {
            return `![${g4}](${g2})`;
        }

        return `!(${g2})`;
    },
},

This works well for current use cases but some details might need to be changed based on expectations

2. Convert video markdown back to <video>, we can add a rule in expensify-common to do this, similar to images. The difference with image is that the src of the video will have a video file extension (like .mp4)

Or we can just modify the rule here to work for both images and videos, based on source file extension

{
    name: 'imageOrVideo',
    regex: MARKDOWN_IMAGE_REGEX,
    replacement: (match, g1, g2) => {
        if (g2.endsWith('.mp4')) { // we should handle other video file extensions too
            return `<video src="${Str.sanitizeURL(g2)}">${g1 ? `${g1}` : ''}<\/video>`
        }
        return `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} />`
    },
    // will need to update this similarly
    rawInputReplacement: (match, g1, g2) => `<img src="${Str.sanitizeURL(g2)}"${g1 ? ` alt="${this.escapeAttributeContent(g1)}"` : ''} data-raw-href="${g2}" data-link-variant="${typeof (g1) === 'string' ? 'labeled' : 'auto'}" />`
},

We should take additional references from this PR too to make it work for videos like for images.

What alternative solutions did you explore? (Optional)

NA

Video

It will work fine like this after the fix
https://github.com/Expensify/App/assets/165644294/365b3911-4854-4fa8-830c-ca65c9846119

[dominictb]

@tgolen @kidroca To confirm:

• We have some details in the <video> like data-expensify-width, data-expensify-height, data-expensify-thumbnail-url
  Do they need to be included in the markdown? If yes, what will the markdown look like with those metadata?

• I assume the vid referred to here is the file name right?

![vid](src.mp4)

Because the format of the html is <video ...>fileName</video>. Or is the [vid] a hardcoding thing to distinguish the video markdown from the image markdown?

(FWIW it's ok for the markdown formats of video and image to be the same, we can distinguish them by the source file extension)

[dominictb]

Proposal updated to include more details

[DylanDylann]

In my opinion, In reality there are not many cases in which users want to update a video/image by updating the video/image link. When users send a video/image with a message, we should only allow the user to edit the message, not the video/image link. It means that when editing the message that is attached to a video/image, we only display the message in the edit composer and exclude the video/image link

[tgolen]

@dominictb I think it's OK to use the same markdown for both images and videos and base all logic off of file extensions.

We have some details in the

I think we do need to keep this, yes. Is there anyway to keep this without including it in the markdown syntax? They are fields that shouldn't be edited by the user.

@DylanDylann I think there are quite a few cases where users need to replace or delete a video/image entirely, so I don't think stripping out video/img tags from the editor is an option at this point.

[DylanDylann]

If that, @dominictb's proposal looks promising. Let's me deep dive into it

[DylanDylann]

@dominictb Same question

Is there anyway to keep this without including it in the markdown syntax?

[dominictb]

I think we do need to keep this, yes. Is there anyway to keep this without including it in the markdown syntax?

@tgolen @DylanDylann We can store the metadata in a local cache or in Onyx, it will be like an object with the video source as the key, similar to how we cached the thumbnail dimensions

We can initialize the cache when the user starts editing the message, and clear the cache after the user finishes editing

[laurenreidexpensify]

@alexpensify adding you here for parental leave sub. no action required this BZ this week, PR is still in review, but adding you for payment in coming weeks when the fix makes it to prod. Thanks

[alexpensify]

Noted!

[tgolen]

Weekly Update

• The expensify-common PR was merged
• Version 2.0.16 was automatically published to NPM

Next Steps

• @dominictb Update App with the new version of expensify-common

ETA

• Merged by Friday, June 28

@dominictb Have you been able to create the App PR that updates the expensify-common version?

[dominictb]

@tgolen #42463 it's already here.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.0-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: cache extra attributes for video markdown conversion #42463

If no regressions arise, payment will be issued on 2024-06-28. 🎊

For reference, here are some details about the assignees on this issue:

• @DylanDylann requires payment automatic offer (Reviewer)
• @dominictb requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@DylanDylann] The PR that introduced the bug has been identified. Link to the PR:
• [@DylanDylann] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@DylanDylann] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@DylanDylann] Determine if we should create a regression test for this bug.
• [@DylanDylann] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

Upwork job price has been updated to $500

[alexpensify]

@DylanDylann and @dominictb - to prepare for the payment date, please apply here:

https://www.upwork.com/jobs/~019eb5f4d74fc52968

Automation failed here and Upwork search is throwing an error. Thanks!

[alexpensify]

Payouts due: 2024-06-28

• Contributor: $500 @dominictb
• Contributor+: $500 @DylanDylann

Upwork job is here.

Status: Search was working better today. Please accept the Upwork jobs, and I can complete the process.

[alexpensify]

There is no update yet; the offers are pending in Upwork. It need to be approved by the contributors. Heads up, I'm OOO this week, but I'm trying to check in sporadically to help finish the required process here. I'll try to check again on Wednesday morning to see if I can complete the next steps for this GH.

[DylanDylann]

@alexpensify I accepted the offer, thanks 🚀

[dominictb]

@alexpensify I accepted it too, ty!

[melvin-bot]

@tgolen, @alexpensify, @DylanDylann, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

[alexpensify]

Closing - I've completed the payment process in Upwork.

#41952 (comment)