HIGH: [Polish] Add support for inline image Markdown

[quinthar]

Problem:

NewDot has the ability to display inline image -- we send them all the time from Concierge. But there is no way for a user to send one.

Solution:

Add support for the Markdown image syntax, initially for publicly hosted images to start.

[quinthar]

@kidroca FYI

[kidroca]

I'm not able to self-assign myself to this issue, but this comment should allow anyone with access to assign me

In the meantime I'll review how we're currently handling MD parsing and how that integrates with the HTML renderer used in report action comments and then propose how we might update it to handle MD image syntax for publicly hosted images

[kidroca]

Proposal

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

The problem is that NewDot currently lacks the functionality for users to send inline images, although it can display images sent from Concierge.

What is the root cause of that problem?

The root cause is that ExpensiMark does not support Markdown image syntax, which is essential for embedding images in user-generated content.

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

To address the problem, we need to enhance the ExpensiMark library, which is responsible for processing comments in NewDot by converting them into HTML before they are sent to the backend. Specifically, the following updates are recommended for ExpensiMark to improve image handling:

1. Markdown Image Syntax to HTML Conversion Rule: Implement a rule within ExpensiMark to convert Markdown image syntax (![Alt text](URL)) to an HTML <img> tag. This rule ensures that users can embed images directly in their comments using Markdown, which the library will then automatically translate into HTML for rendering in the web interface.

2. HTML Image Tag to Markdown Conversion Rule: Add a rule for converting HTML <img> tags back to Markdown image syntax. This is important for maintaining consistency when editing comments that already contain images, allowing the original Markdown formatting to be preserved and edited by users.

3. Optional HTML Image Tag to Text Conversion Rule: Introduce an optional rule for converting HTML <img> tags into plain text descriptions of the images, utilizing the alt attribute.

What alternative solutions did you explore? (Optional)

N/A

[kidroca]

Status Update

I've initiated a pull request to introduce the feature of embedding public images using Markdown syntax, available at: PR #37566.

Upon enabling this functionality, we've encountered a couple of significant, unplanned issues:

1. Non-Unique URLs for Public Images: The feature allows the same public image to be embedded multiple times across comments, leading to non-unique source URLs. This complicates our existing logic for attachment comparison and carousel positioning.

2. Public Images Without File Extensions: Some public images do not have file extensions in their URLs, which disrupts our fullscreen attachment rendering logic. This results in fallback content being displayed instead of the intended image.

Secondary Issue:

• Markdown Syntax Translated into HTML Attributes: In the expensify-common PR, we've noted that Markdown syntax is being translated into HTML within attributes, such as alt tags for images. While this edge case isn't causing visible problems at the moment, it has the potential for unexpected behavior.

Proposed Solutions:

• For the first issue, I recommend dynamically generating identifiers for each embedded image using the format ${reportActionID}-${source}-${attachmentIndex}, allowing for unique identification even when the same image is used multiple times.

• To address the second issue, I propose maintaining the current extension-based identification logic but adding a fallback mechanism. If an image lacks an extension, we should attempt to render it as an image first. Should this fail, we can then resort to displaying fallback content. Alternatively, a function could be developed to predict the attachment type based on known public addresses.

• Regarding the Markdown Syntax in HTML Attributes issue, I recommend deferring this to a secondary follow-up task. Currently, there is no clear path forward that guarantees a timely resolution without potentially delaying our primary task. Without a definitive ETA for devising and implementing a solution, prioritizing this fix would put a halt on the current feature

I look forward to your feedback on these proposals to ensure we can move forward effectively.

[melvin-bot]

@kidroca Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

Assigning @situchan as C+ to review first since the PR got assigned to me.

[melvin-bot]

@puneetlath, @kidroca, @situchan Huh... This is 4 days overdue. Who can take care of this?

[kidroca]

Status Update

• ✅ PR Ready for Review: Add support for inline image Markdown #37566

Addressed Unplanned Issues

1. Non-Unique URLs for Public Images: The feature allows the same public image to be embedded multiple times across comments, leading to non-unique source URLs.

To address this, I've implemented a filter in the fullscreen carousel to exclude duplicate attachments, ensuring only unique items are displayed. This approach simplifies the logic and enhances the carousel's user experience.
(This differs from my original proposal, but it's conveniently simple)

2. Public Images Without File Extensions: Some public images do not have file extensions in their URLs, disrupting our fullscreen attachment rendering.

I've updated the filename extraction logic to assign a generic filename (e.g., image.jpg) for such cases, ensuring these images are correctly handled by our carousel.

3. Markdown Syntax Translated into HTML Attributes: In the expensify-common PR, we've noted that Markdown syntax is being translated into HTML within attributes, such as alt tags for images.

A follow-up PR has been opened in expensify-common to escape Markdown special characters in the alt attribute, preventing unintended HTML conversion.

---

All issues are now resolved, making the feature ready for review and QA. The solutions are straightforward and add minimal complexity (about 30 line changes), offering a solid foundation for any potential future enhancements.

[kidroca]

Status Update

🟢 All related PRs have been approved. I've made a final pass to address minor points and clarifications:

• Add support for inline image Markdown expensify-common#658
• Add support for inline image Markdown - attribute fix expensify-common#661
• Add support for inline image Markdown #37566

Potential Follow-Ups

Post-merge, we might need to tackle:

1. Public Images: Concerns have been raised about using public images directly (Slack thread). Considering in-house processing and hosting could be beneficial.
2. Carousel Navigation: To address issues arising from filtering duplicate sources, selecting a duplicate attachment should navigate to its original instance, improving user experience.
3. URLs Without File Extensions: Some public image URLs lack file extensions, causing AttachmentView to fail in recognizing them as images. Although we have a workaround, a more permanent solution is needed for better image detection.
4. MD Edge Cases: Markdown code blocks within image tags result in invalid HTML (img tags). The conversion process needs refining to avoid transforming image sources into anchor tags.

[situchan]

Payment is still pending. @quinthar can you please reopen and add Bug label? Thanks

PRs I reviewed:
Expensify/expensify-common#658
#37566

[situchan]

@marcaaron can you please reopen and add New feature label for payment?

[melvin-bot]

Triggered auto assignment to @muttmuure (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[marcaaron]

@muttmuure @situchan was C+ on this one

[muttmuure]

Not sure how this was missed

@situchan here's a payment issue: https://www.upwork.com/jobs/~021838962709358979103

[situchan]

This is super old issue before price change and original bounty ($500) applies here

Not sure how this was missed

Please remove Reviewing label

[mallenexpensify]

@situchan can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~021838962709358979103

Also, confirmed the date was when we were paying $500

[situchan]

Accepted thanks

[mallenexpensify]

Contributor+: @situchan paid $500 via Upwork.

@situchan do we want a test case for this?

[situchan]

This is super old issue so I am not sure if we already created regression tests for inline image markdown.

[mallenexpensify]

@situchan can you propose the steps plz? I'd like to create the GH issue, QA can review and close if it's not needed. Thx

[situchan]

Regression Test

1. Login and navigate to any chat
2. Compose a new message with both public image URL and accompanying text.
   i.e.

An inline image and some text ![off](https://images.unsplash.com/photo-1709983723739-d72ea88434dd?q=80&w=2940&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D)

3. Verify that image thumbnail displays below message in composer
4. Send that message
5. Verify that the message correctly displays image thumbnail alongside the text
6. Click on the thumbnail
7. Verify that image preview modal correctly displays

[mallenexpensify]

Thanks @situchan test case created, closing!

• https://github.com/Expensify/Expensify/issues/438050