feat(Wpb-18861): Selectable tabs, conversation filters #19769
Conversation
…references selector, removed old translation keys
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #19769 +/- ##
==========================================
- Coverage 43.45% 43.44% -0.01%
==========================================
Files 1294 1294
Lines 32542 32618 +76
Branches 7229 7244 +15
==========================================
+ Hits 14140 14172 +32
- Misses 16689 16728 +39
- Partials 1713 1718 +5 🚀 New features to boost your workflow:
|
|
🔗 Download Full Report Artifact 🧪 Playwright Test Summary
Failed Tests:❌ Account Management (tags: TC-8639, crit-flow-web)Location: specs/CriticalFlow/accountManagement-TC-8639.spec.ts:37 Errors: ❌ Team owner adds whole team to an all team chat (tags: TC-8631, crit-flow-web)Location: specs/CriticalFlow/addMembersToChat-TC-8631.spec.ts:43 Errors: ❌ Setting up new device with a backup (tags: TC-8634, crit-flow-web)Location: specs/CriticalFlow/backupRestoration-TC-8634.spec.ts:35 Errors: ❌ Calls in channels with device switch and screenshare (tags: TC-8754, crit-flow-web)Location: specs/CriticalFlow/channelsCall-TC-8755.spec.ts:39 Errors: ❌ Channels Management (tags: TC-8752, crit-flow-web)Location: specs/CriticalFlow/channelsManagement-TC-8752.spec.ts:37 Errors: ❌ Conversation Management (tags: TC-8636, crit-flow-web)Location: specs/CriticalFlow/conversationManagement-TC-8636.spec.ts:34 Errors: ❌ Planning group call with sending various messages during call (tags: TC-8632, crit-flow-web)Location: specs/CriticalFlow/groupCalls-TC-8632.spec.ts:37 Errors: ❌ Group Video call (tags: TC-8637, crit-flow-web)Location: specs/CriticalFlow/groupVideoCall-TC-8637.spec.ts:39 Errors: ❌ New person joins team and setups up device (tags: TC-8635, crit-flow-web)Location: specs/CriticalFlow/joinTeam-TC-8635.spec.ts:38 Errors: ❌ Messages in 1:1 (tags: TC-8750, crit-flow-web)Location: specs/CriticalFlow/messagesIn1On1-TC-8750.spec.ts:47 Errors: ❌ Messages in Channels (tags: TC-8753, crit-flow-web)Location: specs/CriticalFlow/messagesInChannels-TC-8753.spec.ts:44 Errors: ❌ Messages in Groups (tags: TC-8751, crit-flow-web)Location: specs/CriticalFlow/messagesInGroups-TC-8751.spec.ts:42 Errors: ❌ 1:1 Video call with device switch and screenshare (tags: TC-8754, crit-flow-web)Location: specs/CriticalFlow/oneOnOneCall-TC-8754.spec.ts:34 Errors: ❌ Personal Account Lifecycle (tags: TC-8638, crit-flow-web)Location: specs/CriticalFlow/personalAccountLifecycle-TC-8638.spec.ts:31 Errors: |
Design update
|
|
|
|
||
| const channelConversationsLength = channelConversations.filter(filterUnreadAndArchivedConversations).length; | ||
| const groupConversationsLength = groupConversations.filter(filterUnreadAndArchivedConversations).length; | ||
| const channelConversationsLength = useMemo( |
There was a problem hiding this comment.
useMemo only helps when the calculation is expensive enough to worry about and its dependencies do not change on every render.
If those arrays are new on every render, then useMemo recomputes every time anyway. In that case, useMemo just adds React bookkeeping overhead with zero gain.
Even if the arrays are referentially stable, these are still cheap O(n) filters which is rarely huge on the UI level. So from a performance POV, most of these are premature micro-optimizations.
I would remove these, but maybe I'm missing some context - others can provide their opinion as well
| type: SidebarTabs.UNREAD, | ||
| title: t('conversationLabelUnread'), | ||
| dataUieName: 'go-unread-view', | ||
| Icon: <Icon.MarkAsUnreadIcon />, |
There was a problem hiding this comment.
@emil-wire MarkAsUnreadIcon is missing on Icon component and locally this breaks the app - popup shows and the only thing to do is reload, which again shows same error. Please fix
There was a problem hiding this comment.
Pull request overview
This PR introduces user-customizable conversation filters and tabs for the left sidebar, allowing users to toggle visibility of specialized conversation views (Unread, Mentions, Replies, Drafts, Pings) via a new settings dropdown. The feature is gated behind the FEATURE_ENABLE_ADVANCED_FILTERS flag.
Key Changes:
- Converts filter dropdown to tab-based navigation with user-customizable visibility
- Adds new sidebar tabs: UNREAD, MENTIONS, REPLIES, DRAFTS, PINGS
- Implements TabsFilterButton component for managing tab visibility preferences
- Enhances draft detection with real-time updates via amplify events and visibility change listeners
- Refactors conversation filtering logic from a single dropdown to dedicated tab views
Reviewed changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/i18n.d.ts | Reorganizes and adds i18n keys for new tab labels and filter UI |
| src/script/util/useChannelsFeatureFlag.ts | Adds shouldShowChannelTab flag to centralize channel visibility logic |
| src/script/page/LeftSidebar/panels/Conversations/utils/draftUtils.ts | Updates draft detection to only check plainMessage content (excludes editorState) |
| src/script/page/LeftSidebar/panels/Conversations/useSidebarStore.ts | Replaces ConversationFilter enum with visibleTabs array and toggleTabVisibility function; defines ALWAYS_VISIBLE_TABS constant |
| src/script/page/LeftSidebar/panels/Conversations/hooks/useDraftConversations.ts | Enhances draft detection with amplify event subscription and visibility change monitoring |
| src/script/page/LeftSidebar/panels/Conversations/helpers.tsx | Removes applyAdvancedFilter function; adds dedicated tab handlers for UNREAD, MENTIONS, REPLIES, DRAFTS, PINGS with conversationFilters utilities |
| src/script/page/LeftSidebar/panels/Conversations/TabsFilterButton/TabsFilterButton.tsx | New component providing dropdown UI for toggling tab visibility with keyboard support |
| src/script/page/LeftSidebar/panels/Conversations/TabsFilterButton/TabsFilterButton.styles.ts | Styles for filter button and dropdown with accessibility focus states |
| src/script/page/LeftSidebar/panels/Conversations/Helpers.test.tsx | Removes conversationFilter from test cases |
| src/script/page/LeftSidebar/panels/Conversations/Conversations.tsx | Fixes typo (hasNoVisbleConversations → hasNoVisibleConversations); adds missing dependency arrays to useEffect/useCallback hooks |
| src/script/page/LeftSidebar/panels/Conversations/ConversationTabs/ConversationTabs.tsx | Adds new tab definitions with useMemo for badge counts; filters visible tabs using isTabVisible; replaces ConversationFilterButton with TabsFilterButton |
| src/script/page/LeftSidebar/panels/Conversations/ConversationSidebar/ConversationSidebar.tsx | Passes conversations and draftConversations props to ConversationTabs |
| src/script/page/LeftSidebar/panels/Conversations/ConversationFilterButton/* | Removes old ConversationFilterButton component and styles |
| src/script/components/InputBar/common/draftState/draftState.ts | Publishes DRAFT_STATE_CHANGED_EVENT when drafts are saved |
| src/i18n/*.json | Removes old conversationFilter* keys; adds new conversationLabel* and search*Conversations keys across all locales |
| if (shouldShowChannelTab) { | ||
| availableTabs.splice(2, 0, {type: SidebarTabs.CHANNELS, label: t('conversationLabelChannels')}); | ||
| } |
There was a problem hiding this comment.
[Important] The channel tab insertion logic uses a hardcoded index 2 which is fragile. If tabs are reordered or removed from the availableTabs array, the channel tab will be inserted at the wrong position. This inconsistency with the display order in ConversationTabs (where it's inserted at index 7) could lead to confusion.
Consider using a position-based approach similar to the suggestion for ConversationTabs.tsx, or define the tab order in a single location to maintain consistency.
| toggleTabVisibility: (tab: SidebarTabs) => { | ||
| set(state => { | ||
| const isCurrentlyVisible = state.visibleTabs.includes(tab); | ||
| const isActiveTab = state.currentTab === tab; | ||
|
|
||
| if (isCurrentlyVisible && isActiveTab) { | ||
| return { | ||
| currentTab: SidebarTabs.RECENT, | ||
| visibleTabs: state.visibleTabs.filter(visibleTab => visibleTab !== tab), | ||
| }; | ||
| } | ||
|
|
||
| const newVisibleTabs = isCurrentlyVisible | ||
| ? state.visibleTabs.filter(visibleTab => visibleTab !== tab) | ||
| : [...state.visibleTabs, tab]; | ||
|
|
||
| return {visibleTabs: newVisibleTabs}; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
[Blocker] The toggleTabVisibility function doesn't check if a tab is in ALWAYS_VISIBLE_TABS before attempting to hide it. Users can currently hide the RECENT tab (which is defined as always visible), creating an inconsistent state.
Add a guard clause at the beginning:
toggleTabVisibility: (tab: SidebarTabs) => {
if (ALWAYS_VISIBLE_TABS.includes(tab)) {
return;
}
set(state => {
// ... rest of logic
});
},| <div css={dropdownCheckboxItem} role="menuitemcheckbox" aria-checked={visibleTabs.includes(tab.type)}> | ||
| <Checkbox | ||
| wrapperCSS={roundCheckbox} | ||
| checked={visibleTabs.includes(tab.type)} | ||
| onChange={() => toggleTabVisibility(tab.type)} | ||
| > | ||
| <CheckboxLabel css={checkboxLabel}>{tab.label}</CheckboxLabel> | ||
| </Checkbox> | ||
| </div> |
There was a problem hiding this comment.
[Important] The checkbox items in the dropdown menu lack keyboard navigation support. Users cannot navigate between checkboxes using arrow keys, which is expected behavior for role="menu" with role="menuitemcheckbox" items. Additionally, the checkboxes should be clickable via the Space or Enter keys when focused.
Consider implementing keyboard event handlers for Up/Down arrow keys to navigate between items, and ensure Space/Enter keys toggle the selected item.
| // Check if plainMessage has actual content (not just whitespace) | ||
| const plainMessage = draft.plainMessage || ''; | ||
| const hasTextContent = plainMessage.trim().length > 0; | ||
|
|
||
| return hasTextContent; |
There was a problem hiding this comment.
[Important] The draft detection logic only checks plainMessage but ignores editorState. In the previous version (visible in the diff context), drafts with only editorState (and no plainMessage) were correctly detected. The new implementation will incorrectly return false for drafts that have rich text content stored in editorState but no plain text version.
Consider checking both fields:
const hasTextContent = plainMessage.trim().length > 0;
const hasEditorState = Boolean(draft.editorState);
return hasTextContent || hasEditorState;| const channelConversationsLength = useMemo( | ||
| () => channelConversations.filter(filterUnreadAndArchivedConversations).length, | ||
| [channelConversations], | ||
| ); | ||
|
|
||
| const groupConversationsLength = useMemo( | ||
| () => groupConversations.filter(filterUnreadAndArchivedConversations).length, | ||
| [groupConversations], | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The useMemo hooks for channelConversationsLength and groupConversationsLength (lines 130-138) don't include filterUnreadAndArchivedConversations in their dependency arrays. While this function is defined in the component and uses conversationFilters, it's technically stable. However, it references external functions which could theoretically change. Consider either:
- Moving
filterUnreadAndArchivedConversationsoutside the component as a module-level function, or - Wrapping it with
useCallbackand including it in the dependency arrays
This ensures the memoization is correctly invalidated if the filter logic ever changes.
| if (shouldShowChannelTab) { | ||
| conversationTabs.splice(7, 0, { | ||
| type: SidebarTabs.CHANNELS, | ||
| title: t('conversationLabelChannels'), | ||
| dataUieName: 'go-channels-view', | ||
| Icon: <ChannelIcon />, | ||
| unreadConversations: channelConversationsLength, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Important] The channel tab insertion logic uses a hardcoded index 7 which is fragile and error-prone. If the order or number of tabs in the conversationTabs array changes (e.g., if someone removes or reorders tabs), the channel tab will be inserted at the wrong position.
Consider refactoring to insert the channel tab relative to a specific tab (e.g., after GROUPS or DIRECTS) or add it to the array based on a logical position marker. Example:
const groupsIndex = conversationTabs.findIndex(tab => tab.type === SidebarTabs.GROUPS);
if (groupsIndex !== -1) {
conversationTabs.splice(groupsIndex + 1, 0, { ... });
}| const availableTabs = [ | ||
| {type: SidebarTabs.FAVORITES, label: t('conversationLabelFavorites')}, | ||
| {type: SidebarTabs.GROUPS, label: t('conversationLabelGroups')}, | ||
| {type: SidebarTabs.DIRECTS, label: t('conversationLabelDirects')}, | ||
| {type: SidebarTabs.FOLDER, label: t('folderViewTooltip')}, | ||
| {type: SidebarTabs.ARCHIVES, label: t('conversationFooterArchive')}, | ||
| {type: SidebarTabs.UNREAD, label: t('conversationLabelUnread')}, | ||
| {type: SidebarTabs.MENTIONS, label: t('conversationLabelMentions')}, | ||
| {type: SidebarTabs.REPLIES, label: t('conversationLabelReplies')}, | ||
| {type: SidebarTabs.DRAFTS, label: t('conversationLabelDrafts')}, | ||
| {type: SidebarTabs.PINGS, label: t('conversationLabelPings')}, | ||
| ]; |
There was a problem hiding this comment.
[Important] The availableTabs array doesn't exclude tabs from ALWAYS_VISIBLE_TABS (specifically SidebarTabs.RECENT). This means users will see the RECENT tab as an option in the filter dropdown, but toggling it will have no effect due to the isTabVisible function. This creates a confusing UX where the checkbox appears to work but the tab remains visible.
The RECENT tab should be excluded from the availableTabs array entirely since it cannot be hidden.
user selectable conversation filters + tabs
This PR is a rewrite of a previous PR that didn't work out so well. Using the feature flag FEATURE_ENABLE_ADVANCED_FILTERS you can turn this on and test it.
Checklist