Skip to content

[HOLD] Enhance MFA multi-device testing on troubleshoot and revoke pages#83488

Open
rafecolton wants to merge 31 commits intomainfrom
enhance-troubleshoot-revoke
Open

[HOLD] Enhance MFA multi-device testing on troubleshoot and revoke pages#83488
rafecolton wants to merge 31 commits intomainfrom
enhance-troubleshoot-revoke

Conversation

@rafecolton
Copy link
Member

@rafecolton rafecolton commented Feb 25, 2026

Hold for https://github.com/Expensify/Web-Expensify/pull/50984 and https://github.com/Expensify/Auth/pull/20130

Explanation of Change

Enhance troubleshooting and device revocation page to:

  1. Make it more clear whether the current device is registered
  2. Revoke on the Troubleshoot page now only revokes the current device
  3. On the device revocation page, you can revoke the current device, other devices, or all devices (whichever are eligible)

Fixed Issues

$ #83482
PROPOSAL:

Tests

  1. Open android, iOS, and web, all on the same account
  2. Visit the Troubleshoot page on each
  3. Use Test to register the iOS device - observe the text go from Never registered to Another device registered on android and web.
  4. Register android, observe the status on web go to Other devices registered
  5. Revoke iOS and observe Android is not revoked.
  6. Navigate to Security > Face/Fingerprint & Passkey on android, observe a row for "This device" and the button says "Revoke access"
  7. Re-register iOS. Notice a "One other device" row appear on android.
  8. On web, navigate to Security > Face/Fingerprint & Passkey, observe one row with "Two other devices"
  9. Navigate to "Security > Face/Fingerprint & Passkey" on iOS and observe two rows - one other device, one this device
  10. Revoke one other device from iOS. Notice web changes to "One other device" and android's "This device" row disappears
  11. Tap "Revoke access" on web and notice iOS now disappears too
  • Verify that no errors appear in the JS console

Offline tests

N/A, online only

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I verified that similar component doesn't exist in the codebase
  • I verified that all props are defined accurately and each prop has a /** comment above it */
  • I verified that each file is named correctly
  • I verified that each component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • I verified that the only data being stored in component state is data necessary for rendering and nothing else
  • In component if we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • I verified that component internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • I verified that all JSX used for rendering exists in the render method
  • I verified that each component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

Screenshots/Videos

It's too big to store on GH: https://drive.google.com/file/d/17QiCyftmHOLrVXoNlMwvJgL9ErPuEQow/view?usp=sharing

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@rafecolton rafecolton self-assigned this Feb 25, 2026
@OSBotify

This comment has been minimized.

@rafecolton rafecolton force-pushed the enhance-troubleshoot-revoke branch from 92072d0 to 2b9ea00 Compare February 25, 2026 20:57
@rafecolton rafecolton closed this Feb 27, 2026
@rafecolton rafecolton force-pushed the enhance-troubleshoot-revoke branch from 2b9ea00 to 5023f38 Compare February 27, 2026 23:47
@rafecolton rafecolton reopened this Feb 28, 2026
@OSBotify

This comment has been minimized.

@rafecolton rafecolton changed the title Enhance MFA multi-device testing on troubleshoot and revoke pages [HOLD] Enhance MFA multi-device testing on troubleshoot and revoke pages Feb 28, 2026
@OSBotify

This comment has been minimized.

@rafecolton rafecolton requested review from a team and chuckdries February 28, 2026 04:09
@rafecolton
Copy link
Member Author

rafecolton commented Feb 28, 2026

@chuckdries @Expensify/design @DylanDylann, I know there are some problems I need to fix with with CI, but could I please get a preliminary review? Many thanks!

The user-facing UI changes are primarily to the Face/Fingerprint & Passkeys page. You can see them in the video. I didn't put them in Figma at all, LMK if you think I should.

Copy link
Member Author

@rafecolton rafecolton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MelvinBot please fix the linter error and add/update the missing types to fix the type check error. After that, please add any missing translation updates - in particular, the Spanish file was not updated.

Once you've done that, please post a comment showing the git diff between this branch and main of only the English and Spanish transaction files. Be sure to put it in a code block so I can easily copy / paste

- Fix ESLint prefer-at error in en.ts (use .at() instead of bracket notation)
- Fix type error in API types by using RevokeMultifactorAuthenticationCredentialsParams
- Add missing translation keys to all non-English language files:
  - biometricsTest: statusNotRegistered, statusRegisteredOtherDevice, statusRegisteredThisDevice
  - revoke: thisDevice, otherDevices
- Add missing Spanish translations: statusNeverRegistered, confirmationPromptAll, ctaAll
- Fix Spanish troubleshootBiometricsStatus to use status param instead of registered

Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
@MelvinBot
Copy link
Contributor

Fixed the following issues and pushed a commit:

1. ESLint error (src/languages/en.ts:751): Changed numberWords[count - 1] to numberWords.at(count - 1) to satisfy the rulesdir/prefer-at lint rule.

2. TypeScript type error (src/libs/API/types.ts:1387): Changed the REVOKE_MULTIFACTOR_AUTHENTICATION_CREDENTIALS command type from EmptyObject to Parameters.RevokeMultifactorAuthenticationCredentialsParams so the onlyKeyID/exceptKeyID params are accepted.

3. Missing translations — Added missing keys to all 9 non-English language files:

  • biometricsTest: statusNotRegistered, statusRegisteredOtherDevice, statusRegisteredThisDevice
  • revoke: thisDevice, otherDevices
  • Spanish additionally needed: statusNeverRegistered, confirmationPromptAll, ctaAll, and the troubleshootBiometricsStatus signature was updated from ({registered}) to ({status}: MultifactorAuthenticationTranslationParams)

Here's the git diff between this branch and main for the English and Spanish translation files:

diff --git a/src/languages/en.ts b/src/languages/en.ts
index 6afd64b..092a544 100644
--- a/src/languages/en.ts
+++ b/src/languages/en.ts
@@ -695,7 +695,11 @@ const translations = {
             biometricsTest: 'Biometrics test',
             authenticationSuccessful: 'Authentication successful',
             successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `You've successfully authenticated using ${authType}.`,
-            troubleshootBiometricsStatus: ({registered}: MultifactorAuthenticationTranslationParams) => `Biometrics (${registered ? 'Registered' : 'Not registered'})`,
+            troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `Biometrics (${status})`,
+            statusNeverRegistered: 'Never registered',
+            statusNotRegistered: 'Not registered',
+            statusRegisteredOtherDevice: ({count}: MultifactorAuthenticationTranslationParams) => (count === 1 ? 'Another device registered' : 'Other devices registered'),
+            statusRegisteredThisDevice: 'Registered',
             yourAttemptWasUnsuccessful: 'Your authentication attempt was unsuccessful.',
             youCouldNotBeAuthenticated: 'You couldn't be authenticated',
             areYouSureToReject: 'Are you sure? The authentication attempt will be rejected if you close this screen.',
@@ -733,12 +737,20 @@ const translations = {
         revoke: {
             revoke: 'Revoke',
             title: 'Face/fingerprint & passkeys',
-            explanation: 'Face/fingerprint or passkey verification are enabled on one or more devices. Revoking access will require a magic code for the next verification on any device.',
-            confirmationPrompt: "Are you sure? You'll need a magic code for the next verification on any device.",
+            explanation: 'Face/fingerprint or passkey verification are enabled on one or more devices. Revoking access will require a magic code for the next verification on that device.',
+            confirmationPrompt: "Are you sure? You'll need a magic code for the next verification on that device.",
+            confirmationPromptAll: "Are you sure? You'll need a magic code for the next verification on any device.",
             cta: 'Revoke access',
+            ctaAll: 'Revoke all',
             noDevices: "You don't have any devices registered for face/fingerprint or passkey verification. If you register any, you will be able to revoke that access here.",
             dismiss: 'Got it',
             error: 'Request failed. Try again later.',
+            thisDevice: 'This device',
+            otherDevices: ({count}: MultifactorAuthenticationTranslationParams) => {
+                const numberWords = ['One', 'Two', 'Three', 'Four', 'Five', 'Six', 'Seven', 'Eight', 'Nine'];
+                const displayCount = count !== undefined && count >= 1 && count <= 9 ? numberWords.at(count - 1) : `${count}`;
+                return `${displayCount} other ${count === 1 ? 'device' : 'devices'}`;
+            },
         },
     },
     validateCodeModal: {
diff --git a/src/languages/es.ts b/src/languages/es.ts
index 42f18a5..83ad24b 100644
--- a/src/languages/es.ts
+++ b/src/languages/es.ts
@@ -3,7 +3,14 @@ import dedent from '@libs/StringUtils/dedent';
 import CONST from '@src/CONST';
 import type {OriginalMessageSettlementAccountLocked, PersonalRulesModifiedFields, PolicyRulesModifiedFields} from '@src/types/onyx/OriginalMessage';
 import type en from './en';
-import type {ConciergeBrokenCardConnectionParams, CreatedReportForUnapprovedTransactionsParams, PaidElsewhereParams, UpdatedPolicyTagParams, ViolationsRterParams} from './params';
+import type {
+    ConciergeBrokenCardConnectionParams,
+    CreatedReportForUnapprovedTransactionsParams,
+    MultifactorAuthenticationTranslationParams,
+    PaidElsewhereParams,
+    UpdatedPolicyTagParams,
+    ViolationsRterParams,
+} from './params';
 import type {TranslationDeepObject} from './types';

 /* eslint-disable max-len */
@@ -538,8 +545,12 @@ const translations: TranslationDeepObject<typeof en> = {
         biometricsTest: {
             biometricsTest: 'Prueba de biometría',
             authenticationSuccessful: 'Autenticación exitosa',
-            successfullyAuthenticatedUsing: ({authType}) => `Te has autenticado exitosamente con ${authType}.`,
-            troubleshootBiometricsStatus: ({registered}) => `Biometría (${registered ? 'Registrada' : 'No registrada'})`,
+            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `Te has autenticado exitosamente con ${authType}.`,
+            troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `Biometría (${status})`,
+            statusNeverRegistered: 'Nunca registrado',
+            statusNotRegistered: 'No registrado',
+            statusRegisteredOtherDevice: ({count}: MultifactorAuthenticationTranslationParams) => (count === 1 ? 'Otro dispositivo registrado' : 'Otros dispositivos registrados'),
+            statusRegisteredThisDevice: 'Registrado',
             yourAttemptWasUnsuccessful: 'Tu intento de autenticación fue fallido.',
             youCouldNotBeAuthenticated: 'No se pudo autenticar',
             areYouSureToReject: '¿Estás seguro? El intento de autenticación será rechazado si cierras esta pantalla.',
@@ -578,13 +589,21 @@ const translations: TranslationDeepObject<typeof en> = {
             revoke: 'Revocar',
             title: 'Reconocimiento facial/huella digital y claves de acceso',
             explanation:
-                'La verificación mediante reconocimiento facial, huella digital o clave de acceso está habilitada en uno o más dispositivos. Revocar el acceso requerirá un código mágico para la próxima verificación en cualquier dispositivo.',
-            confirmationPrompt: '¿Estás seguro? Necesitarás un código mágico para la próxima verificación en cualquier dispositivo.',
+                'La verificación mediante reconocimiento facial, huella digital o clave de acceso está habilitada en uno o más dispositivos. Revocar el acceso requerirá un código mágico para la próxima verificación en ese dispositivo.',
+            confirmationPrompt: '¿Estás seguro? Necesitarás un código mágico para la próxima verificación en ese dispositivo.',
+            confirmationPromptAll: '¿Estás seguro? Necesitarás un código mágico para la próxima verificación en cualquier dispositivo.',
             cta: 'Revocar acceso',
+            ctaAll: 'Revocar todo',
             noDevices:
                 'No tienes ningún dispositivo registrado para la verificación mediante reconocimiento facial, huella digital o clave de acceso. Si registras alguno, podrás revocar ese acceso aquí.',
             dismiss: 'Entendido',
             error: 'La solicitud ha fallado. Inténtalo de nuevo más tarde.',
+            thisDevice: 'Este dispositivo',
+            otherDevices: ({count}: MultifactorAuthenticationTranslationParams) => {
+                const numberWords = ['Un', 'Dos', 'Tres', 'Cuatro', 'Cinco', 'Seis', 'Siete', 'Ocho', 'Nueve'];
+                const displayCount = count !== undefined && count >= 1 && count <= 9 ? numberWords.at(count - 1) : `${count}`;
+                return `${displayCount} ${count === 1 ? 'otro dispositivo' : 'otros dispositivos'}`;
+            },
         },
     },
     validateCodeModal: {

@rafecolton
Copy link
Member Author

@MelvinBot please fix the CI errors

@codecov
Copy link

codecov bot commented Feb 28, 2026

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.

Files with missing lines Coverage Δ
...actorAuthentication/Context/useNativeBiometrics.ts 100.00% <100.00%> (ø)
src/libs/API/types.ts 100.00% <ø> (ø)
src/pages/MultifactorAuthentication/RevokePage.tsx 100.00% <100.00%> (+100.00%) ⬆️
src/components/TestToolMenu.tsx 87.50% <50.00%> (-0.38%) ⬇️
...rc/libs/actions/MultifactorAuthentication/index.ts 9.63% <0.00%> (ø)
src/libs/actions/User.ts 23.18% <0.00%> (-0.11%) ⬇️
src/hooks/useBiometricRegistrationStatus.ts 64.70% <64.70%> (ø)
... and 225 files with indirect coverage changes

@OSBotify

This comment has been minimized.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2026

🦜 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 diff
diff --git a/src/languages/de.ts b/src/languages/de.ts
index f4511088..9b839ae1 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -718,10 +718,11 @@ const translations: TranslationDeepObject<typeof en> = {
         revoke: {
             title: 'Gesicht/Fingerabdruck & Zugangsschlüssel',
             explanation:
-                'Gesichts-/Fingerabdruck- oder Passkey-Verifizierung ist auf einem oder mehreren Geräten aktiviert. Wenn Sie den Zugriff widerrufen, ist für die nächste Verifizierung auf diesem Gerät ein Magic Code erforderlich.',
-            confirmationPrompt: 'Sind Sie sicher? Sie benötigen einen magischen Code für die nächste Verifizierung auf diesem Gerät.',
+                'Gesichts-/Fingerabdruck- oder Passkey-Verifizierung ist auf einem oder mehreren Geräten aktiviert. Durch das Widerrufen des Zugriffs wird für die nächste Verifizierung auf diesem Gerät ein Magic Code erforderlich.',
+            confirmationPrompt: 'Sind Sie sicher? Sie benötigen einen Magic Code für die nächste Verifizierung auf diesem Gerät.',
             cta: 'Zugriff entziehen',
-            noDevices: 'Sie haben keine Geräte für Gesichts-/Fingerabdruck- oder Passkey-Verifizierung registriert. Wenn Sie welche registrieren, können Sie deren Zugriff hier widerrufen.',
+            noDevices:
+                'Sie haben keine Geräte für die Gesichts-/Fingerabdruck- oder Passkey-Verifizierung registriert. Wenn Sie welche registrieren, können Sie den Zugriff hier widerrufen.',
             dismiss: 'Verstanden',
             error: 'Anfrage fehlgeschlagen. Versuche es später noch einmal.',
             revoke: 'Widerrufen',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index f6058556..3fb998c1 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -694,8 +694,8 @@ const translations: TranslationDeepObject<typeof en> = {
                 touchId: 'Touch ID',
                 opticId: 'Optic ID',
             },
-            statusNeverRegistered: 'Jamais inscrit',
-            statusNotRegistered: 'Non enregistré',
+            statusNeverRegistered: 'Jamais enregistré',
+            statusNotRegistered: 'Non inscrit',
             statusRegisteredOtherDevice: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) =>
                 otherDeviceCount === 1 ? 'Un autre appareil enregistré' : 'Autres appareils enregistrés',
             statusRegisteredThisDevice: 'Enregistré',
@@ -720,8 +720,8 @@ const translations: TranslationDeepObject<typeof en> = {
         revoke: {
             title: 'Reconnaissance faciale/empreinte digitale et passkeys',
             explanation:
-                'La vérification par reconnaissance faciale/empreinte digitale ou par passkey est activée sur un ou plusieurs appareils. Révoquer l’accès nécessitera un code magique pour la prochaine vérification sur cet appareil.',
-            confirmationPrompt: 'Êtes-vous sûr ? Vous aurez besoin d’un code magique pour la prochaine vérification sur cet appareil.',
+                'La vérification par reconnaissance faciale/empreinte digitale ou par clé d’accès est activée sur un ou plusieurs appareils. Révoquer l’accès nécessitera un code magique pour la prochaine vérification sur cet appareil.',
+            confirmationPrompt: 'Voulez-vous continuer ? Vous aurez besoin d’un code magique pour la prochaine vérification sur cet appareil.',
             cta: 'Révoquer l’accès',
             noDevices:
                 'Vous n’avez enregistré aucun appareil pour la vérification par reconnaissance faciale/empreinte digitale ou par passkey. Si vous en enregistrez, vous pourrez révoquer cet accès ici.',
@@ -729,7 +729,7 @@ const translations: TranslationDeepObject<typeof en> = {
             error: 'La requête a échoué. Réessayez plus tard.',
             revoke: 'Révoquer',
             confirmationPromptAll: 'Êtes-vous sûr ? Vous aurez besoin d’un code magique pour la prochaine vérification sur n’importe quel appareil.',
-            ctaAll: 'Révoquer tout',
+            ctaAll: 'Tout révoquer',
             thisDevice: 'Cet appareil',
             otherDevices: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) => {
                 const numberWords = ['Un', 'Deux', 'Trois', 'Quatre', 'Cinq', 'Six', 'Sept', 'Huit', 'Neuf'];
@@ -737,7 +737,7 @@ const translations: TranslationDeepObject<typeof en> = {
                 return `${displayCount} ${otherDeviceCount === 1 ? 'autre appareil' : 'autres appareils'}`;
             },
             confirmationPromptThisDevice: 'Êtes-vous sûr·e ? Vous aurez besoin d’un code magique pour la prochaine vérification sur cet appareil.',
-            confirmationPromptMultiple: 'Êtes-vous sûr ? Vous aurez besoin d’un code magique pour la prochaine vérification sur ces appareils.',
+            confirmationPromptMultiple: 'Êtes-vous sûr(e) ? Vous aurez besoin d’un code magique pour la prochaine vérification sur ces appareils.',
         },
         unsupportedDevice: {
             unsupportedDevice: 'Appareil non pris en charge',
diff --git a/src/languages/it.ts b/src/languages/it.ts
index c1dede48..4d59c705 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -675,7 +675,7 @@ const translations: TranslationDeepObject<typeof en> = {
         biometricsTest: {
             biometricsTest: 'Test biometrico',
             authenticationSuccessful: 'Autenticazione riuscita',
-            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `Hai effettuato l’autenticazione con successo usando ${authType}.`,
+            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `Autenticazione eseguita correttamente tramite ${authType}.`,
             troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `Dati biometrici (${status})`,
             yourAttemptWasUnsuccessful: 'Il tentativo di autenticazione non è andato a buon fine.',
             youCouldNotBeAuthenticated: 'Autenticazione non riuscita',
@@ -716,14 +716,14 @@ const translations: TranslationDeepObject<typeof en> = {
             biometrics: 'Attiva una verifica rapida e sicura utilizzando il volto o l’impronta digitale. Nessuna password o codice richiesto.',
         },
         revoke: {
-            title: 'Volto/impronta digitale e passkey',
+            title: 'Riconoscimento facciale/impronta digitale e passkey',
             explanation:
-                'La verifica con volto/impronta digitale o passkey è abilitata su uno o più dispositivi. Revocare l’accesso richiederà un codice magico per la prossima verifica su quel dispositivo.',
+                'La verifica con volto/impronta digitale o passkey è attivata su uno o più dispositivi. Revocare l’accesso richiederà un codice magico per la prossima verifica su quel dispositivo.',
             confirmationPrompt: 'Sei sicuro? Ti servirà un codice magico per la prossima verifica su quel dispositivo.',
             cta: 'Revoca accesso',
             noDevices:
-                'Non hai alcun dispositivo registrato per il riconoscimento facciale/impronta digitale o la verifica tramite passkey. Se ne registri qualcuno, potrai revocare qui tale accesso.',
-            dismiss: 'Ricevuto',
+                'Non hai nessun dispositivo registrato per il riconoscimento facciale/impronta digitale o la verifica con passkey. Se ne registri qualcuno, potrai revocare l’accesso da qui.',
+            dismiss: 'Fatto',
             error: 'Richiesta non riuscita. Riprova più tardi.',
             revoke: 'Revoca',
             confirmationPromptAll: 'Sei sicuro? Ti servirà un codice magico per la prossima verifica su qualsiasi dispositivo.',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index e2722f8a..62da5af8 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -674,7 +674,7 @@ const translations: TranslationDeepObject<typeof en> = {
         biometricsTest: {
             biometricsTest: '生体認証テスト',
             authenticationSuccessful: '認証に成功しました',
-            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `${authType} を使用して正常に認証されました。`,
+            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `${authType} を使用して認証に成功しました。`,
             troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `生体認証(${status})`,
             yourAttemptWasUnsuccessful: '認証を試みましたが、成功しませんでした。',
             youCouldNotBeAuthenticated: '認証できませんでした',
@@ -714,20 +714,20 @@ const translations: TranslationDeepObject<typeof en> = {
             biometrics: '顔や指紋を使って、素早く安全に認証できます。パスワードやコードは不要です。',
         },
         revoke: {
-            title: '顔/指紋 & パスキー',
-            explanation: '1 台以上のデバイスで顔/指紋またはパスキーによる認証が有効になっています。アクセスを取り消すと、そのデバイスで次回認証を行う際にマジックコードが必要になります。',
-            confirmationPrompt: '本当に実行してもよろしいですか?次回そのデバイスで認証するには、マジックコードが必要になります。',
+            title: '顔/指紋認証とパスキー',
+            explanation: '1 台以上のデバイスで顔/指紋認証またはパスキー認証が有効になっています。アクセスを取り消すと、そのデバイスで次回認証する際にマジックコードが必要になります。',
+            confirmationPrompt: '本当によろしいですか?そのデバイスで次回の認証を行うには、マジックコードが必要になります。',
             cta: 'アクセスを取り消す',
-            noDevices: '顔認証・指紋認証またはパスキー認証に登録されているデバイスがありません。デバイスを登録すると、そのアクセス権をここで取り消すことができるようになります。',
+            noDevices: '顔認証・指紋認証またはパスキー認証用の端末が登録されていません。端末を登録すると、そのアクセス権をここで取り消せるようになります。',
             dismiss: '了解しました',
             error: 'リクエストに失敗しました。後でもう一度お試しください。',
             revoke: '取り消す',
-            confirmationPromptAll: '本当によろしいですか?今後どの端末でも次回の認証にはマジックコードが必要になります。',
+            confirmationPromptAll: '本当によろしいですか?今後、どのデバイスでも次回の認証にはマジックコードが必要になります。',
             ctaAll: 'すべて取り消す',
             thisDevice: 'このデバイス',
             otherDevices: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) => `他${otherDeviceCount}台のデバイス`,
             confirmationPromptThisDevice: '本当によろしいですか?このデバイスで次回の認証を行うには、マジックコードが必要になります。',
-            confirmationPromptMultiple: '本当に宜しいですか?そのデバイスで次回の認証を行うには、マジックコードが必要になります。',
+            confirmationPromptMultiple: 'よろしいですか?そのデバイスで次回の認証を行うには、マジックコードが必要になります。',
         },
         unsupportedDevice: {
             unsupportedDevice: '未対応のデバイス',
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index d807b2c6..3fdc2d9d 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -715,12 +715,12 @@ const translations: TranslationDeepObject<typeof en> = {
             biometrics: 'Schakel snelle, veilige verificatie in met je gezicht of vingerafdruk. Geen wachtwoorden of codes nodig.',
         },
         revoke: {
-            title: 'Gezicht/vingerafdruk & passkeys',
+            title: 'Gezichtsherkenning/vingerafdruk & passkeys',
             explanation:
-                'Gezichts-/vingerafdruk- of passkeys-verificatie is ingeschakeld op een of meer apparaten. Toegang intrekken betekent dat er een magische code nodig is voor de volgende verificatie op dat apparaat.',
+                'Gezichts-/vingerafdruk- of toegangssleutelverificatie is ingeschakeld op één of meer apparaten. Toegang intrekken betekent dat er bij de volgende verificatie op dat apparaat een magische code nodig is.',
             confirmationPrompt: 'Weet je het zeker? Je hebt een magische code nodig voor de volgende verificatie op dat apparaat.',
             cta: 'Toegang intrekken',
-            noDevices: 'Je hebt geen apparaten geregistreerd voor gezichts-/vingerafdruk- of passkeys-verificatie. Als je er een registreert, kun je die toegang hier intrekken.',
+            noDevices: 'Je hebt geen apparaten geregistreerd voor gezichts-/vingerafdruk- of passkey-verificatie. Als je er een registreert, kun je hier die toegang intrekken.',
             dismiss: 'Begrepen',
             error: 'Aanvraag mislukt. Probeer het later opnieuw.',
             revoke: 'Intrekken',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index d907b33b..a5fc452c 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -673,7 +673,7 @@ const translations: TranslationDeepObject<typeof en> = {
         },
         biometricsTest: {
             biometricsTest: 'Test biometrii',
-            authenticationSuccessful: 'Uwierzytelnianie powiodło się',
+            authenticationSuccessful: 'Uwierzytelnianie zakończone powodzeniem',
             successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `Pomyślnie uwierzytelniłeś się za pomocą ${authType}.`,
             troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `Dane biometryczne (${status})`,
             yourAttemptWasUnsuccessful: 'Twoja próba uwierzytelnienia nie powiodła się.',
@@ -715,16 +715,16 @@ const translations: TranslationDeepObject<typeof en> = {
             biometrics: 'Włącz szybką i bezpieczną weryfikację za pomocą twarzy lub odcisku palca. Bez haseł i kodów.',
         },
         revoke: {
-            title: 'Face/odcisk palca i klucze dostępu',
+            title: 'Twarz/odcisk palca i klucze dostępu',
             explanation:
-                'Weryfikacja twarzą/odciskiem palca lub kluczem dostępu jest włączona na jednym lub więcej urządzeniach. Cofnięcie dostępu spowoduje, że przy następnej weryfikacji na tym urządzeniu będzie wymagany magiczny kod.',
+                'Na jednym lub więcej urządzeniach włączono weryfikację twarzą/odciskiem palca lub kluczem dostępu. Cofnięcie dostępu spowoduje, że przy następnej weryfikacji na tym urządzeniu będzie wymagany magiczny kod.',
             confirmationPrompt: 'Na pewno? Będziesz potrzebować magicznego kodu przy następnej weryfikacji na tym urządzeniu.',
             cta: 'Cofnij dostęp',
             noDevices:
                 'Nie masz żadnych urządzeń zarejestrowanych do weryfikacji twarzą/odciskiem palca ani kluczem dostępu. Jeśli jakieś zarejestrujesz, będziesz mógł tutaj cofnąć ten dostęp.',
-            dismiss: 'Jasne',
+            dismiss: 'Rozumiem',
             error: 'Żądanie nie powiodło się. Spróbuj ponownie później.',
-            revoke: 'Unieważnij',
+            revoke: 'Odwołaj',
             confirmationPromptAll: 'Na pewno? Będziesz potrzebować magicznego kodu przy następnej weryfikacji na dowolnym urządzeniu.',
             ctaAll: 'Odwołaj wszystkie',
             thisDevice: 'To urządzenie',
@@ -733,7 +733,7 @@ const translations: TranslationDeepObject<typeof en> = {
                 const displayCount = otherDeviceCount !== undefined && otherDeviceCount >= 1 && otherDeviceCount <= 9 ? numberWords.at(otherDeviceCount - 1) : `${otherDeviceCount}`;
                 return `${displayCount} ${otherDeviceCount === 1 ? 'inne urządzenie' : 'innych urządzeń'}`;
             },
-            confirmationPromptThisDevice: 'Na pewno? Do następnej weryfikacji na tym urządzeniu będziesz potrzebować magicznego kodu.',
+            confirmationPromptThisDevice: 'Na pewno? Będziesz potrzebować magicznego kodu przy następnej weryfikacji na tym urządzeniu.',
             confirmationPromptMultiple: 'Na pewno? Będziesz potrzebować magicznego kodu przy następnej weryfikacji na tych urządzeniach.',
         },
         unsupportedDevice: {
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index 260b9f77..21b45469 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -714,9 +714,9 @@ const translations: TranslationDeepObject<typeof en> = {
             biometrics: 'Ative uma verificação rápida e segura usando seu rosto ou impressão digital. Nenhuma senha ou código é necessário.',
         },
         revoke: {
-            title: 'Reconhecimento facial/digital e passkeys',
+            title: 'Rosto/digital & passkeys',
             explanation:
-                'A verificação por rosto/digital ou chave de acesso está ativada em um ou mais dispositivos. Revogar o acesso exigirá um código mágico na próxima verificação nesse dispositivo.',
+                'A verificação por rosto/digital ou chave de acesso está ativada em um ou mais dispositivos. Revogar o acesso vai exigir um código mágico na próxima verificação nesse dispositivo.',
             confirmationPrompt: 'Tem certeza? Você vai precisar de um código mágico para a próxima verificação nesse dispositivo.',
             cta: 'Revogar acesso',
             noDevices: 'Você não tem nenhum dispositivo registrado para verificação por rosto/digital ou passkey. Se registrar algum, você poderá revogar esse acesso aqui.',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 429b30bc..945a3ffa 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -666,8 +666,8 @@ const translations: TranslationDeepObject<typeof en> = {
         },
         biometricsTest: {
             biometricsTest: '生物识别测试',
-            authenticationSuccessful: '认证成功',
-            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `您已成功使用 ${authType} 完成验证。`,
+            authenticationSuccessful: '身份验证成功',
+            successfullyAuthenticatedUsing: ({authType}: MultifactorAuthenticationTranslationParams) => `您已成功通过 ${authType} 完成验证。`,
             troubleshootBiometricsStatus: ({status}: MultifactorAuthenticationTranslationParams) => `生物识别(${status})`,
             yourAttemptWasUnsuccessful: '您的身份验证尝试未成功。',
             youCouldNotBeAuthenticated: '无法验证您的身份',
@@ -708,19 +708,19 @@ const translations: TranslationDeepObject<typeof en> = {
         },
         revoke: {
             title: '面容/指纹和通行密钥',
-            explanation: '一台或多台设备已启用面部/指纹或通行密钥验证。撤销访问后,该设备下次验证时将需要输入魔法验证码。',
-            confirmationPrompt: '你确定吗?下次在该设备上进行验证时,你需要一条魔法验证码。',
+            explanation: '已在一台或多台设备上启用了人脸/指纹或通行密钥验证。撤销访问权限后,该设备下次验证时将需要魔法验证码。',
+            confirmationPrompt: '确定吗?下次在该设备上进行验证时,你需要输入魔法代码。',
             cta: '撤销访问权限',
-            noDevices: '你还没有注册用于人脸 / 指纹或通行密钥验证的任何设备。如果你注册了设备,就可以在这里撤销其访问权限。',
-            dismiss: '知道了',
+            noDevices: '你尚未注册用于面部/指纹或通行密钥验证的任何设备。注册后,你可以在此撤销这些设备的访问权限。',
+            dismiss: '明白了',
             error: '请求失败。请稍后重试。',
             revoke: '撤销',
-            confirmationPromptAll: '确定要这样吗?您在任何设备上进行下一次验证时都需要输入魔法验证码。',
+            confirmationPromptAll: '确定吗?下次在任何设备上进行验证时,你都需要输入魔法验证码。',
             ctaAll: '全部撤销',
             thisDevice: '此设备',
             otherDevices: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) => `其他${otherDeviceCount}台设备`,
-            confirmationPromptThisDevice: '确定吗?此设备下次验证时需要魔法验证码。',
-            confirmationPromptMultiple: '确定要这样做吗?接下来在这些设备上进行验证时,你需要使用魔法验证码。',
+            confirmationPromptThisDevice: '确定吗?此设备下次验证时你将需要一个魔法验证码。',
+            confirmationPromptMultiple: '你确定吗?在那些设备上进行下一次验证时,你需要输入魔法验证码。',
         },
         unsupportedDevice: {
             unsupportedDevice: '不支持的设备',

Note

You can apply these changes to your branch by copying the patch to your clipboard, then running pbpaste | git apply 😉

View workflow run

@rafecolton
Copy link
Member Author

@DylanDylann ready for another code review. Will let you know when the BE is deployed and it's ready for full e2e testing

@rafecolton
Copy link
Member Author

Adding Jakub and Dariusz too since I made some changes to a core file

Copy link
Contributor

@JakubKorytko JakubKorytko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the change, though I think something along (Registered + 1 other device) would be nice when you are registered on device A and B and you are seeing this on a device A

Comment on lines +55 to +60
const statusTextMap = {
[REGISTRATION_STATUS.NEVER_REGISTERED]: translate('multifactorAuthentication.biometricsTest.statusNeverRegistered'),
[REGISTRATION_STATUS.NOT_REGISTERED]: translate('multifactorAuthentication.biometricsTest.statusNotRegistered'),
[REGISTRATION_STATUS.REGISTERED_OTHER_DEVICE]: translate('multifactorAuthentication.biometricsTest.statusRegisteredOtherDevice', {otherDeviceCount}),
[REGISTRATION_STATUS.REGISTERED_THIS_DEVICE]: translate('multifactorAuthentication.biometricsTest.statusRegisteredThisDevice'),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the translation key to extract this from the component. We could put this above the component.

example:

const fieldNameTranslationKeys = {
legalFirstName: 'additionalDetailsStep.legalFirstNameLabel',
legalLastName: 'additionalDetailsStep.legalLastNameLabel',
addressStreet: 'common.personalAddress',
phoneNumber: 'common.phoneNumber',
dob: 'common.dob',
ssn: 'common.ssnLast4',
ssnFull9: 'common.ssnFull9',
} as const;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass otherDeviceCount into the translation, so we can’t move this outside the component.

}

async function revokeMultifactorAuthenticationCredentials() {
async function revokeMultifactorAuthenticationCredentials(params?: {onlyKeyID?: string; exceptKeyID?: string}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use RevokeMultifactorAuthenticationCredentialsParams type here instead of inline declaration

const revokeButtons = screen.getAllByText('multifactorAuthentication.revoke.revoke');
const thisDeviceButton = revokeButtons.at(0);
if (!thisDeviceButton) {
throw new Error('Expected "This device" revoke button to exist');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why throwing errors instead of failing test? We could even expect the button to exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI told me that expecting the button to exist would not satisfy the linter, and I believed it and did not check. I will try it though, I saw some other examples that did that. If that doesn't work, I'll try failing the test.

import {useEffect, useState} from 'react';
import useNativeBiometrics from '@components/MultifactorAuthentication/Context/useNativeBiometrics';

const REGISTRATION_STATUS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to VALUES please.

REGISTERED_THIS_DEVICE: 'this_device',
} as const;

type RegistrationStatus = (typeof REGISTRATION_STATUS)[keyof typeof REGISTRATION_STATUS];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ValueOf please

Comment on lines +38 to +59
useEffect(() => {
let cancelled = false;

(async () => {
try {
const key = await getLocalPublicKey();
if (cancelled) {
return;
}
setLocalPublicKey(key);
} catch {
if (cancelled) {
return;
}
setLocalPublicKey(undefined);
}
})();

return () => {
cancelled = true;
};
}, [getLocalPublicKey, serverKnownCredentialIDs]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps let's do it this way? no need to try-catch, it is done inside Keystore

Suggested change
useEffect(() => {
let cancelled = false;
(async () => {
try {
const key = await getLocalPublicKey();
if (cancelled) {
return;
}
setLocalPublicKey(key);
} catch {
if (cancelled) {
return;
}
setLocalPublicKey(undefined);
}
})();
return () => {
cancelled = true;
};
}, [getLocalPublicKey, serverKnownCredentialIDs]);
useEffect(() => {
let cancelled = false;
getLocalPublicKey().then((key) => {
if (cancelled) {
return;
}
setLocalPublicKey(key);
});
return () => {
cancelled = true;
};
}, [getLocalPublicKey, serverKnownCredentialIDs]);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I had it like that and I swear something complained that I didn't have a catch(). I asked AI which was more common, the anonymous async function or .then().catch() and it said the former. I don't have a strong preference though, I can update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafecolton You updated this based on the AI suggestion above. However, I think @JakubKorytko’s comment is a valid point, so I’ll apply that suggestion instead.

otherDevices: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) => {
const numberWords = ['Jedno', 'Dwa', 'Trzy', 'Cztery', 'Pięć', 'Sześć', 'Siedem', 'Osiem', 'Dziewięć'];
const displayCount = otherDeviceCount !== undefined && otherDeviceCount >= 1 && otherDeviceCount <= 9 ? numberWords.at(otherDeviceCount - 1) : `${otherDeviceCount}`;
return `${displayCount} ${otherDeviceCount === 1 ? 'inne urządzenie' : 'innych urządzeń'}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: The second one from the ternary innych urządzeń is wrong for the 3rd, 4th, 5th numbers - it's "inne urządzenia", and "innych urządzeń" is for the rest numbers except 1st

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you suggest changes please and I will apply them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakubKorytko Please suggest the change on the new PR: #84016

Comment on lines +122 to +127
const confirmPromptKeys = {
thisDevice: 'multifactorAuthentication.revoke.confirmationPromptThisDevice',
single: 'multifactorAuthentication.revoke.confirmationPrompt',
multiple: 'multifactorAuthentication.revoke.confirmationPromptMultiple',
all: 'multifactorAuthentication.revoke.confirmationPromptAll',
} as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this outside component.

Co-authored-by: Jakub Korytko <jakub@korytko.me>
Copy link
Contributor

@chuckdries chuckdries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly works, but let's talk loading states.

Problem: RevokePage's confirm modal doesn't show a loading indicator while the request is in flight.

current revoke page in main shows a loading indicator on the confirm modal
old.confirm.modal.has.loading.state.mp4
confirm modal on this branch does not show a loading state
confirm.modal.does.not.have.loading.prop.mp4

Solution: You could fix this by defining isConfirmLoading={isThisDeviceLoading || isOtherDevicesLoading} as a prop on the <ConfirmModal, but I'm not a huge fan of that idea. revokeMultifactorAuthenticationCredentials is still setting and updating the isLoading flag in the ACCOUNT Onyx key, and TestToolMenu's Revoke button is still using that value to show its loading spinner, so we're using a mixture of component state and global state to represent essentially the same information. I dislike this because it's misleading for future engineers reading the revokeMultifactorAuthenticationCredentials implementation. They might reasonably expect manipulating ACCOUNT isLoading in onyx to change the loading indicator behavior in RevokePage, but that will not be the case.

  1. we can leave it as-is (and implement isConfirmLoading as described above) if you think my qualm doesn't make sense
  2. if we truly need to disambiguate between isThisDeviceLoading and isOtherDevicesLoading, the easiest thing to do is probably to clean up these onyx datas in revokeMultifactorAuthenticationCredentials and add a useState to TestToolMenu for revoke button loading status
  3. Since the ConfirmModal remains visible and blocking the RevokePage for the duration of any request, the user cannot press the underlying buttons anyway, so I would argue that we do not need to show a loading spinner on those buttons at all, and we can rely on the account isLoading flag just in the ConfirmModal. This would allow you to clean up both state hooks and a bit of setting logic in RevokePage

}}
onCancel={hideConfirmModal}
shouldShowCancelButton
isConfirmLoading={isLoading}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to define this prop so that the confirm modal, which remains visible while the request is in-flight, shows a loading indicator.

const response = await makeRequestWithSideEffects(SIDE_EFFECT_REQUEST_COMMANDS.REVOKE_MULTIFACTOR_AUTHENTICATION_CREDENTIALS, params ?? {}, {
optimisticData,
successData,
failureData,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these optimistic, success, and failure datas are used to manipulate account.isLoading for the duration of this request. You're now storing loading state locally within the RevokePage component so they mean nothing there, but you're still relying on this state to show a spinner on TestToolMenu's Revoke button.

Comment on lines +701 to +702
statusRegisteredOtherDevice: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) =>
otherDeviceCount === 1 ? 'Another device registered' : 'Other devices registered',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using PluralForm from src/languages/types.ts for pluralization.

Suggested change
statusRegisteredOtherDevice: ({otherDeviceCount}: MultifactorAuthenticationTranslationParams) =>
otherDeviceCount === 1 ? 'Another device registered' : 'Other devices registered',
statusRegisteredOtherDevice: () => ({
one: 'Another device registered',
other: 'Other devices registered',
}),

Then you'll need to pass a count parameter rather than otherDeviceCount at the call site

@rafecolton
Copy link
Member Author

@DylanDylann is going to take this one over for a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants