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

MM-36696: global threads list now virtualized #8350

Merged
merged 10 commits into from
Jul 19, 2021

Conversation

koox00
Copy link
Contributor

@koox00 koox00 commented Jul 6, 2021

Summary

Global threads list is virtualized to avoid rendering all thread
items at once.
This reduces the time needed to enter the threads route.

Ticket Link

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

Release Note

NONE

Global threads list now is virtualized to avoid rendering all thread
items at once.
This reduces the time needed to enter the threads route.
@koox00 koox00 added 2: Dev Review Requires review by a core commiter Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 6, 2021
Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

non-blocking comments

const OVERSCAN_COUNT_BACKWARD = 4;
const OVERSCAN_COUNT_FORWARD = 4;

const virtListStyles = {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to have the style inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DynamicSizeList accepts a style prop and not a class one.
My guess would be that since there are no stylesheets included in the module, there are only inline styles,
we pass inline styles so we can properly override if needed(?).

selectedThreadId={selectedThreadId}
/>
{currentFilter === ThreadFilter.unread && !someUnread && isEmpty(unreadIds) ? (
<NoResultsIndicator
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be either the threadlist or the noresults? why nesting the no result indicator inside a list that won't be populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Willyfrog it's the noresults for the unreads tab. So it's not like nesting in the list, but nesting in the left pane.
Would renaming the component from thread_list to thread_list_pane make it clearer?

PS:
We have 3 "noresults" cases:

  1. whole global threads page (when not following any thread)
  2. right pane (when there is no selected thread)
  3. left pane (threads list) unreads tab (when we have no unread threads) <--- the case we are discussing

// so we need to reverse the ids before feeding the list
// so that we get the correct order afterwards.
//
// TODO: have dynamic-virtualized-list to maybe accept a direction prop
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this should be an issue in the repo instead of a todo here

Copy link
Member

@hmhealey hmhealey Jul 9, 2021

Choose a reason for hiding this comment

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

Note that the repo was created originally by Sudheer as a fork of react-window, and since he left, I don't think anyone else has been maintaining it.

Edit: Also, that's funny that it works that way since I'm pretty sure the reason it does that is because we send the posts to the client sorted backwards (newest to oldest) which honestly has caused a lot of confusion in the post storage code in the past. I wonder if we should just store posts oldest to newest and then update the list component to never reverse the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: Also, that's funny that it works that way since I'm pretty sure the reason it does that is because we send the posts to the client sorted backwards (newest to oldest) which honestly has caused a lot of confusion in the post storage code in the past. I wonder if we should just store posts oldest to newest and then update the list component to never reverse the list.

@hmhealey, it seems that it doesn't work :( I cannot find a way to add items at the top...
items added at the bottom (0 index) are getting rendered, but items added at the top (last index) don't...

I'd like to add back react-window for this case.

@Willyfrog Willyfrog requested a review from hmhealey July 7, 2021 09:58
@esethna esethna added the 1: UX Review Requires review by a UX Designer label Jul 8, 2021
@esethna esethna added this to the v5.38.0 milestone Jul 8, 2021
@matthewbirtch
Copy link
Contributor

@koox00 this seems great to me, but I'm not sure the test server is the best place to test on since it doesn't have a lot of data yet.

Is there anything specific from a UX perspective that I should pay close attention to in the review? I'm not seeing anything glaring

@esethna
Copy link
Contributor

esethna commented Jul 8, 2021

@matthewbirtch @koox00 do we want to add a loading indicator at the bottom of the list if you scroll down and threads are still fetching? I throttled my connection to "slow 3G" then scrolled down really fast and there was no indication that more threads were loading in until they were returned. In the center channel we have the three dots pulsing animation

@esethna
Copy link
Contributor

esethna commented Jul 8, 2021

This can be important for slow or unstable connections ^

@koox00
Copy link
Contributor Author

koox00 commented Jul 8, 2021

@matthewbirtch @koox00 do we want to add a loading indicator at the bottom of the list if you scroll down and threads are still fetching? I throttled my connection to "slow 3G" then scrolled down really fast and there was no indication that more threads were loading in until they were returned. In the center channel we have the three dots pulsing animation

@esethna the "infinite" loading will be added later on, this PR is just to improve rendering performance of the list so that the time to enter threads view is reduced.

@esethna
Copy link
Contributor

esethna commented Jul 8, 2021

Ah got it, thanks @koox00

@esethna esethna removed the request for review from matthewbirtch July 8, 2021 18:54
@esethna esethna removed the 1: UX Review Requires review by a UX Designer label Jul 8, 2021
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

One non-blocking comment, but otherwise this looks good to me.

Copy link
Member

@calebroseland calebroseland left a comment

Choose a reason for hiding this comment

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

LGTM overall. Seems some memoization (ThreadItem) may be negated: when you mark as unread/read, there's a brief flash and a slight delay.

1st half current, 2nd half rerendering
CleanShot 2021-07-09 at 11 42 14

@hmhealey hmhealey self-requested a review July 9, 2021 17:59
Mostly refactors the use of selectors to avoid re-rendering much.

- Selectors that are not memoized (via let's say createSelector)
  should pase shallowEqual to useSelector hook
- Selectors which take an object as an argument should memo that object
  so that they don't re-render on every re-render, since the custom
  object would be created each time regardless if the property at hand
  hadn't changed

Other changes unrelated to performance:

- spans should not contain divs ideally
- ThreadItem now accepts a style prop to be feeded by Virtualized list
- ThreadViewer had shouldComponentUpdate so it can't extend PureComponent
  in the same time

PS: react-dev-tools' profiler showed that a lot of re-renders are
happening due to "hooks changed" so I tried to tackle them,
but it's not specific on which hooks changed,
so it could be useContext which redux uses or the react ones or our custon ones.
@koox00
Copy link
Contributor Author

koox00 commented Jul 13, 2021

@hmhealey, @calebroseland I've added some changes to improve performance, can you take a look?

This commit replaces DynamicSizeList with react-window's FixedSizeList.
The reason is mostly dynamic-virtualized-list doesn't support adding lists at the top.

The most notable change is that all thread items now have the same height.
@koox00
Copy link
Contributor Author

koox00 commented Jul 14, 2021

/update-branch

Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

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

Not seeing anything glaring from my tests. Seems to work/look great.

@esethna esethna removed the 1: PM Review Requires review by a product manager label Jul 14, 2021
@esethna esethna removed the 1: UX Review Requires review by a UX Designer label Jul 14, 2021
@esethna esethna requested a review from jgilliam17 July 14, 2021 18:52
@esethna esethna added the 3: QA Review Requires review by a QA tester label Jul 14, 2021
Copy link
Member

@calebroseland calebroseland left a comment

Choose a reason for hiding this comment

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

LGTM!

@esethna esethna removed the 2: Dev Review Requires review by a core commiter label Jul 15, 2021
@esethna esethna removed the request for review from hmhealey July 15, 2021 18:16
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thanks @koox00
Tested, looks good to merge.

  • Verified opening long global threads list, list renders as expected.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 16, 2021
@mm-cloud-bot
Copy link

Test server destroyed

@jgilliam17
Copy link
Contributor

@koox00 I didn't merge this due to conflicts, can you please take a look? Thanks.

@koox00 koox00 merged commit cafeacf into mattermost:master Jul 19, 2021
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Jul 19, 2021
@amyblais
Copy link
Member

/cherry-pick cloud

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit to mattermost-build/mattermost-webapp that referenced this pull request Jul 20, 2021
* MM-36696: global threads virtualized list

Global threads list now is virtualized to avoid rendering all thread
items at once.
This reduces the time needed to enter the threads route.

* Fixes tests

* Minor fixes

* Fixes performance issues in global threads

Mostly refactors the use of selectors to avoid re-rendering much.

- Selectors that are not memoized (via let's say createSelector)
  should pase shallowEqual to useSelector hook
- Selectors which take an object as an argument should memo that object
  so that they don't re-render on every re-render, since the custom
  object would be created each time regardless if the property at hand
  hadn't changed

Other changes unrelated to performance:

- spans should not contain divs ideally
- ThreadItem now accepts a style prop to be feeded by Virtualized list
- ThreadViewer had shouldComponentUpdate so it can't extend PureComponent
  in the same time

PS: react-dev-tools' profiler showed that a lot of re-renders are
happening due to "hooks changed" so I tried to tackle them,
but it's not specific on which hooks changed,
so it could be useContext which redux uses or the react ones or our custon ones.

* Uses react-window for virtualized thread list

This commit replaces DynamicSizeList with react-window's FixedSizeList.
The reason is mostly dynamic-virtualized-list doesn't support adding lists at the top.

The most notable change is that all thread items now have the same height.

* Revert needless "optimization"

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
(cherry picked from commit cafeacf)
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jul 20, 2021
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jul 20, 2021
amyblais pushed a commit that referenced this pull request Jul 20, 2021
* MM-36696: global threads virtualized list

Global threads list now is virtualized to avoid rendering all thread
items at once.
This reduces the time needed to enter the threads route.

* Fixes tests

* Minor fixes

* Fixes performance issues in global threads

Mostly refactors the use of selectors to avoid re-rendering much.

- Selectors that are not memoized (via let's say createSelector)
  should pase shallowEqual to useSelector hook
- Selectors which take an object as an argument should memo that object
  so that they don't re-render on every re-render, since the custom
  object would be created each time regardless if the property at hand
  hadn't changed

Other changes unrelated to performance:

- spans should not contain divs ideally
- ThreadItem now accepts a style prop to be feeded by Virtualized list
- ThreadViewer had shouldComponentUpdate so it can't extend PureComponent
  in the same time

PS: react-dev-tools' profiler showed that a lot of re-renders are
happening due to "hooks changed" so I tried to tackle them,
but it's not specific on which hooks changed,
so it could be useContext which redux uses or the react ones or our custon ones.

* Uses react-window for virtualized thread list

This commit replaces DynamicSizeList with react-window's FixedSizeList.
The reason is mostly dynamic-virtualized-list doesn't support adding lists at the top.

The most notable change is that all thread items now have the same height.

* Revert needless "optimization"

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
(cherry picked from commit cafeacf)

Co-authored-by: Kyriakos Z <3829551+koox00@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation release-note-none
Projects
None yet
10 participants