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.33k stars 2.76k forks source link

[$250] Workspace - Long loading and errors after a new non USD WS offline and adding a bank account #41431

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 4 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: 1.4.69.0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4529975&group_by=cases:section_id&group_id=283225&group_order=asc Issue reported by: Applause - Internal Team

Action Performed:

Precondition: Your default workspace currency shouldn't be USD

  1. Log in with a new expensifail account
  2. Go offline
  3. Create a workspace
  4. Enable Workflows in the WS settings
  5. Go online
  6. Start adding a bank account
  7. Click on "Update to USD"

Expected Result:

The loading shouldn't take long and there shouldn't be any errors while adding a bank account

Actual Result:

After clicking on "Connect bank account", the loading takes up to 15 seconds or sometimes it's infinite. "Hmm it's not here" error appears briefly when it loads and the user clicks somewhere to close the "Connect bank account" modal.

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/d9291ee1-a600-4b9e-9788-125f3c266d90

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0177c40c7be0407379
  • Upwork Job ID: 1788976368546947072
  • Last Price Increase: 2024-07-05
Issue OwnerCurrent Issue Owner: @hoangzinh
melvin-bot[bot] commented 4 months ago

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

github-actions[bot] commented 4 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.
lanitochka17 commented 4 months ago

@AndrewGable 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 4 months ago

We think that this bug might be related to #wave-collect - Release 1

francoisl commented 4 months ago

I'm also seeing a delay on production, I think it's just because there are a bunch of sequential API requests to finish before the view can be shown (also there's some crazy flickering, looks like it's due to rerenders; but that's unrelated)

https://github.com/Expensify/App/assets/2229301/8f159d93-bade-4b13-9c15-e08cd307f7af

melvin-bot[bot] commented 4 months ago

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

AndrewGable commented 4 months ago

Agree with @francoisl if it's on production let's look at it like a normal bug.

melvin-bot[bot] commented 4 months ago

@JmillsExpensify, @AndrewGable Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 4 months ago

@JmillsExpensify, @AndrewGable Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 4 months ago

@JmillsExpensify, @AndrewGable Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

nkdengineer commented 4 months ago

Proposal

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

After clicking on "Connect bank account", the loading takes up to 15 seconds or sometimes it's infinite. "Hmm it's not here" error appears briefly when it loads and the user clicks somewhere to close the "Connect bank account" modal.

What is the root cause of that problem?

When CreateWorkspace API is called, there are some gaps onyx updates between the client and the given server updates and then the queue is pause before we call EnablePolicyWorkflows API.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/Network/SequentialQueue.ts#L151

After we handle the gap, the queue is unpause and we call flushOnyxUpdatesQueue here without checking the persisted requests is empty or not and then areWorkflowsEnabled is override as false

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/OnyxUpdateManager/index.ts#L66

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/Network/SequentialQueue.ts#L151

Another problem that makes areWorkflowsEnabled is override is when the queue is pause, the process function returns a resolve promise and then resolveIsReadyPromise is called here which makes openPolicyMoreFeaturesPage is called before EnablePolicyWorkflows API is called

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/Network/SequentialQueue.ts#L132

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

We should check the persisted requests list is empty or not before we resolve IsReady promise here

if (PersistedRequests.getAll().length === 0) {
    resolveIsReadyPromise?.();
}

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/Network/SequentialQueue.ts#L132

and flushOnyxUpdatesQueue here

if (numberOfPersistedRequests === 0) {
    flushOnyxUpdatesQueue();
}

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/Network/SequentialQueue.ts#L151 https://github.com/Expensify/App/blob/52d4f31aeb9436ee009f862ec9bbde134cad84b8/src/libs/PolicyUtils.ts#L319

What alternative solutions did you explore? (Optional)

NA

melvin-bot[bot] commented 4 months ago

@JmillsExpensify, @AndrewGable, @hoangzinh Whoops! This issue is 2 days overdue. Let's get this updated quick!

JmillsExpensify commented 4 months ago

@hoangzinh thoughts on the proposal above?

melvin-bot[bot] commented 4 months ago

@JmillsExpensify @AndrewGable @hoangzinh 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!

hoangzinh commented 4 months ago

@nkdengineer I think your RCA is not correct. I recall API write requests would be queued Onyx updates to avoid replay effect https://github.com/Expensify/App/blob/0424a94e8c0c6d635429b812d6b0140b73ad8645/src/libs/actions/OnyxUpdates.ts#L28-L30

Then update once here https://github.com/Expensify/App/blob/0424a94e8c0c6d635429b812d6b0140b73ad8645/src/libs/Network/SequentialQueue.ts#L134

Can you check why it's not applied in this case? Thanks

nkdengineer commented 4 months ago

@hoangzinh Thanks for your information, I updated the RCA and solution https://github.com/Expensify/App/issues/41431#issuecomment-2105015885

melvin-bot[bot] commented 4 months ago

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

hoangzinh commented 3 months ago

When CreateWorkspace API is called, there are some gaps onyx updates between the client and the given server updates and then the queue is pause before we call EnablePolicyWorkflows API.

Hi @nkdengineer thanks for the updates. Can you point me to the LOC where the queue is paused before we call EnablePolicyWorkflows API.?

Moreover, your solution might cause a regression where the network is offline, we won't trigger resolveIsReadyPromise?.()

nkdengineer commented 3 months ago

Can you point me to the LOC where the queue is paused before we call EnablePolicyWorkflows API.?

@hoangzinh when we create a new workspace, we are missing some updates on our local storage and then the queue is pause here.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/OnyxUpdates.ts#L121-L130

hoangzinh commented 3 months ago

@nkdengineer can you clarify more details about what're we missing in our local storage in this case?

hoangzinh commented 3 months ago

When I traced the log, I found that the pausing queue come from a Pusher event

Screenshot 2024-05-23 at 00 21 46

nkdengineer commented 3 months ago

@hoangzinh Yes, this come from pusher here. Whenever we create a new workspace, we receive pusher data which makes the queue is pause and ONYXKEYS.ONYX_UPDATES_FROM_SERVER is changed

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/User.ts#L601 https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/OnyxUpdates.ts#L128

and then we handle the update gap here to sync the data from server.

https://github.com/Expensify/App/blob/e8ae3c5acedf0e6788dc574c7b6f3043ca37092a/src/libs/actions/OnyxUpdateManager/index.ts#L155

melvin-bot[bot] commented 3 months ago

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

hoangzinh commented 3 months ago

@nkdengineer Actually I realize that there 2 are times that it makes SequentialQueue pause. And the time related to our bug seems the OpenReimbursementAccountPage API

Screenshot 2024-05-24 at 23 01 01

Btw can you take a look at RCA again and my feedback about your solution here? Thanks

nkdengineer commented 3 months ago

@nkdengineer Actually I realize that there 2 are times that it makes SequentialQueue pause. And the time related to our bug seems the OpenReimbursementAccountPage API

@hoangzinh Thanks for your feedback, the not found page still appears even we don't click on Connect bank account which makes OpenReimbursementAccountPage is called.

Moreover, your solution might cause a regression where the network is offline, we won't trigger resolveIsReadyPromise?.()

For that, we can add the check offline to triggerresolveIsReadyPromise?.()

if (NetworkStore.isOffline() || PersistedRequests.getAll().length === 0) {
    resolveIsReadyPromise?.();
}
hoangzinh commented 3 months ago

but the PusherEvent that causes SequentialQueue pause happens before we call CreateWorkspace

Screenshot 2024-05-24 at 23 58 25
nkdengineer commented 3 months ago

@hoangzinh I tested and sometimes the PusherEvent happens before CreateWorkspace, sometimes it happens after CreateWorkspace. I think it's based on backend and network. So summary of the issue's RCA: The bug will happen if a PusherEvent comes between CreateWorkspace and EnablePolicyWorkflows API makes the queue is paused and then a read API is called before EnablePolicyWorkflows API which overrides the policy data. What do you think?

hoangzinh commented 3 months ago

So summary of the issue's RCA: The bug will happen if a PusherEvent comes between CreateWorkspace and EnablePolicyWorkflows API makes the queue is paused and then a read API is called before EnablePolicyWorkflows API which overrides the policy data

Nope, I don't think so. Let's take a look at my recording below, I can reproduce this bug with PusherEvent (that would pause the queue) happens before CreateWorkspace and EnablePolicyWorkflows API

https://github.com/Expensify/App/assets/9639873/014d893b-716d-4cd8-8c92-d7667894eff9

nkdengineer commented 3 months ago

@hoangzinh Thanks for your feedback, I checked again, the queue is paused after CreateWorkspace API is called and before CreateWorkspace is complete. But the queue is unpaused after CreateWorkspace API is complete which makes resolveIsReadyPromise is resolved after CreateWorkspace API is complete and then the read API is called immediately

https://github.com/Expensify/App/blob/ff7ba40d79871079666ef26cd0f22706d4882bb8/src/libs/Network/SequentialQueue.ts#L132

https://github.com/Expensify/App/blob/ff7ba40d79871079666ef26cd0f22706d4882bb8/src/libs/actions/OnyxUpdateManager/index.ts#L144

Screenshot 2024-05-29 at 13 34 14
melvin-bot[bot] commented 3 months ago

@JmillsExpensify @AndrewGable @hoangzinh this issue is now 4 weeks old, please consider:

Thanks!

hoangzinh commented 3 months ago

@nkdengineer I guess the reason why when the SequentialQueue is paused, we would like to update all OnxyUpdateQueue to prevent it would overwriting data from API GetMissingOnyxMessages later. If our solution prevents updating all OnxyUpdateQueue, that would cause unknown regressions.

the queue is paused after CreateWorkspace API is called and before CreateWorkspace is complete

Is there any way that we can prevent it happens?

melvin-bot[bot] commented 3 months ago

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

mvtglobally commented 3 months ago

Issue not reproducible during KI retests. (First week)

nkdengineer commented 3 months ago

I guess the reason why when the SequentialQueue is paused, we would like to update all OnxyUpdateQueue to prevent it would overwriting data from API GetMissingOnyxMessages later. If our solution prevents updating all OnxyUpdateQueue, that would cause unknown regressions.

@hoangzinh Our solution will only prevent updating the OnyxUpdate of write API if we still have some write API in the queue.

GetMissingOnyxMessages API is a read API and the OnyxData of this will update to Onyx immediately

https://github.com/Expensify/App/blob/90dee7accae79c49debf30354c160cab6c52c423/src/libs/actions/OnyxUpdates.ts#L30

Is there any way that we can prevent it happens?

I think we can't because it depends on when BE sends the pusher data.

melvin-bot[bot] commented 3 months ago

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

wildan-m commented 3 months ago

Proposal

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

Workspace - Long loading and errors after a new non USD WS offline and adding a bank account

What is the root cause of that problem?

Policy is not ready to use when we connect a bank account. There are too many fields missing in the optimistic data.

Compare Policy Object. Left (Offline), Right (Online and policy creation completed) ![AwesomeScreenshot-text-compare--2024-06-10_4_58_part1](https://github.com/Expensify/App/assets/11847279/2adf98bc-a254-4aba-a978-2e22c83d6388) ![AwesomeScreenshot-text-compare--2024-06-10_4_58_part2](https://github.com/Expensify/App/assets/11847279/6aefe8f0-b420-422a-80e8-1debb6fcaff6)

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

Better to wait until workspace created to show bank account page. Change this check to:

    const isLoading = (!!isLoadingApp || !!account?.isLoading || isReimbursementAccountLoading || policy?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) && (!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT);

Or

    const isLoading = (!!isLoadingApp || !!account?.isLoading || isReimbursementAccountLoading || policy?.pendingAction) && (!plaidCurrentEvent || plaidCurrentEvent === CONST.BANK_ACCOUNT.PLAID.EVENTS_NAME.EXIT);

What alternative solutions did you explore? (Optional)

Add missing required values to optimistic data, but IMO this approach will require a lot of trial errors to find the required values

hoangzinh commented 3 months ago

Hi @wildan-m, does your proposal work when network is offline?

wildan-m commented 3 months ago

@hoangzinh I can't reproduce it anymore, seems we can close it

hoangzinh commented 3 months ago

I couldn't reproduce this issue either. @JmillsExpensify can you ask QA to test this issue again before we close it? Thanks

melvin-bot[bot] commented 3 months ago

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

hoangzinh commented 3 months ago

Waiting to re-test again.

melvin-bot[bot] commented 2 months ago

@JmillsExpensify, @AndrewGable, @hoangzinh Whoops! This issue is 2 days overdue. Let's get this updated quick!

JmillsExpensify commented 2 months ago

Asked QA to test again.

kavimuru commented 2 months ago

Tester is still able to reproduce.

https://github.com/Expensify/App/assets/43996225/7a842bd5-c5bd-49b4-b2ca-0847c956a53e

melvin-bot[bot] commented 2 months 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 2 months ago

@JmillsExpensify, @AndrewGable, @hoangzinh Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 months ago

@JmillsExpensify, @AndrewGable, @hoangzinh 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

JmillsExpensify commented 2 months ago

QA is able to reproduce so keeping this open.