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.53k stars 2.88k forks source link

[$250] [Search v2.2] Chat is scrolled to the bottom when refreshing the page #48808

Closed IuliiaHerets closed 1 month ago

IuliiaHerets commented 2 months 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: v9.0.31-0 Reproducible in staging?: Y Reproducible in production?: N/A Email or phone of affected tester (no customers): applausetester+shsb22ertt@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Log in to Newdot
  2. Go to Search option from bottom tab
  3. Click on "Chat" type from LHP
  4. Click on any message which is not at the bottom of a chat report
  5. When the report view is opened in RHP refresh the browser
  6. Now notice how the chat report is scrolled to the bottom

Expected Result:

The chat report in RHP is not scrolled down to the bottom and stays in the same report action opened before refreshing the page

Actual Result:

The chat report in RHP is scrolled down to the bottom and user has to scroll up to find the chat report opened before refreshing the page

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/2ec8a3e6-58eb-4939-915b-a47a0f04ef64

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021833217065296959946
  • Upwork Job ID: 1833217065296959946
  • Last Price Increase: 2024-09-16
  • Automatic offers:
    • ikevin127 | Contributor | 103886648
    • huult | Contributor | 104028956
Issue OwnerCurrent Issue Owner: @rojiphil
melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

IuliiaHerets commented 2 months ago

We think that this bug might be related to #wave-control

github-actions[bot] commented 2 months ago

:wave: 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.
luacmartins commented 2 months ago

I'll take this since it's part of Search

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $125

luacmartins commented 2 months ago

We should navigate the user to the reportActionID as well, not only the report

luacmartins commented 2 months ago

@shubham1206agra

melvin-bot[bot] commented 2 months ago

πŸ“£ @ikevin127 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $250

luacmartins commented 2 months ago

I'm gonna demote this to NAB since it's a minor issue.

luacmartins commented 1 month ago

Looking for proposals

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] commented 1 month ago

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

alexpensify commented 1 month ago

Open for proposals here

huult commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-19 15:15:39 UTC.

Proposal

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

Chat is scrolled to the bottom when refreshing the page

What is the root cause of that problem?

The report action list is missing the initialScrollIndex in the InvertedFlatList.

The reason is that sortedVisibleReportActions has not finished rendering, but onStartReached was called to append the remaining data, causing the ReportActionList to render the entire data.

Screenshot 2024-09-20 at 06 31 00

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

We should add initialScrollIndex to the InvertedFlatList. Instead of starting at the top with the first item, it should start at the initialScrollIndex.

```diff // .src/pages/home/report/ReportActionsList.tsx#L202 + const indexOfLinkedMessage = useMemo((): number => sortedVisibleReportActions.findIndex((obj) => String(obj.reportActionID) === String(linkedReportActionID)), [sortedVisibleReportActions, linkedReportActionID]);

// .src/pages/home/report/ReportActionsList.tsx#L646 ~~ <InvertedFlatList ~~ ~~ ...~~ + initialScrollIndex={indexOfLinkedMessage !== -1 ? indexOfLinkedMessage : undefined} /> ```

To resolve this issue, we should delay the call to onStartReached until the initial sortedVisibleReportActions data is applied and the page is re-rendered.

// .src/pages/home/report/ReportActionsList.tsx#L664
-     onStartReached={onStartReached}
+     onStartReached={() => {
+                        InteractionManager.runAfterInteractions(() => {
+                            requestAnimationFrame(() => {
+                                onStartReached();
+                            });
+                        });
+                    }}

What alternative solutions did you explore? (Optional)

We can use reportScrollManager.scrollToIndex to scroll to the message index

```diff + useEffect(() => { + if (indexOfLinkedMessage > -1) { + InteractionManager.runAfterInteractions(() => { + reportScrollManager.scrollToIndex(indexOfLinkedMessage, false); + }); + } + }, [indexOfLinkedMessage, reportScrollManager]); ```

## alternative solutions 02 ```diff // .src/pages/home/report/ReportActionsList.tsx#L664 ~~ onStartReached={() => {~~ - onStartReached(); + InteractionManager.runAfterInteractions(() => { + requestAnimationFrame(() => { + onStartReached(); + }); + }); ~~ }}~~ ```

POC ~~https://github.com/user-attachments/assets/bbc6ec35-706c-4faa-a8c4-31c4c54a39be~~ https://github.com/user-attachments/assets/6186f6f7-7a41-49c4-8f55-9a0f753edca6
alexpensify commented 1 month ago

@ikevin127 when you get a chance, can you review if this proposal fixes this issue? Thanks!

ikevin127 commented 1 month ago

@alexpensify Yes, will review today.

ikevin127 commented 1 month ago

@huult's proposal looks good to me. The suggested root cause is debatable in the sense that it is true if we're looking at implementing the main solution but it's not exactly answer to the "why?" is the issue currently occuring, what's causing the RHP to scroll to jump to the bottom of the report chat after browser refresh.

Given that the proposed main solution fixes our issue, I will accept the RCA as a package with the main solution because I approve of the functionality the solution is implementing.

Note for PR (if hired): If you watch the solution testing videos attached below, you can notice that after the refresh, while the highlighted message does stay in place, looks like there's some scroll jumping happening. This is not a requirement as it's slightly out of scope for this issue but it would be a nice to have if we could fix that scroll jump, specifically targeted for this case (RHP) such that it wouldn't affect any other parts of the ReportActionsList component.

Videos | Test 1 | Test 2 | | ------------- | ------------- | |

πŸŽ€πŸ‘€πŸŽ€Β C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

melvin-bot[bot] commented 1 month ago

πŸ“£ @huult πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

huult commented 1 month ago

Thank you for everything. I will create a pull request for this ticket tonight.

Pujan92 commented 1 month ago

The report action list is missing the initialScrollIndex in the InvertedFlatList.

@huult, I was just wondering if this is the issue then how it is working for the central pane ReportActionsList

huult commented 1 month ago

Proposal updated

Hi @ikevin127 , While creating the pull request, I found a better solution. The pull request is already open. Can you check it? Thanks!

ikevin127 commented 1 month ago

Thanks @Pujan92!

I was just wondering if this is the issue then how it is working for the central pane ReportActionsList

@huult This is a valid question when it comes to the RCA, which I wasn't able to figure out.

I checked out your updated proposal and to be honest I'm reluctant when it comes to adding more useEffects / timeouts in a component which is already complex because these can degrade performance.

I think the best way to go here would be to dig into why the link / scroll to message works when the list is in central pane but not when it's in RHP.

If you're wiling to dig deeper and update your proposal to expand on the RCA and fix the issue such that it would work in RHP as well please let us know - otherwise we can re-open this for proposals πŸ‘

huult commented 1 month ago

Proposal updated

@ikevin127, I've updated the proposal. Can you help check it again? Thank you!

ikevin127 commented 1 month ago

♻️ Reviewing updated proposal.

ikevin127 commented 1 month ago

@huult 🟒 The latest proposal update, RCA and solution are the perfect fix here, I tested it and can confirm that it works as expected without any scroll jumps, works exactly like on the central pane on browser refresh πŸ‘

You can proceed to implementing the latest solution in the opened PR and tag me there once it's ready for review.

Notes for PR:

alexpensify commented 1 month ago

@ikevin127 are we waiting for an update here? Sorry, I don't see the extra PR. Thanks!

ikevin127 commented 1 month ago

@alexpensify PR https://github.com/Expensify/App/pull/49477 for this issue was just deployed to production, no extra PR needed. ⚠️ Automation failed, this should be on [HOLD for Payment 2024-10-04] according to today's production deploy from https://github.com/Expensify/App/pull/49477#issuecomment-2379773426.

alexpensify commented 1 month ago

Thank you for flagging the payment date and clarifying the status.

alexpensify commented 1 month ago

Payouts due: 10-04-24

Upwork job is here.

Closing - I believe that I sent the payment via Upwork, but something looks off in Upwork. Please reply if you didn't get the payment and go through the payment process again.

ikevin127 commented 1 month ago

@alexpensify I only got $125, was that on purpose or a mistake ?

Screenshot 2024-10-04 at 15 24 13
alexpensify commented 1 month ago

No, there was an issue in Upwork, so I input a Bonus too. Let me know if that second payment didn't go through. Here is what I see on my side:

2024-10-04_15-28-31

ikevin127 commented 1 month ago

@alexpensify Indeed, Upwork is acting weird today (reported in other issues too).

So far nothing changed, I got $125 for this issue, looks same as https://github.com/Expensify/App/issues/48808#issuecomment-2394744681 and in Overwiew I only see the $125 transaction:

Screenshot 2024-10-04 at 15 33 05

Contract details look like this right now:

Screenshot 2024-10-04 at 15 33 33
alexpensify commented 1 month ago

Ok, I have a note to check again on Monday. Maybe there is a delay or something on the Upwork side. All my payment states are not updating correctly in Upwork today. If there is still an issue on Monday, I'll reach out to their support since I input a Bonus to correct the price issue.

ikevin127 commented 1 month ago

Understood!

For context: the same thing happened in this issue: https://github.com/Expensify/App/issues/48161 and the BZ wasn't able to do anything with the payment once the initial contract was ended without me being paid anything, so I'm assuming that your additional $125 bonus did not go through once the initial contract was ended after the $125 payment.

In the other issue, the BZ sent me another contract saying:

No idea what happened there. It doesn't show as being paid either. I'll send you another offer, but if you happen to get a rogue $125 show up - I'll trust your honesty to let me know. :)

To which I replied with:

Sure, I accepted the new contract and will let you know in case it comes twice πŸ‘

huult commented 1 month ago

@alexpensify , Can you also check for me on Upwork? My contract for this ticket has ended, but the status on Upwork is still 'work in progress. thank you

ikevin127 commented 1 month ago

@alexpensify I still did not get the other $125, only got half of the payment for this issue so far.

alexpensify commented 1 month ago

Thanks for these updates! There is an issue an Upwork that is being investigated to address the payment process. I'm going to hold until there is a fix. Convo: https://expensify.slack.com/archives/C01GTK53T8Q/p1728319262830679

alexpensify commented 1 month ago

@huult and @ikevin127 - our team figured out the Upwork bug, and I tried to complete the payment via another payment path in Upwork. Their support team is working on a permanent fix for the bug. I'd really appreciate it if you could confirm today's payment was sent successfully. Thanks!

ikevin127 commented 1 month ago

βœ… Confirmation that I just got the other $125, thanks Al !

huult commented 1 month ago

It was sent to me. Thank you, @alexpensify

alexpensify commented 1 month ago

Awesome, thank you for confirming! This bug is a wild one on the Upwork side, so I appreciate your patience.