[$250] Attachment - CSV file appearance changes after edit comment

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.32-0 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Open a chat
3. Enter a text + upload a .csv file and send the message
4. Note in LHN, text +attachment is displayed
5. Long press and tap Edit comment
6. Add something to text and save it
7. Note the appearance of csv file in conversation page
8. Note in LHN, text+csv file name is displayed now

Expected Result:

CSV file appearance must not change after edit comment.

Actual Result:

CSV file appearance changes after edit comment.

Workaround:

Unknown

Platforms:

• [x] Android: Native
• [x] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Bug6599653_1726042133007!Expensify-2024-09-10_13_26_25.660_1.csv

https://github.com/user-attachments/assets/c0581d62-8add-40dc-be14-f22bcf486284

https://github.com/user-attachments/assets/e970e7aa-2c09-4697-966b-137fd6114a57

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021834376631265791320
• Upwork Job ID: 1834376631265791320
• Last Price Increase: 2024-09-19

[melvin-bot[bot]]

Triggered auto assignment to @sakluger (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.

[IuliiaHerets]

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-09-12 18:16:08 UTC.

Proposal

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

After editing a text with attachment, the attachment becomes a link.

What is the root cause of that problem?

The attachment itself (text, csv, etc.) is a link and will be rendered as a default attachment view if the anchor tag contains a data-expensify-source attribute, https://github.com/Expensify/App/blob/7748eff72fc43ecaf3db6159928d8d581a63ab99/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.tsx#L49-L56 https://github.com/Expensify/App/blob/7748eff72fc43ecaf3db6159928d8d581a63ab99/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.tsx#L25

for example:

test<br /><br /><a href=\"https://www.expensify.com/chat-attachments/6278837139050830107/w_cc2b0b1c27de873655ddff7c52ebc88e99b12378.txt\" data-expensify-source=\"https://www.expensify.com/chat-attachments/6278837139050830107/w_cc2b0b1c27de873655ddff7c52ebc88e99b12378.txt\">expire-2024-09-07_09_58_25.120.txt</a>

When we open the edit composer, the HTML will be converted to a link markdown.

And when we save it, we convert it back to HTML again, but the current parser can't differentiate between a normal link and an attachment link, so the attachment now becomes a text link.

test a<br /><br /><a href=\"https://www.expensify.com/chat-attachments/6278837139050830107/w_cc2b0b1c27de873655ddff7c52ebc88e99b12378.txt\" target=\"_blank\" rel=\"noreferrer noopener\">expire-2024-09-07_09_58_25.120.txt</a>

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

In the ExpensiMark link rule, we need to add data-expensify-source back to the anchor tag if it's an attachment. To check whether it's an attachment or not, we can check if the link contains chat-attachments.

const isAttachment = !!g2.match(/chat-attachments/);
if (isAttachment) {
    const src = Str.sanitizeURL(g2);
    return `<a href="${src}" data-expensify-source="${src}">${g1.trim()}</a>`;
}
return `<a href="${Str.sanitizeURL(g2)}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;

But this only fixes the offline case because the BE still removes the data-expensify-source, so we need to fix the BE too.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[sakluger]

Hey @shubham1206agra what do you think of the proposal above?

[shubham1206agra]

@sakluger Can we get an engineer here for 2nd opinion?

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[sakluger]

@shubham1206agra do you think we should fix the BE first before working on the offline case? Or are you saying that you'd rather defer to our team's decision?

[shubham1206agra]

@sakluger I think we need to decide what should we do here.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[sakluger]

I think that we should fix the backend first. @youssef-lr do you agree?

[sakluger]

@youssef-lr what do you think?

[youssef-lr]

Agreed @sakluger with fixing backend first

[Kalydosos]

A workaround in the FE could be to modify TextCommentFragment to return from RenderHTML quite the same visual as in the AttachmentPickerWithMenuItems.tsx wrapping around anchors as the files are already downloadable debug49469 again<br /><br /><a href="https://www.expensify.com/chat-attachments/8222304194949067457/w_f639f45aa90032e3b21efdfe3b18022c3ada6b17.csv" target="_blank" rel="noreferrer noopener">NOTICE.csv</a><edited ></edited>

i mean this visual

`

Also as i have worked on #49469 i can tell that the 2 issues are totally unrelated

[melvin-bot[bot]]

@sakluger @youssef-lr @shubham1206agra this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[sakluger]

Hmm @youssef-lr @shubham1206agra what do you think of the FE workaround proposal above? Would it be worth doing that, or should we still prioritize fixing in the BE?

[Kalydosos]

Edited by proposal-police: This proposal was edited at 2024-10-01 22:44:52 UTC.

Proposal

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

After editing a text with CSV file attachment, the attachment becomes a link.

What is the root cause of that problem?

For plain text files, the BE just returns a simple anchor link for updateComment instead of metadata (data-expensify-source=, data-optimistic-src=, data-name=) like it returns for addTextAndAttachment

<a href="https://www.expensify.com/chat-attachments/8222304194949067457/w_f639f45aa90032e3b21efdfe3b18022c3ada6b17.csv" target="_blank" rel="noreferrer noopener">NOTICE.csv</a><edited ></edited>

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

As the necessary information (the url of the attachment) is already provided by the BE we can just use it to decorate the anchor link so it can be rendered as the anchor link for addTextAndAttachment to have the visual below so changing the BE response won't be necessary anymore

For that, we must add/change the following codes :

1. in isReportMessageAttachment.tsx we must make this regex more accurate from

https://github.com/Expensify/App/blob/0bd13dd98b0a15714c1fd06999c32cabff5f4001/src/libs/isReportMessageAttachment.ts#L5

to

const attachmentRegex = new RegExp( ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}=, 'i'); const chatAttachmentRegex = /\(?[?=https:\/\/][^\)]+expensify\.com\/chat-attachments[^\)]+\)?/mi ; attachmentRegex only match addTextAndAttachment response html not updateComment response html so the need to add chatAttachmentRegex

and then replace

https://github.com/Expensify/App/blob/0bd13dd98b0a15714c1fd06999c32cabff5f4001/src/libs/isReportMessageAttachment.ts#L22

by

const hasAttachmentHtml = attachmentRegex.test(message.html) || chatAttachmentRegex.test(message.html);

2 for TextCommentFragment.tsx we must create the following function

function decorateAttachmentsAnchorsLinks(htmlMessage : string):string { return htmlMessage?.replace(/<a\s+[^>]expensify.com\/chat-attachments[^>]+>.<\/a>/gi, (attachmentAnchor) => { if (!!attachmentAnchor && !new RegExp(${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}=, 'i').test(attachmentAnchor)){ const subpartsRegex = /<a\s+href="(?[^"]+)"(?[^>]+)>(?[^<]+)/ ; const attachmentAnchorSubparts:RegExpMatchArray|null= attachmentAnchor.match(subpartsRegex) ; if (!!attachmentAnchorSubparts){ const anchorSrc = ${attachmentAnchorSubparts.groups?.attachmentSource} ; const anchorDataname = ${attachmentAnchorSubparts.groups?.attachmentName} ; const anchorSrcAttribute = ${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}=${anchorSrc} ; const anchorOptimisticSrcAttribute = ${CONST.ATTACHMENT_OPTIMISTIC_SOURCE_ATTRIBUTE}=${anchorSrc} ; const anchorDatanameAttribute = ${CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE}=${anchorDataname} ; const anchorOtherAttributes = ${attachmentAnchorSubparts.groups?.attachmentAttributs} ; const anchorFirst = <a href=${anchorSrc} ${anchorSrcAttribute} ${anchorOptimisticSrcAttribute} ${anchorDatanameAttribute} ${anchorOtherAttributes}> ; return ${anchorFirst}${anchorDataname}</a><br /> ; } } return attachmentAnchor ; }); }

to decorate the relevant anchors. I added a linebreak to make sure the (edited) info goes under the visual

We can then use that function in TextCommentFragment by changing the line

https://github.com/Expensify/App/blob/0bd13dd98b0a15714c1fd06999c32cabff5f4001/src/pages/home/report/comment/TextCommentFragment.tsx#L62

to

const htmlWithDeletedTag = styleAsDeleted ? <del>${html}</del> : decorateAttachmentsAnchorsLinks(html);

RESULT

The edition works just fine, the visual is renderered correctly

What alternative solutions did you explore? (Optional)

None

[sakluger]

@Kalydosos thanks for the full proposal! Let's see what @shubham1206agra thinks.

[sakluger]

@shubham1206agra bump - would the proposal above work?

[sakluger]

I sent a reminder message in Slack: https://expensify.slack.com/archives/C02NK2DQWUX/p1728247296583179

[shubham1206agra]

@Kalydosos Can you rephrase your solution? I do not understand what you are proposing.

[Kalydosos]

@shubham1206agra I mean we already have the necessary information from the BE to display plain text files attachments with the same visual before and after the comment edition.

1. We can use the following function to make sure that plain text files attachments are decorated with the necessary metadata before passing the information to TextCommentFragment.

function decorateAttachmentsAnchorsLinks(htmlMessage : string):string { return htmlMessage?.replace(/<a\s+[^>]*expensify\.com\/chat-attachments[^>]+>.*<\/a>/gi, (attachmentAnchor) => { if (!!attachmentAnchor && !new RegExp(${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}=, 'i').test(attachmentAnchor)){ const subpartsRegex = /<a\s+href="(?<attachmentSource>[^"]+)"(?<attachmentAttributs>[^>]+)>(?<attachmentName>[^<]+)/ ; const attachmentAnchorSubparts:RegExpMatchArray|null= attachmentAnchor.match(subpartsRegex) ; if (!!attachmentAnchorSubparts){ const anchorSrc =${attachmentAnchorSubparts.groups?.attachmentSource}; const anchorDataname =${attachmentAnchorSubparts.groups?.attachmentName}; const anchorSrcAttribute =${CONST.ATTACHMENT_SOURCE_ATTRIBUTE}=${anchorSrc}; const anchorOptimisticSrcAttribute =${CONST.ATTACHMENT_OPTIMISTIC_SOURCE_ATTRIBUTE}=${anchorSrc}; const anchorDatanameAttribute =${CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE}=${anchorDataname}; const anchorOtherAttributes =${attachmentAnchorSubparts.groups?.attachmentAttributs}; const anchorFirst =<a href=${anchorSrc} ${anchorSrcAttribute} ${anchorOptimisticSrcAttribute} ${anchorDatanameAttribute} ${anchorOtherAttributes}>; return${anchorFirst}${anchorDataname}
; } } return attachmentAnchor ; }); }

That function will be used by changing the line

https://github.com/Expensify/App/blob/0bd13dd98b0a15714c1fd06999c32cabff5f4001/src/pages/home/report/comment/TextCommentFragment.tsx#L62

into

const htmlWithDeletedTag = styleAsDeleted ? ${html} : decorateAttachmentsAnchorsLinks(html);

2. Also we need to add this regex in isReportMessageAttachment.tsx

const chatAttachmentRegex = /(?[?=https:\/\/][^)]+expensify.com\/chat-attachments[^)]+)?/mi ;

and change the line

https://github.com/Expensify/App/blob/0bd13dd98b0a15714c1fd06999c32cabff5f4001/src/libs/isReportMessageAttachment.ts#L22

into

const hasAttachmentHtml = attachmentRegex.test(message.html) || chatAttachmentRegex.test(message.html);

because attachmentRegex doesnt match the plain text files attachments in updateComment response because those links don't contain "data-expensify-source" and contain instead "expensify.com/chat-attachments"

[Kalydosos]

@shubham1206agra Hi, what do you think of it ?

[sakluger]

Commenting for Melvin. @shubham1206agra does the explanation above help?

[shubham1206agra]

@sakluger I have some more questions.

@youssef-lr Do we support globally shareable links? Cause there will be no point of even copying the links if it does not work at other places.

[Kalydosos]

@sakluger I have some more questions.

@youssef-lr Do we support globally shareable links? Cause there will be no point of even copying the links if it does not work at other places.

@shubham1206agra the solution dont copy the links, it replaces them inside the string when they match, there should be no side effects as it only replace the links when they match the pattern of attachment of type anchors and only for/after comments were edited. Does this helps you better ?

[Kalydosos]

@shubham1206agra do you have a scenario in mind that could cause the solution not to function ?

[sakluger]

We're still discussing proposals.

[shubham1206agra]

@sakluger This should be a BE bug. You should mark this as internal.

[sakluger]

@shubham1206agra thanks for confirming. It is already labeled as internal, so we'll keep it that way.

[bernhardoj]

Since we need an FE fix too, I think keeping it as External is better.

[sakluger]

@shubham1206agra @youssef-lr do you agree with @bernhardoj that we should also keep this external?

Personally, I'd rather fix the BE issue first before working on the FE fix. I don't want to get into a situation where we fix the front end and pay that out, then the BE fix doesn't get prioritized for months.

[youssef-lr]

Agreed with you @sakluger, let's fix it in the backend first

[shubham1206agra]

Waiting for BE changes

[sakluger]

@youssef-lr would you be able to work on the BE fix? Or should we discuss prioritization in Slack and find another engineer?

[youssef-lr]

@sakluger I'm afraid I don't have bandwidth right now, let's go with the latter please 🙏🏼

[sakluger]

Sounds good! This is already in the #quality project as a Medium priority, so I'll set to weekly for now.

[sakluger]

No updates.

[sakluger]

No updates.

[sakluger]

No updates. Hopefully we'll get a volunteer soon!