Skip to content

Fix - mweb-expense-Context menu flickers&shown in create expense page in a workspace chat.#59014

Merged
amyevans merged 6 commits intoExpensify:mainfrom
FitseTLT:fix-fab-menu-flickering-on-page-navigation
Apr 1, 2025
Merged

Fix - mweb-expense-Context menu flickers&shown in create expense page in a workspace chat.#59014
amyevans merged 6 commits intoExpensify:mainfrom
FitseTLT:fix-fab-menu-flickering-on-page-navigation

Conversation

@FitseTLT
Copy link
Contributor

Details

Fixed Issues

$ #58188
PROPOSAL: #58188 (comment)

Tests

  1. Open a workspace chat
  2. Tap create expense
  3. Verify that Context menu doesn't flicker and be shown overlapping with create expense page
  • Verify that no errors appear in the JS console

Offline tests

Same as above

QA Steps

Same as above

  • 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 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 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 the PR modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label so the design team can review the changes.
  • 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.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • 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.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
2025-03-25.01-47-30.mp4
iOS: Native
i.mp4
iOS: mWeb Safari
iw.mp4
MacOS: Chrome / Safari
w.mp4
MacOS: Desktop
d.mp4

@FitseTLT FitseTLT requested a review from a team as a code owner March 24, 2025 22:49
@melvin-bot melvin-bot bot requested review from ZhenjaHorbach and removed request for a team March 24, 2025 22:49
@melvin-bot
Copy link

melvin-bot bot commented Mar 24, 2025

@ZhenjaHorbach 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]

@FitseTLT
Copy link
Contributor Author

Android build is failing

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Mar 25, 2025

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 that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • 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 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 either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • 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.ts 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(theme.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 the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • 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.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • 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
2025-04-01.15.22.31.mov
Android: mWeb Chrome
2025-04-01.15.22.31.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
web.mov

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Mar 25, 2025

I think we should enable shouldCallAfterModalHide only for mobile
Because due to the new delay the chat in LHN has time to be focused for a short period of time

2025-03-25.19.59.58.mov

@FitseTLT
Copy link
Contributor Author

Fixed

@FitseTLT
Copy link
Contributor Author

Done Thx

@ZhenjaHorbach
Copy link
Contributor

Done Thx

Nice
I will complete the checklist today !

@melvin-bot melvin-bot bot requested a review from amyevans April 1, 2025 13:31
@ZhenjaHorbach
Copy link
Contributor

LGTM

@amyevans amyevans merged commit 80df1b2 into Expensify:main Apr 1, 2025
17 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Apr 1, 2025

✋ 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
Copy link
Contributor

github-actions bot commented Apr 1, 2025

🚀 Deployed to staging by https://github.com/amyevans in version: 9.1.22-0 🚀

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

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Apr 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

Performance Comparison Report 📊⚠️ Some tests did not pass successfully, so some results are omitted from final report: Linking

Significant Changes To Duration

Name Duration
App start time contentAppeared_To_screenTTI 1007.097 ms → 1175.246 ms (+168.149 ms, +16.7%) 🔴
App start time TTI 1684.764 ms → 1829.109 ms (+144.345 ms, +8.6%) 🔴
Show details
Name Duration
App start time contentAppeared_To_screenTTI Baseline
Mean: 1007.097 ms
Stdev: 243.287 ms (24.2%)
Runs: 546.5719820000231 594.6324159987271 594.8886840008199 613.6860160008073 620.3209469988942 632.5423280000687 632.7570840008557 634.8132410012186 635.202539999038 654.5304020009935 658.0918309986591 658.102267999202 660.9384570010006 670.7470830008388 680.5250190012157 684.3440399989486 696.1443799994886 704.0526540018618 1045.38937599957 1076.3640730008483 1085.0923900008202 1090.133358001709 1090.8917310014367 1106.6697250008583 1111.5904779992998 1114.085965000093 1115.7306599989533 1124.4613539986312 1130.1483240015805 1136.3556349985301 1137.7896130010486 1139.508179999888 1139.5337879993021 1144.959141999483 1148.9262050017715 1155.20228099823 1155.8106269985437 1156.4657130017877 1158.9460380002856 1159.3147980012 1159.3666479997337 1173.9941329993308 1175.043382000178 1175.6669940017164 1179.4355910010636 1181.9265040010214 1183.549263998866 1191.6513240002096 1193.5440119989216 1197.901519998908 1199.0259569995105 1199.227010000497 1201.0882969982922 1217.5671240016818 1218.329989001155 1224.4696639999747 1249.111182000488 1254.6586069986224 1271.8712610006332 1282.1311989985406

Current
Mean: 1175.246 ms
Stdev: 55.442 ms (4.7%)
Runs: 1051.817593999207 1066.5478969998658 1076.235883001238 1081.1465369984508 1099.201071999967 1100.498966999352 1108.8433680012822 1114.558651998639 1118.3139850012958 1123.0609479993582 1128.7722220011055 1136.4123799987137 1136.9678639993072 1146.5796939991415 1154.8467269986868 1158.97521699965 1162.2975509986281 1163.6085290014744 1167.048501998186 1169.5513820014894 1172.556995999068 1173.1437060013413 1174.2777619995177 1181.3768289983273 1181.885664999485 1183.826099999249 1184.6317819990218 1187.0376809984446 1209.2186230011284 1210.938981000334 1211.964549999684 1215.0352099984884 1215.9265649989247 1219.1247220002115 1219.5878029987216 1224.0221940018237 1226.3022469989955 1227.6998590007424 1228.9219769984484 1231.7790989987552 1232.278560001403 1235.6608610004187 1238.846967998892 1263.672951001674 1264.1049069985747 1282.2170279994607
App start time TTI Baseline
Mean: 1684.764 ms
Stdev: 261.266 ms (15.5%)
Runs: 1230.632415998727 1237.9384570010006 1250.6860160008073 1254.5719820000231 1281.102267999202 1291.3440399989486 1298.5423280000687 1299.202539999038 1301.7570840008557 1304.7470830008388 1305.5250190012157 1307.88868400082 1310.3209469988942 1322.0526540018618 1323.8132410012186 1353.0918309986591 1373.5304020009935 1428.1443799994886 1690.38937599957 1693.133358001709 1701.3640730008483 1713.0923900008202 1724.8917310014367 1742.5337879993021 1743.3556349985301 1743.6697250008583 1747.1483240015805 1761.959141999483 1765.085965000093 1781.20228099823 1790.508179999888 1806.3666479997337 1814.6513240002096 1816.3147980012 1828.8106269985437 1829.0882969982922 1834.9941329993308 1847.9460380002856 1848.549263998866 1856.043382000178 1866.0259569995105 1874.7896130010486 1885.6669940017164 1892.329989001155 1897.4355910010636 1897.5671240016818 1898.6586069986224 1902.7306599989533 1905.901519998908 1912.9265040010214 1917.4657130017877 1928.5904779992998 1937.5440119989216 1946.227010000497 1948.4613539986312 1949.9262050017715 1962.111182000488 1975.4696639999747 1994.8712610006332 2035.1311989985406

Current
Mean: 1829.109 ms
Stdev: 128.862 ms (7.0%)
Runs: 1389.678355999291 1427.2257229983807 1465.0940669998527 1666.1465369984508 1696.558651998639 1699.817593999207 1700.201071999967 1747.235883001238 1762.4123799987137 1764.5478969998658 1771.2975509986281 1771.7722220011055 1779.2777619995177 1789.8467269986868 1793.9678639993072 1797.3139850012958 1798.5513820014894 1814.6085290014744 1834.0376809984446 1836.826099999249 1840.8433680012822 1849.938981000334 1852.0609479993582 1858.1247220002115 1863.9265649989247 1864.048501998186 1874.556995999068 1877.3768289983273 1886.1437060013413 1887.278560001403 1889.498966999352 1896.0352099984884 1897.964549999684 1904.2186230011284 1905.6608610004187 1913.5878029987216 1914.6998590007424 1917.9219769984484 1917.97521699965 1934.0221940018237 1936.6317819990218 1939.2170279994607 1939.885664999485 1948.672951001674 1955.3022469989955 1957.5796939991415 1961.846967998892 1962.7790989987552 1972.1049069985747

Meaningless Changes To Duration

Show entries
Name Duration
App start time nativeLaunchEnd_To_appCreationStart 75.750 ms → 76.967 ms (+1.217 ms, +1.6%)
App start time nativeLaunch 24.190 ms → 23.145 ms (-1.044 ms, -4.3%)
App start time appCreation 67.867 ms → 66.068 ms (-1.799 ms, -2.7%)
App start time appCreationEnd_To_contentAppeared 507.085 ms → 509.017 ms (+1.932 ms, ±0.0%)
App start time runJsBundle 307.867 ms → 306.424 ms (-1.443 ms, ±0.0%)
App start time regularAppStart 0.020 ms → 0.020 ms (+0.000 ms, ±0.0%)
App start time (CPU) 146.639 % → 146.016 % (-0.622 %, ±0.0%)
App start time (FPS) 60.000 FPS → 60.000 FPS
App start time (RAM) 392.046 MB → 393.685 MB (+1.640 MB, ±0.0%)
App start time (CPU/JS) 0.000 % → 0.000 %
App start time (CPU/UI) 24.265 % → 23.915 % (-0.350 %, -1.4%)
Open search router TTI Load Search Options 170.421 ms → 169.550 ms (-0.871 ms, -0.5%)
Open search router TTI Open Search Router TTI 1354.761 ms → 1348.666 ms (-6.094 ms, ±0.0%)
Open search router TTI (CPU) 142.696 % → 143.863 % (+1.167 %, +0.8%)
Open search router TTI (FPS) 60.000 FPS → 60.000 FPS
Open search router TTI (RAM) 424.134 MB → 423.358 MB (-0.777 MB, ±0.0%)
Open search router TTI (CPU/JS) 0.000 % → 0.000 %
Open search router TTI (CPU/UI) 23.886 % → 23.803 % (-0.083 %, ±0.0%)
Report typing Composer typing rerender count 1.000 renders → 1.000 renders
Report typing Message sent 474.153 ms → 472.790 ms (-1.363 ms, ±0.0%)
Report typing (CPU) 93.687 % → 93.387 % (-0.300 %, ±0.0%)
Report typing (FPS) 60.000 FPS → 60.000 FPS
Report typing (RAM) 491.744 MB → 489.885 MB (-1.859 MB, ±0.0%)
Report typing (CPU/JS) 0.000 % → 0.000 %
Report typing (CPU/UI) 20.343 % → 20.190 % (-0.153 %, -0.8%)
Chat opening Chat TTI 943.394 ms → 948.524 ms (+5.130 ms, +0.5%)
Chat opening (CPU) 152.654 % → 152.624 % (-0.030 %, ±0.0%)
Chat opening (FPS) 60.000 FPS → 60.000 FPS
Chat opening (RAM) 422.532 MB → 420.340 MB (-2.192 MB, -0.5%)
Chat opening (CPU/JS) 0.000 % → 0.000 %
Chat opening (CPU/UI) 29.541 % → 29.892 % (+0.351 %, +1.2%)

undefined

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

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

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

🚀 Deployed to production by https://github.com/grgia in version: 9.1.22-10 🚀

platform result
🖥 desktop 🖥 success ✅
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 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