Make identity reset consistent with EX #29701
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andybalaam
added
T-Defect
T-Feature
andybalaam
added
T-Enhancement
and removed
T-Feature
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
5ca414f to
fec5e88
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
2 times, most recently
from
0edccfe to
dab78ad
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
dab78ad to
8f0073e
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
8f0073e to
f3035e1
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
f3035e1 to
94c7056
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
94c7056 to
4fdc53e
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
4fdc53e to
fff5732
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
fff5732 to
eccb05c
Compare
andybalaam
force-pushed
the
andybalaam/reset-encryption-redesign2
branch
from
eccb05c to
4abde33
Compare
dbkr
reviewed
Apr 15, 2025
| /** | ||
| * Called when the identity is reset (before onFinished is called). | ||
| */ | ||
| onReset: MouseEventHandler<HTMLButtonElement>; |
There was a problem hiding this comment.
It seems a bit weird for 'onReset' to take a mouse event: it's hard to see how the original click would be relevant.
| }, | ||
| onFinished: () => { | ||
| // The user cancelled the reset dialog or click away - go back a step | ||
| this.onResetBackClick(); |
There was a problem hiding this comment.
onFinished is called in either case isn't it? Is there a risk this will confuse things by clicking back after the dialog is done (also I don't love that we are calling click handlers for reasons other than when a click happened).
| onReset: () => { | ||
| // The user completed the reset process - close this dialog | ||
| this.props.onFinished(); | ||
| this.onDoneClick(); |
There was a problem hiding this comment.
As per below on calling click handlers rather than them just handling clicks.
richvdh
reviewed
Apr 15, 2025
Comment on lines
45
to
55
| // Wrappers for ResetIdentityBody's callbacks so that onFinish gets called | ||
| // whenever the reset is done, whether by completing successfully, or by | ||
| // being cancelled | ||
| const onResetWrapper: MouseEventHandler<HTMLButtonElement> = (...args) => { | ||
| onFinished(); | ||
| onResetFinished(...args); | ||
| }; | ||
| const onCancelWrapper: () => void = () => { | ||
| onFinished(); | ||
| onCancelClick(); | ||
| }; |
There was a problem hiding this comment.
General feedback: this seems to immediately change in the next commit, so reviewing it is trickier than it needs to be
richvdh
requested changes
Apr 15, 2025
Comment on lines
+78
to
-83
| } else if (phase === Phase.Busy || phase === Phase.ConfirmReset) { | ||
| icon = <span className="mx_CompleteSecurity_headerIcon mx_E2EIcon_warning" />; | ||
| title = _t("encryption|verification|after_new_login|verify_this_device"); | ||
| } else if (phase === Phase.ConfirmReset) { | ||
| icon = <span className="mx_CompleteSecurity_headerIcon mx_E2EIcon_warning" />; | ||
| title = _t("encryption|verification|after_new_login|reset_confirmation"); |
There was a problem hiding this comment.
I'm a bit confused about what's going on here. Don't we still need a confirmation step? why do we show "verify this device" (?) when we're in the "ConfirmReset" phase?
| * "forgot" is shown when the user has just forgotten their passphrase. | ||
| * "forgot" is shown when the user chose 'Forgot recovery key?' during `SetupEncryptionToast`. | ||
| * | ||
| * "confirm" is shown when the user chose 'Reset all' during `SetupEncryptionBody`. |
There was a problem hiding this comment.
it would be good to detail how the behaviour is different, rather than when it is called.
Comment on lines
-249
to
+256
| } else if (phase === Phase.ConfirmReset) { | ||
| return ( | ||
| <div> | ||
| <p>{_t("encryption|verification|verify_reset_warning_1")}</p> | ||
| <p>{_t("encryption|verification|verify_reset_warning_2")}</p> | ||
|
|
||
| <div className="mx_CompleteSecurity_actionRow"> | ||
| <AccessibleButton kind="danger_outline" onClick={this.onResetConfirmClick}> | ||
| {_t("encryption|verification|reset_proceed_prompt")} | ||
| </AccessibleButton> | ||
| <AccessibleButton kind="primary" onClick={this.onResetBackClick}> | ||
| {_t("action|go_back")} | ||
| </AccessibleButton> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } else if (phase === Phase.Busy || phase === Phase.Loading) { | ||
| } else if (phase === Phase.Busy || phase === Phase.Loading || phase == Phase.ConfirmReset) { |
There was a problem hiding this comment.
again, bit confused about why it is safe to get rid of the confirm step, and why we now show a spinner for "ConfirmReset".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #29227
This adds
IdentityResetDialog, which wrapsIdentityResetBodyin a dialog, and launches it fromSetupEncryptionBody, instead of doing the reset directly there.This is mostly work by @uhoreg , rebased and refactored a bit here.
I recommend reviewing commit-by-commit.