Revert "Refactored usage of ONYXKEYS.PERSONAL_DETAILS_LIST from Member Actions"#73458
Conversation
melvin-bot
Bot
requested review from
srikarparsi
and removed request for
a team
|
@srikarparsi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| }); | ||
|
|
||
| let allPersonalDetails: OnyxEntry<PersonalDetailsList>; |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Using entire allPersonalDetails object as dependency causes useCallback to re-execute whenever any personal detail changes, even unrelated ones.
Consider specifying individual properties or the specific accountID lookup instead:
const isApproverTemp = useCallback((policy: OnyxEntry<Policy>, employeeAccountID: number) => {
const employeeLogin = allPersonalDetails?.[employeeAccountID]?.login;
return isApprover(policy, employeeLogin ?? '');
}, [allPersonalDetails?.[employeeAccountID]?.login]); // More specific dependency
| @@ -652,21 +675,28 @@ function removeMembers(policyID: string, selectedMemberEmails: string[], policyM | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
❌ PERF-6 (docs)
The accountIDs.reduce() function accesses allPersonalDetails?.[accountID]?.login for each account ID, but the dependency tracking is not optimized. This causes the function to re-execute whenever any personal detail changes.
Consider memoizing the specific personal details needed or restructuring to avoid accessing the entire allPersonalDetails object:
const memberEmails = useMemo(() =>
accountIDs.map(accountID => allPersonalDetails?.[accountID]?.login).filter(Boolean),
accountIDs.map(id => allPersonalDetails?.[id]?.login)
);
|
No need to wait for tests since this is a straight revert. It fixes a deploy blocker |
| function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembersPageProps) { | ||
| const policyMemberEmailsToAccountIDs = useMemo(() => getMemberAccountIDsForWorkspace(policy?.employeeList, true), [policy?.employeeList]); | ||
| const employeeListDetails = useMemo(() => policy?.employeeList ?? ({} as PolicyEmployeeList), [policy?.employeeList]); | ||
| const {policyMemberEmailsToAccountIDs, employeeListDetails} = useMemo(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Using policy?.employeeList as a dependency causes this useMemo to re-execute whenever any property in the employee list changes, even unrelated ones like pending actions or errors.
Consider specifying more granular dependencies:
const {policyMemberEmailsToAccountIDs, employeeListDetails} = useMemo(() => {
// implementation
}, [
Object.keys(policy?.employeeList ?? {}),
...Object.values(policy?.employeeList ?? {}).map(emp => emp?.role),
// other specific properties that actually affect the computation
]);
|
@tgolen looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
| return false; | ||
| } | ||
| if (employee.email === policy?.owner || employee.email === currentUserPersonalDetails.login) { | ||
| const employeeAccountID = getAccountIDsByLogins([employee.email]).at(0); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The getAccountIDsByLogins() function call (expensive operation) is executed before the simple property check !employee?.email. This could be optimized by performing the simple check first.
const filterEmployees = useCallback(
(employee?: PolicyEmployee) => {
// Simple checks first
if (!employee?.email) {
return false;
}
// Expensive operation after simple checks
const employeeAccountID = getAccountIDsByLogins([employee.email]).at(0);
if (!employeeAccountID) {
return false;
}
const isPendingDelete = employeeListDetails?.[employeeAccountID]?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
return accountIDs.includes(employeeAccountID) && !isPendingDelete;
},
[accountIDs, employeeListDetails],
);
| } | ||
| if (employee.email === policy?.owner || employee.email === currentUserPersonalDetails.login) { | ||
| const employeeAccountID = getAccountIDsByLogins([employee.email]).at(0); | ||
| if (!employeeAccountID) { |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The expensive getAccountIDsByLogins([employee.email]).at(0) function call is performed before the simple property check !employeeAccountID. The current structure already has the email check first, but the expensive operation could still be optimized.
Consider caching the account ID lookup or using a more efficient lookup method if this function is called frequently in array iterations.
| const approverEmail = selectedEmployees.find((selectedEmployee) => isApprover(policy, selectedEmployee)); | ||
| if (!approverEmail) { | ||
| const firstSelectedEmployeeAccountID = policyMemberEmailsToAccountIDs[selectedEmployees[0]]; | ||
| const approverAccountID = selectedEmployees.find((selectedEmployee) => isApproverTemp(policy, selectedEmployee)); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The selectedEmployees.find() operation with isApproverTemp() function call (expensive operation) is performed for every selected employee without early termination checks.
Consider adding a simple property check or memoizing the approver status:
const approverAccountID = useMemo(() =>
selectedEmployees.find((selectedEmployee) => {
// Add simple checks first if possible
if (selectedEmployee === policy?.ownerAccountID) return true;
return isApproverTemp(policy, selectedEmployee);
}),
[selectedEmployees, policy, policy?.ownerAccountID]
);
| setRemoveMembersConfirmModalVisible(false); | ||
| setSelectedEmployees((prevSelectedEmployees) => { | ||
| // Filter all personal details in order to use the elements needed for the current workspace | ||
| const currentPersonalDetails = filterPersonalDetails(policy?.employeeList ?? {}, personalDetails); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Using policy?.employeeList and personalDetails as dependencies causes this useEffect to re-execute whenever any property in these large objects changes, even unrelated ones.
Consider memoizing the filtering logic and specifying more granular dependencies:
const memberFiltering = useMemo(() => ({
employeeKeys: Object.keys(policy?.employeeList ?? {}),
personalDetailKeys: Object.keys(personalDetails ?? {})
}), [Object.keys(policy?.employeeList ?? {}), Object.keys(personalDetails ?? {})]);
// Then use memberFiltering in the useEffect dependency
}, [memberFiltering, policyMemberEmailsToAccountIDs]);
| const accountIDsToRemove = session?.accountID ? selectedEmployees.filter((id) => id !== session.accountID) : selectedEmployees; | ||
|
|
||
| // Check if any of the account IDs are approvers | ||
| const hasApprovers = accountIDsToRemove.some((accountID) => isApproverTemp(policy, accountID)); |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The accountIDsToRemove.some((accountID) => isApproverTemp(policy, accountID)) operation calls the expensive isApproverTemp function for each account ID without early termination optimizations.
Consider adding simple checks first or memoizing approver status:
const hasApprovers = accountIDsToRemove.some((accountID) => {
// Simple check first - if it's the owner, it's definitely an approver
if (accountID === policy?.ownerAccountID) return true;
// Then check the expensive approver function
return isApproverTemp(policy, accountID);
});
| ); | ||
| } | ||
|
|
||
| /** Temporary function alias for isApprover with employeeAccountID */ |
There was a problem hiding this comment.
❌ PERF-6 (docs)
This temporary function isApproverTemp directly accesses allPersonalDetails without proper memoization or dependency tracking. This causes performance issues when used in array operations or frequent function calls.
Consider creating a more optimized version or memoizing the login lookup:
const getEmployeeLogin = useMemo(() =>
(employeeAccountID: number) => allPersonalDetails?.[employeeAccountID]?.login,
[allPersonalDetails]
);
function isApproverTemp(policy: OnyxEntry<Policy>, employeeAccountID: number) {
const employeeLogin = getEmployeeLogin(employeeAccountID);
return isApprover(policy, employeeLogin ?? '');
}
Codecov Report❌ Patch coverage is
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
straight revert not an emergency |
Revert "Refactored usage of ONYXKEYS.PERSONAL_DETAILS_LIST from Member Actions" (cherry picked from commit 986deb2) (cherry-picked to staging by blimpich)
|
🚀 Cherry-picked to staging by https://github.com/blimpich in version: 9.2.38-1 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.38-5 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/blimpich in version: 9.2.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.39-3 🚀
|
3 participants
Reverts #70581
Fixes deploy blocker: #73448
See slack thread: https://expensify.slack.com/archives/C01GTK53T8Q/p1761330935961549