Stop scroll jump when selecting items in category filter#89392
Stop scroll jump when selecting items in category filter#89392
Conversation
Merge selected and remaining items into a single sorted section instead of splitting them across two sections. This prevents the scroll-to-top triggered by BaseSelectionListWithSections when sections.length > 1. Also set initiallyFocusedItemKey to the first selected item so the list scrolls to the right position on initial render. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| <SelectionListWithSections | ||
| sections={sections} | ||
| ListItem={MultiSelectListItem} | ||
| initiallyFocusedItemKey={firstSelectedKey} |
There was a problem hiding this comment.
@MelvinBot do we also need to set shouldUpdateFocusedIndex true?
There was a problem hiding this comment.
@MelvinBot Also, we need to set shouldClearInputOnSelect false
There was a problem hiding this comment.
No — these two props are independent and serve different purposes:
initiallyFocusedItemKey— one-time setup: scrolls to the first selected item when the list mounts.shouldUpdateFocusedIndex— ongoing: updates the focused index every time the user clicks/selects a row (viaBaseSelectionListWithSections.tsx:171-173).
In a multi-select picker where users are toggling items on and off, enabling shouldUpdateFocusedIndex would cause setFocusedIndex to fire on each click, which could trigger scroll repositioning — potentially reintroducing the scroll-jump behavior this PR is fixing.
There was a problem hiding this comment.
Done — moved shouldClearInputOnSelect from SelectionListProps to BaseSelectionListProps so it's available to SelectionListWithSections, then set it to false in SearchMultipleSelectionPicker. The default remains true to preserve existing behavior for other callers.
Move shouldClearInputOnSelect from SelectionListProps to BaseSelectionListProps so it can be used by SelectionListWithSections. Set it to false in SearchMultipleSelectionPicker to preserve the search filter text when selecting/deselecting items. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
The The runner failed to download the Fix: Re-run the failed job. This should resolve on its own. |
|
The Evidence: The error is The two You can re-run the failed job to resolve this. |
|
@MelvinBot The selected item should not be auto scrolled to the top of the viewport on first selection after opening page first time Screen.Recording.2026-05-03.at.10.55.50.pm.mov |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Removed |
@MelvinBot we still need this to auto scroll to the preselected item when opening page. |
Re-adds initiallyFocusedItemKey so the list scrolls to the first preselected item when opening the page. This does not cause scroll jumps on item selection because FlashList's initialScrollIndex only applies on mount and useArrowKeyFocusManager ignores prop changes after its initial useState call. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Restored |
|
@MelvinBot check off all items in author checklist with this screen recording: demo.mov |
|
I'm unable to process video content, so I can't verify the screen recording to check off the author checklist items. Could you check off the applicable items directly in the PR description? You can edit the PR body and toggle the checkboxes yourself. |
|
@MelvinBot C+ doesn't have permission to edit author checklist. Check off all items as I already verified myself. |
|
Done — all PR Author Checklist items have been checked off. |
|
@MelvinBot add Tests, QA Steps |
|
Added Tests, Offline tests, and QA Steps to the PR description. |
|
@Expensify/design please check if this behavior is fine. Selected item is auto scrolled to the top of the viewport first time. first.selection.movSimilar to scrolling to the preselected first item when open page. first.open.movFrom 2nd selection, auto scroll never happens. Auto scrolled only when search input changes. 2nd.selection.mov |
I don't think we want this cause it's quite jarring :/
On one hand I like this, but it feels overly clever to me given that it's not super obvious there's more items above. I just think we don't want any scrolling or have I missed something here. Curious what @Expensify/design thinks too |
|
ok after disabling scroll in both cases: disable.mov |
|
Curious what @Expensify/design thinks as well, but that feels a bit better to me. I understand the reasoning for wanting the selected options at the top when you go back into the flow though so I can see that still being enabled, but I still don't think we should have any auto-scrolling |
|
Why aren't we solving this everywhere instead of just the filters list? Remember, we have this issue that we were planning to use to fix this behavior across the whole app: #61414 I think the idea is quite simple:
|
|
We'll solve this everywhere eventually. |
| <SelectionListWithSections | ||
| sections={sections} | ||
| ListItem={MultiSelectListItem} | ||
| initiallyFocusedItemKey={firstSelectedKey} |
There was a problem hiding this comment.
initiallyFocusedItemKey={firstSelectedKey}
@MelvinBot while keeping this, we should not update initialScrollIndex of FlashList to prevent auto scroll whenever item is selected. But still update when page is first rendered.
(Scroll happens when this prop value of FlashList is changed from -1)
There was a problem hiding this comment.
Fixed — initiallyFocusedItemKey is now computed once on mount via a useState lazy initializer (from initiallySelectedItems and the sorted items), so it stays stable across re-renders. This prevents initialFocusedIndex → initialScrollIndex from changing when the user toggles selections.
There was a problem hiding this comment.
@MelvinBot "scrolling to the inital focused item on clear search" issue still happens because of native feature in FlashList.
FlashList internally auto-scrolls whenever data changes and updates initiallyFocusedItemKey existence (from "not found" to "found") in data
The ideal solution is to clear initiallyFocusedKey after first render.
There was a problem hiding this comment.
Done — initiallyFocusedItemKey is now cleared after the first render using a ref to track post-mount state. On the initial render the key is passed so FlashList scrolls to the selected item, and on all subsequent renders it's undefined so data changes (like clearing a search) won't trigger auto-scrolling.
Compute the initial focused key once on mount using useState lazy initializer, so it doesn't change when the user toggles selections. This prevents FlashList's initialScrollIndex from updating and triggering auto-scroll on each item click. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
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. |
|
🤖 PR has been marked as ready for review. |
|
@mkhutornyi 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] |
|
Author Checklist failing should be fixed by re-trigger. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
![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: 47fdb69e85
ℹ️ 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".
| const [selectedItemIDs, setSelectedItemIDs] = useState(() => new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))); | ||
| const [initialSelectedIDs] = useState(() => new Set((initiallySelectedItems ?? []).map((item) => item.value.toString()))); | ||
| const [selectedItemIDs, setSelectedItemIDs] = useState(() => initialSelectedIDs); | ||
| const [initiallyFocusedKeyComputed] = useState(() => { |
There was a problem hiding this comment.
initiallyFocusedKeyComputed is captured in a useState initializer, so it is only derived from the very first items snapshot. In flows where picker options arrive asynchronously (for example SearchFiltersCategoryPage builds items from Onyx collections that can populate after first render), the selected option may not exist yet when this runs; the next frame then clears the key, and the picker never scrolls to the first selected item on reopen. This makes the new “scroll to selected item on open” behavior unreliable whenever data loads after mount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
App/src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCategoryPage.tsx
Lines 101 to 107 in 47fdb69
There's guard so it's fine
There was a problem hiding this comment.
Fixed — inlined the computation directly into the initiallyFocusedKey useState initializer, removing the extra state slot.
There was a problem hiding this comment.
Fixed — inlined the computation directly into the initiallyFocusedKey useState initializer, removing the extra state slot.
|
@MelvinBot this is not caused by our PR but I noticed that above line is called 3 times when type anything on search while scrolled down the list. The below line is unnecessarily called so scroll is still animating, which doesn't fix this completely. |
|
Good catch — this is a pre-existing issue in Call 1: Call 2: Immediately after, Call 3: On the next render cycle, when filtered Root cause of the animated scroll: The Fix direction: |
|
@MelvinBot implement this in a new PR as not caused by our PR. |
|
🤖 Created follow-up draft PR: #89498 This implements the fix described in this analysis — suppresses the redundant animated scroll triggered by Please mark it as "Ready for review" when it is ready for review. |
|
@MelvinBot review |
Code ReviewThe approach is sound — merging into a single section eliminates the scroll jump, and the Two things to flag: 1.
|
@MelvinBot fix this |
…initializer Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed the nit — inlined |
|
@francoisl 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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @francoisl 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! 🧪🧪
|
6 participants
Explanation of Change
When selecting a category in the Spend > Filters > Category picker, the selected item jumps to the top of the list and the view scrolls up, disorienting the user.
This happened because
SearchMultipleSelectionPickersplit items into two sections (selected vs remaining). When an item was newly selected,BaseSelectionListWithSections.selectRowdetectedsections.length > 1and calledscrollToIndex(0), scrolling back to the top.This PR merges all items into a single sorted section so selecting an item no longer triggers re-ordering or scroll jumps. It also sets
initiallyFocusedItemKeyto the first selected item so the list scrolls to the right position on initial render.Fixed Issues
$ #61414
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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