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

Lazy-load contacts #3927

Merged
merged 5 commits into from
Jul 1, 2024
Merged

Lazy-load contacts #3927

merged 5 commits into from
Jul 1, 2024

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Jun 10, 2024

It's easier to review the changes commit-by-commit (also I'd prefer "rebase and merge" instead of "squash"). Also, for the last commit, GitHub is not good at showing whitespace-only changes, which it mostly is.
I did a bit of QA (but maybe not enough), and looked at the code. It doesn't look like anything should break.

TODO:

  • There are 2 more places where <InfiniteLoader> needs to be added. For now I just want to show the general approach and see if anyone has objections to it.
  • Now the only problem is Invalid list ref; please refer to InfiniteLoader documentation. in the console. I think we might need to set it on <AutoSizer instead? not sure. Should not be a blocker for reviews. Update: solved by putting <FixedSizeList as a direct child of <InfiniteLoader

@Simon-Laux
Copy link
Member

no objections so far.

@farooqkz farooqkz changed the title WIP: perf: lazy-load contacts Lazy-load contacts Jun 20, 2024
@farooqkz
Copy link
Collaborator

Is this PR ready for review? If yes, why is it a draft?

@farooqkz farooqkz added the performance Related to (improving) performance label Jun 20, 2024
@WofWca
Copy link
Collaborator Author

WofWca commented Jun 20, 2024

No, not ready. Draft because it's not done yet. Need to clean up, and change a few more places.

@farooqkz
Copy link
Collaborator

No, not ready. Draft because it's not done yet. Need to clean up, and change a few more places.

Alright. Looking forward to it!

@WofWca WofWca force-pushed the wofwca/contacts-infinite-loader branch 2 times, most recently from b24f915 to 0b8174f Compare June 26, 2024 20:46
@WofWca WofWca marked this pull request as ready for review June 26, 2024 20:54
@WofWca WofWca force-pushed the wofwca/contacts-infinite-loader branch from aa7efa5 to ceed14c Compare June 27, 2024 08:52
And add docstring mentioning performance
The approach (`useMemo()`) is the same as in `CreateGroup`
a few lines above

Although this doesn't really affect real-life scenarios,
because on initial load the list is empty anyway,
and when recipients are added, the contacts are already loaded
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 Simon-Laux force-pushed the wofwca/contacts-infinite-loader branch from ceed14c to 8a100c0 Compare July 1, 2024 17:17
@Simon-Laux Simon-Laux merged commit ee1c326 into main Jul 1, 2024
7 checks passed
@Simon-Laux Simon-Laux deleted the wofwca/contacts-infinite-loader branch July 1, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Related to (improving) performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants