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

Add subscribe to parent comment action menu #27994

Merged
merged 23 commits into from
Oct 28, 2023

Conversation

srikarparsi
Copy link
Contributor

@srikarparsi srikarparsi commented Sep 22, 2023

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/309277
PROPOSAL:

Tests

Subscribing/Unsubscribing to a thread with comments:

  1. Comment in a chat and reply to that comment to create a thread
  2. Leave the thread and return to the parent report
  3. Hover over the comment and press subscribe
  4. Ensure the thread report is in your LHN
  5. Then hover over the same chat report and press unsubscribe
  6. Ensure that the thread report disappears from your LHN

Subscribing to a thread without comments (Need 2 users, User A and User B):

  1. With User B, comment in a chat
  2. With User A, subscribe to the comment that was just created
  3. With User B, reply to the comment
  4. Ensure that the thread shows up in User As LHN and they receive a notification
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  • 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
    • 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

Web
Screen.Recording.2023-10-27.at.4.41.32.PM.mov
Screen.Recording.2023-10-17.at.10.35.58.PM.mov
Mobile Web - Chrome
Screen.Recording.2023-10-27.at.4.46.09.PM.mov
Mobile Web - Safari
Screen.Recording.2023-10-27.at.4.47.06.PM.mov
Desktop
Screen.Recording.2023-10-27.at.4.55.40.PM.mov
iOS image
Android
IMG_8975.MOV

@srikarparsi srikarparsi requested a review from a team as a code owner September 22, 2023 08:21
@srikarparsi srikarparsi self-assigned this Sep 22, 2023
@melvin-bot melvin-bot bot requested review from danieldoglas and removed request for a team September 22, 2023 08:22
@melvin-bot
Copy link

melvin-bot bot commented Sep 22, 2023

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

@srikarparsi srikarparsi changed the title [DRAFT] Srikar add subscribe to parent comment [DRAFT] Add subscribe to parent comment action menu Sep 22, 2023
@srikarparsi
Copy link
Contributor Author

Hi @dannymcclain! Do you think you could send me the bell and bell-slash icon whenever you get a chance. I see them in the figma but not sure whether I should use the regular icons or the flattened ones. This is for the subscribe to thread functionality from this thread.

image

@dannymcclain
Copy link
Contributor

dannymcclain commented Sep 26, 2023

No prob! Will post them soon. Sorry, I just posted them and realized the export was goofed up.

@danieldoglas
Copy link
Contributor

@srikarparsi please create the PR as a DRAFT instead of a full PR until it's ready to review :)

I'll unassign myself from it for now

@danieldoglas danieldoglas removed their request for review September 26, 2023 07:47
@dannymcclain
Copy link
Contributor

🔔 🔕
Bell Icons.zip

@srikarparsi
Copy link
Contributor Author

Hey @dannymcclain, I believe the bell icons might be just a little too big?
image

The svg code also looks a little different between the bell icons and the other icons.

For reference, this is the svg code for the chatBubble icon:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 26.5.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
	 viewBox="0 0 20 20" style="enable-background:new 0 0 20 20;" xml:space="preserve">
<path d="M18.2,14.2c-0.1-0.3,0-0.6,0.2-0.9c1-1.2,1.6-2.7,1.6-4.3c0-4.4-4.5-8-10-8C4.5,1,0,4.6,0,9c0,4.4,4.5,8,10,8
	c0.9,0,1.8-0.1,2.7-0.3c0.2-0.1,0.5,0,0.7,0.1l5.2,2.7c0.4,0.2,0.8-0.1,0.7-0.6L18.2,14.2z"/>
</svg>

And this is svg code for the bell icon (I took out the fill: "black" from the zip file you sent)

<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M4 8.93133C4 9.30295 3.89557 9.6671 3.69861 9.98223L1.67719 13.2165C1.5614 13.4018 1.5 13.6158 1.5 13.8343C1.5 14.4781 2.02189 15 2.66569 15H17.3343C17.9781 15 18.5 14.4781 18.5 13.8343C18.5 13.6158 18.4386 13.4018 18.3228 13.2165L16.3014 9.98223C16.1044 9.6671 16 9.30295 16 8.93133V7C16 3.68629 13.3137 1 10 1C6.68629 1 4 3.68629 4 7V8.93133ZM10 19C8.51353 19 7.27954 17.9189 7.0415 16.5H12.9585C12.7205 17.9189 11.4865 19 10 19Z"/>
</svg>

@dannymcclain
Copy link
Contributor

@srikarparsi The reason the SVG code looks a little different is because most of our SVGs have been exported from Illustrator instead of Figma, but I've been running into a lot of issues with Illustrator distorting/changing the paths of an icon. So if it's not a problem, let's go with the slightly cleaner SVG code.

As for the size, I think typically all of our icons are designed and exported at 20px x 20px and then in contexts like this where they need to be smaller, the size is adjusted on the front end.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 12, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 37f5d27
Status: ✅  Deploy successful!
Preview URL: https://3018d0ac.helpdot.pages.dev
Branch Preview URL: https://srikar-addsubscribetoparentc.helpdot.pages.dev

View logs

@srikarparsi
Copy link
Contributor Author

Talking to @chiragsalian 1 on 1 but wanted to leave a comment on this Github. Subscribing and unsubscribing is working but the childReportNotificationPreference data isn't updating so the right icon isn't being shown in the context action menu. I think we might need to do something here to fetch updated data but I'm not sure if there's a better way to do it. A video of it working without updating the icons is below:

Screen.Recording.2023-10-17.at.3.13.09.AM.mov

@srikarparsi srikarparsi changed the title [DRAFT] Add subscribe to parent comment action menu Add subscribe to parent comment action menu Oct 18, 2023
Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

LGTM and works well.

@chiragsalian
Copy link
Contributor

This PR could have been removed from HOLD yesterday.

Also I just noticed you didn't test on all platforms. In future atleast add a C+ reviewer so that they can test it on all platforms.
This PR hasn't had much movement on it for a while so I'm unsure if i should just yolo it without it being tested on all platforms so that this moves ahead sooner 😅 Alright imma yolo it. QA should hopefully catch issues if any.

@chiragsalian
Copy link
Contributor

chiragsalian commented Oct 27, 2023

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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

@chiragsalian chiragsalian changed the title [HOLD Web #39234] Add subscribe to parent comment action menu Add subscribe to parent comment action menu Oct 27, 2023
@chiragsalian
Copy link
Contributor

Removing HOLD since https://github.com/Expensify/Web-Expensify/pull/39234 is live.

@chiragsalian
Copy link
Contributor

Oh look like PR author checklist isn't completed yet. Maybe you are still working on this. Let us know or update accordingly @srikarparsi.

@srikarparsi
Copy link
Contributor Author

Hey @chiragsalian, so there's one problem I found with more testing.

  1. Comment on a report
  2. Reply to the comment
  3. Go into the thread, and then click leave thread
  4. Return to the parent report.
  5. The unsubscribe icon will show up even if the report is no longer shared.
Screen.Recording.2023-10-27.at.4.49.30.PM.mov

The reason this occurs is because for reports you're the owner of, the icon is unsubscribe by default if the child report doesn't exist. I can create a fix for this so that the leaveReport command updates the child report notification preference but the alternative is to modify the leaveThread button to set the notification preference to hidden instead of unsharing the report. What do you think?

Also, would this be better as a follow up issue since it's pretty edge case and would require web, auth and app changes if we go the child report notification preference route. And that way I can fix it alongside any other bugs that come up?

@srikarparsi
Copy link
Contributor Author

Running a build for android

@OSBotify
Copy link
Contributor

@chiragsalian
Copy link
Contributor

for reports you're the owner of, the icon is unsubscribe by default

yes that's correct. But I don't see why that's the issue. I also don't think this would require a web change. I think it might require an auth change such that when leaveRoom is called, before we unshare the report we check if user is the owner or maybe first commenter and set their notification preference appropriately.

Also, would this be better as a follow up issue since it's pretty edge case

Sure a follow-up is fine. Just ask mitch also so that we're on the same page with no surprises.

@chiragsalian chiragsalian merged commit a59d56f into main Oct 28, 2023
16 of 23 checks passed
@chiragsalian chiragsalian deleted the srikar-addSubscribeToParentComment branch October 28, 2023 00:57
@OSBotify
Copy link
Contributor

✋ 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 28, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1093.507 ms → 1165.317 ms (+71.810 ms, +6.6%) 🔴
App start runJsBundle 762.644 ms → 815.600 ms (+52.956 ms, +6.9%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1093.507 ms
Stdev: 29.046 ms (2.7%)
Runs: 1037.952316999901 1041.6490139998496 1042.539758999832 1049.3506289999932 1058.5185349998064 1060.2450870000757 1067.2372790002264 1067.7097910000011 1068.064518999774 1069.815479000099 1069.8857579999603 1071.9293880001642 1073.539206000045 1074.3801299999468 1075.0757869998924 1075.3920999998227 1075.636795999948 1075.640035000164 1075.8847570000216 1076.9693439998664 1079.3526340001263 1080.028897000011 1080.4676490002312 1080.9065169999376 1081.354948000051 1081.674467000179 1084.552620000206 1085.1640329998918 1087.0377540001646 1089.0821030000225 1091.0909139998257 1091.9474470000714 1092.370810000226 1092.6450800001621 1096.483583000023 1099.8343270001933 1100.2980809998699 1101.0192760000937 1102.668750999961 1103.229927000124 1104.0306110000238 1105.2573919999413 1105.6966570001096 1105.8188490001485 1112.11159100011 1113.9737300002016 1114.9716019998305 1124.5881799999624 1128.4588270001113 1129.7558829998598 1129.915500999894 1130.4581539998762 1140.1450600000098 1146.2849510000087 1153.307978999801 1153.6137870000675 1156.996962999925 1159.3783490001224

Current
Mean: 1165.317 ms
Stdev: 49.332 ms (4.2%)
Runs: 1059.017219999805 1062.4856469999067 1080.0153189999983 1091.551667000167 1098.4887060001493 1099.8047239999287 1112.398186000064 1114.956048999913 1115.297511999961 1116.4882470001467 1118.1670289998874 1119.1963410000317 1120.5569509998895 1121.9208049997687 1123.7843289999291 1131.264140999876 1132.08781500021 1137.3782339999452 1139.0457469997928 1139.7052079997957 1141.0496169999242 1141.1725840000436 1147.3538759998046 1149.9949429999106 1150.2663259999827 1152.3587770001031 1157.241499000229 1157.9266980001703 1158.7367190001532 1160.9825340001844 1163.1095360000618 1165.857435000129 1166.971260999795 1169.63469000021 1170.3178440001793 1180.6527780001052 1181.094647000078 1182.3268869998865 1182.6504950001836 1184.657137000002 1187.5088650002144 1188.9839429999702 1193.0859369998798 1198.361051000189 1201.458538999781 1202.9711350002326 1203.1224349997938 1204.2497049998492 1219.5885459999554 1220.1301230001263 1221.3976039998233 1222.4692379999906 1227.375299999956 1231.6521580000408 1233.729937999975 1239.4329519998282 1240.9144930001348 1241.1390979997814 1251.6066410001367 1291.8510480001569
App start runJsBundle Baseline
Mean: 762.644 ms
Stdev: 21.592 ms (2.8%)
Runs: 708 712 728 733 735 737 740 743 744 745 747 747 748 748 749 750 752 752 753 753 754 755 756 757 757 757 759 759 759 760 760 761 762 763 764 765 765 766 766 769 770 771 772 773 778 781 782 783 784 787 788 789 790 791 794 795 800 803 827

Current
Mean: 815.600 ms
Stdev: 41.409 ms (5.1%)
Runs: 730 738 744 756 757 768 769 775 777 779 779 780 782 785 786 786 787 788 789 789 790 793 795 797 798 801 803 804 808 809 810 813 816 818 819 819 828 830 831 832 832 837 839 840 842 848 849 852 860 862 863 865 870 881 883 885 887 889 890 904

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 706.391 ms → 712.707 ms (+6.316 ms, +0.9%)
App start regularAppStart 0.014 ms → 0.014 ms (-0.000 ms, -1.2%)
App start nativeLaunch 22.255 ms → 20.860 ms (-1.395 ms, -6.3%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 706.391 ms
Stdev: 36.272 ms (5.1%)
Runs: 637.5852870000526 648.878703000024 657.419474999886 657.5720220003277 658.0516760000028 659.8480219999328 664.4466149997897 664.5317379999906 666.6685790000483 668.5298669999465 668.6330969999544 669.4483230002224 670.2315679998137 671.8669849997386 673.8430180000141 675.3120940001681 677.5252689998597 677.9873450002633 678.6460770000704 678.9293220001273 679.1602380000986 680.4372570002452 689.5687669999897 689.7728269998915 691.8118090000935 696.2842210000381 696.3717459999025 700.282105000224 701.4466550000943 706.8798830001615 707.4595949999057 709.3720709998161 709.8277590000071 710.1631669998169 714.2233479996212 714.3201100002043 715.3168140002526 716.1559250000864 719.9535730001517 722.1758220000193 723.8493250003085 726.1151530002244 727.7486579995602 728.2551269996911 728.8759770002216 729.4884039997123 730.7242029998451 731.1509610000066 739.9750569998287 740.3498539999127 741.4180499999784 744.4772959998809 752.80102599971 755.7966719996184 758.8701989999972 760.7087820000015 761.1234949999489 768.8513589999638 774.4695639996789 780.4583339998499 787.3812669999897

Current
Mean: 712.707 ms
Stdev: 49.600 ms (7.0%)
Runs: 602.5405279998668 634.1768390000798 654.2642820002511 655.8300379998982 658.1090899999253 664.5011809999123 664.7315269997343 665.7219249997288 666.010295000393 668.6418459997512 670.4862059997395 670.994589000009 671.3311769999564 673.1731370002963 674.3501389999874 676.6990559999831 676.7390959998593 676.8379310001619 679.8710129996762 681.581828000024 682.7050379998982 683.1931970003061 683.3770349998958 685.2878429996781 685.7498370003887 689.7462570001371 694.147501999978 694.8301189998165 695.5227049998939 696.9303390001878 697.9627279997803 699.2263189996593 703.3693849998526 711.6470139999874 716.6565759996884 717.9361979998648 724.475097999908 727.7017419999465 730.1575110000558 730.7098400001414 737.2949219997972 738.5819089999422 738.7766120000742 742.1889239996672 748.8592530000024 749.4206539997831 757.9973959997296 761.3582359999418 762.4929609997198 763.0428880001418 767.0077720000409 768.6524669998325 770.3940020003356 771.5912680001929 780.6382250003517 787.9565030001104 789.6909590000287 793.8104259995744 834.5670579997823 860.1791179999709
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.2%)
Runs: 0.012533000204712152 0.012898999731987715 0.013021000195294619 0.013264999724924564 0.013346000108867884 0.0134680001065135 0.013509000185877085 0.01358999963849783 0.013590000104159117 0.013590999878942966 0.013671999797224998 0.013671999797224998 0.013672000262886286 0.01371300034224987 0.013793999794870615 0.013874999713152647 0.013875999953597784 0.013875999953597784 0.01399700017645955 0.01399700017645955 0.0139979999512434 0.013998000416904688 0.014037999790161848 0.014038000255823135 0.014038000255823135 0.014159999787807465 0.014159999787807465 0.014200999867171049 0.014241000171750784 0.014241000171750784 0.014282000251114368 0.014322999864816666 0.014323000330477953 0.014444999862462282 0.014444999862462282 0.014525999780744314 0.014526000246405602 0.014688999857753515 0.014688999857753515 0.0147299999371171 0.014810999855399132 0.014812000095844269 0.01485099969431758 0.01485099969431758 0.014851999934762716 0.014891999773681164 0.014973999932408333 0.015015000011771917 0.015054999850690365 0.01509599993005395 0.015177000313997269 0.015299000311642885 0.015420999843627214 0.01554399961605668 0.01582799991592765 0.015949999913573265 0.015951000154018402

Current
Mean: 0.014 ms
Stdev: 0.001 ms (5.4%)
Runs: 0.012614000122994184 0.012858000118285418 0.01293900003656745 0.013061999808996916 0.013062000274658203 0.0131029998883605 0.013183000031858683 0.013183999806642532 0.013264999724924564 0.013306000269949436 0.013428000267595053 0.013428000267595053 0.013548999559134245 0.013549999799579382 0.013549999799579382 0.01358999963849783 0.013591000344604254 0.013630999717861414 0.013630999717861414 0.013711999636143446 0.013875000178813934 0.013875000178813934 0.013957000337541103 0.0139979999512434 0.014037999790161848 0.014038000255823135 0.014039000030606985 0.014078999869525433 0.014119999948889017 0.014159999787807465 0.014159999787807465 0.014159999787807465 0.014161000028252602 0.014240999706089497 0.014240999706089497 0.014281999785453081 0.014281999785453081 0.014283000025898218 0.014405000023543835 0.01444500032812357 0.014485999941825867 0.014566999860107899 0.014649000018835068 0.014649000018835068 0.01472900016233325 0.014771000016480684 0.014810999855399132 0.014852000400424004 0.014891999773681164 0.01509599993005395 0.015096999704837799 0.015137000009417534 0.015217999927699566 0.01538099953904748 0.015461999922990799 0.01554399961605668 0.015746999997645617 0.015909000299870968
App start nativeLaunch Baseline
Mean: 22.255 ms
Stdev: 2.698 ms (12.1%)
Runs: 18 18 18 19 19 19 19 19 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 23 23 24 24 24 24 24 25 25 25 27 28 28 28 28 29

Current
Mean: 20.860 ms
Stdev: 1.550 ms (7.4%)
Runs: 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 23 23 23 23 24 24 24 25 25

@github-actions
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

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

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀

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

@OSBotify
Copy link
Contributor

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

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

1 similar comment
@OSBotify
Copy link
Contributor

OSBotify commented Nov 1, 2023

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

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

@OSBotify
Copy link
Contributor

OSBotify commented Nov 2, 2023

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀

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

}
const subscribed = childReportNotificationPreference !== 'hidden';
const isCommentAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && !ReportUtils.isThreadFirstChat(reportAction, reportID);
const isReportPreviewAction = reportAction.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW;
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming from #35092, we shouldn’t display the leave thread option for IOU previews

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 Ready To Build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants