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

[CP Staging] Revert "Handle suggestions only on selection change" #28794

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

cubuspl42
Copy link
Contributor

@cubuspl42 cubuspl42 commented Oct 4, 2023

This reverts commit a4f4a77.

Details

This is an emergency fix for #28657, fully reverting PR #28583. Issue #28170 will need to be solved again in a different manner.

Fixed Issues

$ #28657
PROPOSAL: N/A

Tests

Same as QA Steps

Offline tests

QA Steps

  • Open any personal or group chat
  • Type "@" (as the first character)
  • Verify that the mentions lists opened
  • 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 / 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 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
    • 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 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
revert-pr-28583-web.mp4
Mobile Web - Chrome
revert-pr-28583-android-web-compressed.mp4
Mobile Web - Safari
revert-pr-28583-ios-web.mp4
Desktop
revert-pr-28583-desktop.mp4
iOS
revert-pr-28583-ios.mp4
Android
revert-pr-28583-android-compressed.mp4

@cubuspl42 cubuspl42 requested a review from a team as a code owner October 4, 2023 09:00
@melvin-bot melvin-bot bot requested review from aimane-chnaif and removed request for a team October 4, 2023 09:00
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 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]

@aimane-chnaif
Copy link
Contributor

Please fill all checklist items

@Julesssss Julesssss removed the request for review from aimane-chnaif October 4, 2023 09:15
@Julesssss
Copy link
Contributor

Hey @aimane-chnaif, I'm unassigning you just because it's a straight revert. No need for a full C+ review

@Julesssss
Copy link
Contributor

Generating test builds...

@aimane-chnaif
Copy link
Contributor

I just tested iOS. Confirmed working fine.

ios.mov

@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2023

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖 iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/28794/index.html ❌ FAILED ❌
Android The QR code can't be generated, because the iOS build failed
desktop 💻 web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/28794/NewExpensify.dmg https://28794.pr-testing.expensify.com
Desktop Web

@Julesssss
Copy link
Contributor

@cubuspl42 please complete the author checklist

@cubuspl42
Copy link
Contributor Author

@Julesssss I'm on it, I'm having trouble with my Android emulator for recording videos

@cubuspl42
Copy link
Contributor Author

...but it was working fine on Android Native (but I didn't manage to record it). I didn't test on Android/Web yet (but it's working on iOS/Web and Desktop/Web).

Also, it's a straight revert. So I'll check the checkbox while I figure out what's wrong with my emulator.

@Julesssss
Copy link
Contributor

Cool, just let me know when you're ready for me to take a look.

@cubuspl42
Copy link
Contributor Author

@Julesssss I updated and restarted everything I could, and the emulator is fine again.

I'm sometimes having experiences like that with it, I don't know why.

Videos uploaded for all platforms.

@Julesssss Julesssss merged commit df0cc2a into Expensify:main Oct 4, 2023
14 of 18 checks passed
@Julesssss
Copy link
Contributor

Cherry-picking here

OSBotify pushed a commit that referenced this pull request Oct 4, 2023
Revert "Handle suggestions only on selection change"

(cherry picked from commit df0cc2a)
@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 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 Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1217.612 ms → 1305.473 ms (+87.861 ms, +7.2%) 🔴
App start runJsBundle 843.500 ms → 910.250 ms (+66.750 ms, +7.9%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1217.612 ms
Stdev: 32.937 ms (2.7%)
Runs: 1168.531723999884 1171.3622079999186 1171.4428650001064 1171.6581850000657 1175.4552370000165 1178.311231000116 1188.439158000052 1198.5732120000757 1200.7144200000912 1202.1152810000349 1203.370930999983 1203.5516500000376 1204.4678980000317 1204.642075000098 1209.6988399999682 1218.7962849999312 1224.6544309998862 1225.059283999959 1226.4081929998938 1226.409915999975 1232.158850000007 1234.144472999964 1240.1861330000684 1242.1089060000377 1248.3785069999285 1260.3922709999606 1263.653067999985 1267.4703349999618 1277.1180710000917 1289.1010140000144

Current
Mean: 1305.473 ms
Stdev: 55.218 ms (4.2%)
Runs: 1219.2604320000391 1223.2996010000352 1226.7752489999402 1245.3855069999117 1254.258160999976 1256.234190999996 1258.4844219998922 1258.9787570000626 1260.7522499999031 1262.0623139999807 1264.1374250000808 1267.509408999933 1268.6408190000802 1281.9748050000053 1282.7523680001032 1293.4332030001096 1296.2436130000278 1299.916264000116 1313.7160910000093 1336.0537839999888 1340.0383180000354 1340.4373099999502 1342.0616699999664 1343.1628119999077 1356.8768569999374 1365.2910790001042 1365.9050980000757 1369.2689390000887 1369.4948219999205 1383.7546729999594 1401.8018600000069 1427.1889599999413
App start runJsBundle Baseline
Mean: 843.500 ms
Stdev: 28.021 ms (3.3%)
Runs: 769 809 813 815 817 819 819 821 821 824 835 836 838 838 839 839 841 843 850 850 850 851 860 864 865 867 868 870 874 882 886 919

Current
Mean: 910.250 ms
Stdev: 43.693 ms (4.8%)
Runs: 847 849 852 856 867 871 881 881 882 884 885 888 891 894 898 898 900 900 906 908 920 927 932 939 940 944 947 950 976 994 1007 1014

Meaningless Changes To Duration

Show entries
Name Duration
App start nativeLaunch 21.138 ms → 23.871 ms (+2.733 ms, +12.9%)
App start regularAppStart 0.018 ms → 0.015 ms (-0.003 ms, -17.9%) 🟢
Open Search Page TTI 654.503 ms → 654.168 ms (-0.335 ms, ±0.0%)
Show details
Name Duration
App start nativeLaunch Baseline
Mean: 21.138 ms
Stdev: 1.479 ms (7.0%)
Runs: 19 19 19 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 22 22 22 22 23 23 23 24 24 24

Current
Mean: 23.871 ms
Stdev: 3.526 ms (14.8%)
Runs: 19 20 20 20 20 21 21 21 21 21 22 22 22 22 23 23 24 24 24 24 25 26 26 26 26 27 27 29 29 31 34
App start regularAppStart Baseline
Mean: 0.018 ms
Stdev: 0.001 ms (5.1%)
Runs: 0.01676399982534349 0.016805000137537718 0.017131000058725476 0.017333999974653125 0.01737500005401671 0.01741500000935048 0.017577999969944358 0.017577999969944358 0.01757899997755885 0.017740999814122915 0.0177819998934865 0.017782000126317143 0.017862999811768532 0.017902999883517623 0.01790400012396276 0.017943999962881207 0.017985000042244792 0.018106999807059765 0.01810700003989041 0.018148000119253993 0.01835100003518164 0.018391999881714582 0.0186769999563694 0.018962000031024218 0.019084000028669834 0.01920600002631545 0.019775000168010592 0.01997899985872209 0.02002000017091632 0.020426000002771616

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.9%)
Runs: 0.013550000032410026 0.013550000032410026 0.013630999950692058 0.01383400009945035 0.01387499994598329 0.0139979999512434 0.0139979999512434 0.014038000022992492 0.014201000100001693 0.014282000018283725 0.014282000018283725 0.014322999864816666 0.014566999860107899 0.0147299999371171 0.0147299999371171 0.014730000169947743 0.014810999855399132 0.014932999853044748 0.015177999855950475 0.015299000078812242 0.015339999925345182 0.015664999838918447 0.015666000079363585 0.015746999997645617 0.01603199983946979 0.01619499991647899 0.0163569999858737 0.01639799983240664 0.01684599998407066 0.017496000044047832
Open Search Page TTI Baseline
Mean: 654.503 ms
Stdev: 33.205 ms (5.1%)
Runs: 610.3679609999526 611.0254730000161 611.4644369999878 613.1391600000206 615.6948239998892 623.0157469999976 625.5198569998611 626.5778810000047 627.1547040001024 629.5354009999428 631.1053059999831 634.3538010001648 645.5696209999733 648.15612900001 653.076008999953 655.7153729998972 655.8574629998766 656.8389079999179 660.4048259998672 660.7765309999231 660.8040770001244 661.9034019999672 665.4274089999963 667.4092619998846 667.8092859999742 670.5321049999911 673.7244469998404 694.5144050000235 709.1543380001094 714.605916999979 728.5108239999972 734.3361419998109

Current
Mean: 654.168 ms
Stdev: 29.732 ms (4.5%)
Runs: 620.221639999887 620.8506670000497 623.004760999931 623.2984210001305 624.8378500000108 626.7850759997964 626.8640550000127 631.8177090000827 633.4456790001132 634.8560790000483 635.5581460001413 636.4547940001357 638.9595129999798 643.4122719999868 644.1124680000357 644.8922530000564 645.1158449999057 645.7480469997972 647.204590999987 651.4914969999809 653.4261069998611 654.0973310000263 656.9075110000558 660.7495929999277 669.4706620001234 673.8672289999668 678.1068929999601 687.1448160000145 693.776489000069 695.2117109999526 702.7485759998672 730.8693440000061 732.2294109999202

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

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

@Julesssss Julesssss removed the DeployBlockerCash This issue or pull request should block deployment label Oct 4, 2023
@Julesssss
Copy link
Contributor

This is simply a revert PR. Which kinda proves that the performance regression test isn't very reliable...

@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2023

🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 1.3.77-3 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@Julesssss
Copy link
Contributor

Cherry-pick was successful 👍

@mountiny mountiny changed the title Revert "Handle suggestions only on selection change" [CP Staging] Revert "Handle suggestions only on selection change" Oct 4, 2023
@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 6, 2023

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.79-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 9, 2023

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀

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

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

Successfully merging this pull request may close these issues.

4 participants