Fix/move preselected items to top #80322
Conversation
|
@linhvovan29546 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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| newSections.push(formattedResults.section); | ||
| const selectedIDsSet = new Set(initialSelectedOptions.map((option) => option.accountID)); | ||
| const unselectedFormattedSectionData = formattedResults.section.data.filter((option) => !selectedIDsSet.has(option.accountID)); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Inside the .filter() on line 157, there is a .some() call that iterates through initialSelectedOptions for every item in chatOptions.recentReports. This creates O(n*m) complexity where both operations could be optimized.
Suggested fix: Create a Set of initial selected account IDs before the filter operations to enable O(1) lookups:
const initialSelectedAccountIDs = new Set(initialSelectedOptions.map((option) => option.accountID));
const unselectedRecentReports = chatOptions.recentReports.filter((report) => !initialSelectedAccountIDs.has(report.accountID));This pattern should also be applied to line 162 with chatOptions.personalDetails.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // If the current user is not selected, add them to the top of the list | ||
| if (!selectedCurrentUser && chatOptions.currentUserOption) { | ||
| if (!selectedCurrentUser && chatOptions.currentUserOption && !initialSelectedOptions.some((option) => option.accountID === chatOptions.currentUserOption?.accountID)) { |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The .some() call inside the conditional on line 145 iterates through initialSelectedOptions but doesn't use any iterator-dependent values from the conditional context. Since this is inside a conditional (not an array method callback), this should be optimized.
Suggested fix: Create a Set of account IDs before this conditional:
const initialSelectedAccountIDs = new Set(initialSelectedOptions.map((option) => option.accountID));
// Then use in conditional:
if (!selectedCurrentUser && chatOptions.currentUserOption && !initialSelectedAccountIDs.has(chatOptions.currentUserOption?.accountID)) {This Set can be reused for the filter operations on lines 157 and 162 as well.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| let sortedSectionData = sectionData.sort((a, b) => localeCompare(a?.login?.toLowerCase() ?? '', b?.login?.toLowerCase() ?? '')); | ||
|
|
||
| if (initialSelectedOptions.length && cleanSearchTerm === '') { | ||
| sortedSectionData = [ |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Inside the .map() callback, the selectedOptions.some() call on line 177 doesn't use the iterator (participant) in its logic - it only checks if the participant.accountID matches any selected option. This function call should be hoisted outside the map.
Suggested fix: Create a Set of selected account IDs before the map:
const selectedAccountIDsSet = new Set(selectedOptions.map((option) => option.accountID));
const mappedParticipants = initialSelectedOptions.map((participant) => {
const participantData = {
...participant,
selected: selectedAccountIDsSet.has(participant.accountID),
};
// ... rest of logic
});Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
| allMembers.push({ | ||
| ...formatMemberForList(selected), | ||
| isSelected: true, |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Inside the for loop on line 158, the allMembers.map() call creates a new Set on every iteration. This is an iterator-independent operation that should be hoisted outside the loop.
Suggested fix: Move the seenLogins Set creation before the loop:
const seenLogins = new Set(allMembers.map((member) => member.login));
for (const selected of selectedOptions) {
if (selected.login && seenLogins.has(selected.login)) {
continue;
}
// ... rest of logic
}This eliminates O(n²) complexity where the map runs for every selected option.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| remainingItems.push(option); | ||
| } | ||
| } | ||
| combined.splice(0, combined.length, ...initialItems, ...remainingItems); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Inside the for loop on line 256, the combined.map() call creates a new Set on every iteration. This is an iterator-independent operation that should be hoisted outside the loop.
Suggested fix: Move the seenLogins Set creation before the loop:
const seenLogins = new Set(combined.map((option) => option.login));
for (const option of selectedOptions) {
if (option.login && seenLogins.has(option.login)) {
continue;
}
// ... rest of logic
}This avoids O(n²) complexity where the map executes for every selected option.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| title: undefined, | ||
| data: [availableOptions.userToInvite], | ||
| // Add any selected items not present in searchOptions (defensive) | ||
| const seenLogins = new Set(allMembers.map((member) => member.login)); |
There was a problem hiding this comment.
❌ PERF-13 (docs)
Inside the for loop on line 138, the allMembers.map() call creates a new Set on every iteration. This is an iterator-independent operation that should be hoisted outside the loop.
Suggested fix: Move the seenLogins Set creation before the loop:
const seenLogins = new Set(allMembers.map((member) => member.login));
for (const selected of selectedOptions) {
if (selected.login && seenLogins.has(selected.login)) {
continue;
}
// ... rest of logic
}This eliminates O(n²) complexity where the map executes for every selected option.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| onSelectRow={saveSelectedTimezone} | ||
| textInputOptions={textInputOptions} | ||
| initiallyFocusedItemKey={timezoneOptions.find((tz) => tz.text === timezone.selected)?.keyForList} | ||
| initiallyFocusedItemKey={orderedTimezoneOptions.find((tz) => tz.text === timezone.selected)?.keyForList} |
There was a problem hiding this comment.
❌ PERF-13 (docs)
The .find() call on line 106 is not technically a violation since it's outside of any iteration. However, the inline find could be optimized if performance is a concern by doing the find once and storing the result.
Actually, looking more carefully, this is NOT a PERF-13 violation because:
- The
.find()is not inside another array method callback - It's a standalone operation executed once during render
- There's no iterator-independent function call pattern here
This is acceptable code and should NOT be flagged.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
| }, [addNewCard?.data.selectedCountry, currentCountry, doesCountrySupportPlaid]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The useEffect on line 75 updates state (currentCountry) based on a computed value (getCountry()). This pattern should be avoided - the value can be computed directly.
Suggested fix: Remove the useEffect and derive the value directly:
const currentCountry = useMemo(() => getCountry(), [getCountry]);
// Or simply:
const currentCountry = getCountry();This ensures currentCountry is always synchronized with getCountry() without needing an effect, and eliminates an extra render cycle.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
The effect is intentional:
currentCountry is user-controlled state (it changes when the user taps a country). We can’t derive it directly from getCountry() or we’d lose user selections and be unable to set it via onSelectionChange.
The effect’s job is to resync the default when upstream data (Onyx/route currency) changes; removing it would desync the UI from the new default.
![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: 05b55479e2
ℹ️ 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".
| for (const option of selectedOptions) { | ||
| if (option.login && seenLogins.has(option.login)) { | ||
| continue; | ||
| } | ||
| combined.push(option); |
There was a problem hiding this comment.
This loop appends every selected attendee that isn’t already in the search results, regardless of whether the current search term matches. With a non-empty search term, this makes unrelated selected people appear in the results (previously formatSectionsFromSearchTerm only surfaced selected attendees when they matched the search). That’s a regression for the search UX and can hide relevant matches; consider filtering selectedOptions by the search term before pushing them when cleanSearchTerm is non-empty.
Useful? React with 👍 / 👎.
| for (const selected of selectedOptions) { | ||
| if (selected.login && seenLogins.has(selected.login)) { | ||
| continue; | ||
| } | ||
| allMembers.push({ |
There was a problem hiding this comment.
This block unconditionally re-adds selected members that aren’t part of the current search results. When a user is typing a search term, it causes non-matching selections to show up in the filtered list (previously selectedOptionsForDisplay only showed selected entries that matched the term). That makes the invite search inconsistent and harder to use; consider only appending selected members when there is no active search term.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We compute isSearching once and only append selected members that aren’t in searchOptions when there’s no active search term. This prevents non-matching selected entries from appearing in filtered invite results while keeping the no-jump behavior intact.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Agreed change with @Expensify/design team! Tagging 'em in though, if they want to check it out. 👍
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
@marufsharifi I’ll continue the review after you address the feedback from the AI reviewer.
|
|
||
| const searchResults = searchOptions(debouncedSearchValue, countries); | ||
| const searchResults = useMemo(() => { | ||
| return searchOptions(debouncedSearchValue, countries, initialCountryRef.current ? [initialCountryRef.current] : []); |
There was a problem hiding this comment.
This introduces an unnecessary for loop inside searchOptions that runs whenever countries or debouncedSearchValue changes. We can improve performance by moving the loop outside of searchOptions and handling it at the countries level instead. With this approach, the loop runs only once, improving performance. You also need to apply the same pattern in other places as well.
const allCountries = useMemo(() => {
const excludedCountriesSet = new Set(CONST.PLAID_EXCLUDED_COUNTRIES)
const allCountryKeys = Object.keys(CONST.ALL_COUNTRIES);
// Single pass: filter and reorder simultaneously
const selectedOptions: string[] = [];
const unselectedOptions: string[] = [];
for (const countryISO of allCountryKeys) {
// Skip excluded countries
if (excludedCountriesSet.has(countryISO)) {
continue;
}
// Move initial country to top if it exists
if (initialCountry && countryISO === initialCountry) {
selectedOptions.push(countryISO);
} else {
unselectedOptions.push(countryISO);
}
}
return [...selectedOptions, ...unselectedOptions];
}, [initialCountry])
...
...
const countries = useMemo(
() =>
allCountries
.map((countryISO) => {
const countryName = translate(`allCountries.${countryISO}` as TranslationPaths);
return {
value: countryISO,
keyForList: countryISO,
text: countryName,
isSelected: currentCountry === countryISO,
searchValue: StringUtils.sanitizeString(`${countryISO}${countryName}`),
};
}),
[translate, currentCountry,allCountries],
);
|
Been a while for this one! I also thought we wanted to make sure all selected items would be at the top of the list if they were already selected before opening up the list? Does that sound familiar? Pretty sure we had asked for that a while ago, but I'm not sure where we stand there. |
The previous PR took a long time, and the contributor couldn’t find a working solution after the selection list refactor, so we’re reopening proposals. |
|
Ah okay. Well let's keep that in mind - if you go to a list that already has items selected, those should already be at the top. |
4 participants
Explanation of Change
This will fix the behavior of jumping the lists. When the user selects/deselects, it shouldn't jump. However, when it goes away and comes back, the selected items should be at the very top.
Fixed Issues
$ #69184
PROPOSAL: #69184 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
1. Workspace > Members > Invite new members.
1-v.mp4
2. Workspace > Workflows > Approvals > Approver > set Approver.
2-v.mp4
3. Workspace Chat > Chat Details > Members > Invite New members.
3-v.mp4
4. Workspace > Company Cards > Add Card
Screen.Recording.1404-10-28.at.6.15.28.PM.mov
5. Reports > Search Drop downs > From
Screen.Recording.1404-10-28.at.6.56.03.PM.mov
6. Profile > Timezones
Screen.Recording.1404-10-28.at.7.12.22.PM.mov
7. Reports > Filters > Workspace
Screen.Recording.1404-10-28.at.7.29.56.PM.mov
8. Reports > Filters > Purchase currency
Screen.Recording.1404-10-28.at.7.30.38.PM.mov
9. Reports > Filters > Currency
Screen.Recording.1404-10-28.at.7.32.24.PM.mov
10. Reports > Filters > Category
Screen.Recording.1404-10-28.at.7.37.30.PM.mov
11. Reports > Filters > To
Screen.Recording.1404-10-28.at.7.38.17.PM.mov
12. Workspace > Overview > Default Currency
Screen.Recording.1404-10-28.at.7.57.06.PM.mov
13. Expense details > Amount (tag value)
Screen.Recording.1404-10-28.at.8.07.01.PM.mov
14. Expense details > attendees.
Screen.Recording.1404-11-05.at.12.25.51.PM.mp4
15. Workspace > expensify card > state
Screen.Recording.1404-11-05.at.10.34.22.AM.mov
16. Workspace > workflows > payment account > State
Screen.Recording.1404-11-05.at.10.36.19.AM.mov
17. Profile > Address > Country
Screen.Recording.1404-11-05.at.10.50.14.AM.mov
18. Subscriptions > Add payment card > Currency
Screen.Recording.1404-11-05.at.10.52.08.AM.mov
19. Preferences > Payment Currency
Screen.Recording.1404-11-05.at.10.53.19.AM.mov
20. Reports > Search Drop downs > Workspace
Screen.Recording.1404-11-05.at.12.45.14.PM.mov
21. Wallet > Add Bank account
Screen.Recording.1404-11-05.at.2.10.50.PM.mov
22. Wallet > Add Bank account > Currency
Screen.Recording.1404-11-05.at.2.11.27.PM.mov
23. Profile > Address > State
Screen.Recording.1404-11-05.at.2.13.03.PM.mov
24. Group Chat > Details > members > invite new members
Screen.Recording.1404-11-05.at.1.03.11.PM.mp4
25. Workspace > Workflows > Submissions > Frequency > Monthly > Date of Month
Screen.Recording.1404-11-05.at.2.38.24.PM.mov
26. Preferences > Language
Screen.Recording.1404-11-05.at.3.25.28.PM.mov