Conversation
Codecov Report❌ Patch coverage is
... and 388 files with indirect coverage changes 🚀 New features to boost your workflow:
|
MrMuzyk
force-pushed
the
fix/use-sub-page-followup
branch
from
6be78ec to
eeb0777
Compare
melvin-bot
bot
requested review from
dukenv0307 and
joekaufmanexpensify
and removed request for
a team and
joekaufmanexpensify
|
@dukenv0307 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import {findLastPageIndex, findPageIndex} from '@libs/SubPageUtils'; | ||
| import type {SubPageProps, UseSubPageProps} from './types'; | ||
|
|
||
| const AMOUNT_OF_FRAMES_TO_WAIT_FOR = 20; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The value 20 for AMOUNT_OF_FRAMES_TO_WAIT_FOR lacks justification. While there's a comment explaining the general purpose of waiting frames, it doesn't explain why specifically 20 frames is the right amount.
Suggested fix:
Add a comment documenting why 20 frames was chosen:
// Wait 20 frames (~333ms at 60fps) to ensure modal animations complete before navigation
// This value was empirically determined to prevent visual flickers during the transition
const AMOUNT_OF_FRAMES_TO_WAIT_FOR = 20;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-01-28.at.22.53.05.movAndroid: mWeb ChromeScreen.Recording.2026-01-28.at.22.51.10.moviOS: HybridAppScreen.Recording.2026-01-28.at.22.53.18.moviOS: mWeb SafariScreen.Recording.2026-01-28.at.22.50.05.movMacOS: Chrome / SafariScreen.Recording.2026-01-28.at.22.44.52.mov |
|
@arosiclair All yours |
| }); | ||
|
|
||
| if (isRedirecting) { | ||
| return <FullScreenLoadingIndicator />; |
There was a problem hiding this comment.
If we render the loading here, it won't have a header and its back button.
There was a problem hiding this comment.
I'm aware and did it on purpose
|
Also, btw, I follow the pattern by making the For example, const hasAuthError = isAuthenticationError(policy, CONST.POLICY.CONNECTIONS.NAME.NETSUITE);
...
startFrom: hasAuthError ? CREDENTIALS_PAGE_INDEX : 0,Can we instead make function getNetsuiteTokenInputStartFromPage(policy) {
const hasAuthError = isAuthenticationError(policy, CONST.POLICY.CONNECTIONS.NAME.NETSUITE);
return hasAuthError ? CONST.NETSUITE_CONFIG.TOKEN_INPUT_PAGE_NAME.CREDENTIALS : CONST.NETSUITE_CONFIG.TOKEN_INPUT_PAGE_NAME.INSTALL
}
...
Navigation.navigate(ROUTES.POLICY_ACCOUNTING_NETSUITE_TOKEN_INPUT.getRoute(policyID, getNetsuiteTokenInputStartFromPage(policy)));So we won't have this issue. |
|
@bernhardoj We already discussed this issue in https://expensify.slack.com/archives/C05LX9D6E07/p1769613606220809?thread_ts=1768850966.717579&cid=C05LX9D6E07. Can you please take a look and let us know in the Slack thread if you have any ideas? Thanks 😊 |
@bernhardoj We've discussed it on slack in detail but here is a short version. Calculating the the step to navigate to will differ. Sometimes it's always the same, sometimes its based on some BE values that are often fetched but thats not always the case. When we have multiple places in the app that navigate to certain flow and initial step is calculated based on BE response you need to calculate the step in each of this places. It gets even more tricky if you don't have the data before navigating and you need to fetch it. For such cases we need to show the loader and do it inside the hook. What we can possibly do is to navigate to correct screen if you can and it doesn't bring a lot of overhead and use the way with loader as an alternative for more complex cases. |
I see. That make sense. Thanks! |
|
GH actions are down so |
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.3.11-19 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
5 participants
Explanation of Change
We now display a loading indicator while we're calculating the step to which user will get navigated. All animations should be executed correctly now
Fixed Issues
$ #80403
PROPOSAL:
Tests
Same as QA steps
Offline tests
QA Steps
Precondition: User is assigned a physical Expensify card.
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.web.mp4
iOS: Native
ios.native.mp4
iOS: mWeb Safari
ios.web.mp4
MacOS: Chrome / Safari
web.mp4