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] Request Money - Selecting and unselecting an account when requesting money creates a delay #27949

Closed
4 of 6 tasks
kbecciv opened this issue Sep 21, 2023 · 20 comments
Closed
4 of 6 tasks
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 Sep 21, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open request money
  2. Change the tab to manual
  3. Enter amount and click next
  4. Select any account
  5. Uselect the account

Expected Result:

The app should not experience delays when it is unselected, and it should work smoothly

Actual Result:

The app delays when it is selected and unselected

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.72.8
Reproducible in staging?: y
Reproducible in production?: n
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
Notes/Photos/Videos: Any additional supporting documentation

delay.mp4
Recording.4666.mp4

Expensify/Expensify Issue URL:
Issue reported by: @jo-ui
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695301255308139

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0171404337e9a907ca
  • Upwork Job ID: 1709600531889336320
  • Last Price Increase: 2023-10-04
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Sep 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@OSBotify
Copy link
Contributor

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

@thienlnam thienlnam removed the DeployBlockerCash This issue or pull request should block deployment label Sep 22, 2023
@thienlnam
Copy link
Contributor

Removing the deploy blocker as it is a minor issue

@s-alves10
Copy link
Contributor

s-alves10 commented Sep 22, 2023

Proposal

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

Toggling accounts in request money participants select page creates noticeable delay

What is the root cause of that problem?

When we toggle an account, addParticipantToSelection function is called.

We calculate the new selected participants and notice parent by onAddParticipants callback.
And we calculate the new chat options and set state. This state update toggles the component re-render once

By onAddParticipants callback, participants props would change and the sections would be re-calculated and component is re-rendered. And because participants was changed, useEffect callback is called and setNewChatOptions is called again. This causes the re-render once more.

So 3 times of re-rendering is done
This is the root cause

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

We can reduce the re-render by using useMemo instead of useState and useEffect

Combine two code segments(state and effect) into one using useMemo like below

    const newChatOptions = useMemo(() => {
        const chatOptions = OptionsListUtils.getFilteredOptions(
            reports,
            personalDetails,
            betas,
            searchTerm,
            participants,
            CONST.EXPENSIFY_EMAILS,

            // If we are using this component in the "Request money" flow then we pass the includeOwnedWorkspaceChats argument so that the current user
            // sees the option to request money from their admin on their own Workspace Chat.
            iouType === CONST.IOU.MONEY_REQUEST_TYPE.REQUEST,

            // We don't want to include any P2P options like personal details or reports that are not workspace chats for certain features.
            !isDistanceRequest,
        );
        return {
            recentReports: chatOptions.recentReports,
            personalDetails: chatOptions.personalDetails,
            userToInvite: chatOptions.userToInvite,
        };
    }, [betas, reports, participants, personalDetails, searchTerm, iouType, isDistanceRequest]);

And remove the redundant code

This causes the re-render only once

This works great and fixes another issue below

27949_another_issue.mp4
Result
27949.mp4

What alternative solutions did you explore? (Optional)

@bondydaa bondydaa added the Bug Something is broken. Auto assigns a BugZero manager. label Sep 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@bondydaa
Copy link
Contributor

okay if this isn't a blocker lets get it through the normal BZ process then.

@bondydaa bondydaa added Daily KSv2 and removed Hourly KSv2 labels Sep 22, 2023
@melvin-bot melvin-bot bot added the Overdue label Sep 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 26, 2023

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

@bondydaa
Copy link
Contributor

hax is going on sabbatical so going to redo the BZ assignment.

@melvin-bot melvin-bot bot removed the Overdue label Sep 27, 2023
@bondydaa bondydaa added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot melvin-bot bot added the Overdue label Sep 29, 2023
@bondydaa
Copy link
Contributor

bondydaa commented Oct 1, 2023

@greg-schroeder can you run through the BZ steps and confirm if this still exists and/or if you think it's worthwhile?

@melvin-bot melvin-bot bot removed the Overdue label Oct 1, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 4, 2023
@greg-schroeder
Copy link
Contributor

I tested it and it's still a thing. I mean I don't think it's a huge deal but it's bad performance for sure - it's noticeably slow.

@melvin-bot melvin-bot bot removed the Overdue label Oct 4, 2023
@greg-schroeder greg-schroeder added Overdue External Added to denote the issue can be worked on by a contributor and removed Overdue labels Oct 4, 2023
@melvin-bot melvin-bot bot changed the title Request Money - Selecting and unselecting an account when requesting money creates a delay [$500] Request Money - Selecting and unselecting an account when requesting money creates a delay Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@greg-schroeder
Copy link
Contributor

I will send this through the External flow. Hey @cubuspl42 - mind checking @s-alves10's proposal above?

@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

@cubuspl42 @bondydaa @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@s-alves10
Copy link
Contributor

s-alves10 commented Oct 5, 2023

This issue looks like fixed already

cc @cubuspl42

@greg-schroeder
Copy link
Contributor

Ah, could be the case. Perhaps we can close? I will let @cubuspl42 confirm that choice :)

@cubuspl42
Copy link
Contributor

I can't reproduce. Let's 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
None yet
Development

No branches or pull requests

8 participants