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

perf: make switch between chat list and workspaces smoother #36420

Merged
merged 68 commits into from
Apr 25, 2024

Conversation

hurali97
Copy link
Contributor

@hurali97 hurali97 commented Feb 13, 2024

Details

This PR only makes the switch between Chat lists and Settings->Workspaces smoother. Fix of the hang behaviour which happens sometimes when switching, is fixed by #36927.

Fixed Issues

$ #35704
PROPOSAL: #35704 (comment)

Tests

  • Log in on Web

  • Switch between Chat list and Settings->Workspaces

  • Switch should be smooth

  • 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: 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 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 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.
  • 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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov

@hurali97
Copy link
Contributor Author

@hayata-suenaga @mountiny

Hey folks 👋

As of now, I have implemented the following improvements:

  • use React.Context for sharing of reports by creating a hook called useReports

  • maintain the ordered report IDs by extracting them out as a shared resource in a hook called useOrderedReportIDs

  • remove the cache mechanism from SidebarUtils.getOrderedReportIDs ( This overlaps with an ongoing effort from callstack in context of improving SidebarUtils.getOrderedReportIDs, so we will remove this if callstack's PR gets merged first )

  • improve renderItem in LHNOptionsList by generating all of the required data before hand. This means altering the implementation of SidebarUtils.getOrderedReportIDs. This is made possible by only altering the return value from this function, so instead of returning reportIDs array, we now return the array of objects required for rendering items.


The above points give us important improvements but the hanging issue still happens on web. I was investigating why after making the renderItem lightweight, we still face the hang behaviour. To my surprise, when we are switching from chat list to settings->workspaces, FlashList renders 15k of the items and hangs the main thread which doesn't allow us to use any UI elements.

While we are on the chat list, the FlashList renders around ~100 items which is understandable but when we are navigating to workspaces, why is it rendering 15k items? This doesn't make any sense because in the first place there should not be any rendering happening on the component that is about to be unmounted. Since the web app was hanging with FlashList, so I switched to FlatList to investigate this further.

FlatList rendered around 600 items based on it's virtualization, while I was on chat list. But when I navigated to workspaces, there was no hang and no items was being rendered. But then I switched back to chat list and came back to workspaces, there was around 10 items being rendered twice by FlatList.

Upon investigating with React DevTools there was memoisation missing in the SidebarLinks and LHNOptionsList. Adding that, reduced it to 10 items being rendered only once. This still doesn't sound good, There should be no render when navigating away from a screen/component. I was going through the navigation we have setup and I have some doubt that there might be something related to it. Because in DevTools, when switching from chat list to workspaces, I saw the tree being rendered from Navigation and when inspecting Home screen or Sidebar, it showed me that this is the first time this component is rendering. That's not true because the component was already rendered. My doubt is that there is certain unmount/mount happening, which needs to be investigated further.

As things stands, FlatList doesn't render any item when switched to workspaces for the first time. But on subsequent switches, it renders 10 items once, falsely. Which means it shouldn't be rendering when we are navigating from chat list to workspaces. However, FlashList renders 15k items falsely. Below you can find some videos, with FlashList blocking the main thread and FlatList being smooth:

FlashList on Web

flashlist.mp4

FlatList on Web

flatlist.mp4

On Android, things are a bit different. FlashList renders around 20 items falsely. While FlatList doesn't render any item falsely. Also, both doesn't render any item on subsequent switches.

FlashList on Android

flashlist-android.mov

FlatList on Android

flatlist-android.mov

Where to from now? I know that we have invested a lot in FlashList and I would want to find the root cause why FlashList is behaving weirdly on web. This is rather complicated as FlashList is built on top of RecyclerListView. So it's not going to be an easy job finding the root cause. Because there's a chance that this might be coming from RecyclerListView.

If we have time at our hands, we can dedicate our effort to find the root cause and fix it as required but this would mean that we have to deal with hanging behaviour until this is fixed.

Additionally, we can move forward with FlatList on Web only and still use FlashList on native platforms and dedicate someone to investigate the root cause in FlashList/ RecyclerListView. I would want to investigate this but from next week I am on vacations.

The reason why FlashList renders 15k items and FlatList renders only 10 items, when navigating away from chat list is that FlatList utilises Virtualization and based on initialNumToRender it renders only 10 because it's a default number. The FlashList leverages recycling and initialNumToRender has no affect because the underlying implementation is different, see below.

Screenshot 2024-02-15 at 8 07 47 PM

To summarise, we have two areas to investigate:

  • The Expensify navigation because when switching tabs subsequently on chat list with FlatList implementation, we have 10 items rendered and the expected is no items should be rendered because we are navigating away from chat list. This is also relevant in the case of FlashList as it also renders 15k items when navigating away from chat list.

  • Investigate the root cause why FlashList hangs and renders 15k items when we are navigating away from that component.

    • There's a chance that this might be solved with the first point, so starting from the first one makes more sense.

Let me know what you guys think and then I will proceed with that tomorrow, if it's doable in one day. I will also include someone from callstack in this thread tomorrow 👍

@mountiny
Copy link
Contributor

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

I'm pretty skeptical about introducing some of the patterns from this PR without broader discussion. For example, I believe useOrderedReportIDs and useReports could leverage the (coming-soon) useOnyx hook: Expensify/react-native-onyx#462. Personally I'd rather wait for that hook to land before adding a useReports hook that adds a new context and leverages Onyx.connect internally.

Another piece of feedback is that this PR has a number of different changes and could be more effective if split up into a series of smaller, more focused PRs.

@mountiny
Copy link
Contributor

Another piece of feedback is that this PR has a number of different changes and could be more effective if split up into a series of smaller, more focused PRs.

For context, that is the plan. The full PR was created so benchmarks/ measurements could be taken altogether as the individual changes might not have very noticeable change in their own.

Personally I'd rather wait for that hook to land before adding a useReports hook that adds a new context and leverages Onyx.connect internally.

Niiice, yeah I agree it would be great to use that hook. I was not aware we are getting so close with that implementation. Thanks!

@hayata-suenaga hayata-suenaga self-requested a review February 15, 2024 18:00
@hayata-suenaga
Copy link
Contributor

@roryabraham thank you for reminding us of the upcoming hook.

Does the new hook useOnyx use a context? Are we going to introduce a Onyx Context to the entire app?

@tgolen
Copy link
Contributor

tgolen commented Feb 15, 2024

It sounds like this PR allowed the investigation to get much closer to the root cause of the problem (the re-rendering). I think this is the best next step:

The Expensify navigation because when switching tabs subsequently on chat list with FlatList implementation, we have 10 items rendered and the expected is no items should be rendered because we are navigating away from chat list. This is also relevant in the case of FlashList as it also renders 15k items when navigating away from chat list.

I also am not a fan of rolling out a change this big just so web can use flatlist for now. I think keeping the bug live gives us a good #urgency to get this problem solved.

@hurali97
Copy link
Contributor Author

Thank you guys, really appreciate your input 👍

To summarise, we are moving forward with the following action items:

  • Investigate the navigation issue ( why list renders when we navigate away from the component )

  • Refactor the PR work to use the new Onyx hooks instead of Context but we have to wait until those are landed

  • Investigate the FlashList rendering of 15k items when navigating away from the screen. ( Potentially related to point 1 )

  • I will also try to use Eduardo's suggestion here today

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Feb 16, 2024

is the performance issue stemming from the fact that withOnyx HOC creates a reference to the wrapped component? Or is it stemming from the fact that we're creating several instances of reports object?

if the former is the case, I see how using the new useOnyx hook will solve the issue. But if the latter is the issue, I don't think using the new hook won't solve the issue.

Investigate the navigation issue ( why list renders when we navigate away from the component )

anyway, this seems to be the priority ⬆️ This task will be worked on by Adam from Software Mansion as part of this GH ticket

@perunt
Copy link
Contributor

perunt commented Feb 19, 2024

@hurali97, is it reproducible on each switching to the settings page?

@perunt perunt mentioned this pull request Feb 20, 2024
50 tasks
@jbroma
Copy link
Contributor

jbroma commented Feb 21, 2024

@hurali97, is it reproducible on each switching to the settings page?

Hi @perunt,

I took over this PR since Hur is OOO for the rest of the week,

FYI I am unable to reproduce this reliably on every switch.

@jbroma
Copy link
Contributor

jbroma commented Feb 22, 2024

After some intensive digging, here's the update from me:

The issue with rendering the 15k list items when switching from LHN to settings was very hard to reproduce - I only managed to observe it few times over the last 2 days. That said, it's not really worth digging much deeper into as it's 99% related to Flashlist internals and how it works on the web (support is in beta as stated on their docs).

It's a symptom, rather than a cause, namely because of LHN screen not being properly frozen and triggering rerenders on focus change, when the screen goes out of view. This is something we pointed out in the Report Screen Load Time analysis as part of the Performance Audit done by Callstack. The fix from @perunt looks great on web - removing the pre-rendering completely thus removing the possibility of the issue happening in the first place and allows us to retain Flashlist on the web. On mobile, changes in the FreezeWrapper don’t cause any issue whatsoever.

With that potentially settled, the only agreed action point for this PR that remains is using the useOnyx hook which is still not available as of now.

As for the second issue, where the app hangs during LHN load after chat switch, I wasn’t able to reproduce it even once. I suspect it might be the same case as above, where the list wasn’t frozen and in rare cases rendered 15k items.

Let me know whether I should cleanup this PR & revert to using Flashlist.

CC @mountiny @hayata-suenaga

@hayata-suenaga
Copy link
Contributor

@jbroma thank you so much for providing the update.

So the remaining optimization task is to use useHook once it's implemented, right?

I love the work you put into this PR, but do we need to keep this PR? Are fixes addressed in other PRs enough to solve the major causes of the performance issues?

@jbroma
Copy link
Contributor

jbroma commented Feb 22, 2024

@hayata-suenaga not sure about abandoning this PR yet, I'll summarise the things done in this PR in more detail so they can be picked into separate small PRs. I'll post that in a separate comment.

This PR could stay and contain just the changes with the hooks and contexts. Let me know what you think about that.

The hanging issue looks closely related to the freeze mechanism and Flashlist going haywire, which matches with the fact that it hangs only occasionally and is therefore hard to reproduce, which should solve the issue. Having said that, reduced memory consumption via hooks and contexts in this PR is also an important factor.

@jbroma
Copy link
Contributor

jbroma commented Feb 22, 2024

Done in this PR:

  • Making LHNOptionsList renderItem lighter by generating all of the required data before hand
  • Context & hook for orderedReportIDs
  • Context & hook for reports
  • SidebarLinks memo
  • Cache removal from getOrderedReportIDs (link the PR in which it is removed)
  • WorkspacesListPage Flatlist prop fix

Changes made obsolete by freeze fix

  • FlatList on web
  • withCurrentReportID adjustment

@hayata-suenaga
Copy link
Contributor

@jbroma thank you for much. Because there are so many conversations going on about this performance issue, your summary of this PR is really helpful ❤️ I have some questions.

Context & hook for reports

I think @mountiny expressed concerns about this part that this might introduce a new pattern of creating a context for each Onyx key. I know we're just doing this for report, but if we can avoid this, I think that would be nice. Otherwise, other engineers might prematurely start doing this for other Onyx keys.

I think we're also discussing about using useOnyx once it's implemented. Will this strategy (creating a context) be replaced by the plan to use the useOnyx hook?

Context & hook for orderedReportIDs

Could you remind me why we're creating a context for orderedReportIDs? I do remember that the calculation for this value is expensive. But I also remember that we can't memorize this value because the parameters used for the calculation will constantly change (that's why we removed the cache, right?). Cold you explain how creating a context will help our performance issues?

I think we can definitely keep the following items:

  • Making LHNOptionsList renderItem lighter by generating all of the required data before hand
  • SidebarLinks memo
  • WorkspacesListPage Flatlist prop fix

And if I remember this correctly, we already merged a PR for the following item, right...?

  • Cache removal from getOrderedReportIDs

@jbroma
Copy link
Contributor

jbroma commented Feb 23, 2024

@hayata-suenaga first of all, I totally agree with the concerns about this pattern and it is indeed not a preferred a way of doing things.

When it comes to reports it used in a lot of places and reusing the reports object saves a lot memory. Ideally, it would be nice to subscribe to a collection in a read-only mode but I'm not sure this is possible within the Onyx. Subscription in read-only mode would allow the share an instance between subscriptions and that would remove the need for context in this case. useOnyx will not help much in this case.

When it comes to useOrderedReportIDs, the context here is solely used for the withOnyx HOC. Since getOrderedReportIDs is used in only one place, there is no need for context in this case. Introduction of useOnyx hook will remove the need for this context entirely.

Cache removal from getOrderedReportIDs was indeed merged in this PR

@hayata-suenaga
Copy link
Contributor

@jbroma thank you so much for the explanation.

Reading closely your explanation about the need for creating a context for reports object, I agree with you.

The only question remaining is about your statement about read-only mode. What do you mean by this? Does Onyx provide read-only mode and read-and-write mode?

Anyway, this question is not a blocker. Once you'r ready, could you remove the following parts from this PR?

Changes made obsolete by freeze fix

FlatList on web
withCurrentReportID adjustment

And we can bench mark performance improvement against this PR. Do you think that is a good plan?

@jbroma
Copy link
Contributor

jbroma commented Feb 23, 2024

@hayata-suenaga read-only mode is just something I thought of, pretty sure there is nothing like that inside of Onyx, but it might be something worth investigating in more depth.

I'll resume work on this PR on Monday.

@mountiny
Copy link
Contributor

@jbroma I am ooo for 2 weeks, but it would be great to first post a Proposal with Problem / Solution format if we want to add a context for onyx key, that is quite a substantial shift in approach and such change would need to be highlighted more publicly to get agreement on this solution/ path forwards being the best at this point for the given problem.

If you do not want to create context here and apply more subtle improvements then its fine to just implement it

@hurali97
Copy link
Contributor Author

hurali97 commented Feb 26, 2024

Could you remind me why we're creating a context for orderedReportIDs? I do remember that the calculation for this value is expensive. But I also remember that we can't memorize this value because the parameters used for the calculation will constantly change (that's why we removed the cache, right?). Cold you explain how creating a context will help our performance issues?

@hayata-suenaga The reason we extracted the calculation for orderedReportIDs to a context is because when we switch between the chat lists and workspaces, the SidebarScreen re mounts and as a result we are re-calculating the same value each time we switch back to the chat list. To prevent this, I thought of putting the calculation in useOrderedReportIDs so that we don't re-calculate each time we switch back and we render quickly because we will have data ready upfront.

@jbroma Great work continuing this PR ❤️ . I will stay active, in-case you need anything from me/ any comments.

I have also tested this PR and the one here which improves the freeze wrapper and these two PRs work great together. See here.

@hayata-suenaga
Copy link
Contributor

@jbroma thank you for taking over this issue from Muhammad

if you think we still need a context for the reports object, please post a message in the #expensify-open-source channel or #newdot-performance channel with a problem/solution statement as Vit described above.

I'm still having trouble understanding the need for orderedReportIDs 😓 Because this value won't destroyed when the user navigates to the Settings screen (the chat list screen will freeze, but the state will be preserved?), I was assuming creating a context won't make any change?

@hurali97
Copy link
Contributor Author

@hayata-suenaga @roryabraham PR is ready to be re-reviewed after the usage of useOnyx 👍

hayata-suenaga
hayata-suenaga previously approved these changes Apr 24, 2024
Copy link
Contributor

@hayata-suenaga hayata-suenaga left a comment

Choose a reason for hiding this comment

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

Other than this comment, the PR looks good to me. Thank you for pushing this PR forward!

@hayata-suenaga
Copy link
Contributor

@situchan is OOO and @ntdiary was unassigned from the issue because of the lack of the bandwidth.
I'll ask if any C+ is available for the final review. -> https://expensify.slack.com/archives/C02NK2DQWUX/p1713981034159319

@mananjadhav
Copy link
Collaborator

Reviewing this as per the message

@mananjadhav
Copy link
Collaborator

Just finished with the code review, this is the last pending comment. I am going to start with the checklist as this is a small comment to fix.

@mananjadhav
Copy link
Collaborator

When I tried testing this on Web, I see chat not found error.

Screen.Recording.2024-04-25.at.3.04.44.AM.mov

@hurali97
Copy link
Contributor Author

When I tried testing this on Web, I see chat not found error.

Screen.Recording.2024-04-25.at.3.04.44.AM.mov

@mananjadhav I couldn't repro it on the local. As far as I recall, I have faced the similar issue on main as well but that happens randomly.

repro.mp4

@mananjadhav
Copy link
Collaborator

Let me take a look at this again.

@mananjadhav
Copy link
Collaborator

I was still able to reproduce this consistently on one account on Web and Desktop. But after I signout and signin again, it works fine. I am not sure if this is an issue, because I can't reproduce it on other accounts that were already logged in. Hence, I am going to continue with the checklist.

web-chat-switcher-bug.mov
desktop-chat-switcher-bug.mov

@mananjadhav
Copy link
Collaborator

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 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.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(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.
  • 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
mweb-chrome-switcher.mov
iOS: Native
ios-chat-switcher.mov
iOS: mWeb Safari
mweb-safari-chat-switcher.mov
MacOS: Chrome / Safari
web-chat-switcher-bug.mov
MacOS: Desktop
desktop-chat-switcher.mov

@roryabraham roryabraham merged commit 6fb4c78 into Expensify:main Apr 25, 2024
16 checks passed
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.67-0 🚀

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

@Julesssss
Copy link
Contributor

Hey all, there are a lot of changes here so I can't be sure, but it seems possible that this new issue: 'App redirects to Concierge chat when leaving group chat and room' might have been introduced by these changes. Would you mind taking a look at the issue please

@hurali97
Copy link
Contributor Author

@Julesssss I will try look into this 👍

@Julesssss
Copy link
Contributor

Hey @hurali97, thanks but no need. I just confirmed the issue doesn't occuron the commit where this PR was merged to main 👍

@hurali97
Copy link
Contributor Author

hurali97 commented Apr 29, 2024

@Julesssss Thanks for letting me know 👍

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀

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