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: use 'react-window' for contact list #3842

Merged
merged 2 commits into from
May 23, 2024

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented May 21, 2024

  • Need to clean up, address the TODOs
  • Would also need to verualize it in CreateChatMain
  • And perhaps separate this into a new component, since we might need to implement lazy loading later (with 'react-window-infinite-loader') (but I guess that's a problem for a future us)
  • Personally test it

For huge contact lists (5000 contacts), rendering took ~5 seconds.
This solves this problem by only actually rendering the
part of the list that is currently scrolled to.

We already have this implemented for ChatList.tsx

Parially addresses #1830
Why partially? Because BackendRemote.rpc.getContacts()
still takes 3 seconds for 5000 contacts

This commit has side effects:

  • a negative: the affected contact lists on non-default app themes
    where the item height is not exactly 64px
    are now slightly more ugly. See TODO about itemSize={64}
  • a (positive?) side effect: AddMemberChipsWrapper
    is now outside the scrollable region (i.e. it's always on top)
    It works fine since it's max-height: 33%

move `CreateChat.tsx` to a subdirectory
@WofWca WofWca force-pushed the wofwca/virtualize-contact-list branch 2 times, most recently from 6b301ee to e06c30b Compare May 22, 2024 10:22
@WofWca WofWca marked this pull request as ready for review May 22, 2024 10:23
@WofWca WofWca force-pushed the wofwca/virtualize-contact-list branch from e06c30b to 0b97f11 Compare May 22, 2024 10:40
@WofWca WofWca changed the title WIP: perf: use 'react-window' for contact list perf: use 'react-window' for contact list May 22, 2024
For huge contact lists (5000 contacts), rendering took ~5 seconds.
This solves this problem by only actually rendering the
part of the list that is currently scrolled to.

We already have this implemented for `ChatList.tsx`

Parially addresses #1830
Why partially? Because `BackendRemote.rpc.getContacts()`
still takes 3 seconds for 5000 contacts

This commit has side effects:
- a negative: the affected contact lists on non-default app themes
    where the item height is not exactly 64px
    are now slightly more ugly. See TODO about `itemSize={64}`
- a (positive?) side effect: `AddMemberChipsWrapper`
    is now outside the scrollable region (i.e. it's always on top)
    It works fine since it's `max-height: 33%`
@WofWca WofWca force-pushed the wofwca/virtualize-contact-list branch from 0b97f11 to 670ed8c Compare May 22, 2024 11:09
Comment on lines +9 to +15
/**
* Make sure not to render all of the user's contacts with this component.
* Some users might have thousands of contacts, which can result in
* this component taking seconds to render.
* Instead consider using 'react-window', like we do in the `CreateChat`
* component.
*/
Copy link
Member

@Simon-Laux Simon-Laux May 23, 2024

Choose a reason for hiding this comment

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

I think it would make sense to have the list of Create chat also as component.

In the long term it would also make sense to move this ContactList component to use react-window and lazy loading, though it might be a bit more involved because of the extra features (checkboxes, remove button and so on).

maybe we should rename ContactList to sth like ContactListForEdit or some better name that reflects the extra features when we have a dedicated virtual contact list.

Though maybe it's too much work for now to generalise the virtual list from CreateChat to it's own component because there is the question of how to best represent the pseudo/fake contact items in the api of such an component.

Copy link
Member

@Simon-Laux Simon-Laux May 23, 2024

Choose a reason for hiding this comment

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

Maybe we could have some contact list component with some api like

interface PseudoItem {
   icon: IconName,
   label: string,
   clickAction: ()=>void,
   contextMenu?: ContextMenuItem[]
}

return <ContactList
   extraItemsInBeginning={PseudoItem[]}
   contactIds={number[]}
   /* contact data would be fetched by this component on demand
       and also cached in it (+ event listener for contact changed event) */
>
   { (contact: Contact) => <Contact contact={contact }/> }
   // so you could replace the contact item with alternative versions
   // that support either checkboxes or removal buttons:
   { 
      (contact: Contact) =>
      <RemovableContact
         contact={contact} 
         onRemove= {(contactId:number)=>{/* removal logic */}} />
   }
</ContactList>

I think sth like that could be an api improvement. But definitely out of scope for this pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought it's too early to make an abstraction just for these two places, especially considering that neither 'react-window' nor ContactList are too thick to inline.

Copy link
Member

Choose a reason for hiding this comment

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

well technically there at least 3 cases/variants:

  • normal: new chat dialog
  • checkbox: add members to group
  • remove button: group members

I think the contact list with it's many props is kinda hard to read code, like it could be simpler with such an abstraction.
react-window + infini-loader add quite some code we shouldn't copy paste into every component. anyways this is still general thoughts nothing for this pr.

Copy link
Member

@Simon-Laux Simon-Laux left a comment

Choose a reason for hiding this comment

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

Works nicely

@Simon-Laux Simon-Laux merged commit 5dad347 into main May 23, 2024
5 of 7 checks passed
@Simon-Laux Simon-Laux deleted the wofwca/virtualize-contact-list branch May 23, 2024 15:45
WofWca added a commit that referenced this pull request Jun 26, 2024
I.e. instead of `BackendRemote.rpc.getContacts()`
use `BackendRemote.rpc.getContactIds()`
+`BackendRemote.rpc.getContactsByIds()`,
which is much faster if the user has thousands of contacts

Closes #1830, in cooperation with #3842
WofWca added a commit that referenced this pull request Jun 27, 2024
I.e. instead of `BackendRemote.rpc.getContacts()`
use `BackendRemote.rpc.getContactIds()`
+`BackendRemote.rpc.getContactsByIds()`,
which is much faster if the user has thousands of contacts

Closes #1830, in cooperation with #3842
Simon-Laux pushed a commit that referenced this pull request Jul 1, 2024
I.e. instead of `BackendRemote.rpc.getContacts()`
use `BackendRemote.rpc.getContactIds()`
+`BackendRemote.rpc.getContactsByIds()`,
which is much faster if the user has thousands of contacts

Closes #1830, in cooperation with #3842
Simon-Laux pushed a commit that referenced this pull request Jul 1, 2024
I.e. instead of `BackendRemote.rpc.getContacts()`
use `BackendRemote.rpc.getContactIds()`
+`BackendRemote.rpc.getContactsByIds()`,
which is much faster if the user has thousands of contacts

Closes #1830, in cooperation with #3842
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants