From 82974c64f5e78d72260f264f31da625cf9cc3766 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 23 Jun 2023 11:52:50 +0200 Subject: [PATCH 01/54] Connect OptionRowLHN manually --- src/components/LHNOptionsList/OptionRowLHN.js | 18 ++++++++++++++++-- src/libs/SidebarUtils.js | 8 +++----- src/pages/home/sidebar/SidebarLinks.js | 2 -- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 4492d891e1e5..f6d92be6abdb 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -2,6 +2,7 @@ import _ from 'underscore'; import React from 'react'; import PropTypes from 'prop-types'; import {View, StyleSheet} from 'react-native'; +import {withOnyx} from 'react-native-onyx'; import * as optionRowStyles from '../../styles/optionRowStyles'; import styles from '../../styles/styles'; import * as StyleUtils from '../../styles/StyleUtils'; @@ -23,6 +24,8 @@ import PressableWithSecondaryInteraction from '../PressableWithSecondaryInteract 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'; const propTypes = { /** Style for hovered state */ @@ -43,6 +46,8 @@ const propTypes = { style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), + optionItem: PropTypes.shape({}), + ...withLocalizePropTypes, }; @@ -52,10 +57,11 @@ const defaultProps = { onSelectRow: () => {}, isFocused: false, style: null, + optionItem: null, }; function OptionRowLHN(props) { - const optionItem = SidebarUtils.getOptionData(props.reportID); + const optionItem = props.optionItem; if (!optionItem) { return null; @@ -254,4 +260,12 @@ OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; OptionRowLHN.displayName = 'OptionRowLHN'; -export default withLocalize(OptionRowLHN); +export default compose( + withLocalize, + withOnyx({ + optionItem: { + key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, + selector: SidebarUtils.getOptionData, + }, + }), +)(OptionRowLHN); diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index ee53cd9df69a..8eafc74a6c85 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -20,6 +20,7 @@ import * as UserUtils from './UserUtils'; // 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. +// TODO: get rid of this shit let allReports; Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT, @@ -213,13 +214,10 @@ function getOrderedReportIDs(currentReportId) { /** * Gets all the data necessary for rendering an OptionRowLHN component * - * @param {String} reportID + * @param {Object} report * @returns {Object} */ -function getOptionData(reportID) { - const reportKey = `${ONYXKEYS.COLLECTION.REPORT}${reportID}`; - const report = allReports[reportKey]; - +function getOptionData(report) { // 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/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 77070f272bb6..0e0078800386 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -274,8 +274,6 @@ const chatReportSelector = (report) => errorFields: { addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom, }, - lastReadTime: report.lastReadTime, - lastMentionedTime: report.lastMentionedTime, lastMessageText: report.lastMessageText, lastVisibleActionCreated: report.lastVisibleActionCreated, iouReportID: report.iouReportID, From eada423e171bc36065b941232b3e30e412855626 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 23 Jun 2023 12:09:51 +0200 Subject: [PATCH 02/54] don't rerender SidebarLinks just because modal opens --- src/libs/onyxSubscribe.js | 12 ++++++++++++ src/pages/home/sidebar/SidebarLinks.js | 14 ++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) create mode 100644 src/libs/onyxSubscribe.js 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 0e0078800386..32008b7571fa 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -40,6 +40,7 @@ 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'; const propTypes = { /** Toggles the navigation menu open and closed */ @@ -121,11 +122,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; } @@ -366,8 +375,5 @@ export default compose( preferredLocale: { key: ONYXKEYS.NVP_PREFERRED_LOCALE, }, - modal: { - key: ONYXKEYS.MODAL, - }, }), )(SidebarLinks); From c31f7de846e11c89d24f4d4e545e0a03a303e26b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 23 Jun 2023 12:32:12 +0200 Subject: [PATCH 03/54] Remove currentReportId from shouldReportBeInOptionList There isn't any case where the current active report wouldn't be in the list of reports --- src/libs/ReportUtils.js | 12 ++---------- src/libs/SidebarUtils.js | 5 ++--- src/pages/home/sidebar/SidebarLinks.js | 5 +---- 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js index 64a07113cb51..479be9398d7b 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -1891,14 +1891,13 @@ function canSeeDefaultRoom(report, policies, betas) { * filter out the majority of reports before filtering out very specific minority of reports. * * @param {Object} report - * @param {String} currentReportId * @param {Boolean} isInGSDMode * @param {Object} iouReports * @param {String[]} betas * @param {Object} policies * @returns {boolean} */ -function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies) { +function shouldReportBeInOptionList(report, isInGSDMode, iouReports, betas, policies) { const isInDefaultMode = !isInGSDMode; // Exclude reports that have no data because there wouldn't be anything to show in the option item. @@ -1916,13 +1915,6 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep return false; } - // Include the currently viewed report. If we excluded the currently viewed report, then there - // would be no way to highlight it in the options list and it would be confusing to users because they lose - // a sense of context. - if (report.reportID === currentReportId) { - return true; - } - // Include reports if they have a draft, are pinned, or have an outstanding IOU // These are always relevant to the user no matter what view mode the user prefers if (report.hasDraft || report.isPinned || hasOutstandingIOU(report, iouReports)) { @@ -1945,7 +1937,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 8eafc74a6c85..a1bfa889c8e8 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -118,15 +118,14 @@ function setIsSidebarLoadedReady() { } /** - * @param {String} currentReportId * @returns {String[]} An array of reportIDs sorted in the proper order */ -function getOrderedReportIDs(currentReportId) { +function getOrderedReportIDs() { 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(allReports, (report) => ReportUtils.shouldReportBeInOptionList(report, isInGSDMode, allReports, betas, policies)); if (_.isEmpty(reportsToDisplay)) { // Display Concierge chat report when there is no report to be displayed const conciergeChatReport = _.find(allReports, ReportUtils.isConciergeChatReport); diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 32008b7571fa..8478122450f7 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -28,7 +28,6 @@ import SidebarUtils from '../../../libs/SidebarUtils'; import reportPropTypes from '../../reportPropTypes'; import OfflineWithFeedback from '../../../components/OfflineWithFeedback'; import withNavigationFocus from '../../../components/withNavigationFocus'; -import withCurrentReportId, {withCurrentReportIdPropTypes} from '../../../components/withCurrentReportId'; import withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation'; import Header from '../../../components/Header'; import defaultTheme from '../../../styles/themes/default'; @@ -89,7 +88,6 @@ const propTypes = { }), ...withLocalizePropTypes, - ...withCurrentReportIdPropTypes, ...withNavigationPropTypes, }; @@ -189,7 +187,7 @@ class SidebarLinks extends React.Component { render() { const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports); - const optionListItems = SidebarUtils.getOrderedReportIDs(this.props.currentReportId); + const optionListItems = SidebarUtils.getOrderedReportIDs(); const skeletonPlaceholder = ; @@ -342,7 +340,6 @@ export default compose( withCurrentUserPersonalDetails, withNavigationFocus, withWindowDimensions, - withCurrentReportId, withNavigation, withOnyx({ // Note: It is very important that the keys subscribed to here are the same From 32c2b70b5171a92a68e692cca632e8c127289d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 23 Jun 2023 14:14:20 +0200 Subject: [PATCH 04/54] add active report functionality back --- src/components/LHNOptionsList/OptionRowLHN.js | 41 +++++++++++++++---- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index f6d92be6abdb..0528823013a6 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -26,6 +26,7 @@ import * as ContextMenuActions from '../../pages/home/report/ContextMenu/Context import * as OptionsListUtils from '../../libs/OptionsListUtils'; import compose from '../../libs/compose'; import ONYXKEYS from '../../ONYXKEYS'; +import withCurrentReportId from '../withCurrentReportId'; const propTypes = { /** Style for hovered state */ @@ -260,12 +261,34 @@ OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; OptionRowLHN.displayName = 'OptionRowLHN'; -export default compose( - withLocalize, - withOnyx({ - optionItem: { - key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, - selector: SidebarUtils.getOptionData, - }, - }), -)(OptionRowLHN); +const ConnectedOptonRowLHN = React.memo( + compose( + withLocalize, + withOnyx({ + optionItem: { + key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, + selector: SidebarUtils.getOptionData, + }, + }), + )(OptionRowLHN), +); + +// We only want to forward a boolean value to the memoized component +// Thats why we have this intermediate component. +function OptionRowIsFocusedSupport(props) { + const isFocused = props.currentReportId === props.reportID; + + return ( + + ); +} +OptionRowIsFocusedSupport.propTypes = propTypes; +OptionRowIsFocusedSupport.defaultProps = defaultProps; +OptionRowIsFocusedSupport.displayName = 'OptionRowIsFocusedSupport'; + +// TODO: Note on mobile we could skip this HOC +export default withCurrentReportId(OptionRowIsFocusedSupport); From 6a38ee8fc114f2d32f5791150f36336ec11ce6ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 23 Jun 2023 15:13:00 +0200 Subject: [PATCH 05/54] optimize optionRowLHN to not depend on current report id --- src/components/LHNOptionsList/OptionRowLHN.js | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 0528823013a6..4c34b518f16b 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -61,7 +61,7 @@ const defaultProps = { optionItem: null, }; -function OptionRowLHN(props) { +function BaseOptionRowLHN(props) { const optionItem = props.optionItem; if (!optionItem) { @@ -257,11 +257,11 @@ function OptionRowLHN(props) { ); } -OptionRowLHN.propTypes = propTypes; -OptionRowLHN.defaultProps = defaultProps; -OptionRowLHN.displayName = 'OptionRowLHN'; +BaseOptionRowLHN.propTypes = propTypes; +BaseOptionRowLHN.defaultProps = defaultProps; +BaseOptionRowLHN.displayName = 'BaseOptionRowLHN'; -const ConnectedOptonRowLHN = React.memo( +const MemoedOptionRowLHN = React.memo( compose( withLocalize, withOnyx({ @@ -270,25 +270,26 @@ const ConnectedOptonRowLHN = React.memo( selector: SidebarUtils.getOptionData, }, }), - )(OptionRowLHN), + )(BaseOptionRowLHN), ); -// We only want to forward a boolean value to the memoized component -// Thats why we have this intermediate component. -function OptionRowIsFocusedSupport(props) { +// We only want to pass a boolean to the memoized +// component, thats why we have this intermediate component. +// (We don't want to fully re-render all items, just because the active report changed). +function OptionRowLHN(props) { const isFocused = props.currentReportId === props.reportID; return ( - ); } -OptionRowIsFocusedSupport.propTypes = propTypes; -OptionRowIsFocusedSupport.defaultProps = defaultProps; -OptionRowIsFocusedSupport.displayName = 'OptionRowIsFocusedSupport'; -// TODO: Note on mobile we could skip this HOC -export default withCurrentReportId(OptionRowIsFocusedSupport); +OptionRowLHN.propTypes = propTypes; +OptionRowLHN.defaultProps = defaultProps; +OptionRowLHN.displayName = 'OptionRowIsFocusedSupport'; + +export default withCurrentReportId(OptionRowLHN); From a6511186d9508eb1d70596e4d62bd9bf261a788b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 26 Jun 2023 10:35:12 +0200 Subject: [PATCH 06/54] fix --- src/libs/SidebarUtils.js | 1 - src/pages/home/sidebar/SidebarLinks.js | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index a1bfa889c8e8..87f4d95eacd0 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -20,7 +20,6 @@ import * as UserUtils from './UserUtils'; // 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. -// TODO: get rid of this shit let allReports; Onyx.connect({ key: ONYXKEYS.COLLECTION.REPORT, diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 8478122450f7..62deccc809b2 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -150,6 +150,9 @@ class SidebarLinks extends React.Component { if (this.unsubscribeEscapeKey) { this.unsubscribeEscapeKey(); } + if (this.unsubscribeOnyxModal) { + this.unsubscribeOnyxModal(); + } } showSearchPage() { From 27f8eb006b8e7d98cc27a1c7fe8b8d96654773f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 6 Jul 2023 14:44:54 +0200 Subject: [PATCH 07/54] fix issues after merge --- src/pages/home/sidebar/SidebarLinks.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 907cb28cea1b..e7e07a046995 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -101,7 +101,6 @@ const propTypes = { willAlertModalBecomeVisible: PropTypes.bool, }), - ...withCurrentReportIDPropTypes, ...withLocalizePropTypes, ...withNavigationPropTypes, }; @@ -115,7 +114,6 @@ const defaultProps = { }, priorityMode: CONST.PRIORITY_MODE.DEFAULT, modal: {}, - ...withCurrentReportIDDefaultProps, }; class SidebarLinks extends React.Component { From f3a39b68ba20e857e754561c7576c9ebd0e880d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 6 Jul 2023 14:53:50 +0200 Subject: [PATCH 08/54] fix import issues --- src/components/LHNOptionsList/OptionRowLHN.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index ac618b0279a1..aff45dd08f24 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -28,7 +28,7 @@ import * as ContextMenuActions from '../../pages/home/report/ContextMenu/Context import * as OptionsListUtils from '../../libs/OptionsListUtils'; import compose from '../../libs/compose'; import ONYXKEYS from '../../ONYXKEYS'; -import withCurrentReportId from '../withCurrentReportId'; +import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; import * as Report from '../../libs/actions/Report'; const propTypes = { @@ -56,6 +56,7 @@ const propTypes = { optionItem: PropTypes.shape({}), ...withLocalizePropTypes, + ...withCurrentReportIDPropTypes, }; const defaultProps = { @@ -66,6 +67,7 @@ const defaultProps = { style: null, optionItem: null, comment: '', + ...withCurrentReportIDDefaultProps, }; function BaseOptionRowLHN(props) { @@ -311,7 +313,7 @@ OptionRowLHN.displayName = 'OptionRowIsFocusedSupport'; export default compose( withLocalize, - withCurrentReportId, + withCurrentReportID, withReportCommentDrafts({ propName: 'comment', transformValue: (drafts, props) => { From 99d2d408502e1d4aae7988525e6ca5ced6d0a110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 6 Jul 2023 15:06:29 +0200 Subject: [PATCH 09/54] change back name --- src/components/LHNOptionsList/OptionRowLHN.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index aff45dd08f24..61a922a281ed 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -309,7 +309,7 @@ function OptionRowLHN(props) { OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; -OptionRowLHN.displayName = 'OptionRowIsFocusedSupport'; +OptionRowLHN.displayName = 'OptionRowLHN'; export default compose( withLocalize, From 4bb3a192a12c93643fd57fd3da69d5c5c08a128b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Thu, 6 Jul 2023 15:31:33 +0200 Subject: [PATCH 10/54] remove currentReportId from params --- tests/utils/LHNTestUtils.js | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index c00c69c3532a..e2c94d1dcb80 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -1,5 +1,4 @@ import React from 'react'; -import PropTypes from 'prop-types'; import {render} from '@testing-library/react-native'; import ComposeProviders from '../../src/components/ComposeProviders'; import OnyxProvider from '../../src/components/OnyxProvider'; @@ -7,6 +6,7 @@ import {LocaleContextProvider} from '../../src/components/withLocalize'; import SidebarLinks from '../../src/pages/home/sidebar/SidebarLinks'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; +import {CurrentReportIDContextProvider} from '../../src/components/withCurrentReportID'; // we have to mock `useIsFocused` because it's used in the SidebarLinks component const mockedNavigate = jest.fn(); @@ -189,9 +189,9 @@ function getDefaultRenderedSidebarLinks(currentReportID = '') { * @param {String} [currentReportID] * @returns {JSX.Element} */ -function MockedSidebarLinks({currentReportID}) { +function MockedSidebarLinks() { return ( - + {}} insets={{ @@ -201,18 +201,9 @@ function MockedSidebarLinks({currentReportID}) { bottom: 0, }} isSmallScreenWidth={false} - currentReportID={currentReportID} /> ); } -MockedSidebarLinks.propTypes = { - currentReportID: PropTypes.string, -}; - -MockedSidebarLinks.defaultProps = { - currentReportID: '', -}; - export {fakePersonalDetails, getDefaultRenderedSidebarLinks, getAdvancedFakeReport, getFakeReport, getFakeReportAction, MockedSidebarLinks}; From 8bfad23478c152009931f7dd895314d01b5602fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 7 Jul 2023 23:38:38 +0200 Subject: [PATCH 11/54] wip: data wrapper component - Introduced a data wrppaer component, which gets connected to onyx and constructs the report IDs from all the data - Only a stable ref of report IDs gets passed to the actual sidebarlinks --- src/components/LHNOptionsList/OptionRowLHN.js | 2 + src/libs/OptionsListUtils.js | 3 +- src/libs/ReportUtils.js | 10 +- src/libs/SidebarUtils.js | 34 +-- src/pages/home/sidebar/SidebarLinks.js | 196 ++-------------- src/pages/home/sidebar/SidebarLinksData.js | 209 ++++++++++++++++++ .../SidebarScreen/BaseSidebarScreen.js | 4 +- tests/unit/SidebarFilterTest.js | 111 +++++----- tests/utils/LHNTestUtils.js | 7 +- 9 files changed, 313 insertions(+), 263 deletions(-) create mode 100644 src/pages/home/sidebar/SidebarLinksData.js diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 61a922a281ed..a58ffb92d6b3 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -72,6 +72,8 @@ const defaultProps = { function BaseOptionRowLHN(props) { const optionItem = props.optionItem; + console.log('optionItem', optionItem); + const [isContextMenuActive, setIsContextMenuActive] = useState(false); useEffect(() => { diff --git a/src/libs/OptionsListUtils.js b/src/libs/OptionsListUtils.js index e76cb366206c..a81577efb768 100644 --- a/src/libs/OptionsListUtils.js +++ b/src/libs/OptionsListUtils.js @@ -569,6 +569,7 @@ function isCurrentUser(userDetails) { return _.some(_.keys(loginList), (login) => login.toLowerCase() === userDetailsLogin.toLowerCase()); } +// TODO: add current report id back? /** * Build the options * @@ -624,7 +625,7 @@ function getOptions( const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number.e164 : searchInputValue; // Filter out all the reports that shouldn't be displayed - const filteredReports = _.filter(reports, (report) => ReportUtils.shouldReportBeInOptionList(report, Navigation.getTopmostReportId(), false, iouReports, betas, policies)); + const filteredReports = _.filter(reports, (report) => ReportUtils.shouldReportBeInOptionList(report, false, iouReports, betas, policies)); // Sorting the reports works like this: // - Order everything by the last message timestamp (descending) diff --git a/src/libs/ReportUtils.js b/src/libs/ReportUtils.js index a0912703fab4..94a5cdb82561 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -1959,13 +1959,14 @@ function canAccessReport(report, policies, betas) { * filter out the majority of reports before filtering out very specific minority of reports. * * @param {Object} report + * @param {String} currentReportId * @param {Boolean} isInGSDMode * @param {Object} iouReports * @param {String[]} betas * @param {Object} policies * @returns {boolean} */ -function shouldReportBeInOptionList(report, isInGSDMode, iouReports, betas, policies) { +function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouReports, betas, policies) { const isInDefaultMode = !isInGSDMode; // Exclude reports that have no data because there wouldn't be anything to show in the option item. @@ -1983,6 +1984,13 @@ function shouldReportBeInOptionList(report, isInGSDMode, iouReports, betas, poli return false; } + // Include the currently viewed report. If we excluded the currently viewed report, then there + // would be no way to highlight it in the options list and it would be confusing to users because they lose + // a sense of context. + if (report.reportID === currentReportId) { + return true; + } + // Include reports if they have a draft, are pinned, or have an outstanding IOU // These are always relevant to the user no matter what view mode the user prefers if (report.hasDraft || report.isPinned || hasOutstandingIOU(report, iouReports)) { diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index c9cc2b2255dd..6156b4be8fdd 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -33,18 +33,6 @@ Onyx.connect({ 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 = {}; @@ -71,13 +59,6 @@ Onyx.connect({ }, }); -let policies; -Onyx.connect({ - key: ONYXKEYS.COLLECTION.POLICY, - waitForCollectionCallback: true, - callback: (val) => (policies = val), -}); - let currentUserAccountID; Onyx.connect({ key: ONYXKEYS.SESSION, @@ -117,17 +98,22 @@ function setIsSidebarLoadedReady() { } /** + * @param {String} currentReportId + * @param {Object} allReportsDict + * @param {Object} betas + * @param {String[]} policies + * @param {String} priorityMode * @returns {String[]} An array of reportIDs sorted in the proper order */ -function getOrderedReportIDs() { +function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, priorityMode) { 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, isInGSDMode, allReports, betas, policies)); + const reportsToDisplay = _.filter(allReportsDict, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReportsDict, betas, policies)); 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); } @@ -142,7 +128,7 @@ function getOrderedReportIDs() { 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: @@ -167,7 +153,7 @@ function getOrderedReportIDs() { return; } - if (report.hasOutstandingIOU && !ReportUtils.isIOUOwnedByCurrentUser(report, allReports)) { + if (report.hasOutstandingIOU && !ReportUtils.isIOUOwnedByCurrentUser(report, allReportsDict)) { outstandingIOUReports.push(report); return; } diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index e7e07a046995..fed8ddc22c11 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,14 +16,12 @@ 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 withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation'; @@ -39,84 +36,41 @@ 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), + /** Whether we are viewing below the responsive breakpoint */ + isSmallScreenWidth: PropTypes.bool.isRequired, - /** The personal details of the person who is logged in */ - currentUserPersonalDetails: PropTypes.shape({ - /** Display name of the current user */ - displayName: PropTypes.string, + onLayout: PropTypes.func, +}; - /** Avatar URL or SVG of the current user */ - avatar: PropTypes.oneOfType([PropTypes.string, PropTypes.func]), +const propTypes = { + ...basePropTypes, - /** Login email of the current user */ - login: PropTypes.string, + optionListItems: PropTypes.arrayOf(PropTypes.string).isRequired, - /** AccountID of the current user */ - accountID: PropTypes.number, - }), + isLoading: PropTypes.bool.isRequired, - /** Whether we are viewing below the responsive breakpoint */ - isSmallScreenWidth: PropTypes.bool.isRequired, - - /** The chat priority mode */ - priorityMode: PropTypes.string, - - /** Details about any modals being used */ - modal: PropTypes.shape({ - /** Indicates when an Alert modal is about to be visible */ - willAlertModalBecomeVisible: PropTypes.bool, - }), + currentUserPersonalDetails: personalDetailsPropType, ...withLocalizePropTypes, ...withNavigationPropTypes, }; const defaultProps = { - chatReports: {}, - reportActions: {}, - personalDetails: {}, currentUserPersonalDetails: { avatar: '', }, - priorityMode: CONST.PRIORITY_MODE.DEFAULT, - modal: {}, }; -class SidebarLinks extends React.Component { +class SidebarLinks extends React.PureComponent { constructor(props) { super(props); @@ -203,10 +157,8 @@ class SidebarLinks extends React.Component { } render() { - const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports); - const optionListItems = SidebarUtils.getOrderedReportIDs(); - const skeletonPlaceholder = ; + console.count('HannoDebug render SidebarLinks'); return ( - {isLoading ? ( + {this.props.isLoading ? ( skeletonPlaceholder ) : ( option.toString() === this.props.currentReportID)} + data={this.props.optionListItems} + // TODO: this is using currentReportID which doesn't exist anymore! + focusedIndex={_.findIndex(this.props.optionListItems, (option) => option.toString() === this.props.currentReportID)} onSelectRow={this.showReportPage} shouldDisableFocusOptions={this.props.isSmallScreenWidth} optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT} @@ -283,116 +236,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, - participants: report.participants, - hasDraft: report.hasDraft, - isPinned: report.isPinned, - errorFields: { - addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom, - }, - 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, - }; - -/** - * @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, - 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, - }, - }), -)(SidebarLinks); +export default compose(withLocalize, withCurrentUserPersonalDetails, withNavigationFocus, withWindowDimensions, withNavigation)(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..14c84b161440 --- /dev/null +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -0,0 +1,209 @@ +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 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 participantPropTypes from '../../../components/participantPropTypes'; +import CONST from '../../../CONST'; +import * as UserUtils from '../../../libs/UserUtils'; + +const propTypes = { + ...basePropTypes, + + /* Onyx Props */ + /** List of reports */ + chatReports: PropTypes.objectOf(reportPropTypes), + + // TODO: i think this can be removed? + /** All report actions for all reports */ + 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 chat priority mode */ + priorityMode: PropTypes.string, + + betas: PropTypes.arrayOf(PropTypes.string), + policies: PropTypes.shape({}), + + // TODO: this can maybe be removed + preferredLocale: PropTypes.string, +}; + +const defaultProps = { + chatReports: {}, + reportActions: {}, + personalDetails: {}, + priorityMode: CONST.PRIORITY_MODE.DEFAULT, + betas: [], + policies: [], + preferredLocale: CONST.DEFAULT_LOCALE, +}; + +function SidebarLinksData(props) { + const prevReportIDs = useRef([]); + const optionListItems = useMemo(() => { + const reportIDs = SidebarUtils.getOrderedReportIDs(props.currentReportID, props.chatReports, props.betas, props.policies, props.priorityMode); + if (deepEqual(prevReportIDs.current, reportIDs)) { + return prevReportIDs.current; + } + prevReportIDs.current = reportIDs; + return reportIDs; + }, [props.betas, props.chatReports, props.currentReportID, props.policies, props.priorityMode]); + + const isLoading = _.isEmpty(props.personalDetails) || _.isEmpty(props.chatReports); + + 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, + hasOutstandingIOU: report.hasOutstandingIOU, + statusNum: report.statusNum, + stateNum: report.stateNum, + chatType: report.chatType, + type: report.type, + policyID: report.policyID, + reportName: report.reportName, + visibility: report.visibility, + }; + +/** + * @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( + withCurrentReportID, + 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, + }, + // TOOD: why do we need this? + preferredLocale: { + key: ONYXKEYS.NVP_PREFERRED_LOCALE, + }, + }), +)(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(() => { + const multiSetImpl = Onyx.multiSet; + Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + return 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(() => { @@ -97,13 +101,14 @@ describe('Sidebar', () => { .then(() => Onyx.multiSet({ [ONYXKEYS.BETAS]: [CONST.BETAS.POLICY_EXPENSE_CHAT], - }), + }).then(waitForPromisesToResolve), ) // Then there is one report rendered in the LHN .then(() => { const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); const optionRows = screen.queryAllByAccessibilityHint(hintText); + console.log('the test is deemed a failure'); expect(optionRows).toHaveLength(1); }) ); @@ -290,57 +295,57 @@ describe('Sidebar', () => { // Taken from https://stackoverflow.com/a/39734979/9114791 to generate all possible boolean combinations const AMOUNT_OF_VARIABLES = 6; // eslint-disable-next-line no-bitwise - for (let i = 0; i < 1 << AMOUNT_OF_VARIABLES; i++) { - const boolArr = []; - for (let j = AMOUNT_OF_VARIABLES - 1; j >= 0; j--) { - // eslint-disable-next-line no-bitwise - boolArr.push(Boolean(i & (1 << j))); - } - - // 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]; - - it(`the booleans ${JSON.stringify(boolArr)}`, () => { - const report2 = { - ...LHNTestUtils.getAdvancedFakeReport(...boolArr), - policyID: policy.policyID, - }; - LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID); - - return ( - waitForPromisesToResolve() - // When Onyx is updated to contain that data and the sidebar re-renders - .then(() => - Onyx.multiSet({ - [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.GSD, - [ONYXKEYS.BETAS]: betas, - [ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails, - [`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`]: report1, - [`${ONYXKEYS.COLLECTION.REPORT}${report2.reportID}`]: report2, - [`${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) { - // Only one report visible - const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); - const displayNames = screen.queryAllByLabelText(displayNamesHintText); - const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); - expect(displayNames).toHaveLength(1); - expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); - } else { - // Both reports visible - const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(2); - } - }) - ); - }); - } + // for (let i = 0; i < 1 << AMOUNT_OF_VARIABLES; i++) { + // const boolArr = []; + // for (let j = AMOUNT_OF_VARIABLES - 1; j >= 0; j--) { + // // eslint-disable-next-line no-bitwise + // boolArr.push(Boolean(i & (1 << j))); + // } + + // 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]; + + it(`the booleans [false,false,false,false,false,false]`, () => { + const report2 = { + ...LHNTestUtils.getAdvancedFakeReport(...[false, false, false, false, false, false]), + policyID: policy.policyID, + }; + LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID); + + return ( + waitForPromisesToResolve() + // When Onyx is updated to contain that data and the sidebar re-renders + .then(() => + Onyx.multiSet({ + [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.GSD, + [ONYXKEYS.BETAS]: betas, + [ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails, + [`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`]: report1, + [`${ONYXKEYS.COLLECTION.REPORT}${report2.reportID}`]: report2, + [`${ONYXKEYS.COLLECTION.POLICY}${policy.policyID}`]: policy, + }), + ) + + // Then depending on the outcome, either one or two reports are visible + .then(() => { + if (booleansWhichRemovesActiveReport.indexOf(JSON.stringify([false, false, false, false, false, false])) > -1) { + // Only one report visible + const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); + const displayNames = screen.queryAllByLabelText(displayNamesHintText); + const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); + expect(displayNames).toHaveLength(1); + expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); + } else { + // Both reports visible + const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(2); + } + }) + ); + }); + // } }); }); diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index e2c94d1dcb80..9f561a6d8012 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -164,10 +164,7 @@ function getAdvancedFakeReport(isArchived, isUserCreatedPolicyRoom, hasAddWorksp }; } -/** - * @param {String} [currentReportID] - */ -function getDefaultRenderedSidebarLinks(currentReportID = '') { +function getDefaultRenderedSidebarLinks() { // A try-catch block needs to be added to the rendering so that any errors that happen while the component // renders are caught and logged to the console. Without the try-catch block, Jest might only report the error // as "The above error occurred in your component", without providing specific details. By using a try-catch block, @@ -179,7 +176,7 @@ function getDefaultRenderedSidebarLinks(currentReportID = '') { // are passed to the component. If this is not done, then all the locale props are missing // and there are a lot of render warnings. It needs to be done like this because normally in // our app (App.js) is when the react application is wrapped in the context providers - render(); + render(); } catch (error) { console.error(error); } From 0bdd60868181118e5852b3645497e8c4b56deb70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 7 Jul 2023 23:40:36 +0200 Subject: [PATCH 12/54] logging --- src/pages/home/sidebar/SidebarLinksData.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 14c84b161440..f7cf37f60143 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -76,6 +76,7 @@ function SidebarLinksData(props) { const isLoading = _.isEmpty(props.personalDetails) || _.isEmpty(props.chatReports); + console.count('HannoDebug render SidebarLinksData'); return ( Date: Fri, 7 Jul 2023 23:48:17 +0200 Subject: [PATCH 13/54] wip: fixing tests --- tests/utils/LHNTestUtils.js | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index 9f561a6d8012..415f6442a6a9 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -1,12 +1,13 @@ import React from 'react'; +import PropTypes from 'prop-types'; 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 SidebarLinks from '../../src/pages/home/sidebar/SidebarLinks'; +import SidebarLinksData from '../../src/pages/home/sidebar/SidebarLinksData'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; -import {CurrentReportIDContextProvider} from '../../src/components/withCurrentReportID'; +import {CurrentReportIDContext} from '../../src/components/withCurrentReportID'; // we have to mock `useIsFocused` because it's used in the SidebarLinks component const mockedNavigate = jest.fn(); @@ -186,10 +187,25 @@ function getDefaultRenderedSidebarLinks() { * @param {String} [currentReportID] * @returns {JSX.Element} */ -function MockedSidebarLinks() { +function MockedSidebarLinks({currentReportID}) { return ( - - ( + {}, + currentReportID, + }} + > + {children} + + ), + ]} + > + {}} insets={{ top: 0, @@ -203,4 +219,12 @@ function MockedSidebarLinks() { ); } +MockedSidebarLinks.propTypes = { + currentReportID: PropTypes.string, +}; + +MockedSidebarLinks.defaultProps = { + currentReportID: '', +}; + export {fakePersonalDetails, getDefaultRenderedSidebarLinks, getAdvancedFakeReport, getFakeReport, getFakeReportAction, MockedSidebarLinks}; From 640753c1a9e07e59b16ecf8bbe25c948f3e46569 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 11:32:43 +0200 Subject: [PATCH 14/54] add currentReportID back --- src/libs/OptionsListUtils.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libs/OptionsListUtils.js b/src/libs/OptionsListUtils.js index a81577efb768..e76cb366206c 100644 --- a/src/libs/OptionsListUtils.js +++ b/src/libs/OptionsListUtils.js @@ -569,7 +569,6 @@ function isCurrentUser(userDetails) { return _.some(_.keys(loginList), (login) => login.toLowerCase() === userDetailsLogin.toLowerCase()); } -// TODO: add current report id back? /** * Build the options * @@ -625,7 +624,7 @@ function getOptions( const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number.e164 : searchInputValue; // Filter out all the reports that shouldn't be displayed - const filteredReports = _.filter(reports, (report) => ReportUtils.shouldReportBeInOptionList(report, false, iouReports, betas, policies)); + const filteredReports = _.filter(reports, (report) => ReportUtils.shouldReportBeInOptionList(report, Navigation.getTopmostReportId(), false, iouReports, betas, policies)); // Sorting the reports works like this: // - Order everything by the last message timestamp (descending) From a84e26f9f57fa37537440ca63719dec3bce52ed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 11:37:38 +0200 Subject: [PATCH 15/54] wip(test): simpler context usage --- tests/utils/LHNTestUtils.js | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index 415f6442a6a9..d21077176b68 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -7,7 +7,6 @@ import {LocaleContextProvider} from '../../src/components/withLocalize'; import SidebarLinksData from '../../src/pages/home/sidebar/SidebarLinksData'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; -import {CurrentReportIDContext} from '../../src/components/withCurrentReportID'; // we have to mock `useIsFocused` because it's used in the SidebarLinks component const mockedNavigate = jest.fn(); @@ -165,7 +164,7 @@ function getAdvancedFakeReport(isArchived, isUserCreatedPolicyRoom, hasAddWorksp }; } -function getDefaultRenderedSidebarLinks() { +function getDefaultRenderedSidebarLinks(currentReportID) { // A try-catch block needs to be added to the rendering so that any errors that happen while the component // renders are caught and logged to the console. Without the try-catch block, Jest might only report the error // as "The above error occurred in your component", without providing specific details. By using a try-catch block, @@ -177,7 +176,7 @@ function getDefaultRenderedSidebarLinks() { // are passed to the component. If this is not done, then all the locale props are missing // and there are a lot of render warnings. It needs to be done like this because normally in // our app (App.js) is when the react application is wrapped in the context providers - render(); + render(); } catch (error) { console.error(error); } @@ -193,16 +192,6 @@ function MockedSidebarLinks({currentReportID}) { components={[ OnyxProvider, LocaleContextProvider, - ({children}) => ( - {}, - currentReportID, - }} - > - {children} - - ), ]} > ); From b7cb24df1c1e080cb93483005d28758f8e173e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 11:57:47 +0200 Subject: [PATCH 16/54] wip: tests --- src/components/LHNOptionsList/LHNOptionsList.js | 2 ++ src/components/LHNOptionsList/OptionRowLHN.js | 2 -- src/pages/home/sidebar/SidebarLinks.js | 1 + src/pages/home/sidebar/SidebarLinksData.js | 2 +- tests/unit/SidebarFilterTest.js | 13 +++++++------ tests/utils/LHNTestUtils.js | 12 +++++------- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index a81accc3aed6..bfbb0e08b87c 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -86,6 +86,8 @@ class LHNOptionsList extends Component { this.data = this.props.data; } + console.log("List rendered with data:", this.data.length); + return ( { diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index fed8ddc22c11..a60dfa24ade7 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -225,6 +225,7 @@ class SidebarLinks extends React.PureComponent { focusedIndex={_.findIndex(this.props.optionListItems, (option) => option.toString() === this.props.currentReportID)} onSelectRow={this.showReportPage} shouldDisableFocusOptions={this.props.isSmallScreenWidth} + // TODO: i think this option mode is broken as well, as i removed the prop? 👀 optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT} /> )} diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index f7cf37f60143..f5a2829702ae 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -202,7 +202,7 @@ export default compose( key: ONYXKEYS.COLLECTION.POLICY, selector: policySelector, }, - // TOOD: why do we need this? + // TODO: why do we need this? preferredLocale: { key: ONYXKEYS.NVP_PREFERRED_LOCALE, }, diff --git a/tests/unit/SidebarFilterTest.js b/tests/unit/SidebarFilterTest.js index 9c70da4e5d6e..03ddad0a4e9d 100644 --- a/tests/unit/SidebarFilterTest.js +++ b/tests/unit/SidebarFilterTest.js @@ -35,7 +35,7 @@ describe('Sidebar', () => { beforeEach(() => { const multiSetImpl = Onyx.multiSet; Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); - return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + 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 @@ -258,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]); @@ -304,11 +305,11 @@ 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, false]; - it(`the booleans [false,false,false,false,false,false]`, () => { + it(`the booleans ${boolArr}`, () => { const report2 = { - ...LHNTestUtils.getAdvancedFakeReport(...[false, false, false, false, false, false]), + ...LHNTestUtils.getAdvancedFakeReport(...boolArr), policyID: policy.policyID, }; LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID); @@ -326,14 +327,14 @@ 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([false, false, false, false, false, false])) > -1) { + if (booleansWhichRemovesActiveReport.indexOf(JSON.stringify(boolArr)) > -1) { // Only one report visible const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); const displayNames = screen.queryAllByLabelText(displayNamesHintText); const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + console.log('TEST DEEMED A FAILURE!') expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); expect(displayNames).toHaveLength(1); expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); diff --git a/tests/utils/LHNTestUtils.js b/tests/utils/LHNTestUtils.js index d21077176b68..fdb4f239cfa8 100644 --- a/tests/utils/LHNTestUtils.js +++ b/tests/utils/LHNTestUtils.js @@ -164,7 +164,10 @@ function getAdvancedFakeReport(isArchived, isUserCreatedPolicyRoom, hasAddWorksp }; } -function getDefaultRenderedSidebarLinks(currentReportID) { +/** + * @param {String} [currentReportID] + */ +function getDefaultRenderedSidebarLinks(currentReportID = '') { // A try-catch block needs to be added to the rendering so that any errors that happen while the component // renders are caught and logged to the console. Without the try-catch block, Jest might only report the error // as "The above error occurred in your component", without providing specific details. By using a try-catch block, @@ -188,12 +191,7 @@ function getDefaultRenderedSidebarLinks(currentReportID) { */ function MockedSidebarLinks({currentReportID}) { return ( - + {}} insets={{ From cdb46d62d978de2aff10d4f4a8d66b1fad518c53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:05:52 +0200 Subject: [PATCH 17/54] wip: tests --- tests/unit/SidebarFilterTest.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/SidebarFilterTest.js b/tests/unit/SidebarFilterTest.js index 03ddad0a4e9d..0f88bf533b37 100644 --- a/tests/unit/SidebarFilterTest.js +++ b/tests/unit/SidebarFilterTest.js @@ -101,14 +101,13 @@ describe('Sidebar', () => { .then(() => Onyx.multiSet({ [ONYXKEYS.BETAS]: [CONST.BETAS.POLICY_EXPENSE_CHAT], - }).then(waitForPromisesToResolve), + }), ) // Then there is one report rendered in the LHN .then(() => { const hintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); const optionRows = screen.queryAllByAccessibilityHint(hintText); - console.log('the test is deemed a failure'); expect(optionRows).toHaveLength(1); }) ); @@ -334,7 +333,7 @@ describe('Sidebar', () => { const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); const displayNames = screen.queryAllByLabelText(displayNamesHintText); const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - console.log('TEST DEEMED A FAILURE!') + console.log('TEST DEEMED A FAILURE!'); expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); expect(displayNames).toHaveLength(1); expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); From 69e46ae055c13a52ef9c0658394b6bf07b51fd28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:12:13 +0200 Subject: [PATCH 18/54] include lastReadTime in wrapper component as its needed to compute GSD mode list correctly --- src/pages/home/sidebar/SidebarLinksData.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index f5a2829702ae..fa4ea37676a4 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -122,6 +122,7 @@ const chatReportSelector = (report) => policyID: report.policyID, reportName: report.reportName, visibility: report.visibility, + lastReadTime: report.lastReadTime, }; /** From 4128c8287e2068f2e89103410879b8c0b3e823cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:15:44 +0200 Subject: [PATCH 19/54] fiz style in focus mode --- src/pages/home/sidebar/SidebarLinks.js | 4 +++- src/pages/home/sidebar/SidebarLinksData.js | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index a60dfa24ade7..b9029f332fa2 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -60,6 +60,8 @@ const propTypes = { currentUserPersonalDetails: personalDetailsPropType, + priorityMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), + ...withLocalizePropTypes, ...withNavigationPropTypes, }; @@ -68,6 +70,7 @@ const defaultProps = { currentUserPersonalDetails: { avatar: '', }, + priorityMode: CONST.PRIORITY_MODE.DEFAULT, }; class SidebarLinks extends React.PureComponent { @@ -225,7 +228,6 @@ class SidebarLinks extends React.PureComponent { focusedIndex={_.findIndex(this.props.optionListItems, (option) => option.toString() === this.props.currentReportID)} onSelectRow={this.showReportPage} shouldDisableFocusOptions={this.props.isSmallScreenWidth} - // TODO: i think this option mode is broken as well, as i removed the prop? 👀 optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT} /> )} diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index fa4ea37676a4..9f0f268f9773 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -84,6 +84,7 @@ function SidebarLinksData(props) { insets={props.insets} isSmallScreenWidth={props.isSmallScreenWidth} onLayout={props.onLayout} + priorityMode={props.priorityMode} // Data props: isLoading={isLoading} optionListItems={optionListItems} From 8a1a5c29320a9b19e9262255b3de8ead22f0b650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:20:31 +0200 Subject: [PATCH 20/54] fix SidebarFilterTests --- tests/unit/SidebarFilterTest.js | 104 ++++++++++++++++---------------- 1 file changed, 53 insertions(+), 51 deletions(-) diff --git a/tests/unit/SidebarFilterTest.js b/tests/unit/SidebarFilterTest.js index 0f88bf533b37..51b64ad9b9ce 100644 --- a/tests/unit/SidebarFilterTest.js +++ b/tests/unit/SidebarFilterTest.js @@ -33,8 +33,11 @@ describe('Sidebar', () => { // Initialize the network key for OfflineWithFeedback beforeEach(() => { + // TODO: explain this (investigate whether we render a batch later now!) const multiSetImpl = Onyx.multiSet; Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + const mergeImpl = Onyx.merge; + Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); @@ -295,57 +298,56 @@ describe('Sidebar', () => { // Taken from https://stackoverflow.com/a/39734979/9114791 to generate all possible boolean combinations const AMOUNT_OF_VARIABLES = 6; // eslint-disable-next-line no-bitwise - // for (let i = 0; i < 1 << AMOUNT_OF_VARIABLES; i++) { - // const boolArr = []; - // for (let j = AMOUNT_OF_VARIABLES - 1; j >= 0; j--) { - // // eslint-disable-next-line no-bitwise - // boolArr.push(Boolean(i & (1 << j))); - // } - - // 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, false, false, false, false]; - - it(`the booleans ${boolArr}`, () => { - const report2 = { - ...LHNTestUtils.getAdvancedFakeReport(...boolArr), - policyID: policy.policyID, - }; - LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID); - - return ( - waitForPromisesToResolve() - // When Onyx is updated to contain that data and the sidebar re-renders - .then(() => - Onyx.multiSet({ - [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.GSD, - [ONYXKEYS.BETAS]: betas, - [ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails, - [`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`]: report1, - [`${ONYXKEYS.COLLECTION.REPORT}${report2.reportID}`]: report2, - [`${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) { - // Only one report visible - const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); - const displayNames = screen.queryAllByLabelText(displayNamesHintText); - const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - console.log('TEST DEEMED A FAILURE!'); - expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); - expect(displayNames).toHaveLength(1); - expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); - } else { - // Both reports visible - const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); - expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(2); - } - }) - ); - }); - // } + for (let i = 0; i < 1 << AMOUNT_OF_VARIABLES; i++) { + const boolArr = []; + for (let j = AMOUNT_OF_VARIABLES - 1; j >= 0; j--) { + // eslint-disable-next-line no-bitwise + boolArr.push(Boolean(i & (1 << j))); + } + + // 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, false, false, false]; + + it(`the booleans ${boolArr}`, () => { + const report2 = { + ...LHNTestUtils.getAdvancedFakeReport(...boolArr), + policyID: policy.policyID, + }; + LHNTestUtils.getDefaultRenderedSidebarLinks(report1.reportID); + + return ( + waitForPromisesToResolve() + // When Onyx is updated to contain that data and the sidebar re-renders + .then(() => + Onyx.multiSet({ + [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.GSD, + [ONYXKEYS.BETAS]: betas, + [ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails, + [`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`]: report1, + [`${ONYXKEYS.COLLECTION.REPORT}${report2.reportID}`]: report2, + [`${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) { + // Only one report visible + const displayNamesHintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames'); + const displayNames = screen.queryAllByLabelText(displayNamesHintText); + const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1); + expect(displayNames).toHaveLength(1); + expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four'); + } else { + // Both reports visible + const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat'); + expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(2); + } + }) + ); + }); + } }); }); From f0d54a783b38033cac246beaac6e58588d0129d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:31:19 +0200 Subject: [PATCH 21/54] wip: fixed all tests temporarily --- tests/unit/SidebarOrderTest.js | 8 +++++++- tests/unit/SidebarTest.js | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/unit/SidebarOrderTest.js b/tests/unit/SidebarOrderTest.js index 16cbd1c40698..8dcdfb51a696 100644 --- a/tests/unit/SidebarOrderTest.js +++ b/tests/unit/SidebarOrderTest.js @@ -32,7 +32,13 @@ describe('Sidebar', () => { ); // Initialize the network key for OfflineWithFeedback - beforeEach(() => Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false})); + beforeEach(() => { + const multiSetImpl = Onyx.multiSet; + Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + const mergeImpl = Onyx.merge; + Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + 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..69875415409c 100644 --- a/tests/unit/SidebarTest.js +++ b/tests/unit/SidebarTest.js @@ -31,7 +31,13 @@ describe('Sidebar', () => { ); // Initialize the network key for OfflineWithFeedback - beforeEach(() => Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false})); + beforeEach(() => { + const multiSetImpl = Onyx.multiSet; + Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + const mergeImpl = Onyx.merge; + Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); + }); // Clear out Onyx after each test so that each test starts with a clean slate afterEach(() => { From 7c5521a871f4039f7aa13a4a1bbf9c30ab0c06ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Sat, 8 Jul 2023 12:38:28 +0200 Subject: [PATCH 22/54] removed focused index as its obsolete --- src/components/LHNOptionsList/LHNOptionsList.js | 11 +++-------- src/components/LHNOptionsList/OptionRowLHN.js | 9 +++++++-- src/pages/home/sidebar/SidebarLinks.js | 2 -- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index bfbb0e08b87c..c2b7816467a5 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -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, @@ -65,16 +62,15 @@ class LHNOptionsList extends Component { * * @param {Object} params * @param {Object} params.item - * @param {Number} params.index * * @return {Component} */ - renderItem({item, index}) { + renderItem({item}) { return ( ); @@ -86,7 +82,7 @@ class LHNOptionsList extends Component { this.data = this.props.data; } - console.log("List rendered with data:", this.data.length); + console.log('List rendered with data:', this.data.length); return ( @@ -100,7 +96,6 @@ class LHNOptionsList extends Component { stickySectionHeadersEnabled={false} renderItem={this.renderItem} getItemLayout={this.getItemLayout} - extraData={this.props.focusedIndex} initialNumToRender={5} maxToRenderPerBatch={5} windowSize={5} diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 61a922a281ed..60120a2b1cf0 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -51,6 +51,8 @@ const propTypes = { /** Toggle between compact and default view */ viewMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), + shouldDisableFocusOptions: PropTypes.bool, + style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), optionItem: PropTypes.shape({}), @@ -63,7 +65,7 @@ const defaultProps = { hoverStyle: styles.sidebarLinkHover, viewMode: 'default', onSelectRow: () => {}, - isFocused: false, + shouldDisableFocusOptions: false, style: null, optionItem: null, comment: '', @@ -292,11 +294,14 @@ const MemoedOptionRowLHN = React.memo( )(BaseOptionRowLHN), ); +// TODO: cleaner split which components receives which props. +// maybe even create a data component here as well? + // We only want to pass a boolean to the memoized // component, thats why we have this intermediate component. // (We don't want to fully re-render all items, just because the active report changed). function OptionRowLHN(props) { - const isFocused = props.currentReportId === props.reportID; + const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; return ( option.toString() === this.props.currentReportID)} onSelectRow={this.showReportPage} shouldDisableFocusOptions={this.props.isSmallScreenWidth} optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? CONST.OPTION_MODE.COMPACT : CONST.OPTION_MODE.DEFAULT} From 657fad87119d9791b2da0bad93cf97e68cb4200b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 10:57:54 +0200 Subject: [PATCH 23/54] removed logging statements --- src/components/LHNOptionsList/LHNOptionsList.js | 2 -- src/pages/home/sidebar/SidebarLinks.js | 1 - src/pages/home/sidebar/SidebarLinksData.js | 1 - 3 files changed, 4 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index c2b7816467a5..6fff0018b0f1 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -82,8 +82,6 @@ class LHNOptionsList extends Component { this.data = this.props.data; } - console.log('List rendered with data:', this.data.length); - return ( ; - console.count('HannoDebug render SidebarLinks'); return ( Date: Mon, 10 Jul 2023 12:16:23 +0200 Subject: [PATCH 24/54] split up components + use deepEqual in react memo --- .../LHNOptionsList/LHNOptionsList.js | 4 +- src/components/LHNOptionsList/OptionRowLHN.js | 85 ++++--------------- .../LHNOptionsList/OptionRowLHNData.js | 64 ++++++++++++++ 3 files changed, 84 insertions(+), 69 deletions(-) create mode 100644 src/components/LHNOptionsList/OptionRowLHNData.js diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index 6fff0018b0f1..6846d975f011 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -3,7 +3,7 @@ import React, {Component} from 'react'; import {View, FlatList} from 'react-native'; import PropTypes from 'prop-types'; import styles from '../../styles/styles'; -import OptionRowLHN from './OptionRowLHN'; +import OptionRowLHNData from './OptionRowLHNData'; import variables from '../../styles/variables'; import CONST from '../../CONST'; @@ -67,7 +67,7 @@ class LHNOptionsList extends Component { */ renderItem({item}) { return ( - {}, - shouldDisableFocusOptions: false, style: null, optionItem: null, comment: '', - ...withCurrentReportIDDefaultProps, }; -function BaseOptionRowLHN(props) { +function OptionRowLHN(props) { + const localize = useLocalize(); + const optionItem = props.optionItem; const [isContextMenuActive, setIsContextMenuActive] = useState(false); @@ -170,7 +160,7 @@ function BaseOptionRowLHN(props) { (hovered || isContextMenuActive) && !props.isFocused ? props.hoverStyle : null, ]} accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} - accessibilityLabel={props.translate('accessibilityHints.navigatesToChat')} + accessibilityLabel={localize.translate('accessibilityHints.navigatesToChat')} > @@ -198,7 +188,7 @@ function BaseOptionRowLHN(props) { {optionItem.alternateText} @@ -247,7 +237,7 @@ function BaseOptionRowLHN(props) { {optionItem.hasDraftComment && ( @@ -255,7 +245,7 @@ function BaseOptionRowLHN(props) { {!shouldShowGreenDotIndicator && optionItem.isPinned && ( @@ -268,52 +258,13 @@ function BaseOptionRowLHN(props) { ); } -BaseOptionRowLHN.propTypes = propTypes; -BaseOptionRowLHN.defaultProps = defaultProps; -BaseOptionRowLHN.displayName = 'BaseOptionRowLHN'; - -const MemoedOptionRowLHN = React.memo( - compose( - withLocalize, - withOnyx({ - optionItem: { - key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, - selector: SidebarUtils.getOptionData, - }, - }), - )(BaseOptionRowLHN), -); - -// TODO: cleaner split which components receives which props. -// maybe even create a data component here as well? - -// We only want to pass a boolean to the memoized -// component, thats why we have this intermediate component. -// (We don't want to fully re-render all items, just because the active report changed). -function OptionRowLHN(props) { - const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; - - return ( - - ); -} - OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; OptionRowLHN.displayName = 'OptionRowLHN'; -export default compose( - withLocalize, - withCurrentReportID, - withReportCommentDrafts({ - propName: 'comment', - transformValue: (drafts, props) => { - const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`; - return lodashGet(drafts, draftKey, ''); - }, - }), -)(OptionRowLHN); +// We use deepEqual here, as the selectors results for optionItem +// aren't stable, meaning that content-wise the object is the same, +// but the reference changed. +export default React.memo(OptionRowLHN, deepEqual); + +export {propTypes, defaultProps}; diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js new file mode 100644 index 000000000000..dff74332ca9a --- /dev/null +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -0,0 +1,64 @@ +import {withOnyx} from 'react-native-onyx'; +import lodashGet from 'lodash/get'; +import _ from 'underscore'; +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 from './OptionRowLHN'; +import PropTypes from 'prop-types'; + +const propTypes = { + shouldDisableFocusOptions: PropTypes.bool, + ...withCurrentReportIDPropTypes, + ...OptionRowLHN.propTypes, +}; + +const defaultProps = { + shouldDisableFocusOptions: false, + ...withCurrentReportIDDefaultProps, + ...OptionRowLHN.defaultProps, +}; + +/** + * 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(props) { + // We only want to pass a boolean to the memoized + // component, thats why we have this intermediate component. + // (We don't want to fully re-render all items, just because the active report changed). + const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; + + return ( + + ); +} + +OptionRowLHNData.propTypes = propTypes; +OptionRowLHNData.defaultProps = defaultProps; +OptionRowLHNData.displayName = 'OptionRowLHNData'; + +export default compose( + withCurrentReportID, + withReportCommentDrafts({ + propName: 'comment', + transformValue: (drafts, props) => { + const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`; + return lodashGet(drafts, draftKey, ''); + }, + }), + withOnyx({ + optionItem: { + key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, + selector: SidebarUtils.getOptionData, + }, + }), +)(OptionRowLHNData); From 6c296adc4a45fee07a86490cc1bc09c8c6b621d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 12:21:41 +0200 Subject: [PATCH 25/54] fix prop types --- src/components/LHNOptionsList/OptionRowLHNData.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index dff74332ca9a..3ea3a3be30cc 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -6,19 +6,19 @@ import SidebarUtils from '../../libs/SidebarUtils'; import compose from '../../libs/compose'; import ONYXKEYS from '../../ONYXKEYS'; import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; -import OptionRowLHN from './OptionRowLHN'; +import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN'; import PropTypes from 'prop-types'; const propTypes = { shouldDisableFocusOptions: PropTypes.bool, ...withCurrentReportIDPropTypes, - ...OptionRowLHN.propTypes, + ...basePropTypes, }; const defaultProps = { shouldDisableFocusOptions: false, ...withCurrentReportIDDefaultProps, - ...OptionRowLHN.defaultProps, + ...baseDefaultProps, }; /** From 8625724fa7c61ef214ce3a924d2bd1b273045045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 12:40:40 +0200 Subject: [PATCH 26/54] cleaned up tests --- tests/unit/SidebarFilterTest.js | 7 ++----- tests/unit/SidebarOrderTest.js | 6 ++---- tests/unit/SidebarTest.js | 6 ++---- .../wrapOnyxWithWaitForPromisesToResolve.js | 20 +++++++++++++++++++ 4 files changed, 26 insertions(+), 13 deletions(-) create mode 100644 tests/utils/wrapOnyxWithWaitForPromisesToResolve.js diff --git a/tests/unit/SidebarFilterTest.js b/tests/unit/SidebarFilterTest.js index 51b64ad9b9ce..7b775fc6987c 100644 --- a/tests/unit/SidebarFilterTest.js +++ b/tests/unit/SidebarFilterTest.js @@ -3,6 +3,7 @@ import Onyx from 'react-native-onyx'; import lodashGet from 'lodash/get'; import * as LHNTestUtils from '../utils/LHNTestUtils'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; +import wrapOnyxWithWaitForPromisesToResolve from '../utils/wrapOnyxWithWaitForPromisesToResolve'; import CONST from '../../src/CONST'; import DateUtils from '../../src/libs/DateUtils'; import * as Localize from '../../src/libs/Localize'; @@ -33,11 +34,7 @@ describe('Sidebar', () => { // Initialize the network key for OfflineWithFeedback beforeEach(() => { - // TODO: explain this (investigate whether we render a batch later now!) - const multiSetImpl = Onyx.multiSet; - Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); - const mergeImpl = Onyx.merge; - Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + wrapOnyxWithWaitForPromisesToResolve(Onyx); Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); diff --git a/tests/unit/SidebarOrderTest.js b/tests/unit/SidebarOrderTest.js index 8dcdfb51a696..86f193734fc5 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'; @@ -33,10 +34,7 @@ describe('Sidebar', () => { // Initialize the network key for OfflineWithFeedback beforeEach(() => { - const multiSetImpl = Onyx.multiSet; - Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); - const mergeImpl = Onyx.merge; - Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + wrapOnyxWithWaitForPromisesToResolve(Onyx); return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); diff --git a/tests/unit/SidebarTest.js b/tests/unit/SidebarTest.js index 69875415409c..a318833fca4a 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'; @@ -32,10 +33,7 @@ describe('Sidebar', () => { // Initialize the network key for OfflineWithFeedback beforeEach(() => { - const multiSetImpl = Onyx.multiSet; - Onyx.multiSet = (...args) => multiSetImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); - const mergeImpl = Onyx.merge; - Onyx.merge = (...args) => mergeImpl(...args).then((result) => waitForPromisesToResolve().then(() => result)); + wrapOnyxWithWaitForPromisesToResolve(Onyx); return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); 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)); +} From db85064145a915e6664eeabe37cfe05f2b9cb45d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 12:43:07 +0200 Subject: [PATCH 27/54] fix prop types --- src/components/LHNOptionsList/OptionRowLHN.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 2ed081125789..2c2d637190cf 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -2,6 +2,7 @@ import _ from 'underscore'; import React, {useEffect, useState} from 'react'; import PropTypes from 'prop-types'; import {View, StyleSheet} from 'react-native'; +import {deepEqual} from 'fast-equals'; import * as optionRowStyles from '../../styles/optionRowStyles'; import styles from '../../styles/styles'; import * as StyleUtils from '../../styles/StyleUtils'; @@ -22,7 +23,6 @@ import * as ContextMenuActions from '../../pages/home/report/ContextMenu/Context import * as OptionsListUtils from '../../libs/OptionsListUtils'; import * as Report from '../../libs/actions/Report'; import useLocalize from '../../hooks/useLocalize'; -import {deepEqual} from 'fast-equals'; const propTypes = { /** Style for hovered state */ @@ -45,7 +45,8 @@ const propTypes = { viewMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), /** The item that should be rendered */ - optionItem: PropTypes.shape({}), + // eslint-disable-next-line react/forbid-prop-types + optionItem: PropTypes.object, style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), }; @@ -57,6 +58,7 @@ const defaultProps = { style: null, optionItem: null, comment: '', + isFocused: false, }; function OptionRowLHN(props) { From 0afc57e922166d84fc1ff43568ab7af0dc2aeb52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 15:43:12 +0200 Subject: [PATCH 28/54] fixed last onyx data connections --- .../LHNOptionsList/OptionRowLHNData.js | 4 ++-- src/libs/ReportActionsUtils.js | 5 ++-- src/libs/ReportUtils.js | 10 ++++---- src/libs/SidebarUtils.js | 7 ++++-- src/pages/home/sidebar/SidebarLinksData.js | 23 ++++++++----------- 5 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index 3ea3a3be30cc..a528ef027471 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -1,13 +1,13 @@ import {withOnyx} from 'react-native-onyx'; import lodashGet from 'lodash/get'; import _ from 'underscore'; +import PropTypes from 'prop-types'; 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 PropTypes from 'prop-types'; const propTypes = { shouldDisableFocusOptions: PropTypes.bool, @@ -21,7 +21,7 @@ const defaultProps = { ...baseDefaultProps, }; -/** +/* * This component gets the data from onyx for the actual * OptionRowLHN component. * The OptionRowLHN component is memoized, so it will only diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 9a280711e947..0be8870225aa 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 a99c5d2c58b5..02eee3224376 100644 --- a/src/libs/ReportUtils.js +++ b/src/libs/ReportUtils.js @@ -2017,10 +2017,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; } @@ -2045,9 +2046,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. @@ -2061,7 +2063,7 @@ function shouldReportBeInOptionList(report, currentReportId, isInGSDMode, iouRep return false; } - if (!canAccessReport(report, policies, betas)) { + if (!canAccessReport(report, policies, betas, allReportActions)) { return false; } diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index cf25c23038e9..89a34043d6e4 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -103,14 +103,17 @@ function setIsSidebarLoadedReady() { * @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, allReportsDict, betas, policies, priorityMode) { +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(allReportsDict, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, allReportsDict, 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(allReportsDict, ReportUtils.isConciergeChatReport); diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index b7e8e96e2bac..da5d51a613b1 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -21,9 +21,8 @@ const propTypes = { /** List of reports */ chatReports: PropTypes.objectOf(reportPropTypes), - // TODO: i think this can be removed? /** All report actions for all reports */ - reportActions: PropTypes.objectOf( + allReportActions: PropTypes.objectOf( PropTypes.arrayOf( PropTypes.shape({ error: PropTypes.string, @@ -46,33 +45,33 @@ const propTypes = { /** The chat priority mode */ priorityMode: PropTypes.string, + /** Beta features list */ betas: PropTypes.arrayOf(PropTypes.string), - policies: PropTypes.shape({}), - // TODO: this can maybe be removed - preferredLocale: PropTypes.string, + /** The policies which the user has access to */ + // eslint-disable-next-line react/forbid-prop-types + policies: PropTypes.object, }; const defaultProps = { chatReports: {}, - reportActions: {}, + allReportActions: {}, personalDetails: {}, priorityMode: CONST.PRIORITY_MODE.DEFAULT, betas: [], policies: [], - preferredLocale: CONST.DEFAULT_LOCALE, }; function SidebarLinksData(props) { const prevReportIDs = useRef([]); const optionListItems = useMemo(() => { - const reportIDs = SidebarUtils.getOrderedReportIDs(props.currentReportID, props.chatReports, props.betas, props.policies, props.priorityMode); + const reportIDs = SidebarUtils.getOrderedReportIDs(props.currentReportID, props.chatReports, props.betas, props.policies, props.priorityMode, props.allReportActions); if (deepEqual(prevReportIDs.current, reportIDs)) { return prevReportIDs.current; } prevReportIDs.current = reportIDs; return reportIDs; - }, [props.betas, props.chatReports, props.currentReportID, props.policies, props.priorityMode]); + }, [props.allReportActions, props.betas, props.chatReports, props.currentReportID, props.policies, props.priorityMode]); const isLoading = _.isEmpty(props.personalDetails) || _.isEmpty(props.chatReports); @@ -195,7 +194,7 @@ export default compose( betas: { key: ONYXKEYS.BETAS, }, - reportActions: { + allReportActions: { key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, selector: reportActionsSelector, }, @@ -203,9 +202,5 @@ export default compose( key: ONYXKEYS.COLLECTION.POLICY, selector: policySelector, }, - // TODO: why do we need this? - preferredLocale: { - key: ONYXKEYS.NVP_PREFERRED_LOCALE, - }, }), )(SidebarLinksData); From 1a88a51dd61b35f46238c6a5fb09edc489a71ef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 15:44:19 +0200 Subject: [PATCH 29/54] clean --- src/components/LHNOptionsList/OptionRowLHN.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 2c2d637190cf..9d851802055b 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -44,11 +44,11 @@ const propTypes = { /** Toggle between compact and default view */ viewMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), + style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), + /** The item that should be rendered */ // eslint-disable-next-line react/forbid-prop-types optionItem: PropTypes.object, - - style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), }; const defaultProps = { From b9c6c0f8d5c1f68ad790d73122308f4800430810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 16:16:28 +0200 Subject: [PATCH 30/54] wip: moving comment logic over to data component --- src/components/LHNOptionsList/OptionRowLHN.js | 15 +-------------- src/components/LHNOptionsList/OptionRowLHNData.js | 12 +++++++++++- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 9d851802055b..1fd99d3f98a2 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -1,5 +1,5 @@ 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 {deepEqual} from 'fast-equals'; @@ -21,7 +21,6 @@ import PressableWithSecondaryInteraction from '../PressableWithSecondaryInteract 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 * as Report from '../../libs/actions/Report'; import useLocalize from '../../hooks/useLocalize'; const propTypes = { @@ -29,9 +28,6 @@ const propTypes = { // 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, @@ -57,7 +53,6 @@ const defaultProps = { onSelectRow: () => {}, style: null, optionItem: null, - comment: '', isFocused: false, }; @@ -67,14 +62,6 @@ function OptionRowLHN(props) { const optionItem = props.optionItem; const [isContextMenuActive, setIsContextMenuActive] = useState(false); - 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 - }, []); - if (!optionItem) { return null; } diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index a528ef027471..efbc4ae5116d 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -2,12 +2,14 @@ import {withOnyx} from 'react-native-onyx'; import lodashGet from 'lodash/get'; import _ from 'underscore'; import PropTypes from 'prop-types'; +import React, {useEffect} from 'react'; 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'; const propTypes = { shouldDisableFocusOptions: PropTypes.bool, @@ -33,10 +35,18 @@ function OptionRowLHNData(props) { // (We don't want to fully re-render all items, just because the active report changed). const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; + useEffect(() => { + if (!props.optionItem || props.optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || isFocused) { + return; + } + Report.setReportWithDraft(props.reportID, true); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + return ( ); From f6d7bd06d9c1cdeb3f009542387cb49371870597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 10 Jul 2023 20:57:12 +0200 Subject: [PATCH 31/54] fix option row not upating when personal details changing --- src/components/LHNOptionsList/OptionRowLHN.js | 6 +- .../LHNOptionsList/OptionRowLHNData.js | 74 +++++++++++++++++-- src/libs/SidebarUtils.js | 35 ++++----- src/pages/home/sidebar/SidebarLinksData.js | 49 +++--------- 4 files changed, 93 insertions(+), 71 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHN.js b/src/components/LHNOptionsList/OptionRowLHN.js index 1fd99d3f98a2..35473bd8ddf3 100644 --- a/src/components/LHNOptionsList/OptionRowLHN.js +++ b/src/components/LHNOptionsList/OptionRowLHN.js @@ -2,7 +2,6 @@ import _ from 'underscore'; import React, {useState} from 'react'; import PropTypes from 'prop-types'; import {View, StyleSheet} from 'react-native'; -import {deepEqual} from 'fast-equals'; import * as optionRowStyles from '../../styles/optionRowStyles'; import styles from '../../styles/styles'; import * as StyleUtils from '../../styles/StyleUtils'; @@ -251,9 +250,6 @@ OptionRowLHN.propTypes = propTypes; OptionRowLHN.defaultProps = defaultProps; OptionRowLHN.displayName = 'OptionRowLHN'; -// We use deepEqual here, as the selectors results for optionItem -// aren't stable, meaning that content-wise the object is the same, -// but the reference changed. -export default React.memo(OptionRowLHN, deepEqual); +export default React.memo(OptionRowLHN); export {propTypes, defaultProps}; diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index efbc4ae5116d..57e0c3bb0d3c 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -2,7 +2,8 @@ import {withOnyx} from 'react-native-onyx'; import lodashGet from 'lodash/get'; import _ from 'underscore'; import PropTypes from 'prop-types'; -import React, {useEffect} from 'react'; +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'; @@ -10,15 +11,33 @@ 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, }; @@ -30,13 +49,23 @@ const defaultProps = { * re-render if the data really changed. */ function OptionRowLHNData(props) { - // We only want to pass a boolean to the memoized - // component, thats why we have this intermediate component. - // (We don't want to fully re-render all items, just because the active report changed). + // We only want to pass a boolean to the memoized component, + // instead of a changing number (so we prevent unnecessary re-renders). const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; + const optionItemRef = useRef(); + const optionItem = useMemo(() => { + // Note: ideally we'd have this as a dependent selector in onyx! + const item = SidebarUtils.getOptionData(props.fullReport, props.personalDetails, props.preferredLocale); + if (deepEqual(item, optionItemRef.current)) { + return optionItemRef.current; + } + optionItemRef.current = item; + return item; + }, [props.fullReport, props.preferredLocale, props.personalDetails]); + useEffect(() => { - if (!props.optionItem || props.optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || isFocused) { + if (!optionItem || optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || isFocused) { return; } Report.setReportWithDraft(props.reportID, true); @@ -46,8 +75,9 @@ function OptionRowLHNData(props) { return ( ); } @@ -56,6 +86,28 @@ 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; + }, + {}, + ); + export default compose( withCurrentReportID, withReportCommentDrafts({ @@ -66,9 +118,15 @@ export default compose( }, }), withOnyx({ - optionItem: { + fullReport: { key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, - selector: SidebarUtils.getOptionData, + }, + personalDetails: { + key: ONYXKEYS.PERSONAL_DETAILS_LIST, + selector: personalDetailsSelector, + }, + preferredLocale: { + key: ONYXKEYS.NVP_PREFERRED_LOCALE, }, }), )(OptionRowLHNData); diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index 89a34043d6e4..ed38a01aedd1 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -13,12 +13,16 @@ import * as CollectionUtils from './CollectionUtils'; import * as LocalePhoneNumber from './LocalePhoneNumber'; import * as UserUtils from './UserUtils'; -// 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 to 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 SidebarLunks 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({ @@ -27,12 +31,6 @@ Onyx.connect({ callback: (val) => (allReports = val), }); -let personalDetails; -Onyx.connect({ - key: ONYXKEYS.PERSONAL_DETAILS_LIST, - callback: (val) => (personalDetails = val), -}); - const visibleReportActionItems = {}; const lastReportActions = {}; const reportActions = {}; @@ -59,6 +57,9 @@ Onyx.connect({ }, }); +// 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, @@ -71,12 +72,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) => { @@ -202,9 +197,11 @@ function getOrderedReportIDs(currentReportId, allReportsDict, betas, policies, p * Gets all the data necessary for rendering an OptionRowLHN component * * @param {Object} report + * @param {Object} personalDetails + * @param {String} preferredLocale * @returns {Object} */ -function getOptionData(report) { +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/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index da5d51a613b1..701299aeaf88 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -10,9 +10,7 @@ import withCurrentReportID from '../../../components/withCurrentReportID'; import compose from '../../../libs/compose'; import ONYXKEYS from '../../../ONYXKEYS'; import reportPropTypes from '../../reportPropTypes'; -import participantPropTypes from '../../../components/participantPropTypes'; import CONST from '../../../CONST'; -import * as UserUtils from '../../../libs/UserUtils'; const propTypes = { ...basePropTypes, @@ -39,8 +37,8 @@ const propTypes = { ), ), - /** List of users' personal details */ - personalDetails: PropTypes.objectOf(participantPropTypes), + /** 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, @@ -56,24 +54,24 @@ const propTypes = { const defaultProps = { chatReports: {}, allReportActions: {}, - personalDetails: {}, priorityMode: CONST.PRIORITY_MODE.DEFAULT, + isPersonalDetailsLoading: true, betas: [], policies: [], }; function SidebarLinksData(props) { - const prevReportIDs = useRef([]); + const reportIDsRef = useRef([]); const optionListItems = useMemo(() => { const reportIDs = SidebarUtils.getOrderedReportIDs(props.currentReportID, props.chatReports, props.betas, props.policies, props.priorityMode, props.allReportActions); - if (deepEqual(prevReportIDs.current, reportIDs)) { - return prevReportIDs.current; + if (deepEqual(reportIDsRef.current, reportIDs)) { + return reportIDsRef.current; } - prevReportIDs.current = reportIDs; + reportIDsRef.current = reportIDs; return reportIDs; }, [props.allReportActions, props.betas, props.chatReports, props.currentReportID, props.policies, props.priorityMode]); - const isLoading = _.isEmpty(props.personalDetails) || _.isEmpty(props.chatReports); + const isLoading = _.isEmpty(props.chatReports) || props.isPersonalDetailsLoading; return ( lastReadTime: report.lastReadTime, }; -/** - * @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} @@ -175,18 +151,13 @@ const policySelector = (policy) => export default compose( withCurrentReportID, 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: { + isPersonalDetailsLoading: { key: ONYXKEYS.PERSONAL_DETAILS_LIST, - selector: personalDetailsSelector, + selector: _.isEmpty, }, priorityMode: { key: ONYXKEYS.NVP_PRIORITY_MODE, From 3b5bd35f9f4676baa08d75040c595f354024fe74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Tue, 11 Jul 2023 16:20:27 +0200 Subject: [PATCH 32/54] Update src/libs/SidebarUtils.js Co-authored-by: Wojciech Lewicki --- src/libs/SidebarUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index ed38a01aedd1..393263ab2609 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -15,7 +15,7 @@ import * as UserUtils from './UserUtils'; // Note: Earlier SidebarUtils.getOrderedReportIDs() used to have to 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 SidebarLunks to run SidebarUtils.getOrderedReportIDs() again. +// 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. From c53cf6d6c23b1b3a2b8ef9150e77493d5cf1ac0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:10:26 +0200 Subject: [PATCH 33/54] fix issues after merge --- src/components/LHNOptionsList/LHNOptionsList.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index 343e198b5401..43bed27cfe12 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -31,8 +31,6 @@ const defaultProps = { }; function LHNOptionsList(props) { - const data = useMemo(() => props.data, [props.data]); - /** * 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 @@ -60,7 +58,7 @@ function LHNOptionsList(props) { * * @return {Component} */ - const renderItem = ({item, index}) => ( + const renderItem = ({item}) => ( item} stickySectionHeadersEnabled={false} renderItem={renderItem} From 00a4b5c22f6cde555487aeb82dd78651b89335dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:14:51 +0200 Subject: [PATCH 34/54] dont use this --- src/components/LHNOptionsList/LHNOptionsList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index 43bed27cfe12..6e14ef470d13 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -62,7 +62,7 @@ function LHNOptionsList(props) { ); From 26e661a32a18b9b34a6489b604feec6af053321a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:15:10 +0200 Subject: [PATCH 35/54] removed unused imports --- src/components/LHNOptionsList/LHNOptionsList.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index 6e14ef470d13..e8fca9044c88 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -1,12 +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 */ From 448878df5320f6f41bbf466c0987249c25a760ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:15:59 +0200 Subject: [PATCH 36/54] fix comment --- src/libs/SidebarUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/SidebarUtils.js b/src/libs/SidebarUtils.js index fe8975a18dfd..0925560d710b 100644 --- a/src/libs/SidebarUtils.js +++ b/src/libs/SidebarUtils.js @@ -14,7 +14,7 @@ import * as LocalePhoneNumber from './LocalePhoneNumber'; import * as UserUtils from './UserUtils'; import * as PersonalDetailsUtils from './PersonalDetailsUtils'; -// Note: Earlier SidebarUtils.getOrderedReportIDs() used to have to parameters. All the needed data was loaded here directly +// 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. From 7bc90eb3ef4b11fc033c18321198a838611b7a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:17:41 +0200 Subject: [PATCH 37/54] commented tests --- tests/unit/SidebarFilterTest.js | 3 ++- tests/unit/SidebarOrderTest.js | 3 ++- tests/unit/SidebarTest.js | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit/SidebarFilterTest.js b/tests/unit/SidebarFilterTest.js index 7b775fc6987c..eeeb180f9fde 100644 --- a/tests/unit/SidebarFilterTest.js +++ b/tests/unit/SidebarFilterTest.js @@ -32,9 +32,10 @@ describe('Sidebar', () => { }), ); - // Initialize the network key for OfflineWithFeedback beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); diff --git a/tests/unit/SidebarOrderTest.js b/tests/unit/SidebarOrderTest.js index 86f193734fc5..7c0e12e6b1bc 100644 --- a/tests/unit/SidebarOrderTest.js +++ b/tests/unit/SidebarOrderTest.js @@ -32,9 +32,10 @@ describe('Sidebar', () => { }), ); - // Initialize the network key for OfflineWithFeedback beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); diff --git a/tests/unit/SidebarTest.js b/tests/unit/SidebarTest.js index a318833fca4a..02585d273fb7 100644 --- a/tests/unit/SidebarTest.js +++ b/tests/unit/SidebarTest.js @@ -31,9 +31,10 @@ describe('Sidebar', () => { }), ); - // Initialize the network key for OfflineWithFeedback beforeEach(() => { + // Wrap Onyx each onyx action with waitForPromiseToResolve wrapOnyxWithWaitForPromisesToResolve(Onyx); + // Initialize the network key for OfflineWithFeedback return Onyx.merge(ONYXKEYS.NETWORK, {isOffline: false}); }); From 336d5fd44c00f5cae486df430bfe8df2b57981e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Fri, 14 Jul 2023 16:19:00 +0200 Subject: [PATCH 38/54] inline skeleton --- src/pages/home/sidebar/SidebarLinks.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 07cc95544905..e38792c080dd 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -162,8 +162,6 @@ class SidebarLinks extends React.PureComponent { } render() { - const skeletonPlaceholder = ; - return ( {this.props.isLoading ? ( - skeletonPlaceholder + ) : ( Date: Mon, 17 Jul 2023 09:33:05 +0200 Subject: [PATCH 39/54] destructure props --- .../LHNOptionsList/LHNOptionsList.js | 14 +++++++------- src/pages/home/sidebar/SidebarLinksData.js | 18 +++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/components/LHNOptionsList/LHNOptionsList.js b/src/components/LHNOptionsList/LHNOptionsList.js index e8fca9044c88..13de5a5e8c52 100644 --- a/src/components/LHNOptionsList/LHNOptionsList.js +++ b/src/components/LHNOptionsList/LHNOptionsList.js @@ -29,7 +29,7 @@ const defaultProps = { shouldDisableFocusOptions: false, }; -function LHNOptionsList(props) { +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 @@ -41,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, @@ -60,9 +60,9 @@ function LHNOptionsList(props) { const renderItem = ({item}) => ( ); @@ -71,9 +71,9 @@ function LHNOptionsList(props) { item} stickySectionHeadersEnabled={false} renderItem={renderItem} diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 701299aeaf88..974a18c1ffb1 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -60,27 +60,27 @@ const defaultProps = { policies: [], }; -function SidebarLinksData(props) { +function SidebarLinksData({allReportActions, betas, chatReports, currentReportID, insets, isPersonalDetailsLoading, isSmallScreenWidth, onLayout, onLinkClick, policies, priorityMode}) { const reportIDsRef = useRef([]); const optionListItems = useMemo(() => { - const reportIDs = SidebarUtils.getOrderedReportIDs(props.currentReportID, props.chatReports, props.betas, props.policies, props.priorityMode, props.allReportActions); + const reportIDs = SidebarUtils.getOrderedReportIDs(currentReportID, chatReports, betas, policies, priorityMode, allReportActions); if (deepEqual(reportIDsRef.current, reportIDs)) { return reportIDsRef.current; } reportIDsRef.current = reportIDs; return reportIDs; - }, [props.allReportActions, props.betas, props.chatReports, props.currentReportID, props.policies, props.priorityMode]); + }, [allReportActions, betas, chatReports, currentReportID, policies, priorityMode]); - const isLoading = _.isEmpty(props.chatReports) || props.isPersonalDetailsLoading; + const isLoading = _.isEmpty(chatReports) || isPersonalDetailsLoading; return ( Date: Mon, 17 Jul 2023 09:39:52 +0200 Subject: [PATCH 40/54] fix highlight active chat --- src/components/LHNOptionsList/OptionRowLHNData.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index 57e0c3bb0d3c..bdd11ba72017 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -51,7 +51,7 @@ const defaultProps = { function OptionRowLHNData(props) { // We only want to pass a boolean to the memoized component, // instead of a changing number (so we prevent unnecessary re-renders). - const isFocused = !props.shouldDisableFocusOptions && props.currentReportId === props.reportID; + const isFocused = !props.shouldDisableFocusOptions && props.currentReportID === props.reportID; const optionItemRef = useRef(); const optionItem = useMemo(() => { From c4fed2bd3cc795435d5b7a5073b8b59a5c5fddf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:29:09 +0200 Subject: [PATCH 41/54] add displayName and iouReportAmount for sorting --- src/pages/home/sidebar/SidebarLinksData.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 974a18c1ffb1..b3c6b6e7bff7 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -63,6 +63,10 @@ const defaultProps = { function SidebarLinksData({allReportActions, betas, chatReports, currentReportID, insets, isPersonalDetailsLoading, isSmallScreenWidth, onLayout, onLinkClick, policies, priorityMode}) { const reportIDsRef = useRef([]); const optionListItems = useMemo(() => { + // Object.values(chatReports).forEach((report) => { + // console.log('[HD]', report.reportName, report.reportID, report.lastVisibleActionCreated); + // }); + const reportIDs = SidebarUtils.getOrderedReportIDs(currentReportID, chatReports, betas, policies, priorityMode, allReportActions); if (deepEqual(reportIDsRef.current, reportIDs)) { return reportIDsRef.current; @@ -111,6 +115,7 @@ const chatReportSelector = (report) => lastMessageText: report.lastMessageText, lastVisibleActionCreated: report.lastVisibleActionCreated, iouReportID: report.iouReportID, + iouReportAmount: report.iouReportAmount, hasOutstandingIOU: report.hasOutstandingIOU, statusNum: report.statusNum, stateNum: report.stateNum, @@ -120,6 +125,7 @@ const chatReportSelector = (report) => reportName: report.reportName, visibility: report.visibility, lastReadTime: report.lastReadTime, + displayName: report.displayName, }; /** From 2a735400990703721780336d7d06dfabc1679535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:32:28 +0200 Subject: [PATCH 42/54] include total, remove displayName these were computed properties, iouReportAmount is computed, same as displayname --- src/pages/home/sidebar/SidebarLinksData.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index b3c6b6e7bff7..f26844eb64bd 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -115,7 +115,7 @@ const chatReportSelector = (report) => lastMessageText: report.lastMessageText, lastVisibleActionCreated: report.lastVisibleActionCreated, iouReportID: report.iouReportID, - iouReportAmount: report.iouReportAmount, + total: report.total, hasOutstandingIOU: report.hasOutstandingIOU, statusNum: report.statusNum, stateNum: report.stateNum, @@ -125,7 +125,6 @@ const chatReportSelector = (report) => reportName: report.reportName, visibility: report.visibility, lastReadTime: report.lastReadTime, - displayName: report.displayName, }; /** From de7209f68d10510d74f4efa3ad7bfc647557f888 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 11:38:43 +0200 Subject: [PATCH 43/54] added other properties considered for sorting --- src/pages/home/sidebar/SidebarLinksData.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index f26844eb64bd..0812dc99b991 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -122,9 +122,16 @@ const chatReportSelector = (report) => chatType: report.chatType, type: report.type, policyID: report.policyID, - reportName: report.reportName, visibility: report.visibility, lastReadTime: report.lastReadTime, + // needed for name sorting: + reportName: report.reportName, + policyName: report.policyName, + oldPolicyName: report.oldPolicyName, + // other properites considered for sorting: + ownerAccountID: report.ownerAccountID, + currency: report.currency, + managerID: report.managerID, }; /** From de5fe1d91d8cc12e98d382f5413f72bf64db27aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 13:46:10 +0200 Subject: [PATCH 44/54] use prop spreaing --- src/components/LHNOptionsList/OptionRowLHNData.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index bdd11ba72017..a0cff1e1355d 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -48,34 +48,35 @@ const defaultProps = { * The OptionRowLHN component is memoized, so it will only * re-render if the data really changed. */ -function OptionRowLHNData(props) { +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 = !props.shouldDisableFocusOptions && props.currentReportID === props.reportID; + 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(props.fullReport, props.personalDetails, props.preferredLocale); + const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale); if (deepEqual(item, optionItemRef.current)) { return optionItemRef.current; } optionItemRef.current = item; return item; - }, [props.fullReport, props.preferredLocale, props.personalDetails]); + }, [fullReport, preferredLocale, personalDetails]); useEffect(() => { - if (!optionItem || optionItem.hasDraftComment || !props.comment || props.comment.length <= 0 || isFocused) { + if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) { return; } - Report.setReportWithDraft(props.reportID, true); + Report.setReportWithDraft(reportID, true); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); return ( From ecf4c0a5971647b7ec78c2e9a4ee0f5cdf244a28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 13:54:19 +0200 Subject: [PATCH 45/54] move navigation focus to sidebarlinksdata --- src/pages/home/sidebar/SidebarLinks.js | 9 +--- src/pages/home/sidebar/SidebarLinksData.js | 50 ++++++++++++++++------ 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index e38792c080dd..142418867fe9 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -23,7 +23,6 @@ import withWindowDimensions from '../../../components/withWindowDimensions'; import LHNOptionsList from '../../../components/LHNOptionsList/LHNOptionsList'; import SidebarUtils from '../../../libs/SidebarUtils'; import OfflineWithFeedback from '../../../components/OfflineWithFeedback'; -import withNavigationFocus from '../../../components/withNavigationFocus'; import withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation'; import Header from '../../../components/Header'; import defaultTheme from '../../../styles/themes/default'; @@ -163,11 +162,7 @@ class SidebarLinks extends React.PureComponent { render() { return ( - + { // Object.values(chatReports).forEach((report) => { @@ -78,17 +97,23 @@ function SidebarLinksData({allReportActions, betas, chatReports, currentReportID const isLoading = _.isEmpty(chatReports) || isPersonalDetailsLoading; return ( - + + + ); } @@ -162,6 +187,7 @@ const policySelector = (policy) => export default compose( withCurrentReportID, + withNavigationFocus, withOnyx({ chatReports: { key: ONYXKEYS.COLLECTION.REPORT, From b76a569ad8305955f87c6ebcc2f1b201e213d785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 13:56:20 +0200 Subject: [PATCH 46/54] removed debug comment --- src/pages/home/sidebar/SidebarLinksData.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index 7cb6627471c3..cad5d4ff3820 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -82,10 +82,6 @@ function SidebarLinksData({ const reportIDsRef = useRef([]); const optionListItems = useMemo(() => { - // Object.values(chatReports).forEach((report) => { - // console.log('[HD]', report.reportName, report.reportID, report.lastVisibleActionCreated); - // }); - const reportIDs = SidebarUtils.getOrderedReportIDs(currentReportID, chatReports, betas, policies, priorityMode, allReportActions); if (deepEqual(reportIDsRef.current, reportIDs)) { return reportIDsRef.current; From c0fba28cbd10fc919569fed39b595c130a743d61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 18:59:10 +0200 Subject: [PATCH 47/54] remove withNavigation usage --- src/pages/home/sidebar/SidebarLinks.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 142418867fe9..3d677d49852f 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -23,7 +23,6 @@ import withWindowDimensions from '../../../components/withWindowDimensions'; import LHNOptionsList from '../../../components/LHNOptionsList/LHNOptionsList'; import SidebarUtils from '../../../libs/SidebarUtils'; import OfflineWithFeedback from '../../../components/OfflineWithFeedback'; -import withNavigation, {withNavigationPropTypes} from '../../../components/withNavigation'; import Header from '../../../components/Header'; import defaultTheme from '../../../styles/themes/default'; import OptionsListSkeletonView from '../../../components/OptionsListSkeletonView'; @@ -62,7 +61,6 @@ const propTypes = { priorityMode: PropTypes.oneOf(_.values(CONST.OPTION_MODE)), ...withLocalizePropTypes, - ...withNavigationPropTypes, }; const defaultProps = { @@ -231,5 +229,5 @@ class SidebarLinks extends React.PureComponent { SidebarLinks.propTypes = propTypes; SidebarLinks.defaultProps = defaultProps; -export default compose(withLocalize, withCurrentUserPersonalDetails, withWindowDimensions, withNavigation)(SidebarLinks); +export default compose(withLocalize, withCurrentUserPersonalDetails, withWindowDimensions)(SidebarLinks); export {basePropTypes}; From 31a1167a42d9066c9f03019adc284305b3e428be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Mon, 17 Jul 2023 19:03:40 +0200 Subject: [PATCH 48/54] add other missing fields --- src/pages/home/sidebar/SidebarLinksData.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinksData.js b/src/pages/home/sidebar/SidebarLinksData.js index cad5d4ff3820..6bf45aa3d4fd 100644 --- a/src/pages/home/sidebar/SidebarLinksData.js +++ b/src/pages/home/sidebar/SidebarLinksData.js @@ -145,14 +145,17 @@ const chatReportSelector = (report) => policyID: report.policyID, visibility: report.visibility, lastReadTime: report.lastReadTime, - // needed for name sorting: + // Needed for name sorting: reportName: report.reportName, policyName: report.policyName, oldPolicyName: report.oldPolicyName, - // other properites considered for sorting: + // 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, }; /** From 478288dc7cc5a777e5f7b8437e63b981980df21b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 13:12:44 +0200 Subject: [PATCH 49/54] removed unused onLayout prop --- src/pages/home/sidebar/SidebarLinks.js | 6 +++--- src/pages/home/sidebar/SidebarLinksData.js | 16 +--------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 6a217503f708..407f65297646 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -45,8 +45,6 @@ const basePropTypes = { /** Whether we are viewing below the responsive breakpoint */ isSmallScreenWidth: PropTypes.bool.isRequired, - - onLayout: PropTypes.func, }; const propTypes = { @@ -78,6 +76,8 @@ class SidebarLinks extends React.PureComponent { this.showSettingsPage = this.showSettingsPage.bind(this); this.showReportPage = this.showReportPage.bind(this); + this.styles = [styles.sidebarListContainer, {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom}]; + if (this.props.isSmallScreenWidth) { App.confirmReadyToOpenApp(); } @@ -215,7 +215,7 @@ class SidebarLinks extends React.PureComponent { ) : ( Date: Wed, 19 Jul 2023 13:22:48 +0200 Subject: [PATCH 50/54] Memoize OptionRowLHNData as well --- .../LHNOptionsList/OptionRowLHNData.js | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index a0cff1e1355d..32569944f645 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -4,6 +4,7 @@ import _ from 'underscore'; import PropTypes from 'prop-types'; import React, {useEffect, useRef, useMemo} from 'react'; import {deepEqual} from 'fast-equals'; +import {diff} from 'jest-diff'; import {withReportCommentDrafts} from '../OnyxProvider'; import SidebarUtils from '../../libs/SidebarUtils'; import compose from '../../libs/compose'; @@ -109,25 +110,32 @@ const personalDetailsSelector = (personalDetails) => {}, ); -export default 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); +// 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), +); From 9f883715948ff90b5f83811604b841f04e8056cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 13:27:39 +0200 Subject: [PATCH 51/54] fix lint --- src/components/LHNOptionsList/OptionRowLHNData.js | 1 - src/pages/home/sidebar/SidebarLinks.js | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index 32569944f645..df2b5a13dbf1 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -4,7 +4,6 @@ import _ from 'underscore'; import PropTypes from 'prop-types'; import React, {useEffect, useRef, useMemo} from 'react'; import {deepEqual} from 'fast-equals'; -import {diff} from 'jest-diff'; import {withReportCommentDrafts} from '../OnyxProvider'; import SidebarUtils from '../../libs/SidebarUtils'; import compose from '../../libs/compose'; diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js index 407f65297646..15bde9120b46 100644 --- a/src/pages/home/sidebar/SidebarLinks.js +++ b/src/pages/home/sidebar/SidebarLinks.js @@ -76,8 +76,6 @@ class SidebarLinks extends React.PureComponent { this.showSettingsPage = this.showSettingsPage.bind(this); this.showReportPage = this.showReportPage.bind(this); - this.styles = [styles.sidebarListContainer, {paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom}]; - if (this.props.isSmallScreenWidth) { App.confirmReadyToOpenApp(); } @@ -215,7 +213,7 @@ class SidebarLinks extends React.PureComponent { ) : ( Date: Wed, 19 Jul 2023 18:41:07 +0200 Subject: [PATCH 52/54] Update src/components/LHNOptionsList/OptionRowLHNData.js Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- src/components/LHNOptionsList/OptionRowLHNData.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index df2b5a13dbf1..f0f6ce7b04a1 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -110,7 +110,7 @@ const personalDetailsSelector = (personalDetails) => ); // This component is rendered in a list. -// On scroll we want to avoid that a item re-renders +// On scroll we want to avoid 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. From 17ed2a175c5baf34b3ce40f6e4fc2916d8b4be23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 18:41:20 +0200 Subject: [PATCH 53/54] Update src/libs/ReportActionsUtils.js Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com> --- src/libs/ReportActionsUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/ReportActionsUtils.js b/src/libs/ReportActionsUtils.js index 7450940c02e2..580b93d116a7 100644 --- a/src/libs/ReportActionsUtils.js +++ b/src/libs/ReportActionsUtils.js @@ -100,7 +100,7 @@ function hasCommentThread(reportAction) { * @param {Object} [allReportActionsParam] * @returns {Object} */ -function getParentReportAction(report, allReportActionsParam) { +function getParentReportAction(report, allReportActionsParam = {}) { if (!report || !report.parentReportID || !report.parentReportActionID) { return {}; } From 9ba0811674b2fef63bf3eaf709ae7910931bc743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hanno=20J=2E=20G=C3=B6decke?= Date: Wed, 19 Jul 2023 18:42:39 +0200 Subject: [PATCH 54/54] change comment format --- src/components/LHNOptionsList/OptionRowLHNData.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/LHNOptionsList/OptionRowLHNData.js b/src/components/LHNOptionsList/OptionRowLHNData.js index f0f6ce7b04a1..05e9d2e06cc6 100644 --- a/src/components/LHNOptionsList/OptionRowLHNData.js +++ b/src/components/LHNOptionsList/OptionRowLHNData.js @@ -109,11 +109,13 @@ const personalDetailsSelector = (personalDetails) => {}, ); -// This component is rendered in a list. -// On scroll we want to avoid 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. +/** + * 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,