Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] LHN - Video name is shown in LHN briefly instead of attachment text #54366

Open
2 of 8 tasks
vincdargento opened this issue Dec 19, 2024 · 8 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue

Comments

@vincdargento
Copy link

vincdargento commented Dec 19, 2024

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: V9. 0.77-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team
Device used: Redmi note 10s android 13
App Component: Left Hand Navigation (LHN)

Action Performed:

  1. Go to https://staging.new.expensify.com
  2. Open a chat
  3. Upload a video and immediately navigate to LHN and note the report preview

Expected Result:

Video name must not be shown in LHN briefly and only attachment text must be shown.

Actual Result:

Video name is shown in LHN briefly instead attachment text.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021869790347889402203
  • Upwork Job ID: 1869790347889402203
  • Last Price Increase: 2024-12-19
Issue OwnerCurrent Issue Owner: @akinwale
@vincdargento vincdargento added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 19, 2024
Copy link

melvin-bot bot commented Dec 19, 2024

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

@bernhardoj
Copy link
Contributor

bernhardoj commented Dec 19, 2024

Proposal

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

Video name is shown when uploading a video to a chat. This happen to other attachments too except image.

What is the root cause of that problem?

When we send a new attachment, we get the optimistic HTML representation of the attachment.

App/src/libs/ReportUtils.ts

Lines 4432 to 4435 in e42622c

const attachmentHtml = getUploadingAttachmentHtml(file);
const htmlForNewComment = `${commentText}${commentText && attachmentHtml ? '<br /><br />' : ''}${attachmentHtml}`;
const textForNewComment = Parser.htmlToText(htmlForNewComment);

Then, we convert it to text using htmlToText, which will later be used as the optimistic last message text.

text: textForNewComment,

const lastAction = attachmentAction ?? reportCommentAction;
const currentTime = DateUtils.getDBTimeWithSkew();
const lastComment = ReportActionsUtils.getReportActionMessage(lastAction);
const lastCommentText = ReportUtils.formatReportLastMessageText(lastComment?.text ?? '');
const optimisticReport: Partial<Report> = {
lastVisibleActionCreated: lastAction?.created,
lastMessageTranslationKey: lastComment?.translationKey ?? '',
lastMessageText: lastCommentText,

However, htmlToText doesn't handle the video case yet. It only handles image.
https://github.com/Expensify/expensify-common/blob/706481004516cddf08b7652afdf5db951443fbea/lib/ExpensiMark.ts#L799-L803

Then, the BE return the correct last message text which contains the [Attachment] text.

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

We should add the case for video and other attachment to ExpensiMark parser too.

{
    name: 'video',
    regex: /<video[^><]*data-expensify-source\s*=\s*(['"])(\S*?)\1(.*?)>([^><]*)<\/video>*(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
    replacement: '[Attachment]',
},
{
    name: 'otherAttachment',
    regex: /<a[^><]*data-expensify-source\s*=\s*(['"])(\S*?)\1(.*?)>([^><]*)<\/a>*(?![^<][\s\S]*?(<\/pre>|<\/code>))/gi,
    replacement: '[Attachment]',
},

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We need to add the test to expensify-common repo ExpensiMark by asserting that an attachment link (image, video, others) will be parsed to [Attachment].

@lschurr lschurr added the External Added to denote the issue can be worked on by a contributor label Dec 19, 2024
@melvin-bot melvin-bot bot changed the title LHN - Video name is shown in LHN briefly instead of attachment text [$250] LHN - Video name is shown in LHN briefly instead of attachment text Dec 19, 2024
Copy link

melvin-bot bot commented Dec 19, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 19, 2024
Copy link

melvin-bot bot commented Dec 19, 2024

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

@kaushiktd
Copy link
Contributor

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

LHN - Video name is shown in LHN briefly instead of attachment text

What is the root cause of that problem?

The issue occurs because in buildOptimisticAddCommentReportAction determines the text displayed in the LHN by combining the comment's text and attachmentHtml.

getUploadingAttachmentHtml(file):

This function generates HTML for the attachment, including the file name. For a video file, this HTML includes the video name (e.g., "video.mp4").
When there’s no accompanying text, the optimistic action uses this HTML for both html and text, causing the file name to appear in the LHN.

Root Cause Summary:
The textForNewComment variable is derived from htmlForNewComment, which includes the file name for attachment-only messages. Since this variable is used to populate the LHN in the optimistic update, it briefly shows the file name instead of the intended "Attachment" placeholder.

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

For attachment-only comments, explicitly set the text to a placeholder (CONST.ATTACHMENT_MESSAGE_TEXT) instead of deriving it from the attachment's HTML.

const textForNewComment = Parser.htmlToText(htmlForNewComment);

Replace it with

const textForNewComment = isAttachmentOnly ? CONST. ATTACHMENT_MESSAGE_TEXT:Parser.htmlToText(htmlForNewComment);

video:-
screen-capture.webm

@shubham1206agra
Copy link
Contributor

@lschurr Please look into the ideal behaviour as it might be changed from #54295.
cc @puneetlath

Copy link

melvin-bot bot commented Dec 23, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Dec 23, 2024
Copy link

melvin-bot bot commented Dec 25, 2024

@akinwale, @lschurr Eep! 4 days overdue now. Issues have feelings too...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Overdue
Projects
None yet
Development

No branches or pull requests

6 participants