Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.27k stars 2.7k forks source link

mWeb - Chat scrolls back to linked message after receiving new message #45093

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month ago

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.5.0 Reproducible in staging?: Y Reproducible in production?: Y 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): u.onyeukwu94@gmail.com Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/44819

Action Performed:

  1. Open a chat conversation with a user(B) that has scrollable history
  2. Go to the top comment and copy the link to that comment
  3. Open another chat, send link and tap link to navigate to comment
  4. Scroll to bottom to chat conversation
  5. With User B, send any message to chat

Expected Result:

User receives new message smoothly

Actual Result:

Chat conversation scrolls up to highlighted message that was navigated to initially

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/54c40146-2a95-4c34-9e45-78c99b4ebb2d

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 1 month ago

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

lanitochka17 commented 1 month ago

@bfitzexpensify 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

lanitochka17 commented 1 month ago

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

jacobkim9881 commented 1 month ago

I guess this came from https://github.com/Expensify/App/pull/41962

ishpaul777 commented 1 month ago

I guess this came from https://github.com/Expensify/App/pull/41962

PR is not on Prod. yet this issue is also reproducable on prod. so not from https://github.com/Expensify/App/pull/41962

tsa321 commented 1 month ago

Proposal

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

Chat scrolls back to linked message after receiving new message

What is the root cause of that problem?

Here:

https://github.com/Expensify/App/blob/c973e622ac7bf94a537b01e75408aff782123db3/src/pages/home/report/ReportActionsView.tsx#L141-L143

and here:

https://github.com/Expensify/App/blob/c973e622ac7bf94a537b01e75408aff782123db3/src/pages/home/report/ReportActionsView.tsx#L146-L157

We the useMemo and useLayoutEffect rely on route, which comes from useRoute. Every time the user receives a new message, the route object changes and triggers both useMemo and useLayoutEffect. Inside useMemo, isFirstLinkedActionRender.current is set to true, causing the report to scroll to the linked message.

The changes to the route object/navigation state occur due to:

https://github.com/Expensify/App/blob/4c235a733928b43ddc9e3a2bb5d96e170d2e0545/src/pages/home/report/ReportActionsList.tsx#L271

The Navigation.setParams({ referrer: undefined }) call alters the navigation state and updates the route object.

What changes should we make to solve the problem?

To address this issue, we should add a check before executing Navigation.setParams({ referrer: undefined }). For example:

if (isFromNotification) {
    Navigation.setParams({ referrer: undefined });
}

Alternative solution:

Another approach could be to adjust the dependencies of useMemo and useLayoutEffect to use route.path instead of route.

ishpaul777 commented 1 month ago

i am interested in taking over this issue as c+, i have good context working on comment-linking related issues. : )

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

bfitzexpensify commented 1 month ago

Cool, assigning you @ishpaul777

melvin-bot[bot] commented 1 month ago

Current assignee @ishpaul777 is eligible for the External assigner, not assigning anyone new.

ishpaul777 commented 1 month ago

@tsa321's Proposal looks good to me and test well.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

tsa321 commented 1 month ago

@ishpaul777 PR is ready.

ishpaul777 commented 1 month ago

fix for this was deployed to Prod yesterday.

tsa321 commented 3 weeks ago

cc: @bfitzexpensify . I think melvin automation broken here.

tsa321 commented 2 weeks ago

^^ cc @bfitzexpensify

bfitzexpensify commented 6 days ago

@tsa321 can you please link your Upwork profile?

tsa321 commented 5 days ago

@bfitzexpensify my upwork profile: https://www.upwork.com/freelancers/~01bfc26d267bade652

bfitzexpensify commented 5 days ago

Offer sent @tsa321.

I'm now out of office, so adding a BZ buddy to finalise the payout

@ishpaul777 can you complete the BZ checklist:

melvin-bot[bot] commented 5 days ago

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.

tsa321 commented 5 days ago

@bfitzexpensify I have accepted the offer.

ishpaul777 commented 4 days ago