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

fix 30211 #30787

Merged
merged 1 commit into from
Nov 2, 2023
Merged

fix 30211 #30787

merged 1 commit into from
Nov 2, 2023

Conversation

c3024
Copy link
Contributor

@c3024 c3024 commented Nov 2, 2023

Details

Fixed Issues

$ #30211
PROPOSAL: #30211 (comment)

Tests

  1. Go to any chat with a user
  2. Click on the header to go the profile page RHP
  3. Click on Private Notes -> My note -> Notes
  4. Reload the page
  5. Click on the back arrow in the RHP
  6. Verify that the page goes correctly back to the 'My note' page of 'Notes' and it does not show a not found page
  • Verify that no errors appear in the JS console

Offline tests

  1. Go to any chat with a user
  2. Click on the header to go the profile page RHP
  3. Click on Private Notes -> My note -> Notes
  4. Reload the page
  5. Click on the back arrow in the RHP
  6. Verify that the page goes correctly back to the 'My note' page of 'Notes' and it does not show a not found page

QA Steps

  1. Go to any chat with a user
  2. Click on the header to go the profile page RHP
  3. Click on Private Notes -> My note -> Notes
  4. Reload the page
  5. Click on the back arrow in the RHP
  6. Verify that the page goes correctly back to the 'My note' page of 'Notes' and it does not show a not found page
  • Verify that no errors appear in the JS console

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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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

Android: Native
privateNotesReportIDAndroidNative.mov
Android: mWeb Chrome
privateNotesReportIDAndroidChrome.mov
iOS: Native
privateNotesReportIDiOSNative.mov
iOS: mWeb Safari
privateNotesReportIDiOSSafari.mov
MacOS: Chrome / Safari
privateNotesReportID.mov
MacOS: Desktop
privateNotesReportIDDesktop.mov

@c3024 c3024 marked this pull request as ready for review November 2, 2023 17:13
@c3024 c3024 requested a review from a team as a code owner November 2, 2023 17:13
@melvin-bot melvin-bot bot requested review from aimane-chnaif and removed request for a team November 2, 2023 17:13
Copy link

melvin-bot bot commented Nov 2, 2023

@aimane-chnaif Please 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]

@c3024
Copy link
Contributor Author

c3024 commented Nov 2, 2023

PR ready for review @aimane-chnaif

@aimane-chnaif
Copy link
Contributor

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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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

Android: Native
android.mov
Android: mWeb Chrome
mchrome.mov
iOS: Native
ios.mov
iOS: mWeb Safari
msafari.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov

@chiragsalian chiragsalian merged commit cd1c2bd into Expensify:main Nov 2, 2023
15 of 19 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Nov 2, 2023

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

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

github-actions bot commented Nov 2, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1062.034 ms → 1175.849 ms (+113.815 ms, +10.7%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1062.034 ms
Stdev: 26.823 ms (2.5%)
Runs: 1019.0926649998873 1024.8216949999332 1026.6868190001696 1028.4412249997258 1028.874839999713 1028.935134000145 1030.0337549997494 1030.802938000299 1036.5537689998746 1036.691868999973 1037.5332410000265 1038.1723830001429 1038.3345659999177 1038.4703439995646 1041.0518720000982 1041.3424540003762 1041.8065670002252 1042.172988999635 1042.2647150000557 1043.1616890002042 1044.4238130003214 1044.6894169999287 1044.762872000225 1051.792669000104 1053.6674429997802 1058.8415050003678 1060.7238199999556 1061.1920990003273 1061.2384799998254 1061.430204000324 1061.6194040002301 1062.0182389998809 1063.6464649997652 1063.9973729997873 1064.432652999647 1066.0663360003382 1066.8881149999797 1067.133948000148 1067.701853999868 1071.2159719998017 1071.2499160002917 1073.5395950004458 1076.4356279997155 1076.776123999618 1080.2551570003852 1080.551523000002 1084.2891119997948 1088.4610829995945 1091.3974169995636 1092.158963999711 1096.5153470002115 1100.436499999836 1102.5781570002437 1106.4290690002963 1112.6865950003266 1117.655194000341 1125.687157000415 1128.1403179997578

Current
Mean: 1175.849 ms
Stdev: 42.072 ms (3.6%)
Runs: 1087.1246569999494 1108.1044069998898 1112.4178230001125 1116.1579559999518 1117.0233869999647 1119.0960860000923 1124.87463700003 1127.0296189999208 1134.591399999801 1135.693471999839 1137.5704600000754 1139.3887569999788 1140.2549890000373 1141.8763399999589 1143.286326999776 1144.0936259999871 1144.8689080001786 1147.6588539998047 1147.9092600001022 1149.659429000225 1153.9841760001145 1157.5162610001862 1158.9550890000537 1162.038021999877 1166.9798820000142 1169.7882590000518 1170.5689880000427 1172.2541400000919 1172.7417089999653 1173.302062000148 1173.409645000007 1173.541629999876 1174.0040589999408 1175.3206310002133 1175.9513320000842 1181.5846800000872 1184.4339359998703 1185.0452589998022 1186.0770360000897 1186.198851000052 1191.8543679998256 1192.0692480001599 1199.5125159998424 1205.6581489997916 1206.7224389999174 1207.6913780001923 1208.7555800001137 1211.9786910000257 1212.362654000055 1215.8794940002263 1219.0221549998969 1219.1078209998086 1219.5672109997831 1226.307068000082 1229.9052459998056 1251.5705619999208 1251.960278000217 1252.3254869999364 1270.7186940000392 1285.5889519997872

Meaningless Changes To Duration

Show entries
Name Duration
App start runJsBundle 728.345 ms → 814.633 ms (+86.289 ms, +11.8%)
App start nativeLaunch 20.719 ms → 22.034 ms (+1.315 ms, +6.3%)
App start regularAppStart 0.017 ms → 0.018 ms (+0.001 ms, +6.3%)
Open Search Page TTI 709.246 ms → 701.196 ms (-8.050 ms, -1.1%)
Show details
Name Duration
App start runJsBundle Baseline
Mean: 728.345 ms
Stdev: 22.287 ms (3.1%)
Runs: 682 695 701 703 704 705 705 706 706 706 708 709 710 710 712 712 713 713 714 715 716 717 718 719 719 720 722 723 723 723 726 726 726 728 730 730 733 737 737 737 738 740 742 743 745 745 747 747 751 752 754 761 764 769 771 774 776 786

Current
Mean: 814.633 ms
Stdev: 32.055 ms (3.9%)
Runs: 756 762 769 773 776 777 779 781 781 783 783 784 785 786 787 787 789 791 794 794 795 796 797 797 799 801 805 805 807 812 815 815 816 817 818 819 821 823 825 827 833 834 834 835 837 838 840 841 842 844 853 854 856 858 865 868 875 879 880 885
App start nativeLaunch Baseline
Mean: 20.719 ms
Stdev: 1.714 ms (8.3%)
Runs: 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 23 23 23 23 23 23 23 24 24 24 25

Current
Mean: 22.034 ms
Stdev: 1.646 ms (7.5%)
Runs: 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 23 24 24 24 24 24 24 25 25 25 25 25 26
App start regularAppStart Baseline
Mean: 0.017 ms
Stdev: 0.001 ms (4.8%)
Runs: 0.014811000786721706 0.015542999841272831 0.015584000386297703 0.01566499937325716 0.015786999836564064 0.0157880000770092 0.015828000381588936 0.015828999690711498 0.015829000622034073 0.015829000622034073 0.015829000622034073 0.015868999995291233 0.015868999995291233 0.015990999527275562 0.016032000072300434 0.016032000072300434 0.016153999604284763 0.016154000535607338 0.01619499921798706 0.016276000067591667 0.0163569999858737 0.016358000226318836 0.016438999213278294 0.016479999758303165 0.0165200000628829 0.0165200000628829 0.0165200000628829 0.0165200000628829 0.0165200000628829 0.0165200000628829 0.01655999943614006 0.01664199959486723 0.016682999208569527 0.0166830001398921 0.016764000058174133 0.016805000603199005 0.016846000216901302 0.016887000761926174 0.016927000135183334 0.017008000053465366 0.0170889999717474 0.017129999585449696 0.01729300059378147 0.017416000366210938 0.017416000366210938 0.01749700028449297 0.017578000202775 0.017740000039339066 0.0179449999704957 0.01822900027036667 0.018432999961078167 0.018960999324917793

Current
Mean: 0.018 ms
Stdev: 0.001 ms (8.1%)
Runs: 0.014730000169947743 0.015339999925345182 0.015340000158175826 0.015910000074654818 0.016112999990582466 0.01611300022341311 0.01619499991647899 0.016195000149309635 0.016276000067591667 0.016276999842375517 0.0163569999858737 0.016357999993488193 0.01639799983240664 0.01647999999113381 0.0165200000628829 0.016642000060528517 0.0167239997535944 0.01676500029861927 0.01676500029861927 0.016845000442117453 0.016845999751240015 0.01684599998407066 0.01696800021454692 0.017090000212192535 0.017212000209838152 0.0172520000487566 0.0172520000487566 0.0172520000487566 0.01725299982354045 0.017375000286847353 0.01741599990054965 0.017537999898195267 0.017619000049307942 0.01769999973475933 0.0177819998934865 0.017822000198066235 0.017862999811768532 0.01814799988642335 0.01822899980470538 0.01822899980470538 0.018352000042796135 0.01843199972063303 0.018472999799996614 0.0185139998793602 0.018514000345021486 0.018595000030472875 0.01859500026330352 0.018716999795287848 0.018799000419676304 0.019164999946951866 0.019245999865233898 0.019367999862879515 0.019530999939888716 0.02054900024086237 0.0206300001591444 0.020752000156790018 0.020874000154435635 0.0209969999268651
Open Search Page TTI Baseline
Mean: 709.246 ms
Stdev: 45.913 ms (6.5%)
Runs: 609.8806159999222 635.8061939999461 643.8918049996719 646.7101239999756 647.9205319993198 648.9640299994498 655.2894290005788 655.450032999739 656.8913979995996 661.8777670003474 663.3431400004774 664.487630000338 667.6464849999174 669.5314530003816 669.8280440000817 671.753744000569 673.1086019994691 674.0869950000197 681.2549649998546 681.6671139998361 683.0593259995803 685.8960779998451 692.9831949993968 693.6648770002648 696.7698569996282 697.5580649999902 699.225424000062 705.2221679994836 708.3911950001493 708.4638679996133 709.3381360005587 709.7428799998015 712.3148600002751 712.8145349994302 713.3985599996522 715.6221930002794 716.5469970004633 718.1818439997733 721.0355230001733 721.4216719996184 722.4491779999807 724.7281899992377 730.8182779997587 733.1242680000141 737.8177080005407 741.369425999932 741.3825690001249 741.5324309999123 747.8535979995504 751.138347000815 755.5040279999375 759.3904220005497 760.3924970002845 764.7002360001206 774.9871829999611 775.4049889994785 784.1673590000719 787.0398359997198 798.9056399995461 809.8376469993964 820.4109700005502

Current
Mean: 701.196 ms
Stdev: 41.070 ms (5.9%)
Runs: 631.6118990001269 636.1551509997807 639.867554999888 643.9718430000357 644.7445479999296 647.2004809998907 647.5328379999846 650.1521809999831 653.6728109996766 656.9666340001859 657.3080239999108 657.766072999686 660.2185060000047 661.9958500000648 662.5026050000452 663.1760669997893 664.9635419999249 666.8079429999925 667.7757159997709 671.6749680000357 671.7532150000334 674.4122720002197 678.4420579997823 680.446574000176 683.4459640001878 683.812784999609 686.790974999778 699.7048349999823 700.7093100002967 701.5867920001037 701.6315930001438 701.7032469999976 705.0138349998742 707.381674000062 707.9907639999874 709.1551919998601 716.4983319998719 716.7790530002676 720.1066899998114 725.8806969998404 728.2126070000231 728.9663489996456 730.6663420000114 731.1080320002511 731.8968509999104 733.9804279999807 735.0176189998165 740.5566409998573 741.3417159998789 746.5558679997921 747.0399170001037 750.4935710001737 751.9394129998982 753.6278900001198 757.0743410000578 760.0133870001882 762.0788990003057 763.3722740001976 764.2958990000188 767.1459149997681 788.2373860003427

Copy link
Contributor

github-actions bot commented Nov 2, 2023

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

@OSBotify
Copy link
Contributor

OSBotify commented Nov 6, 2023

🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.96-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Nov 9, 2023

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀

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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants