Fix selection jump in rule and currency pickers - #96587
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
…vent auto-scroll on selection; add tests for CurrencySelectionList
|
@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] |
|
@linhvovan29546 The PR is ready for review. thanks |
|
@linhvovan29546, Gentle bump. thanks. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f123090299
ℹ️ 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 selectedCurrencyCodes = [initiallySelectedCurrencyCode, ...selectedCurrencies].filter(Boolean); | ||
| const initiallyPinnedCurrencyCodes = useInitialSelection(selectedCurrencyCodes, {resetOnFocus: true}); |
There was a problem hiding this comment.
Memoize the pinned currency code list
When the currency picker is focused and re-renders for local state, such as typing in the search field, this expression creates a fresh array every render. useInitialSelection(..., {resetOnFocus: true}) refreshes the snapshot on focused renders and compares with Object.is, so the fresh array is always treated as a new selection and schedules another state update; typing in any currency picker can then loop until React reports a maximum update-depth/console error. Please memoize this code list, or pass a stable primitive/key, before handing it to useInitialSelection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@marufsharifi Sorry, but I think the bot's comment is incorrect since this component already has React Compiler enabled. So I don't think we need to manually memoize it.
I haven't tested this yet. Could you please confirm whether this is actually an issue?
|
|
||
| import type {CurrencyListItem, CurrencySelectionListProps} from './types'; | ||
|
|
||
| const EMPTY_SELECTED_CURRENCIES: string[] = []; |
There was a problem hiding this comment.
Can we use an empty array constant, such as EMPTY_ARRAY, here?
|
@linhvovan29546 The feedbacks have been resolved, Could please take another look? thanks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6327730391838893182.1.mp4Android: mWeb Chrometelegram-cloud-document-5-6327730391838893217.mp4iOS: HybridAppScreen.Recording.2026-08-10.at.20.18.57.moviOS: mWeb SafariScreen.Recording.2026-08-10.at.20.02.46.movScreen.Recording.2026-08-10.at.20.04.26.movScreen.Recording.2026-08-10.at.20.05.06.movMacOS: Chrome / SafariScreen.Recording.2026-08-10.at.19.14.28.movScreen.Recording.2026-08-10.at.19.25.52.movScreen.Recording.2026-08-10.at.19.28.58.mov |
| footerContent={shouldAutoSave ? undefined : footerContent} | ||
| shouldShowLoadingPlaceholder={!noResultsFound} | ||
| shouldUpdateFocusedIndex | ||
| shouldPreventAutoScrollOnSelect |
There was a problem hiding this comment.
This is redundant since shouldPreventAutoScrollOnSelect only works when canSelectMultiple is enabled.
| textInputOptions={textInputOptions} | ||
| shouldShowTextInput={!!searchInputLabel} | ||
| shouldSingleExecuteRowSelect | ||
| shouldPreventAutoScrollOnSelect |
There was a problem hiding this comment.
This is redundant since shouldPreventAutoScrollOnSelect only works when canSelectMultiple is enabled.
…t and SearchSingleSelectionPicker
|
Bug: The currency picker on the Debug Mode page doesn't move to the top or show the selected currency. Screen.Recording.2026-08-10.at.19.40.58.mov |
Thanks for catching that — the Debug picker wasn't passing the current currency into |
|
Bug: The currency picker scrolls when selecting an item. To reproduce this, disable the navigation function when selecting an item to force the issue to reproduce. Screen.Recording.2026-08-10.at.20.22.53.mov |
|
@marufsharifi Just make sure you didn't miss this comment #96587 (comment) |
Thanks, I've fixed that as well. |
I mean, I think the bot's comment is incorrect. |
Yeah , got it. my comment was about another bug you found, I quoted in the comment. |
|
Thanks! |
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective
|
🚧 robertjchen has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/robertjchen in version: 9.4.53-0 🚀
|
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site articles under Why: This PR is a purely internal UI fix. It changes how the rule and currency pickers pin the initially selected item (via a new The help site documents what these features do (rules, currency/payment currency selection), not this kind of list-ordering micro-interaction, so there is nothing to add or correct. A search of Since no changes are required, I did not create a draft docs PR. @marufsharifi, if you believe a specific behavior here should be reflected in the help site, let me know which article and I'll draft the update. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.53-10 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Fixes list jumps in rule and currency pickers where selecting an item could immediately move it to the top before the picker closes.
Fixed Issues
$ #69184
PROPOSAL: #69184 (comment)
Tests
Flow 1:
Flow 2:
Flow 3:
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Screen.Recording.2026-07-31.at.5.14.15.PM.mp4
Android: mWeb Chrome
Screen_Recording_20260731_170338_Chrome.1.mp4
iOS: Native
Screen.Recording.2026-07-31.at.5.54.01.PM.mp4
iOS: mWeb Safari
Screen.Recording.2026-07-31.at.5.42.20.PM.mp4
MacOS: Chrome / Safari
Screen.Recording.2026-07-31.at.4.51.32.PM.mp4