Auto-navigate to magic code on Travel CVV RHP mount#89251
Conversation
The Travel CVV RHP's sole purpose is to reveal the CVV, which always requires a magic code. This adds a useEffect that auto-navigates to the verify account page on first mount, removing an unnecessary intermediate click on "Reveal Details". Guards ensure the auto-navigate only fires when: CVV is not already revealed, user is not a delegate, not offline, account is not locked, and the Onyx account record has loaded. A useRef flag prevents re-triggering after cancel or state changes. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot let's update Test section as this: Prerequisite: You must have a Travel Card and have enabled the Travel beta
To update the Offline Test, follow these steps: Prerequisite: Have a Travel Card + Travel beta enabled
|
|
Updated the Tests and Offline tests sections in the PR description as requested. |
| if (account === undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@MelvinBot Why do we use this check instead of isLoadingOnyxValue? Is there a specific reason for this?
There was a problem hiding this comment.
No specific reason — isLoadingOnyxValue would work just as well here. The account === undefined check is functionally equivalent since useOnyx returns undefined for data before the key is loaded, but using isLoadingOnyxValue with the result metadata is the more idiomatic pattern. Happy to switch to it if you'd prefer.
There was a problem hiding this comment.
@MelvinBot Yep, let's switch to isLoadingOnyxValue - This is a more standard pattern than checking for undefined
There was a problem hiding this comment.
Done — switched to isLoadingOnyxValue(accountMetadata) and updated the dependency array.
There was a problem hiding this comment.
@MelvinBot Currently, we’re including the entire accountMetadata object in the dependencies, even though we only need the status. This causes the useEffect to fire whenever the metadata changes, even when we don’t need it to (for example, if the reference to the object changes but the status value remains the same).
I suggest moving isLoadingOnyxValue(accountMetadata) outside of the useEffect and simply using a constant value inside the useEffect. If you don’t see any issues with this solution, then implement it.
There was a problem hiding this comment.
Done — moved isLoadingOnyxValue(accountMetadata) to a const isLoadingAccount outside the effect and updated the dependency array to use it instead of accountMetadata.
Switch from checking `account === undefined` to `isLoadingOnyxValue(accountMetadata)` which is the standard Onyx pattern for detecting loading state. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
…fires Move isLoadingOnyxValue(accountMetadata) to a constant outside the useEffect so the dependency array uses a primitive boolean instead of the entire accountMetadata object. This prevents the effect from re-firing when the metadata object reference changes but the loading status remains the same. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@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: c98bd610e1
ℹ️ 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".
| if (isLoadingAccount) { | ||
| return; | ||
| } | ||
| if (cvv || isSignedInAsDelegate || isOffline || isAccountLocked) { |
There was a problem hiding this comment.
This auto-navigation gate relies on isAccountLocked, but that value defaults to false until NVP_PRIVATE_LOCK_ACCOUNT_DETAILS hydrates in LockedAccountModalProvider (lockAccountDetails?.isLocked ?? false). On cold start or slow Onyx hydration, a genuinely locked account can pass this condition and be pushed straight into the verify-account flow before the lock flag arrives, instead of showing the locked-account modal. Please gate auto-navigation on lock-account data readiness (or perform the lock check from a loaded source) before navigating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern — fixed in 7250e5f. The auto-navigate effect now also waits for NVP_PRIVATE_LOCK_ACCOUNT_DETAILS to finish loading before firing, so a locked account won't slip through during Onyx hydration.
There was a problem hiding this comment.
The concern is valid — isAccountLocked defaults to false before NVP_PRIVATE_LOCK_ACCOUNT_DETAILS hydrates from Onyx, so on cold start a locked account could bypass the guard. Fixed in 7250e5f by also waiting for that Onyx key to finish loading before auto-navigating.
The auto-navigate useEffect relied on isAccountLocked which defaults to false until NVP_PRIVATE_LOCK_ACCOUNT_DETAILS hydrates from Onyx. On cold start a locked account could slip past the guard. Add an isLoadingOnyxValue check on the lock-account-details metadata so auto-navigation waits for both ACCOUNT and NVP_PRIVATE_LOCK_ACCOUNT_DETAILS to finish loading. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@MelvinBot Check all the boxes (even the ones you couldn't verify—I've made sure everything is correct) |
|
All checklist boxes have been checked as requested. |
|
@MelvinBot I think I found an issue with the navigation flow between Travel CVV pages. Steps to reproduce:
Actual result: Expected result: This seems to be caused by useEffect we have added in this PR. What solution do you have, just propose, don't implement yet |
|
The Proposed fix: Check the navigation state before auto-navigating — if This preserves the auto-navigate UX on first entry while preventing the redirect loop on back-navigation after a refresh. Alternative approaches considered
Next Steps: Reply with |
|
@MelvinBot propose code for you solution |
|
Here's the code change in this PR — a single file modified: What it does: Adds a // New imports
import {useRef} from 'react';
import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue';
// Inside TravelCVVPage():
// Pull metadata from useOnyx to know when loading is done
const [account, accountMetadata] = useOnyx(ONYXKEYS.ACCOUNT);
const [, lockAccountDetailsMetadata] = useOnyx(ONYXKEYS.NVP_PRIVATE_LOCK_ACCOUNT_DETAILS);
const isLoadingAccount = isLoadingOnyxValue(accountMetadata);
const isLoadingLockAccountDetails = isLoadingOnyxValue(lockAccountDetailsMetadata);
// Auto-navigate to the magic code screen on first mount so the user
// doesn't have to click "Reveal Details" manually.
const hasAutoNavigatedRef = useRef(false);
useEffect(() => {
if (hasAutoNavigatedRef.current) {
return;
}
// Wait for both Onyx records to load so guards are reliable
if (isLoadingAccount || isLoadingLockAccountDetails) {
return;
}
if (cvv || isSignedInAsDelegate || isOffline || isAccountLocked) {
return;
}
hasAutoNavigatedRef.current = true;
resetValidateActionCodeSent();
Navigation.navigate(ROUTES.SETTINGS_WALLET_TRAVEL_CVV_VERIFY_ACCOUNT);
}, [isLoadingAccount, isLoadingLockAccountDetails, cvv, isSignedInAsDelegate, isOffline, isAccountLocked]);Guards prevent auto-navigation when:
The |
| const isLoadingAccount = isLoadingOnyxValue(accountMetadata); | ||
| const isLoadingLockAccountDetails = isLoadingOnyxValue(lockAccountDetailsMetadata); | ||
|
|
||
| // Auto-navigate to the magic code screen on first mount so the user | ||
| // doesn't have to click "Reveal Details" manually. | ||
| const hasAutoNavigatedRef = useRef(false); | ||
| useEffect(() => { | ||
| if (hasAutoNavigatedRef.current) { | ||
| return; | ||
| } | ||
| // Wait for both the account and lock-account-details Onyx records to load | ||
| // so isSignedInAsDelegate and isAccountLocked are reliable | ||
| if (isLoadingAccount || isLoadingLockAccountDetails) { | ||
| return; | ||
| } | ||
| if (cvv || isSignedInAsDelegate || isOffline || isAccountLocked) { | ||
| return; | ||
| } | ||
| hasAutoNavigatedRef.current = true; | ||
| resetValidateActionCodeSent(); | ||
| Navigation.navigate(ROUTES.SETTINGS_WALLET_TRAVEL_CVV_VERIFY_ACCOUNT); | ||
| }, [isLoadingAccount, isLoadingLockAccountDetails, cvv, isSignedInAsDelegate, isOffline, isAccountLocked]); |
There was a problem hiding this comment.
@MelvinBot propose a solution you have mentioned here: #89251 (comment)
|
Replace the Three changes in 1. Update imports (line 1 + add new import): -import React, {useCallback, useEffect, useRef} from 'react';
+import React, {useCallback, useEffect} from 'react';
+import {useNavigationState} from '@react-navigation/native';Add after import ROUTES from '@src/ROUTES';
+import SCREENS from '@src/SCREENS';2. Replace - const hasAutoNavigatedRef = useRef(false);
+ const isVerifyAccountInStack = useNavigationState((state) =>
+ state.routes.some((route) => route.name === SCREENS.SETTINGS.WALLET.TRAVEL_CVV_VERIFY_ACCOUNT),
+ );3. Update useEffect(() => {
- if (hasAutoNavigatedRef.current) {
+ if (isVerifyAccountInStack) {
return;
}
if (isLoadingAccount || isLoadingLockAccountDetails) {
@@ ... @@
if (cvv || isSignedInAsDelegate || isOffline || isAccountLocked) {
return;
}
- hasAutoNavigatedRef.current = true;
resetValidateActionCodeSent();
Navigation.navigate(ROUTES.SETTINGS_WALLET_TRAVEL_CVV_VERIFY_ACCOUNT);
- }, [isLoadingAccount, isLoadingLockAccountDetails, cvv, isSignedInAsDelegate, isOffline, isAccountLocked]);
+ }, [isVerifyAccountInStack, isLoadingAccount, isLoadingLockAccountDetails, cvv, isSignedInAsDelegate, isOffline, isAccountLocked]);Why this works: Both Next Steps: Reply with |
|
@MelvinBot can we combine both of them(ref+check in navigation) to achieve best result? Something like this: // Auto-navigate to the magic code screen on first mount so the user
// doesn't have to click "Reveal Details" manually.
const hasAutoNavigatedRef = useRef(false);
useEffect(() => {
if (hasAutoNavigatedRef.current) {
return;
}
if (isLoadingAccount || isLoadingLockAccountDetails) {
return;
}
if (isVerifyAccountInStack) {
hasAutoNavigatedRef.current = true;
return;
}
if (cvv || isSignedInAsDelegate) {
hasAutoNavigatedRef.current = true;
return;
}
if (isOffline || isAccountLocked) {
return;
}
hasAutoNavigatedRef.current = true;
resetValidateActionCodeSent();
Navigation.navigate(ROUTES.SETTINGS_WALLET_TRAVEL_CVV_VERIFY_ACCOUNT);
}, [isLoadingAccount, isLoadingLockAccountDetails, cvv, isSignedInAsDelegate, isOffline, isAccountLocked, isVerifyAccountInStack]);Idea: The effect is split into two types of exit conditions. Permanent ones — like the verify screen already being in the stack, CVV already revealed, or delegate access — set the ref to true because the situation won't change during this page visit, so there's no point re-checking. Transient ones — like loading states, being offline, or account being locked — don't touch the ref, so when those conditions resolve the effect re-runs and gets another chance to navigate. |
Split effect exit conditions into permanent (set ref) and transient (skip without setting ref) so the effect retries after transient conditions like offline/locked resolve. Co-authored-by: Eskalifer1 <36520298+Eskalifer1@users.noreply.github.com> Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
Done — combined the ref guard with the navigation stack check. The effect now distinguishes between:
|
|
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. |
|
Could you clarify what you'd like me to add? For example:
Please let me know what text to add and where. |
| if (cvv || isSignedInAsDelegate) { | ||
| hasAutoNavigatedRef.current = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
@MelvinBot Add a comment explaining why we aren't setting a ref here
Summary of changesThis PR modifies How it works: A
Exit conditions are split by permanence:
The existing "Reveal Details" button at line 90 is preserved as a fallback for retry after cancel, locked-account modal trigger, and coming-back-online scenarios. |
…r transient conditions Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
| // Permanent conditions — set the ref so we never re-check | ||
| if (isVerifyAccountInStack) { | ||
| hasAutoNavigatedRef.current = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
@MelvinBot Move this part before if (isLoadingAccount || isLoadingLockAccountDetails) - Since it doesn't matter whether we've already loaded this data, if the user has already visited the confirmation page
And update comment - make it more detailed
There was a problem hiding this comment.
Done — moved the isVerifyAccountInStack check before the loading guards and expanded the comment to explain why it runs first (doesn't depend on Onyx data) and what it prevents (duplicate screen push after cancel/completion).
There was a problem hiding this comment.
Done — this was addressed in commit 1040f39. The isVerifyAccountInStack check now runs before the loading guards, and the comment explains why: it doesn't depend on Onyx data, so we can skip early regardless of loading state.
|
@MelvinBot I'm assigned as C+(you mentioned it before) on this PR and you have permission to mark this Ready for review |
|
I attempted to mark this PR as Ready for review, but I'm unable to execute that action due to environment permission restrictions. Please mark it as Ready for review manually from the GitHub UI. |
|
@MelvinBot Mark PR as Ready for review |
|
I'm unable to mark this PR as ready for review due to environment permission restrictions. You can do this yourself by clicking Ready for review on the PR page, or by running: |
|
Hi @blimpich, can you please mark this as ready for review? Seems like Melvin can't do it :) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp89251-android-native.movAndroid: mWeb Chrome89251-android-web.moviOS: HybridApp89251-ios-native.moviOS: mWeb Safari89251-ios-web.movMacOS: Chrome / Safari89251-web.mov |
melvin-bot
Bot
requested review from
Eskalifer1 and
JmillsExpensify
and removed request for
a team
|
@Eskalifer1 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] |
Eskalifer1
left a comment
Eskalifer1
left a comment
There was a problem hiding this comment.
LGTM!
The isVerifyAccountInStack const was added to handle a super edge case: if a user navigates to the Magic Code page and reloads it, the ref will revert to its default value (since the entire app is refreshed); subsequently, if the user tries to press the back button, they will be immediately redirected back to the Magic Code page. This check specifically handles this edge case.
All your @blimpich!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @blimpich 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.3.65-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The PR modifies the Travel Card CVV flow (
No existing article documents the Travel Card CVV viewing flow, so there is nothing to update. |
|
@blimpich @Eskalifer1 Is this one internal PR, if not how to assign a travel card? |
|
Hi @m-natarajan I think you need to be added to the Workspace with a travel card. Or, someone from the internal team should test it. |
|
I can confirm it works, just tested it in staging |
|
Can I check it off the list? |
|
Yup! |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.65-6 🚀
|
6 participants
Explanation of Change
The Travel CVV RHP's sole purpose is to reveal the CVV, which always requires a magic code. Currently the user must click "Reveal Details" before the magic code prompt appears — an unnecessary intermediate step.
This PR adds a
useEffectinTravelCVVPagethat auto-navigates to the verify account (magic code) page on first mount, so the magic code screen appears immediately when the RHP opens. The auto-navigate is guarded so it only fires when:ACCOUNTrecord has finished loadingA
useRefflag ensures the effect fires at most once per mount, preventing re-triggering when the user cancels and returns. The existing "Reveal Details" button is preserved as a retry path after cancel, as the locked-account modal trigger, and as the fallback when coming back online.Fixed Issues
$ #88964
PROPOSAL: #88964 (comment)
Tests
Prerequisite: You must have a Travel Card and have enabled the Travel beta
Assigned cardssection, tap on Travel CardOffline tests
Prerequisite: Have a Travel Card + Travel beta enabled
Assigned cardssection, tap on Travel CardYou appear to be offline)QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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