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

Display loading spinner while authenticating from validate code link #15311

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

cristipaval
Copy link
Contributor

@cristipaval cristipaval commented Feb 20, 2023

Details

This is applicable on web App only.

Fixed Issues

$ #15188

Tests and QA steps

  1. Start sign in process in a web browser
  2. Open the magic link in another tab on the same browser
  3. Verify that after the App launch, a loading spinner is shown while the browser is signed in, then Abracadabra page is displayed (on dev this takes very short and you might not see the loading spinner)
  • Verify that no errors appear in the JS console

Offline tests

N/A

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-21.at.10.30.59.mov
Mobile Web - Chrome
Screen.Recording.2023-02-21.at.10.32.33.mov
Mobile Web - Safari
Screen.Recording.2023-02-21.at.13.56.21.mov
Desktop

N/A

iOS

N/A

Android

N/A

@cristipaval cristipaval self-assigned this Feb 20, 2023
@cristipaval cristipaval marked this pull request as ready for review February 21, 2023 11:58
@cristipaval cristipaval requested a review from a team as a code owner February 21, 2023 11:58
@melvin-bot melvin-bot bot requested review from deetergp and thesahindia and removed request for a team February 21, 2023 11:58
@MelvinBot
Copy link

@thesahindia @deetergp One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@thesahindia
Copy link
Member

Hey @cristipaval, the magic code screen flashes at android mWeb before the "Abracadabra" screen

Screen.Recording.2023-02-22.at.1.04.29.AM.mov

I checked the video that you attached and the same screen appears in your video (but in your video it's really hard to see it)
Screenshot 2023-02-22 at 1 16 33 AM

I think we should also show a screen when user opens expired link because right now an endless loader appears but it's out of scope. Is there a plan to implement this or should I raise this in the channel?

@cristipaval
Copy link
Contributor Author

the magic code screen flashes at android mWeb before the "Abracadabra" screen

Is this reproducible on Android only?

I think we should also show a screen when user opens expired link because right now an endless loader appears but it's out of scope. Is there a plan to implement this or should I raise this in the channel?

This is a good catch. I'll create a new issue for this.

@thesahindia
Copy link
Member

I thought it was only reproducible at android mWeb but I can repro this at ios safari as well

@cristipaval
Copy link
Contributor Author

cristipaval commented Feb 23, 2023

Hey @thesahindia !

I can't reproduce it again. I even added a timeout here to make the api call longer, and I don't see that intermediary state.

Screenshot 2023-02-23 at 12 39 08

Screen.Recording.2023-02-23.at.12.43.06.mov

Could you please clear cache and make sure you run the right version of the App on your mobile browser?

@thesahindia
Copy link
Member

Same results even after clearing the cache. It's little weird that it's only reproducible at mWeb. Maybe it has something to do with the way I am testing?
I can't get the magic link on web locally so I am following these steps -

  1. Open ios app locally and enter email > continue > press "Didn't receive a magic code?"
  2. Copy the URL that you get in the mail
  3. Go to web/mWeb > open localhost> enter email > continue
  4. Open new tab > paste the URL and change it to localhost address > enter

I am not in the beta so I have changed the canUsePasswordlessLogins function to return true

@cristipaval
Copy link
Contributor Author

@thesahindia I don't think its related but why do you need the first step that you do in the iOS native App?
You just have to open the App in the mobile browser, sign out if you are already signed in, add your email, continue, and when you get the magic code link, copy-paste it in another tab on the mobile browser.

@thesahindia
Copy link
Member

I am not getting the magic link if I try it in the mWeb or web. If I remember correctly I get 403 forbidden response status.

@cristipaval
Copy link
Contributor Author

@deetergp Could you please test and check if this is reproducible on your end?

@deetergp
Copy link
Contributor

@cristipaval @thesahindia It seems to work fine on my end using Safari on iOS, but maybe I'm not doing the right reproduction steps?

Simulator.Screen.Recording.-.iPhone.13.-.2023-02-24.at.10.59.41.mp4

@deetergp
Copy link
Contributor

Look like I got both the platform and the steps wrong on that last one! 😅 I have retried it with the steps outlined in the description on mWeb in Android and still do not see the code screen flash on the way to telling me I am signed in.

2023-02-24_11-32-02.mp4

@cristipaval
Copy link
Contributor Author

@thesahindia Could you please make sure that you run the right App version on your simulator/emulator?

@thesahindia
Copy link
Member

@thesahindia Could you please make sure that you run the right App version on your simulator/emulator?

Yeah, I have verified it.

In your videos I saw you are using 000000 in the url, can you try reproducing this issue with the magic link that you get in the mail? (Not sure if it will make any difference) Also can you add my email (tempararywork03+006@gmail.com) to passwordless beta ? Just wanna check if there will be any difference after that.

@cristipaval
Copy link
Contributor Author

Hey @thesahindia ! Your email (tempararywork03+006@gmail.com) has now passwordless beta enabled.

@thesahindia
Copy link
Member

Oh it worked fine with the beta enabled account 👏

@thesahindia
Copy link
Member

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Screen.Recording.2023-02-27.at.7.16.00.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-02-27.at.6.44.23.PM.mov
Mobile Web - Safari
Screen.Recording.2023-02-27.at.6.55.22.PM.mov
Desktop

N/A

iOS

N/A

Android

N/A

@cristipaval
Copy link
Contributor Author

Thank you! All on you @deetergp

Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks!

@deetergp deetergp merged commit 99d5741 into main Mar 2, 2023
@deetergp deetergp deleted the cristi_instantly-show-abracadabra-page branch March 2, 2023 00:03
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 608.145 ms → 671.907 ms (+63.761 ms, +10.5%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 608.145 ms
Stdev: 16.175 ms (2.7%)
Runs: 574.5225430000573 583.4750159997493 585.2190759982914 590.9782720003277 594.3490399997681 594.550741000101 597.3421219997108 597.6123459991068 598.9291580002755 599.1181239988655 599.6653649993241 599.7182219997048 603.4468189999461 603.6697189994156 603.7191570010036 608.7461749985814 609.609781999141 609.8908699993044 610.5877280011773 610.7771819997579 612.5151370000094 612.6763920001686 618.8390699997544 620.4921070002019 621.347168000415 622.6069750003517 624.2788900006562 628.6006269995123 628.6348470002413 631.043863998726 655.5418300013989

Current
Mean: 671.907 ms
Stdev: 20.219 ms (3.0%)
Runs: 624.3761800006032 647.5738119985908 649.3343919999897 651.3104249984026 651.6786709986627 651.8149420004338 655.3366700001061 655.5332040004432 658.5392660014331 658.9710689987987 659.559081999585 662.4439300000668 663.2373460009694 664.3980310000479 664.7726639993489 666.0954180005938 666.6141769997776 668.2812510002404 672.1361899990588 673.8548179995269 676.3613689988852 683.089925000444 683.2329509984702 684.6148680001497 686.2908120006323 688.0627449993044 688.3533939998597 693.3139650002122 693.6463620010763 693.7989499997348 706.6765950005502 711.0615640003234 718.5522059984505

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 714.308 ms → 722.264 ms (+7.956 ms, +1.1%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +9.6%)
App start nativeLaunch 19.767 ms → 19.710 ms (-0.057 ms, ±0.0%)
App start runJsBundle 205.281 ms → 193.594 ms (-11.688 ms, -5.7%)
Show details
Name Duration
App start TTI Baseline
Mean: 714.308 ms
Stdev: 28.730 ms (4.0%)
Runs: 645.2964180000126 664.6039579994977 664.7265779990703 680.0239279996604 682.7397700008005 689.8763130009174 693.6968629993498 696.7691939994693 699.497344000265 700.9676789995283 702.5340429991484 704.6905049998313 706.8467570003122 710.8792509995401 717.6198950000107 719.8772860001773 719.891569999978 719.956615999341 722.1818419992924 723.7513740006834 726.9451390001923 729.4445360004902 729.5884530004114 730.7277029994875 734.8262740001082 740.8326130006462 745.6986549999565 749.902928000316 758.4324939996004 762.7576850000769 767.9679540004581

Current
Mean: 722.264 ms
Stdev: 24.996 ms (3.5%)
Runs: 678.516514999792 679.3556409999728 686.6287549994886 691.0365650001913 700.1757050007582 701.758403999731 703.6575940009207 704.7709079999477 705.3503590002656 705.5355900004506 710.9451379999518 711.0535850003362 714.1715530008078 718.8391500003636 720.6916819997132 721.8011610005051 726.0073479991406 726.1649069990963 728.2908730003983 729.7123210001737 731.7163050007075 732.2240329999477 732.5129429996014 735.9782090000808 736.7328350003809 736.9649739991874 742.665643999353 748.5159520003945 770.4171699993312 775.9273729994893 782.0692539997399
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.3%)
Runs: 0.012248000130057335 0.012411000207066536 0.012531999498605728 0.012574000284075737 0.012736000120639801 0.01285799965262413 0.01285799965262413 0.012980001047253609 0.013019999489188194 0.013102000579237938 0.013265000656247139 0.013426998630166054 0.01342799887061119 0.013508999720215797 0.013589998707175255 0.013589998707175255 0.013752998784184456 0.013753999024629593 0.013754000887274742 0.013793999329209328 0.0138349998742342 0.013915998861193657 0.013956999406218529 0.0139979999512434 0.0139979999512434 0.014038000255823135 0.014201000332832336 0.014201000332832336 0.014689000323414803 0.014729999005794525 0.01493300125002861 0.01521800085902214

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.7%)
Runs: 0.01297999918460846 0.013387000188231468 0.013549000024795532 0.013630999252200127 0.013752998784184456 0.013875000178813934 0.014079000800848007 0.014119001105427742 0.014281999319791794 0.0143630001693964 0.014404000714421272 0.01448499970138073 0.014485999941825867 0.014567000791430473 0.014649000018835068 0.014688998460769653 0.014851998537778854 0.014893000945448875 0.01505500078201294 0.01505500078201294 0.015177000313997269 0.01534000039100647 0.01534000039100647 0.015827998518943787 0.015828000381588936 0.015869000926613808 0.015992000699043274 0.01603200100362301 0.016356999054551125 0.01643799990415573 0.01660200022161007 0.0168869998306036
App start nativeLaunch Baseline
Mean: 19.767 ms
Stdev: 1.687 ms (8.5%)
Runs: 17 17 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 22 22 23 23 23

Current
Mean: 19.710 ms
Stdev: 1.570 ms (8.0%)
Runs: 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 21 22 23 24
App start runJsBundle Baseline
Mean: 205.281 ms
Stdev: 20.881 ms (10.2%)
Runs: 163 173 176 181 183 184 187 187 187 188 192 201 201 203 206 207 209 210 210 211 211 212 214 218 219 221 223 226 226 242 246 252

Current
Mean: 193.594 ms
Stdev: 15.789 ms (8.2%)
Runs: 163 172 173 173 174 180 180 181 181 183 187 189 190 190 192 193 194 195 195 196 197 198 198 199 201 207 210 215 217 217 226 229

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Mar 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to staging by https://github.com/deetergp in version: 1.2.78-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham
Copy link
Contributor

This performance regression appears to be consistent with what we were seeing on all PRs since last week, removing the DeployBlockerCash label

https://expensify.slack.com/archives/C07J32337/p1677708239351139?thread_ts=1677703372.513129&cid=C07J32337

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants