Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed LHN Ordering #35907

Merged
merged 26 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9b040c5
Test LHN fix
shubham1206agra Feb 6, 2024
6333dcd
revert unintentional change
shubham1206agra Feb 6, 2024
87826ab
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 17, 2024
06f5775
Optimization attempt 1
shubham1206agra Feb 17, 2024
f556253
Fixed perf test
shubham1206agra Feb 17, 2024
f8ab7cc
Fixed display name filter
shubham1206agra Feb 17, 2024
90343b9
Added violation for RBR in sorting
shubham1206agra Feb 18, 2024
6b19fd2
Optimization attempt 2
shubham1206agra Feb 19, 2024
c61adb1
Revert "Optimization attempt 2"
shubham1206agra Feb 19, 2024
cf3147f
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Feb 19, 2024
6649bb9
Minor fixes
shubham1206agra Feb 19, 2024
40e7a05
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 21, 2024
89a56b2
Optimization Attempt 3
shubham1206agra Feb 21, 2024
102bbda
Fixed perf test
shubham1206agra Feb 21, 2024
1eca3e1
code adjustment according to recommendations
shubham1206agra Feb 21, 2024
1314854
Merge branch 'main' into test-lhn-fix
shubham1206agra Feb 24, 2024
4e35908
Fixed lint
shubham1206agra Feb 24, 2024
e15075f
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Feb 27, 2024
651e372
Revert "Merge branch 'Expensify:main' into test-lhn-fix"
shubham1206agra Feb 27, 2024
70ef3fc
fixing name display
shubham1206agra Feb 27, 2024
91cea78
Revert "Revert "Merge branch 'Expensify:main' into test-lhn-fix""
shubham1206agra Feb 27, 2024
e2f769a
adding small comment
shubham1206agra Feb 27, 2024
8495e60
Rename variable
shubham1206agra Feb 28, 2024
29216c2
Fixed violation to be used properly for all permissions users
shubham1206agra Feb 28, 2024
888195b
Merge branch 'Expensify:main' into test-lhn-fix
shubham1206agra Mar 4, 2024
f14090e
Added transaction as dependency
shubham1206agra Mar 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,8 @@ const CONST = {
ALPHABETIC_AND_LATIN_CHARS: /^[\p{Script=Latin} ]*$/u,
NON_ALPHABETIC_AND_NON_LATIN_CHARS: /[^\p{Script=Latin}]/gu,
ACCENT_LATIN_CHARS: /[\u00C0-\u017F]/g,
INVALID_DISPLAY_NAME_LHN: /[^\p{L}\p{N}\u00C0-\u017F\s-]/gu,
INVALID_DISPLAY_NAME_ONLY_LHN: /^[^\p{L}\p{N}\u00C0-\u017F\s-]$/gu,
POSITIVE_INTEGER: /^\d+$/,
PO_BOX: /\b[P|p]?(OST|ost)?\.?\s*[O|o|0]?(ffice|FFICE)?\.?\s*[B|b][O|o|0]?[X|x]?\.?\s+[#]?(\d+)\b/,
ANY_VALUE: /^.+$/,
Expand Down
4 changes: 4 additions & 0 deletions src/components/LHNOptionsList/LHNOptionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ function LHNOptionsList({
draftComments = {},
transactionViolations = {},
onFirstItemRendered = () => {},
reportsWithErrorsIds = {},
}: LHNOptionsListProps) {
const styles = useThemeStyles();
const {canUseViolations} = usePermissions();
Expand Down Expand Up @@ -63,6 +64,7 @@ function LHNOptionsList({
const itemComment = draftComments?.[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] ?? '';
const sortedReportActions = ReportActionsUtils.getSortedReportActionsForDisplay(itemReportActions);
const lastReportAction = sortedReportActions[0];
const reportErrors = reportsWithErrorsIds[reportID] ?? {};
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved

// Get the transaction for the last report action
let lastReportActionTransactionID = '';
Expand Down Expand Up @@ -91,6 +93,7 @@ function LHNOptionsList({
transactionViolations={transactionViolations}
canUseViolations={canUseViolations}
onLayout={onLayoutItem}
reportErrors={reportErrors}
/>
);
},
Expand All @@ -109,6 +112,7 @@ function LHNOptionsList({
transactionViolations,
canUseViolations,
onLayoutItem,
reportsWithErrorsIds,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
],
);

Expand Down
3 changes: 2 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHNData.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function OptionRowLHNData({
lastReportActionTransaction = {},
transactionViolations,
canUseViolations,
reportErrors,
...propsToForward
}: OptionRowLHNDataProps) {
const reportID = propsToForward.reportID;
Expand All @@ -40,11 +41,11 @@ function OptionRowLHNData({
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData({
report: fullReport,
reportActions,
personalDetails,
preferredLocale: preferredLocale ?? CONST.LOCALES.DEFAULT,
policy,
parentReportAction,
reportErrors,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not added to the dependencies of useMemo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reportErrors seems to be an object, if we add it as dependency we should memoize it also on the parent component

parent:

const reportErrors = useMemo(() => reportIDsWithErrors[reportID] ?? {}, [reportID]);

and not sure about reportIDsWithErrors
cc: @youssef-lr

hasViolations: !!hasViolations,
});
if (deepEqual(item, optionItemRef.current)) {
Expand Down
6 changes: 6 additions & 0 deletions src/components/LHNOptionsList/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {CurrentReportIDContextValue} from '@components/withCurrentReportID'
import type CONST from '@src/CONST';
import type {OptionData} from '@src/libs/ReportUtils';
import type {Locale, PersonalDetailsList, Policy, Report, ReportAction, ReportActions, Transaction, TransactionViolation} from '@src/types/onyx';
import type * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import type {EmptyObject} from '@src/types/utils/EmptyObject';

type OptionMode = ValueOf<typeof CONST.OPTION_MODE>;
Expand Down Expand Up @@ -58,6 +59,8 @@ type CustomLHNOptionsListProps = {

/** Callback to fire when the list is laid out */
onFirstItemRendered: () => void;

reportsWithErrorsIds: Record<string, OnyxCommon.Errors>;
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
};

type LHNOptionsListProps = CustomLHNOptionsListProps & CurrentReportIDContextValue & LHNOptionsListOnyxProps;
Expand Down Expand Up @@ -113,6 +116,9 @@ type OptionRowLHNDataProps = {

/** Callback to execute when the OptionList lays out */
onLayout?: (event: LayoutChangeEvent) => void;

/** The report errors */
reportErrors: OnyxCommon.Errors | undefined;
};

type OptionRowLHNProps = {
Expand Down
26 changes: 21 additions & 5 deletions src/libs/SidebarUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, PersonalDetailsList, TransactionViolation} from '@src/types/onyx';
import type Beta from '@src/types/onyx/Beta';
import type * as OnyxCommon from '@src/types/onyx/OnyxCommon';
import type Policy from '@src/types/onyx/Policy';
import type Report from '@src/types/onyx/Report';
import type {ReportActions} from '@src/types/onyx/ReportAction';
Expand Down Expand Up @@ -74,6 +75,13 @@ function setWithLimit<TKey, TValue>(map: Map<TKey, TValue>, key: TKey, value: TV
// Variable to verify if ONYX actions are loaded
let hasInitialReportActions = false;

function filterDisplayName(displayName: string): string {
if (CONST.REGEX.INVALID_DISPLAY_NAME_ONLY_LHN.test(displayName)) {
return displayName;
}
return displayName.replace(CONST.REGEX.INVALID_DISPLAY_NAME_LHN, '');
}

/**
* @returns An array of reportIDs sorted in the proper order
*/
Expand All @@ -87,6 +95,7 @@ function getOrderedReportIDs(
transactionViolations: OnyxCollection<TransactionViolation[]>,
currentPolicyID = '',
policyMemberAccountIDs: number[] = [],
reportsWithErrorsIds: Record<string, OnyxCommon.Errors> = {},
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
): string[] {
const currentReportActions = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${currentReportId}`];
let reportActionCount = currentReportActions?.length ?? 0;
Expand All @@ -112,13 +121,18 @@ function getOrderedReportIDs(
const isInDefaultMode = !isInGSDMode;
const allReportsDictValues = Object.values(allReports);

const reportIDsWithViolations: Record<string, boolean> = {};
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved

// Filter out all the reports that shouldn't be displayed
let reportsToDisplay = allReportsDictValues.filter((report) => {
const parentReportActionsKey = `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report?.parentReportID}`;
const parentReportActions = allReportActions?.[parentReportActionsKey];
const parentReportAction = parentReportActions?.find((action) => action && report && action?.reportActionID === report?.parentReportActionID);
const doesReportHaveViolations =
betas.includes(CONST.BETAS.VIOLATIONS) && !!parentReportAction && ReportUtils.doesTransactionThreadHaveViolations(report, transactionViolations, parentReportAction);
if (doesReportHaveViolations) {
reportIDsWithViolations[report.reportID] = true;
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
}
return ReportUtils.shouldReportBeInOptionList({
report,
currentReportId: currentReportId ?? '',
Expand Down Expand Up @@ -161,11 +175,13 @@ function getOrderedReportIDs(
// However, this code needs to be very performant to handle thousands of reports, so in the interest of speed, we're just going to disable this lint rule and add
// the reportDisplayName property to the report object directly.
// eslint-disable-next-line no-param-reassign
report.displayName = ReportUtils.getReportName(report);
report.displayName = filterDisplayName(ReportUtils.getReportName(report));

shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
const hasRBR = report.reportID in reportsWithErrorsIds || report.reportID in reportIDsWithViolations;
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved

const isPinned = report.isPinned ?? false;
const reportAction = ReportActionsUtils.getReportAction(report.parentReportID ?? '', report.parentReportActionID ?? '');
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
if (isPinned || hasRBR || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
pinnedAndGBRReports.push(report);
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
} else if (report.hasDraft) {
draftReports.push(report);
Expand Down Expand Up @@ -205,19 +221,19 @@ function getOrderedReportIDs(
*/
function getOptionData({
report,
reportActions,
personalDetails,
preferredLocale,
policy,
parentReportAction,
reportErrors,
hasViolations,
}: {
report: OnyxEntry<Report>;
reportActions: OnyxEntry<ReportActions>;
personalDetails: OnyxEntry<PersonalDetailsList>;
preferredLocale: DeepValueOf<typeof CONST.LOCALES>;
policy: OnyxEntry<Policy> | undefined;
parentReportAction: OnyxEntry<ReportAction> | undefined;
reportErrors: OnyxCommon.Errors | undefined;
hasViolations: boolean;
}): ReportUtils.OptionData | undefined {
// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
Expand All @@ -230,7 +246,7 @@ function getOptionData({
const result: ReportUtils.OptionData = {
text: '',
alternateText: null,
allReportErrors: OptionsListUtils.getAllReportErrors(report, reportActions),
allReportErrors: reportErrors,
brickRoadIndicator: null,
tooltipText: null,
subtitle: null,
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/sidebar/SidebarLinks.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const propTypes = {
isActiveReport: PropTypes.func.isRequired,
};

function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport, isCreateMenuOpen, activePolicy}) {
function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priorityMode = CONST.PRIORITY_MODE.DEFAULT, isActiveReport, isCreateMenuOpen, activePolicy, reportsWithErrorsIds}) {
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const modal = useRef({});
Expand Down Expand Up @@ -157,6 +157,7 @@ function SidebarLinks({onLinkClick, insets, optionListItems, isLoading, priority
shouldDisableFocusOptions={isSmallScreenWidth}
optionMode={viewMode}
onFirstItemRendered={App.setSidebarLoaded}
reportsWithErrorsIds={reportsWithErrorsIds}
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
/>
{isLoading && optionListItems.length === 0 && (
<View style={[StyleSheet.absoluteFillObject, styles.appBG]}>
Expand Down
57 changes: 54 additions & 3 deletions src/pages/home/sidebar/SidebarLinksData.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import compose from '@libs/compose';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import {getPolicyMembersByIdWithoutCurrentUser} from '@libs/PolicyUtils';
import * as ReportUtils from '@libs/ReportUtils';
import SidebarUtils from '@libs/SidebarUtils';
Expand Down Expand Up @@ -139,6 +140,23 @@ function SidebarLinksData({
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => Policy.openWorkspace(activeWorkspaceID, policyMemberAccountIDs), [activeWorkspaceID]);

const reportsWithErrorsIds = useMemo(() => {
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
const reportKeys = _.keys(allReportActions);
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
return _.reduce(
Copy link
Contributor

@gedu gedu Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a though, having 2 reduces, one nested the other, won't make this function O(n*m).
can we create first the all reports.

const allReportsActions = {};
reportKeys.forEach((reportKey) => {
              const allReportsActionsMock = _.reduce(allReportActions[reportKey.replace('report_', 'reportActions_')], (acc, reportAction) => ({...acc, [reportAction.reportActionID]: reportAction}), {});
              allReportsActions = {...allReportsActionsMock, ...allReportsActionsMock}
});

and then later in the reduce you act as normally?? not sure if the code will work just guessing and code it here on the fly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to do some optimization on this. Can you see this again?

reportKeys,
(errorsMap, reportKey) => {
const report = chatReports[reportKey.replace('reportActions_', 'report_')];
const allReportsActions = _.reduce(allReportActions[reportKey], (acc, reportAction) => ({...acc, [reportAction.reportActionID]: reportAction}), {});
const errors = OptionsListUtils.getAllReportErrors(report, allReportsActions) || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should pass the violations to getAllReportErrors and add them to the dependencies of the memo. If we don't, this function will not be re-triggered if a violation changes in Onyx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should re-render as transactionViolations changes, which is a dependency of memo.

Copy link
Contributor

@youssef-lr youssef-lr Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is not a dependency of reportIDsWithErrors here, which means we won't re-run getAllReportErrors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youssef-lr I think this is not needed. We are already subscribing to the violation collection, and it should re-render whenever any of the violations are updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While yes the page will re-render, the reportIDsWithErrors will remain unchanged, as it only listens to changes in report actions and chat reports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1206agra bump, can we pass the transactions to getAllReportErrors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the confusion, @fedirjh I disagree. Prior to the PR we weren’t pinning RBR reports, now that we’re memoized them, they are not subscribing to changes in the transaction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1206agra @fedirjh what's the latest here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are awaiting @shubham1206agra to implement the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh Can you retest this now?

if (_.size(errors) === 0) {
return errorsMap;
}
return {...errorsMap, [reportKey.replace('reportActions_', '')]: OptionsListUtils.getAllReportErrors(report, allReportsActions) || {}};
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
},
{},
);
}, [allReportActions, chatReports]);

const reportIDsRef = useRef(null);
const isLoading = isLoadingApp;
const optionListItems = useMemo(() => {
Expand All @@ -152,6 +170,7 @@ function SidebarLinksData({
transactionViolations,
activeWorkspaceID,
policyMemberAccountIDs,
reportsWithErrorsIds,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
);

if (deepEqual(reportIDsRef.current, reportIDs)) {
Expand All @@ -165,7 +184,20 @@ function SidebarLinksData({
reportIDsRef.current = reportIDs;
}
return reportIDsRef.current || [];
}, [chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs, isLoading, network.isOffline, prevPriorityMode]);
}, [
chatReports,
betas,
policies,
priorityMode,
allReportActions,
transactionViolations,
activeWorkspaceID,
policyMemberAccountIDs,
reportsWithErrorsIds,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
isLoading,
network.isOffline,
prevPriorityMode,
]);

// We need to make sure the current report is in the list of reports, but we do not want
// to have to re-generate the list every time the currentReportID changes. To do that
Expand All @@ -184,10 +216,23 @@ function SidebarLinksData({
transactionViolations,
activeWorkspaceID,
policyMemberAccountIDs,
reportsWithErrorsIds,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
);
}
return optionListItems;
}, [currentReportID, optionListItems, chatReports, betas, policies, priorityMode, allReportActions, transactionViolations, activeWorkspaceID, policyMemberAccountIDs]);
}, [
currentReportID,
optionListItems,
chatReports,
betas,
policies,
priorityMode,
allReportActions,
transactionViolations,
activeWorkspaceID,
policyMemberAccountIDs,
reportsWithErrorsIds,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
]);

const currentReportIDRef = useRef(currentReportID);
currentReportIDRef.current = currentReportID;
Expand All @@ -209,6 +254,7 @@ function SidebarLinksData({
isLoading={isLoading}
optionListItems={optionListItemsWithCurrentReport}
activeWorkspaceID={activeWorkspaceID}
reportsWithErrorsIds={reportsWithErrorsIds}
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
/>
</View>
);
Expand All @@ -232,6 +278,7 @@ const chatReportSelector = (report) =>
isPinned: report.isPinned,
isHidden: report.isHidden,
notificationPreference: report.notificationPreference,
errors: report.errors,
errorFields: {
addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom,
},
Expand All @@ -253,6 +300,9 @@ const chatReportSelector = (report) =>
reportName: report.reportName,
policyName: report.policyName,
oldPolicyName: report.oldPolicyName,
isPolicyExpenseChat: report.isPolicyExpenseChat,
isOwnPolicyExpenseChat: report.isOwnPolicyExpenseChat,
isCancelledIOU: report.isCancelledIOU,
// Other less obvious properites considered for sorting:
ownerAccountID: report.ownerAccountID,
currency: report.currency,
Expand All @@ -271,14 +321,15 @@ const chatReportSelector = (report) =>
const reportActionsSelector = (reportActions) =>
reportActions &&
lodashMap(reportActions, (reportAction) => {
const {reportActionID, parentReportActionID, actionName, errors = []} = reportAction;
const {reportActionID, parentReportActionID, actionName, originalMessage, errors = []} = reportAction;
const decision = lodashGet(reportAction, 'message[0].moderationDecision.decision');

return {
reportActionID,
parentReportActionID,
actionName,
errors,
originalMessage,
message: [
{
moderationDecision: {decision},
Expand Down
2 changes: 1 addition & 1 deletion tests/perf-test/SidebarUtils.perf-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ describe('SidebarUtils', () => {
await measureFunction(() =>
SidebarUtils.getOptionData({
report,
reportActions,
personalDetails,
preferredLocale,
policy,
parentReportAction,
reportErrors: undefined,
shubham1206agra marked this conversation as resolved.
Show resolved Hide resolved
hasViolations: false,
}),
);
Expand Down
Loading