Fix scroll/reorder jump when selecting in From/To/Attendee/Assignee search filters#94609
Conversation
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
The failing check is unrelated to this PR — no fix pushed. Failing test: This is a date-sensitive flaky test that relies on All PR-relevant checks passed (ESLint, Prettier, typecheck, React Compiler, all build variants, the other 7 test jobs). The remaining Recommendation: Re-run |
|
@MelvinBot pre-selected items should be at the top on first render |
…lters Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — pre-selected items now float to the top on first render. Pushed in a75e24e. I capture the pre-selected Changeconst [initiallySelectedAccountIDs] = useState(() => new Set(value));
const orderedPersonalDetails =
initiallySelectedAccountIDs.size === 0
? availableOptions.personalDetails
: [
...availableOptions.personalDetails.filter((option) => initiallySelectedAccountIDs.has(option.accountID.toString())),
...availableOptions.personalDetails.filter((option) => !initiallySelectedAccountIDs.has(option.accountID.toString())),
];Verified locally: Prettier (clean), ESLint ( |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Fixed the failing What was wrong: The check wasn't a normal "incomplete checklist" failure — it was crashing with Fix: Restored the full
All other checks are green — |
| // Order personalDetails so pre-selected rows render at the top, each group keeping its natural sorted order. | ||
| const orderedPersonalDetails = | ||
| initiallySelectedAccountIDs.size === 0 | ||
| ? availableOptions.personalDetails | ||
| : [ | ||
| ...availableOptions.personalDetails.filter((option) => initiallySelectedAccountIDs.has(option.accountID.toString())), | ||
| ...availableOptions.personalDetails.filter((option) => !initiallySelectedAccountIDs.has(option.accountID.toString())), | ||
| ]; |
There was a problem hiding this comment.
@MelvinBot Use existing moveInitialSelectionToTop util function
There was a problem hiding this comment.
Done — replaced the manual pinning loop with moveInitialSelectionToTop in UserSelector.tsx:62-68. Since the util keys on value and these options key on keyForList (= the accountID), I map value: option.keyForList onto each item before passing it in — matching how the other consumers (e.g. PushRowModal) feed the util.
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — switched to the existing The util keys on each item's Changeconst [initialSelectedValues] = useState(() => [...value]);
const orderedPersonalDetails = moveInitialSelectionToTop(
availableOptions.personalDetails.map((option) => ({...option, value: option.keyForList})),
initialSelectedValues,
);Note: the util only reorders when the list is longer than Verified locally: Prettier (clean), ESLint ( |
|
|
||
| // The current user is excluded from personalDetails, so render it (when present) at the top, followed by | ||
| // the rest of the contacts. Selected rows already live within personalDetails in their sorted position. | ||
| const listData = availableOptions.currentUserOption ? [availableOptions.currentUserOption, ...orderedPersonalDetails] : orderedPersonalDetails; |
There was a problem hiding this comment.
@MelvinBot current user should be below selected items, not top
There was a problem hiding this comment.
Done. The current user is now included in the list passed to moveInitialSelectionToTop (right after currentUserOption), so pre-selected rows float above it and the current user renders below the selected items — see UserSelector.tsx:66.
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — the current user now renders below the pinned pre-selected rows, not at the top. Pushed in c064150. The order on first render is now: pinned pre-selected rows → current user → remaining contacts. When nothing is pre-selected (or the list is under the move-to-top threshold), the current user stays at the top as before, since there's nothing pinned above it. Changeconst personalDetailsWithValue = availableOptions.personalDetails.map((option) => ({...option, value: option.keyForList}));
const orderedPersonalDetails = moveInitialSelectionToTop(personalDetailsWithValue, initialSelectedValues);
const selectedValues = new Set(initialSelectedValues);
const pinnedCount =
orderedPersonalDetails === personalDetailsWithValue ? 0 : personalDetailsWithValue.reduce((count, option) => (selectedValues.has(option.value) ? count + 1 : count), 0);
const listData = availableOptions.currentUserOption
? [...orderedPersonalDetails.slice(0, pinnedCount), availableOptions.currentUserOption, ...orderedPersonalDetails.slice(pinnedCount)]
: orderedPersonalDetails;
|
|
Looked at the current failures on
The Jest step never emitted a test failure — the Blacksmith runner died mid-run (OOM/network). Evidence it's unrelated to the change:
Fix: just re-run the failed job (Re-run failed jobs on the run, or push any new commit). I can't trigger the re-run myself — my token gets
Everything else is green — all builds, ESLint, Prettier, Bun tests, perf tests, Storybook, and test shards 1–7. |
@MelvinBot Your last commit is quite complex (i.e. introducing |
…pinnedCount Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Simplified —
const [initiallySelectedAccountIDs] = useState(() => new Set(value));
const shouldPinSelectedRows = initiallySelectedAccountIDs.size > 0 && availableOptions.personalDetails.length > CONST.MOVE_SELECTED_ITEMS_TO_TOP_OF_LIST_THRESHOLD;
const pinnedPersonalDetails: OptionData[] = [];
const remainingPersonalDetails: OptionData[] = [];
for (const option of availableOptions.personalDetails) {
const isPinned = shouldPinSelectedRows && initiallySelectedAccountIDs.has(option.accountID.toString());
(isPinned ? pinnedPersonalDetails : remainingPersonalDetails).push(option);
}
const listData = availableOptions.currentUserOption
? [...pinnedPersonalDetails, availableOptions.currentUserOption, ...remainingPersonalDetails]
: [...pinnedPersonalDetails, ...remainingPersonalDetails];This drops Verified locally: Prettier (clean), ESLint ( |
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot we should NOT use |
…_LIST_THRESHOLD Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — dropped
This also lines the pinning threshold up with the search-input threshold ( Verified locally: Prettier (clean), ESLint ( |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Done — passed It defaults to Verified locally: Prettier (clean), ESLint ( |
mkhutornyi
left a comment
There was a problem hiding this comment.
@MelvinBot > >= CONST.STANDARD_LIST_ITEM_LIMIT. It should match search input visibility. When 12, we show search so should be pinnable
…T_ITEM_LIMIT)
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Done — changed the pin gate from Since Verified locally: Prettier (clean), ESLint ( |
|
|
||
| function moveInitialSelectionToTop<T extends {value: string}>(items: T[], initialSelectedValues: string[]): T[] { | ||
| if (initialSelectedValues.length === 0 || items.length <= CONST.MOVE_SELECTED_ITEMS_TO_TOP_OF_LIST_THRESHOLD) { | ||
| if (initialSelectedValues.length === 0 || items.length <= CONST.STANDARD_LIST_ITEM_LIMIT) { |
There was a problem hiding this comment.
| if (initialSelectedValues.length === 0 || items.length <= CONST.STANDARD_LIST_ITEM_LIMIT) { | |
| if (initialSelectedValues.length === 0 || items.length < CONST.STANDARD_LIST_ITEM_LIMIT) { |
There was a problem hiding this comment.
Applied — changed the guard to items.length < CONST.STANDARD_LIST_ITEM_LIMIT so the list reorders once it's long enough to show the search input (>= STANDARD_LIST_ITEM_LIMIT), consistent with shouldShowSearchInput. See SelectionListOrderUtils.ts:4.
|
@parasharrajat @ChavdaSachin @mkhutornyi One of you needs to 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 750ccd4ed2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function moveInitialSelectionToTop<T extends {value: string}>(items: T[], initialSelectedValues: string[]): T[] { | ||
| if (initialSelectedValues.length === 0 || items.length <= CONST.MOVE_SELECTED_ITEMS_TO_TOP_OF_LIST_THRESHOLD) { | ||
| if (initialSelectedValues.length === 0 || items.length < CONST.STANDARD_LIST_ITEM_LIMIT) { |
There was a problem hiding this comment.
Preserve pinning for 9–11 item picker lists
When this shared helper is called by existing pickers with 9–11 rows, this new threshold now returns the list unchanged, whereas the previous MOVE_SELECTED_ITEMS_TO_TOP_OF_LIST_THRESHOLD behavior pinned those saved selections. That regresses callers such as PushRowModal and ValueSelectionList that rely on moveInitialSelectionToTop to surface the saved value on reopen; the pre-existing PushRowModal test covered a 10-option list and had to be changed because this selected row now stays at the bottom. Please keep the old visibility threshold for the shared helper or make the new threshold opt-in for the search-filter use case.
Useful? React with 👍 / 👎.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.22-1 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help articles under This PR is a UI interaction bug fix — it stops the From / To / Attendee / Assignee people-picker lists in the Search Filters popover from scrolling to the top and reordering when you toggle a selection, while still pinning pre-selected rows to the top on first open. It changes list scroll/ordering behavior only. It does not:
The help site does not document the scroll/reorder behavior of these picker lists (the closest article, Use Search Operators to Filter and Analyze, covers search operators/syntax, not picker interaction mechanics), so there is nothing to update. No draft PR was created. @mkhutornyi, since no help site changes are needed, there's no linked docs PR to review. Let me know if you'd still like me to document anything specific here. |
|
Deploy Blocker #94843 was identified to be related to this PR. |
|
This PR failing because of the issue #94843 |
Please see #94843 (comment) |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.24-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.4.24-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.4.24-0 🚀
|
Explanation of Change
The From / To / Attendee / Assignee people pickers in the Search Filters popover are all rendered by
UserSelector(SearchFiltersParticipantsSelectoris being deprecated). Two things made the list feel unstable while selecting:selectUsercalledselectionListRef.current.scrollToIndex(0)after toggling.listDatarenderedavailableOptions.selectedOptionsas a prefix, so a freshly-toggled row left its sorted position and floated up.This PR keeps a toggled row exactly where it is, while still surfacing the items that were already selected when the filter first opened at the top of the list:
shouldKeepSelectedInAvailableOptions: truetousePersonalDetailSearchSelector, so selected options stay insideavailableOptions.personalDetailsin their natural sorted position (with theirisSelectedflag) instead of being filtered out and re-prepended. This is the same approach already shipped forDynamicWorkspaceInvitePage(PR Stop scroll jump on workspace invite member selection #91250) for this exact issue.scrollToIndex(0)call and the now-unusedselectionListRef/ imports.moveInitialSelectionToToputil. The initial selection is snapshotted once withuseInitialValue(() => value), so the ordering keys on what was selected when the filter opened — rows toggled afterwards keep their sorted position and are not re-pinned. The current user (excluded frompersonalDetails) is prepended to the base list before ordering, then each option'skeyForListis mapped ontovalueso the util can match it.moveInitialSelectionToTop's threshold from the bespokeMOVE_SELECTED_ITEMS_TO_TOP_OF_LIST_THRESHOLD(8) constant — now removed — to the existingSTANDARD_LIST_ITEM_LIMIT(12), the same point at which the search input appears. Short, fully-visible lists are left in their natural order. Because the util is shared, this also updates its existing consumers' tests (SelectionListOrderUtilsTest,PushRowModalTest,CountrySelectionListTest,ValueSelectionListTest).shouldUpdateFocusedIndexto theSelectionListso keyboard focus tracks the reordered rows.Fixed Issues
$ #61414
PROPOSAL:
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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
Screenshots/Videosundefined