[HOLD for payment 2023-12-12] [$1000] Chat - The green line is displayed chaotically at the recipient

[lanitochka17]

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

---

Action Performed:

1. Go to https://staging.new.expensify.com/ on an Android (User B)
2. Sign into Android app (User A)
3. Send a message from User B to User A
4. In LHN of user A, the message received will be shown in bold 5.Open the message 6.From user B, send multiple messages to User A

Expected Result:

As user A is already in conversation page, the green line must not be shown when messages received from user B

Actual Result:

The green line is displayed chaotically at the recipient

Workaround:

Unknown

Platforms:

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

• [x] Android / native
• [x] Android / Chrome
• [ ] iOS / native
• [ ] iOS / Safari
• [ ] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.3.70-2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/78819774/1af8a7f0-1afc-4101-bb8f-26f3fd523695

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018a50aa27edf905a3
• Upwork Job ID: 1703890402212745216
• Last Price Increase: 2023-11-11
• Automatic offers:
• situchan | Contributor | 27473899

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [x] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [x] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Christinadobrzyn]

I can't reproduce this going to reach back out to QA to see if they can confirm it's still happening and update the steps to reproduce.

Reaching out to QA https://expensify.slack.com/archives/C9YU7BX5M/p1694794909719529

[lanitochka17]

Issue reproducible on Build 1.3.70-5 SG A50/Android11 Messages were sent one after another

https://github.com/Expensify/App/assets/78819774/dd41cfd0-e78d-4391-ad7e-3978d817ffe7

[Christinadobrzyn]

I can reproduce this... but it's not super consistent. Because of the inconsistency, I wonder if we should not fix this now and see if something else resolves it.

Using V1.3.71-2 on the Android app app Samsung Galaxy S22 Using v1.3.7 0-8 on web app Samsung

https://github.com/Expensify/App/assets/51066321/2c29f26a-e0ed-4807-bfeb-ded6117bcbbf

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[Christinadobrzyn]

hey @mananjadhav can you give me your thoughts on https://github.com/Expensify/App/issues/27456#issuecomment-1724503801

[Christinadobrzyn]

nudge @mananjadhav on https://github.com/Expensify/App/issues/27456#issuecomment-1724503801

[mananjadhav]

I tried a few times, but I couldn't. I checked on the emulator. Does it require a physical device to check?

[Christinadobrzyn]

I was using the Chrome Website on a physical Android device (User B) and messaging the Android app through Browserstack (User A).

It's not super consistent which is always tough to troubleshoot so I don't know if we should just close this and expect it will clear up with other issues.

[melvin-bot[bot]]

@mananjadhav @Christinadobrzyn 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!

[Christinadobrzyn]

I updated the OP to see if that helps @mananjadhav - can you try again to reproduce and let me know what you think is best next steps here... maybe we should close since it's difficult to reproduce.

[melvin-bot[bot]]

@mananjadhav, @Christinadobrzyn Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mananjadhav]

@Christinadobrzyn I think you can reassign this one. I haven't been able to spend as much time as this requires.

[melvin-bot[bot]]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @Santhosh-Sellavel (Internal)

[Christinadobrzyn]

Hey @Santhosh-Sellavel! Catching you up here, we're still reproducing this and seeing if it's necessary for a fix based on this comment. Let me know what you think!

[Santhosh-Sellavel]

Yeah this is something we should fix, if its reproducible.

[Christinadobrzyn]

Thanks @Santhosh-Sellavel can this be external?

[Santhosh-Sellavel]

Yes, it can be external.

[melvin-bot[bot]]

Current assignee @Santhosh-Sellavel is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

@Christinadobrzyn @Santhosh-Sellavel this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[Christinadobrzyn]

Gonna increase the price to see if that helps with proposals.

[melvin-bot[bot]]

Upwork job price has been updated to $1000

[Christinadobrzyn]

still waiting on proposals. Do you think this would be something that would fit one of our contractors @Santhosh-Sellavel? I can reach out to SWM to see if they have the bandwidth.

[Santhosh-Sellavel]

That's good idea, but lets wait for few days or a week. If no takers by then let's get help from SWM or Callstack!

[melvin-bot[bot]]

@Christinadobrzyn, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[roksanaz]

I am Roksana from Callstack - expert contributor group. I’d like to work on this job.

[melvin-bot[bot]]

📣 @roksanaz! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[Santhosh-Sellavel]

I am Roksana from Callstack - expert contributor group. I’d like to work on this job.

Can you submit a formal proposal please, thanks!

[melvin-bot[bot]]

📣 @Santhosh-Sellavel Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot[bot]]

The BZ member will need to manually hire roksanaz for the Contributor role. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

[mountiny]

Assigning @roksanaz But still please post a proposal here to discuss before raising a PR thanks!

[roksanaz]

I'm working on a proposal (should be ready tomorrow), but just to be sure: the desirable behavior is that when the user is active in chat screen, there shouldn't be a green line for new messages, am I correct? (citing the expected result: "As user A is already in conversation page, the green line must not be shown when messages received from user B")

[Santhosh-Sellavel]

Let's consider a scenario to understand the expected outcome.

There a two User A & User B. Following or the sequence of actions

1. User A is chatting with someone else and new messages are received from User B, thus making User B in Sidebar LHN bold.
2. Now User A navigates to User B's chat. Expected Behaviour is a Line indicator that should be shown above the first unread message.
3. User B still sends a couple more messages. Expected Behaviour is a Line indicator that should persist it's position
4. If User A responds or Navigates to a different change and comes back to chat then the Expected Behaviour is that the indicator should be hidden now. 5.User A is active on User B's chat and scrolling the old chat, now new messages are received. Now the expected behavior is when the user scrolls to the new message manually or by using the new messages badge, should see an indicator above the first unread item.
5. User A is active on User B's chat but viewing the latest, now new messages are received. I'm not sure, but I guess there should not be any indicator new messages should be shown right away.

@Christinadobrzyn/@mountiny Can you loop someone from the product team to confirm the expected behavior, correct me if wrong on the above. thanks!

[melvin-bot[bot]]

@roksanaz @Christinadobrzyn @Santhosh-Sellavel this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @Santhosh-Sellavel is eligible for the Internal assigner, not assigning anyone new.

[roksanaz]

I'm sorry for taking so much time. We had a discussion that maybe another PR concerning unread messages might resolve this issue, but unfortunately that's not the case :( I need to consult 1 thing and my proposal should be ready after that.

[Christinadobrzyn]

I think this is correct but gonna ask Vit if they can double-check- https://github.com/Expensify/App/issues/27456#issuecomment-1758212639

[Christinadobrzyn]

Sounds like this job was discussed between @MonilBhavsar and @roksanaz - https://expensify.slack.com/archives/C03UK30EA1Z/p1697013288899459

so I think they should be able to answer your questions @Santhosh-Sellavel once they get things narrowed down?

[roksanaz]

@Santhosh-Sellavel and @Christinadobrzyn yes, we discussed the logic from this comment and points 1-4 are tested and working in this PR, point 5 will be implemented within this issue.

I'm on the right track with my proposal, should be ready on Monday.

[roksanaz]

Proposal

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

If the user A is already active in the chat thread with the user B, the user A receives a new message from the user B, a green line appears above the new message. If more messages come (especially at the same time) the green line appears erratically. The expected behaviour is that no green line appears for new messages if the thread is scrolled to the latest message.

What is the root cause of that problem?

I could reproduce this behaviour for all platforms. The problem isn’t specific only for Android. The root cause for the problem is that we don’t take into account the position of the user (the vertical offset) in the conditions for the flag called shouldDisplayNewMarker, which if it’s true - the green line appears. Currently on the first render of the message shouldDisplayNewMarker is true if the flag lastReadTime sent by the server is smaller than the message’s created time (and it is always true for new messages):

https://github.com/Expensify/App/blob/7d08e237dad360573c113d5f8c34d2b42ef49a93/src/pages/home/report/ReportActionsList.js#L292

Then, the application sends information to the server that the message is read, the message is re-rendered, shouldDisplayNewMarker is false and the green line disappears.

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

As the desired behaviour is not to display the green line for any new messages provided that the thread is scrolled to the latest message, we should take into account the vertical offset of the user as it’s the case with the floating button. shouldDisplayNewMarker should be false for threads at the end of the threshold before sending the information to the server to avoid green line appearing (or flickering). If the thread is scrolled up to older messages, then the floating button should appear and after scrolling down to the newest messages, the green line should appear above the new message. There is already a variable in the code called canDisplayMarker, which holds the information about the user’s position and if the user is active:

https://github.com/Expensify/App/blob/7d08e237dad360573c113d5f8c34d2b42ef49a93/src/pages/home/report/ReportActionsList.js#L297

But now it’s introduced after shouldDisplayNewMarker is already true and taken into account only to decide which message should be marked as new. So my solution is to introduce canDisplayMarker earlier and check it while deciding for the first time if shouldDisplayNewMarker should be true, like so:



shouldDisplayNewMarker = isCurrentMessageUnread && !isMessageUnread(nextMessage, report.lastReadTime) && canDisplayMarker;

[Santhosh-Sellavel]

Looks good, let's hold for the other PR to get merged.

[MonilBhavsar]

I think we can move forward here since it doesn't overlap with the other PR. @roksanaz do you think we can get rid of canDisplayMarker completely and have only one variable shouldDisplayMarker. We previously thought of doing this, but didn't, but figured it was something we could do. I think this is good opportunity to get rid of two similar variables. Also cc @gedu for thoughts on the above proposal

[roksanaz]

@MonilBhavsar We could get rid of this variable, but then shouldDisplayMarker would consist of an unnamed condition instead of canDisplayMarker, like so:

shouldDisplayNewMarker = isCurrentMessageUnread && 
      !isMessageUnread(nextMessage, report.lastReadTime) &&
      scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true;

If I were to do a complete refactor of this part, I would make it more readable, so instead of this:

https://github.com/Expensify/App/blob/cf16dacf2eb12cd5263e0a1d09eb1638b0fe5c93/src/pages/home/report/ReportActionsList.js#L290-L301

I would write it like that:

const nextMessage = sortedReportActions[index + 1];
const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime);
const isNextMessageRead = !isMessageUnread(nextMessage, report.lastReadTime);
const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < 
    userActiveSince.current : true;

shouldDisplayNewMarker = isCurrentMessageUnread && isNextMessageRead && isWithinVisibleThreshold;

if (shouldDisplayNewMarker && !messageManuallyMarkedUnread) {
    shouldDisplayNewMarker = reportAction.actorAccountID !== Report.getCurrentUserAccountID();
}

if (shouldDisplayNewMarker) {
    setCurrentUnreadMarker(reportAction.reportActionID);
}

Let me know which approach is best for you.

[MonilBhavsar]

Looks like last condition is missing currentUnreadMarker . But later one looks clean to me. Thanks!

 if (!currentUnreadMarker && shouldDisplayNewMarker) {
   setCurrentUnreadMarker(reportAction.reportActionID);
}

[roksanaz]

I think !currentUnreadMarker condition was redundant in line 299, as all of this logic is wrapped in the same condition:

https://github.com/Expensify/App/blob/91e2120e647aa9ee22171b6dbb772b4aebe64992/src/pages/home/report/ReportActionsList.js#L289

So I'll head in this direction with my PR, thank you.

[MonilBhavsar]

Yes, you're right 👍 Thank you!

[melvin-bot[bot]]

Current assignee @Santhosh-Sellavel is eligible for the External assigner, not assigning anyone new.

[MonilBhavsar]

Looks like draft PR is up and in being pre-reviewed