"Verify this device" redesign #30596

uhoreg · 2025-08-19T18:03:47Z

Probably easiest to review commit-by-commit

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

uhoreg · 2025-08-20T15:16:52Z

res/css/views/auth/_CompleteSecurityBody.pcss

    color: $authpage-primary-color;
    background-color: $background;
    border-radius: 4px;
-    padding: 20px;
+    padding: 20px 20px 60px 20px;


For some reason that I haven't been able to figure out, it squishes the bottom of the dialog, so I had to add some extra padding here.

dosubot · 2025-08-20T15:49:17Z

Related Documentation

No published documentation to review for changes on this repository.
Write your first living document

^{How did I do? Any feedback?}

src/components/views/settings/encryption/ResetIdentityBody.tsx

t3chguy

Looks sane otherwise

t3chguy · 2025-08-20T15:52:29Z

src/components/structures/auth/SetupEncryptionBody.tsx

        });
    };

+    private onLogOutClick = (): void => {


for consistency, we seem to use logout as per https://dictionary.cambridge.org/dictionary/english/logout rather than log out so O should not be capitalised here

Not sure this is terribly important, but:

The handler should be named after the button that it handles. The button is labelled 'Sign out', so the handler should be onSignOutClick.

("Log out" is a verb; "logout" is a noun (the act of logging out). They are not interchangeable.)

I had considered calling it onSignOutClick, but decided to use "log out" for consistency, since that term seemed to be used more in the code. But there are places in the code that use "sign out", so maybe I will change it. (Though I think that there is a case to be made about using the same terminology everywhere in the code for the same function, independent of what it says in the UI. For example, it makes it easier to find places that deal with logging out by grepping for a single string.)

If I do change the function to onSignOutClick, should I also change the property name to allowSignOut rather than allowLogout?

Looking at this again, I think all the onFooClick methods (onUsePassphraseClick, onVerifiyClick, etc) should be renamed to match the name of the button that the user is actually clicking on, so yes it should be onSignOutClick.

In an ideal world the properties would also reflect the actual behaviour of the component but in that case I don't think it's so important and I wouldn't bother. The prop does need documentation though.

(In the case of the prop, the lower-case o in allowLogout or allowSignout rather than LogOut is fine: it is about whether to allow a logout to happen.)

richvdh · 2025-08-22T11:49:51Z

I started reviewing this but I'm finding it really hard to follow. It's definitely too big to review in one lump, but the commits don't seem to make much sense either.

Could you try restructuring it, into smaller commits, with more explanation about what each commit is doing and why?

richvdh · 2025-08-22T10:15:37Z

src/components/views/settings/encryption/ResetIdentityBody.tsx

@@ -124,5 +128,7 @@ function titleForVariant(variant: ResetIdentityBodyVariant): string {
            return _t("settings|encryption|advanced|breadcrumb_title_sync_failed");
        case "forgot":
            return _t("settings|encryption|advanced|breadcrumb_title_forgot");
+        case "cant_confirm":
+            return _t("settings|encryption|advanced|breadcrumb_tittle_cant_confirm");


We appear to be referencing a string here that is not defined.

src/i18n/strings/en_EN.json

richvdh · 2025-08-22T10:19:12Z

src/components/views/settings/encryption/ResetIdentityBody.tsx

+ * "cant_confirm" is shown when the device is unverified and has no way of
+ * obtaining the existing keys, and hence the identity needs to be reset to have
+ * a cross-signed device.


It might be clearer if this was called "no_verification_method" or something, to distinguish from the case where the user clicks the "Can't confirm?" button in the "Confirm your identity" dialog.

richvdh · 2025-08-22T10:28:21Z

src/components/structures/auth/SetupEncryptionBody.tsx

        });
    };

+    private onLogOutClick = (): void => {


Looking at this again, I think all the onFooClick methods (onUsePassphraseClick, onVerifiyClick, etc) should be renamed to match the name of the button that the user is actually clicking on, so yes it should be onSignOutClick.

In an ideal world the properties would also reflect the actual behaviour of the component but in that case I don't think it's so important and I wouldn't bother. The prop does need documentation though.

(In the case of the prop, the lower-case o in allowLogout or allowSignout rather than LogOut is fine: it is about whether to allow a logout to happen.)

src/components/structures/auth/SetupEncryptionBody.tsx

richvdh · 2025-08-22T11:21:09Z

src/components/structures/auth/SetupEncryptionBody.tsx

@@ -112,19 +116,23 @@ export default class SetupEncryptionBody extends React.Component<IProps, IState>
        store.returnAfterSkip();
    };

-    private onResetClick = (ev: ButtonEvent): void => {
-        ev.preventDefault();
+    private onResetClick = (): void => {


definitely think this should be called "onCantConfirmClick"

src/components/structures/auth/SetupEncryptionBody.tsx

res/css/structures/auth/_SetupEncryptionBody.pcss

richvdh · 2025-08-22T11:41:16Z

res/css/structures/auth/_SetupEncryptionBody.pcss

+.mx_EncryptionCard.mx_SetupEncryptionBody {
+    width: 600px;
 }


why does this only apply when mx_SetupEncryptionBody is inside an EncryptionCard and not otherwise?

Also, can you help me understand why this is moving from mx_CompleteSecurityBody to here? What is the relationship between mx_CompleteSecurityBody and mx_EncryptionCard.mx_SetupEncryptionBody ?

Hmm. I'm not sure why I have the .mx_EncryptionCard in there. It looks like it isn't needed -- mx_SetupEncryptionBody isn't used anywhere else.

Also, can you help me understand why this is moving from mx_CompleteSecurityBody to here? What is the relationship between mx_CompleteSecurityBody and mx_EncryptionCard.mx_SetupEncryptionBody ?

mx_CompleteSecurityBody is only used when the user initially logs in. When SetupEncryptionBody is used as a modal inside the app, it's just inside of an .mx_Dialog (and associated things). So moving the style here means that it gets used both on initial login and within the app.

res/css/views/settings/encryption/_EncryptionCard.pcss

and update wording

uhoreg · 2025-08-28T00:52:49Z

I've reworked the commits, to break it up into smaller chunks, and addressed the comments. Some of the style changes are still grouped together, but hopefully it's more understandable now.

richvdh

much easier to review now, thank you for rearranging it.

Generally lgtm, a few nits and questions

richvdh · 2025-08-28T11:31:44Z

src/i18n/strings/en_EN.json

@@ -1064,7 +1066,7 @@
            "unverified_sessions_toast_description": "Review to ensure your account is safe",
            "unverified_sessions_toast_reject": "Later",
            "unverified_sessions_toast_title": "You have unverified sessions",
-            "verification_description": "Verify your identity to access encrypted messages and prove your identity to others. If you also use a mobile device, please open the app there before you proceed.",
+            "verification_description": "Verify this device to set up secure messaging",


Apparently we're not allowed to update existing strings outside of localazy (https://github.com/element-hq/element-web/blob/develop/docs/translating-dev.md#editing-existing-strings).

In order to keep the wording changes and the other design changes together, I suggest using a new translation key.

I changed it to use new translation keys.

richvdh · 2025-08-28T11:46:17Z

src/i18n/strings/en_EN.json

@@ -967,7 +967,6 @@
            "title": "Recovery Method Removed",
            "warning": "If you didn't remove the recovery method, an attacker may be trying to access your account. Change your account password and set a new recovery method immediately in Settings."
        },
-        "reset_all_button": "Forgotten or lost all recovery methods? <a>Reset all</a>",


Note to future archaeologists: the reference to this string was removed in an earlier commit in this PR

Sorry, I tried to keep the string removals in the same commits as where the code stops using it, but I guess I missed this on.

src/components/structures/auth/SetupEncryptionBody.tsx

richvdh · 2025-08-28T12:08:21Z

src/i18n/strings/en_EN.json

-            "verify_using_device": "Verify with another device",
-            "verify_using_key": "Verify with Recovery Key",
+            "verify_using_device": "Use another device",
+            "verify_using_key": "Use recovery key",


these changes probably need to happen in localazy

richvdh · 2025-08-28T12:15:23Z

src/components/views/dialogs/security/SetupEncryptionDialog.tsx


+export default class SetupEncryptionDialog extends React.Component<IProps> {


if you're basically rewriting a component, consider taking the opportunity to switch to the functional style. Not a blocker here though

src/components/views/auth/AuthPage.tsx

richvdh · 2025-08-28T12:28:34Z

res/css/views/auth/_AuthPage.pcss

@@ -29,4 +29,13 @@ Please see LICENSE files in the repository root for full details.
    @media only screen and (max-width: 480px) {
        margin-top: 0;
    }
+
+    /* Extra class to indicate that the component is providing its own blurring,
+     * so we don't apply ours */


... nor a box-shadow, apparently.

What exactly does "blurring" mean, in this context?

This whole mechanism of overriding some of the inline styles by addition of another class feels a bit opaque. (Blurry, if you will.) It feels like maybe we should instead have a class which enables the blurring, and which AuthPage enables by default, but which you can disable via a boolean property on AuthPage (instead of the modalClasses prop). WDYT?

This whole mechanism of overriding some of the inline styles by addition of another class feels a bit opaque. (Blurry, if you will.) It feels like maybe we should instead have a class which enables the blurring, and which AuthPage enables by default, but which you can disable via a boolean property on AuthPage (instead of the modalClasses prop). WDYT?

Done. I had originally used the class property to make it more flexible, but the boolean is clearer what the intent is.

richvdh · 2025-08-28T12:52:07Z

playwright/e2e/crypto/dehydration.spec.ts

-        await page.getByRole("button", { name: "Proceed with reset" }).click();
+        await page.getByRole("button", { name: "Can't confirm?" }).click();


just for future reference: it's much nicer to review changes where the author has at least aimed for the tests to pass after each commit (so the tests and app are updated in sync)

Co-authored-by: Richard van der Hoff <[email protected]>

uhoreg added the T-Feature Request to add a new feature which does not exist right now label Aug 19, 2025

github-actions bot deployed to Netlify August 19, 2025 18:51 View deployment

uhoreg added T-Enhancement and removed T-Feature Request to add a new feature which does not exist right now labels Aug 19, 2025

github-actions bot deployed to EndToEndTests August 19, 2025 19:12 View deployment

github-actions bot deployed to Netlify August 19, 2025 19:19 View deployment

github-actions bot deployed to EndToEndTests August 19, 2025 19:39 View deployment

github-actions bot deployed to Netlify August 19, 2025 20:03 View deployment

github-actions bot deployed to EndToEndTests August 19, 2025 20:17 View deployment

uhoreg force-pushed the verify_device_redesign branch from 65a2a47 to 758647f Compare August 19, 2025 21:08

github-actions bot deployed to Netlify August 19, 2025 21:11 View deployment

github-actions bot deployed to EndToEndTests August 19, 2025 21:24 View deployment

uhoreg commented Aug 20, 2025

View reviewed changes

uhoreg marked this pull request as ready for review August 20, 2025 15:49

uhoreg requested review from a team as code owners August 20, 2025 15:49

uhoreg requested review from t3chguy, MidhunSureshR and richvdh August 20, 2025 15:49

t3chguy reviewed Aug 20, 2025

View reviewed changes

src/components/views/settings/encryption/ResetIdentityBody.tsx Outdated Show resolved Hide resolved

t3chguy reviewed Aug 20, 2025

View reviewed changes

github-actions bot deployed to Netlify August 20, 2025 17:47 View deployment

github-actions bot deployed to EndToEndTests August 20, 2025 17:57 View deployment

uhoreg requested a review from t3chguy August 20, 2025 19:20

t3chguy approved these changes Aug 21, 2025

View reviewed changes

richvdh reviewed Aug 22, 2025

View reviewed changes

uhoreg added 2 commits August 27, 2025 17:16

add variant of ResetIdentityBody for when the user has no verif. methods

0fdc4d2

no longer distinguish between the using having a passphrase or not

63dce71

uhoreg added 7 commits August 27, 2025 18:11

use vertical stack of buttons via EncryptionCard

b101488

and update wording

swap logic order to match rendering order

91250c4

use the same dialog when no verification options available

9d9060f

make it agree with the design more

a7a34a0

allow signing out on initial login

d145fa4

apply styling changes and remove duplicate elements

fcf3b1f

fix and add tests

c032ed2

uhoreg force-pushed the verify_device_redesign branch from 4d6c595 to c032ed2 Compare August 27, 2025 23:46

github-actions bot deployed to Netlify August 27, 2025 23:49 View deployment

github-actions bot deployed to EndToEndTests August 27, 2025 23:59 View deployment

Merge branch 'develop' into verify_device_redesign

4eb27b3

github-actions bot deployed to Netlify August 28, 2025 00:06 View deployment

add missing snapshot

a2e32aa

github-actions bot deployed to Netlify August 28, 2025 00:20 View deployment

github-actions bot deployed to EndToEndTests August 28, 2025 00:29 View deployment

uhoreg requested a review from richvdh August 28, 2025 00:53

richvdh reviewed Aug 28, 2025

View reviewed changes

Apply suggestions from code review

52a474c

Co-authored-by: Richard van der Hoff <[email protected]>

github-actions bot deployed to Netlify September 4, 2025 04:27 View deployment

github-actions bot deployed to EndToEndTests September 4, 2025 04:38 View deployment

github-actions bot deployed to Netlify September 4, 2025 05:05 View deployment

github-actions bot deployed to EndToEndTests September 4, 2025 05:15 View deployment

uhoreg added 3 commits September 4, 2025 09:56

Merge branch 'develop' into verify_device_redesign

2ab8cad

use a boolean property to disable blurring instead of adding a class

1d34e2a

change string identifiers

8212149

uhoreg force-pushed the verify_device_redesign branch from 0f34b5b to 8212149 Compare September 4, 2025 13:58

github-actions bot deployed to Netlify September 4, 2025 14:02 View deployment

github-actions bot deployed to EndToEndTests September 4, 2025 14:14 View deployment

uhoreg requested a review from richvdh September 4, 2025 14:46


		export default class SetupEncryptionDialog extends React.Component<IProps> {

		await page.getByRole("button", { name: "Proceed with reset" }).click();
		await page.getByRole("button", { name: "Can't confirm?" }).click();

Uh oh!

"Verify this device" redesign #30596

Are you sure you want to change the base?

"Verify this device" redesign #30596

Conversation

uhoreg commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dosubot bot commented Aug 20, 2025

Uh oh!

Uh oh!

t3chguy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uhoreg commented Aug 28, 2025

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uhoreg commented Aug 19, 2025 •

edited

Loading