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

[$500] The expense chat doesn't scroll to bottom when we create a distance request #39198

Closed
1 of 6 tasks
m-natarajan opened this issue Mar 28, 2024 · 26 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@m-natarajan
Copy link

m-natarajan commented Mar 28, 2024

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: 1.4.57-2
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/p1711431374851699

Action Performed:

  1. Go to a WS chat
  2. Create a money request
  3. Open the expense chat of the request
  4. Send some messages or create other requests
  5. Scroll to the top of the report
  6. Create a distance request in this report
  7. After creating this request, notice the report

Expected Result:

The report should be scrolled to the bottom as it's when we create other requests like manual or scan

Actual Result:

The report isn't scrolled to the bottom

Workaround:

unknown

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

Screen.Recording.2024-03-26.at.12.36.28.mov
Recording.2917.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a3b77e7d3b5ad058
  • Upwork Job ID: 1775261893332221952
  • Last Price Increase: 2024-04-02
  • Automatic offers:
    • dukenv0307 | Reviewer | 0
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 28, 2024
Copy link

melvin-bot bot commented Mar 28, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 28, 2024

Proposal

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

The report isn't scrolled to the bottom

What is the root cause of that problem?

When we create a new distance request, we always notifyNewAction for the chat report. So if we create this in expense report, this doesn't scroll to bottom

Report.notifyNewAction(chatReport.reportID, userAccountID);

Actually, sometime when we open the report, the report doesn't scroll to bottom when we send any action. The RCA of this we're missing scrollToBottomForCurrentUserAction as dependency of this useEffect

}, [report.reportID]);

So if the time we open the report the last report action of the report isn't loaded, hasNewestReportAction is always false and then the report never scrolls to bottom.

const scrollToBottomForCurrentUserAction = useCallback(
(isFromCurrentUser: boolean) => {
// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
// they are now in the list.
if (!isFromCurrentUser || !hasNewestReportAction) {
return;
}
InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
},
[hasNewestReportAction, reportScrollManager],

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

  1. We should do the same way as we do in requestMoney here to get the active report and then pass this to notifyNewAction function

const activeReportID = isMoneyRequestReport ? report.reportID : chatReport.reportID;

const activeReportID = isMoneyRequestReport ? report.reportID : chatReport.reportID; 
Report.notifyNewAction(activeReportID, userAccountID); 

Report.notifyNewAction(chatReport.reportID, userAccountID);

  1. We should create a ref for hasNewestReportAction and use it in scrollToBottomForCurrentUserAction function
const hasNewestReportActionRef = useRef(hasNewestReportAction);
useEffect(() => {
    hasNewestReportActionRef.current = hasNewestReportAction;
}, [hasNewestReportAction])
const scrollToBottomForCurrentUserAction = useCallback(
    (isFromCurrentUser: boolean) => {
        // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
        // they are now in the list.
        if (!isFromCurrentUser || !hasNewestReportActionRef.current) {
            return;
        }
        InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
    },
    [reportScrollManager],
);

const scrollToBottomForCurrentUserAction = useCallback(

What alternative solutions did you explore? (Optional)

NA

@dukenv0307
Copy link
Contributor

@johncschuster I'm the reporter of this bug and have the context on this. I can take over this as C+ if the issue is external

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
@johncschuster
Copy link
Contributor

@dukenv0307 thanks for reporting this and volunteering to be the reviewer! Is "scrolling to the bottom of the report" a global behavior when other requests are created?

@melvin-bot melvin-bot bot removed the Overdue label Apr 1, 2024
@dukenv0307
Copy link
Contributor

@johncschuster Yes, when we create other requests like manual or scan, the report scrolls to the bottom after the request is created.

Screen.Recording.2024-04-02.at.09.45.56.mov

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Apr 2, 2024
@melvin-bot melvin-bot bot changed the title The expense chat doesn't scroll to bottom when we create a distance request [$500] The expense chat doesn't scroll to bottom when we create a distance request Apr 2, 2024
Copy link

melvin-bot bot commented Apr 2, 2024

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

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

melvin-bot bot commented Apr 2, 2024

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

@dukenv0307
Copy link
Contributor

@nkdengineer I tried your solution but it did not worked on my side. Beside, we can reproduce this bug with the following steps

  1. Go to WS chat
  2. Create money request
  3. Go to expense report
  4. Add new message or create money request
  5. Observer the expense chat is not scroll to bottom

Can you pls help take a look? Or Do I miss sth?

@nkdengineer
Copy link
Contributor

Actually, sometime when we open the report, the report doesn't scroll to bottom when we send any action. The RCA of this we're missing scrollToBottomForCurrentUserAction as dependency of this useEffect.
So if the time we open the report the last report action of the report isn't loaded, hasNewestReportAction is always false and then the report never scrolls to bottom.

@dukenv0307 Thanks for your feedback, I also can reproduce this. The RCA is here and I updated proposal #39198 (comment) with additional RCA and solution to fix this case.

@dukenv0307
Copy link
Contributor

@nkdengineer I don't like the way we pass scrollToBottomForCurrentUserAction to dependencies. Can we use the ref for hasNewestReportAction in scrollToBottomForCurrentUserAction function?

@nkdengineer
Copy link
Contributor

@dukenv0307 I think we can do that, updated proposal #39198 (comment) to use ref for hasNewestReportAction.

@dukenv0307
Copy link
Contributor

@nkdengineer's proposal looks good to me

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 4, 2024

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

@aldo-expensify
Copy link
Contributor

@nkdengineer A note on the proposed solution:

Instead of updating the ref in a useEffect, I think it is better if you update the ref right after this line:

const hasNewestReportAction = sortedReportActions?.[0].created === report.lastVisibleActionCreated;

so it is never out of sync.

I'm assuming that you recommended to use a ref and not add hasNewestReportAction as a dependency of scrollToBottomForCurrentUserAction because if we do that, we would have to deal with the subscribe/unsubscribe here each time scrollToBottomForCurrentUserAction changes thanks to hasNewestReportAction changing. Is this correct @dukenv0307 ?

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

melvin-bot bot commented Apr 4, 2024

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Apr 4, 2024

📣 @nkdengineer You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@dukenv0307
Copy link
Contributor

@aldo-expensify Yes we only want to subscribe/unsubscribe NewActionEvent when the reportID gets changed

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 5, 2024
@nkdengineer
Copy link
Contributor

@dukenv0307 The PR is here

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

This issue has not been updated in over 15 days. @johncschuster, @aldo-expensify, @dukenv0307, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Apr 29, 2024
@nkdengineer
Copy link
Contributor

@johncschuster The PR was deployed to production a while ago, could you help with payments here 🙇

@nkdengineer
Copy link
Contributor

@johncschuster Gentle bump on the comment above

@johncschuster
Copy link
Contributor

Working on this now

@johncschuster
Copy link
Contributor

Payment has been issued to @dukenv0307 via Upwork.
@nkdengineer can you accept the invite to the job, here?

@nkdengineer
Copy link
Contributor

@johncschuster Done! thanks

@johncschuster
Copy link
Contributor

Paid! Thanks for your patience!

@nkdengineer
Copy link
Contributor

@johncschuster We can close this issue now.

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. External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

5 participants