Details

Reviewers

inka
• kuba
kamil
ashoat

Commits

rCOMMf61bf372c059: [web] Use react-window

Summary

Part of ENG-3976
This diff uses react-window package to render only visible threads. react-virtualized-auto-sizer is the recommended utility to automatically set height of the VariableSizeList. react-window works by applying a style to each element with an absolute position that it calculated.

There's one issue that I'm aware of: if you scroll down, the search bar will stop rendering and lose focus (but it will keep it's contents). On 1-1 with @ashoat we decided that's good enough for now. I will create an issue explaining a possible solution with docking the search bar before landing this diff.

Adding ashoat as reviewer because of new dependencies.

Test Plan

Check if thread list looks the same, and scrolls correctly. Try searching chat and check if everything works correctly. Check if an empty background tab renders correctly.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Jun 16 2023, 3:24 AM

Herald added a subscriber: tomek. · View Herald TranscriptJun 16 2023, 3:24 AM

michal added a parent revision: D8074: [native] Fetch latest messages in chat list.Jun 16 2023, 3:25 AM

michal edited the summary of this revision. (Show Details)Jun 16 2023, 3:30 AM

michal added inline comments.Jun 16 2023, 3:33 AM

web/chat/chat-thread-list.css
18–20 ↗	(On Diff #27827)	This is removed because it incorrectly shifts the first element (it still has position absolute from the `VariableSizeList`). I added it to the search bar size for a similar effect.
web/chat/chat-thread-list.react.js
20–24 ↗	(On Diff #27827)	I inspected the current web app to get these sizes.
26 ↗	(On Diff #27827)	It's not inlined because it will be used in the next diff.
82–86 ↗	(On Diff #27827)	We need to invalidate the calculated positions if threads change (for example someone started a thread/subthread). This could probably be done smarter and only invalidate after some index, but I don't think there's an easy way to get it.

michal added a child revision: D8235: [web] Fetch latest messages in chat list.Jun 16 2023, 3:39 AM

Harbormaster completed remote builds in B20304: Diff 27827.Jun 16 2023, 3:43 AM

michal requested review of this revision.Jun 16 2023, 3:43 AM

ashoat requested changes to this revision.Jun 16 2023, 4:44 PM

ashoat added inline comments.

web/chat/chat-thread-list.react.js
32–36 ↗	(On Diff #27827)	Don't love seeing conditionals inside JSX It's not clear from here that this is a special-case for the search bar, or why the search bar needs to be special-cased We could make the check more readable if it was eg. `data[0].type === 'search'`. When we need to handle `FlatList`s on `native`, we typically make their data prop a "union of disjoint types"... you could do this by putting `{ itemType: 'search' }` into the data list instead of the raw JSX If you take my suggestion below to refactor `const search` on line 58 into its own component, then you could pass eg. `{ itemType: 'search' }` as the item, and render the component here
58–67 ↗	(On Diff #27827)	It might be good to factor this out into a separate component. It's not great that a `searchText` change forces the whole `threadListContainer` to re-render, when it might only require the `Search` component to re-render. Looking at `ThreadListProvider` more closely, it appears that `searchText` changes are always accompanied by `threadList` changes, so there's no performance issue here. But in case that weren't true (either because the code is smart enough to use the same list if the results don't change, or because there is some delay before updating of `threadList`) then there would be a performance issue. If you factored this out into a separate component, then `searchText` changes could be isolated to this new `ChatThreadListSearch` component.
89 ↗	(On Diff #27827)	Why is this handled in a special case that skips `react-window`? If it's just for perf benefits of skipping `AutoSizer`, can you add a code comment to that effect?
120 ↗	(On Diff #27827)	I can't find this in the reference for `VariableSizeList`. Can you explain it and link to where you learned about it?

This revision now requires changes to proceed.Jun 16 2023, 4:44 PM

michal added inline comments.Jul 13 2023, 2:49 AM

web/chat/chat-thread-list.react.js
120 ↗	(On Diff #27827)	It's in reference for fixed size list. But I've removed it anyway because `1` is the default.

Refactored the code, used a similar approach as it's used on native (union with item type). Extracted search to a separate component.

Harbormaster completed remote builds in B20914: Diff 28666.Jul 13 2023, 3:07 AM

ashoat accepted this revision.Jul 13 2023, 9:54 AM

ashoat added inline comments.

web/chat/chat-thread-list.react.js
30 ↗	(On Diff #28666)	The previous reasoning for not inlining this function was that it would be used in the next diff. Now that the next diff has been abandoned, should we inline it? Alternately, the other logic in `itemSize` could be moved here. But the separation between `itemSize` and `getChatThreadItemSize` doesn't make too much sense to me right now.