Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-50576 Remove most places where a selector factory isn't reused properly #12230

Merged
merged 4 commits into from
Mar 3, 2023
Merged

MM-50576 Remove most places where a selector factory isn't reused properly #12230

merged 4 commits into from
Mar 3, 2023

Conversation

hmhealey
Copy link
Member

@hmhealey hmhealey commented Feb 17, 2023 •
edited
Loading

There's a few places where we're improperly using selector factories and creating new instances of those selectors whenever anything in the store changes. This is exacerbating some other performance issues we're investigating since 5-20% of the time taken during certain actions is being spent on making new selectors.

01 generateId

The ones that are changed here are:

  1. makeGetThreadOrSynthetic (used by DotMenu and RhsHeaderPost)
  2. makeIsPostCommentMention (used by Post)
  3. makeGetCommentCountForPost (used by Post and DotMenu)
  4. makeGetCategory (used by OverageUsersBanner and DelinquencyModal)

makeGetUserTimezone is mentioned in that screenshot above, but it'll be fixed in a followup PR.

Ticket Link

https://mattermost.atlassian.net/browse/MM-50576

Related Pull Requests

#12231

Release Note

Minor performance improvements

@hmhealey hmhealey added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Feb 17, 2023
@mattermost-build
Copy link
Contributor

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

@hmhealey hmhealey mentioned this pull request Feb 17, 2023
Closed
@github-actions
Copy link

github-actions bot commented Feb 17, 2023 •
edited
Loading

Test Results

       1 files     814 suites   20m 30s ⏱️
6 925 tests 6 924 ✔️ 1 💤 0 ❌
7 101 runs  7 100 ✔️ 1 💤 0 ❌

Results for commit fe537ba.

♻️ This comment has been updated with latest results.

@hmhealey
Copy link
Member Author

/e2e-tests

@mattermost-build
Copy link
Contributor

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

@hmhealey hmhealey marked this pull request as ready for review February 21, 2023 19:57
@hmhealey
Copy link
Member Author

Comparing those E2E test results against https://mattermost-cypress-report.s3.amazonaws.com/304278-onprem-ent-master-mm-ee-test:test/mochawesome.html, none of those test failures seem to be exclusive to this PR

@neallred neallred added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 21, 2023
neallred
neallred approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@neallred neallred left a comment

Choose a reason for hiding this comment

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

Scanning through the change, I can confirm the only changes are to factor out the selector factories and that the changes match the purposes and expectation of https://react-redux.js.org/api/connect#factory-functions . After manual smoke testing, it also seems like everything still works as expected. LGTM, thanks @hmhealey !

components/post/index.tsx
const rhsState = getRhsState(state);
if (
rhsState !== RHSStates.PIN && // Not show in pinned posts since they are all for the same channel
!isDMorGM && // Not show for DM or GMs since they don't belong to a team
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name makes me think of De Morgans' laws, more so because its used in a bunch of chained logic statements 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh boy. It's been a while since I've thought of De Morgans' laws 😅

components/announcement_bar/overage_users_banner/index.tsx Outdated Show resolved Hide resolved
@mm-cloud-bot mm-cloud-bot removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Feb 21, 2023
@mm-cloud-bot
Copy link

Test server creation failed. See the logs for more information.

koox00
koox00 reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@koox00 koox00 left a comment

Choose a reason for hiding this comment

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

💯

@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Feb 23, 2023
@hmhealey hmhealey requested a review from stevemudie March 1, 2023 20:54
@hmhealey hmhealey removed their assignment Mar 1, 2023
@hmhealey
Copy link
Member Author

hmhealey commented Mar 1, 2023

/e2e-tests

@mattermost-build
Copy link
Contributor

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

@hmhealey
Copy link
Member Author

hmhealey commented Mar 3, 2023

I looked over the E2E results, and the only test that isn't failing on this nightly run or flaky is MM-T713 Post time should render correct format and locale. That's also failing on the version of master I have checked out locally as well, so it might have been fixed since then since it's passing in the run linked above

@hmhealey hmhealey removed the 3: QA Review Requires review by a QA tester label Mar 3, 2023
@hmhealey hmhealey added the QA Deferred Agreement with QA that these changes will be tested post-merge label Mar 3, 2023
@mattermost-build
Copy link
Contributor

You don't have permissions to trigger this command.
It's only available for organization members.

@hmhealey hmhealey added the 4: Reviews Complete All reviewers have approved the pull request label Mar 3, 2023
@hmhealey hmhealey merged commit fc71abd into master Mar 3, 2023
@hmhealey hmhealey deleted the MM-50576_fix-selectors branch March 3, 2023 21:11
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Changelog/Done Required changelog entry has been written and removed Changelog/Not Needed Does not require a changelog entry labels Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@koox00 koox00 koox00 approved these changes

@neallred neallred neallred approved these changes

@stevemudie stevemudie Awaiting requested review from stevemudie

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation QA Deferred Agreement with QA that these changes will be tested post-merge release-note
Projects
None yet
Milestone
No milestone
6 participants
@hmhealey @mattermost-build @mm-cloud-bot @koox00 @neallred @amyblais