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] Chat - The written message does not appear in the composer after navigating via a deep link to the last message. #39603

Closed
3 of 6 tasks
kbecciv opened this issue Apr 4, 2024 · 20 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering 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

Comments

@kbecciv
Copy link

kbecciv commented Apr 4, 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: v1.4.60-1
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:-Use a Gmail account to reproduce the issue

  1. Sign in with a Gmail account> Go to any chat and write a message.
  2. Hover over the last message, click on the three dots, and select "Copy link."
  3. Go to another chat, paste the link, and send it. Then click on the link. Notice it takes you to the written message.
  4. Write a message and observe that it is not displayed.

Expected Result:

After copying the link of the last message into the composer and navigating to that message via the link, typing a new message should display it in the composer.

Actual Result:

The written message does not appear in the composer after navigating via a deep link to the last message.

Workaround:

n/a

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6437932_1712233977151.copyLink.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016fd0ee48a1e1341d
  • Upwork Job ID: 1777407122895777792
  • Last Price Increase: 2024-04-15
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

Triggered auto assignment to @amyevans (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Apr 4, 2024

👋 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.

@kbecciv
Copy link
Author

kbecciv commented Apr 4, 2024

We think that this bug might be related to #vip-vsb

@amyevans
Copy link
Contributor

amyevans commented Apr 4, 2024

I wouldn't consider this a deploy blocker because the bug existed prior to the latest deploy, the only difference is we've dropped the comment linking beta (and I don't think it's worth re-adding the beta due solely to this bug).

@roryabraham @perunt do you want to work on this? Or should I open it up to external proposals?

@amyevans amyevans added Daily KSv2 and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment labels Apr 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

@amyevans
Copy link
Contributor

amyevans commented Apr 8, 2024

Alright gonna go ahead and open this up to the community!

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2024
@amyevans amyevans added the Bug Something is broken. Auto assigns a BugZero manager. label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

@amyevans amyevans added the External Added to denote the issue can be worked on by a contributor label Apr 8, 2024
@melvin-bot melvin-bot bot changed the title Chat - The written message does not appear in the composer after navigating via a deep link to the last message. [$250] Chat - The written message does not appear in the composer after navigating via a deep link to the last message. Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

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

melvin-bot bot commented Apr 8, 2024

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

@kmbcook
Copy link
Contributor

kmbcook commented Apr 8, 2024

Proposal

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

Report actions view does not scroll to bottom after making a new comment on a page which is linked to a specific comment, preventing the new comment from being immediately visible.

What is the root cause of that problem?

shouldEnableAutoScroll is set to false when route contains a report action id, in code below.

const shouldEnableAutoScroll = hasNewestReportAction && (!reportActionID || !isNavigatingToLinkedMessage);

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

Remove report action id from route after user makes a new comment.

@dominictb
Copy link
Contributor

Proposal

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

The written message does not appear in the composer after navigating via a deep link to the last message.

What is the root cause of that problem?

Here, we use scrollToBottomForCurrentUserAction which is memorized, however, it's not in the dependency list here.

So when the page is first initialized, the scrollToBottomForCurrentUserAction has hasNewestReportAction as false, so it will never trigger scroll to bottom when the current user sends a new message, due to this logic

And scrollToBottomForCurrentUserAction is not in the list of dependencies of useEffect as mentioned above, so when hasNewestReportAction changes to true, the event callback here will still use the version of scrollToBottomForCurrentUserAction with hasNewestReportAction false, so the user is never scrolled to bottom when sending a new message, and is not able to see the message without manually scrolling.

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

Use a ref to store the scrollToBottomForCurrentUserAction, so that we will always access its latest value in the callback.
In here

const scrollToBottomForCurrentUserActionRef = useRef();
scrollToBottomForCurrentUserActionRef.current = scrollToBottomForCurrentUserAction;

In here

const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, (isFromCurrentUser) => scrollToBottomForCurrentUserActionRef.current(isFromCurrentUser));

What alternative solutions did you explore? (Optional)

Add scrollToBottomForCurrentUserAction to the dependency list here.

@melvin-bot melvin-bot bot added the Overdue label Apr 11, 2024
@amyevans
Copy link
Contributor

@alitoshmatov could you review the proposals please?

@melvin-bot melvin-bot bot removed the Overdue label Apr 12, 2024
@alitoshmatov
Copy link
Contributor

@kmbcook Thank you for your proposal, auto scroll is not problem here. The new messages are not available even if you scroll manually

Screen.Recording.2024-04-12.at.10.51.21.PM.mov

@alitoshmatov
Copy link
Contributor

@dominictb Thank you for your proposal, you are also misunderstood the issue, it is not related to scrolling, new messages are not outright visible

@kmbcook
Copy link
Contributor

kmbcook commented Apr 12, 2024

@alitoshmatov I'm not sure why we are seeing different behavior. This is what I am seeing:

39603.mov

@alitoshmatov
Copy link
Contributor

@kmbcook You should copy link for the last message

@kmbcook
Copy link
Contributor

kmbcook commented Apr 15, 2024

@alitoshmatov yes, but everything seems to be working properly now.

Copy link

melvin-bot bot commented Apr 15, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 16, 2024

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

@amyevans
Copy link
Contributor

Confirmed this is fixed on staging:

Screen.Recording.2024-04-16.at.4.11.37.PM.mov

So I'll go ahead and close!

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 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
Projects
Status: Done
Development

No branches or pull requests

6 participants