Fix selected default tax rate ordering in category rules - #96462
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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] |
|
@codex review |
![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: a188209923
ℹ️ 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 [draftTaxRate, setDraftTaxRate] = useState<string>(); | ||
| const selectedTaxRate = draftTaxRate ?? persistedTaxRate; | ||
| const hasChanges = !!selectedTaxRate && selectedTaxRate !== persistedTaxRate; | ||
| const initialSelectedTaxRate = useInitialSelection(selectedTaxRate, {resetOnFocus: true}); |
There was a problem hiding this comment.
When this screen is opened from a cold start/deep link and usePolicy() is still empty while AccessOrNotFoundWrapper shows its loading state, this snapshots selectedTaxRate as undefined. Since no isVisible/loaded condition is supplied to refresh the snapshot when the Onyx policy later arrives on the same focused screen, initialSelectedTaxRate can remain empty and moveInitialSelectionToTop becomes a no-op, leaving the persisted tax rate buried in long tax lists—the exact case this change is meant to fix. Gate the snapshot on the tax data being loaded or refresh it when persistedTaxRate becomes available.
Useful? React with 👍 / 👎.
|
@linhvovan29546 I've addressed the feedback, you can take another look when you get a chance, thanks |
| @@ -94,7 +103,7 @@ function DynamicCategoryDefaultTaxRatePage({ | |||
| onBackButtonPress={() => Navigation.goBack(categorySettingsBackPath)} | |||
| /> | |||
| <SelectionList | |||
There was a problem hiding this comment.
We should update initiallyFocusedItemKey={persistedTaxRate} to initiallyFocusedItemKey={initialSelectedTaxRate} instead.
There was a problem hiding this comment.
Since the pin already keeps the selected rate on top, I removed initiallyFocusedItemKey — with it, the list auto-scrolled to the tapped rate on every selection. Replaced it with shouldScrollToFocusedIndexOnMount={false}
There was a problem hiding this comment.
That would introduce a regression. On the first arrow key press, the next/previous item is not focus. On the main branch, the next/previous item is focus
Screen.Recording.2026-08-04.at.17.27.08.mov
There was a problem hiding this comment.
Thanks, I just missed that. It has been fixed now
|
Bug: The list scrolls when selecting an item. It shouldn't scroll. Screen.Recording.2026-08-03.at.16.27.55.mov |
@linhvovan29546 Thanks for catching that! This is fixed now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6312311029749522500.mp4Android: mWeb Chrometelegram-cloud-document-5-6312311029749522501.mp4iOS: HybridAppScreen.Recording.2026-08-05.at.14.57.58.moviOS: mWeb SafariScreen.Recording.2026-08-05.at.14.36.57.movMacOS: Chrome / SafariScreen.Recording.2026-08-05.at.14.34.31.mov |
|
@marufsharifi Could you please merge main again? Thanks |
|
Could you please fill in the QA Steps section as well? Thanks! |
|
🚧 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! 🧪🧪
|
4 participants
Explanation of Change
Fixes the Category rules Default tax rate picker so the initially selected tax rate is moved to the top of long tax-rate lists.
Fixed Issues
$ #69184
PROPOSAL: #69184 (comment)
Tests
Preconditions:
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-30.at.11.51.23.PM.mov
Android: mWeb Chrome
Screen_Recording_20260730_234846_Chrome.mp4
iOS: Native
Screen.Recording.2026-07-31.at.12.05.10.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-07-31.at.12.07.39.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-30.at.11.28.57.PM.mov