[CP Staging] Revert "follow up: useSearchSelector hook and remove useless code"

[rlinoz]

Reverts #72677

$ #73874
$ #73875

[codecov]

Codecov Report

❌ Patch coverage is 0% with 80 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pages/workspace/WorkspaceInvitePage.tsx	0.00%	41 Missing ⚠️
...gWorkspaceInvite/BaseOnboardingWorkspaceInvite.tsx	0.00%	39 Missing ⚠️

Files with missing lines	Coverage Δ
...gWorkspaceInvite/BaseOnboardingWorkspaceInvite.tsx	1.14% <0.00%> (+0.05%)	⬆️
src/pages/workspace/WorkspaceInvitePage.tsx	0.00% <0.00%> (ø)

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[melvin-bot]

@rlinoz looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[rlinoz]

straight revert, no emergency

[melvin-bot]

@blimpich 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]

❌ PERF-6 (docs)

In the large useEffect around line 109-154 in BaseOnboardingWorkspaceInvite.tsx, the dependencies include entire objects like inviteOptions.personalDetails and inviteOptions.userToInvite.

Reasoning: Passing entire objects/arrays as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. This creates unnecessary re-computations.

Suggested fix: Extract specific properties from these objects as dependencies:

}, [options.personalDetails, policy?.employeeList, betas, debouncedSearchTerm, excludedUsers, areOptionsInitialized]);

Remove inviteOptions.personalDetails and inviteOptions.userToInvite from the dependency array since inviteOptions is already computed from the other dependencies.

[github-actions]

❌ PERF-6 (docs)

In WorkspaceInvitePage.tsx, the large useEffect around line 109-179 has the same issue - dependencies include entire objects inviteOptions.personalDetails and inviteOptions.userToInvite.

Reasoning: Passing entire objects/arrays as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking.

Suggested fix: Remove redundant dependencies and use only the primitive values:

}, [options.personalDetails, policy?.employeeList, betas, debouncedSearchTerm, excludedUsers, areOptionsInitialized]);

[github-actions]

❌ PERF-6 (docs)

In BaseOnboardingWorkspaceInvite.tsx around line 156-222, the sections useMemo depends on entire objects/arrays: selectedOptions, personalDetails, usersToInvite.

Reasoning: These array dependencies cause the entire sections array to be recalculated whenever any item in these arrays changes reference, even if the actual data hasn't changed. This is particularly problematic since these arrays are updated frequently in the useEffect above.

Suggested fix: Consider either:

1. Memoizing the individual arrays properly with stable references
2. Using more specific dependencies like array lengths combined with debouncedSearchTerm

}, [areOptionsInitialized, selectedOptions.length, debouncedSearchTerm, personalDetails.length, translate, usersToInvite.length, countryCode]);

Note: This may require additional refactoring to ensure the sections update correctly when item contents change, not just lengths.

[github-actions]

❌ PERF-6 (docs)

In WorkspaceInvitePage.tsx around line 186-223, the sections useMemo has the same issue - it depends on entire arrays: selectedOptions, personalDetails, usersToInvite.

Reasoning: Array dependencies cause unnecessary recalculations when array references change, even if data is identical. This creates a performance bottleneck.

Suggested fix: Use more granular dependencies:

}, [areOptionsInitialized, selectedOptions.length, debouncedSearchTerm, personalDetails.length, translate, usersToInvite.length, countryCode]);

Note: This optimization requires ensuring sections update correctly when actual item contents change. Consider using a hash or version number if item contents need to trigger updates beyond length changes.

[github-actions]

❌ PERF-4 (docs)

In BaseOnboardingWorkspaceInvite.tsx around line 228-232, the inviteUser callback has incomplete dependencies.

Reasoning: The inviteUser callback uses selectedOptions in its logic (lines 169-190 in the current file) but only includes primitive values in dependencies. When selectedOptions changes, the callback should be recreated to capture the latest state.

Suggested fix: Add selectedOptions to the dependency array:

}, [completeOnboarding, onboardingPolicyID, policy?.employeeList, selectedOptions, welcomeNote, welcomeNoteSubject, formatPhoneNumber]);

[github-actions]

❌ PERF-4 (docs)

In WorkspaceInvitePage.tsx around line 277, the inviteUser callback is missing selectedOptions from its dependency array.

Reasoning: The callback uses selectedOptions to iterate and create the invitedEmailsToAccountIDs object, but doesn't include it in dependencies. This can lead to stale closures where the callback operates on outdated data.

Suggested fix: Add selectedOptions to the dependency array:

}, [route.params.policyID, selectedOptions]);

[github-actions]

❌ PERF-6 (docs)

In WorkspaceInvitePage.tsx around line 281-292, the headerMessage useMemo depends on arrays usersToInvite and personalDetails.

Reasoning: Using entire arrays as dependencies causes the header message to recalculate whenever array references change. Since this useMemo only checks the length/existence of these arrays, using the arrays themselves as dependencies is excessive.

Suggested fix: Use array lengths instead:

}, [excludedUsers, translate, debouncedSearchTerm, policy?.name, usersToInvite.length, personalDetails.length, countryCode]);

[github-actions]

❌ PERF-6 (docs)

In BaseOnboardingWorkspaceInvite.tsx around line 290-301, the headerMessage useMemo depends on entire arrays personalDetails and usersToInvite.

Reasoning: The useMemo only checks the length of these arrays (personalDetails.length !== 0, usersToInvite.length > 0), but depends on the entire arrays. This causes unnecessary recalculations when array references change.

Suggested fix: Use array lengths as dependencies:

}, [excludedUsers, translate, debouncedSearchTerm, policy?.name, usersToInvite.length, personalDetails.length, countryCode]);

[github-actions]

⚠️ Performance Concern - PERF-6

In both files, the defaultOptions useMemo (BaseOnboardingWorkspaceInvite.tsx ~line 87-96, WorkspaceInvitePage.tsx ~line 90-99) depends on options.personalDetails which is an array.

Reasoning: While options comes from useOptionsList, depending on options.personalDetails (an array property) means this useMemo will recalculate whenever the array reference changes, even if contents are identical.

Suggested fix: If useOptionsList provides stable references, this may be acceptable. Otherwise, consider:

}, [areOptionsInitialized, betas, excludedUsers, options.personalDetails]);

Verify that useOptionsList properly memoizes options.personalDetails to avoid unnecessary recalculations.

[github-actions]

❌ PERF-6 (docs)

In both files, the inviteOptions useMemo (BaseOnboardingWorkspaceInvite.tsx ~line 98-101, WorkspaceInvitePage.tsx ~line 101-104) depends on the entire defaultOptions object.

Reasoning: filterAndOrderOptions likely only needs specific properties from defaultOptions. Depending on the entire object causes recalculations whenever any property changes.

Suggested fix: Check what properties filterAndOrderOptions actually uses and depend on those specifically. If it uses multiple properties, this might be acceptable, but verify the function implementation.

Example if it only uses personalDetails:

}, [debouncedSearchTerm, defaultOptions.personalDetails, excludedUsers, countryCode]);

[github-actions]

❌ PERF-6 (docs)

In BaseOnboardingWorkspaceInvite.tsx around line 306-334, the footerContent useMemo depends on the entire styles object with individual properties like styles.mb2, styles.mh3.

Reasoning: While depending on individual style properties is correct, ensure all used styles are in the dependency array. However, typically styles from useThemeStyles() is stable and doesn't need to be in dependencies.

Suggested fix: Remove style dependencies if useThemeStyles provides stable references:

}, [completeOnboarding, inviteUser, onboardingIsMediumOrLargerScreenWidth, selectedOptions.length, translate]);

Or if styles must be included, use the entire styles object:

}, [completeOnboarding, inviteUser, onboardingIsMediumOrLargerScreenWidth, selectedOptions.length, styles, translate]);

[github-actions]

❌ PERF-6 (docs)

In WorkspaceInvitePage.tsx around line 294-306, the footerContent useMemo depends on individual style properties: styles.flexReset, styles.flexGrow0, styles.flexShrink0, styles.flexBasisAuto.

Reasoning: Depending on individual style properties is unnecessarily granular. The styles object from useThemeStyles is typically stable and memoized.

Suggested fix: Either depend on the entire styles object or remove style dependencies entirely if they're stable:

}, [inviteUser, policy?.alertMessage, selectedOptions.length, shouldShowAlertPrompt, translate]);

[github-actions]

⚠️ Code Complexity Concern

In BaseOnboardingWorkspaceInvite.tsx (lines ~109-154) and WorkspaceInvitePage.tsx (lines ~109-179), the large useEffect hooks contain significant complexity with multiple operations:

• Creating multiple dictionaries
• Iterating through options multiple times
• Complex state updates for 3 different arrays

Reasoning: While not a strict rule violation, this complexity makes the code harder to maintain and debug. The disabled eslint comment react-compiler/react-compiler and react-hooks/exhaustive-deps suggests the dependency management is fragile.

Suggested improvements:

1. Consider breaking this into smaller, focused useEffects
2. Use useReducer for related state updates (selectedOptions, personalDetails, usersToInvite)
3. Extract the complex logic into a custom hook for reusability
4. Re-evaluate if this revert is necessary - the useSearchSelector hook likely encapsulated this complexity better

[github-actions]

⚠️ Disabled ESLint Rules

In both files, the large useEffect hooks have disabled exhaustive-deps rules:

BaseOnboardingWorkspaceInvite.tsx ~line 154:

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we don't want to recalculate when selectedOptions change

WorkspaceInvitePage.tsx ~line 179:

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we don't want to recalculate when selectedOptions change

Reasoning: Disabling exhaustive-deps is a code smell indicating the dependencies are not correctly specified. The comment "we don't want to recalculate when selectedOptions change" suggests a design issue - the effect reads selectedOptions but excludes it from dependencies, creating a potential infinite loop or stale closure issue.

Suggested fix: This requires architectural changes:

1. Restructure to avoid reading selectedOptions while also updating it
2. Use a ref to track previous selectedOptions and only update if different
3. Consider using useReducer with action dispatchers instead of direct state updates

[github-actions]

⚠️ Performance - Multiple Array Iterations

In the sections useMemo in both files, there are multiple operations that iterate over arrays:

BaseOnboardingWorkspaceInvite.tsx ~lines 158-183:

• Filtering selectedOptions based on search term
• Creating a Set from selectedOptions
• Filtering personalDetails to exclude selected users
• Mapping filtered personalDetails to format

WorkspaceInvitePage.tsx ~lines 190-219 has similar patterns.

Reasoning: While not a rule violation, these multiple iterations could be combined for better performance, especially with large datasets.

Suggested optimization:
Combine the filter and map operations:

const selectedLoginsSet = new Set(filterSelectedOptions.map(({login}) => login));
const personalDetailsFormatted = Object.values(personalDetails)
    .reduce((acc, detail) => {
        if (!selectedLoginsSet.has(detail.login ?? '')) {
            acc.push(formatMemberForList(detail));
        }
        return acc;
    }, [] as MemberForList[]);

This reduces iterations from 3 (filter, map Set, filter+map) to 2 (map Set, reduce).

[github-actions]

❌ PERF-2 (docs)

In BaseOnboardingWorkspaceInvite.tsx (~line 160-169) and WorkspaceInvitePage.tsx (~line 194-203), the .filter() operation performs expensive operations in the wrong order:

filterSelectedOptions = selectedOptions.filter((option) => {
    const accountID = option.accountID;
    const isOptionInPersonalDetails = Object.values(personalDetails).some(...);
    const searchValue = getSearchValueForPhoneOrEmail(debouncedSearchTerm, countryCode);
    const isPartOfSearchTerm = !!option.text?.toLowerCase().includes(searchValue) || !!option.login?.toLowerCase().includes(searchValue);
    return isPartOfSearchTerm || isOptionInPersonalDetails;
});

Reasoning:

1. getSearchValueForPhoneOrEmail() is called for EVERY item in the filter, but it always returns the same value
2. Object.values(personalDetails).some() iterates through all personalDetails for each option
3. Simple property checks on option.text and option.login could eliminate items earlier

Suggested fix:

const searchValue = getSearchValueForPhoneOrEmail(debouncedSearchTerm, countryCode);
filterSelectedOptions = selectedOptions.filter((option) => {
    // Early return for simple text match check
    const isPartOfSearchTerm = !!option.text?.toLowerCase().includes(searchValue) || !!option.login?.toLowerCase().includes(searchValue);
    if (isPartOfSearchTerm) {
        return true;
    }
    
    // Only perform expensive operation if needed
    const accountID = option.accountID;
    return Object.values(personalDetails).some((personalDetail) => personalDetail.accountID === accountID);
});

[github-actions]

Performance Review Summary

I've reviewed PR #73877 which reverts the useSearchSelector hook implementation. I found multiple performance issues in the reverted code:

Critical Issues Found:

1. PERF-2: Expensive operations in filter without early returns
2. PERF-4: Missing dependencies in useCallback hooks
3. PERF-6: Multiple useMemo/useEffect hooks with sub-optimal dependencies (using entire objects/arrays instead of specific properties)

Code Quality Concerns:

• Large, complex useEffect hooks with disabled ESLint rules
• Multiple array iterations that could be combined
• Fragile dependency management requiring manual ESLint suppressions

Recommendation:

Consider whether this revert is necessary. The original useSearchSelector hook likely encapsulated this complexity better and avoided these performance pitfalls. If the revert is required due to bugs #73874 and #73875, please address the performance issues identified in my inline comments before merging.

Total inline comments created: 16

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.2.41-4 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.41-6 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

[OSBotify]

🚀 Cherry-picked to staging by https://github.com/puneetlath in version: 9.2.42-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.42-11 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅