This shows sidebars on top level if their parent threads have a differing notif setting (home/mute).
In the next diff, the sidebar items listed under a parent that exist as top level items in a different tab will be removed
Details
Details
In a future diff, allowed the notif setting to be changed for sidebars. Tested the following
- Confirmed that moving the child thread had it appear in a separate tab as a top level item
- Confirmed that having a sidebar thread and its parent in the same tab did not result in a top level sidebar thread appearing
- Confirmed moving the parent into the same tab caused the child to no longer appear as top level and into a different tab resulted in a top level item
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1430 ↗ | (On Diff #45211) | as** |
| lib/selectors/chat-selectors.js | ||
|---|---|---|
| 272 ↗ | (On Diff #45211) | Where did you get this type from? ?(ThreadInfo | RawThreadInfo) |
| 273 ↗ | (On Diff #45211) | Why is optional chaining necessary here? |
| 278 ↗ | (On Diff #45211) | In what scenario will isThreadInHome === undefined? |
| 282 ↗ | (On Diff #45211) | It seems like we're making the filtering here stricter, even though our ultimate goal is to make it looser. We're making it stricter here to compensate for the changes in getThreadListSearchResults, which makes the filtering a bit too loose (all sidebars are shown). But getChatThreadItems is called from a bunch more places, and I suspect that they don't all go through getThreadListSearchResults: # git grep chatListData
lib/selectors/chat-selectors.js:const chatListData: (state: BaseAppState<>) => $ReadOnlyArray<ChatThreadItem> =
lib/selectors/chat-selectors.js: chatListData,
lib/shared/thread-utils.js: chatListData: $ReadOnlyArray<ChatThreadItem>,
lib/shared/thread-utils.js: return chatListData.filter(
lib/shared/thread-utils.js: for (const item of chatListData) {
web/chat/thread-list-provider.js: const chatListData = useFlattenedChatListData();
web/chat/thread-list-provider.js: const chatListDataWithoutFilter = getThreadListSearchResults(
web/chat/thread-list-provider.js: chatListData,
web/chat/thread-list-provider.js: let threadListWithTopLevelItem = chatListDataWithoutFilter;
web/chat/thread-list-provider.js: chatListDataWithoutFilter,
# git grep useFilteredChatListData
lib/hooks/child-threads.js: useFilteredChatListData,
lib/hooks/child-threads.js: const allSubchannelsList = useFilteredChatListData(filterSubchannels);
lib/hooks/search-threads.js: useFilteredChatListData,
lib/hooks/search-threads.js: const childThreadInfos = useFilteredChatListData(filterFunc);
lib/selectors/chat-selectors.js: return useFilteredChatListData(threadInChatList);
lib/selectors/chat-selectors.js:function useFilteredChatListData(
lib/selectors/chat-selectors.js: useFilteredChatListData,It seems like your changes are going to make all of these have stricter filtering. What have you do to analyze and mitigate that risk? |
| lib/selectors/chat-selectors.js | ||
|---|---|---|
| 272 ↗ | (On Diff #45211) | This was discussed in-person. I made a mistake and this should have just been ThreadInfo |
| 273 ↗ | (On Diff #45211) | It was not. Going with suggested approach |
| 274 ↗ | (On Diff #45211) | We settled on keeping threadIsHome as threadInfo.currentUser.subscription.home is a boolean with muted being false. Therefore, checking tab by threadIsHome should be fine for now |
| 278 ↗ | (On Diff #45211) | Leftover from a prior change. Changing in next rebase |
| 282 ↗ | (On Diff #45211) | We settled on moving this code from getChatThreadItems (which has several different call sites unrelated to the current work) to the more specific getThreadListSearchResults which will return a helper function when empty search text. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1453 ↗ | (On Diff #45241) | This ensures we also keep the 'seeMore' and 'spacer' sidebarItem types |
Comment Actions
Great work!
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1425 ↗ | (On Diff #45241) | Nit – since we're returning a new array here, and we don't care what the caller does with it, we could allow it to be mutable |
| 1435 ↗ | (On Diff #45241) | Nit |
| 1437 ↗ | (On Diff #45241) | Nit: we only need this inside the conditional below |
| 1439 ↗ | (On Diff #45241) | Nit: invert condition and early exit |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1437–1446 ↗ | (On Diff #45269) | Just a thought |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1437–1446 ↗ | (On Diff #45269) | Thanks! I should be thinking of as many early exits as possible. This code is much more readable. Appending to next rebase before landing |