diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index ef4c6ccb4e20..13de5a5e8c52 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -1,11 +1,11 @@ import PropTypes from 'prop-types'; -import React, {useMemo} from 'react'; +import React from 'react'; import {FlatList, View} from 'react-native'; import _ from 'underscore'; import CONST from '../../CONST'; import styles from '../../styles/styles'; +import OptionRowLHNData from './OptionRowLHNData'; import variables from '../../styles/variables'; -import OptionRowLHN from './OptionRowLHN'; const propTypes = { /** Extra styles for the section list container */ @@ -15,9 +15,6 @@ const propTypes = { /** Sections for the section list */ data: PropTypes.arrayOf(PropTypes.string).isRequired, - /** Index for option to focus on */ - focusedIndex: PropTypes.number.isRequired, - /** Callback to fire when a row is selected */ onSelectRow: PropTypes.func.isRequired, @@ -32,9 +29,7 @@ const defaultProps = { shouldDisableFocusOptions: false, }; -function LHNOptionsList(props) { - const data = useMemo(() => props.data, [props.data]); - +function LHNOptionsList({contentContainerStyles, data, onSelectRow, optionMode, shouldDisableFocusOptions}) { /** * This function is used to compute the layout of any given item in our list. Since we know that each item will have the exact same height, this is a performance optimization * so that the heights can be determined before the options are rendered. Otherwise, the heights are determined when each option is rendering and it causes a lot of overhead on large @@ -46,7 +41,7 @@ function LHNOptionsList(props) { * @returns {Object} */ const getItemLayout = (itemData, index) => { - const optionHeight = props.optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight; + const optionHeight = optionMode === CONST.OPTION_MODE.COMPACT ? variables.optionRowHeightCompact : variables.optionRowHeight; return { length: optionHeight, offset: index * optionHeight, @@ -59,16 +54,15 @@ function LHNOptionsList(props) { * * @param {Object} params * @param {Object} params.item - * @param {Number} params.index * * @return {Component} */ - const renderItem = ({item, index}) => ( - ( + ); @@ -77,14 +71,13 @@ function LHNOptionsList(props) { item} stickySectionHeadersEnabled={false} renderItem={renderItem} getItemLayout={getItemLayout} - extraData={props.focusedIndex} initialNumToRender={5} maxToRenderPerBatch={5} windowSize={5} diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index affee206701c..b2e1a9aca46b 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -1,8 +1,7 @@ import _ from 'underscore'; -import React, {useEffect, useState} from 'react'; +import React, {useState} from 'react'; import PropTypes from 'prop-types'; import {View, StyleSheet} from 'react-native'; -import lodashGet from 'lodash/get'; import * as optionRowStyles from '../../styles/optionRowStyles'; import styles from '../../styles/styles'; import * as StyleUtils from '../../styles/StyleUtils'; @@ -12,30 +11,22 @@ import MultipleAvatars from '../MultipleAvatars'; import Hoverable from '../Hoverable'; import DisplayNames from '../DisplayNames'; import colors from '../../styles/colors'; -import withLocalize, {withLocalizePropTypes} from '../withLocalize'; -import {withReportCommentDrafts} from '../OnyxProvider'; import Text from '../Text'; import SubscriptAvatar from '../SubscriptAvatar'; import CONST from '../../CONST'; import themeColors from '../../styles/themes/default'; -import SidebarUtils from '../../libs/SidebarUtils'; import OfflineWithFeedback from '../OfflineWithFeedback'; import PressableWithSecondaryInteraction from '../PressableWithSecondaryInteraction'; import * as ReportActionContextMenu from '../../pages/home/report/ContextMenu/ReportActionContextMenu'; import * as ContextMenuActions from '../../pages/home/report/ContextMenu/ContextMenuActions'; import * as OptionsListUtils from '../../libs/OptionsListUtils'; -import compose from '../../libs/compose'; -import ONYXKEYS from '../../ONYXKEYS'; -import * as Report from '../../libs/actions/Report'; +import useLocalize from '../../hooks/useLocalize'; const propTypes = { /** Style for hovered state */ // eslint-disable-next-line react/forbid-prop-types hoverStyle: PropTypes.object, - /** The comment left by the user */ - comment: PropTypes.string, - /** The ID of the report that the option is for */ reportID: PropTypes.string.isRequired, @@ -50,29 +41,25 @@ const propTypes = { style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), - ...withLocalizePropTypes, + /** The item that should be rendered */ + // eslint-disable-next-line react/forbid-prop-types + optionItem: PropTypes.object, }; const defaultProps = { hoverStyle: styles.sidebarLinkHover, viewMode: 'default', onSelectRow: () => {}, - isFocused: false, style: null, - comment: '', + optionItem: null, + isFocused: false, }; function OptionRowLHN(props) { - const optionItem = SidebarUtils.getOptionData(props.reportID); - const [isContextMenuActive, setIsContextMenuActive] = useState(false); + const localize = useLocalize(); - useEffect(() => { - if (!optionItem || optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || props.isFocused) { - return; - } - Report.setReportWithDraft(props.reportID, true); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + const optionItem = props.optionItem; + const [isContextMenuActive, setIsContextMenuActive] = useState(false); if (!optionItem) { return null; @@ -169,7 +156,7 @@ function OptionRowLHN(props) { (hovered || isContextMenuActive) && !props.isFocused ? props.hoverStyle : null, ]} accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} - accessibilityLabel={props.translate('accessibilityHints.navigatesToChat')} + accessibilityLabel={localize.translate('accessibilityHints.navigatesToChat')} > @@ -197,7 +184,7 @@ function OptionRowLHN(props) { {optionItem.alternateText} @@ -246,7 +233,7 @@ function OptionRowLHN(props) { {optionItem.hasDraftComment && ( @@ -254,7 +241,7 @@ function OptionRowLHN(props) { {!shouldShowGreenDotIndicator && optionItem.isPinned && ( @@ -271,13 +258,6 @@ OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; OptionRowLHN.displayName = 'OptionRowLHN'; -export default compose( - withLocalize, - withReportCommentDrafts({ - propName: 'comment', - transformValue: (drafts, props) => { - const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`; - return lodashGet(drafts, draftKey, ''); - }, - }), -)(OptionRowLHN); +export default React.memo(OptionRowLHN); + +export {propTypes, defaultProps}; diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js new file mode 100644 index 000000000000..05e9d2e06cc6 --- /dev/null +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -0,0 +1,142 @@ +import {withOnyx} from 'react-native-onyx'; +import lodashGet from 'lodash/get'; +import _ from 'underscore'; +import PropTypes from 'prop-types'; +import React, {useEffect, useRef, useMemo} from 'react'; +import {deepEqual} from 'fast-equals'; +import {withReportCommentDrafts} from '../OnyxProvider'; +import SidebarUtils from '../../libs/SidebarUtils'; +import compose from '../../libs/compose'; +import ONYXKEYS from '../../ONYXKEYS'; +import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; +import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN'; +import * as Report from '../../libs/actions/Report'; +import * as UserUtils from '../../libs/UserUtils'; +import participantPropTypes from '../participantPropTypes'; +import CONST from '../../CONST'; + +const propTypes = { + /** If true will disable ever setting the OptionRowLHN to focused */ + shouldDisableFocusOptions: PropTypes.bool, + + /** List of users' personal details */ + personalDetails: PropTypes.objectOf(participantPropTypes), + + /** The preferred language for the app */ + preferredLocale: PropTypes.string, + + /** The full data of the report */ + // eslint-disable-next-line react/forbid-prop-types + fullReport: PropTypes.object, + + ...withCurrentReportIDPropTypes, + ...basePropTypes, +}; + +const defaultProps = { + shouldDisableFocusOptions: false, + personalDetails: {}, + fullReport: {}, + preferredLocale: CONST.LOCALES.DEFAULT, + ...withCurrentReportIDDefaultProps, + ...baseDefaultProps, +}; + +/* + * This component gets the data from onyx for the actual + * OptionRowLHN component. + * The OptionRowLHN component is memoized, so it will only + * re-render if the data really changed. + */ +function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, ...propsToForward}) { + const reportID = propsToForward.reportID; + // We only want to pass a boolean to the memoized component, + // instead of a changing number (so we prevent unnecessary re-renders). + const isFocused = !shouldDisableFocusOptions && currentReportID === reportID; + + const optionItemRef = useRef(); + const optionItem = useMemo(() => { + // Note: ideally we'd have this as a dependent selector in onyx! + const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale); + if (deepEqual(item, optionItemRef.current)) { + return optionItemRef.current; + } + optionItemRef.current = item; + return item; + }, [fullReport, preferredLocale, personalDetails]); + + useEffect(() => { + if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) { + return; + } + Report.setReportWithDraft(reportID, true); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return ( + + ); +} + +OptionRowLHNData.propTypes = propTypes; +OptionRowLHNData.defaultProps = defaultProps; +OptionRowLHNData.displayName = 'OptionRowLHNData'; + +/** + * @param {Object} [personalDetails] + * @returns {Object|undefined} + */ +const personalDetailsSelector = (personalDetails) => + _.reduce( + personalDetails, + (finalPersonalDetails, personalData, accountID) => { + // It's OK to do param-reassignment in _.reduce() because we absolutely know the starting state of finalPersonalDetails + // eslint-disable-next-line no-param-reassign + finalPersonalDetails[accountID] = { + accountID: Number(accountID), + login: personalData.login, + displayName: personalData.displayName, + firstName: personalData.firstName, + avatar: UserUtils.getAvatar(personalData.avatar, personalData.accountID), + }; + return finalPersonalDetails; + }, + {}, + ); + +/** + * This component is rendered in a list. + * On scroll we want to avoid that a item re-renders + * just because the list has to re-render when adding more items. + * Thats also why the React.memo is used on the outer component here, as we just + * use it to prevent re-renders from parent re-renders. + */ +export default React.memo( + compose( + withCurrentReportID, + withReportCommentDrafts({ + propName: 'comment', + transformValue: (drafts, props) => { + const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`; + return lodashGet(drafts, draftKey, ''); + }, + }), + withOnyx({ + fullReport: { + key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, + }, + personalDetails: { + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + selector: personalDetailsSelector, + }, + preferredLocale: { + key: ONYXKEYS.NVP_PREFERRED_LOCALE, + }, + }), + )(OptionRowLHNData), +); diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 80df0a70c1e6..580b93d116a7 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -97,13 +97,14 @@ function hasCommentThread(reportAction) { * Returns the parentReportAction if the given report is a thread/task. * * @param {Object} report + * @param {Object} [allReportActionsParam] * @returns {Object} */ -function getParentReportAction(report) { +function getParentReportAction(report, allReportActionsParam = {}) { if (!report || !report.parentReportID || !report.parentReportActionID) { return {}; } - return lodashGet(allReportActions, [report.parentReportID, report.parentReportActionID], {}); + return lodashGet(allReportActionsParam || allReportActions, [report.parentReportID, report.parentReportActionID], {}); } /** diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js index eee3d7558666..8c3fa770941a 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -2047,10 +2047,11 @@ function canSeeDefaultRoom(report, policies, betas) { * @param {Object} report * @param {Array} policies * @param {Array} betas + * @param {Object} allReportActions * @returns {Boolean} */ -function canAccessReport(report, policies, betas) { - if (isThread(report) && ReportActionsUtils.isPendingRemove(ReportActionsUtils.getParentReportAction(report))) { +function canAccessReport(report, policies, betas, allReportActions) { + if (isThread(report) && ReportActionsUtils.isPendingRemove(ReportActionsUtils.getParentReportAction(report, allReportActions))) { return false; } @@ -2075,9 +2076,10 @@ function canAccessReport(report, policies, betas) { * @param {Object} iouReports * @param {String[]} betas * @param {Object} policies + * @param {Object} allReportActions * @returns {boolean} */ -function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies) { +function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies, allReportActions) { const isInDefaultMode = !isInGSDMode; // Exclude reports that have no data because there wouldn't be anything to show in the option item. @@ -2091,7 +2093,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep return false; } - if (!canAccessReport(report, policies, betas)) { + if (!canAccessReport(report, policies, betas, allReportActions)) { return false; } @@ -2124,7 +2126,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep return true; } - // Include policy expense chats if the user isn't in the policy expense chat beta + // Exclude policy expense chats if the user isn't in the policy expense chat beta if (isPolicyExpenseChat(report) && !Permissions.canUsePolicyExpenseChat(betas)) { return false; } diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index 7ce126925b24..0925560d710b 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -14,12 +14,16 @@ import * as LocalePhoneNumber from './LocalePhoneNumber'; import * as UserUtils from './UserUtils'; import * as PersonalDetailsUtils from './PersonalDetailsUtils'; -// Note: It is very important that the keys subscribed to here are the same -// keys that are connected to SidebarLinks withOnyx(). If there was a key missing from SidebarLinks and it's data was updated -// for that key, then there would be no re-render and the options wouldn't reflect the new data because SidebarUtils.getOrderedReportIDs() wouldn't be triggered. -// There are a couple of keys here which are OK to have stale data. Having redundant subscriptions causes more re-renders which should be avoided. -// Session also can remain stale because the only way for the current user to change is to sign out and sign in, which would clear out all the Onyx -// data anyway and cause SidebarLinks to rerender. +// Note: Earlier SidebarUtils.getOrderedReportIDs() used to have no parameters. All the needed data was loaded here directly +// using Onyx.connect. We then had to connect SidebarLinks additionally to all the keys that were used in SidebarUtils.getOrderedReportIDs(). +// That's because we wanted to cause a re-render in SidebarLinks to run SidebarUtils.getOrderedReportIDs() again. +// This caused bugs in the past as we were forgetting to include e.g. very nested data. +// Now we pass all the data from SidebarLinks props to SidebarUtils.getOrderedReportIDs(). +// This makes the code easier to understand and less error prone. + +// However, in getOptionData() we still use Onyx.connect() to get some of the data. +// That's because we can't connect to specific nested data. Once we added dependent +// selectors to Onyx, we can remove this as well. let allReports; Onyx.connect({ @@ -28,24 +32,6 @@ Onyx.connect({ callback: (val) => (allReports = val), }); -let personalDetails; -Onyx.connect({ - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - callback: (val) => (personalDetails = val), -}); - -let priorityMode; -Onyx.connect({ - key: ONYXKEYS.NVP_PRIORITY_MODE, - callback: (val) => (priorityMode = val), -}); - -let betas; -Onyx.connect({ - key: ONYXKEYS.BETAS, - callback: (val) => (betas = val), -}); - const visibleReportActionItems = {}; const lastReportActions = {}; const reportActions = {}; @@ -72,13 +58,9 @@ Onyx.connect({ }, }); -let policies; -Onyx.connect({ - key: ONYXKEYS.COLLECTION.POLICY, - waitForCollectionCallback: true, - callback: (val) => (policies = val), -}); - +// Session can remain stale because the only way for the current user to change is to +// sign out and sign in, which would clear out all the Onyx +// data anyway and cause SidebarLinks to rerender. let currentUserAccountID; Onyx.connect({ key: ONYXKEYS.SESSION, @@ -91,12 +73,6 @@ Onyx.connect({ }, }); -let preferredLocale; -Onyx.connect({ - key: ONYXKEYS.NVP_PREFERRED_LOCALE, - callback: (val) => (preferredLocale = val || CONST.LOCALES.DEFAULT), -}); - let resolveSidebarIsReadyPromise; let sidebarIsReadyPromise = new Promise((resolve) => { @@ -119,17 +95,24 @@ function setIsSidebarLoadedReady() { /** * @param {String} currentReportId + * @param {Object} allReportsDict + * @param {Object} betas + * @param {String[]} policies + * @param {String} priorityMode + * @param {Object} allReportActions * @returns {String[]} An array of reportIDs sorted in the proper order */ -function getOrderedReportIDs(currentReportId) { +function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, priorityMode, allReportActions) { const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD; const isInDefaultMode = !isInGSDMode; // Filter out all the reports that shouldn't be displayed - const reportsToDisplay = _.filter(allReports, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReports, betas, policies)); + const reportsToDisplay = _.filter(allReportsDict, (report) => + ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReportsDict, betas, policies, allReportActions), + ); if (_.isEmpty(reportsToDisplay)) { // Display Concierge chat report when there is no report to be displayed - const conciergeChatReport = _.find(allReports, ReportUtils.isConciergeChatReport); + const conciergeChatReport = _.find(allReportsDict, ReportUtils.isConciergeChatReport); if (conciergeChatReport) { reportsToDisplay.push(conciergeChatReport); } @@ -144,7 +127,7 @@ function getOrderedReportIDs(currentReportId) { report.displayName = ReportUtils.getReportName(report); // eslint-disable-next-line no-param-reassign - report.iouReportAmount = ReportUtils.getMoneyRequestTotal(report, allReports); + report.iouReportAmount = ReportUtils.getMoneyRequestTotal(report, allReportsDict); }); // The LHN is split into five distinct groups, and each group is sorted a little differently. The groups will ALWAYS be in this order: @@ -169,7 +152,7 @@ function getOrderedReportIDs(currentReportId) { return; } - if (report.hasOutstandingIOU && !ReportUtils.isIOUOwnedByCurrentUser(report, allReports)) { + if (report.hasOutstandingIOU && !ReportUtils.isIOUOwnedByCurrentUser(report, allReportsDict)) { outstandingIOUReports.push(report); return; } @@ -214,13 +197,12 @@ function getOrderedReportIDs(currentReportId) { /** * Gets all the data necessary for rendering an OptionRowLHN component * - * @param {String} reportID + * @param {Object} report + * @param {Object} personalDetails + * @param {String} preferredLocale * @returns {Object} */ -function getOptionData(reportID) { - const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${reportID}`; - const report = allReports[reportKey]; - +function getOptionData(report, personalDetails, preferredLocale) { // When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for // this method to be called after the Onyx data has been cleared out. In that case, it's fine to do // a null check here and return early. diff --git a/src/libs/onyxSubscribe.js b/src/libs/onyxSubscribe.js new file mode 100644 index 000000000000..600d010ed27f --- /dev/null +++ b/src/libs/onyxSubscribe.js @@ -0,0 +1,12 @@ +import Onyx from 'react-native-onyx'; + +/** + * Connect to onyx data. Same params as Onyx.connect(), but returns a function to unsubscribe. + * + * @param {Object} mapping Same as for Onyx.connect() + * @return {function(): void} Unsubscribe callback + */ +export default (mapping) => { + const connectionId = Onyx.connect(mapping); + return () => Onyx.disconnect(connectionId); +}; diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 12b1d1edb0ee..15bde9120b46 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -4,7 +4,6 @@ import React from 'react'; import {View} from 'react-native'; import _ from 'underscore'; import PropTypes from 'prop-types'; -import {withOnyx} from 'react-native-onyx'; import styles from '../../../styles/styles'; import * as StyleUtils from '../../../styles/StyleUtils'; import ONYXKEYS from '../../../ONYXKEYS'; @@ -17,18 +16,13 @@ import * as Expensicons from '../../../components/Icon/Expensicons'; import AvatarWithIndicator from '../../../components/AvatarWithIndicator'; import Tooltip from '../../../components/Tooltip'; import CONST from '../../../CONST'; -import participantPropTypes from '../../../components/participantPropTypes'; import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize'; import * as App from '../../../libs/actions/App'; import withCurrentUserPersonalDetails from '../../../components/withCurrentUserPersonalDetails'; import withWindowDimensions from '../../../components/withWindowDimensions'; import LHNOptionsList from '../../../components/LHNOptionsList/LHNOptionsList'; import SidebarUtils from '../../../libs/SidebarUtils'; -import reportPropTypes from '../../reportPropTypes'; import OfflineWithFeedback from '../../../components/OfflineWithFeedback'; -import withNavigationFocus from '../../../components/withNavigationFocus'; -import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../../../components/withCurrentReportID'; -import withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation'; import Header from '../../../components/Header'; import defaultTheme from '../../../styles/themes/default'; import OptionsListSkeletonView from '../../../components/OptionsListSkeletonView'; @@ -39,86 +33,42 @@ import * as Session from '../../../libs/actions/Session'; import Button from '../../../components/Button'; import * as UserUtils from '../../../libs/UserUtils'; import KeyboardShortcut from '../../../libs/KeyboardShortcut'; +import onyxSubscribe from '../../../libs/onyxSubscribe'; +import personalDetailsPropType from '../../personalDetailsPropType'; -const propTypes = { +const basePropTypes = { /** Toggles the navigation menu open and closed */ onLinkClick: PropTypes.func.isRequired, /** Safe area insets required for mobile devices margins */ insets: safeAreaInsetPropTypes.isRequired, - /* Onyx Props */ - /** List of reports */ - // eslint-disable-next-line react/no-unused-prop-types - chatReports: PropTypes.objectOf(reportPropTypes), - - /** All report actions for all reports */ - // eslint-disable-next-line react/no-unused-prop-types - reportActions: PropTypes.objectOf( - PropTypes.arrayOf( - PropTypes.shape({ - error: PropTypes.string, - message: PropTypes.arrayOf( - PropTypes.shape({ - moderationDecisions: PropTypes.arrayOf( - PropTypes.shape({ - decision: PropTypes.string, - }), - ), - }), - ), - }), - ), - ), - - /** List of users' personal details */ - personalDetails: PropTypes.objectOf(participantPropTypes), - - /** The personal details of the person who is logged in */ - currentUserPersonalDetails: PropTypes.shape({ - /** Display name of the current user */ - displayName: PropTypes.string, - - /** Avatar URL or SVG of the current user */ - avatar: PropTypes.oneOfType([PropTypes.string, PropTypes.func]), + /** Whether we are viewing below the responsive breakpoint */ + isSmallScreenWidth: PropTypes.bool.isRequired, +}; - /** Login email of the current user */ - login: PropTypes.string, +const propTypes = { + ...basePropTypes, - /** AccountID of the current user */ - accountID: PropTypes.number, - }), + optionListItems: PropTypes.arrayOf(PropTypes.string).isRequired, - /** Whether we are viewing below the responsive breakpoint */ - isSmallScreenWidth: PropTypes.bool.isRequired, + isLoading: PropTypes.bool.isRequired, - /** The chat priority mode */ - priorityMode: PropTypes.string, + currentUserPersonalDetails: personalDetailsPropType, - /** Details about any modals being used */ - modal: PropTypes.shape({ - /** Indicates when an Alert modal is about to be visible */ - willAlertModalBecomeVisible: PropTypes.bool, - }), + priorityMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), - ...withCurrentReportIDPropTypes, ...withLocalizePropTypes, - ...withNavigationPropTypes, }; const defaultProps = { - chatReports: {}, - reportActions: {}, - personalDetails: {}, currentUserPersonalDetails: { avatar: '', }, priorityMode: CONST.PRIORITY_MODE.DEFAULT, - modal: {}, - ...withCurrentReportIDDefaultProps, }; -class SidebarLinks extends React.Component { +class SidebarLinks extends React.PureComponent { constructor(props) { super(props); @@ -136,11 +86,19 @@ class SidebarLinks extends React.Component { SidebarUtils.setIsSidebarLoadedReady(); this.isSidebarLoaded = true; + let modal = {}; + this.unsubscribeOnyxModal = onyxSubscribe({ + key: ONYXKEYS.MODAL, + callback: (modalArg) => { + modal = modalArg; + }, + }); + const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE; this.unsubscribeEscapeKey = KeyboardShortcut.subscribe( shortcutConfig.shortcutKey, () => { - if (this.props.modal.willAlertModalBecomeVisible) { + if (modal.willAlertModalBecomeVisible) { return; } @@ -158,6 +116,9 @@ class SidebarLinks extends React.Component { if (this.unsubscribeEscapeKey) { this.unsubscribeEscapeKey(); } + if (this.unsubscribeOnyxModal) { + this.unsubscribeOnyxModal(); + } } showSearchPage() { @@ -197,16 +158,8 @@ class SidebarLinks extends React.Component { } render() { - const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports); - const optionListItems = SidebarUtils.getOrderedReportIDs(this.props.currentReportID); - const skeletonPlaceholder = ; - return ( - + - {isLoading ? ( - skeletonPlaceholder + {this.props.isLoading ? ( + ) : ( option.toString() === this.props.currentReportID)} + data={this.props.optionListItems} onSelectRow={this.showReportPage} shouldDisableFocusOptions={this.props.isSmallScreenWidth} optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT} @@ -276,123 +228,5 @@ class SidebarLinks extends React.Component { SidebarLinks.propTypes = propTypes; SidebarLinks.defaultProps = defaultProps; -/** - * This function (and the few below it), narrow down the data from Onyx to just the properties that we want to trigger a re-render of the component. This helps minimize re-rendering - * and makes the entire component more performant because it's not re-rendering when a bunch of properties change which aren't ever used in the UI. - * @param {Object} [report] - * @returns {Object|undefined} - */ -const chatReportSelector = (report) => - report && { - reportID: report.reportID, - participantAccountIDs: report.participantAccountIDs, - hasDraft: report.hasDraft, - isPinned: report.isPinned, - errorFields: { - addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom, - }, - lastReadTime: report.lastReadTime, - lastMentionedTime: report.lastMentionedTime, - lastMessageText: report.lastMessageText, - lastVisibleActionCreated: report.lastVisibleActionCreated, - iouReportID: report.iouReportID, - hasOutstandingIOU: report.hasOutstandingIOU, - statusNum: report.statusNum, - stateNum: report.stateNum, - chatType: report.chatType, - policyID: report.policyID, - reportName: report.reportName, - managerEmail: report.managerEmail, - }; - -/** - * @param {Object} [personalDetails] - * @returns {Object|undefined} - */ -const personalDetailsSelector = (personalDetails) => - _.reduce( - personalDetails, - (finalPersonalDetails, personalData, accountID) => { - // It's OK to do param-reassignment in _.reduce() because we absolutely know the starting state of finalPersonalDetails - // eslint-disable-next-line no-param-reassign - finalPersonalDetails[accountID] = { - accountID: Number(accountID), - login: personalData.login, - displayName: personalData.displayName, - firstName: personalData.firstName, - avatar: UserUtils.getAvatar(personalData.avatar, personalData.accountID), - }; - return finalPersonalDetails; - }, - {}, - ); - -/** - * @param {Object} [reportActions] - * @returns {Object|undefined} - */ -const reportActionsSelector = (reportActions) => - reportActions && - _.map(reportActions, (reportAction) => ({ - errors: reportAction.errors, - message: [ - { - moderationDecisions: [{decision: lodashGet(reportAction, 'message[0].moderationDecisions[0].decision')}], - }, - ], - })); - -/** - * @param {Object} [policy] - * @returns {Object|undefined} - */ -const policySelector = (policy) => - policy && { - type: policy.type, - name: policy.name, - avatar: policy.avatar, - }; - -export default compose( - withLocalize, - withCurrentUserPersonalDetails, - withNavigationFocus, - withWindowDimensions, - withCurrentReportID, - withNavigation, - withOnyx({ - // Note: It is very important that the keys subscribed to here are the same - // keys that are subscribed to at the top of SidebarUtils.js. If there was a key missing from here and data was updated - // for that key, then there would be no re-render and the options wouldn't reflect the new data because SidebarUtils.getOrderedReportIDs() wouldn't be triggered. - // This could be changed if each OptionRowLHN used withOnyx() to connect to the Onyx keys, but if you had 10,000 reports - // with 10,000 withOnyx() connections, it would have unknown performance implications. - chatReports: { - key: ONYXKEYS.COLLECTION.REPORT, - selector: chatReportSelector, - }, - personalDetails: { - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - selector: personalDetailsSelector, - }, - priorityMode: { - key: ONYXKEYS.NVP_PRIORITY_MODE, - }, - betas: { - key: ONYXKEYS.BETAS, - }, - reportActions: { - key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, - selector: reportActionsSelector, - }, - policies: { - key: ONYXKEYS.COLLECTION.POLICY, - selector: policySelector, - }, - preferredLocale: { - key: ONYXKEYS.NVP_PREFERRED_LOCALE, - }, - modal: { - key: ONYXKEYS.MODAL, - }, - }), -)(SidebarLinks); +export default compose(withLocalize, withCurrentUserPersonalDetails, withWindowDimensions)(SidebarLinks); +export {basePropTypes}; diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js new file mode 100644 index 000000000000..ed406d967ad1 --- /dev/null +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -0,0 +1,200 @@ +import React, {useMemo, useRef} from 'react'; +import _ from 'underscore'; +import {deepEqual} from 'fast-equals'; +import {withOnyx} from 'react-native-onyx'; +import PropTypes from 'prop-types'; +import lodashGet from 'lodash/get'; +import {View} from 'react-native'; +import SidebarUtils from '../../../libs/SidebarUtils'; +import SidebarLinks, {basePropTypes} from './SidebarLinks'; +import withCurrentReportID from '../../../components/withCurrentReportID'; +import compose from '../../../libs/compose'; +import ONYXKEYS from '../../../ONYXKEYS'; +import reportPropTypes from '../../reportPropTypes'; +import CONST from '../../../CONST'; +import useLocalize from '../../../hooks/useLocalize'; +import styles from '../../../styles/styles'; +import withNavigationFocus from '../../../components/withNavigationFocus'; + +const propTypes = { + ...basePropTypes, + + /* Onyx Props */ + /** List of reports */ + chatReports: PropTypes.objectOf(reportPropTypes), + + /** All report actions for all reports */ + allReportActions: PropTypes.objectOf( + PropTypes.arrayOf( + PropTypes.shape({ + error: PropTypes.string, + message: PropTypes.arrayOf( + PropTypes.shape({ + moderationDecisions: PropTypes.arrayOf( + PropTypes.shape({ + decision: PropTypes.string, + }), + ), + }), + ), + }), + ), + ), + + /** Whether the personal details are loading. When false it means they are ready to be used. */ + isPersonalDetailsLoading: PropTypes.bool, + + /** The chat priority mode */ + priorityMode: PropTypes.string, + + /** Beta features list */ + betas: PropTypes.arrayOf(PropTypes.string), + + /** The policies which the user has access to */ + // eslint-disable-next-line react/forbid-prop-types + policies: PropTypes.object, +}; + +const defaultProps = { + chatReports: {}, + allReportActions: {}, + priorityMode: CONST.PRIORITY_MODE.DEFAULT, + isPersonalDetailsLoading: true, + betas: [], + policies: [], +}; + +function SidebarLinksData({isFocused, allReportActions, betas, chatReports, currentReportID, insets, isPersonalDetailsLoading, isSmallScreenWidth, onLinkClick, policies, priorityMode}) { + const localize = useLocalize(); + + const reportIDsRef = useRef([]); + const optionListItems = useMemo(() => { + const reportIDs = SidebarUtils.getOrderedReportIDs(currentReportID, chatReports, betas, policies, priorityMode, allReportActions); + if (deepEqual(reportIDsRef.current, reportIDs)) { + return reportIDsRef.current; + } + reportIDsRef.current = reportIDs; + return reportIDs; + }, [allReportActions, betas, chatReports, currentReportID, policies, priorityMode]); + + const isLoading = _.isEmpty(chatReports) || isPersonalDetailsLoading; + + return ( + + + + ); +} + +SidebarLinksData.propTypes = propTypes; +SidebarLinksData.defaultProps = defaultProps; +SidebarLinksData.displayName = 'SidebarLinksData'; + +/** + * This function (and the few below it), narrow down the data from Onyx to just the properties that we want to trigger a re-render of the component. This helps minimize re-rendering + * and makes the entire component more performant because it's not re-rendering when a bunch of properties change which aren't ever used in the UI. + * @param {Object} [report] + * @returns {Object|undefined} + */ +const chatReportSelector = (report) => + report && { + reportID: report.reportID, + participants: report.participants, + participantAccountIDs: report.participantAccountIDs, + hasDraft: report.hasDraft, + isPinned: report.isPinned, + errorFields: { + addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom, + }, + lastMessageText: report.lastMessageText, + lastVisibleActionCreated: report.lastVisibleActionCreated, + iouReportID: report.iouReportID, + total: report.total, + hasOutstandingIOU: report.hasOutstandingIOU, + statusNum: report.statusNum, + stateNum: report.stateNum, + chatType: report.chatType, + type: report.type, + policyID: report.policyID, + visibility: report.visibility, + lastReadTime: report.lastReadTime, + // Needed for name sorting: + reportName: report.reportName, + policyName: report.policyName, + oldPolicyName: report.oldPolicyName, + // Other less obvious properites considered for sorting: + ownerAccountID: report.ownerAccountID, + currency: report.currency, + managerID: report.managerID, + // Other important less obivous properties for filtering: + parentReportActionID: report.parentReportActionID, + parentReportID: report.parentReportID, + }; + +/** + * @param {Object} [reportActions] + * @returns {Object|undefined} + */ +const reportActionsSelector = (reportActions) => + reportActions && + _.map(reportActions, (reportAction) => ({ + errors: reportAction.errors, + message: [ + { + moderationDecisions: [{decision: lodashGet(reportAction, 'message[0].moderationDecisions[0].decision')}], + }, + ], + })); + +/** + * @param {Object} [policy] + * @returns {Object|undefined} + */ +const policySelector = (policy) => + policy && { + type: policy.type, + name: policy.name, + avatar: policy.avatar, + }; + +export default compose( + withCurrentReportID, + withNavigationFocus, + withOnyx({ + chatReports: { + key: ONYXKEYS.COLLECTION.REPORT, + selector: chatReportSelector, + }, + isPersonalDetailsLoading: { + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + selector: _.isEmpty, + }, + priorityMode: { + key: ONYXKEYS.NVP_PRIORITY_MODE, + }, + betas: { + key: ONYXKEYS.BETAS, + }, + allReportActions: { + key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, + selector: reportActionsSelector, + }, + policies: { + key: ONYXKEYS.COLLECTION.POLICY, + selector: policySelector, + }, + }), +)(SidebarLinksData); diff --git a/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js b/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js index 014170457b3a..698a72a205cc 100644 --- a/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js +++ b/src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js @@ -1,7 +1,7 @@ import React, {useEffect} from 'react'; import {View} from 'react-native'; import styles from '../../../../styles/styles'; -import SidebarLinks from '../SidebarLinks'; +import SidebarLinksData from '../SidebarLinksData'; import ScreenWrapper from '../../../../components/ScreenWrapper'; import Navigation from '../../../../libs/Navigation/Navigation'; import ROUTES from '../../../../ROUTES'; @@ -47,7 +47,7 @@ function BaseSidebarScreen(props) { {({insets}) => ( <> - { }), ); - // Initialize the network key for OfflineWithFeedback - beforeEach(() => Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false})); + beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve + wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback + Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + }); // Cleanup (ie. unmount) all rendered components and clear out Onyx after each test so that each test starts with a clean slate afterEach(() => { @@ -253,6 +258,7 @@ describe('Sidebar', () => { ); }); + // NOTE: This is also for #focus mode, should we move this test block? describe('all combinations of isArchived, isUserCreatedPolicyRoom, hasAddWorkspaceError, isUnread, isPinned, hasDraft', () => { // Given a report that is the active report and doesn't change const report1 = LHNTestUtils.getFakeReport([3, 4]); @@ -299,9 +305,9 @@ describe('Sidebar', () => { // To test a failing set of conditions, comment out the for loop above and then use a hardcoded array // for the specific case that's failing. You can then debug the code to see why the test is not passing. - // const boolArr = [false, false, true, false, false, false]; + // const boolArr = [false, false, false, false, false]; - it(`the booleans ${JSON.stringify(boolArr)}`, () => { + it(`the booleans ${boolArr}`, () => { const report2 = { ...LHNTestUtils.getAdvancedFakeReport(...boolArr), policyID: policy.policyID, @@ -321,7 +327,6 @@ describe('Sidebar', () => { [`${ONYXKEYS.COLLECTION.POLICY}${policy.policyID}`]: policy, }), ) - // Then depending on the outcome, either one or two reports are visible .then(() => { if (booleansWhichRemovesActiveReport.indexOf(JSON.stringify(boolArr)) > -1) { diff --git a/tests/unit/SidebarOrderTest.js b/tests/unit/SidebarOrderTest.js index 16cbd1c40698..7c0e12e6b1bc 100644 --- a/tests/unit/SidebarOrderTest.js +++ b/tests/unit/SidebarOrderTest.js @@ -2,6 +2,7 @@ import Onyx from 'react-native-onyx'; import {cleanup, screen} from '@testing-library/react-native'; import lodashGet from 'lodash/get'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import wrapOnyxWithWaitForPromisesToResolve from '../utils/wrapOnyxWithWaitForPromisesToResolve'; import * as LHNTestUtils from '../utils/LHNTestUtils'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; @@ -31,8 +32,12 @@ describe('Sidebar', () => { }), ); - // Initialize the network key for OfflineWithFeedback - beforeEach(() => Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false})); + beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve + wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback + return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + }); // Clear out Onyx after each test so that each test starts with a clean slate afterEach(() => { diff --git a/tests/unit/SidebarTest.js b/tests/unit/SidebarTest.js index 9b5b021341ac..02585d273fb7 100644 --- a/tests/unit/SidebarTest.js +++ b/tests/unit/SidebarTest.js @@ -2,6 +2,7 @@ import Onyx from 'react-native-onyx'; import {cleanup, screen} from '@testing-library/react-native'; import lodashGet from 'lodash/get'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import wrapOnyxWithWaitForPromisesToResolve from '../utils/wrapOnyxWithWaitForPromisesToResolve'; import * as LHNTestUtils from '../utils/LHNTestUtils'; import CONST from '../../src/CONST'; import * as Localize from '../../src/libs/Localize'; @@ -30,8 +31,12 @@ describe('Sidebar', () => { }), ); - // Initialize the network key for OfflineWithFeedback - beforeEach(() => Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false})); + beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve + wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback + return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + }); // Clear out Onyx after each test so that each test starts with a clean slate afterEach(() => { diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index 5af4f4da7918..43090cf024e2 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -4,8 +4,8 @@ import {render} from '@testing-library/react-native'; import ComposeProviders from '../../src/components/ComposeProviders'; import OnyxProvider from '../../src/components/OnyxProvider'; import {LocaleContextProvider} from '../../src/components/withLocalize'; +import SidebarLinksData from '../../src/pages/home/sidebar/SidebarLinksData'; import {EnvironmentProvider} from '../../src/components/withEnvironment'; -import SidebarLinks from '../../src/pages/home/sidebar/SidebarLinks'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; @@ -193,7 +193,7 @@ function getDefaultRenderedSidebarLinks(currentReportID = '') { function MockedSidebarLinks({currentReportID}) { return ( - {}} insets={{ top: 0, diff --git a/tests/utils/wrapOnyxWithWaitForPromisesToResolve.js b/tests/utils/wrapOnyxWithWaitForPromisesToResolve.js new file mode 100644 index 000000000000..c560c50538bd --- /dev/null +++ b/tests/utils/wrapOnyxWithWaitForPromisesToResolve.js @@ -0,0 +1,20 @@ +import waitForPromisesToResolve from './waitForPromisesToResolve'; + +/** + * When we change data in onyx, the listeners (components) will be notified + * on the "next tick" (which is implemented by resolving a promise). + * That means, that we have to wait for the next tick, until the components + * are rendered with the onyx data. + * This is a convinience function, which wraps the onyxInstance's + * functions, to for the promises to resolve. + * + * @param {Object} onyxInstance + */ +export default function wrapOnyxWithWaitForPromisesToResolve(onyxInstance) { + const multiSetImpl = onyxInstance.multiSet; + // eslint-disable-next-line no-param-reassign + onyxInstance.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + const mergeImpl = onyxInstance.merge; + // eslint-disable-next-line no-param-reassign + onyxInstance.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); +}