diff --git a/src/hooks/useArchivedReportsIdSet.ts b/src/hooks/useArchivedReportsIdSet.ts index be348188469d2..2f20890043448 100644 --- a/src/hooks/useArchivedReportsIdSet.ts +++ b/src/hooks/useArchivedReportsIdSet.ts @@ -17,6 +17,7 @@ function useArchivedReportsIdSet(): ArchivedReportsIDSet { // useDeepCompareRef is used here to prevent unnecessary re-renders by maintaining referential equality // when the Set contents are the same, even if it's a new Set instance. This is important for performance // optimization since Sets are reference types and would normally cause re-renders even with same values + // Reassure test confirmed additional rerender when not using useDeepCompareRef return useDeepCompareRef(archivedReportsIdSet) ?? new Set(); } diff --git a/src/hooks/useDeepCompareRef.ts b/src/hooks/useDeepCompareRef.ts index d5aa85d5398df..5d188d508bb44 100644 --- a/src/hooks/useDeepCompareRef.ts +++ b/src/hooks/useDeepCompareRef.ts @@ -2,6 +2,8 @@ import {deepEqual} from 'fast-equals'; import {useRef} from 'react'; /** + * ⚠️ **WARNING: ANTI-PATTERN - AVOID IN NEW CODE** ⚠️ + * * This hook returns a reference to the provided value, * but only updates that reference if a deep comparison indicates that the value has changed. * @@ -14,6 +16,22 @@ import {useRef} from 'react'; * useEffect(() => { * // This will run not just when myArray is a new array, but also when its contents change. * }, [deepComparedArray]); + * + * **Why this is problematic:** + * - Violates React's exhaustive deps rule (can cause stale closures) + * - Performance overhead from deep equality checks on every render + * - Incompatible with React Compiler optimizations + * - Usually indicates a data flow problem that should be fixed at the source + * + * **Use instead:** + * - `useMemo` with primitive dependencies + * - Fix selectors/data sources to return stable references + * + * **Only use when ALL of these apply:** + * - Legacy infrastructure forces new references (e.g., Onyx collections) + * - Documented why it's necessary and what the risks are + * - No feasible alternative without major refactoring + * - Performance impact measured with tests (e.g., Reassure) */ export default function useDeepCompareRef(value: T): T | undefined { const ref = useRef(undefined); diff --git a/src/hooks/usePrivateIsArchivedMap.ts b/src/hooks/usePrivateIsArchivedMap.ts index 2a325ad474ca0..dbc28b90dae0a 100644 --- a/src/hooks/usePrivateIsArchivedMap.ts +++ b/src/hooks/usePrivateIsArchivedMap.ts @@ -14,6 +14,8 @@ function usePrivateIsArchivedMap(): PrivateIsArchivedMap { selector: privateIsArchivedMapSelector, }); + // useDeepCompareRef prevents unnecessary rerenders when the object has same content but different reference. + // The selector always returns a new object instance, and useMemo cannot compare by value. return useDeepCompareRef(privateIsArchivedMap) ?? {}; } diff --git a/src/hooks/useSidebarOrderedReports.tsx b/src/hooks/useSidebarOrderedReports.tsx index 26d707ce24a52..c9e1059801334 100644 --- a/src/hooks/useSidebarOrderedReports.tsx +++ b/src/hooks/useSidebarOrderedReports.tsx @@ -222,6 +222,8 @@ function SidebarOrderedReportsContextProvider({ clearCacheDummyCounter, ]); + // useDeepCompareRef prevents unnecessary useCallback recreations when these have same content but different reference. + // Without this, getOrderedReportIDs would be recreated on every render, causing expensive orderedReportIDs recalculation. const deepComparedReportsToDisplayInLHN = useDeepCompareRef(reportsToDisplayInLHN); const deepComparedReportsDrafts = useDeepCompareRef(reportsDrafts); diff --git a/src/pages/inbox/ReportScreen.tsx b/src/pages/inbox/ReportScreen.tsx index 0b2df4f01a018..6ef61499b0dca 100644 --- a/src/pages/inbox/ReportScreen.tsx +++ b/src/pages/inbox/ReportScreen.tsx @@ -24,7 +24,6 @@ import useShowWideRHPVersion from '@components/WideRHPContextProvider/useShowWid import WideRHPOverlayWrapper from '@components/WideRHPOverlayWrapper'; import useAppFocusEvent from '@hooks/useAppFocusEvent'; import {useCurrentReportIDState} from '@hooks/useCurrentReportID'; -import useDeepCompareRef from '@hooks/useDeepCompareRef'; import useIsAnonymousUser from '@hooks/useIsAnonymousUser'; import useIsReportReadyToDisplay from '@hooks/useIsReportReadyToDisplay'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; @@ -192,8 +191,6 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr const deletedParentAction = isDeletedParentAction(parentReportAction); const prevDeletedParentAction = usePrevious(deletedParentAction); - const permissions = useDeepCompareRef(reportOnyx?.permissions); - const isAnonymousUser = useIsAnonymousUser(); const [isLoadingReportData = true] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA, {canBeMissing: true}); const prevIsLoadingReportData = usePrevious(isLoadingReportData); @@ -287,12 +284,12 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr private_isArchived: reportNameValuePairsOnyx?.private_isArchived, lastMentionedTime: reportOnyx.lastMentionedTime, avatarUrl: reportOnyx.avatarUrl, - permissions, + permissions: reportOnyx?.permissions, invoiceReceiver: reportOnyx.invoiceReceiver, policyAvatar: reportOnyx.policyAvatar, nextStep: reportOnyx.nextStep, }, - [reportOnyx, reportNameValuePairsOnyx?.private_isArchived, permissions], + [reportOnyx, reportNameValuePairsOnyx?.private_isArchived], ); const reportID = report?.reportID; diff --git a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx index 9a344373defbe..d21a8e39c0572 100644 --- a/src/pages/iou/request/step/IOURequestStepConfirmation.tsx +++ b/src/pages/iou/request/step/IOURequestStepConfirmation.tsx @@ -15,7 +15,6 @@ import PrevNextButtons from '@components/PrevNextButtons'; import ScreenWrapper from '@components/ScreenWrapper'; import useConfirmModal from '@hooks/useConfirmModal'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; -import useDeepCompareRef from '@hooks/useDeepCompareRef'; import useFetchRoute from '@hooks/useFetchRoute'; import useFilesValidation from '@hooks/useFilesValidation'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; @@ -146,12 +145,6 @@ function IOURequestStepConfirmation({ () => (!isLoadingCurrentTransaction ? (optimisticTransaction ?? existingTransaction) : undefined), [existingTransaction, optimisticTransaction, isLoadingCurrentTransaction], ); - const transactionsCategories = useDeepCompareRef( - transactions.map(({transactionID, category}) => ({ - transactionID, - category, - })), - ); const isUnreported = transaction?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID; const isCreatingTrackExpense = action === CONST.IOU.ACTION.CREATE && iouType === CONST.IOU.TYPE.TRACK; const {policyForMovingExpenses, policyForMovingExpensesID} = usePolicyForMovingExpenses(); @@ -398,7 +391,7 @@ function IOURequestStepConfirmation({ } // We don't want to clear out category every time the transactions change // eslint-disable-next-line react-hooks/exhaustive-deps - }, [policy?.id, policyCategories, transactionsCategories]); + }, [policy?.id, policyCategories, transactions.length]); const policyDistance = Object.values(policy?.customUnits ?? {}).find((customUnit) => customUnit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE); const defaultCategory = policyDistance?.defaultCategory ?? ''; diff --git a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx index f198292521ab1..ddc44eaacd4d1 100644 --- a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx +++ b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsExpensesFromPage.tsx @@ -4,7 +4,6 @@ import type {SelectionListApprover} from '@components/ApproverSelectionList'; import ApproverSelectionList from '@components/ApproverSelectionList'; import FormAlertWithSubmitButton from '@components/FormAlertWithSubmitButton'; import Text from '@components/Text'; -import useDeepCompareRef from '@hooks/useDeepCompareRef'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; @@ -33,7 +32,6 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat const styles = useThemeStyles(); const {translate} = useLocalize(); const [approvalWorkflow, approvalWorkflowResults] = useOnyx(ONYXKEYS.APPROVAL_WORKFLOW, {canBeMissing: true}); - const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {canBeMissing: false}); const icons = useMemoizedLazyExpensifyIcons(['FallbackAvatar']); const isLoadingApprovalWorkflow = isLoadingOnyxValue(approvalWorkflowResults); @@ -45,8 +43,6 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat const shouldShowListEmptyContent = !isLoadingApprovalWorkflow && approvalWorkflow && approvalWorkflow.availableMembers.length === 0; const firstApprover = approvalWorkflow?.originalApprovers?.[0]?.email ?? ''; - const personalDetailLogins = useDeepCompareRef(Object.fromEntries(Object.entries(personalDetails ?? {}).map(([id, details]) => [id, details?.login]))); - useEffect(() => { if (!approvalWorkflow?.members) { return; @@ -56,7 +52,6 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat approvalWorkflow.members.map((member) => { const policyMemberEmailsToAccountIDs = getMemberAccountIDsForWorkspace(policy?.employeeList); const accountID = Number(policyMemberEmailsToAccountIDs[member.email] ?? ''); - const login = personalDetailLogins?.[accountID]; return { text: Str.removeSMSDomain(member.displayName), @@ -69,13 +64,13 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat ), }; }), ); - }, [approvalWorkflow?.members, policy?.employeeList, policy?.owner, personalDetailLogins, translate, icons.FallbackAvatar]); + }, [approvalWorkflow?.members, policy?.employeeList, policy?.owner, translate, icons.FallbackAvatar]); const approversEmail = approvalWorkflow?.approvers.map((member) => member?.email); const allApprovers: SelectionListApprover[] = [...selectedMembers]; @@ -85,7 +80,6 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat .map((member) => { const policyMemberEmailsToAccountIDs = getMemberAccountIDsForWorkspace(policy?.employeeList); const accountID = Number(policyMemberEmailsToAccountIDs[member.email] ?? ''); - const login = personalDetailLogins?.[accountID]; return { text: Str.removeSMSDomain(member.displayName), @@ -98,7 +92,7 @@ function WorkspaceWorkflowsApprovalsExpensesFromPage({policy, isLoadingReportDat ), };