Apply MFA flow to revealing virtual card details for EU/UK cards - #91228
Conversation
|
@abzokhattab 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ea2d1323
ℹ️ 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 cvv = typeof callbackInput.body?.cvv === 'string' ? callbackInput.body.cvv : ''; | ||
| setRevealedCardDetails(payload.cardID, {pan, expiration, cvv}); | ||
| Navigation.closeRHPFlow(); | ||
| Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAIN_CARD.getRoute(String(payload.cardID))); |
There was a problem hiding this comment.
After a successful SCA reveal, this callback always routes to SETTINGS_WALLET_DOMAIN_CARD, which changes behavior for users who started from SCREENS.DOMAIN_CARD.DOMAIN_CARD_DETAIL (RHP flow). The previous magic-code flow preserved context by navigating back within the current navigator, so this introduces a UX/navigation regression where domain-card users are forced out of their current stack after reveal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think it's fine to navigate to SETTINGS_WALLET_DOMAIN_CARD in both cases, like we do in other scenarios. I'll revisit this if anyone disagrees
There was a problem hiding this comment.
I think it's fine too but unresolving so @joekaufmanexpensify can review
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@abzokhattab Let's ignore this PR |
|
@chuckdries Could you review this PR? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e82463ac88
ℹ️ 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".
|
Apply translation diff will resolve all failed checks |
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index c77a6ba1..da1c4b30 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -694,6 +694,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'Wir haben Ihre Karte nicht versendet. Bitte versuchen Sie es erneut.'},
revealPin: {couldNotReveal: 'Wir konnten Ihre PIN nicht anzeigen. Bitte versuchen Sie es erneut.'},
changePin: {didNotChange: 'Wir haben Ihre PIN nicht geändert. Bitte versuchen Sie es erneut.'},
+ revealCardDetail: {couldNotReveal: 'Wir konnten Ihre Kartendaten nicht anzeigen. Bitte versuchen Sie es noch einmal.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/es.ts b/src/languages/es.ts
index cfcfe238..b857d546 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -652,6 +652,7 @@ const translations: TranslationDeepObject<typeof en> = {
changePin: {
didNotChange: 'No pudimos cambiar tu PIN. Por favor, inténtalo de nuevo.',
},
+ revealCardDetail: {couldNotReveal: 'No hemos podido mostrar los datos de tu tarjeta. Por favor, inténtalo de nuevo.'},
},
validateCodeModal: {
successfulSignInTitle: 'Abracadabra,\n¡sesión iniciada!',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index f217296b..b31fd42e 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -696,6 +696,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'Nous n’avons pas envoyé votre carte. Veuillez réessayer.'},
revealPin: {couldNotReveal: "Nous n'avons pas pu afficher votre code PIN. Veuillez réessayer."},
changePin: {didNotChange: "Nous n'avons pas modifié votre code PIN. Veuillez réessayer."},
+ revealCardDetail: {couldNotReveal: 'Nous n’avons pas pu afficher les détails de votre carte. Veuillez réessayer.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/it.ts b/src/languages/it.ts
index 04714349..2eaf05bb 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -695,6 +695,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'Non abbiamo spedito la tua carta. Riprova.'},
revealPin: {couldNotReveal: 'Non siamo riusciti a mostrare il tuo PIN. Riprova.'},
changePin: {didNotChange: 'Non abbiamo modificato il tuo PIN. Riprova.'},
+ revealCardDetail: {couldNotReveal: 'Non siamo riusciti a mostrare i dettagli della tua carta. Riprova.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index c6ee83df..281c3b62 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -683,6 +683,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'カードを発送できませんでした。もう一度お試しください。'},
revealPin: {couldNotReveal: 'PIN を表示できませんでした。もう一度お試しください。'},
changePin: {didNotChange: 'お客様の暗証番号は変更されていません。もう一度お試しください。'},
+ revealCardDetail: {couldNotReveal: 'カード情報を表示できませんでした。もう一度お試しください。'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index 891386fd..3159959a 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -693,6 +693,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'We hebben je kaart niet verzonden. Probeer het opnieuw.'},
revealPin: {couldNotReveal: 'We konden je pincode niet tonen. Probeer het opnieuw.'},
changePin: {didNotChange: 'We hebben je pincode niet gewijzigd. Probeer het opnieuw.'},
+ revealCardDetail: {couldNotReveal: 'We konden je kaartgegevens niet weergeven. Probeer het opnieuw.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 91459e08..68c68fb0 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -694,6 +694,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'Nie wysłaliśmy twojej karty. Spróbuj ponownie.'},
revealPin: {couldNotReveal: 'Nie udało się wyświetlić twojego PIN-u. Spróbuj ponownie.'},
changePin: {didNotChange: 'Nie zmieniliśmy twojego PIN-u. Spróbuj ponownie.'},
+ revealCardDetail: {couldNotReveal: 'Nie udało się wyświetlić szczegółów Twojej karty. Spróbuj ponownie.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index 535659f2..ecfa4dfe 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -692,6 +692,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: 'Não enviamos seu cartão. Tente novamente.'},
revealPin: {couldNotReveal: 'Não foi possível revelar seu PIN. Tente novamente.'},
changePin: {didNotChange: 'Nós não alteramos seu PIN. Tente novamente.'},
+ revealCardDetail: {couldNotReveal: 'Não foi possível exibir os detalhes do seu cartão. Tente novamente.'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 3c9e8607..e8a84a0b 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -673,6 +673,7 @@ const translations: TranslationDeepObject<typeof en> = {
setPin: {didNotShipCard: '我们未能寄出您的卡。请重试。'},
revealPin: {couldNotReveal: '我们无法显示您的 PIN。请重试。'},
changePin: {didNotChange: '我们未更改您的 PIN。请重试。'},
+ revealCardDetail: {couldNotReveal: '我们无法显示您的卡片详情。请重试。'},
},
validateCodeModal: {
successfulSignInTitle: dedent(`
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
rafecolton
left a comment
There was a problem hiding this comment.
Only thing I notice is that when authentication fails and I tap Got it, it closes the RHP, which navigates the user away from the card details. Would it be possible to have the user return to the card details page instead?
NAB though. Assigning a few other reviewers
| const cvv = typeof callbackInput.body?.cvv === 'string' ? callbackInput.body.cvv : ''; | ||
| setRevealedCardDetails(payload.cardID, {pan, expiration, cvv}); | ||
| Navigation.closeRHPFlow(); | ||
| Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAIN_CARD.getRoute(String(payload.cardID))); |
There was a problem hiding this comment.
I think it's fine too but unresolving so @joekaufmanexpensify can review
|
@abzokhattab ready for you to complete the reviewer checklist 😊 |
This will probably be fixed when we rebase #89992 |
|
I'll take care of the checklist - I don't think @abzokhattab has access to UK E-cards and I still haven't gotten a sandbox workspace working in staging yet |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppMFA.for.reveal.card.details.android.mp4Android: mWeb ChromeMFA.for.reveal.card.details.android.chrome.mp4iOS: HybridAppMFA.for.reveal.card.details.ios.mp4iOS: mWeb SafariMFA.for.reveal.card.details.ios.safari.mp4MacOS: Chrome / SafariMFA.for.reveal.card.details.macos.chrome.mp4 |
|
🚧 @rafecolton has triggered a test Expensify/App build. You can view the workflow run here. |
|
Looking good! I agree with Rafe's comment about the RHP, but NAB. |
This comment has been minimized.
This comment has been minimized.
|
Changes are generally working well for me. One thing I did notice testing on web is the screen "flashes" a bit when transitioning from the passkey authentication modal to showing the virtual card details with UK/EU on the adhoc. It's not really an issue per se, but it's definitely noticeable if you test it right after revealing card details on a US virtual card. Is anyone else seeing this? |
I didn't actually notice this when I was testing yesterday, but looking back at the videos I recorded I do see it. |
|
Not a huge deal, but if we can easily fix it, that feels ideal IMO. |
|
If nobody objects by 11am PST, I am going to merge this PR despite the NABs. It's going to cause conflicts on #89992, but that PR totally changes the structure of MFA navigation in a way that will change how both of the NABs need to be handled, and this PR is further along than that one so I think it makes sense to merge first. |
|
@joekaufmanexpensify I didn't notice this problem. Could you please clarify it? It would be helpful if you could point it out in the video. From my initial assessment, I think this flaky problem might be resolved by #89992 |
|
@DylanDylann it's visible in the macos chrome video in my review checklist. The whole page flashes just before the MFA prompt page starts to animate away after the passkey auth succeeds. I've recorded video of me scrubbing through the screen recording to try to show it better mfa.for.reveal.card.details.flash.mp4 |
|
I am hesitant to post a video from my staging account because it contains real cards. What I am describing is visible in the web video you posted screenshots/videos section of the PR. Right after authenticating via passkeys the entire screen flashes as the modal is closing. LMK if that helps! |
|
Ah, we're seeing a flash on the background. I'm not 100% sure about the root cause, but I think it's caused by triggering the RHP modal dismissal right after navigating to a new screen. This navigation logic will change after the MFA navigation refactor. Should we move forward here and revisit this in the MFA navigation PR? |
|
🚧 @chuckdries 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/chuckdries in version: 9.3.80-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the PR changes and the existing help site documentation. Finding: No help site changes are required. The relevant article (Cardholder Settings and Features) describes the "View your virtual card details" flow as:
This PR changes the backend authentication mechanism for EU/UK cards (magic code → biometric MFA), but the user-visible steps, button labels, and navigation paths remain the same. The existing docs don't mention the specific authentication method, so they remain accurate. |
|
@joekaufmanexpensify tagging you as per the QA step. |
|
@kavimuru this one passes QA ✅ |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
Explanation of Change
Fixed Issues
$ #91028
PROPOSAL:
Tests
Offline tests
QA Steps
Assign to @joekaufmanexpensify for 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
Screen.Recording.2026-05-21.at.16.04.36.mov
Screen.Recording.2026-05-21.at.16.01.26.mov
Screen.Recording.2026-05-21.at.16.01.26.mov