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.52k stars 2.87k forks source link

[$250] After hold and unhold expense page is not scrolling down to comments section #49959

Open m-natarajan opened 1 month ago

m-natarajan 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.41-6 Reproducible in staging?: Y Reproducible in production?: Y 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 Expensify/Expensify Issue URL: Issue reported by: @dukenv0307 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1727724158339229

Action Performed:

  1. Go to a workspace
  2. Submit an expense
  3. Go to the expense detail page and click on the header
  4. Hold this expense
  5. Unhold this expense

    Expected Result:

    After hold and unhold expense, it is scroll to bottom

    Actual Result:

    After hold and unhold expense, it isn't scrolling to the comment section

    Workaround:

    Unknown

    Platforms:

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

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/398342a7-aaad-495f-9737-de73d1e94378

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021841168390578719829
  • Upwork Job ID: 1841168390578719829
  • Last Price Increase: 2024-10-29
  • Automatic offers:
    • dukenv0307 | Reviewer | 104747445
    • nkdengineer | Contributor | 104747448
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @dukenv0307
melvin-bot[bot] commented 1 month ago

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

nkdengineer commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-11 04:44:02 UTC.

Proposal

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

After hold and unhold expense is not scrolling down

What is the root cause of that problem?

After hold and unhold expense, we don't have any logic to scroll

https://github.com/Expensify/App/blob/96d897306f02f9a2f2cc71ae85f05889b593a3b8/src/libs/actions/IOU.ts#L7924

https://github.com/Expensify/App/blob/96d897306f02f9a2f2cc71ae85f05889b593a3b8/src/libs/actions/IOU.ts#L8025

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

We should use Report.notifyNewAction in 2 function unholdRequest and putOnHold to scroll to the bottom of this report like we did in other places

    API.write(
        'HoldRequest',
    ...
    );
    const report = ReportUtils.getReport(reportID);
    const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '-1', report?.parentReportActionID ?? '-1');
    const currentReportID = ReportUtils.isOneTransactionThread(report?.reportID ?? '', report?.parentReportID ?? '', parentReportAction) ? report?.parentReportID : reportID ;

    Report.notifyNewAction(currentReportID, userAccountID);
    API.write(
        'UnHoldRequest',
    ...
    );
    const report = ReportUtils.getReport(reportID);
    const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '-1', report?.parentReportActionID ?? '-1');
    const currentReportID = ReportUtils.isOneTransactionThread(report?.reportID ?? '', report?.parentReportID ?? '', parentReportAction) ? report?.parentReportID : reportID ;

    Report.notifyNewAction(currentReportID, userAccountID);

What alternative solutions did you explore? (Optional)

dukenv0307 commented 1 month ago

@sakluger I report this bug so I can help take it as C+

huult commented 1 month ago

Proposal

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

After hold and unhold expense page is not scrolling down to comments section

What is the root cause of that problem?

We haven't handled scrolling to the bottom when HOLD or UNHOLD changes yet

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

We should check if sortedVisibleReportActions has changed to include HOLD or UNHOLD in order to determine if we need to scroll to the bottom. Something like this:

//.src/pages/home/report/ReportActionsList.tsx#L203
+const previousReport = useRef<OnyxTypes.ReportAction[]>([]);
+ useEffect(() => {
+        if (!previousReport.current && Array.isArray(sortedVisibleReportActions) && sortedVisibleReportActions.length) {
+            previousReport.current = sortedVisibleReportActions;
+            return;
+        }

+        if (Array.isArray(sortedVisibleReportActions) && sortedVisibleReportActions.length) {
+            const previousActions = previousReport.current || [];

+            const newActions = sortedVisibleReportActions.filter((newAction) => !previousActions.some((prevAction) => +prevAction.reportActionID === newAction.reportActionID));
+            const containsHoldOrUnhold = newActions.some((action) => action?.actionName === 'HOLD' || action?.actionName + === 'UNHOLD');

+            if (containsHoldOrUnhold) {
+                InteractionManager.runAfterInteractions(() => {
+                    requestAnimationFrame(() => {
+                        reportScrollManager.scrollToBottom();
+                    });
+                });
+            }

+            previousReport.current = sortedVisibleReportActions;
+        }
+    }, [reportScrollManager, sortedVisibleReportActions]);
POC https://github.com/user-attachments/assets/00964595-bdee-4c38-b8f8-2b69b3d1bb62
melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sakluger commented 1 month ago

Thanks @dukenv0307!

raza-ak commented 1 month ago

Proposal

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

After hold and unhold expense page is not scrolling down to comments section

What is the root cause of that problem?

The "ReportActionsList.tsx" component does not currently handle auto-scrolling when new messages or comments are added. As a result, the page does not scroll to the latest comment when an expense is held or unheld.

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

We will update the "ReportActionsList.tsx" component to detect when a new message arrives and implement automatic scrolling to the latest comment whenever an expense is held or unheld. This will ensure that the page always scrolls down to display the newest message.

image

Video:

https://github.com/user-attachments/assets/8421408f-6f37-4b8a-96ec-45938231dc87

dukenv0307 commented 1 month ago

I'm reviewing...

sakluger commented 4 weeks ago

@dukenv0307 any updates?

dukenv0307 commented 4 weeks ago

@sakluger I'll update in a few hours. The first proposal looks promising, but sometime it doesn't work

dukenv0307 commented 4 weeks ago

@nkdengineer I tested your proposal, but sometime it doesn't work, especially when there's only one money request. Can you check again?

melvin-bot[bot] commented 4 weeks ago

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

nkdengineer commented 3 weeks ago

@dukenv0307 i'll update today

huult commented 3 weeks ago

@dukenv0307 Could you please review my proposal and POC? It is working as expected.

dukenv0307 commented 3 weeks ago

@huult

We should check if sortedVisibleReportActions has changed to include HOLD or UNHOLD in order to determine if we need to scroll to the bottom. Something like this:

I don't think it's a good idea, when users add a new comment, the list automatically scrolls to the bottom without checking sortedVisibleReportActions has new action. We should do the same as Add comment

raza-ak commented 3 weeks ago

@dukenv0307 Did you check my proposal?

dukenv0307 commented 3 weeks ago

@raza-ak We don't need to do that for every new message. For example the whisper message, new message from others, ...

nkdengineer commented 3 weeks ago

@dukenv0307 i looks like with one transaction thread, the reportID in this function is the reportID of the transaction https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/libs/actions/IOU.ts#L7906 and the reportID in here is the reportID of transaction thread https://github.com/Expensify/App/blob/4c90d62a6a56ff0000031eceeb9195d30a041a8a/src/pages/home/report/ReportActionsList.tsx#L371 so when we call notifyNewAction it can't run callback function scrollToBottomForCurrentUserAction

updated proposal

sakluger commented 3 weeks ago

We're still reviewing proposals.

dukenv0307 commented 3 weeks ago

@nkdengineer It still doesn't work on my side. Can you take a look at this?

melvin-bot[bot] commented 3 weeks ago

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

nkdengineer commented 2 weeks ago

@dukenv0307 i'm investigating

sakluger commented 2 weeks ago

Not overdue.

melvin-bot[bot] commented 2 weeks ago

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

Anaslancer commented 2 weeks ago

@dukenv0307 Finally, I found the reason. After change my code, It works on my side as expect.

https://github.com/user-attachments/assets/21f008a6-1a1b-43b4-8ebf-f5becd4ab4b9

melvin-bot[bot] commented 2 weeks ago

📣 @Anaslancer! 📣 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. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
Anaslancer commented 2 weeks ago

I will post my proposal soon.

Anaslancer commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-24 11:45:09 UTC.

Proposal

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

After hold and unhold expense is not scrolling down

What is the root cause of that problem?

There are no logic for notifying actions after hold and unhold expense.

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

We should add the notifyNewAction at the last of putOnHold and unholdRequest functions.

    const report = ReportUtils.getReport(reportID);
    Report.notifyNewAction(report?.parentReportID ?? '-1', createdReportAction.actorAccountID);

What alternative solutions did you explore? (Optional)

N/A

Contributor details

Your Expensify account email: anasup1995@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

melvin-bot[bot] commented 2 weeks ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

dukenv0307 commented 2 weeks ago

@Anaslancer Pls write the detail RCA why If condition is wrong, why reportScrollManager.scrollToBottom(); this code is not working as expect. ?

BTW, using setTimeout is not a good idea

Anaslancer commented 2 weeks ago

@dukenv0307 ,

  1. when hasNewestReportAction is true, the result list must be scroll down.
  2. reportScrollManager.scrollToBottom();, Seems like this function is called before rendering all results. Maybe it's a react-native bug. So I think we should delay for a few milliseconds. Please let me know your idea. Thanks

BTW, using setTimeout is not a good idea I agree but we used this sometimes in the code.

melvin-bot[bot] commented 2 weeks ago

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

dukenv0307 commented 1 week ago

@Anaslancer reportScrollManager.scrollToBottom() works well when you add the new message

Seems like this function is called before rendering all results

Anaslancer commented 1 week ago

@dukenv0307 when adding the new message, we are calling the other logic. https://github.com/Expensify/App/blob/d20c564464510262be8f1e6769a98787c5fee73c/src/pages/home/report/ReportActionsList.tsx#L345-L362 We can use InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom()); instead setTimeout in useEffect. I tested and worked as expect.

Anaslancer commented 1 week ago

Hi @dukenv0307 , I updated my proposal.

Anaslancer commented 1 week ago

Hi @dukenv0307 Did you have a chance to review my proposal again? I've fixed the setTimout problem.

dukenv0307 commented 1 week ago

@Anaslancer Can you share the evidence on the iou report (single request money report)? Thanks

Anaslancer commented 1 week ago

@dukenv0307 Can you please tell me where I can make a single request money report?

Anaslancer commented 1 week ago

https://github.com/user-attachments/assets/ff9cce99-159d-48f1-9fae-a17b01073f35

@dukenv0307 Is it correct?

dukenv0307 commented 1 week ago

@Anaslancer We shouldn't change the current logic except if you find the problem about it.

https://github.com/user-attachments/assets/43b86917-3f1b-431c-87fe-be81c1433b0c

Anaslancer commented 1 week ago

@dukenv0307 What problem should I find?

dukenv0307 commented 1 week ago

@Anaslancer As you can see in the video above, user 2 sends message to user 1 so user 1 is scrolled to the bottom automatically -> that is not correct ( you can conpare to the latest main)

Anaslancer commented 1 week ago

@dukenv0307 We can change the if conditions again. I think we don't need to use the hasNewestReportAction variable in the conditions.

if (
    scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
    previousLastIndex.current !== lastActionIndex &&
    reportActionSize.current < sortedVisibleReportActions.length
) {
    ...
}

Then it works as expect. Please see this video.

https://github.com/user-attachments/assets/7f68e69d-7e8a-4b87-94ac-7ac3279337d3

dukenv0307 commented 1 week ago

@Anaslancer When we want to change something, we need to know what and why is. Then explain the reason we should update it. Can you please elaborate?

melvin-bot[bot] commented 1 week ago

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

Anaslancer commented 1 week ago

@dukenv0307 When adding a new value into FlatList, the reportScrollManager will be changed. ATM, this logic will be called. https://github.com/Expensify/App/blob/038b21b710985e961a4816f6fea62edd395f03f7/src/pages/home/report/ReportActionsList.tsx#L372-L389 In here(^^^), if the hasNewestReportActionRef.current is true, then scrollToBottom is called. So we don't need to do same action at here I think. https://github.com/Expensify/App/blob/038b21b710985e961a4816f6fea62edd395f03f7/src/pages/home/report/ReportActionsList.tsx#L322-L333

dukenv0307 commented 6 days ago

When adding a new value into FlatList, the reportScrollManager will be changed. ATM, this logic will be called.

scrollToBottomForCurrentUserAction will be called only when notifyNewAction is triggered

sakluger commented 4 days ago

It looks like proposals are still being discussed and refined.

sakluger commented 4 days ago

I didn't mean to set this overdue. Commenting again to clear the overdue label.