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

Web -Workspace-Blank space appear for a brief moment after refreshing the page #9159

Closed
kavimuru opened this issue May 24, 2022 · 9 comments
Closed
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2

Comments

@kavimuru
Copy link

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:

Precondition: Add VBA into your testing account

  1. Go to https://staging.new.expensify.com/
  2. Log in with any account ( I used High traffic account)
  3. Make sure the bank account added
  4. Go to any Workspace
  5. Select Reimburse expenses
  6. Refresh the page

Expected Result:

There is no blank space appeared for a brief moment after refresh

Actual Result:

Blank space appear for a brief moment after refresh

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • Android

Version Number: 1.1.66-0
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+0901abb@applause.expensifail.com / Feya87Katya
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Bug5584441_Recording__476.mp4
Bug5584441_Screen_Recording_20220524-154210_New_Expensify.mp4

Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

View all open jobs on GitHub

@kavimuru kavimuru added the DeployBlockerCash This issue or pull request should block deployment label May 24, 2022
@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.

@melvin-bot
Copy link

melvin-bot bot commented May 24, 2022

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

@francoisl
Copy link
Contributor

The blank space is almost unnoticeable on my personal account and also on web with the high traffic account applausetester+0901abb@applause.expensifail.com, I only notice it on mWeb.

I can't tell if it's a regression or a fix of some sort though - on production, the label "Unlock next-day reimbursements" appears as a placeholder until the view gets updated with "Fast reimbursements = happy members!". So I'm assuming there's something that makes a network request to know what exact message the user should be shown, and the staging app doesn't show anything until that network request has received a response.

Going to dig a bit more to verify this hypothesis though.

  • Production:
screen-20220524-154353.mp4
  • Staging:
screen-20220524-154334.mp4

@francoisl
Copy link
Contributor

There are 2 separate Get calls that are made when the user clicks Reimburse Expenses:

  • Get nameValuePairs with nvp name = expensify_freePlanBankAccountID,
  • Get nameValuePairs, bankAccountList with nvpNames=private_failedBankValidations_,expensify_migration_2020_04_28_RunKycVerifications,expensify_ACHData_throttled,private_throttledHistory_BankAccount_Get

And that hasn't changed on this version that is currently on staging. The only change is that we no longer show the wrong placeholder "Unlock next-day reimbursements", so I would lean on this not being a blocker or regression. Curious for other opinions though.

@amyevans
Copy link
Contributor

This was a purposeful fix for #9057. While I agree it's not the most optimal to have that brief delay while the ACH state is loading, I think it's an improvement over what was in place. Shouldn't be a deploy blocker at the least.

@Julesssss
Copy link
Contributor

Thanks both, I agree that this is an overall improvement and shouldn't hold up deployment. 👍

@Julesssss Julesssss added Daily KSv2 Improvement Item broken or needs improvement. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels May 25, 2022
@Julesssss
Copy link
Contributor

I guess now the question is: should we leave this open as an improvement? I'd say probably not, but happy either way.

@francoisl
Copy link
Contributor

I agree with not even keeping this as an improvement. The solutions I can think of would be:

  • Add a skeleton screen while the network requests are pending. However that sounds like a bigger project we might want to plan out in detail first, and potentially use those skeletons every time some UI components are drawn after a network request completes (and also would most likely be on hold until the Offline First changes are finished)
    Random example of what I'm talking about:
  • Load the NVPs and bankAccountList preemptively, and store them in Onyx - but I don't think that would be a suitable solution because we get NVPs like expensify_ACHData_throttled and private_throttledHistory_BankAccount_Get, which we probably don't want to cache in Onyx, and instead load every time.
  • ??

@francoisl
Copy link
Contributor

Going to close this since the root issue should be fixed now. We can consider adding skeleton UI later.

@melvin-bot melvin-bot bot removed the Overdue label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants