-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix/75881 - Redirected to Workflow page instead of Frequency page after date selection refresh #77653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e selection refresh
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-06.at.16.08.01.mp4Android: mWeb ChromeScreen.Recording.2026-01-06.at.16.08.55.mp4iOS: HybridAppScreen.Recording.2026-01-06.at.16.11.00.mp4iOS: mWeb SafariScreen.Recording.2026-01-06.at.16.11.40.mp4MacOS: Chrome / SafariScreen.Recording.2026-01-06.at.16.06.17.mp4 |
| const onSelectDayOfMonth = (item: WorkspaceAutoReportingMonthlyOffsetPageItem) => { | ||
| setWorkspaceAutoReportingMonthlyOffset(policy?.id, item.isNumber ? parseInt(item.keyForList, 10) : (item.keyForList as AutoReportingOffsetKeys)); | ||
| Navigation.goBack(ROUTES.WORKSPACE_WORKFLOWS_AUTOREPORTING_FREQUENCY.getRoute(policy?.id)); | ||
| Navigation.setNavigationActionToMicrotaskQueue(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Navigation.setNavigationActionToMicrotaskQueue(() => { | |
| navigateBackAfterPolicyUpdate(() => { |
@dmkt9 I tested the native app, and the flicker issue does not occur on the native platform. Therefore, we won't add this to the native app, because if we do, we might see the scroll-up issue, like in this video.
Screen.Recording.2025-12-15.at.22.06.27.mov
Therefore, we should update setNavigationActionToMicrotaskQueue to be added only for the web platform. For example, in navigateBackAfterPolicyUpdate within helpers, create two files: index.ts and index.native.ts, and wrap navigateBackAfterPolicyUpdate with goBack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huult Thanks for the review. Should we confirm with the design team if this behavior is acceptable? Because I see that it only creates a slight scrolling effect and separating it for two platforms increases code complexity.
I also noticed similar delays after selection in other areas:
2025-12-15.22-35-05.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, Let's wait for @heyjennahay to review.
Hi @heyjennahay When we select a date of the month, the selected item scrolls up a bit and then goes back. Is that okay?
and the currency behavior is:
Screen.Recording.2025-12-15.at.22.51.45.mov
After we made the change:
Screen.Recording.2025-12-15.at.22.52.41.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heyjennahay Could you check my comment when you have time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heyjennahay Could you check my comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huult @MonilBhavsar
On the web, the RHP will be automatically closed after reloading the app. It’s unclear whether this is a new bug or the expected behavior:
It seems that the bug I previously observed has reappeared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, what do we do about it then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve just verified the bug, and it’s resolved. We can proceed with the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmkt9 mind elaborating please? I'm still able to see the redirect bug I mentioned in this branch. Would you want to merge main if you think its fixed there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I’ve just merged the main branch. Thanks.
|
@dmkt9 Please resolve the conflict |
|
The conflict has been resolved. |
|
@heyjennahay Could you review my comment and the behavior in this PR? Thanks. |
|
@dmkt9 Please merge the latest main. |
heyjennahay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the change to the product UI
|
@huult It's ready for review again. |
huult
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Waiting for @dmkt9's thoughts in #77653 (comment) |
|
Looking... |
|
Works now, thanks!! Screen.Recording.2026-01-30.at.6.30.01.PM.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.3.11-19 🚀
|
Explanation of Change
When the user opens the app or refreshes the app while on the modal for selecting the date of the month in the submission frequency, after the refresh, any date the user selects will navigate to the workflow page instead of the Frequency page.
This PR ensures that the user will be navigated correctly after selecting a date.
Fixed Issues
$ #75881
PROPOSAL: #75881 (comment) #75881 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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.native.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
mac.safari.mp4