Reapply "Fix filtering attendees names in the Reports page" and add improvements
#80718
Conversation
This reverts commit 9cd909b.
lakchote
changed the title
Reports page"Reports page"
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
|
@eVoloshchak can you review it please? It's the unrevert of #79498 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return false; | ||
| } | ||
| seenAttendees.add(key); | ||
| return true; |
There was a problem hiding this comment.
❌ PERF-2 (docs)
Using .find() inside .filter() creates O(n²) complexity. For each attendee in the outer filter, .find() searches through the entire attendees array. This can be expensive when dealing with large lists of attendees.
Suggested fix: Create a Set for O(1) lookups:
const attendeeKeys = new Set(attendees.map(({email, displayName}) => email || displayName));
const filteredRecentAttendees = deduplicatedRecentAttendees
.filter((attendee) => \!attendeeKeys.has(attendee.email || attendee.displayName || ""))
.map((attendee) => ({...attendee, login: attendee.email || attendee.displayName, ...getPersonalDetailByEmail(attendee.email)}))
.map((attendee) => getParticipantsOption(attendee, personalDetails));Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This is probably a good comment
|
|
||
| // Fallback: construct a minimal option from the identifier string to preserve | ||
| // name-only filters across sessions (e.g., after cache clear or on another device) | ||
| return { |
There was a problem hiding this comment.
❌ PERF-2 (docs)
Inside the map callback, recentAttendees?.find() is called for each identifier. If the identifier is not found in personalDetails, this creates unnecessary iterations through the recentAttendees array, resulting in O(n×m) complexity.
Suggested fix: Create a lookup map for recentAttendees before the mapping:
const recentAttendeesMap = new Map(
recentAttendees?.map(att => [att.email || att.displayName, att]) ?? []
);
preSelectedOptions = initialAccountIDs.map((identifier) => {
const participant = personalDetails[identifier];
if (participant) return getSelectedOptionData(participant);
const attendee = recentAttendeesMap.get(identifier);
if (attendee) return getOptionDataFromAttendee(attendee);
// Fallback...
}).filter((option): option is NonNullable<OptionData> => \!\!option);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const computedSearchTerm = useMemo(() => { | ||
| return getSearchValueForPhoneOrEmail(debouncedSearchTerm, countryCode); | ||
| }, [debouncedSearchTerm, countryCode]); | ||
| const trimmedSearchInput = debouncedSearchTerm.trim(); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The debouncedSearchTerm.trim() call is executed on every render, even though its result is only used inside the baseOptions useMemo. This creates unnecessary computation when the component re-renders for reasons other than debouncedSearchTerm changes.
Suggested fix: Move the trim operation inside a useMemo:
const trimmedSearchInput = useMemo(() => debouncedSearchTerm.trim(), [debouncedSearchTerm]);This ensures .trim() is only called when debouncedSearchTerm actually changes.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b064b5c85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@eVoloshchak yes, a BE fix was needed. It's basically an un-revert PR with some conflicts that needed to be solved. Plus, a fix for #80718 (comment) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-02.at.11.08.54.movAndroid: mWeb ChromeScreen.Recording.2026-02-02.at.11.05.59.moviOS: HybridAppScreen.Recording.2026-02-02.at.11.19.04.moviOS: mWeb SafariScreen.Recording.2026-02-02.at.11.12.49.movMacOS: Chrome / SafariScreen.Recording.2026-02-02.at.11.00.50.mov |
eVoloshchak
left a comment
eVoloshchak
left a comment
There was a problem hiding this comment.
LGTM!
Also verified that this is working across devices signed in with the same account
|
🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #81158. |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Great product polish. Looks solid!
|
Looking now! Not quite a clean revert so will take a little bit |
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
A few comments but seems fine to me!
|
|
||
| if (shouldUseSearchInputValue) { | ||
| userToInvite.text = displayValue; | ||
| userToInvite.displayName = displayValue; | ||
| userToInvite.alternateText = displayValue; | ||
| } else { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| userToInvite.text = userToInvite.text || displayValue; | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| userToInvite.displayName = userToInvite.displayName || displayValue; | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| userToInvite.alternateText = userToInvite.alternateText || displayValue; | ||
| } |
There was a problem hiding this comment.
differs, but seems fine - looks like we added the shouldUseSearchInputValue here.
| <PressableWithFeedback | ||
| accessibilityLabel={item.text ?? ''} | ||
| role={CONST.ROLE.BUTTON} | ||
| sentryLabel={CONST.SENTRY_LABEL.SEARCH.USER_SELECTION_CHECKBOX} |
There was a problem hiding this comment.
Is this used elsewhere? Still learning about Sentry (and this is a new change)
| canInviteUser: false, | ||
| canInviteUser: shouldAllowNameOnlyOptions, | ||
| shouldAcceptName: shouldAllowNameOnlyOptions, | ||
| searchInputValue: searchTerm, |
There was a problem hiding this comment.
This change makes sense but is not a straight revert. Is used later on
| shouldAllowNameOnlyOptions, | ||
| personalDetails, | ||
| currentUserAccountID, | ||
| currentUserEmail, |
There was a problem hiding this comment.
thought this was a change, it's not, but GH won't let me delete this comment. So ignore me.
| // Transform raw recentAttendees into Option[] format for use with getValidOptions (only for attendee filter) | ||
| const recentAttendeeLists = useMemo( | ||
| () => (shouldAllowNameOnlyOptions ? getFilteredRecentAttendees(personalDetails, [], recentAttendees ?? [], currentUserEmail, currentUserAccountID) : []), | ||
| [personalDetails, recentAttendees, currentUserEmail, currentUserAccountID, shouldAllowNameOnlyOptions], |
There was a problem hiding this comment.
How did this get through before without the currentUserEmail / currentUserAccountID stuff? Or was that from a concurrent PR?
| return false; | ||
| } | ||
| seenAttendees.add(key); | ||
| return true; |
There was a problem hiding this comment.
This is probably a good comment
@lakchote it would be good to put this info in the PR description - the conflicts and the related fix, specifically. Shouldn't call this a revert/reapply, exactly. |
lakchote
changed the title
Reports page"Reports page" and add improvements
|
Answering you here @dangrous:
This is now required to be used for the Pressable component (see action run here where lint failed without it): 
These parameters were added as part of resolving merge conflicts when un-reverting. The original I've updated the PR description, thanks for the suggestion! I'm going to proceed with the merge here @MariaHCD as we have one internal engineer review already, along with @eVoloshchak's review as a C+. |
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.3.11-19 🚀
|
5 participants
This reverts commit 9cd909b.
Explanation of Change
It's basically an un-revert PR of #79498 with some conflicts that needed to be solved.
Plus, a fix for #80718 (comment)
Fixed Issues
Related to #80175
Tests
Action Performed:
Precondition:
→ Name and phone number in Step 6 appear in Attendees list.
Expected Result:
Name and phone number in Step 6 will appear in Attendees list.
Screen.Recording.2026-01-28.at.08.49.21.mov
Offline tests
Offline tests
NA
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as in tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari