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

Web - New line marker on Task does not disappear when sending or receiving a message #25776

Closed
1 of 6 tasks
izarutskaya opened this issue Aug 23, 2023 · 16 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering

Comments

@izarutskaya
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to any chat that have a task shared to it or create a Task on the chat.
  2. Mark us unread the Task
  3. Send a message on the chat / or recieve a message on the chat

Expected Result:

New line marker should disappear when receiving a message or sending a message

Actual Result:

New line marker does not disappear when receiving a message or sending a message

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.56-18

Reproducible in staging?: Y

Reproducible in production?: N

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

T121.Newlinemarker-1.mp4
Recording.1299.mp4

Expensify/Expensify Issue URL:

Issue reported by: @daveSeife

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692752785781209

View all open jobs on GitHub

@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • 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
  • 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.
  • 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.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 23, 2023
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Aug 23, 2023

Proposal

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

New line marker on Task does not disappear when sending or receiving a message

What is the root cause of that problem?

Let's see the optimistic report here, we set lastVisibleActionCreated and lastReadTime equal

const optimisticReport = {
lastVisibleActionCreated: currentTime,
lastMessageTranslationKey: lodashGet(lastAction, 'message[0].translationKey', ''),
lastMessageText: lastCommentText,
lastMessageHtml: lastCommentText,
lastActorAccountID: currentUserAccountID,
lastReadTime: currentTime,

That makes isUnread function here return false so readNewestAction API isn't called and then new line marker doesn't appear

if (ReportUtils.isUnread(report)) {

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

We should update the isUnread function to lastReadTime <= lastVisibleActionCreated or when we add a comment we can set lastReadTime less than lastVisibleActionCreated some milliseconds.

return lastReadTime < lastVisibleActionCreated;

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Hourly KSv2 labels Aug 23, 2023
@tgolen
Copy link
Contributor

tgolen commented Aug 23, 2023

@dukenv0307 Thank you so much for looking into this. Were you by any chance able to find what caused this regression to happen?

@dukenv0307
Copy link
Contributor

@tgolen I think this happens after we refactor unread marker here https://github.com/Expensify/App/pull/18637/files

@dukenv0307
Copy link
Contributor

Before refactoring, if we add a comment we will clear the unread marker and scroll to bottom.

@tgolen
Copy link
Contributor

tgolen commented Aug 23, 2023

Ah, yes. That looks to be it. I haven't been familiar with this code in a long time. I am concerned that the solution that you propose could have other potential side-effects that we don't know about.

What is your gauge on how risky of a change it is to do lastReadTime <= lastVisibleActionCreated and that it could lead to more regressions?

I'm also considering that we should revert that refactoring PR as it seemed tied to several regressions.

@s-alves10
Copy link
Contributor

I think we should revert the PR

@tgolen
Copy link
Contributor

tgolen commented Aug 23, 2023

Yeah, I mostly agree with you. It sounds like there have been a couple of follow-up PRs to fix other blockers from it that would also need to be reverted, so we are evaluating now what kind of a mess we are in and how best to get out of it.

@tgolen
Copy link
Contributor

tgolen commented Aug 23, 2023

All right, we have decided to revert the original PR for now. It turned out there were no other post-fixes that needed reverting.

@tgolen
Copy link
Contributor

tgolen commented Aug 23, 2023

Discussion was in slack

@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@roryabraham
Copy link
Contributor

According to @MonilBhavsar, this is expected behavior, not a 🐛

Closing this out.

@roryabraham roryabraham removed the DeployBlockerCash This issue or pull request should block deployment label Aug 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 23, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

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 Engineering
Projects
None yet
Development

No branches or pull requests

7 participants